From 863eb7fe6d9db85fa9cab0468c212daa9fc92756 Mon Sep 17 00:00:00 2001
From: Markus Reiter <me@reitermark.us>
Date: Sat, 23 Nov 2024 23:45:17 +0100
Subject: [PATCH] Fix incorrect download locking.

---
 Library/Homebrew/download_strategy.rb   |  3 +--
 Library/Homebrew/lock_file.rb           | 29 ++++++++++++++++++-------
 Library/Homebrew/test/lock_file_spec.rb | 29 +++++++++++++++++++++----
 3 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb
index 09858f5f6998ff..5a59e0f7d4666a 100644
--- a/Library/Homebrew/download_strategy.rb
+++ b/Library/Homebrew/download_strategy.rb
@@ -444,8 +444,7 @@ def fetch(timeout: nil)
       raise Timeout::Error, "Timed out downloading #{self.url}: #{e}"
     end
   ensure
-    download_lock&.unlock
-    download_lock&.path&.unlink
+    download_lock&.unlock(unlink: true)
   end
 
   def clear_cache
diff --git a/Library/Homebrew/lock_file.rb b/Library/Homebrew/lock_file.rb
index 3f37bddaaf58d0..cad31b80aeebd8 100644
--- a/Library/Homebrew/lock_file.rb
+++ b/Library/Homebrew/lock_file.rb
@@ -15,19 +15,30 @@ def initialize(type, locked_path)
     @lockfile = nil
   end
 
+  sig { void }
   def lock
-    @path.parent.mkpath
-    create_lockfile
-    return if @lockfile.flock(File::LOCK_EX | File::LOCK_NB)
+    ignore_interrupts do
+      create_lockfile
 
-    raise OperationInProgressError, @locked_path
+      next if @lockfile.flock(File::LOCK_EX | File::LOCK_NB)
+
+      raise OperationInProgressError, @locked_path
+    end
   end
 
-  def unlock
-    return if @lockfile.nil? || @lockfile.closed?
+  sig { params(unlink: T::Boolean).void }
+  def unlock(unlink: false)
+    ignore_interrupts do
+      next if @lockfile.nil? || @lockfile.closed?
+
+      @lockfile.flock(File::LOCK_UN)
+      @lockfile.close
 
-    @lockfile.flock(File::LOCK_UN)
-    @lockfile.close
+      if unlink
+        @path.unlink if @path.exist?
+        @lockfile = nil
+      end
+    end
   end
 
   def with_lock
@@ -42,6 +53,8 @@ def with_lock
   def create_lockfile
     return if @lockfile.present? && !@lockfile.closed?
 
+    @path.dirname.mkpath
+
     begin
       @lockfile = @path.open(File::RDWR | File::CREAT)
     rescue Errno::EMFILE
diff --git a/Library/Homebrew/test/lock_file_spec.rb b/Library/Homebrew/test/lock_file_spec.rb
index cb82d3eda2448f..7f141e4463491d 100644
--- a/Library/Homebrew/test/lock_file_spec.rb
+++ b/Library/Homebrew/test/lock_file_spec.rb
@@ -5,18 +5,26 @@
 RSpec.describe LockFile do
   subject(:lock_file) { described_class.new(:lock, Pathname("foo")) }
 
+  let(:lock_file_copy) { described_class.new(:lock, Pathname("foo")) }
+
   describe "#lock" do
-    it "does not raise an error when already locked" do
+    it "ensures the lock file is created" do
+      expect(lock_file.path).not_to exist
+      lock_file.lock
+      expect(lock_file.path).to exist
+    end
+
+    it "does not raise an error when the same instance is locked multiple times" do
       lock_file.lock
 
       expect { lock_file.lock }.not_to raise_error
     end
 
-    it "raises an error if a lock already exists" do
+    it "raises an error if another instance is already locked" do
       lock_file.lock
 
       expect do
-        described_class.new(:lock, Pathname("foo")).lock
+        lock_file_copy.lock
       end.to raise_error(OperationInProgressError)
     end
   end
@@ -30,7 +38,20 @@
       lock_file.lock
       lock_file.unlock
 
-      expect { described_class.new(:lock, Pathname("foo")).lock }.not_to raise_error
+      expect { lock_file_copy.lock }.not_to raise_error
+    end
+
+    it "allows deleting a lock file only by the instance that locked it" do
+      lock_file.lock
+      expect(lock_file.path).to exist
+
+      expect(lock_file_copy.path).to exist
+      lock_file_copy.unlock(unlink: true)
+      expect(lock_file_copy.path).to exist
+      expect(lock_file.path).to exist
+
+      lock_file.unlock(unlink: true)
+      expect(lock_file.path).not_to exist
     end
   end
 end