diff --git a/CredentialCache.Test/PersonaMutationRaceTests.cs b/CredentialCache.Test/PersonaMutationRaceTests.cs new file mode 100644 index 0000000..8641939 --- /dev/null +++ b/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; + +/// +/// An that parks the first thread to enter +/// so a test can drive a second thread through +/// while it is held there. +/// +/// +/// The wait is bounded rather than indefinite on purpose. Once mutations of one +/// persona are serialized, the second thread cannot reach — +/// it is blocked behind the removal — so an unbounded wait would deadlock the +/// fixed code instead of letting it finish. +/// +internal sealed class RemoveBlockingCredentialStore : ICredentialStore, IDisposable +{ + private readonly InMemoryCredentialStore _inner = new(); + private readonly ManualResetEventSlim _release = new(false); + private int _removeCalls; + + /// + public string Name => "RemoveBlocking"; + + /// + /// Gets an event signalled once a thread has entered . + /// + public ManualResetEventSlim RemoveEntered { get; } = new(false); + + /// + /// Gets how long the parked waits before giving up. + /// + public TimeSpan ReleaseTimeout { get; init; } = TimeSpan.FromSeconds(1); + + /// + /// Gets a value indicating whether the parked gave up waiting + /// rather than being released. True means the interleaving the test tried to force + /// could not happen. + /// + public bool ReleaseTimedOut { get; private set; } + + /// + /// Lets the parked continue. + /// + public void Release() => _release.Set(); + + /// + /// Reports whether the backing store currently holds , + /// without going through the cache. + /// + public bool Holds(PersonaGUID persona) => _inner.TryLoad(persona, out _); + + /// + public bool TryLoad(PersonaGUID persona, out Credential? credential) => + _inner.TryLoad(persona, out credential); + + /// + public void Save(PersonaGUID persona, Credential credential) => _inner.Save(persona, credential); + + /// + public bool Remove(PersonaGUID persona) + { + if (Interlocked.Increment(ref _removeCalls) == 1) + { + RemoveEntered.Set(); + ReleaseTimedOut = !_release.Wait(ReleaseTimeout); + } + + return _inner.Remove(persona); + } + + /// + 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.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."); + } + } +} diff --git a/CredentialCache/CredentialCache.cs b/CredentialCache/CredentialCache.cs index 4ff1ddd..d8160bb 100644 --- a/CredentialCache/CredentialCache.cs +++ b/CredentialCache/CredentialCache.cs @@ -27,8 +27,15 @@ public sealed class CredentialCache : IDisposable private static CredentialCache? _instance; private static ICredentialStore? _configuredStore; + /// + /// The number of locks mutations of one persona are striped across. A power of two, + /// so can mask rather than divide. + /// + private const int PersonaLockStripes = 64; + private readonly ConcurrentDictionary _credentials = new(); private readonly ConcurrentDictionary _factories = new(); + private readonly Lock[] _personaLocks = CreatePersonaLocks(); private bool _disposed; /// @@ -114,10 +121,39 @@ public static void ResetSingletonForTesting() public static PersonaGUID CreatePersonaGUID() => SemanticString.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; + } + + /// + /// Returns the lock guarding mutations of 's cache entry + /// and store entry as one unit. + /// + /// + /// 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. + /// + private Lock LockFor(PersonaGUID persona) => + _personaLocks[(uint)persona.GetHashCode() & (PersonaLockStripes - 1)]; + /// /// Attempts to retrieve the credential associated with , /// loading it from the backing store if it has not yet been cached in memory. /// + /// + /// 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 + /// is retiring: loading a credential and then caching it either + /// side of a removal would put back exactly what the removal took out. + /// public bool TryGet(PersonaGUID persona, out Credential? credential) { ArgumentNullException.ThrowIfNull(persona); @@ -128,10 +164,18 @@ public bool TryGet(PersonaGUID persona, out Credential? credential) 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,7 +188,9 @@ public bool TryGet(PersonaGUID persona, out Credential? credential) /// /// 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 of the same persona cannot land + /// between them. /// public void AddOrReplace(PersonaGUID persona, Credential credential) { @@ -152,22 +198,36 @@ public void AddOrReplace(PersonaGUID persona, Credential credential) ArgumentNullException.ThrowIfNull(credential); ThrowIfDisposed(); - Store.Save(persona, credential); - _credentials[persona] = credential; + lock (LockFor(persona)) + { + Store.Save(persona, credential); + _credentials[persona] = credential; + } } /// /// Removes the credential associated with from both /// the in-memory cache and the backing store. /// + /// + /// Both removals happen under the persona's lock, and the store is cleared first, so + /// this mirrors 's store-then-cache order rather than + /// inverting it. Without the lock a concurrent 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. + /// 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; + } } /// diff --git a/README.md b/README.md index 98aadc7..6f68f91 100644 --- a/README.md +++ b/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