worker_threads: don't build fd stdio or reify all of process at worker startup - #39536
Conversation
…r startup A node:worker_threads Worker's bootstrap rebinds process.stdout/stderr/stdin to port-backed streams. It did so with Object.defineProperty, which first built the real fd-backed streams (node:fs + fs streams) and threw them away, and it deleted four main-thread-only process stubs, which reifies process's whole static table (building the nextTick queue and every other lazy property). Both ran on every worker thread. - 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 simply not installed on a worker's process. - node:worker_threads is loaded by a native hook before preloads/entry instead of being injected as an ESM preload.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 38 minutes Limit details: You’ve used the included review currently available. Your 61 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. WalkthroughChangesThe PR adds MessagePort-backed stdio streams for Node workers. It registers worker stdio ports through native bindings, constructs process streams from them, initializes Node worker stdio
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
…tic-table flag, stripped on reify)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it restructures worker startup across Rust, C++, and built-in JS (bootstrap ordering, a new GC-visited WriteBarrier, and moving process static-table entries to putDirectNativeFunction), a human look would still be worthwhile.
Checked: m_nodeWorkerStdioPorts is in FOR_EACH_GLOBALOBJECT_GC_MEMBER so it's visited; the port.on → addEventListener move in internal/worker/stdio correctly unwraps event.data (and onAck ignores its arg); constructStdioWriteStream's early-return path yields the bare stream, which is what constructStdout/constructStderr pass through; Bun__NodeWorker__bootstrap's failure path mirrors the entry-load failure path (flush logs, entrySettled, shutdown).
Extended reasoning...
Overview
This PR optimizes node:worker_threads worker startup by (1) having process.std*'s existing lazy PropertyCallback getters build port-backed streams directly (via a new nodeWorkerStdioPorts slot on ZigGlobalObject and a new getNodeWorkerStdioStream builtin) instead of building fd-backed streams and overwriting them, (2) not installing four main-thread-only process stubs on worker VMs (moved from the static processObjectTable to putDirectNativeFunction in finishCreation, gated on a new isNodeWorkerVM flag on JSVMClientData) so the JS bootstrap no longer needs to delete them and reify the whole table, and (3) loading node:worker_threads from a native hook (Bun__NodeWorker__bootstrap, called from WebWorker::spin before preloads/entry) instead of injecting it as an ESM preload. The port-stream helpers move from worker_threads.ts to a new internal/worker/stdio.ts shared by both sides, switching from the fake-emitter .on/.off to raw addEventListener/removeEventListener.
Security risks
None identified. The change is internal to worker bootstrap and doesn't touch auth, crypto, permission checks, or untrusted-input parsing. jsFunctionSetNodeWorkerStdioPorts is only reachable via the internal $cpp binding.
Level of scrutiny
High. Worker startup is a critical path and the change spans Rust FFI, C++ JSC bindings (including a new GC-visited WriteBarrier<JSObject>), the process object's static hash table, and built-in JS. The bootstrap ordering change (native requireId before preloads vs. ESM preload injection) subtly shifts when worker_threads.ts module-load side effects (fake-emitter injection, applyWorkerProcessOverrides, parentPort registration) run relative to user preloads, and the processObjectTable edit changes property attributes for four stubs on the main thread. These are the kinds of things where a maintainer's intuition about downstream expectations is valuable.
Other factors
- The new
WriteBarrieris correctly declared inFOR_EACH_GLOBALOBJECT_GC_MEMBER, sovisitChildrenImplcovers it. WorkerMessagingProxy/WorkerOptionswere already used inZigGlobalObject.cpp, so the addedstatic_castcompiles without new includes.Process_stubEmptyFunctionis still used by_linkedBinding/_tickCallbackin the static table, so it's not orphaned.- The
internal/worker/stdiomove changesonMessage(payload)toonMessage(event)+event.dataand.on/.offtoaddEventListener/removeEventListener;onAckdoesn't read its argument so the ack path is unchanged;kFlushSyncis now registered per-stream ingetNodeWorkerStdioStreaminstead of one combinedprocess.on('exit')— semantically equivalent. - The
Readableimport inworker_threads.tsis still used byHeapSnapshotStream. - The PR description says existing worker tests pass on release and were driven under
BUN_JSC_validateExceptionChecks=1on debug, but no new automated test is added in this PR.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks worker thread startup across the Rust spin path, JSC bindings (a new WriteBarrier on ZigGlobalObject, the process static-table lut change), and the built-in JS bootstrap, a human look would still be worthwhile.
Checked: m_nodeWorkerStdioPorts is added to FOR_EACH_GLOBALOBJECT_GC_MEMBER so it's visited; the .on→addEventListener switch in internal/worker/stdio.ts correctly reads event.data and doesn't depend on injectFakeEmitter having run; the four stubs moved from the lut to putDirectNativeFunction use plain attributes (0) as noted in the follow-up commit; Bun__NodeWorker__bootstrap's failure path in spin() mirrors the existing entry-load failure path (exit_code, flush_logs, entrySettled, shutdown); the static_cast<WorkerMessagingProxy*>(worker_ptr) matches the existing cast at line 635 of the same function.
Extended reasoning...
Overview
This PR optimizes node:worker_threads worker startup by (1) having the native lazy process.stdout/stderr/stdin getters build port-backed streams directly instead of Object.defineProperty-ing over already-materialized fd streams, (2) not installing four main-thread-only stub functions on a worker's process (via a new isNodeWorkerVM() flag) instead of delete-ing them (which reified the whole static table), and (3) loading node:worker_threads via a native hook (Bun__NodeWorker__bootstrap) before preloads/entry instead of injecting it as an ESM preload. Port-stream helpers move to a new internal/worker/stdio.ts shared by both parent and worker sides. Touches 10 files across Rust (web_worker.rs, jsc_hooks.rs), C++ bindings (BunProcess.cpp, ZigGlobalObject.{h,cpp}, Worker.cpp, BunClientData.h), and built-in JS.
Security risks
None identified. No untrusted-input parsing, auth, or crypto changes. The new jsFunctionSetNodeWorkerStdioPorts is only reachable via the internal binding array.
Level of scrutiny
High. Worker thread startup is a critical path with a history of subtle bugs around termination races, exception-scope validation, and GC rooting. This change threads through the Rust spin() sequence (adding a new pre-preload hook with its own failure path), adds a WriteBarrier to ZigGlobalObject, moves properties out of a JSC static-table lut into putDirectNativeFunction calls, and changes how the process object's stdio getters branch. While each piece looks correct and follows existing patterns closely, the surface area and the interaction between the pieces (e.g., setupWorkerStdio now depends on _setStdioPorts having run before process.stdout is first read; the addEventListener switch means internal/worker/stdio no longer depends on injectFakeEmitter) warrant a maintainer's eye.
Other factors
The PR description says existing worker_threads / web-worker test suites and Node's test-worker-*.js files pass on release, and captured/auto-piped stdio + stdin: true were driven under BUN_JSC_validateExceptionChecks=1 on debug — good coverage for a refactor that claims semantics are unchanged. The follow-up commit fixing the PropertyAttribute::Function → plain-attribute issue for the moved stubs shows the author already caught one subtlety here. No prior human review comments to address. The bug hunting system found nothing; my own read confirmed GC rooting, exception handling in constructNodeWorkerStdioStream/Bun__NodeWorker__bootstrap, and the failure-path mirroring in spin() all look right — but this is exactly the kind of change a maintainer familiar with the worker lifecycle should sign off on.
- JSVMClientData::create takes the WorkerMessagingProxy and derives both isWorkerVM and isNodeWorkerVM itself. - The worker thread knows whether it is a node:worker_threads worker (passed through WebWorker__create) and, if so, loads node:worker_threads through a plain hook that throws like require(); the caller takes and reports the exception instead of the hook returning a status. - The four main-thread-only process stubs are installed with straight-line code.
BUN_DEBUG_TEST_NODE_WORKER_BOOTSTRAP_THROWS (debug-assertion builds only, like BUN_DEBUG_TEST_WORKER_TEARDOWN_GATE) makes the bootstrap throw instead of loading node:worker_threads; the test checks the Worker gets 'error' then exit code 1, the entry never runs, and the parent exits normally.
|
Updated 3:28 PM PT - Aug 18th, 2026
@dylan-conway, your commit e6dcc3f is building: |
This reverts commit c1abc1a.
| // Node runs its worker bootstrap (parentPort, stdio, process overrides) | ||
| // ahead of user code; ours is node:worker_threads' module body. | ||
| if self.is_node_worker { | ||
| let global = vm.global(); | ||
| if let Err(err) = jsc::host_fn::from_js_host_call_generic(global, || { | ||
| Bun__Worker__loadNodeWorkerThreadsModule(global) | ||
| }) { | ||
| let exception = global.take_exception(err); | ||
| let _ = vm.as_mut().uncaught_exception(global, exception, false); | ||
| if !self.exit_called.load(Ordering::Relaxed) { | ||
| vm.as_mut().exit_handler.exit_code = 1; | ||
| } | ||
| self.flush_logs(vm); | ||
| WebWorker__entrySettled(global); | ||
| return self.shutdown(); | ||
| } |
There was a problem hiding this comment.
🟡 The new node-worker bootstrap-failure branch (is_node_worker → Bun__Worker__loadNodeWorkerThreadsModule throws → uncaught_exception → exit_code=1 → entrySettled → shutdown) ships without a test — c1abc1a added one for exactly this path and e756c97 reverted it 3 minutes later with no stated reason. Could you either restore the test or note in the PR why it was pulled? If it was flaky (or dropped because the debug env-flag felt like production-code-for-a-test), that's worth stating; there's precedent for the pattern (BUN_DEBUG_TEST_WORKER_TEARDOWN_GATE), and REVIEW.md wants every behavioral change covered in the same PR.
Extended reasoning...
What changed
Before this PR, a node:worker_threads Worker's bootstrap ran by injecting "node:worker_threads" at the front of options.preload on the parent side (removed in this diff at worker_threads.ts). Preloads are loaded inside load_entry_point_for_web_worker, so a throw during that load fell out of the existing Err(_) arm at web_worker.rs:874-883 — a path that's already exercised by the entry-point-throws tests.
This PR replaces that with a native call in spin():
if self.is_node_worker {
let global = vm.global();
if let Err(err) = jsc::host_fn::from_js_host_call_generic(global, || {
Bun__Worker__loadNodeWorkerThreadsModule(global)
}) {
let exception = global.take_exception(err);
let _ = vm.as_mut().uncaught_exception(global, exception, false);
if !self.exit_called.load(Ordering::Relaxed) {
vm.as_mut().exit_handler.exit_code = 1;
}
self.flush_logs(vm);
WebWorker__entrySettled(global);
return self.shutdown();
}
}That Err block is genuinely new control flow: it runs before load_entry_point_for_web_worker, so nothing in the existing suite reaches it.
The test that was added, then removed
git log on this branch shows:
- c1abc1a ("Test the node worker bootstrap failure path") added
BUN_DEBUG_TEST_NODE_WORKER_BOOTSTRAP_THROWS(debug-assertion builds only, mirroring the existingBUN_DEBUG_TEST_WORKER_TEARDOWN_GATEpattern) plus a test intest/js/node/worker_threads/worker_threads.test.tsasserting the parent Worker gets['error', <msg>]then['exit', 1], no'online'/'message', and the parent process exits 0. - e756c97, 3 minutes later, reverts it with no message beyond "Revert …".
The final changed-files list contains zero test files.
Why this is worth flagging
REVIEW.md, Tests reviewers reject: "Every behavioral change ships an automated test in the same PR. 'Verified manually', unnamed 'existing tests', and benchmarks don't count, even for one-liners." The PR description cites exactly those two things (existing test/js/node/worker_threads/*.test.ts + "drove … on the debug build").
Nothing now covers that (a) the parent Worker receives an 'error' event carrying the bootstrap failure, (b) the exit code is 1 (not 0), (c) WebWorker__entrySettled fires so a parent's postMessageToThread waiting on this thread doesn't hang, and (d) 'online'/entry evaluation never happen. If the test was reverted because it flaked, that itself hints the path may not behave deterministically and is worth knowing.
Step-by-step: why no existing test reaches this branch
Bun__Worker__loadNodeWorkerThreadsModulecallsinternalModuleRegistry()->requireId(…, NodeWorkerThreads)— it loads Bun's own built-insrc/js/node/worker_threads.ts.- That module body only throws on an internal Bun bug or a workerData deserialize failure inside
createNodeWorkerThreadsBinding— neither is exercised by any test intest/js/node/worker_threads/or the Node paralleltest-worker-*.jsset, which all pass valid workerData. - The pre-existing "entry throws" / "preload throws" tests now go through
load_entry_point_for_web_worker'sErrarm (unchanged), after this new block has already succeeded. - Therefore the only way to hit lines 856-865 in CI is to force the throw, which is precisely what the reverted
BUN_DEBUG_TEST_NODE_WORKER_BOOTSTRAP_THROWSflag did.
Possible reason for the revert, and why it's still worth asking
REVIEW.md also says "Never add production code solely to make a test writable", and the reverted test required a cfg!(debug_assertions)-gated env flag in web_worker.rs. If that's why it was pulled, it's worth saying so in the PR — but note there's already in-tree precedent for exactly this pattern (BUN_DEBUG_TEST_WORKER_TEARDOWN_GATE, referenced two lines above the new block via arm_test_gate), and the flag compiles out of release builds. Alternatively, a user-reachable trigger (e.g. workerData that fails structured-clone deserialize on the worker side) would exercise the branch without a debug flag.
How to fix
Either restore c1abc1a (the debug-flag approach matches the neighboring teardown-gate test), or add a note to the PR description explaining why the test was dropped so the reviewer can weigh it. Not blocking — this is defensive handling whose observable behavior is designed to match the pre-existing preload-failure path, and merging without the test causes no concrete runtime failure — but the unexplained revert is a review question worth answering.
constructNodeWorkerStdioStream (from #39536) builds process.stdout, stderr and stdin of a node:worker_threads worker and had the same clear, report and store undefined shape as the builders converted here.
The worker test was vacuous on current main: #39536 removed the bootstrap read of Bun.main, so a worker with an empty body never populates main_resolved_path. The bun:test modifier test measured a path that creates no WTF string since bind() takes a &'static str. The SocketAddress test now goes through the shared RSS helper and branches its bound on debug/ASAN builds. Fewer iterations with a larger string keep the leaked signal at about 50 MiB per path while the test runs in about 2.5 s under a debug ASAN build.
constructNodeWorkerStdioStream (from #39536) builds process.stdout, stderr and stdin of a node:worker_threads worker and had the same clear, report and store undefined shape as the builders converted here.
2 participants
What does this PR do?
Since #31216 every
node:worker_threadsWorkerruns a bootstrap on the worker thread that rebindsprocess.stdout/stderr/stdinto port-backed streams. Two things in it were pure waste, paid per worker:Object.defineProperty(process, "stdout", { value }), which first builds the real fd-backed stream (loadsnode:fs,internal/fs/streams, …) and then throws it away — for all three streams.deleted four main-thread-onlyprocessstubs. Deleting any static-table property reifies the whole table, which builds the nextTick queue and every other lazyprocessproperty.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'sprocess; andnode:worker_threadsis 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.moduleLoadListis now its own PR.How did you verify your code works?
test/js/node/worker_threads/*.test.ts,test/js/web/workers/worker.test.tsand the nodetest-worker-*.jsfiles match main on a release build; drove captured / auto-piped stdout (incl.process.exit()flush),stdin: trueand parentPort echo on the debug build withBUN_JSC_validateExceptionChecks=1.