Repository navigation
fix(ext/node): implement node:cluster on unix - #33752
Conversation
fibibot
left a comment
There was a problem hiding this comment.
High-level review only — 2662-line diff across 26 files spanning the IPC fd-passing layer (shared with #33605, which I just thoroughly reviewed and the v2 of which addressed every concern), plus a full Node port of lib/internal/cluster/{primary,child,round_robin_handle,shared_handle,worker,utils,linkedlist}.js. The cluster algorithm itself needs a closer spec-comparison pass than I can give in one iteration; high-level it looks faithful.
What I checked at a high level
- Dependency on #33605 is real:
bi_pipe.rs,ext/process/ipc.rs,uv_compat/tcp.rschanges overlap with what I just APPROVED in #33605's v2 (incl. theOwnedFdtyping,MSG_CMSG_CLOEXEC, dup'd fd inuv_tcp_fd_for_ipc). Once #33605 lands, this rebase should cleanly absorb those changes. net.NativeIPC handle type added tochild_process.ts: shape is{ type: "net.Native", socketType: 0|1, nativeKind: "tcp"|"pipe", msg }withcloseAfterSend: falseand the comment explaining cluster's RoundRobinHandle/SharedHandle own the lifecycle. The reconstruction path correctly differentiates between TCP and Pipe wraps.socket_type_for_ipcops added on TCP and Pipe — both have well-commented0 = SOCKET / 1 = SERVERmappings. Receivers use it to decide betweenuv_*_open(connected stream) vsuv_*_open_listener.RoundRobinHandleDeno deviation: re-runssetupTCPListenWrap/setupPipeListenWrapafter assigning a newonconnection, with a comment explaining that Deno's listen-wrap captures the callback at wrap time (so just reassigning would skip the accept-wrap step). That's the kind of Deno-specific deviation that's correct now but easy to lose on future refactors of the listen-wrap layer.- CI: 131 pass, 3 pending, 0 fail. Healthy signal but not full.
What needs a closer pass
- Cluster algorithm parity vs Node: a line-by-line comparison of the seven new
internal/cluster/*.tsfiles against Node'slib/internal/cluster/*.jsto catch any silent semantic deviations. The TODOprefer-primordialsexemption suggests these are intentional ports; worth confirming nothing was lost in the port. closeAfterSend: falsefornet.Native— the comment correctly identifies that cluster owns the lifecycle, but ifprocess.send(handle)is called with a native TCP/Pipe outside the cluster module, the absence of an auto-close means the sender has to remember to close the handle themselves. Worth either documenting thatnet.Nativeis cluster-internal-only or adding an assertion.- Worker termination paths:
RoundRobinHandle.removeclosesthis.handle; ifdistributeis concurrent (RST_STREAM mid-shutdown), the handle could be appended after close. Edge case; needs a Node-source comparison to confirm Node's behavior. - 68 newly-enrolled node_compat tests — those are the load-bearing regression coverage. Nothing in the visible CI marks any of them as failing yet, but the macos/windows shards are still pending.
Holding COMMENT rather than approving — too large a delta for me to vouch for the cluster-algorithm correctness in one pass, and CI hasn't fully landed. The IPC-layer additions look correct against my #33605 mental model; the cluster algorithm needs a maintainer-level review for spec parity.
bartlomieju
left a comment
There was a problem hiding this comment.
Review
The architecture is sound -- faithful port of Node's lib/internal/cluster/*.js with well-documented Deno-specific deviations. The IPC fd-passing layer (sendmsg/recvmsg with SCM_RIGHTS) is clean, the NODE_HANDLE/NODE_HANDLE_ACK protocol in child_process.ts correctly implements ordering guarantees, and the 68 newly enabled node_compat tests provide strong regression coverage.
One blocking issue (stale named exports -- see inline), a few worth-a-look items inline, and some minor notes below.
Minor (no inline)
-
recv_with_fddrops extra fds silently (bi_pipe.rs:229): the loop keeps only the first received fd andclose()s the rest. This is correct for the protocol (one fd per message), but adebug_assert!(fd_count <= 1)would catch protocol bugs during development. -
Partial advanced IPC write + fd leak on receiver side (
ipc.rs:123): ifsend_with_fdsucceeds butwrite_allfor the remainder fails, the sender'sOwnedFdis dropped correctly (RAII), but the receiver got the fd with the first bytes yet will see a truncated message frame. The comment at line 121 acknowledges this ("the reader must close the received fd while discarding the malformed message") but there's no code on the receiver side actually handling that case. Low likelihood but worth a TODO. -
send_with_fdmsg/controlbuffers across retries: afterWouldBlock, the outer loop retries with the samemsgstruct. This is fine becausesendmsgeither fully attaches the fd or returns an error -- there's no partial-fd-attach case. Just worth a note for future readers. -
Missing TypeScript type annotations on
rawFdFromTcpHandle,getIpcHandleInfo,createIpcHandleinchild_process.ts.
| // and the LISTEN-state are not visible. Force-listen here so that the | ||
| // primary's bind error is surfaced now, and the underlying fd is in | ||
| // LISTEN state when the worker dups it. | ||
| if (this.handle && typeof this.handle.listen === "function") { |
There was a problem hiding this comment.
This force-listen in the primary diverges from Node, where SharedHandle leaves listen() to the worker. The comment explains why (Deno's tokio-based TCP doesn't surface deferred bind errors), which is good.
Worth noting: this means the primary holds the fd in LISTEN state, so it will accept SYN packets even though only workers should be handling connections. In practice the primary never calls accept() so the kernel backlog fills and SYN cookies / retries handle it, but it's a subtle behavioral difference from Node. If this ever causes issues with SO_REUSEPORT load balancing (where the kernel distributes SYN across all listeners), this would be the place to look.
bartlomieju
left a comment
There was a problem hiding this comment.
LGTM! Faithful port with well-documented Deno-specific deviations. One minor note:
closeAfterSend: false for net.Native (child_process.ts) -- if user code ever calls process.send(rawTcpOrPipeHandle) outside of cluster, the sender has to remember to close the handle themselves (since there's no postSend hook). Might be worth either documenting that net.Native is cluster-internal-only or adding a defensive check. Not blocking.
fibibot
left a comment
There was a problem hiding this comment.
Re-checking after the large rebase + 5 follow-up commits since my prior pass at 1cdb3e3 (address review comments, disable failing test, fix shared handle forced listen, fix regression, disable test on windows).
What's progressed since my prior pass
address review comments(e8a5fce): code-level cleanup from this PR's reviewers. Without seeing the original review threads I can't tell which comments were addressed, but the diff against1cdb3e3shows the cluster-algorithm files are mostly unchanged — most edits cluster aroundext/process/ipc.rsand the IPC handle plumbing.fix shared handle forced listen(d65e377): 8 lines added tochild_process.ts, 7 added / 18 removed incluster/shared_handle.ts, plus 36/-11 inlibs/core/uv_compat/tcp.rs. Looks like the SharedHandle path was fixed to not redundantlylisten()when receiving an already-listening server handle.fix regression(2af384b): 7 lines added tonet.ts, 29/-25 inuv_compat/tcp.rs— surface area suggests a TCP wrap binding regression introduced by the prior commit, now fixed.- Test gating:
disable failing testanddisable test on windows— appropriate given the PR title'son unixscope. Windows cluster support is deferred, which is reasonable given the size of this already.
CI
Fully green at this head: 99 success, 1 skipped, 0 fail. The 68 newly-enrolled parallel/test-cluster-* node_compat tests (the load-bearing regression coverage I was concerned about in my prior review) all run cleanly. Strong external-spec signal.
What's still on the maintainer
My prior review's substantive concern — line-by-line cluster-algorithm parity vs Node's lib/internal/cluster/{primary,child,round_robin_handle,shared_handle,worker,utils,linkedlist}.js — is still beyond what I can vouch for in one pass at this scale (1561 additions, 18 files). That correctness call belongs with @nathanwhit as both author and the maintainer most familiar with the cluster shape.
The edge cases I called out previously:
closeAfterSend: falsefornet.Nativehandles outside cluster context — still applies; worth a follow-up assertion or doc comment ifnet.Nativeends up exposed beyond cluster.RoundRobinHandle.removevs concurrentdistributerace — node_compat tests would surface a regression here, and they're green.- The
setupTCPListenWrap/setupPipeListenWrapre-run afteronconnectionreassignment — Deno-specific deviation that's correct now but fragile to future listen-wrap refactors.
None are CI-failing concerns; all are maintenance-trail items.
Posture
Holding to COMMENT for the same reason as last time — the PR is too large for confident solo APPROVE, and the algorithm-correctness call is rightly @nathanwhit's. CI green confirms the test enrollment doesn't regress, the review-comment commits show iteration is happening, and the unix-only scope is defensible.
This is approve-shape from the CI/regression-test signal; the cluster-algorithm parity is the remaining variable, which @nathanwhit owns.
Depends on denoland#33605. Closes denoland#27485. Enables 68 node_compat tests
Depends on #33605.
Closes #27485.
Enables 68 node_compat tests