test(require-cache): measure allocator-live bytes instead of RSS in the source code leak fixtures - #37586
test(require-cache): measure allocator-live bytes instead of RSS in the source code leak fixtures#37586robobun wants to merge 4 commits into
Conversation
…leak fixtures
The four "don't leak the output source code" fixtures compared RSS before
and after the measured loop. Each load of index.js creates a ~2 MB CodeBlock
metadata table that is reported to the GC while collection is deferred, so
whether a collection runs between loads depends on concurrent JIT timing,
and mimalloc keeps freed pages mapped for a while afterwards. On a loaded
machine the require() fixture reported RSS diffs of 60-180 MB against a
64 MB bound with nothing retained.
Measure the bytes mimalloc currently has handed out (heapStats({ dump: true }))
instead, through one prelude/check shared by the four fixtures. A retained
copy of the transpiled output per load shows up as 44 MB or more; the noise
floor is about 3 MB; the bound is 16 MB. ASAN builds allocate through the
system allocator and keep the previous RSS bounds.
Drop the todoIf on the require() function-calls fixture: the macOS and
musl-aarch64 failures it named were RSS readings of the same kind.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesThe require-cache leak tests now use shared live-memory measurement with RSS fallback. The require, await-import, and long-export-name fixtures use measured baselines. The function-call Require cache leak tests
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced on a busy glibc x64 box with a release build (the unchanged fixture reported RSS diffs of 65-130 MB in 3 of 20 runs, 8 of 15 with a build running alongside). Fix is in this PR (test only). CI was green on every test lane for the first two commits (builds 92405 and 92426); the previously todo'd alpine aarch64 lane printed a 505 MB RSS diff next to a 6.4 MB live diff in the first run, which is the failure mode this replaces and what moved the bound to 24 MB. The later commits make the fixture verify that the allocator walk actually sees JSC allocations in the build under test and fall back to the RSS comparison where it does not (ASAN, or a WebKit built without USE_MIMALLOC), and document the probe; CI for the current head (69194d4) is build 93079. Per-lane numbers are in the description; review threads are resolved. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. Since it swaps the leak-detection metric (RSS → mimalloc live bytes) across four fixtures, tightens the threshold to 16 MB, and un-skips the macOS / musl-aarch64 todoIf, a maintainer sign-off on the methodology and threshold is still worthwhile.
What was reviewed:
- Confirmed
heapStats({ dump: true }).mimallocDump.heaps[].pages[].{block_size, used}matches the shape intest/js/bun/jsc/heapStats-mimalloc.test.ts. - Verified the shared prelude's
require("bun:jsc")is fine in the ESM fixtures (they already userequire.resolve/require.cache). - Checked the
${isASAN}interpolation and that ASAN RSS bounds (400/320) are preserved unchanged; removed imports (isCI,isMacOS,isMusl) are no longer referenced.
Extended reasoning...
Overview
Test-only change to test/cli/run/require-cache.test.ts. The four "don't leak the output source code" fixtures previously each inlined a near-identical RSS-delta check; this PR extracts a shared leakFixturePrelude / leakFixtureCheck() and switches the non-ASAN metric from RSS to a sum of block_size * used over heapStats({dump:true}).mimallocDump. The test.todoIf(isBroken && isCI && (isMacOS || (isMusl && isArm64))) on the require()/function-calls fixture is removed, and the now-unused isCI/isMacOS/isMusl imports are dropped.
Security risks
None. No runtime code, no external inputs, no network — the fixtures spawn bunExe() against a temp dir exactly as before.
Level of scrutiny
Moderate. It's test-only, but it is not mechanical: it changes the leak-detection methodology, lowers the effective bound from 64–100 MB RSS to 16 MB live bytes, and un-skips a test on two CI lanes the author did not directly re-run on (verification was on a busy glibc x64 host). The root-cause analysis (DeferGCForAWhile + concurrent-JIT-driven collection timing + mimalloc deferred purge) and the 60-run / simulated-leak verification are thorough and convincing, and the mimallocDump shape is confirmed against the existing heapStats-mimalloc.test.ts. Still, a maintainer should agree that mimalloc live bytes is the right long-term signal for this test class and that 16 MB is the right bound.
Other factors
The PR states it supersedes #34159; a human should confirm that PR gets closed. The de-duplication into a shared helper is exactly what REVIEW.md asks for, and the removed per-version RSS comments are summarized in the new shared comment. No prior human review comments to address.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
#34159 is related rather than a duplicate: it converts one fixture in this block (import() with long export names, for the macOS failures) and is based on the file before #36194 and #36429. This PR was opened for the require() function-calls fixture, which fails the same way on alpine x64, and converts all four fixtures in the block to one shared check, so it supersedes #34159 (noted there as well). |
…h64 noise floor The first CI run showed the concurrent JIT holding up to 6.4 MB of the last load's compilation state on the aarch64 lanes at the second measurement (3.5 MB on x64). The smallest leak these fixtures catch adds 44 MB.
There was a problem hiding this comment.
LGTM — focused test-only de-flake with a well-traced root cause and simulated-leak verification.
What was reviewed:
heapStats({ dump: true }).mimallocDump.heaps[].pages[].{block_size, used}shape matches the existing test intest/js/bun/jsc/heapStats-mimalloc.test.ts.- The shared prelude uses
require("bun:jsc")inside ESM-shaped fixtures — fine in Bun, and those fixtures already relied onrequire.resolve/require.cache. - The 24 MB live-bytes bound sits between the stated noise floor (~6.5 MB aarch64) and the smallest simulated leak (~44 MB); ASAN keeps the pre-existing RSS bounds unchanged.
- Removed
todoIfmaps directly to the RSS artifact this change eliminates; unusedisCI/isMacOS/isMuslimports dropped, remaining imports still referenced.
Extended reasoning...
Overview
Test-only change to test/cli/run/require-cache.test.ts. The four fixtures in the "don't leak the output source code" block previously asserted on RSS delta after gc(true), which is noisy because mimalloc keeps freed pages mapped and GC timing depends on the concurrent JIT thread. The PR extracts a shared leakFixturePrelude (defines liveBytes() summing block_size * used over every mimalloc page, plus measure()) and a shared leakFixtureCheck(asanRssMB) that fails on >24 MB of live-byte growth (or the pre-existing RSS bound under ASAN, where mimalloc is bypassed). It also removes the test.todoIf(isBroken && isCI && (isMacOS || (isMusl && isArm64))) on the require()/function-calls fixture and drops the now-unused harness imports.
Security risks
None. Test infrastructure only; no runtime, network, filesystem-permission, or crypto surface touched.
Level of scrutiny
Moderate for a test change, because (a) it un-skips a test on two CI lanes and (b) it swaps the leak-detection signal. Both are well-justified: the root cause is traced into JSC (CodeBlock::finishCreation + DeferGCForAWhile + concurrent-JIT-driven collection timing + mimalloc's deferred purge), the new signal was validated against simulated leaks (44–935 MB, all caught), and the noise floor was measured (≤6.5 MB) with the bound widened to 24 MB in the follow-up commit. Per REVIEW.md's leak-test guidance the bound is well below the unfixed leak and branches on isASAN. The de-duplication into shared prelude/check is exactly the "extract a named helper the second time a block appears" rule.
Other factors
I confirmed the mimallocDump API shape against test/js/bun/jsc/heapStats-mimalloc.test.ts (heaps → pages → block_size/used). The prelude's require("bun:jsc") runs inside two ESM-shaped fixtures (export default ...), which is fine in Bun and consistent with the pre-existing require.resolve/require.cache usage in those same fixtures. The description still says 16 MB while the code says 24 MB — that's the aarch64-widening follow-up commit, not a discrepancy in the check. The author noted they're waiting on the macOS and musl-aarch64 CI lanes to confirm the removed todoIf; that's a CI-gate rather than a code-review question.
|
The description has been updated for the second commit (bound is 24 MB in both now, with the per-lane numbers from build 92405 that motivated it). CI for 8c08df7 is build 92426. |
… trusting it
Whether JSC allocates through the mimalloc heaps that heapStats({ dump: true })
walks depends on the WebKit build: release prebuilts do, ASAN builds and WebKit
built without USE_MIMALLOC (the debug prebuilts, local builds) do not. On the
latter the live-bytes comparison reads 0.0 MB no matter what is retained.
Instead of keying the fallback on isASAN, the prelude allocates a flat 32 MB JS
string and checks that the walk saw it; if not, the fixture compares RSS at the
bound those builds were already using and says so in its output. The check
also prints the absolute live figures next to the diff.
There was a problem hiding this comment.
LGTM — well-researched test de-flake that replaces a noisy RSS delta with an allocator-live-bytes check, backed by simulated-leak verification and per-lane CI data across two green builds. The one inline nit is minor and doesn't block.
Extended reasoning...
Overview
Test-only change to test/cli/run/require-cache.test.ts. The four "don't leak the output source code" fixtures are converted from comparing RSS before/after to summing block_size * used over heapStats({ dump: true }).mimallocDump, with a shared leakFixturePrelude / leakFixtureCheck() replacing four copies of the measurement boilerplate. A runtime probe (walkSeesJSC) allocates a 32 MB flat JS string and checks whether the heap walk sees it; where it doesn't (ASAN, WebKit built without USE_MIMALLOC) the fixture falls back to the same RSS bounds those builds already used. The todoIf(isMacOS || (isMusl && isArm64)) on the require()/function-calls test is removed along with the now-unused isASAN/isCI/isMacOS/isMusl imports.
Security risks
None. Test infrastructure only; no src/ changes, no external inputs, no new dependencies.
Level of scrutiny
Moderate — this de-flakes a leak regression test and removes a todoIf, so the key question is whether it still catches the leak it guards. The description answers that directly: simulated leaks (retaining one readFileSync copy of index.js per load, retaining module.exports, retaining the module namespace) all fail the new 24 MB bound at 44–935 MB, and the description notes the old RSS check would have reported 50–59 MB for the smallest of those, under its 64 MB bound. So the new check is tighter on the target regression while being resistant to the RSS noise the mechanism section explains (DeferGCForAWhile + concurrent-JIT-driven GC timing + mimalloc page retention). The 24 MB bound is 3.7× the largest live-diff residue seen across two full CI runs (6.4 MB on alpine aarch64) and less than half the smallest simulated leak.
Other factors
- The prelude uses
require("bun:jsc")and top-levelconstdeclarations that are spliced into both CJS (exports.abc = 123) and ESM (export default 123) fixture shapes;requireis available in Bun ESM and the description confirms both shapes were exercised (including the ASAN fallback branch). - The RSS fallback uses the old ASAN bounds (400/320 MB) for any build where
walkSeesJSCis false. That's correct because release builds have routed JSC through mimalloc since #34009 (so they take the 24 MB live-bytes branch), and the probe verifies this at runtime rather than assuming it — the third commit added exactly this self-check. - Removed harness imports were verified against the final file:
isArm64,isBroken,isIntelMacOS,isWindowsare still used; the removed four are not. - Two green CI runs (92405, 92426) with per-lane live/RSS numbers in the description; the previously-
todo'd alpine-aarch64 lane printed a 505 MB RSS diff next to a 6.4 MB live diff, which is the exact failure mode being replaced. - The inline nit about
"a".repeat(32 * MB)is convention-only; the probe deliberately wants a JSC StringImpl allocation (per the comment above it) and runs once per fixture in a file that is already too slow for debug builds regardless of this change.
There was a problem hiding this comment.
LGTM — the .repeat() probe is now commented in the fixture source (69194d4), which addresses the nit from the earlier pass.
What was reviewed:
- Confirmed the new 24 MB live-bytes bound still catches the target leak (simulated at 44+ MB) while the old 64 MB RSS bound would not — this tightens rather than weakens the assertion.
- Checked that the RSS fallback bounds (400/320) match what the ASAN branch used before, so the fallback path is behaviorally unchanged for the lane that hits it.
- Verified the removed harness imports (
isASAN,isCI,isMacOS,isMusl) are no longer referenced and the remaining ones still are. - Checked
require("bun:jsc")in the shared prelude works in the two ESM fixtures — the description confirms both CJS and ESM extractions pass.
Extended reasoning...
Overview
Test-only change to test/cli/run/require-cache.test.ts. The four fixtures in the "don't leak the output source code" describe block previously measured RSS delta between two gc(true) calls; this replaces that with a mimalloc live-bytes walk (heapStats({ dump: true }).mimallocDump, summing block_size * used per page). A runtime probe (32 MB flat JS string) determines whether the walk sees JSC allocations in the build under test; where it does not (ASAN, non-mimalloc WebKit), the fixture falls back to the RSS comparison at the same bounds the ASAN branch already used. The four copy-pasted fixture bodies are deduplicated into a shared leakFixturePrelude string and leakFixtureCheck() helper. The todoIf on the require()/function-calls test is removed since the flakiness it excused is what this fixes.
Security risks
None. Test-only; no src/ changes, no user-facing surface, no external I/O beyond spawning bunExe() on temp-dir fixtures (unchanged from before).
Level of scrutiny
Medium. This is more than a mechanical tweak — it changes the measurement methodology for leak detection — but it is confined to test fixtures, has been through two green CI runs (builds 92405, 92426) with per-lane live/RSS numbers tabulated in the description, and the worst-case failure mode is a flaky test rather than a production defect. The key REVIEW.md concern for de-flaking ("keep asserting the property the original assertion protected") is explicitly addressed: the description shows simulated leaks fail the new check at 44–935 MB against the 24 MB bound, and notes the old 64 MB RSS bound would have missed the smallest of those (59/50 MB reported). The bound is 3.7× the largest CI-observed noise floor (6.4 MB on alpine aarch64).
Other factors
- My earlier nit about
"a".repeat(32 * MB)vsBuffer.alloc().toString()was answered with empirical data (Buffer#toString registers 0.0 MB in the walk even on release, so the swap would break the probe) and a code comment was added in 69194d4. The thread is resolved; the delta since my last review is just that comment. - The removed
todoIfun-skips the test on musl-aarch64 and macOS — a strengthening. The alpine aarch64 lane is shown passing (6.4 MB live diff next to a 505 MB RSS diff, which is exactly the failure mode being replaced). macOS is covered only by the mechanism argument since the pipeline currently has no macOS test lanes; the description says so explicitly. - The one loosening is that non-ASAN builds with a non-mimalloc WebKit (debug prebuilts, local WebKit) now get the 400/320 MB RSS fallback instead of the old 100/64 MB RSS bound. That configuration is not in CI, and the runtime probe means the fixture at least reports which check it applied rather than passing vacuously.
- Removed imports checked:
isASAN/isCI/isMacOS/isMuslare gone from the file body;isArm64/isBroken/isIntelMacOS/isWindowsare still used.
1 participant
test/cli/run/require-cache.test.ts> "files transpiled and loaded don't leak the output source code" > "via require() with a lot of function calls" intermittently fails on the alpine 3.23 x64 lane and passes on retry (main builds 84503, 85969, 87643, 88433):The other builds reported 94, 104 and 112 MB against the fixture's 64 MB bound. This is the same failure the fixture's
todoIfalready excused on macOS and musl-aarch64 ("RSS reports ~280 MB"), and the same one #34159 saw on macOS for the import() long-export-names sibling.Cause
Nothing is retained; RSS is the wrong signal for these loops. It reproduces on glibc x64 with a release build whenever the machine is busy (3 of 20 and 4 of 10 runs in two batches here, 8 of 15 with a build running in the background).
Every load of the 20k-call
index.jscreates a CodeBlock whose metadata table is ~1.9 MB.CodeBlock::finishCreationreports it withreportExtraMemoryAllocatedwhileScriptExecutable::prepareForExecutionImplholds aDeferGCForAWhile, so the request is only recorded, and almost nothing else in the loop allocates through a path that re-checks it. In practice the collection gets kicked off when a concurrent JIT plan foriis installed on the main thread. When the JIT thread lags, no GC runs for 30 to 60 loads and the dead CodeBlocks pile up at ~2.3 MB per load;BUN_JSC_logGC=1shows stretches with no collection ending inbytes allocated this cycle: 119104488 exceed bytes allowed: 9147120.gc(true)then frees all of it, but the pages stay mapped past the RSS read (mimalloc purges them ~100 ms later), soRSS diffis whatever the ramp happened to be. WithBUN_JSC_useConcurrentJIT=0the diff is 1 to 5 MB in 20 of 20 runs. Theawait import()siblings yield to the event loop between loads, which is why they rarely hit this.This is not a Bun-side regression to fix in
src/instead: theDeferGCForAWhileand the deferral branch ofHeap::collectIfNecessaryOrDeferare unmodified upstream JSC code, and Bun's own GC scheduling (src/jsc/GarbageCollectionController.rs, both before and after #35356) is driven by event-loop timers, which a synchronousrequire()loop never reaches; nothing in the module loading path ever prompted the GC. The first of the alpine failures (build 84503, 2026-07-29) also predates #35356. What changed recently is only how much of the freed memory stays resident long enough to be read (#34009), which the macOStodofrom 2025 shows was already enough to make RSS flaky before that.Through all of this, the bytes mimalloc actually has handed out (JSC allocates through it too since #34009) stay flat:
heapStats()shows the same object counts after eachgc(true), and the mimalloc heap walk moves by 0 to 3.6 MB on x64. The residue is quantized: in about a quarter of the measurements one load's worth of compilation state (a 2.25 MB block plus a 512 KB and a 320 KB one on x64) is still allocated aftergc(true), with identical JS cell counts either way; it never occurs withBUN_JSC_useConcurrentJIT=0(0 of 24 runs vs 10 of 24 with the default), so it is something the JIT worker thread has not released yet when the measurement is taken. On the aarch64 CI lanes that residue is larger, up to 6.4 MB (see the CI numbers below).Fix
Test only. The four fixtures in that describe block share one prelude and one check:
liveBytes()sumsblock_size * usedover every page of every heap inheapStats({ dump: true }).mimallocDump, and the fixture fails if that grows by more than 24 MB over the measured loop (3.7x the largest residue seen in CI). The smallest leak these fixtures exist to catch, one copy of the transpiled output kept per load, adds 44 MB in the 20k-call fixtures (100 KB x 400 loads) and hundreds of MB in the long-export-name ones; the old RSS check reported 59 and 50 MB for that same leak in the 20k-call fixtures, under its 64 MB bound.Whether JSC's allocations are in the heaps this walk sees is a property of the WebKit build: the release prebuilts are built with
USE_MIMALLOC, while ASAN builds and WebKit built without it (the debug prebuilts, local WebKit builds) use another allocator, and there the walk reads 0.0 MB whatever is retained. So rather than keying onisASAN, the prelude allocates a flat 32 MB JS string (the same kind of allocation as a retained source) and checks thatliveBytes()saw it; where it did not, the fixture compares RSS instead, at the 400 / 320 MB bounds the ASAN lane was already using, and says which check it applied in its output. On ASAN lanes this is the same comparison as before this PR; on non-ASAN builds with a non-mimalloc WebKit it is the RSS check instead of a vacuous pass. The output line also prints the absolute live figures, so a blind walk is visible in CI logs (live 0.0 -> 0.0 MB) and the live baselines per lane can be compared.The other two describe blocks in this file ("don't leak the AST", "don't leak file paths") still compare RSS in their own fixture files. They load many small modules rather than one large one, so they do not build up 2 MB per load, they have not failed on main in the last 60 builds, and one of their fixtures is being changed by #37562, so they are left alone here.
The
todoIfon the require() function-calls test is removed since the readings it excused were this. The per-version RSS figures in the removed comments described the RSS check and are summarized in the shared comment instead. This supersedes #34159, which converts only the import() long-export-names fixture and is based on the file before #36194 and #36429.Verification
Release build on this (busy) machine:
bun test test/cli/run/require-cache.test.tswith the release build: 7 of 7 runs, 11 tests passing each time.readFileSynccopy of index.js per load fails with 44.2 MB (require) / 46.7 MB (import); keepingmodule.exportsper load fails with 303 MB (long names, require) / 900 MB (long names, import); keepingiper load fails with 935 MB."a".repeat(32 MB)registers as +36.0 MB in the walk on the release build and +0.0 MB on the debug ASAN build (Buffer#toString("latin1")would not have worked as a probe: it registers 0.0 even on release).live 3.2 -> 48.2 MB; on the debug ASAN build all three report "allocator walk does not see JSC allocations in this build, checking RSS" and apply the RSS bound.First CI run of this branch (build 92405, every test lane green), live diff per fixture as printed by the four fixtures:
todo)Second run, with the 24 MB bound (build 92426, also green everywhere): largest live diffs were 5.6 MB (ubuntu aarch64), 5.1 MB (windows 11 aarch64) and 2.6 MB (windows x64); everything else was within 0.5 MB. The ~5-6 MB residue shows up on every aarch64 lane regardless of OS, so it is an architecture difference in the size of the per-load compilation state, not a musl one.
The 505 MB RSS reading on alpine aarch64 next to a 6.4 MB live diff is the failure mode this PR is about; that run is what moved the bound from the 16 MB in the first commit to 24 MB. The pipeline currently has no macOS test lanes, so the macOS half of the removed
todoIfis covered only by the mechanism argument.The 557 MB reading on the ASAN lane is the same mechanism on the RSS fallback, which this PR leaves as it was: under ASAN the heap walk sees nothing, so those fixtures still compare RSS (same loops, same 400 / 320 MB bounds as before). It has not failed on main's ASAN lane in the last 60 main builds, so it is not changed here; a live-bytes counterpart for ASAN would need
__sanitizer_get_current_allocated_bytesexposed to the fixture, which is a separate change.The fail-before/pass-after proof does not apply to this change mechanically: there is no
src/change, so the same tree runs both times; the numbers above are the before/after evidence. The file as a whole is too slow to finish under a debug build with these workloads regardless of this change (each load of the 20k-line file takes over a second there), which is pre-existing.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.