fix(responses-ws): handle abort during upstream handshake#1332
Conversation
📝 WalkthroughWalkthrough本次变更修复 Responses WebSocket 在握手及关闭竞态中的异常处理和重复收尾，并新增 ChangesResponses WebSocket 生命周期
Responses WebSocket 配置覆盖
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an environment variable override (ENABLE_OPENAI_RESPONSES_WEBSOCKET) to control the OpenAI Responses WebSocket setting, along with corresponding unit tests. It also refactors the WebSocket closing logic in upstream-adapter.ts to safely handle socket termination and prevent unhandled asynchronous errors from escaping to process-level crash handlers. The review feedback suggests improving safelyCloseWebSocket to prevent duplicate listener registration on already-closing sockets and to avoid potential listener leaks by properly cleaning up both error and close listeners when either fires.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (ws.readyState === 3) return; | ||
|
|
||
| // Active closes can still surface an asynchronous error after request-level | ||
| // listeners are detached. This one-shot sink keeps the error owned by this | ||
| // socket instead of letting it escape to process-level crash handlers. | ||
| const consumeCloseError = () => {}; | ||
| ws.once("error", consumeCloseError); | ||
| ws.once("close", () => { | ||
| ws.off("error", consumeCloseError); | ||
| }); | ||
|
|
||
| // Another close path may already have started the handshake. Keep the | ||
| // temporary sink above, but do not issue a duplicate close or terminate. | ||
| if (ws.readyState === 2) return; |
There was a problem hiding this comment.
There are two potential issues with the current implementation of safelyCloseWebSocket:
- Duplicate Listener Registration: If
safelyCloseWebSocketis called on a socket that is already in theCLOSINGstate (readyState === 2), it will register another set oferrorandcloselisteners before returning early. Moving thereadyStatecheck to the top of the function (e.g.,ws.readyState >= 2) prevents this redundant registration. - Listener Leak: If an
errorevent is emitted, theonce("error")listener is automatically removed, but theonce("close")listener remains registered. Cleaning up both listeners when either fires ensures no listener leaks occur on the WebSocket instance.
Using hoisted function declarations for cleanup and consumeCloseError makes the cleanup logic robust and clean.
| if (ws.readyState === 3) return; | |
| // Active closes can still surface an asynchronous error after request-level | |
| // listeners are detached. This one-shot sink keeps the error owned by this | |
| // socket instead of letting it escape to process-level crash handlers. | |
| const consumeCloseError = () => {}; | |
| ws.once("error", consumeCloseError); | |
| ws.once("close", () => { | |
| ws.off("error", consumeCloseError); | |
| }); | |
| // Another close path may already have started the handshake. Keep the | |
| // temporary sink above, but do not issue a duplicate close or terminate. | |
| if (ws.readyState === 2) return; | |
| if (ws.readyState >= 2) return; | |
| // Active closes can still surface an asynchronous error after request-level | |
| // listeners are detached. This one-shot sink keeps the error owned by this | |
| // socket instead of letting it escape to process-level crash handlers. | |
| function cleanup() { | |
| ws.off("error", consumeCloseError); | |
| ws.off("close", cleanup); | |
| } | |
| function consumeCloseError() { | |
| cleanup(); | |
| } | |
| ws.once("error", consumeCloseError); | |
| ws.once("close", cleanup); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR. The fix correctly consolidates WebSocket teardown into safelyCloseWebSocket, which terminates CONNECTING sockets (avoiding the async error that previously escaped to process-level crash handlers) and installs a one-shot error sink. The new requestFinished re-entrancy guard and the openResolved-aware first-event timer cleanly close the abort/timeout race windows. The ENABLE_OPENAI_RESPONSES_WEBSOCKET tri-state env override is a small, independent addition with correct warn-once behavior and accurate .env.example docs.
PR Size: M
- Lines changed: 508 (478 additions, 30 deletions)
- Files changed: 5
- Note: ~320 of 478 additions are test code; the substantive source change is ~65 lines. The two logical changes (abort-handling fix + env override) are separable but small enough to ship together.
Review Coverage
- Logic and correctness - Clean. Re-traced abort-during-CONNECTING, already-aborted signal, abort/timeout race, and CLOSING-state race; all paths terminate exactly once with no escaped async errors.
- Security (OWASP Top 10) - Clean. No new user-input handling, injection surface, or auth-path changes.
- Error handling - Clean. The
// ignorecatches insafelyCloseWebSocketare intentional, non-actionable teardown-path swallows consistent with the pre-existingcloseWs/terminateWspattern; the one-shotconsumeCloseErrorsink is the meaningful error ownership mechanism. - Type safety - Clean. No
anyin new code; the test-only_readyStatecast is a deliberate private-state probe. - Documentation accuracy - Clean. Comments explain the CONNECTING-terminate rationale and the close-race fallback;
.env.exampleaccurately describes accepted values. - Test coverage - Adequate. Four new cases cover CONNECTING-abort-after-construction, already-aborted signal (asserts no 20s timer), abort/timeout close-once race, and CLOSING-state error-sink retention; all assert zero
uncaughtException/unhandledRejection. - Code clarity - Good.
Automated review by Claude AI
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Fixes an
uncaughtExceptionthat crashed and restarted the app when a client aborted a request while the upstream OpenAI Responses WebSocket was still in theCONNECTINGhandshake state. Also adds a tri-stateENABLE_OPENAI_RESPONSES_WEBSOCKETenv override so immutable-image deployments can disable the feature without DB access.Problem
When
enableOpenaiResponsesWebsocketis on and a client disconnects (or theAbortSignalfires) before the upstream WebSocket handshake completes:onAbort()callsfinishRequest({ closeCode: 1000, forgetSession: true }).finishRequest()callscleanupRequestListeners()first, detaching the request-scopederrorlistener.ws.close(code)on a socket that is stillCONNECTING.wspackage asynchronously emitsWebSocket was closed before the connection was established.errorlistener already removed, the error escapes to the process-level handler ininstrumentation.ts, which callsprocess.exit(1).restart: unless-stopped, the container restarts - interrupting in-flight streams and causing repeated, spurious restarts (one deployment saw 11 auto-restarts in a few hours, all from this single exception).The issue also notes that the system-settings UI does not render a toggle for
enableOpenaiResponsesWebsocket, so operators on immutable images have no way to turn the feature off without editing the database.Related Issues:
Solution
Two coordinated changes keep the request-level abort from becoming a process-level crash:
closeWs()/terminateWs()are replaced by a singlesafelyCloseWebSocket()that installs a one-shoterrorsink before closing, then branches onreadyState:CLOSEDno-ops;CLOSINGkeeps the sink but skips a duplicate close;CONNECTINGcallsterminate()(aborting the incomplete handshake) instead ofclose()(which is what emits the stray error);OPENdoes a normalclose()with aterminate()fallback on a readyState race.finishRequest()that detaches listeners last. ArequestFinishedguard prevents double execution, andcleanupRequestListeners()now runs after the close so the one-shot error sink is in place when the async error fires. The first-event-timeout and open-failure paths are rerouted through this safe path instead of callingcloseAndForget()/terminateWs()directly.The env override is implemented at the read site in
isOpenaiResponsesWebsocketEnabled(): unset falls through to the DB setting;true/1orfalse/0force the value; anything else warns once and falls back to the DB. This preserves existing deployments (no default change, no migration).Changes
Core Changes
src/app/v1/_lib/responses-ws/upstream-adapter.ts- newsafelyCloseWebSocket()(one-shot error sink + readyState-aware close/terminate), idempotentfinishRequest()that cleans up listeners last, first-event-timeout and open-failure paths rerouted through the safe path.Supporting Changes
src/lib/config/system-settings-cache.ts-getOpenaiResponsesWebsocketEnvOverride()tri-state parser with single-warn-on-invalid, applied inisOpenaiResponsesWebsocketEnabled()..env.example- documentsENABLE_OPENAI_RESPONSES_WEBSOCKET.tests/unit/lib/config/system-settings-cache.test.ts- covers true/false/1/0 override, unset fallthrough, and invalid-value warn-once behavior.src/app/v1/_lib/responses-ws/__tests__/upstream-adapter.test.ts- new tests for the CONNECTING-state abort (post-construction and pre-aborted signal), the abort/first-event-timeout race (close only once), and the CLOSING-state race (error sink retained); all assert nouncaughtException/unhandledRejection.Breaking Changes
None. The env var is opt-in (unset preserves current behavior); the close-path changes are internal.
Testing
Automated Tests
upstream-adaptercases + 6system-settings-cachecasesManual Testing
enableOpenaiResponsesWebsocketin DB (or setENABLE_OPENAI_RESPONSES_WEBSOCKET=true)./v1/responsesrequest to a Codex provider whose upstream WebSocket handshake stalls (e.g. point at a TCP blackhole / delayed-upgrade server).CONNECTING.aborted before first upstream WebSocket event; nouncaughtExceptionis logged; the process does not restart; other requests are unaffected.ENABLE_OPENAI_RESPONSES_WEBSOCKET=falseforces HTTP Responses routing regardless of the DB value.Checklist
Description enhanced by Claude AI
Greptile Summary
This PR fixes a crash/unhandled-error scenario that occurred when an upstream WebSocket abort signal fired while the socket was still in the
CONNECTINGstate (handshake in progress). The previous code issued a normalclose()on a CONNECTING socket, which thewslibrary handles by emitting an asynchronous error that had no listener — causing an unhandled exception.safelyCloseWebSocketreplaces the oldcloseWs/terminateWspair and correctly dispatches on all fourreadyStatevalues: CLOSED (no-op), CLOSING (install error sink only), CONNECTING (install sink +terminate()), OPEN (install sink +close()), with aterminate()fallback ifclose()races a state transition.requestFinishedguard infinishRequestprevents double-close when abort and the 20-second first-event timeout fire in the same event loop tick, ensuringcleanupRequestListenersand socket teardown run exactly once.ENABLE_OPENAI_RESPONSES_WEBSOCKET) is added alongside the DB-backed setting, with one-shot warning deduplication for invalid values and full test coverage.Confidence Score: 5/5
Safe to merge. The fix is narrowly scoped to the abort-during-handshake edge case and is validated by four new integration tests that exercise CONNECTING-state abort, pre-aborted signal, abort/timeout race, and CLOSING-state races.
All four readyState branches in safelyCloseWebSocket are handled explicitly and tested. The requestFinished guard correctly prevents double teardown without hiding other bugs. The env override is cleanly layered over the DB cache with no risk to existing behavior when the var is unset.
No files require special attention. The upstream-adapter changes are the most complex but the new tests give good coverage of the tricky races.
Important Files Changed
Reviews (1): Last reviewed commit: "fix(responses-ws): handle abort during u..." | Re-trigger Greptile