diff --git a/backend/database/playlistphantoms.go b/backend/database/playlistphantoms.go new file mode 100644 index 0000000..1d177c5 --- /dev/null +++ b/backend/database/playlistphantoms.go @@ -0,0 +1,125 @@ +package database + +import ( + "context" + "database/sql" + "fmt" + "log/slog" +) + +// Preserving a playlist entry across the loss of its track is two +// statements, not one, and the split is not tidiness -- it is what +// makes the important half work in the situation that needs it most. +// +// `playlist_tracks.audio_file_id` is ON DELETE SET NULL, so an entry +// outlives its file as an id-less row that says nothing about what the +// user put in the playlist. The phantom_* columns carry the answer +// across and ResolvePhantomTracksAfterScan re-links them afterwards -- +// but only if something fills them *before* the rows go. +// +// The two halves are not equally important and are not equally +// available: +// +// - **phantom_file_path is the one that matters.** +// ResolvePhantomTracksAfterScan matches it against +// `audio_files.file_path`, so without it an entry can never be +// re-linked and the playlist is empty for good. It comes straight +// off `audio_files`, whose `file_path` is the table's natural key +// and has been present in every shape it has ever had -- including +// the pre-013 stub of `(id, file_path, recording_id)`. +// - The rest is *display* for a phantom entry before a rescan +// re-links it, and it comes from the `track_metadata` view, which +// is the one definition of a track row and not worth restating. +// +// Reading the view is what cannot be relied on here, and that is the +// whole reason for the split. This runs *before* applySchema, which is +// precisely the moment the schema is inconsistent: the view is whatever +// the last launch's schema declared, while `audio_files` is whatever +// the launch before that left behind. A view over columns the table no +// longer has is not merely empty -- `pragma_table_info` on it *errors*, +// and so does selecting from it. `cmd/indexbuild`'s fixture is exactly +// that shape and is what caught this. +// +// COALESCE keeps an existing phantom value in both halves: an entry +// already phantom is one whose file went missing in an earlier pass, +// and its recorded metadata is the only copy left. Overwriting that +// from a NULL join erases the rows this exists to protect. +const ( + preservePhantomPathSQL = ` + UPDATE playlist_tracks + SET phantom_file_path = COALESCE(phantom_file_path, ( + SELECT af.file_path FROM audio_files af + WHERE af.id = playlist_tracks.audio_file_id + )) + WHERE audio_file_id IS NOT NULL + ` + + preservePhantomDisplaySQL = ` + UPDATE playlist_tracks + SET + phantom_title = COALESCE(phantom_title, ( + SELECT tm.title FROM track_metadata tm + WHERE tm.id = playlist_tracks.audio_file_id + )), + phantom_artist = COALESCE(phantom_artist, ( + SELECT tm.artist_name FROM track_metadata tm + WHERE tm.id = playlist_tracks.audio_file_id + )), + phantom_album = COALESCE(phantom_album, ( + SELECT tm.album FROM track_metadata tm + WHERE tm.id = playlist_tracks.audio_file_id + )), + phantom_duration_ms = COALESCE(phantom_duration_ms, ( + SELECT af.length_milliseconds FROM audio_files af + WHERE af.id = playlist_tracks.audio_file_id + )), + phantom_genre = COALESCE(phantom_genre, ( + SELECT tm.genre FROM track_metadata tm + WHERE tm.id = playlist_tracks.audio_file_id + )), + phantom_cover_art_path = COALESCE(phantom_cover_art_path, ( + SELECT tm.cover_art_path FROM track_metadata tm + WHERE tm.id = playlist_tracks.audio_file_id + )) + WHERE audio_file_id IS NOT NULL + ` +) + +// PreservePlaylistPhantoms records every linked playlist entry's track +// metadata on the entry itself, so the entry survives the rows being +// deleted underneath it. +// +// Every path that empties `audio_files` must call this first, inside +// the same transaction as the delete. There are two such paths and +// they had drifted: the full rescan in backend/library did this and the +// stale-shape retire in this package did not, so the *documented* +// repair ("delete and rescan") preserved playlists while the automatic +// one that exists to spare the user that work silently emptied them. +// +// The display half is skipped, with a warning, when `track_metadata` +// cannot answer -- see the note above. Skipping it costs a phantom +// entry its title until a rescan re-links it; skipping the path half +// would cost the entry outright, so that one is an error. +func PreservePlaylistPhantoms( + ctx context.Context, tx *sql.Tx, logger *slog.Logger, +) error { + if _, err := tx.ExecContext(ctx, preservePhantomPathSQL); err != nil { + return fmt.Errorf( + "could not preserve playlist track file paths: %w", err, + ) + } + + if _, err := tx.ExecContext(ctx, preservePhantomDisplaySQL); err != nil { + // A failed statement does not roll back a SQLite transaction, + // so the path half above stands and the entries remain + // re-linkable. + logger.Warn( + "could not record display metadata for playlist entries; "+ + "they will be re-linked by the next scan but read as "+ + "unknown until then", + "err", err, + ) + } + + return nil +} diff --git a/backend/database/staleshape.go b/backend/database/staleshape.go index 41022ac..1948057 100644 --- a/backend/database/staleshape.go +++ b/backend/database/staleshape.go @@ -245,6 +245,13 @@ func dropDeferred( ctx context.Context, db *sql.DB, logger *slog.Logger, drop map[string]string, ) error { + // Asked before the transaction opens, because the answer is about + // which tables are live and that cannot change underneath us here. + preserve, err := shouldPreservePhantoms(ctx, db, drop) + if err != nil { + return err + } + tx, err := db.BeginTx(ctx, nil) if err != nil { return fmt.Errorf("could not begin the retire transaction: %w", err) @@ -256,6 +263,22 @@ func dropDeferred( return fmt.Errorf("could not defer foreign keys: %w", err) } + // Before any drop, so every entry still has a track to read. It is + // in this transaction rather than beside it because the preservation + // and the delete have to succeed or fail together: a commit that + // dropped the files without the phantoms is the bug, and a commit + // that wrote phantoms without dropping anything is a lie about rows + // that are still there. + if preserve { + logger.Info( + "preserving playlist entries across the retire of audio_files", + ) + + if err := PreservePlaylistPhantoms(ctx, tx, logger); err != nil { + return 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 @@ -283,6 +306,38 @@ func dropDeferred( return nil } +// shouldPreservePhantoms reports whether this retire is about to take +// `audio_files` out from under the playlists. +// +// The `playlist_tracks` check is not defensive padding. This runs +// *before* applySchema, which is the moment the schema is by definition +// mid-repair, and the preservation reads a table it does not drop. A +// database old enough not to have it would otherwise fail here, and +// failing here means the app does not open at all -- while nothing is +// lost by skipping, since an absent `playlist_tracks` holds no +// playlists to save. +// +// It deliberately does *not* ask after `track_metadata`. Whether that +// view can answer is PreservePlaylistPhantoms's own business, because a +// view broken against an older `audio_files` is a state this function +// cannot detect without hitting the same error it is trying to avoid: +// pragma_table_info on such a view errors rather than reporting no +// columns. +func shouldPreservePhantoms( + ctx context.Context, db *sql.DB, drop map[string]string, +) (bool, error) { + if _, going := drop["audio_files"]; !going { + return false, nil + } + + cols, err := liveColumns(ctx, db, "playlist_tracks") + if err != nil { + return false, err + } + + return len(cols) > 0, 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 diff --git a/backend/database/staleshape_test.go b/backend/database/staleshape_test.go index 990cfe9..b607bff 100644 --- a/backend/database/staleshape_test.go +++ b/backend/database/staleshape_test.go @@ -497,3 +497,180 @@ func TestParseCreateTablesReadsTheRealSchema(t *testing.T) { } } } + +// TestRetiringAudioFilesKeepsPlaylistContents is the symptom this +// repair exists for: a playlist survived the retire as a row count and +// nothing else. +// +// TestRetiringOwnedTablesDoesNotDangle already asserts the entry does +// not keep a stale id, which is the *dangerous* half. It is satisfied +// just as well by an entry that says nothing at all, which is the +// half that quietly emptied every playlist -- so this asserts what the +// entry still knows, and specifically phantom_file_path, because that +// is the column ResolvePhantomTracksAfterScan matches back against +// audio_files.file_path. +// +// Note the seed drops `comment`, not `artist_credit`: the mutation has +// to leave `track_metadata` standing, since a real launch reaches the +// retire with the view the previous launch created. A test that drops +// the view first is testing the skip path, not this one. +func TestRetiringAudioFilesKeepsPlaylistContents(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) + } + + 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 artists (id, name) VALUES (3, 'Aurora Fields'); + INSERT INTO cover_art (id, file_path, mime_type) + VALUES (9, 'covers/7.jpg', 'image/jpeg'); + INSERT INTO genres (id, name) VALUES (5, 'Ambient'); + INSERT INTO albums (id, name, artist_id, cover_art_id) + VALUES (4, 'Tideline', 3, 9); + INSERT INTO audio_files + (id, file_path, file_type_id, length_milliseconds, + title, artist_credit, artist_id, album_id) + VALUES (7, '/music/a.flac', 1, 1000, + 'Slack Water', 'Aurora Fields', 3, 4); + INSERT INTO file_genres (audio_file_id, genre_id) VALUES (7, 5); + INSERT INTO playlist_tracks (playlist_id, audio_file_id, position) + VALUES (1, 7, 0); + ALTER TABLE audio_files DROP COLUMN comment; + `); 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 ( + path, title, artist, album, genre, cover sql.NullString + duration sql.NullInt64 + ) + + if err := db.QueryRowContext(ctx, ` + SELECT phantom_file_path, phantom_title, phantom_artist, + phantom_album, phantom_duration_ms, phantom_genre, + phantom_cover_art_path + FROM playlist_tracks WHERE playlist_id = 1 + `).Scan(&path, &title, &artist, &album, &duration, &genre, &cover); err != nil { + t.Fatalf("read the surviving entry: %v", err) + } + + // The one that matters: without it the entry can never be re-linked + // by the rescan the retire itself provokes. + if path.String != "/music/a.flac" { + t.Fatalf( + "phantom_file_path is %q, want %q -- the playlist entry "+ + "cannot be re-linked and the playlist is empty for good", + path.String, "/music/a.flac", + ) + } + + if title.String != "Slack Water" { + t.Errorf("phantom_title is %q, want %q", title.String, "Slack Water") + } + + if artist.String != "Aurora Fields" { + t.Errorf("phantom_artist is %q, want %q", artist.String, "Aurora Fields") + } + + if album.String != "Tideline" { + t.Errorf("phantom_album is %q, want %q", album.String, "Tideline") + } + + if duration.Int64 != 1000 { + t.Errorf("phantom_duration_ms is %d, want 1000", duration.Int64) + } + + if genre.String != "Ambient" { + t.Errorf("phantom_genre is %q, want %q", genre.String, "Ambient") + } + + if cover.String != "covers/7.jpg" { + t.Errorf("phantom_cover_art_path is %q, want %q", cover.String, "covers/7.jpg") + } +} + +// TestRetiringAudioFilesKeepsPathsWhenTheViewCannotAnswer is the case +// that broke cmd/indexbuild: this repair runs *before* applySchema, so +// `track_metadata` is whatever the last launch declared while +// `audio_files` is whatever the launch before that left behind, and a +// view over columns the table no longer has does not read as empty -- +// it errors. +// +// The pre-013 stub shape below is the real one that fixture carries. +// What must survive is phantom_file_path, because `file_path` is the +// table's natural key and has been in every shape it ever had; the +// display columns are allowed to be absent, and the open must not fail. +func TestRetiringAudioFilesKeepsPathsWhenTheViewCannotAnswer(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) + } + + // The rows go in *after* the reshape: dropping audio_files with + // foreign keys on would fire the ON DELETE SET NULL and null the + // entry this test is about, which would pass for the wrong reason. + if _, err := db.ExecContext(ctx, ` + DROP TABLE audio_files; + CREATE TABLE audio_files ( + id INTEGER PRIMARY KEY, + file_path TEXT NOT NULL UNIQUE, + recording_id INTEGER + ); + INSERT INTO playlists (id, name) VALUES (1, 'keepme'); + INSERT INTO audio_files (id, file_path) VALUES (7, '/music/a.flac'); + INSERT INTO playlist_tracks (playlist_id, audio_file_id, position) + VALUES (1, 7, 0); + `); err != nil { + t.Fatalf("seed: %v", err) + } + + // The symptom this guards: the repair must not turn a recoverable + // database into one the app refuses to open. + if err := retireStaleTables(ctx, db, testLogger()); err != nil { + t.Fatalf( + "the retire failed on a view it could not read, so the app "+ + "would not open at all: %v", err, + ) + } + + if err := applySchema(ctx, db); err != nil { + t.Fatalf("applySchema: %v", err) + } + + var path sql.NullString + if err := db.QueryRowContext(ctx, + "SELECT phantom_file_path FROM playlist_tracks WHERE playlist_id = 1", + ).Scan(&path); err != nil { + t.Fatalf("read the surviving entry: %v", err) + } + + if path.String != "/music/a.flac" { + t.Fatalf( + "phantom_file_path is %q, want %q -- the display half being "+ + "unavailable must not cost the entry its one re-link key", + path.String, "/music/a.flac", + ) + } +} diff --git a/backend/library/rescan.go b/backend/library/rescan.go index 5fd75b1..456b0f6 100644 --- a/backend/library/rescan.go +++ b/backend/library/rescan.go @@ -8,6 +8,7 @@ import ( "time" "yellowjacket/backend/coverart" + "yellowjacket/backend/database" ) var errNoLibrariesConfigured = errors.New( @@ -137,34 +138,12 @@ func (l *Library) clearLibraryTables() error { // metadata for all linked tracks before audio_files are deleted. // ON DELETE SET NULL will null out audio_file_id, converting them // to phantoms that ResolvePhantomTracksAfterScan can re-link. - if _, err := tx.ExecContext(l.ctx, ` - UPDATE playlist_tracks - SET - phantom_title = COALESCE(phantom_title, ( - SELECT tm.title FROM track_metadata tm - WHERE tm.id = playlist_tracks.audio_file_id - )), - phantom_artist = COALESCE(phantom_artist, ( - SELECT tm.artist_name FROM track_metadata tm - WHERE tm.id = playlist_tracks.audio_file_id - )), - phantom_album = COALESCE(phantom_album, ( - SELECT tm.album FROM track_metadata tm - WHERE tm.id = playlist_tracks.audio_file_id - )), - phantom_duration_ms = COALESCE(phantom_duration_ms, ( - SELECT af.length_milliseconds FROM audio_files af - WHERE af.id = playlist_tracks.audio_file_id - )), - phantom_file_path = COALESCE(phantom_file_path, ( - SELECT af.file_path FROM audio_files af - WHERE af.id = playlist_tracks.audio_file_id - )) - WHERE audio_file_id IS NOT NULL - `); err != nil { - return fmt.Errorf( - "could not preserve playlist track metadata: %w", err, - ) + // + // Shared with the stale-shape retire in backend/database, which is + // the other path that empties this table and which did not do this + // (#183): the statement lives there so the two cannot drift again. + if err := database.PreservePlaylistPhantoms(l.ctx, tx, l.logger); err != nil { + return err } // Phase 2: the files. file_genres cascades with them.