From 87e4e76dd80514d08518cdcbd3b6f5a26953d58c Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 12 Jun 2026 02:06:27 +0000 Subject: [PATCH 01/12] Drain VM cleanup hooks in global_exit so bun test runs napi at-exit cleanup Every bun test exit site (and the --parallel worker exit) sets is_shutting_down by hand and jumps straight to global_exit(), skipping on_exit(), which was the only place that drained RareData::cleanup_hooks. Each NapiEnv registers its at-exit cleanup on that list, so under bun test: - napi_add_env_cleanup_hook hooks silently never ran - pending napi_wrap finalizers never ran at env teardown - with BUN_DESTRUCT_VM_ON_EXIT=1, the final GC deferred those finalizers as NapiFinalizerTask boxes parked on the same never-walked list, which LeakSanitizer reports at exit (intermittent exit 134 in test/regression/issue/30205.test.ts on the asan CI lane) Extract the drain loop from on_exit() into run_cleanup_hooks() and call it at the top of global_exit() as well. The drain is a no-op when on_exit() already ran, and setting has_run_cleanup_hooks before destructOnExit's final collection makes NapiFinalizerTask::schedule() drop tasks enqueued mid-sweep instead of parking them. Fixes #32144 --- src/jsc/VirtualMachine.rs | 18 +++++ test/napi/napi-exit-cleanup.test.ts | 103 ++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+) create mode 100644 test/napi/napi-exit-cleanup.test.ts diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index 5af7f93e15e..24d3482b3b0 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -1488,7 +1488,16 @@ impl VirtualMachine { ExitHandler::dispatch_on_exit(self); self.is_shutting_down = true; + self.run_cleanup_hooks(); + } + /// Drain `RareData::cleanup_hooks` and set `has_run_cleanup_hooks`. + /// Every `NapiEnv` registers its at-exit cleanup on this list (env + /// cleanup hooks + pending `napi_wrap` finalizers), so it must run while + /// JSC and the global are still fully alive. Idempotent: the drained + /// list stays empty and the flag makes later `NapiFinalizerTask`s drop + /// instead of re-registering. + fn run_cleanup_hooks(&mut self) { // Make sure we run new cleanup hooks introduced by running cleanup // hooks. // Note: each iteration re-fetches `rare_data` so the FFI hook @@ -1512,6 +1521,15 @@ impl VirtualMachine { pub fn global_exit(&mut self) -> ! { debug_assert!(self.is_shutting_down()); + // Exit paths that never go through `on_exit()` (the test runner and + // its --parallel workers jump straight here) must still drain the + // cleanup hooks. Skipping the drain means napi env cleanup hooks and + // at-exit `napi_wrap` finalizers never run, and it leaves + // `has_run_cleanup_hooks` false, so `NapiFinalizerTask::schedule()` + // parks every finalizer deferred by `destructOnExit`'s final + // collection on a list that is never walked again (an LSAN-visible + // leak of the task boxes). No-op when `on_exit()` already ran. + self.run_cleanup_hooks(); // FIXME: we should be doing this, but we're not, but unfortunately // doing it causes like 50+ tests to break // self.event_loop().tick(); diff --git a/test/napi/napi-exit-cleanup.test.ts b/test/napi/napi-exit-cleanup.test.ts new file mode 100644 index 00000000000..da56226e5d8 --- /dev/null +++ b/test/napi/napi-exit-cleanup.test.ts @@ -0,0 +1,103 @@ +// https://github.com/oven-sh/bun/issues/32144 +// +// Every `bun test` exit site (and the --parallel worker exit in +// test/parallel/runner.rs) jumped straight to VirtualMachine::global_exit() +// without draining RareData::cleanup_hooks, the list on which each NapiEnv +// registers its at-exit cleanup (NapiEnv::cleanup: env cleanup hooks + +// pending napi_wrap finalizers). Consequences: +// - napi_add_env_cleanup_hook hooks silently never ran under `bun test` +// (Node runs them whenever the environment tears down); +// - napi_wrap at-exit finalizers never ran, so with +// BUN_DESTRUCT_VM_ON_EXIT=1 the final GC deferred them as +// NapiFinalizerTasks parked on that same never-walked list, which +// LeakSanitizer reports at exit (the intermittent exit-134 failures of +// test/regression/issue/30205.test.ts on the asan CI lane). +// +// These two tests are the deterministic, build-independent half: they fail +// on an unfixed build on every platform because the hook/finalizer output +// never appears. They live in their own file (rather than napi.test.ts) +// only because that 100-test concurrent suite is too heavy to run as a +// whole alongside these. + +import { spawn, spawnSync } from "bun"; +import { beforeAll, describe, expect, test } from "bun:test"; +import { bunEnv, bunExe, tempDirWithFiles } from "harness"; +import { existsSync } from "node:fs"; +import { join } from "node:path"; + +const napiAppDir = join(import.meta.dir, "napi-app"); +const hookAddon = join(napiAppDir, "build", "Debug", "test_cleanup_hook_order.node"); +const wrapAddon = join(napiAppDir, "build", "Debug", "test_wrap_cleanup_order.node"); + +describe.concurrent("napi cleanup at bun test exit", () => { + beforeAll(() => { + if (existsSync(hookAddon) && existsSync(wrapAddon)) return; + // Same one-shot build pattern as test/regression/issue/30205.test.ts. + const install = spawnSync({ + cmd: [bunExe(), "install", "--verbose"], + cwd: napiAppDir, + env: bunEnv, + stderr: "inherit", + stdout: "inherit", + stdin: "inherit", + }); + if (!install.success) throw new Error("node-gyp build failed"); + }, 120_000); + + test("napi env cleanup hooks run when the process exits from bun test", async () => { + const dir = tempDirWithFiles("napi-cleanup-hooks-bun-test", { + "hooks.test.js": ` + import { test, expect } from "bun:test"; + const addon = require(${JSON.stringify(hookAddon)}); + addon.test(); + test("registers env cleanup hooks", () => expect(1).toBe(1)); + `, + }); + await using proc = spawn({ + cmd: [bunExe(), "test", "hooks.test.js"], + env: bunEnv, + cwd: dir, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // The addon registers hooks 1, 2, 3; they must fire at exit in reverse + // order, same as when the process exits from `bun run`. + expect(stdout).toContain("hook3 executed at position 0"); + expect(stdout).toContain("hook2 executed at position 1"); + expect(stdout).toContain("hook1 executed at position 2"); + expect(stderr).toContain("1 pass"); + expect(exitCode).toBe(0); + }); + + test("napi_wrap finalizers run during env teardown when exiting from bun test", async () => { + const dir = tempDirWithFiles("napi-wrap-teardown-bun-test", { + "wrap.test.js": ` + import { test, expect } from "bun:test"; + const addon = require(${JSON.stringify(wrapAddon)}); + globalThis.keep = addon.createParentAndChildren(32); + test("wraps stay rooted until exit", () => expect(globalThis.keep.length).toBe(33)); + `, + }); + await using proc = spawn({ + cmd: [bunExe(), "test", "wrap.test.js"], + env: bunEnv, + cwd: dir, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // The addon prints the accumulated LIFO order once the parent (id 0) is + // finalized; stdout also carries the test runner's version banner, so + // assert the exact line rather than the whole stream. + expect(stdout).toContain( + "finalize order: " + + Array.from({ length: 32 }, (_, i) => 32 - i) + .concat(0) + .join(" ") + + "\n", + ); + expect(stderr).toContain("1 pass"); + expect(exitCode).toBe(0); + }); +}); From a6cc20ae4eb5ead89a525ef2367db3a8a2a639a3 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 12 Jun 2026 02:12:54 +0000 Subject: [PATCH 02/12] test: use disposable tempDir fixture so temp dirs clean up on failure --- test/napi/napi-exit-cleanup.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/napi/napi-exit-cleanup.test.ts b/test/napi/napi-exit-cleanup.test.ts index da56226e5d8..f4d6777cd15 100644 --- a/test/napi/napi-exit-cleanup.test.ts +++ b/test/napi/napi-exit-cleanup.test.ts @@ -21,7 +21,7 @@ import { spawn, spawnSync } from "bun"; import { beforeAll, describe, expect, test } from "bun:test"; -import { bunEnv, bunExe, tempDirWithFiles } from "harness"; +import { bunEnv, bunExe, tempDir } from "harness"; import { existsSync } from "node:fs"; import { join } from "node:path"; @@ -45,7 +45,7 @@ describe.concurrent("napi cleanup at bun test exit", () => { }, 120_000); test("napi env cleanup hooks run when the process exits from bun test", async () => { - const dir = tempDirWithFiles("napi-cleanup-hooks-bun-test", { + using dir = tempDir("napi-cleanup-hooks-bun-test", { "hooks.test.js": ` import { test, expect } from "bun:test"; const addon = require(${JSON.stringify(hookAddon)}); @@ -56,7 +56,7 @@ describe.concurrent("napi cleanup at bun test exit", () => { await using proc = spawn({ cmd: [bunExe(), "test", "hooks.test.js"], env: bunEnv, - cwd: dir, + cwd: String(dir), stdout: "pipe", stderr: "pipe", }); @@ -71,7 +71,7 @@ describe.concurrent("napi cleanup at bun test exit", () => { }); test("napi_wrap finalizers run during env teardown when exiting from bun test", async () => { - const dir = tempDirWithFiles("napi-wrap-teardown-bun-test", { + using dir = tempDir("napi-wrap-teardown-bun-test", { "wrap.test.js": ` import { test, expect } from "bun:test"; const addon = require(${JSON.stringify(wrapAddon)}); @@ -82,7 +82,7 @@ describe.concurrent("napi cleanup at bun test exit", () => { await using proc = spawn({ cmd: [bunExe(), "test", "wrap.test.js"], env: bunEnv, - cwd: dir, + cwd: String(dir), stdout: "pipe", stderr: "pipe", }); From e7957ef77a3c2a2a6bce78a25de71c616e330970 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 12 Jun 2026 02:34:53 +0000 Subject: [PATCH 03/12] Update has_run_cleanup_hooks doc and make wrap test CRLF-safe on Windows --- src/jsc/VirtualMachine.rs | 3 ++- test/napi/napi-exit-cleanup.test.ts | 7 ++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index 24d3482b3b0..1581b925093 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -209,7 +209,8 @@ pub struct VirtualMachine { pub is_printing_plugin: bool, pub is_shutting_down: bool, - /// Set once `on_exit()` has finished draining `RareData::cleanup_hooks`. + /// Set once `run_cleanup_hooks()` (called from `on_exit()` and + /// `global_exit()`) has finished draining `RareData::cleanup_hooks`. /// After this point the cleanup-hook list is never iterated again, so /// pushing to it (e.g. from a deferred N-API finalizer scheduled during /// the final `collectNow()` in `Zig__GlobalObject__destructOnExit`) would diff --git a/test/napi/napi-exit-cleanup.test.ts b/test/napi/napi-exit-cleanup.test.ts index f4d6777cd15..135e347def6 100644 --- a/test/napi/napi-exit-cleanup.test.ts +++ b/test/napi/napi-exit-cleanup.test.ts @@ -89,13 +89,14 @@ describe.concurrent("napi cleanup at bun test exit", () => { const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); // The addon prints the accumulated LIFO order once the parent (id 0) is // finalized; stdout also carries the test runner's version banner, so - // assert the exact line rather than the whole stream. + // assert the exact line rather than the whole stream. No trailing + // newline in the needle: the addon's printf("\n") arrives as CRLF on + // Windows (text-mode CRT stdout). expect(stdout).toContain( "finalize order: " + Array.from({ length: 32 }, (_, i) => 32 - i) .concat(0) - .join(" ") + - "\n", + .join(" "), ); expect(stderr).toContain("1 pass"); expect(exitCode).toBe(0); From b239b1326895412559dd82b65cef777c8ff46399 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 12 Jun 2026 02:42:58 +0000 Subject: [PATCH 04/12] ci: retrigger From b32c64948aa18607fce807215b111afb1c17c763 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 12 Jun 2026 04:20:50 +0000 Subject: [PATCH 05/12] test: strip exception validator from child env, sweep stale on_exit doc --- src/runtime/napi/napi_body.rs | 6 +++--- test/napi/napi-exit-cleanup.test.ts | 16 ++++++++++++++-- test/no-validate-exceptions.txt | 1 + 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/runtime/napi/napi_body.rs b/src/runtime/napi/napi_body.rs index 0f6d2508c95..fb2866aa62d 100644 --- a/src/runtime/napi/napi_body.rs +++ b/src/runtime/napi/napi_body.rs @@ -4342,9 +4342,9 @@ impl NapiFinalizerTask { if vm.is_shutting_down() { if vm.has_run_cleanup_hooks() { - // `on_exit()` already drained cleanup hooks; we are inside the - // final `collectNow()` (Heap::sweepArrayBuffers) and the JSC - // VM is being torn down. The cleanup-hook list will never be + // `run_cleanup_hooks()` already drained cleanup hooks; we are + // inside the final `collectNow()` (Heap::sweepArrayBuffers) + // and the JSC VM is being torn down. The cleanup-hook list will never be // walked again, and running the user finalizer here (mid-GC, // with the global about to be freed) is unsafe. Drop the task // so the `Box` and its `NapiEnvRef` are diff --git a/test/napi/napi-exit-cleanup.test.ts b/test/napi/napi-exit-cleanup.test.ts index 135e347def6..98a428a2158 100644 --- a/test/napi/napi-exit-cleanup.test.ts +++ b/test/napi/napi-exit-cleanup.test.ts @@ -29,6 +29,18 @@ const napiAppDir = join(import.meta.dir, "napi-app"); const hookAddon = join(napiAppDir, "build", "Debug", "test_cleanup_hook_order.node"); const wrapAddon = join(napiAppDir, "build", "Debug", "test_wrap_cleanup_order.node"); +// CI's ASAN lane sets BUN_JSC_validateExceptionChecks=1, which leaks into the +// spawned bun processes via bunEnv. The napi addon init path has a known +// unchecked ThrowScope (see the "3rd party napi" section in +// test/no-validate-exceptions.txt), so the child aborts while loading the +// addon, before the fixture runs. Strip the validator from the child env +// only, same as test/regression/issue/30205.test.ts. +const childEnv = { + ...bunEnv, + BUN_JSC_validateExceptionChecks: undefined, + BUN_JSC_dumpSimulatedThrows: undefined, +}; + describe.concurrent("napi cleanup at bun test exit", () => { beforeAll(() => { if (existsSync(hookAddon) && existsSync(wrapAddon)) return; @@ -55,7 +67,7 @@ describe.concurrent("napi cleanup at bun test exit", () => { }); await using proc = spawn({ cmd: [bunExe(), "test", "hooks.test.js"], - env: bunEnv, + env: childEnv, cwd: String(dir), stdout: "pipe", stderr: "pipe", @@ -81,7 +93,7 @@ describe.concurrent("napi cleanup at bun test exit", () => { }); await using proc = spawn({ cmd: [bunExe(), "test", "wrap.test.js"], - env: bunEnv, + env: childEnv, cwd: String(dir), stdout: "pipe", stderr: "pipe", diff --git a/test/no-validate-exceptions.txt b/test/no-validate-exceptions.txt index 4bece98c3b8..229425f3649 100644 --- a/test/no-validate-exceptions.txt +++ b/test/no-validate-exceptions.txt @@ -73,6 +73,7 @@ test/js/third_party/prisma/prisma.test.ts test/js/third_party/remix/remix.test.ts test/js/third_party/resvg/bbox.test.js test/js/third_party/rollup-v4/rollup-v4.test.ts +test/napi/napi-exit-cleanup.test.ts test/napi/napi-finalizer-delete-ref.test.ts test/napi/uv.test.ts test/napi/uv_stub.test.ts From 4d31248a15a68cc2399d94b7f7f029683ee4c99e Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 12 Jun 2026 06:10:58 +0000 Subject: [PATCH 06/12] Skip pending napi_wrap finalizers at undrained-loop exit; run exit-GC finalizers immediately The asan lane exposed a UAF in the first version of this fix: bun test exits without draining the event loop, so an addon can still have queued async work whose teardown references other wraps (duckdb's Task destructor Unref()s the Connection it captured). Running the pending napi_wrap finalizer pass at that point deleted the Connection ObjectWrap first and the queued ConnectTask's destructor then read a freed env pointer. NapiEnv::cleanup() now runs only the explicit env cleanup hooks when the exit path never drained the loop (flag set by global_exit() before its drain); bun run exits, process.exit(), and worker teardown go through on_exit() and keep running the full cleanup as before. Node also does not guarantee wrap finalizers run at process exit. To keep the LSAN lane clean under BUN_DESTRUCT_VM_ON_EXIT, destructOnExit now requests termination before its final collection, so swept wraps' finalizers run immediately (raw callback, the same mode as lastChanceToFinalize in ~VM) instead of deferring NapiFinalizerTasks to an event loop that never ticks again; the finalizers free their native data so neither the task boxes nor the addon allocations are reported. --- src/jsc/VirtualMachine.rs | 25 +++++++++++---- src/jsc/bindings/ZigGlobalObject.cpp | 10 ++++++ src/jsc/bindings/napi.cpp | 17 ++++++++++ src/jsc/bindings/napi.h | 18 +++++++++++ test/napi/napi-exit-cleanup.test.ts | 48 ++++++++++++++-------------- 5 files changed, 88 insertions(+), 30 deletions(-) diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index 1581b925093..ef49d2cdb1c 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -373,6 +373,9 @@ unsafe extern "C" { safe fn Bun__closeAllSQLiteDatabasesForTermination(); safe fn Bun__WebView__closeAllForTermination(); safe fn Zig__GlobalObject__destructOnExit(global: &JSGlobalObject); + /// Makes `NapiEnv::cleanup()` run only the explicit env cleanup hooks, + /// skipping pending `napi_wrap` finalizers (src/jsc/bindings/napi.cpp). + fn napi_internal_set_cleanup_hooks_only(); } pub const HOT_RELOAD_HOT: u8 = 1; @@ -1524,12 +1527,22 @@ impl VirtualMachine { debug_assert!(self.is_shutting_down()); // Exit paths that never go through `on_exit()` (the test runner and // its --parallel workers jump straight here) must still drain the - // cleanup hooks. Skipping the drain means napi env cleanup hooks and - // at-exit `napi_wrap` finalizers never run, and it leaves - // `has_run_cleanup_hooks` false, so `NapiFinalizerTask::schedule()` - // parks every finalizer deferred by `destructOnExit`'s final - // collection on a list that is never walked again (an LSAN-visible - // leak of the task boxes). No-op when `on_exit()` already ran. + // cleanup hooks. Skipping the drain means napi env cleanup hooks + // never run, and it leaves `has_run_cleanup_hooks` false, so + // `NapiFinalizerTask::schedule()` parks every finalizer deferred by + // `destructOnExit`'s final collection on a list that is never walked + // again (an LSAN-visible leak of the task boxes). No-op when + // `on_exit()` already ran. + // + // Unlike `on_exit()`, these paths reach here without draining the + // event loop, so addons may still have queued async work; running + // their pending `napi_wrap` finalizers now could tear down wraps + // that the abandoned work still references (see `NapiEnv::cleanup`). + // Restrict the napi env cleanup to the explicit cleanup hooks. + if !self.has_run_cleanup_hooks() { + // SAFETY: sets a process-global flag in napi.cpp; no preconditions. + unsafe { napi_internal_set_cleanup_hooks_only() }; + } self.run_cleanup_hooks(); // FIXME: we should be doing this, but we're not, but unfortunately // doing it causes like 50+ tests to break diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index 5be04f65086..876a459d672 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -3977,6 +3977,16 @@ extern "C" void Zig__GlobalObject__destructOnExit(Zig::GlobalObject* globalObjec globalObject->requireMap()->clear(globalObject); scope.exception(); // mirror WebWorker__teardownJSCVM — leave any pending exception in place } + // Nothing enters JS after this point. Request termination so + // NapiEnv::mustDeferFinalizers() turns false: finalizers for napi wraps + // swept by the collection below run immediately (a raw callback, same as + // during lastChanceToFinalize in ~VM) instead of being deferred to an + // event loop that will never tick again. Without this, every swept wrap + // leaks its NapiFinalizerTask box plus whatever the finalizer would have + // freed, and LeakSanitizer reports both on exit paths that skipped + // on_exit()'s full napi cleanup. + vm.ensureTerminationException(); + vm.setHasTerminationRequest(); gcUnprotect(globalObject); globalObject = nullptr; vm.heap.collectNow(JSC::Sync, JSC::CollectionScope::Full); diff --git a/src/jsc/bindings/napi.cpp b/src/jsc/bindings/napi.cpp index a7f5b8143bd..c34e325d38a 100644 --- a/src/jsc/bindings/napi.cpp +++ b/src/jsc/bindings/napi.cpp @@ -3034,6 +3034,23 @@ extern "C" JS_EXPORT napi_status napi_remove_async_cleanup_hook(napi_async_clean NAPI_RETURN_SUCCESS(env); } +// Whether NapiEnv::cleanup() should run only the explicit env cleanup hooks +// and skip pending napi_wrap finalizers. Set (never cleared; the process is +// exiting) by VirtualMachine::global_exit() on exit paths that never drained +// the event loop. Process-global rather than per-VM: only the main thread's +// final exit takes this path, and worker teardown goes through on_exit(). +static bool s_napiCleanupHooksOnly = false; + +extern "C" void napi_internal_set_cleanup_hooks_only() +{ + s_napiCleanupHooksOnly = true; +} + +extern "C" bool napi_internal_cleanup_is_hooks_only() +{ + return s_napiCleanupHooksOnly; +} + extern "C" void napi_internal_cleanup_env_cpp(napi_env env) { env->cleanup(); diff --git a/src/jsc/bindings/napi.h b/src/jsc/bindings/napi.h index 73f2f639eed..889baca0697 100644 --- a/src/jsc/bindings/napi.h +++ b/src/jsc/bindings/napi.h @@ -26,6 +26,11 @@ extern "C" void napi_internal_register_cleanup_zig(napi_env env); extern "C" void napi_internal_suppress_crash_on_abort_if_desired(); extern "C" void Bun__crashHandler(const char* message, size_t message_len); +// Set by VirtualMachine::global_exit() on exit paths that never drained the +// event loop (the test runner and its --parallel workers); read by +// NapiEnv::cleanup(). +extern "C" void napi_internal_set_cleanup_hooks_only(); +extern "C" bool napi_internal_cleanup_is_hooks_only(); static bool equal(napi_async_cleanup_hook_handle, napi_async_cleanup_hook_handle); @@ -211,6 +216,19 @@ struct NapiEnv : public WTF::RefCounted { drain(); } + // When the process exits without draining the event loop (the test + // runner jumps straight to global_exit()), pending napi_wrap + // finalizers are not safe to run: an addon may have queued async + // work whose teardown touches other wraps this loop would already + // have deleted (e.g. duckdb's Task destructor Unref()s the + // Connection it captured). Run only the explicit env cleanup hooks + // above, matching Node, which does not guarantee wrap finalizers run + // at process exit. Under BUN_DESTRUCT_VM_ON_EXIT the final exit + // collection runs the remaining finalizers immediately instead (see + // Zig__GlobalObject__destructOnExit). + if (napi_internal_cleanup_is_hooks_only()) + return; + // Defer GC during entire finalizer cleanup to prevent iterator invalidation. // This prevents any GC-triggered finalizer execution while m_finalizers is being iterated. JSC::DeferGCForAWhile deferGC(m_vm); diff --git a/test/napi/napi-exit-cleanup.test.ts b/test/napi/napi-exit-cleanup.test.ts index 98a428a2158..36ce24cc38d 100644 --- a/test/napi/napi-exit-cleanup.test.ts +++ b/test/napi/napi-exit-cleanup.test.ts @@ -3,21 +3,23 @@ // Every `bun test` exit site (and the --parallel worker exit in // test/parallel/runner.rs) jumped straight to VirtualMachine::global_exit() // without draining RareData::cleanup_hooks, the list on which each NapiEnv -// registers its at-exit cleanup (NapiEnv::cleanup: env cleanup hooks + -// pending napi_wrap finalizers). Consequences: +// registers its at-exit cleanup. Consequences: // - napi_add_env_cleanup_hook hooks silently never ran under `bun test` // (Node runs them whenever the environment tears down); -// - napi_wrap at-exit finalizers never ran, so with -// BUN_DESTRUCT_VM_ON_EXIT=1 the final GC deferred them as -// NapiFinalizerTasks parked on that same never-walked list, which -// LeakSanitizer reports at exit (the intermittent exit-134 failures of -// test/regression/issue/30205.test.ts on the asan CI lane). +// - with BUN_DESTRUCT_VM_ON_EXIT=1 the final GC deferred the swept wraps' +// finalizers as NapiFinalizerTasks parked on that same never-walked +// list, which LeakSanitizer reports at exit (the intermittent exit-134 +// failures of test/regression/issue/30205.test.ts on the asan CI lane). // -// These two tests are the deterministic, build-independent half: they fail -// on an unfixed build on every platform because the hook/finalizer output -// never appears. They live in their own file (rather than napi.test.ts) -// only because that 100-test concurrent suite is too heavy to run as a -// whole alongside these. +// The fix drains the cleanup hooks in global_exit(). Pending napi_wrap +// finalizers are deliberately NOT run on this path: unlike `bun run`, the +// test runner exits without draining the event loop, so addons may still +// have queued async work whose teardown references wraps the finalizer pass +// would have deleted (duckdb's Task destructor Unref()s its Connection). +// Node does not guarantee wrap finalizers run at process exit either. +// +// This file lives apart from napi.test.ts only because that 100-test +// concurrent suite is too heavy to run as a whole alongside these. import { spawn, spawnSync } from "bun"; import { beforeAll, describe, expect, test } from "bun:test"; @@ -82,7 +84,14 @@ describe.concurrent("napi cleanup at bun test exit", () => { expect(exitCode).toBe(0); }); - test("napi_wrap finalizers run during env teardown when exiting from bun test", async () => { + test("pending napi_wrap finalizers are skipped at bun test exit", async () => { + // Negative contract for the hooks-only drain: wraps still rooted when + // `bun test` exits must NOT have their finalizers run (the event loop + // was never drained, so running them can touch wraps that abandoned + // async work still references), and the process must exit cleanly. The + // equivalent `bun run` exit does run them; that behavior is covered by + // "napi_wrap finalizers run in LIFO order during env teardown" in + // napi.test.ts. using dir = tempDir("napi-wrap-teardown-bun-test", { "wrap.test.js": ` import { test, expect } from "bun:test"; @@ -99,17 +108,8 @@ describe.concurrent("napi cleanup at bun test exit", () => { stderr: "pipe", }); const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - // The addon prints the accumulated LIFO order once the parent (id 0) is - // finalized; stdout also carries the test runner's version banner, so - // assert the exact line rather than the whole stream. No trailing - // newline in the needle: the addon's printf("\n") arrives as CRLF on - // Windows (text-mode CRT stdout). - expect(stdout).toContain( - "finalize order: " + - Array.from({ length: 32 }, (_, i) => 32 - i) - .concat(0) - .join(" "), - ); + // The addon prints "finalize order: ..." if any wrap finalizer runs. + expect(stdout).not.toContain("finalize order:"); expect(stderr).toContain("1 pass"); expect(exitCode).toBe(0); }); From 04e49b2cb9a028c2c936ffd3c201e2c717eeb994 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 12 Jun 2026 06:23:19 +0000 Subject: [PATCH 07/12] Run the napi_set_instance_data finalizer in hooks-only exit cleanup It is part of env teardown in Node, and nothing on the hooks-only path has been freed, so it cannot observe the dangling state the skipped wrap-finalizer pass avoids. --- src/jsc/bindings/napi.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/jsc/bindings/napi.h b/src/jsc/bindings/napi.h index 889baca0697..6a7c9cce271 100644 --- a/src/jsc/bindings/napi.h +++ b/src/jsc/bindings/napi.h @@ -226,8 +226,15 @@ struct NapiEnv : public WTF::RefCounted { // at process exit. Under BUN_DESTRUCT_VM_ON_EXIT the final exit // collection runs the remaining finalizers immediately instead (see // Zig__GlobalObject__destructOnExit). - if (napi_internal_cleanup_is_hooks_only()) + if (napi_internal_cleanup_is_hooks_only()) { + // The napi_set_instance_data() finalizer is part of env teardown + // in Node and is still safe here: nothing on this path has been + // freed, so it cannot observe the dangling state the skipped + // wrap-finalizer pass avoids. + instanceDataFinalizer.call(this, instanceData, true); + instanceDataFinalizer.clear(); return; + } // Defer GC during entire finalizer cleanup to prevent iterator invalidation. // This prevents any GC-triggered finalizer execution while m_finalizers is being iterated. From c6f4dbdd853b15dd219becb470a93af7ad0db471 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 12 Jun 2026 06:46:31 +0000 Subject: [PATCH 08/12] Pin exit-cleanup tests to the non-destruct path; make hooks-only flag atomic The asan runner sets BUN_DESTRUCT_VM_ON_EXIT=1 and detect_leaks for files not in test/no-validate-leaksan.txt, and both leak into spawned children via bunEnv. Under destruct-vm the final exit collection deliberately runs the pending wrap finalizers (in sweep order), which would break the negative assertion that they are skipped on the normal exit path. Strip those vars from the child env and list the file in no-validate-leaksan.txt. s_napiCleanupHooksOnly is written by the main thread while worker threads can still be inside their own NapiEnv::cleanup(); use an atomic with relaxed ordering so the concurrent read is not UB. --- src/jsc/bindings/napi.cpp | 11 ++++++++--- test/napi/napi-exit-cleanup.test.ts | 12 ++++++++++++ test/no-validate-leaksan.txt | 5 +++++ 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/jsc/bindings/napi.cpp b/src/jsc/bindings/napi.cpp index c34e325d38a..75b91a98f7f 100644 --- a/src/jsc/bindings/napi.cpp +++ b/src/jsc/bindings/napi.cpp @@ -1,5 +1,7 @@ #include "BunProcess.h" #include "headers.h" + +#include #include "node_api.h" #include "root.h" #include "JavaScriptCore/ConstructData.h" @@ -3039,16 +3041,19 @@ extern "C" JS_EXPORT napi_status napi_remove_async_cleanup_hook(napi_async_clean // exiting) by VirtualMachine::global_exit() on exit paths that never drained // the event loop. Process-global rather than per-VM: only the main thread's // final exit takes this path, and worker teardown goes through on_exit(). -static bool s_napiCleanupHooksOnly = false; +// Atomic because worker threads can be mid-teardown (their own on_exit() -> +// NapiEnv::cleanup()) while the main thread stores the flag; relaxed is +// enough since either value read is safe at exit. +static std::atomic s_napiCleanupHooksOnly { false }; extern "C" void napi_internal_set_cleanup_hooks_only() { - s_napiCleanupHooksOnly = true; + s_napiCleanupHooksOnly.store(true, std::memory_order_relaxed); } extern "C" bool napi_internal_cleanup_is_hooks_only() { - return s_napiCleanupHooksOnly; + return s_napiCleanupHooksOnly.load(std::memory_order_relaxed); } extern "C" void napi_internal_cleanup_env_cpp(napi_env env) diff --git a/test/napi/napi-exit-cleanup.test.ts b/test/napi/napi-exit-cleanup.test.ts index 36ce24cc38d..5c7e61efbb1 100644 --- a/test/napi/napi-exit-cleanup.test.ts +++ b/test/napi/napi-exit-cleanup.test.ts @@ -37,10 +37,22 @@ const wrapAddon = join(napiAppDir, "build", "Debug", "test_wrap_cleanup_order.no // test/no-validate-exceptions.txt), so the child aborts while loading the // addon, before the fixture runs. Strip the validator from the child env // only, same as test/regression/issue/30205.test.ts. +// +// BUN_DESTRUCT_VM_ON_EXIT is stripped too: these tests pin the normal exit +// path, where pending wrap finalizers are skipped. Under destruct-vm the +// final exit collection deliberately runs them (in sweep order, not LIFO), +// which would print "finalize order:" and break the negative assertion +// below. This file is also in test/no-validate-leaksan.txt so the asan +// runner does not set destruct-vm or detect_leaks for it in the first place. const childEnv = { ...bunEnv, BUN_JSC_validateExceptionChecks: undefined, BUN_JSC_dumpSimulatedThrows: undefined, + BUN_DESTRUCT_VM_ON_EXIT: undefined, + // detect_leaks aborts debug builds on known-benign exit allocations and is + // not what these tests measure; drop back to bun's baked ASAN defaults. + ASAN_OPTIONS: undefined, + LSAN_OPTIONS: undefined, }; describe.concurrent("napi cleanup at bun test exit", () => { diff --git a/test/no-validate-leaksan.txt b/test/no-validate-leaksan.txt index 873cc966375..95b306c4051 100644 --- a/test/no-validate-leaksan.txt +++ b/test/no-validate-leaksan.txt @@ -408,6 +408,11 @@ test/napi/node-napi-tests/test/js-native-api/6_object_wrap/do.test.ts # LSAN per-spawn adds no coverage and slows the file significantly. test/napi/uv_stub.test.ts +# Spawns bun test children that pin the non-destruct exit path; destruct-vm +# and detect_leaks would leak into them via bunEnv and change the behavior +# under test (see the childEnv comment in the file). +test/napi/napi-exit-cleanup.test.ts + test/bake/dev/production.test.ts test/js/third_party/pg-gateway/pglite.test.ts test/js/bun/test/parallel/test-integration-rspack.ts From aa88238723cde9071ec12cef3a2307f861db75e5 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 12 Jun 2026 07:08:34 +0000 Subject: [PATCH 09/12] Null instanceData after its finalizer runs; use safe fn for the flag setter Under BUN_DESTRUCT_VM_ON_EXIT the exit collection runs swept wraps' finalizers after NapiEnv::cleanup() already ran the instance-data finalizer on the hooks-only path; a wrap finalizer calling napi_get_instance_data would see the freed pointer. Null it so they see null instead, and do the same in the full-cleanup branch for the finalizers the exit collection can still run afterward. --- src/jsc/VirtualMachine.rs | 6 +++--- src/jsc/bindings/napi.h | 7 ++++++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index ef49d2cdb1c..ced6a756138 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -375,7 +375,8 @@ unsafe extern "C" { safe fn Zig__GlobalObject__destructOnExit(global: &JSGlobalObject); /// Makes `NapiEnv::cleanup()` run only the explicit env cleanup hooks, /// skipping pending `napi_wrap` finalizers (src/jsc/bindings/napi.cpp). - fn napi_internal_set_cleanup_hooks_only(); + /// Sets a process-global flag; no preconditions. + safe fn napi_internal_set_cleanup_hooks_only(); } pub const HOT_RELOAD_HOT: u8 = 1; @@ -1540,8 +1541,7 @@ impl VirtualMachine { // that the abandoned work still references (see `NapiEnv::cleanup`). // Restrict the napi env cleanup to the explicit cleanup hooks. if !self.has_run_cleanup_hooks() { - // SAFETY: sets a process-global flag in napi.cpp; no preconditions. - unsafe { napi_internal_set_cleanup_hooks_only() }; + napi_internal_set_cleanup_hooks_only(); } self.run_cleanup_hooks(); // FIXME: we should be doing this, but we're not, but unfortunately diff --git a/src/jsc/bindings/napi.h b/src/jsc/bindings/napi.h index 6a7c9cce271..83b28d37346 100644 --- a/src/jsc/bindings/napi.h +++ b/src/jsc/bindings/napi.h @@ -230,9 +230,13 @@ struct NapiEnv : public WTF::RefCounted { // The napi_set_instance_data() finalizer is part of env teardown // in Node and is still safe here: nothing on this path has been // freed, so it cannot observe the dangling state the skipped - // wrap-finalizer pass avoids. + // wrap-finalizer pass avoids. Null the pointer so finalizers that + // run later (the exit collection under BUN_DESTRUCT_VM_ON_EXIT + // runs swept wraps' finalizers immediately) see null from + // napi_get_instance_data instead of freed memory. instanceDataFinalizer.call(this, instanceData, true); instanceDataFinalizer.clear(); + instanceData = nullptr; return; } @@ -264,6 +268,7 @@ struct NapiEnv : public WTF::RefCounted { instanceDataFinalizer.call(this, instanceData, true); instanceDataFinalizer.clear(); + instanceData = nullptr; clearExceptionsBetweenFinalizers(); } From dd4933507db5414817fdc2777570ce6111f15547 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 12 Jun 2026 07:36:30 +0000 Subject: [PATCH 10/12] Drop deferred exit-GC finalizer tasks instead of running them mid-sweep The termination request in destructOnExit made swept napi wraps run their finalizers immediately during the sweep; addons whose finalizers call JS-heap-touching napi functions (test_cannot_run_js) then trip JSC's validateIsNotSweeping assertion on the asan lane. Deferral exists exactly for those finalizers, so keep it: the deferred NapiFinalizerTasks now hit schedule()'s drop branch (the cleanup hooks already ran), which frees the task boxes from issue 32144. The addon's own wrap payloads are reclaimed by the OS instead of freed, so 30205.test.ts joins no-validate-leaksan.txt alongside the other tests whose deliberate at-exit non-cleanup LSAN would report. --- src/jsc/bindings/ZigGlobalObject.cpp | 10 ---------- src/jsc/bindings/napi.h | 16 +++++++++------- test/no-validate-leaksan.txt | 8 ++++++++ 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index 876a459d672..5be04f65086 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -3977,16 +3977,6 @@ extern "C" void Zig__GlobalObject__destructOnExit(Zig::GlobalObject* globalObjec globalObject->requireMap()->clear(globalObject); scope.exception(); // mirror WebWorker__teardownJSCVM — leave any pending exception in place } - // Nothing enters JS after this point. Request termination so - // NapiEnv::mustDeferFinalizers() turns false: finalizers for napi wraps - // swept by the collection below run immediately (a raw callback, same as - // during lastChanceToFinalize in ~VM) instead of being deferred to an - // event loop that will never tick again. Without this, every swept wrap - // leaks its NapiFinalizerTask box plus whatever the finalizer would have - // freed, and LeakSanitizer reports both on exit paths that skipped - // on_exit()'s full napi cleanup. - vm.ensureTerminationException(); - vm.setHasTerminationRequest(); gcUnprotect(globalObject); globalObject = nullptr; vm.heap.collectNow(JSC::Sync, JSC::CollectionScope::Full); diff --git a/src/jsc/bindings/napi.h b/src/jsc/bindings/napi.h index 83b28d37346..0cce0859b82 100644 --- a/src/jsc/bindings/napi.h +++ b/src/jsc/bindings/napi.h @@ -223,17 +223,19 @@ struct NapiEnv : public WTF::RefCounted { // have deleted (e.g. duckdb's Task destructor Unref()s the // Connection it captured). Run only the explicit env cleanup hooks // above, matching Node, which does not guarantee wrap finalizers run - // at process exit. Under BUN_DESTRUCT_VM_ON_EXIT the final exit - // collection runs the remaining finalizers immediately instead (see - // Zig__GlobalObject__destructOnExit). + // at process exit. Wraps swept by a later collection (the exit GC + // under BUN_DESTRUCT_VM_ON_EXIT) still defer their finalizers, and + // NapiFinalizerTask::schedule() drops those tasks because the VM + // cleanup hooks already ran, so only the addon's own allocations are + // left for the OS to reclaim. if (napi_internal_cleanup_is_hooks_only()) { // The napi_set_instance_data() finalizer is part of env teardown // in Node and is still safe here: nothing on this path has been // freed, so it cannot observe the dangling state the skipped - // wrap-finalizer pass avoids. Null the pointer so finalizers that - // run later (the exit collection under BUN_DESTRUCT_VM_ON_EXIT - // runs swept wraps' finalizers immediately) see null from - // napi_get_instance_data instead of freed memory. + // wrap-finalizer pass avoids. Null the pointer so any finalizer + // that still runs later (lastChanceToFinalize calls them + // immediately) sees null from napi_get_instance_data instead of + // freed memory. instanceDataFinalizer.call(this, instanceData, true); instanceDataFinalizer.clear(); instanceData = nullptr; diff --git a/test/no-validate-leaksan.txt b/test/no-validate-leaksan.txt index 95b306c4051..2cfef3ae362 100644 --- a/test/no-validate-leaksan.txt +++ b/test/no-validate-leaksan.txt @@ -413,6 +413,14 @@ test/napi/uv_stub.test.ts # under test (see the childEnv comment in the file). test/napi/napi-exit-cleanup.test.ts +# The inner `bun test --isolate/--parallel` processes inherit destruct-vm and +# detect_leaks via bunEnv. Their last file's napi wraps are deliberately not +# finalized at exit (NapiEnv::cleanup runs hooks only on the test-runner exit +# path), so the addon's own 4-byte wrap payloads are reclaimed by the OS, not +# freed, and LSAN reports them. The NapiFinalizerTask boxes from issue 32144 +# no longer leak: schedule() drops them once the cleanup hooks have run. +test/regression/issue/30205.test.ts + test/bake/dev/production.test.ts test/js/third_party/pg-gateway/pglite.test.ts test/js/bun/test/parallel/test-integration-rspack.ts From 83134c2c99ce85ba71d5bc9eafa5701f22bd5cbe Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 12 Jun 2026 08:46:19 +0000 Subject: [PATCH 11/12] test: update childEnv comment for the post-revert destruct-vm behavior --- test/napi/napi-exit-cleanup.test.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/napi/napi-exit-cleanup.test.ts b/test/napi/napi-exit-cleanup.test.ts index 5c7e61efbb1..48dc51db8fb 100644 --- a/test/napi/napi-exit-cleanup.test.ts +++ b/test/napi/napi-exit-cleanup.test.ts @@ -40,10 +40,12 @@ const wrapAddon = join(napiAppDir, "build", "Debug", "test_wrap_cleanup_order.no // // BUN_DESTRUCT_VM_ON_EXIT is stripped too: these tests pin the normal exit // path, where pending wrap finalizers are skipped. Under destruct-vm the -// final exit collection deliberately runs them (in sweep order, not LIFO), -// which would print "finalize order:" and break the negative assertion -// below. This file is also in test/no-validate-leaksan.txt so the asan -// runner does not set destruct-vm or detect_leaks for it in the first place. +// exit collection defers swept wraps' finalizers (and schedule() drops the +// tasks), but wraps that survive the collection can still be finalized +// immediately by ~VM's lastChanceToFinalize, which would print +// "finalize order:" and break the negative assertion below. This file is +// also in test/no-validate-leaksan.txt so the asan runner does not set +// destruct-vm or detect_leaks for it in the first place. const childEnv = { ...bunEnv, BUN_JSC_validateExceptionChecks: undefined, From 3e68a1f378e8c51be20cb9c66387c3d226c5c4ed Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 8 Jul 2026 11:20:52 +0000 Subject: [PATCH 12/12] Gate napi-exit-cleanup on canBuildNodeAddons; clear exceptions in hooks-only branch - Skip the describe when node addons can't be built (older macOS toolchains), matching every sibling node-gyp test including 30205.test.ts, so the build failure surfaces as SKIP not FAIL. - Clear pending exceptions after the instance-data finalizer in the hooks-only branch too, matching the non-hooks-only tail (the #30286 invariant that each finalizer starts from a clean exception state). --- src/jsc/bindings/napi.h | 1 + test/napi/napi-exit-cleanup.test.ts | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/jsc/bindings/napi.h b/src/jsc/bindings/napi.h index 0cce0859b82..6eb2b8f77e8 100644 --- a/src/jsc/bindings/napi.h +++ b/src/jsc/bindings/napi.h @@ -239,6 +239,7 @@ struct NapiEnv : public WTF::RefCounted { instanceDataFinalizer.call(this, instanceData, true); instanceDataFinalizer.clear(); instanceData = nullptr; + clearExceptionsBetweenFinalizers(); return; } diff --git a/test/napi/napi-exit-cleanup.test.ts b/test/napi/napi-exit-cleanup.test.ts index 48dc51db8fb..ba88523c173 100644 --- a/test/napi/napi-exit-cleanup.test.ts +++ b/test/napi/napi-exit-cleanup.test.ts @@ -23,7 +23,7 @@ import { spawn, spawnSync } from "bun"; import { beforeAll, describe, expect, test } from "bun:test"; -import { bunEnv, bunExe, tempDir } from "harness"; +import { bunEnv, bunExe, canBuildNodeAddons, tempDir } from "harness"; import { existsSync } from "node:fs"; import { join } from "node:path"; @@ -57,7 +57,7 @@ const childEnv = { LSAN_OPTIONS: undefined, }; -describe.concurrent("napi cleanup at bun test exit", () => { +describe.concurrent.skipIf(!canBuildNodeAddons())("napi cleanup at bun test exit", () => { beforeAll(() => { if (existsSync(hookAddon) && existsSync(wrapAddon)) return; // Same one-shot build pattern as test/regression/issue/30205.test.ts.