From cfa66e442d3c38712989ac2672dd63ec9810d0f9 Mon Sep 17 00:00:00 2001
From: Paul Keen <125715+pftg@users.noreply.github.com>
Date: Sun, 12 Apr 2026 20:20:49 +0200
Subject: [PATCH 1/5] Make snap_diff reporter notifications safer

---
 .../reporters/html.rb                         | 28 ++++++++---
 .../screenshot_assertion.rb                   | 11 ++++-
 test/unit/reporters/html_reporter_test.rb     | 26 ++++++++++
 test/unit/reporters_mutex_test.rb             | 49 +++++++++++++++++++
 4 files changed, 105 insertions(+), 9 deletions(-)
 create mode 100644 test/unit/reporters_mutex_test.rb

diff --git a/lib/capybara_screenshot_diff/reporters/html.rb b/lib/capybara_screenshot_diff/reporters/html.rb
index 57f910bf..b51fd305 100644
--- a/lib/capybara_screenshot_diff/reporters/html.rb
+++ b/lib/capybara_screenshot_diff/reporters/html.rb
@@ -18,30 +18,44 @@ def initialize(output_path: "tmp/snap_diff/index.html", embed_images: false)
         @failures = []
         @total = 0
         @finalized = false
+        @mutex = Mutex.new
       end
 
       def record(assertions)
+        return if @finalized
+
+        failures = []
+        total = 0
+
         assertions.each do |assertion|
           compare = assertion.compare
           next unless compare
 
-          @total += 1
+          total += 1
           next unless compare.difference&.different?
 
-          @failures << failure_entry_for(assertion.name, compare)
+          failures << failure_entry_for(assertion.name, compare)
         rescue => e
           warn "[snap_diff] Reporter skipped '#{assertion.name}': #{e.message}" if ENV["DEBUG"]
         end
+
+        @mutex.synchronize do
+          return if @finalized
+          @total += total
+          @failures.concat(failures)
+        end
       end
 
       def finalize
-        return if @finalized
+        @mutex.synchronize do
+          return if @finalized
 
-        @finalized = true
-        return if failures.empty?
+          @finalized = true
+          return if failures.empty?
 
-        write_report
-        output_path
+          write_report
+          output_path
+        end
       end
 
       def passed = total - failures.size
diff --git a/lib/capybara_screenshot_diff/screenshot_assertion.rb b/lib/capybara_screenshot_diff/screenshot_assertion.rb
index ba1cb67c..0f40aa46 100644
--- a/lib/capybara_screenshot_diff/screenshot_assertion.rb
+++ b/lib/capybara_screenshot_diff/screenshot_assertion.rb
@@ -133,15 +133,22 @@ def reporters
       @reporters ||= []
     end
 
+    def reporters_mutex
+      @reporters_mutex ||= Mutex.new
+    end
+
     def_delegator :registry, :screenshot_namer
     def_delegator :registry, :verify
 
     private
 
     def notify_reporters(assertions)
-      return if reporters.empty? || assertions.nil? || assertions.empty?
+      return if assertions.nil? || assertions.empty?
+
+      reporters_snapshot = reporters_mutex.synchronize { reporters.dup }
+      return if reporters_snapshot.empty?
 
-      reporters.each do |reporter|
+      reporters_snapshot.each do |reporter|
         reporter.record(assertions)
       rescue => e
         warn "[capybara-screenshot-diff] Reporter failed: #{e.message}"
diff --git a/test/unit/reporters/html_reporter_test.rb b/test/unit/reporters/html_reporter_test.rb
index c3611342..9c7cb9b3 100644
--- a/test/unit/reporters/html_reporter_test.rb
+++ b/test/unit/reporters/html_reporter_test.rb
@@ -98,6 +98,32 @@ class HTMLReporterTest < ActiveSupport::TestCase
         assert_includes html, "data:image/png;base64,"
       end
 
