From b04d0c42cc16fa8ee34dbe5a6861b7b2be9a59d7 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 14 Aug 2026 15:22:37 -0400 Subject: [PATCH] refactor(artwork): own the kind list and the chain predicate in the package Export RecheckKinds and add WalksPriorityChain so the CLI stops keeping its own copies of both, and unexport externalCandidate, which nothing outside the package consumes. --- core/artwork/agent_images.go | 6 +++--- core/artwork/disc.go | 2 +- core/artwork/housekeeping.go | 14 ++++++-------- core/artwork/resolve.go | 12 +++++++++--- core/artwork/resolve_test.go | 12 ++++++++++++ core/artwork/trace.go | 9 ++++----- core/artwork/trace_test.go | 12 +----------- core/artwork/worker_test.go | 2 +- 8 files changed, 37 insertions(+), 32 deletions(-) diff --git a/core/artwork/agent_images.go b/core/artwork/agent_images.go index 35e3c666a..a6f746959 100644 --- a/core/artwork/agent_images.go +++ b/core/artwork/agent_images.go @@ -47,13 +47,13 @@ func fetchArtistImage(ctx context.Context, ag *agents.Agents, gate gateFunc, ar // Synthetic artists would otherwise get an unrelated agent result assigned to them. switch ar.ID { case consts.UnknownArtistID, consts.VariousArtistsID: - traceFrom(ctx).add(TraceStep{Candidate: ExternalCandidate, Outcome: OutcomeSkipped, Detail: "synthetic artist"}) + traceFrom(ctx).add(TraceStep{Candidate: externalCandidate, Outcome: OutcomeSkipped, Detail: "synthetic artist"}) return nil, "", false } name := externalName(ar.Name) imageAgents := ag.ArtistImageAgents() if len(imageAgents) == 0 { - traceFrom(ctx).add(TraceStep{Candidate: ExternalCandidate, Outcome: OutcomeSkipped, + traceFrom(ctx).add(TraceStep{Candidate: externalCandidate, Outcome: OutcomeSkipped, Detail: "no enabled agent provides artist images"}) return nil, "", false } @@ -85,7 +85,7 @@ func fetchAlbumImage(ctx context.Context, ag *agents.Agents, gate gateFunc, al m name, artist := externalName(al.Name), externalName(al.AlbumArtist) imageAgents := ag.AlbumImageAgents() if len(imageAgents) == 0 { - traceFrom(ctx).add(TraceStep{Candidate: ExternalCandidate, Outcome: OutcomeSkipped, + traceFrom(ctx).add(TraceStep{Candidate: externalCandidate, Outcome: OutcomeSkipped, Detail: "no enabled agent provides album images"}) return nil, "", false } diff --git a/core/artwork/disc.go b/core/artwork/disc.go index cfc372142..33789d910 100644 --- a/core/artwork/disc.go +++ b/core/artwork/disc.go @@ -123,7 +123,7 @@ func (d *discArtworkReader) fromDiscArtPriority(ctx context.Context, ffmpeg ffmp fromTag(ctx, d.lib.FS, d.firstTrackRel), fromFFmpegTag(ctx, ffmpeg, d.lib.Abs(d.firstTrackRel)), ) - case pattern == ExternalCandidate: + case pattern == externalCandidate: // Not supported for disc art, silently ignore case pattern == "discsubtitle": if subtitle := strings.TrimSpace(d.album.Discs[d.discNumber]); subtitle != "" { diff --git a/core/artwork/housekeeping.go b/core/artwork/housekeeping.go index 9cfad32bc..5a0937fd1 100644 --- a/core/artwork/housekeeping.go +++ b/core/artwork/housekeeping.go @@ -21,15 +21,15 @@ import ( // StaleAbsentAge is how long an absent state is trusted before a recheck retries it. const StaleAbsentAge = 24 * time.Hour -// recheckKinds omits media files: they resolve embedded-only, at scan or on view. -var recheckKinds = []model.Kind{ +// RecheckKinds omits media files: they resolve embedded-only, at scan or on view. +var RecheckKinds = []model.Kind{ model.KindArtistArtwork, model.KindAlbumArtwork, model.KindPlaylistArtwork, model.KindRadioArtwork, } // hasRecheckPath reports whether a periodic job will revisit this kind, making an absent settle recoverable. func hasRecheckPath(prefix string) bool { kind, ok := model.ParseKind(prefix) - return ok && slices.Contains(recheckKinds, kind) + return ok && slices.Contains(RecheckKinds, kind) } // artworkEpoch invalidates all resolution state when bumped; bump it whenever resolution semantics change. @@ -41,8 +41,7 @@ type FingerprintInput struct { Value string } -// FingerprintInputs is the single listing of what ConfigFingerprint hashes, so a caller can report -// which setting a fingerprint change came from without keeping a second copy of the list. +// FingerprintInputs is the single listing of what ConfigFingerprint hashes. func FingerprintInputs() []FingerprintInput { return []FingerprintInput{ {"CoverArtPriority", conf.Server.CoverArtPriority}, @@ -55,7 +54,6 @@ func FingerprintInputs() []FingerprintInput { } // ConfigFingerprint covers the inputs that affect resolution outcomes; a change invalidates stored state. -// Exported so the CLI reports the value backfill compares instead of computing one that can drift. func ConfigFingerprint() string { values := slice.Map(FingerprintInputs(), func(i FingerprintInput) string { return i.Value }) raw := fmt.Sprintf("%s|%d", strings.Join(values, "|"), artworkEpoch) @@ -119,7 +117,7 @@ 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 recheckKinds { + for _, kind := range RecheckKinds { if _, err := queue.EnqueueStaleAbsent(kind, cutoff); err != nil { return err } @@ -130,7 +128,7 @@ func enqueueStaleAbsentAll(ctx context.Context, ds model.DataStore) error { // enqueueMissingAll is 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 { + for _, kind := range RecheckKinds { if _, err := queue.EnqueueAllMissing(kind, model.ArtworkPriorityRecheck); err != nil { return err } diff --git a/core/artwork/resolve.go b/core/artwork/resolve.go index cc5bbf1b5..2a36e6dfd 100644 --- a/core/artwork/resolve.go +++ b/core/artwork/resolve.go @@ -108,6 +108,12 @@ func (r *resolver) resolve(ctx context.Context, item model.ArtworkQueueItem) (re } } +// WalksPriorityChain reports whether the resolver picks this kind's artwork from a configurable +// priority chain; playlists and radios resolve from a fixed order, so there is no walk to explain. +func WalksPriorityChain(kind model.Kind) bool { + return kind == model.KindArtistArtwork || kind == model.KindAlbumArtwork +} + // fetchExternalAlbum and fetchExternalArtist are the only places resolution touches the network, // so a local-only resolver is stopped here rather than at each point in the chain walk. func (r *resolver) fetchExternalAlbum(ctx context.Context, al model.Album) (io.ReadCloser, string, bool) { @@ -151,7 +157,7 @@ func (r *resolver) resolveAlbum(ctx context.Context, albumID string) (resolution if res, ok = chain.try(pattern, res, ok); ok { return res, nil } - case pattern == ExternalCandidate: + case pattern == externalCandidate: if rd, name, isErr := r.fetchExternalAlbum(ctx, *al); rd != nil { return resolution{reader: rd, source: ExternalPrefix + name}, nil } else if isErr { @@ -218,7 +224,7 @@ func (r *resolver) resolveArtist(ctx context.Context, artistID string) (resoluti continue } switch { - case pattern == ExternalCandidate: + case pattern == externalCandidate: if rd, name, isErr := r.fetchExternalArtist(ctx, *ar); rd != nil { return resolution{reader: rd, source: ExternalPrefix + name}, nil } else if isErr { @@ -385,7 +391,7 @@ func (r *resolver) resolveMediaFile(ctx context.Context, id string) (resolution, func resolveExternalStep(gate gateFunc, name string, sf sourceFunc) (res resolution, ok bool, extErr bool) { r, path, err := gate(name, sf) if r != nil { - return resolution{reader: r, source: ExternalCandidate, sourcePath: path}, true, false + return resolution{reader: r, source: externalCandidate, sourcePath: path}, true, false } return resolution{}, false, err != nil && !errors.Is(err, model.ErrNotFound) } diff --git a/core/artwork/resolve_test.go b/core/artwork/resolve_test.go index 9a0962cf2..23b1ef436 100644 --- a/core/artwork/resolve_test.go +++ b/core/artwork/resolve_test.go @@ -630,3 +630,15 @@ var _ = Describe("decodeTile", func() { Expect(err).To(HaveOccurred()) }) }) + +var _ = Describe("WalksPriorityChain", func() { + It("is true for the kinds the resolver walks", func() { + Expect(WalksPriorityChain(model.KindArtistArtwork)).To(BeTrue()) + Expect(WalksPriorityChain(model.KindAlbumArtwork)).To(BeTrue()) + }) + + It("is false for the kinds resolved directly", func() { + Expect(WalksPriorityChain(model.KindPlaylistArtwork)).To(BeFalse()) + Expect(WalksPriorityChain(model.KindRadioArtwork)).To(BeFalse()) + }) +}) diff --git a/core/artwork/trace.go b/core/artwork/trace.go index 26e5c7b99..5d02fa4ff 100644 --- a/core/artwork/trace.go +++ b/core/artwork/trace.go @@ -8,8 +8,7 @@ import ( "sync" ) -// Outcome is what the priority chain observed for one candidate. The CLI branches on these, -// so they are part of the package's API, not private labels. +// Outcome is what the priority chain observed for one candidate; the CLI renders and branches on these. type Outcome string const ( @@ -22,10 +21,10 @@ const ( ) const ( - // ExternalCandidate labels the external tier itself, for the cases that never reach an agent. - ExternalCandidate = "external" + // externalCandidate labels the external tier itself, for the cases that never reach an agent. + externalCandidate = "external" // ExternalPrefix qualifies a candidate or a stored source with the agent that produced it. - ExternalPrefix = ExternalCandidate + ":" + ExternalPrefix = externalCandidate + ":" ) // TraceStep is one candidate the priority chain considered. diff --git a/core/artwork/trace_test.go b/core/artwork/trace_test.go index 24c32a4b0..06068323a 100644 --- a/core/artwork/trace_test.go +++ b/core/artwork/trace_test.go @@ -26,7 +26,7 @@ var _ = Describe("trace vocabulary", func() { Expect([]Outcome{ OutcomeHit, OutcomeMiss, OutcomeUnreadable, OutcomeSkipped, OutcomeWouldTry, OutcomeError, }).To(Equal([]Outcome{"hit", "miss", "unreadable", "skipped", "would-try", "error"})) - Expect(ExternalCandidate).To(Equal("external")) + Expect(externalCandidate).To(Equal("external")) Expect(ExternalPrefix).To(Equal("external:")) }) }) @@ -111,11 +111,6 @@ var _ = Describe("chainState tracing", func() { {Candidate: "embedded", Outcome: OutcomeHit, Detail: "/music/a.flac"}, })) }) - - It("does not panic when no trace is attached", func() { - c := chainState{} - Expect(func() { _, _ = c.try("cover.*", resolution{}, false) }).ToNot(Panic()) - }) }) var _ = Describe("external gate tracing", func() { @@ -414,11 +409,6 @@ var _ = Describe("NewTracingResolver", func() { t = &ChainTrace{} }) - It("uses the offline gate when live is false", func() { - r := NewTracingResolver(nil, nil, nil, t, false) - Expect(r).ToNot(BeNil()) - }) - Context("offline", func() { var fake *fakeImageAgent diff --git a/core/artwork/worker_test.go b/core/artwork/worker_test.go index 9fad4b503..53b6a43b2 100644 --- a/core/artwork/worker_test.go +++ b/core/artwork/worker_test.go @@ -362,7 +362,7 @@ var _ = Describe("Worker", func() { Expect(ia.Hash).To(Equal("cafebabe"), "a persistent outage must not discard served art") }) - // Media files are excluded from recheckKinds, so an absent row here would never be + // Media files are excluded from RecheckKinds, so an absent row here would never be // revisited: a transient read error would look permanent. It("does not settle absent on exhaustion for a kind with no recheck path", func() { conf.Server.EnableMediaFileCoverArt = true