diff --git a/src/GameCult.Caching.MessagePack/DirectoryMessagePackBackingStore.cs b/src/GameCult.Caching.MessagePack/DirectoryMessagePackBackingStore.cs index 75709c96..a54e239f 100644 --- a/src/GameCult.Caching.MessagePack/DirectoryMessagePackBackingStore.cs +++ b/src/GameCult.Caching.MessagePack/DirectoryMessagePackBackingStore.cs @@ -68,7 +68,8 @@ public sealed class DirectoryMessagePackBackingStore : CacheBackingStore reports.Clear(); // Only pages named by the manifest read under the lease are loaded; orphaned pages are never loaded. With no // lock to lease (none created yet, or a store copied without it) a writer may commit mid-load, so an unleased - // load stands only if its manifest is still current and every page it named read back; otherwise it reloads. + // load stands only if its manifest is still current; if the manifest moved it reloads, under the lease the + // writer created. A page that fails under an unmoved manifest is corruption, not a race. using var lease = AcquireCommitLease(wait: true, create: false); var manifestBytes = ReadManifestBytes(); var manifest = ParseManifest(manifestBytes); @@ -89,7 +90,7 @@ public sealed class DirectoryMessagePackBackingStore : CacheBackingStore break; } } - catch (Exception exception) when (lease == null && IsTornGeneration(exception)) + catch (Exception exception) when (lease == null && IsTornGeneration(exception) && !SameBytes(manifestBytes, ReadManifestBytes())) { if (attempt == UnleasedLoadAttempts) throw Unsettled(exception); @@ -385,11 +386,12 @@ public sealed class DirectoryMessagePackBackingStore : CacheBackingStore private static bool SameBytes(byte[]? first, byte[]? second) => first == null ? second == null : second != null && first.SequenceEqual(second); - // A page the manifest named that vanished or was replaced: a writer committed mid-load. + // A page the manifest named that vanished or was replaced. It is a writer committing mid-load only when the manifest + // moved too; under an unchanged manifest the same failure is corruption and is thrown as it is. private static bool IsTornGeneration(Exception exception) => exception is AggregateException aggregate ? aggregate.Flatten().InnerExceptions.All(IsTornGeneration) - : exception is InvalidDataException or FileNotFoundException; + : exception is InvalidDataException or FileNotFoundException or DirectoryNotFoundException; private InvalidOperationException Unsettled(Exception? last) => new( $"Directory store {_manifestFile.FullName} changed under every one of {UnleasedLoadAttempts} unlocked loads; it did not settle.", last); diff --git a/tests/GameCult.Caching.Tests/BackingStoreTests.cs b/tests/GameCult.Caching.Tests/BackingStoreTests.cs index c8be4b64..ace8a357 100644 --- a/tests/GameCult.Caching.Tests/BackingStoreTests.cs +++ b/tests/GameCult.Caching.Tests/BackingStoreTests.cs @@ -645,9 +645,9 @@ namespace GameCult.Caching.Tests File.Delete(Path.Combine(recordsPath, ".commit.lock")); var committed = new System.Collections.Generic.List(); // The writer is another cache, so it commits from another thread: a thread holds one cache's gate at a time. + // It commits under every unleased read, so only a retry that takes the lease its commit created can settle. readerStore.UnleasedManifestRead = () => { - if (committed.Count > 0) return; Task.Run(async () => { using var writer = CultCacheMessagePack.Create(filePath, new CultCacheOpenOptions { UseDirectoryStore = true }); @@ -661,7 +661,7 @@ namespace GameCult.Caching.Tests await reader.PullAllBackingStoresAsync(); - Assert.That(committed, Has.Count.EqualTo(1), "the writer committed mid-load"); + Assert.That(committed, Has.Count.EqualTo(1), "the writer committed once; the retry held the lease"); Assert.That(reader.Get(committed[0])?.Value, Is.EqualTo("after"), "the reader reloaded the writer's generation"); } finally @@ -671,6 +671,40 @@ namespace GameCult.Caching.Tests } } + [Test] + public async Task DirectoryMessagePackBackingStore_UnlockedLoad_Throws_Corruption_Under_An_Unmoved_Manifest() + { + var filePath = Path.Combine(Path.GetTempPath(), $"cultlib-tests-{Guid.NewGuid():N}.cc"); + var recordsPath = DirectoryMessagePackBackingStore.DefaultRecordDirectoryPath(filePath); + + try + { + using (var seed = await CultCacheMessagePack.OpenAsync(filePath, new CultCacheOpenOptions { UseDirectoryStore = true })) + { + await seed.UpsertAsync(new NamedTestEntry { Name = "copied", Value = "before" }); + await seed.FlushAsync(); + } + + var readerStore = new DirectoryMessagePackBackingStore(filePath, recordsPath); + var reader = new CultCache(); + reader.AddBackingStore(readerStore); + File.Delete(Path.Combine(recordsPath, ".commit.lock")); + File.WriteAllBytes(Directory.GetFiles(recordsPath, "*.msgpack").Single(), new byte[] { 1, 2, 3 }); + var loads = 0; + readerStore.UnleasedManifestRead = () => loads++; + + var thrown = Assert.CatchAsync(() => reader.PullAllBackingStoresAsync())!; + + Assert.That(loads, Is.EqualTo(1), "corruption is not retried as a race"); + Assert.That(thrown.ToString(), Does.Contain("does not match its committed content hash").And.Not.Contain("did not settle")); + } + finally + { + if (File.Exists(filePath)) File.Delete(filePath); + if (Directory.Exists(recordsPath)) Directory.Delete(recordsPath, recursive: true); + } + } + [Test] public async Task DirectoryMessagePackBackingStore_Deletion_Commits_While_Locked_Old_Page_Remains_An_Orphan() {