Fix node renderer graceful shutdown restarts - #4400
Conversation
|
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:
WalkthroughThe PR updates Node renderer graceful shutdown and scheduled restart handling, including shutdown acknowledgement tracking, safer worker restart flow, regression tests, and a changelog entry. ChangesNode renderer shutdown and restart hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 SummaryThis PR updates node-renderer worker shutdown and restart behavior. The main changes are:
Confidence Score: 4/5The shutdown ACK path can leave a stuck worker alive past the early kill window.
packages/react-on-rails-pro-node-renderer/src/master.ts Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant M as Master
participant W as Worker
participant S as Supervisor
M->>W: SHUTDOWN_WORKER_MESSAGE
W-->>M: ACK before drain completes
Note over W: Active request may still be stuck
M->>M: 2s force-kill timer
M--xW: Skipped because worker ACKed
S--xM: Supervisor may kill master before 11s
Note over W: Worker can remain alive without the master
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant M as Master
participant W as Worker
participant S as Supervisor
M->>W: SHUTDOWN_WORKER_MESSAGE
W-->>M: ACK before drain completes
Note over W: Active request may still be stuck
M->>M: 2s force-kill timer
M--xW: Skipped because worker ACKed
S--xM: Supervisor may kill master before 11s
Note over W: Worker can remain alive without the master
Reviews (1): Last reviewed commit: "Document node renderer shutdown fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.github.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
|
+ci-status |
justin808
added
the
ready-for-hosted-ci
![claude[bot]](https://avatars.github.com/in/1236702?s=60&v=4){kind=small}
Review summaryReviewed the shutdown/restart logic changes across
|
size-limit report 📦
|
![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: 9d5ea8bf10
ℹ️ 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".
CI StatusHead SHA: Optimized hosted CI is enabled for this PR. |
…derer-shutdown-restart * origin/main: Add mechanical parity guards for Ruby↔TS protocol constants (#4412) (#4427) Move Node tsconfigs from @tsconfig/node14 to @tsconfig/node18 (#4410) (#4429) [Pro] Remove unused addressable and rainbow runtime deps from gemspec (#4416) (#4422) Delete finished #3313 Prism Gemfile-rewriter spike (#4421) Extract generator scan/tracking helpers (#4405) (#4430) Extract install_dependency_group helper in JsDependencyManager (#4403) (#4424) Remove obsolete Ruby<2.6 YAML-aliases capability shim (#4417) (#4428) Remove inert config.server_render_method option (#4415) (#4423) Prune stale knip ignores and enforce binaries in CI (#4408) (#4425) Extract shared redux_store kwargs validator (#4402) (#4420) Avoid caching async props prerender streams (#4376) Release incremental render context on setup failure (#4383) Optimize response type emitter snapshots (#4397) Skip generated stylesheet metadata for OSS renders (#4395) Avoid mutating render option inputs (#4396) Changelog: document PR 4282 registry cleanup (#4399) # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c406866c3
ℹ️ 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".
Review summaryReviewed the shutdown/restart changes in What looks solid:
One confirmed, still-open risk (left as an inline comment on No security concerns (internal process/IPC lifecycle code, no user-controlled input reaches it) and no performance concerns beyond the shutdown-path timing discussed above. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f7185ea30
ℹ️ 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".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-on-rails-pro-node-renderer/tests/restartWorkers.test.ts (1)
48-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for the empty-worker-set guard.
The four tests here cover the seconds-unit timeout, send-throw, send-callback stale/IPC error, and error-before-exit scenarios, but none exercise the
workerIds.length === 0early-return path inrestartWorkers.ts(if (workerIds.length === 0) { log.warn('No workers to restart'); return; }). This is one of the explicit hardening scenarios called out for issue#4368("empty worker sets... do not crash the master process or break the restart loop").✅ Suggested additional test
+ it('resolves without error when there are no workers to restart', async () => { + const restartWorkers = loadRestartWorkers({}); + await expect(restartWorkers(0, 30)).resolves.toBeUndefined(); + });🤖 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/restartWorkers.test.ts` around lines 48 - 162, Add a test for the empty-worker-set guard in restartWorkers by exercising the early return when no worker IDs are available. In restartWorkers and loadRestartWorkers, verify that calling the restart flow with an empty worker map does not throw, logs the “No workers to restart” warning, and leaves the restart loop untouched. Use the existing restartWorkers test suite and the workerIds.length === 0 branch as the target behavior.
🤖 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.
Nitpick comments:
In `@packages/react-on-rails-pro-node-renderer/tests/restartWorkers.test.ts`:
- Around line 48-162: Add a test for the empty-worker-set guard in
restartWorkers by exercising the early return when no worker IDs are available.
In restartWorkers and loadRestartWorkers, verify that calling the restart flow
with an empty worker map does not throw, logs the “No workers to restart”
warning, and leaves the restart loop untouched. Use the existing restartWorkers
test suite and the workerIds.length === 0 branch as the target behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c0aaa11-cc9a-4bd0-8bcb-9d3c82bf7e26
📒 Files selected for processing (4)
CHANGELOG.mdpackages/react-on-rails-pro-node-renderer/src/master.tspackages/react-on-rails-pro-node-renderer/src/master/restartWorkers.tspackages/react-on-rails-pro-node-renderer/tests/restartWorkers.test.ts
💤 Files with no reviewable changes (1)
- packages/react-on-rails-pro-node-renderer/src/master/restartWorkers.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
…derer-shutdown-restart * origin/main: Document RSC public-page validation and sidecar patterns (#4387)
Review summaryThis PR hardens the node-renderer master/worker shutdown and scheduled-restart machinery: it makes active-request counting exact-once across I traced through the
No other correctness, security, or performance issues stood out — the ACK-based skip-early-kill design and the |
…derer-shutdown-restart * origin/main: Preserve streaming LoadError during dependency failures (#4388) Wire eslint-rules RuleTester suite into a runner, CI, and knip (#4409) (#4433) Handle fire-and-forget RSCRoute retry failures (#4378) Fix visible hydration cleanup for detached roots (#4374) Avoid full locale default obsolete scans (#4398)
|
Review: Node renderer graceful shutdown restarts (PR 4400) Went through Verified correct
Minor / non-blocking
Testing QA evidence in the PR description (targeted Jest suites, type-check, build, lint, format) matches what I'd expect to run for this change; I wasn't able to execute the suite myself in this sandbox, but the new tests ( No blocking findings from this pass. |
…derer-shutdown-restart * origin/main: Remove pre-monorepo node-renderer devDep baggage; consolidate test multipart builders (#4435)
Review summaryThis PR has already been through several rounds of review (5 prior
One still-open finding (left inline on No security or performance concerns — this is internal process/IPC lifecycle code with no user-controlled input, and the changes only affect shutdown/restart timing paths. |
Review SummaryThis PR hardens the node-renderer's graceful shutdown and scheduled-restart paths: (1) a I read through
One pre-existing (not introduced by this PR) edge case worth noting for awareness rather than as a blocking issue: in Overall the changes are well-scoped, the new ACK/dedup mechanisms compose correctly with the existing shutdown timers, and the new Jest coverage ( |
…370' into codex/batch-e-loadable-stats-retry-4371 * origin/codex/batch-e-rsc-parser-flush-4370: Add cached static RSC helper and diagnostics (#4386) Fix Pro tag revalidation retry after delete failures (#4375) Fix node renderer graceful shutdown restarts (#4400) Improve release-finish dry-run fetch handling (#4441) Flush RSC payloads before incomplete HTML tails (#4379) Handle sync RSC route failures as fetch errors (#4393) Delete never-wired RenderRequest/JsCodeBuilder/RenderingStrategy layer (#4414) (#4437) Delegate deprecated base/ shims to capabilities/ instead of cloning (#4413) (#4436) Remove pre-monorepo node-renderer devDep baggage; consolidate test multipart builders (#4435) Preserve streaming LoadError during dependency failures (#4388) Wire eslint-rules RuleTester suite into a runner, CI, and knip (#4409) (#4433) Handle fire-and-forget RSCRoute retry failures (#4378)
…nsport * origin/main: Unify Pro component cache fetch behavior (#4384) Replace chalk with picocolors in create-react-on-rails-app (#4411) (#4444) Fix Pro RSC stylesheet stats retry after read failures (#4401) [Pro] Reduce tag-index cache work during streaming (#4443) Warn on truncated Pro RSC parser streams (#4392) Report response-start send rejections (#4389) Add cached static RSC helper and diagnostics (#4386) Fix Pro tag revalidation retry after delete failures (#4375) Fix node renderer graceful shutdown restarts (#4400) Improve release-finish dry-run fetch handling (#4441) Flush RSC payloads before incomplete HTML tails (#4379)
…w-boundary * origin/main: (26 commits) [Pro] Extract async props settled chunk writer (#4448) Fix incorrect defer_generated_component_packs = false migration guidance (#4451) Fix Pro renderer transport memory and reuse (#4394) Fix Pro RSC loadable stats retry visibility (#4447) Unify Pro component cache fetch behavior (#4384) Replace chalk with picocolors in create-react-on-rails-app (#4411) (#4444) Fix Pro RSC stylesheet stats retry after read failures (#4401) [Pro] Reduce tag-index cache work during streaming (#4443) Warn on truncated Pro RSC parser streams (#4392) Report response-start send rejections (#4389) Add cached static RSC helper and diagnostics (#4386) Fix Pro tag revalidation retry after delete failures (#4375) Fix node renderer graceful shutdown restarts (#4400) Improve release-finish dry-run fetch handling (#4441) Flush RSC payloads before incomplete HTML tails (#4379) Handle sync RSC route failures as fetch errors (#4393) Delete never-wired RenderRequest/JsCodeBuilder/RenderingStrategy layer (#4414) (#4437) Delegate deprecated base/ shims to capabilities/ instead of cloning (#4413) (#4436) Remove pre-monorepo node-renderer devDep baggage; consolidate test multipart builders (#4435) Preserve streaming LoadError during dependency failures (#4388) ... # Conflicts: # CHANGELOG.md # packages/react-on-rails-pro/src/RSCProvider.tsx # packages/react-on-rails-pro/src/RSCRoute.tsx # packages/react-on-rails-pro/tests/boundedCacheProvider.client.test.tsx # packages/react-on-rails-pro/tests/getReactServerComponent.client.test.ts
1 participant
Summary
Fixes #4365.
Fixes #4366.
Fixes #4367.
Fixes #4368.
QA Evidence
pnpm --dir packages/react-on-rails-pro-node-renderer exec jest --runTestsByPath tests/restartWorkers.test.ts tests/handleGracefulShutdown.test.ts tests/masterStartupFailure.test.ts --runInBandpassed: 3 suites, 33 testspnpm --dir packages/react-on-rails-pro-node-renderer run type-checkpassedpnpm --dir packages/react-on-rails-pro-node-renderer run buildpassedpnpm run lintpassedpnpm start format.listDifferentpassedgit diff --checkpassedKnown local limitations:
bundle exec rubocopcurrently fails on pre-existing unrelated files underreact_on_rails/spike/3313_prism_gemfile_rewriter/; this PR does not touch Ruby files, and pre-push branch lint reported no Ruby files to lint.pnpm --filter react-on-rails-pro-node-renderer run test -- --runInBandcurrently fails 4 streaming/RSC suites becausereact_on_rails_pro/spec/dummy/ssr-generated/server-bundle.jsandrsc-bundle.jsare absent in this checkout. The focused shutdown/restart suites above pass on current head.Workflow config: known in current checkout. Base branch is
main; strict merge ledger command isscript/pr-merge-ledger <PR> --strict; hosted CI trigger is+ci-*comments orready-for-hosted-ci/force-full-hosted-cilabels.Batch QA Lane
qa-batch-iPASS evidence on headac42546c5284f0bfc532255758822e4ee04fd76d: serial shared-file sequencing verified, focused node-renderer shutdown/restart Jest suite passed, and type-check passed.9d5ea8bf10d7829fe1b1ec0ca23a5c92c8c0a9e1: Claude threads triaged; three low-risk fixes were applied and one [Node Renderer] Master SIGKILLs all workers 2s into graceful shutdown — draining renders truncated, 10s shutdown-hook budget unreachable #4365 ACK/early-kill thread was resolved as an intentional drain-vs-hard-deadline decision.ready-for-hosted-cion head9d5ea8bf10d7829fe1b1ec0ca23a5c92c8c0a9e1.Summary by CodeRabbit