Conversation
disconnect() postponed the close only when a handle was already written and waited for its ACK. A handle that was still queued behind an unfinished write did not count. The channel closed on the next tick and dropped the handle and every message behind it. node postpones the disconnect from the moment it submits the handle write. Treat a queued item that carries a handle the same as a handle that waits for its ACK.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. Walkthrough
ChangesIPC disconnect behavior
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to Queued handle transfers and following messages retain orderly delivery during disconnect, with no remaining concrete merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: fix is up in this PR (#43081). How I reproduced it: the two-file repro in the PR Notes (
The two new tests in Self-review: the verdict was to ship after changes. State of its requests:
One more check, run by hand: a child that holds nothing else on the event loop (server unref'd, no listeners) stays alive until the postponed CI: build 117060 passes on all 181 jobs, Windows and macOS lanes included. No test in the cluster, child_process, or IPC suites failed or needed a retry in that build. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The Rust change is three lines and mirrors the existing close_after_flush mechanism, but it changes when disconnect() actually closes the channel and overlaps with #43001 in the same function, so a maintainer look at the intended node-parity tradeoff would still be worthwhile.
What was reviewed:
SendQueue::disconnectinsrc/runtime/ipc.rs: thequeue.get().iter()borrow is only entered from the JS host functions (Subprocess.disconnect,Bun__closeChildIPC), never insidecontinue_send'swith_mutclosure, so no re-entrantJsCellborrow;continue_sendalready closes once the queue is empty and no ack is pending, andclose_socket_next_tickstill clearsclose_after_flushon kill/exit/EOF.- Considered whether a peer that never reads/acks now makes
disconnect()hang: that matches node's_handleQueuebehavior andkill()/exit still close immediately, so it is intentional rather than a regression. - Both new tests are inside the existing
describe.skipIf(isWindows)block, drain pipes withPromise.all, and assert a combined object; the 2 MiB pad is needed to exceed the socket buffer so the handle is still queued whendisconnect()runs. I did not build and run them locally.
Extended reasoning...
Overview
The PR touches SendQueue::disconnect() in /home/claude/bun/src/runtime/ipc.rs (lines 1264-1273) and adds two test.concurrent cases to /home/claude/bun/test/js/node/child_process/child_process_ipc_handle.test.ts. Previously disconnect() only deferred the close (close_after_flush = true) when waiting_for_ack was set, i.e. the handle had already been written. Now it also defers when any item still in queue carries a handle. Everything else (continue_send closing once the queue drains, close_socket_next_tick clearing the flag on kill/exit/EOF, is_connected() reporting false while close_after_flush is set) is unchanged.
Security risks
None identified. The change only affects the timing of a local IPC socket close between parent and child processes; it does not parse untrusted data, touch credentials, or alter permission checks.
Level of scrutiny
Moderate. The diff is tiny and reuses an existing mechanism, but it is a semantic change in a process-lifecycle path: a disconnect() that used to close on the next tick now waits for the peer to read and ack a handle that may sit behind an arbitrarily large backlog. That matches node (the PR cites _handleQueue handling in lib/internal/child_process.js, which I could not fetch to verify from this sandbox), and kill(), child exit, and peer EOF still force the close, so I do not consider it a bug. However, the PR description itself notes a conflict with #43001 in the same function and a remaining divergence from node (bun flushes the entire trailing queue after the last ack rather than closing next tick), which are design tradeoffs a maintainer should weigh rather than an automated approval.
Other factors
I checked the JsCell borrow rule from src/CLAUDE.md: self.queue.get() in disconnect() is reached only from Subprocess::disconnect (src/runtime/api/bun/subprocess.rs) and close_child_ipc (src/runtime/hw_exports.rs), both JS host functions, and continue_send's with_mut closure does not run user JS, so no overlapping borrow is possible. The new tests cover both directions (parent child.disconnect() and child process.disconnect()), assert the peer received all three messages plus 'disconnect', and assert each callback got null; they live inside the existing describe.skipIf(isWindows) block. No debug build was available and I did not build one, so I did not confirm locally that the tests fail on the base branch and pass with the change; the author's claim that they fail on 1.4.3-canary is plausible given that on the base branch the handle item would be dropped at next-tick close.
Problem
disconnect()drops asend(message, handle)that is still queued behind an unfinished write. The channel closes on the next tick. The handle and every message behind it are lost. On main no send callback runs, so a caller that waits for one never continues (ipc: run the callbacks of sends still queued when the channel closes #43000 changes which of them run, see Notes). node v26.3.0 delivers them all and calls each callback withnull.SendQueue::disconnect(src/runtime/ipc.rs:1265). It postpones the close only whenwaiting_for_ackis set, so only after the handle is written. node postpones once it submits the handle write:_handleQueue ||= [],disconnect.close_after_flush) for a handle that is already written. A handle that is still queued was left out. The bug needs more unwritten IPC data ahead of the handle than the socket buffer holds (about 208 KiB on Linux) at the momentdisconnect()runs. Notes has the measurement and who passes handles.Fix
disconnect()also setsclose_after_flushwhen an item inqueuecarries a handle.continue_sendalready closes the socket once the queue is empty and no ACK is pending.queuehas a handle, orwaiting_for_ackis set" is the same state as node's_handleQueue !== null.kill(), child exit, and peer EOF still close at once throughclose_socket_next_tick.test/js/node/child_process/child_process_ipc_handle.test.ts, one per direction. Both fail on bun 1.4.3-canary. Other suites: see Notes.Background
SendQueueis the outgoing side of an IPC channel.queueholds the messages that are not fully written. A write larger than the socket buffer is partial.queuetowaiting_for_ack. No other message goes out until the ACK arrives.close_after_flushmarks adisconnect()that waits.connectedreportsfalseat once. The socket closes after the queue drains.Notes
Repro (
bun parent.jsandnode parent.js):big: null, handle: null, after: nullbig,handle+handle,after"never"big: null, handle: null, after: nullbig,handle+handle,afterThe child to parent direction (
process.sendof anet.Server, thenprocess.disconnect()) gives the same three rows.One difference from node that this PR does not change. After the last handle is acknowledged, bun flushes the whole remaining queue before it closes. node closes on the next tick and cancels a trailing write that is still pending. With
big1, handleA, mid, handleB, big2, lastand thendisconnect(), node delivers up tohandleB. bun with this PR delivers all six. That is howclose_after_flushalready worked for a written handle (#31829).Paths that stay as they are.
kill(), child exit, and peer EOF callclose_socket_next_tick, which clearsclose_after_flushand closes. The existing test "channel close: written handle callback fires null; unsent queued handle callback never fires" pins that and still passes.Sibling site left as it is.
serialize_and_senddecides thefalsereturn ofsend()fromwaiting_for_ackalone (indicate_backoff,src/runtime/ipc.rs:1626). It is bun's form of node'sreturn this._handleQueue.length === 1, and it has the same gap: while the handle is still inqueue, everysend()returnstrue. In the repro state, five sends (big,handle, then three plain messages) returnfalse, false, true, false, falseon node v26.3.0 andtruefive times on bun, with and without this PR. It is excluded on purpose. It changes only the return value ofsend(), not what is delivered. The first twofalsecome from node'swriteQueueSizethreshold, which bun does not have. That is #30569 (open; #32925 was a fix for it and was closed unmerged). The return value ofsend()should be fixed in one change, under #30569.Relation to other PRs.
disconnect()closes, and it merges cleanly with this PR (git merge-tree, different hunks). "No send callback runs" in the Problem describes main. With ipc: run the callbacks of sends still queued when the channel closes #43000 and without this PR, the close calls thebigandhandlecallbacks withnull, never callsafter, and the child still receives nothing. That is read from its diff, not run. The new tests fail in that state too: they assert what the peer received and thataftergetsnull.SendQueue::disconnectreturnbool,falsewhile the close is postponed, and it keeps thewaiting_for_ack-only condition. The two conflict insrc/runtime/ipc.rs, so the second to land needs a hand rebase. The resolution is the condition from this PR withreturn falsein that branch. node keepsprocess.channelfor the whole_handleQueuewindow, and only_disconnectsets it to null. child_process: set channel to null once the IPC channel disconnects #43001 also skips its test "process.channel stays while process.disconnect() waits for a sent handle's ack" on Windows, because there the handle is still queued whendisconnect()runs. That is the state this PR changes, so the skip needs another look in the rebase.run_deferred. It does not touchdisconnect().Provenance and reach.
SendQueue::disconnect. A tracker search (disconnect, send, handle, ipc and cluster terms) found no report of the symptom.pad, three runs per size. At 200 KB and 220 KB everything arrives. 240 KB is mixed (2 of 3 runs deliver). From 256 KiB up nothing arrives. The limit is the socket buffer (net.core.wmem_defaultis 208 KiB here) while the forked child is still starting and does not read yet. A busy peer would build the same backlog from smaller messages (not measured).send(message, handle)itself, and cluster fornode:netservers underSCHED_RR, one handle per accepted connection from the primary to the worker.node:httpworkers listen withreusePortand pass no handles.Windows and macOS. The change is in shared code. The tests in this file are POSIX-only (
describe.skipIf(isWindows)), so CI does not run the new tests on Windows. On Windows the pipe write completes asynchronously (write()inipc.rs), so a handle is still inqueuewheneverdisconnect()runs in the same tick as the send. The new branch is the usual path there, not a backlog case. Run by hand on Windows Server 2019 x64 with the two test fixtures: bun 1.4.3-canary.1+b64b63069 deliversbigand the handle and dropsafterin both directions. A debug build with this change delivers all three and calls every callback withnull, 3 of 3 runs per direction. One caveat there: an ACK that is read before the write callback runs is dropped (#37815, open), and a postponeddisconnect()then waits until the child exits.test/js/node/test/parallel/test-cluster-shared-leak.jscan reach that state. In two CI runs of this PR it timed out once, on Windows 11 aarch64, and passed on retry (build 117039). It did not time out in build 117060. main shows the same timeout on the same lane (build 116425, 1 of the last 12 main builds), so two runs do not show whether this change moves the rate. The fixtures were not run by hand on macOS. The two new tests pass on the macOS CI lanes.Suites run on the debug build, all pass:
test/js/node/child_process/child_process_ipc_handle.test.ts(16 tests, 6 full runs, and the 2 new tests 15 more times)test/js/node/child_process/child_process_ipc.test.js,child_process_send_cb.test.js,child_process_ipc_large_disconnect.test.jstest/js/bun/spawn/spawn.ipc.test.ts,spawn.ipc.bun-node.test.ts,spawn.ipc.node-bun.test.ts,bun-ipc-inherit.test.ts,spawn-ipc-gc.test.tstest/js/node/cluster.test.ts(35 tests)test/js/node/test/parallel/test-child-process-{fork,ipc,send,disconnect,recv,net,exit,stdio-ipc,constructor,internal}*andtest-cluster-*test/js/node/test/sequential/test-cluster-send-handle-large-payload.js,test-child-process-pass-fd.js,test-child-process-exit.js,test-cluster-port-reuse-between-workers.js,test-cluster-net-listen-ipv6only-{rr,none}.jsno test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/child_process/child_process_ipc_handle.test.ts