Conversation
…orced websocket close
…) and corked send()
…resets the connection
|
Updated 12:33 AM PT - Sep 22nd, 2026
✅ @robobun, your commit b0f092307bd88f15f5911d0711f2fc4996f22930 passed in 🧪 To try this PR locally: bunx bun-pr 43711That installs a local version of the PR into your bun-43711 --bun |
|
Status: ready for review. How I reproduced it: inside a websocket
|
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. Warning Review limit reached
This review includes 9 billable files and costs up to $2.25.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 6 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (9)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe change adds pending-write flush APIs across uWS and its Rust/C bridge. Shutdown paths flush queued publications and corked frames before closing. WebSocket tests cover same-tick close, process exit, and protocol errors. ChangesWebSocket write flushing
Suggested reviewers: Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
packages/bun-uws/src/WebSocketContext.h— Clients that half-close the TCP connection (send FIN, keep reading) still lose the publish() batch and corked send() frames queued for them in the same tick, while every server-initiated forced close now delivers them. onEnd at packages/bun-uws/src/WebSocketContext.h:381-385 calls uncorkWithoutSending and us_socket_close with no flushBeforeForcedClose. Fix: call flushBeforeForcedClose() in onEnd before the close, the same as forceClose at WebSocketContext.h:78, so the flush covers every forced-close path.Why this was flagged
A non-browser websocket client (node net-based clients, load-testing tools, proxies) shuts down its write side with FIN after sending its last frame and keeps reading. usockets dispatches onEnd (packages/bun-uws/src/WebSocketContext.h:381) on the still-readable socket; uncorkWithoutSending discards any frames a handler corked and us_socket_close runs onClose, which frees the subscriber at WebSocketContext.h:282 without draining. Messages the server published in the same tick that publish() reported as sent are never written although the socket can still deliver them. The base branch behaves the same, so this is not a regression, but the PR's own description names this path as a sibling of forceClose and WebSocket::close that was left without the flush; the dismissal that 'browser clients send a Close frame' does not cover the non-browser population that reaches onEnd. Remedy: invoke flushBeforeForcedClose() in onEnd before us_socket_close.
Verification: pre-existing — acknowledged in diff: the PR description says "Also not changed:
onEnd(the peer sent FIN) still discards the cork buffer and frees the subscriber without a drain"; that note is accurate for the publish batch. Triggering condition: a client writes a WebSocket frame followed by TCP FIN (e.g. anetclient doingsocket.end(frame)) or a peer's FIN lands in the same epoll batch…
WebSocket::close() now flushes the cork buffer, which after a synchronous upgrade still holds the 101 response. The open-exception path closes without the flush so the client does not see an open event.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed the latest push and found no new bugs; the throwing-open handler case from my earlier comment is now handled by close(false) / close_without_flush() with a covering test. Because this changes forced-close and process-exit ordering in the uWS socket lifecycle (C++ and Rust), a human look would still be worthwhile.
What was reviewed:
Loop::flushPendingWrites()invoked fromon_exit: the only registered pre handler is the App'stopicTree->drain(), and the cork drain callsuncork()only, so no user JS runs;uws_get_loop()/uws_get_loop_with_native()always createLoopData, so the per-thread loop is valid there.App::close()cork-slot uncork: restricted to sockets in this app's own websocket groups, so theAsyncSocket<SSL>cast matches the app's SSL mode; runs beforeus_socket_group_close_allso the fd is still open.flushBeforeForcedClose()inWebSocket::close()and parserforceClose: checksus_socket_is_closedfirst; both SSL and non-SSLuws_ws_closethread theflushflag.
Extended reasoning...
The change adds a flush step before forced websocket closes (ws.terminate(), parser protocol errors, server.stop(true)) and at process exit, touching packages/bun-uws/src/{App.h,Loop.h,WebSocket.h,WebSocketContext.h}, the C ABI in src/uws_sys/libuwsockets.cpp, the Rust extern signatures in src/uws_sys/{Loop.rs,WebSocket.rs}, the exit path in src/jsc/VirtualMachine.rs, and the handler-threw path in src/runtime/server/ServerWebSocket.rs. It touches no auth, crypto, or input-parsing surface; the sensitive part is socket/cork lifetime ordering in native code. The latest commit addressed the earlier inline finding about the throwing open handler flushing the corked 101, and six new tests cover the variants; the bug hunt ran dry with no findings. Deferring rather than approving because native socket-lifecycle code and a new exit-path hook are the kind of change a maintainer should sanity-check, and the pre-existing close-handler publish drop noted earlier remains intentionally unaddressed per the PR description.
This branch has not been deployed
2 participants
Problem
server.publish()of a message under 16 KB returns the byte count, but no subscriber receives it whenserver.stop(true),ws.terminate(),server.unref()orprocess.exit()follows in the same tick.ws.send()inside a handler is lost the same way.us_socket_closecloses the fd beforeWebSocketContext::onCloseruns (packages/bun-uws/src/WebSocketContext.h:269).onClosefrees the subscriber without a drain and discards the cork buffer.TopicTree(App.h:533).Fix
WebSocket::flushBeforeForcedClose()drains the socket's subscriber, then uncorks.WebSocket::close()(ws.terminate()) and the parser'sforceClosecall it beforeus_socket_close. Anopenhandler that throws usesclose(false), so the corked 101 stays unsent.App::close()(server.stop(true)) drains theTopicTreeand uncorks its corked websocket before it closes the groups.Loop::flushPendingWrites()runs the uWS pre handler.VirtualMachine::on_exitcalls it after theexitlisteners.publish()andsend()already reported as sent. Bun.serve websocket: deliver queued publish() messages when unsubscribing from last topic #32852 drains the same way. Verified: six new tests intest/js/bun/websocket/websocket-server.test.ts. Five fail on bun 1.4.2. All pass on a debug build. Self-reviewed: 3 concerns raised, 1 addressed (Notes).Background
TopicTreeis the uWS pub/sub index. A smallpublish()is stored once, with an index per subscriber. A loop pre or post handler, or the socket's nextsend(), writes the batch.send()frames go out in one write when the handler returns.ws.terminate(),server.stop(true)and parser errors close the TCP socket at once, with no Close frame.Notes
Repro for the first case (from the report):
Controls that always delivered:
ws.send()from a timer, a publish of 16 KB or more (it bypasses the batch),ws.close()after the publish,setTimeout(() => server.stop(true), 0). The batch also writes itself out at the 32nd pending message, so 40 small publishes thenstop(true)delivered exactly the first 32.Test cases added:
send()andpublish()thenserver.stop(true)inside a message handler. The handler's socket is corked, so its publishes drain into the cork buffer. It must receivesent, bye0, bye1, bye2. The other subscriber must receivebye0, bye1, bye2.ws.terminate()on every socket.server.unref()orprocess.exit(0). Skipped on Windows: the exit resets the connection there and the reset can discard what the client has not read, the same reasonserve.test.tsskips its process-exit test.openhandler that sends a frame and then throws, after a synchronousserver.upgrade(). The raw client must receive no response at all. This passes on bun 1.4.2. It fails on the first version of this PR, whereclose()flushed the corked 101 and the client sawopen, the frame, then a 1006 close.on_exitruns for a natural exit,process.exit(), and a worker that exits on its own. A worker that its parent terminated runs no script, and the flush skips it.on_exitusesuws::Loop::get()and notvm.uws_loop():Bun.spawnSyncswapsevent_loop_handleto an isolated loop that has no uWSLoopData. The drain callback only corks, sends and uncorks, so the flush runs no JavaScript.Self-review concerns:
wss://,stop(true)ends with a reset after the TLS close_notify (packages/bun-usockets/src/context.c:127). That is the existing behavior for every byte written beforestop(true),ws.send()included.publish()made from aclosehandler whilestop(true)walks the sockets is still dropped for the sockets that close later in the same walk. A flush there needs a hook before eachus_socket_closeinsideus_socket_group_close_all.Also not changed:
onEnd(the peer sent FIN) still frees the subscriber without a drain. On Linux the FIN is dispatched in a later loop iteration than the data before it (usockets does not registerEPOLLRDHUP), so the loop's own drain runs first and a test for it passes with or without a flush there. kqueue can report EOF on the data's event (packages/bun-usockets/src/loop.c, theeofdrain), so a half-closing client on macOS can still lose a same-tick publish. I could not produce a failing case on this machine, so that path is left for a separate change.#43013 edits the same lines of
WebSocket::close()andforceClosefor a different bug (a TLS forced close that waits for the peer). The two changes compose: the flush goes before whicheverus_socket_closecall runs.Suites run on the debug build:
test/js/bun/websocket/,test/js/bun/http/serve.test.ts,bun-server.test.ts,serve-listen.test.ts, six files oftest/js/web/websocket/,test/js/web/workers/worker.test.ts,test/js/node/process/process.test.js.cargo check --target x86_64-pc-windows-msvc -p bun_uws_sys -p bun_jscpasses.Local failures that this diff does not cause:
websocket-server.test.ts: eightsend()/sendText()/sendBinary()tests time out at 10 s on the debug build while the concurrent(benchmark)test runs for 29 s. Each passes alone.test/js/first_party/ws/: debug panicassertion failed: !self.body_read_ref.get().has, the subject of node:http: release body_read_ref when a response upgrades to a WebSocket #43427.serve.test.ts: two tests need a non-root user and open egress.process.test.js: one test needsUSERin the environment.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