Repository navigation
fix(proxy): prioritize expiring session windows in account routing - #1167
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughClaude proxy quota ordering now supports configurable session soft limits and reset tolerances, freshens passed resets, prioritizes session saturation and reset timing, and adds deterministic coverage for the updated ordering behavior. ChangesSession-first quota routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ClaudeProxy as Claude proxy
participant Metrics as accountSortMetrics
participant Comparator as orderAccountsByQuota
ClaudeProxy->>Metrics: evaluate account quota and reset state
Metrics-->>Comparator: return usability, saturation, and reset buckets
Comparator-->>ClaudeProxy: return ordered accounts
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
ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages. 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 |
![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/server/routes/claudeProxyRoutes.ts`:
- Around line 700-704: Update the saturated-account comparison in
src/lib/server/routes/claudeProxyRoutes.ts:700-704 to remove the exact
sessionReset tie-break and rely on sessionResetBucket, preserving the configured
reset tolerance and weekly ordering. Add coverage in
test/continuous-test-suite-proxy.ts:1842-1848 with two saturated accounts in the
same reset bucket whose weekly resets verify the expected order.
- Around line 590-606: Update getSessionSoftLimit and getSessionResetToleranceMs
to parse the complete environment value with Number() so partially numeric
strings fall back to their defaults; additionally require the reset tolerance to
be an integer before accepting it.
In `@test/continuous-test-suite-proxy.ts`:
- Around line 1737-1745: Update testSessionFirstOrdering to save both
quota-related environment variables, set deterministic test values before
running its cases, and restore both original values in an outer finally block.
Ensure restoration preserves whether each variable was originally unset,
including the cleanup path covering the later case that deletes the soft limit.
- Around line 1842-1848: Add a test case near the existing “Both saturated”
scenario in the ordering tests that uses saturated accounts with session reset
times in the same tolerance bucket but opposing weekly reset times. Assert
ordering follows the earlier weekly reset as the tie-break, confirming tolerance
bucketing still enables weekly reset comparison.
🪄 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: 94a78bcc-83fe-4837-b3d5-3059b1d34031
📒 Files selected for processing (2)
src/lib/server/routes/claudeProxyRoutes.tstest/continuous-test-suite-proxy.ts
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Files reviewed: 2
New issues raised: 1 MINOR
Blocking issues: 0
Notes
- The session-first ordering, soft-limit demotion, and reset-freshening logic in
claudeProxyRoutes.tsis sound and well-documented. - Added one minor inline comment: the env parsers (
getSessionSoftLimit/getSessionResetToleranceMs) are invoked insideaccountSortMetrics, which is called repeatedly during sorting. Hoisting the parsed values once perorderAccountsByQuotacall avoids both an O(N log N) env re-parse and the risk of a mid-sort env change breaking comparator transitivity. - Two of my observations overlap with feedback already posted by CodeRabbit (saturated-account exact
sessionResetcomparison and test env-variable isolation). I did not re-raise those as new blocking concerns.
No blocking criteria are met; approving.
- Order accounts by soonest 5h session reset; weekly breaks ties. - Demote sessions above a 97% soft limit before they hit 429s. - Treat windows whose reset has passed as fresh at read time.
|
Thanks @coderabbitai and @Tara-ag — all five findings verified and fixed, amended into the single commit (
Re-verified: tsc/lint gates clean, 6/6 runtime checks for the fixes (garbage env → defaults, same-bucket saturated weekly tie-break, plus regressions), 36/36 |
eb52f62 to
9c0e71d
Compare
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cd . 2>/dev/null
rg -n "sessionResetBucket|SESSION_SOFT_LIMIT|SESSION_RESET_TOLERANCE|Number\(" src/lib/server/routes/claudeProxyRoutes.ts | head -60
echo "---"
rg -n "orderAccountsByQuota|accountSortMetrics" src/lib/server/routes/claudeProxyRoutes.ts | head -20Length of output: 1331 Verification confirms the changes match what you described — Thanks for the thorough writeup and re-verification, ᕮ(⌐■_■)ᕭ |
🤖 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
Files reviewed: 2 (src/lib/server/routes/claudeProxyRoutes.ts, test/continuous-test-suite-proxy.ts)
New issues raised: 0
Decision: Approve
Assessment
This PR refines the proxy's quota-aware account ordering to prioritize the expiring 5h session window over the 7d weekly window, adds a soft-limit demotion for saturated sessions, and implements reset freshening at read time. The changes are localized to the proxy routing comparator and its test suite.
Key points verified:
- Comparator transitivity: Session resets are compared via fixed-grid buckets (
Math.floor(sessionReset / tolerance)), which is transitive by construction. The thresholds are read once per sort, so mid-sort env changes cannot break ordering. - Env parsing:
Number()is used instead ofparseFloat/parseInt, correctly rejecting partially numeric strings. The tolerance is required to be an integer. - Reset freshening: Windows whose reset epoch has passed are treated as fresh (
sessionUsed = 0, status"allowed"), preventing stale snapshots from saturating an account after Anthropic has renewed its window. - Soft-limit behavior: Saturated/throttled accounts are demoted but remain usable as fallbacks, matching the documented intent.
- Test coverage: The new
testSessionFirstOrderingsuite covers session-first vs weekly, same-bucket weekly tie-break, soft-limit boundary, throttled demotion, reset freshening, saturated recovery, and env-configurable limit. It also saves/restores both quota-related env variables.
Existing review comments
The outstanding review comments from earlier passes appear addressed in the current revision:
- Env parsers are hoisted outside the comparator in
orderAccountsByQuota. - Saturated accounts no longer use an exact
sessionResettie-break; they fall through to the same bucketed session → weekly → weekly-used chain. - The test wrapper saves and restores both
NEUROLINK_PROXY_SESSION_SOFT_LIMITandNEUROLINK_PROXY_SESSION_RESET_TOLERANCE_MS.
No new blocking or major issues were found. Approving.
|
🎉 This PR is included in version 9.87.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This branch was successfully deployed
3 participants
Description
Refines the quota-aware account ordering introduced in #1143/#1144. The previous comparator ranked accounts by soonest weekly (7d) reset, which ignores the window that actually expires first: the rolling 5h session. With two accounts whose sessions reset at different times, the session window closing soonest is the capacity that is about to be lost — it should be consumed first, regardless of the weekly schedule. The old ordering also considered session state only at hard exhaustion ("rejected"), so window handoffs happened reactively via 429s, and stale snapshots could misrepresent an account whose window had already reset.
Type of Change
Changes Made
NEUROLINK_PROXY_SESSION_RESET_TOLERANCE_MS) so near-simultaneous resets count as equal and the soonest weekly reset breaks the tie; bucketing keeps the comparator transitive. Accounts with no ticking session window (fresh, not yet started) have nothing expiring and sort after ticking windows.NEUROLINK_PROXY_SESSION_SOFT_LIMIT): an account whose session utilization reaches the limit — or whose status isthrottled— is demoted below accounts with headroom (never made unusable; still serves as fallback). The utilization snapshot is at least one response stale and requests arrive in concurrent bursts, so the small buffer makes the exhaustion handoff proactive and error-free instead of a burst of 429s at every window boundary. Saturated accounts order by soonest session reset (first back in service). Setting the limit to1disables demotion.allowed, reset unknown) in every routing decision, so stale pre-reset numbers can never saturate or reject an account whose window Anthropic has since renewed. The snapshot then self-corrects from live response headers on the account's next request.Testing
testSessionFirstOrdering(8 cases): session-first vs weekly, same-bucket weekly tie-break, 0.97 boundary (0.96 stays, 0.97/0.98 demote), throttled demotion, passed-reset freshening for both windows, both-saturated recovery order, env-configurable limit.pnpm exec vitest run test/proxyReliabilityHardening.test.ts— 36/36 pass (adjacent cooldown/quota persistence unaffected).tsc --strict, ESLint gates, prettier,validate:all, and full build pass via the pre-commit hook.Breaking Changes
NEUROLINK_PROXY_QUOTA_ROUTINGstill gates the whole feature; round-robin strategy is unaffected.Summary by CodeRabbit