diff --git a/core/artwork/prune.go b/core/artwork/prune.go index 2146e3778..1fd6197bc 100644 --- a/core/artwork/prune.go +++ b/core/artwork/prune.go @@ -35,36 +35,15 @@ func prune(ctx context.Context, ds model.DataStore, store *ImageStore) error { // Files younger than the grace window may belong to acquisitions whose rows aren't committed yet. cutoff := time.Now().Add(-pruneMinAge) - candidates, err := repo.GetOrphanHashes(cutoff) + orphans, err := repo.DeleteOrphans(cutoff) if err != nil { return err } - if len(candidates) > 0 { - arts, err := repo.GetImages(candidates) - if err != nil { - return err - } - if err := repo.DeleteOrphans(cutoff, candidates); err != nil { - return err - } - // DeleteOrphans may spare candidates reacquired since the snapshot; only drop files whose rows are gone. - survivors, err := repo.GetImages(candidates) - if err != nil { - return err - } - removed := 0 - for _, h := range candidates { - if _, ok := survivors[h]; ok { - continue - } - if err := store.Remove(h, arts[h].Mime, cutoff); err != nil { - log.Warn(ctx, "Artwork: Could not remove orphan file", "hash", h, err) - } - removed++ - } - log.Info(ctx, "Artwork: Removed orphan images", "count", removed) + if orphans > 0 { + log.Info(ctx, "Artwork: Removed orphan images", "count", orphans) } + // Read after the delete, so the sweep below reclaims the files of the rows just removed. mimes, err := repo.GetAllMimes() if err != nil { return err diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index e65d4b1de..0879b8949 100644 --- a/core/artwork/prune_test.go +++ b/core/artwork/prune_test.go @@ -82,7 +82,6 @@ var _ = Describe("Prune", func() { Expect(os.Chtimes(store.path(h, "image/jpeg"), old, old)).To(Succeed()) Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg"})).To(Succeed()) ageArtwork(h, old) - awRepo.OrphanHashes = []string{h} kept := []byte("kept-bytes") hk, _ := hashImage(bytes.NewReader(kept)) @@ -100,13 +99,12 @@ var _ = Describe("Prune", func() { rc.Close() }) - It("spares a candidate reacquired between snapshot and delete", func() { + It("spares an aged row that item_artwork state still references", 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"})).To(Succeed()) ageArtwork(h, time.Now().Add(-2*time.Hour)) - awRepo.OrphanHashes = []string{h} Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: "a1", ImageType: model.ImageTypePrimary, Hash: h, Source: "folder"})).To(Succeed()) @@ -119,13 +117,12 @@ var _ = Describe("Prune", func() { rc.Close() }) - It("spares a candidate whose row was freshly recreated (created_at inside the grace window)", func() { + It("spares an unreferenced row recreated inside the grace window", func() { data := []byte("fresh-reacquired-bytes") h, _ := hashImage(bytes.NewReader(data)) Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed()) - // Reacquisition refreshed created_at after the snapshot; still unreferenced. + // Reacquisition refreshed created_at, so the row is unreferenced but too young to drop. Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg"})).To(Succeed()) - awRepo.OrphanHashes = []string{h} Expect(prune(context.Background(), ds, store)).To(Succeed()) @@ -142,7 +139,6 @@ var _ = Describe("Prune", func() { Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed()) Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg"})).To(Succeed()) ageArtwork(h, time.Now().Add(-2*time.Hour)) - awRepo.OrphanHashes = []string{h} // The row is orphaned, but a concurrent acquisition just touched the file's mtime. Expect(prune(context.Background(), ds, store)).To(Succeed()) @@ -185,7 +181,7 @@ var _ = Describe("Prune", func() { rc.Close() }) - It("warns and continues past a store.Remove failure instead of aborting the loop", func() { + It("reclaims the other orphan files when one of them cannot be removed", func() { tests.SkipOnWindows("uses Unix file permission bits") if os.Geteuid() == 0 { Skip("read-only dir cannot block root (e.g. tests in a container)") @@ -211,9 +207,6 @@ var _ = Describe("Prune", func() { Expect(os.Chmod(shardDir, 0500)).To(Succeed()) DeferCleanup(func() { _ = os.Chmod(shardDir, 0755) }) - // hb is processed first: aborting on its Remove failure would leave hg unreached. - awRepo.OrphanHashes = []string{hb, hg} - Expect(prune(context.Background(), ds, store)).To(Succeed()) _, err := awRepo.GetImage(hg) diff --git a/model/artwork.go b/model/artwork.go index 4a0ed007b..8c11bf487 100644 --- a/model/artwork.go +++ b/model/artwork.go @@ -98,11 +98,8 @@ const ( type ArtworkRepository interface { GetImage(hash string) (*Artwork, error) PutImage(a *Artwork) error - 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) - // DeleteOrphans deletes the given hashes only if still unreferenced and older than cutoff. - DeleteOrphans(createdBefore time.Time, hashes []string) error + // DeleteOrphans deletes rows referenced by no item_artwork row and older than cutoff. + DeleteOrphans(createdBefore time.Time) (int64, error) GetItemArtwork(kind Kind, id, imageType string) (*ItemArtwork, error) PutItemArtwork(ia *ItemArtwork) error DeleteForItem(kind Kind, id string) error diff --git a/persistence/artwork_repository.go b/persistence/artwork_repository.go index a32a98be2..dae76c303 100644 --- a/persistence/artwork_repository.go +++ b/persistence/artwork_repository.go @@ -56,21 +56,6 @@ func (r *artworkRepository) PutImage(a *model.Artwork) error { return err } -func (r *artworkRepository) GetImages(hashes []string) (map[string]model.Artwork, error) { - res := map[string]model.Artwork{} - for chunk := range slices.Chunk(hashes, artworkBatchSize) { - sel := Select("*").From(r.tableName).Where(Eq{"hash": chunk}) - var all []model.Artwork - if err := r.queryAll(sel, &all); err != nil { - return nil, err - } - for _, a := range all { - res[a.Hash] = a - } - } - return res, nil -} - func (r *artworkRepository) GetAllMimes() (map[string]string, error) { sel := Select("hash", "mime").From(r.tableName) var rows []struct { @@ -87,29 +72,12 @@ func (r *artworkRepository) GetAllMimes() (map[string]string, error) { return res, nil } -func (r *artworkRepository) GetOrphanHashes(createdBefore time.Time) ([]string, error) { - sel := Select("hash").From(r.tableName). - Where(And{ - Lt{"created_at": createdBefore}, - Expr("hash NOT IN (SELECT hash FROM " + itemArtworkTable + " WHERE hash <> '')"), - }) - var hashes []string - err := r.queryAllSlice(sel, &hashes) - return hashes, err -} - -func (r *artworkRepository) DeleteOrphans(createdBefore time.Time, hashes []string) error { - for chunk := range slices.Chunk(hashes, artworkBatchSize) { - 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 - } - } - return nil +func (r *artworkRepository) DeleteOrphans(createdBefore time.Time) (int64, error) { + del := Delete(r.tableName).Where(And{ + Lt{"created_at": createdBefore}, + Expr("hash NOT IN (SELECT hash FROM " + itemArtworkTable + " WHERE hash <> '')"), + }) + return r.executeSQL(del) } // artworkOwnerTables maps an artwork kind to the table that owns the entity. diff --git a/persistence/artwork_repository_test.go b/persistence/artwork_repository_test.go index df8c10dfc..559276755 100644 --- a/persistence/artwork_repository_test.go +++ b/persistence/artwork_repository_test.go @@ -86,15 +86,6 @@ var _ = Describe("ArtworkRepository", func() { Expect(err).To(MatchError(model.ErrNotFound)) }) - It("fetches a batch", func() { - Expect(repo.PutImage(&model.Artwork{Hash: "b1", Mime: "image/jpeg"})).To(Succeed()) - Expect(repo.PutImage(&model.Artwork{Hash: "b2", Mime: "image/png"})).To(Succeed()) - got, err := repo.GetImages([]string{"b1", "b2", "missing"}) - Expect(err).ToNot(HaveOccurred()) - Expect(got).To(HaveLen(2)) - Expect(got["b2"].Mime).To(Equal("image/png")) - }) - 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()) @@ -104,28 +95,13 @@ var _ = Describe("ArtworkRepository", func() { Expect(mimes).To(HaveKeyWithValue("all2", "image/png")) }) - It("finds orphans older than cutoff, honoring item_artwork references", func() { - Expect(repo.PutImage(&model.Artwork{Hash: "orph1", Mime: "image/jpeg"})).To(Succeed()) - Expect(repo.PutImage(&model.Artwork{Hash: "ref1", Mime: "image/jpeg"})).To(Succeed()) - Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: "a1", ImageType: model.ImageTypePrimary, Hash: "ref1", Source: "folder"})).To(Succeed()) - - orphans, err := repo.GetOrphanHashes(time.Now().Add(time.Minute)) - Expect(err).ToNot(HaveOccurred()) - Expect(orphans).To(ContainElement("orph1")) - Expect(orphans).ToNot(ContainElement("ref1")) - - orphans, err = repo.GetOrphanHashes(time.Now().Add(-time.Hour)) - Expect(err).ToNot(HaveOccurred()) - Expect(orphans).To(BeEmpty()) - }) - - It("deletes only unreferenced hashes older than the cutoff", func() { + It("deletes only unreferenced rows older than the cutoff, reporting the count", func() { Expect(repo.PutImage(&model.Artwork{Hash: "d1", Mime: "image/jpeg"})).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()) + Expect(repo.DeleteOrphans(time.Now().Add(time.Minute))).To(BeNumerically("==", 1)) _, err := repo.GetImage("d1") Expect(err).To(MatchError(model.ErrNotFound)) @@ -133,26 +109,12 @@ var _ = Describe("ArtworkRepository", func() { Expect(err).ToNot(HaveOccurred()) }) - It("spares an unreferenced hash younger than the cutoff", func() { + It("spares an unreferenced row 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()) + Expect(repo.DeleteOrphans(time.Now().Add(-time.Hour))).To(BeNumerically("==", 0)) _, err := repo.GetImage("young") Expect(err).ToNot(HaveOccurred()) }) - - It("fetches a batch larger than the SQL variable limit", func() { - hashes := make([]string, 0, 250) - for i := range 250 { - h := fmt.Sprintf("big%03d", i) - Expect(repo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg"})).To(Succeed()) - hashes = append(hashes, h) - } - hashes = append(hashes, "absent1", "absent2") - - got, err := repo.GetImages(hashes) - Expect(err).ToNot(HaveOccurred()) - Expect(got).To(HaveLen(250)) - }) }) Context("dangling state cleanup", func() { diff --git a/tests/mock_artwork_repo.go b/tests/mock_artwork_repo.go index c8ebab403..606c107ed 100644 --- a/tests/mock_artwork_repo.go +++ b/tests/mock_artwork_repo.go @@ -12,11 +12,10 @@ import ( type MockArtworkRepo struct { model.ArtworkRepository // mu guards the maps so the worker's concurrent drain can hit this mock race-free. - mu sync.Mutex - Data map[string]model.Artwork - ItemData map[string]model.ItemArtwork // keyed by iaKey(kind, id, imageType) - OrphanHashes []string - Err error + mu sync.Mutex + Data map[string]model.Artwork + ItemData map[string]model.ItemArtwork // keyed by iaKey(kind, id, imageType) + Err error // ExistingIDs, keyed by item_kind, backs PurgeDanglingItemArtwork; nil map keeps everything. ExistingIDs map[string]map[string]bool } @@ -51,27 +50,6 @@ func (m *MockArtworkRepo) PutImage(a *model.Artwork) error { return nil } -func (m *MockArtworkRepo) GetImages(hashes []string) (map[string]model.Artwork, error) { - m.mu.Lock() - defer m.mu.Unlock() - if m.Err != nil { - return nil, m.Err - } - res := map[string]model.Artwork{} - for _, h := range hashes { - if a, ok := m.Data[h]; ok { - res[h] = a - } - } - return res, nil -} - -func (m *MockArtworkRepo) GetOrphanHashes(createdBefore time.Time) ([]string, error) { - m.mu.Lock() - defer m.mu.Unlock() - return m.OrphanHashes, m.Err -} - func (m *MockArtworkRepo) GetAllMimes() (map[string]string, error) { m.mu.Lock() defer m.mu.Unlock() @@ -106,23 +84,21 @@ func (m *MockArtworkRepo) PurgeDanglingItemArtwork() (int64, error) { return purged, nil } -func (m *MockArtworkRepo) DeleteOrphans(createdBefore time.Time, hashes []string) error { +func (m *MockArtworkRepo) DeleteOrphans(createdBefore time.Time) (int64, error) { m.mu.Lock() defer m.mu.Unlock() if m.Err != nil { - return m.Err + return 0, m.Err } - // Mirror the SQL re-check: only unreferenced rows older than the cutoff are deleted. - for _, h := range hashes { - if m.referenced(h) { - continue - } - if a, ok := m.Data[h]; ok && !a.CreatedAt.Before(createdBefore) { + var deleted int64 + for h, a := range m.Data { + if m.referenced(h) || !a.CreatedAt.Before(createdBefore) { continue } delete(m.Data, h) + deleted++ } - return nil + return deleted, nil } func (m *MockArtworkRepo) referenced(hash string) bool {