node:worker_threads: +48 Node.js tests passing — MessagePort, stdio, SHARE_ENV, exit codes, transfer semantics, postMessageToThread + inspector - #31216
Conversation
|
Updated 4:16 PM PT - Jul 10th, 2026
⏳ @cirospaciari, your commit 13a7f81 is still building in |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds C++ MessagePort peer-close and transfer validation; exposes threadName and port-activity to JS; refactors JS MessagePort emitter/close behavior; validates worker filenames/names; implements captured stdio via MessageChannel; updates moveMessagePortToContext and Worker lifecycle/getters; and adds comprehensive tests. ChangesWorker Threads Node.js Compatibility
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 5 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/worker_threads.ts`:
- Around line 389-398: The parameter "context" in function
moveMessagePortToContext is declared but unused; to satisfy the linter and
indicate intentional non-use, rename it to "_context" (i.e., change the
parameter from context to _context) in the moveMessagePortToContext signature
and keep the rest of the function unchanged (retaining the MessagePort checks
and the throwNotImplemented("worker_threads.moveMessagePortToContext") call).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1bf0df59-8c8c-40f9-aecb-b2e02d6868a9
📒 Files selected for processing (35)
src/js/node/worker_threads.tssrc/jsc/bindings/webcore/MessagePort.cppsrc/jsc/bindings/webcore/MessagePort.hsrc/jsc/bindings/webcore/MessagePortPipe.cppsrc/jsc/bindings/webcore/MessagePortPipe.hsrc/jsc/bindings/webcore/Worker.cpptest/js/node/test/parallel/test-worker-crypto-sign-transfer-result.jstest/js/node/test/parallel/test-worker-dns-terminate.jstest/js/node/test/parallel/test-worker-heapdump-failure.jstest/js/node/test/parallel/test-worker-init-failure.jstest/js/node/test/parallel/test-worker-memory.jstest/js/node/test/parallel/test-worker-message-channel.jstest/js/node/test/parallel/test-worker-message-port-arraybuffer.jstest/js/node/test/parallel/test-worker-message-port-close.jstest/js/node/test/parallel/test-worker-message-port-drain.jstest/js/node/test/parallel/test-worker-message-port-message-before-close.jstest/js/node/test/parallel/test-worker-message-port-message-port-transferring.jstest/js/node/test/parallel/test-worker-message-port-multiple-sharedarraybuffers.jstest/js/node/test/parallel/test-worker-nexttick-terminate.jstest/js/node/test/parallel/test-worker-process-exit-async-module.jstest/js/node/test/parallel/test-worker-stdio-flush-inflight.jstest/js/node/test/parallel/test-worker-stdio-flush.jstest/js/node/test/parallel/test-worker-stdio.jstest/js/node/test/parallel/test-worker-terminate-microtask-loop.jstest/js/node/test/parallel/test-worker-terminate-ref-public-port.jstest/js/node/test/parallel/test-worker-terminate-source-map.jstest/js/node/test/parallel/test-worker-terminate-unrefed.jstest/js/node/test/parallel/test-worker-thread-name.jstest/js/node/test/parallel/test-worker-uncaught-exception.jstest/js/node/test/parallel/test-worker-unsupported-path.jstest/js/node/test/parallel/test-worker-voluntarily-exit-followed-by-addition.jstest/js/node/test/parallel/test-worker-voluntarily-exit-followed-by-throw.jstest/js/node/test/sequential/test-worker-fshandles-error-on-termination.jstest/js/node/test/sequential/test-worker-fshandles-open-close-on-termination.jstest/js/node/worker_threads/worker_threads.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/worker_threads.ts`:
- Around line 216-226: The custom inspect function attached to MessagePort via
the kInspectCustom symbol declares unused parameters depth and options which
trigger the linter; update the function signature on MessagePort.prototype (the
value function assigned to kInspectCustom) to prefix those parameters with
underscores (e.g., _depth, _options) so the signature still matches
util.inspect.custom while satisfying the linter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee3c8ab1-4b97-4c4f-8c3a-4c199254153d
📒 Files selected for processing (5)
src/js/node/worker_threads.tssrc/jsc/bindings/webcore/MessagePort.cppsrc/jsc/bindings/webcore/Worker.cpptest/js/node/test/parallel/test-worker-message-port-transfer-self.jstest/js/node/worker_threads/worker_threads.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/node/worker_threads.ts (1)
405-409:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
moveMessagePortToContext()only recognizes JS-initiated closes.
closedMessagePortsis updated only by the JSMessagePort.prototype.close()wrapper, but this PR also closes ports from native code (MessagePort::peerClosed()insrc/jsc/bindings/webcore/MessagePort.cpp, plus teardown paths). Those ports will now be observably closed without ever entering thisWeakSet, so this branch still falls through tothrowNotImplemented()instead ofERR_CLOSED_MESSAGE_PORT.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/node/worker_threads.ts` around lines 405 - 409, moveMessagePortToContext currently only checks the JS-side WeakSet closedMessagePorts and misses ports closed from native (e.g., MessagePort::peerClosed()), causing the code path to fall through instead of throwing ERR_CLOSED_MESSAGE_PORT; update moveMessagePortToContext (and related MessagePort close handling) to detect native-initiated closes by reading a JS-visible closed flag that native code sets (e.g., a new internal slot or property on the MessagePort object that native MessagePort::peerClosed() updates), or alternatively ensure native close paths also add the port to the existing closedMessagePorts WeakSet; reference moveMessagePortToContext and closedMessagePorts and make sure the native MessagePort::peerClosed() path updates the same JS-visible state so moveMessagePortToContext throws $ERR_CLOSED_MESSAGE_PORT for native-closed ports.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/worker_threads.ts`:
- Around line 208-209: The callback passed to close(cb) is queued with
queueMicrotask(cb) which loses the MessagePort receiver; update the close
implementation (MessagePort.prototype.close / close(cb)) to invoke the queued
microtask as a wrapper that calls the original cb with the MessagePort as
receiver (e.g., queueMicrotask(() => cb.call(this))) after the existing typeof
cb check so the callback runs with this === MessagePort while still being
scheduled asynchronously.
---
Outside diff comments:
In `@src/js/node/worker_threads.ts`:
- Around line 405-409: moveMessagePortToContext currently only checks the
JS-side WeakSet closedMessagePorts and misses ports closed from native (e.g.,
MessagePort::peerClosed()), causing the code path to fall through instead of
throwing ERR_CLOSED_MESSAGE_PORT; update moveMessagePortToContext (and related
MessagePort close handling) to detect native-initiated closes by reading a
JS-visible closed flag that native code sets (e.g., a new internal slot or
property on the MessagePort object that native MessagePort::peerClosed()
updates), or alternatively ensure native close paths also add the port to the
existing closedMessagePorts WeakSet; reference moveMessagePortToContext and
closedMessagePorts and make sure the native MessagePort::peerClosed() path
updates the same JS-visible state so moveMessagePortToContext throws
$ERR_CLOSED_MESSAGE_PORT for native-closed ports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0a69db20-fe42-49c1-bafd-1dc766a79ccd
📒 Files selected for processing (3)
src/js/node/worker_threads.tssrc/jsc/bindings/webcore/MessagePort.cpptest/js/node/test/parallel/test-worker-message-port-transfer-closed.js
462b948 to
6267c8e
Compare
75c4056 to
10ace2f
Compare
Since #31216 worker stdout/stderr is always port-backed, so the process.exit() drop also hits workers created without {stdout: true}; a worker that logs N lines then exits surfaces only the first.
Since #31216 worker stdout/stderr is always port-backed, so the process.exit() drop also hits workers created without {stdout: true}; a worker that logs N lines then exits surfaces only the first.
… stale 120s timeout Review follow-ups: the fixture header now describes the current reqId-map design (#31216) instead of the superseded raw-pointer fix, the mid-loop comment no longer claims a GC/teardown overlap this PR measured to be gone, every iteration fails on an empty snapshot stream (one payload per process is parsed as JSON), and the 120s release timeout arm sized for the old 15x300 workload collapses into a single 60s ceiling.
Rescoped onto main after #31216 landed a parallel MessagePort close-event and keep-alive implementation. Two Node-parity gaps remained, covering the repro in #32562: - A CloseKind::Collected teardown (GC of an unreferenced wrapper) notified the entangled peer, so the canonical port1.on('message', cb); port2.postMessage(...) pattern exited at GC timing while node stays alive: collecting port2 delivered peerClosed() to port1, which dropped its listener event-loop ref. Node never closes a channel because a wrapper was collected. Only an Explicit close now notifies the direct peer; harvested in-transit ports keep notifying (their carrier is gone however it died), and the resulting "stranded" listening peer is node parity, not a leak (node retains more: it never collects an entangled port at all). This also fixes the .onmessage variant, which lost its hold the same way. - The onmessageerror setter still took jsRef(), so a messageerror handler alone pinned the event loop; node only refs for 'message' listeners. Tests force Bun.gc(true) after dropping the sender so the collected-peer path is deterministic, plus exit-contract cases (unref, listener removal, messageerror-only). Fixes #32562
Rescoped onto main after #31216 landed a parallel MessagePort close-event and keep-alive implementation. Two Node-parity gaps remained, covering the repro in #32562: - A CloseKind::Collected teardown (GC of an unreferenced wrapper) notified the entangled peer, so the canonical port1.on('message', cb); port2.postMessage(...) pattern exited at GC timing while node stays alive: collecting port2 delivered peerClosed() to port1, which dropped its listener event-loop ref. Node never closes a channel because a wrapper was collected. Only an Explicit close now notifies the direct peer; harvested in-transit ports keep notifying (their carrier is gone however it died), and the resulting "stranded" listening peer is node parity, not a leak (node retains more: it never collects an entangled port at all). This also fixes the .onmessage variant, which lost its hold the same way. - The onmessageerror setter still took jsRef(), so a messageerror handler alone pinned the event loop; node only refs for 'message' listeners. Tests force Bun.gc(true) after dropping the sender so the collected-peer path is deterministic, plus exit-contract cases (unref, listener removal, messageerror-only). Fixes #32562
…r startup (#39536) ### What does this PR do? Since #31216 every `node:worker_threads` `Worker` runs a bootstrap on the worker thread that rebinds `process.stdout/stderr/stdin` to port-backed streams. Two things in it were pure waste, paid per worker: - It used `Object.defineProperty(process, "stdout", { value })`, which first *builds* the real fd-backed stream (loads `node:fs`, `internal/fs/streams`, …) and then throws it away — for all three streams. - It `delete`d four main-thread-only `process` stubs. Deleting any static-table property reifies the whole table, which builds the nextTick queue and every other lazy `process` property. Now the parent's stdio ports are registered on the worker's global and the existing lazy `process.std*` getters build the port-backed streams directly; the four stubs are just not installed on a worker's `process`; and `node:worker_threads` is loaded by a native hook before preloads/entry instead of being injected as an ESM preload. Semantics are unchanged (stdio and console are still set up eagerly, as in Node). `new Worker()` → first message, Linux x64, CI release artifacts on the same machine: main 15.9 ms → 10.2 ms (min), 18.4 → 11.8 ms (median). Split out of #39528; `process.moduleLoadList` is now its own PR. ### How did you verify your code works? `test/js/node/worker_threads/*.test.ts`, `test/js/web/workers/worker.test.ts` and the node `test-worker-*.js` files match main on a release build; drove captured / auto-piped stdout (incl. `process.exit()` flush), `stdin: true` and parentPort echo on the debug build with `BUN_JSC_validateExceptionChecks=1`.
A Worker (node:worker_threads or Web Worker) that assigned process.title overwrote the process-wide title the main thread and ps see. Node keeps the assignment local to the worker. Store it on the worker's own Process object instead: the setter writes there and the getter prefers it once set, so the assignment still round-trips inside the worker. Also serialize process.umask: the no-arg read is a umask(0)+umask(old) pair, so a worker's read could interleave with a main-thread write and clobber it. Take a lock around both paths, matching node's per_process::umask_mutex. chdir, umask(mask) and the set*id family are already stubbed out for node:worker_threads workers by the JS overrides #31216 added.
Rescoped onto main after #31216 landed a parallel MessagePort close-event and keep-alive implementation. Two Node-parity gaps remained, covering the repro in #32562: - A CloseKind::Collected teardown (GC of an unreferenced wrapper) notified the entangled peer, so the canonical port1.on('message', cb); port2.postMessage(...) pattern exited at GC timing while node stays alive: collecting port2 delivered peerClosed() to port1, which dropped its listener event-loop ref. Node never closes a channel because a wrapper was collected. Only an Explicit close now notifies the direct peer; harvested in-transit ports keep notifying (their carrier is gone however it died), and the resulting "stranded" listening peer is node parity, not a leak (node retains more: it never collects an entangled port at all). This also fixes the .onmessage variant, which lost its hold the same way. - The onmessageerror setter still took jsRef(), so a messageerror handler alone pinned the event loop; node only refs for 'message' listeners. Tests force Bun.gc(true) after dropping the sender so the collected-peer path is deterministic, plus exit-contract cases (unref, listener removal, messageerror-only). Fixes #32562
Compatibility — node
worker_threadstest suites (debug build vs bun 1.3.13, which passes 51/138 parallel + 2/5 sequential):Improves Node.js
worker_threadscompatibility acrossMessagePort, captured stdio, environment sharing, structured-clone transfer semantics,Workerconstruction and error handling, and ports the upstream Node worker tests these fix (plus a batch that already passed).MessagePort
on/off/once/emit/addListener/removeListener/…) offMessagePort.prototypeonto an intermediate prototype soObject.getOwnPropertyNames(MessagePort.prototype)matches Node.close([callback]): the callback runs asynchronously after the synchronous detach.'close'event natively, once (guarded), after delivering queued messages, to both the closed port and its entangled peer (including nested in-transit transferred ports); release the peer's event-loop ref.close()is called mid-dispatch.moveMessagePortToContextthrowsERR_CLOSED_MESSAGE_PORTfor a closed port.postMessagetransfer-list errors match Node: source/peer port →DataCloneError "Transfer list contains source port"; already-detached port →"...already detached"(validated before any ArrayBuffer detach); aMessagePortin the message but not in the transfer list →DataCloneError "Object that needs transfer was found in message but not listed in transferList".util.inspectoutput.postMessageto a port's own entangled peer warns (not throws) and loses the channel, matching Node; every transfer-list port is still detached (transfer is atomic), and the source-port check is order-independent within the transfer list.markAsUntransferable / markAsUncloneable / isMarkedAsUntransferable
DataCloneErrorbefore any detach; a marked-uncloneable object throws when cloned/posted (ArrayBuffers/views excepted, matching Node).postMessageToThread
worker_threads.postMessageToThread(threadId, value[, transferList][, timeout])and the process'workerMessage'event (Node 22+). The main thread is a hub holding a controlMessagePortto every thread; each worker keeps one port to the hub, so any thread can reach any other. Errors map toERR_WORKER_MESSAGING_*(same-thread / failed / errored / timeout).MessagePort.unref()to release the event-loop ref taken by amessagelistener (previously only theonmessage/ref()keepalive was released), so an always-listening control port no longer pins a worker's loop.env: SHARE_ENV
Workercreated withenv: SHARE_ENVshares a live, process-wide environment with the parent (write-throughprocess.envover a lock-guarded store; reads/writes/deletes/enumeration). The normal snapshot path is unchanged;env: 42still throwsERR_INVALID_ARG_TYPE.Captured stdio
new Worker(f, { stdin/stdout/stderr: true }):worker.stdout/stderrare Readables fed by the worker'sprocess.stdout/stderr(includingconsole.log);worker.stdinis a Writable. Streams are ended when the worker exits; the stdin listener attaches lazily so an unread{stdin:true}worker doesn't stay alive.Worker construction & lifecycle
Worker#threadName+threadNameexport; validate the filename like Node (ERR_WORKER_PATH/ERR_INVALID_URL_SCHEME);terminate()keeps the loop alive until it resolves; normalizeoptionsfornew Worker(f, null);eval: falsevalidates the path.Worker introspection (Node 24)
worker.getHeapStatistics(),worker.cpuUsage([prevValue]),worker.startCpuProfile([options])(returns a handle with an idempotentstop()), andworker.startHeapProfile([options])— sampled from the worker thread across threads (mirroringgetHeapSnapshot), with Node-matching synchronous option validation andERR_WORKER_NOT_RUNNINGonce the worker has exited.Top-level await
Errors
SyntaxErrorto the parent'serrorevent.Worker process surface
process.abort/chdir(andsetuid/seteuid/setgid/setegid/setgroups/initgroupsoff Windows) are disabled stubs that throwERR_WORKER_UNSUPPORTED_OPERATION(with.disabled === true),process.umask(setMask)throws (the getter still works),debugPortdefaults to 9229, and the main-only_startProfilerIdleNotifier/_stopProfilerIdleNotifier/_debugProcess/_debugEndinternals are removed — fixingprocess.abort()taking down the whole process from a worker. The IPC surface (send/disconnect/channel/connected) is disabled only when an IPC channel was inherited (NODE_CHANNEL_FDset), soif (process.send)still works in a normal worker.Crash & teardown safety
stackgetter throws (a throwingError.prepareStackTrace) no longer crashespostMessage.beforeExithandler exits cleanly with code 1 instead ofSIGTRAP.Worker entry resolution
Workerpointed at a missing entry file reportsCannot find module '<path>'(codeMODULE_NOT_FOUND), matching Node, instead of the native loader'sModuleNotFoundmessage.Tests
Ports the upstream Node worker tests for all of the above, plus a set that already passed. Existing Bun worker tests were updated where behavior is now Node-aligned (workers constructed from
URLobjects, not file:// strings; close delivers queued messages; parse errors areSyntaxError;eval:falsevalidates the path).no test proof · iteration 131 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/worker_threads/worker_heap_snapshot_gc.test.ts