MessagePort: keep hasRef() true until the 'close' event fires, and false inside it

[robobun]

Problem

• port.close(); port.hasRef() returns false in Bun; node returns true until the port's 'close' event has fired (and false inside and after it). Same for a worker's parentPort.
• When the peer closes, Bun's 'close' handler on the surviving port still sees hasRef() === true; node's sees false.
• Cause, own close: the close() binding called jsUnref() before MessagePort::close() (src/jsc/bindings/webcore/JSMessagePort.cpp:357), and close() itself dropped m_hasRef / m_isRefd synchronously (src/jsc/bindings/webcore/MessagePort.cpp:230-242), while the 'close' event is dispatched from a queued task.
• Cause, peer close: peerClosed() called dispatchCloseEvent() first and jsUnref() after it (MessagePort.cpp:287-291).
• hasRef() reads the live ref state (m_hasRef || m_listenerLoopRefActive, MessagePort.h), so in both cases it reports the release at the wrong moment relative to the event. No user report; found while triaging worker_threads PRs against main after Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075 made parentPort a real MessagePort.

Fix

• MessagePort::close() no longer releases the refs; the task it queues releases them (jsUnref()) and then dispatches 'close'. The close() binding no longer calls jsUnref() first.
• peerClosed() releases, then dispatches.
• stop() is now close(); jsUnref(); and contextDestroyed() goes through it, because a close() issued shortly before teardown leaves the refs to a task that teardown discards unrun (the stop phase is where ports are expected to drop their keep-alives; jsRef()'s self-ref would otherwise leak the native port). close() on an already stopped context releases immediately, as before. contextDestroyed() holds a Ref across the release since that self-ref can be the last reference.
• The release code that was duplicated in close(), disentangle() and jsUnref() is now just jsUnref(), which unrefs through the port's ScriptExecutionContext (the same per-thread VM jsRef() ref'd, and what close() already did) and so no longer takes a JSGlobalObject.
• Why this is right: it is node's model. A node MessagePort is a HandleWrap; hasRef() is uv_has_ref() for as long as the handle is not closed, close() only starts the uv close, and the close callback marks the handle closed immediately before emitting 'close' (handle_wrap.h#L64-L72, handle_wrap.cc#L135-L156). A peer close reaches the other side as a close message that closes that side's handle the same way, so its 'close' handler also runs after the refs are gone. Holding the refs until the task is also physically what a closing uv handle does: the loop stays alive until the close callback runs, which here is the next turn of the task queue. Because the refs are simply held rather than snapshotted, unref() and listener changes made between close() and 'close' are reflected by hasRef() exactly as in node, with no extra state.
• Verified with the new tests in test/js/node/worker_threads/worker_threads.test.ts (own close and peer close, each with a 'message' listener and with an explicit ref(); both sides of a channel; changes made during the closing window; a spawned process that must still exit after close(); parentPort; a worker that exits with closes pending; and an LSan check, gated to the ASAN build, that ports closed right before exit are released by the VM teardown). The 9 behavioural tests fail on main and pass with this change; the LSan test passes on both and was confirmed to report the 50 leaked ports when the stop() release is removed.
  • The whole file passes (133 tests), as do test/js/web/workers/message-channel.test.ts, message-port-pipe, message-port-closed-leak, message-port-context-destroy-leak, worker-postmessage-transfer and message-event, and the 21 upstream test-worker-message-port* / test-worker-message-channel / test-worker-workerdata-messageport tests plus test-worker-ref*, test-worker-parent-port-ref, test-worker-terminate-ref-public-port, test-worker-terminate-unrefed, test-worker-unref-from-message-during-exit, test-worker-cleanexit-with-js, test-worker-exit-code, test-worker-on-process-exit, test-worker-terminate-nested.
  • A 13-scenario hasRef() probe comparing node v26.3.0 with this branch is in the details below: every close-related scenario now matches.
• Deliberate remaining difference, noted in the jsRef() comment: node also honours a ref() issued between close() and 'close' (hasRef() briefly true); a closed port still refuses new refs here, since there is nothing left for it to keep the loop alive for.
• The peerClosed() reordering needed one more guard, caught in review: Bun also fires 'close' when the peer port was garbage collected rather than closed (node never collects an entangled port), and a collected peer never sets the ClosedByRequest bit that jsRef()'s guard keyed on, so a ref() or onmessage = inside or after that 'close' handler re-took loop refs that nothing released, pinning the loop forever (before the reorder the post-dispatch release balanced the in-handler case). jsRef() now also keys on m_closeEventDispatched: once a port's 'close' event has fired, ref() is a no-op whatever the close reason. Covered by a new spawned test (collected peer, ref() inside the 'close' handler, the process must still exit).
• Not in this PR: node's own test/parallel/test-messageport-hasref.js asserts the same close ordering but locates the ports through async_hooks MESSAGEPORT init events, which Bun does not emit yet (worker_threads: MESSAGEPORT async_hooks init, worker-visible warnings, data: URL module formats (+4 tests) #35366), and also asserts unref(); on('message') re-refs the port, which is a separate gap in how listener changes and ref()/unref() combine (MessagePort: release an explicit ref() when the last 'message' listener is removed #38009 covers the removal half; the add half was filed separately). MessagePort: release an explicit ref() when the last 'message' listener is removed #38009 edits the same functions (it also turns jsUnref(JSGlobalObject*) into jsUnref()), so the two will need a trivial textual rebase, but they fix different things and neither subsumes the other.

Background

