Build Node renderer VM contexts with DONT_CONTEXTIFY when available - #4830
Build Node renderer VM contexts with DONT_CONTEXTIFY when available#4830alexeyr-ci2 wants to merge 2 commits into
Conversation
`vm.createContext(contextObject)` contextifies the sandbox, which installs V8
named/indexed property interceptors on the context's global object. Every global
lookup inside a server bundle (`React`, `process`, `Buffer`, ...) then traps
through those interceptors instead of hitting V8's ordinary global fast paths --
and SSR bundles are very global-heavy.
`vm.constants.DONT_CONTEXTIFY` (Node 20.18+ / 22.8+) creates a context with an
ordinary global object instead. It is passed *instead of* the sandbox object and
returns the new context's global, so the globals are assigned onto it afterwards.
That object is reference-equal to `globalThis` inside the context, so the
per-request `renderingRequest`/`sharedExecutionContext`/`runOnOtherBundle`
injection and cleanup in `runInVM` are unaffected.
Feature-detected rather than version-gated: `vm.constants` itself only exists
from Node 20.12/21.7 and this package supports Node >= 18.19, so
`'DONT_CONTEXTIFY' in vm.constants` would throw a TypeError on the floor version.
Note that upstream documents the win only qualitatively ("speed up the global
access if they don't need the interceptor behavior", nodejs/node#54394) and
publishes no benchmark numbers. This lands the mechanism; the magnitude for SSR
workloads still needs a local M1 A/B run.
Verified both flavors are observationally identical for everything this module
relies on -- injected globals, `global = this` aliasing, module-wrapped bundle
evaluation, stack-frame filenames, host reads of bundle-defined globals,
per-request injection/cleanup, and no leakage into the worker realm. The new test
covers both branches; the fallback is stubbed since CI's Node always takes the
vanilla path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughVM creation now uses ChangesVM context compatibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR changes Node renderer VM construction to use an ordinary DONT_CONTEXTIFY global when the runtime supports it.
Confidence Score: 4/5The PR should not merge until the DONT_CONTEXTIFY path preserves supported additional-context behavior for keys that collide with initialized global properties. The new Object.assign target contains non-writable intrinsic globals, so an arbitrary additional-context key accepted by the existing configuration API can now abort VM construction only on newer Node runtimes. Files Needing Attention: packages/react-on-rails-pro-node-renderer/src/worker/vm.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Build prepared context object] --> B{DONT_CONTEXTIFY available?}
B -->|No| C[Contextify prepared sandbox]
B -->|Yes| D[Create ordinary VM global]
D --> E[Assign prepared properties onto global]
C --> F[Evaluate bundle]
E --> F
Reviews (1): Last reviewed commit: "Build Node renderer VM contexts with DON..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.github.com/in/867647?s=60&v=4){kind=small}
| // branch can go once 20.18/22.8 are the floor. | ||
| const dontContextify: typeof vm.constants.DONT_CONTEXTIFY | undefined = vm.constants?.DONT_CONTEXTIFY; | ||
| const context = | ||
| dontContextify === undefined |
There was a problem hiding this comment.
When additionalContext contains an allowed key matching a non-writable intrinsic such as Infinity, NaN, or undefined, this Object.assign writes onto the already initialized VM global and throws, preventing the bundle from loading on DONT_CONTEXTIFY-capable Node versions even though the fallback path accepts the same configuration.
Knowledge Base Used: Pro Node Renderer Worker: Request Handling, VM Execution, and Cluster Lifecycle
![chatgpt-codex-connector[bot]](https://avatars.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13d759eea2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const context = | ||
| dontContextify === undefined | ||
| ? vm.createContext(contextObject) | ||
| : Object.assign(vm.createContext(dontContextify), contextObject); |
There was a problem hiding this comment.
On Node versions that take this DONT_CONTEXTIFY branch, a bundle or render request that runs Object.freeze(globalThis) can now freeze the actual context global; the old contextified sandbox throws on that freeze. runInVM injects per-request renderingRequest, sharedExecutionContext, and runOnOtherBundle onto this same object and clears them in finally by assignment, so once the global is frozen the cleanup throws/fails and those request-specific values remain visible to later requests that reuse the pooled VM.
Useful? React with 👍 / 👎.
size-limit report 📦
|
![claude[bot]](https://avatars.github.com/in/1236702?s=60&v=4){kind=small}
| beforeEach(() => { | ||
| // `vm.constants` is frozen, but `vm`'s own `constants` property is writable, so swapping | ||
| // the whole object forces the module down the older-Node fallback path. | ||
| if (!preferVanilla) { | ||
| jest.replaceProperty(vm, 'constants', undefined as unknown as typeof vm.constants); | ||
| } | ||
| resetVM(); | ||
| }); |
There was a problem hiding this comment.
Global mutation isn't restored — likely leaks into every test that runs after it in this file.
This beforeEach stubs the real Node vm.constants (a process-wide singleton) to undefined via jest.replaceProperty, but nothing ever restores it: there's no .restore() call, no afterEach, and no jest.restoreAllMocks(). The package's jest config (packages/react-on-rails-pro-node-renderer/package.json) sets clearMocks: true and resetMocks: true, but not restoreMocks: true. resetMocks/clearMocks only reset mock call state/implementations for jest.fn()/jest.spyOn() — they do not restore properties swapped in via jest.replaceProperty. Only restoreMocks (or an explicit jest.restoreAllMocks()/.restore()) does that.
Given describe.each runs its cases in declaration order, the false (fallback) case runs second and permanently sets vm.constants = undefined for the rest of this test file's execution — there are ~800 more lines of tests after this block (buildVM and runInVM, VM security..., spec/dummy node, VM Pool Management, etc.). All of them will now silently run against the fallback (contextified-sandbox) code path instead of the new DONT_CONTEXTIFY path, even on a Node version that supports it. That undercuts the PR's own testing claim that the flavors were verified "observationally identical" across the suite — after this test runs once, essentially nothing else in the file ever exercises the new branch again.
Suggested fix: capture the handle and restore it, e.g.
let constantsOverride: ReturnType<typeof jest.replaceProperty> | undefined;
beforeEach(() => {
if (!preferVanilla) {
constantsOverride = jest.replaceProperty(vm, 'constants', undefined as unknown as typeof vm.constants);
}
resetVM();
});
afterEach(() => {
constantsOverride?.restore();
constantsOverride = undefined;
});![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/react-on-rails-pro-node-renderer/tests/vm.test.ts`:
- Around line 133-140: Restore the property replacement created in the
beforeEach setup for vm.constants when preferVanilla is false, using the
returned replacement handle’s restore method or the test suite’s mock
restoration mechanism. Ensure subsequent tests see the original vm.constants
value and do not remain on the older-Node fallback path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 85ccd6f0-7d59-40c2-a861-6243878bc82f
📒 Files selected for processing (2)
packages/react-on-rails-pro-node-renderer/src/worker/vm.tspackages/react-on-rails-pro-node-renderer/tests/vm.test.ts
ReviewOverviewAdds a feature-detected branch in Code quality
Bug: test isolation (see inline comment)The added test in Correctness/security
Process notes (informational, not blockers)
Summary: the production change looks correct and low-risk. The main actionable issue is the test-isolation bug that silently defeats most of the new test file's coverage of the new code path — worth fixing before merge so CI is actually validating what the PR claims. |
The parameterized VM-flavor test stubs `vm.constants` to `undefined` to force the older-Node path, but `jest.replaceProperty` was never restored. This package's jest config sets `clearMocks`/`resetMocks` (and `resetModules`) but not `restoreMocks`, and none of those undo a property replacement. Because `describe.each` runs the fallback case second, `vm.constants` stayed `undefined` for the rest of the file -- every later test in vm.test.ts silently ran the contextified fallback regardless of the Node version, so the new DONT_CONTEXTIFY branch was exercised exactly once and never again. That undercut the coverage this PR claims. Verified with a temporary probe appended after the flavor block: it asserts `vm.constants` is still defined and failed with `Received: undefined` before this change, and passes after. Reported independently by claude-review and CodeRabbit on #4830. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4de1521cff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| constantsOverride = jest.replaceProperty( | ||
| vm, | ||
| 'constants', | ||
| undefined as unknown as typeof vm.constants, | ||
| ); |
There was a problem hiding this comment.
When this test is run on the supported Node 18 floor, vm.constants does not exist at all, while jest.replaceProperty only works for properties that already exist. That makes the fallback-flavor beforeEach throw before it can exercise the old-Node path, so pnpm --filter react-on-rails-pro-node-renderer test is broken for a runtime the package still advertises via engines.node >=18.19.0. Use Object.defineProperty/manual save-and-restore, or only call replaceProperty when the property exists.
Useful? React with 👍 / 👎.
|
Review: Build Node renderer VM contexts with DONT_CONTEXTIFY when available Overview Correctness
Tests Security Performance — this is the part that actually gates merge Minor/optional suggestions
Overall: the implementation looks correct and carefully reasoned, with test coverage that matches the actual risk. Hold merge for the local benchmark results per the PR's own stated gating criteria. |
| const dontContextify: typeof vm.constants.DONT_CONTEXTIFY | undefined = vm.constants?.DONT_CONTEXTIFY; | ||
| const context = | ||
| dontContextify === undefined | ||
| ? vm.createContext(contextObject) | ||
| : Object.assign(vm.createContext(dontContextify), contextObject); |
There was a problem hiding this comment.
This looks correct: vm.createContext(DONT_CONTEXTIFY) returns the new context's real globalThis rather than contextifying the sandbox object, so Object.assign onto it is the right way to inject the source-map hooks / supportModules globals / additionalContext. contextObject isn't referenced again after this block, so there's no aliasing hazard between the two branches despite one reusing the same object identity as the context and the other not.
One nit: the explicit typeof vm.constants.DONT_CONTEXTIFY | undefined annotation on dontContextify is load-bearing (per the PR description, without it TS infers number from the incorrect DefinitelyTyped declaration and narrows this ternary's fallback branch to never). Worth a short inline comment saying why the annotation is there — otherwise it reads like a removable type restatement, and a future "cleanup" that drops it reintroduces dead code that tsc won't flag as an error.
Also flagging (from the top-level review) that this is a hot-path change whose performance claim is explicitly unmeasured per the PR body — should hold on merging until the local M1 A/B benchmark run referenced in the PR description actually happens.
2 participants
Why
buildVMcreates each bundle's sandbox withvm.createContext(contextObject). Contextifying a sandbox installs V8 named/indexed property interceptors on the context's global object, so every global lookup inside a server bundle (React,process,Buffer, ...) traps through those interceptors instead of hitting V8's ordinary global fast paths. SSR bundles are very global-heavy, so this is on the hot path for every render.vm.constants.DONT_CONTEXTIFY(Node 20.18+ / 22.8+) creates a context with an ordinary global object instead.Caveat, stated up front: upstream documents this only qualitatively — "speed up the global access if they don't need the interceptor behavior" (nodejs/node#54394, repeated in the 20.18.0 / 22.8.0 release notes and the
vmdocs). That PR added no benchmark, and there is noDONT_CONTEXTIFYbenchmark anywhere innodejs/node. The mechanism is real and documented; the magnitude for our SSR workload is unmeasured. This PR lands the mechanism — the number still has to come from a local M1 A/B run.Implementation
packages/react-on-rails-pro-node-renderer/src/worker/vm.ts— one branch inbuildVM:Three things worth calling out for review:
vm.createContext([contextObject[, options]])wherecontextObjectisObject | vm.constants.DONT_CONTEXTIFY | undefined, and the call returns the new context's global rather than the object passed in — hence theObject.assignafterwards. That returned object is reference-equal toglobalThisinside the context, so the per-requestrenderingRequest/sharedExecutionContext/runOnOtherBundleinjection andfinallycleanup inrunInVMare unaffected.?.is load-bearing.vm.constantsitself only exists from Node 20.12/21.7, and this package declaresengines.node >= 18.19.0, so the more obvious'DONT_CONTEXTIFY' in vm.constantsthrowsTypeError: Cannot use 'in' operator ... in undefinedon the floor version.DONT_CONTEXTIFYis aSymbolat runtime, not anumber.@types/node(20.19.25 here) declares it asnumberand declaresvm.constantsas always-present — both wrong. Only presence is ever checked, and the annotation aliases the declared type (typeof vm.constants.DONT_CONTEXTIFY | undefined) rather than namingnumber, so it self-corrects if DefinitelyTyped is fixed. Without the annotation the value infers as plainnumber, the fallback branch narrows toneverand reads as dead code.Test plan
packages/react-on-rails-pro-node-renderer/tests/vm.test.ts— a parameterized test over both flavors. CI's Node always takes the vanilla branch, so the fallback case stubsvm.constantstoundefinedto exercise the older-Node path that would otherwise never run.Each flavor asserts which context type was actually built, then that behavior is unchanged: injected globals reach the bundle,
global = thisstill aliasesglobalThis, per-request state is visible during execution and cleared afterwards (so it cannot leak into a later request sharing a pooled context), and nothing escapes into the worker's own realm.Separately verified out-of-band that both flavors are observationally identical for every pattern this module relies on — including
Module.wrapbundle evaluation withrequire/__dirname, stack-frame filenames (source-map keying), and host-side reads of globals defined by the bundle. The only difference isglobalThis === context, which is the point of the change.Results (
packages/react-on-rails-pro-node-renderer):tests/vm.test.ts+tests/vmSourceMapSupport.test.ts: 91/91 pass, including the existingvm.createContextsynchronous-throw recovery test.@fastify/formbodyand@fastify/multipart). The only delta is the 2 new passing tests.type-check: clean forvm.ts(the one error is the same pre-existing@fastify/formbodyresolution failure insrc/worker.ts).eslint: clean.Changelog
No entry. There is no user-visible behavior change, and the performance claim is unquantified — I'd rather add a changelog entry once the A/B run produces a number than claim an improvement we haven't measured. Happy to add one if a maintainer prefers.
Labels
Labels: benchmark— classification only. PerAGENTS.md,benchmark*labels do not trigger hosted benchmark suites (hosted benchmark selection is disabled after the false-positive regressions in #4038–#4044).Benchmarks: local M1 A/B required— this is a Node renderer hot-path change and the whole premise is a perf claim, so it needsbenchmarks/run-local-benchmark-comparison.rbon a quiet machine before this is worth merging on performance grounds.Not requesting hosted CI yet; say the word and I'll comment
+ci-run-hosted.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests