From 2711ef51c20a66604954ff42460a61007438ad19 Mon Sep 17 00:00:00 2001
From: Stuart Meeks <stuart.meeks@stuartmeeks.net>
Date: Fri, 21 Aug 2026 10:52:15 +0000
Subject: [PATCH] fix: close the add-then-select visibility race in the
 Keychain/libsecret backends
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

AddCredentialAsync returned as soon as the OS store accepted the item, but a
just-added item isn't always immediately visible to a follow-up query on the
macOS Keychain (and, less often, the Linux Secret Service) under concurrent
access. A caller doing add-then-select/delete — which AddCredentialCommand does
in production when it offers to activate the new credential — could therefore
race the item's own appearance and have the follow-up lookup miss it, returning
false.

Fix it at the source: both backends now confirm the item is queryable (a bounded
~500ms poll) before AddCredentialAsync returns, so any subsequent operation sees
it. Best-effort — returns after the bounded wait regardless, leaving genuine
failures to the caller's own lookup. This also removes the root cause behind the
test-side retry hardening added earlier (kept as defence-in-depth).

Experimental backends only; the file backend has no such window.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 .../Keychain/KeychainCredentialManager.cs     | 31 +++++++++++++++++++
 .../Libsecret/LibsecretCredentialManager.cs   | 30 ++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs b/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs
index 51450a2..f9c79a6 100644
--- a/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs
+++ b/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs
@@ -74,9 +74,40 @@ public Task<string> AddCredentialAsync(string providerName, string accountName,
             };
 
             AddItem(attrs);
+
+            // SecItemAdd can lag SecItemCopyMatching visibility under concurrent
+            // keychain access, so a caller doing add-then-select/delete — e.g.
+            // `accounts add` offering to activate the new credential — can race
+            // the item's own appearance and see the follow-up lookup miss it.
+            // Confirm the item is queryable before returning so that can't happen.
+            ConfirmItemVisible(attrs.Service, accountId);
+
             return Task.FromResult(accountId);
         }
 
+        /// <summary>
+        /// Polls for a just-added item to become visible to
+        /// <c>SecItemCopyMatching</c>, closing the brief add-visibility window.
+        /// Best-effort: returns after a bounded wait even if the item never
+        /// appears, leaving any genuine failure to the caller's own lookup.
+        /// </summary>
+        private static void ConfirmItemVisible(string service, string account)
+        {
+            const int maxAttempts = 20;
+            for (var attempt = 1; attempt <= maxAttempts; attempt++)
+            {
+                if (QuerySingleItem(service, account, includeData: false) is not null)
+                {
+                    return;
+                }
+
+                if (attempt < maxAttempts)
+                {
+                    Thread.Sleep(25);
+                }
+            }
+        }
+
         /// <inheritdoc />
         public Task<IEnumerable<CredentialSummary>> ListCredentialsAsync(string providerName)
         {
diff --git a/src/NextIteration.SpectreConsole.Auth/Persistence/Libsecret/LibsecretCredentialManager.cs b/src/NextIteration.SpectreConsole.Auth/Persistence/Libsecret/LibsecretCredentialManager.cs
index c042a1e..6ad25a8 100644
--- a/src/NextIteration.SpectreConsole.Auth/Persistence/Libsecret/LibsecretCredentialManager.cs
+++ b/src/NextIteration.SpectreConsole.Auth/Persistence/Libsecret/LibsecretCredentialManager.cs
@@ -91,9 +91,39 @@ public Task<string> AddCredentialAsync(string providerName, string accountName,
             var label = $"{_appIdentifier}: {providerName}/{accountName}";
 
             StoreItem(attrs, label, credentialData);
+
+            // A just-stored Secret Service item isn't always immediately visible
+            // to a follow-up search under concurrent access, so a caller doing
+            // add-then-select/delete can race the item's appearance. Confirm it's
+            // queryable before returning so that can't happen.
+            ConfirmItemVisible(providerName, accountId);
+
             return Task.FromResult(accountId);
         }
 
+        /// <summary>
+        /// Polls for a just-stored item to become visible to a Secret Service
+        /// lookup, closing the brief store-visibility window. Best-effort:
+        /// returns after a bounded wait even if the item never appears, leaving
+        /// any genuine failure to the caller's own lookup.
+        /// </summary>
+        private void ConfirmItemVisible(string providerName, string accountId)
+        {
+            const int maxAttempts = 20;
+            for (var attempt = 1; attempt <= maxAttempts; attempt++)
+            {
+                if (LookupCredentialByAccountId(providerName, accountId) is not null)
+                {
+                    return;
+                }
+
+                if (attempt < maxAttempts)
+                {
+                    Thread.Sleep(25);
+                }
+            }
+        }
+
         /// <inheritdoc />
         public Task<IEnumerable<CredentialSummary>> ListCredentialsAsync(string providerName)
         {