credstore: don't mistake a lost probe-write race for an unavailable keyring - #69
Merged
Conversation
…eyring Every invocation probes keyring availability by writing and deleting one fixed-name entry (credstore.probe.<service> / __probe__). Two concurrent invocations therefore churn the same keychain item, and on darwin `security add-generic-password -U` is find-then-create inside the security tool: when a peer's delete lands between the find (miss) and the create, the create fails with errSecDuplicateItem (-25299) even though the keychain is perfectly healthy. The losing process then silently fell back to file storage, and a machine whose credentials live in the keychain got "credentials not found for profile:<name>" (auth_required) for profiles that are fully authenticated — intermittently, only under concurrency. Observed in the wild: a long-running connector polling `basecamp` every 15s alongside a second connector failed ~5% of ticks with auth_required while every interactive (serial) invocation succeeded. Two concurrent probe loops against a real keychain reproduce it at 99/200 lost probes; with this change, 0/400. A probe answer that proves the keychain responsive must count as availability, not grounds for the file fallback: - Bounded darwin probe: capture `security` output and treat errSecDuplicateItem as success, then clean up as usual. Matched by the ABI-stable OSStatus (-25299), not the prose message. - Unbounded probe (go-keyring returns a bare exit error with no output): disambiguate a failed Set with a Get. The entry present (peer's probe) or cleanly absent (ErrNotFound, peer cleaned up) proves the keyring answers; only failing both write and read reports unavailability. Real credential entries are immune to this interleaving — they are never delete/add churned, so -U reliably takes its update path. Only the probe's write-then-delete pattern creates the race.
There was a problem hiding this comment.
Pull request overview
Updates keyring probing to avoid false file fallback during concurrent macOS probes.
Changes:
- Handles duplicate-item races in bounded Darwin probes.
- Disambiguates failed unbounded writes with a read.
- Adds regression and cleanup tests.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
credstore/probe.go |
Adds failed-write disambiguation. |
credstore/probe_test.go |
Tests direct probe behavior. |
credstore/probe_darwin.go |
Recognizes duplicate-item responses. |
credstore/probe_darwin_test.go |
Tests Darwin race handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06d7c4a0d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review caught a hole in the unbounded path: recovering on a clean ErrNotFound read meant a read-only keyring — writes rejected, probe entry absent — would be classified available, and every later Save would fail instead of using the documented file fallback. Recovery now demands fresh write evidence: retry the Set once (the churned entry has settled, so a healthy keyring accepts it — present means -U updates in place, absent means a plain create lands), and if the retry also loses, accept only a Get that finds the entry — a same-uid peer's completed write, which is what sustained concurrent-probe churn looks like. Failing both writes with no peer entry reports the original write error. Concurrent tight-loop validation against a real keychain stays at 0/200 failures.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8f60999f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+92
to
+94
| if _, getErr := keyringGet(serviceName, key); getErr != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
When an earlier timed-out or aborted probe has left the deterministic __probe__ entry and the keyring later permits reads but rejects writes, both Set attempts fail while this Get succeeds on the stale entry. The new retry is fresh evidence that the previously reported issue remains: it establishes two failed writes, but cannot establish that a peer wrote the entry moments ago. probeDirect consequently returns success, NewStore selects the keyring, and subsequent Save calls fail instead of using the file fallback; recovery needs a successful write or a specifically classified duplicate-write error rather than mere entry presence.
AGENTS.md reference: AGENTS.md:L28-L32
Useful? React with 👍 / 👎.
jeremy
added a commit
to basecamp/basecamp-cli
that referenced
this pull request
Aug 28, 2026
* Bump github.com/basecamp/cli for the keyring probe race fix Pulls in basecamp/cli#69: concurrent CLI processes could lose the keychain probe-write race and misread the lost write as an unavailable keyring, failing auth with credentials-not-found. go mod tidy raises the go directive to 1.26.7 to match the dependency. * Advance flake.lock and vendorHash for the Go 1.26.7 floor The cli bump raises go.mod's directive to 1.26.7, but flake.lock still pinned a nixpkgs whose go_1_26 was 1.26.5, so the Nix flake could not build. Advance nixpkgs to a revision carrying 1.26.7 and recompute vendorHash for the changed go.sum; verified via make update-nix-hash. * Document the Go 1.26.7 floor and retire the Termux go.mod workaround The pinned github.com/basecamp/cli module requires Go 1.26.7, so lowering go.mod's go line can no longer make an older patch release build.
jeremy
added a commit
that referenced
this pull request
Aug 28, 2026
…g fell back to file (#70) * credstore: give each process its own probe entry, and name the keyring failure on fallback 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: number probe entries per probe, not per process 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: name the keychain failure on the unbounded darwin probe too 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: pin the healthy-probe branch of NewStore 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Symptom
A long-running connector polling
basecampevery 15s intermittently failed with— for profiles that are fully authenticated, with tokens sitting in the macOS keychain. It only happened while a second CLI process was running concurrently (another connector on a different profile); every interactive, serial invocation succeeded. The error string is
loadFromFile's format, which pins the failing invocations to the file fallback — the keyring probe had failed, silently demoting the process to an emptycredentials.json.Mechanism
Every invocation probes keyring availability by writing and deleting one fixed-name entry (
credstore.probe.<service>/__probe__). Two concurrent invocations churn the same keychain item, and on darwinsecurity add-generic-password -Uis find-then-create inside the security tool: when a peer's delete lands between the find (miss) and the create, the create fails witherrSecDuplicateItem (-25299)on a perfectly healthy keychain. The probe treated any write failure as "keyring unavailable" → file fallback →credentials not found for profile:<name>.The existing comment "Concurrent probes sharing the name are harmless: Set results are unaffected" turns out to be false on darwin — only the delete race was harmless.
Real credential entries are immune: they are never delete/add churned, so
-Ureliably takes its update path. Only the probe's write-then-delete pattern creates the race.Repro (real keychain, before/after)
Two concurrent loops mirroring the probe (
add-generic-password -U+delete-generic-passwordon a scratch service): the losing loop fails 199/200 adds with-25299. Sequential-Uadds never fail. Two concurrentprobe()loops via the package itself: 99/200 failures before this change, 0/400 after (both bounded and unbounded paths). End-to-end, two concurrent loops ofbasecamp me -jon different authenticated profiles produced 2/30auth_requiredfailures on an unpatched binary.Fix
A probe answer that proves the keychain responsive counts as availability, not grounds for the file fallback:
securityoutput; treaterrSecDuplicateItemas success and continue to cleanup. Matched by the ABI-stable OSStatus-25299, not the prose message.exec.ExitErrorwith no output to classify): disambiguate a failedSetwith aGet. Entry present (the peer's probe) or cleanly absent (ErrNotFound— the peer already cleaned up) proves the keyring answers; only failing both write and read reports unavailability.make checkandgo test -race ./credstore/...pass; gofmt clean.