Cleanup partial download queue installations on error/interrupt #20966
Conversation
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.
There was a problem hiding this comment.
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
FormulaInstallerto 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 newcleanup_partial_installation_on_error!method at line 132 callsKeg.new(path), but the Keg class is not explicitly required in this file. Addrequire \"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.
| bottle_filename = T.cast(downloadable, Bottle).filename | ||
| bottle_keg = HOMEBREW_CELLAR/bottle_filename.name/bottle_filename.version.to_s |
There was a problem hiding this comment.
The variable bottle_keg is only initialized inside the if pour && downloadable.is_a?(Bottle) block (lines 81-88), but it's referenced in rescue blocks at lines 97 and 112 that can be triggered from any code path in the method. If an exception occurs when pour is false or downloadable is not a Bottle, bottle_keg will be undefined, causing a NameError. Consider initializing bottle_keg = nil before line 81, or only call cleanup when the variable is defined using defined?(bottle_keg).
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 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 qt
❯ brew 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
^C
❯ for 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 $?
0
❯ ls $(brew --prefix)/Cellar/qtsvg/*/
Frameworks lib share sbom.spdx.json
❯ ls $(brew --prefix)/Cellar/qtbase/*/
bin Frameworks include lib share INSTALL_RECEIPT.json sbom.spdx.json |
This will port the similar logic from
formula_installer.rbthat 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.rbtoKeg#uninstallso I added a newKeg#uninstall_ignore_interrupts_and_raise_failures!method and made bothformula_installer.rbandretryable_download.rbuse 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.