fix(examples): avoid unhandled WebSocket rejections - #2669
Conversation
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. |
Castiron custom code✅ No new custom-code files detected. 32 mixed files remain; 0 existing customizations changed. Compared 32 existing customizations unchanged
A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 34070537085 --repo openai/openai-node \
--name castiron-custom-code-34070537085-1 --dir /tmp/castiron-custom-code-34070537085-1
git apply --stat /tmp/castiron-custom-code-34070537085-1/custom-code.patch
cat /tmp/castiron-custom-code-34070537085-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin fdb038e21509264bcdf81741a40988dd694188b1 09444de4d12396d2392c99b1a5ba64e35cd84f23
python3 scripts/castiron/custom_code_report.py report \
--base fdb038e21509264bcdf81741a40988dd694188b1 \
--head 09444de4d12396d2392c99b1a5ba64e35cd84f23 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-09444de4d123
cat /tmp/castiron-custom-code-09444de4d123/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed both changed files and the SDK send queue, generic/dedicated error dispatch, adapter/emitter lifecycle, and tool-loop callers. No actionable correctness, security, or maintainability findings. This is a useful structural simplification: the canonical queue removes the raw opening waiter, and the dedicated SDK error callback retains ownership until dispatch. Printing the first heading while connecting is reasonable.
Independent validation: 124 focused tests passed; the ten-case example fixture also passed on Node 22.0.0 and 26.7.0 (primary run: 24.20.0). Running the new fixture against base reproduced seven failures with all three controls passing. A separately written real-loopback fixture exercised the actual transpiled example through freshly built public CJS and ESM entrypoints: 78 head runs passed across those three runtimes, with zero uncaught errors, one failure report per failing scenario, preserved error-over-close precedence, six-request response-ID/tool-output chaining, and owned child/server cleanup. Another 78 base runs reproduced the seven targeted unhandled-rejection scenarios. This includes rejected upgrades and a single transport write containing HTTP 101 plus the initial error frame.
Canonical build/lint, TypeScript 6, and whitespace checks passed. Registry installation was unavailable, so validation used a private copy of cached dependencies with the matching lockfile. I did not rerun the full handwritten/generated or ecosystem suites.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed exact head 09444de. Removing the example-local opening waiter and manual error branch correctly leaves socket-open queuing and API/transport error ownership with the SDK connection. The regression covers rejected upgrades, nested/flat API errors, same-write handshake/error delivery, close precedence, multi-turn chaining, and uncaught rejection monitoring. CI is green and there are no unresolved threads. No in-scope findings.
3 participants
Summary
errorcallback own protocol errors, rather than rejecting and removing that callback during generic event dispatch.Reproduction
For an API error frame, the SDK emits the generic
eventcallback before its dedicatederrorcallback. The example previously handled the generic event and removed its error listener, so the SDK then produced an unhandled rejection for the same failure. Opening handshake failures and an error frame delivered together with HTTP 101 exposed related listener gaps.These cases already exited with status 1; the bug was the additional uncaught rejection and crash diagnostics. The fix retains normal failure exit codes, API-error precedence over a subsequent close, the six-request tool loop, and response-ID chaining. The first demo heading can now print while the connection is opening.
Validation
uncaughtExceptionMonitorwithout handling exceptions or changing Node's default exit behavior. It covers flat/nested API errors, immediate closes, rejected upgrades, and HTTP 101 coalesced with an initial error frame.The generated suite and remaining ecosystem/packaging checks run in CI.