node:vm: keep already-queued promise reactions after a timed-out evaluation - #33763
node:vm: keep already-queued promise reactions after a timed-out evaluation#33763robobun wants to merge 2 commits into
Conversation
…uation
When a vm.runInContext / runInThisContext / SourceTextModule.evaluate
with { timeout } expires, checkForTermination called
VM::drainMicrotasksForGlobalObject before throwing
ERR_SCRIPT_EXECUTION_TIMEOUT. Despite the name, that helper clears every
pending microtask whose globalObject matches, so promise reactions the
guest had already enqueued on the shared host queue were silently
discarded, and a host .then() on a promise the guest handed back never
settled. runInThisContext passed the caller's global, so it also dropped
the caller's own pending reactions.
Node only stops execution on timeout; jobs that were already queued run
at the next host checkpoint. Stop clearing them.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
WalkthroughThis PR removes explicit microtask-draining calls ( ChangesVM Termination Microtask Drain Removal
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:14 AM PT - Jul 8th, 2026
❌ @robobun, your commit 37b8d9d has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33763That installs a local version of the PR into your bun-33763 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find issues, but this changes vm termination semantics by removing microtask-clearing on timeout and overlaps with #33411's narrower approach — worth a human confirming the removal (vs. narrowing) is the right call.
Extended reasoning...
Overview
Removes three vm.drainMicrotasksForGlobalObject(...) calls from the termination-handling paths in NodeVMScript.cpp (checkForTermination) and NodeVMModule.cpp (two branches of NodeVMModule::evaluate). Adds three subprocess-based regression tests in test/js/node/vm/vm.test.ts covering runInContext, runInThisContext, and SourceTextModule.evaluate.
The PR's claim is that drainMicrotasksForGlobalObject — despite its name — clears rather than executes pending microtasks (via MicrotaskQueue::clearForGlobalObject), so already-queued promise reactions were silently discarded when a timed evaluation was terminated. The fix lets those reactions survive to run at the next host checkpoint, matching Node.
Security risks
node:vm is sandboxing-adjacent. The change means guest-queued microtasks now survive a timeout and run afterward on the host's queue. The PR argues this already matches Node's behavior in default microtask mode (guest and host share the queue), and microtaskMode: 'afterEvaluate' is unaffected since it uses a separate per-context queue. I don't see a new escape vector here — the guest could already schedule work observable to the host via the shared context object — but it does change what runs after a forced termination, which is the kind of semantic shift a human should confirm.
Level of scrutiny
Moderate-to-high. The native diff is only three deleted lines, but it removes code that was presumably added intentionally in a subtle area (JSC watchdog termination × microtask queue × cross-realm promises). Verifying correctness requires knowing JSC's drainMicrotasksForGlobalObject internals, which I couldn't inspect locally. The PR description is thorough and the tests are well-constructed (verified fail-before/pass-after, plus Node parallel test suites), but this is a behavioral change rather than a mechanical fix.
Other factors
- The PR explicitly notes overlap with #33411, which narrows the same call to the vm-context global instead of removing it. A human should decide between the two approaches (or confirm this supersedes #33411).
- The new tests assert
stderr: ""exactly, which can be brittle under ASAN/debug builds — though several existing tests in the same file do the same, so this follows local convention. - No CODEOWNERS entry covers these files. No prior human review comments on the PR.
|
On removal vs. narrowing (#33411): Narrowing to the vm-context global fixes the $ node -e 'const vm=require("vm");const c=vm.createContext({r:false});
try{vm.runInContext("Promise.resolve().then(()=>{r=true});for(;;);",c,{timeout:50})}catch{}
setTimeout(()=>console.log(c.r),0)'
trueFor So narrowing is a subset: it would still leave the The |
There was a problem hiding this comment.
The reasoning and tests look solid, but since this changes what guest microtasks survive a node:vm timeout (sandbox-adjacent semantics) and takes a different approach than #33411, it's worth a maintainer confirming removal-over-narrowing is the right call.
Extended reasoning...
Overview
Removes three vm.drainMicrotasksForGlobalObject(...) calls from the timeout/SIGINT termination paths in NodeVMScript.cpp (checkForTermination) and NodeVMModule.cpp (NodeVMModule::evaluate, both branches). Adds three subprocess tests in test/js/node/vm/vm.test.ts covering runInContext, runInThisContext, and SourceTextModule.evaluate. The claim is that this JSC helper clears (rather than executes) matching entries on the VM's default microtask queue, so already-queued promise reactions were being silently dropped when a timed evaluation expired — leaving host-held promises permanently pending, which diverges from Node.
Security risks
node:vm is a sandbox boundary, and this change affects which guest-scheduled microtasks run after the guest has been forcibly terminated. The PR argues (convincingly) that the actual isolation mode, microtaskMode: "afterEvaluate", uses a per-context queue that these calls never touched, and that the Node parallel test-vm-timeout-escape-promise* suites still pass. In default microtask mode the guest already shares the host queue by design, so preserving those reactions matches Node rather than weakening isolation. Still, "guest code runs after termination" is exactly the class of behavior a human reviewer should sign off on.
Level of scrutiny
Medium-high. The native diff is trivially small (pure deletions), but it is a deliberate semantic change to termination behavior in a security-adjacent module, and it competes with an open PR (#33411) that instead narrows the same call to the vm-context global. The author has justified removal over narrowing in the thread, but choosing between the two is a maintainer-level design decision. I could not trace the original rationale for these calls from local git history (the file was introduced whole in a squashed commit), so the "is this load-bearing?" question is answered only by the author's analysis and the passing Node compat suites.
Other factors
Test quality is good: each case runs in a fresh subprocess (isolating termination state), pipes are drained concurrently, exact stdout is asserted alongside the ERR_SCRIPT_EXECUTION_TIMEOUT code, and the author reports USE_SYSTEM_BUN=1 fails 0/3 while the debug build passes 3/3 plus the full vm.test.ts and Node's test-vm-timeout* / test-vm-module-* suites. The stderr: "" assertion mirrors adjacent tests in the same file. No bugs were flagged by the bug-hunting pass. Given the sandbox-adjacent semantics and the overlapping PR, deferring rather than auto-approving.
|
CI status: the diff itself is green. The red is unrelated flake, different on each run:
No overlap between the two runs; none of the failing files touch |
1 participant
Repro
The standard sandbox pattern is "give the guest a CPU budget, then await whatever promise it handed back". On bun, jobs that were already enqueued for already-settled promises were discarded with the terminated evaluation, so the host's
.thenon the guest's promise never settled and the host hung with no error. The.catchshape was worse: a rejection handler the guest attached before expiry never ran, and nounhandledRejectionwas reported either.runInThisContextwas affected even more directly: it passed the caller's global, so a timed-out evaluation also dropped the caller's own pending reactions queued before the vm call.Cause
checkForTermination(NodeVMScript.cpp) and the two termination branches inNodeVMModule::evaluatecalledvm.drainMicrotasksForGlobalObject(globalObject)before clearing the termination request and throwingERR_SCRIPT_EXECUTION_TIMEOUT. Despite the name, that helper callsMicrotaskQueue::clearForGlobalObject, which removes every pending entry on the VM's default queue whoseglobalObject()matches. In default microtask mode the guest's reactions live on that shared queue, so they were dropped.Fix
Remove the three
drainMicrotasksForGlobalObjectcalls. Terminating a timed evaluation only stops execution; jobs that were already enqueued stay on the queue and run at the next host checkpoint once the termination request is cleared, matching Node.microtaskMode: "afterEvaluate"is unaffected: those jobs live on the context's own queue, which the removed call never touched anyway.Verification
New tests in
test/js/node/vm/vm.test.tsspawn a subprocess for each ofrunInContext,runInThisContext, andSourceTextModule.evaluate, queue a reaction on an already-resolved promise, spin into{ timeout: 80 }, and assert the reaction ran and a host.thenon the guest's promise settled.USE_SYSTEM_BUN=1 bun test test/js/node/vm/vm.test.ts -t "timeout termination preserves": 0 pass / 3 failbun bd test test/js/node/vm/vm.test.ts -t "timeout termination preserves": 3 pass / 0 failvm.test.ts: 209 pass / 0 failtest-vm-timeout*.js/test-vm-module-*suites: passOverlaps with #33411, which narrows the same call to the vm context's global for
runInThisContext; this PR removes it outright so the guest's own reactions on the shared queue survive too.