test(timers): speed up setTimeout.test.js and tighten its assertions - #38493
test(timers): speed up setTimeout.test.js and tighten its assertions#38493robobun wants to merge 4 commits into
Conversation
The file took 34-36s on the debian x64 ASAN lane against 6s elsewhere, and 150-160s under a local debug build, where the three leak tests also failed because the fixture keyed its ASAN RSS threshold off the binary name. - Leak fixture: RSS cannot tell a freed TimeoutObject from a leaked one under ASAN (the freed block sits in the quarantine; 200k timers grow RSS by ~140 MB either way), so ASAN and debug builds now run one batch per mode and rely on LeakSanitizer at child exit, which the test turns on itself (CI already does for the lane). Release builds keep the 100 batch workload and the 10 MB bound. The fixture prints a JSON report and the test asserts the workload size, protected count, live wrapper count and (on release) the RSS delta. - The five spawnSync unref fixtures and the two promise fixtures become one fixture that arms every scenario at once and reports how often each callback ran; the test asserts the exact report. - CPU usage #7790: measure the idle window in-process as a CPU/wall ratio instead of sleeping for 3s and bounding whole-process CPU, which depended on startup cost. resourceUsage() is still exercised, now against the child's own reading. - timers-fixture-unref.js: inline mustCall() instead of loading node/test/common (~2s on a debug build); failures name the call site. - Child-spawning tests that do not measure anything are it.concurrent; the CPU and latency measurements stay sequential. The refresh tests await the fires they care about instead of fixed 100-300ms waits and assert from the test body. - All children are run through bunRun/toSpawn with exact stdout and empty stderr; the quantization and GC children use console.log rather than process.stdout (whose lazy setup is ~0.9s on a debug build). bun bd test test/js/web/timers/setTimeout.test.js: 152-163s (3 failing) before, 12.3-13.0s after. Release: 6.3s before, 2.0-2.2s after. 21 child processes, all serial, before; 15 after, 6 of them serial.
|
Warning Review limit reached
Next review available in: 1 minute 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 (11)
Comment |
|
Status: ready for review (head 1ab6620). CI: builds 96032 and 96314 are green on every lane that ran (the listed red is retried flakes in unrelated files); the two darwin 14 aarch64 shards expired without ever getting an agent, which is happening to every build in the pipeline right now, so macOS has not run in CI for this PR (the file was run by hand on Linux and Windows, see below). Per-file cost on the asan lane in CI: 7.4s, of which ~4.3s is shard start-up because the modified file runs first (32.3s in test/expected-durations.json); 6.3s on debian x64, 1.6s on windows. Reproduced the slowness locally with The leak fixture was also checked against the bug it guards, with the #30058 leak put back into Review follow-ups so far: ceb7258 keeps the RSS check on every build without ASAN (review comment above); 1ab6620 gives the |
The only ASAN CI lane is Linux and that is where the LeakSanitizer path was verified; an ASAN debug build on macOS keeps the small workload without the explicit LSAN environment.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Keying the RSS check off !isDebug as well left debug builds without ASAN (Windows, x64 macOS) running one batch with neither the RSS check nor LeakSanitizer. Every build without ASAN now runs the 100 batch workload against the 10 MB bound; on a Windows debug build that takes 8-10s per mode (the modes run concurrently), grows RSS by 1-2 MB, and by ~100 MB with the #30058 leak reintroduced. ASAN builds on any platform take the one-batch LeakSanitizer path; bun maintains LSan suppressions for macOS, so the Linux-only guard is gone too.
|
Updated 7:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit ceb7258 has some failures in 🧪 To try this PR locally: bunx bun-pr 38493That installs a local version of the PR into your bun-38493 --bun |
There was a problem hiding this comment.
LGTM — the follow-up commit (ceb7258) addresses the debug-without-ASAN gap: the RSS check is now keyed on !isASAN alone, so Windows and x64-macOS debug builds run the full 100-batch workload against the 10 MB bound, and the comment above the block now names that configuration explicitly.
What was reviewed:
- Traced each of the seven deleted
setTimeout-unref-fixture-*.jsscenarios to a counter in the consolidated fixture; the exact-count report preserves (and in thethis-binding case strengthens) what each old fixture asserted. - Verified
bunRun/toSpawnintest/harness.tsmatch the usage here (array argv, env spread overbunEnv, exit-0 + empty-stderr + optional exact-trimmed-stdout). - Checked that dropping the
isLinuxguard onleakFixtureEnv(now purelyisASAN) is safe for arm64-macOS ASAN builds; verifier agents ruled this out independently.
Extended reasoning...
Overview
Test-only refactor of test/js/web/timers/setTimeout.test.js and its fixtures: consolidates seven setTimeout-unref-fixture-*.js files into one exit-report fixture, rewrites the #30058 leak fixture to print a JSON report and use LeakSanitizer under ASAN / RSS otherwise, replaces the fixed-3s CPU test with a self-measured 300 ms window, inlines a minimal mustCall in timers-fixture-unref.js to avoid ~2 s of Node-common loading, converts the sleep-based refresh tests to await-the-condition, and moves independent subprocess tests to it.concurrent. No runtime code touched.
Delta since my prior review
My earlier inline comment flagged that the branching leakFixtureMeasuresRss = !isASAN && !isDebug left debug builds without ASAN (Windows, x64 macOS) with neither the RSS bound nor LeakSanitizer. Commit ceb7258 changes the gate to leakFixtureBatches = isASAN ? 1 : 100 and if (!isASAN) expect(rssDeltaMB).toBeLessThan(10), so those builds now run the full workload with the RSS check restored. The block comment was rewritten to name all three configurations, the fixture header documents the ~100 MB debug-no-ASAN leak signature, the per-test timeout was raised to 60 s to cover the 8-10 s debug run, and the now-unused isDebug import was dropped. This fully resolves the concern.
Security risks
None — no auth, crypto, network, or parsing of untrusted input; changes are confined to test fixtures and assertions.
Level of scrutiny
Medium. The diff is large (~500 lines) and reshapes how a leak regression is detected, which is safety-net-adjacent, but it ships no runtime code. I focused on whether every deleted assertion still has an equivalent-or-stronger replacement: each old fixture's exit-code/stdout invariant maps to a specific key in the new fixture's exact-count JSON report; the leak fixture keeps the release-build 100-batch/10 MB check unchanged and adds liveTimeouts/protectedTimeouts assertions on top; the CPU test still exercises Subprocess.resourceUsage() with a tighter invariant (total = user + system and total ≥ lifetimeCpuUs); the refresh tests now assert exact fire counts rather than toBeGreaterThanOrEqual.
Other factors
The PR description documents extensive verification (ten debug+ASAN runs, ASAN-lane env, release, Windows release canary, and reintroducing the #30058 leak to confirm all three modes fail with a TimeoutObject::init_with LSAN report). bunRun/toSpawn semantics in test/harness.ts match every call site. The bug-hunting system found no issues this run, and the one candidate it examined (dropping the isLinux guard on leakFixtureEnv) was refuted.
…cess Whether ref() re-refs the event loop is only observable when the timer is the only thing keeping the process alive; inside the combined fixture the other ref'd timers would let the callback run even with a no-op ref(), so that case is its own child again (exit code 1 unless the callback runs), and the combined fixture no longer lists it. The LSAN_OPTIONS fallback used by a plain `bun bd test` now carries print_suppressions=0 like the CI runner's value, otherwise a structural leak covered by test/leaksan.supp would print a "Suppressions used" block and fail the empty-stderr assertion.
There was a problem hiding this comment.
LGTM — the debug-without-ASAN gap I flagged earlier is addressed in ceb7258 (RSS check now keyed on !isASAN alone, verified on a Windows debug build against the reintroduced #30058 leak), and 1ab6620 correctly keeps the unref().ref() keep-alive check in its own process.
What was reviewed:
- Merged unref fixture: traced each of the seven deleted fixtures to a scenario in the new report; the one that must run alone (
unref().ref()) is a separate-etest. - Leak fixture branching: non-ASAN builds keep the 100-batch / 10 MB RSS bound; ASAN builds get the same LSan env CI sets, with
print_suppressions=0so the empty-stderr assertion holds;test/leaksan.supppath resolves. bunRun/toSpawnsemantics inharness.tsmatch what the tests rely on (exit 0, stderr exactly empty, optional exact stdout).- Concurrent grouping: the in-process
heapStatsprotected-count test and the CPU/latency tests stay sequential between the two concurrent groups.
Extended reasoning...
Overview
Test-only change to test/js/web/timers/setTimeout.test.js and its fixtures: consolidates seven single-scenario unref fixtures into one that reports per-scenario callback counts, moves the leak fixture's assertions into the test (JSON report) with the RSS bound applied on every non-ASAN build and LeakSanitizer enabled for the child on ASAN builds, replaces the fixed 3s CPU sleep with a 300 ms self-measured window, rewrites the four refresh tests to await fires instead of sleeping, inlines a minimal mustCall in timers-fixture-unref.js, and marks the subprocess-spawning tests it.concurrent in two groups with the CPU/latency/heapStats tests kept sequential between them. No production code is touched.
Security risks
None. Test-only; child processes are spawned via bunRun with bunEnv; the only added env is ASAN/LSAN options and BUN_DESTRUCT_VM_ON_EXIT for the leak fixture, mirroring what scripts/runner.node.mjs already sets on the ASAN lane.
Level of scrutiny
Moderate — it rewrites tests that guard specific past regressions (#30058, #7790, epoll EINTR, id-map GC), so the check is that each rewritten test still catches the bug it was written for. The PR description and follow-up commit provide direct evidence for the load-bearing ones: #30058 reintroduced locally fails all three leak tests on debug+ASAN via LSan and on a Windows debug build via the 100 MB vs 1-2 MB RSS delta; the CPU test's 50% bound sits between ~0.2-3% idle and ~100% spinning; the merged unref fixture prints the identical report under Node.
Other factors
My earlier review (the debug-no-ASAN configuration ending up with neither RSS nor LSan) was acknowledged and fixed in ceb7258 with measurements on the platform in question, and the thread is resolved. I confirmed bunRun merges the extra env over bunEnv and toSpawn asserts exit 0 + empty stderr (+ exact stdout when given), which is what the tightened assertions depend on. The concurrent-group placement leaves the in-process heapStats().protectedObjectTypeCounts test and the timing-sensitive CPU/quantization/GC tests sequential, so they still run alone. The bug-hunting pass found nothing, and CI build 96032 shows the file at 7.4s (asan) / 6.3s (debian) / 1.6s (windows) versus 32.3s / 7.5s / 5.9s in expected-durations.json.
1 participant
Problem
test/js/web/timers/setTimeout.test.jsis a serial-phase file that takes 34-36s on the debian 13 x64 ASAN lane (builds 95391, 95331) and 6s on every other lane.bun bd test) it takes 152-163s, and the threedoesn't leak when ... inside its own callbacktests fail: the fixture keyed its ASAN RSS limit off the binary namebun-asan, sobun-debug(also ASAN) got the 10 MB limit and measured ~140 MB.GC of many id-accessed timers4.9s,CPU usage #77903.3s (a fixed 3s sleep),timers-fixture-unref.js2.3s (1.9s of it loadingtest/js/node/test/common), plus 21 child processes started one after another.TimeoutObjects are caught by LeakSanitizer at child exit, which does not care how many timers the fixture creates.Fix
print_suppressions=0, since a structural leak covered byleaksan.suppwould otherwise be listed on stderr), sobun bd teston Linux and arm64 macOS detects the leak too. All builds additionally assert the workload ran,protectedTimeouts === 0, and that fewer than 100Timeoutwrappers survive the final GC (2-4 observed; retention leaves thousands). Timeout 90s -> 60s (the non-ASAN debug configuration needs ~10s).setTimeout-unref-fixture.jsreplaces seven fixtures (fivespawnSynctests and the twoReturning a Promisetests): every scenario is armed at once and the exit handler reports how often each callback ran; the test asserts the exact report. Node prints the identical report. Making oneunref()ineffective changes two counts, so a regression is a diff, not a hang. The one scenario that cannot share a process,unref().ref()keeping the loop alive (any other ref'd timer would mask a no-opref()), stays a separate child (setTimeout -> unref -> ref works, a-escript that exits 1 unless the re-ref'd timer runs; verified to exit 1 withTimeout.prototype.refreplaced by a no-op).bun-asanname check in this and ten other fixtures by widening the ASAN RSS bound, which per the measurement above still cannot distinguish a leak under ASAN; Have condition-gated drive loops ref the event loop so unref'd timers fire without spinning #32014 and timers: root the event-loop-delay histogram, fix aliased-&mut in timer drain, arm uv timer on the owning VM's loop #31837 carry small edits to the same fixture. If the release-RSS / ASAN-LeakSanitizer split here is the policy wanted for the other fixtures, the environment block belongs intest/harness.tsnext toisASAN; it is kept local here so this PR stays a single-file change.CPU usage #7790: the child measures its own CPU over a 300ms window with a far-off timer pending and the test asserts under 50%, the shape of the epoll test below it. Spinning reads ~100%, idle reads ~0.2% release / ~3% debug+ASAN, and the bound no longer includes startup cost.Subprocess.resourceUsage(), which this test was the main exerciser of, is still checked:total === user + system, andtotalis at least theprocess.cpuUsage()total the child printed before exiting (same kernel counters read later).timers-fixture-unref.js(shared withsetInterval.test.js):mustCallinlined with the same contract; a mismatch prints the call site and exits 1. 2.3s -> 0.35s on debug.it.concurrent, in two groups; the CPU and latency tests stay sequential and, since bun starts a sequential test only after the preceding concurrent group drains, still run alone. The protected-count test stays sequential as well.refresh()return value and the exact fire counts, and assert from the test body. The "no extra fire" checks rest on deadline order (a wrong re-arm is due before the timer the test waits on), not on margins.bunRun/toSpawn: stderr exactly empty (theWARNING: ASAN interferesfilter matched text no current build prints; other tests here already asserted""), stdout compared exactly. The quantization and GC children useconsole.log;process.stdout's lazy setup is ~0.9s per child on debug. The GC test's workload and bound are unchanged since its thin margin is against the unfixed numbers.bun bd test test/js/web/timers/setTimeout.test.js(debug+ASAN): 152.4s / 162.9s with 3 failures before; 12.3-14.9s, 30-31 pass, over eleven runs after (the slower runs had other work going on in the container; the head adds one test).bun bd testthere): 40.7s on main, 14.8s on this branch, 27 pass + 3 Linux-only skips; the leak modes take 8.6-10.6s each and overlap. With the timers: release heap ref when setTimeout is cleared or refreshed inside its own callback #30058 leak reintroduced in that build the fixture reportsrssDeltaMB100.1-100.9 against 1.1-1.9 clean, withliveTimeouts/protectedTimeoutsunchanged, so the RSS bound is the detector there andheapStatsindeed cannot be.detect_leaks=1,test/leaksan.supp,BUN_DESTRUCT_VM_ON_EXIT=1,--timeout 270000): 30 pass in 14.0s, every child LeakSanitizer-clean.USE_SYSTEM_BUN=1 bun test ...(release, full leak workload and RSS bound): 6.27s before, 2.0-2.4s after.scripts/update-test-durations.mjsmeasures it;test/expected-durations.jsonhas the file at 32.3s asan / 7.5s default / 5.9s windows): 7.4s on the asan lane, 6.3s on debian x64, 1.6s on windows x64. As the modified file it ran first on its shards, so the two Linux numbers include ~4.3-4.5s of shard start-up before the first test finished; the tests themselves span 3.0s (asan) and 1.8s (debian) of those.timer_object_internals.rs(reverted before pushing): all three leak tests fail onbun bd test(debug+ASAN) withDirect leak of 960000 byte(s) in 2000 object(s)fromTimeoutObject::init_with/All::set_timeout.setInterval.test.js: the shared fixture passes on debug and release; itsdoesn't leak memorytest times out on debug exactly as on main (test(timers): speed up setInterval.test.js and tighten its assertions #35750 covers it).USE_SYSTEM_BUN=1, test-only change so no build needed): 27 pass, 3 skip (the Linux-only epoll tests), 1.43s, stable over five runs; the leak fixture at the full workload reportsrssDeltaMB0.7-0.8 andliveTimeouts3-4 there, and the unref fixture prints the same report as on Linux and node.Background
scripts/runner.node.mjsturns it on (detect_leaks=1plustest/leaksan.supp) for every test file not listed intest/no-validate-leaksan.txt, and children spawned withbunEnvinherit it.BUN_DESTRUCT_VM_ON_EXIT=1tears the VM down first, which is what makes the exit scan take ~0.1s instead of ~3s on a debug build.TimeoutObject: the refcounted native box behind a JSTimeout. timers: release heap ref when setTimeout is cleared or refreshed inside its own callback #30058 left one refcount behind when a callback cleared/refreshed/converted its own timer, so the box (~100 bytes on release, ~500 on debug builds) leaked while the JS wrapper was still collected;heapStatstherefore cannot see that leak, RSS or LeakSanitizer is needed.it.concurrenttests run together; a plainitstarts after the group finishes (src/runtime/test_runner/Execution.rs). CI runs files with--timeout90s (270s on ASAN), so the 5s default only applies when running a file by hand.bunRun/toSpawn(test/harness.ts): spawn bun withbunEnv;toSpawn(expected?)asserts exit 0, empty stderr and optionally the exact trimmed stdout, printing both streams on failure.Per-test debug-build timings, before -> after
Release (
USE_SYSTEM_BUN=1) leak fixture at the unchanged 100-batch workload:rssDeltaMB0-1,liveTimeouts3-4,protectedTimeouts0, 0.25-0.43s per mode.