[robobun]

Problem

• req.destroy() on a request that a 'connect' or 'upgrade' listener took over makes Bun write HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n into the tunnel, behind the reply of the listener. A listener that refuses a CONNECT this way in a later task answers it with that 200.
• The cause is NodeHTTPResponse::abort (src/runtime/server/NodeHTTPResponse.rs:1679). It ends the native response with end_without_body(true), and a handed-off request has no response. Node writes nothing and destroys the socket.
• main sends the 200 in 6 of the 8 cases in Notes. Bun 1.4.2 sends it in 4.

Fix

• abort() returns for a response with the TUNNELED flag. upgradeToTunnel() sets the flag before the listener runs.
• The connection still closes: IncomingMessage._destroy destroys the socket right after abort().
• Verified: test/js/node/http/node-http-connect.test.ts (13 new tests, 9 fail on main). Also node-http.test.ts, node-http-server-abort-events and ws.test.ts.

Background

• A tunnel is a connection that the HTTP parser no longer reads. The listener writes the reply on the raw socket.
• Bun creates the native response before it knows that the request is a handoff. TUNNELED marks a response that is never written.
• Considered a test of req.upgrade in IncomingMessage._destroy. ServerResponse.prototype.destroy calls abort() too, and user code can write req.upgrade.

Downsides

