hold/gc: name the third ownership state instead of encoding it as a lie

264d332 fixed the behaviour but encoded it badly. manifestBelongsToHold returned (true, false) for an unreachable hold — "yes, but not really" — a return value that contradicts itself, and isPredecessorHold both applied the fail-open policy and handed back the raw material for that policy. The behaviour was right and the shape was wrong. The underlying problem is that ownership has three states and the return type had two: ours - this hold's manifest, or a confirmed predecessor's not ours - the hold answered, and it is someone else's unknown - the hold did not answer; don't delete, but do not adopt For the first two, "is it ours" and "should its blobs stay referenced" have the same answer, so one bool worked and the design was never stressed. They diverge only on unknown. Every version so far has had to collapse unknown onto one of the other two: before 95d4f7c onto "not ours", which deleted a live predecessor's blobs, and after it onto "ours", which adopted foreign manifests and put ten phantom missing layer records on hold01. Same shape error, opposite sides. That conflation is original, not something 95d4f7c introduced: manifestBelongsToHold has fed knownManifests since the function was written. So name the state. manifestClaim has three values, classifyManifest and classifyPredecessorHold report what they found and apply no policy, and the one decision that matters — an unknown claim is carried for blob protection but never adopted — now sits in the open at the call site instead of two functions deep, which is how it leaked into ownership to begin with. No behaviour change from 264d332; the three outcomes and the blob protection are identical. The regression test was re-verified against this shape: it fails, reporting the adoption, when claimUnknown is allowed to adopt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KWoKzpgtBJ33sCyGxJGR7x
2026-09-02 08:16:57 +00:00 · 2026-09-01 10:41:04 -05:00
parent 264d332bbd
commit 2a58ccebd8
3 changed files with 119 additions and 67 deletions
@@ -280,13 +280,50 @@ type GCResult struct {
 }

 // manifestInfo holds a parsed manifest fetched from a user's PDS
+// manifestClaim is what a manifest's hold field tells us about ownership. The
+// domain genuinely has three states, and collapsing it into a bool is what put
+// phantom layer records on hold01: "we could not reach the hold" is not the same
+// statement as either "ours" or "not ours", and the two questions this drives —
+// keep the blobs, and adopt the manifest — want different answers for it.
+type manifestClaim int
+
+const (
+	// claimNotOurs: the hold answered and this manifest is another hold's, or
+	// there is no hold to ask. Ignore it entirely.
+	claimNotOurs manifestClaim = iota
+	// claimOurs: this hold's own manifest, or one belonging to a confirmed
+	// predecessor. Adopt it and reference its blobs.
+	claimOurs
+	// claimUnknown: the hold did not answer, so ownership is unknowable. Don't
+	// delete, but do not adopt. Its blobs stay referenced because an outage must
+	// never make content deletable, while the manifest stays out of
+	// knownManifests because adopting it reports every layer as a missing layer
+	// record and lets reconcileMissingRecords write io.atcr.hold.layer records
+	// claiming another hold's content.
+	//
+	// This is not always transient. A dev push leaves manifests naming
+	// did:web:localhost:8080 or an RFC1918 address, which can never resolve from
+	// a server, so this state can be permanent for a given hold.
+	claimUnknown
+)
+
+func (c manifestClaim) String() string {
+	switch c {
+	case claimOurs:
+		return "ours"
+	case claimUnknown:
+		return "unknown"
+	default:
+		return "not-ours"
+	}
+}
+
 type manifestInfo struct {
 	URI     string                  // AT-URI of the manifest
 	UserDID string                  // DID of the user who owns it
 	Record  *atproto.ManifestRecord // Parsed manifest data
-	// ProtectOnly marks a manifest whose owning hold could not be reached, so
-	// whether it is ours is unknown. Its blobs stay referenced, but it is not
-	// adopted into knownManifests. See the ProtectOnly branch in analyzeRecords.
+	// ProtectOnly marks a claimUnknown manifest: reference its blobs, but do not
+	// adopt it. See the ProtectOnly branch in analyzeRecords.
 	ProtectOnly bool
 }

@@ -1783,13 +1820,25 @@ func (gc *GarbageCollector) fetchUserManifestsFromEndpoint(ctx context.Context,
 				continue
 			}

