refactor(artwork): one disc-artwork walk for serving and explain

resolveDisc duplicated the loop selectImageReader already ran: try each source in
priority order, take the first that yields an image. The serving path and the CLI
diverged on two details as a result — only selectImageReader checked ctx between
candidates and logged each attempt.

Both now call discArtworkReader.selectImage, which takes the chainState the CLI already
uses for the other kinds. The serving path passes an untraced one, whose nil trace makes
recording a no-op. selectImageReader had no other caller and is gone.

The disc tests move from fromDiscArtPriority to discCandidates, so they assert the skip
reason for an entry that maps to no source rather than that it silently vanished, and
cancellation mid-walk is now covered.

core/artwork/artwork.go

@@ -318,16 +318,21 @@ func (s *service) serveDisc(ctx context.Context, artID model.ArtworkID, size int
 		return nil, err
 	}
 	// Single-disc albums run the chain too: a disc can carry art distinct from the album cover.
-	selectImage := func() (io.ReadCloser, string, error) {
-		funcs := dr.fromDiscArtPriority(ctx, s.ffmpeg, conf.Server.DiscArtPriority)
-		return selectImageReader(ctx, artID, funcs...)
+	selectImage := func() (io.ReadCloser, error) {
+		res, err := dr.selectImage(ctx, s.ffmpeg, conf.Server.DiscArtPriority, &chainState{})
+		if err != nil {
+			return nil, err
+		}
+		if res.reader == nil {
+			return nil, fmt.Errorf("could not get `%s` cover art for %s: %w", artID.Kind, artID, ErrUnavailable)
+		}
+		return res.reader, nil
 	}
 	albumArtID := model.ArtworkID{Kind: model.KindAlbumArtwork, ID: dr.album.ID}
 	// Disc art has no state row, hence no content hash: keying on id, album mtime and
 	// DiscArtPriority lets a warm cache answer without running the chain or touching the disk.
 	key := fmt.Sprintf("%s|%d|%s", artID.ID, dr.cacheTime().UnixNano(), conf.Server.DiscArtPriority)
-	img, err := s.serveSource(ctx, key, "", dr.cacheTime(), size, square,
-		func() (io.ReadCloser, error) { rc, _, err := selectImage(); return rc, err })
+	img, err := s.serveSource(ctx, key, "", dr.cacheTime(), size, square, selectImage)
 	if err != nil {
 		if errors.Is(err, context.Canceled) {
 			return nil, err

core/artwork/disc.go

@@ -154,14 +154,48 @@ func (d *discArtworkReader) discCandidates(ctx context.Context, ffmpeg ffmpeg.FF
 	return cc
 }
 
-// fromDiscArtPriority flattens the candidates for the serving path, which needs the sources in
-// order and has no walk to report.
-func (d *discArtworkReader) fromDiscArtPriority(ctx context.Context, ffmpeg ffmpeg.FFmpeg, priority string) []sourceFunc {
-	var ff []sourceFunc
+// selectImage walks the DiscArtPriority entries and returns the first that yields an image.
+// chain records the walk; the serving path passes an untraced one and pays nothing for it.
+func (d *discArtworkReader) selectImage(ctx context.Context, ffmpeg ffmpeg.FFmpeg, priority string,
+	chain *chainState) (resolution, error) {
 	for _, c := range d.discCandidates(ctx, ffmpeg, priority) {
-		ff = append(ff, c.sources...)
+		if err := ctx.Err(); err != nil {
+			return resolution{}, err
+		}
+		if c.skip != "" {
+			chain.record(c.pattern, OutcomeSkipped, c.skip)
+			continue
+		}
+		res, ok := d.openCandidate(ctx, c)
+		if res, ok = chain.try(c.pattern, res, ok); ok {
+			return res, nil
+		}
 	}
-	return ff
+	return chain.exhausted(), nil
+}
+
+func (d *discArtworkReader) openCandidate(ctx context.Context, c discCandidate) (resolution, bool) {
+	source := "folder"
+	if c.pattern == "embedded" {
+		source = "embedded"
+	}
+	for _, sf := range c.sources {
+		start := time.Now()
+		rd, path, err := sf()
+		if rd == nil {
+			log.Trace(ctx, "Artwork: Failed trying to extract disc artwork", "albumID", d.album.ID,
+				"disc", d.discNumber, "pattern", c.pattern, "source", sf, "elapsed", time.Since(start), err)
+			continue
+		}
+		// The disc sources disagree on this: only ffmpeg hands back an absolute path.
+		if !filepath.IsAbs(path) {
+			path = d.lib.Abs(path)
+		}
+		log.Debug(ctx, "Artwork: Found disc artwork", "albumID", d.album.ID, "disc", d.discNumber,
+			"pattern", c.pattern, "path", path, "elapsed", time.Since(start))
+		return resolution{reader: rd, source: source, sourcePath: path}, true
+	}
+	return resolution{}, false
 }
 
 // fromDiscSubtitle returns a sourceFunc that matches image files whose stem

core/artwork/disc_test.go

@@ -179,16 +179,16 @@ var _ = Describe("Disc Artwork Reader", func() {
 				lib:            libraryView{FS: osDirFS{os.DirFS(tmpDir)}, absRoot: tmpDir},
 			}
 
-			ff := reader.fromDiscArtPriority(ctx, nil, "disc*.*, cover.*")
-			Expect(ff).To(HaveLen(2))
-			r, path, err := ff[0]()
+			cc := reader.discCandidates(ctx, nil, "disc*.*, cover.*")
+			Expect(cc).To(HaveLen(2))
+			r, path, err := cc[0].sources[0]()
 			Expect(err).ToNot(HaveOccurred())
 			Expect(path).To(Equal(f2))
 			r.Close()
 
-			ff = reader.fromDiscArtPriority(ctx, nil, "cover.*, disc*.*")
-			Expect(ff).To(HaveLen(2))
-			r, path, err = ff[0]()
+			cc = reader.discCandidates(ctx, nil, "cover.*, disc*.*")
+			Expect(cc).To(HaveLen(2))
+			r, path, err = cc[0].sources[0]()
 			Expect(err).ToNot(HaveOccurred())
 			Expect(path).To(Equal(f1))
 			r.Close()
@@ -428,64 +428,89 @@ var _ = Describe("Disc Artwork Reader", func() {
 	})
 
 	Describe("discArtworkReader", func() {
-		Describe("fromDiscArtPriority", func() {
-			var (
-				reader *discArtworkReader
-				tmpDir string
-			)
+		var (
+			reader *discArtworkReader
+			tmpDir string
+		)
 
-			BeforeEach(func() {
-				tmpDir = GinkgoT().TempDir()
-				reader = &discArtworkReader{
-					discNumber:     2,
-					isMultiFolder:  true,
-					discFoldersRel: map[string]bool{"music/album/cd2": true},
-					imgFiles: []string{
-						"music/album/cd1/disc.jpg",
-						"music/album/cd2/disc.jpg",
-						"music/album/cd2/disc2.jpg",
-					},
-					firstTrackRel: "music/album/cd2/track1.flac",
-					lib:           libraryView{FS: osDirFS{os.DirFS(tmpDir)}, absRoot: tmpDir},
-				}
+		BeforeEach(func() {
+			tmpDir = GinkgoT().TempDir()
+			reader = &discArtworkReader{
+				discNumber:     2,
+				isMultiFolder:  true,
+				discFoldersRel: map[string]bool{"music/album/cd2": true},
+				imgFiles: []string{
+					"music/album/cd1/disc.jpg",
+					"music/album/cd2/disc.jpg",
+					"music/album/cd2/disc2.jpg",
+				},
+				firstTrackRel: "music/album/cd2/track1.flac",
+				lib:           libraryView{FS: osDirFS{os.DirFS(tmpDir)}, absRoot: tmpDir},
+			}
+		})
+
+		Describe("selectImage", func() {
+			It("abandons the walk when the context is cancelled", func() {
+				ctx, cancel := context.WithCancel(context.Background())
+				cancel()
+
+				res, err := reader.selectImage(ctx, nil, "disc*.*, cover.*", &chainState{})
+
+				Expect(err).To(MatchError(context.Canceled))
+				Expect(res.reader).To(BeNil())
 			})
+		})
 
+		Describe("discCandidates", func() {
 			It("returns source funcs for glob patterns", func() {
-				ff := reader.fromDiscArtPriority(context.Background(), nil, "disc*.*")
-				Expect(ff).To(HaveLen(1))
+				cc := reader.discCandidates(context.Background(), nil, "disc*.*")
+				Expect(cc).To(HaveLen(1))
+				Expect(cc[0].sources).To(HaveLen(1))
 			})
 
 			It("returns source funcs for embedded pattern", func() {
-				ff := reader.fromDiscArtPriority(context.Background(), nil, "embedded")
-				Expect(ff).To(HaveLen(2)) // fromTag + fromFFmpegTag
+				cc := reader.discCandidates(context.Background(), nil, "embedded")
+				Expect(cc).To(HaveLen(1))
+				Expect(cc[0].sources).To(HaveLen(2)) // fromTag + fromFFmpegTag
 			})
 
 			It("handles multiple comma-separated patterns", func() {
-				ff := reader.fromDiscArtPriority(context.Background(), nil, "disc*.*, cd*.*, embedded")
-				Expect(ff).To(HaveLen(4)) // disc*.* + cd*.* + fromTag + fromFFmpegTag
+				cc := reader.discCandidates(context.Background(), nil, "disc*.*, cd*.*, embedded")
+				Expect(cc).To(HaveLen(3))
+				Expect(cc[0].sources).To(HaveLen(1))
+				Expect(cc[1].sources).To(HaveLen(1))
+				Expect(cc[2].sources).To(HaveLen(2))
 			})
 
-			It("ignores 'external' pattern silently", func() {
-				ff := reader.fromDiscArtPriority(context.Background(), nil, "external")
-				Expect(ff).To(HaveLen(0))
+			It("skips an empty entry rather than building a glob that matches nothing", func() {
+				cc := reader.discCandidates(context.Background(), nil, "disc*.*,")
+				Expect(cc).To(HaveLen(1))
 			})
 
-			It("returns no source funcs when imgFiles is empty and pattern is not embedded", func() {
-				reader.imgFiles = nil
-				ff := reader.fromDiscArtPriority(context.Background(), nil, "disc*.*")
-				Expect(ff).To(HaveLen(0))
-			})
+			// The skip reasons below are what `artwork explain` prints, so an entry that maps to no
+			// source must say why instead of vanishing from the walk.
+			DescribeTable("keeps an entry that maps to no source, with its reason",
+				func(setup func(), priority, reason string) {
+					setup()
+					cc := reader.discCandidates(context.Background(), nil, priority)
+					Expect(cc).To(HaveLen(1))
+					Expect(cc[0].sources).To(BeEmpty())
+					Expect(cc[0].skip).To(Equal(reason))
+				},
+				Entry("external is unsupported", func() {}, "external",
+					"external sources are not supported for disc artwork"),
+				Entry("no images in the album folder", func() { reader.imgFiles = nil }, "disc*.*",
+					"no images in album folder"),
+				Entry("the disc has no subtitle",
+					func() { reader.album = model.Album{Discs: model.Discs{2: ""}} }, "discsubtitle",
+					"disc has no subtitle"),
+			)
 
 			It("returns source func for discsubtitle pattern", func() {
 				reader.album = model.Album{Discs: model.Discs{2: "Bonus Tracks"}}
-				ff := reader.fromDiscArtPriority(context.Background(), nil, "discsubtitle")
-				Expect(ff).To(HaveLen(1))
-			})
-
-			It("returns no source func for discsubtitle when disc has no subtitle", func() {
-				reader.album = model.Album{Discs: model.Discs{2: ""}}
-				ff := reader.fromDiscArtPriority(context.Background(), nil, "discsubtitle")
-				Expect(ff).To(HaveLen(0))
+				cc := reader.discCandidates(context.Background(), nil, "discsubtitle")
+				Expect(cc).To(HaveLen(1))
+				Expect(cc[0].sources).To(HaveLen(1))
 			})
 		})
 	})

core/artwork/resolve.go

@@ -9,7 +9,6 @@ import (
 	"io/fs"
 	"net/url"
 	"os"
-	"path/filepath"
 	"strings"
 
 	"github.com/Masterminds/squirrel"
@@ -463,34 +462,7 @@ func (r *resolver) resolveDisc(ctx context.Context, id string) (resolution, erro
 		return resolution{}, err
 	}
 	chain := chainState{trace: traceFrom(ctx)}
-	for _, c := range dr.discCandidates(ctx, r.ffmpeg, conf.Server.DiscArtPriority) {
-		if c.skip != "" {
-			chain.record(c.pattern, OutcomeSkipped, c.skip)
-			continue
-		}
-		res, ok := openDiscCandidate(c, dr.lib)
-		if res, ok = chain.try(c.pattern, res, ok); ok {
-			return res, nil
-		}
-	}
-	return chain.exhausted(), nil
-}
-
-func openDiscCandidate(c discCandidate, lib libraryView) (resolution, bool) {
-	source := "folder"
-	if c.pattern == "embedded" {
-		source = "embedded"
-	}
-	for _, sf := range c.sources {
-		if rd, path, _ := sf(); rd != nil {
-			// The disc sources disagree on this: only ffmpeg hands back an absolute path.
-			if !filepath.IsAbs(path) {
-				path = lib.Abs(path)
-			}
-			return resolution{reader: rd, source: source, sourcePath: path}, true
-		}
-	}
-	return resolution{}, false
+	return dr.selectImage(ctx, r.ffmpeg, conf.Server.DiscArtPriority, &chain)
 }
 
 // resolveExternalStep runs a single external sourceFunc through the named gate. extErr excludes

core/artwork/sources.go

@@ -27,23 +27,6 @@ import (
 // to open it is not evidence the entity has no artwork, so callers must not settle on absent.
 var errSourceUnreadable = errors.New("artwork source unreadable")
 
-func selectImageReader(ctx context.Context, artID model.ArtworkID, extractFuncs ...sourceFunc) (io.ReadCloser, string, error) {
-	for _, f := range extractFuncs {
-		if ctx.Err() != nil {
-			return nil, "", ctx.Err()
-		}
-		start := time.Now()
-		r, path, err := f()
-		if r != nil {
-			msg := fmt.Sprintf("Artwork: Found %s artwork", artID.Kind)
-			log.Debug(ctx, msg, "artID", artID, "path", path, "source", f, "elapsed", time.Since(start))
-			return r, path, nil
-		}
-		log.Trace(ctx, "Artwork: Failed trying to extract artwork", "artID", artID, "source", f, "elapsed", time.Since(start), err)
-	}
-	return nil, "", fmt.Errorf("could not get `%s` cover art for %s: %w", artID.Kind, artID, ErrUnavailable)
-}
-
 type sourceFunc func() (r io.ReadCloser, path string, err error)
 
 func (f sourceFunc) String() string {