node:vm: pass the Node v26.3.0 vm test suite (+25 tests, ~65% → 97%) - #32018
Conversation
|
Updated 4:25 PM PT - Jun 22nd, 2026
❌ @cirospaciari, your commit 2a65eba has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32018That installs a local version of the PR into your bun-32018 --bun |
|
Found 6 issues this PR may fix:
🤖 Generated with Claude Code |
|
Pushed a follow-up commit (caa06e9) with fixes for edge cases the vendored suite doesn't cover, found in self-review:
Known remaining divergence: Full vm suite (100 files) and test/js/node/vm/ re-verified green after the fixes. |
5b9ebc6 to
eb22819
Compare
WalkthroughExtends Changesnode:vm Compatibility
Ancillary test infrastructure updates
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
20ce579 to
befb2b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/js/node/vm.ts`:
- Around line 468-494: The linkRequests method lacks validation for modules with
status === "errored", while the async [kLink] method explicitly checks and
rejects such modules. Add a status validation check in the linkRequests method's
validation loop (after the existing isModule and context checks) to ensure that
each module passed to linkRequests does not have an errored status, and throw an
appropriate error if one is found to maintain consistency with the [kLink]
method's behavior.
In `@src/jsc/bindings/NodeVM.cpp`:
- Line 186: The subtraction `position.m_line.zeroBasedInt() - 1` at the
TextPosition constructor call in the wrappedPosition initialization is unsafe
and can cause signed integer overflow or pass negative values to
OrdinalNumber::fromZeroBasedInt(), which expects non-negative ordinal values.
Before performing the subtraction, add a guard to ensure
position.m_line.zeroBasedInt() is at least 1, either by clamping the value to a
minimum of 1 or by validating it before the operation. Apply the same fix
pattern to the similar subtraction at lines 527-529 to ensure consistency, and
consider whether a wider integer type with bounds-checking would be more
appropriate to prevent overflow when lineOffset can be as extreme as INT_MIN.
In `@src/jsc/bindings/NodeVMModule.cpp`:
- Around line 729-754: The jsNodeVmModuleHasAsyncGraph host function does not
validate the module status before calling hasAsyncGraph(), which relies on
m_resolveCache populated during the link() operation. Add a status precondition
check in jsNodeVmModuleHasAsyncGraph before calling hasAsyncGraph() to ensure
the module is properly linked, preventing silent incorrect returns on unlinked
modules. This validation should follow the defensive pattern used in
jsNodeVmModuleHasTopLevelAwait and align with JSC binding conventions for
validating preconditions at the native layer.
In `@src/jsc/bindings/NodeVMScript.cpp`:
- Around line 288-292: The code block checking
vm.hasPendingTerminationException() violates binding-safety by explicitly
calling clearException() on the DECLARE_TOP_EXCEPTION_SCOPE. Refactor this
control flow to eliminate the direct clearException() call and instead let the
new ERR_SCRIPT_EXECUTION_* error that is thrown afterwards naturally replace the
pending termination exception, ensuring the exception handling follows the
binding-safety guidelines without the intermediate clear step.
🪄 Autofix (Beta)
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: 4dddf8ae-58d8-4036-8c2a-bfb220cf8c37
⛔ Files ignored due to path filters (1)
test/js/node/vm/__snapshots__/vm-sourceUrl.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (65)
src/js/builtins/BunBuiltinNames.hsrc/js/node/vm.tssrc/jsc/ErrorCode.rssrc/jsc/bindings/ErrorCode.cppsrc/jsc/bindings/ErrorCode.tssrc/jsc/bindings/NodeVM.cppsrc/jsc/bindings/NodeVM.hsrc/jsc/bindings/NodeVMModule.cppsrc/jsc/bindings/NodeVMModule.hsrc/jsc/bindings/NodeVMScript.cppsrc/jsc/bindings/NodeVMScript.hsrc/jsc/bindings/NodeVMSourceTextModule.cppsrc/jsc/bindings/NodeVMSourceTextModule.hsrc/jsc/bindings/NodeVMSyntheticModule.cppsrc/jsc/bindings/NodeValidator.cppsrc/jsc/bindings/vm/SigintWatcher.cppsrc/jsc/bindings/vm/SigintWatcher.htest/js/node/test/common/globals.jstest/js/node/test/parallel/test-inspector-enabled.jstest/js/node/test/parallel/test-vm-api-handles-getter-errors.jstest/js/node/test/parallel/test-vm-basic.jstest/js/node/test/parallel/test-vm-codegen.jstest/js/node/test/parallel/test-vm-context-dont-contextify.jstest/js/node/test/parallel/test-vm-context.jstest/js/node/test/parallel/test-vm-global-contextual-store.jstest/js/node/test/parallel/test-vm-global-identity.jstest/js/node/test/parallel/test-vm-global-non-writable-properties.jstest/js/node/test/parallel/test-vm-global-property-enumerator.jstest/js/node/test/parallel/test-vm-global-property-interceptors.jstest/js/node/test/parallel/test-vm-global-property-prototype.jstest/js/node/test/parallel/test-vm-global-setter.jstest/js/node/test/parallel/test-vm-measure-memory-lazy.jstest/js/node/test/parallel/test-vm-measure-memory-multi-context.jstest/js/node/test/parallel/test-vm-measure-memory.jstest/js/node/test/parallel/test-vm-module-after-evaluate.jstest/js/node/test/parallel/test-vm-module-basic.jstest/js/node/test/parallel/test-vm-module-dynamic-import-promise.jstest/js/node/test/parallel/test-vm-module-dynamic-import.jstest/js/node/test/parallel/test-vm-module-errors.jstest/js/node/test/parallel/test-vm-module-evaluate-source-text-module.jstest/js/node/test/parallel/test-vm-module-evaluate-synthethic-module-rejection.jstest/js/node/test/parallel/test-vm-module-evaluate-synthethic-module.jstest/js/node/test/parallel/test-vm-module-evaluate-while-evaluating.jstest/js/node/test/parallel/test-vm-module-hasasyncgraph.jstest/js/node/test/parallel/test-vm-module-hastoplevelawait.jstest/js/node/test/parallel/test-vm-module-instantiate.jstest/js/node/test/parallel/test-vm-module-link-shared-deps.jstest/js/node/test/parallel/test-vm-module-link.jstest/js/node/test/parallel/test-vm-module-linkmodulerequests-circular.jstest/js/node/test/parallel/test-vm-module-linkmodulerequests-deep.jstest/js/node/test/parallel/test-vm-module-referrer-realm.mjstest/js/node/test/parallel/test-vm-module-synthetic.jstest/js/node/test/parallel/test-vm-property-definer-interception.jstest/js/node/test/parallel/test-vm-property-not-on-sandbox.jstest/js/node/test/parallel/test-vm-run-in-new-context.jstest/js/node/test/parallel/test-vm-script-after-evaluate.jstest/js/node/test/parallel/test-vm-source-map-url.jstest/js/node/test/parallel/test-vm-strict-assign.jstest/js/node/test/parallel/test-vm-syntax-error-stderr.jstest/js/node/test/parallel/test-vm-timeout-escape-promise-2.jstest/js/node/test/sequential/test-vm-break-on-sigint.jstest/js/node/test/sequential/test-vm-timeout-escape-promise-module-2.jstest/js/node/vm/vm.test.tstest/js/node/watch/fs.watch.test.tstest/napi/node-napi-tests/test/fixtures/dotenv/uv-threadpool.env
💤 Files with no reviewable changes (1)
- test/js/node/test/parallel/test-inspector-enabled.js
befb2b7 to
eceb256
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/js/node/vm.ts`:
- Around line 122-128: In the emitExperimentalWarning function, replace the
regular Set methods with their tamper-resistant dollar-sign prefixed equivalents
to prevent user code from tampering with these methods. Replace the call to
emittedExperimentalWarnings.has(feature) with
emittedExperimentalWarnings.$has(feature), and replace the call to
emittedExperimentalWarnings.add(feature) with
emittedExperimentalWarnings.$add(feature). This follows the established pattern
used elsewhere in the codebase such as in src/js/node/fs.ts.
In `@src/jsc/bindings/NodeVMModule.cpp`:
- Around line 197-216: After the call to
nodeVmGlobalObject->drainOwnMicrotasks() which can run user code and potentially
throw exceptions, add an exception gate check using RETURN_IF_EXCEPTION or
appropriate JSError propagation before the subsequent code that accesses
innerPromise->status() and transforms the result. This prevents the switch
statement and promise wrapping logic from executing if an exception or
termination occurred during microtask drainage.
🪄 Autofix (Beta)
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: d2cdf808-ce52-4995-a345-1a879ba0a9bb
⛔ Files ignored due to path filters (1)
test/js/node/vm/__snapshots__/vm-sourceUrl.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (65)
src/js/builtins/BunBuiltinNames.hsrc/js/node/vm.tssrc/jsc/ErrorCode.rssrc/jsc/bindings/ErrorCode.cppsrc/jsc/bindings/ErrorCode.tssrc/jsc/bindings/NodeVM.cppsrc/jsc/bindings/NodeVM.hsrc/jsc/bindings/NodeVMModule.cppsrc/jsc/bindings/NodeVMModule.hsrc/jsc/bindings/NodeVMScript.cppsrc/jsc/bindings/NodeVMScript.hsrc/jsc/bindings/NodeVMSourceTextModule.cppsrc/jsc/bindings/NodeVMSourceTextModule.hsrc/jsc/bindings/NodeVMSyntheticModule.cppsrc/jsc/bindings/NodeValidator.cppsrc/jsc/bindings/vm/SigintWatcher.cppsrc/jsc/bindings/vm/SigintWatcher.htest/js/node/test/common/globals.jstest/js/node/test/parallel/test-inspector-enabled.jstest/js/node/test/parallel/test-vm-api-handles-getter-errors.jstest/js/node/test/parallel/test-vm-basic.jstest/js/node/test/parallel/test-vm-codegen.jstest/js/node/test/parallel/test-vm-context-dont-contextify.jstest/js/node/test/parallel/test-vm-context.jstest/js/node/test/parallel/test-vm-global-contextual-store.jstest/js/node/test/parallel/test-vm-global-identity.jstest/js/node/test/parallel/test-vm-global-non-writable-properties.jstest/js/node/test/parallel/test-vm-global-property-enumerator.jstest/js/node/test/parallel/test-vm-global-property-interceptors.jstest/js/node/test/parallel/test-vm-global-property-prototype.jstest/js/node/test/parallel/test-vm-global-setter.jstest/js/node/test/parallel/test-vm-measure-memory-lazy.jstest/js/node/test/parallel/test-vm-measure-memory-multi-context.jstest/js/node/test/parallel/test-vm-measure-memory.jstest/js/node/test/parallel/test-vm-module-after-evaluate.jstest/js/node/test/parallel/test-vm-module-basic.jstest/js/node/test/parallel/test-vm-module-dynamic-import-promise.jstest/js/node/test/parallel/test-vm-module-dynamic-import.jstest/js/node/test/parallel/test-vm-module-errors.jstest/js/node/test/parallel/test-vm-module-evaluate-source-text-module.jstest/js/node/test/parallel/test-vm-module-evaluate-synthethic-module-rejection.jstest/js/node/test/parallel/test-vm-module-evaluate-synthethic-module.jstest/js/node/test/parallel/test-vm-module-evaluate-while-evaluating.jstest/js/node/test/parallel/test-vm-module-hasasyncgraph.jstest/js/node/test/parallel/test-vm-module-hastoplevelawait.jstest/js/node/test/parallel/test-vm-module-instantiate.jstest/js/node/test/parallel/test-vm-module-link-shared-deps.jstest/js/node/test/parallel/test-vm-module-link.jstest/js/node/test/parallel/test-vm-module-linkmodulerequests-circular.jstest/js/node/test/parallel/test-vm-module-linkmodulerequests-deep.jstest/js/node/test/parallel/test-vm-module-referrer-realm.mjstest/js/node/test/parallel/test-vm-module-synthetic.jstest/js/node/test/parallel/test-vm-property-definer-interception.jstest/js/node/test/parallel/test-vm-property-not-on-sandbox.jstest/js/node/test/parallel/test-vm-run-in-new-context.jstest/js/node/test/parallel/test-vm-script-after-evaluate.jstest/js/node/test/parallel/test-vm-source-map-url.jstest/js/node/test/parallel/test-vm-strict-assign.jstest/js/node/test/parallel/test-vm-syntax-error-stderr.jstest/js/node/test/parallel/test-vm-timeout-escape-promise-2.jstest/js/node/test/sequential/test-vm-break-on-sigint.jstest/js/node/test/sequential/test-vm-timeout-escape-promise-module-2.jstest/js/node/vm/vm.test.tstest/js/node/watch/fs.watch.test.tstest/napi/node-napi-tests/test/fixtures/dotenv/uv-threadpool.env
💤 Files with no reviewable changes (1)
- test/js/node/test/parallel/test-inspector-enabled.js
eceb256 to
dcb252c
Compare
dcb252c to
64da844
Compare
Vendor the Node v26.3.0 vm tests (25 new files, 18 refreshed to match upstream) and fix the runtime gaps they surface: - v26 module API: linkRequests(), instantiate(), moduleRequests, hasTopLevelAwait(), hasAsyncGraph(); link()/instantiate() now follow Node's status protocol (link stores resolutions, instantiate flips status and propagates across the graph) - top-level-await modules: switch to JSC's spec-style record evaluate so the async machinery runs to completion (namespace no longer TDZs) - microtaskMode 'afterEvaluate': per-context microtask queue, drained inside the watchdog scope so timeouts fire instead of hanging - vm.measureMemory(): implemented (summary/detailed, context tracking) - SyntheticModule: instantiated at construction like Node; exports initialize to undefined instead of TDZ; re-entrant evaluate rejects - evaluate() returns settled promises for sync completions and rethrows the recorded error for errored modules (same instance every time) - contextify interceptors: defineProperty forwards to Proxy sandboxes, prototype-chain properties are no longer reported as own, sandbox identity maps to the context global, indexed access intercepts - codeGeneration.wasm:false now gates synchronous WebAssembly.Module - script.sourceMapURL available before first run - vm script errors carry Node's source line + caret decoration - compileFunction: filename default matches Node in both call shapes, lineOffset lines up with the body for offsets >= 1 - SigintWatcher: refcount race with worker_threads fixed (lock) - ERR_INVALID_ARG_TYPE phrasing for free-form phrases now matches Node; validateArrayBufferView reports the instance list like validateBuffer; new ERR_MODULE_LINK_MISMATCH code 98 of Node's 101 vm tests pass (97%, from 66 before). Not vendored: 2 tests needing source-phase imports (unsupported by the engine parser) and 1 asserting --experimental-vm-modules flag gating that bun deliberately does not have. Test-side deviations are limited to engine-inherent message text and column attribution, each marked with an inline comment.
64da844 to
2a65eba
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
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/NodeVMScript.cpp (1)
284-292: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle pending termination exceptions even when the request bit is already clear.
drainOwnMicrotasks()can leave a pending termination exception after the afterEvaluate checkpoint; this helper only enters whenhasTerminationRequest()is true, so a pending-only termination can bypass the documentedERR_SCRIPT_EXECUTION_*conversion and hit the later exception barrier.🐛 Proposed fix
- if (vm.hasTerminationRequest()) { + if (vm.hasTerminationRequest() || vm.hasPendingTerminationException()) { vm.drainMicrotasksForGlobalObject(globalObject); // The termination may have fired inside an afterEvaluate microtask // checkpoint, leaving the termination exception pending; clear it soAlso applies to: 365-391
🤖 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/NodeVMScript.cpp` around lines 284 - 292, The checkForTermination function only checks for pending termination exceptions inside the hasTerminationRequest() conditional block. However, drainMicrotasksForGlobalObject() can leave a pending termination exception even after the termination request bit is cleared, allowing a pending-only termination to bypass the ERR_SCRIPT_EXECUTION_* error conversion. Move the pending termination exception check (the if statement checking vm.hasPendingTerminationException() and calling clearException) outside and after the hasTerminationRequest() block so it handles both cases: when there is an active termination request AND when there is only a pending termination exception with no active request. Apply this same fix to all other locations mentioned in the comment where similar patterns exist.
🤖 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/js/node/vm.ts`:
- Around line 590-593: The link() method in the SyntheticModule class currently
returns undefined synchronously, but it overrides the async Module.link() method
which returns a Promise. This breaks callers that use .then() chaining or rely
on Promise-based error handling. Update the link() method to return a resolved
Promise instead of returning undefined, ensuring it maintains the Promise-shaped
interface of the parent Module.link() method while still being a no-op for
synthetic modules.
- Around line 299-314: The timeout default value is set to -1 when
options.timeout is undefined, but the native NodeVMModule::evaluate method
treats any non-zero timeout as an active watchdog timer. When -1 crosses the
unsigned integer boundary, it becomes 4294967295, which can override a shorter
enclosing watchdog. Change the default assignment from timeout = -1 to timeout =
0 in the conditional block where timeout === undefined to properly signal that
no timeout is set.
In `@src/jsc/bindings/NodeVM.cpp`:
- Around line 500-508: The idempotency guard that checks
vmErrorDecoratedPrivateName() is currently positioned too late in the error
handling flow, after materializeErrorInfoIfNeeded() and errorInstance->get()
calls, allowing a previously decorated error to still execute user-defined
getters or fail before the marker is honored. Move the decoration marker check
using vmErrorDecoratedPrivateName() and errorInstance->getDirect() to execute
immediately after the ErrorInstance cast, before any calls to
materializeErrorInfoIfNeeded() or reading the stack property, so that decorated
errors are properly rejected early and undecorated errors proceed with normal
stack formatting.
- Around line 1318-1332: The code currently performs a preflight probe by
calling contextifiedObject->getPropertySlot() to check if a property is declared
on the sandbox before dispatching defineOwnProperty for data descriptors. This
preflight check can trigger Proxy traps prematurely, violating the "observe
exactly once" requirement stated in the comment. To fix this, remove the
isDeclaredOnSandbox check and the conditional block that depends on it (around
the getPropertySlot call and the subsequent if statement checking
isDeclaredOnSandbox and !isDeclaredOnGlobalProxy), and instead dispatch directly
to contextifiedObject->methodTable()->defineOwnProperty() without preflighting,
matching the approach already used for accessor descriptors in the earlier if
block.
In `@src/jsc/bindings/NodeVMModule.cpp`:
- Around line 95-120: The fast path that calls drainOwnMicrotasks() on
nodeVmGlobalObject does not properly mirror the full evaluation safeguards. Wrap
the drainOwnMicrotasks() call with a SIGINT holder (similar to the breakOnSigint
pattern used during initial evaluation) to respect SIGINT handling.
Additionally, after the microtask drain completes, check for any non-termination
exceptions that may have been left pending by the checkpoint operation and
propagate them using RETURN_IF_EXCEPTION before returning
m_evaluationResult.get(), ensuring exceptions are validated and handled
consistently with the coding guidelines.
---
Outside diff comments:
In `@src/jsc/bindings/NodeVMScript.cpp`:
- Around line 284-292: The checkForTermination function only checks for pending
termination exceptions inside the hasTerminationRequest() conditional block.
However, drainMicrotasksForGlobalObject() can leave a pending termination
exception even after the termination request bit is cleared, allowing a
pending-only termination to bypass the ERR_SCRIPT_EXECUTION_* error conversion.
Move the pending termination exception check (the if statement checking
vm.hasPendingTerminationException() and calling clearException) outside and
after the hasTerminationRequest() block so it handles both cases: when there is
an active termination request AND when there is only a pending termination
exception with no active request. Apply this same fix to all other locations
mentioned in the comment where similar patterns exist.
🪄 Autofix (Beta)
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: 7d6701c4-71ff-491d-9a61-a1a657413649
⛔ Files ignored due to path filters (1)
test/js/node/vm/__snapshots__/vm-sourceUrl.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (65)
src/js/builtins/BunBuiltinNames.hsrc/js/node/vm.tssrc/jsc/ErrorCode.rssrc/jsc/bindings/ErrorCode.cppsrc/jsc/bindings/ErrorCode.tssrc/jsc/bindings/NodeVM.cppsrc/jsc/bindings/NodeVM.hsrc/jsc/bindings/NodeVMModule.cppsrc/jsc/bindings/NodeVMModule.hsrc/jsc/bindings/NodeVMScript.cppsrc/jsc/bindings/NodeVMScript.hsrc/jsc/bindings/NodeVMSourceTextModule.cppsrc/jsc/bindings/NodeVMSourceTextModule.hsrc/jsc/bindings/NodeVMSyntheticModule.cppsrc/jsc/bindings/NodeValidator.cppsrc/jsc/bindings/vm/SigintWatcher.cppsrc/jsc/bindings/vm/SigintWatcher.htest/js/node/test/common/globals.jstest/js/node/test/parallel/test-inspector-enabled.jstest/js/node/test/parallel/test-vm-api-handles-getter-errors.jstest/js/node/test/parallel/test-vm-basic.jstest/js/node/test/parallel/test-vm-codegen.jstest/js/node/test/parallel/test-vm-context-dont-contextify.jstest/js/node/test/parallel/test-vm-context.jstest/js/node/test/parallel/test-vm-global-contextual-store.jstest/js/node/test/parallel/test-vm-global-identity.jstest/js/node/test/parallel/test-vm-global-non-writable-properties.jstest/js/node/test/parallel/test-vm-global-property-enumerator.jstest/js/node/test/parallel/test-vm-global-property-interceptors.jstest/js/node/test/parallel/test-vm-global-property-prototype.jstest/js/node/test/parallel/test-vm-global-setter.jstest/js/node/test/parallel/test-vm-measure-memory-lazy.jstest/js/node/test/parallel/test-vm-measure-memory-multi-context.jstest/js/node/test/parallel/test-vm-measure-memory.jstest/js/node/test/parallel/test-vm-module-after-evaluate.jstest/js/node/test/parallel/test-vm-module-basic.jstest/js/node/test/parallel/test-vm-module-dynamic-import-promise.jstest/js/node/test/parallel/test-vm-module-dynamic-import.jstest/js/node/test/parallel/test-vm-module-errors.jstest/js/node/test/parallel/test-vm-module-evaluate-source-text-module.jstest/js/node/test/parallel/test-vm-module-evaluate-synthethic-module-rejection.jstest/js/node/test/parallel/test-vm-module-evaluate-synthethic-module.jstest/js/node/test/parallel/test-vm-module-evaluate-while-evaluating.jstest/js/node/test/parallel/test-vm-module-hasasyncgraph.jstest/js/node/test/parallel/test-vm-module-hastoplevelawait.jstest/js/node/test/parallel/test-vm-module-instantiate.jstest/js/node/test/parallel/test-vm-module-link-shared-deps.jstest/js/node/test/parallel/test-vm-module-link.jstest/js/node/test/parallel/test-vm-module-linkmodulerequests-circular.jstest/js/node/test/parallel/test-vm-module-linkmodulerequests-deep.jstest/js/node/test/parallel/test-vm-module-referrer-realm.mjstest/js/node/test/parallel/test-vm-module-synthetic.jstest/js/node/test/parallel/test-vm-property-definer-interception.jstest/js/node/test/parallel/test-vm-property-not-on-sandbox.jstest/js/node/test/parallel/test-vm-run-in-new-context.jstest/js/node/test/parallel/test-vm-script-after-evaluate.jstest/js/node/test/parallel/test-vm-source-map-url.jstest/js/node/test/parallel/test-vm-strict-assign.jstest/js/node/test/parallel/test-vm-syntax-error-stderr.jstest/js/node/test/parallel/test-vm-timeout-escape-promise-2.jstest/js/node/test/sequential/test-vm-break-on-sigint.jstest/js/node/test/sequential/test-vm-timeout-escape-promise-module-2.jstest/js/node/vm/vm.test.tstest/js/node/watch/fs.watch.test.tstest/napi/node-napi-tests/test/fixtures/dotenv/uv-threadpool.env
💤 Files with no reviewable changes (1)
- test/js/node/test/parallel/test-inspector-enabled.js
…ompat ErrorCode.rs: #32018 inserted ERR_VM_MODULE_DIFFERENT_CONTEXT at 304 and shifted everything after by +1; renumber WORKER_MESSAGING_* and the appended tail (318-331), COUNT=332.
|
🎉 |
…es (#32608) ## What `S3DownloadStreamWrapper::on_stream_cancelled` only set `signal_store.aborted = true` and relied on the HTTP thread to eventually deliver a final `has_more == false` callback (via `opaque_callback` → `callback`'s scopeguard) to free the heap-allocated wrapper and the `S3HttpDownloadStreamingTask`. On an idle socket — the server has sent at least one chunk and then gone quiet without closing, exactly the scenario `test/js/bun/s3/s3-stream-cancel-leak.test.ts` sets up — the HTTP thread is parked in its event loop and never re-checks the flag, so the terminal callback never arrives and both the wrapper and its `path: Box<[u8]>` leak. ASAN on Debian 13 x64 reports it as: ``` SUMMARY: AddressSanitizer: 39731 byte(s) leaked in 84 allocation(s). ``` with the path-clone allocation traced through `s3/client.rs:1265 → ReadableStream::from_blob_copy_ref → Blob::get_stream`. ## Fix Pair the abort flag with `bun_http::http_thread().schedule_shutdown((*task).http)` so the HTTP thread wakes up, observes the abort, closes the socket, and invokes the terminal callback — same pattern `FetchTasklet::abort_task` already uses. Taken under `(*task).mutex` because `update_state` writes `task.http` concurrently. ## Why this isn't in #32018 The vm PR doesn't touch any of `src/runtime/webcore/{s3,Blob,ReadableStream}.rs`; this leak reproduces on `main` independently.
What does this PR do?
Brings
node:vmto 98/101 (97%) of Node v26.3.0's vm test suite, up from 66/101 (~65%) on current main. Vendors 25 new upstream test files (plus 18 refreshed to verbatim v26.3.0), and fixes every runtime gap they surfaced. Every vendored vm test passes, including the two that previously hung and the one that crashed.Pass rate
The 3 upstream tests not vendored:
test-vm-module-modulerequests.js,test-vm-module-linkmodulerequests.js— require source-phase imports (import source), which the engine parser does not support.test-vm-dynamic-import-callback-missing-flag.js— asserts--experimental-vm-modulesflag gating; bun intentionally supports vm modules unflagged, so gating would break existing users.Runtime fixes
Module linking and the v26 API
SourceTextModule:linkRequests(),instantiate(),moduleRequests(withphase),hasTopLevelAwait(),hasAsyncGraph(); newERR_MODULE_LINK_MISMATCHcode.link()/instantiate()now follow Node's status protocol: linking stores resolutions and leaves the moduleunlinked;instantiate()links the record graph, flips status, and propagateslinkedto dependency wrappers (fixes shared-dependency evaluation order andinstantiate()crashing on dependency-free modules).instantiate()without a prior link throwsERR_VM_MODULE_LINK_FAILURE("module is not linked")like Node.moduleRequestsavailable immediately).Top-level await
evaluate()so the async machinery runs to completion — previously the body afterawaitnever resumed and the namespace stayed in TDZ.microtaskMode: 'afterEvaluate'timeoutnow throwsERR_SCRIPT_EXECUTION_TIMEOUTwhere it previously hung (or panicked on older builds).Evaluation semantics
evaluate()is no longerasync: synchronously-completed evaluations return already-settled promises (util.inspectshowsPromise { undefined }), sync throws map to rejected promises, and an errored module rejects with the same error instance on every re-evaluate.SyntheticModuleinstantiates at construction like Node (sosetExport()/evaluate()work immediately), uninitialized exports read asundefinedinstead of throwing TDZ, and re-entrantevaluate()from inside evaluation steps rejects withERR_VM_MODULE_STATUSinstead of recursing.vm.measureMemory()implemented (summary/detailed, per-context entries, experimental warning).Contextify interceptors
Object.defineProperty(this, …)inside a context forwards through the sandbox's[[DefineOwnProperty]], so Proxy sandbox traps fire exactly once.runInContext('window') === runInContext('this')).this[1]) routes through the interceptor; previously it bypassed the sandbox.Scripts
codeGeneration: { wasm: false }now also gates synchronousnew WebAssembly.Module()(throwsCompileError), not just the streaming entry points.script.sourceMapURLis populated before the script first runs.<file>:<line>, the offending source line, and a caret marker.compileFunctiondefaultsfilenameto the empty string in both call shapes like Node (it previously flipped between two labels), andlineOffset ≥ 1now lines up reported positions with the function body.Crashes and races
SigintWatcherrefcount was a plain integer mutated from concurrentworker_threads; lost increments tore the watcher down early and aborted the process (exit 134). Now guarded by a lock.bun -e '…require(file)…'silently exited 0 when the required file failed to load (transpile error or module-not-found). The CJS entry path used theJSC::evaluateoverload that swallows the exception; it now rethrows, so the error prints and the process exits 1.Error messages
ERR_INVALID_ARG_TYPErenders free-form phrases like Node (must be an Array of unique strings), andvalidateBufferproduces Node'smust be an instance of Buffer, TypedArray, or DataView.Test-side deviations
Per the vendoring conventions, upstream files are verbatim except engine-inherent differences, each gated as
typeof Bun === 'undefined' ? <node> : <bun>with a one-line comment: JSC attributes throws to different columns than V8 (affects frame columns and caret position), JSC's strict-mode/defineProperty message texts differ,columnOffsetand thelineOffset: 0wrapper line can't be compensated (the engine clamps zero/negative provider start positions), and the uncaught-error printer casing. All patched files still pass under real Node.How was this verified?
test-vm-*via the node-test config, 0 failures, 0 timeouts, across repeated full-suite runs.test/js/node/vm/(bun's own vm tests): 285 tests, 0 failures; one white-box inline snapshot updated to the new link/instantiate contract and one stack snapshot regenerated for the added error decoration.test/cli/run/run-eval.test.ts(33/33),test/js/node/module(64/64),test/js/bun/resolve(276 tests),node-crypto.test.js(150 tests) — all passing.-efix verified against both regressions it could affect: plain throws still report and exit 1; valid scripts unaffected.