fix(oauth): coordinate pooled Anthropic refreshes - #852
Conversation
kaspesi
left a comment
There was a problem hiding this comment.
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.
Summary
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
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.