[orchestrator-v2] fix(orchestrator): Prevent Claude session release from hanging on idle CLI reads#3756
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| const idAllocator = yield* IdAllocatorV2; | ||
| const queryRunner = yield* ClaudeAgentSdkQueryRunner; | ||
| const serverConfig = yield* ServerConfig; | ||
| const continuationRequests = yield* ProviderContinuationRequests; |
There was a problem hiding this comment.
🟠 High Adapters/ClaudeAdapterV2.ts:3829
ClaudeAdapterV2Driver.create reads ProviderContinuationRequests from the context, but this is a Context.Reference whose default drops all offers. In production, providerAdapterRegistryLayerFromProviderInstances does not provide providerContinuationRequestsLayer, so the driver receives the default sink. Every Claude adapter then stores offer: () => Effect.void, and all background-task wake turns are silently dropped instead of reaching ProviderContinuationService — the continuation mechanism never runs outside tests. Make the production registry layer provide providerContinuationRequestsLayer, or change the driver to require a non-default implementation so the missing provider fails loudly instead of silently dropping work.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.ts around line 3829:
`ClaudeAdapterV2Driver.create` reads `ProviderContinuationRequests` from the context, but this is a `Context.Reference` whose default drops all offers. In production, `providerAdapterRegistryLayerFromProviderInstances` does not provide `providerContinuationRequestsLayer`, so the driver receives the default sink. Every Claude adapter then stores `offer: () => Effect.void`, and all background-task wake turns are silently dropped instead of reaching `ProviderContinuationService` — the continuation mechanism never runs outside tests. Make the production registry layer provide `providerContinuationRequestsLayer`, or change the driver to require a non-default implementation so the missing provider fails loudly instead of silently dropping work.
mwolson
force-pushed
the
fix/claude-idle-release-hang
branch
from
f0bd0f5 to
e94b5f4
Compare
There was a problem hiding this comment.
🟡 Medium
releaseIfStillIdle reuses the stale pinnedSinceMs when re-arming the idle timer after deferring for pending background work. Activity paths that go through touchActivity or markIdle (e.g. respondToRuntimeRequest, steerTurn, interruptTurn, or provider events) never reset pinnedSinceMs — only markBusy clears it. So a session that had pending work, then later receives fresh activity without ever becoming busy, will still carry the original pin timestamp. The next idle check sees now - pinnedSinceMs >= maxIdlePinMs and immediately releases the session, even though it had recent activity and still reports pending background work. Resetting pinnedSinceMs to null in touchActivity would clear the stale pin on fresh activity.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProviderSessionManager.ts around line 712:
`releaseIfStillIdle` reuses the stale `pinnedSinceMs` when re-arming the idle timer after deferring for pending background work. Activity paths that go through `touchActivity` or `markIdle` (e.g. `respondToRuntimeRequest`, `steerTurn`, `interruptTurn`, or provider events) never reset `pinnedSinceMs` — only `markBusy` clears it. So a session that had pending work, then later receives fresh activity without ever becoming `busy`, will still carry the original pin timestamp. The next idle check sees `now - pinnedSinceMs >= maxIdlePinMs` and immediately releases the session, even though it had recent activity and still reports pending background work. Resetting `pinnedSinceMs` to `null` in `touchActivity` would clear the stale pin on fresh activity.
| }, | ||
| ); | ||
|
|
||
| yield* requests.take.pipe( |
There was a problem hiding this comment.
🟡 Medium orchestration-v2/ProviderContinuationService.ts:56
The worker loop permanently drops a continuation request whenever dispatchContinuation fails. requests.take removes the item from the queue, but the Effect.catchCause handler only logs the failure and never retries or re-enqueues it — so the buffered provider wake event is lost and never ingested as a run. This is reachable via the queue_after_active dispatch path, which rejects dispatch while a thread has pending merge-back transfers. Consider re-enqueuing the request (or retrying with backoff) before logging so the wake event survives a transient rejection.
Also found in 1 other location(s)
apps/server/src/orchestration-v2/ProviderSessionManager.ts:44
Using the new
RELEASE_SCOPE_CLOSE_TIMEOUT_MScausesreleaseEntry/close()to report success after 30s even whenScope.close(entry.scope, Exit.void)later fails. OnceEffect.timeoutOption(...)returnsNone, the code only logs the eventualExitin a detached observer and never re-raises it asProviderSessionReleaseError/ProviderSessionCloseError, so callers cannot detect adapter cleanup failures anymore on any close path that finishes after the timeout.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProviderContinuationService.ts around line 56:
The worker loop permanently drops a continuation request whenever `dispatchContinuation` fails. `requests.take` removes the item from the queue, but the `Effect.catchCause` handler only logs the failure and never retries or re-enqueues it — so the buffered provider wake event is lost and never ingested as a run. This is reachable via the `queue_after_active` dispatch path, which rejects dispatch while a thread has pending merge-back transfers. Consider re-enqueuing the request (or retrying with backoff) before logging so the wake event survives a transient rejection.
Also found in 1 other location(s):
- apps/server/src/orchestration-v2/ProviderSessionManager.ts:44 -- Using the new `RELEASE_SCOPE_CLOSE_TIMEOUT_MS` causes `releaseEntry`/`close()` to report success after 30s even when `Scope.close(entry.scope, Exit.void)` later fails. Once `Effect.timeoutOption(...)` returns `None`, the code only logs the eventual `Exit` in a detached observer and never re-raises it as `ProviderSessionReleaseError`/`ProviderSessionCloseError`, so callers cannot detect adapter cleanup failures anymore on any close path that finishes after the timeout.
…on runs When a background task (run_in_background Bash) settles, the Claude CLI streams a whole new turn over the still-open SDK query. handleSdkMessage dropped every message of that turn (activeTurn null), and the 30 minute idle release killed the CLI child before pending tasks could even wake. Buffer null-activeTurn messages per native thread, request one internal continuation run per wake (message.dispatch with createdBy agent / creationSource provider via a new ProviderContinuationRequests queue and worker), and drain the buffer into the continuation turn without sending a prompt to the CLI. Pin idle provider sessions while background work is pending via a new optional hasPendingBackgroundWork runtime capability, capped by maxIdlePinMs (default 4h).
mwolson
force-pushed
the
fix/claude-idle-release-hang
branch
from
e94b5f4 to
6c7e27e
Compare
mwolson
force-pushed
the
fix/claude-idle-release-hang
branch
from
6c7e27e to
90e67c3
Compare
| ordinal: projection.messages.length + 1, | ||
| }); | ||
| const commandId = CommandId.make(`provider-continuation:${messageId}`); | ||
| yield* threads.dispatch({ |
There was a problem hiding this comment.
🟡 Medium orchestration-v2/ProviderContinuationService.ts:42
dispatchContinuation discards the providerThreadId and driver from the ProviderContinuationRequest, dispatching a generic message.dispatch against the thread's current routing. If the thread's active provider changes before the worker drains the request (e.g. after a provider switch or handoff), the synthetic wake turn starts on the wrong provider thread while the buffered provider-native wake messages remain attached to the original provider thread and are never ingested. Consider passing request.providerThreadId and request.driver through to threads.dispatch so the continuation targets the correct provider.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProviderContinuationService.ts around line 42:
`dispatchContinuation` discards the `providerThreadId` and `driver` from the `ProviderContinuationRequest`, dispatching a generic `message.dispatch` against the thread's current routing. If the thread's active provider changes before the worker drains the request (e.g. after a provider switch or handoff), the synthetic wake turn starts on the wrong provider thread while the buffered provider-native wake messages remain attached to the original provider thread and are never ingested. Consider passing `request.providerThreadId` and `request.driver` through to `threads.dispatch` so the continuation targets the correct provider.
There was a problem hiding this comment.
🟡 Medium
When a session has pending background work, releaseIfStillIdle calls scheduleIdleReleaseInternal, which immediately calls cancelIdleFiber(entry.idleFiber). Because entry.idleFiber still points to the currently running idle-release fiber, the fiber interrupts itself and then joins its own interruption, deadlocking forever. Any session that defers idle release for pending background work will wedge instead of rescheduling. Consider passing cancelIdleFiber: false to the reschedule path (or skipping the cancel when the current fiber is the idle fiber) so the self-interrupt does not happen.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProviderSessionManager.ts around line 673:
When a session has pending background work, `releaseIfStillIdle` calls `scheduleIdleReleaseInternal`, which immediately calls `cancelIdleFiber(entry.idleFiber)`. Because `entry.idleFiber` still points to the currently running idle-release fiber, the fiber interrupts itself and then joins its own interruption, deadlocking forever. Any session that defers idle release for pending background work will wedge instead of rescheduling. Consider passing `cancelIdleFiber: false` to the reschedule path (or skipping the cancel when the current fiber is the idle fiber) so the self-interrupt does not happen.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 90e67c3. Configure here.
| providerSessionId: input.providerSessionId, | ||
| reason: "idle_timeout", | ||
| cancelIdleFiber: false, | ||
| onlyIfIdleGeneration: input.generation, |
There was a problem hiding this comment.
Idle release stale pending check
High Severity
releaseIfStillIdle decides not to defer when hasPendingBackgroundWork is false, then calls releaseEntry without re-checking pending work. Wake output can land in the adapter buffer after that read but before the session is torn down, so idle release can drop a live Claude session while wake messages and a queued continuation are still in memory.
Reviewed by Cursor Bugbot for commit 90e67c3. Configure here.
| ? false | ||
| : yield* entry.runtime.hasPendingBackgroundWork.pipe( | ||
| Effect.catchCause(() => Effect.succeed(false)), | ||
| ); |
There was a problem hiding this comment.
Pending work errors imply none
Medium Severity
When hasPendingBackgroundWork fails, releaseIfStillIdle treats the session as having no pending work via Effect.catchCause(() => Effect.succeed(false)), so idle release can proceed while wake buffers or background tasks may still be active.
Reviewed by Cursor Bugbot for commit 90e67c3. Configure here.
| providerThreadId: route.providerThreadId, | ||
| driver: CLAUDE_PROVIDER, | ||
| detail, | ||
| }); |
There was a problem hiding this comment.
Failed continuation blocks retry
Medium Severity
When a wake is detected, the Claude adapter records the native thread in requestedContinuations before enqueueing a continuation request. If ProviderContinuationService later skips or fails dispatch (for example archived thread, orchestration error, or logged dispatch-failed), nothing clears that flag or sends another request. Further wake output stays buffered, hasPendingBackgroundWork remains true, and the wake may never run until idle pin expiry or session release.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 90e67c3. Configure here.
ApprovabilityVerdict: Needs human review 5 blocking correctness issues found. This PR introduces significant new capability (background task wake handling, session pinning, continuation service) beyond a simple fix. Multiple unresolved review comments identify high-severity issues including production misconfiguration causing silent drops and potential deadlocks in the idle release path. You can customize Macroscope's approvability policy. Learn more. |
1 participant
Stacked on #3752 (which stacks on #3750); the last commit is this PR's delta.
Summary
Releasing an idle Claude provider session could deadlock silently after
removing the in-memory entry: the CLI child process stayed alive
indefinitely, no released event was persisted, the thread projection's
provider session stayed
"ready", and nothing was logged. The stale CLIkeeps holding the native session file, which is the likely original
mechanism behind
Session ID ... is already in use(#3750 works around thatsymptom by forcing
--resume; this PR fixes the leak itself).Discovered while live-verifying #3750/#3752 against a real 30-minute idle
release: the release fired (the entry was deleted), then wedged before
query.close, leaving the CLI orphaned.Problem and Fix
openQuery) is interrupted before thecloseSessionfinalizer that callsquery.close. Interruption runs Effect'sChannel.fromAsyncIterablefinalizerEffect.promise(() => iter.return()), anditeris the SDKQuery's rawsdkMessagesasync generator (that is whatQuery[Symbol.asyncIterator]()returns).return()on a generator suspended at an internalawait(reading the next message from an idle CLI) queues behind the in-flight read forever, so the scope close never completes andquery.closeis never reached.claudeQueryMessageshands the stream an iterable whose[Symbol.asyncIterator]()returns theQueryitself. The SDK'sQuery.return()runscleanup()first, which closes the transport, terminates the CLI child, and unblocks the pending read, so interruption completes and the remaining finalizers run.Query.next()already delegates tosdkMessages.next(), so streaming behavior is unchanged.Defensive Fixes
releaseEntryforever with no released events and no log line, leaving the projection stuck on"ready".releaseEntryforks the scope close (Effect.forkDetach({ startImmediately: true })) and joins with a 30s timeout. On timeout it logsprovider-session-scope-close-timeout, attaches a detached observer that logs late completion or failure, and persists the released events regardless.startImmediatelykeeps well-behaved synchronous finalizer stacks completing on the same tick, and close failures still propagate after event writes exactly as before.Known limitations
if a close still wedges (a future adapter regression), the projection
shows "stopped" while the provider process may linger; the
provider-session-scope-close-timeoutwarning is the operational signal.Close failures that finish after the timeout are logged by the detached
observer rather than surfaced as
ProviderSessionReleaseError.step revokes MCP credentials thread-wide, so a replacement provider
session for the same thread opened during the close window could have its
fresh credentials revoked when the old release finishes. The window
already existed (entry removal precedes close and MCP cleanup); per-entry
credential ownership is a candidate follow-up. With the root-cause fix in
place the Claude close path no longer wedges, so hitting the widened
window requires a new bug.
Validation
vp check: 0 errors;vp run typecheck: exit 0 (full monorepo)ClaudeAdapterV2.test.ts: 20/20, including a new regression test thatinterrupts the message stream while a read is in flight; with the wrapper
reverted to the raw generator the test deadlocks and times out at 60s
(verified during development)
ProviderSessionManager.test.ts: 19/19, including a new test where ahanging session-scope finalizer no longer blocks release: released events
persist, and the wedged finalizer is confirmed via the adapter close count
staying 0
ClaudeReplayFixtures.integration.test.ts: 4 pass / 1 skipped;OrchestratorReplayFixtures.integration.test.ts -t "claude": 16 pass /53 skipped
Note
High Risk
Touches Claude session lifecycle, idle release, and turn/continuation ordering in orchestration-v2; regressions could orphan CLI processes, drop wake output, or release sessions while background work is still pending.
Overview
Fixes silent hangs when releasing idle Claude provider sessions: the message stream now iterates the SDK
Query(viaclaudeQueryMessages) so interruption runs transport cleanup instead of deadlocking on the raw generator’sreturn(), andProviderSessionManagertimes out scope close (30s), still persists released events, and logs when cleanup wedges.Adds Claude background “wake turn” handling: SDK messages with no active turn are buffered, a single
ProviderContinuationRequestis offered, and a worker dispatches a provider-originated run that replays buffered output (no duplicate CLI prompt). User turns that race a wake queue behind active work;hasPendingBackgroundWorkdefers idle release with a pin cap and re-validates idle generation before release.Resumes the native CLI session when
providerTurnOrdinal > 1so a fresh manager instance after idle release does not reopen with a fixed session id (“already in use”). Extensive adapter and session-manager tests cover wake flows, hung close, and idle pinning.Reviewed by Cursor Bugbot for commit 90e67c3. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix Claude session release hanging on idle CLI reads by introducing background wake turn buffering
claudeQueryMessagesin ClaudeAdapterV2.ts to iterate the Claude Query runtime (not the raw generator), ensuringQuery.return()runs transport cleanup before the in-flight read.ProviderContinuationRequestis issued per wake event, and a provider-authored continuation turn drains buffered messages without re-prompting the CLI.ThreadManagementServiceto trigger continuation runs.hasPendingBackgroundWorkis true, capped bymaxIdlePinMsto prevent indefinite pinning; idle generation guards prevent races with newly busy sessions.Macroscope summarized 90e67c3.