Repository navigation
fix(logs): keep the Codex WebSocket error wrapper's 5xx status and record the terminal frame kind - #6974
Conversation
…cord the terminal frame kind A bare upstream error frame with an unrecognized code reached request logs as a synthetic 502 upstream_server_error even when its WebSocket wrapper stated the status, which the native client maps directly. Adopt the wrapper's top-level status or status_code (one of the two) provisionally when it is 5xx; 4xx stays excluded because it would move credential and quota routing, and every 5xx is the same transient health class. Record terminalEvent and terminalErrorStatus on the content-free Codex WebSocket stage record so a row shows which terminal frame the upstream actually sent before any synthetic tail. Client terminals, retry and replay behavior are unchanged. Refs #6740
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to No actionable issue remains in the supplied evidence; the change is mergeable after normal checks. Pre-merge checks |
|
|
✅ Deterministic PR hygiene checks passed. |
Ingwannu
left a comment
There was a problem hiding this comment.
Source/contract review at 6fc5e22f90a7fd63b4fadabb001147ad38012c44 (PR base 8eb1fc3de365b019f9bacdcaacc6b7d284bd8d48; trusted current dev a0191a1b6f).
The change keeps the important separation: an unrecognized bare error can preserve an exclusive integer wrapper status/status_code only as a streaming log 5xx; known error classes keep precedence, a genuine terminal supersedes provisional status, and the client failed-tail/replay behavior is unchanged. Terminal-kind observation precedes relay-queue rejection, native-control records remain unobserved, and old/malformed persisted terminal fields normalize to null without dropping the rest of the stage. The added fixtures target those distinctions, not just the normal success path.
No concrete blocker found in these reviewed paths. I did not run the fixtures locally in this pass. Existing exact-head Cross-platform CI 38216613266 and target run 38216613252 succeeded; a later cancelled metadata run is not an additional successful check. No live Windows/backend capture, scan, replay or approval was performed.
Please continue to describe this as diagnostic fidelity, not resolution of #6740 or automatic recovery for #6969. The stage record is evidence for the next investigation, never a resend grant; the mapped docs correctly retain that restriction.
|
Maintainer integration into |
Summary
On the canonical Codex upstream WebSocket, the backend can end a turn with an
errorframe shaped like{"type":"error","status":503,"error":{...}}, sent afterresponse.created. The native Codex client uses thatstatusas the HTTP error status (codex-rs/codex-api/src/endpoint/responses_websocket.rs,map_wrapped_websocket_error_event). Unless the error code was a recognized overload or rate-limit class (#6889), OpenCodex dropped the status and logged a synthetic502 upstream_server_errorwithfailureCause=upstream-fault. That label hides whether the upstream refused the turn (503) or failed it (500). It is the row that #6740 has been collecting for days, and the reporter could not tell whether those 502s were the same admission refusal they saw as an SSE503 server_is_overloaded.errorframe with an unrecognized code now records the 5xx its wrapper states: a top-level integerstatusorstatus_code, one of the two and not both, which is the same rule as the pre-commit rejection projection. A wrapped 503 becomes503 / server_is_overloaded / upstream-declined, and a wrapped 500 becomes500 / upstream-fault. The status stays provisional, so a genuine terminal still replaces it. A wrapper 4xx is not adopted, because 401, 403, 402 and 429 would move credential and quota routing. Every 5xx is already the sametransientclass inclassifyCodexUpstreamOutcome, so account health, the fix(codex-pool): stabilize account health and reuse WS per thread under concurrency #6860 failure window and rotation do not change.codexWsStage.CodexWsStageRecordgainsterminalEvent, which records the normalizedresponse.completed/response.failed/response.incomplete/errorframe that ended a plain exchange before any synthetic tail. It also gainsterminalErrorStatus, the wrapper status of a terminalerrorframe. Both fields are content-free and closed. They are null on native-control exchanges and on legacy rows, and malformed persisted values read as null without dropping the rest of the stage. This is the "terminal event type as observed before any synthetic tail" that the maintainer triage on [Bug] Intermittent pooled Codex upstream response.failed 502 surge after v2.80 install (Windows; transport/egress isolated) #6740 asked reporters to collect.upstream_server_error. Buffered JSON callers keep their own presentation status, as documented.Docs:
structure/dashboard-and-usage.md,structure/transports/responses-wire-shapes.md,docs-site/.../reference/management-api.md. The plan and audit are indevlog/_plan/261012_6740_ws_error_status/.This makes the #6740 failures classifiable; it does not fix them. Source tracing points at an upstream
errorframe that OpenCodex relabels as 502, not a proxy transport failure. The upstream root cause still needs Windows rows from real accounts, and I did not reproduce it here.Refs #6740
Verification
bun test tests/responses/sse-failed-tail.test.ts tests/responses/ws-failure-stage.test.ts tests/usage/usage-log-ws-stage.test.ts tests/usage/upstream-failure-diagnostics.test.ts tests/responses/ws-upstream.test.ts tests/routing/combo-stream-preflight.test.ts tests/responses/ws-ambiguous-resend.test.ts: 413 pass, 1 skip, 0 fail.srcreverted, 8 new cases fail. The queue-overflow capture test fails with the earlier capture position, and the native-control test fails without the!nativeControlguard.bun run typecheck,bun run structure:check,bun run privacy:scanandbun scripts/file-size-ratchet.tsall pass.bun run test:changed(macOS, Bun 1.4.0; it selected 1,672 files becauseusage/log.tsandcodex-ws-wire.tsare widely imported): 36,512 pass, 107 skip, 37 fail and 3 unhandled errors, all in unrelated files: service install state, Lab live probe, live audio, native Codex/Grok toggles, restart replacement. Those 11 files pass on their own, 430/430, both with this change and on the unchanged base, so the failures depend on load from the concurrent full run. Exact-head CI covers the rest.status, and which value, remains to be observed.Checklist
Summary by CodeRabbit