Fix catastrophic CI flake: thread-safe FileWatcher subscriber lists

src/Pennington/Infrastructure/FileWatcher.cs

@@ -1,5 +1,6 @@
 namespace Pennington.Infrastructure;
 
+using System.Collections.Immutable;
 using System.IO.Abstractions;
 using System.Threading;
 using Microsoft.Extensions.Logging;
@@ -27,8 +28,14 @@ public sealed class FileWatcher : IFileWatcher
     private readonly IFileSystem _fileSystem;
     private readonly TimeProvider _clock;
     private readonly Dictionary<string, IFileSystemWatcher> _watchers = new();
-    private readonly List<Action> _subscribers = [];
-    private readonly List<Action<FileChangeNotification>> _pathAwareSubscribers = [];
+
+    // Subscribers register from startup and first-request threads (AuditRunner at host start,
+    // RedirectContentService on first request) while NotifySubscribers enumerates them on the
+    // debounce timer thread. Immutable lists let a registration swap the field atomically without
+    // disturbing an in-flight enumeration — a plain List threw "Collection was modified" on the
+    // unobserved timer thread and crashed the process.
+    private ImmutableList<Action> _subscribers = [];
+    private ImmutableList<Action<FileChangeNotification>> _pathAwareSubscribers = [];
     private readonly Lock _debounceLock = new();
     private readonly Dictionary<string, PendingChange> _pending = new(StringComparer.Ordinal);
     private readonly ILogger<FileWatcher>? _logger;
@@ -83,13 +90,13 @@ public void AddPathWatch(string path, string filePattern, Action<string, Watcher
     /// <inheritdoc/>
     public void SubscribeToChanges(Action onUpdate)
     {
-        _subscribers.Add(onUpdate);
+        ImmutableInterlocked.Update(ref _subscribers, static (list, item) => list.Add(item), onUpdate);
     }
 
     /// <inheritdoc/>
     public void SubscribeToChanges(Action<FileChangeNotification> onUpdate)
     {
-        _pathAwareSubscribers.Add(onUpdate);
+        ImmutableInterlocked.Update(ref _pathAwareSubscribers, static (list, item) => list.Add(item), onUpdate);
     }
 
     /// <summary>
@@ -198,18 +205,23 @@ internal static bool IsEditorTempFile(string fileName)
 
     private void NotifySubscribers(string fullPath, WatcherChangeTypes changeType)
     {
-        foreach (var subscriber in _subscribers)
+        // Snapshot each immutable list once — a concurrent SubscribeToChanges swaps the field, but
+        // these locals keep enumerating the set captured when the notification began.
+        var subscribers = _subscribers;
+        var pathAwareSubscribers = _pathAwareSubscribers;
+
+        foreach (var subscriber in subscribers)
         {
             try { subscriber(); }
             catch (Exception ex) { _logger?.LogError(ex, "Error notifying file watch subscriber"); }
         }
-        if (_pathAwareSubscribers.Count == 0)
+        if (pathAwareSubscribers.Count == 0)
         {
             return;
         }
 
         var notification = new FileChangeNotification(fullPath, changeType);
-        foreach (var subscriber in _pathAwareSubscribers)
+        foreach (var subscriber in pathAwareSubscribers)
         {
             try { subscriber(notification); }
             catch (Exception ex) { _logger?.LogError(ex, "Error notifying path-aware file watch subscriber"); }

tests/Pennington.Tests/Infrastructure/FileWatcherTests.cs

@@ -1,6 +1,7 @@
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
+using Microsoft.Extensions.Time.Testing;
 using Pennington.Infrastructure;
 using Testably.Abstractions;
 using Testably.Abstractions.Testing;
@@ -74,6 +75,47 @@ public async Task SubscribeToChanges_NotifiesSubscribers()
         subscriberCalled.ShouldBeTrue();
     }
 
+    [Fact]
+    public async Task SubscribeDuringNotification_DoesNotCorruptEnumeration()
+    {
+        // Regression: NotifySubscribers enumerates the subscriber list on the debounce timer
+        // thread. A subscriber registering *during* that notification (AuditRunner at host start,
+        // RedirectContentService on first request) used to mutate the backing List mid-foreach and
+        // throw "Collection was modified" on that unobserved thread — a catastrophic, Linux-only CI
+        // crash. Fire the debounce on the test thread via a fake clock so any fault surfaces here,
+        // and assert the subscriber registered after the mid-notification one still runs.
+        var ct = TestContext.Current.CancellationToken;
+        var fs = new RealFileSystem();
+        var clock = new FakeTimeProvider();
+        using var watcher = new FileWatcher(fs, clock);
+
+        var filePath = Path.Combine(_tempDir, "race.txt");
+        await File.WriteAllTextAsync(filePath, "initial", ct);
+        watcher.AddPathWatch(_tempDir, "*.txt", (_, _) => { });
+
+        // This subscriber appends another from inside the notification — the exact
+        // modify-during-enumeration the startup race produced, but deterministic.
+        watcher.SubscribeToChanges(() => watcher.SubscribeToChanges(() => { }));
+        // If that append corrupts the in-flight enumeration, this trailing subscriber never fires.
+        var trailingFired = false;
+        watcher.SubscribeToChanges(() => trailingFired = true);
+
+        await File.WriteAllTextAsync(filePath, "changed", ct);
+
+        // Wait for the OS watcher to buffer the change (it arms a debounce timer), then advance the
+        // fake clock to fire that timer synchronously on this thread.
+        var deadline = DateTime.UtcNow.AddSeconds(5);
+        while (!trailingFired && DateTime.UtcNow < deadline)
+        {
+            clock.Advance(TimeSpan.FromMilliseconds(150));
+            if (trailingFired) { break; }
+            await Task.Delay(20, ct);
+        }
+
+        trailingFired.ShouldBeTrue(
+            "the subscriber registered after a mid-notification subscribe should still fire");
+    }
+
     [Fact]
     public async Task FileWatcher_CoalescesRapidBurst_IntoSingleNotification()
     {