From 32bb64918cd09b116146b3ee9029c0e5a58414ef Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Wed, 9 Sep 2026 10:14:07 -0400 Subject: [PATCH] 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)) + } + } +}