diff --git a/pkg/hold/gc/gc.go b/pkg/hold/gc/gc.go index 2bd39d4..b61911f 100644 --- a/pkg/hold/gc/gc.go +++ b/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, - UserDID: userDID, - Record: &manifest, + URI: rec.URI, + UserDID: userDID, + 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 { - return isPredecessor + if cachedPredecessor, cached := gc.predecessorCache[holdDID]; cached { + 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 diff --git a/pkg/hold/gc/gc_test.go b/pkg/hold/gc/gc_test.go index a173f15..918f6ad 100644 --- a/pkg/hold/gc/gc_test.go +++ b/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