diff --git a/src/GameCult.Caching.MessagePack/DirectoryMessagePackBackingStore.cs b/src/GameCult.Caching.MessagePack/DirectoryMessagePackBackingStore.cs index d07704fa..44536c90 100644 --- a/src/GameCult.Caching.MessagePack/DirectoryMessagePackBackingStore.cs +++ b/src/GameCult.Caching.MessagePack/DirectoryMessagePackBackingStore.cs @@ -186,6 +186,7 @@ public sealed class DirectoryMessagePackBackingStore : CacheBackingStore if (!request.ConditionsHold(manifest.Records, Entries.Values)) return CultCommitOutcome.Mismatch; + RefuseForeign(manifest, request.Upserts.Concat(request.Deletes).Select(entry => entry.Key.Value)); var previousEntries = Entries.ToDictionary(pair => pair.Key, pair => pair.Value, StringComparer.Ordinal); var previousDirtyKeys = _dirtyKeys.Keys.ToArray(); var previousDeletedKeys = _deletedKeys.Keys.ToArray(); @@ -235,23 +236,41 @@ public sealed class DirectoryMessagePackBackingStore : CacheBackingStore Directory.CreateDirectory(_manifestFile.DirectoryName!); using var commitLease = AcquireCommitLease(wait: true, create: true); - WriteGeneration(ReadManifest()); + var manifest = ReadManifest(); + RefuseForeign(manifest, Array.Empty()); + WriteGeneration(manifest); }); } + // The manifest's other records stay as they are, foreign ones included; a write that would replace or remove a foreign + // record is refused before any page is written. The refusal takes the refused keys with it: a staged change to a record that + // is foreign now, and this store's copy of one, are not what the file holds, and kept they would refuse every later write. + private void RefuseForeign(CultPersistedStoreSnapshot currentManifest, IEnumerable written) + { + var foreign = currentManifest.Records + .Where(record => Foreign(record, currentManifest.SchemaCatalog) != null) + .Select(record => record.Key) + .ToHashSet(StringComparer.Ordinal); + var refused = written.Concat(_deletedKeys.Keys).Concat(_dirtyKeys.Keys).Where(foreign.Contains).Distinct().ToArray(); + if (refused.Length == 0) + return; + foreach (var key in refused) + { + Entries.TryRemove(key, out _); + _dirtyKeys.TryRemove(key, out _); + _deletedKeys.TryRemove(key, out _); + } + + IsDirty = !_dirtyKeys.IsEmpty || !_deletedKeys.IsEmpty; + var held = currentManifest.Records.First(record => record.Key == refused[0]); + throw Overwrites(Foreign(held, currentManifest.SchemaCatalog)!); + } + // Runs under the commit lease: pages first, then the manifest that names them. private void WriteGeneration(CultPersistedStoreSnapshot currentManifest) { Directory.CreateDirectory(_recordDirectory.FullName); var currentIndex = currentManifest.Records.ToDictionary(record => record.Key, record => record, StringComparer.Ordinal); - // The manifest's other records stay as they are, foreign ones included; a write that would replace or remove a foreign - // record is refused before any page is written. - foreach (var key in _deletedKeys.Keys.Concat(_dirtyKeys.Keys)) - { - if (currentIndex.TryGetValue(key, out var held) && Foreign(held, currentManifest.SchemaCatalog) is { } carried) - throw Overwrites(carried); - } - foreach (var key in _deletedKeys.Keys) currentIndex.Remove(key); foreach (var key in _dirtyKeys.Keys) diff --git a/tests/GameCult.Caching.Tests/ForeignRecordTests.cs b/tests/GameCult.Caching.Tests/ForeignRecordTests.cs index c00c8292..9c0801bf 100644 --- a/tests/GameCult.Caching.Tests/ForeignRecordTests.cs +++ b/tests/GameCult.Caching.Tests/ForeignRecordTests.cs @@ -202,6 +202,44 @@ namespace GameCult.Caching.Tests Assert.That(Fingerprint(path), Is.EqualTo(before)); } + // A refusal does not poison the store: what the cache held for the refused record goes with the refusal, so a later write + // to another record lands and the file keeps the other writer's record. + [Test] + public void AfterARefusedWriteAWriteToAnotherRecordLands( + [Values(false, true)] bool directory, + [Values(Write.Flush, Write.Commit)] Write refused) + { + var path = Seeded("unstuck.cc", directory); + using var cache = Open(path, DeckOnly, directory); + using (var other = Open(path, Full, directory)) + other.Commit(batch => batch.Upsert(Widget, WidgetOf("now a widget"), D)); + var widget = Stored(path, D); + + Assert.Throws(() => Land(cache, refused, D, DeckOf(Canary))); + Land(cache, Write.Commit, E, DeckOf("e")); + + Assert.That(Stored(path, D), Is.EqualTo(widget), "the other writer's record is as it was"); + Assert.That(EmittedDocumentTypes.Read(cache.Get(E)!, "Name"), Is.EqualTo("e")); + Assert.That(Read(path).Records.Select(record => record.Key), Does.Contain("e")); + } + + // The same when the stale copy was only held, never written: a flush after the other writer's record landed refuses once. + [Test] + public void AFlushRefusedForAStaleCopyOfAForeignRecordRefusesOnce([Values(false, true)] bool directory) + { + var path = Seeded("stale.cc", directory); + using var cache = Open(path, DeckOnly, directory); + using (var other = Open(path, Full, directory)) + other.Commit(batch => batch.Upsert(Widget, WidgetOf("now a widget"), D)); + var widget = Stored(path, D); + + Assert.Throws(() => Land(cache, Write.Commit, D, DeckOf(Canary))); + Land(cache, Write.Flush, E, DeckOf("e")); + Land(cache, Write.Flush, E, DeckOf("e again")); + + Assert.That(Stored(path, D), Is.EqualTo(widget)); + } + [TestCase(false)] [TestCase(true)] public void RemovingARecordAnotherWriterMadeForeignIsRefused(bool directory)