test(require-cache): gate the long-export-names import() leak fixture on mimalloc page growth - #34159
test(require-cache): gate the long-export-names import() leak fixture on mimalloc page growth#34159robobun wants to merge 4 commits into
Conversation
After #34009 moved JSC onto mimalloc, pages freed by gc(true) are not necessarily returned to the OS before the next instruction; mimalloc's idle sweep is rate-limited. The two await-import leak fixtures read RSS immediately after a single gc(true), so on macOS 26 arm64 the 'via import() with a lot of long export names' case now reports 70-97 MB against a 64 MB threshold and fails. Measured on macOS 26 arm64 against a post-#34009 release build: the diff plateaus at ~65 MB and stays flat across 1000+ further iterations, so it is allocator retention, not a leak. (1.3.13 on the same hardware actually kept growing past 130 MB over the same extended run; the first 250 iterations just happened to land below the line.) Match the settledRss() pattern #34009 introduced for the sibling leak tests (min RSS over 5 gc+50ms rounds), and raise the non-ASAN bound for the long-export-names import() case from 64 to 128 MB. The source-code leak this test guards against is ~290 MB (250 iterations x 1.17 MB), so the new bound still catches it with margin.
WalkthroughChangesRequire-cache memory leak tests
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:56 PM PT - Jul 19th, 2026
❌ @robobun, your commit 92d4661 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34159That installs a local version of the PR into your bun-34159 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only de-flake following the settledRss() pattern already merged in #34009.
What was reviewed:
- Confirmed
settledRss()matchesserve-response-stream-sink-leak-fixture.ts:41verbatim; fixtures are inline template strings so can't share the helper. - Threshold raise 64→128 MB still catches the guarded regression (v1.1.21 measured 423 MB, theoretical leak ~290 MB); observed max 74 MB gives ~1.7x headroom.
- Added 2×250ms sleep per fixture is well within the existing 60s test timeout.
Extended reasoning...
Overview
Test-only change to test/cli/run/require-cache.test.ts. Two inline await import() leak-detection fixtures now sample RSS via a settledRss() helper (min of 5 rounds of gc(true) + Bun.sleep(50)) instead of a single gc(true) + immediate read, and the non-ASAN threshold for the long-export-names case is raised from 64 MB to 128 MB. No production code touched.
Security risks
None. Test fixture code only; no auth, crypto, network, or user-input handling changed.
Level of scrutiny
Low. This is a CI de-flake with a clear root cause (#34009 moved JSC onto mimalloc; mimalloc's idle sweep is ~100ms rate-limited, so an immediate post-gc RSS read races it). The fix copies the exact settledRss() helper #34009 already introduced in serve-response-stream-sink-leak-fixture.ts — this is applying an established, already-reviewed pattern to two more fixtures. The PR description includes detailed measurements showing bounded plateau (~65 MB flat over 1000+ iterations) vs the pre-#34009 unbounded growth, and 25/25 passing runs on the affected lane.
Other factors
- The threshold raise is well-justified per CLAUDE.md's de-flaking guidance ("keep asserting the property the original assertion protected"): the guarded leak is ~290 MB of retained transpiled output (v1.1.21 measured 423 MB), so 128 MB still catches it with wide margin.
- The
Bun.sleep(50)loop is acceptable here despite the general "don't sleep in tests" rule: mimalloc's sweep is time-gated with no observable completion event, and taking the min across 5 rounds is polling for a floor rather than sleep-then-check. Same pattern already merged. - Added ~500ms per fixture; existing 60s timeout has ample headroom (base runtime ~4s on M1 release).
- The two sibling
require()fixtures are intentionally left unchanged with stated reasoning (already at 100 MB / already.todoon macOS CI). - Fixtures are inline template literals written to temp dirs, so the helper can't be imported from harness — the duplication is unavoidable.
|
CI on build 75878 at 92d4661: both Remaining red is unrelated to this test-only diff:
Ready for a maintainer. |
|
Additional data point from a separate sweep (builds 74512..75788, 21 hits on darwin arm64): On
|
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Do not add sleep!
Per review: no Bun.sleep(). Revert the sibling fixture to its original shape and for the long-export-names import() case, raise the warmup from 50 to 250 iterations so baseline is captured at mimalloc's working-set plateau rather than mid-climb. Verified on macOS 26 arm64 with a post-#34009 release binary (25 runs, 0-46 MB, median ~6); the 128 MB bound stays as headroom over allocator variance and is still well under the ~290 MB a real source-code leak would produce.
|
Dropped the sleep. The change is now just the one fixture:
Verified on On whether this is a regression: steady-state RSS for this specific loop is ~158 MB on 16c5576 (last commit before #34009) and ~200-225 MB on cc0c1e8 (after), so +~65 MB of allocator page retention. |
robobun
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cli/run/require-cache.test.ts`:
- Around line 177-180: Update the warmup-loop comment above the 250-iteration
loop to remove the stale 50-iteration rationale and state that 250 iterations
establish the baseline after mimalloc’s working set settles.
🪄 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: 43700724-da4a-49d8-98e3-420d38a48a00
📒 Files selected for processing (1)
test/cli/run/require-cache.test.ts
There was a problem hiding this comment.
No issues found — the threshold adjustment is well-justified and the new 128 MB bound still catches the guarded leak (~290 MB source leak, 423 MB v1.1.21 regression) with clear separation. Deferring to a maintainer since this relaxes a memory-leak guard and implicitly accepts the ~65 MB steady-state RSS increase from #34009 as allocator retention rather than a regression; that's a judgment call worth a human sign-off.
Reviewed: warmup 50→250 matches the measured round so baseline sits at the plateau; verified the 60s test timeout comfortably covers the extra 200 warmup iterations (sibling fixtures do 400-500 iters in the same budget); the three sibling fixtures were checked and none share the ESM-long-export-names shape that triggers the JIT/allocator retention, so scoping to this one fixture is reasonable.
Extended reasoning...
Overview
Test-only change to one fixture in test/cli/run/require-cache.test.ts ("via import() with a lot of long export names"): warmup iterations 50→250 to match the measured round, and non-ASAN RSS-diff threshold 64→128 MB. Comments updated to document the #34009 mimalloc plateau. No production code touched.
Security risks
None. Pure test threshold/iteration-count adjustment in a memory-leak regression fixture; no new inputs, no external I/O changes.
Level of scrutiny
Moderate. Mechanically trivial, but it is relaxing a safety net — CLAUDE.md is explicit that when de-flaking, the change must keep asserting the property the original assertion protected. The PR does this well: the original v1.1.21 regression was 423 MB and leaking the transpiled source is ~290 MB/250 iters, so a 128 MB bound still catches both with >2x separation. The 25-run verification on the failing hardware (0-46 MB, median ~6) plus the additional sweep data (max 79 MB) gives the new bound reasonable headroom. heapStats().heapSize and objectCount being flat before/after #34009 is good evidence this is allocator page retention, not a JS-object leak.
Other factors
- The extra 200 warmup iterations fit within the existing 60s timeout — the neighboring fixtures already do 100+400 or 50+500 iterations of similar work under the same budget, and the PR reports ~4s on M1 release for comparable loops.
- Only this fixture is touched; the three siblings (require() long exports, import()/require() function calls) don't share the 10k-ESM-export shape that generates the large JIT'd module body the sweep data attributes the retention to, so leaving them alone is defensible.
- The one thing I can't sign off on: whether the underlying +~65 MB steady-state RSS from moving JSC onto mimalloc (#34009) is acceptable, or whether it warrants investigation before the test is loosened. The PR author presents good evidence it's benign (converging not growing, heapStats flat, JIT-off drops it to ~10 MB), but that's a maintainer call — and the PR itself says "Ready for a maintainer."
heapStats({ dump: true }) walks the live page list, so its count is
independent of OS page reclamation and the allocator's idle sweep.
macOS 26 arm64 (post-#34009, 25 runs): -10 to +26 pages; retaining the
module namespaces is +2666. Linux release: -1; Linux debug+ASAN: 0.
robobun
changed the title
|
Switched to
Bound is 100; no RSS threshold touched, no sleep. Separately: |
The mimalloc heap walk is vacuous under cfg(bun_asan) (global allocator is System and MI_MALLOC_OVERRIDE is off, so SourceProvider storage goes through system malloc). Keep the original 320 MB RSS bound there; use the live-page count on every other build.
There was a problem hiding this comment.
LGTM — the ASAN fallback in 92d4661 addresses my earlier concern; test-only de-flake with a solid positive control.
Checked that heapStats({ dump: true }).mimallocDump.heaps[*].pages is a real array-valued API (covered by test/js/bun/jsc/heapStats-mimalloc.test.ts), and that the ${isASAN} interpolation produces a valid boolean literal in the generated fixture. The 320 MB ASAN RSS bound matches the pre-PR value, so no coverage is lost on that lane.
Extended reasoning...
Overview
Test-only change to one fixture in test/cli/run/require-cache.test.ts ("via import() with a lot of long export names"). Swaps the leak-detection metric from an RSS diff to a mimalloc live-page count via heapStats({ dump: true }).mimallocDump.heaps, keeping the original 320 MB RSS bound under isASAN where mimalloc is bypassed. No production code touched; the three sibling fixtures in the same describe block are unchanged.
Security risks
None. This is a spawned test fixture that imports a temp file in a loop and reads process/heap stats. No network, no auth, no user input.
Level of scrutiny
Low. It is a CI de-flake for a known post-#34009 macOS 26 arm64 failure, backed by a root-cause analysis (allocator page-retention plateau, not a real leak — heapSize/objectCount flat across 2000 iterations) and a positive control (+2666 pages when namespaces are deliberately retained vs. a 100-page bound), so the new metric demonstrably still catches the guarded regression. The main risk with leak-test recalibrations — silently making the assertion vacuous — was the subject of my prior inline comment for the ASAN lane, and 92d4661 restores the exact pre-PR RSS check there.
Other factors
I verified the mimallocDump.heaps[*].pages shape against test/js/bun/jsc/heapStats-mimalloc.test.ts, which asserts heaps is a non-empty array and each entry has an array-valued pages — so livePages() will not throw. The ${isASAN} template interpolation yields true/false (harness exports it as a boolean), producing well-formed JS in the fixture. All prior review threads (CodeRabbit's stale-comment note, my ASAN-vacuity note) are resolved by the current revision. CI on the previously-red darwin lane passed per the thread.
|
#37586 converts all four fixtures in this describe block (including this one) to the same kind of measurement, summed as live bytes rather than a page count, against the current version of the file (this branch predates #36194 and #36429). The require() function-calls sibling has the same failure on alpine x64 (builds 84503, 85969, 87643, 88433), with the cause written up there. If #37586 lands, this PR can be closed. |
2 participants
Follow-up to #34009; fixes
test/cli/run/require-cache.test.tsgoing red on the:darwin: 26 aarch64lane (builds 72796, 72798, 72816, 72819, 72827, 72828, 72836).Failure
70-97 MB observed against a 64 MB RSS-diff threshold, macOS 26 arm64 only.
Cause
The fixture warms up for 50 iterations, reads RSS, runs 250 more, and compares. After #34009 moved JSC onto mimalloc, steady-state RSS for this loop on macOS 26 arm64 is ~200-225 MB (vs ~155-160 MB on the commit before #34009), reached after ~200-250 iterations. The 50-iteration warmup captures baseline below that plateau, so the measured diff is the climb to steady state.
heapStats().heapSize(0.62 MB) andobjectCount(~2132) are identical at every checkpoint on both builds out to 2000 iterations, so no JS objects leak; the extra RSS is allocator page retention.Fix
Measure mimalloc's live page count from
heapStats({ dump: true }).mimallocDumpinstead of RSS. The heap walk is independent of OS page reclamation and the idle sweep, so it reads the same whether or not the sweep has fired. UnderisASAN(whereSourceProviderstorage goes through system malloc and is invisible to the mimalloc walk) keep the original 320 MB RSS bound.On macOS 26 arm64 against a post-#34009 release binary (25 runs, 10 solo + 15 at 3x concurrent): page-diff -10 to +26; retaining the module namespace objects on purpose is +2666 over the same 250 iterations. Linux release: -1. Pre-#34009 macOS: 0. Bound is 100.
No sleep; no change to the other fixtures.
Verification
Side note:
heapStats().mimalloc.pages.current(the aggregate stat counter, not the heap walk) goes negative after a few hundred iterations on post-#34009 builds (45 -> 72 -> -125 -> -534); the fixture uses the heap walk for that reason.no test proof · iteration 4 · Platform-specific test-only change; deferring to CI.