Harden rolling deploy staging diagnostics

Author: justin808

react_on_rails/lib/react_on_rails/doctor.rb

@@ -750,9 +750,7 @@ def auto_fix_versions
 
       report_sync_changes(result)
       report_skipped_specs(result)
-      if result.changes.any?
-        checker.add_info("  ℹ️  FIX=true only updates package.json; update Gemfile constraints manually if needed.")
-      end
+      checker.add_info("  ℹ️  FIX=true only updates package.json; update Gemfile constraints manually if needed.")
     rescue StandardError => e
       checker.add_warning("  ⚠️  FIX=true: Could not auto-sync versions: #{e.message}")
     end
@@ -2917,8 +2915,8 @@ def report_resolved_cache_dir
       cache_dir = ReactOnRailsPro::Utils.resolve_renderer_cache_dir
       if File.directory?(cache_dir)
         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)
+        subdirs = Dir.children(cache_dir).select do |entry|
+          File.directory?(File.join(cache_dir, entry)) && !entry.match?(temp_dir_pattern)
         end
         checker.add_info("ℹ️  Resolved renderer cache dir: #{cache_dir} (#{subdirs.length} bundle-hash subdir(s))")
       else

react_on_rails/spec/lib/react_on_rails/doctor_spec.rb

@@ -2530,6 +2530,33 @@ def self.upload(_hash, **_opts); end
       end
     end
 
+    context "when renderer cache contains rolling-deploy temporary directories" do
+      let(:cache_dir) { Dir.mktmpdir }
+      let(:adapter) do
+        Module.new do
+          def self.previous_bundle_hashes = %w[abc]
+          def self.fetch(_hash); end
+          def self.upload(_hash, **_opts); end
+        end
+      end
+
+      before do
+        FileUtils.mkdir_p(File.join(cache_dir, "abc"))
+        FileUtils.mkdir_p(File.join(cache_dir, "abc.staging-1234-deadbeef12"))
+        FileUtils.mkdir_p(File.join(cache_dir, "abc.previous-1234-feedface12"))
+        cache_dir_value = cache_dir
+        ReactOnRailsPro::Utils.define_singleton_method(:resolve_renderer_cache_dir) { cache_dir_value }
+      end
+
+      after { FileUtils.rm_rf(cache_dir) }
+
+      it "excludes temporary directories from the bundle-hash count" do
+        doctor.send(:check_rolling_deploy_adapter)
+        info = checker.messages.select { |m| m[:type] == :info }
+        expect(info.map { |m| m[:content] }).to include(a_string_matching(/\(1 bundle-hash subdir\(s\)\)/))
+      end
+    end
+
     context "when adapter is missing required methods" do
       let(:adapter) do
         Module.new do

react_on_rails_pro/lib/react_on_rails_pro/rolling_deploy_cache_stager.rb

@@ -356,6 +356,7 @@ def self.replace_bundle_directory(staging_dir, bundle_dir)
       end
 
       FileUtils.mv(staging_dir, bundle_dir)
+      puts "[ReactOnRailsPro] Staged previous bundle hash into #{bundle_dir}"
       remove_previous_bundle_backup(backup_dir)
     rescue StandardError
       restore_previous_bundle_directory(backup_dir, bundle_dir)
@@ -377,13 +378,15 @@ 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)
+      # Catch the narrow TOCTOU window where another writer recreates
+      # bundle_dir between rm_rf and mv; warn through the rescue path instead
+      # of overwriting that writer's work.
+      if File.exist?(bundle_dir)
+        raise ReactOnRailsPro::Error,
+              "Cannot restore previous rolling-deploy bundle directory because #{bundle_dir} already exists."
+      end
+
+      FileUtils.mv(backup_dir, bundle_dir)
     rescue StandardError => e
       warn "[ReactOnRailsPro] Could not restore previous rolling-deploy bundle directory #{backup_dir} " \
            "to #{bundle_dir}: #{e.class}: #{e.message}. Runtime 410-retry remains the fallback."

react_on_rails_pro/spec/dummy/spec/rolling_deploy_cache_stager_spec.rb

@@ -61,9 +61,11 @@ def source_file(name, contents: "// #{name}")
     end
 
     it "copies bundle + assets into <cache>/<hash>/ in :copy mode" do
-      described_class.call(cache_dir: cache_dir, current_hashes: [], mode: :copy)
-
       bundle_dir = File.join(cache_dir, "abc123")
