refactor(cli): replace deprecated vm2 with native node:vm runner - #2310
mbiernacik wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Thanks for taking this on — removing vm2 is clearly the right call, and I think VmRunner is a good shape for the replacement.
Most of my inline comments are suggestions rather than objections. The one I'd genuinely flag is the module cache entry being retained when a module throws (cli/vm/vm_runner.ts:148), since that can turn a load failure into a silently incomplete compiledGraph rather than a clear error.
A few things that didn't fit on a specific line:
- Security framing. The description says this "eliminates security vulnerabilities" and describes a "hermetic" runner with a "secure module resolver".
node:vmdoesn't really provide isolation — Node's docs are fairly explicit that it isn't a security mechanism, and vm2 existed because of that gap. If the CLI only ever compiles the invoking user's own repo on their own machine, then vm2's CVEs probably weren't reachable here either, so the security angle may not be doing much work in either direction. "Removes an unmaintained dependency and is meaningfully faster" seems like ample justification on its own. Would you consider rewording that part? - Feature list. I couldn't find implementations for three things the description mentions: execution timeouts,
node:vm.Script(onlyvm.compileFunctionis used), and script caching (moduleCacheholds module exports rather than compiled scripts). Possibly planned and dropped, or I'm looking in the wrong place — either way it'd be good to bring the description back in line. - Benchmarks. Could the benchmark script land under
cli/vm/? The numbers are a nice result and it'd be good to be able to re-run them. I did notice the vm2 baselines all end in.00while thenode:vmfigures carry two decimals, which made me wonder whether the "before" column was measured or estimated. - Leftover vm2 references.
core/session.ts:72and:634,core/main_session_test.ts:164andcli/index_compile_test.ts:102still explain live workarounds in terms of "vm2's sandbox stack stripping". Sincecompile.ts:65-68now says the opposite holds, is the__df_enter/__dataform_current_filefile-stack machinery still needed for@dataform/core >= 3.0.57?
And lets make sure we are properly rebased on top of main branch now :)
| builtinModules: ["path"], | ||
| resolve: (moduleName, parentDirName) => | ||
| path.join(parentDirName, path.relative(parentDirName, compileConfig.projectDir), moduleName), | ||
| sourceExtensions: ["js", "sql", "sqlx", "yaml", "yml", "ipynb", "md"], |
There was a problem hiding this comment.
ipynb and md look unrelated to the vm2 removal
Adding these two isn't mentioned in the description and doesn't have a test, and it has a couple of knock-on effects: notebook and markdown files now go through the SQLX compiler, and both extensions join allExtensions, so they participate in extension-less resolution and directory-index lookup.
Is this by accident?
There was a problem hiding this comment.
Required for Dataform notebook actions (actions.yaml), which load .ipynb/.md via nativeRequire().asJson. Without registering these extensions, VmRunner tries to evaluate them as JS and throws a syntax error on "cells": [...]. Added a test.
|
After my review I would also like @Ikolina or someone who is longer in Dataform team to also take a look here. |
39bfbb4 to
95db3da
Compare
95db3da to
3fc433c
Compare
|
Thanks for addressing all the previous feedback so thoroughly — moving Just one small thing I noticed on |
Summary
This PR replaces the deprecated and unmaintained
vm2library with nativenode:vm(VmRunner) across Dataform CLI, Core testing, and build targets.Migrating to native
node:vmremoves an unmaintained third-party dependency, addresses deprecation warnings and known CVEs associated withvm2, and delivers substantial performance gains across compilation workflows.Key Changes Across the Codebase
Native
VmRunnerImplementation (common/vm/vm_runner.ts&common/vm/BUILD):VmRunnerunder//common/vm(preventing circular layering with//cliand//testing).node:vm.createContextandnode:vm.compileFunctionwith scoped CommonJS require simulation.projectDirboundary containment (isPathContained) with an opt-inallowedExternalPathsconfiguration.moduleCacheon throw so that errors re-throw consistently on subsequentrequire()calls.envandenvAllowlistoptions.Uint8ArrayandArrayBufferin sandbox globals to ensure cross-realminstanceof Uint8Arraychecks pass inprotobufjs/ JIT compilation.resolveCacheand captures suppressed resolution errors aserr.causeonMODULE_NOT_FOUND.common/vm/vm_runner_test.tsvia//common/vm:tests).CLI Compilation & JIT Worker Migration (
cli/vm/compile.ts,cli/vm/jit_worker.ts):cli/vm/compile.tsfromNodeVMtoVmRunner..ipynb,.md) viasourceExtensions.cli/vm/jit_worker.tstoVmRunner.Core Test Harness & Property Graphs (
testing/run_core.ts,testing/BUILD,core/main_property_graphs_test.ts):testing/run_core.ts(runMainInVm) to execute tests throughVmRunner.core/main_property_graphs_test.tsby removing the artificialvm2empty CallSite stack workaround and unifying error assertions viaasPlainGraph.Packaging & Dependency Cleanup (
package.json,yarn.lock,packages/@dataform/cli/BUILD,packages/rollup.config.js):vm2frompackage.jsonandyarn.lock.vm2fromexternalsinpackages/@dataform/cli/BUILD.vmandmoduletoknownNodeBuiltinsinpackages/rollup.config.js.Performance Benchmark (
common/vm/vm_runner_benchmark.ts):bazel run //common/vm:benchmark(~10,300 module requires/sec).