diff --git a/readthedocs/builds/tests/test_cancel_build.py b/readthedocs/builds/tests/test_cancel_build.py new file mode 100644 index 00000000000..f01f3c3982c --- /dev/null +++ b/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, + ) 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 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 78b69c2c943..875b7bfb410 100644 --- a/readthedocs/core/utils/__init__.py +++ b/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,