Serialize cache and store mutations per persona

CredentialCache.Test/PersonaMutationRaceTests.cs

@@ -0,0 +1,176 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.CredentialCache.Test;
+
+using ktsu.CredentialCache.Storage;
+using ktsu.Semantics.Strings;
+
+/// <summary>
+/// An <see cref="ICredentialStore"/> that parks the first thread to enter
+/// <see cref="Remove"/> so a test can drive a second thread through
+/// <see cref="CredentialCache.AddOrReplace"/> while it is held there.
+/// </summary>
+/// <remarks>
+/// The wait is bounded rather than indefinite on purpose. Once mutations of one
+/// persona are serialized, the second thread cannot reach <see cref="Release"/> —
+/// it is blocked behind the removal — so an unbounded wait would deadlock the
+/// fixed code instead of letting it finish.
+/// </remarks>
+internal sealed class RemoveBlockingCredentialStore : ICredentialStore, IDisposable
+{
+	private readonly InMemoryCredentialStore _inner = new();
+	private readonly ManualResetEventSlim _release = new(false);
+	private int _removeCalls;
+
+	/// <inheritdoc/>
+	public string Name => "RemoveBlocking";
+
+	/// <summary>
+	/// Gets an event signalled once a thread has entered <see cref="Remove"/>.
+	/// </summary>
+	public ManualResetEventSlim RemoveEntered { get; } = new(false);
+
+	/// <summary>
+	/// Gets how long the parked <see cref="Remove"/> waits before giving up.
+	/// </summary>
+	public TimeSpan ReleaseTimeout { get; init; } = TimeSpan.FromSeconds(1);
+
+	/// <summary>
+	/// Gets a value indicating whether the parked <see cref="Remove"/> gave up waiting
+	/// rather than being released. True means the interleaving the test tried to force
+	/// could not happen.
+	/// </summary>
+	public bool ReleaseTimedOut { get; private set; }
+
+	/// <summary>
+	/// Lets the parked <see cref="Remove"/> continue.
+	/// </summary>
+	public void Release() => _release.Set();
+
+	/// <summary>
+	/// Reports whether the backing store currently holds <paramref name="persona"/>,
+	/// without going through the cache.
+	/// </summary>
+	public bool Holds(PersonaGUID persona) => _inner.TryLoad(persona, out _);
+
+	/// <inheritdoc/>
+	public bool TryLoad(PersonaGUID persona, out Credential? credential) =>
+		_inner.TryLoad(persona, out credential);
+
+	/// <inheritdoc/>
+	public void Save(PersonaGUID persona, Credential credential) => _inner.Save(persona, credential);
+
+	/// <inheritdoc/>
+	public bool Remove(PersonaGUID persona)
+	{
+		if (Interlocked.Increment(ref _removeCalls) == 1)
+		{
+			RemoveEntered.Set();
+			ReleaseTimedOut = !_release.Wait(ReleaseTimeout);
+		}
+
+		return _inner.Remove(persona);
+	}
+
+	/// <inheritdoc/>
+	public void Dispose()
+	{
+		RemoveEntered.Dispose();
+		_release.Dispose();
+	}
+}
+
+[TestClass]
+public class PersonaMutationRaceTests
+{
+	private static readonly TimeSpan HandshakeTimeout = TimeSpan.FromSeconds(10);
+	private static readonly TimeSpan CompletionTimeout = TimeSpan.FromSeconds(30);
+
+	[TestMethod]
+	// The second row is not a duplicate: two PersonaGUID instances carrying the same value
+	// are one key to the cache, so they must also be one unit of mutual exclusion. It fails
+	// an implementation that serializes on the instance rather than on the value.
+	[DataRow(false, DisplayName = "the same PersonaGUID instance")]
+	[DataRow(true, DisplayName = "a distinct PersonaGUID of equal value")]
+	public void RemoveInterleavedWithAddOrReplaceDoesNotLeaveACredentialLiveOnlyInMemory(bool useEqualInstance)
+	{
+		using RemoveBlockingCredentialStore store = new();
+		using CredentialCache cache = new(store);
+		PersonaGUID persona = CredentialCache.CreatePersonaGUID();
+		PersonaGUID adderPersona = useEqualInstance
+			? SemanticString<PersonaGUID>.Create(persona.ToString())
+			: persona;
+
+		Assert.AreEqual(persona, adderPersona, "The two personas must be one key to the cache.");
+		Assert.AreEqual(
+			persona.GetHashCode(),
+			adderPersona.GetHashCode(),
+			"Equal personas must hash equally, or they cannot share a lock.");
+
+		cache.AddOrReplace(persona, new CredentialWithNothing());
+
+		Task remover = Task.Run(() => cache.Remove(persona));
+		Task adder = Task.Run(() =>
+		{
+			Assert.IsTrue(
+				store.RemoveEntered.Wait(HandshakeTimeout),
+				"No thread entered the store's Remove, so the interleaving was never set up.");
+			cache.AddOrReplace(adderPersona, new CredentialWithNothing());
+			store.Release();
+		});
+
+		Assert.IsTrue(
+			Task.WaitAll([remover, adder], CompletionTimeout),
+			"The removal and the replacement did not both finish.");
+
+		bool heldInStore = store.Holds(persona);
+		bool readable = cache.TryGet(persona, out _);
+
+		Assert.AreEqual(
+			heldInStore,
+			readable,
+			$"A credential must be readable if and only if the store holds it, but readable={readable} "
+			+ $"and store={heldInStore}. A credential readable from a store that no longer holds it is a "
+			+ $"removal that left it live in memory. (The removal "
+			+ $"{(store.ReleaseTimedOut ? "was not" : "was")} interleaved with the replacement.)");
+	}
+
+	[TestMethod]
+	public void ConcurrentAddOrReplaceAndRemoveOnTheSamePersonaAgreeOnTheFinalState()
+	{
+		const int attempts = 500;
+
+		for (int attempt = 0; attempt < attempts; attempt++)
+		{
+			InMemoryCredentialStore store = new();
+			using CredentialCache cache = new(store);
+			PersonaGUID persona = CredentialCache.CreatePersonaGUID();
+			cache.AddOrReplace(persona, new CredentialWithNothing());
+
+			using Barrier gate = new(2);
+			Task remover = Task.Run(() =>
+			{
+				gate.SignalAndWait();
+				cache.Remove(persona);
+			});
+			Task adder = Task.Run(() =>
+			{
+				gate.SignalAndWait();
+				cache.AddOrReplace(persona, new CredentialWithNothing());
+			});
+
+			Assert.IsTrue(
+				Task.WaitAll([remover, adder], CompletionTimeout),
+				$"Attempt {attempt}: the removal and the replacement did not both finish.");
+
+			bool heldInStore = store.TryLoad(persona, out _);
+			bool readable = cache.TryGet(persona, out _);
+
+			Assert.AreEqual(
+				heldInStore,
+				readable,
+				$"Attempt {attempt}: readable={readable} but store={heldInStore}. Whichever of the two "
+				+ "operations ran second, the cache and the store must end up agreeing.");
+		}
+	}
+}

CredentialCache/CredentialCache.cs

@@ -9,7 +9,7 @@
 /// <summary>
 /// Represents a globally unique identifier for a persona.
 /// </summary>
 public sealed record class PersonaGUID : SemanticString<PersonaGUID> { }
 
 /// <summary>
 /// Caches <see cref="Credential"/> instances in memory and persists each one through
@@ -27,8 +27,15 @@
 	private static CredentialCache? _instance;
 	private static ICredentialStore? _configuredStore;
 
+	/// <summary>
+	/// The number of locks mutations of one persona are striped across. A power of two,
+	/// so <see cref="LockFor"/> can mask rather than divide.
+	/// </summary>
+	private const int PersonaLockStripes = 64;
+
 	private readonly ConcurrentDictionary<PersonaGUID, Credential> _credentials = new();
 	private readonly ConcurrentDictionary<Type, ICredentialFactory> _factories = new();
+	private readonly Lock[] _personaLocks = CreatePersonaLocks();
 	private bool _disposed;
 
 	/// <summary>
@@ -114,10 +121,39 @@
 	public static PersonaGUID CreatePersonaGUID() =>
 		SemanticString<PersonaGUID>.Create(Guid.NewGuid().ToString());
 
+	private static Lock[] CreatePersonaLocks()
+	{
+		Lock[] locks = new Lock[PersonaLockStripes];
+		for (int i = 0; i < locks.Length; i++)
+		{
+			locks[i] = new Lock();
+		}
+		return locks;
+	}
+
+	/// <summary>
+	/// Returns the lock guarding mutations of <paramref name="persona"/>'s cache entry
+	/// and store entry as one unit.
+	/// </summary>
+	/// <remarks>
+	/// Striped rather than one lock per persona: a per-persona dictionary of locks would
+	/// grow for the lifetime of the process with nothing to key its eviction on. Two
+	/// personas landing on the same stripe serialize when they need not, which costs
+	/// contention and never correctness.
+	/// </remarks>
+	private Lock LockFor(PersonaGUID persona) =>
+		_personaLocks[(uint)persona.GetHashCode() & (PersonaLockStripes - 1)];
+
 	/// <summary>
 	/// Attempts to retrieve the credential associated with <paramref name="persona"/>,
 	/// loading it from the backing store if it has not yet been cached in memory.
 	/// </summary>
