db: drop the vestigial tags.id

Nothing joined on it. It was selected into a struct field no caller read, and
used only by DeleteTagsNotInList, which fetched surrogate ids, filtered them in
Go with a nested loop over the keep list, and issued one DELETE per row. The
natural key was already enforced by UNIQUE(did, repository, tag), so that
becomes the primary key and the column goes.

An AUTOINCREMENT rowid is allocated by whichever node performs the insert. That
is fine while every write funnels through one writer and stops being a stable
identity the moment they do not, so removing an identifier nobody used is the
cheapest way to shrink that surface before local-write replicas.

DeleteTagsNotInList now diffs against a set and deletes in batches. It still
reads the current tags first rather than issuing one NOT IN over the keep list:
that would need two placeholders per kept tag and would break past the driver's
parameter ceiling for a user with enough tags, and it cannot be chunked, because
each chunk would delete the tags every other chunk meant to keep. An explicit
delete list chunks safely.

idx_tags_did_repo is dropped rather than recreated: the new primary key indexes
(did, repository) as a prefix. It existed only because the primary key used to be
the surrogate id.

The rebuild names its columns explicitly. Column order is not guaranteed to
match between a fresh install and a migrated one, so INSERT ... SELECT * here
could write values into the wrong columns. TestMigration0032PreservesTagRows runs
the migration body against a table in the old shape and checks the contents
survive, which the schema drift test cannot: it compares shape, not data.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

pkg/appview/db/migrations/0032_drop_tags_id.yaml

@@ -0,0 +1,37 @@
+description: |
+  Drop the surrogate id from tags and key the table by (did, repository, tag).
+
+  Nothing ever joined on tags.id. It was selected into a struct field no caller
+  read, and used only by DeleteTagsNotInList to issue one DELETE per row. The
+  natural key was already enforced by UNIQUE(did, repository, tag), so this
+  promotes that to the primary key and removes the column.
+
+  An AUTOINCREMENT rowid is allocated by whichever node performs the insert,
+  which is fine while every write funnels through a single writer and stops being
+  a stable identity the moment they do not. Removing an identifier nobody used is
+  the cheapest way to shrink that surface.
+
+  idx_tags_did_repo is not recreated: the new primary key indexes
+  (did, repository) as a prefix, so a separate index on those two columns is
+  redundant. It was only needed while the primary key was the surrogate id.
+
+  Rebuilds the table, since SQLite cannot drop an INTEGER PRIMARY KEY column.
+  Columns are named explicitly in the INSERT ... SELECT: column order is not
+  guaranteed to match between a fresh install and a migrated one, so SELECT *
+  here could write values into the wrong columns.
+query: |
+  CREATE TABLE IF NOT EXISTS tags_new (
+      did TEXT NOT NULL,
+      repository TEXT NOT NULL,
+      tag TEXT NOT NULL,
+      digest TEXT NOT NULL,
+      created_at TIMESTAMP NOT NULL,
+      PRIMARY KEY(did, repository, tag),
+      FOREIGN KEY(did) REFERENCES users(did) ON DELETE CASCADE
+  );
+
+  INSERT OR IGNORE INTO tags_new (did, repository, tag, digest, created_at)
+  SELECT did, repository, tag, digest, created_at FROM tags;
+
+  DROP TABLE tags;
+  ALTER TABLE tags_new RENAME TO tags;

pkg/appview/db/models.go

