Bun.serve: stop dispatching pipelined requests after Connection: close (RFC 9112 9.6)

[robobun]

Problem

• Bun.serve answers requests pipelined behind a Connection: close request. One write of GET /a (Connection: close) and GET /b gets two 200 responses, and the socket stays open. RFC 9112 9.6: the server MUST NOT process further requests. Node answers /a and closes.
• The loop in fenceAndConsumePostPadded (packages/bun-uws/src/HttpParser.h) dispatches every request of a read. The latch that stops it was node:http only. The next dispatch calls resetResponseState(), which clears HTTP_CONNECTION_CLOSE.
• Since Bun.serve: keep the socket corked across the requests of one read #42762 this also hits a close-flagged request in the middle of a read: /0, /1 (close), /2, /3 gets four responses. getHeader("connection").length() == 5 also misses keep-alive, close.

Fix

• The sawConnectionClose latch applies to both server types. At the loop top Bun.serve discards the rest of the buffer. node:http still returns HPE_CLOSED_CONNECTION.
• Discarded bytes never reach resetResponseState(). HTTP_CONNECTION_CLOSE stays set, and the existing check at the end of onData sends the responses and closes.
• HttpRequest::hasConnectionClose() scans each Connection line as a case-insensitive token list. The parser calls it once per request, to set the latch before the handler runs. The dispatch code in HttpContext.h reads the latch, so a request scans its Connection header once. Both length() == 5 checks are gone.
• Verified: test/js/bun/http/serve.test.ts, "does not dispatch a pipelined request after Connection: close" (10 cases, 8 fail on main). Also request-smuggling.test.ts, node-http.test.ts, 44 test-http-* files.

Background

• onData (HttpContext.h) receives one TCP read and runs the parser loop over it. The loop calls the handler for each complete request.
• HttpResponseData::state is one word per socket. resetResponseState() clears its per-response bits at each dispatch.
• The latch is a bool on the per-socket HttpParser, so it also covers a later read.

Notes

Repro. With Bun.serve({ port: 0, fetch: req => new Response(new URL(req.url).pathname) }) listening:

printf 'GET /a HTTP/1.1\r\nHost: x\r\nConnection: close\r\n\r\nGET /b HTTP/1.1\r\nHost: x\r\n\r\n' | nc 127.0.0.1 <port>

main runs the handler for /a and /b, writes two 200 responses and leaves the socket open. With this change: one response, then the server closes.

Where the gate sits. It is the last check before getHeaders(). Malformed bytes behind the close-flagged request are discarded. They do not produce a 400. It runs after three node:http blocks: the tunnel check (a switched-protocol connection is no longer HTTP), the flood-prevention park block, and the leading CR/LF skip. Parked bytes go through feedNodeHttpData when reads resume, so they reach the gate then. That keeps the order main had for node:http: park first, HPE_CLOSED_CONNECTION second. The CR/LF skip runs first because llhttp's closed state also skips CR and LF and raises HPE_CLOSED_CONNECTION only on other bytes, so the extra CRLF some clients send after a POST body still gets its response (three cases in node-http.test.ts cover this). All three blocks are compiled out for Bun.serve, where the gate is the first thing in the loop.

The latch is set before the handler runs. An async handler gets the same discard. On main the pipelined request finds the response to /a still pending and takes the "request behind a pending response" close in the dispatch lambda, so the response to /a is never sent (0 bytes, then close). With the discard the response is sent when the handler resolves, and internalEnd closes after it. The body of the close-flagged request itself is consumed in the same loop iteration, before the gate can run. The chunk-size cases in request-smuggling.test.ts still return 400.

Test results. Debug build (ASAN), main at 7d792a5, the new describe block:

