nik/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-08-01 07:21:17 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(artwork): never record absent after a local I/O failure

Browse Source 
Local sources swallowed their open errors, so a stale NFS/SMB mount was
indistinguishable from "this entity has no artwork": the chain returned
no reader, processItem took the absent branch, and the upsert replaced a
good content hash with the empty string. Clients then saw a placeholder
until the 1h request recheck or the 24h stale-absent sweep, and the
orphaned bytes became eligible for the next prune.

A candidate the resolver knows about — a file in the folder listing, a
track's own audio file — failing to open is not evidence of absence, so
it now forces a retry the same way an external agent error does.
This commit is contained in:
Deluan 2026-07-25 10:06:51 -04:00
parent d7385a9f68
commit 381dce0363
4 changed files with 57 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
core/artwork/processor.go
View File

@ -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)

19
core/artwork/processor_test.go
View File

@ -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{

34
core/artwork/resolve.go
View File

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

14
core/artwork/sources.go
View File

@ -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()