From 4bad716e0b2e552bc33330cb8d7f8aa2d503e5a0 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 14 Aug 2026 12:23:17 -0400 Subject: [PATCH] feat(artwork): trace the local priority chain --- core/artwork/resolve.go | 44 +++++++++---- core/artwork/trace.go | 3 + core/artwork/trace_test.go | 129 +++++++++++++++++++++++++++++++++++++ 3 files changed, 164 insertions(+), 12 deletions(-) diff --git a/core/artwork/resolve.go b/core/artwork/resolve.go index 11711469d..2d3b9de14 100644 --- a/core/artwork/resolve.go +++ b/core/artwork/resolve.go @@ -34,18 +34,33 @@ type resolution struct { // chainState carries what a priority walk has seen so far. A hit takes extErr with it so a // transient external failure still retries; localErr is dropped, as the scanner re-lists changes. -type chainState struct{ extErr, localErr bool } +type chainState struct { + extErr, localErr bool + trace *chainTrace // nil unless the CLI asked for a trace +} // try stamps the accumulated external failure onto a hit, and records the miss otherwise. -func (c *chainState) try(res resolution, ok bool) (resolution, bool) { +func (c *chainState) try(candidate string, res resolution, ok bool) (resolution, bool) { if ok { res.extError = c.extErr + c.record(candidate, outcomeHit, res.sourcePath) return res, true } c.localErr = c.localErr || res.localError + if res.localError { + c.record(candidate, outcomeUnreadable, "") + } else { + c.record(candidate, outcomeMiss, "") + } return resolution{}, false } +func (c *chainState) record(candidate, outcome, detail string) { + if c.trace != nil { + c.trace.add(traceStep{Candidate: candidate, Outcome: outcome, Detail: detail}) + } +} + // exhausted is the outcome when no source in the chain yielded an image. func (c *chainState) exhausted() resolution { return resolution{extError: c.extErr, localError: c.localErr} @@ -126,12 +141,13 @@ func (r *resolver) resolveAlbum(ctx context.Context, albumID string) (resolution return resolution{}, err } - var chain chainState + chain := chainState{trace: traceFrom(ctx)} for pattern := range strings.SplitSeq(strings.ToLower(conf.Server.CoverArtPriority), ",") { pattern = strings.TrimSpace(pattern) switch { case pattern == "embedded": - if res, ok := chain.try(resolveEmbedded(ctx, lib, r.ffmpeg, al.EmbedArtPath)); ok { + res, ok := resolveEmbedded(ctx, lib, r.ffmpeg, al.EmbedArtPath) + if res, ok = chain.try(pattern, res, ok); ok { return res, nil } case pattern == "external": @@ -141,7 +157,8 @@ func (r *resolver) resolveAlbum(ctx context.Context, albumID string) (resolution chain.extErr = true } case len(imgFiles) > 0: - if res, ok := chain.try(resolveFolderFile(ctx, lib, imgFiles, pattern)); ok { + res, ok := resolveFolderFile(ctx, lib, imgFiles, pattern) + if res, ok = chain.try(pattern, res, ok); ok { return res, nil } } @@ -155,9 +172,10 @@ func (r *resolver) resolveArtist(ctx context.Context, artistID string) (resoluti if err != nil { return resolution{}, err } - upload, ok := resolveLocalFile(ar.UploadedImagePath(), "upload") - if ok { - return upload, nil + chain := chainState{trace: traceFrom(ctx)} + upload, uploadOK := resolveLocalFile(ar.UploadedImagePath(), "upload") + if res, ok := chain.try("upload", upload, uploadOK); ok { + return res, nil } if upload.localError { // The upload outranks every other source; falling through would persist a lower-priority @@ -191,7 +209,6 @@ func (r *resolver) resolveArtist(ctx context.Context, artistID string) (resoluti } } - var chain chainState for pattern := range strings.SplitSeq(strings.ToLower(conf.Server.ArtistArtPriority), ",") { pattern = strings.TrimSpace(pattern) switch { @@ -202,21 +219,24 @@ func (r *resolver) resolveArtist(ctx context.Context, artistID string) (resoluti chain.extErr = true } case pattern == "image-folder": - if res, ok := chain.try(resolveArtistImageFolder(ar)); ok { + res, ok := resolveArtistImageFolder(ar) + if res, ok = chain.try(pattern, res, ok); ok { return res, nil } case strings.HasPrefix(pattern, "album/"): if lib.FS == nil { continue } - if res, ok := chain.try(resolveFolderFile(ctx, lib, imgFiles, strings.TrimPrefix(pattern, "album/"))); ok { + res, ok := resolveFolderFile(ctx, lib, imgFiles, strings.TrimPrefix(pattern, "album/")) + if res, ok = chain.try(pattern, res, ok); ok { return res, nil } default: if lib.FS == nil || artistFolder == "" { continue } - if res, ok := chain.try(resolveArtistFolderPattern(ctx, lib, artistFolder, pattern)); ok { + res, ok := resolveArtistFolderPattern(ctx, lib, artistFolder, pattern) + if res, ok = chain.try(pattern, res, ok); ok { return res, nil } } diff --git a/core/artwork/trace.go b/core/artwork/trace.go index 28c0e61ab..b46e09bfe 100644 --- a/core/artwork/trace.go +++ b/core/artwork/trace.go @@ -31,6 +31,9 @@ type chainTrace struct { } func (t *chainTrace) add(step traceStep) { + if t == nil { + return + } t.mu.Lock() defer t.mu.Unlock() t.steps = append(t.steps, step) diff --git a/core/artwork/trace_test.go b/core/artwork/trace_test.go index e53fa3490..d2dc3796e 100644 --- a/core/artwork/trace_test.go +++ b/core/artwork/trace_test.go @@ -2,7 +2,16 @@ package artwork import ( "context" + "os" + "path/filepath" + "runtime" + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/core/agents" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -23,6 +32,15 @@ var _ = Describe("chainTrace", func() { {Candidate: "cover.*", Outcome: outcomeMiss}, {Candidate: "embedded", Outcome: outcomeHit, Detail: "/music/a.flac"}, })) + + s := t.Steps() + s[0].Candidate = "mutated" + Expect(t.Steps()[0].Candidate).To(Equal("cover.*")) + }) + + It("does not panic when the trace is nil", func() { + var t *chainTrace + Expect(func() { t.add(traceStep{Candidate: "cover.*", Outcome: outcomeMiss}) }).ToNot(Panic()) }) It("is safe to use concurrently", func() { @@ -41,3 +59,114 @@ var _ = Describe("chainTrace", func() { Expect(t.Steps()).To(HaveLen(10)) }) }) + +var _ = Describe("chainState tracing", func() { + It("records a miss when the candidate was absent", func() { + t := &chainTrace{} + c := chainState{trace: t} + + _, ok := c.try("cover.*", resolution{}, false) + + Expect(ok).To(BeFalse()) + Expect(t.Steps()).To(Equal([]traceStep{{Candidate: "cover.*", Outcome: outcomeMiss}})) + }) + + It("records unreadable when the candidate existed but could not be read", func() { + t := &chainTrace{} + c := chainState{trace: t} + + _, ok := c.try("cover.*", resolution{localError: true}, false) + + Expect(ok).To(BeFalse()) + Expect(t.Steps()).To(HaveLen(1)) + Expect(t.Steps()[0].Outcome).To(Equal(outcomeUnreadable), + "a candidate that existed and failed to decode must be distinguishable from one that was absent") + }) + + It("records a hit with the backing path", func() { + t := &chainTrace{} + c := chainState{trace: t} + + res, ok := c.try("embedded", resolution{reader: nil, source: "embedded", sourcePath: "/music/a.flac"}, true) + + Expect(ok).To(BeTrue()) + Expect(res.source).To(Equal("embedded")) + Expect(t.Steps()).To(Equal([]traceStep{ + {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("resolveArtist tracing", func() { + var ( + ctx context.Context + ds *tests.MockDataStore + artistRepo *tests.MockArtistRepo + ffm *tests.MockFFmpeg + ag *agents.Agents + t *chainTrace + ) + + uploadPath := func(file string) string { + path := model.UploadedImagePath(consts.EntityArtist, file) + Expect(os.MkdirAll(filepath.Dir(path), 0o755)).To(Succeed()) + Expect(os.WriteFile(path, []byte("uploaded artist image"), 0o600)).To(Succeed()) + return path + } + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.DataFolder = conf.NewDir(GinkgoT().TempDir()) + conf.Server.ArtistArtPriority = "" + artistRepo = tests.CreateMockArtistRepo() + ds = &tests.MockDataStore{ + MockedArtist: artistRepo, + MockedFolder: &fakeFolderRepo{}, + MockedLibrary: &tests.MockLibraryRepo{}, + } + ffm = tests.NewMockFFmpeg("") + ag = agents.GetAgents(&tests.MockDataStore{}, nil) + t = &chainTrace{} + ctx = withTrace(context.Background(), t) + }) + + It("records the upload short-circuit as a hit", func() { + path := uploadPath("ar1_test.jpg") + artistRepo.SetData(model.Artists{{ID: "ar1", Name: "Artist", UploadedImage: "ar1_test.jpg"}}) + + res, err := newResolver(ds, ag, ffm, nil).resolve(ctx, model.ArtworkQueueItem{ItemKind: "ar", ItemID: "ar1"}) + Expect(err).ToNot(HaveOccurred()) + Expect(res.reader).ToNot(BeNil()) + defer res.reader.Close() + Expect(t.Steps()).To(Equal([]traceStep{{Candidate: "upload", Outcome: outcomeHit, Detail: path}})) + }) + + It("records an upload miss before walking the chain", func() { + artistRepo.SetData(model.Artists{{ID: "ar2", Name: "Artist"}}) + + _, err := newResolver(ds, ag, ffm, nil).resolve(ctx, model.ArtworkQueueItem{ItemKind: "ar", ItemID: "ar2"}) + Expect(err).ToNot(HaveOccurred()) + Expect(t.Steps()).To(Equal([]traceStep{{Candidate: "upload", Outcome: outcomeMiss}})) + }) + + It("records an upload that exists but cannot be read as unreadable", func() { + if runtime.GOOS == "windows" { + Skip("chmod does not restrict read access on Windows") + } + path := uploadPath("ar3_test.jpg") + Expect(os.Chmod(path, 0o000)).To(Succeed()) + DeferCleanup(func() { _ = os.Chmod(path, 0o600) }) + artistRepo.SetData(model.Artists{{ID: "ar3", Name: "Artist", UploadedImage: "ar3_test.jpg"}}) + + _, err := newResolver(ds, ag, ffm, nil).resolve(ctx, model.ArtworkQueueItem{ItemKind: "ar", ItemID: "ar3"}) + Expect(err).ToNot(HaveOccurred()) + Expect(t.Steps()).To(HaveLen(1)) + Expect(t.Steps()[0].Outcome).To(Equal(outcomeUnreadable), + "an upload that exists and will not open must not look like an absent upload") + }) +})