node:vm: make breakOnSigint interrupt a script parked in Atomics.wait

src/jsc/bindings/NodeVM.cpp

@@ -8,6 +8,7 @@
 #include <JavaScriptCore/SourceProvider.h>
 
 #include "BunClientData.h"
+#include "JavaScriptCore/Watchdog.h"
 #include "NodeVM.h"
 #include "NodeVMScript.h"
 #include "NodeVMModule.h"
@@ -530,6 +531,35 @@ static void writeArrowHeaderStack(VM& vm, ErrorInstance* errorInstance, const St
     errorInstance->putDirect(vm, decoratedName, jsBoolean(true), JSC::PropertyAttribute::DontEnum | JSC::PropertyAttribute::ReadOnly);
 }
 
+bool consumeTermination(JSC::VM& vm, bool armedWatchdog)
+{
+    vm.clearHasTerminationRequest();
+    // A parked thread services no traps, so the SIGINT watcher's
+    // NeedTermination can still be pending here; same for NeedWatchdogCheck
+    // when this run armed a {timeout} watchdog that fired mid-park (left
+    // stale after the time-limit restore, it trips Watchdog::startTimer's
+    // ASSERT(hasTimeLimit())). An outer frame's pending watchdog check is not
+    // ours to clear: its limit is still armed and it must keep enforcing.
+    vm.traps().clearTrap(JSC::VMTraps::NeedTermination);
+    if (armedWatchdog)
+        vm.traps().clearTrap(JSC::VMTraps::NeedWatchdogCheck);
+    // worker.terminate() may have landed after the caller's scriptAllowed
+    // check, and the clears above must not eat its trap: fireTrap and
+    // clearTrap serialize on the trap-signaling lock, so a stop whose trap we
+    // cleared is visible to this re-check. Re-deliver it.
+    if (!Bun__VmHandle__scriptAllowed(WebCore::clientData(vm)->vmHandle)) [[unlikely]] {
+        vm.setHasTerminationRequest();
+        vm.notifyNeedTermination();
+        return false;
+    }
+    return true;
+}
+
+bool outerWatchdogArmed(JSC::VM& vm)
+{
+    return vm.watchdog() && vm.watchdog()->hasTimeLimit();
+}
+
 bool handleException(JSGlobalObject* globalObject, VM& vm, NakedPtr<JSC::Exception> exception, ThrowScope& throwScope)
 {
     if (auto* errorInstance = dynamicDowncast<ErrorInstance>(exception->value())) {

src/jsc/bindings/NodeVM.h

@@ -30,6 +30,18 @@ bool extractCachedData(JSValue cachedDataValue, WTF::Vector<uint8_t>& outCachedD
 String stringifyAnonymousFunction(JSGlobalObject* globalObject, const ArgList& args, ThrowScope& scope, int* outOffset);
 JSC::EncodedJSValue createCachedData(JSGlobalObject* globalObject, const JSC::SourceCode& source);
 bool handleException(JSGlobalObject* globalObject, VM& vm, NakedPtr<JSC::Exception> exception, ThrowScope& throwScope);
+// Clear a termination node:vm consumed (breakOnSigint/timeout). The request
+// flag alone is insufficient: a termination that interrupted Atomics.wait
+// never ran handleTraps, and the pending trap would re-terminate the VM.
+// armedWatchdog: whether this run armed a {timeout} watchdog; only then is a
+// pending NeedWatchdogCheck this run's to stand down.
+// Returns false when the VM began stopping (worker.terminate()) while we
+// consumed: the stop was re-delivered and the caller must propagate the
+// termination instead of reporting ERR_SCRIPT_EXECUTION_*.
+[[nodiscard]] bool consumeTermination(JSC::VM& vm, bool armedWatchdog);
+// An enclosing vm frame's {timeout} watchdog still has a time limit armed, so
+// a termination that carries no SIGINT flag is its timeout, not a raced SIGINT.
+bool outerWatchdogArmed(JSC::VM& vm);
 // `url` must be caller-resolved: `new Script` falls back to evalmachine.<anonymous>
 // when no filename was provided; compileFunction has no such default.
 void decorateParseErrorStack(JSGlobalObject* globalObject, VM& vm, JSObject* error, StringView sourceString, const String& url, const JSC::ParserError& parseError, OrdinalNumber lineOffset);

src/jsc/bindings/NodeVMModule.cpp

@@ -70,6 +70,40 @@ void NodeVMModule::reconcileEvaluationState(JSC::VM& vm)
     }
 }
 
+bool NodeVMModule::reportOwnedTermination(JSC::JSGlobalObject* globalObject, JSC::ThrowScope& scope, NodeVMGlobalObject* nodeVmGlobalObject, uint32_t timeout, bool breakOnSigint)
+{
+    VM& vm = JSC::getVM(globalObject);
+    // Ours only if the VM is not being stopped and this evaluation can
+    // attribute the termination to its own arming: a flagged SIGINT, its own
+    // {timeout}, or breakOnSigint with no enclosing armed watchdog (an armed
+    // outer watchdog means this is the outer frame's timeout firing).
+    bool owned = Bun__VmHandle__scriptAllowed(WebCore::clientData(vm)->vmHandle)
+        && (getSigintReceived() || timeout != 0 || (breakOnSigint && !NodeVM::outerWatchdogArmed(vm)));
+    if (!owned) {
+        if (!vm.hasPendingTerminationException())
+            vm.throwTerminationException();
+        return false;
+    }
+    vm.drainMicrotasksForGlobalObject(nodeVmGlobalObject);
+    if (vm.hasPendingTerminationException())
+        DECLARE_TOP_EXCEPTION_SCOPE(vm).clearException();
+    if (!NodeVM::consumeTermination(vm, timeout != 0)) {
+        vm.throwTerminationException();
+        return false;
+    }
+    if (getSigintReceived()) {
+        setSigintReceived(false);
+        throwError(globalObject, scope, ErrorCode::ERR_SCRIPT_EXECUTION_INTERRUPTED, "Script execution was interrupted by `SIGINT`"_s);
+    } else if (timeout != 0) {
+        throwError(globalObject, scope, ErrorCode::ERR_SCRIPT_EXECUTION_TIMEOUT, makeString("Script execution timed out after "_s, timeout, "ms"_s));
+    } else {
+        // breakOnSigint was armed, but SIGINT raced the watcher registration
+        // or teardown past the sigintReceived flag. It is still an interrupt.
+        throwError(globalObject, scope, ErrorCode::ERR_SCRIPT_EXECUTION_INTERRUPTED, "Script execution was interrupted by `SIGINT`"_s);
+    }
+    return true;
+}
+
 JSValue NodeVMModule::evaluate(JSGlobalObject* globalObject, uint32_t timeout, bool breakOnSigint)
 {
     VM& vm = globalObject->vm();
@@ -100,22 +134,8 @@ JSValue NodeVMModule::evaluate(JSGlobalObject* globalObject, uint32_t timeout, b
             // exception-check validator is satisfied before the TOP scope
             // below, then convert it to ERR_SCRIPT_EXECUTION_*.
             std::ignore = scope.exception();
-            if ((vm.hasTerminationRequest() || vm.hasPendingTerminationException()) && !Bun__VmHandle__scriptAllowed(WebCore::clientData(vm)->vmHandle)) {
-                // The VM itself is being stopped; not ours to consume. Propagate the termination.
-                if (!vm.hasPendingTerminationException())
-                    vm.throwTerminationException();
-                return {};
-            }
             if (vm.hasTerminationRequest() || vm.hasPendingTerminationException()) {
-                vm.drainMicrotasksForGlobalObject(nodeVmGlobalObject);
-                DECLARE_TOP_EXCEPTION_SCOPE(vm).clearException();
-                vm.clearHasTerminationRequest();
-                if (getSigintReceived()) {
-                    setSigintReceived(false);
-                    throwError(globalObject, scope, ErrorCode::ERR_SCRIPT_EXECUTION_INTERRUPTED, "Script execution was interrupted by `SIGINT`"_s);
-                } else {
-                    throwError(globalObject, scope, ErrorCode::ERR_SCRIPT_EXECUTION_TIMEOUT, makeString("Script execution timed out after "_s, timeout, "ms"_s));
-                }
+                reportOwnedTermination(globalObject, scope, nodeVmGlobalObject, timeout, breakOnSigint);
                 return {};
             }
         }
@@ -246,24 +266,12 @@ JSValue NodeVMModule::evaluate(JSGlobalObject* globalObject, uint32_t timeout, b
     // termination one is converted to ERR_SCRIPT_EXECUTION_* here. Observe it
     // so the exception-check validator is satisfied before the TOP scope.
     std::ignore = scope.exception();
-    if ((vm.hasTerminationRequest() || vm.hasPendingTerminationException()) && !Bun__VmHandle__scriptAllowed(WebCore::clientData(vm)->vmHandle)) {
-        // The VM itself is being stopped; not ours to consume. Propagate the termination.
-        if (!vm.hasPendingTerminationException())
-            vm.throwTerminationException();
-        return {};
-    }
     if (vm.hasTerminationRequest() || vm.hasPendingTerminationException()) {
-        vm.drainMicrotasksForGlobalObject(nodeVmGlobalObject);
-        DECLARE_TOP_EXCEPTION_SCOPE(vm).clearException();
-        vm.clearHasTerminationRequest();
-        if (getSigintReceived()) {
-            setSigintReceived(false);
-            throwError(globalObject, scope, ErrorCode::ERR_SCRIPT_EXECUTION_INTERRUPTED, "Script execution was interrupted by `SIGINT`"_s);
-        } else if (timeout != 0) {
-            throwError(globalObject, scope, ErrorCode::ERR_SCRIPT_EXECUTION_TIMEOUT, makeString("Script execution timed out after "_s, timeout, "ms"_s));
-        } else {
-            RELEASE_ASSERT_NOT_REACHED_WITH_MESSAGE("vm.SourceTextModule evaluation terminated due neither to SIGINT nor to timeout");
-        }
+        // A propagated termination must not mark the module Errored (the
+        // VM_RETURN_IF_EXCEPTION below would pin it as the module's
+        // evaluation exception forever); only owned ERR_* errors fall through.
+        if (!reportOwnedTermination(globalObject, scope, nodeVmGlobalObject, timeout, breakOnSigint))
+            return {};
     } else {
         setSigintReceived(false);
     }

src/jsc/bindings/NodeVMModule.h

@@ -61,6 +61,12 @@ class NodeVMModule : public JSC::JSDestructibleObject, public SigintReceiver {
 
     JSValue evaluate(JSGlobalObject* globalObject, uint32_t timeout, bool breakOnSigint);
 
+    // Consume a termination this evaluation owns and throw the matching
+    // ERR_SCRIPT_EXECUTION_* (returns true), or propagate one that is not
+    // ours: VM teardown, an outer vm frame's interrupt, or a worker stop
+    // that raced the consume (returns false, termination left pending).
+    bool reportOwnedTermination(JSC::JSGlobalObject* globalObject, JSC::ThrowScope& scope, NodeVMGlobalObject* nodeVmGlobalObject, uint32_t timeout, bool breakOnSigint);
+
     // For top-level-await modules JSC finishes evaluation asynchronously
     // (record status EvaluatingAsync -> Evaluated); pull the final state into
     // m_status/m_evaluationException once the record has settled.

src/jsc/bindings/NodeVMScript.cpp

@@ -307,28 +307,43 @@
     static_cast<NodeVMScript*>(cell)->NodeVMScript::~NodeVMScript();
 }
 
-static bool checkForTermination(JSC::VM& vm, JSC::JSGlobalObject* globalObject, JSC::ThrowScope& scope, NodeVMScript* script, std::optional<double> timeout)
+static bool checkForTermination(JSC::VM& vm, JSC::JSGlobalObject* globalObject, JSC::ThrowScope& scope, NodeVMScript* script, std::optional<double> timeout, bool breakOnSigint)
 {
     if (vm.hasTerminationRequest()) {
         // The whole VM is being stopped (worker terminate()/exit): that
         // termination is not ours to consume. The caller rethrows what
         // evaluate() caught like any other exception.
         if (!Bun__VmHandle__scriptAllowed(WebCore::clientData(vm)->vmHandle))
             return false;
+        // Not attributable to this run: no SIGINT flag, no own timeout, and
+        // breakOnSigint alone cannot claim it while an enclosing frame's
+        // watchdog is armed (that is the outer timeout firing during this
+        // run). The owning frame's checkForTermination reports it; leave the
+        // request pending.
+        if (!script->getSigintReceived() && !timeout && (!breakOnSigint || outerWatchdogArmed(vm)))
+            return false;
         vm.drainMicrotasksForGlobalObject(globalObject);
         // The termination may have fired inside an afterEvaluate microtask
         // checkpoint, leaving the termination exception pending; clear it so
         // the ERR_SCRIPT_EXECUTION_* error below replaces it.
         if (vm.hasPendingTerminationException())
             DECLARE_TOP_EXCEPTION_SCOPE(vm).clearException();
-        vm.clearHasTerminationRequest();
+        if (!consumeTermination(vm, timeout.has_value())) {
+            // Deliver the re-armed worker stop right away; the caller's
+            // RETURN_IF_EXCEPTION propagates it.
+            vm.throwTerminationException();
+            return false;
+        }
         if (script->getSigintReceived()) {
             script->setSigintReceived(false);
             throwError(globalObject, scope, ErrorCode::ERR_SCRIPT_EXECUTION_INTERRUPTED, "Script execution was interrupted by `SIGINT`"_s);
         } else if (timeout) {
             throwError(globalObject, scope, ErrorCode::ERR_SCRIPT_EXECUTION_TIMEOUT, makeString("Script execution timed out after "_s, *timeout, "ms"_s));
         } else {
-            RELEASE_ASSERT_NOT_REACHED_WITH_MESSAGE("vm.Script terminated due neither to SIGINT nor to timeout");
+            // breakOnSigint was armed, but SIGINT raced the watcher
+            // registration or teardown, so the paired sigintReceived flag
+            // was not set. It is still an interrupt.
+            throwError(globalObject, scope, ErrorCode::ERR_SCRIPT_EXECUTION_INTERRUPTED, "Script execution was interrupted by `SIGINT`"_s);
         }
         return true;
     }
@@ -414,7 +429,7 @@
         vm.watchdog()->setTimeLimit(WTF::Seconds::fromMilliseconds(*oldLimit));
     }
 
-    if (checkForTermination(vm, globalObject, scope, script, newLimit)) {
+    if (checkForTermination(vm, globalObject, scope, script, newLimit, options.breakOnSigint)) {
         return {};
     }
 
@@ -481,7 +496,7 @@
         vm.watchdog()->setTimeLimit(WTF::Seconds::fromMilliseconds(*oldLimit));
     }
 
-    if (checkForTermination(vm, globalObject, scope, script, newLimit)) {
+    if (checkForTermination(vm, globalObject, scope, script, newLimit, options.breakOnSigint)) {
         return {};
     }
 

src/jsc/bindings/vm/SigintWatcher.cpp

@@ -1,6 +1,8 @@
 #include "NodeVM.h"
 #include "SigintWatcher.h"
 
+#include <JavaScriptCore/WaiterListManager.h>
+
 #if OS(WINDOWS)
 #include <windows.h>
 #endif
@@ -207,7 +209,18 @@ bool SigintWatcher::signalAll()
     }
 
     for (JSGlobalObject* globalObject : m_globalObjects) {
-        globalObject->vm().notifyNeedTermination();
+        JSC::VM& vm = globalObject->vm();
+        // Atomics.wait's park loop (WaiterListManager::waitForSync) only exits
+        // on hasTerminationRequest(); the NeedTermination trap wakes the waiter
+        // but is serviced only at safepoints, which a parked thread never reaches.
+        vm.setHasTerminationRequest();
+        vm.notifyNeedTermination();
+        // fireTrap only notifies the sync waiter on the first thread-stop
+        // request; if another async trap (a {timeout} watchdog fire) already
+        // requested one, the parked waiter would not be woken. POSIX hides
+        // this behind the trap SignalSender's retry loop, but that is
+        // compiled out on Windows and under usePollingTraps.
+        vm.syncWaiter()->condition().notifyOne();
     }
 
     return true;