Skip to content

fix(keychain): retry the Linux read path when the collection relocks#561

Open
Benehiko wants to merge 1 commit into
mainfrom
fix/keychain-read-relock-retry
Open

fix(keychain): retry the Linux read path when the collection relocks#561
Benehiko wants to merge 1 commit into
mainfrom
fix/keychain-read-relock-retry

Conversation

@Benehiko

Copy link
Copy Markdown
Member

What

#557 made the Linux keychain write path relock-aware (it landed withRelockRetry and wrapped Save's CreateItem/SetItemSecret/duplicate-collapse DeleteItem, and Delete's DeleteItem). The read path was deliberately left for a focused follow-up — this PR.

Get and Filter (via loadSecret) call GetSecret directly. 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 — after IsLocked reports it unlocked but before the GetSecret runs — surfacing an intermittent org.freedesktop.Secret.Error.IsLocked error.

How

Wrap both GetSecret call sites in the withRelockRetry helper already on main:

  • Get → the single-item GetSecret.
  • FilterloadSecret's GetSecret. loadSecret now takes the enclosing collectionPath so 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 if Unlock itself 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. #557 superseded the rest of #560 (its helper, its Delete wrap, and its obsolete pre-#557 Save wrap); only the read-path coverage was outstanding, and that is what this PR adds.

Tests

  • Fake-backed: TestKeychainGetRetriesWhenCollectionRelocks, TestKeychainFilterRetriesWhenCollectionRelocks — two leading locked failures then a success; assert the retry count and one Unlock per retry.
  • Real gnome-keyring: full store/keychain package green on the ubuntu-24-gnome-keyring and fedora-43-gnome-keyring CI harnesses.
  • Lint clean (golangci-lint v2.12.2, all platforms, 0 issues).

🤖 Generated with Claude Code

#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>

@docker-agent docker-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

No bugs found in the changed code.

The PR correctly extends the withRelockRetry helper to both read paths:

  • Get: the single-item GetSecret call is now wrapped in withRelockRetry.
  • Filter / loadSecret: loadSecret now accepts collectionPath so it can re-unlock on relock, and its GetSecret call 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant