fix(library): sweep orphaned cover art when an album empties
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
This commit is contained in:
@@ -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.
|
||||
|
||||
+8
-50
@@ -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 {
|
||||
|
||||
@@ -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),
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user