Builds: add tests for delete_old_build_objects and fix KeyError in _delete_builds

readthedocs/builds/tasks.py

@@ -652,7 +652,7 @@ def _delete_builds(builds, start: int, end: int) -> int:
 
     _, deleted = delete_in_batches(builds, start=start, end=end)
     remove_build_commands_storage_paths(paths_to_delete)
-    return deleted["builds.Build"]
+    return deleted.get("builds.Build", 0)
 
 
 @app.task(queue="web")

readthedocs/builds/tests/test_tasks.py

@@ -11,6 +11,7 @@
 from readthedocs.builds.constants import (
     BRANCH,
     BUILD_STATE_FINISHED,
+    BUILD_STATE_TRIGGERED,
     EXTERNAL,
     EXTERNAL_VERSION_STATE_CLOSED,
     EXTERNAL_VERSION_STATE_OPEN,
@@ -22,6 +23,7 @@
     archive_builds_task,
     check_and_disable_project_for_consecutive_failed_builds,
     delete_closed_external_versions,
+    delete_old_build_objects,
     post_build_overview,
     remove_orphan_build_config,
 )
@@ -268,6 +270,185 @@ def test_remove_orphan_build_config_no_orphans(self):
         assert BuildConfig.objects.filter(pk=config_2.pk).exists()
 
 
