Repository navigation
bun test --isolate: drop a retired file's deferred work - #42110
Conversation
A FinalizationRegistry cleanup that a finished file created reaches none of the realm fence's checks. JSC schedules it as a DeferredWorkTimer ticket, and runFinalizationCleanup calls the user callback directly, so neither the microtask fence, nor the Job generation check, nor run_callback sees it. It ran in the retired realm while a later file executed. Zig::GlobalObject::scriptExecutionStatus now reports Stopped for a retired realm, and runPendingWork asks the realm's status before it runs a task, as DeferredWorkTimer::doWork does. A retired realm's deferred work is dropped, like the file's timers are dropped at the swap. Still not fenced (unchanged from #41831): calls into JS through an FFI JSCallback, a napi threadsafe function or async completion, functions of a node:vm context or ShadowRealm the finished file created, and microtasks while a debugger is attached.
|
Status: reproduced, fixed, PR open. Waiting on CI. Reproduction: the new case in
Scope note: the crash face of the same area ( |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesThe change marks retired test-isolation realms as stopped and prevents deferred tasks from executing in those realms. It adds an isolation regression test for Retired realm status and detection
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to Retired isolated test realms now discard deferred cleanup work, preventing callbacks and errors from leaking into later test files. The change is covered by an isolation regression scenario and has no remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes Zig::GlobalObject::scriptExecutionStatus and gates all DeferredWorkTimer job execution in runPendingWork — paths that affect every FinalizationRegistry cleanup, wasm completion, and Atomics.waitAsync wake-up in the runtime, not just --isolate — a human familiar with the JSC scheduler should also look.
What was reviewed:
runPendingWork: confirmeddelete joband the event-loop keepalive unref still run on the skipped path; the status check goes through the method table so non-Zig realms (ShadowRealm,node:vm) use their own entry.scriptExecutionStatus: the newStoppedbranch is only reachable whenmicrotaskRunnability() == Discard, which is set exclusively byretireForTestIsolation, so the predicate is inert outsidebun test --isolate. Noted the secondary behavior change the PR calls out: whenBun__VM__scriptExecutionStatusreturns non-Running during process exit, deferred work is now dropped even ifBun__VmHandle__scriptAllowedwould have allowed it.- Test: follows the file's
runTests/describe.concurrentconventions, spreadsbunEnv, uses a bounded 20-iteration poll with an fs sentinel rather than sleeps, and re-registers garbage from the cleanup so the failure window persists.
Extended reasoning...
Overview
This PR extends the bun test --isolate realm-fencing work of #41831 to cover JSC DeferredWorkTimer jobs (FinalizationRegistry cleanups, wasm compile completions, Atomics.waitAsync wake-ups), which bypass the microtask/Job/run_callback checks that fence already covers. Four files change: ZigGlobalObject.h gains an ALWAYS_INLINE isRetiredTestIsolationRealm() helper wrapping the existing microtaskRunnability() == Discard predicate; ZigGlobalObject.cpp's scriptExecutionStatus() returns Stopped when that predicate holds and Bun__JSValue__isFromRetiredTestIsolationRealm is refactored to call the helper; JSCTaskScheduler.cpp's runPendingWork now consults the ticket target's scriptExecutionStatus via the method table and only runs the task when Running, with a debug ASSERT that Suspended never occurs; and test/cli/test/isolation.test.ts gains a two-file fixture proving a finished file's registry cleanup no longer fires while the next file runs.
Security risks
None identified. The change narrows when deferred JS callbacks execute; it does not widen any attack surface, parse untrusted input, or touch auth/crypto paths. The one input the new code reads (microtaskRunnability()) is set only by Bun's own retireForTestIsolation.
Level of scrutiny
Moderate-to-high. The diff is small (~30 native lines + a test) and mirrors JSC's own DeferredWorkTimer::doWork gating, but the two functions it touches sit on the critical path for every deferred-work callback in the runtime. scriptExecutionStatus is a method-table entry JSC itself may consult, and runPendingWork is the sole dispatch point for Bun's DeferredWorkTimer hook. A regression here would affect all FinalizationRegistry/wasm/Atomics.waitAsync users, not just --isolate. The PR description also flags a known adjacent crash (#39994) touching the same lines and a pending rebase, and a secondary behavior change during process shutdown — both are reasons a maintainer with context on the JSC scheduler ownership work should sign off.
Other factors
I traced the skipped-job path in runPendingWork: the pending-set removal, keepalive unref, and delete job all execute regardless of status, so no leak is introduced when a retired realm's job is dropped. The isRetiredTestIsolationRealm predicate is called on JSC::JSGlobalObject* (base-class microtaskRunnability()), so it is safe for any global type, and the method-table dispatch in runPendingWork means non-Zig globals reach their own scriptExecutionStatus implementation rather than the modified one. The new test lives inside the existing describe.concurrent block for --isolate late-completion cases, uses the file's runTests helper with { ...bunEnv, BUN_JSC_collectContinuously: "1" }, polls a bounded window with an fs sentinel (no raw sleeps), asserts stderr before exit code, and the fixture's self-re-registering cleanup keeps the registry live across every collection so the pre-fix failure is deterministic. The bug hunt exited on dry_streak with no findings and no ruled-out candidates.
JSC queues a FinalizationRegistry cleanup as a DeferredWorkTimer ticket, and Bun 1.4.0-1.4.2 run it with no realm check, so it lands after the test file that created the registry is retired. In a retired realm the cleanup surfaces as "Unhandled error between tests" (exit 1 with 0 fail); once the retired global has been collected it reads a freed registry and the process dies with a segfault at address 0x70 (exit 139). The sqlite-smoke job hit the crash twice on main in one day, once right after the plugin-lambda CDK test (aws-cdk-lib's wasm-bindgen registries) and once while loading a CLI test file, where the registry is one of Bun's own builtins. The first face is fixed upstream in oven-sh/bun#42110 (unreleased); the second is oven-sh/bun#39994 with oven-sh/WebKit#487, still open. Until a release carries both, a test:bun preload replaces the global FinalizationRegistry with one that registers nothing on Bun 1.4.x, so no cleanup is ever queued. It runs first, before the other preloads import node:fs. A registered cleanup only ever released memory the test process gives back at exit. Bun 1.3.14 is left untouched. Fixes: #969
JSC queues a FinalizationRegistry cleanup as a DeferredWorkTimer ticket, and Bun 1.4.0-1.4.2 run it with no realm check, so it lands after the test file that created the registry is retired. In a retired realm the cleanup surfaces as "Unhandled error between tests" (exit 1 with 0 fail); once the retired global has been collected it reads a freed registry and the process dies with a segfault at address 0x70 (exit 139). The sqlite-smoke job hit the crash twice on main in one day, once right after the plugin-lambda CDK test (aws-cdk-lib's wasm-bindgen registries) and once while loading a CLI test file, where the registry is one of Bun's own builtins. The first face is fixed upstream in oven-sh/bun#42110 (unreleased); the second is oven-sh/bun#39994 with oven-sh/WebKit#487, still open. Until a release carries both, a test:bun preload replaces the global FinalizationRegistry with one that registers nothing on Bun 1.4.x, so no cleanup is ever queued. It runs first, before the other preloads import node:fs. A registered cleanup only ever released memory the test process gives back at exit. Bun 1.3.14 is left untouched. Fixes: #969
Problem
bun test --isolate: aFinalizationRegistrycleanup that a FINISHED file created runs while a later file executes. A throw from it is charged to the later file, which then loses its remaining tests toCannot call test() after the test run has completed. In the constructed repro (fuzz-found, no user report) 7 of 25 tests ran, exit 1.DeferredWorkTimerticket and calls the callback directly, so no microtask,Joborrun_callbackcheck applies.Bun::runPendingWork(JSCTaskScheduler.cpp:122) ran it with no realm check.Fix
Zig::GlobalObject::scriptExecutionStatusreportsStoppedfor a retired realm, from the fence's own predicate (microtaskRunnability() == Discard, nowBun::isRetiredTestIsolationRealm).runPendingWorkasks that status before it runs a task, as JSC'sDeferredWorkTimer::doWorkdoes, and drops the work, like the file's timers at the swap.Stoppedfor a detached document.isolation.test.tscase fails on main 20 of 20, passes 6 of 6 here. Self-reviewed: 5 concerns, 4 addressed, 1 deferred (Notes).Background
--isolategives each file a freshZig::GlobalObjecton oneJSC::VM. The old global lives until the GC takes it.DeferredWorkTimercarries native completions (registry cleanups, wasm compiles,Atomics.waitAsyncwake-ups) to the JS thread, one ticket each. Bun installsonScheduleWorkSoon, so they run from its event loop, not JSC'sdoWork.ScriptExecutionStatusis how JSC asks the embedder whether a realm may run script. Bun answered from the VM alone, so a retired realm saidRunning.Notes
a.test.tscreates aFinalizationRegistrywhose callback throws and registers 500 objects, then six files each runBun.gc(true)and a short sleep per test, all underBUN_JSC_collectContinuously=1. On main, 27 of 30 runs lose 15 to 19 of the 25 tests to theCannot call test()path. The organic reach is lower than that: it needs--isolateor--parallel, a collection between the finished file's last loop drain and the swap, and a cleanup callback that throws or acts on shared state.BUN_JSC_collectContinuously=1in the child for the same reason: natural GC timing queues the cleanup in that window only sometimes.JSCallback, a napi threadsafe function or napi async completion, functions of anode:vmcontext orShadowRealmthat the finished file created (those globals are not retired, so their own deferred work also still runs), and microtasks while a debugger is attached. The review of this change also pointed at the Workercloseevent:WorkerMessagingProxy::workerGlobalScopeDestroyedInternaldispatches it throughWorker::dispatchCloseEvent(src/jsc/bindings/webcore/Worker.cpp:139), which has no realm check, so a finished file'sworker.on("close")handler may run under the next file when the terminated worker's thread exits late. I have not reproduced that one and left it out of this PR.ticket->scriptExecutionOwner()->vm()after the realm's global is collected and the ticket cancelled:ASSERTION FAILED: !isCancelled()on debug, a stale read on release) belongs to Let JSC's DeferredWorkTimer own the deferred-work tickets so a collection cancels a dead realm's work (WebKit bump) #39994, which moves ticket ownership into JSC through DeferredWorkTimer: keep the embedder's tickets in m_pendingTickets so the end of a collection cancels them WebKit#487. The status gate here is orthogonal to that: JSC'sdoWorkperforms the same check, so it survives the ownership move. The two touch adjacent lines inrunPendingWorkand will need a trivial rebase, nothing more.VM::ClientData::reconcileWeakReferencesAtGCEndhook. That is the shape Let JSC's DeferredWorkTimer own the deferred-work tickets so a collection cancels a dead realm's work (WebKit bump) #39994 already tried and review rejected, so it is not here.runPendingWorkis the only caller ofscriptExecutionStatusthis adds. JSC'sdoWorkis the only other caller, and with Bun's hooks installed its task queue and ticket set are both empty, so the newStoppedanswer changes nothing else.Bun__VM__scriptExecutionStatus(the VM-wide answer that timers use) is untouched. A debugASSERTrecords that no Bun realm reportsSuspended, whichdoWorkwould re-queue rather than drop.is_shutting_down), deferred work that is still dispatched is now dropped as well, which is what timers already do and what Node does for aFinalizationRegistrycallback queued fromprocess.on("exit")(test-finalization-registry-shutdown.js).[human-review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file