Extract queue staging hooks

- Keep bottle pre-pouring behaviour while moving it behind
  `Downloadable` queue staging hooks.
- Move cask extraction and rename helpers onto `Cask::Download`
  so installer orchestration stays thinner.
- Leave cask downloads as a no-op for queued staging so this stays
  behaviour-neutral before adding new queue work.

Author: MikeMcQuaid

Library/Homebrew/bottle.rb

@@ -1,6 +1,8 @@
 # typed: strict
 # frozen_string_literal: true
 
+require "unpack_strategy"
+
 class Bottle
   include Downloadable
 
@@ -247,6 +249,55 @@ def sbom_supplement
   sig { returns(Filename) }
   def filename = Filename.new(@name, @pkg_version, @tag, @spec.rebuild)
 
+  sig { returns(Pathname) }
+  def staged_path_from_download_queue
+    bottle_filename = filename
+    HOMEBREW_TEMP_CELLAR/bottle_filename.name/bottle_filename.version.to_s
+  end
+
+  sig { returns(Pathname) }
+  def staged_path_from_download_queue_marker
+    Pathname("#{staged_path_from_download_queue}.poured")
+  end
+
+  sig { override.params(_download: Pathname, pour: T::Boolean).returns(T::Boolean) }
+  def stage_from_download_queue?(_download, pour:)
+    pour
+  end
+
+  sig { override.params(download: Pathname, pour: T::Boolean).void }
+  def stage_from_download_queue(download, pour:)
+    return unless pour
+
+    bottle_tmp_keg = staged_path_from_download_queue
+    bottle_poured_file = staged_path_from_download_queue_marker
+
+    begin
+      HOMEBREW_TEMP_CELLAR.mkpath
+
+      return if bottle_poured_file.exist?
+
+      FileUtils.rm(bottle_poured_file) if bottle_poured_file.symlink?
+      FileUtils.rm_r(bottle_tmp_keg) if bottle_tmp_keg.directory?
+
+      UnpackStrategy.detect(download, prioritize_extension: true)
+                    .extract_nestedly(to: HOMEBREW_TEMP_CELLAR)
+
+      # Create a separate file to mark a completed extraction. This avoids
+      # a potential race condition if a user interrupts the install.
+      # We use a symlink to easily check that both this extra status file
+      # and the real extracted directory exist via `Pathname#exist?`.
+      FileUtils.ln_s(bottle_tmp_keg, bottle_poured_file)
+    # Catch any exception type here to clean up partial queued extractions.
+    rescue Exception # rubocop:disable Lint/RescueException
+      ignore_interrupts do
+        FileUtils.rm_r(bottle_tmp_keg) if bottle_tmp_keg.directory?
+        bottle_tmp_keg.parent.rmdir_if_possible
+      end
+      raise
+    end
+  end
+
   sig { returns(T.nilable(Resource::BottleManifest)) }
   def github_packages_manifest_resource
     return if @resource.download_strategy != CurlGitHubPackagesDownloadStrategy

Library/Homebrew/cask/download.rb

@@ -3,6 +3,7 @@
 
 require "downloadable"
 require "fileutils"
+require "unpack_strategy"
 require "cask/cache"
 require "cask/quarantine"
 
@@ -87,6 +88,58 @@ def basename
       downloader.basename
     end
 
