Repository navigation
fix(combos): a spent pool account must not cool the whole target (carry #6234) - #6255
Conversation
`advanceComboAfterFailure` cooled the combo target for any target-scoped failure, and `cooldownMapKey` is `comboId + provider/model` with no account in it. A provider with an account pool therefore shared one cooldown entry across every account, so a 429 from a single spent account removed the whole target from selection and the remaining accounts were never tried. On 2026-09-28, 01:17-05:18 WITA, one Anthropic account answered `Retry-After: 318747` (88.5 hours, its weekly window gone). `anthropic/claude-opus-5-5` was cooled outright and the gateway returned 503 with zero upstream sends for four hours, while a direct probe showed four of five accounts returning HTTP 200. A 429 names the ACCOUNT that is spent, not the target. Every account pool already records that account's own cooldown and routes past it (`rotateAnthropicAccountOn429`, `rotateKeyOn429`, the Codex health store), so the combo layer needs to stop duplicating that decision at target scope rather than to grow a second cooldown map beside the pool's. The account is read from the provider label the dispatch stamped (`anthropic-p2e22d0`), which a pool rewrites and a configured provider name never matches. `accountLogLabel` is deliberately not used: it is stamped for a lone configured API key too, so it cannot tell a pooled account apart from a provider with no pool to rotate within. Unknown account still cools the target, which is the half that keeps this from trading a blackhole for a hammer. It also lands exactly where it is needed: when every account is cooled, the pool refuses locally with a 429 BEFORE it picks one, so no account is named, the target cools on the pool's own Retry-After, and nothing is sent upstream. Target-level failures (provider down, model gone) and provider-scoped credential failures are untouched. Co-Authored-By: Claude Code <noreply@anthropic.com> (cherry picked from commit 62bfd5f)
CodeRabbit review on #6234. `cooled` is a filtered subset of `ids`, so its positions are not account indexes. Mapping it with `cooled.map((_, index) => credential(index))` built tokens for accounts 0 and 1 when accounts 0 and 2 were the cooled pair, and the assertion would then pass even if the retry left through cooled account 2 -- the exact failure the test exists to catch. Each cooled id now resolves to its own token, and the same claim is asserted from the other direction: the token actually sent is mapped back to its account and that account must be one the first request never cooled. Co-Authored-By: Claude Code <noreply@anthropic.com> (cherry picked from commit e8ee8a8)
Record Anthropic 429 health after the per-request retry cap, and skip combo target cooling only when the attributed account has a live pool cooldown. Keep unknown and non-pool failures on the target path. Co-authored-by: Vadym O <bolein95@gmail.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (16)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 70 / 80이 PR은 #6234를 현장 이야기는 #6234와 같다. 2026-09-28에 Anthropic 계정 하나가 베이스는 라인 - 라인 - 라인 - 메인테이너의 판단이 필요한 지점 #6234와 #6255 중 무엇을 남길지. 이 PR이 carry + 강화분이면 #6234는 닫는 쪽이 맞다. Codex는 마지막 429 health 기록이 Anthropic만큼 명시적으로 추가되지 않았다. Codex 콤보도 같은 이득을 줘야 하면 기록 지점을 맞출지, 지금은 Anthropic 사고만 고치고 Codex는 health가 있을 때만 이득을 보게 둘지. CI 빨간 소스 스캔을 이 PR에서 고칠지. 너의 추천 방향은 맞다. 풀이 이미 계정 쿨다운을 갖고 있으니 콤보가 타깃을 한 번 더 식히지 않는 쪽이 맞고, live health를 요구한 것도 #6234보다 안전하다. 머지 전에 always-on-429 소스 스캔 실패부터 고치고, #6234는 이 PR에 흡수되면 닫으세요. draft 해제·체크리스트·보안 리뷰는 그다음이다. 이 댓글은 grok-bot이 작성했습니다 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f7737756b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| config, | ||
| pick.target.provider, | ||
| pick.target.model, | ||
| poolAccountProviderLabel(childLog.provider, pick.target.provider), |
There was a problem hiding this comment.
When a canonical Codex pool dispatch uses the main-pool account and its terminal 429 records a cooldown, formatCodexProviderForLog deliberately leaves childLog.provider equal to the configured provider while childLog.accountLogLabel is main (src/codex/routing.ts:1617-1625, src/server/responses/request-prepare.ts:1307-1310). Passing only the provider label here therefore yields no failedAccount, and the combo cools the entire target even when a stored backup account is healthy. Pass the account label as well and resolve MAIN_CODEX_ACCOUNT_ID when it has matching live quota health.
Useful? React with 👍 / 👎.
| if (providerName === "anthropic") { | ||
| const matches = getAccountSet("anthropic")?.accounts.filter(account => | ||
| formatAnthropicProviderForLog("anthropic", account.id) === label) ?? []; | ||
| return matches.length === 1 && getAnthropicAccountHealthSnapshot(matches[0]!.id) ? label : undefined; |
There was a problem hiding this comment.
For an Anthropic combo using a web-search or image sidecar, sidecar-execution.ts:262-306 returns a terminal 429 without calling recordAnthropicAccount429 when the shared hop reservation is denied or the three-failover cap has already been reached. The current account consequently has no health snapshot here, so a five-account pool that exhausts the fourth account cools the whole combo target and blocks the untouched fifth account. Apply the same terminal-429 recorder added to the dispatch and continuation loops before these sidecar returns.
Useful? React with 👍 / 👎.
…pool When every Anthropic OAuth account is cooled, the pool refuses locally with a 429 whose Retry-After is `getAnthropicPoolRetryAfterSeconds`: the earliest per-account cooldown, restated. lidge-jun#6255 deliberately keeps cooling the combo TARGET on that refusal, since no account was identified, so the server-delay path records it as an authoritative upstream delay of up to the 24h ceiling. That number is local state, not an upstream statement, and local state changes: an account is added, resumed, re-authenticated or has its cooldown cleared. Only a combo edit or delete clears a target cooldown (`clearComboTargetCooldowns` is called solely from management/combo-routes.ts), so after a weekly window exhausts every account, a freshly added account sits ignored by every combo that routes through Anthropic until the target expires, up to a day, while the pool itself would already serve it. `isAnthropicPoolLocalRefusal()` classifies that one response by provenance, not by current pool state alone: the target must be the OAuth `anthropic` provider, no account may have been identified for the failure, the status must be 429, and the pool must be holding the wait (no eligible account, at least one known cooldown expiry). Only then is the pool's restated Retry-After withheld from the target cooldown. The target still cools (the anti-hammer guard lidge-jun#6255 intended), but for the local fallback, which `coolComboTarget` already caps at ten minutes. Re-offering the target while the pool still holds the wait costs a local refusal with zero upstream sends. A genuine upstream Retry-After keeps parking the target in full: an upstream 429 on a pool account always cools and names that account, an API-key provider never qualifies, and a single account is never cooled by rotation. The tested wait-vs-failover contract in combo-authoritative-reset.test.ts passes unchanged. The classifier lives in core-combo.ts and only reads the pool through existing exports, so no auth surface is modified. Verification: `bun run typecheck` clean. always-on-429-failover: 11/11 with two new cases covering the pool-held refusal, an identified account, an API-key provider, a non-429 status, recovery on a newly saved account, and the target cooldown expiring at ten minutes rather than the pool's four-day Retry-After. Both fail with the classifier forced false. tests/routing plus the three combo cooldown suites: 1249 pass / 21 fail against 1247 / 21 on dev, the same 21 pre-existing failures. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…pool When every Anthropic OAuth account is cooled, the pool refuses locally with a 429 whose Retry-After is `getAnthropicPoolRetryAfterSeconds`: the earliest per-account cooldown, restated. lidge-jun#6255 deliberately keeps cooling the combo TARGET on that refusal, since no account was identified, so the server-delay path records it as an authoritative upstream delay of up to the 24h ceiling. That number is local state, not an upstream statement, and local state changes: an account is added, resumed, re-authenticated or has its cooldown cleared. Only a combo edit or delete clears a target cooldown (`clearComboTargetCooldowns` is called solely from management/combo-routes.ts), so after a weekly window exhausts every account, a freshly added account sits ignored by every combo that routes through Anthropic until the target expires, up to a day, while the pool itself would already serve it. `isAnthropicPoolLocalRefusal()` classifies that one response by provenance, not by current pool state alone: the target must be the OAuth `anthropic` provider, no account may have been identified for the failure, the status must be 429, and the pool must be holding the wait (no eligible account, at least one known cooldown expiry). Only then is the pool's restated Retry-After withheld from the target cooldown. The target still cools (the anti-hammer guard lidge-jun#6255 intended), but for the local fallback, which `coolComboTarget` already caps at ten minutes. Re-offering the target while the pool still holds the wait costs a local refusal with zero upstream sends. A genuine upstream Retry-After keeps parking the target in full: an upstream 429 on a pool account always cools and names that account, an API-key provider never qualifies, and a single account is never cooled by rotation. The tested wait-vs-failover contract in combo-authoritative-reset.test.ts passes unchanged. The classifier lives in core-combo.ts and only reads the pool through existing exports, so no auth surface is modified. Verification: `bun run typecheck` clean. always-on-429-failover: 12/12 with three new cases. One drives the real `executeComboResponses` failure path with a child that answers the pool's 429 and a four-day Retry-After: the target re-opens at ten minutes when the pool holds the wait, and stays parked in the control run where the same 429 arrives with an account still eligible; reverting only the call site to always forward Retry-After fails it. The other two cover the pool-held refusal, an identified account, an API-key provider, a non-429 status, recovery on a newly saved account, and the target cooldown expiring at ten minutes rather than the pool's four-day Retry-After. Both fail with the classifier forced false. tests/routing plus the three combo cooldown suites: 1249 pass / 21 fail against 1247 / 21 on dev, the same 21 pre-existing failures. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…pool (#6383) When every Anthropic OAuth account is cooled, the pool refuses locally with a 429 whose Retry-After restates the earliest per-account cooldown. The combo target now cools for the local fallback on that pool-local refusal instead of the restated wait (up to the 24h server-delay cap), so a resumed or newly added account is not ignored. Genuine upstream 429s (API-key providers, identified pool accounts) keep their full Retry-After. Follow-up to #6255. Co-authored-by: lidge-jun <jun@lidgeai.com>
2 participants
Summary
Verification
bun install --frozen-lockfile— passed.origin/dev:bun test tests/routing/always-on-429-failover.test.ts tests/server/server-combo-cooldown-recording.test.ts tests/adapters/anthropic/anthropic-combo-account-cooldown.test.ts— 18 passed, 0 failed. The source oracle now checks the quorum gate inrecordAnthropicAccount429, which the rotator calls before choosing a replacement.bun test tests/ci-workflows/file-size-ratchet.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts— 27 passed, 0 failed;bun run typecheck,bun run privacy:scan, andgit diff --check— passed on the new head.bun run typecheck— passed.bun test tests/server/server-combo-cooldown-recording.test.ts tests/adapters/anthropic/anthropic-combo-account-cooldown.test.ts— 9 passed, 0 failed.bun test tests/adapters/anthropic tests/oauth/oauth-accounts-api.test.ts tests/cli/cli-account-pool-verbs.test.ts tests/server/server-combo-cooldown-recording.test.ts tests/usage/usage-provider-label.test.ts) — 649 passed, 5 failed, all inanthropic-model-routes.test.ts. That file passed alone (15/0). Running the same 39 files one process at a time passed 654/0; the combined-run interaction remains to be resolved by CI or follow-up investigation.bun test tests/test-layout.test.ts tests/test-layout-tooling.test.ts tests/ci-workflows/file-size-ratchet.test.ts tests/lab/core-lab-boundary.test.ts— 52 passed, 0 failed.bun run structure:check,bun run skill:surface:check,bun run privacy:scan,git diff --check— passed.cd docs-site && bun install --frozen-lockfile && bun run build— passed; 537 pages built.bun run testwas not run locally because four RT6 lanes share this machine. Exact-head CI run 36615011801 failed test 2/4: batch 11 timed out after 120 seconds in a multi-file process; all 12 files passed in the CI per-file attribution sweep. The PR remains draft. Full local tests were omitted because four RT6 lanes share this machine. Security review of the pool bookkeeping is pending.Checklist
Co-authored-by: Vadym O bolein95@gmail.com