Skip to content

Commit

Permalink
Build: communicate uploading errors to users
Browse files Browse the repository at this point in the history 
Running `store_build_artifacts` from inside `.execute()` method
instead of from `.on_sucess()` allow us to raise any exception and handle it
properly.

We can raise a `BuildUserError` to clearly communicate the specific error to the
user, or any other exception to show the generic message: "Try again or contact
us".

Besides, this allows us to *fail the build* instead of continuing silently
without the user noticing this:

- there more than 1 file of formats that don't support multiple files
- the output path is not a directory
- there were errors uploading the artifacts to the storage
  • Loading branch information
@humitos
humitos committed Jan 25, 2023
1 parent e05470a commit 2f6b59f
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 47 deletions.
7 changes: 6 additions & 1 deletion readthedocs/doc_builder/exceptions.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,12 @@ class BuildUserError(BuildBaseException):
f'No "{BUILD_COMMANDS_OUTPUT_PATH_HTML}" folder was created during this build.'
)
BUILD_OUTPUT_IS_NOT_A_DIRECTORY = gettext_noop(
'Build output directory for format "{format}" is not a directory.',
'Build output directory for format "{artifact_type}" is not a directory.'
)
BUILD_OUTPUT_HAS_MULTIPLE_FILES = gettext_noop(
'Build output directory for format "{artifact_type}" contains multiple files '
"and it is not currently supported. "
'Please, remove all the files but the "{artifact_type}" your want to upload.'
)


Expand Down
99 changes: 53 additions & 46 deletions readthedocs/projects/tasks/builds.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,6 @@ def on_failure(self, exc, task_id, args, kwargs, einfo):
if self.data.version:
version_type = self.data.version.type

# NOTE: autoflake gets confused here. We need the NOQA for now.
status = BUILD_STATUS_FAILURE
if isinstance(exc, BuildUserSkip):
# The build was skipped by returning the magic exit code,
Expand Down Expand Up @@ -537,56 +536,41 @@ def get_valid_artifact_types(self):
continue

