From db07eea3d1996ae5acd7088af016bd20c8c26c7c Mon Sep 17 00:00:00 2001 From: Paul Keen <125715+pftg@users.noreply.github.com> Date: Mon, 24 Aug 2026 08:09:26 +0200 Subject: [PATCH] fix: libvips serves stale pixels when a screenshot is rewritten within the same second libvips caches loader operations keyed on filename + mtime, and mtime has one-second resolution. Overwrite a path and re-read it within the same second and the loader hands back the PREVIOUS image. This gem does exactly that: the screenshoter writes `.png`, `checkout_base_screenshot` writes `.base.png` from VCS, and the comparison then reads both. The result is a comparison against an image that is no longer on disk -- a pass or a bogus diff the user cannot reproduce. `VipsDriver#from_file` is the single load site and every caller routes through it (`load_images`, `Screenshoter#take_screenshot`), so one keyword at that seam fixes all of them. `revalidate: true` is libvips 8.15+, hence the `Vips.at_least_libvips?` guard. Ported from #249, which fixes the same seam on the 2.1 branch. The vips cache flush in `test/system_test_case.rb` teardown (`cache_set_max(0)` then `1000`) was a workaround for this, sitting in the harness where it protected our own suite while the bug shipped to users. It is gone; the regression test in `test/unit/drivers/vips_driver_test.rb` guards the seam instead. Cost: none. Measured over 30 distinct-path reads per shape (the gem reads each path at most once per comparison, so the loader cache never helps either way): 80x60, 800x600, 1920x1080 and 1280x4000 all land within +/-0.9 ms and the sign of the delta flips between runs. --- lib/snap_diff/drivers/vips_driver.rb | 13 ++++++++++- test/system_test_case.rb | 9 ++++---- test/unit/drivers/vips_driver_test.rb | 33 +++++++++++++++++++++++---- 3 files changed, 46 insertions(+), 9 deletions(-) diff --git a/lib/snap_diff/drivers/vips_driver.rb b/lib/snap_diff/drivers/vips_driver.rb index 0cd91f0a..e9e3ec46 100644 --- a/lib/snap_diff/drivers/vips_driver.rb +++ b/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 `.png`, + # `checkout_base_screenshot` writes `.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 diff --git a/test/system_test_case.rb b/test/system_test_case.rb index e2e419e1..f96d176c 100644 --- a/test/system_test_case.rb +++ b/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 diff --git a/test/unit/drivers/vips_driver_test.rb b/test/unit/drivers/vips_driver_test.rb index abf22f53..1c70cfdb 100644 --- a/test/unit/drivers/vips_driver_test.rb +++ b/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 `.png`, + # `checkout_base_screenshot` writes `.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