diff --git a/pkg/appview/db/batch.go b/pkg/appview/db/batch.go index 758dd97..bd78b03 100644 --- a/pkg/appview/db/batch.go +++ b/pkg/appview/db/batch.go @@ -41,20 +41,16 @@ func chunk(n, i int) (start, end int) { return start, end } -// BatchInsertManifests upserts a batch of manifests and returns a map of -// digest → manifest id for the inserted rows (both new and existing). Rows -// are keyed by (did, repository, digest); callers that need the id must -// group their input so that digest is unique per (did, repository) in one -// batch call. +// BatchInsertManifests upserts a batch of manifests. // -// Implementation: one multi-row INSERT per sub-batch, followed by one SELECT -// to fetch ids back (libsql's RETURNING support across replica modes is -// uneven; a second SELECT is reliable and still a single round-trip per -// sub-batch). -func BatchInsertManifests(db DBTX, manifests []Manifest) (map[string]int64, error) { - out := make(map[string]int64, len(manifests)) +// It used to return a map of digest to manifest id, because callers needed the +// rowid the database had just allocated in order to insert layers. They compute +// ManifestKey themselves now, so the follow-up SELECT and its per-DID IN list +// are gone: one round trip per sub-batch instead of two, and no dependency on +// reading back what we just wrote. +func BatchInsertManifests(db DBTX, manifests []Manifest) error { if len(manifests) == 0 { - return out, nil + return nil } for i := 0; i*BatchSize < len(manifests); i++ { @@ -65,24 +61,20 @@ func BatchInsertManifests(db DBTX, manifests []Manifest) (map[string]int64, erro args := make([]any, 0, len(batch)*cols) for _, m := range batch { args = append(args, + ManifestKey(m.DID, m.Repository, m.Digest), m.DID, m.Repository, m.Digest, m.HoldEndpoint, m.SchemaVersion, m.MediaType, m.ConfigDigest, m.ConfigSize, m.ArtifactType, nullString(m.SubjectDigest), m.CreatedAt, - ManifestKey(m.DID, m.Repository, m.Digest), ) } - // The trailing "manifest_key IS NULL" clause on the WHERE is what - // populates rows that predate the column. Without it this upsert skips - // unchanged manifests entirely, so re-running the backfill would never - // fill their key and the column would stay half-empty forever. query := ` INSERT INTO manifests - (did, repository, digest, hold_endpoint, schema_version, media_type, - config_digest, config_size, artifact_type, subject_digest, created_at, - manifest_key) + (manifest_key, did, repository, digest, hold_endpoint, schema_version, + media_type, config_digest, config_size, artifact_type, subject_digest, + created_at) VALUES ` + buildPlaceholders(len(batch), cols) + ` ON CONFLICT(did, repository, digest) DO UPDATE SET hold_endpoint = excluded.hold_endpoint, @@ -91,8 +83,7 @@ func BatchInsertManifests(db DBTX, manifests []Manifest) (map[string]int64, erro config_digest = excluded.config_digest, config_size = excluded.config_size, artifact_type = excluded.artifact_type, - subject_digest = excluded.subject_digest, - manifest_key = excluded.manifest_key + subject_digest = excluded.subject_digest WHERE excluded.hold_endpoint != manifests.hold_endpoint OR excluded.schema_version != manifests.schema_version OR excluded.media_type != manifests.media_type @@ -100,57 +91,13 @@ func BatchInsertManifests(db DBTX, manifests []Manifest) (map[string]int64, erro OR excluded.config_size IS NOT manifests.config_size OR excluded.artifact_type != manifests.artifact_type OR excluded.subject_digest IS NOT manifests.subject_digest - OR manifests.manifest_key IS NULL ` if _, err := db.Exec(query, args...); err != nil { - return nil, fmt.Errorf("batch insert manifests: %w", err) + return fmt.Errorf("batch insert manifests: %w", err) } - // Fetch ids for this sub-batch by (did, digest) — digests are unique enough - // that matching on (did, digest) avoids needing a three-column IN list. - // repository is included in the row to disambiguate if a user genuinely has - // the same digest across repos. - selectArgs := make([]any, 0, 1+2*len(batch)) - // Group by did (caller usually supplies one did per call, but be safe). - didSet := make(map[string]struct{}) - for _, m := range batch { - didSet[m.DID] = struct{}{} - } - // Build a per-did IN (?) query; usually exactly one iteration. - for did := range didSet { - digests := make([]string, 0, len(batch)) - for _, m := range batch { - if m.DID == did { - digests = append(digests, m.Digest) - } - } - selectArgs = append(selectArgs[:0], did) - for _, d := range digests { - selectArgs = append(selectArgs, d) - } - selectQuery := ` - SELECT repository, digest, id FROM manifests - WHERE did = ? AND digest IN (` + - strings.TrimSuffix(strings.Repeat("?,", len(digests)), ",") + `) - ` - rows, err := db.Query(selectQuery, selectArgs...) - if err != nil { - return nil, fmt.Errorf("batch select manifest ids: %w", err) - } - for rows.Next() { - var repo, digest string - var id int64 - if err := rows.Scan(&repo, &digest, &id); err != nil { - rows.Close() - return nil, fmt.Errorf("scan manifest id: %w", err) - } - // Key format matches what callers use: "did|repo|digest". - out[ManifestKey(did, repo, digest)] = id - } - rows.Close() - } } - return out, nil + return nil } // ManifestKey derives a manifest's node-independent identity from its natural @@ -202,13 +149,13 @@ func BatchInsertLayers(db DBTX, layers []Layer) error { s := string(b) annotationsJSON = &s } - args = append(args, l.ManifestID, l.Digest, l.Size, l.MediaType, l.LayerIndex, annotationsJSON) + args = append(args, l.ManifestKey, l.Digest, l.Size, l.MediaType, l.LayerIndex, annotationsJSON) } query := ` - INSERT INTO layers (manifest_id, digest, size, media_type, layer_index, annotations) + INSERT INTO layers (manifest_key, digest, size, media_type, layer_index, annotations) VALUES ` + buildPlaceholders(len(batch), cols) + ` - ON CONFLICT(manifest_id, layer_index) DO NOTHING + ON CONFLICT(manifest_key, layer_index) DO NOTHING ` if _, err := db.Exec(query, args...); err != nil { return fmt.Errorf("batch insert layers: %w", err) @@ -218,7 +165,7 @@ func BatchInsertLayers(db DBTX, layers []Layer) error { } // BatchInsertManifestReferences inserts a batch of manifest references. -// The table has PRIMARY KEY(manifest_id, reference_index); duplicates skip. +// The table has PRIMARY KEY(manifest_key, reference_index); duplicates skip. func BatchInsertManifestReferences(db DBTX, refs []ManifestReference) error { if len(refs) == 0 { return nil @@ -231,7 +178,7 @@ func BatchInsertManifestReferences(db DBTX, refs []ManifestReference) error { args := make([]any, 0, len(batch)*cols) for _, r := range batch { args = append(args, - r.ManifestID, r.Digest, r.Size, r.MediaType, + r.ManifestKey, r.Digest, r.Size, r.MediaType, r.PlatformArchitecture, r.PlatformOS, r.PlatformVariant, r.PlatformOSVersion, r.IsAttestation, r.ReferenceIndex, @@ -239,12 +186,12 @@ func BatchInsertManifestReferences(db DBTX, refs []ManifestReference) error { } query := ` - INSERT INTO manifest_references (manifest_id, digest, size, media_type, + INSERT INTO manifest_references (manifest_key, digest, size, media_type, platform_architecture, platform_os, platform_variant, platform_os_version, is_attestation, reference_index) VALUES ` + buildPlaceholders(len(batch), cols) + ` - ON CONFLICT(manifest_id, reference_index) DO NOTHING + ON CONFLICT(manifest_key, reference_index) DO NOTHING ` if _, err := db.Exec(query, args...); err != nil { return fmt.Errorf("batch insert manifest references: %w", err) diff --git a/pkg/appview/db/batch_test.go b/pkg/appview/db/batch_test.go index 71a7d19..1d793bd 100644 --- a/pkg/appview/db/batch_test.go +++ b/pkg/appview/db/batch_test.go @@ -64,7 +64,7 @@ func TestBuildPlaceholders(t *testing.T) { } } -func TestBatchInsertManifests_InsertsAndReturnsIDs(t *testing.T) { +func TestBatchInsertManifests_Inserts(t *testing.T) { d := setupBatchTestDB(t) createBatchTestUser(t, d, "did:plc:alice") @@ -74,22 +74,23 @@ func TestBatchInsertManifests_InsertsAndReturnsIDs(t *testing.T) { {DID: "did:plc:alice", Repository: "app2", Digest: "sha256:bbb", HoldEndpoint: "did:web:hold", SchemaVersion: 2, MediaType: "application/vnd.oci.image.manifest.v1+json", ArtifactType: "container-image", CreatedAt: now}, } - ids, err := BatchInsertManifests(d, manifests) - if err != nil { + if err := BatchInsertManifests(d, manifests); err != nil { t.Fatalf("batch insert: %v", err) } - if len(ids) != 2 { - t.Fatalf("expected 2 ids, got %d", len(ids)) - } - if ids[ManifestKey("did:plc:alice", "app1", "sha256:aaa")] == 0 { - t.Errorf("missing id for app1") - } - if ids[ManifestKey("did:plc:alice", "app2", "sha256:bbb")] == 0 { - t.Errorf("missing id for app2") - } if got := countRows(t, d, `SELECT COUNT(*) FROM manifests`); got != 2 { t.Errorf("row count = %d, want 2", got) } + // The key is derived from the natural key, so the caller can name any row + // it just wrote without reading anything back. + for _, m := range manifests { + var stored string + if err := d.QueryRow(`SELECT manifest_key FROM manifests WHERE digest = ?`, m.Digest).Scan(&stored); err != nil { + t.Fatalf("read key for %s: %v", m.Digest, err) + } + if want := ManifestKey(m.DID, m.Repository, m.Digest); stored != want { + t.Errorf("key for %s = %q, want %q", m.Digest, stored, want) + } + } } func TestBatchInsertManifests_Idempotent(t *testing.T) { @@ -103,10 +104,10 @@ func TestBatchInsertManifests_Idempotent(t *testing.T) { MediaType: "application/vnd.oci.image.manifest.v1+json", ArtifactType: "container-image", CreatedAt: now, }} - if _, err := BatchInsertManifests(d, m); err != nil { + if err := BatchInsertManifests(d, m); err != nil { t.Fatalf("first insert: %v", err) } - if _, err := BatchInsertManifests(d, m); err != nil { + if err := BatchInsertManifests(d, m); err != nil { t.Fatalf("second insert: %v", err) } if got := countRows(t, d, `SELECT COUNT(*) FROM manifests`); got != 1 { @@ -130,13 +131,9 @@ func TestBatchInsertManifests_Chunking(t *testing.T) { ArtifactType: "container-image", CreatedAt: now, } } - ids, err := BatchInsertManifests(d, manifests) - if err != nil { + if err := BatchInsertManifests(d, manifests); err != nil { t.Fatalf("batch insert: %v", err) } - if len(ids) != n { - t.Errorf("ids len = %d, want %d", len(ids), n) - } if got := countRows(t, d, `SELECT COUNT(*) FROM manifests`); got != n { t.Errorf("row count = %d, want %d", got, n) } @@ -147,20 +144,19 @@ func TestBatchInsertLayers_RespectsFK(t *testing.T) { createBatchTestUser(t, d, "did:plc:alice") now := time.Now() - ids, err := BatchInsertManifests(d, []Manifest{{ + if err := BatchInsertManifests(d, []Manifest{{ DID: "did:plc:alice", Repository: "app", Digest: "sha256:aaa", HoldEndpoint: "did:web:hold", SchemaVersion: 2, MediaType: "application/vnd.oci.image.manifest.v1+json", ArtifactType: "container-image", CreatedAt: now, - }}) - if err != nil { + }}); err != nil { t.Fatalf("insert manifest: %v", err) } - mid := ids[ManifestKey("did:plc:alice", "app", "sha256:aaa")] + mid := ManifestKey("did:plc:alice", "app", "sha256:aaa") layers := []Layer{ - {ManifestID: mid, Digest: "sha256:L0", Size: 100, MediaType: "application/vnd.oci.image.layer.v1.tar+gzip", LayerIndex: 0}, - {ManifestID: mid, Digest: "sha256:L1", Size: 200, MediaType: "application/vnd.oci.image.layer.v1.tar+gzip", LayerIndex: 1}, + {ManifestKey: mid, Digest: "sha256:L0", Size: 100, MediaType: "application/vnd.oci.image.layer.v1.tar+gzip", LayerIndex: 0}, + {ManifestKey: mid, Digest: "sha256:L1", Size: 200, MediaType: "application/vnd.oci.image.layer.v1.tar+gzip", LayerIndex: 1}, } if err := BatchInsertLayers(d, layers); err != nil { t.Fatalf("batch insert layers: %v", err) @@ -347,7 +343,7 @@ func TestBatchUpsertCrewMembers(t *testing.T) { func TestBatchEmptySlices(t *testing.T) { d := setupBatchTestDB(t) // Every batch function must tolerate an empty input slice without erroring. - if _, err := BatchInsertManifests(d, nil); err != nil { + if err := BatchInsertManifests(d, nil); err != nil { t.Errorf("manifests: %v", err) } if err := BatchInsertLayers(d, nil); err != nil { diff --git a/pkg/appview/db/cascade_delete_test.go b/pkg/appview/db/cascade_delete_test.go index 424d80c..9c35709 100644 --- a/pkg/appview/db/cascade_delete_test.go +++ b/pkg/appview/db/cascade_delete_test.go @@ -9,7 +9,7 @@ import ( // seedCascadeFixture inserts a user and a single manifest. Returns the // manifest's row id so callers can attach references (for the multi-arch case). -func seedCascadeFixture(t *testing.T, db *sql.DB, didStr, repo, digest string) int64 { +func seedCascadeFixture(t *testing.T, db *sql.DB, didStr, repo, digest string) string { t.Helper() user := &User{ @@ -229,7 +229,7 @@ func TestShouldCascadeDeleteManifest_MultiArchChildBlocks(t *testing.T) { } if err := InsertManifestReference(db, &ManifestReference{ - ManifestID: parentID, + ManifestKey: parentID, Digest: childDigest, Size: 1234, MediaType: "application/vnd.oci.image.manifest.v1+json", diff --git a/pkg/appview/db/delete_test.go b/pkg/appview/db/delete_test.go index 99d5169..1c8c737 100644 --- a/pkg/appview/db/delete_test.go +++ b/pkg/appview/db/delete_test.go @@ -26,9 +26,10 @@ func TestDeleteUserDataFull_DeletesAllData(t *testing.T) { // Create manifest _, err = db.Exec(` - INSERT INTO manifests (did, repository, digest, hold_endpoint, schema_version, media_type, created_at) - VALUES (?, ?, ?, ?, ?, ?, ?) - `, testUser.DID, "myapp", "sha256:abc123", "did:web:hold.example.com", 2, + INSERT INTO manifests (manifest_key, did, repository, digest, hold_endpoint, schema_version, media_type, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + `, ManifestKey(testUser.DID, "myapp", "sha256:abc123"), + testUser.DID, "myapp", "sha256:abc123", "did:web:hold.example.com", 2, "application/vnd.oci.image.manifest.v1+json", time.Now()) if err != nil { t.Fatalf("Failed to create manifest: %v", err) @@ -145,9 +146,10 @@ func TestDeleteUserDataFull_DoesNotAffectOtherUsers(t *testing.T) { // Create manifests for both users for _, user := range []*User{user1, user2} { _, err = db.Exec(` - INSERT INTO manifests (did, repository, digest, hold_endpoint, schema_version, media_type, created_at) - VALUES (?, ?, ?, ?, ?, ?, ?) - `, user.DID, "myapp", "sha256:"+user.DID, "did:web:hold.example.com", 2, + INSERT INTO manifests (manifest_key, did, repository, digest, hold_endpoint, schema_version, media_type, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + `, ManifestKey(user.DID, "myapp", "sha256:"+user.DID), + user.DID, "myapp", "sha256:"+user.DID, "did:web:hold.example.com", 2, "application/vnd.oci.image.manifest.v1+json", time.Now()) if err != nil { t.Fatalf("Failed to create manifest for %s: %v", user.Handle, err) diff --git a/pkg/appview/db/manifest_key_swap_test.go b/pkg/appview/db/manifest_key_swap_test.go new file mode 100644 index 0000000..30affe4 --- /dev/null +++ b/pkg/appview/db/manifest_key_swap_test.go @@ -0,0 +1,316 @@ +package db + +import ( + "database/sql" + "testing" + "time" +) + +// TestMigration0034PreservesLayersAndReferences is the test the cascade hazard +// demands. +// +// With foreign keys on, DROP TABLE performs an implicit DELETE FROM, so dropping +// manifests while layers still holds an ON DELETE CASCADE reference to it +// deletes every layer row. Migration 0009 did exactly that and the damage went +// unnoticed only because the Jetstream backfill rebuilds layers from PDS +// records. PRAGMA foreign_keys cannot prevent it here: it is a no-op inside a +// transaction, and migrations run in one. +// +// So 0034 orders its statements to keep the old children from ever referencing +// the table being dropped. This checks the rows actually survive, which the +// schema drift test cannot: that compares shape, not contents. +func TestMigration0034PreservesLayersAndReferences(t *testing.T) { + database := revTestDB(t) + reshapeToPre0034WithChildren(t, database) + + const did = "did:plc:swap" + if _, err := database.Exec( + `INSERT INTO users (did, handle, pds_endpoint, last_seen) VALUES (?, ?, ?, ?)`, + did, "swap.example.com", "https://pds.example.com", time.Now(), + ); err != nil { + t.Fatalf("seed user: %v", err) + } + + // Two manifests: one with layers, one acting as an index with references. + type seed struct { + repo, digest string + layers int + refs int + } + seeds := []seed{ + {"myapp", "sha256:image", 3, 0}, + {"myapp", "sha256:index", 0, 2}, + } + for _, s := range seeds { + res, err := database.Exec(` + INSERT INTO manifests (did, repository, digest, hold_endpoint, schema_version, media_type, artifact_type, created_at, manifest_key) + VALUES (?, ?, ?, 'did:web:hold', 2, 'application/vnd.oci.image.manifest.v1+json', 'container-image', ?, ?) + `, did, s.repo, s.digest, time.Now(), ManifestKey(did, s.repo, s.digest)) + if err != nil { + t.Fatalf("seed manifest %s: %v", s.digest, err) + } + id, err := res.LastInsertId() + if err != nil { + t.Fatalf("last insert id: %v", err) + } + for i := range s.layers { + if _, err := database.Exec(` + INSERT INTO layers (manifest_id, digest, size, media_type, layer_index) + VALUES (?, ?, ?, 'application/vnd.oci.image.layer.v1.tar+gzip', ?) + `, id, "sha256:layer", 100+i, i); err != nil { + t.Fatalf("seed layer: %v", err) + } + } + for i := range s.refs { + if _, err := database.Exec(` + INSERT INTO manifest_references (manifest_id, digest, media_type, size, platform_architecture, platform_os, reference_index) + VALUES (?, ?, 'application/vnd.oci.image.manifest.v1+json', 500, 'amd64', 'linux', ?) + `, id, "sha256:child", i); err != nil { + t.Fatalf("seed reference: %v", err) + } + } + } + + // No orphan is seeded here: the existing foreign key refuses to create one, + // which is why the migration's INSERT ... SELECT joins through manifest_id + // rather than copying blindly. The join is insurance for a database whose + // foreign keys were off at some point, not for anything reachable now. + + applyMigration(t, database, 34) + + if got := countRows(t, database, `SELECT COUNT(*) FROM layers`); got != 3 { + t.Errorf("layer count = %d, want 3", got) + } + if got := countRows(t, database, `SELECT COUNT(*) FROM manifest_references`); got != 2 { + t.Errorf("manifest_references count = %d, want 2", got) + } + if got := countRows(t, database, `SELECT COUNT(*) FROM manifests`); got != 2 { + t.Errorf("manifest count = %d, want 2", got) + } + + // Every surviving child must hang off the right parent. + imageKey := ManifestKey(did, "myapp", "sha256:image") + indexKey := ManifestKey(did, "myapp", "sha256:index") + if got := countRows(t, database, `SELECT COUNT(*) FROM layers WHERE manifest_key = '`+imageKey+`'`); got != 3 { + t.Errorf("layers attached to the image manifest = %d, want 3", got) + } + if got := countRows(t, database, `SELECT COUNT(*) FROM manifest_references WHERE manifest_key = '`+indexKey+`'`); got != 2 { + t.Errorf("references attached to the index manifest = %d, want 2", got) + } + + // id must be gone. + if _, err := database.Query(`SELECT id FROM manifests LIMIT 1`); err == nil { + t.Error("manifests.id still exists after the swap") + } +} + +// TestMigration0034KeepsCascadeDelete: the foreign keys have to survive the +// rebuild, or deleting a user would leave orphaned layers behind forever. +func TestMigration0034KeepsCascadeDelete(t *testing.T) { + database := revTestDB(t) + reshapeToPre0034WithChildren(t, database) + + const did = "did:plc:swap" + if _, err := database.Exec( + `INSERT INTO users (did, handle, pds_endpoint, last_seen) VALUES (?, ?, ?, ?)`, + did, "swap.example.com", "https://pds.example.com", time.Now(), + ); err != nil { + t.Fatalf("seed user: %v", err) + } + res, err := database.Exec(` + INSERT INTO manifests (did, repository, digest, hold_endpoint, schema_version, media_type, artifact_type, created_at, manifest_key) + VALUES (?, 'myapp', 'sha256:image', 'did:web:hold', 2, 'application/vnd.oci.image.manifest.v1+json', 'container-image', ?, ?) + `, did, time.Now(), ManifestKey(did, "myapp", "sha256:image")) + if err != nil { + t.Fatalf("seed manifest: %v", err) + } + id, _ := res.LastInsertId() + if _, err := database.Exec(` + INSERT INTO layers (manifest_id, digest, size, media_type, layer_index) + VALUES (?, 'sha256:layer', 100, 'application/vnd.oci.image.layer.v1.tar+gzip', 0) + `, id); err != nil { + t.Fatalf("seed layer: %v", err) + } + + applyMigration(t, database, 34) + + if _, err := database.Exec(`DELETE FROM users WHERE did = ?`, did); err != nil { + t.Fatalf("delete user: %v", err) + } + if got := countRows(t, database, `SELECT COUNT(*) FROM manifests`); got != 0 { + t.Errorf("manifests survived the user delete: %d", got) + } + if got := countRows(t, database, `SELECT COUNT(*) FROM layers`); got != 0 { + t.Errorf("layers survived the user delete: %d; the cascade was lost in the rebuild", got) + } +} + +// applyMigration runs one migration's SQL by version, as the runner would. +func applyMigration(t *testing.T, database *sql.DB, version int) { + t.Helper() + migrations, err := loadMigrations() + if err != nil { + t.Fatalf("loadMigrations: %v", err) + } + for _, m := range migrations { + if m.Version != version { + continue + } + for i, stmt := range splitSQLStatements(m.Query) { + if _, err := database.Exec(stmt); err != nil { + t.Fatalf("migration %04d statement %d: %v\n%s", version, i+1, err, stmt) + } + } + return + } + t.Fatalf("migration %04d not found", version) +} + +// reshapeToPre0034WithChildren rebuilds manifests, layers and +// manifest_references in their 0033 shape: surrogate ids, manifest_key present +// and populated. +func reshapeToPre0034WithChildren(t *testing.T, database *sql.DB) { + t.Helper() + stmts := []string{ + `DROP TABLE layers`, + `DROP TABLE manifest_references`, + `DROP TABLE manifests`, + `CREATE TABLE manifests ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + did TEXT NOT NULL, + repository TEXT NOT NULL, + digest TEXT NOT NULL, + hold_endpoint TEXT NOT NULL, + schema_version INTEGER NOT NULL, + media_type TEXT NOT NULL, + config_digest TEXT, + config_size INTEGER, + artifact_type TEXT NOT NULL DEFAULT 'container-image', + subject_digest TEXT, + created_at TIMESTAMP NOT NULL, + manifest_key TEXT, + UNIQUE(did, repository, digest), + FOREIGN KEY(did) REFERENCES users(did) ON DELETE CASCADE + )`, + `CREATE TABLE layers ( + manifest_id INTEGER NOT NULL, + digest TEXT NOT NULL, + size INTEGER NOT NULL, + media_type TEXT NOT NULL, + layer_index INTEGER NOT NULL, + annotations TEXT, + PRIMARY KEY(manifest_id, layer_index), + FOREIGN KEY(manifest_id) REFERENCES manifests(id) ON DELETE CASCADE + )`, + `CREATE TABLE manifest_references ( + manifest_id INTEGER NOT NULL, + digest TEXT NOT NULL, + media_type TEXT NOT NULL, + size INTEGER NOT NULL, + platform_architecture TEXT, + platform_os TEXT, + platform_variant TEXT, + platform_os_version TEXT, + is_attestation BOOLEAN DEFAULT FALSE, + reference_index INTEGER NOT NULL, + PRIMARY KEY(manifest_id, reference_index), + FOREIGN KEY(manifest_id) REFERENCES manifests(id) ON DELETE CASCADE + )`, + } + for _, stmt := range stmts { + if _, err := database.Exec(stmt); err != nil { + t.Fatalf("reshape to pre-0034: %v", err) + } + } +} + +// TestLatestManifestUsesCreatedAtNotInsertOrder pins the recency change that +// dropping id forced. +// +// Repo cards, search results and starred repos all needed "the newest manifest +// in this repo", and all five queries answered it with MAX(id) — the rowid as a +// proxy for insert order. A derived key has no ordering, so recency now comes +// from created_at, with manifest_key as a deterministic tiebreak. +// +// The two disagree whenever a manifest is indexed out of order, which the +// backfill does routinely: it walks a PDS and inserts whatever it finds, so the +// last row inserted is not the most recently pushed. created_at is the push +// time, and is the answer these queries always wanted. +func TestLatestManifestUsesCreatedAtNotInsertOrder(t *testing.T) { + database := revTestDB(t) + did := manifestKeyTestUser(t, database) + seedPublicHold(t, database) + + now := time.Now().UTC().Truncate(time.Second) + + // Insert the NEWER manifest first, so insert order and created_at disagree. + newer := sampleManifest(did, "myapp", "sha256:newer") + newer.CreatedAt = now + if _, err := InsertManifest(database, newer); err != nil { + t.Fatalf("insert newer: %v", err) + } + older := sampleManifest(did, "myapp", "sha256:older") + older.CreatedAt = now.Add(-48 * time.Hour) + if _, err := InsertManifest(database, older); err != nil { + t.Fatalf("insert older: %v", err) + } + + cards, err := GetUserRepoCards(database, did, "") + if err != nil { + t.Fatalf("GetUserRepoCards: %v", err) + } + if len(cards) != 1 { + t.Fatalf("expected 1 repo card, got %d", len(cards)) + } + if cards[0].Digest != "sha256:newer" { + t.Errorf("repo card shows digest %q, want the most recently created manifest sha256:newer; "+ + "recency is following insert order rather than created_at", cards[0].Digest) + } +} + +// TestLatestManifestTieBreakIsDeterministic: two manifests pushed in the same +// second must not make the card flip between them from query to query. +func TestLatestManifestTieBreakIsDeterministic(t *testing.T) { + database := revTestDB(t) + did := manifestKeyTestUser(t, database) + seedPublicHold(t, database) + + same := time.Now().UTC().Truncate(time.Second) + for _, digest := range []string{"sha256:aaa", "sha256:bbb", "sha256:ccc"} { + m := sampleManifest(did, "myapp", digest) + m.CreatedAt = same + if _, err := InsertManifest(database, m); err != nil { + t.Fatalf("insert %s: %v", digest, err) + } + } + + var first string + for i := range 5 { + cards, err := GetUserRepoCards(database, did, "") + if err != nil { + t.Fatalf("GetUserRepoCards: %v", err) + } + if len(cards) != 1 { + t.Fatalf("expected 1 repo card, got %d", len(cards)) + } + if i == 0 { + first = cards[0].Digest + continue + } + if cards[0].Digest != first { + t.Fatalf("tie broken differently between runs: %q then %q", first, cards[0].Digest) + } + } +} + +// seedPublicHold makes the sample manifests' hold visible to anonymous viewers, +// which the repo-card queries require. +func seedPublicHold(t *testing.T, database *sql.DB) { + t.Helper() + if _, err := database.Exec(` + INSERT INTO hold_captain_records (hold_did, owner_did, public, allow_all_crew) + VALUES ('did:web:hold.example.com', 'did:plc:holdowner', 1, 1) + `); err != nil { + t.Fatalf("seed hold: %v", err) + } +} diff --git a/pkg/appview/db/manifest_key_test.go b/pkg/appview/db/manifest_key_test.go index 9ce94f4..dff7bc4 100644 --- a/pkg/appview/db/manifest_key_test.go +++ b/pkg/appview/db/manifest_key_test.go @@ -95,120 +95,6 @@ func TestInsertManifestPopulatesKey(t *testing.T) { } } -// TestBackfillFillsKeyForUnchangedManifest is the mechanism this whole approach -// rests on. -// -// The column cannot be filled by its migration, because the value is a truncated -// sha256 and SQLite has no hash builtin. Instead the ordinary Jetstream backfill -// fills it while re-upserting every manifest. That only works because the upsert -// carries an extra "OR manifests.manifest_key IS NULL" clause on its WHERE -// guard: without it the upsert skips manifests whose other fields are unchanged, -// which is nearly all of them on a re-run, and the column would stay half-empty -// forever. -func TestBackfillFillsKeyForUnchangedManifest(t *testing.T) { - database := revTestDB(t) - did := manifestKeyTestUser(t, database) - - m := sampleManifest(did, "myapp", "sha256:abc") - if _, err := InsertManifest(database, m); err != nil { - t.Fatalf("InsertManifest: %v", err) - } - - // Simulate a row that predates the column. - if _, err := database.Exec( - `UPDATE manifests SET manifest_key = NULL WHERE did = ? AND repository = ? AND digest = ?`, - did, "myapp", "sha256:abc", - ); err != nil { - t.Fatalf("clear manifest_key: %v", err) - } - if _, ok := storedManifestKey(t, database, did, "myapp", "sha256:abc"); ok { - t.Fatal("failed to simulate a pre-existing row") - } - - // Re-upsert exactly the same manifest, as the backfill does. Nothing else - // about it has changed. - if _, err := InsertManifest(database, sampleManifest(did, "myapp", "sha256:abc")); err != nil { - t.Fatalf("re-insert: %v", err) - } - - got, ok := storedManifestKey(t, database, did, "myapp", "sha256:abc") - if !ok { - t.Fatal("an unchanged manifest kept its NULL key; the upsert's WHERE guard skipped it") - } - if want := ManifestKey(did, "myapp", "sha256:abc"); got != want { - t.Errorf("manifest_key = %q, want %q", got, want) - } -} - -// TestBatchBackfillFillsKeyForUnchangedManifests is the same property for the -// batch path, which is the one the backfill worker actually uses. -func TestBatchBackfillFillsKeyForUnchangedManifests(t *testing.T) { - database := revTestDB(t) - did := manifestKeyTestUser(t, database) - - manifests := []Manifest{ - *sampleManifest(did, "myapp", "sha256:aaa"), - *sampleManifest(did, "myapp", "sha256:bbb"), - *sampleManifest(did, "otherapp", "sha256:ccc"), - } - if _, err := BatchInsertManifests(database, manifests); err != nil { - t.Fatalf("BatchInsertManifests: %v", err) - } - if _, err := database.Exec(`UPDATE manifests SET manifest_key = NULL`); err != nil { - t.Fatalf("clear keys: %v", err) - } - - if _, err := BatchInsertManifests(database, manifests); err != nil { - t.Fatalf("re-run BatchInsertManifests: %v", err) - } - - remaining, total, err := ManifestKeyBackfillProgress(database) - if err != nil { - t.Fatalf("ManifestKeyBackfillProgress: %v", err) - } - if total != int64(len(manifests)) { - t.Errorf("total = %d, want %d", total, len(manifests)) - } - if remaining != 0 { - t.Errorf("%d manifests still have no key after a full backfill pass", remaining) - } -} - -// TestManifestKeyBackfillProgressCountsUnfilled: this is how anyone decides the -// follow-up migration is safe to run, so it has to actually count. -func TestManifestKeyBackfillProgressCountsUnfilled(t *testing.T) { - database := revTestDB(t) - did := manifestKeyTestUser(t, database) - - for _, digest := range []string{"sha256:aaa", "sha256:bbb", "sha256:ccc"} { - if _, err := InsertManifest(database, sampleManifest(did, "myapp", digest)); err != nil { - t.Fatalf("InsertManifest: %v", err) - } - } - - remaining, total, err := ManifestKeyBackfillProgress(database) - if err != nil { - t.Fatalf("ManifestKeyBackfillProgress: %v", err) - } - if remaining != 0 || total != 3 { - t.Errorf("remaining=%d total=%d, want 0 and 3", remaining, total) - } - - if _, err := database.Exec( - `UPDATE manifests SET manifest_key = NULL WHERE digest IN ('sha256:aaa', 'sha256:bbb')`, - ); err != nil { - t.Fatalf("clear keys: %v", err) - } - - remaining, total, err = ManifestKeyBackfillProgress(database) - if err != nil { - t.Fatalf("ManifestKeyBackfillProgress: %v", err) - } - if remaining != 2 || total != 3 { - t.Errorf("remaining=%d total=%d, want 2 and 3", remaining, total) - } -} - // TestManifestKeyUniqueIndexRejectsCollisions: the index is what turns the // 16-byte truncation from an assumption into something the database checks. If // it were ever dropped, a collision would silently attach one manifest's layers @@ -232,22 +118,3 @@ func TestManifestKeyUniqueIndexRejectsCollisions(t *testing.T) { t.Error("two manifests were allowed to share a manifest_key; the unique index is missing") } } - -// TestManifestKeyNullsDoNotCollide: the unique index has to tolerate many -// unfilled rows, or the migration could not add it before the backfill runs. -func TestManifestKeyNullsDoNotCollide(t *testing.T) { - database := revTestDB(t) - did := manifestKeyTestUser(t, database) - - for _, digest := range []string{"sha256:aaa", "sha256:bbb", "sha256:ccc"} { - if _, err := InsertManifest(database, sampleManifest(did, "myapp", digest)); err != nil { - t.Fatalf("InsertManifest: %v", err) - } - } - - // SQLite treats NULLs as distinct in a unique index; several unfilled rows - // must coexist. - if _, err := database.Exec(`UPDATE manifests SET manifest_key = NULL`); err != nil { - t.Errorf("multiple NULL manifest_key rows were rejected: %v", err) - } -} diff --git a/pkg/appview/db/migration_replay_test.go b/pkg/appview/db/migration_replay_test.go index 09a770f..51155dc 100644 --- a/pkg/appview/db/migration_replay_test.go +++ b/pkg/appview/db/migration_replay_test.go @@ -54,17 +54,13 @@ func TestMigrationReplayPopulatesManifestKeys(t *testing.T) { t.Fatalf("replay migrations: %v", err) } - remaining, total, err := ManifestKeyBackfillProgress(database) - if err != nil { - t.Fatalf("ManifestKeyBackfillProgress: %v", err) + var total int + if err := database.QueryRow(`SELECT COUNT(*) FROM manifests`).Scan(&total); err != nil { + t.Fatalf("count manifests: %v", err) } - if total != int64(len(seeded)) { + if total != len(seeded) { t.Fatalf("manifest count = %d after replay, want %d", total, len(seeded)) } - if remaining != 0 { - t.Errorf("%d of %d manifests have no key after replay; a later migration "+ - "depending on manifest_key would copy NULLs", remaining, total) - } for _, s := range seeded { var key string @@ -85,18 +81,15 @@ func TestMigrationReplayPopulatesManifestKeys(t *testing.T) { // the runtime backfill already did some of the work. Neither may break it. func TestManifestKeyHookIsIdempotent(t *testing.T) { database := revTestDB(t) - did := manifestKeyTestUser(t, database) + reshapeManifestsToPre0034(t, database) + seedPre0034Manifests(t, database, "did:plc:alice", []string{"sha256:aaa", "sha256:bbb"}) - for _, digest := range []string{"sha256:aaa", "sha256:bbb"} { - if _, err := InsertManifest(database, sampleManifest(did, "myapp", digest)); err != nil { - t.Fatalf("InsertManifest: %v", err) - } - } // One row already filled, one not: the mixed state a partly-backfilled // database is actually in. if _, err := database.Exec( - `UPDATE manifests SET manifest_key = NULL WHERE digest = ?`, "sha256:aaa"); err != nil { - t.Fatalf("clear one key: %v", err) + `UPDATE manifests SET manifest_key = ? WHERE digest = ?`, + ManifestKey("did:plc:alice", "myapp", "sha256:bbb"), "sha256:bbb"); err != nil { + t.Fatalf("pre-fill one key: %v", err) } for pass := range 2 { @@ -113,12 +106,17 @@ func TestManifestKeyHookIsIdempotent(t *testing.T) { } } - remaining, _, err := ManifestKeyBackfillProgress(database) - if err != nil { - t.Fatalf("ManifestKeyBackfillProgress: %v", err) + if n := countNullKeys(t, database); n != 0 { + t.Errorf("%d manifests still unfilled after two passes", n) } - if remaining != 0 { - t.Errorf("%d manifests still unfilled after two passes", remaining) + for _, digest := range []string{"sha256:aaa", "sha256:bbb"} { + var got string + if err := database.QueryRow(`SELECT manifest_key FROM manifests WHERE digest = ?`, digest).Scan(&got); err != nil { + t.Fatalf("read %s: %v", digest, err) + } + if want := ManifestKey("did:plc:alice", "myapp", digest); got != want { + t.Errorf("%s key = %q, want %q", digest, got, want) + } } } @@ -126,18 +124,14 @@ func TestManifestKeyHookIsIdempotent(t *testing.T) { // interesting case is more rows than one page. func TestManifestKeyHookSpansPages(t *testing.T) { database := revTestDB(t) - did := manifestKeyTestUser(t, database) + reshapeManifestsToPre0034(t, database) const total = manifestKeyBackfillBatch + 21 + digests := make([]string, total) for i := range total { - m := sampleManifest(did, "myapp", digestFor(i)) - if _, err := InsertManifest(database, m); err != nil { - t.Fatalf("InsertManifest %d: %v", i, err) - } - } - if _, err := database.Exec(`UPDATE manifests SET manifest_key = NULL`); err != nil { - t.Fatalf("clear keys: %v", err) + digests[i] = digestFor(i) } + seedPre0034Manifests(t, database, "did:plc:alice", digests) tx, err := database.Begin() if err != nil { @@ -151,15 +145,8 @@ func TestManifestKeyHookSpansPages(t *testing.T) { t.Fatalf("commit: %v", err) } - remaining, count, err := ManifestKeyBackfillProgress(database) - if err != nil { - t.Fatalf("ManifestKeyBackfillProgress: %v", err) - } - if count != total { - t.Fatalf("manifest count = %d, want %d", count, total) - } - if remaining != 0 { - t.Errorf("%d of %d unfilled; the hook stopped before the last page", remaining, total) + if n := countNullKeys(t, database); n != 0 { + t.Errorf("%d of %d unfilled; the hook stopped before the last page", n, total) } } @@ -203,3 +190,63 @@ func revTestDBRaw(t *testing.T) *sql.DB { t.Cleanup(func() { database.Close() }) return database } + +// countNullKeys reports how many manifests still lack a key. Only meaningful on +// a database that has not yet reached migration 0034, where manifest_key becomes +// the primary key and can no longer be null. +func countNullKeys(t *testing.T, database *sql.DB) int { + t.Helper() + var n int + if err := database.QueryRow(`SELECT COUNT(*) FROM manifests WHERE manifest_key IS NULL`).Scan(&n); err != nil { + t.Fatalf("count unfilled keys: %v", err) + } + return n +} + +// reshapeManifestsToPre0034 rebuilds the manifests table in its 0033 shape: a +// surrogate id, with manifest_key present but nullable. backfillManifestKeys only +// ever runs at that point in the migration sequence, so testing it against the +// current schema would be testing something that cannot happen. +func reshapeManifestsToPre0034(t *testing.T, database *sql.DB) { + t.Helper() + stmts := []string{ + `DROP TABLE layers`, + `DROP TABLE manifest_references`, + `DROP TABLE manifests`, + `CREATE TABLE manifests ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + did TEXT NOT NULL, + repository TEXT NOT NULL, + digest TEXT NOT NULL, + hold_endpoint TEXT NOT NULL, + schema_version INTEGER NOT NULL, + media_type TEXT NOT NULL, + config_digest TEXT, + config_size INTEGER, + artifact_type TEXT NOT NULL DEFAULT 'container-image', + subject_digest TEXT, + created_at TIMESTAMP NOT NULL, + manifest_key TEXT, + UNIQUE(did, repository, digest) + )`, + } + for _, stmt := range stmts { + if _, err := database.Exec(stmt); err != nil { + t.Fatalf("reshape manifests: %v", err) + } + } +} + +// seedPre0034Manifests inserts manifests with no key, as rows predating 0033 +// would be. +func seedPre0034Manifests(t *testing.T, database *sql.DB, did string, digests []string) { + t.Helper() + for _, digest := range digests { + if _, err := database.Exec(` + INSERT INTO manifests (did, repository, digest, hold_endpoint, schema_version, media_type, artifact_type, created_at) + VALUES (?, 'myapp', ?, 'did:web:hold', 2, 'application/vnd.oci.image.manifest.v1+json', 'container-image', ?) + `, did, digest, time.Now()); err != nil { + t.Fatalf("seed %s: %v", digest, err) + } + } +} diff --git a/pkg/appview/db/migrations/0034_manifest_key_swap.yaml b/pkg/appview/db/migrations/0034_manifest_key_swap.yaml new file mode 100644 index 0000000..15831de --- /dev/null +++ b/pkg/appview/db/migrations/0034_manifest_key_swap.yaml @@ -0,0 +1,115 @@ +description: | + Key manifests by manifest_key and drop the surrogate id. + + 0033 added manifest_key and filled it in-sequence, so by the time this runs + every manifest has one. This moves layers and manifest_references onto it and + removes id, which was the last node-allocated identifier in the AppView schema. + + Statement order here is load-bearing, and not for style reasons. With foreign + keys on, DROP TABLE performs an implicit DELETE FROM, so dropping manifests + while layers still holds an ON DELETE CASCADE reference to it deletes every + layer row. Migration 0009 did precisely that; it went unnoticed because the + Jetstream backfill rebuilds layers from PDS records, so the damage healed + itself. PRAGMA foreign_keys cannot be used to avoid this, because it is a no-op + inside a transaction and migrations run in one. + + So the new child tables are built pointing at manifests_new, the old children + are dropped first (dropping a child cascades nothing upward), and only then is + the old manifests table dropped, by which point nothing references it. The + renames come last: ALTER TABLE ... RENAME rewrites foreign key references in + other tables, so layers_new's reference to manifests_new becomes a reference to + manifests automatically. + + The INSERT ... SELECT statements join through the old manifest_id, so any + orphaned layer or reference row is dropped rather than carried forward. Those + rows could not satisfy the new foreign key anyway. + + Columns are named explicitly throughout: column order differs between a fresh + install and a migrated one, so SELECT * here would write values into the wrong + columns. + + manifest_key is declared NOT NULL as well as PRIMARY KEY. In SQLite a PRIMARY + KEY column still accepts NULL unless it is INTEGER PRIMARY KEY, so without the + explicit constraint a row could be written with no key at all and every join + through it would silently return nothing. +query: | + CREATE TABLE manifests_new ( + manifest_key TEXT PRIMARY KEY NOT NULL, + did TEXT NOT NULL, + repository TEXT NOT NULL, + digest TEXT NOT NULL, + hold_endpoint TEXT NOT NULL, + schema_version INTEGER NOT NULL, + media_type TEXT NOT NULL, + config_digest TEXT, + config_size INTEGER, + artifact_type TEXT NOT NULL DEFAULT 'container-image', + subject_digest TEXT, + created_at TIMESTAMP NOT NULL, + UNIQUE(did, repository, digest), + FOREIGN KEY(did) REFERENCES users(did) ON DELETE CASCADE + ); + + INSERT INTO manifests_new + (manifest_key, did, repository, digest, hold_endpoint, schema_version, + media_type, config_digest, config_size, artifact_type, subject_digest, created_at) + SELECT manifest_key, did, repository, digest, hold_endpoint, schema_version, + media_type, config_digest, config_size, artifact_type, subject_digest, created_at + FROM manifests; + + CREATE TABLE layers_new ( + manifest_key TEXT NOT NULL, + digest TEXT NOT NULL, + size INTEGER NOT NULL, + media_type TEXT NOT NULL, + layer_index INTEGER NOT NULL, + annotations TEXT, + PRIMARY KEY(manifest_key, layer_index), + FOREIGN KEY(manifest_key) REFERENCES manifests_new(manifest_key) ON DELETE CASCADE + ); + + INSERT OR IGNORE INTO layers_new + (manifest_key, digest, size, media_type, layer_index, annotations) + SELECT m.manifest_key, l.digest, l.size, l.media_type, l.layer_index, l.annotations + FROM layers l + JOIN manifests m ON m.id = l.manifest_id; + + CREATE TABLE manifest_references_new ( + manifest_key TEXT NOT NULL, + digest TEXT NOT NULL, + media_type TEXT NOT NULL, + size INTEGER NOT NULL, + platform_architecture TEXT, + platform_os TEXT, + platform_variant TEXT, + platform_os_version TEXT, + is_attestation BOOLEAN DEFAULT FALSE, + reference_index INTEGER NOT NULL, + PRIMARY KEY(manifest_key, reference_index), + FOREIGN KEY(manifest_key) REFERENCES manifests_new(manifest_key) ON DELETE CASCADE + ); + + INSERT OR IGNORE INTO manifest_references_new + (manifest_key, digest, media_type, size, platform_architecture, platform_os, + platform_variant, platform_os_version, is_attestation, reference_index) + SELECT m.manifest_key, mr.digest, mr.media_type, mr.size, mr.platform_architecture, + mr.platform_os, mr.platform_variant, mr.platform_os_version, + mr.is_attestation, mr.reference_index + FROM manifest_references mr + JOIN manifests m ON m.id = mr.manifest_id; + + DROP TABLE layers; + DROP TABLE manifest_references; + DROP TABLE manifests; + + ALTER TABLE manifests_new RENAME TO manifests; + ALTER TABLE layers_new RENAME TO layers; + ALTER TABLE manifest_references_new RENAME TO manifest_references; + + CREATE INDEX IF NOT EXISTS idx_manifests_did_repo ON manifests(did, repository); + CREATE INDEX IF NOT EXISTS idx_manifests_created_at ON manifests(created_at DESC); + CREATE INDEX IF NOT EXISTS idx_manifests_digest ON manifests(digest); + CREATE INDEX IF NOT EXISTS idx_manifests_artifact_type ON manifests(artifact_type); + CREATE INDEX IF NOT EXISTS idx_manifests_subject_digest ON manifests(subject_digest); + CREATE INDEX IF NOT EXISTS idx_layers_digest ON layers(digest); + CREATE INDEX IF NOT EXISTS idx_manifest_references_digest ON manifest_references(digest); diff --git a/pkg/appview/db/models.go b/pkg/appview/db/models.go index d01e6d8..206323f 100644 --- a/pkg/appview/db/models.go +++ b/pkg/appview/db/models.go @@ -16,7 +16,7 @@ type User struct { // Manifest represents an OCI manifest stored in the cache type Manifest struct { - ID int64 + Key string // node-independent identity; see ManifestKey DID string Repository string Digest string @@ -33,7 +33,7 @@ type Manifest struct { // Layer represents a layer in a manifest type Layer struct { - ManifestID int64 + ManifestKey string Digest string Size int64 MediaType string @@ -43,7 +43,7 @@ type Layer struct { // ManifestReference represents a reference to a manifest in a manifest list/index type ManifestReference struct { - ManifestID int64 + ManifestKey string Digest string Size int64 MediaType string diff --git a/pkg/appview/db/queries.go b/pkg/appview/db/queries.go index 641095c..f54f22e 100644 --- a/pkg/appview/db/queries.go +++ b/pkg/appview/db/queries.go @@ -104,13 +104,18 @@ func SearchRepositories(db DBTX, query string, limit, offset int, currentUserDID sqlQuery := ` WITH latest_manifests AS ( - SELECT did, repository, MAX(id) as latest_id - FROM manifests - WHERE hold_endpoint IN ` + accessibleHoldsSubquery + ` - GROUP BY did, repository + SELECT did, repository, manifest_key AS latest_key FROM ( + SELECT did, repository, manifest_key, + ROW_NUMBER() OVER ( + PARTITION BY did, repository + ORDER BY created_at DESC, manifest_key DESC + ) AS rn + FROM manifests + WHERE hold_endpoint IN ` + accessibleHoldsSubquery + ` + ) WHERE rn = 1 ), matching_repos AS ( - SELECT DISTINCT lm.did, lm.repository, lm.latest_id + SELECT DISTINCT lm.did, lm.repository, lm.latest_key FROM latest_manifests lm JOIN users u ON lm.did = u.did WHERE (u.handle LIKE ? ESCAPE '\' @@ -149,7 +154,7 @@ func SearchRepositories(db DBTX, query string, limit, offset int, currentUserDID MAX(COALESCE(rs.last_push, m.created_at), m.created_at), COALESCE(rp.avatar_cid, '') FROM matching_repos mr - JOIN manifests m ON mr.latest_id = m.id + JOIN manifests m ON mr.latest_key = m.manifest_key JOIN users u ON m.did = u.did JOIN repo_stats ON m.did = repo_stats.did AND m.repository = repo_stats.repository LEFT JOIN repository_stats rs ON m.did = rs.did AND m.repository = rs.repository @@ -197,10 +202,15 @@ func SearchRepositories(db DBTX, query string, limit, offset int, currentUserDID // Get total count of matching repositories countQuery := ` WITH latest_manifests AS ( - SELECT did, repository, MAX(id) as latest_id - FROM manifests - WHERE hold_endpoint IN ` + accessibleHoldsSubquery + ` - GROUP BY did, repository + SELECT did, repository, manifest_key AS latest_key FROM ( + SELECT did, repository, manifest_key, + ROW_NUMBER() OVER ( + PARTITION BY did, repository + ORDER BY created_at DESC, manifest_key DESC + ) AS rn + FROM manifests + WHERE hold_endpoint IN ` + accessibleHoldsSubquery + ` + ) WHERE rn = 1 ) SELECT COUNT(DISTINCT lm.did || '/' || lm.repository) FROM latest_manifests lm @@ -386,7 +396,7 @@ func bulkTagsByRepo(db DBTX, did string, accessible map[string]bool) (map[string // created_at DESC ordering within each repo. func bulkManifestsByRepo(db DBTX, did string, accessible map[string]bool) (map[string][]Manifest, error) { rows, err := db.Query(` - SELECT id, repository, digest, hold_endpoint, schema_version, media_type, + SELECT manifest_key, repository, digest, hold_endpoint, schema_version, media_type, config_digest, config_size, artifact_type, created_at FROM manifests WHERE did = ? @@ -401,7 +411,7 @@ func bulkManifestsByRepo(db DBTX, did string, accessible map[string]bool) (map[s for rows.Next() { var m Manifest m.DID = did - if err := rows.Scan(&m.ID, &m.Repository, &m.Digest, &m.HoldEndpoint, &m.SchemaVersion, + if err := rows.Scan(&m.Key, &m.Repository, &m.Digest, &m.HoldEndpoint, &m.SchemaVersion, &m.MediaType, &m.ConfigDigest, &m.ConfigSize, &m.ArtifactType, &m.CreatedAt); err != nil { return nil, err } @@ -795,20 +805,18 @@ func DeleteTagsNotInList(db DBTX, did string, keepTags []struct{ Repository, Tag return nil } -// InsertManifest inserts or updates a manifest record -// Uses UPSERT to update core metadata if manifest already exists -// Returns the manifest ID (works correctly for both insert and update) +// InsertManifest inserts or updates a manifest record. +// Uses UPSERT to update core metadata if manifest already exists. +// Returns the manifest key, which the caller could equally compute itself. // Note: Annotations are stored separately in repository_annotations table -func InsertManifest(db DBTX, manifest *Manifest) (int64, error) { - // The trailing "manifest_key IS NULL" clause on the WHERE is what populates - // rows that predate the column. Without it this upsert skips unchanged - // manifests entirely, so re-running the backfill would never fill their key - // and the column would stay half-empty forever. +func InsertManifest(db DBTX, manifest *Manifest) (string, error) { + key := ManifestKey(manifest.DID, manifest.Repository, manifest.Digest) + _, err := db.Exec(` INSERT INTO manifests - (did, repository, digest, hold_endpoint, schema_version, media_type, - config_digest, config_size, artifact_type, subject_digest, created_at, - manifest_key) + (manifest_key, did, repository, digest, hold_endpoint, schema_version, + media_type, config_digest, config_size, artifact_type, subject_digest, + created_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT(did, repository, digest) DO UPDATE SET hold_endpoint = excluded.hold_endpoint, @@ -817,8 +825,7 @@ func InsertManifest(db DBTX, manifest *Manifest) (int64, error) { config_digest = excluded.config_digest, config_size = excluded.config_size, artifact_type = excluded.artifact_type, - subject_digest = excluded.subject_digest, - manifest_key = excluded.manifest_key + subject_digest = excluded.subject_digest WHERE excluded.hold_endpoint != manifests.hold_endpoint OR excluded.schema_version != manifests.schema_version OR excluded.media_type != manifests.media_type @@ -826,30 +833,17 @@ func InsertManifest(db DBTX, manifest *Manifest) (int64, error) { OR excluded.config_size IS NOT manifests.config_size OR excluded.artifact_type != manifests.artifact_type OR excluded.subject_digest IS NOT manifests.subject_digest - OR manifests.manifest_key IS NULL - `, manifest.DID, manifest.Repository, manifest.Digest, manifest.HoldEndpoint, + `, key, manifest.DID, manifest.Repository, manifest.Digest, manifest.HoldEndpoint, manifest.SchemaVersion, manifest.MediaType, manifest.ConfigDigest, manifest.ConfigSize, manifest.ArtifactType, sql.NullString{String: manifest.SubjectDigest, Valid: manifest.SubjectDigest != ""}, - manifest.CreatedAt, - ManifestKey(manifest.DID, manifest.Repository, manifest.Digest)) + manifest.CreatedAt) if err != nil { - return 0, err + return "", err } - // Query for the ID (works for both insert and update) - var id int64 - err = db.QueryRow(` - SELECT id FROM manifests - WHERE did = ? AND repository = ? AND digest = ? - `, manifest.DID, manifest.Repository, manifest.Digest).Scan(&id) - - if err != nil { - return 0, fmt.Errorf("failed to get manifest ID after upsert: %w", err) - } - - return id, nil + return key, nil } // InsertLayer inserts a layer record, skipping if it already exists. @@ -865,10 +859,10 @@ func InsertLayer(db DBTX, layer *Layer) error { annotationsJSON = &s } _, err := db.Exec(` - INSERT INTO layers (manifest_id, digest, size, media_type, layer_index, annotations) + INSERT INTO layers (manifest_key, digest, size, media_type, layer_index, annotations) VALUES (?, ?, ?, ?, ?, ?) - ON CONFLICT(manifest_id, layer_index) DO NOTHING - `, layer.ManifestID, layer.Digest, layer.Size, layer.MediaType, layer.LayerIndex, annotationsJSON) + ON CONFLICT(manifest_key, layer_index) DO NOTHING + `, layer.ManifestKey, layer.Digest, layer.Size, layer.MediaType, layer.LayerIndex, annotationsJSON) return err } @@ -1012,10 +1006,10 @@ func getTagsWithPlatformsFiltered(db DBTX, did, repository, tagName string, limi COALESCE(mr.is_attestation, 0) as is_attestation, COALESCE(mr.digest, '') as child_digest, COALESCE(child_m.hold_endpoint, m.hold_endpoint, '') as child_hold_endpoint, - COALESCE((SELECT SUM(l.size) FROM layers l WHERE l.manifest_id = COALESCE(child_m.id, m.id)), 0) as compressed_size + COALESCE((SELECT SUM(l.size) FROM layers l WHERE l.manifest_key = COALESCE(child_m.manifest_key, m.manifest_key)), 0) as compressed_size FROM paged_tags t JOIN manifests m ON t.digest = m.digest AND t.did = m.did AND t.repository = m.repository - LEFT JOIN manifest_references mr ON m.id = mr.manifest_id + LEFT JOIN manifest_references mr ON m.manifest_key = mr.manifest_key LEFT JOIN manifests child_m ON mr.digest = child_m.digest AND child_m.did = t.did AND child_m.repository = t.repository ORDER BY t.created_at DESC, mr.reference_index` @@ -1127,11 +1121,11 @@ func GetManifest(db DBTX, digest string) (*Manifest, error) { var m Manifest err := db.QueryRow(` - SELECT id, did, repository, digest, hold_endpoint, schema_version, + SELECT manifest_key, did, repository, digest, hold_endpoint, schema_version, media_type, config_digest, config_size, created_at FROM manifests WHERE digest = ? - `, digest).Scan(&m.ID, &m.DID, &m.Repository, &m.Digest, &m.HoldEndpoint, + `, digest).Scan(&m.Key, &m.DID, &m.Repository, &m.Digest, &m.HoldEndpoint, &m.SchemaVersion, &m.MediaType, &m.ConfigDigest, &m.ConfigSize, &m.CreatedAt) @@ -1147,14 +1141,14 @@ func GetManifest(db DBTX, digest string) (*Manifest, error) { func GetNewestManifestForRepo(db DBTX, did, repository string) (*Manifest, error) { var m Manifest err := db.QueryRow(` - SELECT id, did, repository, digest, hold_endpoint, schema_version, media_type, + SELECT manifest_key, did, repository, digest, hold_endpoint, schema_version, media_type, config_digest, config_size, created_at FROM manifests WHERE did = ? AND repository = ? ORDER BY created_at DESC LIMIT 1 `, did, repository).Scan( - &m.ID, &m.DID, &m.Repository, &m.Digest, + &m.Key, &m.DID, &m.Repository, &m.Digest, &m.HoldEndpoint, &m.SchemaVersion, &m.MediaType, &m.ConfigDigest, &m.ConfigSize, &m.CreatedAt, ) @@ -1213,13 +1207,13 @@ func GetRepositoriesForDID(db DBTX, did string) ([]string, error) { } // GetLayersForManifest fetches all layers for a manifest -func GetLayersForManifest(db DBTX, manifestID int64) ([]Layer, error) { +func GetLayersForManifest(db DBTX, manifestKey string) ([]Layer, error) { rows, err := db.Query(` - SELECT manifest_id, digest, size, media_type, layer_index, annotations + SELECT manifest_key, digest, size, media_type, layer_index, annotations FROM layers - WHERE manifest_id = ? + WHERE manifest_key = ? ORDER BY layer_index - `, manifestID) + `, manifestKey) if err != nil { return nil, err @@ -1230,7 +1224,7 @@ func GetLayersForManifest(db DBTX, manifestID int64) ([]Layer, error) { for rows.Next() { var l Layer var annotationsJSON sql.NullString - if err := rows.Scan(&l.ManifestID, &l.Digest, &l.Size, &l.MediaType, &l.LayerIndex, &annotationsJSON); err != nil { + if err := rows.Scan(&l.ManifestKey, &l.Digest, &l.Size, &l.MediaType, &l.LayerIndex, &annotationsJSON); err != nil { return nil, err } if annotationsJSON.Valid && annotationsJSON.String != "" { @@ -1247,12 +1241,12 @@ func GetLayersForManifest(db DBTX, manifestID int64) ([]Layer, error) { // InsertManifestReference inserts a new manifest reference record (for manifest lists/indexes) func InsertManifestReference(db DBTX, ref *ManifestReference) error { _, err := db.Exec(` - INSERT INTO manifest_references (manifest_id, digest, size, media_type, + INSERT INTO manifest_references (manifest_key, digest, size, media_type, platform_architecture, platform_os, platform_variant, platform_os_version, is_attestation, reference_index) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) - `, ref.ManifestID, ref.Digest, ref.Size, ref.MediaType, + `, ref.ManifestKey, ref.Digest, ref.Size, ref.MediaType, ref.PlatformArchitecture, ref.PlatformOS, ref.PlatformVariant, ref.PlatformOSVersion, ref.IsAttestation, ref.ReferenceIndex) @@ -1260,15 +1254,15 @@ func InsertManifestReference(db DBTX, ref *ManifestReference) error { } // GetManifestReferencesForManifest fetches all manifest references for a manifest list/index -func GetManifestReferencesForManifest(db DBTX, manifestID int64) ([]ManifestReference, error) { +func GetManifestReferencesForManifest(db DBTX, manifestKey string) ([]ManifestReference, error) { rows, err := db.Query(` - SELECT manifest_id, digest, size, media_type, + SELECT manifest_key, digest, size, media_type, platform_architecture, platform_os, platform_variant, platform_os_version, reference_index FROM manifest_references - WHERE manifest_id = ? + WHERE manifest_key = ? ORDER BY reference_index - `, manifestID) + `, manifestKey) if err != nil { return nil, err @@ -1279,7 +1273,7 @@ func GetManifestReferencesForManifest(db DBTX, manifestID int64) ([]ManifestRefe for rows.Next() { var r ManifestReference var arch, os, variant, osVersion sql.NullString - if err := rows.Scan(&r.ManifestID, &r.Digest, &r.Size, &r.MediaType, + if err := rows.Scan(&r.ManifestKey, &r.Digest, &r.Size, &r.MediaType, &arch, &os, &variant, &osVersion, &r.ReferenceIndex); err != nil { return nil, err @@ -1314,18 +1308,18 @@ func GetTopLevelManifests(db DBTX, did, repository string, limit, offset int, vi -- Get all digests that are children of manifest lists SELECT DISTINCT mr.digest FROM manifest_references mr - JOIN manifests m ON mr.manifest_id = m.id + JOIN manifests m ON mr.manifest_key = m.manifest_key WHERE m.did = ? AND m.repository = ? ) SELECT - m.id, m.did, m.repository, m.digest, m.media_type, + m.manifest_key, m.did, m.repository, m.digest, m.media_type, m.schema_version, m.created_at, m.config_digest, m.config_size, m.hold_endpoint, m.artifact_type, GROUP_CONCAT(DISTINCT t.tag) as tags, COUNT(DISTINCT mr.digest) as platform_count FROM manifests m LEFT JOIN tags t ON m.digest = t.digest AND m.did = t.did AND m.repository = t.repository - LEFT JOIN manifest_references mr ON m.id = mr.manifest_id + LEFT JOIN manifest_references mr ON m.manifest_key = mr.manifest_key WHERE m.did = ? AND m.repository = ? AND m.subject_digest IS NULL AND m.artifact_type != 'unknown' @@ -1337,7 +1331,7 @@ func GetTopLevelManifests(db DBTX, did, repository string, limit, offset int, vi -- Include single-arch NOT referenced by any list m.digest NOT IN (SELECT digest FROM manifest_list_children WHERE digest IS NOT NULL) ) - GROUP BY m.id + GROUP BY m.manifest_key ORDER BY m.created_at DESC LIMIT ? OFFSET ? `, did, repository, did, repository, viewerDID, viewerDID, limit, offset) @@ -1354,7 +1348,7 @@ func GetTopLevelManifests(db DBTX, did, repository string, limit, offset int, vi var configSize sql.NullInt64 if err := rows.Scan( - &m.ID, &m.DID, &m.Repository, &m.Digest, &m.MediaType, + &m.Key, &m.DID, &m.Repository, &m.Digest, &m.MediaType, &m.SchemaVersion, &m.CreatedAt, &configDigest, &configSize, &m.HoldEndpoint, &m.ArtifactType, &tags, &m.PlatformCount, @@ -1393,12 +1387,12 @@ func GetTopLevelManifests(db DBTX, did, repository string, limit, offset int, vi COALESCE(mr.is_attestation, 0) as is_attestation, COALESCE(mr.digest, '') as child_digest, COALESCE(child_m.hold_endpoint, '') as child_hold_endpoint, - COALESCE((SELECT SUM(l.size) FROM layers l WHERE l.manifest_id = child_m.id), 0) as compressed_size + COALESCE((SELECT SUM(l.size) FROM layers l WHERE l.manifest_key = child_m.manifest_key), 0) as compressed_size FROM manifest_references mr LEFT JOIN manifests child_m ON mr.digest = child_m.digest AND child_m.did = ? AND child_m.repository = ? - WHERE mr.manifest_id = ? + WHERE mr.manifest_key = ? ORDER BY mr.reference_index - `, manifests[i].DID, manifests[i].Repository, manifests[i].ID) + `, manifests[i].DID, manifests[i].Repository, manifests[i].Key) if err != nil { return nil, err @@ -1442,16 +1436,16 @@ func GetManifestDetail(db DBTX, did, repository, digest string) (*ManifestWithMe err := db.QueryRow(` SELECT - m.id, m.did, m.repository, m.digest, m.media_type, + m.manifest_key, m.did, m.repository, m.digest, m.media_type, m.schema_version, m.created_at, m.config_digest, m.config_size, m.hold_endpoint, m.artifact_type, GROUP_CONCAT(DISTINCT t.tag) as tags FROM manifests m LEFT JOIN tags t ON m.digest = t.digest AND m.did = t.did AND m.repository = t.repository WHERE m.did = ? AND m.repository = ? AND m.digest = ? - GROUP BY m.id + GROUP BY m.manifest_key `, did, repository, digest).Scan( - &m.ID, &m.DID, &m.Repository, &m.Digest, &m.MediaType, + &m.Key, &m.DID, &m.Repository, &m.Digest, &m.MediaType, &m.SchemaVersion, &m.CreatedAt, &configDigest, &configSize, &m.HoldEndpoint, &m.ArtifactType, &tags, @@ -1491,12 +1485,12 @@ func GetManifestDetail(db DBTX, did, repository, digest string) (*ManifestWithMe COALESCE(mr.is_attestation, 0) as is_attestation, COALESCE(mr.digest, '') as child_digest, COALESCE(child_m.hold_endpoint, '') as child_hold_endpoint, - COALESCE((SELECT SUM(l.size) FROM layers l WHERE l.manifest_id = child_m.id), 0) as compressed_size + COALESCE((SELECT SUM(l.size) FROM layers l WHERE l.manifest_key = child_m.manifest_key), 0) as compressed_size FROM manifest_references mr LEFT JOIN manifests child_m ON mr.digest = child_m.digest AND child_m.did = ? AND child_m.repository = ? - WHERE mr.manifest_id = ? + WHERE mr.manifest_key = ? ORDER BY mr.reference_index - `, m.DID, m.Repository, m.ID) + `, m.DID, m.Repository, m.Key) if err != nil { return nil, err @@ -1610,7 +1604,7 @@ func GetChildManifestPlatform(db DBTX, did, repository, digest string) (*Platfor COALESCE(mr.platform_variant, ''), COALESCE(mr.platform_os_version, '') FROM manifest_references mr - JOIN manifests m ON mr.manifest_id = m.id + JOIN manifests m ON mr.manifest_key = m.manifest_key WHERE m.did = ? AND m.repository = ? AND mr.digest = ? LIMIT 1 `, did, repository, digest).Scan(&os, &arch, &variant, &osVersion) @@ -1635,7 +1629,7 @@ func IsManifestReferenced(db DBTX, did, digest string) (bool, error) { var count int err := db.QueryRow(` SELECT COUNT(*) FROM manifest_references mr - JOIN manifests m ON mr.manifest_id = m.id + JOIN manifests m ON mr.manifest_key = m.manifest_key WHERE mr.digest = ? AND m.did = ? LIMIT 1 `, digest, did).Scan(&count) @@ -1755,11 +1749,11 @@ func GetAllUntaggedManifestDigests(db DBTX, did, repository string) ([]string, e WITH manifest_list_children AS ( SELECT DISTINCT mr.digest FROM manifest_references mr - JOIN manifests m ON mr.manifest_id = m.id + JOIN manifests m ON mr.manifest_key = m.manifest_key WHERE m.did = ? AND m.repository = ? ), untagged_top_level AS ( - SELECT m.id, m.digest, + SELECT m.manifest_key, m.digest, CASE WHEN m.media_type LIKE '%index%' OR m.media_type LIKE '%manifest.list%' THEN 1 ELSE 0 END as is_list FROM manifests m @@ -1771,13 +1765,13 @@ func GetAllUntaggedManifestDigests(db DBTX, did, repository string) ([]string, e OR m.digest NOT IN (SELECT digest FROM manifest_list_children WHERE digest IS NOT NULL) ) - GROUP BY m.id + GROUP BY m.manifest_key HAVING COUNT(t.tag) = 0 ), untagged_children AS ( SELECT DISTINCT mr.digest FROM untagged_top_level ul - JOIN manifest_references mr ON ul.id = mr.manifest_id + JOIN manifest_references mr ON ul.manifest_key = mr.manifest_key JOIN manifests child_m ON mr.digest = child_m.digest AND child_m.did = ? AND child_m.repository = ? LEFT JOIN tags ct ON child_m.digest = ct.digest @@ -1785,7 +1779,7 @@ func GetAllUntaggedManifestDigests(db DBTX, did, repository string) ([]string, e WHERE ul.is_list = 1 AND ct.tag IS NULL AND mr.digest NOT IN ( SELECT mr2.digest FROM manifest_references mr2 - JOIN manifests m2 ON mr2.manifest_id = m2.id + JOIN manifests m2 ON mr2.manifest_key = m2.manifest_key JOIN tags t2 ON m2.digest = t2.digest AND m2.did = t2.did AND m2.repository = t2.repository WHERE m2.did = ? AND m2.repository = ? ) @@ -1818,46 +1812,44 @@ func GetAllUntaggedManifestDigests(db DBTX, did, repository string) ([]string, e // GetAttestationDetails returns attestation manifests and their layers for a manifest list. // Joins manifest_references (is_attestation=true) → manifests → layers. func GetAttestationDetails(db DBTX, did, repository, manifestListDigest string) ([]AttestationDetail, error) { - // Step 1: Get the manifest list ID and hold endpoint - var manifestListID int64 + // Step 1: Get the manifest list key and hold endpoint + var manifestListKey string var parentHoldEndpoint string err := db.QueryRow(` - SELECT id, hold_endpoint FROM manifests + SELECT manifest_key, hold_endpoint FROM manifests WHERE did = ? AND repository = ? AND digest = ? - `, did, repository, manifestListDigest).Scan(&manifestListID, &parentHoldEndpoint) + `, did, repository, manifestListDigest).Scan(&manifestListKey, &parentHoldEndpoint) if err != nil { return nil, err } // Step 2: Get attestation references and join to their manifest records rows, err := db.Query(` - SELECT mr.digest, mr.media_type, mr.size, m.id + SELECT mr.digest, mr.media_type, mr.size, m.manifest_key FROM manifest_references mr LEFT JOIN manifests m ON m.digest = mr.digest AND m.did = ? AND m.repository = ? - WHERE mr.manifest_id = ? AND mr.is_attestation = 1 + WHERE mr.manifest_key = ? AND mr.is_attestation = 1 ORDER BY mr.reference_index - `, did, repository, manifestListID) + `, did, repository, manifestListKey) if err != nil { return nil, err } defer rows.Close() type refRow struct { - digest string - mediaType string - size int64 - manifestID *int64 // may be NULL if attestation manifest not indexed yet + digest string + mediaType string + size int64 + manifestKey string // empty if the attestation manifest is not indexed yet } var refs []refRow for rows.Next() { var r refRow - var mid sql.NullInt64 - if err := rows.Scan(&r.digest, &r.mediaType, &r.size, &mid); err != nil { + var key sql.NullString + if err := rows.Scan(&r.digest, &r.mediaType, &r.size, &key); err != nil { return nil, err } - if mid.Valid { - r.manifestID = &mid.Int64 - } + r.manifestKey = key.String refs = append(refs, r) } if err := rows.Err(); err != nil { @@ -1874,8 +1866,8 @@ func GetAttestationDetails(db DBTX, did, repository, manifestListDigest string) Size: ref.size, HoldEndpoint: parentHoldEndpoint, } - if ref.manifestID != nil { - layers, err := GetLayersForManifest(db, *ref.manifestID) + if ref.manifestKey != "" { + layers, err := GetLayersForManifest(db, ref.manifestKey) if err != nil { return nil, err } @@ -2039,7 +2031,7 @@ func GetRepository(db DBTX, did, repository string) (*Repository, error) { // Get manifests for this repo manifestRows, err := db.Query(` - SELECT id, digest, hold_endpoint, schema_version, media_type, + SELECT manifest_key, digest, hold_endpoint, schema_version, media_type, config_digest, config_size, artifact_type, created_at FROM manifests WHERE did = ? AND repository = ? @@ -2055,7 +2047,7 @@ func GetRepository(db DBTX, did, repository string) (*Repository, error) { m.DID = did m.Repository = repository - if err := manifestRows.Scan(&m.ID, &m.Digest, &m.HoldEndpoint, &m.SchemaVersion, + if err := manifestRows.Scan(&m.Key, &m.Digest, &m.HoldEndpoint, &m.SchemaVersion, &m.MediaType, &m.ConfigDigest, &m.ConfigSize, &m.ArtifactType, &m.CreatedAt); err != nil { manifestRows.Close() return nil, err @@ -2370,10 +2362,15 @@ func GetRepoCards(db DBTX, limit int, currentUserDID string, sortOrder RepoCardS query := ` WITH latest_manifests AS ( - SELECT did, repository, MAX(id) as latest_id - FROM manifests - WHERE hold_endpoint IN ` + accessibleHoldsSubquery + ` - GROUP BY did, repository + SELECT did, repository, manifest_key AS latest_key FROM ( + SELECT did, repository, manifest_key, + ROW_NUMBER() OVER ( + PARTITION BY did, repository + ORDER BY created_at DESC, manifest_key DESC + ) AS rn + FROM manifests + WHERE hold_endpoint IN ` + accessibleHoldsSubquery + ` + ) WHERE rn = 1 ) SELECT m.did, @@ -2392,7 +2389,7 @@ func GetRepoCards(db DBTX, limit int, currentUserDID string, sortOrder RepoCardS MAX(COALESCE(rs.last_push, m.created_at), m.created_at), COALESCE(rp.avatar_cid, '') FROM latest_manifests lm - JOIN manifests m ON lm.latest_id = m.id + JOIN manifests m ON lm.latest_key = m.manifest_key JOIN users u ON m.did = u.did LEFT JOIN repository_stats rs ON m.did = rs.did AND m.repository = rs.repository LEFT JOIN repo_pages rp ON m.did = rp.did AND m.repository = rp.repository @@ -2448,11 +2445,16 @@ func GetRepoCards(db DBTX, limit int, currentUserDID string, sortOrder RepoCardS func GetUserRepoCards(db DBTX, userDID string, currentUserDID string) ([]RepoCardData, error) { query := ` WITH latest_manifests AS ( - SELECT did, repository, MAX(id) as latest_id - FROM manifests - WHERE did = ? - AND hold_endpoint IN ` + accessibleHoldsSubquery + ` - GROUP BY did, repository + SELECT did, repository, manifest_key AS latest_key FROM ( + SELECT did, repository, manifest_key, + ROW_NUMBER() OVER ( + PARTITION BY did, repository + ORDER BY created_at DESC, manifest_key DESC + ) AS rn + FROM manifests + WHERE did = ? + AND hold_endpoint IN ` + accessibleHoldsSubquery + ` + ) WHERE rn = 1 ) SELECT m.did, @@ -2471,7 +2473,7 @@ func GetUserRepoCards(db DBTX, userDID string, currentUserDID string) ([]RepoCar MAX(COALESCE(rs.last_push, m.created_at), m.created_at), COALESCE(rp.avatar_cid, '') FROM latest_manifests lm - JOIN manifests m ON lm.latest_id = m.id + JOIN manifests m ON lm.latest_key = m.manifest_key JOIN users u ON m.did = u.did LEFT JOIN repository_stats rs ON m.did = rs.did AND m.repository = rs.repository LEFT JOIN repo_pages rp ON m.did = rp.did AND m.repository = rp.repository @@ -2534,11 +2536,16 @@ func GetStarredRepoCards(db DBTX, starrerDID string, currentUserDID string) ([]R WHERE starrer_did = ? ), latest_manifests AS ( - SELECT m.did, m.repository, MAX(m.id) as latest_id - FROM manifests m - JOIN starred st ON m.did = st.did AND m.repository = st.repository - WHERE m.hold_endpoint IN ` + accessibleHoldsSubquery + ` - GROUP BY m.did, m.repository + SELECT did, repository, manifest_key AS latest_key FROM ( + SELECT m.did, m.repository, m.manifest_key, + ROW_NUMBER() OVER ( + PARTITION BY m.did, m.repository + ORDER BY m.created_at DESC, m.manifest_key DESC + ) AS rn + FROM manifests m + JOIN starred st ON m.did = st.did AND m.repository = st.repository + WHERE m.hold_endpoint IN ` + accessibleHoldsSubquery + ` + ) WHERE rn = 1 ) SELECT m.did, @@ -2558,7 +2565,7 @@ func GetStarredRepoCards(db DBTX, starrerDID string, currentUserDID string) ([]R COALESCE(rp.avatar_cid, ''), st.starred_at FROM latest_manifests lm - JOIN manifests m ON lm.latest_id = m.id + JOIN manifests m ON lm.latest_key = m.manifest_key JOIN users u ON m.did = u.did JOIN starred st ON st.did = m.did AND st.repository = m.repository LEFT JOIN repository_stats rs ON m.did = rs.did AND m.repository = rs.repository @@ -3133,7 +3140,7 @@ func GetLayerCountForManifest(db DBTX, did, repository, digest string) (int, err var count int err := db.QueryRow(` SELECT COUNT(*) FROM layers l - JOIN manifests m ON l.manifest_id = m.id + JOIN manifests m ON l.manifest_key = m.manifest_key WHERE m.did = ? AND m.repository = ? AND m.digest = ? `, did, repository, digest).Scan(&count) return count, err @@ -3196,24 +3203,3 @@ func GetDailyStats(db DBTX, did, repository, startDate, endDate string) ([]Daily } return stats, rows.Err() } - -// ManifestKeyBackfillProgress reports how far the manifest_key fill has got: -// the number of manifests still missing a key, and the total. -// -// manifest_key is populated by the ordinary Jetstream backfill rather than by a -// migration, because the value is a truncated sha256 and SQLite cannot compute -// it. That makes "is it done yet" a question worth being able to answer: the -// follow-up migration that makes the column NOT NULL, moves layers and -// manifest_references onto it, and drops id is only safe once remaining is zero. -func ManifestKeyBackfillProgress(db DBTX) (remaining, total int64, err error) { - err = db.QueryRow(` - SELECT - COUNT(*) FILTER (WHERE manifest_key IS NULL), - COUNT(*) - FROM manifests - `).Scan(&remaining, &total) - if err != nil { - return 0, 0, fmt.Errorf("manifest_key backfill progress: %w", err) - } - return remaining, total, nil -} diff --git a/pkg/appview/db/queries_test.go b/pkg/appview/db/queries_test.go index 7cc043c..bd11630 100644 --- a/pkg/appview/db/queries_test.go +++ b/pkg/appview/db/queries_test.go @@ -36,9 +36,10 @@ func TestGetRepositoryMetadata(t *testing.T) { // Test 2: Insert manifest and annotations _, err = db.Exec(` - INSERT INTO manifests (did, repository, digest, hold_endpoint, schema_version, media_type, created_at) - VALUES (?, ?, ?, ?, ?, ?, ?) - `, testUser.DID, "myapp", "sha256:abc123", "did:web:hold.example.com", 2, "application/vnd.oci.image.manifest.v1+json", + INSERT INTO manifests (manifest_key, did, repository, digest, hold_endpoint, schema_version, media_type, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + `, ManifestKey(testUser.DID, "myapp", "sha256:abc123"), + testUser.DID, "myapp", "sha256:abc123", "did:web:hold.example.com", 2, "application/vnd.oci.image.manifest.v1+json", time.Now().Add(-2*time.Hour)) if err != nil { t.Fatalf("Failed to insert manifest: %v", err) @@ -84,9 +85,10 @@ func TestGetRepositoryMetadata(t *testing.T) { // Test 4: Insert newer manifest with different annotations _, err = db.Exec(` - INSERT INTO manifests (did, repository, digest, hold_endpoint, schema_version, media_type, created_at) - VALUES (?, ?, ?, ?, ?, ?, ?) - `, testUser.DID, "myapp", "sha256:def456", "did:web:hold.example.com", 2, "application/vnd.oci.image.manifest.v1+json", + INSERT INTO manifests (manifest_key, did, repository, digest, hold_endpoint, schema_version, media_type, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + `, ManifestKey(testUser.DID, "myapp", "sha256:def456"), + testUser.DID, "myapp", "sha256:def456", "did:web:hold.example.com", 2, "application/vnd.oci.image.manifest.v1+json", time.Now()) // Most recent if err != nil { t.Fatalf("Failed to insert newer manifest: %v", err) @@ -123,9 +125,10 @@ func TestGetRepositoryMetadata(t *testing.T) { // Test 6: Manifest with NULL metadata fields _, err = db.Exec(` - INSERT INTO manifests (did, repository, digest, hold_endpoint, schema_version, media_type, created_at) - VALUES (?, ?, ?, ?, ?, ?, ?) - `, testUser.DID, "minimal-app", "sha256:minimal", "did:web:hold.example.com", 2, "application/vnd.oci.image.manifest.v1+json", time.Now()) + INSERT INTO manifests (manifest_key, did, repository, digest, hold_endpoint, schema_version, media_type, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + `, ManifestKey(testUser.DID, "minimal-app", "sha256:minimal"), + testUser.DID, "minimal-app", "sha256:minimal", "did:web:hold.example.com", 2, "application/vnd.oci.image.manifest.v1+json", time.Now()) if err != nil { t.Fatalf("Failed to insert minimal manifest: %v", err) } @@ -178,8 +181,8 @@ func TestInsertManifest(t *testing.T) { if err != nil { t.Fatalf("Failed to insert manifest: %v", err) } - if id1 == 0 { - t.Error("Expected non-zero manifest ID") + if id1 == "" { + t.Error("Expected a manifest key") } // Insert annotations separately @@ -202,8 +205,8 @@ func TestInsertManifest(t *testing.T) { if err != nil { t.Fatalf("Failed to retrieve manifest: %v", err) } - if retrieved.ID != id1 { - t.Errorf("Expected ID %d, got %d", id1, retrieved.ID) + if retrieved.Key != id1 { + t.Errorf("Expected key %s, got %s", id1, retrieved.Key) } // Verify annotations were inserted @@ -233,7 +236,7 @@ func TestInsertManifest(t *testing.T) { if err != nil { t.Fatalf("Failed to insert minimal manifest: %v", err) } - if id2 == 0 { + if id2 == "" { t.Error("Expected non-zero manifest ID for minimal manifest") } @@ -267,9 +270,9 @@ func TestInsertManifest(t *testing.T) { if err != nil { t.Fatalf("Failed to upsert manifest: %v", err) } - // ID should be the same as the original insert (UPDATE, not INSERT) + // Key should be the same as the original insert (UPDATE, not INSERT) if id3 != id1 { - t.Errorf("Expected upsert to return same ID %d, got %d", id1, id3) + t.Errorf("Expected upsert to return the same key %s, got %s", id1, id3) } // Update annotations separately @@ -922,7 +925,7 @@ func TestGetTagsWithPlatforms(t *testing.T) { // Add manifest references with platform info ref1 := &ManifestReference{ - ManifestID: manifestID2, + ManifestKey: manifestID2, Digest: "sha256:amd64", Size: 1000, MediaType: "application/vnd.oci.image.manifest.v1+json", @@ -931,7 +934,7 @@ func TestGetTagsWithPlatforms(t *testing.T) { ReferenceIndex: 0, } ref2 := &ManifestReference{ - ManifestID: manifestID2, + ManifestKey: manifestID2, Digest: "sha256:arm64", Size: 1000, MediaType: "application/vnd.oci.image.manifest.v1+json", @@ -1302,11 +1305,11 @@ func TestDeleteUserData(t *testing.T) { // Add layer layer := &Layer{ - ManifestID: manifestID, - LayerIndex: 0, - Digest: "sha256:layer1", - Size: 1000, - MediaType: "application/vnd.oci.image.layer.v1.tar+gzip", + ManifestKey: manifestID, + LayerIndex: 0, + Digest: "sha256:layer1", + Size: 1000, + MediaType: "application/vnd.oci.image.layer.v1.tar+gzip", } if err := InsertLayer(db, layer); err != nil { t.Fatalf("Failed to insert layer: %v", err) @@ -1341,7 +1344,7 @@ func TestDeleteUserData(t *testing.T) { if count != 1 { t.Fatalf("Expected 1 tag, got %d", count) } - db.QueryRow(`SELECT COUNT(*) FROM layers WHERE manifest_id = ?`, manifestID).Scan(&count) + db.QueryRow(`SELECT COUNT(*) FROM layers WHERE manifest_key = ?`, manifestID).Scan(&count) if count != 1 { t.Fatalf("Expected 1 layer, got %d", count) } @@ -1364,7 +1367,7 @@ func TestDeleteUserData(t *testing.T) { if count != 0 { t.Errorf("Expected 0 tags after cascade delete, got %d", count) } - db.QueryRow(`SELECT COUNT(*) FROM layers WHERE manifest_id = ?`, manifestID).Scan(&count) + db.QueryRow(`SELECT COUNT(*) FROM layers WHERE manifest_key = ?`, manifestID).Scan(&count) if count != 0 { t.Errorf("Expected 0 layers after cascade delete, got %d", count) } @@ -1396,22 +1399,23 @@ func TestIsManifestReferenced(t *testing.T) { // Insert a manifest list _, err = db.Exec(` - INSERT INTO manifests (did, repository, digest, hold_endpoint, schema_version, media_type, created_at) - VALUES (?, ?, ?, ?, ?, ?, ?) - `, "did:plc:test123", "myapp", "sha256:indexabc", "did:web:hold.example.com", 2, + INSERT INTO manifests (manifest_key, did, repository, digest, hold_endpoint, schema_version, media_type, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + `, ManifestKey("did:plc:test123", "myapp", "sha256:indexabc"), + "did:plc:test123", "myapp", "sha256:indexabc", "did:web:hold.example.com", 2, "application/vnd.oci.image.index.v1+json", time.Now()) if err != nil { t.Fatalf("Failed to insert manifest list: %v", err) } - var manifestID int64 - db.QueryRow(`SELECT id FROM manifests WHERE digest = ?`, "sha256:indexabc").Scan(&manifestID) + var manifestKey string + db.QueryRow(`SELECT manifest_key FROM manifests WHERE digest = ?`, "sha256:indexabc").Scan(&manifestKey) // Insert a child manifest reference _, err = db.Exec(` - INSERT INTO manifest_references (manifest_id, digest, media_type, size, platform_architecture, platform_os, reference_index) + INSERT INTO manifest_references (manifest_key, digest, media_type, size, platform_architecture, platform_os, reference_index) VALUES (?, ?, ?, ?, ?, ?, ?) - `, manifestID, "sha256:childdef", "application/vnd.oci.image.manifest.v1+json", 1000, "amd64", "linux", 0) + `, manifestKey, "sha256:childdef", "application/vnd.oci.image.manifest.v1+json", 1000, "amd64", "linux", 0) if err != nil { t.Fatalf("Failed to insert manifest reference: %v", err) } @@ -1468,7 +1472,7 @@ func TestGetAllUntaggedManifestDigests(t *testing.T) { manifestType := "application/vnd.oci.image.manifest.v1+json" hold := "did:web:hold.example.com" - insertManifest := func(t *testing.T, digest, mediaType string) int64 { + insertManifest := func(t *testing.T, digest, mediaType string) string { t.Helper() id, err := InsertManifest(db, &Manifest{ DID: did, Repository: repo, Digest: digest, @@ -1481,10 +1485,10 @@ func TestGetAllUntaggedManifestDigests(t *testing.T) { return id } - insertRef := func(t *testing.T, parentID int64, childDigest string, idx int) { + insertRef := func(t *testing.T, parentKey string, childDigest string, idx int) { t.Helper() err := InsertManifestReference(db, &ManifestReference{ - ManifestID: parentID, + ManifestKey: parentKey, Digest: childDigest, Size: 1000, MediaType: manifestType, @@ -1767,8 +1771,8 @@ func TestGetUserRepositories_BulkGrouping(t *testing.T) { t.Errorf("repoA: expected 2 manifests, got %d", len(a.Manifests)) } // manifests ordered created_at DESC → a2 first - if len(a.Manifests) >= 2 && (a.Manifests[0].ID != manifestA2 || a.Manifests[1].ID != manifestA1) { - t.Errorf("repoA manifests out of order, want [a2, a1] got [%d, %d]", a.Manifests[0].ID, a.Manifests[1].ID) + if len(a.Manifests) >= 2 && (a.Manifests[0].Key != manifestA2 || a.Manifests[1].Key != manifestA1) { + t.Errorf("repoA manifests out of order, want [a2, a1] got [%s, %s]", a.Manifests[0].Key, a.Manifests[1].Key) } if a.Title != "Repo A Title" || a.Description != "alpha" { t.Errorf("repoA annotations not applied: title=%q desc=%q", a.Title, a.Description) @@ -1937,7 +1941,7 @@ func TestGetRepoCards_NullLastPushStillSortsByCreatedAt(t *testing.T) { // "fresh": recent push, but its stats row has last_push = NULL (pulled, // never push-recorded). "stale": older, with a real last_push. - if _, err := BatchInsertManifests(d, []Manifest{ + if err := BatchInsertManifests(d, []Manifest{ {DID: "did:plc:alice", Repository: "fresh", Digest: "sha256:fresh", HoldEndpoint: "did:web:hold", SchemaVersion: 2, MediaType: "application/vnd.oci.image.manifest.v1+json", ArtifactType: "container-image", CreatedAt: now}, {DID: "did:plc:alice", Repository: "stale", Digest: "sha256:stale", HoldEndpoint: "did:web:hold", SchemaVersion: 2, MediaType: "application/vnd.oci.image.manifest.v1+json", ArtifactType: "container-image", CreatedAt: old}, }); err != nil { diff --git a/pkg/appview/db/schema.sql b/pkg/appview/db/schema.sql index 72bba05..c9fc552 100644 --- a/pkg/appview/db/schema.sql +++ b/pkg/appview/db/schema.sql @@ -20,8 +20,12 @@ CREATE TABLE IF NOT EXISTS users ( ); CREATE INDEX IF NOT EXISTS idx_users_handle ON users(handle); +-- Keyed by manifest_key, a node-independent identity derived from +-- (did, repository, digest); see db.ManifestKey. There is no surrogate rowid: one +-- is allocated by whichever node performs the insert, so it is only a stable +-- identity while every write funnels through a single writer. CREATE TABLE IF NOT EXISTS manifests ( - id INTEGER PRIMARY KEY AUTOINCREMENT, + manifest_key TEXT PRIMARY KEY NOT NULL, did TEXT NOT NULL, repository TEXT NOT NULL, digest TEXT NOT NULL, @@ -33,23 +37,9 @@ CREATE TABLE IF NOT EXISTS manifests ( artifact_type TEXT NOT NULL DEFAULT 'container-image', -- container-image, helm-chart, unknown subject_digest TEXT, -- digest of the parent manifest (for attestations/referrers) created_at TIMESTAMP NOT NULL, - -- Node-independent identity: hex(sha256(did || 0 || repository || 0 || digest)) - -- truncated to 16 bytes. See db.ManifestKey. - -- - -- Not yet load-bearing. id is still the primary key and layers / - -- manifest_references still reference it; this column is being populated - -- first so the swap can happen later against data that is already complete - -- and already proven unique. Nullable only because rows predating it are - -- filled in by the ordinary Jetstream backfill rather than all at once. - manifest_key TEXT, UNIQUE(did, repository, digest), FOREIGN KEY(did) REFERENCES users(did) ON DELETE CASCADE ); --- UNIQUE despite the column being nullable: SQLite treats NULLs as distinct, so --- unfilled rows do not collide, while every filled row is checked. That turns --- the 16-byte truncation from an assumption into something production data --- verifies before anything depends on it as a key. -CREATE UNIQUE INDEX IF NOT EXISTS idx_manifests_manifest_key ON manifests(manifest_key); CREATE INDEX IF NOT EXISTS idx_manifests_did_repo ON manifests(did, repository); CREATE INDEX IF NOT EXISTS idx_manifests_created_at ON manifests(created_at DESC); CREATE INDEX IF NOT EXISTS idx_manifests_digest ON manifests(digest); @@ -69,19 +59,19 @@ CREATE INDEX IF NOT EXISTS idx_repository_annotations_did_repo ON repository_ann CREATE INDEX IF NOT EXISTS idx_repository_annotations_key ON repository_annotations(key); CREATE TABLE IF NOT EXISTS layers ( - manifest_id INTEGER NOT NULL, + manifest_key TEXT NOT NULL, digest TEXT NOT NULL, size INTEGER NOT NULL, media_type TEXT NOT NULL, layer_index INTEGER NOT NULL, annotations TEXT, - PRIMARY KEY(manifest_id, layer_index), - FOREIGN KEY(manifest_id) REFERENCES manifests(id) ON DELETE CASCADE + PRIMARY KEY(manifest_key, layer_index), + FOREIGN KEY(manifest_key) REFERENCES manifests(manifest_key) ON DELETE CASCADE ); CREATE INDEX IF NOT EXISTS idx_layers_digest ON layers(digest); CREATE TABLE IF NOT EXISTS manifest_references ( - manifest_id INTEGER NOT NULL, + manifest_key TEXT NOT NULL, digest TEXT NOT NULL, media_type TEXT NOT NULL, size INTEGER NOT NULL, @@ -91,8 +81,8 @@ CREATE TABLE IF NOT EXISTS manifest_references ( platform_os_version TEXT, is_attestation BOOLEAN DEFAULT FALSE, reference_index INTEGER NOT NULL, - PRIMARY KEY(manifest_id, reference_index), - FOREIGN KEY(manifest_id) REFERENCES manifests(id) ON DELETE CASCADE + PRIMARY KEY(manifest_key, reference_index), + FOREIGN KEY(manifest_key) REFERENCES manifests(manifest_key) ON DELETE CASCADE ); CREATE INDEX IF NOT EXISTS idx_manifest_references_digest ON manifest_references(digest); diff --git a/pkg/appview/handlers/delete_test.go b/pkg/appview/handlers/delete_test.go index 8a07a51..d0a0c24 100644 --- a/pkg/appview/handlers/delete_test.go +++ b/pkg/appview/handlers/delete_test.go @@ -152,9 +152,10 @@ func TestDeleteAccountHandler_SuccessfulDeletion(t *testing.T) { // Create some manifests for the user _, err := database.Exec(` - INSERT INTO manifests (did, repository, digest, hold_endpoint, schema_version, media_type, created_at) - VALUES (?, ?, ?, ?, ?, ?, ?) - `, testUser.DID, "myapp", "sha256:abc123", "did:web:hold.example.com", 2, + INSERT INTO manifests (manifest_key, did, repository, digest, hold_endpoint, schema_version, media_type, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + `, db.ManifestKey(testUser.DID, "myapp", "sha256:abc123"), + testUser.DID, "myapp", "sha256:abc123", "did:web:hold.example.com", 2, "application/vnd.oci.image.manifest.v1+json", time.Now()) if err != nil { t.Fatalf("Failed to create manifest: %v", err) diff --git a/pkg/appview/handlers/diff.go b/pkg/appview/handlers/diff.go index 531a1be..9af280e 100644 --- a/pkg/appview/handlers/diff.go +++ b/pkg/appview/handlers/diff.go @@ -419,7 +419,7 @@ func (h *ManifestDiffHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) } } - dbLayers, _ := db.GetLayersForManifest(h.ReadOnlyDB, layerManifest.ID) + dbLayers, _ := db.GetLayersForManifest(h.ReadOnlyDB, layerManifest.Key) var layers []LayerDetail var vulnData *vulnDetailsData diff --git a/pkg/appview/handlers/digest.go b/pkg/appview/handlers/digest.go index 4f6e43c..63d4368 100644 --- a/pkg/appview/handlers/digest.go +++ b/pkg/appview/handlers/digest.go @@ -129,7 +129,7 @@ func (h *DigestDetailHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) // Helm chart: skip OCI history / vuln / SBOM entirely. Fetch helm // chart metadata from the same config blob and the single tarball // layer from the DB. - dbLayers, err := db.GetLayersForManifest(h.ReadOnlyDB, manifest.ID) + dbLayers, err := db.GetLayersForManifest(h.ReadOnlyDB, manifest.Key) if err != nil { slog.Warn("Failed to fetch layers", "error", err) } @@ -144,7 +144,7 @@ func (h *DigestDetailHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) } } else { // Single manifest: fetch layers from DB - dbLayers, err := db.GetLayersForManifest(h.ReadOnlyDB, manifest.ID) + dbLayers, err := db.GetLayersForManifest(h.ReadOnlyDB, manifest.Key) if err != nil { slog.Warn("Failed to fetch layers", "error", err) } diff --git a/pkg/appview/handlers/digest_content.go b/pkg/appview/handlers/digest_content.go index d64fac3..55abd72 100644 --- a/pkg/appview/handlers/digest_content.go +++ b/pkg/appview/handlers/digest_content.go @@ -43,7 +43,7 @@ func (h *DigestContentHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) return } - dbLayers, err := db.GetLayersForManifest(h.ReadOnlyDB, manifest.ID) + dbLayers, err := db.GetLayersForManifest(h.ReadOnlyDB, manifest.Key) if err != nil { slog.Warn("Failed to fetch layers", "error", err) } diff --git a/pkg/appview/handlers/image_advisor.go b/pkg/appview/handlers/image_advisor.go index 4bd6dfc..02e4b0a 100644 --- a/pkg/appview/handlers/image_advisor.go +++ b/pkg/appview/handlers/image_advisor.go @@ -215,7 +215,7 @@ func (h *ImageAdvisorHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) } // Fetch layers for size info - dbLayers, err := db.GetLayersForManifest(h.ReadOnlyDB, manifest.ID) + dbLayers, err := db.GetLayersForManifest(h.ReadOnlyDB, manifest.Key) if err != nil { slog.Debug("Failed to fetch layers for advisor", "error", err) } diff --git a/pkg/appview/handlers/upgrade_banner.go b/pkg/appview/handlers/upgrade_banner.go index fbc996d..e3f26f2 100644 --- a/pkg/appview/handlers/upgrade_banner.go +++ b/pkg/appview/handlers/upgrade_banner.go @@ -75,7 +75,7 @@ func (h *UpgradeBannerHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) return } slog.Debug("Upgrade banner: fetched both manifests", - "currentID", currentManifest.ID, "newerID", newerManifest.ID, + "currentID", currentManifest.Key, "newerID", newerManifest.Key, "currentIsManifestList", currentManifest.IsManifestList, "newerIsManifestList", newerManifest.IsManifestList) // For multi-arch manifests, resolve to a common platform child @@ -134,8 +134,8 @@ func (h *UpgradeBannerHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) // fall through and show a basic banner without layer/vuln details. // Fetch layers for both - currentDBLayers, _ := db.GetLayersForManifest(h.ReadOnlyDB, currentManifestForLayers.ID) - newerDBLayers, _ := db.GetLayersForManifest(h.ReadOnlyDB, newerManifestForLayers.ID) + currentDBLayers, _ := db.GetLayersForManifest(h.ReadOnlyDB, currentManifestForLayers.Key) + newerDBLayers, _ := db.GetLayersForManifest(h.ReadOnlyDB, newerManifestForLayers.Key) // Build layer details (try to get config history for richer diff) var currentLayers, newerLayers []LayerDetail diff --git a/pkg/appview/jetstream/backfill_batch.go b/pkg/appview/jetstream/backfill_batch.go index dd6a8a4..1bd9b3d 100644 --- a/pkg/appview/jetstream/backfill_batch.go +++ b/pkg/appview/jetstream/backfill_batch.go @@ -86,12 +86,13 @@ func (b *BackfillWorker) batchManifests(ctx context.Context, did string, records for i, d := range decodedRecords { manifests[i] = d.manifest } - ids, err := db.BatchInsertManifests(b.db, manifests) - if err != nil { + if err := db.BatchInsertManifests(b.db, manifests); err != nil { return 0, fmt.Errorf("batch insert manifests: %w", err) } - // Phase 2: derive layers, references, and annotations using the returned ids. + // Phase 2: derive layers, references and annotations. The manifest key is + // computed locally rather than read back from the database, so there is no + // window in which a row exists without us knowing how to reference it. var ( layerRows []db.Layer refRows []db.ManifestReference @@ -107,15 +108,7 @@ func (b *BackfillWorker) batchManifests(ctx context.Context, did string, records newestByRepo := make(map[string]newest) for _, d := range decodedRecords { - mid, ok := ids[db.ManifestKey(did, d.manifest.Repository, d.manifest.Digest)] - if !ok { - // BatchInsertManifests did not return an id for this row — either the - // row was constraint-rejected or the SELECT missed it. Skip its - // dependent rows rather than inserting with id 0. - slog.Warn("Backfill manifest missing id after batch insert", - "did", did, "repository", d.manifest.Repository, "digest", d.manifest.Digest) - continue - } + mkey := db.ManifestKey(did, d.manifest.Repository, d.manifest.Digest) if len(d.manifestRecord.Manifests) > 0 { for i, ref := range d.manifestRecord.Manifests { @@ -131,7 +124,7 @@ func (b *BackfillWorker) batchManifests(ctx context.Context, did string, records isAttestation = refType == "attestation-manifest" } refRows = append(refRows, db.ManifestReference{ - ManifestID: mid, + ManifestKey: mkey, Digest: ref.Digest, MediaType: ref.MediaType, Size: ref.Size, @@ -146,7 +139,7 @@ func (b *BackfillWorker) batchManifests(ctx context.Context, did string, records } else { for i, layer := range d.manifestRecord.Layers { layerRows = append(layerRows, db.Layer{ - ManifestID: mid, + ManifestKey: mkey, Digest: layer.Digest, MediaType: layer.MediaType, Size: layer.Size, diff --git a/pkg/appview/jetstream/processor.go b/pkg/appview/jetstream/processor.go index 3eff352..da2fdf9 100644 --- a/pkg/appview/jetstream/processor.go +++ b/pkg/appview/jetstream/processor.go @@ -322,11 +322,11 @@ func (p *Processor) ProcessRecord(ctx context.Context, did, collection, rkey str // ProcessManifest processes a manifest record and stores it in the database // Returns the manifest ID for further processing (layers/references) -func (p *Processor) ProcessManifest(ctx context.Context, did string, recordData []byte) (int64, error) { +func (p *Processor) ProcessManifest(ctx context.Context, did string, recordData []byte) (string, error) { // Unmarshal manifest record var manifestRecord atproto.ManifestRecord if err := json.Unmarshal(recordData, &manifestRecord); err != nil { - return 0, fmt.Errorf("failed to unmarshal manifest: %w", err) + return "", fmt.Errorf("failed to unmarshal manifest: %w", err) } // Detect manifest type isManifestList := len(manifestRecord.Manifests) > 0 @@ -374,24 +374,13 @@ func (p *Processor) ProcessManifest(ctx context.Context, did string, recordData manifest.SubjectDigest = manifestRecord.Subject.Digest } - // Insert manifest - manifestID, err := db.InsertManifest(p.db, manifest) + // Insert manifest. The key is derived from (did, repository, digest), so it + // is known whether the row was inserted, updated or already present; there is + // no need to read an identifier back on a constraint conflict the way there + // was with a rowid. + manifestKey, err := db.InsertManifest(p.db, manifest) if err != nil { - // For backfill: if manifest already exists, get its ID - if strings.Contains(err.Error(), "UNIQUE constraint failed") { - var existingID int64 - err := p.db.QueryRow(` - SELECT id FROM manifests - WHERE did = ? AND repository = ? AND digest = ? - `, manifest.DID, manifest.Repository, manifest.Digest).Scan(&existingID) - - if err != nil { - return 0, fmt.Errorf("failed to get existing manifest ID: %w", err) - } - manifestID = existingID - } else { - return 0, fmt.Errorf("failed to insert manifest: %w", err) - } + return "", fmt.Errorf("failed to insert manifest: %w", err) } // Update repository annotations ONLY if manifest has at least one non-empty annotation @@ -408,7 +397,7 @@ func (p *Processor) ProcessManifest(ctx context.Context, did string, recordData // Replace all annotations for this repository err = db.UpsertRepositoryAnnotations(p.db, did, manifestRecord.Repository, manifestRecord.Annotations) if err != nil { - return 0, fmt.Errorf("failed to upsert annotations: %w", err) + return "", fmt.Errorf("failed to upsert annotations: %w", err) } } } @@ -438,7 +427,7 @@ func (p *Processor) ProcessManifest(ctx context.Context, did string, recordData } if err := db.InsertManifestReference(p.db, &db.ManifestReference{ - ManifestID: manifestID, + ManifestKey: manifestKey, Digest: ref.Digest, MediaType: ref.MediaType, Size: ref.Size, @@ -457,7 +446,7 @@ func (p *Processor) ProcessManifest(ctx context.Context, did string, recordData // Insert layers (for image manifests) for i, layer := range manifestRecord.Layers { if err := db.InsertLayer(p.db, &db.Layer{ - ManifestID: manifestID, + ManifestKey: manifestKey, Digest: layer.Digest, MediaType: layer.MediaType, Size: layer.Size, @@ -470,7 +459,7 @@ func (p *Processor) ProcessManifest(ctx context.Context, did string, recordData } } - return manifestID, nil + return manifestKey, nil } // ProcessTag processes a tag record and stores it in the database diff --git a/pkg/appview/jetstream/processor_test.go b/pkg/appview/jetstream/processor_test.go index be1a337..b1833e1 100644 --- a/pkg/appview/jetstream/processor_test.go +++ b/pkg/appview/jetstream/processor_test.go @@ -164,7 +164,7 @@ func TestProcessManifest_ImageManifest(t *testing.T) { if err != nil { t.Fatalf("ProcessManifest failed: %v", err) } - if manifestID == 0 { + if manifestID == "" { t.Error("Expected non-zero manifest ID") } @@ -201,7 +201,7 @@ func TestProcessManifest_ImageManifest(t *testing.T) { // Verify layers were inserted var layerCount int - err = database.QueryRow("SELECT COUNT(*) FROM layers WHERE manifest_id = ?", manifestID).Scan(&layerCount) + err = database.QueryRow("SELECT COUNT(*) FROM layers WHERE manifest_key = ?", manifestID).Scan(&layerCount) if err != nil { t.Fatalf("Failed to query layers: %v", err) } @@ -211,7 +211,7 @@ func TestProcessManifest_ImageManifest(t *testing.T) { // Verify no manifest references (this is an image, not a list) var refCount int - err = database.QueryRow("SELECT COUNT(*) FROM manifest_references WHERE manifest_id = ?", manifestID).Scan(&refCount) + err = database.QueryRow("SELECT COUNT(*) FROM manifest_references WHERE manifest_key = ?", manifestID).Scan(&refCount) if err != nil { t.Fatalf("Failed to query manifest_references: %v", err) } @@ -272,7 +272,7 @@ func TestProcessManifest_ManifestList(t *testing.T) { // Verify manifest references were inserted var refCount int - err = database.QueryRow("SELECT COUNT(*) FROM manifest_references WHERE manifest_id = ?", manifestID).Scan(&refCount) + err = database.QueryRow("SELECT COUNT(*) FROM manifest_references WHERE manifest_key = ?", manifestID).Scan(&refCount) if err != nil { t.Fatalf("Failed to query manifest_references: %v", err) } @@ -282,7 +282,7 @@ func TestProcessManifest_ManifestList(t *testing.T) { // Verify platform info was stored var arch, os string - err = database.QueryRow("SELECT platform_architecture, platform_os FROM manifest_references WHERE manifest_id = ? AND reference_index = 0", manifestID).Scan(&arch, &os) + err = database.QueryRow("SELECT platform_architecture, platform_os FROM manifest_references WHERE manifest_key = ? AND reference_index = 0", manifestID).Scan(&arch, &os) if err != nil { t.Fatalf("Failed to query platform info: %v", err) } @@ -295,7 +295,7 @@ func TestProcessManifest_ManifestList(t *testing.T) { // Verify no layers (this is a list, not an image) var layerCount int - err = database.QueryRow("SELECT COUNT(*) FROM layers WHERE manifest_id = ?", manifestID).Scan(&layerCount) + err = database.QueryRow("SELECT COUNT(*) FROM layers WHERE manifest_key = ?", manifestID).Scan(&layerCount) if err != nil { t.Fatalf("Failed to query layers: %v", err) } @@ -569,7 +569,7 @@ func TestProcessManifest_Duplicate(t *testing.T) { // Should return existing ID if id1 != id2 { - t.Errorf("Duplicate manifest got different ID: %d vs %d", id1, id2) + t.Errorf("Duplicate manifest got different key: %s vs %s", id1, id2) } // Verify only one manifest exists diff --git a/pkg/appview/server.go b/pkg/appview/server.go index 9dbc623..782db6f 100644 --- a/pkg/appview/server.go +++ b/pkg/appview/server.go @@ -201,8 +201,6 @@ func NewAppViewServer(cfg *Config, branding *BrandingOverrides) (*AppViewServer, slog.Info("Lease manager initialized", "component", "leases", "enabled", cfg.Leases.Enabled, "holder", s.Leases.HolderID()) - reportManifestKeyBackfill(s.Database) - // The cleanup worker starts later, once the hold authorizer exists: it // clears the crew denial backoffs as its first act and needs a handle on it. @@ -977,30 +975,6 @@ func (s *AppViewServer) waitForLeasedWorkers() { s.Leases.Wait(leaseDrainTimeout) } -// reportManifestKeyBackfill logs how much of manifests.manifest_key is still -// unfilled. -// -// The column is populated by the ordinary Jetstream backfill rather than by its -// migration, because the value is a truncated sha256 and SQLite cannot compute -// one. So there is no single moment at which it becomes complete, and the -// follow-up work that moves layers and manifest_references onto the key is only -// safe once this reaches zero. Logging it is how anyone finds out. -func reportManifestKeyBackfill(database *sql.DB) { - remaining, total, err := db.ManifestKeyBackfillProgress(database) - if err != nil { - slog.Warn("Could not check manifest_key backfill progress", "error", err) - return - } - if remaining == 0 { - slog.Info("manifest_key backfill complete", "manifests", total) - return - } - slog.Info("manifest_key backfill in progress", - "remaining", remaining, - "total", total, - "hint", "the Jetstream backfill fills these as it re-upserts each manifest") -} - // startCleanupWorker runs the periodic expiry sweep under the cleanup lease, // after clearing the crew denial backoffs once on acquiring it. //