Make JSReference finalizer thread-safe to fix worker-teardown crash (exit 139)

[Saúl Ponce (GalaxiasKyklos)]

Summary

A Node process hosting the CLR via node-api-dotnet crashes with SIGSEGV / exit 139 (on Node < 24.14; a hang on ≥ 24.14) when a worker_threads Worker is torn down under load. The fatal fault is a managed exception escaping the GC finalizer thread, thrown from JSReference.Finalize().

This is distinct from, and the decisive cause on top of, the module-pinning fix in #487: #487 stops node-api-dotnet's own host module from being unloaded, but it does not make JSReference finalization safe when a worker's JS scope is already gone.

Issue: #493

Captured crash (on the shipping 0.9.23 binary)

Unhandled exception. Microsoft.JavaScript.NodeApi.JSInvalidThreadAccessException:
There is no active JS value scope. Current thread: #2030.
   at Microsoft.JavaScript.NodeApi.JSValueScope.get_Current()
   at Microsoft.JavaScript.NodeApi.JSReference.ThrowIfInvalidThreadAccess()
   at Microsoft.JavaScript.NodeApi.JSReference.Dispose(Boolean)
   at Microsoft.JavaScript.NodeApi.JSReference.Finalize()
   at System.Runtime.__Finalizer.DrainQueue()
   at System.Runtime.__Finalizer.ProcessFinalizers()

Root cause

~JSReference() calls Dispose(disposing: false) on the GC finalizer thread, which never has an active JS value scope. For a reference created from a no-context scope (the native host path, where _context == null), Dispose called ThrowIfInvalidThreadAccess():

protected virtual void Dispose(bool disposing)
{
    if (!IsDisposed)
    {
        IsDisposed = true;
        if (_context == null)
        {
            ThrowIfInvalidThreadAccess();   // reads JSValueScope.Current -> throws
            JSValueScope.CurrentRuntime.DeleteReference(_env, _handle).ThrowIfFailed();
        }
        else { /* post to sync context */ }
    }
}

ThrowIfInvalidThreadAccess() reads JSValueScope.Current, which is s_currentScope ?? throw new JSInvalidThreadAccessException(null). On the finalizer thread there is no scope, so it throws — and an exception escaping a finalizer terminates the process.

When a worker is torn down, any JSReferences that outlived their scope are finalized later on the finalizer thread, each hitting this throw. That is why the crash correlates with concurrency and CLR object/finalizer volume at teardown (frequent in the real per-request worker-recycle pattern, rare in a low-churn repro).

Fix

Split the dispose path so the finalizer never throws, while keeping explicit disposal behavior unchanged:

• Explicit Dispose() (disposing: true) is unchanged — it still asserts thread access for a no-context reference (documented behavior).
• Finalizer (disposing: false) releases the native reference only when it can be done safely, and never throws:
  • _context == null: delete the reference only if the matching JS scope happens to be current on this thread; otherwise skip the release (the JS environment is being torn down and the reference is released along with it).
  • _context != null: defer the delete to the JS thread via the synchronization context (already a safe no-op once the context is disposed).
  • All finalizer work is wrapped so no exception can escape.

Adds a non-throwing JSValueScope.CurrentOrNull accessor for the safe thread check.

protected virtual void Dispose(bool disposing)
{
    if (IsDisposed) return;
    IsDisposed = true;

    if (disposing)
    {
        // unchanged explicit-dispose behavior (asserts thread access when no context)
        ...
    }
    else
    {
        DisposeFromFinalizer();   // never throws
    }
}

Testing

Adds JSReferenceTests cases:

• FinalizeNoContextReferenceFromDifferentThreadDoesNotThrow — creates a no-context (JSValueScopeType.NoContext) reference and runs the finalizer path on a thread with no scope. Reproduces the exact crash stack above and fails without this fix; passes with it.
• FinalizeContextReferenceFromDifferentThreadDoesNotThrow — the context path also never throws off the JS thread.
• DisposeNoContextReferenceFromDifferentThreadThrows — explicit Dispose() still throws, confirming the fix is scoped to finalization only.

All JSReferenceTests and the rest of the managed unit suite (e.g. GCTests) pass on net10.0. Builds clean across all TFMs (net10.0 / net9.0 / net8.0 / netstandard2.0 / net472).

Notes for reviewers

• Skipping the native release in the finalizer's no-context branch intentionally leaks a napi_ref in the rare case the finalizer runs off the JS thread with no matching scope. This is safe: it only happens while the JS environment is being torn down, which releases the reference wholesale.
• On Node ≥ 24.14 the same teardown ordering surfaces as a hang rather than a segfault; this fix removes the throwing finalizer that underlies both failure modes.

