From b0f7d59c9631ab163bfc17d08fa0e5ef9b42b8da Mon Sep 17 00:00:00 2001 From: Meta Vens Date: Fri, 2 Oct 2026 16:51:54 +0200 Subject: [PATCH] Directory store: a refused foreign overwrite leaves the pending set with its failed write WriteGeneration left the refused key in the dirty and deleted sets, so every later generation refused it. The refusal is now one check before any staging, shared by flush and commit, that removes the refused keys from the pending sets. Tests cover both store kinds. Co-Authored-By: Claude Opus 5.5 --- .../DirectoryMessagePackBackingStore.cs | 37 +++++++++++++----- .../ForeignRecordTests.cs | 38 +++++++++++++++++++ 2 files changed, 66 insertions(+), 9 deletions(-) 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)