fix: concurrent callers each spawned their own git rev-parse

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

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