test: make the import() AST leak fixture actually evict the module - #38165
test: make the import() AST leak fixture actually evict the module#38165robobun wants to merge 1 commit into
Conversation
esm-bug-leak-fixture.mjs keyed its `delete require.cache[...]` by the value of import.meta.resolve(), which has returned a file:// URL since #5827. The module cache is keyed by absolute path, so the delete never matched and the 200 KB module was transpiled once; the remaining 54 import() calls were cache hits and the RSS check measured nothing. Resolve the key with require.resolve(), like the sibling fixtures, and fail the fixture up front if the key is not present in require.cache after importing, so the same mistake cannot make the test pass vacuously again.
|
Warning Review limit reached
Next review available in: 3 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 |
|
Status: reproduced and fixed, waiting on CI. Reproduced by appending an evaluation counter to a copy of |
There was a problem hiding this comment.
LGTM — test-only fix that brings this fixture in line with its siblings.
What was reviewed:
- Confirmed
require.resolve()+delete require.cache[dest]+await import(dest)matches the pattern inesm-fixture-leak-small.mjsandrequire-cache-bug-leak-fixture.js. - Verified the consuming test (
require-cache.test.ts:307) assertstoEndWith("--pass--"), so the new--fail--guard on a bad cache key correctly fails the test rather than passing vacuously. - Checked that
import(absolutePath)(vs the previousfile://URL) is already exercised by the sibling ESM fixture, so no behavior divergence there.
Extended reasoning...
Overview
This PR touches a single test fixture, test/cli/run/esm-bug-leak-fixture.mjs, which is spawned by the "files transpiled and loaded don't leak the AST > via import()" test in require-cache.test.ts. Two changes: (1) replace await import.meta.resolve(...) with require.resolve(...) so the delete require.cache[dest] actually evicts the module (the previous file:// URL never matched a cache key, so 54 of the 55 loads were cache hits and the RSS check was measuring nothing), and (2) add a post-warmup guard that fails with --fail-- if dest is not present in require.cache, so a wrong key can never silently pass again.
Security risks
None. Test-fixture-only change; no runtime, native, or user-facing code touched.
Level of scrutiny
Low. This is a small mechanical correction to a leak-test fixture that had drifted after import.meta.resolve() semantics changed. The new key is identical to what the three sibling fixtures (esm-fixture-leak-small.mjs, require-cache-bug-leak-fixture.js, cjs-fixture-leak-small.js) already use, and await import(absolutePath) is already the pattern in esm-fixture-leak-small.mjs, so there's no novel behavior being introduced. The added in require.cache guard is a strict tightening — it makes a previously vacuous test able to fail, which is exactly what REVIEW.md asks for ("prove the test fails for the RIGHT reason").
Other factors
The PR description provides direct before/after evidence (1 vs 55 evaluations, timing parity with the require() sibling) and shows the new guard tripping on the old key. RSS bounds are unchanged and the author reports 0–20 MB on release against the 120 MB bound, well within margin. The consuming test asserts stdout.trim().toEndWith("--pass--") and exitCode === 0, both of which the new failure path violates, so the guard is wired correctly. No prior reviews or outstanding comments on the timeline.
|
Updated 5:05 AM PT - Aug 13th, 2026
❌ @robobun, your commit c03cc39 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 38165That installs a local version of the PR into your bun-38165 --bun |
Problem
require.cache > files transpiled and loaded don't leak the AST > via import()(test/cli/run/require-cache.test.ts:298) spawnstest/cli/run/esm-bug-leak-fixture.mjs, which is supposed to load the 200 KBesm-leak-fixture-large-ast.mjs55 times and check that RSS stays flat.delete require.cache[dest]wheredest = await import.meta.resolve(...)(esm-bug-leak-fixture.mjs:3). Since feat(runtime): align import.meta.resolve with node.js's implementation #5827 that is afile://URL string;require.cacheis keyed by absolute path, so the delete never matches anything. The module is transpiled and evaluated once and the other 54import(dest)calls are cache hits, so the RSS check measures nothing. (When the fixture was written in Fix memory leak inrequire#6790,import.meta.resolve()returned a promise of an absolute path, hence theawait. feat(runtime): align import.meta.resolve with node.js's implementation #5827 switchedesm-fixture-leak-small.mjstorequire.resolve()but left this fixture on the old key.)file://key, 55 with therequire.resolve()key. On a debug+ASAN build the test passes in 1.8 s while itsrequire()sibling, which does the same 55 loads of a same-sized module for real, times out at 20 s.Fix
require.resolve(), asrequire-cache-bug-leak-fixture.js,esm-fixture-leak-small.mjsandcjs-fixture-leak-small.jsalready do.--fail--and exit 1 ifdestis not a key inrequire.cache. A key the cache does not use now fails the test instead of passing it; the previousfile://key trips this check on the first run.require.cacheexposes the runtime module cache under the resolved absolute path, which is exactly whatrequire.resolve()returns: afterimport(url),path in require.cacheis true andurl in require.cacheis false, anddelete require.cache[path]followed byimport()evaluates the module again (checked on both the release and the debug build).inanddeleteonrequire.cacheare thehasanddeletePropertytraps insrc/js/builtins/CommonJS.ts:388, and both look up the ES module registry by the same key, so a key thatinfinds is the keydeleteevicts.require()sibling's 169 MB for the same workload, so on therelease-asanlane it should behave like the sibling, which has been running there against the same 400 MB bound.USE_SYSTEM_BUN=1 bun test test/cli/run/require-cache.test.ts: 11 pass;via import()now takes about as long asvia require().bun bd test test/cli/run/require-cache.test.ts -t "leak the AST": before,via import()passes in 1.8 s andvia require()times out; after, both time out the same way, since debug builds run the 200 KB workload at a fraction of release speed. test(require-cache): skip the leak fixtures under debug builds #38148 skips these blocks on debug builds; CI has no debug lane, so no CI lane is affected by that.esm-bug-leak-fixture.mjs. test(require-cache): skip the leak fixtures under debug builds #38148 (require-cache.test.ts), test(require-cache): measure allocator-live bytes instead of RSS in the source code leak fixtures #37586 and test(require-cache): gate the long-export-names import() leak fixture on mimalloc page growth #34159 (inline fixtures in the test file), test: warm up the import() file-path leak fixture and shorten its loop #37562 (esm-fixture-leak-small.mjs) and test: surface ASAN status to leak fixtures via bunEnv #35081 (the ASAN detection lines of the standalone fixtures) change other lines.Background
require.cachein Bun is a view of the runtime's module cache and also lists ES modules (Node only lists CommonJS there). Keys are resolved absolute paths. Deleting a key evicts the module, so the nextimport()orrequire()of that path reads, transpiles and evaluates the file again; that is what lets these fixtures run the transpiler many times in one process and watch RSS.import.meta.resolve()follows Node: it is synchronous and returns afile://URL string. Bun's original version returned a promise of an absolute path, which is what this fixture was written against.Evaluation-count probe and measurements
large.mjsis a copy ofesm-leak-fixture-large-ast.mjswithglobalThis.__evals = (globalThis.__evals ?? 0) + 1;appended; the probe runs the fixture's two loops against it with either key (BUN_RUNTIME_TRANSPILER_CACHE_PATH=0, as the test harness sets):Fixture output with this change, release linux x64, 7 runs:
leaked= 0, 4, 15, 20, 3, 16, 10 MB (bound 120). Debug+ASAN build: esm fixture 179 MB,require-cache-bug-leak-fixture.js169 MB (the fixtures apply the 120 MB bound there because they detect ASAN by thebun-asanbinary name; #35081 is about that).Same fixture with the
file://key put back, to exercise the new check:bun bd test test/cli/run/require-cache.test.ts -t "leak the AST"on main:via import()passes in 1811 ms,via require()times out after 20000 ms. With this change: both time out after 20000 ms.