Improve locking UX

My experience recently playing around with our locking behaviour is
that, while mostly seamless and not seen by users, it's leaks
implementation details a bit too heavily.

As a result, the following improvements are in this commit:
- Ensure that, whenever possible, we tell the user the actual command
  that is holding a given lock instead of the lock name (an internal
  implementation detail)
- Make the locking error output a little more consistent and user
  friendly
- Add a `DownloadLock` class to simplify locking downloads
- Add a `HOMEBREW_LOCK_CONTEXT` variable to allow adding additional
  context for logging error messages
- Lock paths and leave deciding how this translates to lock names up
  to the locking code itself
- Lock the Cellar/Caskroom paths explicitly rather than implicitly

Co-authored-by: Carlo Cabrera <[email protected]>

Library/Homebrew/brew.rb

@@ -87,6 +87,7 @@
   if internal_cmd || Commands.external_ruby_v2_cmd_path(cmd)
     cmd = T.must(cmd)
     cmd_class = Homebrew::AbstractCommand.command(cmd)
+    Homebrew.running_command = cmd
     if cmd_class
       command_instance = cmd_class.new
 
@@ -97,6 +98,7 @@
       Homebrew.public_send Commands.method_name(cmd)
     end
   elsif (path = Commands.external_ruby_cmd_path(cmd))
+    Homebrew.running_command = cmd
     require?(path)
     exit Homebrew.failed? ? 1 : 0
   elsif Commands.external_cmd_path(cmd)

Library/Homebrew/cleanup.rb

@@ -411,7 +411,7 @@ def cleanup_unreferenced_downloads
       (downloads - referenced_downloads).each do |download|
         if self.class.incomplete?(download)
           begin
-            LockFile.new(download.basename).with_lock do
+            DownloadLock.new(download).with_lock do
               download.unlink
             end
           rescue OperationInProgressError

Library/Homebrew/cmd/vendor-install.sh

@@ -359,7 +359,7 @@ homebrew-vendor-install() {
 
   CACHED_LOCATION="${HOMEBREW_CACHE}/${VENDOR_FILENAME}"
 
-  lock "vendor-install-${VENDOR_NAME}"
+  lock "vendor-install ${VENDOR_NAME}"
   fetch
   install
 

Library/Homebrew/download_strategy.rb

@@ -395,7 +395,7 @@ def initialize(url, name, version, **meta)
   def fetch(timeout: nil)
     end_time = Time.now + timeout if timeout
 
-    download_lock = LockFile.new(temporary_path.basename)
+    download_lock = DownloadLock.new(temporary_path)
     download_lock.lock
 
     urls = [url, *mirrors]

Library/Homebrew/env_config.rb

@@ -295,6 +295,9 @@ module EnvConfig
                       "or `$HOME/.homebrew/livecheck_watchlist.txt` otherwise.",
         default:      "#{ENV.fetch("HOMEBREW_USER_CONFIG_HOME")}/livecheck_watchlist.txt",
       },
+      HOMEBREW_LOCK_CONTEXT:                     {
+        description: "If set, Homebrew will add this output as additional context for locking errors. This is useful when running `brew` in the background.",
+      },
       HOMEBREW_LOGS:                             {
         description:  "Use this directory to store log files.",
         default_text: "macOS: `$HOME/Library/Logs/Homebrew`, " \

Library/Homebrew/exceptions.rb

@@ -359,10 +359,14 @@ def initialize(name)
 
 # Raised when another Homebrew operation is already in progress.
 class OperationInProgressError < RuntimeError
-  def initialize(name)
+  sig { params(locked_path: Pathname).void }
+  def initialize(locked_path)
+    full_command = Homebrew.running_command_with_args.presence || "brew"
+    lock_context = if (env_lock_context = Homebrew::EnvConfig.lock_context.presence)
+      "\n#{env_lock_context}"
+    end
     message = <<~EOS
-      Operation already in progress for #{name}
-      Another active Homebrew process is already using #{name}.
+      A `#{full_command}` process has already locked #{locked_path}.#{lock_context}
       Please wait for it to finish or terminate it to continue.
     EOS
 

Library/Homebrew/global.rb

@@ -110,6 +110,16 @@ def running_as_root_but_not_owned_by_root?
     def auto_update_command?
       ENV.fetch("HOMEBREW_AUTO_UPDATE_COMMAND", false).present?
     end
+
+    sig { params(cmd: T.nilable(String)).void }
+    def running_command=(cmd)
+      @running_command_with_args = "#{cmd} #{ARGV.join(" ")}"
+    end
+
+    sig { returns String }
+    def running_command_with_args
+      "brew #{@running_command_with_args}".strip
+    end
   end
 end
 

Library/Homebrew/lock_file.rb

@@ -3,13 +3,15 @@
 
 require "fcntl"
 
-# A lock file.
+# A lock file to prevent multiple Homebrew processes from modifying the same path.
 class LockFile
   attr_reader :path
 
-  def initialize(name)
-    @name = name.to_s
-    @path = HOMEBREW_LOCKS/"#{@name}.lock"
+  sig { params(type: Symbol, locked_path: Pathname).void }
+  def initialize(type, locked_path)
+    @locked_path = locked_path
+    lock_name = locked_path.basename.to_s
+    @path = HOMEBREW_LOCKS/"#{lock_name}.#{type}.lock"
     @lockfile = nil
   end
 
@@ -18,7 +20,7 @@ def lock
     create_lockfile
     return if @lockfile.flock(File::LOCK_EX | File::LOCK_NB)
 
-    raise OperationInProgressError, @name
+    raise OperationInProgressError, @locked_path
   end
 
   def unlock
@@ -51,14 +53,24 @@ def create_lockfile
 
 # A lock file for a formula.
 class FormulaLock < LockFile
-  def initialize(name)
-    super("#{name}.formula")
+  sig { params(rack_name: String).void }
+  def initialize(rack_name)
+    super(:formula, HOMEBREW_CELLAR/rack_name)
   end
 end
 
 # A lock file for a cask.
 class CaskLock < LockFile
-  def initialize(name)
-    super("#{name}.cask")
+  sig { params(cask_token: String).void }
+  def initialize(cask_token)
+    super(:cask, HOMEBREW_PREFIX/"Caskroom/#{cask_token}")
+  end
+end
+
+# A lock file for a download.
+class DownloadLock < LockFile
+  sig { params(download_path: Pathname).void }
+  def initialize(download_path)
+    super(:download, download_path)
   end
 end

Library/Homebrew/test/exceptions_spec.rb

@@ -149,9 +149,9 @@ class Baz < Formula; end
   end
 
   describe OperationInProgressError do
-    subject(:error) { described_class.new("foo") }
+    subject(:error) { described_class.new(Pathname("foo")) }
 
-    it(:to_s) { expect(error.to_s).to match(/Operation already in progress for foo/) }
+    it(:to_s) { expect(error.to_s).to match(/has already locked foo/) }
   end
 
   describe FormulaInstallationAlreadyAttemptedError do

Library/Homebrew/test/lock_file_spec.rb

@@ -3,7 +3,7 @@
 require "lock_file"
 
 RSpec.describe LockFile do
-  subject(:lock_file) { described_class.new("foo") }
+  subject(:lock_file) { described_class.new(:lock, Pathname("foo")) }
 
   describe "#lock" do
     it "does not raise an error when already locked" do
@@ -16,7 +16,7 @@
       lock_file.lock
 
       expect do
-        described_class.new("foo").lock
+        described_class.new(:lock, Pathname("foo")).lock
       end.to raise_error(OperationInProgressError)
     end
   end
@@ -30,7 +30,7 @@
       lock_file.lock
       lock_file.unlock
 
-      expect { described_class.new("foo").lock }.not_to raise_error
+      expect { described_class.new(:lock, Pathname("foo")).lock }.not_to raise_error
     end
   end
 end

Library/Homebrew/utils/lock.sh

@@ -1,18 +1,20 @@
-# create a lock using `flock(2)`. A name is required as first argument.
-# the lock will be automatically unlocked when the shell process quits.
-# Noted due to the fixed FD, a shell process can only create one lock.
+# Create a lock using `flock(2)`. A command name with arguments is required as
+# first argument. The lock will be automatically unlocked when the shell process
+# quits. Note due to the fixed FD, a shell process can only create one lock.
 # HOMEBREW_LIBRARY is by brew.sh
 # HOMEBREW_PREFIX is set by extend/ENV/super.rb
 # shellcheck disable=SC2154
 lock() {
-  local name="$1"
+  local command_name_and_args="$1"
+  # use bash to replace spaces with dashes
+  local lock_filename="${command_name_and_args// /-}"
   local lock_dir="${HOMEBREW_PREFIX}/var/homebrew/locks"
-  local lock_file="${lock_dir}/${name}"
+  local lock_file="${lock_dir}/${lock_filename}"
   [[ -d "${lock_dir}" ]] || mkdir -p "${lock_dir}"
   if [[ ! -w "${lock_dir}" ]]
   then
     odie <<EOS
-Can't create ${name} lock in ${lock_dir}!
+Can't create \`brew ${command_name_and_args}\` lock in ${lock_dir}!
 Fix permissions by running:
   sudo chown -R ${USER-\$(whoami)} ${HOMEBREW_PREFIX}/var/homebrew
 EOS
@@ -28,18 +30,25 @@ EOS
   exec 200>&-
   # open the lock file to FD, so the shell process can hold the lock.
   exec 200>"${lock_file}"
-  if ! _create_lock 200 "${name}"
+
+  if ! _create_lock 200 "${command_name_and_args}"
   then
+    local lock_context
+    if [[ -n "${HOMEBREW_LOCK_CONTEXT}" ]]
+    then
+      lock_context="\n${HOMEBREW_LOCK_CONTEXT}"
+    fi
+
     odie <<EOS
-Another active Homebrew ${name} process is already in progress.
+Another \`brew ${command_name_and_args}\` process is already running.${lock_context}
 Please wait for it to finish or terminate it to continue.
 EOS
   fi
 }
 
 _create_lock() {
   local lock_file_descriptor="$1"
-  local name="$2"
+  local command_name_and_args="$2"
   local ruby="/usr/bin/ruby"
   local python="/usr/bin/python"
   [[ -x "${ruby}" ]] || ruby="$(type -P ruby)"
@@ -57,6 +66,6 @@ _create_lock() {
   then
     "${python}" -c "import fcntl; fcntl.flock(${lock_fd}, fcntl.LOCK_EX | fcntl.LOCK_NB)"
   else
-    onoe "Cannot create ${name} lock due to missing/too old ruby/flock/python, please avoid running Homebrew in parallel."
+    onoe "Cannot create \`brew ${command_name_and_args}\` lock due to missing/too old ruby/flock/python, please avoid running Homebrew in parallel."
   fi
 }

docs/Manpage.md

@@ -3872,6 +3872,11 @@ command execution e.g. `$(cat file)`.
   `$XDG_CONFIG_HOME` is set or `$HOME/.homebrew/livecheck_watchlist.txt`
   otherwise.
 
+`HOMEBREW_LOCK_CONTEXT`
+
+: If set, Homebrew will add this output as additional context for locking
+  errors. This is useful when running `brew` in the background.
+
 `HOMEBREW_LOGS`
 
 : Use this directory to store log files.

manpages/brew.1

@@ -2526,6 +2526,9 @@ Consult this file for the list of formulae to check by default when no formula a
 \fIDefault:\fP \fB$XDG_CONFIG_HOME/homebrew/livecheck_watchlist\.txt\fP if \fB$XDG_CONFIG_HOME\fP is set or \fB$HOME/\.homebrew/livecheck_watchlist\.txt\fP otherwise\.
 .RE
 .TP
+\fBHOMEBREW_LOCK_CONTEXT\fP
+If set, Homebrew will add this output as additional context for locking errors\. This is useful when running \fBbrew\fP in the background\.
+.TP
 \fBHOMEBREW_LOGS\fP
 Use this directory to store log files\.
 .RS