Skip to content

fix(oauth): coordinate pooled Anthropic refreshes - #852

Merged
mcowger merged 5 commits into
mcowger:mainfrom
kaspesi:fix/pooled-anthropic-oauth-refresh
Sep 8, 2026
Merged

fix(oauth): coordinate pooled Anthropic refreshes#852
mcowger merged 5 commits into
mcowger:mainfrom
kaspesi:fix/pooled-anthropic-oauth-refresh

Conversation

@kaspesi

@kaspesi kaspesi commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • treat valid persisted OAuth credentials as recently refreshed at startup, avoiding a process-start rotation burst
  • serialize Anthropic refresh-token exchanges across pooled accounts with a 30-second minimum interval
  • exponentially back off failed refreshes and keep using an unexpired access token after proactive refresh failure
  • preserve request cancellation while a refresh is queued or in flight
  • serialize Claude quota probes with a 15-second interval and timestamp their actual execution time

Why

A four-account Claude pool reproduced an all-seat outage after synchronized quota checkers started. Every account had empty in-memory refresh cadence state after restart, so all four performed proactive rotating-token exchanges together. The resulting token-endpoint failures were retried immediately; inference then returned revoked-token 401s and the usage endpoint returned 429s.

This keeps the existing hourly proactive rotation behavior while coordinating it safely for pooled accounts. Related OAuth endpoint rate-limit behavior and immediate-retry amplification has also been reported in earendil-works/pi#4767.

Validation

  • focused OAuth/quota tests: 52 passed
  • typecheck: passed
  • Biome format and lint: passed
  • production validation on the pinned downstream build: restart did not alter any credential timestamps; four quota calls completed 15 seconds apart; live Claude request returned HTTP 200
  • full local suite: 822 passed, 1 unrelated failure

Known baseline failure

The pre-commit full suite fails at src/routes/mcp/tests/mcp-routes.test.ts:432 because Bun 1.3.14 omits the asserted Connection: keep-alive response header. The same test fails in a pristine checkout of upstream main. The commit therefore used an explicitly approved one-time --no-verify bypass.

The pre-push repomap hook also required an explicitly approved one-time --no-verify bypass: Universal Ctags 6.2.1 rewrites the existing repomap with widespread unrelated method deletions. No generated repomap changes are included.

@kaspesi kaspesi left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review pass 1 — skeptical maintainer. Verified locally: focused OAuth + quota suites pass (8/8 and 44/44 across sqlite+postgres) and bun run typecheck is clean, matching the PR claims.

The reported failure mode checks out, and the two structural fixes are complementary rather than redundant — worth stating explicitly because it's the strongest justification here: seeding lastRefreshAt at load only moves the synchronized burst to T+cadence, since every account shares the same loadedAt and therefore crosses the 1h threshold at the same instant. The per-provider serialization is what actually prevents that deferred burst (same argument for phase-aligned quota ticks). checkedAt being stamped at real execution start (quota-scheduler.ts:118-121), the cancellation-vs-endpoint-failure distinction (oauth-auth-manager.ts:278,339), and the resolve-only tail promises (can't reject/deadlock on await previous) are all sound. Tests assert concrete intervals/call-counts, not shapes.

That said, several substantive concerns:

1. (medium) Reactive expired-token refresh on the live inference path is now serialized across accounts with up to ~30s spacing, and it's uncancellable. The inference call sites oauth-native-request.ts:691 and :742 pass no signal and no refreshIfOlderThanMs, so refresh only triggers on current.expires <= now — and it then flows through runProviderRefresh's per-provider tail + waitForDelay (oauth-auth-manager.ts:315-319, 373-382). Under the exact token-endpoint incident this PR targets (backoff active, tokens expiring), an expired-token inference for account B can block behind account A's spaced rotation for up to ~30s × queue-depth, and since no signal is threaded the wait isn't abortable by request cancellation. Acceptance: exempt reactive/expired refreshes from inter-account spacing (space only proactive rotations), OR thread the request AbortSignal into the inference getApiKey calls, OR document the bounded worst-case added latency on the live path.

2. (low-med) Per-provider serialization is applied to every provider, not just Anthropic — beyond the PR's stated scope. runProviderRefresh (oauth-auth-manager.ts:359-391) has no zero-interval early return, so openai-codex / copilot pooled-account refreshes that were previously concurrent are now serialized per provider even though their PROVIDER_REFRESH_MIN_INTERVAL_MS is 0. Contrast the quota analogue, which correctly early-returns: if (minIntervalMs === 0) return operation(); (quota-scheduler.ts:159). Acceptance: mirror that early-return in runProviderRefresh so unlisted providers keep concurrency, or state that global per-provider serialization is intended.

3. (low-med) reload() resets the rotation clock for all valid persisted creds, and reload is reachable from the checker's catch/retry path. loadFromDatabaseAsync sets lastRefreshAt = loadedAt for every valid credential (oauth-auth-manager.ts:97-100) and reload() re-runs it (:440). claude-code-checker.ts:189-190 calls authManager.reload() on any resolve failure. So an unrelated checker failure resets lastRefreshAt=now for a genuinely stale account, deferring its due proactive rotation by up to another full cadence; repeated failures can defer indefinitely, aging the rotating refresh token. Acceptance: on load, only seed lastRefreshAt when absent (don't overwrite an earlier timestamp), so reload doesn't reset the cadence.

4. (low) Expired-token requests hard-fail for the entire backoff window (≥60s, escalating to 15m). getApiKey throws when backed off and current.expires <= now (oauth-auth-manager.ts:241-246). The anti-amplification intent is right, but a single transient blip now takes an account fully offline for ≥60s even after the endpoint recovers, and consecutive blips escalate to a 15-minute per-account outage. Acceptance: justify the 60s/15m constants against the observed endpoint limit, and/or use a shorter ceiling when the token is already expired (true outage) vs still-valid (best-effort rotation). Same ask for the 30s/15s spacing values — currently empirical magic numbers; a one-line rationale (or making them configurable) would help operability.

5. (low) Coordination is in-memory / single-process only. All new state (providerRefreshTails, lastProviderRefreshAttemptAt, refreshBackoffs, lastRefreshAt, checkerRunTails, lastCheckerRunAt) is per-process. If Plexus is ever run multi-worker/clustered, N processes reintroduce the synchronized burst. Acceptance: note this limitation near the constants / in the PR body.

6. (low) stop() doesn't cancel already-queued spaced checks. runCheckerWithSpacing uses a bare setTimeout with no abort (quota-scheduler.ts:177); stop() clears the maps (:517-518) but a queued Claude check still fires ~15s later against its captured config/ctx and then persists/applies cooldowns. Minor shutdown-ordering wrinkle. Acceptance: acceptable if intentional; otherwise guard the queued op on a stopped flag or thread an abort.

None of these are correctness bugs in the happy path — they're latency/scope/operability trade-offs worth an explicit decision or a doc line. #1 and #2 are the two I'd want addressed or consciously waved off before merge.

@kaspesi
kaspesi marked this pull request as ready for review September 8, 2026 17:00
@mcowger
mcowger merged commit a0a6208 into mcowger:main Sep 8, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants