From 6f7f9c64633e362e5f175ec475d76eb879752449 Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 21 Jul 2026 23:57:58 -0400 Subject: [PATCH] fix(artwork): address review findings on prune/sweep races and mock fidelity Sweep now honors an mtime grace window (in-flight acquisitions and temp files), reacquired orphans reset the prune grace window, and the queue mock implements real stale-absent semantics. --- core/artwork/image_store.go | 20 +++++++++++---- core/artwork/image_store_test.go | 35 +++++++++++++++++++++++++- core/artwork/prune.go | 7 ++++-- core/artwork/prune_test.go | 2 ++ persistence/artwork_repository.go | 8 +++--- persistence/artwork_repository_test.go | 14 +++++++++++ tests/mock_artwork_queue_repo.go | 27 +++++++++++++++++++- tests/mock_data_store.go | 6 ++++- 8 files changed, 105 insertions(+), 14 deletions(-) diff --git a/core/artwork/image_store.go b/core/artwork/image_store.go index c98064d13..517dfc879 100644 --- a/core/artwork/image_store.go +++ b/core/artwork/image_store.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "strings" + "time" "github.com/zeebo/xxh3" ) @@ -85,18 +86,27 @@ func (s *ImageStore) Remove(hash, mimeType string) error { return err } -func (s *ImageStore) Sweep(keep func(hash string) bool) (int, 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) { removed := 0 err := filepath.WalkDir(s.root, func(path string, d fs.DirEntry, err error) error { if err != nil || d.IsDir() { return err } - name := d.Name() - if strings.HasPrefix(name, ".") { // in-flight temp files + info, err := d.Info() + if err != nil { + return err + } + if info.ModTime().After(cutoff) { return nil } - hash := strings.TrimSuffix(name, filepath.Ext(name)) - if !keep(hash) { + name := d.Name() + remove := strings.HasPrefix(name, ".") // abandoned temp file past the grace window + if !remove { + remove = !keep(strings.TrimSuffix(name, filepath.Ext(name))) + } + if remove { // #nosec G122 -- path comes from WalkDir over our own store root, no attacker-controlled symlinks if err := os.Remove(path); err != nil { return err diff --git a/core/artwork/image_store_test.go b/core/artwork/image_store_test.go index c3ca69a58..2647f3b7f 100644 --- a/core/artwork/image_store_test.go +++ b/core/artwork/image_store_test.go @@ -5,6 +5,7 @@ import ( "io" "os" "path/filepath" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -67,7 +68,10 @@ var _ = Describe("ImageStore", func() { h2, _ := HashImage(bytes.NewReader(d2)) Expect(store.Write(h2, "image/jpeg", bytes.NewReader(d2))).To(Succeed()) - removed, err := store.Sweep(func(h string) bool { return h == h1 }) + 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 }) Expect(err).ToNot(HaveOccurred()) Expect(removed).To(Equal(1)) _, err = store.Open(h2, "image/jpeg") @@ -76,4 +80,33 @@ var _ = Describe("ImageStore", func() { 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 }) + Expect(err).ToNot(HaveOccurred()) + Expect(removed).To(Equal(0)) + rc, err := store.Open(h, "image/jpeg") + Expect(err).ToNot(HaveOccurred()) + rc.Close() + }) + + It("removes abandoned temp files past the grace window, keeps fresh ones", func() { + oldTmp := filepath.Join(root, ".old.tmp") + Expect(os.WriteFile(oldTmp, []byte("x"), 0600)).To(Succeed()) + old := time.Now().Add(-2 * time.Hour) + Expect(os.Chtimes(oldTmp, old, old)).To(Succeed()) + + 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 }) + Expect(err).ToNot(HaveOccurred()) + Expect(removed).To(Equal(1)) + Expect(oldTmp).ToNot(BeAnExistingFile()) + Expect(freshTmp).To(BeAnExistingFile()) + }) }) diff --git a/core/artwork/prune.go b/core/artwork/prune.go index ebbb0bbc4..b787dd1c9 100644 --- a/core/artwork/prune.go +++ b/core/artwork/prune.go @@ -13,7 +13,10 @@ const pruneMinAge = time.Hour func Prune(ctx context.Context, ds model.DataStore, store *ImageStore) error { repo := ds.Artwork(ctx) - orphans, err := repo.GetOrphanHashes(time.Now().Add(-pruneMinAge)) + // One grace cutoff for both the DB orphan check and the file sweep: files younger + // than the window may belong to acquisitions whose rows aren't committed yet. + cutoff := time.Now().Add(-pruneMinAge) + orphans, err := repo.GetOrphanHashes(cutoff) if err != nil { return err } @@ -41,7 +44,7 @@ func Prune(ctx context.Context, ds model.DataStore, store *ImageStore) error { for _, h := range hashes { known[h] = struct{}{} } - removed, err := store.Sweep(func(hash string) bool { + removed, err := store.Sweep(cutoff, func(hash string) bool { _, ok := known[hash] return ok }) diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index 2c8751674..0dd0b660f 100644 --- a/core/artwork/prune_test.go +++ b/core/artwork/prune_test.go @@ -60,6 +60,8 @@ var _ = Describe("Prune", func() { stray := []byte("no-row-bytes") h, _ := HashImage(bytes.NewReader(stray)) Expect(store.Write(h, "image/jpeg", bytes.NewReader(stray))).To(Succeed()) + old := time.Now().Add(-2 * time.Hour) + Expect(os.Chtimes(store.path(h, "image/jpeg"), old, old)).To(Succeed()) Expect(Prune(context.Background(), ds, store)).To(Succeed()) diff --git a/persistence/artwork_repository.go b/persistence/artwork_repository.go index c87836ca9..145e0b3f7 100644 --- a/persistence/artwork_repository.go +++ b/persistence/artwork_repository.go @@ -45,16 +45,16 @@ func (r *artworkRepository) GetImage(hash string) (*model.Artwork, error) { } func (r *artworkRepository) PutImage(a *model.Artwork) error { - if a.CreatedAt.IsZero() { - a.CreatedAt = time.Now() - } + // created_at is the last-acquisition-write time the prune grace window keys on. + a.CreatedAt = time.Now() values, err := toSQLArgs(*a) if err != nil { return err } + // created_at=excluded.created_at: reacquiring an orphan must reset the prune grace window. ins := Insert(r.tableName).SetMap(values).Suffix(`ON CONFLICT (hash) DO UPDATE SET mime=excluded.mime, width=excluded.width, height=excluded.height, size_bytes=excluded.size_bytes, blur_hash=excluded.blur_hash, - source_path=excluded.source_path, ref_mtime=excluded.ref_mtime`) + source_path=excluded.source_path, ref_mtime=excluded.ref_mtime, created_at=excluded.created_at`) _, err = r.executeSQL(ins) return err } diff --git a/persistence/artwork_repository_test.go b/persistence/artwork_repository_test.go index 0fdb122b6..12456bcd1 100644 --- a/persistence/artwork_repository_test.go +++ b/persistence/artwork_repository_test.go @@ -8,6 +8,7 @@ import ( "github.com/navidrome/navidrome/model" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/pocketbase/dbx" ) // clearArtworkTables resets the shared test DB's artwork tables so specs don't leak state. @@ -48,6 +49,19 @@ var _ = Describe("ArtworkRepository", func() { Expect(got.BlurHash).To(Equal("XYZ")) }) + It("refreshes created_at when reacquiring an existing hash", func() { + Expect(repo.PutImage(&model.Artwork{Hash: "reacq", Mime: "image/jpeg"})).To(Succeed()) + _, err := GetDBXBuilder().NewQuery("UPDATE artwork SET created_at={:t} WHERE hash='reacq'"). + Bind(dbx.Params{"t": "2000-01-01 00:00:00"}).Execute() + Expect(err).ToNot(HaveOccurred()) + + Expect(repo.PutImage(&model.Artwork{Hash: "reacq", Mime: "image/png"})).To(Succeed()) + + got, err := repo.GetImage("reacq") + Expect(err).ToNot(HaveOccurred()) + Expect(got.CreatedAt).To(BeTemporally(">", time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC))) + }) + It("returns ErrNotFound for a missing hash", func() { _, err := repo.GetImage("nope") Expect(err).To(MatchError(model.ErrNotFound)) diff --git a/tests/mock_artwork_queue_repo.go b/tests/mock_artwork_queue_repo.go index dcec7486d..fceded1fb 100644 --- a/tests/mock_artwork_queue_repo.go +++ b/tests/mock_artwork_queue_repo.go @@ -11,6 +11,8 @@ type MockArtworkQueueRepo struct { model.ArtworkQueueRepository Data map[string]model.ArtworkQueueItem // keyed by iaKey(kind, id, imageType) Err error + // ItemArtworkSource, when set, backs EnqueueStaleAbsent with real item_artwork state. + ItemArtworkSource *MockArtworkRepo } func CreateMockArtworkQueueRepo() *MockArtworkQueueRepo { @@ -98,5 +100,28 @@ func (m *MockArtworkQueueRepo) Count() (int64, error) { } func (m *MockArtworkQueueRepo) EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) { - return 0, m.Err + if m.Err != nil || m.ItemArtworkSource == nil { + return 0, m.Err + } + now := time.Now() + var inserted int64 + for _, ia := range m.ItemArtworkSource.ItemData { + if ia.ItemKind != kind || ia.Hash != "" || !ia.AttemptedAt.Before(attemptedBefore) { + continue + } + k := iaKey(ia.ItemKind, ia.ItemID, ia.ImageType) + if _, ok := m.Data[k]; ok { // DO NOTHING: never touch existing queue rows + continue + } + m.Data[k] = model.ArtworkQueueItem{ + ItemKind: ia.ItemKind, + ItemID: ia.ItemID, + ImageType: ia.ImageType, + Priority: model.ArtworkPriorityRecheck, + RetryAt: now, + EnqueuedAt: now, + } + inserted++ + } + return inserted, nil } diff --git a/tests/mock_data_store.go b/tests/mock_data_store.go index a6ca29df5..d8585fabf 100644 --- a/tests/mock_data_store.go +++ b/tests/mock_data_store.go @@ -267,7 +267,11 @@ func (db *MockDataStore) ArtworkQueue(ctx context.Context) model.ArtworkQueueRep if db.RealDS != nil { return db.RealDS.ArtworkQueue(ctx) } - db.MockedArtworkQueue = CreateMockArtworkQueueRepo() + q := CreateMockArtworkQueueRepo() + if aw, ok := db.Artwork(ctx).(*MockArtworkRepo); ok { + q.ItemArtworkSource = aw + } + db.MockedArtworkQueue = q return db.MockedArtworkQueue }