feat: rolling_deploy_adapter for seeding previous bundle hashes#3173
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements a rolling-deploy adapter protocol and tooling to pre-seed Node renderer caches: adds adapter configuration and validation, RendererCacheHelpers updates, a RollingDeployCacheStager to fetch/stage previous bundle hashes, publishing hooks in assets precompile, doctor diagnostics, docs, and comprehensive specs. ChangesRolling Deploy Adapter & Cache Seeding Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 9cd1657f83
ℹ️ 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[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Greptile SummaryThis PR introduces
Confidence Score: 4/5Safe to merge after addressing the nil-adapter + PREVIOUS_BUNDLE_HASHES early-return bug; remaining findings are style/test-coverage. One P1 defect: the early-return guard allows a nil-adapter path that silently fails with NoMethodError and produces a misleading doctor message. This affects any operator who sets PREVIOUS_BUNDLE_HASHES without an adapter, and it is untested. The P2 absolute-symlink inconsistency is low-risk for typical same-filesystem deploys. All other pieces — configuration validation, graceful degradation, doctor probes, deduplication logic — are well-implemented and tested. react_on_rails_pro/lib/react_on_rails_pro/rolling_deploy_cache_stager.rb (early-return guard + symlink mode), react_on_rails/lib/react_on_rails/doctor.rb (misleading 'env override will be used' message) Important Files Changed
Sequence DiagramsequenceDiagram
participant AP as AssetsPrecompile.call
participant PS as PreSeedRendererCache.call
participant RD as RollingDeployCacheStager.call
participant DA as rolling_deploy_adapter
AP->>PS: call(mode: :symlink)
PS->>PS: stage current bundle hashes
PS->>RD: call(cache_dir:, current_hashes:, mode:)
alt adapter nil AND env empty
RD-->>PS: return (no-op)
else adapter present OR PREVIOUS_BUNDLE_HASHES set
RD->>DA: previous_bundle_hashes() [or ENV override]
DA-->>RD: ["abc", "def", ...]
RD->>RD: deduplicate against current_hashes
loop each previous hash
RD->>DA: fetch(hash)
DA-->>RD: {server_bundle:, rsc_bundle:, assets:} or nil
RD->>RD: stage_file(src, dest, mode)
end
end
AP->>AP: publish_current_bundle_if_configured
AP->>DA: upload(current_hash, server_bundle:, rsc_bundle:, assets:)
Reviews (1): Last reviewed commit: "feat: add rolling_deploy_adapter for see..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Code ReviewThis is a well-structured PR with good overall design — the adapter protocol is clear, error handling degrades gracefully at every layer, test coverage is solid, and the docs are thorough. A few issues worth addressing before merge: Bugs1. Fix: add an explicit guard after hash resolution when adapter is nil, or document that Security2. Shell injection in ControlPlane reference implementation Correctness / Reliability3. 4. No timeout on 5. Race condition in S3 Style / Idioms6. 7. Minor
🤖 Generated with Claude Code |
MUST-FIX (2):
- **Nil adapter + PREVIOUS_BUNDLE_HASHES bug** (greptile P1, claude):
when the env override was set without config.rolling_deploy_adapter,
the stager proceeded past the early-return guard and crashed with
NoMethodError at adapter.fetch. Now warns and returns instead.
Doctor promotes the corresponding check from :info to :warning and
explains that both env and adapter are required.
- **RSC previous-bundle staging path** (codex P2): the renderer routes
RSC requests by rsc_bundle_hash, which differs from server_bundle_hash
and is a separate <cache>/<hash>/<hash>.js entry. The original
protocol merged both bundles under a single hash's directory using
basenames, so the RSC bundle never landed where the renderer looks.
Fix: protocol refactor — each hash is now one bundle's cache entry.
fetch(hash) → { bundle: path, assets: [paths] }
upload(hash, bundle:, assets:)
previous_bundle_hashes → flat list including server AND rsc hashes
AssetsPrecompile.publish_current_bundle_if_configured now calls
upload twice when RSC is enabled (once per hash). Stager stages each
hash at <cache>/<hash>/<hash>.js with its companion assets.
SHOULD-FIX (4):
- **Timeout on adapter.fetch** (claude): wrap with Timeout.timeout
(default 30s) so a hung external store can't block pre-seeding or
assets:precompile indefinitely.
- **Relative symlinks** (greptile P2): stage_file in :symlink mode now
uses the same Pathname#realpath + relative_path_from pattern as
PreSeedRendererCache.make_relative_symlink, matching current-hash
staging and surviving cache-dir moves.
- **Remove stray module_function** (claude): was paired with
private_class_method (an unusual combination that leaks private
instance methods to any include-r). Switched to `def self.` for all
methods, private_class_method for helpers.
- **respond_to? instead of methods.include?** (claude): more idiomatic
duck-typing in both Configuration.validate_rolling_deploy_adapter
and Doctor.report_adapter_protocol.
DOC POLISH (3):
- Control Plane reference impl: switched from backtick/system shell
interpolation to Open3.capture2e with array-form args to avoid
shell injection via env-var contents. Returns both server AND RSC
hashes via previous_bundle_hashes.
- Both S3 and Control Plane examples: lazy env-var accessor methods
instead of constants evaluated at require time, so KeyError can't
fire at class-load in environments that don't configure the adapter.
- S3 reference impl: explicit note about the read-modify-write race
in update_manifest!; documents serializing deploys or using
If-Match/ETag as strict-safety alternatives.
- Updated protocol docstrings + all reference impls (S3, Control Plane,
Filesystem) to the new fetch/upload signatures.
Tests: 13 rolling-deploy specs now (was 7) — adds nil-adapter+env
warning, fetch timeout, relative-symlink assertion. Doctor spec
updated for the warn-vs-info change. 38 Pro specs pass, 167/168
doctor specs pass (1 pre-existing unrelated failure).
Refs: #3122, #3167 (PR B), #3173 (this PR)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addressed review feedback in 2e9b3f8MUST-FIX (2):
SHOULD-FIX (4):
DOC POLISH (3):
Tests: 13 rolling-deploy specs (was 7) — adds nil-adapter+env warning, fetch timeout, relative-symlink assertion. Doctor spec updated for the warn-vs-info change. 38 Pro specs pass, 167/168 doctor specs pass (1 pre-existing unrelated failure in `auto_fix_versions`, unchanged). I (Claude Code) addressed the feedback on behalf of @justin808. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Code Review — PR #3173:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e9b3f8e58
ℹ️ 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".
Post-revision review surfaced 7 new actionable items. All fixed. Code: 1. PREVIOUS_BUNDLE_HASHES path-traversal (claude): env values are used as directory names under the renderer cache; a malicious or mistyped value like "../../../etc" would escape the cache dir. Now reject anything that doesn't match /\A[A-Za-z0-9_\-.]+\z/ with a warning. 2. node_renderer? guard in publish_current_bundle_if_configured (claude): returns early when ExecJS is configured rather than crashing on NodeRenderingPool.server_bundle_hash. Mirrors the guard used by PreSeedRendererCache. 5. Top-level rescue in publish_current_bundle_if_configured (cursor Medium): collect_assets / server_bundle_hash / rsc_bundle_js_file_path can all raise from the setup code that ran before the per-upload rescue. Extracted publish_bundles helper and wrapped the whole publication path with StandardError rescue so a failure degrades next-deploy seeding but doesn't fail this deploy's assets:precompile. 7. Partial-staging cleanup on asset failure (codex P1): previously if stage_file succeeded on the bundle but failed on an asset, the cache held a bundle file without its companion manifests. The renderer would then find the bundle and serve without those manifests, causing hydration breakage instead of the clean 410 retry fallback. Now rm_rf the hash directory on any failure inside seed_previous_hash — restores the 410-retry safety net. Docs: 3. Control Plane fetch example: replaced Dir[tmp/*.json] with an explicit allowlist so stray JSON metadata in the image layer isn't silently picked up. 4. S3 client mutex: @s3 ||= is not thread-safe under concurrent callers (parallel workers running precompile hooks). Example now uses a Mutex-synchronized memoizer with a doc comment. 6. S3 manifest RETENTION alignment: write and read both trim to RETENTION now, so the persisted manifest matches what discovery returns. Added a comment explaining the intent. Tests: 40 rolling-deploy + pre-seed + shim specs (was 38, +2 for path-traversal reject and partial-staging cleanup). Rubocop clean. Refs: #3122, #3173 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addressed post-revision review feedback in fb3de1bSeven new actionable items since `2e9b3f8e5` — all fixed. None skipped. Security / correctness:
Docs:
Tests: 40 specs pass (was 38; +2 for path-traversal reject + partial-staging cleanup). Rubocop clean. Pre-commit hooks pass. I (Claude Code) addressed the feedback on behalf of @justin808. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Code Review: PR 3173 rolling_deploy_adapter - see inline comments for specifics. Overview: Solid PR completing the rolling-deploy trilogy. Graceful-degradation design is well-executed — every failure path (timeout, nil, raise) falls back to 410-retry, so the adapter can never break a deploy. Strengths: Airtight graceful degradation; defense-in-depth path traversal protection; atomic two-mv staging with concurrent-writer detection; upload signature validation at configure-time; thorough reference implementations. Six issues flagged via inline comments: (1) missing blank line after private_class_method block in assets_precompile.rb:218-219; (2) SAFE_HASH_PATTERN allows leading-dot hashes that create hidden cache dirs; (3) raw regex source in operator warning is not human-readable; (4) display_label.downcase produces 'rsc' lowercase mid-sentence in RSC warning; (5) global Metrics/ModuleLength bump should be a file-level disable; (6) splat-style upload(*args) accepted but undocumented for adapter authors. Minor: rolling_deploy_discovery_timeout_seconds fallback could reference the exact Pro spec path; stub_synchronizer helper refactor in doctor_spec.rb is a clean improvement. |
assets_precompile.rb: - Defer pool.server_bundle_hash / pool.rsc_bundle_hash evaluation behind a block in publish_bundle_if_present so a missing bundle file no longer raises from bundle_hash (File.mtime / Digest::MD5.file) and bypasses the per-bundle "does not exist" warning. Added regression spec. - Use the caller's original bundle_label (not display_label.downcase) in the missing-bundle warning so "RSC bundle ... for RSC bundle" reads consistently instead of mixing cases. - Added blank line between private_class_method list and the next def. rolling_deploy_cache_stager.rb: - SAFE_HASH_PATTERN now rejects any leading-dot hash (`.hidden`, `.git`) in addition to `.` and `..`. Path traversal was already prevented by start_with?, but a leading-dot hash would silently create a hidden cache subdirectory. Added regression spec. - Replaced the raw SAFE_HASH_PATTERN.source dump in the sanitize_hashes warning with a plain-English description that operators can act on. - Backup the bundle_dir entry when it's a dangling symlink, not just when File.exist? reports true, so restore_previous_bundle_directory has something to restore if promotion fails after the first move. - Documented Timeout.timeout's quasi-random interrupt point and the platform divergence (Linux nests staging-dir / macOS+BSD raise ENOTEMPTY) inline so future maintainers don't think the nested-dir check is the only detection path. - Documented STALE_TEMP_DIR_TTL_SECONDS so operators on slow builds can tune it. configuration.rb: - validate_rolling_deploy_upload_signature now points adapter authors at the splat/options-hash variants too, so a `def upload(*args)` author knows the keyword pair lands inside args. docs/pro/rolling-deploy-adapters.md: - Quantified S3 cost note: 2x GetObject + 2x PutObject per deploy when RSC is enabled (vs. the prior generic "two cycles" phrasing). - Documented splat/options-hash upload signature variants alongside the explicit form. .rubocop.yml: - Reverted the global Metrics/ModuleLength bump back to 180. The rolling_deploy_cache_stager.rb file-level `# rubocop:disable Metrics/ModuleLength` already covers the one over-limit case, so the global bump was silently relaxing the rule for every other module. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
|
||
| def self.publish_bundle_if_present(adapter, bundle, assets, bundle_label) | ||
| if File.file?(bundle) | ||
| publish_bundle(adapter, yield, bundle, assets, bundle_label) |
There was a problem hiding this comment.
The implicit yield here means callers must pass a block, but the method signature doesn't declare &block, so this contract isn't visible from the definition — only from the call sites at lines 121 and 126.
When the file doesn't exist the block is silently not called, which is intentional for deferred hash computation. But a future caller who reads only the signature could pass the method without a block, gets LocalJumpError at runtime (only in the file-present branch), and has to trace back to understand why.
Consider using an explicit &block parameter:
| publish_bundle(adapter, yield, bundle, assets, bundle_label) | |
| publish_bundle(adapter, block.call, bundle, assets, bundle_label) |
and declare the method as def self.publish_bundle_if_present(adapter, bundle, assets, bundle_label, &block). The &block makes the requirement visible in signatures, method docs, and IDEs.
|
|
||
| required_positionals = params.count { |type, _name| type == :req } | ||
| optional_positionals = params.count { |type, _name| type == :opt } | ||
| required_positionals == 1 && optional_positionals.positive? |
There was a problem hiding this comment.
This branch accepts upload(hash, options = {}) — but in Ruby 3.0+ keyword arguments are strictly separated from positional hashes. A call like adapter.upload(hash, bundle: bundle, assets: assets) will not populate options; Ruby 3 raises ArgumentError: unknown keywords: bundle, assets instead.
The comment above correctly documents the Ruby 3 rule for *args, but the positional optional case (opt param kind) falls through to this return true without the same guard. An adapter author who writes def self.upload(hash, opts = {}) will pass validation here, deploy to a Ruby 3 app, and get a runtime ArgumentError on the first assets:precompile — a confusing failure mode given that configure accepted the adapter cleanly.
Suggestion: either reject this shape (add || optional_positionals.positive? to the uses_explicit_upload_keywords? guard), or add an explicit config-time warning that upload(hash, opts = {}) only works on Ruby 2.7 and below.
Review round 10 — address-review summaryPosted by me (Claude Code) after running Addressed in code (12 items)
Already addressed in prior commits — replied + resolved (19 items)These threads carry feedback that was silently fixed by subsequent rounds (≤ ba13457) but never closed out. Verified against current code, replied with the resolution, and resolved:
Verification
🤖 Generated with Claude Code |
|
|
||
| if adapter.nil? | ||
| env_override = ENV.fetch("PREVIOUS_BUNDLE_HASHES", nil) | ||
| if env_override && !env_override.empty? |
There was a problem hiding this comment.
Minor inconsistency with the stager: handle_missing_adapter in rolling_deploy_cache_stager.rb does ENV["PREVIOUS_BUNDLE_HASHES"].to_s.strip and then checks emptiness, so a value like " " (spaces only) is treated as unset. Here the check is !env_override.empty?, which would consider " " non-empty and fire the warning — a false positive.
The mismatch is cosmetic in practice (an all-whitespace env var is likely accidental regardless), but aligning the two checks avoids confusing discrepancy:
| if env_override && !env_override.empty? | |
| if env_override && !env_override.strip.empty? |
| return unless report_adapter_protocol(adapter) | ||
|
|
||
| env_override = ENV.fetch("PREVIOUS_BUNDLE_HASHES", nil) | ||
| if env_override && !env_override.empty? |
There was a problem hiding this comment.
Same whitespace inconsistency as the check at line 2889 — aligning with the stager's .strip avoids a false-positive "skipping probe" info message for an all-whitespace value:
| if env_override && !env_override.empty? | |
| if env_override && !env_override.strip.empty? |
|
To use Codex here, create an environment for this repo. |
|
|
||
| PREFIX = "bundles" | ||
| MANIFEST_KEY = "#{PREFIX}/_manifest.json".freeze | ||
| RETENTION = 6 # keep last ~3 deploys' worth (2 hashes per deploy when RSC is enabled) |
There was a problem hiding this comment.
The comment "~3 deploys' worth (2 hashes per deploy when RSC is enabled)" is accurate for RSC apps, but the same RETENTION = 6 value is used unchanged for non-RSC apps — where each deploy produces only 1 hash. In that case RETENTION = 6 keeps the last 6 deploys, not 3.
Since most teams only need to cover draining traffic from the previous 1–2 deploys, an operator who copy-pastes this for a non-RSC app will silently retain far more history than intended.
Consider either:
- Documenting both cases explicitly:
RETENTION = 6 # ~3 deploys with RSC (2 hashes/deploy), or ~6 deploys without RSC (1 hash/deploy), or - Making retention configurable with a smaller default for non-RSC (e.g.
RETENTION = enable_rsc ? 6 : 2).
|
Code review posted with inline comments on 4 specific issues. Summary: (1) Ruby 3 incompatibility in upload signature validation - accepts_upload_options_hash allows upload(hash, opts={}) which fails at runtime on Ruby 3+. (2) Implicit yield without &block declaration in publish_bundle_if_present. (3) PREVIOUS_BUNDLE_HASHES whitespace check inconsistency between doctor.rb and stager. (4) RETENTION=6 doc comment misleading for non-RSC apps. |
* origin/main: docs: explain responseEnd/action_total range overlap for Gumroad RSC benchmark (#3340) chore(ci): upgrade k6 benchmarks to v2.0.0 (#3319) (#3338) docs: add resolve.alias alternative for legacy Webpacker shim (#3252) (#3330) fix: scan .mts and .cts in Pro migration base-package check (#3250) (#3334) Bump version to 16.7.0.rc.0 Update CHANGELOG.md for 16.7.0.rc.0 (#3336) feat: rolling_deploy_adapter for seeding previous bundle hashes (#3173) docs: capture Gumroad benchmark JS transfer-size artifacts (#3259) (#3333) [codex] Document node renderer health probes (#3241) ci(lychee): exclude flyhomes.com from markdown link check (#3327) feat(doctor): expand renderer-cache scan to CI/CD manifests (#3329)
…y-patch-DrhDk-v1 * origin/main: (26 commits) docs: consolidate node renderer probe settings (#3257) (#3348) chore(ci): remove obsolete setup-node-with-retry action (#3352) chore: remove completed planning/analysis docs (#3361) test: stub stable VERSION in --pro install spec to unblock CI (#3363) docs(specs): add RC testing plan design spec (#3350) docs: point users at bin/shakapacker-config --doctor for bundler-config debugging (#3359) docs: polish RSC node renderer test recipe (#3273) (#3339) test: replace call_count stub pattern with and_return sequence (#2157) (#3353) docs: call out Inertia's incompatibility with client-side routers (#3356) docs: replace personal email with contact URL in Pro README (#3347) docs: plan performance test expansion (#3238) docs: explain responseEnd/action_total range overlap for Gumroad RSC benchmark (#3340) chore(ci): upgrade k6 benchmarks to v2.0.0 (#3319) (#3338) docs: add resolve.alias alternative for legacy Webpacker shim (#3252) (#3330) fix: scan .mts and .cts in Pro migration base-package check (#3250) (#3334) Bump version to 16.7.0.rc.0 Update CHANGELOG.md for 16.7.0.rc.0 (#3336) feat: rolling_deploy_adapter for seeding previous bundle hashes (#3173) docs: capture Gumroad benchmark JS transfer-size artifacts (#3259) (#3333) [codex] Document node renderer health probes (#3241) ... # Conflicts: # PROJECTS.md
Part 1 of #3240 (zero-infra rolling-deploy default). Adds the client-side adapter, server-side controller, and config plumbing. Follow-up PRs add routes auto-mount, specs, and docs. What lands: - ReactOnRailsPro::RollingDeployAdapters::Http — built-in adapter that fetches previous bundle hashes over authenticated HTTP. Implements the rolling_deploy_adapter protocol from #3173: previous_bundle_hashes hits /manifest, fetch(hash) downloads + extracts a gzipped tarball into tmp/rolling-deploy/<hash>/, upload is a no-op (the running Rails server IS the artifact store). - ReactOnRailsPro::RollingDeploy::BundlesController — mountable controller serving GET /manifest (current deployment's bundle hashes) and GET /bundles/:hash (gzipped tarball of bundle + companion assets). Constant-time bearer-token auth. Hash allowlist prevents path traversal by construction. Cache-Control: no-store on every response. - ReactOnRailsPro::RollingDeploy::Tarball — stdlib-only tar.gz compose/extract with size cap, path-safety guard (basename only, no leading dot), and regular-files-only enforcement to defeat zip-bomb / symlink-traversal payloads. - Configuration: config.rolling_deploy_token (required when adapter is Http) config.rolling_deploy_previous_url (where to fetch from) config.rolling_deploy_mount_path (default: /react_on_rails_pro/rolling_deploy) Token validated at configure time: required + min 32 chars when the Http adapter is selected. Custom adapters re-using the same knobs are not forced through this validation. Still TODO in follow-up PRs on this branch: - Routes auto-mount via engine initializer (currently controller is defined but not wired into the route table). - Adapter + controller specs. - docs/pro/rolling-deploy-http-adapter.md setup walkthrough. - CHANGELOG entry once the feature lands end-to-end. - Rate limiting, RENDERER_PASSWORD fallback, doctor probes (deferred to PR 2 per agreed split). Refs #3240 Builds on #3173 (rolling_deploy_adapter protocol) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Part 1 of #3240 (zero-infra rolling-deploy default). Adds the client-side adapter, server-side controller, and config plumbing. Follow-up PRs add routes auto-mount, specs, and docs. What lands: - ReactOnRailsPro::RollingDeployAdapters::Http — built-in adapter that fetches previous bundle hashes over authenticated HTTP. Implements the rolling_deploy_adapter protocol from #3173: previous_bundle_hashes hits /manifest, fetch(hash) downloads + extracts a gzipped tarball into tmp/rolling-deploy/<hash>/, upload is a no-op (the running Rails server IS the artifact store). - ReactOnRailsPro::RollingDeploy::BundlesController — mountable controller serving GET /manifest (current deployment's bundle hashes) and GET /bundles/:hash (gzipped tarball of bundle + companion assets). Constant-time bearer-token auth. Hash allowlist prevents path traversal by construction. Cache-Control: no-store on every response. - ReactOnRailsPro::RollingDeploy::Tarball — stdlib-only tar.gz compose/extract with size cap, path-safety guard (basename only, no leading dot), and regular-files-only enforcement to defeat zip-bomb / symlink-traversal payloads. - Configuration: config.rolling_deploy_token (required when adapter is Http) config.rolling_deploy_previous_url (where to fetch from) config.rolling_deploy_mount_path (default: /react_on_rails_pro/rolling_deploy) Token validated at configure time: required + min 32 chars when the Http adapter is selected. Custom adapters re-using the same knobs are not forced through this validation. Still TODO in follow-up PRs on this branch: - Routes auto-mount via engine initializer (currently controller is defined but not wired into the route table). - Adapter + controller specs. - docs/pro/rolling-deploy-http-adapter.md setup walkthrough. - CHANGELOG entry once the feature lands end-to-end. - Rate limiting, RENDERER_PASSWORD fallback, doctor probes (deferred to PR 2 per agreed split). Refs #3240 Builds on #3173 (rolling_deploy_adapter protocol) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…3379) ### Summary Part 1 of #3240 — scaffolds the built-in HTTP rolling-deploy adapter (zero-infra default for the `rolling_deploy_adapter` protocol from #3173). Lands the client-side `ReactOnRailsPro::RollingDeployAdapters::Http` adapter, the server-side `ReactOnRailsPro::RollingDeploy::BundlesController` (`GET /manifest` + `GET /bundles/:hash`), a stdlib-only `Tarball` compose/extract helper, and three new config options (`rolling_deploy_token`, `rolling_deploy_previous_url`, `rolling_deploy_mount_path`) with required-and-≥32-byte token validation at configure time. Security model: constant-time bearer-token auth (length leak documented inline, ≥32 bytes enforced by the validator), hash allowlist (path-traversal proof by construction), `Cache-Control: no-store`, tarball entries must be flat basenames + regular files, uncompressed-size cap defeats zip-bomb payloads. Refs #3240, builds on #3173. ### Latest update — review round 4 (e6a3a04) Worked through the 26 open review threads. Highlights: - **Correctness**: `gz.finish` now wrapped in `rescue` (FD hygiene on the exception path); `ENTRY_NAME_PATTERN` dropped a redundant `(?!\.)` lookahead and its inaccurate "stricter" comment was rewritten; config validator switched from `.length` to `.bytesize` to match the auth-path constant-time check. - **Operator UX**: warn-log when `node_renderer?` is false; loopback hosts now exempt from the plain-HTTP cleartext-token warning so local-dev rehearsals stay quiet. - **Tests**: 11 new examples for `#authenticate_rolling_deploy_request` — the previously-untested security-critical path — plus a loopback-host lock-in in `http_spec.rb`. All 106 relevant local specs pass; rubocop and RBS clean. - **CHANGELOG**: `Added` entry for the new adapter, and a `Changed` entry documenting that the shared `SAFE_HASH_PATTERN` now rejects bundle hashes starting with `-` (a tightening over the previous local pattern in `RollingDeployCacheStager`; no-op for webpack content hashes). - **Discuss item resolution**: `bytesize` pre-`secure_compare` gate kept, with an expanded comment explaining why the leak is bounded to token length. The SHA256-digest alternative remains an option if a stronger constant-time path is preferred in a follow-up. 5 review comments were rationale-replied + resolved as already-addressed / not-applicable. All 26 open threads are now resolved. ### Pull Request checklist - [x] Add/update test to cover these changes - [x] Update documentation - [x] Update CHANGELOG file ### Other Information Per the agreed PR split, **the hard HTTPS-only gate, the `RENDERER_PASSWORD` fallback, streaming download (currently `response.body` buffers up to 200 MB), and doctor probes remain deferred to follow-ups** — orthogonal to the "did we solve the zero-infra problem" question this PR answers. The plain-HTTP token warning in this PR is the bridging guardrail. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Introduces authenticated HTTP exposure of server bundles and tarball parsing; mitigations are strong but misconfiguration (token, HTTP in prod) or custom hyphen-prefixed hashes could affect deploy seeding. > > **Overview** > Adds a **zero-infra HTTP rolling-deploy path** so build CI can seed the Node renderer from the **currently running Rails app** instead of S3 or a custom adapter. > > **Pro** gains `ReactOnRailsPro::RollingDeployAdapters::Http` (manifest + per-hash tarball fetch, warn-and-degrade on failure) and `ReactOnRailsPro::RollingDeploy::BundlesController` with bearer auth (`secure_compare`, ≥32-byte token enforced at boot), hash allowlisting, `Cache-Control: no-store`, and gzipped tarballs built via a new stdlib **`RollingDeploy::Tarball`** helper (flat basenames, regular files only, 200 MB uncompressed cap). Config adds `rolling_deploy_token`, `rolling_deploy_previous_url`, and `rolling_deploy_mount_path`; `rolling_deploy_http_adapter?` gates HTTP-specific validation. Routes are exposed through `BundlesController.draw_routes` (engine auto-mount is described as follow-up on the branch). > > `RollingDeployCacheStager` now uses shared **`SAFE_HASH_PATTERN`**, which also **rejects leading-hyphen hashes** (upgrade note in CHANGELOG). Specs cover auth, tarball companions, adapter HTTP/TLS warnings, and manifest sanitization. Part 1 of a series—HTTPS-only enforcement, streaming, and engine mount hardening are called out as deferred. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit e6a3a04. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
Adds a pluggable
config.rolling_deploy_adapterprotocol that eliminates 410→retry cold-start round-trips for previously-deployed bundle hashes during rolling deploys — complementing the current-hash seeding already handled by PRs #3124 + #3167.Stacked on #3167 (PR B). Target base is `jg/3122-unify-renderer-cache-staging`; will rebase onto `master` once PRs A + B merge. Refs #3122. Design doc previously shared: see earlier session context.
Why
PR A seeded the current hash. PR B unified copy/symlink staging. But during a rolling deploy:
Pre-seeding only the current hash leaves `abc` cold on new renderers → every draining-version request hits the 410 retry path. This PR closes that gap.
Protocol
Parallel in shape to `remote_bundle_cache_adapter` — same duck-typed module pattern, same validation at configure time.
The loadable-stats wrinkle
Each bundle hash must carry its own companion `loadable-stats.json` and RSC manifests (built in lockstep with that bundle). Otherwise the renderer emits HTML referencing chunk URLs for the wrong build → client-side hydration breakage. The `fetch` signature returns bundle + assets together to force callers to think about this; see `docs/pro/rolling-deploy-adapters.md` for the full discussion.
Integration points
Error handling — degrades gracefully
Runtime 410-retry remains the fallback for any seeding failure, so rolling-deploy seeding can never cause a correctness regression:
Doctor (`react_on_rails:doctor`) probes
Doctor never calls `fetch` or `upload` — those have side effects.
Docs
Test plan
Relationship to other adapters
Complementary — configure both if useful.
🤖 Generated with Claude Code
Note
Medium Risk
Touches Pro deploy-time asset publication and renderer-cache staging paths (
assets:precompileandPreSeedRendererCache), which can impact production deploy behavior if misconfigured. Changes are largely additive and guarded with timeouts/warnings, but involve filesystem operations and external adapter calls.Overview
Introduces a new Pro
config.rolling_deploy_adapterprotocol (previous_bundle_hashes,fetch,upload) to seed prior bundle hashes into the Node Renderer cache duringPreSeedRendererCache, withPREVIOUS_BUNDLE_HASHESavailable as a discovery override.Hooks
assets:precompileto publish the current server/RSC bundle hashes + companion assets via the adapter (best-effort with per-hash timeouts and warnings), and extendsreact_on_rails:doctorto validate/probe the adapter (including cache-dir reporting and env-var misuse warnings).Unifies cache staging helpers and updates staging to auto-include
loadable-stats.jsonwhen present, adds extensive specs for the stager/doctor/config validation, and documents the protocol with newdocs/pro/rolling-deploy-adapters.mdplus sidebar/node-renderer doc updates (plus a minor lychee exclude tweak).Reviewed by Cursor Bugbot for commit ca729db. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Configuration
Documentation
Improved Diagnostics
Tests
Chores