Fix run hanging forever when a shutdown callback blocks during abort - #7467
Open
pditommaso wants to merge 1 commit into
Open
Fix run hanging forever when a shutdown callback blocks during abort#7467pditommaso wants to merge 1 commit into
pditommaso wants to merge 1 commit into
Conversation
`Session.abort()` invoked `shutdown0()` before force-terminating the execution barriers, therefore any shutdown callback taking too long (or hanging) prevented the release of the `main` thread awaiting the pipeline termination in `Session.await()`, hanging the execution indefinitely. This commit force-terminates the barriers before running the shutdown callbacks. To preserve the current ordering guarantee, `shutdown0()` invoked by a second thread (i.e. `destroy()` on the main thread) now awaits the completion of the callbacks being executed by the aborting thread, instead of returning immediately, though not indefinitely. It also bounds `SimpleAgent` state retrieval, that awaited on a latch that only the agent runner thread is able to count down, and short-circuits it when the invoking thread is the runner itself, which would otherwise deadlock on the abort path e.g. `WorkflowStatsObserver` agent error handler aborting the session. Finally the agent runner thread is given a name to make this class of problem easier to triage in a thread dump. Closes #7444 Assisted-by: Claude Opus 5 (1M context) Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.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.
Closes #7444
Problem
Session.abort()invokedshutdown0()before force-terminating the execution barriers:Meanwhile
mainparks inSession.await()onprocessesBarrier.awaitCompletion(), which has no timeout. The only code that can release it sits behind the callback drain, so any blocking shutdown callback deadlocks the whole run — noExecution complete -- Goodbye, and no terminal status reported to any observer, sincenotifyFlowComplete()is inside the blockedshutdown0()too. The reporter observed a head job stuck like this for 2h38m.A second unbounded wait exists in
SimpleAgent.getResult(), which awaits aCountDownLatchthat only the agent's own runner thread can count down. That runner also exits its loop onInterruptedException, after which every subsequentgetValue()hangs forever. And there is a guaranteed self-deadlock:WorkflowStatsObserverbuilds its agent as.onError { err -> session.abort(err) }, so the runner thread itself can enterabort()→invokeOnComplete()→getWorkflowStats()→getValue(), enqueueing an event that only that same (no longer polling) thread could serve.Changes
Session.abort()— force-terminate the barriers before running the shutdown callbacks. The relative order ofshutdown0() → notifyError() → logObserver.forceTermination()is preserved, so observer event ordering is unchanged.Session.shutdown0()— releasing the barrier early letsmainrace ahead intodestroy(). With the #7349 CAS guard alone, the second caller returns immediately, which would meandestroy()closing the cache and stopping plugins while the callbacks are still running. Instead the second caller now awaits the in-flight run on aCountDownLatch— restoring exactly the ordering that exists today — bounded at 5 min, after which it logs a warning and proceeds. Normal aborts behave identically; only the pathological case degrades, from "hangs forever" to "warns and exits".One knock-on: on the abort path the pool managers in
destroy()now shut down concurrently with the callbacks rather than after.shutdownOrAbort(aborted=true)isshutdownNow()— non-blocking, and those pools are killed regardless on an abort.SimpleAgent— boundgetResult()at 1 min, returning the current state with a warning on expiry; short-circuitgetValue()/getQuickValue()when the caller is the runner thread, fixing the self-deadlock properly rather than merely bounding it; name the runner threadagent-<StateType>so it is identifiable in a thread dump.Testing
Two regression tests added.
SessionTest > should release the await barrier when a shutdown callback is blockingwas verified to fail against the originalabort()ordering and pass with the fix. Full:nextflow:testsuite passes.Out of scope
Two other anomalies visible in the reporter's thread table are left alone: the
AWSBatch-executorthreads still submitting jobs 8s after the abort, and theTask submitterthread's unconditionalwhile(true).🤖 Generated with Claude Code