fix: release the discarded discovery response on every throw (#2319 round 22)

The timeout and cancellation exits added over the last few rounds drop
the original failed discovery response, which nobody will ever receive
— and an unread body holds its connection open on Node/undici. The
successful-substitution path already released it for that reason; the
five exceptional exits now do too (pre-abort, timeout, fetch-abort,
post-release abort, body-read abort). Matters most for a standalone
user of the exported wrapper, where no timeout wrapper underneath has
already drained it.

Also drops the last descriptions of the superseded rebuild: the test
file's header, an inline comment asserting the response is "not the
same object" when the file later asserts identity, and the PR
description's account of copied metadata and deleted encoding headers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-11 07:42:17 -04:00
commit 45d87911eb
3 changed files with 71 additions and 11 deletions

View file

@ -543,6 +543,41 @@ describe("probe cancellation (#2319)", () => {
expect(await settled).toBe(reason);
});
it("releases the discarded discovery response on an exceptional exit", async () => {
// Nobody receives `response` when the loop throws, and an unread body holds
// its connection open on Node/undici — the same reason the successful
// substitution path releases it.
const cancels = vi.fn<() => Promise<void>>().mockResolvedValue(undefined);
const discovery = new Response(
new ReadableStream<Uint8Array>({
start(ctrl) {
ctrl.enqueue(new TextEncoder().encode("not found"));
},
}),
{ status: 404 },
);
// Spy on the body's cancel, which is what `releaseBody` calls.
const body = discovery.body;
if (body) vi.spyOn(body, "cancel").mockImplementation(cancels);
const inner = vi.fn<typeof fetch>((input) =>
String(input) === RFC8414
? Promise.resolve(discovery)
: new Promise<Response>(() => {}),
);
const wrapped = withRfc8414OidcCompat(inner);
const settled = wrapped(RFC8414, {
signal: AbortSignal.abort(new Error("gone")),
}).then(
() => "resolved",
() => "threw",
);
expect(await settled).toBe("threw");
expect(cancels).toHaveBeenCalled();
});
it("still falls back to the original response on an ordinary probe failure", async () => {
const inner = vi.fn<typeof fetch>((input) => {
if (String(input) === RFC8414) {

View file

@ -7,10 +7,11 @@
* - **The bound itself**, including the *body*: `fetch` resolves on headers, so
* a server that sends them and then stalls has to be caught by the buffering
* race, not by the fetch promise.
* - **The rebuilt response.** Buffering means the caller gets a different
* `Response` object, so `url` / `redirected` / `type` are asserted to survive
* and `content-encoding` / `content-length` to be dropped — the buffer holds
* the decoded body, which makes both of those headers lies.
* - **The response's identity.** The bound drains a *clone* and hands back the
* original, so the cases assert identity, that the body is still readable
* after the drain, and that a caller's own `clone()` keeps its metadata —
* the property a rebuilt response could not have, since `url` / `redirected`
* / `type` are internal slots.
* - **Cancellation semantics.** The caller's signal is forwarded *and* raced,
* so the tests distinguish the two: one uses a fetch that honours the signal,
* another uses one that ignores it entirely.
@ -44,8 +45,9 @@ afterEach(() => {
describe("withOAuthRequestTimeout", () => {
it("passes a prompt response through", async () => {
// Not the same object: the body is buffered under the deadline (see the
// stalled-body case below) and the response rebuilt around it.
// The same object: the body is drained through a clone under the deadline
// (see the stalled-body case below), so nothing about the response the
// caller receives changes.
const inner = vi
.fn<typeof fetch>()
.mockResolvedValue(new Response('{"ok":true}', { status: 200 }));

View file

@ -271,7 +271,18 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch {
// rejects immediately on a pre-aborted signal, so this is mostly about
// not sending the request at all — the same reasoning as the
// short-circuit in `withOAuthRequestTimeout`.
if (callerSignal?.aborted) throw callerSignal.reason;
//
// Every exceptional exit below releases `response` first. On a normal
// return it is the value the caller gets and reads; on a throw nobody
// will ever read it, and an unread body holds its connection open on
// Node/undici — the same reason the successful-substitution path
// releases it (Copilot). Matters most for a standalone user of this
// exported wrapper, where no timeout wrapper is underneath to have
// already drained it.
if (callerSignal?.aborted) {
await releaseBody(response);
throw callerSignal.reason;
}
let probe: Response;
try {
@ -287,13 +298,19 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch {
// is one the SDK's own `buildDiscoveryUrls` emits, so a stall here is a
// stall the SDK would have hit itself, unbounded. Everything else — a
// CORS rejection, DNS, a reset — still falls back as documented above.
if (err instanceof OAuthRequestTimeoutError) throw err;
if (err instanceof OAuthRequestTimeoutError) {
await releaseBody(response);
throw err;
}
// Likewise a caller that gave up: substituting the preceding 404 for
// its own abort would report a discovery failure for a request it
// deliberately cancelled. The caller's own `reason` is what surfaces,
// here and at the two sites below, rather than whatever shape the
// underlying layer happened to reject with.
if (callerSignal?.aborted) throw callerSignal.reason;
if (callerSignal?.aborted) {
await releaseBody(response);
throw callerSignal.reason;
}
return response;
}
if (!probe.ok) {
@ -307,7 +324,10 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch {
// the body is being discarded must not be answered with the preceding
// response either, and `continue` would otherwise carry on probing for
// a caller that has stopped waiting.
if (callerSignal?.aborted) throw callerSignal.reason;
if (callerSignal?.aborted) {
await releaseBody(response);
throw callerSignal.reason;
}
if (continuesDiscovery(probe.status)) continue;
return response;
}
@ -331,7 +351,10 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch {
// discovery failure in place of its own abort reason (Copilot). A body
// that merely will not parse still falls through, which is the case
// this catch exists for.
if (callerSignal?.aborted) throw callerSignal.reason;
if (callerSignal?.aborted) {
await releaseBody(response);
throw callerSignal.reason;
}
return response;
}
if (!isRfc8414OnlyMetadata(parsed)) {