Skip to content

🤖 fix: defer bash-monitor wakes until the active turn ends - #4114

Draft
ibetitsmike wants to merge 32 commits into
mainfrom
mike/bash-wake-turn-end
Draft

🤖 fix: defer bash-monitor wakes until the active turn ends#4114
ibetitsmike wants to merge 32 commits into
mainfrom
mike/bash-wake-turn-end

Conversation

@ibetitsmike

@ibetitsmike ibetitsmike commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

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 own task_await had 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 +428 / -162 lines; the rest is tests.

Implementation

  • WorkspaceService.dispatchBashMonitorWake: when the owner has a pending or preparing turn or a session-backed busy state, schedule scheduleBashMonitorWakeReconcileAfterIdle and return "deferred"; otherwise send the wake with requireIdle: true (no queueDispatchMode: "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.interruptStream with the new retireBashMonitorAttention option (passed by the user Stop entry points: the Stop button, the Escape keybind, the command palette Stop and ACP cancel; internal interrupts such as goal promotion, archive, ACP disconnect and send-now leave monitor output owed): before the interrupt, BashMonitorWakeReconciler.consumeCurrent withdraws 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 because session.interruptStream returns only after the abort settled, when an idle-triggered dispatch may already be admitting; it does not take bashMonitorHistoryLocks, so Stop never waits behind a wake admission that holds the lock across preflight and stream construction. Retirement is bounded to the frontier snapshotted when the Stop was requested: output, settlements, monitor failures and newly armed monitors that land while the Stop settles stay owed and wake normally once idle. When the retirement or the abandon marker below cannot be written, the stream is still aborted but interruptStream returns Err(STOP_UNRECORDED_MESSAGE); the reconciler keeps the retirement owed and retries it before any later wake and on the next Stop. Every renderer Stop goes through stopStream(), which publishes that error to a workspace-keyed chat error store (chatErrorToasts.ts, replacing the one-shot child-budget toast event) that the workspace's chat input drains when it mounts, so the warning survives a workspace switch mid-Stop. Stops that also opt out of auto-retry (Escape and Ctrl+C, both barriers, compaction cancel, the palette command) pass disableAutoRetry inside the same interruptStream call: the backend starts the opt-out after consumeCurrent has withdrawn the pending wake and reserved the reconciler lock (disabling retry releases the idle gate a wake may wait behind), interrupts without waiting on its disk write, and verifies it with the abandon marker before acknowledging. stopStream also surfaces a transport rejection as the workspace chat error. The ACP cancel settles the pending prompt as cancelled for that sentinel too (the stream did stop) before reporting the durability failure; STOP_UNRECORDED_MESSAGE lives in common/constants/workspace.ts for that. Monitors stay registered and later output still wakes. consumeCurrent becomes public; the now-unread dedupeKey field is dropped from BashMonitorWakeDispatch.
  • AgentSession.sendMessage: a cancelable wake whose signal fires after the acceptance point of no return (its row is already durable) but before PREPARING now resolves Ok without starting a stream, the same contract as cancelBeforeAcceptance and 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. The withdrawn wake records a startup auto-retry abandon marker against the row actually persisted (the compaction request under on-send compaction) so crash recovery does not replay it; WorkspaceService.interruptStream joins the in-flight wake send and, if the marker write failed, retries it (recordPendingStartupAutoRetryAbandon) before reporting the Stop recorded. The auto-retry preference file is read once per session and every reader and mutator of that state awaits the read; writes are serialized and only the newest state change's completed write marks the file recorded. A late load cannot overwrite a newer marker, an older clear cannot land after a newer marker write and be acknowledged as durable, and a write never rebuilds the file from unloaded defaults.
  • BashMonitorWakeReconciler acceptance is durable-first: a cancelable wake is accepted the moment its row is durable (AgentSession.prepareMessage calls the idempotent accept() right after markRowsDurable(), before goal sync), so a crash can never leave a durable, unaccepted row. The accepted dispatch stays registered in the reconciler until its send settles, so a Stop landing anywhere before the stream starts still withdraws it. Consumption I/O (watermark, registry row, acknowledgement) failing neither fails the send nor lets the signal redeliver: it stays owed in reconciler state and is retried at the top of the next reconcile pass, ahead of any dispatch. If that I/O keeps failing until the app exits, the row is the only record of delivery, so before dispatching, the reconciler looks outstanding signals of processes older than the running instance up in the owner's transcript (listDeliveredBashMonitorWakes: full history scanned newest-first, compaction archive included, stopping once a chunk predates every process being checked, memoized per outstanding key, malformed persisted rows ignored) and consumes any wake the transcript already carries instead of sending it again. A wake that triggered on-send compaction is recognized through the compaction request row that carries it as follow-up content. A failed history read holds dispatch in the retry backoff. Owners that are archived or being archived hold their wakes without an idle-retry loop (no session exists to wait on and sendMessage refuses them); unarchive reconciles them once snapshot restoration has succeeded, after lifecycle startup and even if a follow-up step throws.
  • MessageQueue: hasAllWorkspaceTurnContinuations, hasAllWorkspaceTurnContinuationsAheadOfPromotedToolEnd, hasNextWorkspaceTurnContinuation, getNextQueueCutCandidate and isNextEntryBashMonitorWake now read the first entry whose cancel signal has not fired (the rule getNextDispatchableMode already used), so a withdrawn entry can neither supersede nor misattribute a delegated turn's correlation; the visible queue badge and correlation revalidation after a promotion read the same entry, and the raw FIFO-head reader is removed.
  • StreamManager.cleanupAbortedStream adds model: streamInfo.model and the request-pinned metadataModel to the stream-abort metadata (schema gains both as optional), and AgentSession.handleTurnAbort prefers the effective model over the requested model string and passes metadataModel into 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

  • Producer-to-stream tests in workspaceService.test.ts drive a fake SDK stream through AIService.streamMessage and 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 owns bashMonitorHistoryLocks, and returns STOP_UNRECORDED_MESSAGE when retirement I/O fails while the retirement still lands before any later wake; settlements, monitor failures and monitor arms that happen after the Stop request stay owed; stopStream retains an Err Stop for the workspace's chat input (shown on mount, after unmount, not for another workspace, and one at a time until each is dismissed) and stays silent on Ok; an unrecorded Stop still settles the ACP prompt as cancelled while cancel reports the failure; an interrupt without retireBashMonitorAttention keeps 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.
  • Red-green for the effective-model fix: removing the metadata.model fallback or the metadataModel pass-through fails the new streamManager and agentSession.queueDispatch cases (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.
  • Remote UAT (Coder Agents on dogfood, claude-sonnet-5 through 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 by task_await ended 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; no finishReason: "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 on main, not touched here).

Risks

Moderate, scoped to bash-monitor wakes and turn correlation.

  • Behavior change: a monitor match during a long turn is reported after the turn ends instead of at the next tool boundary. An agent that needs the output mid-turn still gets it through task_await; it is only the unsolicited wake that moves later.
  • The hard-Stop path retires owed attention before the abort rather than after it. Output that arrives during the few milliseconds the abort takes to settle is treated as post-Stop output and wakes normally once idle. session.interruptStream returns Ok even 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 pass retireBashMonitorAttention retire attention, so a caller added later without the option keeps the pre-PR behavior (output stays owed). A Stop whose retirement or abandon marker cannot be written reports an error (shown as a toast) instead of succeeding silently; the stream is still stopped and nothing is lost, the dismissed output may wake once on the next launch.
  • The queue reader change only affects entries whose cancel signal has already fired; those entries were already dispatched as no-ops.

Pains

The branch was rebased across #4109 (turn lifecycle centralization), which required porting the test harnesses from direct aiEmitter events to settling TurnStreamHandle.completion. Locally, bun on PATH resolved to 1.2.15 while the repo pins 1.3.5; under 1.2.15 the injected-failure tests in streamManager.test.ts and agentSession.queueDispatch.test.ts fail identically on main, which cost a diagnosis round.


Generated with xum • Model: anthropic:claude-fable-5-1 • Thinking: xhigh • Cost: $904.97

@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/node/services/streamManager.ts
Comment thread src/node/services/workspaceService.ts Outdated
Comment thread src/node/services/workspaceService.ts Outdated
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
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Addressed all three findings in 27acae7: abort metadata carries metadataModel for pinned pricing, hard Stop retires owed attention best-effort before the abort, and no longer takes bashMonitorHistoryLocks.

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/node/services/workspaceService.ts Outdated
Comment thread src/node/services/workspaceService.ts Outdated
…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.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Round 3 (12dd020): a wake withdrawn past the acceptance point of no return now resolves without claiming PREPARING, and interruptStream retires owed bash-monitor attention only when the caller passes retireBashMonitorAttention (user Stop entry points), not on every non-soft interrupt.

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/browser/features/Messages/ChatBarrier/StreamingBarrier.tsx Outdated
Comment thread src/node/services/workspaceService.ts Outdated
Comment thread src/node/services/agentSession.ts
Comment thread src/node/services/workspaceService.ts
…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.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Round 4 (a8f06c1). Addressed the four round 3 findings:

  • Compaction Stop paths (cancelCompaction, StreamingBarrier fallback) pass retireBashMonitorAttention so a monitor match during compaction does not wake the agent once the session goes idle.
  • consumeCurrent aborts the in-flight dispatch synchronously before taking the owner lock; interruptStream starts retirement before session.interruptStream and awaits it afterwards.
  • The 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 (hasActiveOrPendingTurnWork); the idle waiter waits on a deterministic waitForMidStreamCompactionSettled signal.

Each fix has a red-green test; the ACP prompt-correlation expectation is updated for the retireBashMonitorAttention option.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/node/services/agentSession.ts Outdated
Comment thread src/node/services/workspaceService.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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.

Comment thread src/node/services/workspaceService.ts
…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.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Round 5 (head 1f3cc0b). Addresses both Round 4 findings:

  • The prefix-swap-invalidated path no longer settles the mid-stream compaction window before finishContinuousCompaction dispatches the continuation; the observation's finally settles it afterwards, so idle monitor wakes cannot race the follow-up send for turn admission.
  • consumeCurrent takes a commit gate: it still withdraws the in-flight wake synchronously, but advances watermarks and cleans up registry rows only after session.interruptStream succeeded. A failed Stop leaves the signals owed and schedules a reconcile.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/node/services/bashMonitorWakeReconciler.ts Outdated
@chatgpt-codex-connector

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.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Head is now 4876b42 (supersedes the round 5 request on 1f3cc0b). Adds the fix for the security finding: interruptStream joins the withdrawn wake's send before acknowledging Stop, so the startup abandon marker for its durable row is written before Stop returns.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/node/services/workspaceService.ts
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.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Head is now 47f3759. Adds the fix for the snapshot finding: consumeCurrent collects the owed signals before waiting on the stop gate, so output arriving while Stop settles stays owed to the idle agent.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: 47f37591cf

ℹ️ 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".

@chatgpt-codex-connector

This comment has been minimized.

@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

24d2253 (head): a cancelable monitor wake refused as stale past the rollback horizon is finalized (accept()) before the refusal returns, so the durable row the manual turn consumes is the single delivery and the dispatcher no longer redelivers the same attention after that turn.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 24d22537f9

ℹ️ 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".

Comment thread src/node/services/agentSession.ts Outdated
@chatgpt-codex-connector

This comment has been minimized.

…ering

An accepted wake's row is durable, so when its watermark or acknowledgement I/O
fails the consumption is kept owed in reconciler state and retried at the top
of the next reconcile (retry backoff) before any dispatch. Acceptance no longer
rejects the send, and the dispatch is cleared after the I/O either way, so a
failed finalization can neither suppress the owner nor redeliver signals the
transcript already carries.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

6e68e07 (head): failed wake acceptance I/O now keeps the consumption owed in reconciler state and retries it ahead of any dispatch (reconcile retry backoff) instead of rejecting the send or leaving an accepted dispatch that later redelivers the consumed row.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6e68e0702a

ℹ️ 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".

Comment thread src/node/services/bashMonitorWakeReconciler.ts
Adopt main's serialized auto-retry preference writes (#4120), awaitable session disposal and closing-signal turn handles (#4118) while keeping this branch's unrecorded-write tracking, idle-wake predicates, and pricing-identity tests.
When an accepted wake's consumption I/O keeps failing until the app exits, the
durable transcript row is the only record of delivery: on the next run the
signal derives as outstanding again and the in-memory owed acceptance is gone.
The reconciler now asks the owner's history for wake records before dispatching
and treats a signal whose (processId, wakeUpdatedAt) key is already in the
transcript as delivered, advancing its watermark and acknowledging it with no
new row. The lookup scans full history backward, compaction archive included,
stopping once a chunk predates every process being checked, and is memoized per
outstanding key so an unchanged frontier reconciles without another read. A
failed history read rejects so dispatch stays held in the retry backoff.

Also retires the wake-wiring test harness's never-completed stream handles on
session close, which #4118's awaitable dispose otherwise drains forever.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Head 551e2c1 (merge of main through #4120 plus the transcript-based wake consumption for PRRT_kwDOPxxmWM6f3meT).

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 551e2c1675

ℹ️ 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".

Comment thread src/node/services/workspaceService.ts Outdated
Comment thread src/node/services/bashMonitorWakeReconciler.ts Outdated
The transcript lookup treats a persisted wake row as proof of acceptance, so
acceptance must not trail the row: a cancelable wake now calls accept() right
after markRowsDurable(), ahead of goal sync, and accept() is idempotent so the
later exits only record the abandon marker. To keep a Stop able to withdraw the
wake anywhere before its stream starts, the reconciler no longer releases an
accepted dispatch's slot at acceptance; the slot is released once the send has
also settled (onWake returned), and abortDispatch can still reach it meanwhile.

listDeliveredBashMonitorWakes validates persisted metadata instead of
spreading it: a malformed row degrades to "not delivered" rather than failing
the scan and holding every wake of that owner in the retry backoff.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Head 4dfe2b9: acceptance at row durability (accepted dispatch stays withdrawable until its send settles) for PRRT_kwDOPxxmWM6f8G1W, and malformed persisted wake rows filtered for PRRT_kwDOPxxmWM6f8G1Q.

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

state.dispatch.controller.abort();
state.dispatch = undefined;

P2 Badge Join wake withdrawal when its monitor is canceled

When a monitor is canceled after its wake row was accepted but before the send settles, this aborts the now-retained dispatch and the stopped listener proceeds after only removing the registry row. AgentSession attempts to persist an abandon marker as the send unwinds, but this cancellation path neither joins that send nor verifies the marker write; a crash or write failure therefore leaves the trailing accepted wake eligible for startup auto-retry even though its process was explicitly canceled. Retain and join the withdrawn send obligation, with the same durability check used by Stop.

AGENTS.md reference: AGENTS.md:L112-L112


...(typeof entry.matchedThroughOffset === "number"
? { matchedThroughOffset: entry.matchedThroughOffset }

P2 Badge Reject invalid persisted watermark offsets

When bash-monitor-watermark.json is corrupted but still contains the real process identity, this accepts any JSON number as matchedThroughOffset, including a fractional, extremely large, or non-finite value such as one parsed from 1e400. A high value makes derive()'s throughOffset > matchedThroughOffset test remain false for every subsequent real match, silently disabling wakes for that process instead of healing the persisted state. Discard entries whose offsets are not finite non-negative integers.

AGENTS.md reference: AGENTS.md:L110-L110

ℹ️ 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".

Comment thread src/node/services/agentSession.ts Outdated
Comment thread src/browser/hooks/useAIViewKeybinds.ts Outdated
Comment thread src/node/services/messageQueue.ts
…to the durable row

Every renderer Stop that also opts out of auto-retry (Escape and Ctrl+C, the
streaming and retry barriers, compaction cancel, the palette command) now does
so through stopStream({ disableAutoRetry: true }), which awaits the opt-out and
surfaces its failure before issuing the Stop. The Stop is acknowledged only once
the session's auto-retry state is on disk, so an opt-out still in flight escaped
that check and left the trailing row replayable after a restart.

A withdrawn wake records its abandon marker against the row that was actually
persisted: under on-send compaction that is the compaction request, not the
user message that is deliberately never appended, so startup recovery no longer
misses the marker and resumes the withdrawn wake.

The queue badge derives from the entry the next drain actually sends, matching
the dispatch readers that already skip withdrawn entries; the raw FIFO head
reader had no other caller and is removed.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Head 7ad183f: retry opt-out landed inside stopStream for every Stop path (PRRT_kwDOPxxmWM6f9Ypr), withdrawn-wake abandon marker keyed to the persisted compaction row (PRRT_kwDOPxxmWM6f9Ypl), queue badge from the dispatchable head (PRRT_kwDOPxxmWM6f9Ypy).

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7ad183fa44

ℹ️ 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".

Comment thread src/node/services/workspaceService.ts Outdated
Comment thread src/node/services/bashMonitorWakeReconciler.ts
Comment thread src/node/services/messageQueue.ts
Comment thread src/node/services/workspaceService.ts
…nd revalidation off accepted or withdrawn entries

An archived owner has no session to wait on and sendMessage refuses it, so the
after-idle retry spun through history scans and refused sends until unarchive.
dispatchBashMonitorWake now defers such wakes without rescheduling and
unarchive reconciles them.

The transcript scan also reads the wake identity nested in a compaction-request
row's follow-up, the only durable row when a wake triggered on-send compaction.

A discarded process no longer withdraws an accepted wake (its row is durable and
only a user Stop joins the send and verifies the abandon marker), and queue
correlation revalidation skips withdrawn entries like the other readers, so a
canceled no-op cannot strip a promoted continuation's correlation.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Head f00391c: archived owners hold wakes without the idle-retry loop (PRRT_kwDOPxxmWM6f-Csc), compaction-request rows recognized as delivered wakes (PRRT_kwDOPxxmWM6f-CsQ), discardProcess leaves accepted wakes alone (PRRT_kwDOPxxmWM6f-CsV), correlation revalidation skips withdrawn entries (PRRT_kwDOPxxmWM6f-CsZ).

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f00391c20f

ℹ️ 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".

Comment thread src/node/services/workspaceService.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit: f00391c20f

ℹ️ 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.

Comment thread src/browser/utils/stopStream.ts Outdated
…ttention only after unarchive restores

Disabling auto-retry releases the idle gate a pending monitor wake may wait
behind, so an opt-out issued ahead of the Stop let that wake enter sendMessage
before the Stop reserved its attention. interruptStream now takes
disableAutoRetry and persists the opt-out after consumeCurrent has withdrawn the
dispatch and reserved the reconciler lock, before the session interrupt, and
verifies it with the abandon marker before acknowledging. stopStream passes the
flag through instead of making a separate call.

Unarchive schedules the held monitor reconcile after snapshot restoration and
lifecycle startup succeed, alongside the workflow reconciliation, so a wake
cannot run against a half-restored checkout or survive a failed restoration's
rollback to archived.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Head acbf958: retry opt-out moved inside the Stop transaction after attention retirement (PRRT_kwDOPxxmWM6f-eqc); held monitor attention reconciled only after unarchive restoration succeeds (PRRT_kwDOPxxmWM6f-aOL).

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: acbf958813

ℹ️ 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".

Comment thread src/node/services/workspaceService.ts Outdated
Comment thread src/node/services/bashMonitorWakeReconciler.ts
Comment thread src/browser/utils/stopStream.ts Outdated
Comment thread src/node/services/workspaceService.ts Outdated
…r recovered processes

The Stop's session interrupt no longer waits on the auto-retry opt-out's disk
write (a stuck write kept the stream running); the write starts after
retirement is reserved, runs alongside the interrupt, and is joined before the
durability check. stopStream also publishes a transport rejection as the
workspace's chat error instead of letting a void-called Stop reject unseen.

The reconciler consults the transcript only for processes created before this
instance: a failed acceptance from this instance stays owed in memory, so a
long-lived monitor no longer triggers a history scan for every new match.

Unarchive schedules the held monitor reconcile in a finally around the
post-restoration follow-ups, so a follow-up that throws after unarchivedAt is
persisted cannot strand the attention behind the !didUnarchive early return.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Head ebce837: interrupt no longer waits on the opt-out write (PRRT_kwDOPxxmWM6f-5he), transcript scans limited to processes older than the instance (PRRT_kwDOPxxmWM6f-5hk), stopStream catches transport rejections (PRRT_kwDOPxxmWM6f-5hn), unarchive reconciles held wakes in a failure-safe finally (PRRT_kwDOPxxmWM6f-5hq).

@chatgpt-codex-connector

Copy link
Copy Markdown

Security review completed. No security issues were found in this pull request.

Reviewed commit: ebce8372be

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant