Release the maxForks slot when task submission fails - #7463
Open
Mohit-Ak wants to merge 2 commits into
Open
Conversation
TaskPollingMonitor.submitPendingTasks() increments the process forks counter before attempting the submit, but the slot was only returned via handleException() -> evict(), and evict() removes from the running queue. A handler whose submit() threw was never added to that queue, so evict() returned false and the slot was never given back. After maxForks such failures canForkProcess() stays false and the process makes no further progress. Release the slot at the submit-failure site instead of widening the condition inside handleException(): checkTaskStatus() already calls decProcessForks() before evict(), so a broader condition there would double-release for tasks that fail after a successful submit. ParallelPollingMonitor routes submit failures through its own Recoverable.onFailure(), which has the same leak, so it releases the slot too. Fixes nextflow-io#7447 Signed-off-by: Mohit Arvind Khakharia <mohitkhakharia@gmail.com>
✅ Deploy Preview for nextflow-docs canceled.
|
Mohit-Ak
marked this pull request as ready for review
August 10, 2026 14:12
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.
Problem
maxForksslots are leaked wheneverTaskHandler.submit()throws. InTaskPollingMonitor.submitPendingTasks()the process forks counter isincremented before the submit is attempted:
handler.incProcessForks() submit(handler)but the slot is only returned through
handleException(), which isconditional on eviction:
and
evict()removes from the running queue. A handler whosesubmit()threw was never added to that queue —
TaskPollingMonitor.submit()adds it onlyafter
handler.submit()returns — soevict()returns false, the decrement isskipped, and the slot is gone for good.
After
maxForkssuch failurescanForkProcess()is permanently false and theprocess is starved: its remaining tasks sit in the pending queue indefinitely and
are re-reported every
dumpInterval.This only bites processes that set
maxForksexplicitly. With the defaultmaxForks = 0,TaskProcessorleavesforksCountnull and the?.inincProcessForks()/decProcessForks()makes the whole thing a no-op, so thereis no counter and no leak.
The evict-conditional decrement came in with 05ea0c8 (#2787) to handle
exceptions from running tasks, where the handler really is in
runningQueueand a successful
evict()is the correct ownership test. The submit-failure pathreuses the same
handleException()but can never satisfy that test.Fix
Release the slot at the submit-failure site rather than widening the condition
inside
handleException(). The newreleaseSubmitSlot()helper decrements onlywhen the handler is not in the running queue, which is precisely the "took a
slot but never made it into the queue" case:
Loosening
handleException()instead would double-release:checkTaskStatus()already calls
decProcessForks()before its ownevict(), so tasks that failafter a successful submit would give the slot back twice and let the process
exceed
maxForks.I deliberately did not move
incProcessForks()down next torunningQueue.add(handler).ParallelPollingMonitor.submit()hands the realsubmission to a thread pool and returns immediately, so the increment would land
after
canForkProcess()had already been evaluated for later pending tasks —which would let
maxForksbe exceeded — and theTaskArrayRunbranch adds Nqueue entries for a single incremented handler.
ParallelPollingMonitorroutes submit failures through its ownRecoverable.onFailure()rather than thetry/catchinsubmitPendingTasks(),so it has the identical leak and gets the same release. Its early return on
!session.successis left alone — that is separate behaviour and not this bug.Testing
Two Spock specs, one per affected call path.
Per the note in the issue,
Mock(TaskHandler)is unusable here: it stubs thenon-final
canForkProcess()to false so nothing is ever submitted, and itintercepts the
finalincProcessForks()/decProcessForks()bodies so thecounter never moves and the assertion passes vacuously. Both tests use
Spy(TaskHandler)over a realLongAdder.TaskPollingMonitorTestalso neededpendingLock/taskAvail/slotAvailset by hand, sinceevict()dereferencesthem and they are only created in
start().Reverting both source changes and re-running the new tests:
The other 34 tests in those two classes pass with the fix reverted, so the two
failures are specific to the leak and not to the harness.
With the fix applied:
Fixes #7447