From e347e74f6879830f394d4b145b9f6e208e6f272b Mon Sep 17 00:00:00 2001
From: Paul Keen <125715+pftg@users.noreply.github.com>
Date: Mon, 24 Aug 2026 08:06:38 +0200
Subject: [PATCH] fix: concurrent callers each spawned their own git rev-parse

The per-root cache added in #250 removed 200 process spawns from a
200-screenshot serial suite, but bought nothing under threads. MRI
releases the GVL for the whole of `Open3.capture3`, so every thread
misses `key?` before any thread writes: measured 8 spawns for 8
threads asking about a single root.

That is the default parallel mode on JRuby, which additionally has no
GVL to make the unsynchronized Hash write safe.

Synchronize the lookup itself, not just the write. Holding the lock
across the spawn is deliberate -- callers almost always share one root,
so the other threads wait once and then read the cache.
---
 lib/snap_diff/vcs.rb  | 18 +++++++++++++++---
 test/unit/vcs_test.rb | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/lib/snap_diff/vcs.rb b/lib/snap_diff/vcs.rb
index 87205df3..a5b34081 100644
--- a/lib/snap_diff/vcs.rb
+++ b/lib/snap_diff/vcs.rb
@@ -6,6 +6,7 @@
 module SnapDiff
   module Vcs
     @git_roots = {}
+    @git_roots_lock = Mutex.new
 
     def self.checkout_vcs(root, screenshot_path, checkout_path)
       root_path = root.to_s
@@ -39,11 +40,22 @@ def self.checkout_vcs(root, screenshot_path, checkout_path)
     # while the suite runs, so remember it per directory. `false` (not a repo)
     # is remembered too: that is the every-assertion answer for anyone whose
     # screenshots live outside a git checkout.
+    #
+    # Synchronized because the lookup itself is what must not be duplicated:
+    # MRI releases the GVL for the whole of `Open3.capture3`, so eight threads
+    # asking about one root all miss `key?` before any of them writes -- eight
+    # spawns, the exact cost this cache exists to remove. Threads are the
+    # default parallel mode on JRuby, which has no GVL to make the Hash write
+    # safe either. Holding the lock across the spawn is deliberate: callers
+    # almost always share one root, so the other threads wait once and then
+    # read the cache, which is the outcome we want.
     def self.git_root_for(root_path)
-      return @git_roots[root_path] if @git_roots.key?(root_path)
+      @git_roots_lock.synchronize do
+        next @git_roots[root_path] if @git_roots.key?(root_path)
 
-      git_root, _, status = Open3.capture3("git", "-C", root_path, "rev-parse", "--show-toplevel")
-      @git_roots[root_path] = status.success? && git_root.chomp
+        git_root, _, status = Open3.capture3("git", "-C", root_path, "rev-parse", "--show-toplevel")
+        @git_roots[root_path] = status.success? && git_root.chomp
+      end
     end
   end
 end
diff --git a/test/unit/vcs_test.rb b/test/unit/vcs_test.rb
index 72588620..83ab98b3 100644
--- a/test/unit/vcs_test.rb
+++ b/test/unit/vcs_test.rb
@@ -54,4 +54,36 @@ def status.success? = true
 
     assert_equal 1, calls
   end
+
+  # Concurrent callers must share the cached answer, not each spawn their own
+  # git. MRI releases the GVL for the duration of `Open3.capture3`, so without
+  # synchronization every thread misses `key?` before any thread writes --
+  # measured 8 spawns for 8 threads asking about ONE root, i.e. the cache
+  # bought nothing in exactly the mode (`parallelize(with: :threads)`) that is
+  # JRuby's default. On JRuby there is no GVL at all, so the unsynchronized
+  # Hash write is unsafe as well as wasteful.
+  test "#checkout_vcs shares one git lookup across concurrent callers" do
+    root = @tmp_dir / "concurrent_root_#{Time.now.nsec}"
+    FileUtils.mkdir_p(root)
+    screenshot_path = file_fixture("images/a.png")
+    base_screenshot_path = Pathname.new(@base_screenshot.path)
+
+    calls = 0
+    counter_lock = Mutex.new
+    status = Object.new
+    def status.success? = true
+    counting_rev_parse = ->(*) {
+      counter_lock.synchronize { calls += 1 }
+      sleep 0.01 # stand in for the real subprocess, which releases the GVL
+      ["#{PROJECT_ROOT}\n", "", status]
+    }
+
+    Open3.stub(:capture3, counting_rev_parse) do
+      8.times.map {
+        Thread.new { SnapDiff::Vcs.checkout_vcs(root, screenshot_path, base_screenshot_path) }
+      }.each(&:join)
+    end
+
+    assert_equal 1, calls, "concurrent callers must share one git lookup, not spawn one each"
+  end
 end