• Cost: one flag test per abort() call. abort() grows by 5 bytes, and .text keeps its size.
• Still wrong after this change: a plain request that is destroyed from a later task still gets an empty 200 OK (node:http: send nothing on destroy() before the headers go out, and a 500 (not an empty 200) for a throwing listener #39366).

Notes

Repro. Run with <connect|upgrade> <sync|later> <reply|refuse>. extra is what the client gets besides the reply of the listener.

const http = require("node:http");
const net = require("node:net");
const [event, when, mode] = process.argv.slice(2);
const reply = event === "connect" ? "HTTP/1.1 200 Connection Established\r\n\r\n" : "HTTP/1.1 101 Switching Protocols\r\nConnection: Upgrade\r\nUpgrade: raw\r\n\r\n";
const request = event === "connect" ? "CONNECT example.com:443 HTTP/1.1\r\nHost: example.com:443\r\n\r\n" : "GET / HTTP/1.1\r\nHost: example.com\r\nConnection: Upgrade\r\nUpgrade: raw\r\n\r\n";
const server = http.createServer();
server.on(event, (req, socket) => {
  socket.on("error", () => {});
  if (mode === "reply") socket.write(reply);
  if (when === "sync") req.destroy();
  else setImmediate(() => req.destroy());
});
server.listen(0, "127.0.0.1", () => {
  const client = net.connect(server.address().port, "127.0.0.1");
  let received = "";
  client.on("data", chunk => (received += chunk));
  client.on("error", () => {});
  client.on("close", () => {
    console.log(JSON.stringify({ event, when, mode, extra: received.slice(mode === "reply" ? reply.length : 0) }));
    server.close();
  });
  client.write(request);
});

case	Node v26.3.0	Bun 1.4.2	main 6c1eb067e3	this change
connect, sync, reply	nothing	nothing	200 OK	nothing
connect, later, reply	nothing	200 OK	200 OK	nothing
upgrade, sync, reply	nothing	nothing	200 OK	nothing
upgrade, later, reply	nothing	200 OK	200 OK	nothing
connect, sync, refuse	nothing	nothing	nothing	nothing
connect, later, refuse	nothing	200 OK	200 OK	nothing
upgrade, sync, refuse	nothing	nothing	nothing	nothing
upgrade, later, refuse	nothing	200 OK	200 OK	nothing

Why sync differs between 1.4.2 and main. Inside the listener the socket is corked. Until #43557 the close threw the cork buffer away together with the 200. Since #43557 a response that ends is sent at once.

The matrix also runs with an Upgrade request whose body still arrives (2 of 5 bytes). There main writes the 200 in the two later-task cases.

The last test. Bun constructs a ServerResponse for an Upgrade request before it knows that the request is a handoff. A custom ServerResponse class can keep that object, and its destroy() reaches the same abort(). Node constructs no response there.

Relation to #39366. That pull request stops abort() from writing for every destroyed request. It conflicts with main. This change is the handoff case alone. When #39366 lands, the flag test here has no effect on the bytes, and the tests stay valid.

Size. Release builds of main 6c1eb067e3 with and without the change, linux x64. The .text section is 65,818,853 bytes in both, and the stripped binary is 89,318,984 bytes in both.

Suites. Debug build: node-http-connect, node-http-upgrade-body, node-http-server-abort-events, node-http-with-ws, node-http-transfer-encoding, node-http-req-socket-pause, node-http-displaced-response, node-http-server-close-drain, node-http.test.ts and ws.test.ts.

---

no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http/node-http-connect.test.ts

[robobun]

^{Updated 11:04 PM PT - Oct 11th, 2026}

❌ @robobun, your commit deea32a has 2 failures in Build #124752 (All Failures):

• test/js/sql/sql-close-pending-connection.test.ts - code 1 on 🍎 27 aarch64
• test/js/node/net/node-net.test.ts - code 1 on 🍎 27 aarch64

---

🧪   To try this PR locally:

bunx bun-pr 45026

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

bun-45026 --bun

[robobun]

Status: ready for review. CI is green on deea32a.

How it was reproduced: the script under Notes in the description, run with <connect|upgrade> <sync|later> <reply|refuse>. Node v26.3.0 sends nothing besides the reply of the listener in 8 of 8 cases. main 6c1eb067e3 sends HTTP/1.1 200 OK in 6 cases, Bun 1.4.2 in 4, this branch in 0.

Pull request: #45026. #45058 is stacked on it.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
• Review profile: ASSERTIVE
• Plan: Essentials
• Run ID: 1bd0b79a-ed1c-4616-ac6b-f1c186156769

---

📥 Commits

Reviewing files that changed from the base of the PR and between f379e88 and deea32a.

---

📒 Files selected for processing (1)

• test/js/node/http/node-http-connect.test.ts

---

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

---

---

Walkthrough

NodeHTTPResponse::abort now returns without writing a response when the response is done or the connection is tunneled. CONNECT and Upgrade tests cover request destruction and destruction of a constructed ServerResponse.

Changes

Tunneled response abort handling

Layer / File(s)	Summary
Skip abort termination for tunneled responses src/runtime/server/NodeHTTPResponse.rs, test/js/node/http/node-http-connect.test.ts	abort now returns early for tunneled connections. Tests cover destroying handed-off CONNECT and Upgrade requests and a constructed ServerResponse. They check the bytes received by the client and server shutdown completion.

Suggested reviewers: jarred-sumner

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to deea3

The change prevents abort-generated response bytes from entering CONNECT and Upgrade tunnels while preserving socket closure. No actionable merge-blocking risk is established in the changed paths.

Pre-merge checks | 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title clearly describes the primary fix: preventing req.destroy() on accepted CONNECT or Upgrade requests from writing an unintended 200 OK response into the tunnel.
Description check		The description explains the problem, implementation, verification, test coverage, and known limitation. It does not use the template headings exactly, but it provides the required information in equi…

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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

[claude]

Beyond the inline findings, I also checked what the new early return in abort() skips for a tunneled handle: on_request_complete already short-circuits on TUNNELED (src/runtime/server/NodeHTTPResponse.rs:784) and finalize accepts a TUNNELED response without SOCKET_CLOSED (line 2665), so the skipped release_body_slot/mark_socket_closed bookkeeping does not strand the request in the no-body handoff case; and IncomingMessage._destroy / ServerResponse.prototype.destroy (src/js/node/_http_server.ts:4049) still call socket.destroy() after handle.abort(), so the connection still closes.

Extended reasoning...

One-line Rust guard in NodeHTTPResponse::abort plus nine new tests in test/js/node/http/node-http-connect.test.ts; no security-sensitive surface. Three inline findings remain (partial coverage of the write paths on the retained ServerResponse, the Upgrade-with-body variant untested, and the non-tunneled later-task 200 OK left as-is), so a human look is already signalled by them; this note only records the resource-release and socket-close checks that were ruled out.

Findings marked 🟡 are optional suggestions and need no follow-up push.

[claude]

LGTM — the new guard is a single flag test and the latest push adds the Upgrade-with-body row I asked for, so the one handoff where TUNNELED is set while uws still parses is now covered for both destroy timings.

Beyond the inline nit, I checked that skipping the rest of abort() for a tunneled response does not strand accounting: the socket teardown that IncomingMessage._destroy triggers still reaches set_closed/on_abort → mark_socket_closed → leave_pending, which releases the body slot and lets mark_request_as_done drop the pending-request ref, and every matrix row asserts server.close() resolves.

Extended reasoning...

The change adds one condition to NodeHTTPResponse::abort in src/runtime/server/NodeHTTPResponse.rs so a response flagged TUNNELED (CONNECT or Upgrade handoff) returns before end_without_body writes a 200 into the raw tunnel, plus a 12-case describe.each matrix and a custom-ServerResponse test in test/js/node/http/node-http-connect.test.ts. It touches node:http server native code but no auth, crypto, or input-parsing surface. The fix is three lines with clear intent, the tests assert exact bytes and that server.close() completes, and the second push addressed the with-body variant raised in the prior review; the remaining inline item is a nit about a vacuous optional-chaining call in one test. The plain-request later-task 200 raised earlier is pre-existing behavior the author explicitly scopes to a separate PR.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Still open from earlier reviews (1):

• 🔴 src/runtime/server/NodeHTTPResponse.rs:1688 — Every plain request that user code destroys from a later task still answers the client with an empty 'HTTP/1.1 200 OK'…

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

[claude]

I re-reviewed the latest push and found no new bugs; the two commits since my last pass address the earlier feedback (the Upgrade-with-body row in the describe.each matrix, and expect(response).toBeInstanceOf(Response) so the subclass test can no longer pass vacuously). Because the one-line change moves teardown of a TUNNELED response off abort() and onto the socket.destroy() close path in lifecycle-sensitive native code, a human look at src/runtime/server/NodeHTTPResponse.rs is still worthwhile.

What was reviewed:

• Traced Flags::TUNNELED from mark_tunneled (set before the listener runs) through should_request_be_pending, the dispatch tail in src/runtime/server/mod.rs:1540, and the Drop assertions — the new early return leaves no path where the flag is unset for a handoff.
• Checked what the skipped cleanup (clear_pending_pinned_write, release_body_slot, mark_socket_closed, on_request_complete) now depends on: both JS callers (_http_incoming.ts:377, _http_server.ts:4049) call socket.destroy() right after abort(), and the uws close reaches mark_socket_closed / mark_request_as_done_if_necessary; the matrix's server.close() resolving asserts the pending-request accounting is released, including the body-still-arriving row.
• The remaining open thread (plain, non-handoff request destroyed from a later task) is pre-existing behavior the description scopes to #39366, not something this diff introduces.

Extended reasoning...

The diff adds one condition to NodeHTTPResponse::abort() in src/runtime/server/NodeHTTPResponse.rs so a TUNNELED (CONNECT/Upgrade handoff) response returns before resume_socket/mark_socket_closed/end_without_body/on_request_complete, plus 13 tests in test/js/node/http/node-http-connect.test.ts covering the connect/upgrade/upgrade-with-body x reply/no-reply x sync/later matrix and a custom-ServerResponse variant. It touches no auth, crypto, or injection surface; the sensitive part is refcount and pending-request lifecycle in native server code, where the skipped cleanup now relies on the subsequent socket.destroy() close path. The two commits since the prior review added the with-body matrix row and a precondition assertion, addressing earlier inline feedback, and no CODEOWNER covers the changed files. Defer rather than approve because the lifecycle-sensitive native area and the still-open plain-request thread warrant a maintainer's confirmation that scoping the fix to handoffs is the intended split from #39366.