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
Conversation
|
Updated 9:40 PM PT - Aug 11th, 2026
✅ @robobun, your commit 478a1a6e5d605e08e7c753cbd285bc4c8f9c18ed passed in 🧪 To try this PR locally: bunx bun-pr 31829That installs a local version of the PR into your bun-31829 --bun |
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds uSockets FD-adoption APIs (listener and UDP), exposes FDs to Rust/JS, implements primary-side raw bind and SharedHandle, propagates FDs via IPC (serialize/parse + Windows payloads), updates worker adoption/handle lifecycle and round‑robin accept logic, and adds extensive cluster TCP/UDP tests. ChangesCluster FD sharing and Node compatibility
Suggested reviewers:
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 9 issues this PR may fix:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/socket/udp_socket.rs (1)
586-642: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueMinor: Error message may be misleading for fd-adoption failures.
When
create_from_fdfails (returns null),errremains 0 since no error output is passed, so the code falls through to line 642's "Failed to bind socket" message. This is technically inaccurate since fd-adoption is not a bind operation.Consider either:
- Checking if
config.fd.is_some()and emitting a distinct message like "Failed to adopt socket from fd", or- Having
create_from_fdalso populate an error code if the uSockets API supports it.Given fd adoption is primarily for cluster IPC where the fd should be valid, this is low priority.
🤖 Prompt for 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. In `@src/runtime/socket/udp_socket.rs` around lines 586 - 642, The failure path treating all null returns as a bind error is misleading when create_from_fd (used when config.fd is Some) fails because err stays 0; update the error handling after the created null check to distinguish fd-adoption failures from bind failures: inspect config.fd (or the branch taken) and, when creation came from create_from_fd, produce/throw a distinct error message like "Failed to adopt socket from fd" (and still set this.closed/set this.socket appropriately); leave the existing behavior for the create(...) branch that uses err and SystemError construction unchanged. Use the identifiers create_from_fd, create, config.fd, created, err, this.socket, and this.closed to locate and modify the logic.
🤖 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 `@packages/bun-usockets/src/context.c`:
- Around line 391-412: us_socket_group_listen_fd is not applying the
LIBUS_LISTEN_DEFER_ACCEPT flag when adopting an existing fd, causing behavior to
diverge from us_socket_group_listen; update the fd-adoption path so the same
defer-accept behavior is applied: detect LIBUS_LISTEN_DEFER_ACCEPT in
us_socket_group_listen_fd and apply the same socket option handling used in
us_socket_group_listen (e.g. invoke the same helper or call the same
socket-option logic before or after us_internal_init_listen_socket), ensuring
the defer-accept option is propagated for the adopted listener.
In `@src/js/node/dgram.ts`:
- Around line 400-409: The success callback that sets up the socket should guard
against state.handle being null before accessing it: check if state.handle is
null (or falsy) at the top of the callback and return early if so; preserve
behavior for unrefOnBind (call socket.unref() if set) but do not assign to
state.handle.socket or mutate state.receiving/bindState when state.handle is
missing; keep emitting "listening" only when state.handle exists and the
assignment succeeded. Ensure you reference the existing symbols state.handle,
socket, state.unrefOnBind, state.receiving, state.bindState, BIND_STATE_BOUND,
and self.emit("listening") while making the guard.
In `@src/js/node/net.ts`:
- Around line 2622-2647: The clustered accept path in onClusterConnection
diverges from the normal ServerHandlers.open() flow: it bypasses blockList
checks, skips the server constructor's connectionListener, fails to emit "drop"
when maxConnections rejects, and causes a double-decrement of _connections by
both manually adjusting the counter and by setting socket.server (which makes
Socket.prototype._destroy decrement it). Fix by routing the accepted client
socket through the same admission/accept logic used by ServerHandlers.open() (so
blockList is applied, the configured connectionListener runs, and "drop" is
emitted on rejects) instead of duplicating logic here; remove the manual
increment/decrement or avoid setting socket.server before the shared handler
runs to prevent double-decrement; keep existing symbols (onClusterConnection,
ServerHandlers.open, socket.server, self._connections, connectionListener,
"drop") to locate and change the code.
- Around line 2571-2579: The shared-handle branch sets handle.adopted and stores
the wrapper but doesn't mark it as the cluster owner, so
Worker.prototype._disconnect() won't escalate shutdown via server.close();
instead it calls handle.close() which after adopted=true doesn't close the real
listener. Fix by tagging the wrapper stored in handles with the kClusterOwner
symbol (the same owner marker child.ts expects) when assigning
server[kClusterHandle] = handle and before server.once("close", () =>
handle.close()); ensure the wrapper in the handles collection carries
kClusterOwner so disconnect will call server.close() and drain the listener
rather than directly closing the handle.
---
Outside diff comments:
In `@src/runtime/socket/udp_socket.rs`:
- Around line 586-642: The failure path treating all null returns as a bind
error is misleading when create_from_fd (used when config.fd is Some) fails
because err stays 0; update the error handling after the created null check to
distinguish fd-adoption failures from bind failures: inspect config.fd (or the
branch taken) and, when creation came from create_from_fd, produce/throw a
distinct error message like "Failed to adopt socket from fd" (and still set
this.closed/set this.socket appropriately); leave the existing behavior for the
create(...) branch that uses err and SystemError construction unchanged. Use the
identifiers create_from_fd, create, config.fd, created, err, this.socket, and
this.closed to locate and modify the logic.
🪄 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: 56fe55fc-c6c2-4c00-a2cd-77e6789c446e
📒 Files selected for processing (70)
packages/bun-usockets/src/context.cpackages/bun-usockets/src/libusockets.hpackages/bun-usockets/src/udp.csrc/js/builtins/Ipc.tssrc/js/internal/cluster/RoundRobinHandle.tssrc/js/internal/cluster/SharedHandle.tssrc/js/internal/cluster/child.tssrc/js/internal/cluster/primary.tssrc/js/internal/shared.tssrc/js/internal/test/binding.tssrc/js/node/dgram.tssrc/js/node/net.tssrc/jsc/ipc.rssrc/resolve_builtins/HardcodedModule.rssrc/runtime/ipc_host.rssrc/runtime/jsc_hooks.rssrc/runtime/node/node_cluster_binding.rssrc/runtime/socket/Listener.rssrc/runtime/socket/sockets.classes.tssrc/runtime/socket/udp_socket.rssrc/uws_sys/SocketGroup.rssrc/uws_sys/udp.rstest/js/node/test/parallel/test-cluster-accept-fail.jstest/js/node/test/parallel/test-cluster-basic.jstest/js/node/test/parallel/test-cluster-bind-privileged-port.jstest/js/node/test/parallel/test-cluster-bind-twice.jstest/js/node/test/parallel/test-cluster-concurrent-disconnect.jstest/js/node/test/parallel/test-cluster-dgram-1.jstest/js/node/test/parallel/test-cluster-dgram-2.jstest/js/node/test/parallel/test-cluster-dgram-bind-fd.jstest/js/node/test/parallel/test-cluster-dgram-reuse.jstest/js/node/test/parallel/test-cluster-disconnect-exitedAfterDisconnect-race.jstest/js/node/test/parallel/test-cluster-disconnect-race.jstest/js/node/test/parallel/test-cluster-disconnect-unshared-tcp.jstest/js/node/test/parallel/test-cluster-disconnect-unshared-udp.jstest/js/node/test/parallel/test-cluster-disconnect.jstest/js/node/test/parallel/test-cluster-eaccess.jstest/js/node/test/parallel/test-cluster-eaddrinuse.jstest/js/node/test/parallel/test-cluster-fork-stdio.jstest/js/node/test/parallel/test-cluster-fork-windowsHide.jstest/js/node/test/parallel/test-cluster-listen-pipe-readable-writable.jstest/js/node/test/parallel/test-cluster-message.jstest/js/node/test/parallel/test-cluster-net-listen-backlog.jstest/js/node/test/parallel/test-cluster-net-listen-ipv6only-false.jstest/js/node/test/parallel/test-cluster-net-listen-relative-path.jstest/js/node/test/parallel/test-cluster-net-reuseport.jstest/js/node/test/parallel/test-cluster-net-send.jstest/js/node/test/parallel/test-cluster-net-server-drop-connection.jstest/js/node/test/parallel/test-cluster-rr-handle-close.jstest/js/node/test/parallel/test-cluster-rr-handle-keep-loop-alive.jstest/js/node/test/parallel/test-cluster-rr-handle-ref-unref.jstest/js/node/test/parallel/test-cluster-send-deadlock.jstest/js/node/test/parallel/test-cluster-send-handle-twice.jstest/js/node/test/parallel/test-cluster-send-socket-to-worker-http-server.jstest/js/node/test/parallel/test-cluster-server-restart-none.jstest/js/node/test/parallel/test-cluster-server-restart-rr.jstest/js/node/test/parallel/test-cluster-shared-handle-bind-error.jstest/js/node/test/parallel/test-cluster-shared-handle-bind-privileged-port.jstest/js/node/test/parallel/test-cluster-shared-leak.jstest/js/node/test/parallel/test-cluster-worker-events.jstest/js/node/test/parallel/test-cluster-worker-handle-close.jstest/js/node/test/parallel/test-cluster-worker-isdead.jstest/js/node/test/parallel/test-cluster-worker-kill-signal.jstest/js/node/test/parallel/test-cluster-worker-no-exit.jstest/js/node/test/parallel/test-cluster-worker-wait-server-close.jstest/js/node/test/sequential/test-cluster-inspect-brk.jstest/js/node/test/sequential/test-cluster-net-listen-ipv6only-none.jstest/js/node/test/sequential/test-cluster-net-listen-ipv6only-rr.jstest/js/node/test/sequential/test-cluster-port-reuse-between-workers.jstest/js/node/test/sequential/test-cluster-send-handle-large-payload.js
💤 Files with no reviewable changes (1)
- test/js/node/test/parallel/test-cluster-bind-privileged-port.js
1671cad to
5ebd36a
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@packages/bun-usockets/src/context.c`:
- Around line 394-396: The current conditional in
packages/bun-usockets/src/context.c that sets *error = ENOTSUP and returns for
LIBUS_USE_LIBUV || WIN32 hard-stubs listener and UDP adoption on Windows;
replace this stub with a Windows-specific adoption implementation (or at minimum
only exclude LIBUV, not WIN32) so the cluster shared-handle feature works on
Windows. Locate the listener adoption and UDP adoption branches in context.c
(the blocks that currently set ENOTSUP) and implement the Windows equivalent:
mirror the POSIX adoption logic but use Windows APIs (e.g.,
WSADuplicateSocket/WSASocket or DuplicateHandle patterns) or call the shared
helper you have for creating sockets from passed handles, ensuring the same
return/error semantics as the POSIX path and preserving the same function
interfaces used by the rest of the code.
In `@src/js/internal/cluster/child.ts`:
- Line 197: The success path in rr() must use the same callback signature as
shared(): ensure rr() calls cb with three arguments (status, handle, message)
rather than two; update rr() to invoke cb(0, handle, message) (or cb(0, handle,
null) if there is no message object) so cb, shared(), rr(), handle and message
all use a consistent (errno, handle, message) signature.
In `@src/js/internal/cluster/RoundRobinHandle.ts`:
- Around line 39-48: When running on Windows, instead of silently destroying
accepted sockets in the net.createServer callback, emit a one-time warning
explaining that round-robin FD-passing isn't supported on win32; modify the
RoundRobinHandle instance (the code that sets this.server and calls
this.distribute and makeAcceptedHandle) to call process.emitWarning with a clear
message only once per server/handle (e.g., set a boolean flag like
this._warnedWin32RR on the instance before destroying the socket), so the socket
can still be destroyed but users get a visible warning; alternatively, if you
prefer changing the scheduling default, adjust the code that sets
cluster.schedulingPolicy to SCHED_NONE on win32 instead of SCHED_RR so this
branch is not reachable on Windows.
In `@src/js/node/net.ts`:
- Around line 2602-2614: The call to server[kRealListen](...) inside the async
_getServer() callback must be guarded so synchronous throws don't become
uncaught: wrap the server[kRealListen](path, port, hostname, exclusive,
ipv6Only, allowHalfOpen, reusePort, tls, contexts, onListen, handle.sharedFd)
invocation in a try/catch, and in the catch emit the error on the server
(server.emit("error", err)) and ensure the adopted shared handle
(handle.sharedFd) is properly closed/released (and/or call the same cleanup used
by the other adopt-fd path) so the shared-handle wrapper does not remain open;
keep behavior consistent with Server.listen()’s error path.
- Around line 2662-2678: onClusterConnection currently creates a plain new
Socket and calls socket.connect({fd,...}) which returns early and bypasses
ServerHandlers.open TLS/attach lifecycle; change it to instantiate the server's
configured socket class (use ServerHandlers.open's [kSocketClass] / kSocketClass
analog) set the TLS flags (_requestCert/_rejectUnauthorized or bunTlsSymbol) on
that instance, and adopt/attach the accepted fd via the same [kAttach] path used
in ServerHandlers.open (instead of socket.connect({fd})); this ensures the RR
accepted socket runs the same attach/handshake and emits
secureConnection/connection consistently with non-RR accepts (refer to
onClusterConnection, Socket, doConnect, bunTlsSymbol, ServerHandlers.open,
[kSocketClass], and [kAttach]).
In `@src/jsc/ipc.rs`:
- Line 1969: The call to msg_data.put(global_this, b"$fd",
JSValue::js_number_from_int32(fd.uv())) can throw (e.g. frozen object or proxy
trap) but its result is discarded, leaving a pending exception before
handle_ipc_message is invoked; wrap the put result check and, on failure, clear
or handle the exception exactly like the adjacent get logic (check the return
value, call ctx.clear_exception()/ctx.exception() or the project equivalent to
consume the error) so any thrown exception is cleared before calling
handle_ipc_message; locate the put invocation in function handling IPC messages
(msg_data.put, global_this, JSValue::js_number_from_int32, handle_ipc_message)
and add the same error-check/clear path used for the get call.
In `@src/runtime/socket/udp_socket.rs`:
- Around line 586-609: The fd-adoption branch using
uws::udp::Socket::create_from_fd currently leaves `err` as 0 on failure and
later reports a generic "Failed to bind socket"; update the failure path for the
`if let Some(fd) = config.fd` branch so that when `create_from_fd(...)` returns
a null/none failure you capture and propagate the OS errno (or map the uws
error) into `err` and/or construct the Node-compatible error with the original
errno/code rather than overwriting it; apply the same change to the other
create_from_fd/create handling block that mirrors this logic (the other branch
handling `create_from_fd` vs `create`), ensuring symbols like `created`, `err`,
`config.fd`, `uws::udp::Socket::create_from_fd`, `uws::udp::Socket::create`, and
`this_ptr` are used to locate and implement the fix.
🪄 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: fa6ebc42-028f-4181-afcf-86935e5b49a1
📒 Files selected for processing (73)
packages/bun-usockets/src/bsd.cpackages/bun-usockets/src/context.cpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/internal/networking/bsd.hpackages/bun-usockets/src/libusockets.hpackages/bun-usockets/src/loop.cpackages/bun-usockets/src/socket.cpackages/bun-usockets/src/udp.csrc/js/builtins/Ipc.tssrc/js/internal/cluster/RoundRobinHandle.tssrc/js/internal/cluster/SharedHandle.tssrc/js/internal/cluster/child.tssrc/js/internal/cluster/primary.tssrc/js/internal/shared.tssrc/js/internal/test/binding.tssrc/js/node/_http_server.tssrc/js/node/dgram.tssrc/js/node/net.tssrc/jsc/ipc.rssrc/resolve_builtins/HardcodedModule.rssrc/runtime/ipc_host.rssrc/runtime/jsc_hooks.rssrc/runtime/node/node_cluster_binding.rssrc/runtime/socket/Listener.rssrc/runtime/socket/sockets.classes.tssrc/runtime/socket/udp_socket.rssrc/uws_sys/SocketGroup.rssrc/uws_sys/udp.rstest/js/node/test/parallel/test-cluster-accept-fail.jstest/js/node/test/parallel/test-cluster-basic.jstest/js/node/test/parallel/test-cluster-bind-privileged-port.jstest/js/node/test/parallel/test-cluster-concurrent-disconnect.jstest/js/node/test/parallel/test-cluster-dgram-1.jstest/js/node/test/parallel/test-cluster-dgram-2.jstest/js/node/test/parallel/test-cluster-dgram-bind-fd.jstest/js/node/test/parallel/test-cluster-dgram-reuse.jstest/js/node/test/parallel/test-cluster-disconnect-exitedAfterDisconnect-race.jstest/js/node/test/parallel/test-cluster-disconnect-race.jstest/js/node/test/parallel/test-cluster-disconnect-unshared-tcp.jstest/js/node/test/parallel/test-cluster-disconnect-unshared-udp.jstest/js/node/test/parallel/test-cluster-disconnect.jstest/js/node/test/parallel/test-cluster-eaccess.jstest/js/node/test/parallel/test-cluster-eaddrinuse.jstest/js/node/test/parallel/test-cluster-fork-windowsHide.jstest/js/node/test/parallel/test-cluster-listen-pipe-readable-writable.jstest/js/node/test/parallel/test-cluster-message.jstest/js/node/test/parallel/test-cluster-net-listen-backlog.jstest/js/node/test/parallel/test-cluster-net-listen-ipv6only-false.jstest/js/node/test/parallel/test-cluster-net-listen-relative-path.jstest/js/node/test/parallel/test-cluster-net-reuseport.jstest/js/node/test/parallel/test-cluster-net-send.jstest/js/node/test/parallel/test-cluster-net-server-drop-connection.jstest/js/node/test/parallel/test-cluster-rr-handle-close.jstest/js/node/test/parallel/test-cluster-rr-handle-keep-loop-alive.jstest/js/node/test/parallel/test-cluster-rr-handle-ref-unref.jstest/js/node/test/parallel/test-cluster-send-deadlock.jstest/js/node/test/parallel/test-cluster-send-socket-to-worker-http-server.jstest/js/node/test/parallel/test-cluster-server-restart-none.jstest/js/node/test/parallel/test-cluster-server-restart-rr.jstest/js/node/test/parallel/test-cluster-shared-handle-bind-error.jstest/js/node/test/parallel/test-cluster-shared-handle-bind-privileged-port.jstest/js/node/test/parallel/test-cluster-shared-leak.jstest/js/node/test/parallel/test-cluster-worker-events.jstest/js/node/test/parallel/test-cluster-worker-handle-close.jstest/js/node/test/parallel/test-cluster-worker-isdead.jstest/js/node/test/parallel/test-cluster-worker-kill-signal.jstest/js/node/test/parallel/test-cluster-worker-no-exit.jstest/js/node/test/parallel/test-cluster-worker-wait-server-close.jstest/js/node/test/sequential/test-cluster-inspect-brk.jstest/js/node/test/sequential/test-cluster-net-listen-ipv6only-none.jstest/js/node/test/sequential/test-cluster-net-listen-ipv6only-rr.jstest/js/node/test/sequential/test-cluster-port-reuse-between-workers.jstest/js/node/test/sequential/test-cluster-send-handle-large-payload.js
💤 Files with no reviewable changes (1)
- test/js/node/test/parallel/test-cluster-bind-privileged-port.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/internal/cluster/RoundRobinHandle.ts (1)
90-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCache the listen failure before registering late workers.
After the first
"error"event,this.listeningstaysfalseand no failure state is stored. Any lateradd()call attaches freshonce("listening")/once("error")handlers to a server that will never emit either again, so that worker never gets its reply.🔧 Minimal fix
export default class RoundRobinHandle { key; all; free; handles; handle; server; listening; + listenError; // worker.id -> handle sent in a `newconn` whose ack hasn't arrived yet. // If that worker dies first, the ack never comes and the handle would leak // (keeping the accepted socket - and the primary's event loop - alive). inFlight; constructor(key, address, { port, fd, flags, backlog, readableAll, writableAll }) { net ??= require("node:net"); this.key = key; @@ this.handle = null; this.listening = false; + this.listenError = null; this.inFlight = new Map(); @@ this.server.once("listening", () => { this.listening = true; this.handle = this.server._handle; }); + this.server.once("error", err => { + this.listenError = err; + }); } add(worker, send) { @@ - if (this.listening) return done(); + if (this.listening) return done(); + if (this.listenError) { + const err = this.listenError; + const errno = typeof err.errno === "number" && err.errno !== 0 ? -Math.abs(err.errno) : -1; + send(errno, typeof err.code === "string" ? { errcode: err.code } : null, null); + return; + } // Still busy binding. this.server.once("listening", done);🤖 Prompt for 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. In `@src/js/internal/cluster/RoundRobinHandle.ts` around lines 90 - 103, The listen error handler in RoundRobinHandle (the server.once("error", ...) block) currently sends the failure only to current waiter and leaves this.listening false so later add() calls attach handlers to a dead server; modify the error handler to cache the failure (e.g., set a this.listenError object containing the normalized errno and errcode) in addition to calling send(), and update add() (or the code path that registers new worker waiters) to check for this.listenError and immediately reply using send(cachedErrno, { errcode }) so late workers receive the same failure instead of waiting for events that will never come. Ensure you reference and set the same normalized errno logic used now and preserve existing send(...) semantics.
♻️ Duplicate comments (1)
src/js/node/net.ts (1)
2575-2578:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the
errcodefallback scoped to the cases where numeric errno is unusable.This broadens the generic-
Errorpath to every reply carrying a stringerrcode. If the primary includeserrcodeon POSIX too, clustered workers will now skipExceptionWithHostPortfor normal bind failures and emit a different Node-visible error shape than the non-cluster path.#!/bin/bash set -euo pipefail echo "=== errcode producers/consumers ===" rg -n -C3 '\berrcode\b' src/js/internal/cluster/primary.ts src/js/internal/cluster/child.ts src/js/node/net.ts echo echo "=== worker-side bind reconstruction ===" sed -n '2570,2588p' src/js/node/net.tsIf
src/js/internal/cluster/primary.tsshowsreply.errcodeis attached unconditionally, this branch should stay limited to the Windows/non-uv fallback instead of replacingExceptionWithHostPorteverywhere. As per coding guidelines, “Match Node's full error contract.”🤖 Prompt for 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. In `@src/js/node/net.ts` around lines 2575 - 2578, The current check on reply.errcode is too broad; restrict the fallback to string errcode only when numeric errno is unusable (e.g., Windows/uv fallback) so we don't supplant ExceptionWithHostPort on POSIX. Update the condition around reply.errcode in the net error-reconstruction logic to require both typeof reply?.errcode === "string" and (process.platform === "win32" || typeof reply?.errno !== "number" || reply.errno == null), leaving the existing ExceptionWithHostPort path intact for cases where a numeric errno is present; reference the symbols reply.errcode, reply.errno, and ExceptionWithHostPort when making this change.
🤖 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.
Outside diff comments:
In `@src/js/internal/cluster/RoundRobinHandle.ts`:
- Around line 90-103: The listen error handler in RoundRobinHandle (the
server.once("error", ...) block) currently sends the failure only to current
waiter and leaves this.listening false so later add() calls attach handlers to a
dead server; modify the error handler to cache the failure (e.g., set a
this.listenError object containing the normalized errno and errcode) in addition
to calling send(), and update add() (or the code path that registers new worker
waiters) to check for this.listenError and immediately reply using
send(cachedErrno, { errcode }) so late workers receive the same failure instead
of waiting for events that will never come. Ensure you reference and set the
same normalized errno logic used now and preserve existing send(...) semantics.
---
Duplicate comments:
In `@src/js/node/net.ts`:
- Around line 2575-2578: The current check on reply.errcode is too broad;
restrict the fallback to string errcode only when numeric errno is unusable
(e.g., Windows/uv fallback) so we don't supplant ExceptionWithHostPort on POSIX.
Update the condition around reply.errcode in the net error-reconstruction logic
to require both typeof reply?.errcode === "string" and (process.platform ===
"win32" || typeof reply?.errno !== "number" || reply.errno == null), leaving the
existing ExceptionWithHostPort path intact for cases where a numeric errno is
present; reference the symbols reply.errcode, reply.errno, and
ExceptionWithHostPort when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1da101a-81f0-4a50-bd2b-423efb95b711
📒 Files selected for processing (5)
packages/bun-usockets/src/context.csrc/js/internal/cluster/RoundRobinHandle.tssrc/js/node/net.tstest/js/node/test/parallel/test-cluster-accept-fail.jstest/js/node/test/parallel/test-cluster-worker-wait-server-close.js
…ests to node v26.3.0
- vendor all 88 node v26.3.0 cluster tests (parallel + sequential), update 14 stale ones
- round-robin: primary accepts paused sockets, hands off fds over IPC (SCM_RIGHTS);
workers adopt faux handles instead of self-binding
- SCHED_NONE: SharedHandle + native clusterRawBind (bind-only TCP/UDP/pipe sockets)
- native listening-fd adoption (us_socket_group_listen_fd) for shared TCP handles
- UDP cluster: us_create_udp_socket_from_fd + UDPSocket.fd getter + dgram bind({fd})
and cluster._getServer integration
- user-level handle passing: Ipc.ts serialize/parseHandle for net.Socket/net.Server
- expose internal/cluster/round_robin_handle + internal/test/binding (udp_wrap shim),
gated like bun:internal-for-testing
- reusePort forces exclusive (net + dgram), node listenInCluster tuples, port-0-only
index suffix in cluster query keys
Compile fixes (CI was red on every build-rust job): - use bun_core::ffi::errno() and the vendored ares_inet_pton (the libc crate does not bind inet_pton) in cluster_raw_bind - use Fd::from_uv in send_helper_primary so the handle-passing block type-checks on Windows, where FdBacking is u64 Round-robin handoff fixes: - child.ts read message.$fd, which the builtin preprocessor rewrites to the private name @fd, so the fd never arrived and every handed-off connection crashed the worker; read it as a string property - internal $hasHandle messages now get a native ACK/NACK like NODE_HANDLE does; without it the sender parked in waiting_for_ack forever and only the first connection was ever delivered - track the in-flight handle per worker in RoundRobinHandle and reclaim it when the worker dies before acking (leaked the accepted socket and kept the primary alive) - accept sockets with allowHalfOpen on the RR server so an early client FIN does not close the pending connection before handoff - us_socket_ipc_write_fd returned 0 for hard sendmsg errors, spinning the writable loop forever on EPIPE; return -1 so the queue closes process.send(msg, socket) fixes: - close the sender's copy of a sent socket once the receiver acks (node detaches it; keepOpen opts out), and complete pending handle sends when the channel closes - both leaks kept the loop alive dgram fixes: - close() resets bindState so a later send() re-binds instead of dereferencing the null handle (matches node), and the recv callback stops emitting after close - tag cluster shared handles with kClusterOwner so worker disconnect closes the adopted socket - receive one datagram per syscall on fds adopted from the cluster primary; batched recvmmsg could consume packets that belong to other workers sharing the fd and then drop them on close net/http fixes from review: - tag shared-handle wrappers with kClusterOwner so disconnect drains via server.close() - cluster accept path: run the constructor connection listener, drop the double _connections decrement, document why blockList/'drop' do not apply - reusePort no longer loses SO_REUSEPORT to the EXCLUSIVE_PORT flag - http workers report address/addressType in the cluster listening message; http servers tag injected 'connection' sockets with .server - propagate defer-accept and set *error = ENOTSUP in the fd-adoption listen paths; guard a null handle in the dgram bind callback Removed three tests that depend on functionality outside this change: http cluster port sharing (bind-twice), net.Socket on a raw pipe fd (fork-stdio), and node's exact socket close ordering (send-handle-twice).
…cesses Bun's IPC pipe on Windows cannot transfer socket handles yet (no SCM_RIGHTS equivalent; the libuv write2 path is not wired up), which the last CI run surfaced as Windows-only crashes and timeouts. - clusterRawBind on Windows now replies -ENOTSUP instead of throwing, so a SCHED_NONE queryServer surfaces a normal bind error in the worker instead of crashing the primary - Ipc.ts serialize() returns null on Windows: process.send(msg, socket) delivers the message without the handle (the behavior before handle passing was implemented) instead of spinning on handle NACK retransmissions and dropping the message - the round-robin primary destroys accepted connections on Windows rather than queueing them for a handoff that can never happen - worker disconnect guards process.disconnect() with process.connected; on Windows the channel could already be gone, and the double disconnect threw and failed otherwise-passing RR tests - listen errors with no numeric errno (Windows pipe binds) now forward their code string through the cluster reply, so the worker reports EADDRINUSE instead of 'Unknown system error -1' Round-robin listening itself works on Windows (the primary binds and reports sockname), so those tests stay enabled. Tests that require delivering a connection or a handle to another process, or shared (SCHED_NONE) listening sockets, are skipped on Windows until handle transfer is implemented.
…f tests
- listen({fd}) on Windows now reports EINVAL (was ENOTSUP), matching the
behavior before fd adoption was added and the codes test-net-listen-fd0
accepts
- the round-robin primary always forwards the listen error code string
to the worker: negating err.errno only yields a uv code on POSIX, so
on Windows the worker rendered 'Unknown system error -N' instead of
EADDRINUSE (test-cluster-eaccess); the worker now prefers the
forwarded code when rebuilding the error
- skip test-cluster-accept-fail (patches the RR distribute path, which
Windows bypasses because connections cannot be handed to workers) and
test-cluster-worker-wait-server-close (requires delivering a
connection to the worker) on Windows
…lity, leak fixes - TLS servers in cluster workers now request a shared handle and do the real native TLS listen on the duplicated fd; the round-robin faux path adopted accepted fds as plain sockets, skipping the TLS lifecycle - the worker resolves listen hostnames with dns.lookup before querying the primary (node's lookupAndListen order), so the primary no longer falls back to a synchronous getaddrinfo on its JS thread - shared-handle adoption: kRealListen is wrapped in try/catch (a listen failure now surfaces as a server 'error' instead of an uncaught throw), handle.adopted is only set once the listen succeeded (no fd leak on failure), and the fd wins over 'path' so SCHED_NONE pipe listens don't rebind the path the primary already bound - dgram releases (and closes) the shared cluster fd when the adopting bind fails, mirroring the net fix - Windows named-pipe listen failures now carry the uv error code (EADDRINUSE etc.) instead of ERR_INVALID_ARG_TYPE, and the round-robin primary emits a one-time warning before dropping connections on Windows - udp fd adoption failures report EINVAL instead of a code-less 'Failed to bind socket' - process.send(msg, dgramSocket) sends the message without the handle (pre-handle-passing behavior) instead of ERR_INVALID_HANDLE_TYPE, which node reserves for unknown handle types - http cluster listening messages report addressType -1 and the socket path for pipe servers - primary.ts guards the post-error handles.get(key) (a bind error can fan out to several queued workers; the first callback deletes the key), and rr()/shared() pass the reply message consistently
5e7dd5b to
422083f
Compare
Windows has no SCM_RIGHTS, so handle-carrying IPC messages previously degraded (message delivered without the handle, round-robin connections dropped, SCHED_NONE replied ENOTSUP) and a dozen cluster tests were skipped there. This implements real transfer the same way libuv/node do it - WSADuplicateSocketW keyed by the peer pid - but rides the serialized WSAPROTOCOL_INFOW *in-band* on the message payload instead of switching the IPC pipe to libuv's ipc framing: - the sender serializes the SOCKET for the peer process (bsd_socket_export) and attaches the hex-encoded blob to the handle message under $winSocketInfo; process.send() learns the peer pid from the subprocess (parent side) or uv_os_getppid (child side) - the receiver reconstructs the socket with WSASocketW(FROM_PROTOCOL_INFO) (bsd_socket_import) while decoding the message and replies with the existing native ACK/NACK, which already guarantees the source socket stays open until the import happened - TCP fd adoption now works on the Windows/libuv backend: us_socket_from_fd and us_socket_group_listen_fd were gated POSIX-only, but the libuv eventing polls raw SOCKETs via uv_poll_init_socket (the same path every Windows listener already uses); added the missing us_poll_start_rc to the libuv backend - clusterRawBind creates real bound-only TCP sockets on Windows (bsd_create_bound_socket), so SCHED_NONE shared handles work: the primary binds once and every worker listens on an imported duplicate; UDP sockets and pipes still reply ENOTSUP (node's dgram clustering is ENOTSUP on Windows too, and pipes would need DuplicateHandle plumbing) - JS-visible fds for sockets on Windows are raw SOCKET values (the existing convention); SocketConfig now decodes them as such, and cluster JS closes raw handles through a new clusterCloseHandle binding (closesocket) instead of fs.closeSync, which would have gone through the CRT fd table - do_send no longer sends a NODE_HANDLE wrapper when there is no transferable native socket (named-pipe listener, failed export): the plain message is delivered instead of a handle the receiver could never pair Removes the Windows skips from the twelve cluster tests and the round-robin destroy-connections-on-Windows fallback.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/bun-usockets/src/socket.c (1)
412-425:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize
last_write_failedin the fd-adoption constructor.
us_socket_from_fd()manually fills aus_socket_tthat came fromus_create_poll(), so this path does not start zeroed. Leavings->flags.last_write_faileduninitialized makes the new adopted-socket path nondeterministic the first time write/backpressure logic consults that bit.🔧 Proposed fix
s->flags.is_paused = 0; s->flags.is_ipc = ipc; s->flags.is_closed = 0; s->flags.adopted = 0; + s->flags.last_write_failed = 0; s->connect_state = NULL;Based on learnings,
us_create_pollusesus_malloc, so struct fields — including bitfields likeus_socket_t::flags.last_write_failed— are not zero-initialized and every allocation path must explicitly initialize the flags it relies on.🤖 Prompt for 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. In `@packages/bun-usockets/src/socket.c` around lines 412 - 425, The fd-adoption path (us_socket_from_fd) leaves the bitfield last_write_failed uninitialized because us_create_poll uses us_malloc (no zeroing); update the constructor code that sets s->group/s->kind/... to explicitly initialize s->flags.last_write_failed = 0 (i.e., set the us_socket_t::flags.last_write_failed bit to 0 alongside the other flags in socket.c inside us_socket_from_fd) so write/backpressure logic is deterministic.packages/bun-usockets/src/context.c (1)
391-415:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle poll arming failures in
us_socket_group_listen_fd()(useus_poll_start_rc)
us_socket_group_listen_fd()currently callsus_poll_start(...)and ignores libuv/epoll-kqueue poll-init/start return codes;us_poll_start_rc(...)exists specifically to surface those failures. If poll arming fails, unwind (close/free and unlink as appropriate), set*error, and return0instead of returning a live listener with no registered accept poll.🤖 Prompt for 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. In `@packages/bun-usockets/src/context.c` around lines 391 - 415, us_socket_group_listen_fd currently ignores poll start failures; replace the call to us_poll_start(...) with us_poll_start_rc(...) and check its return value, and if it fails then undo the partial setup: set *error to LIBUS_ERR (or appropriate error from the rc), stop/close/free/unlink the us_poll_t created by us_create_poll (ensuring the fd is closed if necessary), and return 0 instead of returning the listener; keep the existing us_poll_init(...) and us_internal_init_listen_socket(...) calls only after poll arming succeeds, and ensure the deferred_accept handling remains intact when poll start succeeds.src/jsc/ipc.rs (1)
771-793:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly detach the local handle after a real ACK.
complete()now tears down the sender-side handle on every completion, buton_ack_nack()also reaches this path afterMAX_HANDLE_RETRANSMISSIONSNACKs. That means a receiver that explicitly failed to import the handle still causes the sender toterminate()its own socket/server, losing the only live copy. Split success completion from failed/aborted completion so the local handle is closed only on the ACKed transfer path.🤖 Prompt for 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. In `@src/jsc/ipc.rs` around lines 771 - 793, complete() currently calls terminate() and drops the local handle whenever close_on_complete is set, but on_ack_nack() routes NACK/failure completions through the same path causing premature teardown; modify the logic so that only the ACK-success path (not the NACK/failure path) triggers closing the local handle and calling the native terminate() on handle.js. Locate the complete() method and the on_ack_nack() caller paths and split them into two outcomes (e.g., complete_success vs complete_abort) or add an explicit boolean parameter to complete(self, global, acked: bool) so you only perform the terminate() / close_on_complete branch when acked is true; ensure on_ack_nack() invokes the abort path (or calls complete(..., false)) and successful ACK handling invokes complete(..., true) so the sender retains the socket/server on failed imports.
🤖 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 `@packages/bun-usockets/src/bsd.c`:
- Around line 1090-1092: The TCP path in bsd_create_bound_socket currently sets
*error = LIBUS_ERR (or WSAGetLastError on Windows) when getaddrinfo() fails;
change it to mirror bsd_create_udp_socket by capturing the getaddrinfo() return
(gai_result) and set *error = -gai_result so callers receive the actual
getaddrinfo() error code; update the error assignment and return path around the
getaddrinfo() call in bsd_create_bound_socket to use the negative gai_result
value instead of LIBUS_ERR/WSA error so the Rust-side translation contract
remains consistent.
In `@src/runtime/node/node_cluster_binding.rs`:
- Around line 245-257: The code currently calls
crate::ipc_host::attach_windows_socket_payload(...) and then unconditionally
sets message.put(global, b"$hasHandle", JSValue::TRUE) and native_handle =
Some(bun_jsc::ipc::Handle::init(...)); change this so the Windows socket-export
result is checked: call attach_windows_socket_payload and only set the
$hasHandle flag and assign native_handle when that call succeeds; on failure
treat it as "send without handle" (do not set $hasHandle or native_handle) to
match ipc_host::do_send behavior and avoid advertising a handle the receiver
cannot reconstruct. Ensure you still call bun_jsc::ipc::Handle::init(...) only
in the success branch.
In `@src/runtime/socket/Handlers.rs`:
- Around line 512-524: The Windows branch narrows the JS-visible socket value by
casting to u32 before rebuilding an Fd, which corrupts pointer-sized SOCKET
values on 64-bit Windows; in the #[cfg(windows)] block that constructs
Fd::from_system use a pointer-sized cast (preserve full width) instead of v as
u32 — e.g., cast v to usize (or the platform pointer-sized integer) then to *mut
c_void when calling Fd::from_system for generated.fd; ensure you only change the
#[cfg(windows)] arm around Fd::from_system and keep the #[cfg(not(windows))]
Fd::from_uv branch unchanged.
---
Outside diff comments:
In `@packages/bun-usockets/src/context.c`:
- Around line 391-415: us_socket_group_listen_fd currently ignores poll start
failures; replace the call to us_poll_start(...) with us_poll_start_rc(...) and
check its return value, and if it fails then undo the partial setup: set *error
to LIBUS_ERR (or appropriate error from the rc), stop/close/free/unlink the
us_poll_t created by us_create_poll (ensuring the fd is closed if necessary),
and return 0 instead of returning the listener; keep the existing
us_poll_init(...) and us_internal_init_listen_socket(...) calls only after poll
arming succeeds, and ensure the deferred_accept handling remains intact when
poll start succeeds.
In `@packages/bun-usockets/src/socket.c`:
- Around line 412-425: The fd-adoption path (us_socket_from_fd) leaves the
bitfield last_write_failed uninitialized because us_create_poll uses us_malloc
(no zeroing); update the constructor code that sets s->group/s->kind/... to
explicitly initialize s->flags.last_write_failed = 0 (i.e., set the
us_socket_t::flags.last_write_failed bit to 0 alongside the other flags in
socket.c inside us_socket_from_fd) so write/backpressure logic is deterministic.
In `@src/jsc/ipc.rs`:
- Around line 771-793: complete() currently calls terminate() and drops the
local handle whenever close_on_complete is set, but on_ack_nack() routes
NACK/failure completions through the same path causing premature teardown;
modify the logic so that only the ACK-success path (not the NACK/failure path)
triggers closing the local handle and calling the native terminate() on
handle.js. Locate the complete() method and the on_ack_nack() caller paths and
split them into two outcomes (e.g., complete_success vs complete_abort) or add
an explicit boolean parameter to complete(self, global, acked: bool) so you only
perform the terminate() / close_on_complete branch when acked is true; ensure
on_ack_nack() invokes the abort path (or calls complete(..., false)) and
successful ACK handling invokes complete(..., true) so the sender retains the
socket/server on failed imports.
🪄 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: 3a4b14c5-d27b-40d4-b7f4-be3fec5418dc
📒 Files selected for processing (28)
packages/bun-usockets/src/bsd.cpackages/bun-usockets/src/context.cpackages/bun-usockets/src/eventing/libuv.cpackages/bun-usockets/src/internal/networking/bsd.hpackages/bun-usockets/src/socket.csrc/js/builtins/Ipc.tssrc/js/internal/cluster/RoundRobinHandle.tssrc/js/internal/cluster/SharedHandle.tssrc/js/internal/cluster/child.tssrc/jsc/ipc.rssrc/runtime/api/bun/subprocess.rssrc/runtime/ipc_host.rssrc/runtime/node/node_cluster_binding.rssrc/runtime/socket/Handlers.rssrc/uws/lib.rssrc/uws_sys/lib.rstest/js/node/test/parallel/test-cluster-accept-fail.jstest/js/node/test/parallel/test-cluster-disconnect.jstest/js/node/test/parallel/test-cluster-message.jstest/js/node/test/parallel/test-cluster-net-send.jstest/js/node/test/parallel/test-cluster-send-socket-to-worker-http-server.jstest/js/node/test/parallel/test-cluster-server-restart-none.jstest/js/node/test/parallel/test-cluster-shared-handle-bind-error.jstest/js/node/test/parallel/test-cluster-shared-leak.jstest/js/node/test/parallel/test-cluster-worker-handle-close.jstest/js/node/test/parallel/test-cluster-worker-wait-server-close.jstest/js/node/test/sequential/test-cluster-net-listen-ipv6only-none.jstest/js/node/test/sequential/test-cluster-send-handle-large-payload.js
💤 Files with no reviewable changes (14)
- test/js/node/test/parallel/test-cluster-message.js
- test/js/node/test/parallel/test-cluster-accept-fail.js
- test/js/node/test/parallel/test-cluster-shared-leak.js
- test/js/node/test/parallel/test-cluster-server-restart-none.js
- src/js/builtins/Ipc.ts
- test/js/node/test/parallel/test-cluster-worker-handle-close.js
- test/js/node/test/parallel/test-cluster-shared-handle-bind-error.js
- test/js/node/test/parallel/test-cluster-disconnect.js
- test/js/node/test/parallel/test-cluster-net-send.js
- test/js/node/test/parallel/test-cluster-send-socket-to-worker-http-server.js
- test/js/node/test/parallel/test-cluster-worker-wait-server-close.js
- test/js/node/test/sequential/test-cluster-net-listen-ipv6only-none.js
- src/js/internal/cluster/RoundRobinHandle.ts
- test/js/node/test/sequential/test-cluster-send-handle-large-payload.js
First Windows CI run of the handle transfer surfaced two bugs:
- Socket.connect({fd}) parsed the fd with Fd::from_uv, which tags it as
a CRT fd; on Windows the value is a raw SOCKET, so native() round-
tripped it through uv_get_osfhandle and adoption failed with
'Failed to connect' ENOENT. This was the single root cause behind the
disconnect / net-send / send-deadlock / send-handle-large-payload /
net-listen-ipv6only-false failures (every received handle and every
round-robin connection goes through connect({fd})). The SocketConfig
parser was already fixed; this is the second, listener-side parse.
- clusterRawBind bound 0.0.0.0 when no address was given. On Windows a
v4-wildcard bind does not conflict with an existing dual-stack
listener, so a SCHED_NONE worker could bind a port that was already
in use (test-cluster-shared-handle-bind-error). Prefer the IPv6
wildcard like node's createServerHandle, falling back to 0.0.0.0 only
for non-EADDRINUSE failures (machines without IPv6) - falling back on
EADDRINUSE would mask the very collision the caller needs to see.
- the stale-newconn-ack guard in RoundRobinHandle now compares handle identity, not just key presence: a worker removed and re-added while an ack was in transit could have its new in-flight handle deleted by the stale reply, double-distributing the old one - listenInCluster's dns.lookup window is covered by the listening-id guard, so a listen() issued while the lookup is in flight invalidates the stale callback instead of having its reply discarded - the primary's cluster fd send now reports failure instead of emitting a newconn when the Windows socket export failed (worker already dead); the worker-removal path reclaims the pending connection - clusterRawBind always writes IPV6_V6ONLY (0 or 1) for AF_INET6 like uv__tcp_bind, instead of only setting it to 1 - kernels defaulting to v6only (FreeBSD, sysctl'd Linux) would otherwise lose dual-stack - the sender-side pause of a transferred socket moved from Ipc.ts serialize() to do_send after the handle is confirmed transferable: IPCSerialize never forwarded options so keepOpen was ignored, and a reverted Windows send left the socket paused forever - bsd_create_bound_socket reports getaddrinfo failures in the error domain its caller translates (WSA codes on Windows, EINVAL on POSIX where EAI_* has no errno equivalent) - drop a leftover unused fs declaration in cluster child
edde3ce to
f600fb8
Compare
connect({fd}) on Windows probes the fd with uv_guess_handle to detect
named pipes, but Fd.uv() panics for system-tagged descriptors - which
is exactly what a transferred socket is now. A system-tagged fd is a
raw SOCKET by convention and can never be a libuv pipe fd, so skip the
probe for that kind (this crashed every worker that received a socket
handle: 'Cast bun.FD.uv(N[handle]) makes closing impossible').
This PR's base branch (ciro/cluster-tests-v26) merged to main as #31829, so most of the shared IPC/cluster machinery now comes from main; this merge reduces the branch to its own additions (socket_list, probePort/ shareListenFd/ReusePortHandle, http listen({fd}), process.channel.fd, externally framed cluster acks, fd-less stdio pumping) on top of it. Resolutions that were not a straight pick of one side: - ipc_host.rs do_send: main's UDPSocket arm (native_fd, works on Windows) replaces this branch's raw-fd dgram arm; this branch's node:http server-socket arm, is_invalid_handle_type split, target param and EBADF-on-lost-fd are kept, with the EBADF path now also closing the handle main's serialize() detaches. The termination guard is re-expressed with has_pending_termination_exception() since #37275 folded Terminated into Thrown. - Ipc.ts serialize(): main's TLS rejection, _handle detach and native dgram payload, plus this branch's socket_list wiring, kHandle lookup and http parser teardown; the detach is skipped for http server sockets, which have no _handle. Receiving dgram keeps the adoption error listener. - ipc.rs: main's abort_unsent close model and #39123/#37275 error idioms replace this branch's parked-queue model, close_handle_fn cache and clear_exception_except_termination calls; channel_fd and the 5-arg ipc_serialize stay. finish_decode's extra global param is gone because main's Fail(JSError) arm folds the exception instead. - cluster/child.ts: dispatch is an 'internalMessage' listener as in node (so removeAllListeners and externally framed acks behave like node); the native entry stamps cmd, because RoundRobinHandle sends newconn through sendHelper without it. - cluster/primary.ts: main's lazy requires and uv constants, with this branch's feature block rewritten onto them. - net.ts: public _listeningId, nextTick 'listening' and _connectionKey are node's behavior and stay; main's kClusterUnixPath/close() release, silent blockList close and early-byte buffering (no forced resume) are node's behavior and replace this branch's versions; the fd adoption guard combines both fixes (fd 0 adoptable, no double attach). - bsd.c keeps one pre-dup listen() (the reviewed SOMAXCONN block); context.c, RoundRobinHandle.ts and child_process.test.ts are fully superseded by main. - Tests: both test files are main's versions plus this branch's tests for features main lacks. Dropped as superseded by main's contract: the two queued-callback "settles null" tests (main: unsent callbacks are never called), the duplicate dgram delivery test, and the blockList 'drop' test (main: silent close). The mid-handoff redistribution fixture prepends its listener, since cluster dispatch now runs as a listener registered at startup. Verified: cluster.test.ts 36/36, child_process_ipc_handle 14/14, 117 vendored cluster/handle-passing tests, net-server 22/22, spawn.ipc.
|
Follow-up: #40041 emits a received |
… a 1 ms timer (#40041) ### Problem - `child_process_ipc_handle.test.ts` > "a handle sent right before the child's disconnect() is still delivered" is red on main (darwin aarch64, builds 103063 and 102999): the parent never gets the `net.Server`. - Cause: `net.Server` emits `'listening'` from a 1 ms timer (`net.ts:3976`). A received server is emitted from that event (`Ipc.ts:58`), so it arrives one I/O poll late: after the next message, with its backlog already accepted. A `close()` from `'listening'` can inherit a connection, which hangs vite's port probe (#39114). - `http.Server` has the same timer (`_http_server.ts:1067`). Under `jest.useFakeTimers()` it never fires, so `listen(0, cb)` never calls `cb` (#37959). http also registered `cb` only after a successful bind, so a `listen(0)` retried from the EADDRINUSE `'error'` handler never called it: vite's port auto-increment hangs (#27406). ### Fix - Both servers emit `'listening'`, and a failed `listen()` its `'error'`, with `process.nextTick`, as node does. `Bun.listen()` and `Bun.serve()` have called `listen(2)` when they return, so the event is not early. A tick runs before the next I/O poll and is not a timer. - http registers the `listen()` callback before the bind and sets `server.listening` as soon as `Bun.serve()` returns, as node and `net.Server` do. - `net.Server.close()` queues a no-op `setImmediate` so the loop lives one more turn, as node's `uv_close()` does. The timer provided that turn by accident. http gets it from the `Bun.serve` stop task. - Verified: eighteen new or pinned tests in five files, fourteen fail on the unfixed build (notes). The net, tls, http, cluster, IPC, third-party and 54 upstream suites match main. Adopts #39114. Includes the http hunk and tests of #37959. Fixes #27406. Supersedes #37802 and #39242. ### Background - Handle passing: `process.send(msg, server)` sends the socket's descriptor with the message. The receiver wraps it in a new `net.Server` and emits `'message'` from its `'listening'`. Later messages wait for the receiver's ack. - Accept backlog: connections wait in the kernel until a holder of the listening socket accepts them. bun accepts the whole backlog on the first readable poll. - Fake timers: `jest.useFakeTimers()` puts every new `setTimeout`, even one made by a built-in module, into a heap that only `jest.advanceTimersByTime()` drains. `process.nextTick` is never diverted. <details><summary>Notes</summary> Tests (thirteen fail on the unfixed build): `node-net-server.test.ts`: `'listening'` before a later `nextTick`, a listen `'error'` before a later `nextTick`, no accept before a `close()` from the `'listening'` handler, `'beforeExit'` re-emitted (passes on main, fails with the nextTick change alone, which is why `close()` holds the loop). `node-http.test.ts`: the same two order tests for http (they also assert `server.listening` right after `listen()`: true after a bind, false after EADDRINUSE), the retry shape of #27406 (the callback runs once after the retry, fails on main), and http and https twins of the `'beforeExit'` test (pass on main too: the 1 ms timer gave the turn by accident, now the task that emits `'close'` gives it). `cluster.test.ts`: a worker's `listen()` callback runs before the worker reports `'listening'` to the primary, for net and http (http fails on main: the callback used to be registered after the notifier). `child_process_ipc_handle.test.ts`: backlog served after a handoff (`acceptedBeforeDelivery: 20, served: 0` on the released bun), a handle before the message sent after it, and the disconnect test pinned to node's order and reporting on `'close'`. `test-timers.test.ts`: `'listening'` and EADDRINUSE `'error'` under fake timers for net and http (`jest.getTimerCount()` is 1 right after `listen()` on main). Earlier shape of this PR: emit the received server right after `listen({ fd })` returns, with `Ipc.ts` the only runtime change. A review pointed out that this patches the consumer around the stale timer, duplicates the open root fix #39114, and diverges from node once that lands (the received server would emit `'listening'` after `'message'`). This PR now carries the root fix. All three IPC tests pass with the nextTick `'listening'` and the unchanged `Ipc.ts`. #27406 ("`http.Server.listen()` hangs when called again after EADDRINUSE") was closed as not reproducible on 1.3.13. It reproduces on 1.4.0 with a dynamic port: `http.Server.listen` passed the callback into `kRealListen`, which registered it as `once('listening')` only after `Bun.serve()` returned. On EADDRINUSE `Bun.serve()` throws first, so the callback was dropped and the retry's `'listening'` had no listener for it. node (`Server.prototype.listen`: `if (cb !== null) this.once('listening', cb)` before `listenInCluster`) and `net.ts:3807` register it before the bind. The review of this PR found it in the hunk being changed; the fix is the same three lines in `Server.prototype.listen`, and `kRealListen` loses its `onListen` argument. In a cluster worker the user callback now runs before the `act: 'listening'` notification to the primary, as in node (node v26.3.0: `callback:online, cluster:listening` for net and http), and `cluster.worker.state` is still `'online'` inside it. `server.listening` on http was set to true only by the deferred emit, so it lagged `address()` by a tick (by 1 ms before this PR). Node's `listening` is a getter on `_handle`, true as soon as `listen()` returns for the host-less form (with a host, both runtimes report false for one tick: node because of `dns.lookup`, bun because the flag was set by the emit; now bun reports true at once in both forms). `kRealListen` sets the flag right after `Bun.serve()` returns; `close()` and `closeAllConnections()` already clear it, and `emitListeningNextTick` only guards on the handle now, as `net.ts` does. #37959 made the same four `net.ts` changes for the fake-timer symptom and also moved the two `_http_server.ts` emits. Its http hunk and its four tests are folded in here (commit "node:http: emit listen() results on the next tick too"), and #37959 is closed in favor of this PR. The http path was checked like net: a server listened from `'beforeExit'` and closed from `'listening'` prints `beforeExit, listening, nextTick, closeCb, close, beforeExit again` on the debug build. Node prints the same order, except that it emits `'close'` before the `close()` callback, a pre-existing difference not touched here. `http.Server.close()` goes through the `Bun.serve` stop, whose all-connections-closed task (`ServerAllConnectionsClosedTask`, an event-loop task, not a microtask) keeps the loop alive for that turn and then emits `'close'`, so http needs no `setImmediate`; the http and https `'beforeExit'` tests pin that turn. Node's `listen(port, host)` resolves `host` through `dns.lookup` before it binds, and `dns.lookup` defers even an IP literal to a `process.nextTick`. So in node, `listen(port, "127.0.0.1")` emits one tick later than `listen(port)`. bun binds an IP literal synchronously in both cases. The order tests therefore call `listen()` without a host, where node v26.3.0 and bun agree tick for tick: `listening, nextTick` and `error:EADDRINUSE, nextTick`. `node-http.test.ts` requires its tests to pass in node. Measurements with the fixture of the disconnect test, 16 at a time: - bun 1.4.0 (unfixed, report on `'exit'`, 200 runs): 175 deliver the server after `'disconnect'`, 13 between the message and `'disconnect'`, 11 have no server at `'exit'` (the CI shape), 1 has node's order. - bun 1.4.0 (unfixed, report on `'close'`, 200 runs): 199 have the wrong order, 1 of them has no server even at `'close'`. - node v26.3.0, 50 runs: all `handle:srv, after-handle, disconnect`. - debug build with this change, report on `'close'`, 150 runs: all `handle:srv, after-handle, disconnect`. Why `'close'`: `fork()` counts the IPC channel as a channel to close (`child_process.ts:1468`), and `#onDisconnect` emits `'disconnect'` one tick before it counts the close (`child_process.ts:1555`). The stderr pipe counts too (`child_process.ts:1258`), so the child's report is complete at `'close'`. On `'exit'` neither is guaranteed: the IPC EOF is a deferred task, so `'disconnect'` can follow `'exit'`. With the fix and the report on `'exit'`, 18 of 100 runs still had no `'disconnect'`. Why the order is send order: once a message with a handle is fully written, the sender moves it to `waiting_for_ack` and writes nothing but an ack or nack until the receiver's ack arrives (`ipc.rs:1487`, `ipc.rs:1550`). So the message behind a handle never shares a read buffer with it. On the receiver, `PosixSocket::on_data` (`ipc.rs:2436`) and `WindowsNamedPipe::on_read` (`ipc.rs:2515`) decode a whole read buffer inside one event-loop scope, and the nextTick queue drains with the microtasks when that outer scope exits (`event_loop.rs:313`), before the next I/O poll and so before the ack'd follow-up message is read. The scope inside the handle arm (`ipc.rs:2169`) is nested and does not drain on its own. With the 1 ms timer, the follow-up message was read by a poll that ran before the timer expired, so it overtook the handle. Why the timer was there: 25097cd (2023-03-18, "[node:net] Fix issue with `listen` callback firing before it's listening") replaced the `process.nextTick` of #2337 with `setTimeout(..., 1)`, with no test and no linked issue, and kept the error path on `nextTick`. The error path moved to the timer in 6baedd2 (tls.Server, #2552) and #31829 copied it for the cluster path. Whatever the symptom was then, the socket is listening when `listen()` returns today: `Bun.listen()` runs `us_socket_group_listen` -> `bsd_create_listen_socket` -> `bind(2)` and `listen(2)` before it returns (`packages/bun-usockets/src/context.c:388`, `bsd.c:1328`, `bsd.c:1067`), and `kRealListen` already depends on that: it reads the bound address with `getsockname` right after `Bun.listen()` returns (`net.ts:3952`) and chmods the unix socket file (`net.ts:3906`). `Bun.serve()` binds and listens in `Server::listen` (`src/runtime/server/mod.rs`) before it returns, and `_http_server.ts` reads `this[serverSymbol].port` right after. The `'listening'` tests and the upstream files are the evidence that nothing needs the delay. #39628 (deferred module init in `node:net`) is not a factor: bun 1.4.0 has #31829 but not #39628 and reproduces the failure. Not changed: the `dgram.Socket` arm of `parseHandle` emits from `bind()`'s callback, which resolves in the microtask drain of the same read. `http.Server`'s `connectionsCheckingInterval` (a 30 s `setInterval` made on `'listening'`) still lands in the fake heap once `'listening'` fires under fake timers; #37987 covers built-in timers under fake timers in general. #37831 (close the descriptor when `listen({ fd })` refuses it) stays independent. #34659 adds a `nextTickListening` flag to `kRealListen` for cluster-adopted descriptors and deflakes the disconnect test by reporting at process exit with `got.sort()`; both hunks become unnecessary with this change. Suites run with the debug build, same results as main: `test/js/node/net` (the 10 `localhost` failures of `node-net.test.ts` fail on the released bun too), `test/js/node/tls` (1 `ECONNREFUSED` test on a `localhost` bind, same on the released bun), `test/js/node/http` (`node-http.test.ts`: 146 pass, the proxy test fails on `localhost` on the released bun too; `node-http-connect.test.ts`: one 5 s spawn timeout under the debug build, same without the http change), `test/js/node/http2`, `test/js/bun/net` (the `localhost` failures, same on the released bun), `cluster.test.ts` (32 pass), `child_process_ipc.test.js`, `child_process_send_cb.test.js`, `child_process_ipc_large_disconnect.test.js`, `spawn.ipc*.test.ts`, `bun-ipc-inherit.test.ts` (55 pass with cluster), `test/js/third_party/express`, `@fastify`, `socket.io` (308 pass, the same 2 timeouts without the http change). Upstream, all pass: `test-net-listen-*` (9), `test-net-server-listen-*` (4), `test-net-server-close*` (3), `test-net-server-unref*` (2), `test-net-listening`, `test-net-pingpong`, `test-process-beforeexit`, `test-child-process-fork-net-server`, `fork-net-socket`, `send-keep-open`, `test-cluster-basic`, `dgram-1`, `dgram-2`, `eaddrinuse`, `listening-port`, `message`, `net-send`, `server-restart-none`, `server-restart-rr`, `shared-handle-bind-error`, `test-cluster-http-pipe`, `test-http-listening`, `test-http-server-close-all`, `test-http-server-close-destroy-timeout`, `test-http-server-close-idle`, `test-http-server-close-idle-wait-response`, `test-http-server-consumed-timeout`, `test-http-server-options-incoming-message`, `test-http-server-options-server-response`, `test-https-server-close-all`, `test-https-server-close-destroy-timeout`, `test-https-server-close-idle`, `test-http-bind-twice`, `test-http-server-stale-close`, `test-http-server-keepalive-end`, `test-http-server-multiheaders`, `test-http-server-keep-alive-timeout`. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/net/node-net-server.test.ts, test/js/node/cluster.test.ts, test/js/node/child_process/child_process_ipc_handle.test.ts <!-- robobun:evidence:end -->
|
On Windows, a process that shares a listening socket with another process could block in accept() and stop its event loop. #41358 fixes this in the accept path. |
|
The "a received handle that lands on fd 0 is adopted" test from this PR flakes on the x64-asan lane: JSC's lazily opened |
Brings
node:clusterin line with Node v26.3.0 by porting the upstream cluster suite verbatim and implementing the runtime machinery the tests expose as missing — until now workers silently bound their own sockets, so nothing was actually shared. IPC handle passing (process.send(msg, handle)) is implemented on the way, since round-robin scheduling is built on it, and the upstreamchild_processtests that exercise it are ported too.test-cluster-*(parallel + sequential)test-child-process-*handle passing / forkfork-net-server,fork-net-socket,send-keep-open,pass-fd,internal, advanced-serialization framing, …)In total 43 upstream tests are added (31 cluster + 12 child_process) and 15 existing ones re-synced, alongside ~41 new Bun-side test cases (
cluster.test.ts, the newchild_process_ipc_handle.test.ts,child_process.test.ts,node-net.test.ts,spawn.ipc.bun-node.test.ts). Numbers are local debug-build sweeps (Linux and macOS). Every vendored file is byte-identical to upstream;test/expectations.txtis untouched.Behavior changes
ref()/unref()via timer, idempotentclose(),getsocknamefrom the primary's cached sockname — instead of binding their own socket.newconn/acceptedreplies, redistribution when a worker declines, and themaxConnectionsaccept check match Node. Beyond Node: a connection that was in flight to a worker that crashed is handed to another worker instead of being leaked; after a graceful disconnect the primary just drops its copy; a live worker'sact:closeleaves the connection to its ack exactly as Node does. A peer that resets a queued connection no longer crashes the primary.listen(2)on the duplicated fd (newus_socket_group_listen_fd, which validates before touching the descriptor's flags). Bind errnos are captured once and replayed to every worker (EADDRINUSEarrives as an'error'event with Node'sExceptionWithHostPortshape).server.close()in a worker releases its share immediately, so the same worker can re-listen;server.address()reports the shared unix path; abstract-namespace names bind with the exact length. A user-suppliedlisten({ fd })is validated the way Node'screateServerHandledoes — anything that cannot serve (non-sockets, connected stream sockets such as a spawned process's stdio, datagram sockets) isEINVAL, and the primary's own descriptor is left alone.bind({ fd }); new native UDP-from-fd adoption (us_create_udp_socket_from_fd) and aUDPSocket.fdgetter back it. A worker that fails to adopt the shared fd releases it.cluster._getServerquery tuples match Node'slistenInCluster: pipes query as(path, -1, -1), fds as(null, null, null),listen(port, host)carries the real address/family, andreusePort: trueforcesexclusive(net and dgram) so each worker binds its own SO_REUSEPORT socket. Primary handle keys only get the per-listen:indexsuffix for port-0 listens, so fixed ports/pipes/fds share one handle across workers.http.Server.listen()in a worker reportslisteningto the primary (and stays quiet in a child that merely inheritedNODE_UNIQUE_IDwithout a channel).child.send/process.send/worker.sendwith a handle):net.Socket,net.Serveranddgram.Sockethandles cross the channel (fd via SCM_RIGHTS, the NODE_HANDLE ack protocol that already existed in the wire layer; the JS serialize/parse halves were stubs). Semantics followlib/internal/child_process.js: a sentnet.Socketis detached from the sender (no'end'/'close'there, server connection count decremented) unlesskeepOpen; TLS sockets are rejected withERR_INVALID_HANDLE_TYPE; a received handle that lands on fd 0 is adopted; a handle that arrives right before the channel's EOF is still delivered; a send that fails after detaching closes the native socket; owned descriptors are released even when they were allocated at 0–2.disconnect()reports disconnected at once (latersend()fails, a seconddisconnect()errors) but, as in Node, the close waits for the messages queued behind a handle that is still awaiting its ack — only user-initiated disconnects wait; a dead peer or exit closes immediately.NODE_-prefixed messages ({ cmd: "NODE_…" }) are routed to'internalMessage'on both ends, as Node documents; this applies toBun.spawn({ ipc })children too.WSADuplicateSocketW— the exportedWSAPROTOCOL_INFOWrides along with the IPC message and the receiver imports it. Pipe and UDP shared binds reportENOTSUPon Windows rather than pretending to work.fork()from a parent whose stdio slot is closed gives the child/dev/nullfor that slot (libuv's behaviour). The plain inherit action captured whatever descriptor was created next — the IPC socketpair's parent end — so the child held its own channel open and never sawdisconnect().child_processstdio acceptsfswrite streams (their fd is taken from the stream's sink), and passing a stream with no descriptor gives a clear error instead of aTODO.internal/cluster/round_robin_handleandinternal/test/binding(audp_wrap.UDPbind/fd shim) resolve for the vendored--expose-internalstests, gated exactly likebun:internal-for-testing.listeningId) so alisten()superseded before the primary replies closes the late handle instead of adopting it.server.blockListrejections under round-robin are closed silently ('drop'is formaxConnections), as in Node.Bun-side tests
test/js/node/cluster.test.ts(+~1000 lines: scheduling, shared handles, unix/abstract paths, fd validation, dgram release, mid-handoff crash, early bytes, blockList, …), newtest/js/node/child_process/child_process_ipc_handle.test.ts(handle passing semantics: detach, keepOpen, TLS rejection, fd 0, queued handles on channel close, disconnect ordering), plus additions tochild_process.test.ts,node-net.test.tsandspawn.ipc.bun-node.test.ts. The latter previously asserted that a receivednet.Sockethandle is rejected — that only held whileparseHandlethrew; it now asserts thatnet.Socketanddgram.Sockethandles are delivered and that the descriptors are released.Known limitations / follow-ups
http/httpsserver connection sockets have no transferable handle in Bun (they are native, like TLS sockets), sochild.send(msg, httpSocket)sends the message without the handle — the same path Node takes for a handle-less socket, but Node's http sockets do have one; documenting or throwing like TLS is a follow-up.send()is called, not when the message is actually written as in Node, so a socket queued behind an un-acked handle is not reusable if the channel dies first. A late-adopted server/dgram handle is emitted after'disconnect'where Node emits it before.NODE_prefix on receipt, so a worker that monkey-patchesprocess.sendbreaks the worker↔primary protocol (works in Node).'drop'event formaxConnectionsis emitted without Node's address payload;server.listen({ fd })on an fd that is already listening resolves rather than failing withEEXIST.ENOTSUP); the cluster tests covering them are skipped there.cluster.schedulingPolicystill defaults toSCHED_RRon Windows (Node defaults toSCHED_NONEthere); flipping it deserves its own PR now that shared handles work.spawnSyncwith a closed inherited stdio slot still fails withEBADF(pre-existing; Node aborts on the same input).no test proof · iteration 71 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/net/named-pipe-listen-error.test.ts test/js/bun/spawn/spawn.ipc.bun-node.test.ts test/js/node/child_process/child_process_ipc_handle.test.ts test/js/node/cluster.test.ts