fix(responses-ws): handle abort during upstream handshake - #1334
Conversation
📝 WalkthroughWalkthroughChangesResponses WebSocket 生命周期修复
Responses WebSocket 配置覆盖
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
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 |
![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 feature, bypassing database settings when configured. It also hardens the WebSocket lifecycle management in upstream-adapter.ts by introducing safelyCloseWebSocket to handle aborted handshakes and prevent unhandled asynchronous errors. The review feedback suggests two key improvements: caching the parsed environment variable to avoid reading process.env on every request, and tracking the error sink registration in safelyCloseWebSocket to prevent duplicate listeners when the function is called multiple times on a closing socket.
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.
| function safelyCloseWebSocket(ws: WebSocketType, code: number): void { | ||
| 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.
In safelyCloseWebSocket, if the function is called multiple times on a socket that is in the CLOSING (readyState === 2) state (which can happen in certain failure paths like sendFrame or onUnexpectedResponse where closeAndForget is called first and then finishRequest is called), it will register duplicate once("error") and once("close") listeners.
To prevent duplicate listener registration and potential listener leaks, we can track whether the error sink has already been registered on the socket using a custom property.
function safelyCloseWebSocket(ws: WebSocketType, code: number): void {
if (ws.readyState === 3) return;
const customWs = ws as any;
if (!customWs._cchCloseErrorSink) {
const consumeCloseError = () => {};
customWs._cchCloseErrorSink = consumeCloseError;
ws.once("error", consumeCloseError);
ws.once("close", () => {
ws.off("error", consumeCloseError);
delete customWs._cchCloseErrorSink;
});
}
// 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;| /** Avoid repeating the same invalid environment-variable warning on every request. */ | ||
| let hasWarnedInvalidResponsesWebsocketEnv = false; | ||
|
|
||
| function getOpenaiResponsesWebsocketEnvOverride(): boolean | undefined { | ||
| const rawValue = process.env.ENABLE_OPENAI_RESPONSES_WEBSOCKET; | ||
|
|
||
| if (rawValue === undefined) { | ||
| return undefined; | ||
| } | ||
|
|
||
| switch (rawValue) { | ||
| case "true": | ||
| case "1": | ||
| return true; | ||
| case "false": | ||
| case "0": | ||
| return false; | ||
| default: | ||
| if (!hasWarnedInvalidResponsesWebsocketEnv) { | ||
| hasWarnedInvalidResponsesWebsocketEnv = true; | ||
| logger.warn( | ||
| "[SystemSettingsCache] Invalid ENABLE_OPENAI_RESPONSES_WEBSOCKET, using database setting", | ||
| { value: rawValue } | ||
| ); | ||
| } | ||
| return undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
Reading process.env on every request can introduce unnecessary overhead. Since environment variables are static during the application's lifecycle, we can parse and cache the ENABLE_OPENAI_RESPONSES_WEBSOCKET override value once on first access. This improves performance and simplifies the warning logic by eliminating the need for a separate hasWarnedInvalidResponsesWebsocketEnv flag.
let cachedEnvOverride: boolean | undefined = undefined;
let hasParsedEnvOverride = false;
function getOpenaiResponsesWebsocketEnvOverride(): boolean | undefined {
if (hasParsedEnvOverride) {
return cachedEnvOverride;
}
const rawValue = process.env.ENABLE_OPENAI_RESPONSES_WEBSOCKET;
hasParsedEnvOverride = true;
if (rawValue === undefined) {
cachedEnvOverride = undefined;
return undefined;
}
switch (rawValue) {
case "true":
case "1":
cachedEnvOverride = true;
break;
case "false":
case "0":
cachedEnvOverride = false;
break;
default:
logger.warn(
"[SystemSettingsCache] Invalid ENABLE_OPENAI_RESPONSES_WEBSOCKET, using database setting",
{ value: rawValue }
);
cachedEnvOverride = undefined;
break;
}
return cachedEnvOverride;
}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function safelyCloseWebSocket(ws: WebSocketType, code: number): void { | ||
| 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 === 0) { | ||
| // ws emits this error asynchronously when a CONNECTING socket is closed | ||
| // or terminated, so abort the incomplete handshake instead of attempting | ||
| // a normal close handshake. | ||
| try { | ||
| ws.terminate(); | ||
| } catch { | ||
| // ignore | ||
| } | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| function terminateWs(ws: WebSocketType): void { | ||
| try { | ||
| ws.terminate?.(); | ||
| ws.close(code); | ||
| } catch { | ||
| // ignore | ||
| // A readyState transition can race with close(). If the socket is still | ||
| // live, terminate it using the error consumer installed above. | ||
| if (!isWsClosingOrClosed(ws)) { | ||
| try { | ||
| ws.terminate(); | ||
| } catch { | ||
| // ignore | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
safelyCloseWebSocket can install two overlapping error sinks on the same socket
safelyCloseWebSocket has no idempotency guard of its own. closeAndForget (which wraps it) is also unguarded. A late-firing ws.send callback (e.g. from sendFrame on a reused socket) can call closeAndForget after finishRequest has already run and initiated a first close — at that point the socket is in readyState === 2 (CLOSING), so safelyCloseWebSocket installs a second once("error", consumeCloseError) and a second once("close", ...) cleaner. The first close event fires, removes only the first error sink, and the second consumeCloseError listener is orphaned for the lifetime of the socket. The impact is a minor listener leak on an already-dying socket, not a crash, but it runs counter to the careful cleanup the rest of this function performs. Adding a simple if (isWsClosingOrClosed(ws)) return; early-exit — or tracking whether the sink is already installed — would close the gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/responses-ws/upstream-adapter.ts
Line: 209-250
Comment:
**`safelyCloseWebSocket` can install two overlapping error sinks on the same socket**
`safelyCloseWebSocket` has no idempotency guard of its own. `closeAndForget` (which wraps it) is also unguarded. A late-firing `ws.send` callback (e.g. from `sendFrame` on a reused socket) can call `closeAndForget` after `finishRequest` has already run and initiated a first close — at that point the socket is in `readyState === 2` (CLOSING), so `safelyCloseWebSocket` installs a second `once("error", consumeCloseError)` and a second `once("close", ...)` cleaner. The first `close` event fires, removes only the first error sink, and the second `consumeCloseError` listener is orphaned for the lifetime of the socket. The impact is a minor listener leak on an already-dying socket, not a crash, but it runs counter to the careful cleanup the rest of this function performs. Adding a simple `if (isWsClosingOrClosed(ws)) return;` early-exit — or tracking whether the sink is already installed — would close the gap.
How can I resolve this? If you propose a fix, please make it concise.| "already-aborted upstream WebSocket attempt hung" | ||
| ); | ||
| await flushProcessEvents(); | ||
|
|
||
| expect("failed" in result).toBe(true); | ||
| if (!("failed" in result)) return; | ||
| expect(result.message).toContain("aborted before first upstream WebSocket event"); |
There was a problem hiding this comment.
_readyState field is fragile
The CLOSING-race test sets mutableClientSocket._readyState = WebSocket.CLOSING by casting through an intersection type to reach the ws library's internal private field. This works today because ws stores readyState in _readyState, but the library makes no API promise about that name. If a future ws patch renames or wraps the field, the cast silently succeeds while the socket stays OPEN, and the test becomes a false-positive. Consider whether there is a higher-level way to reach CLOSING state (e.g. by having the mock server close its end and waiting for the half-close) or, if the direct field mutation is necessary, adding a runtime assertion right after (expect(clientSocket.readyState).toBe(WebSocket.CLOSING)) to fail loudly when the internal shape changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/responses-ws/__tests__/upstream-adapter.test.ts
Line: 914-920
Comment:
**Direct mutation of private `_readyState` field is fragile**
The CLOSING-race test sets `mutableClientSocket._readyState = WebSocket.CLOSING` by casting through an intersection type to reach the `ws` library's internal private field. This works today because `ws` stores `readyState` in `_readyState`, but the library makes no API promise about that name. If a future `ws` patch renames or wraps the field, the cast silently succeeds while the socket stays `OPEN`, and the test becomes a false-positive. Consider whether there is a higher-level way to reach `CLOSING` state (e.g. by having the mock server close its end and waiting for the half-close) or, if the direct field mutation is necessary, adding a runtime assertion right after (`expect(clientSocket.readyState).toBe(WebSocket.CLOSING)`) to fail loudly when the internal shape changes.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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
This PR cleanly fixes a process-crash class bug (abort during the upstream WebSocket handshake could emit an async 'error' with no listener) and adds an optional ENABLE_OPENAI_RESPONSES_WEBSOCKET env override. The core mechanism — a unified safelyCloseWebSocket that installs a one-shot error sink and routes CONNECTING sockets through terminate(), plus an idempotent finishRequest — is well-reasoned, and the new tests cover the hard race conditions (abort-vs-timeout, already-aborted signal, CLOSING-state abort). No issues survived validation.
PR Size: M
- Lines changed: 508 (478 additions, 30 deletions)
- Files changed: 5
Notes from deep review (no action required)
- All four call sites of the removed
closeWs/terminateWsare updated (lines 238, 444, 666, 727); no dangling references. Build will not break. - The
catch { // ignore }blocks and the no-opconsumeCloseErrorsink insafelyCloseWebSocketdeliberately swallow close-time ws errors. This is intentional and pre-existing (the originalcloseWs/terminateWshad the identical pattern); the new sink is strictly safer (it catches the async error the old code let escape to process-level handlers). The explanatory comments document the intent. finishRequestreordering (cleanup moved to the end, behind an idempotency guard) is correct: the request'sonError/onClosestay attached during the close so bookkeeping stays coherent, and therequestFinishedguard prevents the double-close/double-cleanup that the abort-vs-timeout race would otherwise cause. Verified the error sink reliably catches the post-close async error (no async gap exists between attaching the sink andcleanupRequestListeners).getOpenaiResponsesWebsocketEnvOverrideshort-circuits before any DB read on valid values; the once-per-process invalid-value warning is intentional (per-request call path, documented in the comment) and avoids log spam.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean (silent swallows are intentional/pre-existing)
- Type safety - Clean
- Documentation accuracy - Clean (comments match behavior)
- Test coverage - Strong (abort races, CONNECTING/CLOSING states, env override)
- Code clarity - Good
Automated review by Claude AI
d7feeb0 to
8e8965a
Compare
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
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 database 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 reports that the system-settings UI no longer renders a toggle for
enableOpenaiResponsesWebsocket(deliberately hidden in #1162), so operators on immutable images have no way to turn the feature off without editing the database. This PR provides the env-var mitigation the issue requested rather than re-adding the UI switch.Related Issues & PRs:
main(closed); this PR targetsdevSolution
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
.env.example)Description enhanced by Claude AI
Greptile Summary
This PR fixes a crash (
uncaughtException → process.exit(1)) that occurred when a client aborted a request while the upstream OpenAI Responses WebSocket was still in theCONNECTINGhandshake state. It also adds a tri-stateENABLE_OPENAI_RESPONSES_WEBSOCKETenv-var override for immutable-image deployments.safelyCloseWebSocket()replaces the oldcloseWs/terminateWspair with a single state-aware helper that installs a one-shoterrorsink before callingterminate()(for CONNECTING) orclose()(for OPEN), ensuring the async "WebSocket was closed before the connection was established" error is absorbed rather than escaping to the process-level crash handler.finishRequest()gains arequestFinishedguard to prevent double-execution, andcleanupRequestListeners()now runs after the close so the one-shot error sink is in place before the request-levelonErrorlistener is detached.getOpenaiResponsesWebsocketEnvOverride()insystem-settings-cache.tsparsestrue/false/1/0, falls through to the DB on unset, and emits a single warn-once log on invalid values.Confidence Score: 5/5
Safe to merge; the crash path is correctly closed and the change is backward compatible.
The root cause (async error escaping after request listeners were detached) is accurately diagnosed and the fix is mechanically sound: the one-shot error sink is installed synchronously before terminate()/close() is called, and cleanupRequestListeners() now runs after the close so the sink is always in place when the deferred error fires. The requestFinished guard prevents double-execution of the teardown path. The ENABLE_OPENAI_RESPONSES_WEBSOCKET env override is additive, defaults to unset, and has no effect on existing deployments.
No files require special attention beyond the minor known issues already tracked in prior review comments on upstream-adapter.ts.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant Adapter as upstream-adapter.ts participant WS as WebSocket (ws) participant Proc as process (crash handler) Note over Adapter,WS: BEFORE fix — crash path Client->>Adapter: abort signal fires Adapter->>Adapter: onAbort() → finishOpen() + cleanupRequestListeners() [removes onError] Adapter->>WS: ws.close(1000) on CONNECTING socket WS-->>Proc: async error: WebSocket was closed before the connection was established Proc->>Proc: uncaughtException → process.exit(1) Note over Adapter,WS: AFTER fix — safe path Client->>Adapter: abort signal fires Adapter->>Adapter: onAbort() → finishOpen() + finishRequest() Note right of Adapter: requestFinished guard prevents re-entry Adapter->>WS: safelyCloseWebSocket(ws, 1000) Note right of WS: readyState=0 (CONNECTING) 1. Install once error consumeCloseError 2. Install once close remove-sink-cleaner 3. ws.terminate() Adapter->>Adapter: cleanupRequestListeners() [removes onError, listeners] WS-->>WS: async error fires WS->>WS: consumeCloseError() absorbs error WS-->>Adapter: close event → remove-sink-cleaner fires Adapter-->>Client: failed true message aborted before first upstream WebSocket eventReviews (2): Last reviewed commit: "fix(responses-ws): handle abort during u..." | Re-trigger Greptile