napi: treat finalizers fired by VM destruction as running from GC

src/jsc/bindings/napi.h

@@ -387,9 +387,15 @@ struct NapiEnv : public WTF::RefCounted<NapiEnv> {
         delete handle;
     }
 
+    // True when a finalizer running right now would be running from the garbage collector.
+    // ~VM (a Worker exiting, or BUN_DESTRUCT_VM_ON_EXIT) is the second such context besides a
+    // collection: Heap::lastChanceToFinalize fires every finalizer still registered, but runs
+    // the weak-handle ones, and the destructors of precise allocations (the first few cells of
+    // every IsoSubspace), without the Sweeping mutator state isCollectorBusyOnCurrentThread()
+    // keys on. It sets isShuttingDown() first.
     bool inGC() const
     {
-        return this->vm().isCollectorBusyOnCurrentThread();
+        return this->vm().isCollectorBusyOnCurrentThread() || this->vm().heap.isShuttingDown();
     }
 
     void checkGC() const
@@ -490,11 +496,12 @@ struct NapiEnv : public WTF::RefCounted<NapiEnv> {
         // The deferred path (NapiFinalizerTask::schedule) is responsible for the
         // shutdown case: once is_shutting_down() it either pushes a cleanup hook
         // (if those haven't run yet) or drops the task (if they have). Running a
-        // non-EXPERIMENTAL finalizer immediately during the final collectNow() is
-        // never safe — by then on_exit() has already run cleanup hooks (including
-        // the napi_set_instance_data finalizer that frees per-addon state the
-        // object finalizer reads), the heap is sweeping (no allocation, no handle
-        // scope), and napi_call_function returns the termination exception.
+        // non-EXPERIMENTAL finalizer immediately during the final collectNow(), or
+        // during the ~VM that follows it (inGC() reports both), is never safe: by
+        // then on_exit() has already run cleanup hooks (including the
+        // napi_set_instance_data finalizer that frees per-addon state the object
+        // finalizer reads), the heap is sweeping (no allocation, no handle scope),
+        // and napi_call_function returns the termination exception.
         return m_napiModule.nm_version != NAPI_VERSION_EXPERIMENTAL;
     }
 

src/jsc/bindings/napi_handle_scope.cpp

@@ -103,7 +103,10 @@ NapiHandleScopeImpl* NapiHandleScope::open(Zig::GlobalObject* globalObject, bool
     // 2. Do an allocation in a hot code path
     // 3. the napi_ref finalizer is called while the constructor is running
     // 4. The finalizer creates a new handle scope (yes, it should not do that. No, we can't change that.)
-    if (vm.heap.mutatorState() == JSC::MutatorState::Sweeping) {
+    //
+    // Same for a finalizer fired by ~VM (Heap::lastChanceToFinalize): the allocators are
+    // already stopped, but mutatorState() does not say Sweeping there (see NapiEnv::inGC()).
+    if (vm.heap.mutatorState() == JSC::MutatorState::Sweeping || vm.heap.isShuttingDown()) {
         return nullptr;
     }
 

test/napi/napi-app/binding.gyp

@@ -297,5 +297,28 @@
                 "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1",
             ],
         },
+        {
+            "target_name": "test_vm_teardown_finalizers",
+            "sources": ["test_vm_teardown_finalizers.c"],
+            "include_dirs": ["<!@(node -p \"require('node-addon-api').include\")"],
+            "libraries": [],
+            "dependencies": ["<!(node -p \"require('node-addon-api').gyp\")"],
+            "defines": [
+                "NAPI_DISABLE_CPP_EXCEPTIONS",
+                "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1",
+            ],
+        },
+        {
+            "target_name": "test_vm_teardown_finalizers_experimental",
+            "sources": ["test_vm_teardown_finalizers.c"],
+            "include_dirs": ["<!@(node -p \"require('node-addon-api').include\")"],
+            "libraries": [],
+            "dependencies": ["<!(node -p \"require('node-addon-api').gyp\")"],
+            "defines": [
+                "NAPI_DISABLE_CPP_EXCEPTIONS",
+                "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1",
+                "TEST_EXPERIMENTAL=1",
+            ],
+        },
     ]
 }

test/napi/napi-app/test_vm_teardown_finalizers.c

@@ -0,0 +1,157 @@
+// Finalizers that only the destruction of the JSC VM can reach.
+//
+// setup(kind) registers one finalizer of the given kind and then pins its
+// object with a strong napi_ref that is never released, so no garbage
+// collection can run the finalizer while the env is alive. Env teardown
+// (NapiEnv::cleanup, whose last step is the instance data finalizer below)
+// does not run these kinds either, so the finalizer is still registered when
+// the JSC VM is destroyed afterwards: a worker_threads Worker exiting, or the
+// main thread exiting under BUN_DESTRUCT_VM_ON_EXIT=1. JSC's
+// Heap::lastChanceToFinalize then fires every remaining finalizer, without
+// the bookkeeping a collection has (MutatorState::Sweeping), so Bun must
+// recognize that state on its own.
+//
+// Built twice (binding.gyp): as a regular module, and with TEST_EXPERIMENTAL
+// as a NAPI_EXPERIMENTAL module. Whatever the finalizer reports on stderr
+// starts with "FAIL:".
+//   - Regular module: a finalizer may call any Node-API function, which cannot
+//     be honored while the heap is being destroyed, so it must not be invoked
+//     from there at all.
+//   - Experimental module: finalizers run synchronously from whatever frees
+//     their object and must not call functions that may affect GC state (Node
+//     aborts with "FATAL ERROR" when they do). The finalizer calls one such
+//     function, napi_get_undefined; Bun has to abort rather than let it through.
+
+#ifdef TEST_EXPERIMENTAL
+#define NAPI_EXPERIMENTAL
+#define NODE_API_EXPERIMENTAL_NO_WARNING
+#endif
+
+#include <js_native_api.h>
+#include <node_api.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#define NODE_API_CALL(env, call)                                               \
+  do {                                                                         \
+    if ((call) != napi_ok) {                                                   \
+      napi_throw_error((env), NULL, #call " failed");                          \
+      return NULL;                                                             \
+    }                                                                          \
+  } while (0)
+
+static int env_torn_down = 0;
+static int pending_finalizers = 0;
+static int instance_data;
+
+static void instance_data_finalizer(napi_env env, void *data, void *hint) {
+  (void)env;
+  (void)data;
+  (void)hint;
+  env_torn_down = 1;
+  printf("env teardown: %d finalizer(s) still pending\n", pending_finalizers);
+  fflush(stdout);
+}
+
+// Registered as the finalize callback of every kind; `hint` is the kind name.
+static void finalizer(napi_env env, void *data, void *hint) {
+  const char *kind = hint;
+  (void)data;
+  pending_finalizers--;
+
+  if (!env_torn_down) {
+    printf("finalizer: %s\n", kind);
+    fflush(stdout);
+    return;
+  }
+
+  // Only VM destruction gets here.
+#ifdef TEST_EXPERIMENTAL
+  // Bun has to refuse this (it aborts, as during a collection); returning from
+  // it means the call went through.
+  napi_value undefined;
+  napi_status status = napi_get_undefined(env, &undefined);
+  fprintf(stderr,
+          "FAIL: %s finalizer called napi_get_undefined during VM destruction "
+          "and got status %d\n",
+          kind, (int)status);
+#else
+  fprintf(stderr, "FAIL: %s finalizer ran after env teardown\n", kind);
+  // What node-addon-api's finalizer wrapper does before anything else; here it
+  // allocates a JSC cell in the heap being destroyed.
+  napi_handle_scope scope;
+  if (napi_open_handle_scope(env, &scope) == napi_ok) {
+    napi_close_handle_scope(env, scope);
+  }
+#endif
+}
+
+static char *dup_kind(const char *kind) {
+  // Leaked on purpose: it is the finalize hint, read whenever the finalizer
+  // runs, including from VM destruction.
+  char *copy = malloc(strlen(kind) + 1);
+  strcpy(copy, kind);
+  return copy;
+}
+
+static napi_value setup(napi_env env, napi_callback_info info) {
+  size_t argc = 1;
+  napi_value argv[1];
+  char kind[64];
+  NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, argv, NULL, NULL));
+  if (argc < 1) {
+    napi_throw_error(env, NULL, "setup(kind) needs a kind");
+    return NULL;
+  }
+  NODE_API_CALL(env, napi_get_value_string_utf8(env, argv[0], kind,
+                                                sizeof(kind), NULL));
+
+  napi_value pinned;
+  if (strcmp(kind, "add_finalizer") == 0) {
+    // No napi_ref asked for: registered straight on the JSC heap
+    // (Heap::addFinalizer).
+    NODE_API_CALL(env, napi_create_object(env, &pinned));
+    NODE_API_CALL(env, napi_add_finalizer(env, pinned, NULL, finalizer,
+                                          dup_kind(kind), NULL));
+  } else if (strcmp(kind, "add_finalizer_ref") == 0) {
+    // Registered through a weak NapiRef (the napi_ref variant).
+    napi_ref weak;
+    NODE_API_CALL(env, napi_create_object(env, &pinned));
+    NODE_API_CALL(env, napi_add_finalizer(env, pinned, NULL, finalizer,
+                                          dup_kind(kind), &weak));
+  } else if (strcmp(kind, "external") == 0) {
+    // Runs from the NapiExternal cell's destructor.
+    NODE_API_CALL(env, napi_create_external(env, NULL, finalizer,
+                                            dup_kind(kind), &pinned));
+  } else if (strcmp(kind, "empty_external_buffer") == 0) {
+    // A zero-length external buffer has no contents to hang the finalizer
+    // on, so it too is registered straight on the JSC heap.
+    NODE_API_CALL(env, napi_create_external_buffer(env, 0, NULL, finalizer,
+                                                   dup_kind(kind), &pinned));
+  } else {
+    napi_throw_error(env, NULL, "unknown kind");
+    return NULL;
+  }
+  pending_finalizers++;
+
+  // Never deleted: the object stays reachable until the VM is destroyed.
+  napi_ref pin;
+  NODE_API_CALL(env, napi_create_reference(env, pinned, 1, &pin));
+
+  napi_value undefined;
+  NODE_API_CALL(env, napi_get_undefined(env, &undefined));
+  return undefined;
+}
+
+static napi_value init(napi_env env, napi_value exports) {
+  NODE_API_CALL(env, napi_set_instance_data(env, &instance_data,
+                                            instance_data_finalizer, NULL));
+  napi_value setup_fn;
+  NODE_API_CALL(env, napi_create_function(env, "setup", NAPI_AUTO_LENGTH,
+                                          setup, NULL, &setup_fn));
+  NODE_API_CALL(env, napi_set_named_property(env, exports, "setup", setup_fn));
+  return exports;
+}
+
+NAPI_MODULE(test_vm_teardown_finalizers, init)

test/napi/napi-app/vm-teardown-finalizers.js

@@ -0,0 +1,27 @@
+// Usage: bun vm-teardown-finalizers.js <addon target> <kind>[,<kind>...] [--main-thread]
+//
+// Loads build/Debug/<addon target>.node (see test_vm_teardown_finalizers.c)
+// and registers one pinned finalizer per kind, in a worker_threads Worker
+// that then exits, or on the main thread with --main-thread (run under
+// BUN_DESTRUCT_VM_ON_EXIT=1 so that the main thread's VM is destroyed too).
+// Any other argument is ignored.
+const { Worker, isMainThread, workerData } = require("node:worker_threads");
+const path = require("node:path");
+
+function setup({ addon, kinds }) {
+  const { setup } = require(path.join(__dirname, "build/Debug", addon + ".node"));
+  for (const kind of kinds) setup(kind);
+  console.log("registered:", kinds.join(","));
+}
+
+if (!isMainThread) {
+  setup(workerData);
+} else {
+  const [addon, kindList] = process.argv.slice(2);
+  const args = { addon, kinds: kindList.split(",") };
+  if (process.argv.includes("--main-thread")) {
+    setup(args);
+  } else {
+    new Worker(__filename, { workerData: args }).on("exit", code => console.log("worker exited:", code));
+  }
+}

test/napi/napi.test.ts

@@ -1126,12 +1126,13 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => {
       await checkSameOutput("bigint_to_i64", testsString);
       await checkSameOutput("bigint_to_u64", testsString);
     });
+    // Three serial node + bun runs: over 5s on a debug ASAN build.
     it("returns the right error code", async () => {
       const badTypes = '[null, undefined, 5, "123", "abc"]';
       await checkSameOutput("bigint_to_i64", badTypes);
       await checkSameOutput("bigint_to_u64", badTypes);
       await checkSameOutput("bigint_to_64_null", []);
-    });
+    }, 10_000);
   });
 
   describe("create_bigint_words", () => {
@@ -1445,6 +1446,90 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => {
     },
     25_000,
   );
+
+  // Destroying a JSC VM (a Worker exiting; the main thread under
+  // BUN_DESTRUCT_VM_ON_EXIT) fires every finalizer still registered. That
+  // happens after env teardown and without the "sweeping" mutator state a
+  // collection has, which Bun used to mistake for an ordinary non-GC context:
+  // it ran the addon's callbacks inline, inside the heap being destroyed. The
+  // fixture pins its objects with strong refs, so VM destruction is the only
+  // thing that can reach these finalizers; the "still pending" count shows they
+  // were all still registered when env teardown finished.
+  describe("finalizers still registered when the VM is destroyed", () => {
+    const fixture = join(__dirname, "napi-app/vm-teardown-finalizers.js");
+    const kinds = ["add_finalizer", "add_finalizer_ref", "external", "empty_external_buffer"];
+
+    // The ASAN lanes export BUN_DESTRUCT_VM_ON_EXIT and LSan settings that the
+    // children would inherit. The variants below choose which VM is destroyed
+    // themselves, and the fixture's pins are never deleted (nothing of the
+    // addon's runs after the point they have to survive), so they keep its
+    // NapiEnv and the VM handle ref it holds alive, which LSan reports once the
+    // Worker is gone.
+    const { BUN_DESTRUCT_VM_ON_EXIT: _destruct, BUN_INSPECT_CONNECT_TO: _inspect, ...inheritedEnv } = bunEnv;
+    const fixtureEnv = (...asanOptions: string[]) => ({
+      ...inheritedEnv,
+      ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=0", ...asanOptions].filter(Boolean).join(":"),
+    });
+
+    const vms = [
+      { vm: "a worker_threads Worker", flags: [] as string[], extraEnv: {} as Record<string, string> },
+      { vm: "the main thread", flags: ["--main-thread"], extraEnv: { BUN_DESTRUCT_VM_ON_EXIT: "1" } },
+    ];
+    it.each(vms)(
+      "a regular module's finalizers are not invoked from the dying heap of $vm",
+      async ({ flags, extraEnv }) => {
+        await using proc = spawn({
+          cmd: [bunExe(), fixture, "test_vm_teardown_finalizers", kinds.join(","), ...flags],
+          env: { ...fixtureEnv(), ...extraEnv },
+          stdout: "pipe",
+          stderr: "pipe",
+        });
+        const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+        // Before the fix: one "FAIL: <kind> finalizer ran after env teardown" line
+        // per kind, then (debug builds) an assertion failure in JSCell::JSCell
+        // when the finalizer's handle scope allocates in the heap being destroyed.
+        expect(stderr).toBe("");
+        expect(stdout).toContain(`env teardown: ${kinds.length} finalizer(s) still pending`);
+        if (flags.length === 0) expect(stdout).toContain("worker exited: 0");
+        expect(exitCode).toBe(0);
+      },
+    );
+
+    // Experimental modules have their finalizers run synchronously by whatever
+    // frees the object, VM destruction included; what has to hold is that a
+    // GC-affecting call made from there is refused the same way it is during a
+    // collection. One kind per path JSC fires them from: the fixture's
+    // NapiExternal is destroyed as an individually allocated cell
+    // (PreciseAllocation::sweep), a napi_add_finalizer callback is a weak-handle
+    // finalizer (WeakBlock::lastChanceToFinalize). The first finalizer to run
+    // aborts the process, so each kind gets its own run.
+    it.each(["external", "add_finalizer"])(
+      "an experimental module's %s finalizer is told it is running from GC",
+      async kind => {
+        await using proc = spawn({
+          // The trailing flag makes a debug build's crash handler skip its slow
+          // symbolized backtrace; the fixture ignores it.
+          cmd: [
+            bunExe(),
+            fixture,
+            "test_vm_teardown_finalizers_experimental",
+            kind,
+            "--debug-crash-handler-use-trace-string",
+          ],
+          env: { ...fixtureEnv("disable_coredump=1", "symbolize=0"), BUN_INTERNAL_SUPPRESS_CRASH_ON_NAPI_ABORT: "1" },
+          stdout: "pipe",
+          stderr: "pipe",
+        });
+        const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+        expect(stdout).toContain("env teardown: 1 finalizer(s) still pending");
+        // Before the fix: "FAIL: <kind> finalizer called napi_get_undefined
+        // during VM destruction and got status 0" and a clean exit.
+        expect(stderr).not.toContain("FAIL:");
+        expect(stderr).toContain("FATAL ERROR: Finalizer is calling a function that may affect GC state.");
+        expect(exitCode).not.toBe(0);
+      },
+    );
+  });
 });
 
 // Kept outside describe.concurrent("napi") so RSS measurement isn't skewed by