+      promoted_bundle_dir = File.join(File.realpath(cache_dir), "abc123")
+      expect { described_class.call(cache_dir: cache_dir, current_hashes: [], mode: :copy) }
+        .to output(/Staged previous bundle hash into #{Regexp.escape(promoted_bundle_dir)}/).to_stdout
+
       expect(File.exist?(File.join(bundle_dir, "abc123.js"))).to be(true)
       expect(File.exist?(File.join(bundle_dir, "loadable-stats.json"))).to be(true)
       expect(File.symlink?(File.join(bundle_dir, "abc123.js"))).to be(false)
@@ -378,6 +380,36 @@ def source_file(name, contents: "// #{name}")
     end
   end
 
+  context "when restore finds the bundle directory recreated by another process" do
+    let(:src_bundle) { source_file("bundle-new.js", contents: "// new bundle") }
+    let(:src_asset) { source_file("loadable-stats.json", contents: "{}") }
+    let(:bundle_dir) { File.join(cache_dir, "abc123") }
+    let(:existing_bundle) { File.join(bundle_dir, "abc123.js") }
+
+    before do
+      FileUtils.mkdir_p(bundle_dir)
+      File.write(existing_bundle, "// existing bundle")
+      allow(adapter).to receive_messages(previous_bundle_hashes: ["abc123"])
+      allow(adapter).to receive(:fetch).with("abc123").and_return(bundle: src_bundle, assets: [src_asset])
+      allow(FileUtils).to receive(:mv).and_wrap_original do |original, source, destination, *args|
+        raise Errno::EIO, "promotion failed" if source.to_s.include?("abc123.staging-")
+
+        original.call(source, destination, *args)
+      end
+      allow(FileUtils).to receive(:rm_rf).and_wrap_original do |original, path, *args|
+        original.call(path, *args)
+        FileUtils.mkdir_p(bundle_dir) if path.to_s.end_with?("/abc123")
+      end
+    end
+
+    it "warns instead of silently skipping backup restore" do
+      expect { described_class.call(cache_dir: cache_dir, current_hashes: [], mode: :copy) }
+        .to output(/Could not restore previous rolling-deploy bundle directory/).to_stderr
+
+      expect(Dir.children(cache_dir).grep(/abc123\.previous/)).not_to be_empty
+    end
+  end
+
   context "when refreshing an existing seeded hash cannot remove the old backup after promotion" do
     let(:src_bundle) { source_file("bundle-new.js", contents: "// new bundle") }
     let(:src_asset) { source_file("loadable-stats.json", contents: "{}") }
@@ -405,25 +437,29 @@ 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-1234-deadbeef") }
-    let(:fresh_previous_dir) { File.join(cache_dir, "abc123.previous-1234-feedface") }
+    let(:stale_staging_dir) { File.join(cache_dir, "abc123.staging-1234-deadbeef12") }
+    let(:fresh_previous_dir) { File.join(cache_dir, "abc123.previous-1234-feedface12") }
+    let(:hash_like_dir) { File.join(cache_dir, "release.staging-123-deadbeef") }
 
     before do
       stub_const("ReactOnRailsPro::RollingDeployCacheStager::STALE_TEMP_DIR_TTL_SECONDS", 60)
       allow(adapter).to receive_messages(previous_bundle_hashes: [])
       FileUtils.mkdir_p(stale_staging_dir)
       FileUtils.mkdir_p(fresh_previous_dir)
+      FileUtils.mkdir_p(hash_like_dir)
       old_time = Time.now - 120
       File.utime(old_time, old_time, stale_staging_dir)
+      File.utime(old_time, old_time, hash_like_dir)
     end
 
-    it "removes stale temp directories and keeps fresh ones" do
+    it "removes stale temp directories while preserving hash-like names outside the temp pattern" do
       expect { described_class.call(cache_dir: cache_dir, current_hashes: [], mode: :copy) }
         .to output(/Removed stale rolling-deploy temp directory/).to_stderr
         .and output(/No previous bundle hashes/).to_stdout
 
       expect(File.exist?(stale_staging_dir)).to be(false)
       expect(File.exist?(fresh_previous_dir)).to be(true)
+      expect(File.exist?(hash_like_dir)).to be(true)
     end
 
     it "does not match real bundle hash dirs that look superficially similar" do