feat(orchestrator): Surface waiting background work#4378
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 Plus 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 |
There was a problem hiding this comment.
🟡 Medium
isBackgroundCapableTurnItemType causes replayable waiting runs to have their background-capable turn items (command_execution, dynamic_tool, subagent) cancelled, even though those runs were intentionally skipped from terminalization because a pending/running checkpoint effect still exists. When a waiting run has a replayable checkpoint, reconcileProjection omits it from runs, so its ID is absent from recoveredNonterminalRunIds. The later stale-item loop then matches that run's items via isBackgroundCapableTurnItemType + isNonterminalTurnItemStatus and emits turn-item.updated cancellation events, destroying items that belong to a still-recoverable run and checkpoint. The stale-item loop needs to also exclude turn items whose runId belongs to a replayable waiting run, not just those already in recoveredNonterminalRunIds.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/ProviderRuntimeRecoveryService.ts around line 304:
`isBackgroundCapableTurnItemType` causes replayable `waiting` runs to have their background-capable turn items (command_execution, dynamic_tool, subagent) cancelled, even though those runs were intentionally skipped from terminalization because a pending/running checkpoint effect still exists. When a `waiting` run has a replayable checkpoint, `reconcileProjection` omits it from `runs`, so its ID is absent from `recoveredNonterminalRunIds`. The later stale-item loop then matches that run's items via `isBackgroundCapableTurnItemType` + `isNonterminalTurnItemStatus` and emits `turn-item.updated` cancellation events, destroying items that belong to a still-recoverable run and checkpoint. The stale-item loop needs to also exclude turn items whose `runId` belongs to a replayable `waiting` run, not just those already in `recoveredNonterminalRunIds`.
There was a problem hiding this comment.
Thanks for the careful trace. The control flow you describe is accurate, but I don't think the cancellation is a defect, so I'm leaving this as-is.
A replayable checkpoint.capture cannot revive the provider process or the background work it owned. Its only run-level action is finalizing the already-settled root run: CheckpointCaptureService emits run.updated with status: "completed". So on restart, the command_execution / dynamic_tool / subagent items belonging to that run are orphaned regardless, because the process that could complete them is gone.
That makes cancelling them the correct cleanup rather than the destruction of recoverable work. The skip set exists to avoid emitting duplicate cancellation events for runs already terminalized in the loop above, not to exempt preserved runs from item cleanup. Exempting them as suggested would leave permanently nonterminal cards for work that can never finish, which is a worse outcome than the current behavior: a stuck spinner with no path to resolution.
The sequence on restart during that window is: background items are cancelled, the roster clears, and the durable checkpoint effect replays and completes the root run. That matches "the server died while background work was in flight," which is what happened.
I did act on your neighbouring comment about the same loop (the missing subagent.updated for settled runs); that one is a real gap and is being fixed.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
e46af0a to
f318877
Compare
1e58e65 to
a286c60
Compare
f318877 to
2bf4cdf
Compare
2987fd6 to
4f72b01
Compare
4f72b01 to
dcb197d
Compare
Summary
background work remains.
and Claude subagents.
Bash.
web sidebar while preserving Working for an active turn.
Relationship to #4415
This is the orchestrator-v2 counterpart to Theo's
#4415. That PR owns the current
SidebarV2 and mobile-v2 Waiting presentation. This PR carries the equivalent
provider-thread and projection contract for orchestrator v2.
The active desktop build still renders the classic
Sidebar.tsxlist, whichdoes not consume #4415's SidebarV2 resolver. This PR adds only the shared
classic status-pill bridge required to display the already-normalized v2 roster.
It does not modify SidebarV2 or the native mobile list.
Problem and Fix
pendingBackgroundTasks, after active Working and Connecting precedence.pendingBackgroundTaskswith an empty-array default for backward compatibility.Defensive Fixes
Validation
suites pass. The final Claude adapter suite passes 53 tests, and the
classic-sidebar suite passes 87 tests.
typecheck passes, and
git diff --checkpasses.subagents, Codex background exec and subagents, and Grok continuations and
subagents.
the classic sidebar row displayed static, muted Waiting after root settlement,
then returned to its timestamp after the roster cleared.
that ended with notification-only traffic. Waiting cleared after run 2, no
empty run 3 appeared, and the provider stayed idle through a 35-second
residual window.
notification-only path and its regression guards. Two independent scenarios
drove a real nonpersistent Monitor whose terminal notification arrived with no
native wake frames: both cleared Waiting without opening a run, and each held
at two completed runs, idle with an empty roster, through a residual window of
about 176 seconds. The prior defect left an empty run active for 225 seconds.
once across a separate follow-up turn, a resumed subagent still wakes, and Stop
during a blocking TaskOutput still leaves two distinct clean recovery turns.
Every wake log marker in the round was attributable to an expected wake, with
none unattributed.
server: the row rendered a static muted Waiting pill with dot class
bg-sidebar-muted-foregroundand no pulse while the roster was nonempty, andtitle plus timestamp only after it cleared. An adjacent thread rendered Working
in the same view, confirming the states are visually distinct.
RunExecutionServiceoverlap that will need conflict resolution if thebranches are combined.
The failing Check job is the known Vite+ stdout panic ("This is a bug in Vite+,
not your code") raised while printing pre-existing warnings, not a lint or type
error from this branch.
vp checkon this exact tree reports 0 errors and 70pre-existing warnings, and Test, Release Smoke, Mobile Native Static Analysis,
and both Macroscope checks pass on this head.
This branch is based on the live
t3code/codex-turn-mappingtip. It does notsemantically depend on #4218.