From 8147f7c40bf2bafd58dc43c701dce5ad5e3908c3 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 22 Jul 2026 00:28:51 -0400 Subject: [PATCH] fix(artwork): rewrite vanished duplicates and sweep stale mime variants Write falls through to a real write when the liveness touch fails, and sweep retention now matches the recorded mime's extension so obsolete variants are reclaimed. --- core/artwork/image_store.go | 11 ++++--- core/artwork/image_store_test.go | 44 ++++++++++++++++++++++++-- core/artwork/prune.go | 13 +++----- core/artwork/prune_test.go | 22 ++++++++++++- model/artwork.go | 4 +-- persistence/artwork_repository.go | 19 ++++++++--- persistence/artwork_repository_test.go | 7 ++-- tests/mock_artwork_repo.go | 10 +++--- 8 files changed, 99 insertions(+), 31 deletions(-) diff --git a/core/artwork/image_store.go b/core/artwork/image_store.go index ef27ed00a..178c19d10 100644 --- a/core/artwork/image_store.go +++ b/core/artwork/image_store.go @@ -56,8 +56,10 @@ func (s *ImageStore) Write(hash, mimeType string, r io.Reader) error { 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.Chtimes(dst, now, now); err == nil { + return nil + } + // touch failed (file likely pruned concurrently) — fall through and write it } if err := os.MkdirAll(filepath.Dir(dst), 0755); err != nil { return err @@ -104,7 +106,7 @@ func (s *ImageStore) Remove(hash, mimeType string, olderThan time.Time) error { // Sweep removes store files not accepted by keep. Files modified after cutoff // (including temp files) are always kept: their acquisition row may not be committed yet. -func (s *ImageStore) Sweep(cutoff time.Time, keep func(hash string) bool) (int, error) { +func (s *ImageStore) Sweep(cutoff time.Time, keep func(hash, ext string) bool) (int, error) { removed := 0 err := filepath.WalkDir(s.root, func(path string, d fs.DirEntry, err error) error { if err != nil || d.IsDir() { @@ -120,7 +122,8 @@ func (s *ImageStore) Sweep(cutoff time.Time, keep func(hash string) bool) (int, name := d.Name() remove := strings.HasPrefix(name, ".") // abandoned temp file past the grace window if !remove { - remove = !keep(strings.TrimSuffix(name, filepath.Ext(name))) + ext := filepath.Ext(name) + remove = !keep(strings.TrimSuffix(name, ext), ext) } if remove { // #nosec G122 -- path comes from WalkDir over our own store root, no attacker-controlled symlinks diff --git a/core/artwork/image_store_test.go b/core/artwork/image_store_test.go index 89646deba..0bf721623 100644 --- a/core/artwork/image_store_test.go +++ b/core/artwork/image_store_test.go @@ -65,6 +65,22 @@ var _ = Describe("ImageStore", func() { Expect(info.ModTime()).To(BeTemporally(">", time.Now().Add(-time.Minute))) }) + It("rewrites the bytes when the existing file vanished before the liveness touch", func() { + data := []byte("vanishing") + h, _ := HashImage(bytes.NewReader(data)) + for range 10 { + Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed()) + Expect(os.Remove(store.path(h, "image/png"))).To(Succeed()) + Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed()) + + rc, err := store.Open(h, "image/png") + Expect(err).ToNot(HaveOccurred()) + got, _ := io.ReadAll(rc) + rc.Close() + Expect(got).To(Equal(data)) + } + }) + It("returns fs.ErrNotExist for missing images", func() { _, err := store.Open("beefbeefbeefbeef", "image/jpeg") Expect(os.IsNotExist(err)).To(BeTrue()) @@ -107,7 +123,7 @@ var _ = Describe("ImageStore", func() { old := time.Now().Add(-2 * time.Hour) Expect(os.Chtimes(store.path(h2, "image/jpeg"), old, old)).To(Succeed()) - removed, err := store.Sweep(time.Now().Add(-time.Hour), func(h string) bool { return h == h1 }) + removed, err := store.Sweep(time.Now().Add(-time.Hour), func(h, _ string) bool { return h == h1 }) Expect(err).ToNot(HaveOccurred()) Expect(removed).To(Equal(1)) _, err = store.Open(h2, "image/jpeg") @@ -117,12 +133,34 @@ var _ = Describe("ImageStore", func() { rc.Close() }) + It("sweeps a stale mime variant of a known hash, keeps the current one", func() { + data := []byte("same-bytes") + h, _ := HashImage(bytes.NewReader(data)) + Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed()) + 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/png"), old, old)).To(Succeed()) + Expect(os.Chtimes(store.path(h, "image/jpeg"), old, old)).To(Succeed()) + + // The recorded mime is image/jpeg, so the .png variant is obsolete. + removed, err := store.Sweep(time.Now().Add(-time.Hour), func(hash, ext string) bool { + return hash == h && ext == ".jpg" + }) + Expect(err).ToNot(HaveOccurred()) + Expect(removed).To(Equal(1)) + _, err = store.Open(h, "image/png") + Expect(os.IsNotExist(err)).To(BeTrue()) + rc, err := store.Open(h, "image/jpeg") + Expect(err).ToNot(HaveOccurred()) + rc.Close() + }) + It("keeps young unknown files inside the grace window", func() { d := []byte("fresh-orphan") h, _ := HashImage(bytes.NewReader(d)) Expect(store.Write(h, "image/jpeg", bytes.NewReader(d))).To(Succeed()) - removed, err := store.Sweep(time.Now().Add(-time.Hour), func(string) bool { return false }) + removed, err := store.Sweep(time.Now().Add(-time.Hour), func(string, string) bool { return false }) Expect(err).ToNot(HaveOccurred()) Expect(removed).To(Equal(0)) rc, err := store.Open(h, "image/jpeg") @@ -139,7 +177,7 @@ var _ = Describe("ImageStore", func() { freshTmp := filepath.Join(root, ".fresh.tmp") Expect(os.WriteFile(freshTmp, []byte("y"), 0600)).To(Succeed()) - removed, err := store.Sweep(time.Now().Add(-time.Hour), func(string) bool { return true }) + removed, err := store.Sweep(time.Now().Add(-time.Hour), func(string, string) bool { return true }) Expect(err).ToNot(HaveOccurred()) Expect(removed).To(Equal(1)) Expect(oldTmp).ToNot(BeAnExistingFile()) diff --git a/core/artwork/prune.go b/core/artwork/prune.go index 70aac091e..db1e4fdc1 100644 --- a/core/artwork/prune.go +++ b/core/artwork/prune.go @@ -49,17 +49,14 @@ func Prune(ctx context.Context, ds model.DataStore, store *ImageStore) error { log.Info(ctx, "Prune: removed orphan artwork", "count", removed) } - hashes, err := repo.GetAllHashes() + mimes, err := repo.GetAllMimes() if err != nil { return err } - known := make(map[string]struct{}, len(hashes)) - for _, h := range hashes { - known[h] = struct{}{} - } - removed, err := store.Sweep(cutoff, func(hash string) bool { - _, ok := known[hash] - return ok + removed, err := store.Sweep(cutoff, func(hash, ext string) bool { + // A known hash under a stale extension is a superseded mime variant — reclaim it. + m, ok := mimes[hash] + return ok && ext == extForMime(m) }) if err != nil { return err diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index 9d4aad350..2e5c80d23 100644 --- a/core/artwork/prune_test.go +++ b/core/artwork/prune_test.go @@ -17,7 +17,7 @@ type flakyGetArtworkRepo struct { *tests.MockArtworkRepo } -func (f *flakyGetArtworkRepo) GetAllHashes() ([]string, error) { +func (f *flakyGetArtworkRepo) GetAllMimes() (map[string]string, error) { return nil, errors.New("db locked") } @@ -108,6 +108,26 @@ var _ = Describe("Prune", func() { Expect(os.IsNotExist(err)).To(BeTrue()) }) + It("sweeps an obsolete mime variant of a reacquired hash", func() { + data := []byte("variant-bytes") + h, _ := HashImage(bytes.NewReader(data)) + Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed()) + 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/png"), old, old)).To(Succeed()) + Expect(os.Chtimes(store.path(h, "image/jpeg"), old, old)).To(Succeed()) + // The row records the current mime; the .png file is a superseded variant. + Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg"})).To(Succeed()) + + Expect(Prune(context.Background(), ds, store)).To(Succeed()) + + _, err := store.Open(h, "image/png") + Expect(os.IsNotExist(err)).To(BeTrue()) + rc, err := store.Open(h, "image/jpeg") + Expect(err).ToNot(HaveOccurred()) + rc.Close() + }) + It("never sweeps files on a transient DB error", func() { ds.MockedArtwork = &flakyGetArtworkRepo{MockArtworkRepo: tests.CreateMockArtworkRepo()} diff --git a/model/artwork.go b/model/artwork.go index 5b50fbda0..f5f3ab4db 100644 --- a/model/artwork.go +++ b/model/artwork.go @@ -73,8 +73,8 @@ type ArtworkRepository interface { DeleteForItem(kind, id string) error // GetInfoForItems hydrates a page: one batched query, item_artwork joined to artwork. GetInfoForItems(kind string, ids []string) (map[string]ItemArtworkInfo, error) - // GetAllHashes returns every stored artwork hash, for sweep membership checks. - GetAllHashes() ([]string, error) + // GetAllMimes returns hash -> current mime for every stored artwork, for sweep retention checks. + GetAllMimes() (map[string]string, error) } type ArtworkQueueRepository interface { diff --git a/persistence/artwork_repository.go b/persistence/artwork_repository.go index a18b9d4af..05912a0d6 100644 --- a/persistence/artwork_repository.go +++ b/persistence/artwork_repository.go @@ -74,11 +74,20 @@ func (r *artworkRepository) GetImages(hashes []string) (map[string]model.Artwork return res, nil } -func (r *artworkRepository) GetAllHashes() ([]string, error) { - sel := Select("hash").From(r.tableName) - var hashes []string - err := r.queryAllSlice(sel, &hashes) - return hashes, err +func (r *artworkRepository) GetAllMimes() (map[string]string, error) { + sel := Select("hash", "mime").From(r.tableName) + var rows []struct { + Hash string + Mime string + } + if err := r.queryAll(sel, &rows); err != nil { + return nil, err + } + res := make(map[string]string, len(rows)) + for _, row := range rows { + res[row.Hash] = row.Mime + } + return res, nil } func (r *artworkRepository) GetOrphanHashes(createdBefore time.Time) ([]string, error) { diff --git a/persistence/artwork_repository_test.go b/persistence/artwork_repository_test.go index b3971f88f..a13b1e7cb 100644 --- a/persistence/artwork_repository_test.go +++ b/persistence/artwork_repository_test.go @@ -76,12 +76,13 @@ var _ = Describe("ArtworkRepository", func() { Expect(got["b2"].Mime).To(Equal("image/png")) }) - It("returns every stored hash", func() { + It("returns every stored hash with its current mime", func() { Expect(repo.PutImage(&model.Artwork{Hash: "all1", Mime: "image/jpeg"})).To(Succeed()) Expect(repo.PutImage(&model.Artwork{Hash: "all2", Mime: "image/png"})).To(Succeed()) - hashes, err := repo.GetAllHashes() + mimes, err := repo.GetAllMimes() Expect(err).ToNot(HaveOccurred()) - Expect(hashes).To(ConsistOf("all1", "all2")) + Expect(mimes).To(HaveKeyWithValue("all1", "image/jpeg")) + Expect(mimes).To(HaveKeyWithValue("all2", "image/png")) }) It("finds orphans older than cutoff, honoring item_artwork references", func() { diff --git a/tests/mock_artwork_repo.go b/tests/mock_artwork_repo.go index 6575416b9..851ee7f48 100644 --- a/tests/mock_artwork_repo.go +++ b/tests/mock_artwork_repo.go @@ -58,15 +58,15 @@ func (m *MockArtworkRepo) GetOrphanHashes(createdBefore time.Time) ([]string, er return m.OrphanHashes, m.Err } -func (m *MockArtworkRepo) GetAllHashes() ([]string, error) { +func (m *MockArtworkRepo) GetAllMimes() (map[string]string, error) { if m.Err != nil { return nil, m.Err } - hashes := make([]string, 0, len(m.Data)) - for h := range m.Data { - hashes = append(hashes, h) + mimes := make(map[string]string, len(m.Data)) + for h, a := range m.Data { + mimes[h] = a.Mime } - return hashes, nil + return mimes, nil } func (m *MockArtworkRepo) DeleteOrphans(createdBefore time.Time, hashes []string) error {