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) })