From 547ae5a2676a6414eefd312321ec57bbb1013146 Mon Sep 17 00:00:00 2001 From: sahil bhalekar <129488267+SahiRB1104@users.noreply.github.com> Date: Sun, 7 Dec 2025 15:30:59 +0530 Subject: [PATCH 1/9] Fix Build marked as retry isn't canceled correctly( #12615 ) + add regression tests --- readthedocs/builds/tests/test_cancel_build.py | 90 +++++++++++++++++++ readthedocs/core/utils/__init__.py | 55 +++++++----- 2 files changed, 124 insertions(+), 21 deletions(-) create mode 100644 readthedocs/builds/tests/test_cancel_build.py diff --git a/readthedocs/builds/tests/test_cancel_build.py b/readthedocs/builds/tests/test_cancel_build.py new file mode 100644 index 00000000000..739a1290f69 --- /dev/null +++ b/readthedocs/builds/tests/test_cancel_build.py @@ -0,0 +1,90 @@ +from unittest import mock + +from django.test import TestCase + +from readthedocs.builds.constants import ( + BUILD_STATE_TRIGGERED, + BUILD_STATE_CANCELLED, + BUILD_STATE_BUILDING, + BUILD_FINAL_STATES, +) +from readthedocs.builds.models import Build, Version +from readthedocs.core.utils import cancel_build +from readthedocs.projects.models import Project + + +class CancelBuildTests(TestCase): + + def setUp(self): + self.project = Project.objects.create( + name="Test project", + slug="test-project", + repo="https://example.com/repo.git", + ) + self.version = Version.objects.create( + project=self.project, + slug="latest", + verbose_name="latest", + type="branch", + identifier="main", + active=True, + ) + + @mock.patch("readthedocs.core.utils.app.control.revoke") + def test_cancel_triggered_build_sets_cancelled_state(self, revoke): + build = Build.objects.create( + project=self.project, + version=self.version, + state=BUILD_STATE_TRIGGERED, + success=True, + task_id="dummy-task-id", + ) + + cancel_build(build) + build.refresh_from_db() + + # Build must be cancelled and considered final + self.assertEqual(build.state, BUILD_STATE_CANCELLED) + self.assertIn(build.state, BUILD_FINAL_STATES) + + # Build must no longer be active + active_ids = Build.objects.exclude( + state__in=BUILD_FINAL_STATES, + ).values_list("id", flat=True) + self.assertNotIn(build.id, active_ids) + + # Celery revoke should be called with terminate=False + revoke.assert_called_once_with( + "dummy-task-id", + signal="SIGINT", + terminate=False + ) + + @mock.patch("readthedocs.core.utils.app.control.revoke") + def test_cancel_running_or_retry_build_sets_cancelled_state(self, revoke): + # Use BUILD_STATE_BUILDING to simulate a running build + build = Build.objects.create( + project=self.project, + version=self.version, + state=BUILD_STATE_BUILDING, + success=True, + task_id="dummy-task-id-2", + ) + + cancel_build(build) + build.refresh_from_db() + + self.assertEqual(build.state, BUILD_STATE_CANCELLED) + self.assertIn(build.state, BUILD_FINAL_STATES) + + active_ids = Build.objects.exclude( + state__in=BUILD_FINAL_STATES, + ).values_list("id", flat=True) + self.assertNotIn(build.id, active_ids) + + # Running builds should be terminated=True + revoke.assert_called_once_with( + "dummy-task-id-2", + signal="SIGINT", + terminate=True + ) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 78b69c2c943..3f0c3539ec0 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -247,38 +247,50 @@ 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. + We also update the Build object here so the UI reflects the + cancellation immediately, instead of relying solely on the task's + failure handler (which doesn't always run for retried builds). """ + # 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 - 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, - ) + terminate = build.state != BUILD_STATE_TRIGGERED + + # 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 (avoid duplicate notifications if you want, + # but it's usually fine). + Notification.objects.add( + message_id=BuildCancelled.CANCELLED_BY_USER, + attached_to=build, + dismissable=False, + ) + 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 + + build.save() 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, @@ -286,6 +298,7 @@ def cancel_build(build): app.control.revoke(build.task_id, signal="SIGINT", terminate=terminate) + def send_email_from_object(email: EmailMultiAlternatives | EmailMessage): """Given an email object, send it using our send_email_task task.""" from readthedocs.core.tasks import send_email_task From e1dba9e1b2758e1631f17fa3e96484051d4f181b Mon Sep 17 00:00:00 2001 From: sahil bhalekar <129488267+SahiRB1104@users.noreply.github.com> Date: Sun, 7 Dec 2025 15:47:33 +0530 Subject: [PATCH 2/9] Fix formatting reported by pre-commit --- readthedocs/core/utils/__init__.py | 1 - 1 file changed, 1 deletion(-) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 3f0c3539ec0..e6d11f44df2 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -298,7 +298,6 @@ def cancel_build(build): app.control.revoke(build.task_id, signal="SIGINT", terminate=terminate) - def send_email_from_object(email: EmailMultiAlternatives | EmailMessage): """Given an email object, send it using our send_email_task task.""" from readthedocs.core.tasks import send_email_task From 2f391e28202c95c479b99553db034f7db9f4d2c1 Mon Sep 17 00:00:00 2001 From: sahil bhalekar <129488267+SahiRB1104@users.noreply.github.com> Date: Sun, 7 Dec 2025 17:15:53 +0530 Subject: [PATCH 3/9] Fix cancel build retry logic and add regression tests --- readthedocs/builds/tests/test_cancel_build.py | 96 +++++++++---------- readthedocs/builds/tests/test_views.py | 2 +- readthedocs/core/utils/__init__.py | 46 +++++---- 3 files changed, 73 insertions(+), 71 deletions(-) diff --git a/readthedocs/builds/tests/test_cancel_build.py b/readthedocs/builds/tests/test_cancel_build.py index 739a1290f69..f01f3c3982c 100644 --- a/readthedocs/builds/tests/test_cancel_build.py +++ b/readthedocs/builds/tests/test_cancel_build.py @@ -1,90 +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_TRIGGERED, - BUILD_STATE_CANCELLED, BUILD_STATE_BUILDING, - BUILD_FINAL_STATES, + 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="Test project", - slug="test-project", + name="Cancel build project", + slug="cancel-build-project", repo="https://example.com/repo.git", ) - self.version = Version.objects.create( - project=self.project, - slug="latest", - verbose_name="latest", - type="branch", - identifier="main", - active=True, - ) + 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.control.revoke") - def test_cancel_triggered_build_sets_cancelled_state(self, revoke): + @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, - state=BUILD_STATE_TRIGGERED, + task_id="1234", + state=BUILD_STATE_BUILDING, success=True, - task_id="dummy-task-id", ) cancel_build(build) - build.refresh_from_db() - # Build must be cancelled and considered final - self.assertEqual(build.state, BUILD_STATE_CANCELLED) - self.assertIn(build.state, BUILD_FINAL_STATES) + build.refresh_from_db() + assert build.state == BUILD_STATE_CANCELLED + assert build.success is False + assert build.length is not None - # Build must no longer be active - active_ids = Build.objects.exclude( - state__in=BUILD_FINAL_STATES, - ).values_list("id", flat=True) - self.assertNotIn(build.id, active_ids) + # 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() - # Celery revoke should be called with terminate=False - revoke.assert_called_once_with( - "dummy-task-id", - signal="SIGINT", - terminate=False + # 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.control.revoke") - def test_cancel_running_or_retry_build_sets_cancelled_state(self, revoke): - # Use BUILD_STATE_BUILDING to simulate a running build + @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, - state=BUILD_STATE_BUILDING, + task_id="1234", + state=BUILD_STATE_TRIGGERED, success=True, - task_id="dummy-task-id-2", ) cancel_build(build) - build.refresh_from_db() - self.assertEqual(build.state, BUILD_STATE_CANCELLED) - self.assertIn(build.state, BUILD_FINAL_STATES) - - active_ids = Build.objects.exclude( - state__in=BUILD_FINAL_STATES, - ).values_list("id", flat=True) - self.assertNotIn(build.id, active_ids) + build.refresh_from_db() + assert build.state == BUILD_STATE_CANCELLED + assert build.success is False - # Running builds should be terminated=True - revoke.assert_called_once_with( - "dummy-task-id-2", - signal="SIGINT", - terminate=True + # Triggered (not yet running): terminate=False + app.control.revoke.assert_called_once_with( + build.task_id, + signal=mock.ANY, + terminate=False, ) diff --git a/readthedocs/builds/tests/test_views.py b/readthedocs/builds/tests/test_views.py index f1c64abde0a..1c1fc633b33 100644 --- a/readthedocs/builds/tests/test_views.py +++ b/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 diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index e6d11f44df2..976d0fdb687 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -134,7 +134,12 @@ 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 +242,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. @@ -248,9 +253,11 @@ def cancel_build(build): Workers will know about this and will discard it. - Running: Communicate Celery to force the termination of the current build. - We also update the Build object here so the UI reflects the - cancellation immediately, instead of relying solely on the task's - failure handler (which doesn't always run for retried builds). + + :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: @@ -269,23 +276,22 @@ def cancel_build(build): # need it. terminate = build.state != BUILD_STATE_TRIGGERED - # 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 (avoid duplicate notifications if you want, - # but it's usually fine). - Notification.objects.add( - message_id=BuildCancelled.CANCELLED_BY_USER, - attached_to=build, - dismissable=False, - ) + 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 + + Notification.objects.add( + message_id=BuildCancelled.CANCELLED_BY_USER, + attached_to=build, + dismissable=False, + ) - if build.length is None: - build.length = 0 + if build.length is None: + build.length = 0 - build.save() + build.save() log.warning( "Canceling build.", From defc6987696ce403d17ec57a3a311c88c2498077 Mon Sep 17 00:00:00 2001 From: sahil bhalekar <129488267+SahiRB1104@users.noreply.github.com> Date: Sun, 7 Dec 2025 17:49:36 +0530 Subject: [PATCH 4/9] Fix cancel_build positional signature for compatibility --- readthedocs/core/utils/__init__.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 976d0fdb687..9c23f6f14a8 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -137,8 +137,7 @@ def prepare_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) - + cancel_build(running_build,False) # Start the build in X minutes and mark it as limited From 3d3a73a643462d2649a7747aa864d20837306d81 Mon Sep 17 00:00:00 2001 From: sahil bhalekar <129488267+SahiRB1104@users.noreply.github.com> Date: Sun, 7 Dec 2025 17:53:18 +0530 Subject: [PATCH 5/9] Fix formatting after ruff-format --- readthedocs/core/utils/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 9c23f6f14a8..0c8a595989a 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -137,7 +137,7 @@ def prepare_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,False) + cancel_build(running_build, False) # Start the build in X minutes and mark it as limited From 0d031428d06a326d4383c0b139f60cef28e62af2 Mon Sep 17 00:00:00 2001 From: sahil bhalekar <129488267+SahiRB1104@users.noreply.github.com> Date: Sun, 7 Dec 2025 18:07:19 +0530 Subject: [PATCH 6/9] Adjust test to expect update_state=False for auto cancel --- readthedocs/builds/tests/test_trigger_build.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/builds/tests/test_trigger_build.py b/readthedocs/builds/tests/test_trigger_build.py index 00b83420b6f..bf01150ec65 100644 --- a/readthedocs/builds/tests/test_trigger_build.py +++ b/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 From 0c304e47cec120958a0dbc592ed1ee14e8b99914 Mon Sep 17 00:00:00 2001 From: sahil bhalekar <129488267+SahiRB1104@users.noreply.github.com> Date: Sun, 7 Dec 2025 18:18:32 +0530 Subject: [PATCH 7/9] Fix cancel_build positional argument in tests --- readthedocs/builds/tests/test_trigger_build.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/builds/tests/test_trigger_build.py b/readthedocs/builds/tests/test_trigger_build.py index bf01150ec65..89790c97f8a 100644 --- a/readthedocs/builds/tests/test_trigger_build.py +++ b/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, update_state=False) + cancel_build.assert_called_once_with(build, False) assert result == (mock.ANY, triggered_build) assert builds_count_before == builds_count_after - 1 assert update_docs_task.signature.called From 6f740cd6d6cf117682b78bd31db0b3470eb6822d Mon Sep 17 00:00:00 2001 From: sahil bhalekar <129488267+SahiRB1104@users.noreply.github.com> Date: Sun, 7 Dec 2025 18:22:41 +0530 Subject: [PATCH 8/9] Apply ruff-format auto-fix --- readthedocs/core/utils/__init__.py | 1 - 1 file changed, 1 deletion(-) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 0c8a595989a..7cfd7d3f2a7 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -139,7 +139,6 @@ def prepare_build( # concurrency limits until the worker finishes handling them. cancel_build(running_build, False) - # Start the build in X minutes and mark it as limited limit_reached, _, max_concurrent_builds = Build.objects.concurrent(project) if limit_reached: From 72aac523046dbbd0bd3277e1aaae1803714ea621 Mon Sep 17 00:00:00 2001 From: sahil bhalekar <129488267+SahiRB1104@users.noreply.github.com> Date: Sun, 7 Dec 2025 19:09:51 +0530 Subject: [PATCH 9/9] fix errors of state in cancel_build and test_trigger_build --- readthedocs/builds/tests/test_trigger_build.py | 2 +- readthedocs/core/utils/__init__.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/builds/tests/test_trigger_build.py b/readthedocs/builds/tests/test_trigger_build.py index 89790c97f8a..bf01150ec65 100644 --- a/readthedocs/builds/tests/test_trigger_build.py +++ b/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, False) + 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 diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 7cfd7d3f2a7..875b7bfb410 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -137,7 +137,7 @@ def prepare_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, False) + 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)