+      test "#record and #finalize synchronize internal state" do
+        reporter = HTML.new(output_path: @output_path)
+
+        fake_mutex = Class.new do
+          attr_reader :synchronize_calls
+
+          def initialize
+            @synchronize_calls = 0
+          end
+
+          def synchronize
+            @synchronize_calls += 1
+            yield
+          end
+        end.new
+
+        # Using private state and a fake mutex here is intentional.
+        # This test guards a tricky synchronization detail; avoid refactors unless behavior changes.
+        reporter.instance_variable_set(:@mutex, fake_mutex)
+
+        reporter.record([build_passing_assertion("sync")])
+        reporter.finalize
+
+        assert_operator fake_mutex.synchronize_calls, :>=, 1
+      end
+
       private
 
       def build_passing_assertion(name)
diff --git a/test/unit/reporters_mutex_test.rb b/test/unit/reporters_mutex_test.rb
new file mode 100644
index 00000000..fbca332f
--- /dev/null
+++ b/test/unit/reporters_mutex_test.rb
@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+require "test_helper"
+
+module CapybaraScreenshotDiff
+  class ReportersMutexTest < ActiveSupport::TestCase
+    setup do
+      @original_reporters = CapybaraScreenshotDiff.reporters.dup
+      CapybaraScreenshotDiff.reporters.clear
+    end
+
+    teardown do
+      CapybaraScreenshotDiff.reporters.clear
+      CapybaraScreenshotDiff.reporters.concat(@original_reporters)
+      CapybaraScreenshotDiff.instance_variable_set(:@reporters_mutex, nil)
+    end
+
+    test "reporters notification uses mutex snapshot" do
+      fake_mutex = Class.new do
+        attr_reader :synchronize_calls
+
+        def initialize
+          @synchronize_calls = 0
+        end
+
+        def synchronize
+          @synchronize_calls += 1
+          yield
+        end
+      end.new
+
+      CapybaraScreenshotDiff.instance_variable_set(:@reporters_mutex, fake_mutex)
+
+      reporter = Class.new do
+        def record(_assertions); end
+      end.new
+
+      CapybaraScreenshotDiff.reporters << reporter
+
+      assertion = CapybaraScreenshotDiff::ScreenshotAssertion.new("sample")
+      assertion.compare = Object.new
+      CapybaraScreenshotDiff.add_assertion(assertion)
+
+      CapybaraScreenshotDiff.reset
+
+      assert_operator fake_mutex.synchronize_calls, :>=, 1
+    end
+  end
+end

From ad46203a995da26cb4dd909b4638eaa178931413 Mon Sep 17 00:00:00 2001
From: Paul Keen <125715+pftg@users.noreply.github.com>
Date: Sun, 12 Apr 2026 20:22:48 +0200
Subject: [PATCH 2/5] Update thread safety doc for snap_diff

---
 docs/THREAD_SAFETY.md | 93 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)
 create mode 100644 docs/THREAD_SAFETY.md

