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

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

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())
+	})
 })

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

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

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
 }

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

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
 }

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
 }