http: deliver the in-flight response when a pipelined request arrives behind an async handler by robobun · Pull Request #32868 · oven-sh/bun

robobun · 2026-06-27T17:53:40Z

What

A raw TCP client pipelines two HTTP/1.1 requests back to back:

GET /a HTTP/1.1\r\nHost: x\r\n\r\nGET /b HTTP/1.1\r\nHost: x\r\n\r\n

The handler for /a responds asynchronously:

http.createServer((req, res) => {
  if (req.url === "/a")
    setTimeout(() => { res.writeHead(200); res.write("A1"); setTimeout(() => res.end("A2"), 20); }, 20);
  else
    res.end("B");
});

Before: the connection is destroyed with zero bytes written. Node.js serves both responses in order.

After: /a's full response is delivered and the socket closes; the client retries /b on a new connection.

Cause

HttpContext<SSL>::onData's per-request callback checks HTTP_RESPONSE_PENDING:

if (httpResponseData->state & HttpResponseData<SSL>::HTTP_RESPONSE_PENDING) {
    us_socket_close((us_socket_t *) s, 0, nullptr);
    return nullptr;
}

uWS has one HttpResponseData per socket, so when /b is parsed while /a's handler is still pending the flag is set and the socket is closed outright. The socket is still corked at that point, so nothing reaches the wire.

Fix

Set HTTP_CONNECTION_CLOSE and return the live socket instead of closing. The parser discards the pipelined request's body (its inStream callback was already cleared after the first request's body fin), and when the first handler eventually calls res.end() the normal close-after-drain path in internalEnd/onWritable shuts the socket down. The offset = 0 reset moves below the check so it no longer clobbers the in-flight response's write offset.

This is not full async pipelining support; the dropped request is not queued. Per RFC 9112 9.3.2 a pipelining client must be prepared to retry unanswered requests when the server closes, so delivering the first response and closing is a conforming degradation. Synchronous pipelining (handler responds before returning, so markDone() clears HTTP_RESPONSE_PENDING before the next request is parsed) is unchanged.

Both node:http and Bun.serve share HttpContext.h and both benefit.

Verification

New tests in test/js/node/http/node-http.test.ts (same-segment and separate-packet pipelined request behind an async handler) and test/js/bun/http/serve.test.ts (Bun.serve variant). On the unfixed build:

error: expect(received).toStartWith(expected)
Expected to start with: "HTTP/1.1 200 OK\r\n"
Received: ""

Also ran: node-http.test.ts (full), serve.test.ts (full), hspec.test.ts, http-server-chunking.test.ts, request-smuggling.test.ts, and the vendored test-http-get-pipeline-problem.js / test-http-keep-alive-pipeline-max-requests.js / test-http-keep-alive-drop-requests.js / test-http-1.0-keep-alive.js. No new failures.

… behind an async handler When a client sends two pipelined HTTP/1.1 requests in one TCP segment and the handler for the first responds asynchronously, the second request reaches uWS's per-request callback while HTTP_RESPONSE_PENDING is still set. uWS has a single HttpResponseData per socket, so it cannot dispatch the second request; previously it called us_socket_close(), which tore the connection down before any bytes of the first response reached the wire. Instead, mark the in-flight response for connection-close and let the parser discard the pipelined request. The first response is delivered in full and the socket closes once it is drained; the client retries the dropped request on a new connection (RFC 9112 9.3.2). The write offset reset is moved after the check so it no longer clobbers the in-flight response's offset. Affects both node:http and Bun.serve (they share HttpContext.h). Synchronous pipelining (handler responds before returning) is unchanged: markDone() clears HTTP_RESPONSE_PENDING before the next request is parsed.

coderabbitai · 2026-06-27T17:53:47Z

Walkthrough

HttpContext::onData is changed to handle pipelined requests arriving while a response is pending by setting HTTP_CONNECTION_CLOSE and dropping the new request without force-closing the socket. Two regression tests are added verifying the first async response is still delivered, and a failing expectation entry is added for a related Node parallel test.

HTTP Pipelining Fix