diff --git a/docs/THREAD_SAFETY.md b/docs/THREAD_SAFETY.md
new file mode 100644
index 00000000..546fae32
--- /dev/null
+++ b/docs/THREAD_SAFETY.md
@@ -0,0 +1,93 @@
+# thread safety guide for parallel testing
+
+this document explains how `snap_diff` behaves under rails parallel tests with the `:thread` strategy.
+
+## overview
+
+`snap_diff` is thread safe for parallel test execution as long as global configuration is set before tests run. per thread state is isolated, and shared state is protected where it matters.
+
+## architecture summary
+
+### per-thread assertion registry
+
+each thread gets its own `assertionregistry` stored in thread-local storage:
+
+```ruby
+def registry
+  thread.current[:capybara_screenshot_diff_registry] ||= assertionregistry.new
+end
+```
+
+this prevents cross-thread leakage for assertions and screenshot naming.
+
+### reporters snapshot on notify
+
+reporters are notified using a snapshot protected by a mutex:
+
+```ruby
+def notify_reporters(assertions)
+  reporters_snapshot = reporters_mutex.synchronize { reporters.dup }
+  reporters_snapshot.each { |reporter| reporter.record(assertions) }
+end
+```
+
+this ensures a stable list while notifying without forcing a global lock around reporter work.
+
+### html reporter internal lock
+
+the html reporter protects `@failures`, `@total`, and `@finalized` with a mutex so record and finalize can run safely:
+
+```ruby
+@mutex.synchronize do
+  return if @finalized
+  @total += total
+  @failures.concat(failures)
+end
+```
+
+### screenshot naming isolation
+
+each thread gets its own `screenshotnamer` via the per-thread registry, so counters, sections, and groups do not collide.
+
+### snapshot manager per call
+
+`snapmanager.instance` returns a new instance for each call, avoiding shared mutable state.
+
+## global configuration
+
+configuration uses `mattr_accessor` and should be set once before tests run. do not mutate config during parallel execution.
+
+## parallel test lifecycle
+
+- setup: per-thread registry is created, config is read
+- execution: assertions are added to the thread-local registry
+- teardown: verify and reset operate on the thread-local registry, reporters are notified
+- exit: reporters finalize once per process
+
+## usage examples
+
+```ruby
+parallelize(workers: :number_of_processors, with: :threads)
+
+capybara::screenshot::diff.configure do |screenshot, diff|
+  screenshot.window_size = [1280, 1024]
+  screenshot.save_path = "doc/screenshots"
+  diff.tolerance = 0.001
+end
+```
+
+## do and do not
+
+do:
+- set config once in test helper
+- pass per-screenshot options in the call
+
+do not:
+- change global config inside tests
+- manually mutate registry internals
+
+## file system notes
+
+- paths are unique per screenshot name and counter
+- `fileutils.mv` is atomic on most file systems
+- directory creation uses `mkpath`

From 330d0689f9ffd734ed50c01096349e7929cef90c Mon Sep 17 00:00:00 2001
From: Paul Keen <125715+pftg@users.noreply.github.com>
Date: Sun, 12 Apr 2026 20:25:47 +0200
Subject: [PATCH 3/5] Rename thread safety doc

---
 docs/{THREAD_SAFETY.md => thread_safety.md} | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename docs/{THREAD_SAFETY.md => thread_safety.md} (100%)

diff --git a/docs/THREAD_SAFETY.md b/docs/thread_safety.md
similarity index 100%
rename from docs/THREAD_SAFETY.md
rename to docs/thread_safety.md

From 3b564f507554f964fcb7355166425e616441e540 Mon Sep 17 00:00:00 2001
From: Paul Keen <125715+pftg@users.noreply.github.com>
Date: Sun, 12 Apr 2026 20:28:06 +0200
Subject: [PATCH 4/5] Cover reporters snapshot mutation

---
 test/unit/reporters_mutex_test.rb | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/test/unit/reporters_mutex_test.rb b/test/unit/reporters_mutex_test.rb
index fbca332f..b922ac5a 100644
--- a/test/unit/reporters_mutex_test.rb
+++ b/test/unit/reporters_mutex_test.rb
@@ -45,5 +45,29 @@ def record(_assertions); end
 
       assert_operator fake_mutex.synchronize_calls, :>=, 1
     end
+
+    test "reporters notification iterates over snapshot" do
+      received = []
+
+      mutating_reporter = Class.new do
+        define_method :record do |assertions|
+          received << [:original, assertions]
+          CapybaraScreenshotDiff.reporters.clear
+          CapybaraScreenshotDiff.reporters << Class.new {
+            define_method(:record) { |a| received << [:added, a] }
+          }.new
+        end
+      end.new
+
+      CapybaraScreenshotDiff.reporters << mutating_reporter
+
+      assertions = [:some, :assertions]
+
+      assert_nothing_raised do
+        CapybaraScreenshotDiff.send(:notify_reporters, assertions)
+      end
+
+      assert_equal [[:original, assertions]], received
+    end
   end
 end

