From fc8b4a7ea3d70631dc3355552ce93976aef5d363 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 24 Jul 2026 20:42:14 -0400 Subject: [PATCH] feat(artwork): make artwork re-resolution targeted, not blunt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps in when the pipeline re-resolves artwork: The recheck job only requeued absent-state rows (hash=''), so an entity that was never processed — added between scans, or on a server with the scanner disabled — had no periodic safety net and stayed without artwork indefinitely. Add EnqueueMissing(kind): a SQL set-difference enqueueing entities with no item_artwork row at Recheck priority (ON CONFLICT DO NOTHING, so it never disturbs a queued row). Run it once at startup and hourly alongside the stale-absent recheck. Rename staleAbsentKinds -> recheckKinds accordingly. Conversely, the config fingerprint included consts.Version, which embeds the git SHA and so changed on every build, re-enqueueing every entity in the library (~34k here) and re-querying external agents at the configured RPS for anything without local art. Replace it with an explicit artworkEpoch constant, bumped deliberately when resolution semantics change. The cases that motivated the version input — absent art becoming available — are already covered by the stale-absent and missing-row rechecks; only a corrected wrong-pick needs the epoch. A test guards against reintroducing the version. --- cmd/root.go | 9 ++++ core/artwork/housekeeping.go | 30 +++++++++---- core/artwork/housekeeping_test.go | 44 ++++++++++++++++++++ model/artwork.go | 3 ++ persistence/artwork_queue_repository.go | 17 ++++++++ persistence/artwork_queue_repository_test.go | 41 ++++++++++++++++++ tests/mock_artwork_queue_repo.go | 42 +++++++++++++++++++ 7 files changed, 179 insertions(+), 7 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 4df5c1cb8..799dd6ab5 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -368,6 +368,9 @@ func scheduleArtworkHousekeeping(ctx context.Context, worker *artwork.Worker) fu if err := artwork.EnqueueStaleAbsentAll(ctx, ds); err != nil { log.Error(ctx, "Error enqueueing stale artwork rechecks", err) } + if err := artwork.EnqueueMissingAll(ctx, ds); err != nil { + log.Error(ctx, "Error enqueueing missing artwork rechecks", err) + } }); err != nil { log.Error(ctx, "Error scheduling artwork stale-absent recheck", err) } @@ -380,6 +383,12 @@ func scheduleArtworkHousekeeping(ctx context.Context, worker *artwork.Worker) fu log.Error(ctx, "Error scheduling artwork prune", err) } + // Also run the missing-row recheck once at startup so a never-scanned entity is picked up + // immediately, not only on the next hourly tick (e.g. after enabling the feature). + if err := artwork.EnqueueMissingAll(ctx, ds); err != nil { + log.Error(ctx, "Error enqueueing missing artwork rechecks", err) + } + backfilled, err := artwork.Backfill(ctx, ds) if err != nil { log.Error(ctx, "Error running artwork backfill", err) diff --git a/core/artwork/housekeeping.go b/core/artwork/housekeeping.go index 400c48a69..99172f017 100644 --- a/core/artwork/housekeeping.go +++ b/core/artwork/housekeeping.go @@ -8,7 +8,6 @@ import ( "time" "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -21,17 +20,22 @@ const FingerprintPropertyKey = "artwork.fingerprint" // staleAbsentAge is how old an absent resolution must be before the recheck job retries it. const staleAbsentAge = 24 * time.Hour -// staleAbsentKinds are the item kinds eligible for the periodic stale-absent recheck. -var staleAbsentKinds = []model.Kind{ +// recheckKinds are the item kinds eligible for the periodic recheck jobs (stale-absent and +// missing-row). Media files are excluded: they resolve embedded-only, at scan or on view. +var recheckKinds = []model.Kind{ model.KindArtistArtwork, model.KindAlbumArtwork, model.KindPlaylistArtwork, model.KindRadioArtwork, } -// Fingerprint summarizes the config knobs that affect artwork resolution outcomes; a +// artworkEpoch invalidates all resolution state when bumped; bump it in the same change that +// alters resolution semantics. Deliberately not the server version, which changes every build. +const artworkEpoch = 1 + +// Fingerprint summarizes the inputs that affect artwork resolution outcomes; a // change means previously resolved (or absent) state may no longer be correct. func Fingerprint() string { - raw := fmt.Sprintf("%s|%s|%s|%s|%t|%t|%s", + raw := fmt.Sprintf("%s|%s|%s|%s|%t|%t|%d", conf.Server.CoverArtPriority, conf.Server.ArtistArtPriority, conf.Server.ArtistImageFolder, - conf.Server.Agents, conf.Server.EnableExternalServices, conf.Server.EnableM3UExternalAlbumArt, consts.Version) + conf.Server.Agents, conf.Server.EnableExternalServices, conf.Server.EnableM3UExternalAlbumArt, artworkEpoch) sum := md5.Sum([]byte(raw)) //nolint:gosec // fingerprint, not security-sensitive return hex.EncodeToString(sum[:]) } @@ -95,10 +99,22 @@ func enqueueBackfillKind(ctx context.Context, ds model.DataStore, kind model.Kin func EnqueueStaleAbsentAll(ctx context.Context, ds model.DataStore) error { cutoff := time.Now().Add(-staleAbsentAge) queue := ds.ArtworkQueue(ctx) - for _, kind := range staleAbsentKinds { + for _, kind := range recheckKinds { if _, err := queue.EnqueueStaleAbsent(kind, cutoff); err != nil { return err } } return nil } + +// EnqueueMissingAll requeues entities that have no item_artwork row yet, across every recheck +// kind: the safety net for entities a scan never enqueued (added between scans, or scanner off). +func EnqueueMissingAll(ctx context.Context, ds model.DataStore) error { + queue := ds.ArtworkQueue(ctx) + for _, kind := range recheckKinds { + if _, err := queue.EnqueueMissing(kind); err != nil { + return err + } + } + return nil +} diff --git a/core/artwork/housekeeping_test.go b/core/artwork/housekeeping_test.go index 1df8312e8..cd182ca22 100644 --- a/core/artwork/housekeeping_test.go +++ b/core/artwork/housekeeping_test.go @@ -6,6 +6,7 @@ import ( "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/tests" @@ -110,6 +111,15 @@ var _ = Describe("Housekeeping", func() { conf.Server.EnableM3UExternalAlbumArt = true Expect(Fingerprint()).NotTo(Equal(f1)) }) + + It("does not change when the server version changes", func() { + original := consts.Version + DeferCleanup(func() { consts.Version = original }) + f1 := Fingerprint() + consts.Version = original + "-next" + Expect(Fingerprint()).To(Equal(f1), + "the version must not invalidate artwork state: it would re-resolve every entity on every build") + }) }) Describe("Backfill", func() { @@ -223,4 +233,38 @@ var _ = Describe("Housekeeping", func() { Expect(findQueued(queueRepo.MockArtworkQueueRepo, "al", "al2")).To(BeNil()) }) }) + + Describe("EnqueueMissingAll", func() { + var artRepo *tests.MockArtworkRepo + + BeforeEach(func() { + artRepo = tests.CreateMockArtworkRepo() + ds.MockedArtwork = artRepo + queueRepo.ItemArtworkSource = artRepo + queueRepo.ExistingIDs = map[string]map[string]bool{ + "al": {"al1": true, "al2": true}, + "ar": {"ar1": true}, + "pl": {"pl1": true}, + "ra": {"ra1": true}, + } + }) + + It("enqueues only entities that have no item_artwork row, across all kinds", func() { + // al1 is already resolved and ar1 already absent: both must be skipped. + artRepo.ItemData["al-resolved"] = model.ItemArtwork{ItemKind: "al", ItemID: "al1", ImageType: model.ImageTypePrimary, Hash: "somehash", AttemptedAt: time.Now()} + artRepo.ItemData["ar-absent"] = model.ItemArtwork{ItemKind: "ar", ItemID: "ar1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()} + + err := EnqueueMissingAll(ctx, ds) + Expect(err).ToNot(HaveOccurred()) + + for _, it := range queueRepo.Data { + Expect(it.Priority).To(Equal(model.ArtworkPriorityRecheck)) + } + Expect(findQueued(queueRepo.MockArtworkQueueRepo, "al", "al2")).ToNot(BeNil()) + Expect(findQueued(queueRepo.MockArtworkQueueRepo, "pl", "pl1")).ToNot(BeNil()) + Expect(findQueued(queueRepo.MockArtworkQueueRepo, "ra", "ra1")).ToNot(BeNil()) + Expect(findQueued(queueRepo.MockArtworkQueueRepo, "al", "al1")).To(BeNil()) + Expect(findQueued(queueRepo.MockArtworkQueueRepo, "ar", "ar1")).To(BeNil()) + }) + }) }) diff --git a/model/artwork.go b/model/artwork.go index 8bbd7a2cb..b2dae0024 100644 --- a/model/artwork.go +++ b/model/artwork.go @@ -112,6 +112,9 @@ type ArtworkQueueRepository interface { Count() (int64, error) // EnqueueStaleAbsent inserts queue rows (priority Recheck) for absent states older than cutoff. EnqueueStaleAbsent(kind Kind, attemptedBefore time.Time) (int64, error) + // EnqueueMissing inserts queue rows (priority Recheck) for entities of the kind that have no + // item_artwork row at all, so a never-processed entity is eventually resolved even without a scan. + EnqueueMissing(kind Kind) (int64, error) // PurgeDangling removes queue rows whose entity no longer exists. PurgeDangling() (int64, error) } diff --git a/persistence/artwork_queue_repository.go b/persistence/artwork_queue_repository.go index ba8c42f95..7fd7cd387 100644 --- a/persistence/artwork_queue_repository.go +++ b/persistence/artwork_queue_repository.go @@ -2,6 +2,7 @@ package persistence import ( "context" + "fmt" "slices" "time" @@ -120,4 +121,20 @@ func (r *artworkQueueRepository) EnqueueStaleAbsent(kind model.Kind, attemptedBe return r.executeSQL(ins) } +func (r *artworkQueueRepository) EnqueueMissing(kind model.Kind) (int64, error) { + entityTable, ok := danglingItemArtworkKinds[kind] + if !ok { + return 0, fmt.Errorf("artwork queue: no entity table for kind %q", kind.Prefix()) + } + now := time.Now() + // DO NOTHING is deliberate: rechecks must not bump priority/retry_at of already-queued items. + ins := Expr(`INSERT INTO `+r.tableName+` (item_kind, item_id, image_type, priority, attempts, retry_at, enqueued_at) + SELECT ?, id, ?, ?, 0, ?, ? + FROM `+entityTable+` + WHERE id NOT IN (SELECT item_id FROM `+itemArtworkTable+` WHERE item_kind = ?) + ON CONFLICT (item_kind, item_id, image_type) DO NOTHING`, + kind.Prefix(), model.ImageTypePrimary, model.ArtworkPriorityRecheck, now, now, kind.Prefix()) + return r.executeSQL(ins) +} + var _ model.ArtworkQueueRepository = (*artworkQueueRepository)(nil) diff --git a/persistence/artwork_queue_repository_test.go b/persistence/artwork_queue_repository_test.go index 10015a225..e7c0967a1 100644 --- a/persistence/artwork_queue_repository_test.go +++ b/persistence/artwork_queue_repository_test.go @@ -184,4 +184,45 @@ var _ = Describe("ArtworkQueueRepository", func() { Expect(items[0].ItemID).To(Equal("stale1")) Expect(items[0].Priority).To(Equal(model.ArtworkPriorityRecheck)) }) + + It("enqueues entities that have no item_artwork row at all", func() { + awRepo := NewArtworkRepository(context.Background(), GetDBXBuilder()) + // albumSgtPeppers has a resolved row and albumAbbeyRoad an absent row: both already + // processed, so neither should be enqueued as "missing". + Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: albumSgtPeppers.ID, ImageType: model.ImageTypePrimary, Hash: "hX", AttemptedAt: time.Now()})).To(Succeed()) + Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: albumAbbeyRoad.ID, ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()})).To(Succeed()) + + n, err := repo.EnqueueMissing(model.KindAlbumArtwork) + Expect(err).ToNot(HaveOccurred()) + Expect(n).To(BeNumerically(">=", 1)) + + got, err := repo.DequeueBatch(1000) + Expect(err).ToNot(HaveOccurred()) + ids := make([]string, 0, len(got)) + for _, it := range got { + Expect(it.ItemKind).To(Equal("al")) + Expect(it.Priority).To(Equal(model.ArtworkPriorityRecheck)) + ids = append(ids, it.ItemID) + } + Expect(ids).To(ContainElement(albumRadioactivity.ID), "an album with no row must be enqueued") + Expect(ids).ToNot(ContainElement(albumSgtPeppers.ID), "a resolved album must not be re-enqueued") + Expect(ids).ToNot(ContainElement(albumAbbeyRoad.ID), "an absent-state album must not be enqueued as missing") + }) + + It("does not disturb an already-queued entity when enqueueing missing rows", func() { + Expect(repo.Enqueue(item("al", albumRadioactivity.ID, model.ArtworkPriorityBump))).To(Succeed()) + + _, err := repo.EnqueueMissing(model.KindAlbumArtwork) + Expect(err).ToNot(HaveOccurred()) + + got, _ := repo.DequeueBatch(1000) + var count int + for _, it := range got { + if it.ItemID == albumRadioactivity.ID { + count++ + Expect(it.Priority).To(Equal(model.ArtworkPriorityBump), "existing bump priority must survive") + } + } + Expect(count).To(Equal(1), "the already-queued row must not be duplicated") + }) }) diff --git a/tests/mock_artwork_queue_repo.go b/tests/mock_artwork_queue_repo.go index 43bc6218d..19bb3b6e8 100644 --- a/tests/mock_artwork_queue_repo.go +++ b/tests/mock_artwork_queue_repo.go @@ -214,3 +214,45 @@ func (m *MockArtworkQueueRepo) EnqueueStaleAbsent(kind model.Kind, attemptedBefo } return inserted, nil } + +// EnqueueMissing enqueues entities in ExistingIDs[kind] that have no item_artwork row in +// ItemArtworkSource and are not already queued, mirroring the SQL set-difference insert. +func (m *MockArtworkQueueRepo) EnqueueMissing(kind model.Kind) (int64, error) { + m.mu.Lock() + defer m.mu.Unlock() + if m.Err != nil { + return 0, m.Err + } + hasRow := func(id string) bool { + if m.ItemArtworkSource == nil { + return false + } + for _, ia := range m.ItemArtworkSource.ItemData { + if ia.ItemKind == kind.Prefix() && ia.ItemID == id { + return true + } + } + return false + } + now := time.Now() + var inserted int64 + for id := range m.ExistingIDs[kind.Prefix()] { + if hasRow(id) { + continue + } + k := iaKey(kind.Prefix(), id, model.ImageTypePrimary) + if _, ok := m.Data[k]; ok { // DO NOTHING: never touch existing queue rows + continue + } + m.Data[k] = model.ArtworkQueueItem{ + ItemKind: kind.Prefix(), + ItemID: id, + ImageType: model.ImageTypePrimary, + Priority: model.ArtworkPriorityRecheck, + RetryAt: now, + EnqueuedAt: now, + } + inserted++ + } + return inserted, nil +}