fix(server): settle upstream bodies on the failure paths too - #1539
Conversation
Hardening found while investigating #1419. It is NOT a fix for the native SIGTRAP reported there, and the issue stays open pending crash frames. cancelBodyOnAbort exists because Bun rejects an in-flight internal read when a fetch response body is torn down before our code attaches a reader, and that rejection is uncatchable by any caller try/catch. The guard was installed on the success paths only: - responses/core.ts read the non-2xx error body with .text() before the guard at :3436, and the Anthropic continuation did the same; - both web-search executors read the failure body before their guard; - /v1/live had no guard at all, passed no signal to readBodyCapped, and released the reader lock without cancelling when a read threw. Each of those is the same fetch-resolution-to-reader-attach window the guard was written for, just on the branch nobody guarded. readBodyCapped now cancels the reader when a read throws. A body.cancel() from the abort listener throws once a reader holds the lock, so the guard covers the window before attach and the reader covers the window after; a test pins that division because it is easy to assume one covers both. An earlier version of my audit claimed no unguarded site existed. That was wrong twice over: it checked whether a guard was present in the function rather than on the branch, and it enumerated from files that already imported the helper, which can only ever find sites that already have one. 004_audit_wp5_synthesis.md records the correction. Refs #1419
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change hardens abort-aware response-body handling across live relay, Responses error paths, and web-search executors. It adds stream behavior tests and documents corrected WP5 audit findings and hardening scope. ChangesAbort-body hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: ad6c4bec0b
ℹ️ 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".
| } from "../codex/auth-context"; | ||
| import { formatCodexProviderForLog } from "../codex/routing"; | ||
| import { signalWithTimeout } from "../lib/abort"; | ||
| import { cancelBodyOnAbort, signalWithTimeout } from "../lib/abort"; |
There was a problem hiding this comment.
When /v1/live fetch resolves and the client aborts before readBodyCapped() attaches its reader, the original Bun teardown race remains: cancelBodyOnAbort is imported here but never invoked anywhere in live.ts, while handleLive still proceeds directly from fetch() through recordOutcome to the body read. Attach the guard immediately after fetch resolution and detach it after body consumption so this request boundary cannot crash the process.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| } finally { | ||
| try { | ||
| reader.releaseLock(); | ||
| if (!readFailed) reader.releaseLock(); |
There was a problem hiding this comment.
Whenever reader.read() throws, readFailed makes this branch skip releaseLock(), but reader.cancel() settles the stream without releasing the reader's lock. The body therefore remains permanently locked; the added test's expectation that a second getReader() throws actually demonstrates the leak rather than proving it absent. Release the lock unconditionally in finally after the cancellation attempt.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| Deferred, with reasons recorded rather than silently dropped: request-time OAuth | ||
| refresh (`oauth/*` token endpoints) is a broad surface touching credential | ||
| handling and wants its own change with security review; CCA image fallback, MiMo | ||
| bootstrap, and the xAI clients have a narrower pre-attach interval; quota and | ||
| model discovery are not turn-body paths. Each is named in the follow-up so the |
There was a problem hiding this comment.
Before the deferred fixes have shipped, committing this _plan note publishes the still-unfixed OAuth refresh and other body-attachment candidates, including where to investigate them. The repository explicitly requires such pre-disclosure material to remain in scratch space until the relevant fixes ship; keep this audit in .tmp/ and publish only the completed outcome under _fin/ afterward.
AGENTS.md reference: AGENTS.md:L79-L83
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/live.ts (1)
355-377: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAlways release the reader lock after a failed read.
reader.cancel(err)does not release the lock. When the source already errored,cancel()can also reject without invoking the sourcecancel()hook. The currentreadFailedguard therefore leavesreadBodyCappedrethrowing while the stream remains locked.In
src/server/live.ts:355-377, retain cancellation before rethrowing, but callreader.releaseLock()unconditionally infinally. Intests/cancel-body-on-abort.test.ts:14-37, assertfailing.lockedisfalseafter rejection. Do not assert thatfailing.getReader()throws.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/live.ts` around lines 355 - 377, Ensure the readBodyCapped cleanup in src/server/live.ts:355-377 always calls reader.releaseLock() in finally, while retaining cancellation before rethrowing failed reads; remove the readFailed guard from lock release. Update tests/cancel-body-on-abort.test.ts:14-37 to assert failing.locked is false after rejection, without asserting that failing.getReader() throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/responses/core.ts`:
- Around line 3388-3393: Update the passthrough failure branch around
consumeComboFailure and upstreamResponse.text() to create a cancelBodyOnAbort
guard before either body read, using the upstream response body and signal.
Ensure the guard is detached in a finally block while preserving the existing
failure handling and response behavior.
---
Outside diff comments:
In `@src/server/live.ts`:
- Around line 355-377: Ensure the readBodyCapped cleanup in
src/server/live.ts:355-377 always calls reader.releaseLock() in finally, while
retaining cancellation before rethrowing failed reads; remove the readFailed
guard from lock release. Update tests/cancel-body-on-abort.test.ts:14-37 to
assert failing.locked is false after rejection, without asserting that
failing.getReader() throws.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 48adc9b7-60e2-463a-be79-4c7318ab5adf
📒 Files selected for processing (6)
devlog/_plan/260812_five_bug_fix_campaign/004_audit_wp5_synthesis.mdsrc/server/live.tssrc/server/responses/core.tssrc/web-search/anthropic-executor.tssrc/web-search/executor.tstests/cancel-body-on-abort.test.ts
…branch Three review findings, all correct. The /v1/live change imported cancelBodyOnAbort and never called it. The PR described a guard that was not there, and every existing test still passed because they exercise readBodyCapped and the helper directly and cannot see the call site. The guard is now attached before the read and detached in a finally, and a wiring test asserts that ordering — crude, but it is the thing that was missing. readBodyCapped held the reader lock after a failed read. reader.cancel() does not release the lock, so suppressing releaseLock() left the stream permanently locked for any later consumer. It now always releases, and the test asserts a second reader can be acquired and observes the original error instead of asserting the lock is retained. The native Responses passthrough had the same unguarded non-2xx branch as the translated path — consumeComboFailure or .text() before any guard. It was neither fixed nor listed among the deliberate deferrals, so it is fixed here. Refs #1419
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/cancel-body-on-abort.test.ts`:
- Around line 85-91: Strengthen the passthrough guard test around the existing
source-order assertions so cancelBodyOnAbort appears before both
consumeComboFailure and upstreamResponse.text(). Keep checking that
detachPassthroughErrorGuard exists, and compare symbol positions to fail if
either body consumption occurs before the guard is attached.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 64254b74-cd2c-41ad-893b-0c9a1c81f7c4
📒 Files selected for processing (3)
src/server/live.tssrc/server/responses/core.tstests/cancel-body-on-abort.test.ts
| test("the passthrough error branch attaches the body guard before consuming it", async () => { | ||
| const source = await Bun.file(new URL("../src/server/responses/core.ts", import.meta.url)).text(); | ||
|
|
||
| const guardAt = source.indexOf("cancelBodyOnAbort(upstreamResponse.body, upstream.signal)"); | ||
| expect(guardAt).toBeGreaterThan(-1); | ||
| expect(source).toContain("detachPassthroughErrorGuard"); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert passthrough guard ordering.
At Lines 85-91, the test only checks that the guard and detacher exist. It does not verify that the guard precedes consumeComboFailure and upstreamResponse.text().
If a later change moves the guard after either body read, this test still passes and the abort race returns.
Proposed test fix
const guardAt = source.indexOf("cancelBodyOnAbort(upstreamResponse.body, upstream.signal)");
+ const comboReadAt = source.indexOf("const failure = await consumeComboFailure(upstreamResponse");
+ const textReadAt = source.indexOf("const errorText = await upstreamResponse.text()");
expect(guardAt).toBeGreaterThan(-1);
+ expect(comboReadAt).toBeGreaterThan(-1);
+ expect(textReadAt).toBeGreaterThan(-1);
+ expect(guardAt).toBeLessThan(comboReadAt);
+ expect(guardAt).toBeLessThan(textReadAt);
expect(source).toContain("detachPassthroughErrorGuard");As per path instructions, “A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("the passthrough error branch attaches the body guard before consuming it", async () => { | |
| const source = await Bun.file(new URL("../src/server/responses/core.ts", import.meta.url)).text(); | |
| const guardAt = source.indexOf("cancelBodyOnAbort(upstreamResponse.body, upstream.signal)"); | |
| expect(guardAt).toBeGreaterThan(-1); | |
| expect(source).toContain("detachPassthroughErrorGuard"); | |
| }); | |
| test("the passthrough error branch attaches the body guard before consuming it", async () => { | |
| const source = await Bun.file(new URL("../src/server/responses/core.ts", import.meta.url)).text(); | |
| const guardAt = source.indexOf("cancelBodyOnAbort(upstreamResponse.body, upstream.signal)"); | |
| const comboReadAt = source.indexOf("const failure = await consumeComboFailure(upstreamResponse"); | |
| const textReadAt = source.indexOf("const errorText = await upstreamResponse.text()"); | |
| expect(guardAt).toBeGreaterThan(-1); | |
| expect(comboReadAt).toBeGreaterThan(-1); | |
| expect(textReadAt).toBeGreaterThan(-1); | |
| expect(guardAt).toBeLessThan(comboReadAt); | |
| expect(guardAt).toBeLessThan(textReadAt); | |
| expect(source).toContain("detachPassthroughErrorGuard"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/cancel-body-on-abort.test.ts` around lines 85 - 91, Strengthen the
passthrough guard test around the existing source-order assertions so
cancelBodyOnAbort appears before both consumeComboFailure and
upstreamResponse.text(). Keep checking that detachPassthroughErrorGuard exists,
and compare symbol positions to fail if either body consumption occurs before
the guard is attached.
Source: Path instructions
CI caught this: 'captures passthrough failed usage from its original bounded body exactly once' went red on both combo branches. The test is right and the guard was wrong. consumeComboFailure -> readBoundedResponseBody reads response.body itself and threads the abort signal through its own read, so it already owns settlement on that path. Attaching cancelBodyOnAbort first added a SECOND .body getter access, which is exactly what that test pins against — the combo contract is that the body is touched once. Both combo branches now go straight to consumeComboFailure. The plain .text() paths keep their guard, because there the reader only attaches when .text() runs and nothing else settles the body. A test records the distinction so the guard does not get 'helpfully' added back. Refs #1419
|
Full-suite result at this head on the Linux runner (Bun 1.3.14): All four Linux CI shards are green, including the |
…un#1539) * fix(server): settle upstream bodies on the failure paths too Hardening found while investigating lidge-jun#1419. It is NOT a fix for the native SIGTRAP reported there, and the issue stays open pending crash frames. cancelBodyOnAbort exists because Bun rejects an in-flight internal read when a fetch response body is torn down before our code attaches a reader, and that rejection is uncatchable by any caller try/catch. The guard was installed on the success paths only: - responses/core.ts read the non-2xx error body with .text() before the guard at :3436, and the Anthropic continuation did the same; - both web-search executors read the failure body before their guard; - /v1/live had no guard at all, passed no signal to readBodyCapped, and released the reader lock without cancelling when a read threw. Each of those is the same fetch-resolution-to-reader-attach window the guard was written for, just on the branch nobody guarded. readBodyCapped now cancels the reader when a read throws. A body.cancel() from the abort listener throws once a reader holds the lock, so the guard covers the window before attach and the reader covers the window after; a test pins that division because it is easy to assume one covers both. An earlier version of my audit claimed no unguarded site existed. That was wrong twice over: it checked whether a guard was present in the function rather than on the branch, and it enumerated from files that already imported the helper, which can only ever find sites that already have one. 004_audit_wp5_synthesis.md records the correction. Refs lidge-jun#1419 * fix(server): actually wire the live guard, and cover the passthrough branch Three review findings, all correct. The /v1/live change imported cancelBodyOnAbort and never called it. The PR described a guard that was not there, and every existing test still passed because they exercise readBodyCapped and the helper directly and cannot see the call site. The guard is now attached before the read and detached in a finally, and a wiring test asserts that ordering — crude, but it is the thing that was missing. readBodyCapped held the reader lock after a failed read. reader.cancel() does not release the lock, so suppressing releaseLock() left the stream permanently locked for any later consumer. It now always releases, and the test asserts a second reader can be acquired and observes the original error instead of asserting the lock is retained. The native Responses passthrough had the same unguarded non-2xx branch as the translated path — consumeComboFailure or .text() before any guard. It was neither fixed nor listed among the deliberate deferrals, so it is fixed here. Refs lidge-jun#1419 * fix(responses): leave the combo failure branches unguarded, deliberately CI caught this: 'captures passthrough failed usage from its original bounded body exactly once' went red on both combo branches. The test is right and the guard was wrong. consumeComboFailure -> readBoundedResponseBody reads response.body itself and threads the abort signal through its own read, so it already owns settlement on that path. Attaching cancelBodyOnAbort first added a SECOND .body getter access, which is exactly what that test pins against — the combo contract is that the body is touched once. Both combo branches now go straight to consumeComboFailure. The plain .text() paths keep their guard, because there the reader only attaches when .text() runs and nothing else settles the body. A test records the distinction so the guard does not get 'helpfully' added back. Refs lidge-jun#1419
1 participant
Summary
Hardening found while investigating #1419. This is not a fix for the native SIGTRAP reported there, and that issue stays open pending the crash frames — see the disposition comment on it.
cancelBodyOnAbortexists because Bun rejects an in-flight internal read when a fetch response body is torn down before our code attaches a reader, and that rejection is orphaned off the awaited path where no callertry/catchcan intercept it (src/lib/abort.ts). The guard was installed on the success paths only:src/server/responses/core.tsread the non-2xx error body with.text()before the guard at:3436; the Anthropic terminal-guard continuation did the same./v1/livehad no guard at all, passed no signal toreadBodyCapped, and released the reader lock without cancelling when a read threw.Each is the same fetch-resolution-to-reader-attach window the guard was written for, just on the branch nobody guarded.
readBodyCappednow cancels the reader when a read throws. That division matters and is easy to get wrong:body.cancel()from the abort listener throws once a reader holds the lock, so the guard covers the window before attach and the reader covers the window after it. A test pins exactly that, because assuming either one covers both is the mistake this PR is correcting.An earlier version of my audit claimed no unguarded site existed. It was wrong twice: it checked whether a guard was present in the function rather than on the branch, and it enumerated candidates from files that already imported the helper — a survivorship filter that can only ever find sites which already have one.
/v1/livenever appeared because it never imported it. The correction is recorded indevlog/_plan/260812_five_bug_fix_campaign/004_audit_wp5_synthesis.md.Deliberately not in scope, with reasons rather than silence: request-time OAuth token refresh touches credential handling and wants its own change with security review; the CCA image fallback, MiMo JWT bootstrap, and xAI image/video clients have a narrower pre-attach interval; provider quota and model discovery are not turn-body paths. Each is named in the devlog so the list is not lost.
Verification
On a Linux runner (Bun 1.3.14):
bun x tsc --noEmit— exit 0bun test tests/cancel-body-on-abort.test.ts— 8 pass, 0 failbun test tests/cancel-body-on-abort.test.ts tests/abort-race.test.ts tests/server-live.test.ts— 42 pass, 0 failbun test tests/web-search.test.ts— 51 tests, 0 failbun run privacy:scan— passedNote on what the new tests can and cannot assert: a source whose own
pull()rejects is errored by the stream machinery, which by spec does not invoke the source'scancel(). So the first test asserts propagation plus a settled stream, and a second test proves the locked-stream limitation directly. Two earlier drafts of these tests asserted behavior the Streams spec does not provide; they were corrected rather than worked around.Checklist
Refs #1419
Summary by CodeRabbit
Bug Fixes
Tests