fix: libvips serves stale pixels when a screenshot is rewritten within the same second

lib/snap_diff/drivers/vips_driver.rb

@@ -18,6 +18,17 @@ module Drivers
     class VipsDriver
       include SnapDiff::Driver
 
+      # libvips caches loader operations keyed on filename + mtime, and mtime
+      # has ONE-SECOND resolution -- so overwriting a path and re-reading it
+      # within the same second hands back the PREVIOUS image. This gem does
+      # exactly that: the screenshoter writes `<name>.png`,
+      # `checkout_base_screenshot` writes `<name>.base.png` from VCS, and the
+      # comparison then reads both.
+      #
+      # `revalidate: true` tells the loader to skip the cached result (libvips
+      # 8.15+).
+      REVALIDATE = Vips.at_least_libvips?(8, 15) ? {revalidate: true}.freeze : {}.freeze
+
       def find_difference_region(comparison)
         new_image, base_image, options = comparison.new_image, comparison.base_image, comparison.options
 
@@ -86,7 +97,7 @@ def load_images(old_file_name, new_file_name)
       end
 
       def from_file(filename)
-        result = ::Vips::Image.new_from_file(filename.to_s)
+        result = ::Vips::Image.new_from_file(filename.to_s, **REVALIDATE)
 
         result = result.colourspace(:srgb) if result.bands < 3
         result = result.bandjoin(255) if result.bands == 3

test/system_test_case.rb

@@ -51,10 +51,11 @@ class SystemTestCase < ActiveSupport::TestCase
     SnapDiff.config.tolerance = @orig_tolerance
     Capybara.current_driver = Capybara.default_driver
 
-    if SnapDiff.config.driver == :vips
-      Vips.cache_set_max(0)
-      Vips.cache_set_max(1000)
-    end
+    # The vips cache flush that used to live here (cache_set_max 0 then 1000)
+    # papered over libvips serving a stale image when a screenshot path was
+    # rewritten within the same second. VipsDriver#from_file now passes
+    # `revalidate: true`, so the workaround is gone -- see the regression test
+    # in test/unit/drivers/vips_driver_test.rb.
   end
 
   private

test/unit/drivers/vips_driver_test.rb

@@ -24,11 +24,36 @@ class VipsDriverTest < ActiveSupport::TestCase
           @new_screenshot_result.close
           @new_screenshot_result.unlink
         end
+      end
 
-        if defined?(Vips)
-          Vips.cache_set_max(0)
-          Vips.cache_set_max(1000)
-        end
+      # REGRESSION. libvips caches loaders on filename + mtime, and mtime has
+      # one-second resolution, so rewriting a path and re-reading it within the
+      # same second used to hand back the PREVIOUS image. #from_file passes
+      # `revalidate: true` to defeat that.
+      #
+      # The gem does exactly this: the screenshoter writes `<name>.png`,
+      # `checkout_base_screenshot` writes `<name>.base.png` from VCS, and the
+      # comparison then reads both -- a stale read compares against an image
+      # that is no longer on disk.
+      #
+      # The teardown above used to flush the whole vips cache
+      # (`Vips.cache_set_max(0); Vips.cache_set_max(1000)`) to paper over this;
+      # with the driver fixed, that workaround is gone.
+      test "#from_file re-reads a path that was overwritten within the same second" do
+        driver = VipsDriver.new
+        path = Rails.root / "revalidate_probe.png"
+
+        FileUtils.cp(TEST_IMAGES_DIR / "b.png", path)
+        first_avg = driver.from_file(path).avg # force evaluation BEFORE the overwrite
+
+        FileUtils.cp(TEST_IMAGES_DIR / "a.png", path)
+        second_avg = driver.from_file(path).avg
+
+        assert_not_equal first_avg, second_avg,
+          "vips served the cached b.png after the path was overwritten with a.png"
+        assert_equal driver.from_file(TEST_IMAGES_DIR / "a.png").avg, second_avg
+      ensure
+        FileUtils.rm_f(path)
       end
 
       test "#different? returns false when comparing identical images" do