Conversation
|
Updated 5:25 PM PT - Jul 12th, 2026
❌ @robobun, your commit 8560b4e has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32805That installs a local version of the PR into your bun-32805 --bun |
WalkthroughThe PR adds async-context snapshot and restore helpers, applies them to AbortSignal timeout aborts, BroadcastChannel and MessagePort delivery, and Worker parent-side dispatch, and adds GC tracing plus regression tests. ChangesAsync context propagation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/jsc/bindings/webcore/AbortSignal.cpp (1)
222-249: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winClear the one-shot timeout context on teardown.
JSAbortSignalCustom.cppnow tracesm_timeoutAsyncContext, so after this path consumes the snapshot it becomes a strong edge from any retainedAbortSignalto the wholeAsyncLocalStoragestore graph. Unlike thecreationAsyncContextfields onMessagePort/BroadcastChannel, this value is never needed again after the timeout fires, and the same retention happens whencancelTimer()tears the timeout down early. Please copy the snapshot into a local forAsyncContextFrameScopeand clear the member on every terminal timeout path.🤖 Prompt for 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. In `@src/jsc/bindings/webcore/AbortSignal.cpp` around lines 222 - 249, The one-shot timeout async context on AbortSignal is being retained after it is consumed, creating an unintended strong reference to AsyncLocalStorage state. Update the AbortSignal timeout teardown path in AbortSignal.cpp to copy m_timeoutAsyncContext into a local for AsyncContextFrameScope, then clear the member on every terminal timeout path, including the normal fire path and the cancelTimer() teardown path. Use the existing AbortSignal timeout handling around markAborted(), runAbortSteps(), and the timeout async context member to keep the snapshot only for the scope of the abort work.src/jsc/bindings/webcore/Worker.cpp (1)
351-362: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExpand the worker message scope to cover the whole dispatch turn.
Line 360 restores the async context only for
dispatchEvent(event), butdrainInbox()entangles transferred ports before the callback and callsglobalObject->drainMicrotasks()after it returns. That meansPromise/queueMicrotaskcallbacks scheduled fromworker.onmessage, and anyMessagePorts transferred in that message, fall back to the ambient parent context instead of the worker’s creation context. This also diverges fromMessagePort::dispatchOneMessage, which scopes beforeentanglePorts()for the same reason.Suggested fix
void Worker::drainToParent(ScriptExecutionContext& context) { auto* globalObject = defaultGlobalObject(context.jsGlobalObject()); if (!globalObject) { Locker locker { m_toParent.lock }; m_toParent.drainScheduled.store(false, std::memory_order_relaxed); return; } + AsyncContextFrameScope asyncContextScope(globalObject, m_creationAsyncContext.getValue()); bool reschedule = drainInbox(m_toParent, globalObject, context, [&](Event& event) { - AsyncContextFrameScope asyncContextScope(globalObject, m_creationAsyncContext.getValue()); dispatchEvent(event); }); if (reschedule) { postTaskToParent([protectedThis = Ref { *this }](ScriptExecutionContext& c) { protectedThis->drainToParent(c);🤖 Prompt for 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. In `@src/jsc/bindings/webcore/Worker.cpp` around lines 351 - 362, The worker message dispatch scope is too narrow in Worker::drainToParent, because AsyncContextFrameScope currently wraps only dispatchEvent(event) while drainInbox() also entangles transferred ports and drains microtasks afterward. Move the AsyncContextFrameScope to cover the entire dispatch turn in Worker::drainToParent, matching MessagePort::dispatchOneMessage so the worker’s creation async context is active for entanglePorts, dispatchEvent, and any Promise/queueMicrotask callbacks triggered during that message.
🤖 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.
Outside diff comments:
In `@src/jsc/bindings/webcore/AbortSignal.cpp`:
- Around line 222-249: The one-shot timeout async context on AbortSignal is
being retained after it is consumed, creating an unintended strong reference to
AsyncLocalStorage state. Update the AbortSignal timeout teardown path in
AbortSignal.cpp to copy m_timeoutAsyncContext into a local for
AsyncContextFrameScope, then clear the member on every terminal timeout path,
including the normal fire path and the cancelTimer() teardown path. Use the
existing AbortSignal timeout handling around markAborted(), runAbortSteps(), and
the timeout async context member to keep the snapshot only for the scope of the
abort work.
In `@src/jsc/bindings/webcore/Worker.cpp`:
- Around line 351-362: The worker message dispatch scope is too narrow in
Worker::drainToParent, because AsyncContextFrameScope currently wraps only
dispatchEvent(event) while drainInbox() also entangles transferred ports and
drains microtasks afterward. Move the AsyncContextFrameScope to cover the entire
dispatch turn in Worker::drainToParent, matching MessagePort::dispatchOneMessage
so the worker’s creation async context is active for entanglePorts,
dispatchEvent, and any Promise/queueMicrotask callbacks triggered during that
message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9589c255-ba29-476d-999c-4ff320e58082
📒 Files selected for processing (20)
src/jsc/bindings/AsyncContextFrame.cppsrc/jsc/bindings/AsyncContextFrame.hsrc/jsc/bindings/webcore/AbortSignal.cppsrc/jsc/bindings/webcore/AbortSignal.hsrc/jsc/bindings/webcore/BroadcastChannel.cppsrc/jsc/bindings/webcore/BroadcastChannel.hsrc/jsc/bindings/webcore/JSAbortSignalCustom.cppsrc/jsc/bindings/webcore/JSBroadcastChannel.cppsrc/jsc/bindings/webcore/JSBroadcastChannel.hsrc/jsc/bindings/webcore/JSMessagePortCustom.cppsrc/jsc/bindings/webcore/JSWorker.cppsrc/jsc/bindings/webcore/JSWorker.hsrc/jsc/bindings/webcore/MessagePort.cppsrc/jsc/bindings/webcore/MessagePort.hsrc/jsc/bindings/webcore/Worker.cppsrc/jsc/bindings/webcore/Worker.htest/js/node/async_hooks/AsyncLocalStorage-tracking.test.tstest/js/node/async_hooks/async-context/async-context-abort-signal-timeout.jstest/js/node/async_hooks/async-context/async-context-broadcast-channel.jstest/js/node/async_hooks/async-context/async-context-message-channel.js
|
Thanks, both findings are addressed in 51cbf85. Clear the one-shot timeout context (CodeRabbit, Hoist the
import { AsyncLocalStorage } from "node:async_hooks";
const als = new AsyncLocalStorage();
const { port1, port2 } = als.run("RECEIVER", () => new MessageChannel());
port1.onmessage = e => {
e.data.port.onmessage = () => console.log("inner:", als.getStore());
};
const { port1: i1, port2: i2 } = new MessageChannel();
port2.postMessage({ port: i1 }, [i1]);
i2.postMessage("hi");
// node v26.3.0: "inner: undefined"Hoisting the scope would have made Bun keep a context that Node drops, a divergence the dual-runner fixture harness cannot even cover (every fixture must pass under Node too). What the two reviews did catch is a real inconsistency, just in the other direction: |
|
Both of the GC-liveness findings on commit 51cbf85 are real; I verified them against the source ( Two wrapper-liveness holes in one review round means the wrapper-visited Two new fixtures cover the exact reported shapes, pass under Node, and fail under the unfixed build:
The tracking suite is 80 pass / 0 fail, and the abort, broadcast-channel, message-channel, message-port leak, and worker transfer suites show no new failures. |
|
The cycle finding on c68802e takes the robust option instead, and with all four review findings on the table the right shape is now clear:
The invariant is documented once, on |
There was a problem hiding this comment.
c68802e addresses all my prior concerns and the bug hunter found nothing new on it; given how subtle the GC-liveness story turned out to be across three review rounds (wrapper-visited Weak → Strong → back to Weak + JSMessageChannel visitor, with AbortSignal keeping the Strong), I'd still like a human to sign off on the final Strong-vs-Weak split and the visitChildren additions.
Extended reasoning...
Overview
This PR threads AsyncLocalStorage context through four async event-dispatch boundaries (AbortSignal.timeout, MessagePort, Worker parent-side events, BroadcastChannel) by snapshotting the active context at resource creation and restoring it via a new RAII AsyncContextFrameScope around each posted-task dispatch. It touches 16 source files in src/jsc/bindings/ (core AsyncContextFrame, four impl/header pairs, four JS-wrapper files including new visitChildren overrides) plus 7 test files. The captured context is stored as a wrapper-visited JSValueInWrappedObject for MessagePort/Worker/BroadcastChannel and as a JSC::Strong (released by cancelTimer()) for AbortSignal.timeout.
Security risks
None identified. This is observability/tracing infrastructure (ALS propagation), not auth/crypto/permissions. The GC changes could in principle cause UAF or leaks, but the design now uses the standard JSValueInWrappedObject + visitChildren pattern with one well-bounded JSC::Strong whose release is timer-guaranteed.
Level of scrutiny
High. This is production-critical runtime code with concurrent-GC interactions. Over three prior review rounds on this very PR I flagged: (1) scope ordering vs entanglePorts (resolved by narrowing to match Node's actual behaviour, empirically verified), (2) two wrapper-liveness gaps that lost the captured context under GC (AbortSignal.any source and lazy JSMessageChannel port getters), and (3) a JSC::Strong native↔GC cycle leak introduced by the fix for (2). All are now addressed in c68802e with the hybrid design (Weak + per-wrapper visitors + JSMessageChannel visiting its ports' contexts; Strong only on AbortSignal where cancelTimer() guarantees release), and the bug hunter found nothing new on the final revision. But the fact that three rounds each surfaced a distinct GC-correctness issue is itself a signal that this area is delicate enough to warrant a human reviewer's sign-off on the final shape.
Other factors
- Test coverage is good: new dual-runtime (Bun + Node) parity fixtures for every boundary, GC-survival fixtures for the two wrapper-liveness gaps, and a heap-stats leak regression test for the Strong-cycle case.
- CI on the previous commit (5169257) showed failures; c68802e's build status isn't reflected in the timeline yet.
- All my prior inline comments are marked resolved with detailed responses from the author, including empirical verification against Node v26.3.0 where my initial premise was wrong.
|
Thanks. All four findings across the three review rounds were real and each one improved the final shape. Agreed that the Strong-vs-Weak split deserves a human look; it is the subtle part of this PR. For whoever reviews: the one-paragraph invariant lives on
CI: the only red lane on this revision is |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/jsc/bindings/AsyncContextFrame.cpp`:
- Around line 39-55: The AsyncContextFrame::captureCurrentContext overloads
currently only treat JSValue::isUndefined() as “no async context,” which can
incorrectly snapshot an empty value and leave stale data in a reused slot.
Update both overloads to also treat JSValue::isEmpty() as the no-context case,
and in that branch explicitly clear the destination slot instead of just
returning. Apply the fix in the captureCurrentContext(JSGlobalObject*,
WebCore::JSValueInWrappedObject&) and captureCurrentContext(JSGlobalObject*,
JSC::Strong<JSC::Unknown>&) paths.
In `@src/jsc/bindings/webcore/MessagePort.cpp`:
- Line 62: Clear the captured async context when MessagePort becomes inert: in
MessagePort.cpp, reset m_creationAsyncContext on the terminal paths that set
m_isDetached so a detached or closed port no longer keeps AsyncLocalStorage
alive. Update the relevant MessagePort methods that detach/transfer the port and
ensure JSMessagePort::visitAdditionalChildrenInGCThread() can’t retain the stale
snapshot after dispatchOneMessage() is no longer possible.
🪄 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: 3b56ede5-09ba-481d-9357-b52d123f8273
📒 Files selected for processing (15)
src/jsc/bindings/AsyncContextFrame.cppsrc/jsc/bindings/AsyncContextFrame.hsrc/jsc/bindings/webcore/AbortSignal.cppsrc/jsc/bindings/webcore/AbortSignal.hsrc/jsc/bindings/webcore/BroadcastChannel.cppsrc/jsc/bindings/webcore/BroadcastChannel.hsrc/jsc/bindings/webcore/JSMessageChannelCustom.cppsrc/jsc/bindings/webcore/MessagePort.cppsrc/jsc/bindings/webcore/MessagePort.hsrc/jsc/bindings/webcore/Worker.cppsrc/jsc/bindings/webcore/Worker.htest/js/node/async_hooks/async-context/async-context-abort-signal-any.jstest/js/node/async_hooks/async-context/async-context-message-channel-gc.jstest/js/node/async_hooks/async-context/async-context-message-channel.jstest/js/web/workers/message-port-closed-leak.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/webcore/MessagePort.cpp (1)
211-216: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCheck the throw scope after creating the message event.
MessageEvent::create(...)can leave an exception pending; this path then installs the async-context scope and dispatches the event anyway. Mirror theBroadcastChannel::dispatchMessage()check before Line 215.Proposed fix
auto event = MessageEvent::create(*context.jsGlobalObject(), message.message.releaseNonNull(), {}, {}, {}, WTF::move(ports)); + if (scope.exception()) [[unlikely]] { + RELEASE_ASSERT(vm->hasPendingTerminationException()); + return; + } // Listeners observe the async context that was active when this port wasAs per coding guidelines, “In C++ code that can enter JS, check for exceptions after every call that can throw or run user code before using the result.”
🤖 Prompt for 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. In `@src/jsc/bindings/webcore/MessagePort.cpp` around lines 211 - 216, After MessageEvent::create in MessagePort::dispatchMessage, check the JS throw scope before proceeding, since creating the event can leave a pending exception. Mirror the exception handling used in BroadcastChannel::dispatchMessage: if the creation call throws, return early and do not install AsyncContextFrameScope or call dispatchEvent. Use the MessageEvent::create and dispatchEvent sites in MessagePort.cpp to place the guard correctly.Source: Coding guidelines
🤖 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 `@src/jsc/bindings/webcore/BroadcastChannel.cpp`:
- Around line 108-109: The GC visitor comment in
JSBroadcastChannel::visitChildrenImpl is stale because creationAsyncContext() is
no longer immutable after wrapper creation and is now cleared on close. Update
the comment to describe the terminal clearing behavior in
BroadcastChannel::close/creationAsyncContext handling, or add the necessary
GC/write-barrier rationale if the field can mutate after wrapper creation. Keep
the comment durable and non-obvious, and align it with the current behavior
around m_creationAsyncContext.clear().
In `@src/jsc/bindings/webcore/Worker.cpp`:
- Around line 571-573: Update the JSWorker GC visitor comment in
JSWorker::visitChildrenImpl() so it no longer says creationAsyncContext() is
“set once … and never mutated afterwards”; revise it to mention the terminal
clear that happens when the worker reaches State::Closed and
m_creationAsyncContext is cleared. Keep the comment aligned with the current
behavior around m_state and m_creationAsyncContext, and remove any wording that
is now stale or misleading.
---
Outside diff comments:
In `@src/jsc/bindings/webcore/MessagePort.cpp`:
- Around line 211-216: After MessageEvent::create in
MessagePort::dispatchMessage, check the JS throw scope before proceeding, since
creating the event can leave a pending exception. Mirror the exception handling
used in BroadcastChannel::dispatchMessage: if the creation call throws, return
early and do not install AsyncContextFrameScope or call dispatchEvent. Use the
MessageEvent::create and dispatchEvent sites in MessagePort.cpp to place the
guard correctly.
🪄 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: db2259a5-96eb-4110-af62-cbbb8964a241
📒 Files selected for processing (3)
src/jsc/bindings/webcore/BroadcastChannel.cppsrc/jsc/bindings/webcore/MessagePort.cppsrc/jsc/bindings/webcore/Worker.cpp
|
Latest round: the two stale GC-visitor comments were real (my previous commit made my own "never mutated afterwards" wording false), reworded in ffa2efd. The no-output-constraint rationale they document still holds: after the wrapper exists the slot is only ever cleared, never given a new value, so a marked wrapper can never end up pointing at a newly acquired unmarked value. On the outside-diff suggestion to add an exception check after |
|
b3192a1 addresses the review finding above (the two Rust timer-teardown paths that bypassed A note on CI, for whoever reviews this, because the aggregate status will very likely stay red for reasons that are not this PR. Across the last three full builds on this branch (65254, 65311, 65432) the only job that failed every time is That agent fails to fetch the already-built artifact, so the lane never tests anything. The remaining red lanes on the last completed build (65311) were:
281 of 286 jobs passed on that build, and nothing in any failure intersects |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/AsyncContextFrame.h (1)
26-39: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCondense this header comment to the 3-line limit.
The current block exceeds the repo’s code-comment length rule; keep the key invariant here and move deeper rationale out of the header if needed.
Proposed wording
- // Snapshots the currently-active async context (the value AsyncLocalStorage - // is running with) into `slot`, for AsyncContextFrameScope to restore - // around a later asynchronous event dispatch. No-op when there is none. - // - // Prefer the JSValueInWrappedObject overload and visit `slot` from the - // owner's JS wrapper. The snapshot can reference that wrapper (the user's - // store may hold the resource), so rooting it with a JSC::Strong on an - // object whose wrapper holds a Ref back to it forms an uncollectable - // native-to-GC cycle. Only use the Strong overload when a GC-independent - // release is guaranteed to run: AbortSignal.timeout() needs it (its abort - // is observed through a dependent AbortSignal.any() signal's wrapper, not - // its own), and every path that retires its timer, including the Rust - // teardown paths that never reach cancelTimer(), calls - // clearTimeoutAsyncContext() to drop the handle. + // Snapshot the active AsyncLocalStorage context for later async dispatch. + // Prefer wrapper-visited slots; use Strong only when a GC-independent + // teardown path clears it, such as AbortSignal.timeout().As per coding guidelines, "Keep code comments to 3 lines max".
🤖 Prompt for 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. In `@src/jsc/bindings/AsyncContextFrame.h` around lines 26 - 39, Condense the header comment in AsyncContextFrame to fit the 3-line limit while preserving the key invariant. Keep the guidance on preferring the JSValueInWrappedObject overload, the risk of native-to-GC cycles when rooting with JSC::Strong, and the limited cases where the Strong overload is valid. Move the longer explanation about AbortSignal.timeout(), AbortSignal.any(), and clearTimeoutAsyncContext() out of this header comment if it still needs to be documented.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/jsc/bindings/AsyncContextFrame.h`:
- Around line 26-39: Condense the header comment in AsyncContextFrame to fit the
3-line limit while preserving the key invariant. Keep the guidance on preferring
the JSValueInWrappedObject overload, the risk of native-to-GC cycles when
rooting with JSC::Strong, and the limited cases where the Strong overload is
valid. Move the longer explanation about AbortSignal.timeout(),
AbortSignal.any(), and clearTimeoutAsyncContext() out of this header comment if
it still needs to be documented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c3bffc1-5cdb-4e9b-b7c2-6e53c29bf4ea
📒 Files selected for processing (8)
src/jsc/AbortSignal.rssrc/jsc/bindings/AsyncContextFrame.hsrc/jsc/bindings/bindings.cppsrc/jsc/bindings/webcore/AbortSignal.cppsrc/jsc/bindings/webcore/AbortSignal.hsrc/jsc/bindings/webcore/JSBroadcastChannel.cppsrc/jsc/bindings/webcore/JSWorker.cppsrc/runtime/timer/mod.rs
|
On the last review round's one remaining suggestion (condense the That comment is the GC-lifetime contract that three separate review findings on this PR were needed to establish, and the proposed three-line version drops the two non-obvious facts each of those findings turned on: why a With that, every review finding on this PR is addressed or answered (14 inline comments across 8 bot rounds, all threads resolved), claude has signed off and asked for a human on the final Strong-vs-Weak split, and the diff is unchanged from b3192a1. The only outstanding item is CI: as described in the comment above, |
There was a problem hiding this comment.
b3192a1 addresses my last finding (the Rust timer-teardown paths now release the captured context); no further issues from me on this revision. Given how much of the correctness here rests on the Strong-vs-Weak GC lifetime split — which took several rounds to converge and which you've already flagged yourself — I'd still want a maintainer to sign off on that design before merge.
Extended reasoning...
Overview
This PR threads AsyncLocalStorage context through four async event-dispatch boundaries (AbortSignal.timeout, MessagePort, Worker parent-side events, BroadcastChannel) by snapshotting the active context at resource creation and restoring it via a new RAII AsyncContextFrameScope around each asynchronous dispatchEvent. It touches 26 files across C++ JSC bindings (AsyncContextFrame.{h,cpp}, AbortSignal.{h,cpp}, MessagePort.{h,cpp}, Worker.{h,cpp}, BroadcastChannel.{h,cpp}, four JS* wrappers), Rust FFI (AbortSignal.rs, runtime/timer/mod.rs), and adds five Node-parity test fixtures plus a leak regression test.
Security risks
None identified. The change does not touch auth, crypto, permissions, or untrusted-input parsing. The captured value is the user's own ALS store, and the restore scope only swaps a per-global internal field that user code already controls via AsyncLocalStorage.run().
Level of scrutiny
High. The hard part is not the context restore (that is mechanical) but the GC lifetime story for the captured snapshot, which sits at the native↔GC-heap boundary:
- Three classes hold the snapshot in a wrapper-visited
JSValueInWrappedObject(weak), relying on the wrapper being kept alive byhasPendingActivity()for exactly as long as dispatch is observable, with a special-case visit fromJSMessageChannelto cover the lazy-port-getter window. AbortSignal.timeout()alone holds it in aJSC::Strong, justified by an invariant that every timer-retirement path (C++cancelTimer(), Rustcancel_all_timeout_objects, theTimeout::runstale-generation branch) callsclearTimeoutAsyncContext()so theStrongcannot form a permanent cycle.
Across five review rounds I found and the author fixed: (1) a transferred-port scope-ordering inconsistency, (2) an AbortSignal.any source-wrapper GC hole, (3) a MessageChannel lazy-getter GC hole, (4) a Strong-induced uncollectable cycle on MessagePort/Worker/BroadcastChannel, and (5) two Rust teardown paths that bypassed the Strong release. Each was real and each reshaped the design. That convergence-by-iteration, plus the author's own note that the split "deserves a human look", is exactly why I'm not approving: the final shape looks right to me and the bug-hunting pass on b3192a1 found nothing, but the cost of a missed GC edge here is a silent leak or a use-after-sweep in production-critical event paths.
Other factors
- All inline review threads are resolved; the latest commit (b3192a1) directly implements my last suggestion.
- Test coverage is good: each boundary has a dual-runtime (Bun + Node) parity fixture, plus targeted GC fixtures for the two wrapper-liveness holes and a heap-count regression for the cycle case.
- CI is red on one darwin-aarch64 lane due to a Buildkite artifact-download timeout (infra, not test); the author has documented this and the other lanes' flakes do not intersect any code touched here.
- The change adds
DECLARE_VISIT_CHILDRENtoJSWorkerandJSBroadcastChanneland a new FFI export — small ABI-surface additions a maintainer should be aware of.
|
Thanks for the careful review across all of the rounds; every real finding reshaped this for the better, and I agree the Strong-vs-Weak split is the part that deserves a human's eyes. An update to my CI note above, because the picture improved on the latest build (65441, for b3192a1): the So: the diff is unchanged from b3192a1, every review thread is resolved, and every test that ran on the latest build passed. This is ready for a maintainer. |
|
Small correction to my last comment now that build 65441 has finished: darwin-14-aarch64 was still scheduled when I wrote that it was green. It has since finished with the same two |
b3192a1 to
421b9d5
Compare
|
Rebased onto main (421b9d5) to clear the merge conflict. The only conflict was a single |
|
A note on the rebased build (67840), because it looks much worse than it is: it is red on 15 jobs, but every one of them is inherited from the new base or is the known infrastructure issue, and none of it is this diff. I dug into this one properly because the failure shape was new. The two new error annotations are I also verified it locally rather than trusting the correlation. On this machine (linux x64, ASAN debug build),
So the hang is specific to the Buildkite test environment, it predates this branch on that base, and nothing in this diff reproduces or affects it. (Separately: this diff is behaviourally inert unless The rest of the red on 67840 is the usual set: two I have already used the one CI re-roll this conversation allows and a re-roll cannot help here anyway, since every build on this base will inherit the same two main-side failures until they are fixed on main. The diff is unchanged and ready; it just needs a maintainer who knows the red is not from here. |
|
Completing the note above now that build 67840 has finished (17 failed / 269 passed): two more error annotations appeared on the last jobs, |
…orker, AbortSignal.timeout, and BroadcastChannel events These four objects dispatch their events from posted event-loop tasks, so by the time dispatchEvent runs, the active async context is whatever the event loop left there (undefined). Node restores the context that was active when the resource was created: its AsyncWrap captures async_context_frame::current() at construction and MakeCallback restores it, and AbortSignal.timeout() gets the same for free from the setTimeout that backs it. Match that. Each impl snapshots the active async context at construction (for AbortSignal, at timeout()) into a JSValueInWrappedObject visited through its JS wrapper, and a new AsyncContextFrameScope RAII class restores it around the asynchronous dispatch. Synchronous dispatch paths (AbortController.abort, EventEmitter.emit, dispatchEvent from JS) are unchanged. Un-skips the existing test.todo for async-context-worker_threads-message.js.
… match Node Clear AbortSignal::m_timeoutAsyncContext in cancelTimer() so a retained aborted signal does not pin the AsyncLocalStorage store graph after the one-shot timer can no longer fire. Move MessagePort::dispatchOneMessage's AsyncContextFrameScope to after entanglePorts(), matching Worker::drainToParent. Verified against Node v26.3.0: a MessagePort transferred inside another message's payload does NOT inherit the receiving resource's async context there either, because Node deserializes the transferred ports before MakeCallback restores the receiver's context. Scoping only the event dispatch is the Node-exact behavior at every site. Strengthen the message-channel fixture to also assert that await continuations and queueMicrotask callbacks registered inside the restored handler keep the store.
…ed Weak Review found two cases where a JSValueInWrappedObject held on the impl and visited through its JS wrapper is not enough: - new MessageChannel() does not create a JSMessagePort wrapper until .port1 or .port2 is first read, so nothing visited the ports' captured context in between and a full GC could collect it. - An AbortSignal.timeout() consumed only as an AbortSignal.any() source has no abort listener of its own, so JSAbortSignalOwner does not keep its wrapper alive and the captured context could be collected before the timer fired into the dependent signal's listeners. Switch the four captured-context members to JSC::Strong, released at the same point the object can last dispatch (cancelTimer() for the timeout, the destructor otherwise). This removes the wrapper-liveness dependency entirely along with the per-wrapper visitChildren plumbing it required, and matches the existing JSC::Strong members on MessageEvent and ErrorEvent. The capture is centralized in AsyncContextFrame::captureCurrentContext, which replaces currentContext(). Adds fixtures for both scenarios; each passes under Node and fails under the unfixed build.
…owns Review found that holding the creation-time async context in a JSC::Strong forms an uncollectable native-to-GC cycle whenever the AsyncLocalStorage store references the resource itself: impl -> Strong (HandleSet root) -> context array -> store -> JS wrapper -> Ref<impl>. Clearing it on close is not enough either; an abandoned open MessagePort never gets close() called. Go back to a JSValueInWrappedObject visited from the owner's JS wrapper for MessagePort, Worker, and BroadcastChannel. Their owners already keep the wrapper alive for exactly as long as the object can observably dispatch, so the only real liveness gap from the earlier review round was MessageChannel: its ports have no JSMessagePort wrapper until .port1/.port2 is first read. JSMessageChannel::visitAdditionalChildrenInGCThread now visits both ports' captured contexts to cover that window. AbortSignal.timeout() keeps the JSC::Strong: it is the one case where the wrapper is not a reliable root (the abort is observed through a dependent AbortSignal.any() signal whose wrapper, not its own, stays alive) and the one case where a Strong is cycle-safe, because the timer heap guarantees cancelTimer() releases the handle independent of GC reachability. AsyncContextFrame::captureCurrentContext now has both overloads, with the invariant documented in one place. Adds a leak regression test asserting that MessagePorts referenced by their own creation-time store are still collected.
MessagePort::close() and disentangle(), BroadcastChannel::close(), and the Worker close task now clear the creation-time async context snapshot, the same release point AbortSignal's cancelTimer() already uses. After those points the object can never dispatch again, so there is no reason for a still-referenced closed/transferred/terminated object to keep the captured AsyncLocalStorage store reachable until its wrapper is collected.
…own paths The JSC::Strong holding AbortSignal.timeout()'s captured async context is justified by "every path that retires the timer releases the handle", but two Rust paths retire the timer without ever reaching cancelTimer(): the bun test --isolate teardown (cancel_all_timeout_objects) and Timeout::run's stale-generation branch. Both unref the signal directly, so if the captured AsyncLocalStorage store referenced the signal's own wrapper, the Strong kept that wrapper (and therefore the signal) alive forever and the handle was never released. Expose AbortSignal::clearTimeoutAsyncContext() over the C ABI, have cancelTimer() go through it, and call it from both Rust paths before the unref, so the stated invariant actually holds on every terminal path.
…urrent load Worker thread startup creates a fresh JSC VM and module graph; on an ASAN debug build the full round-trip-and-terminate takes several seconds and, under describe.concurrent alongside 79 other process-spawning fixtures, brushes past the 5s default. Matches the explicit timeout worker tests in test/js/node/worker_threads/worker_threads.test.ts already use.
421b9d5 to
8faa433
Compare
|
Rebased onto main again (8faa433) to clear the merge conflict. The only conflict was in the One small test-harness adjustment: the |
…the Strong.h include Main's MessagePort rework (#31216) added a deferred 'close' event after close() and a peerClosed() path, both of which dispatch after my earlier commit's "can never dispatch again" clear and without an AsyncContextFrameScope. Verified against Node v26.3.0: both sides' 'close' listeners see the port's creation context. dispatchCloseEvent() now scopes its dispatch and releases the snapshot afterwards (one-shot, so that is the last possible dispatch), matching Worker::dispatchExit(). close()'s early clear is removed; the terminating branch that skips dispatchCloseEvent() keeps one. Adds a fixture. Main's #31216 also added JSC::Strong<JSC::JSPromise> members to Worker.h for postMessageToThread; my Strong-to-Weak revert commit (written before that landed) was dropping the pre-existing Strong.h include that those still need. Keep it.
…k, not dispatchCloseEvent peerClosed() reaches dispatchCloseEvent() without setting m_isDetached, so clearing the snapshot there would drop the context for a buffered message that a later-added onmessage listener can still drain. Verified against Node v26.3.0: in that ordering it delivers both the late message and the close, both with the port's creation context. Clear from close()'s deferred task lambda instead (m_isDetached is already set there), mirroring Worker::dispatchExit.
There was a problem hiding this comment.
No issues found on 8560b4e. This is GC-sensitive C++ across four EventTarget subclasses (new visitChildrenImpl overrides, a JSC::Strong vs wrapper-visited JSValueInWrappedObject split, FFI release-path invariants) and has already been through several correctness iterations here — the final Strong-vs-Weak shape is worth a maintainer's eyes.
What was reviewed:
MessagePort::close()'s deferred task now owns the.clear();peerClosed()leaves the snapshot intact so late-attached listeners still see buffered messages in context.signalAbort()readsm_timeoutAsyncContextinto the scope beforemarkAborted()→cancelTimer()clears it; the value stays rooted viam_asyncContextData[0]across dispatch.- Both Rust timer-teardown paths (
cancel_all_timeout_objects, the stale-generation branch inTimeout::run) callclear_timeout_async_context()beforeunref(). Worker.hkeeps the explicit<JavaScriptCore/Strong.h>include form_pendingCrossVMRequests.
Extended reasoning...
Overview
The PR snapshots the active AsyncLocalStorage context at resource creation for MessagePort, Worker, BroadcastChannel, and AbortSignal.timeout(), and restores it (via a new RAII AsyncContextFrameScope) around every asynchronous event dispatch on those objects. It touches 27 files: the AsyncContextFrame primitive, four WebCore impl/header pairs, four JS wrapper classes (adding visitChildrenImpl / visitAdditionalChildrenInGCThread), Rust-side AbortSignal timer teardown, a new C-ABI shim in bindings.cpp, and seven test fixtures plus a leak regression test.
Security risks
None identified. The change reads and writes the per-VM m_asyncContextData internal-field tuple that AsyncLocalStorage already uses; it does not touch auth, crypto, permissions, or any untrusted-input parsing. The new FFI symbol (WebCore__AbortSignal__clearTimeoutAsyncContext) is a trivial .clear() on a JSC::Strong.
Level of scrutiny
High. This is GC-lifetime code in the JSC bindings — JSValueInWrappedObject visited from wrapper visitChildren, a JSC::Strong on a ref-counted object whose wrapper holds a Ref back to it, and release-path invariants that span C++ and Rust. The review history bears that out: across ~10 rounds, real bugs were found at nearly every iteration (scope-ordering vs Node parity, two separate wrapper-liveness holes, an uncollectable-cycle regression from a Strong swap, a Rust teardown path that bypassed the release funnel, and two rebase-interaction gaps around the new MessagePort close event). Each was addressed, and the current bug-hunting pass on 8560b4e found nothing.
Other factors
The author has explicitly flagged the Strong-vs-Weak split as the subtle part deserving human review, and the invariant is documented on AsyncContextFrame::captureCurrentContext. Test coverage is good: every fixture is dual-run under Bun and Node, there are GC-stress fixtures for both wrapper-liveness holes, and a heapStats-based leak regression for the cycle case. All prior inline threads on this PR are resolved. Given the scope and the number of iterations it took to converge, this should not be auto-approved; a maintainer familiar with JSC GC semantics should confirm the final shape (particularly that the JSValueInWrappedObject members being visited only from visitChildrenImpl — not visitAdditionalChildrenInGCThread — is sufficient given the set-once-then-only-cleared invariant, and that AbortSignal's Strong release is truly reached on every terminal path).
|
CI for 8560b4e (build 72360) finished at 283 of 286 passed. None of the three reds intersect this change:
The previous build on this branch (72339 for 8faa433) ended 285/286 with the single red |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-12, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
Summary
AsyncLocalStoragecontext is silently lost across four async boundaries that Node preserves. Each printsundefinedon Bun and the stored value on Node v26.3.0:BroadcastChannelhas the same bug (it is the same pattern), so it is fixed here too.AsyncLocalStorageis what tracing/APM and request-context libraries are built on, and these boundaries do not error when the store is dropped; downstream code just seesundefinedand attributes work to the wrong request.AbortSignal.timeoutin particular is common inside instrumentedfetchwrappers.Cause
Nothing in
src/jsc/bindings/webcore/referencesAsyncContextFrame. All four objects dispatch their events from posted event-loop tasks, not from the user's JS stack:AbortSignal.timeoutfires from the per-VM timer heap (AbortSignal.rsTimeout::dispatch->AbortSignal::signalAbort->runAbortSteps)MessagePortfromMessagePortPipe::drainAndDispatch->MessagePort::dispatchOneMessageWorker(parent side) fromWorker::drainToParentand the otherpostTaskToParentlambdasBroadcastChannelfrom theBunBroadcastChannelRegistryfan-out ->BroadcastChannel::dispatchMessageBy the time
dispatchEventruns,m_asyncContextData[0]is whatever the event loop left there, which isundefined. The synchronous dispatches that already work (AbortController.abort(),EventEmitter.emit()) work because the caller's context is still on the stack.The semantic to match, verified empirically against Node v26.3.0: listeners observe the async context that was active when the resource was created (
AbortSignal.timeout()call,new MessageChannel(),new Worker(),new BroadcastChannel()), not the one at listener registration and not the one atpostMessage(). That follows from Node'sAsyncWrap:AsyncWrap::AsyncResetcapturesasync_context_frame::current()intocontext_frame_at construction andMakeCallbackrestores it.AbortSignal.timeout()gets the same result from the internalsetTimeoutthat backs it.Fix
AbortSignal, attimeout(), its only asynchronous entry point) via a newAsyncContextFrame::captureCurrentContext().MessagePort,Worker, andBroadcastChannelthe snapshot lives in aJSValueInWrappedObjectvisited from the object's JS wrapper, the same mechanismAbortSignal::m_reasonuses. That is deliberate: the snapshot can transitively reference the object's own wrapper (the user's store may hold the resource), so aJSC::Strongthere would be an uncollectable native-to-GC cycle, and their owners already keep the wrapper alive for exactly as long as the object can observably dispatch. The one gap isMessageChannel, whose ports have noJSMessagePortwrapper until.port1/.port2is first read, soJSMessageChannel::visitAdditionalChildrenInGCThreadalso visits both ports' captured contexts.AbortSignal.timeout()alone uses aJSC::Strong. It is the one case where the wrapper is not a reliable root (the abort is observed through a dependentAbortSignal.any()signal whose wrapper, not its own, stays alive) and the one case where aStrongcannot form an uncollectable cycle, because the timer heap guaranteescancelTimer()releases it on every terminal path, independent of GC reachability. The invariant is documented once, oncaptureCurrentContext.cancelTimer(),MessagePort::close()/disentangle(),BroadcastChannel::close(), and theWorkerclose task.AsyncContextFrameScopeRAII class inAsyncContextFrame.{h,cpp}swapsm_asyncContextData[0]to the captured value around the asynchronous dispatch and restores the previous one. It is a no-op when the captured value is undefined, mirroringwithAsyncContextIfNeeded.AbortSignal::signalAbort(only timeout signals ever have a captured value, soAbortController.abort()still runs listeners in the aborting caller's context),MessagePort::dispatchOneMessage,BroadcastChannel::dispatchMessage, and every parent-sideWorkerdispatch (message, open, error, close).Intentionally unchanged: the worker-side
WorkerGlobalScopemessage event (parentPort.on("message")inside a worker). Its receiving port is created at worker bootstrap, before any user code runs, so there is no async context to capture in Node either. Also unchanged: aMessagePorttransferred inside another message's payload does not inherit the receiving resource's context. Verified against Node v26.3.0, which behaves the same way because it deserializes the transferred ports before restoring the receiver's context; theAsyncContextFrameScopeat each dispatch site therefore wraps only the event dispatch, not the port entanglement.Test plan
test/js/node/async_hooks/AsyncLocalStorage-tracking.test.tsruns everyasync-context/async-context-*.jsfixture under both Bun and Node and requires both to exit 0, so each fixture doubles as a Node parity check.async-context-abort-signal-timeout.js,async-context-message-channel.js, andasync-context-broadcast-channel.js. The MessageChannel and BroadcastChannel fixtures also assert the discriminator (creation context wins over thepostMessage()caller's context), which Node passes.test.todoforasync-context-worker_threads-message.js, which covers the parent'smessageandexitevents.Before the fix, all four fixtures pass under Node and fail under Bun; after,
bun bd test test/js/node/async_hooks/AsyncLocalStorage-tracking.test.tsis 78 pass / 0 fail. The abort, broadcast-channel, message-channel, message-port leak,AsyncLocalStorage.test.ts, and Worker suites show no new failures, and the captured context survives forced fullBun.gc(true)cycles between creation and dispatch.no test proof · iteration 10 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts