Fix: Retry builds were not being cancelled properly (#12615)

readthedocs/builds/tests/test_cancel_build.py

@@ -0,0 +1,86 @@
+from unittest import mock
+
+from django.contrib.contenttypes.models import ContentType
+from django.test import TestCase
+
+from readthedocs.builds.constants import (
+    BUILD_STATE_BUILDING,
+    BUILD_STATE_CANCELLED,
+    BUILD_STATE_TRIGGERED,
+)
+from readthedocs.builds.models import Build, Version
+from readthedocs.core.utils import cancel_build
+from readthedocs.notifications.models import Notification
+from readthedocs.projects.models import Project
+
+
+class CancelBuildTests(TestCase):
+    def setUp(self):
+        self.project = Project.objects.create(
+            name="Cancel build project",
+            slug="cancel-build-project",
+            repo="https://example.com/repo.git",
+        )
+        self.version = self.project.versions.first()
+        if self.version is None:
+            self.version = Version.objects.create(
+                project=self.project,
+                identifier="main",
+                verbose_name="main",
+                slug="main",
+                active=True,
+            )
+
+    @mock.patch("readthedocs.core.utils.app")
+    def test_cancel_running_or_retry_build_sets_cancelled_state(self, app):
+        build = Build.objects.create(
+            project=self.project,
+            version=self.version,
+            task_id="1234",
+            state=BUILD_STATE_BUILDING,
+            success=True,
+        )
+
+        cancel_build(build)
+
+        build.refresh_from_db()
+        assert build.state == BUILD_STATE_CANCELLED
+        assert build.success is False
+        assert build.length is not None
+
+        # A notification was created for this build (match by Generic FK)
+        build_ct = ContentType.objects.get_for_model(build)
+        assert Notification.objects.filter(
+            attached_to_id=build.pk,
+            attached_to_content_type=build_ct,
+        ).exists()
+
+        # Running build: terminate=True
+        app.control.revoke.assert_called_once_with(
+            build.task_id,
+            signal=mock.ANY,
+            terminate=True,
+        )
+
+    @mock.patch("readthedocs.core.utils.app")
+    def test_cancel_triggered_build_sets_cancelled_state(self, app):
+        build = Build.objects.create(
+            project=self.project,
+            version=self.version,
+            task_id="1234",
+            state=BUILD_STATE_TRIGGERED,
+            success=True,
+        )
+
+        cancel_build(build)
+
+        build.refresh_from_db()
+        assert build.state == BUILD_STATE_CANCELLED
+        assert build.success is False
+
+        # Triggered (not yet running): terminate=False
+        app.control.revoke.assert_called_once_with(
+            build.task_id,
+            signal=mock.ANY,
+            terminate=False,
+        )

readthedocs/builds/tests/test_trigger_build.py

@@ -53,7 +53,7 @@ def test_cancel_old_running_build(self, update_docs_task, cancel_build, state):
         triggered_build = Build.objects.first()
         builds_count_after = Build.objects.count()
 
-        cancel_build.assert_called_once_with(build)
+        cancel_build.assert_called_once_with(build, update_state=False)
         assert result == (mock.ANY, triggered_build)
         assert builds_count_before == builds_count_after - 1
         assert update_docs_task.signature.called

readthedocs/builds/tests/test_views.py

@@ -42,7 +42,7 @@ def test_cancel_running_build(self, app):
             self.build.task_id, signal=mock.ANY, terminate=True
         )
         self.build.refresh_from_db()
-        self.assertEqual(self.build.state, BUILD_STATE_INSTALLING)
+        self.assertEqual(self.build.state, BUILD_STATE_CANCELLED)
 
     def test_cancel_triggered_build(self, app):
         self.build.state = BUILD_STATE_TRIGGERED

readthedocs/core/utils/__init__.py

@@ -134,7 +134,10 @@ def prepare_build(
     # If there are builds triggered/running for this particular project and version,
     # we cancel all of them and trigger a new one for the latest commit received.
     for running_build in running_builds:
-        cancel_build(running_build)
+        # Here we only revoke the Celery task and let the build's failure handler
+        # update the DB state. This way, these builds still count towards
+        # concurrency limits until the worker finishes handling them.
+        cancel_build(running_build, update_state=False)
 
     # Start the build in X minutes and mark it as limited
     limit_reached, _, max_concurrent_builds = Build.objects.concurrent(project)
@@ -237,7 +240,7 @@ def trigger_build(project, version=None, commit=None):
     return task, build
 
 
-def cancel_build(build):
+def cancel_build(build, *, update_state=True):
     """
     Cancel a triggered/running build.
 
@@ -247,38 +250,51 @@ def cancel_build(build):
         Update the build status and tells Celery to revoke this task.
         Workers will know about this and will discard it.
     - Running:
-        Communicate Celery to force the termination of the current build
-        and rely on the worker to update the build's status.
+        Communicate Celery to force the termination of the current build.
+
+    :param update_state:
+        When True (default), update the Build object immediately so the UI
+        reflects the cancellation. When False, only revoke the Celery task
+        and let the caller decide how/when to update the Build object.
     """
+    # If the build is already in a final state, there is nothing to do.
+    if build.state in BUILD_FINAL_STATES:
+        log.info(
+            "Cancel called on build in final state.",
+            project_slug=build.project.slug,
+            version_slug=getattr(build.version, "slug", None),
+            build_id=build.pk,
+            build_state=build.state,
+        )
+        return
+
     # NOTE: `terminate=True` is required for the child to attend our call
     # immediately when it's running the build. Otherwise, it finishes the
     # task. However, to revoke a task that has not started yet, we don't
     # need it.
-    if build.state == BUILD_STATE_TRIGGERED:
-        # Since the task won't be executed at all, we need to update the
-        # Build object here.
-        terminate = False
+    terminate = build.state != BUILD_STATE_TRIGGERED
+
+    if update_state:
+        # Move the build out of the "active" states immediately, regardless of
+        # whether the task had started or is still pending/retrying.
         build.state = BUILD_STATE_CANCELLED
         build.success = False
 
-        # Add a notification for this build
         Notification.objects.add(
             message_id=BuildCancelled.CANCELLED_BY_USER,
             attached_to=build,
             dismissable=False,
         )
 
-        build.length = 0
+        if build.length is None:
+            build.length = 0
+
         build.save()
-    else:
-        # In this case, we left the update of the Build object to the task
-        # itself to be executed in the `on_failure` handler.
-        terminate = True
 
     log.warning(
         "Canceling build.",
         project_slug=build.project.slug,
-        version_slug=build.version.slug,
+        version_slug=getattr(build.version, "slug", None),
         build_id=build.pk,
         build_task_id=build.task_id,
         terminate=terminate,