bun:test: don't block on pending promises in expect().resolves/.rejects - #30595
bun:test: don't block on pending promises in expect().resolves/.rejects#30595robobun wants to merge 26 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR implements async deferral for ChangesAsync Promise Deferral for
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 9:12 PM PT - Jun 18th, 2026
❌ @robobun, your commit c276620 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 30595That installs a local version of the PR into your bun-30595 --bun |
|
Found 6 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Status (c276620): rebased onto main dae2e87; review threads all resolved; the diff is green. Single red lane in build 63427 is a known darwin-only socket race unrelated to this PR. Needs a maintainer to merge. CI triage for build 63427 (main 63422 on the same base is green):
Rebase: one trivial conflict in Verified on this tip: Open follow-up (deferred, non-blocking per review): deferred-rerun What this PR does (fixes #14950, #25181): When Tests: find-issues bot cross-check:
|
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 `@src/test_runner/expect.zig`:
- Around line 288-322: In settle, replace the early "catch return" on calls that
can fail (the getLength call and the args_array.getIndex loop) with logic that
captures the exception via globalThis.tryTakeException() (falling back to
JSValue.js_undefined if none), calls this.deferred.reject(globalThis, exception)
catch {}, and then returns; this mirrors the matcher_fn.call error path and
ensures the deferred promise is always settled on error (refer to settle,
args_array.getLength, args_array.getIndex, and this.deferred.reject).
In `@test/js/bun/test/expect-resolves-pending.test.ts`:
- Around line 263-265: The test currently converts the regex result directly to
Number which yields NaN if the match fails; first capture the match into a
variable (e.g., const m = out.match(/MAX_INFLIGHT=(\d+)/)), assert the match
exists (use expect(m).toBeTruthy() or explicitly fail with a descriptive
message), then parse Number(m[1]) into maxInFlight and assert it equals 10; this
ensures a clear diagnostic when the MAX_INFLIGHT line is missing and uses the
existing identifiers maxInFlight and out.match for locating the change.
🪄 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: 27bba966-d474-408a-bbdb-7e4010052f44
📒 Files selected for processing (2)
src/test_runner/expect.zigtest/js/bun/test/expect-resolves-pending.test.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/test_runner/expect.zig:936-939—inlineSnapshot()usesasync_rerun_srclocwhenever it's non-null, but the field is never cleared after a deferred re-run completes —rerun()'sdeferonly resetsis_async_rerun, andmaybeDeferMatcher()early-returns at thepromise.status() != .pendingcheck (L216) before updating it. So if the sameExpectinstance is reused —const e = expect(pending).resolves; await e.toBe(1); await e.toMatchInlineSnapshot();— the second call takes the synchronous path with the first call's stale srcloc and writes the snapshot to the wrong source line. This is the same per-instance-state-not-reset class as the opencounted_expect_callcomment above, just a different field; gating this branch onis_async_rerun(or clearingasync_rerun_srclocinrerun()'sdeferalongsideis_async_rerun = false) fixes it consistently with that one.Extended reasoning...
What the bug is
inlineSnapshot()(expect.zig:936) selects the source location for the snapshot write with:const srcloc, const owns_srcloc = if (this.async_rerun_srcloc) |s| .{ s, false } else .{ callFrame.getCallerSrcLoc(globalThis), true };
This branches on whether
async_rerun_srclocis non-null, not on whether the current invocation is actually a deferred re-run (is_async_rerun). The field is populated bymaybeDeferMatcher()at L224-225 when it decides to defer, but is only cleared infinalize()on destruction (L502).rerun()'sdeferblock (L321-323) resetsis_async_rerunbut leavesasync_rerun_srclocuntouched, and a grep confirms there are exactly four references to the field: declaration, set-on-defer, deref-on-finalize, and this read.The code path that triggers it
.resolves/.rejects/.notare getters that mutatethis.flagsin place andreturn thisValue— they don't create a newExpect. So:let resolve; const p = new Promise(r => (resolve = r)); const e = expect(p).resolves; // one Expect instance await e.toBe(1); // line 10 await e.toMatchInlineSnapshot(); // line 11
both matcher calls share the same
*Expect.Why nothing prevents it
maybeDeferMatcher()has four early-return guards in order:flags.promise == .none,is_async_rerun,asAnyPromise() == null, andpromise.status() != .pending(L213-216). The srcloc capture at L224-225 sits after all four. On the second matcher call the promise is now settled, so the function returnsnullat L216 without touchingasync_rerun_srcloc, leaving the value captured by the first call in place. The matcher then proceeds synchronously intoinlineSnapshot()withis_async_rerun == falsebutasync_rerun_srcloc != null, and the L936 check picks the stale location.Step-by-step proof
e.toBe(1)(line 10):getValue()→maybeDeferMatcher().flags.promise == .resolves,is_async_rerun == false, value is a pending promise → passes all guards. L225 setsasync_rerun_srcloc = {file, line: 10, …}.PendingMatcheris created;toBereturns the deferred promise.resolve(1)(microtask) →onResolve→settle()→rerun(): setsis_async_rerun = true, re-invokestoBe, thendeferresetsis_async_rerun = false.async_rerun_srclocis still{…, line: 10}. Theawaiton line 10 resumes.e.toMatchInlineSnapshot()(line 11):getValue()→maybeDeferMatcher().flags.promise == .resolves,is_async_rerun == false, value is a promise, butpromise.status() == .fulfilled→ returnsnullat L216, skipping L224-225.processPromise()extracts1synchronously;getValue()returns it.inlineSnapshot()runs on the write path (no existing snapshot string). At L936,this.async_rerun_srclocis non-null →srcloc = {…, line: 10}. The snapshot writer records line 10 as the insertion point and writestoMatchInlineSnapshot(`1`)over thetoBe(1)call on line 10 instead of into the emptytoMatchInlineSnapshot()on line 11 — corrupting the source file.
Pre-PR there was no
async_rerun_srcloc;inlineSnapshot()always calledcallFrame.getCallerSrcLoc(), which on step 3 would have correctly returned line 11.Impact
The trigger requires reusing a saved
expect().resolvesinstance across a deferred non-snapshot matcher followed by an inline-snapshot write — uncommon, but valid (Jest supports it and pre-PR Bun did too viawaitForPromise). The consequence, however, is destructive: the snapshot updater silently writes to the wrong line of the user's test file. This is also the exact same per-instance-state-leak class as the still-opencounted_expect_callcomment (3230849139), just a different field, so it's worth fixing both consistently in the same pass.Suggested fix
Either is sufficient and both compose with the
counted_expect_callfix:- Gate the read on the re-run flag: change L936 to
if (this.is_async_rerun) this.async_rerun_srcloc else null(or equivalent) so the stored location is only used when we're actually insidererun(). - Clear in
rerun()'sdefer: addif (expect.async_rerun_srcloc) |*s| { s.str.deref(); expect.async_rerun_srcloc = null; }next toexpect.is_async_rerun = false;at L321-323, mirroring whatever reset is chosen forcounted_expect_call.
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 `@src/test_runner/expect.zig`:
- Around line 226-229: The shared Expect field async_rerun_srcloc is being
overwritten when multiple pending matchers are created, causing later reruns to
use the wrong callsite; instead capture and store the caller src location per
PendingMatcher instance and restore it only when that PendingMatcher is actually
rerun. Update PendingMatcher.create (and the PendingMatcher struct) to accept
and retain the value returned by callframe.getCallerSrcLoc(globalThis) (instead
of writing to Expect.async_rerun_srcloc), and change the rerun path that
currently reads Expect.async_rerun_srcloc to read the stored src-loc from the
PendingMatcher being run; apply the same change for the other occurrences
referenced (lines around 334-340 and 957-960) so each deferred
.resolves/.rejects keeps its own src location.
In `@test/js/bun/test/expect-resolves-pending.test.ts`:
- Around line 15-34: The helper runFixture currently adds an internal 20s
watchdog (Promise.race with Bun.sleep + proc.kill("SIGKILL")) which duplicates
bun:test's timeout and forces callers to bump test timeouts; remove the
Promise.race/Bun.sleep/kill block and the timedOut logic so runFixture simply
awaits proc.exited and then reads proc.stdout/proc.stderr, relying on bun:test's
outer timeout/cleanup; apply the same removal for the other similar helper
blocks referenced (lines 39-118, 120-156, 161-220, 226-245, 251-286) to
eliminate fixture-level watchdogs.
🪄 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: 78bf6852-0130-4312-b370-bf3f992e12a3
📒 Files selected for processing (7)
src/test_runner/expect.zigtest/cli/install/migration/yarn-lock-migration.test.tstest/cli/install/minimum-release-age.test.tstest/js/bun/test/expect-resolves-pending.test.tstest/js/node/fs/glob.test.tstest/js/web/fetch/client-fetch.test.tstest/js/web/fetch/wasm-streaming.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/js/bun/test/expect-resolves-pending.test.ts (1)
15-34: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winLet bun:test own the hang budget.
The
Promise.race(...)watchdog on Line 26 duplicates bun:test's timeout/cleanup for anawait using proc = Bun.spawn(...)subprocess and is what forces the40_000overrides on Lines 118, 156, 220, 258, and 299. I’d drop the helper-level timeout/kill path and the per-test timeout overrides together so the outer test timeout remains the single source of truth.Based on learnings, in
test/**/*.test.tsfiles that usetempDirandawait using proc = Bun.spawn(...), avoid fixture-level watchdogs and rely on Bun’s outer timeout; fortest/js/bun/**, do not add explicit per-test timeouts unless they are already established as required by the file.Also applies to: 118-118, 156-156, 220-220, 258-258, 299-299
🤖 Prompt for 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. In `@test/js/bun/test/expect-resolves-pending.test.ts` around lines 15 - 34, The helper runFixture currently implements its own watchdog (the Promise.race + timedOut check, proc.kill("SIGKILL") and subsequent await proc.exited) which duplicates bun:test's timeout/cleanup; remove that watchdog logic (the Promise.race call, the timedOut boolean, the kill path and its conditional await) so runFixture simply spawns the subprocess with await using proc = Bun.spawn(...) and then reads proc.stdout.text() and proc.stderr.text(); also remove the corresponding per-test explicit timeout overrides that were added to accommodate this helper-level kill behavior so the file relies solely on bun:test's outer timeout mechanism.
🤖 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.
Duplicate comments:
In `@test/js/bun/test/expect-resolves-pending.test.ts`:
- Around line 15-34: The helper runFixture currently implements its own watchdog
(the Promise.race + timedOut check, proc.kill("SIGKILL") and subsequent await
proc.exited) which duplicates bun:test's timeout/cleanup; remove that watchdog
logic (the Promise.race call, the timedOut boolean, the kill path and its
conditional await) so runFixture simply spawns the subprocess with await using
proc = Bun.spawn(...) and then reads proc.stdout.text() and proc.stderr.text();
also remove the corresponding per-test explicit timeout overrides that were
added to accommodate this helper-level kill behavior so the file relies solely
on bun:test's outer timeout mechanism.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d265888-218e-4586-b25a-89e1c64e1c7a
📒 Files selected for processing (2)
src/test_runner/expect.zigtest/js/bun/test/expect-resolves-pending.test.ts
There was a problem hiding this comment.
No outstanding issues from my side after several review rounds, but this is a user-visible behavior change to core bun:test semantics (every .resolves/.rejects matcher now defers instead of blocking, with knock-on effects across ~26 test files that previously relied on the blocking behavior) — worth a human look before merging.
Extended reasoning...
Overview
This PR replaces the synchronous vm.waitForPromise() spin in expect().resolves/.rejects with a deferred-rerun mechanism: when the input promise is still pending, the matcher captures its callee/args/srcloc/flags into a heap-allocated PendingMatcher, attaches a .then() to the input promise, and returns a new promise to the caller. When the input settles, the matcher is re-invoked and the returned promise is resolved/rejected with the outcome. Two new promise-handler IDs are registered in ZigGlobalObject. The getValue() signature changes to return ?JSValue, and ~70 matcher files in src/test_runner/expect/ are updated mechanically with orelse return this.deferredResult(thisValue). About 26 test files are swept to add await to assertions that previously relied on the blocking behavior. A new 322-line subprocess test file pins the fix.
Security risks
None identified. The change is confined to the test runner; no auth, crypto, network, filesystem, or user-input parsing surfaces are touched. The new promise handlers only re-invoke an already-registered matcher with already-captured arguments.
Level of scrutiny
High. This is a behavior change visible to every bun test user: .resolves/.rejects matchers on pending promises now return a thenable instead of blocking, which means un-awaited assertions that previously "worked" (by blocking) now silently pass or surface as later unhandled rejections — exactly what required the ~70-site test-suite sweep in this PR. The core expect.zig changes involve subtle shared-state management on the Expect instance (is_async_rerun, counted_expect_call, async_rerun_srcloc, flags) across the defer/rerun pair, with save/restore semantics that took several review rounds to get right for instance-reuse and nested-rerun cases. The inline-snapshot path needed special handling to capture the call-site source location before the user's frame leaves the stack.
Other factors
- Iterative review history: I flagged ~12 issues across multiple rounds (assertion double-counting, inline-snapshot srcloc on rerun,
counted_expect_callleaking across reused instances,flags/srcloc/is_async_rerunnot snapshotted per-PendingMatcher,applyCustomMatchernot callingpostMatch, ~70 missed un-awaited test sites that caused real CI failures including a 14-platform deterministic failure indeinitialization.test.ts). All were fixed and all threads are resolved. The number and depth of issues found is itself a signal that this change is non-trivial. - Acknowledged follow-up deferred: error stack traces for failed deferred matchers no longer include the user's call-site frame (the author agreed this is valid but is leaving it for a follow-up PR since the
awaitsite still gives a usable line number in the common case). - CI: green on the latest build (53965); the 3 remaining failures are confirmed pre-existing main flakes per robobun.
- Test coverage: comprehensive new subprocess-based test file covering hang regression, failing-assertion propagation,
expect.assertionscounting across both matcher orderings and custom matchers, inline-snapshot writing through the deferred path, instance reuse, andtest.concurrentoverlap.
Given the scope (103 files), the user-visible semantic change, the JSC-bindings touch, and the demonstrated subtlety of the state-management edge cases, this should have human review before merge.
c49e09a to
45a36db
Compare
ed7abd1 to
b287117
Compare
770a652 to
2eb1538
Compare
The deferred promise is returned directly via MaybeDeferred::Deferred and rooted by PendingMatcher.deferred: JSPromiseStrong; the resultValue cached slot is no longer read (its only reader was deferred_result(), removed in ee7f710).
Matches the Bun__TestScope__Describe2__* pattern this code cites as its model. Zero functional change.
…t tests - maybe_defer_matcher doc still referenced the removed resultValue slot write; drop that clause. - packages/bun-inspector-protocol/test/inspector/websocket.test.ts had 13 un-awaited expect(...).resolves.<matcher>() calls in sync test bodies. These relied on the old blocking waitForPromise behavior and would race under the deferred path. Make the tests async and await each assertion.
The JSON.stringify() wrap around css() output was a local workaround
for the host bun's define-value parser choking on raw '*{...}' CSS.
#30679 fixed the JSON lexer to tokenize ?/*/(/) without erroring so
parse_env_json's auto-quote fallback can recover — and that fix is
now in the rebase base.
Reverting restores bake-codegen as the in-tree exerciser of that path
and un-stales the two comments that cite it (json_lexer.rs:1304 and
bun-build-api.test.ts:76).
…ckStart return types Both docs still described the old tuple return shapes and direct destructuring. 45a36db changed the signatures to return MatcherStart<'a>/MockStart<'a> enums so the Deferred(promise) path can early-return; callers now unwrap via ready_matcher!/ready_mock!. Align the docs with the actual signatures and mention the deferred branch.
… leak The test fires expect(Bun.sleep(1)).resolves.toBe(undefined) from a sync onExit callback without awaiting it. Under the deferred-matcher path that allocates a PendingMatcher Box and attaches it via .then() to the sleep promise. On release-asan CI the process exits before the 1ms timer fires, so on_settle() never runs and the 72-byte Box leaks, tripping LSan (surfaced by #31029's suppression cleanup). Capture the deferred promise and await it after Promise.all(exits) so the PendingMatcher is freed before process exit regardless of timing. The assertion is now actually checked too (1 expect() call instead of 0 on the fast path). The original purpose of this line was to force a synchronous nested event-loop tick via waitForPromise; that no longer applies under the deferred path (already noted in the preceding comment), and the test still validates the level-triggered pidfd fix via the all-exits-fire check.
The comment said 'self.expect_this: Strong', but self here is &mut RerunGuard which has no such field. The Strong that keeps the Expect wrapper alive is on the enclosing PendingMatcher, owned by the Box in on_settle() which outlives _guard. Safety argument is unchanged; only the field attribution was wrong.
The PendingMatcher Box (with Strong roots for the Expect wrapper, matcher fn, args array, and deferred promise) was only freed in on_settle(). If the input promise never settled before process exit — e.g. an un-awaited expect(pending).resolves... whose promise races teardown, as vendor/elysia's router.test.ts does — the Box leaked and LeakSanitizer aborted the ASAN lane. Keep all per-deferral state in a 9-slot JS array passed as the promise reaction's context via then_with_value, and make the deferred promise a plain GC JSPromise rather than a Strong. The reaction traces the array, so a never-settling promise takes the whole bundle with it when it is collected: nothing native outlives the reaction and there is nothing left for LSan to report. on_settle() now unpacks the array (flags/counted bits and srcloc round-trip as number/boolean/string slots) and rerun() takes the unpacked values; behaviour on the settle path is unchanged.
…comments - test/js/web/fetch/fetch.test.ts: the maxRedirects test added by #31518 (post-sweep, came in via rebase) fired expect(fetch()).rejects.toThrow without awaiting; under the deferred path the assertion would never be checked within the test. Same one-line await as the rest of the file. - expect.rs: async_rerun_srcloc doc referenced the removed PendingMatcher.srcloc_at_defer field; it now describes the SLOT_SRCLOC_* context-array slots and on_settle() ownership. - pidfd-exit-nested-tick.test.ts: drop the stale LSan rationale (nothing native is allocated since the GC-managed-context rewrite); the await is about completing the assertion within the test.
Read SLOT_DEFERRED first and move the remaining fallible slot reads into rerun(), so any failure there feeds the deferred's reject arm instead of leaving it pending. Also fix a stale comment in inline_snapshot that referenced PendingMatcher owning the srcloc deref.
5c06bfd to
c276620
Compare
|
Superseded by #33289, closing to keep the review surface clean. #33289 does this properly: it covers all three matcher entry points ( Its 129-site Tracked by #33261 work item 1. |
Reproduction
Jest passes this test in 1ms.
Root cause
Expect.processPromise()(called by every matcher'sgetValue()) ranvm.waitForPromise(promise)when.resolves/.rejectswas set, which synchronously spins the event loop until the promise settles. But the matcher is invoked from inside the test function —resolve(25)is the next statement on the same call stack and can never run whilewaitForPromise()is spinning below it.Fix
When
.resolves/.rejectsis set and the input promise is still pending,getValue()now:Promiseand returns it from the matcher (stored in the existing, previously-unusedresultValueslot).then()to the input promise; when it settles, the matcher is re-invoked — the input promise is now settled soprocessPromise()extracts its value synchronously — and the returned Promise is resolved/rejected with the matcher's outcomeAlready-settled promises still take the synchronous path through
processPromise(), so there's no behavior change forexpect(Promise.resolve(x)).resolves.….This matches Jest's behavior where
.resolves.<matcher>()returns a thenable: if youawait/returnit the test waits on the assertion; if you don't and it fails, it surfaces as an unhandled rejection that fails the test.getValue()now takes the*CallFrameand returns?JSValue—nullmeans "deferred, returndeferredResult(thisValue)". Every matcher call site is updated mechanically (orelse return this.deferredResult(thisValue)).applyCustomMatcher(forexpect.extend) is updated the same way.An
is_async_rerunflag onExpectprevents double-counting the expectation on the re-invocation.The new
onResolve/onRejectthenable handlers are registered inZigGlobalObject::PromiseFunctions.Not changed
processPromise()itself keeps itswaitForPromise()fallback forreadFlagsAndProcessPromise(asymmetric matchers called from C++), whereflags.promiseis.nonein practice and there's noCallFrameto defer with.getValueAsToThrow()/executeCustomMatcher()stillwaitForPromise()on a promise returned by the user's function/matcher — those aren't the.resolves/.rejectsinput promise and are a separate concern (see fix(test): invoke JS-level then() for lazy promise subclasses in expect().rejects/resolves #27260).Verification
The test spawns subprocesses with a bounded
Promise.raceso unfixed builds fail cleanly instead of hanging the runner (on current main the test-level--timeoutcan't interruptwaitForPromise()).Existing
expect.test.jsresolves/rejects tests,expect-extend.test.js, andjest-extended.test.jsall pass.zig:check-allpasses on all platforms.Fixes #14950
Fixes #25181
Rebase onto #30412
Main's #30412 ("Rewrite Bun in Rust") landed while this PR was in review and ported
src/test_runner/expect.zig→src/runtime/test_runner/expect.rs(plus eachexpect/*.zig→*.rs). The.zigfiles are no longer compiled; the live implementation is Rust, and it had the originalwait_for_promise()hang.Commit 45a36db ports the
PendingMatchermechanism to Rust:Expectgainsis_async_rerun/counted_expect_call/async_rerun_srclocasCell<_>fields.get_value()/matcher_prelude()/mock_prologue()returnMaybeDeferred/MatcherStart/MockStartenums;ready_matcher!/ready_value!/ready_mock!macros handle the early-return at each call site.PendingMatcherholdsStrongrefs to theExpectwrapper, matcher fn, and args array, plus aJSPromiseStrongfor the deferred result and per-defer snapshots ofcounted_expect_call/flags/ callerCallerSrcLoc. On settle it re-invokes the matcher with the snapshotted state installed/restored via an RAII guard.Bun__Expect__PendingMatcher__onResolve/onRejectare exported viajsc_host_abi!{ #[no_mangle] }so C++promiseHandlerID()matches them by fn-ptr identity (same pattern asBun__TestScope__Describe2__*).The earlier Zig commits in this branch document the design evolution and review rounds but touch only dead code after the rebase; the final commit reverts them.