diff --git a/pkg/hold/gc/gc.go b/pkg/hold/gc/gc.go index b61911f..b96a726 100644 --- a/pkg/hold/gc/gc.go +++ b/pkg/hold/gc/gc.go @@ -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 diff --git a/pkg/hold/gc/gc_test.go b/pkg/hold/gc/gc_test.go index 918f6ad..670cf3d 100644 --- a/pkg/hold/gc/gc_test.go +++ b/pkg/hold/gc/gc_test.go @@ -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) } } diff --git a/pkg/hold/gc/predecessor_test.go b/pkg/hold/gc/predecessor_test.go index 473f922..fc102ec 100644 --- a/pkg/hold/gc/predecessor_test.go +++ b/pkg/hold/gc/predecessor_test.go @@ -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. //