Conversation
jest.advanceTimersByTime(), advanceTimersToNextTimer(), runAllTimers() and runOnlyPendingTimers() are the sync fake-timer APIs. In Jest (via @sinonjs/fake-timers) they fire each due callback without flushing the microtask queue; a Promise scheduled inside a callback settles at the test's next real await. The *Async variants opt into per-timer flushing. Bun fired each fake timer through TimerObjectInternals::fire, which wraps the callback in event_loop().enter()/exit(). The test body itself is not inside an enter/exit pair, so exit() saw entered_event_loop_count == 1 and drained microtasks after every fired timer. A migrated Jest suite asserting "callback ran but its .then hasn't yet" observed a different (still deterministic) order. Set vm.suppress_microtask_drain for the duration of the four sync host functions so the inner exit() becomes a no-op. The guard restores the prior value on drop so nesting (e.g. inside spawnSync) is preserved.
WalkthroughChangesThe change adds scoped VM microtask-drain suppression, uses it during event-loop and synchronous fake-timer execution, and adds subprocess tests for Jest-compatible callback and microtask ordering. Microtask suppression
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:27 PM PT - Jul 24th, 2026
❌ @robobun, your commit f2271f9 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35496That installs a local version of the PR into your bun-35496 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I also checked: the guard in advance_timers_by_time is armed only after argument validation, so the early-return error paths never touch the flag; drain_microtasks_with_global() (event_loop.rs:321) does honor suppress_microtask_drain; and there are no *Async fake-timer variants in-tree yet, so the four sync host functions are the complete set.
Extended reasoning...
The mechanism is sound and the tests cover each affected API plus the guard-drop restore. Deferring rather than approving because this is a user-observable ordering change to Jest-compat semantics — worth a human sign-off on the compat trade-off and on whether the guard dedup should happen before merge.
…g in a subprocess Define the guard once as VirtualMachine::suppress_microtask_drain_scope() and reuse it in FakeTimers and EventLoop::tick_tasks_only (the erased-pointer SpawnSyncEventLoop copy keeps its FFI shim for crate layering). The in-process ordering assertions depended on the first test in the file to run without a prior real-time await; once an earlier test suspends through the event loop, subsequent tests run at enter/exit depth >= 1 and the eager drain is already skipped. Run the ordering check in a fresh bun test subprocess so it is independent of what ran before it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/jsc/VirtualMachine.rs`:
- Around line 1176-1183: Update the SuppressMicrotaskDrain guard to include a
PhantomData<*mut ()> marker, preventing automatic Send and Sync implementations.
Initialize the marker in suppress_microtask_drain_scope while preserving the
existing vm and prev fields and drop behavior.
In `@test/js/bun/test/fake-timers/fake-timers.test.ts`:
- Around line 139-154: Update runOrderingFixture and the inline
advanceTimersToNextTimer subprocess test to assert stdout, stderr, and exitCode
through one combined result object after the existing concurrent Promise.all
drain. Preserve the current expected values and ordering while replacing the
separate expect calls in both locations with a single combined assertion.
- Around line 160-188: Refactor the advanceTimersToNextTimer test to use the
shared runOrderingFixture helper instead of duplicating tempDir creation,
Bun.spawn, stream draining, and exit assertions. Parameterize runOrderingFixture
as needed to supply this test’s ordering fixture and expected log output, while
preserving the existing assertions and behavior of the three sibling tests.
🪄 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: b0f2d278-5fdd-4dbe-a918-92780f4454aa
📒 Files selected for processing (4)
src/jsc/VirtualMachine.rssrc/jsc/event_loop.rssrc/runtime/test_runner/timers/FakeTimers.rstest/js/bun/test/fake-timers/fake-timers.test.ts
…lper VirtualMachine is unsafe impl Sync, so the guard would auto-derive Send/Sync and could be dropped off-thread; add a PhantomData<*mut ()> to pin it to the JS thread. Parameterize runOrderingFixture(timerCount, api, expected) so all four subprocess tests share one spawn path, and assert the extracted order line and exit code as a single object (stderr surfaces only on non-zero exit).
Matches the other RAII scope guards in bun_jsc (AutoGcOnDrop, MacroModeGuard, EventLoopEnterGuard); an unbound call would set and immediately restore the flag in the same statement.
There was a problem hiding this comment.
LGTM — all prior feedback (guard dedup onto VirtualMachine, !Send marker, #[must_use], test helper dedup, combined subprocess assertion) has been addressed.
What was reviewed:
SuppressMicrotaskDrainguard: save/restore viaCell::replace/setpreserves nesting;#[must_use]+PhantomData<*mut ()>match sibling guards in the file.event_loop.rs::tick_tasks_onlyrefactor is behaviorally identical to the prior inline replace/set (and now restore-on-unwind safe);vm_ref()returns&'staticso the guard doesn't conflict with&mut self.- Guard placement in each host fn: created after all
?validation, dropped at scope end;CURRENT_TIME.setinside the scope doesn't enter/exit the event loop. describe.concurrent: the four subprocess tests don't touch in-process fake timers, so the fifth in-process test running concurrently is isolated.
Extended reasoning...
Overview
Four sync fake-timer host functions (advanceTimersByTime, advanceTimersToNextTimer, runAllTimers, runOnlyPendingTimers) now hold a suppress_microtask_drain RAII guard while firing callbacks, matching Jest 30 / sinon fake-timers semantics where only the *Async variants flush microtasks between timers. The guard is a new public helper on VirtualMachine (per earlier review), also adopted at the pre-existing inline site in EventLoop::tick_tasks_only. Subprocess-based ordering tests cover all four APIs plus a "drain resumes after return" check.
Security risks
None. Test-runner-only code path; no untrusted input parsing, no auth/crypto, no I/O. The Cell<bool> is per-VM state written on the JS thread only (enforced by the PhantomData<*mut ()> marker).
Level of scrutiny
Medium-low. This is a user-visible ordering change, but (a) it uses the existing suppress_microtask_drain mechanism already proven by spawnSync, (b) it's scoped strictly to the four sync fake-timer host functions inside bun:test, not the general runtime, and (c) it moves toward Jest parity rather than away from it. The event_loop.rs hunk is a pure refactor (manual replace/set → RAII) with identical semantics.
Other factors
Two prior review rounds (mine and CodeRabbit's) surfaced five items — guard duplication, auto-Send, missing #[must_use], test boilerplate dedup, combined subprocess assertion — all addressed in e2574e6/a644a6d/f2271f9 and marked resolved. The PR description reports the full fake-timers suite (36 pass) and downstream fake-timer users (test-timers, cron tests, regressions 25869/26284) still passing. The tests correctly use tempDir/bunEnv/bunExe, drain pipes concurrently, and assert a combined {order, exitCode, stderr} object.
|
CI on build #79999:
Ready for review. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-25, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
2 participants
What
jest.advanceTimersByTime()(andadvanceTimersToNextTimer,runAllTimers,runOnlyPendingTimers) no longer drain the microtask queue while firing timer callbacks. This matches Jest 30 /@sinonjs/fake-timers, where only the*Asyncvariants flush microtasks between timers.Repro
Before:
ORDER ["T1","P1","after-advance10","after-await"]Jest 30.4.1:
ORDER ["T1","after-advance10","P1","after-await"]After:
ORDER ["T1","after-advance10","P1","after-await"]Cause
Each fake timer fires through
TimerObjectInternals::fire, which wraps the callback inevent_loop().enter()/exit(). The test body itself is not inside an enter/exit pair (run_callback_with_result_and_forcefully_drain_microtaskscalls the test callback without one), soexit()seesentered_event_loop_count == 1and callsdrain_microtasks()after every fired timer.Fix
Set
vm.suppress_microtask_drain = truefor the duration of the four sync fake-timer host functions, restored via RAII drop.drain_microtasks_with_global()already early-returns on that flag (it's the same mechanismspawnSyncuses). The guard saves/restores the prior value so nesting is preserved.Verification
All existing fake-timer users (
test-timers.test.ts,in-process-cron.test.ts,cron-local-time.test.ts, regression 25869/26284) still pass.[review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file