Skip to content

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

Open
robobun wants to merge 9 commits into
mainfrom
farm/b6ff2cf8/sigint-atomics-wait
Open

node:vm: make breakOnSigint interrupt a script parked in Atomics.wait#37268
robobun wants to merge 9 commits into
mainfrom
farm/b6ff2cf8/sigint-atomics-wait

Conversation

@robobun

@robobun robobun commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Problem

A breakOnSigint vm script parked in Atomics.wait ignores SIGINT forever:

const vm = require("node:vm");
const { Worker } = require("worker_threads");
const sab = new SharedArrayBuffer(16);
globalThis.ia = new Int32Array(sab);
new Worker(
  `const { workerData } = require("worker_threads");
   const ia = new Int32Array(workerData);
   while (Atomics.notify(ia, 0, 1) === 0) {}   // proves the main thread is parked
   Atomics.wait(ia, 1, 0, 100);                 // let it re-park
   process.kill(process.pid, "SIGINT");`,
  { eval: true, workerData: sab },
).unref();
try {
  vm.runInThisContext("for(;;) Atomics.wait(ia, 0, 0);", { breakOnSigint: true });
} catch (e) { console.log("caught", e.code); }

Node (v26.3.0) prints caught ERR_SCRIPT_EXECUTION_INTERRUPTED and exits 0. Bun hangs forever.

Cause

Several layers of the off-thread termination contract were missing in how node:vm delivers and consumes a SIGINT interrupt.

  1. The park loop never observes the interrupt. SigintWatcher::signalAll() only called vm.notifyNeedTermination(), which fires the NeedTermination VM trap. The trap machinery wakes the VM's sync Atomics waiter, but the park loop in WaiterListManager::waitForSync only exits when vm.hasTerminationRequest() is set:

    while (syncWaiter->isOnList() && time.now() < time && !vm.hasTerminationRequest())
        syncWaiter->condition().waitUntil(list->lock, ...);

    That flag is normally set by VMTraps::handleTraps at a JS safepoint, and a thread parked in a futex never reaches one, so the woken waiter re-parks forever. The watcher now sets the request itself before firing the trap.

  2. The trap bit outlives the run. Once the request is set off-thread, the woken waiter throws the termination exception directly (AtomicsObject.cpp), bypassing handleTraps, so the NeedTermination trap bit is never consumed. It re-fires at the next trap check, which lands inside checkForTermination's own throwError: createError is terminated mid-construction and returns an empty JSValue, and ThrowScope::throwException calls isObject() on it (empty reports isCell() == true with asCell() == nullptr). On debug builds this is a deterministic UBSan abort before the script's catch block ever runs:

    JSCJSValueCell.h:77:34: runtime error: member call on null pointer of type 'JSC::JSCell'
      #1 JSC::ThrowScope::throwException(JSC::JSGlobalObject*, JSC::JSValue) ThrowScope.cpp:84
      #2 Bun::throwError(...) ErrorCode.cpp:1811
      #3 Bun::checkForTermination(...) NodeVMScript.cpp:327
    

    The points that consume a SIGINT/timeout termination and revive the VM now stand the trap machinery down (NodeVM::consumeTermination).

  3. The wake is skipped when {timeout} is also armed. fireTrap only notifies the sync waiter on the first thread-stop request. With { breakOnSigint: true, timeout } the watchdog's NeedWatchdogCheck fires first (and is never serviced while parked), so the later SIGINT's trap does not notify at all. POSIX default config recovers through the trap SignalSender's retry loop, but that component does not exist on Windows or under usePollingTraps (the composed case hung there), and the stale NeedWatchdogCheck reaching Watchdog::startTimer after the time limit was restored aborted debug builds with ASSERTION FAILED: hasTimeLimit(). The watcher now notifies vm.syncWaiter() directly, and consumeTermination clears NeedWatchdogCheck alongside NeedTermination (an outer timed run is unaffected: restoring its limit re-arms the timer).

  4. A raced SIGINT could trip a RELEASE_ASSERT. The watcher reads the receiver and global-object lists in two critical sections, and the holder registers/unregisters them in two as well, so a SIGINT landing mid-registration or mid-teardown can set the request without the paired sigintReceived flag. checkForTermination treated that state as unreachable. With breakOnSigint armed it is now reported as ERR_SCRIPT_EXECUTION_INTERRUPTED like any other SIGINT.

  5. Nested vm runs consumed (or crashed on) the outer frame's interrupt. Nested runs share the VM, so an inner run observes the request set for an outer breakOnSigint/timeout frame. An optionless inner run hit the same RELEASE_ASSERT_NOT_REACHED, which needs no SIGINT and crashes released bun on main today:

    globalThis.vm = require("node:vm");
    vm.runInThisContext("vm.runInThisContext('for(;;);');", { timeout: 100 });
    // main (1.4.0 canary): panic(main thread): abort() called
    // node: throws ERR_SCRIPT_EXECUTION_TIMEOUT

    And an inner run armed only with breakOnSigint would have claimed an outer {timeout}'s termination as a SIGINT: the mislabeled error is catchable, and since only the run that armed the watchdog re-arms it, a guest swallowing it would run past the outer timeout forever. Attribution is now explicit: a termination is this run's to consume only for a flagged SIGINT, its own {timeout}, or breakOnSigint with no enclosing watchdog still armed (Watchdog::hasTimeLimit). Anything else propagates (uncatchable by guest JS) to the frame that armed it, which reports ERR_SCRIPT_EXECUTION_TIMEOUT/_INTERRUPTED as node does, and consumeTermination only stands down NeedWatchdogCheck for the run that armed the watchdog.

  6. Consuming could race worker.terminate(). The new trap clearing runs after the scriptAllowed check, so a terminate landing in between would have its NeedTermination trap silently discarded (before this PR the trap survived and re-fired at the next safepoint, so terminate self-healed). consumeTermination re-checks scriptAllowed after clearing: fireTrap and clearTrap serialize on the trap-signaling lock, so a stop whose trap was cleared is provably visible to the re-check, and it is re-delivered while the caller propagates the termination instead of reporting ERR_SCRIPT_EXECUTION_*.

