fix(artwork): fix stale cache and top-level album artwork for multi-disc albums (#5457)

* fix(artwork): include top-level album folders in parent cover art lookup The Path != "." guard added in #5451 was too aggressive — it excluded any folder with Path=".", which includes top-level album folders (not just the library root). Changed to ParentID != "" which correctly excludes only the actual library root folder. Fixes #5456 * fix: correct comment in test — album is under library root, not artist root * test: add ascii tree diagram to top-level album e2e test * test: replace internal bug references with issue link in e2e comments Signed-off-by: Deluan <deluan@navidrome.org> * test: add e2e test matching reporter's exact library layout (#5456) Adds a deeply nested test (Genre/Artist/Album/Disc) with 12 discs using the reporter's actual folder names to verify artwork resolution works for non-top-level album folders too. * fix(scanner): use a syntectic admin user when no admin user is found Signed-off-by: Deluan <deluan@navidrome.org> * fix(scanner): bump album UpdatedAt on Phase 3 refresh to invalidate artwork cache When Phase 3 corrects an album's FolderIDs (or any other field), bump UpdatedAt to the current time. This ensures the artwork cache key changes, invalidating any stale artwork that was resolved and cached during Phase 1 when the album had incomplete folder data. * fix(artwork): include ImportedAt in artwork cache key to invalidate stale cache Reverts the Phase 3 UpdatedAt bump (which would change album.UpdatedAt semantics) and instead includes album.ImportedAt in the artwork cache key computation. Since ImportedAt is bumped to time.Now() on every album Put, any Phase 3 correction naturally invalidates cached artwork that was resolved mid-scan with incomplete folder data. * fix(artwork): simplify lastUpdate logic using TimeNewest utility Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2026-07-09 17:18:45 +00:00 · 2026-05-04 17:26:39 -04:00 · 2026-05-04 17:26:39 -04:00 · f48416685f
commit f48416685f
parent a34a4abbc1
6 changed files with 193 additions and 30 deletions
--- a/core/artwork/e2e/album_test.go
+++ b/core/artwork/e2e/album_test.go
@ -37,15 +37,15 @@ var _ = Describe("Album artwork resolution", func() {
 		})
 	})

-	// Bug 2 variant: cover.* basenames tie across album-root and per-disc folders;
-	// compareImageFiles' lexicographic full-path tiebreaker ranks disc-subfolder
-	// files first.
+	// https://github.com/navidrome/navidrome/issues/5376
+	// cover.* basenames tie across album-root and per-disc folders;
+	// compareImageFiles must prefer shallower paths.
 	When("a multi-disc album has a cover.jpg at the album root and per-disc covers", func() {
 		// Artist/
 		// └── Album/
 		//     ├── CD1/
 		//     │   ├── 01 - Track.mp3
-		//     │   └── cover.jpg        ← currently wins (bug)
+		//     │   └── cover.jpg        ← should not win
 		//     ├── CD2/
 		//     │   ├── 01 - Track.mp3
 		//     │   └── cover.jpg
@ -68,15 +68,15 @@ var _ = Describe("Album artwork resolution", func() {
 		})
 	})

-	// Bug 2: folder.jpg basenames tie across album-root and per-disc folders;
-	// the lexicographic full-path tiebreaker in compareImageFiles ranks
-	// "Artist/Album/CD1/folder.jpg" ahead of "Artist/Album/folder.jpg".
+	// https://github.com/navidrome/navidrome/issues/5376
+	// folder.jpg basenames tie across album-root and per-disc folders;
+	// compareImageFiles must prefer shallower paths.
 	When("a multi-disc album has folder.jpg at the album root AND in each disc subfolder", func() {
 		// Artist/
 		// └── Album/
 		//     ├── CD1/
 		//     │   ├── 01 - Track.mp3
-		//     │   └── folder.jpg       ← currently wins (bug)
+		//     │   └── folder.jpg       ← should not win
 		//     ├── CD2/
 		//     │   ├── 01 - Track.mp3
 		//     │   └── folder.jpg
@ -97,15 +97,14 @@ var _ = Describe("Album artwork resolution", func() {
 		})
 	})