+class TestDeleteOldBuildObjects(TestCase):
+    def _create_old_build(self, project, version, days_old, state=BUILD_STATE_FINISHED, cold_storage=False):
+        """Create a build with a specific age."""
+        build = get(
+            Build,
+            project=project,
+            version=version,
+            state=state,
+            cold_storage=cold_storage,
+        )
+        # Override the auto-set date with a specific old date.
+        Build.objects.filter(pk=build.pk).update(
+            date=timezone.now() - timezone.timedelta(days=days_old)
+        )
+        return build
+
+    def test_old_builds_beyond_keep_recent_are_deleted(self):
+        """Builds older than `days` and beyond `keep_recent` per version are deleted."""
+        project = get(Project)
+        version = project.versions.get(slug=LATEST)
+
+        # Create 5 old builds with different ages so ordering is deterministic.
+        old_builds = [self._create_old_build(project, version, days_old=400 + i) for i in range(5)]
+
+        assert Build.objects.filter(version=version).count() == 5
+
+        # Keep 2 recent builds per version, so 3 should be deleted.
+        delete_old_build_objects(days=365, keep_recent=2, start=0)
+
+        assert Build.objects.filter(version=version).count() == 2
+        # The 2 most-recently-dated builds (those with the smallest days_old) should be kept.
+        remaining_pks = set(
+            Build.objects.filter(version=version).values_list("pk", flat=True)
+        )
+        assert remaining_pks == {old_builds[0].pk, old_builds[1].pk}
+
+    def test_recent_builds_are_not_deleted(self):
+        """Builds newer than `days` threshold are never deleted."""
+        project = get(Project)
+        version = project.versions.get(slug=LATEST)
+
+        # Create builds that are recent (within 'days' cutoff).
+        for _ in range(5):
+            get(Build, project=project, version=version, state=BUILD_STATE_FINISHED)
+
+        assert Build.objects.filter(version=version).count() == 5
+
+        delete_old_build_objects(days=365, keep_recent=2, start=0)
+
+        # None should be deleted since they are all recent.
+        assert Build.objects.filter(version=version).count() == 5
+
+    def test_builds_within_keep_recent_are_not_deleted(self):
+        """The most recent `keep_recent` builds per version are never deleted, even if old."""
+        project = get(Project)
+        version = project.versions.get(slug=LATEST)
+
+        # Create 3 old builds.
+        for _ in range(3):
+            self._create_old_build(project, version, days_old=400)
+
+        assert Build.objects.filter(version=version).count() == 3
+
+        # keep_recent=5 means all 3 should be preserved.
+        delete_old_build_objects(days=365, keep_recent=5, start=0)
+
+        assert Build.objects.filter(version=version).count() == 3
+
+    def test_non_final_state_builds_not_deleted(self):
+        """Builds in non-final states (e.g. triggered) are never deleted."""
+        project = get(Project)
+        version = project.versions.get(slug=LATEST)
+
+        # Create old builds in a non-final state.
+        for _ in range(5):
+            self._create_old_build(project, version, days_old=400, state=BUILD_STATE_TRIGGERED)
+
+        assert Build.objects.filter(version=version).count() == 5
+
+        delete_old_build_objects(days=365, keep_recent=0, start=0)
+
+        # Non-final builds should not be deleted.
+        assert Build.objects.filter(version=version).count() == 5
+
+    def test_versionless_builds_deleted(self):
+        """Old builds without a version are also deleted, beyond `keep_recent` per project."""
+        project = get(Project)
+
+        # Create 5 old versionless builds with different ages so ordering is deterministic.
+        old_builds = [
+            self._create_old_build(project, version=None, days_old=400 + i) for i in range(5)
+        ]
+
+        assert Build.objects.filter(project=project, version=None).count() == 5
+
+        delete_old_build_objects(days=365, keep_recent=2, start=0)
+
+        # 3 should be deleted (keeping only 2 most recent).
+        assert Build.objects.filter(project=project, version=None).count() == 2
+        remaining_pks = set(
+            Build.objects.filter(project=project, version=None).values_list("pk", flat=True)
+        )
+        assert remaining_pks == {old_builds[0].pk, old_builds[1].pk}
+
+    def test_limit_stops_deletion(self):
+        """Deletion stops once `limit` builds have been deleted."""
+        project = get(Project)
+        version = project.versions.get(slug=LATEST)
+
+        for _ in range(10):
+            self._create_old_build(project, version, days_old=400)
+
+        assert Build.objects.filter(version=version).count() == 10
+
+        # With limit=3 and keep_recent=0, only 3 builds should be deleted.
+        delete_old_build_objects(days=365, keep_recent=0, limit=3, start=0)
+
+        assert Build.objects.filter(version=version).count() == 7
+
+    def test_max_projects_limits_projects_processed(self):
+        """Only `max_projects` projects are processed per execution."""
+        project1 = get(Project)
+        project2 = get(Project)
+        version1 = project1.versions.get(slug=LATEST)
+        version2 = project2.versions.get(slug=LATEST)
+
+        for _ in range(5):
+            self._create_old_build(project1, version1, days_old=400)
+        for _ in range(5):
+            self._create_old_build(project2, version2, days_old=400)
+
+        # Only process 1 project at a time (max_projects=1).
+        delete_old_build_objects(days=365, keep_recent=0, max_projects=1, start=0)
+
+        total_remaining = (
+            Build.objects.filter(version=version1).count()
+            + Build.objects.filter(version=version2).count()
+        )
+        # Only one project's builds were processed.
+        assert total_remaining == 5
+
+    def test_keeps_builds_per_version_independently(self):
+        """keep_recent applies independently to each version."""
+        project = get(Project)
+        version1 = project.versions.get(slug=LATEST)
+        version2 = get(Version, project=project, slug="stable")
+
+        for _ in range(5):
+            self._create_old_build(project, version1, days_old=400)
+        for _ in range(5):
+            self._create_old_build(project, version2, days_old=400)
+
+        delete_old_build_objects(days=365, keep_recent=2, start=0)
+
+        # Each version should retain 2 builds.
+        assert Build.objects.filter(version=version1).count() == 2
+        assert Build.objects.filter(version=version2).count() == 2
+
+    @mock.patch("readthedocs.builds.tasks.build_commands_storage")
+    def test_cold_storage_paths_are_deleted(self, build_commands_storage):
+        """Cold storage paths of deleted builds are removed."""
+        project = get(Project)
+        version = project.versions.get(slug=LATEST)
+
+        # Create old builds in cold storage.
+        for _ in range(3):
+            self._create_old_build(project, version, days_old=400, cold_storage=True)
+        # Also create 2 recent builds in cold storage (should NOT be deleted).
+        for _ in range(2):
+            get(Build, project=project, version=version, state=BUILD_STATE_FINISHED, cold_storage=True)
+
+        delete_old_build_objects(days=365, keep_recent=0, start=0)
+
+        # All 3 old builds should have been deleted.
+        assert Build.objects.filter(version=version).count() == 2
+        # Storage delete should have been called for each deleted build's path.
+        assert build_commands_storage.delete.call_count == 3
+
+
 @override_settings(
     PRODUCTION_DOMAIN="readthedocs.org",
     PUBLIC_DOMAIN="readthedocs.io",