Repository navigation
bun test --isolate: a macro call is undefined from the second test file on - #46405
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 15 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked that the manual unprotect() loop over vm.macros is balanced: Bun.registerMacro (src/runtime/api/BunObject.rs) is the only insert path and protects every stored callback, so no entry is unprotected without a matching protect. Calling deinit() on the JS thread does not drop the source-code printer, since drop_source_code_printer_if_macro_owned returns early when the runtime VM owns it. BUN_TEST_PARALLEL_SCALE_MS is read by the parallel runner, so the --parallel test row does route all three files through one worker.
Extended reasoning...
The change adds ~9 lines to swap_global_for_test_isolation in src/jsc/VirtualMachine.rs (unprotect and clear vm.macros, take and deinit vm.transpiler.macro_context), rewrites three comments that described the context as process-lifetime, and adds two tests plus a fixture generalization in test/cli/test/isolation.test.ts. It touches no security-sensitive surface. Two inline findings are already posted (a missed comment site in src/bundler/transpiler.rs and the pool-thread macro VM still carrying module state, which the PR description acknowledges), so a human look is already signalled; this note only records what else was examined and ruled out.
|
There is nothing in this PR to fix for it. BuildKite has already queued a retry of both shards, and they are waiting for an agent. I cannot re-run BuildKite jobs myself. Generated by Claude Code |
What does this PR do?
Under
bun test --isolate(and so--parallel), a macro call in a test file evaluates toundefinedfrom the second file of the process on.bun test --isolate: 3 pass on 1.4.2, 1 pass and 2 fail on 1.4.3 and main.Cause
Two pieces of macro state belong to the VM, not to the global, and
swap_global_for_test_isolationleft both alone:vm.macrosmaps a macro's id to the function thatBun.registerMacrowas given,protect()ed.vm.transpiler.macro_contextremembers which ids have been loaded, and skipsMacro::initfor those.So the second file finds the macro already loaded and calls the function the first file registered, which is a function of the first file's realm. That realm was retired at the swap. Since #44327
JSValue::calldoes not enter a retired realm and returnsundefined, and the macro runner turns thatundefinedinto the call's value.Before #44327 the call went through, which hid two other things: every file ran the first file's copy of the macro module (module-level state carried over from file to file), and the
protect()kept the global of any file that loaded a macro alive for the rest of the run.Fix
src/jsc/VirtualMachine.rs, inswap_global_for_test_isolation: unprotect and clearvm.macros, and freevm.transpiler.macro_context.parse_maybemakes a new context when there is none, so the next file loads the macro in its own realm. It is done after the realm is retired, when nothing of the outgoing file can load a macro again.The context is freed with
deinit()and not only set toNone: it has noDrop, so that would leak the box and its arena once per file. No AST is waiting to be printed at the swap, which is what the arena has to outlive.The other source changes are comments that said
vm.transpiler's context is leaked for the life of the process.Not changed
A module that is transpiled on a pool thread (a static or dynamic
importof a non-entry file) runs its macros in that thread's own macro VM. That VM has no realm to retire, so those calls were neverundefined. It also still keeps macro module state from file to file, as before.How did you verify your code works?
New tests in
test/cli/test/isolation.test.ts:a test file's macros are loaded again for each file, for--isolateand for a--parallelworker that takes all three files. Each file calls the macro itself and through a module itrequire()s, and the macro counts its calls, so the test also sees which copy of the module ran.a macro left loaded, beside the other "collects globals pinned by leaked handles" tests. Each of the 8 files loads a different macro.491aaad15), debugundefinedin files 2 and 3vm.macrosclearedmacro_contextfreedUSE_SYSTEM_BUN=1)b:4,b:5,required-b:6(the first file's copy goes on counting)bun bd test test/cli/test/isolation.test.ts: 50 pass, 0 fail.By hand with the debug build: the repro above gives 3 pass with
--isolate, with--parallel=2and with neither. Macros reached through a static import,import(),require(), a--preloadand an async macro give the right value in all three files. A macro module that throws while loading in the first file loads in the third.Generated by Claude Code