node:http: hand off upgrade socket to userland by robobun · Pull Request #30664 · oven-sh/bun

robobun · 2026-05-14T05:50:05Z

Fixes #30661. Supersedes #28158 / #27237 (both stalled and conflicted against current main).

Reproduction

// repro.mjs — same code the rsbuild issue hits via bundled `ws`
import http from 'node:http';
const server = http.createServer((req, res) => { res.writeHead(200); res.end('ok'); });
server.on('upgrade', (req, socket) => {
  socket.write(
    'HTTP/1.1 101 Switching Protocols\r\n' +
    'Upgrade: websocket\r\n' +
    'Connection: Upgrade\r\n' +
    'Sec-WebSocket-Accept: s3pPLMBiTxaQ9kYGzzhZRbK+xOo=\r\n\r\n'
  );
});
server.listen(0, () => console.log('PORT:', server.address().port));

$ curl -v --max-time 3 -H 'Connection: Upgrade' -H 'Upgrade: websocket' \
       -H 'Sec-WebSocket-Version: 13' -H 'Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==' \
       http://127.0.0.1:$PORT/
# Before: "Operation timed out with 0 bytes received" (server says it wrote 101; client sees nothing)
# After:  HTTP/1.1 101 Switching Protocols ... `

This bites every ws-based stack — rsbuild, vite, webpack-dev-server, http-proxy, socket.io — because they all write the switching-protocols line to the socket themselves. Bun's built-in ws shim uses server.upgrade() directly and sidesteps it, which is why WebSocketServer({ server }) appeared to work.

Root cause

Two bugs stacked on top of each other:

NodeHTTPServerSocket._write gates on handle.ondrain. The upgrade path fell through socket[kEnableStreaming](false) (line 570) just before the is_upgrade branch, so ondrain was undefined. Every write became a silent no-op with a successful callback — the handshake never reached the socket.
uWS kept owning the connection as an HTTP stream. Inbound bytes after the 101 were parsed as new HTTP requests and answered with 400 Bad Request (or dropped). Only CONNECT flipped HttpResponseData::isConnectRequest = true, which is the flag that puts uWS into pass-through mode.

Fix

Mirror the CONNECT handoff exactly:

socket[kEnableStreaming](true) — wire handle.ondata / ondrain so _write actually calls us_socket_buffered_js_write and reads surface to the Duplex.
socketHandle.markAsRawMode() — new host function on JSNodeHTTPServerSocket; sets isConnectRequest = true on the uWS HttpResponseData so uWS stops HTTP-parsing and routes bytes through onSocketData.
Return a promise that settles on socket.once('close') instead of falling through to socket.cork() and the response lifecycle.
Preserved: upgrade with no 'upgrade' listener still falls through to 'request', matching Node ("By default all HTTP upgrade requests are ignored (i.e. only regular 'request' events are emitted)").

Tests

test/js/node/http/node-http-with-ws.test.ts adds two targeted cases — raw TCP and TLS — asserting:

The 101 handshake reaches the client verbatim.
Post-upgrade bytes the client sends hit socket.on('data', …) on the server (uWS no longer swallows them as HTTP).
Only one HTTP status line crosses the wire — no stray 200 OK from the response-lifecycle tail.
socket.on('close') fires on clean teardown.

