test(keychain): isolate real-keychain dedup tests and make unlock/cleanup reliable

The real-keychain dedup tests shared the com.test.test/test service namespace
with TestKeychain. When their cleanup failed, the leftover credential leaked into
the shared collection and broke TestKeychain (list_all_credentials saw an extra
entry; filter_credentials failed to unmarshal the seed's secret). Cleanup failed
because purgeRealItems deleted without unlocking the collection, and the seed
itself intermittently hit "Cannot create an item in a locked collection": the
first Unlock in a run can return before the daemon has actually unlocked, so an
immediate CreateItem/DeleteItem races and fails.

- Give the dedup tests their own service group/name (com.test.dedup/dedup) so
  their items are namespace-isolated; a leak can no longer reach TestKeychain's
  GetAllMetadata/Filter.
- Add ensureUnlocked, which unlocks then polls IsLocked until the daemon reports
  the collection unlocked, closing the unlock race for the direct-to-daemon seed
  and purge paths.
- Make purgeRealItems unlock first and delete all matches, asserting success so a
  silent cleanup failure surfaces as a test failure rather than a cross-test leak.
- Seed items with a valid username:password secret so seeded items are
  well-formed.

Refs: docker/secrets-engine-private#446

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: Benehiko

store/keychain/keychain_linux_test.go

@@ -180,6 +180,30 @@ func TestKeychainSaveCollapsesDuplicatesInPlace(t *testing.T) {
 		"the remaining duplicates must be collapsed, leaving only the first match")
 }
 
+// The real-keychain dedup tests use their own service group/name so their items
+// are namespace-isolated from TestKeychain (which shares com.test.test/test).
+// GetAllMetadata/Filter search by {service:group, service:name}, so a leaked
+// dedup item can never show up in — and break — the shared suite.
+const (
+	dedupServiceGroup = "com.test.dedup"
+	dedupServiceName  = "dedup"
+)
+
+// ensureUnlocked unlocks the collection and waits until the daemon actually
+// reports it unlocked. The freedesktop Unlock call can return before the
+// collection is fully unlocked, so a CreateItem/DeleteItem issued immediately
+// after can still fail with "locked collection" — polling IsLocked closes that
+// race. (The store's own Save/Get/Delete avoid it only because the collection
+// stays unlocked once any earlier operation has unlocked it.)
+func ensureUnlocked(t *testing.T, svc *kc.SecretService, collection dbus.ObjectPath) {
+	t.Helper()
+	require.NoError(t, svc.Unlock([]dbus.ObjectPath{collection}))
+	require.Eventually(t, func() bool {
+		locked, err := svc.IsLocked(collection)
+		return err == nil && !locked
+	}, 5*time.Second, 100*time.Millisecond, "collection did not unlock")
+}
+
 // searchRealItems queries a live Secret Service for every item sharing id's
 // stable identity triple {service:group, service:name, id}. The store API keys
 // results by ID and so would collapse duplicates into one logical entry —
