Repository navigation
fix(proxy): make account admission opt-in - #1265
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR changes per-account admission capacity handling in the Claude proxy. ChangesOptional account admission capacity
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ModelRouter
participant claudeProxyRoutes
participant AccountAdmission
ModelRouter->>claudeProxyRoutes: provide optional maxInflightPerAccount
claudeProxyRoutes->>AccountAdmission: acquire admission
AccountAdmission-->>claudeProxyRoutes: return unlimited lease or capped lease
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/lib/proxy/modelRouter.tsParsing error: Unable to parse the specified 'tsconfig' file. Ensure it's correct and has valid syntax. error TS5012: Cannot read file '/.svelte-kit/tsconfig.json': ENOENT: no such file or directory, open '/.svelte-kit/tsconfig.json'. src/lib/server/routes/claudeProxyRoutes.tsParsing error: Unable to parse the specified 'tsconfig' file. Ensure it's correct and has valid syntax. error TS5012: Cannot read file '/.svelte-kit/tsconfig.json': ENOENT: no such file or directory, open '/.svelte-kit/tsconfig.json'. src/lib/types/proxy.tsParsing error: Unable to parse the specified 'tsconfig' file. Ensure it's correct and has valid syntax. error TS5012: Cannot read file '/.svelte-kit/tsconfig.json': ENOENT: no such file or directory, open '/.svelte-kit/tsconfig.json'.
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 |
✅ 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 makes per-account admission limiting in the Claude proxy explicitly opt-in by removing the implicit default cap of 2 in-flight requests per OAuth account. It updates routing/config behavior so that omitting routing.max-inflight-per-account results in unlimited admission (no queue), while still supporting bounded admission for explicit values 1..20, with corresponding docs and tests.
Changes:
- Remove the implicit default per-account admission cap (2) and treat an omitted cap as unlimited admission.
- Update Claude proxy routing logic to only enqueue/wait when an explicit cap is configured.
- Add/adjust tests and documentation to cover and describe the new “unlimited-by-default when omitted” behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/proxyReliabilityHardening.test.ts | Adds a regression test asserting no admission queue/state is created when no cap is configured. |
| test/proxyConfigHotReload.test.ts | Adds/updates tests to confirm the router reports undefined when the cap is omitted and hot reload preserves the new semantics. |
| src/lib/types/subscription.ts | Updates routing config type docs to describe opt-in admission caps and unlimited-by-default when omitted. |
| src/lib/types/proxy.ts | Widens ModelRouterInterface.getMaxInflightPerAccount return type to include undefined for unlimited admission. |
| src/lib/server/routes/claudeProxyRoutes.ts | Implements unlimited admission when cap is omitted and gates queueing behavior on explicit capacity presence. |
| src/lib/proxy/modelRouter.ts | Removes the default cap and stores maxInflightPerAccount as `number |
| docs/features/claude-proxy-config-reference.md | Updates documentation and defaults table to reflect that max-inflight-per-account is optional and unlimited when omitted. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/lib/proxy/modelRouter.ts`:
- Around line 17-24: Normalize config.maxInflightPerAccount in the ModelRouter
constructor using the same valid-capacity contract as
normalizeAccountAdmissionCapacity, mapping non-integer and out-of-range values
such as 0, 21, and 1.5 to undefined. Store the normalized result in
maxInflightPerAccount so getMaxInflightPerAccount and downstream admission logic
expose and reuse consistent behavior.
In `@src/lib/server/routes/claudeProxyRoutes.ts`:
- Around line 326-330: In the admission setup flow, normalize and validate
capacity before calling getAccountAdmissionState(accountKey), so invalid
capacity throws without creating a state entry. Keep the existing
explicit-capacity error and use the validated normalizedCapacity when
initializing the admission state.
In `@src/lib/types/subscription.ts`:
- Around line 1121-1122: Define invalid-capacity semantics consistently: update
maxInflightPerAccount in src/lib/types/subscription.ts (lines 1121-1122) to
state that out-of-range and non-integer values mean unlimited admission; update
the YAML example comments in docs/features/claude-proxy-config-reference.md
(lines 429-432) and the field reference (lines 553-561) with the same behavior.
In `@test/proxyConfigHotReload.test.ts`:
- Around line 64-72: Move the omitted maxInflightPerAccount admission test from
test/proxyConfigHotReload.test.ts:64-72 and the no-queue admission test from
test/proxyReliabilityHardening.test.ts:1438-1452 into the nearest continuous
suite. Remove both cases from their current Vitest test files and ensure the
relocated coverage runs through the required tsx harness.
🪄 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 Plus
Run ID: 26e5fd33-683b-479c-8b7c-957907fd0f0b
📒 Files selected for processing (7)
docs/features/claude-proxy-config-reference.mdsrc/lib/proxy/modelRouter.tssrc/lib/server/routes/claudeProxyRoutes.tssrc/lib/types/proxy.tssrc/lib/types/subscription.tstest/proxyConfigHotReload.test.tstest/proxyReliabilityHardening.test.ts
7739f15 to
eaa1b16
Compare
🤖 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 |
Review SummaryDecision: APPROVED ✅ This PR successfully implements optional per-account admission limits for the Claude proxy. When Changes Overview:
Key Implementation Details:
Impact Analysis:
Verification:✅ No hardcoded secrets or credentials The implementation follows best practices and safely extends the existing admission control system. |
eaa1b16 to
365736d
Compare
🤖 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 by Yama
Summary
This PR removes the implicit per-account admission cap of two and makes account concurrency limits opt-in. The changes are well-structured with proper validation, documentation updates, and tests. However, there are two issues that need attention:
- MAJOR: The behavioral change where
getMaxInflightPerAccount()now returnsundefinedinstead of a default value of 2 may break downstream consumers expecting numeric values. - MINOR: Test imports in bugfixes.ts need verification.
Findings
- 🔒 MAJOR: ModelRouter.getMaxInflightPerAccount returns undefined when no explicit cap set, breaking callers expecting numeric default
- 💡 MINOR: Test imports for ModelRouter not visible in bugfixes.ts file structure
Impact on existing code
- Changes affect any code that calls
ModelRouter.getMaxInflightPerAccount()and expects a numeric value - Documentation examples in docs/features/proxy-routes.md may need updating
- Tests in test/continuous-test-suite-bugfixes.ts need import verification
Decision: CHANGES_REQUESTED
Due to the MAJOR finding about the API contract change, this PR requires changes before merge.
|
🔒 MAJOR: ModelRouter.getMaxInflightPerAccount returns undefined when no explicit cap set, breaking callers expecting numeric default Changed behavior: previously models without explicit maxInflightPerAccount returned 2 (implicit default), now return undefined. This breaks code in docs/features/proxy-routes.md example that checks getMaxInflightPerAccount() === 2, and any consumer relying on numeric comparison. Either maintain backward compatibility by returning 2 as implicit default, or update all documentation/examples/tests to handle undefined case explicitly (docs/features/proxy-routes.md line ~150 shows expect(...).toBe(2)). |
|
💡 MINOR: Test imports for ModelRouter not visible in bugfixes.ts file structure Cannot verify that test/continuous-test-suite-bugfixes.ts properly imports claudeProxyTestHooks and ModelRouter types at the top of the file. Need to confirm imports are present before confirming tests will run. Add missing imports at top of file:
|
Yama Review SummaryDecision: CHANGES_REQUESTEDThis PR removes the implicit per-account admission cap of two and makes account concurrency limits opt-in. The changes are well-structured with proper validation, documentation updates, and tests. However, there are two issues that need attention before merge: Findings (2 total)
Impact on existing code
Review ScopeReviewed 7 changed files focusing on:
Final Verdict: CHANGES_REQUESTEDDue to the MAJOR finding about the API contract change where |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review by Yama
Summary
This PR removes the implicit per-account admission cap of two and makes account concurrency limits opt-in. The changes are well-structured with proper validation, documentation updates, and tests. However, there are two issues that need attention:
- MAJOR: The behavioral change where
getMaxInflightPerAccount()now returnsundefinedinstead of a default value of 2 may break downstream consumers expecting numeric values. - MINOR: Test imports in bugfixes.ts need verification.
Findings
- 🔒 MAJOR: ModelRouter.getMaxInflightPerAccount returns undefined when no explicit cap set, breaking callers expecting numeric default
- 💡 MINOR: Test imports for ModelRouter not visible in bugfixes.ts file structure
Impact on existing code
- Changes affect any code that calls
ModelRouter.getMaxInflightPerAccount()and expects a numeric value - Documentation examples in docs/features/proxy-routes.md may need updating
- Tests in test/continuous-test-suite-bugfixes.ts need import verification
Decision: CHANGES_REQUESTED
Due to the MAJOR finding about the API contract change, this PR requires changes before merge.
🛡️ Yama Review Verdict: CHANGES_REQUESTEDSeverity counts — 🔒 CRITICAL: 0 · 🤖 Yama Review Summary
No verified findings were accepted by the review gate. The verdict reported findings but carried no detail — treat it as unverified and re-run the review or inspect manually. |
365736d to
92f48d9
Compare
Re: both findings — neither reproducesRebased onto MAJOR: "breaks docs/features/proxy-routes.md which checks
|
🤖 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 |
|
Verified against current head |
92f48d9 to
e596137
Compare
🤖 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 |
e596137 to
c4bcb75
Compare
🤖 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 |
|
🎉 This PR is included in version 10.8.13 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Moves the existing quota-fill comparator into a new pure src/lib/proxy/accountRanking.ts as compareExpiryFirst, with orderAccountsByQuotaWithMetrics becoming a thin wrapper around a new rankAccounts(). Also unifies unlimited- and capacity-limited account admission onto the same accountAdmissionStates map so getAccountInflight() works for every account, which PR2's spill-inflight policy needs. Account order is unchanged: the comparator moves rung for rung, the existing route tests pass unmodified, and a new direct-vs-wrapper parity test checks both entry points on the same fixture. A local replay of recorded routing decisions through a port of the comparator also matched every one (see PR description). Uncapped leases now count toward in-flight, so the admission test from #1265 asserts that count instead of an empty state; it still requires that uncapped requests are admitted immediately and never queue.
Moves the existing quota-fill comparator into a new pure src/lib/proxy/accountRanking.ts as compareExpiryFirst, with orderAccountsByQuotaWithMetrics becoming a thin wrapper around a new rankAccounts(). Also unifies unlimited- and capacity-limited account admission onto the same accountAdmissionStates map so getAccountInflight() works for every account, which PR2's spill-inflight policy needs. Account order is unchanged: the comparator moves rung for rung, the existing route tests pass unmodified, and a new direct-vs-wrapper parity test checks both entry points on the same fixture. A local replay of recorded routing decisions through a port of the comparator also matched every one (see PR description). Uncapped leases now count toward in-flight, so the admission test from #1265 asserts that count instead of an empty state; it still requires that uncapped requests are admitted immediately and never queue.
Summary
routing.max-inflight-per-accountis omittedVerification
pnpm exec vitest run test/proxyReliabilityHardening.test.ts test/proxyConfigHotReload.test.tspnpm exec tsc --noEmit --strict --project tsconfig.cli.jsonpnpm run build:clipnpm run test:bugfixespnpm exec prettier --check ...git diff --checkThe pre-commit hook could not complete because the repository is missing the unrelated optional package
landing/@sveltejs/adapter-vercel; the listed targeted and build checks passed.Summary by CodeRabbit