From 969ac6243abd9e82687c78eeb366f705a1bfbbcb Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Fri, 28 Aug 2026 13:30:15 -0700
Subject: [PATCH 1/4] credstore: give each process its own probe entry, and
 name the keyring failure on fallback
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Every invocation probed keyring availability by writing and deleting one
fixed-name entry, credstore.probe.<service> / __probe__. Twenty concurrent
invocations therefore churned one keychain item, and on darwin `security
add-generic-password -U` is find-then-create inside the security tool: a
peer's delete or add landing in that window fails the add with
errSecDuplicateItem (rc=45) on a perfectly healthy keychain. The loser
silently switched to the plaintext file fallback, so a machine whose
credentials live in the keychain answered "credentials not found for
profile:<name>" — and where a months-stale credentials.json survived, it
answered with expired tokens instead. Observed today from a connector that
fans out CLI calls: 19 of 20 parallel `basecamp auth status` reported
stale or missing credentials; serial runs were 20/20 fine.

#69 taught the probe to tolerate the lost race (accept the duplicate-item
answer; retry a failed Set and read back a peer's entry). That closed the
bounded darwin path but left the unbounded go-keyring path losing 149 of
200 probes at 20-way concurrency — the retry loses too and the peer has
already deleted its entry — and it was armor around the actual defect: a
shared item.

Now the probe account is __probe__.<pid>, and NewStore serializes probes
within a process, so no two in-flight probes ever touch the same entry.
The duplicate-item and retry/read-back recovery is retired as unreachable.
The account stays deterministic per pid rather than random so a leaked
entry still self-heals — the next process reusing that pid overwrites and
removes it — instead of becoming unfindable, go-keyring having no list API.

Measured against a real keychain, 20 parallel probes x 10 rounds:
raw `security` on one shared account 190/200 failed (all rc=45), on
per-pid accounts 0/200; credstore at the pre-#69 pin 190/200 both paths,
at #69 bounded 0/200 but unbounded 149/200, with this change 0/200 and
0/500 on both paths.

A probe failure is also no longer silent on read. The store keeps the
probe error (ProbeError), the fallback warning names it, and a miss on the
file fallback says "system keyring unavailable (<reason>), fell back to
<path>" rather than a bare "credentials not found". The darwin probe folds
the security tool's diagnostic into its error and a timed-out probe says
so, so the reason is readable rather than "exit status 36".
---
 credstore/probe.go             |  98 +++++++++++++--------------
 credstore/probe_darwin.go      |  28 ++++----
 credstore/probe_darwin_test.go |  53 +++++++--------
 credstore/probe_other.go       |   6 +-
 credstore/probe_test.go        | 117 +++++++++++----------------------
 credstore/store.go             |  66 ++++++++++++++-----
 credstore/store_test.go        |  89 ++++++++++++++++++++++++-
 7 files changed, 263 insertions(+), 194 deletions(-)

