diff --git a/pkg/appview/db/queries_test.go b/pkg/appview/db/queries_test.go index bd11630..97fc199 100644 --- a/pkg/appview/db/queries_test.go +++ b/pkg/appview/db/queries_test.go @@ -1,6 +1,7 @@ package db import ( + "slices" "testing" "time" ) @@ -1991,3 +1992,76 @@ func TestGetRepoCards_NullLastPushStillSortsByCreatedAt(t *testing.T) { t.Error("expected 'fresh' LastUpdated to fall back to created_at, got zero time") } } + +// TestGetAllUntaggedManifestDigests_TagInOtherRepoIsNotUntagged pins the +// interaction that makes the delete-untagged path safe. +// +// GetAllUntaggedManifestDigests joins tags scoped to one repository +// (m.repository = t.repository), so a digest tagged only in a DIFFERENT +// repository of the same DID is reported as untagged here. That is fine as a +// per-repository view, and dangerous as a delete list: the io.atcr.manifest +// record is keyed by digest alone, so one record backs every repository holding +// identical content, and deleting on this basis strips the record out from +// under the other repo and purges the shared layers on the hold. +// +// The handler therefore gates each digest on IsManifestTaggedAnyRepo, which is +// DID-wide. Both halves are asserted: the query's repo-scoped answer, so a +// future change to it is noticed, and the guard's DID-wide answer, which is the +// thing actually standing between a sweep and another repo's live image. +func TestGetAllUntaggedManifestDigests_TagInOtherRepoIsNotUntagged(t *testing.T) { + db, err := InitDB(":memory:", LibsqlConfig{}) + if err != nil { + t.Fatalf("Failed to init database: %v", err) + } + defer db.Close() + + did := "did:plc:shared" + shared := "sha256:sharedcontent" + now := time.Now() + manifestType := "application/vnd.oci.image.manifest.v1+json" + + if err := UpsertUser(db, &User{ + DID: did, Handle: "shared.test", + PDSEndpoint: "https://pds.example.com", LastSeen: now, + }); err != nil { + t.Fatalf("Failed to insert user: %v", err) + } + + // Identical content in two repositories — what `crane copy` produces, and + // what any two images built on the same base produce by accident. + for _, repo := range []string{"app-a", "app-b"} { + if _, err := InsertManifest(db, &Manifest{ + DID: did, Repository: repo, Digest: shared, + HoldEndpoint: "did:web:hold.example.com", SchemaVersion: 2, + MediaType: manifestType, CreatedAt: now, + }); err != nil { + t.Fatalf("Failed to insert manifest for %s: %v", repo, err) + } + } + + // Tagged in app-b only. app-a holds the same content, untagged. + if err := UpsertTag(db, &Tag{ + DID: did, Repository: "app-b", Tag: "v1", + Digest: shared, CreatedAt: now, + }); err != nil { + t.Fatalf("Failed to insert tag: %v", err) + } + + untagged, err := GetAllUntaggedManifestDigests(db, did, "app-a") + if err != nil { + t.Fatalf("GetAllUntaggedManifestDigests: %v", err) + } + if !slices.Contains(untagged, shared) { + t.Errorf("expected the repo-scoped view to report %s as untagged in app-a, got %v; "+ + "if this query became DID-wide the handler guard is now redundant, not wrong", shared, untagged) + } + + tagged, err := IsManifestTaggedAnyRepo(db, did, shared) + if err != nil { + t.Fatalf("IsManifestTaggedAnyRepo: %v", err) + } + if !tagged { + t.Error("IsManifestTaggedAnyRepo said the shared digest is untagged while app-b still tags it; " + + "the delete-untagged sweep would remove the record and purge app-b's layers") + } +} diff --git a/pkg/appview/handlers/images.go b/pkg/appview/handlers/images.go index 3e02257..43fccee 100644 --- a/pkg/appview/handlers/images.go +++ b/pkg/appview/handlers/images.go @@ -212,6 +212,24 @@ func (h *DeleteManifestHandler) ServeHTTP(w http.ResponseWriter, r *http.Request holdDID = cached.HoldEndpoint } + // This repository's tags are gone by now, so a remaining tag can only + // belong to another repository. The io.atcr.manifest record is keyed by + // digest alone, so one record backs every repository of this DID holding + // identical content — deleting it here would strip it out from under a repo + // nobody touched and purge the shared layers on the hold. Same guard + // DeleteTagHandler applies via ShouldCascadeDeleteManifest. + if stillTagged, err := db.IsManifestTaggedAnyRepo(h.ReadOnlyDB, user.DID, digest); err != nil { + // Fail closed: leaving a manifest behind is recoverable, deleting one + // another repository is still serving is not. + http.Error(w, fmt.Sprintf("Failed to check cross-repository tags: %v", err), http.StatusInternalServerError) + return + } else if stillTagged { + slog.Info("delete-manifest: digest still tagged in another repository, keeping the shared record", + "did", user.DID, "repo", repo, "digest", digest) + render.JSON(w, r, map[string]any{"success": true, "sharedRecordKept": true}) + return + } + // Compute rkey for manifest record (digest without "sha256:" prefix) rkey := strings.TrimPrefix(digest, "sha256:") @@ -292,7 +310,25 @@ func (h *DeleteUntaggedManifestsHandler) ServeHTTP(w http.ResponseWriter, r *htt } var failures []failure + skipped := 0 for _, digest := range digests { + // GetAllUntaggedManifestDigests scopes its tag join to this repository, + // so a digest tagged in a DIFFERENT repository of the same DID is + // reported as untagged here. The record is keyed by digest alone, so + // deleting on that basis breaks the other repo and purges its layers. + if stillTagged, err := db.IsManifestTaggedAnyRepo(h.ReadOnlyDB, user.DID, digest); err != nil { + // Fail closed for this digest rather than guessing. + slog.Warn("delete-untagged: cross-repository tag check failed, skipping", + "did", user.DID, "repo", req.Repo, "digest", digest, "error", err) + skipped++ + continue + } else if stillTagged { + slog.Info("delete-untagged: digest still tagged in another repository, skipping", + "did", user.DID, "repo", req.Repo, "digest", digest) + skipped++ + continue + } + rkey := strings.TrimPrefix(digest, "sha256:") // Snapshot hold ownership before the delete so we can purge after. @@ -328,8 +364,11 @@ func (h *DeleteUntaggedManifestsHandler) ServeHTTP(w http.ResponseWriter, r *htt } render.JSON(w, r, map[string]any{ - "deleted": deleted, - "failed": len(failures), + "deleted": deleted, + "failed": len(failures), + // Reported separately from failed: a skip is the guard working, not an + // error, and conflating the two would make a correct run look broken. + "skipped": skipped, "total": len(digests), "failures": failures, })