From 1b9868ddd02bb266c70ca1633be002234e086600 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Wed, 9 Sep 2026 10:09:17 -0400 Subject: [PATCH 1/2] fix(library): preserve playlist phantoms on incremental scan and removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The incremental scan's orphan cleanup and RemoveFromLibrary deleted audio_files rows without first filling the playlist phantom columns, so a track removed from the library folder outside YellowJacket (or removed from the library) became a permanently empty playlist row that nothing could re-link — the same bug #183 fixed on the full-rescan and retire paths, on the two paths it missed. Add a scoped PreservePlaylistPhantomsForFiles and run it in the same transaction as the deletes on both paths. Closes #246 --- backend/database/playlistphantoms.go | 68 ++++++++++-- backend/database/playlistphantoms_test.go | 94 +++++++++++++++++ backend/library/library.go | 122 ++++++++++++++++------ backend/library/removal_test.go | 51 +++++++++ backend/library/remove_tracks.go | 20 ++++ backend/library/scan_dirdisc_test.go | 95 +++++++++++++++++ 6 files changed, 412 insertions(+), 38 deletions(-) create mode 100644 backend/database/playlistphantoms_test.go 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, + ) + } +} From 32bb64918cd09b116146b3ee9029c0e5a58414ef Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Wed, 9 Sep 2026 10:14:07 -0400 Subject: [PATCH 2/2] fix(library): sweep orphaned cover art when an album empties MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pruneEmptyEntities deleted empty albums but never the cover_art rows they referenced, so removing the last track of an album leaked the row and its files forever — the janitor's covers sweep computes its live set from cover_art.file_path, which keeps the orphaned row's files exempt. Extract sweepOrphanedCoverArt/removeCoverArtFiles as one implementation and run it from pruneEmptyEntities (scan orphan path and RemoveFromLibrary) and RemoveLibrary alike. Closes #247 --- backend/library/coverart.go | 72 +++++++++++++++++++++++++++++++++ backend/library/crud.go | 58 ++++---------------------- backend/library/library.go | 17 +++++++- backend/library/removal_test.go | 50 +++++++++++++++++++++++ 4 files changed, 146 insertions(+), 51 deletions(-) diff --git a/backend/library/coverart.go b/backend/library/coverart.go index 659e6c9..f13243c 100644 --- a/backend/library/coverart.go +++ b/backend/library/coverart.go @@ -3,6 +3,7 @@ package library import ( "bytes" "crypto/sha256" + "database/sql" "encoding/hex" "fmt" "image" @@ -69,6 +70,77 @@ func CoverArtFileSet(coverPath string) []string { return paths } +// sweepOrphanedCoverArt deletes the cover_art rows no album references +// and returns their file paths, for the caller to remove from disk +// after the transaction commits. Cover art is referenced only by +// albums.cover_art_id, so an orphan is a cover whose album is gone — +// which is every album the caller just swept. +// +// One implementation because the scan path, RemoveFromLibrary and +// RemoveLibrary all reach this state, and the scan side used to skip it +// entirely while RemoveLibrary did it inline (#247). +func (l *Library) sweepOrphanedCoverArt(tx *sql.Tx) ([]string, error) { + const orphanSQL = ` + SELECT file_path FROM cover_art WHERE id NOT IN ( + SELECT DISTINCT cover_art_id FROM albums + WHERE cover_art_id IS NOT NULL + )` + + rows, err := tx.QueryContext(l.ctx, orphanSQL) + if err != nil { + return nil, fmt.Errorf("could not query orphaned cover art: %w", err) + } + + var paths []string + + for rows.Next() { + var filePath string + + if err := rows.Scan(&filePath); err != nil { + l.logger.Warn("could not scan cover art path", "err", err) + + continue + } + + paths = append(paths, filePath) + } + + // Close before the DELETE: the two run on the one writer connection. + if err := rows.Close(); err != nil { + l.logger.Warn("could not close cover art rows", "err", err) + } + + if len(paths) > 0 { + if _, err := tx.ExecContext(l.ctx, ` + DELETE FROM cover_art WHERE id NOT IN ( + SELECT DISTINCT cover_art_id FROM albums + WHERE cover_art_id IS NOT NULL + )`); err != nil { + return nil, fmt.Errorf("could not delete orphaned cover_art: %w", err) + } + } + + return paths, nil +} + +// removeCoverArtFiles removes a cover original and its derived size +// variants. Only the original is stored in cover_art.file_path; the +// _sm/_md/_lg tiers are derived filenames beside it, so they have to be +// removed by name or they accumulate forever. +func (l *Library) removeCoverArtFiles(coverPaths []string) { + for _, coverPath := range coverPaths { + for _, path := range CoverArtFileSet(coverPath) { + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + l.logger.Warn( + "could not remove orphaned cover art file", + "path", path, + "err", err, + ) + } + } + } +} + // saveCoverArt saves embedded cover art to the cache directory. // Returns the file path where the art was saved, or empty string // if no picture data. Timing is recorded in the provided metrics. diff --git a/backend/library/crud.go b/backend/library/crud.go index a7ddbf4..132bd53 100644 --- a/backend/library/crud.go +++ b/backend/library/crud.go @@ -320,43 +320,12 @@ func (l *Library) RemoveLibrary(id int64) (*RemovalSummary, error) { genresRemoved, _ := result.RowsAffected() - // 15. Collect orphaned cover_art file paths for post-commit cleanup. - // SAFETY: Hand-crafted SELECT for orphaned cover art identification. - // Parameterless. - rows, err := tx.QueryContext(l.ctx, - `SELECT file_path FROM cover_art WHERE id NOT IN ( - SELECT DISTINCT cover_art_id FROM albums - WHERE cover_art_id IS NOT NULL - )`) + // Collect and delete orphaned cover_art rows before the commit. The + // shared helper is the one place this sweep lives, so the scan path, + // RemoveFromLibrary and this removal cannot drift (#247). + orphanedCoverArtPaths, err := l.sweepOrphanedCoverArt(tx) if err != nil { - return nil, fmt.Errorf("could not query orphaned cover art: %w", err) - } - - var orphanedCoverArtPaths []string - - for rows.Next() { - var filePath string - if err := rows.Scan(&filePath); err != nil { - l.logger.Warn("could not scan cover art path", "err", err) - - continue - } - - orphanedCoverArtPaths = append(orphanedCoverArtPaths, filePath) - } - - if err := rows.Close(); err != nil { - l.logger.Warn("could not close cover art rows", "err", err) - } - - // 16. Delete orphaned cover_art rows. - // SAFETY: Hand-crafted orphan cleanup SQL. Parameterless. - if _, err := tx.ExecContext(l.ctx, - `DELETE FROM cover_art WHERE id NOT IN ( - SELECT DISTINCT cover_art_id FROM albums - WHERE cover_art_id IS NOT NULL - )`); err != nil { - return nil, fmt.Errorf("could not delete orphaned cover_art: %w", err) + return nil, err } // 17. Delete the library's tagging queue. tagging_items holds a @@ -392,20 +361,9 @@ func (l *Library) RemoveLibrary(id int64) (*RemovalSummary, error) { // avoids a costly full re-index of all remaining tracks (~10s for // 25K tracks). - // 21. Post-commit: Delete orphaned cover art files and their sized - // variants. Only the original is stored in cover_art.file_path; the - // _sm/_md/_lg thumbnails are derived filenames beside it, so they - // have to be removed by name or they accumulate forever. - for _, coverPath := range orphanedCoverArtPaths { - for _, path := range CoverArtFileSet(coverPath) { - if err := os.Remove(path); err != nil && !os.IsNotExist(err) { - l.logger.Warn("could not remove orphaned cover art file", - "path", path, - "err", err, - ) - } - } - } + // Post-commit: remove the orphaned cover art files and their sized + // variants. + l.removeCoverArtFiles(orphanedCoverArtPaths) // 22. Post-commit: Compact queue. if l.removalHooks.CompactQueue != nil { diff --git a/backend/library/library.go b/backend/library/library.go index e161c0f..b8f40ac 100644 --- a/backend/library/library.go +++ b/backend/library/library.go @@ -1193,17 +1193,32 @@ func (l *Library) pruneEmptyEntities() { } } + // Cover art after albums: a cover whose album just went is + // unreferenced, and leaving the row behind keeps its files exempt + // from the janitor's covers sweep forever (#247). + orphanedCovers, err := l.sweepOrphanedCoverArt(tx) + if err != nil { + l.logger.Warn("could not sweep orphaned cover art", "err", err) + + return + } + if err := tx.Commit(); err != nil { l.logger.Warn("could not commit entity cleanup", "err", err) return } - if len(albumIDs) > 0 || len(artistIDs) > 0 || len(genreIDs) > 0 { + // Post-commit: the rows are gone, so their files can go too. + l.removeCoverArtFiles(orphanedCovers) + + if len(albumIDs) > 0 || len(artistIDs) > 0 || len(genreIDs) > 0 || + len(orphanedCovers) > 0 { l.logger.Info("pruned empty library entities", "albums", len(albumIDs), "artists", len(artistIDs), "genres", len(genreIDs), + "covers", len(orphanedCovers), ) } } diff --git a/backend/library/removal_test.go b/backend/library/removal_test.go index 605eb1a..9ddd35d 100644 --- a/backend/library/removal_test.go +++ b/backend/library/removal_test.go @@ -198,3 +198,53 @@ func TestCoverArtFileSet(t *testing.T) { } } } + +// Removing the last track of an album must take the album's cover art +// with it — both the row and every derived file — or the row keeps its +// files exempt from the janitor's covers sweep forever (#247). +func TestRemoveFromLibrary_DeletesOrphanedCoverArt(t *testing.T) { + t.Parallel() + + lib, _ := setupTestLibrary(t) + + dir := t.TempDir() + + // The largest tier is what cover_art.file_path names; write every + // variant so the sweep has a real set to remove. + for _, tier := range thumbnailTiers { + p := filepath.Join(dir, coverart.SizedFilename("abc123.jpg", tier.Suffix)) + if err := os.WriteFile(p, []byte("img"), 0o600); err != nil { + t.Fatalf("write %s: %v", p, err) + } + } + + cover := filepath.Join(dir, coverart.SizedFilename("abc123.jpg", "_lg")) + + seedRemovableLibrary(t, lib, cover) + + // Link the album to the cover so it is not orphaned until the track + // (and with it the album) goes. + if _, err := lib.db.ExecContext( + `UPDATE albums SET cover_art_id = + (SELECT id FROM cover_art WHERE file_path = ?) + WHERE name = 'Test Album'`, + cover, + ); err != nil { + t.Fatalf("link cover art: %v", err) + } + + if _, err := lib.RemoveFromLibrary([]string{"/music/song.mp3"}); err != nil { + t.Fatalf("RemoveFromLibrary: %v", err) + } + + if n := countRows(t, lib, "cover_art"); n != 0 { + t.Errorf("cover_art has %d rows after removal, want 0", n) + } + + for _, tier := range thumbnailTiers { + p := filepath.Join(dir, coverart.SizedFilename("abc123.jpg", tier.Suffix)) + if _, err := os.Stat(p); !os.IsNotExist(err) { + t.Errorf("cover art file still present: %s", filepath.Base(p)) + } + } +}