error: recover async stack frames dropped under AsyncLocalStorage - #35772
error: recover async stack frames dropped under AsyncLocalStorage#35772robobun wants to merge 5 commits into
Conversation
When an AsyncLocalStorage store is active at an await point, JSPromise::resolveWithInternalMicrotaskForAsyncAwait wraps the awaiting generator in an InternalFieldTuple(generator, asyncContext) and stores that as the reaction context. Interpreter::getAsyncStackTrace reads the reaction context and casts it with dynamicDowncast<JSAsyncFunctionGenerator>, which fails on the tuple, so every 'at async <fn>' frame is dropped from error.stack. Install a VM::onAppendStackTrace hook that replicates getParentGenerator with tuple unwrapping and appends the frames JSC's walk misses. The hook runs the unwrapped and non-unwrapped walks in lockstep so frames JSC does find (when ALS was inactive at some hop) are not duplicated. Fixes #24003.
|
Warning Review limit reached
Next review available in: 18 minutes 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 (4)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ddf954c to
57c5923
Compare
The hook is now registered the first time AsyncLocalStorage turns tracking on, so code that never touches ALS pays nothing at error-capture time. Also point at oven-sh/WebKit#347, which unwraps the tuple inside getAsyncStackTrace and will make this hook unnecessary once WEBKIT_VERSION moves past it.
57c5923 to
052714f
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/install/lockfile/Package/Scripts.rs:140-144— This one-line change (self.preinstall→self.postinstall) is a real behavioral fix — packages withbinding.gyp+ apostinstallscript previously had their postinstall silently replaced with an empty string — but it's completely unrelated to async stack traces, isn't mentioned in the PR title/description/commits, and ships with no test. Per REVIEW.md every behavioral change ships an automated test in the same PR; this looks like a stray edit from another branch and should be split into its own PR with coverage intest/cli/install/, or at minimum acknowledged in the description with a test added here.Extended reasoning...
What the change does
get_script_entriesinScripts.rsbuilds the ordered array of lifecycle scripts to run for a package. Whenadd_node_gyp_rebuild_scriptis true, it hardcodes"node-gyp rebuild"into theinstallslot (index 1), then — ifself.postinstallis non-empty — populates thepostinstallslot (index 2). Before this PR that slot was filled withself.preinstall.slice(lockfile_buf); after, withself.postinstall.slice(lockfile_buf).Why this is a real behavioral fix, not a no-op
Both callers only set
add_node_gyp_rebuild_script = truewhenself.install.is_empty() && self.preinstall.is_empty():get_list()(Scripts.rs:296-299):self.install.is_empty() && self.preinstall.is_empty()gates thebinding.gypexistence check.create_from_package_json()(Scripts.rs:371-373): same predicate.
So inside the
add_node_gyp_rebuild_scriptbranch,self.preinstallis guaranteed empty. The old code therefore storedSome(Box::from(&[][..]))— an empty script — in the postinstall slot, while the guard immediately above it (if !self.postinstall.is_empty()) and the comment (// missing install and preinstall, only need to check postinstall) both make clearpostinstallwas the intended field. The new code stores the actual postinstall script.Step-by-step proof of the behavior change
Take a package with
binding.gyp, noinstall/preinstall, and"postinstall": "node ./verify.js":get_list()seesinstallandpreinstallempty,binding.gypexists →add_node_gyp_rebuild_script = true.get_script_entriesenters theadd_node_gyp_rebuild_scriptbranch.script_index=1gets"node-gyp rebuild".script_indexadvances to 2.!self.postinstall.is_empty()is true, so slot 2 is populated.- Before: slot 2 =
self.preinstall.slice(...)=b""→ the lifecycle runner executes an empty command for postinstall;./verify.jsnever runs. - After: slot 2 =
self.postinstall.slice(...)=b"node ./verify.js"→ postinstall runs as authored.
This is user-visible: any native addon that relies on a postinstall step after
node-gyp rebuild(verification, file copying, prebuild fallback cleanup) was silently skipping it duringbun install.Why it doesn't belong in this PR as-is
This PR is titled "error: recover async stack frames dropped under AsyncLocalStorage" and the description exhaustively covers the
AsyncStackTrace.cpp/NodeAsyncHooks.cppchanges without a single word aboutScripts.rsor lifecycle scripts. The only test added (24003.test.ts) exercises async stack traces exclusively. REVIEW.md is explicit on both counts:- "Every behavioral change ships an automated test in the same PR. 'Verified manually', unnamed 'existing tests', and benchmarks don't count, even for one-liners." This one-liner changes what
bun installexecutes for a whole class of native packages and has zero coverage. - "don't ride file-wide standardization on a focused bugfix" — the same principle applies to riding an unrelated bugfix on a focused bugfix. If this regresses,
git bisectwill point at an async-stack-trace PR, and nobody reviewing async stack traces is scrutinizing lockfile lifecycle-script assembly.
The most likely explanation is a stray commit from another branch that got picked up in a rebase — the change is correct but orphaned.
How to fix
Preferred: drop this hunk from the PR and open a separate PR with a test in
test/cli/install/that installs a fixture package containingbinding.gyp+ apostinstallthat writes a marker file, then asserts the marker exists. Alternatively, if the author wants to keep it here: add that test to this PR and mention the fix in the PR description so reviewers know it's intentional.
Constructing an AsyncLocalStorage without entering a store installs the onAppendStackTrace hook (via jsSetAsyncHooksEnabled) while leaving every await context unwrapped, so the lockstep walk runs and must append nothing.
There was a problem hiding this comment.
Both earlier findings are addressed (nested-VM ordering is now called out as a known limitation in the PR body, and 9f710c7 makes the no-store baseline tests actually install the hook). No new issues from this pass, but this replicates JSC-private getParentGenerator logic and installs a VM-level onAppendStackTrace hook as an interim workaround pending oven-sh/WebKit#347 — worth a maintainer look at the design tradeoff (land the hook now vs. wait for the WebKit bump).
What was reviewed:
- Lockstep dedup loop —
jscStoppedtransitions once,currentadvances on both branches, 256-hop cap bounds pathological chains. cellAsempty-JSValue guard vs.asyncStackTraceContext()returning empty on JSVALUE64.- Lazy hook install in
jsSetAsyncHooksEnabled— no otheronAppendStackTraceconsumer to conflict with; reached only from ALS/AsyncResource constructors. - Refactored
appendGeneratorFrame/computeGeneratorBytecodeIndexare byte-for-byte moves of the prior lambdas.
Extended reasoning...
Overview
Adds Bun::appendAsyncLocalStorageStackFrames as a VM::onAppendStackTrace hook that recovers at async <fn> frames dropped when AsyncLocalStorage wraps await-reaction contexts in an InternalFieldTuple. The hook walks VM entry frames to find the origin JSAsyncFunctionGenerator, then replicates JSC's private getParentGenerator walk twice in lockstep (with and without tuple unwrapping) so it appends only the frames JSC's own walk misses. Refactors two existing lambdas (computeBytecodeIndex, appendFrame) into file-static helpers reused by both the existing promise-chain walker and the new hook. Installs the hook lazily from jsSetAsyncHooksEnabled so code that never touches ALS pays nothing. Adds six subprocess regression tests.
Security risks
None. This is diagnostic-only (error stack formatting); it reads JSC-internal fields under AssertNoGC and never calls into user JS. No untrusted-input parsing, allocation sizing, or FFI boundary is touched.
Level of scrutiny
High. The change reaches into JSC internals not normally touched from Bun's side — VMEntryRecord::m_context, JSAsyncFunctionGenerator internal fields, asyncStackTraceContext() — and duplicates logic from a private JSC function that will drift when WebKit updates. It is explicitly framed as an interim workaround with documented limitations (combinator hops still stop under ALS; frame ordering diverges under nested VM entries), and the real fix lives in oven-sh/WebKit#347. Whether to land the interim hook or hold for the WebKit bump is a maintainer call.
Other factors
Both of my earlier inline findings were addressed: the nested-VM-entry ordering caveat is now in the PR body's Known limitations, and 9f710c7 prepends an ALS constructor to the two no-store baseline subprocesses so the lockstep-dedup path is actually exercised. The comment-cop flags were resolved by trimming the block comments. Test coverage is solid (fails on main, passes with fix, covers run/enterWith/Promise.race/mid-chain/no-store baselines). I confirmed onAppendStackTrace has no other consumer in the tree, so the !vm.onAppendStackTrace() guard is just an idempotency check. The refactored helpers are behavior-preserving moves of the previous lambdas.
|
CI is stalled on infra for this branch: #81467 had two No lane that built the code has failed a test. The earlier run on 0c8a853 passed Ready for a maintainer to re-run or merge. oven-sh/WebKit#347 has the in-tree unwrap (preview autobuild published at |
|
Closing: the in-tree fix this hook was standing in for (oven-sh/WebKit#347, unwrapping the Verified on current main (165dc9f) by applying |
1 participant
Fixes #24003.
Problem
When an
AsyncLocalStoragestore is active,error.stackfor errors created deep in anawaitchain drops everyat async <fn>frame:at fn3→at async fn2→at async fn1at fn3at fn3→at async fn2→at async fn1The same chain without ALS already shows the full trace.
Cause
Under
USE(BUN_JSC_ADDITIONS),JSPromise::resolveWithInternalMicrotaskForAsyncAwaitwraps the awaiting generator in anInternalFieldTuple(generator, asyncContext)whenever an async context is active, and stores that tuple as the reaction context so the context can be restored when the microtask resumes.Interpreter::getAsyncStackTracewalks the await chain by reading the reaction context viaasyncStackTraceContext()and casting it withdynamicDowncast<JSAsyncFunctionGenerator>. On a tuple that cast (and the combinator/promise fallbacks) fails, so the walk returns nothing and every async frame is dropped.Fix
oven-sh/WebKit#347 unwraps the tuple inside
getContextValueFromPromise, which every branch ofgetParentGeneratorflows through. That is the complete fix (directawait,Promise.all/allSettled/any,Promise.race) with no extra per-Error work.Until
WEBKIT_VERSIONmoves past that change, this PR installs aVM::onAppendStackTracehook that replicatesgetParentGeneratorwith tuple unwrapping. The hook is registered lazily fromjsSetAsyncHooksEnabledthe first time AsyncLocalStorage turns tracking on, so code that never touches ALS pays nothing. To avoid duplicating frames JSC does find (when ALS was inactive at some hop), the hook runs the unwrapped and non-unwrapped walks in lockstep and only starts appending once the non-unwrapped walk stops.Known limitations of the interim hook
Promise.all/allSettled/anyunder ALS still stop at the combinator hop;JSPromiseCombinatorsGlobalContextis a private JSC header. getAsyncStackTrace: unwrap InternalFieldTuple in reaction context WebKit#347 covers this.node:vmwithmicrotaskMode: "afterEvaluate"), the recoveredat asyncframes land after those outer frames instead of directly after JSC's async block. The frames are still present (they were absent before).Tests
test/regression/issue/24003.test.tscoversals.run(),als.enterWith(),Promise.raceunder ALS, the no-ALS baselines (no duplication, direct and viaPromise.race), and a mid-chain ALS entry where JSC's walk finds one frame and the hook supplies the rest.Existing
test/js/bun/test/stack.test.ts,test/js/node/v8/capture-stack-trace.test.js, andtest/js/node/async_hooks/AsyncLocalStorage.test.tsall pass unchanged.Related
Supersedes #29619.
[review] gate passed · iteration 2 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 2
evidence per changed file