+    sig { returns(UnpackStrategy) }
+    def primary_container
+      @primary_container ||= T.let(
+        begin
+          downloaded_path = cask.download || fetch(quiet: true)
+          UnpackStrategy.detect(downloaded_path, type: cask.container&.type, merge_xattrs: true)
+        end,
+        T.nilable(UnpackStrategy),
+      )
+    end
+
+    sig { params(to: Pathname, verbose: T::Boolean).void }
+    def extract_primary_container(to:, verbose:)
+      odebug "Extracting primary container"
+
+      container = primary_container
+      raise "unexpected nil primary_container" unless container
+
+      odebug "Using container class #{container.class} for #{container.path}"
+
+      if (nested_container = cask.container&.nested)
+        Dir.mktmpdir("cask-installer", HOMEBREW_TEMP) do |tmpdir|
+          tmpdir = Pathname(tmpdir)
+          container.extract(to: tmpdir, basename:, verbose:)
+
+          FileUtils.chmod_R "+rw", tmpdir/nested_container, force: true, verbose: verbose
+
+          UnpackStrategy.detect(tmpdir/nested_container, merge_xattrs: true)
+                        .extract_nestedly(to:, verbose:)
+        end
+      else
+        container.extract_nestedly(to:, basename:, verbose:)
+      end
+
+      return unless @quarantine
+      return unless Quarantine.available?
+
+      Quarantine.propagate(from: container.path, to:)
+    end
+
+    sig { params(target_dir: Pathname).void }
+    def process_rename_operations(target_dir:)
+      return if cask.rename.empty?
+
+      odebug "Processing rename operations in #{target_dir}"
+
+      cask.rename.each do |rename_operation|
+        odebug "Renaming #{rename_operation.from} to #{rename_operation.to}"
+        rename_operation.perform_rename(target_dir)
+      end
+    end
+
     sig { override.returns(T::Boolean) }
     def downloaded_and_valid?
       return false unless super

Library/Homebrew/cask/installer.rb

@@ -264,13 +264,9 @@ def download(quiet: nil, timeout: nil)
 
     sig { returns(UnpackStrategy) }
     def primary_container
-      @primary_container ||= T.let(
-        begin
-          downloaded_path = download(quiet: true)
-          UnpackStrategy.detect(downloaded_path, type: @cask.container&.type, merge_xattrs: true)
-        end,
-        T.nilable(UnpackStrategy),
-      )
+      download(quiet: true) if @cask.download.nil?
+
+      downloader.primary_container
     end
 
     sig { returns(ArtifactSet) }
@@ -280,46 +276,12 @@ def artifacts
 
     sig { params(to: Pathname).void }
     def extract_primary_container(to: @cask.staged_path)
-      odebug "Extracting primary container"
-
-      container = primary_container
-      raise "unexpected nil primary_container" unless container
-
-      odebug "Using container class #{container.class} for #{container.path}"
-
-      basename = downloader.basename
-
-      if (nested_container = @cask.container&.nested)
-        Dir.mktmpdir("cask-installer", HOMEBREW_TEMP) do |tmpdir|
-          tmpdir = Pathname(tmpdir)
-          container.extract(to: tmpdir, basename:, verbose: verbose?)
-
-          FileUtils.chmod_R "+rw", tmpdir/nested_container, force: true, verbose: verbose?
-
-          UnpackStrategy.detect(tmpdir/nested_container, merge_xattrs: true)
-                        .extract_nestedly(to:, verbose: verbose?)
-        end
-      else
-        container.extract_nestedly(to:, basename:, verbose: verbose?)
-      end
-
-      return unless quarantine?
-      return unless Quarantine.available?
-
-      Quarantine.propagate(from: container.path, to:)
+      downloader.extract_primary_container(to:, verbose: verbose?)
     end
 
     sig { params(target_dir: T.nilable(Pathname)).void }
     def process_rename_operations(target_dir: nil)
-      return if @cask.rename.empty?
-
-      working_dir = target_dir || @cask.staged_path
-      odebug "Processing rename operations in #{working_dir}"
-
-      @cask.rename.each do |rename_operation|
-        odebug "Renaming #{rename_operation.from} to #{rename_operation.to}"
-        rename_operation.perform_rename(working_dir)
-      end
+      downloader.process_rename_operations(target_dir: target_dir || @cask.staged_path)
     end
 
     sig { params(predecessor: T.nilable(Cask)).void }

Library/Homebrew/download_queue.rb

@@ -40,18 +40,19 @@ def initialize(retries: 1, force: false, pour: false)
       params(
         downloadable:      Downloadable,
         check_attestation: T::Boolean,
+        stage:             T::Boolean,
       ).void
     }
