diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index 7f3d6154..e75a0e58 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -4,7 +4,9 @@ import ( "cmp" "context" "encoding/json" + "errors" "fmt" + "os" "slices" "strings" "time" @@ -12,6 +14,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" @@ -315,7 +318,19 @@ func (r *artistRepository) GetIndex(includeMissing bool, libraryIds []int, roles } func (r *artistRepository) purgeEmpty() error { - del := Delete(r.tableName).Where("id not in (select artist_id from album_artists)") + orphanFilter := "id not in (select artist_id from album_artists)" + + // Collect uploaded image filenames before deleting + sel := Select("uploaded_image").From(r.tableName). + Where(orphanFilter). + Where("uploaded_image != ''") + var imageFiles []string + if err := r.queryAllSlice(sel, &imageFiles); err != nil && !errors.Is(err, model.ErrNotFound) { + return fmt.Errorf("collecting artist images for cleanup: %w", err) + } + + // Delete orphan artists + del := Delete(r.tableName).Where(orphanFilter) c, err := r.executeSQL(del) if err != nil { return fmt.Errorf("purging empty artists: %w", err) @@ -323,6 +338,19 @@ func (r *artistRepository) purgeEmpty() error { if c > 0 { log.Debug(r.ctx, "Purged empty artists", "totalDeleted", c) } + + if len(imageFiles) == 0 { + return nil + } + + // Best-effort cleanup of uploaded image files + log.Debug(r.ctx, "Cleaning up artist images", "totalImages", len(imageFiles)) + for _, filename := range imageFiles { + path := model.UploadedImagePath(consts.EntityArtist, filename) + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + log.Warn(r.ctx, "Failed to remove artist image during GC", "path", path, err) + } + } return nil } diff --git a/persistence/artist_repository_test.go b/persistence/artist_repository_test.go index 90e449e8..e2904466 100644 --- a/persistence/artist_repository_test.go +++ b/persistence/artist_repository_test.go @@ -3,11 +3,14 @@ package persistence import ( "context" "encoding/json" + "os" + "path/filepath" "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/request" "github.com/navidrome/navidrome/utils" @@ -829,6 +832,89 @@ var _ = Describe("ArtistRepository", func() { }) }) }) + + Describe("purgeEmpty", func() { + var repo *artistRepository + var tmpDir string + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + tmpDir = GinkgoT().TempDir() + conf.Server.DataFolder = tmpDir + + ctx := request.WithUser(GinkgoT().Context(), adminUser) + repo = NewArtistRepository(ctx, GetDBXBuilder()).(*artistRepository) + }) + + // Helper to create an artist image file on disk and return its path + createImageFile := func(filename string) string { + dir := filepath.Join(tmpDir, consts.ArtworkFolder, consts.EntityArtist) + Expect(os.MkdirAll(dir, 0755)).To(Succeed()) + path := filepath.Join(dir, filename) + Expect(os.WriteFile(path, []byte("fake image data"), 0600)).To(Succeed()) + return path + } + + It("removes uploaded image files for purged artists", func() { + // Create an orphan artist (not in album_artists) with an uploaded image + orphanArtist := model.Artist{ID: "orphan-with-image", Name: "Orphan Artist", UploadedImage: "orphan-with-image_Orphan_Artist.jpg"} + Expect(repo.Put(&orphanArtist)).To(Succeed()) + imgPath := createImageFile("orphan-with-image_Orphan_Artist.jpg") + + Expect(repo.purgeEmpty()).To(Succeed()) + + // Artist should be gone from DB + exists, err := repo.Exists("orphan-with-image") + Expect(err).ToNot(HaveOccurred()) + Expect(exists).To(BeFalse()) + + // Image file should be removed from disk + _, err = os.Stat(imgPath) + Expect(os.IsNotExist(err)).To(BeTrue()) + }) + + It("handles missing image files gracefully", func() { + // Artist has UploadedImage set but no actual file on disk + orphanArtist := model.Artist{ID: "orphan-no-file", Name: "Ghost Image", UploadedImage: "orphan-no-file_Ghost_Image.jpg"} + Expect(repo.Put(&orphanArtist)).To(Succeed()) + + Expect(repo.purgeEmpty()).To(Succeed()) + + // Artist should be gone from DB + exists, err := repo.Exists("orphan-no-file") + Expect(err).ToNot(HaveOccurred()) + Expect(exists).To(BeFalse()) + }) + + It("does not delete images for artists that are kept", func() { + // Create an artist with an uploaded image AND an album_artists entry so it won't be purged + keptArtist := model.Artist{ID: "kept-artist", Name: "Kept Artist", UploadedImage: "kept-artist_Kept_Artist.jpg"} + Expect(repo.Put(&keptArtist)).To(Succeed()) + imgPath := createImageFile("kept-artist_Kept_Artist.jpg") + + // Insert an album_artists record to keep this artist from being purged + _, err := repo.executeSQL(squirrel.Insert("album_artists"). + SetMap(map[string]any{"album_id": "101", "artist_id": "kept-artist", "role": "artist", "sub_role": ""})) + Expect(err).ToNot(HaveOccurred()) + + DeferCleanup(func() { + _, _ = repo.executeSQL(squirrel.Delete("album_artists").Where(squirrel.Eq{"artist_id": "kept-artist"})) + _ = repo.delete(squirrel.Eq{"id": "kept-artist"}) + }) + + Expect(repo.purgeEmpty()).To(Succeed()) + + // Artist should still exist (check directly, bypassing library filter) + var ids []string + err = repo.queryAllSlice(squirrel.Select("id").From("artist").Where(squirrel.Eq{"id": "kept-artist"}), &ids) + Expect(err).ToNot(HaveOccurred()) + Expect(ids).To(HaveLen(1)) + + // Image file should still be on disk + _, err = os.Stat(imgPath) + Expect(err).ToNot(HaveOccurred()) + }) + }) }) // Helper function to create an artist with proper library association. diff --git a/persistence/persistence_suite_test.go b/persistence/persistence_suite_test.go index 0ee1570a..3ed44312 100644 --- a/persistence/persistence_suite_test.go +++ b/persistence/persistence_suite_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "testing" + "github.com/Masterminds/squirrel" _ "github.com/mattn/go-sqlite3" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/db" @@ -211,6 +212,27 @@ var _ = BeforeSuite(func() { } } + // Populate album_artists based on the AlbumArtistID relationships in testAlbums + artistIDs := map[string]bool{} + for _, a := range testArtists { + artistIDs[a.ID] = true + } + for i := range testAlbums { + a := testAlbums[i] + if a.AlbumArtistID == "" || !artistIDs[a.AlbumArtistID] { + continue + } + _, err := alr.executeSQL(squirrel.Insert("album_artists").SetMap(map[string]any{ + "album_id": a.ID, + "artist_id": a.AlbumArtistID, + "role": "artist", + "sub_role": "", + })) + if err != nil { + panic(err) + } + } + mr := NewMediaFileRepository(ctx, conn) for i := range testSongs { err := mr.Put(&testSongs[i])