Give each process its own keyring probe item, and say when the keyring fell back to file

credstore/probe.go

@@ -2,6 +2,10 @@ package credstore
 
 import (
 	"context"
+	"errors"
+	"fmt"
+	"os"
+	"sync/atomic"
 	"time"
 
 	"github.com/zalando/go-keyring"
@@ -14,34 +18,39 @@ 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 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 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."
-	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).
+// 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.
 var (
 	keyringSet    = keyring.Set
-	keyringGet    = keyring.Get
 	keyringDelete = keyring.Delete
 )
 
@@ -50,49 +59,39 @@ func probeService(serviceName string) string {
 	return probeServicePrefix + serviceName
 }
 
+// probeKey derives a fresh probe account for this process.
+func probeKey() string {
+	return fmt.Sprintf("%s%d.%d", probeKeyPrefix, os.Getpid(), probeSeq.Add(1))
+}
+
 // 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.
+// 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 {
-	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 keyringError(err)
 	}
 	_ = keyringDelete(serviceName, key)
 	return nil

credstore/probe_darwin.go

@@ -3,6 +3,7 @@ package credstore
 import (
 	"context"
 	"encoding/base64"
+	"errors"
 	"fmt"
 	"os/exec"
 	"regexp"
@@ -25,12 +26,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 +45,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 +63,51 @@ 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)
+}
+
+// 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

credstore/probe_darwin_test.go

@@ -3,7 +3,9 @@ package credstore
 import (
 	"context"
 	"os"
+	"os/exec"
 	"path/filepath"
+	"regexp"
 	"strconv"
 	"strings"
 	"syscall"
@@ -108,12 +110,31 @@ 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-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", probeKey)
+
+	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
+// 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,39 +154,21 @@ 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")
 
 	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)
@@ -174,6 +177,31 @@ func TestProbeBoundedNonDuplicateFailureStillFails(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(""))

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) }()
@@ -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 }