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 |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
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
4f72b01 to
dcb197d
Compare
Replays the amended PR pingdotgg#4378 wake-settlement fix onto the v2.1 trial. A terminal task notification no longer opens an opaque-task continuation unless native user, assistant, or result output proves Claude began the wake turn.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
| if (yield* Ref.get(providerThreadOwnerLost)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Owner loss stops background item ingest
High Severity
providerThreadOwnerLost forces ingestion to stop before the activeBackgroundTurnItems keep-alive check. A rejected post-terminal provider_thread.updated (for example when a newer run advances lastRunOrdinal on the same thread) therefore closes the stream while finite background turn items for the settled run are still non-terminal, so their completion events can miss the projection and leave Waiting or spinners stuck.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit dcb197d. Configure here.
ApprovabilityVerdict: Needs human review This PR introduces a significant new feature (surfacing background work as 'Waiting' state) with cross-cutting changes across orchestration, persistence, and UI layers. Two unresolved review comments identify potential bugs, including a high-severity issue where background item ingestion may stop prematurely. You can customize Macroscope's approvability policy. Learn more. |
bd45aae to
29cfba8
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 29cfba8. Configure here.
| providerThreads: serverProjection.providerThreads, | ||
| turnItems: serverProjection.turnItems, | ||
| activeProviderThreadId: serverProjection.thread.activeProviderThreadId, | ||
| }), |
There was a problem hiding this comment.
Waiting includes rolled-back run work
Medium Severity
derivePendingBackgroundWork is fed raw turnItems, which still include items from rolled_back runs. Those abandoned nonterminal items are treated as pending background work, so the chat timeline can show Waiting for work that will never finish. The SQL shell path already excludes rolled-back runs for this reason; threadShellFromProjection has the same gap.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 29cfba8. Configure here.
| return false; | ||
| } | ||
| return TERMINAL_RUN_STATUSES.has(latestRun.status); | ||
| } |
There was a problem hiding this comment.
Waiting gated past checkpoint
Medium Severity
derivePendingBackgroundWork treats only terminal run statuses as settled and omits waiting. After a successful provider turn, writeFinalRunEvents persists the run as waiting until checkpoint capture flips it to completed, so shell and ChatView pending-task lists stay empty for that whole window. The sidebar keeps a pulsing Working pill even though the client already parks runtime idle when a nonempty roster arrives with checkpoint-oriented waiting.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 29cfba8. Configure here.
1 participant
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
Manual test guide
Browsable scenario guide: https://nam7nt0rbtm6.postplan.dev/
This PR owns the Waiting state, so most background scenarios in the guide assert
it. The ones that matter, across all three providers:
Waiting must appear
5, Background subagent post-settle,
6, Resume completed subagent,
7, App-owned delegated child.
4,
5,
6,
7. Scenarios 5 and 6
are the two-task cases, where Waiting must report both.
and 5, Steer while run is held open.
Waiting must NOT appear. These are the false-positive guards, and they are
the easier half to regress:
3,
4: work that
finishes before the root settles.
same, for a background command.
and 3: persistent Grok
monitors are intentionally excluded from the roster.
Known failure, not caused by this PR
After a mid-turn steer the row can go blank, showing neither Working nor
Waiting. That is the status resolver correctly reporting a terminal run and an
empty roster; the run settled early upstream of it. Tracked separately, and
deliberately not patched here, because showing Working without a live run
would mislabel genuinely idle threads.
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.
Note
High Risk
Large changes to post-settle run ingestion, Claude wake/continuation gating, and provider-thread persistence; incorrect probes or ordering could terminate streams early, spawn spurious runs, or show stale Waiting state.
Overview
Adds
pendingBackgroundTaskson provider threads and thread shells so the UI can show Waiting after a root turn settles while finite background work is still in flight.Claude adapter gains per-native-thread roster tracking from
background_tasks_changed/task_started, emits roster-bearingprovider_thread.updatedevents, splits Waiting roster from wake eligibility (with replay tombstones forlocal_bash), gates opaque continuations until native assistant/user/result proves a wake turn started, and resets roster/wake state on query replace/failure.hasPendingBackgroundWorkForThreadscopes root-run stop gates to one native thread.ACP adapter extends
hasPendingBackgroundWorkto count live carryover subagents so idle release does not drop post-settle Grok work.Projection / shell uses shared
derivePendingBackgroundWork(settled runs only; excludes rolled-back runs).writeIfProviderThreadOwnerprevents stale post-terminal roster writes from overwriting a newer run.Recovery cancels orphaned background turn items (including linked subagents/nodes) and clears persisted rosters after process loss.
Extensive adapter and persistence tests cover roster lifecycle, notification-only wakes, sibling process replacement, and ownership guards.
Reviewed by Cursor Bugbot for commit 29cfba8. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Surface waiting background work in thread status and timeline UI
derivePendingBackgroundWorkfunction inpackages/sharedthat produces a deduplicated list of pending background tasks after a run settles, combining provider-thread rosters and nonterminal turn items.OrchestrationV2ProviderThreadandOrchestrationV2ThreadShellcontracts to carry apendingBackgroundTasksarray (defaults to[]for historical data).provider_thread.updatedwithpendingBackgroundTasksat turn end, and gates opaque-task continuations on evidence of a native wake frame.RunExecutionServicekeeps the root-run stream open after terminal while the provider thread reports pending background work, and closes early if provider-thread ownership is lost.'Waiting'status pill (priority 2.5) to the sidebar for idle threads with a non-empty background task roster.'waiting-background'row variant to the messages timeline, rendered as a pulsing indicator with a label when not working and tasks are pending.pendingBackgroundTasksfrom provider threads.hasPendingBackgroundWorkForThread; a probe failure is treated as no pending work and allows termination.Macroscope summarized 29cfba8.