napi: treat finalizers fired by VM destruction as running from GC - #38506
napi: treat finalizers fired by VM destruction as running from GC#38506robobun wants to merge 5 commits into
Conversation
Heap::lastChanceToFinalize (reached from ~VM when a Worker exits, or on the main thread under BUN_DESTRUCT_VM_ON_EXIT) fires every finalizer that is still registered, but runs weak-handle finalizers and the destructors of precise allocations without the Sweeping mutator state a collection has. NapiEnv::inGC() only looked at that state, so a regular module's napi_add_finalizer / empty external buffer callbacks ran inline from the dying heap after env teardown, an experimental module's finalizers got past the GC check, and NapiHandleScope::open allocated a cell in a heap whose allocators were already stopped. inGC() and the handle scope check now also consult heap.isShuttingDown(), which lastChanceToFinalize sets first, so these finalizers take the same paths they take during the final collection: deferred (and dropped by NapiFinalizerTask::schedule) for regular modules, run under the GC check for experimental ones.
|
Warning Review limit reached
Next review available in: 38 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 (6)
Comment |
|
Reproduced on the current release canary and on a debug build with the new fixture ( Fix is in this PR (#38506): |
![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 found no bugs. The production change is two lines (inGC() and NapiHandleScope::open each gain a heap.isShuttingDown() check), but it changes finalizer semantics during Worker/VM teardown, so a human look would still be worthwhile.
What was reviewed:
- Traced all four
inGC()call sites (checkGC,doFinalizer,NapiRef::callFinalizer) — each takes the intended path whenisShuttingDown()is true. - Confirmed
NapiFinalizerTask::schedulealready handles theis_shutting_down() && has_run_cleanup_hooks()case by dropping the task, so the newly-deferred regular-module finalizers are released rather than leaked or run. - Test fixture covers both build variants × both VM-destruction paths × four finalizer-registration kinds; assertions pin the pre-fix failure modes.
Extended reasoning...
Overview
The PR adds || vm().heap.isShuttingDown() to NapiEnv::inGC() in src/jsc/bindings/napi.h and the same disjunct to the Sweeping guard in NapiHandleScope::open in src/jsc/bindings/napi_handle_scope.cpp. Everything else is comment updates, a new C test addon (test_vm_teardown_finalizers.c, built twice via binding.gyp), a JS driver, four new test cases in napi.test.ts, and an unrelated 10s timeout on a slow bigint test.
Security risks
None. This is internal GC-state detection for the NAPI finalizer scheduler; no user-facing API surface, no parsing of untrusted input, no auth/crypto.
Level of scrutiny
High. Despite the tiny production diff, this sits at the intersection of JSC heap teardown, NAPI finalizer ordering, and Worker lifecycle — the most-blocked native memory-safety category in this repo. The change alters when addon finalizers run (or are dropped) at VM destruction, which is exactly the kind of semantic shift a maintainer should confirm: e.g. that silently dropping regular-module finalizers at Worker exit (via the existing NapiFinalizerTask::schedule shutdown branch) is the intended behavior pending #32912, and that no other inGC() caller is adversely affected by the widened predicate.
Other factors
- I traced every
inGC()caller insrc/jsc/bindings/and each behaves as the PR describes under the new condition:checkGC()aborts experimental modules,doFinalizer/callFinalizerroute throughnapi_internal_enqueue_finalizer, andNapiFinalizerTask::schedule(napi_body.rs:5209) drops the task once cleanup hooks have run. NapiHandleScope::openreturningnullptris already handled by every caller (the RAII wrapper andNapiHandleScope::closeboth null-check).- The test addon uses static globals (
env_torn_down,pending_finalizers), which is fine because the fixture loads the addon in exactly one env per process (either the Worker or the main thread, never both). - The experimental-module test only runs the Worker path, not
--main-thread— but both JSC firing paths (PreciseAllocation::sweepand weak-handle finalize) are covered per-kind, and the regular-module test covers both VMs. - No CODEOWNERS match the changed files. No prior human review comments to address.
…rdown fixture children
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?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 how NAPI finalizer dispatch detects GC context during JSC VM teardown — a memory-safety-critical interaction between Heap::lastChanceToFinalize, mustDeferFinalizers(), and the Rust-side NapiFinalizerTask::schedule drop path — a human look from someone familiar with the NAPI/JSC finalizer lifecycle would still be worthwhile.
What was reviewed:
inGC()/NapiHandleScope::open: addingheap.isShuttingDown()is strictly more conservative; verified the deferred path (NapiFinalizerTask::schedule) already drops tasks whenis_shutting_down() && has_run_cleanup_hooks(), so regular-module finalizers routed there during~VMare released, not leaked or run.- Test fixture: pins via strong
napi_refso onlylastChanceToFinalizereaches the finalizers; covers all four registration kinds × Worker/main-thread × regular/experimental; assertions check the positive invariant (pending count, FATAL ERROR text) rather than absence-of-crash. - comment-cop feedback was addressed in later commits — the current diff has one-line comments at both sites.
Extended reasoning...
Overview
Two-line native change plus ~270 lines of test infrastructure. NapiEnv::inGC() (src/jsc/bindings/napi.h) and NapiHandleScope::open (src/jsc/bindings/napi_handle_scope.cpp) each gain || vm.heap.isShuttingDown() so finalizers fired by Heap::lastChanceToFinalize during ~VM are treated as GC-context. New test addon test_vm_teardown_finalizers.c (built as regular and experimental), fixture vm-teardown-finalizers.js, two binding.gyp targets, and a new describe block in napi.test.ts. One unrelated timeout bump on the bigint error-code test.
Security risks
None. No user-controlled input parsing; the change tightens an internal safety predicate. The only observable behavior change is that (a) regular-module finalizers still registered at VM destruction are dropped instead of run inline in a dying heap, and (b) experimental-module finalizers making GC-affecting calls from there now abort as documented.
Level of scrutiny
High. This is JSC GC / NAPI finalizer lifecycle code — REVIEW.md's most-blocked category. The change is small and the direction (more conservative inGC()) is safe by construction, but the interaction between JSC's Heap::isShuttingDown(), Bun's Rust-side VirtualMachine::is_shutting_down(), NapiEnv::cleanup() ordering, and NapiFinalizerTask::schedule's drop path is subtle enough that a maintainer who owns this area should confirm the reasoning (and its relationship to #32912).
Other factors
- I traced the deferred path:
doFinalizer→napi_internal_enqueue_finalizer→NapiFinalizerTask::schedulechecksvm.is_shutting_down()andhas_run_cleanup_hooks()(src/runtime/napi/napi_body.rs:5209) and drops the task. SinceNapiEnv::cleanup()runs beforedestroyVM, the drop branch is taken for finalizers deferred fromlastChanceToFinalize, matching the PR's claim. - Test quality is strong: strong-ref pins guarantee only
~VMcan reach the finalizers; the instance-data finalizer prints the pending count so the test asserts the precondition; regular-module tests assertstderris empty and exit 0, experimental tests assert the exact FATAL ERROR text and non-zero exit; LSan is disabled with a stated reason (undeletable pins keep theNapiEnvref alive past Worker exit). - The github-actions comment-cop flagged paragraph-length comments on earlier commits; commits
62c3c9a6and761939d2shortened them to one-liners, which is what the current diff shows. - The bug hunting system found no issues.
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. The two-line native change (inGC() and NapiHandleScope::open also checking heap.isShuttingDown()) looks correct and is thoroughly tested, but since it changes finalizer scheduling in the JSC GC/VM-teardown path — memory-safety-critical NAPI binding code — a human look is still worthwhile.
What was reviewed:
- Confirmed
NapiFinalizerTask::schedule(napi_body.rs:5209) already drops deferred finalizers whenis_shutting_down && has_run_cleanup_hooks, so the widenedinGC()routes regular-module finalizers to a path that safely discards them. - Checked the other
inGC()call sites (checkGC,doFinalizer,NapiRef::callFinalizer) — all take the intended branch during heap shutdown. - The test fixture's
detect_leaks=0is justified (pins are intentionally never deleted) but is a weakened sanitizer that deserves a second pair of eyes.
Extended reasoning...
Overview
The production change is two lines: NapiEnv::inGC() in src/jsc/bindings/napi.h and the sweep guard in NapiHandleScope::open in src/jsc/bindings/napi_handle_scope.cpp each gain || vm.heap.isShuttingDown(). The rest is test infrastructure: a new C addon (test_vm_teardown_finalizers.c) built as both a regular and an experimental NAPI module, a JS driver, and four new test cases in napi.test.ts. There's also an unrelated 10s timeout on the bigint error-code test.
Security risks
No user-facing input parsing or auth surface. The risk is memory safety: getting the inGC() predicate wrong either lets addon finalizers allocate in a heap being destroyed (the bug being fixed — debug assertion, release UAF/corruption) or, in the other direction, would suppress finalizers that should run. The change is a strict widening gated on Heap::isShuttingDown(), which JSC only sets on entry to lastChanceToFinalize inside ~VM, so paths outside VM destruction are unchanged.
Level of scrutiny
High. This is JSC GC / NAPI finalizer interaction — the review guide's most-blocked category. The PR description traces the mechanism precisely (WeakSet::lastChanceToFinalize and PreciseAllocation::sweep run without SweepingScope), and I verified the deferred-finalizer sink in napi_body.rs:5209-5221 drops the task rather than running it once cleanup hooks have run. The design note that this is "complementary to, not a replacement for" running these finalizers at env teardown (#32912) is a judgment call a maintainer should weigh in on.
Other factors
The comment-cop bot flagged long comments; the author addressed all of them (now one-liners, all threads resolved). Test coverage is thorough: both VM-destruction paths (Worker exit, main thread with BUN_DESTRUCT_VM_ON_EXIT), both module flavours, all four registration kinds that survive env teardown, with assertions on stderr content and exit code. The fixture disables LSan for its children — the justification (undeleteable strong pins keep the NapiEnv/VM handle alive past Worker exit) is sound, but per the review guide weakening a sanitizer warrants explicit human sign-off.
2 participants
Problem
worker_threadsWorker exiting; the main thread underBUN_DESTRUCT_VM_ON_EXIT=1),Heap::lastChanceToFinalizefires every Node-API finalizer that is still registered. Bun did not recognize that as a GC context, so:napi_add_finalizercallbacks (both thenapi_refand theresult == NULLvariants) and zero-lengthnapi_create_external_buffercallbacks ran inline, afterNapiEnv::cleanup()had already run the addon's cleanup hooks and instance-data finalizer.NapiEnv::mustDeferFinalizers()documents why that is never safe; the same callbacks are deferred and dropped when the final collection reaches them instead.ASSERTION FAILED: m_cellState == CellState::DefinitelyWhiteinJSC::JSCell::JSCell, fromNapiHandleScopeImpl::create <- NapiHandleScope::open <- ... <- Heap::lastChanceToFinalize <- JSC::VM::~VM <- destroyVM.napi_get_undefinedreturnsnapi_okfrom insidePreciseAllocation::sweep <- Heap::lastChanceToFinalize <- ~VM) instead of theFATAL ERRORthe same call gets during a collection.NapiEnv::inGC()(src/jsc/bindings/napi.h) isvm().isCollectorBusyOnCurrentThread(), i.e.mayBeGCThread() || mutatorState() != Running, andNapiHandleScope::open(src/jsc/bindings/napi_handle_scope.cpp) checksmutatorState() == Sweeping.lastChanceToFinalizeruns on the mutator thread and leaves the stateRunningon two of its three paths:WeakSet::lastChanceToFinalize(weak-handle finalizers:Heap::addFinalizerlambdas andNapiRefowners) runs before the block sweep that installs aSweepingScope, andPreciseAllocation::sweep(destructors such as~NapiExternal) never installs one. Only cells swept throughMarkedBlock::Handle::sweepwere detected.Fix
inGC()also returns true whilevm().heap.isShuttingDown(), andNapiHandleScope::openreturns null in that state too.lastChanceToFinalizesets that flag before it fires anything, so every finalizer it fires now takes the path it takes from the finalcollectNow():doFinalizer/NapiRef::callFinalizerdefer a regular module's callback (whichNapiFinalizerTask::schedulethen drops, cleanup hooks having run),checkGC()refuses an experimental module's GC-affecting calls, and no handle scope cell is allocated.inGC()is the one predicate every Node-API GC check goes through (doFinalizer,callFinalizer,checkGC()behindNAPI_CHECK_ENV_NOT_IN_GCandnapi_internal_check_gc); the handle scope check is the only one outside it.isShuttingDown()is set nowhere else:NapiEnv::cleanup(),wrap_cleanupand everything beforedestroyVMare unchanged. Experimental modules keep running their finalizers synchronously, as they do from a collection, so native memory they free at Worker exit is still freed.napi_add_finalizereither way, zero-length external buffers) used to have their callback run at Worker exit, unsafely; with this PR it is not run at all, exactly as already happens to the same callbacks when the final collection gets to the object first (the usual case: those objects are unreachable once the global is released; only objects still rooted, e.g. by anapi_ref, survive into~VM). Running them at env teardown instead, as Node does and as Bun already does fornapi_wrapand non-empty external buffers, is separate work: napi: run napi_add_finalizer and napi_create_external finalizers at env teardown #32912 does it fornapi_add_finalizerandnapi_create_externalbut predates Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075 and currently conflicts; nothing covers zero-length external buffers or external strings (whose callbacks a JSString cell swept by~VMalready had dropped, via the detectedMarkedBlockpath) yet. Whatever is left unbound still ends up inlastChanceToFinalize, so this check is needed in every one of those shapes.test/napi/napi.test.ts("finalizers still registered when the VM is destroyed"): new addontest_vm_teardown_finalizers.c, built as a regular and as an experimental module, registers one finalizer per kind and pins each object with a strong ref so only~VMcan reach it, in a Worker and on the main thread; the instance-data finalizer reports how many are still registered when env teardown finishes (4), which is what ties the test to the~VMpath (a change that starts running one of these kinds at env teardown fails that line and should drop the kind from the list). Before: oneFAIL:line per kind (and theJSCellassertion on the debug build) for the regular module,status 0and a clean exit for the experimental one; after: the regular module runs none of them, the experimental module aborts with theFATAL ERROR. Also reproduced on the current release canary with the fixture.NapiEnv, and the VM handle ref it holds, alive past the Worker's exit, which LSan otherwise reports on the ASAN lanes.test/napi/napi.test.ts,napi-finalizer-delete-ref.test.ts, and the finalizer / reference / env-teardown / worker node-napi-tests pass on the debug ASAN build. One unrelated test innapi.test.ts(bigint error codes: three serial node + bun runs) takes ~5.4s on a debug ASAN build and gets the same 10s budget its neighbours have.Background
doFinalizer/NapiRef::callFinalizerenqueue it as aNapiFinalizerTaskwheninGC()is true. An experimental (NAPI_EXPERIMENTAL) module's finalizer runs synchronously from the collector and may only call functions that do not affect GC state;checkGC()aborts the process with aFATAL ERRORotherwise, as Node does.NapiEnv::cleanup(), a VM cleanup hook that runs with the exit handlers, beforedestroyVM: it runs the addon's cleanup hooks, the finalizers bound to the env (napi_wrap, non-empty external buffers) and finally the instance-data finalizer. Finalizers registered any other way stay on the JSC heap and are only reached by a collection or by~VM.Heap::mutatorState()is JSC's record of what the JS thread is doing;MutatorState::Sweepingis installed by aSweepingScopewhile aMarkedBlockis swept (destructors and weak finalizers run from there during a collection).Heap::finalize()installs one around the precise-allocation sweep too, so during a collection every finalizer is covered;lastChanceToFinalizeis the one caller that is not.PreciseAllocationis a cell JSC allocates and sweeps individually rather than inside aMarkedBlock; the first few cells of everyIsoSubspace(such asNapiExternal's) are allocated that way.Heap::isShuttingDown()is the flaglastChanceToFinalizesets on entry; JSC itself uses it (CodeBlock,JSLock) to detect work happening during heap destruction.destroyVM(src/jsc/bindings/ZigGlobalObject.cpp) releases the global, runs a finalcollectNow(), then the~VMthat callslastChanceToFinalize. Workers always go through it; the main thread only withBUN_DESTRUCT_VM_ON_EXIT=1.