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