From bf614e66adad3f1ce99a479fc1a8a1cd02e48d71 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 22 Jul 2026 00:19:45 -0400 Subject: [PATCH] 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 | 20 +++++++++++++++-- core/artwork/image_store_test.go | 38 +++++++++++++++++++++++++++++++- core/artwork/prune.go | 4 +++- core/artwork/prune_test.go | 19 ++++++++++++++++ 4 files changed, 77 insertions(+), 4 deletions(-) diff --git a/core/artwork/image_store.go b/core/artwork/image_store.go index 517dfc879..ef27ed00a 100644 --- a/core/artwork/image_store.go +++ b/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 } diff --git a/core/artwork/image_store_test.go b/core/artwork/image_store_test.go index 2647f3b7f..89646deba 100644 --- a/core/artwork/image_store_test.go +++ b/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() { diff --git a/core/artwork/prune.go b/core/artwork/prune.go index de8811e14..70aac091e 100644 --- a/core/artwork/prune.go +++ b/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++ diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index aee8cd17c..9d4aad350 100644 --- a/core/artwork/prune_test.go +++ b/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))