hold/gc: cover checkPredecessorAt and the unresolved-holds reset

95d4f7c split the fetch-and-parse half of the predecessor probe into checkPredecessorAt precisely so it could be tested against a local server, but no test ever followed. The paths that had none are the ones that used to delete another hold's blobs: a 500, a refused connection, a body that is not JSON, and an envelope wrapping a garbage record all have to report definitive=false, because only a definitive answer is allowed into the process-lifetime predecessorCache. Verified by mutation rather than by passing: flipping the six failure-path returns in checkPredecessorAt to definitive=true, which is the pre-95d4f7c semantic, fails all four cases. Also pins the reset that the predecessorUnresolved field documents but nothing enforced. Removing the clear at the top of analyzeRecords makes the test fail, which is the point: without it one outage is permanent, every later run short-circuits on the stale entry, and GC silently stops reclaiming anything that hold's manifests touch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-02 00:06:58 +00:00 · 2026-08-25 16:34:25 -05:00
parent bb45a80d51
commit 48eee49ef9
1 changed files with 136 additions and 0 deletions
@@ -0,0 +1,136 @@
+package gc
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"atcr.io/pkg/atproto"
+)
+
+// captainResponse renders the getRecord envelope a hold returns for its own
+// captain record, with the successor field set to whatever the caller wants.
+func captainResponse(t *testing.T, successor string) string {
+	t.Helper()
+	captain := atproto.CaptainRecord{
+		Type:      atproto.CaptainCollection,
+		Owner:     "did:plc:owner",
+		Public:    true,
+		Successor: successor,
+	}
+	value, err := json.Marshal(captain)
+	if err != nil {
+		t.Fatalf("marshal captain: %v", err)
+	}
+	envelope, err := json.Marshal(map[string]json.RawMessage{"value": value})
+	if err != nil {
+		t.Fatalf("marshal envelope: %v", err)
+	}
+	return string(envelope)
+}
+
+// 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
+// dropped that hold's manifests out of the referenced set and deleted blobs
+// this hold is still serving on the predecessor's behalf.
+//
+// definitive must be false for all of them. isPredecessor is false too, but it
+// is the definitive flag that decides whether the answer is allowed to be
+// cached, and therefore the flag that carries the blob loss.
+func TestCheckPredecessorAt_InconclusiveOnTransportFailure(t *testing.T) {
+	tests := []struct {
+		name string
+		// holdURL returns the URL to probe. Each case gets its own server so a
+		// closed listener cannot disturb the others.
+		holdURL func(t *testing.T) string
+	}{
+		{
+			name: "hold answers 500",
+			holdURL: func(t *testing.T) string {
+				srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+					w.WriteHeader(http.StatusInternalServerError)
+				}))
+				t.Cleanup(srv.Close)
+				return srv.URL
+			},
+		},
+		{
+			name: "listener closed, connection refused",
+			holdURL: func(t *testing.T) string {
+				srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}))
+				url := srv.URL
+				// Close before probing: the port is real but nothing is on it,
+				// which is what a stopped hold looks like from here.
+				srv.Close()
+				return url
+			},
+		},
+		{
+			name: "200 with a body that is not JSON",
+			holdURL: func(t *testing.T) string {
+				srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+					fmt.Fprint(w, "<html>502 Bad Gateway</html>")
+				}))
+				t.Cleanup(srv.Close)
+				return srv.URL
+			},
+		},
+		{
+			name: "200 with a valid envelope wrapping a garbage record",
+			holdURL: func(t *testing.T) string {
+				srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+					fmt.Fprint(w, `{"value":"not-an-object"}`)
+				}))
+				t.Cleanup(srv.Close)
+				return srv.URL
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gc := &GarbageCollector{logger: newTestLogger()}
+
+			isPredecessor, definitive := gc.checkPredecessorAt(
+				context.Background(), "did:web:predecessor.example.com", tt.holdURL(t))
+
+			if definitive {
+				t.Errorf("definitive = true for an unanswered probe: the negative gets cached "+
+					"for the life of the process and this hold's blobs are deleted (isPredecessor=%v)",
+					isPredecessor)
+			}
+			if isPredecessor {
+				t.Errorf("isPredecessor = true, want false on a failed probe")
+			}
+		})
+	}
+}
+
+// TestAnalyzeRecordsClearsPredecessorUnresolved pins the reset documented on
+// the field itself. predecessorUnresolved exists so one unreachable hold costs
+// a single timeout per run rather than one per manifest. If the clear is ever
+// dropped, the first outage becomes permanent: every later run short-circuits
+// on the stale entry, treats that hold's manifests as referenced forever, and
+// GC quietly stops reclaiming anything those manifests touch.
+func TestAnalyzeRecordsClearsPredecessorUnresolved(t *testing.T) {
+	gc, _, ctx := newRegressionGC(t)
+
+	const staleHold = "did:web:was-down-last-night.example.com"
+	gc.predecessorUnresolved = map[string]bool{staleHold: true}
+
+	// An empty hold discovers no users, so this returns almost immediately. The
+	// clear happens at the top of the function, before any of that work.
+	if _, err := gc.analyzeRecords(ctx); err != nil {
+		t.Fatalf("analyzeRecords: %v", err)
+	}
+
+	if gc.predecessorUnresolved[staleHold] {
+		t.Error("predecessorUnresolved survived analyzeRecords: a hold that was down once " +
+			"is never re-checked, and GC stops reclaiming its manifests' blobs")
+	}
+}