🤖 fix: defer bash-monitor wakes until the active turn ends - #4114
🤖 fix: defer bash-monitor wakes until the active turn ends#4114ibetitsmike wants to merge 13 commits into
Conversation
|
@codex review |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c91286c4da
ℹ️ 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".
Codex review on c91286c: - hard Stop no longer takes bashMonitorHistoryLocks (a wake admission holds it across preflight and stream construction); consumeCurrent runs before the abort and is best-effort so a persistence failure cannot fail the Stop - stream-abort metadata carries the request-pinned metadataModel so aborted Coder-runtime streams keep their pricing identity in goal accounting
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27acae7dd7
ℹ️ 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".
…ly on user Stop A cancelable monitor wake withdrawn past the acceptance point of no return (a hard Stop retiring owed attention while the wake is in goal sync or acceptance) keeps its durable rows but resolves Ok without starting a stream instead of claiming PREPARING after the Stop returned. interruptStream retires owed attention only when the caller passes retireBashMonitorAttention (user Stop button, Escape, command palette, ACP cancel); goal promotion, archive, ACP disconnect and send-now keep monitor output owed.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12dd020f18
ℹ️ 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".
…onously; defer during pending compaction - Compaction Stop paths (cancelCompaction, StreamingBarrier fallback) pass retireBashMonitorAttention so a match during compaction does not wake the agent. - consumeCurrent aborts the in-flight dispatch before taking the owner lock; interruptStream starts retirement before the abort and awaits it after. - Point-of-no-return withdrawal persists the user-abort abandon marker so startup recovery does not replay the retired wake. - Wake dispatch treats a pending mid-stream compaction as turn work; the idle waiter waits on a deterministic settle signal from AgentSession.
|
@codex review Round 4 (a8f06c1). Addressed the four round 3 findings:
Each fix has a red-green test; the ACP prompt-correlation expectation is updated for the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8f06c1964
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: a8f06c1964
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
…mmit retirement only after a successful stop - The prefix-swap-invalidated path no longer settles the mid-stream compaction window before finishContinuousCompaction dispatches the saved continuation; the observation's finally settles it afterwards, as the usage-delta path already does. - consumeCurrent takes a commit gate: it still withdraws the in-flight wake and reserves the reconciler lock before the abort, but advances watermarks only once the stop succeeded. A failed stop leaves the signals owed and schedules a reconcile.
|
@codex review Round 5 (head 1f3cc0b). Addresses both Round 4 findings:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f3cc0be6d
ℹ️ 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".
This comment has been minimized.
This comment has been minimized.
A wake withdrawn past its point of no return (durable row, not yet PREPARING) resolves only after recording the startup abandon marker for that row. Stop now waits for that send to settle, so a forced exit right after Stop cannot leave the row eligible for startup replay.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4876b427e7
ℹ️ 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".
consumeCurrent snapshots the outstanding signals before waiting on the stop gate, so output that arrives while the stop settles is new and stays owed to the idle agent instead of being consumed by the successful Stop.
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This comment has been minimized.
This comment has been minimized.
… of no return A hard Stop that withdraws a bash monitor wake after its row is durable but while goal sync is pending left the row eligible for startup replay when goal sync threw: AgentSession.sendMessage finalized onAccepted and rethrew before reaching the abort-marker write, WorkspaceService.sendMessage resolved Err, and interruptStream discarded the joined Result. Route the goal-sync catch, the disposed exit, the onAccepted failure exit, and the existing withdrawn exit through one abandonWithdrawnSend helper so the marker lands before the send settles on every path, which is what the Stop join relies on.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72038d802c
ℹ️ 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".
… point of no return The goal-sync failure and disposed exits checked the cancel signal before awaiting onAccepted, so a Stop that aborted the signal while the reconciler was persisting acceptance was missed: the catch rethrew without an abandon marker and Stop joined the send and returned success with the durable wake row still eligible for startup replay. Run abandonWithdrawnSend in a finally after onAccepted on those exits and after the accepted admission-stale failure, so the check is the last await of every exit that skips PREPARING.
|
@codex review Round 7: 80e6716 makes the withdrawal check the last await of every exit past the point of no return (onAccepted wrapped in try/finally on the goal-sync failure and disposed exits, plus the accepted admission-stale exit), so a Stop landing during acceptance I/O still records the abandon marker. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80e6716d9e
ℹ️ 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".
persistAutoRetryState swallowed write errors, so a wake withdrawn past its point of no return resolved Ok even when the startup abandon marker never reached disk, and Stop acknowledged success while leaving the trailing synthetic row eligible for startup replay. The marker writer now reports whether the file reflects memory; a withdrawn wake whose marker could not be recorded fails with WITHDRAWN_WAKE_UNRECORDED_MESSAGE, and the joining interruptStream propagates that failure. Other preference writes stay best-effort.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c9f5f68d3
ℹ️ 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".
…wake markers Round 8 review fixes for bash monitor wakes: - consumeCurrent snapshots the process frontier synchronously on entry and keeps it as an owed retirement in reconciler state once the stop commits; reconcileOnce retries it before any dispatch, so retirement I/O that fails cannot let the stop's idle reconcile re-dispatch dismissed attention, and output arriving while the stop waits behind acceptance stays owed - the unrecorded startup abandon marker is tracked in AgentSession and retried on every later user Stop, which fails with STOP_UNRECORDED_MESSAGE until the marker is on disk - the idle waiter releases its map slot before scheduling reconciliation so a reconcile that loses the race to a new turn installs the next idle wait - the RetryBarrier Stop button issues the same attention-retiring interrupt as its keyboard shortcut
|
@codex review |
1 participant
Summary
Bash-monitor attention no longer cuts an active turn. While the owner workspace is busy (a queued or preparing turn, or a session-backed stream), a monitor match is deferred through the existing reconcile-after-idle path instead of being queued as a tool-end message, and the wake itself is sent with
requireIdle. The turn finishes in its original stream and the agent answers on the same tool result; the owed attention arrives as a separate wake once the workspace is idle, or is withdrawn if the agent already consumed that output. A hard Stop retires the attention that is currently owed while leaving the monitor armed. Two independent fixes found along the way ride along: queue correlation readers skip withdrawn entries, and aborted-stream usage is priced against the effective (fallback) model.Supersedes #4065, which is left open for its author to close.
Background
#4065 addressed this incident: monitored background bash tasks settled while the agent was streaming, the wake was queued as a tool-end message, the stream was stopped at the next tool boundary (
finishReason: "tool-calls"), the agent's owntask_awaithad already consumed the output so the reconciler withdrew the wake, and the turn was left stranded on an unanswered tool result until a human typed. #4065 kept the mid-turn cut and added detection of stranded turns plus a synthetic[CONTINUE]restart, a continuation lifecycle, a recovery loop and caps (+1,563 net lines across 22 files).This PR removes the cause instead: attention that the running turn can consume must never be allowed to cut that turn. Production diff here is +99 / -66 lines; the rest is tests.
Implementation
WorkspaceService.dispatchBashMonitorWake: when the owner has a pending or preparing turn or a session-backed busy state, schedulescheduleBashMonitorWakeReconcileAfterIdleand return"deferred"; otherwise send the wake withrequireIdle: true(noqueueDispatchMode: "tool-end", no queue dedupe key, no abort listener that removed queued entries). A wake that loses the race with a user send is skipped by the existing requireIdle preflight and re-reconciled after idle, so manual input is never held behind background attention.WorkspaceService.interruptStreamwith the newretireBashMonitorAttentionoption (passed by the user Stop entry points: the Stop button, the Escape keybind, the command palette Stop and ACPcancel; internal interrupts such as goal promotion, archive, ACP disconnect and send-now leave monitor output owed): before the interrupt,BashMonitorWakeReconciler.consumeCurrentwithdraws any in-flight wake dispatch and marks the currently owed attention consumed, so the abort's own idle transition has nothing to send and no wake fires for output the user just stopped around. It runs before the abort becausesession.interruptStreamreturns only after the abort settled, when an idle-triggered dispatch may already be admitting; it is best-effort (a persistence failure is logged, the Stop still completes) and does not takebashMonitorHistoryLocks, so Stop never waits behind a wake admission that holds the lock across preflight and stream construction. Monitors stay registered and later output still wakes.consumeCurrentbecomes public; the now-unreaddedupeKeyfield is dropped fromBashMonitorWakeDispatch.AgentSession.sendMessage: a cancelable wake whose signal fires after the acceptance point of no return (its row is already durable) but before PREPARING now resolvesOkwithout starting a stream, the same contract ascancelBeforeAcceptanceand the disposed path. Without this, a Stop issued while the wake was in goal sync saw no turn to abort and the wake started a stream after the Stop returned.MessageQueue:hasAllWorkspaceTurnContinuations,hasAllWorkspaceTurnContinuationsAheadOfPromotedToolEnd,hasNextWorkspaceTurnContinuation,getNextQueueCutCandidateandisNextEntryBashMonitorWakenow read the first entry whose cancel signal has not fired (the rulegetNextDispatchableModealready used), so a withdrawn entry can neither supersede nor misattribute a delegated turn's correlation.StreamManager.cleanupAbortedStreamaddsmodel: streamInfo.modeland the request-pinnedmetadataModelto thestream-abortmetadata (schema gains both as optional), andAgentSession.handleTurnAbortprefers the effective model over the requested model string and passesmetadataModelinto goal accounting, mirroring the stream-end path. Usage of an aborted stream that fell back to another model is priced against the model that actually ran, and a Coder runtime ID keeps its pinned pricing identity instead of recording $0.Validation
workspaceService.test.tsdrive a fake SDK stream throughAIService.streamMessageand assert: repeated owed wakes during a turn never cut it and the answer arrives in the original stream; unconsumed attention coalesces into one wake after natural completion while idle attention starts promptly; a hard Stop retires owed attention without disarming later idle wakes, completes while another holder ownsbashMonitorHistoryLocks, and still succeeds whenconsumeCurrentrejects; an interrupt withoutretireBashMonitorAttentionkeeps the attention owed; a Stop issued during a wake's acceptance window leaves the session idle with no stream and a later match still wakes; owed attention neither holds a delegated completion open nor inherits its closed correlation; a withdrawn idle wake rolls back its admission and a fresh delivery succeeds.metadata.modelfallback or themetadataModelpass-through fails the newstreamManagerandagentSession.queueDispatchcases (the Coder-ID case asserts a non-zero cost). Red-green for the Stop ordering: reinstating the lock-around-interrupt version times out the lock-holder test and fails the best-effort test.claude-sonnet-5through the gateway, xum built at c91286c and driven through the UI): PASS on all 10 scenarios. Three runs of a monitored task consumed in-turn bytask_awaitended in the original stream with no wake; unconsumed attention arrived as exactly one wake ~60 ms after the turn ended; an idle match woke promptly; a user message typed during a turn dispatched ahead of the wake and the wake followed that turn; Stop produced no wake for the stopped output while the next match woke normally; 10 rapid matches coalesced into one wake; reload kept the history intact. 7 wake rows total, all accounted for; nofinishReason: "tool-calls"turn was left unanswered except the E1 turn the tester interrupted by hand, where the queued user message backgrounded the running foreground bash and dispatched at the tool boundary (existing behavior onmain, not touched here).Risks
Moderate, scoped to bash-monitor wakes and turn correlation.
task_await; it is only the unsolicited wake that moves later.session.interruptStreamreturnsOkeven with no active stream, so pressing Stop on an idle workspace retires its owed attention as well; that matches the previous head. Only callers that passretireBashMonitorAttentionretire attention, so a caller added later without the option keeps the pre-PR behavior (output stays owed).Pains
The branch was rebased across #4109 (turn lifecycle centralization), which required porting the test harnesses from direct
aiEmitterevents to settlingTurnStreamHandle.completion. Locally,bunon PATH resolved to 1.2.15 while the repo pins 1.3.5; under 1.2.15 the injected-failure tests instreamManager.test.tsandagentSession.queueDispatch.test.tsfail identically onmain, which cost a diagnosis round.Generated with
xum• Model:anthropic:claude-fable-5-1• Thinking:xhigh• Cost:$649.72