Both fail against system Bun 1.3.14, pass with this change. The two existing tests in that file (ws-shim path, TLS socket-close crash) still pass. No regressions in the rest of test/js/node/http (the 3 pre-existing failures — node-http-uaf-fixture*.ts, issue#4295 — fail on main without this diff).

Related: #9882, #18945, #14522, #26924, #18569.

robobun · 2026-05-14T05:50:16Z

^{Updated 4:00 PM PT - May 14th, 2026}

❌ @robobun, your commit a1482b1 has 1 failures in Build #54350 (All Failures):

test/js/web/fetch/fetch-tcp-keepalive.test.ts - code 1 on 🐧 13 x64-asan
test/js/web/fetch/fetch-tcp-keepalive.test.ts - code 1 on 🐧 13 aarch64
test/js/web/fetch/fetch-tcp-keepalive.test.ts - code 1 on 🐧 25.04 x64
test/js/web/fetch/fetch-tcp-keepalive.test.ts - code 1 on 🐧 13 x64
test/js/web/fetch/fetch-tcp-keepalive.test.ts - code 1 on 🐧 25.04 x64-baseline
test/js/web/fetch/fetch-tcp-keepalive.test.ts - code 1 on 🐧 13 x64-baseline
test/js/web/fetch/fetch-tcp-keepalive.test.ts - code 1 on 🐧 25.04 aarch64
test/js/web/fetch/fetch-tcp-keepalive.test.ts - code 1 on 🐧 3.23 aarch64
test/js/web/fetch/fetch-tcp-keepalive.test.ts - code 1 on 🐧 3.23 x64
test/js/web/fetch/fetch-tcp-keepalive.test.ts - code 1 on 🐧 3.23 x64-baseline

🧪 To try this PR locally:

bunx bun-pr 30664

That installs a local version of the PR into your bun-30664 executable, so you can run:

bun-30664 --bun

coderabbitai · 2026-05-14T05:54:50Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

Walkthrough

The PR gates server "upgrade" handling on present listeners and adds a native markAsRawMode binding; when listeners exist the server enables streaming, marks sockets raw, emits "upgrade" with the head, and returns a close-resolved promise. Tests exercise plain and TLS upgrade flows.

Changes

HTTP Upgrade Listener-Gating and Raw Mode Support

Layer / File(s)	Summary
Mark-as-raw-mode native binding `src/jsc/bindings/node/JSNodeHTTPServerSocketPrototype.cpp`	New C++ JSC host function `markAsRawMode` added: includes `bun-uws/src/App.h`, declares the host function, registers `markAsRawMode` on the socket prototype, and implements validation plus setting of `isConnectRequest` on the underlying uWS HTTP response (SSL-conditional).
Listener-gated server upgrade handler `src/js/node/_http_server.ts`	Server upgrade path now checks for `"upgrade"` listeners before switching to upgrade mode. When listeners exist, it enables bi-directional streaming, marks the socket raw, creates a promise resolved on socket close, emits `"upgrade"` with the head, and returns the promise; otherwise it proceeds with normal request handling.
HTTP and TLS upgrade regression tests `test/js/node/http/node-http-with-ws.test.ts`	Add tests that perform manual RFC6455 WebSocket upgrade handshakes over plain TCP and TLS, verify a single `101 Switching Protocols` handshake, compute `Sec-WebSocket-Accept`, and confirm post-handshake bytes are delivered to the server's upgrade socket `data` handler.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title clearly and concisely summarizes the main change: fixing HTTP upgrade socket handoff to userland.
Description check	✅ Passed	Description includes detailed problem analysis, root causes, fix explanation, reproduction steps, and comprehensive test validation, far exceeding the template requirements.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 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 `@src/js/node/_http_server.ts`:
- Around line 633-635: The upgrade event currently emits kEmptyBuffer, dropping
any bytes already read past the header terminator; change the server.emit call
that currently does server.emit("upgrade", http_req, socket, kEmptyBuffer) to
pass the parser head buffer (the buffer containing bytes read past the header
terminator) as the third argument so upgrade listeners receive those pipelined
bytes; ensure the same variable used to represent the parser head earlier in the
request parsing path is supplied so ws.handleUpgrade() and other listeners get
the correct head instead of an empty buffer.

🪄 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: 4c743fe7-d65f-426e-9253-cace80c36a03

📥 Commits

Reviewing files that changed from the base of the PR and between b8ecc78 and 674bb9d.

📒 Files selected for processing (3)

src/js/node/_http_server.ts
src/jsc/bindings/node/JSNodeHTTPServerSocketPrototype.cpp
test/js/node/http/node-http-with-ws.test.ts

github-actions · 2026-05-14T05:55:11Z

Found 8 issues this PR may fix:

node:http is broken for proxies — 4 PRs fixing createConnection, upgrade sockets, connection close, and socket cleanup #28396 - Meta-issue explicitly naming upgrade socket.write() silently dropping data as one of four root causes affecting ws, vite, http-proxy, and express-ws
Proxying WebSockets with node-http-proxy (from Vite) doesn't work, works with Node #10441 - WebSocket proxying with node-http-proxy (from Vite) fails because socket.write() after 101 upgrade is a silent no-op
Websocket proxying does not work with bun but works with node #15489 - WebSocket proxying via http-proxy/httpxy broken by the same silent socket.write() failure on upgrade sockets
faye-websocket does not work #19724 - faye-websocket directly uses server.on('upgrade') and socket.write() for the 101 handshake, which silently fails
'bunx --bun vite' throw an error while proxy websockets #24127 - Vite's ws: true proxy config uses node-http-proxy upgrade path internally, broken by the same no-op write
Bun does not run properly on the hmr of vite #17825 - Vite HMR WebSocket fails under --bun due to its dev server relying on node-http-proxy upgrade handling
Bun runtime brokes hmr WS in dev mode for nuxt #18440 - Nuxt HMR WebSocket broken under bunx --bun because Nuxt/Vite proxies WebSocket connections through node:http upgrade events
Nuxt DevTools connection fails with WebSocket/HMR errors under Bun runtime #18737 - Nuxt DevTools WebSocket connection fails under --bun runtime due to the same Vite/h3 upgrade path issue

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #28396
Fixes #10441
Fixes #15489
Fixes #19724
Fixes #24127
Fixes #17825
Fixes #18440
Fixes #18737

🤖 Generated with Claude Code

github-actions · 2026-05-14T05:55:57Z

This PR may be a duplicate of:

fix(node:http): hand off upgrade socket to userland for bidirectional communication #28158 - Fixes the same upgrade socket handoff bug with the same approach (kEnableStreaming + markAsRawMode); explicitly superseded by this PR
fix(node:http): socket.write not sending data in upgrade event handler #27237 - Fixes the same root cause (kEnableStreaming not called before upgrade emit, causing writes to no-op); explicitly superseded by this PR
fix(node:http): make upgrade socket.write() actually send data #28347 - Fixes the identical bug (upgrade socket.write() silently dropped, 101 never reaches client) with the same markAsConnectRequest approach
fix socket.write() dropping data #28871 - Fixes socket.write() dropping data, targeting the same server-side write path

🤖 Generated with Claude Code

CodeRabbit (#30664 review): the CONNECT path forwards pipelined data past the header terminator via the 3rd `head` arg, but the upgrade path I added hardcoded `kEmptyBuffer`. Matches Node semantics and lets `ws.handleUpgrade` feed a first WebSocket frame piggy-backed onto the handshake into the new stream instead of losing it. New test pipelines a WebSocket frame immediately after the Upgrade header terminator and asserts the listener sees exactly those bytes as `head`.

coderabbitai

Actionable comments posted: 1

🤖 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/node/http/node-http-with-ws.test.ts`:
- Around line 231-240: The TLS regression test should mirror the plain-upgrade
invariants: after awaiting handshakeDone assert the handshake starts with
"HTTP/1.1 101 Switching Protocols\r\n" and contains the Sec-WebSocket-Accept
header, also assert there is only one HTTP status line in the handshake (same
invariant used in the plain test), then write/read the same websocket frame and
await serverGotData and assert Buffer.concat(serverReceived) equals the sent
buffer, and finally await the upgrade-socket close promise (as the plain test
does) before calling client.end() so the TLS test covers the same upgrade/close
surface.

🪄 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: c6ce45c7-5f9f-4d82-97db-6d0598b84270

📥 Commits

Reviewing files that changed from the base of the PR and between 674bb9d and ac75f9e.

📒 Files selected for processing (1)

test/js/node/http/node-http-with-ws.test.ts

robobun · 2026-05-14T07:27:28Z

🟡 Diff is green — CI failures are all flake on lanes my diff does not touch:

`test-http-should-emit-close-when-connection-is-aborted.ts` timing out on Windows 2019 x64 & x64-baseline. Same timeout hit builds #54197 and #54194 on unrelated branches (`openineditor-options-type`, `bunstring-fromjs-dead-no-exception`) in the same window. My diff for a plain `fetch()` with no `Upgrade:` header is literally no-op — `handleAsUpgrade` is false, the `assignSocket` branch is byte-identical to pre-PR.
Darwin 14 aarch64: `s3-storage-class.test.ts` S3 UnknownError (network flake); `bun-install-registry.test.ts` lockfile snapshot (retried once already).
Darwin 26 aarch64: `v8-heap-snapshot.test.ts` SIGTRAP (sanitizer flake).

None touch node:http or the socket handoff. Already spent my one re-roll in e4d25ee; not pushing another ci: retrigger. Needs a maintainer to land.

`server.on('upgrade', (req, socket) => socket.write(response))` was a silent no-op: the 101 handshake never left the server, the client timed out, and rsbuild (bundled `ws`) surfaced that as 'HMR broken'. Same path hits vite, webpack-dev-server, http-proxy, socket.io — anything that writes the switching-protocols line to the socket itself. Two things were broken: 1. `NodeHTTPServerSocket._write` only calls `handle.write()` when `handle.ondrain` is truthy. The upgrade path fell through `kEnableStreaming(false)` (from the non-CONNECT branch just above) so `ondrain` was undefined and every write became a no-op with a successful callback. 2. uWS kept parsing inbound bytes as HTTP even after the event fired, so post-upgrade traffic (WebSocket frames, tunnel bytes) was either dropped or answered with 400 Bad Request. Mirror the CONNECT handoff: turn on bidirectional streaming, call a new `socketHandle.markAsRawMode()` that sets `HttpResponseData::isConnectRequest = true` (the same flag CONNECT flips to switch uWS into pass-through), and return a promise that settles when the socket closes instead of falling through to `socket.cork()` + the HTTP response lifecycle. Preserves the no-listener path — falls through to the 'request' event as Node does. Fixes #30661. Related: #9882, #18945, #14522, #18569.

CodeRabbit (#30664 review): the CONNECT path forwards pipelined data past the header terminator via the 3rd `head` arg, but the upgrade path I added hardcoded `kEmptyBuffer`. Matches Node semantics and lets `ws.handleUpgrade` feed a first WebSocket frame piggy-backed onto the handshake into the new stream instead of losing it. New test pipelines a WebSocket frame immediately after the Upgrade header terminator and asserts the listener sees exactly those bytes as `head`.

Per CodeRabbit review: the TLS test was checking the handshake + data-flow but missed the "only one HTTP status line" assertion and didn't await socket close on the server side. Now matches the plain-TCP test one for one.

Per claude[bot] review: when a request carries an `Upgrade:` header but the server has no `'upgrade'` listener, my listener-gate makes control fall through to the `'request'` event — but the skip guard at line 604 still keyed on raw `is_upgrade`, so `http_res.socket` was never bound to the real `NodeHTTPServerSocket`. The `ServerResponse` socket getter lazily manufactures a `FakeSocket`, which (a) reports the wrong `remoteAddress`, (b) never emits the `'socket'` event on the response, (c) breaks timeout/close wiring, and (d) silently drops `writeContinue()` / `writeEarlyHints()`. Compute `handleAsUpgrade = is_upgrade && server.listenerCount('upgrade') > 0` once and gate both the skip and the branch on it. New test fires an `Upgrade: h2c` request against a server with no upgrade listener and asserts `res.socket === req.socket` and a real `remoteAddress` — fails on the previous two commits, passes here.

CI on 6306eef hit flaky failures on Windows lanes unrelated to this diff: - test-http-should-emit-close-when-connection-is-aborted.ts timeout on Windows 2019 (this test doesn't touch the upgrade path I changed — `handleAsUpgrade` is false for a plain `fetch()`, so the `assignSocket` branch is identical to pre-PR behavior) - hot.test.ts `reloadCounter expected 50 got 40` (Windows 11 aarch64) - bun-install-lifecycle-scripts.test.ts CPU-time assertion (flaky, retried once already) - bun-install-registry.test.ts hoisting/registry version flake None are HTTP-upgrade or node:http related. Re-rolling CI.

robobun · 2026-05-14T17:57:03Z

Still all flake, still green on my diff:

Build #54350 (a1482b1, after rebase) → one failure: fetch-tcp-keepalive.test.ts on Debian-13 x64-asan. The test reads /proc/self/net/tcp to check SO_KEEPALIVE on fetch client sockets — 0 bytes of overlap with this PR, which changes server-side node:http upgrade handling.

It is also failing on 18 other PRs in the same CI window (builds #54340–#54357: napi-tsfn-null-js-callback, domain-catches-async-throws, hot-reload-stdin-listeners, sql-lazy-pool, …). The test was added in b8ecc78b03 (#30640, on main); this is a self-inflicted race in that new test, not something the diff here can affect.

Needs a maintainer to either merge this or fix the keepalive test. Not pushing another retrigger — already spent one in e4d25ee.

robobun mentioned this pull request May 14, 2026

The HMR failed when running bun --bun rsbuild. #30661

Closed

github-actions Bot added the claude label May 14, 2026

This was referenced May 14, 2026

fix(node:http): hand off upgrade socket to userland for bidirectional communication #28158

Closed

fix(node:http): socket.write not sending data in upgrade event handler #27237

Open

coderabbitai Bot reviewed May 14, 2026

View reviewed changes

Comment thread src/js/node/_http_server.ts Outdated

coderabbitai Bot reviewed May 14, 2026

View reviewed changes

Comment thread test/js/node/http/node-http-with-ws.test.ts

claude Bot reviewed May 14, 2026

View reviewed changes

Comment thread src/js/node/_http_server.ts Outdated

robobun and others added 7 commits May 14, 2026 17:29

[autofix.ci] apply automated fixes

dbf378b

test: mirror plain-upgrade invariants in TLS regression

a166aff

Per CodeRabbit review: the TLS test was checking the handshake + data-flow but missed the "only one HTTP status line" assertion and didn't await socket close on the server side. Now matches the plain-TCP test one for one.

[autofix.ci] apply automated fixes

6061421

robobun force-pushed the farm/86830794/fix-http-upgrade-socket-handoff branch from e4d25ee to a1482b1 Compare May 14, 2026 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node:http: hand off upgrade socket to userland#30664

node:http: hand off upgrade socket to userland#30664
robobun wants to merge 7 commits into
mainfrom
farm/86830794/fix-http-upgrade-socket-handoff

robobun commented May 14, 2026

Uh oh!

robobun commented May 14, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented May 14, 2026 •

edited

Loading

Reviews paused

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

github-actions Bot commented May 14, 2026

Uh oh!

github-actions Bot commented May 14, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

robobun commented May 14, 2026

Uh oh!

robobun commented May 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

robobun commented May 14, 2026

Reproduction

Root cause

Fix

Tests

Uh oh!

robobun commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented May 14, 2026

Uh oh!

github-actions Bot commented May 14, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

robobun commented May 14, 2026

Uh oh!

robobun commented May 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robobun commented May 14, 2026 •

edited

Loading

coderabbitai Bot commented May 14, 2026 •

edited

Loading