node:vm: make breakOnSigint interrupt a script parked in Atomics.wait - #37268
Open
robobun wants to merge 9 commits into
Open
node:vm: make breakOnSigint interrupt a script parked in Atomics.wait#37268robobun wants to merge 9 commits into
robobun wants to merge 9 commits into
Claude / Claude Code Review
completed
Aug 9, 2026 in 40m 0s
Code review found 3 important issues
Found 2 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 3 |
| 🟡 Nit | 0 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/jsc/bindings/NodeVM.cpp:533-537 |
consumeTermination clearTrap can swallow a raced worker.terminate(), letting the worker hang |
| 🔴 Important | src/jsc/bindings/NodeVMScript.cpp:330-336 |
RELEASE_ASSERT still reachable via nested vm run where inner has neither breakOnSigint nor timeout |
Annotations
Check failure on line 537 in src/jsc/bindings/NodeVM.cpp
claude / Claude Code Review
consumeTermination clearTrap can swallow a raced worker.terminate(), letting the worker hang
`consumeTermination()` adds `vm.traps().clearTrap(NeedTermination)`, but the guarding `Bun__VmHandle__scriptAllowed(...)` check at each call site is a TOCTOU: a `worker.terminate()` that lands between that check and `clearTrap` has its `NeedTermination` trap silently discarded. `WebWorker::request_termination` (web_worker.rs:487-493) does `handle().stop()` then `notifyNeedTermination()` — the trap is the only thing that interrupts already-running JS, so worker JS that catches the `ERR_SCRIPT_EXE
Check failure on line 336 in src/jsc/bindings/NodeVMScript.cpp
claude / Claude Code Review
RELEASE_ASSERT still reachable via nested vm run where inner has neither breakOnSigint nor timeout
The `RELEASE_ASSERT_NOT_REACHED` is still user-reachable via nested vm runs: an outer `runInThisContext({breakOnSigint:true})` whose guest calls an inner `script.runInThisContext()` with no options leaves the outer's globalObject registered in the watcher; a SIGINT during the inner run makes `signalAll()` set `vm.setHasTerminationRequest()` on the shared VM and `sigintReceived` only on the *outer* script, so the inner's `checkForTermination` sees hasTerminationRequest=true, `innerScript->getSigi
Loading