Repository navigation
fix(proxy): harden account routing and process reliability - #1163
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Documentation Validation Results🚀 Documentation validation passed!
📦 Build artifact uploaded successfully. Ready for deployment preview. Commit: |
There was a problem hiding this comment.
Pull request overview
This PR hardens the Claude-compatible proxy’s reliability and safety by persisting rate-limit/auth cooldown state across restarts, tightening account eligibility boundaries via an explicit allowlist, and improving process supervision + streaming terminal outcome telemetry to better reflect real client outcomes.
Changes:
- Persist per-account cooldown state to disk and restore it at startup; never retry accounts with active cooldowns (including when all accounts are cooling).
- Add
routing.account-allowlistas an eligibility boundary across TokenStore / legacy-file / env credentials; prevent hidden fallbacks when any TokenStore Anthropic entry exists. - Improve operational robustness: serialize rotating OAuth refreshes, refine terminal vs transient refresh failures, harden update-state atomic writes, and track stream terminal outcomes (upstream error vs client cancellation vs completion).
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/proxyReliabilityHardening.test.ts | Adds regression tests covering cooldown persistence, unified rate-limit handling, refresh classification/serialization, allowlist behavior, stream terminal outcomes, and launchd invariants. |
| test/continuous-test-suite-bugfixes.ts | Updates continuous test assertions to match new “all accounts cooling / earliest retry” behavior and non-rehammering invariant. |
| src/lib/types/subscription.ts | Extends proxy routing config type with accountAllowlist?: string[]. |
| src/lib/types/proxy.ts | Adds allowlist/cooldown/stream-outcome related types and updates cooldown/quota documentation. |
| src/lib/types/cli.ts | Persists the enforced account allowlist into proxy runtime state. |
| src/lib/types/auth.ts | Updates disable-reason example to reflect new refresh_invalid semantics. |
| src/lib/server/routes/claudeProxyRoutes.ts | Core logic changes: persisted cooldowns, allowlist enforcement, unified-status handling, refresh classification, non-rehammer routing, and stream terminal outcome telemetry/status reporting. |
| src/lib/proxy/updateState.ts | Hardens atomic update-state writes with per-process unique temp files and cleanup. |
| src/lib/proxy/tokenRefresh.ts | Serializes refresh by refresh-token value, adds transient vs permanent refresh failure classification, and uses shared atomic snapshot writer. |
| src/lib/proxy/streamOutcome.ts | Introduces a first-writer-wins tracker and merge logic for SSE terminal error promotion. |
| src/lib/proxy/sseInterceptor.ts | Captures terminal SSE event: error message into telemetry for downstream terminal-outcome classification. |
| src/lib/proxy/snapshotPersistence.ts | Tightens state-dir permissions and supports serialized atomic snapshot writes. |
| src/lib/proxy/requestLogger.ts | Records terminal stream outcomes distinctly (e.g., terminalStatus/outcome) for mid-stream failures. |
| src/lib/proxy/proxyPaths.ts | Adds cooldownFile to proxy paths for both dev and non-dev state directories. |
| src/lib/proxy/proxyConfig.ts | Adds validation + parsing support for routing.account-allowlist / accountAllowlist. |
| src/lib/proxy/accountSelection.ts | New module for key normalization/equality, allowlist creation, and fallback credential gating rules. |
| src/lib/proxy/accountQuota.ts | Parses authoritative top-level unified status; hardens cache-load concurrency and moves persistence to atomic snapshot writer. |
| src/lib/proxy/accountCooldown.ts | New module implementing restart-safe cooldown persistence with validation and atomic writes. |
| src/lib/auth/tokenStore.ts | Preserves manual disable metadata across automatic token rotations; adds getDisabledReason(). |
| src/cli/commands/proxy.ts | Ensures launchd-only restart supervision, makes auto-update opt-in, enforces allowlist at boot, exposes allowlist in status, and improves shutdown drain behavior. |
| src/cli/commands/auth.ts | Makes explicit login re-enable previously disabled accounts (markEnabled). |
| docs/features/claude-proxy.md | Updates user-facing proxy behavior docs for cooldown persistence, allowlist semantics, refresh behavior, and terminal stream outcomes. |
| docs/features/claude-proxy-config-reference.md | Documents new account-allowlist option and updated guard/auto-update behavior and file inventory. |
| docs/analysis/claude-proxy-reliability-hardening.md | Adds incident/root-cause analysis and rollout verification plan for the hardening changeset. |
| .changeset/proxy-reliability-hardening.md | Publishes a patch changeset describing the reliability/security hardening. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the proxy reliability hardening PR (25 changed files, ~2.6K additions). The change addresses real production incidents and the core invariants are well documented. However, I found 5 MAJOR and 3 MINOR new issues that need to be addressed before merge.
Blocking criterion
Per the review charter, 3 or more MAJOR issues in the same PR is a BLOCK. This PR crosses that threshold.
MAJOR issues (blocking)
-
src/lib/proxy/accountCooldown.ts:saveAccountCooldown— read-modify-write race can violate the extend-only cooldown invariant. Two concurrent callers can load the same stale snapshot and the shorter cooldown wins. Serialize the load-check-write path with aMutex. -
src/lib/proxy/accountCooldown.ts:clearAccountCooldown— same race as above; a concurrent save and clear can clobber each other. -
src/lib/proxy/accountQuota.ts:saveAccountQuota— debounced flush snapshotsmemoryCacheat scheduling time, but concurrent first-callers can load{}and wipe each other's account quotas. The load-update-dirty path needs serialization. -
src/lib/proxy/tokenRefresh.ts:refreshToken— the in-flight cache is keyed only by the old refresh token. A caller that loads the new rotating token from disk just before the winner persists it will not find an in-flight entry and will trigger a second refresh, invalidating the winner's new token. Cache by account label or also by the new token forSUCCESS_CACHE_MS. -
src/cli/commands/proxy.ts:registerProxyShutdownHandlers—process.on("SIGTERM", ...)andSIGINThandlers are async and unprotected; a thrown rejection inshutdown()can hang the process or produce an unintended exit code. Wrap shutdown in try/catch and always exit deterministically.
MINOR issues (non-blocking, please address)
-
src/lib/server/routes/claudeProxyRoutes.ts— the post-load "credentials changed, re-enabling" log/runtime-clear contradicts the stated invariant that token changes no longer alter eligibility. Clarify or remove the misleading path. -
src/lib/server/routes/claudeProxyRoutes.ts— when all accounts are in auth cooldown,loopState.sawTransientFailureis set and the final response falls through to a generic 502 instead of the more accurate auth/reauth message. -
src/lib/auth/tokenStore.ts:saveTokens— disable metadata preservation only works when the existing record is already disabled; a save that races ahead of a disable can lose the operator's intent. Consider allowing callers to pass disable state atomically.
Next steps
Please address the five MAJOR items (especially the cooldown/quota races and the shutdown handler) and re-request review. The test coverage for the new behavior is good, but the race windows above are not exercised by the current hermetic tests.
9850505 to
6b4edd7
Compare
📝 WalkthroughWalkthroughThe Claude proxy gains account allowlists, coordinated OAuth refresh, persistent cooldown and quota state, unified rate-limit handling, terminal stream outcome tracking, and revised launchd/foreground guard lifecycle behavior. Documentation, public types, startup wiring, persistence, routes, and reliability tests are updated accordingly. ChangesClaude proxy reliability
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the full diff for PR #1163 (23 changed files, +2506/-520).
Existing review comments
All 10 prior inline threads from Tara-ag and copilot-pull-request-reviewer have been addressed by the author in commit 6b4edd7b:
- cooldown/quota read-modify-write races are now serialized under
AsyncMutex; - rotating OAuth refresh-token deduplication now caches successes under both the old and newly-rotated token;
- shutdown handlers drain the server deterministically and surface close errors;
- TokenStore disable metadata is preserved across automatic saves and protected by the shared mutex;
- all-auth-cooldown pools now return a specific
503 token_refresh_unavailableresponse.
New findings this run
- 0 CRITICAL, 0 MAJOR, 0 MINOR new issues.
- No hardcoded secrets, injection vectors, SSRF, unsafe eval, or PII exposure in the changed source.
- No violations of CLAUDE.md Critical Rules enforced outside CI (dynamic-import registry rule, Gemini tools/schema rule, CLI/SDK separation,
formatProviderErrorreturn rule, public SDK backward compatibility). - New types are defined only in the canonical
src/lib/types/location and imported via the barrel. - The new
account-allowlistboundary is applied consistently across TokenStore, legacy-file, and environment credential discovery, and hidden fallbacks are correctly suppressed while any Anthropic TokenStore entry exists. - Cooldown/quota persistence, rotating refresh serialization, stream terminal-outcome tracking, and deterministic shutdown all appear correct.
Tests
The PR adds test/proxyReliabilityHardening.test.ts covering unified rate-limit classification, cooldown/quota persistence races, refresh failure classification, rotating-token serialization, account restriction, stream terminal outcomes, and launchd lifecycle invariants. This aligns with the existing vitest-based test files in the repo.
Verdict
Approve — no new blocking criteria are met.
|
🎉 This PR is included in version 9.86.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/proxy/tokenRefresh.ts (1)
29-86: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTreat only documented credential failures as terminal
isRefreshCredentialRejectioncurrently classifies 403/404 as permanent. The fallback token endpoint can return a 403 challenge unrelated to refresh-token validity, so a transient infra block could mark a valid account asrefresh_invalidand disable it until re-login. Restrict this to the documented invalid-grant responses, or inspect the response body before settingterminalFailure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/proxy/tokenRefresh.ts` around lines 29 - 86, Update isRefreshCredentialRejection and the terminalFailure assignment in performTokenRefresh so only documented invalid-grant credential responses are treated as permanent failures; do not classify generic 403/404 fallback endpoint responses as terminal. Preserve fallback retry behavior and return the failure normally unless the status/body confirms an invalid refresh credential.
🧹 Nitpick comments (3)
src/lib/proxy/accountQuota.ts (1)
199-222: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winLoaded quota entries aren't validated against the
AccountQuotashape.
memoryCache = JSON.parse(raw) as Record<string, AccountQuota>;blindly trusts the on-disk JSON. The siblingaccountCooldown.tsmodule validates each loaded entry (isPersistedCooldown) and drops malformed ones; this module has no equivalent guard, so a stale/corruptedaccount-quotas.json(e.g. from an older schema or partial external edit) would load fields asundefined/wrong-typed straight into runtime state used for quota-based routing decisions, rather than falling back safely.♻️ Proposed validation guard
+function isValidAccountQuota(value: unknown): value is AccountQuota { + if (!value || typeof value !== "object") { + return false; + } + const c = value as Partial<AccountQuota>; + return ( + typeof c.sessionUsed === "number" && + typeof c.weeklyUsed === "number" && + typeof c.sessionStatus === "string" && + typeof c.weeklyStatus === "string" && + typeof c.lastUpdated === "number" + ); +} + async function ensureAccountQuotasLoaded(): Promise<void> { if (!cacheLoaded) { if (!cacheLoadPromise) { cacheLoadPromise = (async () => { try { const raw = await fs.readFile(getQuotaFilePath(), "utf-8"); - memoryCache = JSON.parse(raw) as Record<string, AccountQuota>; + const parsed = JSON.parse(raw) as Record<string, unknown>; + memoryCache = Object.fromEntries( + Object.entries(parsed).filter(([, v]) => isValidAccountQuota(v)), + ) as Record<string, AccountQuota>; } catch { memoryCache = {}; } cacheLoaded = true; })().finally(() => { cacheLoadPromise = null; }); } await cacheLoadPromise; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/proxy/accountQuota.ts` around lines 199 - 222, Update ensureAccountQuotasLoaded to validate parsed entries before assigning memoryCache, using an AccountQuota-shaped type guard equivalent to accountCooldown.ts’s isPersistedCooldown pattern. Retain only entries that pass validation and discard malformed or stale records, falling back to an empty cache when the parsed data is invalid or not an object.src/lib/server/routes/claudeProxyRoutes.ts (1)
2897-2985: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated stream-terminal-outcome tracking scaffolding vs. the auth-retry path.
The
ReadableStreamwrapper here (seed first chunk,pull/cancelhandlers drivingstreamOutcomeTracker.complete()/fail()/cancel(), plus the mid-stream error-event SSE emission) is duplicated almost verbatim inhandleAnthropicSuccessfulRetryResponseat lines 3591-3638, differing only in variable names (mainStreamClosedvsretryStreamClosed) and the initialstart()seeding. Any future fix to this stream-tracking logic (e.g., an edge case in outcome settlement) needs to be applied in both places.Consider extracting a shared helper (parameterized by an optional "first chunk to seed" and the reader) that both the initial success path and the auth-retry success path can call, similar to how
trackUpstreamReadableStreamalready centralizes the passthrough-mode variant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/server/routes/claudeProxyRoutes.ts` around lines 2897 - 2985, Extract the duplicated stream-terminal tracking and SSE error-handling logic from the current success path and handleAnthropicSuccessfulRetryResponse into a shared helper, parameterized by the reader and optional initial chunk. Have both callers use it while preserving first-chunk seeding, complete/fail/cancel tracking, stream closure, and mid-stream error-event behavior.src/cli/commands/proxy.ts (1)
136-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the resolved allowlist instead of recomputing it.
resolveStatusPrimaryAccountrebuilds the allowlist viacreateAccountAllowlist(proxyConfig?.routing?.accountAllowlist)(Lines 140-142) rather than reusingparams.accountAllowlist, which was already resolved (with validation/warnings) byresolveBootAccountAllowlistand is available in the/statushandler's closure at the call site (Line 1320-1322). Two independent constructions of the same security-relevant allowlist risk silently diverging if one path changes but not the other.♻️ Proposed fix
async function resolveStatusPrimaryAccount( proxyConfig: LoadedProxyConfig | null, + accountAllowlist?: AccountAllowlist, ): Promise<ProxyStatusPrimaryAccount> { const configured = proxyConfig?.routing?.primaryAccount?.trim() || null; - const accountAllowlist = createAccountAllowlist( - proxyConfig?.routing?.accountAllowlist, - ); let enabledAnthropicKeys: string[] = [];and update the call site to pass
params.accountAllowlist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/commands/proxy.ts` around lines 136 - 188, Update resolveStatusPrimaryAccount to accept the already-resolved accountAllowlist as an argument and remove its local createAccountAllowlist(proxyConfig?.routing?.accountAllowlist) construction. At the /status handler call site, pass params.accountAllowlist from the existing closure so account filtering reuses the validated allowlist.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/commands/proxy.ts`:
- Around line 1808-1812: Update StateFileManager.save() to create or write
proxy-state.json with mode 0o600, preserving restrictive permissions even when
the file already exists. In docs/features/claude-proxy-config-reference.md lines
627-633, update the config reference row from “user default” to reflect the
hardened file mode.
In `@src/lib/proxy/sseInterceptor.ts`:
- Around line 415-430: Cap the trimmed derived message assigned in the
error-event branch of the SSE interceptor to MAX_EVENT_DATA_BYTES, matching the
existing fallback truncation. Preserve the nested error.message and top-level
message selection and fallback behavior, while ensuring acc.streamErrorMessage
is bounded before downstream logging and tracing.
---
Outside diff comments:
In `@src/lib/proxy/tokenRefresh.ts`:
- Around line 29-86: Update isRefreshCredentialRejection and the terminalFailure
assignment in performTokenRefresh so only documented invalid-grant credential
responses are treated as permanent failures; do not classify generic 403/404
fallback endpoint responses as terminal. Preserve fallback retry behavior and
return the failure normally unless the status/body confirms an invalid refresh
credential.
---
Nitpick comments:
In `@src/cli/commands/proxy.ts`:
- Around line 136-188: Update resolveStatusPrimaryAccount to accept the
already-resolved accountAllowlist as an argument and remove its local
createAccountAllowlist(proxyConfig?.routing?.accountAllowlist) construction. At
the /status handler call site, pass params.accountAllowlist from the existing
closure so account filtering reuses the validated allowlist.
In `@src/lib/proxy/accountQuota.ts`:
- Around line 199-222: Update ensureAccountQuotasLoaded to validate parsed
entries before assigning memoryCache, using an AccountQuota-shaped type guard
equivalent to accountCooldown.ts’s isPersistedCooldown pattern. Retain only
entries that pass validation and discard malformed or stale records, falling
back to an empty cache when the parsed data is invalid or not an object.
In `@src/lib/server/routes/claudeProxyRoutes.ts`:
- Around line 2897-2985: Extract the duplicated stream-terminal tracking and SSE
error-handling logic from the current success path and
handleAnthropicSuccessfulRetryResponse into a shared helper, parameterized by
the reader and optional initial chunk. Have both callers use it while preserving
first-chunk seeding, complete/fail/cancel tracking, stream closure, and
mid-stream error-event behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c1c34ce-2638-4c3b-b71e-cafbc15ece74
📒 Files selected for processing (23)
docs/features/claude-proxy-config-reference.mddocs/features/claude-proxy.mdsrc/cli/commands/auth.tssrc/cli/commands/proxy.tssrc/lib/auth/tokenStore.tssrc/lib/proxy/accountCooldown.tssrc/lib/proxy/accountQuota.tssrc/lib/proxy/accountSelection.tssrc/lib/proxy/proxyConfig.tssrc/lib/proxy/proxyPaths.tssrc/lib/proxy/requestLogger.tssrc/lib/proxy/snapshotPersistence.tssrc/lib/proxy/sseInterceptor.tssrc/lib/proxy/streamOutcome.tssrc/lib/proxy/tokenRefresh.tssrc/lib/proxy/updateState.tssrc/lib/server/routes/claudeProxyRoutes.tssrc/lib/types/auth.tssrc/lib/types/cli.tssrc/lib/types/proxy.tssrc/lib/types/subscription.tstest/continuous-test-suite-bugfixes.tstest/proxyReliabilityHardening.test.ts
Summary
routing.account-allowlistas a fail-closed boundary across TokenStore, legacy-file, and environment credentials.Account isolation
primary-accountremains an ordering preference. The new allowlist is the eligibility boundary and is applied before token loading or refresh. Empty means deny all; absent preserves existing unrestricted behavior. Legacy and environment fallbacks require explicitlegacy-defaultorenventries.Automatic token rotation no longer clears an operator-disabled account. Only explicit login re-enables current disable metadata; legacy
refresh_failedrecords from older releases receive one migration recheck.Verification
pnpm run typecheck: pass.pnpm exec vitest run test/proxyReliabilityHardening.test.ts: 23/23 pass.pnpm test:bugfixes: 89/89 pass.pnpm run build:cli: pass.Rollout safety
Development and tests did not start, stop, signal, reconfigure, or install over the live proxy. The existing process-level proxy suite was intentionally not run because it manages global proxy state and processes. Release rollout should use an isolated state directory first, then a controlled launchd replacement with health and restart-count verification.