fix(codex): isolate provider host transport health - #922
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds Codex upstream-host health tracking, admission leases, circuit breaking, and retry-attempt observations. Standard and compact Responses separate transport outcomes from account failures. Tests and localized documentation cover redirects, timeouts, retries, replay restrictions, and host-level failure handling. ChangesCodex upstream host health
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Responses
participant HostHealth
participant Upstream
participant AccountPool
Client->>Responses: send Codex request
Responses->>HostHealth: acquire host admission lease
HostHealth-->>Responses: allow or block request
Responses->>Upstream: execute request and retries
Upstream-->>Responses: response or transport observation
Responses->>HostHealth: record host response or failure
Responses->>AccountPool: record account outcome when applicable
Responses-->>Client: return upstream response or 502
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
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.
399a991 to
2b46044
Compare
luvs01
changed the title
|
@lidge-jun @Wibias @Ingwannu This is now rewritten on current Fresh exact-head validation: focused runtime/routing tests 97/97 (887 assertions), host-only affinity contracts 3/3 (23 assertions), typecheck, privacy scan, diff check, and a 216-page docs build. Ready for re-review. |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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: Synchronize the upstreamFailoverThreshold contract across
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 with
runtime behavior: describe the bounded-cause-chain pre-connect classifier
including ConnectionRefused as host-neutral, keep read-then-close ECONNRESET
account-scoped when authenticated data may have been consumed, remove blanket
no-response wording, restrict host updates to exact classified outcomes, and
state that the 503 evidence rule applies only to a later host-neutral rejection.
In `@src/codex/upstream-host-health.ts`:
- Around line 90-119: Update oldestNonLeasedKey and its callers makeRoom and
pruneOverflow to accept the current now value and prefer eviction candidates
without an active cooldownUntil before comparing lastTouchedAt. Preserve leased
entries and the existing capacity-bound behavior, while retaining cooldown
entries whenever any non-cooldown, non-leased entry is available.
- Around line 235-258: Update recordCodexUpstreamHostFailure and the
corresponding reset/delete path to preserve the current generation and remaining
activeLeaseIds when resetting host health, removing only the completed lease
instead of replacing or deleting state needed by concurrent requests. Ensure
subsequent transport failures from remaining leases still match and record
evidence, and add a test covering an observed response followed by a concurrent
transport failure.
In `@src/server/responses/core.ts`:
- Around line 1792-1801: Release the auth-context probe lease for every
transport-only failure by changing the host-lease-guarded branches in
src/server/responses/core.ts:1792-1801 and
src/server/responses/compact.ts:476-485 to run releaseCodexAuthContextProbeLease
whenever observedStatus is undefined, regardless of host lease presence. Add a
focused regression test near the existing response subsystem tests covering a
pool-authenticated forward request with a null canonical host key and
pre-response rejection, asserting the account probe lease is free afterward.
In `@tests/codex-host-health-runtime.test.ts`:
- Around line 323-337: Replace the DNS-dependent invalidOrigin used by the
installCanonicalRouter callback with a deterministic refused loopback origin
from closedEphemeralPort(), matching the setup used by other tests in this file.
Keep the actualFetch error capture and existing circuit-breaker assertions
unchanged, including the distinction from canonicalHostKey.
- Around line 142-169: Add an `error` listener that ignores expected socket
errors to each tracked socket in the `createServer` connection callback,
alongside the existing `data` and `close` listeners. Ensure sockets destroyed by
the request handler or cleanup of `trackedSockets` cannot emit unhandled errors
during tests.
In `@tests/codex-upstream-host-health.test.ts`:
- Around line 36-45: Extend the existing normalized host-key test around
canonicalCodexUpstreamHostKey to cover trailing-dot removal, IPv6 bracket
normalization/round-tripping, and implicit :80 equivalence for HTTP URLs. Import
isCodexUpstreamRedirectStatus and add focused assertions covering its redirect
and non-redirect outcomes. Keep the tests adjacent to the existing upstream host
health cases.
In `@tests/issue-452-empty-503.test.ts`:
- Around line 194-205: Update the fetch interceptor in installCodexTransport to
be async, ensuring synchronous throws from send(init) become promise rejections
while preserving the existing routing behavior for non-Codex requests.
- Around line 261-276: Update the circuit-opening test around
sendRegularPoolRequest to import and use CODEX_UPSTREAM_HOST_FAILURE_THRESHOLD
for the failure loop bound and expected sends count, replacing both hardcoded 3
values so the test remains aligned with the exported threshold.
🪄 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: 3a99c21e-814f-4718-980d-8293077aa951
📒 Files selected for processing (12)
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/codex/upstream-host-health.tssrc/lib/upstream-retry.tssrc/server/responses/compact.tssrc/server/responses/core.tstests/codex-host-health-runtime.test.tstests/codex-upstream-host-health.test.tstests/issue-452-empty-503.test.ts
2b46044 to
92dd06c
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
tests/codex-host-health-runtime.test.ts (1)
326-336: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe circuit test still depends on DNS resolution of a
.invalidname.Line 326 keeps
http://same-host-health-target.invalid. A resolver that synthesizes wildcard A records for NXDOMAIN returns an address,actualFetchat line 331 then returns an HTTP response, and no host failure is recorded. Lines 344-347 and 352 then fail with no indication that DNS caused the failure. Every other test in this file usesclosedEphemeralPort(), which refuses deterministically without DNS.Use a refused loopback origin here too. The refused loopback key still differs from
canonicalHostKey, so lines 353-355 remain valid.♻️ Proposed change to a deterministic refused origin
- const invalidOrigin = "http://same-host-health-target.invalid"; + const refusedPort = await closedEphemeralPort(); + const invalidOrigin = `http://127.0.0.1:${refusedPort}`;🤖 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 `@tests/codex-host-health-runtime.test.ts` around lines 326 - 336, Replace the DNS-dependent invalidOrigin in the circuit test with a refused loopback origin obtained from closedEphemeralPort(), matching the deterministic pattern used elsewhere in the file. Keep the resulting host key distinct from canonicalHostKey so the existing assertions around lines 353-355 remain valid.
🤖 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 state that
thresholded account failures can trigger failover for subsequent requests,
including requests from existing bound tasks, by clearing thread affinity and
changing active-account routing; do not limit the behavior to future new
sessions. Apply the corresponding translated wording in
docs-site/src/content/docs/ja/reference/configuration/providers.md:24,
docs-site/src/content/docs/ko/reference/configuration/providers.md:24,
docs-site/src/content/docs/ru/reference/configuration/providers.md:25, and
docs-site/src/content/docs/zh-cn/reference/configuration/providers.md:24, while
updating docs-site/src/content/docs/reference/configuration/providers.md:25.
In `@src/server/responses/core.ts`:
- Around line 1860-1873: Both retry paths incorrectly use per-attempt executor
flags to classify failures across the entire retry ladder. In
src/server/responses/core.ts lines 1860-1873, require attemptHistory.length ===
0 alongside !primaryAttemptExecutorStarted; in src/server/responses/compact.ts
lines 520-540, require primaryAttempts.length === 0 alongside
!primaryAttemptBoundary.executorStarted. Add a focused regression test near the
existing response subsystem tests that rejects the first physical attempt, fails
before executor start on retry, and verifies host health records the failure.
- Around line 459-472: Use the same hostKey-based condition for the
alternate-account request’s redirect mode and its redirect-to-502 guard. Thread
the caller’s boolean through CodexPoolAccountRetryArgs, apply it when setting
redirect in the alternate send, and reuse it in the guard so uncanonicalizable
upstreams continue following redirects as before.
In `@tests/codex-host-health-runtime.test.ts`:
- Around line 120-133: The tests must bind their server fixtures before
reserving the refused port so those servers cannot reclaim it. Update the setup
around the tests using closedEphemeralPort() to start each serve(..., port: 0)
fixture first, then call closedEphemeralPort(), preserving the intended
connection-rejection assertions.
In `@tests/responses-compaction-routing.test.ts`:
- Around line 1675-1694: Update the test around the “an open compact host
circuit returns Retry-After without another send” case to use the imported
CODEX_UPSTREAM_HOST_FAILURE_THRESHOLD for both the failure-attempt loop bound
and the expected sends count, replacing the hardcoded 3 while preserving the
existing blocked-request assertions.
- Around line 1141-1190: Update the test containing “compact settles A's
half-open account probe when response header processing throws” to isolate
credential persistence using the existing temporary-home pattern from
neighboring tests, such as withPoolEnv or explicit OPENCODEX_HOME/CODEX_HOME
setup and restoration. Ensure saveCodexAccountCredential writes only to the
temporary directory while preserving the test’s current assertions and cleanup.
- Around line 772-802: Update throwingPreExecutorHeaders to define a
BOUNDARY_FRAME constant and track whether the fetchWithHeaderTimeout stack frame
is observed during header cloning. Assert that the frame was observed before the
affected tests assert rejects.toBe(expected), so stack-format or
function-renaming changes fail with an explicit contract error rather than a
misleading rejection failure.
In `@tests/server-auth.test.ts`:
- Around line 2604-2630: Introduce a descriptive constant near the response
proxy setup for the expected pre-alternate-preparation response.headers read
count, documenting that it mirrors the access sequence in core.ts. Use this
constant for both the throw condition and the headerReads assertion, preserving
the existing value and test behavior while making the coupling explicit.
---
Duplicate comments:
In `@tests/codex-host-health-runtime.test.ts`:
- Around line 326-336: Replace the DNS-dependent invalidOrigin in the circuit
test with a refused loopback origin obtained from closedEphemeralPort(),
matching the deterministic pattern used elsewhere in the file. Keep the
resulting host key distinct from canonicalHostKey so the existing assertions
around lines 353-355 remain valid.
🪄 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: 255e1afd-d815-47da-9a53-20ebe46b00b5
📒 Files selected for processing (15)
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/codex/upstream-host-health.tssrc/lib/upstream-retry.tssrc/server/responses/compact.tssrc/server/responses/core.tstests/codex-host-health-runtime.test.tstests/codex-upstream-host-health.test.tstests/issue-452-empty-503.test.tstests/responses-compaction-routing.test.tstests/server-auth.test.tstests/upstream-transient-retry.test.ts
|
Updated comparison with #966 for current #922 head Both proposals share the important invariants: shared reachability failures must not penalize or rotate a pool account; credential-bearing redirects must not be followed automatically; prior HTTP evidence must survive a later rejection; discarded retry bodies must be cancelled; and probe settlement must respect lease ownership. #922 explicitly credits and incorporates the strongest evidence from #966:
The remaining differences are intentional:
#966 is currently an independent draft at |
[GD] Verdict: changes-requestedTLDR
Full verdictSemantic propagation
Linked: UsefulnessUseful. Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
Simplification (for the PR owner)Foreign PR: candidates only; nothing edited or pushed.
No broad dedup of core/compact settlement is recommended in this PR. Gate
Bottom lineShip direction is right: isolate shared host reachability from account health, fence half-open admission, preserve ordered evidence, keep redirects manual for pooled Codex sends. Do not merge yet. @luvs01 should rebase/update onto current |
92dd06c to
d6c3734
Compare
|
@Wibias Current head
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6c373439e
ℹ️ 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 account-scoped transient HTTP or semantic failure outcomes required before subsequent requests, including existing bound tasks, may fail over; `0` disables only account failover. A terminal logical request that rejects before any HTTP status is observed updates only process-local host health keyed by `(provider, canonical HTTP(S) origin)` and releases any account probe lease; it does not change account quarantine, account cooldown/failure streak, affinity, pool rotation, or active account selection. Three terminal logical host failures within five minutes open the host circuit for 30 seconds; then exactly one half-open logical request is admitted while concurrent requests remain blocked. An observed HTTP failure status remains account evidence. If a `503` response is followed by a rejection, ordered evidence retains the `503` for the account and the later rejection for the host. Codex bearer redirects for pooled regular Responses and native compact requests are handled manually: they are not followed, `Location` is not exposed, and they become a bounded account-scoped `502`. If the peer may have consumed a credential-bearing request but the transport rejects before any HTTP status is observed, the request is not replayed under another credential; that terminal rejection can contribute to a host circuit that temporarily blocks an otherwise healthy alternate. | |
There was a problem hiding this comment.
When a pooled OpenAI vision or web-search sidecar rejects before returning an HTTP status, this unconditional guarantee is still false: src/web-search/executor.ts:96-98 and src/vision/describe.ts:116-118 report connect_error/timeout, and src/providers/openai-sidecar.ts:160-168 forwards that outcome to recordCodexUpstreamOutcome, which increments the selected account's transient-failure streak and can clear affinity or rotate it. Fresh evidence in the reviewed tree is that these catches and callbacks remain unchanged despite the earlier thread saying they were addressed. Apply the host-only classifier and probe-lease settlement to these pool-backed sidecars, or explicitly limit the documentation to pooled regular Responses and native compact requests.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
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 describe
host-only attribution only for exact bounded-chain pre-connect DNS/reachability
failures, including ConnectionRefused for nonexistent hosts and refused ports.
Explicitly keep timeouts, aborts, ECONNRESET, socket closures, HTTP failures,
semantic failures, and read-then-close ECONNRESET account-scoped, while
retaining no-replay credential protection and the rule that 503 evidence is
followed only by a later host-neutral rejection. Apply synchronized wording 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,
translating the classifier and negative cases appropriately.
In `@tests/codex-host-health-runtime.test.ts`:
- Around line 349-351: Replace the vacuous runtimeErrorLabel/every assertion
with an assertion that runtimeErrors.length equals physicalSends.length (using
the existing threshold relationship) in the test around physicalSends. Preserve
the label-shape check only if needed for its documented tolerance, and ensure
the rejection-count assertion identifies missing resolver rejection before the
cooldown assertion.
🪄 Autofix
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: 23584a92-0d4e-44c9-8477-7ea20ff821fb
📒 Files selected for processing (17)
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/codex/routing.tssrc/codex/upstream-host-health.tssrc/lib/upstream-retry.tssrc/server/responses/compact.tssrc/server/responses/core.tstests/codex-host-health-runtime.test.tstests/codex-upstream-host-health.test.tstests/helpers/isolated-codex-home.tstests/issue-452-empty-503.test.tstests/responses-compaction-routing.test.tstests/server-auth.test.tstests/upstream-transient-retry.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 HTTP or semantic failure outcomes required before subsequent requests, including existing bound tasks, may fail over; `0` disables only account failover. A terminal logical request that rejects before any HTTP status is observed updates only process-local host health keyed by `(provider, canonical HTTP(S) origin)` and releases any account probe lease; it does not change account quarantine, account cooldown/failure streak, affinity, pool rotation, or active account selection. Three terminal logical host failures within five minutes open the host circuit for 30 seconds; then exactly one half-open logical request is admitted while concurrent requests remain blocked. An observed HTTP failure status remains account evidence. If a `503` response is followed by a rejection, ordered evidence retains the `503` for the account and the later rejection for the host. Codex bearer redirects for pooled regular Responses and native compact requests are handled manually: they are not followed, `Location` is not exposed, and they become a bounded account-scoped `502`. If the peer may have consumed a credential-bearing request but the transport rejects before any HTTP status is observed, the request is not replayed under another credential; that terminal rejection can contribute to a host circuit that temporarily blocks an otherwise healthy alternate. | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the exact runtime classifier as the account-neutral boundary.
All five entries classify any terminal rejection before an HTTP status as host-only. Runtime behavior limits host-neutral handling to exact pre-connect DNS/reachability failures found within a bounded cause chain, including ConnectionRefused for nonexistent hosts and refused ports. Timeouts, aborts, ECONNRESET, socket closures, HTTP failures, and semantic failures remain account-scoped. A read-then-close ECONNRESET remains account-scoped when authenticated data may have been consumed. The no-replay control protects credentials but does not change failure attribution. Keep the 503 rule limited to a later host-neutral rejection.
docs-site/src/content/docs/reference/configuration/providers.md#L25-L25: update the canonical English wording with the exact pre-connect classifier and account-scoped negative cases.docs-site/src/content/docs/ja/reference/configuration/providers.md#L24-L24: translate the same classifier and negative cases into Japanese.docs-site/src/content/docs/ko/reference/configuration/providers.md#L24-L24: translate the same classifier and negative cases into Korean.docs-site/src/content/docs/ru/reference/configuration/providers.md#L25-L25: translate the same classifier and negative cases into Russian.docs-site/src/content/docs/zh-cn/reference/configuration/providers.md#L24-L24: translate the same classifier and negative cases into Simplified Chinese.
As per path instructions, keep the English provider contract and all translated locale pages synchronized with actual runtime 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 to describe host-only
attribution only for exact bounded-chain pre-connect DNS/reachability failures,
including ConnectionRefused for nonexistent hosts and refused ports. Explicitly
keep timeouts, aborts, ECONNRESET, socket closures, HTTP failures, semantic
failures, and read-then-close ECONNRESET account-scoped, while retaining
no-replay credential protection and the rule that 503 evidence is followed only
by a later host-neutral rejection. Apply synchronized wording 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,
translating the classifier and negative cases appropriately.
Source: Path instructions
| // Bun 1.3.14 on Windows has emitted more than one label for this same target. | ||
| // Activation correctness intentionally depends only on real rejection count. | ||
| expect(runtimeErrors.map(runtimeErrorLabel).every(label => label.length > 0)).toBe(true); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
This assertion cannot fail, so it does not protect the .invalid fixture.
Line 351 maps runtimeErrors through runtimeErrorLabel and asserts every label has a non-zero length. Two properties make the check vacuous. Array.prototype.every returns true for an empty array, so an empty runtimeErrors passes. runtimeErrorLabel always returns at least "::" from the template on line 278, so any element passes too.
The failure mode this matters for is the one the retained .invalid fixture exists to exercise. If a resolver synthesizes an address for same-host-health-target.invalid, actualFetch at line 331 resolves, runtimeErrors stays empty, and line 351 still reports success. The test then fails later at line 352 with a missing cooldown, which does not name the resolution as the cause.
Assert the rejection count instead. physicalSends already equals the threshold at line 347, so every physical send must have rejected for the circuit to open.
♻️ Proposed change to make the rejection count the contract
// Bun 1.3.14 on Windows has emitted more than one label for this same target.
// Activation correctness intentionally depends only on real rejection count.
- expect(runtimeErrors.map(runtimeErrorLabel).every(label => label.length > 0)).toBe(true);
+ expect(runtimeErrors).toHaveLength(CODEX_UPSTREAM_HOST_FAILURE_THRESHOLD);
+ expect(runtimeErrors.map(runtimeErrorLabel).every(label => label.length > 0)).toBe(true);This keeps the label-shape tolerance the comment describes, and it names the resolution failure at the line that observes it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Bun 1.3.14 on Windows has emitted more than one label for this same target. | |
| // Activation correctness intentionally depends only on real rejection count. | |
| expect(runtimeErrors.map(runtimeErrorLabel).every(label => label.length > 0)).toBe(true); | |
| // Bun 1.3.14 on Windows has emitted more than one label for this same target. | |
| // Activation correctness intentionally depends only on real rejection count. | |
| expect(runtimeErrors).toHaveLength(CODEX_UPSTREAM_HOST_FAILURE_THRESHOLD); | |
| expect(runtimeErrors.map(runtimeErrorLabel).every(label => label.length > 0)).toBe(true); |
🤖 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 `@tests/codex-host-health-runtime.test.ts` around lines 349 - 351, Replace the
vacuous runtimeErrorLabel/every assertion with an assertion that
runtimeErrors.length equals physicalSends.length (using the existing threshold
relationship) in the test around physicalSends. Preserve the label-shape check
only if needed for its documented tolerance, and ensure the rejection-count
assertion identifies missing resolver rejection before the cooldown assertion.
Source: Learnings
[GD] Verdict: changes-requestedTLDR
Full verdictSemantic propagation
Linked: UsefulnessUseful. Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
Simplification (for the PR owner)Foreign PR: candidates only; nothing was edited or pushed.
Gate
Bottom lineThe redesign is right and the previous round's blockers are genuinely fixed: reject-then-pre-exec evidence is preserved (regular + compact regressions), five-locale failover wording is corrected, temp-home isolation is in place, alternate redirects match the primary nullable-host rule, and thresholds are named constants. Do not merge yet: scope the docs guarantee to the covered paths in all five locales, fix the vacuous assertion, decline the stale classifier thread with runtime evidence, get |
3 participants
Summary
503 -> rejection, across regular Responses and native Compact without changing the fix(compact): try one alternate account on a pool 429/402, and keep the backoff headers #927 primary ladder or single-send alternateLocation, and never replay a credential-consuming no-status rejection under another accountRoot cause
The pool previously had only account-scoped transport health. A terminal rejection before any HTTP status therefore looked like evidence against the selected credential even when every account shared the same unreachable origin. Runtime error labels are not a reliable ownership boundary: Bun can expose DNS and refused-port failures with the same label, and timeout, redirect, read-close, and retry histories add further ambiguity.
This revision uses an independent process-local host ledger keyed by provider and canonical HTTP(S) origin. Admission leases carry a generation so stale completions cannot close or reopen a newer circuit, active half-open leases cannot be evicted, and concurrent ordinary leases settle without invalidating siblings.
Behavior and scope
401precedence, then checks host admission before dispatch.502withRetry-After; after cooldown, exactly one logical request owns the half-open lease.Verification
Official-runtime checks used Bun
1.3.14+0d9b296afon exact headd6c373439e3c12ad595cfb14e70e609419695980, based ondevate44d234f08e03dd4dbf0c4aa13af43046d86b0a6.bun run typecheckbun run privacy:scangit diff --checkbun run buildfromdocs-site/devFull-suite environment note
The official Bun 1.3.14 full suite stopped with Bun's own internal assertion in
api-storage-policy-run.test.ts. The same single-file panic and report URL reproduced on clean currentdev, so it is not a branch-only product failure.As supplemental evidence, installed
1.4.0-canary.1+5f65d3785completed the full traversal (8,215 pass, 6 skip, 43 fail, 2 errors) but ran about 2,480s versus the runner's normal ~210s. Its Windows ACL, EBUSY, and timeout failure classes reproduced on cleandev; the #922 files that appeared in the loaded full run passed when isolated. The canary result is not presented as the official project runtime.Checklist
Fixes #914.
Related: #919 and #915 cover separate post-header and recovery-probe policies. #966 remains an overlapping alternative; the accepted evidence and intentional design differences are recorded in the comparison comment.
Summary by CodeRabbit
Bug Fixes
Documentation
upstreamFailoverThresholdbehavior and failure classification across supported languages.