-    def enqueue(downloadable, check_attestation: false)
+    def enqueue(downloadable, check_attestation: false, stage: pour)
       @cancelled.make_false
       cached_location = downloadable.cached_download
 
       @symlink_targets[cached_location] ||= Set.new
       targets = @symlink_targets.fetch(cached_location)
       targets << downloadable
 
-      @downloads_by_location[cached_location] ||= Concurrent::Promises.future_on(
-        pool, RetryableDownload.new(downloadable, tries:, pour:),
+      download = @downloads_by_location[cached_location] ||= Concurrent::Promises.future_on(
+        pool, RetryableDownload.new(downloadable, tries:),
         @cancelled, force, quiet, check_attestation
       ) do |download, cancelled, force, quiet, check_attestation|
         raise CancelledDownloadError if cancelled.true?
@@ -70,14 +71,28 @@ def enqueue(downloadable, check_attestation: false)
 
           check_bottle_attestation(downloadable, check_attestation:)
           create_symlinks_for_shared_download(cached_location)
+          cached_location
         rescue Interrupt
           raise CancelledDownloadError
         ensure
           @download_threads.delete(Thread.current)
         end
       end
 
-      downloads[downloadable] = @downloads_by_location.fetch(cached_location)
+      downloads[downloadable] = if stage
+        download.then_on(
+          pool, downloadable, pour
+        ) do |downloaded_path, queued_downloadable, queue_pour|
+          if queued_downloadable.stage_from_download_queue?(downloaded_path, pour: queue_pour)
+            queued_downloadable.extracting!
+            queued_downloadable.stage_from_download_queue(downloaded_path, pour: queue_pour)
+            queued_downloadable.downloaded!
+          end
+          downloaded_path
+        end
+      else
+        download
+      end
     end
 
     sig { void }

Library/Homebrew/downloadable.rb

@@ -168,6 +168,14 @@ def fetch(verify_download_integrity: true, timeout: nil, quiet: false)
     download
   end
 
+  sig { overridable.params(_download: Pathname, pour: T::Boolean).returns(T::Boolean) }
+  def stage_from_download_queue?(_download, pour:)
+    false
+  end
+
+  sig { overridable.params(_download: Pathname, pour: T::Boolean).void }
+  def stage_from_download_queue(_download, pour:); end
+
   sig { overridable.params(filename: Pathname).void }
   def verify_download_integrity(filename)
     verifying!

Library/Homebrew/formula_installer.rb

@@ -1570,22 +1570,21 @@ def api_bottle
   sig { void }
   def pour
     HOMEBREW_CELLAR.cd do
-      ohai "Pouring #{downloadable.downloader.basename}"
+      downloadable_object = downloadable
+      ohai "Pouring #{downloadable_object.downloader.basename}"
 
       formula.rack.mkpath
 
       # Download queue may have already extracted the bottle to a temporary directory.
       # We cannot rely on `download_queue` here as dependencies may be poured by another installer.
-      formula_prefix_relative_to_cellar = formula.prefix.relative_path_from(HOMEBREW_CELLAR)
-      bottle_tmp_keg = HOMEBREW_TEMP_CELLAR/formula_prefix_relative_to_cellar
-      bottle_poured_file = Pathname("#{bottle_tmp_keg}.poured")
-
-      if bottle_poured_file.exist?
+      if downloadable_object.is_a?(Bottle) &&
+         (bottle_poured_file = downloadable_object.staged_path_from_download_queue_marker).exist?
+        bottle_tmp_keg = downloadable_object.staged_path_from_download_queue
         FileUtils.rm(bottle_poured_file)
         FileUtils.mv(bottle_tmp_keg, formula.prefix)
         bottle_tmp_keg.parent.rmdir_if_possible
       else
-        downloadable.downloader.stage
+        downloadable_object.downloader.stage
       end
     end
 

Library/Homebrew/retryable_download.rb

@@ -19,14 +19,13 @@ def checksum = downloadable.checksum
     sig { override.returns(T::Array[String]) }
     def mirrors = downloadable.mirrors
 
-    sig { params(downloadable: Downloadable, tries: Integer, pour: T::Boolean).void }
-    def initialize(downloadable, tries:, pour: false)
+    sig { params(downloadable: Downloadable, tries: Integer).void }
+    def initialize(downloadable, tries:)
       super()
 
       @downloadable = downloadable
       @try = T.let(0, Integer)
       @tries = tries
-      @pour = pour
     end
 
     sig { override.returns(String) }
@@ -58,8 +57,6 @@ def downloader = downloadable.downloader
       ).returns(Pathname)
     }
     def fetch(verify_download_integrity: true, timeout: nil, quiet: false)
