fix(codex): pre-connection transport failures no longer rotate pool accounts (#914) - #1023
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: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![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: a8e10eb0b2
ℹ️ 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".
| | `accountPoolStrategy?` | `"quota" \| "round-robin" \| "fill-first"` | `"quota"` | Assignment strategy for new/unbound Codex requests. A request is unbound when it has no live (parent thread id, quota scope) affinity; a visible existing task can become unbound after proxy restart or affinity reset. `quota` picks the lowest-usage eligible account when no active account exists, keeps an eligible active account below `autoSwitchThreshold`, and after the threshold may move an unbound request or proactively rebind a bound task to a lower-usage eligible account. `round-robin` distributes unbound requests evenly; `fill-first` keeps assigning unbound requests to the active account until cooldown, unavailability, or the configured drain threshold. | | ||
| | `accountPoolStickyLimit?` | `number` | `1` | New/unbound task assignments retained on one round-robin selection before advancing; the counter advances when a task is bound, not after an upstream success. Range 1–100. | | ||
| | `upstreamFailoverThreshold?` | `number` | `3` | Consecutive transient failures before future new sessions fail over. Set `0` to disable. | | ||
| | `upstreamFailoverThreshold?` | `number` | `3` | Consecutive transient failures before future new sessions fail over. Set `0` to disable. Proven pre-connection DNS/TCP reachability failures are tracked at the provider-host level: they never affect account health, cooldowns, thread/session affinity, active-account selection, or Pool routing, and never count toward this threshold. | |
There was a problem hiding this comment.
This sentence is broader than the runtime change: I checked the sidecar paths, and enabled web-search/vision sidecars still record any non-timeout fetch rejection as connect_error (src/web-search/executor.ts:96-98, src/vision/describe.ts:116-118), while src/providers/openai-sidecar.ts:160-168 forwards that outcome into Codex account health. In that context a DNS/TCP failure can still count toward upstreamFailoverThreshold, so the public docs should either limit this guarantee to main/compact forward sends or the sidecar recorders should use the new neutral classifier too.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
2724c76 to
6c8a683
Compare
a8e10eb to
2ac083a
Compare
…ccounts (#914) DNS/TCP-refusal rejections are host-wide, not account evidence: - new connect_neutral outcome class (with relayed 3xx as neutral) - upstream-reachability classifier (bounded cause walk, code-only) - retry attempt evidence (5xx/reset before rejection stays attributed) - bounded (provider, host) health ledger (128 entries, stalest-prune) - manual redirects on credential-bearing forward sends; 3xx relayed with Location instead of followed into dead-host rejections Classifier/retry semantics extracted from PR #966 (Yuxin-Qiao) with attribution; sidecar blast radius intentionally not inherited.
… not account evidence (#914)
…y, prune ordering (#914) - record neutral host evidence before account early-returns; reset the host streak on every real HTTP response (responses + compact, primary and alternate sends) - move the 3xx relay to the actual forward/passthrough branch (Location preserved); drop the generic-pipeline short-circuit (body lifetime, combo bookkeeping) - prune the host ledger only for genuinely new keys; key by canonical origin so http/https never share an entry - server-level 307 relay activation with seeded host-streak clear
…utral evidence (#914) - reset the host streak inside every fetch thunk (intermediate 5xx and 429 replays included), not only after the retry wrapper returns - neutral host evidence is written at the catch sites for pool AND direct forward sends; pool-only recorders stay account-scoped
2ac083a to
1ea1b29
Compare
1 participant
Summary
Stack 02 of the bug-stack campaign (
devlog/_plan/260805_bug_stack_campaign/030), stacked on #1020. Fixes #914.DNS/TCP-refusal rejections are machine/network-wide — every pool account shares the provider host, so rotating accounts cannot repair them. Today every non-timeout fetch rejection maps to
connect_error, and atupstreamFailoverThresholdthe streak soft-avoids a healthy account and clears thread affinity.What changes:
connect_neutraloutcome class: proven pre-connection failures (DNS, TCP refusal; BunConnectionRefused/FailedToOpenSocket, NodeECONNREFUSED/ENOTFOUND/EAI_AGAIN/ENETUNREACH/ENETDOWN/EHOSTUNREACH) record only to a bounded (provider, origin) host ledger — account health, streaks, soft-avoid, affinity, and rotation are untouched. Resets/TLS/unknown shapes stay account-attributed.UpstreamRetryEvidenceError): a transient 5xx or credential-visible reset before the terminal rejection keeps the failure account-attributed (mixed sequences can't be downgraded to neutral).Location(neutral class) instead of being followed into a dead-host rejection that masquerades as a pre-connection failure.Classifier/retry semantics extracted from PR #966 (Yuxin-Qiao) with attribution; its 22-file sidecar blast radius intentionally not inherited. Equivalence analysis in
devlog/_plan/260805_bug_stack_campaign/030.Tests
tests/upstream-reachability.test.ts(11) andtests/issue-914-transport-attribution.test.ts(6): classifier matrix, mixed evidence, host-ledger retention/churn, real Bun dead-port activation.tests/server-auth.test.ts: updated the dead-port expectation to the new neutral contract + new 307 relay activation (Location preserved, no health evidence, seeded host streak cleared).bun run typecheck0 errors;bun run privacy:scanpass; focused suites green.bun run teston Linux (ssh lidge): 8250 pass / 0 fail (baseline 8222). One Bun segfault during a run was rerun green —tests/multi-agent-compat.test.tsflake, unrelated to this diff (same tree passed).Provider docs
Five locale files updated: pre-connection reachability failures are host-level and never count toward
upstreamFailoverThreshold.