From 18c964144c25ca91723f88ce8cd84f271cb4336b Mon Sep 17 00:00:00 2001
From: Paul Keen <125715+pftg@users.noreply.github.com>
Date: Sun, 12 Apr 2026 20:38:57 +0200
Subject: [PATCH 5/5] Fix thread safety issues found in review

- Eagerly initialize reporters_mutex to prevent race on lazy init
- Move @finalized flag after write_report so finalize can retry on failure
- Use mutex-protected snapshot in at_exit hook for consistency
- Fix THREAD_SAFETY.md: use proper Ruby class names, document eager mutex
- Add real multi-threaded concurrency test (10 threads x 5 assertions)
- Add finalize retry-after-failure test

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---
 docs/thread_safety.md                         | 80 ++++++++++---------
 .../reporters/html.rb                         |  5 +-
 .../screenshot_assertion.rb                   |  6 +-
 test/unit/reporters/html_reporter_test.rb     | 38 +++++++++
 test/unit/reporters_mutex_test.rb             | 34 ++------
 5 files changed, 90 insertions(+), 73 deletions(-)

diff --git a/docs/thread_safety.md b/docs/thread_safety.md
index 546fae32..65731249 100644
--- a/docs/thread_safety.md
+++ b/docs/thread_safety.md
@@ -1,41 +1,43 @@
-# thread safety guide for parallel testing
+# Thread Safety Guide for Parallel Testing
 
-this document explains how `snap_diff` behaves under rails parallel tests with the `:thread` strategy.
+This document explains how `snap_diff` behaves under Rails parallel tests with the `:thread` strategy.
 
-## overview
+## Overview
 
-`snap_diff` is thread safe for parallel test execution as long as global configuration is set before tests run. per thread state is isolated, and shared state is protected where it matters.
+`snap_diff` is thread safe for parallel test execution as long as global configuration is set before tests run. Per-thread state is isolated, and shared state is protected where it matters.
 
-## architecture summary
+## Architecture Summary
 
-### per-thread assertion registry
+### Per-thread Assertion Registry
 
-each thread gets its own `assertionregistry` stored in thread-local storage:
+Each thread gets its own `AssertionRegistry` stored in thread-local storage:
 
 ```ruby
 def registry
-  thread.current[:capybara_screenshot_diff_registry] ||= assertionregistry.new
+  Thread.current[:capybara_screenshot_diff_registry] ||= AssertionRegistry.new
 end
 ```
 
-this prevents cross-thread leakage for assertions and screenshot naming.
+This prevents cross-thread leakage for assertions and screenshot naming.
 
-### reporters snapshot on notify
+### Reporters Snapshot on Notify
 
-reporters are notified using a snapshot protected by a mutex:
+Reporters are notified using a snapshot protected by an eagerly initialized mutex:
 
 ```ruby
+@reporters_mutex = Mutex.new
+
 def notify_reporters(assertions)
   reporters_snapshot = reporters_mutex.synchronize { reporters.dup }
   reporters_snapshot.each { |reporter| reporter.record(assertions) }
 end
 ```
 
-this ensures a stable list while notifying without forcing a global lock around reporter work.
+This ensures a stable list while notifying without forcing a global lock around reporter work.
 
-### html reporter internal lock
+### HTML Reporter Internal Lock
 
-the html reporter protects `@failures`, `@total`, and `@finalized` with a mutex so record and finalize can run safely:
+The HTML reporter protects `@failures`, `@total`, and `@finalized` with a mutex so `record` and `finalize` can run safely:
 
 ```ruby
 @mutex.synchronize do
@@ -45,49 +47,51 @@ the html reporter protects `@failures`, `@total`, and `@finalized` with a mutex
 end
 ```
 
-### screenshot naming isolation
+`@finalized` is set only after `write_report` succeeds, so a failed write can be retried.
+
+### Screenshot Naming Isolation
 
