Fix Content-Length mismatch and null renderingRequest errors in node renderer

[justin808]

Fixes #3071

Problem

Two related errors appear in production node renderer logs:

Error 1: FST_ERR_CTP_INVALID_CONTENT_LENGTH

{
  "level": "error",
  "msg": "Unhandled Fastify error",
  "err": {
    "code": "FST_ERR_CTP_INVALID_CONTENT_LENGTH",
    "message": "Request body size did not match Content-Length",
    "statusCode": 400
  }
}

Error 2: INVALID NIL or NULL result for rendering

{
  "level": "error",
  "msg": "INVALID result for prepareResult\n\nJS code for rendering request was:\nnull\n\nEXCEPTION MESSAGE:\nINVALID NIL or NULL result for rendering"
}

Root Cause Analysis

These two errors are causally linked — Error 1 triggers Error 2.

How the render request normally works

1. The Ruby gem sends an HTTP/2 POST to the node renderer with a multipart form body containing renderingRequest (the JavaScript code to execute in the SSR VM), optional bundle files, and metadata fields
2. Fastify's multipart parser reads the body, validates that the received bytes match the Content-Length header, and attaches parsed fields to req.body
3. The render handler extracts req.body.renderingRequest and passes it to the VM for server-side rendering

What goes wrong

The Ruby gem uses HTTPX with persistent HTTP/2 connections to the node renderer. HTTP/2 connections are long-lived and multiplexed, but they can become stale — the node renderer may close its end of the connection (due to idle timeout, restart, or resource pressure) while the Ruby side still considers it active.

When the Ruby gem writes a render request into a stale connection:

1. The connection may accept only part of the request body before the transport layer detects the broken connection
2. The node renderer receives a truncated body — fewer bytes than the Content-Length header promised
3. Fastify's body parser detects the mismatch and throws FST_ERR_CTP_INVALID_CONTENT_LENGTH (Error 1)
4. Because the parser aborted, the multipart fields were never fully parsed — renderingRequest is never attached to req.body
5. The render handler receives req.body.renderingRequest as undefined/null, which propagates into prepareResult() → runInVM(null, ...) → the VM returns null → "INVALID NIL or NULL result for rendering" (Error 2)

The confusing part for operators is that Error 2 looks like a JavaScript/rendering bug ("JS code for rendering request was: null"), when the real problem is a transport-layer issue that happened before any JavaScript executed.

Changes

1. Prevent stale connections (root cause fix)

Ruby gem (request.rb, configuration.rb): Add keep_alive_timeout (default: 30s) to the HTTPX persistent connection configuration. This tells HTTPX to close idle connections after 30 seconds, preventing the Ruby side from writing into connections that the node renderer has already closed. Configurable via renderer_http_keep_alive_timeout with validation (must be a positive number or nil).

2. Early validation with actionable diagnostics (defense in depth)

Node renderer (worker.ts): Validate renderingRequest immediately after body parsing, before entering the render pipeline. When the field is missing, null, or empty, the renderer now returns a descriptive 400 error instead of letting null propagate through the VM:

Invalid "renderingRequest" field in render request.
Expected a non-empty string of JavaScript to execute in the SSR VM.
Received type: null.
Received body keys: gemVersion, protocolVersion, railsEnv.
Likely causes: request body truncation, malformed multipart form data, or Content-Length mismatch in a proxy/client.

This gives operators immediate insight into what happened and where to look, rather than the misleading "INVALID NIL or NULL result for rendering" message.

3. Specific Content-Length mismatch logging

Node renderer (worker.ts): The Fastify onError hook now detects FST_ERR_CTP_INVALID_CONTENT_LENGTH specifically and logs it as "Invalid request body framing" with an actionable hint about client/proxy truncation, rather than the generic "Unhandled Fastify error" message.

4. Security: expanded sensitive key filtering

Node renderer (worker.ts): The diagnostic message includes body keys to help debugging, but filters out sensitive field names. Expanded from just password to also filter token, secret, api_key, auth_token, authorization, and credentials — since these diagnostics flow through error reporters (Sentry, Honeybadger).

Test plan

• Node renderer worker tests pass (23/23), including new tests for missing, null, and empty-string renderingRequest, and expanded sensitive key filtering
• Pro gem configuration spec passes (46/46), including 6 new tests for renderer_http_keep_alive_timeout validation
• RuboCop passes on all changed files
• ESLint and Prettier pass on all changed files
• CI

