Skip to content

Preserve AsyncLocalStorage context in unhandledRejection handlers - #31721

Open
robobun wants to merge 19 commits into
mainfrom
farm/71122839/als-unhandled-rejection
Open

Preserve AsyncLocalStorage context in unhandledRejection handlers#31721
robobun wants to merge 19 commits into
mainfrom
farm/71122839/als-unhandled-rejection

Conversation

@robobun

@robobun robobun commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

Problem

AsyncLocalStorage context propagates into uncaughtException handlers, but unhandledRejection handlers observed getStore() === undefined:

import { AsyncLocalStorage } from "node:async_hooks";
const als = new AsyncLocalStorage();

process.on("unhandledRejection", () => {
  console.log("unhandledRejection store:", als.getStore());
});

als.run(7, () => {
  new Promise((_, rej) => setTimeout(() => rej(new Error("late")), 50));
});
// bun:  unhandledRejection store: undefined
// node: unhandledRejection store: 7

Cause

Zig::GlobalObject::promiseRejectionTracker only recorded the promise. The unhandledRejection event is emitted later, from the end-of-tick drain (handleRejectedPromises()), by which point the context that was live at rejection time has been unwound. uncaughtException works because timers report the error before restoring the context.

Fix

  • promiseRejectionTracker(Reject) snapshots the async context via the existing AsyncContextFrame::withAsyncContextIfNeeded, so a rejection raised inside a context is queued as an AsyncContextFrame wrapping the promise. The pending list holds JSCells; both places that match a promise against it unwrap through one helper.
  • handleRejectedPromises() installs the captured context around the dispatch and restores it afterwards. A promise rejected with no context replays "no context" rather than inheriting whatever the drain happened to be running under (the drain is re-entrant: expect(fn).toThrow() drains synchronously).
  • A throwing unhandledRejection listener reaches uncaughtException with the slot restored (Node's processPromiseRejections restores in a finally before the throw propagates). The emit path used for unhandledRejection now returns the listener's throw to the caller instead of reporting it inside, which also means a throwing listener now halts later listeners — that is what Node's emit does; previously Bun continued to the next listener after reporting.
  • The microtask drains in unhandled_rejection() and the auto-GC that follows the dispatch run with the context cleared. Node exchanges the context frame around the per-mode dispatch only, and the propagation machinery assumes the ambient slot is undefined during a top-level drain (JSNextTickQueue resets it after draining).

Which semantic this pins: the context the promise was rejected in, not the one it was created in. That matches Node >= 24 and Node 22 with --experimental-async-context-frame, which store AsyncContextFrame.current() at rejection time and exchange() around the emit (lib/internal/process/promises.js). Node 22's default async_hooks-based AsyncLocalStorage replays the creation context instead, so the dual-runtime fixtures only cover cases where the two agree; AsyncLocalStorage.test.ts pins the distinguishing case against Bun alone.

Depends on oven-sh/WebKit#268

Two JSC microtask cases settled without the async context the call belonged to:

  • InternalMicrotask::AsyncFunctionResume restored the async context before promise->reject() / promise->resolve() — and reject() invokes the rejection tracker synchronously, so an async function failing after an await reported an already-popped slot. PromiseReactionJob settles first and restores after, with a comment explaining why. Fixed in Keep async context active while settling the async function promise WebKit#295 (merged).
  • InternalMicrotask::PromiseFinallyAwaitJob (phase 2 of .finally() — the callback's returned thenable settles) neither captured the context at schedule time nor installed it in its case, so .finally(() => Promise.reject(e)) observed undefined. This is what Use Node.js v18.x from NodeSource to use string.replaceAll method #268 now carries — it captures the context alongside the reaction in promiseFinallyReactionJob and installs it in the phase-2 case, the way PromiseFinallyReactionJob (phase 1) already does.

The AsyncGenerator* branches and PromiseFinallyReactionJob were already correct; PromiseFinallyAwaitJob was not, which an earlier version of this description got wrong.

WEBKIT_VERSION points at #268's preview build (autobuild-preview-pr-268-5f70edce, on top of WebKit a8d15c1c — which already has #295 merged and #301's AsyncContextSwapScope RAII helper, which #268 now uses). Re-pin to the autobuild tag of #268's merge commit before this merges. A/B against the two pins, same tree:

main's pin (4895f45d) preview pin (#268 on #295)
await-throw / awaited native rejection / escaped async fn FAIL: ... observed store null pass
.finally(() => Promise.reject(e)) FAIL: ... observed store null pass
async-generator await-throw / for-await pass pass
AsyncLocalStorage-tracking 75 pass / 2 todo 76 pass / 1 todo

Without an AsyncLocalStorage active the reorder is inert — the restore is guarded by a pointer that is only non-null when the continuation captured a context — which is why nothing outside the rejection paths moves.

Verification

Fixtures under async-context/ run against both Bun and Node:

  • async-context-unhandled-rejection.js: sync rejections in two different stores, a rejection with no context, a timer-deferred rejection, and a final in-context rejection followed by a context-free poll that fails if the drain leaks a context.
  • async-context-unhandled-rejection-async-fn.js (todo, see above): await-then-throw, an awaited native rejection, and an escaped async function.

Bun-only, in AsyncLocalStorage.test.ts: the rejection-time vs creation-time semantic, a same-tick .catch() emitting neither unhandledRejection nor rejectionHandled, a contextless rejection drained from inside a context, and --unhandled-rejections=strict keeping the context for uncaughtException but not for the drain that follows (also run against Node).

test/js/node/process/process.test.js's #32554 regression test is parametrized with AsyncLocalStorage so every rejection is frame-wrapped, covering both Handle-path unwrap sites.

Each clause was mutation-tested: reverting either unwrap, deleting the restore-after-dispatch, dropping the replay-undefined branch, or removing the drain's context-clearing guard each breaks at least one test. The termination-path restore (isTerminationException) has no test: it only runs while the VM is being torn down, so there is no point at which JS could observe the slot.

No regressions in test/js/node/async_hooks/ (77 incl. 2 todo), process.test.js, the 14 test-promise*unhandled* / *rejection* parallel tests (every --unhandled-rejections mode plus rejectionHandled), event-emitter, and timers.promises.


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

@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

This PR captures the async context when promises are rejected and restores that context while invoking unhandled-rejection handlers so AsyncLocalStorage values are observable in those handlers.

Changes

Async context preservation for unhandled promise rejections

Layer / File(s) Summary
Queue data structure update
src/jsc/bindings/ZigGlobalObject.h
The m_aboutToBeNotifiedRejectedPromises member type changes from WriteBarrierList<JSPromise> to WriteBarrierList<JSCell> so queue entries can be either raw promises or AsyncContextFrame wrappers carrying rejection-time context.
Promise rejection tracking and handling with async context
src/jsc/bindings/ZigGlobalObject.cpp
promiseRejectionTracker captures async context on Reject and wraps promises in AsyncContextFrame when present; Handle and handleRejectedPromises recognize frame-wrapped entries, extract the promise/context, restore the captured context while calling Bun__handleRejectedPromise, and then restore the prior context.
Test verification of async context preservation
test/js/node/async_hooks/AsyncLocalStorage.test.ts, test/js/node/async_hooks/async-context/async-context-unhandled-rejection.js
Adds tests that run a subprocess and an inline script to assert that AsyncLocalStorage.getStore() returns the rejection-time store value inside unhandledRejection handlers across multiple scenarios.

Suggested reviewers

  • Jarred-Sumner
🚥 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 summarizes the main change: preserving AsyncLocalStorage context for unhandledRejection handlers.
Description check ✅ Passed The description covers the problem, fix, and verification, though it doesn't use the template's exact section headings.

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

@robobun

robobun commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 10:18 AM PT - Jul 18th, 2026

@autofix-ci[bot], your commit 30e1f88 has 3 failures in Build #75466 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31721

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

bun-31721 --bun

@github-actions github-actions Bot added the claude label Jun 2, 2026
Comment thread src/jsc/bindings/ZigGlobalObject.cpp Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/node/async_hooks/AsyncLocalStorage.test.ts`:
- Around line 590-596: The test currently sets proc with stderr: "pipe" but
never consumes it; update the Promise.all consumer to drain stderr as well by
awaiting proc.stderr.text() (e.g., const [stdout, stderr, exitCode] = await
Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited])); then add an
assertion or include stderr in the failure diagnostics (for example
expect(stderr).toBe("") or include it in the test error messages) so the pipe
cannot fill and test failures surface stderr content; locate and change the code
around the proc usage and the existing stdout/exitCode awaits.
🪄 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: 674e4315-278d-4d16-ad2e-4908415d8e51

📥 Commits

Reviewing files that changed from the base of the PR and between f58d146 and 8faa3ca.

📒 Files selected for processing (4)
  • src/jsc/bindings/ZigGlobalObject.cpp
  • src/jsc/bindings/ZigGlobalObject.h
  • test/js/node/async_hooks/AsyncLocalStorage.test.ts
  • test/js/node/async_hooks/async-context/async-context-unhandled-rejection.js

Comment thread test/js/node/async_hooks/AsyncLocalStorage.test.ts Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — the contextless-drain edge case and the stderr nit are both addressed; no further issues found. Leaving final sign-off to a maintainer since this touches the core promise-rejection tracker and GC-visible state in ZigGlobalObject.

Extended reasoning...

Overview

This PR makes unhandledRejection handlers observe the AsyncLocalStorage context that was active when the promise was rejected, matching Node.js. It changes m_aboutToBeNotifiedRejectedPromises from WriteBarrierList<JSPromise> to WriteBarrierList<JSCell> so entries can be either a raw JSPromise or an AsyncContextFrame wrapping the promise plus the captured context. promiseRejectionTracker snapshots the context on Reject and unwraps on Handle; handleRejectedPromises() installs the captured context (or jsUndefined() for contextless entries when tracking is enabled) around Bun__handleRejectedPromise, then restores. Two new tests cover the behavior, including a dual-runtime fixture run against both bun and node.

Security risks

None identified. This is a behavioral correctness fix for async-context propagation; no auth, crypto, untrusted input parsing, or privilege boundaries are involved.

Level of scrutiny

Moderate-to-high. ZigGlobalObject.cpp is core runtime: the promise-rejection tracker fires on every unhandled rejection, the pending list is GC-visited, and the drain now does uncheckedDowncast<JSPromise> based on the invariant that only JSPromise or AsyncContextFrame cells are ever appended. The implementation closely mirrors the existing pattern in NodeTimerObject.cpp (same m_asyncContextData save/install/restore dance) and m_asyncContextData.get() is already used unguarded throughout the file, so the new code is consistent with established idioms. The WriteBarrierList<JSCell> is still visited at the existing m_aboutToBeNotifiedRejectedPromises.visit(...) site, and AsyncContextFrame visits its own callback/context barriers, so GC reachability looks correct. Still, changing the cell type of a GC-visited container plus adding an allocation inside promiseRejectionTracker is the kind of thing a Bun/JSC maintainer should eyeball.

Other factors

  • My previous inline nit (contextless rejections leaking the ambient drain-time context when handleRejectedPromises() is re-entered from inside als.run()) was fixed in 8faa3ca by installing jsUndefined() for raw entries when tracking is enabled — verified in the current diff.
  • CodeRabbit's stderr-drain suggestion was applied in 19f2869. All review threads are resolved.
  • The bug-hunting pass on the current revision found nothing.
  • The two CI failures reported by robobun (FreeBSD linker warning, musl build-bun) are in scripts/build/ci.ts infrastructure and appear unrelated to this change.
  • Test coverage is solid: a direct subprocess test plus a dual-runtime fixture covering sync rejections in two stores, a no-context rejection, and a timer-deferred rejection.

@robobun

robobun commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator Author

Status: complete end-to-end, including the three issues the pushback review found; waiting on oven-sh/WebKit#268 merging.

WEBKIT_VERSION is autobuild-preview-pr-268-5f70edceoven-sh/WebKit#268 rebased onto WebKit a8d15c1c, which already has the AsyncFunctionResume settle-ordering fix (#295) and #301's AsyncContextSwapScope RAII helper. #268 now uses that helper (one wrapWithCurrent at the schedule point, one unwrapContextTuple + RAII scope in the case — 19 lines instead of 51) and carries only the PromiseFinallyAwaitJob fix. The async-fn fixture covers seven shapes (await-throw / awaited native rejection / escaped async fn / two async-generator shapes / both .finally() shapes) and passes on bun and node alike.

Every clause is mutation-tested: reverting either Handle-path unwrap, deleting the restore-after-dispatch, dropping the replay-undefined branch, removing the drain's context-clearing guard, or reverting the throwing-listener handling to the catching emit each breaks at least one test.

Remaining action (maintainer): merge oven-sh/WebKit#268, then re-pin from the preview tag to its merge commit's autobuild tag before this lands. I'll push the re-pin the moment it merges.


pushback findings (comment here): three should-fix concerns survived both refutation rounds. All three verified empirically, all three fixed. One caught a real hole in my WebKit audit — PromiseFinallyAwaitJob doesn't carry the context across, so .finally(() => Promise.reject(e)) observed undefined. I'd wrongly called that arm "already correct"; the PR body has been corrected.

CI on 30e1f88 (build #75466): only owned main breaks — node-net.test.ts (mimalloc page-count threshold) and test-net-connect-memleak.js (GC-timing flake), each confirmed owned by another session. binary-size passes. Every test this PR adds or changes passes.

@robobun
robobun force-pushed the farm/71122839/als-unhandled-rejection branch from 2475297 to 035b030 Compare June 29, 2026 16:41
@mintlify

mintlify Bot commented Jun 29, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
bun 🟢 Ready View Preview Jun 29, 2026, 4:49 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@alii alii left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — the core design here (snapshot the rejection-time context onto the pending entry, install it around the whole per-mode dispatch) is the right shape, and I verified it matches what Node >= 24 does in lib/internal/process/promises.js (store AsyncContextFrame.current() at rejection time, exchange() around the emit). Requesting changes for one blocker and five smaller items; details with file:line traces in the inline comments.

The blocker: the headline scenario — als.run(id, async () => { await x; throw e }), i.e. an async function failing after an await, which is the most common unhandled rejection there is — still observes getStore() === undefined with this PR, and no test in the PR can notice. The root cause is an ordering inconsistency in our WebKit fork: JSMicrotask.cpp's AsyncFunctionResume error branch restores the async context before promise->reject(), while PromiseReactionJob a page above settles first and restores after (and has a comment saying exactly why). Your snapshot hook fires inside that reject, so it reads the already-popped context. Every rejection in the test matrix is a synchronous Promise.reject or a setTimeout callback — the two paths that already keep the context installed — so the suite passes and the node-parity fixture certifies a case it never distinguishes. Fixing this properly is a one-line ordering change in the WebKit fork (plus the same audit on the sibling async-generator/finally branches) and a pin bump in this PR — details on the inline comment.

Summary of asks:

  1. (blocker) Fix the AsyncFunctionResume restore-vs-reject ordering in the WebKit fork + bump the pin here; add await-throw / awaited-native-rejection / escaped-async-fn legs to the fixture and confirm they fail first.
  2. Replace the hand-rolled Reject block with the existing AsyncContextFrame::withAsyncContextIfNeeded helper (drops a redundant flag gate and a dead null check).
  3. Make each of the two restore mechanisms in handleRejectedPromises() individually load-bearing under test — today either one can be deleted and every test still passes.
  4. Add the one case that pins the semantic this PR chooses (rejection-time vs creation-context — they differ, and Node itself flipped between 22 and >= 24), and note the Node-version dependency: the parity harness runs an unpinned system node.
  5. Cover the Handle-path unwrap sites with a frame-wrapped entry (they currently have zero coverage, and reverting either one silently regresses #32554).
  6. Don't run the microtask drain + GC inside the installed-context window (--unhandled-rejections=warn|strict|throw|none all do today); add a strict-mode test.

Happy to re-review quickly. The design is right — the blocker is that it doesn't yet handle the case it was built for, and the test matrix was (accidentally) constructed so it can't tell.

Comment thread src/jsc/bindings/ZigGlobalObject.cpp Outdated
if (auto* asyncContextData = globalObj->m_asyncContextData.get()) {
JSC::JSValue context = asyncContextData->getInternalField(0);
if (!context.isUndefined())
entry = AsyncContextFrame::create(obj->vm(), globalObj->AsyncContextFrameStructure(), promise, context);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker. This snapshot is taken too late for the most important rejection shape: an async function that throws (or awaits a rejection) after its first await.

Trace, at this PR's pinned WebKit (scripts/build/deps/webkit.ts → the vendor/WebKit checkout):

  • JSMicrotask.cpp, case InternalMicrotask::AsyncFunctionResume, error branch (~1964–1973): it does asyncContextData->putInternalField(vm, 0, restoreAsyncContext) and then promise->reject(vm, error).
  • promise->reject()rejectPromise()promiseRejectionTracker(..., Reject) fires synchronously.
  • So by the time this line reads m_asyncContextData->getInternalField(0), the async function's [als, ctx] has already been popped back to the outer (usually undefined) value → the entry is stored raw → the unhandledRejection listener sees undefined.

Contrast PromiseReactionJob (~1832 in the same file), which settles first and restores after, with the comment "Note: Keep async context active during resolvePromise/rejectPromise …". That's why the PR's two sync fixtures and the timer fixture work — they never go through AsyncFunctionResume. The one path with the inverted order is the one path with no coverage, and it's the canonical one:

als.run(7, async () => { await Bun.sleep(5); throw new Error("late"); });
// with this PR: unhandledRejection sees undefined. Node prints 7.

Two asks:

  1. Add these legs to async-context-unhandled-rejection.js (they run against Node too, so they double as the parity proof) and confirm they fail on the current PR build before changing anything:
    • als.run({test:"await-throw"}, async () => { await sleep(5); throw new Error("await-throw"); })
    • als.run({test:"await-native-reject"}, async () => { await fetch("http://127.0.0.1:1/"); })
    • const p = als.run(ctx, () => asyncFn()) with nobody catching p
  2. Fix the root cause in the WebKit fork rather than working around it here: in AsyncFunctionResume, restore the async context after promise->reject() / promise->resolve(), matching PromiseReactionJob's documented ordering, and bump the pin in this PR. While there, please audit the sibling terminal branches (the Executing resolve arm, AsyncGeneratorBodyCall*, PromiseFinallyReactionJob) for the same restore-before-settle inversion — it's the same bug class.

Without the WebKit half, this feature returns undefined for its own motivating case while shipping a green node-parity fixture.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed upstream: oven-sh/WebKit#268.

You're right on every point. AsyncFunctionResume restores the slot at JSMicrotask.cpp:1990-1991 and then calls promise->reject() at 1994, while PromiseReactionJob a few cases above settles first and restores after (with the comment explaining why). Reproduced with the full bun-side fix in place and the current pin:

FAIL: unhandledRejection for "await-throw" observed store null, expected "await-throw"

(the sync, no-context and timer legs all pass at that point, which is exactly why the old matrix couldn't see it).

The WebKit PR settles before restoring in both terminal arms. I audited every putInternalField(vm, 0, restoreAsyncContext) site in the file: only those two were inverted. AsyncGeneratorYieldAwaited, AsyncGeneratorBodyCallNormal/Return, AsyncGeneratorAwaitReturnContinuation, PromiseFinallyReactionJob and the await-continuation arm all already restore after their call. It doesn't change what .then()/.catch() handlers observe, since performPromiseThen() captures at registration time; what changes is the tracker and any thenable job the settle schedules.

The three legs are in async-context/async-context-unhandled-rejection-async-fn.js (await-throw, awaited native rejection via fs.promises.readFile of a missing path, escaped async fn). They pass on Node and fail on the current pin, so the file is in the tracking test's todos with a comment pointing at the WebKit PR — I'll drop the todo and bump WEBKIT_VERSION in this PR as soon as #268 merges and an autobuild tag exists. Worth flagging: the preview-build workflow on oven-sh/WebKit currently fails before building (actions/github-script@v7 isn't SHA-pinned and the org requires that), so I can't pin a preview tag in the meantime.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pin bumped in 0cde717 — the blocker is closed end-to-end.

oven-sh/WebKit#268's preview build is green across all 43 artifacts, so WEBKIT_VERSION now points at autobuild-preview-pr-268-ee98a203 and the async-fn fixture is out of todos. (The preview workflow had been failing for an unrelated reason — actions/github-script@v7 wasn't SHA-pinned; WebKit main fixed that in #269, so rebasing my branch onto it was enough.) Re-pin to the autobuild tag of the merge commit before this lands — happy to do that the moment #268 merges.

A/B on the same tree, flipping only the pin:

old pin preview pin
await-throw, awaited native rejection, escaped async fn FAIL: ... observed store null pass (bun and node)
AsyncLocalStorage-tracking 75 pass / 2 todo 76 pass / 1 todo

I also baselined the only other thing that moved locally: three setTimeout doesn't leak ... RSS-threshold tests fail under debug+ASAN on both pins, so they're pre-existing and unrelated. That matches the code — without an AsyncLocalStorage active, asyncContextData is null and the reorder is inert, so nothing outside the rejection paths can move.

Also smoke-tested the engine change beyond this feature: all 36 test-promise*/test-async-*/test-microtask* parallel tests, event-emitter, timers.promises and the async_hooks suites are green.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Landed in c626ed7 — pin is autobuild-preview-pr-268-48232e38 (on top of WebKit 4895f45d, so #34009's allocator change is kept). Leaving this open: re-pin to the autobuild tag of the merge commit once #268 lands on WebKit main.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: the AsyncFunctionResume half landed independently as oven-sh/WebKit#295, so #268 was rebased onto it and now carries only the PromiseFinallyAwaitJob fix. bun's pin is bumped to the new preview (86efe056, on top of #295). All 7 async-fn fixture cases pass on bun and node. Still open: re-pin to #268's merge-commit tag once it lands.

Comment thread src/jsc/bindings/ZigGlobalObject.cpp Outdated
Comment thread src/jsc/bindings/ZigGlobalObject.cpp
Comment thread src/jsc/bindings/ZigGlobalObject.cpp
Comment thread src/jsc/bindings/ZigGlobalObject.cpp
@alii

alii commented Jul 2, 2026

Copy link
Copy Markdown
Member

@robobun please address the review feedback above (#31721 (review)).

Suggested order: start with the blocker's WebKit half — fix the restore-vs-settle ordering in AsyncFunctionResume (and audit its sibling terminal branches) in the WebKit fork, bump the pin in this PR, and add the await-throw / awaited-native-rejection fixture legs, confirming they fail before the WebKit fix. Then the withAsyncContextIfNeeded swap, then the test-hardening items (each restore clause individually load-bearing, the frame-wrapped Handle-path coverage via parametrizing the existing #32554 test, the strict-mode leg), and the drain-outside-the-window fix in VirtualMachine::unhandled_rejection. Please don't raise any test timeouts, and note in the PR description which semantic you're pinning (rejection-time, i.e. Node >= 24) and why.

@robobun

robobun commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks — this was a genuinely good catch, and the test matrix was constructed so it couldn't see the blocker. All six items are addressed; details in the inline replies.

The blocker

Confirmed exactly as you described, and fixed upstream in oven-sh/WebKit#268. AsyncFunctionResume restores the context slot at JSMicrotask.cpp:1990-1991 and then calls promise->reject() at 1994; reject() runs the tracker synchronously, so the snapshot reads an already-popped slot. PromiseReactionJob settles first and restores after, with a comment saying why. The WebKit PR makes both of AsyncFunctionResume's terminal arms match it.

I audited every putInternalField(vm, 0, restoreAsyncContext) site in JSMicrotask.cpp: only those two were inverted — the AsyncGenerator* branches, PromiseFinallyReactionJob and the await-continuation arm all restore after their call already. The reorder doesn't move what .then()/.catch() handlers see, because performPromiseThen() captures at registration time; what moves is the tracker and any thenable job the settle schedules, which is the property PromiseReactionJob already guarantees.

Fail-before, with the complete bun-side fix and the current pin:

FAIL: unhandledRejection for "await-throw" observed store null, expected "await-throw"

The await-throw / awaited-native-rejection / escaped-async-fn legs live in async-context/async-context-unhandled-rejection-async-fn.js. They pass on Node today and fail on bun until the pin moves, so the file sits in the tracking test's todos with a comment pointing at the WebKit PR. I'll drop the todo and bump WEBKIT_VERSION in this PR the moment #268 merges and an autobuild tag exists — note the preview-build workflow on oven-sh/WebKit currently dies before building (actions/github-script@v7 isn't SHA-pinned and the org requires pinned actions), so I can't pin a preview tag in the meantime.

The rest

  • withAsyncContextIfNeeded — swapped in; the tracking-enabled gate and the dead null check are gone.
  • Drain outside the windowunhandled_rejection's drains and the auto-GC that follows the dispatch now run under a ClearedAsyncContextScope (RAII exchange). The install still brackets the whole per-mode dispatch, and that's pinned by a test so it can't be narrowed later.
  • Each restore clause individually load-bearing — my first attempt at your setTimeout probe was accidentally order-dependent (a contextless entry draining last scrubs the slot), so the fixture now ends with an in-context rejection queued by the poll itself, and the poll checks the slot every iteration from outside any context.
  • Frame-wrapped Handle path — the jsc: drain rejected-promise list in O(n) instead of O(n^2) #32554 test is parametrized with AsyncLocalStorage; reverting the in-flight unwrap fails only that variant, with spurious rejectionHandled fired 1x. The weak timer-duplicate test is replaced by a same-tick-.catch() test that covers the other unwrap.
  • Semanticthe rejection-time context wins over the creation-time context pins created-in-A/rejected-from-B → B, bun-only, with the Node-version dependency written down in the test and in the PR description.

Every clause was mutation-tested: reverting either unwrap, deleting the restore-after-dispatch, dropping the replay-undefined branch, or removing the drain guard each breaks at least one test. The one thing I couldn't cover is the termination-path restore — it only runs while the VM is being torn down, so there's no point at which JS could observe the slot; that's stated in the description rather than tested.

One correction to your reproduction note for the replay-undefined branch: under bun test an unhandled rejection never reaches process.on("unhandledRejection") (the isBunTest branch routes to the runner's reporting), so expect(fn).toThrow() inside a test can't observe it. It is observable in a plain script that imports expect from bun:test, which is what the new test does — on main it prints the caller's store leaking into a contextless rejection's handler.

No test timeouts were raised.

@robobun

robobun commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator Author

Pin bumped in 0cde717 — the blocker is now fixed end-to-end in CI, not just argued for.

oven-sh/WebKit#268's preview build went green across all 43 artifacts, so WEBKIT_VERSION points at autobuild-preview-pr-268-ee98a203 and async-context-unhandled-rejection-async-fn.js is out of the tracking test's todos. (The preview workflow had been dying before it built anything — actions/github-script@v7 wasn't SHA-pinned, which the org forbids; WebKit main fixed that in #269, so rebasing onto it was all it took.)

A/B on the same tree, flipping only the pin:

old pin preview pin
await-throw, awaited native rejection, escaped async fn FAIL: ... observed store null pass, on bun and node
AsyncLocalStorage-tracking 75 pass / 2 todo 76 pass / 1 todo

I baselined the only other thing that moved locally, too: three setTimeout doesn't leak ... RSS-threshold tests fail under debug+ASAN on both pins, so they're pre-existing and unrelated. That lines up with the code — with no AsyncLocalStorage active, asyncContextData is null and the reorder is inert, so nothing outside the rejection paths can move. Beyond this feature I also ran all 36 test-promise* / test-async-* / test-microtask* parallel tests, event-emitter, timers.promises and the async_hooks suites: green.

One thing left, and it needs a human: the pin points at a preview tag, so oven-sh/WebKit#268 has to merge and then this wants a re-pin to its merge commit's autobuild tag. I've left that review thread open as the reminder and I'm happy to push the re-pin the moment it lands.

Comment thread test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts Outdated
@robobun

robobun commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

pushback results

Ran farm:rdr/pushback (76 agents, 4 rounds). Three should-fix concerns survived both refutation rounds. I verified each empirically before touching anything, and one caught a real hole in my WebKit audit.

1. A throwing unhandledRejection listener leaked the promise's store into uncaughtException — fixed in a56f796

bun (before):  unhandledRejection store: 7  /  uncaughtException store: 7
node v26.3.0:  unhandledRejection store: 7  /  uncaughtException store: null

Node's processPromiseRejections restores the frame in a finally before the throw propagates. Bun's EventEmitter catches listener throws inside emit and calls Bun__reportUnhandledError there — inside my installed window. Fixed by adding an emit overload that returns the exception instead of reporting it; Bun__handleUnhandledRejection now reports the throw itself after clearing the slot. handleRejectedPromises also restores before reportUncaughtExceptionAtEventLoop for anything that escapes the whole dispatch. Dual-runtime test added; mutation-tested (reverting to the catching emit fails the bun variant, node passes).

2. The isBunTest early-return ran the test runner's handler with the context installed, and nothing tested that path — fixed in a56f796

The next-test-callback leak the concern described doesn't actually reproduce (handleRejectedPromises' restore cleans it before the next test runs), but nothing guarded the path and nothing tested isBunTest=true. Wrapped the handler call in ClearedAsyncContextScope and added a spawned-bun test fixture so the path is exercised.

3. .finally(() => Promise.reject(e)) lost the store — my audit missed PromiseFinallyAwaitJob — fixed upstream in oven-sh/WebKit@0aef04ea

bun (preview pin 48232e38):  finally-throw=ok asyncgen=ok  finally-returns-rejected=null
node:                        finally-throw=ok asyncgen=ok  finally-returns-rejected=ok

I'd claimed the PromiseFinally arms were "already correct". PromiseFinallyReactionJob (phase 1 — the callback runs) is; PromiseFinallyAwaitJob (phase 2 — the callback's returned thenable settles) isn't. It neither captures the context at schedule time nor installs it in its microtask case. Fixed in oven-sh/WebKit#268 the same way the neighbouring case does; preview rebuilding now. Added four fixture cases (both finally shapes, two async-generator shapes) — all pass on node; the file is back in todos until the pin picks up 0aef04ea.

A fourth "consider"-severity concern was truncated in the result.

What's still open

Comment thread test/js/node/async_hooks/AsyncLocalStorage.test.ts Outdated
Comment thread src/jsc/bindings/webcore/EventEmitter.cpp
Comment thread src/jsc/bindings/BunProcess.cpp
Comment thread test/js/node/async_hooks/AsyncLocalStorage.test.ts Outdated
@robobun
robobun force-pushed the farm/71122839/als-unhandled-rejection branch from 2481dc8 to d28503d Compare July 18, 2026 06:00
@robobun
robobun force-pushed the farm/71122839/als-unhandled-rejection branch from d28503d to 70602eb Compare July 18, 2026 11:03
robobun and others added 19 commits July 18, 2026 13:57
When a promise is rejected with no handler, the rejection is queued and
the "unhandledRejection" event is only emitted at the end of the tick,
after the async context that was live at rejection time has been
unwound — so AsyncLocalStorage.getStore() returned undefined inside the
listener.

Snapshot the async context in the promise rejection tracker (wrapping
the promise in an AsyncContextFrame when a context is active) and
reinstall it around the unhandledRejection dispatch, mirroring how
timers keep the async context installed while reporting an uncaught
exception. Matches Node.js behavior.
When async context tracking is enabled, promises rejected with no
active context must also install (undefined) around the
unhandledRejection dispatch, so a drain re-entered from inside a
context does not leak the caller's store into the listener.
…the installed window, harden tests

- promiseRejectionTracker(Reject) now goes through
  AsyncContextFrame::withAsyncContextIfNeeded instead of open-coding it,
  dropping a redundant tracking-enabled gate and a dead null check.
- unhandled_rejection()'s microtask drains and the auto-GC that follows the
  dispatch no longer run with the rejected promise's async context installed.
  Node drains outside its exchange window, and the propagation machinery
  assumes the ambient context is undefined during a top-level drain.
- Tests: make each restore clause individually load-bearing (a contextless
  rejection drained from inside a context; a drain that must restore the slot
  afterwards), pin the rejection-time semantic against the creation-time one,
  cover the frame-wrapped Handle path in both unwrap sites (same-tick catch,
  plus the #32554 regression test parametrized with AsyncLocalStorage), and
  assert --unhandled-rejections=strict keeps the context for uncaughtException
  but not for the drain that follows.
oven-sh/WebKit#268 settles the async function's promise before restoring
the async context, so a function that fails after an await reports its
rejection with its own context still installed. Un-skips the async-fn
fixture, which now passes on bun and node alike.

Pinned to the PR's preview build (on top of WebKit 4895f45d, so it keeps
the shared-allocator change from #34009). Re-pin to the autobuild tag of
its merge commit once it lands on WebKit main.
…ches uncaughtException

A throwing unhandledRejection listener is reported as an uncaught
exception, and Node's processPromiseRejections restores the previous
frame in a finally before that throw propagates to
triggerUncaughtException — so the uncaughtException handler does not
see the promise's store. Bun's EventEmitter catches listener throws
inside emit and reports them there, which was inside the installed
window.

Add an emit overload that returns the exception instead of reporting it.
Bun__handleUnhandledRejection uses it and reports the throw itself after
clearing the slot, which matches Node. handleRejectedPromises also now
restores before reportUncaughtExceptionAtEventLoop for exceptions that
escape the whole dispatch.

Also clear the slot around the isBunTest early-return so the test
runner's handler (and anything it drives) never observes the promise's
context.

Dual-runtime tests cover the throwing-listener case, and a spawned
`bun test` fixture covers the isBunTest path.
…fixture cases

The claim that these arms were already correct was wrong: a .finally()
callback that returns a rejected thenable settles from
PromiseFinallyAwaitJob, which did not carry the async context across, so
the unhandledRejection handler observed undefined. Fixed alongside the
AsyncFunctionResume ordering in oven-sh/WebKit#268.

Add fixture cases for both .finally() shapes and two async-generator
shapes. The file is back in the tracking test's todos until the WebKit
pin picks up the PromiseFinallyAwaitJob fix.
oven-sh/WebKit#268's second commit carries the async context through
PromiseFinallyAwaitJob, so a .finally() callback that returns a
rejected thenable now reports its unhandled rejection with the
callback's store. Un-skips the async-fn fixture (now 7 cases, all
passing on bun and node).

Also makes the "unhandledRejection async context" block concurrent —
8 hermetic subprocess spawns, so there's no reason to run them
sequentially.
The emit overload that returns the listener's throw (instead of
reporting it) also stops calling later listeners, which is what Node
does — Node's emit lets the throw propagate. The old path continued to
the next listener after reporting. Add a second listener to the
throwing-listener test so that Node-parity fix is load-bearing.
…tures

On a fast release build the setImmediate poll could run 10000 iterations
before a 10ms timer fired, so the fixture timed out waiting for a
rejection that was still pending. The bailout is only a safety net; use
a 30s wall-clock deadline instead of an iteration count.
The AsyncFunctionResume settle-ordering fix landed independently as
oven-sh/WebKit#295, so #268 was rebased onto it and now carries only the
PromiseFinallyAwaitJob fix. Pin to the new preview (on top of e5f7fc2b).
…tion with the slot cleared

Add a persistent enterWith("Y") to the throwing-listener test so it
distinguishes "cleared to undefined" from "restored to the drain's
ambient". Node v26's uncaughtException handler observes undefined here,
so clearing is the Node-matching choice; the test now runs the
distinguishing case against both runtimes. Rewrite the comment to state
that rather than "restores the previous context", which was ambiguous.
oven-sh/WebKit#268 rebased onto WebKit main a8d15c1c and rewritten to use
the AsyncContextSwapScope helper from #301, so it now matches every other
microtask case (one wrapWithCurrent at the schedule point, one
unwrapContextTuple + RAII scope in the case). Same behaviour, 19 lines
instead of 51.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants