test: warm up the import() file-path leak fixture and shorten its loop - #37562
test: warm up the import() file-path leak fixture and shorten its loop#37562robobun wants to merge 1 commit into
Conversation
esm-fixture-leak-small.mjs ran 100k import() loads to get a leak signal that cleared the 5-10 MB of heap growth a 5-load warmup left inside the measurement. On the 4 vCPU Linux CI agents that loop takes 20-27s of the test's 30s budget while it shares the machine with the other fixtures in require-cache.test.ts, and on a slow agent it timed out on all four attempts. Warm up for 2k loads so the baseline is taken at steady state, then measure 40k loads against a 20 MB bound. A non-leaking build measures 0-7 MB (Linux x64, 20 runs; Windows x64, 10 runs), the 1 KB per load leak this fixture exists for measures 40 MB or more, and the fixture takes about a third of the time.
|
Warning Review limit reached
Next review available in: 12 minutes 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 (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — well-measured de-flake that shrinks the workload instead of raising the timeout, and the tighter 20 MB bound actually improves the signal-to-threshold ratio over the old 100 MB bound.
What was reviewed:
- Confirmed the new bound still catches the target leak: ~1 KB/load × 40k = 40+ MB vs the 20 MB threshold (better margin than the old 100-120 MB vs 100 MB).
- Checked that the ASAN branch is unchanged and the measured 240 MB non-leaking ASAN growth stays under the 500 MB bound.
- Verified
require-cache.test.tsis the only consumer of this fixture and its 30s timeout is left alone.
Extended reasoning...
Overview
This PR tunes a single test fixture, test/cli/run/esm-fixture-leak-small.mjs, which is spawned by the "via import()" case in require-cache.test.ts. The change increases warmup from 5 to 2,000 loads, cuts the measured window from 100k to 40k loads, and tightens the non-ASAN RSS-growth threshold from 100 MB to 20 MB. The ASAN threshold (500 MB) is unchanged. No production code is touched.
Security risks
None. This is a test fixture that measures RSS growth of repeated import() calls; it takes no external input and touches no security-sensitive paths.
Level of scrutiny
Low. Test-only, single file, no logic beyond loop counts and a threshold constant. The relevant review criteria are the de-flaking rules: don't weaken the assertion the test protects, shrink workload rather than raise timeouts, and keep RSS bounds well below the unfixed leak while branching on ASAN. All three are met — the new design has a ~2× signal-to-bound margin (40+ MB leak vs 20 MB bound) versus the old ~1-1.2× (100-120 MB vs 100 MB), so sensitivity is preserved or improved. The historical context from the removed comment (bun 1.0.0 leaked ~1 KB/load) is retained in the new comment.
Other factors
The PR description includes measured data across Linux release, Windows release, and Linux debug+ASAN for both non-leaking and synthetically-leaking builds, plus wall-clock timings that reproduce the CI failure mode. The fixture has exactly one caller (require-cache.test.ts:331), which is unmodified — consistent with the description's stated intent to leave per-test timeouts alone. Total work (2k + 40k = 42k loads) is well under the previous 100k, so the debug+ASAN lane also gets faster (426s → 189s per the description).
2 participants
Failure
test/cli/run/require-cache.test.tson the:alpine: 3.23 x64lane of build 91764 (#32020, at a commit that only touches http code), red on all four attempts:Cause
The test spawns
esm-fixture-leak-small.mjs, which did 100kimport()loads of a one-line module and failed if RSS grew by 100 MB or more. The loop is that long because the fixture only warmed up for 5 loads, so the 5-10 MB the heap and allocator grow by on the way to steady state landed inside the measurement, and the leak signal had to be large enough to clear it.100k loads cost 8-18s on a release build by themselves.
require-cache.test.tsis adescribe.concurrentblock whose other fixtures are also CPU bound, and the CI agents have 4 vCPUs, so in CI this fixture runs at a fraction of a core. From the job logs of five main builds between Aug 5 and Aug 11, the file takes 20.8-25.2s on the alpine x64 lane (27.3s on debian x64 in build 91764). Running the file locally pinned to 2 cores reproduces that file time and this fixture finishes at about 0.7 of it, which puts it at roughly 15-18s of its 30s budget on a normal alpine run. The agent that ran build 91764 was about twice as slow (the file took 41-52s on each attempt) and the fixture timed out every time. The other fixtures in the file have 60s budgets; this one has 30s (60s on Windows).Fix
Warm up for 2,000 loads before taking the baseline, then measure 40,000 loads against a 20 MB bound. The ASAN bound is unchanged at 500 MB (the quarantine dominates that number). Per-test timeouts are untouched.
Numbers
Non-leaking build, RSS growth inside the measured window:
The leak this fixture exists for retained about 1 KB per load (the removed comment recorded 100-120 MB over 100k loads on bun 1.0.0), which is 40 MB or more over the new window. Retaining a 1 KB string per load on purpose measures 42-74 MB on Linux and 56-75 MB on Windows against the 20 MB bound. The old design caught the same leak with a 100-120 MB signal against a 100 MB bound.
Time:
via import()took 15.6s before and 8.9-9.0s after; file time unchanged at 20-22s since the 60s-budget fixtures now set itThe test passes before and after this change on a healthy machine by design; the change only moves the fixture off the timeout edge, so there is no fail-before proof to give.