fix(server): bound Codex subagent progress events - #5682
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:
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c74ae21515
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Effect.flatMap((nextValue) => | ||
| nextValue === null ? Effect.void : processKey(key, nextValue), | ||
| Effect.flatMap((next) => | ||
| next === null ? Effect.void : processKey(key, next.value, next.flush), |
There was a problem hiding this comment.
Yield hot keys before processing their next value
When one Codex child produces progress at least once per 250 ms debounce window, latestByKey remains populated and this recursive call immediately processes that same key again instead of returning it to the queue. Normal progress for every other queued child can therefore remain stale indefinitely until the hot child pauses or a terminal event invokes flushKey; requeue the next value so the worker can service keys round-robin.
AGENTS.md reference: AGENTS.md:L142-L142
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 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 c74ae21. Configure here.
| Effect.catchCause(() => cleanupFailedKey(key)), | ||
| ), | ||
| ), | ||
| Effect.andThen(drainKey(key)), |
There was a problem hiding this comment.
Flush swallows interrupt on close
High Severity
flushKey wraps processKey in Effect.catchCause that recovers from all causes, including fiber interrupts. Codex runs flushKey on the notification consumer fiber and now closes runtimeScope before queue shutdown, so a close that lands mid-flush can be swallowed. The consumer then keeps waiting on serverNotifications while Scope.close waits for that fiber, deadlocking session teardown.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit c74ae21. Configure here.
|
|
||
| return [ | ||
| { key, value } as const, | ||
| { key, value, flush: state.flushRequestedKeys.has(key) } as const, |
There was a problem hiding this comment.
Flush races with stale queue take
Medium Severity
flushKey can steal a queued key and run processKey on the caller fiber while leaving a stale TxQueue entry. The queue consumer only checks whether latestByKey still has a value, not whether the key is already in activeKeys. A concurrent enqueue during that flush can repopulate latestByKey, so the stale take starts a second processor for the same key and can clear activeKeys early, letting drainKey finish before progress emission completes.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c74ae21. Configure here.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR introduces a new concurrent KeyedCoalescingWorker pattern to debounce subagent progress events with flush/drain semantics, and modifies session lifecycle timing. Multiple unresolved review comments flag potential deadlocks and race conditions in the new concurrency logic. The substantial new runtime behavior and open correctness concerns warrant human review. You can customize Macroscope's approvability policy. Learn more. |


Closes #5681
What Changed
KeyedCoalescingWorkerwithout changing existingdrainKeybehavior.Why
Native Codex subagent observability currently emits every child
item/started,item/completed, and cumulative token-usage snapshot as a separate provider event. The adapter maps each event totask.progress, and provider ingestion persists every update as a durable activity append.Stable task activity IDs keep projected row count small, but do not reduce event-store or command-processing work. A busy subagent fleet can therefore fill the serial ingestion path with redundant progress snapshots and delay unrelated threads.
Coalescing belongs at the provider boundary because these events are latest-state snapshots: the UI needs the newest item summary and cumulative usage, not every intermediate value. Terminal lifecycle events still flush the latest pending values first, so task state cannot overtake its final progress.
Normal progress is throttled per Codex session. Terminal flushes intentionally bypass the debounce and may emit up to the latest item and usage snapshots immediately; this avoids cross-child head-of-line blocking without claiming an absolute rate cap over lifecycle flushes.
Validation
node_modules/vite-plus/bin/vp test run packages/shared/src/KeyedCoalescingWorker.test.ts apps/server/src/provider/Layers/CodexCollabRuntime.integration.test.tsvp lint --type-aware --type-check --report-unused-disable-directiveson all four changed files passed.vp fmt --checkon all four changed files passed.git diff --checkpassed.UI Changes
None.
Scope
This PR does not change raw assistant streaming, global orchestration scheduling, projectors, SQLite cleanup, or UI behavior. It addresses only Codex child progress amplification and the worker semantics required for correct terminal flushing.
Checklist
Related
a2ca89aaPrepared with Codex CLI (GPT-5.6-sol).
Note
Medium Risk
Touches provider event ordering and ingestion volume for collab children; terminal flush ordering is correctness-sensitive but scoped to Codex runtime and covered by integration tests.
Overview
Codex collab child progress (
collabAgent/item,collabAgent/tokenUsage) is no longer emitted on every wire notification. Updates are merged per child thread into separate latest-item and latest-usage lanes, debounced (~250ms) through a session-scopedKeyedCoalescingWorker, then emitted as provider events.Terminal lifecycle (
turn/completed, non-activethread/status/changed,thread/closed, terminal childerror) callsflushKeyso the newest pending progress is delivered before idle/complete/close events, without waiting behind other children. Normal flushes skip the debounce sleep.KeyedCoalescingWorkergainsflushKey(priority processing for one key,context.flushonprocess) and documents thatdrainKeybehavior is unchanged. Closing the worker’s scope interrupts in-flight work.Session
closenowScope.close(runtimeScope)beforesession/closedand queue shutdown so coalesced progress cannot enqueue after teardown.Integration and unit tests cover burst coalescing, latest-value retention, progress-before-terminal ordering, cross-key flush, and scope-close interruption.
Reviewed by Cursor Bugbot for commit c74ae21. Configure here.
Note
Bound Codex subagent progress events with coalescing and flush-before-terminal semantics
collabAgent/itemandcollabAgent/tokenUsage) are now coalesced via a keyed worker with a 250ms cooldown, emitting only the latest item and token usage per child within each window.KeyedCoalescingWorkerin KeyedCoalescingWorker.ts with aflushKeymethod that bypasses the queue and cooldown for a specific key, passing a flush-context flag to the process callback.session/closed, cancelling any pending best-effort progress emissions.Macroscope summarized d765754.