bun test: don't park the event loop after a test file's entry promise settles - #36453
Conversation
… settles With a ref'd handle pending (e.g. a user setTimeout left running by an earlier test file), the final auto_tick in load_entry_point_for_test_runner kept the loop active and parked in the poller until the next timer deadline, typically JSC's incremental sweeper armed ~100ms out by the per-file GC. Every subsequent test file that loaded a module paid that wait. Pre-arm the waker so the tick drains I/O without blocking, matching the existing wakeup before the entry-point load. Fixes #36450
WalkthroughThe test-runner load path now pre-arms the event-loop waker before ChangesTest runner event-loop wakeup
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/regression/issue/36450.test.ts`:
- Around line 4-11: In test/regression/issue/36450.test.ts, remove the
explanatory prose comments surrounding the regression test and retain only the
issue URL comment. Apply the same cleanup to the additional locations identified
by the review, preserving the test code unchanged.
- Around line 15-34: Strengthen the regression fixture in the subprocess setup
and generated plain modules: seed an expected-timer environment flag for each
subprocess, set an armed flag immediately after the setTimeout call in
leak.test.ts, and add a top-level guard in each plain module that fails when the
timer is expected but the armed flag is absent. Preserve the existing module
imports and test assertions while ensuring the setup precondition is validated
before plain-module evaluation.
🪄 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: 4fe77fd7-5654-419b-ad44-484787bde526
📒 Files selected for processing (2)
src/jsc/VirtualMachine.rstest/regression/issue/36450.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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 checked that the new self.wakeup() also runs on the watch-mode branch (it's placed after the if/else) — harmless there since wakeup() just signals the eventfd and the loop already exited with the promise settled. The pre-arm mirrors the existing one at test_command.rs:3268. Still worth a human look at whether this is the right layer vs. making the post-settle auto_tick() non-blocking or conditional.
Extended reasoning...
The runtime change is 2 lines in load_entry_point_for_test_runner and follows the exact precedent at test_command.rs:3268 (same vm.wakeup() before an auto_tick that shouldn't park). The regression test is well-designed (paired median against a control run, fixture guard proving the timer is armed). The one open question is architectural — REVIEW.md's "fix at the layer that owns the invariant" — since an unconditional auto_tick() after the promise has already settled arguably shouldn't be a blocking tick at all. That's a judgment call for someone who owns the event-loop draining semantics, not something I can settle from the diff.
There was a problem hiding this comment.
LGTM — the stderr-surfacing feedback from the earlier pass is addressed in 0d7b612.
What was reviewed:
- The
self.wakeup()pre-arm mirrors the existing pattern at test_command.rs:3267-3268 (same rationale comment), and sits after the if/else so both watch and non-watch paths get it. - Regression test: paired-median design against a control run, fixture guard proves the timer is armed before measured files, pipes drained concurrently, combined
{gapCount, exitCode, stderr}assertion so child failures self-diagnose. - Checked that
wakeup()is a pure eventfd pre-arm (delegates toevent_loop.wakeup()) with no side effects beyond making the subsequentauto_tick()non-blocking.
Extended reasoning...
Overview
Two-file change: a one-line self.wakeup() call (plus comment) inserted before the final auto_tick() in VirtualMachine::load_entry_point_for_test_runner, and a new regression test at test/regression/issue/36450.test.ts.
Security risks
None. The change only affects test-runner event-loop scheduling (whether a poll blocks or returns immediately). No user input, no parsing, no auth/crypto surface.
Level of scrutiny
Low-to-moderate. The runtime change is a single method call following a pattern already established two lines up the call stack — test_command.rs:3267-3268 calls vm.wakeup() immediately before load_entry_point_for_test_runner with the comment "need to wake up so autoTick() doesn't wait for 16-100ms after loading the entrypoint". This PR applies the same pre-arm before the trailing auto_tick(), which the original pre-arm no longer covers once an async-transpiled import consumes the eventfd edge. The root-cause analysis in the PR description is detailed and the mechgate evidence shows the test failing on the unfixed debug build with a 91.48ms delta (well above the 10ms threshold).
Other factors
- All prior review threads are resolved: CodeRabbit's fixture-guard suggestion (7ad69c4), comment-cop's comment-length nags (22c255c, 1e9cecc), and my own stderr-surfacing note (0d7b612).
- The test uses a paired median across 6 files with a control run, so machine speed cancels out and the 10ms threshold sits well below the ~15ms (release) / ~90ms (debug) unfixed signal — a reasonable design against timing flakiness.
- Test follows harness conventions:
tempDir,bunEnvspread,bunExe(), concurrent pipe drain viaPromise.all,await usingfor the subprocess. - The bug-hunting system found no issues on the current revision.
2 participants
Fixes #36450
Problem
In
bun test, one pending ref'd timer (e.g. @tanstack/react-query's 5-minutegcTimetimeout) made every subsequent test file that loads a module take a fixed extra ~15-100ms, linear in file count. A 20-file suite went from ~15ms to ~2s on the reporter's machine.Repro
One file runs
setTimeout(() => {}, 300_000)in a test; later files eachimporta small module and run a trivial test. Wall time grows ~100ms per file while CPU stays idle.Cause
load_entry_point_for_test_runnerends with an unconditionalauto_tick()after the entry promise has already settled. With a ref'd handle pending, the uws loop is active, so that tick parks in epoll/kqueue until the next timer deadline. The per-fileperform_gc()arms JSC's incremental sweeper ~100ms out, so the park runs the full ~90ms, once per file. Files without an async-transpiled import dodge it only because thevm.wakeup()pre-arm before the entry-point load leaves an eventfd edge pending; a module load's async transpile round-trip consumes that edge first.Fix
Pre-arm the waker before the final
auto_tick, same as the existingwakeup()before the entry-point load: the tick still drains ready I/O and due timers but returns immediately instead of blocking, since the test runner continues synchronously either way.Verification
New regression test measures the gap between module evaluation and the first test callback, paired against a control run without the pending timer. Unfixed the delta is ~15ms (release) / ~92ms (debug); fixed it is <2ms. Repro timing with the debug build (21 files): 2.53s before, 0.62s after (0.58s baseline without the pending timer).
[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The test runner's pre-load wakeup pre-armed the event loop waker once, but an async-transpiled import consumed that eventfd edge before the trailing auto_tick() ran, so with a pending ref'd timer keeping the loop alive the tick had nothing to wake it and blocked until the poll's 100ms timeout, costing every subsequent module-loading test file that delay. The fix calls self.wakeup() after the test file's entry promise settles and before the final auto_tick(), re-arming the waker so the tick returns immediately instead of parking. This mirrors the existing pre-arm pattern used just before the…