[Copilot]

Pull request overview

Updates JSReference finalization to prevent managed exceptions during worker teardown.

Changes:

• Adds non-throwing finalizer cleanup paths.
• Adds finalization and explicit-disposal tests.
• Adds unrelated generated SDK configuration files.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
src/NodeApi/JSReference.cs	Implements guarded finalizer cleanup.
src/NodeApi/JSValueScope.cs	Adds nullable current-scope access.
test/JSReferenceTests.cs	Adds finalization regression tests.
test/TestCases/projects/ts-esm-module/global.json	Adds generated SDK pin.
test/TestCases/projects/ts-esm-dynamic/global.json	Adds generated SDK pin.
test/TestCases/projects/ts-cjs-module/global.json	Adds generated SDK pin.
test/TestCases/projects/ts-cjs-dynamic/global.json	Adds generated SDK pin.
test/TestCases/projects/js-esm-module/global.json	Adds generated SDK pin.
test/TestCases/projects/js-esm-dynamic/global.json	Adds generated SDK pin.
test/TestCases/projects/js-cjs-module/global.json	Adds generated SDK pin.
test/TestCases/projects/js-cjs-dynamic/global.json	Adds generated SDK pin.
test/TestCases/node-addon-api/global.json	Adds generated SDK pin.
test/TestCases/napi-dotnet/global.json	Adds generated SDK pin.
test/TestCases/napi-dotnet-init/global.json	Adds generated SDK pin.
test/TestCases/edgejs-perf/global.json	Adds generated SDK pin.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Saúl Ponce (GalaxiasKyklos)]

A no-context reference ( _context == null ) has no synchronization context or TSFN by definition, so  JSReference  has no mechanism to schedule the  DeleteReference  back onto the JS thread when the finalizer runs off-thread. The only options at this layer are delete-if-already-on-the-JS-thread, else skip, which is what this does. The previous behavior threw  JSInvalidThreadAccessException  out of the finalizer and terminated the process, so this is strictly safer. A proper JS-thread dispatch/cleanup queue for no-context references would require the native host to expose one; I've filed that as a follow-up rather than expanding this crash-fix PR.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (13)

src/NodeApi/JSReference.cs:393

• Finalization is not coupled to environment teardown. An undisposed no-context reference can be collected while its environment is still healthy, and an ordinary finalizer thread has no matching thread-static JS scope, so this path skips deletion and leaves the napi_ref (and a strong JS value) pinned for the rest of a long-lived environment. Route this cleanup to an environment-owned JS-thread queue/dispatcher rather than treating a missing scope as proof of teardown.

                // A no-context reference (for example one created from the native host scope) must
                // be deleted on the JS thread. Only delete it if this finalizer happens to run
                // while the matching JS scope is current on this thread; otherwise skip -- the JS
                // environment is being torn down and the reference is released along with it.
                JSValueScope? scope = JSValueScope.CurrentOrNull;

test/TestCases/projects/ts-esm-dynamic/global.json:3

• This is a generated test artifact, not part of the finalizer fix: TestBuilder.WriteCurrentFrameworkGlobalJson rewrites this file from the runtime executing the tests. Committing its net10 output adds an unrelated SDK pin and causes net8/net9 runs to rewrite the tracked checkout. Remove this file from the PR.

        "version": "10.0.100",

test/TestCases/projects/ts-cjs-module/global.json:3

• This is a generated test artifact, not part of the finalizer fix: TestBuilder.WriteCurrentFrameworkGlobalJson rewrites this file from the runtime executing the tests. Committing its net10 output adds an unrelated SDK pin and causes net8/net9 runs to rewrite the tracked checkout. Remove this file from the PR.

        "version": "10.0.100",

test/TestCases/projects/ts-cjs-dynamic/global.json:3

• This is a generated test artifact, not part of the finalizer fix: TestBuilder.WriteCurrentFrameworkGlobalJson rewrites this file from the runtime executing the tests. Committing its net10 output adds an unrelated SDK pin and causes net8/net9 runs to rewrite the tracked checkout. Remove this file from the PR.

        "version": "10.0.100",

test/TestCases/projects/js-esm-module/global.json:3

• This is a generated test artifact, not part of the finalizer fix: TestBuilder.WriteCurrentFrameworkGlobalJson rewrites this file from the runtime executing the tests. Committing its net10 output adds an unrelated SDK pin and causes net8/net9 runs to rewrite the tracked checkout. Remove this file from the PR.

        "version": "10.0.100",

