[robobun]

Repro

const server = Bun.serve({ port: 0, fetch: () => new Response("nope", { status: 404 }) });
const sock = await Bun.connect({
  hostname: "127.0.0.1", port: server.port,
  socket: {
    data(_, d) { console.log(JSON.stringify(d.toString().split("\r\n")[0])); },
    close() { console.log("closed"); },
  },
});
sock.write("CONNECT example.com:443 HTTP/1.1\r\nHost: example.com:443\r\n\r\n");
// before: "HTTP/1.1 404 Not Found", then nothing: the connection stays open and
//         anything written to it afterwards (a pipelined GET, tunnel bytes) is
//         discarded without a response
// after:  "HTTP/1.1 404 Not Found", "closed"

Cause

HttpParser.h sets the per-connection isConnectRequest flag when it parses a CONNECT request-line and from then on stops parsing the connection as HTTP: after the dispatch it marks the rest of the buffer consumed, and every later read goes straight to the data handler (consumePostPadded), which on a Bun.serve connection has no inStream to deliver to. node:http relies on this to hand the connection to its 'connect' listener as a tunnel. Bun.serve has nothing to hand a tunnel to: the request went through the normal handler (fetch, routes, or the 404 fallback), the response was written, and the connection was then kept alive in tunnel mode as a sink.

Fix

In the uWS request dispatch (HttpContext.h), when a CONNECT is dispatched on a Bun.serve context (!IsNodeHttp), set HTTP_CONNECTION_CLOSE on the connection, the same state an HTTP/1.0 or Connection: close request sets a few lines above. The handler still answers however it likes, and the existing close gates (post-parse in onData, internalEnd, onWritable) close the socket once that response has been flushed. Every Bun.serve response path already feeds this state into its end() call, so this covers fetch, routes, static/file routes and the no-handler 404 alike. The node:http instantiation is unchanged (its CONNECT handling lives in _http_server.ts: 'connect' listener, or close when there is none).

As with the other close-marked requests on Bun.serve, no Connection: close header is added; the response is complete and the socket is closed behind it.

#34261 addresses the same report differently, by dropping CONNECT before dispatch with no response written. This PR keeps the response and closes afterwards.

Verification