Fix

  • SigintWatcher::signalAll(): set hasTerminationRequest before notifyNeedTermination(), then notify vm.syncWaiter() unconditionally.
  • NodeVM::consumeTermination() (new, used by checkForTermination and both NodeVMModule::evaluate blocks): clear the request plus the NeedTermination and NeedWatchdogCheck traps, re-delivering a raced worker stop.
  • The consume sites propagate a termination this run did not arm (nested frames, raced terminate) and fall back to ERR_SCRIPT_EXECUTION_INTERRUPTED when breakOnSigint was armed and the request arrived without the sigintReceived flag. The RELEASE_ASSERT_NOT_REACHED there is gone.

This is the SIGINT sibling of the worker.terminate() fix in #32802 (a terminated worker's VM never runs again, so no trap cleanup is needed there). The timeout option's own inability to interrupt Atomics.wait is a separate watchdog redesign, tracked in #33764; this change only makes SIGINT able to rescue a composed run and composes with both PRs.

One known, pre-existing attribution gap remains out of scope: an inner {timeout} run nested in an outer {breakOnSigint} frame claims the outer's SIGINT as its own timeout (mislabeled, catchable; the next Ctrl+C works since the outer's watcher hold stays registered). Attributing that correctly needs "did this run's watchdog deadline actually pass", which JSC::Watchdog does not expose; the per-evaluation deadline in #33764 can answer it, so it belongs in that follow-up. The available "enclosing watcher hold active" signal cannot be used instead: it would mislabel every legitimate inner timeout under an outer breakOnSigint as an interrupt.

A residual nanosecond window remains on configs without the SignalSender (Windows, usePollingTraps): a notify that lands between the waiter's predicate check and its park is lost, and the next Ctrl+C delivers the interrupt. Closing it fully would need a watcher-side retry loop; before this PR every Ctrl+C was lost on every platform.

Verification

  • The repro above matches node v26.3.0 output exactly; the process keeps executing JS afterwards (trap checks would rethrow a lingering termination) and exits 0. The nested-timeout repro also now matches node (ERR_SCRIPT_EXECUTION_TIMEOUT instead of a panic).
  • Eight tests in test/js/node/vm/vm.test.ts: Script path, SourceTextModule.evaluate, the worker-hosted guest (also pins that a main-thread SIGINT listener stays silent during the hold and fires again after), a nested plain run inside a breakOnSigint frame (the guest's try/catch must not observe the termination), the composed {breakOnSigint, timeout} mode under default and polling traps, the nested-timeout crash (cross-platform, no SIGINT needed), and the outer-timeout-during-nested-breakOnSigint bypass. All fail on bun without the fix (hang or abort).
  • Windows x64, manually with a real console CTRL_C_EVENT (GenerateConsoleCtrlEvent from a worker, isolated console): canary bun hangs; this build prints caught ERR_SCRIPT_EXECUTION_INTERRUPTED / alive true and exits 0 in both plain and composed modes. The automated tests skip Windows because process.kill cannot deliver a console Ctrl+C there.
  • Without the trap-bit clearing, the UBSan null-member-call fires 3/3 on the debug ASAN build; with it, 0/3. Without the direct waiter notify, composed mode aborts (default traps) or hangs (polling traps) deterministically; with it, both pass.
  • test/js/node/vm/vm.test.ts (219 pass), sourcetextmodule-*.test.ts, node's test-vm-sigint.js, test-vm-sigint-existing-handler.js, test-vm-break-on-sigint.js, and test/js/node/worker_threads/worker_threads.test.ts (121 pass) are green on the debug build.

@robobun

robobun commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

Status: complete and ready for review.

Reproduced deterministically: a breakOnSigint guest parked in Atomics.wait ignores SIGINT forever on main (node throws ERR_SCRIPT_EXECUTION_INTERRUPTED); the nested-timeout variant panics released bun with no SIGINT involved. The fix and the review findings it absorbed are documented in the PR description (six layers: park-loop wake, trap standdown, direct waiter notify for composed/Windows/polling-traps, raced-SIGINT tear, nested-frame attribution, raced worker.terminate re-delivery).

Verification: eight tests in test/js/node/vm/vm.test.ts, each failing on unfixed bun (hang or abort) and passing with the fix; output matches node v26.3.0; Windows x64 verified manually with a real console CTRL_C_EVENT in plain and composed modes. CI build 91024 (99d8eff): all vm, worker_threads, and sigint suites green on every lane; the remaining red lanes are a pre-existing darwin test/cli/test/parallel.test.ts failure (also failing on main, reported separately) and parallel-batch flakes that passed alone, none touching this diff.

@github-actions github-actions Bot added the claude label Aug 9, 2026
@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

NodeVM termination handling now tracks request ownership across nested script and module evaluations. SIGINT wakes synchronous waiters, while active evaluations consume their own requests and propagate outer requests. Regression tests cover interruption, timeout propagation, and recovery.

Changes

NodeVM termination handling

Layer / File(s) Summary
Deliver termination to blocked VMs
src/jsc/bindings/vm/SigintWatcher.cpp
signalAll() records the termination request before notifying the VM and explicitly wakes one synchronous waiter.
Consume or propagate termination
src/jsc/bindings/NodeVM.h, src/jsc/bindings/NodeVM.cpp, src/jsc/bindings/NodeVMScript.cpp, src/jsc/bindings/NodeVMModule.cpp
consumeTermination() clears owned termination state and rechecks for concurrent termination. Script and module evaluation consume owned requests, propagate outer requests, and handle SIGINT races.
Validate interruption and propagation
test/js/node/vm/vm.test.ts
Windows-skipped tests cover Atomics.wait, nested runs, worker execution, timeout combinations, recovery, stderr, and process exit.

Possibly related PRs

  • oven-sh/bun#35976: Modifies the same Node VM termination paths for other termination sources.
  • oven-sh/bun#35979: Updates Node VM termination attribution and consumption.
  • oven-sh/bun#36342: Changes JavaScriptCore termination-request cleanup and propagation.

Suggested reviewers: cirospaciari, jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the primary change: enabling breakOnSigint to interrupt scripts blocked in Atomics.wait.
Description check ✅ Passed The description explains the problem, cause, fix, scope, limitations, and verification results in substantial detail.

Comment @coderabbitai help to get the list of available commands.

Comment thread src/jsc/bindings/NodeVMModule.cpp Outdated
Comment thread src/jsc/bindings/NodeVMModule.cpp Outdated
Comment thread src/jsc/bindings/NodeVMScript.cpp Outdated
Comment thread src/jsc/bindings/vm/SigintWatcher.cpp Outdated
@robobun

robobun commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 11:23 AM PT - Aug 9th, 2026

@robobun, your commit 99d8eff has 1 failures in Build #91024 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 37268

That installs a local version of the PR into your bun-37268 executable, so you can run:

bun-37268 --bun

Comment thread src/jsc/bindings/NodeVM.h
Comment thread src/jsc/bindings/vm/SigintWatcher.cpp

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🔴 src/jsc/bindings/vm/SigintWatcher.cpp:209-219 — Setting vm.setHasTerminationRequest() from the watcher thread makes a pre-existing lock-ordering gap crash-reachable: ~GlobalObjectHolder unregisters the receiver before the global, so if signalAll() runs between those two steps as a breakOnSigint script (no timeout) returns normally, it skips setSigintReceived but still sets hasTerminationRequest on the still-registered VM, and checkForTermination() falls through to RELEASE_ASSERT_NOT_REACHED (same assert in the second NodeVMModule.cpp block). Swapping the destructor order in ~GlobalObjectHolder (unregister global before receivers) closes the window; alternatively, drop the assert and throw ERR_SCRIPT_EXECUTION_INTERRUPTED when breakOnSigint was armed.

    Extended reasoning...

    What the bug is

    signalAll() now calls vm.setHasTerminationRequest() directly from the watcher thread instead of only firing the NeedTermination trap. That's the right fix for the Atomics.wait hang, but it exposes a pre-existing inconsistency between how ~GlobalObjectHolder tears down and how signalAll() reads the two registration lists, turning a narrow race into a user-reachable RELEASE_ASSERT_NOT_REACHED_WITH_MESSAGE process abort.

    The code path

    ~GlobalObjectHolder (SigintWatcher.h:42-52) unregisters in this order:

    1. unregisterReceiver(script) under m_receiversMutex
    2. unregisterGlobalObject(globalObject) under m_globalObjectsMutex

    signalAll() reads in the same order — receivers under m_receiversMutex, then globals under m_globalObjectsMutex — but as two independent critical sections. Nothing keeps the two views consistent with each other.

    Step-by-step proof

    Take vm.runInThisContext(src, { breakOnSigint: true }) with no timeout, where src returns normally:

    1. run() returns; the holder destructor begins on the JS thread.
    2. JS thread: unregisterReceiver(script) locks m_receiversMutex, removes the script, releases.
    3. Watcher thread (woken by SIGINT): signalAll() locks m_receiversMutex — the script is already gone, so setSigintReceived() is not called — releases.
    4. Watcher thread: locks m_globalObjectsMutex before the JS thread does — the global is still registered — calls vm.setHasTerminationRequest() (new in this PR) and vm.notifyNeedTermination(), releases.
    5. JS thread: unregisterGlobalObject() (was blocked on step 4), then deref(); destructor finishes.
    6. JS thread: checkForTermination() sees vm.hasTerminationRequest() == true, Bun__VmHandle__scriptAllowed is true (no VM teardown), script->getSigintReceived() == false, timeout is std::nullopt → falls through to RELEASE_ASSERT_NOT_REACHED_WITH_MESSAGE("vm.Script terminated due neither to SIGINT nor to timeout").

    The identical assert in the second termination block of NodeVMModule::evaluate (breakOnSigint && timeout == 0) is reachable via the same interleaving.

    Why this wasn't reachable before

    Before this PR, step 4 only called vm.notifyNeedTermination(), which fires the NeedTermination trap but does not set m_hasTerminationRequest — that flag is set by VMTraps::handleTraps at a JS safepoint. There is no safepoint between the holder destructor and checkForTermination()'s vm.hasTerminationRequest() check (only watchdog()->setTimeLimit runs between them), so the flag stayed false and checkForTermination() returned false. That left a lingering trap (a latent bug), but never hit the assert. The direct setHasTerminationRequest() added here is what makes the assert reachable.

    Impact

    The window is a handful of instructions — the JS thread must be preempted between releasing m_receiversMutex and acquiring m_globalObjectsMutex, and the watcher must win the race for m_globalObjectsMutex. But the watcher was likely blocked on exactly the m_receiversMutex the JS thread just released, so it wakes right into the gap; and SIGINT timing is user-controlled (e.g. process.kill in a tight loop from a worker). Per REVIEW.md, a user-reachable RELEASE_ASSERT is a panic-on-user-input / DoS regardless of window width, and this PR is specifically hardening the SIGINT-termination cleanup path, so it belongs here.

    How to fix

    Two self-contained options; either is sufficient:

    • Reorder ~GlobalObjectHolder to unregister the global before the receivers. Then any signalAll() that observes the receiver gone necessarily also observes the global gone (the destructor's steps are sequential and signalAll() reads receivers first), so setHasTerminationRequest() cannot be called without the paired setSigintReceived().
    • Drop the RELEASE_ASSERT in checkForTermination (and the NodeVMModule.cpp equivalent) and throw ERR_SCRIPT_EXECUTION_INTERRUPTED in the else branch. Bun__VmHandle__scriptAllowed has already filtered non-node:vm terminations, so with timeout unset and breakOnSigint armed the only remaining source of hasTerminationRequest is a late SIGINT from the watcher — reporting it as an interrupt is the correct user-visible behavior.

A SIGINT can hit the watcher while the holder is mid-registration or
mid-teardown: signalAll then observes the global object but not the
script receiver, setting hasTerminationRequest without the paired
sigintReceived flag. checkForTermination used to fall through to
RELEASE_ASSERT_NOT_REACHED on that state; with breakOnSigint armed it
now reports ERR_SCRIPT_EXECUTION_INTERRUPTED like any other SIGINT.
@robobun

robobun commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

Good catch on the raced SIGINT, addressed in 768b032.

The tear is real and my change did make the RELEASE_ASSERT reachable from the teardown window. It also exists at registration (the holder registers the global before the receiver, and signalAll reads receivers before globals), so reordering the destructor alone cannot close it: the watcher can always read the receiver list before a registration pair is complete. The fix is therefore at the consumption point, per your second option: when breakOnSigint was armed and the request arrives without the paired sigintReceived flag (and no timeout), checkForTermination and both NodeVMModule blocks now report ERR_SCRIPT_EXECUTION_INTERRUPTED. The assert stays for the neither-armed state, which the watcher cannot produce: signalAll's global-list section is serialized with the holder's unregister by the same mutex, so a request it sets is always observed by the checkForTermination of the run that registered the global, never by a later run.

I also ran a stress probe (worker spamming process.kill SIGINT for 2.5s against ~1500 short breakOnSigint runs, ~450 mid-run interrupts per round): no assert fires even on the previous commit, so the window is too narrow to force from JS; the fix is justified by the interleaving argument rather than a reproducing test.

Comment thread src/jsc/bindings/NodeVMModule.cpp Outdated
Comment thread src/jsc/bindings/NodeVMModule.cpp Outdated
Comment thread src/jsc/bindings/NodeVMScript.cpp Outdated
Comment thread src/jsc/bindings/vm/SigintWatcher.cpp
fireTrap only notifies the VM's sync waiter on the first thread-stop
request, so with {timeout} armed the watchdog's NeedWatchdogCheck claims
that transition and a later SIGINT never wakes the parked Atomics.wait.
POSIX usually recovers through the trap SignalSender's retry loop, but
that is compiled out on Windows and under usePollingTraps, and composed
mode aborted debug builds through a stale NeedWatchdogCheck reaching
Watchdog::startTimer after the time limit was restored. The watcher now
notifies vm.syncWaiter() directly, and consumeTermination clears
NeedWatchdogCheck alongside NeedTermination.

Adds tests for the worker-hosted breakOnSigint guest and for the
composed {breakOnSigint, timeout} mode under both trap configurations.
Comment thread src/jsc/bindings/NodeVM.cpp Outdated
Comment thread src/jsc/bindings/vm/SigintWatcher.cpp
@robobun

robobun commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

Re the repeated raced-SIGINT finding: that was addressed in 768b032 (the ERR_SCRIPT_EXECUTION_INTERRUPTED fallback at the consume sites, covering both the registration-side and teardown-side windows); the review was against the earlier diff. Since then, 4766251 also fixed wake delivery for the composed { breakOnSigint, timeout } case (direct syncWaiter notify, so the interrupt works on Windows and under usePollingTraps where there is no SignalSender retry loop) and stood down the stale NeedWatchdogCheck that tripped ASSERT(hasTimeLimit()) on debug builds. Windows x64 verified manually with a real console CTRL_C_EVENT: plain and composed both interrupt cleanly; details in the updated PR description.

Comment thread src/jsc/bindings/NodeVM.cpp Outdated
Comment thread src/jsc/bindings/NodeVMScript.cpp Outdated
A nested vm run with no options observes the shared VM's termination
request when an outer frame's breakOnSigint or timeout fires, and its
cleanup hit RELEASE_ASSERT_NOT_REACHED (a crash in release builds:
vm.runInThisContext nesting a plain run under {timeout} aborts on main).
The consume sites now leave a request pending when this run armed
nothing and carries no sigintReceived flag, so the frame that armed the
interrupt reports it, as node does.

consumeTermination also re-checks scriptAllowed after clearing the
traps: a worker.terminate() landing after the caller's check would
otherwise have its NeedTermination trap silently discarded (pre-PR the
trap survived and re-fired at the next safepoint). fireTrap and
clearTrap serialize on the trap-signaling lock, so a stop whose trap was
cleared is visible to the re-check and gets re-delivered.
Comment thread src/jsc/bindings/NodeVM.cpp Outdated
Comment thread src/jsc/bindings/NodeVM.cpp
Comment thread src/jsc/bindings/NodeVM.h
Comment thread src/jsc/bindings/NodeVMModule.cpp Outdated
Comment thread src/jsc/bindings/NodeVMModule.cpp Outdated
Comment thread src/jsc/bindings/NodeVMModule.cpp Outdated
Comment thread src/jsc/bindings/NodeVMModule.cpp Outdated
Comment thread src/jsc/bindings/NodeVMScript.cpp Outdated
Comment thread src/jsc/bindings/NodeVMScript.cpp
@robobun

robobun commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

Both findings were real; addressed in 2c790f8.

Nested frames: confirmed, and it is worse than the SIGINT case suggests. The nested variant needs no SIGINT at all: an inner optionless run under an outer { timeout } hits the same RELEASE_ASSERT, so vm.runInThisContext("vm.runInThisContext('for(;;);');", { timeout: 100 }) aborts released bun on main today (node throws ERR_SCRIPT_EXECUTION_TIMEOUT). Implemented your first option: when a run armed nothing and carries no sigintReceived flag, the consume sites return before touching any termination state, and the interrupt propagates (uncatchable by guest JS) to the frame that armed it. The assert is gone from all three sites; new tests cover the nested timeout (cross-platform) and a nested plain run parked in Atomics.wait under an outer breakOnSigint.

Raced worker.terminate(): also confirmed as a regression of the new clearTrap. consumeTermination now re-checks scriptAllowed after the clears and re-delivers the stop; fireTrap and clearTrap both serialize on VMTraps' trap-signaling lock, so a stop whose trap the clears ate is provably visible to that re-check (the stop store happens before its fireTrap's critical section, which precedes ours). The callers propagate the termination in that case instead of reporting ERR_SCRIPT_EXECUTION_*.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/jsc/bindings/NodeVMModule.cpp (1)

269-295: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Extract the shared termination-reporting block.

Lines 110-134 and 269-295 now implement the same protocol: ownership gate, microtask clear, exception clear, consumeTermination with re-delivery, then the SIGINT / timeout / raced-SIGINT branches. The two copies must stay in step; the clearException() guard already differs from the script path. Extract one helper (for example reportOwnedTermination(vm, globalObject, scope, timeout, breakOnSigint)) and call it from both sites.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/jsc/bindings/NodeVMModule.cpp` around lines 269 - 295, Extract the
duplicated termination-reporting protocol from the termination handling near the
existing script path and the shown NodeVM path into a shared helper such as
reportOwnedTermination, preserving each path’s existing clearException guard and
ownership behavior. Have the helper perform microtask draining, exception
clearing, consumeTermination re-delivery, and the SIGINT, timeout, and
raced-SIGINT error branches, then replace both inline blocks with calls to it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/jsc/bindings/NodeVMModule.cpp`:
- Line 118: Guard the clearException calls in both module termination paths with
vm.hasPendingTerminationException(): update the already-evaluated afterEvaluate
path at src/jsc/bindings/NodeVMModule.cpp:118-118 and the normal evaluation path
at src/jsc/bindings/NodeVMModule.cpp:278-278. Leave regular pending user
exceptions untouched while still clearing actual pending termination exceptions.

In `@src/jsc/bindings/NodeVMScript.cpp`:
- Around line 329-330: Update the termination handling around consumeTermination
in checkForTermination to call vm.throwTerminationException() when
consumeTermination returns false before returning false. Keep the existing
request-flag reset and trap behavior, and align this path with
NodeVMModule::evaluate so callers’ RETURN_IF_EXCEPTION checks observe the
termination.

In `@test/js/node/vm/vm.test.ts`:
- Around line 1611-1614: Replace the fixed-duration Atomics.wait re-park delays
in test/js/node/vm/vm.test.ts at lines 1611-1614 and 1683-1685 with a second
while loop spinning on Atomics.notify until it returns non-zero. Update both
fixtures so SIGINT is sent only after the observable parked condition,
preserving the existing interrupt and timeout assertions.

---

Outside diff comments:
In `@src/jsc/bindings/NodeVMModule.cpp`:
- Around line 269-295: Extract the duplicated termination-reporting protocol
from the termination handling near the existing script path and the shown NodeVM
path into a shared helper such as reportOwnedTermination, preserving each path’s
existing clearException guard and ownership behavior. Have the helper perform
microtask draining, exception clearing, consumeTermination re-delivery, and the
SIGINT, timeout, and raced-SIGINT error branches, then replace both inline
blocks with calls to it.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: cb8e4134-7eb2-4c3d-96df-48db99887dfd

📥 Commits

Reviewing files that changed from the base of the PR and between 54eba71 and 2c790f8.

📒 Files selected for processing (6)
  • src/jsc/bindings/NodeVM.cpp
  • src/jsc/bindings/NodeVM.h
  • src/jsc/bindings/NodeVMModule.cpp
  • src/jsc/bindings/NodeVMScript.cpp
  • src/jsc/bindings/vm/SigintWatcher.cpp
  • test/js/node/vm/vm.test.ts

Comment thread src/jsc/bindings/NodeVMModule.cpp Outdated
Comment thread src/jsc/bindings/NodeVMScript.cpp Outdated
Comment thread test/js/node/vm/vm.test.ts Outdated
Extract NodeVMModule::reportOwnedTermination so the two evaluate blocks
share one ownership gate, guard the clearException with
hasPendingTerminationException so a pending user exception is not
discarded, throw the termination immediately when a raced worker stop is
re-delivered in checkForTermination, and replace the fixed re-park
delays in the test fixtures with a second Atomics.notify spin (the
composed-mode fixture keeps its 400ms wait, which orders the 200ms
watchdog fire before the SIGINT).
Comment thread src/jsc/bindings/NodeVMModule.cpp Outdated
Comment thread src/jsc/bindings/NodeVMModule.cpp
Comment thread src/jsc/bindings/NodeVMModule.cpp
Comment thread src/jsc/bindings/NodeVMModule.h
Comment thread src/jsc/bindings/NodeVMScript.cpp
Comment thread src/jsc/bindings/NodeVM.cpp Outdated
An inner breakOnSigint-only run nested in an outer {timeout} claimed the
outer watchdog's termination as a raced SIGINT: the misattributed error
is catchable, and the inner never re-arms the outer's one-shot watchdog,
so a guest that swallowed it ran past the outer timeout forever (before
this branch existed the same state hit the RELEASE_ASSERT). breakOnSigint
alone no longer claims a termination while an enclosing frame's watchdog
still has a time limit armed, and consumeTermination only stands down
NeedWatchdogCheck for the run that armed the watchdog, so an outer
frame's pending check keeps enforcing its budget.
Comment thread src/jsc/bindings/NodeVM.cpp
Comment thread src/jsc/bindings/NodeVM.h
Comment thread src/jsc/bindings/NodeVM.h
Comment thread src/jsc/bindings/NodeVMModule.cpp
Comment thread src/jsc/bindings/NodeVMScript.cpp
@robobun

robobun commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

Confirmed and fixed in 99d8eff. The repro on the previous head printed the misattributed interrupt and then hung past the outer timeout exactly as described; with the fix it reports ERR_SCRIPT_EXECUTION_TIMEOUT from the outer frame and exits.

Both suggested pieces are in: breakOnSigint alone no longer claims a termination while an enclosing frame's watchdog still has a time limit armed (Watchdog::hasTimeLimit(), checked before any state is touched, so the request propagates intact), and consumeTermination now takes the run's own timeout and only clears NeedWatchdogCheck when this run armed the watchdog. The checks run after this run's own time-limit restore, so hasTimeLimit() reflects enclosing frames only; the composed single-run { breakOnSigint, timeout } case is unaffected (own timeout claims it), and the plain raced-SIGINT tear still resolves to ERR_SCRIPT_EXECUTION_INTERRUPTED since no watchdog is armed there. New cross-platform test covers the bypass: guest swallows whatever the inner throws and spins; it must still be bounded by the outer timeout.

Comment thread src/jsc/bindings/NodeVMScript.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant