From 25a05fd017e0226a8560c92e687ff1c77c13e4ed Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 22 Jul 2026 10:21:48 -0400 Subject: [PATCH] feat(artwork): enqueue artwork resolution from scan and CRUD paths --- core/artwork/prune.go | 9 +++++++ core/artwork/prune_test.go | 19 ++++++++++++++ model/artwork.go | 2 ++ persistence/artwork_repository.go | 24 +++++++++++++++++ persistence/artwork_repository_test.go | 36 ++++++++++++++++++++++++++ persistence/radio_repository.go | 12 ++++++++- persistence/radio_repository_test.go | 21 +++++++++++++++ scanner/phase_1_folders.go | 17 ++++++++++++ scanner/phase_4_playlists.go | 5 ++++ scanner/phase_4_playlists_test.go | 23 ++++++++++++++++ scanner/scanner_test.go | 23 ++++++++++++++++ tests/mock_artwork_repo.go | 21 +++++++++++++++ 12 files changed, 211 insertions(+), 1 deletion(-) diff --git a/core/artwork/prune.go b/core/artwork/prune.go index a40194b8a..5278beaa4 100644 --- a/core/artwork/prune.go +++ b/core/artwork/prune.go @@ -13,6 +13,15 @@ const pruneMinAge = time.Hour func Prune(ctx context.Context, ds model.DataStore, store *ImageStore) error { repo := ds.Artwork(ctx) + + purged, err := repo.PurgeDanglingItemArtwork() + if err != nil { + return err + } + if purged > 0 { + log.Info(ctx, "Prune: purged dangling item artwork state", "count", purged) + } + // One grace cutoff for both the DB orphan check and the file sweep: files younger // than the window may belong to acquisitions whose rows aren't committed yet. cutoff := time.Now().Add(-pruneMinAge) diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index 1f621363b..983e7c555 100644 --- a/core/artwork/prune_test.go +++ b/core/artwork/prune_test.go @@ -39,6 +39,25 @@ var _ = Describe("Prune", func() { awRepo.Data[h] = a } + It("purges dangling item_artwork state for gone entities, summed across kinds", func() { + Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: "gone-album", ImageType: model.ImageTypePrimary})).To(Succeed()) + Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "gone-artist", ImageType: model.ImageTypePrimary})).To(Succeed()) + Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "live-artist", ImageType: model.ImageTypePrimary})).To(Succeed()) + awRepo.ExistingIDs = map[string]map[string]bool{ + "al": {}, + "ar": {"live-artist": true}, + } + + Expect(Prune(context.Background(), ds, store)).To(Succeed()) + + _, err := awRepo.GetItemArtwork("al", "gone-album", model.ImageTypePrimary) + Expect(err).To(MatchError(model.ErrNotFound)) + _, err = awRepo.GetItemArtwork("ar", "gone-artist", model.ImageTypePrimary) + Expect(err).To(MatchError(model.ErrNotFound)) + _, err = awRepo.GetItemArtwork("ar", "live-artist", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + }) + It("deletes orphan rows and their store files, keeps referenced ones", func() { data := []byte("orphan-bytes") h, _ := HashImage(bytes.NewReader(data)) diff --git a/model/artwork.go b/model/artwork.go index 7cb117764..73db00ca5 100644 --- a/model/artwork.go +++ b/model/artwork.go @@ -77,6 +77,8 @@ type ArtworkRepository interface { GetInfoForItems(kind string, ids []string) (map[string]ItemArtworkInfo, error) // GetAllMimes returns hash -> current mime for every stored artwork, for sweep retention checks. GetAllMimes() (map[string]string, error) + // PurgeDanglingItemArtwork removes state rows whose entity no longer exists. + PurgeDanglingItemArtwork() (int64, error) } type ArtworkQueueRepository interface { diff --git a/persistence/artwork_repository.go b/persistence/artwork_repository.go index 505e3839d..622f47bfe 100644 --- a/persistence/artwork_repository.go +++ b/persistence/artwork_repository.go @@ -114,6 +114,30 @@ func (r *artworkRepository) DeleteOrphans(createdBefore time.Time, hashes []stri return nil } +// danglingItemArtworkKinds maps item_kind prefixes to the table that owns the entity. +var danglingItemArtworkKinds = map[string]string{ + "al": "album", + "ar": "artist", + "pl": "playlist", + "ra": "radio", +} + +func (r *artworkRepository) PurgeDanglingItemArtwork() (int64, error) { + var total int64 + for kind, table := range danglingItemArtworkKinds { + del := Delete(itemArtworkTable).Where(And{ + Eq{"item_kind": kind}, + Expr("item_id NOT IN (SELECT id FROM " + table + ")"), + }) + c, err := r.items.executeSQL(del) + if err != nil { + return total, err + } + total += c + } + return total, nil +} + func (r *artworkRepository) GetItemArtwork(kind, id, imageType string) (*model.ItemArtwork, error) { sel := Select("*").From(itemArtworkTable). Where(Eq{"item_kind": kind, "item_id": id, "image_type": imageType}) diff --git a/persistence/artwork_repository_test.go b/persistence/artwork_repository_test.go index 82c2888c8..ae6752def 100644 --- a/persistence/artwork_repository_test.go +++ b/persistence/artwork_repository_test.go @@ -136,6 +136,42 @@ var _ = Describe("ArtworkRepository", func() { }) }) + Context("dangling state cleanup", func() { + It("purges item_artwork rows per kind whose entity no longer exists, summing counts", func() { + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: albumSgtPeppers.ID, ImageType: model.ImageTypePrimary, Hash: "keepAl"})).To(Succeed()) + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: "no-such-album", ImageType: model.ImageTypePrimary, Hash: "danglingAl"})).To(Succeed()) + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: artistKraftwerk.ID, ImageType: model.ImageTypePrimary, Hash: "keepAr"})).To(Succeed()) + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "no-such-artist", ImageType: model.ImageTypePrimary, Hash: "danglingAr"})).To(Succeed()) + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "pl", ItemID: plsBest.ID, ImageType: model.ImageTypePrimary, Hash: "keepPl"})).To(Succeed()) + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "pl", ItemID: "no-such-playlist", ImageType: model.ImageTypePrimary, Hash: "danglingPl"})).To(Succeed()) + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ra", ItemID: radioWithHomePage.ID, ImageType: model.ImageTypePrimary, Hash: "keepRa"})).To(Succeed()) + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ra", ItemID: "no-such-radio", ImageType: model.ImageTypePrimary, Hash: "danglingRa"})).To(Succeed()) + + purged, err := repo.PurgeDanglingItemArtwork() + Expect(err).ToNot(HaveOccurred()) + Expect(purged).To(Equal(int64(4))) + + for _, kept := range []model.ItemArtwork{ + {ItemKind: "al", ItemID: albumSgtPeppers.ID}, + {ItemKind: "ar", ItemID: artistKraftwerk.ID}, + {ItemKind: "pl", ItemID: plsBest.ID}, + {ItemKind: "ra", ItemID: radioWithHomePage.ID}, + } { + _, err := repo.GetItemArtwork(kept.ItemKind, kept.ItemID, model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + } + for _, gone := range []model.ItemArtwork{ + {ItemKind: "al", ItemID: "no-such-album"}, + {ItemKind: "ar", ItemID: "no-such-artist"}, + {ItemKind: "pl", ItemID: "no-such-playlist"}, + {ItemKind: "ra", ItemID: "no-such-radio"}, + } { + _, err := repo.GetItemArtwork(gone.ItemKind, gone.ItemID, model.ImageTypePrimary) + Expect(err).To(MatchError(model.ErrNotFound)) + } + }) + }) + Context("item state", func() { It("upserts and reads state, including per-item provenance", func() { ia := &model.ItemArtwork{ItemKind: "al", ItemID: "al1", ImageType: model.ImageTypePrimary, diff --git a/persistence/radio_repository.go b/persistence/radio_repository.go index a073643db..20f82ee36 100644 --- a/persistence/radio_repository.go +++ b/persistence/radio_repository.go @@ -7,6 +7,7 @@ import ( . "github.com/Masterminds/squirrel" "github.com/deluan/rest" + "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/id" "github.com/pocketbase/dbx" @@ -72,7 +73,16 @@ func (r *radioRepository) Put(radio *model.Radio, colsToUpdate ...string) error colsToUpdate = append(colsToUpdate, "UpdatedAt") } _, err := r.put(radio.ID, radio, colsToUpdate...) - return err + if err != nil { + return err + } + // Enqueue artwork resolution for the created/updated radio. Never fails the save. + item := model.ArtworkQueueItem{ItemKind: "ra", ItemID: radio.ID, ImageType: model.ImageTypePrimary, + Priority: model.ArtworkPriorityScan} + if err := NewArtworkQueueRepository(r.ctx, r.db).Enqueue(item); err != nil { + log.Warn(r.ctx, "could not enqueue radio artwork", "id", radio.ID, err) + } + return nil } func (r *radioRepository) Count(options ...rest.QueryOptions) (int64, error) { diff --git a/persistence/radio_repository_test.go b/persistence/radio_repository_test.go index 05628ca41..1d6cce4f8 100644 --- a/persistence/radio_repository_test.go +++ b/persistence/radio_repository_test.go @@ -107,6 +107,27 @@ var _ = Describe("RadioRepository", func() { Expect(err).To(BeNil()) Expect(all[2].StreamUrl).To(Equal("https://example.com:4533/app")) }) + + It("enqueues artwork resolution for the saved radio", func() { + err := repo.Put(&model.Radio{ + Name: "Artwork radio", + StreamUrl: "https://example.com:4533/artwork", + }) + Expect(err).To(BeNil()) + + all, err := repo.GetAll() + Expect(err).To(BeNil()) + created := all[len(all)-1] + + queueRepo := NewArtworkQueueRepository(context.Background(), GetDBXBuilder()) + queued, err := queueRepo.DequeueBatch(1000) + Expect(err).To(BeNil()) + Expect(queued).To(ContainElement(SatisfyAll( + HaveField("ItemKind", "ra"), + HaveField("ItemID", created.ID), + HaveField("Priority", model.ArtworkPriorityScan), + ))) + }) }) }) diff --git a/scanner/phase_1_folders.go b/scanner/phase_1_folders.go index 5e898590b..3a7265cc4 100644 --- a/scanner/phase_1_folders.go +++ b/scanner/phase_1_folders.go @@ -332,6 +332,8 @@ func (p *phaseFolders) persistChanges(entry *folderEntry) (*folderEntry, error) // Collect artwork IDs to pre-cache after the transaction commits var artworkIDs []model.ArtworkID + // Collect artwork queue items for changed albums/artists, enqueued in the same transaction + var queueItems []model.ArtworkQueueItem err := p.ds.WithTx(func(tx model.DataStore) error { // Instantiate all repositories just once per folder @@ -372,6 +374,10 @@ func (p *phaseFolders) persistChanges(entry *folderEntry) (*folderEntry, error) } if entry.artists[i].Name != consts.UnknownArtist && entry.artists[i].Name != consts.VariousArtists { artworkIDs = append(artworkIDs, entry.artists[i].CoverArtID()) + queueItems = append(queueItems, model.ArtworkQueueItem{ + ItemKind: "ar", ItemID: entry.artists[i].ID, ImageType: model.ImageTypePrimary, + Priority: model.ArtworkPriorityScan, + }) } } @@ -384,6 +390,10 @@ func (p *phaseFolders) persistChanges(entry *folderEntry) (*folderEntry, error) } if entry.albums[i].Name != consts.UnknownAlbum { artworkIDs = append(artworkIDs, entry.albums[i].CoverArtID()) + queueItems = append(queueItems, model.ArtworkQueueItem{ + ItemKind: "al", ItemID: entry.albums[i].ID, ImageType: model.ImageTypePrimary, + Priority: model.ArtworkPriorityScan, + }) } } @@ -415,6 +425,13 @@ func (p *phaseFolders) persistChanges(entry *folderEntry) (*folderEntry, error) return err } } + + // Enqueue artwork resolution for changed albums/artists. Never fails the scan. + if len(queueItems) > 0 { + if err := tx.ArtworkQueue(p.ctx).Enqueue(queueItems...); err != nil { + log.Warn(p.ctx, "Scanner: could not enqueue artwork resolution", "folder", entry.path, err) + } + } return nil }, "scanner: persist changes") if err != nil { diff --git a/scanner/phase_4_playlists.go b/scanner/phase_4_playlists.go index 8ba014235..15798b054 100644 --- a/scanner/phase_4_playlists.go +++ b/scanner/phase_4_playlists.go @@ -149,6 +149,11 @@ func (p *phasePlaylists) processPlaylistsInFolder(folder *model.Folder) (*model. log.Debug("Scanner: Imported playlist", "name", pls.Name, "lastUpdated", pls.UpdatedAt, "path", pls.Path, "numTracks", len(pls.Tracks), "elapsed", time.Since(started)) } p.cw.PreCache(pls.CoverArtID()) + item := model.ArtworkQueueItem{ItemKind: "pl", ItemID: pls.ID, ImageType: model.ImageTypePrimary, + Priority: model.ArtworkPriorityScan} + if err := p.ds.ArtworkQueue(p.ctx).Enqueue(item); err != nil { + log.Warn(p.ctx, "Scanner: could not enqueue playlist artwork", "id", pls.ID, err) + } p.refreshed.Add(1) } return folder, nil diff --git a/scanner/phase_4_playlists_test.go b/scanner/phase_4_playlists_test.go index 49ffc7fb7..1adf34a63 100644 --- a/scanner/phase_4_playlists_test.go +++ b/scanner/phase_4_playlists_test.go @@ -193,6 +193,29 @@ var _ = Describe("phasePlaylists", func() { Expect(phase.refreshed.Load()).To(Equal(uint32(2))) }) + It("enqueues artwork resolution for the imported playlist", func() { + libPath := GinkgoT().TempDir() + folder := &model.Folder{LibraryPath: libPath, Path: "path/to", Name: "folder"} + _ = os.MkdirAll(folder.AbsolutePath(), 0755) + + file1 := filepath.Join(folder.AbsolutePath(), "playlist1.m3u") + _ = os.WriteFile(file1, []byte{}, 0600) + + pls.On("ImportFromFolder", mock.Anything, folder, "playlist1.m3u"). + Return(&model.Playlist{ID: "pl1"}, nil) + + _, err := phase.processPlaylistsInFolder(folder) + Expect(err).ToNot(HaveOccurred()) + + queued, err := ds.ArtworkQueue(ctx).DequeueBatch(10) + Expect(err).ToNot(HaveOccurred()) + Expect(queued).To(ContainElement(SatisfyAll( + HaveField("ItemKind", "pl"), + HaveField("ItemID", "pl1"), + HaveField("Priority", model.ArtworkPriorityScan), + ))) + }) + It("reports an error if there is an error reading files", func() { tests.SkipOnWindows("relies on Unix /etc filesystem") progress := make(chan *ProgressInfo) diff --git a/scanner/scanner_test.go b/scanner/scanner_test.go index cc3720717..0a7fb07b2 100644 --- a/scanner/scanner_test.go +++ b/scanner/scanner_test.go @@ -152,6 +152,29 @@ var _ = Describe("Scanner", Ordered, func() { HaveField("SongCount", Equal(4)), )) }) + It("should enqueue artwork resolution for the scanned albums and artists", func() { + Expect(runScanner(ctx, true)).To(Succeed()) + + albums, _ := ds.Album(ctx).GetAll() + artists, _ := ds.Artist(ctx).GetAll(model.QueryOptions{Filters: squirrel.NotEq{"name": consts.UnknownArtist}}) + queued, err := ds.ArtworkQueue(ctx).DequeueBatch(1000) + Expect(err).ToNot(HaveOccurred()) + + for _, al := range albums { + Expect(queued).To(ContainElement(SatisfyAll( + HaveField("ItemKind", "al"), + HaveField("ItemID", al.ID), + HaveField("Priority", model.ArtworkPriorityScan), + ))) + } + for _, ar := range artists { + Expect(queued).To(ContainElement(SatisfyAll( + HaveField("ItemKind", "ar"), + HaveField("ItemID", ar.ID), + HaveField("Priority", model.ArtworkPriorityScan), + ))) + } + }) }) When("a file was changed", func() { It("should update the media_file", func() { diff --git a/tests/mock_artwork_repo.go b/tests/mock_artwork_repo.go index 13ad4d5e1..db73af84d 100644 --- a/tests/mock_artwork_repo.go +++ b/tests/mock_artwork_repo.go @@ -12,6 +12,8 @@ type MockArtworkRepo struct { ItemData map[string]model.ItemArtwork // keyed by iaKey(kind, id, imageType) OrphanHashes []string Err error + // ExistingIDs, keyed by item_kind, backs PurgeDanglingItemArtwork; nil map keeps everything. + ExistingIDs map[string]map[string]bool } func CreateMockArtworkRepo() *MockArtworkRepo { @@ -68,6 +70,25 @@ func (m *MockArtworkRepo) GetAllMimes() (map[string]string, error) { return mimes, nil } +func (m *MockArtworkRepo) PurgeDanglingItemArtwork() (int64, error) { + if m.Err != nil { + return 0, m.Err + } + var purged int64 + for k, ia := range m.ItemData { + // A nil per-kind map means that kind isn't tracked by the test, so keep it. + existing := m.ExistingIDs[ia.ItemKind] + if existing == nil { + continue + } + if !existing[ia.ItemID] { + delete(m.ItemData, k) + purged++ + } + } + return purged, nil +} + func (m *MockArtworkRepo) DeleteOrphans(createdBefore time.Time, hashes []string) error { if m.Err != nil { return m.Err