fix(codex): keep oversized Responses turns off the WS transport - #2473
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change wraps provider HTTP fetches, enforces a UTF-8 byte limit for Codex WebSocket create frames, routes oversized requests through HTTP SSE, and reports upstream WebSocket close details. ChangesCodex WebSocket safety and fallback
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to The change routes oversized request frames to SSE and preserves close diagnostics and protocol settings. The current head is not merge-ready because the full test run was not green and readiness remains unchecked; one failure reproduces on the base branch and the other passed in isolation, so this is a readiness risk rather than an identified functional regression. Sequence Diagram(s)sequenceDiagram
participant Client
participant wsUpstream
participant providerFetch
participant HTTP_SSE
participant WebSocket
Client->>wsUpstream: submit response.create request
wsUpstream->>wsUpstream: measure UTF-8 frame size
alt frame exceeds configured limit
wsUpstream->>providerFetch: use wrapped HTTP fetch
providerFetch->>HTTP_SSE: send HTTP SSE request
else frame fits configured limit
wsUpstream->>WebSocket: open WebSocket connection
WebSocket-->>wsUpstream: close event with code and reason
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
리뷰 · 우선순위 63 / 80설명: 이 풀 리퀘스트는 2426 을 고친다. 코덱스 위 웹소켓이 열여섯 메가 이상 장을 1009 로 닫고, 열린 뒤에는 에스에스로 안 돌아가서 긴 일이 502 로 영원히 죽는 구멍이다. 작성자는 올드동키다. 오늘 열다섯 시 서른여섯 분에 열렸다. 베이스는 지금 HEAD 지금 HEAD 를 열었다. src/server/responses/ws-upstream.ts 82줄 이 PR 이 하는 일은 예전 리뷰가 말한 첫 바퀴와 같다. 125줄에서 장을 만든 뒤에, 소켓을 열기 전에 장 바이트를 잰다. 한도는 열여섯 메가에서 예순넷 키로를 뺀 값이다. 산 측정에서 16777000 은 살고 16777300 은 죽었다. 이 한도는 그 아래다. 넘으면 소켓을 안 열고 처음부터 에스에스다. 유티에프8 바이트로 잰다. 글자 수로 안 잰다. 닫힘 코드 1009 는 이제 큰 장 거절이라고 적는다. 다른 코드도 번호가 붙는다. fetch-helpers 는 실패 대체를 핀이 있는 에이치티티피 fetch 로 바꾼다. 시험 여덟이 더해진다. 큰 장은 소켓을 안 연다. 맞는 장은 위를 탄다. 1009 문장과 1006 문장을 잠근다. 범위는 맞다. 그림 줄이기와 턴 안 증분은 안 넣었다. 1009 에서 계정 회전을 막는 것도 안 넣었다. 그건 다음 기차가 맞다. 크기 가드를 사용자 길이로는 긴 일이 열여섯 메가에서 영원히 502 로 끝나서 63. 초안이라 지금 착지하면 안 된다. 2471 은 같은 구멍의 닫힌 복사본이다. 다시 열지 말 것. 산 추적은 2426 이다. 카탈로그 팁은 Ox Alpha x-preview-f-free + deepseek-v4-flash-vision-exp. Cursor 정적 카탈로그는 opus-4-8-fast / opus-5-fast. 2334 CursorCredentialRouter 는 여전히 src/providers/cursor-pool.ts 모듈+테스트만 있고 어댑터에 연결되지 않았다. 2332 H2 는 discovery 전용. 2320 overflow + 2342 는 이미 dev. 2188 사이드카는 이미 dev. 2361 src/server/responses/ws-upstream.ts 라인 82 - HEAD 의 shouldUseCodexWsUpstream 은 본문 크기를 안 본다. 이 PR 도 여기가 아니라 장 만든 뒤에 잰다 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
5727e6c to
5a3d32c
Compare
Opens the docs-only cycle for the next release train. The planning note this started from targeted v2.31.1; that baseline is void because v2.32.0 shipped from main on 2026-08-24. This unit re-derives the baseline from live git state and plans the train as v2.32.1, bugfix-only. The first draft got the branch relationship wrong: it read a one-way --is-ancestor result as divergence. An independent audit re-ran both directions and dev turns out to be an ancestor of main, 0 ahead and 27 behind, with a one-line tree delta. wp1 is therefore a fast-forward, not a backmerge, and the correction is recorded in the document rather than quietly fixed. Three audit rounds moved two other things. lidge-jun#2427 was reordered from first to last: changing the test runner before the runtime fixes would make every later failure ambiguous between a real regression and parallel-execution flakiness. And lidge-jun#2472's regression got its own work-phase (wp9) once the audit pointed out the plan had made it a mandatory gate while assigning nobody to write it. Contents: 000 baseline/scope/roadmap, 001 verbatim reviewer-lane evidence, and one diff-level decade doc per implementation phase (010 wp1, 020 wp3/lidge-jun#2483, 030 wp4/lidge-jun#2481, 040 wp5/lidge-jun#2473, 050 wp6/lidge-jun#2477, 060 wp7/lidge-jun#2476, 070 wp2/lidge-jun#2427, 080 wp8 freeze, 090 wp9/lidge-jun#2472). No code changes. No promotion, tag, or publish.
The Codex backend closes the socket on any inbound message of 16 MiB or more without sending a Responses terminal event, which reached clients as a bare 502 upstream_server_error. Because the wrapper only fell back to SSE when the *upgrade* failed, a thread that crossed the ceiling could never recover: every retry resent the same oversized frame. Measured against the live endpoint on 2026-08-23: 16,777,000 B completed, 16,777,300 B closed the socket in ~1s, reproducibly. The same body still succeeds over HTTP SSE, so the limit belongs to this transport alone. Size the `response.create` frame before dialing and take the SSE path when it does not fit. Deciding before the socket opens is what keeps the resend safe -- after open the caller already holds a streaming Response, and a retry there could double-generate the turn. Two supporting changes: - Carry the WS close code and reason into the stream error. A 1009 was previously indistinguishable from a network drop, and nothing in usage.jsonl or /api/logs recorded the real cause. - Apply the provider's `upstreamHttpVersion` pin to the SSE fallback. The fallback is a routine path now, and serving a turn over HTTP while silently dropping the operator's protocol pin is wrong. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5a3d32c to
f093bf0
Compare
The sizing helper already had unit tests, but nothing proved the real serialized frame routes correctly one byte on each side of the limit. That gap matters because the request body is not the frame: `stream` is deleted and `type` is added before sending, so padding sized against the body sits eleven bytes away from what is actually transmitted. An off-by-one would live exactly there and pass every existing test. These two build padding so the serialized frame is exactly limit-1 and exactly limit, then assert the whole path: one socket and one send of the expected byte length under, zero sockets and one SSE call at it. Flipping the gate from >= to > fails the second one, so it catches a real off-by-one at the transport level rather than only in the helper. Two comment corrections while here. The close-code comment claimed the named 1009 message makes the failure diagnosable from the logs; it does not. The eager relay turns any stream error into a generic `upstream_reset` synthetic terminal without feeding it back through the inspector, so `/api/logs` retains only `streamAborted`. The message reaches the client and stops there, and the comment now says so rather than promising observability the code does not deliver. The margin comment described 64 KiB as absorbing a future append. There is no append. It is a conservative cushion, and the useful thing to record is what it actually covers: RFC 6455 framing is 14 bytes at this payload size — an 8-byte extended length plus a 4-byte client mask — so even a backend counting frame headers has ~65.5 KiB of room. Tests: 59 pass, 1 skip across ws-upstream, sse-failed-tail, and upstream-http-version. tsc --noEmit clean.
|
This is the right shape for the defect, and the call order is what makes it work: the frame is serialized at I pushed one commit ( The test gap. The sizing helper had unit coverage, but nothing proved the real serialized frame routes correctly at the boundary. That distinction is load-bearing here: the request body is not the frame, because Padding sized against the body sits 11 bytes away from what is transmitted, so an off-by-one would live exactly there and pass every existing test. The two new cases build padding so the serialized frame is exactly Two comments were promising more than the code delivers. The close-code comment said naming 1009 makes the failure diagnosable from the logs. It does not. The eager relay converts any stream error into a generic The margin comment described 64 KiB as absorbing a future append; there is no append. It is a conservative cushion, so I recorded what it actually covers: RFC 6455 framing at this payload size is 14 bytes (8-byte extended length + 4-byte client mask), leaving roughly 65.5 KiB of room even if the backend counted frame headers. On the fast path. I went looking for a defect in the Verification: 59 pass / 1 skip / 0 fail across Approving once Cross-platform CI is green. |
lidge-jun
left a comment
lidge-jun
left a comment
There was a problem hiding this comment.
Approving. All required jobs are green at 566a4714d.
The macOS suite needed one re-run, failing Cursor discovery bounded retry > retries a transient timeout once with a fresh session — a timing-bounded test in a file that does not import either changed module, and 41 pass / 0 fail locally on this branch. That is the third distinct macOS timing flake observed across this release train, each in a different subsystem; none has been attributable to the diff under test.
…e-jun#2473) * fix(codex): keep oversized Responses turns off the WS transport The Codex backend closes the socket on any inbound message of 16 MiB or more without sending a Responses terminal event, which reached clients as a bare 502 upstream_server_error. Because the wrapper only fell back to SSE when the *upgrade* failed, a thread that crossed the ceiling could never recover: every retry resent the same oversized frame. Measured against the live endpoint on 2026-08-23: 16,777,000 B completed, 16,777,300 B closed the socket in ~1s, reproducibly. The same body still succeeds over HTTP SSE, so the limit belongs to this transport alone. Size the `response.create` frame before dialing and take the SSE path when it does not fit. Deciding before the socket opens is what keeps the resend safe -- after open the caller already holds a streaming Response, and a retry there could double-generate the turn. Two supporting changes: - Carry the WS close code and reason into the stream error. A 1009 was previously indistinguishable from a network drop, and nothing in usage.jsonl or /api/logs recorded the real cause. - Apply the provider's `upstreamHttpVersion` pin to the SSE fallback. The fallback is a routine path now, and serving a turn over HTTP while silently dropping the operator's protocol pin is wrong. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(codex): pin the transport boundary at the adjacent byte The sizing helper already had unit tests, but nothing proved the real serialized frame routes correctly one byte on each side of the limit. That gap matters because the request body is not the frame: `stream` is deleted and `type` is added before sending, so padding sized against the body sits eleven bytes away from what is actually transmitted. An off-by-one would live exactly there and pass every existing test. These two build padding so the serialized frame is exactly limit-1 and exactly limit, then assert the whole path: one socket and one send of the expected byte length under, zero sockets and one SSE call at it. Flipping the gate from >= to > fails the second one, so it catches a real off-by-one at the transport level rather than only in the helper. Two comment corrections while here. The close-code comment claimed the named 1009 message makes the failure diagnosable from the logs; it does not. The eager relay turns any stream error into a generic `upstream_reset` synthetic terminal without feeding it back through the inspector, so `/api/logs` retains only `streamAborted`. The message reaches the client and stops there, and the comment now says so rather than promising observability the code does not deliver. The margin comment described 64 KiB as absorbing a future append. There is no append. It is a conservative cushion, and the useful thing to record is what it actually covers: RFC 6455 framing is 14 bytes at this payload size — an 8-byte extended length plus a 4-byte client mask — so even a backend counting frame headers has ~65.5 KiB of room. Tests: 59 pass, 1 skip across ws-upstream, sse-failed-tail, and upstream-http-version. tsc --noEmit clean. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: bitkyc08-arch <bitkyc08@gmail.com>
2 participants
Summary
Closes #2426.
The Codex backend closes the socket on any inbound message of 16 MiB or more without sending a Responses terminal event.
codexWsUpstreamFetchonly fell back to SSE when the upgrade failed, so a close after open became a bare502 upstream_server_errorwith no fallback — and a thread that crossed the ceiling could never recover, because every retry resent the same oversized frame.I measured the boundary against the live endpoint on 2026-08-23:
Exactly 16 MiB = 16,777,216 B, reproducible byte-for-byte. It is not a Bun send-side cap — a local Bun WS server with
maxPayloadLength: 512 MiBaccepts a 20 MiB frame from the same client — so the ceiling is the backend's, consistent with close code 1009. #2426's control observation shows the same body still succeeding over HTTP SSE on 2.28.0.The fix: size the
response.createframe before dialing and take the SSE path when it does not fit. Deciding before the socket opens is what keeps the resend safe — after open the caller already holds a streamingResponse, and retrying there could double-generate the turn (the concern raised in #2426).Two supporting changes in the same path:
usage.jsonlnor/api/logsrecorded the real cause — the only place the truth survived was the client's rollout file. An oversized close now says so.upstreamHttpVersionpin to the SSE fallback.providerFetchpassed the rawbasefetch as the fallback, so any WS turn that fell back lost the operator's protocol pin. That was already true for the existing fallbacks; this change makes the fallback a routine path, so it is fixed here rather than left as a latent hole.Not in scope, and worth separate issues: an image budget / downscaling for inline
input_imagepayloads (what actually keeps threads away from the ceiling — under full replay ~11 pasted screenshots is enough to cross it), and suppressing account rotation on a 1009-class refusal.Verification
Exact head
f093bf06f, rebased ontoa60d51748./node_modules/.bin/bun test tests/ws-upstream.test.ts tests/upstream-http-version.test.ts tests/request-pacing.test.ts tests/cursor-adapter.test.ts tests/agent-task-recovery.test.ts tests/agent-task-recovery-security.test.ts— 113 passed, 1 runtime-specific skip../node_modules/.bin/bun run typecheck— passed../node_modules/.bin/bun run privacy:scan— passed.git diff --check upstream/dev...HEAD— passed.bun run test:changedis not available on currentdev; that script is introduced by the still-open feat(test): add test:changed and make it the local check during implementation #2429, so it was not reported as passed../node_modules/.bin/bun run test— 2 failures across 14,574 tests / 907 files in 636.51s:tests/key-login-live-update.test.tsreproduces identically on untoucheddevata60d51748;Checklist
redactSecretStringat the log layer. No auth or credential paths touched; the fallback reuses the same authenticated fetch the non-WS branch already used.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit