diff --git a/backend/database/playlistphantoms.go b/backend/database/playlistphantoms.go index 1d177c5..9703d26 100644 --- a/backend/database/playlistphantoms.go +++ b/backend/database/playlistphantoms.go @@ -5,6 +5,7 @@ import ( "database/sql" "fmt" "log/slog" + "strings" ) // Preserving a playlist entry across the loss of its track is two @@ -89,12 +90,14 @@ const ( // 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. +// Every path that empties `audio_files` must call this (or the scoped +// variant below) first, inside the same transaction as the delete. +// These paths have drifted before: 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 (#183). The incremental scan's orphan cleanup and +// RemoveFromLibrary drifted the same way and are #246. // // The display half is skipped, with a warning, when `track_metadata` // cannot answer -- see the note above. Skipping it costs a phantom @@ -103,13 +106,39 @@ const ( func PreservePlaylistPhantoms( ctx context.Context, tx *sql.Tx, logger *slog.Logger, ) error { - if _, err := tx.ExecContext(ctx, preservePhantomPathSQL); err != nil { + return preservePlaylistPhantoms(ctx, tx, nil, logger) +} + +// PreservePlaylistPhantomsForFiles is PreservePlaylistPhantoms scoped to +// the given audio file ids, for the two removal paths that delete a +// known subset of the table rather than all of it: the incremental +// scan's orphan cleanup and RemoveFromLibrary. A bulk pass there would +// rewrite every linked playlist row on every scan for nothing. +func PreservePlaylistPhantomsForFiles( + ctx context.Context, tx *sql.Tx, ids []int64, logger *slog.Logger, +) error { + return preservePlaylistPhantoms(ctx, tx, ids, logger) +} + +func preservePlaylistPhantoms( + ctx context.Context, tx *sql.Tx, ids []int64, logger *slog.Logger, +) error { + clause, args, skip := phantomIDFilter(ids) + if skip { + return nil + } + + if _, err := tx.ExecContext( + ctx, preservePhantomPathSQL+clause, args..., + ); err != nil { return fmt.Errorf( "could not preserve playlist track file paths: %w", err, ) } - if _, err := tx.ExecContext(ctx, preservePhantomDisplaySQL); err != nil { + if _, err := tx.ExecContext( + ctx, preservePhantomDisplaySQL+clause, args..., + ); err != nil { // A failed statement does not roll back a SQLite transaction, // so the path half above stands and the entries remain // re-linkable. @@ -123,3 +152,26 @@ func PreservePlaylistPhantoms( return nil } + +// phantomIDFilter builds the extra WHERE terms and arguments that scope +// a preservation pass to a set of audio file ids. A nil ids returns the +// empty clause (a bulk run over every linked entry); an empty slice +// reports skip, since there is nothing to preserve. +func phantomIDFilter(ids []int64) (clause string, args []any, skip bool) { + switch { + case ids == nil: + return "", nil, false + case len(ids) == 0: + return "", nil, true + } + + clause = " AND audio_file_id IN (" + + strings.Repeat("?,", len(ids)-1) + "?)" + + args = make([]any, len(ids)) + for i, id := range ids { + args[i] = id + } + + return clause, args, false +} diff --git a/backend/database/playlistphantoms_test.go b/backend/database/playlistphantoms_test.go new file mode 100644 index 0000000..3aec6e8 --- /dev/null +++ b/backend/database/playlistphantoms_test.go @@ -0,0 +1,94 @@ +package database + +import ( + "context" + "database/sql" + "testing" +) + +// TestPreservePlaylistPhantomsForFilesScopesToTheRequestedIDs is the +// scoping half of the scoped variant: a run over one file's id must +// fill that file's playlist entries and leave every other entry alone, +// because the incremental scan calls this once per orphan batch and a +// pass that rewrote the whole table would touch every playlist row on +// every scan. +func TestPreservePlaylistPhantomsForFilesScopesToTheRequestedIDs(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 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), + (8, '/music/b.flac', 1, 2000, + 'Second Tide', 'Aurora Fields', 3, 4); + INSERT INTO playlist_tracks (playlist_id, audio_file_id, position) + VALUES (1, 7, 0), (1, 8, 1); + `); err != nil { + t.Fatalf("seed: %v", err) + } + + tx, err := db.BeginTx(ctx, nil) + if err != nil { + t.Fatalf("begin: %v", err) + } + + defer func() { _ = tx.Rollback() }() + + if err := PreservePlaylistPhantomsForFiles( + ctx, tx, []int64{7}, testLogger(), + ); err != nil { + t.Fatalf("preserve: %v", err) + } + + if err := tx.Commit(); err != nil { + t.Fatalf("commit: %v", err) + } + + var filled, untouched sql.NullString + + if err := db.QueryRowContext(ctx, + "SELECT phantom_file_path FROM playlist_tracks WHERE audio_file_id = 7", + ).Scan(&filled); err != nil { + t.Fatalf("read the requested entry: %v", err) + } + + if filled.String != "/music/a.flac" { + t.Errorf( + "requested entry phantom_file_path = %q, want %q", + filled.String, "/music/a.flac", + ) + } + + if err := db.QueryRowContext(ctx, + "SELECT phantom_file_path FROM playlist_tracks WHERE audio_file_id = 8", + ).Scan(&untouched); err != nil { + t.Fatalf("read the untouched entry: %v", err) + } + + if untouched.Valid { + t.Errorf( + "untouched entry got phantom_file_path = %q, want NULL "+ + "(a scoped run must not rewrite the whole table)", + untouched.String, + ) + } +} diff --git a/backend/library/library.go b/backend/library/library.go index e161c0f..0d96b86 100644 --- a/backend/library/library.go +++ b/backend/library/library.go @@ -911,30 +911,96 @@ func (l *Library) scanInternal( orphanStart := time.Now() + // Snapshot the orphan set first. The playlist-phantom + // preservation and the deletes are one transaction (the + // preservation has to land before the ON DELETE SET NULL, and + // both have to succeed or neither does), and the ids are what + // scope that preservation to just these files instead of + // rewriting every playlist row on a routine scan. + var ( + orphans []sqlcgen.AudioFile + orphanPaths []string + ) + existingPaths.Range(func(key, value any) bool { - path := key.(string) - audioFile := value.(sqlcgen.AudioFile) + orphanPaths = append(orphanPaths, key.(string)) + orphans = append(orphans, value.(sqlcgen.AudioFile)) - l.logger.Debug( - "removing orphaned database entry", - "path", path, "id", audioFile.ID, - ) + return true + }) - if err := l.db.Queries.DeleteAudioFile( - l.ctx, audioFile.ID, - ); err != nil { - l.logger.Warn( - "failed to delete orphaned audio file", - "path", path, - "id", audioFile.ID, - "err", err, - ) + deleted := make([]bool, len(orphans)) - metrics.addWarning(path, "orphan", err) - - return true + if len(orphans) > 0 { + orphanIDs := make([]int64, len(orphans)) + for i, f := range orphans { + orphanIDs[i] = f.ID } + tx, beginErr := l.db.BeginTx() + if beginErr != nil { + metrics.addWarning("", "orphan", beginErr) + } else { + defer func() { _ = tx.Rollback() }() // no-op after commit + + if err := database.PreservePlaylistPhantomsForFiles( + l.ctx, tx, orphanIDs, l.logger, + ); err != nil { + // Deleting without the phantoms is exactly the + // playlist-emptying bug the preservation exists to + // prevent, so leave the rows for the next scan + // rather than empty the playlists now. + l.logger.Error( + "skipping orphan deletion: could not preserve "+ + "playlist entries", + "err", err, + ) + + metrics.addWarning("", "orphan", err) + + _ = tx.Rollback() + } else { + txq := l.db.Queries.WithTx(tx) + + for i, f := range orphans { + if err := txq.DeleteAudioFile( + l.ctx, f.ID, + ); err != nil { + l.logger.Warn( + "failed to delete orphaned audio file", + "path", orphanPaths[i], + "id", f.ID, + "err", err, + ) + + metrics.addWarning(orphanPaths[i], "orphan", err) + + continue + } + + deleted[i] = true + } + + if err := tx.Commit(); err != nil { + l.logger.Error( + "could not commit orphan deletion", + "err", err, + ) + + metrics.addWarning("", "orphan", err) + } + } + } + } + + // Post-commit bookkeeping for the files that actually went. + for i, f := range orphans { + if !deleted[i] { + continue + } + + path := orphanPaths[i] + // Keep the file's tagging group in sync: drop the group's // track count and clear it out once empty, mirroring the // bookkeeping maybeRebindTaggingGroup does for a group_key @@ -942,25 +1008,25 @@ func (l *Library) scanInternal( // and replaced leaves a stale tagging_items row behind — // its track_count still counts the deleted files, and it // never clears from the autotag queue. - if audioFile.GroupKey != "" { + if f.GroupKey != "" { if err := l.db.Queries.DecrementTaggingItemTrackCount( - l.ctx, audioFile.GroupKey, + l.ctx, f.GroupKey, ); err != nil { l.logger.Warn( "failed to decrement tagging group for orphan", "path", path, - "group_key", audioFile.GroupKey, + "group_key", f.GroupKey, "err", err, ) metrics.addWarning(path, "orphan", err) } else if err := l.db.Queries.DeleteTaggingItemIfEmpty( - l.ctx, audioFile.GroupKey, + l.ctx, f.GroupKey, ); err != nil { l.logger.Warn( "failed to clean up emptied tagging group for orphan", "path", path, - "group_key", audioFile.GroupKey, + "group_key", f.GroupKey, "err", err, ) @@ -969,12 +1035,10 @@ func (l *Library) scanInternal( } // Remove from FTS5 search index. - if err := l.db.DeleteSearchIndex( - audioFile.ID, - ); err != nil { + if err := l.db.DeleteSearchIndex(f.ID); err != nil { l.logger.Warn( "failed to delete FTS entry for orphan", - "id", audioFile.ID, + "id", f.ID, "err", err, ) @@ -982,9 +1046,7 @@ func (l *Library) scanInternal( } removed.Add(1) - - return true - }) + } metrics.OrphanCleanup = time.Since(orphanStart) diff --git a/backend/library/removal_test.go b/backend/library/removal_test.go index 605eb1a..13ff8c8 100644 --- a/backend/library/removal_test.go +++ b/backend/library/removal_test.go @@ -94,6 +94,57 @@ func countRows( return n } +// RemoveFromLibrary is the one path that empties a track *deliberately*: +// the file stays on disk but is excluded, so nothing re-imports it. The +// playlist entry must still survive as a re-linkable phantom rather than +// an empty row, because a later full rescan clears the exclusion and is +// what re-links the entry then (#246). +func TestRemoveFromLibrary_PreservesPlaylistPhantoms(t *testing.T) { + t.Parallel() + + lib, db := setupTestLibrary(t) + + seedRemovableLibrary(t, lib, "/nonexistent/cover.jpg") + + if _, err := lib.db.ExecContext( + `INSERT INTO playlists (name) VALUES ('keepme')`, + ); err != nil { + t.Fatalf("seed playlist: %v", err) + } + + playlistID := queryInt( + t, db, `SELECT id FROM playlists WHERE name = 'keepme'`, + ) + + if _, err := lib.db.ExecContext( + `INSERT INTO playlist_tracks (playlist_id, audio_file_id, position) + SELECT ?, id, 0 FROM audio_files + WHERE file_path = '/music/song.mp3'`, + playlistID, + ); err != nil { + t.Fatalf("seed playlist_tracks: %v", err) + } + + if _, err := lib.RemoveFromLibrary([]string{"/music/song.mp3"}); err != nil { + t.Fatalf("RemoveFromLibrary: %v", err) + } + + phantomPath := queryString( + t, db, + `SELECT phantom_file_path FROM playlist_tracks + WHERE playlist_id = ?`, + playlistID, + ) + + if phantomPath != "/music/song.mp3" { + t.Fatalf( + "phantom_file_path is %q, want %q -- the entry cannot be "+ + "re-linked after a later full rescan", + phantomPath, "/music/song.mp3", + ) + } +} + // A library with tagging_items must still be removable. tagging_items // FK-references libraries with no ON DELETE clause, so leaving those // rows behind fails the DELETE and rolls back the entire removal. diff --git a/backend/library/remove_tracks.go b/backend/library/remove_tracks.go index a6e3098..83f5d77 100644 --- a/backend/library/remove_tracks.go +++ b/backend/library/remove_tracks.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" + "yellowjacket/backend/database" "yellowjacket/backend/events" ) @@ -57,6 +58,25 @@ func (l *Library) RemoveFromLibrary(filePaths []string) (*RemovalResult, error) var result RemovalResult + // Preserve the playlist entries before the rows go, so they survive + // as re-linkable phantoms rather than empty rows. The track is + // excluded and will not be re-imported on its own, but a later full + // rescan clears the exclusion and this is what lets the entry + // re-link then — the same preservation every other path that empties + // audio_files performs (#246). + rowIDs := make([]int64, len(rows)) + for i, row := range rows { + rowIDs[i] = row.ID + } + + if err := database.PreservePlaylistPhantomsForFiles( + l.ctx, tx, rowIDs, l.logger, + ); err != nil { + return nil, fmt.Errorf( + "could not preserve playlist entries for removal: %w", err, + ) + } + // Exclude every path the caller named, including one whose row has // already gone: the user asked for that file to stay out, and a row // that disappeared between the click and the commit is not a reason diff --git a/backend/library/scan_dirdisc_test.go b/backend/library/scan_dirdisc_test.go index 8d7ea62..e95008a 100644 --- a/backend/library/scan_dirdisc_test.go +++ b/backend/library/scan_dirdisc_test.go @@ -231,3 +231,98 @@ func TestScan_MultipleDirectoriesDoNotCrossContaminate(t *testing.T) { t.Errorf("Album A and Album B must not share a group_key: %+v", keys) } } + +// TestScan_OrphanCleanupPreservesPlaylistPhantoms guards #246: a file +// deleted from the library folder *outside* YellowJacket is discovered +// as an orphan by the next scan, and its playlist entry must survive as +// a re-linkable phantom — the same preservation the full rescan and +// stale-retire paths already perform, scoped here to just the orphaned +// file. Before the fix the entry became an empty row (audio_file_id +// NULL and no phantom_file_path), which nothing can ever re-link. +func TestScan_OrphanCleanupPreservesPlaylistPhantoms(t *testing.T) { + t.Parallel() + + lib, db := setupTestLibrary(t) + + root := t.TempDir() + track := filepath.Join(root, "gone.mp3") + + writeTestTrack(t, track, 0) + + library, err := db.Queries.CreateLibrary(lib.ctx, sqlcgen.CreateLibraryParams{ + Name: "orphans", + Path: root, + }) + if err != nil { + t.Fatalf("create library: %v", err) + } + + if metrics := lib.scanInternal(library.ID, library.Name, library.Path); metrics == nil { + t.Fatal("first scan returned nil metrics") + } + + trackID := queryInt( + t, db, "SELECT id FROM audio_files WHERE file_path = ?", track, + ) + if trackID == 0 { + t.Fatal("first scan did not import the track") + } + + if _, err := db.ExecContext( + `INSERT INTO playlists (name) VALUES ('keepme')`, + ); err != nil { + t.Fatalf("seed playlist: %v", err) + } + + playlistID := queryInt( + t, db, "SELECT id FROM playlists WHERE name = 'keepme'", + ) + + if _, err := db.ExecContext( + `INSERT INTO playlist_tracks (playlist_id, audio_file_id, position) + VALUES (?, ?, 0)`, + playlistID, trackID, + ); err != nil { + t.Fatalf("seed playlist_tracks: %v", err) + } + + // The file goes away outside the app. + if err := os.Remove(track); err != nil { + t.Fatalf("remove track: %v", err) + } + + if metrics := lib.scanInternal(library.ID, library.Name, library.Path); metrics == nil { + t.Fatal("second scan returned nil metrics") + } + + if n := queryInt( + t, db, "SELECT COUNT(*) FROM audio_files WHERE file_path = ?", track, + ); n != 0 { + t.Fatalf("audio_files still holds the removed path: %d rows", n) + } + + if n := queryInt( + t, db, + "SELECT COUNT(*) FROM playlist_tracks WHERE playlist_id = ? "+ + "AND audio_file_id IS NULL", + playlistID, + ); n != 1 { + t.Fatalf( + "playlist entry did not become a phantom: %d null-id rows, want 1", + n, + ) + } + + phantomPath := queryString( + t, db, + "SELECT phantom_file_path FROM playlist_tracks WHERE playlist_id = ?", + playlistID, + ) + if phantomPath != track { + t.Fatalf( + "phantom_file_path = %q, want %q -- the entry cannot be "+ + "re-linked if the file comes back", + phantomPath, track, + ) + } +}