🤖 Generated with Claude Code

---

Note

Medium Risk
Touches the Rails↔Node renderer transport configuration (HTTPX timeouts) and request validation/logging, which can affect SSR reliability and performance if misconfigured, but is scoped and covered by tests.

Overview
Fixes SSR failures caused by stale persistent connections by adding a new Ruby config renderer_http_keep_alive_timeout (default 30) and wiring it into HTTPX keep_alive_timeout (with validation and improved connection error context).

Hardens the Node renderer against truncated/malformed multipart bodies by returning an actionable 400 when renderingRequest is missing/null/empty/array, refining the reported type for empty strings, and filtering additional sensitive body keys (including credentials) from diagnostics; corresponding tests are updated/added.

^{Reviewed by Cursor Bugbot for commit 818020c. Bugbot is set up for automated code reviews on this repo. Configure here.}

Summary by CodeRabbit

• New Features

  • Added configurable HTTP keep-alive timeout for Node renderer connections (default: 30s); accepts positive numbers or nil.

• Bug Fixes

  • Clarified invalid rendering-request diagnostics — empty-string inputs now reported as "empty string" for received-type messaging.

• Tests

  • Added tests for keep-alive timeout validation and updated expectation for empty-string rendering-request diagnostics.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Adds a configurable renderer HTTP keep-alive timeout (default 30s), wires it into the HTTP client, validates and tests the config, and adjusts node-renderer invalid-request error messaging and its test expectation (adds credentials to filtered keys and changes empty-string reporting).

Changes

Cohort / File(s)	Summary
Node renderer: error messaging & tests packages/react-on-rails-pro-node-renderer/src/worker.ts, packages/react-on-rails-pro-node-renderer/tests/worker.test.ts	Added credentials to sensitive request-body keys; changed how empty-string renderingRequest is reported ("empty string" vs previous "string (empty)"); updated test expectation accordingly.
Configuration: keep-alive timeout react_on_rails_pro/lib/react_on_rails_pro/configuration.rb, react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb	Added DEFAULT_RENDERER_HTTP_KEEP_ALIVE_TIMEOUT = 30; exposed renderer_http_keep_alive_timeout with validated setter (positive Numeric or nil); updated initializer and added specs for default, valid, and invalid values.
HTTP client: apply setting & test fixture react_on_rails_pro/lib/react_on_rails_pro/request.rb, react_on_rails_pro/spec/dummy/spec/requests/upload_asset_spec.rb	Threaded renderer_http_keep_alive_timeout into HTTPX pool timeout options when set; included the new config value in a test double used by request specs.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Rails as Rails App
  participant HTTPX as HTTPX Pool
  participant Node as Node Renderer
  Rails->>HTTPX: open/pool request (keep_alive_timeout configured)
  HTTPX->>Node: send HTTP/2 request (body + Content-Length)
  Node->>Node: validate renderingRequest (null/array/empty-string)
  alt invalid renderingRequest
    Node->>Rails: respond 400 with detailed diagnostic (type, filtered body keys)
  else valid renderingRequest
    Node->>Node: render and return result
    Node->>Rails: respond 200 with render result
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐇 I nibbled headers, kept the stream alive,
I hid the secrets — credentials survive.
Thirty ticks to guard the channel's beat,
No more truncated messages on the street.
A rabbit's wink: connections tidy and spry.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title refers to fixing Content-Length and null renderingRequest errors, which are the primary issues addressed by the PR changes to request handling, configuration, and error validation.
Linked Issues check	✅ Passed	PR fully implements the stated objectives from #3071: adds renderer_http_keep_alive_timeout configuration with validation, validates renderingRequest immediately with clear 400 errors, expands sensitive-key filtering, and improves diagnostics.
Out of Scope Changes check	✅ Passed	All changes are directly tied to addressing the Content-Length and null renderingRequest issues: configuration for keep-alive timeout, request body validation, error message improvements, and related test updates.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jg/rorp-content-length-null-render

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Review

Good diagnostic improvement overall. The two-layer fix (upstream validation in the route handler + better onError logging) clearly addresses the confusing nil-render error that was masking the real Content-Length framing problem. The Ruby-side keep_alive_timeout addition is a sensible preventive measure.

Issues

