diff --git a/src/NexusMods.App.GarbageCollection.DataModel/RunGarbageCollector.cs b/src/NexusMods.App.GarbageCollection.DataModel/RunGarbageCollector.cs index ae38a5452..188be0edb 100644 --- a/src/NexusMods.App.GarbageCollection.DataModel/RunGarbageCollector.cs +++ b/src/NexusMods.App.GarbageCollection.DataModel/RunGarbageCollector.cs @@ -1,7 +1,9 @@ using NexusMods.Abstractions.IO; using NexusMods.Abstractions.Settings; using NexusMods.App.GarbageCollection.Nx; +using NexusMods.Hashing.xxHash64; using NexusMods.MnemonicDB.Abstractions; +using NexusMods.Paths; namespace NexusMods.App.GarbageCollection.DataModel; /// @@ -10,25 +12,55 @@ namespace NexusMods.App.GarbageCollection.DataModel; /// public static class RunGarbageCollector { + private static readonly AsyncFriendlyReaderWriterLock _gcLock = new(); + /// /// The archive locations, usually obtained from 'DataModelSettings'. /// The that requires locking for concurrent access. /// The MneumonicDB to the DataModel. public static void Do(Span archiveLocations, IFileStore store, IConnection connection) { - using var lck = store.Lock(); + // See 'SAFETY' comment below for explanation of 'gcLock' + using var gcLock = _gcLock.WriteLock(); + var toUpdateInDataStore = new List(); + + using (store.Lock()) + { + var gc = new ArchiveGarbageCollector(); + DataStoreNxArchiveFinder.FindAllArchives(archiveLocations, gc); + DataStoreReferenceMarker.MarkUsedFiles(connection, gc); + gc.CollectGarbage(new Progress(), (progress, toArchive, toRemove, archive) => + { + NxRepacker.RepackArchive(progress, toArchive, toRemove, archive, false, out var newArchivePath); + toUpdateInDataStore.Add(new ToUpdateInDataStoreEntry(toRemove, archive.FilePath, newArchivePath)); + }); + } + + // SAFETY: Updating the FileStore interacts with external non-GC components, + // such as MnemonicDB. This may cause us to yield to external code + // that could touch the FileStore lock. To avoid deadlocks, we should + // prevent this from happening if possible. + // + // This is why we release `store.Lock()` early. + + // NOTE: In theory UpdateNxFileStore can call GC back again. This is unlikely to happen + // however for the time being; because we only run GC when deleting a library item + // or loadout. No callback should do that. Long term we want to prevent re-entrancy. + // + // Running arbitrary code in GC in any system is however prone to possible failure, + // so long term we will want to avoid UpdateNxFileStore (MnemonicDB Commit) to avoid + // yielding to external code. We need a non-blocking `Commit`; that + // sends stuff off to another thread or internal queue without blocking. var updater = new NxFileStoreUpdater(connection); - var gc = new ArchiveGarbageCollector(); - DataStoreNxArchiveFinder.FindAllArchives(archiveLocations, gc); - DataStoreReferenceMarker.MarkUsedFiles(connection, gc); - gc.CollectGarbage(new Progress(), (progress, toArchive, toRemove, archive) => + foreach (var entry in toUpdateInDataStore) { - NxRepacker.RepackArchive(progress, toArchive, toRemove, archive, false, out var newArchivePath); - updater.UpdateNxFileStore(toRemove, archive.FilePath, newArchivePath); + updater.UpdateNxFileStore(entry.ToRemove, entry.OldFilePath, entry.NewFilePath); // Delete original archive. We do this in a delayed fashion such that // a power loss during the UpdateNxFileStore operation does not lead // to an inconsistent state - archive.FilePath.Delete(); - }); + entry.OldFilePath.Delete(); + } } + + private record struct ToUpdateInDataStoreEntry(List ToRemove, AbsolutePath OldFilePath, AbsolutePath NewFilePath); } diff --git a/src/NexusMods.DataModel/NxFileStore.cs b/src/NexusMods.DataModel/NxFileStore.cs index f43bb6319..560d2da32 100644 --- a/src/NexusMods.DataModel/NxFileStore.cs +++ b/src/NexusMods.DataModel/NxFileStore.cs @@ -60,6 +60,7 @@ public NxFileStore( /// public ValueTask HaveFile(Hash hash) { + using var lck = _lock.ReadLock(); var db = _conn.Db; var archivedFiles = ArchivedFile.FindByHash(db, hash).Any(x => x.IsValid()); return ValueTask.FromResult(archivedFiles);