GameCult/CultLib
Watch
9
0
Fork
You've already forked CultLib
 0
Code Issues Pull requests 1 Projects Releases Packages Wiki Activity Actions 3

Retry an unlocked directory load only when the manifest moved

Browse source 
A missing or hash-failing page under an unchanged manifest is corruption,
not a writer mid-commit, but every InvalidDataException was retried five
times and reported as "did not settle" with the real cause buried. The
retry now also requires the manifest bytes to have changed; otherwise the
corruption error is thrown on the first attempt. A vanished page directory
is a race under the same condition.

The reload test's writer now commits under every unleased read, so it only
settles if the retry takes the lease that commit created.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Meta Vens 2026-09-14 14:33:43 +02:00
commit 838bbfffec
2 changed files with 42 additions and 6 deletions
Download patch file Download diff file Expand all files Collapse all files

10
src/GameCult.Caching.MessagePack/DirectoryMessagePackBackingStore.cs
View file

@ -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);

38
tests/GameCult.Caching.Tests/BackingStoreTests.cs
View file

@ -645,9 +645,9 @@ namespace GameCult.Caching.Tests
File.Delete(Path.Combine(recordsPath, ".commit.lock"));
var committed = new System.Collections.Generic.List<CultRecordKey>();
// 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<NamedTestEntry>(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()
{