From 1ed8ebf9b0ea47643cd87f637fec38057ab3bfd3 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 22 Jul 2026 11:16:46 -0400 Subject: [PATCH] test(artwork): leak/soak coverage and deferred assertions --- core/artwork/artwork_suite_test.go | 17 +-- core/artwork/image_store_test.go | 13 ++- core/artwork/prune_test.go | 49 +++++++++ core/artwork/worker_soak_test.go | 159 +++++++++++++++++++++++++++++ 4 files changed, 230 insertions(+), 8 deletions(-) create mode 100644 core/artwork/worker_soak_test.go diff --git a/core/artwork/artwork_suite_test.go b/core/artwork/artwork_suite_test.go index 1fd04627a..b721200e1 100644 --- a/core/artwork/artwork_suite_test.go +++ b/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) diff --git a/core/artwork/image_store_test.go b/core/artwork/image_store_test.go index 214cab3ad..6537ca906 100644 --- a/core/artwork/image_store_test.go +++ b/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() { diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index 983e7c555..3de46942a 100644 --- a/core/artwork/prune_test.go +++ b/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()} diff --git a/core/artwork/worker_soak_test.go b/core/artwork/worker_soak_test.go new file mode 100644 index 000000000..f6ecef422 --- /dev/null +++ b/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) + } +}