From 0ffcb38eaec71ee923d374bfb2802ec5fccc25ca Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 17 Jul 2026 23:03:20 -0400 Subject: [PATCH] feat(album): clean up uploaded cover files when albums are purged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirror the artist purgeEmpty file cleanup: collect uploaded_image filenames of albums about to be purged and best-effort remove them from the data folder. Filenames still referenced by a surviving album are kept — CopyAttributes carries the filename (not the file) across album-ID changes, so two rows can legitimately share one image file. --- persistence/album_repository.go | 43 ++++++++++++++++++++++++++- persistence/album_repository_test.go | 44 ++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/persistence/album_repository.go b/persistence/album_repository.go index d59fef9a7..89ce7fb88 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -3,9 +3,11 @@ package persistence import ( "context" "encoding/json" + "errors" "fmt" "iter" "maps" + "os" "slices" "strings" "sync" @@ -14,6 +16,7 @@ import ( . "github.com/Masterminds/squirrel" "github.com/deluan/rest" "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/slice" @@ -373,11 +376,23 @@ on conflict (user_id, item_id, item_type) do update } func (r *albumRepository) purgeEmpty(libraryIDs ...int) error { - del := Delete(r.tableName).Where("id not in (select distinct(album_id) from media_file)") + orphanFilter := "id not in (select distinct(album_id) from media_file)" + + // Collect uploaded image filenames before deleting + sel := Select("uploaded_image").From(r.tableName). + Where(orphanFilter). + Where("uploaded_image <> ''") + del := Delete(r.tableName).Where(orphanFilter) // If libraryIDs are specified, only purge albums from those libraries if len(libraryIDs) > 0 { + sel = sel.Where(Eq{"library_id": libraryIDs}) del = del.Where(Eq{"library_id": libraryIDs}) } + var imageFiles []string + if err := r.queryAllSlice(sel, &imageFiles); err != nil && !errors.Is(err, model.ErrNotFound) { + return fmt.Errorf("collecting album images for cleanup: %w", err) + } + c, err := r.executeSQL(del) if err != nil { return fmt.Errorf("purging empty albums: %w", err) @@ -385,6 +400,32 @@ func (r *albumRepository) purgeEmpty(libraryIDs ...int) error { if c > 0 { log.Debug(r.ctx, "Purged empty albums", "totalDeleted", c) } + + if len(imageFiles) == 0 { + return nil + } + // CopyAttributes carries the filename (not the file) across album-ID changes, so a + // surviving album may still reference a purged album's image — keep those. + var stillUsed []string + if err := r.queryAllSlice(Select("uploaded_image").From(r.tableName).Where(Eq{"uploaded_image": imageFiles}), &stillUsed); err != nil && !errors.Is(err, model.ErrNotFound) { + return fmt.Errorf("checking album images still in use: %w", err) + } + used := make(map[string]struct{}, len(stillUsed)) + for _, f := range stillUsed { + used[f] = struct{}{} + } + + // Best-effort cleanup of uploaded image files + log.Debug(r.ctx, "Cleaning up album images", "totalImages", len(imageFiles)) + for _, filename := range imageFiles { + if _, ok := used[filename]; ok { + continue + } + path := model.UploadedImagePath(consts.EntityAlbum, filename) + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + log.Warn(r.ctx, "Failed to remove album image during GC", "path", path, err) + } + } return nil } diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index 64eb7d4a4..a3883cb1a 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -3,11 +3,14 @@ package persistence import ( "errors" "fmt" + "os" + "path/filepath" "time" "github.com/Masterminds/squirrel" "github.com/deluan/rest" "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/id" @@ -85,6 +88,47 @@ var _ = Describe("AlbumRepository", func() { }) }) + Describe("purgeEmpty image cleanup", func() { + var artDir string + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + tempDir := GinkgoT().TempDir() + conf.Server.DataFolder = conf.NewDir(tempDir) + artDir = filepath.Join(tempDir, "artwork", "album") + Expect(os.MkdirAll(artDir, 0755)).To(Succeed()) + + _, err := albumRepo.executeSQL(squirrel.Insert("library").Columns("id", "name", "path").Values(99, "purge-lib", "/tmp/purge-lib")) + Expect(err).ToNot(HaveOccurred()) + Expect(albumRepo.Put(&model.Album{ID: "purge-a", Name: "a", LibraryID: 99})).To(Succeed()) + Expect(albumRepo.Put(&model.Album{ID: "purge-b", Name: "b", LibraryID: 99})).To(Succeed()) + Expect(albumRepo.Put(&model.Album{ID: "purge-keeper", Name: "k", LibraryID: 1})).To(Succeed()) + Expect(albumRepo.UpdateImage("purge-a", "orphan.jpg")).To(Succeed()) + Expect(albumRepo.UpdateImage("purge-b", "shared.jpg")).To(Succeed()) + Expect(albumRepo.UpdateImage("purge-keeper", "shared.jpg")).To(Succeed()) + Expect(os.WriteFile(filepath.Join(artDir, "orphan.jpg"), []byte("x"), 0600)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(artDir, "shared.jpg"), []byte("x"), 0600)).To(Succeed()) + + DeferCleanup(func() { + _, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": []string{"purge-a", "purge-b", "purge-keeper"}})) + _, _ = albumRepo.executeSQL(squirrel.Delete("library").Where(squirrel.Eq{"id": 99})) + }) + }) + + It("removes orphaned image files but keeps ones still referenced elsewhere", func() { + Expect(albumRepo.purgeEmpty(99)).To(Succeed()) + + var ids []string + Expect(albumRepo.queryAllSlice(squirrel.Select("id").From("album").Where(squirrel.Eq{"id": []string{"purge-a", "purge-b"}}), &ids)).To(Succeed()) + Expect(ids).To(BeEmpty(), "purged album rows should be gone") + Expect(albumRepo.queryAllSlice(squirrel.Select("id").From("album").Where(squirrel.Eq{"id": "purge-keeper"}), &ids)).To(Succeed()) + Expect(ids).To(HaveLen(1), "album in another library must survive") + + _, err := os.Stat(filepath.Join(artDir, "orphan.jpg")) + Expect(os.IsNotExist(err)).To(BeTrue(), "orphaned image file should be removed") + Expect(filepath.Join(artDir, "shared.jpg")).To(BeAnExistingFile(), "file still referenced by a surviving album must be kept") + }) + }) + Describe("CopyAttributes", func() { var srcTime, dstTime time.Time BeforeEach(func() {