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

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

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
 	}

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

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.

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