diff --git a/modules/zim/lib.py b/modules/zim/lib.py index 30365e1c..0a746854 100644 --- a/modules/zim/lib.py +++ b/modules/zim/lib.py @@ -586,8 +586,10 @@ async def remove_outdated_zim_files(path: pathlib.Path = None) -> int: deleted_count += 1 if deleted_count: - # Refresh synchronously to clean up DB before returning. - await file_worker.refresh_sync(list(outdated)) + # Refresh synchronously to clean up DB before returning. Deleting needs no modeling, and the + # global modelers would work through the whole unindexed backlog inside this request. + await file_worker.refresh_sync(list(outdated), post_processing=False) + flag_outdated_zim_files() # Trigger Kiwix restart so the deleted zim files are no longer served. restart_kiwix_handler.activate_switch() diff --git a/modules/zim/test/test_restart_kiwix.py b/modules/zim/test/test_restart_kiwix.py index e66fdec9..c7e72841 100644 --- a/modules/zim/test/test_restart_kiwix.py +++ b/modules/zim/test/test_restart_kiwix.py @@ -7,6 +7,7 @@ from modules.zim import lib from modules.zim.models import Zim from wrolpi.api_utils import api_app from wrolpi.files.models import FileGroup +from wrolpi.files.worker import file_worker @pytest.mark.asyncio @@ -145,3 +146,28 @@ async def test_remove_outdated_zim_files_no_restart_when_nothing_deleted(async_c assert deleted_count == 0 switches = dict(api_app.shared_ctx.switches) if api_app.shared_ctx.switches else {} assert 'restart_kiwix' not in switches + + +@pytest.mark.asyncio +async def test_remove_outdated_zim_files_skips_post_processing(async_client, test_session, test_directory, + zim_path_factory): + """Deleting outdated Zims must not run the global modelers inside the request. + + `DELETE /api/zim/outdated` awaits this; modeling the whole unindexed backlog there exceeded the + response timeout and the cancelled request stranded the file worker status.""" + zim_dir = test_directory / 'zims' + zim_dir.mkdir(exist_ok=True) + test_zim_bytes = zim_path_factory().read_bytes() + paths = [zim_dir / 'wikipedia_en_all_maxi_2020-01.zim', zim_dir / 'wikipedia_en_all_maxi_2020-02.zim'] + for path in paths: + path.write_bytes(test_zim_bytes) + await file_worker.refresh_sync(paths) + assert test_session.query(FileGroup).count() == 2 + + with mock.patch('wrolpi.files.worker.apply_modelers') as apply_modelers: + deleted_count = await lib.remove_outdated_zim_files(zim_dir) + + assert deleted_count == 1 + apply_modelers.assert_not_called() + # The deleted Zim is still removed from the DB. + assert test_session.query(FileGroup).count() == 1 diff --git a/wrolpi/files/test/test_worker.py b/wrolpi/files/test/test_worker.py index b59c1903..8ef1ae08 100644 --- a/wrolpi/files/test/test_worker.py +++ b/wrolpi/files/test/test_worker.py @@ -7,6 +7,7 @@ from unittest import mock import pytest from sqlalchemy import text +from wrolpi import flags from wrolpi.conftest import await_switches from wrolpi.dates import from_timestamp from wrolpi.vars import IS_MACOS @@ -637,6 +638,25 @@ async def test_refresh_sync_records_error_status_on_failure(async_client, test_s assert 'boom' in (file_worker.status['error'] or '') +@pytest.mark.asyncio +async def test_refresh_sync_resets_status_when_cancelled(async_client, test_session, test_directory, + make_files_structure, flags_lock): + """A cancelled synchronous refresh must not strand the status at its last phase. + + `refresh_sync` runs inside request handlers, which Sanic cancels on response timeout. The + phase flags clear when their `with` block unwinds, but the shared status does not, so the + status endpoint kept reporting 'modeling' while nothing ran. + """ + paths = make_files_structure(['docs/file1.txt']) + + with mock.patch('wrolpi.files.worker.apply_modelers', side_effect=asyncio.CancelledError): + with pytest.raises(asyncio.CancelledError): + await file_worker.refresh_sync(paths) + + assert file_worker.status['status'] == 'idle' + assert not flags.file_worker_modeling.is_set() + + @pytest.mark.asyncio async def test_process_queue_resets_status_on_handler_error(async_client, test_session, test_directory, make_files_structure): diff --git a/wrolpi/files/worker.py b/wrolpi/files/worker.py index 99890a5c..ac4e989d 100644 --- a/wrolpi/files/worker.py +++ b/wrolpi/files/worker.py @@ -346,9 +346,9 @@ class FileWorker(FileMoveMixin, FileReorganizeMixin): # `_upsert_file_groups`/`_delete_file_groups`/`_apply_post_processing` all advance the # shared status (upserting/deleting/modeling/indexing/cleanup). Unlike the queued # handlers this path has no `process_queue` wrapper to reset it, so reset here or the UI - # is stranded at the last phase (e.g. 'cleanup' after deleting files). Mirror - # `process_queue`: reset to idle only on success, leave a terminal 'error' status on - # failure (and re-raise so the synchronous caller still sees it). + # is stranded at the last phase (e.g. 'cleanup' after deleting files). Reset to idle on + # success or cancellation, leave a terminal 'error' status on failure (and re-raise so the + # synchronous caller still sees it). try: result = await self._refresh_files_directly(paths) self._cleanup_modified_models(result.modified) @@ -361,6 +361,9 @@ class FileWorker(FileMoveMixin, FileReorganizeMixin): # unindexed files and could exceed Sanic's RESPONSE_TIMEOUT. await self._apply_post_processing(is_global_refresh=False) except asyncio.CancelledError: + # Sanic cancels the request handler on response timeout. Nothing is running anymore, so + # report idle; the unfinished modeling is picked up by the next refresh. + self.reset_status() raise except Exception as e: logger.error(f'refresh_sync failed for {len(paths)} paths', exc_info=e)