ws: emit client upgrade event with the handshake response#31408
ws: emit client upgrade event with the handshake response#31408robobun wants to merge 14 commits into
upgrade event with the handshake response#31408Conversation
The `ws` client shim hardcoded the `upgrade` event as "not implemented"
and printed a compatibility warning instead of firing it. Node's `ws`
emits `upgrade` with the HTTP handshake response (an `IncomingMessage`)
right before `open`, so code that waits for `upgrade` (e.g. to read
handshake response headers) broke under Bun.
The native WebSocket client already parsed the 101 handshake response but
never forwarded it. This surfaces it:
- `WebSocketUpgradeClient` forwards the parsed 101 response (status line +
headers) to the C++ `WebSocket` right before `didConnect`, while the
parse buffer is still valid. A `handshake`/`upgrade` handler that
synchronously closes the socket is handled by the existing
`!tcp.is_closed() && has_ws` re-check (cancel() clears
outgoing_websocket), so `didConnect` is skipped.
- `WebSocket::didReceiveHandshakeResponse` dispatches a `handshake`
MessageEvent carrying `{ statusCode, statusMessage, rawHeaders, body }`.
It is a no-op unless a `handshake` listener is registered, so the
browser-style `new WebSocket()` path pays nothing.
- The `ws` shim lazily wires that native listener when the user subscribes
to `upgrade`, builds a minimal `http.IncomingMessage` from the response,
and emits `upgrade` before `open`.
`unexpected-response` and `redirect` remain stubbed; they require
forwarding non-101 responses, which the native client fails before this
point.
Closes #31406
|
Warning Review limit reached
More reviews will be available in 36 minutes and 30 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds end-to-end WebSocket upgrade/handshake support: C++ types and event, Rust FFI bridge, HTTP client forwarding with delayed overflow-pointer extraction, JS IncomingMessage construction and lazy listener wiring, and tests ensuring upgrade fires before open with correct handshake data. ChangesWebSocket upgrade event implementation
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 10:27 PM PT - May 25th, 2026
❌ @robobun, your commit 7455d72 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 31408That installs a local version of the PR into your bun-31408 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The upgrade-event tests were in ws.test.ts alongside tests that spawn a subprocess echo server per case. Those subprocess tests time out under the slow ASAN debug build (1000ms timeout vs >2s cold spawn), so the file never reaches a clean pass there regardless of the fix. Move the upgrade-event tests into ws-upgrade.test.ts, which uses only in-process servers and runs fast under ASAN.
|
CI note for maintainers: the red This PR's diff is limited to the WebSocket client ( Not pushing a retrigger since the affected lanes already recovered on their own. |
…atch node headers prototype Two review fixes: - WebSocketUpgradeClient: the bytes trailing the 101 header block were leaked across FFI (`heap::into_raw`) before the `!tcp.is_closed() && has_ws` re-check. When an `upgrade` handler synchronously closes the socket, `cancel()` clears `outgoing_websocket`/closes `tcp`, so the re-check routes into an else-arm that never calls `did_connect` — the only consumer that reclaims the buffer — leaking it whenever the server piggybacks frame data on the 101 write. Keep the buffer as an owned `Box<[u8]>` and only leak it across FFI inside the success arms, right before `did_connect`/`did_connect_with_tunnel`; every other path drops it normally. - ws.js: `makeHandshakeResponse` built `res.headers` with a null prototype, but node's `http.IncomingMessage.headers` inherits from Object.prototype (so `res.headers.hasOwnProperty(...)` works). Use a plain object and `Object.hasOwn` for the header-folding dup-check so a header named "constructor" isn't confused with Object.prototype.constructor.
The `upgrade` event's IncomingMessage body is always built from `null` in the shim (for a 101 the trailing bytes are the first WebSocket frame, not an HTTP body, and are delivered via did_connect's overflow handoff). Passing `remain_buf` to did_receive_handshake_response made C++ allocate a Uint8Array copy of those bytes that is immediately discarded. Pass an empty slice instead; the C++ body parameter stays for a future unexpected-response.
…Listener `#onOrOnce` (which wires the native `#ws` listener that drives `this.emit`) was only reached from `on`/`once`. Subscribing via `addListener`, `prependListener`, or `prependOnceListener` — all distinct EventEmitter prototype methods — registered the user listener but never wired the native listener, so the event (including the new `upgrade`) silently never fired, unlike node + npm ws. Extract the native-wiring side effect into `#armNativeBridge(event, once)` and call it from all subscription entry points: `addListener` (an alias of `on`) routes through `#onOrOnce`; `prependListener`/`prependOnceListener` arm the bridge then delegate to the matching `super` method for correct ordering. This fixes `upgrade` via every subscription method and also closes the same pre-existing gap for open/close/message/error/ping/pong. Adds tests covering `upgrade` via addListener/prependListener/prependOnceListener.
|
CI update (build #58061, sha 8f8670c): the red This PR's diff is limited to the WebSocket client + Not pushing a retrigger — it would only re-roll the same unrelated flaky Windows test without changing this diff's (green) result. This needs a maintainer to merge past the flaky lane. |
`did_receive_handshake_response` and `did_connect` each wrap their FFI call in their own event-loop `enter()/exit()`, and the uWS poll that drives `handle_data` runs at `entered_event_loop_count == 0`. So the `exit()` after the `upgrade` dispatch hit count 1 and drained microtasks / `process.nextTick` before `did_connect` fired `open` — a microtask/nextTick scheduled in the `upgrade` handler observed CONNECTING (and `ws.send` from it hit InvalidStateError). Node + ws emit `upgrade` and `open` from the same socket-data turn with no checkpoint between them. Bracket the handshake dispatch and the `did_connect`/`did_connect_with_tunnel` handoff in one outer `EventLoop::enter_scope` guard so the inner pairs nest (count stays ≥ 1, no drain); the drain happens when the guard drops at function end, after `open`. The guard holds only the VM-owned loop pointer, so the trailing derefs that may free `this` stay safe. Adds a test asserting a queueMicrotask/process.nextTick from the `upgrade` handler observes OPEN (fails without the scope guard).
Adds a consumer-level test for the microtask-ordering fix: a process.nextTick(() => ws.send(...)) from the upgrade handler runs after open (socket OPEN), so the send is delivered rather than hitting InvalidStateError while still CONNECTING.
…idge Commit 8f8670c extracted the native-wiring into #armNativeBridge, which is now the method that calls #ensureHandshakeListener (and prependListener/ prependOnceListener reach it without going through #onOrOnce). Update the comment accordingly.
Replace the open-coded unsafe { EventLoop::enter_scope(vm.event_loop()) }
with the VirtualMachine::enter_event_loop_scope() safe wrapper — semantically
identical, drops an unsafe block, and matches the convention used elsewhere
(ServerWebSocket, RequestContext, dns, AbortSignal, ...).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/thirdparty/ws.js (1)
105-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace
Object.hasOwnwith intrinsichasOwnProperty.$callinws.jsheader foldingIn
src/js/thirdparty/ws.js(around Lines 105-110), replaceObject.hasOwn(headers, lower)with the repo’s intrinsic-styleObject.prototype.hasOwnProperty.$call(headers, lower)to matchsrc/jshardening conventions.♻️ Proposed fix
- const seen = Object.hasOwn(headers, lower); + const seen = Object.prototype.hasOwnProperty.$call(headers, lower);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/thirdparty/ws.js` around lines 105 - 110, Replace the use of Object.hasOwn in the header folding loop: instead of calling Object.hasOwn(headers, lower) update the check to use the repo hardening intrinsic style by calling Object.prototype.hasOwnProperty.$call(headers, lower) so that the variables involved (headers, lower, rawHeaders) and the surrounding loop in ws.js remain unchanged; locate the occurrence inside the block that defines const headers = (res.headers = {}); and const lower = rawHeaders[i].toLowerCase(); and replace only the predicate expression to use hasOwnProperty.$call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/first_party/ws/ws-upgrade.test.ts`:
- Around line 139-142: Replace the disallowed setTimeout call inside the
ws.on("open", ...) handler with a test-safe macrotask barrier by making the
callback async and awaiting Bun.sleep(0) before calling resolve(states as {
microtask: number; nextTick: number }); this keeps the same “next turn” behavior
without using timers; reference the ws.on("open", ...) callback, the resolve
call, and the states variable when making the change.
---
Outside diff comments:
In `@src/js/thirdparty/ws.js`:
- Around line 105-110: Replace the use of Object.hasOwn in the header folding
loop: instead of calling Object.hasOwn(headers, lower) update the check to use
the repo hardening intrinsic style by calling
Object.prototype.hasOwnProperty.$call(headers, lower) so that the variables
involved (headers, lower, rawHeaders) and the surrounding loop in ws.js remain
unchanged; locate the occurrence inside the block that defines const headers =
(res.headers = {}); and const lower = rawHeaders[i].toLowerCase(); and replace
only the predicate expression to use hasOwnProperty.$call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f36e6ea0-7a13-4ff6-af03-ef7851577404
📒 Files selected for processing (3)
src/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/js/thirdparty/ws.jstest/js/first_party/ws/ws-upgrade.test.ts
The test suite disallows setTimeout; Bun.sleep(0) gives the same next-turn checkpoint for asserting the microtask/nextTick ran after open.
|
CI update (build #58085, sha 8bead67): same outcome as every prior build on this PR — the only red is the flaky That test is unrelated to this PR: the diff is limited to the WebSocket client ( Every relevant lane is green, including Not pushing a retrigger — it would only re-roll the same oscillating Windows test without changing this diff's (green) result. This needs a maintainer to merge past the flaky lane. |
|
CI update (build #58085): investigated the two red lanes; both are unrelated infra/flake, not regressions from this PR.
My last commit was a test-only |
There was a problem hiding this comment.
All prior review threads are resolved and I found nothing new this pass, but given the cross-FFI surface (Rust ↔ C++ ↔ JS), reentrant JS dispatch during process_response, and the overflow-buffer ownership refactor, this warrants a maintainer's eyes before merge.
Extended reasoning...
Overview
This PR implements the ws client upgrade event by plumbing the parsed 101 handshake response from Rust (WebSocketUpgradeClient::process_response) through a new C++ FFI entry point (WebSocket::didReceiveHandshakeResponse) into a lazily-registered native handshake listener that the JS ws shim folds into an IncomingMessage-shaped object. It touches 8 files across three languages: a new #[repr(C)] header struct + FFI wrapper in CppWebSocket.rs, a new JS-dispatching call site plus an overflow-buffer ownership refactor and an outer event-loop scope guard in WebSocketUpgradeClient.rs, a new MessageEvent dispatch path in WebSocket.cpp/h, a new event name + three builtin identifiers, the ws.js shim's makeHandshakeResponse / #armNativeBridge / addListener/prependListener overrides, and a new 179-line test file.
Security risks
None identified. Header bytes flow from PicoHTTP's parser through borrowed slices into WTF::String copies; there's no shell/SQL/path interpolation. The res.headers object is now a plain {} with Object.hasOwn dup-checks, so a server-supplied header named __proto__/constructor becomes an own data property rather than a prototype write. The new handshake event is internal-only (not on the public WebSocket IDL surface) and gated on hasEventListeners.
Level of scrutiny
High. This is the first place process_response dispatches into JS before did_connect, which makes the previously-defensive !tcp.is_closed() && has_ws else-arms reachable via reentrant ws.close()/terminate(). The PR correctly refactored overflow-buffer ownership to an owned Option<Box<[u8]>> leaked only at the handoff point (fixing a leak I flagged earlier), and bracketed the two dispatches in one enter_event_loop_scope() so microtasks don't drain between upgrade and open (matching Node). Both fixes look correct and are ASAN-verified per the author, but the combination of raw-pointer FFI, reentrancy into cancel(), event-loop nesting semantics, and the _event_loop_scope guard's drop ordering relative to the trailing tcp.close()/terminate() derefs is subtle enough that a maintainer familiar with the WebSocket client lifecycle should sign off.
Other factors
I left seven inline comments across earlier revisions (one 🔴 leak, six 🟡 nits); all were addressed or reasonably declined, and all threads are resolved. CodeRabbit's one suggestion was applied. The new test file covers ordering, custom headers, once/addListener/prependListener paths, and the microtask-ordering invariant. CI is green on the relevant lanes (including ASAN); the only red is an unrelated Windows transpiler flake. Two prior PRs (#25777, #28114) attempted the same feature, so a maintainer may also want to reconcile/close those.
|
CI update (build #58123): the two red lanes are unrelated flake, not regressions from this PR's diff. This build is a no-op
This PR's diff is limited to the WebSocket client ( |
|
CI update (build #58123, final): every red lane on this commit also has a green pass recorded on the same commit — this is non-deterministic flake, not a regression from this diff. Commit Per-lane history on
Identical source producing both passes and fails on the same lanes is flake by definition. None are reproducible failures from this PR. I also re-verified there's no ws regression: Diff scope: WebSocket client ( |
1 participant
Problem
Bun's
wsclient shim hardcoded theupgrade(andunexpected-response/redirect) event as "not implemented" and printed a compatibility warning instead of firing it. Node'swsemitsupgradewith the HTTP handshake response (anhttp.IncomingMessage) right beforeopen, so code that waits forupgrade— e.g. to read handshake response headers before resolving a WebSocket-backed connection — broke under Bun.Fixes #31406.
Reproduction
Before (Bun): prints
[bun] Warning: ws.WebSocket 'upgrade' event is not implemented in bunthenopen upgrade=false, exit 1.After (matches Node):
upgrade event emittedthenopen upgrade=true, exit 0.Cause
The native WebSocket upgrade client (
WebSocketUpgradeClient) already parsed the 101 handshake response (status line + headers) but never forwarded it to the JSWebSocket, so the shim had nothing to build theupgradeevent'sIncomingMessagefrom. The shim therefore stubbed the event with a warning.Fix
WebSocketUpgradeClientforwards the parsed 101 response (status code, reason phrase, header list) to the C++WebSocketright beforedidConnect, while the parse buffer is still valid. Dispatching beforedidConnect(stillCONNECTING) matches node's ordering; ahandshake/upgradehandler that synchronously closes the socket is handled by the existing!tcp.is_closed() && has_wsre-check (cancel()clearsoutgoing_websocket, sodidConnectis skipped).WebSocket::didReceiveHandshakeResponsedispatches ahandshakeMessageEventcarrying{ statusCode, statusMessage, rawHeaders, body }. It early-returns unless ahandshakelistener is registered, so the browser-stylenew WebSocket()path pays nothing.src/js/thirdparty/ws.jslazily wires that nativehandshakelistener when the user subscribes toupgrade, folds therawHeadersinto anhttp.IncomingMessage-shaped object (vianode:stream'sReadable, withset-cookiekept as an array like Node), and emitsupgradebeforeopen. Theupgradewarning stub is removed.unexpected-responseandredirectremain stubbed — they require forwarding non-101 responses, which the native client fails before this point.Verification
New tests in
test/js/first_party/ws/ws.test.ts(upgrade eventdescribe):upgradefires beforeopen, and its argument is the handshake response (statusCode === 101,statusMessage, standard headers,rawHeadersarray).set-cookieas an array) are surfaced, using aBun.serveserver that sets them.once("upgrade")works.These pass with the debug build and time out (event never fires) under
USE_SYSTEM_BUN=1, confirming they exercise the fix. Verified reentrantclose()/terminate()inside theupgradehandler and a 50-connection stress loop are clean under the ASAN debug build.Related issues
upgradeandunexpected-responseevents. This PR implementsupgrade;unexpected-response(non-101 responses) is still stubbed, so this does not fully close it.@cloudflare/vite-pluginand Bun ^1.3 #24229 — Vite +@cloudflare/vite-pluginfails with theupgrade/unexpected-response"not implemented" warnings. This PR removes theupgradewarning and fires the event; whether that alone unblocks the plugin (vs. also needingunexpected-response) would need confirmation against that repro.