Serialize cache and store mutations per persona

[matt-edmondson]

Fixes #159

The defect

AddOrReplace persists then caches; Remove did the opposite, cache then store; and nothing synchronized the two. A concurrent AddOrReplace for the same persona could land between Remove's two halves, persist and cache a new credential, and have Remove then delete the entry it had just written. End state: readable from _credentials, absent from the store, with Remove having returned true.

Measured at main (410f9d1), .NET SDK 10.0.401, Linux — a store that parks the first thread to enter its Remove while a second thread completes AddOrReplace:

readable=True  store=False

That is a Remove used as a logout or revoke primitive reporting success while the credential stays usable until the process exits.

The change

Mutations of one persona's cache entry and store entry now happen together under a lock:

private Lock LockFor(PersonaGUID persona) =>
	_personaLocks[(uint)persona.GetHashCode() & (PersonaLockStripes - 1)];

Striped (64), not one lock per persona. A ConcurrentDictionary<PersonaGUID, Lock> would grow for the lifetime of the process with nothing to key its eviction on — this library's personas are long-lived and there is no signal for when one is done with. Two personas sharing a stripe serialize when they need not, which costs a little contention and never correctness.

Striping on GetHashCode() is what makes it sound. _credentials keys on the default EqualityComparer<PersonaGUID>, so equal personas are one key and — by the hash/equals contract — one stripe. Two distinct PersonaGUID instances carrying the same value therefore serialize against each other; a ConditionalWeakTable or any instance-keyed scheme would not, and would have left the race open for exactly the callers who build the GUID rather than holding one.

Remove also clears the store first, mirroring AddOrReplace's persist-then-cache order rather than inverting it. Under the lock the ordering no longer decides the race, but it does decide what a throwing store leaves behind: store-first means a failed Store.Remove leaves both halves intact, matching the invariant AddOrReplace's doc comment already states. The issue offers this as an alternative to locking; it is neither sufficient alone (it narrows the window rather than closing it) nor redundant once the lock is there.

TryGet takes the lock too

This is the one part of the change the issue does not ask for, so the reasoning in full.

TryGet looks like a read, but on a cache miss it writes — Store.TryLoad then GetOrAdd. That is the same pair of mutations, and it can straddle a removal the same way:

1. TryGet misses the cache and loads the credential from the store.
2. Remove runs to completion.
3. TryGet caches what it loaded.

Same end state, same severity, reached without AddOrReplace at all. Guarding only the two methods named in the issue would have left the invariant it states — present in both or absent from both — still false. A cache hit, which is the common path, is answered before the lock is taken, so the fast path is unchanged.

The cost is real and worth naming: the lock is held across a native store call, so a Remove racing an AddOrReplace on one persona now serializes at the speed of libsecret or Keychain rather than returning immediately with the wrong answer. README.md already tells callers to treat these as I/O; this adds the sentence saying why that matters more now.

Tests

PersonaMutationRaceTests.cs. RemoveBlockingCredentialStore parks the first thread to enter Remove and lets the test drive a second thread through AddOrReplace while it is held there, which is what makes the interleaving deterministic rather than hoped for.

Its wait is bounded, not indefinite, and that is load-bearing: once mutations serialize, the second thread cannot reach Release — it is blocked behind the removal — so an unbounded wait would deadlock the fixed code instead of letting it finish. The fixed code takes the 1s timeout and then completes; the unfixed code is released in milliseconds. ReleaseTimedOut is reported in the failure message rather than asserted on, so the test pins the outcome and not the implementation.

case	covers
RemoveInterleavedWithAddOrReplace… / the same PersonaGUID instance	the reported defect, deterministically
RemoveInterleavedWithAddOrReplace… / a distinct PersonaGUID of equal value	an implementation that serializes on the instance rather than the value
ConcurrentAddOrReplaceAndRemoveOnTheSamePersonaAgreeOnTheFinalState	the issue's literal ask — 500 unsynchronized attempts on one persona

The second row is not a duplicate of the first. It is the case a plausible wrong fix passes, and it also asserts the hash/equals property the striping depends on, so the scheme breaking is a test failure rather than a silent return of the race.

The third is the issue's requested stress test and it does catch the defect — the window turned out to be wide enough to hit within 500 attempts on every run — but the deterministic cases are the ones that catch it every time, and they are what I would rely on.

Proved failing without the fix. Reverting only CredentialCache/CredentialCache.cs and keeping the tests, three consecutive runs:

failed the same PersonaGUID instance (8ms)
failed a distinct PersonaGUID of equal value (1ms)
failed ConcurrentAddOrReplaceAndRemoveOnTheSamePersonaAgreeOnTheFinalState (12ms)

  total: 40   failed: 3   succeeded: 32   skipped: 5

All three fail on the reported symptom — readable=True but store=False — not on a message that merely changed shape. The 32 pre-existing tests are unaffected in both directions. CredentialCacheIsThreadSafeUnderConcurrentAccess still passes; as the issue notes, it never interleaved the two methods on one key.

Verification

• dotnet build CredentialCache.sln -c Release — succeeded, 0 warnings, 0 errors, across net9.0 and net10.0
• dotnet test CredentialCache.sln -c Release — 40 total, 35 passed, 0 failed, 5 skipped, three consecutive runs
• Same suite against reverted CredentialCache.cs — 3 of 40 failed, three consecutive runs, as above

The 5 skips are pre-existing and environmental: 4 NativeCredentialStoreTests cases self-skip because no Secret Service is available in this container, and 1 is Windows-only.

Worth knowing

Building this repository rewrites .gitignore in the working tree from the SDK's shared template — it adds Unity .meta and Godot rules unrelated to anything here. It is not part of this diff; I reverted it. Anyone working this repository will see it appear after a build and should not commit it by accident.

Not in this change

Dispose still clears _credentials without taking the persona locks. It is documented as releasing in-memory state only and touches no store, so it cannot produce the disagreement this fixes; making disposal wait on in-flight mutations is a lifetime question rather than this race.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LLBd6CLjwb1r6wuDqDzW4J

---

Generated by Claude Code

[sonarqubecloud]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
91.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud