MessagePort: don't drop messages when a GC runs during dispatch

[robobun]

Repro

const big = () => ({ a: Array.from({ length: 3000 }, (_, i) => ({ i, s: "x".repeat(8) })) });
let fired = 0;
for (let i = 0; i < 50; i++) {
  const { port1, port2 } = new MessageChannel();
  port1.onmessage = () => { fired++; };  // does not capture port1
  port2.postMessage(big());
  port2.close();
}
await new Promise(r => setTimeout(r, 0));
console.log(`fired=${fired}/50`);  // node: 50/50

With a small heap (BUN_JSC_forceRAMSize=16777216) this reliably loses a message on bun 1.4.0 (fired=49/50 on every run here), and a debug build aborts every run:

ASSERTION FAILED: m_wrapper
src/jsc/bindings/webcore/JSEventListener.h(157) : JSC::JSObject *WebCore::JSEventListener::ensureJSFunction(ScriptExecutionContext &) const

"post a message, then close the sending port" is the normal one-shot pattern, so under memory pressure some fraction of those deliveries silently never happen.

Cause

MessagePortPipe::drainAndDispatch() pops a message and decrements the pipe's queued count under the lock, then deserializes and dispatches it. MessagePort::hasPendingActivity() is

hasMessageListener && (queuedCount > 0 || isOtherSideOpen)

so once the peer is closed and the last message has been popped, nothing reports pending activity for the receiving port. Its JSMessagePort wrapper is only kept alive through JSMessagePortOwner::isReachableFromOpaqueRoots(), and the listener function is only marked (JSEventListener::visitJSFunction) when that wrapper is visited. Deserialization inside MessageEvent::create() allocates, so a GC there collects the wrapper and clears the listener's weak m_jsFunction; the subsequent dispatchEvent() finds no function and does nothing (or trips ASSERT(m_wrapper) on debug builds). The message is gone.

Regression from #29937 (v1.3.14): before that, hasPendingActivity() returned m_entangled, which kept the wrapper alive across dispatch (and also forever after the peer closed, which is the leak that change fixed).

Fix

Release the message's QueuedOne unit only after dispatchOneMessage() returns, so queuedCount > 0 holds for the whole pop to dispatch window and the wrapper (with its listeners) survives any GC in between. The release is skipped when the handler called close(), which already reset the state word; subtracting there would underflow the count.

takeOne() (used by receiveMessageOnPort) still decrements at pop time: there the port is an argument on the JS stack, so its wrapper is already rooted across the deserialize. BroadcastChannel::hasPendingActivity() does not depend on a queued count and is unaffected.

Verification

New test in test/js/web/workers/message-port-pipe.test.ts: 100 one-shot channels whose receiving ports are unreferenced, a 1 MiB string payload (its deserialization reports enough extra memory to trigger collections inside the dispatch window), BUN_JSC_forceRAMSize to keep the heap small.

Without the src/ change the test fails every run on a debug build (the m_wrapper assertion above, exit 134) and on the released 1.4.0 (fired=98/100). With the change it passes, along with the rest of message-port-pipe.test.ts, message-channel.test.ts, message-port-closed-leak.test.ts, message-port-context-destroy-leak.test.ts, message-event.test.ts, worker-postmessage-transfer.test.ts, and worker_threads.test.ts.