build	result
packages/ from main	8 fail, 2 pass (the two keep-alive controls)
this branch	10 pass
1.4.3-canary 09bb546 (before #42762)	7 fail, 3 pass

The canary passes the middle-of-a-read case: before #42762 the first response of a read released the cork, so the response to /1 ran the close check in internalEnd and closed there. #42762 keeps the cork for the whole read, so that path no longer runs. The parser latch does not depend on the cork state.

Cases. Over a raw socket, one write each:

• close, Close, keep-alive, close, close, TE, HTTP/1.0: one response, handler runs for /a only, server closes.
• /0, /1 (close), /2, /3: two responses, handler runs for /0 and /1, server closes.
• /a (close) with an async handler, then /b: the response to /a arrives, then the server closes.
• Connection: nope! (five bytes) and no Connection header: both requests are served.
• @@@ not HTTP behind the close-flagged request: one response, no 400.

Suites run with the debug build. serve.test.ts (311 pass), request-smuggling.test.ts (89), node-http.test.ts (159), node-http-req-socket-pause.test.ts, node-http-parser.test.ts, node-http-connect.test.ts, bun-server.test.ts, http-server-chunking.test.ts, bun-serve-headers.test.ts, serve-close-delimited-framing.test.ts, hspec.test.ts, and 44 test/js/node/test/parallel/test-http-* files (pipeline, keep-alive, upgrade-server, smuggling, server-close, 1.0). The failures also fail on a debug build of main in the same container: root-range port and /bun:info loopback (the container runs as root), two IPv6 tests (no IPv6), and the node-http-connect wrapper that runs its 7 inner tests in 5.6 s against a 5 s timeout.

Scope. This PR is the MUST NOT half of 9.6 plus the token-list detection. The Connection: close response header (the SHOULD half) is not part of it. Earlier revisions of this PR carried it, along with a state bitfield refactor that main has since absorbed. The changes-requested reviews from June refer to that code. It is no longer in the diff. #33878 is closed as superseded.

---

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

[robobun]

^{Updated 6:12 PM PT - Sep 16th, 2026}

⏳ @Jarred-Sumner, your commit 5bf4ec2 is still building in Build #116791, but has 1 failures so far (All Failures):

• test/js/bun/s3/s3.test.ts - code 1 on 🐧 13 x64

[github-actions]

Found 1 issue this PR may fix:

1. node:http is broken for proxies — 4 PRs fixing createConnection, upgrade sockets, connection close, and socket cleanup #28396 - Sub-issue Support import assertions #4 ("TCP connection never closed after async chunked response with Connection: close") is directly addressed by this PR's fix for async/corked responses never hitting the close path (uncorkAndCloseIfNeeded())

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

Fixes #28396

🤖 Generated with Claude Code

[coderabbitai]

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

Updates HTTP/1.1 Connection: close handling across parsing, response state, teardown, runtime propagation, and integration tests.

Changes

HTTP/1.1 Connection: close correctness

Layer / File(s)	Summary
Request parsing and pipeline close latch packages/bun-uws/src/HttpParser.h	Adds hasConnectionToken(token) for comma-separated Connection header parsing, removes TransferEncoding, introduces sawConnectionClose, and stops consuming pipelined bytes after a close-flagged request.
Per-response parsing and framing state packages/bun-uws/src/HttpResponseData.h, packages/bun-uws/src/HttpContext.h, packages/bun-uws/src/HttpContextData.h, src/jsc/bindings/NodeHTTP.cpp	Moves parsing state to HttpResponseData, removes HttpContextData::isParsingHttp, expands response state bits, and marks Node HTTP responses that already wrote a Connection header.
Response close and framing path packages/bun-uws/src/HttpResponse.h	Adds uncorkAndCloseIfNeeded(...), updates Connection: close header emission, routes completion paths through the helper, and switches write/end framing checks to the new state bits, including the WebSocket upgrade snapshot.
Header emission guard in uws_res_end_without_body src/uws_sys/libuwsockets.cpp	Updates SSL and non-SSL close_connection branches to emit Connection: close only when the header block is finished conditions are met and no user-written Connection header is recorded.
Connection-close propagation in server responders src/runtime/server/HTMLBundle.rs, src/runtime/server/mod.rs, src/runtime/server/server_body.rs, src/runtime/webcore/streams.rs	Switches runtime response termination paths to pass resp.should_close_connection() through 404, info, HTML bundle fallback, and stream finalization code paths.
Connection: close integration coverage test/js/bun/http/serve.test.ts	Adds the raw-socket Connection: close test suite and Node createServer import alias, covering pipelining, bodies, HEAD, async handlers, streamed responses, 404s, and node:http header behavior.

Possibly related PRs

• oven-sh/bun#32781: Both PRs modify src/runtime/webcore/streams.rs response-ending paths, including end_from_js and finalization.
• oven-sh/bun#32800: Both PRs modify packages/bun-uws/src/HttpResponse.h response framing logic around no-body handling and header suppression.

Suggested reviewers

• cirospaciari
• Jarred-Sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly identifies the main change: Bun.serve stops dispatching pipelined requests after Connection: close. It is concise and specific.
Description check	✅ Passed	The description explains the problem, implementation, scope, edge cases, and verification results in substantial detail. It does not use the template headings exactly, but it provides the required inf…

---

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

[coderabbitai]

Actionable comments posted: 6

🤖 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 `@packages/bun-uws/src/HttpContext.h`:
- Around line 312-315: Shorten the framing-state reset comment in HttpContext so
it fits the repository’s 3-line comment limit. Edit the existing comment near
the writeHead/writeHeader framing flags logic to preserve the same meaning about
stale per-request flags on keep-alive sockets, but condense it to three lines or
fewer without changing behavior.

In `@packages/bun-uws/src/HttpParser.h`:
- Around line 240-243: Trim the newly added block comments in HttpParser.h to 3
lines max, as requested by the review. Update the long explanatory comments near
the connection-token scanner text and the other affected comment blocks so they
keep the same meaning but fit the repository comment-length guideline. Focus on
the comment blocks around the Connection-header/token logic and the other
highlighted parser comments, keeping each concise and within the 3-line limit.

In `@packages/bun-uws/src/HttpResponse.h`:
- Around line 99-113: Condense the expanded explanatory comments in
HttpResponse.h so each comment block stays within the repo’s 3-line limit while
preserving the local invariant; update the comment near the response completion
logic and the matching longer block later in the file, keeping only the
essential behavior around uncorking, deferred close during parsing, and
keepCorked handling, and move any extra rationale out of the code comments.

In `@src/uws_sys/libuwsockets.cpp`:
- Around line 1347-1349: The close-header path in uws_res_end_without_body can
still call writeHeader() after HTTP_END_CALLED, so update the guard around the
Connection: close append to treat HTTP_END_CALLED as headers already closed.
Make the change in uws_res_end_without_body and keep the behavior consistent
with the existing end checks already used later in that function, including the
repeated/late close path handled by the same logic.

In `@test/js/bun/http/serve.test.ts`:
- Around line 2571-2581: The new test comments in the Connection: close request
header block are too long for the repository comment limit. Trim the doc/comment
text around the relevant describe/helper blocks (including the
`describe("Connection: close request header", ...)` section and the related
helpers) so each comment is at most 3 lines, keeping only the durable invariant
needed to understand the test.
- Around line 2753-2754: The cleanup in the test’s finally block is using
nodeServer.close() without waiting for the close event, so the test can finish
before the server is actually shut down. Update the finally path in this test to
await the close operation by wrapping the callback-based close in a promise or
equivalent, so the server is fully closed before the test exits.

🪄 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: f5f8692d-3f2d-42ec-b5e3-2543d0e40a5b

📥 Commits

Reviewing files that changed from the base of the PR and between a1c39de and 93c5872.

📒 Files selected for processing (6)

• packages/bun-uws/src/HttpContext.h
• packages/bun-uws/src/HttpParser.h
• packages/bun-uws/src/HttpResponse.h
• packages/bun-uws/src/HttpResponseData.h
• src/uws_sys/libuwsockets.cpp
• test/js/bun/http/serve.test.ts

[github-actions]

This PR may be a duplicate of:

1. fix: close connection after async chunked response with Connection: close #28390 - Fixes the same latent bug where async chunked responses with Connection: close never actually closed the socket because internalEnd took the uncork() branch without checking the close flag

🤖 Generated with Claude Code

[robobun]

Thanks, I looked at #28396. I am not adding Fixes #28396: that issue is an umbrella of four separate bugs and this PR only touches one of them (sub-issue 4, the async Connection: close never closing). The other three (createConnection, upgrade socket writes, socket destroy on native close) are untouched here, so auto-closing the issue would be wrong.

Two more reasons it is not a clean match:

1. Sub-issue 4's repro is node:http with an async chunked response. That specific shape already closes correctly on current main and on the released 1.4.0 (the res.write() before res.end() uncorks the socket, so internalEnd's existing !isCorked() close check fires). What this PR fixes is the Bun.serve variant, where the one-shot corked try_end never reaches a close check; that one does reproduce on main.

2. The community PR for sub-issue 4, fix: close connection after async chunked response with Connection: close #28390, diagnoses the same root cause and applies the same "uncork then close-check" shape inside internalEnd. An earlier draft of this PR did exactly that and it regressed request-smuggling.test.ts: when the handler responds synchronously the socket is still corked by onData, so the close now fires from inside the request handler and the parser bails out via us_socket_is_closed() before it validates the rest of that request's body. Concretely, the 26 chunk size strict hex digit validation cases (invalid chunk-size bytes with Connection: close set) start returning 200 OK instead of 400 Bad Request. This PR gates the new close on !flags.isParsingHttp so that inside the parser the close still defers to onData's tail. I left a note on fix: close connection after async chunked response with Connection: close #28390 with the details.

[robobun]

Not a duplicate of #28390, though they share the async-close root cause. The differences are covered in my earlier comment above; the short version: #28390's fix shape (uncork then close-check in internalEnd with no guard) regresses the chunked-body 400 validation on current main because it also closes from inside the synchronous request handler, and its node:http repro no longer fires on the released 1.4.0. This PR gates that close on !flags.isParsingHttp to avoid the regression, and also fixes the three other Connection: close violations (the token parsing, the pipelined-request discard, and the missing response header), none of which #28390 touches. I left a detailed note on #28390.

[claude]

