hold/gc: test the blob sweep, and give the S3 mock object ages

deleteOrphanedBlobs is the only part of GC that removes bytes and it had no test. It could not have had one: object age decides whether a blob is deletable, the in-process harness stamps every object with time.Now(), and MockS3Client's ListObjectsV2 set no LastModified at all. Neither could express "this blob is nine days old", so both halves of the grace rule went unexercised — including the half that protects a push still in flight. MockS3Client gains ObjectTimes, a per-key LastModified consulted by ListObjectsV2. Keys with no entry list without a timestamp exactly as before, so existing tests are unaffected. It also gains DeleteObjectError, matching the error injection the other operations already had. Four cases, three of which are reasons NOT to delete: an old unreferenced blob goes; a young unreferenced blob stays; a referenced old blob stays; a /link object is never treated as a blob. Plus a failure case pinning that one undeletable object does not abort the walk, and that a blob which never left storage is not counted as deleted or reported as reclaimed space. Verified by mutation. Disabling the grace check deletes the young blob; disabling the referenced check deletes the live one; both fail. Disabling the /data suffix check changes nothing, because extractDigestFromPath anchors on /data$ and rejects everything else — so that check is a redundant early-out rather than a guard, and the test says so rather than implying otherwise. 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 5f74299bd7
commit 2984331f0c
2 changed files with 176 additions and 2 deletions
@@ -0,0 +1,153 @@
+package gc
+
+import (
+	"context"
+	"errors"
+	"testing"
+	"time"
+
+	"atcr.io/pkg/s3"
+)
+
+// errAllDeletesFail stands in for whatever S3 returns when an object cannot be
+// removed: a permissions change, an object lock, a bucket in a bad state.
+var errAllDeletesFail = errors.New("access denied")
+
+// blobKey builds the S3 key layout deleteOrphanedBlobs walks. The digest is
+// recovered from this path, so the shape matters: a key that does not parse is
+// silently skipped rather than deleted.
+func blobKey(digest string) string {
+	return "docker/registry/v2/blobs/sha256/" + digest[:2] + "/" + digest + "/data"
+}
+
+// TestDeleteOrphanedBlobs covers the only part of GC that removes bytes.
+//
+// It had no test, and could not have had one: the in-process harness stamps
+// every object with time.Now(), and the S3 mock stamped nothing at all, so
+// neither could express "this blob is nine days old". Object age is the input
+// that decides whether a blob is deletable, so both halves of that rule went
+// unexercised. MockS3Client.ObjectTimes exists to close exactly this gap.
+//
+// The four cases are the four ways this function must behave, and three of them
+// are reasons NOT to delete. That ratio is the point: the failure that matters
+// here is deleting something, not keeping it.
+//
+// Verified by mutation: disabling the grace check deletes the young blob,
+// disabling the referenced check deletes the live one. Both fail this test.
+func TestDeleteOrphanedBlobs(t *testing.T) {
+	const (
+		orphanOld    = "aaaa000000000000000000000000000000000000000000000000000000000001"
+		orphanYoung  = "bbbb000000000000000000000000000000000000000000000000000000000002"
+		referencedID = "cccc000000000000000000000000000000000000000000000000000000000003"
+	)
+
+	mock := s3.NewMockS3Client("")
+	svc := &s3.S3Service{Client: mock, Bucket: "test-bucket"}
+
+	longAgo := time.Now().Add(-30 * 24 * time.Hour)
+	justNow := time.Now().Add(-1 * time.Hour)
+
+	// Unreferenced and well past the grace period: the one blob that should go.
+	mock.Objects[blobKey(orphanOld)] = make([]byte, 4096)
+	mock.ObjectTimes[blobKey(orphanOld)] = longAgo
+
+	// Unreferenced but young. A push in flight looks exactly like this, and its
+	// records may not exist yet, so age is the only thing protecting it.
+	mock.Objects[blobKey(orphanYoung)] = make([]byte, 128)
+	mock.ObjectTimes[blobKey(orphanYoung)] = justNow
+
+	// Referenced and old. Age alone must never be enough.
+	mock.Objects[blobKey(referencedID)] = make([]byte, 256)
+	mock.ObjectTimes[blobKey(referencedID)] = longAgo
+
+	// Not a /data object. The link file sits beside the blob under the same
+	// prefix; deleting it is not a byte reclaim, it is corruption of the layout.
+	//
+	// Guarded twice, and worth knowing which one carries it: the HasSuffix
+	// check in deleteOrphanedBlobs is a redundant early-out, because
+	// extractDigestFromPath anchors its regex on /data$ and returns "" for
+	// anything else. Removing the suffix check leaves this case passing.
+	linkKey := "docker/registry/v2/blobs/sha256/dd/dddd000000000000000000000000000000000000000000000000000000000004/link"
+	mock.Objects[linkKey] = []byte("sha256:dddd")
+	mock.ObjectTimes[linkKey] = longAgo
+
+	gc := &GarbageCollector{s3: svc, logger: newTestLogger()}
+	referenced := map[string]bool{"sha256:" + referencedID: true}
+
+	result := &GCResult{}
+	if err := gc.deleteOrphanedBlobs(context.Background(), referenced, result); err != nil {
+		t.Fatalf("deleteOrphanedBlobs: %v", err)
+	}
+
+	if result.BlobsDeleted != 1 {
+		t.Errorf("BlobsDeleted = %d, want 1", result.BlobsDeleted)
+	}
+	if result.OrphanedBlobs != 1 {
+		t.Errorf("OrphanedBlobs = %d, want 1: only the old unreferenced blob is a candidate",
+			result.OrphanedBlobs)
+	}
+	if result.BytesReclaimed != 4096 {
+		t.Errorf("BytesReclaimed = %d, want 4096: reclaimed bytes are what the operator "+
+			"is shown, and they come from the listing rather than the delete",
+			result.BytesReclaimed)
+	}
+
+	survivors := map[string]string{
+		blobKey(orphanYoung):  "unreferenced but inside the blob grace period",
+		blobKey(referencedID): "referenced by a live manifest",
+		linkKey:               "not a /data object",
+	}
+	for key, why := range survivors {
+		if _, ok := mock.Objects[key]; !ok {
+			t.Errorf("deleted a blob that should have survived (%s): %s", why, key)
+		}
+	}
+	if _, ok := mock.Objects[blobKey(orphanOld)]; ok {
+		t.Error("the old unreferenced blob was counted but never actually removed from S3")
+	}
+}
+
+// TestDeleteOrphanedBlobs_KeepsCountingAfterADeleteFails pins the deliberate
+// choice to log and continue rather than abort. One unreclaimable object must
+// not strand every blob behind it in the walk, which on a large hold would mean
+// the sweep never finishes its first pass.
+//
+// The counters have to stay honest through it: the blob is a candidate either
+// way, but only a blob that actually left storage may be counted as deleted or
+// have its bytes reported as reclaimed.
+func TestDeleteOrphanedBlobs_KeepsCountingAfterADeleteFails(t *testing.T) {
+	const (
+		doomed   = "eeee000000000000000000000000000000000000000000000000000000000005"
+		stubborn = "ffff000000000000000000000000000000000000000000000000000000000006"
+	)
+
+	mock := s3.NewMockS3Client("")
+	svc := &s3.S3Service{Client: mock, Bucket: "test-bucket"}
+	longAgo := time.Now().Add(-30 * 24 * time.Hour)
+
+	for _, d := range []string{doomed, stubborn} {
+		mock.Objects[blobKey(d)] = make([]byte, 512)
+		mock.ObjectTimes[blobKey(d)] = longAgo
+	}
+
+	// DeleteObject fails for everything; both blobs are candidates, neither goes.
+	mock.DeleteObjectError = errAllDeletesFail
+
+	gc := &GarbageCollector{s3: svc, logger: newTestLogger()}
+	result := &GCResult{}
+	if err := gc.deleteOrphanedBlobs(context.Background(), map[string]bool{}, result); err != nil {
+		t.Fatalf("deleteOrphanedBlobs returned an error instead of continuing: %v", err)
+	}
+
+	if result.OrphanedBlobs != 2 {
+		t.Errorf("OrphanedBlobs = %d, want 2: the walk must reach the second blob "+
+			"after the first one fails to delete", result.OrphanedBlobs)
+	}
+	if result.BlobsDeleted != 0 {
+		t.Errorf("BlobsDeleted = %d, want 0: nothing left storage", result.BlobsDeleted)
+	}
+	if result.BytesReclaimed != 0 {
+		t.Errorf("BytesReclaimed = %d, want 0: reporting reclaimed space for bytes that "+
+			"are still there is how a hold quietly runs out of disk", result.BytesReclaimed)
+	}
+}
@@ -29,6 +29,16 @@ type MockS3Client struct {
 	// Objects stores in-memory blobs for PutObject/HeadObject/DeleteObject/CopyObject/ListObjectsV2.
 	Objects map[string][]byte

+	// ObjectTimes optionally sets LastModified per key for ListObjectsV2. A key
+	// with no entry is listed without a timestamp, which is what this mock has
+	// always done — WalkBlobs then reports the zero time, and anything reading
+	// it as an age sees the object as arbitrarily old.
+	//
+	// It exists because object age is a real input to behaviour: the GC blob
+	// sweep protects anything inside gcBlobGracePeriod, and a mock that cannot
+	// make an object young cannot test the protecting half of that rule.
+	ObjectTimes map[string]time.Time
+
 	// Track calls for verification in tests
 	mu                   sync.Mutex
 	CreateMultipartCalls []CreateMultipartCall
@@ -45,6 +55,7 @@ type MockS3Client struct {
 	AbortError           error
 	HeadObjectError      error
 	CopyObjectError      error
+	DeleteObjectError    error
 }

 // CreateMultipartCall records a CreateMultipartUpload call
@@ -99,6 +110,7 @@ func NewMockS3Client(testServerURL string) *MockS3Client {
 	return &MockS3Client{
 		TestServerURL:        testServerURL,
 		Objects:              make(map[string][]byte),
+		ObjectTimes:          make(map[string]time.Time),
 		CreateMultipartCalls: []CreateMultipartCall{},
 		CompleteCalls:        []CompleteCall{},
 		AbortCalls:           []AbortCall{},
@@ -266,8 +278,13 @@ func (m *MockS3Client) DeleteObject(ctx context.Context, input *awss3.DeleteObje
 	m.mu.Lock()
 	defer m.mu.Unlock()

+	if m.DeleteObjectError != nil {
+		return nil, m.DeleteObjectError
+	}
+
 	key := aws.ToString(input.Key)
 	delete(m.Objects, key)
+	delete(m.ObjectTimes, key)

 	return &awss3.DeleteObjectOutput{}, nil
 }
@@ -302,10 +319,14 @@ func (m *MockS3Client) ListObjectsV2(ctx context.Context, input *awss3.ListObjec

 		size := int64(len(data))
 		k := key
-		contents = append(contents, s3types.Object{
+		obj := s3types.Object{
 			Key:  &k,
 			Size: &size,
-		})
+		}
+		if lm, ok := m.ObjectTimes[key]; ok {
+			obj.LastModified = &lm
+		}
+		contents = append(contents, obj)
 	}

 	var cps []s3types.CommonPrefix