Fix pouring dependencies extracted by download queue #21208
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug where bottles are extracted multiple times when pouring dependencies with concurrent downloads, which doubles disk space usage. The issue occurs because download_queue == nil for dependencies, causing the previous conditional check to fail.
The fix introduces a marker file (symlink) named {bottle_tmp_keg}.poured to track whether a bottle has already been extracted. This allows the code to skip re-extraction when the marker exists, regardless of whether download_queue is nil.
Key changes:
- Uses a symlink marker file to track completed bottle extractions
- Checks for the marker file instead of relying on
download_queuebeing non-nil - Cleans up existing directories and marker files before extraction to handle edge cases
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Library/Homebrew/retryable_download.rb | Adds logic to create and check for a .poured marker file after bottle extraction, preventing duplicate extractions |
| Library/Homebrew/formula_installer.rb | Updates the pour method to check for the marker file instead of relying on download_queue, allowing it to work correctly for dependencies |
Comments suppressed due to low confidence (2)
Library/Homebrew/formula_installer.rb:1507
- If
bottle_poured_fileis a broken symlink (target directory was removed),exist?will return false, and the code will try to extract again. However, the old broken symlink won't be cleaned up before moving the newly extracted directory, potentially causingFileUtils.mvto fail.
Add cleanup of the marker file before extraction in the else branch to handle this edge case.
else
downloadable.downloader.stage
end
Library/Homebrew/formula_installer.rb:1507
- The new marker file logic for handling already-extracted bottles lacks test coverage. Consider adding tests to verify:
- Bottles are reused when marker file exists
- Re-extraction occurs when marker file is missing
- Edge cases like broken symlinks are handled correctly
Tests exist for bottle pouring in test/formula_installer_bottle_spec.rb which could be extended.
# Download queue may have already extracted the bottle to a temporary directory.
# We cannot check `download_queue` as it is nil when pouring dependencies.
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?
FileUtils.rm(bottle_poured_file)
FileUtils.mv(bottle_tmp_keg, formula.prefix)
bottle_tmp_keg.parent.rmdir_if_possible
else
downloadable.downloader.stage
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Makes sense and clever solution, thanks @cho-m!
As to cleaning up and avoiding homebrew-core disk space issues: this feels like a good candidate for cleanup_during! in brew test-bot?
brew lgtm(style, typechecking and tests) with your changes locally?There is a bug with concurrent downloads when pouring dependencies that cause duplicate extractions, doubling the disk space usage.
This is because
download_queue == nilfor dependencies. I did try passing the download queue tofiatbrew/Library/Homebrew/formula_installer.rb
Lines 903 to 905 in fddcd48
But it causes errors trying to re-fetch manifest files. Didn't figure out the reason, so trying this PR's solution.
Hopefully will help with out-of-space issues in Homebrew/core runners.
Not sure if any scenarios we want to invalidate temporary directory.
In the case user interrupts
brew install(e.g. due to stalled download), it helps to reuse the already extracted bottles to avoid unnecessary disk writes