Skip to content

Conversation

@MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Oct 31, 2025

This will port the similar logic from formula_installer.rb that handles errors or manual user interrupts from producing partial installations.

I noticed this code is almost identical (with some missing edge-cases) in formula_installer.rb to Keg#uninstall so I added a new Keg#uninstall_ignore_interrupts_and_raise_failures! method and made both formula_installer.rb and retryable_download.rb use it.

@cho-m can you try this out and see if it fixes the issue? I cannot reliably reproduce this so I've had to hack together something a little.

Copilot AI review requested due to automatic review settings October 31, 2025 15:39
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds automatic cleanup of partial bottle installations when errors occur during the download and extraction process in RetryableDownload#fetch. The cleanup logic is extracted into a reusable method Keg#uninstall_ignore_interrupts_and_raise_failures! that is shared between RetryableDownload and FormulaInstaller.

Key Changes

  • Added cleanup_partial_installation_on_error! method to cleanup partial bottle installations in error scenarios
  • Extracted cleanup logic into a new Keg#uninstall_ignore_interrupts_and_raise_failures! method for reusability
  • Refactored FormulaInstaller to use the new shared cleanup method

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
Library/Homebrew/retryable_download.rb Added bottle_keg variable tracking and cleanup logic in rescue blocks; added cleanup_partial_installation_on_error! method
Library/Homebrew/keg.rb Added new uninstall_ignore_interrupts_and_raise_failures! method and added type signature to existing uninstall method
Library/Homebrew/formula_installer.rb Replaced inline cleanup code with call to new Keg#uninstall_ignore_interrupts_and_raise_failures! method
Comments suppressed due to low confidence (1)

Library/Homebrew/retryable_download.rb:6

  • Missing require \"keg\" statement. The new cleanup_partial_installation_on_error! method at line 132 calls Keg.new(path), but the Keg class is not explicitly required in this file. Add require \"keg\" to the require statements at the top of the file.
require "bottle"
require "api/json_download"
require "utils/output"

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cho-m
Copy link
Member

cho-m commented Oct 31, 2025

@cho-m can you try this out and see if it fixes the issue? I cannot reliably reproduce this so I've had to hack together something a little.

An issue that remains is when the pour completes (i.e. promise is fulfilled) and then user interrupts. Since the job is done, there is no interrupt exception here.

Also can happen when interrupt is done later, e.g. when I cancel at

==> Pouring qtbase--6.9.3.arm64_tahoe.bottle.1.tar.gz
🍺  /opt/homebrew/Cellar/qtbase/6.9.3: 3,991 files, 58.9MB
==> Installing qt dependency: qtsvg
^C

Not sure what is best way to show case reproduction, but here is my steps for above case run on this branch:

brew list | grep qtbrew install qt
==> Fetching downloads for: qt
✔︎ Bottle Manifest qt (6.9.3)
...
==> Installing qt dependency: md4c
==> Pouring md4c--0.5.2.arm64_tahoe.bottle.tar.gz
🍺  /opt/homebrew/Cellar/md4c/0.5.2: 20 files, 301.3KB
==> Installing qt dependency: qtbase
==> Pouring qtbase--6.9.3.arm64_tahoe.bottle.1.tar.gz
🍺  /opt/homebrew/Cellar/qtbase/6.9.3: 3,991 files, 58.9MB
==> Installing qt dependency: qtsvg
^Cfor formula in $(brew list --formulae); do
  ls "$(brew --prefix)/Cellar/$formula"/*/INSTALL_RECEIPT.json >/dev/null
done
zsh: no matches found: /opt/homebrew/Cellar/abseil/*/INSTALL_RECEIPT.json
zsh: no matches found: /opt/homebrew/Cellar/gumbo-parser/*/INSTALL_RECEIPT.json
zsh: no matches found: /opt/homebrew/Cellar/hunspell/*/INSTALL_RECEIPT.json
zsh: no matches found: /opt/homebrew/Cellar/libmng/*/INSTALL_RECEIPT.json
...brew autoremove; echo $?
0ls $(brew --prefix)/Cellar/qtsvg/*/
Frameworks  lib  share  sbom.spdx.jsonls $(brew --prefix)/Cellar/qtbase/*/
bin  Frameworks  include  lib  share  INSTALL_RECEIPT.json  sbom.spdx.json

@cho-m
Copy link
Member

cho-m commented Nov 2, 2025

Was looking into this a bit more and I am thinking (with current install flow) remove logic will need to be done around

valid_formula_installers = fetch_formulae(formula_installers)
valid_formula_installers.each do |fi|
formula = fi.formula
upgrade = formula.linked? && formula.outdated? && !formula.head? && !Homebrew::EnvConfig.no_install_upgrade?
install_formula(fi, upgrade:)
Cleanup.install_formula_clean!(formula)
end

As an interrupt can happen at any point from start of fetch_formulae to end of install_formula.

Only thing is we don't have all attempted pours available as a FormulaInstaller can create another FormulaInstaller for dependency. Could potentially track Bottle pours as part of download_queue.


Also, there was a user who hit a similar issue (Homebrew/homebrew-core#252010)

But it looks like they ended up with <formula>.tmp. Looking at code, I am not sure how that happened now given we should have handled this at:

ignore_interrupts do
tmp_keg.rename(installed_keg.to_path) if tmp_keg && !installed_keg.directory?

ignore_interrupts { FileUtils.rm_r(tmp_keg) if tmp_keg&.directory? }

@MikeMcQuaid
Copy link
Member Author

@cho-m Thanks for reviewing. Unless I've misunderstood: this PR addresses at least one of the initial issues? Would it not be worth making ✅ and merging and then I can work on more cases in more PRs?

Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me!

Copy link
Member

@cho-m cho-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logically seems fine. Haven't figured out a good way to test this though due to concurrent timing. May require programmatically triggering interrupt during a download or adding sleeps to allow manual interrupt.


EDIT: I did try adding a timeout in directory unpack but not sure what signal is sent to all threads as ctrl+c still exits. Manually had to mkdir /opt/homebrew/Cellar/qtsvg/6.9.3 and tried sleep when moving /opt/homebrew/Cellar/qtsvg/6.9.3/lib

✔︎ Bottle qtwebengine (6.9.3)
^CBottle qtsvg (6.9.3)
ls /opt/homebrew/Cellar/qtsvg/6.9.3
Frameworks

This will port the similar logic from `formula_installer.rb` that
handles errors or manual user interrupts from producing partial
installations.

I noticed this code is almost identical (with some missing edge-cases)
in `formula_installer.rb` to `Keg#uninstall` so I added a new
`Keg#uninstall_ignore_interrupts_and_raise_failures!` method and made
both `formula_installer.rb` and `retryable_download.rb` use it.
@MikeMcQuaid MikeMcQuaid force-pushed the cleanup_partial_installation_on_error branch from c6d84e1 to d162130 Compare November 4, 2025 17:03
@MikeMcQuaid MikeMcQuaid enabled auto-merge November 4, 2025 17:04
@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Nov 4, 2025
Merged via the queue into main with commit cd3564b Nov 4, 2025
38 checks passed
@MikeMcQuaid MikeMcQuaid deleted the cleanup_partial_installation_on_error branch November 4, 2025 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants