From 84a65f28d89275dc622ad96593da3287bc8505c0 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 01:25:25 -0400 Subject: [PATCH 1/3] fix(auth): floor the shared revocation budget instead of racing zero MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shared teardown deadline was decided on `remainingMs > 0` against `Date.now()`. Both halves of that are noise-sensitive: the deadline is enforced by a `setTimeout`, and a wall clock at millisecond resolution can still read a hair short of it when the loop comes round, so the next grant inherits a fractional budget and spends it on a request that cannot possibly complete. The same run then issues one request or two depending on scheduling — which is what makes the "shares one deadline across grants" test intermittent on CI (#2252). Measure the deadline with `performance.now()` (monotonic, so an NTP step or a suspend cannot expire or extend the budget, and sub-millisecond, so none is lost to rounding), and skip any grant reaching the loop with less than MIN_REVOCATION_REQUEST_BUDGET_MS left. The skipped grant is already reported as `failed`, so nothing is silently dropped — the needless call to the authorization server is. The CLI's outer per-plan budget in `sendPlans` shares both the shape and the defect, so it gets the same treatment. One consequence worth naming: a monotonic clock hands `revokeToken` a fractional budget, and Node's `AbortSignal.timeout` throws `ERR_OUT_OF_RANGE` on a non-integer delay — before the fetch, so the request would never be sent and the revocation would report that as its failure. `revokeToken` now rounds its own budget to whole milliseconds, which also covers any other caller passing a fractional one. Tests: the epsilon skip is pinned by a grant that lands inside the floor but above zero (removing the floor fails it, verified); the fractional budget by a `revokeToken` call at 19.996ms. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017wEXtbs8UEHUMxDEAxbs99 Signed-off-by: cliffhall --- .../cli/src/clear-stored-auth-for-relogin.ts | 9 ++- .../web/src/test/core/auth/revocation.test.ts | 64 +++++++++++++++++++ core/auth/index.ts | 1 + core/auth/revocation.ts | 43 +++++++++++-- 4 files changed, 110 insertions(+), 7 deletions(-) diff --git a/clients/cli/src/clear-stored-auth-for-relogin.ts b/clients/cli/src/clear-stored-auth-for-relogin.ts index 6ec64e6e8..a67695d9d 100644 --- a/clients/cli/src/clear-stored-auth-for-relogin.ts +++ b/clients/cli/src/clear-stored-auth-for-relogin.ts @@ -4,6 +4,7 @@ import { } from "@inspector/core/auth/node/storage-node.js"; import { DEFAULT_REVOCATION_TIMEOUT_MS, + MIN_REVOCATION_REQUEST_BUDGET_MS, clearAndPlanRevocation, executeOAuthRevocation, type OAuthRevocationPlan, @@ -113,17 +114,19 @@ async function sendPlans( budgetMs: number, ): Promise { const fetchFn = createProxyFetch() ?? fetch; - const deadlineAt = Date.now() + budgetMs; + // Monotonic and epsilon-floored for the same reasons as the shared deadline + // inside `executeOAuthRevocation` — see MIN_REVOCATION_REQUEST_BUDGET_MS. + const deadlineAt = performance.now() + budgetMs; let reported: TokenRevocationOutcome | undefined; let lastSkip: TokenRevocationOutcome | undefined; for (const plan of plans) { - const remainingMs = deadlineAt - Date.now(); + const remainingMs = deadlineAt - performance.now(); // A plan that already knows its answer needs no network, so the budget is // irrelevant to it. Synthesising exhaustion here would warn that a grant // may still be live when the key held no grant at all — a false alarm, and // one that outranks the real outcome under the failure-first rule below. const needsNetwork = plan.outcome === undefined; - if (needsNetwork && remainingMs <= 0) { + if (needsNetwork && remainingMs <= MIN_REVOCATION_REQUEST_BUDGET_MS) { // Overrides an earlier success rather than deferring to it: this key's // grant may still be live at the authorization server, and that is the // thing the user needs to hear about. Same failure-first rule as below. diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index fe5c29f15..7e938a7f1 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -3,6 +3,7 @@ import type { OAuthMetadata } from "@modelcontextprotocol/client"; import { BrowserOAuthStorage } from "@inspector/core/auth/browser/storage.js"; import { DEFAULT_REVOCATION_TIMEOUT_MS, + MIN_REVOCATION_REQUEST_BUDGET_MS, aggregateOutcomes, buildRevocationRequest, revocationAuthMethods, @@ -431,6 +432,27 @@ describe("revokeToken", () => { expect(seen?.signal).toBeInstanceOf(AbortSignal); expect(DEFAULT_REVOCATION_TIMEOUT_MS).toBeGreaterThan(0); }); + + // What is left of a shared deadline is measured with a sub-millisecond clock, + // so the budget handed down here is routinely fractional — and Node's + // `AbortSignal.timeout` throws `ERR_OUT_OF_RANGE` on a non-integer delay, + // before the fetch, turning a perfectly good request into a revocation failure + // that never left the process (#2252). + it("accepts a fractional budget and still sends the request", async () => { + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + const outcome = await revokeToken({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + supportedAuthMethods: [], + fetchFn, + timeoutMs: 19.996, + }); + expect(fetchFn).toHaveBeenCalledTimes(1); + expect(outcome).toMatchObject({ status: "revoked" }); + }); }); /** @@ -838,6 +860,48 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => { expect(fetchFn).toHaveBeenCalledTimes(1); }); + // The boundary the shared budget used to be decided on was `remainingMs > 0`, + // which timer resolution can land either side of: a grant that finished a + // hair before the deadline left a fractional budget behind, and the next grant + // spent it on a request that could not possibly complete (#2252). The floor + // makes that decision the same on every run. + it("does not issue a request with less than the minimum budget left", async () => { + const grant = (n: string) => ({ + issuer: "https://as.example.com", + token: `r-${n}`, + tokenTypeHint: "refresh_token" as const, + }); + const timeoutMs = 40; + // Lands the second grant inside the floor but *above* zero — the sliver the + // old bound would have spent. A slow machine only pushes the remainder + // further below the floor, so the assertion cannot flip the other way. + const firstRequestMs = timeoutMs - MIN_REVOCATION_REQUEST_BUDGET_MS + 1; + const fetchFn = vi.fn(async () => { + await new Promise((resolve) => setTimeout(resolve, firstRequestMs)); + return new Response(null, { status: 200 }); + }); + + const outcome = await executeOAuthRevocation( + { + serverUrl: SERVER_URL, + grants: [grant("a"), grant("b")], + failures: [], + endpoint: REVOKE_URL, + supportedAuthMethods: [], + metadataIssuer: "https://as.example.com", + }, + { fetchFn, timeoutMs }, + ); + + expect(fetchFn).toHaveBeenCalledTimes(1); + // The unattempted grant outranks the first one's success, so the caller + // hears that a grant may still be live rather than that all was well. + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "budget was exhausted", + ); + }); + // A grant bound to an issuer the cached metadata does not describe cannot be // revoked — that endpoint belongs to a different authorization server, and // sending it another AS's token would hand a credential to a server that diff --git a/core/auth/index.ts b/core/auth/index.ts index 2ba73e832..c74702ce3 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -142,6 +142,7 @@ export { discoverScopes } from "./discovery.js"; // RFC 7009 token revocation (#2144) export { DEFAULT_REVOCATION_TIMEOUT_MS, + MIN_REVOCATION_REQUEST_BUDGET_MS, aggregateOutcomes, buildRevocationRequest, revocationAuthMethods, diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index 3514e4a43..f2d16c0f9 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -47,6 +47,25 @@ import type { OAuthStorage, RevocationSnapshot } from "./storage.js"; */ export const DEFAULT_REVOCATION_TIMEOUT_MS = 5000; +/** + * The least budget worth spending a revocation request on. + * + * The shared deadline below is consumed sequentially, so the grant that follows + * a slow one can arrive with a sliver of budget left — a couple of milliseconds, + * which is less than a TCP handshake, let alone a round trip. Issuing that + * request buys nothing: it is guaranteed to time out, and its outcome is the + * same `failed` the exhausted-budget branch already reports, only after a + * needless call to the authorization server. + * + * It also removes a boundary that nothing can land on cleanly. `remainingMs > 0` + * is decided by timer resolution: the deadline is enforced by a `setTimeout`, + * and a clock that has not yet ticked past the deadline when the loop comes + * round leaves a fractional budget behind, so the same run issues one request or + * two depending on scheduling noise (#2252). A floor an order of magnitude above + * that noise makes the decision the same every time. + */ +export const MIN_REVOCATION_REQUEST_BUDGET_MS = 5; + /** Why a revocation request was not sent. */ export type TokenRevocationSkipReason = /** The caller turned revocation off for this server. */ @@ -231,7 +250,17 @@ export interface RevokeTokenParams extends RevocationRequestParams { export async function revokeToken( params: RevokeTokenParams, ): Promise { - const timeoutMs = params.timeoutMs ?? DEFAULT_REVOCATION_TIMEOUT_MS; + // Whole milliseconds, because `AbortSignal.timeout` takes an integer: Node + // throws `ERR_OUT_OF_RANGE` on a fractional delay — before the fetch, so the + // request is never sent and the caller gets that as the revocation's failure + // detail. What is left of a shared deadline is measured with a + // sub-millisecond clock, so a fractional budget does arrive here. Rounded + // rather than floored so a caller's own whole-millisecond timeout survives the + // trip through that clock and is still the number the timeout message names. + const timeoutMs = Math.max( + 0, + Math.round(params.timeoutMs ?? DEFAULT_REVOCATION_TIMEOUT_MS), + ); try { // Inside the try: `encodeURIComponent` throws on a lone UTF-16 surrogate, // which is valid JSON and so can reach here from a persisted client id or @@ -692,7 +721,11 @@ async function runPlan( // on purpose (a burst of parallel requests to one authorization server is // not a kindness), so the budget is shared instead. const timeoutMs = params.timeoutMs ?? DEFAULT_REVOCATION_TIMEOUT_MS; - const deadlineAt = Date.now() + timeoutMs; + // `performance.now()` rather than `Date.now()`: this is an elapsed-time + // measurement, and a wall clock can be stepped by NTP or a suspend/resume + // mid-teardown, which would either expire the budget early or extend it. It is + // also sub-millisecond, so a budget is not spent or preserved by rounding. + const deadlineAt = performance.now() + timeoutMs; for (const grant of plan.grants) { // Metadata is cached once per server, not per issuer, so it describes @@ -735,8 +768,10 @@ async function runPlan( continue; } - const remainingMs = deadlineAt - Date.now(); - if (remainingMs <= 0) { + const remainingMs = deadlineAt - performance.now(); + // Not `<= 0`: see MIN_REVOCATION_REQUEST_BUDGET_MS. A budget too small to + // complete a request is treated as no budget at all. + if (remainingMs <= MIN_REVOCATION_REQUEST_BUDGET_MS) { outcomes.push({ status: "failed", endpoint, From c736f1d1857ef0db2ee27d47f9888dbdf1056a80 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 01:43:01 -0400 Subject: [PATCH 2/3] test(cli): pin the CLI's own minimum-budget floor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot round 1 on #2256: the `budgetMs: 0` case passes under the old `remainingMs <= 0` bound too, so nothing in the CLI suite detected the floor added to `sendPlans`. The new case gives it a budget that is positive and below the floor, and asserts on *this loop's* exhaustion message — the one naming the server URL. That distinction is the test: with the floor removed the plan is handed a 4ms budget and core's identical floor declines it one level down, so a bare "budget was exhausted" match passes either way. Verified as a detector — 1 failed / 14 passed with the CLI floor reverted, 15 passed with it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017wEXtbs8UEHUMxDEAxbs99 Signed-off-by: cliffhall --- .../clear-stored-auth-for-relogin.test.ts | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts index 9b5e587d5..be7632897 100644 --- a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts +++ b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts @@ -6,6 +6,7 @@ import { getStateFilePath, resetNodeOAuthStorageCache, } from "@inspector/core/auth/node/storage-node.js"; +import { MIN_REVOCATION_REQUEST_BUDGET_MS } from "@inspector/core/auth/revocation.js"; import { clearStoredAuthForRelogin } from "../src/clear-stored-auth-for-relogin.js"; const AS_ISSUER = "https://as.example.com"; @@ -379,6 +380,35 @@ describe("clearStoredAuthForRelogin", () => { } }); + // The zero-budget case above passes under the old `remainingMs <= 0` bound + // too, so it says nothing about the floor. This one is the floor's own + // detector: a budget that is genuinely positive, and genuinely too small to + // complete a request, must be spent on no request at all (#2252). The + // budget only shrinks as the loop runs, so a slow machine cannot flip it. + it("issues no request for a positive budget below the minimum", async () => { + seedBothSpellings("live-r", "stale-r"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response(null, { status: 200 })); + try { + const outcome = await clearStoredAuthForRelogin("https://example.com", { + budgetMs: MIN_REVOCATION_REQUEST_BUDGET_MS - 1, + }); + expect(fetchSpy).not.toHaveBeenCalled(); + expect(outcome).toMatchObject({ status: "failed" }); + // The *plan* was never attempted, so the report has to be this loop's + // own — naming the server URL — and not the per-grant one from inside + // `executeOAuthRevocation`. Without that distinction the assertion + // passes on the old `<= 0` bound too: the plan would be handed a 4ms + // budget, and core's identical floor would decline it one level down. + expect(outcome?.status === "failed" ? outcome.detail : "").toContain( + 'budget was exhausted before "', + ); + } finally { + fetchSpy.mockRestore(); + } + }); + it("reports a later failure over an earlier success", async () => { seedBothSpellings("live-r", "stale-r"); const fetchSpy = vi From 927a70872d561cc032a98ca33aaf4e92a7dd1553 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 02:00:08 -0400 Subject: [PATCH 3/3] test(auth): stub the clock in both floor regression tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot round 3 on #2256: both detectors were one-sided. Each measured the sub-floor remainder against the real clock, so a preempted worker overshoots the deadline, the remainder goes negative, and the unfixed `remainingMs <= 0` bound takes the same branch and prints the same message — the test passes on an implementation with no floor. It could never fail wrongly, but it could silently stop testing anything, which on a flake fix is the wrong half of that guarantee to keep. Both now stub `performance.now()`: the web case advances it inside the fetch so the second grant's remainder is exactly MIN_REVOCATION_REQUEST_BUDGET_MS - 1, and the CLI case freezes it so the remainder at the check is the budget itself. Positive and under the floor on every machine, which is the only state that separates the two bounds. Verified as detectors with the clock stubbed: reverting the core floor gives 1 failed / 57 passed, reverting the CLI floor 1 failed / 14 passed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017wEXtbs8UEHUMxDEAxbs99 Signed-off-by: cliffhall --- .../clear-stored-auth-for-relogin.test.ts | 7 +++ .../web/src/test/core/auth/revocation.test.ts | 56 +++++++++++-------- 2 files changed, 40 insertions(+), 23 deletions(-) diff --git a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts index be7632897..09b02f18f 100644 --- a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts +++ b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts @@ -390,6 +390,12 @@ describe("clearStoredAuthForRelogin", () => { const fetchSpy = vi .spyOn(globalThis, "fetch") .mockResolvedValue(new Response(null, { status: 200 })); + // Frozen, so the remainder at the check is exactly the budget. Left to + // the real clock this is a one-sided detector: a worker preempted for + // more than the budget reaches the check with a NEGATIVE remainder, where + // the unfixed `remainingMs <= 0` bound takes the same branch and prints + // the same message — passing without the floor (Copilot). + const nowSpy = vi.spyOn(performance, "now").mockReturnValue(1_000); try { const outcome = await clearStoredAuthForRelogin("https://example.com", { budgetMs: MIN_REVOCATION_REQUEST_BUDGET_MS - 1, @@ -405,6 +411,7 @@ describe("clearStoredAuthForRelogin", () => { 'budget was exhausted before "', ); } finally { + nowSpy.mockRestore(); fetchSpy.mockRestore(); } }); diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index 7e938a7f1..e50fa1e96 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -872,34 +872,44 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => { tokenTypeHint: "refresh_token" as const, }); const timeoutMs = 40; - // Lands the second grant inside the floor but *above* zero — the sliver the - // old bound would have spent. A slow machine only pushes the remainder - // further below the floor, so the assertion cannot flip the other way. - const firstRequestMs = timeoutMs - MIN_REVOCATION_REQUEST_BUDGET_MS + 1; + // The clock is stubbed rather than slept against. A real sleep makes this a + // ONE-SIDED detector: under contention the first request overruns, the + // remainder goes negative, and the unfixed `remainingMs <= 0` bound skips + // the second grant for the wrong reason — so the test passes on an + // implementation that has no floor at all. Advancing a stub inside the + // fetch puts the second grant's remainder at exactly + // `MIN_REVOCATION_REQUEST_BUDGET_MS - 1` on every machine: positive, and + // under the floor, which is the only state that tells the two apart. + let now = 1_000; + const nowSpy = vi.spyOn(performance, "now").mockImplementation(() => now); const fetchFn = vi.fn(async () => { - await new Promise((resolve) => setTimeout(resolve, firstRequestMs)); + now += timeoutMs - MIN_REVOCATION_REQUEST_BUDGET_MS + 1; return new Response(null, { status: 200 }); }); - const outcome = await executeOAuthRevocation( - { - serverUrl: SERVER_URL, - grants: [grant("a"), grant("b")], - failures: [], - endpoint: REVOKE_URL, - supportedAuthMethods: [], - metadataIssuer: "https://as.example.com", - }, - { fetchFn, timeoutMs }, - ); + try { + const outcome = await executeOAuthRevocation( + { + serverUrl: SERVER_URL, + grants: [grant("a"), grant("b")], + failures: [], + endpoint: REVOKE_URL, + supportedAuthMethods: [], + metadataIssuer: "https://as.example.com", + }, + { fetchFn, timeoutMs }, + ); - expect(fetchFn).toHaveBeenCalledTimes(1); - // The unattempted grant outranks the first one's success, so the caller - // hears that a grant may still be live rather than that all was well. - expect(outcome).toMatchObject({ status: "failed" }); - expect(outcome.status === "failed" ? outcome.detail : "").toContain( - "budget was exhausted", - ); + expect(fetchFn).toHaveBeenCalledTimes(1); + // The unattempted grant outranks the first one's success, so the caller + // hears that a grant may still be live rather than that all was well. + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "budget was exhausted", + ); + } finally { + nowSpy.mockRestore(); + } }); // A grant bound to an issuer the cached metadata does not describe cannot be