Layer / File(s)	Summary
`onData` pipelining logic `packages/bun-uws/src/HttpContext.h`	Replaces force-close (`nullptr` return) when `HTTP_RESPONSE_PENDING` is set with `HTTP_CONNECTION_CLOSE` marking and continued socket draining; reorders per-request timeout/offset reset after the not-ready guard.
Regression tests and expectations `test/js/bun/http/serve.test.ts`, `test/js/node/http/node-http.test.ts`, `test/expectations.txt`	Adds Bun and Node http pipelining tests asserting first async response is delivered and pipelined second request is dropped; adds a `[ FAIL ]` expectation for the upstream Node parallel pipelining test pending queued-response support.

Suggested reviewers

Jarred-Sumner
cirospaciari

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: preserving the in-flight response for pipelined requests behind an async handler.
Description check	✅ Passed	The description covers the change and verification steps, though it uses custom section headings instead of the template wording.
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.

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

robobun · 2026-06-27T17:53:50Z

^{Updated 5:11 PM PT - Jun 27th, 2026}

❌ @robobun, your commit 6f6b9aa has 2 failures in Build #65651 (All Failures):

test/js/sql/tls-sql.test.ts - code 1 on 🍎 14 aarch64
test/js/bun/test/parallel/test-docker-build-debian.ts - code 1 on 🍎 14 aarch64

🧪 To try this PR locally:

bunx bun-pr 32868

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

bun-32868 --bun

github-actions · 2026-06-27T17:57:21Z

Found 1 issue this PR may fix:

node:http keep-alive server drops the next reused request after a Content-Length response finalized by a deferred end() (graceful FIN, no Connection: close) #31889 - Describes the same root cause: HTTP_RESPONSE_PENDING guard in HttpContext.h calls us_socket_close() when a reused/pipelined request arrives behind a deferred end(), destroying the connection before the in-flight response is delivered

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

Fixes #31889

🤖 Generated with Claude Code

github-actions · 2026-06-27T18:01:25Z

This PR may be a duplicate of:

node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed #32488 - Both PRs modify the same HTTP_RESPONSE_PENDING pipelining check in HttpContext.h that previously called us_socket_close() when a pipelined request arrived behind an async handler; node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed #32488 implements full async pipelining with request queuing for node:http compat and will conflict on the same lines

🤖 Generated with Claude Code

robobun · 2026-06-27T18:03:50Z

On the two bot suggestions above:

#32488: that PR implements full HTTP/1.1 pipelining for node:http (queued responses, ordered flush) and touches the same HTTP_RESPONSE_PENDING branch in HttpContext.h, so the two conflict there. #32488 is the complete fix for the node:http path; this PR is the minimal change that takes the existing hard close (zero bytes delivered) to "first response delivered, then close" without adding a queue. #32488 gates its pipelining support on usingNodeHttpCompat, so Bun.serve keeps the hard close there; this change applies to both paths. Happy to close this in favour of #32488 if that is landing soon, or to keep it as an interim until it does.

#31889: not fixed by this PR. In that scenario the first response's body is already fully written when the reused request arrives; this change still drops the reused request and closes once the deferred end() runs, so the client still sees sent > got. Verified with the repro from the issue against this branch: sent=20 got=10 handled=10. #32488's queued pipelining would fix #31889.

The client calling socket.end() once the first response arrives means the server always sends FIN in reply (uWS onEnd closes on half-close), so the serverClosed check could not distinguish a proactive server close from an echoed client FIN. Keep the substantive assertions: the first response reached the wire and the pipelined handler was never dispatched.

Addresses three review findings: When the in-flight response completes under backpressure, markDone() clears HTTP_RESPONSE_PENDING before the socket is closed; a third pipelined request arriving in that window would pass the PENDING check and be dispatched (and the assignment at state = HTTP_RESPONSE_PENDING would wipe HTTP_CONNECTION_CLOSE), misattributing its response to the request that was dropped. Extend the early return to also cover HTTP_CONNECTION_CLOSE so no further request is dispatched once one has been dropped on the socket. Move the per-request us_socket_timeout(s, 0) below the early return so a dropped request does not disarm the in-flight response's idle timer. Drop the separate-packet test: its ordering depends on event-loop phase timing rather than an explicit barrier, so it cannot observe whether the fix path was actually taken. The same-segment test (and the Bun.serve variant with expect(calls).toBe(1)) deterministically guard the HTTP_RESPONSE_PENDING branch. Mark test-http-pipeline-socket-parser-typeerror.js as expected-fail: it requires full async pipelining (second handler dispatched while the first is pending). It previously passed only because the pipelined request triggered a hard socket close and the process exited before the test logic ran.

