fix: SSH-race and remote-resource-leak fixes around stopProject/runProject (aicshud/WHEEL#1020, #1021, #1022) - #140
Merged
so5 merged 7 commits intoSep 16, 2026
Conversation
Adds failing tests demonstrating that runProject() and stopProject() independently call removeSsh()/removeExecuters()/removeTransferrers() (and runProject() also runDeferredCleanups()), which race each other when a project is stopped mid-run: Dispatcher.pause() resolves start()'s promise (via a synchronous "stop" emit) well before its own, still-in-flight nested job-task cancellation (pjdel etc., which needs the project's SSH connections) actually completes, so runProject()'s teardown can disconnect SSH out from under stopProject()'s still- running cancellation - observed as "ssh instance is not registerd for the project", which aborts onStopProject() before it ever reaches its own explicit "stopped" state write. - dispatcher.js: Dispatcher#start()'s onStop handler should set a new `stoppedExternally` flag before resolving, so callers can tell this settled via an external stop rather than natural completion. - core/projectController.js: runProject() should skip its own removeSsh/removeExecuters/removeTransferrers/runDeferredCleanups when the dispatcher was stopped externally, deferring entirely to stopProject()'s own (already fully-awaited) teardown instead. Confirmed red: 3 failing (server/scratchpad/jobmanager-repro-test-output.log). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu
…shud/WHEEL#1020) Dispatcher#start()'s onStop handler now sets `stoppedExternally = true` before resolving, recording that this settlement came from an external stop (pause()/remove(), as triggered by stopProject()) rather than the dispatcher's own natural completion. pause() emits "stop" synchronously before awaiting its own nested job-task cancellation (pjdel etc., which needs the project's SSH connections), so start()'s promise - and therefore runProject()'s await on it - can resolve well before that cancellation actually finishes. runProject() now skips its own removeSsh/removeExecuters/ removeTransferrers/runDeferredCleanups teardown when rootDispatcher.stoppedExternally is set, instead deferring entirely to stopProject()'s own teardown (which only runs those once its own await on rootDispatcher.remove() - including all nested pause() calls - has fully finished). Previously, runProject()'s teardown could win the race and disconnect SSH out from under stopProject()'s still-running nested cancellation, surfacing as "ssh instance is not registerd for the project" and aborting onStopProject() before it ever reached its own explicit "stopped" state write. Full server test suite: 1656 passing, 0 failing, 15 pending, including both new #1020 reproduction tests. Test output: server/scratchpad/jobmanager-repro-test-output.log Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu
…oo (aicshud/WHEEL#1020) The previous #1020 commit (f2996dc7) only guarded core/projectController.js's runProject() against redundantly tearing down SSH/executers/transferrers when the dispatcher was stopped externally. It missed a third, independent call site: handlers/projectController.js's runDispatcher() - the function that actually calls runProject() - had its own unconditional removeSsh(projectRootDir) in a shared `finally` block that runs on every exit path (success, external stop, or error), completely bypassing runProject()'s stoppedExternally guard. Since runProject() now returns almost immediately once "stop" is emitted, this outer finally block's removeSsh could still fire while stopProject()'s own, separate (concurrent, fire-and-forget) invocation was still mid-flight cancelling nested job tasks over SSH - reproducing the exact same "ssh instance is not registerd for the project" error the first commit was meant to fix. stopProject() only ever settles a Dispatcher's start() via the "stop" event (never "error"), so runProject() never throws when a stop (user-initiated or task-failure-triggered) is in progress - meaning this outer removeSsh call is only ever needed as a safety net for the genuine, rare case where runProject() itself threw before reaching its own cleanup (e.g. rootDispatcher.start() rejecting via the dispatcher's "error" event). Moved it (and removeAllJWTServerPassphrase) from the shared `finally` into the `catch` block, where it can't race a concurrent stopProject() at all - runProject() now unconditionally owns cleanup on every non-throwing exit. Verified on Fugaku with debug instrumentation (temporary, removed before this commit): confirmed removeSsh was called exactly once, from stopProject()'s own (correct) call site, with no "ssh instance is not registerd" error, and the project's persisted state ended up correctly as "stopped". Repeated 3 consecutive clean->run->stop cycles with no recurrence (this was a race condition, hence the repeat). Full server test suite: 1656 passing, 0 failing, 15 pending. Test output: server/scratchpad/jobmanager-repro-test-output.log Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu
Adds a failing test demonstrating that stopProject() discards pending deferred remote cleanups (files preserved as remote-symlink targets for a not-yet-executed downstream task) via clearDeferredCleanups(). This permanently leaks them: a resumed run never re-registers the entry (finished components are skipped on restart), and cleanProject only touches local git state, never the remote host - so once an entry is cleared here, nothing ever deletes the corresponding remote files. Confirmed red: registering a deferred-cleanup entry, calling stopProject(), then calling runDeferredCleanups() again finds nothing to process (getSsh never called), because stopProject() already wiped the registry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu
…ups on stop stopProject() called clearDeferredCleanups(), which silently discards any entries in the deferred-cleanup registry without deleting the remote files they describe. Those entries exist precisely because a finished task's output was delivered downstream via a remote symlink whose target must be preserved for a not-yet-executed consumer; discarding the entry on stop means that consumer's target file leaks on the remote host forever, since no later resume-to-completion or cleanProject can ever re-register it. Simply stop calling clearDeferredCleanups() in stopProject() and leave the registry untouched, so a later natural completion (after 0+ resumes) can still process it via runDeferredCleanups(). Running the deferred cleanup immediately at stop time was considered and rejected: at stop time we cannot safely tell whether the not-yet-executed downstream consumer has already run, so deleting immediately risks removing a file it still needs. Ref: aicshud/WHEEL#1021 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu
runDispatcher()'s catch block (handlers/projectController.js) is the last chance to release SSH connections/executers/transferrers when runProject() throws before ever reaching its own cleanup (e.g. rootDispatcher.start() itself rejecting, not a task failure). It only ever called removeSsh(), never removeExecuters()/removeTransferrers() - leaking stale executer/ transferrer map entries that a subsequent run attempt could reuse. Ref: aicshud/WHEEL#1022 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu
…s/removeTransferrers cleanup set Every path that tears down a project run's SSH connections, executers, and transferrers (natural completion, an external stop, and a fatal dispatch error) used to hand-roll its own subset of these three calls, spread across core/projectController.js's stopProject()/runProject() and handlers/projectController.js's runDispatcher(). This is how runDispatcher()'s catch block came to only call removeSsh() and silently skip removeExecuters()/removeTransferrers() (aicshud/WHEEL#1022) - a genuine implementation gap, not an intentional omission, since both functions are pure in-memory Map cleanups with no side effects that make them unsafe to call unconditionally. Introduce releaseRuntimeResources(projectRootDir) in core/projectController.js as the single shared implementation of this 3-point cleanup set, and call it from all three sites: - stopProject() - runProject()'s guarded tail (only on natural completion, unchanged from aicshud/WHEEL#1020) - runDispatcher()'s catch block, now also releasing executers/transferrers This is a pure consolidation of already-reviewed cleanup logic (no new runtime behavior beyond closing the #1022 gap above) - deferred cleanups (runDeferredCleanups) and JWT passphrase cleanup are deliberately left out of this shared function, since they are not called uniformly from all three sites and have their own separate semantics. Ref: aicshud/WHEEL#1022 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu
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.
Summary
This PR fixes three related bugs around SSH connection and remote-resource lifecycle management during
stopProject()/runProject(), found while investigating a customer-reported resume failure.aicshud/WHEEL#1020: a race betweenstopProject()'s teardown andrunDispatcher()'s own error-path teardown, both of which could callremoveSsh()concurrently for the same project, tearing down an SSH connection that the other path still needed (or vice versa).aicshud/WHEEL#1021:stopProject()was unconditionally clearingdeferredCleanupRegistryentries on stop, discarding pending remote-resource cleanups (executers/transferrers) that had not actually run yet — leaking those remote resources.aicshud/WHEEL#1022: consolidated the three cleanup calls that always belong together (removeSsh,removeExecuters,removeTransferrers) into a singlereleaseRuntimeResources()helper incore/projectController.js, and used it to close a gap whererunDispatcher()'s fatal-error catch block called onlyremoveSsh(), neverremoveExecuters()/removeTransferrers().Commits
test: reproduce aicshud/WHEEL#1020 (red)fix: stop runDispatcher()'s own removeSsh from racing stopProject() too (aicshud/WHEEL#1020)fix: don't race stopProject()'s teardown with runProject()'s own (aicshud/WHEEL#1020)test: reproduce aicshud/WHEEL#1021 (red)fix(projectController): stop discarding pending deferred remote cleanups on stoptest: reproduce aicshud/WHEEL#1022 (red)refactor(projectController): consolidate the removeSsh/removeExecuters/removeTransferrers cleanup setEach fix follows a test-first (red → fix → green) pattern; the reproduction tests remain in the suite as regression coverage.
Testing
npm run testDocker -w server— full suite green (1658 passing).Related
Filed against
aicshud/WHEEL#1020,#1021,#1022on GitLab (issue tracker for this project). These will be closed manually with a link to this PR once merged, since GitLab and GitHub are separate platforms with no automatic cross-linking.🤖 Generated with Claude Code
https://claude.ai/code/session_01C3jKNM1qubomM8UTRdkEWu