node:http: emit 'upgrade' on ClientRequest for 101 responses#32204
node:http: emit 'upgrade' on ClientRequest for 101 responses#32204robobun wants to merge 2 commits into
Conversation
A 101 Switching Protocols response was delivered as a regular 'response' event with no way to reach the raw connection, so every client that drives a WebSocket-style handshake through http.request() hung waiting for 'upgrade'. The native client already supports the whole flow (an Upgrade header sets upgrade_state = Pending, a 101 stops HTTP parsing and streams the raw bytes as the response body, and streaming request-body writes skip chunked framing after the upgrade); only the JS ClientRequest never used it. Detect upgrade requests, keep the request body generator alive past end(), and on a 101 emit 'upgrade' with a Duplex socket that reads from the response body stream and writes through the body generator. With no 'upgrade' listener the connection is destroyed, matching Node. Non-101 responses to upgrade requests release the generator so the request can finish.
|
Warning Review limit reached
More reviews will be available in 6 minutes and 47 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ 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 (2)
Comment |
|
Updated 12:20 PM PT - Jun 12th, 2026
❌ @robobun, your commit 01d9361 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32204That installs a local version of the PR into your bun-32204 --bun |
|
Found 7 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Closing as a duplicate of #28828, which I missed when scanning for prior work (the dedupe bot caught it): same architecture (gate duplex on the Upgrade header, Duplex socket bridging the fetch streams), maintainer-driven, and with a much deeper test suite, including the real-ws and connectOverCDP regression tests. It is mergeable and waiting on review. Two data points from this PR that may still be useful:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| res[noBodySymbol] = true; | ||
| res.complete = true; | ||
| process.nextTick( |
There was a problem hiding this comment.
🟡 The IncomingMessage passed to the 'upgrade' event is missing res.upgrade = true. Node sets this in parserOnHeadersComplete (and Bun mirrors that at _http_common.ts:105 for the parser path), but the fetch-based path here never assigns it, so listeners see undefined for the documented message.upgrade property. One-line fix: add res.upgrade = true; next to res.complete = true;.
Extended reasoning...
What the bug is
Node's documented message.upgrade property is supposed to be true on the IncomingMessage handed to an 'upgrade' listener. In Node this is set by parserOnHeadersComplete in lib/_http_common.js (parser.incoming.upgrade = upgrade), and Bun already mirrors that for the HTTP-parser path at src/js/node/_http_common.ts:105 (incoming.upgrade = upgrade;).
This PR introduces a new path that constructs the IncomingMessage directly from a fetch Response (the NodeHTTPIncomingRequestType.FetchResponse constructor branch), bypassing parserOnHeadersComplete. The isUpgradeResponse block sets res[noBodySymbol] = true and res.complete = true, but never sets res.upgrade. A grep of src/js/node/_http_incoming.ts confirms IncomingMessage has no default for this property, so it stays undefined.
Code path
// src/js/node/_http_client.ts (this PR)
if (isUpgradeResponse) {
res[noBodySymbol] = true;
res.complete = true;
// ↓ missing
// res.upgrade = true;
process.nextTick((self, res) => {
...
self.emit("upgrade", res, socket, kEmptyBuffer);
}, this, res);
}Step-by-step proof
- Client sends a request with
Upgrade: websocket; server replies101 Switching Protocols. nodeHttpClient(...).then(response => ...)runs withresponse.status === 101, soisUpgradeResponseistrue.handleResponse()constructsres = new IncomingMessage(response, { [typeSymbol]: FetchResponse, ... }). Neither theIncomingMessageconstructor (noupgradereference in_http_incoming.ts) nor this block assignsres.upgrade.self.emit("upgrade", res, socket, head)fires.- In the listener,
res.upgradeevaluates toundefined. In Node it istrue.
Why existing code doesn't prevent it
The only place Bun sets .upgrade on an IncomingMessage is _http_common.ts:105, inside the llhttp-driven parserOnHeadersComplete. ClientRequest is fetch-based and never goes through that parser; before this PR the 'upgrade' event was never emitted on ClientRequest, so the gap was unobservable. This PR creates the path where res.upgrade becomes observable but doesn't populate it.
Impact
Low. Code already inside an 'upgrade' handler knows it's an upgrade, and the major consumers (ws, Playwright's CDP client) inspect res.statusCode / res.headers, not res.upgrade. But it is a documented Node API, the PR's purpose is Node-compat for upgrades, and if (res.upgrade) is a reasonable thing for user code to write — it would be falsy here and truthy in Node.
Fix
Add one line alongside the existing assignments:
res[noBodySymbol] = true;
res.complete = true;
res.upgrade = true;| destroy: err => { | ||
| endUpgradeBody(); | ||
| const callback = upgradeWriteCallback; | ||
| upgradeWriteChunk = upgradeWriteCallback = undefined; | ||
| callback?.(err); | ||
| }, |
There was a problem hiding this comment.
🟡 When the upgrade socket is destroyed without an error (e.g. socket.destroy() with no arg, or via the abort-signal listener), the pending write callback is invoked with undefined, which Writable treats as success — so a user's socket.write(data, cb) fires cb(null) even though upgradeWriteChunk was cleared and never yielded to the body generator. Consider callback?.(err ?? $ERR_STREAM_DESTROYED('write')) so an in-flight write is errored on teardown.
Extended reasoning...
What the bug is
In createUpgradeSocket's channel.destroy handler, when the upgrade socket is destroyed without an error, any pending _write callback is invoked with err = undefined:
destroy: err => {
endUpgradeBody();
const callback = upgradeWriteCallback;
upgradeWriteChunk = upgradeWriteCallback = undefined;
callback?.(err); // err is undefined when socket.destroy() had no arg
},That callback is Writable's internal onwrite. Calling onwrite(undefined) tells the Writable machinery the chunk was written successfully, which then fires the user's socket.write(data, cb) callback with no error — even though upgradeWriteChunk was cleared on the line above and was never yielded to the body generator, so the bytes never reached the wire.
How it triggers — step-by-step proof
- User calls
socket.write(data, cb)on the upgrade socket. Writable invokesUpgradeSocket._write(chunk, enc, onwrite), which callschannel.write(chunk, onwrite). channel.writesynchronously setsupgradeWriteChunk = chunkandupgradeWriteCallback = onwrite, then callswakeUpgradeBody()to resolveupgradeWake. The body generator is parked onawait new Promise(...), so it won't actually pick the chunk up until a microtask runs.- In the same tick (before that microtask), user calls
socket.destroy()with no argument — orreq.abort()/req.setTimeoutfires, which abortskAbortController, whose listener callssocket.destroy()with no argument. Duplex.prototype.destroysetsstate.destroyed = trueand synchronously callsUpgradeSocket._destroy(undefined, cb), which callschannel.destroy(undefined).channel.destroycallsendUpgradeBody()(setsupgradeBodyEnded = true), capturescallback = onwrite, clearsupgradeWriteChunk/upgradeWriteCallback, then callsonwrite(undefined).- Writable's
onwrite(undefined)takes the success path →afterWrite→ user'scb(null). - When the generator's microtask later resumes,
upgradeWriteCallbackis alreadyundefinedandupgradeBodyEndedistrue, so the loop breaks without everyielding the chunk.
Net result: the user is told the write succeeded, but the data was dropped.
Why existing code doesn't prevent it
channel.write does guard with if (upgradeBodyEnded) callback($ERR_STREAM_DESTROYED('write')), but that only covers writes after destroy. The race here is a write that was accepted before destroy and is still pending when destroy runs. Writable also won't supply its own ERR_STREAM_DESTROYED here because _write already handed the chunk to onwrite before destroyed was set.
Impact
Edge case during teardown: socket.write(d, cb); socket.destroy(); in the same tick, or an abort/timeout landing while a write is in flight. Real upgrade consumers (ws, playwright) track socket 'close' rather than relying on per-write callback errors during destroy, so this is unlikely to break them — hence nit. But it is a correctness divergence from Node's net.Socket, which errors the in-flight write when the handle closes, and it's in new code with a one-token fix.
Fix
destroy: err => {
endUpgradeBody();
const callback = upgradeWriteCallback;
upgradeWriteChunk = upgradeWriteCallback = undefined;
callback?.(err ?? $ERR_STREAM_DESTROYED('write'));
},1 participant
This is the client half of #32195 (the server half, raw socket writes after the server's
'upgrade'event, is #30664). Fixes #18945.Reproduction
Node prints
got upgraded!and exits. Bun emits'response'(status 101) instead, never fires'upgrade', and hangs forever. This breaks every client that drives a websocket handshake throughhttp.request(): the realwspackage fails withUnexpected server response: 101, playwright'sconnectOverCDPtimes out, etc.Root cause
ClientRequestis fetch-based and has no 101 handling: the only delivery path isself.emit("response", res). The native side already supports the whole flow (anUpgrade:header setsupgrade_state = Pendingin src/http/lib.rs, a 101 stops HTTP parsing and streams the raw bytes as the response body, streaming request-body writes skip chunked framing after the upgrade, and ending the body stream shuts the connection down), but nothing on the JS side used it;kUpgradeOrConnectwas initialized and never read.Fix
All in
src/js/node/_http_client.ts:Upgradeheader other than h2/h2c)req.end(), so the connection retains a writable channelupgrade(res, socket, head)instead ofresponse:socketis aDuplexthat reads from the fetch response body (the raw post-upgrade bytes) and writes through the body generator, which the native client writes unframed after the 101; write acks ride the sink's pulls so native backpressure reaches the socket'upgrade'listener, destroy the connection and emit no'response', matching NodeEvent order (
socket,finish,upgrade,close),res.complete,socket === req.socket, andreq.destroyedwere verified side by side against Node v24.3.0 (reference:lib/_http_client.jssocketOnData).headis always empty: bytes that arrived together with the 101 flow through the socket instead. Upgrade consumersunshift(head)back onto the socket before reading, so both shapes behave the same.Verification
test/js/node/http/node-http-client-upgrade.test.ts, 8 tests against rawnet/tlsservers so that only the client side is exercised: echo roundtrip plus res/socket invariants, Node event order, no-listener destroy, 101+data in one packet, non-101 completion, flushHeaders-without-end, TLS, and a spawned child proving the process exits instead of hanging.Also verified the real
ws@8.18.3client (required by path to bypass the bundled shim) completes open/send/echo/close against a Node ws server: fails withUnexpected server response: 101on bun 1.4.0, works with this change.test/js/node/http/suite has no new failures (the pre-existing proxy and uaf failures reproduce on main).Not covered: request bodies sent before the 101 are still held back by the native client while the upgrade is pending (
write_to_streamreturns early in thePendingstate), so the dockerode "send a chunked body, then read the 101" flow (#29012) still deadlocks. That part needs a native-side change.Supersedes #28470 (reroutes upgrade requests through a JS-parsed
net.connectpath, stalled since March) and #29015 (same fetch-based approach, but its native half targets the dormant.zigreference files from before the Rust port).Related issues (scoped, not auto-closed)
Of the issues the bot flagged, none is fully closed by this PR alone:
websocketnpm package): the client half is fixed here, verified locally (before:connectFailed Error: Server responded with a non-101 status: 101 Switching Protocols; after: connects and echoes, same output as Node). The issue also reports the library's server side broken, which is node:http: hand off upgrade socket to userland #30664.@cloudflare/vite-pluginand Bun ^1.3 #24229: these hit the bundledwsshim (ws.WebSocket 'upgrade' event is not implemented), which wraps Bun's native WebSocket and never goes throughhttp.request(); that is ws: emit clientupgradeevent with the handshake response #31408's territory, not this PR.