Repository navigation
Conversation
|
Updated 5:08 AM PT - Oct 22nd, 2026
❌ @robobun, your commit 147965f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 46067That installs a local version of the PR into your bun-46067 --bun |
|
Status
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change records peer FINs in native socket state and exposes that state to HTTP close handling. Node HTTP handoff sockets now track connection and readable-stream closure separately. Tests cover buffered data and teardown behavior across CONNECT and upgrade tunnels. ChangesHTTP handoff socket teardown
Possibly related issues
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the primary change: preserving unread bytes for node:http CONNECT and upgrade sockets after connection closure. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description check | |
The description is complete and relevant. It explains the problem, implementation, behavior changes, trade-offs, known differences, and verification results. It does not use the exact template heading… |
Full details: Description check
-
Explanation
The description is complete and relevant. It explains the problem, implementation, behavior changes, trade-offs, known differences, and verification results. It does not use the exact template headings, but it provides the required information in equivalent sections.
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the native side of this change: the new read_fin bit is initialized at every socket-init site (listen and connect init in context.c, the adopt path in loop.c, us_socket_from_fd in socket.c), is set only when a read returns 0, and the new gate in HttpContext::onClose is IsNodeHttp-only, so Bun.serve / uWS CONNECT tunnels keep the unconditional end-of-stream dispatch they had before.
Extended reasoning...
The PR adds a one-bit peer-FIN record to usockets, gates uWS's tunnel end-of-stream dispatch on it for node:http only, and reworks NodeHTTPServerSocket close handling in src/js/node/_http_server.ts so a handed-off 'connect'/'upgrade' socket with unread bytes outlives its native connection and is counted separately by the server. It touches no auth, crypto, or input-parsing surface. The inline findings cover the user-visible behavior change (sockets nobody reads now keep server.close and connection counts pending) that the author explicitly flags as needing a maintainer decision, so a human must weigh that design choice; this note only records that the small native portion is consistent and scoped.
Findings marked 🟡 are optional suggestions and need no follow-up push.
…n the connection closes The socket that a 'connect' listener gets, and the socket of an 'upgrade' listener whose request has no body, is the connection itself as a net.Socket. usockets closes a connection when both sides have sent their FIN, and NodeHTTPServerSocket answered that close with destroy(). The bytes that its reader had not taken were dropped, and there was no 'end'. Like Node's net.Socket, the socket closes after its own 'end' and 'finish'. After the peer's FIN it waits for its reader, or for its owner's destroy(). The end of the stream is push(null) and read(0), which leaves a paused socket paused. usockets records the peer's FIN on the socket (us_socket_read_fin), and uWS reports the end of a tunnel's stream only for it. A connection that closes with no FIN of the peer (a reset, a failed TLS session) closes the socket as before. The server tracks connections. It stops tracking the socket when its connection closes, so a socket that waits is not counted, server.close() does not wait for it, and a socket that its owner dropped is collected.
4796e20 to
147965f
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Its values. Node hands its listener the net.Socket itself when the request is complete at the emit, and otherwise an | ||
| // UpgradeStream, which the close of the connection destroys: | ||
| // https://github.com/nodejs/node/blob/v26.3.0/lib/_http_server.js#L1027-L1050 | ||
| // https://github.com/nodejs/node/blob/v26.3.0/lib/_http_server.js#L1110-L1143 | ||
| // Bun emits 'upgrade' before it parses the body, so an Upgrade request with a body is never complete there. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // #closeHandled of a kHandedOffSocket that has the end of its stream. Like Node's net.Socket, it closes after its own | ||
| // 'end' and 'finish', so the close of the connection leaves it the bytes that its reader has not taken. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Whether a handed-off socket whose connection closed with no FIN of the peer gets 'end' when its reader has | ||
| // taken every byte, as it always has. That is the close after a reset of the peer, and after a failed TLS | ||
| // session. Node emits 'error' there. pipe() needs the 'end' to end its destination. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The peer's FIN ended the stream of a net.Socket, and its reader has not taken all of it. Like Node's, the | ||
| // socket closes after its own 'end' and 'finish': _destroy() runs the rest of this function. A write that | ||
| // waits for a drain has failed, and destroySoon() asked for the close, so neither waits for the reader. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // So did the server's part: it tracks connections, and keeps nothing for a socket that its owner can drop. | ||
| // Node, whose socket keeps its handle, counts the socket until it closes, and server.close() waits for it. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The connection has closed, and the socket waits for its reader. The write fails like one that waited for a | ||
| // drain at the close: ERR_STREAM_DESTROYED to its callback, and no 'error'. Node fails it with EPIPE, and | ||
| // emits 'error'. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
This automated review ran on the new push and found no bugs; the points raised on the previous version are addressed in the code (a waiting socket is now dropped from kTrackedConnections in #onClose, with tests asserting getConnections() 0 and a completing server.close(cb), and the 120 s per-test timeouts are gone). Because this is a +1312-line change to socket lifecycle across usockets, uWS and node:http with a deliberate behavioral divergence from Node that the description itself asks a maintainer to decide, a human look is still worthwhile.
What was reviewed:
read_finis zeroed at all fourus_socket_tinit sites (listen/connect in context.c, adopt in loop.c,us_socket_from_fdin socket.c) and set only on a 0-byte read; the uWS gate isIsNodeHttp-only, so Bun.serve CONNECT paths are unchanged.- The
#onClosetri-state: thekOutlivesConnectionbranch only runs when not destroyed/ended/destroySoon'd/pending-write, nullskHandlebefore returning, and_destroyre-enters withnullafterdestroyedis set, so the full close sequence runs once; a 'readable' listener destroying synchronously insidepush(null)lands in the same path. - One thing for the human reviewer: the PR description ("the server counts a waiting socket", table rows "getConnections() 1, server.close(cb) waits — as Node") no longer matches the code or the new test at test/js/node/http/node-http-connect.test.ts:2511, which asserts the opposite; the description should be updated to the behavior that lands.
Extended reasoning...
The commit adds a read_fin bit to usockets and exports us_socket_read_fin, gates uWS's end-of-stream dispatch for node:http tunnels on it, and reworks NodeHTTPServerSocket's close handling so a 'connect'/body-less 'upgrade' socket survives the native close until its reader drains it. It touches no auth, crypto or input-parsing surface beyond the existing TLS close path, where a failed session no longer reports stream end. Both inline findings from the prior run (permanent connection count, test timeouts) are addressed by this push with dedicated tests. Deferral rather than approval rests on the size and complexity of the lifecycle change, the native/uWS surface, and the explicit maintainer decision the author requests on keeping unread sockets alive outside server tracking.
Problem
'connect'or body-less'upgrade'listener that ends the socket and reads late loses data. At the client's FIN the socket emits'close'with no'end'(for awaitthrowsERR_STREAM_PREMATURE_CLOSE). Node.js v26.3.0 delivers every byte, then'end', then'close'.NodeHTTPServerSocket#onClose(src/js/node/_http_server.ts) answered withdestroy().Fix
node:netdoes, and closes after'end'and'finish'. The FIN no longer resumes a paused socket.us_socket_read_fin), and uWS ends a tunnel's stream only for it. A reset closes the socket as on main.server.close()does not wait for a waiting socket.node-http-connect.node.mts(35 tests, also run by Node) andnode-http-connect.test.ts(44). 46 fail on main. Self-reviewed: 47 concerns raised, 42 addressed.Background
net.Socketfor a complete request, and otherwise anUpgradeStream, which the close destroys. In Bun an Upgrade with a body keeps that behaviour.Downsides
'close'when its connection closes, as in Node. It has no fd, and it is collected when its owner drops it..text0.Notes
Repro (Linux x64, idle machine, Node.js v26.3.0, debug builds of main and of this branch). The first test of
describe("a tunnel socket whose connection closes before it is read")innode-http-connect.node.mtsis the same flow with no timers.{"received":16384,"events":["read-start","end","close"]}{"received":0,"events":["close","read-start"]}{"received":16384,"events":["read-start","end","close"]}Rows (the same scripts under each runtime)
end,closeclosedestroySoon(), peer silentfinish,closefinish,end,closeerror ECONNRESET,closeend,closepipe()calls and an'error'listener that ends the other leg (http-proxy's WebSocket pass), 8 client resets: upstream legs left opengetConnections()1,server.close(cb)waitsgetConnections()0,server.close(cb)calls backserver.close(cb)waitsserver.close(cb)calls backend,closecloseServer accounting. Node counts a socket until it closes, because the socket keeps its handle, and
server.close(cb)waits for it. So in Node a socket whose peer sent bytes that nobody reads is a connection for ever. The two-pipe()proxy of the Node.js docs makes such sockets:unpipe()leaves the tunnel paused, and the client's TLS close_notify arrives behind it. Here the connection of such a socket has closed, so the server stops tracking it:getConnections(),maxConnectionsandserver.close(cb)do what they do on main. The first revision of this PR counted these sockets as Node does. A listener that ends a socket and never reads it then left a count that nothing could release (#45095 is a report of that kind), so the count is gone.Still different from Node.js
getConnections(),maxConnectionsandserver.close(cb)do not see it. Node counts it until it closes.'end'(if the reader took everything) and'close', no'error'. Node emits'error'. node:http: emit 'end' and 'error' on req.socket when the client closes the connection #43708 left this on purpose. With Node's behaviour a relay that only logs'error'stops ending its upstream leg, and a relay with no listener stops the process.write()on a socket that waits (possible after a hangup): the callback getsERR_STREAM_DESTROYEDand there is no'error'. Node givesEPIPEon both.remoteAddress(Bun remembers the peer, Node does not) and notlocalAddressandlocalPort(Node does).server.close()before the'close'of the last socket. Bun runs it after (node:http: queue the server 'close' check from the connection's close handler, not from a socket 'close' listener #45257).net.Socket(req.completeis true at the emit). Bun emits'upgrade'before it parses the body, so that socket still closes with its connection. node:http: an Upgrade request with a body follows Node v26 #45058 moves the emit. The PR that lands second setskHandedOffSocketthere.Measurements (main
4354b80d7bagainst this branch, before the rebase and before the server accounting changed, except where a line says otherwise)'connect'and'upgrade': JS frames -5 (-6 with a'readable'listener), tick entries +0, native calls -2 (entry counters on every builtin function, debug build)._destroy+6,#onClose+0 to +4, aborted response +4. No allocation.heapStats): the same objects on both (10 Function, 1 Promise, 11 Object, 6 Array, 3 JSLexicalEnvironment, the socket and the response).epoll_ctlADD 2002, MOD 3501, DEL 2001,close2001,shutdown2000,recv4000,send2500, from anLD_PRELOADcounter).sizeof(struct us_socket_t): 80 bytes on both (the bit uses a spare bit)._http_serverbuiltin +1533 bytes (99002 to 100535, frombundle-modules.ts --debug=OFF)..text+0 (81369941) and.rodata+0 (size -A, measured before the server accounting changed; the native part is the same). The fourHttpContext::onCloseinstantiations keep their sizes, andus_socket_read_finis inlined.Suites run on the debug build:
test/js/node/http/(39 files, 1270 tests) andtest/js/first_party/ws/(21 files, 3189 tests) on this revision. Before the server accounting changed, also 845 files oftest/js/node/test/parallel(test-http*,test-https*,test-http2*, and every connect, upgrade, proxy and tunnel test). The 4 of them that fail also fail on main.Self-review: 47 concerns, 42 addressed. Not taken: Node's
'error'for a reset on a tunnel (4 concerns, reason above). Not possible in this tree: thereq.completekey (1, #45058). Not run: Windows and macOS. The tests size their payloads fromreadableHighWaterMark, and each AF_UNIX row has a TCP twin.Neighbours: #45058 (Upgrade with a whole body), #44885 (parked sockets above the mark), #45257 (order of the server's
'close'), #39889 (write callbacks of a dead connection), #40897 (speed of this test file), #43708 and #43557 (the code this builds on). #44034 reported this loss and was closed as not planned, because no user had reported it.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/node/http/node-http-connect.test.ts