Skip to content

test(responses): pin established websocket fallback - #6011

Closed
Ingwannu wants to merge 1 commit into
devfrom
codex/issue-4191-established-ws-fallback
Closed

Ingwannu wants to merge 1 commit into
devfrom
codex/issue-4191-established-ws-fallback

Conversation

@Ingwannu

@Ingwannu Ingwannu commented Sep 26, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • pin the already-landed established WebSocket → HTTP/SSE fallback from fix(responses): bundle lane E — combo resend safety, WebSocket replacement, goal helpers, Devin retry delays #5675
  • prove that pong/quota/metadata control traffic does not count as semantic output
  • forbid replacement after response creation, text, tool, usage, cancellation, deadline, stored turns, or hosted tools
  • prove request identity/fields survive the one HTTP replacement and terminal HTTP outcomes never create a third send
  • document the shared resend-budget, credential-selection, and physical-send ownership boundary in the SSOT/ADR

Closes #4191.

Current runtime status

No production source change is needed on current dev. Commit aed3bb8f42 (#5675) already routes an eligible zero-semantic-frame socket death through authorizeResendForRecovery and one HTTP-only sendAmbiguousReplacement. The replacement shares the request execution budget, OAuth dispatch/selection guard, physical-send accounting, abort state, and request identity.

This PR closes the evidence/documentation gap that left #4191 open. It does not widen the existing opt-in retryOnReset policy or change the explicit replacements: 2 contract.

Validation

Commands ran in disposable HOME, CODEX_HOME, and OPENCODEX_HOME paths under a user systemd scope. Runtime tests used CPUQuota=75%, MemoryMax=1536M, MemorySwapMax=0, TasksMax=64; structure/privacy checks used CPUQuota=50%, MemoryMax=512M, TasksMax=32.

  • bun test ./tests/responses/ws-ambiguous-resend.test.ts — 33 pass
  • bun run structure:check — pass
  • bun run privacy:scan — pass
  • git diff --check — pass

No full suite, build, repository-wide typecheck, live model request, or service restart was performed.

Summary by CodeRabbit

  • Documentation
    • Clarified when an interrupted WebSocket request may be replaced with an HTTP request: replacement eligibility ends after a Responses event, while ping/pong and quota metadata alone do not end it.
    • Documented that cancellation and connect or silence timeouts do not trigger replacement, and that replacement follows existing retry and request-safety checks.
  • Tests
    • Added coverage for control-frame traffic, cancellation, timeouts, replacement request settings, and terminal HTTP responses.

@Ingwannu
Ingwannu requested a review from lidge-jun as a code owner September 26, 2026 23:03
@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: fcda18fe-e786-413b-875f-0a908d2c4642

📥 Commits

Reviewing files that changed from the base of the PR and between 5518653 and 6ccf027.

📒 Files selected for processing (4)
  • docs-site/src/content/docs/reference/configuration/server.md
  • structure/decisions/ADR-4191-established-websocket-fallback.md
  • structure/transports/responses-failover.md
  • tests/responses/ws-ambiguous-resend.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

This change documents the established-WebSocket HTTP replacement boundary and adds tests for its eligibility and outcomes. It covers semantic Responses events, control frames, cancellation, deadlines, replacement request fields, and terminal HTTP responses. It does not change runtime behavior.

Changes

WebSocket fallback

Layer / File(s) Summary
Document fallback eligibility and accounting
structure/decisions/ADR-4191-established-websocket-fallback.md, structure/transports/responses-failover.md, docs-site/src/content/docs/reference/configuration/server.md
The documents describe which Responses events end HTTP replacement eligibility, which control frames do not, and the shared credential-selection and physical-send accounting constraints.
Test fallback eligibility and outcomes
tests/responses/ws-ambiguous-resend.test.ts
Tests cover control-frame preludes, semantic events that prevent replacement, cancellation and connect-deadline outcomes, replacement request fields, hosted tools, and terminal HTTP responses.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to 6ccf0

No issue requiring a change before merge is established. Normal validation remains appropriate.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (3 skipped: 3 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: adding Responses tests that pin the established WebSocket fallback behavior. It is concise and specific.
Linked Issues check ✅ Passed Issue #4191 requires safe fallback after an established Codex WebSocket fails before semantic output. The ADR records that the dispatch-owned HTTP replacement already exists at the merge base and uses…
Out of Scope Changes check ✅ Passed The changed files are limited to focused regression tests and documentation for the established WebSocket fallback. The tests validate the existing #4191 behavior and its safety boundaries. The docume…
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (3 skipped: 3 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Sep 26, 2026
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 57 / 80

이 PR은 돌아가는 서버 코드를 바꾸지 않습니다. 이미 dev에 있는 규칙을 테스트와 설명으로 고정합니다. Codex 웹소켓이 열린 뒤, 답의 첫 신호보다 먼저 끊기면, retryOnReset을 켠 요청만 HTTP로 한 번 다시 보냅니다. 핑, 퐁, 남은 사용량 숫자만 온 경우는 답이 시작된 것이 아닙니다. response.created, 도구 호출, 토큰 사용량이 적힌 답 신호가 한 번이라도 오면 다시 보내지 않습니다. 사람이 취소했거나, 연결 시간 안에 답이 없어 504가 된 뒤에는 소켓이 늦게 닫혀도 다시 보내지 않습니다. 다시 보낸 HTTP가 response.failed나 response.incomplete로 끝나도 세 번째는 없습니다. 이 동작은 #5675에 이미 들어가 있습니다. 바탕 브랜치는 dev입니다. types.ts와 config.ts 분할과는 관계가 없고, 같은 주제로 열려 있는 다른 풀리퀘스트도 없습니다.

라인 - structure/decisions/ADR-4191-established-websocket-fallback.md 14행. 결정 기록은 이 작업에서 테스트를 실행하지 않았고, 추가한 단언이 통과한다는 주장이 아니라고 적습니다. PR 본문은 같은 파일이 33개 통과했다고 적습니다. 33은 이 파일의 케이스 수와 같습니다. 실제로 돌린 기록인지, 개수를 센 것인지를 두 글이 서로 다르게 말합니다.

라인 - tests/responses/ws-ambiguous-resend.test.ts 438행, 488행. 답 신호가 온 뒤의 테스트와, HTTP가 실패나 잘림으로 끝난 테스트는 response.text() 오류를 삼킵니다. 다시 보내지 않았는지만 봅니다. 호출자가 받은 상태 코드와 본문은 기대값에 없습니다. 두 번째 전송 없이 성공처럼 끝나는 회귀는 이 단언을 통과합니다.

라인 - 이슈 #4191과 Closes #4191. 신고된 실패는 두 가지입니다. 답이 끝나기 전에 소켓이 1006으로 닫힌 경우, 그리고 프리루드 시간 초과입니다. src/server/responses/passthrough-dispatch.ts 1707행은 소켓 사망 표시가 있을 때만 HTTP로 바꿉니다. 시간 초과와 response.created 이후의 끊김에는 그 표시가 없습니다. 표시가 있어도 retryOnReset이 꺼져 있으면 다시 보내지 않습니다. 기본 설정에서는 신고된 긴 스레드가 같은 실패로 남습니다.

메인테이너의 판단이 필요한 지점

#4191을 이 PR로 닫을지 정하면 됩니다. 닫는 범위가 "답 신호 전에 소켓이 죽었고, 재전송 설정을 켠 경우"라면 본문에 그 한계를 적는 편이 맞습니다. 프리루드 시간 초과와 답이 시작된 뒤의 1006은 이 변경 밖에 있습니다.

결정 기록 14행과 본문의 "33개 통과" 중 어느 쪽을 남길지도 정하면 됩니다.

너의 추천

테스트와 문서 보강은 머지해도 됩니다. 머지 전에 결정 기록 14행을 실제 실행 결과에 맞추거나, 본문에서 통과 문장을 빼면 됩니다. 438행과 488행에는 상태 코드나 오류 문장을 기대값으로 남기면, 재전송이 없다는 사실과 호출자가 받는 답이 같이 고정됩니다. #4191은 프리루드 시간 초과와 답 시작 이후의 끊김을 이 PR이 다루지 않으므로, 그 범위를 남긴 채 닫지 않는 쪽을 권합니다. 바탕은 dev라 그대로 두면 됩니다.

이 댓글은 grok-bot이 작성했습니다

@Ingwannu

Copy link
Copy Markdown
Owner Author

Exact head 6ccf0276e4 is now green: all four test shards, gates, structure/storage/API checks, docs, Docker/keyring, hygiene, React Doctor and the aggregate ci job passed. This PR only adds regression coverage for the already-landed runtime fallback behavior. @lidge-jun please review/approve.

@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev in #6059 (merge 8923ad9835) as one squashed commit that keeps your authorship. A follow-up commit updates ADR-4191's validation note. Thank you. Closing because this repository merges into dev, so GitHub does not close carried PRs automatically.

@lidge-jun lidge-jun closed this Sep 27, 2026
mdwsk88 pushed a commit to mdwsk88/opencodex that referenced this pull request Sep 27, 2026
Carried from lidge-jun#6011 into merge train round 3.

Co-authored-by: Ingwannu <ingwannu@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants