bun:test: bound expect() promise waits by the test timeout - #36716
bun:test: bound expect() promise waits by the test timeout#36716robobun wants to merge 6 commits into
Conversation
expect(promise).resolves/.rejects, expect(fn).toThrow on an async fn, and async custom matchers spin the event loop via wait_for_promise until the subject promise settles. When the promise can only settle after the matcher returns (e.g. expect(p).resolves.toBe(x); resolve(x);) the spin never ends. The per-test timeout timer fires inside that spin (via auto_tick -> drain_timers -> bun_test_timeout_callback), but since e01f454 it no longer calls requestTermination(), so nothing breaks the loop. The runner records the timeout, returns immediately because it is re-entrant, and the spin continues at 100% CPU forever. Replace the three expect.rs wait_for_promise calls with a bounded loop that also breaks once the enclosing test's deadline (read via TestRunner::get_active_timeout(), the same source spawnSync already uses) passes, and throw a matcher error naming the still-pending promise instead of hitting unreachable!(). Outside a timed test the deadline is EPOCH and the loop behaves exactly like wait_for_promise. Fixes #14950
|
Status (0ebfa04): reproduced on canary (child Local verification: CI: build #87458 finished 195 pass / 1 fail. Note: this is the minimal "timeout must still fire" fix. Full Jest parity (matcher returns a |
|
Warning Review limit reached
Next review available in: 11 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 (3)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/test_runner/expect.rs:941-948— Thestill_pendingearly-return inget_value_as_to_throwbypassespromise.unwrap(..., MarkHandled), and — unlike the two sibling sites (process_promiseL527,execute_custom_matcherL1539) — there is nopromise.set_handled(vm)before the bounded wait here. If the abandoned async fn later rejects (e.g.expect(async () => { await Bun.sleep(200); throw ... }).toThrow()with a 100ms timeout), the rejection surfaces as an unhandled rejection attributed to whichever test is running when it fires. One-line fix: addpromise.set_handled(global_this.vm())before the bounded wait, matching the siblings.Extended reasoning...
What the bug is
This PR replaces three
wait_for_promisecall sites inexpect.rswithwait_for_promise_bounded_by_test, each followed by an early return when the promise is still pending at the test deadline. Two of those sites —process_promise(~L527) andexecute_custom_matcher(~L1539) — already had a pre-existingpromise.set_handled(vm)call before the wait, so on their new early-return path the promise is already marked handled. The third site,get_value_as_to_throw, has no such call: it previously relied onpromise.unwrap(global_this.vm(), UnwrapMode::MarkHandled)at L949 to set the handled flag, and that line is now skipped by the newif still_pending { return Err(...) }at L944-948.Code path
if let Some(promise) = return_value.as_any_promise() { let still_pending = Self::wait_for_promise_bounded_by_test(global_this, promise); scope.apply(vm); // ← restores the runner's real on_unhandled_rejection if still_pending { return Err(global_this.throw(...)); // ← promise never marked handled } match promise.unwrap(global_this.vm(), js_promise::UnwrapMode::MarkHandled) { // ← skipped
scope.apply(vm)on L943 restores the runner's realon_unhandled_rejectionhandler (theUnhandledRejectionScopeat the top of this function had swapped inon_quiet_unhandled_rejection_handler_capture_value). So once this function returns on the timeout path, any later rejection ofpromiseroutes throughjest::on_unhandled_rejection, which attributes it to the currently-active test.Why existing code doesn't prevent it
JSC's
rejectPromisechecksisHandledFlagat rejection time before callinghostPromiseRejectionTracker.JSC__JSPromise__setHandled→markAsHandled()sets that flag, so calling it on a still-pending promise (as the two sibling sites do) suppresses the future report. Here the flag is never set on the timeout path, so nothing suppresses it.Step-by-step proof
test("a", () => { expect(async () => { await Bun.sleep(200); throw new Error("late"); }).toThrow(); }, 100); test("b", async () => { await Bun.sleep(300); expect(1).toBe(1); });
- Test a starts. The async fn is called and returns a pending promise;
Bun.sleep(200)is queued. wait_for_promise_bounded_by_testspins until the 100ms deadline, then returnsstill_pending = true.scope.apply(vm)restores the runner's real unhandled-rejection handler.- L944-948 throws the matcher error and returns. The promise is still pending,
isHandledFlagis unset. Test a is reported as timed out — expected. - Test b starts and awaits
Bun.sleep(300). At t≈200ms, test a'sBun.sleep(200)fires, the async fn body throws, and its promise rejects. isHandledFlagis unset → JSC calls the host rejection tracker →jest::on_unhandled_rejection→unhandled_error_counterbumped on the currently-active test, which is b.- Test b is reported as failed even though its own assertion passes.
Note that the 23865 fixture (
await Bun.sleep(100); throw ...with a 50ms timeout) is exactly this shape — it happens to only contain one test, but any user file with a follow-up test would hit this. Pre-PR,wait_for_promisespun until the promise settled at 200ms andunwrap(MarkHandled)always ran, so test b passed cleanly; this is a new regression path introduced by the PR that directly undermines its own "a test after one that spins ... still runs" goal.Why the new tests don't catch it
The "a test after one that spins on .resolves still runs" case in
14950.test.tsgoes throughprocess_promise, which does callset_handled. ThetoThrowcase uses() => new Promise(() => {}), which never rejects.Fix
Match the two siblings — add before L942 (or inside the
still_pendingbranch):promise.set_handled(global_this.vm());
This falls squarely under REVIEW.md's "fix the whole class in the same PR — grep for every sibling site sharing the pattern" rule.
- Test a starts. The async fn is called and returns a pending promise;
The still_pending early return skips unwrap(MarkHandled), so a promise abandoned at the test deadline that later rejects would surface as an unhandled rejection attributed to whichever test is running then. Mark it handled up front like the process_promise and execute_custom_matcher siblings already do, and add a regression case.
There was a problem hiding this comment.
Thanks — c14e9e3 addresses the earlier finding: the toThrow path now calls promise.set_handled() before the bounded wait, and the new "late rejection from an abandoned toThrow" case in 14950.test.ts covers exactly that scenario. No further issues found this pass. Deferring to a maintainer given this reworks how expect() spins the event loop against the test-timeout timer, and there's an open design question vs. #33289's promise-reaction approach.
What was reviewed:
wait_for_promise_bounded_by_testmatchesEventLoop::wait_for_promisestructure (tick → auto_tick,execution_forbidden()guard) plus the deadline check;EPOCH→ unbounded, so behavior outside a timed test is unchanged.- All three call sites now
set_handledbefore the wait; thesilentbranch's bareErr(JsError::Thrown)matches the existing sibling arms inprocess_promise. get_active_timeout()is the same sourcespawnSyncuses; returnsEPOCHwhen no active file/entry.
Extended reasoning...
Overview
Replaces three unbounded wait_for_promise calls in src/runtime/test_runner/expect.rs (.resolves/.rejects, toThrow on an async fn, and async expect.extend matchers) with a new wait_for_promise_bounded_by_test helper that also breaks when the enclosing test's deadline (via TestRunner::get_active_timeout()) passes, then throws a matcher error naming the still-pending promise. Adds test/regression/issue/14950.test.ts (7 spawned-child cases with a 20s watchdog) and updates the 23865.test.ts snapshot for the new error line.
Prior finding addressed
My earlier inline comment flagged that the toThrow path's new still_pending early return bypassed promise.unwrap(..., MarkHandled) without a prior set_handled, so a late rejection could surface as an unhandled rejection during the next test. Commit c14e9e3 adds promise.set_handled(global_this.vm()) before the bounded wait at that site (matching the two siblings) and adds a regression case ("a late rejection from an abandoned toThrow async fn does not fail the next test") that asserts 1 pass / 1 fail and not.toContain("Unhandled"). The comment thread is resolved.
Security risks
None. Test-runner-internal control flow; no untrusted-input parsing, auth, crypto, or network surface.
Level of scrutiny
High. This is a hand-rolled event-loop spin that interacts with the test-runner's timeout timer (which fires inside the spin via auto_tick), execution_forbidden(), and the in_run_loop reentrancy guard the PR description mentions. The loop structure mirrors EventLoop::wait_for_promise exactly plus a wall-clock deadline check, and get_active_timeout() is already the deadline source spawnSync uses — so the pattern has precedent. But the interaction between the timeout callback re-entering BunTest::run() mid-spin and this loop then breaking on the deadline is subtle enough that a maintainer familiar with the #24355 history should confirm.
Other factors
- Design overlap with #33289: that PR takes a different approach (promise reactions instead of a bounded spin) to the same #14950 hang. The author explicitly scopes this as a minimal safety-net fix that doesn't overlap #33289's files, but which approach lands is a maintainer call.
- Tests: gate evidence shows the new tests fail on main (children hung, killed by watchdog) and pass with the fix on both debug+ASAN and release. Existing
expect.test.js(415),expect-extend.test.js(28),jest-extended.test.js(58) reported unchanged. - User-facing error messages: three new error strings are introduced — worth a quick maintainer eye for wording consistency with other matcher errors.
There was a problem hiding this comment.
Thanks — c14e9e3 addresses the earlier finding (the toThrow path now calls set_handled before the bounded wait, and the new "late rejection" case in 14950.test.ts covers it). I didn't find further issues this pass, but this reworks how expect() spins the event loop and overlaps in intent with #33289's reaction-based approach, so it's worth a maintainer look before landing.
What was reviewed:
wait_for_promise_bounded_by_testmirrorsevent_loop.rs::wait_for_promise(early-exit,execution_forbidden,tick/auto_tick) plus the sameget_active_timeout()deadline patternspawnSyncalready uses.- All three modified sites now
set_handledbefore the wait; thestill_pendingbranches throw instead of reaching the pendingunwraparm. - The
deadline == EPOCHcase (no runner / no per-test timeout) preserves the old unbounded behavior.
Extended reasoning...
Overview
The PR replaces three unbounded wait_for_promise calls in src/runtime/test_runner/expect.rs (.resolves/.rejects, toThrow on an async fn, and async expect.extend matchers) with a new wait_for_promise_bounded_by_test helper that additionally breaks out when the enclosing test's deadline (TestRunner::get_active_timeout()) has passed, and throws a descriptive matcher error on the pending arm instead of hanging. New regression tests in test/regression/issue/14950.test.ts cover all three sites plus a subsequent-test-still-runs case, a late-rejection-does-not-leak case, and an already-settled control; 23865.test.ts's snapshot is updated to include the new error line.
Prior review resolved
My previous inline comment flagged that the toThrow site's early return bypassed unwrap(..., MarkHandled) without a prior set_handled, so a promise that rejects after the deadline would surface as an unhandled rejection during the next test. Commit c14e9e3 adds promise.set_handled(global_this.vm()) before the bounded wait at that site (matching the two siblings), and adds the "a late rejection from an abandoned toThrow async fn does not fail the next test" case asserting stderr does not contain "Unhandled" and that test "b" still passes. That fully addresses the finding.
Security risks
None. This is test-runner-internal control flow; no untrusted-input parsing, auth, crypto, or network surface is touched.
Level of scrutiny
Moderate-to-high. The diff is small (~60 lines of Rust), but it changes how expect() drives the event loop under bun test — a hot, subtle path where prior changes (#24355 removing requestTermination()) have caused regressions in both directions. The new loop is a faithful copy of event_loop.rs::wait_for_promise with one extra deadline comparison, and the deadline source is the same one spawnSync already consumes, so the mechanism is precedented. Still, this is not a mechanical/config change.
Other factors
- Competing design: #33289 fixes the same issue by making the matcher return a promise (Jest parity) rather than bounding the spin. The PR description explicitly positions this as a non-overlapping safety-net stopgap, but which approach lands (or whether both do) is a maintainer call.
- Test quality: Tests use
describe.concurrentwith subprocess spawns, a 20s watchdog well under the 60s per-test timeout, and assert on specific error text plushung === false. The late-rejection test relies on wall-clock ordering (700ms reject inside a 900ms sleep after a 500ms timeout) which is generous enough not to worry me, though under extreme ASAN slowdown the child's inner 5s timeout on test "b" is the tightest bound. - Behavior outside a timed context: When
Jest::runner()isNoneor the active timeout is unset,deadline == EPOCHand the loop degenerates to the original unboundedwait_for_promise, so non-bun testcallers ofexpect()(if any) are unaffected.
Given the event-loop sensitivity and the open design question vs. #33289, I'm deferring rather than approving.
|
Hit this again on current main (b5afcac, also on 1.4.0-canary da3851e) while working through the test suite for #37993, with a second shape that is worth adding to import { expect, test } from "bun:test";
test("rejects matcher called after an await", async () => {
await Bun.sleep(1);
await expect(new Promise(() => {})).rejects.toThrow();
}, 500);
test("runs after", () => {
expect(1).toBe(1);
});The sync-body shape from the PR description (matcher called before the first Rebase note for the current conflict in |
2 participants
What does this PR do?
expect(promise).resolves.<matcher>()(and.rejects,expect(asyncFn).toThrow(), and asyncexpect.extendmatchers) spin the event loop viawait_for_promiseuntil the subject promise settles. When the promise can only settle after the matcher returns, the spin never ends:On current canary this hangs
bun testat 100% CPU forever. In 1.1.34 the per-test timeout would fire after 5s becausehandle_timeoutcalledrequestTermination(); e01f454 (#24355) removed that call to fix a crash where the termination exception escaped throughautoTick, so nothing breaks thewait_for_promiseloop anymore. The timer still fires inside the spin (viaauto_tick -> drain_timers -> bun_test_timeout_callback), the runner records the timeout and tries to re-enterBunTest::run(), which returns immediately becausein_run_loopis already set, and the spin continues.This PR replaces the three
wait_for_promisecall sites inexpect.rswith a bounded loop that also breaks once the enclosing test's deadline passes (read viaTestRunner::get_active_timeout(), the same sourcespawnSyncalready uses for its own wait loop), and throws a matcher error naming the still-pending promise instead of hittingunreachable!()on the pending arm:Outside a timed test the deadline is
EPOCHand the loop behaves exactly likewait_for_promise.requestTermination()stays removed, so #23865 stays fixed; its regression test snapshot is updated because the output now includes the new matcher error before the existingtimed out afterline.This is a safety-net fix only (the timeout must fire; the runner must not spin forever). It does not change the matcher to return a
Promiseor give Jest parity for the un-awaited case; that is #33289, which this PR intentionally does not overlap with (noevent_loop.rs/Execution.rschanges, no matcher signature changes).Fixes #14950
How did you verify your code works?
New
test/regression/issue/14950.test.tsspawns six childbun testruns with--timeout 500and a 20s watchdog:.resolveson a post-call-resolved promise,.rejectson a never-settling promise,toThrowon an async fn returning one, an async custom matcher returning one, a second test after a hanging one, and an already-settled control. On an unpatched build the five hanging children are killed by the watchdog (hung: true); with the fix all six children exit on their own within ~1s and report the new matcher error plustimed out after Nms.Existing suites pass unchanged:
expect.test.js(415 pass),expect-extend.test.js(28 pass),jest-extended.test.js(58 pass),expect-assertions.test.ts(1 pass),test-test.test.ts(24 pass),23865.test.ts(snapshot updated, 1 pass).[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file