-	// Bug 1: commonParentFolder's `len(folders) < 2` guard skips the parent-folder
-	// lookup whenever an album lives entirely under a single subfolder, so an
-	// album-root cover is never considered.
+	// https://github.com/navidrome/navidrome/issues/5376
+	// Single-subfolder albums must still consider the parent folder's images.
 	When("an album lives entirely under a single disc subfolder with cover.jpg at the parent", func() {
 		// Artist/
 		// └── Album/
 		//     ├── disc1/
 		//     │   └── 01 - Track.mp3
-		//     └── cover.jpg            ← should win (parent-folder fallback, currently ignored — bug)
+		//     └── cover.jpg            ← should win (parent-folder fallback)
 		It("uses the parent-folder cover for single-disc-subfolder albums", func() {
 			conf.Server.CoverArtPriority = defaultCoverPriority
 			setLayout(fstest.MapFS{
@ -119,6 +118,32 @@ var _ = Describe("Album artwork resolution", func() {
 		})
 	})

+	// https://github.com/navidrome/navidrome/issues/5456
+	When("a top-level multi-disc album has cover.jpg at the album root and per-disc folder.jpg", func() {
+		// Album/                        (top-level folder, Path=".")
+		// ├── CD1/
+		// │   ├── 01 - Track.mp3
+		// │   └── folder.jpg
+		// ├── CD2/
+		// │   ├── 01 - Track.mp3
+		// │   └── folder.jpg
+		// └── cover.jpg                ← should win (album-root)
+		It("prefers the album-root cover.jpg", func() {
+			conf.Server.CoverArtPriority = defaultCoverPriority
+			setLayout(fstest.MapFS{
+				"Album/CD1/01 - Track.mp3": trackFile(1, "Track CD1"),
+				"Album/CD2/01 - Track.mp3": trackFile(1, "Track CD2"),
+				"Album/cover.jpg":          imageFile("album-root"),
+				"Album/CD1/folder.jpg":     imageFile("disc1"),
+				"Album/CD2/folder.jpg":     imageFile("disc2"),
+			})
+			scan()
+
+			al := firstAlbum()
+			Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root")))
+		})
+	})
+
 	When("CoverArtPriority puts embedded first and the album has both embedded and external art", func() {
 		// Artist/
 		// └── Album/
--- a/core/artwork/e2e/disc_test.go
+++ b/core/artwork/e2e/disc_test.go
@ -1,6 +1,7 @@
 package artworke2e_test

 import (
+	"fmt"
 	"testing/fstest"

 	"github.com/navidrome/navidrome/conf"
@ -255,6 +256,100 @@ var _ = Describe("Disc artwork resolution", func() {
 		})
 	})

+	// Reproduces https://github.com/navidrome/navidrome/issues/5456
+	// Deeply nested layout matching the reporter's actual structure.
+	When("a deeply nested multi-disc album has cover.jpg and per-disc folder.jpg", func() {
+		// Genre/Artist/Album/                 ← album root with cover.jpg
+		// ├── cover.jpg                       ← album-level cover
+		// ├── Disc 01 (Subtitle)/
+		// │   ├── 01 - Track.mp3
+		// │   └── folder.jpg                  ← disc 1 art
+		// ├── Disc 02 (Subtitle)/
+		// │   ├── 01 - Track.mp3
+		// │   └── folder.jpg
+		// └── ... (12 discs)
+		It("uses album-root cover.jpg for album art and per-disc folder.jpg for each disc", func() {
+			conf.Server.DiscArtPriority = defaultDiscPriority
+			conf.Server.CoverArtPriority = defaultCoverPriority
+			discNames := []string{
+				"Disc 01 (Birth of the Dead - The Studio Sides)",
+				"Disc 02 (Birth of the Dead - The Live Sides)",
+				"Disc 03 (The Grateful Dead)",
+				"Disc 04 (Anthem of the Sun)",
+				"Disc 05 (Aoxomoxoa)",
+				"Disc 06 (Live; Dead)",
+				"Disc 07 (Workingman's Dead)",
+				"Disc 08 (American Beauty)",
+				"Disc 09 (Grateful Dead)",
+				"Disc 10 (Europe '72)",
+				"Disc 11 (Europe '72)",
+				"Disc 12 (History of the Grateful Dead, Volume One (Bear's Choice))",
+			}
+			layout := fstest.MapFS{
+				"Pop; Rock/Grateful Dead/(2001) The Golden Road/cover.jpg": imageFile("album-root-cover"),
+			}
+			for i, name := range discNames {
+				discNum := i + 1
+				prefix := fmt.Sprintf("Pop; Rock/Grateful Dead/(2001) The Golden Road/%s/", name)
+				layout[prefix+"01 - Track.mp3"] = trackFile(1, fmt.Sprintf("T%d", discNum), map[string]any{"disc": fmt.Sprintf("%d", discNum)})
+				layout[prefix+"folder.jpg"] = imageFile(fmt.Sprintf("disc-%02d-folder", discNum))
+			}
+			setLayout(layout)
+			scan()
+
+			al := firstAlbum()
+
+			Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root-cover")))
+
+			for i := range discNames {
+				discNum := i + 1
+				discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, discNum), &al.UpdatedAt)
+				Expect(readArtwork(discID)).To(Equal(imageBytes(fmt.Sprintf("disc-%02d-folder", discNum))),
+					"disc %d should use its own folder.jpg", discNum)
+			}
+		})
+	})
+
+	// https://github.com/navidrome/navidrome/issues/5456
+	// Top-level album variant — album folder at library root (Path=".").
+	When("a top-level multi-disc album has cover.jpg and per-disc folder.jpg", func() {
+		// Album/                       (top-level, Path=".")
+		// ├── cover.jpg                ← album-level cover
+		// ├── Disc 01/
+		// │   ├── 01 - Track.mp3
+		// │   └── folder.jpg           ← disc 1 art
+		// ├── Disc 02/
+		// │   ├── 01 - Track.mp3
+		// │   └── folder.jpg
+		// └── Disc 03/
+		//     ├── 01 - Track.mp3
+		//     └── folder.jpg
+		It("uses album-root cover.jpg for album art and per-disc folder.jpg for each disc", func() {
+			conf.Server.DiscArtPriority = defaultDiscPriority
+			conf.Server.CoverArtPriority = defaultCoverPriority
+			layout := fstest.MapFS{
+				"Album/cover.jpg": imageFile("album-root-cover"),
+			}
+			for i := 1; i <= 3; i++ {
+				prefix := fmt.Sprintf("Album/Disc %02d/", i)
+				layout[prefix+"01 - Track.mp3"] = trackFile(1, fmt.Sprintf("T%d", i), map[string]any{"disc": fmt.Sprintf("%d", i)})
+				layout[prefix+"folder.jpg"] = imageFile(fmt.Sprintf("disc-%02d-folder", i))
+			}
+			setLayout(layout)
+			scan()
+
+			al := firstAlbum()
+
+			Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root-cover")))
+
+			for i := 1; i <= 3; i++ {
+				discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, i), &al.UpdatedAt)
+				Expect(readArtwork(discID)).To(Equal(imageBytes(fmt.Sprintf("disc-%02d-folder", i))),
+					"disc %d should use its own folder.jpg", i)
+			}
+		})
+	})
+
 	When("discsubtitle is set but no image filename matches the subtitle", func() {
 		// Artist/
 		// └── Album/
--- a/core/artwork/reader_album.go
+++ b/core/artwork/reader_album.go
@ -18,6 +18,7 @@ import (
 	"github.com/navidrome/navidrome/core/ffmpeg"
 	"github.com/navidrome/navidrome/log"
 	"github.com/navidrome/navidrome/model"
+	"github.com/navidrome/navidrome/utils"
 	"github.com/navidrome/navidrome/utils/natural"
 )

@ -53,10 +54,9 @@ func newAlbumArtworkReader(ctx context.Context, artwork *artwork, artID model.Ar
 		lib:       lib,
 	}
 	a.cacheKey.artID = artID
-	if a.updatedAt != nil && a.updatedAt.After(al.UpdatedAt) {
-		a.cacheKey.lastUpdate = *a.updatedAt
-	} else {
-		a.cacheKey.lastUpdate = al.UpdatedAt
+	a.cacheKey.lastUpdate = utils.TimeNewest(al.UpdatedAt, al.ImportedAt)
+	if imagesUpdateAt != nil {
+		a.cacheKey.lastUpdate = utils.TimeNewest(a.cacheKey.lastUpdate, *imagesUpdateAt)
 	}
 	return a, nil
 }
@ -131,7 +131,7 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo
 			} else if err != nil {
 				return nil, nil, nil, err
 			}
-			if parentFolder != nil && parentFolder.Path != "." {
+			if parentFolder != nil && parentFolder.ParentID != "" {
 				folders = append(folders, *parentFolder)
 			}
 		}
--- a/core/artwork/reader_album_test.go
+++ b/core/artwork/reader_album_test.go
@ -141,6 +141,7 @@ var _ = Describe("Album Artwork Reader", func() {
 				ID:              "parentFolder",
 				Path:            "Artist",
 				Name:            "Album",
+				ParentID:        "artistFolder",
 				ImagesUpdatedAt: expectedAt,
 				ImageFiles:      []string{"cover.jpg", "back.jpg"},
 			}
@ -213,14 +214,14 @@ var _ = Describe("Album Artwork Reader", func() {
 			Expect(repo.getCallCount).To(Equal(0))
 		})

-		It("does not include top-level parent for multi-folder albums", func() {
-			// Two album parts under the same artist folder — parent is artist-level
+		It("does not include library root parent for multi-folder albums", func() {
+			// Two album parts directly under the library root — parent is the root itself
 			repo.result = []model.Folder{
 				{
 					ID:              "folder1",
 					Path:            ".",
 					Name:            "AlbumPart1",
-					ParentID:        "artistFolder",
+					ParentID:        "rootFolder",
 					ImagesUpdatedAt: now,
 					ImageFiles:      []string{"cover.jpg"},
 				},
@ -228,16 +229,17 @@ var _ = Describe("Album Artwork Reader", func() {
 					ID:              "folder2",
 					Path:            ".",
 					Name:            "AlbumPart2",
-					ParentID:        "artistFolder",
+					ParentID:        "rootFolder",
 					ImagesUpdatedAt: now,
 					ImageFiles:      []string{},
 				},
 			}
 			repo.parentResult = &model.Folder{
-				ID:         "artistFolder",
-				Path:       ".",
-				Name:       "Artist",
-				ImageFiles: []string{"artist.jpg"},
+				ID:         "rootFolder",
+				Path:       "",
+				Name:       ".",
+				ParentID:   "",
+				ImageFiles: []string{"unrelated.jpg"},
 			}

 			_, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album)
@ -248,6 +250,46 @@ var _ = Describe("Album Artwork Reader", func() {
 			Expect(repo.getCallCount).To(Equal(1))
 		})

+		It("includes top-level album folder for multi-disc albums", func() {
+			// Album folder directly under library root, with disc subfolders
+			repo.result = []model.Folder{
+				{
+					ID:              "folder1",
+					Path:            "Album",
+					Name:            "Disc1",
+					ParentID:        "albumFolder",
+					ImagesUpdatedAt: now,
+					ImageFiles:      []string{"folder.jpg"},
+				},
+				{
+					ID:              "folder2",
+					Path:            "Album",
+					Name:            "Disc2",
+					ParentID:        "albumFolder",
+					ImagesUpdatedAt: now,
+					ImageFiles:      []string{"folder.jpg"},
+				},
+			}
+			repo.parentResult = &model.Folder{
+				ID:              "albumFolder",
+				Path:            ".",
+				Name:            "Album",
+				ParentID:        "rootFolder",
+				ImagesUpdatedAt: expectedAt,
+				ImageFiles:      []string{"cover.jpg"},
+			}
+
+			_, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album)
+
+			Expect(err).ToNot(HaveOccurred())
+			Expect(*imagesUpdatedAt).To(Equal(expectedAt))
+			Expect(imgFiles).To(HaveLen(3))
+			Expect(imgFiles[0]).To(Equal("Album/cover.jpg"))
+			Expect(imgFiles[1]).To(Equal("Album/Disc1/folder.jpg"))
+			Expect(imgFiles[2]).To(Equal("Album/Disc2/folder.jpg"))
+			Expect(repo.getCallCount).To(Equal(1))
+		})
+
 		It("does not query parent for single-folder albums that already have images", func() {
 			repo.result = []model.Folder{
 				{
@ -283,6 +325,7 @@ var _ = Describe("Album Artwork Reader", func() {
 				ID:              "albumFolder",
 				Path:            "Artist",
 				Name:            "Album",
+				ParentID:        "artistFolder",
 				ImagesUpdatedAt: expectedAt,
 				ImageFiles:      []string{"cover.jpg"},
 			}
--- a/core/artwork/reader_disc.go
+++ b/core/artwork/reader_disc.go
@ -16,6 +16,7 @@ import (
 	"github.com/navidrome/navidrome/core/ffmpeg"
 	"github.com/navidrome/navidrome/log"
 	"github.com/navidrome/navidrome/model"
+	"github.com/navidrome/navidrome/utils"
 )

 type discArtworkReader struct {
@ -105,10 +106,9 @@ func newDiscArtworkReader(ctx context.Context, a *artwork, artID model.ArtworkID
 		updatedAt:      imagesUpdatedAt,
 	}
 	r.cacheKey.artID = artID
-	if r.updatedAt != nil && r.updatedAt.After(al.UpdatedAt) {
-		r.cacheKey.lastUpdate = *r.updatedAt
-	} else {
-		r.cacheKey.lastUpdate = al.UpdatedAt
+	r.cacheKey.lastUpdate = utils.TimeNewest(al.UpdatedAt, al.ImportedAt)
+	if imagesUpdatedAt != nil {
+		r.cacheKey.lastUpdate = utils.TimeNewest(r.cacheKey.lastUpdate, *imagesUpdatedAt)
 	}
 	return r, nil
 }
--- a/core/auth/auth.go
+++ b/core/auth/auth.go
@ -100,7 +100,7 @@ func WithAdminUser(ctx context.Context, ds model.DataStore) context.Context {
 		} else {
 			log.Error(ctx, "No admin user found!", err)
 		}
-		u = &model.User{}
+		u = &model.User{IsAdmin: true, UserName: "admin"}
 	}

 	ctx = request.WithUsername(ctx, u.UserName)