fix(codex): keep shared network failures account-neutral - #922
Conversation
📝 WalkthroughWalkthroughChangesThe PR adds bounded transport-error classification and applies it across Codex Responses, relay, sidecar, vision, web-search, and Live flows. Account-neutral failures preserve account state and release probe leases. Tests and localized documentation cover the new behavior. Account-neutral transport handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Relay
participant TransportClassifier
participant AccountPool
participant ProbeLease
Client->>Relay: Send upstream request
Relay->>TransportClassifier: Classify transport result
TransportClassifier-->>Relay: Return neutral or account-scoped outcome
Relay->>AccountPool: Record account-scoped outcome when applicable
Relay->>ProbeLease: Release owned lease
Relay-->>Client: Return upstream or transport response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 888c9558b3
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/reference/configuration/providers.md`:
- Line 25: Update the upstreamFailoverThreshold documentation to explicitly
describe account-neutral failures as runtime-classified exact pre-connect DNS
and reachability errors within a bounded cause chain, and state that timeouts,
aborts, resets, socket closures, HTTP failures, and semantic upstream failures
remain account-scoped. Apply equivalent wording in
docs-site/src/content/docs/reference/configuration/providers.md lines 25-25,
docs-site/src/content/docs/ja/reference/configuration/providers.md lines 24-24,
docs-site/src/content/docs/ko/reference/configuration/providers.md lines 24-24,
docs-site/src/content/docs/ru/reference/configuration/providers.md lines 25-25,
and docs-site/src/content/docs/zh-cn/reference/configuration/providers.md lines
24-24, translating the qualifiers appropriately for each locale.
In `@src/lib/upstream-retry.ts`:
- Around line 32-53: Update the ACCOUNT_NEUTRAL_NETWORK_ERROR_CODES and
ACCOUNT_SCOPED_CONNECTION_ERROR_CODES sets to match Bun 1.3.14: retain ENOTFOUND
and the existing supported codes, and remove DNSResolveFailed,
DNSResolutionFailed, Timeout, Aborted, AbortedBeforeConnecting, and
ClientAborted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8aa7417e-a90c-4725-b3f6-750bc7b24b6d
📒 Files selected for processing (10)
docs-site/src/content/docs/ja/reference/configuration/providers.mddocs-site/src/content/docs/ko/reference/configuration/providers.mddocs-site/src/content/docs/reference/configuration/providers.mddocs-site/src/content/docs/ru/reference/configuration/providers.mddocs-site/src/content/docs/zh-cn/reference/configuration/providers.mdsrc/lib/upstream-retry.tssrc/server/responses/compact.tssrc/server/responses/core.tstests/codex-account-neutral-network.test.tstests/server-auth.test.ts
|
Reviewing this against a runtime probe, because I spent four audit rounds on the same problem in a planning unit and want to hand over what killed each attempt rather than have you rediscover it. Including But there is a consequence worth deciding deliberately. On Bun 1.3.14 a nonexistent hostname and a refused port are indistinguishable from the error object: Same code, same errno, no Two cases my audit found that are worth adding to your test matrix, both of which put a rejection on this path after the credential was seen:
One more, from the same audit: a transient 5xx that exhausts The analysis that produced all of this is in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/reference/configuration/providers.md`:
- Line 25: Update the upstreamFailoverThreshold documentation in
docs-site/src/content/docs/reference/configuration/providers.md:25-25,
docs-site/src/content/docs/ja/reference/configuration/providers.md:24-24,
docs-site/src/content/docs/ko/reference/configuration/providers.md:24-24,
docs-site/src/content/docs/ru/reference/configuration/providers.md:25-25, and
docs-site/src/content/docs/zh-cn/reference/configuration/providers.md:24-24 to
state that account-neutral pre-connect failures leave account quarantine,
cooldown, failure streak, thread affinity, rotation, and active-account
selection unchanged, while preserving the existing account-scoped transitions
for 401/403, 429, timeouts, resets, socket closures, HTTP failures, and semantic
upstream failures; provide equivalent wording in each locale.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 022e3281-6268-49d8-b09e-b0d908c5cdf4
📒 Files selected for processing (20)
docs-site/src/content/docs/ja/reference/configuration/providers.mddocs-site/src/content/docs/ko/reference/configuration/providers.mddocs-site/src/content/docs/reference/configuration/providers.mddocs-site/src/content/docs/ru/reference/configuration/providers.mddocs-site/src/content/docs/zh-cn/reference/configuration/providers.mdsrc/lib/upstream-retry.tssrc/providers/openai-sidecar.tssrc/server/images.tssrc/server/index.tssrc/server/live.tssrc/server/responses/core.tssrc/server/search.tssrc/server/ws-bridge.tssrc/vision/describe.tssrc/vision/index.tssrc/web-search/executor.tssrc/web-search/loop.tstests/codex-account-neutral-network.test.tstests/codex-sidecar-turn-lease.test.tstests/sidecar-abort.test.ts
| | `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 account-scoped transient outcomes before future new sessions fail over. Only exact pre-connect DNS and network reachability errors that the runtime classifies within a bounded cause chain are account-neutral and do not count. Timeouts, aborts, connection resets, socket closures, HTTP status failures, and semantic upstream failures remain account-scoped. Set `0` to disable. | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Document the complete account-neutral routing contract in every locale.
All five lines document only threshold exclusion. They must also state that account-neutral pre-connect failures leave account quarantine, cooldown, failure streak, thread affinity, rotation, and active-account selection unchanged. Preserve the existing account-scoped transitions for 401/403, 429, timeouts, resets, socket closures, HTTP failures, and semantic upstream failures.
docs-site/src/content/docs/reference/configuration/providers.md#L25-L25: add the complete guarantee to the canonical English wording.docs-site/src/content/docs/ja/reference/configuration/providers.md#L24-L24: add the equivalent Japanese wording.docs-site/src/content/docs/ko/reference/configuration/providers.md#L24-L24: add the equivalent Korean wording.docs-site/src/content/docs/ru/reference/configuration/providers.md#L25-L25: add the equivalent Russian wording.docs-site/src/content/docs/zh-cn/reference/configuration/providers.md#L24-L24: add the equivalent Simplified Chinese wording.
As per path instructions, localized provider documentation must stay synchronized with the full runtime routing behavior.
📍 Affects 5 files
docs-site/src/content/docs/reference/configuration/providers.md#L25-L25(this comment)docs-site/src/content/docs/ja/reference/configuration/providers.md#L24-L24docs-site/src/content/docs/ko/reference/configuration/providers.md#L24-L24docs-site/src/content/docs/ru/reference/configuration/providers.md#L25-L25docs-site/src/content/docs/zh-cn/reference/configuration/providers.md#L24-L24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/src/content/docs/reference/configuration/providers.md` at line 25,
Update the upstreamFailoverThreshold documentation in
docs-site/src/content/docs/reference/configuration/providers.md:25-25,
docs-site/src/content/docs/ja/reference/configuration/providers.md:24-24,
docs-site/src/content/docs/ko/reference/configuration/providers.md:24-24,
docs-site/src/content/docs/ru/reference/configuration/providers.md:25-25, and
docs-site/src/content/docs/zh-cn/reference/configuration/providers.md:24-24 to
state that account-neutral pre-connect failures leave account quarantine,
cooldown, failure streak, thread affinity, rotation, and active-account
selection unchanged, while preserving the existing account-scoped transitions
for 401/403, 429, timeouts, resets, socket closures, HTTP failures, and semantic
upstream failures; provide equivalent wording in each locale.
Source: Path instructions
lidge-jun
left a comment
lidge-jun
left a comment
There was a problem hiding this comment.
Thank you for this — the problem is real and your framing of it in #914 was accurate. But I have to ask for changes, and the reason is specific: this is the first of four designs that were already tried and rejected for this exact issue.
The full history is on dev at devlog/_plan/260803_transport_attribution/000_plan.md. Worth reading before the next revision, because it will save you the next two attempts as well.
The blocker
src/lib/upstream-retry.ts:32-43 allowlists Node codes plus Bun aliases including ConnectionRefused, and :140-182 walks the cause chain returning neutral on a match. That is "infer attribution from the rejected fetch's error code." Adding the Bun spellings makes the branch reachable — which the original design was not — but reachable is not correct.
A review probed Bun 1.3.14 against your classifier directly:
same .invalid host, 8 calls: ConnectionRefused → neutral=true
FailedToOpenSocket → neutral=false (alternating)
refused port: ConnectionRefused → neutral=true
server read Authorization: Bearer …, then 307 → refused port:
ConnectionRefused → neutral=true
first attempt 503, retry redirected to refusal:
ConnectionRefused → neutral=true
Three consequences:
- A real DNS outage still rotates the account. Bun evicts its DNS cache after a failure, so calls alternate between the two labels. Calls 2, 4, 6 return
FailedToOpenSocket, are not neutral, accumulate, and trip the threshold. The exact symptom #914 reports survives. - Credential-visible failures get suppressed. Bun follows redirects by default. A server that received your bearer and answered 307 to a dead host produces a final
ConnectionRefused— your classifier calls that neutral, but a credential-aware upstream can behave differently for account A than for B. That is a genuine account signal being discarded. - The
503 → rejectionhole.fetchWithTransientRetryreturns a transient 5xx only on the final attempt. An attributable 503 observed on attempt one vanishes when a later attempt rejects, and the rejection is then marked neutral.
Why the tests pass anyway
tests/codex-account-neutral-network.test.ts:153-256 injects hand-built errors — codedError("ENOTFOUND") and friends. That mirrors the allowlist rather than testing it. The suite is green because it asks the classifier the same question the classifier answers by construction; the runtime probe asks the question production asks, and gets a different answer.
This is not a criticism of your care — it is the specific trap this issue keeps setting, and the reason the recorded plan requires a runtime-grounded test against real Bun errors for any classifier here.
What the recorded conclusion suggests instead
Four designs died in the same place, which is evidence about the problem rather than about the designs. Every one tried to answer "was this the credential's fault?" from evidence that is insufficient in principle.
The direction the plan lands on is separating host health from account health: every pool account shares the host, so a network fault should mark the (provider, host) pair, not a credential, and rotation stops being the response at all. This PR does not do that — src/codex/routing.ts is untouched, there is no host key, threshold, or cooldown; it only skips account recording when the classifier fires.
If you want to take that on, it is genuinely valuable work and I would review it gladly.
Two smaller things
Scope. The sidecar quota-probe ownership work (OpenAiSidecarTurnLease, releaseProbeLease, the 387-line lifecycle suite) and the post-header outcome precedence changes in images.ts:445 / web-search/executor.ts:85 are separate policy questions — the latter is adjacent to #919, not required by #914. They would land more easily on their own.
Conflict. src/server/responses/compact.ts conflicts semantically, not textually: dev gained #913's bounded alternate-account send with per-context recording, and this PR edits the older single-account catch. Reconciling needs a decision about attribution for both contexts.
The documentation also states two things the implementation does not do: it calls the classified failures "pre-connect" (the rejection cannot prove that), and says aborts stay account-scoped while accountScopedTransportOutcome():198 makes them neutral. Same text in all four locales.
Leaving this open rather than closing it — the issue is real and you clearly have the context to solve it.
2 participants
Summary
Root cause
Regular Responses and native compact mapped every non-timeout transport exception to an account-specific
connect_error. A host-wide DNS outage therefore incremented a selected credential's failure streak and could clear affinity or rotate accounts even though every account used the same unreachable ChatGPT host.Pool-backed sidecars had the same policy gap plus an ownership problem: a probe lease could be acquired before the final consumer was known, released by an intermediate vision attempt before web-search, stranded on skipped/discarded paths, or followed by a stale outcome after cancellation. Some paths also recorded a known HTTP status and then recorded the subsequent body error a second time.
The classifier remains deliberately narrow. It trusts exact own-data
codeandcauseproperties through a bounded chain, and any abort, timeout, reset, or socket-close blocker wins unless the event is specifically the parent/client cancellation of that operation.User impact
An existing Codex task remains on its affined account during shared DNS/reachability failures and user cancellation. The request keeps its existing retryable error or timeout response, but OpenCodex no longer treats an account-neutral interruption as evidence that another credential is healthier.
Actual upstream
429,402,5xx, credential rejection, semantic failure, OpenCodex-owned timeout, and pre-header reset behavior remain account-scoped. This change does not replay a partially committed request.Validation
All contributor tests below used Bun
1.4.0-canary.1unless otherwise noted.bun test tests/codex-account-neutral-network.test.ts tests/codex-sidecar-turn-lease.test.ts tests/sidecar-abort.test.ts --timeout 15000bun test tests/server-combo-failover-e2e.test.ts tests/combo-child-headers.test.ts --timeout 15000bun test tests/server-search.test.ts tests/server-images.test.ts tests/server-live.test.ts --timeout 15000bun test tests/server-auth.test.ts --timeout 15000bun run typecheckbun run privacy:scangit diff --checknode_modules/bun/bin/bun install --frozen-lockfile && node_modules/bun/bin/bun run buildfromdocs-site/Fixes #914.
Related: #919 covers the distinct main Responses post-header mid-stream body failure; this PR keeps that separate from pre-connect classification and sidecar ownership.
Summary by CodeRabbit
Bug Fixes
Documentation
upstreamFailoverThresholdbehavior across supported languages, including which failures count and how to disable the threshold.