From 2580dcdb0f93e5d3bcaa7233dd64467161c4c857 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Mon, 10 Aug 2026 20:22:08 -0500 Subject: [PATCH] appview: stop a tag delete cascading into another repo's live image io.atcr.manifest rkeys are the digest alone (digestToRKey), so a single record backs every repository of a user holding identical content. Both paths that cascade-delete that record checked for remaining tags scoped to one repository, which asks the wrong question: a tag in another repo keeps the shared record alive just as much as a tag in this one. With me/a:v1 and me/b:v1 at the same digest, deleting me/a:v1 saw no remaining tags in repo a, deleted the shared PDS record, and purged the layers on the hold. me/b:v1 was left pointing at content that no longer exists, and the firehose delete handler then cleared the rows for every repo (DeleteManifest with an empty repository argument). The collision predates this, but it was reachable only behind the opt-in AutoRemoveUntagged profile flag. 1b91768 enabled OCI manifest DELETE and made TagStore.Untag cascade unconditionally, which turned a latent metadata collision into blob-level data loss on an ordinary skopeo/crane delete. - cleanupUntaggedManifest no longer filters candidate tags to rctx.Repository. The surrounding comment already noted that the tag collection is account-wide; the filter contradicted it. - ShouldCascadeDeleteManifest takes the tag question DID-wide via a new IsManifestTaggedAnyRepo, and drops its now-meaningless repository parameter. - IsManifestTagged stays repository-scoped and keeps its caller: the delete-manifest confirmation prompt is genuinely asking about the one repo whose tags the user is about to remove. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/appview/db/cascade_delete_test.go | 57 +++++++++++++++++++++++++-- pkg/appview/db/queries.go | 39 ++++++++++++++---- pkg/appview/handlers/images.go | 2 +- pkg/appview/storage/manifest_store.go | 16 +++++--- 4 files changed, 98 insertions(+), 16 deletions(-) diff --git a/pkg/appview/db/cascade_delete_test.go b/pkg/appview/db/cascade_delete_test.go index f5246be..424d80c 100644 --- a/pkg/appview/db/cascade_delete_test.go +++ b/pkg/appview/db/cascade_delete_test.go @@ -101,7 +101,7 @@ func TestShouldCascadeDeleteManifest_LastTagAndNoParent(t *testing.T) { seedCascadeFixture(t, db, did, repo, digest) // No tags pointing to this digest, no manifest_references entries. - ok, err := ShouldCascadeDeleteManifest(db, did, repo, digest) + ok, err := ShouldCascadeDeleteManifest(db, did, digest) if err != nil { t.Fatalf("ShouldCascadeDeleteManifest: %v", err) } @@ -135,7 +135,7 @@ func TestShouldCascadeDeleteManifest_RemainingTagBlocks(t *testing.T) { t.Fatalf("UpsertTag: %v", err) } - ok, err := ShouldCascadeDeleteManifest(db, did, repo, digest) + ok, err := ShouldCascadeDeleteManifest(db, did, digest) if err != nil { t.Fatalf("ShouldCascadeDeleteManifest: %v", err) } @@ -144,6 +144,57 @@ func TestShouldCascadeDeleteManifest_RemainingTagBlocks(t *testing.T) { } } +// TestShouldCascadeDeleteManifest_TagInOtherRepoBlocks: the same digest is +// tagged in a *different* repository of the same user. The io.atcr.manifest +// record is keyed by digest alone, so one record backs both repos — cascading +// here would delete the record out from under the other repo and purge the +// shared layers on the hold, breaking an image the user never touched. +func TestShouldCascadeDeleteManifest_TagInOtherRepoBlocks(t *testing.T) { + db, err := InitDB(":memory:", LibsqlConfig{}) + if err != nil { + t.Fatalf("InitDB: %v", err) + } + defer db.Close() + + const did = "did:plc:cascade4" + const deletingRepo = "myapp" + const otherRepo = "myapp-mirror" + const digest = "sha256:sharedacrossrepos" + + // Same content pushed to two repositories. + seedCascadeFixture(t, db, did, deletingRepo, digest) + if _, err := InsertManifest(db, &Manifest{ + DID: did, + Repository: otherRepo, + Digest: digest, + HoldEndpoint: "did:web:hold.example.com", + SchemaVersion: 2, + MediaType: "application/vnd.oci.image.manifest.v1+json", + CreatedAt: time.Now(), + }); err != nil { + t.Fatalf("InsertManifest(otherRepo): %v", err) + } + + // Only the other repo still carries a tag. The deleting repo has none. + if err := UpsertTag(db, &Tag{ + DID: did, + Repository: otherRepo, + Tag: "v1", + Digest: digest, + CreatedAt: time.Now(), + }); err != nil { + t.Fatalf("UpsertTag: %v", err) + } + + ok, err := ShouldCascadeDeleteManifest(db, did, digest) + if err != nil { + t.Fatalf("ShouldCascadeDeleteManifest: %v", err) + } + if ok { + t.Error("expected cascade=false when the digest is still tagged in another repository") + } +} + // TestShouldCascadeDeleteManifest_MultiArchChildBlocks: the digest is a child // of a manifest list (multi-arch parent). Even with no tags, deleting it // would orphan the parent's reference, so we must NOT cascade. @@ -189,7 +240,7 @@ func TestShouldCascadeDeleteManifest_MultiArchChildBlocks(t *testing.T) { t.Fatalf("InsertManifestReference: %v", err) } - ok, err := ShouldCascadeDeleteManifest(db, did, repo, childDigest) + ok, err := ShouldCascadeDeleteManifest(db, did, childDigest) if err != nil { t.Fatalf("ShouldCascadeDeleteManifest: %v", err) } diff --git a/pkg/appview/db/queries.go b/pkg/appview/db/queries.go index adbf7e2..b607822 100644 --- a/pkg/appview/db/queries.go +++ b/pkg/appview/db/queries.go @@ -1635,12 +1635,18 @@ func GetTagDigest(db DBTX, did, repository, tag string) (string, error) { } // ShouldCascadeDeleteManifest returns true iff a manifest can be safely -// deleted after a tag is removed: it has no remaining tags AND is not -// referenced by any manifest list (multi-arch parent). Manifest-list -// children must be preserved even when untagged, since their parent index -// still depends on them. -func ShouldCascadeDeleteManifest(db DBTX, did, repository, digest string) (bool, error) { - tagged, err := IsManifestTagged(db, did, repository, digest) +// deleted after a tag is removed: it has no remaining tags in ANY of the +// user's repositories AND is not referenced by any manifest list (multi-arch +// parent). Manifest-list children must be preserved even when untagged, since +// their parent index still depends on them. +// +// The tag check is deliberately DID-wide rather than repository-scoped. The +// io.atcr.manifest record this gates is keyed by digest alone, so a single +// record backs every repository holding identical content; a repo-scoped check +// would cascade a delete in one repo into another repo's live image and purge +// the shared layers on the hold. +func ShouldCascadeDeleteManifest(db DBTX, did, digest string) (bool, error) { + tagged, err := IsManifestTaggedAnyRepo(db, did, digest) if err != nil { return false, err } @@ -1654,7 +1660,26 @@ func ShouldCascadeDeleteManifest(db DBTX, did, repository, digest string) (bool, return !referenced, nil } -// IsManifestTagged checks if a manifest has any tags +// IsManifestTaggedAnyRepo reports whether a digest still carries a tag in any +// of the user's repositories. Use this to gate deletion of per-digest state +// (the io.atcr.manifest record, hold layer records); use IsManifestTagged when +// the question is genuinely about one repository, such as a confirmation +// prompt listing the tags a user is about to remove. +func IsManifestTaggedAnyRepo(db DBTX, did, digest string) (bool, error) { + var count int + err := db.QueryRow(` + SELECT COUNT(*) FROM tags + WHERE did = ? AND digest = ? + `, did, digest).Scan(&count) + + if err != nil { + return false, err + } + + return count > 0, nil +} + +// IsManifestTagged checks if a manifest has any tags in the given repository func IsManifestTagged(db DBTX, did, repository, digest string) (bool, error) { var count int err := db.QueryRow(` diff --git a/pkg/appview/handlers/images.go b/pkg/appview/handlers/images.go index afbae84..3e02257 100644 --- a/pkg/appview/handlers/images.go +++ b/pkg/appview/handlers/images.go @@ -90,7 +90,7 @@ func (h *DeleteTagHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // only-tagged image. Failures here are non-fatal: the tag is already // gone, so the worst case is a leftover untagged manifest the user can // clean up via "Delete untagged". - shouldCascade, err := db.ShouldCascadeDeleteManifest(h.ReadOnlyDB, user.DID, repo, digest) + shouldCascade, err := db.ShouldCascadeDeleteManifest(h.ReadOnlyDB, user.DID, digest) if err != nil { slog.Warn("delete-tag: failed to evaluate cascade", "did", user.DID, "repo", repo, "digest", digest, "error", err) } else if shouldCascade { diff --git a/pkg/appview/storage/manifest_store.go b/pkg/appview/storage/manifest_store.go index 596769d..b48d6c1 100644 --- a/pkg/appview/storage/manifest_store.go +++ b/pkg/appview/storage/manifest_store.go @@ -909,14 +909,20 @@ func cleanupUntaggedManifest(rctx *RegistryContext, oldDigest string) { if json.Unmarshal(rec.Value, &tagRecord) != nil { continue } - if tagRecord.Repository != rctx.Repository { - continue - } + // Deliberately not filtered to rctx.Repository. The record this + // check protects is keyed by digest alone (digestToRKey), so one + // io.atcr.manifest record backs every repository of this DID that + // holds identical content. A tag in any of them keeps it alive. + // Scoping the check to the current repository would let a delete in + // one repo remove the record out from under another and purge the + // shared layers on the hold, breaking an image nobody touched. d, dErr := tagRecord.GetManifestDigest() if dErr == nil && d == oldDigest { - // Still tagged by another tag — do not delete + // Still tagged, here or in another repository — do not delete slog.Debug("Auto-remove: manifest still tagged, skipping", - "digest", oldDigest, "tag", tagRecord.Tag) + "digest", oldDigest, "tag", tagRecord.Tag, + "taggedRepository", tagRecord.Repository, + "deletingRepository", rctx.Repository) return } }