mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-01 07:21:17 +00:00
fix(artwork): TTL for no-result memoization, prompt Close, no warmup force spike
- memoize every no-result outcome (ErrUnavailable can wrap transient agent and storage failures, so it is not a reliable definitive/transient discriminator) but bound it with a 1h TTL, which also un-poisons entries recorded under future file mtimes - cancel the worker context and check done in the drain loop, so Close returns promptly instead of draining the backlog at up to 30s per item - only force recompute when the image cache is operational: during warmup every serve is a miss, which caused a recompute spike at startup
This commit is contained in:
parent
43500c0ffe
commit
f9f6d36ebb
@ -83,10 +83,9 @@ func (a *artwork) Get(ctx context.Context, artID model.ArtworkID, size int, squa
|
||||
return nil, time.Time{}, err
|
||||
}
|
||||
if a.blurHashes != nil {
|
||||
// A cache miss means the image is new or changed, even when no entity row moved (e.g. an
|
||||
// in-place cover.jpg swap) — force a recompute so the stored blurhash follows the image.
|
||||
// The reader's LastUpdated covers the same case when the image cache is disabled.
|
||||
force := !r.Cached && !a.cache.Disabled(ctx)
|
||||
// A miss on an operational cache means a new/changed image even when no entity row moved;
|
||||
// while warming up or disabled every serve misses, so only the LastUpdated signal applies.
|
||||
force := !r.Cached && a.cache.Available(ctx)
|
||||
a.blurHashes.Enqueue(artID, artReader.LastUpdated(), force)
|
||||
}
|
||||
return r, artReader.LastUpdated(), nil
|
||||
|
||||
@ -2,7 +2,6 @@ package artwork
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"image"
|
||||
"sync"
|
||||
@ -15,9 +14,6 @@ import (
|
||||
"github.com/navidrome/navidrome/model/request"
|
||||
)
|
||||
|
||||
// blurHashUpdater keeps stored blurhashes in sync with the artwork actually served. Enqueue is
|
||||
// cheap (dedup map insert); the worker re-checks freshness against the DB and only decodes when
|
||||
// the hash is missing or was computed from an older artwork version.
|
||||
// enqueueRequest carries the staleness signals seen at serve time: force (image-cache miss) and
|
||||
// the reader's LastUpdated, which tracks file mtimes that no entity row timestamp reflects.
|
||||
type enqueueRequest struct {
|
||||
@ -25,23 +21,35 @@ type enqueueRequest struct {
|
||||
imageUpdatedAt time.Time
|
||||
}
|
||||
|
||||
// blurHashUpdater keeps stored blurhashes in sync with the artwork actually served: Enqueue is a
|
||||
// cheap dedup insert, and a single worker re-checks freshness before decoding.
|
||||
type blurHashUpdater struct {
|
||||
a *artwork
|
||||
mutex sync.Mutex
|
||||
buffer map[model.ArtworkID]enqueueRequest
|
||||
noResult map[model.ArtworkID]time.Time
|
||||
wake chan struct{}
|
||||
done chan struct{}
|
||||
runDone chan struct{}
|
||||
started bool
|
||||
stopped bool
|
||||
a *artwork
|
||||
mutex sync.Mutex
|
||||
buffer map[model.ArtworkID]enqueueRequest
|
||||
noResult map[model.ArtworkID]noResultEntry
|
||||
wake chan struct{}
|
||||
done chan struct{}
|
||||
runDone chan struct{}
|
||||
runCancel context.CancelFunc
|
||||
started bool
|
||||
stopped bool
|
||||
}
|
||||
|
||||
// noResultTTL bounds how long a failed or empty computation suppresses retries, so transient
|
||||
// outages (agents, storage) self-heal despite being indistinguishable from "no artwork".
|
||||
const noResultTTL = time.Hour
|
||||
|
||||
type noResultEntry struct {
|
||||
sig time.Time
|
||||
at time.Time
|
||||
}
|
||||
|
||||
func newBlurHashUpdater(a *artwork) *blurHashUpdater {
|
||||
return &blurHashUpdater{
|
||||
a: a,
|
||||
buffer: make(map[model.ArtworkID]enqueueRequest),
|
||||
noResult: make(map[model.ArtworkID]time.Time),
|
||||
noResult: make(map[model.ArtworkID]noResultEntry),
|
||||
wake: make(chan struct{}, 1),
|
||||
done: make(chan struct{}),
|
||||
runDone: make(chan struct{}),
|
||||
@ -61,9 +69,11 @@ func (u *blurHashUpdater) Enqueue(artID model.ArtworkID, imageUpdatedAt time.Tim
|
||||
}
|
||||
if !u.started {
|
||||
u.started = true
|
||||
// Playlist artwork readers require a user in the context. Lazy-starting keeps idle Artwork
|
||||
// Admin context: playlist artwork readers require a user. Lazy-starting keeps idle Artwork
|
||||
// instances goroutine-free; stop() ends the worker (tests must call it, the server never does).
|
||||
go u.run(request.WithUser(context.Background(), model.User{IsAdmin: true}))
|
||||
ctx, cancel := context.WithCancel(request.WithUser(context.Background(), model.User{IsAdmin: true}))
|
||||
u.runCancel = cancel
|
||||
go u.run(ctx)
|
||||
}
|
||||
req := u.buffer[artID]
|
||||
req.force = req.force || force
|
||||
@ -88,9 +98,11 @@ func (u *blurHashUpdater) stop() {
|
||||
}
|
||||
u.stopped = true
|
||||
started := u.started
|
||||
cancel := u.runCancel
|
||||
u.mutex.Unlock()
|
||||
close(u.done)
|
||||
if started {
|
||||
cancel()
|
||||
<-u.runDone
|
||||
}
|
||||
}
|
||||
@ -104,6 +116,11 @@ func (u *blurHashUpdater) run(ctx context.Context) {
|
||||
case <-u.wake:
|
||||
}
|
||||
for {
|
||||
select {
|
||||
case <-u.done:
|
||||
return
|
||||
default:
|
||||
}
|
||||
artID, req, ok := u.next()
|
||||
if !ok {
|
||||
break
|
||||
@ -153,19 +170,14 @@ func (u *blurHashUpdater) process(ctx context.Context, artID model.ArtworkID, re
|
||||
if stored != "" && storedAt != nil && !storedAt.Before(version) && !sig.After(*storedAt) {
|
||||
return
|
||||
}
|
||||
if last, ok := u.lastNoResult(artID); ok && !sig.After(last) {
|
||||
if last, ok := u.lastNoResult(artID); ok && !sig.After(last.sig) && time.Since(last.at) < noResultTTL {
|
||||
return
|
||||
}
|
||||
}
|
||||
hash, err := u.computeFromArtwork(ctx, artID)
|
||||
if err != nil && !errors.Is(err, ErrUnavailable) {
|
||||
// Transient failure (timeout, storage/agent hiccup) — leave un-memoized so a later serve retries.
|
||||
log.Trace(ctx, "BlurHash: compute failed", "artID", artID, err)
|
||||
return
|
||||
}
|
||||
if err != nil || hash == "" {
|
||||
// Definitively no artwork (or a placeholder) for this state — remember it, so browsing
|
||||
// artwork-less entities doesn't re-resolve them on every serve.
|
||||
// Any no-result (no artwork, placeholder, decode failure, transient outage) is memoized
|
||||
// with a TTL: browsing stays cheap, and failures still retry once it expires.
|
||||
log.Trace(ctx, "BlurHash: nothing to persist", "artID", artID, err)
|
||||
u.setNoResult(artID, sig)
|
||||
return
|
||||
@ -177,24 +189,24 @@ func (u *blurHashUpdater) process(ctx context.Context, artID model.ArtworkID, re
|
||||
u.clearNoResult(artID)
|
||||
}
|
||||
|
||||
func (u *blurHashUpdater) lastNoResult(artID model.ArtworkID) (time.Time, bool) {
|
||||
func (u *blurHashUpdater) lastNoResult(artID model.ArtworkID) (noResultEntry, bool) {
|
||||
u.mutex.Lock()
|
||||
defer u.mutex.Unlock()
|
||||
t, ok := u.noResult[artID]
|
||||
return t, ok
|
||||
e, ok := u.noResult[artID]
|
||||
return e, ok
|
||||
}
|
||||
|
||||
// maxNoResultEntries bounds the negative cache; entries only accumulate for artwork-less entities,
|
||||
// so a wholesale reset just costs those entities one extra verification pass each.
|
||||
const maxNoResultEntries = 25_000
|
||||
|
||||
func (u *blurHashUpdater) setNoResult(artID model.ArtworkID, version time.Time) {
|
||||
func (u *blurHashUpdater) setNoResult(artID model.ArtworkID, sig time.Time) {
|
||||
u.mutex.Lock()
|
||||
defer u.mutex.Unlock()
|
||||
if len(u.noResult) >= maxNoResultEntries {
|
||||
clear(u.noResult)
|
||||
}
|
||||
u.noResult[artID] = version
|
||||
u.noResult[artID] = noResultEntry{sig: sig, at: time.Now()}
|
||||
}
|
||||
|
||||
func (u *blurHashUpdater) clearNoResult(artID model.ArtworkID) {
|
||||
|
||||
@ -1,6 +1,7 @@
|
||||
package artwork
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"time"
|
||||
|
||||
"github.com/navidrome/navidrome/model"
|
||||
@ -9,6 +10,13 @@ import (
|
||||
. "github.com/onsi/gomega"
|
||||
)
|
||||
|
||||
// failingFolderRepo makes the artwork reader chain fail with a clean (transient-style) error.
|
||||
type failingFolderRepo struct{ model.FolderRepository }
|
||||
|
||||
func (failingFolderRepo) GetAll(...model.QueryOptions) ([]model.Folder, error) {
|
||||
return nil, errors.New("boom")
|
||||
}
|
||||
|
||||
var _ = Describe("blurHashUpdater", func() {
|
||||
var u *blurHashUpdater
|
||||
var ds *tests.MockDataStore
|
||||
@ -19,7 +27,7 @@ var _ = Describe("blurHashUpdater", func() {
|
||||
u = &blurHashUpdater{
|
||||
a: &artwork{ds: ds},
|
||||
buffer: make(map[model.ArtworkID]enqueueRequest),
|
||||
noResult: make(map[model.ArtworkID]time.Time),
|
||||
noResult: make(map[model.ArtworkID]noResultEntry),
|
||||
wake: make(chan struct{}, 1),
|
||||
started: true,
|
||||
}
|
||||
@ -76,20 +84,21 @@ var _ = Describe("blurHashUpdater", func() {
|
||||
Expect(stored.BlurHash).To(BeEmpty())
|
||||
})
|
||||
|
||||
It("does not refresh the no-result entry when a retry fails transiently", func() {
|
||||
It("memoizes a failed retry under the newer signal", func() {
|
||||
al := model.Album{ID: "al-1", UpdatedAt: version}
|
||||
repo := tests.CreateMockAlbumRepo()
|
||||
repo.SetData(model.Albums{al})
|
||||
ds.MockedAlbum = repo
|
||||
ds.MockedFolder = failingFolderRepo{}
|
||||
u.setNoResult(al.CoverArtID(), version)
|
||||
|
||||
// A newer image mtime must bypass the no-result skip and attempt a compute; the compute
|
||||
// fails transiently here (no readers wired), so the no-result entry must NOT be refreshed.
|
||||
// A newer image mtime bypasses the no-result skip; the compute fails cleanly here, so
|
||||
// the entry is refreshed under the newer signal, with the TTL as the retry bound.
|
||||
newer := version.Add(time.Hour)
|
||||
u.process(GinkgoT().Context(), al.CoverArtID(), enqueueRequest{imageUpdatedAt: newer})
|
||||
last, ok := u.lastNoResult(al.CoverArtID())
|
||||
Expect(ok).To(BeTrue())
|
||||
Expect(last).To(Equal(version))
|
||||
Expect(last.sig).To(Equal(newer))
|
||||
})
|
||||
|
||||
It("does nothing when the entity is gone", func() {
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user