Missing validation for renderer_http_keep_alive_timeout (see inline) — no guard against 0, negative, or non-numeric values before they reach HTTPX. Other timeout options also lack explicit validation, so this follows the existing pattern, but adding it here would be a net improvement.

Missing test for empty-string renderingRequest (see inline) — isValidRenderingRequest rejects "" but no test covers it. Worth adding for completeness.

Body keys forwarded to error reporters (see inline) — errorResponseResult calls errorReporter.message(), so the body key list ends up in Sentry/Honeybadger. Low risk for a server-to-server SSR channel, but worth knowing.

Observations

• Widening Body to Record<string, unknown> is the right call here — the stricter type would have hidden the exact runtime failure the PR surfaces. The type predicate on isValidRenderingRequest correctly re-narrows to string for all downstream use.
• The onError hook differentiation is clean: specific actionable message for the known framing error, generic fallback for everything else.
• The improved message in handleRenderRequest.ts (Ensure renderingRequest is a valid string and returns a value) remains useful as a last-resort fallback for cases that slip past the new route-level guard (e.g. a non-empty string that evaluates to null in the VM).

[github-actions]

size-limit report 📦

Path	Size
react-on-rails/client bundled (gzip)	62.63 KB (0%)
react-on-rails/client bundled (gzip) (time)	62.63 KB (0%)
react-on-rails/client bundled (brotli)	53.7 KB (0%)
react-on-rails/client bundled (brotli) (time)	53.7 KB (0%)
react-on-rails-pro/client bundled (gzip)	63.56 KB (0%)
react-on-rails-pro/client bundled (gzip) (time)	63.56 KB (0%)
react-on-rails-pro/client bundled (brotli)	54.6 KB (0%)
react-on-rails-pro/client bundled (brotli) (time)	54.6 KB (0%)
registerServerComponent/client bundled (gzip)	127.39 KB (0%)
registerServerComponent/client bundled (gzip) (time)	127.39 KB (0%)
registerServerComponent/client bundled (brotli)	61.52 KB (0%)
registerServerComponent/client bundled (brotli) (time)	61.52 KB (0%)
wrapServerComponentRenderer/client bundled (gzip)	122.02 KB (0%)
wrapServerComponentRenderer/client bundled (gzip) (time)	122.02 KB (0%)
wrapServerComponentRenderer/client bundled (brotli)	56.77 KB (0%)
wrapServerComponentRenderer/client bundled (brotli) (time)	56.77 KB (0%)

[greptile-apps]

Greptile Summary

This PR fixes two related production errors: stale HTTPX persistent connections causing FST_ERR_CTP_INVALID_CONTENT_LENGTH (addressed by adding keep_alive_timeout: 30 to the HTTPX client), and the cascading null renderingRequest error it triggers (addressed by adding early runtime validation in the Fastify route with a descriptive 400 response). The onError hook is also updated to emit a more actionable log message specifically for Content-Length framing errors.

Confidence Score: 5/5

Safe to merge — all remaining findings are non-blocking P2 style suggestions

The core logic is sound: the keep_alive_timeout fix directly addresses the root cause, and the early renderingRequest validation with a type guard correctly narrows the type for downstream use. Both new tests are meaningful regression coverage. No P0 or P1 issues found.

No files require special attention before merge

Important Files Changed

Filename	Overview
packages/react-on-rails-pro-node-renderer/src/worker.ts	Adds isValidRenderingRequest guard and invalidRenderingRequestMessage helper; relaxes Body type to Record<string, unknown>; detects FST_ERR_CTP_INVALID_CONTENT_LENGTH in the onError hook with an actionable log message
packages/react-on-rails-pro-node-renderer/src/worker/handleRenderRequest.ts	Improves the nil-result error message to mention renderingRequest as a likely cause, aiding post-incident debugging
packages/react-on-rails-pro-node-renderer/tests/worker.test.ts	Adds two regression tests covering missing and null renderingRequest payloads, both asserting 400 status and actionable message content
react_on_rails_pro/lib/react_on_rails_pro/configuration.rb	Adds renderer_http_keep_alive_timeout constant (30s default), attr_accessor, and initializer wiring; no validation guard added
react_on_rails_pro/lib/react_on_rails_pro/request.rb	Moves keep_alive_timeout from a comment into the live HTTPX timeout: hash, directly fixing the stale-stream root cause

Sequence Diagram

