From 22c87fd5300404060904d4cfdfb24b5968f464e1 Mon Sep 17 00:00:00 2001 From: Meta Vens Date: Sun, 25 Oct 2026 20:09:12 +0100 Subject: [PATCH] Caching: a cacheless write never re-enters its store's gate (Soul s3 attach-window-cacheless-write) InStoreHold gains a cacheless flag: on re-entry of the same gate a cacheless call throws instead of running, so the attach's hold (until it publishes Cache) and an enclosing store call's user code cannot write the store behind the cache. Gate passes the flag; no field mirrors Cache. Tests through a real attach and a nested cacheless write, a header pin for attached variant stores, contract and CHANGELOG. Builds on the previous commit; neither is built or run until the verify job. Co-Authored-By: Claude Sonnet 5.5 --- .../Contracts/cultcache-store-composition.md | 11 ++-- src/GameCult.Caching/CultCache.cs | 22 ++++--- .../CachelessSingleFileTests.cs | 59 +++++++++++++++++++ .../GameCult.Caching.Tests/ElementIdTests.cs | 30 ++++++++++ unity/org.gamecult.cultlib/CHANGELOG.md | 2 +- 5 files changed, 112 insertions(+), 12 deletions(-) diff --git a/src/GameCult.Caching/Contracts/cultcache-store-composition.md b/src/GameCult.Caching/Contracts/cultcache-store-composition.md index 2d9894ab..1400025f 100644 --- a/src/GameCult.Caching/Contracts/cultcache-store-composition.md +++ b/src/GameCult.Caching/Contracts/cultcache-store-composition.md @@ -131,8 +131,9 @@ which route wrote it. a cacheless file), never the reverse. What the code enforces, at run time and by `InvalidOperationException`: a thread holds at most one store's gate; while it holds a store's gate it enters no cache gate it does not already - hold, reads included; and a cache's hold (a write, a flush, an attach) entered - inside another cache's hold is refused. What it does not enforce: a cache + hold, reads included; a cacheless write made while the thread already holds + that store's gate is refused; and a cache's hold (a write, a flush, an attach) + entered inside another cache's hold is refused. What it does not enforce: a cache read made inside another cache's hold takes that cache's gate without a check, so two threads each holding one cache and reading the other can deadlock. That is open as `variants:follow_up:cross-cache-read-lock-order`; @@ -147,8 +148,10 @@ which route wrote it. `Cache` is written only by the attach, holding both gates, once and after its pull has succeeded (a failed attach never shows it, and nothing detaches), so under the cache's gate `Cache` naming that cache cannot change, and under the - store's gate it cannot change at all. The pull reads the registry and the codec - the attach lent the store, not `Cache`. A call whose guess went stale (it + store's gate it cannot change at all. The pull reads the registry the attach + lent, not `Cache`; a variant it loads carries no codec, and the cache resolves + it with its own. Until `Cache` is published the store's hold is the attach's, + so a journal the pull runs cannot write the store cacheless. A call whose guess went stale (it guessed no cache and the store then gained one) releases and retries once, so a call makes at most two passes; a cacheless write that finds a cache is refused in that same hold. A diff --git a/src/GameCult.Caching/CultCache.cs b/src/GameCult.Caching/CultCache.cs index 63bb74e6..344c94cf 100644 --- a/src/GameCult.Caching/CultCache.cs +++ b/src/GameCult.Caching/CultCache.cs @@ -2264,7 +2264,8 @@ namespace GameCult.Caching // The only writer of store.Cache, and it writes it once, last, holding the cache's gate and the store's, after the pull has // succeeded: a failed attach never publishes it, and there is no detach, so under the cache's gate Cache == this cache cannot // change, and under the store's gate Cache cannot change at all. A store call that guessed null waits on the store's gate, finds - // the cache published and retries once. + // the cache published and retries once. Until it publishes Cache the attach owns the store's hold, so a cacheless write on the + // attaching thread (a journal the pull's Admit runs) re-enters that hold and InStoreHold refuses it. Held(() => store.UnderStoreGate(() => { if (store.Cache != null) @@ -2821,13 +2822,19 @@ namespace GameCult.Caching return _gate; } - // The one lock of a store's gate. A thread holds one store's gate at a time; a store call may be entered again from inside it. - internal static T InStoreHold(object storeGate, Func body) + // The one lock of a store's gate. A thread holds one store's gate at a time; a store call may be entered again from inside it, + // except a cacheless write: it owns its store outright, so it is the outermost hold of the store's gate. + internal static T InStoreHold(object storeGate, Func body, bool cacheless = false) { if (_storeHeld != null) { if (ReferenceEquals(_storeHeld, storeGate)) + { + if (cacheless) + throw new InvalidOperationException( + "A cacheless write was made inside a call that already holds this store (an attach's pull, or a store call whose user code made it); write through the cache."); return body(); + } throw new InvalidOperationException( "A store's gate was entered inside another store's hold; user code run by a cacheless call or an attach may not enter a second store."); } @@ -3635,8 +3642,9 @@ namespace GameCult.Caching // Cache == that cache cannot change, and under the store's gate Cache cannot change at all. Attached, the call holds the cache's // gate: the store and its cache share one lock, so a load calling back into the cache never takes the gate after the store's, // and a direct call publishes what it loaded when it returns. Detached, it holds the store's gate; a cacheless call (WriteRecord, - // WriteDocument) that finds a cache there is refused in that same hold. File locks are always taken inside either gate. A call - // makes at most MaxPasses passes: Cache changes once, from null to the cache that attached the store. + // WriteDocument) that finds a cache there is refused in that same hold, and one that re-enters a hold of this store (an attach in + // its pull, or another store call's user code) is refused by InStoreHold before it reads Cache. File locks are always taken inside + // either gate. A call makes at most MaxPasses passes: Cache changes once, from null to the cache that attached the store. private T Gate(Func body, bool cacheless) { for (var pass = 0; ; pass++) @@ -3657,7 +3665,7 @@ namespace GameCult.Caching ? (true, body()) : cacheless ? throw new InvalidOperationException("The store is attached to a cache, write through the cache.") - : (false, default(T)!)); + : (false, default(T)!), cacheless); if (ran) return result; } @@ -3686,7 +3694,7 @@ namespace GameCult.Caching }, cacheless: true); // The store's own gate, taken only here. Lock order: a cache's gate, then a store's; nothing takes a store's gate and then a cache's. - internal T UnderStoreGate(Func body) => CultCache.InStoreHold(_storeGate, body); + internal T UnderStoreGate(Func body, bool cacheless = false) => CultCache.InStoreHold(_storeGate, body, cacheless); // The one writer of the lent registry: a cacheless call for the call, an attach for as long as it holds the store, and a failed // attach puts it back, so a cacheless store holds none between calls and a pull on one loads nothing. Nothing else is lent, and the diff --git a/tests/GameCult.Caching.Tests/CachelessSingleFileTests.cs b/tests/GameCult.Caching.Tests/CachelessSingleFileTests.cs index 7a0ce418..67d59a5b 100644 --- a/tests/GameCult.Caching.Tests/CachelessSingleFileTests.cs +++ b/tests/GameCult.Caching.Tests/CachelessSingleFileTests.cs @@ -1170,6 +1170,65 @@ namespace GameCult.Caching.Tests Assert.That(store.Attached, Is.Null, "the registry was taken back"); } + private const string ReentryRefusal = "A cacheless write was made inside a call that already holds this store"; + + // Until the attach publishes the cache the store's hold is the attach's, so a journal the pull's Admit runs on the attaching + // thread cannot write the store cacheless: it would write the file behind the cache and take back the registry the attach lent. + [Test] + public void AJournalOnTheAttachingThreadCannotWriteItsStoreCacheless() + { + var registry = CultDocumentRegistry.ForTypes(new[] { typeof(IdDeck) }); + new SingleFileMessagePackBackingStore(_path).WriteDocument(new CultRecordKey("a"), new IdDeck { Name = "a" }, registry: registry); + var store = new RoleStore(_path); + using var cache = new CultCache(registry, Codec(registry)); + var journalRuns = 0; + Exception? journalError = null; + using var journal = cache.AddJournal(_ => + { + journalRuns++; + try { store.WriteDocument(new CultRecordKey("b"), new IdDeck { Name = "b" }, registry: registry); } + catch (Exception error) { journalError = error; } + }); + + cache.AddBackingStore(store); + + Assert.That(journalRuns, Is.EqualTo(1), "the pull's admission ran the journal"); + Assert.That(journalError, Is.InstanceOf()); + Assert.That(journalError!.Message, Does.StartWith(ReentryRefusal)); + Assert.That(store.Attached, Is.SameAs(registry), "the refused call did not take back the attach's registry"); + Assert.That(store.ReadStoreSnapshot()!.Records.Select(record => record.Key), Is.EquivalentTo(new[] { "a" })); + + Assert.DoesNotThrow(() => cache.AddAsync(new IdDeck { Name = "later" }).GetAwaiter().GetResult()); + Assert.DoesNotThrow(() => cache.FlushAllBackingStores()); + Assert.DoesNotThrow(() => store.PullAll()); + var keys = store.ReadStoreSnapshot()!.Records.Select(record => record.Key).ToArray(); + Assert.That(keys, Has.Length.EqualTo(2)); + Assert.That(keys, Does.Contain("a")); + Assert.That(keys, Does.Not.Contain("b")); + } + + // A cacheless write owns its store outright: user code another cacheless write of the same store runs may not make a second. + [Test] + public void ACachelessWriteInsideACachelessWriteOfTheSameStoreIsRefused() + { + var registry = CultDocumentRegistry.ForTypes(new[] { typeof(TapDoc) }); + var store = new RoleStore(_path); + Exception? inner = null; + TapDoc.Hook = () => + { + try { store.WriteDocument(new CultRecordKey("inner"), new TapDoc { Name = "inner" }, registry: registry); } + catch (Exception error) { inner = error; } + }; + + var outer = Assert.Catch(() => store.WriteDocument(new CultRecordKey("outer"), new TapDoc { Name = "outer" }, registry: registry)); + + Assert.That(inner, Is.InstanceOf()); + Assert.That(inner!.Message, Does.StartWith(ReentryRefusal)); + Assert.That(Refused(outer), Is.True, "the outer write failed with the getter's exception"); + Assert.That(File.Exists(_path), Is.False); + Assert.That(store.Attached, Is.Null); + } + // The bound on a call's passes is structural: Cache goes from null to a cache once. This test writes it any other way (the // cache, then null again, under the cache's gate and the store's, in the order they are taken); the reader, which guessed null and // then the cache, is stale twice and throws instead of looping. diff --git a/tests/GameCult.Caching.Tests/ElementIdTests.cs b/tests/GameCult.Caching.Tests/ElementIdTests.cs index 3d709b74..41af34fb 100644 --- a/tests/GameCult.Caching.Tests/ElementIdTests.cs +++ b/tests/GameCult.Caching.Tests/ElementIdTests.cs @@ -918,6 +918,36 @@ namespace GameCult.Caching.Tests Assert.That(HeaderOf(path), Is.EqualTo("cultcache.store.v3"), "read without a codec, the variant it writes cannot be seen, so it is marked"); } + // Through a real attach: the second cache reads the variant from the file, and the flush it then makes writes the header its + // resolved values hold, never the one a codecless copy would claim (a codecless variant counts as holding ids). + [TestCase("name", "cultcache.store.v2")] + [TestCase("reels", "cultcache.store.v3")] + public void AnAttachedVariantStoreFlushesTheHeaderItsValuesHold(string overridden, string header) + { + var path = PathOf("attached-variant-" + overridden + ".cc"); + using (var cache = Open(path)) + { + cache.Commit(batch => + { + batch.Upsert(typeof(IdDeck), EmptyDeck("base"), new CultRecordKey("base")); + batch.UpsertVariant(new CultRecordKey("v"), new CultRecordKey("base"), new[] + { + overridden == "name" + ? cache.Override(nameof(IdDeck.Name), "v") + : cache.Override(nameof(IdDeck.Reels), new List { new() { Label = "r", Id = HexA } }), + }); + }); + } + + using (var second = Open(path)) + { + second.Commit(batch => batch.Upsert(typeof(IdDeck), EmptyDeck("other"), new CultRecordKey("other"))); + second.FlushAllBackingStores(); + } + + Assert.That(HeaderOf(path), Is.EqualTo(header)); + } + [Test] public void ADirectoryStoreIsMarkedByTheIdsItWritesAndAnEmptyDeckDoesNotMarkIt() { diff --git a/unity/org.gamecult.cultlib/CHANGELOG.md b/unity/org.gamecult.cultlib/CHANGELOG.md index 0821d622..88160f4f 100644 --- a/unity/org.gamecult.cultlib/CHANGELOG.md +++ b/unity/org.gamecult.cultlib/CHANGELOG.md @@ -23,7 +23,7 @@ Releases as 3.0.0 (major). ### Added -- A cacheless `WriteRecord` or `WriteDocument`, and an attach, hold the store's gate while they run, and user code they run (a document's property getters, a constructor while a copied record is read) now throws `InvalidOperationException` if it enters a cache it does not hold or a second store; it used to wait forever against an attach of the same store. A store call that queued on an attach that then failed runs under the gate that owns the store, not beside a cacheless call or a second cache's attach. An attach that fails takes its registry back, so a `PullAll` on the store afterwards is refused as unattached instead of loading records the next cache never sees. +- A cacheless `WriteRecord` or `WriteDocument`, and an attach, hold the store's gate while they run, and user code they run (a document's property getters, a constructor while a copied record is read) now throws `InvalidOperationException` if it enters a cache it does not hold or a second store; it used to wait forever against an attach of the same store. A store call that queued on an attach that then failed runs under the gate that owns the store, not beside a cacheless call or a second cache's attach. An attach that fails takes its registry back, so a `PullAll` on the store afterwards is refused as unattached instead of loading records the next cache never sees. A cacheless `WriteRecord` or `WriteDocument` made while the same thread already holds that store (from a journal an attach's pull runs, or from user code another cacheless write of the store runs) throws `InvalidOperationException`; it used to write the file behind the attaching cache and take that cache's registry, so the cache could never flush or pull the store again. - `SingleFileBackingStore.ReadStoreSnapshot()` reads a store file once, under the store's in-process gate, with the reader a cache's store opens with, and returns the durable snapshot or null when nothing is at the path. `SingleFileBackingStore.WriteRecord(record, catalogEntry, holdsIds, registry)` writes one record without a cache: under the store's lock every other record and catalog entry is copied as stored, the file is replaced atomically, and the header is decided as a flush decides it. It refuses on a store attached to a cache. - `SingleFileBackingStore.WriteDocument(key, document, storedAt, registry)` writes one typed document without a cache, as a cache write does: it plans element ids, encodes the document as `T` (the type its schema id and ids are taken from, also for a derived instance) and marks the store when it holds an id; a failed write gives the minted ids back. It refuses on a store attached to a cache. The `registry` of `WriteRecord` and `WriteDocument` serves that call only; a store keeps none. - `CultCodec.DecodeDocument(key, payload)` decodes a stored payload as a cache load does (`CultCacheMessagePack.CreateCodec(registry)` makes the codec). It reads no file and holds no state. `CultElementIds` stays internal.