diff --git a/.planning/NOTES.md b/.planning/NOTES.md index a66f1d7..bf85c00 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -2983,3 +2983,54 @@ terminates the tagged template, and the failure arrives as `Expected "]" but found "wa"` from the CSS parser, at a line number in the *comment*. `make css-check` exists for this and named it immediately. + +## The index artifact could not be exported, and the reason is a rule this repo already had (2026-08-16) + +`maintain-index` failed on an unrelated push: + +``` +indexexport: copy rows: SQL logic error: no such column: total_tracks (1) +``` + +Three minutes in, on the one job that owns the ~205 GB checkpoint and +publishes the catalog every user downloads. + +**The cause is the exception that keeps that checkpoint alive.** The +index job's `/cache` is a real `YJ_HOME` that survives between runs, so +`explore_index` there is classified `Cache` and is deliberately *not* +dropped and recreated by `cmd/indexbuild`'s schema repair +(`staleschema.go`). A column added to the schema afterwards is +therefore simply absent from that database — and `total_tracks` was +added by the album-completeness work. The exporter selected it anyway. + +**The fix is the rule the importer already follows.** +`artifactHasTotals()` exists precisely because "adding a column to the +importer's SELECT is how you break every artifact already published"; +the mirror image — *reading* an index older than the binary — had no +such guard. `sourceColumns()` asks +`pragma_table_info('explore_index', 'main')` and selects a literal `0` +when the column is not there, which is what the column already means by +"the catalog does not say" and what the app already renders as unknown +rather than as incomplete. The destination keeps every column, so an +importer needs no second shape. + +So the pattern generalises, and is worth stating once: **any query that +crosses a version boundary in either direction asks the schema rather +than trusting it.** There are now three of these — `artifactStoresText` +(encoding), `artifactHasTotals` (import), `sourceColumns` (export). + +Two things about the test are worth keeping. + +It reproduces the failure **symptom first**: with the fix removed it +fails with the CI message verbatim, `copy rows: SQL logic error: no +such column: total_tracks (1)`. That was checked, not assumed. + +And its first version silently proved nothing. `oldColumns` was +`strings.Replace(catalogColumns, "total_tracks, ", "", 1)` — which +matches *nothing*, because the list is formatted across lines and the +name is followed by a newline rather than a space. So the "old" index +had every current column, the probe correctly said so, and the only +reason this was caught is that the assertion about the probe ran before +the assertion about the export. A fixture built by string surgery on a +formatted constant needs to be whitespace-independent; it filters the +list now. diff --git a/cmd/indexexport/export_test.go b/cmd/indexexport/export_test.go new file mode 100644 index 0000000..d4c983b --- /dev/null +++ b/cmd/indexexport/export_test.go @@ -0,0 +1,191 @@ +//go:build indexbuild + +package main + +import ( + "database/sql" + "path/filepath" + "strings" + "testing" + + _ "modernc.org/sqlite" +) + +// The columns an index built before the completeness work has: every +// current one except total_tracks. +// +// Filtered rather than string-replaced, because the list is formatted +// across lines: `strings.Replace(catalogColumns, "total_tracks, ", …)` +// matches nothing (the name is followed by a newline, not a space) and +// silently yields the *current* list -- so the test built a modern +// source index and proved nothing while passing its own premise. +var oldColumns = withoutTotals(catalogColumns) + +func withoutTotals(cols string) string { + kept := make([]string, 0, 20) + + for _, part := range strings.Split(cols, ",") { + if strings.TrimSpace(part) == "total_tracks" { + continue + } + + kept = append(kept, strings.TrimSpace(part)) + } + + return strings.Join(kept, ", ") +} + +// TestExportFromAnIndexWithoutTotals reproduces the failure that broke +// the index-artifact job, symptom first. +// +// The job's /cache volume is a real YJ_HOME that survives between runs +// and holds ~205 GB, so its explore_index is Cache and is deliberately +// not dropped by cmd/indexbuild's schema repair -- which means a column +// added to the schema afterwards is simply absent from it. The exporter +// selected it anyway and the whole run died with +// +// indexexport: copy rows: SQL logic error: no such column: total_tracks +// +// after three minutes of work, on a job that publishes the catalog +// every user downloads. +func TestExportFromAnIndexWithoutTotals(t *testing.T) { + t.Parallel() + + db := openWithSource(t, oldColumns) + + if got := sourceColumns(db); strings.Contains(got, "total_tracks") { + t.Fatalf("source list still names total_tracks: %s", got) + } + + if err := copyRows(db, 10, 5, 5); err != nil { + t.Fatalf("export from an index without total_tracks: %v", err) + } + + // Zero, not absent: the artifact keeps every column so an importer + // needs no second shape, and 0 is what the column already means by + // "the catalog does not say". + var total int + if err := db.QueryRow( + `SELECT total_tracks FROM core.explore_index WHERE entity_type = 2`, + ).Scan(&total); err != nil { + t.Fatalf("read exported total_tracks: %v", err) + } + + if total != 0 { + t.Errorf("total_tracks = %d, want 0", total) + } +} + +// TestExportCarriesTotalsWhenTheIndexHasThem is the other half: the +// probe must not cost the totals of an index that does have them. +func TestExportCarriesTotalsWhenTheIndexHasThem(t *testing.T) { + t.Parallel() + + db := openWithSource(t, catalogColumns) + + if got := sourceColumns(db); !strings.Contains(got, "total_tracks") { + t.Fatalf("source list dropped total_tracks: %s", got) + } + + if err := copyRows(db, 10, 5, 5); err != nil { + t.Fatalf("export: %v", err) + } + + var total int + if err := db.QueryRow( + `SELECT total_tracks FROM core.explore_index WHERE entity_type = 2`, + ).Scan(&total); err != nil { + t.Fatalf("read exported total_tracks: %v", err) + } + + if total != 12 { + t.Errorf("total_tracks = %d, want 12", total) + } +} + +// openWithSource builds a source index carrying exactly `columns`, with +// one artist and one of its release groups, and attaches a fresh +// artifact database as `core`. +func openWithSource(t *testing.T, columns string) *sql.DB { + t.Helper() + + dir := t.TempDir() + + db, err := sql.Open("sqlite", filepath.Join(dir, "src.db")) + if err != nil { + t.Fatalf("open source: %v", err) + } + + t.Cleanup(func() { _ = db.Close() }) + + // The source's shape is the point of the test, so it is spelled + // out here rather than taken from the app's schema, which is + // always current by definition. + create := `CREATE TABLE explore_index ( + id INTEGER PRIMARY KEY, + entity_type INTEGER NOT NULL, + mbid BLOB NOT NULL, + title TEXT NOT NULL DEFAULT '', + artist_name TEXT NOT NULL DEFAULT '', + artist_mbid BLOB NOT NULL DEFAULT x'', + aliases TEXT NOT NULL DEFAULT '', + popularity INTEGER NOT NULL DEFAULT 0, + listener_count INTEGER NOT NULL DEFAULT 0, + duration INTEGER NOT NULL DEFAULT 0, + caa_release_mbid BLOB NOT NULL DEFAULT x'', + release_name TEXT NOT NULL DEFAULT '', + primary_type TEXT NOT NULL DEFAULT '', + secondary_types TEXT NOT NULL DEFAULT '', + release_date TEXT NOT NULL DEFAULT '', + total_tracks INTEGER NOT NULL DEFAULT 0, + artist_type TEXT NOT NULL DEFAULT '', + country TEXT NOT NULL DEFAULT '', + disambiguation TEXT NOT NULL DEFAULT '', + sort_name TEXT NOT NULL DEFAULT '', + discog_fetched INTEGER NOT NULL DEFAULT 0 + )` + + if !strings.Contains(columns, "total_tracks") { + create = strings.Replace( + create, "total_tracks INTEGER NOT NULL DEFAULT 0,\n", "", 1, + ) + } + + if _, err := db.Exec(create); err != nil { + t.Fatalf("create source: %v", err) + } + + seed := `INSERT INTO explore_index (` + columns + `) VALUES ` + + if strings.Contains(columns, "total_tracks") { + seed += `(1, x'00000000000000000000000000000001', 'A', 'A', + x'00000000000000000000000000000001', '', 100, 100, 0, x'', + '', '', '', '', 12, '', '', '', '', 0), + (2, x'00000000000000000000000000000002', 'RG', 'A', + x'00000000000000000000000000000001', '', 90, 90, 0, x'', + '', 'Album', '', '', 12, '', '', '', '', 0)` + } else { + seed += `(1, x'00000000000000000000000000000001', 'A', 'A', + x'00000000000000000000000000000001', '', 100, 100, 0, x'', + '', '', '', '', '', '', '', '', 0), + (2, x'00000000000000000000000000000002', 'RG', 'A', + x'00000000000000000000000000000001', '', 90, 90, 0, x'', + '', 'Album', '', '', '', '', '', '', 0)` + } + + if _, err := db.Exec(seed); err != nil { + t.Fatalf("seed source: %v", err) + } + + if _, err := db.Exec( + `ATTACH DATABASE ? AS core`, filepath.Join(dir, "core.db"), + ); err != nil { + t.Fatalf("attach core: %v", err) + } + + if err := createSchema(db); err != nil { + t.Fatalf("create artifact schema: %v", err) + } + + return db +} diff --git a/cmd/indexexport/main.go b/cmd/indexexport/main.go index e97c667..b9d943c 100644 --- a/cmd/indexexport/main.go +++ b/cmd/indexexport/main.go @@ -25,6 +25,7 @@ import ( "os" "path/filepath" "strconv" + "strings" "time" _ "modernc.org/sqlite" @@ -43,6 +44,41 @@ const catalogColumns = `entity_type, mbid, title, artist_name, artist_mbid, release_name, primary_type, secondary_types, release_date, total_tracks, artist_type, country, disambiguation, sort_name, discog_fetched` +// sourceColumns is catalogColumns as read *from* the built index, +// which is not always shaped like the one this binary was compiled +// against. +// +// The index job's /cache volume is a real YJ_HOME that survives +// between runs and holds ~205 GB nobody can re-download casually, so +// its explore_index is classified Cache and is deliberately **not** +// dropped and recreated by cmd/indexbuild's schema repair. A column +// added to the schema after that database was built is therefore +// absent from it, and selecting it fails the whole export with +// "no such column: total_tracks" -- which is what happened the first +// time the job ran after the completeness work. +// +// So the source list is asked for rather than assumed, exactly as +// artifactHasTotals does on the importing side. Zero is what the +// column means by "the catalog does not say", and the app already +// renders that as unknown rather than as incomplete. +func sourceColumns(db *sql.DB) string { + var n int + + err := db.QueryRow( + `SELECT COUNT(*) FROM pragma_table_info('explore_index', 'main') + WHERE name = 'total_tracks'`, + ).Scan(&n) + if err == nil && n > 0 { + return catalogColumns + } + + fmt.Println( + " note: this index predates total_tracks; exporting 0 for it", + ) + + return strings.Replace(catalogColumns, "total_tracks", "0", 1) +} + var errNoHome = errors.New( "YJ_HOME must be set to the directory holding the built index", ) @@ -189,6 +225,10 @@ func createSchema(db *sql.DB) error { // dumpcatalog.go — a flat global top-N would give a handful of // superstars everything and everyone else nothing. func copyRows(db *sql.DB, artists, perArtistRGs, perArtistRecs int) error { + // The destination is created by this binary and always has every + // column; only the source may be older. + srcColumns := sourceColumns(db) + if _, err := db.Exec(` CREATE TEMP TABLE core_artists AS SELECT mbid FROM main.explore_index @@ -201,7 +241,7 @@ func copyRows(db *sql.DB, artists, perArtistRGs, perArtistRecs int) error { copied, err := insertSelect(db, ` INSERT INTO core.explore_index (`+catalogColumns+`) - SELECT `+catalogColumns+` + SELECT `+srcColumns+` FROM main.explore_index WHERE entity_type = 1 /* artist */ AND mbid IN (SELECT mbid FROM core_artists)`) @@ -228,7 +268,7 @@ func copyRows(db *sql.DB, artists, perArtistRGs, perArtistRecs int) error { // most `limit` rows, ranked by their own listen counts. n, err := insertSelect(db, ` INSERT INTO core.explore_index (`+catalogColumns+`) - SELECT `+catalogColumns+` FROM ( + SELECT `+srcColumns+` FROM ( SELECT *, ROW_NUMBER() OVER ( PARTITION BY artist_mbid ORDER BY popularity DESC ) AS rn