Skip to content

cask: remove pre-staged downloads when installs fail - #24107

Merged
MikeMcQuaid merged 2 commits into
Homebrew:mainfrom
dergachoff:cask-temp-caskroom-cleanup
Sep 28, 2026
Merged

MikeMcQuaid merged 2 commits into
Homebrew:mainfrom
dergachoff:cask-temp-caskroom-cleanup

Conversation

@dergachoff

Copy link
Copy Markdown

  • Have you followed our Contributing guidelines?
  • Have you checked for other open Pull Requests for the same change?
  • Have you explained what your changes do? Performance claims (e.g. "this is faster") must include brew benchmark results.
  • Have you explained why you'd like these changes included, not just what they do?
  • For bug fixes, have you given step-by-step brew commands to reproduce the bug?
  • Have you written new tests (excluding integration tests)? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) locally?

  • I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

I used Claude Code (Claude Opus 5.5) to find the bug, write the patch and tests, and draft this text. I used Codex CLI (GPT-6 Astra) to review the patch. I reviewed the diff and the test results.


Problem

Since #23034, brew extracts each cask to var/homebrew/tmp/.caskroom/<token>/<version> before it installs it.

If the install or upgrade fails before the move into the Caskroom, this copy stays.

brew cleanup does not remove it. It only cleans tmp/.cellar.

On my Mac, tmp/.caskroom/chatgpt had 22 old versions (31.4GB).

Steps to reproduce

  1. Make a local cask, version 1.0, with an uninstall_preflight block that raises.
  2. Install it.
  3. Change the version to 2.0.
  4. Run brew upgrade --cask <cask>. It fails.
  5. Run brew cleanup.
  6. ls $(brew --prefix)/var/homebrew/tmp/.caskroom/<cask> still shows 2.0 and 2.0.staged.

Fix

  • A failed install or upgrade now removes its extracted copy.
  • It does this after the old version is restored.
  • If the removal fails, brew prints a warning and shows the original error.
  • brew cleanup now also empties tmp/.caskroom. This covers interrupted runs.
  • If cleanup cannot remove a folder, it warns and continues.
  • New constant HOMEBREW_TEMP_CASKROOM replaces the path string.

Validation

  • brew lgtm passes.
  • The full brew tests suite passes.
  • Each new test fails without its fix.
  • On my Mac, the patched brew cleanup removed the 31.4GB.

A cask install or upgrade that fails before `Cask::Installer#stage`
left its pre-staged copy in `var/homebrew/tmp/.caskroom`, and
`brew cleanup` only emptied the formula `.cellar` sibling.

Purge the queued copy after the install and upgrade rollbacks and have
`brew cleanup` empty both temporary directories.
Copilot AI lite review requested due to automatic review settings September 27, 2026 16:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

馃煛 Changes recommended

Moderate cleanup and error-preservation issues remain unresolved.

Review effort: Lite
Findings: 2 Medium severity

Open (2)
What changed in this PR

This pull request cleans up pre-staged cask downloads after failed installs/upgrades and extends brew cleanup to remove interrupted staging data.

Changes:

  • Adds HOMEBREW_TEMP_CASKROOM.
  • Purges staged downloads during failure handling.
  • Adds cleanup behavior and regression tests.

Outstanding moderate findings concern cleanup being skipped when restore/rollback fails, and staging cleanup errors masking the original failure.

File Reviewed changes
Library/鈥婬omebrew/鈥媡est/鈥媠upport/鈥媗ib/鈥媠tartup/鈥媍onfig.rb Adds the test caskroom path constant.
Library/鈥婬omebrew/鈥媡est/鈥媠pec_helper.rb Preserves the new constant during tests.
Library/鈥婬omebrew/鈥媡est/鈥媍leanup_spec.rb Tests temporary cask staging cleanup.
Library/鈥婬omebrew/鈥媡est/鈥媍ask/鈥媢pgrade_spec.rb Tests upgrade-failure cleanup.
Library/鈥婬omebrew/鈥媡est/鈥媍ask/鈥媔nstaller_spec.rb Tests install-failure cleanup.
Library/鈥婬omebrew/鈥媠tartup/鈥媍onfig.rb Defines the temporary caskroom path.
Library/鈥婬omebrew/鈥媍leanup.rb Cleans temporary cellar and caskroom contents.
Library/鈥婬omebrew/鈥媍ask/鈥媢pgrade.rb Purges staged downloads during upgrade rollback.
Library/鈥婬omebrew/鈥媍ask/鈥媔nstaller.rb Removes staged downloads after install failures.
Library/鈥婬omebrew/鈥媍ask/鈥媎ownload.rb Uses the new caskroom constant.

馃挕 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread Library/Homebrew/cask/installer.rb Outdated
Comment thread Library/Homebrew/cask/upgrade.rb Outdated
The purge ran after `restore_backup` and inside the upgrade rollback
block, so a failing rollback step skipped it. Run it in an `ensure`
after `restore_backup` and after the upgrade rollback block.

@MikeMcQuaid MikeMcQuaid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@MikeMcQuaid
MikeMcQuaid added this pull request to the merge queue Sep 28, 2026
Merged via the queue into Homebrew:main with commit 2295103 Sep 28, 2026
81 of 82 checks passed
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.

3 participants