Do not record a canceled job as an error when its outputs are missing - #258
Open
PaulHax wants to merge 1 commit into
Open
Do not record a canceled job as an error when its outputs are missing#258PaulHax wants to merge 1 commit into
PaulHax wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Item 3 of #256.
Cancelling a running CLI job today ends with the job marked
ERROR, notCANCELED:Task.__call__then applies thegirder_result_hooksupload transforms anyway.GirderUploadVolumePathToFolder.transformraisesFileNotFoundErroron the missing output path.ERRORwith aFileNotFoundErrortraceback in its log.To reproduce: submit any CLI with a file output, cancel it while the container is running, and watch the final status.
The fix: the
runtask returns an empty result tuple whentask.canceled, so the upload hooks are skipped and girder_worker's normal revoked handling records the job asCANCELED. Onlytask.canceledgates the skip — a real failure (container exits without writing outputs, no cancel) still goes down the error path unchanged.One supporting change: cancellation is latched per task request. The base
canceledproperty re-inspects the broker on every read, so a single timed-out inspection after the docker loop already observed the cancel would flip it back toFalseand send the canceled task down the missing-output error path anyway. Theruntask reads only the latch after the container returns (a cancel can only have stopped the container through the docker loop's polling, which reads the property), so successful runs pay no extra broker round-trip.Tests included.
Related: #228 and #183 mark stuck or old jobs as canceled after the fact; this fixes the cause by not turning a cancel into an error in the first place.