fix: do not await the release on a cancellation path (#2319 round 25)
`ReadableStream.cancel()` adopts the underlying source's cancel promise, which is permitted never to settle — so awaiting `releaseBody` while propagating a cancellation can hang the very thing it was meant to end. The five exceptional exits in the compat probe loop now start the release without waiting on it: the release is a courtesy to the connection pool, and the caller's abort or timeout must reach it regardless. The two non-exceptional awaits are unchanged. Same class as the tee teardown in round 24, one file over. Also moves the `redactUrlQuery` import above DEFAULT_OAUTH_REQUEST_TIMEOUT_MS's JSDoc, which it had split from the constant — so hover and API tooling see the doc again. Verified by mutation: awaiting the release makes the new test time out. 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:
parent
78f82475c4
commit
b4c02865e4
3 changed files with 53 additions and 7 deletions
|
|
@ -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<Uint8Array>({
|
||||
start(ctrl) {
|
||||
ctrl.enqueue(new TextEncoder().encode("not found"));
|
||||
},
|
||||
cancel() {
|
||||
return new Promise<void>(() => {});
|
||||
},
|
||||
}),
|
||||
{ status: 404 },
|
||||
);
|
||||
const inner = vi.fn<typeof fetch>((input) =>
|
||||
String(input) === RFC8414
|
||||
? Promise.resolve(discovery)
|
||||
: new Promise<Response>(() => {}),
|
||||
);
|
||||
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<typeof fetch>((input) => {
|
||||
if (String(input) === RFC8414) {
|
||||
|
|
|
|||
|
|
@ -170,6 +170,22 @@ async function releaseBody(response: Response): Promise<void> {
|
|||
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;
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Reference in a new issue