Conversation
parseHandle() emitted the message from the listen() callback. net.Server
fires 'listening' from a 1ms timer, and the first poll of the adopted fd
accepts everything already queued in its backlog, so every connection that
was waiting when the handle was sent was emitted as 'connection' before the
receiver had the server and could attach a listener. Those sockets were
never closed and the clients hung.
Bun.listen({ fd }) adopts the descriptor synchronously, so emit right after
listen() returns, the same way the net.Socket arm does. This also delivers
the handle before any message queued behind it, matching node's order.
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 9:05 AM PT - Aug 12th, 2026
❌ @robobun, your commit 0e34fd6 has some failures in 🧪 To try this PR locally: bunx bun-pr 37802That installs a local version of the PR into your bun-37802 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-reasoned ordering fix that matches the existing net.Socket arm.
What was reviewed:
- Verified
Bun.listen({ fd })sets_handlesynchronously (net.ts:3909) and'listening'fires from a 1ms timer (net.ts:3961), so the PR's mechanism holds. - Checked the sibling
net.Socketanddgram.Socketarms — Socket already emits synchronously; dgram is stated unaffected and left untouched. - New test wires
error/exitpaths into the asserted JSON so any failure mode breaks thetoEqual; the tightened order test removes a documented node divergence rather than weakening coverage.
Extended reasoning...
Overview
Five-line change to src/js/builtins/Ipc.ts: the net.Server arm of parseHandle() now calls emit() immediately after server.listen({ fd }) returns instead of from the 'listening' callback. Test file gains one new concurrent test reproducing the stranded-backlog bug, and one existing test drops its .sort() workaround to now pin node's delivery order.
Security risks
None. This is event-ordering within the IPC handle-receive path; no parsing of untrusted data, no auth, no new fd handling — the fd was already being adopted, only the timing of the 'message' emit changed.
Level of scrutiny
Medium. IPC handle passing is subtle, but the change is minimal and mirrors what the net.Socket arm two cases below already does. I confirmed the two load-bearing claims in the source: this._handle = Bun.listen({ fd, ... }) at net.ts:3909 (synchronous adoption) and setTimeout(emitListeningNextTick, 1, this) at net.ts:3961 (the timer that made the old callback-based emit lose the backlog race). The ipc.rs comment at :2193-2196 about messages arriving "maybe even before parseHandle calls emit()" is now strictly less of a concern for net.Server since emit is synchronous.
Other factors
- The new test asserts an exact JSON shape (
{ served: 20, acceptedBeforeDelivery: 0 }), so any of the wired error paths (childExit,clientError) add a key and fail thetoEqual— failure modes are observable, not swallowed. - The updated "handle sent right before disconnect()" test replaces a
.sort()+ comment documenting a known divergence with an in-order assertion matching node — this is a strengthening, not a weakening, of an existing test. - The added source comment explains the non-obvious "why not from the callback" choice concisely.
- PR description reports 20/20 reruns pass and the relevant vendored node tests pass; no prior reviewer comments to address.
|
Not adding |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM. The inline nit about the now-observable 'listening' event on the received server is real but not blocking — the backlog fix is strictly an improvement over main and the divergence it introduces is far smaller than the one it removes.
Extended reasoning...
Overview
Three-line change in src/js/builtins/Ipc.ts: when a net.Server handle arrives over IPC, emit the 'message' event synchronously after server.listen({ fd, exclusive: true }) returns instead of from the 'listening' callback. Adds a regression test that queues 20 connections in the backlog before the handoff and asserts the receiver's 'connection' listener sees all of them; tightens an existing test to pin Node's delivery order (handle → queued message → 'disconnect') now that the ordering matches, and switches its report from 'exit' to 'close' to remove a race with the stderr pipe and 'disconnect'.
Security risks
None. No untrusted input parsing, auth, or crypto is touched. The fd being adopted was already passed to server.listen before this change; only the timing of the user-visible 'message' emit moved.
Level of scrutiny
Medium — Node compat and event-loop ordering are subtle, but the change is tiny and its premise is verifiable in-tree. I confirmed in src/js/node/net.ts that with exclusive: true the listenInCluster path goes straight to kRealListen, which assigns this._handle = Bun.listen({ fd, ... }) synchronously (net.ts:3908-3919, 4075-4090), so _handle is set when listen() returns and emitting immediately is safe. The net.Socket arm already uses this pattern. The PR description's account of why the dgram.Socket arm doesn't need the same treatment (its 'listening' fires in the same read's microtask drain) is consistent with the code.
Other factors
The one finding is a nit-level Node-compat divergence: the received server now fires 'listening' after the user's 'message' handler runs, so a listener attached there would fire in Bun but not in Node. Attaching 'listening' to an already-listening IPC-received server is unusual, server.listening is already true at delivery, and the extra event is not incorrect — worth noting but not worth blocking on given the fix resolves connections silently hanging under load. The new test has clear failure semantics (asserts an exact JSON object; on the unfixed build acceptedBeforeDelivery is nonzero and the assertion fails) and the 'exit' → 'close' change in the ordering test is the correct way to avoid the race the comment describes. The comment-cop bot feedback was addressed in 0e34fd6.
|
Status: ready for review.
|
|
Data point from another branch: the version of "a handle sent right before the child's disconnect() is still delivered" that is on main today is flaky under load, and this change is what fixes it.
So until this lands, that test can be expected to flake on main occasionally. |
|
Closing in favor of #40041. It carries the same change to |
… 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 -->
2 participants
Problem
net.Serverto a child while connections were already queued in its accept backlog stranded them: the clients hung with no data, FIN or RST, and the accepted sockets stayed open in the child. node v26.3.0 serves all of them.'listening'callback, a loop turn after adopting the fd, and the first poll of the fd accepts the whole backlog. Those connections were emitted on a server no user code held yet.'listening'runs before I/O, so the same shape works there.Fix
'message'as soon aslisten()returns instead of from the'listening'callback, as thenet.Socketarm already does.dgram.Socketis unaffected.'listening'about 1ms later (node never does); that touches every net/tls server and is left for a separate change.acceptedBeforeDelivery: 20, served: 0) and passes with the fix, 20/20 reruns; the reordered disconnect test also fails on main. The vendored node handle-passing tests pass on a debug build.Background
child.send(msg, server)duplicates the server's fd into the child, which wraps it in a newnet.Serverand delivers it as the second argument of'message'. Both processes then hold the same listening socket.net.Serveremits'listening'from a 1ms timer, but adopting an fd completes beforelisten()returns.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/child_process/child_process_ipc_handle.test.ts
Original description
Follow-up to #31829. Passing a listening
net.Serverthat already has connections waiting in its accept backlog (child.send('srv', server)under load) stranded those connections in the receiver.Repro
Parent listens, sends the server, closes its own copy (the descriptor was dup'd for the message, so the socket keeps listening) and then opens 20 connections, which can only be accepted by the child:
Before, on every run:
{"served":0,"acceptedBeforeDelivery":20}. The child'sserver._connectionswas already 20 when the'message'event fired, its handler saw none of them, the 20 accepted sockets were never closed (fds stay open in the child) and the clients hung with no data, FIN or RST. node v26.3.0 prints{"served":20,"acceptedBeforeDelivery":0}.Cause
parseHandle()insrc/js/builtins/Ipc.tsemitted the message from thelisten()callback.net.Serveremits'listening'from a 1mssetTimeout, i.e. after at least one turn of the event loop, and uSockets' accept loop drains the whole backlog on the first readiness of the adopted fd. So every queued connection was emitted as'connection'on a server no user code had a reference to yet. node gets away with the same shape because its'listening'is anextTick, which runs before I/O.Fix
Bun.listen({ fd })adopts the descriptor synchronously (_handleis set whenlisten()returns), so emit right afterlisten()returns, as thenet.Socketarm already does. The receiver attaches its listeners inside the'message'handler, before the loop polls the fd. A side effect is that the handle is now delivered before any message queued behind it, which is node's order; the test that documented the old order as a known divergence now pins node's order. Thedgram.Socketarm is unaffected:Bun.udpSocket()resolves synchronously and its'listening'fires in the microtask drain of the same read, so datagrams queued before a handoff were already delivered (verified with the same kind of repro).Not changed here: the 1ms deferral of
'listening'itself innet.ts, which is long-standing and not specific to handle passing. One consequence of emitting before it: the received server still fires its'listening'about 1ms after delivery, so a listener attached to it inside the'message'handler runs, where node never fires it (the server is already listening at delivery in both). Converging on that means emitting'listening'fromnextTickinnet.tslike node, which affects every net/tls server and is left for a separate change.Verification
test/js/node/child_process/child_process_ipc_handle.test.ts: new "connections queued before a net.Server handoff" test fails on main withacceptedBeforeDelivery: 20, served: 0and passes with the fix (20/20 reruns); the "handle sent right before disconnect()" test now asserts the delivery order and also fails on main (it reports from'close', since'exit'can fire before the parent's'disconnect', which one CI run on alpine aarch64 hit). The rest of that file,spawn.ipc*.test.ts, and the vendoredtest-child-process-fork-net-server,fork-net-socket,send-keep-open,cluster-net-sendandcluster-send-handle-large-payloadtests pass with the debug build.