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>

Author: pftg

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`

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

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

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)

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