@@ -57,7 +57,6 @@ type ManifestReference struct {
 
 // Tag represents a tag pointing to a manifest
 type Tag struct {
-	ID         int64
 	DID        string
 	Repository string
 	Tag        string

pkg/appview/db/queries.go

@@ -356,7 +356,7 @@ func GetUserRepositories(db DBTX, did string, viewerDID string) ([]Repository, e
 // ordering within each repo.
 func bulkTagsByRepo(db DBTX, did string, accessible map[string]bool) (map[string][]Tag, error) {
 	rows, err := db.Query(`
-        SELECT id, repository, tag, digest, created_at
+        SELECT repository, tag, digest, created_at
         FROM tags
         WHERE did = ?
         ORDER BY repository, created_at DESC
@@ -370,7 +370,7 @@ func bulkTagsByRepo(db DBTX, did string, accessible map[string]bool) (map[string
 	for rows.Next() {
 		var t Tag
 		t.DID = did
-		if err := rows.Scan(&t.ID, &t.Repository, &t.Tag, &t.Digest, &t.CreatedAt); err != nil {
+		if err := rows.Scan(&t.Repository, &t.Tag, &t.Digest, &t.CreatedAt); err != nil {
 			return nil, err
 		}
 		if !accessible[t.Repository] {
@@ -731,6 +731,10 @@ func GetTagsForDID(db DBTX, did string) ([]struct{ Repository, Tag string }, err
 
 // DeleteTagsNotInList deletes all tags for a DID that are not in the provided list.
 // Atomicity is provided by the caller's transaction when used during backfill.
+//
+// Tags are identified by (did, repository, tag), which is their natural key and
+// the table's primary key. This used to select surrogate ids, filter them in Go
+// with a nested loop, and then issue one DELETE per row.
 func DeleteTagsNotInList(db DBTX, did string, keepTags []struct{ Repository, Tag string }) error {
 	if len(keepTags) == 0 {
 		// No tags to keep - delete all for this DID
@@ -738,39 +742,52 @@ func DeleteTagsNotInList(db DBTX, did string, keepTags []struct{ Repository, Tag
 		return err
 	}
 
-	// First, get all current tags
-	rows, err := db.Query(`SELECT id, repository, tag FROM tags WHERE did = ?`, did)
+	// Work out what to remove in Go rather than with a NOT IN over the keep
+	// list. A single NOT IN would need two placeholders per kept tag and break
+	// past the driver's parameter ceiling for a user with enough tags, and it
+	// cannot be split into chunks: each chunk would delete the tags every other
+	// chunk meant to keep.
+	keep := make(map[struct{ Repository, Tag string }]struct{}, len(keepTags))
+	for _, k := range keepTags {
+		keep[k] = struct{}{}
+	}
+
+	rows, err := db.Query(`SELECT repository, tag FROM tags WHERE did = ?`, did)
 	if err != nil {
 		return err
 	}
-
-	var toDelete []int64
+	var toDelete []struct{ Repository, Tag string }
 	for rows.Next() {
-		var id int64
-		var repo, tag string
-		if err := rows.Scan(&id, &repo, &tag); err != nil {
+		var t struct{ Repository, Tag string }
+		if err := rows.Scan(&t.Repository, &t.Tag); err != nil {
 			rows.Close()
 			return err
 		}
-
-		// Check if this tag should be kept
-		found := false
-		for _, keep := range keepTags {
-			if keep.Repository == repo && keep.Tag == tag {
-				found = true
-				break
-			}
-		}
-
-		if !found {
-			toDelete = append(toDelete, id)
+		if _, ok := keep[t]; !ok {
+			toDelete = append(toDelete, t)
 		}
 	}
 	rows.Close()
+	if err := rows.Err(); err != nil {
+		return err
+	}
 
-	// Delete tags not in keep list
-	for _, id := range toDelete {
-		if _, err := db.Exec(`DELETE FROM tags WHERE id = ?`, id); err != nil {
+	// An explicit delete list chunks safely, unlike the NOT IN above.
+	for i := 0; i*BatchSize < len(toDelete); i++ {
+		start, end := chunk(len(toDelete), i)
+		batch := toDelete[start:end]
+
+		args := make([]any, 0, 1+2*len(batch))
+		args = append(args, did)
+		pairs := make([]string, 0, len(batch))
+		for _, t := range batch {
+			pairs = append(pairs, "(?, ?)")
+			args = append(args, t.Repository, t.Tag)
+		}
+
+		query := `DELETE FROM tags WHERE did = ? AND (repository, tag) IN (` +
+			strings.Join(pairs, ", ") + `)`
+		if _, err := db.Exec(query, args...); err != nil {
 			return err
 		}
 	}
@@ -962,7 +979,7 @@ func getTagsWithPlatformsFiltered(db DBTX, did, repository, tagName string, limi
 
 	query := `
         WITH paged_tags AS (
-            SELECT t.id, t.did, t.repository, t.tag, t.digest, t.created_at
+            SELECT t.did, t.repository, t.tag, t.digest, t.created_at
             FROM tags t
             JOIN manifests m ON t.did = m.did AND t.repository = m.repository AND t.digest = m.digest
             WHERE t.did = ? AND t.repository = ?
@@ -972,7 +989,6 @@ func getTagsWithPlatformsFiltered(db DBTX, did, repository, tagName string, limi
             LIMIT ? OFFSET ?
         )
         SELECT
-            t.id,
             t.did,
             t.repository,
             t.tag,
@@ -1013,7 +1029,7 @@ func getTagsWithPlatformsFiltered(db DBTX, did, repository, tagName string, limi
 		var childDigest, childHoldEndpoint string
 		var compressedSize int64
 
-		if err := rows.Scan(&t.ID, &t.DID, &t.Repository, &t.Tag, &t.Digest, &t.CreatedAt,
+		if err := rows.Scan(&t.DID, &t.Repository, &t.Tag, &t.Digest, &t.CreatedAt,
 			&mediaType, &artifactType, &holdEndpoint,
 			&platformOS, &platformArch, &platformVariant, &platformOSVersion,
 			&isAttestation, &childDigest, &childHoldEndpoint, &compressedSize); err != nil {
@@ -1991,7 +2007,7 @@ func GetRepository(db DBTX, did, repository string) (*Repository, error) {
 
 	// Get tags for this repo
 	tagRows, err := db.Query(`
-        SELECT id, tag, digest, created_at
+        SELECT tag, digest, created_at
         FROM tags
         WHERE did = ? AND repository = ?
         ORDER BY created_at DESC
@@ -2005,7 +2021,7 @@ func GetRepository(db DBTX, did, repository string) (*Repository, error) {
 		var t Tag
 		t.DID = did
 		t.Repository = repository
-		if err := tagRows.Scan(&t.ID, &t.Tag, &t.Digest, &t.CreatedAt); err != nil {
+		if err := tagRows.Scan(&t.Tag, &t.Digest, &t.CreatedAt); err != nil {
 			tagRows.Close()
 			return nil, err
 		}

pkg/appview/db/schema.sql

@@ -82,17 +82,21 @@ CREATE TABLE IF NOT EXISTS manifest_references (
 );
 CREATE INDEX IF NOT EXISTS idx_manifest_references_digest ON manifest_references(digest);
 
+-- Keyed by its natural key. There is no surrogate id: nothing joined on it, and
+-- an AUTOINCREMENT rowid is allocated by whichever node does the insert, which
+-- stops being a stable identity as soon as writes are not funnelled through a
+-- single writer.
 CREATE TABLE IF NOT EXISTS tags (
-    id INTEGER PRIMARY KEY AUTOINCREMENT,
     did TEXT NOT NULL,
     repository TEXT NOT NULL,
     tag TEXT NOT NULL,
     digest TEXT NOT NULL,
     created_at TIMESTAMP NOT NULL,
-    UNIQUE(did, repository, tag),
+    PRIMARY KEY(did, repository, tag),
     FOREIGN KEY(did) REFERENCES users(did) ON DELETE CASCADE
 );
-CREATE INDEX IF NOT EXISTS idx_tags_did_repo ON tags(did, repository);
+-- No separate (did, repository) index: the primary key already indexes that as a
+-- prefix. It was needed only while the primary key was the surrogate id.
 
 -- rev increments on every successful write, so a writer that read revision N can
 -- tell whether anyone has written since. Refresh tokens rotate on use, and the

pkg/appview/db/tag_natural_key_test.go

@@ -0,0 +1,281 @@
+package db
+
+import (
+	"database/sql"
+	"fmt"
+	"testing"
+	"time"
+)
+
+// seedTags inserts n tags under one repository and returns the user's DID.
+func seedTags(t *testing.T, database *sql.DB, repository string, n int) string {
+	t.Helper()
+
+	const did = "did:plc:tagowner"
+	if err := UpsertUser(database, &User{
+		DID:         did,
+		Handle:      "tagowner.example.com",
+		PDSEndpoint: "https://pds.example.com",
+		LastSeen:    time.Now(),
+	}); err != nil {
+		t.Fatalf("UpsertUser: %v", err)
+	}
+
+	for i := range n {
+		tag := fmt.Sprintf("v%d", i)
+		if err := UpsertTag(database, &Tag{
+			DID:        did,
+			Repository: repository,
+			Tag:        tag,
+			Digest:     fmt.Sprintf("sha256:%064d", i),
+			CreatedAt:  time.Now(),
+		}); err != nil {
+			t.Fatalf("UpsertTag %s: %v", tag, err)
+		}
+	}
+	return did
+}
+
+// TestDeleteTagsNotInListSpansChunks exercises the chunked delete.
+//
+// The deletions are batched at BatchSize to keep the placeholder count under the
+// driver's parameter ceiling, so the interesting case is more tags than fit in
+// one chunk. Getting the chunking wrong here deletes tags that should have been
+// kept, which during backfill silently destroys a user's tag list.
+func TestDeleteTagsNotInListSpansChunks(t *testing.T) {
+	database := revTestDB(t)
+
+	const (
+		repository = "myapp"
+		total      = BatchSize*2 + 37 // spans three chunks, last one partial
+	)
+	did := seedTags(t, database, repository, total)
+
+	// Keep every tenth tag; delete the rest.
+	var keep []struct{ Repository, Tag string }
+	kept := map[string]bool{}
+	for i := 0; i < total; i += 10 {
+		tag := fmt.Sprintf("v%d", i)
+		keep = append(keep, struct{ Repository, Tag string }{repository, tag})
+		kept[tag] = true
+	}
+
+	if err := DeleteTagsNotInList(database, did, keep); err != nil {
+		t.Fatalf("DeleteTagsNotInList: %v", err)
+	}
+
+	remaining, err := GetTagsForDID(database, did)
+	if err != nil {
+		t.Fatalf("GetTagsForDID: %v", err)
+	}
+	if len(remaining) != len(keep) {
+		t.Errorf("kept %d tags, want %d", len(remaining), len(keep))
+	}
+	for _, r := range remaining {
+		if !kept[r.Tag] {
+			t.Errorf("tag %q survived but was not in the keep list", r.Tag)
+		}
+	}
+	for tag := range kept {
+		found := false
+		for _, r := range remaining {
+			if r.Tag == tag {
+				found = true
+				break
+			}
+		}
+		if !found {
+			t.Errorf("tag %q was in the keep list but got deleted", tag)
+		}
+	}
+}
+
+// TestDeleteTagsNotInListEmptyKeepListDeletesAll pins the documented shortcut.
+func TestDeleteTagsNotInListEmptyKeepListDeletesAll(t *testing.T) {
+	database := revTestDB(t)
+	did := seedTags(t, database, "myapp", 5)
+
+	if err := DeleteTagsNotInList(database, did, nil); err != nil {
+		t.Fatalf("DeleteTagsNotInList: %v", err)
+	}
+
+	remaining, err := GetTagsForDID(database, did)
+	if err != nil {
+		t.Fatalf("GetTagsForDID: %v", err)
+	}
+	if len(remaining) != 0 {
+		t.Errorf("expected every tag to be deleted, %d remain", len(remaining))
+	}
+}
+
+// TestDeleteTagsNotInListScopedToDID: one user's backfill must not delete
+// another user's tags. The DID is part of the natural key, so this is really a
+// check that the key is applied and not just the (repository, tag) pair.
+func TestDeleteTagsNotInListScopedToDID(t *testing.T) {
+	database := revTestDB(t)
+	did := seedTags(t, database, "myapp", 3)
+
+	const otherDID = "did:plc:someoneelse"
+	if err := UpsertUser(database, &User{
+		DID:         otherDID,
+		Handle:      "other.example.com",
+		PDSEndpoint: "https://pds.example.com",
+		LastSeen:    time.Now(),
+	}); err != nil {
+		t.Fatalf("UpsertUser: %v", err)
+	}
+	if err := UpsertTag(database, &Tag{
+		DID:        otherDID,
+		Repository: "myapp",
+		Tag:        "v0",
+		Digest:     "sha256:other",
+		CreatedAt:  time.Now(),
+	}); err != nil {
+		t.Fatalf("UpsertTag: %v", err)
+	}
+
+	// Wipe every tag belonging to the first user.
+	if err := DeleteTagsNotInList(database, did, nil); err != nil {
+		t.Fatalf("DeleteTagsNotInList: %v", err)
+	}
+
+	otherTags, err := GetTagsForDID(database, otherDID)
+	if err != nil {
+		t.Fatalf("GetTagsForDID: %v", err)
+	}
+	if len(otherTags) != 1 {
+		t.Errorf("another user's tags were affected: %d remain, want 1", len(otherTags))
+	}
+}
+
+// TestTagNaturalKeyRejectsDuplicates: (did, repository, tag) is the primary key
+// now, so a repeated upsert must update in place rather than adding a row.
+func TestTagNaturalKeyRejectsDuplicates(t *testing.T) {
+	database := revTestDB(t)
+	did := seedTags(t, database, "myapp", 1)
+
+	if err := UpsertTag(database, &Tag{
+		DID:        did,
+		Repository: "myapp",
+		Tag:        "v0",
+		Digest:     "sha256:updated",
+		CreatedAt:  time.Now(),
+	}); err != nil {
+		t.Fatalf("UpsertTag: %v", err)
+	}
+
+	var count int
+	if err := database.QueryRow(
+		`SELECT COUNT(*) FROM tags WHERE did = ? AND repository = ? AND tag = ?`,
+		did, "myapp", "v0",
+	).Scan(&count); err != nil {
+		t.Fatalf("count: %v", err)
+	}
+	if count != 1 {
+		t.Errorf("expected the upsert to replace the row, found %d rows", count)
+	}
+}
+
+// TestMigration0032PreservesTagRows runs the rebuild migration against a table
+// in the OLD shape and checks every row survives.
+//
+// A rebuild migration is the one kind that can silently lose data: it copies
+// rows between tables, and a mistake in the INSERT ... SELECT shows up as
+// missing or misplaced values rather than an error. The drift test proves the
+// resulting *shape* is right but says nothing about the contents.
+func TestMigration0032PreservesTagRows(t *testing.T) {
+	database := revTestDB(t)
+
+	// Rebuild the old shape: surrogate id, UNIQUE rather than PRIMARY KEY.
+	oldShape := []string{
+		`DROP TABLE tags`,
+		`CREATE TABLE tags (
+			id INTEGER PRIMARY KEY AUTOINCREMENT,
+			did TEXT NOT NULL,
+			repository TEXT NOT NULL,
+			tag TEXT NOT NULL,
+			digest TEXT NOT NULL,
+			created_at TIMESTAMP NOT NULL,
+			UNIQUE(did, repository, tag),
+			FOREIGN KEY(did) REFERENCES users(did) ON DELETE CASCADE
+		)`,
+		`CREATE INDEX idx_tags_did_repo ON tags(did, repository)`,
+	}
+	for _, stmt := range oldShape {
+		if _, err := database.Exec(stmt); err != nil {
+			t.Fatalf("set up the old tags shape: %v", err)
+		}
+	}
+
+	if err := UpsertUser(database, &User{
+		DID:         "did:plc:tagowner",
+		Handle:      "tagowner.example.com",
+		PDSEndpoint: "https://pds.example.com",
+		LastSeen:    time.Now(),
+	}); err != nil {
+		t.Fatalf("UpsertUser: %v", err)
+	}
+
+	type row struct{ repo, tag, digest string }
+	want := []row{
+		{"myapp", "latest", "sha256:aaa"},
+		{"myapp", "v1.0.0", "sha256:bbb"},
+		{"otherapp", "latest", "sha256:ccc"},
+	}
+	for _, r := range want {
+		if _, err := database.Exec(
+			`INSERT INTO tags (did, repository, tag, digest, created_at) VALUES (?, ?, ?, ?, ?)`,
+			"did:plc:tagowner", r.repo, r.tag, r.digest, time.Now(),
+		); err != nil {
+			t.Fatalf("seed %s/%s: %v", r.repo, r.tag, err)
+		}
+	}
+
+	// Run the migration body itself, not a paraphrase of it.
+	migrations, err := loadMigrations()
+	if err != nil {
+		t.Fatalf("loadMigrations: %v", err)
+	}
+	var query string
+	for _, m := range migrations {
+		if m.Version == 32 {
+			query = m.Query
+		}
+	}
+	if query == "" {
+		t.Fatal("migration 0032 not found")
+	}
+	for i, stmt := range splitSQLStatements(query) {
+		if _, err := database.Exec(stmt); err != nil {
+			t.Fatalf("migration 0032 statement %d: %v\n%s", i+1, err, stmt)
+		}
+	}
+
+	for _, r := range want {
+		var digest string
+		err := database.QueryRow(
+			`SELECT digest FROM tags WHERE did = ? AND repository = ? AND tag = ?`,
+			"did:plc:tagowner", r.repo, r.tag,
+		).Scan(&digest)
+		if err != nil {
+			t.Errorf("%s/%s did not survive the rebuild: %v", r.repo, r.tag, err)
+			continue
+		}
+		if digest != r.digest {
+			t.Errorf("%s/%s digest = %q, want %q (columns are misaligned)", r.repo, r.tag, digest, r.digest)
+		}
+	}
+
+	var count int
+	if err := database.QueryRow(`SELECT COUNT(*) FROM tags`).Scan(&count); err != nil {
+		t.Fatalf("count: %v", err)
+	}
+	if count != len(want) {
+		t.Errorf("tag count = %d after the rebuild, want %d", count, len(want))
+	}
+
+	// The surrogate id must be gone.
+	if _, err := database.Query(`SELECT id FROM tags LIMIT 1`); err == nil {
+		t.Error("tags.id still exists after the migration")
+	}
+}