fix(keychain): close D-Bus connection after each Linux operation#542
Merged
1 commit merged intoJun 2, 2026
Merged
Conversation
On Linux every keychain operation (Get/Save/Delete/GetAllMetadata/Filter) dials a fresh private session-bus connection via NewService -> dbus.ConnectSessionBus, but only closed the secret-service session, never the connection itself. Each call therefore leaked one socket file descriptor, and long-lived processes eventually exhausted the session bus's max_connections_per_user limit (failing with "maximum number of active connections has been reached"). Add SecretService.Close to release the connection and defer it in every operation. A regression test asserts the process's open-fd count stays flat across repeated lookups. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Benehiko
reviewed
Jun 2, 2026
Comment on lines
+32
to
+37
| func openFDCount(t *testing.T) int { | ||
| t.Helper() | ||
| entries, err := os.ReadDir("/proc/self/fd") | ||
| require.NoError(t, err) | ||
| return len(entries) | ||
| } |
Member
There was a problem hiding this comment.
Let's drop this test. I don't like depending on assertions that depend on the OS environment staying a certain way. We most definitely will see flakes on this.
| // secret. That keeps the test focused on the connection lifecycle — the thing | ||
| // the fix changes — without creating, reading, or deleting shared keyring | ||
| // items (which is both stateful and prone to gnome-keyring item-lock quirks). | ||
| func TestKeychainDoesNotLeakConnections(t *testing.T) { |
Member
There was a problem hiding this comment.
Drop these tests - I have a follow-up PR which will introduce mock service which at least helps against regressions #543
Benehiko
added a commit
that referenced
this pull request
Jun 2, 2026
The Linux keychain store called the concrete *kc.SecretService directly and reached into raw dbus.BusObject values (ServiceObj/Obj) inside its helpers, so the only way to test the store was against a live secret service over D-Bus. Introduce a narrow secretService interface that the store depends on, and a package-level newService seam tests can swap. To keep the interface free of dbus.BusObject (which would force a fake to talk to the bus), push the three low-level property/alias lookups down into the secretservice package as high-level methods: Collections, ReadAlias and IsLocked. getDefaultCollection and isCollectionUnlocked now call those instead of GetProperty/Call. Add a pure in-memory fakeService and two tests that drive the store without a keyring: a not-found path test and a deterministic connection-close regression test (asserts every opened connection is closed), replacing the need to count host file descriptors. NOTE: depends on the SecretService.Close method added in #542; this branch does not compile against main until that lands. Once #542 merges and this rebases on it, the close-balance test verifies the leak fix directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Benehiko
added a commit
that referenced
this pull request
Jun 2, 2026
The Linux keychain store called the concrete *kc.SecretService directly and reached into raw dbus.BusObject values (ServiceObj/Obj) inside its helpers, so the only way to test the store was against a live secret service over D-Bus. Introduce a narrow secretService interface that the store depends on, and a package-level newService seam tests can swap. To keep the interface free of dbus.BusObject (which would force a fake to talk to the bus), push the three low-level property/alias lookups down into the secretservice package as high-level methods: Collections, ReadAlias and IsLocked. getDefaultCollection and isCollectionUnlocked now call those instead of GetProperty/Call. Add a pure in-memory fakeService and two tests that drive the store without a keyring: a not-found path test and a deterministic connection-close regression test (asserts every opened connection is closed), replacing the need to count host file descriptors. NOTE: depends on the SecretService.Close method added in #542; this branch does not compile against main until that lands. Once #542 merges and this rebases on it, the close-balance test verifies the leak fix directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 participants
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
On Linux, every keychain operation leaked one socket file descriptor. Long-lived
processes (e.g. a daemon that reads credentials periodically) accumulated these
until they hit the session bus's
max_connections_per_userlimit and could nolonger open any connection, failing with "The maximum number of active
connections has been reached". This closes the connection after each operation.
What changed
SecretService.Close()instore/keychain/internal/go-keychain/secretservice/secretservice.go, which closes the underlying*dbus.Conn. It's nil-guarded, idempotent (godbus closes the connection once), and tearing down the connection also stops the signal goroutine thatNewServicestarts.service.Close()immediately afterNewService()in all five Linux ops —Get,Save,Delete,GetAllMetadata,Filter(store/keychain/keychain_linux.go). It's deferred beforeCloseSession, so by LIFO the session (which makes a live D-Bus call) closes first and the connection last. This also covers a path that previously leaked whenOpenSessionfailed afterNewServicesucceeded.TestKeychainDoesNotLeakConnections(store/keychain/keychain_linux_test.go,//go:build linux).Why
Each Linux op opens a fresh private session-bus connection via
NewService()→dbus.ConnectSessionBus(), but the code only deferredCloseSession(session),which releases the secret-service session object — not the underlying
*dbus.Conn. The connection (and its fd) stayed open for the lifetime of theprocess, one per operation.
Scope is Linux-only. The darwin and windows backends use native OS APIs (no
D-Bus) and were never affected.
Testing
DOCKER_TARGET=ubuntu-24-gnome-keyring make keychain-linux-ci-unit-tests— pass.DOCKER_TARGET=fedora-43-gnome-keyring make keychain-linux-ci-unit-tests— pass./proc/self/fdacross 30 lookups of anon-existent id. That id drives the full connection setup (
NewService→OpenSession→ resolve collection → search) but returnsErrCredentialNotFoundbefore fetching a secret, so it exercises the connection lifecycle without
mutating shared keyring state. It asserts the open-fd count stays flat.
over 30 lookups", confirming a true red-before / green-after regression test.