feat(artwork): trace the local priority chain

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
 			}
 		}

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)

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")
+	})
+})