From eb059a3d7161a2604dbb91389fbcb9562566aefc Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Mon, 17 Aug 2026 08:27:05 -0400 Subject: [PATCH] fix(database): retire a table whose shape the schema moved past MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `applySchema` is CREATE ... IF NOT EXISTS and there is no migration chain, so a *changed* table never migrates: the statement silently no-ops against the old shape. Two plans had already landed on that, and neither showed up in a test because a fresh install is perfectly healthy. - 014 added `total_tracks` to explore_index and to `indexRowFields`, the projection every explore read uses, so every search, browse, artist page and album page failed with "no such column: total_tracks" on any database that already had a catalog. - 013 reshaped audio_files, so applySchema could not run at all and the app did not open. staleshape.go runs before applySchema and drops what disagrees, so the create is a create. It parses sql/schemas/ for the expectation rather than writing the column list down a second time, and it notices a changed *type* as well as a missing column — 013 moved mbid TEXT to BLOB, which no ALTER could express and which SQLite will not coerce, so a query against 16 raw bytes returns no rows rather than an error. Only Authored tables are exempt. Cache is rebuildable by definition, Owned is what a rescan rebuilds (plan 013's stated "delete and rescan"), and a table the schema no longer describes at all goes too -- 013 left seven behind plus schema_migrations. Three things in it are load-bearing, and each was a bug first: - The parser read `UNIQUE(mbid)` as a column, which made a healthy catalog look stale. That would have retired it on every launch and cost every user an artifact download per start. - The drops are one transaction with defer_foreign_keys. Those legacy tables reference each other, so any order fails on whichever goes first; turning foreign keys off instead would suppress playlist_tracks.audio_file_id's ON DELETE SET NULL and leave entries pointing at ids a rescan reissues to *different songs*. Nulled entries are empty; stale ones are wrong, and wrong quietly. - The order is sorted, so a failure reproduces. Map order is random, and the foreign-key bug passed its own regression test on two runs in three until the order was fixed. Verified against a real pre-013 install: it opens, its 22 playlists survive, 1,887 linked playlist entries become 0 rather than dangling, and the legacy tables are swept. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AfVYUVExXsx1nSWrXN8mAh --- CLAUDE.md | 45 +++ backend/database/database.go | 8 + backend/database/staleshape.go | 552 ++++++++++++++++++++++++++++ backend/database/staleshape_test.go | 499 +++++++++++++++++++++++++ cmd/indexbuild/staleschema_test.go | 17 +- 5 files changed, 1117 insertions(+), 4 deletions(-) create mode 100644 backend/database/staleshape.go create mode 100644 backend/database/staleshape_test.go diff --git a/CLAUDE.md b/CLAUDE.md index bae21c5..bb4889d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -233,6 +233,51 @@ rather than renaming them. the drift it caused before — `sql/schemas/` and the migrations disagreed, and sqlc generated against the stale one. + **What that costs an existing database is repaired once, at open.** + `CREATE ... IF NOT EXISTS` reaches an existing table only if its shape + already matches and otherwise silently no-ops, so a *changed* table + never migrates. Plan 014 added `total_tracks` to `explore_index` and + to `indexRowFields` — the projection every explore read uses — and no + database that already existed grew the column: **every** Explore + search, browse, artist and album page on such an install failed with + `no such column: total_tracks`, while a fresh install was perfectly + healthy, which is exactly why no test saw it. Plan 013 was worse on + the same install: `applySchema` could not be applied at all over a + pre-013 `audio_files`, so the app did not open. + + `backend/database/staleshape.go` runs before `applySchema` and + retires what is stale, so the create is a create. Five things about + it are load-bearing: + - **It parses `sql/schemas/` for the expectation** rather than + writing the column list down a second time, because a second list + is a second thing to forget — the fault it exists to repair. + - **It notices a changed *type*, not just a missing column.** 013 + moved `mbid` from TEXT to BLOB, and SQLite does not coerce between + them: a comparison against 16 raw bytes returns no rows rather than + an error. `ALTER TABLE ADD COLUMN` would have handled + `total_tracks` alone and cannot express this at all, which is why + the repair drops rather than migrates. + - **`Authored` is never retired**, and that boundary is a test + (`TestAuthoredTablesAreNeverRetired`), not a comment. Everything + else is rebuildable: `Cache` by definition, `Owned` by a rescan — + plan 013's stated "delete and rescan" — and `Derived` from Owned. + A table the schema no longer describes at all goes too; 013 left + seven behind plus `schema_migrations`. + - **The drops are one transaction with `defer_foreign_keys`.** Those + legacy tables reference each other, so dropping them in any order + fails on whichever goes first, and turning foreign keys *off* + instead would silently take `playlist_tracks.audio_file_id`'s + ON DELETE SET NULL with it — leaving playlist entries pointing at + ids a rescan reissues to *different songs*. Nulled entries are + empty; stale ones are wrong, and wrong quietly. + - **The order is sorted, so a failure reproduces.** Map order is + random, and the foreign-key bug above passed its own regression + test on two runs in three until the order was fixed. + + Retiring `explore_index` takes its FTS and its meta with it, because + the `dump_import_done` marker is what would otherwise stop the + artifact ever being fetched again. + **What that costs an existing database is that it does not open**, and "delete and rescan" is the answer (plan 013, open question 1) — free for everyone except one machine. The index job's `/cache` volume is a diff --git a/backend/database/database.go b/backend/database/database.go index 16c9e73..a82fa72 100644 --- a/backend/database/database.go +++ b/backend/database/database.go @@ -89,6 +89,14 @@ func NewDB(logger *slog.Logger) (*DB, error) { return nil, fmt.Errorf("could not apply PRAGMAs: %w", err) } + // Before the schema is applied, not after: applySchema is + // CREATE ... IF NOT EXISTS, which no-ops against a table that + // already exists in an older shape. Retiring the stale one first is + // what turns that no-op into a create. + if err := retireStaleTables(dbCtx, db, logger); err != nil { + return nil, err + } + if err := applySchema(dbCtx, db); err != nil { return nil, err } diff --git a/backend/database/staleshape.go b/backend/database/staleshape.go new file mode 100644 index 0000000..118f249 --- /dev/null +++ b/backend/database/staleshape.go @@ -0,0 +1,552 @@ +package database + +import ( + "context" + "database/sql" + "fmt" + "io/fs" + "log/slog" + "maps" + "path" + "slices" + "strings" + + "yellowjacket/backend/datamap" +) + +// This file repairs the one thing `CREATE TABLE IF NOT EXISTS` cannot. +// +// `sql/schemas/` is the single description of the schema and there is no +// migration chain (plan 013): a schema change is one edit to one file. +// That works perfectly for a *new* table, which every install then +// creates, and not at all for a changed one -- `IF NOT EXISTS` reaches +// an existing table only if its shape already matches, and otherwise +// silently no-ops. The user's answer to that is "delete and rescan" +// (plan 013, open question 1), which is free for everything a rescan +// rebuilds. +// +// It is not free for the catalog. explore_index is a *downloaded +// artifact*, not something derived from the user's files, and it is the +// largest thing this app stores. So it went stale instead: plan 014 +// added `total_tracks` to the schema and to `indexRowFields` -- the one +// projection every explore read uses -- and no database that already +// existed ever grew the column. Every Explore search, browse, artist +// page and album page on such an install fails with +// "no such column: total_tracks", while a fresh install is perfectly +// healthy, which is why the tests did not see it. The same databases +// are stale a second way, from the same plan: their `mbid` columns are +// still TEXT where the schema now declares BLOB, and SQLite does not +// coerce between the two -- a comparison against 16 raw bytes simply +// returns no rows. +// +// The repair is to notice and drop, not to migrate. A dropped catalog +// costs one artifact download (about a minute); the alternative -- +// ALTER TABLE ADD COLUMN, which would handle `total_tracks` alone +// cheaply -- cannot express the TEXT-to-BLOB half at all, and would +// leave those installs quietly broken while reporting success. +// +// Everything except `Authored` is eligible. `Cache` is rebuildable by +// definition; `Owned` is a projection of the user's files and a rescan +// rebuilds it, which is plan 013's stated answer to exactly this +// situation ("delete and rescan", open question 1); `Derived` is +// computed from Owned. No `Authored` table is ever dropped here -- +// that is the whole point of the datamap, and it is asserted by +// TestAuthoredTablesAreNeverRetired rather than only stated. +// +// What that does *not* buy is immunity for authored rows that reference +// a retired table. `audio_files` is MIXED KIND: `play_count`, +// `last_played` and `tag_status` are authored columns on an Owned +// table, and they go with it. Playlists survive as playlists, and +// their entries survive pointing at nothing. That cost was weighed and +// accepted rather than overlooked -- the alternative is to carry the +// authored columns across the rebuild keyed on file_path, which stays a +// real option if this ever bites harder than it is worth. +// +// **This relies on foreign_keys being ON**, which applyPRAGMAs has +// already done by the time NewDB calls it, and the dependency is not +// cosmetic. SQLite performs an implicit DELETE before dropping a table +// when foreign keys are enabled, so `playlist_tracks.audio_file_id` -- +// declared ON DELETE SET NULL -- is nulled. With foreign keys off, no +// action fires and those rows keep the ids they had, which a rescan +// then reissues starting from 1: every playlist would silently fill +// with *different songs*. Nulled entries are merely empty; stale ones +// are wrong, and wrong quietly. TestRetiringOwnedTablesDoesNotDangle +// is what stops a future reordering turning one into the other. + +// retireGroups are tables that must be retired together. A catalog +// whose rows are gone must not keep the full-text index built over +// them, nor the metadata claiming the import that produced them +// finished -- that marker is exactly what stops the artifact being +// fetched again. applySchema recreates all three empty immediately +// afterwards, and the ordinary "no index yet" path takes over. +var retireGroups = [][]string{ + { + "explore_index", + "explore_index_fts", + "explore_index_meta", + "explore_champion_fts", + }, +} + +// schemaColumn is one column as the schema file declares it. +type schemaColumn struct { + name string + typ string +} + +// retireStaleTables drops every non-authored table whose live shape no +// longer matches what sql/schemas/ declares, plus any table the schema +// no longer describes at all, so applySchema can create the current +// shape afresh. It runs before applySchema and is a no-op on a new +// database, where the tables do not exist yet. +func retireStaleTables( + ctx context.Context, db *sql.DB, logger *slog.Logger, +) error { + declared, err := declaredTables() + if err != nil { + return err + } + + stale := make(map[string]string) + + for table, columns := range declared { + entry, ok := datamap.Lookup(table) + if !ok || entry.Kind == datamap.Authored || entry.FTS { + continue + } + + reason, err := staleReason(ctx, db, table, columns) + if err != nil { + return err + } + + if reason != "" { + stale[table] = reason + } + } + + obsolete, err := obsoleteTables(ctx, db) + if err != nil { + return err + } + + maps.Copy(stale, obsolete) + + if len(stale) == 0 { + return nil + } + + return retireGroupsFor(ctx, db, logger, stale) +} + +// obsoleteTables are live tables the schema no longer describes at all. +// TestCatalogCoversSchema makes the datamap a complete description of +// the current schema, so a table it does not know is one a past version +// created and this one does not -- plan 013 alone left seven behind +// (recordings, release_groups, artist_credit, artist_credit_artist, +// release_group_recordings, recording_genres) plus the +// schema_migrations table that squashing the chain retired. They are +// dead weight, and one of them holding a foreign key into a table being +// rebuilt is worse than dead weight. +// +// SQLite's own bookkeeping and FTS shadow tables are not obsolete: +// datamap.Lookup resolves a shadow table to its parent, and IsInternal +// covers the rest. +func obsoleteTables(ctx context.Context, db *sql.DB) (map[string]string, error) { + rows, err := db.QueryContext( + ctx, "SELECT name FROM sqlite_master WHERE type = 'table'", + ) + if err != nil { + return nil, fmt.Errorf("could not list tables: %w", err) + } + + defer func() { _ = rows.Close() }() + + out := make(map[string]string) + + for rows.Next() { + var name string + + if err := rows.Scan(&name); err != nil { + return nil, fmt.Errorf("could not scan table name: %w", err) + } + + if datamap.IsInternal(name) { + continue + } + + if _, known := datamap.Lookup(name); !known { + out[name] = "the schema no longer describes this table" + } + } + + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("could not read table list: %w", err) + } + + return out, nil +} + +// retireGroupsFor drops each stale table along with everything its +// retire group says must go with it. +func retireGroupsFor( + ctx context.Context, db *sql.DB, logger *slog.Logger, + stale map[string]string, +) error { + drop := make(map[string]string) + + for table, reason := range stale { + drop[table] = reason + + for _, group := range retireGroups { + if !slices.Contains(group, table) { + continue + } + + for _, member := range group { + if _, already := drop[member]; !already { + drop[member] = "retired with " + table + } + } + } + } + + return dropDeferred(ctx, db, logger, drop) +} + +// dropDeferred drops every named table in one transaction with foreign +// key enforcement deferred to the commit. +// +// The deferral is required and the two obvious alternatives are both +// wrong. These tables reference each other -- pre-013 `audio_files` +// has a foreign key into `recordings`, which is itself being retired -- +// so dropping them one at a time in an arbitrary order fails with +// "FOREIGN KEY constraint failed" on whichever is unlucky enough to go +// first, and there is no order that is safe in general. Turning +// foreign keys *off* for the duration would fix that and silently take +// the ON DELETE SET NULL on `playlist_tracks.audio_file_id` with it, +// leaving playlist entries pointing at ids a rescan reissues to +// different songs -- the exact failure +// TestRetiringOwnedTablesDoesNotDangle exists to prevent. +// +// Deferring keeps the actions firing while tolerating the inconsistency +// in the middle, and the commit then checks that the end state is +// sound. It is set inside the transaction because SQLite resets it at +// every commit. +func dropDeferred( + ctx context.Context, db *sql.DB, logger *slog.Logger, + drop map[string]string, +) error { + tx, err := db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("could not begin the retire transaction: %w", err) + } + + defer func() { _ = tx.Rollback() }() + + if _, err := tx.ExecContext(ctx, "PRAGMA defer_foreign_keys = ON"); err != nil { + return fmt.Errorf("could not defer foreign keys: %w", err) + } + + // Sorted, so a failure is reproducible. Map order is random, and a + // bug that depends on which table happens to go first reproduces on + // one run in three and passes review on the other two -- which is + // exactly how the foreign-key ordering above reached a real + // database. Sorting does not make any order *safe*; the deferral + // does that. + for _, table := range slices.Sorted(maps.Keys(drop)) { + logger.Warn( + "retiring a table the schema no longer describes", + "table", table, + "reason", drop[table], + ) + + if _, err := tx.ExecContext( + ctx, "DROP TABLE IF EXISTS "+quoteIdent(table), + ); err != nil { + return fmt.Errorf("could not retire stale table %s: %w", table, err) + } + } + + if err := tx.Commit(); err != nil { + return fmt.Errorf("could not commit the retire: %w", err) + } + + return nil +} + +// staleReason reports why a live table disagrees with its declaration, +// or "" when it agrees. A column the live table does not have is the +// additive case; a column whose declared type changed is the one an +// ALTER could not fix anyway. Columns the live table has and the +// schema no longer declares are ignored: they cost nothing and dropping +// the table over one would retire a healthy catalog. +func staleReason( + ctx context.Context, db *sql.DB, table string, columns []schemaColumn, +) (string, error) { + live, err := liveColumns(ctx, db, table) + if err != nil { + return "", err + } + + if len(live) == 0 { + // Not present at all: applySchema is about to create it. + return "", nil + } + + for _, col := range columns { + liveType, present := live[col.name] + if !present { + return "missing column " + col.name, nil + } + + if !sameDeclaredType(col.typ, liveType) { + return fmt.Sprintf( + "column %s is %s, schema declares %s", + col.name, liveType, col.typ, + ), nil + } + } + + return "", nil +} + +// liveColumns returns the live table's columns and their declared types, +// empty when the table does not exist. +func liveColumns( + ctx context.Context, db *sql.DB, table string, +) (map[string]string, error) { + rows, err := db.QueryContext( + ctx, "SELECT name, type FROM pragma_table_info(?)", table, + ) + if err != nil { + return nil, fmt.Errorf("could not inspect table %s: %w", table, err) + } + + defer func() { _ = rows.Close() }() + + out := make(map[string]string) + + for rows.Next() { + var name, typ string + + if err := rows.Scan(&name, &typ); err != nil { + return nil, fmt.Errorf("could not scan column of %s: %w", table, err) + } + + out[name] = typ + } + + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("could not read columns of %s: %w", table, err) + } + + return out, nil +} + +// sameDeclaredType compares two SQLite type names. They are compared +// case-insensitively and only on the leading word, so INTEGER matches +// INTEGER and VARCHAR(20) matches VARCHAR -- SQLite's affinity rules +// make finer distinctions meaningless, and a difference that fine is +// not worth retiring a catalog over. An empty declared type matches +// anything, which is what a column declared with only constraints has. +func sameDeclaredType(declared, live string) bool { + d := strings.ToUpper(strings.Fields(declared + " ")[0]) + l := strings.ToUpper(strings.Fields(live + " ")[0]) + + if d == "" || l == "" { + return true + } + + if i := strings.IndexByte(d, '('); i >= 0 { + d = d[:i] + } + + if i := strings.IndexByte(l, '('); i >= 0 { + l = l[:i] + } + + return d == l +} + +// declaredTables parses every CREATE TABLE in sql/schemas/ into its +// column list. Parsing the schema rather than writing the expectation +// down a second time is the point: a second list is a second thing to +// forget, which is the fault this whole file exists to repair. +func declaredTables() (map[string][]schemaColumn, error) { + dirEntries, err := schemas.ReadDir("sql/schemas") + if err != nil { + return nil, fmt.Errorf("could not read schemas directory: %w", err) + } + + out := make(map[string][]schemaColumn) + + for _, dirEntry := range dirEntries { + if dirEntry.IsDir() { + continue + } + + content, err := fs.ReadFile(schemas, path.Join("sql/schemas", dirEntry.Name())) + if err != nil { + return nil, fmt.Errorf("could not read %s: %w", dirEntry.Name(), err) + } + + maps.Copy(out, parseCreateTables(string(content))) + } + + return out, nil +} + +// constraintKeywords begin a table constraint rather than a column. +var constraintKeywords = map[string]bool{ + "PRIMARY": true, "FOREIGN": true, "UNIQUE": true, + "CHECK": true, "CONSTRAINT": true, +} + +// parseCreateTables extracts the column names and declared types of +// every non-virtual CREATE TABLE in one schema file. +func parseCreateTables(content string) map[string][]schemaColumn { + out := make(map[string][]schemaColumn) + rest := stripLineComments(content) + + for { + idx := indexFold(rest, "CREATE TABLE ") + if idx < 0 { + return out + } + + rest = rest[idx+len("CREATE TABLE "):] + + head, body, ok := splitTableBody(rest) + if !ok { + return out + } + + if name := tableName(head); name != "" { + out[name] = parseColumns(body) + } + } +} + +// tableName pulls the table name out of the text between "CREATE TABLE" +// and its opening parenthesis, dropping an IF NOT EXISTS and any +// quoting. +func tableName(head string) string { + head = strings.TrimSpace(head) + head = strings.TrimPrefix(head, "IF NOT EXISTS ") + head = strings.TrimPrefix(head, "if not exists ") + + fields := strings.Fields(head) + if len(fields) == 0 { + return "" + } + + return strings.Trim(fields[len(fields)-1], `"'`+"`") +} + +// splitTableBody returns the text before the table's opening paren and +// the balanced text inside it. +func splitTableBody(s string) (head, body string, ok bool) { + open := strings.IndexByte(s, '(') + if open < 0 { + return "", "", false + } + + depth := 0 + + for i := open; i < len(s); i++ { + switch s[i] { + case '(': + depth++ + case ')': + depth-- + + if depth == 0 { + return s[:open], s[open+1 : i], true + } + } + } + + return "", "", false +} + +// parseColumns splits a table body on its top-level commas and keeps +// the parts that are columns rather than table constraints. +func parseColumns(body string) []schemaColumn { + var ( + out []schemaColumn + depth int + start int + ) + + parts := make([]string, 0, 8) + + for i := range len(body) { + switch body[i] { + case '(': + depth++ + case ')': + depth-- + case ',': + if depth == 0 { + parts = append(parts, body[start:i]) + start = i + 1 + } + } + } + + parts = append(parts, body[start:]) + + for _, part := range parts { + fields := strings.Fields(part) + if len(fields) == 0 { + continue + } + + // A table constraint need not be followed by a space -- + // "UNIQUE(mbid)" is one field, and reading it as a column name + // makes an entirely healthy table look stale, which retires a + // catalog nobody asked to lose. + head := fields[0] + if i := strings.IndexByte(head, '('); i >= 0 { + head = head[:i] + } + + if constraintKeywords[strings.ToUpper(head)] { + continue + } + + col := schemaColumn{name: strings.Trim(head, `"'`+"`")} + if len(fields) > 1 { + col.typ = fields[1] + } + + out = append(out, col) + } + + return out +} + +// stripLineComments removes -- comments, which otherwise contribute +// stray parentheses and commas to the parse. +func stripLineComments(s string) string { + lines := strings.Split(s, "\n") + for i, line := range lines { + if idx := strings.Index(line, "--"); idx >= 0 { + lines[i] = line[:idx] + } + } + + return strings.Join(lines, "\n") +} + +// indexFold is a case-insensitive strings.Index. +func indexFold(s, substr string) int { + return strings.Index(strings.ToUpper(s), strings.ToUpper(substr)) +} + +// quoteIdent quotes a table name for interpolation into DDL, which +// cannot take a bound parameter. +func quoteIdent(name string) string { + return `"` + strings.ReplaceAll(name, `"`, `""`) + `"` +} diff --git a/backend/database/staleshape_test.go b/backend/database/staleshape_test.go new file mode 100644 index 0000000..990cfe9 --- /dev/null +++ b/backend/database/staleshape_test.go @@ -0,0 +1,499 @@ +package database + +import ( + "context" + "database/sql" + "log/slog" + "path" + "testing" + + _ "modernc.org/sqlite" +) + +// testLogger discards the repair's warnings; the tests assert on the +// database, not on the log. +func testLogger() *slog.Logger { + return slog.New(slog.DiscardHandler) +} + +// openRaw opens a scratch database file with no schema applied, so a +// test can build an *old* shape and then let NewDB's repair meet it. +func openRaw(t *testing.T, dir string) *sql.DB { + t.Helper() + + db, err := sql.Open("sqlite", path.Join(dir, "yj.db")) + if err != nil { + t.Fatalf("open: %v", err) + } + + t.Cleanup(func() { _ = db.Close() }) + + return db +} + +// TestRetiresIndexMissingAColumn is plan 014's bug, symptom first: an +// explore_index created before `total_tracks` existed, met by the +// projection every explore read uses. Before the repair this failed +// with "no such column: total_tracks" on every install that already had +// a catalog, while a fresh one was perfectly healthy. +func TestRetiresIndexMissingAColumn(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + db := openRaw(t, dir) + + // The pre-014 shape: the columns the projection needs, minus the + // one the plan added. + if _, err := db.ExecContext(ctx, ` + CREATE TABLE explore_index ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + entity_type INTEGER NOT NULL, + mbid BLOB NOT NULL, + title TEXT NOT NULL, + artist_name TEXT NOT NULL, + artist_mbid BLOB NOT NULL + ); + INSERT INTO explore_index (entity_type, mbid, title, artist_name, artist_mbid) + VALUES (1, x'00112233445566778899aabbccddeeff', 'x', 'y', x''); + `); err != nil { + t.Fatalf("seed: %v", err) + } + + if err := retireStaleTables(ctx, db, testLogger()); err != nil { + t.Fatalf("retire: %v", err) + } + + if err := applySchema(ctx, db); err != nil { + t.Fatalf("applySchema: %v", err) + } + + // The column the projection needs is there now. + var n int + if err := db.QueryRowContext(ctx, + `SELECT COUNT(*) FROM pragma_table_info('explore_index') + WHERE name = 'total_tracks'`, + ).Scan(&n); err != nil { + t.Fatalf("inspect: %v", err) + } + + if n != 1 { + t.Fatalf("explore_index still has no total_tracks column") + } + + // And the catalog really was retired rather than patched, so the + // artifact is fetched again instead of half a catalog being served. + if err := db.QueryRowContext(ctx, + "SELECT COUNT(*) FROM explore_index", + ).Scan(&n); err != nil { + t.Fatalf("count: %v", err) + } + + if n != 0 { + t.Fatalf("stale rows survived the retire: %d", n) + } +} + +// TestRetiresIndexWithTextMBIDs is the half an ALTER could not have +// repaired: plan 013 changed mbid from TEXT to BLOB, and SQLite does not +// coerce between them, so a query against 16 raw bytes returns no rows +// rather than an error. +func TestRetiresIndexWithTextMBIDs(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + db := openRaw(t, dir) + + if _, err := db.ExecContext(ctx, ` + CREATE TABLE explore_index ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + entity_type TEXT NOT NULL, + mbid TEXT NOT NULL, + title TEXT NOT NULL, + artist_name TEXT NOT NULL, + artist_mbid TEXT NOT NULL, + total_tracks INTEGER NOT NULL DEFAULT 0 + ); + `); err != nil { + t.Fatalf("seed: %v", err) + } + + if err := retireStaleTables(ctx, db, testLogger()); err != nil { + t.Fatalf("retire: %v", err) + } + + if err := applySchema(ctx, db); err != nil { + t.Fatalf("applySchema: %v", err) + } + + var typ string + if err := db.QueryRowContext(ctx, + `SELECT type FROM pragma_table_info('explore_index') WHERE name = 'mbid'`, + ).Scan(&typ); err != nil { + t.Fatalf("inspect: %v", err) + } + + if typ != "BLOB" { + t.Fatalf("mbid is still %s, want BLOB", typ) + } +} + +// TestRetiringTheIndexTakesItsMetaWithIt guards the thing that makes the +// repair actually repair: the marker saying the import finished is what +// stops the artifact being fetched again, so a catalog dropped without +// it would stay empty forever. +func TestRetiringTheIndexTakesItsMetaWithIt(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + db := openRaw(t, dir) + + if _, err := db.ExecContext(ctx, ` + CREATE TABLE explore_index ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + entity_type INTEGER NOT NULL, + mbid BLOB NOT NULL + ); + CREATE TABLE explore_index_meta (key TEXT PRIMARY KEY, value TEXT NOT NULL); + INSERT INTO explore_index_meta VALUES ('dump_import_done', '1'); + `); err != nil { + t.Fatalf("seed: %v", err) + } + + if err := retireStaleTables(ctx, db, testLogger()); err != nil { + t.Fatalf("retire: %v", err) + } + + if err := applySchema(ctx, db); err != nil { + t.Fatalf("applySchema: %v", err) + } + + var n int + if err := db.QueryRowContext(ctx, + "SELECT COUNT(*) FROM explore_index_meta WHERE key = 'dump_import_done'", + ).Scan(&n); err != nil { + t.Fatalf("meta: %v", err) + } + + if n != 0 { + t.Fatalf("the import-done marker survived a retired catalog") + } +} + +// TestHealthyDatabaseIsUntouched is the other half, and the one that +// would make this dangerous if it failed: a current schema must survive +// a launch with its catalog intact. A repair that retires a healthy +// catalog costs every user an artifact download on every start. +func TestHealthyDatabaseIsUntouched(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + db := openRaw(t, dir) + + if err := applySchema(ctx, db); err != nil { + t.Fatalf("applySchema: %v", err) + } + + if _, err := db.ExecContext(ctx, ` + INSERT INTO explore_index (entity_type, mbid, title, artist_name, artist_mbid) + VALUES (1, x'00112233445566778899aabbccddeeff', 'x', 'y', x'') + `); err != nil { + t.Fatalf("seed: %v", err) + } + + if err := retireStaleTables(ctx, db, testLogger()); err != nil { + t.Fatalf("retire: %v", err) + } + + var n int + if err := db.QueryRowContext(ctx, + "SELECT COUNT(*) FROM explore_index", + ).Scan(&n); err != nil { + t.Fatalf("count: %v", err) + } + + if n != 1 { + t.Fatalf("a healthy catalog was retired: %d rows left", n) + } +} + +// TestAuthoredTablesAreNeverRetired states the boundary in a test rather +// than only in a comment: this mechanism deletes data, and the only +// thing standing between it and a user's playlists is the Kind filter. +func TestAuthoredTablesAreNeverRetired(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + db := openRaw(t, dir) + + // A playlists table missing most of its current columns. + if _, err := db.ExecContext(ctx, ` + CREATE TABLE playlists (id INTEGER PRIMARY KEY, name TEXT NOT NULL); + INSERT INTO playlists (name) VALUES ('irreplaceable'); + `); err != nil { + t.Fatalf("seed: %v", err) + } + + if err := retireStaleTables(ctx, db, testLogger()); err != nil { + t.Fatalf("retire: %v", err) + } + + var n int + if err := db.QueryRowContext(ctx, + "SELECT COUNT(*) FROM playlists", + ).Scan(&n); err != nil { + t.Fatalf("count: %v", err) + } + + if n != 1 { + t.Fatalf("an authored table was retired; rows left: %d", n) + } +} + +// TestRetiresTablesTheSchemaNoLongerDescribes covers what plan 013 left +// behind on every database that predates it: seven tables the schema +// stopped describing, plus the schema_migrations table that squashing +// the chain retired. They are not stale in shape — they are simply not +// ours any more. +func TestRetiresTablesTheSchemaNoLongerDescribes(t *testing.T) { + ctx := context.Background() + db := openRaw(t, t.TempDir()) + + if _, err := db.ExecContext(ctx, ` + CREATE TABLE recordings (id INTEGER PRIMARY KEY, name TEXT); + CREATE TABLE artist_credit (id INTEGER PRIMARY KEY, text TEXT); + CREATE TABLE schema_migrations (version INTEGER PRIMARY KEY); + `); err != nil { + t.Fatalf("seed: %v", err) + } + + if err := retireStaleTables(ctx, db, testLogger()); err != nil { + t.Fatalf("retire: %v", err) + } + + for _, table := range []string{"recordings", "artist_credit", "schema_migrations"} { + var n int + + if err := db.QueryRowContext(ctx, + "SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name = ?", + table, + ).Scan(&n); err != nil { + t.Fatalf("inspect %s: %v", table, err) + } + + if n != 0 { + t.Errorf("%s survived; the schema no longer describes it", table) + } + } +} + +// TestFTSShadowTablesAreNotObsolete is the sweep's sharp edge: an FTS5 +// virtual table is backed by four shadow tables that appear in +// sqlite_master under their own names and are in no schema file. +// Dropping one destroys the index it belongs to. +func TestFTSShadowTablesAreNotObsolete(t *testing.T) { + ctx := context.Background() + db := openRaw(t, t.TempDir()) + + if err := applySchema(ctx, db); err != nil { + t.Fatalf("applySchema: %v", err) + } + + obsolete, err := obsoleteTables(ctx, db) + if err != nil { + t.Fatalf("obsoleteTables: %v", err) + } + + if len(obsolete) != 0 { + t.Fatalf("a freshly created schema reported obsolete tables: %v", obsolete) + } +} + +// TestRetiringOwnedTablesDoesNotDangle pins the one behaviour that is +// silently wrong rather than loudly broken. +// +// Retiring audio_files leaves playlist entries behind. With +// foreign_keys ON — which applyPRAGMAs has done before NewDB gets here — +// SET NULL fires and they point at nothing. With it OFF they keep ids +// that the rescan reissues from 1, so every playlist quietly fills with +// different songs. Nothing about the schema makes that ordering +// obvious, so it is asserted rather than assumed. +func TestRetiringOwnedTablesDoesNotDangle(t *testing.T) { + ctx := context.Background() + db := openRaw(t, t.TempDir()) + + if _, err := db.ExecContext(ctx, "PRAGMA foreign_keys = ON"); err != nil { + t.Fatalf("pragma: %v", err) + } + + if err := applySchema(ctx, db); err != nil { + t.Fatalf("applySchema: %v", err) + } + + // Break audio_files' shape so it is retired, keeping a playlist + // entry that references it. + if _, err := db.ExecContext(ctx, ` + INSERT INTO playlists (id, name) VALUES (1, 'keepme'); + INSERT INTO libraries (id, name, path) VALUES (0, 'test', '/music'); + INSERT INTO audio_files (id, file_path, file_type_id, length_milliseconds) + VALUES (7, '/music/a.flac', 1, 1000); + INSERT INTO playlist_tracks (playlist_id, audio_file_id, position) + VALUES (1, 7, 0); + DROP VIEW IF EXISTS track_metadata; + ALTER TABLE audio_files DROP COLUMN artist_credit; + `); err != nil { + t.Fatalf("seed: %v", err) + } + + if err := retireStaleTables(ctx, db, testLogger()); err != nil { + t.Fatalf("retire: %v", err) + } + + if err := applySchema(ctx, db); err != nil { + t.Fatalf("applySchema: %v", err) + } + + var dangling int + if err := db.QueryRowContext(ctx, + "SELECT COUNT(*) FROM playlist_tracks WHERE audio_file_id IS NOT NULL", + ).Scan(&dangling); err != nil { + t.Fatalf("count: %v", err) + } + + if dangling != 0 { + t.Fatalf( + "%d playlist entries still point at retired audio_files ids; "+ + "a rescan will reissue those ids to different tracks", + dangling, + ) + } + + // The playlist itself is authored and must be untouched. + var playlists int + if err := db.QueryRowContext(ctx, + "SELECT COUNT(*) FROM playlists", + ).Scan(&playlists); err != nil { + t.Fatalf("playlists: %v", err) + } + + if playlists != 1 { + t.Fatalf("authored playlist lost: %d", playlists) + } +} + +// TestRetiringInterlinkedLegacyTables is the bug the unit tests missed +// and a real database found. +// +// The tables plan 013 retired reference each other -- pre-013 +// audio_files has a foreign key into recordings -- so with foreign keys +// ON, dropping them one at a time fails with "FOREIGN KEY constraint +// failed" on whichever goes first, and map iteration order decides +// which that is. Every other test in this file ran with foreign keys +// off and passed happily; the app enables them in applyPRAGMAs before +// the repair runs, so only the real launch path showed it. +func TestRetiringInterlinkedLegacyTables(t *testing.T) { + ctx := context.Background() + db := openRaw(t, t.TempDir()) + + if _, err := db.ExecContext(ctx, "PRAGMA foreign_keys = ON"); err != nil { + t.Fatalf("pragma: %v", err) + } + + // The pre-013 shape, with the reference that makes ordering matter. + // release_group_recordings sorts *after* recordings and references + // it, so the deterministic order retires the parent while the child + // still holds rows pointing at it -- which is the case that fails + // without the deferral, rather than one that fails on some runs. + if _, err := db.ExecContext(ctx, ` + CREATE TABLE recordings (id INTEGER PRIMARY KEY, name TEXT); + CREATE TABLE artist_credit (id INTEGER PRIMARY KEY, text TEXT); + CREATE TABLE release_group_recordings ( + id INTEGER PRIMARY KEY, + recording_id INTEGER NOT NULL, + FOREIGN KEY(recording_id) REFERENCES recordings(id) + ); + CREATE TABLE audio_files ( + id INTEGER PRIMARY KEY, + file_path TEXT NOT NULL UNIQUE, + recording_id INTEGER, + FOREIGN KEY(recording_id) REFERENCES recordings(id) + ); + INSERT INTO recordings (id, name) VALUES (1, 'x'); + INSERT INTO release_group_recordings (id, recording_id) VALUES (1, 1); + INSERT INTO audio_files (id, file_path, recording_id) + VALUES (1, '/music/a.flac', 1); + `); err != nil { + t.Fatalf("seed: %v", err) + } + + if err := retireStaleTables(ctx, db, testLogger()); err != nil { + t.Fatalf("retire: %v", err) + } + + if err := applySchema(ctx, db); err != nil { + t.Fatalf("applySchema: %v", err) + } + + for _, table := range []string{"recordings", "artist_credit"} { + var n int + + if err := db.QueryRowContext(ctx, + "SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name = ?", + table, + ).Scan(&n); err != nil { + t.Fatalf("inspect %s: %v", table, err) + } + + if n != 0 { + t.Errorf("%s survived the retire", table) + } + } + + // And the rebuilt audio_files is the current shape, which is the + // whole reason the old one had to go. + var n int + if err := db.QueryRowContext(ctx, + `SELECT COUNT(*) FROM pragma_table_info('audio_files') + WHERE name = 'artist_credit'`, + ).Scan(&n); err != nil { + t.Fatalf("inspect audio_files: %v", err) + } + + if n != 1 { + t.Fatal("audio_files was not rebuilt in the current shape") + } +} + +// TestParseCreateTablesReadsTheRealSchema keeps the parser honest +// against the files it actually runs on: a parser that silently found +// no columns would report every table healthy and repair nothing. +func TestParseCreateTablesReadsTheRealSchema(t *testing.T) { + declared, err := declaredTables() + if err != nil { + t.Fatalf("declaredTables: %v", err) + } + + cols, ok := declared["explore_index"] + if !ok { + t.Fatal("explore_index was not parsed out of the schema files") + } + + want := map[string]string{ + "mbid": "BLOB", + "total_tracks": "INTEGER", + "artist_name": "TEXT", + } + + got := make(map[string]string, len(cols)) + for _, c := range cols { + got[c.name] = c.typ + } + + for name, typ := range want { + if got[name] != typ { + t.Errorf("explore_index.%s parsed as %q, want %q", name, got[name], typ) + } + } + + // A table constraint must not be mistaken for a column. + for _, c := range cols { + switch c.name { + case "PRIMARY", "FOREIGN", "UNIQUE", "CHECK", "CONSTRAINT": + t.Errorf("parsed table constraint %q as a column", c.name) + } + } +} diff --git a/cmd/indexbuild/staleschema_test.go b/cmd/indexbuild/staleschema_test.go index 6837638..a0a3b1e 100644 --- a/cmd/indexbuild/staleschema_test.go +++ b/cmd/indexbuild/staleschema_test.go @@ -53,10 +53,19 @@ func TestRetireLibraryTables(t *testing.T) { CREATE TABLE recordings (id INTEGER PRIMARY KEY, title TEXT); `) - // The symptom, before the repair: the schema cannot be applied over - // a table whose shape has moved on. - if _, err := database.NewDB(logger); err == nil { - t.Fatal("expected the stale shape to fail to open; it did not") + // This used to assert the symptom -- that the schema cannot be + // applied over a table whose shape has moved on -- because at the + // time nothing repaired it and only this job did. The app-side + // repair (backend/database/staleshape.go) now retires a stale + // non-authored table before applySchema meets it, so opening + // succeeds and the symptom no longer reproduces from here. + // + // That does not make retireLibraryTables redundant, and the rest of + // this test is why: the app-side repair only removes what is *stale*, + // while this database wants its library half gone entirely, healthy + // or not, because nothing here scans, plays or authors. + if _, err := database.NewDB(logger); err != nil { + t.Fatalf("the app-side repair should have opened this: %v", err) } if err := retireLibraryTables(context.Background(), logger); err != nil {