fix: cancellable compat probes and a validated budget (#2319 round 16)

Two real bugs and two stale rationales.

The OIDC fallback probes were issued with `{ headers }` and no signal,
so a caller that aborted after the initial 404 got a timeout reported
in place of its own cancellation, while the probe ran on to its own
deadline. The probes now carry the caller's signal — via the shared
`callerSignalOf`, so the precedence rule has one definition — a
pre-aborted caller is not probed at all, and an abort is rethrown
rather than folded back into the preceding response.

`Math.max(0, Math.round(timeoutMs))` let NaN and Infinity through.
`setTimeout(fn, NaN)` fires immediately, and a delay above 2**31-1
overflows a 32-bit signed int and falls back to 1ms — so a caller
asking for a very large budget, or one whose arithmetic produced NaN,
got an immediate OAuth timeout. `normalizeBudget` falls back to the
default on non-finite and clamps to the timer's range; the route's
envelope-supplied deadline gets the same clamp, but ignores a
non-finite value outright since a malformed deadline reads safest as
no deadline.

The wire-marker docblock and the matching route test still said a
backend deadline is the only one on the transport-internal discovery
path, which round 11 changed. Both now give the real reason the marker
matters: both ends run the same budget, and it is the backend winning
the race — a backgrounded tab throttling setTimeout — that must not
decide whether the error names its endpoint.

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>

clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts

@@ -439,3 +439,82 @@ describe("withRfc8414OidcCompat", () => {
     expect(base).toHaveBeenCalledTimes(2);
   });
 });
+
+describe("probe cancellation (#2319)", () => {
+  const RFC8414 =
+    "https://as.example.com/.well-known/oauth-authorization-server/tenant";
+
+  /** 404 on the RFC 8414 leg, then never answers the OIDC probe. */
+  function stallingProbeFetch() {
+    return vi.fn<typeof fetch>((input, init) => {
+      if (String(input) === RFC8414) {
+        return Promise.resolve(new Response(null, { status: 404 }));
+      }
+      return new Promise<Response>((_resolve, reject) => {
+        // A real `fetch` rejects immediately on a signal that is already
+        // aborted, so the double is only useful if it does the same.
+        if (init?.signal?.aborted) {
+          reject(init.signal.reason as Error);
+          return;
+        }
+        init?.signal?.addEventListener("abort", () =>
+          reject(init.signal?.reason as Error),
+        );
+      });
+    });
+  }
+
+  it("carries the caller's signal into the probe", async () => {
+    const inner = stallingProbeFetch();
+    const wrapped = withRfc8414OidcCompat(inner);
+    const caller = new AbortController();
+
+    const settled = wrapped(RFC8414, { signal: caller.signal }).then(
+      (r) => r,
+      (e: unknown) => e,
+    );
+    // Wait until the probe is actually in flight, so the abort exercises
+    // cancellation rather than the pre-aborted short-circuit.
+    await vi.waitFor(() => {
+      expect(inner).toHaveBeenCalledTimes(2);
+    });
+    const reason = new Error("caller gave up");
+    caller.abort(reason);
+
+    // Not the preceding 404: substituting it would report a discovery failure
+    // for a request the caller deliberately cancelled.
+    expect(await settled).toBe(reason);
+    expect((inner.mock.calls[1][1] as RequestInit).signal).toBe(caller.signal);
+  });
+
+  it("does not issue a probe the caller already cancelled", async () => {
+    const inner = stallingProbeFetch();
+    const wrapped = withRfc8414OidcCompat(inner);
+    const reason = new Error("already gone");
+
+    const settled = wrapped(RFC8414, {
+      signal: AbortSignal.abort(reason),
+    }).then(
+      (r) => r,
+      (e: unknown) => e,
+    );
+
+    expect(await settled).toBe(reason);
+    // Only the RFC 8414 leg, which the caller did make.
+    expect(inner).toHaveBeenCalledTimes(1);
+  });
+
+  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) {
+        return Promise.resolve(new Response(null, { status: 404 }));
+      }
+      return Promise.reject(new TypeError("network error"));
+    });
+    const wrapped = withRfc8414OidcCompat(inner);
+
+    const response = await wrapped(RFC8414);
+
+    expect(response.status).toBe(404);
+  });
+});

clients/web/src/test/core/auth/requestTimeout.test.ts

@@ -387,6 +387,46 @@ describe("withOAuthRequestTimeout", () => {
     });
   });
 
