hold/gc: stop an unreachable hold's manifests from being adopted

95d4f7c made the predecessor check fail open so a five-second blip against
a live predecessor could not drop its blobs out of the referenced set. That
was right, but the boolean it flipped does two jobs: manifestBelongsToHold
decides both "keep these blobs referenced" and "this manifest is ours", and
for an unreachable hold those want different answers.

The consequence showed up on hold01 the moment it started running this
code. Five stale dev manifests pointing at did:web:localhost%3A8080 and
did:web:172.28.0.3:8080 were adopted into knownManifests, and since hold01
had never stored them, every one of their ten layers was reported as a
missing layer record. Worse than the noise: reconcileMissingRecords acts on
exactly that list, so a Reconcile would have written io.atcr.hold.layer
records asserting hold01 stores blobs for a localhost hold.

These DIDs are loopback and RFC1918, so they can never resolve from a
server. This is not a transient outage that clears itself on the next run.

manifestBelongsToHold and isPredecessorHold now return (value, definitive),
matching the idiom checkPredecessor already uses. An indefinite answer still
carries the manifest so its blobs stay referenced, but marks it ProtectOnly,
and analyzeRecords protects its digests without adding it to knownManifests
— the same shape the in-grace takedown branch above it already uses.

Left alone deliberately: the legacy holdEndpoint path still treats a resolve
failure as a definitive "not ours". That predates 95d4f7c and fails closed
rather than open, so it is a different bug with a different blast radius.

The regression test was verified to fail without the fix, reporting the
adoption rather than a build error.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KWoKzpgtBJ33sCyGxJGR7x

pkg/hold/gc/gc.go

@@ -284,6 +284,10 @@ 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 bool
 }
 
 // analysisResult holds intermediate data from record analysis, shared between Run and Preview
@@ -850,6 +854,23 @@ func (gc *GarbageCollector) analyzeRecords(ctx context.Context) (*analysisResult
 				continue
 			}
 
+			// Ownership was indefinite: the manifest names a hold we could not
+			// reach, so we cannot say it is ours. Protect its blobs exactly as an
+			// in-grace takedown does, but do not adopt it. Adopting would report
+			// every layer as a missing layer record and let reconcileMissingRecords
+			// write io.atcr.hold.layer records claiming another hold's content.
+			if m.ProtectOnly {
+				for _, layer := range m.Record.Layers {
+					result.referenced[layer.Digest] = true
+				}
+				if m.Record.Config != nil && m.Record.Config.Digest != "" {
+					result.referenced[m.Record.Config.Digest] = true
+				}
+				gc.logger.Debug("Manifest hold unreachable: blobs protected, ownership not claimed",
+					"manifest", m.URI, "holdDid", m.Record.HoldDID)
+				continue
+			}
+
 			knownManifests[m.URI] = m
 			if d := extractDigestFromManifestURI(m.URI); d != "" {
 				knownDigests[d] = true
@@ -1762,11 +1783,12 @@ func (gc *GarbageCollector) fetchUserManifestsFromEndpoint(ctx context.Context,
 				continue
 			}
 
-			if gc.manifestBelongsToHold(ctx, &manifest, holdDID) {
+			if belongs, definitive := gc.manifestBelongsToHold(ctx, &manifest, holdDID); belongs {
 				manifests = append(manifests, &manifestInfo{
-					URI:     rec.URI,
+					URI:         rec.URI,
-					UserDID: userDID,
+					UserDID:     userDID,
-					Record:  &manifest,
+					Record:      &manifest,
+					ProtectOnly: !definitive,
 				})
 			}
 		}
@@ -1782,12 +1804,23 @@ func (gc *GarbageCollector) fetchUserManifestsFromEndpoint(ctx context.Context,
 
 // manifestBelongsToHold checks if a manifest references this hold via HoldDID,
 // legacy HoldEndpoint, or a predecessor hold that has been migrated.
-func (gc *GarbageCollector) manifestBelongsToHold(ctx context.Context, manifest *atproto.ManifestRecord, holdDID string) bool {
+//
+// 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) {
 	manifestHoldDID := manifest.HoldDID
 
 	// Direct match
 	if manifestHoldDID == holdDID {
-		return true
+		return true, true
 	}
 
 	// Legacy: resolve holdEndpoint to DID
@@ -1795,16 +1828,16 @@ func (gc *GarbageCollector) manifestBelongsToHold(ctx context.Context, manifest
 		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
+			return false, true
 		}
 		manifestHoldDID = resolved
 		if manifestHoldDID == holdDID {
-			return true
+			return true, true
 		}
 	}
 
 	if manifestHoldDID == "" {
-		return false
+		return false, true
 	}
 
 	// Check if the manifest's hold is a predecessor (has a successor label set)
@@ -1817,34 +1850,34 @@ func (gc *GarbageCollector) manifestBelongsToHold(ctx context.Context, manifest
 // 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) bool {
+func (gc *GarbageCollector) isPredecessorHold(ctx context.Context, holdDID string) (isPredecessor, definitive bool) {
 	if gc.predecessorCache == nil {
 		gc.predecessorCache = make(map[string]bool)
 	}
 
-	if isPredecessor, cached := gc.predecessorCache[holdDID]; cached {
+	if cachedPredecessor, cached := gc.predecessorCache[holdDID]; cached {
-		return isPredecessor
+		return cachedPredecessor, true
 	}
 
 	// Already inconclusive 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
+		return true, false
 	}
 
-	isPredecessor, definitive := gc.checkPredecessor(ctx, holdDID)
+	isPredecessor, definitive = gc.checkPredecessor(ctx, holdDID)
 	if !definitive {
 		if gc.predecessorUnresolved == nil {
 			gc.predecessorUnresolved = make(map[string]bool)
 		}
 		gc.predecessorUnresolved[holdDID] = true
-		gc.logger.Warn("GC: predecessor status unresolved, treating hold's manifests as referenced",
+		gc.logger.Warn("GC: predecessor status unresolved, keeping hold's blobs referenced but not adopting its manifests",
 			"holdDID", holdDID)
-		return true
+		return true, false
 	}
 
 	gc.predecessorCache[holdDID] = isPredecessor
-	return isPredecessor
+	return isPredecessor, true
 }
 
 // checkPredecessor fetches a hold's captain record to check if it has a successor label

pkg/hold/gc/gc_test.go

@@ -263,10 +263,13 @@ func TestManifestBelongsToHold(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := gc.manifestBelongsToHold(context.Background(), tt.manifest, holdDID)
+			got, definitive := gc.manifestBelongsToHold(context.Background(), tt.manifest, holdDID)
 			if got != tt.want {
 				t.Errorf("manifestBelongsToHold() = %v, want %v", got, tt.want)
 			}
+			if !definitive {
+				t.Errorf("manifestBelongsToHold() definitive = false, want true: no hold lookup is involved in this case")
+			}
 		})
 	}
 }
@@ -285,10 +288,19 @@ func TestPredecessorUnresolvedStaysReferenced(t *testing.T) {
 	const unreachable = "did:web:unreachable.invalid"
 	manifest := &atproto.ManifestRecord{HoldDID: unreachable}
 
-	if !gc.manifestBelongsToHold(context.Background(), manifest, holdDID) {
+	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
+	// 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")
+	}
+
 	if _, cached := gc.predecessorCache[unreachable]; cached {
 		t.Error("inconclusive check cached in predecessorCache: the outage would outlive itself")
 	}
@@ -298,9 +310,13 @@ func TestPredecessorUnresolvedStaysReferenced(t *testing.T) {
 	}
 
 	// Second call must be answered from predecessorUnresolved, not re-dialled.
-	if !gc.manifestBelongsToHold(context.Background(), manifest, holdDID) {
+	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")
+	}
 }
 
 // A hold that answers and declares no successor is the one negative worth caching.
@@ -440,6 +456,96 @@ func TestFetchUserManifests(t *testing.T) {
 	}
 }
 