I didn't find any bugs, but this reworks connection-close handling across the uWS HTTP parser/response lifecycle (the sawConnectionClose latch, the uncork-then-close refactor gated on isParsingHttp, and the wroteConnectionHeader suppression), so it's worth a human pass on the close/parser interaction.

Extended reasoning...

Overview

This PR fixes RFC 9112 §9.6 compliance in Bun.serve by changing how the vendored uWS fork handles a request's Connection: close option. It touches five production source files: HttpParser.h adds hasConnectionToken() and a per-socket sawConnectionClose latch that stops the parse loop and discards pipelined bytes; HttpContext.h switches detection from a length check to the token scanner and resets the new wroteConnectionHeader flag per request; HttpResponse.h extracts an uncorkAndCloseIfNeeded() helper and rewires both chunked and non-chunked end paths through it, gated on !flags.isParsingHttp; HttpResponseData.h adds the wroteConnectionHeader member; and libuwsockets.cpp updates both SSL variants of uws_res_end_without_body. A 14-case test block in serve.test.ts covers the new behavior over a raw socket.

Security risks

The HTTP parser loop and connection-close semantics are request-smuggling-adjacent. The PR description itself notes that an earlier draft (uncork-then-close without the isParsingHttp guard) regressed 26 chunked-size strict-validation cases in request-smuggling.test.ts by closing the socket from inside the synchronous handler before the parser could 400 the malformed body. The current shape defers that close to onData's tail when inside the parser, which the author reports passes the smuggling suite, but the correctness of that gate — and whether discarding pipelined bytes via consumedTotal += length; break; interacts cleanly with the post-padding / fallback-buffer paths in consumePostPadded — is exactly the kind of invariant a human familiar with this parser should sanity-check.

Level of scrutiny

