Do not record a canceled job as an error when its outputs are missing

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

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()