-      bottle_tmp_keg = nil
-
       @try += 1
 
       downloadable.downloading!
@@ -84,41 +81,12 @@ def fetch(verify_download_integrity: true, timeout: nil, quiet: false)
       json_download = downloadable.is_a?(API::JSONDownload)
       downloadable.verify_download_integrity(download) if verify_download_integrity && !json_download
 
-      if pour && downloadable.is_a?(Bottle)
-        downloadable.extracting!
-
-        HOMEBREW_TEMP_CELLAR.mkpath
-
-        bottle_filename = T.cast(downloadable, Bottle).filename
-        bottle_tmp_keg = HOMEBREW_TEMP_CELLAR/bottle_filename.name/bottle_filename.version.to_s
-        bottle_poured_file = Pathname("#{bottle_tmp_keg}.poured")
-
-        unless bottle_poured_file.exist?
-          FileUtils.rm(bottle_poured_file) if bottle_poured_file.symlink?
-          FileUtils.rm_r(bottle_tmp_keg) if bottle_tmp_keg.directory?
-
-          UnpackStrategy.detect(download, prioritize_extension: true)
-                        .extract_nestedly(to: HOMEBREW_TEMP_CELLAR)
-
-          # Create a separate file to mark a completed extraction. This avoids
-          # a potential race condition if a user interrupts the install.
-          # We use a symlink to easily check that both this extra status file
-          # and the real extracted directory exist via `Pathname#exist?`.
-          FileUtils.ln_s(bottle_tmp_keg, bottle_poured_file)
-        end
-
-        downloadable.downloaded!
-      elsif json_download
-        FileUtils.touch(download, mtime: Time.now)
-      end
+      FileUtils.touch(download, mtime: Time.now) if json_download
 
       download
     rescue DownloadError, ChecksumMismatchError, Resource::BottleManifest::Error => e
       tries_remaining = @tries - @try
-      if tries_remaining.zero?
-        cleanup_partial_installation_on_error!(bottle_tmp_keg)
-        raise
-      end
+      raise if tries_remaining.zero?
 
       wait = 2 ** @try
       unless quiet
@@ -132,10 +100,6 @@ def fetch(verify_download_integrity: true, timeout: nil, quiet: false)
       # fully-downloaded file is known-bad (checksum or manifest mismatch).
       downloadable.clear_cache unless e.is_a?(DownloadError)
       retry
-    # Catch any other types of exceptions as they leave us with partial installations.
-    rescue Exception # rubocop:disable Lint/RescueException
-      cleanup_partial_installation_on_error!(bottle_tmp_keg)
-      raise
     end
 
     sig { override.params(filename: Pathname).void }
@@ -145,19 +109,5 @@ def verify_download_integrity(filename) = downloadable.verify_download_integrity
 
     sig { returns(Downloadable) }
     attr_reader :downloadable
-
-    sig { returns(T::Boolean) }
-    attr_reader :pour
-
-    sig { params(path: T.nilable(Pathname)).void }
-    def cleanup_partial_installation_on_error!(path)
-      return if path.nil?
-      return unless path.directory?
-
-      ignore_interrupts do
-        FileUtils.rm_r(path)
-        path.parent.rmdir_if_possible
-      end
-    end
   end
 end