node:worker_threads: per-thread --use-system-ca, real eventLoopUtilization, --cpu-prof in workers, node's online timing, error.code / stack-getter / timeOrigin fixes, async_hooks WORKER resource, worker_threads dc channel (+10 upstream tests) - #34424
Code review found 1 important issue
Found 5 candidates, confirmed 4. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 3 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | test/js/node/test/parallel/test-worker-memory.js:1-8 |
test-worker-memory.js is a new CI failure on macOS aarch64 |
| 🟡 Nit | src/js/node/worker_threads.ts:1099-1104 |
async_hooks WORKER init and diagnostics_channel publish fire in reverse order vs Node |
| 🟡 Nit | src/js/node/worker_threads.ts:936-938 |
#hasRef initialized to true regardless of options.ref |
| 🟡 Nit | src/js/node/async_hooks.ts:383-384 |
Stale header comment in internal/async_hooks_tick.ts |
Annotations
Check failure on line 8 in test/js/node/test/parallel/test-worker-memory.js
claude / Claude Code Review
test-worker-memory.js is a new CI failure on macOS aarch64
This test is red on CI: robobun reports `test-worker-memory.js - code 1 on 🍎 14 aarch64 (new)` in Build #74530. Per the bar already applied on this PR ("we should not add any tests that dont pass here" — same reason `test-worker-dns-terminate.js` and both fshandles tests were dropped), this file should be removed until the RSS-ratio assertion holds on the macOS aarch64 runner, or the underlying growth is fixed.
Check warning on line 1104 in src/js/node/worker_threads.ts
claude / Claude Code Review
async_hooks WORKER init and diagnostics_channel publish fire in reverse order vs Node
The async_hooks `WORKER` init and the `worker_threads` diagnostics_channel publish fire in the opposite order to Node — Node emits the async_hooks init first (synchronously from AsyncWrap's constructor when `this[kHandle] = new WorkerImpl(...)` runs mid-constructor), then does the dc publish at the very end. Swapping these two blocks so `this.#emitAsyncHooksInit()` runs before `workerThreadsChannel.publish(...)` would match Node exactly.
Check warning on line 938 in src/js/node/worker_threads.ts
claude / Claude Code Review
#hasRef initialized to true regardless of options.ref
`#hasRef` is unconditionally initialized to `true`, but the native WebWorker constructor honours Bun's `options.ref` (JSWorker.cpp:191-195), and this constructor spreads user options through — so `new Worker(f, { eval: true, ref: false })` starts unref'd on the event loop while the async_hooks WORKER resource's `hasRef()` reports `true`. Nit only (`ref` is a Bun-only extension), but the initializer could read `options?.ref !== false` to keep the stated "mirrors ref()/unref()" invariant.
Check warning on line 384 in src/js/node/async_hooks.ts
claude / Claude Code Review
Stale header comment in internal/async_hooks_tick.ts
nit: the sibling header comment in `src/js/internal/async_hooks_tick.ts` (lines 9-11) still says "Currently only TickObject `init` events are delivered … promise, timer and native resource events are still unimplemented", which is now stale since `worker_threads.ts` dispatches WORKER init events into the same `tickInitHooks` array. Worth updating alongside this comment for consistency.