Skip to content

test: rewrite in-process-cron.test.ts on top of fake timers - #33635

Closed
robobun wants to merge 2 commits into
mainfrom
farm/e3c7623d/cron-test-fake-timers
Closed

robobun wants to merge 2 commits into
mainfrom
farm/e3c7623d/cron-test-fake-timers

Conversation

@robobun

@robobun robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to #33623. Now that Bun.cron honors fake timers, every firing test can trigger a cron deterministically instead of waiting for a real minute boundary.

What changed

Subprocesses and workers can require("bun:test") and call jest.useFakeTimers() / setSystemTime() / advanceTimersByTime() even outside the test runner, so the subprocess and worker bodies prepend a small mockClock snippet and fire the cron with advanceTimersByTime(60_000).

  • callback fires at minute boundary, async callback: stop() during await, unreferenced job survives GC → in-process fake timers (no subprocess)
  • error-handling subprocess tests (sync throw, async throw, stop() while pending, unhandled error exits process) → same bun -e shape, cron fired via mocked clock
  • worker-terminate tests → worker script uses the mocked clock; N cut from 20 to 4 since the fire is now deterministic instead of probabilistic on a minute alignment
  • --hot reload → v1 arms a cron under the mocked clock; v2 advances past its fire time and checks it never fired

Assertions are unchanged from the originals except where a real-time await (Bun.sleep) now needs a second advanceTimersByTime to resolve.

Timing

Debug + ASAN, this container, 3 runs:

before after
file time ~117s 5.05s / 5.21s / 5.68s
tests 27 pass 27 pass

The remaining time is subprocess spawn overhead (all concurrent; --hot is the long pole at ~2s).

Subprocesses and workers can require("bun:test") and use
jest.useFakeTimers()/setSystemTime()/advanceTimersByTime(), so every
firing test is now deterministic instead of waiting for a real minute
boundary. The --hot and worker-terminate tests keep their subprocess
isolation but trigger the cron fire via the mocked clock.

File time on a debug build drops from ~117s to ~5s; release lanes will
be faster again.
@coderabbitai

coderabbitai Bot commented Jul 7, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 14 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d4f6a131-ca3e-4bca-8c0d-d92a839b2458

📥 Commits

Reviewing files that changed from the base of the PR and between 3f5d816 and 345eccb.

📒 Files selected for processing (1)
  • test/js/bun/cron/in-process-cron.test.ts

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the claude label Jul 7, 2026
@robobun

robobun commented Jul 7, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 7:46 AM PT - Jul 7th, 2026

❌ @robobun, your commit 345eccb has 2 failures in Build #69797 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 33635

That installs a local version of the PR into your bun-33635 executable, so you can run:

bun-33635 --bun

@robobun

robobun commented Jul 7, 2026 •

Copy link
Copy Markdown
Collaborator Author

Build 69797 finished: 283 jobs passed, 3 failed, none related to this diff.

in-process-cron.test.ts on this branch:

  • linux-x64-asan: 27 pass in 14.47s (was ~123s on main)
  • darwin-aarch64 release: 27 pass in 107ms (was ~60-120s on main)

Unrelated failures:

  • 2x :darwin: 26 aarch64 never ran tests: buildkite-agent artifact download timed out after 120s (same infra error on build 69785)
  • :darwin: 14 aarch64: test/integration/next-pages/test/dev-server.test.ts (Puppeteer WebSocket Connection ended after 3 retries) and test/js/bun/webview/webview.test.ts (WebView host process killed by signal 5)

Ready for review.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Re-verified this branch's test file against a current debug + ASAN build of main (da3851e, which includes the cron local-time change from #35122, the cron state cleanup in #37411 and the Worker teardown rework in #37075, all of which landed after this branch was cut). test/js/bun/cron/in-process-cron.test.ts has not changed on main since then, so the branch still applies as is.

Same binary, same machine:

file version result wall time
main 27 pass 100.7s (the daily slow-test sweep currently measures it at 124s on debian 13 x64-asan, the second slowest non-integration test file)
this branch 27 pass, 4 of 4 runs 5.1s to 5.9s

