Skip to content

fix(combos): a spent generic OAuth account cools the account, not the combo target - #7191

Closed
vadymhimself wants to merge 2 commits into
lidge-jun:devfrom
vadymhimself:fix/combo-pool-account-cooldown-all-pools
Closed

vadymhimself wants to merge 2 commits into
lidge-jun:devfrom
vadymhimself:fix/combo-pool-account-cooldown-all-pools

Conversation

@vadymhimself

@vadymhimself vadymhimself commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

A 429 from one account in a generic OAuth pool (xai, kiro, cursor, kimi, github-copilot, google-antigravity, and similar) can cool the whole combo target while other accounts in the pool are still healthy. The cooldown lasts as long as that account's Retry-After, up to the 24h server-delay ceiling in src/combos/failover.ts. Until it expires, the combo sends traffic to its fallback target (which may cost more) or answers with zero-attempt 503s.

This is the generic OAuth version of #6255 (carry of #6234), which made a 429 from an identified Anthropic OAuth or Codex pool account cool the account rather than the target.

Root cause: in a combo, the per-request account budget can stop the pool's own rotation before it reaches every stored account. With 5 xai accounts and the first 4 answering 429 Retry-After: 3600, the request reaches accounts 1-4. The arm that would rotate again is skipped once the budget is spent, so the pool never records account 4. The combo then gets the 429, has no identified pool account to attribute it to (cooledPoolAccountLabel covers only Anthropic and Codex), and cools xai/<model> for an hour while account 5 is healthy.

