diff --git a/pkg/appview/storage/manifest_store.go b/pkg/appview/storage/manifest_store.go index b48d6c1..3507a08 100644 --- a/pkg/appview/storage/manifest_store.go +++ b/pkg/appview/storage/manifest_store.go @@ -439,6 +439,45 @@ func (s *ManifestStore) Put(ctx context.Context, manifest distribution.Manifest, // firehose #delete handler (jetstream), consistent with how the push path // relies on the firehose rather than writing the cache inline. func (s *ManifestStore) Delete(ctx context.Context, dgst digest.Digest) error { + // The io.atcr.manifest record is keyed by digest alone (digestToRKey), so a + // single record backs every repository of this DID holding identical + // content. Deleting it unconditionally reaches into repositories nobody + // touched, and purgeDeletedManifest then frees their layers on the hold — + // the second repo loses its manifest and its bytes. + // + // TagStore.Untag already routes through a DID-wide, fail-closed check for + // exactly this reason (cleanupUntaggedManifest); the by-digest path did not. + // One pass over the tag records answers both questions: which tags in THIS + // repository point at the digest, and whether any other repository still + // does. + inRepo, taggedElsewhere, err := tagsForDigest(ctx, s.ctx, dgst.String()) + if err != nil { + // Fail closed. Leaving a manifest behind is recoverable; deleting one + // another repository is still serving is not. + return err + } + + // A DELETE scoped to this repository must stop this repository serving the + // image, whether or not the shared record survives below. + for _, tag := range inRepo { + tagRKey := atproto.RepositoryTagToRKey(s.ctx.Repository, tag) + if err := s.ctx.ATProtoClient.DeleteRecord(ctx, atproto.TagCollection, tagRKey); err != nil { + if rl := rateLimitToErrcode(ctx, err); rl != err { + return rl + } + return err + } + } + + if taggedElsewhere { + slog.Info("Manifest delete: digest still tagged in another repository, keeping the shared record", + "component", "manifest-store", + "digest", dgst.String(), + "repository", s.ctx.Repository, + "did", s.ctx.DID) + return nil + } + rkey := digestToRKey(dgst) if err := s.ctx.ATProtoClient.DeleteRecord(ctx, atproto.ManifestCollection, rkey); err != nil { if rl := rateLimitToErrcode(ctx, err); rl != err { @@ -450,6 +489,56 @@ func (s *ManifestStore) Delete(ctx context.Context, dgst digest.Digest) error { return nil } +// tagsForDigest pages the DID's io.atcr.tag records once and reports the tags +// in rctx.Repository pointing at oldDigest, plus whether any OTHER repository +// still tags it. +// +// The enumeration must be exhaustive for the same reason cleanupUntaggedManifest +// says: tag records for every repository share one collection, so a single +// capped page is a per-account budget rather than a per-repo one. Past that, a +// live tag falls off the end and a referenced manifest gets deleted. Any +// failure to enumerate completely is returned as an error so the caller can +// fail closed rather than assume the digest is unreferenced. +func tagsForDigest(ctx context.Context, rctx *RegistryContext, oldDigest string) (inRepo []string, taggedElsewhere bool, err error) { + const tagPageSize = 100 + const maxTagPages = 100 + + cursor := "" + for page := 0; ; page++ { + if page >= maxTagPages { + return nil, false, fmt.Errorf("tag listing exceeded %d pages for %s; refusing to decide", maxTagPages, oldDigest) + } + + records, next, listErr := rctx.ATProtoClient.ListRecordsWithCursor(ctx, atproto.TagCollection, tagPageSize, cursor) + if listErr != nil { + return nil, false, fmt.Errorf("list tag records: %w", listErr) + } + + for _, rec := range records { + var tagRecord atproto.TagRecord + if json.Unmarshal(rec.Value, &tagRecord) != nil { + continue + } + d, dErr := tagRecord.GetManifestDigest() + if dErr != nil || d != oldDigest { + continue + } + if tagRecord.Repository == rctx.Repository { + inRepo = append(inRepo, tagRecord.Tag) + } else { + taggedElsewhere = true + } + } + + if next == "" || len(records) == 0 { + break + } + cursor = next + } + + return inRepo, taggedElsewhere, nil +} + // purgeDeletedManifest asks the hold, in the background, to drop the layer, // scan, and image-config records for a manifest we just deleted from the PDS, // freeing any blobs no longer referenced by another manifest. Best-effort: the diff --git a/pkg/appview/storage/manifest_store_test.go b/pkg/appview/storage/manifest_store_test.go index 5fb0f95..5135643 100644 --- a/pkg/appview/storage/manifest_store_test.go +++ b/pkg/appview/storage/manifest_store_test.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -696,14 +697,23 @@ func TestManifestStore_Delete(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + var deletedManifest bool server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Verify it's a DELETE request to deleteRecord endpoint + // Delete now pages the tag records first, to find out whether + // another repository still tags this digest. No tags here, so + // the digest is unreferenced and the delete proceeds. + if r.URL.Path == atproto.RepoListRecords { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"records":[]}`)) + return + } if r.Method != "POST" || r.URL.Path != atproto.RepoDeleteRecord { t.Errorf("Expected POST to %s, got %s %s", atproto.RepoDeleteRecord, r.Method, r.URL.Path) } + deletedManifest = true w.WriteHeader(tt.serverStatus) - w.Write([]byte(tt.serverResp)) + _, _ = w.Write([]byte(tt.serverResp)) })) defer server.Close() @@ -715,10 +725,64 @@ func TestManifestStore_Delete(t *testing.T) { if (err != nil) != tt.wantErr { t.Errorf("Delete() error = %v, wantErr %v", err, tt.wantErr) } + if !deletedManifest { + t.Error("Delete() never issued the manifest deleteRecord") + } }) } } +// TestManifestStore_Delete_KeepsRecordTaggedInAnotherRepo pins the guard added +// for the shared-digest case. The io.atcr.manifest record is keyed by digest +// alone, so one record backs every repository of a DID holding identical +// content: deleting through one repository used to strip the record out from +// under the others and purge their layers on the hold. +// +// The integration test TestManifestDelete_SharedDigestAcrossRepos covers the +// same property end to end. This one is here because it can assert the thing +// that actually matters — that no deleteRecord for the manifest collection is +// issued at all — which an end-to-end pull can only infer. +func TestManifestStore_Delete_KeepsRecordTaggedInAnotherRepo(t *testing.T) { + const dgst = "sha256:abc123" + + var manifestDeletes, tagDeletes int + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == atproto.RepoListRecords { + // One tag, in a DIFFERENT repository, pointing at this digest. + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"records":[{"uri":"at://did:plc:test123/io.atcr.tag/other_v1","cid":"bafy","value":{"$type":"io.atcr.tag","repository":"otherapp","tag":"v1","manifest":"at://did:plc:test123/io.atcr.manifest/abc123"}}]}`)) + return + } + if r.URL.Path == atproto.RepoDeleteRecord { + body, _ := io.ReadAll(r.Body) + if strings.Contains(string(body), atproto.ManifestCollection) { + manifestDeletes++ + } else { + tagDeletes++ + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"commit":{"cid":"bafytest","rev":"12345"}}`)) + return + } + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + client := atproto.NewClient(server.URL, "did:plc:test123", "token") + ctx := mockRegistryContext(client, "myapp", "did:web:hold.example.com", "did:plc:test123", "test.handle", nil) + store := NewManifestStore(ctx, nil) + + if err := store.Delete(context.Background(), dgst); err != nil { + t.Fatalf("Delete() error = %v, want nil", err) + } + if manifestDeletes != 0 { + t.Errorf("Delete() removed the shared io.atcr.manifest record %d time(s) while otherapp still tags it", manifestDeletes) + } + if tagDeletes != 0 { + t.Errorf("Delete() removed %d tag record(s); the only tag belongs to another repository", tagDeletes) + } +} + // TestManifestStore_Put_ManifestListValidation tests validation of manifest list child references func TestManifestStore_Put_ManifestListValidation(t *testing.T) { // Create a valid child manifest that exists diff --git a/test/integration/manifest_delete_test.go b/test/integration/manifest_delete_test.go index 6978d40..f068362 100644 --- a/test/integration/manifest_delete_test.go +++ b/test/integration/manifest_delete_test.go @@ -3,6 +3,7 @@ package integration import ( + "context" "fmt" "testing" @@ -112,3 +113,63 @@ func TestManifestDelete(t *testing.T) { } }) } + +// TestManifestDelete_SharedDigestAcrossRepos is the two-repo case the plan +// calls for, and it is the one shape none of the delete tests covered. +// +// The io.atcr.manifest record is keyed by digest alone (ManifestStore.Delete → +// digestToRKey), so a single record backs every repository of a user holding +// identical content. Deleting through one repository therefore reaches into +// every other repository sharing that digest — and ManifestStore.Delete then +// calls purgeDeletedManifest, which asks the hold to drop the layer records and +// free the blobs. The second repo loses both its manifest and its bytes. +// +// 2580dcd fixed exactly this class for the tag-delete UI handler and added the +// helper for it (db.ShouldCascadeDeleteManifest / db.IsManifestTaggedAnyRepo), +// but the OCI path never consulted it. +func TestManifestDelete_SharedDigestAcrossRepos(t *testing.T) { + h := testharness.New(t) + alice := h.AddSailor("alice.test") + creds := h.RegistryCreds(alice) + authOpts := []crane.Option{crane.WithAuth(toAuthn(creds)), crane.Insecure} + + repoA := fmt.Sprintf("%s/%s/shared-a", h.AppViewHostPort(), alice.Handle()) + repoB := fmt.Sprintf("%s/%s/shared-b", h.AppViewHostPort(), alice.Handle()) + tagA, tagB := repoA+":v1", repoB+":v1" + + img, err := random.Image(1<<20, 2) + if err != nil { + t.Fatalf("build image: %v", err) + } + if err := crane.Push(img, tagA, authOpts...); err != nil { + t.Fatalf("push a: %v", err) + } + // The same content in a second repository — what `crane copy` produces, and + // what any two repos built from the same base layer produce naturally. + if err := crane.Push(img, tagB, authOpts...); err != nil { + t.Fatalf("push b: %v", err) + } + + dgst, err := img.Digest() + if err != nil { + t.Fatalf("digest: %v", err) + } + if _, err := crane.Head(tagB, authOpts...); err != nil { + t.Fatalf("pre-delete head b: %v", err) + } + + // Delete through repo A only. + if err := crane.Delete(fmt.Sprintf("%s@%s", repoA, dgst.String()), authOpts...); err != nil { + t.Fatalf("delete a by digest: %v", err) + } + + // Repo B must be untouched. Pull rather than head: the manifest and the + // layer bytes are two separate casualties here, and a HEAD would only + // notice the first. + if _, err := crane.Head(tagB, authOpts...); err != nil { + t.Fatalf("repo B lost its manifest when repo A was deleted: %v", err) + } + if _, err := (craneClient{}).Pull(context.Background(), tagB, creds); err != nil { + t.Fatalf("repo B is no longer pullable after repo A was deleted: %v", err) + } +}