test/TestCases/projects/js-esm-dynamic/global.json:3

• This is a generated test artifact, not part of the finalizer fix: TestBuilder.WriteCurrentFrameworkGlobalJson rewrites this file from the runtime executing the tests. Committing its net10 output adds an unrelated SDK pin and causes net8/net9 runs to rewrite the tracked checkout. Remove this file from the PR.

        "version": "10.0.100",

test/TestCases/projects/ts-esm-module/global.json:3

• This is a generated test artifact, not part of the finalizer fix: TestBuilder.WriteCurrentFrameworkGlobalJson rewrites this file from the runtime executing the tests. Committing its net10 output adds an unrelated SDK pin and causes net8/net9 runs to rewrite the tracked checkout. Remove this file from the PR.

        "version": "10.0.100",

test/TestCases/projects/js-cjs-module/global.json:3

• This is a generated test artifact, not part of the finalizer fix: TestBuilder.WriteCurrentFrameworkGlobalJson rewrites this file from the runtime executing the tests. Committing its net10 output adds an unrelated SDK pin and causes net8/net9 runs to rewrite the tracked checkout. Remove this file from the PR.

        "version": "10.0.100",

test/TestCases/projects/js-cjs-dynamic/global.json:3

• This is a generated test artifact, not part of the finalizer fix: TestBuilder.WriteCurrentFrameworkGlobalJson rewrites this file from the runtime executing the tests. Committing its net10 output adds an unrelated SDK pin and causes net8/net9 runs to rewrite the tracked checkout. Remove this file from the PR.

        "version": "10.0.100",

test/TestCases/node-addon-api/global.json:3

• This is a generated test artifact, not part of the finalizer fix: TestBuilder.WriteCurrentFrameworkGlobalJson rewrites this file from the runtime executing the tests. Committing its net10 output adds an unrelated SDK pin and causes net8/net9 runs to rewrite the tracked checkout. Remove this file from the PR.

        "version": "10.0.100",

test/TestCases/napi-dotnet/global.json:3

• This is a generated test artifact, not part of the finalizer fix: TestBuilder.WriteCurrentFrameworkGlobalJson rewrites this file from the runtime executing the tests. Committing its net10 output adds an unrelated SDK pin and causes net8/net9 runs to rewrite the tracked checkout. Remove this file from the PR.

        "version": "10.0.100",

test/TestCases/napi-dotnet-init/global.json:3

• This is a generated test artifact, not part of the finalizer fix: TestBuilder.WriteCurrentFrameworkGlobalJson rewrites this file from the runtime executing the tests. Committing its net10 output adds an unrelated SDK pin and causes net8/net9 runs to rewrite the tracked checkout. Remove this file from the PR.

        "version": "10.0.100",

test/TestCases/edgejs-perf/global.json:3

• This is a generated test artifact, not part of the finalizer fix: TestBuilder.WriteCurrentFrameworkGlobalJson rewrites this file from the runtime executing the tests. Committing its net10 output adds an unrelated SDK pin and causes net8/net9 runs to rewrite the tracked checkout. Remove this file from the PR.

        "version": "10.0.100",

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/NodeApi/JSReference.cs:397

• This leaks every no-context reference that reaches the finalizer while its environment is still active. A GC finalizer runs on the dedicated finalizer thread during ordinary collections too, so CurrentOrNull is normally null here regardless of whether worker teardown has started; consequently the napi_ref (and its JS value) can remain rooted for the full lifetime of a long-running environment. Please retain a JS-thread cleanup route for no-context references—for example, enqueue pending deletes per environment and drain them from subsequent no-context callback scopes—and abandon that queue only once environment cleanup is known to have begun.

                JSValueScope? scope = JSValueScope.CurrentOrNull;
                if (scope != null && scope.UncheckedEnvironmentHandle == _env)
                {
                    scope.Runtime.DeleteReference(_env, _handle);
                }

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/NodeApi/JSReference.cs:397

• CurrentOrNull is thread-static, so a real GC finalizer thread will never satisfy this condition. A no-context JSReference that becomes unreachable while its environment is still active therefore retains its napi_ref (and potentially a strongly referenced JS object) until the entire environment is destroyed; finalization is not limited to worker teardown. Please arrange deletion on the owning JS thread, or another environment-scoped cleanup mechanism, rather than assuming the environment is already shutting down.

                JSValueScope? scope = JSValueScope.CurrentOrNull;
                if (scope != null && scope.UncheckedEnvironmentHandle == _env)
                {
                    scope.Runtime.DeleteReference(_env, _handle);
                }

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.