sequenceDiagram
    participant Rails as Rails (HTTPX client)
    participant Node as Node Renderer (Fastify)

    Note over Rails,Node: keep_alive_timeout=30s closes idle connections before they go stale

    Rails->>Node: POST /bundles/:ts/render/:digest (multipart)

    alt Body framing valid
        Node->>Node: isValidRenderingRequest(renderingRequest)
        alt renderingRequest is non-empty string
            Node->>Node: handleRenderRequest()
            Node-->>Rails: 200 OK + SSR result
        else renderingRequest missing / null / empty
            Node-->>Rails: 400 Bad Request + actionable message
        end
    else FST_ERR_CTP_INVALID_CONTENT_LENGTH
        Node->>Node: onError hook → log "Invalid request body framing"
        Node-->>Rails: 400 Content-Length mismatch
        Note over Rails: Connection closed; next request opens fresh stream
    end

Loading

_{Reviews (1): Last reviewed commit: "Add keep_alive_timeout to HTTPX renderer..." | Re-trigger Greptile}

[coderabbitai]

🧹 Nitpick comments (1)

packages/react-on-rails-pro-node-renderer/tests/worker.test.ts (1)

104-145: Add one test for empty-string renderingRequest to fully cover the new validator.

You now validate non-empty strings, but only missing and null are asserted here.

Proposed test addition

+  test('POST /bundles/:bundleTimestamp/render/:renderRequestDigest returns actionable error when renderingRequest is empty string', async () => {
+    const app = worker({
+      serverBundleCachePath: serverBundleCachePathForTest(),
+    });
+
+    const res = await app
+      .inject()
+      .post(`/bundles/${BUNDLE_TIMESTAMP}/render/d41d8cd98f00b204e9800998ecf8427e`)
+      .payload({
+        gemVersion,
+        protocolVersion,
+        railsEnv,
+        renderingRequest: '',
+      })
+      .end();
+
+    expect(res.statusCode).toBe(400);
+    expect(res.payload).toContain('Invalid "renderingRequest" field in render request.');
+    expect(res.payload).toContain('Received type: string.');
+  });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/react-on-rails-pro-node-renderer/tests/worker.test.ts` around lines
104 - 145, Add a third test alongside the two existing cases in worker.test.ts
that posts to /bundles/:bundleTimestamp/render/:renderRequestDigest with
renderingRequest set to an empty string (""), using the same app setup
(worker(...) with serverBundleCachePathForTest()) and payload fields
(gemVersion, protocolVersion, railsEnv); assert the response status is 400 and
that res.payload contains the same invalid field message ('Invalid
"renderingRequest" field in render request.'), the specific received-type
message for an empty string (e.g. 'Received type: empty string.'), and the
actionable hint ('Likely causes: request body truncation') so the validator is
fully covered for missing, null, and empty-string cases.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/react-on-rails-pro-node-renderer/tests/worker.test.ts`:
- Around line 104-145: Add a third test alongside the two existing cases in
worker.test.ts that posts to
/bundles/:bundleTimestamp/render/:renderRequestDigest with renderingRequest set
to an empty string (""), using the same app setup (worker(...) with
serverBundleCachePathForTest()) and payload fields (gemVersion, protocolVersion,
railsEnv); assert the response status is 400 and that res.payload contains the
same invalid field message ('Invalid "renderingRequest" field in render
request.'), the specific received-type message for an empty string (e.g.
'Received type: empty string.'), and the actionable hint ('Likely causes:
request body truncation') so the validator is fully covered for missing, null,
and empty-string cases.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7a27081d-dd61-4943-af1a-bb2f284273ef

📥 Commits

Reviewing files that changed from the base of the PR and between 7dd9ede and 2fdaaaf.

📒 Files selected for processing (5)

• packages/react-on-rails-pro-node-renderer/src/worker.ts
• packages/react-on-rails-pro-node-renderer/src/worker/handleRenderRequest.ts
• packages/react-on-rails-pro-node-renderer/tests/worker.test.ts
• react_on_rails_pro/lib/react_on_rails_pro/configuration.rb
• react_on_rails_pro/lib/react_on_rails_pro/request.rb

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2fdaaaf572

ℹ️ 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".

[claude]

Review Summary

Overall the approach is correct and addresses a real production issue. The error chain (Content-Length mismatch → body parsing failure → null renderingRequest) is well-diagnosed, and the early validation with descriptive error messages is a meaningful improvement. A few things to address before merging:

Security — incomplete sensitive-key filtering (inline comment)

SENSITIVE_REQUEST_BODY_KEYS only filters password. The error message including body keys is forwarded to external error-tracking integrations via errorReporter.message. Keys like token, secret, api_key, auth_token, authorization, and credentials would leak through. See inline comment at worker.ts:154.

Potential noise — double reporting on Content-Length mismatch (inline comment)

When FST_ERR_CTP_INVALID_CONTENT_LENGTH is the root cause, both the onError hook logs it and errorResponseResult fires all Sentry/Honeybadger notifiers for the resulting "invalid renderingRequest" 400. This creates two alerts per event, with the second one misleading on root cause. See inline comment at worker.ts:349.

Missing spec coverage for keep_alive_timeout (inline comment)

The new renderer_http_keep_alive_timeout config has no unit test, and stream_spec.rb still has # :keep_alive_timeout as a disabled comment while the production code now enables it. See inline comment at request.rb:303.

Minor — test count in PR description

The description says "2 new tests" but the diff adds 4 tests (missing, null, empty string, and password-key filtering). Not a code issue, just a mismatch.

[AbanoubGhadban]

Opened #3071 to track the production errors this PR addresses. We couldn't reproduce locally, but the fix targets the right root cause. Merging and will monitor.

[AbanoubGhadban]

LGTM. This addresses the production errors tracked in #3071. The early validation, Fastify error hook, and HTTPX keep_alive_timeout all look correct. Merging to verify the fix in production.

[justin808]

Review feedback addressed — all threads resolved

Code changes (3 commits on jg/3071-content-length-null-render)

a05ba36 — Address review feedback on content-length/null-render fix:

• Expanded SENSITIVE_REQUEST_BODY_KEYS from 1 to 14 entries (token, secret, api_key, authorization, credentials, etc.)
• Changed empty-string type label to 'empty string' via dedicated else-if branch
• Added custom setter with validation for renderer_http_keep_alive_timeout (positive number or nil)
• Added 6 configuration spec cases (default, positive, nil, zero, negative, non-numeric)

54b3cd0 — Sync stream_spec.rb timeout hash with production keep_alive_timeout:

• Updated stream_spec.rb to include keep_alive_timeout: 30 in the HTTPX timeout hash, matching production create_connection config

Thread resolution summary

#	File	Comment	Status
1	request.rb:303	stream_spec keep_alive_timeout sync	Fixed in 54b3cd0
2	worker.ts:180	Body keys in error reporters	Acknowledged; 14 sensitive keys now filtered
3	worker.ts:151	Empty-string test missing	Fixed in a05ba36
4	worker.ts:337	Body type widened	Informational; type predicate narrows correctly
5	configuration.rb:129	keep_alive_timeout validation	Fixed in a05ba36
6	worker.ts:164-183	Empty-string type label	Fixed in a05ba36
7	configuration.rb:48	keep_alive_timeout validation (dup)	Fixed in a05ba36
8	request.rb:303	Test doubles breakage claim	Verified not an issue
9	worker.ts	Sensitive key set too narrow	Fixed in a05ba36
10	worker.ts:359	Double error reporting	Already handled by badRequestResponseResult

All 10 review threads replied to and resolved.

[claude]

Review: Fix Content-Length mismatch and null renderingRequest errors

Overall this is a solid, well-reasoned fix with good test coverage. The root-cause analysis is accurate — stale HTTP/2 connections causing truncated bodies is a real and non-obvious failure mode. The three-layer approach (prevent staleness, validate early, improve diagnostics) is the right pattern for production hardening.

A few things to flag:

Minor logic imprecision in invalidRenderingRequestMessage

} else if (typeof renderingRequest === 'string') {
  renderingRequestType = 'empty string';
}

This branch runs for any string, not specifically an empty one. It's functionally correct today because isValidRenderingRequest already gates non-empty strings, so only empty strings reach this function — but the code doesn't make that contract explicit. If the validator changes or someone calls the function directly in a test, a non-empty string would be mislabelled as 'empty string'. renderingRequest === '' would be more precise and self-documenting.

Missing renderer_http_keep_alive_timeout in the connection error diagnostic

request.rb's create_connection rescue block prints a diagnostic listing pool size, pool timeout, warn timeout, and URL — but doesn't include renderer_http_keep_alive_timeout. Since that new field can influence connection behaviour, including it in the diagnostic would make it complete.