This change:

  • src/server/responses/core-combo.ts: new cooledGenericOAuthAccountLabel, next to cooledPoolAccountLabel, runs only for a 429 with target cooldown scope. It finds the refused account through the child's accountLogLabel, which must match exactly one stored account, and passes it to the pool's own rotateGenericOAuthAccountOn429. That cools the account under the pool's existing Retry-After policy, including the last account the budget left unrecorded. The failure is attributed to the account only when the rotator returns an eligible sibling. In that case the existing advanceComboAfterFailure account-scoped path leaves the target uncooled. A lone account, a fully cooled pool, an ambiguous label, and any non-429 failure still cool the target as before.
  • No new constants. Anthropic OAuth, Codex, and isAnthropicPoolLocalRefusal are unchanged. No file under src/oauth/ or src/combos/ changes.
  • API-key pools are unchanged on purpose. In my testing, key-pool 429 rotation walked every key inside the attempt (6 of 6, and 10 of 10 in both stream and non-stream), so a 429 only reaches the combo once every key is cooled. In that case cooling the target is the correct anti-hammer behavior.
  • structure/runtime.md: the combo cooldown paragraph used to say generic OAuth always keeps the target cooldown. It now describes the new rule and explains why API-key pools need no attribution.
  • tests/oauth/generic-oauth-failover.test.ts: the rotator-count source oracle goes from 5 to 6 rotateGenericOAuthAccountOn429( sites. A comment says the new site sends nothing and only records and attributes.

Possible conflict: #7187 also edits core-combo.ts, around lines 1076 and 1224-1239. This PR changes only the imports, a new function after cooledPoolAccountLabel, and the failedAccount expression (~1154). The hunks do not overlap.

Verification

  • Red before the fix, on clean upstream/dev 0b13396: the new tests/oauth/generic-oauth-combo-account-cooldown.test.ts failed 1/2. With 5 accounts and 4 refusing, isComboTargetInCooldown("pooled", xai/m1) was true.
  • Green after: bun test tests/oauth/generic-oauth-combo-account-cooldown.test.ts passed 2/2.
    • The 5-account case first checks the precondition that the request never reached account 5 before failing over to the fallback. It then checks that the target is not cooled and that the pool cooled every account the request reached, the last one included. The next request is served by the pool, ends on account 5, and never reaches the fallback.
    • When every account is spent (2 accounts, both refusing), the target still cools.
  • Focused related files: 129 tests across 8 files, 0 fail: generic-oauth-combo-account-cooldown, anthropic-combo-account-cooldown, always-on-429-failover, generic-oauth-failover, combo-codex-exhaustion-cooldown, server-combo-cooldown-recording, test-layout, test-layout-tooling.
  • bun run typecheck: clean.
  • bun run structure:check: passed.
  • bun run privacy:scan: passed.
  • bun run test:changed (Bun 1.4.3): 14701 pass / 35 skip / 1 fail. The one failure is connected-client runtime probe scope > a rejected preferred runtime falls back without rewriting the saved selection in tests/cli/cli-connect-readiness.test.ts. It fails the same way on a clean upstream/dev 0b13396 checkout and is unrelated to this change.
  • Full bun run test was not run locally. CI covers it.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of rate limits for generic OAuth accounts. When one account is rate-limited and another eligible account is available, requests can continue through the alternate account without placing the entire combo target on cooldown.
    • When no eligible alternate account is available, the combo target still enters cooldown as before. Unknown or ambiguous account attribution also retains the existing cooldown behavior.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • Required local validation passed; commands, results, and any full-suite exception are documented.

  • I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

@vadymhimself

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 53d22dc1-1fd5-433e-9e40-cad9c959bf01
📥 Commits

Reviewing files that changed from the base of the PR and between 0b13396 and 3fc3b18.

📒 Files selected for processing: 6
  • scripts/test-layout/layout.json
  • src/server/responses/core-combo.ts
  • structure/runtime.md
  • tests/fixtures/test-layout-expected.json
  • tests/oauth/generic-oauth-combo-account-cooldown.test.ts
  • tests/oauth/generic-oauth-failover.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

📝 Walkthrough

Generic OAuth 429 handling now checks whether the failed account can rotate to an eligible sibling before applying a combo-target cooldown. New integration tests cover available-sibling and exhausted-account cases. The new test is also added to the OAuth shard mapping.

Changes: Generic OAuth combo cooldown

Layer / File(s) Summary
Account attribution and cooldown handling
src/server/responses/core-combo.ts (lines 55–63, 139–158, 1157–1159), structure/runtime.md (line 507)
For a target-scoped 429, core-combo attributes the failure to a generic OAuth account only when one account matches and rotation identifies a sibling. Otherwise, target cooldown handling remains in place.
Behavior tests and test mappings
tests/oauth/generic-oauth-combo-account-cooldown.test.ts (lines 1–131), tests/oauth/generic-oauth-failover.test.ts (lines 562–587), scripts/test-layout/layout.json (line 922), tests/fixtures/test-layout-expected.json (line 635)
Integration tests cover a remaining eligible account and a pool where both accounts are spent. The test-layout mappings include the new test, and the expected generic rotator count changes from five to six.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant CoreCombo
  participant cooledGenericOAuthAccountLabel
  participant rotateGenericOAuthAccountOn429
  participant ComboFailureHandling
  CoreCombo->>cooledGenericOAuthAccountLabel: Resolve account for target-scoped 429
  cooledGenericOAuthAccountLabel->>rotateGenericOAuthAccountOn429: Rotate account using Retry-After and model
  rotateGenericOAuthAccountOn429-->>cooledGenericOAuthAccountLabel: Return sibling identification or no rotation
  cooledGenericOAuthAccountLabel-->>CoreCombo: Return account label or undefined
  CoreCombo->>ComboFailureHandling: Attribute account failure or cool target
Loading

Suggested reviewers: lidge-jun

Merge Risk: ⚪ Minimal · up to 3fc3b

The changed cooldown behavior appears mergeable after normal checks: a spent generic OAuth account can leave the combo target available when another account is eligible.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (3 skipped: 3… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: a spent generic OAuth account now receives cooldown handling instead of cooling the combo target when appropriate.
Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (3 skipped: 3 unsupported.)

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 15, 2026
@vadymhimself
vadymhimself force-pushed the fix/combo-pool-account-cooldown-all-pools branch from 3fc3b18 to b8f934e Compare October 15, 2026 20:35
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • hygiene: unsponsored_surface.

What to do

  • Fix unsponsored_surface — This changes an authentication, workflow, release-automation, or dependency surface. MAINTAINERS.md requires security review for these; ask a maintainer to apply maintainer-sponsored once they have reviewed it. Paths: src/oauth/generic-account-failover.ts.
  • Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

  • ⬜ Required local validation passed; commands, results, and any full-suite exception are documented.
  • ⬜ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • ⬜ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

0/4 boxes ticked.

This pull request was already a draft. Its draft status will be preserved after every issue above is resolved.
@vadymhimself Tick the boxes once required local validation has passed with commands, results, and any full-suite exception documented, your branch is on the latest dev commit, and every correct Codex and CodeRabbit finding is resolved.

… combo target

The per-request account budget can end a generic OAuth pool's rotation
before it reaches every stored account, so a 429 reaches the combo layer
while a sibling is still healthy. The combo then cooled the whole target
for that account's Retry-After (up to the 24h server-delay ceiling) and
sent traffic to the fallback target, or answered 503.

Resolve the refused account from the child's accountLogLabel and hand it
to the pool's own rotator, which cools it under the pool's Retry-After
policy. The target is spared only when the rotator names an eligible
sibling; a lone account or a fully cooled pool still cools the target.
Anthropic OAuth and Codex attribution are unchanged. API-key pools need
no change: their 429 rotation walks every key inside the attempt.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Being carried into #7224 with your authorship preserved; this PR will be closed when the bundle lands.

The combo layer asks the generic OAuth rotator only whether a healthy
sibling exists; it never dispatches to the account the rotator names.
Under pool.kernel round-robin the rotator committed that pick, advancing
the ring and making it sticky to an account no request was sent to.

Add a `commit` flag to rotateGenericOAuthAccountOn429 (default true).
The combo passes false: the failed account is still cooled, but the ring
is only peeked.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@github-actions github-actions Bot added the intake: hygiene-blocked Deterministic PR hygiene checks failed label Oct 16, 2026
lidge-jun added a commit that referenced this pull request Oct 16, 2026
…evidence fixes (#7191, #7204, #7187, #7209, #7175, #7185, #7193, #7197, #7198) (#7224)

Carried from #7191, #7204, #7187, #7209, #7175, #7185, #7193, #7197, #7198 (one commit per source PR, original author preserved). Security review follow-up: the Anthropic 402 combo cooldown now validates the sending credential's ownership before cooling an account.

Remote validation (hosted GitHub Actions unreliable; local tests not run by maintainer rule): union candidate codex/prr-integ-8 = dev c03eafd + #7224 + #7223 + #7225 + #7217 + #6454 + #6594 plus a Gentle Shell import-cycle fix (integ-9 e1f289c: typecheck + full suite on mini (Windows) and lidge (Linux), compared with the same host's dev runs) and a test-only Windows path-escaping fix for the reasoning-retention routing test (integ-10 0d4cba7, tree a2b309a: that file re-run on both hosts).

Co-authored-by: Vadym O <bolein95@gmail.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev in 6ccb47a via #7224 (your change is its own commit in that bundle, and the squash keeps your authorship as Co-authored-by). Thank you!

@lidge-jun lidge-jun closed this Oct 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working intake: hygiene-blocked Deterministic PR hygiene checks failed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants