Skip to content

fix(ext/node): implement node:cluster on unix - #33752

Merged
nathanwhit merged 15 commits into
denoland:mainfrom
nathanwhit:node-cluster
May 4, 2026
Merged

nathanwhit merged 15 commits into
denoland:mainfrom
nathanwhit:node-cluster

Conversation

@nathanwhit

Copy link
Copy Markdown
Member

Depends on #33605.
Closes #27485.

Enables 68 node_compat tests

@nathanwhit nathanwhit changed the title Node cluster fix(ext/node): implement node:cluster Apr 30, 2026
@nathanwhit nathanwhit changed the title fix(ext/node): implement node:cluster fix(ext/node): implement node:cluster on unix Apr 30, 2026

@fibibot fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.rs changes overlap with what I just APPROVED in #33605's v2 (incl. the OwnedFd typing, MSG_CMSG_CLOEXEC, dup'd fd in uv_tcp_fd_for_ipc). Once #33605 lands, this rebase should cleanly absorb those changes.
  • net.Native IPC handle type added to child_process.ts: shape is { type: "net.Native", socketType: 0|1, nativeKind: "tcp"|"pipe", msg } with closeAfterSend: false and the comment explaining cluster's RoundRobinHandle/SharedHandle own the lifecycle. The reconstruction path correctly differentiates between TCP and Pipe wraps.
  • socket_type_for_ipc ops added on TCP and Pipe — both have well-commented 0 = SOCKET / 1 = SERVER mappings. Receivers use it to decide between uv_*_open (connected stream) vs uv_*_open_listener.
  • RoundRobinHandle Deno deviation: re-runs setupTCPListenWrap / setupPipeListenWrap after assigning a new onconnection, 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

  1. Cluster algorithm parity vs Node: a line-by-line comparison of the seven new internal/cluster/*.ts files against Node's lib/internal/cluster/*.js to catch any silent semantic deviations. The TODO prefer-primordials exemption suggests these are intentional ports; worth confirming nothing was lost in the port.
  2. closeAfterSend: false for net.Native — the comment correctly identifies that cluster owns the lifecycle, but if process.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 that net.Native is cluster-internal-only or adding an assertion.
  3. Worker termination paths: RoundRobinHandle.remove closes this.handle; if distribute is concurrent (RST_STREAM mid-shutdown), the handle could be appended after close. Edge case; needs a Node-source comparison to confirm Node's behavior.
  4. 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 bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_fd drops extra fds silently (bi_pipe.rs:229): the loop keeps only the first received fd and close()s the rest. This is correct for the protocol (one fd per message), but a debug_assert!(fd_count <= 1) would catch protocol bugs during development.

  • Partial advanced IPC write + fd leak on receiver side (ipc.rs:123): if send_with_fd succeeds but write_all for the remainder fails, the sender's OwnedFd is 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_fd msg/control buffers across retries: after WouldBlock, the outer loop retries with the same msg struct. This is fine because sendmsg either 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, createIpcHandle in child_process.ts.

Comment thread ext/node/polyfills/cluster.ts Outdated
Comment thread ext/node/polyfills/internal/cluster/round_robin_handle.ts Outdated
// 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") {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread libs/core/uv_compat.rs

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nathanwhit
nathanwhit enabled auto-merge (squash) May 4, 2026 19:14
@nathanwhit
nathanwhit merged commit 04e5db2 into denoland:main May 4, 2026
136 checks passed

@fibibot fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 against 1cdb3e3 shows the cluster-algorithm files are mostly unchanged — most edits cluster around ext/process/ipc.rs and the IPC handle plumbing.
  • fix shared handle forced listen (d65e377): 8 lines added to child_process.ts, 7 added / 18 removed in cluster/shared_handle.ts, plus 36/-11 in libs/core/uv_compat/tcp.rs. Looks like the SharedHandle path was fixed to not redundantly listen() when receiving an already-listening server handle.
  • fix regression (2af384b): 7 lines added to net.ts, 29/-25 in uv_compat/tcp.rs — surface area suggests a TCP wrap binding regression introduced by the prior commit, now fixed.
  • Test gating: disable failing test and disable test on windows — appropriate given the PR title's on unix scope. 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:

  1. closeAfterSend: false for net.Native handles outside cluster context — still applies; worth a follow-up assertion or doc comment if net.Native ends up exposed beyond cluster.
  2. RoundRobinHandle.remove vs concurrent distribute race — node_compat tests would surface a regression here, and they're green.
  3. The setupTCPListenWrap/setupPipeListenWrap re-run after onconnection reassignment — 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.

littledivy pushed a commit to crowlKats/deno that referenced this pull request Jun 10, 2026
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.

Error: Not implemented: cluster.fork

3 participants