From 3541b9e034ced4bb4d8affdd6283565dceccd022 Mon Sep 17 00:00:00 2001 From: Richard Janita <136868970+jimbolina@users.noreply.github.com> Date: Fri, 2 Oct 2026 09:13:12 -0400 Subject: [PATCH] [ruby/rubygems] Fix file locks letting multiple processes in when lock files are removed Gem.open_file_with_lock removed its lock file after releasing the lock. Since flock locks the file itself rather than its path, a process that was waiting on the old file could acquire it after it had been removed, while a newly arriving process created a fresh lock file and locked it immediately, so both ran at the same time. Remove the lock file while still holding the lock, and after acquiring it, check that the locked file is still the one at the lock path, retrying otherwise. https://github.com/ruby/rubygems/commit/9a93c235e7 --- lib/bundler/rubygems_ext.rb | 19 ++++++++-- lib/rubygems.rb | 28 +++++++++++--- spec/bundler/install/process_lock_spec.rb | 46 +++++++++++++++++++++++ test/rubygems/test_gem.rb | 45 ++++++++++++++++++++++ 4 files changed, 128 insertions(+), 10 deletions(-) diff --git a/lib/bundler/rubygems_ext.rb b/lib/bundler/rubygems_ext.rb index d3c4ce6d9dc..f777e06c01e 100644 --- a/lib/bundler/rubygems_ext.rb +++ b/lib/bundler/rubygems_ext.rb @@ -86,11 +86,22 @@ module Gem end end - def open_file_with_lock(path, &block) + def open_file_with_lock(path) file_lock = "#{path}.lock" - open_file_with_flock(file_lock, &block) - ensure - FileUtils.rm_f file_lock + + loop do + open_file_with_flock(file_lock) do |io| + # The previous holder removed the file while we were waiting on it, + # so leave this block and try again on the file now at the lock path + next unless File.identical?(io, file_lock) + + begin + return yield io + ensure + FileUtils.rm_f file_lock + end + end + end end end end diff --git a/lib/rubygems.rb b/lib/rubygems.rb index 62b857d0868..e1031665aa2 100644 --- a/lib/rubygems.rb +++ b/lib/rubygems.rb @@ -868,14 +868,30 @@ An Array (#{env.inspect}) was passed in from #{caller[3]} end ## - # Open a file with given flags, and protect access with a file lock + # Open a file with given flags, and protect access with a file lock. + # + # The lock file is removed once done, while still holding the lock. Since + # processes waiting for the lock may end up acquiring it on the removed file, + # after acquiring it we check that the locked file is still the one at the + # lock path, and start over otherwise. - def self.open_file_with_lock(path, &block) + def self.open_file_with_lock(path) file_lock = "#{path}.lock" - open_file_with_flock(file_lock, &block) - ensure - require "fileutils" - FileUtils.rm_f file_lock + + loop do + open_file_with_flock(file_lock) do |io| + # The previous holder removed the file while we were waiting on it, so + # leave this block and try again on the file now at the lock path + next unless File.identical?(io, file_lock) + + begin + return yield io + ensure + require "fileutils" + FileUtils.rm_f file_lock + end + end + end end ## diff --git a/spec/bundler/install/process_lock_spec.rb b/spec/bundler/install/process_lock_spec.rb index b096291d1a9..3bea0340bd4 100644 --- a/spec/bundler/install/process_lock_spec.rb +++ b/spec/bundler/install/process_lock_spec.rb @@ -21,6 +21,52 @@ RSpec.describe "process lock spec" do expect(the_bundle).to include_gems "myrack 1.0" end + it "keeps excluding late arrivals after the lock is handed over to a waiting process" do + events = Queue.new + release_waiter = Queue.new + + # The warning printed while blocking on the lock is the only observable + # sign that a contender has opened the lock file and is waiting on it + allow(Gem).to receive(:warn) do |message| + events << [Thread.current.name, :waiting] if message.include?("Waiting for another process") + end + + next_event = lambda do + events.pop(timeout: 10) || raise("Timed out waiting for a lock event") + end + + contender = lambda do |name, &block| + Thread.new do + Thread.current.name = name + Thread.current.abort_on_exception = true + Bundler::ProcessLock.lock(default_bundle_path) do + events << [name, :locked] + block&.call + end + end + end + + waiter = nil + Bundler::ProcessLock.lock(default_bundle_path) do + waiter = contender.call("waiter") { release_waiter.pop } + expect(next_event.call).to eq(["waiter", :waiting]) + end + expect(next_event.call).to eq(["waiter", :locked]) + + # Arrives only after the original holder is gone, while the waiter is + # still inside its critical section + late_arrival = contender.call("late_arrival") + expect(next_event.call).to eq(["late_arrival", :waiting]) + + release_waiter << true + [waiter, late_arrival].each(&:join) + expect(next_event.call).to eq(["late_arrival", :locked]) + expect(default_bundle_path("bundler.lock")).not_to exist + ensure + # Never leave the waiter blocked while holding the lock + release_waiter << true + end + context "when creating a lock raises Errno::ENOTSUP" do before { allow(File).to receive(:open).and_raise(Errno::ENOTSUP) } diff --git a/test/rubygems/test_gem.rb b/test/rubygems/test_gem.rb index 88b461a2a0a..052a15aa638 100644 --- a/test/rubygems/test_gem.rb +++ b/test/rubygems/test_gem.rb @@ -941,6 +941,51 @@ class TestGem < Gem::TestCase RbConfig::CONFIG["sitelibdir"] = orig_sitelibdir end + def test_self_open_file_with_lock_excludes_late_arrivals_after_handover + path = File.join(@tempdir, "resource") + events = Queue.new + release_waiter = Queue.new + + on_warn = lambda do |message| + events << [Thread.current.name, :waiting] if message.include?("Waiting for another process") + end + + next_event = lambda do + events.pop(timeout: 10) || flunk("Timed out waiting for a lock event") + end + + contender = lambda do |name, &block| + Thread.new do + Thread.current.name = name + Thread.current.abort_on_exception = true + Gem.open_file_with_lock(path) do + events << [name, :locked] + block&.call + end + end + end + + Gem.stub(:warn, on_warn) do + waiter = nil + Gem.open_file_with_lock(path) do + waiter = contender.call("waiter") { release_waiter.pop } + assert_equal ["waiter", :waiting], next_event.call + end + assert_equal ["waiter", :locked], next_event.call + + late_arrival = contender.call("late_arrival") + assert_equal ["late_arrival", :waiting], next_event.call + + release_waiter << true + [waiter, late_arrival].each(&:join) + assert_equal ["late_arrival", :locked], next_event.call + ensure + release_waiter << true + end + + assert_path_not_exist "#{path}.lock" + end + def test_self_read_binary File.open "test", "w" do |io| io.write "\xCF\x80"