fix(server): clean up uploaded artist images during GC
When artists are purged during garbage collection, any custom uploaded cover images were left orphaned on disk. Modified purgeEmpty() to query for uploaded_image filenames before the bulk DELETE, then remove the corresponding files from disk afterwards. Image cleanup is best-effort to avoid failing the GC if a file is already missing or inaccessible. Also populated album_artists entries in the persistence test suite setup to reflect the actual album-artist relationships from test data, ensuring purgeEmpty() doesn't inadvertently delete shared test artists.
This commit is contained in:
@@ -4,7 +4,9 @@ import (
|
|||||||
"cmp"
|
"cmp"
|
||||||
"context"
|
"context"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
|
"os"
|
||||||
"slices"
|
"slices"
|
||||||
"strings"
|
"strings"
|
||||||
"time"
|
"time"
|
||||||
@@ -12,6 +14,7 @@ import (
|
|||||||
. "github.com/Masterminds/squirrel"
|
. "github.com/Masterminds/squirrel"
|
||||||
"github.com/deluan/rest"
|
"github.com/deluan/rest"
|
||||||
"github.com/navidrome/navidrome/conf"
|
"github.com/navidrome/navidrome/conf"
|
||||||
|
"github.com/navidrome/navidrome/consts"
|
||||||
"github.com/navidrome/navidrome/log"
|
"github.com/navidrome/navidrome/log"
|
||||||
"github.com/navidrome/navidrome/model"
|
"github.com/navidrome/navidrome/model"
|
||||||
"github.com/navidrome/navidrome/utils"
|
"github.com/navidrome/navidrome/utils"
|
||||||
@@ -315,7 +318,19 @@ func (r *artistRepository) GetIndex(includeMissing bool, libraryIds []int, roles
|
|||||||
}
|
}
|
||||||
|
|
||||||
func (r *artistRepository) purgeEmpty() error {
|
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)
|
c, err := r.executeSQL(del)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return fmt.Errorf("purging empty artists: %w", err)
|
return fmt.Errorf("purging empty artists: %w", err)
|
||||||
@@ -323,6 +338,19 @@ func (r *artistRepository) purgeEmpty() error {
|
|||||||
if c > 0 {
|
if c > 0 {
|
||||||
log.Debug(r.ctx, "Purged empty artists", "totalDeleted", c)
|
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
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -3,11 +3,14 @@ package persistence
|
|||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
|
"os"
|
||||||
|
"path/filepath"
|
||||||
|
|
||||||
"github.com/Masterminds/squirrel"
|
"github.com/Masterminds/squirrel"
|
||||||
"github.com/deluan/rest"
|
"github.com/deluan/rest"
|
||||||
"github.com/navidrome/navidrome/conf"
|
"github.com/navidrome/navidrome/conf"
|
||||||
"github.com/navidrome/navidrome/conf/configtest"
|
"github.com/navidrome/navidrome/conf/configtest"
|
||||||
|
"github.com/navidrome/navidrome/consts"
|
||||||
"github.com/navidrome/navidrome/model"
|
"github.com/navidrome/navidrome/model"
|
||||||
"github.com/navidrome/navidrome/model/request"
|
"github.com/navidrome/navidrome/model/request"
|
||||||
"github.com/navidrome/navidrome/utils"
|
"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.
|
// Helper function to create an artist with proper library association.
|
||||||
|
|||||||
@@ -5,6 +5,7 @@ import (
|
|||||||
"path/filepath"
|
"path/filepath"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
|
"github.com/Masterminds/squirrel"
|
||||||
_ "github.com/mattn/go-sqlite3"
|
_ "github.com/mattn/go-sqlite3"
|
||||||
"github.com/navidrome/navidrome/conf"
|
"github.com/navidrome/navidrome/conf"
|
||||||
"github.com/navidrome/navidrome/db"
|
"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)
|
mr := NewMediaFileRepository(ctx, conn)
|
||||||
for i := range testSongs {
|
for i := range testSongs {
|
||||||
err := mr.Put(&testSongs[i])
|
err := mr.Put(&testSongs[i])
|
||||||
|
|||||||
Reference in New Issue
Block a user