node:http: hand a pipelined Upgrade request to 'upgrade' and keep its writes behind the responses ahead - #43441
Conversation
… writes behind the responses ahead An Upgrade request that arrives while an earlier response on the same connection is still in flight was dispatched to 'request' with req.upgrade === false. shouldUpgradeCallback now runs at dispatch for every Upgrade request. When it accepts, the connection enters tunnel mode at once and the socket goes to 'upgrade' at once, like Node. What the listener writes to the socket waits until the pipeline reaches this request, so the responses ahead keep their place on the wire. A pipelined CONNECT uses the same queue. The builtin ws adopts the socket through that queue as well, since the native upgrade takes the connection over, and the request keeps its WebSocket upgrade context when the listener ends the response ahead during the dispatch.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 3:31 PM PT - Sep 19th, 2026
✅ @robobun, your commit dc2445606c4f090b4863b843d74df9369d3d1dab passed in 🧪 To try this PR locally: bunx bun-pr 43441That installs a local version of the PR into your bun-43441 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
src/js/node/_http_server.ts— Behind a pipeline, an 'upgrade' listener that reads the socket of an Upgrade request carrying a body can get the request ahead's buffered body delivered as the Upgrade request's body, and the request ahead loses it.#resumeSocketat _http_server.ts:1817 takeshandle.response, which for the pipelined hand-off added at :903-913 is still the response in flight, and :1825-1827 pushes whatever itsresume()drains intokUpgradeIncoming. Fix: resolve the hand-off's own native response (this[kHandoffResponse] ?? handle?.response, as :2023 and :1622 already do) in#resumeSocketand inpause()at :2007, so pause/resume and the drained bytes target the handed-off request on every path. [also at: src/js/node/_http_server.ts:1817 - Behind a pipeline, reading the socket handed to 'upgrade' for an Upgrade request that carries a body can push the unread body tail of the request ahead into the Upgrade request's stream.]Extended reasoning...
The base never reached this path: a pipelined Upgrade went to 'request' (the
!isPipelinedgate), and for a non-pipelined Upgradehandle.responseis the Upgrade's own native response. Trigger:POST /firstwith a body whose handler callsreq.pause()before the body is consumed (node-http-connect.test.ts:753-766 does exactly this for the CONNECT sibling), or leaves a body above the readable high-water mark unread (readStop -> handle.pause()), followed in the same write byGET /wswithConnection: Upgrade,Upgrade: fooand aContent-Lengthbody. The native side buffers /first's body in buffered_request_body_data_during_pause and sets IS_DATA_BUFFERED_DURING_PAUSE_LAST at its fin (NodeHTTPResponse.rs:658-664), then parses the Upgrade head and dispatches it pipelined. _http_server.ts:903-913 runs upgradeToTunnel(true, handle) (HTTP_NODE_TUNNEL_AFTER_BODY only, isConnectRequest stays false: JSNodeHTTPServerSocket.cpp:114-119), then…Verification: normal — triggered when a pipelined Upgrade request that carries a body (Content-Length/Transfer-Encoding, so
hasBodyis true) arrives behind an in-flight request whose body bytes are parked natively (its handler calledreq.pause(), or a full push readStop'd the socket), and the raw 'upgrade' listener reads the socket. Mechanism verified in the code: - The new pipelined branch at… | normal —…
There was a problem hiding this comment.
Code review completed
Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.
Still open from earlier reviews (9):
- 🔴
src/js/node/_http_server.ts:912—A raw 'upgrade' listener behind a pipelined response never gets its 101 onto the wire unless it also ends or destroys t… - 🔴
src/js/node/_http_server.ts:1984—An 'upgrade'/'connect' listener behind a pipelined response whose client disconnects early never gets its socket.write(… - 🔴
src/js/node/_http_server.ts:2610—A client that pipelines an Upgrade or CONNECT behind a large response can receive the listener's 101 spliced into the m… - Also unresolved: 6 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
A CONNECT or Upgrade socket wrote straight to the connection. Behind a response whose tail was still in the uWS send buffer or in a zero-copy write, those bytes landed inside that response, and a socket.end() lost its FIN. The tunnel's bytes now wait in its stream buffer while the connection owes HTTP output, and HttpContext::onWritable flushes them once the responses ahead are done. Also from the review of the pipelined hand-off: uncork the socket at the hand-off, fail parked write callbacks when the socket is destroyed, let pause/resume target the handed-off request's response, recheck the WebSocketServer state in the deferred ws upgrade, and build no ServerResponse for a CONNECT that is not pipelined.
…n queue order From the review: the ws adoption runs on a fresh turn, since a native callback on the stack may not outlive the socket it replaces. A socket whose FIN waits for the bytes ahead of it completes 'finish' once the FIN is out. A listener's socket.end() keeps its place behind every queued response, and a response that closes the connection releases the hand-off before the FIN. Failed parked callbacks run on the next tick.
|
This PR and my branch fix the same bug, so I stop there and do not open a second PR. Branch: I ran the tests of that branch against the head of this PR (beba491) with a debug build. Four results can be useful here.
One more observation that follows from the design of this PR: The tests are in the two test files on the branch. Take what helps. |
… close response ahead A response with Connection: close closed the connection natively at its end, before the hand-off queued behind it could write. The native close gate now leaves that to the tunnel's own socket.end(). A destroyed socket also fails the write callback that waits for a native drain.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/jsc/bindings/node/JSNodeHTTPServerSocket.cpp— A raw 'upgrade' listener behind a pipelined response cannot read the Upgrade request's body until the response ahead finishes, so a listener that waits for that body before ending the response ahead hangs; Node resumes reads once the transport drains. queuedResponsesHoldReads at JSNodeHTTPServerSocket.cpp:469 exempts only isConnectRequest, but an Upgrade with a body stays HTTP_NODE_TUNNEL_AFTER_BODY until its body fin, so onNodeHttpReadsResumable keeps the flood pause while the hand-off is still counted in nodeHttpQueuedPipelinedCount. Fix: use the same tunnel predicate as HttpContext.h:863 and :930 (isConnectRequest || HTTP_NODE_TUNNEL_AFTER_BODY) so a queued hand-off never holds reads.Extended reasoning...
Trigger: the client pipelines GET /first and an Upgrade request carrying a body (Content-Length or chunked) whose body bytes arrive in a later packet than its head (Node's http client writes head and body separately), while /first's response still has unsent bytes in the uWS send buffer. The dispatch of the Upgrade takes the pipelined branch at packages/bun-uws/src/HttpContext.h:482-490: nodeHttpQueuedPipelinedCount++ and, because getBufferedAmount() > 0, HTTP_NODE_READS_PAUSED is set and the socket is paused. The JS dispatcher (src/js/node/_http_server.ts:897-903) calls socketHandle.upgradeToTunnel(hasBody=true, handle): upgradeToTunnelModeImpl at JSNodeHTTPServerSocket.cpp:114-119 only sets HTTP_NODE_TUNNEL_AFTER_BODY and returns; isConnectRequest stays false until the body fin (HttpContext.h:620-623). 'upgrade' is emitted at once. The listener does req.on('data')/req.on('end', () => first.end()). req's _read resumes via handle.resume() -> do_resume (NodeHTTPResponse.rs:1393) -> resume_socket -> Bun__NodeHTTP__onReadsResumable -> onNodeHttpReadsResumable…
Verification: nit (mechanism verified; the trigger is a narrow chain of client and listener behaviour, and the stall is bounded by requestTimeout). Triggering condition: a client pipelines an Upgrade request that carries a body (Content-Length/chunked) behind a response that has kernel-level backpressure (uWS getBufferedAmount() > 0) when the Upgrade head is parsed, the body bytes arrive in a later packet,…
…on: close response ahead An Upgrade with a body is a tunnel before its body ends; a queued one no longer keeps reads paused, so the listener can read the body while the response ahead drains. After a response that closes the connection, the hand-off socket is destroyed once its FIN is out, like Node's destroySoon().
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
packages/bun-uws/src/HttpResponseData.h— pre-existing, widened: a pipelined request whose body is still arriving when the response ahead ends loses the rest of that body and never emits 'end', so the handler waiting on it hangs; the pipelined Upgrade-with-body this PR now hands to 'upgrade' (src/js/node/_http_server.ts:898) takes the same path. markDone at HttpResponseData.h:52 sets inStream = nullptr for the whole connection, although inStream was armed by the queued request's on_data (HttpResponse.h:977), and HttpContext.h:625 then discards every later body chunk. Fix: markDone must not clear an inStream/userData that belong to a queued pipelined request (or startPipelinedResponse / the tunnel switch must re-arm it), for POST and Upgrade bodies alike.Extended reasoning...
Same drop for a plain pipelined POST on the base; new here is that the Upgrade case now reaches the 'upgrade' listener, which typically reads req and ends the response ahead itself.
Client pipelines GET /first and an Upgrade (or POST) with Content-Length N behind it; the body spans several segments (slow upload).
/first stays pending, so the second request is dispatched with HTTP_NODE_PIPELINED_DISPATCH. The dispatcher sets handle.ondata (src/js/node/_http_server.ts:845) -> set_on_data (NodeHTTPResponse.rs:2282) -> raw_response.on_data -> HttpResponse.h:977 inStream = on_data_shim, userData = this request.
For the Upgrade, line 898 upgradeToTunnel(true) sets HTTP_NODE_TUNNEL_AFTER_BODY and 'upgrade' is emitted; the listener attaches req.on('data'). Each chunk pushed re-arms inStream through _read -> resume (NodeHTTPResponse.rs:1415).
The response ahead ends from a timer or its own async work: res.end -> uws_res_end -> HttpResponse::end -> internalEnd -> markDone (HttpResponse.h:247 or 310) -> HttpResponseData.h:52 inStream = nullptr.
The next body segment arrives:…Verification: pre-existing (widened in consumer, same route): triggered when a client pipelines a request with a body (POST, or now an Upgrade with Content-Length) behind an in-flight response and the response ahead ends while that body is still arriving in later segments. Mechanism verified in the code: -
packages/bun-uws/src/HttpResponseData.h:46-52—markDone()unconditionally setsinStream = nullptr…
This branch has not been deployed
1 participant
Stacked on #43376 (the CONNECT half). The diff here is relative to that branch. Fixes #43420.
Problem
'request'withreq.upgrade === false.shouldUpgradeCallbacknever runs and'upgrade'is never emitted. Node v26.3.0 runs the callback and emits'upgrade'at once.!isPipelinedgate in the dispatcher (src/js/node/_http_server.ts:889on the base). The gate exists because the builtinwsanswers through the socket's current response, which behind a pipeline is the response in flight.us_socket_write), past the uWS send buffer. Behind a large response its bytes landed inside that response's body and its FIN was lost. node:http: emit 'connect' for a CONNECT that is pipelined behind a pending response #43376 documents this race for CONNECT.Fix
shouldUpgradeCallbackruns for every Upgrade request. When it accepts, the connection enters tunnel mode and the socket goes to'upgrade'at once, like Node. The listener can end the response ahead from inside'upgrade', as the script in the issue does._write/_finalcallback (kPendingHandoff) untiladvanceResponsePipelinereaches this request, so the 101 follows the responses ahead on the wire. Node writes it first. A pipelined CONNECT (node:http: emit 'connect' for a CONNECT that is pipelined behind a pending response #43376) uses the same queue, with a test for the new order.HttpContext::onWritableflushes them once that is done. This applies to every tunnel, pipelined or not.wsadopts the socket throughkOnHandoffActive, and the dispatch tail keeps the upgrade context of a tunneled request (src/runtime/server/mod.rs:1501).test/js/node/http/node-http.test.ts(12 new tests),node-http-connect.test.ts(1 new),node-http-with-ws.test.ts(1 new). All fail on the base. Notes below.Background
isPipelined, queues its response, andadvanceResponsePipelinehands the socket to the next queued response when the current one finishes.upgradeToTunnel): the parser stops reading HTTP after this request and passes later bytes to the JS socket unparsed.kHandoffResponse(node:http: emit 'connect' for a CONNECT that is pipelined behind a pending response #43376) is the native response of the handed-off request.Writableissues one_writeat a time. Holding its callback holds every later write and the_finalbehind them.Notes
Shape. A review of the issue asked for the emit at dispatch with the writes held back, instead of an emit deferred until the responses ahead finish. A deferred emit hangs a listener that ends the response ahead from inside
'upgrade': the script in the issue and Node'stest-http-pipeline-socket-parser-typeerror.jsboth do that.Wire order. For
GET /firstfollowed by an Upgrade in one write, with/firstended from the'upgrade'listener: Node v26.3.0 sends101, then200. This branch sends200, then101. The events andreq.upgradematch Node.Cork. The dispatcher corks the socket after each request and nothing uncorked it for a tunnel (#43342, fix in #35664). A listener that wrote the 101 and kept the tunnel open never got it onto the wire. The hand-off now uncorks the socket. #35664 stays useful for the
'request'path.Second review round. A tunnel's deferred FIN now holds the Writable's
_finalcallback until the native drain sent it. The ws adoption runs on a fresh turn (setImmediate), never inside the native writable callback of the response ahead, becauseHttpResponse::upgradereplaces the socket. Asocket.end()from the listener keeps its place behind the responses queued ahead.Third review round. A response ahead with an explicit
Connection: closeclosed the connection natively at its end, before the queued hand-off could write.shouldCloseConnection()now leaves that to the tunnel's ownsocket.end()while a hand-off is queued. A destroyed socket also fails a write callback that waits for a native drain.Fourth review round. A queued Upgrade with a body no longer holds the connection's reads while the response ahead drains, so the listener can read its body. After a
Connection: closeresponse ahead, the hand-off socket is destroyed once its FIN is out, like Node'sdestroySoon().The upgrade context. Without the change in
mod.rs, the dispatch tail saw no pending response (the listener had ended it) and discarded the context withmaybe_stop_reading_body.server.upgrade()then returned false andwsanswered 500.Packaging. The self-review asked to fold the hand-off queue into #43376 so both tunnels land with one ordering. #43376 is a separate PR of the same author. The stack can be merged as one unit, or #43376 first and this PR after it.
Found next to this, not fixed here.
markDone()(packages/bun-uws/src/HttpResponseData.h:52) clears the connection'sinStreamwhen a response ends. A pipelined request whose body is still arriving then loses the rest of its body, and its'end'never fires. This is the same for a pipelined POST on main. An Upgrade with a body behind a response that ends mid-body now reaches'upgrade'and takes the same path.Related open PRs. #43413 sets
req.upgradebeforeshouldUpgradeCallbackruns. #43176 also editscompleteUpgrade()inws.js(replays early frames). Whichever lands second needs a small rebase there.Suites run with the debug build.
node-http.test.ts(170 pass),node-http-with-ws.test.ts,node-http-connect.test.ts,node-http-server-timeouts,node-http-req-socket-pause,node-http-backpressure,node-http-server-abort-events,node-http-server-socket-end-drain,first_party/ws/ws.test.ts,ws-proxy.test.ts, and 54test/js/node/test/parallel/test-http-*files for upgrade, CONNECT, pipelining, keep-alive, pause and abort. On the shared machine (load average above 250) the 64 MBCONNECT > should handle backpressurecase takes 6 s under the debug build and passes with a longer limit.