diff --git a/core/artwork/prune.go b/core/artwork/prune.go index b787dd1c9..de8811e14 100644 --- a/core/artwork/prune.go +++ b/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() diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index 0dd0b660f..aee8cd17c 100644 --- a/core/artwork/prune_test.go +++ b/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)) diff --git a/model/artwork.go b/model/artwork.go index 986a5c65d..5b50fbda0 100644 --- a/model/artwork.go +++ b/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 diff --git a/persistence/artwork_repository.go b/persistence/artwork_repository.go index 145e0b3f7..a18b9d4af 100644 --- a/persistence/artwork_repository.go +++ b/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 diff --git a/persistence/artwork_repository_test.go b/persistence/artwork_repository_test.go index 12456bcd1..b3971f88f 100644 --- a/persistence/artwork_repository_test.go +++ b/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()) diff --git a/tests/mock_artwork_queue_repo.go b/tests/mock_artwork_queue_repo.go index fceded1fb..f62afe43a 100644 --- a/tests/mock_artwork_queue_repo.go +++ b/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 diff --git a/tests/mock_artwork_repo.go b/tests/mock_artwork_repo.go index 8ac4820e6..6575416b9 100644 --- a/tests/mock_artwork_repo.go +++ b/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 }