diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index 5af7f93e15e..ced6a756138 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 @@ -372,6 +373,10 @@ 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). + /// Sets a process-global flag; no preconditions. + safe fn napi_internal_set_cleanup_hooks_only(); } pub const HOT_RELOAD_HOT: u8 = 1; @@ -1488,7 +1493,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 +1526,24 @@ 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 + // 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() { + 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 // self.event_loop().tick(); diff --git a/src/jsc/bindings/napi.cpp b/src/jsc/bindings/napi.cpp index a7f5b8143bd..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" @@ -3034,6 +3036,26 @@ 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(). +// 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.store(true, std::memory_order_relaxed); +} + +extern "C" bool napi_internal_cleanup_is_hooks_only() +{ + return s_napiCleanupHooksOnly.load(std::memory_order_relaxed); +} + 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..6eb2b8f77e8 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,33 @@ 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. 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 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; + clearExceptionsBetweenFinalizers(); + 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); @@ -239,6 +271,7 @@ struct NapiEnv : public WTF::RefCounted { instanceDataFinalizer.call(this, instanceData, true); instanceDataFinalizer.clear(); + instanceData = nullptr; clearExceptionsBetweenFinalizers(); } 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 new file mode 100644 index 00000000000..ba88523c173 --- /dev/null +++ b/test/napi/napi-exit-cleanup.test.ts @@ -0,0 +1,130 @@ +// 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. Consequences: +// - napi_add_env_cleanup_hook hooks silently never ran under `bun test` +// (Node runs them whenever the environment tears down); +// - 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). +// +// 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"; +import { bunEnv, bunExe, canBuildNodeAddons, tempDir } 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"); + +// 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. +// +// 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 +// 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, + 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.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. + 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 () => { + using dir = tempDir("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: childEnv, + cwd: String(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("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"; + 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: childEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // 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); + }); +}); 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 diff --git a/test/no-validate-leaksan.txt b/test/no-validate-leaksan.txt index 873cc966375..2cfef3ae362 100644 --- a/test/no-validate-leaksan.txt +++ b/test/no-validate-leaksan.txt @@ -408,6 +408,19 @@ 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 + +# 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