+  it("falls back to the default on a non-finite budget", async () => {
+    vi.useFakeTimers();
+    // `NaN` survives `Math.max(0, Math.round(NaN))` and `setTimeout(fn, NaN)`
+    // fires immediately, so without this every OAuth request under a budget
+    // that came out of bad arithmetic would fail at once with "timed out after
+    // NaNms".
+    for (const bad of [NaN, Infinity, -Infinity]) {
+      const wrapped = withOAuthRequestTimeout(neverSettles, bad);
+      const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err);
+
+      await vi.advanceTimersByTimeAsync(DEFAULT_OAUTH_REQUEST_TIMEOUT_MS - 1);
+      expect(await Promise.race([assertion, Promise.resolve("pending")])).toBe(
+        "pending",
+      );
+
+      await vi.advanceTimersByTimeAsync(1);
+      expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({
+        timeoutMs: DEFAULT_OAUTH_REQUEST_TIMEOUT_MS,
+      });
+    }
+  });
+
+  it("clamps an over-large budget to what setTimeout can schedule", async () => {
+    vi.useFakeTimers();
+    // Past 2**31-1 the delay overflows a 32-bit signed int and Node falls back
+    // to 1ms — an immediate timeout, the opposite of what was asked for.
+    const wrapped = withOAuthRequestTimeout(neverSettles, 2 ** 40);
+
+    const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err);
+    await vi.advanceTimersByTimeAsync(1000);
+    expect(await Promise.race([assertion, Promise.resolve("pending")])).toBe(
+      "pending",
+    );
+
+    await vi.advanceTimersByTimeAsync(2_147_483_647);
+    expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({
+      timeoutMs: 2_147_483_647,
+    });
+  });
+
   it("clamps a negative budget to zero rather than throwing", async () => {
     vi.useFakeTimers();
     const wrapped = withOAuthRequestTimeout(neverSettles, -1);

clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts

@@ -473,10 +473,12 @@ describe("server.ts supplemental coverage", () => {
 
     it("answers 504 with a typed marker when the request's deadline fires (#2319)", async () => {
       // The caller's budget travels in the envelope, so this costs milliseconds
-      // rather than the production thirty seconds. The marker matters because
-      // a backend-enforced deadline is the only one on the transport-internal
-      // discovery path, and its error has to survive the hop as something an
-      // `instanceof OAuthRequestTimeoutError` check can still recognize.
+      // rather than the production thirty seconds. What it pins is typed-marker
+      // preservation when the *backend* wins the client-side race — both ends
+      // run the same budget, and a backgrounded tab throttling `setTimeout` is
+      // the plausible way the server's fires first. The error has to survive
+      // the hop as something an `instanceof OAuthRequestTimeoutError` check can
+      // still recognize, whichever end produced it.
       const res = await fetch(`${h.baseUrl}/api/fetch`, {
         method: "POST",
         headers: { "content-type": "application/json" },

core/auth/oidcDiscoveryCompat.ts

@@ -90,7 +90,7 @@
  * wiring in `core/mcp/inspectorClient.ts`.
  */
 
-import { OAuthRequestTimeoutError } from "./requestTimeout.js";
+import { callerSignalOf, OAuthRequestTimeoutError } from "./requestTimeout.js";
 import {
   OAuthMetadataSchema,
   OpenIdProviderDiscoveryMetadataSchema,
@@ -249,6 +249,13 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch {
     if (candidates.length === 0) return response;
 
     const headers = discoveryHeaders(input, init);
+    // The probe is a request the caller never made, issued on its behalf — so
+    // it has to be cancellable by the caller too. Without the signal, aborting
+    // after the initial 404 left the probe running to its own deadline and the
+    // caller was handed a timeout in place of its own abort (Copilot). Same
+    // precedence rule as everywhere else, hence the shared helper rather than a
+    // second copy of it.
+    const callerSignal = callerSignalOf(input, init);
     // The loop advances to the next candidate only where the SDK's own loop
     // would. Anywhere else it hands the original response back and lets
     // discovery run its normal course, because *promoting a later candidate
@@ -260,9 +267,15 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch {
     // those, returning `response` leaves the SDK to make the same request and
     // reach the same verdict it always would.
     for (const candidate of candidates) {
+      // Don't issue a probe the caller has already given up on. A real `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;
+
       let probe: Response;
       try {
-        probe = await fetchFn(candidate, { headers });
+        probe = await fetchFn(candidate, { headers, signal: callerSignal });
       } catch (err) {
         // A deadline the Inspector imposed is *our* error, not the server's
         // answer, so it escapes rather than being folded back into the original
@@ -275,6 +288,10 @@ 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) 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.
+        if (callerSignal?.aborted) throw err;
         return response;
       }
       if (!probe.ok) {

core/auth/requestTimeout.ts

@@ -83,6 +83,33 @@ export class OAuthRequestTimeoutError extends Error {
   }
 }
 
+/**
+ * The largest delay `setTimeout` can actually schedule. Past `2 ** 31 - 1` the
+ * delay overflows a 32-bit signed integer and Node falls back to **1ms**, so an
+ * `Infinity` or an over-large budget produces an *immediate* timeout — the exact
+ * opposite of what the caller asked for (Copilot).
+ */
+const MAX_TIMER_DELAY_MS = 2_147_483_647;
+
+/**
+ * Coerce a caller's budget into something `setTimeout` can honour and the error
+ * can honestly report.
+ *
+ * - Non-finite (`NaN`, `±Infinity`) falls back to the default. `NaN` survives
+ *   `Math.max(0, Math.round(...))` and `setTimeout(fn, NaN)` fires immediately,
+ *   so a caller whose arithmetic produced a `NaN` would otherwise see every
+ *   OAuth request fail at once with "timed out after NaNms".
+ * - Above `MAX_TIMER_DELAY_MS` it clamps rather than overflowing to 1ms.
+ * - Negative clamps to 0, which is a real budget (fire on the next tick), not
+ *   an error.
+ *
+ * Rounded because the budget is *reported* — see the note at the call site.
+ */
+function normalizeBudget(timeoutMs: number): number {
+  if (!Number.isFinite(timeoutMs)) return DEFAULT_OAUTH_REQUEST_TIMEOUT_MS;
+  return Math.min(MAX_TIMER_DELAY_MS, Math.max(0, Math.round(timeoutMs)));
+}
+
 /**
  * How a proxy-side deadline is carried back to the browser (#2319).
  *
@@ -90,10 +117,12 @@ export class OAuthRequestTimeoutError extends Error {
  * `createRemoteFetch` turns any non-OK answer into a plain `Error` — so a
  * timeout enforced by the *backend* would arrive as an untyped error and the
  * `instanceof OAuthRequestTimeoutError` checks downstream would all be false
- * (Copilot). That matters most exactly where there is no client-side wrapper to
- * have fired first: the discovery the SDK runs from inside the transport. So
- * the route stamps this marker and `createRemoteFetch` reconstructs the typed
- * error from it.
+ * (Copilot). Both ends run the same budget, so the client's race usually
+ * settles first; this matters when the backend's timer wins anyway — most
+ * plausibly a backgrounded tab, where the browser throttles `setTimeout` while
+ * the server's fires on schedule. Which of two equal deadlines happened to fire
+ * must not decide whether the error names its endpoint. So the route stamps
+ * this marker and `createRemoteFetch` reconstructs the typed error from it.
  */
 export const OAUTH_TIMEOUT_WIRE_CODE = "oauth_request_timeout";
 
@@ -204,7 +233,7 @@ function requestUrlOf(input: RequestInfo | URL): string {
  * `Request`'s signal rather than clear it (Copilot). An explicit `null` is the
  * genuine "no signal" override and is honoured as one.
  */
-function callerSignalOf(
+export function callerSignalOf(
   input: RequestInfo | URL,
   init: RequestInit | undefined,
 ): AbortSignal | undefined {
@@ -324,7 +353,7 @@ export function withOAuthRequestTimeout(
   // public field. Rounding rather than flooring so a caller's own
   // whole-millisecond timeout survives the trip through that clock and is still
   // the number the message names.
-  const budget = Math.max(0, Math.round(timeoutMs));
+  const budget = normalizeBudget(timeoutMs);
 
   return async (input, init) => {
     const url = requestUrlOf(input);

core/mcp/remote/node/server.ts

@@ -1256,10 +1256,15 @@ export function createRemoteApp(
     // the regression `exemptMcpEndpoint` exists to prevent on the client, and
     // reporting it as an OAuth timeout besides (Copilot). No deadline in the
     // envelope means no timer at all.
+    // Clamped to what `setTimeout` can schedule: past 2**31-1 the delay
+    // overflows and Node falls back to 1ms, turning an over-large budget into
+    // an immediate timeout. Non-finite is ignored outright rather than
+    // defaulted — an envelope carrying `NaN` is a malformed request, and the
+    // safe reading of a malformed deadline is "no deadline".
     const deadlineMs =
       typeof requestedTimeoutMs === "number" &&
       Number.isFinite(requestedTimeoutMs)
-        ? Math.max(0, Math.round(requestedTimeoutMs))
+        ? Math.min(2_147_483_647, Math.max(0, Math.round(requestedTimeoutMs)))
         : undefined;
     const timer =
       deadlineMs === undefined