Fix descriptor closed error caused by workers restart while request is in progress
#1970
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds graceful worker restart support to the Node Renderer (config, master scheduling, sequential restarts, worker-side graceful shutdown), updates config/ docs, and enables/adjusts several integration system tests and assertions. Changes
Sequence Diagram(s)sequenceDiagram
participant Master as Master
participant Worker as Worker
participant App as Fastify App
Note over Master: Recursive scheduler (setTimeout)
Master->>Master: schedule restartWorkers(...)
Master->>Worker: Send SHUTDOWN_WORKER_MESSAGE
Worker->>Worker: isShuttingDown = true
Worker->>App: track activeRequestsCount (on each request/response)
alt activeRequestsCount > 0
App->>Worker: requests finish (decrement)
Worker->>Worker: when 0 -> destroy() (forced if timed out)
else no in-flight requests
Worker->>Worker: disconnect() (immediate)
end
Master->>Master: after all workers processed -> reschedule next restart
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
react_on_rails_pro/lib/react_on_rails_pro/request.rb (2)
10-15: Remove unused connection management.The
@connection_without_retriesis closed and recreated here, but since line 102 now always usesconnection, this secondary connection pool is never used. This dead code should be removed to eliminate unnecessary resource allocation.Apply this diff to remove the unused connection:
def reset_connection @connection&.close - @connection_without_retries&.close @connection = create_connection - @connection_without_retries = create_connection(enable_retries: false) end
87-97: Update or remove outdated architecture comment and unused method.The comment describes maintaining two separate HTTP connection pools to handle streaming vs non-streaming requests, but line 102 now uses only
connectionfor all requests. Theconnection_without_retriesmethod is now dead code and should be removed along with the misleading comment.Apply this diff:
-# NOTE: We maintain two separate HTTP connection pools to handle streaming vs non-streaming requests. -# This doubles the memory footprint (e.g., if renderer_http_pool_size is 10, we use 20 total connections). -# This tradeoff is acceptable to prevent body duplication in streaming responses. - def connection @connection ||= create_connection end - -def connection_without_retries - @connection_without_retries ||= create_connection(enable_retries: false) -end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react_on_rails_pro/lib/react_on_rails_pro/request.rb(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
- GitHub Check: claude-review
🔇 Additional comments (1)
react_on_rails_pro/lib/react_on_rails_pro/request.rb (1)
244-245: Streaming requests now enable connection retries, but comment references outdated concern.The comment warns about body duplication from retries in streaming requests (citing issue #1895), but the current implementation at line 102 enables retries for all requests, including streaming. The comment should be updated or reconciled with this behavior change.
Code ReviewSummaryThis PR re-enables previously skipped RSpec system tests for streaming components and React Router while making minor RSpec style improvements. The changes are investigative in nature to understand the "descriptor closed" error. Positive Changes ✅1. RSpec Best Practices
2. Code Cleanup
3. Type Specification
Concerns & Issues
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb (3)
14-14: LGTM: Improved assertion style.The change from
have_no_contenttonot_to have_contentis more idiomatic RSpec and improves consistency with standard matcher patterns.
242-242: Type annotation potentially redundant but acceptable.RSpec typically infers
type: :systemfrom thespec/system/directory path, making this annotation redundant. However, explicit type declarations can improve clarity and are acceptable practice.
410-413: LGTM: Consistent assertion style improvements.These changes align with the refactor on line 14, making the assertions more idiomatic by using
not_to have_textinstead ofhave_no_text.Also applies to: 420-421
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb(5 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
- GitHub Check: claude-review
🔇 Additional comments (2)
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb (2)
113-113: LGTM: Rubocop comment cleanup.Removing the inline rubocop disable comments suggests these are no longer needed. Ensure that your linter passes without warnings on these lines.
Also applies to: 177-177, 184-184, 189-189, 247-247
425-428: The premise of this review comment is incorrect.The tests at lines 425-428 and 430-432 are not being re-enabled. They are already active in the codebase with no
skipkeyword. The PR's changes to this file are purely stylistic refactoring (convertinghave_no_*tonot_to have_*and removing rubocop disable comments), not behavioral changes that re-enable previously skipped tests. Additionally, no "descriptor closed" error appears in the codebase as a concrete error string to verify against.Likely an incorrect or invalid review comment.
Updated Code ReviewThis PR refactors system test assertions and re-enables two previously skipped test suites. The changes improve RSpec style but raise test stability questions. Strengths
Critical Concerns
RecommendationsBefore Merging:
Code Quality
Verdict: CHANGES REQUESTEDCode changes are good, but PR needs better documentation about the investigation findings and justification for re-enabling previously flaky tests. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
react_on_rails_pro/spec/dummy/client/node-renderer.js (1)
45-45: Clarify the rationale for the fixed 100-minute interval and address inconsistency.The hardcoded value of 100 minutes creates an inconsistency with line 48, which still uses environment-based logic (
env.CI ? 0.01 : 1). Additionally, 100 minutes seems quite long for a test/dummy application.Since this PR is investigating a "descriptor closed" error, is this fixed value intended to create more deterministic test conditions? Please clarify:
- Why was 100 minutes specifically chosen?
- Is the inconsistency with line 48's dynamic approach intentional?
- How does this change help investigate the descriptor closed error?
Consider adding a comment explaining the rationale for this specific value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react_on_rails_pro/spec/dummy/client/node-renderer.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: claude-review
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
PR Review: Investigate descriptor closed errorSummaryThis PR appears to be investigating flaky test failures related to a descriptor closed error by re-enabling previously skipped system tests, improving test assertions, and updating regex patterns for bundle matching. Code Quality - Positive Changes
CRITICAL ISSUE - Hardcoded Worker Restart IntervalChanged in react_on_rails_pro/spec/dummy/client/node-renderer.js:45 from dynamic config to hardcoded 100 minutes. Problems:
Recommendation: Revert this change before merging. The original logic was better with CI at 2 minutes and development configurable. Other Concerns
RecommendationsMUST FIX BEFORE MERGE:
SHOULD ADDRESS:
VerdictNot Ready to Merge - The hardcoded worker restart interval appears to be debugging code. The RSpec improvements are good but the node-renderer.js change should be reverted and more context is needed. Review generated with guidance from CLAUDE.md conventions |
Code ReviewThank you for investigating the descriptor closed error. I have reviewed the changes and have several concerns. Critical Concerns1. Streaming Request Retry Logic Change (High Risk) The change in request.rb:102 removes the conditional that uses connection_without_retries for streaming requests. This appears to contradict the original fix for issue #1895, which documented that using retries for streaming requests causes body duplication when connection errors occur mid-stream. Questions:
2. Re-enabling Previously Skipped Tests PR #1941 (merged 2 days ago) skipped these tests because they are flaky and were replaced by Playwright E2E tests. Why re-enable them now? Code Quality Issues
Positive Changes
Recommendations
The investigation is valuable work, but needs clearer documentation and justification given the risks with streaming retry logic. |
Code Review: Investigate
|
Code ReviewSummaryThis PR investigates and attempts to fix the
🔴 Critical Issues1. Potential Body Duplication in Streaming RequestsLocation: -conn = post_options[:stream] ? connection_without_retries : connection
+conn = connectionIssue: This change removes the safeguard against body duplication in streaming requests. The original logic was carefully designed to prevent this exact problem. Evidence from code comments:
Why this is problematic:
Reference: This relates to issue #1895 mentioned in line 245 comments. Recommendation:
|
Code ReviewThank you for working on this PR! I've reviewed the changes and have important feedback regarding the implementation. Critical Concerns1. Streaming Request Retry Logic Change (HIGH PRIORITY) The change from conditional connection selection to always using connection (with retries) contradicts the existing comment and design. The comment at line 100-101 explicitly states streaming requests should NOT use retries to prevent body duplication. StreamRequest#each_chunk (line 78-80 in stream_request.rb) documents that retry logic is handled at the stream level precisely because the HTTP connection has retries disabled. This creates a conflict: HTTP-level retries + stream-level retries could cause duplicate content delivery to clients. The skip message in the old test (line 431) mentions fix-body-dup-retry suggesting this was a known issue. Recommendation: If this change is intentional to fix the descriptor closed error, you need to:
Test Changes2. Removed Test Skip Markers 3. Test Assertion Updates - Good! The changes from have_no_content to not_to have_content follow modern RSpec/Capybara best practices. 4. Regex Pattern for Bundle Detection - Changed to use regex matching. If Shakapacker 9.3.0 adds hash suffixes to bundle names, this makes sense. Consider adding a comment explaining why. Missing Requirements (per CLAUDE.md)5. PR Description - Still template boilerplate. Please explain what descriptor closed error is, the root cause, and why this fixes it. 6. Changelog Entry - Per CLAUDE.md, user-visible bug fixes require a CHANGELOG entry. 7. Tests - Add regression test for the descriptor closed error if possible, and tests verifying no body duplication. Action Items Before Merging
Overall Assessment: The code changes are minimal and targeted, but the streaming retry logic change is significant and needs careful validation. The test improvements are welcome, but the core fix requires better documentation and verification. Let me know if you need help with any of these items! |
Code Review - PR #1970: Investigate
|
| Category | Rating | Notes |
|---|---|---|
| Code Style | ✅ Excellent | RSpec improvements follow best practices |
| Test Changes | 🔴 Concerning | Re-enabling flaky tests without evidence of fixes |
| Logic Changes | 🔴 Critical | Streaming retry behavior change contradicts documented design |
| Documentation | ❌ Poor | PR description incomplete, checklist unchecked |
| Risk Level | 🔴 HIGH | Could introduce streaming bugs and CI flakiness |
🚨 Recommendations
Before Merging:
-
Clarify Intent of Streaming Change
- Is line 102 change intentional or accidental?
- If intentional, update/remove comments on lines 87-89, 100-101
- Remove dead code:
connection_without_retriesmethod and related cleanup - Document why body duplication is no longer a concern
-
Justify Test Re-enabling
- Provide evidence that flaky tests are now stable (e.g., "ran 20 times locally, all passed")
- Explain what fixed the underlying issues
- If not fixed, revert the un-skip changes to avoid CI noise
-
Update PR Description
- Fill out the template with actual information
- Describe what "descriptor closed" error is and where it occurs
- Document investigation findings
- Complete the checklist
-
Validate Changes
- Run
bundle exec rubocop(required per CLAUDE.md) - Run
yarn run eslint --report-unused-disable-directives - Run
bundle exec rake run_rspec:all_dummymultiple times to check stability - Ensure trailing newlines in all files
- Run
-
Consider Splitting the PR
- Style improvements (RSpec refactoring) could be merged separately
- Streaming logic change needs its own PR with thorough testing
- Test re-enabling needs separate justification
❓ Questions for Author
- What is the "descriptor closed" error? Where does it occur? In which logs/tests?
- Is the streaming retry change intentional? If so, what testing confirms it's safe?
- What investigation was performed? What were the findings?
- Why re-enable flaky tests when Playwright E2E coverage already exists?
- Have you run the tests locally? How many times? What was the success rate?
- Related work? Is branch
justin808/fix-body-dup-retrymerged? Related to this PR?
🎯 Verdict
Status: 🔴 CHANGES REQUESTED
Blockers:
- Clarify and justify the streaming request retry logic change (line 102)
- Provide evidence that re-enabled tests are stable
- Complete PR description with investigation details
The RSpec style improvements are excellent and ready to merge once separated from the higher-risk changes.
📚 References
- Per CLAUDE.md: Always run
bundle exec rubocopbefore commits - Per CLAUDE.md: Run
yarn run eslint --report-unused-disable-directivesbefore commits - Test locally with:
bundle exec rake run_rspec:all_dummy - Related comment in code: react_on_rails_pro/lib/react_on_rails_pro/request.rb:100-101
AbanoubGhadban
force-pushed
the
investigate-description-closed-error
branch
from
d8b6c86 to
d3c02bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
react_on_rails_pro/packages/node-renderer/src/shared/configBuilder.ts (1)
205-222: Preserve explicit0timeout when logging configUsing truthy checks means a legit
0(which your docs say disables the kill) is treated as “not provided”, so the logs wrongly claim the env var was used and the sanitized config prints<NOT PROVIDED>. Please preserve0values explicitly.- GRACEFUL_WORKER_RESTART_TIMEOUT: - !userConfig.gracefulWorkerRestartTimeout && env.GRACEFUL_WORKER_RESTART_TIMEOUT, + GRACEFUL_WORKER_RESTART_TIMEOUT: + !('gracefulWorkerRestartTimeout' in userConfig) && env.GRACEFUL_WORKER_RESTART_TIMEOUT, @@ - gracefulWorkerRestartTimeout: aConfig.gracefulWorkerRestartTimeout || defaultValue, + gracefulWorkerRestartTimeout: aConfig?.gracefulWorkerRestartTimeout ?? defaultValue,
🧹 Nitpick comments (1)
react_on_rails_pro/docs/node-renderer/troubleshooting.md (1)
5-5: Polish the wording for clarity“Ensure to not set” reads awkwardly. A small tweak improves readability.
-- If your app contains streamed pages that take too much time to be streamed to the client, ensure to not set the `gracefulWorkerRestartTimeout` parameter or set to a high number, so the worker is not killed while it's still serving an active request. +- If your app streams pages for a long time, make sure you either leave `gracefulWorkerRestartTimeout` unset or set it to a high number so the worker isn’t killed while it’s still serving an active request.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
react_on_rails_pro/docs/installation.md(1 hunks)react_on_rails_pro/docs/node-renderer/js-configuration.md(1 hunks)react_on_rails_pro/docs/node-renderer/troubleshooting.md(1 hunks)react_on_rails_pro/packages/node-renderer/src/master.ts(2 hunks)react_on_rails_pro/packages/node-renderer/src/master/restartWorkers.ts(2 hunks)react_on_rails_pro/packages/node-renderer/src/shared/configBuilder.ts(4 hunks)react_on_rails_pro/packages/node-renderer/src/shared/utils.ts(1 hunks)react_on_rails_pro/packages/node-renderer/src/worker.ts(2 hunks)react_on_rails_pro/packages/node-renderer/src/worker/handleGracefulShutdown.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- react_on_rails_pro/docs/installation.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
react_on_rails_pro/packages/node-renderer/src/worker.ts
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
react_on_rails_pro/packages/node-renderer/src/worker.ts
🧬 Code graph analysis (4)
react_on_rails_pro/packages/node-renderer/src/worker/handleGracefulShutdown.ts (2)
react_on_rails_pro/packages/node-renderer/src/worker/types.ts (1)
FastifyInstance(9-9)react_on_rails_pro/packages/node-renderer/src/shared/utils.ts (1)
SHUTDOWN_WORKER_MESSAGE(14-14)
react_on_rails_pro/packages/node-renderer/src/master/restartWorkers.ts (1)
react_on_rails_pro/packages/node-renderer/src/shared/utils.ts (1)
SHUTDOWN_WORKER_MESSAGE(14-14)
react_on_rails_pro/packages/node-renderer/src/worker.ts (1)
react_on_rails_pro/packages/node-renderer/tests/htmlStreaming.test.js (2)
app(7-7)app(23-23)
react_on_rails_pro/packages/node-renderer/src/master.ts (1)
react_on_rails_pro/spec/dummy/client/node-renderer.js (1)
config(30-66)
🪛 LanguageTool
react_on_rails_pro/docs/node-renderer/js-configuration.md
[grammar] ~25-~25: Ensure spelling is correct
Context: ...id situations where worker stucks at an inifite loop and doesn't restart. This config i...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
react_on_rails_pro/docs/node-renderer/troubleshooting.md
[style] ~5-~5: Consider changing the order of words to improve your wording.
Context: ...me to be streamed to the client, ensure to not set the gracefulWorkerRestartTimeout ...
(TO_NOT_VB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: markdown-link-check
- GitHub Check: claude-review
🔇 Additional comments (4)
react_on_rails_pro/packages/node-renderer/src/master.ts (1)
22-65: Sequential rescheduling logic looks solidPassing the timeout through and re-scheduling via
setTimeoutavoids overlapping restarts and keeps the cadence predictable. Nice improvement.react_on_rails_pro/packages/node-renderer/src/shared/utils.ts (1)
14-14: New shutdown message constant fits wellCentralizing the shutdown message string here keeps the master/worker handshake tidy. Looks good.
react_on_rails_pro/packages/node-renderer/src/worker.ts (1)
20-132: Glad to see graceful shutdown wired inHooking the Fastify instance into
handleGracefulShutdownright after creation makes the worker opt-in automatic. Thanks for threading it through.react_on_rails_pro/packages/node-renderer/src/worker/handleGracefulShutdown.ts (1)
1-49: Graceful shutdown hook looks correctThe message handler, active-request accounting, and destroy/disconnect flow align with how cluster workers should wind down. Implementation looks good to me.
react_on_rails_pro/packages/node-renderer/src/master/restartWorkers.ts
Outdated
Show resolved
Hide resolved
| within("h3") do | ||
| if expect_no_change | ||
| expect(subject).to have_no_content new_text | ||
| expect(subject).not_to have_content new_text |
There was a problem hiding this comment.
All of these changes at this file just reverts the changes made at the PR that skips flaky tests (I think many of these changes unnecessary at that PR)
AbanoubGhadban
changed the title
descriptor closed errordescriptor closed error caused by workers restart while request is in progress
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
react_on_rails_pro/packages/node-renderer/src/master/restartWorkers.ts (2)
27-27: Consider using a TypeScript type guard for better type safety.While the current filter
(w) => !!wworks correctly, using an explicit type guard would provide better TypeScript type narrowing and make the intent clearer.Apply this diff to use a type guard:
- for (const worker of Object.values(cluster.workers).filter((w) => !!w)) { + for (const worker of Object.values(cluster.workers).filter((w): w is cluster.Worker => w !== undefined)) {This matches the suggestion from the past review and eliminates any remaining ambiguity about the worker type within the loop.
33-33: Fix typo in comment.The comment contains a typo: "inteded" should be "intended".
Apply this diff:
- // It's inteded to restart worker in sequence, it shouldn't happens in parallel + // It's intended to restart workers in sequence, it shouldn't happen in parallel
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
react_on_rails_pro/docs/node-renderer/js-configuration.md(1 hunks)react_on_rails_pro/packages/node-renderer/src/master/restartWorkers.ts(2 hunks)react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- react_on_rails_pro/docs/node-renderer/js-configuration.md
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
🧬 Code graph analysis (1)
react_on_rails_pro/packages/node-renderer/src/master/restartWorkers.ts (1)
react_on_rails_pro/packages/node-renderer/src/shared/utils.ts (1)
SHUTDOWN_WORKER_MESSAGE(14-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: markdown-link-check
- GitHub Check: claude-review
🔇 Additional comments (8)
react_on_rails_pro/packages/node-renderer/src/master/restartWorkers.ts (2)
8-8: Good addition of graceful shutdown support.The import of
SHUTDOWN_WORKER_MESSAGEand the updated async function signature withgracefulWorkerRestartTimeoutparameter align well with the new sequential restart flow. The changes enable coordinated graceful shutdown across workers.Also applies to: 18-21
35-58: Well-structured sequential restart logic with graceful shutdown.The implementation correctly handles the sequential restart flow:
- Sends
SHUTDOWN_WORKER_MESSAGEto signal graceful shutdown- Waits for worker exit with an optional timeout
- Forces termination via
worker.destroy()if the timeout expires- Enforces delay between individual worker restarts
The timeout handling (lines 36-52) works as intended. As you clarified in the past review,
clearTimeout(undefined)is safe in Node.js and the code functions correctly at runtime.The cleanup on line 49 (
worker.off('exit', onExit)) before callingresolve()in the timeout handler is good practice to prevent any potential race conditions.react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb (6)
14-14: LGTM! Prefer explicit negation.The change from
have_no_contenttonot_to have_contentis a style improvement that makes the assertion more explicit and aligns with RSpec best practices.
242-242: LGTM! Proper system test metadata.Adding
type: :systemto the describe block is the correct way to mark system tests in RSpec, ensuring proper configuration and test helpers are applied.
399-399: LGTM! Better pattern matching for hashed asset filenames.The regex pattern
/client-bundle[^\"]*.js/correctly handles hashed or versioned bundle filenames (e.g., "client-bundle.abc123.js") that result from asset compilation and cache-busting strategies. This makes the test more resilient to changes in the build process.Also applies to: 406-406
410-412: LGTM! Consistent negation style.Changing from
have_no_texttonot_to have_textmaintains consistency with the explicit negation style used throughout the file and aligns with RSpec best practices.Also applies to: 420-421
425-425: Confirm test stability in CI.The git history confirms these tests were previously skipped (commit a66fb93: "don't skip flaky tests" and 8b32c4c: "don't skip tests"), and they are now properly enabled in the current state. However, the primary concerns require manual verification:
- Run these tests multiple times in CI to confirm they pass consistently without flakiness
- Verify that the "descriptor closed" error investigated in this PR is resolved
- Monitor that the graceful worker restart mechanism prevents re-occurrence of these issues
113-113: ****The review comment references the removal of RuboCop disable comments from lines 113, 177, 184, and 189, but verification shows:
- No RuboCop disable/enable comments exist at these lines in the current file
- The actual changes in commit
4b0f5816("tiny fixes and linting") are at line 406 (updating an expect statement), not the referenced lines- All lines at 113, 177, 184, and 189 are already RuboCop-compliant (well under the 120-character line length limit)
The code requires no action; the original review comment does not match the actual changes made.
Likely an incorrect or invalid review comment.
Summary
Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.
Pull Request checklist
Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by
~.Add the CHANGELOG entry at the top of the file.
Other Information
Remove this paragraph and mention any other important and relevant information such as benchmarks.
This change is
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests