fix(auth): do not park healthy quota accounts as expired - #12452
diegosouzapw merged 4 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The required npm run test:coverage gate and final coverage result (60/60/60/60 minimum) are not demonstrated in the PR validation notes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes a routing/auth resilience regression where healthy provider connections could be persisted into terminal states (expired / credits_exhausted) after certain 401 responses, causing combo routing to permanently pre-skip them without reattempting.
Changes:
- Classify 401 responses with credits/quota bodies as
QUOTA_EXHAUSTED(instead of genericUNAUTHORIZED) and persist them ascredits_exhausted. - Avoid terminal-parking OAuth connections as
expiredwhen the stored token expiry is still in the future (cooldown instead of terminal). - Extend connection-recovery to re-probe stale
expiredrows after 30 minutes (with a blocklist for true deactivation/invalid-grant cases) and adjust token health sweep behavior to includecredits_exhausted.
Commands run (review): none
Tests changed: tests/unit/quota-connection-recovery.test.ts, tests/unit/false-terminal-401-quota.test.ts
Coverage result (custom): not provided / not verified in this review (npm run test:coverage not run)
File summaries
| File | Description |
|---|---|
tests/unit/quota-connection-recovery.test.ts |
Adds assertions for the new “expired reprobe” selection behavior and inclusion in recoverable selection. |
tests/unit/false-terminal-401-quota.test.ts |
New regression tests ensuring 401 quota bodies don’t park connections as expired, and valid-token 401s don’t terminal-expire OAuth connections. |
src/sse/services/auth.ts |
Updates terminal-status resolution to detect quota signals (including 401 quota cases) and prevents “still-valid token” OAuth 401s from being persisted as expired. |
src/lib/tokenHealthCheck.ts |
Stops skipping credits_exhausted so OAuth refresh can clear false quota marks; keeps banned/expired as terminal (with existing exceptions). |
src/lib/quota/connectionRecovery.ts |
Adds expired reprobe candidate logic (with lastErrorType blocklist) and threads lastErrorType through the tick wiring. |
open-sse/services/errorClassifier.ts |
Expands quota classification to include HTTP 401 when the body indicates credits/quota exhaustion. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Conflicts resolved — rebased onto current release/v3.8.51. Kept the per-model 402 connection-wide guard from #12242 and still classify 401 quota/credits bodies as credits_exhausted (not expired) for non-per-model providers. |
2f266b4 to
6eb763b
Compare
6eb763b to
1bb648e
Compare
|
Follow-up pushed: If |
|
sweep-reds / babysit: reds on this PR are inherited from base-red #12581, not this 401-quota classification diff. Skipping a code change; did not merge
|
|
sweep-reds round 6 / babysit: re-verified — every remaining red is inherited from base-red #12581, not this 401-quota classification diff. Skipping a code change; did not merge
STOP. Resume after #12581 drains. |
Co-authored-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
Shrink tokenHealthCheck comments under the frozen LOC cap, split authTerminalStatus helpers so cyclomatic stays at the base count, and register the new 401-quota unit file in tap.testFiles. Co-authored-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
The helper moved to authTerminalStatus.ts; the leftover import trips @typescript-eslint/no-unused-vars on parent Quality Gates. Co-authored-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
04e0625 to
ccf25b9
Compare
85b8d12
into
diegosouzapw:release/v3.8.51
…w#12452) Validado em lote numa worktree combinada com os 10 PRs desta leva sobre o tip de `release/v3.8.51`: `typecheck:core` limpo, `check-file-size` OK e **241/242** nos 29 arquivos de teste que os PRs tocam. A única "falha" não é falha: `tests/unit/autoCombo/strict-zero-cost-filter.test.ts` é um teste em estilo Vitest que eu incluí por engano na invocação do runner nativo do Node — ele quebra no import (`@vitest/runner`), não numa asserção. Ao investigar, descobri que esse arquivo não roda em nenhum dos dois runners hoje (o glob do `test:unit` não lista `autoCombo` e o `include` do Vitest só pega `.tsx` nessa pasta); é um problema pré-existente do repositório, sem relação com esta leva, e vou registrá-lo separadamente. O diegosouzapw#12636 conflitava apenas na lista de testes do `@omniroute/opencode-plugin/package.json`, de forma aditiva: o tip já tinha `models-fetcher.test.ts` (do diegosouzapw#12607, irmão desta mesma leva) e o diegosouzapw#12636 acrescenta `telemetry.test.ts`. Fiz a união dos dois lados (25 arquivos contra 24 de cada) em vez de escolher um, o que teria removido um arquivo da suíte do plugin em silêncio. Obrigado, @RaviTharuma.
A non-terminal 401 on an OAuth connection (a still-valid token after a refresh, which diegosouzapw#12452 rightly keeps out of `expired`, or an invalid-token class like diegosouzapw#12594) got cooldown 0: the status_401 rule declares no cooldown, although the diegosouzapw#12452 comment says such a 401 "must cooldown". Every request then re-selected the account, refreshed the token and failed again. Seen in production during a 21-minute upstream 401 burst on two Codex accounts: 273 token refreshes and 216 requests losing ~3 s each before falling back. - Cool such a connection down with exponential backoff: the runtime OAuth base cooldown (5 s by default), doubling per consecutive 401, capped at the existing but unused COOLDOWN_MS.unauthorized (2 min). The streak is kept in memory because the 401 rule does not raise backoffLevel and selection resets it once a cooldown passes; a success (clearAccountError) or a quiet window ends it. testStatus is left untouched so the connection stays refreshable (diegosouzapw#12594), and a 401 that only names an unsupported model is excluded (diegosouzapw#7268). - Do not wait out an auth cooldown in the single-model cooldown-aware retry: it has no known end, so each retry would hit the same 401 and refresh again (a single-account setup would wait ~155 s and refresh 6 times, then fail).
Summary
Healthy accounts with remaining quota were persisted as
expired/credits_exhaustedand then never retried.UNAUTHORIZED.tokenExpiresAtis still in the future is a cooldown, notexpired.expiredrows after 30m (unlessinvalid_grant/ deactivated).credits_exhausted, so OAuth refresh can clear a false no-quota mark.Real invalid keys (
unauthorizedon API-key providers) still terminal-expire (#8200 contract).Related Issues
Validation
npm run lint(CI)22 pass, 0 fail.
Tests Added Or Updated
tests/unit/false-terminal-401-quota.test.tstests/unit/quota-connection-recovery.test.tsCoverage Notes
Covers 401 credits body, 401 with a still-valid access token, and expired reprobe selection.
Reviewer Notes
Does not re-enable the credential-health scheduler. Operators who set
OMNIROUTE_DISABLE_CREDENTIAL_HEALTH_CHECK=truestill get recovery via the connection-recovery tick.