Independent of the open close-event work (#32564, #32565): neither changes when the queued count is decremented, and a peer torn down without an explicit close() never gets a close notification, so this window has to be closed in the drain loop itself. The repro above also never exits on its own because the onmessage setter's event-loop ref is not released when the peer closes; that is the separate issue #32564 addresses, and it is why the test child calls process.exit(0) after printing its count.

[robobun]

^{Updated 3:44 AM PT - Jun 29th, 2026}

❌ @robobun, your commit e613491 has 1 failures in Build #66696 (All Failures):

• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64-baseline

---

🧪   To try this PR locally:

bunx bun-pr 33051

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

bun-33051 --bun

[github-actions]

Found 3 issues this PR may fix:

1. Bun exits instead of waiting when MessageChannel from node:worker_threads is used #32562 - Bun exits instead of waiting when MessageChannel is used; the one-shot post-then-exit pattern causes the receiver to be GC'd before dispatch
2. Bun exits even if MessageChannel from node:worker_threads is still working and has unfinished tasks #32563 - Bun exits even if MessageChannel is still working and has unfinished tasks; close event is dropped because hasPendingActivity() returns false too early
3. Worker messages and close events often lost if worker calls process.exit immediately after postMessage #14144 - Worker messages and close events often lost if worker calls process.exit immediately after postMessage; same race between GC and dispatch

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #32562
Fixes #32563
Fixes #14144

🤖 Generated with Claude Code

[robobun]

Checked all three; none is closed by this change, so I left the Fixes lines out.

• Bun exits instead of waiting when MessageChannel from node:worker_threads is used #32562: the message is delivered in that repro. The bug is that a local MessageChannel port with an on('message') listener does not hold the event loop open, which is event-loop ref accounting, not GC. The open MessagePort: don't close a listening peer when its sibling is GC'd #32564 addresses it.
• Bun exits even if MessageChannel from node:worker_threads is still working and has unfinished tasks #32563: same event-loop lifetime problem plus the missing close event on the peer, which is the close-event work in MessagePort: don't close a listening peer when its sibling is GC'd #32564 and Fire the close event on MessagePort #32565. This PR does not add close events.
• Worker messages and close events often lost if worker calls process.exit immediately after postMessage #14144: the losses there happen when the worker calls process.exit() right after postMessage. Worker inboxes have their own drain loop in Worker.cpp, and Worker::hasPendingActivity() is state != Closed rather than queue-count based, so the collected-wrapper window fixed here does not exist on that path; that one is about termination ordering.

This PR only changes when a popped message stops counting as pending activity for the GC. It does not touch event-loop refs and does not add close events.

[coderabbitai]

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: 88c2170a-d4b1-4406-ac2f-3a138446ce97

📥 Commits

Reviewing files that changed from the base of the PR and between a4fb86d and e613491.

📒 Files selected for processing (1)

• test/js/web/workers/message-port-pipe.test.ts

---

Walkthrough

MessagePortPipe::drainAndDispatch() now decrements the queued message count after dispatch returns, with a guard for handler-triggered closure. A header comment documents the new invariant, and a GC stress test exercises message delivery under constrained heap conditions.

Changes

In-flight message pending activity fix

Layer / File(s)	Summary
Dispatch ordering fix and documentation src/jsc/bindings/webcore/MessagePortPipe.h, src/jsc/bindings/webcore/MessagePortPipe.cpp	drainAndDispatch() decrements QueuedOne after dispatchOneMessage() under the side lock, guarded by a queuedCount > 0 check for handler-triggered closure; the header documents that the count covers the currently-dispatched message until dispatch returns.
GC stress regression test test/js/web/workers/message-port-pipe.test.ts	New test spawns a subprocess with a constrained JSC heap, posts 100 messages to otherwise-unreferenced listener ports, and asserts fired=100/100 while allowing stderr output.

Suggested reviewers

• Jarred-Sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately summarizes the main GC-related MessagePort fix.
Description check	✅ Passed	It covers the repro, cause, fix, and verification details, though the headings differ from the template.
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.

---

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

[coderabbitai]

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 `@test/js/web/workers/message-port-pipe.test.ts`:
- Around line 277-281: The GC-stress subprocess assertion in the
message-port-pipe test is over-constraining stderr by requiring it to be exactly
empty. Update the expectation in message-port-pipe.test.ts to keep validating
the combined result from the subprocess, but make stderr unconstrained so benign
debug/ASAN diagnostics do not fail the test. Use the existing test block around
the stdout/exitCode assertion to locate the check and preserve the stdout and
exitCode expectations.

🪄 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: fbab7f96-30ff-436b-b733-5eb4b8d96857

📥 Commits

Reviewing files that changed from the base of the PR and between f789198 and a4fb86d.

📒 Files selected for processing (3)

• src/jsc/bindings/webcore/MessagePortPipe.cpp
• src/jsc/bindings/webcore/MessagePortPipe.h
• test/js/web/workers/message-port-pipe.test.ts

[claude]

I didn't find any bugs and the reasoning looks sound, but this reorders a lock-protected state decrement in the MessagePort GC-liveness path, so it's worth a human sanity check on the close()/detach()-during-dispatch interactions.

Extended reasoning...

Overview

This PR moves the QueuedOne decrement in MessagePortPipe::drainAndDispatch() from before dispatchOneMessage() to after it, so that queuedCount > 0 (and therefore hasPendingActivity()) holds across the deserialize/dispatch window. It adds a queuedCount(st) > 0 guard to skip the decrement when a handler-triggered close() has already reset the state word, updates the header comment for the new invariant, and adds a GC-stress regression test in message-port-pipe.test.ts.

Security risks

None. This is purely an internal GC-liveness/ordering fix in the MessagePort dispatch loop; no parsing, auth, or externally-controlled-input surface is touched.

Level of scrutiny

Moderate-to-high. The diff is small (~12 logic lines) and the rationale is clearly explained, but it changes an invariant in a lock-protected atomic state machine that interacts with the JSC GC visitor, concurrent send() from the peer thread, and handler-driven close()/detach(). I traced the obvious cases (handler closes the port → state reset to Closed, guard skips; handler transfers the port → detach() preserves queuedCount, post-dispatch decrement still applies and the next loop iteration's s.port.get() != port check exits; concurrent peer send() only adds units so the guard can't mis-skip) and they look correct, but GC-lifetime + lock-ordering code in a core runtime primitive is exactly where a maintainer's eyes are valuable.

Other factors

The one CodeRabbit comment (don't constrain stderr in the GC-stress subprocess test) was addressed in e613491. The CI failures are build-rust infrastructure on several targets and unrelated to this C++/test change. The PR description is thorough, the regression test reproduces the failure on unfixed builds, and the author already verified the surrounding test files still pass. No CODEOWNERS entry matches these paths.

[robobun]

For whoever does that sanity check, the two interactions called out above, as I traced them:

• close() from inside the handler: MessagePortPipe::close() nulls s.port, clears the inbox, and stores a bare Closed state word (count 0). The post-dispatch release is therefore skipped by the queuedCount(st) > 0 guard (nothing to release, nothing resurrected), and the next loop iteration exits at the s.ctxId != expectedCtx || s.port.get() != port check. Covered by the existing test "close() inside onmessage handler stops further deliveries".
• transfer (detach()) from inside the handler: detach() preserves the count and clears Attached | DrainScheduled. The new owner's attach() sees queuedCount > 0 and schedules its own drain. The post-dispatch release then drops exactly the in-flight message's unit, so the count returns to the remaining inbox size, and the old loop exits at the identity check without touching DrainScheduled, which the new drain task now owns. Covered by the existing test "same-context re-attach inside handler: inbox follows new wrapper".

In both cases the release (or its skip) happens before the loop re-checks identity, so a unit is never dropped twice and never survives a close(). Concurrent send() is unaffected: the release and send() mutate the state word under the same per-side lock.

[robobun]

CI status for e613491, build 66696 (finished): 282 jobs passed, 4 failed. None of the 4 is related to this change:

• 2x darwin 26 aarch64 - test-bun: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'. The runner never obtained a binary, so no tests ran.
• alpine 3.23 x64 - test-bun and alpine 3.23 x64-baseline - test-bun: test/js/node/test/parallel/test-net-connect-memleak.js fails with collected: false !== true (all 3 retries). That test has no MessagePort in it, and this diff is only reachable through MessagePortPipe::drainAndDispatch, which never runs there. The same two lanes fail on the same test in every Buildkite build started after about 07:30 UTC today across unrelated branches (66692, 66697, 66698, 66700, 66701, 66705, 66706), while builds before that (66656, 66668, 66691) passed them, so it is a repo-wide Alpine lane problem that appeared today, independent of this PR.

The new regression test in message-port-pipe.test.ts passed on every platform that ran it. I am not pushing a retrigger while the Alpine lanes fail for every build, since it would come back red for the same reason; this is ready for review as is, and a re-run once those lanes are healthy should be green.