fix(combos): classify failover failures by blast radius (bounded carry of #3348) - #3565
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
At exact head d0f80e85fe33d6e2ac9666ebea1a10fd5a05d92f, the hop decision and cooldown scope disagree for a structured model-scoped 403. comboFailureDecision(403, ..., { code: "unsupported_model" }) correctly returns hop, but comboFailureCooldownScope() reaches the blanket status === 403 branch and returns provider. That cools every target using the provider row, including another model that may be valid, even though the new comment explicitly says model-scoped rejection is target-local. Please classify the structured model codes (model_not_found, model_unavailable, unsupported_model, and any lifecycle codes intentionally shared with the decision path) before the blanket credential-status branch and return target scope, then add a regression that a 403 unsupported model cools only the failed target and leaves a same-provider/different-model combo target eligible. The key-pool 401 rotation and the other bounded classifications look directionally sound, but this blast-radius mismatch is a merge blocker.
리뷰 · 우선순위 61 / 80이 PR은 #3348의 분류(classification) 슬라이스를 가져와 blast radius에 맞게 쿨다운·홉을 나눕니다. 지금 src/combos/failover.ts src/combos/resolve.ts - src/providers/key-failover.ts src/server/responses/core.ts key-401 while 루프 - OAuth replay 이후에 두는 순서는 맞다. body cancel 후 rebuild는 소켓 누수 방지용. 실패 키를 다 쓰면 break 후 기존 401 응답 경로로 가는지 테스트에 있으면 충분하다. src/lib/errors.ts - #3348 - 원본 OPEN. persistence 조각은 이번 PR에 없음 — 닫을 때 “classification landed; remainder still open/out of scope”를 댓글에 남기자. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
`ocx claude` hard-errored and returned 1 whenever Claude routing was disabled (`src/cli/claude.ts:420` on dev), so the command was unusable with the Claude toggle off even though a native `claude` binary was available. Launch natively in that case instead. Only an explicit `false` triggers the fallback — from config, or reported live by `GET /api/claude-code` — so a proxy predating the `enabled` field stays routed, and an absent proxy still starts one rather than falling back. The native session must not inherit proxy state, so it removes only values it can prove OpenCodex owns: `ANTHROPIC_BASE_URL` when it targets this proxy's own loopback host and configured port with a proxy-issued admission token, the `CLAUDE_CODE_*` discovery and auto-context levers, and model slots that resolve only through the proxy. An unrelated `http://localhost:8080` gateway and a user `sk-ant-` credential are preserved. Client-ownership preflight runs before any fallback, so an invalid or mismatched connected client still fails closed. Three fixes on top of the contributor's head: - Sync all eight `docs-site` `guides/claude-code.md` pages, which still described `ocx claude` as proxy-only. - Distinguish an absent `settings.json` from a corrupt one in `readPickerDefaultModel`. Swallowing both alike dropped the "saved model requires the proxy" warning exactly when the file was broken; a corrupt file now warns and names the path without echoing contents. - Restore the `#764 / SERVICE_STOP_LIVENESS` rationale comment above `ensureProxyForClaude`, which the diff deleted while keeping the behavior. Carry of #3519. Co-authored-by: everton-dgn <58889432+everton-dgn@users.noreply.github.com>
reconcileOAuthProviders mutated the in-memory config and called saveConfig(config), so a startup snapshot overwrote any operator edit made after loadConfig() returned. runModelRenameStartupMigration had the same shape. Both now project onto a clone and commit through mutatePersistedConfig, which rebases the write on the newest on-disk snapshot, so a concurrent edit survives. Persistence failure degrades rather than throws. Both functions run inside startServer (src/server/index.ts:651 and :663), which is synchronous by design and wraps neither call in try/catch, so a throw there takes the whole proxy down over a config file the operator can still repair. A missing, malformed or contended config now warns once and adopts the projection in memory, matching every other mutatePersistedConfig consumer (src/storage/policy.ts, src/codex/plan-from-token.ts, src/server/management/agent-settings-routes.ts). Adoption is key by key over the touched keys only. A clear-and-reassign preserves the top-level object identity while silently detaching every nested sub-object a caller still holds a reference to. Tests: the concurrent-edit cases are the RED-on-dev proof of the defect (they fail against unmodified dev, which clobbers). The degrade-not-throw assertions are RED against #3524's head, which threw. The new tests/server/server-startup-reconcile-resilience.test.ts covers the boot path; its /healthz case binds a listener and is skipped where Bun.serve cannot bind, so it is a hosted-CI-only assertion. Co-authored-by: yansigit <44089734+yansigit@users.noreply.github.com>
A combo failure recorded the same cooldown regardless of what the failure actually said. An oversized request cooled a healthy target, a per-request free-tier cap cooled the whole provider for every other combo, and a rejected credential cooled only the one target that happened to hit it. Meanwhile pickComboTarget never consulted the cooldown map at all, so a target cooled a moment earlier was picked again on the next attempt. ComboFailureCooldownScope gains "none" for request-shape failures (413, input_admission_refused, context_length_exceeded, tool_catalog_too_large, cursor_root_envelope_limit, target_incompatible, and the provider hard-cap overflow), and returns "provider" for 401/402/403 and credential/billing codes. free_rate_limited leaves isProviderScopedQuotaCap: it is evaluated per request, so it keeps its hop verdict but stops recording provider-wide evidence. comboFailureDecision additionally hops model-scoped rejections and 402/425. Generic 410 and 413 remain terminal, asserted explicitly so a future widening of the hop list cannot swallow them silently. "malformed upstream" now infers 502 rather than falling into the generic "malformed" 400 branch: bytes the upstream mangled are a provider protocol failure, not a bad client request. Scoped to that phrase, so plain "malformed" keeps its 400 verdict, and asserted on the message-only path where the existing structuredServerClass override in httpStatusFromTerminalError cannot absorb it. Carries the classification half of #3348. Disk persistence of cooldowns and the policy-fallback status synthesis are deliberately separate and not included. Co-authored-by: RHODIZSECURITY <180237049+RHODIZSECURITY@users.noreply.github.com>
… the request A static API-key pool already rotates on 429 but abandoned the provider on 401, even though one revoked or mistyped key says nothing about its siblings. Add rotateKeyOn401/rotateProviderTransportOn401 alongside the 429 pair (sharing the same persisted-config CAS and transport-rebuild rules) and consult them in the Responses recovery loop, after the OAuth replay so a refreshable token is never treated as a dead key. hasKeyPoolFailover already excludes oauth/forward modes. A 401 is a verdict about the credential, not a timing signal, and upstreams send no Retry-After for it, so the failed key is held for the full cap rather than the 429 default. The new key-401 recovery kind is a four-site chain, not one edit: the union in src/usage/log.ts, the ATTEMPT_RECOVERY_KINDS set that filters it back on read, the emit site in the Responses loop, and COOLDOWN_RECOVERY_KINDS in routing analytics. The regression round-trips a persisted attempt through the log file, because a kind added to the type but missing from the set writes fine and vanishes on read-back. Carries the key-401 half of #3348. Co-authored-by: RHODIZSECURITY <180237049+RHODIZSECURITY@users.noreply.github.com>
… recovery site Review round 1 (023): the rotator-count guard now records the pre-stream 401 site (key = 3) and rotateKeyOn401 / rotateProviderTransportOn401 get their own cooldown assertions (MAX_COOLDOWN_MS on 401 vs the 429 default). Co-authored-by: RHODIZSECURITY <180237049+RHODIZSECURITY@users.noreply.github.com>
589347f to
29182de
Compare
2faac80 to
6a31fcb
Compare
|
Maintainer admin merge (ruleset bypass recorded per MAINTAINERS.md): bounded classification-only carry of #3348, exact-head CI green on 6a31fcb (24 pass / 2 skipped / 0 fail), stack layer above the merged #3564 (526d4bf). Review 023 blockers (rotator-count guard = 3, 401 cooldown tests) folded in the top commit. #3348's persistence and policy-fallback halves remain deferred. wp2 Stack B top of the 260905 open-work closeout. |
2 participants
Summary
The classification-only slice of #3348 (its disk-persistence and policy-fallback status changes are deliberately excluded).
src/combos/failover.ts: cooldown scope gains"none"for request-shape refusals,free_rate_limitedno longer cools the whole provider, credential/billing failures cool the provider, model-scoped rejections plus 402/425 hop instead of ending the chain; generic 410/413 staystop.src/combos/resolve.ts: a target inside its cooldown window is skipped.src/lib/errors.ts:"malformed upstream"infers 502. Second commit: a key-pool 401 is recovered by rotating the key (src/providers/key-failover.ts, usage log + analyticskey-401recovery kind, pre-stream loop incore.ts:6148). No timers introduced, so nothing tounref().Carries #3348 (author @RHODIZSECURITY) onto current
dev; contributor hunks reapplied. Supersedes #3348.Stack (merge bottom-up; each layer targets the branch below):
Depends on the layers below it. Review this PR's diff only. Unit:
devlog/_plan/260905_open_work_closeout/(020, 021, 022).Verification
bun run typecheck— exit 0 on this layer and on the stack top (d0f80e85f).bun test tests/routing/router-combo-failover-classification.test.ts— RED on dev 6 pass / 8 fail (one per claim), GREEN 14 pass / 0 fail; 156 pass / 0 fail across the eight related files incl.tests/codex-integration/combos.test.ts(one assertion updated:free_rate_limited→none). Layout guard 17/0.test:changedpulls ~776 files throughcore.tsand is port-bind-red in the sandbox; hosted CI is the gate.tests/lab/core-lab-boundary.test.ts.Checklist
codex/260905-startup-reconcile-persistence(stack layer)Co-authored-bytrailer in the branch commitCo-authored-by: RHODIZSECURITY 180237049+RHODIZSECURITY@users.noreply.github.com