Polish rolling deploy review follow-ups (round 4)

Address claude[bot] re-review on PR #3173 (six inline + summary items).

Comments and clarifying notes (no behavior change):

- replace_bundle_directory: document the brief between-mv unavailability
  window, the kernel-atomic fast path, and the cross-FS slow path so the
  trade-off is explicit at the call site.
- sweep_stale_temporary_directories: explain the no-op-on-missing-cache_dir
  behavior is desired for Docker rebuilds but skips one cycle on
  persistent-volume deploys where the cache_dir was wiped in between.
- valid_required_rsc_payload?: note that this only checks basename
  presence; existence is validated downstream by valid_asset_payload?.
- handle_missing_adapter: explicit `return nil` after warn so the caller's
  `return handle_missing_adapter` semantics don't depend on warn's value.
- docs/pro/rolling-deploy-adapters.md: new "When upload runs" section
  documenting that staging/qa/preview/etc. all publish to the adapter and
  pointing operators at a per-env-initializer escape hatch.

Behavior changes (defensive):

- seed_previous_hash: explicit FileUtils.mkdir_p(staging_dir) before the
  first stage_previous_file call so a permission error surfaces with
  "Failed to seed previous bundle hash" attribution instead of being
  side-effect-created downstream by copy_file_atomically.

New tests:

- TEMPORARY_DIRECTORY_PATTERN consistency: assert it matches both
  temporary_bundle_directory output AND the .previous-<pid>-<hex> backup
  suffix produced by replace_bundle_directory, guarding against a future
  rename of either suffix drifting the sweep regex.
- configuration: regression spec for kwarg-only upload signature
  `def upload(bundle:, assets:)` (no positional bundle_hash) — should be
  rejected.
- assets_precompile: assert publish_current_bundle_if_configured *does*
  publish in non-dev/non-test envs (staging, production, qa, preview,
  custom), guarding the skip list from being widened by accident.

All 33 rolling_deploy_cache_stager_spec, 92 configuration+assets_precompile
spec examples pass. RuboCop clean on touched files.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: justin808

docs/pro/rolling-deploy-adapters.md

@@ -95,6 +95,12 @@ PREVIOUS_BUNDLE_HASHES=abc123,def456 rake react_on_rails_pro:pre_seed_renderer_c
 
 This runs the adapter's `fetch(hash)` for each listed hash but skips discovery. The adapter is still required to fetch the actual bundle files; setting the env var without configuring `config.rolling_deploy_adapter` produces a warning and skips seeding.
 
+## When `upload` runs
+
+`assets:precompile` invokes `upload` for the current build's bundle hashes whenever an adapter is configured **and** `Rails.env` is anything other than `development` or `test`. That includes `staging`, `production`, custom envs like `qa` or `preview`, and any other non-dev/non-test value.
+
+In practice this means a `staging` deploy hits the same artifact store as production — it must have the same credentials and write access. This is intentional: a `staging`-→-`staging` rolling deploy needs the previous staging hash seeded, and a `staging`-→-`production` promotion benefits from staging having warmed the store. If you need to keep `staging` out of the artifact store entirely, set `config.rolling_deploy_adapter = nil` in a `staging`-specific initializer rather than relying on env-based skipping at the gem level.
+
 ## Edge cases and error handling
 
 | Scenario                                | Behavior                                                                     |

react_on_rails_pro/lib/react_on_rails_pro/rolling_deploy_cache_stager.rb

@@ -66,14 +66,15 @@ def self.call(cache_dir:, current_hashes:, mode:)
 
     def self.handle_missing_adapter
       env_override = ENV["PREVIOUS_BUNDLE_HASHES"].to_s.strip
-      return if env_override.empty?
+      return nil if env_override.empty?
 
       # PREVIOUS_BUNDLE_HASHES overrides *discovery*; the adapter is still required
       # to fetch the actual bundle files. Refuse to proceed rather than raise a raw
       # NoMethodError on the nil adapter.
       warn "[ReactOnRailsPro] PREVIOUS_BUNDLE_HASHES=#{env_override.inspect} is set but no " \
            "rolling_deploy_adapter is configured. Rolling-deploy seeding requires both. " \
            "Set config.rolling_deploy_adapter to enable. Skipping previous-hash seeding."
+      nil
     end
     private_class_method :handle_missing_adapter
 
@@ -120,6 +121,10 @@ def self.seed_previous_hash(adapter, hash, cache_dir, mode)
 
       bundle_dir = bundle_directory(cache_dir, hash)
       staging_dir = temporary_bundle_directory(bundle_dir)
