Skip to content

fix(ext/node): capture IPC handle eagerly to fix cluster send deadlock#34661

Merged
littledivy merged 3 commits into
mainfrom
orch/divybot-339
Jun 1, 2026
Merged

fix(ext/node): capture IPC handle eagerly to fix cluster send deadlock#34661
littledivy merged 3 commits into
mainfrom
orch/divybot-339

Conversation

@divybot

@divybot divybot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Problem

node_compat::parallel::test-cluster-send-deadlock.js was flaky in CI and was disabled in #34325. The most visible symptom was:

Not implemented: ChildProcess.send with non-TCP net.Socket handle

(see #34328, originally tracked as #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.Sockets 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 #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.

`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]>
@divybot divybot changed the title [denoland/deno#34328] flaky: node_compat parallel/test-cluster-send-deadlock.js fix(ext/node): capture IPC handle eagerly to fix cluster send deadlock Jun 1, 2026
divybot added 2 commits June 1, 2026 17:34
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
littledivy merged commit 472d006 into main Jun 1, 2026
137 checks passed
@littledivy
littledivy deleted the orch/divybot-339 branch June 1, 2026 13:40
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants