Fix rolling-deploy VM context thrash - #4811
Conversation
|
+ci-status |
|
+ci-run-hosted |
CI StatusHead SHA: Only the required gate is active unless hosted CI is requested. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds bounded Node Renderer VM retention across rolling-deploy bundle generations, configurable drain timing, rollout-capacity Doctor diagnostics, deployment guidance, benchmark tooling, and regression coverage. The default ChangesNode Renderer rollout capacity
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant RailsRequest
participant NodeRenderer
participant VMPool
participant DrainScheduler
RailsRequest->>NodeRenderer: render with resolved bundle set
NodeRenderer->>VMPool: build or reuse VM context
VMPool->>VMPool: record bundle generation
VMPool->>DrainScheduler: schedule inactive-generation retirement
DrainScheduler->>VMPool: evict drained contexts after timeout
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
Hosted CI RequestedTriggered 9 workflow(s) for View progress in the Actions tab. |
github-actions
Bot
added
the
ready-for-hosted-ci
Greptile SummaryRaises and validates Node Renderer VM-pool capacity while adding bounded rollout-generation retention and improved deployment diagnostics.
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code defect identified. The VM lifecycle retains active execution resources across eviction, records only successful bundle generations, bounds both pooled contexts and generation metadata, and validates the new configuration before use; the Doctor analysis also fails closed when static evidence is ambiguous. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Request[Render request with bundle set] --> Build[Build or reuse VM contexts]
Build --> Success{All contexts available?}
Success -- No --> Error[Return existing build or cache error]
Success -- Yes --> Observe[Record successful rollout generation]
Observe --> Cap[Apply absolute VM-pool LRU cap]
Cap --> Active{Evicted context still active?}
Active -- Yes --> Defer[Retain source-map registration until release]
Active -- No --> Cleanup[Unregister source map]
Observe --> Timer[Schedule one unreferenced drain timer]
Timer --> Retire[Retire expired non-latest generations]
Retire --> Shared{Context referenced by another generation?}
Shared -- Yes --> Keep[Keep pooled context]
Shared -- No --> Cleanup
Reviews (1): Last reviewed commit: "Fix rolling-deploy VM rebuild thrash" | Re-trigger Greptile |
size-limit report 📦
|
![claude[bot]](https://avatars.github.com/in/1236702?s=60&v=4){kind=small}
Review: Fix rolling-deploy VM context thrashOverview: this raises the Node Renderer's per-worker VM pool default from 2 → 4, adds rollout-generation tracking with a timer-driven drain window so a draining bundle set stays reusable during rolling deploys, keeps the absolute LRU hard cap as a backstop, and adds a new Correctness — VM pool / generation logic (
|
![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: 5d8b023336
ℹ️ 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".
justin808
added
the
agent-claimed
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts (1)
240-248: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider parsing the drain timeout with
Numberfor parity withMAX_VM_POOL_SIZE.
parseFloatsilently accepts trailing garbage (VM_POOL_ROLLOUT_DRAIN_TIMEOUT=45s→45), whereasNumber(env.MAX_VM_POOL_SIZE)fails fast on the same input. Aligning both keeps the "invalid env fails startup" contract uniform.♻️ Proposed change
function defaultVmPoolRolloutDrainTimeout() { - return env.VM_POOL_ROLLOUT_DRAIN_TIMEOUT ? parseFloat(env.VM_POOL_ROLLOUT_DRAIN_TIMEOUT) : 60; + return env.VM_POOL_ROLLOUT_DRAIN_TIMEOUT ? Number(env.VM_POOL_ROLLOUT_DRAIN_TIMEOUT) : 60; }🤖 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 `@packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts` around lines 240 - 248, Update defaultVmPoolRolloutDrainTimeout to parse VM_POOL_ROLLOUT_DRAIN_TIMEOUT with Number instead of parseFloat, preserving the existing fallback to 60 when the environment variable is absent and ensuring malformed values fail consistently with defaultMaxVMPoolSize.packages/react-on-rails-pro-node-renderer/tests/configBuilder.test.ts (1)
179-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd ENV-level invalid coverage for
VM_POOL_ROLLOUT_DRAIN_TIMEOUT.
MAX_VM_POOL_SIZEgets anit.eachover invalid ENV strings, but the drain timeout is only exercised through explicit config values. An ENV case would pin down theparseFloatbehavior (e.g.'45s'currently yields45rather than failing).🧪 Suggested additional case
+ it.each(['abc', '0', '-1'])('rejects invalid VM_POOL_ROLLOUT_DRAIN_TIMEOUT=%p from ENV', (timeout) => { + process.env.VM_POOL_ROLLOUT_DRAIN_TIMEOUT = timeout; + const { buildConfig } = loadConfigBuilderWithMockedLogger(); + + expect(() => buildConfig()).toThrow('vmPoolRolloutDrainTimeout must be a positive number of seconds'); + });🤖 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 `@packages/react-on-rails-pro-node-renderer/tests/configBuilder.test.ts` around lines 179 - 216, Add an ENV-focused parameterized test alongside the existing MAX_VM_POOL_SIZE invalid-ENV coverage, using loadConfigBuilderWithMockedLogger and buildConfig to verify invalid VM_POOL_ROLLOUT_DRAIN_TIMEOUT strings—including a suffixed value such as “45s”—throw the expected positive-seconds validation error. Keep the explicit-configuration tests unchanged.react_on_rails/lib/react_on_rails/doctor.rb (1)
124-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving the launcher list from
NodeRendererProcfile::DEFAULT_COMMANDS.
check_launcher_procfiles(Line 1787) already notes launcher filenames must stay aligned withNodeRendererProcfile::DEFAULT_COMMANDS. This is a third hardcoded copy that can silently drift when a Procfile variant is added.🤖 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 `@react_on_rails/lib/react_on_rails/doctor.rb` around lines 124 - 130, Update NODE_RENDERER_LAUNCHER_PATHS to derive its launcher filenames from NodeRendererProcfile::DEFAULT_COMMANDS instead of maintaining a hardcoded list. Ensure check_launcher_procfiles and related launcher checks continue using the derived collection so newly added Procfile variants remain aligned automatically.react_on_rails/spec/lib/react_on_rails/doctor_spec.rb (3)
3707-3761: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese three examples test Node's semantics, not Doctor.
Open3.capture3("node", ...)makes the Ruby suite fail on any machine or CI lane without Node onPATH, and the assertions verify V8 behavior rather thancheck_node_renderer_rollout_capacity. Consider gating them (skip unless system("node --version", out: File::NULL)) or moving them to a JS-side test.🤖 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 `@react_on_rails/spec/lib/react_on_rails/doctor_spec.rb` around lines 3707 - 3761, Remove the three direct Node semantics examples from the Doctor spec, or gate each example before invoking Open3 so the examples are skipped when Node is unavailable on PATH. Keep the Doctor-focused tests independent of external Node availability, preferably moving these runtime-semantics checks to an appropriate JavaScript test suite.
3543-3546: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffNo named subject for the method under test.
Every example calls
doctor.send(:check_node_renderer_rollout_capacity)directly. A named subject (e.g.subject(:capacity_check) { doctor.send(:check_node_renderer_rollout_capacity) }) would satisfy the repo convention and remove the repetition across ~80 examples.As per coding guidelines: "Ruby code must ... use named RSpec subjects such as
subject(:method_result)".🤖 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 `@react_on_rails/spec/lib/react_on_rails/doctor_spec.rb` around lines 3543 - 3546, Define a named RSpec subject for the result of doctor.send(:check_node_renderer_rollout_capacity) near the affected examples, then replace direct repeated calls with that subject. Preserve the existing doctor setup and assertions while applying the subject consistently across the capacity-check examples.Source: Coding guidelines
3899-4427: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared fixture table and the JSON-doctor setup.
The array at Lines 4188-4405 is a near-subset of the one at Lines 3899-4174, and the
json_doctorsetup (described_class.new(format: :json, only: ...)+ threeallows + output capture) is repeated in at least seven examples. A single frozen fixture constant plus arun_json_capacity_checkhelper would keep these in sync as cases are added.🤖 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 `@react_on_rails/spec/lib/react_on_rails/doctor_spec.rb` around lines 3899 - 4427, The duplicated renderer source fixture arrays and JSON doctor setup should be consolidated. Extract the shared cases into one frozen fixture constant, reuse it in both example groups, and add a run_json_capacity_check helper encapsulating described_class.new, the three stubs, output capture, and JSON parsing; update the affected examples to call these shared symbols.benchmarks/bench-node-renderer-rollout.mjs (2)
87-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRequire the already-computed module paths.
configModulePath/vmModulePathare validated at Lines 89-94 but then discarded in favor of hardcoded relative specifiers, so the existence check and the actual load can drift. Using the computed absolute paths also resolves the twoimport/extensionsESLint errors.♻️ Proposed change
- const { buildConfig } = require('../packages/react-on-rails-pro-node-renderer/lib/shared/configBuilder'); - const { - buildExecutionContext, - getVMContext, - resetVM, - } = require('../packages/react-on-rails-pro-node-renderer/lib/worker/vm'); + const { buildConfig } = require(configModulePath); + const { buildExecutionContext, getVMContext, resetVM } = require(vmModulePath);🤖 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 `@benchmarks/bench-node-renderer-rollout.mjs` around lines 87 - 101, Update the module loading in the benchmark to require the already-computed configModulePath and vmModulePath values after validation, replacing the hardcoded relative specifiers while preserving the existing imported symbols and missing-file error handling.Source: Linters/SAST tools
102-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFixture path is assumed to exist.
fixturePathis not covered by the Lines 89-94 preflight; iftests/fixtures/bundle.jsmoves,copyFileSyncfails with a bare ENOENT instead of the actionable build hint. Adding it to the existingexistsSyncguard is a one-line change.🤖 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 `@benchmarks/bench-node-renderer-rollout.mjs` around lines 102 - 115, Update the existing preflight existsSync guard around the benchmark setup to also validate fixturePath, alongside the paths already checked in lines 89-94. Keep the existing actionable build hint and ensure the check runs before copyFileSync uses fixturePath.
🤖 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 `@CHANGELOG.md`:
- Around line 29-40: Update the CHANGELOG entry for Issue 4810 to append the
required PR 4811 link and author attribution, matching the exact “[PR NNNN](...)
by [author](...)” format used by neighboring entries while preserving the inline
**[Pro]** marker.
In `@docs/oss/building-features/node-renderer/js-configuration.md`:
- Line 60: Change the “Sizing and draining the VM pool” heading from h3 to h2 to
satisfy markdownlint while preserving its existing slug and inbound links.
In `@react_on_rails/lib/react_on_rails/doctor.rb`:
- Around line 3512-3517: Update add_node_renderer_capacity_guidance to accept
the selected node renderer config path and use it in the remediation message
instead of hardcoding renderer/node-renderer.js. Pass node_renderer_config_path
from the caller, preserving the existing capacity guidance and legacy
client/node-renderer.js behavior handled by node_renderer_procfile_command.
- Around line 3226-3240: Update node_renderer_rsc_assignment_evidence so it
skips node.first only when that element is the node-type symbol; recursively
visit every element for plain Ripper child arrays, including the first
assignment. Preserve conditional detection for typed nodes and ensure
conflicting top-level assignments are both collected before producing any
inferred verdict.
- Around line 3392-3402: Update node_renderer_constructor_call? to return false
before calling String#rindex when call.begin(0) is zero, preventing the
negative-position scan from searching backward from the end of content; preserve
the existing constructor detection for calls with a positive starting offset.
---
Nitpick comments:
In `@benchmarks/bench-node-renderer-rollout.mjs`:
- Around line 87-101: Update the module loading in the benchmark to require the
already-computed configModulePath and vmModulePath values after validation,
replacing the hardcoded relative specifiers while preserving the existing
imported symbols and missing-file error handling.
- Around line 102-115: Update the existing preflight existsSync guard around the
benchmark setup to also validate fixturePath, alongside the paths already
checked in lines 89-94. Keep the existing actionable build hint and ensure the
check runs before copyFileSync uses fixturePath.
In `@packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts`:
- Around line 240-248: Update defaultVmPoolRolloutDrainTimeout to parse
VM_POOL_ROLLOUT_DRAIN_TIMEOUT with Number instead of parseFloat, preserving the
existing fallback to 60 when the environment variable is absent and ensuring
malformed values fail consistently with defaultMaxVMPoolSize.
In `@packages/react-on-rails-pro-node-renderer/tests/configBuilder.test.ts`:
- Around line 179-216: Add an ENV-focused parameterized test alongside the
existing MAX_VM_POOL_SIZE invalid-ENV coverage, using
loadConfigBuilderWithMockedLogger and buildConfig to verify invalid
VM_POOL_ROLLOUT_DRAIN_TIMEOUT strings—including a suffixed value such as
“45s”—throw the expected positive-seconds validation error. Keep the
explicit-configuration tests unchanged.
In `@react_on_rails/lib/react_on_rails/doctor.rb`:
- Around line 124-130: Update NODE_RENDERER_LAUNCHER_PATHS to derive its
launcher filenames from NodeRendererProcfile::DEFAULT_COMMANDS instead of
maintaining a hardcoded list. Ensure check_launcher_procfiles and related
launcher checks continue using the derived collection so newly added Procfile
variants remain aligned automatically.
In `@react_on_rails/spec/lib/react_on_rails/doctor_spec.rb`:
- Around line 3707-3761: Remove the three direct Node semantics examples from
the Doctor spec, or gate each example before invoking Open3 so the examples are
skipped when Node is unavailable on PATH. Keep the Doctor-focused tests
independent of external Node availability, preferably moving these
runtime-semantics checks to an appropriate JavaScript test suite.
- Around line 3543-3546: Define a named RSpec subject for the result of
doctor.send(:check_node_renderer_rollout_capacity) near the affected examples,
then replace direct repeated calls with that subject. Preserve the existing
doctor setup and assertions while applying the subject consistently across the
capacity-check examples.
- Around line 3899-4427: The duplicated renderer source fixture arrays and JSON
doctor setup should be consolidated. Extract the shared cases into one frozen
fixture constant, reuse it in both example groups, and add a
run_json_capacity_check helper encapsulating described_class.new, the three
stubs, output capture, and JSON parsing; update the affected examples to call
these shared symbols.
🪄 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: 2d0d7869-86a9-4294-bbde-16890a8cfac5
📒 Files selected for processing (14)
CHANGELOG.mdbenchmarks/bench-node-renderer-rollout.mjsdocs/oss/api-reference/doctor.mddocs/oss/building-features/node-renderer/container-deployment.mddocs/oss/building-features/node-renderer/js-configuration.mddocs/pro/rolling-deploy-adapters.mdpackages/react-on-rails-pro-node-renderer/src/shared/configBuilder.tspackages/react-on-rails-pro-node-renderer/src/worker/vm.tspackages/react-on-rails-pro-node-renderer/src/worker/vmSourceMapSupport.tspackages/react-on-rails-pro-node-renderer/tests/configBuilder.test.tspackages/react-on-rails-pro-node-renderer/tests/vm.test.tsreact_on_rails/lib/react_on_rails/doctor.rbreact_on_rails/lib/react_on_rails/doctor_schema.rbreact_on_rails/spec/lib/react_on_rails/doctor_spec.rb
|
+ci-status |
|
+ci-run-hosted |
CI StatusHead SHA: Optimized hosted CI is enabled for this PR. |
Hosted CI RequestedTriggered 9 workflow(s) for View progress in the Actions tab. |
Review summaryOverview: this PR raises the default Node Renderer Two issues found, posted as inline comments:
Minor positive note: switching No security concerns beyond the above (the Doctor check only reads local files/env, no |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
+ci-status |
CI StatusHead SHA: Optimized hosted CI is enabled for this PR. |
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 `@react_on_rails/spec/lib/react_on_rails/doctor_spec.rb`:
- Around line 3721-3726: Update the test around
check_node_renderer_rollout_capacity to poll until child_pid_path exists and
contains a complete, nonzero PID before calling File.read(child_pid_path).to_i,
reusing the polling approach from the later example while preserving the
existing process-exit assertions.
🪄 Autofix
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: 02d635b3-a740-4d1c-acaf-43fb44df5854
📒 Files selected for processing (5)
CHANGELOG.mddocs/oss/building-features/node-renderer/container-deployment.mddocs/oss/building-features/node-renderer/js-configuration.mdreact_on_rails/lib/react_on_rails/doctor.rbreact_on_rails/spec/lib/react_on_rails/doctor_spec.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- docs/oss/building-features/node-renderer/js-configuration.md
- docs/oss/building-features/node-renderer/container-deployment.md
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/react-on-rails-pro-node-renderer/tests/vm.test.ts:980
warningSpy(created viajest.spyOn(...).mockImplementation(...)) is never restored. With the repo’s Jest config usingclearMocks: true(notrestoreMocks/resetMocks), that mocked implementation can leak into subsequent tests and alter behavior.
expect(getVMPoolDiagnostics().hardLimitEvictions).toBe(4);
expect(
warningSpy.mock.calls.filter(
([payload]) =>
typeof payload === 'object' &&
Review summaryThis is a large, well-documented PR (VM pool bump 2→4, rollout-generation tracking/draining, config validation, a new Doctor provenance check, and race-safe process cleanup). I focused on the VM pool/rollout logic, config validation, and the new Doctor JS-provenance parser. Two inline comments were posted on concrete bugs; a third target (a pre-existing, unrelated-to-this-diff function) couldn't take an inline comment since GitHub only allows commenting on lines touched by the diff, so it's noted below instead. Findings (inline)
Related observation (not inline-commentable — pre-existing code untouched by this diff)
def signal_rsc_dist_tag_process(signal, pid)
Process.kill(signal, -pid)
rescue Errno::ESRCH
Process.kill(signal, pid)
endThis is exactly the reused-PID race this PR's description says has been eliminated ("never falls back to a positive PID that could have been reused"), and the new sibling Reviewed and found solid
Design-level note (non-blocking)The new Doctor provenance check is a ~700-line hand-rolled JS analyzer (masking, delimiter-stack matching, ambiguity heuristics) implemented in Ruby string/regex operations rather than a real parser. It already shells out to |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Addressed the complete review wave scanned since 2026-07-28T19:13:13Z at exact head Mattered
Optional
Skipped
No deferred tracking issue was created. The exact-head configured reviewers and hosted CI are running again; any new feedback after this comment belongs to the next review scan. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (2)
react_on_rails/lib/react_on_rails/doctor.rb:3759
- Launcher MAX_VM_POOL_SIZE assignments are validated with /\A[1-9]\d*\z/, which rejects values that the renderer accepts (e.g., leading zeros). This can prevent Doctor from recognizing an otherwise-static proven assignment and push the evidence path into the ambiguous/unverified bucket.
return { state: :unverified, reason: "dynamic_or_ambiguous_launcher_vm_pool_assignment" } unless
assignments.length == assignment_matches.length &&
assignments.all? { |assignment| assignment.match?(/\A[1-9]\d*\z/) } &&
assignments.uniq.one?
react_on_rails/lib/react_on_rails/doctor.rb:3733
- Doctor treats MAX_VM_POOL_SIZE as invalid unless it matches /\A[1-9]\d*\z/, but the Node renderer accepts other positive-integer string forms (e.g., leading zeros or surrounding whitespace) because configBuilder coerces with Number() then checks integer-ness. This can misreport a valid setting as "invalid_doctor_process_environment_value" and unnecessarily degrade the rollout-capacity check to unverified.
This issue also appears on line 3756 of the same file.
env_value = ENV.fetch("MAX_VM_POOL_SIZE", nil)
if env_value
unless env_value.match?(/\A[1-9]\d*\z/)
return { state: :unverified, reason: "invalid_doctor_process_environment_value" }
end
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Review summaryFocused on the core VM-pool rollout logic (
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts:345
- envValuesUsed() uses truthiness to decide whether rollout settings came from ENV, which can misreport the sanitized metadata when a user explicitly provides a (falsy) value (e.g., via config file templating) and an ENV value is also present. Using an
in userConfigcheck is consistent with other options in this object and avoids false attribution to ENV.
MAX_VM_POOL_SIZE: !userConfig.maxVMPoolSize && env.MAX_VM_POOL_SIZE,
VM_POOL_ROLLOUT_DRAIN_TIMEOUT: !userConfig.vmPoolRolloutDrainTimeout && env.VM_POOL_ROLLOUT_DRAIN_TIMEOUT,
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Review disposition — exact candidate
|
Closes #4810.
Why
During a rolling deploy, one Node Renderer can receive alternating requests from the old and new Rails revisions.
An RSC-enabled revision needs both server and RSC VM contexts, so the old/new overlap needs four contexts per worker.
The prior default of two repeatedly evicted and rebuilt those contexts. Disk-cache seeding avoided uploads but did not
prevent VM rebuild churn.
The same topology also made deployment diagnosis easy to overstate: Rails can only prove capacity when it can
statically observe the renderer configuration, and a hard VM-pool cap does not bound execution contexts or source
maps still held by concurrent in-flight requests.
What changed
MAX_VM_POOL_SIZEon every config build.inactive unshared contexts after a configurable 60-second timeout using one unreferenced timer.
eviction, and emit rate-limited pressure plus cumulative build/eviction diagnostics.
vmPoolRolloutDrainTimeout/VM_POOL_ROLLOUT_DRAIN_TIMEOUT.node_renderer_rollout_capacityDoctor check. Successful Rails boot is required before live Proconfiguration can prove RSC enablement; a failed boot is reported as unverified and conservatively enabled.
Renderer capacity is observed only from unambiguous CommonJS provenance and directly scoped launcher/config
literals. Rebound or destructured
require, direct eval, wrapper arguments, hoistedfunction require, aliases,mutation, spreads, computed properties, conditional or unreachable calls, and all other ambiguous cases fail
closed as unverified.
ECHILDas a cleanup race, and never falls back to a positive PID that could have been reused./readylimitation, pre-seeding versus VM compilation,fleet capacity, and pooled-versus-transient memory boundary.
RSS, heap, and caveat output.
Rollout evidence
On an M1 arm64 (Node 24.8.0, Ruby 3.4.6, Rails 8.1.3), two cloned flagship Rails revisions with distinct server and
RSC bundle hashes shared one candidate renderer. Each fresh-process 40-second run offered 3 requests/second at a fixed
concurrent rate and cycled old server, old RSC, new server, and new RSC paths. The balanced order used three cap-2 and
three cap-4 runs with a preseeded disk cache and cold VM contexts.
Short debug-counter runs (32 successful requests each) recorded 32 builds / 30 evictions / max 2 retained contexts at
cap 2, versus 4 builds / 0 evictions / max 4 retained contexts at cap 4. The seed gate built exactly four contexts and
rendered all old/new server/RSC paths.
Cap 4 reduced median Rails p95 by 58.4% and client p95 by 46.5% in this workload. This is workload-specific evidence,
not a universal production-memory claim. In-flight requests can temporarily retain evicted execution contexts and
source maps outside the pool, so deployments still need measured concurrency headroom.
The M1 run executed renderer build
977168a6. Its VM runtime-pool files are byte-identical at exact candidate5ec6f3220ab64833a4c404ca176cdae97dd61956(vm.tsbloba19c397f45a8846514a2a7919cf0ec065ef84f37;vmSourceMapSupport.tsblob5f7667774b6be45ca79132139ee08367f055bf36). Later exact-head amendments affectDoctor provenance analysis, process cleanup, specs, and matching diagnostics wording, not the VM runtime pool. The M1
run did not capture RSS or heap evidence; no real-workload memory measurement is claimed.
Five exact-head cap-2/cap-4 benchmark pairs on the synthetic fixture had zero timeouts. Candidate p95 median was
0.408625 ms versus 3.737083 ms at baseline, with 4 builds / 0 rebuilds / 0 evictions / 4 retained contexts versus
16 builds / 12 rebuilds / 14 evictions / 2 retained contexts. This supports repeatability; it does not replace the
cloned-Rails evidence or establish a production-memory ceiling.
Verification
Exact candidate:
5ec6f3220ab64833a4c404ca176cdae97dd61956publication before cleanup assertions, and rollout test spy restoration all passed
closed through the existing startup error path
reaping is time-bounded through a detached waiter, and macOS
EPERMprobes distinguish a live leader from acompeting-waiter group-only state
bin/ci-localpassed setup, builds, lint, formatting, type-checking, JS/RSC suites, renderer suites,and generator execution before the gem-only RBS runtime wrapper failed across unrelated unchanged surfaces.
Untouched
origin/mainreproduces the identical representative failure list (19 failures in the same 68-examplepacker_utils+render_optionsslice) under Ruby 4.0.5, so this is recorded as a pre-existing baseline/toolchainlimitation rather than candidate regression.
review-fix candidate; their prior-head results are not used as current-head merge evidence.
Companion demo
The flagship demo documents and configures the four-context old/new RSC rollout headroom:
shakacode/react-on-rails-demo-flagship#39