Conversation
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
Comment |
|
Status: reproduced on bun 1.4.0 with the three-worker |
In a cluster worker, http.Server#listen() bound its own SO_REUSEPORT socket with Bun.serve(), so listen(0) gave every worker a different port, a unix path could only be served by one worker, bind errors had Bun.serve's shape and worker.disconnect() did not know about the server. The worker now asks the primary through cluster._getServer() with sharedOnly, like a TLS net.Server does: the primary binds once per key and ships the descriptor, and the worker's Bun.serve() accepts on it through a new listen_fd path (uws HttpContext::listen_fd -> us_socket_group_listen_fd, ServerConfig.listen_fd, only read for node:http servers). config.address still describes the bound address, so address()/url/errors are unchanged, and a server started on an inherited descriptor no longer unlinks the unix socket file the primary owns. close()/closeAllConnections() release the handle; the handle's owner is the server so disconnect() closes it. exclusive (and reusePort, which implies it) still binds in the worker, as does a worker without a channel, and Windows keeps binding per worker: several processes accepting on copies of one listening socket block each other in accept() there and Bun.serve cannot take round-robin handoffs.
|
Updated 10:43 AM PT - Aug 13th, 2026
✅ @robobun, your commit 54dae5d14781d4f7ded7fec880d8698a825ea041 passed in 🧪 To try this PR locally: bunx bun-pr 38109That installs a local version of the PR into your bun-38109 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it spans the C++/Rust/JS boundary with new fd-ownership semantics, makes a design call to force sharedOnly (SCHED_NONE) for http workers and keep the Windows divergence, and the author has flagged a follow-up push for known edge cases, a human look would still be worthwhile.
What was reviewed:
- The new
listen_fdpath throughHttpContext→uws_app_listen_fd→App::listen_fd→mod.rsAddr::Fdarm, including errno propagation on failure and the unix-unlink skip for inherited descriptors. _http_server.ts: thekClusterListeningIdstale-reply guard,handle.adoptedtoggling aroundkRealListen, andreleaseClusterHandleon bothclose()andcloseAllConnections()— a candidate concern thatcloseAllConnections()doesn't invalidate a pending listen was examined and ruled out.ServerConfig.rs:fdgated behindonNodeHTTPRequest,allow_hotdisabled, http3+fd rejected.
Extended reasoning...
Overview
This PR routes node:http Server.prototype.listen in cluster workers through cluster._getServer (with sharedOnly: true) instead of binding directly, so all workers share one primary-bound port for listen(0) and unix paths. To support this, it adds a new listen_fd entry point through the full uWS stack (HttpContext.h / App.h / libuwsockets.cpp / uws_sys/App.rs), teaches ServerConfig to accept an fd (only when onNodeHTTPRequest is set), and plumbs it through mod.rs's listen path with errno-based error reporting and a guard against unlinking a unix socket file the primary owns. _http_server.ts gains ~110 lines: DNS resolution before querying the primary, stale-reply invalidation via a listen counter, kClusterHandle bookkeeping so close() / closeAllConnections() / worker.disconnect() release the primary's handle, and ExceptionWithHostPort bind errors. Six new subprocess tests cover http/https shared port, unix path sharing and unlink timing, bind-error shape, re-listen after close, and disconnect-closes-server.
Security risks
None identified. The fd option is not exposed on public Bun.serve (gated behind the internal onNodeHTTPRequest key), and the descriptor arrives via SCM_RIGHTS from the cluster primary over the trusted IPC channel. listen_fd_from_number bounds-checks the value before casting.
Level of scrutiny
High. This is a ~500-line change across four languages with new fd-ownership contracts at every boundary (C++ says "on failure the caller still owns it", Rust reads errno after the C++ call, JS toggles handle.adopted around a fallible Bun.serve). It changes the default listen behavior for every node:http server running in a cluster worker, forces shared-handle scheduling regardless of SCHED_RR, and explicitly leaves Windows on the old divergent path. These are design tradeoffs a maintainer should sign off on.
Other factors
- The author's own status comment says a follow-up push is coming for known edge cases (bracketed IPv6 hosts, mixed-server hint text, descriptor ownership when listen fails after
Bun.serve()returned). That last one is exactly the kind of fd-lifecycle edge this review would want settled before approval. - The bug-hunting pass raised and refuted one candidate:
closeAllConnections()not bumpingkClusterListeningId(unlikeclose()). It was ruled out, but the asymmetry between the two close paths is the sort of thing a maintainer may want to comment on. - Test coverage is thorough for the happy paths and several failure paths, and the description enumerates which tests fail on the unfixed build. All new tests are
skipIf(isWindows), consistent with the stated Windows carve-out.
…down, update mixed-kind hint The primary binds addresses, so strip the brackets Bun.serve() accepts around an IPv6 literal before isIP and the query; listen(0, "[::1]") in a worker otherwise went to dns.lookup and failed with ENOTFOUND. A listen() that throws after Bun.serve() returned (while wiring up the listener) used to close the shared descriptor underneath the live listener; tear it down with close() instead, on every listen path, so a failed listen never leaves a listener running either. The hint the primary attaches when a shared-only query meets a round-robin handle now describes http/https/tls vs net servers, since http servers reach it too.
6e15314 to
36154f0
Compare
|
On the The follow-up push (rebased on current main) covers the three edge cases from the status comment, each with a test: bracketed IPv6 hosts are unbracketed before the primary query, the primary's mixed-kind hint now describes http/https/tls vs net, and a listen that fails after |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs (the earlier bracket-stripping nit is fixed in 54dae5d — thanks). Given the scope — a new native Bun.serve({fd}) listen path threaded through uws/uws_sys/ServerConfig/mod.rs, plus ~150 lines of new cluster-worker listen logic in _http_server.ts with fd-ownership handoff across several failure modes — a human look is still worthwhile.
What was reviewed:
- fd ownership on every exit path (Bun.serve throws vs. late failure vs. stale reply vs. close/closeAllConnections) — each releases or hands back exactly once
listen_fderrno propagation and the unix-socket unlink guard when the primary owns the filelisten_fd_from_numberbounds (NaN/negative/fractional/overflow) and the http3+fd / allow_hot interactions- IPv6 bracket handling and DNS lookup path against the primary's key format
Extended reasoning...
Overview
This PR routes node:http server listen() in cluster workers through cluster._getServer (like node:net already does) so all workers share the primary's bound socket instead of each binding its own SO_REUSEPORT socket. To make that work it adds an fd-based listen path to Bun.serve: HttpContext::listen_fd → TemplatedApp::listen_fd → uws_app_listen_fd → Rust App::listen_fd → a new ServerConfig.listen_fd field and Addr::Fd arm in mod.rs. The JS side (_http_server.ts) grows ~150 lines: listenInCluster, queryPrimary, releaseClusterHandle, emitListenError, plus a listening-id counter to detect stale primary replies. The primary's mixed-kind EINVAL hint is reworded (two existing test assertions updated). ~540 lines of new tests cover shared port/unix, bind errors, re-listen, disconnect, mixed kinds, IPv6/hostname forms, and the late-failure teardown.
Security risks
Low. The new fd option is gated behind onNodeHTTPRequest so it is not a public Bun.serve surface. listen_fd_from_number rejects NaN, negatives, fractionals, and out-of-range values before casting. The fd comes from the trusted primary over IPC (SCM_RIGHTS), not from untrusted input. No auth/crypto/permission code is touched.
Level of scrutiny
High. This is a cross-cutting Node-compat feature spanning C++ (uws), Rust FFI, native server config, and built-in JS with careful fd-ownership rules across process boundaries. The failure-mode handling (kClusterListeningId staleness check, the handle.adopted toggle depending on whether Bun.serve succeeded, emitListenError calling close() only when a new listener exists) is subtle and easy to get wrong. Windows is deliberately excluded with a stated reason. There is also a design choice — always sharedOnly for http servers rather than round-robin — that a maintainer should sign off on, and it interacts with #34659 which the description notes overlaps on the fd path.
Other factors
Test coverage is thorough (10+ new tests, each described as failing on the unfixed build where applicable), and the existing 80 vendored test-cluster-* tests plus related suites are reported passing. The comment-cop bot flags were all addressed in 7c34434/6255a6a. My prior inline nit (half-bracketed IPv6 host) is fixed in 54dae5d with a test. Nothing outstanding blocks review, but the size and native reach put this well outside auto-approval territory.
Problem
node:clusterworker,http.createServer().listen(0)binds a different ephemeral port in every worker. Node resolveslisten(0)once in the primary, soserver.address().portand everycluster.on("listening")payload report the same port in all workers (3 workers: node prints one port three times, bun prints three different ports; repro below).Server.prototype.listeninsrc/js/node/_http_server.tsnever asked the primary. In a worker it calledBun.serve({ port, reusePort: true })directly (old lines 667-685), so each worker got its ownSO_REUSEPORTsocket and, for port 0, its own kernel-chosen port.node:nethas gone throughcluster._getServersince cluster: port Node's cluster and child_process handle-passing suites (+43 upstream tests; cluster 54 → 85) and implement what they expose — round-robin fd handoff, SCHED_NONE shared handles, UDP clustering, IPC handle passing #31829;node:httpwas left on the direct bind.listen(path)in two workers fails the second one withEADDRINUSE(only one worker can ever serve a unix path), bind errors have Bun.serve's shape (listen EADDRINUSE ... Failed to start server. Is port N in use?, noaddress/port) instead of node'sbind EADDRINUSE 127.0.0.1:N, andworker.disconnect()does not close http servers because the cluster never learns about them, so the worker never exits.Fix
_http_server.ts: a worker'slisten()now goes throughcluster._getServerwith the same(address, port, addressType)querynet.Serversends plussharedOnly: true. The primary binds the address once per key (or reuses the handle it already has for that key) and ships the descriptor; the worker then callsBun.serve({ ..., fd })and accepts on it. A non-IP host is resolved withdns.lookupfirst, likenetand node, and an IPv6 literal in brackets (which Bun.serve accepts) is unbracketed for the query. Bind errors becomeExceptionWithHostPort(err, "bind", address, port)likenet.close()/closeAllConnections()release the handle (act: "close"to the primary); the handle's owner is the server, soworker.disconnect()closes it the way it closesnetservers. A stale reply (server closed or re-listened before the primary answered) hands the descriptor straight back. IfBun.serve()itself throws, the descriptor is closed and the handle released; iflisten()fails afterBun.serve()returned, the listener already owns the descriptor, so the server is torn down throughclose()(which now also applies to a late failure on the other listen paths, instead of leaving a listener running behind the'error').sharedOnlyis correct here for the same reason it is used for TLSnetservers: Bun.serve accepts (and parses) natively, so it can take a listening socket but not the connections the round-robin handle passes one by one. This is node's shared-handle (SCHED_NONE) behavior for http servers; the port/path semantics, which is what this fixes, are identical under both policies.reusePort: trueunlessexclusivewas asked for:exclusive: true/reusePort: true(node binds in the worker too), a process that inheritedNODE_UNIQUE_IDwithout an IPC channel (previous behavior), arguments the primary could not bind (Bun.serve reports them as before), and Windows. On Windows several processes accepting on copies of one listening socket block each other inaccept()(see cluster: fix two ways a Windows worker stops reacting (shared accept() race, early handle ack); fixes test-cluster-shared-leak timeouts #37815), and the round-robin alternative is exactly what Bun.serve cannot take, so Windows keeps the current per-worker bind and thelisten(0)divergence remains there for now.internal/cluster/primary.ts: the hint attached to the EINVAL a shared-only query gets when its key is held by a round-robin handle (and vice versa) only talked about TLS; http/https servers now reach it too (e.g. anetworker and anhttpworker both callinglisten(0)), so it describes the natively accepting kinds vsnetand names the remedies. The two existing assertions on it are updated.Bun.servelearns to accept on an existing descriptor:HttpContext::listen_fd/TemplatedApp::listen_fd/uws_app_listen_fd/App::listen_fdwrap theus_socket_group_listen_fdthatBun.listen({ fd })already uses;ServerConfig.listen_fdis only read for node:http servers (theonNodeHTTPRequestbranch), so it is not a new public option, and it disables--hotserver reuse, which would otherwise drop the descriptor.config.addressstill carries the port/host or unix path, soaddress,url,requestIPand error messages behave as before, andstop_listeningno longer unlinks a unix socket file when the descriptor was inherited (the primary owns the file and unlinks it when the last worker releases the handle, as node does). A descriptor that cannot be listened on throws aSystemErrorwith the real errno (ENOTSOCK,EBADF, ...),EINVALif none is available, and is left open for the caller, matchingus_socket_group_listen_fd's contract.test/js/node/cluster.test.ts: http and https workers share one port forlisten(0)while anexclusiveworker gets its own; two workers share a unix path, the file survives the first worker'sclose()and disappears after the last; a busy port reports node'sbind EADDRINUSE 127.0.0.1:Nshape;close()releases the port so the same worker can listen on it again;worker.disconnect()closes the http server before the channel goes and the worker exits 0. Also: anhttpworker'slisten(0)on a key anetworker's round-robin handle holds gets EINVAL with the new hint;listen(0, "[::1]")binds::1through the primary;listen(0, "localhost")resolves first and reports the address it bound; alisten()that throws afterBun.serve()returned leaves the server not running (close()reportsERR_SERVER_NOT_RUNNING). On the unfixed build the listen(0), unix, bind-error, disconnect, mixed-kind and late-failure tests fail (distinctReportedPorts: 2,EADDRINUSEfor the second unix worker,syscall: "listen",served 200after disconnect, an unexpected'listening',closeError: null); the re-listen,[::1]andlocalhostones guard the release and the address handling.cluster 'listening' reports the address a http server boundtest (127.0.0.1, wildcard, ::1 and a unix path now all take the new path), all 80 vendoredtest-cluster-*.jsplustest-http-server-drop-connections-in-cluster.js,test-cluster-http-pipe.js,test-tls-ticket-cluster.jsand the fork-net tests (all exit 0),test/js/node/cluster/test-docs-http-server.ts(every CPU's worker on one fixed port) andtest-worker-no-exit-http.ts,test/js/node/http/node-http.test.ts,test/js/bun/http/serve-listen.test.ts;cargo check -p bun_runtime --target x86_64-pc-windows-msvc.Bun.servefd path (forlisten({ fd })) but keepslisten(0)on the direct bind; node:cluster: give every worker the same port for listen(0) #33025 and node:cluster: fix net.Server worker EADDRINUSE and port:0 divergence #30603 were the pre-cluster: port Node's cluster and child_process handle-passing suites (+43 upstream tests; cluster 54 → 85) and implement what they expose — round-robin fd handoff, SCHED_NONE shared handles, UDP clustering, IPC handle passing #31829 attempts at this; node:cluster: let workers shut down gracefully on worker.disconnect() #30548's "disconnect() with an http server never exits" case is covered here as a side effect, its other cases are not. RawBun.servein workers (Bun.serve and node:cluster don't round-robin http requests when listening on unix domain socket #13611, Bun dont work with cluster #14727) is unchanged.Background
server.listen()does not bind. It sends the primary aqueryServermessage keyed byaddress:port:addressType:fd(plus a per-listen index when the port is 0, which is why every worker's firstlisten(0)lands on the same key). The primary answers from a handle object it keeps per key.SCHED_RR): the primary listens, accepts, and sends each accepted connection to a worker over the IPC channel; the worker's server sits on a faux handle. Shared handle (SCHED_NONE): the primary only binds, sends the bound descriptor to each worker, and every worker callslisten(2)and accepts on its own copy.sharedOnlyin the query forces the shared kind; Bun uses it for TLSnetservers because their accept is native. The descriptor crosses the channel asSCM_RIGHTSancillary data and arrives inchild.tsashandle.sharedFd;handle.adoptedtells itsclose()that a listener owns the fd now and it must only notify the primary.us_socket_group_listen_fd(usockets) turns an already bound descriptor into a listen socket:listen(2), non-blocking, register with the event loop. On success the listen socket owns the fd; on failure the caller still does.Bun.listen({ fd })is built on it; this PR gives the uws HTTP app (whatBun.serveis built on) the same entry point.ServerConfig.addressis whatBun.servereports and formats (server.address,server.url, EADDRINUSE text, unix-vs-tcp decisions such asrequestIP); the listen socket's real port is read back from the socket, so with an inherited descriptor the configured port 0 is reported as the shared port.Repro and before/after output
Bind error shape for a port the primary cannot bind, worker side:
Native failure paths of the new fd option (internal, needs
onNodeHTTPRequest): a regular file reportsENOTSOCK: socket operation on non-socket, listenand stays open, fd 9999 reportsEBADF,-1/1.5are rejected while parsing options, and a plainBun.serve({ fd })ignores the key.