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