diff --git a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts index 14096711..c9ea94b9 100644 --- a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts +++ b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts @@ -578,6 +578,36 @@ describe("probe cancellation (#2319)", () => { expect(cancels).toHaveBeenCalled(); }); + it("does not wait on a cancel that never settles", async () => { + // ⚠️ `ReadableStream.cancel()` adopts the underlying source's cancel + // promise, which is permitted never to settle — so awaiting the release on + // a path that is propagating a cancellation would hang the very thing it + // was meant to end. + const discovery = new Response( + new ReadableStream({ + start(ctrl) { + ctrl.enqueue(new TextEncoder().encode("not found")); + }, + cancel() { + return new Promise(() => {}); + }, + }), + { status: 404 }, + ); + const inner = vi.fn((input) => + String(input) === RFC8414 + ? Promise.resolve(discovery) + : new Promise(() => {}), + ); + const wrapped = withRfc8414OidcCompat(inner); + const reason = new Error("gone"); + + // Would hang if the release were awaited; the assertion is that it settles. + await expect( + wrapped(RFC8414, { signal: AbortSignal.abort(reason) }), + ).rejects.toBe(reason); + }); + it("still falls back to the original response on an ordinary probe failure", async () => { const inner = vi.fn((input) => { if (String(input) === RFC8414) { diff --git a/core/auth/oidcDiscoveryCompat.ts b/core/auth/oidcDiscoveryCompat.ts index d6ca43d3..ce0ace87 100644 --- a/core/auth/oidcDiscoveryCompat.ts +++ b/core/auth/oidcDiscoveryCompat.ts @@ -170,6 +170,22 @@ async function releaseBody(response: Response): Promise { await response.body?.cancel().catch(() => {}); } +/** + * The same release, started but not waited on. + * + * ⚠️ `ReadableStream.cancel()` adopts the underlying source's cancel promise, + * which is permitted never to settle — so awaiting it on a path that is + * *propagating a cancellation* can hang the very thing that was meant to end + * (Copilot). Every exceptional exit below uses this: the release is a courtesy + * to the connection pool, and the caller's abort or timeout must reach it + * regardless. The `void` is the documented case where the callee owns its + * failures — `releaseBody` swallows its own — and the caller genuinely cannot + * await. + */ +function releaseBodyDetached(response: Response): void { + void releaseBody(response); +} + /** * Whether a parsed body is RFC 8414 authorization-server metadata that is *not* * a valid OpenID provider document — the exact shape the upstream schema @@ -280,7 +296,7 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // exported wrapper, where no timeout wrapper is underneath to have // already drained it. if (callerSignal?.aborted) { - await releaseBody(response); + releaseBodyDetached(response); throw callerSignal.reason; } @@ -299,7 +315,7 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // 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) { - await releaseBody(response); + releaseBodyDetached(response); throw err; } // Likewise a caller that gave up: substituting the preceding 404 for @@ -308,7 +324,7 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // here and at the two sites below, rather than whatever shape the // underlying layer happened to reject with. if (callerSignal?.aborted) { - await releaseBody(response); + releaseBodyDetached(response); throw callerSignal.reason; } return response; @@ -325,7 +341,7 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // response either, and `continue` would otherwise carry on probing for // a caller that has stopped waiting. if (callerSignal?.aborted) { - await releaseBody(response); + releaseBodyDetached(response); throw callerSignal.reason; } if (continuesDiscovery(probe.status)) continue; @@ -352,7 +368,7 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // that merely will not parse still falls through, which is the case // this catch exists for. if (callerSignal?.aborted) { - await releaseBody(response); + releaseBodyDetached(response); throw callerSignal.reason; } return response; diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 7740b58c..7b771919 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -56,6 +56,8 @@ * shape, as `revokeToken`. */ +import { redactUrlQuery } from "../mcp/fetchTracking.js"; + /** * 30 seconds. Deliberately generous: discovery against a slow or cold-starting * authorization server is legitimate, and a bound that fires on a server that @@ -63,8 +65,6 @@ * halves the 60s the SDK handshake incidentally provides, and it is the only * bound at all on the legs that sit outside an SDK request. */ -import { redactUrlQuery } from "../mcp/fetchTracking.js"; - export const DEFAULT_OAUTH_REQUEST_TIMEOUT_MS = 30_000; /**