Bun.serve: deliver the in-flight response when a pipelined request arrives while it is still pending - #35036
Bun.serve: deliver the in-flight response when a pipelined request arrives while it is still pending#35036robobun wants to merge 10 commits into
Conversation
…rives while it is still pending uWS has one HttpResponseData per socket. When a pipelined request is parsed while the previous response is still pending (async fetch handler, or a Bun.file() body whose read goes through the libuv threadpool on Windows), the connection was hard-closed with the cork buffer discarded and zero bytes reached the client. Drop the pipelined request and mark the in-flight response for connection-close instead. RFC 9112 9.3.2: a pipelining client retries unanswered requests on a new connection. cork()'s early-return when the handler already uncorked (internalEnd) skipped the close-after-drain check; gate it on !isParsingHttp so an async handler completing outside onData still closes once marked.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughChangesThe HTTP server now preserves in-flight responses when dropping pipelined requests. Upgrade and cork handling retain socket ownership safely, with regression tests covering file responses, async handlers, and WebSocket handshakes. HTTP pipelining and WebSocket upgrade
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:56 AM PT - Jul 22nd, 2026
✅ @robobun, your commit 5609c00046d5d3fad722cc3c16107a5ae28f7861 passed in 🧪 To try this PR locally: bunx bun-pr 35036That installs a local version of the PR into your bun-35036 --bun |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…sParsingHttp The isParsingHttp flag lives on HttpContextData (one per server), so it answered 'is some socket inside onData' rather than 'is this socket'. A socket whose async response completes during another socket's onData (via drain_microtasks) would have its close check skipped and stay open until idle timeout. The socket onData corked reaches here via the already-corked else branch, so the guard only needs to exclude the upgrade case (c): compare the socket's group before/after handler() (in-place adopt changes the group; a reallocated adopt marks the old allocation closed). Also fold the file-route test's stderr/exitCode into a combined object assertion per review.
There was a problem hiding this comment.
The per-server isParsingHttp guard from the previous revision is now a per-socket group comparison in cork() — verified it captures httpGroup before handler() and skips the close check when the socket was adopted into the WebSocket group or closed, so the cross-socket delayed-close corner from the earlier review is resolved. The standalone expect(stderr).toBe("") was also folded into the combined-object assertion.
Extended reasoning...
Commit 2fabfd0 addressed both points from the previous review round: the cork() close-after-drain check now gates on us_socket_is_closed + us_socket_group == httpGroup (per-socket) instead of !isParsingHttp (per-server), and the bun-serve-file test now asserts { firstLine, body, stderr, exitCode } as one object. I re-read the new guard against the three enumerated INVALID_CORK_SLOT cases (large-write uncork, stolen empty slot, upgrade) and it correctly excludes (b) via HTTP_RESPONSE_PENDING and (c) via the group change / closed check. The remaining concern is the new inline finding on the async-WebSocket-upgrade path in internalEnd(), which is separate from the cork() guard.
… harden tests internalEnd(): gate the uncorked close check on !keepCorked. Only upgrade() passes keepCorked=true, and closing there would destruct HttpResponseData while upgrade() still reads and destructs it (confirmed double-free of HttpParser::fallback under ASAN when a partial third request is pipelined behind the handshake). upgrade() also resume()s the socket so the adopted WebSocket can read after the drop path paused it. Drop path: move the check above the per-request timeout clear so a dropped pipelined request cannot disarm the in-flight request's idleTimeout, and resetTimeout()+pause() so a pipelined flood cannot keep extending it across segments. The IsNodeHttp pending branch is left where it was; the hoisted !IsNodeHttp check makes its Bun.serve arm unreachable. Tests: add an ASAN regression test for the async-upgrade double-free; extend the serve.test.ts pipelined coverage to a POST body + third request; bound the close wait and set idleTimeout: 0 so a regression in the close-after- drain path is a named failure, not an idle-timeout pass; wire socket 'error' in the file-route fixture.
…s armed HttpResponse::pause() does Super::timeout(0) after Super::pause(), so the previous order overwrote the re-arm.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bun-uws/src/HttpContext.h`:
- Around line 395-403: Replace the __builtin_unreachable() in the !IsNodeHttp
branch of the HTTP_RESPONSE_PENDING/hasQueuedPipelinedResponses handling with an
assertion that explicitly validates the invariant. Preserve the surrounding
node:http queued-pipelining behavior while ensuring any future invariant
violation fails through a defined assertion rather than undefined behavior.
🪄 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: fb25c15f-a4df-44b4-b7d0-fd85cf3dedb1
📒 Files selected for processing (5)
packages/bun-uws/src/HttpContext.hpackages/bun-uws/src/HttpResponse.htest/js/bun/http/bun-serve-file.test.tstest/js/bun/http/serve.test.tstest/js/bun/websocket/websocket-server.test.ts
For !IsNodeHttp, HTTP_RESPONSE_PENDING is handled above and hasQueuedPipelinedResponses stays false, so gating the whole block on IsNodeHttp makes the else branch the only path without a runtime-unreachable arm.
…elined drop The HTTP_CONNECTION_CLOSE bit no longer only means 'the client already knows' now that the pipelined-drop path sets it on a keep-alive client. Drop the (state & HTTP_CONNECTION_CLOSE) == 0 term from internalEnd()'s header guard (END_CALLED is the write-once guard) so the final response tells the client it is final (RFC 9112 9.6). Assert the header in the pipelined-GETs test.
…rs behind a dropped request
Dropping the (state & HTTP_CONNECTION_CLOSE) == 0 header guard regressed
node:http (writeAutoHeaders() writes Connection: close via raw write, so
internalEnd() wrote a second one) and missed the copy-pasted guard in
uws_res_end_without_body(). Use a distinct HTTP_PIPELINED_DROP bit that
shouldCloseConnection() reads; the header-write guards keep checking
HTTP_CONNECTION_CLOSE ('client already knows / already written') so the
header is emitted after a drop and not duplicated on node:http.
Returning s from the drop path lets the parser keep validating the dropped
request's bytes. A parse error there wrote a 4xx via us_socket_write() and
closed, which the client reads as the first (valid) request's answer and
aborts the in-flight response. Skip the 4xx write/close when
HTTP_PIPELINED_DROP is set; the connection is already paused and marked for
close-after-drain.
There was a problem hiding this comment.
No further findings after the HTTP_PIPELINED_DROP rework in 0484107, but this reworks the core Bun.serve request-dispatch and close-after-drain path in vendored uWS (cork early-return, internalEnd() keepCorked gate, upgrade() resume, parse-error suppression), so it's worth a human look.
What was reviewed:
HTTP_PIPELINED_DROPas a separate bit:internalEnd()'sHTTP_CONNECTION_CLOSEguard is restored, so node:http'swriteAutoHeaders()no longer double-writesConnection: close;uws_res_end_without_body()now seesshould_close_connection()==truevia the Rust-sideis_http_connection_close()change.- Parse-error path after a drop: unrefs, uncorks, and returns without the 4xx write/close — the in-flight response's cork buffer survives; the
!IsNodeHttpgate keeps node:http'sclientErrorpath unchanged. cork()early-return close check: gated on!is_closed && group unchanged, so a reallocated/in-place-adopted WebSocket ext block is not read asHttpResponseData.resetResponseState()clearsHTTP_PIPELINED_DROP(not inHTTP_CONNECTION_SCOPED), so a sync-pipelined request after a completed response starts clean.
Extended reasoning...
Overview
The PR changes how Bun.serve handles a pipelined HTTP/1.1 request that arrives while the previous async response is still pending. Previously the socket was hard-closed with the cork buffer discarded (zero bytes written); now the pipelined request is dropped, the connection is marked for close-after-drain via a new HTTP_PIPELINED_DROP state bit, reads are paused, and the in-flight response completes before the socket closes. Follow-through changes: cork() runs the close-after-drain check on its early-return path (gated on the socket still being in the HTTP group), internalEnd()'s uncorked close check is gated on !keepCorked so upgrade() doesn't destruct the ext block mid-adopt, upgrade() resumes reads, and the parse-error path skips the 4xx write when a drop already happened. The Rust State bitflags mirror gained the new bit and is_http_connection_close() reads it. Three new tests cover async-handler pipelining, Bun.file route pipelining (the Windows repro), and async server.upgrade() behind a pipelined handshake (the ASAN double-free the keepCorked gate prevents).
Security risks
No new attack surface. The change is strictly more defensive than the previous behavior (which hard-closed). The pause() + resetTimeout() on drop bounds a pipelined-flood client the same way a lone async request already is; the parse-error suppression only fires when HTTP_PIPELINED_DROP is already set, and that path still marks the connection for close. isConnectRequest = false on drop prevents a dropped CONNECT from switching the in-flight response into tunnel handling.
Level of scrutiny
High — this is the vendored uWebSockets HTTP dispatch/response path that every Bun.serve request goes through, and the changes interact with corking, socket adoption (WebSocket upgrade), the node:http compat instantiation, and parse-error handling. The PR went through five rounds of review feedback, each of which surfaced a real cross-path interaction (dead resetTimeout(), Connection: close header suppression, node:http duplicate-header regression, 4xx-overwrite on trailing parse error). All are addressed and the current diff looks correct to me, but the density of subtle interactions here warrants a maintainer's eyes.
Other factors
All prior inline comments are resolved with fix commits. Test coverage is good: the three new tests exercise the drop path, the Connection: close header on the delivered response, the parse-error-after-drop case, and the upgrade-after-drop ASAN case; the PR description lists the broader suites that were re-run. HTTP_PIPELINED_DROP is not in HTTP_CONNECTION_SCOPED, so resetResponseState() clears it — a sync-pipelined next request on the same socket won't inherit it (though in practice the drop path always leads to close). The IsNodeHttp path is unchanged.
There was a problem hiding this comment.
No issues found in the current revision — all earlier review threads (dead resetTimeout(), vacuous if constexpr, Connection: close header emission, node:http duplicate-header fallout, and the trailing-parse-error 4xx overwrite) are addressed via the separate HTTP_PIPELINED_DROP bit and the gated error path. That said, this reworks Bun.serve's per-socket connection lifecycle across onData, cork(), internalEnd(), and upgrade(), with memory-safety-adjacent interactions (ext-block destruction on adopt, close-after-drain vs. WebSocket group move), so it warrants a human look before landing.
What was reviewed:
- Verified
HTTP_PIPELINED_DROPis read by both C++shouldCloseConnection()and Rustis_http_connection_close(), and thatinternalEnd()'s header-write guard is restored so node:http'swriteAutoHeaders()de-dupe still holds. - Checked the new
cork()early-return close path is gated on!is_closed && group unchangedso an in-place or reallocated WebSocket adopt doesn't reinterpret the ext block. - Checked
resetResponseState()clearsHTTP_PIPELINED_DROP(not inHTTP_CONNECTION_SCOPED) — fine since the drop path never dispatches a next request on this socket.
Extended reasoning...
Overview
The PR changes how Bun.serve handles a pipelined HTTP/1.1 request that arrives while the previous response is still pending. Previously the socket was hard-closed (dropping the in-flight response's corked bytes); now the pipelined request is dropped, the connection is marked for close-after-drain via a new HTTP_PIPELINED_DROP state bit, reads are paused, and the in-flight response completes normally before the socket closes. Touches HttpContext.h (per-request callback + parse-error path), HttpResponse.h (internalEnd !keepCorked gate, cork() early-return close check, upgrade() resume), HttpResponseData.h (new bit + shouldCloseConnection()), and src/uws_sys/Response.rs (mirror bit). Three new tests cover async-fetch pipelining, Bun.file route pipelining (Windows repro), and async server.upgrade() behind a pipelined request (ASAN double-free guard).
Security risks
The change sits on a DoS-protection path (async pipelining rejection). The new behavior pauses reads and re-arms idleTimeout, so a pipelined flood cannot hold the socket open indefinitely — same exposure as a lone async request. The parse-error path now suppresses the 4xx write when HTTP_PIPELINED_DROP is set; that's scoped to !IsNodeHttp and only after a request was already dropped, so it doesn't open a smuggling vector (the connection is already marked for close and reads are paused). No auth/crypto surface.
Level of scrutiny
High. This is core Bun.serve connection-lifecycle C++ with cross-cutting effects on cork()/uncork(), WebSocket upgrade() (which destructs the ext block in place), the node:http instantiation, and the Rust render path via should_close_connection(). The PR went through five rounds of review, each surfacing a non-obvious interaction (dead resetTimeout store, node:http Connection: close duplication, 4xx overwriting the in-flight response), which is exactly the profile where a maintainer familiar with uWS's cork-slot and adopt semantics should sign off.
Other factors
Test coverage for the new paths is thorough (three targeted tests plus a Connection: close header assertion and a malformed-trailing-bytes case). Two alternative PRs (#32868, #33664) are called out in the description with different design trade-offs (buffer-and-serve vs. drop-and-close); a maintainer should confirm this minimal-degradation approach is the one to land. uws_res_end_without_body in libuwsockets.cpp keeps its HTTP_CONNECTION_CLOSE guard unchanged, so bodyless responses after a drop now correctly emit Connection: close via is_http_connection_close() reading the new bit.
|
CI on builds 77518 and 77599: every test failure is tagged My new tests ( |
…d it HttpContext::onData pauses the socket while request bytes are parked behind a pending response. If that response turns out to be a WebSocket upgrade, upgrade() destructs the HttpResponseData (and the parked bytes with it, as for bytes trailing a synchronous upgrade) but us_socket_adopt carries the paused flag over, so the WebSocket sent its 101 and then never read a frame. Drop the parked bytes and resume before internalEnd(), so markDone() does not arm a replay dispatch for them either. Tests folded in from the earlier pipelining PRs (#33664, #35036, #32868) for the cases bun-serve-pipelining.test.ts did not cover yet: a Bun.file() body from the handler and a Bun.file() route over each transport, a held request that carries a body with a Connection: close request behind it, held bytes that are a parse error, and the upgrade case above.
|
Closing in favour of #38128, which is current with main (this branch conflicts) and goes further for the same close in This PR's tests were run against #38128: the in-flight response is delivered in every case as they assert; the assertions that differ are the ones pinning the close and the undispatched second request, which #38128 answers instead. Two findings from here were carried over into |
…d it HttpContext::onData pauses the socket while request bytes are parked behind a pending response. If that response turns out to be a WebSocket upgrade, upgrade() destructs the HttpResponseData (and the parked bytes with it, as for bytes trailing a synchronous upgrade) but us_socket_adopt carries the paused flag over, so the WebSocket sent its 101 and then never read a frame. Drop the parked bytes and resume before internalEnd(), so markDone() does not arm a replay dispatch for them either. Tests folded in from the earlier pipelining PRs (#33664, #35036, #32868) for the cases bun-serve-pipelining.test.ts did not cover yet: a Bun.file() body from the handler and a Bun.file() route over each transport, a held request that carries a body with a Connection: close request behind it, held bytes that are a parse error, and the upgrade case above.
…d it HttpContext::onData pauses the socket while request bytes are parked behind a pending response. If that response turns out to be a WebSocket upgrade, upgrade() destructs the HttpResponseData (and the parked bytes with it, as for bytes trailing a synchronous upgrade) but us_socket_adopt carries the paused flag over, so the WebSocket sent its 101 and then never read a frame. Drop the parked bytes and resume before internalEnd(), so markDone() does not arm a replay dispatch for them either. Tests folded in from the earlier pipelining PRs (#33664, #35036, #32868) for the cases bun-serve-pipelining.test.ts did not cover yet: a Bun.file() body from the handler and a Bun.file() route over each transport, a held request that carries a body with a Connection: close request behind it, held bytes that are a parse error, and the upgrade case above.
…d it HttpContext::onData pauses the socket while request bytes are parked behind a pending response. If that response turns out to be a WebSocket upgrade, upgrade() destructs the HttpResponseData (and the parked bytes with it, as for bytes trailing a synchronous upgrade) but us_socket_adopt carries the paused flag over, so the WebSocket sent its 101 and then never read a frame. Drop the parked bytes and resume before internalEnd(), so markDone() does not arm a replay dispatch for them either. Tests folded in from the earlier pipelining PRs (#33664, #35036, #32868) for the cases bun-serve-pipelining.test.ts did not cover yet: a Bun.file() body from the handler and a Bun.file() route over each transport, a held request that carries a body with a Connection: close request behind it, held bytes that are a parse error, and the upgrade case above.
2 participants
Repro
On Windows, two pipelined HTTP/1.1 requests to a
Bun.file()route in one TCP segment close the connection with zero bytes written. On any platform, the same happens with an asyncfetchhandler:The Windows
Bun.file()route case (routes: { "/": new Response(Bun.file(p)) }) is the same bug: on POSIXFileResponseStreamreads a regular file synchronously inside the request handler, so the response completes before the parser moves on; on Windows the read goes to the libuv threadpool and completes on a later loop tick, so the second pipelined request is parsed whileHTTP_RESPONSE_PENDINGis still set.Cause
uWS has one
HttpResponseDataper socket. When a pipelined request is parsed while the previous response is still pending, the!IsNodeHttpbranch inHttpContext<SSL>::onData's per-request callback did:The socket is still corked at that point, so the first response's bytes never reach the wire.
Fix
HttpContext.h: mark the in-flight response for connection-close, re-arm its idleTimeout, and pause reads instead of closing. The pipelined request is dropped without dispatch; when the first handler eventually ends the response, the close-after-drain path shuts the socket down. RFC 9112 9.3.2: a pipelining client must be prepared to retry unanswered requests on a new connection. The check is hoisted above the per-requestus_socket_timeout(s, 0)so a dropped request cannot disarm the idle timeout, andpause()stops further segments so a pipelined flood cannot keep extending it. Synchronous pipelining (handler responds before returning) is unchanged.HttpResponse.h: two follow-throughs so the close mark is acted on and cannot corrupt an upgrade.cork()early-returned without running its close-after-drain check when the handler had already uncorked viainternalEnd(); an async handler completing outsideonDatawith the close mark would leave the socket open until idle timeout. The check now runs on that early-return when the socket is still in the HTTP group (excludes the WebSocket-upgrade case, per-socket).internalEnd()'s uncorked close check is gated on!keepCorked. Onlyupgrade()passeskeepCorked=true; closing there would destructHttpResponseDatamid-upgrade()and double-freeHttpParser::fallback(confirmed under ASAN).upgrade()alsoresume()s the socket so the adopted WebSocket can read after the drop path paused it.The
IsNodeHttp(node:http) branch already queues pipelined responses and is unchanged.#32868 takes the same approach against the pre-
IsNodeHttpHttpContext.hand conflicts with current main. #33664 goes further and buffers pipelined bytes to serve every request in order (full async pipelining for Bun.serve); it also conflicts with current main. This PR applies the minimal RFC-9112-conforming degradation to the current structure.Verification
New tests:
test/js/bun/http/serve.test.ts: pipelined GETs behind an async fetch handler (first response delivered, second handler not called, socket closes), and pipelined POST-with-body plus a third request (all dropped past the first).idleTimeout: 0plus a bounded close-wait so a regression in the close-after-drain path is a named failure, not an idle-timeout pass.test/js/bun/http/bun-serve-file.test.ts: pipelined requests to aBun.file()route. Before (Windows):firstLine === "". After: at least the first response reaches the wire; POSIX still serves both (sync file read).test/js/bun/websocket/websocket-server.test.ts: WS handshake + pipelined GET + partial third request, asyncserver.upgrade(). Before (with only theHttpContext.hchange): ASAN double-free ofHttpParser::fallback. After: upgrade succeeds and the server keeps answering.Also ran:
serve.test.ts(full),bun-serve-file.test.ts(full, Linux + Windows),request-smuggling.test.ts,http-server-chunking.test.ts,bun-serve-headers.test.ts(Connection: closecases),bun-serve-static.test.ts,hspec.test.ts,websocket-server.test.ts,node-http.test.ts,test-http-keep-alive*.js,test-http-pipeline*.js,test-http-upgrade-server.js. No new failures.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/bun-serve-file.test.ts test/js/bun/http/serve.test.ts test/js/bun/websocket/websocket-server.test.ts