Make keyring availability checks read-only - #165
Conversation
03f855a to
f36ec0f
Compare
There was a problem hiding this comment.
Pull request overview
Makes credential-store initialization read-only, preventing concurrent keyring probes from causing false plaintext fallbacks.
Changes:
- Adds an injectable keyring interface.
- Replaces write/delete availability probes with one read.
- Tests read-only probing and fallback behavior.
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 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
internal/auth/store.go |
Implements read-only keyring availability checks. |
internal/auth/store_test.go |
Verifies probe calls and fallback behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
I independently confirmed this race on macOS with the same shared Two local CLI processes can overlap their probe writes. Keychain then returns a duplicate unique-index error for one process; that process treats Keychain as unavailable, switches to an empty plaintext fallback, and reports I tested a current-code variant that gives every process and Store a unique probe account:
The read-only lookup in this PR is conceptually cleaner because it removes temporary probe writes entirely. This PR now conflicts with current |
What changed
Credential-store initialization now checks Keychain availability with a read-only lookup. A missing availability entry means the keyring is working. Other errors keep the existing plaintext-file fallback.
The keyring interface lets the tests verify that initialization calls
Getonce and never callsSetorDelete.Why
Every CLI process previously wrote and deleted the same
hey::testitem before loading the real credential. Parallel processes could make that probe fail. The affected process then switched to an empty plaintext store and reportednot logged in, even though valid credentials were still in Keychain.I reproduced this with 60 read-only
hey auth status --jsoncalls at concurrency 6. Before the change, 25 printed the keyring warning and reported unauthenticated. After the change, all 60 completed without either failure.Checks
env GOWORK=off mise x golangci-lint@2.10.1 -- make checkenv GOWORK=off mise x -- go test -race -count=1 ./internal/authSummary by cubic
Makes keyring availability checks read-only to remove concurrency races that caused false plaintext fallbacks. Previously the CLI wrote/deleted "hey::test"; now it does a single Get on "hey::availability" where
ErrNotFoundmeans the keyring is available, and only other errors fall back to the file store.credentialKeyringinterface with a defaultsystemCredentialKeyringwrapper aroundgithub.com/zalando/go-keyring;Storenow depends on this interface so tests assert exactly oneGetand zeroSet/Deleteduring init.github.com/zalando/go-keyringwiths.keyringacross load/save/delete paths. External behavior is unchanged except fewer keyring warnings and no spurious unauthenticated states under concurrency.Written for commit f36ec0f. Summary will update on new commits.