Fix error reporting during worker termination

src/NodeApi/JSError.cs

@@ -7,6 +7,7 @@
 using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
 using System.Text;
+using Microsoft.JavaScript.NodeApi.Runtime;
 using static Microsoft.JavaScript.NodeApi.Runtime.JSRuntime;
 
 namespace Microsoft.JavaScript.NodeApi;
@@ -142,7 +143,8 @@ public JSError(Exception exception)
             }
         }
 
-        _errorRef = CreateErrorReference(CreateErrorValueForException(exception, out _message));
+        _message = GetExceptionMessage(exception);
+        _errorRef = CreateErrorReference(CreateErrorValueForException(exception, _message));
     }
 
     public string Message
@@ -190,11 +192,12 @@ public readonly JSValue Value
         }
     }
 
-    private static JSValue CreateErrorValueForException(Exception exception, out string message)
-    {
-        message = (exception as TargetInvocationException)?.InnerException?.Message
+    private static string GetExceptionMessage(Exception exception)
+        => (exception as TargetInvocationException)?.InnerException?.Message
             ?? exception.Message;
 
+    private static JSValue CreateErrorValueForException(Exception exception, string message)
+    {
         // If the exception is a JSException for an error value, use that error value;
         // otherwise construct a new error value from the exception message.
         JSValue error = (exception as JSException)?.Error?.Value ??
@@ -270,19 +273,60 @@ public readonly void ThrowError()
     /// </remarks>
     public static void ThrowError(Exception exception)
     {
-        // Do not construct a JSError object here, because that would require a runtime context.
-        JSValue error = CreateErrorValueForException(exception, out string message);
+        JSRuntime runtime = JSValue.GetCurrentRuntime(out napi_env env);
+
+        // If an exception is already pending, it will be propagated by Node when this callback
+        // returns. This also covers a terminating environment, where attempting any additional
+        // N-API work while constructing an error can throw out of the native callback boundary.
+        if (HasPendingExceptionOrIsClosing(runtime, env))
+        {
+            return;
+        }
+
+        string message = GetExceptionMessage(exception);
+        JSValue error;
+        try
+        {
+            // Do not construct a JSError object here, because that would require a runtime context.
+            error = CreateErrorValueForException(exception, message);
+        }
+        catch (JSException)
+        {
+            // Termination may begin after the check above, causing any of the N-API operations used
+            // to construct or augment the JS Error object to fail. Fall back to the direct runtime
+            // error API, which does not require creating or inspecting any JS values.
+            napi_status fallbackStatus = runtime.ThrowError(env, code: null, message);
+            if (fallbackStatus == napi_status.napi_ok ||
+                fallbackStatus == napi_status.napi_pending_exception ||
+                fallbackStatus == napi_status.napi_closing)
+            {
+                return;
+            }
 
-        napi_status status = error.Scope.Runtime.Throw(
-            (napi_env)JSValueScope.Current, (napi_value)error);
+            throw;
+        }
 
-        if (status != napi_status.napi_ok && status != napi_status.napi_pending_exception)
+        napi_status status = runtime.Throw(env, (napi_value)error);
+
+        if (status != napi_status.napi_ok &&
+            status != napi_status.napi_pending_exception &&
+            status != napi_status.napi_closing)
         {
             throw new JSException(
                 $"Failed to throw JS Error. Status: {status}\n{message}");
         }
     }
 
+    private static bool HasPendingExceptionOrIsClosing(JSRuntime runtime, napi_env env)
+    {
+        // Use the raw runtime API instead of IsExceptionPending(), because ThrowIfFailed() would
+        // attempt to construct another JSError while this error-reporting path is already failing.
+        napi_status status = runtime.IsExceptionPending(env, out bool isPending);
+        return (status == napi_status.napi_ok && isPending) ||
+            status == napi_status.napi_pending_exception ||
+            status == napi_status.napi_closing;
+    }
+
     public static void ThrowError(string message)
         => JSValue.GetCurrentRuntime(out napi_env env).ThrowError(env, code: null, message);
 

test/JSErrorTests.cs

@@ -0,0 +1,53 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+using System;
+using Microsoft.JavaScript.NodeApi.Runtime;
+using Xunit;
+using static Microsoft.JavaScript.NodeApi.Runtime.JSRuntime;
+
+namespace Microsoft.JavaScript.NodeApi.Test;
+
+public class JSErrorTests
+{
+    [Theory]
+    [InlineData(napi_status.napi_ok, true)]
+    [InlineData(napi_status.napi_pending_exception, false)]
+    [InlineData(napi_status.napi_closing, false)]
+    public void ThrowErrorReturnsWhenEnvironmentCannotAcceptAnException(
+        napi_status status, bool isPending)
+    {
+        var runtime = new TerminatingJSRuntime(status, isPending);
+        napi_env env = new(Environment.CurrentManagedThreadId);
+        using var scope = new JSValueScope(
+            JSValueScopeType.NoContext,
+            env,
+            runtime,
+            new MockJSRuntime.SynchronizationContext());
+
+        JSError.ThrowError(new InvalidOperationException("test"));
+
+        Assert.Equal(1, runtime.IsExceptionPendingCallCount);
+    }
+
+    private sealed class TerminatingJSRuntime : MockJSRuntime
+    {
+        private readonly napi_status _status;
+        private readonly bool _isPending;
+
+        public TerminatingJSRuntime(napi_status status, bool isPending)
+        {
+            _status = status;
+            _isPending = isPending;
+        }
+
+        public int IsExceptionPendingCallCount { get; private set; }
+
+        public override napi_status IsExceptionPending(napi_env env, out bool result)
+        {
+            IsExceptionPendingCallCount++;
+            result = _isPending;
+            return _status;
+        }
+    }
+}

test/NativeAotTests.cs

@@ -24,6 +24,11 @@ public class NativeAotTests
         // NativeAOT module has its own entry point that never calls it, so this case is only
         // meaningful under the hosted CLR host. See HostedClrTests.
         !testCaseName.Contains("/worker_teardown") &&
+
+        // This case reproduces termination of the hosted CoreCLR worker used by the
+        // node-api-dotnet package. Terminating a generated NativeAOT module while its unmanaged
+        // callback is still executing has separate fail-fast behavior outside this regression.
+        !testCaseName.Contains("/worker_termination_during_call") &&
         !testCaseName.StartsWith("projects/", StringComparison.Ordinal));
 
     [Theory]

test/TestCases/napi-dotnet/Errors.cs

@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 
 using System;
+using System.Threading;
 using System.Threading.Tasks;
 
 namespace Microsoft.JavaScript.NodeApi.TestCases;
@@ -24,10 +25,24 @@ public static async Task ThrowAsyncDotnetError(string message)
         await Task.Yield();
         throw new Exception(message);
     }