One note for anyone re-running locally: on a machine whose fs.inotify.max_user_instances budget is nearly used up, the --hot test can fail with Failed to enable File Watcher: EMFILE. That hits the version on main the same way and is unrelated to this change (the crash on that path is being addressed separately in #34070).

@robobun

robobun commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

Re-verified again on a debug + ASAN build of main at 6e906e4. That build contains three runtime changes that landed after the previous check and that this test file depends on:

test/js/bun/cron/in-process-cron.test.ts is still unchanged on main since this branch was cut, so the branch still applies as is. Same binary, same machine:

file version result wall time
main 27 pass 79.1s (the daily slow-test sweep measures it at 109s on debian 13 x64-asan in build 102501)
this branch 27 pass, 3 of 3 runs 4.3s to 4.5s

With this branch no test in the file waits for a real minute boundary.

@robobun

robobun commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Re-ran this branch's test file on a debug build of main at adc354d (18 commits behind today's HEAD 06820dc, none of them touch this file). The branch still applies cleanly. In 16 full-file runs, 12 pass in 4 to 7s and 4 fail. Every failure is the same test:

worker terminate mid-callback does not report TerminationException as uncaught

Two failure shapes show up in the child process:

  1. The worker panics (3 of 4 failures):
panic: assertion failed: now.eql(&prev.unwrap()) || now.greater(&prev.unwrap())
  <FakeTimers>::fire            src/runtime/test_runner/timers/FakeTimers.rs:256
  <FakeTimers>::execute_until   src/runtime/test_runner/timers/FakeTimers.rs:289
  advance_timers_by_time        src/runtime/test_runner/timers/FakeTimers.rs:442
  1. A worker emits an error event, so stdout is errors=1 instead of errors=0 (1 of 4 failures). The thrown error is Fake timers not initialized. Initialize with useFakeTimers() first. from advanceTimersByTime.

Cause

The fake clock is process wide, not per VM. CURRENT_TIME in src/runtime/test_runner/timers/FakeTimers.rs and the bun_core::mock_time statics are shared by every thread. That test starts N = 4 workers in one process. Each worker runs useFakeTimers(), setSystemTime(), Bun.cron() and advanceTimersByTime(60_000) on its own, so the workers move one shared clock.

  • Shape 1: several workers arm their cron job while the shared clock is at 0, so each job sits at +60s. The workers then advance one after another. Each advance moves the shared clock 60s further. The third worker pops its +60s timer while the shared clock is at +120s, and the debug assert at FakeTimers.rs:261 (Environment::CI_ASSERT) fails. A release build compiles the assert out, so this shape is only visible on the debug and ASAN lanes. The assert predates this branch (23427db, May 14).
  • Shape 2: the first worker to be terminated runs stop_active_handles (src/runtime/jsc_hooks.rs:1760), which calls reset_for_isolation and clears the shared CURRENT_TIME. A worker that has not yet reached advanceTimersByTime then throws TypeError: Fake timers not initialized. Initialize with useFakeTimers() first., the exception is uncaught in the worker script, and errors becomes 1. This shape also happens on release builds. It reproduces 3 of 3 times with two workers when the second worker calls Bun.sleepSync(1500) between Bun.cron() and advanceTimersByTime().

#38740 (a fake clock per VM) would remove both shapes. It is open and currently conflicts with main.

Suggested change

Use one worker per process in that test. Under fake timers the fire is deterministic, so the N loop no longer adds coverage. The other worker test in this file already uses one worker and passed in all 16 runs. If repetition is wanted, spawn several child processes with one worker each. Several workers with fake timers in one process will stay racy until #38740 lands.

Note for review: after this change no test in the file fires a job from a real timer tick. The fake clock path shares CronJob::on_timer_fire with the real one, so what is no longer covered end to end is the real-clock arming in compute_next_timespec and the event loop drain.

@robobun

robobun commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #40889. That PR keeps this rewrite and fixes the race described above: the terminate-mid-callback test now starts one worker per process, because the fake clock is process-wide. It also strengthens the assertions (exact messages, inline snapshot of the handle, keep-alive cases that can fail, a second fire after a handled error, the no-overlap guarantee). Closing this one so there is a single PR for the file.

@robobun robobun closed this Aug 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant