ipc: pass net.Server and net.Socket handles over send(message, handle) - #31715
ipc: pass net.Server and net.Socket handles over send(message, handle)#31715robobun wants to merge 7 commits into
Conversation
|
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:
WalkthroughThis PR adds support for adopting already-listening file descriptors as socket listeners and for serializing ChangesSocket FD adoption and IPC handle passing
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:12 PM PT - Jun 28th, 2026
✅ @robobun, your commit f8cb2ff2e68d2cc301e5d3edf9cfc3bb04e84d0f passed in 🧪 To try this PR locally: bunx bun-pr 31715That installs a local version of the PR into your bun-31715 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Thanks — added `Fixes #22559`: making `Bun.listen({ fd })` adopt an inherited listening socket is exactly the systemd socket-activation mechanism, and I verified it end-to-end ( I left #6743 out: that report specifically passes a |
![claude[bot]](https://avatars.github.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/node/child_process/child_process_ipc_handle.test.ts`:
- Around line 137-140: The test collects proc.stdout, proc.stderr, and
proc.exited but never asserts on stderr, allowing unexpected diagnostics to go
unnoticed; add an assertion checking stderr is empty (expect(stderr).toBe(""))
right after the stdout assertion and before the exitCode assertion for the
spawned process referenced by proc (used alongside bunEnv/harness), so failures
printing to stderr are caught.
🪄 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: 6f811324-6b93-42d1-b159-b40a3cf0df36
📒 Files selected for processing (8)
packages/bun-usockets/src/context.cpackages/bun-usockets/src/libusockets.hsrc/js/builtins/Ipc.tssrc/runtime/socket/Handlers.rssrc/runtime/socket/Listener.rssrc/uws_sys/SocketGroup.rstest/js/node/child_process/child_process_ipc_handle.test.tstest/js/node/net/listen-fd.test.ts
|
The diff is green. The only failing CI lane is My own tests ( This needs a maintainer to merge / re-run the macOS lane on a healthy agent. Handing off. |
|
Rebased onto main. The only conflict was in |
1883d82 to
65aafd4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 431-434: The `us_socket_group_listen_fd` branch in `context.c` is
incorrectly rejecting all `LIBUS_USE_LIBUV`/`WIN32` builds with `EINVAL`, which
blocks TCP fd adoption. Remove this unconditional failure and route
Windows/libuv builds through the existing TCP fd-adoption path instead,
preserving support for `server.listen({ fd })` and IPC-transferred `net.Server`
handles while leaving any truly unsupported cases gated by the relevant socket
kind or backend-specific checks.
In `@packages/bun-usockets/src/libusockets.h`:
- Around line 346-351: The public contract for us_socket_group_listen_fd is too
narrow and should match the implementation in context.c and the Rust bindings.
Update the documentation near us_socket_group_listen_fd to describe the full
errno surface returned via *error, including the existing ENOTSOCK/EINVAL cases
plus other propagated failures such as EBADF and poll-registration errors. Keep
the comment aligned with the function behavior so the header no longer implies
only two possible error values.
🪄 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: 984d15a1-ad0d-4af8-84b0-f7d931321b0d
📒 Files selected for processing (2)
packages/bun-usockets/src/context.cpackages/bun-usockets/src/libusockets.h
|
CI status: the diff is green everywhere it runs. The only red lane is That is a Buildkite artifact-store/agent timeout (the build step produced the binary; the test agent could not download it within 120s), unrelated to this change. It recurred identically across builds 65548 and 65570, and a re-roll did not clear it. On 65570, 84 jobs passed and this was the sole failure (the rest still running). The Format/clippy/lint and all other platform test lanes are green. Needs a maintainer to re-run the |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/js/builtins/Ipc.ts`:
- Around line 185-193: Condense the long inline rationale in the Ipc.ts comment
near the exclusive fd handling so it fits the repo’s 3-line limit. Keep only the
essential invariant about exclusive: true, Bun.listen({ fd }) adoption, and why
the handle message is emitted immediately; move the detailed cluster/order
history out of the comment and into the PR or commit message.
In `@test/js/node/child_process/child_process_ipc_handle.test.ts`:
- Around line 225-233: In the worker message handler for
child_process_ipc_handle.test.ts, close the primary listening server before
creating the probe client so the connection can only be accepted by the worker;
use the existing server and worker.on('message') flow to stop listening
immediately after receiving { ready: true }. Also handle the { ready: false }
path explicitly by failing the test through the same completion path instead of
waiting indefinitely, and keep the finish callback wired so both success and
failure complete the test deterministically.
🪄 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: cae241f3-10ce-4670-ac9b-ab508a67ed92
📒 Files selected for processing (3)
src/js/builtins/Ipc.tssrc/uws_sys/SocketGroup.rstest/js/node/child_process/child_process_ipc_handle.test.ts
There was a problem hiding this comment.
All my prior concerns are addressed and the bug hunter found nothing new, but this adds fd-adoption to the uSockets event loop and changes the IPC wire protocol across C/Rust/JS — substantial enough that a maintainer should sign off.
Extended reasoning...
Overview
This PR implements net.Server handle passing over child_process IPC and makes Bun.listen({ fd }) adopt an already-listening socket (enabling systemd socket activation). It spans four layers: a new C function us_socket_group_listen_fd() in bun-usockets (~75 lines of fd validation + epoll/kqueue registration), a Rust FFI wrapper in SocketGroup.rs, the UnixOrHost::Fd arm in Listener::listen, flag plumbing in Handlers.rs, and the serialize()/parseHandle() rewrite in Ipc.ts. Two new test files cover Bun↔Bun, Bun↔Node, message-ordering, and cluster-worker scenarios.
Review history
I left ten inline findings across earlier revisions (test races, Windows error-message regression, msg vs message wire-format key, us_poll_start_rc error handling, allowHalfOpen plumbing, IPC reordering, cluster queryServer routing, dead socktype store, doc-comment sync). Every one is resolved — the most recent commit (82b7e37) closes the last open item (the cluster test's accept() race + missing stderr assertion). CodeRabbit's open items are also addressed. The current bug-hunting pass found nothing.
Security risks
Low. The new surface is listen({ fd: N }), where N is supplied by the calling process itself (or received via SCM_RIGHTS from a parent it forked). The C code validates the fd with getsockopt(SO_TYPE) (rejects non-sockets / closed fds), checks SOCK_STREAM, and on Linux checks SO_ACCEPTCONN; failures surface as structured SystemErrors. There's no privilege boundary crossed — a process that can call Bun.listen can already open arbitrary sockets. The IPC envelope change matches Node's documented NODE_HANDLE format.
Level of scrutiny
High. This is not a mechanical change: it introduces a new code path that registers an arbitrary user-supplied fd with the core event loop, changes the IPC wire protocol (now interoperating with Node.js peers), and alters cluster-worker listen routing (exclusive: true). It touches packages/bun-usockets (the lowest networking layer), Listener.rs (the Bun.listen entry point), and IPC builtins. The macOS SO_ACCEPTCONN exclusion and Windows EINVAL stub are platform-specific decisions a maintainer should ratify.
Other factors
The PR has good test coverage (4 IPC tests + 1 fd-inheritance test, all with deterministic accept ordering and stderr assertions), and CI is green except for an unrelated darwin artifact-download flake per the author's status comment. But the breadth (8 files, C+Rust+JS) and the fact that it activates previously-dead code paths in parseHandle and listenInCluster put it well outside the "simple/obvious" bar for bot approval.
|
Status: ready for maintainer review. Reproduced by forking a child, accepting a TCP connection in the parent, and calling Scope note: the previous head of this branch covered #31829 builds the full |
subprocess.send(message, handle) and process.send(message, handle)
returned true but the peer's message handler received undefined as the
handle. fd passing over the IPC channel did not exist, even though the
SCM_RIGHTS transport and the NODE_HANDLE ack/nack handshake were already
in place in src/jsc/ipc.rs.
- src/js/builtins/Ipc.ts: implement serialize() and parseHandle() for
net.Socket and net.Server using Node's NODE_HANDLE wire format (the
user payload under `msg`), so Bun and Node interoperate in both
directions. Unsupported handle types throw ERR_INVALID_HANDLE_TYPE
instead of silently dropping. Unless options.keepOpen is set, the
sender's net.Socket is detached and its native handle closed one tick
after the fd has been duplicated into the transfer.
- packages/bun-usockets: add us_socket_group_listen_fd() so
net.Server.listen({ fd }) can adopt an already-listening fd. The fd is
validated (SO_TYPE, SO_ACCEPTCONN where reliable) before adoption, and
the listen flags are carried through on the fd path.
- src/runtime/ipc_host.rs: do_send extracts the fd from a TCPSocket or
TLSSocket as well as a Listener, dup()s it so the in-flight Handle
owns its own descriptor, and only ships the wrapped NODE_HANDLE
message once an fd is secured. An unresolvable handle previously still
sent NODE_HANDLE with no fd, which the receiver NACKs until the retry
limit and the message is lost. Threads `options` to serialize().
- src/jsc/ipc.rs: Handle owns its fd and closes it on Drop.
- src/js/node/net.ts: connect({ fd }) adopts the fd and fires `open`
synchronously inside doConnect, so it must run after `connecting` is
set, not before; fd-adopted sockets were stuck on readyState
"opening".
Fixes #6743
Fixes #22559
82b7e37 to
a1d9991
Compare
robobun
changed the title
…e check
The squash that added net.Socket support dropped several fixes that had
landed and were verified earlier in review:
- parseHandle net.Server: listen({ fd, exclusive: true }) + synchronous
emit. Without exclusive, a cluster worker routes the worker-local fd
back to the primary as a queryServer (re-breaking
test-net-listen-handle-in-cluster-1.js); the deferred listen callback
let a follow-up message overtake the handle.
- us_socket_group_listen_fd: reject non-SOCK_STREAM fds (the SO_TYPE
value was read but unused; on macOS the SO_ACCEPTCONN check is compiled
out, so this is the only non-stream guard).
- Widen the error-surface doc comments (EBADF / poll-registration errnos).
- Re-add the message-ordering and cluster-worker regression tests.
…handle has no fd Two net.Socket handoff fixes: - serialize(): on a no-keepOpen handoff of a server-accepted socket, decrement handle.server._connections synchronously and null handle.server (Node semantics). Without it, the detached socket's _destroy never runs under allowHalfOpen, so server.close() never drains / never emits 'close' and getConnections() over-reports. - do_send: if serialize() returned a handle but no transferable fd can be extracted (e.g. a not-yet-connected socket), surface an error instead of silently sending the message with no handle — serialize() may have already detached the sender's socket. Adds a regression test that server.close() drains after an allowHalfOpen handoff.
…ests concurrently Windows has no SOCKET duplication over the IPC named pipe yet. serialize() threw NotImplementedError for any net.Server/net.Socket handle, which propagates synchronously out of send() and broke Node IPC tests that previously tolerated a dropped handle (child_process_ipc, fork-closed- channel-segfault). Return null instead (send the message with no handle, receiver sees undefined), matching the pre-feature behavior. The handle- requiring cluster test stays at its main baseline (unsupported on Windows). Also make the 10 subprocess-spawning tests concurrent per test/CLAUDE.md.
- SocketGroup::listen_fd doc: on success the group owns the fd (us_listen_socket_close closes it); only on failure does the caller keep it — matching the C header/impl contract. - IPC handle test header: Windows send() falls back to delivering the message with no handle (serialize returns null), it does not throw.
|
Status: the diff is green on every lane it touches.
The only lane not passing is This needs a maintainer to merge on the strength of the green lanes, or to re-run the |
|
Heads up for anyone picking this up. Review on #31829 surfaced two issues in shared plumbing that this PR also reaches. Both are pre-existing on main and both are made live here by
Tracking both in #31829 rather than landing the same shared-plumbing change in two open PRs. Detail: #31829 (comment) |
|
I arrived at 1.
int result;
do
result = listen(fd, backlog);
while (IS_EINTR(result));
if (result != 0) {
*error = LIBUS_ERR;
return LIBUS_SOCKET_ERROR;
}2.
3. The error a failed
Node's One overlap to be aware of: this PR sets |
Stacked on the cluster branch, which owns IPC handle passing and fd adoption. This adds the vendored child_process tests neither that branch nor #31715 carries, and fixes three gaps they expose. child_process parallel tests: 93 -> 100 of 109 passing (+13 test files). 1. process.stdout/stderr leaked O_NONBLOCK to `stdio: "inherit"` children. Merely reading the `process.stdout` getter switched fd 1 to O_NONBLOCK. That flag lives on the open file description, which is shared with every child that inherits the descriptor, so children failed large writes with EAGAIN (python: "BlockingIOError: [Errno 35]"). Bun already intended to prevent this - BunProcess.cpp forces stdio sinks synchronous on POSIX - but the undo in Bun__ForceFileSinkToBeSynchronousForProcessObjectStdio was gated on `self.fd`, which FileSink::setup() never populates, so it never ran. Consult the writer, which owns the descriptor after start(), and clear the poll's Nonblocking flag with it so the two agree. Only observable when stdout is a pipe; a terminal already took the isatty branch. Costs nothing: POSIX stdio was already force-sync, so this only stops the flag escaping to children. Fixes test-child-process-set-blocking. 2. A NODE_-prefixed cmd is reserved for the channel's own traffic and emits "internalMessage", not "message". Bun recognised the three NODE_HANDLE{,_ACK,_NACK} commands and delivered everything else, including a caller's own NODE_-prefixed cmd, as a plain "message". Fixes test-child-process-internal. 3. spawn('cat', { stdio: ['pipe', other.stdin, 'inherit'] }) threw "TODO: stream.Writable stdio". A subprocess's `.stdin` is a WriteStream over a FileSink: it carries no `fd`, but the sink knows the pipe's write end and already exposes it through _getFd(). A Writable also satisfies isNodeStreamReadable (both carry .on/.pipe), so the Readable branch was the one actually taken and its Writable twin was unreachable; fold them. Fixes test-child-process-stdio-merge-stdouts-into-cat. Also re-syncs common.isPi to v26.3.0's function form; it had been rewritten as an eagerly-evaluated const, so `common.isPi()` threw.
…(93 → 99 of 109) (#34433) > **Stacked on #31829** (`ciro/cluster-tests-v26`), which owns IPC handle passing and fd adoption. Review that first; this PR's own diff is **4 source files, +61/−21**, plus 13 vendored tests. > > An earlier revision of this PR reimplemented handle passing and `us_socket_group_listen_fd` independently — that duplicated #31829 and #31715 and has been dropped. #31829's version is better: it dups the descriptor (`Handle::init_dup`) instead of transferring ownership, throws `ERR_INVALID_HANDLE_TYPE` for unsupported handles, covers `dgram.Native`, and handles Windows socket transfer. ## Target **100% of Node.js v26.3.0's `child_process` test suite.** | | tests | |---|---| | Node v26.3.0 `test-child-process-*` | 109 | | Passing on `main` today | **93** | | Passing on #31829 alone | 93 (it adds no `test-child-process-*` files) | | Passing with this PR | **99** | | New test files added here | **12** vendored + 2 bun-suite | | Remaining gap to 100% | 10 | Every added test is copied **byte-identical** from `v26.3.0` (verified with `diff -q`), per `test/js/node/test/parallel/CLAUDE.md`. No test is added that does not pass, and none is flaky — see the note on `stdio-reuse-readable-stdio` below. Neither #31829 nor #31715 adds a single `test-child-process-*` file, so these 13 are what actually pin the behavior to v26.3.0. ## The gaps these tests surfaced ### 1. `process.stdout` leaked `O_NONBLOCK` onto `stdio: "inherit"` children Merely *reading* the `process.stdout` getter switched fd 1 to `O_NONBLOCK`. That flag lives on the **open file description**, shared with every child inheriting the descriptor — so children died on large writes: ``` BlockingIOError: [Errno 35] write could not complete without blocking ``` Bun already intended to prevent this (`BunProcess.cpp` forces stdio sinks synchronous on POSIX), but the undo in `Bun__ForceFileSinkToBeSynchronousForProcessObjectStdio` was gated on `self.fd`, which `FileSink::setup()` never populates — so it silently never ran. Only observable when stdout is a **pipe**; a TTY already took the `isatty` branch. Fixes `test-child-process-set-blocking`. ### 2. `NODE_`-prefixed messages did not emit `internalMessage` Node reserves that `cmd` prefix for channel traffic. Bun recognised only the three `NODE_HANDLE{,_ACK,_NACK}` commands and delivered everything else — including a caller's own `NODE_`-prefixed cmd — as a plain `message`. (Bun's cluster keys off `NODE_UNIQUE_ID` and a separate internal wire flag, so nothing in-tree depended on the old behavior; #31829's cluster suite is 27/27 with this change.) Applies on **both** ends, as Node's `setupChannel()` does: the parent-side `ChildProcess` in `child_process.ts` and the child-side `process` in `Process__emitMessageEvent`. Verified against node v26.3.0 — `NODE_foo` is internal, while `fooNODE_` and a bare `NODE_` stay plain messages. Fixes `test-child-process-internal` (child→parent); the parent→child direction is covered by a case in bun's own suite, since no vendored test exercises it. ### 3. Another subprocess's `.stdin` could not be a stdio target `spawn('cat', { stdio: ['pipe', other.stdin, 'inherit'] })` threw `TODO: stream.Writable stdio`. A subprocess's `.stdin` is a WriteStream over a FileSink: it has no `fd`, but the sink knows the pipe's write end and already exposes `_getFd()`. A Writable also satisfies `isNodeStreamReadable` (both carry `.on`/`.pipe`), so the Readable branch was the one actually taken and the Writable twin was unreachable dead code. Covered by a posix-gated case in bun's own `child_process.test.ts` rather than node's `test-child-process-stdio-merge-stdouts-into-cat`: on Windows the sink hands back a raw HANDLE rather than a descriptor, so there is no fd to pass along and `nodeToBun` still throws. Node carries no Windows guard for that test and vendored tests must stay verbatim, so it is not added. The fix is a no-op on Windows. ### Also `common.isPi` re-synced to v26.3.0's function form; it had been rewritten as an eagerly-evaluated const, so `common.isPi()` threw. ## Verification Each fix was checked against **real node v26.3.0** and an **unmodified canary** as a negative control: | flow | this PR | node v26.3.0 | canary | |---|---|---|---| | `stdio:'inherit'` + 100KB child write, stdout a pipe | `NONBLOCK=False`, exit 0 | same | `NONBLOCK=True`, `BlockingIOError`, exit 120 | Two review concerns were **measured and refuted** rather than accepted: a later `Bun.write(Bun.stdout)` does *not* re-set `O_NONBLOCK`, and making stdout blocking costs nothing (10MB through a slow pipe: 1016ms vs canary's 1034ms, event loop stays live) — POSIX stdio was already force-sync, so this only stops the flag escaping to children. No regressions: `node/cluster` 27/0, `node/child_process`, `node/net`, `node/http`, `node/http2`, `node/stream`, `node/dgram`, `node/process`, `web/websocket` all at parity. (This box is heavily loaded and ~2 timing-sensitive tests flake per full run; counts above are with a retry pass.) ## Remaining - **Socket-list / worker bookkeeping** (2: `fork-getconnections`, `fork-net`) — needs `internal/socket_list`, `server._setupWorker` and `_connections` accounting. (`pass-fd` is now included and passing: because the send path dups the descriptor, the child's copy is independent and Node's sender-detach dance isn't required for it.) - **Readable-direction stdio reuse** (3: `stdio-reuse-readable-stdio`, `pipe-dataflow`, `fork-stdio`). Handing a child's `.stdout` to another child races: bun's subprocess pipe reader starts eagerly, so the parent steals bytes from the second child. A prototype made `stdio-reuse` pass but flaked ~1/3 of runs (node: 6/6 clean), so it was dropped rather than shipped — the invariant is spelled out in `pipe-dataflow`, which asserts `readStart` is never called. Needs bun's stdout buffering to become lazy. `pipe-dataflow` additionally wants node's internal `_handle.readStart`. - **`dgram.Socket` handle passing** (1: `fork-dgram`) — #31829 covers `dgram.Native`, not `dgram.Socket`. - **`send()` backpressure return value** (1: `send-returns-boolean`). Bun's `indicate_backoff` (`waiting_for_ack.is_some() && !queue.is_empty()`) already mirrors Node's `_handleQueue.length === 1`; the test fails only because it sends a raw `server._handle`, which Node classifies as `net.Native` and the serializer drops. `net.Native` has no clean bun equivalent — wants a maintainer's call. - **`node:test` standalone** (1: `windows-hide`) — pre-existing, owned elsewhere. - **`process.channel.fd` on the child side** — `test-child-process-fork-advanced-header-serialization` is vendored and green, but it does not exercise what it was written for. Its child gates the hostile length-header write on `process.channel?.fd`, and Bun's child-side `process.channel` is a `Control` EventEmitter with no `fd` (node reports `fd: 3`, Bun `undefined`), so the write is skipped and only "the children exit 0" is asserted. Counted above because it passes and is byte-identical, but the meaningful figure is 98, not 99. Exposing `fd` is not a one-liner: it would make this test start pushing malformed 4-byte headers into the IPC channel, which the parser then has to survive to keep the test green. - **`O_NONBLOCK` reaching a child that inherits another subprocess's `.stdin`** — an intentionally-excluded site of the same bug class as gap 1 above, newly reachable through this PR's `streamFdOf`. The parent's write end of `p3`'s stdin carries `O_NONBLOCK` so the parent's FileSink can write asynchronously, and the flag rides the `dup2` into the child because it lives on the shared open file description. A child that does not retry on `EAGAIN` therefore fails once its write outgrows the socketpair buffer — verified: `cat` reports `write error: Resource temporarily unavailable` and exits 1, and a `fcntl` probe confirms `NONBLOCK=True` on the child's fd 1. Clearing the flag is not a fix, since the parent shares that description and would start blocking, and `posix_spawn` has no post-fork hook to clear it child-side; a real fix needs the child to get its own description via a blocking relay pipe. Pinned by a `.todo`'d large-write case next to the stdin-as-stdio test in `child_process.test.ts`. The small writes in that test never fill the buffer, which is why it passes. - **Windows fd inheritance for socket/pipe stdio** (2: `stdio-merge-stdouts-into-cat`, `server-close`). Both pass on posix but go red on Windows: a subprocess's stdin sink and a live `net.Socket` both hand back a HANDLE rather than a CRT descriptor `Bun.spawn` can inherit into `CreateProcess`. Node has no Windows guard for either and vendored files must stay verbatim, so neither is added. `stdio-merge`'s behaviour is covered posix-gated in `child_process.test.ts`.
|
landed in #31829, passing net.Server and net.Socket over send() works on main |
2 participants
What
subprocess.send(message, handle)andprocess.send(message, handle)silently dropped the handle:send()returnedtrue, the message arrived, but the peer'smessagehandler receivedhandle === undefined. fd/socket passing over the IPC channel did not exist.send(msg, handle)is the primitive under every pre-fork multi-process pattern in the Node ecosystem:node:cluster's connection distribution, socket-passing load balancers, graceful-restart managers, "pass the listening fd to the new version" deploys. All of them degraded silently.Fixes #6743
Fixes #22559
Root cause
The low-level transport already existed in
src/jsc/ipc.rs(SCM_RIGHTS send and receive on the IPC socketpair, theNODE_HANDLE/NODE_HANDLE_ACK/NODE_HANDLE_NACKhandshake, the ack-gated send queue). Nothing fed it:serialize()insrc/js/builtins/Ipc.tswas a stub (return null; // sending file descriptors is not supported yet), so the handle was discarded before any of it ran, andsend()still returnedtrue.net.Server:parseHandle()reconstructs a server withserver.listen({ fd }), butBun.listen({ fd })threwEINVAL: Bun does not support listening on a file descriptor.net.Socket:parseHandle()'snet.Socketcase wasthrow new Error("TODO").Fix
src/js/builtins/Ipc.ts: implementserialize()andparseHandle()fornet.Socketandnet.Server, using Node'sNODE_HANDLEwire format (user payload undermsg, perlib/internal/child_process.js), so a Bun peer interoperates with a Node peer in either direction. Anything else throwsERR_INVALID_HANDLE_TYPE(Node's own error for unsendable handle types) rather than silently dropping. Unlessoptions.keepOpenis set, the sender'snet.Socketis detached (_handle = null, matching Node) and its native handle closed one tick later, after the fd has been duplicated into the transfer.packages/bun-usockets: newus_socket_group_listen_fd()adopts an already-bound, already-listening fd as a listen socket. The fd is validated withgetsockopt(SO_TYPE)(rejects a non-socket likelisten({ fd: 0 })withENOTSOCK) andSO_ACCEPTCONNwhere it is reliable (Linux; on macOS it can report 0 for a genuinely listening inherited fd). Wired throughSocketGroup::listen_fdand theUnixOrHost::Fdarm inListener::listen, which previously threw unconditionally.SocketConfignow carries the listen flags (allowHalfOpen,exclusive,reusePort,ipv6Only) through on the fd path, which dropped them before.src/runtime/ipc_host.rs(do_send): extract the fd from aTCPSocketorTLSSocketas well as aListener, anddup()it so the in-flightHandleowns its own descriptor for the lifetime of the transfer. PreviouslyHandle.fdborrowed the source's live fd and never closed it; the source could be closed and the number reused before the queuedsendmsgran. The wrapped{ cmd: "NODE_HANDLE" }message is now only used once an fd is secured: before this, any handle whose fd could not be resolved would still ship theNODE_HANDLEenvelope, which the receiver NACKs until the retry limit and the message is lost entirely.optionsis threaded through toserialize()sokeepOpenis honored.src/jsc/ipc.rs:Handleowns its fd and closes it onDrop(after the ack, after the retry budget is exhausted, or on queue teardown).src/js/node/net.ts:connect({ fd })randoConnect()first (which adopts the fd and firesopensynchronously, settingconnecting = falseand_handle), then unconditionally setconnecting = true, leaving every fd-adopted socket reportingreadyState: "opening"forever. Reordered so the assignment happens before the adoption.Not covered
dgram.Sockethandles: need an fd getter on the nativeUDPSocketplus a bind-to-fd path, neither of which exist yet.send()now throws for them instead of silently dropping.WSADuplicateSocketW) yet.serialize()returns null there, sosend()delivers the message with no handle (receiver seeshandle === undefined) rather than throwing.#31829 builds the full
node:clusterfamily (round-robin handoff,SCHED_NONEshared handles, UDP, Windows) on top of this same mechanism.Verification
8 tests across
test/js/node/child_process/child_process_ipc_handle.test.tsandtest/js/node/net/listen-fd.test.ts:net.Socketparent to child (the reproduction above), child to parent, and{ keepOpen: true }net.Serverparent to childsend(msg, {})throwsERR_INVALID_HANDLE_TYPElisten({ fd })systemd socket activation (an inherited listening fd at fd 3)nodechild reconstructs both anet.Serverand anet.Socketfrom Bun's wire format, proving theNODE_HANDLEenvelope and the SCM_RIGHTS ancillary fd are what Node expectsAll 8 fail on an unfixed build (
USE_SYSTEM_BUN=1: 0 pass, 8 fail) and pass with this one. The three vendored Node tests forlisten({ fd })(test-listen-fd-detached.js,test-listen-fd-detached-inherit.js,test-net-listen-fd0.js) also pass now;test-net-listen-fd0.jsis the one theSO_TYPEvalidation is for.