From bbfc5948d1f0c004bc785383bb8f619fec5b9b24 Mon Sep 17 00:00:00 2001
From: Matthew Edmondson <matthew.edmondson@gmail.com>
Date: Mon, 14 Sep 2026 20:23:16 +0000
Subject: [PATCH] fix: persist a credential before caching it in AddOrReplace
 [patch]

AddOrReplace wrote the credential into the in-memory dictionary before
calling Store.Save. When a native store rejected the write (an oversized
blob on Windows Credential Manager, a transient keychain or libsecret
failure), the caller saw the exception but a later TryGet still returned
the credential from memory, so the process behaved as though it had been
durably stored until a restart or a second cache instance revealed it was
never written.

Save first, then update the cache, so a failed save leaves the cache
holding exactly what the store holds.

Fixes #145

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016dQa3DK7HjcyQkRaYBxumh
---
 CredentialCache.Test/CredentialCacheTests.cs | 71 ++++++++++++++++++++
 CredentialCache/CredentialCache.cs           |  7 +-
 2 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/CredentialCache.Test/CredentialCacheTests.cs b/CredentialCache.Test/CredentialCacheTests.cs
index 8c2aced..1a85fbc 100644
--- a/CredentialCache.Test/CredentialCacheTests.cs
+++ b/CredentialCache.Test/CredentialCacheTests.cs
@@ -60,6 +60,51 @@ public void AddOrReplacePersistsCredentialToBackingStore()
 		Assert.AreEqual("hunter2", typed.Password.ToString());
 	}
 
+	[TestMethod]
+	public void AddOrReplaceDoesNotCacheCredentialWhenStoreSaveThrows()
+	{
+		ThrowingCredentialStore store = new() { ThrowOnSave = true };
+		using CredentialCache cache = new(store);
+		PersonaGUID guid = CredentialCache.CreatePersonaGUID();
+
+		Assert.Throws<CredentialStoreException>(() => cache.AddOrReplace(guid, new CredentialWithNothing()));
+
+		Assert.IsFalse(
+			cache.TryGet(guid, out Credential? credential),
+			"A credential the store refused to persist must not be served from the in-memory cache");
+		Assert.IsNull(credential);
+	}
+
+	[TestMethod]
+	public void AddOrReplaceKeepsThePersistedCredentialWhenReplacementFailsToSave()
+	{
+		ThrowingCredentialStore store = new();
+		using CredentialCache cache = new(store);
+		PersonaGUID guid = CredentialCache.CreatePersonaGUID();
+		CredentialWithUsernamePassword persisted = new()
+		{
+			Username = SemanticString<CredentialUsername>.Create("alice"),
+			Password = SemanticString<CredentialPassword>.Create("hunter2"),
+		};
+		CredentialWithUsernamePassword replacement = new()
+		{
+			Username = SemanticString<CredentialUsername>.Create("alice"),
+			Password = SemanticString<CredentialPassword>.Create("oversized"),
+		};
+
+		cache.AddOrReplace(guid, persisted);
+		store.ThrowOnSave = true;
+		Assert.Throws<CredentialStoreException>(() => cache.AddOrReplace(guid, replacement));
+
+		Assert.IsTrue(cache.TryGet(guid, out Credential? credential));
+		CredentialWithUsernamePassword? typed = credential as CredentialWithUsernamePassword;
+		Assert.IsNotNull(typed);
+		Assert.AreEqual(
+			"hunter2",
+			typed!.Password.ToString(),
+			"The cache must keep the credential the store actually holds, not the one whose save failed");
+	}
+
 	[TestMethod]
 	public void RemoveDeletesCredentialFromBothLayers()
 	{
@@ -268,6 +313,32 @@ public class CredentialWithNothingFactory : ICredentialFactory<CredentialWithNot
 	public CredentialWithNothing Create() => new();
 }
 
+/// <summary>
+/// An in-memory store whose <see cref="Save"/> can be made to fail on demand, standing in
+/// for a native store that rejects a write (oversized blob, transient keychain failure).
+/// </summary>
+public sealed class ThrowingCredentialStore : ICredentialStore
+{
+	private readonly InMemoryCredentialStore _inner = new();
+
+	public bool ThrowOnSave { get; set; }
+
+	public string Name => "Throwing";
+
+	public bool TryLoad(PersonaGUID persona, out Credential? credential) => _inner.TryLoad(persona, out credential);
+
+	public void Save(PersonaGUID persona, Credential credential)
+	{
+		if (ThrowOnSave)
+		{
+			throw new CredentialStoreException("Simulated store failure.");
+		}
+		_inner.Save(persona, credential);
+	}
+
+	public bool Remove(PersonaGUID persona) => _inner.Remove(persona);
+}
+
 public class AnotherCredential : Credential { }
 
 public class AnotherCredentialFactory : ICredentialFactory<AnotherCredential>
diff --git a/CredentialCache/CredentialCache.cs b/CredentialCache/CredentialCache.cs
index 60024dc..4ff1ddd 100644
--- a/CredentialCache/CredentialCache.cs
+++ b/CredentialCache/CredentialCache.cs
@@ -141,14 +141,19 @@ public bool TryGet(PersonaGUID persona, out Credential? credential)
 	/// <summary>
 	/// Adds or replaces the credential for <paramref name="persona"/> and persists it.
 	/// </summary>
+	/// <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.
+	/// </remarks>
 	public void AddOrReplace(PersonaGUID persona, Credential credential)
 	{
 		ArgumentNullException.ThrowIfNull(persona);
 		ArgumentNullException.ThrowIfNull(credential);
 		ThrowIfDisposed();
 
-		_credentials[persona] = credential;
 		Store.Save(persona, credential);
+		_credentials[persona] = credential;
 	}
 
 	/// <summary>