Repository navigation
Conversation
…rker
Node.js does not expose a global `onmessage` event-handler accessor inside
a worker_threads worker, but Bun installs one for Web Workers and left it in
place for node workers as well. Emscripten's pthread worker shim registers
both `parentPort.on('message', d => onmessage({data: d}))` and
`self.onmessage = handler`; since parentPort is backed by the same global
event target, the handler ran once for each path and the pthread 'load'
command was processed twice, aborting with
"Module.arguments has been replaced with plain arguments_".
Delete `globalThis.onmessage` in node workers so `self.onmessage = fn`
is a plain property assignment, and make `parentPort.onmessage` track its
own listener via addEventListener/removeEventListener rather than aliasing
`self.onmessage`.
Fixes #7816
Fixes #29211
|
Warning Review limit reached
Next review available in: 7 minutes 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 for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling 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 (3)
Comment |
|
Updated 3:05 AM PT - Aug 1st, 2026
❌ @robobun, your commit 3de4278 has some failures in 🧪 To try this PR locally: bunx bun-pr 36639That installs a local version of the PR into your bun-36639 --bun |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
globalThis.onmessage is no longer defined inside a node:worker_threads worker, so referencing it as a bare identifier threw a ReferenceError when the napi test harness was loaded inside a worker.
6c682be to
7694a07
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/resolver/resolver.rs:1949-1966— This PR ships two unrelated, untested bundler-resolver behavior changes: thebrowser_map_overrides_builtingate here (letting the importer's"browser"map override the builtin Node polyfill table underpolyfill_node_globals), and treatingjsnext:mainlikemodulefor the auto-main require/import fallback at line ~5495. Neither is mentioned in the PR description or the commit message that added them (6c682be), and neither has atest/bundler/test —grep -r jsnext:main test/returns nothing. Please drop theresolver.rshunk from this worker_threads PR (or move it to its own PR with bundler tests).Extended reasoning...
What changed
src/resolver/resolver.rsin this PR contains two independent behavioral changes to bundler module resolution, both landed in commit6c682becwhose message is "test: reference onmessage/onerror via globalThis in napi common harness":- Lines ~1949–2016 — a new
browser_map_overrides_builtincheck inside thepolyfill_node_globalsblock. Previously, whenpolyfill_node_globalswas on, an import of e.g."buffer"unconditionally short-circuited toNodeFallbackModules::map()(or the disabled-node:stub). Now the resolver first consults the importer's enclosingpackage.json"browser"map viacheck_browser_map::<PackagePath>; if it matches, the polyfill/disabled-stub path is skipped and resolution falls through tocheck_package_pathwith the bare (un-node:-prefixed) name. - Line ~5495 — the auto-main require/import fallback previously fired only for
key == b"module"; it now also fires forkey == b"jsnext:main", sorequire("pkg")on a package that listsjsnext:main+main(the comment namesmoment) will now resolve to the CJSmaininstead of the ESMjsnext:mainfile.
Both are user-visible changes to which file a package import resolves to.
Why this is unrelated to the PR
The PR title, description, and both commit messages are exclusively about removing the global
onmessageaccessor inside anode:worker_threadsworker to fix Emscripten pthread double-delivery (#7816 et al.).polyfill_node_globalsis a bundler option (it controls browser-target polyfill substitution inBun.build), not runtime worker resolution — z3-solver in a worker does not go through this code. Thejsnext:mainchange is likewise pure bundler main-field ordering. Neither appears in the PR description; the commit that added them says only "reference onmessage/onerror via globalThis in napi common harness". These slipped in from another branch.No tests
The PR touches exactly three files:
src/resolver/resolver.rs,test/js/node/worker_threads/worker_threads.test.ts, andtest/napi/node-napi-tests/test/common/index.js. There is notest/bundler/ortest/resolver/addition.grep -r "jsnext:main" test/returns zero hits;grep -r "polyfill_node_globals\|browser_map_overrides_builtin" test/returns zero hits. Neither behavior is covered anywhere in the suite.Step-by-step: concrete effect of change (2)
Take
moment, whosepackage.jsonhas"main": "./moment.js"and"jsnext:main": "./src/moment.js"but no"module"field. UnderBun.build({ target: "browser" })with the default main-field list (which includesjsnext:main), arequire("moment"):- Before this PR: the loop reaches
key = "jsnext:main",auto_main && key == b"module"is false, so it takes the plainelsebranch and returns thejsnext:mainESM entry —require("moment")gets{ default: … }. - After this PR:
auto_main && (key == b"module" || key == b"jsnext:main")is true, so the auto-main fallback runs, finds"main", and (sincekind == Require) returns./moment.js—require("moment")gets the CJS export.
That may well be the desired fix, but it is a real change in what bundles resolve to, shipped without a single test and hidden inside a worker_threads PR where nobody bisecting a bundler regression would look.
REVIEW.md
The repo's review rules are explicit on both counts:
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.
don't ride file-wide standardization on a focused bugfix.
Fix
Drop the
src/resolver/resolver.rshunk from this PR entirely. If the two resolver changes are intentional, they belong in their own PR withitBundledtests undertest/bundler/covering (a) a"browser": { "buffer": "./shim.js" }map winning over the builtin polyfill undertarget: "browser", and (b)require("pkg")on ajsnext:main-only package resolving tomain. - Lines ~1949–2016 — a new
b0c44ee to
9deeba9
Compare
…user function Keeps the handler-attribute slot independent of addEventListener registrations of the same function, matching Node's defineEventHandler semantics.
There was a problem hiding this comment.
Both earlier inline findings are addressed and I didn't find new issues, but this changes user-visible behavior in the Node compat layer (removes globalThis.onmessage inside a node:worker_threads worker and reworks parentPort.onmessage semantics), and there are three other open PRs (#26293, #29215, #35655) taking different approaches to the same bug — worth a human deciding which approach lands.
What was reviewed
delete globalThis.onmessageis gated on_isNodeWorker; the accessor is a configurable own property (putDirectCustomAccessor(..., 0)in ZigGlobalObject.cpp:3187), so the delete succeeds and Web Workers are unaffected.- The new
defineHandlerAttributewrapper keeps the handler-attribute slot independent ofparentPort.addEventListener(the earlier dedup concern). - The napi
common/index.jschange avoids a ReferenceError on bareonmessagewhen this harness is loaded inside a node worker after the accessor is deleted.
Extended reasoning...
Overview
The PR touches three files: src/js/node/worker_threads.ts (rewrites parentPort.onmessage/onmessageerror as self-tracking accessors backed by a stable wrapper on self.addEventListener, and deletes globalThis.onmessage inside a node worker), test/js/node/worker_threads/worker_threads.test.ts (two new tests: the Emscripten double-delivery repro and a parentPort.onmessage round-trip), and test/napi/node-napi-tests/test/common/index.js (references onmessage/onerror via globalThis. so the harness survives the deleted accessor).
Prior review
I left two inline findings on earlier revisions: (1) commit 6c682be had accidentally reverted the entire implementation — fixed by force-pushing the branch back to 8c5312c + 7694a07; (2) the first version of the setter passed the raw user function to self.addEventListener, so it deduped against parentPort.addEventListener('message', fn) — fixed in b526997 by registering a stable wrapper closure. Both are resolved in the current diff.
Security risks
None. This only affects which global properties exist inside a node-style worker and how parentPort.onmessage is wired to the global event target. No untrusted-input parsing, auth, or filesystem paths are touched.
Level of scrutiny
Medium. The mechanism is small and well-tested, but it is a user-visible Node-compat behavior change: code running inside a node:worker_threads worker that previously relied on globalThis.onmessage being an event-handler accessor will now see a plain (initially absent) property. That is the Node behavior and is exactly the point of the fix, but the repo's .claude/docs/landing-prs.md calls out Node/Web-compat and API-surface changes as needing deliberate review. fakeParentPort() runs for any non-main thread that loads this module, so parentPort.onmessage also stops aliasing self.onmessage inside a raw globalThis.Worker that imports worker_threads — a minor semantic shift on an already-incomplete emulation.
Other factors
The PR description explicitly positions this as the minimal alternative to three larger open PRs (#26293 real MessagePort for parentPort, #29215 separate event target with forwarders, #35655 remove all Web-Worker globals). A maintainer should decide which of those approaches to land rather than an automated approval picking one. The two new tests await real conditions (once(worker, 'message')), clean up via worker.terminate() in finally, and would fail on the pre-fix build (hadOnMessage true, count 2).
|
Closing: the double delivery this PR targeted was fixed on main by #37075 (merged as 9d519e8). In a Every issue this PR listed is closed as fixed by #37075: #7816, #19453, #25454, #25860, #29211, #29635. The issue comments include z3-solver 4.13 runs against main for #7816, #19453, #25454, #29211 and #29635. Checked with a worker that registers both
main also carries a test for this case, "a parent message reaches parentPort only, not self.onmessage, in a node worker" in script used for the counts aboveimport { Worker } from "node:worker_threads";
const src = `
const { parentPort } = require("node:worker_threads");
if (typeof globalThis.self === "undefined") globalThis.self = globalThis;
const counts = { parentPort: 0, selfOnmessage: 0, globalAddEventListener: 0 };
parentPort.on("message", (m) => { counts.parentPort++; self.onmessage({ data: m }); });
self.onmessage = () => { counts.selfOnmessage++; };
if (typeof self.addEventListener === "function") self.addEventListener("message", () => { counts.globalAddEventListener++; });
parentPort.on("message", () => setImmediate(() => parentPort.postMessage(counts)));
`;
const w = new Worker(src, { eval: true });
const result = await new Promise((resolve, reject) => {
w.once("message", resolve);
w.once("error", reject);
w.postMessage("ping");
});
await w.terminate();
console.log(JSON.stringify(result)); |
What does this PR do?
Fixes #7816
Fixes #29211
Fixes #25860
Fixes #19453
Fixes #29635
Fixes #25454
Node.js does not expose a global
onmessageevent-handler accessor inside aworker_threadsworker. Bun installs one for Web Workers and left it in place for node workers as well. Emscripten's pthread worker shim sets up message handling like this:In Bun,
parentPortis backed by the same global event target asself, so the same message was delivered to both theparentPort.onwrapper (which forwards toglobalThis.onmessage) and directly to theself.onmessageevent-handler attribute. The pthreadloadcommand therefore ran twice, and the second run hit Emscripten'slegacyModulePropabort trap:Fix
node:worker_threadsworker (gated on the existing_isNodeWorkerflag), deleteglobalThis.onmessagesoself.onmessage = fnis a plain property assignment, matching Node.parentPort.onmessage(andonmessageerror) track its own listener viaself.addEventListener/removeEventListenerinstead of aliasingself.onmessage, so it keeps working after the global accessor is gone.Web Workers (
new globalThis.Worker(...)) still have theglobalThis.onmessageaccessor; only node-style workers change.Verification
Works with both
z3-solver@4.12.4(from #7816) andz3-solver@5.0.0.Related
#26293, #29215 and #35655 are earlier, larger attempts at this same problem (real
MessagePortforparentPort, separate event target with forwarders, and removing all Web Worker globals respectively). This PR is the minimal version: it only removes the one global that causes the double delivery.[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file