Do not submit queued tasks after the session has aborted - #7482
Draft
Mohit-Ak wants to merge 1 commit into
Draft
Conversation
ParallelPollingMonitor.submit() only enqueues the submission onto the throttling executor, so the session.canSubmitTasks() guard in TaskPollingMonitor.submitPendingTasks() is an enqueue-time check on work that runs later. Every wrapper already queued when a session aborts is still submitted to the backend, and those jobs are neither killed by the shutdown sweep nor observed by the polling loop. Re-check the session state inside the wrapper so a queued submission becomes a no-op once the session can no longer accept submissions. Signed-off-by: Mohit Arvind Khakharia <Mohit-Ak@users.noreply.github.com>
✅ Deploy Preview for nextflow-docs 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
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.
Bug report
Once a session aborts, submissions already sitting in
ParallelPollingMonitor'sthrottled executor queue still go through to the backend. The resulting jobs are
never killed and never polled — the shutdown kill sweep has already run and the
task monitor has already exited — so they run to completion in the backend with
nothing watching them. On a cloud batch service that is paid compute belonging to
a run that is already dead.
TaskPollingMonitor.submitPendingTasks()does guard the loop:but
ParallelPollingMonitoroverridessubmit()so that it only enqueues aThrottlingExecutor.Recoverablewrapper and returns immediately. By the time apool thread dequeues that wrapper and calls
submit0(handler)— which runsprepareLauncher(),handler.submit(),runningQueue.add(handler)andnotifyTaskSubmit(handler)— the session state is never re-read. The guard istherefore an enqueue-time check on work that executes later, and everything
already queued when the abort lands is submitted regardless.
What this changes
One check inside the wrapper, so a queued submission becomes a no-op if the
session can no longer accept submissions by the time it actually runs:
This closes the queued-wrapper window, which is the bulk of the exposure. A
submission that is already inside
submit0()when the abort lands can stillrace through; closing that residue needs the kill sweep to cover late
runningQueueadditions, which is a larger change and felt out of scope here —happy to look at it separately if you want it folded in.
The debug line is deliberate: the silent bypass is a good part of what makes
orphaned jobs hard to track down after the fact.
How this was tested
Two tests added to
ParallelPollingMonitorTest, one per side of the branch:should not submit a queued task once the session can no longer submit—stubs
canSubmitTasks() >> falseand assertsprepareLauncher(),submit()andnotifyTaskSubmit()are never invoked.should submit a queued task while the session can still submit— thecontrol; stubs
canSubmitTasks() >> trueand asserts all three areinvoked exactly once, so the first test can't pass by simply breaking
submission.
Against unpatched
masterthe first test fails withTooManyInvocationsError: 0 * handler.prepareLauncher() (1 invocation), which isthe bug stated as an assertion, while the control passes. With the change both
pass.
One existing test needed a stub added.
should retry taskbuilds its sessionwith
Mock(Session)and never stubscanSubmitTasks(); Spock returnsfalsefor unstubbed booleans, so under the new check that scenario would skip
submission and the retry counters would stay at zero. That test is about the
submit operation failing and being retried, not about an aborted session, so
_ * session.canSubmitTasks() >> truemakes the intended precondition explicit.No assertion was weakened.
AwsBatchExecutoris the only current consumer ofParallelPollingMonitor, soAWS Batch is where this is observable today, but the fix sits in core and covers
any future executor that adopts async submission.
Fixes #7445