diff --git a/core/artwork/processor.go b/core/artwork/processor.go index 6de91d145..ab3bf6e5d 100644 --- a/core/artwork/processor.go +++ b/core/artwork/processor.go @@ -66,8 +66,9 @@ func processItem(ctx context.Context, deps *workerDeps, item model.ArtworkQueueI return outcomeFailed } if res.reader == nil { - if res.extError { - // An external source errored/timed out: never settle on absent, keep serving old state. + if res.extError || res.localError { + // A source errored/timed out rather than answering "no image": never settle on + // absent, keep serving old state. return outcomeFailed } return writeAbsent(ctx, repo, item) diff --git a/core/artwork/processor_test.go b/core/artwork/processor_test.go index de765ae9d..33675fda8 100644 --- a/core/artwork/processor_test.go +++ b/core/artwork/processor_test.go @@ -138,6 +138,25 @@ var _ = Describe("processItem", func() { Expect(ia.AttemptedAt).To(BeTemporally("~", time.Now(), time.Second)) }) + It("failed-on-unreadable-local: a listed cover that will not open never records absent", func() { + conf.Server.CoverArtPriority = "cover.jpg" + // A healthy library whose folder listing names a cover the FS will not hand over — + // what a stale mount looks like from here. + libRoot := GinkgoT().TempDir() + Expect(os.MkdirAll(filepath.Join(libRoot, "an-album"), 0o755)).To(Succeed()) + libRepo.SetData(model.Libraries{{ID: 0, Path: testFileLibPath(libRoot)}}) + folderRepo.result = []model.Folder{{Path: "an-album", ImageFiles: []string{"cover.jpg"}}} + ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{ + {ID: "al-io", Name: "Album", FolderIDs: []string{"f1"}}, + }) + + out := processItem(ctx, deps, model.ArtworkQueueItem{ItemKind: "al", ItemID: "al-io"}) + Expect(out).To(Equal(outcomeFailed)) + + _, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "al-io", model.ImageTypePrimary) + Expect(err).To(MatchError(model.ErrNotFound), "an I/O fault must not be recorded as absent") + }) + It("failed-on-extError: leaves the item's state untouched", func() { conf.Server.CoverArtPriority = "external" ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{ diff --git a/core/artwork/resolve.go b/core/artwork/resolve.go index 919a56978..228d58585 100644 --- a/core/artwork/resolve.go +++ b/core/artwork/resolve.go @@ -30,6 +30,9 @@ type resolution struct { // external source errored/timed out. With no reader: forces failed (never absent). // On a hit: a higher-priority external step failed—serve this, but retry later. extError bool + // a local source that should have been readable wasn't (stale mount, permissions). + // With no reader: forces failed, so a transient I/O fault never records absent. + localError bool } // resolveItem walks the kind's priority chain and returns the first hit. @@ -80,15 +83,17 @@ func resolveAlbum(ctx context.Context, ds model.DataStore, ag *agents.Agents, ff return resolution{}, err } - var extErr bool + var extErr, localErr bool for pattern := range strings.SplitSeq(strings.ToLower(conf.Server.CoverArtPriority), ",") { pattern = strings.TrimSpace(pattern) switch { case pattern == "embedded": - if res, ok := resolveEmbedded(ctx, lib, ffm, al.EmbedArtPath); ok { + res, ok := resolveEmbedded(ctx, lib, ffm, al.EmbedArtPath) + if ok { res.extError = extErr return res, nil } + localErr = localErr || res.localError case pattern == "external": if localOnly { continue @@ -99,13 +104,15 @@ func resolveAlbum(ctx context.Context, ds model.DataStore, ag *agents.Agents, ff extErr = true } case len(imgFiles) > 0: - if res, ok := resolveFolderFile(ctx, lib, imgFiles, pattern); ok { + res, ok := resolveFolderFile(ctx, lib, imgFiles, pattern) + if ok { res.extError = extErr return res, nil } + localErr = localErr || res.localError } } - return resolution{extError: extErr}, nil + return resolution{extError: extErr, localError: localErr}, nil } // resolveArtist ports the upload/folder/external selection from @@ -145,7 +152,7 @@ func resolveArtist(ctx context.Context, ds model.DataStore, ag *agents.Agents, f } } - var extErr bool + var extErr, localErr bool for pattern := range strings.SplitSeq(strings.ToLower(conf.Server.ArtistArtPriority), ",") { pattern = strings.TrimSpace(pattern) switch { @@ -167,10 +174,12 @@ func resolveArtist(ctx context.Context, ds model.DataStore, ag *agents.Agents, f if lib.FS == nil { continue } - if res, ok := resolveFolderFile(ctx, lib, imgFiles, strings.TrimPrefix(pattern, "album/")); ok { + res, ok := resolveFolderFile(ctx, lib, imgFiles, strings.TrimPrefix(pattern, "album/")) + if ok { res.extError = extErr return res, nil } + localErr = localErr || res.localError default: if lib.FS == nil || artistFolder == "" { continue @@ -181,7 +190,7 @@ func resolveArtist(ctx context.Context, ds model.DataStore, ag *agents.Agents, f } } } - return resolution{extError: extErr}, nil + return resolution{extError: extErr, localError: localErr}, nil } // resolvePlaylist ports reader_playlist.go's chain: uploaded image, sidecar, @@ -337,18 +346,21 @@ func resolveEmbedded(ctx context.Context, lib libraryView, ffm ffmpeg.FFmpeg, em return resolution{}, false } abs := lib.Abs(embedRel) + var unreadable bool for _, sf := range []sourceFunc{fromTag(ctx, lib.FS, embedRel), fromFFmpegTag(ctx, ffm, abs)} { - if r, _, _ := sf(); r != nil { + r, _, err := sf() + if r != nil { return resolution{reader: r, source: "embedded", sourcePath: abs, refMtime: mtimeViaFS(lib.FS, embedRel)}, true } + unreadable = unreadable || errors.Is(err, errSourceUnreadable) } - return resolution{}, false + return resolution{localError: unreadable}, false } func resolveFolderFile(ctx context.Context, lib libraryView, imgFiles []string, pattern string) (resolution, bool) { - r, path, _ := fromExternalFile(ctx, lib.FS, imgFiles, pattern)() + r, path, err := fromExternalFile(ctx, lib.FS, imgFiles, pattern)() if r == nil { - return resolution{}, false + return resolution{localError: errors.Is(err, errSourceUnreadable)}, false } return resolution{reader: r, source: "folder", sourcePath: lib.Abs(path), refMtime: mtimeViaFS(lib.FS, path)}, true } diff --git a/core/artwork/sources.go b/core/artwork/sources.go index b31c69f79..4706db712 100644 --- a/core/artwork/sources.go +++ b/core/artwork/sources.go @@ -3,6 +3,7 @@ package artwork import ( "bytes" "context" + "errors" "fmt" "io" "io/fs" @@ -22,6 +23,10 @@ import ( "go.senan.xyz/taglib" ) +// errSourceUnreadable marks a candidate the resolver knows exists but could not read. Failing +// 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 { @@ -53,6 +58,7 @@ func (f sourceFunc) String() string { func fromExternalFile(ctx context.Context, libFS fs.FS, files []string, pattern string) sourceFunc { return func() (io.ReadCloser, string, error) { + var openErr error for _, file := range files { _, name := filepath.Split(file) match, err := filepath.Match(pattern, strings.ToLower(name)) @@ -66,10 +72,14 @@ func fromExternalFile(ctx context.Context, libFS fs.FS, files []string, pattern f, err := libFS.Open(file) if err != nil { log.Warn(ctx, "Could not open cover art file", "file", file, err) + openErr = fmt.Errorf("%w: %s: %w", errSourceUnreadable, file, err) continue } return f, file, nil } + if openErr != nil { + return nil, "", openErr + } return nil, "", fmt.Errorf("pattern '%s' not matched by files %v", pattern, files) } } @@ -88,7 +98,7 @@ func fromTag(ctx context.Context, libFS fs.FS, relPath string) sourceFunc { } f, err := libFS.Open(relPath) if err != nil { - return nil, "", err + return nil, "", fmt.Errorf("%w: %s: %w", errSourceUnreadable, relPath, err) } rs, ok := f.(io.ReadSeeker) if !ok { @@ -101,7 +111,7 @@ func fromTag(ctx context.Context, libFS fs.FS, relPath string) sourceFunc { ) if err != nil { f.Close() - return nil, "", err + return nil, "", fmt.Errorf("%w: %s: %w", errSourceUnreadable, relPath, err) } // Close in LIFO order: tf first (it holds rs internally), then f. defer f.Close()