fix(github): serialize keychain access to close BUG-052 data race - #287
Merged
Conversation
github/keychain.go called the zalando/go-keyring package's Get/Set/Delete directly with no synchronization, so an AddGitHubAccountWithToken RPC call (write) and UserPRCache's background refresh loop (read, via ListKeychainAccounts/GetAllKeychainTokens) could race on the shared keyring backend, confirmed under go test -race across 4 CI runs on PR #285. Add a package-level sync.Mutex plus keyringGet/keyringSet/keyringDelete wrapper functions and route every keyring.* call in the package through them. The lock lives in the leaf wrappers (not the outer exported functions) to avoid self-deadlock from this package's internal nested calls (e.g. GetKeychainToken -> ListKeychainAccounts, SetKeychainTokenForAccount -> addToAccountList -> ListKeychainAccounts). Adds github/keychain_test.go with two concurrent regression tests, verified to reproduce the original race (and a fatal concurrent-map crash) against the pre-fix wrappers and pass clean with the mutex restored. Moves docs/bugs/open/BUG-052-*.md to docs/bugs/fixed/ with the fix writeup. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W3683CH7Fs9zYR2yP3Dpba
Contributor
✅ Registry ValidationTest Coverage: 22/180 features have
|
Contributor
Go Benchmarks (Tier 1) |
Contributor
📊 Feature E2E CoverageFeature coverage report unavailable
|
Contributor
E2E RPC Latency |
Contributor
Frontend Terminal Throughput |
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
1 participant
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.
Summary
Fixes BUG-052: a data race in
github/keychain.gobetweenSetKeychainTokenForAccount(write, e.g.AddGitHubAccountWithTokenRPC) andListKeychainAccounts/GetAllKeychainTokens(read,UserPRCache's background refresh loop) — confirmed undergo test -raceacross 4 separate CI runs on PR #285.Context
github/keychain.gocalledzalando/go-keyring'sGet/Set/Deletedirectly with no synchronization of its own. The package has two independently-triggerable concurrent callers: RPC handlers andUserPRCache.loop()'s periodicfetch -> resolveAllLogins -> collectAllTokensrefresh. Neither the test-only mock backend nor the real OS backends (macOS Keychain, Secret Service over D-Bus) are guaranteed thread-safe by this dependency, so nothing prevented these two paths racing on shared state.Changes
github/keychain.go: added a package-levelsync.Mutex(keychainMu) and three private wrapper functions (keyringGet,keyringSet,keyringDelete) that guard everykeyring.Get/Set/Deletecall in the file. All 11 directkeyring.*call sites across the package's 8 keyring-touching functions now route through the wrappers. The lock lives in these leaf wrappers (not the outer exported functions) specifically to avoid self-deadlock from this package's internal nested calls (e.g.SetKeychainTokenForAccount→addToAccountList→ListKeychainAccounts).github/keychain_test.go(new): two regression tests exercising the write path concurrently with both read paths from the original race trace, plus the delete path, using goroutines +sync.WaitGroupunder-race.docs/bugs/open/BUG-052-*.md→ moved todocs/bugs/fixed/with the full fix writeup and Phase D reflection.Impact
githubpackage only (keychain access). No RPC/proto/UI changes.Mutexwas chosen overRWMutexsince read-read concurrency has no measurable benefit at this frequency.Testing
go build ./github/... ./server/services/...— cleango test ./github/... ./server/services/... -race— both packages pass (github1.06s,server/services393s)go test ./server/services/... -run TestAddGitHubAccountWithToken_ValidToken_StoresAndReturnsAccount -race -count=5 -v— 5/5 pass, no race warnings (previously flaked under-race)go test ./github/... -run 'TestSetKeychainTokenForAccount_NoRace|TestDeleteKeychainTokenForAccount_NoRace' -race -count=5 -v— 5/5 each, no race warnings-race -count=5— reproduced the original race plus afatal error: concurrent map read and map writecrash; restored the fix, reran cleangolangci-lint run --enable=nilnil,staticcheck,ineffassign,govet ./github/... ./server/services/...— 0 issuesbin/linter ./github/... ./server/services/...) — 0 issuesmake lintfull run — blocked in this dev environment by a pre-existing, unrelatedserver/web/embed.goweb-dist gap (same issue already documented indocs/bugs/fixed/BUG-048-*.md); both lint stages were run directly against the touched packages instead (see above)Reviewer Notes
keyringGet/keyringSet/keyringDelete) doesn't miss anykeyring.*call site — verified viagrep -n "keyring\." github/keychain.goshowing only the three wrapper definitions call the real package now.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com