fix(auth): honor quota.skip_exhausted in isAuthenticated#581
Open
icebear0828 wants to merge 1 commit into
Open
fix(auth): honor quota.skip_exhausted in isAuthenticated#581icebear0828 wants to merge 1 commit into
icebear0828 wants to merge 1 commit into
Conversation
isAuthenticated() previously short-circuited any account whose cachedQuota was reported exhausted, regardless of the quota.skip_exhausted config. That diverges from hasAvailableAccounts and AccountLifecycle.acquire(), which both still allow acquiring limit-reached accounts when skip_exhausted=false (the documented opt-in for letting requests retry against still-limit-reached accounts on the chance the upstream window has actually rolled). The mismatch causes any pool where every account is at the cached limit to fail isAuthenticated() while routing would still produce a viable account, so every router that calls accountPool.isAuthenticated() (`/v1/chat/completions`, `/v1/messages`, `/v1/responses`, model listing, health) returns 401 even though acquire() would succeed. Gate the cachedQuota check on the same skipExhausted flag. Use !== false so a missing quota config block (tests with minimal getConfig mocks, hand-rolled configs) treats the default as true, matching the schema default. Tests cover empty pool, default-skip non-exhausted, default-skip exhausted (still 401), explicit skip_exhausted=false exhausted (now authenticated), and disabled accounts (still 401).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AccountRegistry.isAuthenticated()ignoredquota.skip_exhausted, treating every cached-quota-exhausted account as unusable regardless of config. That diverges fromhasAvailableAccounts()andAccountLifecycle.acquire(), which still allow acquiring limit-reached accounts whenskip_exhausted=false. Any pool where every account is at the cached limit failsisAuthenticated()whileacquire()would succeed, so every router gated onaccountPool.isAuthenticated()(/v1/chat/completions,/v1/messages,/v1/responses, model listing, health) returns 401 even though the request could be served.This was flagged as [P1] by Codex review on the diff that retired
status="rate_limited".Changes
src/auth/account-registry.ts:294-313— gate thecachedQuotacheck onskipExhausted, mirror the rule used byhasAvailableAccounts(). Use!== falseso test mocks with partial config (quotablock missing) observe the schema default (true) rather than tripping a TypeError.tests/unit/auth/account-pool-has-available.test.ts:188-251— newAccountPool.isAuthenticateddescribe with 5 cases: empty pool → false; active healthy → true; cached-exhausted +skip_exhausted=true(default) → false (regression guard); cached-exhausted +skip_exhausted=false→ true (P1 fix); disabled-only → false.CHANGELOG.md—### Fixedbullet under[Unreleased].Default config behaviour unchanged:
quota.skip_exhausteddefaults totrue, so the cached-quota check still applies for all out-of-the-box deployments.Test Plan
npx vitest run tests/unit/auth/account-pool-has-available.test.ts— 19 → 24 tests pass (5 new)npx vitest run— 2259 tests pass, 1 skipped, 0 failuresnpx tsc --noEmit— cleanNotes
A related [P2] finding from the same review (
src/auth/cf-path-block-tracker.ts:34-35— auto-disable counter doesn't reset on successful upstream use) lives on an unmerged local branch, not ondev. It will be addressed when that branch opens its own PR.