From 927a70872d561cc032a98ca33aaf4e92a7dd1553 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 02:00:08 -0400 Subject: [PATCH] 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 be763289..09b02f18 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 7e938a7f..e50fa1e9 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