+	/// <remarks>
+	/// A cache hit is answered without taking a lock. Populating the cache from the store
+	/// is not, because it is a mutation of the same pair of state a concurrent
+	/// <see cref="Remove"/> is retiring: loading a credential and then caching it either
+	/// side of a removal would put back exactly what the removal took out.
+	/// </remarks>
 	public bool TryGet(PersonaGUID persona, out Credential? credential)
 	{
 		ArgumentNullException.ThrowIfNull(persona);
@@ -128,10 +164,18 @@
 			return true;
 		}
 
-		if (Store.TryLoad(persona, out Credential? loaded) && loaded is not null)
+		lock (LockFor(persona))
 		{
-			credential = _credentials.GetOrAdd(persona, loaded);
-			return true;
+			if (_credentials.TryGetValue(persona, out credential))
+			{
+				return true;
+			}
+
+			if (Store.TryLoad(persona, out Credential? loaded) && loaded is not null)
+			{
+				credential = _credentials.GetOrAdd(persona, loaded);
+				return true;
+			}
 		}
 
 		credential = null;
@@ -144,30 +188,46 @@
 	/// <remarks>
 	/// The credential is persisted before the in-memory cache is updated, so a store
 	/// that throws leaves the cache untouched rather than serving a credential that
-	/// was never written to the backing store.
+	/// was never written to the backing store. Both writes happen under the persona's
+	/// lock, so a concurrent <see cref="Remove"/> of the same persona cannot land
+	/// between them.
 	/// </remarks>
 	public void AddOrReplace(PersonaGUID persona, Credential credential)
 	{
 		ArgumentNullException.ThrowIfNull(persona);
 		ArgumentNullException.ThrowIfNull(credential);
 		ThrowIfDisposed();
 
-		Store.Save(persona, credential);
-		_credentials[persona] = credential;
+		lock (LockFor(persona))
+		{
+			Store.Save(persona, credential);
+			_credentials[persona] = credential;
+		}
 	}
 
 	/// <summary>
 	/// Removes the credential associated with <paramref name="persona"/> from both
 	/// the in-memory cache and the backing store.
 	/// </summary>
+	/// <remarks>
+	/// Both removals happen under the persona's lock, and the store is cleared first, so
+	/// this mirrors <see cref="AddOrReplace"/>'s store-then-cache order rather than
+	/// inverting it. Without the lock a concurrent <see cref="AddOrReplace"/> could
+	/// persist and cache a new credential between the two halves of this method, which
+	/// then deleted the newly persisted entry and left the new credential readable from
+	/// memory alone — a removal reporting success while the credential stayed usable.
+	/// </remarks>
 	public bool Remove(PersonaGUID persona)
 	{
 		ArgumentNullException.ThrowIfNull(persona);
 		ThrowIfDisposed();
 
-		bool removedInMemory = _credentials.TryRemove(persona, out _);
-		bool removedFromStore = Store.Remove(persona);
-		return removedInMemory || removedFromStore;
+		lock (LockFor(persona))
+		{
+			bool removedFromStore = Store.Remove(persona);
+			bool removedInMemory = _credentials.TryRemove(persona, out _);
+			return removedInMemory || removedFromStore;
+		}
 	}
 
 	/// <summary>

README.md

@@ -152,6 +152,7 @@ else
 - **macOS** uses the user's default login keychain. The first access from an application prompts the user for permission, as with any keychain client.
 - **Linux** requires `libsecret-1` plus an active Secret Service. Headless CI agents typically have neither — use `InMemoryCredentialStore` there, or set up `dbus-run-session` + `gnome-keyring-daemon` as the `cross-platform.yml` workflow does.
 - All native calls happen on the thread the API is invoked from. The library's in-memory cache is thread-safe (`ConcurrentDictionary`); the native APIs themselves are documented as thread-safe by their respective platform owners, but blocking calls (especially libsecret) are not cheap — treat `Save` / `Remove` as I/O, not as cheap accessors.
+- `AddOrReplace`, `Remove`, and the store-loading half of `TryGet` each update the cache *and* the store, so they hold a lock on the persona for the duration rather than relying on the dictionary alone. Concurrent calls for the same persona therefore run one at a time and the two always end up agreeing; concurrent calls for different personas are unaffected. Because that lock is held across a store call, a `Remove` and an `AddOrReplace` racing on one persona serialize at the speed of the native store — another reason to treat them as I/O.
 
 ## API summary