fix(artwork): guard orphan file removal with the prune grace window

Duplicate ImageStore writes refresh the file mtime and Remove skips files newer than the cutoff, so overlapping acquisitions cannot lose their store files to a concurrent prune.

core/artwork/image_store.go

@@ -54,6 +54,9 @@ func (s *ImageStore) path(hash, mimeType string) string {
 func (s *ImageStore) Write(hash, mimeType string, r io.Reader) error {
 	dst := s.path(hash, mimeType)
 	if _, err := os.Stat(dst); err == nil {
+		// A touched mtime marks the file live so a concurrent prune spares it.
+		now := time.Now()
+		_ = os.Chtimes(dst, now, now)
 		return nil
 	}
 	if err := os.MkdirAll(filepath.Dir(dst), 0755); err != nil {
@@ -78,8 +81,21 @@ func (s *ImageStore) Open(hash, mimeType string) (io.ReadCloser, error) {
 	return os.Open(s.path(hash, mimeType))
 }
 
-func (s *ImageStore) Remove(hash, mimeType string) error {
-	err := os.Remove(s.path(hash, mimeType))
+// Remove deletes the store file unless it is newer than olderThan, in which case
+// an overlapping acquisition may have just touched it and be about to commit its row.
+func (s *ImageStore) Remove(hash, mimeType string, olderThan time.Time) error {
+	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
 	}

core/artwork/image_store_test.go

@@ -51,13 +51,49 @@ var _ = Describe("ImageStore", func() {
 		Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed())
 	})
 
+	It("refreshes the mtime on a duplicate write", func() {
+		data := []byte("touch-me")
+		h, _ := HashImage(bytes.NewReader(data))
+		Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed())
+		old := time.Now().Add(-2 * time.Hour)
+		Expect(os.Chtimes(store.path(h, "image/png"), old, old)).To(Succeed())
+
+		Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed())
+
+		info, err := os.Stat(store.path(h, "image/png"))
+		Expect(err).ToNot(HaveOccurred())
+		Expect(info.ModTime()).To(BeTemporally(">", time.Now().Add(-time.Minute)))
+	})
+
 	It("returns fs.ErrNotExist for missing images", func() {
 		_, err := store.Open("beefbeefbeefbeef", "image/jpeg")
 		Expect(os.IsNotExist(err)).To(BeTrue())
 	})
 
 	It("removes without error when already gone", func() {
-		Expect(store.Remove("beefbeefbeefbeef", "image/jpeg")).To(Succeed())
+		Expect(store.Remove("beefbeefbeefbeef", "image/jpeg", time.Now())).To(Succeed())
+	})
+
+	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() {

core/artwork/prune.go

@@ -39,7 +39,9 @@ func Prune(ctx context.Context, ds model.DataStore, store *ImageStore) error {
 			if _, ok := survivors[h]; ok {
 				continue
 			}
-			if err := store.Remove(h, arts[h].Mime); err != nil {
+			// A spared fresh file is at worst a stray a later sweep reclaims; full
+			// worker/prune mutual exclusion is Phase 2's concern.
+			if err := store.Remove(h, arts[h].Mime, cutoff); err != nil {
 				log.Warn(ctx, "Prune: could not remove artwork file", "hash", h, err)
 			}
 			removed++

core/artwork/prune_test.go

@@ -36,6 +36,8 @@ var _ = Describe("Prune", func() {
 		data := []byte("orphan-bytes")
 		h, _ := HashImage(bytes.NewReader(data))
 		Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed())
+		old := time.Now().Add(-2 * time.Hour)
+		Expect(os.Chtimes(store.path(h, "image/jpeg"), old, old)).To(Succeed())
 		Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg",
 			CreatedAt: time.Now().Add(-2 * time.Hour)})).To(Succeed())
 		awRepo.OrphanHashes = []string{h}
@@ -76,6 +78,23 @@ var _ = Describe("Prune", func() {
 		rc.Close()
 	})
 
+	It("spares an orphan file freshly touched by an overlapping acquisition", func() {
+		data := []byte("racing-bytes")
+		h, _ := HashImage(bytes.NewReader(data))
+		Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed())
+		Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg",
+			CreatedAt: time.Now().Add(-2 * time.Hour)})).To(Succeed())
+		awRepo.OrphanHashes = []string{h}
+		// The row is legitimately orphaned, but a concurrent acquisition just touched the
+		// file's mtime (duplicate Write) and is about to commit a row referencing it.
+
+		Expect(Prune(context.Background(), ds, store)).To(Succeed())
+
+		rc, err := store.Open(h, "image/jpeg")
+		Expect(err).ToNot(HaveOccurred())
+		rc.Close()
+	})
+
 	It("sweeps store files that have no artwork row", func() {
 		stray := []byte("no-row-bytes")
 		h, _ := HashImage(bytes.NewReader(stray))