From ad6c4bec0b4f60aa2e9ef943d616242008b09df1 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 12 Aug 2026 22:25:06 +0900 Subject: [PATCH 1/3] fix(server): settle upstream bodies on the failure paths too 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 --- .../004_audit_wp5_synthesis.md | 79 +++++++++++++++++++ src/server/live.ts | 12 ++- src/server/responses/core.ts | 14 +++- src/web-search/anthropic-executor.ts | 6 +- src/web-search/executor.ts | 6 +- tests/cancel-body-on-abort.test.ts | 79 +++++++++++++++++++ 6 files changed, 191 insertions(+), 5 deletions(-) create mode 100644 devlog/_plan/260812_five_bug_fix_campaign/004_audit_wp5_synthesis.md diff --git a/devlog/_plan/260812_five_bug_fix_campaign/004_audit_wp5_synthesis.md b/devlog/_plan/260812_five_bug_fix_campaign/004_audit_wp5_synthesis.md new file mode 100644 index 0000000000..c345e39be7 --- /dev/null +++ b/devlog/_plan/260812_five_bug_fix_campaign/004_audit_wp5_synthesis.md @@ -0,0 +1,79 @@ +# 004 — WP5 audit: my fetch-site audit was wrong + +Independent reviewer verdict on the `051` audit deliverable: **FAIL**. The claim +it falsified was mine, and the falsification is correct. + +## What I claimed, and why it was wrong + +I enumerated the request-path fetch sites, found `cancelBodyOnAbort` at eight of +them, and concluded there was no unguarded site. Two errors: + +**1. I checked whether a guard exists in the function, not whether it covers the +branch.** At `src/server/responses/core.ts` the guard is installed at `:3436`, +but the non-2xx branch runs at `:3387-3394` and does +`await upstreamResponse.text()` **before** it. Same shape in +`src/web-search/executor.ts:85-90` and `anthropic-executor.ts:169-175`: the +failure branch reads the body, then the guard is attached for the success path. +So the error path reopens exactly the fetch-resolution-to-reader-attach race the +guard exists to close. Verified both by reading the code. + +**2. I enumerated from a grep of files that already imported the helper.** That +is a survivorship filter: it can only ever find sites that already have a guard. +`/v1/live` (`src/server/live.ts:554-570`) has no guard at all, passes no signal +to `readBodyCapped`, and releases its reader lock without cancelling on read +failure. It never appeared in my table because it never imported the helper. + +The reviewer also found request-time OAuth refresh (`src/oauth/index.ts` dispatch +reached from `core.ts:1799`), the CCA image fallback, the MiMo JWT bootstrap, and +the xAI image/video clients — all model-turn paths, none in my table. + +## Consequence for the disposition + +`051` acceptance criterion 1 is **unmet**, and the disposition must not say "no +unguarded request-path site found". That sentence would have been a false +all-clear on the exact question the issue is about. + +What stays true and was independently confirmed: + +- `installCrashGuards` does install both handlers, redacts, persists best-effort, + and keeps the process alive for JS-level failures — with bounded gaps + (best-effort persistence, a five-minute fold for known native teardown + rejections, installation after listener startup). None of that explains a + native `SIGTRAP`. +- Bun's fetch/TLS teardown is the only in-tree mechanism that fits the reported + sequence. Keyring N-API, `bun:sqlite`, Workers, and subprocesses exist but have + no temporal or causal link to TLS verification failure; the FFI sites are + Windows-gated and cannot explain a macOS crash. +- Supervision exists (launchd `KeepAlive`, systemd `Restart=on-failure`, WinSW + restart-on-failure) but **does not cover an `ocx gui`-started process**: + `src/cli/dispatch.ts:240-248` calls a detached spawn directly, and the child at + `src/cli/index.ts:924-931` is not adopted by any supervisor. That is precisely + why the reporter saw the dashboard die and stay dead. +- #1419 stays open pending the `.ips` frames. + +## Amendment: hardening lands, framed honestly + +The unguarded sites are real defects worth fixing on their own merits. They are +**not** a fix for the reported crash, and the PR and issue comment must say so: +this is hardening discovered while investigating #1419, not proof the trap is +resolved. + +Scope for this work-phase, ordered by how directly the path serves a user turn: + +1. `src/server/responses/core.ts` — guard the non-2xx branches (initial and + continuation) before reading the error body. +2. `src/server/live.ts` — guard the call-create response and cancel on read + failure. +3. `src/web-search/executor.ts`, `anthropic-executor.ts` — move the guard above + the failure branch. + +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 +list is not lost. + +Disclosure ordering (`AGENTS.md`): the working table stays in `.tmp/`. The public +comment names a finding only once its fix is merged and therefore already +disclosed by the diff. diff --git a/src/server/live.ts b/src/server/live.ts index af5d19498b..c040b14713 100644 --- a/src/server/live.ts +++ b/src/server/live.ts @@ -32,7 +32,7 @@ import { CodexThreadAffinityExpiredError, } from "../codex/auth-context"; import { formatCodexProviderForLog } from "../codex/routing"; -import { signalWithTimeout } from "../lib/abort"; +import { cancelBodyOnAbort, signalWithTimeout } from "../lib/abort"; import { sidecarEnter } from "../lib/sidecar-tracker"; import type { OcxConfig } from "../types"; import { resolveFirstUsableOpenAiSidecar, selectOpenAiImagesProvider } from "../providers/openai-sidecar"; @@ -352,6 +352,7 @@ export async function readBodyCapped( const reader = stream.getReader(); const chunks: Uint8Array[] = []; let total = 0; + let readFailed = false; try { for (;;) { const { done, value } = await reader.read(); @@ -364,9 +365,16 @@ export async function readBodyCapped( } chunks.push(value); } + } catch (err) { + // A read that throws leaves the stream neither drained nor cancelled, and releasing the + // lock alone hands back an unsettled body. Cancel first, then rethrow so the caller's + // existing classification (client abort / timeout / connect error) is unchanged. + readFailed = true; + await reader.cancel(err).catch(() => {}); + throw err; } finally { try { - reader.releaseLock(); + if (!readFailed) reader.releaseLock(); } catch { // already released / cancelled } diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index ecec8f3d8f..3765ce3eee 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -3385,13 +3385,20 @@ async function handleResponsesInner( break; } if (!upstreamResponse.ok) { + // Guard the error body BEFORE anything reads it. The success path is guarded further + // down, but a non-2xx response is still a Bun fetch body: if the request aborts between + // fetch resolution and `.text()` attaching its reader, the orphaned internal rejection is + // the same uncatchable teardown `cancelBodyOnAbort` exists to absorb (src/lib/abort.ts). + // Found while investigating #1419; not a fix for the native trap reported there. + const detachErrorBodyGuard = cancelBodyOnAbort(upstreamResponse.body, upstream.signal); if (options.comboAttempt) { const failure = await consumeComboFailure(upstreamResponse, options.abortSignal) - .finally(cleanupUpstreamAbort); + .finally(() => { detachErrorBodyGuard(); cleanupUpstreamAbort(); }); options.onConsumedComboFailure?.(failure); return failure.response; } const errorText = await upstreamResponse.text().catch(() => "unknown error"); + detachErrorBodyGuard(); cleanupUpstreamAbort(); if (!isFixedCodexAccount(authCtx)) { recordSubagentQuotaFailureForThreadSpawn( @@ -3655,7 +3662,12 @@ async function handleResponsesInner( } if (!response.ok) { + // Same pre-read guard as the initial response's error branch: a non-2xx continuation body + // is still a Bun fetch body, and an abort landing before `.text()` attaches its reader + // orphans the internal rejection. + const detachContinuationErrorGuard = cancelBodyOnAbort(response.body, upstream.signal); const errorText = await response.text().catch(() => "unknown error"); + detachContinuationErrorGuard(); yield { type: "error", status: response.status, diff --git a/src/web-search/anthropic-executor.ts b/src/web-search/anthropic-executor.ts index 8aded090eb..bb58f89be9 100644 --- a/src/web-search/anthropic-executor.ts +++ b/src/web-search/anthropic-executor.ts @@ -166,13 +166,17 @@ export async function runAnthropicWebSearch( () => fetch(url, { method: "POST", headers, body: JSON.stringify(body), signal: linkedSignal.signal }), { abortSignal: linkedSignal.signal, label: "web-search-sidecar-anthropic" }, ); + // Guard before any branch reads the body: the failure branch's `res.text()` ran ahead of + // the success-path guard, reopening the fetch-resolution-to-reader-attach race + // (found investigating #1419). + const detachBodyGuard = cancelBodyOnAbort(res.body, linkedSignal.signal); if (!res.ok) { const t = await res.text().catch(() => ""); + detachBodyGuard(); console.warn(`[web-search] anthropic sidecar HTTP ${res.status} for query "${query.slice(0, 80)}" (${Date.now() - t0}ms)`); // Redact before surfacing: the body can echo auth headers/tokens (#398 review). return { text: "", sources: [], error: `sidecar HTTP ${res.status}: ${redactSecretString(t.slice(0, 200))}` }; } - const detachBodyGuard = cancelBodyOnAbort(res.body, linkedSignal.signal); try { return await parseAnthropicSidecarSSE(res); } finally { diff --git a/src/web-search/executor.ts b/src/web-search/executor.ts index 1672d6176e..5ad7f67a71 100644 --- a/src/web-search/executor.ts +++ b/src/web-search/executor.ts @@ -82,12 +82,16 @@ export async function runWebSearch( { abortSignal: linkedSignal.signal, label: "web-search-sidecar" }, ); recordOutcome?.(res.status); + // Attach the body guard before ANY branch reads it. The success path guarded itself below, + // but the failure branch's `res.text()` runs first, so a cancel landing between fetch + // resolution and reader attach orphaned the internal rejection (found investigating #1419). + const detachBodyGuard = cancelBodyOnAbort(res.body, linkedSignal.signal); if (!res.ok) { const t = await res.text().catch(() => ""); + detachBodyGuard(); console.warn(`[web-search] sidecar HTTP ${res.status} for query "${query.slice(0, 80)}" (${Date.now() - t0}ms)`); return { text: "", sources: [], error: `sidecar HTTP ${res.status}: ${redactSecretString(t.slice(0, 200))}` }; } - const detachBodyGuard = cancelBodyOnAbort(res.body, linkedSignal.signal); try { return await parseSidecarSSE(res); } finally { diff --git a/tests/cancel-body-on-abort.test.ts b/tests/cancel-body-on-abort.test.ts index ed275862bf..29004a06d5 100644 --- a/tests/cancel-body-on-abort.test.ts +++ b/tests/cancel-body-on-abort.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test"; import { cancelBodyOnAbort } from "../src/lib/abort"; +import { readBodyCapped } from "../src/server/live"; function bodyWithCancelSpy(): { body: ReadableStream; cancelled: () => boolean } { let cancelled = false; @@ -10,6 +11,84 @@ function bodyWithCancelSpy(): { body: ReadableStream; cancelled: () return { body, cancelled: () => cancelled }; } +describe("readBodyCapped settles the stream when a read throws", () => { + test("a rejected read propagates and leaves no live reader lock", async () => { + // Note on what this can and cannot assert: a source whose own `pull()` rejects is errored + // by the stream machinery itself, which by spec does NOT invoke the source's `cancel()`. + // So the observable contract here is that the failure propagates to the caller (whose + // existing classification turns it into 499/504/502) and that the stream is left settled + // rather than pending. The cancel path added alongside this covers the case where the + // source is still live — an abort delivered while a read is outstanding. + let cancelled = false; + const failing = new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode("partial")); + }, + pull() { return Promise.reject(new Error("upstream reset")); }, + cancel() { cancelled = true; }, + }); + + await expect(readBodyCapped(failing, 1024, total => `too large (${total})`)).rejects.toThrow("upstream reset"); + // The stream errored itself, so `cancel()` is not expected to have run. + expect(cancelled).toBe(false); + // Locked-and-settled: a second reader is refused, proving no lock was leaked in a state + // where the body could still be pending. + expect(() => failing.getReader()).toThrow(); + }); + + test("cancelBodyOnAbort cannot settle a body once a reader holds the lock", async () => { + // The reason readBodyCapped needed its own cancel path. `cancelBodyOnAbort` calls + // `body.cancel()`, which throws on a locked stream, so once `getReader()` has run the + // guard alone can no longer settle the body — only the code holding the reader can. + // This is why the guard is attached BEFORE the read (covering the pre-attach window) and + // the reader cancels on failure (covering the window after it). + let cancelled = false; + const pending = new ReadableStream({ + pull() { return new Promise(() => {}); }, + cancel() { cancelled = true; }, + }); + + const reader = pending.getReader(); + const ac = new AbortController(); + cancelBodyOnAbort(pending, ac.signal); + ac.abort(new DOMException("client closed request", "AbortError")); + await Promise.resolve(); + await Promise.resolve(); + + expect(cancelled).toBe(false); + + // The reader itself can still settle it, which is what the new failure path does. + await reader.cancel(new Error("client closed request")); + expect(cancelled).toBe(true); + }); + + test("a normal read still returns the buffered payload", async () => { + const body = new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode("hello")); + controller.close(); + }, + }); + + const payload = await readBodyCapped(body, 1024, total => `too large (${total})`); + expect(payload).toBeInstanceOf(ArrayBuffer); + expect(new TextDecoder().decode(payload as ArrayBuffer)).toBe("hello"); + }); + + test("the byte cap still short-circuits with a 502 rather than throwing", async () => { + const body = new ReadableStream({ + start(controller) { + controller.enqueue(new Uint8Array(64)); + controller.close(); + }, + }); + + const payload = await readBodyCapped(body, 8, total => `too large (${total})`); + expect(payload).toBeInstanceOf(Response); + expect((payload as Response).status).toBe(502); + }); +}); + describe("cancelBodyOnAbort", () => { test("cancels the body when the signal aborts", async () => { const { body, cancelled } = bodyWithCancelSpy(); From ceec3229f67f0a83da61b1b220ab6f295881ffda Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 12 Aug 2026 22:40:12 +0900 Subject: [PATCH 2/3] fix(server): actually wire the live guard, and cover the passthrough branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/server/live.ts | 32 ++++++++++++++++++++-------- src/server/responses/core.ts | 26 +++++++++++++++-------- tests/cancel-body-on-abort.test.ts | 34 +++++++++++++++++++++++++++--- 3 files changed, 71 insertions(+), 21 deletions(-) diff --git a/src/server/live.ts b/src/server/live.ts index c040b14713..7b0d4ee4e3 100644 --- a/src/server/live.ts +++ b/src/server/live.ts @@ -352,7 +352,6 @@ export async function readBodyCapped( const reader = stream.getReader(); const chunks: Uint8Array[] = []; let total = 0; - let readFailed = false; try { for (;;) { const { done, value } = await reader.read(); @@ -368,13 +367,16 @@ export async function readBodyCapped( } catch (err) { // A read that throws leaves the stream neither drained nor cancelled, and releasing the // lock alone hands back an unsettled body. Cancel first, then rethrow so the caller's - // existing classification (client abort / timeout / connect error) is unchanged. - readFailed = true; + // existing classification (client abort / timeout / connect error) is unchanged. The + // cancel itself can reject with the stream's stored error — that is expected and must not + // mask the original failure, so it is swallowed here. await reader.cancel(err).catch(() => {}); throw err; } finally { try { - if (!readFailed) reader.releaseLock(); + // Always release: `reader.cancel()` does NOT drop the lock, and holding it would leave + // the stream permanently locked for any later consumer (audit R-WP5-2). + reader.releaseLock(); } catch { // already released / cancelled } @@ -571,11 +573,23 @@ export async function handleLive( // Record every completed upstream response before body size handling so account health / // cooldown still updates when we reject an oversized payload. relay.recordOutcome?.(upstreamResponse.status); - const payload = await readBodyCapped( - upstreamResponse.body, - LIVE_RESPONSE_MAX_BYTES, - total => `live response too large (${total} bytes)`, - ); + // Settle the body on abort before the reader attaches. Without this, a client cancel or the + // linked timeout landing between fetch resolution and `readBodyCapped`'s `getReader()` + // leaves Bun's internal read rejection orphaned off the awaited path, where no caller + // try/catch can intercept it (src/lib/abort.ts). The guard covers the window BEFORE the + // reader exists; once a reader holds the lock only the reader can cancel, which is why + // readBodyCapped also cancels on a failed read. Found while investigating #1419. + const detachBodyGuard = cancelBodyOnAbort(upstreamResponse.body, linkedSignal.signal); + let payload: ArrayBuffer | Response; + try { + payload = await readBodyCapped( + upstreamResponse.body, + LIVE_RESPONSE_MAX_BYTES, + total => `live response too large (${total} bytes)`, + ); + } finally { + detachBodyGuard(); + } if (payload instanceof Response) return payload; const relayHeaders: Record = {}; for (const name of LIVE_RELAY_HEADERS) { diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 3765ce3eee..afb8a671dc 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -2355,16 +2355,24 @@ async function handleResponsesInner( }); } if (!upstreamResponse.ok) { - if (options.comboAttempt) { - const failure = await consumeComboFailure(upstreamResponse, options.abortSignal); - options.onConsumedComboFailure?.(failure); - return failure.response; + // Same pre-read guard as the translated path's error branch: a non-2xx passthrough body + // is still a Bun fetch body, so an abort landing between fetch resolution and the + // consumer attaching its reader orphans the internal rejection (src/lib/abort.ts). + const detachPassthroughErrorGuard = cancelBodyOnAbort(upstreamResponse.body, upstream.signal); + try { + if (options.comboAttempt) { + const failure = await consumeComboFailure(upstreamResponse, options.abortSignal); + options.onConsumedComboFailure?.(failure); + return failure.response; + } + const errorText = await upstreamResponse.text().catch(() => ""); + return formatPassthroughUpstreamError(upstreamResponse.status, errorText, { + statusText: upstreamResponse.statusText, + headers, + }); + } finally { + detachPassthroughErrorGuard(); } - const errorText = await upstreamResponse.text().catch(() => ""); - return formatPassthroughUpstreamError(upstreamResponse.status, errorText, { - statusText: upstreamResponse.statusText, - headers, - }); } // Bun#32111 workaround: passthrough SSE uses tee()+native relay to avoid the diff --git a/tests/cancel-body-on-abort.test.ts b/tests/cancel-body-on-abort.test.ts index 29004a06d5..3bb38d7d13 100644 --- a/tests/cancel-body-on-abort.test.ts +++ b/tests/cancel-body-on-abort.test.ts @@ -31,9 +31,11 @@ describe("readBodyCapped settles the stream when a read throws", () => { await expect(readBodyCapped(failing, 1024, total => `too large (${total})`)).rejects.toThrow("upstream reset"); // The stream errored itself, so `cancel()` is not expected to have run. expect(cancelled).toBe(false); - // Locked-and-settled: a second reader is refused, proving no lock was leaked in a state - // where the body could still be pending. - expect(() => failing.getReader()).toThrow(); + // The lock is released even though the cancel rejected with the stored error: holding it + // would leave the stream permanently locked for any later consumer. A second reader is + // therefore obtainable, and it observes the original failure rather than hanging. + expect(failing.locked).toBe(false); + await expect(failing.getReader().read()).rejects.toThrow("upstream reset"); }); test("cancelBodyOnAbort cannot settle a body once a reader holds the lock", async () => { @@ -62,6 +64,32 @@ describe("readBodyCapped settles the stream when a read throws", () => { expect(cancelled).toBe(true); }); + // Wiring guard. The unit tests above exercise readBodyCapped and cancelBodyOnAbort + // directly, which means they ALL still pass when the /v1/live relay forgets to call the + // guard — an earlier revision of this change imported the helper and never invoked it, and + // no test noticed. Asserting the call site is crude but it is the thing that was actually + // missing. + test("the live relay attaches the body guard before consuming the upstream body", async () => { + const source = await Bun.file(new URL("../src/server/live.ts", import.meta.url)).text(); + + const guardAt = source.indexOf("cancelBodyOnAbort(upstreamResponse.body"); + const readAt = source.indexOf("payload = await readBodyCapped("); + expect(guardAt).toBeGreaterThan(-1); + expect(readAt).toBeGreaterThan(-1); + // Guard first, read second. + expect(guardAt).toBeLessThan(readAt); + // And detached on the normal path. + expect(source).toContain("detachBodyGuard()"); + }); + + 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("a normal read still returns the buffered payload", async () => { const body = new ReadableStream({ start(controller) { From 2b0bf78e08f721a7550bce3625bff2fe57d6cf84 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 12 Aug 2026 22:58:33 +0900 Subject: [PATCH 3/3] fix(responses): leave the combo failure branches unguarded, deliberately MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/server/responses/core.ts | 39 ++++++++++++++++++------------ tests/cancel-body-on-abort.test.ts | 23 ++++++++++++++++++ 2 files changed, 47 insertions(+), 15 deletions(-) diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index afb8a671dc..42e0e193b3 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -2355,16 +2355,22 @@ async function handleResponsesInner( }); } if (!upstreamResponse.ok) { - // Same pre-read guard as the translated path's error branch: a non-2xx passthrough body - // is still a Bun fetch body, so an abort landing between fetch resolution and the - // consumer attaching its reader orphans the internal rejection (src/lib/abort.ts). + if (options.comboAttempt) { + // No pre-read guard here: `consumeComboFailure` -> `readBoundedResponseBody` reads + // `response.body` itself and already threads the abort signal through its own read, + // and the combo contract is that this body's getter is touched exactly once (pinned by + // "captures passthrough failed usage from its original bounded body exactly once"). + // Attaching a guard would be a second `.body` access and break that contract for no + // gain, since the bounded reader owns settlement on this path. + const failure = await consumeComboFailure(upstreamResponse, options.abortSignal); + options.onConsumedComboFailure?.(failure); + return failure.response; + } + // The plain passthrough error path has no bounded reader of its own: `.text()` attaches + // the reader only when it runs, so an abort landing between fetch resolution and that + // call orphans Bun's internal read rejection (src/lib/abort.ts). const detachPassthroughErrorGuard = cancelBodyOnAbort(upstreamResponse.body, upstream.signal); try { - if (options.comboAttempt) { - const failure = await consumeComboFailure(upstreamResponse, options.abortSignal); - options.onConsumedComboFailure?.(failure); - return failure.response; - } const errorText = await upstreamResponse.text().catch(() => ""); return formatPassthroughUpstreamError(upstreamResponse.status, errorText, { statusText: upstreamResponse.statusText, @@ -3393,18 +3399,21 @@ async function handleResponsesInner( break; } if (!upstreamResponse.ok) { - // Guard the error body BEFORE anything reads it. The success path is guarded further - // down, but a non-2xx response is still a Bun fetch body: if the request aborts between - // fetch resolution and `.text()` attaching its reader, the orphaned internal rejection is - // the same uncatchable teardown `cancelBodyOnAbort` exists to absorb (src/lib/abort.ts). - // Found while investigating #1419; not a fix for the native trap reported there. - const detachErrorBodyGuard = cancelBodyOnAbort(upstreamResponse.body, upstream.signal); if (options.comboAttempt) { + // No pre-read guard: `consumeComboFailure` -> `readBoundedResponseBody` reads + // `response.body` itself with the abort signal threaded through, and the combo + // contract is that this body's getter is touched exactly once. A guard here would be + // a second `.body` access for no gain, since the bounded reader owns settlement. const failure = await consumeComboFailure(upstreamResponse, options.abortSignal) - .finally(() => { detachErrorBodyGuard(); cleanupUpstreamAbort(); }); + .finally(cleanupUpstreamAbort); options.onConsumedComboFailure?.(failure); return failure.response; } + // The plain error path has no bounded reader of its own: `.text()` attaches the reader + // only when it runs, so an abort landing between fetch resolution and that call orphans + // Bun's internal read rejection — the uncatchable teardown `cancelBodyOnAbort` absorbs + // (src/lib/abort.ts). Found while investigating #1419; not a fix for the native trap. + const detachErrorBodyGuard = cancelBodyOnAbort(upstreamResponse.body, upstream.signal); const errorText = await upstreamResponse.text().catch(() => "unknown error"); detachErrorBodyGuard(); cleanupUpstreamAbort(); diff --git a/tests/cancel-body-on-abort.test.ts b/tests/cancel-body-on-abort.test.ts index 3bb38d7d13..015a2bfcd5 100644 --- a/tests/cancel-body-on-abort.test.ts +++ b/tests/cancel-body-on-abort.test.ts @@ -90,6 +90,29 @@ describe("readBodyCapped settles the stream when a read throws", () => { expect(source).toContain("detachPassthroughErrorGuard"); }); + // The combo branches are deliberately NOT guarded: consumeComboFailure -> + // readBoundedResponseBody reads `response.body` itself with the abort signal threaded + // through, and the combo contract is that the getter is touched exactly once (pinned by + // "captures passthrough failed usage from its original bounded body exactly once" in + // tests/server-combo-failover-e2e.test.ts). An earlier revision guarded them anyway and + // broke that test by adding a second `.body` read. + test("the combo failure branches do not add a second body read", async () => { + const source = await Bun.file(new URL("../src/server/responses/core.ts", import.meta.url)).text(); + + for (const marker of ["const failure = await consumeComboFailure("]) { + let from = 0; + for (;;) { + const at = source.indexOf(marker, from); + if (at === -1) break; + // Look back a short window: no body guard may be attached immediately before a + // combo consumption. + const preceding = source.slice(Math.max(0, at - 400), at); + expect(preceding).not.toContain("cancelBodyOnAbort(upstreamResponse.body"); + from = at + marker.length; + } + } + }); + test("a normal read still returns the buffered payload", async () => { const body = new ReadableStream({ start(controller) {