diff --git a/core/artwork/image_store.go b/core/artwork/image_store.go index 72cb34ceb..5ec2df294 100644 --- a/core/artwork/image_store.go +++ b/core/artwork/image_store.go @@ -13,6 +13,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/log" "github.com/zeebo/xxh3" ) @@ -157,7 +158,9 @@ func (s *ImageStore) Sweep(ctx context.Context, cutoff time.Time, keep func(hash if remove { // #nosec G122 -- path comes from WalkDir over our own store root, no attacker-controlled symlinks if err := os.Remove(path); err != nil { - return err + // One unremovable file must not strand the rest of the store until the next prune. + log.Warn(ctx, "Artwork: Could not remove store file", "path", path, err) + return nil } removed++ } diff --git a/core/artwork/image_store_test.go b/core/artwork/image_store_test.go index c2cca399d..975194c6d 100644 --- a/core/artwork/image_store_test.go +++ b/core/artwork/image_store_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "time" + "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -205,6 +206,35 @@ var _ = Describe("ImageStore", func() { Expect(freshTmp).To(BeAnExistingFile()) }) + It("keeps sweeping past a file it cannot remove", func() { + tests.SkipOnWindows("uses Unix file permission bits") + if os.Geteuid() == 0 { + Skip("read-only dir cannot block root (e.g. tests in a container)") + } + old := time.Now().Add(-2 * time.Hour) + // "blocked" sorts before "ok", so the walk hits the unremovable file first. + blockedDir := filepath.Join(root, "blocked") + Expect(os.MkdirAll(blockedDir, 0755)).To(Succeed()) + blocked := filepath.Join(blockedDir, "a.jpg") + Expect(os.WriteFile(blocked, []byte("x"), 0600)).To(Succeed()) + Expect(os.Chtimes(blocked, old, old)).To(Succeed()) + + okDir := filepath.Join(root, "ok") + Expect(os.MkdirAll(okDir, 0755)).To(Succeed()) + reachable := filepath.Join(okDir, "b.jpg") + Expect(os.WriteFile(reachable, []byte("y"), 0600)).To(Succeed()) + Expect(os.Chtimes(reachable, old, old)).To(Succeed()) + + Expect(os.Chmod(blockedDir, 0500)).To(Succeed()) + DeferCleanup(func() { _ = os.Chmod(blockedDir, 0755) }) + + removed, err := store.Sweep(ctx, time.Now().Add(-time.Hour), func(string, string) bool { return false }) + Expect(err).ToNot(HaveOccurred()) + Expect(removed).To(Equal(1)) + Expect(blocked).To(BeAnExistingFile()) + Expect(reachable).ToNot(BeAnExistingFile()) + }) + // Prune holds the worker's write lock for the whole sweep, and shutdown waits on the // worker, so an uncancellable walk over a large store stalls it until SIGKILL. It("abandons the walk when the context is cancelled", func() { diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index 348c7d15e..e65d4b1de 100644 --- a/core/artwork/prune_test.go +++ b/core/artwork/prune_test.go @@ -214,11 +214,9 @@ var _ = Describe("Prune", func() { // hb is processed first: aborting on its Remove failure would leave hg unreached. awRepo.OrphanHashes = []string{hb, hg} - // Prune still errors: Sweep revisits hb's leftover file with no warn-and-continue of its own. - err := prune(context.Background(), ds, store) - Expect(err).To(HaveOccurred()) + Expect(prune(context.Background(), ds, store)).To(Succeed()) - _, err = awRepo.GetImage(hg) + _, err := awRepo.GetImage(hg) Expect(err).To(MatchError(model.ErrNotFound)) _, err = store.Open(hg, "image/jpeg") Expect(os.IsNotExist(err)).To(BeTrue())