Conversation
|
Updated 1:38 PM PT - Aug 17th, 2026
❌ @robobun, your commit ea01f7c has some failures in 🧪 To try this PR locally: bunx bun-pr 31772That installs a local version of the PR into your bun-31772 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (7)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 1 remains after this review. WalkthroughChangesThe PR adds a non-blocking full-GC VM API, clears module state during test-isolation global swaps, performs mimalloc cleanup, and adds a Linux RSS regression test for isolated module graphs. Memory Reclamation During Test Isolation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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 `@src/jsc/bindings/ZigGlobalObject.cpp`:
- Around line 636-658: Extract the duplicated module teardown sequence (the
DECLARE_THROW_SCOPE(vm) scope, obtaining moduleLoader, taking WTF::Locker on
moduleLoader->cellLock(), calling moduleLoader->clearAll(), then calling
requireMap()->clear(oldGlobal) and scope.assertNoException()) into a single
helper function (e.g., ZigGlobalObject::teardownModuleState or a static helper)
and replace the duplicate blocks in GlobalObject::reload(),
Zig__GlobalObject__destructOnExit(), and the current location in
ZigGlobalObject.cpp with calls to that helper; ensure the helper accepts the
GlobalObject* (or appropriate vm/context) so it can lock moduleLoader and clear
the requireMap and preserves the
DECLARE_THROW_SCOPE(vm)/scope.assertNoException() behavior.
In `@test/regression/issue/31771.test.ts`:
- Around line 81-84: The test asserts exitCode too early; move the
expect(exitCode).toBe(0) assertion to after the other assertions (after checks
on stderr and peakKb) so diagnostic messages (expect(stderr).toContain(`${n}
pass`), expect(stderr).toContain("0 fail"), and
expect(peakKb).toBeGreaterThan(0)) run first; update the order in the test so
variables stderr, n, peakKb are asserted before exitCode.
🪄 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: a556fc8b-2e59-46e2-a48b-af6a84a46b9c
📒 Files selected for processing (7)
src/jsc/VM.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/bindings.cppsrc/jsc/bindings/headers.htest/cli/test/isolation.test.tstest/regression/issue/31771.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/regression/issue/31771.test.ts (1)
80-84:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert subprocess exit code last in this helper.
expect(exitCode).toBe(0)should come after other assertions so failures show richer diagnostics first.Suggested patch
expect(stderr).toContain(`${n} pass`); expect(stderr).toContain("0 fail"); - expect(exitCode).toBe(0); expect(peakKb).toBeGreaterThan(0); + expect(exitCode).toBe(0); return peakKb / 1024;As per coding guidelines: "Assert the exit code last in tests - this gives a more useful error message on test failure".
🤖 Prompt for 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. In `@test/regression/issue/31771.test.ts` around lines 80 - 84, The exit code assertion (expect(exitCode).toBe(0)) needs to be the last assertion so failures show richer diagnostics: reorder the assertions so you first assert stderr contains `${n} pass`, "0 fail", and peakKb > 0, then assert expect(exitCode).toBe(0) last; ensure you do not return before checking exitCode (e.g., compute and store peakKb/1024 in a variable, run all assertions including expect(exitCode).toBe(0), then return the computed value). Use the existing symbols stderr, n, peakKb, and exitCode to locate and update the test.
🤖 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.
Duplicate comments:
In `@test/regression/issue/31771.test.ts`:
- Around line 80-84: The exit code assertion (expect(exitCode).toBe(0)) needs to
be the last assertion so failures show richer diagnostics: reorder the
assertions so you first assert stderr contains `${n} pass`, "0 fail", and peakKb
> 0, then assert expect(exitCode).toBe(0) last; ensure you do not return before
checking exitCode (e.g., compute and store peakKb/1024 in a variable, run all
assertions including expect(exitCode).toBe(0), then return the computed value).
Use the existing symbols stderr, n, peakKb, and exitCode to locate and update
the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d59041f-278d-4395-9bb8-a1343aeede4f
📒 Files selected for processing (2)
test/cli/test/isolation.test.tstest/regression/issue/31771.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found, but this touches JSC heap/GC timing and module-loader teardown under cellLock across the Rust↔C++ boundary, so it's worth a human pass before merge.
Extended reasoning...
Overview
This PR fixes #31771 by reclaiming each finished test file's module graph during the bun test --isolate global swap. It spans four layers: a new JSC__VM__collectNowFull C++ binding (synchronous full GC that preserves bytecode/source-provider caches), a Rust FFI wrapper, a refactored GlobalObject::clearModuleRegistry() helper now shared by reload(), createForTestIsolation, and destructOnExit, and a forced collect_now_full() + mimalloc_cleanup in swap_global_for_test_isolation. A Linux-only regression test asserts RSS slope < 0.5 MB/file, and an existing heavy isolation test gets an explicit 30s timeout.
Security risks
None identified. This is internal heap-lifecycle management with no user-controlled input reaching the new code paths; the only externally observable effect is lower RSS during --isolate runs.
Level of scrutiny
Moderate-to-high. The implementation is well-reasoned and closely mirrors existing patterns (reload()'s cellLock+clearAll, runGC's finalizeSynchronousJSExecution bracketing), and the refactor into clearModuleRegistry() is a clean consolidation that coderabbit confirmed as addressed. However, this is JSC heap/GC interaction code where mistakes (clearing the registry while the GC thread is visiting it, forcing collection at an unsafe point, or breaking the source-provider cache invariant) would manifest as intermittent crashes or UAF rather than test failures. The cellLock is taken correctly and exception-scope handling is preserved at each call site, but a maintainer familiar with the isolation swap path and JSC's concurrent-GC visiting should confirm the ordering (clear registry → gcUnprotect → collectNowFull) is sound.
Other factors
The PR description is unusually thorough (cause analysis, before/after RSS table, verification against existing isolation and NAPI-finalizer tests). The bug-hunting system found nothing, coderabbit's two nits were addressed, and the regression test is well-designed (slope-based, ASAN-aware threshold). CI shows unrelated musl-LTO build failures and a pre-existing bunx.test.ts flake. Given the change is in core runtime memory management rather than a leaf feature, I'm deferring rather than approving.
|
CI status: the diff is green on every lane that exercises this change.
The red lanes ( I've used my one CI re-roll; the remaining failures are unrelated flake. Flagging for a maintainer to merge. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Dropping the module graph after microtasks and such are run makes sense. Calling synchronous full gc does not.
|
Good call — dropped the synchronous full GC. The swap still drains microtasks and clears the outgoing global's module registry + require cache ( RSS stays flat (release: 50.2 → 51.4 MB at N=1/20 on the 1000-module repro, same as the sync version and matching default mode), and the per-file swap no longer blocks (N=20 runs in ~133ms). Pushed in b6cd1bf. |
|
CI on b6cd1bf (async-GC version): the red lanes are unrelated flaky tests, confirmed by pulling the logs. Failing tests:
None touch the The diff is green where it counts. Flagging for merge past the unrelated flake. |
…info on the sync module path (#31793) ### What Two `bun test --isolate` memory/CPU fixes found while auditing test isolation between the Zig spec and the Rust port: 1. **Leaked handles pinned the outgoing global forever.** A test file that leaks an `fs.watch` handle, a running `Bun.serve`/node:http server, or a long `setTimeout`/`setInterval` used to pin its **entire global object** (module graph, closures, everything) for the rest of the run — memory grew by one full global per such file, exactly the "leaked handles" scenario `--isolate` exists to absorb. 2. **The synchronous module path skipped the isolation source cache's module records.** `require(esm)`/`importSync` passed `module_info: null` (spec: ModuleLoader.zig:423-428/:516-551; only the async `RuntimeTranspilerStore` path had been ported), so cached SourceProviders stayed plain `Module` and every isolated file re-ran JSC's parser over the transpiled source to rebuild the record in its fresh global. ### Reproduction 8 isolated files, each leaking one handle and counting live globals after a forced full GC: ```sh mkdir leak && cd leak for i in $(seq 1 8); do cat > "f$i.test.js" <<'JS' import { test, expect } from "bun:test"; import { heapStats } from "bun:jsc"; import fs from "node:fs"; const w = fs.watch(import.meta.dir, () => {}); // or: Bun.serve({ port: 0, fetch: () => new Response("x") }); test("t", () => { // or: setTimeout(() => {}, 3_600_000); Bun.gc(true); Bun.gc(true); console.log("GLOBALS=" + heapStats().objectTypeCounts.GlobalObject); expect(1).toBe(1); }); JS done bun test --isolate # GLOBALS climbs 1,2,3,...,8 — every old global retained ``` For the sync path: 8 isolated files `require()`ing one 3000-export `.mjs` — every file pays a full JSC re-parse (~1.2s/file debug, ~20% slower loads in release) instead of rebuilding the record from the cache. ### Cause `swapGlobalForTestIsolation` gcUnprotects the outgoing global so its graph becomes collectable — but three handle types kept independent GC roots into it: 1. **fs.watch** — isolation teardown (`closeAllWatchersForIsolation`) calls `FSWatcher.detach()`, which never drops the initial `pending_activity_count = 1` ref. Only `close()` does. `hasPendingActivity()` therefore stays true forever and JSC can never collect the wrapper, whose cached listener closure roots the global. (Stat watchers register `close()` and don't leak.) 2. **Bun.serve** — the swap's socket-group walk closes the listen socket *fd* at the uws layer, but the Server object never learns: `hasListener()` stays true, so its strong `js_value` (fetch handler closure) roots the global. node:http servers go through the same path. 3. **Timers** — generation-stale `TimeoutObject`s only self-cancel when they *fire*; until then the armed timer holds a Strong on its wrapper. A module-scope `setInterval(fn, 3_600_000)` roots its global for an hour — effectively the whole run. And on the sync module path, `module_info` is what flips the cached provider to `BunTranspiledModule` (`ZigSourceProvider.cpp` — "allows JSC to skip parsing during the analyze phase"); without it the cache only dedupes transpiles, not the per-global re-parse. ### Fix All gated on `test_isolation_enabled` / `use_isolation_source_provider_cache()`; zero behavior change outside `--isolate`/`--parallel`: - `FSWatcher::close_for_isolation()` — `close()` minus the `'close'` event (no user JS mid-swap, parity with `StatWatcher::close`); the isolation registry now calls it so the pending-activity ref drops and the wrapper becomes collectable. - Watchers and servers register into a per-VM `IsolationHandles` set on `RuntimeState` (unified `ArrayHashMap` registry, maintainer refactor of the original per-kind `rare_data` registries). `close_isolation_handles()` drains it before each global swap, popping until empty so close/error handlers that register new handles are also swept. Servers register on successful listen, unregister in `stop_listening()`/`deinit()`, and each leaked server is stopped through its real lifecycle (listener close, poll unref, `js_value` downgrade) *before* the blind socket-group walk. - `close_isolation_handles()` drains microtasks first: a microtask still pending at end-of-file can register a handle when it runs, and must land in the registry before it empties (keeps the pre-refactor drain-before-teardown ordering). - `StatWatcher` leaves the registry in `close()` (always the JS thread), not in the refcount destructor: the last deref can land on the work-pool thread, where the thread-local registry is unreachable, so the old placement silently skipped removal and left a dangling pointer that the next file-boundary drain closed. ASAN: `heap-use-after-free ... in StatWatcher::close`, `freed by thread T11 (Bun Pool 0) ... work_pool_callback`. - The swap now invokes the existing `cancel_all_timers` hook (the same sweep `global_exit()` uses) right after bumping the isolation generation, eagerly releasing every `TimeoutObject`/`ImmediateObject`/`AbortSignal.timeout` pin. Internal timers (DNS, BunTest, SQL, …) are untouched by that sweep's tag filter. - Sync module path: fresh transpiles collect the printer's ESM-record analysis (`ModuleInfo::create` + `has_tla`, destroyed on print failure) and attach it to `ResolvedSource` as a deserialized record — which also makes the sync path persist `esm_record` into on-disk RuntimeTranspilerCache entries; disk-cache hits deserialize the stored `esm_record` (`create_from_cached_record`). Mirrors `RuntimeTranspilerStore.rs` exactly. - Test-only: `bun:internal-for-testing` gains `isolatedModuleCacheSourceType(path)` (thin lookup over the per-VM cache) so the sync-path test can assert the provider type deterministically. Complementary to #31772 (which clears the old global's module registry + forces a collection): that PR makes a *collectable* global's graph actually get reclaimed; this one makes pinned globals collectable in the first place. No overlapping hunks beyond both touching `swap_global_for_test_isolation`. ### Verification - New tests in `test/cli/test/isolation.test.ts`: - `--isolate: collects globals pinned by leaked handles` (3 fixtures × 8 isolated files, asserting max live `GlobalObject` ≤ 4): on baseline all three fail with `Received: 8` (every global pinned); with the fix the count plateaus at 2–3. - `--isolate: require(esm) caches a BunTranspiledModule SourceProvider`: asserts the cached provider type for a `require`d ESM module across both the fresh-transpile and disk-cache-hit (`BUN_RUNTIME_TRANSPILER_CACHE_PATH`) branches. Fails on baseline with `"Module"`. - `--isolate: unwatchFile'd watcher freed on the work pool leaves no dangling registry entry`: watchFile until the listener fires (scheduler queue ref exists), then unwatchFile + `Bun.gc` + sleep past a few scheduler ticks so the work pool drops the last ref; without the close()-side unregistration ASAN aborts at the file boundary with the heap-use-after-free above. - Full `isolation.test.ts`: 19/19 pass with the fixes; each new test verified to fail with the matching source change stashed. - Kitchen-sink repro (watch + serve + timers + listeners + dangling promises per file, 14 files): heap flat at 50MB / 3 globals, was +17MB & +1 global per file. - Release-binary measurement for the sync path: per-file load of a 3000-export module drops to async-path parity (~4.6ms vs ~5.7ms; debug-build cost matches the async path's existing module_info profile). - Adjacent suites checked on this container: `parallel.test.ts` fails 4 scheduling-sensitive cases *identically on baseline and fix* (machine-capacity artifacts); `fs.watch.test.ts` fails only the 2 run-as-root permission cases (baseline too); `serve-listen.test.ts` 27/27. ### Peak RSS benchmark (50 isolated test files × 50 shared 1MB modules) Method: 50 `.test.ts` files, each statically importing the same 50 shared modules; each shared module is **~1MB of source** (≈1MB transpiled: 9,000 exported functions + a 64×512-char retained string table), so one file's evaluated import graph is ~50MB of JS and per-global retention is unmistakable in RSS. All contents are salted with a per-generation runtime value and regenerated before every run, and the child runs with `BUN_RUNTIME_TRANSPILER_CACHE_PATH=0`, so the on-disk runtime transpiler cache can never replay entries (the in-memory `--isolate` SourceProvider cache stays active, as in production). Peak RSS = `VmHWM` of the `bun test` process, polled at 25ms; 3 runs each, fresh salt per run. Linux x64, release builds, local. **Clean suite** (no leaked handles): | binary | `--isolate` peak RSS (3 runs) | wall | no `--isolate` (reference) | |---|---|---|---| | bun 1.3.14 (pre-port Zig, system) | 1696 / 1752 / 1694 MB | ~25s | ~1397 MB | | release @ base `a3464c666` (pre-PR) | 1489 / 1476 / 1479 MB | ~23s | ~1182 MB | | release @ PR head `9de5ec654` | 1493 / 1469 / 1477 MB | ~23s | ~1191 MB | Neutral, as expected: static imports load through the async path (which already attached `module_info`) and nothing leaks, so none of this PR's paths fire beyond the per-swap sweeps — which cost nothing measurable. **Leaked-handle suite** — identical fixture, plus one `Bun.serve({ port: 0, ... })` leaked at module scope of every test file (the scenario this PR fixes): | binary | `--isolate` peak RSS (3 runs) | wall | |---|---|---| | bun 1.3.14 (before) | **10912 / 10920 / 10721 MB** | ~38s | | release @ base `a3464c666` (before) | **8502 / 9559 / 8512 MB** | ~36s | | release @ PR head (after) | **2798 / 2789 / 2993 MB** | ~25.5s | Before the fix, every leaked server pins its file's entire ~50MB global graph for the rest of the run — peak RSS grows linearly with file count (≈10.9GB at just 50 files; an OOM at suite scale). With the fix, leaked servers are stopped at each swap, the globals become collectable, and peak RSS drops ~3.5× (and wall time ~33%, from reduced GC pressure). The residual gap over the clean suite (~1.3GB) is the transiently-pinned current/previous global plus GC lag — bounded, not growing with file count. (At a 10KB-module scale the same fixture showed all three binaries within noise of each other on the clean suite — 158-161MB — confirming Zig↔Rust port parity; the 1MB scale is what makes the retention effects dominate RSS.) --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
b6cd1bf to
349b427
Compare
There was a problem hiding this comment.
No bugs found, and my three earlier nits (parallel-worker mimalloc_cleanup parity, the isolation.test.ts timeout regression, and routing WebWorker__teardownJSCVM through clearModuleRegistry()) are all addressed — but this touches JSC GC scheduling and module-loader teardown across four lifecycle paths, so it's worth a human look.
Checked: clearModuleRegistry() takes cellLock() before clearAll() so the concurrent marker in visitChildrenImpl can't race the async full collection it triggers; the new JSC__VM__collectFullAsync mirrors the existing collectAsync shim's JSLockHolder pattern.
Checked: createForTestIsolation clears the registry while oldGlobal is still gc-protected, and requireMap() is a plain internal JSMap so scope.assertNoException() after clear() is not weaker than the RETURN_IF_EXCEPTION used by reload().
Checked: the Worker.cpp and destructOnExit refactors are byte-equivalent to the prior inline sequence; runner.rs now matches the serial loop's pre-swap mimalloc_cleanup.
Extended reasoning...
Overview
The PR fixes #31771 (bun test --isolate peak RSS grows linearly with file count) by (1) clearing the outgoing global's module-loader registry and require cache before unprotecting it in Zig__GlobalObject__createForTestIsolation, and (2) requesting an async full collection after each swap so the detached graph is reclaimed between files. Along the way it extracts the shared cellLock() + moduleLoader->clearAll() + requireMap()->clear() sequence into GlobalObject::clearModuleRegistry() and routes all four call sites (reload(), createForTestIsolation, destructOnExit, WebWorker__teardownJSCVM) through it. It also adds a new C ABI JSC__VM__collectFullAsync (Rust wrapper VM::collect_full_async), inserts mimalloc_cleanup(false) before the swap in the --parallel worker loop for parity with the serial loop, and adds a Linux-only RSS-slope regression test.
Security risks
None identified. No user-controlled input reaches the new code paths; the change is confined to internal test-runner lifecycle and GC scheduling.
Level of scrutiny
High. This is production runtime code on a critical path: JSC heap collection scheduling, module-loader registry teardown under cellLock() (which must stay ordered against the concurrent marker's visitChildrenImpl), and worker-VM teardown. An earlier revision used a synchronous collectNow(Sync, Full) and was changed to collectAsync(Full) mid-review — that design choice (async vs. sync, and whether clearing the registry while the old global may still be transiently reachable is always safe) is exactly the kind of judgment call a maintainer familiar with JSC's GC should confirm. The exception-handling choice at the new createForTestIsolation site (assertNoException vs. the RETURN_IF_EXCEPTION used by reload()) also warrants a human eye, though it looks correct given requireMap() is a plain internal JSMap.
Other factors
- All three of my prior inline comments were addressed (0822f2f, 53c14f3, 1e41fba); the PR thread shows CodeRabbit's nits were also resolved.
- The regression test asserts an RSS slope (< 0.5 MB/file across a 10-file delta) rather than an absolute value, which is the right shape for a memory-leak test and should be robust across build variants; it is Linux-only via
test.skipIf(!isLinux)since it reads/proc/<pid>/statusVmHWM. - The
Worker.cppanddestructOnExithunks are pure dedup (byte-equivalent to the prior inline sequence);runner.rsis a one-line parity addition. - CI on the ASAN lane (which exercises
isolation.test.tsand the new regression test) passed per the author's status comments; the remaining red lanes are documented as unrelated flake.
Given the scope (GC internals across four teardown paths) and the mid-review design pivot from sync to async collection, this should be approved by a human maintainer rather than auto-approved.
|
Correction to my earlier flake assessment: the ASAN-lane failures on the last two builds were a real bug in this PR, now fixed in c0c1382. The x64-asan lane runs with Fix: open a Rust-side Verification: deterministic repro with |
There was a problem hiding this comment.
No issues found in the latest revision — all three earlier inline comments are addressed (parallel-worker mimalloc_cleanup parity, the isolation.test.ts timeout revert, and routing WebWorker__teardownJSCVM through clearModuleRegistry()), and c0c1382's top_scope! wrapper around create_for_test_isolation looks correct against TopExceptionScope's contract. Deferring to a maintainer given this touches JSC GC scheduling, module-loader teardown across four lifecycle paths, and the Rust/C++ exception-scope boundary.
Checked: clearModuleRegistry() is now the single implementation for all four teardown sites (grep found no remaining open-coded moduleLoader->clearAll() + requireMap()->clear() sequences); JSC__VM__collectFullAsync takes the JSLock before heap.collectAsync, matching sibling JSC__VM__collectAsync; the new top_scope! guard is scoped so it drops before self.global is reassigned; and test/cli/test/isolation.test.ts is no longer in the diff (revert confirmed).
Extended reasoning...
Overview
The PR fixes #31771 (bun test --isolate retaining each file's module graph, causing linear RSS growth) by (1) adding GlobalObject::clearModuleRegistry() and calling it on the outgoing global in Zig__GlobalObject__createForTestIsolation before gcUnprotect, (2) requesting an async full GC after each swap via a new JSC__VM__collectFullAsync binding, (3) refactoring three pre-existing teardown sites (reload(), destructOnExit, WebWorker__teardownJSCVM) to use the same helper, (4) adding mimalloc_cleanup before the swap in the --parallel worker for parity with the serial loop, and (5) wrapping the FFI call in a Rust-side TopExceptionScope so BUN_JSC_validateExceptionChecks=1 doesn't abort on the next file. A Linux-only regression test asserts the peak-RSS slope stays under 0.5 MB/file across a 500-module graph.
Security risks
None. No user-controlled input reaches the changed code; the new surface is an internal GC-request binding and an internal helper method. The regression test reads /proc/<pid>/status for a subprocess it spawned — no path traversal or privilege concerns.
Level of scrutiny
High. This is JSC heap/GC and module-loader-lifecycle code, and the change fans out to four teardown paths (watch-mode reload, per-file isolate swap, worker-VM teardown, process-exit teardown). Getting the ordering wrong (clearing the registry while a concurrent marker walks it, or missing the exception-check obligation across the FFI boundary) is exactly the kind of thing that manifests as rare ASAN-only crashes — and indeed one such bug (the validateExceptionChecks abort) surfaced and was fixed mid-PR in c0c1382. The async-vs-sync full-GC decision and whether collectAsync(Full) per file is the right latency/RSS trade-off are design calls a maintainer should confirm.
Other factors
- All prior review threads (CodeRabbit's helper-extraction and exit-code-ordering nits; my three inline comments on mimalloc parity, the timeout regression, and the missed fourth dedup site) are resolved and reflected in the current diff.
- I verified
crate::top_scope!andTopExceptionScope::assert_no_exceptionexist and behave as the c0c1382 comment describes; the scope is opened againstself.global()(the old global, still valid) and drops beforeself.globalis overwritten, so no dangling reference. JSC__VM__collectFullAsyncacquiresJSLockHolderbefore callingheap.collectAsync, matching the neighbouringJSC__VM__collectAsyncshim's pattern.- The regression test is Linux-only (
/procVmHWM), uses a slope metric with generous headroom (0.5 MB/file vs. ~0.06 observed), drains stderr concurrently withproc.exited, and usesawait usingon the subprocess — reasonable for a memory regression test. - CI on the latest commit is still building (#71957); prior builds passed on the ASAN lane that exercises this path most heavily.
c0c1382 to
b01bbb2
Compare
There was a problem hiding this comment.
No further findings on b01bbb2 — all three earlier nits (parallel-worker mimalloc_cleanup parity, the isolation.test.ts timeout hunk, and the fourth clearModuleRegistry caller in WebWorker__teardownJSCVM) are addressed, and the new top_scope!/assert_no_exception wrapper around create_for_test_isolation looks correct for the validateExceptionChecks issue. Deferring to a human because this touches JSC module-loader/GC teardown and exception-scope handling across the Rust↔C++ boundary.
What was reviewed:
clearModuleRegistry()extraction and all four call sites (reload,createForTestIsolation,destructOnExit,WebWorker__teardownJSCVM) — cellLock ordering and per-caller scope handling preserved.JSC__VM__collectFullAsyncbinding +swap_global_for_test_isolationplacement (runs after the global pointer is swapped andVMHolderupdated; both serial and parallel callers now precede it withmimalloc_cleanup).- The RSS-slope regression test — Linux-gated, polls
VmHWMwith a slope threshold rather than an absolute, drains stderr concurrently withexited.
Extended reasoning...
Overview
This PR fixes #31771: bun test --isolate retained each finished file's module graph across the per-file global swap, so peak RSS grew linearly with file count. The fix (1) clears the outgoing global's ESM module-loader registry and CJS require map before gcUnprotect, (2) requests an async full collection after the swap, and (3) adds mimalloc_cleanup to the parallel-worker per-file loop for parity with the serial loop. It extracts the shared teardown sequence into GlobalObject::clearModuleRegistry() and routes four callers through it (reload, isolate swap, process-exit teardown, worker-VM teardown). A new JSC__VM__collectFullAsync C ABI + Rust binding is added. A Linux-only regression test asserts the RSS-per-file slope stays under 0.5 MB/file.
Nine files touched: ZigGlobalObject.{cpp,h}, Worker.cpp, bindings.cpp, headers.h, VM.rs, VirtualMachine.rs, parallel/runner.rs, and the new test.
Security risks
None identified. No user-controlled input reaches the new code paths; the changes are internal GC/teardown plumbing on the test-isolation swap. No auth, crypto, network, or filesystem-path handling is involved.
Level of scrutiny
High — human review warranted. This is native memory-management code at the JSC boundary: module-loader registry clearing under cellLock() (races the concurrent marker), forced full collections, gcUnprotect ordering, and JSC exception-scope validation across an FFI call. The exception-check fix in b01bbb2 (opening a Rust TopExceptionScope around create_for_test_isolation because the C++ side's DECLARE_THROW_SCOPE dtor sets m_needExceptionCheck) is subtle and was only caught by the ASAN CI lane. The refactor also changes WebWorker__teardownJSCVM, which runs on the worker-thread teardown path. These are exactly the areas CLAUDE.md flags as most-blocked (GC rooting, exception checks after every JS-entering call, thread affinity), so a maintainer familiar with the isolate-swap and worker-teardown lifecycles should sign off.
Other factors
- All three of my prior inline comments were addressed in follow-up commits (0822f2f, 53c14f3, 1e41fba); the diff now contains no
isolation.test.tschange (reverted per feedback) and the parallel worker has cleanup parity. - The bug-hunting system found no issues on the current revision.
- The author's own CI analysis identified and fixed a real ASAN-lane failure (exception-check validation) after initially misreading it as flake — the fix looks correct but reinforces that this area is delicate.
- Test coverage: the new regression test measures slope (robust across build types) and the existing
isolation.test.tssuite exercises the swap path; ASAN CI covers the exception-check validator.
|
Investigated the duplicate report #36815 (closed in favor of #31771) and confirmed this PR's approach with two extra data points that may be useful:
Minor, unrelated to the RSS growth: one gcProtect'd Symbol leaks per realm under |
|
Not stuck, the work is done. Current state:
What it needs: a maintainer re-review, since the changes-requested state from the earlier synchronous-GC version is still standing and I cannot dismiss it. |
b01bbb2 to
6dd445d
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…leRegistry Centralize the cellLock() + moduleLoader->clearAll() + requireMap()->clear() sequence shared by reload(), the --isolate global swap, and process-exit teardown so the paths can't drift. Each caller keeps its own exception policy. Also assert the subprocess exit code last in the regression test so stderr/RSS diagnostics surface first on failure.
Per review: dropping the outgoing global's module graph on the --isolate swap is correct, but forcing a synchronous full collection per file stalls the event loop. Keep the registry/require-cache teardown (clearModuleRegistry) and replace collectNow(Sync, Full) with a non-blocking collectAsync(Full) so the concurrent collector reclaims the old global's heap between files. Peak RSS stays flat (release: 50.2 -> 51.4 MB at N=1/20 on a 1000-module graph, matching default mode) and the per-file swap no longer blocks (N=20 completes in ~133ms).
Gives --parallel workers the same per-file RSS hygiene as the serial --isolate loop, and makes the swap's comment about freed blocks accurate for both callers.
…default The explicit 30s timeout predates the rebase; main now sets setDefaultTimeout(isASAN ? 120_000 : 30_000) for this file, and a per-test timeout overrides it, shrinking the ASAN budget from 120s to 30s. Revert to the file-level default.
Fourth copy of the cellLock + clearAll + requireMap clear sequence, in WebWorker__teardownJSCVM, now uses the shared helper. Also report the child's stderr tail, exit code, and signal in one combined assertion in the 31771 regression test so a failed child is fully diagnosable from CI output.
Zig__GlobalObject__createForTestIsolation declares a ThrowScope (around clearModuleRegistry); its destructor runs simulateThrow(), so with JSC exception-check validation enabled (the x64-asan CI lane), the next ThrowScope (the next file's loadAndEvaluateModule) aborted with 'exception check validation failed' (SIGABRT), killing the --isolate child and --parallel workers mid-run. Open a Rust-side TopExceptionScope across the create_for_test_isolation call and query it afterward, same as the generated check_slow wrappers. Reproduced deterministically with the validateExceptionChecks JSC option on a release-asan build; 10/10 isolate and 5/5 parallel runs pass after. Note: the previous version of this message had a wrapped line beginning with the literal option assignment, which the Windows CI env plumbing interpreted as a real JSC environment variable and failed every child process on every Windows shard.
- assert_no_exception() is debug/asan-only, so the release build-bun lanes failed to compile (no method on TopExceptionScope in release). Use assert_no_exception_except_termination(), which exists in all build configs, still queries the scope to satisfy the asan exception-check validator, and tolerates a pending worker-termination exception. - Shorten the explanatory comments flagged by the source-lint comment cap.
…comments - Route GlobalObject::forbidExecution() (VM/worker teardown) through the shared clearModuleRegistry() helper instead of its inline cellLock + clearAll + requireMap clear copy. Behavior-equivalent (verified: worker terminate tests behave identically with and without this change). - Collapse the explanatory comments flagged by the source-lint comment cap to single lines.
Per test/CLAUDE.md and REVIEW.md: reduce LARGE_N 14 -> 10 to cut child wall-clock, and drop the per-test timeout 90s -> 30s. A memory-slope test that spawns two child bun test --isolate runs transpiling a 500-module graph under debug+ASAN can't fit the default budget without shrinking the graph below the fail-before discrimination floor, so it keeps an explicit timeout as the root CLAUDE.md 'rare outlier'.
proc.exited resolves only after the child is reaped, so /proc/<pid> is gone by then (and a zombie's status has no Vm* lines anyway); the poll loop already captures the monotonic peak while the child runs.
8e8401f to
d89f273
Compare
Matches the serial per-file loop's shape exactly instead of duplicating the call in both arms.
There was a problem hiding this comment.
I reviewed the current revision (ea01f7c) and found no issues — all earlier nits are addressed. Given this touches JSC GC lifetime paths (module-registry teardown under cellLock(), gcUnprotect ordering, collectAsync(Full), exception-scope balancing across the FFI boundary) and there's a standing changes-requested from an earlier revision, a maintainer re-review is still the right call.
What was reviewed:
clearModuleRegistry()extraction — confirmed all three sites (reload(),forbidExecution(), isolation swap) route through it with caller-owned exception scopes.collect_full_async→heap.collectAsync(CollectionScope::Full)underJSLockHolder; non-blocking, does not delete code blocks or clear source-provider caches.- Parallel worker loop now matches the serial loop's shape (unconditional
mimalloc_cleanuphoisted above the isolation branch). - Regression test: slope-based VmHWM assertion, Linux-only skip, workload shrunk to fit 30s; dead post-exit procfs read removed.
Extended reasoning...
Overview
This PR fixes #31771: bun test --isolate retained each finished file's module graph across the per-file global swap, causing peak RSS to grow linearly with file count. The fix has two parts: (1) in Zig__GlobalObject__createForTestIsolation, clear the outgoing global's module loader registry and require cache before gcUnprotect (via a new shared GlobalObject::clearModuleRegistry() helper, also adopted by reload() and forbidExecution()); (2) in swap_global_for_test_isolation, request a non-blocking full collection via a new JSC__VM__collectFullAsync binding. A Rust-side top_scope! was added around the FFI call to balance the C++ ThrowScope for the exception-checks validator. The parallel worker loop was reshaped to match the serial loop's unconditional mimalloc_cleanup. A Linux-only regression test asserts the RSS-per-file slope stays under 0.5 MB/file by polling child VmHWM.
Security risks
None identified. No user-controlled input reaches the changed paths; the module-registry teardown and GC scheduling are internal test-runner lifecycle operations. The cellLock() around moduleLoader->clearAll() mirrors the existing pattern in reload()/forbidExecution() and pairs with the GC thread's visitChildrenImpl.
Level of scrutiny
High. This is JSC GC lifetime code: mutating the module loader's registry maps under cellLock(), dropping a permanent GC root, scheduling a full collection on the GC thread, and threading exception scopes across a Rust↔C++ FFI boundary. REVIEW.md flags native memory safety as "the most-blocked category," and a maintainer (Jarred-Sumner) previously requested changes on the synchronous-GC version — that review state is still standing per the PR thread. The change is well-motivated and the implementation mirrors existing teardown paths, but bot approval is not appropriate here.
Other factors
The PR has been through many review rounds; every automated finding (including several from prior runs of this reviewer) has been addressed and marked resolved. The regression test is slope-based rather than absolute-threshold, which is the right shape for a leak test. CI is reported green on the lanes that exercise this path. The remaining blocker is the human re-review the author already flagged as needed.
|
CI on ea01f7c: 178 jobs passed, including both asan shards and every lane that exercises this change. The single red job is a darwin-aarch64 agent that stalled before running any test (53 log lines, no test output, cancelled by the job timeout) - an infra stall, not a test failure. Ready for re-review. |
|
Superseded by #39818 — on current main the retention is the global's own Strong-held mock/plugin registries (any file using |
|
Sounds good. #39818 covers the module-registry drop at the swap and also fixes the Strong-held mock and plugin registry roots, which this PR did not reach. Closing in favor of that one. |
… mocks and plugins (#39818) ### What Closes #31772. Fixes #31771. Under `bun test --isolate` each file runs in a fresh `GlobalObject` inside one VM. Two pieces of state that live *on the global itself* were held through `JSC::Strong` handles: - `JSMockModule::activeSpies` / `activeMocks` — created by the first `mock()` / `jest.fn()` / `spyOn()` in a file - `onLoadPlugins` / `onResolvePlugins` (+ virtual modules) — populated by `Bun.plugin()` and `mock.module()` A Strong root → realm object → Structure → global cycle is uncollectable, so any file that created a mock or registered a plugin kept its entire global and everything it imported alive for the rest of the run. That is the linear growth people hit on real suites. ### Changes - `activeSpies` / `activeMocks` become `WriteBarrier`s visited from `JSMockModule::visit` (already called by `GlobalObject::visitChildren`). - The isolation swap clears the outgoing global's plugin lists (`OnLoad::clear()` now also drops virtual modules; `Bun.plugin.clearAll()` shares it) and resets the VM's cached `plugin_runner`, which pointed at whichever global first registered a plugin and would dangle once that global is collectable. - The swap drops the outgoing global's module registry and `require.cache` right after microtasks/handles are drained, instead of whenever the old global happens to be collected. `reload()` and `forbidExecution()` share the same `clearModuleRegistry()` helper. No forced GC of any kind. ### Numbers Release builds of `main` vs this branch, same machine, interleaved runs. 1000-module shared graph imported by every file; "mock" files additionally do `const fn = mock(() => 1)`. | files | `main --isolate` (mock) | this PR `--isolate` (mock) | `main` no isolate | |---|---|---|---| | 10 | 227 MB | 152 MB | | | 20 | 426 MB | 190 MB | | | 40 | 810 MB | 152 MB | | | 80 | 1591 MB | 215 MB | 59 MB | Wall time at N=80: 1.95s → 1.46s. Live `GlobalObject` count after `Bun.gc(true)` in the Nth file (release): `main` = N, this PR = 1–3. For the mock-free repro in the issue, current `main` already plateaus (finished globals are collectable there); `main` and this branch measure the same (~140/225/225/310 MB at N=10/20/40/80). What remains above shared-global mode in that case is generational GC pacing — each file's graph is promoted to old space while the file runs and is only reclaimed by the next full collection — which this PR deliberately does not force. ### Tests Two new cases in `test/cli/test/isolation.test.ts` under "collects globals pinned by leaked handles": 8 isolated files that each create mocks / register a plugin, assert live `GlobalObject` ≤ 4. Both fail on `main` (8) and pass here. Existing isolation, mock, and plugin suites pass on the debug+ASAN build, including with `BUN_JSC_validateExceptionChecks=1`.
### Problem - GitHub closes only the first reference after a keyword, so "Fixes #1, #2" leaves #2 open. "Supersedes #3" links nothing, and no reference closes a pull request. - The last 1000 merged PRs name 274 such references. PR #32292 is open although merged #36135 says "Supersedes #32292". ### Fix - `.github/workflows/close-linked-issues.yml` runs on `pull_request_target` `closed` (a merge into the default branch of `oven-sh/bun`) and on `workflow_dispatch` with a PR number and `dry_run`. Everything is inline in one `actions/github-script` step, with no checkout. - Each open target is closed as `completed` with the comment "Closed as completed by #N." or "Superseded by #N.". Closed or missing targets, the PR itself and other repositories are skipped. - The parser has no regex. A closing keyword (close, fix, resolve, supersede, replace, any tense) must lead the reference, alone or in a list. A negated, hedged or noun keyword, or one whose subject is another reference, does not count ("may fix", "the rm fix #1", "#100 supersedes #1"). - Verified: `test/internal/close-linked-issues.test.ts` (333 cases) runs the YAML's script against fake `github`, `context` and `core`. Also the 1000-PR parse (Notes). ### Background - GitHub's own keywords are close, fix and resolve (-s, -ed). Each links one reference, and only a merge into the default branch closes it. - `pull_request_target` runs in the base repository with a write token, also for fork PRs. That is safe only when no PR-controlled code runs. Here the description is the only PR input, parsed as text. <details><summary>Notes</summary> A close through the API does not create the "closed this in #N" timeline link that GitHub makes for its own closes. The comment carries the PR number instead. How the parser was calibrated. I pulled the descriptions of the last 1000 merged PRs and listed every line with a keyword next to a reference. The keyword families, list shapes and reference forms in the script are the ones that appear there. A reference is `#1`, `owner/repo#1`, an issue or pull URL (bare or in `<>`), or a markdown link. Four lines would have been wrong with a plain keyword-then-reference rule, and each led to a rule: - "the open `rm` fix #37521" (#38379): "fix" as a noun. Base forms (fix, close, resolve, supersede, replace) count only at the start of a sentence or line, or after will, should, does, and, and a few similar words. "to" is not one of them ("unable to fix #1", "how to fix #1"). - "May also fix #12318 / #10046, untested" (#38242): hedged. may, might, could, would, partially and the negations disqualify the keyword, looking past adverbs such as "also". - "Supersedes the closed #26040" (#36289) and "a comment on closed #35351" (#35365): "closed" as an adjective. A determiner or preposition before the keyword disqualifies it. - "supersedes #33130's optimisation" (#35843): a number that continues into a word is not a reference. Review added: a reference before the keyword is the subject ("#100 supersedes #1"), also through "which" or "that" ("reverts #100, which fixed #1") and across a removed span ("#100 ~~also~~ fixes #1"). A hedge two words before the keyword disqualifies it ("hopefully this fixes #1", "could this fix #1?"). A clause that starts with if, when, once, until or unless is not a statement. The tokenizer keeps a line break as a token so that "Fixes #1" on one line and "Fixes #2" on the next stay two statements. Code spans, fences, indented code, blockquotes, HTML comments and strikethrough are skipped. The block stripping follows CommonMark for fences (also inside a blockquote), indented code, blockquotes with lazy continuation, setext underlines and HTML comments, and GFM for `~~` flanking. Result over the 1000 descriptions: 274 distinct references in 135 PRs. I checked the current state of all of them through GraphQL. All but one are closed (202 issues completed, 5 duplicates, 66 pull requests). The one open target is PR #32292, superseded by merged #36135. No open target is a false positive. Every review change kept this result. Patterns that are deliberately not handled: a bulleted list under "Closes:" on its own line (not seen in the sample), references separated by whitespace only ("#1 #2"), "fix for #1", and GH-1 style references. A `?` after the list is not treated as a question. The block parser tracks no list containers, so a second paragraph of a list item indented by four spaces is read as an indented code block and skipped. A removed span or inline comment reads as one word, so "Fixes <!-- n --> #1" finds nothing. The test suite covers: the phrases above, stopping at the right place in real sentences, CRLF descriptions, URLs with fragments or a `/files` suffix, case-insensitive `Owner/Repo#1`, the fake API where a lookup, an update or a comment fails, the `dry_run` input, an invalid `pr_number` input, an unmerged PR, a PR merged into a non-default branch, the merge event body against a later edit, and a description with no closing statement. The first revision of this PR checked out the repository and ran `scripts/close-linked-issues.ts`. Jarred asked for no checkout and no script file, so the script moved inline into the workflow and the test now reads it out of the YAML. </details> <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 9 · 2 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/internal/close-linked-issues.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/internal/close-linked-issues.test.ts bun test v1.4.1 (4448a2e) test/internal/close-linked-issues.test.ts: (pass) finds "Fixes #39852" [176.21ms] (pass) finds "Closes #31772. Fixes #31771." [22.28ms] (pass) finds "- Fixes #39930" [12.28ms] (pass) finds "Fixes: #30429" [10.46ms] (pass) finds "FIXES #1" [7.86ms] (pass) finds "(Fixes #1)" [8.97ms] (pass) finds "**Fixes #1**" [10.20ms] (pass) finds "__Fixes #1__" [9.83ms] (pass) finds "_Fixes #1_" [11.25ms] (pass) finds "Fixes **#1**" [9.72ms] (pass) finds "**Fixes** #1" [7.13ms] (pass) finds "**Fixes:** #1" [8.11ms] (pass) finds "Fixes #1 and **#2**" [11.47ms] (pass) finds "Fixes **#1**, **#2**" [9.13ms] (pass) finds "## Why (fixes #13771, closes #30543)" [16.08ms] (pass) finds "Closes #11418" [19.46ms] (pass) finds "Resolves #1. Resolved #2. Resolve #3." [12.09ms] (pass) finds "Fixes #34055, #30327, #24394, #20816, #32403, #11898, #10056." [17.11ms] (pass) finds "Fixes #18192 and #31675 as a consequence" [10.45ms] (pass) finds "Fixes #1, #2, and #3" [10.96ms] (pass) finds "Fixes #1 & #2" [7.63ms] (pass) finds "Closes #33280, Closes #32864 and Closes #29696 (the timer in #32949 is orthogonal)" [20.29ms] (pass) finds "Closes #33182 and #32947 on top of current main (which already has #36304 for catalogs)." [16.12ms] (pass) finds "Fixes #1,\n#2" [7.76ms] (pass) finds "Fixes #1, #2,\nand #3" [9.27ms] (pass) finds "Fixes #1\nand #2" [8.57ms] (pass) finds "Fixes #1\n& #2" [6.80ms] (pass) finds "Fixes #1 and\n#2" [7.31ms] (pass) finds "Supersedes #39908 (same change, moved from a fork branch)" [13.21ms] (pass) finds "Supersedes #38778 and #38391. Carries the entry point arm of #35053." [14.43ms] (pass) finds "Supersedes #39193 and keeps its three tests." [11.48ms] (pass) finds "This supersedes #33306 and #32803. Their tests are kept here." [13.73ms] (pass) finds "- This replaces #33793. Its ... (truncated) Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` .github/workflows/close-linked-issues.yml | 950 ++++++++++++++++++++++++++++++ test/internal/close-linked-issues.test.ts | 598 +++++++++++++++++++ 2 files changed, 1548 insertions(+) ``` </details> **gate history** · 29 passed · 0 rejected · iteration 9 <details><summary>evidence per changed file</summary> ``` file reads edits tests .github/workflows/close-linked-issues.yml 6 12 0 test/internal/close-linked-issues.test.ts 3 11 0 ``` </details> <!-- robobun:evidence:end -->
2 participants
What
Fixes #31771 —
bun test --isolateretained each finished file's module graph across the per-file global swap, so peak RSS grew linearly with the number of files and OOMed large suites (~13 GB on a ~300-file suite sharing a big import graph). Default (shared-global) mode loads the graph once and stays flat.Reproduction
Cause
--isolateruns each file in a fresh global within one process viaswap_global_for_test_isolation→Zig__GlobalObject__createForTestIsolation. That swap unprotected the outgoing global (gcUnprotect) but:moduleLoader->clearAll()) or require cache (requireMap()->clear()) — every value in a module's top-level binding is rooted throughmoduleLoader -> ModuleRegistryEntry -> AbstractModuleRecord -> JSModuleEnvironment, so the graph survives a collection for as long as the global is reachable at all (and the global can stay transiently reachable across the swap — e.g. aScriptExecutionContextmap entry or a not-yet-swept cell); andDeferGCschedules on scope exit doesn't run a full mark+sweep, and the detached graph sits in the old heap until the end of the run.This is why
--smol(aggressive GC heuristics) and disabling theIsolatedModuleCacheboth failed to flatten it: the source-provider cache holds only source text/bytecode (not records), and without allocation pressure no full GC fires.The process-exit teardown (
Zig__GlobalObject__destructOnExit) and watch-modeGlobalObject::reload()already do exactly the missing steps (clear the registry + collect); the isolation swap didn't.Fix
createForTestIsolation, beforegcUnprotect(oldGlobal), clear the outgoing global's module loader registry (undercellLock()) and require map — mirroringreload()/destructOnExit(). This severs the module graph from the global so it's reclaimable even while the global shell briefly lingers.swap_global_for_test_isolation, which runs for both serial--isolateand--parallelworkers), force a focused full collection (JSC__VM__collectNowFull) +mimalloc_cleanupto reclaim the detached graph and return freed blocks to the OS. The new collection deliberately does not delete unlinked code blocks or clear source-provider caches (unlikeJSC__VM__runGC), so the VM-scoped bytecode andIsolatedModuleCachestay hot and the next file remains fast.Verification
Peak RSS (release, 1000-module graph), polling child
VmHWM:--isolate)--isolate)Slope drops from ~2.8 MB/file to ~0.06 MB/file — flat, matching default mode. Per-file GC cost is negligible in release (N=20 completes in 158 ms) because the heap is kept small.
test/regression/issue/31771.test.tsasserts the RSS slope stays below 0.5 MB/file. Fails on baseline (1.08 MB/file), passes with the fix (0.06 MB/file).test/cli/test/isolation.test.tscases pass (fresh global, module-state isolation, SourceProvider cache, leaked-resource cleanup), as do the NAPI-finalizer isolation tests in panic during tests with parallel #30205 which exercise the same swap path.--isolate: cached SourceProvider's module_info rebuilds correct exportscase (a 2000-export module that runs ~10 s under ASAN and was already over the 5 s default).no test proof · iteration 11 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/regression/issue/31771.test.ts