Cask upgrade: failed cleanup of the old version's backup reverts a completed install

[sethgoldin2]

Output of brew config

HOMEBREW_VERSION: 6.0.21-30-g07798f8
ORIGIN: https://github.com/Homebrew/brew
HEAD: 07798f85fc61b37a703a55704289ff0698193ca2
Last commit: 3 hours ago
Branch: main
Core tap: N/A
Core cask tap: N/A
HOMEBREW_PREFIX: /home/linuxbrew/.linuxbrew
HOMEBREW_REPOSITORY: /var/home/linuxbrew/.linuxbrew
Homebrew Ruby: 4.0.6 => /var/home/linuxbrew/.linuxbrew/Library/Homebrew/vendor/portable-ruby/4.0.6/bin/ruby
CPU: octa-core 64-bit skylake
Clang: N/A
Git: 2.55.0 => /home/linuxbrew/.linuxbrew/bin/git
Curl: 8.15.0 => /bin/curl
Kernel: Linux 7.1.9-100.fc43.x86_64 x86_64 GNU/Linux
Landlock ABI: 9
OS: Caligra Workbench
Host glibc: 2.42
Host libstdc++: 6.0.34
/usr/bin/gcc: 15.3.1
/usr/bin/ruby: N/A
glibc: N/A
gcc@13: N/A
gcc: N/A
xorg: N/A

Output of brew doctor

This is a Tier 2 configuration:
  https://docs.brew.sh/Support-Tiers#tier-2
You can report issues with Tier 2 configurations to Homebrew/* repositories!
  https://docs.brew.sh/Troubleshooting
Read the above document before opening any issues or PRs.

Description of issue

Cleanup of the previous version's backup runs inside the install's success path, so any
failure to delete that backup discards a cask install that has already completed — and the
rescue that handles it is less complete than revert_upgrade, leaving the cask with no
artifacts installed at all.

Posting as a discussion rather than a bug report because the incident is repaired and I can
no longer reproduce it on demand. Disclosure: the analysis and this write-up were
drafted with Claude Code (Claude Opus 5); I will answer any follow-up questions myself.

What happens

In Cask::Installer#install, purge_backed_up_versioned_files runs after the new version
is fully installed — after install_artifacts, after Tab.create/tab.write, after
analytics — but still inside the same begin/rescue:

install_artifacts(predecessor:)
tab = Tab.create(@cask)
tab.installed_on_request = installed_on_request?
tab.write
# ...analytics...
purge_backed_up_versioned_files
puts summary
rescue
  restore_backup
  raise
end

If the previous version's <staged_path>.upgrading backup cannot be deleted, that raises,
and the rescue calls restore_backup, which rm_rs the newly installed staging directory
and renames the old backup over it. A completed install is discarded because cleanup of the
previous version's backup failed.

Two compounding problems:

1. Cleanup of the old backup is treated as part of the install's success criteria.
2. This rescue path calls only restore_backup, unlike revert_upgrade, which is
   restore_backup + install_artifacts(predecessor:). So the restored old version never
   gets its artifacts reinstalled. The cask ends with no .desktop, no linked binaries, and
   its Moved artifacts sitting as real files in the staged directory.

That last state is unrecoverable by normal means: every subsequent brew upgrade fails in
Moved#move_back at Utils.path_occupied?(source) with

Error: <token>: It seems there is already a Generic Artifact at
'<prefix>/Caskroom/<token>/<version>/<file>'.

because move_back expects that path to be absent or a symlink to the target. Only
brew uninstall --cask --force clears it.

How the delete failed here

Any EPERM/EACCES from rmtree reaches this path — a root-owned or immutable file, a
read-only mount, a full disk during the rename, a stale handle on a network filesystem.
Mine was ownership: a third-party cask's postflight had chowned a subtree of its Caskroom
staging directory to root:root. On upgrade, rmtree of the backup failed on ownership, so
Cask::Utils.gain_permissions_remove (cask/utils.rb:123) fell back to

sudo chown -R -- <user> <prefix>/Caskroom/<token>/<version>.upgrading

which ran with no controlling terminal (terminal=? in the audit record) and could not
prompt for a password. I am reporting the root-owning separately to that tap; the question
here is only about the revert behaviour.

Suggested reproduction

1. brew install --cask <any cask with a Moved artifact>
2. sudo chown -R root:root "$(brew --prefix)/Caskroom/<token>/<version>/<subdir>"
3. Ensure sudo cannot prompt — no tty, SUDO_ASKPASS unset
4. Bump the cask version, then brew upgrade --cask <token>

Expected: the new version installs, the purge fails, the install is reverted, and the cask is
left with nothing installed. Re-running brew upgrade then fails permanently in move_back.

I have not run this on a Tier 1 prefix. Nothing in the path appears to depend on the prefix
layout — the raise comes from a failed delete and the revert is unconditional — but that is
an inference, not a tested claim.

Suggested fix

• Treat post-install cleanup as non-fatal: wrap purge_backed_up_versioned_files in its own
  rescue that opoos, or move it after puts summary, outside the guarded region.
• Consider giving the rescue revert_upgrade semantics, so a reverted cask is left with its
  artifacts installed rather than half-uninstalled.
• Optional: have Moved#move_back's error mention --force, since the wedged state has no
  other exit.

Question

Is the revert-on-cleanup-failure behaviour intended? If not, I am happy to build a
reproduction on a Tier 1 prefix and open a proper issue.

[yaohuangguan]

I can't speak for maintainer intent, but the failure mode you described is still present in current main and the two rollback paths are indeed asymmetric.

In Cask::Installer#install, the sequence is currently:

install_artifacts(predecessor:)
# write Tab / analytics
purge_backed_up_versioned_files
puts summary
rescue => e
  begin
    restore_backup
  ensure
    purge_staged_download
  end
  raise e
end

So an exception from purge_backed_up_versioned_files is treated exactly like an installation failure even though the new artifacts and tab have already been installed/written.

And your second point also matches current source:

def restore_backup
  # restores staged + metadata directories
end

def revert_upgrade(predecessor:)
  restore_backup
  install_artifacts(predecessor:)
end

The generic install rescue calls only restore_backup, not revert_upgrade(predecessor:). That means reverting after a late cleanup exception can restore the old staged files without reinstalling their external artifacts.

Current source: https://github.com/Homebrew/brew/blob/main/Library/Homebrew/cask/installer.rb

I'd separate two issues:

1. Primary bug: cleanup of the previous backup can throw after the new install has completed, causing a rollback for a post-install cleanup failure.
2. Recovery bug: that rollback is not symmetric with the normal upgrade rollback because it restores the backup but does not reinstall predecessor artifacts.

Your root-owned subtree/no-TTY sudo case is one trigger, but the control-flow problem does not depend on that specific trigger; any exception from the purge reaches the same rescue.

A Tier-1 reproduction would be worth filing as an issue. For a fix, either of these is defensible:

• make purge_backed_up_versioned_files best-effort after the successful install boundary (warn and leave the backup for later cleanup), or
• if cleanup failure is intentionally fatal, make the rescue perform a complete rollback equivalent to revert_upgrade(predecessor:) whenever a predecessor exists.

I'd lean toward the first because failure to remove an old backup should not invalidate an already successful new install, but that policy choice is for Homebrew maintainers. The important part for a bug report is that current code can leave the cask in a state that neither represents the new install nor a fully restored predecessor.