[Pro] Keep RSC payload retries inside one cached promise - #4564
Merged
Conversation
Contributor
WalkthroughRSC payload loading now caches one logical-load Promise, retries eligible browser failures once, retains terminal failures temporarily, and distinguishes HTTP and abort errors. Client integrations enable retries, server rendering disables them, and tests cover retry, retention, cache, and refetch behavior. ChangesRSC payload retry flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RSCProvider
participant getServerComponent
participant RSCProviderCache
RSCProvider->>RSCProviderCache: read cached logical-load Promise
RSCProvider->>getServerComponent: request RSC payload
getServerComponent-->>RSCProvider: return payload or classified error
RSCProvider->>getServerComponent: force one retry for eligible failure
getServerComponent-->>RSCProvider: return retry result
RSCProvider->>RSCProviderCache: retain or release cache pin
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
ihabadham
marked this pull request as ready for review
July 11, 2026 00:03
Collaborator
Author
|
+ci-status |
Contributor
size-limit report 📦
|
Contributor
CI StatusHead SHA: Only the required gate is active unless hosted CI is requested. |
ihabadham
added
ready-for-hosted-ci
![greptile-apps[bot]](https://avatars.github.com/in/867647?s=60&v=4){kind=small}
![claude[bot]](https://avatars.github.com/in/1236702?s=60&v=4){kind=small}
Contributor
Review: Keep RSC payload retries inside one cached promiseSummaryThis replaces the render-driven retry from #4562 with a single cached Promise per logical RSC payload load: one bounded internal retry (with Strengths
Findings (posted inline)
Other notes (not blocking)
|
Contributor
Review SummaryThis PR replaces the render-driven retry from #4562 with a single-cached-promise retry model in Overall this is a solid, well-reasoned fix — the request-amplification bug it targets (thousands of requests/sec from a render-driven retry loop) is real and the "one Promise owns the retry budget" design is the right shape. Test coverage is extensive (retry classification matrix, stale-refetch races, LRU-capacity protection, embedded-payload bypass). A couple of points worth double-checking before merge (left as inline comments):
Neither of these undermines the core fix (bounded request count), but they're worth a second look given how central the cache-bookkeeping invariants are to this file's correctness story. |
Contributor
|
Review summary Overview: this PR replaces the render-driven retry from #4562 with a single cached Promise per logical RSC payload load — Code quality
Test coverage
Risk / design tradeoffs (as documented by the author)
Overall: solid, well-tested fix for a real correctness bug (request amplification). The two inline comments are minor/non-blocking. |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
Collaborator
Author
Address-review summaryScan scope: full PR history; no prior summary. Mattered
Optional
Skipped
Follow-up issue: None. Next default scan starts after this comment. Say |
Contributor
Review: Keep RSC payload retries inside one cached promise (#4564)OverviewReplaces #4562's render-driven retry (evict the rejected promise, rely on React re-rendering to create a second attempt) with a single cached Promise per logical load that performs one internal retry ( Strengths
Issues
SecurityNo injection/XSS concerns — this only changes retry/caching control flow and error classification, not payload rendering or DOM injection paths. The PerformanceRetry is capped at exactly one extra request, matching the browser-verification table in the PR description (2 requests max for a persistently failing key, vs. thousands before the fix). Overhead per No blocking issues found; the one moderate item (SSR retry-disabled coverage) is worth addressing before/after merge but doesn't affect shipped runtime behavior. |
Contributor
Pro Node Renderer Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Collaborator
Author
Address-review summaryScan scope: activity after latest summary at 2026-07-11T00:30:27Z. Mattered
Optional
Skipped
Follow-up issue: None. Next default scan starts after this comment. Say |
Contributor
Core Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Contributor
Pro (shard 2/2) Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Contributor
Pro (shard 1/2) Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
This was referenced Jul 11, 2026
Member
QA Evidence
|
This was referenced Jul 11, 2026
justin808
added a commit
that referenced
this pull request
Jul 12, 2026
## Why Backport the RSC payload cache integrity fix from #4551 to the active `17.0.0` release train. Without this fix, a subsequent cache hit can return an empty RSC payload after the original response stream has been consumed. Fixes #4566. ## Source and provenance - Source PR: #4551 - Source merge commit: `176dc8ae044ceb6f27a39fcf8482421b56a90816` - Backport commit: `2bd78492a309c4d505788f575487e63b66d60b58` - Base at backport time: `release/17.0.0` at `3acfc4425225951725d748962eeaff88107602eb` - Stable patch ID for the five code/test files: `12f1074adc9887feb72a27ddc549987708928b5d` The five Pro code/test changes are byte-for-byte equivalent to the source change. The only cherry-pick conflict was `CHANGELOG.md`; it was resolved by adding the #4551 entry once beneath the existing Unreleased `Fixed` heading while leaving stamped `17.0.0.rc.8` content unchanged. Unrelated mainline changes were excluded. ## Changes - Preserve RSC stream cache content so repeated cache reads return the complete payload. - Exercise the cache implementation, helper behavior, and real-renderer request path. - Add the release-train changelog entry. ## Verification - `react_on_rails_pro/spec/react_on_rails_pro/stream_cache_spec.rb`: 5 examples, 0 failures - Dummy test bundle built successfully - Helper and RSC payload request specs with the real renderer: 121 examples, 0 failures - Full Pro RuboCop: 241 files, 0 offenses - License headers: 844 files checked - Prettier check: clean - Repo-compatible Lychee 0.23.0: 0 link errors - Pre-push hook: branch RuboCop and online Markdown links passed ## QA Evidence Independent QA checked out the exact candidate commit `2bd78492a309c4d505788f575487e63b66d60b58` and confirmed: - Exact live base, head, source commit, and cherry-pick provenance - Identical source/backport blobs for all five code/test files and identical stable patch IDs - Unchanged stamped changelog section - All focused specs, real-renderer integration specs, RuboCop, license, and detector checks passed - No release-blocking findings; renderer process stopped and its port was clear after testing ## Review evidence - Independent self-review: clean - Private Claude Opus 4.8 review: no blocking, discuss, or follow-up findings - Read-only simplify pass: no changes recommended - High-risk adversarial review: no code/provenance blockers; one release-order discussion was dispositioned below - The configured local Codex review model could not run because the installed CLI does not support `gpt-5.6-sol`; no code was changed as a result ## Release sequencing decision Draft PR #4588 stamped rc.9 before this backport existed. It remains intentionally outside this backport and must not merge first. The chosen sequence is: 1. Merge this exact-source #4551 backport. 2. Build and merge the serialized #4564 backport from the updated release branch. 3. Rebase or regenerate #4588 afterward so both backport entries are included in the rc.9 stamped section. This disposes the adversarial review's changelog-order concern without changing the code payload or touching #4588 in this PR. ## Release notes - Review churn: one changelog conflict, resolved as described above; no other deviations from the source PR - Human decision point: final merge authorization remains with the maintainer - Merge authority remains with the maintainer Co-authored-by: Abanoub Ghadban <abanoub@shakacode.com>
This was referenced Jul 12, 2026
justin808
added a commit
that referenced
this pull request
Jul 12, 2026
## Why Backports merged source PR #4564 to the 17.0.0 release train as its own source-atomic release PR. This prevents failed RSC payloads from letting React renders repeatedly reopen the browser request budget. Source: - PR: #4564 - merged commit: `9d470c475e3670e2bfa45b7b15e336a2e838e5db` - backport commit: `01199896eab5d8ef7b3629b1dc0883089bdd8437` - dependency already on the release branch: #4589 / `c16b4bdee` ## Backport method - branched from the current `release/17.0.0` tip after #4589 landed - used `git cherry-pick -x` - retained the source PR's changelog entry under `Unreleased -> Fixed` - the normalized zero-context patch for all 11 Pro runtime/test files has the same SHA-256 on source and backport: `670bb3906a8ee1e2e9c4ce6cb99aa7749e4033f1e7855af07266d84507cbf747` - no other source PR is included ## Validation - focused affected tests: 5 suites, 136 tests passed - full React on Rails Pro package suite: - non-RSC: 421 tests passed - streaming: 134 tests passed - RSC: 23 tests passed - TypeScript type-check - ESLint and Prettier - Lychee offline and pre-push online link checks - branch-wide Ruby lint - `git diff --check` - all 845 in-scope Pro license headers - high-risk adversarial review: no blocking code concern; request ceiling, retry/cache/refetch races, SSR/browser gating, pin accounting, release compatibility, and #4589 dependency were checked ## Review decision carried from the source PR The adversarial pass surfaced the intentional server behavior: official server rendering disables browser retries and reuses the same failed Promise for a same-key lookup. That behavior was explicitly documented and tested in #4564, then accepted when the source PR was merged; this backport preserves it exactly. ## Remaining gates - optimized hosted CI and benchmarks on this release-branch head - current-head review agents and thread triage - release-branch merge ledger <!-- qa-evidence v1 required: yes status: satisfied head_sha: 0119989 tested_at: local release backport head; source PR base-red/head-green replay scope: Pro RSC retry cache refetch SSR behavior and release-base compatibility automated_checks: 136 focused tests; 578 full Pro package tests; type-check; lint; formatting; link checks; license headers manual_checks: normalized source/backport patch equivalence and changelog conflict inspection findings: no blocking code concern release_blocking: clear pending hosted CI and current-head reviews process_gap_disposition: handled by source-atomic backport workflow PRs --> Co-authored-by: Ihab Adham <71561048+ihabadham@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Alternative implementation to #4562 for the client-side request amplifier reported in shakacode/react_on_rails_rsc#187.
This PR and #4562 solve the same client-side problem and are mutually exclusive. This implementation is intended to replace #4562’s render-driven retry architecture, not build on top of it.
Relationship to #4562
#4562 handles retries by deleting the Promise for attempt one and relying on React’s retry render to create attempt two:
This PR keeps one Promise for the complete logical load:
The correctness distinction is ownership: the retry budget belongs to the logical payload load, not to React renders. With one Promise, React scheduling cannot change the number of requests, and stale attempts, cache eviction, and explicit refetch do not need to synchronize with a separate attempt record.
This also changes the behavior of the retry itself. #4562’s retry re-enters the ordinary
getComponentpath, so malformed embedded Flight data may be read a second time without making an HTTP request. This implementation retries withenforceRefetch: true, bypassing the failed embedded value and requesting a fresh payload.Controlled browser comparison:
Implementation
RSCProvidernow caches one Promise for the complete logical browser load:The automatic retry uses
enforceRefetch: true, ensuring a failed embedded or prefetched payload is bypassed in favor of a fresh HTTP request.Retry policy:
AbortError.window.Final failures remain pinned during the five-second retention window so ordinary LRU pressure cannot erase the request limit. This means a high-cardinality outage may temporarily exceed the normal 50-entry cache limit; those entries are removed and unpinned when retention ends.
Why one Promise?
The retry budget belongs to the logical payload load, not to React renders.
React always receives the same cached Promise regardless of how often it rerenders. React scheduling therefore cannot create additional attempts, and explicit refetches or stale operations cannot corrupt the current load’s ownership.
This differs from a render-driven retry, where attempt one is deleted and a later React render creates attempt two, requiring separate state to reconnect both Promises.
Browser verification
windowshimValidation
CI labels
Labels:
ready-for-hosted-ci,benchmark— this changes Pro RSC cache and retry behavior under concurrency and cache pressure, so it needs optimized hosted confirmation plus the Pro benchmark route.Summary by CodeRabbit
Merge qualification
developmentfrom release tracker Release gate: react_on_rails 17.0.0 #3823; targetmainis beta phase. The standard beta gate is satisfied.5d895d25388a5cf55ee903a65c2138c991142750.pr-ci-readinessisREADYwith the required gate in use; optimized hosted CI, Pro package/integration jobs, bundle-size checks, CodeQL, and all four benchmark suites completed successfully for this head. Selector skips are explained byscript/ci-changes-detector origin/main.complete_allowed: truewithchangelog_presentand noUNKNOWNfields.b2eb62ee4967de17ad56f7dfe65eff569b7732e5; it is degraded for the final head and is not cited as a merge gate.f45df4d6c082caf0e2e064256bbcd63af623e88dwith 3 producer calls instead of 2, then passed on this head with exactly 2 calls,enforceRefetch: trueon attempt two, and later lookups reusing the original rejected Promise.Confidence note:
git diff --check; deterministic base-red/head-green replay; hosted CI and benchmarks.script/pr-merge-ledger 4564 --changelog-classification changelog_present --strict; QA replay at the merge-base and current head above.Codex Decision Log
AbortError, for the bounded retention window.