30s default needs alignment with node renderer idle timeout

The fix works by closing idle Ruby connections before the node renderer closes its end. That only works if the Ruby-side timeout is shorter than the node renderer's actual idle/keep-alive timeout. If the node renderer closes connections after (say) 20s, the 30s default would still leave a window. Worth documenting the expected relationship (e.g. "set this to less than your node renderer's keep-alive or idle timeout") in the config setter's docstring.

Tests

The six new configuration specs and three new worker tests cover the important paths well. The sensitive-key filtering test is thorough and the camelCase/screaming-snake-case variants are a good edge-case check.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Bugbot Autofix is ON, but it could not run because on-demand usage is turned off. To enable Bugbot Autofix, turn on on-demand usage and set a spend limit in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 7183faa. Configure here.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7183faa7a9

ℹ️ 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".

[coderabbitai]

🧹 Nitpick comments (2)

react_on_rails_pro/lib/react_on_rails_pro/request.rb (1)

307-313: Include keep-alive timeout in connection error diagnostics.

When connection creation fails, logging the new keep-alive setting would make misconfiguration triage faster.

Suggested diff

         message = <<~MSG
           [ReactOnRailsPro] Error creating HTTPX connection.
           renderer_http_pool_size = #{ReactOnRailsPro.configuration.renderer_http_pool_size}
           renderer_http_pool_timeout = #{ReactOnRailsPro.configuration.renderer_http_pool_timeout}
           renderer_http_pool_warn_timeout = #{ReactOnRailsPro.configuration.renderer_http_pool_warn_timeout}
