bun test --isolate: opt-in global reuse fast path - #36871
Open
robobun wants to merge 11 commits into
Open
Conversation
On import-heavy suites the per-file --isolate cost is dominated by re-linking and re-evaluating every module in a fresh JSGlobalObject, and re-tiering the same hot functions per global. Profiling bench/test/app (2000 files importing zod + date-fns + lodash) showed parsing is already negligible (SourceProvider/CodeCache sharing works); the cost is the module graph rebuilding. When a file leaves globalThis in its post-preload shape (no built-in own property overwritten, no prototype watchpoint fired, no top-level lexical bindings added, Module.wrapper/prepareStackTrace untouched) we now scrub any extra own properties it leaked, drop only project-path ESM/CJS registry entries, and reuse the same global. node_modules and builtin module records survive, so their linked CodeBlocks and JIT'd code carry over to the next file. Anything that fails the check falls back to the existing createForTestIsolation swap. Also cache each internal JS module's UnlinkedFunctionExecutable per-VM so a fresh global re-link()s instead of re-parsing; createBuiltinExecutable bypasses CodeCache so this was paying a full parse + bytecodegen per global before. bench/test/app, 2000 files, 16-core linux release: --isolate (serial) 88.2s / 152s user -> 18.9s / 31s user --parallel 15.2s / 172s user -> 3.2s / 32s user BUN_FEATURE_FLAG_DISABLE_ISOLATION_GLOBAL_REUSE=1 forces the full swap.
Contributor
WalkthroughChangesTest isolation now supports experimental global reuse. The VM captures baseline state, resets reusable globals, and falls back to full swaps when validation fails. CLI controls, statistics bindings, and isolation tests cover the behavior. Test isolation global reuse
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Collaborator
|
Split the unlinked executable code out. And then let's make the isolation more granular. What about process.stdin. What about reassigning to the LazyProperty callbacks on Bun? What about stdin, stdout, stderr. What about mocks. And this definitely can't be a default. It has to be some other setting. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 807-810: Update the deletion loop in the global-object cleanup
path to inspect the boolean result of JSC::JSCell::deleteProperty for each id;
when any deletion fails, immediately return swap() instead of reporting
successful cleanup, while preserving the existing loop for successful deletions.
- Around line 702-706: Update the Entry baseline storage in the ownProperties
map to use JSC::Strong<JSC::Unknown> instead of JSC::EncodedJSValue, ensuring
baseline JavaScript values remain GC-rooted. Adjust the baseline capture and
reset comparison logic associated with ownProperties to compare the Strong’s
current value while preserving the unchanged-value behavior.
In `@test/cli/test/isolation.test.ts`:
- Around line 506-518: Add a test case alongside the existing built-in and
Array.prototype mutation cases that assigns a symbol-keyed own property on
globalThis, then verifies the next isolated file cannot observe it and that
stats?.swap is at least 1. Include the relevant symbol-keyed mutation variants
required by the fixture matrix while preserving the existing successful exit and
pass-count assertions.
- Around line 521-529: Update the reuseFixtures scenario in the
“BUN_FEATURE_FLAG_DISABLE_ISOLATION_GLOBAL_REUSE=1” test so b.test.ts mutates an
observable package or global value and c.test.ts asserts that value is absent.
Preserve the existing a.test.ts-to-b.test.ts isolation assertion while adding
coverage for the b.test.ts-to-c.test.ts transition, verifying both forced full
swaps.
🪄 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: 13b2a422-c0c4-4a68-848d-71e07f12d4e4
📒 Files selected for processing (13)
src/bun_core/env_var.rssrc/js/internal-for-testing.tssrc/jsc/JSGlobalObject.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/BunClientData.hsrc/jsc/bindings/BunProcess.hsrc/jsc/bindings/InternalForTesting.cppsrc/jsc/bindings/InternalForTesting.hsrc/jsc/bindings/InternalModuleRegistry.cppsrc/jsc/bindings/ZigGlobalObject.cppsrc/runtime/cli/test/parallel/runner.rssrc/runtime/cli/test_command.rstest/cli/test/isolation.test.ts
Collaborator
Author
|
Splitting the For this PR I'll flip the reuse path to opt-in (new setting, off by default) and add the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Collaborator
Author
|
Updated 6:10 AM PT - Aug 4th, 2026
✅ @robobun, your commit 5aaa813f7e45c15aa3aaef6b85c5d99844a27bd9 passed in 🧪 To try this PR locally: bunx bun-pr 36871That installs a local version of the PR into your bun-36871 --bun |
…opes
Per review: the reuse path changes --isolate semantics (node_modules
module state persists; Bun.* / process.* mutations not detected), so it
cannot be a default. Gate on BUN_FEATURE_FLAG_EXPERIMENTAL_ISOLATION_GLOBAL_REUSE=1.
Also:
- mockModule = {} zeroed every LazyProperty m_pointer, losing the
initLater callback tag, so jest.fn() after a reuse passed a null
Structure* into allocateCell. Clear only activeSpies/activeMocks.
- Wrap deleteProperty/JSMapIterator/remove/reifyAllStaticProperties in a
TopExceptionScope (CI x64-asan tripped the unchecked-exception
validator at JSOrderedHashTableHelper removeImpl).
- swap() if deleteProperty returns false (non-configurable leak).
- Skip baseline capture entirely when reuse is off.
- Drop the internal-module executable cache (split out to #36873).
robobun
changed the title
Collaborator
Author
|
Diff is ready for review at 5aaa813. CI build 88801 has 9 failures, all classified The gate check separately keeps hitting "BUILD FAILED (no junit output)" (stray untracked Whole reuse path is gated on |
…eload chain - Entry::value is now Strong<Unknown> (raw EncodedJSValue in malloc'd memory is an ABA hazard once a test overwrites a baseline global). - JSMock__resetSpies instead of activeSpies.clear() so an unrestored jest.spyOn target is restored rather than having its recovery path dropped. - Remove m_nextTickQueue.clear(): the reified process.nextTick closure captures the queue cell, so nulling the global's field left every drain site skipping and nextTick callbacks silently dropped. - Record module-map and require-map keys at baseline (i.e. everything --preload loaded) and evict them on reset regardless of path, so a node_modules preload (or its transitive CJS deps) re-evaluates and re-registers hooks that reset_hook_scope_for_test_isolation dropped.
Contributor
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 `@src/jsc/bindings/ZigGlobalObject.cpp`:
- Around line 896-897: Update the cleanup flow around JSMock__resetSpies so
that, when spy restoration leaves an exception, the exception is cleared and
swap() is returned before reporting successful reuse. Preserve the normal
successful-reset path while preventing the next isolated file from observing
partially restored spies.
🪄 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: 1d99ced2-bf0c-4cd6-bacf-2665ae0b7ede
📒 Files selected for processing (7)
src/bun_core/env_var.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/BunClientData.hsrc/jsc/bindings/ZigGlobalObject.cppsrc/runtime/cli/test/parallel/runner.rssrc/runtime/cli/test_command.rstest/cli/test/isolation.test.ts
💤 Files with no reviewable changes (1)
- src/jsc/bindings/BunClientData.h
…e check; re-capture baseline after reuse - Move JSMock__resetSpies before the own-property compare so spyOn(globalThis, builtin) is reverted to the baseline value (reuse succeeds) and the scrub can't be undone by clearSpy restoring a key it just deleted. - Clear baseline_captured on the reuse return: the preload chain re-evaluates on the reused global and may assign fresh object identities to globalThis; without a re-capture the next pristine check compares against stale Strong<> refs and swap-alternates. - Reset overridenDateNow (jest.setSystemTime without useFakeTimers leaves it set) and process EventEmitter listeners (preload re-eval would append one more per file) alongside clearCachedCwd.
…ptureCallback Re-capturing the baseline on a reused global was snapshotting the entire moduleMap into preloadModuleKeys, which at that point also holds the node_modules records the previous reset deliberately kept, so the next reset evicted them (alternating hit/miss). The preload graph is stable across reuses; keep the first capture's snapshot. Also clear m_uncaughtExceptionCaptureCallback / m_reportOnUncaughtException in the Process reset block (same native-field class as cachedCwd and overridenDateNow already handled there).
Contributor
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/test/isolation.test.ts`:
- Around line 503-508: In test/cli/test/isolation.test.ts, assert the collected
exitCode is zero after the existing output and state assertions at lines
503-508, specifically after pkgSlotC, and at lines 606-609, after the
reset-statistics assertion. Update both subprocess test sites so successful
output and state checks also require clean child termination.
🪄 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: d553c147-a27f-4a74-b408-82ac860b5d9f
📒 Files selected for processing (3)
src/jsc/VirtualMachine.rssrc/jsc/bindings/ZigGlobalObject.cpptest/cli/test/isolation.test.ts
…at baseline - onLoadPlugins/onResolvePlugins were never cleared, so a preload that calls Bun.plugin(...) appended one more filter/callback pair per reused file (unbounded Strong<> accumulation, O(N) regex matches per import). - The hasOverriddenModule* / prepareStackTrace checks compared against zero, so a preload that sets Error.prepareStackTrace or patches Module._resolveFilename (source-map-support, tsconfig-paths) swapped on every file. Snapshot them at baseline and compare to the snapshot.
… reset
- swap() now clears ownProperties/prepareStackTraceValue so the outgoing
global isn't pinned by ~100+ Strong<> handles through the idle window
before the next capture.
- BunPlugin::OnLoad has a user-declared dtor, so = {} selects the
implicit copy-assign which overwrites the raw virtualModules pointer
without deleting it (leaking the map + every Strong<JSObject> mock
callback per reused file).
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/ZigGlobalObject.cpp (1)
880-889: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace
isProjectPathheuristics with URL and path parsing to prevent test-isolation cache mismatches.The
isProjectPathfunction classifies module cache keys using substring matching and character position checks. When a cache key is scheme-qualified (e.g.,file:///path/to/module.js) or non-canonical, it fails to match the patterns and is incorrectly retained. This breaks test isolation when project modules are not removed.Parse the key with
WTF::URLif it contains a scheme. For path-only keys, use the existing path normalization andnode_modulesdetection. Ensure both ESM (moduleMap) and CommonJS (requireMap) keys are classified consistently.🤖 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 `@src/jsc/bindings/ZigGlobalObject.cpp` around lines 880 - 889, Replace the heuristic logic in isProjectPath with URL parsing for scheme-qualified keys and the existing path normalization plus node_modules detection for path-only keys. Ensure canonical and non-canonical project paths, including file URLs, are recognized consistently, and apply the same classification when filtering both moduleMap and requireMap entries.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/jsc/bindings/ZigGlobalObject.cpp`:
- Around line 880-889: Replace the heuristic logic in isProjectPath with URL
parsing for scheme-qualified keys and the existing path normalization plus
node_modules detection for path-only keys. Ensure canonical and non-canonical
project paths, including file URLs, are recognized consistently, and apply the
same classification when filtering both moduleMap and requireMap entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0723b25e-d34c-4f20-b904-2ea80270db64
📒 Files selected for processing (1)
src/jsc/bindings/ZigGlobalObject.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds an experimental opt-in fast path for
bun test --isolate/--parallel: when a test file doesn't mutate the global environment, reuse the sameJSGlobalObjectfor the next file (scrubbing leaked own properties and dropping only project-path module records) instead of creating a fresh one.node_modulesand builtin module records survive, so their linkedCodeBlocks and JIT'd code carry over.Gated on
BUN_FEATURE_FLAG_EXPERIMENTAL_ISOLATION_GLOBAL_REUSE=1; off by default.The always-safe internal-module
UnlinkedFunctionExecutablecache is split out to #36873.bench/test/app(cd bench/test && bun install && bun app/setup.ts 2000 20 && cd app), 16-core Linux release, with the flag set:bun test --isolate testsbun test --parallel testsHow
After
--preloadruns on a fresh global,Zig__GlobalObject__captureTestIsolationBaselinereifies every static hash-table global and snapshotsglobalThis's own-property slot values + attributes (asStrong<Unknown>), the lexical/var symbol-table sizes, the module-override /prepareStackTraceflags, and the set of module keys the preload chain loaded.Before the next file,
Zig__GlobalObject__tryResetForTestIsolationchecks:isHavingABadTime,{object,array,string}PrototypeChainIsSane,{array,map,set}IteratorProtocol)globalLexicalEnvironment/ var symbol table sizes unchangedModule.wrapper/_resolveFilename/runMain/Error.prepareStackTracematch the baseline snapshotjest.spyOntargets) every baseline own property's slot value and attributes unchangedIf all of that holds, it deletes any extra own properties the file leaked (falling back to swap if a delete fails), drops the preload chain + project-path ESM/CJS registry entries, clears
nextTickQueue/ jest-mock active spies / event listeners /Bun.pluginstate /overridenDateNow/ process listeners+cached-cwd+uncaughtExceptionCaptureCallback, and reuses the global. Any check failing falls back to the fullcreateForTestIsolationswap and re-captures the baseline on the new global.The existing Rust-side per-file cleanup (sockets, timers, watchers, subprocesses, cwd restore) runs in both cases.
Known limitations (why this is experimental)
Under the fast path, things the pristine check does not catch leak into the next file:
node_modules/node:*/bun:*module-level state loaded by the test file (e.g.axios.defaults.baseURL = ...,z.setErrorMap(...)). The preload chain itself re-evaluates every file.Math.random = fn,process.env.X = y,process.stdin,Bun.inspect = ...)node_modulespackage loaded by--preload(e.g.reactvia@testing-library/reactcleanup) is also imported by a test-file-loadednode_modulespackage (e.g.@tanstack/react-query), the preload copy is evicted and re-evaluated while the test-loaded importer stays linked to the original, so the next file sees two instances (Invalid hook call/instanceof/ context-provider mismatches).Project-path modules still re-evaluate fresh, and a file that replaces a global (
globalThis.fetch = mock) or fires a prototype watchpoint is detected and falls back to the full swap.Tests
6 new cases in
test/cli/test/isolation.test.tsvia atestIsolationResetStatshook onbun:internal-for-testing: off by default; reuses when clean (string- and symbol-keyed leaks scrubbed,jest.fn()/process.nextTickwork, node_modules record survives two hops); falls back on built-in overwrite; falls back on prototype watchpoint; falls back on non-configurable leak; node_modules--preloadre-registers hooks,jest.spyOn/jest.setSystemTime/process.onare restored and don't accumulate. The existingleaked global is not visible,module state is not shared, preload re-run, chdir, fake-timer and socket/watch cleanup cases still pass (they take the default full-swap path).