diff --git a/CHANGELOG.md b/CHANGELOG.md index 474fb8ae..0a63f523 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ and this project adheres to ## [Unreleased] +### Added + +- ✨(backend) allow converting a file while it is being analyzed + ### Fixed - 🐛(frontend) keep uploaded items usable while malware analysis runs diff --git a/src/backend/wopi/tasks/conversion.py b/src/backend/wopi/tasks/conversion.py index 5ebe8603..c854ab4e 100644 --- a/src/backend/wopi/tasks/conversion.py +++ b/src/backend/wopi/tasks/conversion.py @@ -12,6 +12,12 @@ from drive.celery_app import app logger = logging.getLogger(__name__) +# Seconds to wait before re-checking a source still under malware analysis, +# and how many times to wait before giving up (~10 minutes total, matching the +# frontend polling timeout). +ANALYSIS_RETRY_COUNTDOWN = 30 +ANALYSIS_MAX_RETRIES = 20 + class ConvertFileTask(app.Task): """Celery task base deleting the placeholder when conversion ultimately fails.""" @@ -32,6 +38,7 @@ class ConvertFileTask(app.Task): @app.task( + bind=True, base=ConvertFileTask, autoretry_for=(ConversionProviderError,), retry_backoff=True, @@ -39,7 +46,7 @@ class ConvertFileTask(app.Task): retry_jitter=True, max_retries=3, ) -def convert_file(source_item_id, converted_item_id, user_id): +def convert_file(self, source_item_id, converted_item_id, user_id): """Convert the source item and attach the result to the placeholder.""" User = get_user_model() # pylint: disable=invalid-name @@ -71,6 +78,13 @@ def convert_file(source_item_id, converted_item_id, user_id): logger.error("convert_file: user %s does not exist, aborting", user_id) return + if source.upload_state == models.ItemUploadStateChoices.ANALYZING: + logger.info( + "convert_file: source %s still analyzing, retrying conversion later", + source_item_id, + ) + raise self.retry(countdown=ANALYSIS_RETRY_COUNTDOWN, max_retries=ANALYSIS_MAX_RETRIES) + try: perform_conversion(source, placeholder, user) except Exception: diff --git a/src/backend/wopi/tests/tasks/test_conversion.py b/src/backend/wopi/tests/tasks/test_conversion.py index 2159589c..98f0fc64 100644 --- a/src/backend/wopi/tests/tasks/test_conversion.py +++ b/src/backend/wopi/tests/tasks/test_conversion.py @@ -3,6 +3,7 @@ from unittest import mock import pytest +from celery.exceptions import Retry from core import factories, models from wopi.conversion import exceptions @@ -10,6 +11,9 @@ from wopi.tasks import conversion pytestmark = pytest.mark.django_db +# As the task is bound, we need to ignore this error. +# pylint: disable=no-value-for-parameter + def _source_and_placeholder(): """Build a user with a ready source file and a converting placeholder.""" @@ -100,6 +104,27 @@ def test_convert_file_aborts_when_placeholder_state_changed(): perform_mock.assert_not_called() +def test_convert_file_retries_while_source_analyzing(): + """Wait for malware analysis to finish before converting the source bytes.""" + user, source, placeholder = _source_and_placeholder() + source.upload_state = models.ItemUploadStateChoices.ANALYZING + source.save(update_fields=["upload_state", "updated_at"]) + + with ( + mock.patch.object(conversion.convert_file, "retry", side_effect=Retry()) as retry_mock, + mock.patch.object(conversion, "perform_conversion") as perform_mock, + ): + with pytest.raises(Retry): + conversion.convert_file( + source_item_id=str(source.id), + converted_item_id=str(placeholder.id), + user_id=str(user.id), + ) + + retry_mock.assert_called_once() + perform_mock.assert_not_called() + + def test_convert_file_ignores_missing_source(): """Ignore a missing source item without crashing.""" conversion.convert_file(