-			if belongs, definitive := gc.manifestBelongsToHold(ctx, &manifest, holdDID); belongs {
+			// The fail-open decision lives here, in the open, rather than inside
+			// the classifier: an unknown claim is carried so its blobs stay
+			// referenced, but marked so it is never adopted as ours.
+			switch gc.classifyManifest(ctx, &manifest, holdDID) {
+			case claimOurs:
+				manifests = append(manifests, &manifestInfo{
+					URI:     rec.URI,
+					UserDID: userDID,
+					Record:  &manifest,
+				})
+			case claimUnknown:
 				manifests = append(manifests, &manifestInfo{
 					URI:         rec.URI,
 					UserDID:     userDID,
 					Record:      &manifest,
-					ProtectOnly: !definitive,
+					ProtectOnly: true,
 				})
+			case claimNotOurs:
+				// Definitively another hold's: nothing to keep and nothing to protect.
 			}
 		}

@@ -1802,70 +1851,73 @@ func (gc *GarbageCollector) fetchUserManifestsFromEndpoint(ctx context.Context,
 	return manifests, nil
 }

-// manifestBelongsToHold checks if a manifest references this hold via HoldDID,
-// legacy HoldEndpoint, or a predecessor hold that has been migrated.
-//
-// The second return value reports whether that answer is definitive. It is false
-// only when the manifest's hold could not be reached, and "unreachable" is not the
-// same claim as "ours": the blobs must stay referenced, because an outage must
-// never make content deletable, but the manifest must not be adopted. Adopting it
-// reports every one of its layers as a missing layer record and lets Reconcile
-// write ownership records for another hold's content. A dev push to a hold that
-// can never resolve — did:web:localhost:8080, an RFC1918 address — is permanently
-// unreachable rather than briefly down, so this is not a transient state that
-// clears itself. Callers must not read an indefinite answer as ownership; see the
-// ProtectOnly branch in analyzeRecords.
-func (gc *GarbageCollector) manifestBelongsToHold(ctx context.Context, manifest *atproto.ManifestRecord, holdDID string) (belongs, definitive bool) {
+// classifyManifest reports what a manifest's hold field says about ownership,
+// via HoldDID, legacy HoldEndpoint, or a predecessor hold that has been migrated.
+// It states what it found and applies no policy; the caller decides what an
+// unknown claim is worth. See manifestClaim.
+func (gc *GarbageCollector) classifyManifest(ctx context.Context, manifest *atproto.ManifestRecord, holdDID string) manifestClaim {
 	manifestHoldDID := manifest.HoldDID

 	// Direct match
 	if manifestHoldDID == holdDID {
-		return true, true
+		return claimOurs
 	}

-	// Legacy: resolve holdEndpoint to DID
+	// Legacy: resolve holdEndpoint to DID.
+	//
+	// A resolve failure here reports claimNotOurs rather than claimUnknown,
+	// preserving long-standing behaviour: this path predates the predecessor
+	// check and fails closed. That is arguably the same shape error in the other
+	// direction — an unresolvable endpoint drops the blobs — but it is a distinct
+	// bug with a distinct blast radius and wants its own change.
 	if manifestHoldDID == "" && manifest.HoldEndpoint != "" {
 		resolved, err := atproto.ResolveHoldDID(ctx, manifest.HoldEndpoint)
 		if err != nil {
 			gc.logger.Debug("Failed to resolve hold DID from legacy endpoint", "holdEndpoint", manifest.HoldEndpoint, "error", err)
-			return false, true
+			return claimNotOurs
 		}
 		manifestHoldDID = resolved
 		if manifestHoldDID == holdDID {
-			return true, true
+			return claimOurs
 		}
 	}

 	if manifestHoldDID == "" {
-		return false, true
+		return claimNotOurs
 	}

 	// Check if the manifest's hold is a predecessor (has a successor label set)
-	return gc.isPredecessorHold(ctx, manifestHoldDID)
+	return gc.classifyPredecessorHold(ctx, manifestHoldDID)
 }

-// isPredecessorHold checks if the given holdDID is a predecessor of this hold
-// by fetching its captain record and checking for a successor label.
-// Results are cached to avoid repeated network calls. An inconclusive check
-// reports true — the manifest is kept referenced — matching how this package
-// already treats a user whose PDS cannot be reached: an outage must never be
-// the reason content becomes eligible for deletion.
-func (gc *GarbageCollector) isPredecessorHold(ctx context.Context, holdDID string) (isPredecessor, definitive bool) {
+// classifyPredecessorHold reports whether holdDID is a predecessor of this hold,
+// by fetching its captain record and checking for a successor label that names
+// us. Definitive answers are cached to avoid repeated network calls; an
+// unreachable hold is reported as claimUnknown and deliberately not cached as a
+// negative, since caching it would make one five-second blip permanent for the
+// life of the process.
+//
+// This reports what it found. It does not decide what an unknown claim earns —
+// that policy lives at the call site in fetchUserManifestsFromEndpoint.
+func (gc *GarbageCollector) classifyPredecessorHold(ctx context.Context, holdDID string) manifestClaim {
 	if gc.predecessorCache == nil {
 		gc.predecessorCache = make(map[string]bool)
 	}

 	if cachedPredecessor, cached := gc.predecessorCache[holdDID]; cached {
-		return cachedPredecessor, true
+		if cachedPredecessor {
+			return claimOurs
+		}
+		return claimNotOurs
 	}

-	// Already inconclusive earlier in this run. Answer the same way without
+	// Already unreachable earlier in this run. Answer the same way without
 	// paying another timeout; the next run starts fresh and re-checks.
 	if gc.predecessorUnresolved[holdDID] {
-		return true, false
+		return claimUnknown
 	}

-	isPredecessor, definitive = gc.checkPredecessor(ctx, holdDID)
+	isPredecessor, definitive := gc.checkPredecessor(ctx, holdDID)
 	if !definitive {
 		if gc.predecessorUnresolved == nil {
 			gc.predecessorUnresolved = make(map[string]bool)
@@ -1873,11 +1925,14 @@ func (gc *GarbageCollector) isPredecessorHold(ctx context.Context, holdDID strin
 		gc.predecessorUnresolved[holdDID] = true
 		gc.logger.Warn("GC: predecessor status unresolved, keeping hold's blobs referenced but not adopting its manifests",
 			"holdDID", holdDID)
-		return true, false
+		return claimUnknown
 	}

 	gc.predecessorCache[holdDID] = isPredecessor
-	return isPredecessor, true
+	if isPredecessor {
+		return claimOurs
+	}
+	return claimNotOurs
 }

 // checkPredecessor fetches a hold's captain record to check if it has a successor label
@@ -200,7 +200,7 @@ func TestConfig(t *testing.T) {
 	})
 }

-func TestManifestBelongsToHold(t *testing.T) {
+func TestClassifyManifest(t *testing.T) {
 	gc := &GarbageCollector{
 		logger: newTestLogger(),
 		// Seeded with a definitive answer so the non-matching cases resolve from
@@ -216,35 +216,35 @@ func TestManifestBelongsToHold(t *testing.T) {
 	tests := []struct {
 		name     string
 		manifest *atproto.ManifestRecord
-		want     bool
+		want     manifestClaim
 	}{
 		{
 			name: "matching HoldDID",
 			manifest: &atproto.ManifestRecord{
 				HoldDID: "did:web:hold01.atcr.io",
 			},
-			want: true,
+			want: claimOurs,
 		},
 		{
 			name: "non-matching HoldDID",
 			manifest: &atproto.ManifestRecord{
 				HoldDID: "did:web:other-hold.atcr.io",
 			},
-			want: false,
+			want: claimNotOurs,
 		},
 		{
 			name: "legacy HoldEndpoint matching",
 			manifest: &atproto.ManifestRecord{
 				HoldEndpoint: "https://hold01.atcr.io",
 			},
-			want: true,
+			want: claimOurs,
 		},
 		{
 			name: "legacy HoldEndpoint non-matching",
 			manifest: &atproto.ManifestRecord{
 				HoldEndpoint: "https://other-hold.atcr.io",
 			},
-			want: false,
+			want: claimNotOurs,
 		},
 		{
 			name: "HoldDID takes precedence over endpoint",
@@ -252,23 +252,24 @@ func TestManifestBelongsToHold(t *testing.T) {
 				HoldDID:      "did:web:hold01.atcr.io",
 				HoldEndpoint: "https://other-hold.atcr.io",
 			},
-			want: true,
+			want: claimOurs,
 		},
 		{
 			name:     "empty manifest",
 			manifest: &atproto.ManifestRecord{},
-			want:     false,
+			want:     claimNotOurs,
 		},
 	}

 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, definitive := gc.manifestBelongsToHold(context.Background(), tt.manifest, holdDID)
+			got := gc.classifyManifest(context.Background(), tt.manifest, holdDID)
 			if got != tt.want {
-				t.Errorf("manifestBelongsToHold() = %v, want %v", got, tt.want)
+				t.Errorf("classifyManifest() = %v, want %v", got, tt.want)
 			}
-			if !definitive {
-				t.Errorf("manifestBelongsToHold() definitive = false, want true: no hold lookup is involved in this case")
+			if got == claimUnknown {
+				t.Errorf("classifyManifest() = unknown: every case here resolves from cache or " +
+					"the DID field, so an unknown means a real network call leaked into the test")
 			}
 		})
 	}
@@ -288,17 +289,17 @@ func TestPredecessorUnresolvedStaysReferenced(t *testing.T) {
 	const unreachable = "did:web:unreachable.invalid"
 	manifest := &atproto.ManifestRecord{HoldDID: unreachable}

-	belongs, definitive := gc.manifestBelongsToHold(context.Background(), manifest, holdDID)
-	if !belongs {
-		t.Fatal("unreachable hold treated as not-a-predecessor: its blobs would be deleted")
-	}
-
-	// Blobs stay referenced, but the answer must not masquerade as ownership:
-	// analyzeRecords keys its ProtectOnly branch off this, and adopting an
-	// unreachable hold's manifest reports phantom missing layer records and lets
+	// Don't delete, but do not adopt: claimUnknown is the only answer that keeps
+	// the blobs referenced without asserting the manifest is ours. claimNotOurs
+	// would drop its blobs out of the referenced set; claimOurs would adopt it
+	// into knownManifests, reporting phantom missing layer records and letting
 	// Reconcile write ownership records for another hold's content.
-	if definitive {
-		t.Error("unreachable hold reported as a definitive answer: its manifests would be adopted into knownManifests")
+	claim := gc.classifyManifest(context.Background(), manifest, holdDID)
+	if claim == claimNotOurs {
+		t.Fatal("unreachable hold treated as not-ours: its blobs would be deleted")
+	}
+	if claim != claimUnknown {
+		t.Errorf("unreachable hold classified %v, want unknown: anything else asserts ownership we cannot verify", claim)
 	}

 	if _, cached := gc.predecessorCache[unreachable]; cached {
@@ -310,12 +311,8 @@ func TestPredecessorUnresolvedStaysReferenced(t *testing.T) {
 	}

 	// Second call must be answered from predecessorUnresolved, not re-dialled.
-	belongs, definitive = gc.manifestBelongsToHold(context.Background(), manifest, holdDID)
-	if !belongs {
-		t.Error("second call disagreed with the first")
-	}
-	if definitive {
-		t.Error("cached inconclusive answer reported as definitive")
+	if again := gc.classifyManifest(context.Background(), manifest, holdDID); again != claimUnknown {
+		t.Errorf("second call classified %v, want unknown: it disagreed with the first", again)
 	}
 }

@@ -35,7 +35,7 @@ func captainResponse(t *testing.T, successor string) string {
 // TestCheckPredecessorAt_InconclusiveOnTransportFailure pins the distinction
 // 95d4f7c introduced: a hold that could not be asked is not a hold that
 // answered "no". Every one of these paths used to return a bare false, which
-// isPredecessorHold recorded in a process-lifetime cache — so a single blip
+// classifyPredecessorHold recorded in a process-lifetime cache — so a single blip
 // dropped that hold's manifests out of the referenced set and deleted blobs
 // this hold is still serving on the predecessor's behalf.
 //