-each thread gets its own `screenshotnamer` via the per-thread registry, so counters, sections, and groups do not collide.
+Each thread gets its own `ScreenshotNamer` via the per-thread registry, so counters, sections, and groups do not collide.
 
-### snapshot manager per call
+### SnapManager Per Call
 
-`snapmanager.instance` returns a new instance for each call, avoiding shared mutable state.
+`SnapManager` returns a new instance for each call, avoiding shared mutable state.
 
-## global configuration
+## Global Configuration
 
-configuration uses `mattr_accessor` and should be set once before tests run. do not mutate config during parallel execution.
+Configuration uses `mattr_accessor` and should be set once before tests run. Do not mutate config during parallel execution.
 
-## parallel test lifecycle
+## Parallel Test Lifecycle
 
-- setup: per-thread registry is created, config is read
-- execution: assertions are added to the thread-local registry
-- teardown: verify and reset operate on the thread-local registry, reporters are notified
-- exit: reporters finalize once per process
+- Setup: per-thread registry is created, config is read
+- Execution: assertions are added to the thread-local registry
+- Teardown: `verify` and `reset` operate on the thread-local registry, reporters are notified
+- Exit: reporters finalize once per process (using mutex-protected snapshot)
 
-## usage examples
+## Usage Examples
 
 ```ruby
 parallelize(workers: :number_of_processors, with: :threads)
 
-capybara::screenshot::diff.configure do |screenshot, diff|
+Capybara::Screenshot::Diff.configure do |screenshot, diff|
   screenshot.window_size = [1280, 1024]
   screenshot.save_path = "doc/screenshots"
   diff.tolerance = 0.001
 end
 ```
 
-## do and do not
+## Do and Do Not
 
-do:
-- set config once in test helper
-- pass per-screenshot options in the call
+Do:
+- Set config once in test helper
+- Pass per-screenshot options in the call
 
-do not:
-- change global config inside tests
-- manually mutate registry internals
+Do not:
+- Change global config inside tests
+- Manually mutate registry internals
 
-## file system notes
+## File System Notes
 
-- paths are unique per screenshot name and counter
-- `fileutils.mv` is atomic on most file systems
-- directory creation uses `mkpath`
+- Paths are unique per screenshot name and counter
+- `FileUtils.mv` is atomic on most file systems
+- Directory creation uses `mkpath`
diff --git a/lib/capybara_screenshot_diff/reporters/html.rb b/lib/capybara_screenshot_diff/reporters/html.rb
index b51fd305..92349327 100644
--- a/lib/capybara_screenshot_diff/reporters/html.rb
+++ b/lib/capybara_screenshot_diff/reporters/html.rb
@@ -49,11 +49,10 @@ def record(assertions)
       def finalize
         @mutex.synchronize do
           return if @finalized
-
-          @finalized = true
           return if failures.empty?
 
           write_report
+          @finalized = true
           output_path
         end
       end
@@ -122,7 +121,7 @@ def write_report
 end
 
 at_exit do
-  CapybaraScreenshotDiff.reporters.each do |reporter|
+  CapybaraScreenshotDiff.reporters_mutex.synchronize { CapybaraScreenshotDiff.reporters.dup }.each do |reporter|
     result = reporter.finalize
     $stdout.puts "[snap_diff] HTML report: #{result}" if result.is_a?(Pathname)
   rescue => e
diff --git a/lib/capybara_screenshot_diff/screenshot_assertion.rb b/lib/capybara_screenshot_diff/screenshot_assertion.rb
index 0f40aa46..9faffce3 100644
--- a/lib/capybara_screenshot_diff/screenshot_assertion.rb
+++ b/lib/capybara_screenshot_diff/screenshot_assertion.rb
@@ -133,9 +133,7 @@ def reporters
       @reporters ||= []
     end
 
-    def reporters_mutex
-      @reporters_mutex ||= Mutex.new
-    end
+    attr_reader :reporters_mutex
 
     def_delegator :registry, :screenshot_namer
     def_delegator :registry, :verify
@@ -155,4 +153,6 @@ def notify_reporters(assertions)
       end
     end
   end