High. This is core HTTP server connection-lifecycle code that every Bun.serve and node:http request flows through. The change is not mechanical: it refactors the uncork/close decision out of two open-coded sites into a helper with a new gating condition, adds state to both the parser (sawConnectionClose) and the response (wroteConnectionHeader), and changes when sockets are shut down. The PR also overlaps with two other open changes (#28390 on the same async-close root cause, and #32945 on HTTP/1.0 keep-alive sharing the same hasConnectionToken helper), so coordination matters.

Other factors

The PR is well-described and the test coverage is thorough (sync/async handlers, HEAD via end_without_body, POST body delivery, node:http single-header check, and negative cases for the old length-5 bug and substring matching). All CodeRabbit comments have been addressed in 2696d48, including the HTTP_END_CALLED guard. CI shows musl build failures on the first commit but the test lanes appear clean. Given the criticality of the code path and the explicit note that an earlier draft regressed the request-smuggling tests, a maintainer who owns this layer should review the isParsingHttp gate and the parser-loop discard before merge.

[robobun]

whether discarding pipelined bytes via consumedTotal += length; break; interacts cleanly with the post-padding / fallback-buffer paths in consumePostPadded

I traced every entry into fenceAndConsumePostPadded for this before pushing; writing it down here since it is the one thing not already in the description.

The invariant that makes the discard safe is ordering: the latch is set for request N after its headers parse (before the body branches run), and the discard is at the top of iteration N+1. So a close-flagged request's own body is always consumed and validated first; only bytes received after that body are discarded. That is also why the 26 chunk size strict hex digit validation cases in request-smuggling.test.ts still 400 correctly (the chunked validation runs in iteration N, before the discard can fire).

Per entry path into consumePostPadded:

• Fallback buffer (the close-flagged request's headers arrive split across reads): the <ConsumeMinimally=true> call on the fallback buffer latches sawConnectionClose and parses exactly that one request. consumePostPadded then consumes its body (via the remainingStreamingBytes the <true> call set) before the final <false> call runs, so the <false> loop-top discard only ever sees bytes past the body.
• Body spanning multiple reads (remainingStreamingBytes != 0): step 1 of consumePostPadded consumes body bytes bounded by the remaining Content-Length or the chunked state, and its short-read branch returns before <false> is reached at all. Only once the body is complete does <false> run on what is left, which is exactly the pipelined data.
• Post padding: the two fence bytes are written at data[length] and data[length + 1] before the loop; the new check is inside the loop and only does consumedTotal += length; break, never reading at or past length. It is the same shape as the pre-existing isConnectRequest discard a few lines below it.

sawConnectionClose lives on HttpParser, a base of HttpResponseData, which is placement-new'd per connection in onOpen, so it is connection-scoped and starts false; a latched socket is closed by the response path, so it never carries over.

On the other two points: the isParsingHttp gate and its interaction with the smuggling suite are covered in the PR description (an ungated version of this change is what #28390 proposes, and I left a note there), and I adopted hasConnectionToken() byte for byte from #32945 so the two compose in either merge order.

[robobun]

Status for reviewers (current as of 5bf4ec2ac3, which merges current main): the change is complete and mergeable.

Reproducing the bug against released Bun or main, with Bun.serve({ port: 0, fetch: req => new Response("body:" + new URL(req.url).pathname) }) listening:

printf 'GET /a HTTP/1.1\r\nHost: x\r\nConnection: close\r\n\r\nGET /b HTTP/1.1\r\nHost: x\r\n\r\n' | nc 127.0.0.1 <port>

Both /a and /b are answered. RFC 9112 section 9.6 says the server MUST NOT process anything after the request that carried the close connection option. With this PR only /a is answered and the server closes.

Diff. Four files (two source, two test). main has since grown the state bitfield, resetResponseState(), the HTTP_CONNECTION_CLOSE seeding in HttpContext, and the response-side close plumbing, so most of the earlier revision of this PR is already upstream. What remains:

• HttpParser.h: a sawConnectionClose latch on the parser, so fenceAndConsumePostPadded discards everything after the close-flagged request. The gate is the last check before getHeaders() (so malformed trailing bytes are discarded, not answered with a 400). It runs after the node:http tunnel check, read-pause park, and leading CR/LF skip, so a switched-protocol or paused connection is unaffected, and a stray CRLF after the close-flagged request is tolerated the way llhttp's closed state tolerates it. HttpRequest::hasConnectionClose() does the token-list detection.
• HttpContext.h: the dispatch lambda reads the parser's sawConnectionClose latch in place of the getHeader("connection").length() == 5 heuristic, which missed keep-alive, close and matched any five-byte value. The parser sets the latch before it calls the handler, and the gate stops the loop before any later request is parsed, so at dispatch the latch always describes the current request. Each request scans its Connection header once (2f6f85e).
• test/js/bun/http/serve.test.ts: ten cases. Five close spellings that fail on released Bun, a four-request pipeline with the close on the second request (fails on main since Bun.serve: keep the socket corked across the requests of one read #42762), an async handler behind a pipelined request (on main the response to /a is never sent), two persistent-connection controls (one is the five-byte non-close token that trips the old heuristic), and a malformed-follow-up case. The close cases assert that the server closed the socket.
• test/js/node/http/node-http.test.ts: three node:http cases for the bytes after a close-flagged request while its response is still pending. A CRLF after a POST body and a blank line after an HTTP/1.0 request are not a clientError; other bytes after the CRLF are HPE_CLOSED_CONNECTION. All six payloads I compared now behave the same as Node 26.3.0.

The three earlier changes-requested reviews (per-header overhead, the duplicate isParsingHttp flag, packing bools into state) referred to code that is no longer in this diff.

CI. Build 116791 on the current head: all 13 build steps passed, so the change compiles on every platform. One job is red, debian 13 x64 - test-bun, and it is unrelated to this diff: in test/js/bun/s3/s3.test.ts one test that uploads a large file to the real R2 service timed out after 15 s (430 other tests in that file passed). This PR does not touch the S3 client or the HTTP client. I reported it for triage as a failure on main. None of this PR's tests appear in the error or flaky annotations. The ASAN and Windows test lanes were still running when I wrote this. The previous head (bdce2489b9, before 2f6f85e and the merge of main) was fully green: build 115810, 181 jobs passed, 0 failed.

[Jarred-Sumner]

Too much overhead in the C++ changes. Rewrite & simplify.

[robobun]

Done in 5b3b203. The three places this was adding per-request work in the C++, and what each became:

1. writeHeader() had gained a key.length() == 10 && strncasecmp(key, "connection") check on every header of every response. Gone entirely. It only existed so node:http (whose JS layer writes its own Connection: close) would not get a second one, and node:http's writeHead already claims ownership of the Date header with one unconditional assignment. wroteConnectionHeader is now set the same way, on the same line, so HttpResponse::writeHeader is byte-for-byte what it was on main.

2. hasConnectionToken("close") was being computed twice per request (the parser latched it, then HttpContext scanned the headers again). HttpContext now reads the parser's latch, so the Connection header is scanned once, and only behind the existing bloom-filter check.

3. The deferred-close gate was a socket-group deref plus a pointer compare on every response end (getSocketContextDataS(this)->parsingHttpSocket == this). It is now one bool on the HttpResponseData the function already has in hand, set and cleared by onData next to the existing isIdle write. HttpContextData is back to being untouched.

What remains per request in the hot path is: one bloom-gated Connection-header scan, one predicted-not-taken branch at the top of the parser loop, and four bool stores. uncorkAndCloseIfNeeded() is the two previously copy-pasted close blocks folded into one, with the uncork moved ahead of the close decision (that reordering is the actual fix for async responses never closing).

Behavior and tests are unchanged: 17 pass (9 fail on the released 1.4.0), and request-smuggling.test.ts plus the node:http suite are clean. Happy to cut further if there is a specific part you want gone.

[Jarred-Sumner]

Why do we need both flags.isParsingHttp and response data isParsingHttp?

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@packages/bun-uws/src/HttpContext.h`:
- Around line 414-416: The `consumePostPadded()` / `upgrade()` flow can
synchronously destroy `HttpResponseData`, so clearing `isParsingHttp` via the
cached `httpResponseData` pointer is unsafe. Update the `HttpContext` logic to
only touch `httpResponseData->isParsingHttp` after verifying the returned socket
still owns valid HTTP response data, and keep the
`httpContextData->flags.isParsingHttp` reset separate from the stale
response-data write.

In `@packages/bun-uws/src/HttpResponse.h`:
- Around line 159-162: The automatic close-header guard in
HttpResponseData::writeHeader() is relying on wroteConnectionHeader, but that
flag is not being updated when the application writes a Connection header.
Update the writeHeader path so it detects user-supplied Connection headers and
sets wroteConnectionHeader accordingly, keeping the existing close-connection
logic from adding Connection: close when the user has already provided one. Use
the existing HttpResponseData<SSL> state and writeHeader symbol to locate the
right branch.

🪄 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: 1fdc519f-c88e-42e2-a756-d6a461ef9bf2

📥 Commits

Reviewing files that changed from the base of the PR and between 93c5872 and 5b3b203.

📒 Files selected for processing (11)

• packages/bun-uws/src/HttpContext.h
• packages/bun-uws/src/HttpParser.h
• packages/bun-uws/src/HttpResponse.h
• packages/bun-uws/src/HttpResponseData.h
• src/jsc/bindings/NodeHTTP.cpp
• src/runtime/server/HTMLBundle.rs
• src/runtime/server/mod.rs
• src/runtime/server/server_body.rs
• src/runtime/webcore/streams.rs
• src/uws_sys/libuwsockets.cpp
• test/js/bun/http/serve.test.ts

[robobun]

We don't. Removed in 27865b4; there is one flag now.

flags.isParsingHttp on HttpContextData was pre-existing (one per server), and its only reader was upgrade(). The per-socket one is what uncorkAndCloseIfNeeded() needs, because a Connection: close response can finish inside a different socket's onData (its promise continuation runs in that request handler's microtask drain), where the per-server flag would wrongly defer the close. I had initially left the context flag alone to keep upgrade() out of the diff, which is how there ended up being two.

upgrade()'s question is also really about this socket, not the server, so it now reads the same per-socket flag (captured into a local before ~HttpResponseData() runs, since us_socket_adopt can invalidate the pointer). That is a strict improvement: the per-server flag would also have mishandled an upgrade performed from inside another socket's handler, handing the new WebSocket to the wrong onData tail. flags.isParsingHttp is deleted from HttpFlags, which is C++-internal and never crosses the FFI.

One follow-up on top of that (09f7554): CodeRabbit caught that onData's tail was then clearing the flag through the httpResponseData pointer it cached on entry, which a synchronous upgrade() invalidates. The clear moved inside the existing if (returnedData != nullptr) block, which already dereferences that pointer for the same reason: non-null means the socket was neither upgraded nor closed by the handler.

serve.test.ts's Connection block (17), websocket-server.test.ts (100), the upgrade-reentrant and node:http-with-ws suites, and request-smuggling.test.ts all pass on the result.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🔴 packages/bun-uws/src/HttpContext.h:416 — This unconditional write to httpResponseData runs after a synchronous WebSocket upgrade has already called responseData->~HttpResponseData() and us_socket_adopt() (HttpResponse.h:344/352), so it writes to a destructed object — or, when the adopt keeps the block in place, into a live WebSocketData that was just placement-new'd over the same bytes. Every other post-parser httpResponseData access in this function is inside the if (returnedData != nullptr) guard at line 431 for exactly this reason; this write should move there too (the companion httpContextData->flags.isParsingHttp = false on line 415 is per-context and was always safe).

  Extended reasoning...

  What the bug is. httpResponseData is captured at HttpContext.h:262 as us_socket_ext(s) before consumePostPadded runs. The new line 416 writes httpResponseData->isParsingHttp = false unconditionally after the parser returns. But during a synchronous WebSocket upgrade inside the request handler, HttpResponse::upgrade() explicitly calls responseData->~HttpResponseData() (HttpResponse.h:344) and then us_socket_adopt(...) (HttpResponse.h:352, with the comment "Adopting a socket invalidates it, do not rely on it directly to carry any data"). After that, the captured httpResponseData pointer no longer refers to a live HttpResponseData.

  The two failure modes. us_socket_adopt calls us_poll_resize (epoll_kqueue.c:486): if sizeof(WebSocketData) + sizeof(UserData) > sizeof(HttpResponseData) it allocates a new block and links the old socket onto loop->data.closed_head for deferred free (context.c:296‑297) — so line 416 writes to a destructed object on the close-list (formal UB, never read again). If the new ext fits in the old allocation, us_poll_resize returns p unchanged and webSocket->init() (HttpResponse.h:363) placement-news WebSocketData over the same bytes — so line 416 writes 0x00 at offsetof(HttpResponseData, isParsingHttp) into a live WebSocketData, which is type-punned memory corruption whose effect depends entirely on struct layout.

  Why the existing code doesn't prevent it. The pre-existing code is explicitly aware of this hazard: the comment at line 430 says "except for nullptr (closed socket, or upgraded socket)", and the only other post-parser httpResponseData access (line 443) is gated behind if (returnedData != nullptr). The request-handler lambda detects the upgrade via httpContextData->upgradedWebSocket (line 337‑340) and returns nullptr, so result.returnedData is nullptr and that guard correctly excludes the upgraded socket. Line 416 sits outside that guard, making it the only unguarded post-parser write to httpResponseData in the function. The PR's commit message says the new flag is "set and cleared by onData next to the existing isIdle write", but isIdle is only written before the parser (line 270), never after — there is no precedent for an unguarded post-parser write here. The same hazard applies to the closed-socket path: onClose (HttpContext.h:237) also destructs HttpResponseData, and the request handler returns nullptr for that too.

  Step-by-step proof.

  1. A Bun.serve app with a websocket handler receives a WebSocket upgrade request; the fetch handler calls server.upgrade(req) synchronously (the common pattern).
  2. onData captures httpResponseData = us_socket_ext(s) at line 262, sets isParsingHttp = true at line 269, and enters consumePostPadded.
  3. The request handler runs HttpResponse::upgrade(): line 344 runs responseData->~HttpResponseData(); line 352 runs us_socket_adopt, which either reallocates the socket or keeps it in place; line 363 placement-news WebSocketData over the ext bytes; line 371 sets httpContextData->upgradedWebSocket.
  4. Back in onData's request-handler lambda, line 337 sees upgradedWebSocket set and returns nullptr, so result.returnedData == nullptr.
  5. Line 416 executes httpResponseData->isParsingHttp = false — a write through a pointer to a destructed object whose storage now holds either nothing (close-list) or a live WebSocketData.
  6. Line 431's if (returnedData != nullptr) is false, so the function falls through to the upgraded-websocket branch at line 457 — but the stray write has already happened.

  Impact. The PR description says websocket-server*.test.ts passes, so the corruption is currently silent — most likely Bun's UserData makes WebSocketData + UserData larger than HttpResponseData (the realloc case), or the byte happens to land on padding/a benign field. But that is layout-dependent and can change with any field addition to either struct, any compiler/platform alignment difference, or a smaller UserData. It is a write-after-lifetime-end on every synchronous WebSocket upgrade regardless.

  Fix. Move httpResponseData->isParsingHttp = false; inside the if (returnedData != nullptr) block at line 431 (e.g. right after the guard opens), matching the existing convention. The flag is only read by uncorkAndCloseIfNeeded() to defer the socket close to onData's tail, and an upgraded/closed socket has no such tail to defer to, so clearing it there is unnecessary anyway. The companion httpContextData->flags.isParsingHttp = false at line 415 is on per-context data which is never adopted and can stay where it is.

[robobun]

On the "Additional findings" item about HttpContext.h:416 (httpResponseData->isParsingHttp = false running after a synchronous upgrade() has destroyed the HttpResponseData and possibly reallocated the socket): confirmed real, and it was already fixed in 09f7554, which landed just before this review ran. CodeRabbit independently flagged the same bug; both bots converging on it is about as strong a signal as a review can give.

The fix is exactly what this finding proposes: the clear moved inside the existing if (returnedData != nullptr) block, which already dereferences httpResponseData and is non-null precisely when the socket was neither upgraded nor closed by the handler. Your mechanism trace is also the most precise of the three (the us_poll_resize realloc vs. in-place-adopt split is why my 164 WebSocket upgrade tests passed either way: the stray write was landing in live WebSocketData bytes, which neither ASAN nor any JS-observable test can see), so thank you for writing it down; it is the right record for a future reader of that line.

[Jarred-Sumner]

Can the 6 booleans and the uint8_t state be combined into a single uint16_t state?

[robobun]

Four of the six, done in e14567a. state is a uint16_t now, with HTTP_FROM_ANCIENT_REQUEST, HTTP_NO_BODY_STATUS, HTTP_CLOSE_DELIMITED and HTTP_WROTE_CONNECTION_HEADER as the new bits. Two things came out for free:

• HttpContext already resets state wholesale at the start of every request (state = HTTP_RESPONSE_PENDING), so the three explicit per-request bool resets that followed it are gone.
• Every condition that mixed a state & mask test with && !someBool terms is now a single mask test.

The other two can't become state bits, because that wholesale per-request reset would wipe them mid-flight:

• isConnectRequest is per connection, not per response, and consumePostPadded() takes it by bool& so that upgradeToTunnelMode()'s write from inside the request handler reaches the parse that is already running. That is how node:http CONNECT tunneling stops the parser from interpreting the rest of the buffer as HTTP, and a state bit can't be passed by reference.
• isParsingHttp brackets one onData call, which can span several pipelined requests, each of which resets state.

Both keep a short comment on the struct saying why. Getting them in too would need state split into a per-response mask and a per-connection mask that the reset preserves, plus a parser signature change for the reference. I can do that if you'd rather have literally everything in one word, but it looked like more machinery than it saves.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

packages/bun-uws/src/HttpResponse.h (2)

510-513: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Keep this framing comment within the comment-length limit.

This block is four lines.

Proposed trim

-        /* When the user wrote an explicit Transfer-Encoding header but never started
-         * streaming writes (a one-shot end), the body must still be chunk-framed.
-         * Terminate the headers and enter chunked mode so internalEnd() takes the
-         * chunked path instead of writing the raw body after the headers. */
+        /* With explicit Transfer-Encoding and one-shot end, terminate headers
+         * and enter chunked mode so internalEnd() frames the body. */

As per coding guidelines, “Keep code comments to 3 lines max.”

🤖 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 `@packages/bun-uws/src/HttpResponse.h` around lines 510 - 513, Shorten the
framing comment in HttpResponse::internalEnd to fit the 3-line comment limit
while preserving the key behavior: explicit Transfer-Encoding with a one-shot
end must still enter chunked mode so the body is chunk-framed. Keep the
reference to the headers being terminated and internalEnd() taking the chunked
path, but merge the four-line explanation into at most three concise lines.

Source: Coding guidelines

---

499-503: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Condense the close-delimited response comment.

This comment block is five lines.

Proposed trim

-        /* Close-delimited responses (the user removed the framing headers):
-         * raw body bytes after the headers, no Content-Length, no chunked
-         * framing, and the connection closes to delimit the message. The
-         * CONNECTION_CLOSE state is set directly so internalEnd() closes the
-         * socket without adding a Connection: close header the user removed. */
+        /* Close-delimited responses: raw body bytes with no Content-Length or
+         * chunked framing; close delimits the message. Set close directly so
+         * internalEnd() does not re-add a removed Connection header. */

As per coding guidelines, “Keep code comments to 3 lines max.”

🤖 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 `@packages/bun-uws/src/HttpResponse.h` around lines 499 - 503, Condense the
close-delimited response comment in HttpResponse-related handling so it fits the
3-line max guideline. Keep the explanation around the close-delimited flow and
CONNECTION_CLOSE behavior, but remove redundant wording in the comment block
near the logic that sets CONNECTION_CLOSE and calls internalEnd().

Source: Coding guidelines

src/jsc/bindings/NodeHTTP.cpp (1)

728-731: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Shorten this sentinel comment to three lines.

The block is four lines; keep the invariant concise.

Proposed trim

-                // node:http marks framing decisions with a NUL-named sentinel
-                // pair instead of a real header: value "1" = close-delimited
-                // (the user removed the framing headers), value "2" = no body
-                // (HEAD - suppress all body framing like 204/304).
+                // node:http uses a NUL-named sentinel: value "1" means
+                // close-delimited, value "2" means no body (HEAD-style
+                // framing suppression, like 204/304).

As per coding guidelines, “Keep code comments to 3 lines max.”

🤖 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/jsc/bindings/NodeHTTP.cpp` around lines 728 - 731, Shorten the sentinel
comment in NodeHTTP::addHeaders to three lines by trimming the explanatory
wording while preserving the key invariant about the NUL-named header sentinel
and the meanings of values "1" and "2". Update only the comment block near the
framing decision logic so it remains concise and within the 3-line coding
guideline.

Source: Coding guidelines

🤖 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.

Outside diff comments:
In `@packages/bun-uws/src/HttpResponse.h`:
- Around line 510-513: Shorten the framing comment in HttpResponse::internalEnd
to fit the 3-line comment limit while preserving the key behavior: explicit
Transfer-Encoding with a one-shot end must still enter chunked mode so the body
is chunk-framed. Keep the reference to the headers being terminated and
internalEnd() taking the chunked path, but merge the four-line explanation into
at most three concise lines.
- Around line 499-503: Condense the close-delimited response comment in
HttpResponse-related handling so it fits the 3-line max guideline. Keep the
explanation around the close-delimited flow and CONNECTION_CLOSE behavior, but
remove redundant wording in the comment block near the logic that sets
CONNECTION_CLOSE and calls internalEnd().

In `@src/jsc/bindings/NodeHTTP.cpp`:
- Around line 728-731: Shorten the sentinel comment in NodeHTTP::addHeaders to
three lines by trimming the explanatory wording while preserving the key
invariant about the NUL-named header sentinel and the meanings of values "1" and
"2". Update only the comment block near the framing decision logic so it remains
concise and within the 3-line coding guideline.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5631f141-54f3-417d-a0cc-2d6ff3c1f96f

📥 Commits

Reviewing files that changed from the base of the PR and between 9262a41 and e14567a.

📒 Files selected for processing (6)

• packages/bun-uws/src/HttpContext.h
• packages/bun-uws/src/HttpResponse.h
• packages/bun-uws/src/HttpResponseData.h
• src/jsc/bindings/NodeHTTP.cpp
• src/uws_sys/libuwsockets.cpp
• test/js/node/http/node-http.test.ts

[robobun]

All three of these comments already exist on main and are not part of this PR's diff. CodeRabbit flagged them because the refactor in e14567a changed the condition line directly below each one, but the comment blocks themselves are untouched:

• HttpResponse.h:496 on main ("Close-delimited responses ...")
• HttpResponse.h:507 on main ("When the user wrote an explicit Transfer-Encoding header ...")
• NodeHTTP.cpp:720 on main ("node:http marks framing decisions with a NUL-named sentinel ...")

Rewording pre-existing comments is outside the scope of this PR, so I am leaving them as they are. The comments this PR does add are all 3 lines or fewer.

[robobun]

The uws_res_end_without_body finding from the review is real. Fixed in 7c16ab4 with the uncorkAndCloseIfNeeded(data, false) it proposed, plus a regression test.

What I confirmed before changing anything:

• uws_res_end_without_body sets HTTP_CONNECTION_CLOSE and writes the header but never closes. Its synchronous callers are closed by onData's tail, and its corked callers by HttpResponse::cork()'s tail, which is why the async HEAD handler I checked first was already fine.
• HTMLBundle::resume_pending_responses() is neither: it runs from the bundle-completion task, outside any onData frame, and does not cork. With a Connection: close request the server replied HTTP/1.1 500 Build Failed + Connection: close and then sat on the socket until the idle timeout.
• The bundle build is started lazily by the first request (Route::init leaves the route in State::Pending, and the State::Pending arm of on_any_request is what calls schedule_bundle), so the first request is always the queued, asynchronously resumed one. That makes the test deterministic, not a race.

While anchoring the edit I found a second function with the same end-of-function shape, uws_res_end_sendfile. I left it alone deliberately: it ignores its close_connection parameter completely (no header, no state bit, no close), which is a larger pre-existing bug in a function this PR does not otherwise touch and whose only caller (FileResponseStream.rs) has an empty diff against main. Happy to do that one as a follow-up PR.

[claude]

No issues found in the rebased diff — both post-rebase findings (gate-before-getHeaders(), tunnel-check ordering) are addressed in 473daf1 and the 8 new tests cover the close variants, controls, and malformed-trailing-bytes case. Deferring to a human because this is the per-request hot path in fenceAndConsumePostPadded and the earlier revision drew maintainer feedback on parser overhead; the remaining diff is small but a human should sign off on core HTTP parser control-flow changes.

What was reviewed:

• hasConnectionClose() token-list scan: mirrors getTransferEncoding()'s shape, handles multiple Connection lines, OWS, mixed case; bloom-filter fast path preserved.
• Bun.serve discard path returns success(consumedTotal + length, user) so nothing lands in the fallback buffer and onData's existing shouldCloseConnection() tail closes; traced the cross-segment and ConsumeMinimally fallback paths.
• Tunnel check stays above the gate so node:http Upgrade-with-body is unaffected; the gate is above getHeaders() so malformed trailing bytes are discarded rather than 400'd.
• The nope! control confirms the old length() == 5 false-positive is gone.

Extended reasoning...

Overview

Three-file rebase of a longer earlier revision. HttpParser.h: (1) new HttpRequest::hasConnectionClose() that scans every Connection header line as a comma-separated, OWS-trimmed, case-insensitive token list (same shape as the existing getTransferEncoding()); (2) the per-connection nodeHttpSawConnectionClose latch is renamed sawConnectionClose and its if constexpr (IsNodeHttp) guard dropped so it applies to Bun.serve too; (3) the loop-top gate now sits after the IsNodeHttp && isConnectRequest tunnel check but before getHeaders(), and for Bun.serve returns success(consumedTotal + length, user) to discard the rest of the buffer. HttpContext.h: the dispatch lambda's getHeader("connection").length() == 5 becomes hasConnectionClose(). serve.test.ts: 8 raw-socket tests (5 close spellings, 2 persistent-connection controls, 1 malformed-trailing-bytes).

Security risks

This is RFC 9112 §9.6 compliance — the same neighborhood as request-smuggling protections. The change is strictly a tightening (fewer requests dispatched, not more): a pipelined request after Connection: close is now discarded, and the token scan replaces a heuristic that both under-matched (keep-alive, close) and over-matched (any 5-byte value). I traced the discard path to confirm nothing reaches the fallback buffer and no second response is written, including for malformed trailing bytes (the 2302116 hoist). The tunnel-check ordering fix in 473daf1 keeps node:http Upgrade-with-body flowing. I did not find a way for the new gate to cause a request that should be served to be dropped.

Level of scrutiny

High. fenceAndConsumePostPadded is the per-request parse loop for every HTTP/1.x connection in both Bun.serve and node:http, and the earlier revision of this PR drew explicit maintainer changes-requested over per-header C++ overhead. That feedback was about code no longer in this diff, but a human should confirm the now-unconditional hasConnectionClose() scan (called once in the parser latch and once in the dispatch lambda per request) is acceptable in the hot path — it's a linear header walk behind a bloom-filter fast path, same cost class as getTransferEncoding(), but it's their call.

Other factors

All prior review threads (bot-only; no human reviewer on record) are resolved. Both of my post-rebase findings were fixed exactly as suggested. The tests are hermetic (raw net.connect to port: 0), settle on server close or an over-count fast-fail, and assert both the handler-side (handled) and wire-side (responses, body markers) invariants. request-smuggling.test.ts and the node:http HPE_CLOSED_CONNECTION test were reported passing by the author. The diff line count in the PR body (hasConnectionClose is 39 lines) is the largest single addition; everything else is a rename, a guard removal, and a block move.

[claude]

Code review found no new issues

No new issues were found in this update; 1 finding from earlier reviews is still open above.

[claude]

I re-reviewed after bdce248 and didn't find any new issues — the sawConnectionClose gate now sits below the node:http CR/LF skip and above getHeaders(), with tests covering both the trailing-CRLF and non-CRLF-bytes cases. Given this is the HTTP/1.1 request-boundary parser and there are outstanding changes-requested reviews from a maintainer, a human sign-off is still the right next step.

Checked: hasConnectionClose() token-scan bounds (all pos/tokenEnd accesses guarded, strncasecmp length-gated); gate ordering vs. tunnel/park/CRLF-skip matches the comment's claims; no remaining getHeader("connection").length() == 5 sites; the Bun.serve discard path returns consumedTotal + length so onData's tail still runs its close check.

Extended reasoning...

Overview

The PR replaces a naive length() == 5 heuristic with a proper token-list scanner (HttpRequest::hasConnectionClose()) and generalizes the previously node:http-only sawConnectionClose latch so Bun.serve also stops dispatching pipelined requests after a Connection: close (RFC 9112 §9.6). Since my last review, commit bdce248 reordered the gate to run after the node:http leading-CR/LF skip in fenceAndConsumePostPadded, so a stray CRLF after a close-flagged POST body or HTTP/1.0 request no longer raises HPE_CLOSED_CONNECTION before the response goes out — matching llhttp's s_n_llhttp__internal__n_closed, which loops on \r/\n and errors only on other bytes. The commit also added three node-http.test.ts cases covering both sides of that behavior. The gate remains above getHeaders(), below the tunnel check, and below the park check, and each ordering constraint in the comment block is accurate against the surrounding code (HttpParser.h:1151/1159/1173/1196/1202).

Security risks

This is the HTTP/1.1 request-boundary logic — directly adjacent to request-smuggling territory. The change tightens behavior (fewer requests dispatched, more accurate close detection) rather than loosening it, and the discard path for Bun.serve returns success(consumedTotal + length, user) so the caller's post-loop close check still fires with HTTP_CONNECTION_CLOSE set. The token scanner walks untrusted header bytes, but every index is bounds-checked against value.length() before dereference and the strncasecmp is gated on tokenEnd - tokenStart == 5. I found no new attack surface, but parser-boundary changes in a request-smuggling-sensitive loop warrant a maintainer's eyes regardless.

Level of scrutiny

High. packages/bun-uws/src/HttpParser.h is the hand-rolled HTTP parser shared by both Bun.serve and node:http compat; a mistake here is a request-smuggling or DoS vector for every Bun HTTP server. The diff is small and the ordering is now well-commented, but three CHANGES_REQUESTED reviews from Jarred-Sumner remain on the PR without a subsequent approval. The PR description states those reviews targeted code since removed from the diff (the response-header / bitfield refactor), which is consistent with the current 4-file diff, but I cannot verify the maintainer's concerns are cleared from metadata alone.

Other factors

Test coverage is solid for the fix itself: serve.test.ts covers pipelined-after-close, close-mid-read, and async-handler cases; node-http.test.ts now covers the CRLF-tolerance and HPE_CLOSED_CONNECTION error-code cases. I confirmed no getHeader("connection").length() == 5 call sites remain in the tree. The scope is now tightly focused on the §9.6 MUST (the SHOULD half — response Connection: close header — was dropped per the description), which keeps the diff reviewable.

[Jarred-Sumner]

don't we already track this in a bool somewhere?

[robobun]

Yes, and that bool is the one this PR uses. It does not add a field.

main already has HttpParser::nodeHttpSawConnectionClose. It is set and checked only under if constexpr (IsNodeHttp). This PR renames it to sawConnectionClose in place and removes the IsNodeHttp guard, so Bun.serve uses the same latch. It stays in the same slot after remainingStreamingBytes, so the layout does not change. The name lost the nodeHttp prefix because it is no longer node-only. I can keep the old name if you prefer the smaller diff.

The other place this is tracked is the HTTP_CONNECTION_CLOSE bit in HttpResponseData::state. It cannot drive the gate, which is why main keeps the latch separate:

• state describes the response that is in flight. For a node:http pipelined dispatch the per-response reset is skipped (HttpContext.h: "it still belongs to the in-flight response"). When the close-flagged request is a queued one, state does not have the bit.
• The gate runs in HttpParser, before getHeaders(). HttpResponseData derives from HttpParser, so the parser cannot read state. The dispatch lambda can read it, but it runs after the next request head is parsed. That is where the check sat on main, and there malformed trailing bytes get a 400 from Bun.serve, or their own parse error from node:http where Node reports HPE_CLOSED_CONNECTION.

[robobun]

A gap check against this branch. A second fix for the same report was in progress on another branch (linked below). It is not opened as a PR, to keep one PR for the report.

Verified on debug builds of main 7d792a5 with packages/ from bdce248:

• The report's repro (/0, /1 with Connection: close, /2, /3 in one write) answers R:0 R:1 and closes. All 10 cases of this PR pass.
• A request that the handler answers with a Connection: close response header does not stop the read. GET /a, GET /close, GET /b, GET /c in one write, where /close returns new Response(path, { headers: { Connection: "close" } }): all four handlers run, four responses, the connection stays open. Canary 09bb546 (before Bun.serve: keep the socket corked across the requests of one read #42762) runs /a /close and closes, so the middle position is part of the same regression. The same request at the start of a read was already wrong before Bun.serve: keep the socket corked across the requests of one read #42762 (same canary).

Cause. The latch reads the request head. A close mark from the response side (writeFetchHeadersToUWSResponse in NodeHTTP.cpp, internalEnd(..., closeConnection), a close-delimited end()) only sets HTTP_CONNECTION_CLOSE in state. The dispatch of the next request calls resetResponseState(), which clears it.

A patch that closes it. One test of the existing HTTP_CONNECTION_CLOSE bit at the top of the dispatch lambda in HttpContext::onData, before the reset. It adds no field and does not touch the parser. It applies cleanly on top of this branch (a different hunk of HttpContext.h):

            HttpResponseData<SSL> *httpResponseData = (HttpResponseData<SSL> *) us_socket_ext((us_socket_t *) s);

            /* Bun.serve, RFC 9112 9.6: a complete earlier response closes this
             * connection (Connection: close, HTTP/1.0), so the requests behind it
             * are not processed; resetResponseState() below would drop the mark.
             * Same steps as onData's tail. Runs before the timeout reset: a socket
             * that has not drained keeps its timeout until onWritable closes it. */
            if constexpr (!IsNodeHttp) {
                constexpr uint32_t closeOrPending = HttpResponseData<SSL>::HTTP_CONNECTION_CLOSE | HttpResponseData<SSL>::HTTP_RESPONSE_PENDING;
                if ((httpResponseData->state & closeOrPending) == HttpResponseData<SSL>::HTTP_CONNECTION_CLOSE) [[unlikely]] {
                    us_socket_unref((us_socket_t *) s);
                    ((AsyncSocket<SSL> *) s)->uncork();
                    ((HttpResponse<SSL> *) s)->closeIfDoneAndMarked(httpResponseData);
                    return nullptr;
                }
            }

Commit 1fff7d4 on robobun/65cb78f8/serve-close-mid-pipelined-read. It adds 7 cases at the end of "requests pipelined in one read" in serve.test.ts: three closers (a Connection: close request, an HTTP/1.0 request, a request answered with Connection: close), each at the start and in the middle of a read, plus the middle case over TLS.

packages/ on main 7d792a5	the 10 cases of this PR	those 7 cases
this PR	10 pass	5 pass (the two response header cases fail)
only the patch above	5 pass (both token lists, the async handler, the nope! control and the malformed bytes fail)	7 pass
both	10 pass	7 pass

With both applied the rest of "requests pipelined in one read" also passes, including the one send() per read check from #42762.

Two options: fold the patch and the two response header cases in here, or I send them as a follow-up after this lands. Either is fine with me.

[claude]

I reviewed the latest push (the merge of main plus the dispatch-time read of sawConnectionClose) and found no bugs; since this is the HTTP/1.1 request parser on the network-facing path and a maintainer changes-requested review is still on the record, a human look is still worthwhile.

What was reviewed:

• The dispatch refactor at packages/bun-uws/src/HttpContext.h:494 is behavior-preserving: the latch is set at HttpParser.h:1212 before requestHandler runs for the same request, and the parser never dispatches again once it is set, so the flag is never read for a request other than the one that set it.
• hasConnectionClose() token walk: every iteration advances pos (past a comma or to end of value), tokenEnd > tokenStart guards the trailing-OWS trim, and the 5-byte strncasecmp is gated on token length, so it does not read past value.
• The gate now sits below the node:http CRLF skip, so the trailing-CRLF-after-POST case raised earlier is handled by the code and exercised by the two new node-http.test.ts cases; no getHeader("connection").length() == 5 callers remain in the tree.
• The node:http async-pipelined branch (startPipelinedResponseImpl) takes connectionClose from JS and is untouched by this change.

Extended reasoning...

Overview

The diff against the base touches two native files and two test files. packages/bun-uws/src/HttpParser.h adds HttpRequest::hasConnectionClose() (a comma-separated, OWS-trimmed, case-insensitive token scan over every Connection header, gated on the bloom filter), promotes the node-only nodeHttpSawConnectionClose latch to a public sawConnectionClose on HttpParser that is set for both server types right after getHeaders(), and adds a loop-top gate below the tunnel/park/CRLF-skip blocks that returns HPE_CLOSED_CONNECTION for node:http and silently consumes the remainder of the read for Bun.serve. packages/bun-uws/src/HttpContext.h replaces the getHeader("connection").length() == 5 heuristic at dispatch with a read of that latch (the maintainer's own last commit). Tests were added to test/js/bun/http/serve.test.ts (10 cases: close/Close/keep-alive, close/close, TE/HTTP 1.0, mid-read close, async handler, two keep-alive controls, malformed trailing bytes) and test/js/node/http/node-http.test.ts (trailing CRLF after a close-flagged POST or HTTP/1.0 request is not a clientError; other trailing bytes are HPE_CLOSED_CONNECTION).

Security risks

This is request-parsing code on network bytes, adjacent to request-smuggling concerns. I checked the new token scanner for out-of-bounds reads (every index is bounded by value.length(), the trailing-whitespace trim is guarded by tokenEnd > tokenStart, and the 5-byte compare only runs when the token is exactly 5 bytes) and for termination (each outer iteration either consumes a comma or reaches the end of the value). The Bun.serve discard path returns consumedTotal + length so the caller sees the whole read consumed and nothing spills into the fallback buffer; the close-flagged request's own body is consumed in the same iteration before the gate can fire on the next iteration, so body framing is unchanged. Widening the latch from node-only to both server types is a strict tightening (fewer requests dispatched), not a relaxation.

Level of scrutiny

High. The change alters which pipelined requests are dispatched and when a connection is closed, in the shared uWS parser used by both Bun.serve and node:http. The last push is by the maintainer who previously requested changes, and there is no independent non-author sign-off recorded, so I am not approving: a human should confirm the maintainer's own refactor lands the semantics they intended. The dispatch-time read of the latch is behavior-preserving as far as I can trace it: the latch is set at HttpParser.h:1212 before requestHandler(user, req) at line 1334 in the same iteration, and once set the gate at line 1181 prevents any later dispatch, so the flag cannot be observed on a different request than the one that set it. The HttpResponseData is placement-new'ed per socket in onOpen, so the latch has the connection lifetime the RFC requires.

Other factors

The earlier finding I posted about the gate firing on a stray CRLF after a Connection: close POST is addressed in the current code (the gate sits below the isNewline skip) and the two node-http test cases target exactly that input. The raw-socket helpers in the tests settle on either the server close or the expected response count, so a regressed server that answers /b and keeps the socket open still fails fast rather than hanging; closedByServer is captured before socket.destroy(). The remaining risk is whether CI on all platforms agrees with the debug-build results claimed in the description, which the automated review cannot verify here. Overall: no bugs found by the hunt (exit reason dry_streak), but network-parser scope plus an outstanding maintainer review state means defer rather than approve.