+          renderer_http_keep_alive_timeout = #{ReactOnRailsPro.configuration.renderer_http_keep_alive_timeout}
           renderer_url = #{url}
           Be sure to use a url that contains the protocol of http or https.
           Original error is
           #{e}
         MSG

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@react_on_rails_pro/lib/react_on_rails_pro/request.rb` around lines 307 - 313,
The connection error diagnostic message built in Request (where `message` is
constructed) is missing the keep-alive timeout; update the error message string
to include the keep-alive setting (e.g.
`ReactOnRailsPro.configuration.renderer_http_keepalive_timeout` or the actual
keep-alive config name used in your code) so the log prints the keep-alive
timeout alongside `renderer_http_pool_size`, `renderer_http_pool_timeout`,
`renderer_http_pool_warn_timeout`, and `renderer_url`; modify the `message`
construction in `ReactOnRailsPro::Request` (the same block that sets `message =
<<~MSG`) to interpolate the keep-alive config value for easier triage.

react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb (1)

433-437: Prefer constant-based default expectation.

Using the constant here avoids brittle duplication if the default changes later.

Suggested diff

-      it "defaults to 30" do
+      it "defaults to the configured constant" do
         ReactOnRailsPro.configure {} # rubocop:disable Lint/EmptyBlock

-        expect(ReactOnRailsPro.configuration.renderer_http_keep_alive_timeout).to eq(30)
+        expect(ReactOnRailsPro.configuration.renderer_http_keep_alive_timeout)
+          .to eq(ReactOnRailsPro::Configuration::DEFAULT_RENDERER_HTTP_KEEP_ALIVE_TIMEOUT)
       end

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb` around
lines 433 - 437, Replace the hardcoded 30 with the configuration default
constant: update the spec to expect
ReactOnRailsPro.configuration.renderer_http_keep_alive_timeout to equal the
constant that defines the default timeout (e.g.,
ReactOnRailsPro::Configuration::DEFAULT_RENDERER_HTTP_KEEP_ALIVE_TIMEOUT or the
actual constant name found in the configuration class/module) instead of 30 so
the test follows the source-of-truth constant used by ReactOnRailsPro.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@react_on_rails_pro/lib/react_on_rails_pro/request.rb`:
- Around line 307-313: The connection error diagnostic message built in Request
(where `message` is constructed) is missing the keep-alive timeout; update the
error message string to include the keep-alive setting (e.g.
`ReactOnRailsPro.configuration.renderer_http_keepalive_timeout` or the actual
keep-alive config name used in your code) so the log prints the keep-alive
timeout alongside `renderer_http_pool_size`, `renderer_http_pool_timeout`,
`renderer_http_pool_warn_timeout`, and `renderer_url`; modify the `message`
construction in `ReactOnRailsPro::Request` (the same block that sets `message =
<<~MSG`) to interpolate the keep-alive config value for easier triage.

In `@react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb`:
- Around line 433-437: Replace the hardcoded 30 with the configuration default
constant: update the spec to expect
ReactOnRailsPro.configuration.renderer_http_keep_alive_timeout to equal the
constant that defines the default timeout (e.g.,
ReactOnRailsPro::Configuration::DEFAULT_RENDERER_HTTP_KEEP_ALIVE_TIMEOUT or the
actual constant name found in the configuration class/module) instead of 30 so
the test follows the source-of-truth constant used by ReactOnRailsPro.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 49f32f83-9383-494c-aea8-a644c3adaa12

📥 Commits

Reviewing files that changed from the base of the PR and between 4153c9e and 7183faa.

📒 Files selected for processing (5)

• packages/react-on-rails-pro-node-renderer/src/worker.ts
• packages/react-on-rails-pro-node-renderer/tests/worker.test.ts
• react_on_rails_pro/lib/react_on_rails_pro/configuration.rb
• react_on_rails_pro/lib/react_on_rails_pro/request.rb
• react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb

✅ Files skipped from review due to trivial changes (1)

• packages/react-on-rails-pro-node-renderer/tests/worker.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/react-on-rails-pro-node-renderer/src/worker.ts

[claude]

Review

The root-cause analysis and layered defense strategy are well thought out — stale connection prevention at the transport layer plus early validation with actionable diagnostics is the right approach. Tests are thorough and cover the meaningful edge cases. A few notes:

Correctness: The initial typeof renderingRequest assignment in invalidRenderingRequestMessage returns 'object' for a null input (since typeof null === 'object' in JS) before being corrected on the next line. The output is correct today, but it is a silent footgun. The inline suggestion restructures this as a proper if-else chain so the fallback is last, not first.

Simplification: The **(if … else {} end) construct in request.rb achieves the goal but is noisy. Ruby's Hash#compact gives the same semantics in one line and scales better if more optional timeout keys are added later. See inline suggestion.

Security: 'credentials' was added to the sensitive key filter, but 'credential' (singular — used by some AWS/cloud SDKs) is not covered. Low-risk since this is an internal service, but worth closing while here.

Tests: The validator accepts any Numeric including floats, but only integer cases are exercised. A single float test would document that intent and guard against an accidental narrowing to Integer in a future refactor.

Question on the 30-second default: The goal is to close idle connections before the node renderer closes them first. Does the node renderer have an explicit HTTP/2 idle timeout configured? If Fastify/Node has no idle timeout (or a very long one), any client-side value works fine. But if the renderer closes connections after, say, 25 seconds, the 30-second Ruby default would still leave a window where stale connections get picked up. Worth documenting the intended relationship between the two values, even just in a code comment near the constant.

[justin808]

Third round of review feedback addressed — all threads resolved

Code change (commit a298f04)

1. Use .compact for conditional keep_alive_timeout (request.rb:300-304) — Replaced the **(if…else…end) idiom with .compact, which drops keep_alive_timeout: nil automatically and is cleaner to extend with future optional timeout keys.

Declined with rationale (4 threads)

2. typeof ordering nit (worker.ts:175-182) — Code produces correct result; restructuring for a transient intermediate value isn't worth the churn.
3. Add 'credential' singular to sensitive keys (worker.ts:170) — Server-to-server SSR context with well-known body shape; 14 keys is sufficient.
4. Add float test for keep_alive_timeout (configuration_spec.rb:443) — Validator accepts Numeric (includes Float) and works correctly; not worth the added test surface.

Already addressed (2 duplicate threads)

5. typeof renderingRequest === 'string' → renderingRequest === '' — Already fixed in fe6d0ed.
6. Verify HTTPX nil behavior — Already addressed in fe6d0ed, further simplified with .compact in a298f04.

All 6 unresolved threads now resolved. Zero open review threads remaining.

[claude]

Overall this is a clean, well-scoped fix. The root cause analysis is accurate and the layered approach (prevent stale connections + early validation + actionable diagnostics) is solid. A few notes below.

Issue 1: Float::INFINITY passes validation silently

The setter guard value.is_a?(Numeric) && value.positive? accepts Float::INFINITY since it satisfies both conditions. That is almost certainly not a useful keep-alive timeout and would likely confuse operators who set it by mistake. Consider rejecting infinite values explicitly: unless value.nil? || (value.is_a?(Numeric) && value.positive? && !value.infinite?)

Issue 2: Client timeout should be shorter than the server-side idle timeout

The default of 30 s is only correct if the node renderer Fastify server keeps connections open for longer than 30 s. If Fastify own keep-alive/idle timeout is <= 30 s, writes can still land on a just-closed connection in the race window. Worth documenting the relationship between this setting and Fastify keepAliveTimeout so operators know to set the client value shorter than the server value.

Nitpick: No CHANGELOG entry

The new renderer_http_keep_alive_timeout config option has no CHANGELOG entry. Worth adding for discoverability.

Nitpick: Hard-coded default in upload_asset_spec.rb

The fixture hard-codes renderer_http_keep_alive_timeout: 30. If the default constant changes later this will silently diverge. Using Configuration::DEFAULT_RENDERER_HTTP_KEEP_ALIVE_TIMEOUT would make the coupling explicit.

[justin808]

Fourth round of review feedback addressed — all threads resolved

Code change (commit 818020c)

1. Reject Float::INFINITY (configuration.rb:101) — Added .finite? to the validation guard so infinite values are rejected. Updated the error message to say "finite positive number or nil". Added a spec case for Float::INFINITY.

Declined with rationale (2 threads)

2. Document client/server timeout relationship (request.rb:303) — Previously raised and declined in round 2. The existing docstring covers type/validation/purpose; the server relationship is an operational concern for deployment docs.
3. Use constant instead of hard-coded 30 in upload_asset_spec.rb — The fixture intentionally mirrors the expected value. Using the constant would couple the test to the implementation. A test failure on default change is desirable.

All 3 threads resolved. Zero open review threads remaining.

[claude]

TEST COMMENT - PLEASE IGNORE - will be deleted

[claude]

TEST COMMENT REPLACED

[claude]

Review: Fix Content-Length mismatch and null renderingRequest errors

Overall this is a well-structured fix with good defense-in-depth layering (root cause fix + early validation + improved diagnostics). The test coverage is thorough for the new configuration option and the renderingRequest validation paths. A few issues worth addressing:

[claude]

Review: Fix Content-Length mismatch and null renderingRequest errors

Overall this is a well-structured fix with good defense-in-depth layering (root cause fix + early validation + improved diagnostics). The test coverage is thorough for the new configuration option and the renderingRequest validation paths. A few issues worth addressing:

Bug: credentials is not tested in the sensitive-key filter

The PR adds 'credentials' to SENSITIVE_REQUEST_BODY_KEYS but the sensitive-key filtering test does not include credentials in the payload, nor assert that it is filtered out. Every other key added to the set has corresponding test coverage — credentials should too.

Architecture concern: client-side keep-alive must be shorter than server-side

The 30 s default for renderer_http_keep_alive_timeout is a client-side idle-close timeout. For this to fully eliminate the stale-connection window, the client timeout must be shorter than whatever idle timeout the node renderer HTTP/2 stack applies server-side. If the server closes idle connections after, say, 25 s, a client configured at 30 s still has a 25 s race window.

The PR does not mention what the node renderer server-side keep-alive or idle timeout is, or how the 30 s default was chosen relative to it. This should be documented in the Ruby doc comment for the option so operators know to keep renderer_http_keep_alive_timeout shorter than the server idle timeout.

Minor: renderingRequest appears in the diagnostic body-keys list

When renderingRequest is present but invalid (null, empty, wrong type), invalidRenderingRequestMessage will include renderingRequest in the bodyKeys output since it is not a sensitive key. The message then reads something like:

Received type: null.
Received body keys: renderingRequest, gemVersion, protocolVersion, railsEnv.

An operator might find it confusing that renderingRequest appears in "body keys" right after a line explaining its type is wrong. Filtering it from bodyKeys (since it is already reported separately) would make the diagnostic cleaner. This is minor, not blocking.

Nits

• Removing # :keep_alive_timeout from the "Other timeouts supported" comment block is the right cleanup.
• value.finite? in the Ruby setter correctly handles Float::INFINITY and Float::NAN; the guard is solid.
• Using .compact to drop nil from the timeout hash is the right idiom for the "use HTTPX default" escape hatch.