worker_threads: bind this to the port in MessagePort on()/once() listeners

[robobun]

Problem

• A listener registered on a node:worker_threads MessagePort with port.on(...) or port.once(...) runs with this === undefined (globalThis in sloppy-mode code). Node calls it with this bound to the port, so port.on("message", function () { this.postMessage(...) }) works in node and in bun either throws (strict code) or hits the global object (sloppy code).
• The same applies to 'close' listeners, to the callback of close(cb) (which is once('close', cb)), to listeners fired through port.emit(), and to parentPort (a MessagePort in a node:worker_threads worker).
• Cause: the node-style emitter shim in src/js/node/worker_threads.ts (injectFakeEmitter) registers a wrapper through addEventListener. wrapped() unwraps the Event and then calls the user's listener bare, listener(run(event)), so the listener gets no this at all.
• Repro and node v26.3.0 output are in the details block below.

Fix

• on() and once() pass the object they were called on into wrapped(), and the wrapper calls listener.$call(target, run(event)). functionForEventType() only threads the extra argument through.
• Matches node: NodeEventTarget dispatch (lib/internal/event_target.js, kHybridDispatch) invokes a node-style listener as FunctionPrototypeCall(callback, this, arg) where this is the target whose on() registered it. Node's EventTarget has no propagation, so that is always the object .on() was called on, which is what this binds.
• The target is bound at registration instead of forwarding the this bun's EventTarget hands the wrapper because the two differ in one case: the stand-in parentPort that fakeParentPort() builds for a plain globalThis.Worker that loads node:worker_threads. Its addEventListener forwards to the worker's global scope, so EventTarget would invoke the listener with the global scope as this; binding gives this === parentPort there as well. For a real port both are the same object.
• One binding covers every entry point: native 'message', native 'close', close(cb), and emit() all end up in the same wrapper. The wrapper still returns the listener's result, so a rejecting async listener is still turned into an uncaughtException by JSEventListener (same as node).
• $call is the tamper-proof call used throughout src/js.
• Verified (test/js/node/worker_threads/worker_threads.test.ts, four new tests):
  • on()/once() listeners are called with \this` bound to the port: on+oncefor a nativeMessageEvent, a native close Event, close(cb), and emit(). Fails before the fix (every trueisfalse`).
  • parentPort.on() listeners are called with \this` bound to parentPort`: real worker; fails before the fix.
  • the stand-in parentPort of a non-node Worker also binds \this` to parentPort: the fakeParentPort()path; fails before the fix and also fails if the wrapper forwardsEventTarget's this instead of binding (this` is the global scope there).
  • a rejecting async on()/once() listener is reported as an uncaughtException: pins the return-value forwarding the rewritten lines are responsible for. Passes before and after, and node prints the same output.
  • Whole file: 126 pass with the debug build. Upstream test/js/node/test/parallel/test-worker-message-port*.js, test-worker-message-channel.js, test-worker-message-event.js, test-worker-messaging.js, test-worker-onmessage.js, test-worker-parent-port-ref.js, test-worker-workerdata-messageport.js all exit 0; test/js/web/workers/message-channel.test.ts passes.
• Related: worker_threads: pass the close Event to MessagePort 'close' listeners #38060 changes what a 'close' listener receives as its argument in this same function and is independent of this change (the new tests do not assert the close listener's argument). MessagePort: implement NodeEventTarget on the native listener list #35811 rewrites the shim natively and would bind this as a side effect; this PR is the small fix for the shim as it exists today.

Background

• MessagePort in bun is WebCore's EventTarget-based port. node:worker_threads adds node's emitter-style surface (on, once, off, emit, listenerCount, ...) to its prototype in injectFakeEmitter. .on(type, fn) does not store fn itself; it registers a small wrapper via addEventListener that converts the Event into the value a node listener expects (MessageEvent.data for 'message', and so on) and then calls fn.
• Node's MessagePort is a NodeEventTarget: an EventTarget with node-style methods on top. A node-style listener receives the unwrapped value instead of an Event and is called with this bound to the target.
• fakeParentPort() (same file): in a plain web Worker there is no parent MessagePort, so parentPort is an object created from MessagePort.prototype whose addEventListener/postMessage forward to the worker's global scope. It inherits the shim's on()/once().

