Repository navigation
Conversation
…ing the connection
RFC 6455 7.1.7 says an endpoint that Fails the WebSocket Connection
SHOULD send a Close frame with the appropriate status code before
closing the underlying TCP connection. Bun.serve's parser previously
called us_socket_close() directly for every protocol violation, so a
conforming client only ever observed 1006 (abnormal closure) regardless
of whether the server rejected the frame for bad masking, an invalid
opcode, invalid UTF-8, or an oversized payload.
Add an Impl::failConnection hook that routes through the existing
WebSocket::end() path (format Close payload, send, set isShuttingDown,
half-close once drained) and point every violation-detection site at it
with the status code 7.4.1 mandates:
1002 protocol error (masking, RSV bits, opcode, fragment state,
invalid/reserved close code, 1-byte close body)
1007 invalid UTF-8 text or close reason
1009 message too big (raw or post-inflation)
parseClosePayload now returns 1002/1007 for an invalid close body
instead of the 1005/1006 sentinels that formatClosePayload refuses to
serialize, so the reply is a real Close frame rather than an empty one.
The app's close() handler now receives the same code and reason the peer
sees on the wire (matching the ws module) instead of 1006 plus a
diagnostic string carried on the socket-close side channel. forceClose
is kept only for the idle-timeout path.
Collaborator
Author
|
Updated 11:27 PM PT - Jul 25th, 2026
❌ @robobun, your commit 0b7a450 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35874That installs a local version of the PR into your bun-35874 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
WalkthroughChangesWebSocket protocol and message-processing failures now send explicit Close frames with codes 1002, 1007, or 1009. Close-payload validation distinguishes malformed status codes from invalid UTF-8 reasons, and tests cover wire-level responses and callback behavior. WebSocket close error handling
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Collaborator
Author
|
Confirmed: this subsumes #33691. That PR maps a 1-byte Close body to the 1006 sentinel, which |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ser hot path failConnection is reached from a dozen error branches inside the templated consume()/consumeMessage()/handleFragment() bodies. With end() and send() both header-inline, the optimizer was cloning the full close-frame-and-shutdown path at every branch of every template instantiation. One out-of-line copy per <SSL, isServer, USERDATA> instantiation is enough for a cold failure path. (The +500KB reported by the size check on build 82105 is mostly a stale baseline: main #79916 predates the quic/repl/inspector merges this branch is based on.)
Collaborator
Author
|
The binary-size check is comparing against a stale baseline. Build #82105 (before The baseline is main #79916 at Tests are green apart from two yellow retries ( |
Contributor
There was a problem hiding this comment.
No issues found beyond the two resolved nits; deferring for a maintainer to sign off on the user-visible behavior change (app close() handler now sees 1002/1007/1009 instead of 1006) and confirm CI is green across platforms.
What was reviewed:
- All twelve
forceClose→failConnectionsites return/propagatetrueback toconsume(), andend()setsisShuttingDownbefore the outeronDatauncork, so no double-processing after the Close is queued. CloseFrame::messagewidening toconst char *: only two callers, both feed it into astd::string_viewforend(); no writes through it.WebSocketProtocol's onlyImplinstantiation isWebSocketContext, so no other consumer needsfailConnectionadded;__attribute__((noinline))is fine under clang-cl on Windows (used unguarded elsewhere insrc/jsc/bindings/).
Extended reasoning...
Overview
This PR replaces bare-TCP forceClose() with a new failConnection() → WebSocket::end(code, reason) at all twelve protocol-violation branches in uWebSockets' WebSocketProtocol.h/WebSocketContext.h, so Bun.serve now writes an RFC 6455 Close frame carrying 1002/1007/1009 before dropping TCP. parseClosePayload is reworked to return 1002 for a 1-byte body or invalid/reserved code and 1007 for a non-UTF-8 reason (previously it returned the 1005/1006 sentinels that formatClosePayload refuses to serialize). A new 19-case wire-level test file covers every violation class plus three well-formed controls, and three existing test files are updated to expect the new codes the app handler now receives.
Security risks
None identified. The only new outbound write on the violation path is a ≤125-byte Close frame; end() immediately arms the short ping-timeout and half-closes once drained, so a hostile client cannot hold the connection open longer than before. Input validation is unchanged — the same predicates fire, only the response differs. No new allocation on the failure path beyond the fixed-size stack closePayload buffer already used for normal closes.
Level of scrutiny
This is a behavior change in the vendored uWS WebSocket parser hot path and it changes what user code observes in the websocket.close(ws, code, reason) handler (1006 → 1002/1007/1009). That is intentional, RFC-aligned, and matches Node's ws, but it is user-visible and touches a critical networking codepath, so it warrants a maintainer look rather than bot approval. The 0b7a450 follow-up adds __attribute__((noinline)) to keep end()/send() from being inlined at a dozen templated error sites — a code-size decision a human should also glance at.
Other factors
Both of my earlier inline nits are resolved: the probe() cleanup was fixed in b5bde3d (using server + try/finally), and the 1009-vs-1007 conflation for InflationStream::inflate() failures was acknowledged as a follow-up requiring a PerMessageDeflate.h refactor. I verified parseClosePayload has no callers outside WebSocketContext.h, that the CloseFrame::message const-qualification change is compatible with both call sites, and that WebSocketContext is the sole Impl template argument to WebSocketProtocol so nothing else needs the new failConnection static. The test coverage is thorough (raw-TCP framing client asserting both wire bytes and app-handler view) and includes controls proving well-formed frames are not over-rejected.
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 25, 2026
…he peer (#43013) ### Problem - With `Bun.serve({ tls })`, `ws.terminate()` waits for the peer's TLS close_notify. A peer that does not read never sends it: the `close` handler does not run, the fd stays open, and `message()` keeps firing. Each inbound frame re-arms the idle timeout, so nothing bounds the wait. Plain `ws://` closes at once. - The cause is `WebSocket::close()` (`packages/bun-uws/src/WebSocket.h:76`): `us_socket_close(s, 0, nullptr)`. On TLS, code 0 is the graceful close (`us_internal_ssl_close`, `packages/bun-usockets/src/crypto/openssl.c:2194`). `forceClose()` with no reason (a reserved opcode) passes the same code. ### Fix - `WebSocket::close()` closes with `FAST_SHUTDOWN` (a FIN, no close_notify). If the socket is still open, usockets parked the close behind its ciphertext spill, and `close()` closes again with `CONNECTION_RESET` (the idiom of #39548). `forceClose()` with no reason calls `close()`. - Correct because every caller relies on the close event having run on return (`CloseCode`, `src/uws_sys/us_socket_t.rs:23`). Plain sockets treat codes 0 and 2 alike. A peer that keeps up sees no change. Under send backpressure the close is now a reset (see Notes). - Not changed: the other code 0 closes in uWS (HTTP, HTTP/2), listed in the Notes. - Verified: new `test/js/bun/websocket/websocket-server-forced-close.test.ts`, the 3 `wss` cases fail without the fix. Also `test/js/bun/websocket/`, `test/js/web/websocket/`, `test/js/first_party/ws/`. ### Background - `us_socket_close(s, code, reason)` closes the fd and runs the close handler. On TLS, code 0 sends close_notify and waits for the peer, code 1 resets, and code 2 sends a FIN but waits while a spill is pending. - A spill is TLS ciphertext that bun reported as written, but the kernel did not accept yet. - uWS `close()` is the forced close (1006, no Close frame). `end()`, the graceful close, does not change. - #38243 fixed this wait for the client `WebSocket`. <details><summary>Notes</summary> **Repro** (1.4.3, canary c6b7fcb, and a debug build of main b8eacea). A `wss://` server sends one message. The client calls `pause()` and sends one message back. The server calls `ws.terminate()` in `message()`. ``` wss, before: 0.0s SERVER terminate(), readyState 3 12.0s done waiting (close handler never ran) ws, before: 0.0s SERVER close handler: 1006 "" 0.0s SERVER terminate(), readyState 3 wss, after: 0.1s SERVER close handler: 1006 "" 0.1s SERVER terminate(), readyState 3 ``` - With `idleTimeout: 8` and a silent peer, the unfixed server closes at 8.0 s from the idle timer, with the reason "WebSocket timed out from inactivity". - With `idleTimeout: 4` and a paused peer that sends one frame each second, the unfixed server never closes. In 14 s it delivered 14 messages to `message()` after `terminate()`, all with `ws.readyState === 3`. The uWS parser stops only when `us_socket_is_closed()` or `isShuttingDown` is true, and a deferred TLS close sets neither. - Protocol error: a paused `Bun.connect({ tls })` client sends a masked frame with opcode 3, and 300 ms later a valid text frame. Before: no `close`, and the text frame reaches `message()`. After: `close` 1006 at once. The `forceClose()` calls that pass a reason already close at once, because the reason length is the close code and it is never 0. **A peer that keeps up sees no change.** Matrix: `ws.send(payload); ws.terminate()` from a handler and from a timer, payload 100 B, 64 KB, 300 KB and 4 MB, 10 runs each, a Bun `WebSocket` client that reads. All 16 cells are the same for `ws` and `wss`, before and after: the same messages arrive, then `close 1006`, and no `error` event. A node `ws` client also reports `close 1006` with no `error` event, before and after. **A peer under send backpressure gets a reset.** A spill does not prove a dead peer. It means the last flush hit a full kernel buffer, which also happens with a slow peer that still reads. If the first close cannot drain the spill, the second close resets: the kernel drops its send buffer and the peer sees ECONNRESET. Before this change that peer got the kernel buffer and the spill, then close_notify, whenever the spill drained. Plain `ws://` gives it the kernel buffer and a FIN. The uWS backpressure buffer is lost in every variant, so the stream ends in the middle of a message in every variant. The reset is the documented cost of a close that must finish now (`CloseCode::failure` names `terminate()`). The client `WebSocket.terminate()` (#38243) and `Socket.terminate()` reset on every close. A FIN that keeps the kernel buffer needs a usockets close that drops the spill without a wait, which is the durable fix that #39548 names. **Why a FIN first and not a reset for every TLS close.** With a node TLS client, a server reset gives `error: ECONNRESET`. A bare FIN gives `end`, then `close` with no error, which is what `ws://` gives today. npm `ws` 8.18.3 does the same: its `terminate()` is `socket.destroy()`, and the comment in `us_internal_ssl_close` records that node's destroy sends a bare FIN. **Why the second close.** A build with `FAST_SHUTDOWN` only still hangs in the backpressure test (paused client, the server sends 64 KB messages until `send()` returns -1, then `terminate()`). `us_internal_ssl_close` defers codes 0 and 2 until the spill drains. The check after the first close detects the deferral and does not predict it. usockets first tries to drain the spill, so a peer that recovered still gets a FIN. A check of `us_socket_ssl_spill_pending()` before the close is not equivalent: it resets a peer that has recovered. #39548 records that. The read of the socket after the first close is safe. `us_internal_socket_close_raw` links a socket into the loop's closed list only after the close handler returns, so JS that re-enters the event loop from the handler cannot free it. **Other callers of `WebSocket::close()`** that get the same fix: the `open()` exception path in `ServerWebSocket.rs`, the HMR socket (`hmr_socket.rs`), and `DevServer` teardown, which has `debug_assert!(self.active_websocket_connections.is_empty())` right after it closes every socket. **Not fixed here.** These sites have the same exposure. Each needs its own repro and tests, and the durable fix is a usockets close that cannot defer (see the last Background bullet of #39548). - `AsyncSocket<SSL>::close()` (`AsyncSocket.h:101`) and the HTTP paths that use it. One case is reproduced: `Bun.serve({ tls, idleTimeout: 4 })` with a peer that completes the handshake and stops reading. The plain server socket is in FIN-WAIT-2 at 12 s. The TLS server socket is still ESTAB at 40 s. - The code 0 closes in `Http2Context.h`. - `JSNodeHTTPServerSocket::close()` (`JSNodeHTTPServerSocket.cpp:90`). It already uses `FAST_SHUTDOWN`, with no second close for a spill. - A peer FIN on a TLS WebSocket that still has a spill, and `closeOnBackpressureLimit`. These are not forced closes. **Order with #35874.** That PR replaces the `forceClose()` calls that pass no reason with a graceful `end(1002)`. If it merges first, the `reason.empty()` branch here has no caller and can go, and the reserved opcode test must expect the code that PR reports. If this PR merges first, #35874 needs no change to `close()`. **Test file.** The test is a sibling file and not part of `websocket-server.test.ts`. That file has load-related timeouts on a debug build (8 here, in `send()`, `sendText()`, `sendBinary()` and the benchmark). The same tests pass alone. The two `terminate()` tests fail at once on an unfixed build, with `close: 1006 ""` missing between `message` and `terminate() returned`. The reserved opcode test times out there. **Suites run** on a debug+ASAN build: all of `test/js/bun/websocket/`, `websocket.test.js`, `websocket-pause.test.ts`, `websocket-client.test.ts`, `websocket-close-code.test.ts`, `websocket-close-fragmented.test.ts`, `websocket-proxy.test.ts`, `websocket-subprotocol-strict.test.ts`, `websocket-client-short-read.test.ts`, `websocket-close-connecting.test.ts`, `websocket-close-async-dispatch.test.ts`, `websocket-buffered-amount.test.ts`, `websocket-server-send-from-drain.test.ts`, `ws.test.ts`, `ws-upgrade-events.test.ts`, and the pipelined upgrade tests in `serve.test.ts`. Failures that do not come from this change: two `websocket.test.js` tests that need `ws.postman-echo.com`, and tests that spawn a debug build per case and go over 5 s when this machine is loaded ("should connect many times over https", `ws-upgrade-events.test.ts`). They pass alone. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 3 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 3 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/websocket/websocket-server-forced-close.test.ts bun test v1.4.3 (c6b7fcb) test/js/bun/websocket/websocket-server-forced-close.test.ts: (pass) ws: forced close with a peer that does not read > terminate() runs the close handler before it returns [189.90ms] (pass) ws: forced close with a peer that does not read > terminate() runs the close handler before it returns under backpressure [156.98ms] (pass) ws: forced close with a peer that does not read > a frame with a reserved opcode closes the socket [139.63ms] 53 | client.onmessage = () => { 54 | client.pause(); 55 | client.send("terminate"); 56 | }; 57 | await terminated.promise; 58 | expect(events).toEqual(["message: terminate", 'close: 1006 ""', "terminate() returned"]); ^ error: expect(received).toEqual(expected) [ "message: terminate", - "close: 1006 """, "terminate() returned", ] - Expected - 1 + Received + 0 at <anonymous> (/workspace/bun/test/js/bun/websocket/websocket-server-f ... (truncated) release without fix: all passed bun test v1.4.3-canary.1 (fed761c) test/js/bun/websocket/websocket-server-forced-close.test.ts: (pass) ws: forced close with a peer that does not read > terminate() runs the close handler before it returns [18.47ms] (pass) ws: forced close with a peer that does not read > terminate() runs the close handler before it returns under backpressure [16.71ms] (pass) ws: forced close with a peer that does not read > a frame with a reserved opcode closes the socket [16.37ms] (pass) wss: forced close with a peer that does not read > terminate() runs the close handler before it returns [17.73ms] (pass) wss: forced close with a peer that does not read > terminate() runs the close handler before it returns under backpressure [11.70ms] (pass) wss: forced close with a peer that does not read > a frame with a reserved opcode closes the socket [10.75ms] 6 pass 0 fail 6 expect() calls Ran 6 tests across 1 file. [85.00ms] __F:0:S:0 ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/websocket/websocket-server-forced-close.test.ts bun test v1.4.3 (c6b7fcb) test/js/bun/websocket/websocket-server-forced-close.test.ts: (pass) ws: forced close with a peer that does not read > terminate() runs the close handler before it returns [278.56ms] (pass) ws: forced close with a peer that does not read > terminate() runs the close handler before it returns under backpressure [237.39ms] (pass) ws: forced close with a peer that does not read > a frame with a reserved opcode closes the socket [210.23ms] (pass) wss: forced close with a peer that does not read > terminate() runs the close handler before it returns [211.46ms] (pass) wss: forced close with a peer that does not read > terminate() runs the close handler before it returns under backpressure [155.50ms] (pass) wss: forced close with a peer that does not read > a frame with a reserved opcode closes the socket [71.47ms] 6 pass 0 fail 6 expect() calls Ran 6 tests across 1 file. [2.82s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 841ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/128] gen generated_host_exports.rs generated_host_exports.rs: 121 exports (host=5, lazy=10, generic=106, rust=0); 242 extern-C blocks audited [2/128] gen JSSink.{cpp,h,lut.h,rs} generated_jssink.rs: 7 sinks, 84 exported symbols Generating /workspace/bun/build/release/codegen/JSSink.lut.h from /workspace/bun/build/release/codegen/JSSink.lut.txt [3/128] gen cpp.rs (cppbind) [4/128] gen JS modules (bundle-modules) Preprocess modules (11568ms) Bundle modules (91ms) Postprocesss modules (179ms) Bundle Functions (587ms) Generate Code (41ms) [12.48s] Bundled "src/js" for production 2600 kb 197 internal modules 13 native modules 50 internal functions across 16 files [4/26] cargo bun_runtime → libbun_runtime.a �[1m�[33mwarning�[0m�[1m: binary `bun_shim_impl` should have a kebab-case name�[0m �[1m�[94m|�[0m �[1m�[94m 1�[0m �[1m�[94m|�[0m /workspace/bun/build/release/rust-target/.../bun_shim_impl �[1m�[94m|�[0m �[1m�[33m^^^^^^^^^^^^^�[0m �[1m�[94m|�[ ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` packages/bun-uws/src/WebSocket.h | 8 +- packages/bun-uws/src/WebSocketContext.h | 5 + .../websocket-server-forced-close.test.ts | 152 +++++++++++++++++++++ 3 files changed, 164 insertions(+), 1 deletion(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests packages/bun-uws/src/WebSocket.h 4 5 21 packages/bun-uws/src/WebSocketContext.h 2 3 20 …/js/bun/websocket/websocket-server-forced-close.test.ts 1 2 20 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: Ciro Spaciari <ciro.spaciari@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
When
Bun.serve's WebSocket parser detects a protocol violation (bad masking, reserved opcode/RSV bits, invalid fragment state, invalid UTF-8, oversized payload, or an invalid inbound Close body), it now writes a Close frame carrying the RFC 6455 7.4.1 status code before closing TCP, instead of tearing the socket down bare.Why
RFC 6455 7.1.7 says an endpoint that Fails the WebSocket Connection SHOULD send a Close frame with the appropriate status code before closing the underlying TCP connection. Every violation-detection branch in
WebSocketProtocol.h/WebSocketContext.hpreviously calledforceClose()= a bareus_socket_close(). A conforming client therefore only ever saw 1006 (abnormal closure) no matter why the server rejected the frame, and the diagnostic reason string only rode the socket-close side channel into the app'sclose(1006, "...")callback.Separately,
parseClosePayloadmapped an invalid inbound close code, an invalid-UTF-8 reason, or a 1-byte body to the sentinels{1006, ...}/{1005, ...}, whichformatClosePayloadrefuses to serialize, so the server answered with an empty-payload Close (peer sees 1005) instead of 1002/1007.How
Impl::failConnection(wState, s, code, reason)inWebSocketContext.hthat routes through the existingWebSocket::end()path: format Close payload,send(..., OpCode::CLOSE), setisShuttingDownso later inbound bytes are ignored, half-close (FIN) once drained.CLOSE_PROTOCOL_ERROR: masking mismatch, RSV1/RSV2/RSV3, reserved/unknown opcode, fragmented or >125-byte control frame, continuation with no message in progress, new data frame interleaved in a fragmented message.CLOSE_INVALID_DATA: TEXT frame or Close reason that is not valid UTF-8.CLOSE_MESSAGE_TOO_BIG: single frame, fragmented total, or post-inflation size overmaxPayloadLength.parseClosePayloadnow returns 1002 for an invalid/reserved code and for a 1-byte body (RFC 6455 5.5.1: if there is a body, its first two bytes MUST be the status code), and 1007 for an invalid-UTF-8 reason.forceClose()is retained only for the idle-timeout path.The app's
close()handler now receives the same code and reason that go out on the wire (matching thewsmodule) instead of1006plus the side-channel diagnostic.Verification
test/js/bun/websocket/websocket-server-fail-close-codes.test.tsis a raw RFC 6455 TCP client againstBun.serve({websocket})that asserts a Close frame (0x88) with the mandated code arrives on the wire before the TCP close, plus what the app handler observed. 16 violation cells + 3 controls.Existing
websocket-server-unmasked-frames.test.ts,websocket-server-rsv-frames.test.ts, and themaxPayloadLengthcase inwebsocket-server.test.tsare updated to expect the new codes (1002/1009) that the app handler now sees.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/websocket/websocket-server.test.ts