From 6cd12e607083fbcbfcc2181267a8d6cc521da8c6 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 25 Jul 2026 13:36:23 -0400 Subject: [PATCH] fix(artwork): treat an unreadable upload as a failure, not a miss resolveLocalFile swallowed every os.Open error, so uploads, playlist sidecars, a local M3U image and the artist image folder still had the bug that was fixed for folder and embedded sources: a permission or transient I/O error on a file that exists read as "no image here". The worker then settled the item absent and dropped its queue row. Uploads outrank every other source, so an unreadable one now stops the chain rather than letting a lower-priority image be persisted in its place. A genuinely missing file stays a clean miss. Reported by Codex on #5847. --- core/artwork/processor_test.go | 23 +++++++++++++++++ core/artwork/resolve.go | 47 ++++++++++++++++++++++++---------- 2 files changed, 56 insertions(+), 14 deletions(-) diff --git a/core/artwork/processor_test.go b/core/artwork/processor_test.go index b7d9ece19..45b288072 100644 --- a/core/artwork/processor_test.go +++ b/core/artwork/processor_test.go @@ -13,6 +13,7 @@ import ( "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" @@ -157,6 +158,28 @@ var _ = Describe("processItem", func() { Expect(err).To(MatchError(model.ErrNotFound), "an I/O fault must not be recorded as absent") }) + // An upload outranks every other source, so an unreadable one must neither settle absent + // nor let a lower-priority image take its place. + It("failed-on-unreadable-upload: an upload that will not open never records absent", func() { + radioRepo := tests.CreateMockedRadioRepo() + radioRepo.Data = map[string]*model.Radio{} + ds.MockedRadio = radioRepo + dir := GinkgoT().TempDir() + conf.Server.DataFolder = conf.NewDir(dir) + upload := model.UploadedImagePath(consts.EntityRadio, "ra-io.jpg") + Expect(os.MkdirAll(filepath.Dir(upload), 0o755)).To(Succeed()) + Expect(os.WriteFile(upload, []byte("x"), 0o600)).To(Succeed()) + Expect(os.Chmod(upload, 0o000)).To(Succeed()) // present, but unreadable + DeferCleanup(func() { _ = os.Chmod(upload, 0o600) }) + radioRepo.Data["ra-io"] = &model.Radio{ID: "ra-io", Name: "Station", UploadedImage: "ra-io.jpg"} + + out, _ := processItem(ctx, deps, model.ArtworkQueueItem{ItemKind: "ra", ItemID: "ra-io"}) + Expect(out).To(Equal(outcomeFailed)) + + _, err := artRepo.GetItemArtwork(model.KindRadioArtwork, "ra-io", model.ImageTypePrimary) + Expect(err).To(MatchError(model.ErrNotFound), "an unreadable upload 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 228d58585..0e15c1dad 100644 --- a/core/artwork/resolve.go +++ b/core/artwork/resolve.go @@ -122,8 +122,14 @@ func resolveArtist(ctx context.Context, ds model.DataStore, ag *agents.Agents, f if err != nil { return resolution{}, err } - if res, ok := resolveLocalFile(ar.UploadedImagePath(), "upload"); ok { - return res, nil + upload, ok := resolveLocalFile(ar.UploadedImagePath(), "upload") + if ok { + return upload, nil + } + if upload.localError { + // The upload outranks every other source; falling through would persist a lower-priority + // image as if the upload were gone. + return upload, nil } // Only consider albums where the artist is the sole album artist, same as reader_artist.go. @@ -166,10 +172,12 @@ func resolveArtist(ctx context.Context, ds model.DataStore, ag *agents.Agents, f extErr = true } case pattern == "image-folder": - if res, ok := resolveArtistImageFolder(ar); ok { + res, ok := resolveArtistImageFolder(ar) + if ok { res.extError = extErr return res, nil } + localErr = localErr || res.localError case strings.HasPrefix(pattern, "album/"): if lib.FS == nil { continue @@ -201,18 +209,29 @@ func resolvePlaylist(ctx context.Context, ds model.DataStore, ag *agents.Agents, return resolution{}, err } - var extErr bool - if res, ok := resolveLocalFile(pl.UploadedImagePath(), "upload"); ok { - return res, nil - } - if res, ok := resolveLocalFile(findPlaylistSidecarPath(ctx, pl.Path), "folder"); ok { - return res, nil + var extErr, localErr bool + for _, src := range []struct{ path, source string }{ + {pl.UploadedImagePath(), "upload"}, + {findPlaylistSidecarPath(ctx, pl.Path), "folder"}, + } { + res, ok := resolveLocalFile(src.path, src.source) + if ok { + return res, nil + } + if res.localError { + // These outrank the generated grid; falling through would replace them with it. + return res, nil + } } // A local ExternalImageURL is a file-backed reference: serve it in place (staleness-checked, // and available even on the request path). Only http(s) URLs need the gated remote fetch. localImg, remoteImg := classifyPlaylistImage(pl.ExternalImageURL) if localImg != "" { - if res, ok := resolveLocalFile(localImg, "folder"); ok { + res, ok := resolveLocalFile(localImg, "folder") + if ok { + return res, nil + } + if res.localError { return res, nil } } @@ -266,7 +285,7 @@ func resolvePlaylist(ctx context.Context, ds model.DataStore, ag *agents.Agents, if tileErr != nil { return resolution{}, fmt.Errorf("resolvePlaylist: sampled album art failed: %w", tileErr) } - return resolution{extError: extErr}, nil + return resolution{extError: extErr, localError: localErr}, nil } // Grow to 4 tiles by repeating what we have, mirroring reader_playlist.go's loadTiles. switch len(tiles) { @@ -381,15 +400,15 @@ func resolveArtistFolderPattern(ctx context.Context, lib libraryView, artistFold return resolution{reader: r, source: "folder", sourcePath: path, refMtime: mtimeOf(path)}, true } -// resolveLocalFile opens an absolute path directly (uploads, image-folder). A -// missing or unreadable path is "no source", not an error. +// resolveLocalFile opens an absolute path directly (uploads, image-folder). A missing path is +// "no source"; any other open failure says nothing about whether the image exists. func resolveLocalFile(path, source string) (resolution, bool) { if path == "" { return resolution{}, false } f, err := os.Open(path) if err != nil { - return resolution{}, false + return resolution{localError: !errors.Is(err, fs.ErrNotExist)}, false } return resolution{reader: f, source: source, sourcePath: path, refMtime: mtimeOf(path)}, true }