+
+  @reporters_mutex = Mutex.new
 end
diff --git a/test/unit/reporters/html_reporter_test.rb b/test/unit/reporters/html_reporter_test.rb
index 9c7cb9b3..5713296d 100644
--- a/test/unit/reporters/html_reporter_test.rb
+++ b/test/unit/reporters/html_reporter_test.rb
@@ -98,6 +98,22 @@ class HTMLReporterTest < ActiveSupport::TestCase
         assert_includes html, "data:image/png;base64,"
       end
 
+      test "#record from multiple threads produces correct totals" do
+        reporter = HTML.new(output_path: @output_path)
+        threads = 10
+        assertions_per_thread = 5
+
+        workers = threads.times.map do
+          Thread.new do
+            batch = assertions_per_thread.times.map { |i| build_passing_assertion("t#{Thread.current.object_id}_#{i}") }
+            reporter.record(batch)
+          end
+        end
+        workers.each(&:join)
+
+        assert_equal threads * assertions_per_thread, reporter.total
+      end
+
       test "#record and #finalize synchronize internal state" do
         reporter = HTML.new(output_path: @output_path)
 
@@ -124,6 +140,28 @@ def synchronize
         assert_operator fake_mutex.synchronize_calls, :>=, 1
       end
 
+      test "#finalize can retry after write_report failure" do
+        reporter = HTML.new(output_path: @output_path)
+        reporter.record([build_failing_assertion("retry")])
+
+        # Make write_report fail on first attempt
+        FileUtils.rm_rf(@output_dir)
+        File.write(@output_dir.to_s, "not a directory")
+
+        begin
+          reporter.finalize
+        rescue # rubocop:disable Lint/SuppressedException
+        end
+
+        # Restore writable directory and retry
+        File.delete(@output_dir.to_s)
+        FileUtils.mkdir_p(@output_dir)
+
+        result = reporter.finalize
+        assert_instance_of Pathname, result
+        assert @output_path.exist?
+      end
+
       private
 
       def build_passing_assertion(name)
diff --git a/test/unit/reporters_mutex_test.rb b/test/unit/reporters_mutex_test.rb
index b922ac5a..11daac7e 100644
--- a/test/unit/reporters_mutex_test.rb
+++ b/test/unit/reporters_mutex_test.rb
@@ -12,38 +12,14 @@ class ReportersMutexTest < ActiveSupport::TestCase
     teardown do
       CapybaraScreenshotDiff.reporters.clear
       CapybaraScreenshotDiff.reporters.concat(@original_reporters)
-      CapybaraScreenshotDiff.instance_variable_set(:@reporters_mutex, nil)
     end
 
-    test "reporters notification uses mutex snapshot" do
-      fake_mutex = Class.new do
-        attr_reader :synchronize_calls
-
-        def initialize
-          @synchronize_calls = 0
-        end
-
-        def synchronize
-          @synchronize_calls += 1
-          yield
-        end
-      end.new
-
-      CapybaraScreenshotDiff.instance_variable_set(:@reporters_mutex, fake_mutex)
-
-      reporter = Class.new do
-        def record(_assertions); end
-      end.new
-
-      CapybaraScreenshotDiff.reporters << reporter
-
-      assertion = CapybaraScreenshotDiff::ScreenshotAssertion.new("sample")
-      assertion.compare = Object.new
-      CapybaraScreenshotDiff.add_assertion(assertion)
-
-      CapybaraScreenshotDiff.reset
+    test "reporters_mutex is eagerly initialized" do
+      assert_instance_of Mutex, CapybaraScreenshotDiff.reporters_mutex
+    end
 
-      assert_operator fake_mutex.synchronize_calls, :>=, 1
+    test "reporters_mutex returns the same instance" do
+      assert_same CapybaraScreenshotDiff.reporters_mutex, CapybaraScreenshotDiff.reporters_mutex
     end
 
     test "reporters notification iterates over snapshot" do