+
+    public static void WaitAndThrow(
+        int milliseconds, IWorkerTerminationCallbacks workerTerminationCallbacks)
+    {
+        workerTerminationCallbacks.Started();
+        Thread.Sleep(milliseconds);
+        throw new Exception("Worker termination test");
+    }
 }
 
 [JSExport]
 public interface IJSErrors
 {
     void ThrowJSError(string message);
 }
+
+[JSExport]
+public interface IWorkerTerminationCallbacks
+{
+    void Started();
+}

test/TestCases/napi-dotnet/worker_termination_during_call.js

@@ -0,0 +1,31 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+// Regression test for terminating a Worker while it is executing a .NET call.
+//
+// The managed method calls back into JS to signal that it has started, then blocks before
+// throwing. The parent terminates the Worker while the managed call is blocked. When the
+// exception reaches JSError.ThrowError(Exception), the environment is already terminating.
+// Attempting to construct another JS error in that state used to let a JSException escape the
+// native callback boundary, aborting the entire process on Linux with exit code 134.
+
+const { Worker, isMainThread, parentPort } = require('worker_threads');
+
+if (isMainThread) {
+  const worker = new Worker(__filename);
+  worker.on('error', (error) => { throw error; });
+  worker.once('message', async (message) => {
+    if (message !== 'started') {
+      throw new Error(`Unexpected worker message: ${message}`);
+    }
+
+    await worker.terminate();
+  });
+} else {
+  const binding = require('../common').binding;
+  binding.Errors.waitAndThrow(250, {
+    started() {
+      parentPort.postMessage('started');
+    },
+  });
+}