Fix AsyncLocalStorage context loss in thenables returned from async functions - #34277
Fix AsyncLocalStorage context loss in thenables returned from async functions#34277robobun wants to merge 2 commits into
Conversation
…unctions Bump WEBKIT_VERSION to pick up oven-sh/WebKit#295: keep the async context active while settling the async function's promise, so a thenable returned after a suspension runs its then() with the correct context. Fixes #34266
|
Warning Review limit reached
Next review available in: 59 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 5:06 PM PT - Jul 15th, 2026
❌ @robobun, your commit cf8c0d4 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34277That installs a local version of the PR into your bun-34277 --bun |
There was a problem hiding this comment.
I didn't find any issues with the test or the version-bump mechanics, but this needs a human: the actual fix lives in oven-sh/WebKit#295 (JSC async-context restore ordering in InternalMicrotask::AsyncFunctionResume), and WEBKIT_VERSION currently points at that PR's preview build pending its merge.
What was reviewed:
- New test follows the existing thenable-test pattern in the same file;
mustCall+ the getter assertion cover both theGet(thenable, "then")lookup and thethen()invocation. prebuiltUrl/prebuiltDestDiralready handleautobuild--prefixed version strings, so the preview tag resolves correctly.- Confirmed the test is additive and doesn't touch neighboring cases.
Extended reasoning...
Overview
This PR bumps WEBKIT_VERSION in scripts/build/deps/webkit.ts from a main-branch sha to the preview-build tag autobuild-preview-pr-295-ff8154f3, and adds one regression test to test/js/node/async_hooks/async-local-storage-thenable.test.ts. The behavioral fix itself — reordering async-context restoration relative to promise->resolve()/reject() in JSC's InternalMicrotask::AsyncFunctionResume — is entirely in the companion WebKit PR (oven-sh/WebKit#295), not in this diff.
Security risks
None identified. No auth, crypto, or input-parsing surface is touched in this repo's diff. The WebKit change reorders two operations on an internal microtask path; it doesn't expose new API or accept new input.
Level of scrutiny
High. A WebKit version bump changes the JS engine for every code path in Bun, and the specific change touches async-context propagation — subtle, load-bearing, and historically a source of hard-to-diagnose regressions. The PR description also explicitly states the version pin is temporary (preview build) and will be updated once oven-sh/WebKit#295 merges, so this is not yet in its final mergeable state. Both the WebKit-side reordering and the final sha bump need human sign-off.
Other factors
The new test is well-placed (alongside the existing "Returning a thenable in an async function" case it mirrors), uses createCallCheckCtx/mustCall consistently with siblings, includes the issue URL comment, and asserts context inside both the then getter and the then body. The build-script side already special-cases autobuild--prefixed versions in both the release-tag URL and the cache-dir key, so the preview tag won't collide with or be truncated like a sha. No prior human or bot reviews to consider; CI is still building.
|
Correct: the behavioral fix is in oven-sh/WebKit#295 and the preview pin is temporary. Once that PR merges and main artifacts publish, I will update WEBKIT_VERSION here to the merged sha before this lands. |
There was a problem hiding this comment.
No issues with the in-repo changes — the new test mirrors the neighboring cases in the same file and the WEBKIT_VERSION bump is handled correctly by the existing autobuild- prefix logic in prebuiltUrl/prebuiltDestDir. Deferring to a human because the actual behavioral fix (reordering async-context restoration around promise->resolve() in JSC's AsyncFunctionResume) lives in oven-sh/WebKit#295 and touches core microtask/promise machinery, and the version pin here is still the preview tag pending that PR's merge.
What was reviewed:
- New test follows the existing
mustCall/createCallCheckCtxpattern; asserts both theget thenlookup and thethen()body see the store. prebuiltUrlalready branches onstartsWith("autobuild-")andprebuiltDestDirslices the full tag, so the non-hex version string doesn't break tarball resolution or cache keying.- Checked that
await store.run(...)awaiting the returned thenable is consistent with the sibling "Returning a thenable in an async function" test.
Extended reasoning...
Overview
Two files: a one-line WEBKIT_VERSION bump in scripts/build/deps/webkit.ts (to a preview build of oven-sh/WebKit#295) and a new 24-line test case in test/js/node/async_hooks/async-local-storage-thenable.test.ts. The behavioral fix — swapping the order of "restore outer async context" and "settle promise" in JSC's InternalMicrotask::AsyncFunctionResume completed-body path — is entirely in the WebKit fork, not in this diff.
Security risks
None in the in-repo diff. The WebKit change itself alters when the async-context tuple is restored relative to JSPromise::resolvePromise; that's correctness-sensitive (context leakage in either direction) but not a security boundary.
Level of scrutiny
High. WebKit version bumps change the JS engine for every user, and the specific change is in the promise/microtask scheduling path — one of the most subtle areas of the runtime, where ordering bugs manifest as rare context leaks or hangs rather than immediate failures. The PR description's cause/fix analysis is coherent and matches the observed symptom, but I can't review the actual C++ diff from here, and the author explicitly notes the version pin is temporary pending the WebKit PR's merge. A human should confirm WebKit#295 is reviewed/merged and that WEBKIT_VERSION is updated to the final sha before this lands.
Other factors
The new test is a close copy of the existing "Returning a thenable in an async function" case with await null inserted and then moved behind a getter — both deltas are exactly what the bug report calls for. mustCall(..., 1) plus the done callback guarantees the assertion inside then actually runs. The build-script side already handles autobuild-* version strings (both for the release-tag URL and the cache-dir key), so the non-hex pin doesn't introduce a resolution bug. No prior claude[bot] reviews on this PR.
|
CI status for build 73436: the only red tests are pre-existing failures on main, unrelated to this diff:
The remaining failures are Windows lanes that passed on retry. The new test here passes on all lanes. This PR is otherwise ready, pending oven-sh/WebKit#295: once that merges and main artifacts publish, WEBKIT_VERSION gets updated to the merged sha. |
|
#34266 was fixed in oven-sh/WebKit#295 (e5f7fc2b1a), which bun picked up with the WebKit bump in #34669 and every bump since. Current main (165dc9f, WebKit 7b763944f0) includes that commit, and the test added here ("Returning a thenable in an async function after an await") passes on plain main. The remaining change in this PR pins WEBKIT_VERSION to a preview build, which is no longer wanted. Closing as superseded. |
Fixes #34266. Depends on oven-sh/WebKit#295;
WEBKIT_VERSIONcurrently points at that PR's preview build (autobuild-preview-pr-295-ff8154f3) and will be updated to the merged main sha before this merges.Problem
A thenable returned from an async function that has suspended at least once runs its
then()(and, for a getter-definedthen, theGet(thenable, "then")lookup) with an empty AsyncLocalStorage context:Bun prints
store in then(): undefined; Node printsCTX. All neighboring cases (directawait thenable,Promise.resolve(thenable), return with no prior await) already preserve the context. This breaks ORMs that expose queries as thenables and read the ambient transaction from AsyncLocalStorage insidethen()(Knex, Objection.js, Orchid ORM): the query silently runs outside the transaction.Cause
In JSC's
InternalMicrotask::AsyncFunctionResume, the body-completed paths restored Bun's async context before callingpromise->resolve()/promise->reject().JSPromise::resolvePromiseperformsGet(resolution, "then")synchronously and captures the current async context into the queuedPromiseResolveThenableJob, so by then the context had already been swapped back to the outer (empty) one.Fix
oven-sh/WebKit#295: settle the promise first, then restore the outer context, matching the ordering
PromiseReactionJoband the await-suspension path in the same handler already use.Verification
New test "Returning a thenable in an async function after an await" in
test/js/node/async_hooks/async-local-storage-thenable.test.ts(covers both the getter lookup and thethen()call). Fails with the previous prebuilt WebKit, passes with the bumped one.test/js/node/async_hooks/passes (112 pass, 0 fail), andtest/js/bun/jsc/test/js/web/fetchfailure sets are identical to an unpatched debug build (pre-existing debug-build timing failures only).[decide:webkit] gate passed · iteration 1 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
When an async function that had suspended at least once returned a thenable, JSC's InternalMicrotask::AsyncFunctionResume invoked promise resolution before restoring the saved async context, so the Get(thenable, "then") lookup and the subsequent then() call ran with an empty AsyncLocalStorage store. The fix reorders the resume path so the async context is restored prior to calling resolve or reject, ensuring the thenable's then() observes the context that was active in the returning async function. This matches Node.js behavior and the already correct neighboring paths such as direct await …