Flush atomic write temp files before renaming over the destination

The temporary file was renamed into place before being closed, so
buffered data had not reached the filesystem yet and a concurrent reader
could observe an empty destination. A flush failure such as ENOSPC would
surface only after the destination had already been replaced. Cleanup
also moved to an ensure block, so an interrupt no longer leaks the
temporary file.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: hsbt

lib/rubygems/util/atomic_file_writer.rb

@@ -45,47 +45,50 @@ def self.open(file_name)
       flags = File::RDWR | File::CREAT | File::EXCL | File::BINARY
       flags |= File::SHARE_DELETE if defined?(File::SHARE_DELETE)
 
-      File.open(tmp_path, flags) do |temp_file|
+      renamed = false
+      temp_file = File.open(tmp_path, flags)
+
+      begin
         temp_file.binmode
         if old_stat
           # Set correct permissions on new file
           begin
-            File.chown(old_stat.uid, old_stat.gid, temp_file.path)
+            File.chown(old_stat.uid, old_stat.gid, tmp_path)
             # This operation will affect filesystem ACL's
-            File.chmod(old_stat.mode, temp_file.path)
+            File.chmod(old_stat.mode, tmp_path)
           rescue Errno::EPERM, Errno::EACCES
             # Changing file ownership failed, moving on.
           end
         end
 
         return_val = yield temp_file
-      rescue StandardError => error
-        begin
-          temp_file.close
-        rescue StandardError
-          nil
-        end
 
-        begin
-          File.unlink(temp_file.path)
-        rescue StandardError
-          nil
-        end
+        # Any data still buffered is handed to the filesystem on close, so a
+        # failing flush must surface while the destination is still intact.
+        # That means closing the temporary file before the rename. Note this
+        # does not fsync, so the write is atomic but not crash durable.
+        temp_file.close
+        File.rename(tmp_path, file_name)
+        renamed = true
 
-        raise error
-      else
-        begin
-          File.rename(temp_file.path, file_name)
-        rescue StandardError
+        return_val
+      ensure
+        unless renamed
           begin
-            File.unlink(temp_file.path)
+            temp_file.close
           rescue StandardError
+            nil
+          ensure
+            # The unlink runs from an ensure so that a non-StandardError raised by
+            # the close above, a second Ctrl-C for instance, still reaches it. An
+            # interrupt landing on the unlink itself is not covered.
+            begin
+              File.unlink(tmp_path)
+            rescue StandardError
+              nil
+            end
           end
-
-          raise
         end
-
-        return_val
       end
     end
 

test/rubygems/test_gem_util_atomic_file_writer.rb

@@ -18,6 +18,151 @@ def test_external_encoding
     end
   end
 
+  def test_returns_block_value_and_leaves_no_temp_file
+    result = Gem::AtomicFileWriter.open(@path) do |file|
+      file.write "hello"
+      :done
+    end
+
+    assert_equal :done, result
+    assert_equal "hello", File.binread(@path)
+    assert_equal ["out.txt"], Dir.children(@dir)
+  end
+
+  def test_content_is_flushed_before_rename
+    original_rename = File.method(:rename)
+    size_at_rename = nil
+
+    rename_spy = lambda do |src, dest|
+      size_at_rename = File.size(src)
+      original_rename.call(src, dest)
+    end
+
+    File.stub(:rename, rename_spy) do
+      Gem::AtomicFileWriter.open(@path) do |file|
+        file.write "hello"
+      end
+    end
+
+    assert_equal 5, size_at_rename
+    assert_equal "hello", File.binread(@path)
+  end
+
+  def test_keeps_destination_and_removes_temp_file_on_error
+    File.binwrite @path, "old"
+
+    error = assert_raise(RuntimeError) do
+      Gem::AtomicFileWriter.open(@path) do |file|
+        file.write "new"
+        raise "boom"
+      end
+    end
+
+    assert_equal "boom", error.message
+    assert_equal "old", File.binread(@path)
+    assert_equal ["out.txt"], Dir.children(@dir)
+  end
+
+  def test_keeps_destination_and_removes_temp_file_on_interrupt
+    File.binwrite @path, "old"
+
+    assert_raise(Interrupt) do
+      Gem::AtomicFileWriter.open(@path) do |file|
+        file.write "new"
+        raise Interrupt
+      end
+    end
+
+    assert_equal "old", File.binread(@path)
+    assert_equal ["out.txt"], Dir.children(@dir)
+  end
+
+  def test_removes_temp_file_when_closing_it_keeps_raising
+    File.binwrite @path, "old"
+
+    temp_file = nil
+    closes = 0
+    raising = true
+    # A real file is closed even when its close raises, so only a stub can keep
+    # raising like this. The writer leaves that file to the garbage collector,
+    # which is why this test closes it itself.
+    failing_open = temp_file_open_stub do |file|
+      temp_file = file
+      file.define_singleton_method(:close) do
+        closes += 1
+        raise Interrupt if raising
+
+        super()
+      end
+    end
+
+    File.stub(:open, failing_open) do
+      assert_raise(Interrupt) do
+        Gem::AtomicFileWriter.open(@path) do |file|
+          file.write "new"
+        end
+      end
+    end
+
+    assert_equal 2, closes
+    assert_equal "old", File.binread(@path)
+    assert_equal ["out.txt"], Dir.children(@dir)
+  ensure
+    raising = false
+    temp_file&.close
+  end
+
+  def test_keeps_destination_when_closing_the_temp_file_fails
+    File.binwrite @path, "old"
+
+    failing_open = temp_file_open_stub do |file|
+      file.define_singleton_method(:close) do
+        super()
+        raise Errno::ENOSPC
+      end
+    end
+
+    File.stub(:open, failing_open) do
+      assert_raise(Errno::ENOSPC) do
+        Gem::AtomicFileWriter.open(@path) do |file|
+          file.write "new"
+        end
+      end
+    end
+
+    assert_equal "old", File.binread(@path)
+    assert_equal ["out.txt"], Dir.children(@dir)
+  end
+
+  def test_keeps_destination_when_renaming_fails
+    File.binwrite @path, "old"
+
+    File.stub(:rename, ->(_src, _dest) { raise Errno::EXDEV }) do
+      assert_raise(Errno::EXDEV) do
+        Gem::AtomicFileWriter.open(@path) do |file|
+          file.write "new"
+        end
+      end
+    end
+
+    assert_equal "old", File.binread(@path)
+    assert_equal ["out.txt"], Dir.children(@dir)
+  end
+
+  def test_preserves_destination_permissions
+    pend "Windows cannot round-trip the POSIX permission bits" if Gem.win_platform?
+
+    File.binwrite @path, "old"
+    File.chmod 0o604, @path
+
+    Gem::AtomicFileWriter.open(@path) do |file|
+      file.write "new"
+    end
+
+    assert_equal "new", File.binread(@path)
+    assert_equal 0o604, File.stat(@path).mode & 0o777
+  end
+
   def test_multibyte_basename_is_truncated_at_char_boundary
     pend "long file names easily exceed MAX_PATH on Windows" if Gem.win_platform?
 
@@ -82,4 +227,31 @@ def test_byteslice_at_char_boundary_with_invalid_encoding
 
     assert_equal "\xFFa".dup.force_encoding(Encoding::UTF_8), sliced
   end
+
+  private
+
+  # Returns a File.open replacement that hands the writer's temporary file to
+  # the given block, which installs whatever close behaviour a test needs.
+  # Every other path is opened normally. Call this before installing the stub,
+  # since it captures the current File.open.
+  def temp_file_open_stub(&injection)
+    original_open = File.method(:open)
+    prefix = ".#{File.basename(@path)}.tmp."
+
+    lambda do |name, *args, **kwargs, &block|
+      unless File.basename(name.to_s).start_with?(prefix)
+        next original_open.call(name, *args, **kwargs, &block)
+      end
+      raise ArgumentError, "this stub cannot replace close on a File.open with a block" if block
+
+      file = original_open.call(name, *args, **kwargs)
+      begin
+        injection.call(file)
+      rescue StandardError
+        file.close
+        raise
+      end
+      file
+    end
+  end
 end