thread safety: reporters and docs

docs/thread_safety.md

@@ -0,0 +1,97 @@
+# 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 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.
+
+### 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
+```
+
+`@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.
+
+### SnapManager Per Call
+
+`SnapManager` 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 (using mutex-protected snapshot)
+
+## 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`

lib/capybara_screenshot_diff/reporters/html.rb

@@ -18,30 +18,43 @@ 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
+          return if failures.empty?
 
-        @finalized = true
-        return if failures.empty?
-
-        write_report
-        output_path
+          write_report
+          @finalized = true
+          output_path
+        end
       end
 
       def passed = total - failures.size
@@ -108,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

lib/capybara_screenshot_diff/screenshot_assertion.rb

@@ -133,19 +133,26 @@ def reporters
       @reporters ||= []
     end
 
+    attr_reader :reporters_mutex
+
     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}"
       end
     end
   end
+
+  @reporters_mutex = Mutex.new
 end

test/unit/reporters/html_reporter_test.rb

@@ -98,6 +98,70 @@ 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)
+
+        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
+
+      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)

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)
+    end
+
+    test "reporters_mutex is eagerly initialized" do
+      assert_instance_of Mutex, CapybaraScreenshotDiff.reporters_mutex
+    end
+
+    test "reporters_mutex returns the same instance" do
+      assert_same CapybaraScreenshotDiff.reporters_mutex, CapybaraScreenshotDiff.reporters_mutex
+    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