+      # Create the staging dir explicitly so a permission error here surfaces with a
+      # clear "Failed to seed previous bundle hash" attribution rather than as a
+      # downstream copy/symlink failure inside `stage_previous_file`.
+      FileUtils.mkdir_p(staging_dir)
       stage_previous_file(
         payload[:bundle],
         File.join(staging_dir, "#{hash}.js"),
@@ -230,6 +235,11 @@ def self.warn_non_file_asset_payload(hash, non_file_assets)
     end
     private_class_method :warn_non_file_asset_payload
 
+    # Only checks that the required RSC basenames *appear* in the payload's asset
+    # list. Existence and file-ness of those paths on disk are validated downstream
+    # by `valid_asset_payload?`, which attributes any missing required RSC files
+    # via `warn_missing_asset_payload`. Splitting the two passes lets each warning
+    # attribute the failure mode (contract gap vs. dangling path) accurately.
     def self.valid_required_rsc_payload?(asset_paths, hash)
       missing = required_rsc_asset_basenames - asset_paths.map { |path| File.basename(path) }
       return true if missing.empty?
@@ -286,6 +296,13 @@ def self.stage_file(src, dest, mode, log_prefix)
     end
     private_class_method :stage_file
 
+    # No-ops when `cache_dir` does not exist yet. On Docker-style deploys (cache
+    # rebuilt from an immutable image layer each release) that's the desired
+    # behavior. On persistent-volume deploys where the cache survives across
+    # releases but `cache_dir` was wiped between runs, orphaned `.staging-*` /
+    # `.previous-*` dirs from a prior run would re-appear when the volume is
+    # remounted under a fresh, empty `cache_dir` — they'll be swept on the next
+    # successful seeding pass once `cache_dir` is recreated, not this one.
     def self.sweep_stale_temporary_directories(cache_dir)
       return unless Dir.exist?(cache_dir)
 
@@ -348,6 +365,15 @@ def self.bundle_directory(cache_dir, hash)
     end
     private_class_method :bundle_directory
 
+    # There is a brief window between the two `mv` calls below where `bundle_dir`
+    # does not exist on disk. A renderer lookup for this hash during that window
+    # would miss the cache and fall back to the runtime 410-retry path — a single
+    # cold-start, not a correctness regression. On Linux/same-filesystem deploys
+    # `File.rename` is atomic at the kernel level, so the window is sub-millisecond.
+    # On cross-filesystem or NFS mounts, `FileUtils.mv` falls back to copy+delete
+    # and the window can widen to seconds. The trade-off is intentional: this
+    # design favors full-replacement atomicity (no half-staged dir ever observed)
+    # over zero-downtime swap, since the 410-retry fallback bounds the worst case.
     def self.replace_bundle_directory(staging_dir, bundle_dir)
       backup_dir = nil
       if File.exist?(bundle_dir)

react_on_rails_pro/spec/dummy/spec/rolling_deploy_cache_stager_spec.rb

@@ -563,4 +563,20 @@ def source_file(name, contents: "// #{name}")
       expect(described_class::DISCOVERY_TIMEOUT_SECONDS).to eq(10)
     end
   end
+
+  # Guards against a future rename of the temp/backup suffixes drifting out of
+  # sync with TEMPORARY_DIRECTORY_PATTERN — that would silently break the sweep.
+  describe "TEMPORARY_DIRECTORY_PATTERN" do
+    let(:bundle_dir) { "/tmp/cache/abc123" }
+
+    it "matches temporary_bundle_directory output" do
+      staging_dir = described_class.send(:temporary_bundle_directory, bundle_dir)
+      expect(File.basename(staging_dir)).to match(described_class::TEMPORARY_DIRECTORY_PATTERN)
+    end
+
+    it "matches the .previous-<pid>-<hex> backup suffix produced by replace_bundle_directory" do
+      backup_basename = "abc123.previous-#{Process.pid}-#{SecureRandom.hex(6)}"
+      expect(backup_basename).to match(described_class::TEMPORARY_DIRECTORY_PATTERN)
+    end
+  end
 end

react_on_rails_pro/spec/react_on_rails_pro/assets_precompile_spec.rb

@@ -383,6 +383,21 @@ def upload(*); end
       end
     end
 
+    # Documents the intentional behavior that any non-dev/non-test env (staging,
+    # production, qa, preview, custom envs) publishes to the configured adapter.
+    # Guards against the skip list being widened by accident — staging deploys
+    # need to seed the artifact store so the next staging-→-staging or
+    # staging-→-production rolling deploy can pre-seed the previous hash.
+    it "publishes in environments other than development and test (e.g. staging)" do
+      allow(adapter).to receive(:upload)
+      %w[staging production qa preview anything-else].each do |env_name|
+        allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new(env_name))
+        described_class.publish_current_bundle_if_configured
+      end
+
+      expect(adapter).to have_received(:upload).with("abc123", bundle: server_bundle, assets: []).exactly(5).times
+    end
+
     it "warns and continues when upload times out" do
       stub_const("ReactOnRailsPro::AssetsPrecompile::UPLOAD_TIMEOUT_SECONDS", 0.05)
       allow(adapter).to receive(:upload) { sleep 1 }

react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb

@@ -239,6 +239,25 @@ def self.upload(_hash, _options); end
         end.to raise_error(ReactOnRailsPro::Error, /upload\(bundle_hash, bundle:, assets:\)/)
       end
 
+      # Regression: `accepts_bundle_hash_argument?` rejects signatures that have
+      # zero positional parameters even when the required upload keywords are
+      # present. A plausible mistake is to swap the positional `bundle_hash` for
+      # a `bundle_hash:` keyword; the adapter would still raise at upload time
+      # because the stager passes `bundle_hash` positionally.
+      it "rejects kwarg-only upload signatures with no positional bundle_hash" do
+        adapter = Class.new do
+          def self.previous_bundle_hashes = []
+          def self.fetch(_hash) = nil
+          def self.upload(bundle:, assets:) = [bundle, assets]
+        end
+
+        expect do
+          ReactOnRailsPro.configure do |config|
+            config.rolling_deploy_adapter = adapter
+          end
+        end.to raise_error(ReactOnRailsPro::Error, /upload\(bundle_hash, bundle:, assets:\)/)
+      end
+
       it "rejects adapters that require extra positional upload arguments" do
         adapter = Class.new do
           def self.previous_bundle_hashes = []