nik/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-10-18 07:30:29 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(artwork): let the sweep reclaim orphan files instead of prune

Browse Source 
Deleting unreferenced artwork rows took five round trips: snapshot the orphan
hashes, fetch their mimes, delete, re-fetch to see which rows actually went, then
remove each file. The last four exist only to work out which files to delete -
but the sweep that runs moments later already derives exactly that from the
database, since GetAllMimes is read after the delete and Sweep removes any file
whose hash has no row, under the same mtime guard. The sweep is also the more
thorough of the two: it reclaims stale mime variants of an orphan hash, which the
per-hash removal never looked at.

Delete the rows in one predicate-only statement and let the sweep follow. The
orphan predicate now runs once per prune instead of once for the snapshot plus
once per 200-hash delete chunk, and with no hash list to bind there is nothing
left to chunk.

GetOrphanHashes and GetImages had no other caller and are gone. Dropping them
also removes the mock's OrphanHashes lever, which let a spec declare a hash
orphaned while item_artwork still referenced it - a state the SQL repository
could never produce, since both predicates were always identical.
This commit is contained in:
Deluan 2026-08-01 20:59:33 -04:00
parent 1344e6e660
commit 3be5fbd222
6 changed files with 31 additions and 156 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

29
core/artwork/prune.go
View File

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

15
core/artwork/prune_test.go
View File

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

7
model/artwork.go
View File

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

44
persistence/artwork_repository.go
View File

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

46
persistence/artwork_repository_test.go
View File

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

46
tests/mock_artwork_repo.go
View File

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