fix(session): keep immediate prompt after abort from being swallowed (#4749) - #4753
Conversation
|
Exact-head bind for #4749:
Local gates: session-runtime + agent-session-concurrent 76 pass; coding-agent — |
|
Requesting independent exact-head lifecycle review of This is issue #4749: abort acknowledgement followed by immediate — |
![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: 525bae4661
ℹ️ 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".
| const queuedPlainPrompt = | ||
| options?.queuedAtDispatch === true && options.deliverAs === undefined && this.agent.state.isStreaming; |
There was a problem hiding this comment.
When an SDK turn.prompt is dispatched while abort unwind still makes ctx.isIdle() false, the control layer records startsOwnTurn = false; after the await above finishes, this new condition makes queuedPlainPrompt false because the old agent is no longer streaming. Consequently freshAtReservation and onQueuedPromoted can never fire, even though the prompt now starts its own successor turn. That turn therefore has no pending correlation or owning connection, so turn.prompt_status can remain accepted indefinitely and terminal abort cannot authorize its requester. Keep the original queued-at-dispatch classification available for the fresh-turn promotion path while separately preventing steering into the dying loop.
Useful? React with 👍 / 👎.
525bae4 to
012f188
Compare
|
Rebased onto current Exact head: Includes Codex P1: preserve — |
|
@codex review exact head — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 012f188acf
ℹ️ 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.#flushPendingAgentEnd(); | ||
| await this.#agentEndPublicationPromise; |
There was a problem hiding this comment.
When the aborted run's asynchronous agent_end handler is still processing after agent.waitForIdle() resolves, #flushPendingAgentEnd() is a no-op because the handler has not installed the pending event yet, and this await observes the previously resolved publication promise. The abort acknowledgement can therefore release an immediate successor while the dying handler is still active; its delayed agent_end may then be held behind or overwritten by the successor lifecycle, so the race this change is intended to close remains. Await #agentEndHandlingPromise (and then the resulting publication) before resolving the abort unwind.
Useful? React with 👍 / 👎.
|
Exact-head refresh after Codex P1 on abort publication:
Abort unwind now waits for the dying turn's — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ef5fd7e86
ℹ️ 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".
| if (managedLogicalRunId !== undefined) | ||
| this.agent.requestRunTerminal(managedLogicalRunId, { stopReason: "cancelled" }); | ||
| this.#flushPendingBackgroundExchanges(); | ||
| await this.#awaitAbortedTurnTerminal(); |
There was a problem hiding this comment.
When an active provider stream or tool ignores cancellation and remains in flight after timeoutMs, the timeout branch calls forceAbort, but this unconditional await then waits on #promptInFlightCount through #awaitAbortedTurnTerminal() with no further bound. Because forced recovery explicitly permits the old work to continue unwinding, abort({ timeoutMs })—including the 5-second cancel-and-submit path—can now hang indefinitely instead of returning a timeout and rolling back; skip this unbounded terminal drain for a timeout outcome or apply the remaining deadline to it.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review at exact head 1ef5fd7e — merge blocked on one major.
the diagnosis is right
The lost prompt is the one submitted immediately after an ordinary turn.abort: it could be accepted while the aborted turn's delayed agent_end still owned terminalization, then get consumed as steering or lifecycle work for the dying turn.
The fix awaits AgentSession.#abortUnwind, fences stale in-flight work with #abortEpoch, and pairs SDK agent_end events to FIFO openLifecycleBatches. That is the right structure.
the retention direction is handled
I check retention fixes for the opposite error — retaining too long — and most of it is covered: a single retained prompt is delivered once (queued promotion has a one-shot guard, SDK lifecycle batches consume oldest-start-first), and disposal and session-selection fences recheck admission before the retained prompt can execute.
major — overlapping aborts let work restart after the user's latest abort
session/agent-session.ts:12007-12010. An abort arriving while another unwind is active only awaits the first unwind and returns settled — it never creates a new cancellation fence.
abort A -> prompt P waits on #abortUnwind -> abort B
When A finishes, B acknowledges success and P is still allowed to refresh its generation and start. So the user aborts twice and work begins anyway. turn.abort can run concurrently with ordered prompt operations, so this is reachable through the SDK.
Abort must stay terminal. Advance an abort-admission epoch on every abort request and reject prompts admitted before a later abort, even when the physical unwind is shared.
ordering, for the record
Multiple prompts waiting on the same unwind resume FIFO through promise reaction order and the session admission queue, generally as separate sequential root turns; a prompt arriving after the first successor is live follows normal steering semantics. That is reasonable, but it is timing-dependent and no test documents the coalescing boundary — worth pinning whichever policy you intend.
coverage
session-runtime.test.ts's immediate-prompt case pins FIFO lifecycle pairing, but its abort hook is a no-op so it never exercises production unwind; its abort_and_prompt case pins exactly one replacement in the host harness. agent-session-concurrent.test.ts does exercise the real unwind and pins one queued promotion plus an empty steer/follow-up queue, though it catches both prompt promises and never asserts an exact final stream-call count.
Existing abort-timeout and cancel-and-submit contracts do not cover a prompt between two overlapping aborts — which is why the major survived.
The regression that closes it: hold abort A's unwind, submit P, issue abort B, release A, then assert B acknowledges only after P is fenced or rejected and that no successor stream or message starts. Plus two retained prompts to pin the intended FIFO/coalescing policy with exactly-once delivery.
Reviewed by @probepark — method: detached worktree at 1ef5fd7e, traced the retained prompt from acceptance through #abortUnwind and #abortEpoch to promotion, checked duplicate-delivery and disposal/session-transition fences, then walked the overlapping-abort sequence against the admission epoch. Tests not executed.
gajae.pr-review-verdict.v1 merge-blocked sha256:7da2c03189eee42b7ff5352b3918972a6369ebbb5115b44f8894acea11f15001 reviewer:human reviewer-id:probepark evidence:exact-head-1ef5fd7e-second-abort-shares-the-first-unwind-so-a-retained-prompt-starts-after-it
7dd86cd to
d9c38d2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9c38d2279
ℹ️ 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".
| // Abort visibility is per-request: a later real abort must not inherit an | ||
| // earlier silent abort's suppression and swallow the user-visible notice. | ||
| if (options?.silent !== true) this.#silentAbortPending = false; | ||
| await this.#abortUnwind; |
There was a problem hiding this comment.
When an abort with timeoutMs arrives while an earlier abort is still unwinding, this await ignores the newer request's deadline and waits for the first unwind without any bound. If that first abort is stalled in agent.waitForIdle() or terminal publication, the interactive five-second abort can hang indefinitely despite its explicit timeout; race the shared unwind against this request's timeout rather than returning only after the original caller settles.
Useful? React with 👍 / 👎.
| // still creates a cancellation fence, so a prompt admitted between the two | ||
| // aborts is refused rather than started after the shared unwind resolves. | ||
| this.#abortAdmissionEpoch++; | ||
| if (this.#abortUnwind) { |
There was a problem hiding this comment.
When a user_interrupt abort is unwinding with queued steering and a session_switch or new_session abort overlaps it, this shared branch never records the later abort's cause or advances the prompt generation. The first invocation therefore still reaches the user_interrupt check below and schedules continueQueuedMessages() using a generation the teardown abort did not invalidate, allowing stale queued work to race session teardown; the joined abort needs to apply the later request's continuation-cancellation semantics.
Useful? React with 👍 / 👎.
|
@probepark exact-head re-review requested. Exact head: Your major — overlapping aborts let work restart after the user's latest abortFixed. The shared-unwind path additionally applies the aborting request's own effects, which the old early return skipped entirely: preflight cancellation, plus abort visibility ( coverage — the regression you specifiedAdded to
ordering, on your "for the record" noteThe coalescing boundary is now pinned rather than left timing-dependent: the retained-prompt test asserts exactly-once delivery as exactly one successor turn, and the overlapping-abort test asserts the refusal boundary. A prompt admitted after the first successor is live keeps normal steering semantics. verification
Body verdict is |
d9c38d2 to
c960ec5
Compare
|
Rebased onto current New exact head: The body verdict is rebound to this exact head. Re-verified at this head: focused abort/ @probepark exact-head re-review requested on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c960ec5c7a
ℹ️ 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".
| if (managedLogicalRunId !== undefined) | ||
| this.agent.requestRunTerminal(managedLogicalRunId, { stopReason: "cancelled" }); | ||
| this.#flushPendingBackgroundExchanges(); | ||
| await this.#awaitAbortedTurnTerminal(); |
There was a problem hiding this comment.
When an interactive extension event handler executes await ctx.abort(), this terminal wait includes that same handler in #agentEventHandlersInFlight. The abort promise cannot resolve until the handler returns, while the handler cannot return until the abort resolves, so cancellation stalls until the ExtensionRunner's 30-second handler timeout fires. Track and exclude the initiating handler from this drain, or make this context's abort non-blocking.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-review at exact head c960ec5c — approved. The overlapping-abort hole is closed.
the fix
agent-session.ts:12039-12062. #abortWithOutcome now runs this.#abortAdmissionEpoch++ synchronously before the shared-unwind branch, and an overlapping abort cancels the current preflight controller before awaiting that unwind.
Prompts parked on the unwind capture the admission epoch and return false when it advances (:2875-2881), which surfaces as PromptPreflightCancelledError in both prompt() and sendUserMessage() (:9906, :11488).
Advancing the epoch for every abort request — including one that shares a physical unwind — is the right shape. The previous version conflated "the unwind is already running" with "no new cancellation intent", and that conflation was the bug. Abort is terminal again: A → P → B now rejects P rather than letting it start after the user aborted twice.
the regression discriminates
test/agent-session-concurrent.test.ts:414-440 drives A → P → B and asserts PromptPreflightCancelledError, no successor provider dispatch, no transcript message, empty queues and idle state. On the prior behavior P would resolve and start, so it fails there. Checking both dispatched() and userTexts() is the right pair — a prompt that starts but produces no transcript entry would still be a failure.
minor — the ordering assertion is implicit
:420-429 does not explicitly hold and release A's unwind, nor assert that P is already rejected at the instant B acknowledges. Same-tick calls deterministically install A's unwind, park P and then admit B, so production ordering does satisfy the requirement — but the test relies on that ordering rather than controlling it. A held-release version would survive a scheduling change.
still a comment, unchanged
The multiple-waiter FIFO/coalescing boundary remains undocumented. The new test pins one retained prompt delivered exactly once, not two waiters and their intended boundary. Worth pinning whichever policy you intend, since it is currently timing-dependent by construction.
Reviewed by @probepark — method: detached worktree at c960ec5c, checked that the epoch advances before any await on every abort path including the shared-unwind branch, traced the parked prompt's epoch snapshot to the rejection surfaced in both entry points, and evaluated whether the new regression fails on the prior behavior. Tests not executed.
gajae.pr-review-verdict.v1 merge-approved sha256:f39c47d0eb8b97884c3e3aaceed911d4111e4b56cb940d37a2aa5ad30537966b reviewer:human reviewer-id:probepark evidence:exact-head-c960ec5c-abort-admission-epoch-advances-synchronously-on-every-abort-including-shared-unwind
|
Independent verification from the #4749 issue-owner lane (read-only on this branch — no push, rebase, or merge from here). Verified at exact head
Reproduced the bug on dev by applying only this PR's two test files onto Two blockers remain, both mechanical:
Approved review is in place, so after the changelog rebase and a green exact-head Dev CI this is mergeable. I will close #4749 with signed evidence once this merges. — |
|
Approved at Recent One thing to preserve through the rebase: |
c960ec5 to
6be50c2
Compare
|
Rebase-only push to clear a stale base. Your approval at New exact head: proof this is rebase-only
The only non-source delta is the fixes and coverage confirmed intact after rebase
re-verified at this exact head
@probepark please re-approve at |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6be50c2d26
ℹ️ 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".
| // unwind the successor starts its own turn and must fire onQueuedPromoted | ||
| // so SDK pending correlation is not dropped. | ||
| const dispatchedWhileBusy = options?.queuedAtDispatch === true && options.deliverAs === undefined; | ||
| const queuedPlainPrompt = dispatchedWhileBusy && this.agent.state.isStreaming; |
There was a problem hiding this comment.
When an SDK prompt is dispatched during abort unwind while an earlier external follow-up remains queued, this assignment becomes false after the dying stream stops, so followUpAheadAtReservation cannot observe that queued work and the retained prompt starts a fresh root turn ahead of it. At this head, the fresh evidence beyond the earlier promotion-tracking report is that the original busy-dispatch bit is preserved only for promoteAfterAbortUnwind, not for the existing follow-up-ordering branch; use that bit to retain ordering while separately avoiding delivery as steering.
Useful? React with 👍 / 👎.
|
CI note for this exact head — the one red shard is a pre-existing
Proof it is not mine:
So the shard is red on The remaining |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-review at exact head 6be50c2d — approved. The rebase preserved the ordering the whole fix depends on.
This was the one thing worth checking, because a conflict resolution that moved a single statement past an await would silently reopen the hole while leaving the code looking correct.
agent-session.ts:12097 — #abortAdmissionEpoch++ is the first executable statement in #abortWithOutcome, before the shared-unwind branch at :12098 and before every await. On the overlap path :12104-12108 cancels and replaces the current preflight controller before awaiting the shared unwind at :12111.
The admission fence captures the epoch at :2912, waits at :2913-2915, and returns false when it advanced at :2915-2917. prompt() converts that directly to PromptPreflightCancelledError at :9938-9943, and sendUserMessage() does the same at :11521-11524, with the name assigned at :876-880. Both entry points, not one.
the delta is genuinely unrelated
Relative to the approved c960ec5c, this PR's own implementation and regression hunks are unchanged. session-runtime.ts, session-runtime.test.ts and sdk/session.ts have no delta at all. What moved in the shared files came from dev: agent-session.ts gained the Anthropic overload retry handling and drainable queue counting, agent-session-concurrent.test.ts gained the hidden-nextTurn/drainable-count regression from #4750, and the changelog gained merged entries. No conflict resolution touched the abort epoch, the unwind fence, the error surfacing, or the A → P → B test.
the pin survived
test/agent-session-concurrent.test.ts:414-427 still drives A → P → B and requires PromptPreflightCancelledError; :429-436 still pins no successor provider dispatch, no retained transcript message, empty steering and follow-up queues, waitForIdle completion, and isStreaming === false. It discriminates the old behavior, where P started after B.
prior comments, unchanged and still not blocking
The A → P → B test relies on same-tick ordering rather than an explicitly held and released unwind, and the multiple-waiter FIFO/coalescing boundary is still not separately pinned. Neither was worsened by the rebase; both are worth doing when someone next touches this area.
Reviewed by @probepark — method: located the epoch increment relative to the shared-unwind branch and every await rather than trusting that a rebase preserved it, confirmed the fence-to-error path in both entry points, and separated this PR's own hunks from dev-originated changes in the shared files.
gajae.pr-review-verdict.v1 merge-approved sha256:839f16d1d2cde9b9fefc66085efb111b6477b70f83454ec13688b6f3531f573b reviewer:human reviewer-id:probepark evidence:exact-head-6be50c2d-abort-epoch-still-first-statement-before-shared-unwind-and-all-awaits-pr-hunks-unchanged-from-approved-c960ec5c
|
Your failing check is not caused by this PR. Measured in a detached worktree at
It also predates today's merges — same 12 failures at My approval at |
|
Exact-head CI reconciliation at Exact-head status (30 checks): everything green except the product aggregate, which fails only on
The shard failure is not attributable to this PR. All 3 failures are #4767 MCP autoload user-scope: The same shard is also failing on plain One caveat I introduced: my rerun produced a So this PR is functionally terminal-green: I will merge the moment the aggregate is terminal green, then rerun the 3 #4749 regressions on the resulting — |
6be50c2 to
30a74e2
Compare
|
Re-push after rebase onto current New exact head: production code is unchanged from what you approved
the one new commit:
|
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-review at exact head 30a74e23 — approved. Third rebase, ordering intact.
I check this one property directly every round rather than trusting the rebase, because a conflict resolution that moves a single statement past an await reopens the hole while leaving the code looking correct.
agent-session.ts:12097—#abortAdmissionEpoch++is the first executable statement, before the shared-unwind branch at:12098and before any await.:12106-12107— the overlap path aborts the current preflight controller and replaces it, then awaits the shared unwind at:12111.:2912snapshots the epoch,:2914awaits,:2915/:2917return false on advancement;prompt()throwsPromptPreflightCancelledErrorat:9942andsendUserMessage()at:11524, with the name assigned at:879.
Both entry points, every abort path.
the delta is one test improvement
This PR's production files — agent-session.ts, session-runtime.ts, session-runtime.test.ts, sdk/session.ts — have no direct diff against 6be50c2d. The only PR-owned change is agent-session-concurrent.test.ts:275-296: the immediate-successor test now keys its mock stream on the newest user-message content instead of a stream-call counter.
That is a genuine improvement, not churn — a counter-keyed mock is order-dependent and would drift as unrelated dispatches are added around it. Content-keying makes it robust to that. Everything else in the direct delta is rebased dev work (timezone/context), not attributable here.
the pin still discriminates
agent-session-concurrent.test.ts:430-432 drives A → P → B; :435-437 awaits both abort acknowledgements and requires PromptPreflightCancelledError; :443 proves P never reached the provider; :444 no retained transcript message; :445 empty steering and follow-up queues; :441 waitForIdle completion; :446 isStreaming === false. The provider recorder is populated at :377-379, so the dispatch assertion has something real to observe rather than passing vacuously — worth confirming, since a no-dispatch assertion against an unpopulated recorder proves nothing.
The A → P → B regression at :414-449 is unchanged.
prior comments, still open, still not blocking
The overlap regression relies on same-tick ordering rather than a separately held and released unwind, and the multiple-waiter FIFO/coalescing boundary is not independently pinned. Neither was worsened. Both are worth doing whenever this area is next touched — the second in particular is currently timing-dependent by construction, so whichever policy you intend should be written down as a test rather than inferred.
Reviewed by @probepark — method: located the epoch increment relative to the shared-unwind branch and every await at the exact head rather than assuming the rebase preserved it, separated this PR's own hunks from dev-originated changes in the shared files, and confirmed the provider recorder is populated so the no-dispatch assertion is meaningful. Tests not executed.
gajae.pr-review-verdict.v1 merge-approved sha256:87df43b832bcde415bf2c7c585e0c3d883e502a639b9812f14596692db4b3f90 reviewer:human reviewer-id:probepark evidence:exact-head-30a74e23-epoch-increment-still-first-statement-production-files-unchanged-from-approved-6be50c2d-delta-is-one-test-hardening
1feb71f to
dc51627
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1feb71f506
ℹ️ 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".
| if (!(await this.#awaitAbortUnwindFence())) throw promptPreflightCancelledError(); | ||
| preflightCancellationGeneration = this.#promptPreflightCancellationGeneration; | ||
| admissionSignal = options?.preflightSignal | ||
| ? AbortSignal.any([this.#promptPreflightAbortController.signal, options.preflightSignal]) | ||
| : this.#promptPreflightAbortController.signal; |
There was a problem hiding this comment.
When abort B is scheduled from abort A's completion (for example, abortA.then(() => session.abort())), #awaitAbortUnwindFence() can validate the epoch and resolve its promise, after which B's callback runs before this outer continuation. These lines then refresh the cancellation generation and signal from B, treating the retained pre-B prompt as though it were admitted after B; because waitedForAbortUnwind is also set, that prompt may start while B unwinds despite the later abort. Preserve and revalidate the original admission epoch after the await, rather than refreshing to the latest abort's generation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc516277ce
ℹ️ 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".
| // Pair this agent_end with the oldest unmatched start. A delayed | ||
| // aborted-turn end that lands after a successor agent_start must | ||
| // terminalize the aborted invocation, never the successor. | ||
| const ended = current.openLifecycleBatches[0]; |
There was a problem hiding this comment.
When the aborted turn's agent_end handler pauses in noteTransition and a fast successor starts and ends, both end handlers can read the same head batch because it is shifted only after the asynchronous transition work. The successor end then terminalizes the predecessor again, while both handlers eventually remove their respective queue entries without ever applying an end transition to the successor, leaving its prompt status accepted or in-flight indefinitely. Fresh evidence beyond the earlier delayed-handler report is that this revision lets both handlers capture openLifecycleBatches[0] before either removes it; claim or shift each batch synchronously when its end handler starts.
Useful? React with 👍 / 👎.
| if (this.#agentEventHandlersInFlight > 0) waiters.push(this.#agentEndHandlingPromise); | ||
| if (this.#agentEndPublicationInFlight > 0) waiters.push(this.#agentEndPublicationPromise); | ||
| await Promise.race(waiters); |
There was a problem hiding this comment.
When the latest agent_end handler has completed but another asynchronous agent-event handler remains in #agentEventHandlersInFlight—for example, a slow tool or turn extension handler—this adds the already-resolved #agentEndHandlingPromise to every race. That promise wins immediately on each iteration while the global pending predicate remains true, producing an endless microtask loop that can starve the remaining handler's timer or I/O and prevent the abort from settling. Wait on the registered wake signal or track the actual outstanding handler promises instead.
Useful? React with 👍 / 👎.
Abort acknowledgement returned while the aborted turn was still unwinding, so an immediate turn.prompt was classified as steering into the dying loop or terminalized by the delayed agent_end. Fence successor admission on abort epoch, await abort unwind before a fresh prompt, pair lifecycle ends with the oldest unmatched start, and ack turn.abort only after session.abort() settles. Lore-id: 4749abort Constraint: no arbitrary sleep or client delay as the fix Constraint: delayed aborted-turn teardown must not clear or misattribute a successor prompt Rejected: wait 1000ms between abort and prompt | hides the race and is the reported workaround Confidence: high Scope-risk: medium Reversibility: revert-safe Tested: bun test session-runtime.test.ts agent-session-concurrent.test.ts; bun --cwd=packages/coding-agent run check Not-tested: live interactive TUI Esc-then-Enter against a real provider
An SDK turn.prompt dispatched while abort unwind still reports busy snapshots queuedAtDispatch. After unwind the successor starts its own turn, but dropping queuedPlainPrompt also dropped onQueuedPromoted, so the successor had no pending correlation or owning connection. Fire promotion for that abort-unwind fresh turn without steering into the dying loop. Lore-id: 4749abort-promote Constraint: delayed abort teardown must not drop successor command/turn identity Rejected: keep steering into the aborted loop | swallows the successor prompt Confidence: high Scope-risk: narrow Reversibility: revert-safe Tested: queuedAtDispatch onQueuedPromoted after abort unwind Not-tested: live multi-connection terminal abort of the successor
agent.waitForIdle can resolve while the session's async agent_end handler is still in flight, so flushPendingAgentEnd was a no-op and abort awaited a stale resolved publication promise. Wait for the handler, in-flight count, pending terminal, and actual publication before releasing abort unwind so a successor prompt cannot race the dying turn. Lore-id: 4749abort-terminal Constraint: no sleep/client delay; fence on handler/publication identity Rejected: ack on agent idle alone | delayed agent_end still races the successor Confidence: high Scope-risk: medium Reversibility: revert-safe Tested: bun test session-runtime + agent-session-concurrent (76 pass); bun --cwd=packages/coding-agent run check Not-tested: injected delayed agent_end handler barrier in a dedicated unit test
An abort arriving while another unwind was active only awaited the first unwind and returned settled -- it never created its own cancellation fence. A prompt admitted between the two aborts then refreshed its generation and started after the shared unwind resolved, so the user aborted twice and work began anyway. The same early return skipped the aborting request's own option effects, letting a later real abort inherit an earlier silent abort's suppression. Every abort request now advances an abort-admission epoch synchronously, before any await and before the shared-unwind branch. A prompt that waits on an unwind proves through that epoch that no later abort was admitted while it waited, and refuses as a cancelled preflight otherwise. The shared path also applies its request-scoped effects: preflight cancellation and abort visibility. Lore-id: 4f21c9a3 Constraint: abort must stay terminal -- a shared physical unwind is not a shared cancellation fence Constraint: a prompt admitted AFTER an abort settles must still start a normal successor turn Rejected: reject every prompt that ever waited on an unwind | breaks the #4749 retained-prompt fix Rejected: give each overlapping abort its own physical unwind | duplicates teardown of one turn Confidence: high Scope-risk: medium Reversibility: easy Fixes: #4749 Tested: overlapping abort fences a retained prompt, retained prompt exactly-once, queued steering resume, rapid repeated abort, successor execution completion, successor execution error Tested: bun test agent-session-concurrent session-runtime agent-session-silent-abort agent-session-queued-prompts agent-session-steer-interrupt agent-session-terminal-abort-chain agent-session-abort-timeout Checked: bun --cwd=packages/coding-agent run check
The immediate-prompt-after-abort case branched its mock streamFn on a stream-call counter. The session reports isStreaming before streamFn runs, so the aborted turn's provider dispatch races the abort: when the first turn's dispatch had not landed, the SUCCESSOR turn received call === 1, took the blocking branch, and waited for an abort that never came. The case failed 6/6 in isolation and ~1/3 in file order, which is the same counter-vs-content trap the overlapping-abort cases avoid. Branch on the newest user message instead, matching createAbortLifecycleSession. No production change. Lore-id: b8e3d5f1 Confidence: high Scope-risk: narrow Reversibility: easy Tested: target case 6/6 isolated, 4/4 in the four-file abort/lifecycle combo Checked: bun --cwd=packages/coding-agent run check
dc51627 to
3beb2f7
Compare
|
Rebase + digest refresh from the #4749 issue-owner lane. The #4767 blocker is gone.
Shard-1-of-8 — the only red check on this PR for the last several heads — is green. Every product validation job passes. Rebased onto current
Nothing in the fix changed. The production diff across Verification at exact head
Verdict set to The moment that approval lands I merge, rerun the abort→immediate-prompt lifecycle harness on the exact merge SHA, and close #4749 with signed evidence. — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3beb2f7467
ℹ️ 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".
| // Pair this agent_end with the oldest unmatched start. A delayed | ||
| // aborted-turn end that lands after a successor agent_start must | ||
| // terminalize the aborted invocation, never the successor. | ||
| const ended = current.openLifecycleBatches[0]; |
There was a problem hiding this comment.
When the aborted turn's agent_end handler is delayed and a fast successor starts and ends first, the successor handler can reach the global terminalPublicationCapture.resolvers and resolve every waiter even if its claimed lifecycle batch belongs to the successor. The terminal abort then records the predecessor's terminalPublished as true and returns before the predecessor event reaches the ring/broadcast. Associate each publication waiter with the invocation batch/correlation selected here and resolve only waiters for that batch.
Useful? React with 👍 / 👎.
|
Exact-head gate status at Product CI: 28/28 non-contract checks green, zero failures. Including I also fixed two body-contract defects the gate caught, both mine from the rebase:
The only remaining red is the intended one: @probepark — re-review requested and you are assigned. The delta from the I hold no merge authority here without your exact-head approval and will not self-approve my own lane's covering PR. On your approval I merge immediately, rerun the abort→immediate-prompt lifecycle harness on the exact merge SHA, and close #4749 with signed evidence. — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-review at exact head 3beb2f74 — approved. Fourth rebase, ordering intact.
Four of the six PR files are byte-identical to the approved 30a74e23: src/sdk/host/session-runtime.ts and its test, src/session/agent-session.ts, and test/agent-session-concurrent.test.ts. The two that differ are rebase-only — src/sdk/session.ts picked up four dev MCP-autoload lines from 73f83693, and the changelog gained 25 dev/release lines. This PR's own production and regression hunks are unchanged.
I still re-derive the ordering every round rather than trusting that, because a conflict resolution moving one statement past an await reopens the hole while leaving the code looking right:
agent-session.ts:12097—#abortAdmissionEpoch++is the first executable statement, before the shared-unwind branch at:12098and before any await.:12106-12107— the overlap branch cancels and replaces the preflight controller before awaiting the shared unwind at:12111.:2912captures the epoch,:2914awaits,:2915/:2917return false on advance;prompt()surfacesPromptPreflightCancelledErrorat:9942,sendUserMessage()at:11524.
correcting something I said last round
I wrote that the provider recorder "is populated at :377-379, so the dispatch assertion has something real to observe". That was checking the callback exists, not that it fires in this scenario — and on a closer look it may not.
test/agent-session-concurrent.test.ts:427-443 waits for isStreaming before abort A, but Agent sets isStreaming before provider dispatch and can short-circuit an already-aborted request. So dispatched() can be empty at assertion time, and expect(dispatched()).not.toContain("retained prompt") passes vacuously — it would also pass in a world where nothing dispatched at all.
Non-blocking, because the assertion that actually pins the bug is the typed rejection at :437: the retained prompt must reject with PromptPreflightCancelledError, which is false on the old behavior where P started after B. :444's transcript check is likewise substantive.
Worth tightening: before abort A, wait for or assert dispatched() contains the blocking prompt. That makes the no-successor assertion prove a negative against a recorder known to be working, which is the only way a "did not happen" assertion carries weight.
prior caveats, unchanged
The A → P → B test still relies on same-tick ordering rather than a separately held and released unwind, and the multiple-waiter FIFO/coalescing boundary is still unpinned. Neither was worsened; both are worth doing when this area is next touched.
Reviewed by @probepark — method: derived the PR file set from the base and byte-compared each against the previously approved head, re-located the epoch increment relative to the shared-unwind branch and every await, then traced isStreaming through provider dispatch to test whether the no-dispatch assertion can be vacuous — which corrected my own conclusion from the previous round. Tests not executed.
gajae.pr-review-verdict.v1 merge-approved sha256:2fd87e559acb2f5188b233f3e733620e8a6f4884e14b0e41d647c37731753f2b reviewer:human reviewer-id:probepark evidence:exact-head-3beb2f74-four-pr-files-byte-identical-to-approved-30a74e23-epoch-ordering-reverified-dispatch-assertion-caveat-noted
|
Lane ownership reconciliation — duplicate lane retired (no repo changes) A parallel dedicated lane for this PR ( Evidence:
Retirement receipt retained at |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
The five-axis review completed against the exact head without actionable P0-P2 findings. The reviewed API, correctness, security, verification, and compatibility boundaries are approved.
Findings / Required Changes
None.
CI / Verification
- Reviewed the exact remote head:
3beb2f7467ed19a2d1e5427e693522cb6662f455. - CI summary: 16 passing, 0 failing, 15 pending/cancelled/skipped.
- Non-successful checks without pass evidence:
Telegram daemon generation guard,gjc-state-gates / ${{ matrix.group }},Affected path validation / native-build,Virtual integration validation,Affected path validation / ${{ matrix.key }},Affected path validation / plan,gjc-state-gates,Windows Telegram daemon safety. - Passing evidence reviewed:
Validate exact-head PR contract,PR contract bootstrap,Affected path validation / ts-build:ts:Y29kaW5nLWFnZW50:cGFja2FnZXMvY29kaW5nLWFnZW50,Affected path validation / test:packages/coding-agent/test/notifications-live-stream.test.ts,Affected path validation / test:packages/coding-agent/src/sdk/host/session-runtime.test.ts,Affected path validation / test:packages/coding-agent/test/session-manager-resident-cache.test.ts,Affected path validation / test:@gajae-code/coding-agent:shard-1-of-8,Affected path validation / test:packages/coding-agent/test/agent-session-concurrent.test.ts. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | APPROVED | The reviewed API lifecycle and abort changes preserve the examined session boundaries; no concrete compatibility regression was established. |
| A2. Architecture / Correctness / Failure | APPROVED | Concurrency correctness is established for abort/successor ordering and delayed lifecycle events; no actionable correctness defect was found. |
| A3. Security / Privacy / Trust | APPROVED | A3 review found no established security, privacy, or trust risk in the changed abort lifecycle and session-handling code. |
| A4. Verification / Tests / CI | APPROVED | A4 verification evidence is favorable from passed affected tests and checks, with residual platform and integration risk because current jobs were skipped. |
| A5. Context / Compatibility / Platform | APPROVED | Integration lifecycle behavior is covered by passing affected tests and documented in the changelog; skipped platform jobs leave target-specific compatibility unverified. |
Limitations
- Windows/native and virtual integration validation jobs were skipped, so cross-platform compatibility is not established.
- Several repository-wide and platform-specific CI jobs were skipped, so cross-platform regression coverage is limited.
- CI summary contains no dedicated security or privacy scan result, so specialized scan coverage is not established.
- Several current Dev CI affected-path, native-build, integration, and platform jobs were skipped, so those configurations lack current-run verification.
- Windows, WSL, and native-build CI jobs were skipped, so platform behavior on those targets was not independently established.
3 participants
What
Immediate
turn.promptafter an acknowledgedturn.abortnow starts exactly one successor turn instead of being silently consumed by aborted-turn teardown — and abort stays terminal across overlapping aborts.turn.abortawaits session abort unwind before acknowledging.agent_endhandler, in-flight count, pending terminal, and publication — not justagent.waitForIdle().emitLifecyclepairs eachagent_endwith the oldest unmatchedagent_start.onQueuedPromotedafter unwind so command/turn identity is not dropped.silent: trueabort).Why
Fixes #4749. Owner reproduction: abort an active turn, submit
turn.promptimmediately, and the successor is sometimes accepted then lost; waiting ~1000 ms hides it. Sleep/client delay is not an acceptable fix.Retention must be bounded in the other direction too. Exact-head review of
1ef5fd7efound the major this revision closes: an abort arriving while another unwind was active only awaited the first unwind and returnedsettled, never creating its own cancellation fence. A prompt admitted between the two aborts then refreshed its generation and started after the shared unwind resolved — the user aborted twice and work began anyway. The same early return skipped the aborting request's own option effects, which additionally let a later real abort inherit an earlier silent abort's suppression (a regression this revision also fixes;agent-session-silent-abort.test.tspassed ondevand failed at1ef5fd7e).A prompt admitted after an abort settles is unaffected and still starts a normal successor turn.
Testing
New deterministic coverage in
agent-session-concurrent.test.ts(dispatch is keyed on message content, not a call counter — the aborted turn's provider dispatch races the abort, so a counter cannot identify which turn a stream belongs to):PromptPreflightCancelledError, and no successor reaches the provider or the transcript. Verified to fail without the fix (P resolves) and pass with it.onQueuedPromoted.Runs:
bun test packages/coding-agent/test/agent-session-concurrent.test.ts packages/coding-agent/src/sdk/host/session-runtime.test.ts packages/coding-agent/test/agent-session-silent-abort.test.ts— 87 pass.bun testonagent-session-queued-prompts,agent-session-steer-interrupt,agent-session-terminal-abort-chain,agent-session-abort-timeout,agent-session-fallback-cancel-idle,agent-session-deferred-shell-flush,agent-session-issue-2261-esc-subagent-cancel,agent-session-skill-reroute-cancellation— 95 pass.bun --cwd=packages/coding-agent run check— biome + tsc clean.Exact head:
3beb2f7467ed19a2d1e5427e693522cb6662f455Exact base:
497142cff57ae06b6b79a9af35a726463a3e11fe(origin/dev)Diff digest:
sha256:2fd87e559acb2f5188b233f3e733620e8a6f4884e14b0e41d647c37731753f2b(canonicalgit diff --binary --full-index --no-ext-diff <base>...<head>)Rebased onto current
dev(497142cff5) by the #4749 issue-owner lane. The production diff acrossagent-session.ts,session-runtime.ts, andsdk/session.tsis byte-identical to the approved30a74e2374head (line offsets only); the sole delta is changelog conflict resolution keeping both the #4767 and #4749 entries. A fresh exact-head approval is still required because the digest changed.Risk classification
low-risk— ordinary fix/maintenance; the repository owner may use the explicitmerge-self-approvedsolo verdict (no independent human review; the verdict name itself records this) with a risk-record comment bound to the exact head.regression-risk— fix with material regression risk; requires one assigned independent domain reviewer whose authenticated exact-headAPPROVEDreview the gate verifies (extra:independent:<login>; the token alone never suffices).high-risk— large refactor, feature, or materially high-risk change (security/auth/install/remove/public API/destructive lifecycle/architecture); requires one assigned independent domain reviewer with an authenticated exact-headAPPROVEDreview (extra:independent:<login>).GJC verdict
The
1ef5fd7emerge-blockedverdict is superseded: its one major (overlapping aborts let work restart after the user's latest abort) is closed at this head with the regression test the review specified. No authenticated approving GitHub review exists for this exact head yet. Re-review requested from @probepark. No merge until an authenticated approving review lands on exactly3beb2f7467. The verdict is deliberatelyneeds-human: @probepark approved30a74e2374, and although the production diff is byte-identical, the rebase changed the digest, so the prior approval cannot be carried forward.devbun checkpasses (packages/coding-agent)