Repro

const { MessageChannel } = require("worker_threads");
const { port1, port2 } = new MessageChannel();
port1.on("message", function (data) { console.log("on message this===port1:", this === port1); });
port1.once("message", function () { console.log("once message this===port1:", this === port1); port1.close(); });
port1.addEventListener("message", function () { console.log("addEventListener this===port1:", this === port1); });
port1.on("close", function () { console.log("on close this===port1:", this === port1); });
port1.on("custom", function (d) { console.log("emit custom this===port1:", this === port1); });
port1.emit("custom", 42);
port2.postMessage(1);

node v26.3.0: every line prints true.

bun before this change: addEventListener prints true, every on/once/emit line prints false.

bun with this change: every line prints true.

First version of this PR

The first push forwarded the this that EventTarget invokes the wrapper with (listener.$call(this, ...)). That is the port for a real MessagePort, but for the stand-in parentPort of a plain globalThis.Worker it is the worker's global scope, since that object registers its listeners there. Binding the registration target covers both, at the same size; the stand-in test was added to pin it.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 56 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 345b023d-ab1a-45fa-9b6d-269932523619

📥 Commits

Reviewing files that changed from the base of the PR and between d61dec3 and 0e322f0.

📒 Files selected for processing (1)

• src/js/node/worker_threads.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4d22417a-9f6c-493d-addb-a1bbae753a74

📥 Commits

Reviewing files that changed from the base of the PR and between 04148c8 and d61dec3.

📒 Files selected for processing (2)

• src/js/node/worker_threads.ts
• test/js/node/worker_threads/worker_threads.test.ts

---

Walkthrough

Event listener wrappers now preserve their registration target as the callback this value. The change applies to on() and once() for MessagePort and parentPort events, with tests covering event types, worker modes, and rejected async listeners.

Changes

Worker listener context

Layer / File(s)	Summary
Bind listener wrappers to registration targets src/js/node/worker_threads.ts	Event wrappers receive the registration target and invoke callbacks with that target as this. Both on() and once() pass the target when creating wrappers.
Validate listener context and async errors test/js/node/worker_threads/worker_threads.test.ts	Tests cover MessagePort and parentPort callbacks across event types and worker modes. Subprocess tests verify rejected async on() and once() listeners emit uncaughtException without stderr output.

Suggested reviewers: cirospaciari, dylan-conway

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main change: binding MessagePort on() and once() listeners to the port.
Description check	✅ Passed	The description explains the problem, fix, affected paths, and verification results, although it does not use the template headings.

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 11:59 AM PT - Aug 13th, 2026}

❌ @robobun, your commit 0e322f0 has 2 failures in Build #94731 (All Failures):

• test/cli/run/require-cache.test.ts - code 1 on 🐧 13 x64-asan
• test/js/workerd/html-rewriter-leak.test.ts - code 1: SyntaxError: JSON Parse error: Unexpected EOF on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 38068

That installs a local version of the PR into your bun-38068 executable, so you can run:

bun-38068 --bun

[robobun]

Status: ready for review (head 0e322f0).

Reproduced with the script in the PR body (bun 1.4.0 and a debug build of main print false for every on/once/emit line; node v26.3.0 prints true) and with the new tests in test/js/node/worker_threads/worker_threads.test.ts: the three this-binding tests fail before the change and pass with it; the async-rejection test pins existing behavior and passes both ways. The whole file (126 tests), test/js/web/workers/message-channel.test.ts and the upstream test-worker-message-port* set pass with the debug build.

Change since the first push: the wrapper now binds the object on()/once() was called on instead of forwarding the this EventTarget passes, so the stand-in parentPort of a plain globalThis.Worker gets this === parentPort too (covered by its own test).

[claude]

LGTM — two-line this-forwarding fix that makes the on()/once() wrappers transparent, matching Node's NodeEventTarget behavior.

