mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-01 07:21:17 +00:00
feat(artwork): make artwork re-resolution targeted, not blunt
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.
This commit is contained in:
parent
f7261b00ef
commit
fc8b4a7ea3
@ -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)
|
||||
|
||||
@ -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
|
||||
}
|
||||
|
||||
@ -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())
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@ -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)
|
||||
}
|
||||
|
||||
@ -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)
|
||||
|
||||
@ -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")
|
||||
})
|
||||
})
|
||||
|
||||
@ -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
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user