From 7aacb01f4f5abd05a9b8ee9ef0b3c10161a2d951 Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 21 Jul 2026 22:31:03 -0400 Subject: [PATCH 01/21] feat(artwork): add artwork, item_artwork and artwork_queue tables --- .../20260722023032_add_artwork_tables.sql | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 db/migrations/20260722023032_add_artwork_tables.sql diff --git a/db/migrations/20260722023032_add_artwork_tables.sql b/db/migrations/20260722023032_add_artwork_tables.sql new file mode 100644 index 000000000..94e0ebe5f --- /dev/null +++ b/db/migrations/20260722023032_add_artwork_tables.sql @@ -0,0 +1,41 @@ +-- +goose Up +CREATE TABLE artwork ( + hash TEXT PRIMARY KEY, + mime TEXT NOT NULL, + width INTEGER NOT NULL DEFAULT 0, + height INTEGER NOT NULL DEFAULT 0, + size_bytes INTEGER NOT NULL DEFAULT 0, + blur_hash TEXT NOT NULL DEFAULT '', + source_path TEXT NOT NULL DEFAULT '', + ref_mtime INTEGER NOT NULL DEFAULT 0, + created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP +); + +CREATE TABLE item_artwork ( + item_kind TEXT NOT NULL, + item_id TEXT NOT NULL, + image_type TEXT NOT NULL DEFAULT 'primary', + hash TEXT NOT NULL DEFAULT '', + source TEXT NOT NULL DEFAULT '', + attempted_at TIMESTAMP, + updated_at TIMESTAMP, + PRIMARY KEY (item_kind, item_id, image_type) +) WITHOUT ROWID; +CREATE INDEX ix_item_artwork_hash ON item_artwork(hash); + +CREATE TABLE artwork_queue ( + item_kind TEXT NOT NULL, + item_id TEXT NOT NULL, + image_type TEXT NOT NULL DEFAULT 'primary', + priority INTEGER NOT NULL DEFAULT 0, + attempts INTEGER NOT NULL DEFAULT 0, + retry_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, + enqueued_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, + PRIMARY KEY (item_kind, item_id, image_type) +) WITHOUT ROWID; +CREATE INDEX ix_artwork_queue_drain ON artwork_queue(retry_at, priority, enqueued_at); + +-- +goose Down +DROP TABLE artwork_queue; +DROP TABLE item_artwork; +DROP TABLE artwork; From 6cce65f7598f65e2437a3335f3021c813899ebda Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 21 Jul 2026 22:35:34 -0400 Subject: [PATCH 02/21] feat(artwork): add artwork models, repository interfaces and mocks --- model/artwork.go | 85 +++++++++++++++++++++++ model/datastore.go | 3 + persistence/artwork_queue_repository.go | 43 ++++++++++++ persistence/artwork_repository.go | 43 ++++++++++++ persistence/item_artwork_repository.go | 43 ++++++++++++ persistence/persistence.go | 12 ++++ tests/mock_artwork_queue_repo.go | 91 +++++++++++++++++++++++++ tests/mock_artwork_repo.go | 65 ++++++++++++++++++ tests/mock_data_store.go | 36 ++++++++++ tests/mock_item_artwork_repo.go | 67 ++++++++++++++++++ 10 files changed, 488 insertions(+) create mode 100644 model/artwork.go create mode 100644 persistence/artwork_queue_repository.go create mode 100644 persistence/artwork_repository.go create mode 100644 persistence/item_artwork_repository.go create mode 100644 tests/mock_artwork_queue_repo.go create mode 100644 tests/mock_artwork_repo.go create mode 100644 tests/mock_item_artwork_repo.go diff --git a/model/artwork.go b/model/artwork.go new file mode 100644 index 000000000..dd6a3f436 --- /dev/null +++ b/model/artwork.go @@ -0,0 +1,85 @@ +package model + +import "time" + +// Artwork is one unique image, identified by the XXH3-64 hash of its bytes. +type Artwork struct { + Hash string `structs:"hash"` + Mime string `structs:"mime"` + Width int `structs:"width"` + Height int `structs:"height"` + SizeBytes int64 `structs:"size_bytes"` + BlurHash string `structs:"blur_hash"` + SourcePath string `structs:"source_path"` + RefMtime int64 `structs:"ref_mtime"` + CreatedAt time.Time `structs:"created_at"` +} + +const ImageTypePrimary = "primary" + +// ItemArtwork is an entity's resolved artwork state. Hash=="" means known absent. +type ItemArtwork struct { + ItemKind string `structs:"item_kind"` + ItemID string `structs:"item_id"` + ImageType string `structs:"image_type"` + Hash string `structs:"hash"` + Source string `structs:"source"` + AttemptedAt time.Time `structs:"attempted_at"` + UpdatedAt time.Time `structs:"updated_at"` +} + +// ItemArtworkInfo is the list-hydration projection (item_artwork joined with artwork). +type ItemArtworkInfo struct { + ItemID string + Hash string + BlurHash string + Absent bool +} + +type ArtworkQueueItem struct { + ItemKind string `structs:"item_kind"` + ItemID string `structs:"item_id"` + ImageType string `structs:"image_type"` + Priority int `structs:"priority"` + Attempts int `structs:"attempts"` + RetryAt time.Time `structs:"retry_at"` + EnqueuedAt time.Time `structs:"enqueued_at"` +} + +// Queue priorities: higher drains first. +const ( + ArtworkPriorityRecheck = 0 + ArtworkPriorityBackfill = 10 + ArtworkPriorityScan = 50 + ArtworkPriorityBump = 100 +) + +type ArtworkRepository interface { + Get(hash string) (*Artwork, error) + Put(a *Artwork) error + GetBatch(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) + Delete(hashes ...string) error +} + +type ItemArtworkRepository interface { + Get(kind, id, imageType string) (*ItemArtwork, error) + Put(ia *ItemArtwork) error + DeleteForItem(kind, id string) error + // GetInfoForItems hydrates a page: one batched query, item_artwork joined to artwork. + GetInfoForItems(kind string, ids []string) (map[string]ItemArtworkInfo, error) + // EnqueueStaleAbsent inserts queue rows (priority Recheck) for absent states older than cutoff. + EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) +} + +type ArtworkQueueRepository interface { + // Enqueue upserts; an existing row keeps the higher of the two priorities. + Enqueue(items ...ArtworkQueueItem) error + // DequeueBatch returns up to n items with retry_at <= now, priority desc, enqueued_at asc. + DequeueBatch(n int) ([]ArtworkQueueItem, error) + // MarkFailed increments attempts and pushes retry_at into the future. + MarkFailed(kind, id, imageType string, retryAt time.Time) error + Delete(kind, id, imageType string) error + Count() (int64, error) +} diff --git a/model/datastore.go b/model/datastore.go index 94c3c3622..c72eb9a96 100644 --- a/model/datastore.go +++ b/model/datastore.go @@ -40,6 +40,9 @@ type DataStore interface { ScrobbleBuffer(ctx context.Context) ScrobbleBufferRepository Scrobble(ctx context.Context) ScrobbleRepository Plugin(ctx context.Context) PluginRepository + Artwork(ctx context.Context) ArtworkRepository + ItemArtwork(ctx context.Context) ItemArtworkRepository + ArtworkQueue(ctx context.Context) ArtworkQueueRepository Resource(ctx context.Context, model any) ResourceRepository diff --git a/persistence/artwork_queue_repository.go b/persistence/artwork_queue_repository.go new file mode 100644 index 000000000..4b166aa57 --- /dev/null +++ b/persistence/artwork_queue_repository.go @@ -0,0 +1,43 @@ +package persistence + +import ( + "context" + "time" + + "github.com/navidrome/navidrome/model" + "github.com/pocketbase/dbx" +) + +type artworkQueueRepository struct { + sqlRepository +} + +func NewArtworkQueueRepository(ctx context.Context, db dbx.Builder) model.ArtworkQueueRepository { + r := &artworkQueueRepository{} + r.ctx = ctx + r.db = db + r.tableName = "artwork_queue" + return r +} + +func (r *artworkQueueRepository) Enqueue(items ...model.ArtworkQueueItem) error { + return nil +} + +func (r *artworkQueueRepository) DequeueBatch(n int) ([]model.ArtworkQueueItem, error) { + return nil, nil +} + +func (r *artworkQueueRepository) MarkFailed(kind, id, imageType string, retryAt time.Time) error { + return nil +} + +func (r *artworkQueueRepository) Delete(kind, id, imageType string) error { + return nil +} + +func (r *artworkQueueRepository) Count() (int64, error) { + return 0, nil +} + +var _ model.ArtworkQueueRepository = (*artworkQueueRepository)(nil) diff --git a/persistence/artwork_repository.go b/persistence/artwork_repository.go new file mode 100644 index 000000000..4db47741b --- /dev/null +++ b/persistence/artwork_repository.go @@ -0,0 +1,43 @@ +package persistence + +import ( + "context" + "time" + + "github.com/navidrome/navidrome/model" + "github.com/pocketbase/dbx" +) + +type artworkRepository struct { + sqlRepository +} + +func NewArtworkRepository(ctx context.Context, db dbx.Builder) model.ArtworkRepository { + r := &artworkRepository{} + r.ctx = ctx + r.db = db + r.tableName = "artwork" + return r +} + +func (r *artworkRepository) Get(hash string) (*model.Artwork, error) { + return nil, model.ErrNotFound +} + +func (r *artworkRepository) Put(a *model.Artwork) error { + return nil +} + +func (r *artworkRepository) GetBatch(hashes []string) (map[string]model.Artwork, error) { + return nil, nil +} + +func (r *artworkRepository) GetOrphanHashes(createdBefore time.Time) ([]string, error) { + return nil, nil +} + +func (r *artworkRepository) Delete(hashes ...string) error { + return nil +} + +var _ model.ArtworkRepository = (*artworkRepository)(nil) diff --git a/persistence/item_artwork_repository.go b/persistence/item_artwork_repository.go new file mode 100644 index 000000000..8363264ca --- /dev/null +++ b/persistence/item_artwork_repository.go @@ -0,0 +1,43 @@ +package persistence + +import ( + "context" + "time" + + "github.com/navidrome/navidrome/model" + "github.com/pocketbase/dbx" +) + +type itemArtworkRepository struct { + sqlRepository +} + +func NewItemArtworkRepository(ctx context.Context, db dbx.Builder) model.ItemArtworkRepository { + r := &itemArtworkRepository{} + r.ctx = ctx + r.db = db + r.tableName = "item_artwork" + return r +} + +func (r *itemArtworkRepository) Get(kind, id, imageType string) (*model.ItemArtwork, error) { + return nil, model.ErrNotFound +} + +func (r *itemArtworkRepository) Put(ia *model.ItemArtwork) error { + return nil +} + +func (r *itemArtworkRepository) DeleteForItem(kind, id string) error { + return nil +} + +func (r *itemArtworkRepository) GetInfoForItems(kind string, ids []string) (map[string]model.ItemArtworkInfo, error) { + return nil, nil +} + +func (r *itemArtworkRepository) EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) { + return 0, nil +} + +var _ model.ItemArtworkRepository = (*itemArtworkRepository)(nil) diff --git a/persistence/persistence.go b/persistence/persistence.go index 93f0e3e71..ed6d29865 100644 --- a/persistence/persistence.go +++ b/persistence/persistence.go @@ -97,6 +97,18 @@ func (s *SQLStore) Plugin(ctx context.Context) model.PluginRepository { return NewPluginRepository(ctx, s.getDBXBuilder()) } +func (s *SQLStore) Artwork(ctx context.Context) model.ArtworkRepository { + return NewArtworkRepository(ctx, s.getDBXBuilder()) +} + +func (s *SQLStore) ItemArtwork(ctx context.Context) model.ItemArtworkRepository { + return NewItemArtworkRepository(ctx, s.getDBXBuilder()) +} + +func (s *SQLStore) ArtworkQueue(ctx context.Context) model.ArtworkQueueRepository { + return NewArtworkQueueRepository(ctx, s.getDBXBuilder()) +} + func (s *SQLStore) Resource(ctx context.Context, m any) model.ResourceRepository { switch m.(type) { case model.User: diff --git a/tests/mock_artwork_queue_repo.go b/tests/mock_artwork_queue_repo.go new file mode 100644 index 000000000..b08a1297c --- /dev/null +++ b/tests/mock_artwork_queue_repo.go @@ -0,0 +1,91 @@ +package tests + +import ( + "sort" + "time" + + "github.com/navidrome/navidrome/model" +) + +type MockArtworkQueueRepo struct { + model.ArtworkQueueRepository + Data map[string]model.ArtworkQueueItem // key: kind + "|" + id + "|" + imageType + Err error +} + +func CreateMockArtworkQueueRepo() *MockArtworkQueueRepo { + return &MockArtworkQueueRepo{Data: map[string]model.ArtworkQueueItem{}} +} + +func (m *MockArtworkQueueRepo) Enqueue(items ...model.ArtworkQueueItem) error { + if m.Err != nil { + return m.Err + } + for _, it := range items { + k := iaKey(it.ItemKind, it.ItemID, it.ImageType) + if prev, ok := m.Data[k]; ok && prev.Priority > it.Priority { + it.Priority = prev.Priority + } + if it.EnqueuedAt.IsZero() { + it.EnqueuedAt = time.Now() + } + if it.RetryAt.IsZero() { + it.RetryAt = time.Now() + } + m.Data[k] = it + } + return nil +} + +func (m *MockArtworkQueueRepo) DequeueBatch(n int) ([]model.ArtworkQueueItem, error) { + if m.Err != nil { + return nil, m.Err + } + var res []model.ArtworkQueueItem + now := time.Now() + for _, it := range m.Data { + if !it.RetryAt.After(now) { + res = append(res, it) + } + } + sort.Slice(res, func(i, j int) bool { + if res[i].Priority != res[j].Priority { + return res[i].Priority > res[j].Priority + } + return res[i].EnqueuedAt.Before(res[j].EnqueuedAt) + }) + if len(res) > n { + res = res[:n] + } + return res, nil +} + +func (m *MockArtworkQueueRepo) MarkFailed(kind, id, imageType string, retryAt time.Time) error { + if m.Err != nil { + return m.Err + } + k := iaKey(kind, id, imageType) + it, ok := m.Data[k] + if !ok { + return model.ErrNotFound + } + it.Attempts++ + it.RetryAt = retryAt + m.Data[k] = it + return nil +} + +func (m *MockArtworkQueueRepo) Delete(kind, id, imageType string) error { + if m.Err != nil { + return m.Err + } + delete(m.Data, iaKey(kind, id, imageType)) + return nil +} + +func (m *MockArtworkQueueRepo) Count() (int64, error) { + if m.Err != nil { + return 0, m.Err + } + return int64(len(m.Data)), nil +} diff --git a/tests/mock_artwork_repo.go b/tests/mock_artwork_repo.go new file mode 100644 index 000000000..c8b95a025 --- /dev/null +++ b/tests/mock_artwork_repo.go @@ -0,0 +1,65 @@ +package tests + +import ( + "time" + + "github.com/navidrome/navidrome/model" +) + +type MockArtworkRepo struct { + model.ArtworkRepository + Data map[string]model.Artwork + Err error +} + +func CreateMockArtworkRepo() *MockArtworkRepo { + return &MockArtworkRepo{Data: map[string]model.Artwork{}} +} + +func (m *MockArtworkRepo) Get(hash string) (*model.Artwork, error) { + if m.Err != nil { + return nil, m.Err + } + if a, ok := m.Data[hash]; ok { + return &a, nil + } + return nil, model.ErrNotFound +} + +func (m *MockArtworkRepo) Put(a *model.Artwork) error { + if m.Err != nil { + return m.Err + } + if a.CreatedAt.IsZero() { + a.CreatedAt = time.Now() + } + m.Data[a.Hash] = *a + return nil +} + +func (m *MockArtworkRepo) GetBatch(hashes []string) (map[string]model.Artwork, error) { + 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) { + return nil, m.Err +} + +func (m *MockArtworkRepo) Delete(hashes ...string) error { + if m.Err != nil { + return m.Err + } + for _, h := range hashes { + delete(m.Data, h) + } + return nil +} diff --git a/tests/mock_data_store.go b/tests/mock_data_store.go index e016a28de..345a376e6 100644 --- a/tests/mock_data_store.go +++ b/tests/mock_data_store.go @@ -28,6 +28,9 @@ type MockDataStore struct { MockedScrobble model.ScrobbleRepository MockedRadio model.RadioRepository MockedPlugin model.PluginRepository + MockedArtwork model.ArtworkRepository + MockedItemArtwork model.ItemArtworkRepository + MockedArtworkQueue model.ArtworkQueueRepository scrobbleBufferMu sync.Mutex repoMu sync.Mutex @@ -247,6 +250,39 @@ func (db *MockDataStore) Plugin(ctx context.Context) model.PluginRepository { return db.MockedPlugin } +func (db *MockDataStore) Artwork(ctx context.Context) model.ArtworkRepository { + if db.MockedArtwork != nil { + return db.MockedArtwork + } + if db.RealDS != nil { + return db.RealDS.Artwork(ctx) + } + db.MockedArtwork = CreateMockArtworkRepo() + return db.MockedArtwork +} + +func (db *MockDataStore) ItemArtwork(ctx context.Context) model.ItemArtworkRepository { + if db.MockedItemArtwork != nil { + return db.MockedItemArtwork + } + if db.RealDS != nil { + return db.RealDS.ItemArtwork(ctx) + } + db.MockedItemArtwork = CreateMockItemArtworkRepo() + return db.MockedItemArtwork +} + +func (db *MockDataStore) ArtworkQueue(ctx context.Context) model.ArtworkQueueRepository { + if db.MockedArtworkQueue != nil { + return db.MockedArtworkQueue + } + if db.RealDS != nil { + return db.RealDS.ArtworkQueue(ctx) + } + db.MockedArtworkQueue = CreateMockArtworkQueueRepo() + return db.MockedArtworkQueue +} + func (db *MockDataStore) WithTx(block func(tx model.DataStore) error, label ...string) error { return block(db) } diff --git a/tests/mock_item_artwork_repo.go b/tests/mock_item_artwork_repo.go new file mode 100644 index 000000000..7d9cfbd9e --- /dev/null +++ b/tests/mock_item_artwork_repo.go @@ -0,0 +1,67 @@ +package tests + +import ( + "time" + + "github.com/navidrome/navidrome/model" +) + +type MockItemArtworkRepo struct { + model.ItemArtworkRepository + Data map[string]model.ItemArtwork // key: kind + "|" + id + "|" + imageType + Err error +} + +func CreateMockItemArtworkRepo() *MockItemArtworkRepo { + return &MockItemArtworkRepo{Data: map[string]model.ItemArtwork{}} +} + +func iaKey(kind, id, imageType string) string { return kind + "|" + id + "|" + imageType } + +func (m *MockItemArtworkRepo) Get(kind, id, imageType string) (*model.ItemArtwork, error) { + if m.Err != nil { + return nil, m.Err + } + if ia, ok := m.Data[iaKey(kind, id, imageType)]; ok { + return &ia, nil + } + return nil, model.ErrNotFound +} + +func (m *MockItemArtworkRepo) Put(ia *model.ItemArtwork) error { + if m.Err != nil { + return m.Err + } + ia.UpdatedAt = time.Now() + m.Data[iaKey(ia.ItemKind, ia.ItemID, ia.ImageType)] = *ia + return nil +} + +func (m *MockItemArtworkRepo) DeleteForItem(kind, id string) error { + if m.Err != nil { + return m.Err + } + for k, ia := range m.Data { + if ia.ItemKind == kind && ia.ItemID == id { + delete(m.Data, k) + } + } + return nil +} + +func (m *MockItemArtworkRepo) GetInfoForItems(kind string, ids []string) (map[string]model.ItemArtworkInfo, error) { + if m.Err != nil { + return nil, m.Err + } + res := map[string]model.ItemArtworkInfo{} + for _, id := range ids { + if ia, ok := m.Data[iaKey(kind, id, model.ImageTypePrimary)]; ok { + res[id] = model.ItemArtworkInfo{ItemID: id, Hash: ia.Hash, Absent: ia.Hash == ""} + } + } + return res, nil +} + +func (m *MockItemArtworkRepo) EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) { + return 0, m.Err +} From f926539c044a87219e0c6be915ea4d2c2365f8b1 Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 21 Jul 2026 22:44:24 -0400 Subject: [PATCH 03/21] feat(artwork): implement artwork repository --- persistence/artwork_repository.go | 49 +++++++++++++-- persistence/artwork_repository_test.go | 85 ++++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 5 deletions(-) create mode 100644 persistence/artwork_repository_test.go diff --git a/persistence/artwork_repository.go b/persistence/artwork_repository.go index 4db47741b..a49f30597 100644 --- a/persistence/artwork_repository.go +++ b/persistence/artwork_repository.go @@ -4,6 +4,7 @@ import ( "context" "time" + . "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/model" "github.com/pocketbase/dbx" ) @@ -21,23 +22,61 @@ func NewArtworkRepository(ctx context.Context, db dbx.Builder) model.ArtworkRepo } func (r *artworkRepository) Get(hash string) (*model.Artwork, error) { - return nil, model.ErrNotFound + sel := Select("*").From(r.tableName).Where(Eq{"hash": hash}) + var res model.Artwork + if err := r.queryOne(sel, &res); err != nil { + return nil, err + } + return &res, nil } func (r *artworkRepository) Put(a *model.Artwork) error { - return nil + if a.CreatedAt.IsZero() { + a.CreatedAt = time.Now() + } + ins := Insert(r.tableName).SetMap(map[string]any{ + "hash": a.Hash, "mime": a.Mime, "width": a.Width, "height": a.Height, + "size_bytes": a.SizeBytes, "blur_hash": a.BlurHash, + "source_path": a.SourcePath, "ref_mtime": a.RefMtime, "created_at": a.CreatedAt, + }).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`) + _, err := r.executeSQL(ins) + return err } func (r *artworkRepository) GetBatch(hashes []string) (map[string]model.Artwork, error) { - return nil, nil + res := map[string]model.Artwork{} + if len(hashes) == 0 { + return res, nil + } + sel := Select("*").From(r.tableName).Where(Eq{"hash": hashes}) + 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) GetOrphanHashes(createdBefore time.Time) ([]string, error) { - return nil, nil + sel := Select("hash").From(r.tableName). + Where(And{ + Lt{"created_at": createdBefore}, + Expr("hash NOT IN (SELECT hash FROM item_artwork WHERE hash <> '')"), + }) + var hashes []string + err := r.queryAllSlice(sel, &hashes) + return hashes, err } func (r *artworkRepository) Delete(hashes ...string) error { - return nil + if len(hashes) == 0 { + return nil + } + return r.delete(Eq{"hash": hashes}) } var _ model.ArtworkRepository = (*artworkRepository)(nil) diff --git a/persistence/artwork_repository_test.go b/persistence/artwork_repository_test.go new file mode 100644 index 000000000..e158754a0 --- /dev/null +++ b/persistence/artwork_repository_test.go @@ -0,0 +1,85 @@ +package persistence + +import ( + "context" + "time" + + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// clearArtworkTables resets the shared test DB's artwork tables so specs don't leak state. +func clearArtworkTables() { + db := GetDBXBuilder() + for _, t := range []string{"artwork_queue", "item_artwork", "artwork"} { + _, err := db.NewQuery("DELETE FROM " + t).Execute() + Expect(err).ToNot(HaveOccurred()) + } +} + +var _ = Describe("ArtworkRepository", func() { + var repo model.ArtworkRepository + + BeforeEach(func() { + clearArtworkTables() + repo = NewArtworkRepository(context.Background(), GetDBXBuilder()) + }) + + It("stores and retrieves an artwork by hash", func() { + a := &model.Artwork{Hash: "abc123", Mime: "image/jpeg", Width: 500, Height: 500, SizeBytes: 1234, BlurHash: "LKO2?U%2Tw=w"} + Expect(repo.Put(a)).To(Succeed()) + + got, err := repo.Get("abc123") + Expect(err).ToNot(HaveOccurred()) + Expect(got.Mime).To(Equal("image/jpeg")) + Expect(got.BlurHash).To(Equal("LKO2?U%2Tw=w")) + Expect(got.CreatedAt).ToNot(BeZero()) + }) + + It("is idempotent on Put (upsert by hash)", func() { + a := &model.Artwork{Hash: "dup1", Mime: "image/png"} + Expect(repo.Put(a)).To(Succeed()) + a.BlurHash = "XYZ" + Expect(repo.Put(a)).To(Succeed()) + got, _ := repo.Get("dup1") + Expect(got.BlurHash).To(Equal("XYZ")) + }) + + It("returns ErrNotFound for a missing hash", func() { + _, err := repo.Get("nope") + Expect(err).To(MatchError(model.ErrNotFound)) + }) + + It("fetches a batch", func() { + Expect(repo.Put(&model.Artwork{Hash: "b1", Mime: "image/jpeg"})).To(Succeed()) + Expect(repo.Put(&model.Artwork{Hash: "b2", Mime: "image/png"})).To(Succeed()) + got, err := repo.GetBatch([]string{"b1", "b2", "missing"}) + Expect(err).ToNot(HaveOccurred()) + Expect(got).To(HaveLen(2)) + Expect(got["b2"].Mime).To(Equal("image/png")) + }) + + It("finds orphans older than cutoff, honoring item_artwork references", func() { + Expect(repo.Put(&model.Artwork{Hash: "orph1", Mime: "image/jpeg"})).To(Succeed()) + Expect(repo.Put(&model.Artwork{Hash: "ref1", Mime: "image/jpeg"})).To(Succeed()) + iaRepo := NewItemArtworkRepository(context.Background(), GetDBXBuilder()) + Expect(iaRepo.Put(&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 by hashes", func() { + Expect(repo.Put(&model.Artwork{Hash: "d1", Mime: "image/jpeg"})).To(Succeed()) + Expect(repo.Delete("d1")).To(Succeed()) + _, err := repo.Get("d1") + Expect(err).To(MatchError(model.ErrNotFound)) + }) +}) From 14dd57052ede5a55a493ff9b79d453c48b172218 Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 21 Jul 2026 22:45:03 -0400 Subject: [PATCH 04/21] feat(artwork): implement item_artwork repository with batched hydration --- persistence/item_artwork_repository.go | 60 +++++++++++++-- persistence/item_artwork_repository_test.go | 83 +++++++++++++++++++++ 2 files changed, 138 insertions(+), 5 deletions(-) create mode 100644 persistence/item_artwork_repository_test.go diff --git a/persistence/item_artwork_repository.go b/persistence/item_artwork_repository.go index 8363264ca..08b4caf3c 100644 --- a/persistence/item_artwork_repository.go +++ b/persistence/item_artwork_repository.go @@ -4,6 +4,7 @@ import ( "context" "time" + . "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/model" "github.com/pocketbase/dbx" ) @@ -21,23 +22,72 @@ func NewItemArtworkRepository(ctx context.Context, db dbx.Builder) model.ItemArt } func (r *itemArtworkRepository) Get(kind, id, imageType string) (*model.ItemArtwork, error) { - return nil, model.ErrNotFound + sel := Select("*").From(r.tableName). + Where(Eq{"item_kind": kind, "item_id": id, "image_type": imageType}) + var res model.ItemArtwork + if err := r.queryOne(sel, &res); err != nil { + return nil, err + } + return &res, nil } func (r *itemArtworkRepository) Put(ia *model.ItemArtwork) error { - return nil + if ia.ImageType == "" { + ia.ImageType = model.ImageTypePrimary + } + ia.UpdatedAt = time.Now() + ins := Insert(r.tableName).SetMap(map[string]any{ + "item_kind": ia.ItemKind, "item_id": ia.ItemID, "image_type": ia.ImageType, + "hash": ia.Hash, "source": ia.Source, + "attempted_at": ia.AttemptedAt, "updated_at": ia.UpdatedAt, + }).Suffix(`ON CONFLICT (item_kind, item_id, image_type) DO UPDATE SET + hash=excluded.hash, source=excluded.source, + attempted_at=excluded.attempted_at, updated_at=excluded.updated_at`) + _, err := r.executeSQL(ins) + return err } func (r *itemArtworkRepository) DeleteForItem(kind, id string) error { - return nil + return r.delete(Eq{"item_kind": kind, "item_id": id}) } func (r *itemArtworkRepository) GetInfoForItems(kind string, ids []string) (map[string]model.ItemArtworkInfo, error) { - return nil, nil + res := map[string]model.ItemArtworkInfo{} + if len(ids) == 0 { + return res, nil + } + sel := Select("ia.item_id", "ia.hash", "COALESCE(a.blur_hash, '') as blur_hash"). + From(r.tableName + " ia"). + LeftJoin("artwork a ON a.hash = ia.hash"). + Where(And{ + Eq{"ia.item_kind": kind}, + Eq{"ia.image_type": model.ImageTypePrimary}, + Eq{"ia.item_id": ids}, + }) + var rows []struct { + ItemID string + Hash string + BlurHash string + } + if err := r.queryAll(sel, &rows); err != nil { + return nil, err + } + for _, row := range rows { + res[row.ItemID] = model.ItemArtworkInfo{ + ItemID: row.ItemID, Hash: row.Hash, BlurHash: row.BlurHash, Absent: row.Hash == "", + } + } + return res, nil } func (r *itemArtworkRepository) EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) { - return 0, nil + now := time.Now() + ins := Expr(`INSERT INTO artwork_queue (item_kind, item_id, image_type, priority, attempts, retry_at, enqueued_at) + SELECT item_kind, item_id, image_type, ?, 0, ?, ? + FROM item_artwork WHERE item_kind = ? AND hash = '' AND attempted_at < ? + ON CONFLICT (item_kind, item_id, image_type) DO NOTHING`, + model.ArtworkPriorityRecheck, now, now, kind, attemptedBefore) + return r.executeSQL(ins) } var _ model.ItemArtworkRepository = (*itemArtworkRepository)(nil) diff --git a/persistence/item_artwork_repository_test.go b/persistence/item_artwork_repository_test.go new file mode 100644 index 000000000..6aec55d89 --- /dev/null +++ b/persistence/item_artwork_repository_test.go @@ -0,0 +1,83 @@ +package persistence + +import ( + "context" + "time" + + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("ItemArtworkRepository", func() { + var repo model.ItemArtworkRepository + var awRepo model.ArtworkRepository + + BeforeEach(func() { + clearArtworkTables() + repo = NewItemArtworkRepository(context.Background(), GetDBXBuilder()) + awRepo = NewArtworkRepository(context.Background(), GetDBXBuilder()) + }) + + It("upserts and reads state", func() { + ia := &model.ItemArtwork{ItemKind: "al", ItemID: "al1", ImageType: model.ImageTypePrimary, + Hash: "h1", Source: "folder", AttemptedAt: time.Now()} + Expect(repo.Put(ia)).To(Succeed()) + ia.Source = "embedded" + Expect(repo.Put(ia)).To(Succeed()) + + got, err := repo.Get("al", "al1", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + Expect(got.Source).To(Equal("embedded")) + Expect(got.UpdatedAt).ToNot(BeZero()) + }) + + It("represents known-absent as empty hash", func() { + Expect(repo.Put(&model.ItemArtwork{ItemKind: "ar", ItemID: "ar1", + ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()})).To(Succeed()) + got, err := repo.Get("ar", "ar1", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + Expect(got.Hash).To(BeEmpty()) + }) + + It("hydrates a page in one batch, including blurhash and absence", func() { + Expect(awRepo.Put(&model.Artwork{Hash: "h9", Mime: "image/jpeg", BlurHash: "BH9"})).To(Succeed()) + Expect(repo.Put(&model.ItemArtwork{ItemKind: "al", ItemID: "x1", ImageType: model.ImageTypePrimary, Hash: "h9", Source: "folder"})).To(Succeed()) + Expect(repo.Put(&model.ItemArtwork{ItemKind: "al", ItemID: "x2", ImageType: model.ImageTypePrimary, Hash: "", Source: ""})).To(Succeed()) + + info, err := repo.GetInfoForItems("al", []string{"x1", "x2", "x3"}) + Expect(err).ToNot(HaveOccurred()) + Expect(info).To(HaveLen(2)) + Expect(info["x1"].Hash).To(Equal("h9")) + Expect(info["x1"].BlurHash).To(Equal("BH9")) + Expect(info["x1"].Absent).To(BeFalse()) + Expect(info["x2"].Absent).To(BeTrue()) + _, unresolved := info["x3"] + Expect(unresolved).To(BeFalse()) + }) + + It("deletes all rows for an item", func() { + Expect(repo.Put(&model.ItemArtwork{ItemKind: "pl", ItemID: "p1", ImageType: model.ImageTypePrimary, Hash: "h1"})).To(Succeed()) + Expect(repo.DeleteForItem("pl", "p1")).To(Succeed()) + _, err := repo.Get("pl", "p1", model.ImageTypePrimary) + Expect(err).To(MatchError(model.ErrNotFound)) + }) + + It("enqueues stale absent states for recheck", func() { + old := time.Now().Add(-48 * time.Hour) + Expect(repo.Put(&model.ItemArtwork{ItemKind: "ar", ItemID: "stale1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old})).To(Succeed()) + Expect(repo.Put(&model.ItemArtwork{ItemKind: "ar", ItemID: "fresh1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()})).To(Succeed()) + Expect(repo.Put(&model.ItemArtwork{ItemKind: "ar", ItemID: "found1", ImageType: model.ImageTypePrimary, Hash: "hX", AttemptedAt: old})).To(Succeed()) + + n, err := repo.EnqueueStaleAbsent("ar", time.Now().Add(-24*time.Hour)) + Expect(err).ToNot(HaveOccurred()) + Expect(n).To(Equal(int64(1))) + + qRepo := NewArtworkQueueRepository(context.Background(), GetDBXBuilder()) + items, err := qRepo.DequeueBatch(10) + Expect(err).ToNot(HaveOccurred()) + Expect(items).To(HaveLen(1)) + Expect(items[0].ItemID).To(Equal("stale1")) + Expect(items[0].Priority).To(Equal(model.ArtworkPriorityRecheck)) + }) +}) From fcff9c63e787012badd95232754c22ac55612cab Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 21 Jul 2026 22:45:08 -0400 Subject: [PATCH 05/21] feat(artwork): implement artwork_queue repository --- persistence/artwork_queue_repository.go | 45 ++++++++++++-- persistence/artwork_queue_repository_test.go | 65 ++++++++++++++++++++ 2 files changed, 106 insertions(+), 4 deletions(-) create mode 100644 persistence/artwork_queue_repository_test.go diff --git a/persistence/artwork_queue_repository.go b/persistence/artwork_queue_repository.go index 4b166aa57..fdf88db24 100644 --- a/persistence/artwork_queue_repository.go +++ b/persistence/artwork_queue_repository.go @@ -4,6 +4,7 @@ import ( "context" "time" + . "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/model" "github.com/pocketbase/dbx" ) @@ -21,23 +22,59 @@ func NewArtworkQueueRepository(ctx context.Context, db dbx.Builder) model.Artwor } func (r *artworkQueueRepository) Enqueue(items ...model.ArtworkQueueItem) error { + now := time.Now() + for _, it := range items { + if it.ImageType == "" { + it.ImageType = model.ImageTypePrimary + } + ins := Insert(r.tableName).SetMap(map[string]any{ + "item_kind": it.ItemKind, "item_id": it.ItemID, "image_type": it.ImageType, + "priority": it.Priority, "attempts": 0, "retry_at": now, "enqueued_at": now, + }).Suffix(`ON CONFLICT (item_kind, item_id, image_type) DO UPDATE SET + priority = MAX(priority, excluded.priority), retry_at = excluded.retry_at`) + if _, err := r.executeSQL(ins); err != nil { + return err + } + } return nil } func (r *artworkQueueRepository) DequeueBatch(n int) ([]model.ArtworkQueueItem, error) { - return nil, nil + sel := Select("*").From(r.tableName). + Where(LtOrEq{"retry_at": time.Now()}). + OrderBy("priority DESC", "enqueued_at ASC"). + Limit(uint64(n)) + var res []model.ArtworkQueueItem + err := r.queryAll(sel, &res) + return res, err } func (r *artworkQueueRepository) MarkFailed(kind, id, imageType string, retryAt time.Time) error { - return nil + upd := Update(r.tableName). + Set("attempts", Expr("attempts + 1")). + Set("retry_at", retryAt). + Where(Eq{"item_kind": kind, "item_id": id, "image_type": imageType}) + c, err := r.executeSQL(upd) + if err == nil && c == 0 { + return model.ErrNotFound + } + return err } func (r *artworkQueueRepository) Delete(kind, id, imageType string) error { - return nil + return r.delete(Eq{"item_kind": kind, "item_id": id, "image_type": imageType}) } func (r *artworkQueueRepository) Count() (int64, error) { - return 0, nil + sel := Select("count(*)").From(r.tableName) + var counts []int64 + if err := r.queryAllSlice(sel, &counts); err != nil { + return 0, err + } + if len(counts) == 0 { + return 0, nil + } + return counts[0], nil } var _ model.ArtworkQueueRepository = (*artworkQueueRepository)(nil) diff --git a/persistence/artwork_queue_repository_test.go b/persistence/artwork_queue_repository_test.go new file mode 100644 index 000000000..73665b07c --- /dev/null +++ b/persistence/artwork_queue_repository_test.go @@ -0,0 +1,65 @@ +package persistence + +import ( + "context" + "time" + + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("ArtworkQueueRepository", func() { + var repo model.ArtworkQueueRepository + + item := func(kind, id string, prio int) model.ArtworkQueueItem { + return model.ArtworkQueueItem{ItemKind: kind, ItemID: id, + ImageType: model.ImageTypePrimary, Priority: prio} + } + + BeforeEach(func() { + clearArtworkTables() + repo = NewArtworkQueueRepository(context.Background(), GetDBXBuilder()) + }) + + It("enqueues and dequeues by priority then FIFO", func() { + Expect(repo.Enqueue(item("al", "low", model.ArtworkPriorityBackfill))).To(Succeed()) + Expect(repo.Enqueue(item("ar", "high", model.ArtworkPriorityBump))).To(Succeed()) + + got, err := repo.DequeueBatch(10) + Expect(err).ToNot(HaveOccurred()) + Expect(got).To(HaveLen(2)) + Expect(got[0].ItemID).To(Equal("high")) + }) + + It("keeps the higher priority on duplicate enqueue", func() { + Expect(repo.Enqueue(item("al", "a1", model.ArtworkPriorityBump))).To(Succeed()) + Expect(repo.Enqueue(item("al", "a1", model.ArtworkPriorityBackfill))).To(Succeed()) + got, _ := repo.DequeueBatch(10) + Expect(got).To(HaveLen(1)) + Expect(got[0].Priority).To(Equal(model.ArtworkPriorityBump)) + }) + + It("hides failed items until retry_at", func() { + Expect(repo.Enqueue(item("al", "f1", model.ArtworkPriorityScan))).To(Succeed()) + Expect(repo.MarkFailed("al", "f1", model.ImageTypePrimary, time.Now().Add(time.Hour))).To(Succeed()) + + got, err := repo.DequeueBatch(10) + Expect(err).ToNot(HaveOccurred()) + Expect(got).To(BeEmpty()) + + Expect(repo.MarkFailed("al", "f1", model.ImageTypePrimary, time.Now().Add(-time.Minute))).To(Succeed()) + got, _ = repo.DequeueBatch(10) + Expect(got).To(HaveLen(1)) + Expect(got[0].Attempts).To(Equal(2)) + }) + + It("deletes on completion and counts", func() { + Expect(repo.Enqueue(item("al", "c1", 0))).To(Succeed()) + n, _ := repo.Count() + Expect(n).To(Equal(int64(1))) + Expect(repo.Delete("al", "c1", model.ImageTypePrimary)).To(Succeed()) + n, _ = repo.Count() + Expect(n).To(BeZero()) + }) +}) From b72597821a0bdbf2624985196bf16c516ed09a2b Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 21 Jul 2026 22:53:00 -0400 Subject: [PATCH 06/21] feat(artwork): add content-addressed originals store --- core/artwork/originals/originals.go | 112 ++++++++++++++++++ .../artwork/originals/originals_suite_test.go | 17 +++ core/artwork/originals/originals_test.go | 80 +++++++++++++ go.mod | 2 +- 4 files changed, 210 insertions(+), 1 deletion(-) create mode 100644 core/artwork/originals/originals.go create mode 100644 core/artwork/originals/originals_suite_test.go create mode 100644 core/artwork/originals/originals_test.go diff --git a/core/artwork/originals/originals.go b/core/artwork/originals/originals.go new file mode 100644 index 000000000..12cfe56cc --- /dev/null +++ b/core/artwork/originals/originals.go @@ -0,0 +1,112 @@ +// Package originals is the content-addressed store for artwork images that have +// no library file backing them (external downloads, embedded extractions, generated). +package originals + +import ( + "errors" + "fmt" + "io" + "io/fs" + "os" + "path/filepath" + "strings" + + "github.com/zeebo/xxh3" +) + +func Hash(r io.Reader) (string, error) { + d := xxh3.New() + if _, err := io.Copy(d, r); err != nil { + return "", err + } + return fmt.Sprintf("%016x", d.Sum64()), nil +} + +type Store struct { + root string +} + +func New(rootDir string) *Store { + return &Store{root: rootDir} +} + +// extForMime is deliberately NOT mime.ExtensionsByType: extensions are baked into +// content-addressed paths and re-derived on Open, so they must be stable across OSes. +func extForMime(m string) string { + switch m { + case "image/jpeg": + return ".jpg" + case "image/png": + return ".png" + case "image/gif": + return ".gif" + case "image/webp": + return ".webp" + } + return ".img" +} + +func (s *Store) path(hash, mimeType string) string { + return filepath.Join(s.root, hash[0:2], hash[2:4], hash+extForMime(mimeType)) +} + +func (s *Store) Write(hash, mimeType string, r io.Reader) error { + dst := s.path(hash, mimeType) + if _, err := os.Stat(dst); err == nil { + return nil + } + if err := os.MkdirAll(filepath.Dir(dst), 0755); err != nil { + return err + } + tmp, err := os.CreateTemp(filepath.Dir(dst), "."+hash+".tmp*") + if err != nil { + return err + } + defer os.Remove(tmp.Name()) + if _, err := io.Copy(tmp, r); err != nil { + tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + return os.Rename(tmp.Name(), dst) +} + +func (s *Store) Open(hash, mimeType string) (io.ReadCloser, error) { + return os.Open(s.path(hash, mimeType)) +} + +func (s *Store) Remove(hash, mimeType string) error { + err := os.Remove(s.path(hash, mimeType)) + if errors.Is(err, fs.ErrNotExist) { + return nil + } + return err +} + +func (s *Store) Sweep(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 + return nil + } + hash := strings.TrimSuffix(name, filepath.Ext(name)) + if !keep(hash) { + // #nosec G122 -- path comes from WalkDir over our own store root, no attacker-controlled symlinks + if err := os.Remove(path); err != nil { + return err + } + removed++ + } + return nil + }) + if errors.Is(err, fs.ErrNotExist) { + return removed, nil + } + return removed, err +} diff --git a/core/artwork/originals/originals_suite_test.go b/core/artwork/originals/originals_suite_test.go new file mode 100644 index 000000000..2686f9ec9 --- /dev/null +++ b/core/artwork/originals/originals_suite_test.go @@ -0,0 +1,17 @@ +package originals_test + +import ( + "testing" + + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestOriginals(t *testing.T) { + tests.Init(t, false) + log.SetLevel(log.LevelFatal) + RegisterFailHandler(Fail) + RunSpecs(t, "Originals Store Suite") +} diff --git a/core/artwork/originals/originals_test.go b/core/artwork/originals/originals_test.go new file mode 100644 index 000000000..caa861a6f --- /dev/null +++ b/core/artwork/originals/originals_test.go @@ -0,0 +1,80 @@ +package originals_test + +import ( + "bytes" + "io" + "os" + "path/filepath" + + "github.com/navidrome/navidrome/core/artwork/originals" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Originals Store", func() { + var store *originals.Store + var root string + + BeforeEach(func() { + root = GinkgoT().TempDir() + store = originals.New(root) + }) + + It("hashes deterministically", func() { + h1, err := originals.Hash(bytes.NewReader([]byte("some image bytes"))) + Expect(err).ToNot(HaveOccurred()) + h2, _ := originals.Hash(bytes.NewReader([]byte("some image bytes"))) + Expect(h1).To(Equal(h2)) + Expect(h1).To(HaveLen(16)) + h3, _ := originals.Hash(bytes.NewReader([]byte("other bytes"))) + Expect(h3).ToNot(Equal(h1)) + }) + + It("writes sharded and reads back", func() { + data := []byte("jpeg-bytes") + h, _ := originals.Hash(bytes.NewReader(data)) + Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed()) + + Expect(filepath.Join(root, h[0:2], h[2:4], h+".jpg")).To(BeAnExistingFile()) + + rc, err := store.Open(h, "image/jpeg") + Expect(err).ToNot(HaveOccurred()) + defer rc.Close() + got, _ := io.ReadAll(rc) + Expect(got).To(Equal(data)) + }) + + It("is idempotent on duplicate writes", func() { + data := []byte("dup") + h, _ := originals.Hash(bytes.NewReader(data)) + Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed()) + Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed()) + }) + + It("returns fs.ErrNotExist for missing images", func() { + _, err := store.Open("beefbeefbeefbeef", "image/jpeg") + Expect(os.IsNotExist(err)).To(BeTrue()) + }) + + It("removes without error when already gone", func() { + Expect(store.Remove("beefbeefbeefbeef", "image/jpeg")).To(Succeed()) + }) + + It("sweeps unknown files, keeps known ones", func() { + d1 := []byte("keep-me") + h1, _ := originals.Hash(bytes.NewReader(d1)) + Expect(store.Write(h1, "image/jpeg", bytes.NewReader(d1))).To(Succeed()) + d2 := []byte("orphan") + h2, _ := originals.Hash(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 }) + Expect(err).ToNot(HaveOccurred()) + Expect(removed).To(Equal(1)) + _, err = store.Open(h2, "image/jpeg") + Expect(os.IsNotExist(err)).To(BeTrue()) + rc, err := store.Open(h1, "image/jpeg") + Expect(err).ToNot(HaveOccurred()) + rc.Close() + }) +}) diff --git a/go.mod b/go.mod index 5488b41e4..b72f4035c 100644 --- a/go.mod +++ b/go.mod @@ -57,6 +57,7 @@ require ( github.com/tetratelabs/wazero v1.12.0 github.com/unrolled/secure v1.17.0 github.com/xrash/smetrics v0.0.0-20250705151800-55b8f293f342 + github.com/zeebo/xxh3 v1.1.0 go.senan.xyz/taglib v0.11.1 go.uber.org/goleak v1.3.0 golang.org/x/image v0.44.0 @@ -128,7 +129,6 @@ require ( github.com/subosito/gotenv v1.6.0 // indirect github.com/tetratelabs/wabin v0.0.0-20230304001439-f6f874872834 // indirect github.com/valyala/fastjson v1.6.10 // indirect - github.com/zeebo/xxh3 v1.1.0 // indirect go.opentelemetry.io/proto/otlp v1.10.0 // indirect go.uber.org/multierr v1.11.0 // indirect go.yaml.in/yaml/v2 v2.4.3 // indirect From db16b3de9a6f6eb5f0fe8087fc41e9a45e4329c4 Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 21 Jul 2026 22:58:20 -0400 Subject: [PATCH 07/21] feat(artwork): add artwork prune (orphan cleanup) --- core/artwork/prune.go | 48 ++++++++++++++++++++++++++++++ core/artwork/prune_test.go | 61 ++++++++++++++++++++++++++++++++++++++ tests/mock_artwork_repo.go | 7 +++-- 3 files changed, 113 insertions(+), 3 deletions(-) create mode 100644 core/artwork/prune.go create mode 100644 core/artwork/prune_test.go diff --git a/core/artwork/prune.go b/core/artwork/prune.go new file mode 100644 index 000000000..7d66bd8fe --- /dev/null +++ b/core/artwork/prune.go @@ -0,0 +1,48 @@ +package artwork + +import ( + "context" + "time" + + "github.com/navidrome/navidrome/core/artwork/originals" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" +) + +// pruneMinAge guards the window between artwork insert and item_artwork upsert. +const pruneMinAge = time.Hour + +func Prune(ctx context.Context, ds model.DataStore, store *originals.Store) error { + repo := ds.Artwork(ctx) + orphans, err := repo.GetOrphanHashes(time.Now().Add(-pruneMinAge)) + if err != nil { + return err + } + if len(orphans) > 0 { + arts, err := repo.GetBatch(orphans) + if err != nil { + return err + } + if err := repo.Delete(orphans...); 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) + } + } + log.Info(ctx, "Prune: removed orphan artwork", "count", len(orphans)) + } + + removed, err := store.Sweep(func(hash string) bool { + _, err := repo.Get(hash) + return err == nil + }) + if err != nil { + return err + } + if removed > 0 { + log.Info(ctx, "Prune: swept stray artwork files", "count", removed) + } + return nil +} diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go new file mode 100644 index 000000000..ae6901da4 --- /dev/null +++ b/core/artwork/prune_test.go @@ -0,0 +1,61 @@ +package artwork + +import ( + "bytes" + "context" + "os" + "time" + + "github.com/navidrome/navidrome/core/artwork/originals" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Prune", func() { + var ds *tests.MockDataStore + var store *originals.Store + var awRepo *tests.MockArtworkRepo + + BeforeEach(func() { + ds = &tests.MockDataStore{} + awRepo = ds.Artwork(context.Background()).(*tests.MockArtworkRepo) + store = originals.New(GinkgoT().TempDir()) + }) + + It("deletes orphan rows and their store files, keeps referenced ones", func() { + data := []byte("orphan-bytes") + h, _ := originals.Hash(bytes.NewReader(data)) + Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed()) + Expect(awRepo.Put(&model.Artwork{Hash: h, Mime: "image/jpeg", + CreatedAt: time.Now().Add(-2 * time.Hour)})).To(Succeed()) + awRepo.OrphanHashes = []string{h} + + kept := []byte("kept-bytes") + hk, _ := originals.Hash(bytes.NewReader(kept)) + Expect(store.Write(hk, "image/jpeg", bytes.NewReader(kept))).To(Succeed()) + Expect(awRepo.Put(&model.Artwork{Hash: hk, Mime: "image/jpeg"})).To(Succeed()) + + Expect(Prune(context.Background(), ds, store)).To(Succeed()) + + _, err := awRepo.Get(h) + Expect(err).To(MatchError(model.ErrNotFound)) + _, err = store.Open(h, "image/jpeg") + Expect(os.IsNotExist(err)).To(BeTrue()) + rc, err := store.Open(hk, "image/jpeg") + Expect(err).ToNot(HaveOccurred()) + rc.Close() + }) + + It("sweeps store files that have no artwork row", func() { + stray := []byte("no-row-bytes") + h, _ := originals.Hash(bytes.NewReader(stray)) + Expect(store.Write(h, "image/jpeg", bytes.NewReader(stray))).To(Succeed()) + + Expect(Prune(context.Background(), ds, store)).To(Succeed()) + + _, err := store.Open(h, "image/jpeg") + Expect(os.IsNotExist(err)).To(BeTrue()) + }) +}) diff --git a/tests/mock_artwork_repo.go b/tests/mock_artwork_repo.go index c8b95a025..a30ffe97b 100644 --- a/tests/mock_artwork_repo.go +++ b/tests/mock_artwork_repo.go @@ -8,8 +8,9 @@ import ( type MockArtworkRepo struct { model.ArtworkRepository - Data map[string]model.Artwork - Err error + Data map[string]model.Artwork + OrphanHashes []string + Err error } func CreateMockArtworkRepo() *MockArtworkRepo { @@ -51,7 +52,7 @@ func (m *MockArtworkRepo) GetBatch(hashes []string) (map[string]model.Artwork, e } func (m *MockArtworkRepo) GetOrphanHashes(createdBefore time.Time) ([]string, error) { - return nil, m.Err + return m.OrphanHashes, m.Err } func (m *MockArtworkRepo) Delete(hashes ...string) error { From 3e7685adc23be7235d6d3c79ef2b48b683d74dfa Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 21 Jul 2026 23:02:47 -0400 Subject: [PATCH 08/21] fix(artwork): never sweep files on transient DB errors during prune --- core/artwork/prune.go | 3 ++- core/artwork/prune_test.go | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/core/artwork/prune.go b/core/artwork/prune.go index 7d66bd8fe..931990056 100644 --- a/core/artwork/prune.go +++ b/core/artwork/prune.go @@ -2,6 +2,7 @@ package artwork import ( "context" + "errors" "time" "github.com/navidrome/navidrome/core/artwork/originals" @@ -36,7 +37,7 @@ func Prune(ctx context.Context, ds model.DataStore, store *originals.Store) erro removed, err := store.Sweep(func(hash string) bool { _, err := repo.Get(hash) - return err == nil + return !errors.Is(err, model.ErrNotFound) }) if err != nil { return err diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index ae6901da4..54564a170 100644 --- a/core/artwork/prune_test.go +++ b/core/artwork/prune_test.go @@ -3,6 +3,7 @@ package artwork import ( "bytes" "context" + "errors" "os" "time" @@ -13,6 +14,14 @@ import ( . "github.com/onsi/gomega" ) +type flakyGetArtworkRepo struct { + *tests.MockArtworkRepo +} + +func (f *flakyGetArtworkRepo) Get(string) (*model.Artwork, error) { + return nil, errors.New("db locked") +} + var _ = Describe("Prune", func() { var ds *tests.MockDataStore var store *originals.Store @@ -58,4 +67,18 @@ var _ = Describe("Prune", func() { _, err := store.Open(h, "image/jpeg") Expect(os.IsNotExist(err)).To(BeTrue()) }) + + It("never sweeps files on a transient DB error", func() { + ds.MockedArtwork = &flakyGetArtworkRepo{MockArtworkRepo: tests.CreateMockArtworkRepo()} + + data := []byte("live-bytes") + h, _ := originals.Hash(bytes.NewReader(data)) + Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed()) + + Expect(Prune(context.Background(), ds, store)).To(Succeed()) + + rc, err := store.Open(h, "image/jpeg") + Expect(err).ToNot(HaveOccurred()) + rc.Close() + }) }) From b16ef725c90bb40bdf69a8dc7229fc1c47314450 Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 21 Jul 2026 23:09:31 -0400 Subject: [PATCH 09/21] refactor(artwork): fold originals package into core/artwork as ImageStore --- .../originals.go => image_store.go} | 24 +++++++++---------- .../originals_test.go => image_store_test.go} | 23 +++++++++--------- .../artwork/originals/originals_suite_test.go | 17 ------------- core/artwork/prune.go | 3 +-- core/artwork/prune_test.go | 13 +++++----- 5 files changed, 30 insertions(+), 50 deletions(-) rename core/artwork/{originals/originals.go => image_store.go} (74%) rename core/artwork/{originals/originals_test.go => image_store_test.go} (76%) delete mode 100644 core/artwork/originals/originals_suite_test.go diff --git a/core/artwork/originals/originals.go b/core/artwork/image_store.go similarity index 74% rename from core/artwork/originals/originals.go rename to core/artwork/image_store.go index 12cfe56cc..c98064d13 100644 --- a/core/artwork/originals/originals.go +++ b/core/artwork/image_store.go @@ -1,6 +1,4 @@ -// Package originals is the content-addressed store for artwork images that have -// no library file backing them (external downloads, embedded extractions, generated). -package originals +package artwork import ( "errors" @@ -14,7 +12,7 @@ import ( "github.com/zeebo/xxh3" ) -func Hash(r io.Reader) (string, error) { +func HashImage(r io.Reader) (string, error) { d := xxh3.New() if _, err := io.Copy(d, r); err != nil { return "", err @@ -22,12 +20,14 @@ func Hash(r io.Reader) (string, error) { return fmt.Sprintf("%016x", d.Sum64()), nil } -type Store struct { +// ImageStore is the content-addressed store for artwork images that have no +// library file backing them (external downloads, embedded extractions, generated). +type ImageStore struct { root string } -func New(rootDir string) *Store { - return &Store{root: rootDir} +func NewImageStore(rootDir string) *ImageStore { + return &ImageStore{root: rootDir} } // extForMime is deliberately NOT mime.ExtensionsByType: extensions are baked into @@ -46,11 +46,11 @@ func extForMime(m string) string { return ".img" } -func (s *Store) path(hash, mimeType string) string { +func (s *ImageStore) path(hash, mimeType string) string { return filepath.Join(s.root, hash[0:2], hash[2:4], hash+extForMime(mimeType)) } -func (s *Store) Write(hash, mimeType string, r io.Reader) error { +func (s *ImageStore) Write(hash, mimeType string, r io.Reader) error { dst := s.path(hash, mimeType) if _, err := os.Stat(dst); err == nil { return nil @@ -73,11 +73,11 @@ func (s *Store) Write(hash, mimeType string, r io.Reader) error { return os.Rename(tmp.Name(), dst) } -func (s *Store) Open(hash, mimeType string) (io.ReadCloser, error) { +func (s *ImageStore) Open(hash, mimeType string) (io.ReadCloser, error) { return os.Open(s.path(hash, mimeType)) } -func (s *Store) Remove(hash, mimeType string) error { +func (s *ImageStore) Remove(hash, mimeType string) error { err := os.Remove(s.path(hash, mimeType)) if errors.Is(err, fs.ErrNotExist) { return nil @@ -85,7 +85,7 @@ func (s *Store) Remove(hash, mimeType string) error { return err } -func (s *Store) Sweep(keep func(hash string) bool) (int, error) { +func (s *ImageStore) Sweep(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() { diff --git a/core/artwork/originals/originals_test.go b/core/artwork/image_store_test.go similarity index 76% rename from core/artwork/originals/originals_test.go rename to core/artwork/image_store_test.go index caa861a6f..c3ca69a58 100644 --- a/core/artwork/originals/originals_test.go +++ b/core/artwork/image_store_test.go @@ -1,4 +1,4 @@ -package originals_test +package artwork import ( "bytes" @@ -6,33 +6,32 @@ import ( "os" "path/filepath" - "github.com/navidrome/navidrome/core/artwork/originals" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) -var _ = Describe("Originals Store", func() { - var store *originals.Store +var _ = Describe("ImageStore", func() { + var store *ImageStore var root string BeforeEach(func() { root = GinkgoT().TempDir() - store = originals.New(root) + store = NewImageStore(root) }) It("hashes deterministically", func() { - h1, err := originals.Hash(bytes.NewReader([]byte("some image bytes"))) + h1, err := HashImage(bytes.NewReader([]byte("some image bytes"))) Expect(err).ToNot(HaveOccurred()) - h2, _ := originals.Hash(bytes.NewReader([]byte("some image bytes"))) + h2, _ := HashImage(bytes.NewReader([]byte("some image bytes"))) Expect(h1).To(Equal(h2)) Expect(h1).To(HaveLen(16)) - h3, _ := originals.Hash(bytes.NewReader([]byte("other bytes"))) + h3, _ := HashImage(bytes.NewReader([]byte("other bytes"))) Expect(h3).ToNot(Equal(h1)) }) It("writes sharded and reads back", func() { data := []byte("jpeg-bytes") - h, _ := originals.Hash(bytes.NewReader(data)) + h, _ := HashImage(bytes.NewReader(data)) Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed()) Expect(filepath.Join(root, h[0:2], h[2:4], h+".jpg")).To(BeAnExistingFile()) @@ -46,7 +45,7 @@ var _ = Describe("Originals Store", func() { It("is idempotent on duplicate writes", func() { data := []byte("dup") - h, _ := originals.Hash(bytes.NewReader(data)) + h, _ := HashImage(bytes.NewReader(data)) Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed()) Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed()) }) @@ -62,10 +61,10 @@ var _ = Describe("Originals Store", func() { It("sweeps unknown files, keeps known ones", func() { d1 := []byte("keep-me") - h1, _ := originals.Hash(bytes.NewReader(d1)) + h1, _ := HashImage(bytes.NewReader(d1)) Expect(store.Write(h1, "image/jpeg", bytes.NewReader(d1))).To(Succeed()) d2 := []byte("orphan") - h2, _ := originals.Hash(bytes.NewReader(d2)) + 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 }) diff --git a/core/artwork/originals/originals_suite_test.go b/core/artwork/originals/originals_suite_test.go deleted file mode 100644 index 2686f9ec9..000000000 --- a/core/artwork/originals/originals_suite_test.go +++ /dev/null @@ -1,17 +0,0 @@ -package originals_test - -import ( - "testing" - - "github.com/navidrome/navidrome/log" - "github.com/navidrome/navidrome/tests" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -func TestOriginals(t *testing.T) { - tests.Init(t, false) - log.SetLevel(log.LevelFatal) - RegisterFailHandler(Fail) - RunSpecs(t, "Originals Store Suite") -} diff --git a/core/artwork/prune.go b/core/artwork/prune.go index 931990056..9c88611a3 100644 --- a/core/artwork/prune.go +++ b/core/artwork/prune.go @@ -5,7 +5,6 @@ import ( "errors" "time" - "github.com/navidrome/navidrome/core/artwork/originals" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" ) @@ -13,7 +12,7 @@ import ( // pruneMinAge guards the window between artwork insert and item_artwork upsert. const pruneMinAge = time.Hour -func Prune(ctx context.Context, ds model.DataStore, store *originals.Store) error { +func Prune(ctx context.Context, ds model.DataStore, store *ImageStore) error { repo := ds.Artwork(ctx) orphans, err := repo.GetOrphanHashes(time.Now().Add(-pruneMinAge)) if err != nil { diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index 54564a170..925e46fd1 100644 --- a/core/artwork/prune_test.go +++ b/core/artwork/prune_test.go @@ -7,7 +7,6 @@ import ( "os" "time" - "github.com/navidrome/navidrome/core/artwork/originals" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" @@ -24,25 +23,25 @@ func (f *flakyGetArtworkRepo) Get(string) (*model.Artwork, error) { var _ = Describe("Prune", func() { var ds *tests.MockDataStore - var store *originals.Store + var store *ImageStore var awRepo *tests.MockArtworkRepo BeforeEach(func() { ds = &tests.MockDataStore{} awRepo = ds.Artwork(context.Background()).(*tests.MockArtworkRepo) - store = originals.New(GinkgoT().TempDir()) + store = NewImageStore(GinkgoT().TempDir()) }) It("deletes orphan rows and their store files, keeps referenced ones", func() { data := []byte("orphan-bytes") - h, _ := originals.Hash(bytes.NewReader(data)) + h, _ := HashImage(bytes.NewReader(data)) Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed()) Expect(awRepo.Put(&model.Artwork{Hash: h, Mime: "image/jpeg", CreatedAt: time.Now().Add(-2 * time.Hour)})).To(Succeed()) awRepo.OrphanHashes = []string{h} kept := []byte("kept-bytes") - hk, _ := originals.Hash(bytes.NewReader(kept)) + hk, _ := HashImage(bytes.NewReader(kept)) Expect(store.Write(hk, "image/jpeg", bytes.NewReader(kept))).To(Succeed()) Expect(awRepo.Put(&model.Artwork{Hash: hk, Mime: "image/jpeg"})).To(Succeed()) @@ -59,7 +58,7 @@ var _ = Describe("Prune", func() { It("sweeps store files that have no artwork row", func() { stray := []byte("no-row-bytes") - h, _ := originals.Hash(bytes.NewReader(stray)) + h, _ := HashImage(bytes.NewReader(stray)) Expect(store.Write(h, "image/jpeg", bytes.NewReader(stray))).To(Succeed()) Expect(Prune(context.Background(), ds, store)).To(Succeed()) @@ -72,7 +71,7 @@ var _ = Describe("Prune", func() { ds.MockedArtwork = &flakyGetArtworkRepo{MockArtworkRepo: tests.CreateMockArtworkRepo()} data := []byte("live-bytes") - h, _ := originals.Hash(bytes.NewReader(data)) + h, _ := HashImage(bytes.NewReader(data)) Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed()) Expect(Prune(context.Background(), ds, store)).To(Succeed()) From 4f835437a9787f9900fc875b8d11056d49bb1c64 Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 21 Jul 2026 23:14:01 -0400 Subject: [PATCH 10/21] refactor(artwork): merge item artwork state into ArtworkRepository --- core/artwork/prune.go | 6 +- core/artwork/prune_test.go | 8 +- model/artwork.go | 20 ++-- model/datastore.go | 1 - persistence/artwork_repository.go | 79 ++++++++++++++- persistence/artwork_repository_test.go | 102 +++++++++++++++++--- persistence/item_artwork_repository.go | 93 ------------------ persistence/item_artwork_repository_test.go | 83 ---------------- persistence/persistence.go | 4 - tests/mock_artwork_repo.go | 61 +++++++++++- tests/mock_data_store.go | 12 --- tests/mock_item_artwork_repo.go | 67 ------------- 12 files changed, 232 insertions(+), 304 deletions(-) delete mode 100644 persistence/item_artwork_repository.go delete mode 100644 persistence/item_artwork_repository_test.go delete mode 100644 tests/mock_item_artwork_repo.go diff --git a/core/artwork/prune.go b/core/artwork/prune.go index 9c88611a3..11d9ce34f 100644 --- a/core/artwork/prune.go +++ b/core/artwork/prune.go @@ -19,11 +19,11 @@ func Prune(ctx context.Context, ds model.DataStore, store *ImageStore) error { return err } if len(orphans) > 0 { - arts, err := repo.GetBatch(orphans) + arts, err := repo.GetImages(orphans) if err != nil { return err } - if err := repo.Delete(orphans...); err != nil { + if err := repo.DeleteImages(orphans...); err != nil { return err } for _, a := range arts { @@ -35,7 +35,7 @@ func Prune(ctx context.Context, ds model.DataStore, store *ImageStore) error { } removed, err := store.Sweep(func(hash string) bool { - _, err := repo.Get(hash) + _, err := repo.GetImage(hash) return !errors.Is(err, model.ErrNotFound) }) if err != nil { diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index 925e46fd1..68211879d 100644 --- a/core/artwork/prune_test.go +++ b/core/artwork/prune_test.go @@ -17,7 +17,7 @@ type flakyGetArtworkRepo struct { *tests.MockArtworkRepo } -func (f *flakyGetArtworkRepo) Get(string) (*model.Artwork, error) { +func (f *flakyGetArtworkRepo) GetImage(string) (*model.Artwork, error) { return nil, errors.New("db locked") } @@ -36,18 +36,18 @@ var _ = Describe("Prune", func() { data := []byte("orphan-bytes") h, _ := HashImage(bytes.NewReader(data)) Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed()) - Expect(awRepo.Put(&model.Artwork{Hash: h, Mime: "image/jpeg", + Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg", CreatedAt: time.Now().Add(-2 * time.Hour)})).To(Succeed()) awRepo.OrphanHashes = []string{h} kept := []byte("kept-bytes") hk, _ := HashImage(bytes.NewReader(kept)) Expect(store.Write(hk, "image/jpeg", bytes.NewReader(kept))).To(Succeed()) - Expect(awRepo.Put(&model.Artwork{Hash: hk, Mime: "image/jpeg"})).To(Succeed()) + Expect(awRepo.PutImage(&model.Artwork{Hash: hk, Mime: "image/jpeg"})).To(Succeed()) Expect(Prune(context.Background(), ds, store)).To(Succeed()) - _, err := awRepo.Get(h) + _, err := awRepo.GetImage(h) Expect(err).To(MatchError(model.ErrNotFound)) _, err = store.Open(h, "image/jpeg") Expect(os.IsNotExist(err)).To(BeTrue()) diff --git a/model/artwork.go b/model/artwork.go index dd6a3f436..8fe2bc7fe 100644 --- a/model/artwork.go +++ b/model/artwork.go @@ -55,21 +55,17 @@ const ( ) type ArtworkRepository interface { - Get(hash string) (*Artwork, error) - Put(a *Artwork) error - GetBatch(hashes []string) (map[string]Artwork, error) - // GetOrphanHashes returns hashes referenced by no item_artwork row and older than cutoff. + // Image identity (artwork table) + GetImage(hash string) (*Artwork, error) + PutImage(a *Artwork) error + GetImages(hashes []string) (map[string]Artwork, error) GetOrphanHashes(createdBefore time.Time) ([]string, error) - Delete(hashes ...string) error -} - -type ItemArtworkRepository interface { - Get(kind, id, imageType string) (*ItemArtwork, error) - Put(ia *ItemArtwork) error + DeleteImages(hashes ...string) error + // Per-item state (item_artwork table) + GetItemArtwork(kind, id, imageType string) (*ItemArtwork, error) + PutItemArtwork(ia *ItemArtwork) error DeleteForItem(kind, id string) error - // GetInfoForItems hydrates a page: one batched query, item_artwork joined to artwork. GetInfoForItems(kind string, ids []string) (map[string]ItemArtworkInfo, error) - // EnqueueStaleAbsent inserts queue rows (priority Recheck) for absent states older than cutoff. EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) } diff --git a/model/datastore.go b/model/datastore.go index c72eb9a96..273ca714b 100644 --- a/model/datastore.go +++ b/model/datastore.go @@ -41,7 +41,6 @@ type DataStore interface { Scrobble(ctx context.Context) ScrobbleRepository Plugin(ctx context.Context) PluginRepository Artwork(ctx context.Context) ArtworkRepository - ItemArtwork(ctx context.Context) ItemArtworkRepository ArtworkQueue(ctx context.Context) ArtworkQueueRepository Resource(ctx context.Context, model any) ResourceRepository diff --git a/persistence/artwork_repository.go b/persistence/artwork_repository.go index a49f30597..839a390ed 100644 --- a/persistence/artwork_repository.go +++ b/persistence/artwork_repository.go @@ -21,7 +21,7 @@ func NewArtworkRepository(ctx context.Context, db dbx.Builder) model.ArtworkRepo return r } -func (r *artworkRepository) Get(hash string) (*model.Artwork, error) { +func (r *artworkRepository) GetImage(hash string) (*model.Artwork, error) { sel := Select("*").From(r.tableName).Where(Eq{"hash": hash}) var res model.Artwork if err := r.queryOne(sel, &res); err != nil { @@ -30,7 +30,7 @@ func (r *artworkRepository) Get(hash string) (*model.Artwork, error) { return &res, nil } -func (r *artworkRepository) Put(a *model.Artwork) error { +func (r *artworkRepository) PutImage(a *model.Artwork) error { if a.CreatedAt.IsZero() { a.CreatedAt = time.Now() } @@ -45,7 +45,7 @@ func (r *artworkRepository) Put(a *model.Artwork) error { return err } -func (r *artworkRepository) GetBatch(hashes []string) (map[string]model.Artwork, error) { +func (r *artworkRepository) GetImages(hashes []string) (map[string]model.Artwork, error) { res := map[string]model.Artwork{} if len(hashes) == 0 { return res, nil @@ -72,11 +72,82 @@ func (r *artworkRepository) GetOrphanHashes(createdBefore time.Time) ([]string, return hashes, err } -func (r *artworkRepository) Delete(hashes ...string) error { +func (r *artworkRepository) DeleteImages(hashes ...string) error { if len(hashes) == 0 { return nil } return r.delete(Eq{"hash": hashes}) } +func (r *artworkRepository) GetItemArtwork(kind, id, imageType string) (*model.ItemArtwork, error) { + sel := Select("*").From("item_artwork"). + Where(Eq{"item_kind": kind, "item_id": id, "image_type": imageType}) + var res model.ItemArtwork + if err := r.queryOne(sel, &res); err != nil { + return nil, err + } + return &res, nil +} + +func (r *artworkRepository) PutItemArtwork(ia *model.ItemArtwork) error { + if ia.ImageType == "" { + ia.ImageType = model.ImageTypePrimary + } + ia.UpdatedAt = time.Now() + ins := Insert("item_artwork").SetMap(map[string]any{ + "item_kind": ia.ItemKind, "item_id": ia.ItemID, "image_type": ia.ImageType, + "hash": ia.Hash, "source": ia.Source, + "attempted_at": ia.AttemptedAt, "updated_at": ia.UpdatedAt, + }).Suffix(`ON CONFLICT (item_kind, item_id, image_type) DO UPDATE SET + hash=excluded.hash, source=excluded.source, + attempted_at=excluded.attempted_at, updated_at=excluded.updated_at`) + _, err := r.executeSQL(ins) + return err +} + +func (r *artworkRepository) DeleteForItem(kind, id string) error { + del := Delete("item_artwork").Where(Eq{"item_kind": kind, "item_id": id}) + _, err := r.executeSQL(del) + return err +} + +func (r *artworkRepository) GetInfoForItems(kind string, ids []string) (map[string]model.ItemArtworkInfo, error) { + res := map[string]model.ItemArtworkInfo{} + if len(ids) == 0 { + return res, nil + } + sel := Select("ia.item_id", "ia.hash", "COALESCE(a.blur_hash, '') as blur_hash"). + From("item_artwork ia"). + LeftJoin("artwork a ON a.hash = ia.hash"). + Where(And{ + Eq{"ia.item_kind": kind}, + Eq{"ia.image_type": model.ImageTypePrimary}, + Eq{"ia.item_id": ids}, + }) + var rows []struct { + ItemID string + Hash string + BlurHash string + } + if err := r.queryAll(sel, &rows); err != nil { + return nil, err + } + for _, row := range rows { + res[row.ItemID] = model.ItemArtworkInfo{ + ItemID: row.ItemID, Hash: row.Hash, BlurHash: row.BlurHash, Absent: row.Hash == "", + } + } + return res, nil +} + +func (r *artworkRepository) EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) { + now := time.Now() + ins := Expr(`INSERT INTO artwork_queue (item_kind, item_id, image_type, priority, attempts, retry_at, enqueued_at) + SELECT item_kind, item_id, image_type, ?, 0, ?, ? + FROM item_artwork WHERE item_kind = ? AND hash = '' AND attempted_at < ? + ON CONFLICT (item_kind, item_id, image_type) DO NOTHING`, + model.ArtworkPriorityRecheck, now, now, kind, attemptedBefore) + return r.executeSQL(ins) +} + var _ model.ArtworkRepository = (*artworkRepository)(nil) diff --git a/persistence/artwork_repository_test.go b/persistence/artwork_repository_test.go index e158754a0..b4da4ebbe 100644 --- a/persistence/artwork_repository_test.go +++ b/persistence/artwork_repository_test.go @@ -28,9 +28,9 @@ var _ = Describe("ArtworkRepository", func() { It("stores and retrieves an artwork by hash", func() { a := &model.Artwork{Hash: "abc123", Mime: "image/jpeg", Width: 500, Height: 500, SizeBytes: 1234, BlurHash: "LKO2?U%2Tw=w"} - Expect(repo.Put(a)).To(Succeed()) + Expect(repo.PutImage(a)).To(Succeed()) - got, err := repo.Get("abc123") + got, err := repo.GetImage("abc123") Expect(err).ToNot(HaveOccurred()) Expect(got.Mime).To(Equal("image/jpeg")) Expect(got.BlurHash).To(Equal("LKO2?U%2Tw=w")) @@ -39,32 +39,31 @@ var _ = Describe("ArtworkRepository", func() { It("is idempotent on Put (upsert by hash)", func() { a := &model.Artwork{Hash: "dup1", Mime: "image/png"} - Expect(repo.Put(a)).To(Succeed()) + Expect(repo.PutImage(a)).To(Succeed()) a.BlurHash = "XYZ" - Expect(repo.Put(a)).To(Succeed()) - got, _ := repo.Get("dup1") + Expect(repo.PutImage(a)).To(Succeed()) + got, _ := repo.GetImage("dup1") Expect(got.BlurHash).To(Equal("XYZ")) }) It("returns ErrNotFound for a missing hash", func() { - _, err := repo.Get("nope") + _, err := repo.GetImage("nope") Expect(err).To(MatchError(model.ErrNotFound)) }) It("fetches a batch", func() { - Expect(repo.Put(&model.Artwork{Hash: "b1", Mime: "image/jpeg"})).To(Succeed()) - Expect(repo.Put(&model.Artwork{Hash: "b2", Mime: "image/png"})).To(Succeed()) - got, err := repo.GetBatch([]string{"b1", "b2", "missing"}) + 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("finds orphans older than cutoff, honoring item_artwork references", func() { - Expect(repo.Put(&model.Artwork{Hash: "orph1", Mime: "image/jpeg"})).To(Succeed()) - Expect(repo.Put(&model.Artwork{Hash: "ref1", Mime: "image/jpeg"})).To(Succeed()) - iaRepo := NewItemArtworkRepository(context.Background(), GetDBXBuilder()) - Expect(iaRepo.Put(&model.ItemArtwork{ItemKind: "al", ItemID: "a1", ImageType: model.ImageTypePrimary, Hash: "ref1", Source: "folder"})).To(Succeed()) + 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()) @@ -77,9 +76,80 @@ var _ = Describe("ArtworkRepository", func() { }) It("deletes by hashes", func() { - Expect(repo.Put(&model.Artwork{Hash: "d1", Mime: "image/jpeg"})).To(Succeed()) - Expect(repo.Delete("d1")).To(Succeed()) - _, err := repo.Get("d1") + Expect(repo.PutImage(&model.Artwork{Hash: "d1", Mime: "image/jpeg"})).To(Succeed()) + Expect(repo.DeleteImages("d1")).To(Succeed()) + _, err := repo.GetImage("d1") Expect(err).To(MatchError(model.ErrNotFound)) }) }) + +var _ = Describe("ArtworkRepository item state", func() { + var repo model.ArtworkRepository + + BeforeEach(func() { + clearArtworkTables() + repo = NewArtworkRepository(context.Background(), GetDBXBuilder()) + }) + + It("upserts and reads state", func() { + ia := &model.ItemArtwork{ItemKind: "al", ItemID: "al1", ImageType: model.ImageTypePrimary, + Hash: "h1", Source: "folder", AttemptedAt: time.Now()} + Expect(repo.PutItemArtwork(ia)).To(Succeed()) + ia.Source = "embedded" + Expect(repo.PutItemArtwork(ia)).To(Succeed()) + + got, err := repo.GetItemArtwork("al", "al1", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + Expect(got.Source).To(Equal("embedded")) + Expect(got.UpdatedAt).ToNot(BeZero()) + }) + + 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()) + got, err := repo.GetItemArtwork("ar", "ar1", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + Expect(got.Hash).To(BeEmpty()) + }) + + It("hydrates a page in one batch, including blurhash and absence", func() { + Expect(repo.PutImage(&model.Artwork{Hash: "h9", Mime: "image/jpeg", BlurHash: "BH9"})).To(Succeed()) + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: "x1", ImageType: model.ImageTypePrimary, Hash: "h9", Source: "folder"})).To(Succeed()) + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: "x2", ImageType: model.ImageTypePrimary, Hash: "", Source: ""})).To(Succeed()) + + info, err := repo.GetInfoForItems("al", []string{"x1", "x2", "x3"}) + Expect(err).ToNot(HaveOccurred()) + Expect(info).To(HaveLen(2)) + Expect(info["x1"].Hash).To(Equal("h9")) + Expect(info["x1"].BlurHash).To(Equal("BH9")) + Expect(info["x1"].Absent).To(BeFalse()) + Expect(info["x2"].Absent).To(BeTrue()) + _, unresolved := info["x3"] + Expect(unresolved).To(BeFalse()) + }) + + It("deletes all rows for an item", func() { + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "pl", ItemID: "p1", ImageType: model.ImageTypePrimary, Hash: "h1"})).To(Succeed()) + Expect(repo.DeleteForItem("pl", "p1")).To(Succeed()) + _, err := repo.GetItemArtwork("pl", "p1", model.ImageTypePrimary) + Expect(err).To(MatchError(model.ErrNotFound)) + }) + + It("enqueues stale absent states for recheck", func() { + old := time.Now().Add(-48 * time.Hour) + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "stale1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old})).To(Succeed()) + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "fresh1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()})).To(Succeed()) + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "found1", ImageType: model.ImageTypePrimary, Hash: "hX", AttemptedAt: old})).To(Succeed()) + + n, err := repo.EnqueueStaleAbsent("ar", time.Now().Add(-24*time.Hour)) + Expect(err).ToNot(HaveOccurred()) + Expect(n).To(Equal(int64(1))) + + qRepo := NewArtworkQueueRepository(context.Background(), GetDBXBuilder()) + items, err := qRepo.DequeueBatch(10) + Expect(err).ToNot(HaveOccurred()) + Expect(items).To(HaveLen(1)) + Expect(items[0].ItemID).To(Equal("stale1")) + Expect(items[0].Priority).To(Equal(model.ArtworkPriorityRecheck)) + }) +}) diff --git a/persistence/item_artwork_repository.go b/persistence/item_artwork_repository.go deleted file mode 100644 index 08b4caf3c..000000000 --- a/persistence/item_artwork_repository.go +++ /dev/null @@ -1,93 +0,0 @@ -package persistence - -import ( - "context" - "time" - - . "github.com/Masterminds/squirrel" - "github.com/navidrome/navidrome/model" - "github.com/pocketbase/dbx" -) - -type itemArtworkRepository struct { - sqlRepository -} - -func NewItemArtworkRepository(ctx context.Context, db dbx.Builder) model.ItemArtworkRepository { - r := &itemArtworkRepository{} - r.ctx = ctx - r.db = db - r.tableName = "item_artwork" - return r -} - -func (r *itemArtworkRepository) Get(kind, id, imageType string) (*model.ItemArtwork, error) { - sel := Select("*").From(r.tableName). - Where(Eq{"item_kind": kind, "item_id": id, "image_type": imageType}) - var res model.ItemArtwork - if err := r.queryOne(sel, &res); err != nil { - return nil, err - } - return &res, nil -} - -func (r *itemArtworkRepository) Put(ia *model.ItemArtwork) error { - if ia.ImageType == "" { - ia.ImageType = model.ImageTypePrimary - } - ia.UpdatedAt = time.Now() - ins := Insert(r.tableName).SetMap(map[string]any{ - "item_kind": ia.ItemKind, "item_id": ia.ItemID, "image_type": ia.ImageType, - "hash": ia.Hash, "source": ia.Source, - "attempted_at": ia.AttemptedAt, "updated_at": ia.UpdatedAt, - }).Suffix(`ON CONFLICT (item_kind, item_id, image_type) DO UPDATE SET - hash=excluded.hash, source=excluded.source, - attempted_at=excluded.attempted_at, updated_at=excluded.updated_at`) - _, err := r.executeSQL(ins) - return err -} - -func (r *itemArtworkRepository) DeleteForItem(kind, id string) error { - return r.delete(Eq{"item_kind": kind, "item_id": id}) -} - -func (r *itemArtworkRepository) GetInfoForItems(kind string, ids []string) (map[string]model.ItemArtworkInfo, error) { - res := map[string]model.ItemArtworkInfo{} - if len(ids) == 0 { - return res, nil - } - sel := Select("ia.item_id", "ia.hash", "COALESCE(a.blur_hash, '') as blur_hash"). - From(r.tableName + " ia"). - LeftJoin("artwork a ON a.hash = ia.hash"). - Where(And{ - Eq{"ia.item_kind": kind}, - Eq{"ia.image_type": model.ImageTypePrimary}, - Eq{"ia.item_id": ids}, - }) - var rows []struct { - ItemID string - Hash string - BlurHash string - } - if err := r.queryAll(sel, &rows); err != nil { - return nil, err - } - for _, row := range rows { - res[row.ItemID] = model.ItemArtworkInfo{ - ItemID: row.ItemID, Hash: row.Hash, BlurHash: row.BlurHash, Absent: row.Hash == "", - } - } - return res, nil -} - -func (r *itemArtworkRepository) EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) { - now := time.Now() - ins := Expr(`INSERT INTO artwork_queue (item_kind, item_id, image_type, priority, attempts, retry_at, enqueued_at) - SELECT item_kind, item_id, image_type, ?, 0, ?, ? - FROM item_artwork WHERE item_kind = ? AND hash = '' AND attempted_at < ? - ON CONFLICT (item_kind, item_id, image_type) DO NOTHING`, - model.ArtworkPriorityRecheck, now, now, kind, attemptedBefore) - return r.executeSQL(ins) -} - -var _ model.ItemArtworkRepository = (*itemArtworkRepository)(nil) diff --git a/persistence/item_artwork_repository_test.go b/persistence/item_artwork_repository_test.go deleted file mode 100644 index 6aec55d89..000000000 --- a/persistence/item_artwork_repository_test.go +++ /dev/null @@ -1,83 +0,0 @@ -package persistence - -import ( - "context" - "time" - - "github.com/navidrome/navidrome/model" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -var _ = Describe("ItemArtworkRepository", func() { - var repo model.ItemArtworkRepository - var awRepo model.ArtworkRepository - - BeforeEach(func() { - clearArtworkTables() - repo = NewItemArtworkRepository(context.Background(), GetDBXBuilder()) - awRepo = NewArtworkRepository(context.Background(), GetDBXBuilder()) - }) - - It("upserts and reads state", func() { - ia := &model.ItemArtwork{ItemKind: "al", ItemID: "al1", ImageType: model.ImageTypePrimary, - Hash: "h1", Source: "folder", AttemptedAt: time.Now()} - Expect(repo.Put(ia)).To(Succeed()) - ia.Source = "embedded" - Expect(repo.Put(ia)).To(Succeed()) - - got, err := repo.Get("al", "al1", model.ImageTypePrimary) - Expect(err).ToNot(HaveOccurred()) - Expect(got.Source).To(Equal("embedded")) - Expect(got.UpdatedAt).ToNot(BeZero()) - }) - - It("represents known-absent as empty hash", func() { - Expect(repo.Put(&model.ItemArtwork{ItemKind: "ar", ItemID: "ar1", - ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()})).To(Succeed()) - got, err := repo.Get("ar", "ar1", model.ImageTypePrimary) - Expect(err).ToNot(HaveOccurred()) - Expect(got.Hash).To(BeEmpty()) - }) - - It("hydrates a page in one batch, including blurhash and absence", func() { - Expect(awRepo.Put(&model.Artwork{Hash: "h9", Mime: "image/jpeg", BlurHash: "BH9"})).To(Succeed()) - Expect(repo.Put(&model.ItemArtwork{ItemKind: "al", ItemID: "x1", ImageType: model.ImageTypePrimary, Hash: "h9", Source: "folder"})).To(Succeed()) - Expect(repo.Put(&model.ItemArtwork{ItemKind: "al", ItemID: "x2", ImageType: model.ImageTypePrimary, Hash: "", Source: ""})).To(Succeed()) - - info, err := repo.GetInfoForItems("al", []string{"x1", "x2", "x3"}) - Expect(err).ToNot(HaveOccurred()) - Expect(info).To(HaveLen(2)) - Expect(info["x1"].Hash).To(Equal("h9")) - Expect(info["x1"].BlurHash).To(Equal("BH9")) - Expect(info["x1"].Absent).To(BeFalse()) - Expect(info["x2"].Absent).To(BeTrue()) - _, unresolved := info["x3"] - Expect(unresolved).To(BeFalse()) - }) - - It("deletes all rows for an item", func() { - Expect(repo.Put(&model.ItemArtwork{ItemKind: "pl", ItemID: "p1", ImageType: model.ImageTypePrimary, Hash: "h1"})).To(Succeed()) - Expect(repo.DeleteForItem("pl", "p1")).To(Succeed()) - _, err := repo.Get("pl", "p1", model.ImageTypePrimary) - Expect(err).To(MatchError(model.ErrNotFound)) - }) - - It("enqueues stale absent states for recheck", func() { - old := time.Now().Add(-48 * time.Hour) - Expect(repo.Put(&model.ItemArtwork{ItemKind: "ar", ItemID: "stale1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old})).To(Succeed()) - Expect(repo.Put(&model.ItemArtwork{ItemKind: "ar", ItemID: "fresh1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()})).To(Succeed()) - Expect(repo.Put(&model.ItemArtwork{ItemKind: "ar", ItemID: "found1", ImageType: model.ImageTypePrimary, Hash: "hX", AttemptedAt: old})).To(Succeed()) - - n, err := repo.EnqueueStaleAbsent("ar", time.Now().Add(-24*time.Hour)) - Expect(err).ToNot(HaveOccurred()) - Expect(n).To(Equal(int64(1))) - - qRepo := NewArtworkQueueRepository(context.Background(), GetDBXBuilder()) - items, err := qRepo.DequeueBatch(10) - Expect(err).ToNot(HaveOccurred()) - Expect(items).To(HaveLen(1)) - Expect(items[0].ItemID).To(Equal("stale1")) - Expect(items[0].Priority).To(Equal(model.ArtworkPriorityRecheck)) - }) -}) diff --git a/persistence/persistence.go b/persistence/persistence.go index ed6d29865..9d3a33cfc 100644 --- a/persistence/persistence.go +++ b/persistence/persistence.go @@ -101,10 +101,6 @@ func (s *SQLStore) Artwork(ctx context.Context) model.ArtworkRepository { return NewArtworkRepository(ctx, s.getDBXBuilder()) } -func (s *SQLStore) ItemArtwork(ctx context.Context) model.ItemArtworkRepository { - return NewItemArtworkRepository(ctx, s.getDBXBuilder()) -} - func (s *SQLStore) ArtworkQueue(ctx context.Context) model.ArtworkQueueRepository { return NewArtworkQueueRepository(ctx, s.getDBXBuilder()) } diff --git a/tests/mock_artwork_repo.go b/tests/mock_artwork_repo.go index a30ffe97b..3989499ce 100644 --- a/tests/mock_artwork_repo.go +++ b/tests/mock_artwork_repo.go @@ -9,15 +9,18 @@ import ( type MockArtworkRepo struct { model.ArtworkRepository Data map[string]model.Artwork + ItemData map[string]model.ItemArtwork // key: kind + "|" + id + "|" + imageType OrphanHashes []string Err error } func CreateMockArtworkRepo() *MockArtworkRepo { - return &MockArtworkRepo{Data: map[string]model.Artwork{}} + return &MockArtworkRepo{Data: map[string]model.Artwork{}, ItemData: map[string]model.ItemArtwork{}} } -func (m *MockArtworkRepo) Get(hash string) (*model.Artwork, error) { +func iaKey(kind, id, imageType string) string { return kind + "|" + id + "|" + imageType } + +func (m *MockArtworkRepo) GetImage(hash string) (*model.Artwork, error) { if m.Err != nil { return nil, m.Err } @@ -27,7 +30,7 @@ func (m *MockArtworkRepo) Get(hash string) (*model.Artwork, error) { return nil, model.ErrNotFound } -func (m *MockArtworkRepo) Put(a *model.Artwork) error { +func (m *MockArtworkRepo) PutImage(a *model.Artwork) error { if m.Err != nil { return m.Err } @@ -38,7 +41,7 @@ func (m *MockArtworkRepo) Put(a *model.Artwork) error { return nil } -func (m *MockArtworkRepo) GetBatch(hashes []string) (map[string]model.Artwork, error) { +func (m *MockArtworkRepo) GetImages(hashes []string) (map[string]model.Artwork, error) { if m.Err != nil { return nil, m.Err } @@ -55,7 +58,7 @@ func (m *MockArtworkRepo) GetOrphanHashes(createdBefore time.Time) ([]string, er return m.OrphanHashes, m.Err } -func (m *MockArtworkRepo) Delete(hashes ...string) error { +func (m *MockArtworkRepo) DeleteImages(hashes ...string) error { if m.Err != nil { return m.Err } @@ -64,3 +67,51 @@ func (m *MockArtworkRepo) Delete(hashes ...string) error { } return nil } + +func (m *MockArtworkRepo) GetItemArtwork(kind, id, imageType string) (*model.ItemArtwork, error) { + if m.Err != nil { + return nil, m.Err + } + if ia, ok := m.ItemData[iaKey(kind, id, imageType)]; ok { + return &ia, nil + } + return nil, model.ErrNotFound +} + +func (m *MockArtworkRepo) PutItemArtwork(ia *model.ItemArtwork) error { + if m.Err != nil { + return m.Err + } + ia.UpdatedAt = time.Now() + m.ItemData[iaKey(ia.ItemKind, ia.ItemID, ia.ImageType)] = *ia + return nil +} + +func (m *MockArtworkRepo) DeleteForItem(kind, id string) error { + if m.Err != nil { + return m.Err + } + for k, ia := range m.ItemData { + if ia.ItemKind == kind && ia.ItemID == id { + delete(m.ItemData, k) + } + } + return nil +} + +func (m *MockArtworkRepo) GetInfoForItems(kind string, ids []string) (map[string]model.ItemArtworkInfo, error) { + if m.Err != nil { + return nil, m.Err + } + res := map[string]model.ItemArtworkInfo{} + for _, id := range ids { + if ia, ok := m.ItemData[iaKey(kind, id, model.ImageTypePrimary)]; ok { + res[id] = model.ItemArtworkInfo{ItemID: id, Hash: ia.Hash, Absent: ia.Hash == ""} + } + } + return res, nil +} + +func (m *MockArtworkRepo) EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) { + return 0, m.Err +} diff --git a/tests/mock_data_store.go b/tests/mock_data_store.go index 345a376e6..a6ca29df5 100644 --- a/tests/mock_data_store.go +++ b/tests/mock_data_store.go @@ -29,7 +29,6 @@ type MockDataStore struct { MockedRadio model.RadioRepository MockedPlugin model.PluginRepository MockedArtwork model.ArtworkRepository - MockedItemArtwork model.ItemArtworkRepository MockedArtworkQueue model.ArtworkQueueRepository scrobbleBufferMu sync.Mutex repoMu sync.Mutex @@ -261,17 +260,6 @@ func (db *MockDataStore) Artwork(ctx context.Context) model.ArtworkRepository { return db.MockedArtwork } -func (db *MockDataStore) ItemArtwork(ctx context.Context) model.ItemArtworkRepository { - if db.MockedItemArtwork != nil { - return db.MockedItemArtwork - } - if db.RealDS != nil { - return db.RealDS.ItemArtwork(ctx) - } - db.MockedItemArtwork = CreateMockItemArtworkRepo() - return db.MockedItemArtwork -} - func (db *MockDataStore) ArtworkQueue(ctx context.Context) model.ArtworkQueueRepository { if db.MockedArtworkQueue != nil { return db.MockedArtworkQueue diff --git a/tests/mock_item_artwork_repo.go b/tests/mock_item_artwork_repo.go deleted file mode 100644 index 7d9cfbd9e..000000000 --- a/tests/mock_item_artwork_repo.go +++ /dev/null @@ -1,67 +0,0 @@ -package tests - -import ( - "time" - - "github.com/navidrome/navidrome/model" -) - -type MockItemArtworkRepo struct { - model.ItemArtworkRepository - Data map[string]model.ItemArtwork // key: kind + "|" + id + "|" + imageType - Err error -} - -func CreateMockItemArtworkRepo() *MockItemArtworkRepo { - return &MockItemArtworkRepo{Data: map[string]model.ItemArtwork{}} -} - -func iaKey(kind, id, imageType string) string { return kind + "|" + id + "|" + imageType } - -func (m *MockItemArtworkRepo) Get(kind, id, imageType string) (*model.ItemArtwork, error) { - if m.Err != nil { - return nil, m.Err - } - if ia, ok := m.Data[iaKey(kind, id, imageType)]; ok { - return &ia, nil - } - return nil, model.ErrNotFound -} - -func (m *MockItemArtworkRepo) Put(ia *model.ItemArtwork) error { - if m.Err != nil { - return m.Err - } - ia.UpdatedAt = time.Now() - m.Data[iaKey(ia.ItemKind, ia.ItemID, ia.ImageType)] = *ia - return nil -} - -func (m *MockItemArtworkRepo) DeleteForItem(kind, id string) error { - if m.Err != nil { - return m.Err - } - for k, ia := range m.Data { - if ia.ItemKind == kind && ia.ItemID == id { - delete(m.Data, k) - } - } - return nil -} - -func (m *MockItemArtworkRepo) GetInfoForItems(kind string, ids []string) (map[string]model.ItemArtworkInfo, error) { - if m.Err != nil { - return nil, m.Err - } - res := map[string]model.ItemArtworkInfo{} - for _, id := range ids { - if ia, ok := m.Data[iaKey(kind, id, model.ImageTypePrimary)]; ok { - res[id] = model.ItemArtworkInfo{ItemID: id, Hash: ia.Hash, Absent: ia.Hash == ""} - } - } - return res, nil -} - -func (m *MockItemArtworkRepo) EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) { - return 0, m.Err -} From 1041e45ca7bfdbe04a8db61f87464ef9713792a6 Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 21 Jul 2026 23:24:42 -0400 Subject: [PATCH 11/21] fix(artwork): chunk unbounded IN clauses and restore interface docs --- model/artwork.go | 15 ++++-- persistence/artwork_repository.go | 71 +++++++++++++------------- persistence/artwork_repository_test.go | 15 ++++++ 3 files changed, 61 insertions(+), 40 deletions(-) diff --git a/model/artwork.go b/model/artwork.go index 8fe2bc7fe..6507c24d7 100644 --- a/model/artwork.go +++ b/model/artwork.go @@ -19,11 +19,13 @@ const ImageTypePrimary = "primary" // ItemArtwork is an entity's resolved artwork state. Hash=="" means known absent. type ItemArtwork struct { - ItemKind string `structs:"item_kind"` - ItemID string `structs:"item_id"` - ImageType string `structs:"image_type"` - Hash string `structs:"hash"` - Source string `structs:"source"` + ItemKind string `structs:"item_kind"` + ItemID string `structs:"item_id"` + ImageType string `structs:"image_type"` + Hash string `structs:"hash"` + Source string `structs:"source"` + // attempted_at/updated_at are nullable in the schema but always set by PutItemArtwork; + // raw inserts must set them too, since these non-pointer time.Time fields fail to scan NULL. AttemptedAt time.Time `structs:"attempted_at"` UpdatedAt time.Time `structs:"updated_at"` } @@ -59,13 +61,16 @@ 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) DeleteImages(hashes ...string) error // Per-item state (item_artwork table) GetItemArtwork(kind, id, imageType string) (*ItemArtwork, error) PutItemArtwork(ia *ItemArtwork) error DeleteForItem(kind, id string) error + // GetInfoForItems hydrates a page: one batched query, item_artwork joined to artwork. GetInfoForItems(kind string, ids []string) (map[string]ItemArtworkInfo, error) + // EnqueueStaleAbsent inserts queue rows (priority Recheck) for absent states older than cutoff. EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) } diff --git a/persistence/artwork_repository.go b/persistence/artwork_repository.go index 839a390ed..4556bc588 100644 --- a/persistence/artwork_repository.go +++ b/persistence/artwork_repository.go @@ -2,6 +2,7 @@ package persistence import ( "context" + "slices" "time" . "github.com/Masterminds/squirrel" @@ -47,16 +48,15 @@ func (r *artworkRepository) PutImage(a *model.Artwork) error { func (r *artworkRepository) GetImages(hashes []string) (map[string]model.Artwork, error) { res := map[string]model.Artwork{} - if len(hashes) == 0 { - return res, nil - } - sel := Select("*").From(r.tableName).Where(Eq{"hash": hashes}) - var all []model.Artwork - if err := r.queryAll(sel, &all); err != nil { - return nil, err - } - for _, a := range all { - res[a.Hash] = a + for chunk := range slices.Chunk(hashes, 200) { + 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 } @@ -73,10 +73,12 @@ func (r *artworkRepository) GetOrphanHashes(createdBefore time.Time) ([]string, } func (r *artworkRepository) DeleteImages(hashes ...string) error { - if len(hashes) == 0 { - return nil + for chunk := range slices.Chunk(hashes, 200) { + if err := r.delete(Eq{"hash": chunk}); err != nil { + return err + } } - return r.delete(Eq{"hash": hashes}) + return nil } func (r *artworkRepository) GetItemArtwork(kind, id, imageType string) (*model.ItemArtwork, error) { @@ -113,28 +115,27 @@ func (r *artworkRepository) DeleteForItem(kind, id string) error { func (r *artworkRepository) GetInfoForItems(kind string, ids []string) (map[string]model.ItemArtworkInfo, error) { res := map[string]model.ItemArtworkInfo{} - if len(ids) == 0 { - return res, nil - } - sel := Select("ia.item_id", "ia.hash", "COALESCE(a.blur_hash, '') as blur_hash"). - From("item_artwork ia"). - LeftJoin("artwork a ON a.hash = ia.hash"). - Where(And{ - Eq{"ia.item_kind": kind}, - Eq{"ia.image_type": model.ImageTypePrimary}, - Eq{"ia.item_id": ids}, - }) - var rows []struct { - ItemID string - Hash string - BlurHash string - } - if err := r.queryAll(sel, &rows); err != nil { - return nil, err - } - for _, row := range rows { - res[row.ItemID] = model.ItemArtworkInfo{ - ItemID: row.ItemID, Hash: row.Hash, BlurHash: row.BlurHash, Absent: row.Hash == "", + for chunk := range slices.Chunk(ids, 200) { + sel := Select("ia.item_id", "ia.hash", "COALESCE(a.blur_hash, '') as blur_hash"). + From("item_artwork ia"). + LeftJoin("artwork a ON a.hash = ia.hash"). + Where(And{ + Eq{"ia.item_kind": kind}, + Eq{"ia.image_type": model.ImageTypePrimary}, + Eq{"ia.item_id": chunk}, + }) + var rows []struct { + ItemID string + Hash string + BlurHash string + } + if err := r.queryAll(sel, &rows); err != nil { + return nil, err + } + for _, row := range rows { + res[row.ItemID] = model.ItemArtworkInfo{ + ItemID: row.ItemID, Hash: row.Hash, BlurHash: row.BlurHash, Absent: row.Hash == "", + } } } return res, nil diff --git a/persistence/artwork_repository_test.go b/persistence/artwork_repository_test.go index b4da4ebbe..a77fd7038 100644 --- a/persistence/artwork_repository_test.go +++ b/persistence/artwork_repository_test.go @@ -2,6 +2,7 @@ package persistence import ( "context" + "fmt" "time" "github.com/navidrome/navidrome/model" @@ -81,6 +82,20 @@ var _ = Describe("ArtworkRepository", func() { _, err := repo.GetImage("d1") Expect(err).To(MatchError(model.ErrNotFound)) }) + + It("fetches a batch larger than the SQL variable limit", func() { + hashes := make([]string, 0, 250) + for i := 0; i < 250; i++ { + 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)) + }) }) var _ = Describe("ArtworkRepository item state", func() { From 8fd7ef19f385640206cf5b8c57c434fe9bffc080 Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 21 Jul 2026 23:37:05 -0400 Subject: [PATCH 12/21] refactor(artwork): apply simplify-pass cleanups Internal item_artwork sqlRepository helper, toSQLArgs upserts, batched queue enqueue, EnqueueStaleAbsent moved to queue repo, snapshot-based prune sweep, mock/real semantics aligned. --- core/artwork/prune.go | 13 +- core/artwork/prune_test.go | 4 +- model/artwork.go | 10 +- persistence/artwork_queue_repository.go | 42 ++-- persistence/artwork_queue_repository_test.go | 18 ++ persistence/artwork_repository.go | 76 +++--- persistence/artwork_repository_test.go | 247 +++++++++---------- tests/mock_artwork_queue_repo.go | 27 +- tests/mock_artwork_repo.go | 26 +- 9 files changed, 260 insertions(+), 203 deletions(-) diff --git a/core/artwork/prune.go b/core/artwork/prune.go index 11d9ce34f..ebbb0bbc4 100644 --- a/core/artwork/prune.go +++ b/core/artwork/prune.go @@ -2,7 +2,6 @@ package artwork import ( "context" - "errors" "time" "github.com/navidrome/navidrome/log" @@ -34,9 +33,17 @@ func Prune(ctx context.Context, ds model.DataStore, store *ImageStore) error { log.Info(ctx, "Prune: removed orphan artwork", "count", len(orphans)) } + hashes, err := repo.GetAllHashes() + if err != nil { + return err + } + known := make(map[string]struct{}, len(hashes)) + for _, h := range hashes { + known[h] = struct{}{} + } removed, err := store.Sweep(func(hash string) bool { - _, err := repo.GetImage(hash) - return !errors.Is(err, model.ErrNotFound) + _, ok := known[hash] + return ok }) if err != nil { return err diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index 68211879d..2c8751674 100644 --- a/core/artwork/prune_test.go +++ b/core/artwork/prune_test.go @@ -17,7 +17,7 @@ type flakyGetArtworkRepo struct { *tests.MockArtworkRepo } -func (f *flakyGetArtworkRepo) GetImage(string) (*model.Artwork, error) { +func (f *flakyGetArtworkRepo) GetAllHashes() ([]string, error) { return nil, errors.New("db locked") } @@ -74,7 +74,7 @@ var _ = Describe("Prune", func() { h, _ := HashImage(bytes.NewReader(data)) Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed()) - Expect(Prune(context.Background(), ds, store)).To(Succeed()) + Expect(Prune(context.Background(), ds, store)).ToNot(Succeed()) rc, err := store.Open(h, "image/jpeg") Expect(err).ToNot(HaveOccurred()) diff --git a/model/artwork.go b/model/artwork.go index 6507c24d7..986a5c65d 100644 --- a/model/artwork.go +++ b/model/artwork.go @@ -35,9 +35,11 @@ type ItemArtworkInfo struct { ItemID string Hash string BlurHash string - Absent bool } +// Absent reports a known-absent artwork state (resolved, no image). +func (i ItemArtworkInfo) Absent() bool { return i.Hash == "" } + type ArtworkQueueItem struct { ItemKind string `structs:"item_kind"` ItemID string `structs:"item_id"` @@ -70,8 +72,8 @@ type ArtworkRepository interface { DeleteForItem(kind, id string) error // GetInfoForItems hydrates a page: one batched query, item_artwork joined to artwork. GetInfoForItems(kind string, ids []string) (map[string]ItemArtworkInfo, error) - // EnqueueStaleAbsent inserts queue rows (priority Recheck) for absent states older than cutoff. - EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) + // GetAllHashes returns every stored artwork hash, for sweep membership checks. + GetAllHashes() ([]string, error) } type ArtworkQueueRepository interface { @@ -83,4 +85,6 @@ type ArtworkQueueRepository interface { MarkFailed(kind, id, imageType string, retryAt time.Time) error Delete(kind, id, imageType string) error Count() (int64, error) + // EnqueueStaleAbsent inserts queue rows (priority Recheck) for absent states older than cutoff. + EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) } diff --git a/persistence/artwork_queue_repository.go b/persistence/artwork_queue_repository.go index fdf88db24..ced5e9913 100644 --- a/persistence/artwork_queue_repository.go +++ b/persistence/artwork_queue_repository.go @@ -2,6 +2,7 @@ package persistence import ( "context" + "slices" "time" . "github.com/Masterminds/squirrel" @@ -9,6 +10,9 @@ import ( "github.com/pocketbase/dbx" ) +// enqueueChunkSize keeps each multi-row insert under SQLite's bind-variable limit (7 cols -> 700 vars). +const enqueueChunkSize = 100 + type artworkQueueRepository struct { sqlRepository } @@ -23,14 +27,15 @@ func NewArtworkQueueRepository(ctx context.Context, db dbx.Builder) model.Artwor func (r *artworkQueueRepository) Enqueue(items ...model.ArtworkQueueItem) error { now := time.Now() - for _, it := range items { - if it.ImageType == "" { - it.ImageType = model.ImageTypePrimary + for chunk := range slices.Chunk(items, enqueueChunkSize) { + ins := Insert(r.tableName).Columns("item_kind", "item_id", "image_type", "priority", "attempts", "retry_at", "enqueued_at") + for _, it := range chunk { + if it.ImageType == "" { + it.ImageType = model.ImageTypePrimary + } + ins = ins.Values(it.ItemKind, it.ItemID, it.ImageType, it.Priority, 0, now, now) } - ins := Insert(r.tableName).SetMap(map[string]any{ - "item_kind": it.ItemKind, "item_id": it.ItemID, "image_type": it.ImageType, - "priority": it.Priority, "attempts": 0, "retry_at": now, "enqueued_at": now, - }).Suffix(`ON CONFLICT (item_kind, item_id, image_type) DO UPDATE SET + ins = ins.Suffix(`ON CONFLICT (item_kind, item_id, image_type) DO UPDATE SET priority = MAX(priority, excluded.priority), retry_at = excluded.retry_at`) if _, err := r.executeSQL(ins); err != nil { return err @@ -66,15 +71,20 @@ func (r *artworkQueueRepository) Delete(kind, id, imageType string) error { } func (r *artworkQueueRepository) Count() (int64, error) { - sel := Select("count(*)").From(r.tableName) - var counts []int64 - if err := r.queryAllSlice(sel, &counts); err != nil { - return 0, err - } - if len(counts) == 0 { - return 0, nil - } - return counts[0], nil + var res struct{ Count int64 } + err := r.queryOne(Select("count(*) as count").From(r.tableName), &res) + return res.Count, err +} + +func (r *artworkQueueRepository) EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) { + now := time.Now() + // DO NOTHING is deliberate: rechecks must not bump priority/retry_at of already-queued items. + ins := Expr(`INSERT INTO `+r.tableName+` (item_kind, item_id, image_type, priority, attempts, retry_at, enqueued_at) + SELECT item_kind, item_id, image_type, ?, 0, ?, ? + FROM `+itemArtworkTable+` WHERE item_kind = ? AND hash = '' AND attempted_at < ? + ON CONFLICT (item_kind, item_id, image_type) DO NOTHING`, + model.ArtworkPriorityRecheck, now, now, kind, attemptedBefore) + return r.executeSQL(ins) } var _ model.ArtworkQueueRepository = (*artworkQueueRepository)(nil) diff --git a/persistence/artwork_queue_repository_test.go b/persistence/artwork_queue_repository_test.go index 73665b07c..2b6f0d8c5 100644 --- a/persistence/artwork_queue_repository_test.go +++ b/persistence/artwork_queue_repository_test.go @@ -62,4 +62,22 @@ var _ = Describe("ArtworkQueueRepository", func() { n, _ = repo.Count() Expect(n).To(BeZero()) }) + + It("enqueues stale absent states for recheck", func() { + awRepo := NewArtworkRepository(context.Background(), GetDBXBuilder()) + old := time.Now().Add(-48 * time.Hour) + Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "stale1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old})).To(Succeed()) + Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "fresh1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()})).To(Succeed()) + Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "found1", ImageType: model.ImageTypePrimary, Hash: "hX", AttemptedAt: old})).To(Succeed()) + + n, err := repo.EnqueueStaleAbsent("ar", time.Now().Add(-24*time.Hour)) + Expect(err).ToNot(HaveOccurred()) + Expect(n).To(Equal(int64(1))) + + items, err := repo.DequeueBatch(10) + Expect(err).ToNot(HaveOccurred()) + Expect(items).To(HaveLen(1)) + Expect(items[0].ItemID).To(Equal("stale1")) + Expect(items[0].Priority).To(Equal(model.ArtworkPriorityRecheck)) + }) }) diff --git a/persistence/artwork_repository.go b/persistence/artwork_repository.go index 4556bc588..c87836ca9 100644 --- a/persistence/artwork_repository.go +++ b/persistence/artwork_repository.go @@ -10,8 +10,18 @@ import ( "github.com/pocketbase/dbx" ) +const ( + itemArtworkTable = "item_artwork" + artworkBatchSize = 200 +) + +type itemArtworkSQL struct { + sqlRepository +} + type artworkRepository struct { sqlRepository + items itemArtworkSQL } func NewArtworkRepository(ctx context.Context, db dbx.Builder) model.ArtworkRepository { @@ -19,6 +29,9 @@ func NewArtworkRepository(ctx context.Context, db dbx.Builder) model.ArtworkRepo r.ctx = ctx r.db = db r.tableName = "artwork" + r.items.ctx = ctx + r.items.db = db + r.items.tableName = itemArtworkTable return r } @@ -35,20 +48,20 @@ func (r *artworkRepository) PutImage(a *model.Artwork) error { if a.CreatedAt.IsZero() { a.CreatedAt = time.Now() } - ins := Insert(r.tableName).SetMap(map[string]any{ - "hash": a.Hash, "mime": a.Mime, "width": a.Width, "height": a.Height, - "size_bytes": a.SizeBytes, "blur_hash": a.BlurHash, - "source_path": a.SourcePath, "ref_mtime": a.RefMtime, "created_at": a.CreatedAt, - }).Suffix(`ON CONFLICT (hash) DO UPDATE SET mime=excluded.mime, width=excluded.width, + values, err := toSQLArgs(*a) + if err != nil { + return err + } + 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`) - _, err := r.executeSQL(ins) + _, err = r.executeSQL(ins) return err } func (r *artworkRepository) GetImages(hashes []string) (map[string]model.Artwork, error) { res := map[string]model.Artwork{} - for chunk := range slices.Chunk(hashes, 200) { + 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 { @@ -61,11 +74,18 @@ func (r *artworkRepository) GetImages(hashes []string) (map[string]model.Artwork return res, nil } +func (r *artworkRepository) GetAllHashes() ([]string, error) { + sel := Select("hash").From(r.tableName) + var hashes []string + err := r.queryAllSlice(sel, &hashes) + return hashes, err +} + 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 item_artwork WHERE hash <> '')"), + Expr("hash NOT IN (SELECT hash FROM " + itemArtworkTable + " WHERE hash <> '')"), }) var hashes []string err := r.queryAllSlice(sel, &hashes) @@ -73,7 +93,7 @@ func (r *artworkRepository) GetOrphanHashes(createdBefore time.Time) ([]string, } func (r *artworkRepository) DeleteImages(hashes ...string) error { - for chunk := range slices.Chunk(hashes, 200) { + for chunk := range slices.Chunk(hashes, artworkBatchSize) { if err := r.delete(Eq{"hash": chunk}); err != nil { return err } @@ -82,10 +102,10 @@ func (r *artworkRepository) DeleteImages(hashes ...string) error { } func (r *artworkRepository) GetItemArtwork(kind, id, imageType string) (*model.ItemArtwork, error) { - sel := Select("*").From("item_artwork"). + sel := Select("*").From(itemArtworkTable). Where(Eq{"item_kind": kind, "item_id": id, "image_type": imageType}) var res model.ItemArtwork - if err := r.queryOne(sel, &res); err != nil { + if err := r.items.queryOne(sel, &res); err != nil { return nil, err } return &res, nil @@ -96,28 +116,26 @@ func (r *artworkRepository) PutItemArtwork(ia *model.ItemArtwork) error { ia.ImageType = model.ImageTypePrimary } ia.UpdatedAt = time.Now() - ins := Insert("item_artwork").SetMap(map[string]any{ - "item_kind": ia.ItemKind, "item_id": ia.ItemID, "image_type": ia.ImageType, - "hash": ia.Hash, "source": ia.Source, - "attempted_at": ia.AttemptedAt, "updated_at": ia.UpdatedAt, - }).Suffix(`ON CONFLICT (item_kind, item_id, image_type) DO UPDATE SET + values, err := toSQLArgs(*ia) + if err != nil { + return err + } + ins := Insert(itemArtworkTable).SetMap(values).Suffix(`ON CONFLICT (item_kind, item_id, image_type) DO UPDATE SET hash=excluded.hash, source=excluded.source, attempted_at=excluded.attempted_at, updated_at=excluded.updated_at`) - _, err := r.executeSQL(ins) + _, err = r.items.executeSQL(ins) return err } func (r *artworkRepository) DeleteForItem(kind, id string) error { - del := Delete("item_artwork").Where(Eq{"item_kind": kind, "item_id": id}) - _, err := r.executeSQL(del) - return err + return r.items.delete(Eq{"item_kind": kind, "item_id": id}) } func (r *artworkRepository) GetInfoForItems(kind string, ids []string) (map[string]model.ItemArtworkInfo, error) { res := map[string]model.ItemArtworkInfo{} - for chunk := range slices.Chunk(ids, 200) { + for chunk := range slices.Chunk(ids, artworkBatchSize) { sel := Select("ia.item_id", "ia.hash", "COALESCE(a.blur_hash, '') as blur_hash"). - From("item_artwork ia"). + From(itemArtworkTable + " ia"). LeftJoin("artwork a ON a.hash = ia.hash"). Where(And{ Eq{"ia.item_kind": kind}, @@ -129,26 +147,16 @@ func (r *artworkRepository) GetInfoForItems(kind string, ids []string) (map[stri Hash string BlurHash string } - if err := r.queryAll(sel, &rows); err != nil { + if err := r.items.queryAll(sel, &rows); err != nil { return nil, err } for _, row := range rows { res[row.ItemID] = model.ItemArtworkInfo{ - ItemID: row.ItemID, Hash: row.Hash, BlurHash: row.BlurHash, Absent: row.Hash == "", + ItemID: row.ItemID, Hash: row.Hash, BlurHash: row.BlurHash, } } } return res, nil } -func (r *artworkRepository) EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) { - now := time.Now() - ins := Expr(`INSERT INTO artwork_queue (item_kind, item_id, image_type, priority, attempts, retry_at, enqueued_at) - SELECT item_kind, item_id, image_type, ?, 0, ?, ? - FROM item_artwork WHERE item_kind = ? AND hash = '' AND attempted_at < ? - ON CONFLICT (item_kind, item_id, image_type) DO NOTHING`, - model.ArtworkPriorityRecheck, now, now, kind, attemptedBefore) - return r.executeSQL(ins) -} - var _ model.ArtworkRepository = (*artworkRepository)(nil) diff --git a/persistence/artwork_repository_test.go b/persistence/artwork_repository_test.go index a77fd7038..0fdb122b6 100644 --- a/persistence/artwork_repository_test.go +++ b/persistence/artwork_repository_test.go @@ -27,144 +27,129 @@ var _ = Describe("ArtworkRepository", func() { repo = NewArtworkRepository(context.Background(), GetDBXBuilder()) }) - It("stores and retrieves an artwork by hash", func() { - a := &model.Artwork{Hash: "abc123", Mime: "image/jpeg", Width: 500, Height: 500, SizeBytes: 1234, BlurHash: "LKO2?U%2Tw=w"} - Expect(repo.PutImage(a)).To(Succeed()) + Context("image identity", func() { + It("stores and retrieves an artwork by hash", func() { + a := &model.Artwork{Hash: "abc123", Mime: "image/jpeg", Width: 500, Height: 500, SizeBytes: 1234, BlurHash: "LKO2?U%2Tw=w"} + Expect(repo.PutImage(a)).To(Succeed()) - got, err := repo.GetImage("abc123") - Expect(err).ToNot(HaveOccurred()) - Expect(got.Mime).To(Equal("image/jpeg")) - Expect(got.BlurHash).To(Equal("LKO2?U%2Tw=w")) - Expect(got.CreatedAt).ToNot(BeZero()) + got, err := repo.GetImage("abc123") + Expect(err).ToNot(HaveOccurred()) + Expect(got.Mime).To(Equal("image/jpeg")) + Expect(got.BlurHash).To(Equal("LKO2?U%2Tw=w")) + Expect(got.CreatedAt).ToNot(BeZero()) + }) + + It("is idempotent on Put (upsert by hash)", func() { + a := &model.Artwork{Hash: "dup1", Mime: "image/png"} + Expect(repo.PutImage(a)).To(Succeed()) + a.BlurHash = "XYZ" + Expect(repo.PutImage(a)).To(Succeed()) + got, _ := repo.GetImage("dup1") + Expect(got.BlurHash).To(Equal("XYZ")) + }) + + It("returns ErrNotFound for a missing hash", func() { + _, err := repo.GetImage("nope") + 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", 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()) + hashes, err := repo.GetAllHashes() + Expect(err).ToNot(HaveOccurred()) + Expect(hashes).To(ConsistOf("all1", "all2")) + }) + + 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 by hashes", func() { + Expect(repo.PutImage(&model.Artwork{Hash: "d1", Mime: "image/jpeg"})).To(Succeed()) + Expect(repo.DeleteImages("d1")).To(Succeed()) + _, err := repo.GetImage("d1") + Expect(err).To(MatchError(model.ErrNotFound)) + }) + + 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)) + }) }) - It("is idempotent on Put (upsert by hash)", func() { - a := &model.Artwork{Hash: "dup1", Mime: "image/png"} - Expect(repo.PutImage(a)).To(Succeed()) - a.BlurHash = "XYZ" - Expect(repo.PutImage(a)).To(Succeed()) - got, _ := repo.GetImage("dup1") - Expect(got.BlurHash).To(Equal("XYZ")) - }) + Context("item state", func() { + It("upserts and reads state", func() { + ia := &model.ItemArtwork{ItemKind: "al", ItemID: "al1", ImageType: model.ImageTypePrimary, + Hash: "h1", Source: "folder", AttemptedAt: time.Now()} + Expect(repo.PutItemArtwork(ia)).To(Succeed()) + ia.Source = "embedded" + Expect(repo.PutItemArtwork(ia)).To(Succeed()) - It("returns ErrNotFound for a missing hash", func() { - _, err := repo.GetImage("nope") - Expect(err).To(MatchError(model.ErrNotFound)) - }) + got, err := repo.GetItemArtwork("al", "al1", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + Expect(got.Source).To(Equal("embedded")) + Expect(got.UpdatedAt).ToNot(BeZero()) + }) - 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("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()) + got, err := repo.GetItemArtwork("ar", "ar1", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + Expect(got.Hash).To(BeEmpty()) + }) - 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()) + It("hydrates a page in one batch, including blurhash and absence", func() { + Expect(repo.PutImage(&model.Artwork{Hash: "h9", Mime: "image/jpeg", BlurHash: "BH9"})).To(Succeed()) + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: "x1", ImageType: model.ImageTypePrimary, Hash: "h9", Source: "folder"})).To(Succeed()) + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: "x2", ImageType: model.ImageTypePrimary, Hash: "", Source: ""})).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")) + info, err := repo.GetInfoForItems("al", []string{"x1", "x2", "x3"}) + Expect(err).ToNot(HaveOccurred()) + Expect(info).To(HaveLen(2)) + Expect(info["x1"].Hash).To(Equal("h9")) + Expect(info["x1"].BlurHash).To(Equal("BH9")) + Expect(info["x1"].Absent()).To(BeFalse()) + Expect(info["x2"].Absent()).To(BeTrue()) + _, unresolved := info["x3"] + Expect(unresolved).To(BeFalse()) + }) - orphans, err = repo.GetOrphanHashes(time.Now().Add(-time.Hour)) - Expect(err).ToNot(HaveOccurred()) - Expect(orphans).To(BeEmpty()) - }) - - It("deletes by hashes", func() { - Expect(repo.PutImage(&model.Artwork{Hash: "d1", Mime: "image/jpeg"})).To(Succeed()) - Expect(repo.DeleteImages("d1")).To(Succeed()) - _, err := repo.GetImage("d1") - Expect(err).To(MatchError(model.ErrNotFound)) - }) - - It("fetches a batch larger than the SQL variable limit", func() { - hashes := make([]string, 0, 250) - for i := 0; i < 250; i++ { - 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)) - }) -}) - -var _ = Describe("ArtworkRepository item state", func() { - var repo model.ArtworkRepository - - BeforeEach(func() { - clearArtworkTables() - repo = NewArtworkRepository(context.Background(), GetDBXBuilder()) - }) - - It("upserts and reads state", func() { - ia := &model.ItemArtwork{ItemKind: "al", ItemID: "al1", ImageType: model.ImageTypePrimary, - Hash: "h1", Source: "folder", AttemptedAt: time.Now()} - Expect(repo.PutItemArtwork(ia)).To(Succeed()) - ia.Source = "embedded" - Expect(repo.PutItemArtwork(ia)).To(Succeed()) - - got, err := repo.GetItemArtwork("al", "al1", model.ImageTypePrimary) - Expect(err).ToNot(HaveOccurred()) - Expect(got.Source).To(Equal("embedded")) - Expect(got.UpdatedAt).ToNot(BeZero()) - }) - - 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()) - got, err := repo.GetItemArtwork("ar", "ar1", model.ImageTypePrimary) - Expect(err).ToNot(HaveOccurred()) - Expect(got.Hash).To(BeEmpty()) - }) - - It("hydrates a page in one batch, including blurhash and absence", func() { - Expect(repo.PutImage(&model.Artwork{Hash: "h9", Mime: "image/jpeg", BlurHash: "BH9"})).To(Succeed()) - Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: "x1", ImageType: model.ImageTypePrimary, Hash: "h9", Source: "folder"})).To(Succeed()) - Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: "x2", ImageType: model.ImageTypePrimary, Hash: "", Source: ""})).To(Succeed()) - - info, err := repo.GetInfoForItems("al", []string{"x1", "x2", "x3"}) - Expect(err).ToNot(HaveOccurred()) - Expect(info).To(HaveLen(2)) - Expect(info["x1"].Hash).To(Equal("h9")) - Expect(info["x1"].BlurHash).To(Equal("BH9")) - Expect(info["x1"].Absent).To(BeFalse()) - Expect(info["x2"].Absent).To(BeTrue()) - _, unresolved := info["x3"] - Expect(unresolved).To(BeFalse()) - }) - - It("deletes all rows for an item", func() { - Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "pl", ItemID: "p1", ImageType: model.ImageTypePrimary, Hash: "h1"})).To(Succeed()) - Expect(repo.DeleteForItem("pl", "p1")).To(Succeed()) - _, err := repo.GetItemArtwork("pl", "p1", model.ImageTypePrimary) - Expect(err).To(MatchError(model.ErrNotFound)) - }) - - It("enqueues stale absent states for recheck", func() { - old := time.Now().Add(-48 * time.Hour) - Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "stale1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old})).To(Succeed()) - Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "fresh1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()})).To(Succeed()) - Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "ar", ItemID: "found1", ImageType: model.ImageTypePrimary, Hash: "hX", AttemptedAt: old})).To(Succeed()) - - n, err := repo.EnqueueStaleAbsent("ar", time.Now().Add(-24*time.Hour)) - Expect(err).ToNot(HaveOccurred()) - Expect(n).To(Equal(int64(1))) - - qRepo := NewArtworkQueueRepository(context.Background(), GetDBXBuilder()) - items, err := qRepo.DequeueBatch(10) - Expect(err).ToNot(HaveOccurred()) - Expect(items).To(HaveLen(1)) - Expect(items[0].ItemID).To(Equal("stale1")) - Expect(items[0].Priority).To(Equal(model.ArtworkPriorityRecheck)) + It("deletes all rows for an item", func() { + Expect(repo.PutItemArtwork(&model.ItemArtwork{ItemKind: "pl", ItemID: "p1", ImageType: model.ImageTypePrimary, Hash: "h1"})).To(Succeed()) + Expect(repo.DeleteForItem("pl", "p1")).To(Succeed()) + _, err := repo.GetItemArtwork("pl", "p1", model.ImageTypePrimary) + Expect(err).To(MatchError(model.ErrNotFound)) + }) }) }) diff --git a/tests/mock_artwork_queue_repo.go b/tests/mock_artwork_queue_repo.go index b08a1297c..dcec7486d 100644 --- a/tests/mock_artwork_queue_repo.go +++ b/tests/mock_artwork_queue_repo.go @@ -9,7 +9,7 @@ import ( type MockArtworkQueueRepo struct { model.ArtworkQueueRepository - Data map[string]model.ArtworkQueueItem // key: kind + "|" + id + "|" + imageType + Data map[string]model.ArtworkQueueItem // keyed by iaKey(kind, id, imageType) Err error } @@ -21,16 +21,23 @@ func (m *MockArtworkQueueRepo) Enqueue(items ...model.ArtworkQueueItem) error { if m.Err != nil { return m.Err } + now := time.Now() for _, it := range items { - k := iaKey(it.ItemKind, it.ItemID, it.ImageType) - if prev, ok := m.Data[k]; ok && prev.Priority > it.Priority { - it.Priority = prev.Priority - } - if it.EnqueuedAt.IsZero() { - it.EnqueuedAt = time.Now() + if it.ImageType == "" { + it.ImageType = model.ImageTypePrimary } if it.RetryAt.IsZero() { - it.RetryAt = time.Now() + it.RetryAt = now + } + k := iaKey(it.ItemKind, it.ItemID, it.ImageType) + if prev, ok := m.Data[k]; ok { + prev.Priority = max(prev.Priority, it.Priority) + prev.RetryAt = it.RetryAt + m.Data[k] = prev + continue + } + if it.EnqueuedAt.IsZero() { + it.EnqueuedAt = now } m.Data[k] = it } @@ -89,3 +96,7 @@ func (m *MockArtworkQueueRepo) Count() (int64, error) { } return int64(len(m.Data)), nil } + +func (m *MockArtworkQueueRepo) EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) { + return 0, m.Err +} diff --git a/tests/mock_artwork_repo.go b/tests/mock_artwork_repo.go index 3989499ce..8ac4820e6 100644 --- a/tests/mock_artwork_repo.go +++ b/tests/mock_artwork_repo.go @@ -9,7 +9,7 @@ import ( type MockArtworkRepo struct { model.ArtworkRepository Data map[string]model.Artwork - ItemData map[string]model.ItemArtwork // key: kind + "|" + id + "|" + imageType + ItemData map[string]model.ItemArtwork // keyed by iaKey(kind, id, imageType) OrphanHashes []string Err error } @@ -58,6 +58,17 @@ func (m *MockArtworkRepo) GetOrphanHashes(createdBefore time.Time) ([]string, er return m.OrphanHashes, m.Err } +func (m *MockArtworkRepo) GetAllHashes() ([]string, error) { + if m.Err != nil { + return nil, m.Err + } + hashes := make([]string, 0, len(m.Data)) + for h := range m.Data { + hashes = append(hashes, h) + } + return hashes, nil +} + func (m *MockArtworkRepo) DeleteImages(hashes ...string) error { if m.Err != nil { return m.Err @@ -82,6 +93,9 @@ func (m *MockArtworkRepo) PutItemArtwork(ia *model.ItemArtwork) error { if m.Err != nil { return m.Err } + if ia.ImageType == "" { + ia.ImageType = model.ImageTypePrimary + } ia.UpdatedAt = time.Now() m.ItemData[iaKey(ia.ItemKind, ia.ItemID, ia.ImageType)] = *ia return nil @@ -106,12 +120,12 @@ func (m *MockArtworkRepo) GetInfoForItems(kind string, ids []string) (map[string res := map[string]model.ItemArtworkInfo{} for _, id := range ids { if ia, ok := m.ItemData[iaKey(kind, id, model.ImageTypePrimary)]; ok { - res[id] = model.ItemArtworkInfo{ItemID: id, Hash: ia.Hash, Absent: ia.Hash == ""} + info := model.ItemArtworkInfo{ItemID: id, Hash: ia.Hash} + if a, ok := m.Data[ia.Hash]; ok { + info.BlurHash = a.BlurHash + } + res[id] = info } } return res, nil } - -func (m *MockArtworkRepo) EnqueueStaleAbsent(kind string, attemptedBefore time.Time) (int64, error) { - return 0, m.Err -} From 6f7f9c64633e362e5f175ec475d76eb879752449 Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 21 Jul 2026 23:57:58 -0400 Subject: [PATCH 13/21] 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 | 20 +++++++++++---- core/artwork/image_store_test.go | 35 +++++++++++++++++++++++++- core/artwork/prune.go | 7 ++++-- core/artwork/prune_test.go | 2 ++ persistence/artwork_repository.go | 8 +++--- persistence/artwork_repository_test.go | 14 +++++++++++ tests/mock_artwork_queue_repo.go | 27 +++++++++++++++++++- tests/mock_data_store.go | 6 ++++- 8 files changed, 105 insertions(+), 14 deletions(-) diff --git a/core/artwork/image_store.go b/core/artwork/image_store.go index c98064d13..517dfc879 100644 --- a/core/artwork/image_store.go +++ b/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 diff --git a/core/artwork/image_store_test.go b/core/artwork/image_store_test.go index c3ca69a58..2647f3b7f 100644 --- a/core/artwork/image_store_test.go +++ b/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()) + }) }) diff --git a/core/artwork/prune.go b/core/artwork/prune.go index ebbb0bbc4..b787dd1c9 100644 --- a/core/artwork/prune.go +++ b/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 }) diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index 2c8751674..0dd0b660f 100644 --- a/core/artwork/prune_test.go +++ b/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()) diff --git a/persistence/artwork_repository.go b/persistence/artwork_repository.go index c87836ca9..145e0b3f7 100644 --- a/persistence/artwork_repository.go +++ b/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 } diff --git a/persistence/artwork_repository_test.go b/persistence/artwork_repository_test.go index 0fdb122b6..12456bcd1 100644 --- a/persistence/artwork_repository_test.go +++ b/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)) diff --git a/tests/mock_artwork_queue_repo.go b/tests/mock_artwork_queue_repo.go index dcec7486d..fceded1fb 100644 --- a/tests/mock_artwork_queue_repo.go +++ b/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 } diff --git a/tests/mock_data_store.go b/tests/mock_data_store.go index a6ca29df5..d8585fabf 100644 --- a/tests/mock_data_store.go +++ b/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 } From 623b7d6a6c864f6c340db8fa7724001b0f0ddb36 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 22 Jul 2026 00:11:15 -0400 Subject: [PATCH 14/21] 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 | 29 ++++++++++++++++++-------- core/artwork/prune_test.go | 20 ++++++++++++++++++ model/artwork.go | 3 ++- persistence/artwork_repository.go | 13 ++++++++++-- persistence/artwork_repository_test.go | 27 ++++++++++++++++++++++-- tests/mock_artwork_queue_repo.go | 12 +++++------ tests/mock_artwork_repo.go | 18 +++++++++++++++- 7 files changed, 100 insertions(+), 22 deletions(-) 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 } From bf614e66adad3f1ce99a479fc1a8a1cd02e48d71 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 22 Jul 2026 00:19:45 -0400 Subject: [PATCH 15/21] fix(artwork): guard orphan file removal with the prune grace window Duplicate ImageStore writes refresh the file mtime and Remove skips files newer than the cutoff, so overlapping acquisitions cannot lose their store files to a concurrent prune. --- core/artwork/image_store.go | 20 +++++++++++++++-- core/artwork/image_store_test.go | 38 +++++++++++++++++++++++++++++++- core/artwork/prune.go | 4 +++- core/artwork/prune_test.go | 19 ++++++++++++++++ 4 files changed, 77 insertions(+), 4 deletions(-) diff --git a/core/artwork/image_store.go b/core/artwork/image_store.go index 517dfc879..ef27ed00a 100644 --- a/core/artwork/image_store.go +++ b/core/artwork/image_store.go @@ -54,6 +54,9 @@ func (s *ImageStore) path(hash, mimeType string) string { func (s *ImageStore) Write(hash, mimeType string, r io.Reader) error { dst := s.path(hash, mimeType) if _, err := os.Stat(dst); err == nil { + // A touched mtime marks the file live so a concurrent prune spares it. + now := time.Now() + _ = os.Chtimes(dst, now, now) return nil } if err := os.MkdirAll(filepath.Dir(dst), 0755); err != nil { @@ -78,8 +81,21 @@ func (s *ImageStore) Open(hash, mimeType string) (io.ReadCloser, error) { return os.Open(s.path(hash, mimeType)) } -func (s *ImageStore) Remove(hash, mimeType string) error { - err := os.Remove(s.path(hash, mimeType)) +// Remove deletes the store file unless it is newer than olderThan, in which case +// an overlapping acquisition may have just touched it and be about to commit its row. +func (s *ImageStore) Remove(hash, mimeType string, olderThan time.Time) error { + path := s.path(hash, mimeType) + info, err := os.Stat(path) + if errors.Is(err, fs.ErrNotExist) { + return nil + } + if err != nil { + return err + } + if info.ModTime().After(olderThan) { + return nil + } + err = os.Remove(path) if errors.Is(err, fs.ErrNotExist) { return nil } diff --git a/core/artwork/image_store_test.go b/core/artwork/image_store_test.go index 2647f3b7f..89646deba 100644 --- a/core/artwork/image_store_test.go +++ b/core/artwork/image_store_test.go @@ -51,13 +51,49 @@ var _ = Describe("ImageStore", func() { Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed()) }) + It("refreshes the mtime on a duplicate write", func() { + data := []byte("touch-me") + h, _ := HashImage(bytes.NewReader(data)) + Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed()) + old := time.Now().Add(-2 * time.Hour) + Expect(os.Chtimes(store.path(h, "image/png"), old, old)).To(Succeed()) + + Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed()) + + info, err := os.Stat(store.path(h, "image/png")) + Expect(err).ToNot(HaveOccurred()) + Expect(info.ModTime()).To(BeTemporally(">", time.Now().Add(-time.Minute))) + }) + It("returns fs.ErrNotExist for missing images", func() { _, err := store.Open("beefbeefbeefbeef", "image/jpeg") Expect(os.IsNotExist(err)).To(BeTrue()) }) It("removes without error when already gone", func() { - Expect(store.Remove("beefbeefbeefbeef", "image/jpeg")).To(Succeed()) + Expect(store.Remove("beefbeefbeefbeef", "image/jpeg", time.Now())).To(Succeed()) + }) + + It("spares a file newer than the cutoff, removes an aged one", func() { + fresh := []byte("fresh") + hf, _ := HashImage(bytes.NewReader(fresh)) + Expect(store.Write(hf, "image/jpeg", bytes.NewReader(fresh))).To(Succeed()) + + aged := []byte("aged") + ha, _ := HashImage(bytes.NewReader(aged)) + Expect(store.Write(ha, "image/jpeg", bytes.NewReader(aged))).To(Succeed()) + old := time.Now().Add(-2 * time.Hour) + Expect(os.Chtimes(store.path(ha, "image/jpeg"), old, old)).To(Succeed()) + + cutoff := time.Now().Add(-time.Hour) + Expect(store.Remove(hf, "image/jpeg", cutoff)).To(Succeed()) + Expect(store.Remove(ha, "image/jpeg", cutoff)).To(Succeed()) + + rc, err := store.Open(hf, "image/jpeg") + Expect(err).ToNot(HaveOccurred()) + rc.Close() + _, err = store.Open(ha, "image/jpeg") + Expect(os.IsNotExist(err)).To(BeTrue()) }) It("sweeps unknown files, keeps known ones", func() { diff --git a/core/artwork/prune.go b/core/artwork/prune.go index de8811e14..70aac091e 100644 --- a/core/artwork/prune.go +++ b/core/artwork/prune.go @@ -39,7 +39,9 @@ func Prune(ctx context.Context, ds model.DataStore, store *ImageStore) error { if _, ok := survivors[h]; ok { continue } - if err := store.Remove(h, arts[h].Mime); err != nil { + // A spared fresh file is at worst a stray a later sweep reclaims; full + // worker/prune mutual exclusion is Phase 2's concern. + if err := store.Remove(h, arts[h].Mime, cutoff); err != nil { log.Warn(ctx, "Prune: could not remove artwork file", "hash", h, err) } removed++ diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index aee8cd17c..9d4aad350 100644 --- a/core/artwork/prune_test.go +++ b/core/artwork/prune_test.go @@ -36,6 +36,8 @@ var _ = Describe("Prune", func() { data := []byte("orphan-bytes") h, _ := HashImage(bytes.NewReader(data)) Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed()) + old := time.Now().Add(-2 * time.Hour) + Expect(os.Chtimes(store.path(h, "image/jpeg"), old, old)).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} @@ -76,6 +78,23 @@ var _ = Describe("Prune", func() { rc.Close() }) + It("spares an orphan file freshly touched by an overlapping acquisition", func() { + data := []byte("racing-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} + // The row is legitimately orphaned, but a concurrent acquisition just touched the + // file's mtime (duplicate Write) and is about to commit a row referencing it. + + Expect(Prune(context.Background(), ds, store)).To(Succeed()) + + 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)) From 8147f7c40bf2bafd58dc43c701dce5ad5e3908c3 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 22 Jul 2026 00:28:51 -0400 Subject: [PATCH 16/21] fix(artwork): rewrite vanished duplicates and sweep stale mime variants Write falls through to a real write when the liveness touch fails, and sweep retention now matches the recorded mime's extension so obsolete variants are reclaimed. --- core/artwork/image_store.go | 11 ++++--- core/artwork/image_store_test.go | 44 ++++++++++++++++++++++++-- core/artwork/prune.go | 13 +++----- core/artwork/prune_test.go | 22 ++++++++++++- model/artwork.go | 4 +-- persistence/artwork_repository.go | 19 ++++++++--- persistence/artwork_repository_test.go | 7 ++-- tests/mock_artwork_repo.go | 10 +++--- 8 files changed, 99 insertions(+), 31 deletions(-) diff --git a/core/artwork/image_store.go b/core/artwork/image_store.go index ef27ed00a..178c19d10 100644 --- a/core/artwork/image_store.go +++ b/core/artwork/image_store.go @@ -56,8 +56,10 @@ func (s *ImageStore) Write(hash, mimeType string, r io.Reader) error { if _, err := os.Stat(dst); err == nil { // A touched mtime marks the file live so a concurrent prune spares it. now := time.Now() - _ = os.Chtimes(dst, now, now) - return nil + if err := os.Chtimes(dst, now, now); err == nil { + return nil + } + // touch failed (file likely pruned concurrently) — fall through and write it } if err := os.MkdirAll(filepath.Dir(dst), 0755); err != nil { return err @@ -104,7 +106,7 @@ func (s *ImageStore) Remove(hash, mimeType string, olderThan time.Time) 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) { +func (s *ImageStore) Sweep(cutoff time.Time, keep func(hash, ext 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() { @@ -120,7 +122,8 @@ func (s *ImageStore) Sweep(cutoff time.Time, keep func(hash string) bool) (int, name := d.Name() remove := strings.HasPrefix(name, ".") // abandoned temp file past the grace window if !remove { - remove = !keep(strings.TrimSuffix(name, filepath.Ext(name))) + ext := filepath.Ext(name) + remove = !keep(strings.TrimSuffix(name, ext), ext) } if remove { // #nosec G122 -- path comes from WalkDir over our own store root, no attacker-controlled symlinks diff --git a/core/artwork/image_store_test.go b/core/artwork/image_store_test.go index 89646deba..0bf721623 100644 --- a/core/artwork/image_store_test.go +++ b/core/artwork/image_store_test.go @@ -65,6 +65,22 @@ var _ = Describe("ImageStore", func() { Expect(info.ModTime()).To(BeTemporally(">", time.Now().Add(-time.Minute))) }) + It("rewrites the bytes when the existing file vanished before the liveness touch", func() { + data := []byte("vanishing") + h, _ := HashImage(bytes.NewReader(data)) + for range 10 { + Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed()) + Expect(os.Remove(store.path(h, "image/png"))).To(Succeed()) + Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed()) + + rc, err := store.Open(h, "image/png") + Expect(err).ToNot(HaveOccurred()) + got, _ := io.ReadAll(rc) + rc.Close() + Expect(got).To(Equal(data)) + } + }) + It("returns fs.ErrNotExist for missing images", func() { _, err := store.Open("beefbeefbeefbeef", "image/jpeg") Expect(os.IsNotExist(err)).To(BeTrue()) @@ -107,7 +123,7 @@ var _ = Describe("ImageStore", func() { 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 }) + 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") @@ -117,12 +133,34 @@ var _ = Describe("ImageStore", func() { rc.Close() }) + It("sweeps a stale mime variant of a known hash, keeps the current one", func() { + data := []byte("same-bytes") + h, _ := HashImage(bytes.NewReader(data)) + Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed()) + Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed()) + old := time.Now().Add(-2 * time.Hour) + Expect(os.Chtimes(store.path(h, "image/png"), old, old)).To(Succeed()) + Expect(os.Chtimes(store.path(h, "image/jpeg"), old, old)).To(Succeed()) + + // The recorded mime is image/jpeg, so the .png variant is obsolete. + removed, err := store.Sweep(time.Now().Add(-time.Hour), func(hash, ext string) bool { + return hash == h && ext == ".jpg" + }) + Expect(err).ToNot(HaveOccurred()) + Expect(removed).To(Equal(1)) + _, err = store.Open(h, "image/png") + Expect(os.IsNotExist(err)).To(BeTrue()) + rc, err := store.Open(h, "image/jpeg") + 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 }) + removed, err := store.Sweep(time.Now().Add(-time.Hour), func(string, string) bool { return false }) Expect(err).ToNot(HaveOccurred()) Expect(removed).To(Equal(0)) rc, err := store.Open(h, "image/jpeg") @@ -139,7 +177,7 @@ var _ = Describe("ImageStore", func() { 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 }) + removed, err := store.Sweep(time.Now().Add(-time.Hour), func(string, string) bool { return true }) Expect(err).ToNot(HaveOccurred()) Expect(removed).To(Equal(1)) Expect(oldTmp).ToNot(BeAnExistingFile()) diff --git a/core/artwork/prune.go b/core/artwork/prune.go index 70aac091e..db1e4fdc1 100644 --- a/core/artwork/prune.go +++ b/core/artwork/prune.go @@ -49,17 +49,14 @@ func Prune(ctx context.Context, ds model.DataStore, store *ImageStore) error { log.Info(ctx, "Prune: removed orphan artwork", "count", removed) } - hashes, err := repo.GetAllHashes() + mimes, err := repo.GetAllMimes() if err != nil { return err } - known := make(map[string]struct{}, len(hashes)) - for _, h := range hashes { - known[h] = struct{}{} - } - removed, err := store.Sweep(cutoff, func(hash string) bool { - _, ok := known[hash] - return ok + removed, err := store.Sweep(cutoff, func(hash, ext string) bool { + // A known hash under a stale extension is a superseded mime variant — reclaim it. + m, ok := mimes[hash] + return ok && ext == extForMime(m) }) if err != nil { return err diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index 9d4aad350..2e5c80d23 100644 --- a/core/artwork/prune_test.go +++ b/core/artwork/prune_test.go @@ -17,7 +17,7 @@ type flakyGetArtworkRepo struct { *tests.MockArtworkRepo } -func (f *flakyGetArtworkRepo) GetAllHashes() ([]string, error) { +func (f *flakyGetArtworkRepo) GetAllMimes() (map[string]string, error) { return nil, errors.New("db locked") } @@ -108,6 +108,26 @@ var _ = Describe("Prune", func() { Expect(os.IsNotExist(err)).To(BeTrue()) }) + It("sweeps an obsolete mime variant of a reacquired hash", func() { + data := []byte("variant-bytes") + h, _ := HashImage(bytes.NewReader(data)) + Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed()) + Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed()) + old := time.Now().Add(-2 * time.Hour) + Expect(os.Chtimes(store.path(h, "image/png"), old, old)).To(Succeed()) + Expect(os.Chtimes(store.path(h, "image/jpeg"), old, old)).To(Succeed()) + // The row records the current mime; the .png file is a superseded variant. + Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg"})).To(Succeed()) + + Expect(Prune(context.Background(), ds, store)).To(Succeed()) + + _, err := store.Open(h, "image/png") + Expect(os.IsNotExist(err)).To(BeTrue()) + rc, err := store.Open(h, "image/jpeg") + Expect(err).ToNot(HaveOccurred()) + rc.Close() + }) + It("never sweeps files on a transient DB error", func() { ds.MockedArtwork = &flakyGetArtworkRepo{MockArtworkRepo: tests.CreateMockArtworkRepo()} diff --git a/model/artwork.go b/model/artwork.go index 5b50fbda0..f5f3ab4db 100644 --- a/model/artwork.go +++ b/model/artwork.go @@ -73,8 +73,8 @@ type ArtworkRepository interface { DeleteForItem(kind, id string) error // GetInfoForItems hydrates a page: one batched query, item_artwork joined to artwork. GetInfoForItems(kind string, ids []string) (map[string]ItemArtworkInfo, error) - // GetAllHashes returns every stored artwork hash, for sweep membership checks. - GetAllHashes() ([]string, error) + // GetAllMimes returns hash -> current mime for every stored artwork, for sweep retention checks. + GetAllMimes() (map[string]string, error) } type ArtworkQueueRepository interface { diff --git a/persistence/artwork_repository.go b/persistence/artwork_repository.go index a18b9d4af..05912a0d6 100644 --- a/persistence/artwork_repository.go +++ b/persistence/artwork_repository.go @@ -74,11 +74,20 @@ func (r *artworkRepository) GetImages(hashes []string) (map[string]model.Artwork return res, nil } -func (r *artworkRepository) GetAllHashes() ([]string, error) { - sel := Select("hash").From(r.tableName) - var hashes []string - err := r.queryAllSlice(sel, &hashes) - return hashes, err +func (r *artworkRepository) GetAllMimes() (map[string]string, error) { + sel := Select("hash", "mime").From(r.tableName) + var rows []struct { + Hash string + Mime string + } + if err := r.queryAll(sel, &rows); err != nil { + return nil, err + } + res := make(map[string]string, len(rows)) + for _, row := range rows { + res[row.Hash] = row.Mime + } + return res, nil } func (r *artworkRepository) GetOrphanHashes(createdBefore time.Time) ([]string, error) { diff --git a/persistence/artwork_repository_test.go b/persistence/artwork_repository_test.go index b3971f88f..a13b1e7cb 100644 --- a/persistence/artwork_repository_test.go +++ b/persistence/artwork_repository_test.go @@ -76,12 +76,13 @@ var _ = Describe("ArtworkRepository", func() { Expect(got["b2"].Mime).To(Equal("image/png")) }) - It("returns every stored hash", func() { + 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()) - hashes, err := repo.GetAllHashes() + mimes, err := repo.GetAllMimes() Expect(err).ToNot(HaveOccurred()) - Expect(hashes).To(ConsistOf("all1", "all2")) + Expect(mimes).To(HaveKeyWithValue("all1", "image/jpeg")) + Expect(mimes).To(HaveKeyWithValue("all2", "image/png")) }) It("finds orphans older than cutoff, honoring item_artwork references", func() { diff --git a/tests/mock_artwork_repo.go b/tests/mock_artwork_repo.go index 6575416b9..851ee7f48 100644 --- a/tests/mock_artwork_repo.go +++ b/tests/mock_artwork_repo.go @@ -58,15 +58,15 @@ func (m *MockArtworkRepo) GetOrphanHashes(createdBefore time.Time) ([]string, er return m.OrphanHashes, m.Err } -func (m *MockArtworkRepo) GetAllHashes() ([]string, error) { +func (m *MockArtworkRepo) GetAllMimes() (map[string]string, error) { if m.Err != nil { return nil, m.Err } - hashes := make([]string, 0, len(m.Data)) - for h := range m.Data { - hashes = append(hashes, h) + mimes := make(map[string]string, len(m.Data)) + for h, a := range m.Data { + mimes[h] = a.Mime } - return hashes, nil + return mimes, nil } func (m *MockArtworkRepo) DeleteOrphans(createdBefore time.Time, hashes []string) error { From 034cd17498cbee76b0044edfc9fd850407031630 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 22 Jul 2026 00:36:22 -0400 Subject: [PATCH 17/21] fix(artwork): index artwork_queue in dequeue order The previous leading retry_at range column forced a temp B-tree sort of the whole eligible set on every DequeueBatch; ordering the index by (priority DESC, enqueued_at) lets scans stop after the batch size. --- db/migrations/20260722023032_add_artwork_tables.sql | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/db/migrations/20260722023032_add_artwork_tables.sql b/db/migrations/20260722023032_add_artwork_tables.sql index 94e0ebe5f..7133b71ce 100644 --- a/db/migrations/20260722023032_add_artwork_tables.sql +++ b/db/migrations/20260722023032_add_artwork_tables.sql @@ -33,7 +33,8 @@ CREATE TABLE artwork_queue ( enqueued_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, PRIMARY KEY (item_kind, item_id, image_type) ) WITHOUT ROWID; -CREATE INDEX ix_artwork_queue_drain ON artwork_queue(retry_at, priority, enqueued_at); +-- Ordered to match DequeueBatch (priority DESC, enqueued_at) so drains stop after n rows; retry_at makes it covering. +CREATE INDEX ix_artwork_queue_drain ON artwork_queue(priority DESC, enqueued_at, retry_at); -- +goose Down DROP TABLE artwork_queue; From 0147cc59b1a6dbefb0d688588b8d1c6d3f2bf066 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 22 Jul 2026 00:42:42 -0400 Subject: [PATCH 18/21] fix(artwork): honor the orphan cutoff in the repository mock The mock's DeleteOrphans now applies createdBefore like the SQL implementation, and a new spec covers a freshly reacquired row surviving prune. --- core/artwork/prune_test.go | 18 ++++++++++++++++++ tests/mock_artwork_repo.go | 5 ++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index 2e5c80d23..4a45ddd13 100644 --- a/core/artwork/prune_test.go +++ b/core/artwork/prune_test.go @@ -78,6 +78,24 @@ var _ = Describe("Prune", func() { rc.Close() }) + It("spares a candidate whose row was freshly recreated (created_at 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. + Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg", + CreatedAt: time.Now()})).To(Succeed()) + awRepo.OrphanHashes = []string{h} + + 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("spares an orphan file freshly touched by an overlapping acquisition", func() { data := []byte("racing-bytes") h, _ := HashImage(bytes.NewReader(data)) diff --git a/tests/mock_artwork_repo.go b/tests/mock_artwork_repo.go index 851ee7f48..9565abd7b 100644 --- a/tests/mock_artwork_repo.go +++ b/tests/mock_artwork_repo.go @@ -73,11 +73,14 @@ func (m *MockArtworkRepo) DeleteOrphans(createdBefore time.Time, hashes []string if m.Err != nil { return m.Err } - // Mirror the SQL reference re-check; createdBefore is ignored in the mock for simplicity. + // 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) { + continue + } delete(m.Data, h) } return nil From ebbe533c6a8c1876732d71385c2571673977ede5 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 22 Jul 2026 00:51:13 -0400 Subject: [PATCH 19/21] fix(artwork): reject malformed hashes in ImageStore operations Known-absent states carry an empty hash and malformed persisted hashes could panic path sharding or inject separators; Write/Open/Remove now return an error for anything but 16 lowercase hex chars. --- core/artwork/image_store.go | 23 +++++++++++++++++++++++ core/artwork/image_store_test.go | 9 +++++++++ 2 files changed, 32 insertions(+) diff --git a/core/artwork/image_store.go b/core/artwork/image_store.go index 178c19d10..4b4e482c9 100644 --- a/core/artwork/image_store.go +++ b/core/artwork/image_store.go @@ -47,11 +47,28 @@ func extForMime(m string) string { return ".img" } +// validHash rejects anything but 16 lowercase hex chars: known-absent states carry "", +// and malformed persisted hashes must never reach path sharding (slice panics, separators). +func validHash(hash string) bool { + if len(hash) != 16 { + return false + } + for _, c := range []byte(hash) { + if (c < '0' || c > '9') && (c < 'a' || c > 'f') { + return false + } + } + return true +} + func (s *ImageStore) path(hash, mimeType string) string { return filepath.Join(s.root, hash[0:2], hash[2:4], hash+extForMime(mimeType)) } func (s *ImageStore) Write(hash, mimeType string, r io.Reader) error { + if !validHash(hash) { + return fmt.Errorf("imagestore: invalid hash %q", hash) + } dst := s.path(hash, mimeType) if _, err := os.Stat(dst); err == nil { // A touched mtime marks the file live so a concurrent prune spares it. @@ -80,12 +97,18 @@ func (s *ImageStore) Write(hash, mimeType string, r io.Reader) error { } func (s *ImageStore) Open(hash, mimeType string) (io.ReadCloser, error) { + if !validHash(hash) { + return nil, fmt.Errorf("imagestore: invalid hash %q", hash) + } return os.Open(s.path(hash, mimeType)) } // Remove deletes the store file unless it is newer than olderThan, in which case // an overlapping acquisition may have just touched it and be about to commit its row. func (s *ImageStore) Remove(hash, mimeType string, olderThan time.Time) error { + if !validHash(hash) { + return fmt.Errorf("imagestore: invalid hash %q", hash) + } path := s.path(hash, mimeType) info, err := os.Stat(path) if errors.Is(err, fs.ErrNotExist) { diff --git a/core/artwork/image_store_test.go b/core/artwork/image_store_test.go index 0bf721623..214cab3ad 100644 --- a/core/artwork/image_store_test.go +++ b/core/artwork/image_store_test.go @@ -90,6 +90,15 @@ var _ = Describe("ImageStore", func() { Expect(store.Remove("beefbeefbeefbeef", "image/jpeg", time.Now())).To(Succeed()) }) + It("rejects invalid hashes instead of panicking", func() { + for _, h := range []string{"", "ab", "BEEFBEEFBEEFBEEF", "../../../../etcpw", "beefbeefbeefbee/"} { + Expect(store.Write(h, "image/jpeg", bytes.NewReader([]byte("x")))).To(MatchError(ContainSubstring("invalid hash"))) + _, err := store.Open(h, "image/jpeg") + Expect(err).To(MatchError(ContainSubstring("invalid hash"))) + Expect(store.Remove(h, "image/jpeg", time.Now())).To(MatchError(ContainSubstring("invalid hash"))) + } + }) + It("spares a file newer than the cutoff, removes an aged one", func() { fresh := []byte("fresh") hf, _ := HashImage(bytes.NewReader(fresh)) From c04c8ee02a68e7c97dc99f8b7f06066894aecf63 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 22 Jul 2026 00:58:22 -0400 Subject: [PATCH 20/21] fix(artwork): mock PutImage refreshes created_at like the SQL repository Prune specs now age fixtures directly instead of seeding stale timestamps through the upsert. --- core/artwork/prune_test.go | 22 ++++++++++++++-------- tests/mock_artwork_repo.go | 5 ++--- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index 4a45ddd13..1f621363b 100644 --- a/core/artwork/prune_test.go +++ b/core/artwork/prune_test.go @@ -32,14 +32,21 @@ var _ = Describe("Prune", func() { store = NewImageStore(GinkgoT().TempDir()) }) + // PutImage refreshes created_at like the SQL repo, so fixtures are aged directly. + ageArtwork := func(h string, t time.Time) { + a := awRepo.Data[h] + a.CreatedAt = t + awRepo.Data[h] = a + } + It("deletes orphan rows and their store files, keeps referenced ones", func() { data := []byte("orphan-bytes") h, _ := HashImage(bytes.NewReader(data)) Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed()) old := time.Now().Add(-2 * time.Hour) Expect(os.Chtimes(store.path(h, "image/jpeg"), old, old)).To(Succeed()) - Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg", - CreatedAt: time.Now().Add(-2 * time.Hour)})).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") @@ -62,8 +69,8 @@ var _ = Describe("Prune", 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()) + Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg"})).To(Succeed()) + ageArtwork(h, time.Now().Add(-2*time.Hour)) 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", @@ -83,8 +90,7 @@ var _ = Describe("Prune", func() { 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. - Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg", - CreatedAt: time.Now()})).To(Succeed()) + Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg"})).To(Succeed()) awRepo.OrphanHashes = []string{h} Expect(Prune(context.Background(), ds, store)).To(Succeed()) @@ -100,8 +106,8 @@ var _ = Describe("Prune", func() { data := []byte("racing-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()) + 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 legitimately orphaned, but a concurrent acquisition just touched the // file's mtime (duplicate Write) and is about to commit a row referencing it. diff --git a/tests/mock_artwork_repo.go b/tests/mock_artwork_repo.go index 9565abd7b..13ad4d5e1 100644 --- a/tests/mock_artwork_repo.go +++ b/tests/mock_artwork_repo.go @@ -34,9 +34,8 @@ func (m *MockArtworkRepo) PutImage(a *model.Artwork) error { if m.Err != nil { return m.Err } - if a.CreatedAt.IsZero() { - a.CreatedAt = time.Now() - } + // Mirrors the SQL repository: every upsert refreshes created_at. Age fixtures via Data directly. + a.CreatedAt = time.Now() m.Data[a.Hash] = *a return nil } From 1f818e7633ae917e29b67004b18f894e93ac918d Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 22 Jul 2026 15:52:54 -0400 Subject: [PATCH 21/21] fix(artwork): store backing-file provenance per item, not per hash --- .../20260722023032_add_artwork_tables.sql | 4 ++-- model/artwork.go | 20 ++++++++++--------- persistence/artwork_repository.go | 5 ++--- persistence/artwork_repository_test.go | 8 ++++++-- 4 files changed, 21 insertions(+), 16 deletions(-) diff --git a/db/migrations/20260722023032_add_artwork_tables.sql b/db/migrations/20260722023032_add_artwork_tables.sql index 7133b71ce..bc400f90e 100644 --- a/db/migrations/20260722023032_add_artwork_tables.sql +++ b/db/migrations/20260722023032_add_artwork_tables.sql @@ -6,8 +6,6 @@ CREATE TABLE artwork ( height INTEGER NOT NULL DEFAULT 0, size_bytes INTEGER NOT NULL DEFAULT 0, blur_hash TEXT NOT NULL DEFAULT '', - source_path TEXT NOT NULL DEFAULT '', - ref_mtime INTEGER NOT NULL DEFAULT 0, created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ); @@ -17,6 +15,8 @@ CREATE TABLE item_artwork ( image_type TEXT NOT NULL DEFAULT 'primary', hash TEXT NOT NULL DEFAULT '', source TEXT NOT NULL DEFAULT '', + source_path TEXT NOT NULL DEFAULT '', + ref_mtime INTEGER NOT NULL DEFAULT 0, attempted_at TIMESTAMP, updated_at TIMESTAMP, PRIMARY KEY (item_kind, item_id, image_type) diff --git a/model/artwork.go b/model/artwork.go index f5f3ab4db..7cb117764 100644 --- a/model/artwork.go +++ b/model/artwork.go @@ -4,15 +4,13 @@ import "time" // Artwork is one unique image, identified by the XXH3-64 hash of its bytes. type Artwork struct { - Hash string `structs:"hash"` - Mime string `structs:"mime"` - Width int `structs:"width"` - Height int `structs:"height"` - SizeBytes int64 `structs:"size_bytes"` - BlurHash string `structs:"blur_hash"` - SourcePath string `structs:"source_path"` - RefMtime int64 `structs:"ref_mtime"` - CreatedAt time.Time `structs:"created_at"` + Hash string `structs:"hash"` + Mime string `structs:"mime"` + Width int `structs:"width"` + Height int `structs:"height"` + SizeBytes int64 `structs:"size_bytes"` + BlurHash string `structs:"blur_hash"` + CreatedAt time.Time `structs:"created_at"` } const ImageTypePrimary = "primary" @@ -24,6 +22,10 @@ type ItemArtwork struct { ImageType string `structs:"image_type"` Hash string `structs:"hash"` Source string `structs:"source"` + // SourcePath is the backing file (folder/upload: the image; embedded: the audio file); "" otherwise. + SourcePath string `structs:"source_path"` + // RefMtime is SourcePath's mtime at resolution; 0 when there is no SourcePath. + RefMtime int64 `structs:"ref_mtime"` // attempted_at/updated_at are nullable in the schema but always set by PutItemArtwork; // raw inserts must set them too, since these non-pointer time.Time fields fail to scan NULL. AttemptedAt time.Time `structs:"attempted_at"` diff --git a/persistence/artwork_repository.go b/persistence/artwork_repository.go index 05912a0d6..505e3839d 100644 --- a/persistence/artwork_repository.go +++ b/persistence/artwork_repository.go @@ -53,8 +53,7 @@ func (r *artworkRepository) PutImage(a *model.Artwork) error { } // 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, created_at=excluded.created_at`) + height=excluded.height, size_bytes=excluded.size_bytes, blur_hash=excluded.blur_hash, created_at=excluded.created_at`) _, err = r.executeSQL(ins) return err } @@ -139,7 +138,7 @@ func (r *artworkRepository) PutItemArtwork(ia *model.ItemArtwork) error { return err } ins := Insert(itemArtworkTable).SetMap(values).Suffix(`ON CONFLICT (item_kind, item_id, image_type) DO UPDATE SET - hash=excluded.hash, source=excluded.source, + hash=excluded.hash, source=excluded.source, source_path=excluded.source_path, ref_mtime=excluded.ref_mtime, attempted_at=excluded.attempted_at, updated_at=excluded.updated_at`) _, err = r.items.executeSQL(ins) return err diff --git a/persistence/artwork_repository_test.go b/persistence/artwork_repository_test.go index a13b1e7cb..82c2888c8 100644 --- a/persistence/artwork_repository_test.go +++ b/persistence/artwork_repository_test.go @@ -137,16 +137,20 @@ var _ = Describe("ArtworkRepository", func() { }) Context("item state", func() { - It("upserts and reads state", func() { + It("upserts and reads state, including per-item provenance", func() { ia := &model.ItemArtwork{ItemKind: "al", ItemID: "al1", ImageType: model.ImageTypePrimary, - Hash: "h1", Source: "folder", AttemptedAt: time.Now()} + Hash: "h1", Source: "folder", SourcePath: "/music/a/cover.jpg", RefMtime: 111, AttemptedAt: time.Now()} Expect(repo.PutItemArtwork(ia)).To(Succeed()) ia.Source = "embedded" + ia.SourcePath = "/music/a/track.mp3" + ia.RefMtime = 222 Expect(repo.PutItemArtwork(ia)).To(Succeed()) got, err := repo.GetItemArtwork("al", "al1", model.ImageTypePrimary) Expect(err).ToNot(HaveOccurred()) Expect(got.Source).To(Equal("embedded")) + Expect(got.SourcePath).To(Equal("/music/a/track.mp3")) + Expect(got.RefMtime).To(Equal(int64(222))) Expect(got.UpdatedAt).ToNot(BeZero()) })