What was reviewed:

• Confirmed both wrappers are regular function expressions (not arrows), so this is the port EventTarget already passes as currentTarget; $call is the tamper-proof intrinsic used elsewhere in this file.
• Return value is still forwarded, so async-listener rejection handling in JSEventListener is unchanged.
• New tests cover on/once for native MessageEvent, native close, emit(), and parentPort in a real worker; ordering is deterministic (synchronous emit → awaited message → awaited close), and the close-listener argument is deliberately not asserted to stay independent of #38060.

Extended reasoning...

Overview

Two-line change in src/js/node/worker_threads.ts's injectFakeEmitter: wrapped() now calls listener.$call(this, run(event)) instead of listener(run(event)), and onceWrapper now calls wrapper.$call(this, ev) instead of wrapper(ev). This forwards the this that EventTarget already invokes the wrapper with (the port, per DOM currentTarget semantics implemented in JSEventListener::handleEvent) through to the user's node-style listener. Two new tests in test/js/node/worker_threads/worker_threads.test.ts cover on/once for message, close, and emit()-dispatched custom events, plus parentPort inside a real worker.

Security risks

None. This is a Node.js compatibility fix that changes only the this binding of user-registered listener callbacks. No new inputs are parsed, no privilege boundaries are crossed, and $call is the tamper-proof primordial already used throughout src/js (e.g., nativeMessagePortClose.$call(this) in the same file).

Level of scrutiny

Low-to-moderate. The production change is two lines, mechanically transparent (forward this through a wrapper), and matches both the DOM spec (currentTarget) and Node's documented NodeEventTarget dispatch (FunctionPrototypeCall(callback, this, arg)). Both changed call sites use regular function expressions, so this is dynamically bound as intended. The return value is still returned, preserving async-listener rejection handling.

Other factors

• Tests await real observable conditions (Promise.withResolvers resolved from inside the listeners) rather than sleeping, and assert an exact call array whose ordering is deterministic: emit() dispatches synchronously, then postMessage is awaited, then close is awaited.
• The close-listener assertions deliberately omit the argument so this stays independent of #38060, as the PR description notes.
• No CODEOWNERS entry covers these paths.
• The PR description documents that the whole test file (124 tests), the upstream test-worker-message-port* suite, and message-channel.test.ts all pass with the debug build, and that both new tests fail before the fix — satisfying the repo's "prove the test fails for the right reason" requirement.

[claude]

LGTM — small, well-scoped fix that binds this to the registration target in the injectFakeEmitter wrappers, matching Node's NodeEventTarget semantics.

What was reviewed:

• The target capture approach vs. forwarding the wrapper's own this — capturing is correct because fakeParentPort() forwards addEventListener to the global scope, and the third new test covers exactly that case.
• onceWrapper still calls wrapper(ev) bare, which is fine since wrapper now closes over target and ignores its own this.
• The closure now references the port it's registered on — that's a cycle within the same JS heap (port → registry → wrapper → port), so it's GC-collectable and not a leak.
• The four new tests await real events (no sleeps), wire error paths to reject, drain subprocess pipes concurrently, and assert exact values.

The comment-cop bot flag on the 3-line comment above wrapped() looks like a false positive — it's a concise "why not the alternative" note, not a workaround justification.

Extended reasoning...

Overview

This PR fixes this binding for listeners registered via MessagePort.on() / .once() in node:worker_threads. The injectFakeEmitter shim wraps user listeners before passing them to addEventListener, and the wrapper was calling the user's function bare (listener(run(event))), so this was undefined. The fix threads the registration target through functionForEventType → wrapped and calls listener.$call(target, run(event)). once() captures const target = this before building the wrapper. Four new tests in test/js/node/worker_threads/worker_threads.test.ts cover on/once for native message/close events, emit(), close(cb), a real worker's parentPort, the stand-in parentPort used by non-node globalThis.Worker, and preservation of async-rejection forwarding.

Security risks