+// TestFetchUserManifests_UnreachableHoldIsProtectOnly pins the split between
+// "keep these blobs referenced" and "this manifest is ours". A hold that cannot
+// be reached must not be written off, or its blobs are deleted. But it must not
+// be adopted either: adopting reports every layer of a foreign manifest as a
+// missing layer record, and reconcileMissingRecords acts on exactly that list,
+// writing io.atcr.hold.layer records that claim another hold's content.
+//
+// This is not hypothetical. A dev push to did:web:localhost:8080 leaves a
+// manifest whose hold can never resolve from a server, so it is permanently
+// indefinite rather than briefly down, and it surfaced on hold01 as ten missing
+// layer records for manifests it had never stored.
+func TestFetchUserManifests_UnreachableHoldIsProtectOnly(t *testing.T) {
+	holdDID := "did:web:hold.example.com"
+	// .invalid is reserved by RFC 2606 and never resolves, standing in for the
+	// localhost and RFC1918 hold DIDs that dev pushes leave behind.
+	const unreachable = "did:web:dev-push-stand-in.invalid"
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		response := map[string]any{
+			"records": []map[string]any{
+				{
+					"uri": "at://did:plc:user1/io.atcr.manifest/ours",
+					"cid": "bafyrei1",
+					"value": map[string]any{
+						"$type":      "io.atcr.manifest",
+						"repository": "ours",
+						"digest":     "sha256:ours",
+						"holdDid":    holdDID,
+						"layers": []map[string]any{
+							{"digest": "sha256:ourlayer", "size": 1000},
+						},
+					},
+				},
+				{
+					"uri": "at://did:plc:user1/io.atcr.manifest/devpush",
+					"cid": "bafyrei2",
+					"value": map[string]any{
+						"$type":      "io.atcr.manifest",
+						"repository": "valtest",
+						"digest":     "sha256:devpush",
+						"holdDid":    unreachable,
+						"layers": []map[string]any{
+							{"digest": "sha256:devlayer", "size": 170},
+						},
+					},
+				},
+			},
+		}
+		_ = json.NewEncoder(w).Encode(response)
+	}))
+	defer server.Close()
+
+	gc := &GarbageCollector{logger: newTestLogger()}
+
+	manifests, err := gc.fetchUserManifestsFromEndpoint(
+		t.Context(), "did:plc:user1", server.URL, holdDID)
+	if err != nil {
+		t.Fatalf("fetchUserManifestsFromEndpoint() error = %v", err)
+	}
+
+	// Both survive: the unreachable one is still carried so its blobs stay
+	// referenced. Dropping it here is the failure 95d4f7c fixed.
+	if len(manifests) != 2 {
+		t.Fatalf("expected 2 manifests (ours, plus the unreachable one kept for blob protection), got %d", len(manifests))
+	}
+
+	byURI := make(map[string]*manifestInfo, len(manifests))
+	for _, m := range manifests {
+		byURI[m.URI] = m
+	}
+
+	ours := byURI["at://did:plc:user1/io.atcr.manifest/ours"]
+	if ours == nil {
+		t.Fatal("our own manifest was dropped")
+	}
+	if ours.ProtectOnly {
+		t.Error("a directly-matching manifest was marked ProtectOnly: it would never be adopted, " +
+			"so its genuinely missing layer records would go unreported and unreconciled")
+	}
+
+	dev := byURI["at://did:plc:user1/io.atcr.manifest/devpush"]
+	if dev == nil {
+		t.Fatal("unreachable hold's manifest was dropped: its blobs fall out of the referenced set and are deleted")
+	}
+	if !dev.ProtectOnly {
+		t.Error("unreachable hold's manifest was adopted as ours: analyzeRecords would report its " +
+			"layers as missing layer records and Reconcile would write ownership records for another hold's content")
+	}
+}
+
 func TestFetchUserManifests_Pagination(t *testing.T) {
 	holdDID := "did:web:hold.example.com"
 	callCount := 0