socket/websocket: guard the error-handler dispatch against re-entering JS with a pending termination exception - #34414
Merged
Jarred-Sumner merged 9 commits intoJul 21, 2026
Claude / Claude Code Review
completed
Jul 21, 2026 in 20m 25s
Code review found 1 important issue
Found 2 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/runtime/server/WebSocketServerContext.rs:88-92 |
Same-class sibling: termination in websocket open() handler still aborts via synchronous on_close re-entry before this g |
| 🟡 Nit | test/js/bun/net/socket-handler-worker-terminate.test.ts:83-106 |
nit: worker-message and net.connect awaits do not wire error → reject (REVIEW.md convention; wsDriver already does) |
Annotations
Check failure on line 92 in src/runtime/server/WebSocketServerContext.rs
claude / Claude Code Review
Same-class sibling: termination in websocket open() handler still aborts via synchronous on_close re-entry before this guard
Same-class sibling not covered: when termination lands inside the websocket `open()` handler, `on_open`'s error branch calls `self.websocket().close()` (ServerWebSocket.rs:445) *before* reaching this guard, and that `close()` synchronously re-enters `on_close`, which then calls `on_close_handler.call(...)` at :744 with the termination still pending — the same `assertNoException` abort, just via native-close re-entry instead of the error-handler dispatch. A matching `has_exception()` guard is nee
Check warning on line 106 in test/js/bun/net/socket-handler-worker-terminate.test.ts
claude / Claude Code Review
nit: worker-message and net.connect awaits do not wire error → reject (REVIEW.md convention; wsDriver already does)
nit: per REVIEW.md ("Wire EVERY failure event … to reject the awaited promise"), the `worker.once("message", resolve)` await at line 84 and the `conn.once("data", resolve)` await at line 105 could add `worker.once("error", reject)` / `conn.once("error", reject)`. In practice an unhandled `'error'` on these EventEmitters already crashes the subprocess with the message on stderr (which is in the `toMatchObject` diff), so this is convention-only — not a hang risk.
Loading