Conversation
…ade request is complete Node removes socketOnTimeout and frees the parser at the 'upgrade' hand-off only when the request message is already complete. While a declared body still arrives, the socket inactivity timeout emits 'timeout' on the request and on the server, and destroys the socket when nothing listens. The hand-off removed the 'timeout' listener in every case, so the socket timeout never applied to a body that stalled. The listener and the parser stand-in now stay until the message completes. Bun sees the end of the body only while it reads the connection and the request. So the hand-off becomes final at once when the 'upgrade' listener pauses the request or the socket, or ends the socket. A timeout does nothing when the native handle has the end of the body and JS does not, when the request was dumped or destroyed, and after ws turned the connection into a WebSocket. In those states the behavior is the same as before.
|
Updated 4:28 AM PT - Sep 20th, 2026
❌ @robobun, your commit 381ba4c has 1 failures in
🧪 To try this PR locally: bunx bun-pr 43617That installs a local version of the PR into your bun-43617 --bun |
|
Status: ready for review. This PR fixes flow 8 of #43455. It needs a maintainer to merge: the diff is green, and CI is red only from tests that this diff does not touch. How to reproduce the bug:
Test: CI: the test file passes (27 pass, 0 fail) on all 11 lanes in the last two builds: Linux x64 and aarch64 (glibc, musl, ASAN), Windows x64 and aarch64, macOS x64 and aarch64. Each build has a different red test, and none of them uses
All three are reported for triage. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughChangesUpgrade request handoffs now complete when request bodies finish, reads pause, sockets finalize, or relevant timeouts occur. Parser and timeout ownership remain active until handoff completion. Tests cover Upgrade and CONNECT behavior. Upgrade handoff completion
Suggested reviewers: Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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/js/node/_http_incoming.ts— nit: The new per-request slot is never declared in the IncomingMessage constructor, unlike most other symbol fields.kFinishUpgradeHandoffis only assigned lazily at src/js/node/_http_server.ts:984 for an Upgrade request with a body, so those objects get a late shape transition andfinishUpgradeHandoffreads a missing property on every other upgrade EOF. Fix: initialisethis[kFinishUpgradeHandoff] = undefinednext tothis[kAbortController] = nullat src/js/node/_http_incoming.ts:94 (import the symbol from internal/http), so the field exists on every instance like the sibling symbol fields do.Extended reasoning...
REVIEW.md asks that every instance field is declared with a default in the class body. src/js/node/_http_incoming.ts:78-94 initialises abortedSymbol, eofInProgress, kHeaders, kRawHeaders, kTrailers, kAbortController and the rest on every IncomingMessage. The diff adds a new symbol-keyed slot kFinishUpgradeHandoff (src/js/internal/http.ts:46) but only writes it at src/js/node/_http_server.ts:984, and only for an accepted Upgrade request that declares a body and is not paused. finishUpgradeHandoff (src/js/internal/http.ts:47-53) reads req[kFinishUpgradeHandoff] from emitEOFIncomingMessageOuter (src/js/internal/http.ts:165), the request 'pause' handler (src/js/node/_http_incoming.ts:58), socket.pause() and _final (src/js/node/_http_server.ts:1775, 1987); for CONNECT and body-less upgrades that lookup misses the own object and walks the prototype chain. Consequence is a hidden-class transition and a slower miss on an internal path, not a behavioural bug; nit only.
Verification: nit. Trigger: any accepted Upgrade request that declares a body and is not paused in shouldUpgradeCallback. Mechanism verified: the IncomingMessage constructor (/home/claude/bun/src/js/node/_http_incoming.ts:77-94) initialises abortedSymbol, eofInProgress, kHeaders, kReqShouldKeepAlive, kRawHeaders, kHeaderSource, kHeadersCount, kTrailers, kTrailersCount, kAbortController on every instance, but…
-
🟣
src/js/node/_http_server.ts— pre-existing, widened: after merging, an 'upgrade' listener that consumes the body of a stalled request has its socket destroyed at server.timeout, butreqnever reports the truncation. src/js/node/_http_server.ts:243 destroys the socket while the body is incomplete, and nothing aborts the request: #onClose at _http_server.ts:1678 only destroys_httpMessage.req, which is null for an Upgrade, and onDataIncomingMessage returns early on the abort event. Fix: when the connection closes with an incomplete Upgrade body, destroysocket[kUpgradeIncoming]with 'aborted' (ECONNRESET) like a normal request, so every close path (timeout, client reset, socket.destroy) reports it. The PR notes call this #43543; on the base branch the timeout never reached this path.Extended reasoning...
Client sends an Upgrade POST with Content-Length: 100 and 10 body bytes, then stalls. server.timeout = 200 and no 'timeout' listener on req or server. The 'upgrade' listener reads req with on('data') and processes the body on 'end', or waits for 'aborted'/'error'. The timer fires: onNodeHTTPServerSocketTimeout at _http_server.ts:225-233 finds a handle, not upgraded, hasBody pending, so it falls through; no listener returns true, so this.destroy() at line 243 runs. Socket._destroy -> #closeHandle -> handle.close(); native handle_abort_or_timeout (NodeHTTPResponse.rs:1262-1299) runs onabort (onServerRequestEvent: socket already destroyed) and then on_data_or_aborted(b"", last=true, Abort), which first sets body_read_state = Done (NodeHTTPResponse.rs:1655) and calls req's ondata; onDataIncomingMessage (internal/http.ts:183-189) returns immediately for the abort event, so no push(null) and no destroy. #onClose (_http_server.ts:1636-1707) computes req from this._httpMessage, which the upgrade dispatch never assigned (assignSocket is skipped at line 918), so the request is not destroyed…
Verification: nit (pre-existing misreport, widened by a new server-initiated path) — acknowledged in diff: the PR description's "Differences that stay" says "A timeout that destroys the socket in the middle of the body shows up on
reqas 'end' withcomplete === true(#43543, the same for every close of the connection there)"; that claim matches the code. Trigger: an accepted Upgrade request with a…
…e socket The release of the socket after the body of an Upgrade request freed whatever socket.parser held at that time. An http client request that the 'upgrade' listener runs over the socket puts its own parser there. The deferred release now frees only the stand-in that the hand-off left. IncomingMessage declares the kFinishUpgradeHandoff slot in its constructor, like its other fields. Tests: the chunked flow sends the head and the body in one write, so it does not depend on the time between the two. A close of the socket fails a 'tunnel survives' test at once.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…ments The three states in which JS cannot see the end of an Upgrade request's body move into cannotSeeUpgradeBodyEnd(). Each comment that this change adds is now one line.
|
Reply to the two findings outside the diff in the review above.
The inline threads have their own replies. Two of them changed code: the deferred release now leaves a parser alone that the 'upgrade' listener put on the socket (78e03f3, with a test), and every comment that this PR adds under |
There was a problem hiding this comment.
I re-reviewed the two follow-up commits and found no new bugs; the captured-parser check in releaseSocketForHandoff and the close-wired/asserted tunnel tests cover the earlier inline comments. Because the release now depends on several flow-control guards in the node:http hand-off, a maintainer look is still worthwhile.
What was reviewed:
releaseSocketForHandoff(socket, req, parser?): the bound closure capturessocket.parserat hand-off, so a parser installed by the 'upgrade' listener survives the deferred release; the CONNECT and body-less paths pass no parser and release unconditionally, as before.cannotSeeUpgradeBodyEndinonNodeHTTPServerSocketTimeout: checked the three states (no handle,handle.upgraded,hasBody & done) against the flows the new "tunnel survives" matrix pins; the handler still falls through to Node's socketOnTimeout sequence otherwise.- The chunked "fills the request buffer" flow now sends head and body in one write, so it no longer depends on the head-to-body round trip beating the 200 ms timer.
Extended reasoning...
Overview
The PR changes when Bun's node:http server releases the socket's 'timeout' listener and parser shim for an Upgrade request that declares a body: instead of at hand-off, the release is deferred through a kFinishUpgradeHandoff closure on the request and run from message completion, request/socket pause, socket end, or the timeout handler itself when the body's end is no longer observable from JS. Files touched: src/js/internal/http.ts, src/js/node/_http_incoming.ts, src/js/node/_http_server.ts, plus about 250 lines of new tests in test/js/node/http/node-http-server-timeouts.test.ts. Since the prior review, commits 78e03f3 and 428214a added the captured-parser guard and its test, hardened the tunnel tests against hangs (close listeners resolve/reject, stream.writable asserted), combined the chunked head and body into one client write, and shortened comments.
Security risks
None specific to this change. The code runs in the JS builtin layer and does not parse untrusted bytes; the only new input-derived decisions are on handle.hasBody flags and Readable state. A hostile client can still only affect whether its own connection is destroyed at the inactivity timeout, which is the existing behaviour on main for non-upgrade requests.
Level of scrutiny
Moderate to high. The correctness of the change rests on a set of guards that encode where Bun stops reading the connection (pause handlers, _final, native done state, upgraded). Each is reasoned from Bun-specific flow-control behaviour that differs from Node, and the author notes one flow (ws upgrading a request with a body) that cannot be tested on a debug build. The bug hunt over the current head ran dry without new findings, and the earlier inline comments on the parser release and the test hangs are addressed in code; the _final-timing comment remains as an open thread for the author to weigh. That combination supports a human maintainer pass rather than an automated approve.
Other factors
The new tests follow the harness conventions (port 0, 127.0.0.1, Buffer.alloc fills, Promise.withResolvers, test.each matrices, failure events wired to the awaited promises) and pin listener counts and socket.parser state as observable compat surface. Comments were reduced to one-liners in the last commit, matching the repo's comment guidance. No pre-existing tests were weakened or removed.
This branch has not been deployed
1 participant
Problem
node:httpUpgrade request arrives throughreqafter 'upgrade'. If it stalls,socket.setTimeout()andserver.timeoutdo nothing. The socket stays open untilrequestTimeout(default 300 s). Node v26.3.0 emits 'timeout' onreqand the server, or destroys the socket.src/js/node/_http_server.ts:975) always removesonNodeHTTPServerSocketTimeout. Node'sonParserExecuteCommonremovessocketOnTimeoutand frees the parser only oncereq.completeis true.Fix
socket.parser).releaseSocketForHandoffremoves both when the message completes. CONNECT and a body-less Upgrade do not change.wsowns the connection. Those states behave as on main. Without the guards it destroyed live tunnels (Notes).test/js/node/http/node-http-server-timeouts.test.ts(17 new tests, main fails 5). Alsonode-http.test.ts, the ws suites, upstreamtest-http-upgrade-*.Background
server.timeoutarmssocket.setTimeout()per connection. Node'ssocketOnTimeoutforwards the socket's 'timeout' to the request (while!req.complete), the response and the server. With no listener it destroys the socket.onNodeHTTPServerSocketTimeoutis Bun's copy.ServerResponse, so onlyreqand the server hear 'timeout'.req.pause()andsocket.pause()stop reads of the connection in Bun, not in Node.Notes
Provenance
A differential probe against Node v26.3.0 found this during the review of #43456. No user reported it. It is flow 8 of #43455. The notes of #43337 and #43561 name the early listener detach as separate work.
Repro
An Upgrade POST with
Content-Length: 100and 10 body bytes. The 'upgrade' listener callsreq.socket.setTimeout(200)and adds its own socket 'timeout' listener.listenalso adds a 'timeout' listener toreq.More probes that give the same result as Node on this branch and differ on the canary:
server.setTimeout(200, cb)(the server gets 'timeout'),req.setTimeout(200, cb), a chunked body that stalls inside a chunk, an https server, and an Upgrade on a kept-alive connection after a normal request. While the body arrives,req.socket.listenerCount("timeout")is 1 andreq.socket.parseris set. After it, both are released, as in Node. A timeout that fires after the body is complete does nothing, read or unread.Why the release has guards
The first version released only at the end of the message. Two rounds of self-review found flows where it destroyed a live tunnel, and the review of this PR found one more (the last row). In each flow the client sent the whole body, Node and the canary keep the tunnel open at
server.timeout, and the version without the guard destroyed it. The cause is always the same:req.completein Bun follows what JS has seen, not the wire.req.pause()(also pipe backpressure)socket.pause()UpgradeStreamwrapperpause()shouldUpgradeCallbackpauses the request_finalreqagain (#43466)handle.hasBodyreq._dump()while a read is pending_dumpclearshandle.ondata(flow 7 of #43455)donefor await (const chunk of req) break, orpipeline(req, dest)with a destination that fails_destroyreleases the native handle (flow 9 of #43455)wsupgrades a handshake that declares a bodyhandle.upgradedsocket.parser, and the deferred release freed itThe event-time releases sit exactly where Bun stops reading at the request of the listener. The states that cannot go back are checked when the timeout fires. So the handler acts only while Bun reads the body and nothing is parked, and in that state
req.completefollows the wire. A stalled consumer whose request buffer is full stops reads in Node too (readStop), and Node destroys that socket at the timeout, so this branch does the same.The nine "tunnel survives" tests and the parser test pin these flows, and each fails without its guard. The
wsflow has no test: a debug build aborts withassertion failed: !self.body_read_ref.get().haswhenwsupgrades a request that declares a body (#43408, #43427). A probe shows the WebSocket alive pastserver.timeouton this branch.Two batteries ran against the final version, with every byte of the body sent and
server.timeoutset. In 69 flow-control scenarios the tunnel outcome is the same as in Node, except for three rows. Two differ in the other direction: Node reads 64 KiB at a time and stops at the high water mark, Bun reads the whole 100 KB body at once and completes it. One is the corked socket below, the same on the canary. In 50 ways to abandon the request, the events and the tunnel outcome are the same as on the canary. The timeout in these runs was 1000 to 1500 ms: with 300 ms, the stalls of a debug build are longer than the timeout.The first review proposed to stack this change on #43557 and on a fix for flow 9. I did not do that. The guards make the change independent of both, and the behaviour in those states is the behaviour of main. When #43557 removes the 'pause' handler, its guard goes with it.
Differences that stay
requestTimeoutstill does. The same holds after_dump(), after a destroy that keeps the socket, and aftersocket.end().req.complete === trueat the hand-off and releases the socket before 'upgrade'. Bun learns it a tick later, so inside the listenerlistenerCount("timeout")is 1 andsocket.parseris set.socketOnErroron the raw socket while the body arrives, so a socket error also reaches 'clientError'. Bun gives the listener the raw socket, not anUpgradeStream, so the listener must see the error on the socket it received.test-http-upgrade-server-with-body-error.mjsdepends on that. For the same reason the listener can callsocket.setTimeout(0)on what it received and switch the timeout off. In Node it needsreq.socketfor that.reqas 'end' withcomplete === true(node:http: an Upgrade request whose body is cut short by a connection close emits 'end' with complete=true #43543, the same for every close of the connection there).Found on the way, not part of this change
server.timeout, the timeout then ends that connection.test-http-agent-keepalive,test-http-client-timeout-optionandtest-https-timeoutfail on a debug build of main and pass on the release canary.Merge order
#43441 moves the upgrade hand-off into
emitUpgradeHandoff, and #43461 adds anelsebranch next to it. Both can takereleaseSocketForHandoffas is: release at once without a body, leavekFinishUpgradeHandoffon the request with one.Suites run with the debug build
node-http-server-timeouts,node-http,node-http-with-ws,node-http-req-socket-pause,node-http-server-abort-events,node-http-backpressure,node-http-transfer-encoding,node-http-connect,first_party/ws/ws.test.ts,ws-upgrade-events.test.ts, and 226 upstreamtest-http-*/test-https-*scripts (timeout, upgrade, connect, keepalive, server, pipeline, parser, incoming, socket, pause, abort, destroy, request, stream). The new tests passed 5 sequential runs and 8 parallel runs of the final version, and as many of each earlier version. In CI the file passed on Linux (x64, aarch64, musl, ASAN), Windows (x64, aarch64) and macOS (x64, aarch64).Failures that are the same without this change: the three scripts above, the
tests should run on buncase ofnode-http-connect.test.ts(needs more than its 5 s on a debug build), and thebody_read_refabort when the ws suites andnode-http-connectrun in one process. I left out 41 proxy scripts: the container setsHTTP_PROXY, and the 16 of them that I ran fail the same way on the canary.[human-review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 1
evidence per changed file