getHeaders writes isConnectRequest = true through the bool& before requestHandler runs, so a dropped pipelined CONNECT would leave the per-socket flag set and route later bytes on the socket through the CONNECT-tunnel path. Clear it in the early-return so the in-flight response's handling is unaffected.

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 `@test/js/bun/http/serve.test.ts`:
- Around line 2401-2416: The test around the net.connect flow in serve.test.ts
is too weak because socket.end() lets the close event come from the client FIN
instead of proving the server drained and closed the connection. Update the
assertion logic in this test to wait for and verify the server’s own Connection:
close / socket shutdown behavior after the FIRST response, using the existing
wire capture and the net socket event handlers so the test fails if the server
keeps the connection reusable.

🪄 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: 51d8530f-3eb4-4d5f-ae23-979bb19e045f

📥 Commits

Reviewing files that changed from the base of the PR and between df92f8f and 6f6b9aa.

📒 Files selected for processing (4)

packages/bun-uws/src/HttpContext.h
test/expectations.txt
test/js/bun/http/serve.test.ts
test/js/node/http/node-http.test.ts

robobun · 2026-06-27T20:26:14Z

CI status on 6f6b9aa (build #65651): the only red is unrelated to this change.

darwin-26-aarch64-test-bun: buildkite-agent artifact download timed out after 120s in runner.node.mjs:2182; zero tests ran. Same failure occurred on build #65603 for this agent.
bun-plugin-svelte (GitHub Action): bundler __commonJS output-shape assertion; no overlap with packages/bun-uws.
Flaky-annotated Windows retries (Prisma postinstall ECONNRESET, update_interactive_install, bun-security-scanner-workspaces EBADF, bake/dev-and-prod): all package-manager / dev-server tests, passed on retry.

The test-http-pipeline-socket-parser-typeerror.js timeout from build #65622 is gone (expectations.txt entry in 155782e). No [error] annotations. The new tests in node-http.test.ts and serve.test.ts pass on every lane that ran them. All review threads resolved.

Not pushing another retrigger; ready for review when convenient, or close in favour of #32488 if that is landing soon.

robobun · 2026-06-27T20:36:42Z

Pushed an alternative approach on claude/farm/3142c8df/http-async-pipelining that buffers the pipelined request and re-dispatches it from markDone() once the in-flight response finishes, so both responses are served in order (matching Node.js) instead of closing after the first.

Summary of the difference:

This PR: first response delivered, Connection: close, pipelined request dropped (client retries).
The branch above: pipelined request(s) stashed in a per-socket buffer, re-fed through onData after markDone(); remainingStreamingBytes/isConnectRequest reset so the re-parse starts at a request boundary; 64 KiB cap on the buffer.

Three tests in test/js/node/http/node-http.test.ts cover: two requests in one write, a three-deep async chain, and a pipelined POST with a body. All fail on current main (empty wire) and pass on the branch.

Happy to fold this into this PR or open a separate one, whichever is preferred.

robobun · 2026-06-27T21:56:44Z

Thanks, I read through claude/farm/3142c8df/http-async-pipelining (4417145). It is a real improvement over what this PR does, and the hard parts are handled: the HTTP_PIPELINE_DEFERRED sentinel stops the parse loop so the dropped request's body never reaches the chunk validator (the misattribution case raised in review here), remainingStreamingBytes/isConnectRequest are reset, later recvs append in wire order, and the 64 KiB cap bounds the buffer. Keeping this PR minimal rather than folding that in, for three reasons:

It does not fix test-http-pipeline-socket-parser-typeerror.js either. That test's handler stores first = res on request 1 and returns without responding; first.end('hello') is only reached from the 'upgrade' listener, which requires requests 2..N to have already been dispatched. A deferred buffer that re-feeds from markDone() can never fire because request 1's markDone() is waiting on request 2 being dispatched, so it deadlocks the same way this PR does. That test needs true concurrent dispatch (handlers run while earlier responses are pending, responses flushed in order), which is what node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed #32488 implements. So the expectations.txt entry is needed either way.
It is ungated, so it changes Bun.serve's dispatch model too. node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed #32488 gates its pipelining on usingNodeHttpCompat specifically so Bun.serve keeps the existing async-pipeline-denied behaviour and its fast paths. Changing that default inside a bugfix PR needs a maintainer's call, not mine.
The markDone() -> onData() -> user handler -> end() -> markDone() re-entrancy (with the us_socket_is_closed probes in internalEnd to detect a destructed httpResponseData) is exactly the class of problem pipelining support has to solve, and node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed #32488's queued-response design solves it once, reviewed by the people who own this code.

So the shape I'd suggest: this PR stays the minimal "do not lose the in-flight response" fix (small, reviewed, all threads resolved), and the buffered-re-dispatch work is a good intermediate if a maintainer wants it before #32488, but as its own PR so it gets its own review. If #32488 is landing soon both are moot and this can just close.

If a maintainer would rather have the buffered approach in this PR I am happy to do that instead.

robobun · 2026-07-07T13:23:51Z

Opened #33664 with a Bun.serve-only variant that buffers the pipelined request(s) and re-dispatches from markDone(), so every pipelined request is served in order rather than dropping the tail and closing. It is gated on !flags.usingCustomExpectHandler so node:http keeps its current behaviour (and test-http-pipeline-socket-parser-typeerror.js keeps passing); this PR or #32488 remain the route for node:http.

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

robobun · 2026-08-13T20:04:43Z

Closing: both halves of this PR are covered elsewhere now, and this branch conflicts with main.

node:http: node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed #32488 landed on 2026-07-16. The node-http.test.ts case from this PR passes on current main as written (both pipelined responses are served, like Node), and test-http-pipeline-socket-parser-typeerror.js passes on main too, so the expectations.txt entry is not needed.
Bun.serve: Bun.serve: hold a pipelined request until the response ahead of it completes instead of closing the connection #38128 replaces the close in HttpContext.h by holding the pipelined request and answering it once the response ahead of it completes. This PR's serve.test.ts case delivers the first response there as it asserts; only its calls === 1 pin differs, because the second request is answered as well.

Any further work on the Bun.serve side should go to #38128.

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

github-actions Bot added the claude label Jun 27, 2026

ci: retrigger

c0bdaae

claude Bot reviewed Jun 27, 2026

View reviewed changes

Comment thread test/js/bun/http/serve.test.ts Outdated

Comment thread packages/bun-uws/src/HttpContext.h Outdated

claude Bot reviewed Jun 27, 2026

View reviewed changes

Comment thread packages/bun-uws/src/HttpContext.h Outdated

Comment thread packages/bun-uws/src/HttpContext.h

Comment thread packages/bun-uws/src/HttpContext.h Outdated

Comment thread test/js/node/http/node-http.test.ts Outdated

robobun requested a review from Jarred-Sumner as a code owner June 27, 2026 19:00

claude Bot reviewed Jun 27, 2026

View reviewed changes

Comment thread packages/bun-uws/src/HttpContext.h Outdated

Comment thread packages/bun-uws/src/HttpContext.h Outdated

coderabbitai Bot reviewed Jun 27, 2026

View reviewed changes

Comment thread packages/bun-uws/src/HttpContext.h

Comment thread test/js/bun/http/serve.test.ts

robobun mentioned this pull request Jul 7, 2026

Bun.serve: defer pipelined HTTP/1.1 requests behind an async response instead of closing the socket #33664

Closed

github-actions Bot mentioned this pull request Jul 20, 2026

node:http: deliver a pipelined POST's body when the previous response is still in flight #34761

Closed

robobun mentioned this pull request Jul 22, 2026

Bun.serve: deliver the in-flight response when a pipelined request arrives while it is still pending #35036

Closed

robobun mentioned this pull request Aug 13, 2026

Bun.serve: hold a pipelined request until the response ahead of it completes instead of closing the connection #38128

Open

robobun closed this Aug 13, 2026

Conversation

robobun commented Jun 27, 2026

What

Cause

Fix

Verification

Uh oh!

coderabbitai Bot commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Suggested reviewers

Uh oh!

robobun commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jun 27, 2026

Uh oh!

github-actions Bot commented Jun 27, 2026

Uh oh!

robobun commented Jun 27, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

robobun commented Jun 27, 2026

Uh oh!

robobun commented Jun 27, 2026

Uh oh!

robobun commented Jun 27, 2026

Uh oh!

robobun commented Jul 7, 2026

Uh oh!

robobun commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Jun 27, 2026 •

edited

Loading

robobun commented Jun 27, 2026 •

edited

Loading