Polish rolling deploy review follow-ups (round 3)

Address four optional review findings from claude[bot] on PR #3173:

- Tighten TEMPORARY_DIRECTORY_PATTERN to require >=4-digit PID and
  >=8-char hex suffix so a real bundle hash like
  `bundle.staging-1-abc123` cannot accidentally match the temp-dir
  sweep pattern and be removed after STALE_TEMP_DIR_TTL_SECONDS.
- Log a per-hash success message after replace_bundle_directory so
  successful rolling-deploy seeding is visible in production logs
  (previously every failure path warned but success was silent).
- Document why restore_previous_bundle_directory's File.exist? guard
  intentionally skips the mv on a TOCTOU race (defer to runtime
  410-retry rather than overwrite a competing writer; backup_dir
  gets swept on a later run).
- Filter `.staging-...`/`.previous-...` temp dirs out of the
  doctor's renderer-cache bundle-hash count via a new
  rolling_deploy_temp_dir_pattern helper, with a fallback constant
  for when the Pro gem isn't loaded.

Adds regression specs for each change.

Author: justin808

react_on_rails/lib/react_on_rails/doctor.rb

@@ -2908,16 +2908,32 @@ def rolling_deploy_discovery_timeout_seconds
       end
     end
 
+    # Mirror of ReactOnRailsPro::RollingDeployCacheStager::TEMPORARY_DIRECTORY_PATTERN
+    # used as a fallback when the Pro gem isn't loaded so doctor still filters
+    # leftover staging/backup dirs out of the bundle-hash count.
+    ROLLING_DEPLOY_TEMP_DIR_PATTERN = /\.(?:staging|previous)-\d{4,}-[0-9a-f]{8,}\z/
+
     def report_resolved_cache_dir
       cache_dir = ReactOnRailsPro::Utils.resolve_renderer_cache_dir
       if File.directory?(cache_dir)
-        subdirs = Dir.children(cache_dir).select { |c| File.directory?(File.join(cache_dir, c)) }
+        temp_dir_pattern = rolling_deploy_temp_dir_pattern
+        subdirs = Dir.children(cache_dir).select do |c|
+          File.directory?(File.join(cache_dir, c)) && !c.match?(temp_dir_pattern)
+        end
         checker.add_info("ℹ️  Resolved renderer cache dir: #{cache_dir} (#{subdirs.length} bundle-hash subdir(s))")
       else
         checker.add_info("ℹ️  Resolved renderer cache dir: #{cache_dir} (does not exist yet)")
       end
     end
 
+    def rolling_deploy_temp_dir_pattern
+      if defined?(ReactOnRailsPro::RollingDeployCacheStager::TEMPORARY_DIRECTORY_PATTERN)
+        ReactOnRailsPro::RollingDeployCacheStager::TEMPORARY_DIRECTORY_PATTERN
+      else
+        ROLLING_DEPLOY_TEMP_DIR_PATTERN
+      end
+    end
+
     # The base 'react-on-rails' npm package is a transitive dependency of 'react-on-rails-pro',
     # so `import ... from 'react-on-rails'` resolves silently — loading the base package instead
     # of Pro. Components registered through the base package won't have Pro features (streaming,

react_on_rails/spec/lib/react_on_rails/doctor_spec.rb

@@ -2589,6 +2589,48 @@ def self.upload(_hash, **_opts); end
     end
   end
 
+  describe "report_resolved_cache_dir" do
+    let(:doctor) { described_class.new(verbose: false, fix: false) }
+    let(:checker) { doctor.instance_variable_get(:@checker) }
+    let(:cache_dir) { Dir.mktmpdir("doctor-cache") }
+
+    before do
+      cache_dir_value = cache_dir
+      pro_module = Module.new
+      utils_module = Module.new
+      utils_module.define_singleton_method(:resolve_renderer_cache_dir) { cache_dir_value }
+      pro_module.const_set(:Utils, utils_module)
+      stub_const("ReactOnRailsPro", pro_module)
+    end
+
+    after { FileUtils.rm_rf(cache_dir) }
+
+    it "excludes leftover staging/backup temp dirs from the bundle-hash count" do
+      FileUtils.mkdir_p(File.join(cache_dir, "abc123"))
+      FileUtils.mkdir_p(File.join(cache_dir, "def456"))
+      FileUtils.mkdir_p(File.join(cache_dir, "abc123.staging-1234-deadbeef"))
+      FileUtils.mkdir_p(File.join(cache_dir, "abc123.previous-1234-feedface"))
+
+      doctor.send(:report_resolved_cache_dir)
+
+      info = checker.messages.find { |m| m[:type] == :info && m[:content].include?(cache_dir) }
+      expect(info[:content]).to include("(2 bundle-hash subdir(s))")
+    end
+
+    it "uses the Pro stager constant when the Pro gem is loaded" do
+      stager_module = Module.new
+      stager_module.const_set(:TEMPORARY_DIRECTORY_PATTERN, /\.tempmarker\z/)
+      ReactOnRailsPro.const_set(:RollingDeployCacheStager, stager_module)
+      FileUtils.mkdir_p(File.join(cache_dir, "abc123"))
+      FileUtils.mkdir_p(File.join(cache_dir, "abc123.tempmarker"))
+
+      doctor.send(:report_resolved_cache_dir)
+
+      info = checker.messages.find { |m| m[:type] == :info && m[:content].include?(cache_dir) }
+      expect(info[:content]).to include("(1 bundle-hash subdir(s))")
+    end
+  end
+
   describe "check_deprecated_renderer_cache_task" do
     let(:doctor) { described_class.new(verbose: false, fix: false) }
     let(:checker) { doctor.instance_variable_get(:@checker) }

react_on_rails_pro/lib/react_on_rails_pro/rolling_deploy_cache_stager.rb

@@ -38,7 +38,12 @@ module RollingDeployCacheStager # rubocop:disable Metrics/ModuleLength
     DISCOVERY_TIMEOUT_SECONDS = 10
     FETCH_TIMEOUT_SECONDS = 30
     STALE_TEMP_DIR_TTL_SECONDS = 3600
-    TEMPORARY_DIRECTORY_PATTERN = /\.(?:staging|previous)-\d+-[0-9a-f]+\z/
+    # Match temp dirs created by `temporary_bundle_directory` (and the analogous
+    # `.previous-` backup suffix in `replace_bundle_directory`). The minimum
+    # widths (`\d{4,}` PID, `[0-9a-f]{8,}` random) defeat false positives where
+    # a real bundle hash happens to end with `.staging-<digits>-<hex>` — without
+    # them, a hash like `bundle.staging-1-abc123` could match and be swept.
+    TEMPORARY_DIRECTORY_PATTERN = /\.(?:staging|previous)-\d{4,}-[0-9a-f]{8,}\z/
 
     def self.call(cache_dir:, current_hashes:, mode:)
       adapter = ReactOnRailsPro.configuration.rolling_deploy_adapter
@@ -135,6 +140,7 @@ def self.seed_previous_hash(adapter, hash, cache_dir, mode)
 
       replace_bundle_directory(staging_dir, bundle_dir)
       staging_dir = nil
+      puts "[ReactOnRailsPro] Seeded previous bundle hash #{hash} at #{bundle_dir}."
     rescue StandardError => e
       # Remove only files created by this attempt. If the hash directory was
       # already valid from an earlier seed on a persistent cache volume, keep it
@@ -371,6 +377,12 @@ def self.restore_previous_bundle_directory(backup_dir, bundle_dir)
       return unless backup_dir && File.exist?(backup_dir)
 
       FileUtils.rm_rf(bundle_dir)
+      # The `unless File.exist?` guard catches the narrow TOCTOU window where
+      # another writer recreates `bundle_dir` between the rm_rf and the mv.
+      # We deliberately skip the mv in that case rather than overwriting that
+      # writer's work — the runtime 410-retry path is still a valid fallback,
+      # and `backup_dir` will be swept by `sweep_stale_temporary_directories`
+      # on the next run.
       FileUtils.mv(backup_dir, bundle_dir) unless File.exist?(bundle_dir)
     rescue StandardError => e
       warn "[ReactOnRailsPro] Could not restore previous rolling-deploy bundle directory #{backup_dir} " \

react_on_rails_pro/spec/dummy/spec/rolling_deploy_cache_stager_spec.rb

@@ -69,6 +69,11 @@ def source_file(name, contents: "// #{name}")
       expect(File.symlink?(File.join(bundle_dir, "abc123.js"))).to be(false)
     end
 
+    it "logs a per-hash success message after promotion" do
+      expect { described_class.call(cache_dir: cache_dir, current_hashes: [], mode: :copy) }
+        .to output(/Seeded previous bundle hash abc123 at/).to_stdout
+    end
+
     it "creates relative symlinks in :symlink mode" do
       described_class.call(cache_dir: cache_dir, current_hashes: [], mode: :symlink)
 
@@ -400,8 +405,8 @@ def source_file(name, contents: "// #{name}")
   end
 
   context "when stale temporary bundle directories are present" do
-    let(:stale_staging_dir) { File.join(cache_dir, "abc123.staging-123-deadbeef") }
-    let(:fresh_previous_dir) { File.join(cache_dir, "abc123.previous-123-feedface") }
+    let(:stale_staging_dir) { File.join(cache_dir, "abc123.staging-1234-deadbeef") }
+    let(:fresh_previous_dir) { File.join(cache_dir, "abc123.previous-1234-feedface") }
 
     before do
       stub_const("ReactOnRailsPro::RollingDeployCacheStager::STALE_TEMP_DIR_TTL_SECONDS", 60)
@@ -420,6 +425,19 @@ def source_file(name, contents: "// #{name}")
       expect(File.exist?(stale_staging_dir)).to be(false)
       expect(File.exist?(fresh_previous_dir)).to be(true)
     end
+
+    it "does not match real bundle hash dirs that look superficially similar" do
+      # Hash dir whose suffix has a sub-4-digit PID and sub-8-char hex segment.
+      # The tightened TEMPORARY_DIRECTORY_PATTERN must reject this so a real
+      # bundle is never silently swept after the TTL.
+      false_positive_dir = File.join(cache_dir, "bundle.staging-1-abc123")
+      FileUtils.mkdir_p(false_positive_dir)
+      File.utime(Time.now - 120, Time.now - 120, false_positive_dir)
+
+      described_class.call(cache_dir: cache_dir, current_hashes: [], mode: :copy)
+
+      expect(File.exist?(false_positive_dir)).to be(true)
+    end
   end
 
   context "when RSC support is enabled" do