Bun.serve: keep the connection-close mark across a pipelined request - #38343
Bun.serve: keep the connection-close mark across a pipelined request#38343robobun wants to merge 1 commit into
Conversation
HttpResponseData::resetResponseState() runs for every request dispatched on a connection and cleared HTTP_CONNECTION_CLOSE along with the per-response framing bits. A well-formed HTTP/1.1 request pipelined behind an HTTP/1.0 request, a Connection: close request, or a response that carried Connection: close therefore turned the connection persistent again and the close recorded for the earlier request never happened. Until bdb7382 the HTTP/1.0 case was masked by the parser latching its ancient flag for the rest of the recv buffer, which re-marked the connection on the pipelined request; that flag is per-request now. The mark describes the connection, not the response in flight, so add it to HTTP_CONNECTION_SCOPED, the set of bits resetResponseState() preserves. The pipelined request is still answered and the existing shouldCloseConnection() gates close the socket once that response has flushed.
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 1:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 790a5bc has some failures in 🧪 To try this PR locally: bunx bun-pr 38343That installs a local version of the PR into your bun-38343 --bun |
|
Status: reproduced on current main and fixed here; ready for a maintainer. Reproduced with the raw-socket repro in the description (HTTP/1.0 request + pipelined HTTP/1.1 request in one segment): on main ( CI (build 95379): 177 of 179 jobs passed, including every other darwin lane; the remaining 2 are |
There was a problem hiding this comment.
LGTM — one-bit addition to HTTP_CONNECTION_SCOPED so resetResponseState() no longer wipes a pending close mark when a pipelined request is dispatched.
What was reviewed:
- Grepped every reader/writer of
HTTP_CONNECTION_CLOSEacrosspackages/bun-uws/andsrc/: all setters are|=-only, nothing intentionally clears it, and fresh connections zero-initstate, so making it sticky is safe. - Checked both
resetResponseState()callers (HttpContext.hrequest lambda,JSNodeHTTPServerSocket::startPipelinedResponse) — the latter's manualconnectionClosere-apply is now redundant but harmless. - Verified all
shouldCloseConnection()gates still guard onHTTP_RESPONSE_PENDINGcleared +hasFullyDrained(), so the pipelined response is still delivered before FIN. - Test: event-driven (no sleeps),
port: 0,usingcleanup, wireserror/end/close, includes a keep-alive control case;netalready imported at serve.test.ts:27.
Extended reasoning...
Overview
Single-line native change in packages/bun-uws/src/HttpResponseData.h: adds HTTP_CONNECTION_CLOSE to the HTTP_CONNECTION_SCOPED mask, plus a clarifying comment on the enum member. resetResponseState() masks the state word down to HTTP_CONNECTION_SCOPED at the start of every request dispatched on a connection, so before this change a pipelined HTTP/1.1 request in the same recv buffer would erase the close mark set by an HTTP/1.0 or Connection: close request ahead of it. A new 6-case describe block in test/js/bun/http/serve.test.ts covers the five non-persistent first-request shapes plus a keep-alive control.
Security risks
None. This tightens behaviour toward RFC 9112 §9.3/9.6 (a connection marked non-persistent stays non-persistent) and restores released-build behaviour for the HTTP/1.0 shapes regressed by #35864. No new input parsing, no allocation, no untrusted data handling — one bit added to a compile-time mask.
Level of scrutiny
Medium: the flag word is shared connection-lifetime state on a hot path (Bun.serve and node:http compat). I verified the PR's central claim by grepping every use of HTTP_CONNECTION_CLOSE: HttpContext.h:440/442, HttpResponse.h:156/584, libuwsockets.cpp:1414/1440, NodeHTTP.cpp:838, and JSNodeHTTPServerSocket.cpp:254/541 all only |= the bit; nothing ever &= ~ it. The only place that could clear it was resetResponseState() itself, which is exactly the bug. The one read that gates a side effect on the bit being unset (HttpResponse.h:152, deciding whether to emit a Connection: close header) is inside if (closeConnection) and only skips a redundant header write — the connection still closes via the existing shouldCloseConnection() gates, all of which check HTTP_RESPONSE_PENDING == 0 && hasFullyDrained() before shutting down, so the pipelined response is still delivered.
Other factors
The node:http pipelining path (startPipelinedResponseImpl) already worked around this by re-applying a captured connectionClose after resetResponseState(); that re-apply is now a no-op OR of an already-set bit, which is harmless. The Rust-side readers (NodeHTTPResponse.rs is_http_connection_close()) feed the bit back into end(..., closeConnection), which composes correctly. The test is well-constructed per repo guidelines: no sleeps (races "server closed" vs "probe answered" via socket events), port: 0, using server, wires error to a no-op so the following close still resolves, destroys the socket after settling, and includes a positive control. The PR description documents that all five failing cases fail on current main and pass with the fix, and that the surrounding HTTP suites (serve, request-smuggling, node-http, 55 vendored Node keep-alive/pipeline tests) still pass.
Problem
Bun.serveleaves a connection open that it had already marked for close when a well-formed HTTP/1.1 request is pipelined behind the request that marked it, in the same TCP segment. Affected first requests:GET / HTTP/1.0(with or without aConnectionheader), an HTTP/1.1 request withConnection: close, and an HTTP/1.1 request whoseResponsecarriesConnection: close. A request sent on that connection later is still served.HttpParser.hused to latchisAncientHTTPfor the rest of the recv buffer, so the pipelined HTTP/1.1 request was itself treated as HTTP/1.0 and re-marked the connection for close. That PR made the flag per-request (needed for its Transfer-Encoding check), which exposed the bug below. The twoConnection: closeshapes were already broken the same way in released builds (checked on 1.3.14, where the three HTTP/1.0 shapes still close).HttpResponseData::resetResponseState()(packages/bun-uws/src/HttpResponseData.h) runs for every request dispatched on a connection (HttpContext.h, the request handler lambda) and clears the whole state word exceptHTTP_CONNECTION_SCOPED.HTTP_CONNECTION_CLOSEwas not in that set, so dispatching the pipelined request wiped the close mark set by the previous request, and the post-parseshouldCloseConnection()gate inHttpContext::onDatafound nothing to act on.Fix
HTTP_CONNECTION_CLOSEtoHTTP_CONNECTION_SCOPED, soresetResponseState()keeps it.Connection: closeinHttpContext.h;end(..., closeConnection)and close-delimited bodies inHttpResponse.h; aConnection: closeresponse header inNodeHTTP.cpp; node:http's deferredsocket.end()inJSNodeHTTPServerSocket.cpp), and nothing ever clears it on purpose: a connection that has been marked can not legitimately become persistent again. A fresh connection starts with a zeroed state word, so nothing leaks between connections.shouldCloseConnection()gates (onDatatail,internalEnd,onWritable) close the socket once that response has drained. Whether the pipelined request should be dispatched at all (RFC 9112 9.6) is a separate question, tracked in Bun.serve: stop dispatching pipelined requests after Connection: close (RFC 9112 9.6) #33005 forBun.serveand node:http: stop parsing after a deferred socket.end() so a pipelined request can't lose the shutdown #35054 for node:http'ssocket.end(); both compose with this change.test/js/bun/http/serve.test.ts, describea request pipelined behind a non-persistent request does not keep the connection alive. Five first-request shapes (HTTP/1.0, HTTP/1.0 +Connection: keep-alive, HTTP/1.0 +Connection: close, HTTP/1.1 +Connection: close, HTTP/1.1 answered withConnection: close) each followed by a pipelined HTTP/1.1 request; the client sends a probe request once it has the pipelined response and the test settles on whichever comes first, the server closing or the probe being answered. All five fail on current main withprobe answeredand pass with the fix; on 1.3.14 the three HTTP/1.0 shapes pass and the twoConnection: closeshapes fail; the keep-alive control (probe answered) passes everywhere.--rerun-each(150/150),serve.test.ts(288 pass; the 4 failures,requestIP v6, privileged port,#6583,/bun:infoloopback, fail identically on an unmodified build in this environment),request-smuggling.test.ts(87 pass),bun-server.test.ts(3 failures, same environment-only set as on an unmodified build),bun-serve-headers/-file/-routes/-static,http-server-chunking,proxy-stress-protocol,serve-directory-routes,serve-direct-readable-streamand 8 HTTP regression files (396 pass),node-http.test.ts(1 failure,request via http proxy, environment-only), 6 node:http connection-handling files (52 pass), and 55 vendored Node http keep-alive / pipeline / close / client-error tests (all pass).Background
HttpResponseDataper socket and reuses it for every request on a keep-alive connection. Itsstateword mixes per-response bits (status written, Content-Length written, response pending, ...) with per-connection bits.resetResponseState()starts a new response by clearing the word down toHTTP_CONNECTION_SCOPED, the bits that describe the connection (parsing stopped, reads paused, peer FIN received, close when idle); that set is what this PR extends.fenceAndConsumePostPadded(HttpParser.h) loops over every request present in one recv buffer and dispatches each one through theHttpContext.hlambda.Bun.servesupports this only when the previous response completed synchronously (otherwise it closes the connection), which is why the test's handler returns aResponsedirectly.shouldCloseConnection()is consulted after the parse loop (HttpContext::onData), when a response ends (internalEnd) and when buffered output drains (onWritable), and each only acts onceHTTP_RESPONSE_PENDINGis clear and the socket has fully drained, so the response to the pipelined request is delivered before the FIN.Connection: keep-alive;Bun.servenever does, so it always marks such connections for close (HttpContext.h,isAncient()), independent of what the request'sConnectionheader says.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/bun/http/serve.test.ts