Repository navigation
fix(codex-pool): stabilize account health and reuse WS per thread under concurrency - #6860
Conversation
…er concurrency Track transient Codex failures as a 60-second ratio so concurrent completions can steer new threads without dropping a bound thread or a manual pin. Cross-turn WebSocket reuse stays off unless it is explicitly enabled. Co-authored-by: JUN <jun@lidgeai.com>
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing (27)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: c9e49e7162
ℹ️ 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".
| if (reuseAcrossTurns && signal?.aborted) { | ||
| return Promise.reject(signal.reason ?? new DOMException("The operation was aborted.", "AbortError")); | ||
| } |
There was a problem hiding this comment.
When a busy socket becomes idle, changed() reserves it before resolving acquireWaiting(). If the request is aborted after that resolution but before this async continuation runs, this branch rejects without releasing or disposing pooled. The entry consequently remains permanently busy, has no idle-expiry timer, and every later request for the same identity waits and then opens a throwaway socket until shutdown. Release/dispose the acquired session before rejecting, or transfer it to the exchange so the normal abort cleanup owns it.
Useful? React with 👍 / 👎.
| state.samples.push({ at: now, failed }); | ||
| if (state.samples.length > MAX_SAMPLES) state.samples.splice(0, state.samples.length - MAX_SAMPLES); |
There was a problem hiding this comment.
Once an account records more than 512 terminal outcomes per minute, this truncation discards samples that are still inside the advertised 60-second window. For example, 500 failures followed by 512 successes in the same minute are treated as a 0% failure ratio rather than roughly 49%, allowing the account to recover incorrectly after the sustain period; reversing completion order produces the opposite result. Use bounded time buckets or aggregate counts so memory stays bounded without making high-concurrency health depend on the last 512 completions.
Useful? React with 👍 / 👎.
| const headers = sanitizePassthroughHeaders(upstreamResponse.headers, codexSafetyBufferingOptions); | ||
| if (!upstreamResponse.ok) noteUpstreamRequestId(logCtx, headers); |
There was a problem hiding this comment.
For a streaming response accepted with HTTP 200 that later emits response.failed, this condition is false at the only point where the upstream response headers are available. The SSE inspector subsequently records the terminal error code and status, but it cannot recover the header request ID, so the new diagnostics omit openai-request-id/x-request-id for this common failure mode. Store the bounded ID for successful streaming handshakes as well, while deferring the warning until a failure is actually observed.
Useful? React with 👍 / 👎.
Summary
codexPinnedTransientPolicytodetour-new-threadsto place only new threads elsewhere.codexFailureWindow: falserestores consecutive-only steering.codexWsReuseAcrossTurns(defaultfalse) retains one Codex WebSocket per account and thread across turns, waits at most 750ms for a busy socket before dialing another, keeps up to two sockets when the model or service tier changes, and leaves turn-state headers on each frame instead of the handshake. Authorization, account, proxy, or dial-URL changes still retire the old socket.error.codeandopenai-request-id/x-request-idon the request log. The log line does not include the response body. HTTP 5xx (and failures with no status yet) also print that line.Refs #6740
Plan
The consecutive counter in
src/codex/routing.tstreats interleaved successes and failures as a streak, so concurrent completions on one account can hide a high failure rate or trip failover from ordering alone. The window stores timestamped success and transient-fault samples and computesfailed / samples. Quota and caller errors are not samples. Bound threads keep consultingshouldFailover, which stays consecutive-only, so the window cannot move an in-flight conversation. Unbound selection consultsfailureWindowSteersNewThreads.WebSocket reuse today includes
turn_idin the pool scope and copiesx-codex-turn-state/x-codex-turn-metadataonto the upgrade. A busy socket returns immediately and the caller dials a throwaway. Those changes are gated bycodexWsReuseAcrossTurnsso the default path matches current behavior, including the plugin test that the handshake keeps the original turn-state header.Audit
devwas still006cec99f, the same commit the design was checked against.CODEX_FAILURE_WINDOW_MSis the 5-minute consecutive-streak gap, so the ratio window uses its own 60-second constant.shouldFailoverwas not widened: the affinity path uses it to detour a bound thread, which this change must not do.src/codex/routing.tsis at its file-size cap, so the success-streak bookkeeping moved intosrc/codex/routing/failure-window.tsand the routing file shrank.src/server/request-log.tsandscripts/test-layout/layout.jsonwere within a few lines of the 2000-line scan, so the diagnostic helper and the new layout keys were kept inside that budget. The reuse flag is published fromwarnDegradedTopLevelOptInson a successful config load rather than from schema parsing, so a partial salvage parse cannot clear an operator opt-in.No part of the requested scope was dropped as unsafe. The dashboard does not yet show a degraded badge; the pin warning is the proxy log line. In-flight turns were already left on their captured account, and this change does not add a resend after send.
Verification
bun test tests/codex-integration/codex-failure-window.test.ts tests/responses/codex-ws-pool-contention.test.ts tests/usage/upstream-failure-diagnostics.test.ts— 15 passbun test tests/codex-integration/codex-routing.test.ts tests/usage/request-log.test.ts tests/config/protocol-settings.test.tsplus the three new files — 288 pass, 1 skip, 0 failbun test tests/responses/ws-upstream-reuse.test.ts tests/lib/plugin-upstream-hooks.test.ts tests/test-layout.test.ts— passbun run typecheck— passbun run structure:check— passbun run privacy:scan— passbun scripts/file-size-ratchet.ts— passbun testwas not run locally. The remaining coverage is CI.tests/test-layout-tooling.test.tshas one environment failure here: the VM's default Bun is 1.4.2 and that case requires the pinned 1.4.0 runner. The layout membership oracle in the same file passed.Checklist