From 362941fb5c7658b37ba387c28fb08d10aff58b30 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 1 Aug 2026 21:28:01 -0400 Subject: [PATCH] refactor(artwork): drop the orphaned store remover, aggregate sweep failures Review follow-ups to the prune rework. ImageStore.Remove lost its only production caller when the sweep took over orphan-file reclamation. It carried its own copy of the mtime guard that now lives solely in Sweep, so the two could drift, and it stood as an invitation to grow a second reclaim path. Its specs either duplicated Sweep's own coverage or tested nothing else, so they go with it; the one unrelated test that used it to delete a file calls os.Remove directly. Sweep warned once per file it could not remove, which is fine for a stray permission bit and useless for a store mounted read-only - a large library would emit one line per aged file on every prune, and prune would still report success. Count the failures instead and warn once, with the count and the last error, so a systemic failure is legible without drowning the log. Also: the orphan log line counts rows, not files, and now says so; and the blocked-removal spec asserts its two fixtures really do land in different shards, since the assertion is vacuous if they collide. --- core/artwork/artwork_test.go | 2 +- core/artwork/image_store.go | 34 ++++++++------------------------ core/artwork/image_store_test.go | 27 ------------------------- core/artwork/prune.go | 2 +- core/artwork/prune_test.go | 1 + 5 files changed, 11 insertions(+), 55 deletions(-) diff --git a/core/artwork/artwork_test.go b/core/artwork/artwork_test.go index c58a0fe4e..c6b848101 100644 --- a/core/artwork/artwork_test.go +++ b/core/artwork/artwork_test.go @@ -126,7 +126,7 @@ var _ = Describe("Artwork", func() { // Deleting the store file proves the warm entry serves without touching the original. hash, _ := hashImage(bytes.NewReader(coverBytes)) - Expect(store.Remove(hash, "image/jpeg", time.Now().Add(time.Hour))).To(Succeed()) + Expect(os.Remove(store.path(hash, "image/jpeg"))).To(Succeed()) Eventually(func(g Gomega) { img2, err := svc.Get(ctx, model.MustParseArtworkID("al-al1"), 100, false) g.Expect(err).ToNot(HaveOccurred()) diff --git a/core/artwork/image_store.go b/core/artwork/image_store.go index 5ec2df294..5dbe727e4 100644 --- a/core/artwork/image_store.go +++ b/core/artwork/image_store.go @@ -108,33 +108,11 @@ func (s *ImageStore) Open(hash, mimeType string) (io.ReadCloser, error) { return os.Open(s.path(hash, mimeType)) } -// Remove skips files newer than olderThan: an overlapping acquisition may not have committed its row yet. -func (s *ImageStore) Remove(hash, mimeType string, olderThan time.Time) error { - if !validHash(hash) { - return fmt.Errorf("imagestore: invalid hash %q", hash) - } - path := s.path(hash, mimeType) - info, err := os.Stat(path) - if errors.Is(err, fs.ErrNotExist) { - return nil - } - if err != nil { - return err - } - if info.ModTime().After(olderThan) { - return nil - } - err = os.Remove(path) - if errors.Is(err, fs.ErrNotExist) { - return nil - } - return err -} - // Sweep removes store files not accepted by keep. Files modified after cutoff are always // kept: their acquisition row may not be committed yet. func (s *ImageStore) Sweep(ctx context.Context, cutoff time.Time, keep func(hash, ext string) bool) (int, error) { - removed := 0 + removed, failed := 0, 0 + var lastErr error err := filepath.WalkDir(s.root, func(path string, d fs.DirEntry, err error) error { if err != nil || d.IsDir() { return err @@ -159,13 +137,17 @@ func (s *ImageStore) Sweep(ctx context.Context, cutoff time.Time, keep func(hash // #nosec G122 -- path comes from WalkDir over our own store root, no attacker-controlled symlinks if err := os.Remove(path); err != nil { // 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 + failed, lastErr = failed+1, err + return nil //nolint:nilerr // counted and reported in aggregate below } removed++ } return nil }) + // Aggregated: a store that has gone read-only would otherwise warn once per file, every prune. + if failed > 0 { + log.Warn(ctx, "Artwork: Could not remove store files", "count", failed, "swept", removed, lastErr) + } if errors.Is(err, fs.ErrNotExist) { return removed, nil } diff --git a/core/artwork/image_store_test.go b/core/artwork/image_store_test.go index 975194c6d..f7b2467ca 100644 --- a/core/artwork/image_store_test.go +++ b/core/artwork/image_store_test.go @@ -99,41 +99,14 @@ var _ = Describe("ImageStore", func() { Expect(os.IsNotExist(err)).To(BeTrue()) }) - It("removes without error when already gone", func() { - Expect(store.Remove("beefbeefbeefbeef", "image/jpeg", time.Now())).To(Succeed()) - }) - It("rejects invalid hashes instead of panicking", func() { for _, h := range []string{"", "ab", "BEEFBEEFBEEFBEEF", "../../../../etcpw", "beefbeefbeefbee/"} { Expect(store.Write(h, "image/jpeg", bytes.NewReader([]byte("x")))).To(MatchError(ContainSubstring("invalid hash"))) _, err := store.Open(h, "image/jpeg") Expect(err).To(MatchError(ContainSubstring("invalid hash"))) - Expect(store.Remove(h, "image/jpeg", time.Now())).To(MatchError(ContainSubstring("invalid hash"))) } }) - It("spares a file newer than the cutoff, removes an aged one", func() { - fresh := []byte("fresh") - hf, _ := hashImage(bytes.NewReader(fresh)) - Expect(store.Write(hf, "image/jpeg", bytes.NewReader(fresh))).To(Succeed()) - - aged := []byte("aged") - ha, _ := hashImage(bytes.NewReader(aged)) - Expect(store.Write(ha, "image/jpeg", bytes.NewReader(aged))).To(Succeed()) - old := time.Now().Add(-2 * time.Hour) - Expect(os.Chtimes(store.path(ha, "image/jpeg"), old, old)).To(Succeed()) - - cutoff := time.Now().Add(-time.Hour) - Expect(store.Remove(hf, "image/jpeg", cutoff)).To(Succeed()) - Expect(store.Remove(ha, "image/jpeg", cutoff)).To(Succeed()) - - rc, err := store.Open(hf, "image/jpeg") - Expect(err).ToNot(HaveOccurred()) - rc.Close() - _, err = store.Open(ha, "image/jpeg") - Expect(os.IsNotExist(err)).To(BeTrue()) - }) - It("sweeps unknown files, keeps known ones", func() { d1 := []byte("keep-me") h1, _ := hashImage(bytes.NewReader(d1)) diff --git a/core/artwork/prune.go b/core/artwork/prune.go index 9d00bdfec..c79458b8f 100644 --- a/core/artwork/prune.go +++ b/core/artwork/prune.go @@ -40,7 +40,7 @@ func prune(ctx context.Context, ds model.DataStore, store *ImageStore) error { return err } if orphans > 0 { - log.Info(ctx, "Artwork: Removed orphan images", "count", orphans) + log.Info(ctx, "Artwork: Removed orphan image rows", "count", orphans) } // Read after the delete, so the sweep below reclaims the files of the rows just removed. diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index 5643bb45b..8b8504f89 100644 --- a/core/artwork/prune_test.go +++ b/core/artwork/prune_test.go @@ -204,6 +204,7 @@ var _ = Describe("Prune", func() { // A read-only shard directory makes os.Remove fail (EACCES) for hb's file only. shardDir := filepath.Dir(store.path(hb, "image/jpeg")) + Expect(shardDir).ToNot(Equal(filepath.Dir(store.path(hg, "image/jpeg"))), "fixtures must land in different shards") Expect(os.Chmod(shardDir, 0500)).To(Succeed()) DeferCleanup(func() { _ = os.Chmod(shardDir, 0755) })