bun test: fail the test or hook whose done() received an error - #39112
bun test: fail the test or hook whose done() received an error#39112robobun wants to merge 4 commits into
Conversation
done(err) was reported through the generic uncaught-exception path, which attributes errors with get_current_state_data(). That cannot name a sequence inside a concurrent group and files hook errors under "between tests", so a concurrent test (or any test under --concurrent, including every node:test failure) and any hook calling done(err) printed "Unhandled error between tests" and was then reported as passing. Stamp each DoneCallback with the file and RefDataValue it was created for and report done(err) through on_uncaught_exception keyed by that value, the same way a throw or rejection from the callback is reported. A stale done(err) from an entry that already finished (timed out, or threw before the runner stamped its completion ref) fails the runner's validity check and is reported as an unhandled error instead of being charged to whatever entry is running. node:test's runtime t.skip()/t.todo() mark uses the same stamp, which replaces Execution.on_stack_entry_data.
|
Warning Review limit reached
Next review available in: 14 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 (8)
Comment |
|
Status: reproduced on the current release and on a debug build of main ( Related open PRs touching the same function: #33089 (hook |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #33089, though the two overlap. #33089 was opened for |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix is well-reasoned — stamping the RefDataValue on the DoneCallback at creation and routing done(err) through on_uncaught_exception with that key mirrors exactly how a throw/rejection from the same callback is already attributed. Because it changes pass/fail attribution in the test runner (serial hooks, concurrent groups, test.failing, JUnit, node:test) and shifts an observable timing edge (next test now starts on the next tick after done(err), hence the two dropped snapshot frames), a human look would still be worthwhile.
What was checked:
on_stack_entry_datahas no remaining references after removal;js_node_test_mark_resultnow reads the sameownerstamp instead.- New
DoneCallbackfields (Weak,RefDataValue) drop via the Box infinalize; onlyr#refneeds the explicit.deref(). - Stale
done(err)is rejected byget_current_and_valid_execution_sequenceand lands as "Unhandled error between tests" — covered by three explicit test cases. - The re-entrancy shape of
strong.get().on_uncaught_exception(...)matches the existingbun_test_then_or_catchpath; no new aliasing hazard.
Extended reasoning...
Overview
This PR fixes done(err) attribution in bun test: previously the error was routed through the generic VirtualMachine::uncaught_exception → get_current_state_data() path, which cannot name a sequence inside a concurrent group and demotes hook errors to RefDataValue::Start. The result was that done(err) in a concurrent test, or in any beforeEach/afterEach, printed as # Unhandled error between tests while the test itself was reported as (pass). The fix stamps the owning RefDataValue (and a Weak<BunTestCell>) on the DoneCallback at creation in run_test_callback, and bun_test_done_callback calls BunTest::on_uncaught_exception keyed by that stamp — the same path a throw or rejection from the callback already takes. js_node_test_mark_result reads the same stamp, which lets Execution.on_stack_entry_data be deleted. Four Rust files change (~60 lines net), plus three test files gaining ~200 lines of coverage.
Security risks
None. This is test-runner-internal error attribution; no untrusted input parsing, no auth/crypto/permissions.
Level of scrutiny
High. This is the test runner's pass/fail attribution logic — a bug here silently misreports test outcomes, which is exactly the failure mode the PR is fixing. It also touches concurrent execution (interleaved sequences, stale-completion rejection) and produces two observable behavior changes: (1) two stack frames disappear from an existing snapshot because the next test now starts on the next tick after done(err) rather than synchronously inside it, and (2) a late done(err) after its test finished is now reported as unhandled-between-tests instead of failing whichever test happens to be running. Both are improvements, but they're the kind of edge a maintainer should sign off on.
Other factors
The test coverage is thorough: one fixture asserted identical across serial / --concurrent / it.concurrent+describe.concurrent, covering sync/microtask/macrotask done(err), it.failing, and beforeEach/afterEach; three stale-done(err) cases; a JUnit test for concurrent done(err); and node:test's failing-hooks fixture under --concurrent. I verified on_stack_entry_data has no remaining references, that the new DoneCallback fields drop correctly via the Box in finalize (only r#ref needs the explicit .deref() because RefPtr has no Drop), and that the strong.get().on_uncaught_exception(...) re-entrancy shape matches the existing bun_test_then_or_catch path. No CODEOWNERS apply. Deferring because pass/fail attribution changes in the runner core warrant a maintainer's eyes even when the mechanism is sound.
|
Nothing to change from that review. The two behavior edges it points at (the next test starting on the following tick after done(err), and a stale done(err) being reported as an unhandled error instead of failing the test that happens to be running) are intentional and are the ones described in the Fix section; both are pinned by the snapshot update and the three stale-done cases in test-error-code-done-callback.test.ts. |
|
Updated 2:01 PM PT - Aug 15th, 2026
✅ @robobun, your commit 08773174961c97ea3d0802fb0ba920735ac83f4c passed in 🧪 To try this PR locally: bunx bun-pr 39112That installs a local version of the PR into your bun-39112 --bun |
…hooks Adds the beforeAll and afterAll hook kinds to the done(err) fixture (so all four hook kinds are pinned in the serial and both concurrent modes) and a node:test fixture whose suite-level before()/after() hooks throw, which node:test reports through those hooks' done callbacks. These are the cases #33089 was opened for; it is being closed in favor of this PR.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
#33089 has been closed in favor of this PR. Its cases were checked against this branch and the ones not already pinned here were added in 1768287: One case from the discussion on #33089 is worth carrying once #38876 lands: a late While adding the |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks error attribution in the test runner's done-callback path (unsafe raw-pointer state on DoneCallback, removal of on_stack_entry_data, and an intentional behavior change to when the next test starts after done(err)), a human look from someone familiar with the Execution/RefDataValue lifecycle would still be worthwhile.
Checked: the stamped owner: RefDataValue is only passed to on_uncaught_exception after buntest_weak.upgrade() succeeds, and get_current_and_valid_execution_sequence compares the stored entry pointer without dereferencing it, so a stale owner is rejected rather than UAF'd. called is now set via mem::replace before on_uncaught_exception runs user JS, so a re-entrant done() is a no-op (previously it could re-report). The removed on_stack_entry_data had js_node_test_mark_result as its only reader; that now reads the same stamp.
Extended reasoning...
Overview
This PR fixes done(err) attribution in bun test: previously a done(new Error(...)) in a concurrent group, or in any hook even serially, was routed through the generic VirtualMachine::uncaught_exception path and printed as # Unhandled error between tests while the test/hook itself was reported as passing. The fix stamps each DoneCallback at creation with a Weak<BunTest> and the RefDataValue that run_test_callback was invoked with, then bun_test_done_callback calls BunTest::on_uncaught_exception keyed by that value — the same path a throw or promise rejection from the same callback already takes. js_node_test_mark_result (node:test's runtime t.skip()/t.todo()) is rewired to read the same stamp, which lets Execution.on_stack_entry_data be deleted. Four Rust files change (~80 net lines) plus ~250 lines of new tests across four test files.
Security risks
None. This is test-runner-internal error routing; no untrusted input parsing, auth, crypto, or network surface is touched.
Level of scrutiny
Medium-high. The change is small and the mechanism is clearly the right layer (attribute the error to the entry the done was created for, exactly as throws/rejections already are), but it lives in the test runner's execution core where lifetime, aliasing, and re-entrancy invariants are hand-managed through raw pointers and UnsafeCell. It also carries two intentional behavior changes: the next test starts on the next tick after done(err) (matching done() without an error; two stack-frame lines drop from an existing snapshot as a result), and a stale done(err) — one whose entry has already completed — is now reported as an unhandled error rather than being pinned on whichever test happens to be running. Both are pinned by tests and match how late promise rejections are already handled, but a maintainer should confirm they agree with the semantics.
Other factors
- Test coverage is thorough: one fixture asserts identical per-test results serially, under
--concurrent, and withit.concurrent/describe.concurrentacross sync/microtask/macrotaskdone(err),done(),it.failing, and all four hook kinds; three stale-done(err)cases pin the "don't blame the running test" behavior; JUnit and node:test (--concurrentand suite-levelbefore()/after()) get their own tests. All are stated to fail on the release build and pass on the debug build. - Memory/aliasing: I verified the new fields don't introduce a UAF —
owneris only consumed afterWeak::upgradesucceeds (arena entries alive), and the validator compares the rawentrypointer without dereferencing it.called = trueis now set beforeon_uncaught_exceptioncan re-enter JS, closing a (pre-existing) window where a re-entrantdone()could double-report. Thestrong.get().on_uncaught_exception(...)pattern matches whatbun_test_then_or_catchalready does. - Dead-code removal:
on_stack_entry_datawas added specifically for the case this PR now covers via the stamp; its only reader is rewired here and the field/cell/scopeguard are removed together. - Open edge: the author notes a late
done(err)from a timed-out attempt of aretrytest still lands on the current attempt because attempts of the same sequence aren't yet distinguishable; that depends on #38876 and is out of scope here. - The comment-cop bot's long-comment flags have been addressed in the latest commits.
|
Nothing to change from this second review either. It covers the same two behavior edges as the first (next test starts on the following tick after done(err), stale done(err) reported as an unhandled error), both intentional and pinned by tests. The retry/timeout attempt edge it mentions is out of scope here and waits on #38876, as noted in the PR body. |
1 participant
Problem
done(new Error(...))is printed as(pass)when it runs in a concurrent group (it.concurrent,describe.concurrent, or any test underbun test --concurrent). The error is printed as# Unhandled error between testsinstead, and the summary reads e.g.4 pass, 0 fail, 4 errors. The exit code is still 1, but the JUnit report has no<failure>for the test, and a-trerun of the test looks green.beforeAll/beforeEach/afterEach/afterAllhook callingdone(err)behaves the same way even serially: the hook is not reported as failed, the dependent tests still run and pass (a failingbeforeEach/beforeAlldoes not even skip the bodies), the error is counted as unhandled. A hook that throws fails them.it.concurrent.failing("x", done => done(err))is reported as a failure (the error is not credited to the test, so it "passed").node:testreports every failure by calling bun:test's done callback, so underbun test --concurrentevery failing node:test test is reported as passing, and a suite-levelbefore()/after()hook that throws (reported through abeforeAll/afterAlldone callback) is reported that way even serially.bun_test_done_callback(src/runtime/test_runner/bun_test.rs) reports the error through the genericVirtualMachine::uncaught_exception, which attributes it withBunTest::get_current_state_data(). For a group with more than one sequence that returnsentry_data: None, and for a hookjest::on_unhandled_rejectiondemotes it toRefDataValue::Start; either wayExecution::handle_uncaught_exceptionreturnsShowUnhandledErrorBetweenTestsand never marks the sequence failed, so theadd_resulta few lines later completes it as a pass. Serially in a test body it worked only becauseget_current_state_data()happened to resolve to the same test.Fix
DoneCallbacknow records, at creation, the file (Weak) and theRefDataValuethatrun_test_callbackwas invoked with;done(err)callson_uncaught_exceptionkeyed by that value, which is exactly how a throw or rejection from the same callback is reported (run_test_callback/bun_test_then_or_catch). The result for a test body, a hook,test.failingand JUnit is therefore identical to throwing from that callback, in serial and concurrent groups alike.donecan fire: synchronously in the body or in its microtask drain (before the completion ref exists), later from a macrotask (ref stamped), or after the entry already finished. In the last caseget_current_and_valid_execution_sequencerejects the stale value and the error is reported asUnhandled error between tests, like a late rejection already is; previously such a latedone(err)failed (and completed) whichever test happened to be running.done(err)from a timed-out attempt is still charged to the attempt running at the time and triggers another retry, as it already is serially on main. In a concurrent group of several tests main happened to report it as an unhandled error (exit 1) and this PR retries there too (exit 0); with bun test: ignore completions from an earlier attempt of a retried test #38876 both are rejected as stale. Theretry: 2fixture posted on test runner: fail dependent tests when a hook's done() receives an error #33089 belongs to whichever of the two lands second.add_result/run_next_tickpath runs exactly once. A side effect is that the next test now starts on the next tick afterdone(err), as it already did afterdone(); before, the generic path advanced the runner synchronously inside thedone()call, which is why two stack frames from the previous test'sdone()call disappear from the existing snapshot intest-error-code-done-callback.test.ts.node:test's runtimet.skip()/t.todo()mark (js_node_test_mark_result) reads the same stamp, which replacesExecution.on_stack_entry_data(added for exactly this "done fired before the ref was stamped" case); its existing serial and--concurrenttests still pass.done(false)/done("")still count as errors, and a seconddone()call is still a no-op (bun:test: fail the test when done(err) is called after done() #34041 is about that; it touches the same lines and would report through the sameownerstamp). Anasynccallback that also declaresdonestill completes as soon as its promise fulfills (theFulfilledarm ofrun_test_callback), so adone(err)it fires later is one of the stale cases above: reported as an unhandled error instead of, as before, failing the next test. Whether such a callback should wait fordoneis a separate question. test runner: fail dependent tests when a hook's done() receives an error #33089 (closed in favor of this PR) attributeddone(err)through the ref as well but fell back toget_current_state_data()when the ref is not stamped yet, so the synchronous / microtask concurrent cases above (and node:test under--concurrent) stayed broken there; its hook-kind and node:test suite-hook cases are carried here.test/js/bun/test/test-error-code-done-callback.test.ts: one fixture (sync, microtask and macrotaskdone(err),done(),it.failing,done(err)from each ofbeforeAll/beforeEach/afterEach/afterAll) must produce the same per-test results serially, under--concurrent, and withit.concurrent/describe.concurrent; plus three stale-done(err)cases (timed out, body threw after scheduling it on a macrotask / microtask) that must not fail the test running at the time. All 7 fail on the current release (USE_SYSTEM_BUN=1), pass withbun bd test.test/js/junit-reporter/junit.test.js: a concurrent group'sdone(err)failures land in their own<testcase>(before:failures="0", no<failure>elements).test/js/node/test_runner/node-test.test.ts: the failing-hooks fixture reports10 failunder--concurrenttoo (before: all ten reported as passing, ten unhandled errors).test/js/node/test_runner/node-test.test.ts+fixtures/30-failing-suite-hooks.js: two suites whosebefore()/after()throw report1 pass, 2 failand the body under the failedbefore()does not run (before:2 pass, 0 fail, 2 errors, body ran).test/js/bun/test/that covers done callbacks, hooks, failing/retry/skip and concurrency,test/cli/test/bun-test.test.ts, the rest ofjunit.test.jsandnode-test.test.ts, and the vendoredtest/js/node/test/parallel/test-runner-*.jsfiles run the wayscripts/runner.node.mjsruns them.Background
ExecutionSequences (one test plus itsbeforeEach/afterEachentries), grouped intoConcurrentGroups. A serial test is a group of one sequence; consecutive concurrent tests share one group and run interleaved.RefDataValue: the runner's handle for "this completion belongs to group G, sequence S, entry E, repeat N".run_test_callbackis invoked with one; the promise.then/.catchand the done callback hand it back viaadd_result, andExecution::get_current_and_valid_execution_sequencerefuses it once the sequence has moved on to another entry or completed, which is what makes late completions harmless (telling a retried attempt from its predecessor is bun test: ignore completions from an earlier attempt of a retried test #38876).on_uncaught_exception(value, &RefDataValue)marks that sequence failed (or passed fortest.failing, skipping the rest of the sequence for a hook) and prints the error as that entry's failure, also recording it for JUnit. With a value it cannot resolve it printsUnhandled error between testsand bumps the error counter instead.get_current_state_data()is the fallback used for genuinely stray errors (an uncaught exception from a timer, an unhandled rejection): it can only point at the single sequence of a serial group, which is why it is the wrong tool for an error whose owner is known.r#refis a refcounted copy of the sameRefDataValue, but it is only created after the callback returns (so a promise-returning callback anddone()can agree on who completes the test); adone(err)called from the body, from the microtask drain, or after the body threw never sees it, hence the separate stamp taken at creation.[review] gate passed · iteration 0 · 8 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