From 9cee5e3f429b00a422b0321dc55358b61e176e9b Mon Sep 17 00:00:00 2001 From: Meta Vens Date: Wed, 30 Sep 2026 11:04:12 +0200 Subject: [PATCH] C#: no store writer restamps a record; the directory store has one source of arrived entries Deleted the heuristic that rewrote a record the cache held "unchanged since read" (same key and storedAt) under the registered id, in the single-file commit and in the directory generation. It could overwrite a record another writer changed while keeping its storedAt. A commit onto a file now writes its batch and leaves every other record as the file holds it; a record under an id only an arrived entry lists is refused, typed, with the file left as it was. Deleted the directory store's _durableCatalog, a second stale-able source of arrived entries beside the manifest read under the lease. Tests: the lost-update probe, the two refusals that replace the restamp tests, the arrived-overwrites-registered guard in both id orders, the content-hash tie-break against the compatible-list order, and refusal determinism across record arrival order. Co-Authored-By: Claude Sonnet 5.5 --- .../DirectoryMessagePackBackingStore.cs | 16 +-- src/GameCult.Caching/CultCache.cs | 14 --- .../StoreReadabilityTests.cs | 113 ++++++++++++++---- 3 files changed, 92 insertions(+), 51 deletions(-) diff --git a/src/GameCult.Caching.MessagePack/DirectoryMessagePackBackingStore.cs b/src/GameCult.Caching.MessagePack/DirectoryMessagePackBackingStore.cs index 4dee3a2f..167218d9 100644 --- a/src/GameCult.Caching.MessagePack/DirectoryMessagePackBackingStore.cs +++ b/src/GameCult.Caching.MessagePack/DirectoryMessagePackBackingStore.cs @@ -25,7 +25,6 @@ public sealed class DirectoryMessagePackBackingStore : CacheBackingStore private readonly ConcurrentDictionary _dirtyKeys = new(StringComparer.Ordinal); private readonly ConcurrentDictionary _deletedKeys = new(StringComparer.Ordinal); private readonly HashSet _hydratedKeys = new(StringComparer.Ordinal); - private CultSchemaCatalogEntry[] _durableCatalog = Array.Empty(); public DirectoryMessagePackBackingStore(string manifestPath, string? recordDirectoryPath = null, bool readOnly = false) : base(readOnly) @@ -64,7 +63,6 @@ public sealed class DirectoryMessagePackBackingStore : CacheBackingStore var reports = new List(); var loaded = new Dictionary(StringComparer.Ordinal); - CultSchemaCatalogEntry[] catalog; for (var attempt = 1; ; attempt++) { loaded.Clear(); @@ -88,7 +86,6 @@ public sealed class DirectoryMessagePackBackingStore : CacheBackingStore reports); if (lease != null || !ManifestMoved(manifestBytes)) { - catalog = manifest.SchemaCatalog; Trace($"indexed-pages loaded={loaded.Count}"); break; } @@ -123,7 +120,6 @@ public sealed class DirectoryMessagePackBackingStore : CacheBackingStore Loaded?.Invoke(arrived, departed); // Adopted only once the cache admitted the load; a refused load leaves the store's durable view untouched. - _durableCatalog = catalog; foreach (var stored in departed) Entries.TryRemove(stored.Key.Value, out _); _hydratedKeys.RemoveWhere(key => !loaded.ContainsKey(key) && !Staged(key)); @@ -251,16 +247,7 @@ public sealed class DirectoryMessagePackBackingStore : CacheBackingStore currentIndex[key] = ToIndexRecord(stored); } - // A record this cache holds, unchanged since the manifest was read, is rewritten under its registered schema's id, as a dirty - // one is: the manifest entry that lists an older id for it is then not needed to publish it. - var restamped = currentIndex.Values - .Where(record => !_dirtyKeys.ContainsKey(record.Key) && - Entries.TryGetValue(record.Key, out var known) && known.StoredAt == record.StoredAt && - !string.Equals(known.Descriptor.SchemaId, record.SchemaId, StringComparison.Ordinal)) - .Select(record => record.Key) - .ToArray(); var keysToWrite = _dirtyKeys.Keys - .Concat(restamped) .OrderBy(value => value, StringComparer.Ordinal) .ToArray(); foreach (var key in keysToWrite) @@ -286,7 +273,7 @@ public sealed class DirectoryMessagePackBackingStore : CacheBackingStore var targetCatalog = CultSchemaCatalogEntry.Derive( currentIndex.Values, Entries.Values.Select(entry => entry.Descriptor.ToCatalogEntry()).ToArray(), - currentManifest.SchemaCatalog.Concat(_durableCatalog).ToArray()) + currentManifest.SchemaCatalog) .OrderBy(entry => entry.SchemaName, StringComparer.Ordinal) .ThenBy(entry => entry.SchemaId, StringComparer.Ordinal) .ToArray(); @@ -303,7 +290,6 @@ public sealed class DirectoryMessagePackBackingStore : CacheBackingStore DeleteUnreferencedRecordPages(currentIndex.Values); - _durableCatalog = targetCatalog; _dirtyKeys.Clear(); _deletedKeys.Clear(); MarkFlushSucceeded(); diff --git a/src/GameCult.Caching/CultCache.cs b/src/GameCult.Caching/CultCache.cs index 7cbac9f4..a1db32f9 100644 --- a/src/GameCult.Caching/CultCache.cs +++ b/src/GameCult.Caching/CultCache.cs @@ -3625,20 +3625,6 @@ namespace GameCult.Caching records.Remove(entry.Key.Value); foreach (var entry in request.Upserts) records[entry.Key.Value] = ToPersistedRecord(entry, SerializePayload); - if (ontoDisk) - { - // A record this cache holds, unchanged since the file was read, is written under its registered schema's id, as a - // whole-view write writes it: the file's entry that lists an older id for it is then not needed to publish it. - foreach (var record in disk.Records) - { - if (records.TryGetValue(record.Key, out var carried) && ReferenceEquals(carried, record) && - Entries.TryGetValue(record.Key, out var known) && known.StoredAt == record.StoredAt && - !string.Equals(known.Descriptor.SchemaId, record.SchemaId, StringComparison.Ordinal)) - { - records[record.Key] = ToPersistedRecord(known, SerializePayload); - } - } - } // Onto the file, only the batch's records are seen: the rest stay as they are, and the header they carry stays. An // unconditional commit writes this store's whole view, so what its records hold decides. A record this commit diff --git a/tests/GameCult.Caching.Tests/StoreReadabilityTests.cs b/tests/GameCult.Caching.Tests/StoreReadabilityTests.cs index 642c6e63..4f0808bf 100644 --- a/tests/GameCult.Caching.Tests/StoreReadabilityTests.cs +++ b/tests/GameCult.Caching.Tests/StoreReadabilityTests.cs @@ -208,8 +208,9 @@ namespace GameCult.Caching.Tests [Test] public void EntriesThatTieAreTakenInOneFixedOrder() { - var one = Entry("x", "n", "h1", "x"); - var two = Entry("x", "n", "h2", "x", "y"); + // The content hash orders them against the order their compatible ids would give: h1 sorts first, its list sorts last. + var one = Entry("x", "n", "h2", "x"); + var two = Entry("x", "n", "h1", "x", "y"); foreach (var order in new[] { new[] { one, two }, new[] { two, one } }) { Assert.That(CultSchemaCatalogEntry.Derive(new[] { Rec("a", "x") }, order, Array.Empty()).Single().ContentHash, Is.EqualTo("h1")); @@ -217,6 +218,37 @@ namespace GameCult.Caching.Tests } } + // A registered descriptor that owns an id keeps it against an arrived entry that owns it too and lists the id another + // record sits under, whichever of the two ids sorts first: the arrived entry's list is not merged in, so the record under + // the listed id is published by no chosen entry. + [TestCase("a", "b")] + [TestCase("b", "a")] + public void AnArrivedEntryThatOwnsARegisteredIdDoesNotReplaceTheDescriptorNorPublishWhatItLists(string ownId, string listedId) + { + var registered = new[] { Entry(ownId, "registered", "fresh", ownId) }; + var arrived = new[] { Entry(ownId, "arrived", "stale", ownId, listedId) }; + var records = new[] { Rec("k1", ownId), Rec("k2", listedId) }; + + var refusal = Assert.Throws(() => CultSchemaCatalogEntry.Derive(records, registered, arrived))!; + Assert.That(refusal.SchemaId, Is.EqualTo(listedId)); + Assert.That(refusal.RecordKey, Is.EqualTo("k2")); + } + + // Ids are taken in sorted order, so which entry survives, and so which id the refusal names, does not depend on the order + // the records arrive in. + [Test] + public void TheRefusalNamesTheSameIdWhateverOrderTheRecordsArriveIn() + { + var arrived = new[] { Entry("x", "n", "h1", "x", "y"), Entry("x", "n", "h2", "x", "z") }; + foreach (var records in new[] { new[] { Rec("r1", "y"), Rec("r2", "z") }, new[] { Rec("r2", "z"), Rec("r1", "y") } }) + { + var refusal = Assert.Throws(() => + CultSchemaCatalogEntry.Derive(records, Array.Empty(), arrived))!; + Assert.That(refusal.SchemaId, Is.EqualTo("y")); + Assert.That(refusal.RecordKey, Is.EqualTo("r1")); + } + } + [Test] public void TwoArrivedEntriesThatShareAnOwnIdAndDisagreeOnTheSchemaNameRefuseTheWrite() { @@ -255,8 +287,11 @@ namespace GameCult.Caching.Tests return path; } + // The cache holds the record d, but a commit onto the file writes only its batch: d stays as the file holds it, under an id + // only the file's entry lists, and the registered descriptor does not. No store writer restamps a record; the write is + // refused and the file left as it was. [Test] - public void ACommitOntoAFileWhoseEntryListsAnOlderIdLeavesAStoreThatReopens() + public void ACommitOntoAFileWhoseCachedRecordSitsUnderAnIdOnlyAnArrivedEntryListsIsRefusedAndTheFileLeftAsItWas() { var path = Seed("legacy-conditional.cc"); var manifest = CultDocumentMessagePackSerialization.DeserializeSnapshot(File.ReadAllBytes(path)); @@ -264,21 +299,50 @@ namespace GameCult.Caching.Tests manifest.Records.Single().SchemaId = "old.id"; entry.ContentHash = "stale"; entry.CompatibleSchemaIds = new[] { entry.SchemaId, "old.id" }; - File.WriteAllBytes(path, CultDocumentMessagePackSerialization.SerializeSnapshot(manifest)); + var bytes = CultDocumentMessagePackSerialization.SerializeSnapshot(manifest); + File.WriteAllBytes(path, bytes); - using (var cache = CultCacheMessagePack.Create(path, new CultCacheOpenOptions { Registry = Registry })) + using var cache = CultCacheMessagePack.Create(path, new CultCacheOpenOptions { Registry = Registry }); + Assert.That(cache.Get(new CultRecordKey("d")), Is.Not.Null, "the cache reads the record under the older id"); + var refusal = Assert.Throws(() => cache.Commit(batch => { - Assert.That(cache.Commit(batch => - { - batch.Expect(new CultRecordKey("e"), null); - batch.Upsert(typeof(IdDeck), new IdDeck { Name = "e" }, new CultRecordKey("e")); - }), Is.True); - } + batch.Expect(new CultRecordKey("e"), null); + batch.Upsert(typeof(IdDeck), new IdDeck { Name = "e" }, new CultRecordKey("e")); + }))!; + Assert.That(refusal.SchemaId, Is.EqualTo("old.id")); + Assert.That(refusal.RecordKey, Is.EqualTo("d")); + Assert.That(File.ReadAllBytes(path), Is.EqualTo(bytes)); + } - var written = CultDocumentMessagePackSerialization.DeserializeSnapshot(File.ReadAllBytes(path)); - Assert.That(written.SchemaCatalog.Single().ContentHash, Is.Not.EqualTo("stale")); - using var reopened = CultCacheMessagePack.Create(path, new CultCacheOpenOptions { Registry = Registry }); - Assert.That(reopened.Get(new CultRecordKey("d")), Is.Not.Null, "the record kept under the older id stays readable"); + // Another writer rewrites the record d under a different id, keeping its storedAt. A cache that read d earlier commits a + // record of its own onto the file: the other writer's d is not the cache's to rewrite, so it is left exactly as written. + [Test] + public void ACommitNeverRewritesARecordAnotherWriterChangedThatKeptItsStoredAt() + { + var path = Seed("lost-update.cc"); + var other = Seed("lost-update-other.cc"); + using (var cache = CultCacheMessagePack.Create(other, new CultCacheOpenOptions { Registry = Registry })) + cache.Commit(batch => batch.Upsert(typeof(IdDeck), new IdDeck { Name = "changed-by-other" }, new CultRecordKey("d"))); + var otherPayload = CultDocumentMessagePackSerialization.DeserializeSnapshot(File.ReadAllBytes(other)).Records.Single().Payload; + + using var reader = CultCacheMessagePack.Create(path, new CultCacheOpenOptions { Registry = Registry }); + Assert.That(reader.Get(new CultRecordKey("d"))!.Name, Is.EqualTo("d")); + + var manifest = CultDocumentMessagePackSerialization.DeserializeSnapshot(File.ReadAllBytes(path)); + var entry = manifest.SchemaCatalog.Single(); + var record = manifest.Records.Single(); + record.SchemaId = "other.writer.id"; + record.Payload = otherPayload; + entry.CompatibleSchemaIds = new[] { entry.SchemaId, "other.writer.id" }; + var bytes = CultDocumentMessagePackSerialization.SerializeSnapshot(manifest); + File.WriteAllBytes(path, bytes); + + Assert.Throws(() => reader.Commit(batch => + { + batch.Expect(new CultRecordKey("e"), null); + batch.Upsert(typeof(IdDeck), new IdDeck { Name = "e" }, new CultRecordKey("e")); + })); + Assert.That(File.ReadAllBytes(path), Is.EqualTo(bytes), "the other writer's record is on disk as it wrote it"); } [Test] @@ -320,10 +384,10 @@ namespace GameCult.Caching.Tests Assert.That(File.ReadAllBytes(path), Is.EqualTo(bytes)); } - // The directory store derives its manifest the same way: a record kept under an older id, the entry naming an older schema, - // and a commit leaves a manifest that reads, with the registered entry. + // The directory store derives its manifest the same way and restamps nothing: a record kept under an older id only the + // manifest's entry lists is refused, and the manifest is left as it was. [Test] - public void ADirectoryRewriteWhoseEntryListsAnOlderIdLeavesAManifestThatReads() + public void ADirectoryRewriteWhoseKeptRecordSitsUnderAnIdOnlyTheManifestEntryListsIsRefusedAndTheManifestLeftAsItWas() { var path = DirectoryStore("legacy-dir.cc"); var manifest = CultDocumentMessagePackSerialization.DeserializeSnapshot(File.ReadAllBytes(path)); @@ -331,13 +395,18 @@ namespace GameCult.Caching.Tests entry.ContentHash = "stale"; entry.CompatibleSchemaIds = new[] { entry.SchemaId, "old.id" }; manifest.Records.Single().SchemaId = "old.id"; - File.WriteAllBytes(path, CultDocumentMessagePackSerialization.SerializeSnapshot(manifest)); + var bytes = CultDocumentMessagePackSerialization.SerializeSnapshot(manifest); + File.WriteAllBytes(path, bytes); using (var cache = CultCacheMessagePack.Create(path, new CultCacheOpenOptions { Registry = Registry, UseDirectoryStore = true })) - cache.Commit(batch => batch.Upsert(typeof(IdDeck), new IdDeck { Name = "e" }, new CultRecordKey("e"))); + { + var refusal = Assert.Throws(() => + cache.Commit(batch => batch.Upsert(typeof(IdDeck), new IdDeck { Name = "e" }, new CultRecordKey("e"))))!; + Assert.That(refusal.SchemaId, Is.EqualTo("old.id")); + Assert.That(refusal.RecordKey, Is.EqualTo("d")); + } - var rewritten = CultDocumentMessagePackSerialization.DeserializeSnapshot(File.ReadAllBytes(path)); - Assert.That(rewritten.SchemaCatalog.Single().ContentHash, Is.Not.EqualTo("stale")); + Assert.That(File.ReadAllBytes(path), Is.EqualTo(bytes)); using var reopened = CultCacheMessagePack.Create(path, new CultCacheOpenOptions { Registry = Registry, UseDirectoryStore = true }); Assert.That(reopened.Get(new CultRecordKey("d")), Is.Not.Null); }