• A MessagePort "refs" its thread's event loop while something may still need the loop to stay alive, and hasRef() reports whether it currently does. Bun's native port holds up to two such refs: m_hasRef, taken by .ref() or a callable onmessage = (an event-loop ref plus a self-ref() so the native object outlives its JS wrapper), and a listener ref held while m_isRefd (not .unref()'d) and at least one 'message' listener is registered. jsUnref() drops both.
• 'close' is dispatched from a task queued on the event loop rather than synchronously from close(), so that listeners added after close() (and node's close(cb)) still observe it; the event therefore runs after the current script and its microtasks, which is also when node's uv close callback runs.
• ActiveDOMObject::stop() is called on every live port when its context is torn down (worker exit/terminate, process.exit() with BUN_DESTRUCT_VM_ON_EXIT, bun test --isolate retiring a file); after that phase the VM releases everything still queued without running it, which is why a release that lives in a queued task needs a teardown counterpart. contextDestroyed() is the later notification from the context's destructor and is the backstop for a context that was never stopped.

Repros from the report: node v26.3.0 vs Bun main vs this branch

// own close
import { MessageChannel } from "node:worker_threads";
const { port1 } = new MessageChannel();
port1.on("message", () => {});
port1.on("close", () => console.log("in close event:", port1.hasRef()));
port1.close();
console.log("right after own close():", port1.hasRef());

	node	main	this PR
right after own close()	true	false	true
in close event	false	false	false

// peer close
import { MessageChannel } from "node:worker_threads";
const { port1, port2 } = new MessageChannel();
port1.on("message", () => {});
port1.on("close", () => console.log("in close event:", port1.hasRef()));
port2.close();
console.log("right after peer close():", port1.hasRef());

	node	main	this PR
right after peer close()	true	true	true
in close event	false	true	false

parentPort inside a Worker (parentPort.close(); parentPort.hasRef(), then inside its 'close' handler): node true / false, main false / false, this PR true / false.

hasRef() probe, 13 scenarios, node v26.3.0 vs this branch

Each scenario uses a fresh channel; "during" is read synchronously after the listed calls, "inClose" inside the port's 'close' handler.

scenario	node	this PR
on; close; unref	false	false
on; unref; close; ref	during true, inClose false	during false, inClose false (deliberate, see above)
on; close; await 'close'; ref	false	false
on; peer close; await 'close'; ref	false	false
on; unref; close	false	false
close; on	during true, inClose false	during true, inClose false
on f; close; off f	false	false
ref; close	during true, inClose false	during true, inClose false
on; peer close; await 'close'	false	false
ref; on f; off f	false	true (pre-existing, #38009)
onmessage=f; unref; onmessage=g	true	true
on close only; peer close	before false, inClose false	before false, inClose false
ref; peer close	before true, inClose false	before true, inClose false

On main, close; on, ref; close and ref; peer close (plus the two repros above) also differed from node; they match now. The two annotated rows are the only differences left.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/worker_threads/worker_threads.test.ts

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 16 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: 9914903a-a56e-4932-bef3-dad6d500761e

📥 Commits

Reviewing files that changed from the base of the PR and between bdb7382 and 8b83617.

📒 Files selected for processing (4)

• src/jsc/bindings/webcore/JSMessagePort.cpp
• src/jsc/bindings/webcore/MessagePort.cpp
• src/jsc/bindings/webcore/MessagePort.h
• test/js/node/worker_threads/worker_threads.test.ts

---

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

[robobun]

^{Updated 8:05 AM PT - Aug 13th, 2026}

❌ @robobun, your commit 8b83617 has some failures in Build #94421 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 38019

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

bun-38019 --bun

[robobun]

Status: reproduced on main with both snippets from the report (USE_SYSTEM_BUN=1 and a bun bd build at f426a8e, compared against node v26.3.0 on the same machine), plus the parentPort variant. The new tests in test/js/node/worker_threads/worker_threads.test.ts fail on main and pass with this branch; details, node citations and the probe table are in the PR description. Review feedback welcome.

[claude]

I reviewed this PR and didn't find any bugs. Because it reorders native ref-count releases and event-loop keep-alives across close(), stop(), contextDestroyed() and the deferred 'close' task, a human look at the lifecycle paths would still be worthwhile.

What was reviewed:

• jsUnref() now goes through ScriptExecutionContext::unrefEventLoop(), which is the same Bun__eventLoop__refKeepAlive(bunVM, -1) the old code called directly — equivalent.
• The queued close task's port.jsUnref() → deref() is safe: queueTaskKeepingObjectAlive captures Ref { object }, so the port outlives dispatchCloseEvent() and removeAllEventListeners().
• contextDestroyed()'s new Ref protectedThis covers the case where jsUnref()'s deref() drops the last self-ref before ActiveDOMObject::contextDestroyed() runs; disentangle() calls jsUnref() before observeContext(nullptr), so the context is still reachable there.

Extended reasoning...

Overview

The PR changes when MessagePort releases its event-loop keep-alive and self-ref relative to the 'close' event, to match Node's HandleWrap timing (hasRef() stays true until the close callback, false inside it). Concretely: close() defers the jsUnref() into the same queued task that dispatches 'close'; peerClosed() releases before dispatching instead of after; stop() becomes close(); jsUnref(); so a close issued shortly before teardown doesn't strand its refs in a discarded task; contextDestroyed() routes through stop() under a Ref protectedThis; and the three copies of the release block in close()/disentangle()/jsUnref() collapse into a single jsUnref() that unrefs via the port's ScriptExecutionContext (dropping the JSGlobalObject* parameter). Nine new behavioural tests plus an LSan-gated leak test cover own-close, peer-close, both-sides, in-window ref changes, process-exit, worker teardown, and parentPort.

Security risks

None identified. This is Node-compat timing for hasRef() and internal ref-count/keep-alive plumbing; no untrusted-input parsing, no auth/crypto, no new externally reachable surface.

Level of scrutiny

High. This is native lifecycle/refcount code: it moves a deref() from a synchronous path into a queued task, adds a second release point in stop() for the teardown-discards-task case, and changes contextDestroyed(). REVIEW.md's memory-safety section ("reference counts provably balanced on every terminal path", "a struct gaining an owning field wires its release into … ALL lifecycle exits") applies directly. I traced each release path — the queued task holds Ref { object }, peerClosed() holds Ref protectedThis, contextDestroyed() now holds Ref protectedThis, disentangle()'s caller holds a RefPtr, and jsUnref() is idempotent so the double call in stop() after an already-closed port is safe — and context->unrefEventLoop() is exactly Bun__eventLoop__refKeepAlive(m_bunVM, -1), so the parameter drop is a pure refactor. Nothing looks wrong, but the number of interacting terminal paths (user close(), peer close, transfer, stop(), contextDestroyed(), task run vs. task discarded) is large enough that a maintainer familiar with the ActiveDOMObject/ScriptExecutionContext teardown ordering should confirm.

Other factors

The PR description is unusually thorough (13-scenario Node comparison, upstream test list, LSan verification with the fix removed), and the tests follow the harness conventions well (event-driven waits, spawned subprocess for the hang case, skipIf(!isASAN || !isLinux) for the leak test). The change also notes an intentional overlap with #38009 that will need a textual rebase. None of that changes the calculus: this is a non-mechanical change to native ref-counting across several teardown paths, which per the approval guidelines is not a candidate for auto-approval.

[claude]

Beyond the inline finding, I also checked that context->unrefEventLoop() in the new jsUnref() is the same Bun__eventLoop__refKeepAlive(bunVM, -1) call the old path made (ScriptExecutionContext.cpp:103-106), so the ref/unref pair stays balanced; and that queueTaskKeepingObjectAlive holds a Ref across the callback (ActiveDOMObject.h:93), so port.jsUnref()'s deref() inside the queued 'close' task cannot drop the last reference.

Extended reasoning...

This PR reworks MessagePort ref-release ordering across close(), peerClosed(), stop(), contextDestroyed(), disentangle() and changes jsUnref()'s signature — subtle lifecycle/refcount code that warrants a human look regardless. The inline nit covers the one edge (Collected peer + .ref() inside 'close') the reorder opens; the note above records the two adjacent concerns I traced and found safe.

[robobun]

Addressed the reviews in 8b83617: the jsRef() guard now also keys on m_closeEventDispatched, closing the collected-peer case where a ref() or onmessage assignment inside or after the GC-triggered close handler re-took loop refs that nothing released (the ClosedByRequest bit is never set for a collected peer). Added a spawned regression test for it and condensed the lifecycle comments the style check flagged. The full worker_threads file passes locally (133 tests).

[claude]

Thanks for addressing the earlier note — jsRef() now guards on m_closeEventDispatched and the new "ref() inside 'close' after the peer is collected" test covers it. This run's automated review found no further bugs. Because the change re-times self-ref and event-loop-ref release across close() / peerClosed() / stop() / contextDestroyed() in C++ MessagePort lifecycle code, a human look would still be worthwhile.

What was reviewed:

• Verified context->unrefEventLoop() in the consolidated jsUnref() is the same Bun__eventLoop__refKeepAlive(-1) the old paths used, and that every caller (close()'s task, peerClosed(), disentangle(), stop(), the JS bindings) holds a ref across the deref().
• Checked stop() → close() under stopActiveDOMObjects(): m_activeDOMObjectsAreStopped is set before iteration, so close() takes the immediate-release branch and the second jsUnref() is a harmless no-op.
• Confirmed the CloseKind::Collected peer-close path is now closed by the m_closeEventDispatched guard (previous finding), with a spawned regression test.

Extended reasoning...

Overview

This PR retimes when a MessagePort releases its event-loop keep-alive and self-ref relative to the 'close' event, to match Node's HandleWrap semantics (hasRef() stays true until the close callback, false inside/after). It touches src/jsc/bindings/webcore/MessagePort.{cpp,h} (moves the release from close() into the queued task, reorders peerClosed(), folds three copies of the release logic into a single argless jsUnref(), rewrites stop() as close(); jsUnref() and routes contextDestroyed() through it with a protecting Ref), JSMessagePort.cpp (drops the pre-close() jsUnref() and updates the two other call sites), and adds ~230 lines of tests to worker_threads.test.ts.

Since the previous review

I previously flagged that reordering peerClosed() to jsUnref(); dispatchCloseEvent() opened a window where a .ref() inside a GC-triggered (CloseKind::Collected) 'close' handler would take a never-released loop ref. Commit 8b83617 applied the suggested fix — jsRef() now early-returns on m_closeEventDispatched — and added a spawned test that drops port2, pumps GC, calls port1.ref() inside 'close', and asserts hasRef() is false and the process exits cleanly. That closes both the in-handler and after-handler variants.

Security risks

None identified. This is Node-compat behaviour timing, not a security boundary; no untrusted-input parsing, no crypto/auth. The memory-safety angle (self-deref() and event-loop refcounts) is the real risk surface.

Level of scrutiny

High. This is exactly the "Native code: memory safety" bucket the review guide calls the most-blocked category: C++ refcount balance across every terminal path (close(), peerClosed(), disentangle(), stop(), contextDestroyed(), the queued task discarded by teardown), with the release now living in a deferred task and a teardown counterpart. The reasoning in the PR is thorough and cites Node source, the test matrix is broad (own/peer close × listener/ref, both sides, changes made in the closing window, parentPort, worker exit with pending closes, an ASAN/LSan leak check), and I spot-checked that unrefEventLoop() is the same primitive jsRef() acquires and that every jsUnref() caller holds a reference across the deref(). Even so, retiming ref release around VM teardown in worker_threads is not something I'd auto-approve.

Other factors

• stopActiveDOMObjects() sets m_activeDOMObjectsAreStopped before iterating, so stop()'s close() always hits the isContextStopped() immediate-release branch and the trailing jsUnref() is idempotent — no double-release.
• The consolidated jsUnref() uses scriptExecutionContext()->unrefEventLoop(), which is a thin wrapper over Bun__eventLoop__refKeepAlive(m_bunVM, -1) — the same primitive the removed close()/disentangle() blocks already used, so this is a dedup rather than a semantic change on that axis.
• The comment-cop bot left a batch of "paragraph-long comment" flags; those are automated length lints on explanatory node-compat comments, not substantive review feedback.
• The PR description's paragraph about a GC-triggered ref() "now sticking" appears to predate the m_closeEventDispatched guard added in 8b83617 (the code and the new test assert it does not stick); description-only, no code impact.