From 916a5054531ce8424c0b151012450c94a55b76be Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 10 Jul 2026 13:59:03 -0400 Subject: [PATCH] Do not record a canceled job as an error when its outputs are missing Cancelling a running CLI job stops the container, but the output upload result hooks still ran and raised FileNotFoundError for the outputs the stopped container never wrote, so a user-requested cancel was recorded as a failed job. Return an empty result tuple from the run task when the task was canceled so the result hooks are skipped, and propagate the parent result from DirectDockerTask.__call__. Cancellation is latched per task request: the base property performs a fresh broker inspection on every read, and one failed inspection after the docker loop already observed the cancel would otherwise send the canceled task down the missing-output error path anyway. The run task reads only the latch after the container returns -- a cancel can only have stopped the container through the docker loop's polling, so this adds no broker round-trip to successful runs. Real CLI failures are unaffected: the output upload path only changes when the task was actually canceled. --- .../girder_worker_plugin/direct_docker_run.py | 22 +++++++++- .../test_direct_docker_run.py | 40 +++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/slicer_cli_web/girder_worker_plugin/direct_docker_run.py b/slicer_cli_web/girder_worker_plugin/direct_docker_run.py index a9a4f76..ffd5672 100644 --- a/slicer_cli_web/girder_worker_plugin/direct_docker_run.py +++ b/slicer_cli_web/girder_worker_plugin/direct_docker_run.py @@ -92,7 +92,20 @@ def resolve(arg, **kwargs): return extra_volumes +def _cancel_latched(task): + return getattr(task.request, '_slicer_cli_web_canceled', False) + + class DirectDockerTask(DockerTask): + @property + def canceled(self): + # Latch the first observed cancel: the base property re-inspects the + # broker on every read, so a later inspection that times out to False + # must not un-cancel a task the docker loop already stopped. + if not _cancel_latched(self): + self.request._slicer_cli_web_canceled = super().canceled + return self.request._slicer_cli_web_canceled + def __call__(self, *args, **kwargs): extra_volumes = _resolve_direct_file_paths(args, kwargs) if extra_volumes: @@ -104,7 +117,7 @@ def __call__(self, *args, **kwargs): for extra_volume in extra_volumes: volumes.update(extra_volume._repr_json_()) - super().__call__(*args, **kwargs) + return super().__call__(*args, **kwargs) def _has_image(image): @@ -140,4 +153,9 @@ def run(task, **kwargs): output=CLIProgressCLIWriter(task.job_manager) )) - return _docker_run(task, **kwargs) + results = _docker_run(task, **kwargs) + # Drop a canceled run's results so the upload hooks are skipped: the stopped + # container never wrote its outputs, and uploading nothing would fail the + # job as an error instead of a cancel. Read the latch, not the broker, to + # keep successful runs free of an extra round-trip. + return () if _cancel_latched(task) else results diff --git a/tests/girder_worker_plugin/test_direct_docker_run.py b/tests/girder_worker_plugin/test_direct_docker_run.py index 9d72a94..b345dcd 100644 --- a/tests/girder_worker_plugin/test_direct_docker_run.py +++ b/tests/girder_worker_plugin/test_direct_docker_run.py @@ -1,4 +1,5 @@ from os.path import basename +from unittest import mock import pytest @@ -54,3 +55,42 @@ def test_direct_docker_run(mocker, server, adminToken, file): assert kwargs['container_args'] == [target_path] # volumes assert len(kwargs['volumes']) == 2 + + +@pytest.mark.plugin('slicer_cli_web') +@pytest.mark.parametrize('canceled', [True, False]) +def test_direct_docker_run_canceled_skips_result_hooks(mocker, canceled): + docker_run_mock = mocker.patch( + 'slicer_cli_web.girder_worker_plugin.direct_docker_run._docker_run') + docker_run_mock.return_value = (None, ) + + hook = mock.Mock() + run.push_request(girder_result_hooks=[hook]) + try: + if canceled: + # stand in for the docker loop having latched the cancel mid-run + run.request._slicer_cli_web_canceled = True + run(image='test', container_args=[]) + finally: + run.pop_request() + + docker_run_mock.assert_called_once() + # a canceled run must not upload the outputs its stopped container skipped + if canceled: + hook.transform.assert_not_called() + else: + hook.transform.assert_called_once() + + +@pytest.mark.plugin('slicer_cli_web') +def test_direct_docker_run_cancel_is_latched(mocker): + # once the broker reports the revocation, a later inspection that times out + # to False must not un-cancel the run. + mocker.patch('girder_worker.task.is_revoked', side_effect=[True, False, False]) + + run.push_request() + try: + assert run.canceled + assert run.canceled + finally: + run.pop_request()