@@ -251,15 +275,14 @@ func seedRealDuplicates(t *testing.T, serviceGroup, serviceName string, id store
 	collection, err := getDefaultCollection(svc)
 	require.NoError(t, err)
 
-	// Unlock the collection first — talking to the daemon directly skips the
-	// unlock the store does internally, and gnome-keyring reports even the
-	// passwordless 'login' collection as locked until then. Unlock is a no-op on
-	// an already-unlocked collection.
-	require.NoError(t, svc.Unlock([]dbus.ObjectPath{collection}))
+	// Talking to the daemon directly skips the unlock the store does internally,
+	// and gnome-keyring reports even the passwordless 'login' collection as
+	// locked until then.
+	ensureUnlocked(t, svc, collection)
 
 	label := serviceGroup + ":" + serviceName + ":" + id.String()
 	for i := range n {
-		sessSecret, err := session.NewSecret(fmt.Appendf(nil, "seed-secret-%d", i))
+		sessSecret, err := session.NewSecret(fmt.Appendf(nil, "seed-user:seed-pass-%d", i))
 		require.NoError(t, err)
 
 		attrs := map[string]string{
@@ -276,19 +299,26 @@ func seedRealDuplicates(t *testing.T, serviceGroup, serviceName string, id store
 }
 
 // purgeRealItems removes every item for id, draining any leftover duplicates so
-// a failed assertion cannot leak state into another test. Best-effort: a delete
-// that fails should not mask the test's own failure.
+// the test cannot leak state. It unlocks the collection first (DeleteItem fails
+// on a locked collection) and deletes all matches, asserting success so a silent
+// cleanup failure surfaces as a leak rather than corrupting a later test.
 func purgeRealItems(t *testing.T, serviceGroup, serviceName string, id store.ID) {
 	t.Helper()
-	items := findRealItems(t, serviceGroup, serviceName, id)
-	if len(items) == 0 {
-		return
-	}
 	svc, err := kc.NewService()
 	require.NoError(t, err)
 	defer func() { _ = svc.Close() }()
+
+	collection, err := getDefaultCollection(svc)
+	require.NoError(t, err)
+	ensureUnlocked(t, svc, collection)
+
+	attrs := map[string]string{}
+	safelySetMetadata(serviceGroup, serviceName, attrs)
+	safelySetID(id, attrs)
+	items, err := svc.SearchCollection(collection, attrs)
+	require.NoError(t, err)
 	for _, item := range items {
-		_ = svc.DeleteItem(item)
+		require.NoError(t, svc.DeleteItem(item))
 	}
 }
 
@@ -297,18 +327,14 @@ func purgeRealItems(t *testing.T, serviceGroup, serviceName string, id store.ID)
 // identity, a single Save must update one item in place and delete the rest,
 // leaving exactly one item holding the latest secret.
 func TestKeychainCollapsesExistingDuplicates(t *testing.T) {
-	const (
-		serviceGroup = "com.test.test"
-		serviceName  = "test"
-	)
-	id := store.MustParseID("com.test.test/test/dup-backlog")
-	t.Cleanup(func() { purgeRealItems(t, serviceGroup, serviceName, id) })
+	id := store.MustParseID(dedupServiceGroup + "/" + dedupServiceName + "/backlog")
+	t.Cleanup(func() { purgeRealItems(t, dedupServiceGroup, dedupServiceName, id) })
 
 	// Pre-existing backlog: three duplicate items for one identity.
-	seedRealDuplicates(t, serviceGroup, serviceName, id, 3)
-	require.Len(t, findRealItems(t, serviceGroup, serviceName, id), 3, "precondition: three duplicates seeded")
+	seedRealDuplicates(t, dedupServiceGroup, dedupServiceName, id, 3)
+	require.Len(t, findRealItems(t, dedupServiceGroup, dedupServiceName, id), 3, "precondition: three duplicates seeded")
 
-	ks, err := New(serviceGroup, serviceName, func(_ context.Context, _ store.ID) store.Secret {
+	ks, err := New(dedupServiceGroup, dedupServiceName, func(_ context.Context, _ store.ID) store.Secret {
 		return &mocks.MockCredential{}
 	})
 	require.NoError(t, err)
@@ -318,7 +344,7 @@ func TestKeychainCollapsesExistingDuplicates(t *testing.T) {
 		Password: "final-password",
 	}))
 
-	requireItemCount(t, serviceGroup, serviceName, id, 1,
+	requireItemCount(t, dedupServiceGroup, dedupServiceName, id, 1,
 		"a single Save must collapse the duplicate backlog to one item")
 
 	got, err := ks.Get(t.Context(), id)
@@ -332,14 +358,10 @@ func TestKeychainCollapsesExistingDuplicates(t *testing.T) {
 // metadata that changes on every save (mimicking volatile JWT claims) must keep
 // exactly one item, never minting duplicates.
 func TestKeychainSaveDoesNotAccumulate(t *testing.T) {
-	const (
-		serviceGroup = "com.test.test"
-		serviceName  = "test"
-	)
-	id := store.MustParseID("com.test.test/test/no-accumulate")
-	t.Cleanup(func() { purgeRealItems(t, serviceGroup, serviceName, id) })
-
-	ks, err := New(serviceGroup, serviceName, func(_ context.Context, _ store.ID) store.Secret {
+	id := store.MustParseID(dedupServiceGroup + "/" + dedupServiceName + "/no-accumulate")
+	t.Cleanup(func() { purgeRealItems(t, dedupServiceGroup, dedupServiceName, id) })
+
+	ks, err := New(dedupServiceGroup, dedupServiceName, func(_ context.Context, _ store.ID) store.Secret {
 		return &mocks.MockCredential{}
 	})
 	require.NoError(t, err)
@@ -355,7 +377,7 @@ func TestKeychainSaveDoesNotAccumulate(t *testing.T) {
 		}))
 	}
 
-	requireItemCount(t, serviceGroup, serviceName, id, 1,
+	requireItemCount(t, dedupServiceGroup, dedupServiceName, id, 1,
 		"saving with changing metadata must not accumulate duplicate items")
 
 	got, err := ks.Get(t.Context(), id)