fix(artwork): atomic orphan deletion and timestamp semantics from review

DeleteOrphans re-checks age+references at delete time, PutItemArtwork defaults attempted_at, queue mock timestamps mirror SQL.

core/artwork/prune.go

@@ -16,24 +16,35 @@ func Prune(ctx context.Context, ds model.DataStore, store *ImageStore) error {
 	// 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)
+	candidates, err := repo.GetOrphanHashes(cutoff)
 	if err != nil {
 		return err
 	}
-	if len(orphans) > 0 {
-		arts, err := repo.GetImages(orphans)
+	if len(candidates) > 0 {
+		arts, err := repo.GetImages(candidates)
 		if err != nil {
 			return err
 		}
-		if err := repo.DeleteImages(orphans...); err != nil {
+		if err := repo.DeleteOrphans(cutoff, candidates); err != nil {
 			return err
 		}
-		for _, a := range arts {
-			if err := store.Remove(a.Hash, a.Mime); err != nil {
-				log.Warn(ctx, "Prune: could not remove artwork file", "hash", a.Hash, err)
-			}
+		// DeleteOrphans may spare candidates reacquired since the snapshot; only remove files
+		// for rows actually gone (absent from the post-delete re-read).
+		survivors, err := repo.GetImages(candidates)
+		if err != nil {
+			return err
 		}
-		log.Info(ctx, "Prune: removed orphan artwork", "count", len(orphans))
+		removed := 0
+		for _, h := range candidates {
+			if _, ok := survivors[h]; ok {
+				continue
+			}
+			if err := store.Remove(h, arts[h].Mime); err != nil {
+				log.Warn(ctx, "Prune: could not remove artwork file", "hash", h, err)
+			}
+			removed++
+		}
+		log.Info(ctx, "Prune: removed orphan artwork", "count", removed)
 	}
 
 	hashes, err := repo.GetAllHashes()

core/artwork/prune_test.go

@@ -56,6 +56,26 @@ var _ = Describe("Prune", func() {
 		rc.Close()
 	})
 
+	It("spares a candidate reacquired between snapshot and delete", func() {
+		data := []byte("reacquired-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}
+		// Reacquisition: an item now references the hash the snapshot flagged as orphan.
+		Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: "a1",
+			ImageType: model.ImageTypePrimary, Hash: h, Source: "folder"})).To(Succeed())
+
+		Expect(Prune(context.Background(), ds, store)).To(Succeed())
+
+		_, err := awRepo.GetImage(h)
+		Expect(err).ToNot(HaveOccurred())
+		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))

model/artwork.go

@@ -65,7 +65,8 @@ type ArtworkRepository interface {
 	GetImages(hashes []string) (map[string]Artwork, error)
 	// GetOrphanHashes returns hashes referenced by no item_artwork row and older than cutoff.
 	GetOrphanHashes(createdBefore time.Time) ([]string, error)
-	DeleteImages(hashes ...string) error
+	// DeleteOrphans deletes the given hashes only if still unreferenced and older than cutoff (atomic re-check).
+	DeleteOrphans(createdBefore time.Time, hashes []string) error
 	// Per-item state (item_artwork table)
 	GetItemArtwork(kind, id, imageType string) (*ItemArtwork, error)
 	PutItemArtwork(ia *ItemArtwork) error

persistence/artwork_repository.go

@@ -92,9 +92,14 @@ func (r *artworkRepository) GetOrphanHashes(createdBefore time.Time) ([]string,
 	return hashes, err
 }
 
-func (r *artworkRepository) DeleteImages(hashes ...string) error {
+func (r *artworkRepository) DeleteOrphans(createdBefore time.Time, hashes []string) error {
 	for chunk := range slices.Chunk(hashes, artworkBatchSize) {
-		if err := r.delete(Eq{"hash": chunk}); err != nil {
+		del := Delete(r.tableName).Where(And{
+			Eq{"hash": chunk},
+			Lt{"created_at": createdBefore},
+			Expr("hash NOT IN (SELECT hash FROM " + itemArtworkTable + " WHERE hash <> '')"),
+		})
+		if _, err := r.executeSQL(del); err != nil {
 			return err
 		}
 	}
@@ -116,6 +121,10 @@ func (r *artworkRepository) PutItemArtwork(ia *model.ItemArtwork) error {
 		ia.ImageType = model.ImageTypePrimary
 	}
 	ia.UpdatedAt = time.Now()
+	// PutItemArtwork records the outcome of an attempt, so an unset attempted_at is now.
+	if ia.AttemptedAt.IsZero() {
+		ia.AttemptedAt = ia.UpdatedAt
+	}
 	values, err := toSQLArgs(*ia)
 	if err != nil {
 		return err

persistence/artwork_repository_test.go

@@ -99,11 +99,25 @@ var _ = Describe("ArtworkRepository", func() {
 			Expect(orphans).To(BeEmpty())
 		})
 
-		It("deletes by hashes", func() {
+		It("deletes only unreferenced hashes older than the cutoff", func() {
 			Expect(repo.PutImage(&model.Artwork{Hash: "d1", Mime: "image/jpeg"})).To(Succeed())
-			Expect(repo.DeleteImages("d1")).To(Succeed())
+			Expect(repo.PutImage(&model.Artwork{Hash: "dref", Mime: "image/jpeg"})).To(Succeed())
+			Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: "a1",
+				ImageType: model.ImageTypePrimary, Hash: "dref", Source: "folder"})).To(Succeed())
+
+			Expect(repo.DeleteOrphans(time.Now().Add(time.Minute), []string{"d1", "dref"})).To(Succeed())
+
 			_, err := repo.GetImage("d1")
 			Expect(err).To(MatchError(model.ErrNotFound))
+			_, err = repo.GetImage("dref")
+			Expect(err).ToNot(HaveOccurred())
+		})
+
+		It("spares an unreferenced hash younger than the cutoff", func() {
+			Expect(repo.PutImage(&model.Artwork{Hash: "young", Mime: "image/jpeg"})).To(Succeed())
+			Expect(repo.DeleteOrphans(time.Now().Add(-time.Hour), []string{"young"})).To(Succeed())
+			_, err := repo.GetImage("young")
+			Expect(err).ToNot(HaveOccurred())
 		})
 
 		It("fetches a batch larger than the SQL variable limit", func() {
@@ -135,6 +149,15 @@ var _ = Describe("ArtworkRepository", func() {
 			Expect(got.UpdatedAt).ToNot(BeZero())
 		})
 
+		It("defaults attempted_at to now when unset", func() {
+			before := time.Now().Add(-time.Second)
+			Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "noattempt",
+				ImageType: model.ImageTypePrimary, Hash: ""})).To(Succeed())
+			got, err := repo.GetItemArtwork("ar", "noattempt", model.ImageTypePrimary)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(got.AttemptedAt).To(BeTemporally(">", before))
+		})
+
 		It("represents known-absent as empty hash", func() {
 			Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "ar1",
 				ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()})).To(Succeed())

tests/mock_artwork_queue_repo.go

@@ -28,19 +28,17 @@ func (m *MockArtworkQueueRepo) Enqueue(items ...model.ArtworkQueueItem) error {
 		if it.ImageType == "" {
 			it.ImageType = model.ImageTypePrimary
 		}
-		if it.RetryAt.IsZero() {
-			it.RetryAt = now
-		}
 		k := iaKey(it.ItemKind, it.ItemID, it.ImageType)
+		// Mirror the SQL: retry_at/enqueued_at are server-set, never taken from the caller.
 		if prev, ok := m.Data[k]; ok {
 			prev.Priority = max(prev.Priority, it.Priority)
-			prev.RetryAt = it.RetryAt
+			prev.RetryAt = now
 			m.Data[k] = prev
 			continue
 		}
-		if it.EnqueuedAt.IsZero() {
-			it.EnqueuedAt = now
-		}
+		it.Attempts = 0
+		it.RetryAt = now
+		it.EnqueuedAt = now
 		m.Data[k] = it
 	}
 	return nil

tests/mock_artwork_repo.go

@@ -69,16 +69,29 @@ func (m *MockArtworkRepo) GetAllHashes() ([]string, error) {
 	return hashes, nil
 }
 
-func (m *MockArtworkRepo) DeleteImages(hashes ...string) error {
+func (m *MockArtworkRepo) DeleteOrphans(createdBefore time.Time, hashes []string) error {
 	if m.Err != nil {
 		return m.Err
 	}
+	// Mirror the SQL reference re-check; createdBefore is ignored in the mock for simplicity.
 	for _, h := range hashes {
+		if m.referenced(h) {
+			continue
+		}
 		delete(m.Data, h)
 	}
 	return nil
 }
 
+func (m *MockArtworkRepo) referenced(hash string) bool {
+	for _, ia := range m.ItemData {
+		if ia.Hash == hash {
+			return true
+		}
+	}
+	return false
+}
+
 func (m *MockArtworkRepo) GetItemArtwork(kind, id, imageType string) (*model.ItemArtwork, error) {
 	if m.Err != nil {
 		return nil, m.Err
@@ -97,6 +110,9 @@ func (m *MockArtworkRepo) PutItemArtwork(ia *model.ItemArtwork) error {
 		ia.ImageType = model.ImageTypePrimary
 	}
 	ia.UpdatedAt = time.Now()
+	if ia.AttemptedAt.IsZero() {
+		ia.AttemptedAt = ia.UpdatedAt
+	}
 	m.ItemData[iaKey(ia.ItemKind, ia.ItemID, ia.ImageType)] = *ia
 	return nil
 }