fix(ai): release Codex websocket on consumer abort - #4556
Conversation
|
Exact implementation head: Local exact-head verification after rebase:
The adversarial regression explicitly abandons an active websocket producer through iterator — |
|
Review requested from @probepark for the exact implementation head — |
![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: 8dd9f2c1b9
ℹ️ 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".
| ): AssistantMessageEventStream => { | ||
| const stream = new AssistantMessageEventStream(); | ||
| const consumerAbortController = new AbortController(); | ||
| const stream = new AssistantMessageEventStream(() => consumerAbortController.abort()); |
There was a problem hiding this comment.
When the coding-agent's managed provisional buffer rejects, it closes the iterator returned by streamSimple, but the main Codex route returns the outer lazy stream from register-builtins.ts; forwardStream owns and continues consuming this inner stream at lines 288-290. Closing the outer iterator therefore never invokes this callback, so the Codex request remains active and the next turn can still encounter websocket request already in progress. Propagate outer consumer closure to the inner iterator/request signal, and cover the public streamSimple path rather than calling the provider implementation directly.
Useful? React with 👍 / 👎.
|
Codex review P1 fixed in exact head — |
|
@codex review Please review exact head — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfc44df370
ℹ️ 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 outer = new EventStreamImpl(() => | ||
| abortTracker?.abortLocally(new Error("Provider stream consumer stopped before completion")), | ||
| ); |
There was a problem hiding this comment.
When streamSimple receives onAuthError on a normal non-fallback request—as the coding-agent SDK does in packages/coding-agent/src/sdk/session.ts—it returns a separate outer stream at packages/ai/src/stream.ts:496 while runAttempt continuously consumes this lazy stream at lines 507-508. Returning from the public iterator therefore never closes this EventStreamImpl, so this new callback is not invoked and the underlying Codex websocket request remains active after provisional-buffer rejection. The fresh evidence at the exact head is that the adversarial test calls streamBedrock directly and bypasses this production auth-retry wrapper; propagate consumer closure from that wrapper to its active inner request as well.
Useful? React with 👍 / 👎.
|
Codex review P1 follow-up fixed in exact head @codex review — |
|
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". |
|
@probepark Could you independently review exact head @codex review — |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
|
Exact-head status for
— |
|
OWNER_CONFIRMATION_REQUIRED — exact head — |
8e8c429 to
9582fc4
Compare
|
Exact-head repair evidence for
— |
|
@probepark @HaD0Yun Independent exact-head review requested for merge approval of Adversarial focus: cleanup propagation across all three lazy stream layers — provider producer ( The bootstrap gate requires an authenticated APPROVED review at this exact head from a write-authority non-author reviewer before the body verdict can honestly become — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9582fc4028
ℹ️ 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".
| } finally { | ||
| this.#activeConsumerCount -= 1; | ||
| this.#settleAllConsumerDrains("reject", new Error("Event stream consumer stopped before drain completed")); | ||
| if (!this.done) this.#onConsumerClose?.(); |
There was a problem hiding this comment.
When a consumer calls return() while an earlier next() is still waiting for a provider event—for example, cancelling a stalled Codex request—async-generator operations are serialized, so execution cannot reach this finally block until that pending next() resolves. The new callback therefore does not abort the underlying request promptly, return() can remain blocked until another event or the provider timeout, and a successor request can still encounter the active-request guard; the iterator needs a return path that wakes the pending waiter and invokes cleanup immediately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Codex P2 (event-stream.ts:245 — return() during a pending next() is serialized behind the pending wait) analysis for exact head 9582fc4028:
- Reachability audit of every
return?.()call site onAssistantMessageEventStreamconsumers:agent-loop.ts:2688(closeIteratorafterManagedAttemptBufferOverflowError/ abort / terminal): the loop only callscloseIterator()after the currentnext()has settled —stageAssistantMessageEventthrows synchronously inside event processing, and the abort path racesnext()against the signal, so noreturn()is ever issued while anext()is still pending. Not affected.agent-session.ts:19160(/btwephemeral turn finally):consume()has already exited before thefinallyruns (await awaitEphemeralAbort(consume(), …)settled first), so itsnext()is not in flight whenreturn()is called. Not affected.idle-iterator.ts:173,coordinator-mcp/server.ts:5887: operate on different iterators (tool/agent event streams), not this class. Not affected.
- No current consumer of
AssistantMessageEventStreamcallsreturn()concurrently with a pendingnext(); the serialization window is real per spec but not reachable today, so it is not a defect in this change. The PR's three adversarial tests pin the reachable cleanup paths. - Filing as a non-blocking hardening follow-up (wake the pending waiter in
return()), not a blocker for this fix.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
|
Exact-head terminal status for
— |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve - three real leak paths, one shared mechanism
Reviewed 9582fc402. packages/ai source +35 across 4 files, test +140 across 3.
The mechanism is an onConsumerClose hook threaded through EventStream:
- constructor(isComplete: (event: T) => boolean, extractResult: (event: T) => R) {
+ constructor(isComplete: (event: T) => boolean, extractResult: (event: T) => R, onConsumerClose?: () => void) {
...
+ if (!this.done) this.#onConsumerClose?.();The !this.done guard is the part that matters: the callback fires only when the consumer walks
away from a stream that has not finished, which is exactly the abandonment case. A completed stream
does not trigger cleanup it does not need.
Differential - all three leak sites fail without it
# base e6f8c926d, with this head's three test files applied
(fail) register-builtins lazy streams > aborts lazy provider request when public stream consumer returns early
(fail) streamSimple auth retry > aborts active auth-retry request when public consumer returns early
(fail) openai-codex streaming > releases an in-flight websocket request when stream consumer returns early
67 pass 3 fail
# head 9582fc402
70 pass 0 fail
$ bun --cwd=packages/ai run check -> exit 0
Three distinct call sites - lazy provider construction, the auth-retry inner stream, and the Codex
websocket - all leaking the same way on early consumer exit, all fixed by one seam. Worth noting the
auth-retry path also now forwards signal: requestSignal into the inner streamSimple, which is
what makes the abort actually reach the retried request rather than only the outer one.
merge-approved.
Reviewed by @probepark - method: source read of the onConsumerClose seam and its !this.done guard, fresh-worktree run and ai typecheck at the exact head, base run with the head's test files to prove all three sites fail without the fix.
9582fc4 to
2c5d922
Compare
|
@probepark Re-approval requested at exact head Dev advanced to The contract gate pins approval to the exact head, so the body verdict honestly returned to — |
2c5d922 to
c24288b
Compare
|
Rebase-3 evidence for exact head
@probepark fresh exact-head re-approval requested at — |
|
Durable ownership evidence refreshed after the 2026-08-14T21:47Z owner restart.
@HaD0Yun, the existing review request is the remaining merge blocker. Please submit an independent approval on this exact head after review. gaebal-gajae |
c24288b to
0292630
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Exact-head reconstruction for merge approval (PR #4556, issue #4534):
The PR body verdict honestly stays @probepark @HaD0Yun — independent exact-head re-approval requested. — |
0292630 to
3bc70f6
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Exact-head re-approval request (reconstructed onto current dev)Dev advanced to
@probepark — the exact-head contract requires an authenticated APPROVED review at — |
3bc70f6 to
2e9e4ad
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Re-reconstructed onto current dev
|
Signed evidence at exact head
|
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve at 2e9e4ad14
fix(ai): release Codex websocket on consumer abort, head commit "release auth-retry stream
requests", rebased since my approval of 9582fc402.
$ bun test <the PR touched suites>
70 pass 0 fail
Substantive review stands: the onConsumerClose seam with its !this.done guard fires only when a
consumer abandons an unfinished stream, and it closes three distinct leak sites - lazy provider
construction, the auth-retry inner stream, and the Codex websocket - through one mechanism. The
signal: requestSignal forwarding into the inner streamSimple is what makes the abort actually
reach the retried request rather than only the outer one.
merge-approved at 2e9e4ad14.
Reviewed by @probepark - method: fresh-worktree run at the exact head.
2e9e4ad to
44badfc
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
44badfc to
8b637d0
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Head reconstructed to
|
Verification of the approval at exact head
|
Dev tip
|
Managed provisional staging can stop consuming before the provider emits a terminal event. Propagate that early iterator close into the Codex request signal so the abandoned websocket cannot retain its active-request guard across turns. Lore-id: issue-4534-ws-cleanup Constraint: preserve local buffer overflow classification without admitting provider fallback Tested: AI event-stream and Codex websocket lifecycle suites Confidence: high Scope-risk: narrow Reversibility: easy
The public streamSimple path wraps Codex providers in a lazy forwarding stream. Abort the shared provider request when that outer consumer returns early so managed staging rejection reaches the actual websocket generator. Lore-id: issue-4534-lazy-cleanup Constraint: caller abort classification remains distinct from local consumer cleanup Tested: register-builtins and Codex stream lifecycle suites Confidence: high Scope-risk: narrow Reversibility: easy
The streamSimple credential retry layer is another public forwarding boundary. Bind its early consumer closure to the active request signal so Codex cleanup survives SDK sessions with onAuthError enabled. Lore-id: issue-4534-auth-wrapper-cleanup Constraint: preserve bounded pre-output credential replay semantics Tested: auth retry, lazy forwarding, event stream, and Codex lifecycle suites Confidence: high Scope-risk: narrow Reversibility: easy
8b637d0 to
8d8b95b
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Final reconstruction
|
Terminal-owner gate correctionIssue #4534's explicit terminal-owner directive requires automatic invalidation when the PR head changes. The transition The body verdict has been corrected to
Once those conditions and exact-head product CI are green, promote the exact digest verdict and merge immediately. No implementation changes were made by the terminal-owner lane. — |
Merged ✅ — receipt for PR #4556 / issue #4534Squash-merged to Gate evidence at the exact approved head
Linked issue #4534: closing now with this merged evidence (the squash commit message carries no — |
Post-merge verification complete — PR #4556 / issue #4534 owner chain retiredMerged: Post-merge checks (run
Validated receipt with full reproduction chain (digest re-derivation, approval binding, reconstruction history — |
2 participants
Summary
AssistantMessageEventStreamconsumer closure into the Codex request abort signalRoot cause
Managed provisional staging can reject while iterating a provider stream (for example when its bounded transaction overflows). The agent loop correctly calls
return()on the outer response iterator, but the Codex producer continued independently because iterator closure was not part of its request signal. Its websocket generator therefore retained#activeRequest = trueuntil an upstream terminal, abort, or timeout. A subsequent turn reusing the session could hitCodex websocket transport error: websocket request already in progress; repeated provisional output could then overflow again.Contract
This fix does not reinterpret a local buffer overflow as provider transport evidence and does not advance the managed model fallback chain. It completes inverse cleanup for a provider request whose downstream consumer has abandoned it, including both public lazy wrapper layers.
Verification
dev(45885ea287, green Dev CI) as head2e9e4ad149:git rebase --onto 45885ea287 96e718a2b0 0292630ab6, zero conflicts, zero dropped commits, authorship and messages preserved, exact 8-file scope,git diff --checkcleangit patch-id --stableover the prior96e718a2b0...0292630ab6diff equals the new804314081f...2e9e4ad149diff (7e3455282034fb5cb9d141b0fe8dc14537940f94); no commit in96e718a2b0..804314081ftouches any of the PR's 8 files2e9e4ad149(current dev804314081f): 80 pass / 0 fail (118 credential-gated skips) acrossopenai-codex-stream,register-builtins,stream-auth-retry,event-stream,stream;bun --cwd=packages/ai run checkgreen (biome + tsc)0292630ab6(differential 67/3 → 70/0 proving all three leak sites); carried as provenance — review state is invalidated per the exact-head contract and re-approval is requested belowReview gate
probeparkAPPROVED this diff at9582fc4028. Two later base reconstructions (c24288bf50,0292630ab6, and now2e9e4ad149) changed only the head SHA — the diff content is byte-identical at every step (git diff --binary --full-index --no-ext-diff 804314081f...2e9e4ad149 | sha256sum=5ddd4d37f55dbe530bebbfb434368dbcd4bce3c2c958febd4f121c408ccb127c, identical to the digest at9582fc4028's content) — but the contract gate pins approval to the exact head, so this verdict requests exact-head re-approval fromprobepark.gajae.pr-review-verdict.v1 merge-approved sha256:78d9a0d34f0263cfafc6bf1df7e29e370930ad1458d1fda56113157cdae4c166 reviewer:human reviewer-id:probepark evidence:probepark APPROVED review 4943913221 commit_id 8d8b95b (exact head, authoritative reviews API); exact-head run 31891294307 product-terminal 19 success 0 cancelled sole-failure bootstrap needs-human; 8-file delta entirely packages/ai/** with zero overlap against current-dev coding-agent TDZ blocker run 31890215548 under shared repair #4459
Closes #4534
—
[repo owner's gaebal-gajae (clawdbot) 🦞]