test/js/bun/http/bun-server.test.ts, new CONNECT requests block. Each case sends a CONNECT head followed by a pipelined GET on a server with a long idle timeout and waits for the server to end the connection, then checks that exactly one response (the handler's) came back:

• head in one read, and head split across two reads (exercises the fallback buffer path)
• handler that responds from a later event loop turn (close happens outside the parse window)
• routes-only server with no fetch (404 fallback)

Without the change all four time out waiting for the close (the 404/405 arrives, the connection stays open); with it they pass. test/js/node/http/node-http-connect.test.ts and the node CONNECT regression tests (25862, 32734) still pass.

---

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

[coderabbitai]

Warning

Review limit reached

Your included review limit has been reached.

You’re in a promotional period — use the checkbox below to run this review for free:

• Run review for free

_{On-demand reviews are free for the next 29 days. After that, they cost $0.25 per reviewed file.}

How can I continue?

Run this review now using the option above, or comment @coderabbitai review --use-credits.

You can also wait for the limit to reset (next review available in 11 minutes), then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1c583361-4b76-47e8-8626-9e071c599e59

📥 Commits

Reviewing files that changed from the base of the PR and between 8eb5b6e and 0d11782.

📒 Files selected for processing (2)

• packages/bun-uws/src/HttpContext.h
• test/js/bun/http/bun-server.test.ts

---

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

[robobun]

Status: reproduced on 1.4.0 with a raw socket sending CONNECT example.com:443 HTTP/1.1 to Bun.serve (404 comes back, connection stays open, a pipelined GET after it gets no response). Fix and tests are in this PR, rebased onto main.

The diff is green. The new CONNECT requests tests passed on every lane that ran them, in both CI runs (builds 103799 and 103823). The remaining red is unrelated to this change:

• Build 103799: test/cli/install/bun-add.test.ts failed on darwin aarch64 and Windows x64 with GET https://api.github.com/repos/mishoo/UglifyJS/tarball/v3.14.1 - 504 (GitHub API errors). Reported separately for triage. Everything else passed on retry.
• Build 103823: every test failure passed on retry or alone (install, napi, child_process IPC, inspector protocol, http2 conformance). The two darwin x64 jobs never ran tests: git clone failed three times with Failed to connect to github.com port 443 and RPC failed; curl 56. The same lane passed in build 103799 on the same diff.

Nothing in the failure set touches Bun.serve, uWS, or bun-server.test.ts. Ready for review.

[claude]

I reviewed this PR and didn't find any bugs. The fix is small and reuses the existing HTTP_CONNECTION_CLOSE mechanism, but it changes user-visible Bun.serve behavior in the core uWS request dispatch and the description notes a competing approach in #34261 — a maintainer should confirm the respond-then-close design is the one to land.

What was reviewed:

• Confirmed isConnectRequest is set by getHeaders before the request handler runs, so the flag is live when the new check reads it.
• Checked that on !IsNodeHttp a CONNECT can only reach the non-pipelined else branch (the pipelined branch force-closes), so the placement covers every dispatch.
• Verified the node:http instantiation is untouched (if constexpr (!IsNodeHttp)) and its tunnel/close paths in onClose/onEnd/the data handler still key off isConnectRequest unchanged.
• Tests await the server-side close (not a sleep), wire end/close/error to resolve, and assert exactly one response; idleTimeout: 255 rules out timeout as the close cause.

Extended reasoning...

Overview

The PR adds a 5-line guarded block to packages/bun-uws/src/HttpContext.h inside the request-dispatch lambda: on a Bun.serve context (!IsNodeHttp), when the parser has marked the connection as a CONNECT request, set HTTP_CONNECTION_CLOSE on the response state. This is the same flag the two lines directly above set for HTTP/1.0 and Connection: close requests, so all downstream close gates (post-parse in onData, internalEnd, onWritable) already handle it. A new describe.concurrent block in test/js/bun/http/bun-server.test.ts covers four cases: head in one read, head split across two reads (fallback-buffer path), an async handler that responds after the parse window, and a routes-only server with no fetch (404 fallback).

Security risks

None introduced. The change closes a connection that was previously kept open as a silent sink; if anything it removes a minor resource-hold vector. No new parsing, no new allocation, no untrusted input reaches new code.

Level of scrutiny

Medium-high. The code change is tiny, mechanical, and gated behind if constexpr (!IsNodeHttp) plus an [[unlikely]] runtime flag, so it cannot affect the common path or node:http. But it lives in the per-request dispatch of the vendored uWS HTTP server — every Bun.serve request flows through this function — and it changes user-visible behavior (connections now close after a CONNECT response). The PR description also explicitly names a competing PR (#34261) that solves the same report by dropping CONNECT before dispatch with no response written; picking respond-then-close over reject-early is a small API/behavior design call that a maintainer should ratify.

Other factors

I traced isConnectRequest: it is written through a bool& by getHeaders in HttpParser.h before requestHandler is invoked, so the new check reads a live value. On !IsNodeHttp the pipelined branch of the dispatch (HTTP_RESPONSE_PENDING) force-closes the socket, so a CONNECT can only ever land in the else branch where the new code sits — no sibling path is missed. The node:http tunnel handling (onSocketData, HTTP_NODE_TUNNEL_AFTER_BODY, onEnd half-open) all sits behind IsNodeHttp or socketData checks and is unaffected. The tests follow the repo's conventions well: port: 0, await using server, awaiting the actual close event with all failure events wired to resolve, idleTimeout: 255 so a passing test cannot be the idle timeout closing the socket, and asserting both the status line and that no second response was parsed from the trailing GET. The PR states the existing node CONNECT tests (node-http-connect.test.ts, regression 25862/32734) still pass.

Given the design-choice aspect and the location in core server dispatch, deferring to a human rather than auto-approving.

[robobun]

^{Updated 6:25 PM PT - Aug 22nd, 2026}

❌ @robobun, your commit 0d11782 has some failures in Build #103823 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 37585

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

bun-37585 --bun

[claude]

LGTM — small, well-scoped fix that reuses the existing HTTP_CONNECTION_CLOSE mechanism for CONNECT on Bun.serve.

What was reviewed:

• Verified isConnectRequest is latched in HttpParser.h::getHeaders before the request handler runs, so the flag is set when the new check reads it.
• Confirmed the new block sits after resetResponseState() and inside the !IsNodeHttp non-pipelined branch; the pipelined branch already closes for Bun.serve, and node:http is compile-time excluded.
• Traced HTTP_CONNECTION_CLOSE through the existing close gates (onData post-parse, closeIfDoneAndMarked/internalEnd, onWritable) — the async-response test case is covered by internalEnd.
• Tests await the server-initiated close with idleTimeout: 255, so a pass can only come from the fix; they cover single-read, split-head (fallback buffer), async handler, and the routes-only 404 path.

Extended reasoning...

Overview

The PR adds a 5-line block to packages/bun-uws/src/HttpContext.h inside the per-request dispatch lambda: when !IsNodeHttp and httpResponseData->isConnectRequest is set, it ORs HTTP_CONNECTION_CLOSE into the response state. This is placed immediately after the identical treatment for HTTP/1.0 and Connection: close requests, and reuses the same close machinery (shouldCloseConnection() gates in onData, onWritable, and HttpResponse::closeIfDoneAndMarked). A new describe.concurrent("CONNECT requests") block in test/js/bun/http/bun-server.test.ts adds four tests.

Security risks

None introduced. The change strictly narrows behavior: a connection that previously stayed open indefinitely as a silent sink after a CONNECT response now closes once the response flushes. This is a resource-cleanup improvement, not new attack surface. The node:http path is unchanged (compile-time gated by if constexpr (!IsNodeHttp)), so its 'connect' tunnel semantics are unaffected.

Level of scrutiny

Medium. HttpContext.h is core server dispatch, but the change is (a) compile-time gated to Bun.serve only, (b) runtime-gated to isConnectRequest only (marked [[unlikely]]), and (c) purely additive — it sets a flag whose downstream handling is already exercised by every HTTP/1.0 and Connection: close request. I traced that isConnectRequest is set in HttpParser.h::getHeaders before requestHandler is invoked, so the flag is live at the check. resetResponseState() runs just before the new block, so the flag can't be cleared out from under it, and after a CONNECT the parser stops treating the connection as HTTP so no subsequent resetResponseState() will clear it either.

Other factors

The tests are well-constructed per repo guidelines: port: 0, await using for server/socket cleanup, describe.concurrent, awaiting the actual observable condition (server-initiated end/close) rather than sleeping, and idleTimeout: 255 so a passing test proves the close came from the fix rather than idle timeout. The four cases cover the single-read path, the split-head fallback-buffer path, an async handler (response written outside the parse window, so internalEnd's close gate is what fires), and the no-fetch 404 fallback. The PR notes the node:http CONNECT tests (node-http-connect.test.ts, regressions 25862/32734) still pass. The double-setImmediate between split-write chunks is a write-spacing device, not a condition wait, and is commented as such.

[claude]

LGTM — a five-line, well-scoped fix that reuses the existing HTTP_CONNECTION_CLOSE mechanism, gated to !IsNodeHttp.

Checked that httpResponseData->isConnectRequest is set by getHeaders (HttpParser.h:1001) before requestHandler runs (HttpParser.h:1338), so the flag is live when the new block reads it. Placement is after resetResponseState() alongside the HTTP/1.0 / Connection: close cases that already set the same bit, so the existing close gates in onData/internalEnd/onWritable handle the shutdown. The if constexpr (!IsNodeHttp) guard leaves node:http's CONNECT tunnel path untouched. Tests cover fetch handler (sync and deferred response), routes-only 404 fallback, and the split-head fallback-buffer path, each asserting the server ends the connection and no second response is written for the pipelined GET.

Extended reasoning...

Overview

Two files: a five-effective-line addition in packages/bun-uws/src/HttpContext.h (inside the request-dispatch lambda) that ORs HTTP_CONNECTION_CLOSE into the response state when isConnectRequest is set on a Bun.serve (!IsNodeHttp) context, plus a new describe.concurrent("CONNECT requests") block in test/js/bun/http/bun-server.test.ts with four cases.

Security risks

None introduced. If anything this tightens behaviour: previously a CONNECT to Bun.serve left the connection open in tunnel mode as a silent sink (bytes discarded, socket held until idle timeout). Closing after the response removes that resource-hold path. The change does not touch parsing, validation, TLS, or the node:http CONNECT/Upgrade tunnel handling.

Level of scrutiny

The file is a hot path, but the change itself is mechanical: it adds one more case to the existing set of "mark this response as the connection's last" (HTTP/1.0, Connection: close) that already set the same flag a few lines above. I traced that isConnectRequest is written by the parser before the request handler callback runs, that the new block sits after resetResponseState() in the non-pipelined branch (a CONNECT is always the connection's first and only dispatched request, so the pipelined branch is unreachable here), and that if constexpr (!IsNodeHttp) compiles the block out of the node:http instantiation entirely — its 'connect'-listener tunnel handling in _http_server.ts is unaffected.

Other factors

The tests are well-designed per the repo's review rules: they await the server-side close (wiring end/close/error all to resolve), use port: 0 with a raised idleTimeout so a close can only come from the response path, cover the variant matrix (sync fetch, async fetch responding from a later tick, routes-only 404, single-read vs split-head fallback buffer), and assert both the status line and that the pipelined GET produced no second response. The PR description confirms the node:http CONNECT suites (node-http-connect.test.ts, regressions 25862/32734) still pass. PR #34261 is noted as taking a different approach (drop before dispatch, no response); this PR's respond-then-close is the more informative behaviour for clients and the smaller diff, and the description states the case for it — no open design question I'd block on.