diff --git a/credstore/probe.go b/credstore/probe.go
index e196fd1..e7cc969 100644
--- a/credstore/probe.go
+++ b/credstore/probe.go
@@ -2,6 +2,11 @@ package credstore
 
 import (
 	"context"
+	"errors"
+	"fmt"
+	"os"
+	"strconv"
+	"sync"
 	"time"
 
 	"github.com/zalando/go-keyring"
@@ -14,34 +19,38 @@ var probeKeyring = probe
 // probeServicePrefix plus the caller's service — publicly documented on
 // StoreOptions.ProbeTimeout as reserved by credstore, so probing never
 // touches the caller's real service and a colliding consumer would have to
-// deliberately adopt this package's declared namespace. Within it, the key
-// is deliberately deterministic, not random: go-keyring has no list API, so
-// an entry leaked by an abandoned probe (a timed-out probe whose blocked Set
-// completes after the process exits, or a darwin cleanup cut short) would be
-// permanently unfindable under a random name. Under a fixed name, the next
-// probe's Set overwrites the leftover and its Delete removes it — leaks
-// self-heal on the following run.
+// deliberately adopt this package's declared namespace.
 //
-// The fixed name makes concurrent probes race on one shared item. The
-// losing Delete just fails, which is ignored — but on darwin the losing Set
-// can fail too: `security add-generic-password -U` is find-then-create
-// inside the security tool, so a peer's delete/add interleaving surfaces
-// errSecDuplicateItem even though the keychain is healthy. Every probe
-// therefore treats a lost write race backed by write evidence — a
-// duplicate-item error, a successful retry, or the peer's completed write —
-// as availability, not as grounds for the file fallback; see probeDirect
-// and the darwin probeBounded.
+// Within that namespace the account is per process: probeKeyPrefix plus the
+// pid. Concurrent invocations must never share a keychain item, because on
+// darwin `security add-generic-password -U` is find-then-create inside the
+// security tool, and a peer's delete/add landing in that window fails the
+// add with errSecDuplicateItem on a perfectly healthy keychain — twenty
+// concurrent probes of one shared item lost 190 of 200. Distinct pids give
+// each in-flight process its own item, and NewStore serializes probes within
+// a process (probeMu), so no two probes ever touch the same entry.
+//
+// The account is still deterministic for a given pid rather than random:
+// go-keyring has no list API, so an entry leaked by an abandoned probe (a
+// timed-out probe whose blocked Set completes after the process exits, or a
+// darwin cleanup cut short) would be permanently unfindable under a random
+// name. Under the pid-derived name, the next process to reuse that pid
+// overwrites the leftover with its own probe and removes it — leaks still
+// self-heal, on pid reuse instead of on the very next run.
 const (
 	probeServicePrefix = "credstore.probe."
-	probeKey           = "__probe__"
+	probeKeyPrefix     = "__probe__."
 )
 
-// keyring operations, extracted as vars so tests can exercise probeDirect's
-// write-race disambiguation (go-keyring's mock cannot fail Set while
-// answering Get).
+// probeMu serializes probes within a process. The probe account is unique
+// per process, not per probe, so two stores constructed concurrently in one
+// process would otherwise share an item and reintroduce the race above.
+var probeMu sync.Mutex
+
+// keyring operations, extracted as vars so tests can observe the entry
+// probeDirect writes and removes without a live keyring.
 var (
 	keyringSet    = keyring.Set
-	keyringGet    = keyring.Get
 	keyringDelete = keyring.Delete
 )
 
@@ -50,49 +59,36 @@ func probeService(serviceName string) string {
 	return probeServicePrefix + serviceName
 }
 
+// probeKey derives this process's probe account.
+func probeKey() string {
+	return probeKeyPrefix + strconv.Itoa(os.Getpid())
+}
+
 // probe writes and removes a throwaway keyring entry to check availability.
 // A zero or negative timeout probes unbounded, matching historical behavior.
 // A positive timeout bounds the probe; on platforms where the probe runs a
-// child process (darwin), the child is killed when the timeout expires.
+// child process (darwin), the child is killed when the timeout expires. A
+// probe that hits the bound reports the timeout by name, since that reason
+// reaches users through Store.FallbackWarning and Load errors.
 func probe(serviceName string, timeout time.Duration) error {
-	service := probeService(serviceName)
+	service, key := probeService(serviceName), probeKey()
 	if timeout <= 0 {
-		return probeDirect(service, probeKey)
+		return probeDirect(service, key)
 	}
 
 	ctx, cancel := context.WithTimeout(context.Background(), timeout)
 	defer cancel()
-	return probeBounded(ctx, service, probeKey)
+	err := probeBounded(ctx, service, key)
+	if errors.Is(err, context.DeadlineExceeded) {
+		return fmt.Errorf("keyring probe timed out after %s: %w", timeout, err)
+	}
+	return err
 }
 
 // probeDirect probes via go-keyring, which has no cancellation path.
-//
-// A failed Set is not yet an unavailable keyring: concurrent probes share
-// one fixed-name entry, and on darwin a peer's delete/add interleaving makes
-// the write lose with errSecDuplicateItem (see the probeKey comment). The
-// write error alone cannot be classified — go-keyring returns a bare exit
-// error with no output — so recovery demands fresh evidence the keyring
-// accepts writes, never a mere read answer (a read-only keyring cleanly
-// misses a Get of the absent probe entry, and reporting it available would
-// break every later Save). Two forms of write evidence qualify:
-//
-//   - An immediate retry of the Set succeeds — the contended entry has
-//     settled (present, so darwin's -U updates in place; absent, so a plain
-//     create lands) and this process demonstrably wrote.
-//   - The retry also loses, but Get finds the entry — a peer process of the
-//     same uid completed exactly this write moments ago, which is what
-//     sustained churn from concurrent probes looks like.
-//
-// A keyring that fails both writes and cannot show a peer's is reported
-// unavailable with the original write error.
 func probeDirect(serviceName, key string) error {
-	err := keyringSet(serviceName, key, "probe")
-	if err != nil {
-		if retryErr := keyringSet(serviceName, key, "probe"); retryErr != nil {
-			if _, getErr := keyringGet(serviceName, key); getErr != nil {
-				return err
-			}
-		}
+	if err := keyringSet(serviceName, key, "probe"); err != nil {
+		return err
 	}
 	_ = keyringDelete(serviceName, key)
 	return nil
diff --git a/credstore/probe_darwin.go b/credstore/probe_darwin.go
index 1cbd39e..000e24e 100644
--- a/credstore/probe_darwin.go
+++ b/credstore/probe_darwin.go
@@ -25,12 +25,6 @@ var securityPath = "/usr/bin/security"
 // documents this additive bound.
 const probeCleanupTimeout = 5 * time.Second
 
-// errSecDuplicateItem marks a `security` failure that proves the keychain is
-// alive: the OSStatus for "item already exists", printed by `security -i` as
-// "add-generic-password: returned -25299". Matched numerically — the code is
-// ABI-stable where the prose message is not.
-const errSecDuplicateItem = "-25299"
-
 // probeBounded mirrors go-keyring's darwin Set — `security -i` fed an
 // add-generic-password command over stdin — via exec.CommandContext so the
 // child is killed when ctx expires. go-keyring's own exec has no
@@ -50,15 +44,7 @@ func probeBounded(ctx context.Context, serviceName, key string) error {
 		if ctx.Err() != nil {
 			return ctx.Err()
 		}
-		// errSecDuplicateItem is availability, not failure: add -U is
-		// find-then-create inside `security`, and a concurrent probe's
-		// delete/add on the shared fixed-name entry can interleave so the
-		// create loses to a duplicate (see the probeKey comment). The
-		// keychain answered — it is responsive and usable. Fall through to
-		// cleanup, which removes whichever entry won.
-		if !strings.Contains(string(out), errSecDuplicateItem) {
-			return err
-		}
+		return securityError(out, err)
 	}
 
 	afterProbeAdd()
@@ -76,6 +62,18 @@ func probeBounded(ctx context.Context, serviceName, key string) error {
 // cleanup's independence from it.
 var afterProbeAdd = func() {}
 
+// securityError folds the security tool's diagnostic into its exit error.
+// A bare "exit status 36" tells nobody why the keychain was unavailable;
+// the tool's own line ("User interaction is not allowed.") does, and that
+// reason reaches users through Store.FallbackWarning and Load errors.
+func securityError(out []byte, err error) error {
+	diagnostic := strings.Join(strings.Fields(string(out)), " ")
+	if diagnostic == "" {
+		return err
+	}
+	return fmt.Errorf("%s (%w)", diagnostic, err)
+}
+
 var securityArgUnsafe = regexp.MustCompile(`[^\w@%+=:,./-]`)
 
 // quoteSecurityArg mirrors go-keyring's internal shellescape.Quote so the
diff --git a/credstore/probe_darwin_test.go b/credstore/probe_darwin_test.go
index 1f7ee49..5e81835 100644
--- a/credstore/probe_darwin_test.go
+++ b/credstore/probe_darwin_test.go
@@ -108,12 +108,25 @@ func TestProbeBoundedSuccess(t *testing.T) {
 }
 
 // The probe must operate in its own service namespace so it can never touch
-// a credential in the caller's real service, whatever its name.
-func TestProbeUsesIsolatedNamespace(t *testing.T) {
+// a credential in the caller's real service, whatever its name — and under
+// its own per-process account, so concurrent invocations never contend for
+// one keychain item (see probeKey).
+func TestProbeUsesIsolatedPerProcessEntry(t *testing.T) {
 	argsFile := argsStub(t)
 
 	require.NoError(t, probe("svc", 5*time.Second))
-	requireCleanupDelete(t, argsFile, probeServicePrefix+"svc", probeKey)
+	requireCleanupDelete(t, argsFile, probeServicePrefix+"svc", "__probe__."+strconv.Itoa(os.Getpid()))
+}
+
+// A probe that hits its bound must say so: the reason reaches users through
+// the fallback warning and Load errors, where a bare "context deadline
+// exceeded" explains nothing.
+func TestProbeTimeoutIsNamed(t *testing.T) {
+	stubSecurity(t, "#!/bin/sh\nexec sleep 60\n")
+
+	err := probe("svc", 20*time.Millisecond)
+	assert.ErrorIs(t, err, context.DeadlineExceeded)
+	assert.ErrorContains(t, err, "keyring probe timed out after 20ms")
 }
 
 // Regression: the probe deadline expiring immediately after a successful add
@@ -133,31 +146,10 @@ func TestProbeBoundedCleanupSurvivesProbeExpiry(t *testing.T) {
 	requireCleanupDelete(t, argsFile, "test", "__probe_expiry")
 }
 
-// Regression: two concurrent CLI invocations probe the same fixed-name
-// entry, and `add-generic-password -U` is find-then-create inside
-// `security` — a peer's delete/add interleaving makes the losing add fail
-// with errSecDuplicateItem (-25299) on a perfectly healthy keychain. That
-// answer proves availability; treating it as failure silently degraded the
-// loser to the file fallback, which reports "credentials not found" for
-// profiles whose tokens sit in the keychain.
-func TestProbeBoundedDuplicateItemMeansAvailable(t *testing.T) {
-	argsFile := filepath.Join(stubDir(t), "args")
-	// The add (`security -i`) loses the duplicate race; the cleanup delete
-	// succeeds, removing whichever probe entry won.
-	stubSecurity(t, "#!/bin/sh\nAF="+shQuote(argsFile)+"\necho \"$@\" >> \"$AF\"\n"+
-		"if [ \"$1\" = -i ]; then\ncat > /dev/null\necho 'add-generic-password: returned -25299'\n"+
-		"echo 'security: SecKeychainItemCreateFromContent (<default>): The specified item already exists in the keychain.' >&2\nexit 45\nfi\nexit 0\n")
-
-	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
-	defer cancel()
-
-	require.NoError(t, probeBounded(ctx, "test", "__probe_dup"))
-	requireCleanupDelete(t, argsFile, "test", "__probe_dup")
-}
-
-// Any other add failure still reports the keyring unavailable, and cleanup
-// is not attempted.
-func TestProbeBoundedNonDuplicateFailureStillFails(t *testing.T) {
+// A failed add reports the keyring unavailable with the security tool's own
+// diagnostic folded in — that reason is what users see when the store
+// explains its fallback — and cleanup is not attempted.
+func TestProbeBoundedFailureCarriesDiagnostic(t *testing.T) {
 	argsFile := filepath.Join(stubDir(t), "args")
 	stubSecurity(t, "#!/bin/sh\nAF="+shQuote(argsFile)+"\necho \"$@\" >> \"$AF\"\ncat > /dev/null\n"+
 		"echo 'security: SecKeychainItemCreateFromContent (<default>): User interaction is not allowed.' >&2\nexit 36\n")
@@ -165,7 +157,10 @@ func TestProbeBoundedNonDuplicateFailureStillFails(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()
 
-	require.Error(t, probeBounded(ctx, "test", "__probe_fail"))
+	err := probeBounded(ctx, "test", "__probe_fail")
+	require.Error(t, err)
+	assert.ErrorContains(t, err, "User interaction is not allowed.")
+	assert.ErrorContains(t, err, "exit status 36")
 
 	raw, err := os.ReadFile(argsFile)
 	require.NoError(t, err)
diff --git a/credstore/probe_other.go b/credstore/probe_other.go
index 6d63e80..70ca328 100644
--- a/credstore/probe_other.go
+++ b/credstore/probe_other.go
@@ -8,9 +8,9 @@ import "context"
 // backends (dbus secret service, Windows credential manager) run in-process,
 // so timing out abandons at most a goroutine — there is no child process to
 // reclaim. An abandoned probe whose blocked Set later succeeds can leak the
-// probe entry if the process exits before Delete runs; the deterministic
-// probeKey makes that self-healing — the next probe overwrites and removes
-// the leftover (see probeKey).
+// probe entry if the process exits before Delete runs; the pid-derived
+// probeKey makes that self-healing — the next probe from a process reusing
+// that pid overwrites and removes the leftover (see probeKey).
 func probeBounded(ctx context.Context, serviceName, key string) error {
 	done := make(chan error, 1)
 	go func() { done <- probeDirect(serviceName, key) }()
diff --git a/credstore/probe_test.go b/credstore/probe_test.go
index 1c37c21..7b2e7a0 100644
--- a/credstore/probe_test.go
+++ b/credstore/probe_test.go
@@ -2,99 +2,62 @@ package credstore
 
 import (
 	"errors"
+	"os"
+	"strconv"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	"github.com/zalando/go-keyring"
 )
 
-// Pins the leak-containment contract on every platform, not just darwin: a
-// probe abandoned mid-flight (a timed-out non-darwin probe whose blocked Set
-// completes after process exit, or a darwin cleanup cut short) can leave at
-// most the one known entry — deterministic account, reserved service — which
-// the next probe's Set overwrites and Delete removes. The names below are
-// publicly documented on StoreOptions.ProbeTimeout; changing either breaks
-// self-healing across versions and orphans entries written by earlier
-// releases.
-func TestProbeContractIsDeterministicAndReserved(t *testing.T) {
+// Pins the probe-entry contract on every platform, not just darwin.
+//
+// Per process: concurrent invocations must never share a keychain item —
+// on darwin `add-generic-password -U` is find-then-create, and a peer's
+// delete/add landing in that window fails the add with errSecDuplicateItem
+// on a healthy keychain, silently demoting the loser to the file fallback.
+//
+// Deterministic for a pid, not random: a probe abandoned mid-flight can
+// leave at most one entry — this account, reserved service — which the next
+// process reusing the pid overwrites and removes. The names are publicly
+// documented on StoreOptions.ProbeTimeout.
+func TestProbeContractIsPerProcessAndReserved(t *testing.T) {
 	assert.Equal(t, "credstore.probe.svc", probeService("svc"))
-	assert.Equal(t, "__probe__", probeKey)
+	assert.Equal(t, "__probe__."+strconv.Itoa(os.Getpid()), probeKey())
 }
 
-// stubKeyringOps replaces probeDirect's keyring operations for one test.
-// go-keyring's mock cannot fail Set while answering Get, which is exactly
-// the shape of the concurrent-probe write race.
-func stubKeyringOps(t *testing.T, set func(string, string, string) error, get func(string, string) (string, error)) (deleted *bool) {
+// recordKeyringOps replaces probeDirect's keyring operations for one test,
+// recording the entry each touched.
+func recordKeyringOps(t *testing.T, setErr error) (set, deleted *[2]string) {
 	t.Helper()
-	deleted = new(bool)
-	restoreSet, restoreGet, restoreDelete := keyringSet, keyringGet, keyringDelete
-	keyringSet = set
-	keyringGet = get
+	set, deleted = new([2]string), new([2]string)
+	restoreSet, restoreDelete := keyringSet, keyringDelete
+	keyringSet = func(service, key, _ string) error {
+		*set = [2]string{service, key}
+		return setErr
+	}
 	keyringDelete = func(service, key string) error {
-		*deleted = true
+		*deleted = [2]string{service, key}
 		return nil
 	}
-	t.Cleanup(func() { keyringSet, keyringGet, keyringDelete = restoreSet, restoreGet, restoreDelete })
-	return deleted
+	t.Cleanup(func() { keyringSet, keyringDelete = restoreSet, restoreDelete })
+	return set, deleted
 }
 
-// Regression: concurrent probes share one fixed-name entry, and a losing
-// write (darwin surfaces errSecDuplicateItem through go-keyring as a bare
-// exit error) must not demote a healthy keyring to the file fallback.
-// Recovery requires fresh write evidence — this process's retry landing, or
-// a peer's completed write sitting in the keyring — never a bare read
-// answer, which a read-only keyring could also give.
-func TestProbeDirectWriteRaceRecovery(t *testing.T) {
-	setErr := errors.New("exit status 45")
-
-	t.Run("retry lands once the churned entry settles", func(t *testing.T) {
-		calls := 0
-		deleted := stubKeyringOps(t,
-			func(_, _, _ string) error {
-				calls++
-				if calls == 1 {
-					return setErr
-				}
-				return nil
-			},
-			func(_, _ string) (string, error) { t.Fatal("no read needed when the retry lands"); return "", nil })
-
-		assert.NoError(t, probeDirect("credstore.probe.svc", probeKey))
-		assert.Equal(t, 2, calls)
-		assert.True(t, *deleted, "cleanup should remove the retried entry")
-	})
-
-	t.Run("retry loses too but the peer's write is present", func(t *testing.T) {
-		deleted := stubKeyringOps(t,
-			func(_, _, _ string) error { return setErr },
-			func(_, _ string) (string, error) { return "probe", nil })
-
-		assert.NoError(t, probeDirect("credstore.probe.svc", probeKey))
-		assert.True(t, *deleted, "cleanup should remove the peer's entry")
-	})
-
-	t.Run("read-only keyring: writes fail, probe entry absent", func(t *testing.T) {
-		stubKeyringOps(t,
-			func(_, _, _ string) error { return setErr },
-			func(_, _ string) (string, error) { return "", keyring.ErrNotFound })
-
-		assert.ErrorIs(t, probeDirect("credstore.probe.svc", probeKey), setErr)
-	})
-
-	t.Run("keyring fails the read too: genuinely unavailable", func(t *testing.T) {
-		stubKeyringOps(t,
-			func(_, _, _ string) error { return setErr },
-			func(_, _ string) (string, error) { return "", errors.New("no keyring provider") })
+// The unbounded probe goes through go-keyring rather than the security
+// binary, so it must derive the same per-process entry as the bounded one:
+// a fixed account on either path would put that path back in the race.
+func TestProbeDirectUsesPerProcessEntry(t *testing.T) {
+	set, deleted := recordKeyringOps(t, nil)
 
-		assert.ErrorIs(t, probeDirect("credstore.probe.svc", probeKey), setErr)
-	})
+	assert.NoError(t, probe("svc", 0))
+	assert.Equal(t, [2]string{"credstore.probe.svc", probeKey()}, *set)
+	assert.Equal(t, *set, *deleted, "cleanup should remove exactly the entry the probe wrote")
 }
 
-func TestProbeDirectSuccessCleansUp(t *testing.T) {
-	deleted := stubKeyringOps(t,
-		func(_, _, _ string) error { return nil },
-		func(_, _ string) (string, error) { t.Fatal("no read needed when the write succeeds"); return "", nil })
+func TestProbeDirectFailureSkipsCleanup(t *testing.T) {
+	setErr := errors.New("no keyring provider")
+	_, deleted := recordKeyringOps(t, setErr)
 
-	assert.NoError(t, probeDirect("credstore.probe.svc", probeKey))
-	assert.True(t, *deleted)
+	assert.ErrorIs(t, probe("svc", 0), setErr)
+	assert.Zero(t, *deleted, "a failed write has nothing to clean up")
 }
diff --git a/credstore/store.go b/credstore/store.go
index dd2556f..7c3a6e6 100644
--- a/credstore/store.go
+++ b/credstore/store.go
@@ -3,7 +3,6 @@ package credstore
 import (
 	"fmt"
 	"os"
-	"path/filepath"
 	"time"
 
 	"github.com/zalando/go-keyring"
@@ -29,9 +28,11 @@ type StoreOptions struct {
 	// means no bound, matching historical behavior. When the probe times
 	// out, the store falls back to file storage as if the probe had failed.
 	// Probing writes and removes a throwaway entry under the dedicated
-	// keyring service "credstore.probe.<ServiceName>" (account "__probe__")
-	// — a namespace reserved by this package — never under ServiceName
-	// itself, so a probe cannot touch real credentials.
+	// keyring service "credstore.probe.<ServiceName>" (account
+	// "__probe__.<pid>") — a namespace reserved by this package — never
+	// under ServiceName itself, so a probe cannot touch real credentials.
+	// The account is per process so concurrent invocations never contend
+	// for one keychain item, and probes within a process are serialized.
 	//
 	// On darwin, removal of the throwaway probe entry runs synchronously
 	// after a successful probe with a short budget of its own, so worst-case
@@ -56,10 +57,15 @@ type StoreOptions struct {
 
 // Store handles credential storage with keyring preference and file fallback.
 type Store struct {
-	serviceName     string
-	useKeyring      bool
-	fallbackDir     string
-	fallbackWarning string
+	serviceName string
+	useKeyring  bool
+	fallbackDir string
+
+	// probeErr is why the keyring probe failed when the store fell back to
+	// file storage against the caller's wishes. Nil when the keyring is in
+	// use and when file storage was requested (ForceFile, DisableEnvVar):
+	// a requested fallback is not a degradation and warrants no warning.
+	probeErr error
 }
 
 // NewStore creates a credential store. It probes the system keyring
@@ -69,22 +75,36 @@ func NewStore(opts StoreOptions) *Store {
 		return &Store{serviceName: opts.ServiceName, useKeyring: false, fallbackDir: opts.FallbackDir}
 	}
 
-	if probeKeyring(opts.ServiceName, opts.ProbeTimeout) == nil {
-		return &Store{serviceName: opts.ServiceName, useKeyring: true, fallbackDir: opts.FallbackDir}
-	}
+	probeMu.Lock()
+	err := probeKeyring(opts.ServiceName, opts.ProbeTimeout)
+	probeMu.Unlock()
 
 	return &Store{
-		serviceName:     opts.ServiceName,
-		useKeyring:      false,
-		fallbackDir:     opts.FallbackDir,
-		fallbackWarning: fmt.Sprintf("system keyring unavailable, credentials stored in plaintext at %s", filepath.Join(opts.FallbackDir, "credentials.json")),
+		serviceName: opts.ServiceName,
+		useKeyring:  err == nil,
+		fallbackDir: opts.FallbackDir,
+		probeErr:    err,
 	}
 }
 
+// ProbeError returns why the keyring probe failed and the store fell back
+// to file storage, or nil when the keyring is in use or file storage was
+// requested outright.
+func (s *Store) ProbeError() error {
+	return s.probeErr
+}
+
 // FallbackWarning returns a warning message if the store fell back to file
-// storage, or empty string if using keyring.
+// storage because the keyring probe failed, or empty string otherwise. The
+// message names the probe failure so a fallback is never silent about its
+// cause. Callers decide where to surface it; surface it on reads as well as
+// writes, since a fallback read may return a stale file left over from an
+// earlier fallback rather than the credentials the keyring holds.
 func (s *Store) FallbackWarning() string {
-	return s.fallbackWarning
+	if s.probeErr == nil {
+		return ""
+	}
+	return fmt.Sprintf("system keyring unavailable (%v), credentials stored in plaintext at %s", s.probeErr, s.credentialsPath())
 }
 
 func (s *Store) key(name string) string {
@@ -92,6 +112,11 @@ func (s *Store) key(name string) string {
 }
 
 // Load retrieves credentials for the given key.
+//
+// On the file fallback, a miss is reported together with the keyring probe
+// failure that caused the fallback: "credentials not found" alone reads as
+// "log in again" when the truth may be that the credentials sit safely in
+// the keyring and only this process could not reach it.
 func (s *Store) Load(key string) ([]byte, error) {
 	if s.useKeyring {
 		data, err := keyring.Get(s.serviceName, s.key(key))
@@ -100,7 +125,12 @@ func (s *Store) Load(key string) ([]byte, error) {
 		}
 		return []byte(data), nil
 	}
-	return s.loadFromFile(key)
+
+	data, err := s.loadFromFile(key)
+	if err != nil && s.probeErr != nil {
+		return nil, fmt.Errorf("%w: system keyring unavailable (%w), fell back to %s", err, s.probeErr, s.credentialsPath())
+	}
+	return data, err
 }
 
 // Save stores credentials for the given key.
diff --git a/credstore/store_test.go b/credstore/store_test.go
index bb9a833..300400f 100644
--- a/credstore/store_test.go
+++ b/credstore/store_test.go
@@ -2,8 +2,11 @@ package credstore
 
 import (
 	"context"
+	"errors"
 	"os"
 	"path/filepath"
+	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -115,7 +118,91 @@ func TestProbeTimeoutFallsBackToFile(t *testing.T) {
 	})
 
 	assert.False(t, store.UsingKeyring())
-	assert.Contains(t, store.FallbackWarning(), "system keyring unavailable")
+	assert.ErrorIs(t, store.ProbeError(), context.DeadlineExceeded)
+	assert.Contains(t, store.FallbackWarning(), "system keyring unavailable (context deadline exceeded)")
+}
+
+// Regression: a probe failure silently demoted reads to the plaintext file,
+// and a miss there reported "credentials not found" — indistinguishable from
+// never having logged in, when the credentials sat safely in the keyring
+// this process merely failed to reach. The store must keep the probe error
+// and name it wherever the fallback shows: the warning and Load's error.
+func TestProbeFailureIsReportedOnLoad(t *testing.T) {
+	dir := t.TempDir()
+	probeErr := errors.New("User interaction is not allowed. (exit status 36)")
+	stubProbe(t, func(string, time.Duration) error { return probeErr })
+
+	store := NewStore(StoreOptions{ServiceName: "test", FallbackDir: dir})
+	credentialsPath := filepath.Join(dir, "credentials.json")
+
+	assert.Same(t, probeErr, store.ProbeError())
+	assert.Equal(t, "system keyring unavailable ("+probeErr.Error()+"), credentials stored in plaintext at "+credentialsPath,
+		store.FallbackWarning())
+
+	_, err := store.Load("profile:work")
+	require.Error(t, err)
+	assert.ErrorIs(t, err, probeErr)
+	assert.ErrorContains(t, err, "credentials not found for profile:work")
+	assert.ErrorContains(t, err, "system keyring unavailable ("+probeErr.Error()+")")
+	assert.ErrorContains(t, err, "fell back to "+credentialsPath)
+}
+
+// The file fallback keeps working after a failed probe — it is the only
+// storage on hosts with no keyring at all — and a hit there is not an error.
+func TestProbeFailureStillReadsFallbackFile(t *testing.T) {
+	dir := t.TempDir()
+	stubProbe(t, func(string, time.Duration) error { return errors.New("no keyring") })
+
+	store := NewStore(StoreOptions{ServiceName: "test", FallbackDir: dir})
+
+	require.NoError(t, store.Save("mykey", []byte(`{"token":"abc123"}`)))
+	data, err := store.Load("mykey")
+	require.NoError(t, err)
+	assert.JSONEq(t, `{"token":"abc123"}`, string(data))
+}
+
+// File storage the caller asked for is not a fallback: no probe ran, so
+// there is no probe error to report and nothing to warn about.
+func TestRequestedFileStorageReportsNoProbeFailure(t *testing.T) {
+	dir := t.TempDir()
+	stubProbe(t, func(string, time.Duration) error {
+		t.Error("probe should not run when file storage is requested")
+		return nil
+	})
+
+	store := NewStore(StoreOptions{ServiceName: "test", ForceFile: true, FallbackDir: dir})
+
+	assert.NoError(t, store.ProbeError())
+	assert.Empty(t, store.FallbackWarning())
+	_, err := store.Load("mykey")
+	assert.EqualError(t, err, "credentials not found for mykey")
+}
+
+// The probe entry is unique per process, not per probe, so stores built
+// concurrently within one process must not probe at the same time — they
+// would share the entry and reintroduce the cross-process race in-process.
+func TestNewStoreSerializesProbes(t *testing.T) {
+	var inFlight, maxInFlight atomic.Int32
+	stubProbe(t, func(string, time.Duration) error {
+		n := inFlight.Add(1)
+		defer inFlight.Add(-1)
+		for {
+			seen := maxInFlight.Load()
+			if n <= seen || maxInFlight.CompareAndSwap(seen, n) {
+				break
+			}
+		}
+		time.Sleep(5 * time.Millisecond)
+		return nil
+	})
+
+	var wg sync.WaitGroup
+	for range 8 {
+		wg.Go(func() { NewStore(StoreOptions{ServiceName: "test", FallbackDir: t.TempDir()}) })
+	}
+	wg.Wait()
+
+	assert.Equal(t, int32(1), maxInFlight.Load(), "probes must run one at a time within a process")
 }
 
 func TestZeroValueOptionsProbeUnbounded(t *testing.T) {

From 1b3e5b5aa4e43e5af303cf91b2f83bb71d1cc86f Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Fri, 28 Aug 2026 13:39:58 -0700
Subject: [PATCH 2/4] credstore: number probe entries per probe, not per
 process
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

A per-process account plus an in-process mutex left one gap: on
non-darwin, a bounded probe that times out abandons its worker goroutine
mid-Set, and releasing the mutex then let a later NewStore in the same
process probe under the same pid account while that worker was still
running against it. Holding the mutex until the worker finished would
have made the next NewStore wait on the very hang the timeout exists to
escape.

An in-process sequence number in the account — __probe__.<pid>.<n> —
gives every probe its own entry, which makes the mutex unnecessary and
removes it. Leaks still self-heal on pid reuse: the next process with that
pid overwrites and removes the same-numbered leftover, and in practice a
process probes once, so that is entry 1.

Real keychain, 20 parallel x 10 rounds: still 0/200 on both paths.
---
 credstore/probe.go             | 46 ++++++++++++-------------
 credstore/probe_darwin_test.go | 15 +++++---
 credstore/probe_test.go        | 40 ++++++++++++++--------
 credstore/store.go             |  9 ++---
 credstore/store_test.go        | 62 ----------------------------------
 5 files changed, 63 insertions(+), 109 deletions(-)

diff --git a/credstore/probe.go b/credstore/probe.go
index e7cc969..485b32f 100644
--- a/credstore/probe.go
+++ b/credstore/probe.go
@@ -5,8 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"strconv"
-	"sync"
+	"sync/atomic"
 	"time"
 
 	"github.com/zalando/go-keyring"
@@ -21,31 +20,32 @@ var probeKeyring = probe
 // touches the caller's real service and a colliding consumer would have to
 // deliberately adopt this package's declared namespace.
 //
-// Within that namespace the account is per process: probeKeyPrefix plus the
-// pid. Concurrent invocations must never share a keychain item, because on
-// darwin `security add-generic-password -U` is find-then-create inside the
-// security tool, and a peer's delete/add landing in that window fails the
-// add with errSecDuplicateItem on a perfectly healthy keychain — twenty
-// concurrent probes of one shared item lost 190 of 200. Distinct pids give
-// each in-flight process its own item, and NewStore serializes probes within
-// a process (probeMu), so no two probes ever touch the same entry.
+// Within that namespace the account is per probe: probeKeyPrefix, the pid,
+// and an in-process sequence number. Concurrent probes must never share a
+// keychain item, because on darwin `security add-generic-password -U` is
+// find-then-create inside the security tool, and a peer's delete/add
+// landing in that window fails the add with errSecDuplicateItem on a
+// perfectly healthy keychain — twenty concurrent probes of one shared item
+// lost 190 of 200. Distinct pids separate processes; the sequence number
+// separates probes within a process, including one still running after its
+// bounded wait gave up (non-darwin abandons the worker goroutine) from any
+// probe started later. No two probes ever touch the same entry.
 //
-// The account is still deterministic for a given pid rather than random:
-// go-keyring has no list API, so an entry leaked by an abandoned probe (a
-// timed-out probe whose blocked Set completes after the process exits, or a
-// darwin cleanup cut short) would be permanently unfindable under a random
-// name. Under the pid-derived name, the next process to reuse that pid
-// overwrites the leftover with its own probe and removes it — leaks still
-// self-heal, on pid reuse instead of on the very next run.
+// The account is still deterministic rather than random: go-keyring has no
+// list API, so an entry leaked by an abandoned probe (a timed-out probe
+// whose blocked Set completes after the process exits, or a darwin cleanup
+// cut short) would be permanently unfindable under a random name. Under
+// the pid-and-sequence name, the next process to reuse that pid overwrites
+// the leftover with its own same-numbered probe — the first, in practice,
+// since a process probes once — and removes it. Leaks still self-heal, on
+// pid reuse instead of on the very next run.
 const (
 	probeServicePrefix = "credstore.probe."
 	probeKeyPrefix     = "__probe__."
 )
 
-// probeMu serializes probes within a process. The probe account is unique
-// per process, not per probe, so two stores constructed concurrently in one
-// process would otherwise share an item and reintroduce the race above.
-var probeMu sync.Mutex
+// probeSeq numbers this process's probes so no two share an account.
+var probeSeq atomic.Uint64
 
 // keyring operations, extracted as vars so tests can observe the entry
 // probeDirect writes and removes without a live keyring.
@@ -59,9 +59,9 @@ func probeService(serviceName string) string {
 	return probeServicePrefix + serviceName
 }
 
-// probeKey derives this process's probe account.
+// probeKey derives a fresh probe account for this process.
 func probeKey() string {
-	return probeKeyPrefix + strconv.Itoa(os.Getpid())
+	return fmt.Sprintf("%s%d.%d", probeKeyPrefix, os.Getpid(), probeSeq.Add(1))
 }
 
 // probe writes and removes a throwaway keyring entry to check availability.
diff --git a/credstore/probe_darwin_test.go b/credstore/probe_darwin_test.go
index 5e81835..50a3787 100644
--- a/credstore/probe_darwin_test.go
+++ b/credstore/probe_darwin_test.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strconv"
 	"strings"
 	"syscall"
@@ -109,13 +110,19 @@ func TestProbeBoundedSuccess(t *testing.T) {
 
 // The probe must operate in its own service namespace so it can never touch
 // a credential in the caller's real service, whatever its name — and under
-// its own per-process account, so concurrent invocations never contend for
-// one keychain item (see probeKey).
-func TestProbeUsesIsolatedPerProcessEntry(t *testing.T) {
+// its own per-probe account, so concurrent probes never contend for one
+// keychain item (see probeKey).
+func TestProbeUsesIsolatedPerProbeEntry(t *testing.T) {
 	argsFile := argsStub(t)
 
 	require.NoError(t, probe("svc", 5*time.Second))
-	requireCleanupDelete(t, argsFile, probeServicePrefix+"svc", "__probe__."+strconv.Itoa(os.Getpid()))
+
+	raw, err := os.ReadFile(argsFile)
+	require.NoError(t, err)
+	lines := strings.Split(strings.TrimSpace(string(raw)), "\n")
+	require.Len(t, lines, 2, "probe should add then delete the probe entry")
+	assert.Equal(t, "-i", lines[0])
+	assert.Regexp(t, "^delete-generic-password -s "+regexp.QuoteMeta(probeServicePrefix+"svc")+" -a "+probeKeyPattern()[1:], lines[1])
 }
 
 // A probe that hits its bound must say so: the reason reaches users through
diff --git a/credstore/probe_test.go b/credstore/probe_test.go
index 7b2e7a0..a612e4c 100644
--- a/credstore/probe_test.go
+++ b/credstore/probe_test.go
@@ -11,18 +11,29 @@ import (
 
 // Pins the probe-entry contract on every platform, not just darwin.
 //
-// Per process: concurrent invocations must never share a keychain item —
-// on darwin `add-generic-password -U` is find-then-create, and a peer's
-// delete/add landing in that window fails the add with errSecDuplicateItem
-// on a healthy keychain, silently demoting the loser to the file fallback.
+// Per probe: concurrent probes must never share a keychain item — on darwin
+// `add-generic-password -U` is find-then-create, and a peer's delete/add
+// landing in that window fails the add with errSecDuplicateItem on a
+// healthy keychain, silently demoting the loser to the file fallback. The
+// pid separates processes; the sequence separates probes within one,
+// including a timed-out probe's abandoned worker from a later probe.
 //
-// Deterministic for a pid, not random: a probe abandoned mid-flight can
-// leave at most one entry — this account, reserved service — which the next
-// process reusing the pid overwrites and removes. The names are publicly
-// documented on StoreOptions.ProbeTimeout.
-func TestProbeContractIsPerProcessAndReserved(t *testing.T) {
+// Deterministic, not random: a probe abandoned mid-flight can leave at most
+// one entry — this account, reserved service — which the next process
+// reusing the pid overwrites and removes. The names are publicly documented
+// on StoreOptions.ProbeTimeout.
+func TestProbeContractIsPerProbeAndReserved(t *testing.T) {
 	assert.Equal(t, "credstore.probe.svc", probeService("svc"))
-	assert.Equal(t, "__probe__."+strconv.Itoa(os.Getpid()), probeKey())
+
+	first, second := probeKey(), probeKey()
+	assert.Regexp(t, probeKeyPattern(), first)
+	assert.Regexp(t, probeKeyPattern(), second)
+	assert.NotEqual(t, first, second, "each probe gets its own account")
+}
+
+// probeKeyPattern matches any probe account this process derives.
+func probeKeyPattern() string {
+	return `^__probe__\.` + strconv.Itoa(os.Getpid()) + `\.\d+$`
 }
 
 // recordKeyringOps replaces probeDirect's keyring operations for one test,
@@ -44,13 +55,14 @@ func recordKeyringOps(t *testing.T, setErr error) (set, deleted *[2]string) {
 }
 
 // The unbounded probe goes through go-keyring rather than the security
-// binary, so it must derive the same per-process entry as the bounded one:
-// a fixed account on either path would put that path back in the race.
-func TestProbeDirectUsesPerProcessEntry(t *testing.T) {
+// binary, so it must derive the same per-probe entry as the bounded one: a
+// fixed account on either path would put that path back in the race.
+func TestProbeDirectUsesPerProbeEntry(t *testing.T) {
 	set, deleted := recordKeyringOps(t, nil)
 
 	assert.NoError(t, probe("svc", 0))
-	assert.Equal(t, [2]string{"credstore.probe.svc", probeKey()}, *set)
+	assert.Equal(t, "credstore.probe.svc", set[0])
+	assert.Regexp(t, probeKeyPattern(), set[1])
 	assert.Equal(t, *set, *deleted, "cleanup should remove exactly the entry the probe wrote")
 }
 
diff --git a/credstore/store.go b/credstore/store.go
index 7c3a6e6..a8af293 100644
--- a/credstore/store.go
+++ b/credstore/store.go
@@ -29,10 +29,10 @@ type StoreOptions struct {
 	// out, the store falls back to file storage as if the probe had failed.
 	// Probing writes and removes a throwaway entry under the dedicated
 	// keyring service "credstore.probe.<ServiceName>" (account
-	// "__probe__.<pid>") — a namespace reserved by this package — never
+	// "__probe__.<pid>.<n>") — a namespace reserved by this package — never
 	// under ServiceName itself, so a probe cannot touch real credentials.
-	// The account is per process so concurrent invocations never contend
-	// for one keychain item, and probes within a process are serialized.
+	// The account is unique per probe, so concurrent invocations never
+	// contend for one keychain item.
 	//
 	// On darwin, removal of the throwaway probe entry runs synchronously
 	// after a successful probe with a short budget of its own, so worst-case
@@ -75,10 +75,7 @@ func NewStore(opts StoreOptions) *Store {
 		return &Store{serviceName: opts.ServiceName, useKeyring: false, fallbackDir: opts.FallbackDir}
 	}
 
-	probeMu.Lock()
 	err := probeKeyring(opts.ServiceName, opts.ProbeTimeout)
-	probeMu.Unlock()
-
 	return &Store{
 		serviceName: opts.ServiceName,
 		useKeyring:  err == nil,
diff --git a/credstore/store_test.go b/credstore/store_test.go
index 300400f..8594107 100644
--- a/credstore/store_test.go
+++ b/credstore/store_test.go
@@ -5,8 +5,6 @@ import (
 	"errors"
 	"os"
 	"path/filepath"
-	"sync"
-	"sync/atomic"
 	"testing"
 	"time"
 
@@ -177,63 +175,3 @@ func TestRequestedFileStorageReportsNoProbeFailure(t *testing.T) {
 	_, err := store.Load("mykey")
 	assert.EqualError(t, err, "credentials not found for mykey")
 }
-
-// The probe entry is unique per process, not per probe, so stores built
-// concurrently within one process must not probe at the same time — they
-// would share the entry and reintroduce the cross-process race in-process.
-func TestNewStoreSerializesProbes(t *testing.T) {
-	var inFlight, maxInFlight atomic.Int32
-	stubProbe(t, func(string, time.Duration) error {
-		n := inFlight.Add(1)
-		defer inFlight.Add(-1)
-		for {
-			seen := maxInFlight.Load()
-			if n <= seen || maxInFlight.CompareAndSwap(seen, n) {
-				break
-			}
-		}
-		time.Sleep(5 * time.Millisecond)
-		return nil
-	})
-
-	var wg sync.WaitGroup
-	for range 8 {
-		wg.Go(func() { NewStore(StoreOptions{ServiceName: "test", FallbackDir: t.TempDir()}) })
-	}
-	wg.Wait()
-
-	assert.Equal(t, int32(1), maxInFlight.Load(), "probes must run one at a time within a process")
-}
-
-func TestZeroValueOptionsProbeUnbounded(t *testing.T) {
-	dir := t.TempDir()
-	probed := false
-	stubProbe(t, func(serviceName string, timeout time.Duration) error {
-		probed = true
-		assert.Zero(t, timeout)
-		return nil
-	})
-
-	store := NewStore(StoreOptions{
-		ServiceName: "test",
-		FallbackDir: dir,
-	})
-
-	assert.True(t, probed)
-	assert.True(t, store.UsingKeyring())
-	assert.Empty(t, store.FallbackWarning())
-}
-
-func TestLoadNonexistent(t *testing.T) {
-	dir := t.TempDir()
-	t.Setenv("TEST_NO_KEYRING", "1")
-
-	store := NewStore(StoreOptions{
-		ServiceName:   "test",
-		DisableEnvVar: "TEST_NO_KEYRING",
-		FallbackDir:   dir,
-	})
-
-	_, err := store.Load("nonexistent")
-	assert.Error(t, err)
-}

From 38e7b4447658b2fa0dc1e01cf96821b3739130b9 Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Fri, 28 Aug 2026 13:56:47 -0700
Subject: [PATCH 3/4] credstore: name the keychain failure on the unbounded
 darwin probe too
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Only the bounded probe folded security's diagnostic into its error. The
unbounded probe goes through go-keyring, whose darwin Set returns
cmd.Wait()'s bare "exit status N" and discards the diagnostic line — and
the unbounded probe is the interactive path, every session with a
terminal. So a headless fallback read "User interaction is not allowed.
(exit status 36)" while an interactive user with a locked keychain got
"system keyring unavailable (exit status 36)": a number, not a reason.

security exits with the low byte of the SecBase.h OSStatus, so the codes
are stable. A darwin-only table names the ones an add can produce on an
unavailable keychain (36, 37, 45, 50, 51, 52, 53, 128) with the text
`security error <OSStatus>` prints, in the same "<reason> (exit status
N)" shape as the bounded path. Unknown exit statuses and non-exit errors
pass through unchanged; other platforms' backends run in-process and
already name their failures.
---
 credstore/probe.go             |  7 +++++--
 credstore/probe_darwin.go      | 34 ++++++++++++++++++++++++++++++++++
 credstore/probe_darwin_test.go | 26 ++++++++++++++++++++++++++
 credstore/probe_other.go       |  4 ++++
 4 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/credstore/probe.go b/credstore/probe.go
index 485b32f..4729ccb 100644
--- a/credstore/probe.go
+++ b/credstore/probe.go
@@ -85,10 +85,13 @@ func probe(serviceName string, timeout time.Duration) error {
 	return err
 }
 
-// probeDirect probes via go-keyring, which has no cancellation path.
+// probeDirect probes via go-keyring, which has no cancellation path. Its
+// failure is named by the platform (keyringError) so the unbounded path's
+// reason reads as well as the bounded path's: on darwin go-keyring returns
+// a bare "exit status N" with the security tool's diagnostic discarded.
 func probeDirect(serviceName, key string) error {
 	if err := keyringSet(serviceName, key, "probe"); err != nil {
-		return err
+		return keyringError(err)
 	}
 	_ = keyringDelete(serviceName, key)
 	return nil
diff --git a/credstore/probe_darwin.go b/credstore/probe_darwin.go
index 000e24e..1d12096 100644
--- a/credstore/probe_darwin.go
+++ b/credstore/probe_darwin.go
@@ -3,6 +3,7 @@ package credstore
 import (
 	"context"
 	"encoding/base64"
+	"errors"
 	"fmt"
 	"os/exec"
 	"regexp"
@@ -74,6 +75,39 @@ func securityError(out []byte, err error) error {
 	return fmt.Errorf("%s (%w)", diagnostic, err)
 }
 
+// securityExitReasons names the keychain failures go-keyring's darwin Set
+// can surface, by exit status. go-keyring returns cmd.Wait()'s bare "exit
+// status N" and discards security's own diagnostic line, and the unbounded
+// probe — every session with a terminal — goes through go-keyring, so
+// without this an interactive user's fallback read "exit status 36" where
+// the bounded (headless) probe's reads "User interaction is not allowed."
+// security exits with the low byte of the SecBase.h OSStatus, so the codes
+// are stable; the text is what `security error <OSStatus>` prints.
+var securityExitReasons = map[int]string{
+	36:  "User interaction is not allowed.",                                 // errSecInteractionNotAllowed (-25308)
+	37:  "A default keychain could not be found.",                           // errSecNoDefaultKeychain (-25307)
+	45:  "The specified item already exists in the keychain.",               // errSecDuplicateItem (-25299)
+	50:  "The specified keychain could not be found.",                       // errSecNoSuchKeychain (-25294)
+	51:  "The user name or passphrase you entered is not correct.",          // errSecAuthFailed (-25293)
+	52:  "This keychain cannot be modified.",                                // errSecReadOnly (-25292)
+	53:  "No keychain is available. You may need to restart your computer.", // errSecNotAvailable (-25291)
+	128: "User canceled the operation.",                                     // errSecUserCanceled (-128)
+}
+
+// keyringError folds the security tool's reason into a go-keyring exit
+// error, in the same shape securityError gives the bounded path. Any other
+// error — an exit status with no keychain meaning, or no exit at all —
+// passes through unchanged rather than being given an invented reason.
+func keyringError(err error) error {
+	var exit *exec.ExitError
+	if errors.As(err, &exit) {
+		if reason, ok := securityExitReasons[exit.ExitCode()]; ok {
+			return fmt.Errorf("%s (%w)", reason, err)
+		}
+	}
+	return err
+}
+
 var securityArgUnsafe = regexp.MustCompile(`[^\w@%+=:,./-]`)
 
 // quoteSecurityArg mirrors go-keyring's internal shellescape.Quote so the
diff --git a/credstore/probe_darwin_test.go b/credstore/probe_darwin_test.go
index 50a3787..c71cdca 100644
--- a/credstore/probe_darwin_test.go
+++ b/credstore/probe_darwin_test.go
@@ -3,6 +3,7 @@ package credstore
 import (
 	"context"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"regexp"
 	"strconv"
@@ -176,6 +177,31 @@ func TestProbeBoundedFailureCarriesDiagnostic(t *testing.T) {
 	assert.Equal(t, "-i", lines[0])
 }
 
+// Regression: go-keyring's darwin Set returns cmd.Wait()'s bare "exit
+// status 36" with security's diagnostic discarded, so the unbounded probe —
+// the interactive path — explained its fallback with a number where the
+// bounded probe gave the reason. The exit status must be named the same way.
+func TestProbeDirectNamesSecurityExitStatus(t *testing.T) {
+	exit36 := exec.Command("/bin/sh", "-c", "exit 36").Run()
+	require.Error(t, exit36)
+	recordKeyringOps(t, exit36)
+
+	err := probe("svc", 0)
+	assert.ErrorIs(t, err, exit36)
+	assert.EqualError(t, err, "User interaction is not allowed. (exit status 36)")
+}
+
+// An exit status with no keychain meaning passes through unchanged rather
+// than being given an invented reason. (A non-exit error is covered by
+// TestProbeDirectFailureSkipsCleanup.)
+func TestProbeDirectPassesUnknownExitStatusThrough(t *testing.T) {
+	exit3 := exec.Command("/bin/sh", "-c", "exit 3").Run()
+	require.Error(t, exit3)
+	recordKeyringOps(t, exit3)
+
+	assert.Same(t, exit3, probe("svc", 0))
+}
+
 func TestQuoteSecurityArg(t *testing.T) {
 	assert.Equal(t, "basecamp", quoteSecurityArg("basecamp"))
 	assert.Equal(t, "''", quoteSecurityArg(""))
diff --git a/credstore/probe_other.go b/credstore/probe_other.go
index 70ca328..1dd09ad 100644
--- a/credstore/probe_other.go
+++ b/credstore/probe_other.go
@@ -22,3 +22,7 @@ func probeBounded(ctx context.Context, serviceName, key string) error {
 		return ctx.Err()
 	}
 }
+
+// keyringError passes a go-keyring failure through: non-darwin backends run
+// in-process and their errors already name the failure.
+func keyringError(err error) error { return err }

From 945a6dc838f5644aaaa03a7af702b53fb83b197e Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Fri, 28 Aug 2026 13:56:47 -0700
Subject: [PATCH 4/4] credstore: pin the healthy-probe branch of NewStore
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The rewrite of NewStore replaced TestZeroValueOptionsProbeUnbounded, the
only test of a successful probe, with the fallback-side tests, so nothing
asserted that a healthy probe keeps the keyring: a mutant that always
fell back to the plaintext file (useKeyring: false) passed the suite.
Restore the happy path — UsingKeyring true, ProbeError nil, no warning —
and the zero-timeout contract that tests mocking the keyring rely on.
---
 credstore/store_test.go | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/credstore/store_test.go b/credstore/store_test.go
index 8594107..0940c6d 100644
--- a/credstore/store_test.go
+++ b/credstore/store_test.go
@@ -101,6 +101,27 @@ func TestForceFileSkipsProbe(t *testing.T) {
 	assert.JSONEq(t, `{"token":"abc123"}`, string(data))
 }
 
+// A healthy probe keeps the keyring: the store uses it, reports no probe
+// error, and has nothing to warn about. Zero-value options probe unbounded
+// — the timeout reaches the probe as zero — which is the documented contract
+// for tests that mock the keyring (see StoreOptions.ProbeTimeout).
+func TestHealthyProbeUsesKeyring(t *testing.T) {
+	probed := false
+	stubProbe(t, func(serviceName string, timeout time.Duration) error {
+		probed = true
+		assert.Equal(t, "test", serviceName)
+		assert.Zero(t, timeout)
+		return nil
+	})
+
+	store := NewStore(StoreOptions{ServiceName: "test", FallbackDir: t.TempDir()})
+
+	assert.True(t, probed)
+	assert.True(t, store.UsingKeyring())
+	assert.NoError(t, store.ProbeError())
+	assert.Empty(t, store.FallbackWarning())
+}
+
 func TestProbeTimeoutFallsBackToFile(t *testing.T) {
 	dir := t.TempDir()
 	stubProbe(t, func(serviceName string, timeout time.Duration) error {