fix(artwork): keep sweeping past a store file that cannot be removed

Sweep returned the os.Remove error straight out of WalkDir, so a single
unremovable file abandoned the rest of the walk. Every later orphan, stale mime
variant and abandoned temp file then survived until the next prune, which would
abort at the same place.

Warn and carry on instead. The orphan-file loop in prune already had this
resilience; Sweep is where it belongs, since it is the only step that reaches
stray files and superseded variants.
This commit is contained in:
Deluan 2026-08-01 20:54:13 -04:00
parent a48bdefdc7
commit 1344e6e660
3 changed files with 36 additions and 5 deletions

View File

@ -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++
}

View File

@ -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() {

View File

@ -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())