[Pro] Reconnect stale renderer keep-alive; clarify per-thread connection math (#4571)#4575
[Pro] Reconnect stale renderer keep-alive; clarify per-thread connection math (#4571)#4575AbanoubGhadban wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughPersistent renderer clients now recover from selected stale keep-alive failures by reconnecting and retrying replay-safe requests once. Configuration comments, upgrade guidance, changelog entries, type declarations, and tests document and validate the updated behavior. ChangesRenderer transport updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RailsRequest
participant PersistentThreadClient
participant AsyncHTTPClient
participant NodeRenderer
RailsRequest->>PersistentThreadClient: submit render request
PersistentThreadClient->>AsyncHTTPClient: send request on warm client
AsyncHTTPClient->>NodeRenderer: reuse keep-alive connection
NodeRenderer-->>AsyncHTTPClient: stale connection failure
PersistentThreadClient->>AsyncHTTPClient: rebuild client
PersistentThreadClient->>AsyncHTTPClient: retry replay-safe request once
AsyncHTTPClient->>NodeRenderer: send request over fresh connection
NodeRenderer-->>RailsRequest: render response
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
|
+ci-run-hosted |
Hosted CI RequestedTriggered 9 workflow(s) for View progress in the Actions tab. |
github-actions
Bot
added
the
ready-for-hosted-ci
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.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: c516e957a0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ReviewOverviewThis PR fixes issue #4571: the no-scheduler persistent per-thread renderer HTTP client (introduced in #4394) fails hard on a stale/idle-closed keep-alive connection instead of transparently reconnecting, and the Strengths
Potential issue: retry scope also covers mid-response-body drops (see inline comment)In Minor notes
|
Review: [Pro] Reconnect stale renderer keep-alive; clarify per-thread connection math (#4571)OverviewThis is a targeted regression fix for the reliability issue introduced by #4394, which switched the plain-Puma (no Code quality / correctness
Test coverageGood breadth: warm reconnect+retry (POST and GET/nil-body), not-warm no-retry, non-replayable multipart no-retry, second-drop no-infinite-retry, DocsThe SecurityNo security concerns - this only affects retry/reconnect behavior on an already-authenticated internal renderer connection; no new external input handling. ScopeThe decision to leave the "global cap across threads" ask from #4571 as a doc-only fix (rather than new config) is reasonable and well-justified in the PR description - avoids introducing a new public config surface for a regression fix. Overall: solid, well-tested, appropriately scoped fix. No blocking issues found. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…ion math (#4571) PR #4394 changed the plain-Puma (no Fiber.scheduler) non-streaming renderer path to a persistent async-http client per Puma request thread, keeping a renderer connection warm across requests on that thread. That client is built with retries: 0, so reusing a keep-alive connection the renderer, a load balancer, or a proxy has since idle-closed fails hard instead of transparently reconnecting -- a new failure mode (issue #4571). It also means renderer_http_pool_size bounds connections per client, not globally, which the config docs still described as per-request. Fix (minimal, no new config): - PersistentThreadClient now transparently reconnects and retries a request ONCE when a warm client (>=1 prior success) hits a dropped-connection error on a replay-safe body (nil or String). The retry is skipped for first-contact failures (so a genuinely-down renderer still fails fast), for consumable upload bodies (MultipartBody, never re-sent after a partial write), and for reachability/startup errors and HTTP/2 stream resets (which stay with the outer renderer_request_retry_limit loop). New STALE_KEEPALIVE_RETRY_ERRORS set scopes this to IOError/EOFError, ECONNRESET, EPIPE, and RefusedError. - Docs: correct renderer_http_pool_size to describe the per-thread persistent client and clarify that total warm renderer capacity scales with live Puma request threads, not with the pool size alone (configuration.rb, configuration-pro.md, updating.md, CHANGELOG). The global-connection-cap suggestion from #4571 is intentionally handled as a documentation change rather than new config: a cross-thread cap would either be a no-op default or an arbitrary new public config contract with a silent ephemeral-fallback policy. Fixes #4571. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ilures (#4575) Review feedback on the reconnect fix (#4571): - greptile P1 + Claude: the retry `rescue` wrapped both the send and `buffer_response`, so a connection drop while reading the response body (after the renderer had already executed the render) was retried, re-running the render. The `begin/rescue *STALE_KEEPALIVE_RETRY_ERRORS` now wraps only the send; a drop during buffering falls to the terminal rescue and surfaces as an error with no retry. - Codex P2: `IO::TimeoutError` is a subclass of `IOError`, so a warm read timeout entered the retry branch. `retry_stale_connection?` now excludes it, so timeouts flow to the outer timeout/`renderer_request_retry_limit` handling. - Claude: after a failed reconnect+retry, `warm` is reset to false so a sustained renderer outage stops paying a doomed reconnect on every subsequent request. - Claude: a build failure during reconnect now surfaces via the terminal rescue instead of killing the worker thread. Adds specs for the mid-body-read drop, IO::TimeoutError exclusion, warm reset, and reconnect-build-failure worker survival. Docs/CHANGELOG note the retry only covers failures before the renderer returns a response. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
342afeb to
b899342
Compare
|
+ci-run-hosted |
Addressed review feedback (commit
|
Hosted CI RequestedTriggered 9 workflow(s) for View progress in the Actions tab. |
| def handle_request(client, message, warm) | ||
| _type, method, path, headers, body, result = message | ||
| raw_response = method == :post ? client.post(path, headers:, body:) : client.get(path, headers:) | ||
| reconnected = false | ||
| begin | ||
| raw_response = method == :post ? client.post(path, headers:, body:) : client.get(path, headers:) | ||
| rescue *STALE_KEEPALIVE_RETRY_ERRORS => e | ||
| if retry_stale_connection?(warm, reconnected, e, body) | ||
| reconnected = true | ||
| # A build failure here is surfaced by the terminal rescue below, so it never kills the worker. | ||
| client = reconnect_async_client(client) | ||
| retry | ||
| end | ||
| result << [:error, e] if result | ||
| # If we reconnected and still failed, drop `warm` so a sustained renderer outage stops | ||
| # paying a doomed reconnect+retry on every subsequent request on this thread. | ||
| return [client, reconnected ? false : warm] | ||
| end | ||
|
|
||
| # Response acquired: a drop while buffering is not retried (see method comment). Non-stale | ||
| # send errors and buffering failures both fall through to the terminal rescue below. `warm` | ||
| # is preserved: these paths either already obtained a response (connection was healthy) or | ||
| # failed before any reconnect. The failed-reconnect case that resets `warm` is handled above; | ||
| # a reconnect *build* failure (Async::HTTP::Client.new is lazy, so this is effectively | ||
| # unreachable) also lands here, self-limited by `reconnected` to a single attempt. | ||
| result << [:ok, buffer_response(raw_response)] | ||
| [client, true] | ||
| rescue StandardError => e | ||
| raise unless result | ||
|
|
||
| result << [:error, e] | ||
| [client, warm] | ||
| end |
There was a problem hiding this comment.
warm isn't reset if reconnect_async_client itself raises, undermining the "stop paying a doomed reconnect" guarantee.
The normal retry-failure path explicitly drops warm when a second attempt fails after reconnecting:
return [client, reconnected ? false : warm]But if reconnect_async_client(client) (line ~313) itself raises — e.g. build_async_client fails inside its own rescue (see the rebuilding the client during reconnect fails spec) — that exception isn't caught by the inner rescue *STALE_KEEPALIVE_RETRY_ERRORS clause (already past it) and instead falls through to the method-level rescue StandardError => e at the bottom. That branch returns [client, warm] using the original warm parameter, even though reconnected was already set to true beforehand. So warm stays true forever after a rebuild failure, and every subsequent request on that Puma thread will keep attempting (and failing) a reconnect instead of failing fast — the opposite of what the comment on this method promises ("a sustained renderer outage stops paying a doomed reconnect+retry on every subsequent request").
The added spec (surfaces the error without killing the worker when rebuilding the client during reconnect fails) only asserts that ConnectionError is raised on requests 2 and 3 — it doesn't assert on Async::HTTP::Client.new call counts, so it doesn't catch this. Worth either resetting warm to false in the outer rescue when reconnected is true, or asserting/fixing this explicitly.
This is low real-world impact since Async::HTTP::Client.new is lazy and rarely raises synchronously, but it's a genuine logic gap given a test was written specifically to cover this scenario.
ReviewOverviewThis PR fixes a real regression from #4394: the plain-Puma non-streaming path's per-thread persistent async-http client ( Code quality
Potential issue (see inline comment)
Other observations
🤖 Generated with Claude Code |
1 participant
Summary
Fixes the reliability + documentation regression #4571 introduced by PR #4394.
PR #4394 changed the plain-Puma (no
Fiber.scheduler) non-streaming renderer path from connect-per-request to a persistent async-http client per Puma request thread, keeping a renderer connection warm across requests on that thread. Two consequences:retries: 0, so reusing a keep-alive connection the renderer, a load balancer, or a proxy has since idle-closed fails hard (ECONNRESET/EPIPE/EOF) instead of transparently reconnecting. The old connect-per-request path never hit this.renderer_http_pool_sizeis now a per-client, not global, cap — total warm renderer capacity scales with the number of live Puma request threads. The config docs still described the no-scheduler path as "per-request".Fix (minimal, no new config)
PersistentThreadClientnow transparently reconnects and retries a request once when a warm client (≥1 prior success) hits a dropped-connection error on a replay-safe body (nilorString). Guards keep it surgical:MultipartBody) are never retried — they can't be re-sent after a partial write.ECONNREFUSED/EHOSTUNREACH/ENETUNREACH/ETIMEDOUT/SocketError) and HTTP/2 stream resets (Protocol::HTTP2::StreamError, a request-level abort) are excluded from the reconnect path and continue to flow to the existingrenderer_request_retry_limitloop unchanged.STALE_KEEPALIVE_RETRY_ERRORSset scopes the retry toIOError/EOFError,ECONNRESET,EPIPE, andProtocol::HTTP::RefusedError.renderer_http_pool_sizedocs (constant + setter,configuration-pro.md,updating.md) to describe the per-thread persistent client and clarify that total warm renderer capacity scales with live Puma request threads, not with the pool size alone. CHANGELOG entry added.Scope decision: global cap is doc-only
#4571 suggested bounding total warm connections independent of thread count. This PR intentionally handles that as documentation rather than new config. A cross-thread cap would either be a no-op default or an arbitrary new public config contract with a silent ephemeral-fallback policy — additional surface and risk not warranted for a regression fix. (async-http clients/connections are reactor/thread-bound, which is why per-thread clients exist in the first place.)
Testing
cd react_on_rails_pro && bundle exec rspec spec/react_on_rails_pro/renderer_http_client_spec.rb— 70 examples, 0 failures (10 new).Transport suite (
renderer_http_client_spec,request_spec,stream_request_spec,stream_spec) — 184 examples, 0 failures, 1 pending (pre-existing).New specs:
pool_limit).ECONNREFUSEDexcluded;StreamErrorexcluded.Also:
bundle exec rubocop ... --ignore-parent-exclusionclean,bundle exec rake rbs:validatepasses,git diff --checkclean, Prettier clean on changed docs.Process
Developed with Codex (gpt-5.5, xhigh) as peer reviewer at each step — reproduce, investigate, plan, implement, review, test — with agreement reached before advancing each step.
Fixes #4571.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
renderer_http_pool_sizeas a per-client connection limit and explained how warm capacity scales with active Puma threads, including different lifecycles for non-streaming vs streaming requests and updated retry expectations.