test(artwork): leak/soak coverage and deferred assertions

core/artwork/artwork_suite_test.go

@@ -19,12 +19,17 @@ import (
 )
 
 func TestArtwork(t *testing.T) {
-	// Only run goleak checks when the GOLEAK env var is set
-	if os.Getenv("GOLEAK") != "" {
-		defer goleak.VerifyNone(t,
-			goleak.IgnoreTopFunction("github.com/onsi/ginkgo/v2/internal/interrupt_handler.(*InterruptHandler).registerForInterrupts.func2"),
-		)
-	}
+	// Runs unconditionally: the two leaks below are pre-existing and out of this
+	// package's control, so they're ignored by exact top-function instead.
+	defer goleak.VerifyNone(t,
+		goleak.IgnoreTopFunction("github.com/onsi/ginkgo/v2/internal/interrupt_handler.(*InterruptHandler).registerForInterrupts.func2"),
+		// notify's own init() starts this dispatcher the moment it's imported
+		// (via core/storage/local or plugins); it never exits.
+		goleak.IgnoreTopFunction("github.com/rjeczalik/notify.(*recursiveTree).dispatch"),
+		// The old cache_warmer.go starts a goroutine per NewCacheWarmer call with
+		// no shutdown path (dark-launch target for Phase 2, not touched here).
+		goleak.IgnoreTopFunction("github.com/navidrome/navidrome/core/artwork.(*cacheWarmer).waitSignal"),
+	)
 
 	tests.Init(t, false)
 	log.SetLevel(log.LevelFatal)

core/artwork/image_store_test.go

@@ -44,11 +44,20 @@ var _ = Describe("ImageStore", func() {
 		Expect(got).To(Equal(data))
 	})
 
-	It("is idempotent on duplicate writes", func() {
+	It("is idempotent on duplicate writes and preserves the original content", func() {
 		data := []byte("dup")
 		h, _ := HashImage(bytes.NewReader(data))
 		Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed())
-		Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed())
+		// A duplicate write only touches mtime; passing different bytes under the same
+		// hash proves the second reader is never consumed to overwrite the file.
+		Expect(store.Write(h, "image/png", bytes.NewReader([]byte("not-dup")))).To(Succeed())
+
+		rc, err := store.Open(h, "image/png")
+		Expect(err).ToNot(HaveOccurred())
+		defer rc.Close()
+		got, err := io.ReadAll(rc)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(got).To(Equal(data))
 	})
 
 	It("refreshes the mtime on a duplicate write", func() {

core/artwork/prune_test.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"errors"
 	"os"
+	"path/filepath"
 	"time"
 
 	"github.com/navidrome/navidrome/model"
@@ -171,6 +172,54 @@ var _ = Describe("Prune", func() {
 		rc.Close()
 	})
 
+	It("warns and continues past a store.Remove failure instead of aborting the loop", func() {
+		tests.SkipOnWindows("uses Unix file permission bits")
+		old := time.Now().Add(-2 * time.Hour)
+
+		blocked := []byte("blocked-bytes")
+		hb, _ := HashImage(bytes.NewReader(blocked))
+		Expect(store.Write(hb, "image/jpeg", bytes.NewReader(blocked))).To(Succeed())
+		Expect(os.Chtimes(store.path(hb, "image/jpeg"), old, old)).To(Succeed())
+		Expect(awRepo.PutImage(&model.Artwork{Hash: hb, Mime: "image/jpeg"})).To(Succeed())
+		ageArtwork(hb, old)
+
+		good := []byte("good-bytes")
+		hg, _ := HashImage(bytes.NewReader(good))
+		Expect(store.Write(hg, "image/jpeg", bytes.NewReader(good))).To(Succeed())
+		Expect(os.Chtimes(store.path(hg, "image/jpeg"), old, old)).To(Succeed())
+		Expect(awRepo.PutImage(&model.Artwork{Hash: hg, Mime: "image/jpeg"})).To(Succeed())
+		ageArtwork(hg, old)
+
+		// A read-only shard directory makes os.Remove fail (EACCES) for hb's file only.
+		shardDir := filepath.Dir(store.path(hb, "image/jpeg"))
+		Expect(os.Chmod(shardDir, 0500)).To(Succeed())
+		DeferCleanup(func() { _ = os.Chmod(shardDir, 0755) })
+
+		// hb (blocked) is processed first: if store.Remove's failure aborted the loop
+		// instead of warning and continuing, hg would never be reached.
+		awRepo.OrphanHashes = []string{hb, hg}
+
+		// Prune still errors: Sweep independently revisits hb's leftover file and,
+		// unlike the loop below, has no warn-and-continue fallback of its own.
+		err := Prune(context.Background(), ds, store)
+		Expect(err).To(HaveOccurred())
+
+		// hg: reached and fully pruned despite being queued after the failing hb -
+		// proof the loop didn't return/break on the first Remove error.
+		_, err = awRepo.GetImage(hg)
+		Expect(err).To(MatchError(model.ErrNotFound))
+		_, err = store.Open(hg, "image/jpeg")
+		Expect(os.IsNotExist(err)).To(BeTrue())
+
+		// hb: row still purged (DeleteOrphans doesn't depend on file removal), but the
+		// file itself survives since store.Remove failed and only warned.
+		_, err = awRepo.GetImage(hb)
+		Expect(err).To(MatchError(model.ErrNotFound))
+		rc, err := store.Open(hb, "image/jpeg")
+		Expect(err).ToNot(HaveOccurred())
+		rc.Close()
+	})
+
 	It("never sweeps files on a transient DB error", func() {
 		ds.MockedArtwork = &flakyGetArtworkRepo{MockArtworkRepo: tests.CreateMockArtworkRepo()}
 

core/artwork/worker_soak_test.go

@@ -0,0 +1,159 @@
+package artwork
+
+import (
+	"context"
+	"io"
+	"os"
+	"runtime"
+	"testing"
+	"time"
+
+	"github.com/navidrome/navidrome/conf"
+	"github.com/navidrome/navidrome/conf/configtest"
+	"github.com/navidrome/navidrome/log"
+	"github.com/navidrome/navidrome/model"
+	"github.com/navidrome/navidrome/tests"
+)
+
+// soakCycles is deliberately >2000: this is a leak regression guard, not a
+// performance benchmark, so it favors a stable signal over raw speed.
+const soakCycles = 2200
+
+// TestWorkerSoak drives processItem across a mix of sources (folder, embedded
+// extraction, dangling refs) for many cycles, reading each acquired image back
+// through ImageStore.Open, and asserts goroutines/heap plateau instead of
+// growing unbounded. Skipped under -short.
+func TestWorkerSoak(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping soak test in short mode")
+	}
+	tests.Init(t, false)
+	log.SetLevel(log.LevelFatal)
+	defer configtest.SetupConfig()()
+
+	repoRoot, err := os.Getwd()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	libRepo := &tests.MockLibraryRepo{}
+	libRepo.SetData(model.Libraries{{ID: 0, Path: testFileLibPath(repoRoot)}})
+	folderRepo := &fakeFolderRepo{result: []model.Folder{{
+		Path:       "tests/fixtures/artist/an-album",
+		ImageFiles: []string{"cover.jpg"},
+	}}}
+	ffm := tests.NewMockFFmpeg("")
+	prov := &fakeExternalProvider{}
+	artRepo := tests.CreateMockArtworkRepo()
+	albumRepo := tests.CreateMockAlbumRepo()
+	albumRepo.SetData(model.Albums{
+		{ID: "al-folder", Name: "Folder Album", FolderIDs: []string{"f1"}},
+		{ID: "al-embed", Name: "Embedded Album", EmbedArtPath: "tests/fixtures/artist/an-album/test.mp3", FolderIDs: []string{"f1"}},
+	})
+	ds := &tests.MockDataStore{
+		MockedFolder:  folderRepo,
+		MockedLibrary: libRepo,
+		MockedArtwork: artRepo,
+		MockedAlbum:   albumRepo,
+	}
+	store := NewImageStore(t.TempDir())
+	deps := &workerDeps{ds: ds, store: store, prov: prov, ffmpeg: ffm}
+	conf.Server.CoverArtPriority = "cover.jpg, embedded"
+
+	// Dangling refs (al/ra ids the repos don't know about) mirror an entity
+	// deleted after being enqueued; ds.Radio auto-provisions an empty mock repo.
+	items := []model.ArtworkQueueItem{
+		{ItemKind: "al", ItemID: "al-folder"},
+		{ItemKind: "al", ItemID: "al-embed"},
+		{ItemKind: "al", ItemID: "al-does-not-exist"},
+		{ItemKind: "ra", ItemID: "ra-does-not-exist"},
+	}
+
+	fdCount := func() int {
+		if runtime.GOOS != "linux" {
+			return -1
+		}
+		entries, err := os.ReadDir("/proc/self/fd")
+		if err != nil {
+			return -1
+		}
+		return len(entries)
+	}
+
+	settleGoroutines := func() int {
+		// Background goroutines (GC workers, etc.) can take a moment to wind down;
+		// poll for two consecutive equal samples instead of trusting a single one.
+		prev := -1
+		for range 100 {
+			runtime.GC()
+			n := runtime.NumGoroutine()
+			if n == prev {
+				return n
+			}
+			prev = n
+			time.Sleep(10 * time.Millisecond)
+		}
+		return prev
+	}
+
+	baselineGoroutines := settleGoroutines()
+	baselineFDs := fdCount()
+
+	var heapAt10Pct uint64
+	start := time.Now()
+	for i := range soakCycles {
+		it := items[i%len(items)]
+		out := processItem(context.Background(), deps, it)
+
+		// "Serve-adjacent" read-back: exercise the Phase 2 surfaces a caller would
+		// use after acquisition, not the old serving pipeline.
+		if out == outcomeFound {
+			ia, err := artRepo.GetItemArtwork(it.ItemKind, it.ItemID, model.ImageTypePrimary)
+			if err != nil {
+				t.Fatalf("cycle %d: GetItemArtwork: %v", i, err)
+			}
+			art, err := artRepo.GetImage(ia.Hash)
+			if err != nil {
+				t.Fatalf("cycle %d: GetImage: %v", i, err)
+			}
+			rc, err := store.Open(ia.Hash, art.Mime)
+			switch {
+			case err == nil:
+				_, _ = io.Copy(io.Discard, rc)
+				rc.Close()
+			case os.IsNotExist(err):
+				// Folder-backed art has no store file; that's expected.
+			default:
+				t.Fatalf("cycle %d: store.Open: %v", i, err)
+			}
+		}
+
+		if i == soakCycles/10 {
+			runtime.GC()
+			var ms runtime.MemStats
+			runtime.ReadMemStats(&ms)
+			heapAt10Pct = ms.HeapAlloc
+		}
+	}
+	elapsed := time.Since(start)
+
+	finalGoroutines := settleGoroutines()
+	finalFDs := fdCount()
+
+	runtime.GC()
+	var ms runtime.MemStats
+	runtime.ReadMemStats(&ms)
+
+	t.Logf("soak: cycles=%d elapsed=%s goroutines(baseline=%d final=%d) heap(10%%-mark=%d final=%d) fds(baseline=%d final=%d)",
+		soakCycles, elapsed, baselineGoroutines, finalGoroutines, heapAt10Pct, ms.HeapAlloc, baselineFDs, finalFDs)
+
+	if finalGoroutines > baselineGoroutines {
+		t.Errorf("goroutine count grew: baseline=%d final=%d", baselineGoroutines, finalGoroutines)
+	}
+	if heapAt10Pct > 0 && ms.HeapAlloc > 2*heapAt10Pct {
+		t.Errorf("heap did not plateau: 10%%-mark=%d final=%d (final > 2x 10%%-mark)", heapAt10Pct, ms.HeapAlloc)
+	}
+	if runtime.GOOS == "linux" && baselineFDs >= 0 && finalFDs > baselineFDs {
+		t.Errorf("fd count grew: baseline=%d final=%d", baselineFDs, finalFDs)
+	}
+}