Repository navigation
Conversation
…ilures Thread affinity stayed pinned after connect_error/timeout while only new threads failed over, so continue in a dead session kept 502ing (#186). Soft- avoid the flaky account, clear affinities on failover, and force Connection: close on keep-alive reset retries.
![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: 51a27c1898
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Review pass: wire compact through connect-timeout + reset/5xx retries, record pool outcomes only after the body is buffered, and plumb thread affinity into sidecar outcome recording so continue/compact leave a flaky account.
Connection: close alone is unreliable in Bun (historically ignored). Use keepalive: false as the transport-level pool opt-out on reset retries, and keep Connection: close as a secondary signal.
|
Please retarget this PR to the |
Adversarial Review FeedbackThe soft-avoid concept is sound, and the keepalive:false recovery is correct for Bun 1.3.14. Blockers
Please retarget to |
|
Closing this PR — the soft-avoid concept has real value for transient failure recovery, but the implementation has two fundamental design issues that need a rework rather than incremental fixes:
The keepalive:false recovery for connection-reset retries is solid and we may adopt that piece separately. Thank you for the contribution — the soft-avoid direction is worth revisiting with proper status classification. 🙏 Co-authored-by: Wibias |
…atus (#205) * fix(codex): rework soft-avoid pool affinity with semantic terminal status Rework the transient-failure soft-avoid mechanism to address the two fundamental design issues identified in #194: 1. Non-transient failures no longer trigger soft avoidance — the terminal outcome recorder now uses the semantic HTTP status from logCtx.terminalHttpStatus (derived from the SSE error payload via httpStatusFromTerminalError) instead of collapsing every non-completed terminal to 502. A 400 invalid_request_error no longer soft-avoids the account or rebinds threads. 2. Late failures can no longer delete a newer healthy affinity — the threadId meta now guards the threadAccountMap.delete with an ownership check (bound?.accountId === accountId), preventing the race: T→A, A fails, T→B, late A failure must not delete B's mapping. Additional improvements: - Transport-level keepalive:false via applyUpstreamRecoveryInit for connection-reset retries (Bun has historically ignored the Connection: close header alone) - Compact endpoint (/v1/responses/compact) now uses the same transient-retry + recovery-init + pool-outcome recording as /v1/responses - Failover streak clears all affinities for the failing account (matching 429 behavior) - Soft-avoid extends on repeated transient failures and clears on 2xx (hard quota cooldown intentionally survives) - Affinity reuse now checks shouldFailover to catch post-avoid residual streaks Closes #194 * fix(codex): address Codex review — incomplete terminals, compact size, combo WS logCtx Three fixes from the Codex bot review on #205: 1. Incomplete terminals (max_output_tokens, content_filter, adapter EOF) no longer fall back to 502 and trigger soft-avoid. They are normal delivery outcomes — record success so a healthy account isn't penalized. 2. Compact endpoint: when upstream returns 200 but bufferCompactResponse produces a 502 (oversized body exceeding COMPACT_RESPONSE_MAX_BYTES), record the upstream status instead of connect_error. The account is healthy; the failure is a deterministic local payload-size limit. 3. Combo WS path: the terminal recorder closed over the child logCtx, but sendResponseToWebSocket inspects SSE payloads into the parent logCtx. The recorder now accepts an httpStatusOverride parameter, and the WS caller passes logCtx.terminalHttpStatus so the semantic status is not lost. Also properly types the WS logCtx as RequestLogContext.
…atus (lidge-jun#205) * fix(codex): rework soft-avoid pool affinity with semantic terminal status Rework the transient-failure soft-avoid mechanism to address the two fundamental design issues identified in lidge-jun#194: 1. Non-transient failures no longer trigger soft avoidance — the terminal outcome recorder now uses the semantic HTTP status from logCtx.terminalHttpStatus (derived from the SSE error payload via httpStatusFromTerminalError) instead of collapsing every non-completed terminal to 502. A 400 invalid_request_error no longer soft-avoids the account or rebinds threads. 2. Late failures can no longer delete a newer healthy affinity — the threadId meta now guards the threadAccountMap.delete with an ownership check (bound?.accountId === accountId), preventing the race: T→A, A fails, T→B, late A failure must not delete B's mapping. Additional improvements: - Transport-level keepalive:false via applyUpstreamRecoveryInit for connection-reset retries (Bun has historically ignored the Connection: close header alone) - Compact endpoint (/v1/responses/compact) now uses the same transient-retry + recovery-init + pool-outcome recording as /v1/responses - Failover streak clears all affinities for the failing account (matching 429 behavior) - Soft-avoid extends on repeated transient failures and clears on 2xx (hard quota cooldown intentionally survives) - Affinity reuse now checks shouldFailover to catch post-avoid residual streaks Closes lidge-jun#194 * fix(codex): address Codex review — incomplete terminals, compact size, combo WS logCtx Three fixes from the Codex bot review on lidge-jun#205: 1. Incomplete terminals (max_output_tokens, content_filter, adapter EOF) no longer fall back to 502 and trigger soft-avoid. They are normal delivery outcomes — record success so a healthy account isn't penalized. 2. Compact endpoint: when upstream returns 200 but bufferCompactResponse produces a 502 (oversized body exceeding COMPACT_RESPONSE_MAX_BYTES), record the upstream status instead of connect_error. The account is healthy; the failure is a deterministic local payload-size limit. 3. Combo WS path: the terminal recorder closed over the child logCtx, but sendResponseToWebSocket inspects SSE payloads into the parent logCtx. The recorder now accepts an httpStatusOverride parameter, and the WS caller passes logCtx.terminalHttpStatus so the semantic status is not lost. Also properly types the WS logCtx as RequestLogContext.
Summary
connect_error/timeout/ transient 5xx so thread affinity can leave a flaky peer on the next continue ([Bug]: Frequent 502 upstream_server_error after the first failure in a conversation. #186)x-codex-parent-thread-idinto outcome recording for immediate per-thread unbindConnection: closeon keep-alive connection-reset retries so Bun does not reuse the half-closed socketTest plan
bun test tests/codex-routing.test.ts tests/upstream-retry.test.tsbun test, typecheck, lint)Provider unreachable: socket connection was closed unexpectedlyin one Codex session, confirm continue rebinds to another account instead of sticky 502s