bun:test: defer the AsyncContextFrame wrap past .length/.bind() for tests and hooks - #35334
bun:test: defer the AsyncContextFrame wrap past .length/.bind() for tests and hooks#35334robobun wants to merge 3 commits into
Conversation
parse_arguments() wraps a test/hook callback in an AsyncContextFrame when an AsyncLocalStorage context is active, so the context is restored when the callback runs. The callers then read .length from the returned callback to decide whether to pass a done() parameter. AsyncContextFrame has no .length property, so getLengthIfPropertyExistsInternal falls through to infinity, which clamps to I52_MAX, and every zero-arg callback registered inside an ALS context waits for a done() that is never called. On Linux the hook times out after 5 seconds and the test file fails with 'hook timed out before its done callback was called' pointing at a callback that has no parameter. On Windows prior to #34478 it hung forever. Read .length from the user's function inside parse_arguments(), before wrapping, and return it alongside the (possibly wrapped) callback.
WalkthroughChangesAsyncLocalStorage callback handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:53 PM PT - Jul 23rd, 2026
❌ @robobun, your commit ea0889b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35334That installs a local version of the PR into your bun-35334 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Move the with_async_context_if_needed() call out of parse_arguments() and into the two storage points (enqueue_describe_or_test_callback and generic_hook_impl), after the caller has read .length and, for test.each, bound the per-row arguments. An AsyncContextFrame has no .length and is not callable via getCallData, so wrapping first made every zero-arg callback inside an ALS context wait for done() and made test.each throw 'bind() called on non-callable'. The wrap still runs synchronously inside the user's als.run() body, so it captures the same context.
robobun
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/js/bun/test/als-hook-arity.fixture.ts`:
- Around line 15-55: Add assertions that als.getStore()?.tag equals "collection"
inside at least one lifecycle hook and the nested describe callback in the
fixture. Keep the existing order checks and test callback assertions unchanged,
covering both hook context restoration and deferred nested-describe context
capture.
- Around line 4-8: Remove the implementation-history comment from
test/js/bun/test/als-hook-arity.fixture.ts lines 4-8 and
test/js/bun/test/als-hook-arity.test.ts lines 5-10, or replace each with the
relevant issue URL if available. Keep regression-test comments limited to
exactly the issue URL.
🪄 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: ad93a624-8f83-48e0-883e-12b629a2febb
📒 Files selected for processing (4)
src/runtime/test_runner/ScopeFunctions.rssrc/runtime/test_runner/bun_test.rstest/js/bun/test/als-hook-arity.fixture.tstest/js/bun/test/als-hook-arity.test.ts
The fix moved the AsyncContextFrame wrap to the storage points; record als.getStore() in every callback (hooks, tests, test.each, nested describe body) so the order check also proves the captured context is restored on each path.
There was a problem hiding this comment.
LGTM — the wrap point moves from parse_arguments() to the two storage sites (enqueue_describe_or_test_callback, generic_hook_impl), both still synchronous inside the user's als.run() body, so the captured frame is unchanged while .length/.bind() now see the bare function.
What was reviewed:
- Confirmed
parse_arguments()has exactly two callers and both now wrap at the storage point; no path stores an unwrapped callback. test.eachnow binds the bare function, then wraps the resultingJSBoundFunction—bind() called on non-callableis fixed and covered by the fixture.- Fixture asserts
:ctxon every hook/test/describe entry, so a dropped wrap on any path surfaces as:nonein thetoEqualdiff. - Earlier findings (vacuous stdout assertion,
.eachsibling site) were addressed in 8b4e33e / ea0889b.
Extended reasoning...
Overview
This PR fixes a bug where registering a zero-arg test() or hook inside an active AsyncLocalStorage context caused bun:test to wait for a nonexistent done() callback (timing out), and test.each() to throw bind() called on non-callable. The root cause: parse_arguments() wrapped the callback in an AsyncContextFrame before the caller read .length (for done-param detection) or called .bind() (for test.each) — and AsyncContextFrame has no .length and is not callable via JSC::getCallData.
The fix moves the with_async_context_if_needed() call from parse_arguments() to the two storage points (enqueue_describe_or_test_callback() in ScopeFunctions.rs, generic_hook_impl() in bun_test.rs), both of which run synchronously in the same call stack, so the captured ALS frame is identical.
Security risks
None. This is test-runner-internal control flow with no untrusted-input parsing, no auth/crypto, no filesystem/network surface.
Level of scrutiny
Moderate. It touches the test runner's callback registration path, which affects every test()/describe()/hook call. However the change is mechanical (moving one call from one synchronous point to two later synchronous points in the same call stack), the invariant it preserves is clearly documented in the doc comment on ParseArgumentsResult.callback, and I verified via grep that parse_arguments() has exactly the two callers that now wrap. The fixture exercises every affected entry point (all four hook types, zero-arg test, done-arg test, test.each, nested describe) and asserts both correct arity detection and ALS context restoration at every one.
Other factors
I previously left two inline findings on earlier revisions of this PR: a vacuous stdout assertion (fixed in 8b4e33e by asserting toStartWith("bun test ")) and the .each sibling-site bug (fixed in 8b4e33e by moving the wrap to storage points so .bind() sees the bare function; test.each case added to fixture). CodeRabbit's request to assert ALS restoration in hooks and nested describe was addressed in ea0889b (every entry now records als.getStore()?.tag and the final toEqual asserts :ctx on all of them). All review threads are resolved. The bug hunting system found no issues on the current revision.
|
CI on ea0889b: the new test ( |
|
Cross-reference: #38910 fixes |
Same change as #35334, carried here because get_length returning 0 for the length-less wrapper depends on it: tests and hooks registered while an AsyncLocalStorage context is active (node:test registers everything inside a describe() body that way) had their .length read from the wrapper, and only the 2^51 - 1 that get_length returned for it made their done callbacks work. Read .length and .bind() from the function itself and wrap at the point where the callback is stored, which is still synchronous inside the caller's als.run() body.
Same change as #35334, carried here because get_length returning 0 for the length-less wrapper depends on it: tests and hooks registered while an AsyncLocalStorage context is active (node:test registers everything inside a describe() body that way) had their .length read from the wrapper, and only the 2^51 - 1 that get_length returned for it made their done callbacks work. Read .length and .bind() from the function itself and wrap at the point where the callback is stored, which is still synchronous inside the caller's als.run() body.
|
Closing in favor of #38910, which carries this change rebased onto current main as its second commit (5cea179). The #38910 also changes |
1 participant
What
Registering a zero-arg
test()or hook (beforeAll/afterAll/beforeEach/afterEach) while anAsyncLocalStoragecontext is active makes bun:test wait for adone()callback that was never declared, so the entry times out (or, on Windows before #34478, hangs the file).test.each()in the same context throwsTypeError: bind() called on non-callable:Why
parse_arguments()wrapped the callback withwith_async_context_if_needed()so the captured ALS context is restored when the callback runs. AnAsyncContextFramehas no.length(soJSC__JSValue__getLengthIfPropertyExistsInternalfalls through toinfinity, whichget_length()clamps to(1<<51)-1, andhas_done_parameteris always true) and is not itself callable viaJSC::getCallData(soBun__JSValue__bindrejects it).Fix
Move the
with_async_context_if_needed()call out ofparse_arguments()and into the two storage points:enqueue_describe_or_test_callback()(after.lengthis read and, fortest.each, after.bind()) andgeneric_hook_impl()(after.lengthis read). Both run synchronously inside the user'sals.run(...)body, so the captured context frame is the same oneparse_arguments()would have seen.Found while investigating a Windows-only hang in #34515's
node:testrun-child mode, wheredescribe()wrappers register the suite-completionafterAllfrom insideAsyncLocalStorage.run(); that PR's workaround of making the hook take an explicitdoneparameter is no longer needed with this fix.Verification
Passes on linux-x64 and windows-x64. Fails on the released bun with
hook timed out before its done callback was called(hooks/test) andbind() called on non-callable(test.each).[review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file