Skip to content

Commit

Permalink
Build: set CANCELLED state when the build is cancelled (#11171)
Browse files Browse the repository at this point in the history 
* Build: set `CANCELLED` state when the build is cancelled

This bug was introduced in
https://github.com/readthedocs/readthedocs.org/pull/10922/files#diff-4eae6664ca2124d124dbfe04a8000483c7a23d426c3f390721d6e80fe5891064L504
by mistake.

Closes #11170

* Lint

* Test for `CANCELLED_BY_USER`
  • Loading branch information
@humitos
humitos committed Feb 29, 2024
1 parent 695667e commit b9d9cf0
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 40 deletions.
80 changes: 41 additions & 39 deletions readthedocs/projects/tasks/builds.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
ARTIFACT_TYPES_WITHOUT_MULTIPLE_FILES_SUPPORT,
BUILD_FINAL_STATES,
BUILD_STATE_BUILDING,
BUILD_STATE_CANCELLED,
BUILD_STATE_CLONING,
BUILD_STATE_FINISHED,
BUILD_STATE_INSTALLING,
Expand Down Expand Up @@ -136,7 +137,7 @@ class SyncRepositoryTask(SyncRepositoryMixin, Task):
in our database.
"""

name = __name__ + '.sync_repository_task'
name = __name__ + ".sync_repository_task"
max_retries = 5
default_retry_delay = 7 * 60
throws = (
Expand All @@ -145,7 +146,7 @@ class SyncRepositoryTask(SyncRepositoryMixin, Task):
)

def before_start(self, task_id, args, kwargs):
log.info('Running task.', name=self.name)
log.info("Running task.", name=self.name)

# Create the object to store all the task-related data
self.data = TaskData()
Expand All @@ -168,7 +169,7 @@ def before_start(self, task_id, args, kwargs):

# Also note there are builds that are triggered without a commit
# because they just build the latest commit for that version
self.data.build_commit = kwargs.get('build_commit')
self.data.build_commit = kwargs.get("build_commit")

log.bind(
project_slug=self.data.project.slug,
Expand All @@ -179,7 +180,7 @@ def on_failure(self, exc, task_id, args, kwargs, einfo):
# Do not log as error handled exceptions
if isinstance(exc, RepositoryError):
log.warning(
'There was an error with the repository.',
"There was an error with the repository.",
)
elif isinstance(exc, SyncRepositoryLocked):
log.warning(
Expand Down Expand Up @@ -274,10 +275,8 @@ class UpdateDocsTask(SyncRepositoryMixin, Task):
build all the documentation formats and upload them to the storage.
"""

name = __name__ + '.update_docs_task'
autoretry_for = (
BuildMaxConcurrencyError,
)
name = __name__ + ".update_docs_task"
autoretry_for = (BuildMaxConcurrencyError,)
max_retries = settings.RTD_BUILDS_MAX_RETRIES
default_retry_delay = settings.RTD_BUILDS_RETRY_DELAY
retry_backoff = False
Expand Down Expand Up @@ -320,10 +319,12 @@ class UpdateDocsTask(SyncRepositoryMixin, Task):

def _setup_sigterm(self):
def sigterm_received(*args, **kwargs):
log.warning('SIGTERM received. Waiting for build to stop gracefully after it finishes.')
log.warning(
"SIGTERM received. Waiting for build to stop gracefully after it finishes."
)

def sigint_received(*args, **kwargs):
log.warning('SIGINT received. Canceling the build running.')
log.warning("SIGINT received. Canceling the build running.")

# Only allow to cancel the build if it's not already uploading the files.
# This is to protect our users to end up with half of the documentation uploaded.
Expand All @@ -347,12 +348,12 @@ def _check_concurrency_limit(self):
)
concurrency_limit_reached = response.get("limit_reached", False)
max_concurrent_builds = response.get(
'max_concurrent',
"max_concurrent",
settings.RTD_MAX_CONCURRENT_BUILDS,
)
except Exception:
log.exception(
'Error while hitting/parsing API for concurrent limit checks from builder.',
"Error while hitting/parsing API for concurrent limit checks from builder.",
project_slug=self.data.project.slug,
version_slug=self.data.version.slug,
)
Expand All @@ -375,7 +376,7 @@ def _check_concurrency_limit(self):

