fix(oauth): sweep both chunk generations when a store regrows

A shrink writes the blob back under the anchor, which replaces the manifest
and takes the per-generation chunk counts with it. From then on nothing can
name the chunks a failed cleanup left behind, so the growth branch's sweep of
the target generation is the only one that will ever reach them — and it only
ever targets family A. A keychain that refused one delete during the shrink
therefore kept a superseded generation of access, ID and refresh tokens
indefinitely, with no way for the user to know.

Sweep the other generation alongside the target, and document that the
reclaim waits for the next growth: a store that shrinks once and never grows
again keeps the residue, which sweeping on every whole write would close at a
cost of 128 `security` invocations per save on macOS.

Also record why Load and Status take the cross-process lock, and give the
missing-chunk error the same "log in again" advice the digest failure carries.

Refs #937

Author: euxaristia

internal/oauth/store.go

@@ -579,7 +579,7 @@ func (b keyringBlob) read() ([]byte, bool, error) {
 			return nil, false, err
 		}
 		if !ok {
-			return nil, false, fmt.Errorf("oauth: keyring token blob at %s is missing chunk %d of %d", b.location(), index+1, count)
+			return nil, false, fmt.Errorf("oauth: keyring token blob at %s is missing chunk %d of %d; the entries are incomplete, so log in again", b.location(), index+1, count)
 		}
 		encoded.WriteString(strings.TrimSpace(part))
 	}
@@ -613,6 +613,14 @@ func (b keyringBlob) write(data []byte) error {
 // writeWhole stores the blob under the anchor account, the layout every backend
 // without a size limit uses and the one a shrinking store returns to. The Set
 // is the commit point; the chunks it retires are deleted after it lands.
+//
+// A delete that fails here leaves residue the manifest can no longer describe,
+// because the anchor now holds the blob rather than the counts. Nothing
+// reclaims it until the store next outgrows a single entry, where writeChunked
+// sweeps both generations; a store that shrinks once and never grows again
+// keeps it. Sweeping on every whole write would close that, but it costs a
+// keyringMaxChunks-wide probe per save — 128 `security` invocations on macOS —
+// for residue that only an already-failed delete can produce.
 func (b keyringBlob) writeWhole(encoded string, previous keyringManifest) error {
 	if err := b.kr.Set(b.service, b.account, encoded); err != nil {
 		return err
@@ -651,11 +659,24 @@ func (b keyringBlob) writeChunked(data []byte, encoded string, previous keyringM
 	// commit below.
 	if previous.live == "" {
 		// Nothing to reserve against: the anchor still holds the whole blob, so
-		// writing a manifest here would destroy the only copy. Sweep the target
-		// generation instead. This runs once, when a store first outgrows a
+		// writing a manifest here would destroy the only copy. Sweep both
+		// generations instead. This runs once, when a store first outgrows a
 		// single entry, and only has anything to find if an earlier shrink was
 		// interrupted before its cleanup finished.
-		if err := b.deleteChunkRange(family, count, keyringMaxChunks, nil); err != nil {
+		//
+		// Both, not just the target: writeWhole replaced the anchor with the
+		// blob, so the counts that named the chunks a failed cleanup left
+		// behind are gone. The manifest cannot state the range any more and no
+		// later write derives it, so this is the only sweep that will ever
+		// reach them. Sweeping just the target would leave the other
+		// generation's chunks, and the token material in them, unreferenced
+		// for good.
+		other := keyringChunkFamilyB
+		if family == keyringChunkFamilyB {
+			other = keyringChunkFamilyA
+		}
+		err := b.deleteChunkRange(family, count, keyringMaxChunks, nil)
+		if err = b.deleteChunkRange(other, 0, keyringMaxChunks, err); err != nil {
 			return err
 		}
 	} else if count > previous.counts[family] {
@@ -739,6 +760,13 @@ func (b keyringBlob) chunkAccount(family string, index int) string {
 // processes can't both read the blob, modify, and write — dropping a token.
 // The chunked layout needs it for a second reason: it makes a write's chunk
 // fills and its manifest commit one indivisible sequence to other processes.
+//
+// Readers (Load, Status) take it as well, which is deliberate and not a
+// leftover: the generational layout already hands them a consistent view
+// without it, but holding it keeps a read from observing a torn manifest on a
+// backend whose Set is not atomic, and it bounds a reader behind at most one
+// writer. acquireFileLock reclaims after fileLockStaleAfter, so a crashed
+// holder cannot wedge the hot path.
 func (b keyringBlob) withLock(now func() time.Time, fn func() error) error {
 	if b.lockPath == "" {
 		return fn()

internal/oauth/store_keyring_chunked_test.go

@@ -564,3 +564,62 @@ func TestStoreKeyringReadSerializedWithLockDuringChunkedWrite(t *testing.T) {
 		t.Fatal("reader.Status timed out waiting for lock release")
 	}
 }
+
+// TestStoreKeyringShrinkResidueIsReclaimedOnRegrowth is the regression for a
+// retired generation orphaned permanently. Cleanup is hygiene rather than
+// correctness only while a manifest exists to state the counts; writeWhole
+// replaces the anchor with the blob and the counts go with it. A shrink whose
+// delete of the retired generation fails therefore leaves chunks that the next
+// growth would never sweep, because that growth only ever targets family A.
+// The token material in them would sit in the keychain for good.
+func TestStoreKeyringShrinkResidueIsReclaimedOnRegrowth(t *testing.T) {
+	kr := newCappedFakeKR(macOSLikeBudget)
+	s := newCappedKeyringStore(t, kr)
+
+	// Grow until family B is the live generation, so a shrink from here is the
+	// one that retires it.
+	mustSave(t, s, "first", bigToken("a"))
+	mustSave(t, s, "second", bigToken("b"))
+	mustSave(t, s, "third", bigToken("c"))
+	if live := manifestOf(t, kr).live; live != keyringChunkFamilyB {
+		t.Fatalf("live generation = %q, want %q", live, keyringChunkFamilyB)
+	}
+
+	// Shrink back to a whole entry with a keychain that refuses to remove
+	// family B, the generation being retired on the way down.
+	kr.failDelete = func(account string) error {
+		if strings.HasPrefix(account, keyringAccount+"."+keyringChunkFamilyB+".") {
+			return errors.New("keychain busy")
+		}
+		return nil
+	}
+	for _, key := range []string{"second", "third"} {
+		if _, err := s.Delete(ProviderKey(key)); err != nil && !strings.Contains(err.Error(), "could not be removed") {
+			t.Fatalf("Delete(%s): %v", key, err)
+		}
+	}
+	kr.failDelete = nil
+	if head, _, _ := kr.Get(keyringService, keyringAccount); strings.HasPrefix(head, keyringManifestPrefix) {
+		t.Fatal("store did not shrink back to a whole entry, so the orphaning path was never taken")
+	}
+	orphans := kr.chunkAccounts(keyringChunkFamilyB)
+	if len(orphans) == 0 {
+		t.Fatal("setup did not strand any family-B chunks")
+	}
+
+	// Grow back into the chunked layout. This is the only sweep that can still
+	// reach the stranded generation, because the manifest that named it is gone.
+	mustSave(t, s, "fourth", bigToken("d"))
+
+	manifest := manifestOf(t, kr)
+	if manifest.live != keyringChunkFamilyA {
+		t.Fatalf("live generation after regrowth = %q, want %q", manifest.live, keyringChunkFamilyA)
+	}
+	if got := kr.chunkAccounts(keyringChunkFamilyB); len(got) != 0 {
+		t.Errorf("family B still holds %d unreferenced chunks after regrowth: %v", len(got), got)
+	}
+	assertNoStrayChunks(t, kr, manifest)
+	if got := mustLoad(t, s, "fourth"); got.AccessToken != bigToken("d").AccessToken {
+		t.Error("token stored across the regrowth did not survive")
+	}
+}

internal/oauth/store_keyring_test.go

@@ -24,6 +24,9 @@ type fakeKR struct {
 	// sets counts successful writes per account, so a test can assert the order
 	// a write publishes in.
 	sets map[string]int
+	// failDelete, when non-nil, decides whether removing account fails, standing
+	// in for a keychain that refuses a delete while it is busy or locked.
+	failDelete func(account string) error
 }
 
 func newFakeKR() *fakeKR { return &fakeKR{data: map[string]string{}, sets: map[string]int{}} }
@@ -54,6 +57,11 @@ func (f *fakeKR) Set(service, account, secret string) error {
 	return nil
 }
 func (f *fakeKR) Delete(service, account string) (bool, error) {
+	if f.failDelete != nil {
+		if err := f.failDelete(account); err != nil {
+			return false, err
+		}
+	}
 	key := service + "/" + account
 	_, ok := f.data[key]
 	delete(f.data, key)