From 86bff91f2ad39b51577db2ac9dab302d1f70a7c5 Mon Sep 17 00:00:00 2001 From: Phil Scott Date: Tue, 7 Jul 2026 11:33:20 -0400 Subject: [PATCH] fix(infrastructure): make FileWatcher subscriber lists thread-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FileWatcher.NotifySubscribers enumerated the _subscribers / _pathAwareSubscribers List fields on the debounce timer thread while SubscribeToChanges appended to them lock-free from other threads (AuditRunner at host start, RedirectContentService on first request). A registration landing during an in-flight notification threw "Collection was modified; enumeration operation may not execute" on the unobserved timer thread and terminated the process — a catastrophic, Linux-only flake in the integration tests, where a real inotify watcher fires the 100ms debounce squarely in the startup/first-request subscription window. Switch both lists to ImmutableList appended via ImmutableInterlocked.Update; NotifySubscribers snapshots each field once, so a concurrent registration swaps the reference without disturbing the in-flight enumeration. The regression test drives the debounce off a FakeTimeProvider so the modify-during-enumeration surfaces deterministically on the test thread instead of as an unobservable process crash. --- src/Pennington/Infrastructure/FileWatcher.cs | 26 ++++++++---- .../Infrastructure/FileWatcherTests.cs | 42 +++++++++++++++++++ 2 files changed, 61 insertions(+), 7 deletions(-) diff --git a/src/Pennington/Infrastructure/FileWatcher.cs b/src/Pennington/Infrastructure/FileWatcher.cs index d426eeac..6e32d18b 100644 --- a/src/Pennington/Infrastructure/FileWatcher.cs +++ b/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 _watchers = new(); - private readonly List _subscribers = []; - private readonly List> _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 _subscribers = []; + private ImmutableList> _pathAwareSubscribers = []; private readonly Lock _debounceLock = new(); private readonly Dictionary _pending = new(StringComparer.Ordinal); private readonly ILogger? _logger; @@ -83,13 +90,13 @@ public void AddPathWatch(string path, string filePattern, Action public void SubscribeToChanges(Action onUpdate) { - _subscribers.Add(onUpdate); + ImmutableInterlocked.Update(ref _subscribers, static (list, item) => list.Add(item), onUpdate); } /// public void SubscribeToChanges(Action onUpdate) { - _pathAwareSubscribers.Add(onUpdate); + ImmutableInterlocked.Update(ref _pathAwareSubscribers, static (list, item) => list.Add(item), onUpdate); } /// @@ -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"); } diff --git a/tests/Pennington.Tests/Infrastructure/FileWatcherTests.cs b/tests/Pennington.Tests/Infrastructure/FileWatcherTests.cs index dc530147..025288d9 100644 --- a/tests/Pennington.Tests/Infrastructure/FileWatcherTests.cs +++ b/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() {