From 4e4dca0f5b8a5ab40421d21f06f4ce8fddceaaf3 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 25 Sep 2026 10:13:41 -0400 Subject: [PATCH 1/7] Load each FwData project once under concurrent requests FwDataFactory.GetProjectServiceCached used IMemoryCache.GetOrCreate with a factory that ran the slow LoadCache. GetOrCreate isn't atomic, so concurrent requests for the same project (e.g. the Platform.Bible extension fetching writing systems for every project, then retrying after a client timeout) each loaded it. When a later load finished, its Set replaced the earlier entry, and the eviction callback disposed the earlier LcmCache (EvictionReason.Replaced). The logs showed "loaded" immediately followed by "Evicting"/"disposed", and the load queue grew with every retry. Cache a Lazy (ExecutionAndPublication) created under a lock, so concurrent callers join the one in-flight load. A failed load is removed from the cache so the next call retries. Eviction, Dispose and CloseProjectAsync only dispose a Lazy whose value was created. The eviction log now includes the reason. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../FwDataFactoryTests.cs | 89 +++++++++++++++++++ .../FwDataMiniLcmBridge/FwDataFactory.cs | 54 ++++++++--- 2 files changed, 130 insertions(+), 13 deletions(-) create mode 100644 backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs diff --git a/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs b/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs new file mode 100644 index 0000000000..2bd8f04140 --- /dev/null +++ b/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs @@ -0,0 +1,89 @@ +using FwDataMiniLcmBridge.LcmUtils; +using FwDataMiniLcmBridge.Tests.Fixtures; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using SIL.LCModel; + +namespace FwDataMiniLcmBridge.Tests; + +public class FwDataFactoryTests : IDisposable +{ + private readonly ServiceProvider _services; + private readonly FwDataFactory _factory; + private readonly MockFwProjectLoader _mockLoader; + private readonly GatedProjectLoader _gatedLoader; + private readonly string _projectsFolder; + + public FwDataFactoryTests() + { + _services = new ServiceCollection() + .AddTestFwDataBridge() + .AddSingleton() + .AddSingleton(sp => sp.GetRequiredService()) + .BuildServiceProvider(); + _factory = _services.GetRequiredService(); + _mockLoader = _services.GetRequiredService(); + _gatedLoader = _services.GetRequiredService(); + _projectsFolder = _services.GetRequiredService>().Value.ProjectsFolder; + } + + public void Dispose() + { + _gatedLoader.Release.Set(); + _services.Dispose(); + } + + private FwDataProject NewProject(string name) => new($"{name}_{Guid.NewGuid()}", _projectsFolder); + + [Fact] + public async Task ConcurrentRequestsForTheSameProjectShareOneLoad() + { + var project = NewProject("concurrent-load"); + _mockLoader.NewProject(project, "en", "en"); + + var first = Task.Run(() => _factory.GetFwDataMiniLcmApi(project, false).Cache); + _gatedLoader.Entered.Wait(TimeSpan.FromSeconds(10)).Should().BeTrue(); + var second = Task.Run(() => _factory.GetFwDataMiniLcmApi(project, false).Cache); + // Give the second caller time to reach the cache while the first load is still running. + await Task.Delay(200); + _gatedLoader.Release.Set(); + var caches = await Task.WhenAll(first, second); + + _gatedLoader.LoadCount.Should().Be(1); + caches[1].Should().BeSameAs(caches[0]); + caches[0].IsDisposed.Should().BeFalse(); + } + + [Fact] + public void FailedLoadIsRetriedOnTheNextRequest() + { + _gatedLoader.Release.Set(); + var project = NewProject("failed-load"); + var getCache = () => _factory.GetFwDataMiniLcmApi(project, false).Cache; + + getCache.Should().Throw(); + _mockLoader.NewProject(project, "en", "en"); + + getCache().IsDisposed.Should().BeFalse(); + _gatedLoader.LoadCount.Should().Be(2); + } + + private class GatedProjectLoader(MockFwProjectLoader inner) : IProjectLoader + { + private int _loadCount; + public int LoadCount => _loadCount; + public ManualResetEventSlim Entered { get; } = new(); + public ManualResetEventSlim Release { get; } = new(); + + public LcmCache LoadCache(FwDataProject project) + { + Interlocked.Increment(ref _loadCount); + Entered.Set(); + Release.Wait(TimeSpan.FromSeconds(10)); + return inner.LoadCache(project); + } + + public LcmCache NewProject(FwDataProject project, string analysisWs, string vernacularWs) => + inner.NewProject(project, analysisWs, vernacularWs); + } +} diff --git a/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs b/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs index 8dff6914bc..3f0d488f70 100644 --- a/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs +++ b/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs @@ -49,24 +49,48 @@ public FwDataMiniLcmApi GetFwDataMiniLcmApi(FwDataProject project, bool saveOnDi } private HashSet _projectCacheKeys = []; + private readonly Lock _cacheEntryLock = new(); private LcmCache GetProjectServiceCached(FwDataProject project) { var key = CacheKey(project); - var projectService = cache.GetOrCreate(key, + // IMemoryCache.GetOrCreate isn't atomic: concurrent callers would each load the project, and the later Set + // would evict (and dispose) the earlier LcmCache as Replaced. So create the entry under a lock, and let the + // Lazy make every caller share the one slow load without holding the lock during it. + Lazy lazyProjectService; + lock (_cacheEntryLock) + { + lazyProjectService = cache.GetOrCreate(key, entry => { entry.SlidingExpiration = CacheSlidingExpiration; entry.RegisterPostEvictionCallback(OnLcmProjectCacheEviction, (logger, _projectCacheKeys)); - logger.LogInformation("Loading project {ProjectFileName}", project.FileName); - var projectService = projectLoader.LoadCache(project); - logger.LogInformation("Project {ProjectFileName} loaded", project.FileName); _projectCacheKeys.Add(key); - return projectService; - }); - if (projectService is null) + return new Lazy(() => + { + logger.LogInformation("Loading project {ProjectFileName}", project.FileName); + var projectService = projectLoader.LoadCache(project); + logger.LogInformation("Project {ProjectFileName} loaded", project.FileName); + return projectService; + }, LazyThreadSafetyMode.ExecutionAndPublication); + }) ?? throw new InvalidOperationException("Project service is null"); + } + + LcmCache projectService; + try { - throw new InvalidOperationException("Project service is null"); + projectService = lazyProjectService.Value; } + catch + { + // The Lazy caches its exception, so drop the entry to let the next call retry the load. + lock (_cacheEntryLock) + { + if (cache.TryGetValue(key, out Lazy? current) && ReferenceEquals(current, lazyProjectService)) + cache.Remove(key); + } + throw; + } + if (projectService.IsDisposed) { throw new InvalidOperationException("Project service is disposed"); @@ -81,7 +105,7 @@ private static void OnLcmProjectCacheEviction(object keyObj, object? value, Evic // todo this could trigger when the service is still referenced elsewhere, for example in a long running task. // disposing of the service while it's still in use would be bad. // one way around this would be to return a lease object, only after a timeout and no more references to the lease object would the service be disposed. - var lcmCache = (LcmCache)value; + var lazyLcmCache = (Lazy)value; var (logger, projectCacheKeys) = ((ILogger, HashSet))state!; if (keyObj.ToString() is not string key) { @@ -90,9 +114,10 @@ private static void OnLcmProjectCacheEviction(object keyObj, object? value, Evic return; } var filePath = FilePathFromCacheKey(key); - logger.LogInformation("Evicting project {ProjectFileName} from cache", filePath); + logger.LogInformation("Evicting project {ProjectFileName} from cache ({EvictionReason})", filePath, reason); projectCacheKeys.Remove(key); - if (!lcmCache.IsDisposed) + // Not created means the load failed or is still running, so there's nothing to dispose. + if (lazyLcmCache.IsValueCreated && lazyLcmCache.Value is { IsDisposed: false } lcmCache) { lcmCache.Dispose(); logger.LogInformation("FW Data Project {ProjectFileName} disposed", filePath); @@ -107,7 +132,8 @@ public void Dispose() var projectCacheKeys = Interlocked.Exchange(ref _projectCacheKeys, []); foreach (var key in projectCacheKeys) { - var lcmCache = cache.Get(key); + var lazyLcmCache = cache.Get>(key); + var lcmCache = lazyLcmCache is { IsValueCreated: true } ? lazyLcmCache.Value : null; if (lcmCache is null || lcmCache.IsDisposed) continue; var filePath = FilePathFromCacheKey(key); lcmCache.Dispose(); //need to explicitly call dispose as that blocks, just removing from the cache does not block, meaning it will not finish disposing before the program exits. @@ -122,7 +148,9 @@ public async Task CloseProjectAsync(FwDataProject project) if (_shuttingDown) return; logger.LogInformation("Explicitly Closing project {ProjectFileName}", project.FilePath); var cacheKey = CacheKey(project); - var lcmCache = cache.Get(cacheKey); + var lazyLcmCache = cache.Get>(cacheKey); + // A load still in flight has nothing to dispose yet, so it's left alone (as before it was cached). + var lcmCache = lazyLcmCache is { IsValueCreated: true } ? lazyLcmCache.Value : null; if (lcmCache is null) return; // Dispose cache immediately so file locks are released before we return. // The caller assumes the project is ready to be opened somewhere else (e.g. in FieldWorks) From 3daf2106ebbec159efe1c37f57bfc568c7f38b7f Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 25 Sep 2026 10:39:41 -0400 Subject: [PATCH 2/7] Dispose FwData loads that are closed, evicted or retried mid-flight - CloseProjectAsync takes the entry out of the cache and waits for a load still in flight, then disposes it, so file locks are released before it returns. - Evicting an entry whose load hasn't finished disposes the cache once the load completes. - Track cache entries instead of keys, so a failed load's late eviction callback can't untrack its replacement and hide it from shutdown. Each entry owns a run-once disposal shared by eviction, close and shutdown. The concurrency test no longer depends on a fixed delay. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../FwDataFactoryTests.cs | 121 +++++++++++- .../FwDataMiniLcmBridge/FwDataFactory.cs | 187 +++++++++++++----- 2 files changed, 249 insertions(+), 59 deletions(-) diff --git a/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs b/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs index 2bd8f04140..ab45a6d25c 100644 --- a/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs +++ b/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs @@ -1,6 +1,9 @@ +using System.Collections.Concurrent; using FwDataMiniLcmBridge.LcmUtils; using FwDataMiniLcmBridge.Tests.Fixtures; +using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using SIL.LCModel; @@ -8,16 +11,19 @@ namespace FwDataMiniLcmBridge.Tests; public class FwDataFactoryTests : IDisposable { + private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(10); private readonly ServiceProvider _services; private readonly FwDataFactory _factory; private readonly MockFwProjectLoader _mockLoader; private readonly GatedProjectLoader _gatedLoader; + private readonly LogCapture _logs = new(); private readonly string _projectsFolder; public FwDataFactoryTests() { _services = new ServiceCollection() .AddTestFwDataBridge() + .AddLogging(builder => builder.AddProvider(_logs)) .AddSingleton() .AddSingleton(sp => sp.GetRequiredService()) .BuildServiceProvider(); @@ -35,17 +41,41 @@ public void Dispose() private FwDataProject NewProject(string name) => new($"{name}_{Guid.NewGuid()}", _projectsFolder); + private LcmCache GetCache(FwDataProject project) => _factory.GetFwDataMiniLcmApi(project, false).Cache; + + // Starts a load and returns once it's blocked inside LoadCache. + private Task StartBlockedLoad(FwDataProject project) + { + var load = Task.Run(() => GetCache(project)); + _gatedLoader.Entered.Wait(Timeout).Should().BeTrue(); + return load; + } + + private static async Task WaitUntil(Func condition) + { + var deadline = DateTime.UtcNow + Timeout; + while (!condition()) + { + if (DateTime.UtcNow > deadline) throw new TimeoutException("Condition not met in time"); + await Task.Delay(20); + } + } + [Fact] public async Task ConcurrentRequestsForTheSameProjectShareOneLoad() { var project = NewProject("concurrent-load"); _mockLoader.NewProject(project, "en", "en"); - var first = Task.Run(() => _factory.GetFwDataMiniLcmApi(project, false).Cache); - _gatedLoader.Entered.Wait(TimeSpan.FromSeconds(10)).Should().BeTrue(); - var second = Task.Run(() => _factory.GetFwDataMiniLcmApi(project, false).Cache); - // Give the second caller time to reach the cache while the first load is still running. - await Task.Delay(200); + var first = StartBlockedLoad(project); + Thread? secondThread = null; + var second = Task.Run(() => + { + secondThread = Thread.CurrentThread; + return GetCache(project); + }); + // The second caller blocks either joining the first load or (if loads aren't shared) inside its own LoadCache. + await WaitUntil(() => secondThread?.ThreadState.HasFlag(ThreadState.WaitSleepJoin) == true); _gatedLoader.Release.Set(); var caches = await Task.WhenAll(first, second); @@ -59,8 +89,8 @@ public void FailedLoadIsRetriedOnTheNextRequest() { _gatedLoader.Release.Set(); var project = NewProject("failed-load"); - var getCache = () => _factory.GetFwDataMiniLcmApi(project, false).Cache; + var getCache = () => GetCache(project); getCache.Should().Throw(); _mockLoader.NewProject(project, "en", "en"); @@ -68,22 +98,99 @@ public void FailedLoadIsRetriedOnTheNextRequest() _gatedLoader.LoadCount.Should().Be(2); } + [Fact] + public async Task CloseWaitsForAnInFlightLoadAndDisposesIt() + { + var project = NewProject("close-during-load"); + var lcmCache = _mockLoader.NewProject(project, "en", "en"); + var load = StartBlockedLoad(project); + + var close = _factory.CloseProjectAsync(project); + await Task.WhenAny(close, Task.Delay(200)); + close.IsCompleted.Should().BeFalse(); + _gatedLoader.Release.Set(); + await close.WaitAsync(Timeout); + + lcmCache.IsDisposed.Should().BeTrue(); + await IgnoreFailure(load); + } + + [Fact] + public async Task EvictionDuringALoadDisposesItOnceLoaded() + { + var project = NewProject("evict-during-load"); + var lcmCache = _mockLoader.NewProject(project, "en", "en"); + var load = StartBlockedLoad(project); + + _services.GetRequiredService().Remove(FwDataFactory.CacheKey(project)); + await WaitForEvictionCallback(project); + _gatedLoader.Release.Set(); + + await WaitUntil(() => lcmCache.IsDisposed); + await IgnoreFailure(load); + } + + [Fact] + public async Task ShutdownDisposesAProjectRetriedAfterAFailedLoad() + { + _gatedLoader.Release.Set(); + var project = NewProject("retry-then-shutdown"); + var lcmCache = _mockLoader.NewProject(project, "en", "en"); + _gatedLoader.FailNext = true; + var getCache = () => GetCache(project); + getCache.Should().Throw(); + getCache().Should().BeSameAs(lcmCache); + + // The failed entry's eviction callback runs on the thread pool, typically after the retry. + await WaitForEvictionCallback(project); + _factory.Dispose(); + + lcmCache.IsDisposed.Should().BeTrue(); + } + + private Task WaitForEvictionCallback(FwDataProject project) => + WaitUntil(() => _logs.Messages.Any(m => m.Contains("Evicting project") && m.Contains(project.FilePath))); + + // The requester may get the cache or an "already disposed" error, depending on how it races the disposal. + private static async Task IgnoreFailure(Task load) + { + try { await load.WaitAsync(Timeout); } + catch (InvalidOperationException) { } + } + private class GatedProjectLoader(MockFwProjectLoader inner) : IProjectLoader { private int _loadCount; public int LoadCount => _loadCount; public ManualResetEventSlim Entered { get; } = new(); public ManualResetEventSlim Release { get; } = new(); + public bool FailNext { get; set; } public LcmCache LoadCache(FwDataProject project) { Interlocked.Increment(ref _loadCount); + if (FailNext) + { + FailNext = false; + throw new InvalidOperationException("Simulated load failure"); + } Entered.Set(); - Release.Wait(TimeSpan.FromSeconds(10)); + Release.Wait(Timeout); return inner.LoadCache(project); } public LcmCache NewProject(FwDataProject project, string analysisWs, string vernacularWs) => inner.NewProject(project, analysisWs, vernacularWs); } + + private class LogCapture : ILoggerProvider, ILogger + { + public ConcurrentQueue Messages { get; } = new(); + public ILogger CreateLogger(string categoryName) => this; + public IDisposable? BeginScope(TState state) where TState : notnull => null; + public bool IsEnabled(LogLevel logLevel) => true; + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) => Messages.Enqueue(formatter(state, exception)); + public void Dispose() { } + } } diff --git a/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs b/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs index 3f0d488f70..92c7cddfd0 100644 --- a/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs +++ b/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs @@ -1,4 +1,3 @@ -using System.Diagnostics; using FwDataMiniLcmBridge.Api; using FwDataMiniLcmBridge.LcmUtils; using FwDataMiniLcmBridge.Media; @@ -40,54 +39,51 @@ public FwDataFactory(ILogger fwdataLogger, }); } - private static string CacheKey(FwDataProject project) => $"{nameof(FwDataFactory)}|{project.FilePath}"; - private static string FilePathFromCacheKey(string cacheKey) => cacheKey.Split('|')[1]; + internal static string CacheKey(FwDataProject project) => $"{nameof(FwDataFactory)}|{project.FilePath}"; public FwDataMiniLcmApi GetFwDataMiniLcmApi(FwDataProject project, bool saveOnDispose) { return new FwDataMiniLcmApi(new(() => GetProjectServiceCached(project)), saveOnDispose, fwdataLogger, project, mediaAdapter, config); } - private HashSet _projectCacheKeys = []; private readonly Lock _cacheEntryLock = new(); + // Entries rather than keys, so a stale eviction callback can't untrack a newer entry for the same project. + private HashSet _projectCacheEntries = []; private LcmCache GetProjectServiceCached(FwDataProject project) { var key = CacheKey(project); // IMemoryCache.GetOrCreate isn't atomic: concurrent callers would each load the project, and the later Set // would evict (and dispose) the earlier LcmCache as Replaced. So create the entry under a lock, and let the - // Lazy make every caller share the one slow load without holding the lock during it. - Lazy lazyProjectService; + // entry make every caller share the one slow load without holding the lock during it. + ProjectCacheEntry projectEntry; lock (_cacheEntryLock) { - lazyProjectService = cache.GetOrCreate(key, + projectEntry = cache.GetOrCreate(key, entry => { entry.SlidingExpiration = CacheSlidingExpiration; - entry.RegisterPostEvictionCallback(OnLcmProjectCacheEviction, (logger, _projectCacheKeys)); - _projectCacheKeys.Add(key); - return new Lazy(() => + entry.RegisterPostEvictionCallback(OnLcmProjectCacheEviction); + var newEntry = new ProjectCacheEntry(key, project.FilePath, () => { logger.LogInformation("Loading project {ProjectFileName}", project.FileName); var projectService = projectLoader.LoadCache(project); logger.LogInformation("Project {ProjectFileName} loaded", project.FileName); return projectService; - }, LazyThreadSafetyMode.ExecutionAndPublication); + }, logger); + _projectCacheEntries.Add(newEntry); + return newEntry; }) ?? throw new InvalidOperationException("Project service is null"); } LcmCache projectService; try { - projectService = lazyProjectService.Value; + projectService = projectEntry.LcmCache; } catch { - // The Lazy caches its exception, so drop the entry to let the next call retry the load. - lock (_cacheEntryLock) - { - if (cache.TryGetValue(key, out Lazy? current) && ReferenceEquals(current, lazyProjectService)) - cache.Remove(key); - } + // The entry caches its load exception, so drop it to let the next call retry the load. + RemoveIfCurrent(projectEntry); throw; } @@ -99,46 +95,53 @@ private LcmCache GetProjectServiceCached(FwDataProject project) return projectService; } - private static void OnLcmProjectCacheEviction(object keyObj, object? value, EvictionReason reason, object? state) + private void RemoveIfCurrent(ProjectCacheEntry projectEntry) + { + lock (_cacheEntryLock) + { + if (cache.TryGetValue(projectEntry.Key, out ProjectCacheEntry? current) && ReferenceEquals(current, projectEntry)) + cache.Remove(projectEntry.Key); + } + } + + private void OnLcmProjectCacheEviction(object key, object? value, EvictionReason reason, object? state) { - if (value is null) return; + if (value is not ProjectCacheEntry projectEntry) return; // todo this could trigger when the service is still referenced elsewhere, for example in a long running task. // disposing of the service while it's still in use would be bad. // one way around this would be to return a lease object, only after a timeout and no more references to the lease object would the service be disposed. - var lazyLcmCache = (Lazy)value; - var (logger, projectCacheKeys) = ((ILogger, HashSet))state!; - if (keyObj.ToString() is not string key) + logger.LogInformation("Evicting project {ProjectFileName} from cache ({EvictionReason})", projectEntry.FilePath, reason); + lock (_cacheEntryLock) { - Debug.Fail($"Eviction callback called with null key {keyObj}"); - logger.LogError("Eviction callback called with null key {Key}", keyObj); - return; + _projectCacheEntries.Remove(projectEntry); } - var filePath = FilePathFromCacheKey(key); - logger.LogInformation("Evicting project {ProjectFileName} from cache ({EvictionReason})", filePath, reason); - projectCacheKeys.Remove(key); - // Not created means the load failed or is still running, so there's nothing to dispose. - if (lazyLcmCache.IsValueCreated && lazyLcmCache.Value is { IsDisposed: false } lcmCache) + _ = projectEntry.DisposeLoaded().ContinueWith(disposal => { - lcmCache.Dispose(); - logger.LogInformation("FW Data Project {ProjectFileName} disposed", filePath); + if (disposal.IsFaulted) + logger.LogError(disposal.Exception, "Failed to dispose project {ProjectFileName}", projectEntry.FilePath); GC.Collect(); - } + }, TaskScheduler.Default); } public void Dispose() { logger.LogInformation("Closing all projects"); - //ensure a race condition doesn't cause us to dispose of a project that's already been disposed - var projectCacheKeys = Interlocked.Exchange(ref _projectCacheKeys, []); - foreach (var key in projectCacheKeys) + HashSet projectEntries; + lock (_cacheEntryLock) { - var lazyLcmCache = cache.Get>(key); - var lcmCache = lazyLcmCache is { IsValueCreated: true } ? lazyLcmCache.Value : null; - if (lcmCache is null || lcmCache.IsDisposed) continue; - var filePath = FilePathFromCacheKey(key); - lcmCache.Dispose(); //need to explicitly call dispose as that blocks, just removing from the cache does not block, meaning it will not finish disposing before the program exits. - logger.LogInformation("FW Data Project {ProjectFileName} disposed", filePath); - cache.Remove(key); + projectEntries = _projectCacheEntries; + _projectCacheEntries = []; + } + foreach (var projectEntry in projectEntries) + { + //need to explicitly dispose as that blocks, just removing from the cache does not block, meaning it will not finish disposing before the program exits. + var disposal = projectEntry.DisposeLoaded(); + // Not waited for: a load that hasn't finished has no changes to save, and exiting releases its file locks. + if (!disposal.IsCompleted) + logger.LogWarning("Project {ProjectFileName} is still loading at shutdown, not waiting for it", projectEntry.FilePath); + else if (disposal.IsFaulted) + logger.LogError(disposal.Exception, "Failed to dispose project {ProjectFileName}", projectEntry.FilePath); + RemoveIfCurrent(projectEntry); } } @@ -147,15 +150,21 @@ public async Task CloseProjectAsync(FwDataProject project) // if we are shutting down, don't do anything because we want project dispose to be called as part of the shutdown process. if (_shuttingDown) return; logger.LogInformation("Explicitly Closing project {ProjectFileName}", project.FilePath); - var cacheKey = CacheKey(project); - var lazyLcmCache = cache.Get>(cacheKey); - // A load still in flight has nothing to dispose yet, so it's left alone (as before it was cached). - var lcmCache = lazyLcmCache is { IsValueCreated: true } ? lazyLcmCache.Value : null; - if (lcmCache is null) return; - // Dispose cache immediately so file locks are released before we return. + var projectEntry = TakeEntry(CacheKey(project)); + if (projectEntry is null) return; + // Dispose immediately (waiting for a load still in flight) so file locks are released before we return. // The caller assumes the project is ready to be opened somewhere else (e.g. in FieldWorks) - if (!lcmCache.IsDisposed) await Task.Run(lcmCache.Dispose); - cache.Remove(cacheKey); + await Task.Run(projectEntry.DisposeLoaded); + } + + private ProjectCacheEntry? TakeEntry(string key) + { + lock (_cacheEntryLock) + { + if (!cache.TryGetValue(key, out ProjectCacheEntry? projectEntry)) return null; + cache.Remove(key); + return projectEntry; + } } public IAsyncDisposable DeferCloseAsync(FwDataProject project) @@ -193,4 +202,78 @@ public Task StopAsync(CancellationToken cancellationToken) Dispose(); return Task.CompletedTask; } + + private sealed class ProjectCacheEntry + { + // Lets disposal wait for the load without blocking a thread on the Lazy. + private readonly TaskCompletionSource _loaded = new(TaskCreationOptions.RunContinuationsAsynchronously); + private LcmCache? _loadedCache; + private readonly Lazy _lcmCache; + private readonly TaskCompletionSource _disposed = new(TaskCreationOptions.RunContinuationsAsynchronously); + private int _disposeRequested; + private readonly ILogger _logger; + + public ProjectCacheEntry(string key, string filePath, Func load, ILogger logger) + { + Key = key; + FilePath = filePath; + _logger = logger; + _lcmCache = new(() => + { + try + { + var lcmCache = load(); + _loadedCache = lcmCache; + _loaded.SetResult(); + return lcmCache; + } + catch (Exception e) + { + _loaded.SetException(e); + throw; + } + }, LazyThreadSafetyMode.ExecutionAndPublication); + } + + public string Key { get; } + public string FilePath { get; } + public LcmCache LcmCache => _lcmCache.Value; + + /// + /// Disposes the LcmCache once its load finishes, synchronously if it already has. + /// Runs once however many of eviction, close and shutdown call it; every caller gets the same task. + /// + public Task DisposeLoaded() + { + if (Interlocked.Exchange(ref _disposeRequested, 1) == 0) + { + if (_loaded.Task.IsCompleted) DisposeAfterLoad(_loaded.Task); + else _ = _loaded.Task.ContinueWith(DisposeAfterLoad, TaskScheduler.Default); + } +#pragma warning disable VSTHRD003 // Avoid awaiting foreign Tasks: this entry owns the TaskCompletionSource + return _disposed.Task; +#pragma warning restore VSTHRD003 + } + + private void DisposeAfterLoad(Task load) + { + try + { + if (!load.IsCompletedSuccessfully) + { + _logger.LogWarning(load.Exception, "FW Data Project {ProjectFileName} failed to load, nothing to dispose", FilePath); + } + else if (_loadedCache is { IsDisposed: false } lcmCache) + { + lcmCache.Dispose(); + _logger.LogInformation("FW Data Project {ProjectFileName} disposed", FilePath); + } + _disposed.SetResult(); + } + catch (Exception e) + { + _disposed.SetException(e); + } + } + } } From 2ac73aab5098f2cd340136f83570d8db8fa00ee7 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 25 Sep 2026 10:40:25 -0400 Subject: [PATCH 3/7] Trim FwDataFactory comments that only justify the change Co-Authored-By: Claude Opus 5.5 (1M context) --- backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs b/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs index 92c7cddfd0..7a1bdf9b48 100644 --- a/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs +++ b/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs @@ -47,14 +47,11 @@ public FwDataMiniLcmApi GetFwDataMiniLcmApi(FwDataProject project, bool saveOnDi } private readonly Lock _cacheEntryLock = new(); - // Entries rather than keys, so a stale eviction callback can't untrack a newer entry for the same project. private HashSet _projectCacheEntries = []; private LcmCache GetProjectServiceCached(FwDataProject project) { var key = CacheKey(project); - // IMemoryCache.GetOrCreate isn't atomic: concurrent callers would each load the project, and the later Set - // would evict (and dispose) the earlier LcmCache as Replaced. So create the entry under a lock, and let the - // entry make every caller share the one slow load without holding the lock during it. + // IMemoryCache.GetOrCreate isn't atomic, so the entry is created under the lock; its load runs outside it. ProjectCacheEntry projectEntry; lock (_cacheEntryLock) { From 6eef3685958cc05c2c5ed53f69fa690440f1b9a9 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 25 Sep 2026 11:11:14 -0400 Subject: [PATCH 4/7] Test that closing an FwData project mid-load releases its lock file Loads a real on-disk project through a gated loader, closes it while the load is blocked, and checks LCM's lock file is gone afterwards. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../FwDataFactoryOnDiskTests.cs | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryOnDiskTests.cs diff --git a/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryOnDiskTests.cs b/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryOnDiskTests.cs new file mode 100644 index 0000000000..26ec5100fd --- /dev/null +++ b/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryOnDiskTests.cs @@ -0,0 +1,75 @@ +using FwDataMiniLcmBridge.LcmUtils; +using FwDataMiniLcmBridge.Tests.Fixtures; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using SIL.LCModel; + +namespace FwDataMiniLcmBridge.Tests; + +public class FwDataFactoryOnDiskTests : IDisposable +{ + private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(30); + private readonly ServiceProvider _services; + private readonly GatedProjectLoader _gatedLoader; + private readonly FwDataProject _project; + + public FwDataFactoryOnDiskTests() + { + _services = new ServiceCollection() + .AddTestFwDataBridge(mockProjectLoader: false) + .PostConfigure(config => config.TemplatesFolder = Path.GetFullPath("Templates")) + .AddSingleton(sp => new GatedProjectLoader(ActivatorUtilities.CreateInstance(sp))) + .AddSingleton(sp => sp.GetRequiredService()) + .BuildServiceProvider(); + _gatedLoader = _services.GetRequiredService(); + var projectsFolder = _services.GetRequiredService>().Value.ProjectsFolder; + Directory.CreateDirectory(projectsFolder); + _project = new FwDataProject($"close-releases-lock_{Guid.NewGuid()}", projectsFolder); + _gatedLoader.NewProject(_project, "en", "en").Dispose(); + } + + public void Dispose() + { + _gatedLoader.Release.Set(); + _services.Dispose(); + if (Directory.Exists(_project.ProjectFolder)) + Directory.Delete(_project.ProjectFolder, true); + } + + [Fact] + public async Task CloseDuringALoadReleasesTheLockFile() + { + var factory = _services.GetRequiredService(); + var load = Task.Run(() => factory.GetFwDataMiniLcmApi(_project, false).Cache); + _gatedLoader.Entered.Wait(Timeout).Should().BeTrue(); + var close = factory.CloseProjectAsync(_project); + _gatedLoader.Release.Set(); + await close.WaitAsync(Timeout); + // The requester may get the disposed cache or the "disposed" error, depending on how it races the close. + try { await load.WaitAsync(Timeout); } + catch (InvalidOperationException) { } + + // LCM's XML backend doesn't hold the .fwdata open; its lock is this file, deleted when the cache is disposed. + _gatedLoader.LockFileWhileLoaded.Should().BeTrue(); + File.Exists(_project.FilePath + ".lock").Should().BeFalse(); + } + + private class GatedProjectLoader(ProjectLoader inner) : IProjectLoader + { + public ManualResetEventSlim Entered { get; } = new(); + public ManualResetEventSlim Release { get; } = new(); + public bool LockFileWhileLoaded { get; private set; } + + public LcmCache LoadCache(FwDataProject project) + { + Entered.Set(); + Release.Wait(Timeout); + var lcmCache = inner.LoadCache(project); + LockFileWhileLoaded = File.Exists(project.FilePath + ".lock"); + return lcmCache; + } + + public LcmCache NewProject(FwDataProject project, string analysisWs, string vernacularWs) => + inner.NewProject(project, analysisWs, vernacularWs); + } +} From d2516f90a0d654fe694813e0939a9876672808a7 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 25 Sep 2026 12:16:26 -0400 Subject: [PATCH 5/7] Hold a per-project lock around FwData loads Replace the Lazy-based cache entry with a lock per project held for the whole load, so the cache only ever holds a finished LcmCache. A failed load caches nothing, an entry can't be evicted mid-load, and a request made during a close waits for it to finish disposing. Open caches are tracked by instance, and whoever untracks one disposes it, so eviction, close and shutdown dispose each cache exactly once. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../FwDataFactoryTests.cs | 59 ++--- .../FwDataMiniLcmBridge/FwDataFactory.cs | 221 ++++++------------ 2 files changed, 98 insertions(+), 182 deletions(-) diff --git a/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs b/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs index ab45a6d25c..85c83dad3b 100644 --- a/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs +++ b/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs @@ -1,9 +1,7 @@ -using System.Collections.Concurrent; using FwDataMiniLcmBridge.LcmUtils; using FwDataMiniLcmBridge.Tests.Fixtures; using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using SIL.LCModel; @@ -16,20 +14,20 @@ public class FwDataFactoryTests : IDisposable private readonly FwDataFactory _factory; private readonly MockFwProjectLoader _mockLoader; private readonly GatedProjectLoader _gatedLoader; - private readonly LogCapture _logs = new(); + private readonly IMemoryCache _memoryCache; private readonly string _projectsFolder; public FwDataFactoryTests() { _services = new ServiceCollection() .AddTestFwDataBridge() - .AddLogging(builder => builder.AddProvider(_logs)) .AddSingleton() .AddSingleton(sp => sp.GetRequiredService()) .BuildServiceProvider(); _factory = _services.GetRequiredService(); _mockLoader = _services.GetRequiredService(); _gatedLoader = _services.GetRequiredService(); + _memoryCache = _services.GetRequiredService(); _projectsFolder = _services.GetRequiredService>().Value.ProjectsFolder; } @@ -116,22 +114,23 @@ public async Task CloseWaitsForAnInFlightLoadAndDisposesIt() } [Fact] - public async Task EvictionDuringALoadDisposesItOnceLoaded() + public async Task RemovingTheCacheKeyDuringALoadDoesNotLoseTheLoad() { - var project = NewProject("evict-during-load"); - var lcmCache = _mockLoader.NewProject(project, "en", "en"); + var project = NewProject("remove-during-load"); + _mockLoader.NewProject(project, "en", "en"); var load = StartBlockedLoad(project); - _services.GetRequiredService().Remove(FwDataFactory.CacheKey(project)); - await WaitForEvictionCallback(project); + _memoryCache.Remove(FwDataFactory.CacheKey(project)); _gatedLoader.Release.Set(); + var lcmCache = await load.WaitAsync(Timeout); - await WaitUntil(() => lcmCache.IsDisposed); - await IgnoreFailure(load); + lcmCache.IsDisposed.Should().BeFalse(); + GetCache(project).Should().BeSameAs(lcmCache); + _gatedLoader.LoadCount.Should().Be(1); } [Fact] - public async Task ShutdownDisposesAProjectRetriedAfterAFailedLoad() + public void ShutdownDisposesAProjectRetriedAfterAFailedLoad() { _gatedLoader.Release.Set(); var project = NewProject("retry-then-shutdown"); @@ -141,15 +140,32 @@ public async Task ShutdownDisposesAProjectRetriedAfterAFailedLoad() getCache.Should().Throw(); getCache().Should().BeSameAs(lcmCache); - // The failed entry's eviction callback runs on the thread pool, typically after the retry. - await WaitForEvictionCallback(project); _factory.Dispose(); lcmCache.IsDisposed.Should().BeTrue(); } - private Task WaitForEvictionCallback(FwDataProject project) => - WaitUntil(() => _logs.Messages.Any(m => m.Contains("Evicting project") && m.Contains(project.FilePath))); + [Fact] + public async Task RequestDuringACloseGetsANewCacheAfterIt() + { + _gatedLoader.Release.Set(); + var project = NewProject("request-during-close"); + var firstCache = _mockLoader.NewProject(project, "en", "en"); + GetCache(project).Should().BeSameAs(firstCache); + var secondCache = _mockLoader.NewProject(NewProject("request-during-close-reload"), "en", "en"); + _mockLoader.Projects[project.Name] = secondCache; + + var close = _factory.CloseProjectAsync(project); + // Close takes the entry out of the cache before disposing it; request once it's gone. + await WaitUntil(() => !_memoryCache.TryGetValue(FwDataFactory.CacheKey(project), out _)); + var reloaded = GetCache(project); + await close.WaitAsync(Timeout); + + firstCache.IsDisposed.Should().BeTrue(); + reloaded.Should().BeSameAs(secondCache); + reloaded.IsDisposed.Should().BeFalse(); + _gatedLoader.LoadCount.Should().Be(2); + } // The requester may get the cache or an "already disposed" error, depending on how it races the disposal. private static async Task IgnoreFailure(Task load) @@ -182,15 +198,4 @@ public LcmCache LoadCache(FwDataProject project) public LcmCache NewProject(FwDataProject project, string analysisWs, string vernacularWs) => inner.NewProject(project, analysisWs, vernacularWs); } - - private class LogCapture : ILoggerProvider, ILogger - { - public ConcurrentQueue Messages { get; } = new(); - public ILogger CreateLogger(string categoryName) => this; - public IDisposable? BeginScope(TState state) where TState : notnull => null; - public bool IsEnabled(LogLevel logLevel) => true; - public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, - Func formatter) => Messages.Enqueue(formatter(state, exception)); - public void Dispose() { } - } } diff --git a/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs b/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs index 7a1bdf9b48..24e7ad9959 100644 --- a/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs +++ b/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using FwDataMiniLcmBridge.Api; using FwDataMiniLcmBridge.LcmUtils; using FwDataMiniLcmBridge.Media; @@ -46,99 +47,84 @@ public FwDataMiniLcmApi GetFwDataMiniLcmApi(FwDataProject project, bool saveOnDi return new FwDataMiniLcmApi(new(() => GetProjectServiceCached(project)), saveOnDispose, fwdataLogger, project, mediaAdapter, config); } - private readonly Lock _cacheEntryLock = new(); - private HashSet _projectCacheEntries = []; + // One lock per project, held for the whole load, so concurrent requests share a load instead of racing + // (IMemoryCache.GetOrCreate isn't atomic) and the cache only ever holds a finished LcmCache. + private readonly ConcurrentDictionary _keyLocks = new(); + private Lock KeyLock(string key) => _keyLocks.GetOrAdd(key, _ => new()); + + private readonly Lock _openCachesLock = new(); + // Whoever removes an instance from here disposes it, so eviction, close and shutdown dispose it exactly once. + // Keyed by instance so a stale eviction callback can't untrack a newer cache for the same project. + private Dictionary _openCaches = []; + + private static string FilePathFromCacheKey(string cacheKey) => cacheKey.Split('|')[1]; + private LcmCache GetProjectServiceCached(FwDataProject project) { var key = CacheKey(project); - // IMemoryCache.GetOrCreate isn't atomic, so the entry is created under the lock; its load runs outside it. - ProjectCacheEntry projectEntry; - lock (_cacheEntryLock) + if (cache.TryGetValue(key, out LcmCache? hit) && !hit!.IsDisposed) return hit; + lock (KeyLock(key)) { - projectEntry = cache.GetOrCreate(key, - entry => - { - entry.SlidingExpiration = CacheSlidingExpiration; - entry.RegisterPostEvictionCallback(OnLcmProjectCacheEviction); - var newEntry = new ProjectCacheEntry(key, project.FilePath, () => - { - logger.LogInformation("Loading project {ProjectFileName}", project.FileName); - var projectService = projectLoader.LoadCache(project); - logger.LogInformation("Project {ProjectFileName} loaded", project.FileName); - return projectService; - }, logger); - _projectCacheEntries.Add(newEntry); - return newEntry; - }) ?? throw new InvalidOperationException("Project service is null"); - } - - LcmCache projectService; - try - { - projectService = projectEntry.LcmCache; - } - catch - { - // The entry caches its load exception, so drop it to let the next call retry the load. - RemoveIfCurrent(projectEntry); - throw; - } - - if (projectService.IsDisposed) - { - throw new InvalidOperationException("Project service is disposed"); + if (cache.TryGetValue(key, out hit) && !hit!.IsDisposed) return hit; + logger.LogInformation("Loading project {ProjectFileName}", project.FileName); + var lcmCache = projectLoader.LoadCache(project); + logger.LogInformation("Project {ProjectFileName} loaded", project.FileName); + lock (_openCachesLock) + { + _openCaches.Add(lcmCache, key); + } + // The entry is committed to the cache when it's disposed. + using (var entry = cache.CreateEntry(key)) + { + entry.SlidingExpiration = CacheSlidingExpiration; + entry.RegisterPostEvictionCallback(OnLcmProjectCacheEviction); + entry.Value = lcmCache; + } + return lcmCache; } - - return projectService; } - private void RemoveIfCurrent(ProjectCacheEntry projectEntry) + private bool Untrack(LcmCache lcmCache) { - lock (_cacheEntryLock) + lock (_openCachesLock) { - if (cache.TryGetValue(projectEntry.Key, out ProjectCacheEntry? current) && ReferenceEquals(current, projectEntry)) - cache.Remove(projectEntry.Key); + return _openCaches.Remove(lcmCache); } } - private void OnLcmProjectCacheEviction(object key, object? value, EvictionReason reason, object? state) + private void OnLcmProjectCacheEviction(object keyObj, object? value, EvictionReason reason, object? state) { - if (value is not ProjectCacheEntry projectEntry) return; + if (value is not LcmCache lcmCache) return; // todo this could trigger when the service is still referenced elsewhere, for example in a long running task. // disposing of the service while it's still in use would be bad. // one way around this would be to return a lease object, only after a timeout and no more references to the lease object would the service be disposed. - logger.LogInformation("Evicting project {ProjectFileName} from cache ({EvictionReason})", projectEntry.FilePath, reason); - lock (_cacheEntryLock) - { - _projectCacheEntries.Remove(projectEntry); - } - _ = projectEntry.DisposeLoaded().ContinueWith(disposal => - { - if (disposal.IsFaulted) - logger.LogError(disposal.Exception, "Failed to dispose project {ProjectFileName}", projectEntry.FilePath); - GC.Collect(); - }, TaskScheduler.Default); + var filePath = FilePathFromCacheKey((string)keyObj); + logger.LogInformation("Evicting project {ProjectFileName} from cache ({EvictionReason})", filePath, reason); + if (!Untrack(lcmCache) || lcmCache.IsDisposed) return; + lcmCache.Dispose(); + logger.LogInformation("FW Data Project {ProjectFileName} disposed", filePath); + GC.Collect(); } public void Dispose() { logger.LogInformation("Closing all projects"); - HashSet projectEntries; - lock (_cacheEntryLock) + Dictionary openCaches; + lock (_openCachesLock) { - projectEntries = _projectCacheEntries; - _projectCacheEntries = []; + openCaches = _openCaches; + _openCaches = []; } - foreach (var projectEntry in projectEntries) + foreach (var (lcmCache, key) in openCaches) { - //need to explicitly dispose as that blocks, just removing from the cache does not block, meaning it will not finish disposing before the program exits. - var disposal = projectEntry.DisposeLoaded(); - // Not waited for: a load that hasn't finished has no changes to save, and exiting releases its file locks. - if (!disposal.IsCompleted) - logger.LogWarning("Project {ProjectFileName} is still loading at shutdown, not waiting for it", projectEntry.FilePath); - else if (disposal.IsFaulted) - logger.LogError(disposal.Exception, "Failed to dispose project {ProjectFileName}", projectEntry.FilePath); - RemoveIfCurrent(projectEntry); + if (!lcmCache.IsDisposed) + { + //need to explicitly call dispose as that blocks, just removing from the cache does not block, meaning it will not finish disposing before the program exits. + lcmCache.Dispose(); + logger.LogInformation("FW Data Project {ProjectFileName} disposed", FilePathFromCacheKey(key)); + } + if (cache.TryGetValue(key, out LcmCache? current) && ReferenceEquals(current, lcmCache)) + cache.Remove(key); } } @@ -147,21 +133,20 @@ public async Task CloseProjectAsync(FwDataProject project) // if we are shutting down, don't do anything because we want project dispose to be called as part of the shutdown process. if (_shuttingDown) return; logger.LogInformation("Explicitly Closing project {ProjectFileName}", project.FilePath); - var projectEntry = TakeEntry(CacheKey(project)); - if (projectEntry is null) return; + var key = CacheKey(project); // Dispose immediately (waiting for a load still in flight) so file locks are released before we return. // The caller assumes the project is ready to be opened somewhere else (e.g. in FieldWorks) - await Task.Run(projectEntry.DisposeLoaded); - } - - private ProjectCacheEntry? TakeEntry(string key) - { - lock (_cacheEntryLock) + await Task.Run(() => { - if (!cache.TryGetValue(key, out ProjectCacheEntry? projectEntry)) return null; - cache.Remove(key); - return projectEntry; - } + lock (KeyLock(key)) + { + if (!cache.TryGetValue(key, out LcmCache? lcmCache) || lcmCache is null) return; + // Untracked first, so the eviction callback that Remove triggers leaves the disposal to us. + var owned = Untrack(lcmCache); + cache.Remove(key); + if (owned && !lcmCache.IsDisposed) lcmCache.Dispose(); + } + }); } public IAsyncDisposable DeferCloseAsync(FwDataProject project) @@ -199,78 +184,4 @@ public Task StopAsync(CancellationToken cancellationToken) Dispose(); return Task.CompletedTask; } - - private sealed class ProjectCacheEntry - { - // Lets disposal wait for the load without blocking a thread on the Lazy. - private readonly TaskCompletionSource _loaded = new(TaskCreationOptions.RunContinuationsAsynchronously); - private LcmCache? _loadedCache; - private readonly Lazy _lcmCache; - private readonly TaskCompletionSource _disposed = new(TaskCreationOptions.RunContinuationsAsynchronously); - private int _disposeRequested; - private readonly ILogger _logger; - - public ProjectCacheEntry(string key, string filePath, Func load, ILogger logger) - { - Key = key; - FilePath = filePath; - _logger = logger; - _lcmCache = new(() => - { - try - { - var lcmCache = load(); - _loadedCache = lcmCache; - _loaded.SetResult(); - return lcmCache; - } - catch (Exception e) - { - _loaded.SetException(e); - throw; - } - }, LazyThreadSafetyMode.ExecutionAndPublication); - } - - public string Key { get; } - public string FilePath { get; } - public LcmCache LcmCache => _lcmCache.Value; - - /// - /// Disposes the LcmCache once its load finishes, synchronously if it already has. - /// Runs once however many of eviction, close and shutdown call it; every caller gets the same task. - /// - public Task DisposeLoaded() - { - if (Interlocked.Exchange(ref _disposeRequested, 1) == 0) - { - if (_loaded.Task.IsCompleted) DisposeAfterLoad(_loaded.Task); - else _ = _loaded.Task.ContinueWith(DisposeAfterLoad, TaskScheduler.Default); - } -#pragma warning disable VSTHRD003 // Avoid awaiting foreign Tasks: this entry owns the TaskCompletionSource - return _disposed.Task; -#pragma warning restore VSTHRD003 - } - - private void DisposeAfterLoad(Task load) - { - try - { - if (!load.IsCompletedSuccessfully) - { - _logger.LogWarning(load.Exception, "FW Data Project {ProjectFileName} failed to load, nothing to dispose", FilePath); - } - else if (_loadedCache is { IsDisposed: false } lcmCache) - { - lcmCache.Dispose(); - _logger.LogInformation("FW Data Project {ProjectFileName} disposed", FilePath); - } - _disposed.SetResult(); - } - catch (Exception e) - { - _disposed.SetException(e); - } - } - } } From e7292f301da4f55d831208a4fd6796aecb42317f Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 25 Sep 2026 12:54:30 -0400 Subject: [PATCH 6/7] Note that FwDataFactory's per-project locks are never pruned Co-Authored-By: Claude Opus 5.5 (1M context) --- backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs b/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs index 24e7ad9959..069115b7b3 100644 --- a/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs +++ b/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs @@ -49,6 +49,7 @@ public FwDataMiniLcmApi GetFwDataMiniLcmApi(FwDataProject project, bool saveOnDi // One lock per project, held for the whole load, so concurrent requests share a load instead of racing // (IMemoryCache.GetOrCreate isn't atomic) and the cache only ever holds a finished LcmCache. + // Never pruned: it grows by one small Lock per distinct project path this factory opens. private readonly ConcurrentDictionary _keyLocks = new(); private Lock KeyLock(string key) => _keyLocks.GetOrAdd(key, _ => new()); From d267b089f244f80621f192cd1466026c36742594 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 25 Sep 2026 13:43:54 -0400 Subject: [PATCH 7/7] Dispose an expired FwData cache before reloading or closing An expired entry's eviction callback runs on the thread pool, so a reload could start LoadCache before the old LcmCache was disposed, and disposing it afterwards deleted the new copy's lock file. Close could also return while that callback was still disposing. Reload and close now dispose any tracked copies of the project under its lock first, and the eviction callback takes the same lock. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../FwDataFactoryTests.cs | 40 ++++++++++++++++++ .../FwDataMiniLcmBridge/FwDataFactory.cs | 41 +++++++++++++++---- 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs b/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs index 85c83dad3b..3930e87251 100644 --- a/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs +++ b/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs @@ -167,6 +167,44 @@ public async Task RequestDuringACloseGetsANewCacheAfterIt() _gatedLoader.LoadCount.Should().Be(2); } + [Fact] + public async Task CloseRightAfterRemovalDisposesTheOldCache() + { + _gatedLoader.Release.Set(); + for (var i = 0; i < 5; i++) + { + var project = NewProject("close-after-removal"); + var lcmCache = _mockLoader.NewProject(project, "en", "en"); + GetCache(project).Should().BeSameAs(lcmCache); + + // Queues the eviction callback, as expiry does, so close races it for the key lock. + _memoryCache.Remove(FwDataFactory.CacheKey(project)); + await _factory.CloseProjectAsync(project); + + lcmCache.IsDisposed.Should().BeTrue($"iteration {i}"); + } + } + + [Fact] + public void ReloadAfterRemovalDisposesTheOldCacheBeforeLoading() + { + _gatedLoader.Release.Set(); + var project = NewProject("reload-after-removal"); + var firstCache = _mockLoader.NewProject(project, "en", "en"); + GetCache(project).Should().BeSameAs(firstCache); + var secondCache = _mockLoader.NewProject(NewProject("reload-after-removal-second"), "en", "en"); + _mockLoader.Projects[project.Name] = secondCache; + bool? firstDisposedWhenLoading = null; + _gatedLoader.OnLoadCache = () => firstDisposedWhenLoading = firstCache.IsDisposed; + + _memoryCache.Remove(FwDataFactory.CacheKey(project)); + var reloaded = GetCache(project); + + firstDisposedWhenLoading.Should().BeTrue(); + reloaded.Should().BeSameAs(secondCache); + reloaded.IsDisposed.Should().BeFalse(); + } + // The requester may get the cache or an "already disposed" error, depending on how it races the disposal. private static async Task IgnoreFailure(Task load) { @@ -181,10 +219,12 @@ private class GatedProjectLoader(MockFwProjectLoader inner) : IProjectLoader public ManualResetEventSlim Entered { get; } = new(); public ManualResetEventSlim Release { get; } = new(); public bool FailNext { get; set; } + public Action? OnLoadCache { get; set; } public LcmCache LoadCache(FwDataProject project) { Interlocked.Increment(ref _loadCount); + OnLoadCache?.Invoke(); if (FailNext) { FailNext = false; diff --git a/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs b/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs index 069115b7b3..f8e46ae090 100644 --- a/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs +++ b/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs @@ -67,6 +67,7 @@ private LcmCache GetProjectServiceCached(FwDataProject project) lock (KeyLock(key)) { if (cache.TryGetValue(key, out hit) && !hit!.IsDisposed) return hit; + DisposeTrackedCaches(key); logger.LogInformation("Loading project {ProjectFileName}", project.FileName); var lcmCache = projectLoader.LoadCache(project); logger.LogInformation("Project {ProjectFileName} loaded", project.FileName); @@ -93,16 +94,37 @@ private bool Untrack(LcmCache lcmCache) } } + // Call with KeyLock(key) held: an expired entry's eviction callback may not have run yet. + private void DisposeTrackedCaches(string key) + { + List untracked; + lock (_openCachesLock) + { + untracked = _openCaches.Where(open => open.Value == key).Select(open => open.Key).ToList(); + foreach (var lcmCache in untracked) _openCaches.Remove(lcmCache); + } + foreach (var lcmCache in untracked) + { + if (lcmCache.IsDisposed) continue; + lcmCache.Dispose(); + logger.LogInformation("FW Data Project {ProjectFileName} disposed", FilePathFromCacheKey(key)); + } + } + private void OnLcmProjectCacheEviction(object keyObj, object? value, EvictionReason reason, object? state) { if (value is not LcmCache lcmCache) return; // todo this could trigger when the service is still referenced elsewhere, for example in a long running task. // disposing of the service while it's still in use would be bad. // one way around this would be to return a lease object, only after a timeout and no more references to the lease object would the service be disposed. - var filePath = FilePathFromCacheKey((string)keyObj); + var key = (string)keyObj; + var filePath = FilePathFromCacheKey(key); logger.LogInformation("Evicting project {ProjectFileName} from cache ({EvictionReason})", filePath, reason); - if (!Untrack(lcmCache) || lcmCache.IsDisposed) return; - lcmCache.Dispose(); + lock (KeyLock(key)) + { + if (!Untrack(lcmCache) || lcmCache.IsDisposed) return; + lcmCache.Dispose(); + } logger.LogInformation("FW Data Project {ProjectFileName} disposed", filePath); GC.Collect(); } @@ -141,11 +163,14 @@ await Task.Run(() => { lock (KeyLock(key)) { - if (!cache.TryGetValue(key, out LcmCache? lcmCache) || lcmCache is null) return; - // Untracked first, so the eviction callback that Remove triggers leaves the disposal to us. - var owned = Untrack(lcmCache); - cache.Remove(key); - if (owned && !lcmCache.IsDisposed) lcmCache.Dispose(); + if (cache.TryGetValue(key, out LcmCache? lcmCache) && lcmCache is not null) + { + // Untracked first, so the eviction callback that Remove triggers leaves the disposal to us. + var owned = Untrack(lcmCache); + cache.Remove(key); + if (owned && !lcmCache.IsDisposed) lcmCache.Dispose(); + } + DisposeTrackedCaches(key); } }); }