fix(keychain): retry the Linux read path when the collection relocks#561
Open
Benehiko wants to merge 1 commit into
Open
fix(keychain): retry the Linux read path when the collection relocks#561Benehiko wants to merge 1 commit into
Benehiko wants to merge 1 commit into
Conversation
#557 made the write path relock-aware but left the reads bare. Get and Filter (via loadSecret) call GetSecret directly, which hits the same gnome-keyring relock race the write path did: a prior operation's closing D-Bus connection can relock the collection asynchronously — after the unlock but before the read runs — surfacing an intermittent org.freedesktop.Secret.Error.IsLocked error. Wrap both GetSecret call sites in the withRelockRetry helper already on main, so a relock is unlocked and retried rather than failing the read. loadSecret now takes the enclosing collectionPath so it can re-unlock. Tests (fake-backed): TestKeychainGetRetriesWhenCollectionRelocks and TestKeychainFilterRetriesWhenCollectionRelocks. Verified green against real gnome-keyring on the ubuntu-24 and fedora-43 CI harnesses; lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
No bugs found in the changed code.
The PR correctly extends the withRelockRetry helper to both read paths:
Get: the single-itemGetSecretcall is now wrapped inwithRelockRetry.Filter/loadSecret:loadSecretnow acceptscollectionPathso it can re-unlock on relock, and itsGetSecretcall is similarly wrapped.
The collectionPath parameter is threaded through correctly, defer clear(value) is registered only after a successful withRelockRetry return (matching the pre-existing pattern), and the new tests (TestKeychainGetRetriesWhenCollectionRelocks, TestKeychainFilterRetriesWhenCollectionRelocks) properly exercise two leading locked failures then a success, asserting retry count and one Unlock call per retry. No issues detected.
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.
What
#557made the Linux keychain write path relock-aware (it landedwithRelockRetryand wrappedSave'sCreateItem/SetItemSecret/duplicate-collapseDeleteItem, andDelete'sDeleteItem). The read path was deliberately left for a focused follow-up — this PR.GetandFilter(vialoadSecret) callGetSecretdirectly. That call is subject to the same gnome-keyring relock race the write path was: the store dials a fresh D-Bus connection per operation, and a prior operation's closing connection can relock the collection asynchronously — afterIsLockedreports it unlocked but before theGetSecretruns — surfacing an intermittentorg.freedesktop.Secret.Error.IsLockederror.How
Wrap both
GetSecretcall sites in thewithRelockRetryhelper already onmain:Get→ the single-itemGetSecret.Filter→loadSecret'sGetSecret.loadSecretnow takes the enclosingcollectionPathso it can re-unlock on a relock.No new mechanism — this reuses the exact helper merged in
#557(react to the authoritative locked D-Bus error, bounded exponential backoff, abort immediately ifUnlockitself fails). After this, all lock-gated operations (reads and writes) are uniformly relock-robust.Context
This is the read-path remainder of the now-closed
#560.#557superseded the rest of#560(its helper, itsDeletewrap, and its obsolete pre-#557Savewrap); only the read-path coverage was outstanding, and that is what this PR adds.Tests
TestKeychainGetRetriesWhenCollectionRelocks,TestKeychainFilterRetriesWhenCollectionRelocks— two leading locked failures then a success; assert the retry count and oneUnlockper retry.store/keychainpackage green on theubuntu-24-gnome-keyringandfedora-43-gnome-keyringCI harnesses.golangci-lintv2.12.2, all platforms, 0 issues).🤖 Generated with Claude Code