fix(ext/node): capture IPC handle eagerly to fix cluster send deadlock#34661
Merged
Conversation
`ChildProcess.prototype.send` deferred deriving a handle's IPC info
(`getIpcHandleInfo`) until the message was actually written. When a
previous handle send was still awaiting its `NODE_HANDLE_ACK`, the new
send was queued and only had its handle serialized later, at drain time.
That races against the socket being torn down. In the cluster
send-deadlock scenario the primary hands two freshly-accepted
connections to a worker that immediately closes them, so by the time the
queued send drained the socket's `_handle` was already null and
`getIpcHandleInfo` threw
`notImplemented("ChildProcess.send with non-TCP net.Socket handle")`,
crashing the worker.
Derive the handle info eagerly in `send()`, while the socket is still
alive, before deciding whether to queue. `fdForIpc()` dups the fd, so the
captured copy survives the original socket's destruction. The write/queue
logic is factored into `enqueueOrDispatch`/`dispatch` so queued items
carry their already-derived `handleInfo`.
Re-enables the previously-disabled flaky test
`parallel/test-cluster-send-deadlock.js`.
Co-Authored-By: Divy Srivastava <[email protected]>
A handle send that already wrote successfully keeps its local copy open until NODE_HANDLE_ACK arrives; queued sends haven't been written yet. When the channel is torn down (disconnect, peer CLOSE, or a read error) that ACK never comes, so those handles leaked. A `closeAfterSend` handle is a live resource (e.g. a received net.Socket materialized with `readable: true`, which holds a ref'd read) that keeps the event loop alive, so the process hangs instead of exiting. This is what made `test-cluster-send-deadlock` time out on slow debug builds: the worker forwards its sockets back to the primary, then disconnects before the ACKs land. The forwarded sockets were never destroyed and kept the worker alive past the 10s test timeout. Release builds win the race often enough to pass; debug Linux did not. Add `cleanupPendingHandles()` and call it from `disconnect()` and the read loop's teardown path so pending/queued `closeAfterSend` handles are released once the channel is gone.
The IPC handle fixes in this PR (eager handle derivation, release of held handles on channel teardown) make this test pass reliably on release builds and on every platform locally, but it still times out intermittently on debug Linux from a mutual handle-send / disconnect race in the cluster IPC protocol. The failure is not reproducible outside debug Linux (verified with 400+ local runs, including a forcing repro and CPU-saturated parallelism on macOS), so keep the test disabled rather than flaking CI. The underlying flakiness remains tracked in #34180. The runtime fixes are retained: they are genuine correctness improvements and stay covered by the sibling cluster/child_process handle-passing tests that do run.
littledivy
approved these changes
Jun 1, 2026
littledivy
added a commit
to crowlKats/deno
that referenced
this pull request
Jun 10, 2026
denoland#34661) ## Problem `node_compat::parallel::test-cluster-send-deadlock.js` was flaky in CI and was disabled in denoland#34325. The most visible symptom was: ``` Not implemented: ChildProcess.send with non-TCP net.Socket handle ``` (see denoland#34328, originally tracked as denoland#34180). ## Root cause (the `notImplemented` crash) `ChildProcess.prototype.send` derived a handle's IPC info (`getIpcHandleInfo`) **lazily**, only when the message was about to be written. The IPC protocol keeps a single handle in flight at a time: while one handle send awaits its `NODE_HANDLE_ACK`, subsequent sends are pushed onto `handleQueue` and only serialized later, when the queue drains on ACK. That deferral races against the socket being torn down. The primary accepts two connections and hands each to a worker that immediately `end()`s them. The second `worker.send('handle', socket)` gets queued behind the first handle's ACK; by the time it drains, the server socket has been destroyed and its `_handle` is `null`. `getIpcHandleInfo` then hits `null instanceof TCP === false` and throws `notImplemented`, crashing the worker. ## Fixes in this PR Two IPC handle-passing correctness fixes, both also exercised by the cluster/child_process handle-passing tests that already run in CI: 1. **Derive the handle's IPC info eagerly** in `send()`, while the socket is still alive, *before* deciding whether the send must be queued. `fdForIpc()` dups the underlying fd, so the captured copy outlives the original socket's destruction and the deferred write still sends a valid descriptor. The write/queue logic is factored out of `send()` into `enqueueOrDispatch` (queue-or-write) and `dispatch` (write + ACK/error handling), so queued items carry their already-derived `handleInfo`. A genuinely unsupported handle now throws synchronously from `send()` (matching Node) rather than later at drain time. 2. **Release held handles when the channel disconnects.** A handle send that already wrote keeps its local copy open until `NODE_HANDLE_ACK` arrives; queued sends aren't written yet. On teardown (disconnect / peer CLOSE / read error) that ACK never comes, so those `closeAfterSend` handles — live `net.Socket`s materialized with `readable: true`, holding a ref'd read — leaked and kept the event loop alive. `cleanupPendingHandles()` releases them from `disconnect()` and the read loop's teardown path. ## Test status — still disabled With these fixes the test passes reliably on release builds and on every platform I could test locally. However, it **still times out intermittently (10s) on debug Linux** due to a mutual handle-send / disconnect race in the cluster IPC protocol (the worker forwards its sockets back to the primary, then disconnects; the disconnect handshake is queued behind a reply handle awaiting an ACK). I could not reproduce this outside debug Linux despite 400+ local runs (forcing repro + CPU-saturated parallelism on macOS), so I have **kept the test disabled** rather than flake CI. The remaining flakiness stays tracked in denoland#34180. So this PR does **not** re-enable the test / close the flakiness issue; it lands the two runtime correctness improvements and documents the residual debug-Linux race. ## Testing - All config-listed cluster/child_process handle-passing tests pass: `test-cluster-concurrent-disconnect`, `test-cluster-net-send`, `test-cluster-rr-*`, `test-cluster-send-handle-twice`, `test-cluster-send-socket-to-worker-http-server`, `test-cluster-shared-*`, `test-child-process-send-*`, `test-child-process-ipc*`, etc. - `test-cluster-send-deadlock.js` itself: passes 50/50 locally, but remains disabled because of the debug-Linux timeout described above. --------- Co-authored-by: divybot <[email protected]> Co-authored-by: Divy Srivastava <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
node_compat::parallel::test-cluster-send-deadlock.jswas flaky in CI and was disabled in #34325. The most visible symptom was:(see #34328, originally tracked as #34180).
Root cause (the
notImplementedcrash)ChildProcess.prototype.sendderived a handle's IPC info (getIpcHandleInfo) lazily, only when the message was about to be written. The IPC protocol keeps a single handle in flight at a time: while one handle send awaits itsNODE_HANDLE_ACK, subsequent sends are pushed ontohandleQueueand only serialized later, when the queue drains on ACK.That deferral races against the socket being torn down. The primary accepts two connections and hands each to a worker that immediately
end()s them. The secondworker.send('handle', socket)gets queued behind the first handle's ACK; by the time it drains, the server socket has been destroyed and its_handleisnull.getIpcHandleInfothen hitsnull instanceof TCP === falseand throwsnotImplemented, crashing the worker.Fixes in this PR
Two IPC handle-passing correctness fixes, both also exercised by the cluster/child_process handle-passing tests that already run in CI:
Derive the handle's IPC info eagerly in
send(), while the socket is still alive, before deciding whether the send must be queued.fdForIpc()dups the underlying fd, so the captured copy outlives the original socket's destruction and the deferred write still sends a valid descriptor. The write/queue logic is factored out ofsend()intoenqueueOrDispatch(queue-or-write) anddispatch(write + ACK/error handling), so queued items carry their already-derivedhandleInfo. A genuinely unsupported handle now throws synchronously fromsend()(matching Node) rather than later at drain time.Release held handles when the channel disconnects. A handle send that already wrote keeps its local copy open until
NODE_HANDLE_ACKarrives; queued sends aren't written yet. On teardown (disconnect / peer CLOSE / read error) that ACK never comes, so thosecloseAfterSendhandles — livenet.Sockets materialized withreadable: true, holding a ref'd read — leaked and kept the event loop alive.cleanupPendingHandles()releases them fromdisconnect()and the read loop's teardown path.Test status — still disabled
With these fixes the test passes reliably on release builds and on every platform I could test locally. However, it still times out intermittently (10s) on debug Linux due to a mutual handle-send / disconnect race in the cluster IPC protocol (the worker forwards its sockets back to the primary, then disconnects; the disconnect handshake is queued behind a reply handle awaiting an ACK). I could not reproduce this outside debug Linux despite 400+ local runs (forcing repro + CPU-saturated parallelism on macOS), so I have kept the test disabled rather than flake CI. The remaining flakiness stays tracked in #34180.
So this PR does not re-enable the test / close the flakiness issue; it lands the two runtime correctness improvements and documents the residual debug-Linux race.
Testing
test-cluster-concurrent-disconnect,test-cluster-net-send,test-cluster-rr-*,test-cluster-send-handle-twice,test-cluster-send-socket-to-worker-http-server,test-cluster-shared-*,test-child-process-send-*,test-child-process-ipc*, etc.test-cluster-send-deadlock.jsitself: passes 50/50 locally, but remains disabled because of the debug-Linux timeout described above.