fix(responses): classify Codex WS failures instead of restating them - #4232
Conversation
A long Codex thread that dies on the WebSocket transport reports one of two sentences, and neither distinguishes the cases it is true of: a socket that was never answered, a socket that carried only quota control frames, and a socket that died after the response was already flowing all produce the same text. The reporter of #4191 had to establish that much by toggling the proxy by hand. Add a content-free stage record to the exchange: create-frame byte count, whether the send completed, upstream frame count, how many of those the metadata channel claimed, Responses events relayed downstream, and the durations from send to first frame and to the failure. classifyCodexWsFailure reduces it to before-send, no-upstream-frame, no-response-event, or after-response-started, and the detail is appended after the existing message so the close-code tail stays one contiguous substring for every reader that matches on it. The frame is measured only when a failure message is being built, so the happy path never pays for sizing a multi-megabyte string. The classification is not a fallback-eligibility signal, and the no-replay-after- send contract is unchanged: a completed send may be executing upstream whatever the counters say. Refs #4191
|
✅ 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change adds Codex WebSocket failure-stage classification and diagnostic details. The exchange records frame, control, relay, and timing data. New tests cover classification, formatting, close handling, timeouts, and streamed-response failures. ChangesCodex WebSocket diagnostics
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Codex WebSocket
participant codex-ws-exchange
participant codex-ws-wire
participant SSE stream
Codex WebSocket->>codex-ws-exchange: Send frames and emit WebSocket events
codex-ws-exchange->>codex-ws-exchange: Record counters and timing
codex-ws-exchange->>codex-ws-wire: Create failure-stage snapshot
codex-ws-wire-->>codex-ws-exchange: Return cause and diagnostic suffix
codex-ws-exchange->>SSE stream: Report the classified failure
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This PR adds diagnostic detail to Codex WebSocket failure messages without changing behavior, fallback logic, or size/queue-limit handling. Review found no unresolved correctness, security, or data-exposure issues in the classification or rendering logic, and the new test coverage exercises the classifier stages and message formatting paths. The change is low risk and safe to merge from a code-correctness standpoint, with hosted CI serving as the remaining runtime validation since local checks were not run. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 3 files. (4 skipped: 4 unsupported.)
✨ 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 |
리뷰 · 우선순위 72 / 80이 PR은 Codex WebSocket이 중간에 죽을 때 사용자에게 보이던 두 문장( 구현은 레인 패킷이 정한 범위도 지켰습니다. 자동 HTTP/SSE 재전송, prelude 예산 늘리기(#3976), 16 MiB−64 KiB 아래 대역의 더 낮은 size preflight는 일부러 안 넣었고, 분류 결과를 fallback 자격으로 읽지 말라는 주석까지 타입 정의에 박아 두었습니다. 보내기 끝난 뒤에는 upstream이 이미 돌고 있을 수 있어서 테스트는 경로 경로 경로 경로 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
82b41d5 to
030316c
Compare
1 participant
Summary
A long Codex thread that dies on the WebSocket transport reports one of two sentences, and neither distinguishes the cases it is true of:
A socket that was never answered, a socket that answered in 40 ms with a quota frame and then produced nothing for 90 seconds, and a socket that died after the response was already flowing all reach the user as the same text. The reporter of #4191 established the little that was knowable by toggling the proxy by hand and observing that the same thread recovers immediately when OpenCodex is bypassed. Making that failure legible is what this change does.
The exchange now keeps a content-free stage record: the
response.createframe's byte count, whether the send completed, how many upstream frames arrived, how many of those the metadata channel claimed as quota or response metadata, how many Responses events were relayed downstream, and the durations from send to the first upstream frame and to the failure.classifyCodexWsFailurereduces that to one ofbefore-send,no-upstream-frame,no-response-event,after-response-started, and the detail is appended to the existing message:The detail is a suffix, after the close-code tail, so
(close 1006 Connection ended)remains one contiguous substring for every existing reader and assertion. The frame is measured only while a failure message is being built, so the happy path never pays for sizing a multi-megabyte string. Three paths carry it: the prelude timeout, the close-before-terminal message, and the transport error. The size- and queue-limit failures already name their own precise cause and are untouched.Every field is a size, a count, or a duration. No request body, header, close reason beyond what was already surfaced, or account identifier can reach a message built from this record.
A finding worth recording separately. Reading the exchange for this issue shows where the 90-second prelude budget actually goes:
preludeTimeris armed once after a successful send and is cleared only bycommitResponse(), which runs solely on a frame the metadata channel did not claim. So the budget measures send to the first Responses event, and upstream liveness on the control channel does not extend it. A socket that keeps sending quota updates while the backend works through a large replayed thread still dies at exactly 90 s, and before this change it reported the same sentence as a socket that was never answered.cause=no-response-eventwithcontrol=3 first-frame=41msis what that now looks like.Deliberately not implemented, per the lane packet, which scopes this issue to honest classification and returns the rest as a report:
failStreamtreats a completed send as possibly executing upstream. The maintainer comment on the issue is explicit thatresponseCommitted === falseand zero downstream bytes are not proof the upstream did not accept or execute the frame, so a resend gated on either can duplicate a turn. The new classification must not be read as a fallback-eligibility signal; the type comment says so at the definition, becauseno-upstream-frameis exactly the value a future reader would be tempted to misuse.codexWsCreateFrameExceedsLimitroutes at 16 MiB − 64 KiB; the band just under it still dials the socket, and a long full-replay thread sits there. Narrowing eligibility by predicted size or expected time-to-first-token changes transport selection for every user, not only failing ones. The byte count now in the failure message is what would supply the measurements for that decision.Design notes and the reproduction reasoning are in
devlog/_plan/260911_l6_streaming_tools/010_4191_ws_failure_classification.md.Refs #4191 — the report's remaining asks are answered above rather than patched, so the issue stays open.
Verification
bun run test,bun run test:changed,bun run typecheckandbun run build:gui: NOT RUN, by operator instruction for this dispatch round. Hosted CI on the exact pushed head is the only product evidence this change offers.tests/responses/ws-failure-stage.test.ts, in theresponsesdomain besidews-upstream.test.ts, registered inscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json. It covers the classifier's four stages, the renderer's exact output including then/adurations, the contiguity of the close-code tail that existing assertions depend on, and four end-to-end cases through a fake socket: an unanswered close, a quota-only close, a close after relayed events, and the prelude timeout under fake timers.tests/responses/ws-upstream.test.ts(lines 1061, 1281, 1542-1543, 1556) was checked by reading each one: all four match on substrings or unanchored regexes that end at or before the close-code tail, and the detail is appended after it.Checklist
Summary by CodeRabbit
Bug Fixes
Tests