appview: stop an OCI manifest delete destroying another repo's image

The io.atcr.manifest record is keyed by digest alone (digestToRKey), so one
record backs every repository of a DID holding identical content.
ManifestStore.Delete removed it unconditionally and then called
purgeDeletedManifest, which asks the hold to drop the layer records and free
the blobs. Deleting through one repository therefore stripped the manifest out
from under every other repository sharing that digest and took their bytes with
it — reachable from the public OCI API with `crane delete`, and not recoverable.

Reproduced end to end before fixing: push identical content to shared-a and
shared-b, delete shared-a by digest, and shared-b:v1 answers 404.

The codebase already had the right guard and the right policy written down.
2580dcd added cleanupUntaggedManifest for exactly this hazard, and its comment
says it plainly — the check is "deliberately not filtered to rctx.Repository"
because "a tag in any of them keeps it alive". TagStore.Untag routes through it.
The by-digest path never did.

Delete now makes one pass over the DID's tag records, which answers both
questions at once: which tags in THIS repository point at the digest, and
whether any other repository still does. This repository's tags are removed
either way, because a DELETE scoped to a repository has to stop that repository
serving the image; the shared record and the hold purge only happen when
nothing else tags it. Enumeration failure is returned as an error rather than
swallowed, so the caller fails closed — leaving a manifest behind is
recoverable, deleting a live one is not, and the page budget exists for the
same reason cleanupUntaggedManifest has one.

Covered twice on purpose. The integration test proves the user-visible property
(repo B still pulls). The unit test asserts the thing an end-to-end pull can
only infer: that no deleteRecord for the manifest collection is issued at all.
Both fail against the pre-fix code.

TestManifestStore_Delete needed updating rather than fixing: its fake server
asserted every request was a deleteRecord, which the new tag-listing call
breaks. It now serves an empty tag list and additionally asserts the manifest
delete still happens, so the unshared path stays pinned.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh

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

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

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)
+	}
+}