def _check_project_disabled(self):
if self.data.project.skip:
log.warning('Project build skipped.')
log.warning("Project build skipped.")
raise BuildAppError(BuildAppError.BUILDS_DISABLED)

def before_start(self, task_id, args, kwargs):
Expand Down Expand Up @@ -403,21 +404,21 @@ def before_start(self, task_id, args, kwargs):
self.data.project = self.data.version.project

# Save the builder instance's name into the build object
self.data.build['builder'] = socket.gethostname()
self.data.build["builder"] = socket.gethostname()

# Reset any previous build error reported to the user
self.data.build['error'] = ''
self.data.build["error"] = ""
# Also note there are builds that are triggered without a commit
# because they just build the latest commit for that version
self.data.build_commit = kwargs.get('build_commit')
self.data.build_commit = kwargs.get("build_commit")

self.data.build_director = BuildDirector(
data=self.data,
)

log.bind(
# NOTE: ``self.data.build`` is just a regular dict, not an APIBuild :'(
builder=self.data.build['builder'],
builder=self.data.build["builder"],
commit=self.data.build_commit,
project_slug=self.data.project.slug,
version_slug=self.data.version.slug,
Expand Down Expand Up @@ -470,7 +471,7 @@ def on_failure(self, exc, task_id, args, kwargs, einfo):
#
# So, we create the `self.data.build` with the minimum required data.
self.data.build = {
'id': self.data.build_pk,
"id": self.data.build_pk,
}

# Known errors in our application code (e.g. we couldn't connect to
Expand All @@ -488,6 +489,10 @@ def on_failure(self, exc, task_id, args, kwargs, einfo):
else:
message_id = BuildUserError.GENERIC

# Set build state as cancelled if the user cancelled the build
if isinstance(exc, BuildCancelled):
self.data.build["state"] = BUILD_STATE_CANCELLED

else:
# We don't know what happened in the build. Log the exception and
# report a generic notification to the user.
Expand All @@ -513,7 +518,7 @@ def on_failure(self, exc, task_id, args, kwargs, einfo):
if message_id not in self.exceptions_without_notifications:
self.send_notifications(
self.data.version_pk,
self.data.build['id'],
self.data.build["id"],
event=WebHookEvent.BUILD_FAILED,
)

Expand Down Expand Up @@ -541,7 +546,7 @@ def on_failure(self, exc, task_id, args, kwargs, einfo):

send_external_build_status(
version_type=version_type,
build_pk=self.data.build['id'],
build_pk=self.data.build["id"],
commit=self.data.build_commit,
status=status,
)
Expand Down Expand Up @@ -661,20 +666,20 @@ def on_success(self, retval, task_id, args, kwargs):

self.send_notifications(
self.data.version.pk,
self.data.build['id'],
self.data.build["id"],
event=WebHookEvent.BUILD_PASSED,
)

if self.data.build_commit:
send_external_build_status(
version_type=self.data.version.type,
build_pk=self.data.build['id'],
build_pk=self.data.build["id"],
commit=self.data.build_commit,
status=BUILD_STATUS_SUCCESS,
)

# Update build object
self.data.build['success'] = True
self.data.build["success"] = True

def on_retry(self, exc, task_id, args, kwargs, einfo):
"""
Expand All @@ -686,11 +691,11 @@ def on_retry(self, exc, task_id, args, kwargs, einfo):
See https://docs.celeryproject.org/en/master/userguide/tasks.html#retrying
"""
log.info('Retrying this task.')
log.info("Retrying this task.")

if isinstance(exc, BuildMaxConcurrencyError):
log.warning(
'Delaying tasks due to concurrency limit.',
"Delaying tasks due to concurrency limit.",
project_slug=self.data.project.slug,
version_slug=self.data.version.slug,
)
Expand All @@ -713,7 +718,7 @@ def after_return(self, status, retval, task_id, args, kwargs, einfo):
so some attributes from the `self.data` object may not be defined.
"""
# Update build object
self.data.build['length'] = (timezone.now() - self.data.start_time).seconds
self.data.build["length"] = (timezone.now() - self.data.start_time).seconds

build_state = None
# The state key might not be defined
Expand Down Expand Up @@ -742,9 +747,9 @@ def after_return(self, status, retval, task_id, args, kwargs, einfo):
)

log.info(
'Build finished.',
length=self.data.build['length'],
success=self.data.build['success']
"Build finished.",
length=self.data.build["length"],
success=self.data.build["success"],
)

def update_build(self, state=None):
Expand Down Expand Up @@ -856,23 +861,20 @@ def get_build(self, build_pk):
if build_pk:
build = self.data.api_client.build(build_pk).get()
private_keys = [
'project',
'version',
'resource_uri',
'absolute_uri',
"project",
"version",
"resource_uri",
"absolute_uri",
]
# TODO: try to use the same technique than for ``APIProject``.
return {
key: val
for key, val in build.items() if key not in private_keys
}
return {key: val for key, val in build.items() if key not in private_keys}

# NOTE: this can be just updated on `self.data.build['']` and sent once the
# build has finished to reduce API calls.
def set_valid_clone(self):
"""Mark on the project that it has been cloned properly."""
self.data.api_client.project(self.data.project.pk).patch(
{'has_valid_clone': True}
{"has_valid_clone": True}
)
self.data.project.has_valid_clone = True
self.data.version.project.has_valid_clone = True
Expand All @@ -887,7 +889,7 @@ def store_build_artifacts(self):
Remove build artifacts of types not included in this build (PDF, ePub, zip only).
"""
time_before_store_build_artifacts = timezone.now()
log.info('Writing build artifacts to media storage')
log.info("Writing build artifacts to media storage")
self.update_build(state=BUILD_STATE_UPLOADING)

valid_artifacts = self.get_valid_artifact_types()
Expand Down
51 changes: 50 additions & 1 deletion readthedocs/projects/tests/test_build_tasks.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from readthedocs.config.config import BuildConfigV2
from readthedocs.config.exceptions import ConfigError
from readthedocs.config.tests.test_config import get_build_config
from readthedocs.doc_builder.exceptions import BuildUserError
from readthedocs.doc_builder.exceptions import BuildCancelled, BuildUserError
from readthedocs.projects.exceptions import RepositoryError
from readthedocs.projects.models import EnvironmentVariable, Project, WebHookEvent
from readthedocs.projects.tasks.builds import sync_repository_task, update_docs_task
Expand Down Expand Up @@ -695,6 +695,55 @@ def test_failed_build(
assert revoke_key_request._request.method == "POST"
assert revoke_key_request.path == "/api/v2/revoke/"

@mock.patch("readthedocs.projects.tasks.builds.send_external_build_status")
@mock.patch("readthedocs.projects.tasks.builds.UpdateDocsTask.execute")
def test_cancelled_build(
self,
execute,
send_external_build_status,
):
# Force an exception from the execution of the task. We don't really
# care "where" it was raised: setup, build, syncing directories, etc
execute.side_effect = BuildCancelled(
message_id=BuildCancelled.CANCELLED_BY_USER
)

self._trigger_update_docs_task()

send_external_build_status.assert_called_once_with(
version_type=self.version.type,
build_pk=self.build.pk,
commit=self.build.commit,
status=BUILD_STATUS_FAILURE,
)

notification_request = self.requests_mock.request_history[-3]
assert notification_request._request.method == "POST"
assert notification_request.path == "/api/v2/notifications/"
assert notification_request.json() == {
"attached_to": f"build/{self.build.pk}",
"message_id": BuildCancelled.CANCELLED_BY_USER,
"state": "unread",
"dismissable": False,
"news": False,
"format_values": {},
}

# Test we are updating the DB by calling the API with the updated build object
# The second last one should be the PATCH for the build
build_status_request = self.requests_mock.request_history[-2]
assert build_status_request._request.method == "PATCH"
assert build_status_request.path == "/api/v2/build/1/"
assert build_status_request.json() == {
"builder": mock.ANY,
"commit": self.build.commit,
"error": "", # We are not sending ``error`` anymore
"id": self.build.pk,
"length": mock.ANY,
"state": "cancelled",
"success": False,
}

@mock.patch("readthedocs.doc_builder.director.load_yaml_config")
def test_build_commands_executed(
self,
Expand Down

0 comments on commit b9d9cf0

Please sign in to comment.