if not os.path.isdir(artifact_directory):
log.exception(
log.error(
"The output path is not a directory.",
output_format=artifact_type,
)
# TODO: we should raise an exception here, fail the build,
# and communicate the error to the user.
# We are just skipping this output format for now.
#
# raise BuildUserError(BuildUserError.BUILD_OUTPUT_IS_NOT_A_DIRECTORY.format(artifact_type))
continue
raise BuildUserError(
BuildUserError.BUILD_OUTPUT_IS_NOT_A_DIRECTORY.format(
artifact_type=artifact_type
)
)

# Check if there are multiple files on artifact directories.
# These output format does not support multiple files yet.
# In case multiple files are found, the upload for this format is not performed.
#
# TODO: we should fail the build for these cases and clearly communicate this.
# to the user. To do this, we need to call this method (``store_build_artifacts``)
# since the ``execute`` method.
# It will allow us to just `raise BuildUserError` and handle it at
# Celery `on_failure` handler.
if artifact_type in ("htmlzip", "epub", "pdf"):
if len(os.listdir(artifact_directory)) > 1:
log.exception(
log.error(
"Multiple files are not supported for this format. "
"Skipping this output format.",
output_format=artifact_type,
)
continue
raise BuildUserError(
BuildUserError.BUILD_OUTPUT_HAS_MULTIPLE_FILES.format(
artifact_type=artifact_type
)
)

# If all the conditions were met, the artifact is valid
valid_artifacts.append(artifact_type)

return valid_artifacts

def on_success(self, retval, task_id, args, kwargs):

valid_artifacts = self.get_valid_artifact_types()
time_before_store_build_artifacts = timezone.now()
# Store build artifacts to storage (local or cloud storage)
#
# TODO: move ``store_build_artifacts`` inside ``execute`` so we can
# handle exceptions properly on ``on_failure``
self.store_build_artifacts(valid_artifacts)
log.info(
"Store build artifacts finished.",
time=(timezone.now() - time_before_store_build_artifacts).seconds,
)

# NOTE: we are updating the db version instance *only* when
if "html" in valid_artifacts:
# TODO: remove this condition and *always* update the DB Version instance
if "html" in self.get_valid_artifact_types():
try:
api_v2.version(self.data.version.pk).patch(
{
Expand All @@ -601,7 +585,8 @@ def on_success(self, retval, task_id, args, kwargs):
# NOTE: I think we should fail the build if we cannot update
# the version at this point. Otherwise, we will have inconsistent data
log.exception(
'Updating version failed, skipping file sync.',
"Updating version db object failed. "
'Files are synced in the storage, but "Version" object is not updated',
)

# Index search data
Expand Down Expand Up @@ -698,10 +683,13 @@ def update_build(self, state=None):
try:
api_v2.build(self.data.build['id']).patch(self.data.build)
except Exception:
# NOTE: I think we should fail the build if we cannot update it
# at this point otherwise, the data will be inconsistent and we
# may be serving "new docs" but saying the "build have failed"
log.exception('Unable to update build')
# NOTE: we are updating the "Build" object on each `state`.
# Only if the last update fails, there may be some inconsistency
# between the "Build" object in our db and the reality.
#
# The `state` argument will help us to track this more and understand
# at what state our updates are failing and decide what to do.
log.exception("Error while updating the build object.", state=state)

def execute(self):
self.data.build_director = BuildDirector(
Expand Down Expand Up @@ -749,6 +737,12 @@ def execute(self):
finally:
self.data.build_data = self.collect_build_data()

# At this point, the user's build already succeeded.
# However, we cannot use `.on_success()` because we still have to upload the artifacts;
# which could fail, and we want to detect that and handle it properly at `.on_failure()`
# Store build artifacts to storage (local or cloud storage)
self.store_build_artifacts()

def collect_build_data(self):
"""
Collect data from the current build.
Expand Down Expand Up @@ -817,7 +811,7 @@ def set_valid_clone(self):
self.data.project.has_valid_clone = True
self.data.version.project.has_valid_clone = True

def store_build_artifacts(self, artifacts):
def store_build_artifacts(self):
"""
Save build artifacts to "storage" using Django's storage API.
Expand All @@ -826,17 +820,16 @@ def store_build_artifacts(self, artifacts):
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')
# NOTE: I don't remember why we removed this state from the Build
# object. I'm re-adding it because I think it's useful, but we can
# remove it if we want
self.update_build(state=BUILD_STATE_UPLOADING)

valid_artifacts = self.get_valid_artifact_types()
types_to_copy = []
types_to_delete = []

for artifact_type in ARTIFACT_TYPES:
if artifact_type in artifacts:
if artifact_type in valid_artifacts:
types_to_copy.append(artifact_type)
# Never delete HTML nor JSON (search index)
elif artifact_type not in UNDELETABLE_ARTIFACT_TYPES:
Expand All @@ -857,14 +850,20 @@ def store_build_artifacts(self, artifacts):
try:
build_media_storage.sync_directory(from_path, to_path)
except Exception:
# Ideally this should just be an IOError
# but some storage backends unfortunately throw other errors
# NOTE: the exceptions reported so far are:
# - botocore.exceptions:HTTPClientError
# - botocore.exceptions:ClientError
# - readthedocs.doc_builder.exceptions:BuildCancelled
log.exception(
'Error copying to storage (not failing build)',
"Error copying to storage",
media_type=media_type,
from_path=from_path,
to_path=to_path,
)
# Re-raise the exception to fail the build and handle it
# automatically at `on_failure`.
# It will clearly communicate the error to the user.
raise BuildAppError("Error uploading files to the storage.")

# Delete formats
for media_type in types_to_delete:
Expand All @@ -877,13 +876,21 @@ def store_build_artifacts(self, artifacts):
try:
build_media_storage.delete_directory(media_path)
except Exception:
# Ideally this should just be an IOError
# but some storage backends unfortunately throw other errors
# NOTE: I didn't find any log line for this case yet
log.exception(
'Error deleting from storage (not failing build)',
"Error deleting files from storage",
media_type=media_type,
media_path=media_path,
)
# Re-raise the exception to fail the build and handle it
# automatically at `on_failure`.
# It will clearly communicate the error to the user.
raise BuildAppError("Error deleting files from storage.")

log.info(
"Store build artifacts finished.",
time=(timezone.now() - time_before_store_build_artifacts).seconds,
)

def send_notifications(self, version_pk, build_pk, event):
"""Send notifications to all subscribers of `event`."""
Expand Down

0 comments on commit 2f6b59f

Please sign in to comment.