test(timers): speed up setInterval.test.js and tighten its assertions - #35750
test(timers): speed up setInterval.test.js and tighten its assertions#35750robobun wants to merge 3 commits into
Conversation
The file was one of the slowest in the suite (24s on debian 13 x64-asan in CI, and the leak test actually times out at 30s on a local debug+ASAN build). The work was dominated by the leak fixture (350 batches of 1,000 timers with 10,000 ticks each) and four serial spawnSync subprocesses. Fixture changes (coverage preserved): - setInterval-leak-fixture.js: 50+300 batches of 1,000 timers -> 3+20 batches of 200 timers; attach a 32 KiB Buffer per timer instead of a ~200 byte string so a real leak is still >100 MB over baseline while the no-leak delta stays <20 MB. The protectedObjectTypeCounts.Timeout check is unchanged. Also detect ASAN via bun:internal-for-testing (the old execPath name check was false on bun-debug even though it is ASAN-instrumented). - setinterval-cancel-fixture.js: 50,000 -> 5,000 timers; 1M-element arg array -> 100k. A single stray callback after clearInterval still fails the test. - setInterval-fixture.js: 100 -> 25 ticks at 16ms; drop the per-tick log line, print PASS on success and the offending gap on failure. Test file changes: - Convert the four subprocess tests from the sync .toRun() matcher to it.concurrent with Bun.spawn, and assert stderr/stdout before exitCode. - Fix the 'refreshed setInterval should not reschedule again' test: it returned immediately without awaiting the interval, so its assertions never ran. It now awaits three fires and uses expect() instead of throwing from the callback. - Fix the 'clearInterval' test to clear its second interval instead of leaving it running for the rest of the file. - Add an explicit expect(remaining).toBe(0) to the async setInterval test. Local bun bd test timings (debug+ASAN, Linux x64): before: 53-57s (leak test times out at 30s) after: 8-11s, 8/8 pass over 10 consecutive runs
WalkthroughChangesThe timer tests now validate cumulative interval timing, explicit callback outcomes, concurrent fixture processes, cancellation behavior, and leak thresholds. Fixtures use reduced allocations and batch sizes, with direct stdout, stderr, and exit-code assertions. Timer test coverage
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs. The changes look correct and well-motivated, but reducing the leak-fixture workload ~75× (300×1000 → 20×200 timers) and lowering the ASAN RSS threshold from 256 MB to 50 MB are sensitivity judgment calls I'd like a human to sign off on.
What was reviewed:
- Verified
isASANEnabledexists inbun:internal-for-testingandbunEnvsetsBUN_DEBUG_QUIET_LOGS=1/NO_COLOR=1, soexpect(stderr).toBe("")is safe here. - Confirmed the fixture filename case now matches disk (
setinterval-cancel-fixture.js) andtimers-fixture-unref.jsproduces no stdout/stderr on success. - Confirmed the original
refreshed setIntervaltest was a no-op (returned before the interval fired) with a fallthrough bug in its switch — the rewrite is a genuine fix. - Checked pipes are drained concurrently and
await usingcleans up the spawned processes.
Extended reasoning...
Overview
Test-only PR touching four files under test/js/web/timers/: three fixtures and one test file. No production code. It reduces iteration counts in the leak/cancel/delay fixtures, replaces the sync .toRun() matcher with it.concurrent + Bun.spawn (draining stdout/stderr/exit concurrently), rewrites a test that previously never awaited its interval, and tightens several assertions from exit-code-only to explicit stdout/stderr checks.
Security risks
None. Pure test changes; no user-facing API surface, no auth/crypto/permissions, no untrusted input handling.
Level of scrutiny
Medium. Test-only changes are low blast-radius, but this PR deliberately reduces the workload of a leak-detection test by roughly 75× and lowers the ASAN RSS threshold from 256 MB to 50 MB. The PR description backs these numbers with 20 local runs showing 7–17 MB (no leak) vs 143 MB (simulated leak) under debug+ASAN, and the primary signal (protectedObjectTypeCounts.Timeout) is unchanged. That's convincing, but whether 20×200 timers still catches the class of leak the original 300×1000 was written for — and whether the 50 MB threshold holds on Windows/macOS CI — is a judgment call better made by a maintainer than a bot.
Other factors
- The rewrite of
refreshed setInterval should not reschedule againis a real fix: the old body returned synchronously so the callback (and its throws, which also had a switch-fallthrough bug requiringelapsedto be exactly 180) never ran. The new version awaits three fires with properexpect()assertions and clears the timer on both paths. This is strictly better, though it introduces new timing assertions (>= 180/< 180) that could in principle be flaky on very slow CI — the PR reports 10 consecutive passing runs on debug+ASAN. - The
expect(stderr).toBe("")pattern was flagged as a candidate concern but ruled out: it matches the pattern intest/CLAUDE.md, andbunEnvsilences debug logs. Promise.withResolverspolyfill removal is fine (native in Bun); the removed.toRun()calls are replaced with strictly tighter assertions.- Cancel fixture drop from 50k→5k timers: the invariant being tested (first fire cancels all before any fires twice) is still exercised, but 50k may have been chosen to stress the O(n) cancel loop under load. Worth a maintainer glance.
- setInterval-leak-fixture.js: assert on heapStats.objectTypeCounts.Timeout (201 in the no-leak case, thousands on leak) as the primary retention check. The RSS check becomes a backstop with a single 50 MB threshold, so the isASAN detection and dual threshold are no longer needed. - setInterval-fixture.js: check each tick against its scheduled time (N*delta from start) instead of the gap since the previous tick. If the event loop stalls past the interval, the catch-up tick legitimately fires with a near-zero gap; the old check reported that as an early fire. This was a pre-existing flake on main, not introduced here. - setInterval.test.js: drop the explicit 30s timeouts on the delay and cancel subprocess tests (both <2s), drop the Windows-only 90s leak timeout and the isWindows import (the 80s Windows runtime it guarded against no longer exists), and keep 30s only on the leak and unref fixtures which approach the default 5s under debug+ASAN with concurrent subprocesses.
![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/js/web/timers/setInterval.test.js`:
- Line 127: Remove the exact-empty stderr assertions from the subprocess tests
at the affected expectations, while preserving stderr draining. Determine each
test’s success using fixture stdout and exitCode instead of requiring stderr to
be empty.
- Around line 142-170: Remove the explicit 30_000 timeout arguments from the two
setInterval tests. Adjust the fixture workload or scheduling around the
“setInterval canceling with unref...” and “setInterval doesn't leak memory”
tests so both complete within the default timeout without per-test overrides,
while preserving their assertions and behavior.
🪄 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: b5700d5f-de0f-48a4-9996-1b94dcba214a
📒 Files selected for processing (4)
test/js/web/timers/setInterval-fixture.jstest/js/web/timers/setInterval-leak-fixture.jstest/js/web/timers/setInterval.test.jstest/js/web/timers/setinterval-cancel-fixture.js
There was a problem hiding this comment.
LGTM — test-only speedup with strictly tighter assertions.
Checked that bunEnv sets BUN_DEBUG_QUIET_LOGS=1, so expect(stderr).toBe("") is safe (and matches test/CLAUDE.md's own examples). Verified timers-fixture-unref.js emits nothing on success. The filename case fix (setinterval-cancel-fixture.js) matches what's actually on disk. The now-live refreshed setInterval test and the reduced leak thresholds are backed by the PR's empirical runs; ruled out the 30_000 timeouts as a regression — they replace equal-or-longer timeouts that were already there.
Extended reasoning...
Overview
This PR touches four test-only files under test/js/web/timers/: three subprocess fixtures (setInterval-fixture.js, setInterval-leak-fixture.js, setinterval-cancel-fixture.js) and the test file that drives them. No production code is modified. The goal is to cut wall-clock time (53-57s → 8-11s under debug+ASAN) by shrinking fixture workloads while keeping the same failure modes, and to replace bare .toRun() exit-code checks with concurrent Bun.spawn calls that assert stdout/stderr content. Along the way it fixes a test (refreshed setInterval should not reschedule again) whose body returned synchronously and never awaited its interval — the assertions inside were dead code — and clears a second interval that the clearInterval test previously leaked for the rest of the file.
Security risks
None. These are timer test fixtures that spawn the local bunExe() with hardcoded fixture paths and bunEnv. No network, no untrusted input, no auth/crypto surface.
Level of scrutiny
Moderate — test-only, but changes leak-detection thresholds and enables a previously-dead timing-based test, both of which carry flake risk. I reviewed each threshold change against the PR's stated measurements: the leak fixture's flat 50 MB RSS threshold sits between the measured 7-17 MB (no leak, debug+ASAN) and 133+ MB (simulated leak), and the new timeoutCount > 500 check gives 2.5× headroom over one live batch of 200. The revived refresh() test's 180 ms boundary sits between the two expected outcomes (~100 ms if refresh doesn't reschedule, ~200 ms if it does); the spinloop is wall-clock-bounded so preemption doesn't inflate the loop itself. The author reports 10 consecutive passing full-file runs under debug+ASAN.
Other factors
expect(stderr).toBe("")was flagged by finders as potentially violating the "don't assert stderr empty" guidance, butbunEnvintest/harness.tssetsBUN_DEBUG_QUIET_LOGS=1and stripsBUN_DEBUG_*, andtest/CLAUDE.md's own subprocess-test examples use exactly this assertion. Ruled out.- The two
30_000timeouts are not new — the old leak test used!isWindows ? 30_000 : 90_000and the old cancel test used30_000, so this is a net reduction. Ruled out as a violation of the no-timeouts rule. - The old test referenced
setInterval-cancel-fixture.js(capital I) but the file on disk has always been lowercase; the new reference matches the real filename. - The PR description mentions ASAN detection via
bun:internal-for-testing, but the final diff instead removes ASAN detection entirely in favor of a single 50 MB threshold — a simplification in the follow-up commit that the description didn't catch up with. The code is what matters and the flat threshold is justified by the measurements. - No CODEOWNERS entry covers these files, and there are no outstanding human review comments.
There was a problem hiding this comment.
LGTM — test-only speedup with tighter assertions and two real fixes (a previously-vacuous test now awaits, and a pre-existing timing flake is corrected).
What was reviewed:
- Leak fixture workload reduction is compensated by the new
objectTypeCounts.Timeout <= 500check, which is a more direct retention signal than RSS; the 50 MB RSS bound sits well between the measured no-leak (2-18 MB) and simulated-leak (133+ MB) deltas. - Delay fixture's switch to cumulative
N*deltatiming correctly handles catch-up ticks after event-loop stalls; the old per-tick-gap check was a genuine flake source. - Verified fixture filename casing (
setinterval-cancel-fixture.js) and thattimers-fixture-unref.jsproduces no stdout, so the new exact-output assertions match. - The
refreshed setIntervaltest previously returned without awaiting — its assertions were dead code; now properly awaited with cleanup on both paths.
Extended reasoning...
Overview
This PR touches four files, all under test/js/web/timers/: three subprocess fixtures and the test file that drives them. No runtime code is modified. The goal is to reduce wall-clock time (53-57s → 7-11s under debug+ASAN) while tightening assertions. Along the way it fixes two pre-existing correctness issues in the tests themselves: (1) the "refreshed setInterval should not reschedule again" test set up a callback and returned immediately without awaiting, so its throw statements never ran; (2) setInterval-fixture.js compared each tick against the previous tick's timestamp, which false-positives when the event loop stalls and a legitimate catch-up tick fires with a near-zero gap.
Security risks
None. Test-only changes to timing/leak fixtures with no external I/O beyond spawning the local bunExe().
Level of scrutiny
Low-to-moderate. This is test infrastructure with no effect on shipped behavior; the worst-case failure mode is a test that becomes less sensitive to a regression or flakes in CI, both of which surface quickly. The judgment calls (batch counts, RSS threshold, timer counts) are all documented with measured before/after numbers in the PR description, and the leak test's primary signal is now the direct heapStats().objectTypeCounts.Timeout count rather than RSS heuristics — a strictly better detector for the JS-side retention this test targets. The 5,000-timer cancel workload still exercises the same invariant (first fire cancels all before any second fire) as 50,000 did.
Other factors
- The bug-hunting system found no issues.
- Both CodeRabbit findings (stderr emptiness, per-test timeouts) were addressed with reasoned responses and withdrawn; the resolved threads' reasoning holds up against
test/CLAUDE.md(which shows the exactexpect(stderr).toBe("")pattern) and the file already had two explicit timeouts onmain— this PR relocates rather than adds them, and drops the Windows-specific 90s override. - The subprocess pattern (
await using+Promise.allon stdout/stderr/exited, stderr asserted before exitCode) matches the harness conventions exactly. - I verified
timers-fixture-unref.jswrites nothing to stdout (onlymustCallfrom node/test/common, which reports failures via stderr/exit code), soexpect(stdout).toBe("")is correct there. - The PR was verified across 15 consecutive debug+ASAN runs and 20+ release runs per the description and inline thread responses.
|
CI on a4e16ec (build #81861): This diff is green; needs a maintainer merge once the agent backlog clears. |
|
Data point for this one from the x64-asan lane on 2026-08-13: |
1 participant
test/js/web/timers/setInterval.test.jsis one of the slower files in the suite: 24s wall time ondebian 13 x64-asanin build #80083, and on a local debug+ASAN build the leak test actually hits its 30s timeout.What changed
Fixtures
setInterval-leak-fixture.jsdid 50 warmup + 300 measurement batches, each creating 1,000 timers and running 10,000 ticks. Reduced to 3 + 20 batches of 200 timers with 1,000 ticks, and the per-timer payload switched from a ~200-byte.repeat(50)string to a 32 KiBBuffer.alloc. Retention is now asserted viaheapStats:protectedObjectTypeCounts.Timeoutmust be absent (unchanged from before)objectTypeCounts.Timeoutmust be<= 500(one live batch is 200; a leak of cleared timers shows as thousands)RSS is kept as a backstop for native-side leaks with a single 50 MB bound (no-leak delta measured at 2-18 MB over 20 release and 20 debug+ASAN runs; a simulated leak of retained timers measures 133-143 MB). The ASAN-specific threshold and the
isASANdetection that supported it are gone.setInterval-fixture.js: 100 ticks at 16 ms reduced to 25; dropped the per-tickconsole.log. The early-fire check is now against each tick's scheduled time (N * deltafrom start) instead of the gap since the previous tick. The old check was a pre-existing flake on main: if the event loop stalls past the interval, the catch-up tick legitimately fires with a near-zero gap and the fixture reported that as an early fire (reproduced in 3 of 30 standalone runs of the original fixture on a loaded host).setinterval-cancel-fixture.js: 50,000 timers + a 1M-element arg array, reduced to 5,000 + 100k. The test asserts the first fire can cancel every scheduled interval before any of them fires twice; 5,000 still exercises that.Test file
.toRun()matcher, which only checks the exit code and blocks serially. They now useit.concurrent+Bun.spawnand assertstderr === ""and the expectedstdoutbefore the exit code.refreshed setInterval should not reschedule againnever ran. The body set up a 100 ms interval and returned immediately (reported as2msin CI) without awaiting it, so thethrowstatements inside the callback were dead code. It now awaits three fires and asserts withexpect(), clearing the timer on both success and failure.clearIntervaltest cleared its first interval but left the second one running for the rest of the file; now cleared.async setIntervalnow asserts the finalremaining === 0instead of only relying on the promise resolving.performance.now() - start > 9is nowtoBeGreaterThanOrEqual(9)for a readable failure message.30_000from the cancel test and the Windows-only90_000from the leak test (both sized for the old workload); kept30_000on the leak and unref fixtures only, since those approach the 5s default under debug+ASAN with concurrent subprocesses.Why this is safe
No behaviour under test was removed or skipped. Each reduction keeps the same failure mode: a single post-
clearIntervalcallback still fails the cancel fixture; a single protectedTimeout, more than one batch of liveTimeoutobjects, or a >50 MB RSS growth still fails the leak fixture; and a single tick firing before its scheduled time still fails the delay fixture. The new stdout/stderr assertions are strictly tighter than the bare exit-code checks they replace.Timings (
bun bd test test/js/web/timers/setInterval.test.js, debug+ASAN, Linux x64)Release build: ~1.0s total.
[stamp-90s] gate passed · iteration 2 · 4 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file