None. This is a this-binding change in a Node compat shim; no auth, crypto, filesystem, or untrusted-input parsing is touched. The tamper-proof $call intrinsic is used, consistent with the rest of src/js/.

Level of scrutiny

Low-to-medium. The runtime change is a handful of lines in one function (wrapped) plus threading a parameter through two call sites. The design choice — capturing the registration target rather than forwarding the wrapper's own this — is the more robust option and is required for the fakeParentPort() case, where addEventListener is bound to the global scope so EventTarget would otherwise pass self as this. The PR body traces this against Node's kHybridDispatch and Bun's JSEventListener::handleEvent, and the third test asserts thisIsParentPort: true, thisIsGlobal: false for exactly that path. I checked that onceWrapper calling wrapper(ev) bare is still correct (the inner closure already has target captured), and that the internal port.on("message", ...) uses in makePortReadable/makePortWritable don't rely on this and are unaffected.

Other factors

The tests follow the repo's conventions: conditions are awaited via Promise.withResolvers() and once(), error events are wired to reject, the subprocess test drains stdout/stderr/exited concurrently and asserts a combined {stdout, stderr, exitCode} object, and resources are cleaned up in finally. The wrapper now captures a reference to the port, forming a port → listener-registry → wrapper → port cycle, but that's an ordinary intra-heap cycle the GC handles. The only outstanding thread comment is from the automated comment-cop bot flagging the 3-line comment above wrapped(); that comment is short and explains a non-obvious "why this and not the alternative" (which REVIEW.md explicitly asks for), so I'm treating it as a heuristic false positive rather than a blocker.

[claude]

LGTM — small, focused Node compat fix.

What was reviewed:

• wrapped() now captures the registration target and invokes via $call, matching Node's NodeEventTarget this-binding; the return-value forwarding (async rejection → uncaughtException) is preserved.
• Checked the fakeParentPort() path: binding at registration (rather than forwarding EventTarget's this) is the right choice there since its addEventListener forwards to the global scope; the third test pins this.
• Confirmed the closure over target introduces no new retention — the wrapper is already stored in the port's own listener registry.
• The comment-cop feedback was addressed in 0e322f0 (comment is now one line).

Extended reasoning...

Overview

This PR fixes this-binding for node-style on()/once() listeners on MessagePort in node:worker_threads. Previously the wrapper called the user's listener bare (listener(run(event))), so this was undefined; now it captures the object on()/once() was called on and invokes via listener.$call(target, run(event)), matching Node's NodeEventTarget dispatch. The production change is ~6 lines in src/js/node/worker_threads.ts (threading a target parameter through wrapped() → functionForEventType() → on()/once()), plus four new tests in test/js/node/worker_threads/worker_threads.test.ts.

Security risks

None. This is a pure Node.js compatibility fix in a JS shim — no auth, crypto, filesystem, or untrusted-input parsing. The $call intrinsic is the standard tamper-proof call used throughout src/js (500+ occurrences).

Level of scrutiny

Low-to-medium. The change is mechanical parameter threading with a well-defined reference behavior (Node's kHybridDispatch in lib/internal/event_target.js). The one non-obvious design choice — binding the registration target rather than forwarding the wrapper's dispatch-time this — is explained in the PR body and covered by a dedicated test (the stand-in parentPort case, where the two would differ). I verified the wrapper still returns the listener's result, so the async-rejection → uncaughtException path is preserved (test 4 pins this). The new closure over target doesn't change lifetime: the wrapper was already stored in the port's own registry (symbol-keyed on the port) and in EventTarget's listener list, so port ↔ wrapper was already a cycle.

Other factors

• Tests follow repo conventions: await observable conditions (no sleeps), drain subprocess pipes concurrently via Promise.all, use bunExe()/bunEnv, try/finally cleanup, and assert exact values with toEqual. once from node:events is already imported at the top of the file.
• The github-actions comment-cop flagged a long code comment on an earlier revision; commit 0e322f0 shortened it to a single line, so that feedback is addressed.
• No CODEOWNERS entry for src/js/node/worker_threads.ts.
• The bug-hunting system found no issues.