fix(auth): floor the shared revocation budget instead of racing zero

[cliffhall]

Closes #2252

The race

executeOAuthRevocation spends one budget across every grant, and decided whether the next grant got a turn with remainingMs = deadlineAt - Date.now() against remainingMs <= 0. Both halves of that sit on noise:

• the deadline is enforced by a setTimeout, so the grant that consumed it can return with the clock a hair short of deadlineAt;
• Date.now() is millisecond-resolution, so it can read short of the deadline it has actually passed.

Either way the next grant inherits a sliver of budget — less than a TCP handshake — and spends it on a request guaranteed to time out. The same run therefore issues one request or two depending on scheduling, which is exactly what makes the shares one deadline across grants test intermittent on CI.

The fix

• Monotonic clock. performance.now() instead of Date.now(): this is an elapsed-time measurement, so an NTP step or a suspend/resume mid-teardown must not expire or extend the budget, and sub-millisecond resolution means none of it is spent or preserved by rounding.
• A floor. MIN_REVOCATION_REQUEST_BUDGET_MS (5ms) — a budget too small to complete a request is treated as no budget at all. The skipped grant is still reported as failed with the exhausted-budget detail, so nothing is silently dropped; only the pointless call to the authorization server is. The decision is now an order of magnitude away from timer noise, so it comes out the same on every run.

The CLI’s outer per-plan budget in sendPlans (clear-stored-auth-for-relogin.ts) has the same shape and the same defect, so it gets the same treatment.

One consequence, found by the gate

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 is never sent and the revocation reports The value of "delay" is out of range as its failure detail. revokeToken now rounds its own budget to whole milliseconds, which also covers any other caller passing a fractional one. Rounded rather than floored so a caller’s own whole-millisecond timeout survives the trip through the clock and is still the number the timeout message names.

Tests

• does not issue a request with less than the minimum budget left — a first grant that lands the second inside the floor but above zero, i.e. the sliver the old bound would have spent. Verified as a real detector: reverting the bound to <= 0 fails it (1 failed, 56 passed), and it degrades safely — a slower machine only pushes the remainder further below the floor.
• accepts a fractional budget and still sends the request — revokeToken at timeoutMs: 19.996.
• The original flaky test is unchanged, so the shared-budget property is still asserted exactly as before (a per-grant bound still fails it).

npm run local:gate green. Two unrelated 5s-timeout flakes surfaced on earlier runs of the gate (ServerImportJsonModal — that is #2250 — and one ServerSettingsModal case); both pass in isolation and neither is touched by this diff.

No UI change, so no screenshots.

🤖 Generated with Claude Code

https://claude.ai/code/session_017wEXtbs8UEHUMxDEAxbs99

[Copilot]

🟡 Changes recommended

The CLI-specific minimum-budget branch lacks a distinguishing regression test.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes #2252 by making OAuth revocation deadlines monotonic and skipping requests with unusably small remaining budgets.

Changes:

• Uses performance.now() and a shared 5 ms minimum budget.
• Rounds fractional timeout values for AbortSignal.timeout.
• Adds core regression tests for fractional and near-exhausted budgets.

File summaries

File	Description
core/auth/revocation.ts	Implements monotonic deadlines, budget flooring, and timeout rounding.
core/auth/index.ts	Exports the minimum-budget constant.
clients/web/src/test/core/auth/revocation.test.ts	Tests fractional and near-exhausted budgets.
clients/cli/src/clear-stored-auth-for-relogin.ts	Applies the same deadline floor across CLI plans.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Balanced

[cliffhall]

Copilot review round 1 — addressed (c736f1d), mirrored here since inline replies go hidden once the fix is pushed.

Comment	Response
clear-stored-auth-for-relogin.ts — the CLI floor has no regression test; budgetMs: 0 passes under the old bound too	Done. Added issues no request for a positive budget below the minimum: budget MIN_REVOCATION_REQUEST_BUDGET_MS - 1 (positive, under the floor), asserting no fetch and an exhaustion report.

The finding was right, and the first version of the fix was not good enough: asserting "budget was exhausted" also passes with the floor removed, because the plan then enters executeOAuthRevocation with a 4ms budget and core’s identical floor declines it one level down — same outcome, same substring, still no fetch. So the assertion is on this loop’s own message (budget was exhausted before "<serverUrl>"), which only sendPlans emits.

Bound in sendPlans	Result
remainingMs <= MIN_REVOCATION_REQUEST_BUDGET_MS (shipped)	15 passed
remainingMs <= 0 (reverted)	1 failed | 14 passed

npm run local:gate green.

[Copilot]

🟢 Approval recommended

The implementation consistently addresses the deadline race and includes targeted regression coverage for both shared-budget paths.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Both new floor regression tests rely on real scheduler timing and can pass against the old implementation under CI contention.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 2
• Review effort level: Balanced

[cliffhall]

Copilot review round 3 — both comments addressed (927a708), mirrored here since inline replies go hidden once the fix is pushed.

Comment	Response
revocation.test.ts — the web detector still depends on scheduler timing; a preempted worker makes the remainder non-positive and the old bound passes	Done. performance.now() is stubbed with a variable the fetch advances, so the second grant’s remainder is exactly MIN_REVOCATION_REQUEST_BUDGET_MS - 1 and the fetch resolves immediately.
clear-stored-auth-for-relogin.test.ts — same for the CLI detector: preempted longer than 4ms and the reverted <= 0 takes the same branch	Done. The clock is frozen for the call, so the remainder at the check is the injected budget itself.

The point stands and was the important half: neither test could fail wrongly, but either could silently stop testing anything — which on a flake fix is the wrong half of that guarantee to keep.

Mutation table, re-run with the stubs in place:

Reverted bound	Result
core/auth/revocation.ts → remainingMs <= 0	1 failed | 57 passed
clear-stored-auth-for-relogin.ts → remainingMs <= 0	1 failed | 14 passed
neither (shipped)	58 passed / 15 passed

The original flake, shares one deadline across grants, is deliberately left on the real clock: it asserts the shared-budget property rather than the floor, and with the floor in place its remainder can only land further below it.

npm run local:gate green.

[Copilot]

🟢 Approval recommended

The focused implementation addresses the scheduling race and includes deterministic regression coverage for both affected paths.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The implementation addresses the timing race consistently and includes focused regression coverage for both affected paths.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced