[legendecas]

Fix the TSFN finalization re-entrance. state = kClosed marks a TSFN as ready to be
deleted, so a TSFN should not set this state before calling into user code synchronously,
which could call into napi_release_threadsafe_function, and delete the TSFN
synchronously in the middle of ThreadSafeFunction::MaybeDelete.

Fixes: #65100

[nodejs-github-bot]

Review requested:

• @nodejs/gyp
• @nodejs/node-api
• @nodejs/cpp-reviewers

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.69%. Comparing base (cfea992) to head (3265cc4).

Files with missing lines	Patch %	Lines
src/node_api.cc	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #65967   +/-   ##
=======================================
  Coverage   90.68%   90.69%           
=======================================
  Files         797      797           
  Lines      278779   278780    +1     
  Branches    53612    53614    +2     
=======================================
+ Hits       252809   252834   +25     
+ Misses      16338    16297   -41     
- Partials     9632     9649   +17     

Files with missing lines	Coverage Δ
src/node_api.cc	77.10% <66.66%> (+0.02%)	⬆️

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/77524/

[legendecas]

Can I have a second review from @nodejs/cpp-reviewers / @nodejs/node-api? Thank you!

[vmoroz]

nit: I passed this PR through Copilot (Claude Opus 5.5). It gave this suggestion to improve the test case (feel free to completely ignore):

Non-blocking suggestion: the 'exit' check confirms that the worker exited cleanly, but it can't tell whether the re-entrant release actually happened. The test catches the bug only through a crash, and the crash requires FinalizeHolder to run inside the TSFN's own env->Unref(). If that ever stops happening, nothing crashes and the test still passes. That could follow a change in the teardown order, or the holder no longer being kept alive until teardown. The abort() in FinalizeHolder doesn't help in that case, because it runs only when FinalizeHolder runs.

Could the test assert that the scenario ran? One option is for the addon to record what happened in static variables, and for the existing 'exit' handler to check them on the main thread:

• The worker thread is joined before 'exit' is emitted, so reading the statics there is safe.
• Worker teardown unloads the addons the worker loaded, so the main thread loads the addon first. That keeps the addon, and its statics, loaded.

I tried this locally on Windows. With Node v26.13.0, which has the bug, I made the worker drop the holder and call gc() so that FinalizeHolder runs before teardown. The current test still passed, and the check below failed with holderFinalizedAfterTsfn: false. With the holder kept, both versions pass on v24.11.1, which predates #55877.

Proposed change

--- a/test/node-api/test_threadsafe_function_shutdown/reentrant_release.c
+++ b/test/node-api/test_threadsafe_function_shutdown/reentrant_release.c
@@ -6,24 +6,34 @@ typedef struct {
   napi_threadsafe_function tsfn;
 } Holder;
 
+// Written on the worker thread while its napi_env tears down. Read on the main
+// thread from the worker 'exit' handler, which runs after the thread is joined.
+static bool tsfn_finalized = false;
+static int holder_finalize_count = 0;
+static bool holder_finalized_after_tsfn = false;
+static napi_status release_status = napi_generic_failure;
+
 static void CallJs(napi_env env, napi_value js_cb, void* context, void* data) {
   NODE_API_BASIC_ASSERT_RETURN_VOID(false, "The queue stays empty");
 }
 
+static void FinalizeTsfn(napi_env env, void* data, void* hint) {
+  tsfn_finalized = true;
+}
+
 // Runs while the napi_env tears down, reached from the thread-safe function
 // dropping its own napi_env reference. Releasing the thread-safe function
 // from here reenters TSFN finalization.
 static void FinalizeHolder(napi_env env, void* data, void* hint) {
   Holder* holder = data;
-  napi_status status =
+  holder_finalize_count++;
+  holder_finalized_after_tsfn = tsfn_finalized;
+  release_status =
       napi_release_threadsafe_function(holder->tsfn, napi_tsfn_abort);
-  if (status != napi_ok) {
-    abort();
-  }
   free(holder);
 }
 
-NAPI_MODULE_INIT() {
+static napi_value Setup(napi_env env, napi_callback_info info) {
   napi_value name;
   napi_value external;
   Holder* holder = malloc(sizeof(*holder));
@@ -42,18 +52,44 @@ NAPI_MODULE_INIT() {
                                                 0,
                                                 1,
                                                 NULL,
-                                                NULL,
+                                                FinalizeTsfn,
                                                 NULL,
                                                 CallJs,
                                                 &holder->tsfn));
   // Allow the worker uv_loop to exit.
   NODE_API_CALL(env, napi_unref_threadsafe_function(env, holder->tsfn));
 
-  // Held by the module exports, which the module cache keeps alive, so the
-  // napi_external finalizer runs during napi_env teardown.
   NODE_API_CALL(
       env, napi_create_external(env, holder, FinalizeHolder, NULL, &external));
-  NODE_API_CALL(env, napi_set_named_property(env, exports, "holder", external));
+  return external;
+}
+
+static napi_value GetResult(napi_env env, napi_callback_info info) {
+  napi_value result;
+  napi_value value;
+  NODE_API_CALL(env, napi_create_object(env, &result));
+  NODE_API_CALL(env, napi_create_int32(env, holder_finalize_count, &value));
+  NODE_API_CALL(
+      env, napi_set_named_property(env, result, "holderFinalizeCount", value));
+  NODE_API_CALL(env,
+                napi_get_boolean(env, holder_finalized_after_tsfn, &value));
+  NODE_API_CALL(
+      env,
+      napi_set_named_property(env, result, "holderFinalizedAfterTsfn", value));
+  NODE_API_CALL(env, napi_create_int32(env, release_status, &value));
+  NODE_API_CALL(env,
+                napi_set_named_property(env, result, "releaseStatus", value));
+  return result;
+}
 
+NAPI_MODULE_INIT() {
+  napi_property_descriptor properties[] = {
+      DECLARE_NODE_API_PROPERTY("setup", Setup),
+      DECLARE_NODE_API_PROPERTY("getResult", GetResult),
+  };
+  NODE_API_CALL(
+      env,
+      napi_define_properties(
+          env, exports, sizeof(properties) / sizeof(*properties), properties));
   return exports;
 }

--- a/test/node-api/test_threadsafe_function_shutdown/test-reentrant-release.js
+++ b/test/node-api/test_threadsafe_function_shutdown/test-reentrant-release.js
@@ -3,6 +3,9 @@
 const common = require('../../common');
 const assert = require('assert');
 const { Worker, isMainThread } = require('worker_threads');
+// Load the addon on the main thread too: it keeps the addon loaded, and its
+// static result state with it, after the worker environment unloads it.
+const binding = require(`./build/${common.buildType}/reentrant_release`);
 
 // A worker that exits while a native addon still owns a thread-safe function.
 // The TSFN finalizer could trigger napi_env finalization and the addon
@@ -14,7 +17,15 @@ if (isMainThread) {
   worker.on('error', common.mustNotCall());
   worker.on('exit', common.mustCall((code) => {
     assert.strictEqual(code, 0);
+    // The worker thread is joined before 'exit' is emitted, so its napi_env
+    // is already torn down. Check that the re-entrant release did happen.
+    assert.deepStrictEqual(binding.getResult(), {
+      holderFinalizeCount: 1,
+      holderFinalizedAfterTsfn: true,
+      releaseStatus: 0, // napi_ok
+    });
   }));
 } else {
-  require(`./build/${common.buildType}/reentrant_release`);
+  // Keep the external alive until the worker's napi_env is torn down.
+  globalThis.holder = binding.setup();
 }

[legendecas]

Thanks for the suggestion! This test is specifically about re-entrancy so I only added a re-entrant flag for testing and not bloating the test size.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/78750/