style(artwork): trim verbose comments to the 1-2 line budget

Comments only; no executable code changed. Verified by comparing the Go
token stream of every touched file before and after: identical.

Removes 375 of the 1104 comment lines this branch added, targeting content
that belongs in a commit message or PR body rather than in the code:
rejected alternatives ("DeleteIfUnchanged, not Delete", "Waking all beats
routing by kind"), refactor history ("as the legacy reader did"), issue
references (#5798, #5597, #5376), benchmark numbers (~400ms, ~16k allocs),
and four persistence doc comments that duplicated the interface godoc in
model/artwork.go verbatim.

Comments predating this branch are left untouched.

The ASCII fixture trees in the e2e suites are deliberately kept above the
line budget: they diagram the fixture layout with its expected outcomes,
and every pre-existing block in those files carries one.
This commit is contained in:
Deluan 2026-07-27 18:57:31 -04:00
parent 20c40521c6
commit 02fcf8bea7
73 changed files with 428 additions and 841 deletions

View File

@ -576,10 +576,8 @@ var _ = Describe("lastfmAgent", func() {
httpClient.Res = http.Response{Body: io.NopCloser(bytes.NewBufferString(lastfmError6)), StatusCode: 200}
_, err := agent.GetAlbumInfo(ctx, "123", "U2", "mbid-1234")
Expect(err).To(HaveOccurred())
// error 6 is a definitive not-found, so it must satisfy the shared sentinel (else it
// would trip the artwork worker's circuit breaker and be retried as a transient fault).
// A definitive not-found must satisfy the sentinel, or the artwork worker retries it.
Expect(errors.Is(err, agents.ErrNotFound)).To(BeTrue())
// No MBID retry: album.getInfo is queried by name+artist only, in a single call.
Expect(httpClient.RequestCount).To(Equal(1))
Expect(httpClient.SavedRequest.URL.Query().Get("mbid")).To(BeEmpty())
})
@ -615,8 +613,7 @@ var _ = Describe("lastfmAgent", func() {
It("maps a Last.fm error 6 (artist not found) to the shared not-found sentinel", func() {
apiClient.Res = http.Response{Body: io.NopCloser(bytes.NewBufferString(lastfmError6)), StatusCode: 200}
_, err := agent.GetArtistImages(ctx, "123", "Nonexistent Artist", "")
// Definitive not-found, not a fault — must satisfy the sentinel through the %w wrap so
// runs of missing artists never trip the artwork worker's circuit breaker.
// Not a fault: runs of missing artists must not trip the worker's circuit breaker.
Expect(errors.Is(err, agents.ErrNotFound)).To(BeTrue())
})

View File

@ -358,7 +358,7 @@ func startArtworkWorker(ctx context.Context, worker *artwork.Worker) func() erro
}
// scheduleArtworkHousekeeping runs the startup fingerprint backfill and registers the
// recurring stale-absent recheck and prune jobs. Scan-triggered prune lands in a later phase.
// recurring stale-absent recheck and prune jobs.
func scheduleArtworkHousekeeping(ctx context.Context, worker *artwork.Worker) func() error {
return func() error {
schedulerInstance := scheduler.GetInstance()

View File

@ -13,9 +13,7 @@ import (
"github.com/navidrome/navidrome/utils/str"
)
// externalName applies the DevPreserveUnicodeInExternalCalls normalization the aggregate
// provider used, so agent searches match the same way (typographic quotes/dashes cleared
// unless preserved).
// externalName mirrors the normalization the aggregate provider applies, so agent searches match.
func externalName(name string) string {
if conf.Server.DevPreserveUnicodeInExternalCalls {
return name
@ -23,9 +21,8 @@ func externalName(name string) string {
return str.Clear(name)
}
// bestImageURL returns the largest-Size image URL, skipping empty or unparseable
// URLs; nil when none qualifies. Parsing happens per candidate so a malformed largest
// URL never shadows a valid smaller one.
// bestImageURL returns the largest parseable image URL, so a malformed candidate never
// shadows a smaller valid one.
func bestImageURL(imgs []agents.ExternalImage) *url.URL {
var best *url.URL
var bestSize int
@ -44,12 +41,10 @@ func bestImageURL(imgs []agents.ExternalImage) *url.URL {
return best
}
// fetchArtistImage tries each enabled artist-image agent in order, each under its own gate.
// Returns the winning reader + agent name; extErr is true only when NO agent succeeded and
// at least one failed transiently (a later success beats an earlier agent error).
// fetchArtistImage tries each enabled artist-image agent in order. extErr is true only when no
// agent succeeded and at least one failed transiently.
func fetchArtistImage(ctx context.Context, ag *agents.Agents, gate gateFunc, ar model.Artist) (r io.ReadCloser, agentName string, extErr bool) {
// Synthetic artists have no real external image; mirror Agents.GetArtistImages' guard so a
// direct retriever call can't assign an unrelated result to Unknown/Various Artists.
// Synthetic artists would otherwise get an unrelated agent result assigned to them.
switch ar.ID {
case consts.UnknownArtistID, consts.VariousArtistsID:
return nil, "", false
@ -71,7 +66,7 @@ func fetchArtistImage(ctx context.Context, ag *agents.Agents, gate gateFunc, ar
return reader, a.Name, false
}
if isTransientExternal(err) {
extErr = true // includes errBreakerOpen and download failures: retry via the next agent
extErr = true
log.Debug(ctx, "Artwork: External artist-image lookup failed", "agent", a.Name, "artist", ar.Name, err)
}
}

View File

@ -54,9 +54,8 @@ func (f *fakeImageAgent) GetAlbumImages(_ context.Context, name, _, _ string) ([
return f.imgs, f.err
}
// imageAgents registers the fakes as built-in agents (ignoring the DataStore) and
// enables them in order, returning the process-wide Agents. Because the fakes ignore
// ds, reusing the GetAgents singleton across tests is safe.
// imageAgents registers the fakes as built-in agents and enables them in order. The fakes
// ignore the DataStore, so reusing the process-wide GetAgents singleton across tests is safe.
func imageAgents(fakes ...*fakeImageAgent) *agents.Agents {
names := make([]string, 0, len(fakes))
for _, f := range fakes {

View File

@ -20,32 +20,30 @@ import (
var ErrUnavailable = errors.New("artwork unavailable")
// errStaleSource signals that a backing file's mtime no longer matches the state
// row's RefMtime: the stored hash may be stale, so the load is aborted (dangling).
// errStaleSource means the backing file's mtime no longer matches RefMtime, so the stored hash may be stale.
var errStaleSource = errors.New("artwork: source file changed since resolution")
// Image is one servable artwork response.
type Image struct {
io.ReadCloser
Hash string // pixel-identity hash (immutable URL match); "" for placeholders
ETag string // served-representation validator; "" falls back to Hash (full-size original)
LastUpdated time.Time // zero for placeholders
Hash string // pixel identity; "" for placeholders
ETag string // representation validator; "" means Hash applies (full-size original)
LastUpdated time.Time
Placeholder bool
}
// representationTag identifies a served resized representation for HTTP validation: it changes with
// the dimensions and the encode settings (CoverArtQuality/EnableWebPEncoding), so a config change
// invalidates a revalidating client's cache even though the pixel hash is unchanged.
// representationTag varies with dimensions and encode settings, so a config change invalidates
// a revalidating client's cache even though the pixel hash is unchanged.
func representationTag(hash string, size int, square bool) string {
return fmt.Sprintf("%s.%d.%v.%s", hash, size, square, formatQualityTag())
}
type Artwork interface {
// Get serves resolved/provisional artwork; ErrUnavailable or model.ErrNotFound when
// there is nothing to serve (absent, pending, dangling) — caller picks placeholder vs 404.
// Get returns ErrUnavailable when there is nothing to serve and model.ErrNotFound when
// the id resolves to nothing, so the caller can pick placeholder vs 404.
Get(ctx context.Context, artID model.ArtworkID, size int, square bool) (*Image, error)
// GetOrPlaceholder parses a raw id token (raw entity ids accepted, as today) and falls
// back to the kind's placeholder image (never resized, Placeholder=true).
// GetOrPlaceholder accepts an artwork token or a raw entity id, falling back to the
// kind's placeholder image (never resized, Placeholder=true).
GetOrPlaceholder(ctx context.Context, id string, size int, square bool) (*Image, error)
}
@ -55,7 +53,6 @@ func NewArtwork(ds model.DataStore, cache cache.FileCache, store *ImageStore, ff
// EntityExists reports whether the entity an artwork id points at is still there: state rows
// outlive a deleted entity until the next prune, so a servable row is not evidence of its owner.
// The repositories are ctx-scoped, so a request context makes this a visibility check too.
func EntityExists(ctx context.Context, ds model.DataStore, artID model.ArtworkID) bool {
var found bool
var err error
@ -95,8 +92,8 @@ func (s *service) GetOrPlaceholder(ctx context.Context, id string, size int, squ
if err == nil {
img, err = s.Get(ctx, artID, size, square)
}
// Only a resolvable entity with no art gets the placeholder. An id that matches no entity
// stays ErrNotFound, so getCoverArt keeps answering error 70 and Jellyfin keeps 404ing.
// Only a resolvable entity with no art gets a placeholder; an unknown id must stay
// ErrNotFound so callers can still answer 404 / Subsonic error 70.
if errors.Is(err, ErrUnavailable) {
return placeholderImage(artID.Kind), nil
}
@ -108,7 +105,7 @@ func (s *service) Get(ctx context.Context, artID model.ArtworkID, size int, squa
return nil, ErrUnavailable
}
if size < 0 {
size = 0 // a negative size is a full-size request, not a giant (OOM) resize rectangle
size = 0 // a negative size means full-size, not a giant (OOM) resize rectangle
}
switch artID.Kind {
case model.KindDiscArtwork:
@ -120,13 +117,10 @@ func (s *service) Get(ctx context.Context, artID model.ArtworkID, size int, squa
}
}
// requestRecheckAge throttles view-triggered rechecks of an absent entity so repeatedly opening a
// genuinely-absent page can't hammer external services; below staleAbsentAge to catch younger absences.
// requestRecheckAge throttles view-triggered rechecks so reopening a genuinely-absent page can't
// hammer external services; below staleAbsentAge to catch younger absences.
const requestRecheckAge = time.Hour
// serveEntity serves an entity whose state the worker owns (album/artist/playlist/radio):
// found row serves its hash, absent row is unavailable (promoting a stale recheck on view),
// missing row reads through provisionally.
func (s *service) serveEntity(ctx context.Context, artID model.ArtworkID, size int, square bool) (*Image, error) {
ia, err := s.ds.Artwork(ctx).GetItemArtwork(artID.Kind, artID.ID, model.ImageTypePrimary)
switch {
@ -135,8 +129,8 @@ func (s *service) serveEntity(ctx context.Context, artID model.ArtworkID, size i
case err != nil:
return nil, err
case ia.Hash == "":
// EnqueueBump preserves an existing backoff row's retry_at; for a settled absent row
// (no queue row) it inserts a fresh, immediately-eligible recheck.
// EnqueueBump preserves an existing backoff row's retry_at, and inserts an
// immediately-eligible recheck for a settled absent row.
if time.Since(ia.AttemptedAt) > requestRecheckAge {
s.enqueue(ctx, artID, model.ArtworkPriorityBump)
}
@ -146,10 +140,8 @@ func (s *service) serveEntity(ctx context.Context, artID model.ArtworkID, size i
}
}
// serveSource is the one place bytes become an Image: a full-size request streams open()
// directly, anything else goes through the resize cache under key. hash is the pixel identity
// where one exists ("" for disc art, which has no state row) and doubles as the full-size
// validator, so an ETag is only needed when the bytes are resized or the hash is missing.
// serveSource is the one place bytes become an Image. hash is the pixel identity ("" for disc art)
// and doubles as the full-size validator, so an ETag is only needed when resized or hash is "".
func (s *service) serveSource(ctx context.Context, key, hash string, lastUpdate time.Time,
size int, square bool, open func() (io.ReadCloser, error),
) (*Image, error) {
@ -176,11 +168,10 @@ func (s *service) serveSource(ctx context.Context, key, hash string, lastUpdate
return &Image{ReadCloser: stream, Hash: hash, ETag: representationTag(key, size, square), LastUpdated: lastUpdate}, nil
}
// serveHash serves the bytes of a found state row. A mismatch/open error is dangling (a warm
// cache still serves), but a cancelled request is not: it must not enqueue a re-resolution.
// serveHash serves the bytes of a found state row. A mismatch/open error is dangling, but a
// cancelled request is not: it must not enqueue a re-resolution.
func (s *service) serveHash(ctx context.Context, artID model.ArtworkID, ia *model.ItemArtwork, size int, square bool) (*Image, error) {
// Only this path can hand back a deleted entity's bytes: an absent row is already
// unavailable, and the provisional and disc paths load their entity to resolve at all.
// Only this path can hand back a deleted entity's bytes; the others load their entity anyway.
if !EntityExists(ctx, s.ds, artID) {
return nil, ErrUnavailable
}
@ -203,8 +194,7 @@ func (s *service) serveHash(ctx context.Context, artID model.ArtworkID, ia *mode
return img, nil
}
// openOriginal opens the full-resolution bytes for a found state row, enforcing the
// mtime invariant: bytes are never served under a hash they no longer match.
// openOriginal enforces the mtime invariant: bytes are never served under a hash they no longer match.
func openOriginal(ia *model.ItemArtwork, mime string, store *ImageStore) (io.ReadCloser, error) {
if isFileBacked(ia.Source) {
f, err := os.Open(ia.SourcePath)
@ -224,8 +214,7 @@ func openOriginal(ia *model.ItemArtwork, mime string, store *ImageStore) (io.Rea
}
return f, nil
}
// Store-backed (embedded/external/generated): the bytes live in the content-addressed
// store, but an embedded source still carries the audio file's mtime to detect edits.
// Store-backed bytes still carry the source's mtime, to detect edits to embedded art.
if ia.SourcePath != "" && ia.RefMtime != 0 {
info, err := os.Stat(ia.SourcePath)
if err != nil {
@ -240,8 +229,8 @@ func openOriginal(ia *model.ItemArtwork, mime string, store *ImageStore) (io.Rea
return store.Open(ia.Hash, mime)
}
// provisional does a local-only read-through for an entity with no state row: it enqueues
// the worker (Bump) and serves any local bytes immediately, never writing a state row.
// provisional serves local bytes for an entity with no state row, enqueuing the worker but
// never writing a state row itself.
func (s *service) provisional(ctx context.Context, artID model.ArtworkID, size int, square bool) (*Image, error) {
item := model.ArtworkQueueItem{ItemKind: artID.Kind.Prefix(), ItemID: artID.ID, ImageType: model.ImageTypePrimary}
res, err := newLocalResolver(s.ds, s.ffmpeg).resolve(ctx, item)
@ -254,8 +243,7 @@ func (s *service) provisional(ctx context.Context, artID model.ArtworkID, size i
return s.serveResolution(ctx, res, size, square)
}
// serveResolution turns a local resolution's bytes into a servable Image (byte-hash
// only, no decode). A resolution with no reader is unavailable.
// serveResolution turns a local resolution's bytes into a servable Image (byte-hash only, no decode).
func (s *service) serveResolution(ctx context.Context, res resolution, size int, square bool) (*Image, error) {
if res.reader == nil {
return nil, ErrUnavailable
@ -274,12 +262,9 @@ func (s *service) serveResolution(ctx context.Context, res resolution, size int,
func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(data)), nil })
}
// serveMediaFile serves a track: own found art wins; an absent row delegates to the album;
// a missing row extracts embedded art (if eligible, enqueuing) else delegates without enqueue.
func (s *service) serveMediaFile(ctx context.Context, artID model.ArtworkID, size int, square bool) (*Image, error) {
// Per-track art can be disabled after mf rows were resolved (the setting is not in the
// config fingerprint). Honor it at serve time so a direct mf- URL falls back to disc/album
// instead of serving stale persisted embedded art.
// The setting is not in the config fingerprint, so honor it at serve time: a direct mf- URL
// must fall back to disc/album instead of serving stale persisted embedded art.
if !conf.Server.EnableMediaFileCoverArt {
mf, err := s.ds.MediaFile(ctx).Get(artID.ID)
if err != nil {
@ -292,9 +277,9 @@ func (s *service) serveMediaFile(ctx context.Context, artID model.ArtworkID, siz
case err == nil && ia.Hash != "":
return s.serveHash(ctx, artID, ia, size, square)
case err == nil:
// absent row → fall through to album delegation
// absent row: fall through
case errors.Is(err, model.ErrNotFound):
// no row → fall through to embedded eligibility / album delegation
// no row: fall through
default:
return nil, err
}
@ -307,13 +292,11 @@ func (s *service) serveMediaFile(ctx context.Context, artID model.ArtworkID, siz
if noRow && conf.Server.EnableMediaFileCoverArt && mf.HasCoverArt {
return s.provisionalEmbedded(ctx, artID, *mf, size, square)
}
// Mirror MediaFile.CoverArtID's fallback: a multi-disc track defers to its disc art
// (which itself falls back to the album), not straight to the album.
// Mirror MediaFile.CoverArtID: a track defers to its disc art, which falls back to the album.
return s.Get(ctx, mf.DiscCoverArtID(), size, square)
}
// provisionalEmbedded extracts a track's embedded art for an immediate serve and always
// enqueues the track (Bump) so the worker persists state; it never writes a state row.
// provisionalEmbedded serves a track's embedded art immediately, leaving the state row to the worker.
func (s *service) provisionalEmbedded(ctx context.Context, artID model.ArtworkID, mf model.MediaFile, size int, square bool) (*Image, error) {
lib, err := loadLibraryView(ctx, s.ds, mf.LibraryID)
if err != nil {
@ -322,31 +305,26 @@ func (s *service) provisionalEmbedded(ctx context.Context, artID model.ArtworkID
res, ok := resolveEmbedded(ctx, lib, s.ffmpeg, mf.Path)
s.enqueue(ctx, artID, model.ArtworkPriorityBump)
if !ok {
// Eligible but unextractable (truncated frame, no ffmpeg): fall back the way
// CoverArtID does rather than answer with a placeholder.
// Eligible but unextractable: fall back the way CoverArtID does, not to a placeholder.
return s.Get(ctx, mf.DiscCoverArtID(), size, square)
}
return s.serveResolution(ctx, res, size, square)
}
// serveDisc serves disc-level artwork as a pure provisional read-through: no state rows,
// no enqueue. It tries the disc-folder selection chain and falls back to the album cover.
// serveDisc reads disc art through with no state row and no enqueue, falling back to the album cover.
func (s *service) serveDisc(ctx context.Context, artID model.ArtworkID, size int, square bool) (*Image, error) {
dr, err := newDiscArtworkReader(ctx, s.ds, artID)
if err != nil {
return nil, err
}
// Single-disc albums run the chain too: a disc can carry its own art, distinct from the
// album cover, and DiscArtPriority is what expresses that preference.
// Single-disc albums run the chain too: a disc can carry art distinct from the album cover.
selectImage := func() (io.ReadCloser, string, error) {
funcs := dr.fromDiscArtPriority(ctx, s.ffmpeg, conf.Server.DiscArtPriority)
return selectImageReader(ctx, artID, funcs...)
}
albumArtID := model.ArtworkID{Kind: model.KindAlbumArtwork, ID: dr.album.ID}
// Disc art has no state row, so there is no stored content hash — to key the resize cache
// on, or to fall back to as a validator. Keying on the id and the album's mtime, as the
// legacy reader did, lets a warm cache answer without touching the filesystem; the chain
// runs only on a miss. The key carries DiscArtPriority so changing it invalidates.
// Disc art has no state row, hence no content hash: keying on id, album mtime and
// DiscArtPriority lets a warm cache answer without running the chain or touching the disk.
key := fmt.Sprintf("%s|%d|%s", artID.ID, dr.cacheTime().UnixNano(), conf.Server.DiscArtPriority)
img, err := s.serveSource(ctx, key, "", dr.cacheTime(), size, square,
func() (io.ReadCloser, error) { rc, _, err := selectImage(); return rc, err })
@ -359,16 +337,14 @@ func (s *service) serveDisc(ctx context.Context, artID model.ArtworkID, size int
return img, nil
}
// dangling enqueues a re-resolution at Scan priority and reports the artwork as
// unavailable, leaving the state row untouched.
// dangling enqueues a re-resolution and reports unavailable, leaving the state row untouched.
func (s *service) dangling(ctx context.Context, artID model.ArtworkID) (*Image, error) {
log.Debug(ctx, "Artwork: State row points at bytes we cannot serve, re-resolving", "artID", artID)
s.enqueue(ctx, artID, model.ArtworkPriorityScan)
return nil, ErrUnavailable
}
// enqueue schedules a request-triggered re-resolution. It uses EnqueueBump so an incidental
// read-through never resets a failed resolution's backoff (unlike scan/manual re-resolve).
// enqueue uses EnqueueBump so an incidental read-through never resets a failed resolution's backoff.
func (s *service) enqueue(ctx context.Context, artID model.ArtworkID, priority int) {
err := s.ds.ArtworkQueue(ctx).EnqueueBump(model.ArtworkQueueItem{
ItemKind: artID.Kind.Prefix(),
@ -390,8 +366,8 @@ func placeholderImage(kind model.Kind) *Image {
return &Image{ReadCloser: r, Placeholder: true}
}
// PlaceholderFor returns the kind-appropriate placeholder for an artwork id, for callers that must
// serve a placeholder without consulting persisted state (e.g. an access-control denial).
// PlaceholderFor returns the kind-appropriate placeholder for an artwork id, for callers that
// must not consult persisted state (e.g. an access-control denial).
func PlaceholderFor(id string) *Image {
artID, _ := model.ParseArtworkID(id)
return placeholderImage(artID.Kind)
@ -401,8 +377,7 @@ type coverArtIDGetter interface {
CoverArtID() model.ArtworkID
}
// parseArtworkID ports the legacy getArtworkId: parse the token, and if it is a raw
// entity id, resolve the entity and take its CoverArtID.
// parseArtworkID accepts an artwork token or a raw entity id, resolving the latter to its CoverArtID.
func (s *service) parseArtworkID(ctx context.Context, id string) (model.ArtworkID, error) {
if id == "" {
return model.ArtworkID{}, ErrUnavailable

View File

@ -38,7 +38,6 @@ func TestArtwork(t *testing.T) {
}
// osDirFS wraps os.DirFS as a storage.MusicFS for integration tests.
// ReadTags is not exercised by these tests, so it is left as a stub.
type osDirFS struct{ fs.FS }
func (o osDirFS) ReadTags(...string) (map[string]metadata.Info, error) { return nil, nil }

View File

@ -41,8 +41,6 @@ var _ = Describe("Artwork", func() {
primaryKey := func(kind, id string) string { return kind + "|" + id + "|" + model.ImageTypePrimary }
// seedFoundStore installs a store-backed found state (bytes in the content-addressed
// store, no backing file) and returns the hash.
seedFoundStore := func(kind, id string, imgBytes []byte) string {
hash, err := hashImage(bytes.NewReader(imgBytes))
Expect(err).ToNot(HaveOccurred())
@ -53,8 +51,7 @@ var _ = Describe("Artwork", func() {
return hash
}
// seedEntity registers the owning entity, without which the state row describes something
// that no longer exists and the service correctly refuses to serve it.
// Without its owning entity, a state row is not served at all.
seedEntity = func(kind, id string) {
GinkgoHelper()
switch kind {
@ -119,8 +116,7 @@ var _ = Describe("Artwork", func() {
img, err := svc.Get(ctx, model.MustParseArtworkID("al-al1"), 100, false)
Expect(err).ToNot(HaveOccurred())
// A resized response versions its validator with the encode settings, distinct from
// the pixel hash, so a CoverArtQuality/WebP change invalidates client caches.
// A resized response versions its ETag with the encode settings, not the pixel hash.
Expect(img.ETag).To(Equal(representationTag(img.Hash, 100, false)))
Expect(img.ETag).ToNot(Equal(img.Hash))
resized := readAll(img)
@ -128,8 +124,7 @@ var _ = Describe("Artwork", func() {
Expect(err).ToNot(HaveOccurred())
Expect(cfg.Width).To(Equal(100))
// Delete the store file: a warm resize-cache entry must keep serving without
// ever touching the original (the stale-serve self-heal).
// Deleting the store file proves the warm entry serves without touching the original.
hash, _ := hashImage(bytes.NewReader(coverBytes))
Expect(store.Remove(hash, "image/jpeg", time.Now().Add(time.Hour))).To(Succeed())
Eventually(func(g Gomega) {
@ -199,8 +194,7 @@ var _ = Describe("Artwork", func() {
Expect(queueRepo.Data[primaryKey("al", "al3b")].Priority).To(Equal(model.ArtworkPriorityScan))
})
// State rows and their bytes outlive a deleted entity until the next prune, so serving
// straight from the row would keep handing out a removed entity's image.
// State rows outlive a deleted entity until the next prune.
It("refuses to serve a found row whose entity is gone", func() {
hash := seedFoundStore("al", "alzz", coverBytes)
Expect(hash).ToNot(BeEmpty())
@ -228,7 +222,6 @@ var _ = Describe("Artwork", func() {
_, err := svc.Get(ctx, model.MustParseArtworkID("al-al4b"), 0, false)
Expect(err).To(MatchError(ErrUnavailable))
Expect(queueRepo.Data[primaryKey("al", "al4b")].Priority).To(Equal(model.ArtworkPriorityBump))
// The absent state row is left intact; only a recheck is scheduled.
ia, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "al4b", model.ImageTypePrimary)
Expect(err).ToNot(HaveOccurred())
Expect(ia.Hash).To(BeEmpty())
@ -272,7 +265,6 @@ var _ = Describe("Artwork", func() {
It("ignores a resolved mf row and delegates to the album when per-track art is disabled", func() {
conf.Server.EnableMediaFileCoverArt = false
// A resolved mf row exists (from when the setting was on) but must not be served.
seedFoundStore("mf", "mf7", []byte("stale embedded track art"))
seedFoundStore("al", "albz", coverBytes)
mfRepo.SetData(model.MediaFiles{{ID: "mf7", AlbumID: "albz"}})
@ -322,7 +314,6 @@ var _ = Describe("Artwork", func() {
})
It("delegates a multi-disc track to its disc art, not straight to the album", func() {
// Not embedded-eligible: the fallback must mirror CoverArtID (disc first, then album).
folderRepo.result = []model.Folder{{Path: "tests/fixtures/artist/an-album", ImageFiles: []string{"cover.jpg"}}}
albumRepo.SetData(model.Albums{{ID: "aldd", Name: "Album", FolderIDs: []string{"f1"}, Discs: model.Discs{1: "One", 2: "Two"}}})
seedFoundStore("al", "aldd", []byte("album-art-distinct")) // album's own found art differs
@ -330,15 +321,13 @@ var _ = Describe("Artwork", func() {
img, err := svc.Get(ctx, model.MustParseArtworkID("mf-mf5"), 0, false)
Expect(err).ToNot(HaveOccurred())
// The disc-folder image wins over the album's found art, proving it routed via serveDisc.
// The disc-folder image, not the album's own art: proof it routed through serveDisc.
Expect(readAll(img)).To(Equal(coverBytes))
})
// Jellyfin clients are now told to request the track image before it resolves, so an
// unextractable frame must not answer with a placeholder where the album has art.
It("falls back to the album when an eligible track's embedded art will not extract", func() {
conf.Server.EnableMediaFileCoverArt = true
// HasCoverArt is set, but the file yields no extractable image (it is not audio).
// HasCoverArt is set, but the file is not audio, so nothing extracts.
mfRepo.SetData(model.MediaFiles{{
ID: "mfbad", AlbumID: "albad", LibraryID: 0, HasCoverArt: true,
Path: "tests/fixtures/artist/an-album/front.png",
@ -352,8 +341,7 @@ var _ = Describe("Artwork", func() {
})
It("routes a single-disc track through disc resolution too", func() {
// A single disc can carry its own art: DiscArtPriority still applies, so the disc
// image is served even when the album has different found art.
// DiscArtPriority applies to single-disc albums too, over the album's own found art.
folderRepo.result = []model.Folder{{Path: "tests/fixtures/artist/an-album", ImageFiles: []string{"cover.jpg"}}}
albumRepo.SetData(model.Albums{{ID: "alsd", Name: "Album", FolderIDs: []string{"f1"}, Discs: model.Discs{1: ""}}})
seedFoundStore("al", "alsd", []byte("album-art-distinct"))
@ -375,9 +363,8 @@ var _ = Describe("Artwork", func() {
Expect(readAll(img)).To(Equal(coverBytes))
})
// The resize cache is keyed on the id and the album's mtime, not on the image bytes, so a
// warm hit needs no filesystem access. Dropping the source between the two requests is
// how that shows: re-reading would fall back to the album instead.
// The resize cache keys on id + album mtime, not on the bytes, so dropping the source
// between the two requests is what shows a warm hit never touches the filesystem.
It("serves a sized disc image from cache without re-reading the source", func() {
folderRepo.result = []model.Folder{{Path: "tests/fixtures/artist/an-album", ImageFiles: []string{"cover.jpg"}}}
albumRepo.SetData(model.Albums{{ID: "aldc3", Name: "Album", FolderIDs: []string{"f1"}}})
@ -388,8 +375,6 @@ var _ = Describe("Artwork", func() {
warmed := readAll(first)
Expect(warmed).ToNot(BeEmpty())
// With the source gone and the album holding no art of its own, a re-read would
// fall through to the album and fail.
folderRepo.result = nil
second, err := svc.Get(ctx, discID, 64, false)
@ -397,8 +382,7 @@ var _ = Describe("Artwork", func() {
Expect(readAll(second)).To(Equal(warmed), "a warm sized request must not touch the source")
})
// A disc image can be replaced without the album row changing, so the key folds in the
// folder's ImagesUpdatedAt; keying on album.UpdatedAt alone would serve the old image.
// A disc image can change without the album row changing, so the key folds in ImagesUpdatedAt.
It("invalidates the cached image when the folder's images change", func() {
folderRepo.result = []model.Folder{{
Path: "tests/fixtures/artist/an-album", ImageFiles: []string{"cover.jpg"},
@ -412,7 +396,6 @@ var _ = Describe("Artwork", func() {
firstKey := first.ETag
readAll(first)
// The image was replaced: same album row, newer folder images timestamp.
folderRepo.result[0].ImagesUpdatedAt = time.Now()
second, err := svc.Get(ctx, discID, 64, false)
Expect(err).ToNot(HaveOccurred())
@ -420,8 +403,7 @@ var _ = Describe("Artwork", func() {
Expect(second.ETag).ToNot(Equal(firstKey), "a replaced image must not keep the old cache entry")
})
// Disc art has no content hash to fall back to, so without an explicit validator the
// full-size response would carry an empty ETag — identical for every disc image.
// Disc art has no content hash, so without an explicit validator every ETag would be empty.
It("gives a full-size disc image a validator that tracks the source", func() {
folderRepo.result = []model.Folder{{
Path: "tests/fixtures/artist/an-album", ImageFiles: []string{"cover.jpg"},
@ -490,8 +472,7 @@ var _ = Describe("Artwork", func() {
Expect(readAll(img)).To(Equal(phBytes))
})
// An entity that has no art and an id that names no entity are different answers:
// Subsonic reports error 70 for the latter, and Jellyfin 404s.
// "No art" and "no such entity" are different answers: clients 404 only on the latter.
It("reports not-found rather than a placeholder for an id with no entity", func() {
_, err := svc.GetOrPlaceholder(ctx, "al-nosuchalbum", 0, false)
Expect(err).To(MatchError(model.ErrNotFound))

View File

@ -1,5 +1,5 @@
// Package blurhash implements the blurhash encoding algorithm (https://github.com/woltapp/blurhash),
// matching Jellyfin's parameters so clients tuned against Jellyfin see equivalent hashes.
// Package blurhash implements the blurhash encoding (https://github.com/woltapp/blurhash),
// parameterized to match Jellyfin so clients see equivalent hashes.
package blurhash
import (
@ -15,10 +15,10 @@ import (
const alphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz#$%*+,-.:;=?@[]^_{|}~"
// maxInputSize matches Jellyfin: larger inputs are slower with no visually discernible difference.
// maxInputSize: larger inputs are slower with no visible difference in the result.
const maxInputSize = 128
// Components picks x/y component counts for an image, targeting ~16 near-square tiles (Jellyfin's formula).
// Components picks x/y component counts targeting ~16 near-square tiles.
func Components(width, height int) (int, int) {
if width <= 0 || height <= 0 {
return 0, 0
@ -109,8 +109,7 @@ func Encode(img image.Image, xComp, yComp int) (string, error) {
return sb.String(), nil
}
// toRGBA gives the pixel loop direct Pix access, avoiding a per-pixel allocation through the
// image.At interface (~16k allocs per encode).
// toRGBA gives the pixel loop direct Pix access, avoiding a per-pixel allocation via image.At.
func toRGBA(img image.Image) *image.RGBA {
if rgba, ok := img.(*image.RGBA); ok {
return rgba
@ -165,8 +164,7 @@ func linearToSRGB(v float64) int {
return int((1.055*math.Pow(v, 1/2.4)-0.055)*255 + 0.5)
}
// Encode83 encodes value as a fixed-width, big-endian base83 string of the given length, using the
// blurhash spec's alphabet.
// Encode83 encodes value as a fixed-width, big-endian base83 string of the given length.
func Encode83(value, length int) string {
b := make([]byte, length)
for i := length - 1; i >= 0; i-- {

View File

@ -103,7 +103,6 @@ var _ = Describe("Encode", func() {
})
It("downscales large images internally without changing the result materially", func() {
// A 1000px solid image must encode fine and carry the same DC as its small version.
big, err := blurhash.Encode(solidImage(1000, 1000, color.NRGBA{R: 60, G: 120, B: 180, A: 255}), 5, 5)
Expect(err).ToNot(HaveOccurred())
small, err := blurhash.Encode(solidImage(16, 16, color.NRGBA{R: 60, G: 120, B: 180, A: 255}), 5, 5)

View File

@ -27,14 +27,13 @@ type discArtworkReader struct {
isMultiFolder bool
firstTrackRel string // library-relative; for fromTag / ffmpeg via lib.Abs
lib libraryView
// imagesUpdatedAt is the newest ImagesUpdatedAt across the album's and this disc's folders.
// An image can be replaced without the album row changing, so this is what makes a cache
// key notice it.
// Newest ImagesUpdatedAt across the album's and this disc's folders: an image can be
// replaced without the album row changing, so this is what makes a cache key notice it.
imagesUpdatedAt time.Time
}
// cacheTime is the disc image's validity stamp: any of these moving means the selection may
// have changed. Mirrors what the legacy reader folded into its cache key.
// have changed.
func (d *discArtworkReader) cacheTime() time.Time {
return utils.TimeNewest(d.album.UpdatedAt, d.album.ImportedAt, d.imagesUpdatedAt)
}
@ -161,19 +160,12 @@ func (d *discArtworkReader) fromDiscSubtitle(ctx context.Context, subtitle strin
}
}
// globMetaChars holds the substitution metacharacters understood by
// filepath.Match. The '\' escape character is intentionally excluded:
// disc art patterns come from user config and never include escaped
// metachars in practice, and treating '\' as a metachar would misalign
// the literal-prefix extraction in extractDiscNumber.
// filepath.Match's '\' escape is excluded on purpose: treating it as a metachar
// would misalign the literal-prefix extraction in extractDiscNumber.
const globMetaChars = "*?["
// extractDiscNumber parses the disc number from a filename matched by a
// filepath.Match-style glob pattern.
//
// Both pattern and filename must already be lowercased by the caller, which
// is also expected to have verified that filepath.Match(pattern, filename)
// is true before calling this function.
// extractDiscNumber parses the disc number from a filename matched by a filepath.Match-style
// glob. Caller must lowercase both args and have already verified the match.
func extractDiscNumber(pattern, filename string) (int, bool) {
metaIdx := strings.IndexAny(pattern, globMetaChars)
if metaIdx < 0 {
@ -199,9 +191,8 @@ func extractDiscNumber(pattern, filename string) (int, bool) {
return num, true
}
// fromExternalFile returns a sourceFunc that matches image files against a glob
// pattern. A numbered filename whose number equals the target disc wins over
// any unnumbered candidate; callers must pass a lowercase pattern.
// fromExternalFile matches image files against a (lowercase) glob pattern. A numbered
// filename whose number equals the target disc wins over any unnumbered candidate.
func (d *discArtworkReader) fromExternalFile(ctx context.Context, pattern string) sourceFunc {
isLiteral := !strings.ContainsAny(pattern, globMetaChars)
return func() (io.ReadCloser, string, error) {

View File

@ -20,9 +20,7 @@ import (
. "github.com/onsi/gomega"
)
// These specs wire the real Worker and Service over the same ImageStore and mock repositories,
// then drive the full enqueue → drain → serve loop. They assert the integration of the chain, not
// the per-source resolution rules (which the unit suites in package artwork already cover).
// Covers the enqueue → drain → serve chain; per-source resolution rules live in the unit suites.
var _ = Describe("Acquisition → serve loop", func() {
var (
ctx context.Context
@ -42,7 +40,6 @@ var _ = Describe("Acquisition → serve loop", func() {
coverBytes []byte
)
// itemFound reports whether the worker has persisted a resolved (hash-bearing) state row.
itemFound := func(kind model.Kind, id string) func() bool {
return func() bool {
ia, err := artRepo.GetItemArtwork(kind, id, model.ImageTypePrimary)
@ -66,7 +63,7 @@ var _ = Describe("Acquisition → serve loop", func() {
conf.Server.CacheFolder = conf.NewDir(GinkgoT().TempDir())
conf.Server.DataFolder = conf.NewDir(GinkgoT().TempDir())
conf.Server.CoverArtPriority = "cover.jpg"
conf.Server.ArtistArtPriority = "artist.png" // upload wins first; kept offline as a safety net
conf.Server.ArtistArtPriority = "artist.png" // keeps artist resolution offline
conf.Server.EnableMediaFileCoverArt = true
conf.Server.ArtworkWorkerConcurrency = 1
@ -94,8 +91,7 @@ var _ = Describe("Acquisition → serve loop", func() {
}
ffm := tests.NewMockFFmpeg("")
store = artwork.NewImageStore(GinkgoT().TempDir())
// size=0 requests stream originals and never touch the resize cache, so this reader is a
// compile-time stand-in only; the resize path is covered by the package's serving_test.
// size=0 requests stream originals, so this reader is never called (serving_test covers resizing).
imgCache := cache.NewFileCache("ArtworkPipelineE2E", "100MB", "images", 0,
func(context.Context, cache.Item) (io.Reader, error) {
return nil, errors.New("resize not exercised in e2e")
@ -106,8 +102,6 @@ var _ = Describe("Acquisition → serve loop", func() {
worker = artwork.NewWorker(ds, store, agents.GetAgents(ds, nil), ffm, events.NoopBroker(), imgCache)
})
// seedFolderAlbum wires an album backed by the real fixture folder cover, shared by the album
// and playlist-grid scenarios.
seedFolderAlbum := func(albumID string) {
folderRepo.result = []model.Folder{{Path: albumFolderPath, ImageFiles: []string{"cover.jpg"}}}
albumRepo.SetData(model.Albums{{ID: albumID, Name: "Album", FolderIDs: []string{"f1"}, LibraryID: 0}})
@ -159,7 +153,6 @@ var _ = Describe("Acquisition → serve loop", func() {
img, err := svc.Get(ctx, model.MustParseArtworkID("pl-pl1"), 0, false)
Expect(err).ToNot(HaveOccurred())
Expect(img.Hash).To(Equal(ia.Hash))
// The generated grid is a fresh PNG placed in the content-addressed store.
art, err := artRepo.GetImage(ia.Hash)
Expect(err).ToNot(HaveOccurred())
Expect(art.Mime).To(Equal("image/png"))
@ -187,7 +180,6 @@ var _ = Describe("Acquisition → serve loop", func() {
ID: "mf1", AlbumID: "al1", HasCoverArt: true, LibraryID: 0, Path: mp3Fixture,
}})
// First read: no state row yet → extract embedded art provisionally and enqueue the track.
provisional, err := svc.Get(ctx, model.MustParseArtworkID("mf-mf1"), 0, false)
Expect(err).ToNot(HaveOccurred())
Expect(provisional.Placeholder).To(BeFalse())
@ -198,14 +190,13 @@ var _ = Describe("Acquisition → serve loop", func() {
_, err = artRepo.GetItemArtwork(model.KindMediaFileArtwork, "mf1", model.ImageTypePrimary)
Expect(err).To(MatchError(model.ErrNotFound), "provisional serving must not write a state row")
// The provisional read enqueued a Bump; drain it and confirm the persisted hash matches.
// The provisional read enqueued a Bump; drain it.
runWorkerUntil(ctx, worker, itemFound(model.KindMediaFileArtwork, "mf1"))
ia, err := artRepo.GetItemArtwork(model.KindMediaFileArtwork, "mf1", model.ImageTypePrimary)
Expect(err).ToNot(HaveOccurred())
Expect(ia.Source).To(Equal("embedded"))
Expect(ia.Hash).To(Equal(provisional.Hash))
// Second read: now served from the persisted state row / store, same bytes.
resolved, err := svc.Get(ctx, model.MustParseArtworkID("mf-mf1"), 0, false)
Expect(err).ToNot(HaveOccurred())
Expect(resolved.Hash).To(Equal(ia.Hash))
@ -225,7 +216,6 @@ var _ = Describe("Acquisition → serve loop", func() {
Expect(art.Width).To(BeNumerically(">", 0))
Expect(art.Height).To(BeNumerically(">", 0))
Expect(art.SizeBytes).To(BeNumerically("==", len(coverBytes)))
// Never a synthesized value: the blurhash is encoded from the real pixels.
Expect(art.BlurHash).ToNot(BeEmpty())
})
@ -273,17 +263,16 @@ var _ = Describe("Acquisition → serve loop", func() {
Expect(err).ToNot(HaveOccurred())
staleHash := ia.Hash
// Replace the backing file with different bytes and a newer mtime.
path := model.UploadedImagePath(consts.EntityRadio, name)
Expect(os.WriteFile(path, readFixture(artistPngFixture), 0o600)).To(Succeed())
newer := time.Now().Add(2 * time.Second)
Expect(os.Chtimes(path, newer, newer)).To(Succeed())
// The mtime no longer matches the state row, so the old hash's bytes are never served.
// The mtime no longer matches the state row, so the stale bytes are not served.
_, err = svc.Get(ctx, model.MustParseArtworkID("ra-ra1"), 0, false)
Expect(err).To(MatchError(artwork.ErrUnavailable))
// That read enqueued a re-resolution; draining it republishes the new bytes.
// That failed read enqueued a re-resolution.
runWorkerUntil(ctx, worker, func() bool {
cur, gerr := artRepo.GetItemArtwork(model.KindRadioArtwork, "ra1", model.ImageTypePrimary)
return gerr == nil && cur.Hash != "" && cur.Hash != staleHash
@ -307,15 +296,14 @@ var _ = Describe("Acquisition → serve loop", func() {
})
})
// mustGet unwraps a Service.Get result for inline byte assertions.
func mustGet(img *artwork.Image, err error) *artwork.Image {
GinkgoHelper()
Expect(err).ToNot(HaveOccurred())
return img
}
// gifFixture is a 4x4 GIF held as raw bytes on purpose: encoding one would import image/gif into
// this test binary and register the decoder, masking the production import the spec above guards.
// Raw bytes on purpose: encoding a GIF here would register image/gif in the test binary, masking
// the production import the spec above guards.
var gifFixture = []byte{
0x47, 0x49, 0x46, 0x38, 0x39, 0x61, 0x04, 0x00, 0x04, 0x00, 0x80, 0x00,
0x00, 0x2e, 0x86, 0xc1, 0xf4, 0xd0, 0x3f, 0x2c, 0x00, 0x00, 0x00, 0x00,

View File

@ -9,10 +9,8 @@ import (
. "github.com/onsi/gomega"
)
// Folder-backed album art is served via os.Open(SourcePath), which the in-memory library FS
// cannot satisfy; the worker's persisted state row (Source + SourcePath) is the resolver's
// selection, so folder scenarios assert on it. Embedded art lands in the content-addressed store
// and is asserted byte-for-byte; a no-art album settles absent.
// The in-memory library FS cannot satisfy the os.Open(SourcePath) used to serve folder art, so
// folder scenarios assert on the worker's state row (Source + SourcePath) instead of the bytes.
var _ = Describe("Album artwork resolution", func() {
BeforeEach(func() {
setupResolutionHarness()
@ -354,8 +352,7 @@ var _ = Describe("Album artwork resolution", func() {
})
scan()
// Album B first: a drain settles every ready item, and folder art is only
// byte-servable while the album still has no state row.
// Album B first: the acquire in expectAbsent would settle Album B too.
expectFolderCover(albumByName("Album B"), "Artist/Album B/cover.jpg")
expectAbsent(albumByName("Album A"))
})
@ -389,11 +386,10 @@ var _ = Describe("Album artwork resolution", func() {
})
})
// albumRootParent refuses the library root as an album root (parent.ParentID == ""), so a
// stray image at the top of the library never becomes some album's cover.
// albumRootParent refuses the library root as an album root (parent.ParentID == "").
When("a multi-disc album sits directly at the library root with a cover.jpg beside it", func() {
// (library root)
// ├── cover.jpg ← must NOT be adopted: the root is never an album root
// ├── cover.jpg ← must NOT be adopted
// ├── CD1/
// │ └── 01 - Track.mp3
// └── CD2/
@ -410,15 +406,14 @@ var _ = Describe("Album artwork resolution", func() {
})
})
// compareImageFiles prefers shallower paths on a basename tie, so an artist-folder cover.jpg
// would outrank the album's own if the parent folder were ever considered here. It is not:
// albumRootParent skips the parent for a single-folder album that has images of its own.
// The shallower artist-folder cover.jpg would win the basename tie, but albumRootParent skips
// the parent folder for a single-folder album that has images of its own.
When("a single-folder album has its own cover.jpg and the artist folder has one too", func() {
// Artist/
// ├── cover.jpg ← shallower, but must NOT win
// └── Album/
// ├── 01 - Track.mp3
// └── cover.jpg ← should win (the album has images of its own)
// └── cover.jpg ← should win
It("prefers the album's own cover over the shallower artist-folder cover", func() {
conf.Server.CoverArtPriority = defaultCoverPriority
setLayout(fstest.MapFS{
@ -441,8 +436,7 @@ var _ = Describe("Album artwork resolution", func() {
// ├── Album A bonus/
// │ └── 02 - Track.mp3 (album: "Album A")
// └── Album B/
// └── 01 - Track.mp3 other-album audio, so the artist folder is
// correctly rejected as Album A's root
// └── 01 - Track.mp3 (other-album audio: rejects the artist folder as a root)
It("prefers the album's own art over the artist image", func() {
conf.Server.CoverArtPriority = defaultCoverPriority
setLayout(fstest.MapFS{

View File

@ -11,8 +11,7 @@ import (
)
// Disc art is a serve-time read through the library FS (no worker state row), so per-disc images
// are asserted byte-for-byte. Single-disc albums run the disc chain too — a disc can carry art
// distinct from the album cover. Album-root covers are folder-backed and asserted on the state row.
// are asserted byte-for-byte, while album-root covers are asserted on the state row.
var _ = Describe("Disc artwork resolution", func() {
BeforeEach(func() {
setupResolutionHarness()

View File

@ -1,6 +1,5 @@
// Package e2e exercises the artwork pipeline end to end: it enqueues real entities, drives the
// real Worker to drain the queue, and serves the result through the real Service, over a real
// ImageStore and real library files.
// Package e2e exercises the artwork pipeline end to end: the real Worker drains the queue and the
// real Service serves the result, over a real ImageStore and real library files.
package e2e
import (
@ -36,7 +35,6 @@ const (
albumFolderPath = "tests/fixtures/artist/an-album"
)
// readFixture returns the raw bytes of a project-relative fixture file.
func readFixture(rel string) []byte {
GinkgoHelper()
data, err := os.ReadFile(rel)
@ -44,7 +42,6 @@ func readFixture(rel string) []byte {
return data
}
// readAll drains an artwork image to bytes and closes it.
func readAll(img *artwork.Image) []byte {
GinkgoHelper()
Expect(img).ToNot(BeNil())
@ -54,8 +51,6 @@ func readAll(img *artwork.Image) []byte {
return data
}
// runWorkerUntil starts the real worker loop, waits for a condition, then cancels and joins it,
// mirroring how cmd drives Worker.Run in production.
func runWorkerUntil(ctx context.Context, worker *artwork.Worker, until func() bool) {
GinkgoHelper()
runCtx, cancel := context.WithCancel(ctx)
@ -66,8 +61,6 @@ func runWorkerUntil(ctx context.Context, worker *artwork.Worker, until func() bo
Eventually(done, 2*time.Second).Should(Receive(BeNil()))
}
// fakeFolderRepo is the minimal FolderRepository the album/playlist resolution chains touch:
// GetAll yields the seeded folders and the album-root parent lookup finds nothing.
type fakeFolderRepo struct {
model.FolderRepository
result []model.Folder
@ -81,8 +74,6 @@ func (f *fakeFolderRepo) HasAudioOutsideFolders(model.Folder, []string) (bool, e
func (f *fakeFolderRepo) Get(string) (*model.Folder, error) { return nil, model.ErrNotFound }
// writeUpload copies a fixture into the per-entity upload folder under the data dir and returns
// the bare filename UploadedImagePath expects.
func writeUpload(entityType, name, srcFixture string) string {
GinkgoHelper()
dst := model.UploadedImagePath(entityType, name)

View File

@ -126,8 +126,7 @@ var _ = Describe("MediaFile artwork resolution", func() {
scan()
replaceWithRealMP3("Artist/Album/01 - Track.mp3")
// The setting is not part of the artwork fingerprint, so a direct mf- request must
// honor it at serve time rather than serving previously-eligible embedded art.
// The setting is not part of the artwork fingerprint, so it must be honored at serve time.
conf.Server.EnableMediaFileCoverArt = false
mf := mediafileOn("Artist/Album/01 - Track.mp3")
trackArtID := model.NewArtworkID(model.KindMediaFileArtwork, mf.ID, nil)

View File

@ -169,7 +169,6 @@ var _ = Describe("Playlist artwork resolution", func() {
// ├── AlbumB/{01 - Track.mp3, cover.png} ← tile 2
// ├── AlbumC/{01 - Track.mp3, cover.png} ← tile 3
// └── AlbumD/{01 - Track.mp3, cover.png} ← tile 4
// Four distinct tiles fill the grid outright, with no mirroring.
It("fills all four grid quadrants with distinct album art", func() {
conf.Server.CoverArtPriority = "cover.*"
layout := fstest.MapFS{}

View File

@ -38,12 +38,6 @@ import (
"go.senan.xyz/taglib"
)
// This harness restores the pre-cutover artwork resolution edge-case coverage, but drives it
// through the real pipeline: a real scanner populates the folder graph from an in-memory library,
// the real Worker drains the queue to resolve/persist state, and the real Service serves it.
// It documents the folder-selection rules (album/disc/artist priority, #5376/#5456/#5451/#5457)
// that the lightweight acquire_serve_test.go intentionally leaves to this suite.
const fakeLibScheme = "artworkfake"
const fakeLibPath = fakeLibScheme + ":///music"
@ -61,9 +55,8 @@ var (
fakeFS *storagetest.FakeFS
)
// The DB file lives in a suite-level tempdir: the go-sqlite3 singleton keeps the file open for the
// whole suite, and Ginkgo's per-spec TempDir cleanup can't unlink a file with a live handle on
// Windows. A suite-level tempdir plus an AfterSuite close avoids the lock conflict.
// The go-sqlite3 singleton holds the file open for the whole suite, and Windows cannot unlink a
// file with a live handle, so the DB cannot live in Ginkgo's per-spec TempDir.
var suiteDBTempDir string
// Migrating the schema costs ~400ms, so it runs once per suite and specs reset by truncating.
@ -117,8 +110,7 @@ func setupResolutionHarness() {
ffm := tests.NewMockFFmpeg("")
rstore = artwork.NewImageStore(filepath.Join(tempDir, "store"))
// size=0 requests stream originals and never touch the resize cache, so this reader is a
// compile-time stand-in only; the resize path is covered by the package's serving_test.
// size=0 requests stream originals, so this reader is never called (serving_test covers resizing).
imgCache := cache.NewFileCache("ArtworkResolutionE2E", "100MB", "images", 0,
func(context.Context, cache.Item) (io.Reader, error) {
return nil, fmt.Errorf("resize not exercised in e2e")
@ -129,7 +121,7 @@ func setupResolutionHarness() {
rworker = artwork.NewWorker(rds, rstore, agents.GetAgents(rds, nil), ffm, events.NoopBroker(), imgCache)
}
// setLayout populates the fake library. All paths must be forward-slash and relative.
// setLayout paths must be relative and forward-slash.
func setLayout(files fstest.MapFS) {
GinkgoHelper()
fakeFS.SetFiles(files)
@ -143,8 +135,6 @@ func scan() {
Expect(err).ToNot(HaveOccurred())
}
// acquire drives the worker to resolve one entity and returns its persisted state row. It fails if
// the worker never settles (found or absent) within the timeout.
func acquire(kind model.Kind, id string) model.ItemArtwork {
GinkgoHelper()
rworker.Bump(kind.Prefix(), id)
@ -170,7 +160,6 @@ func runResolutionWorkerUntil(until func() bool) {
Eventually(done, 2*time.Second).Should(Receive(BeNil()))
}
// serveBytes reads an artwork ID through the real Service at full size and returns its bytes.
func serveBytes(artID model.ArtworkID) []byte {
GinkgoHelper()
img, err := rsvc.Get(rctx, artID, 0, false)
@ -189,7 +178,6 @@ func serveErr(artID model.ArtworkID) error {
return err
}
// libFileBytes returns the contents of the one library file whose path ends with suffix.
func libFileBytes(suffix string) []byte {
GinkgoHelper()
var match string
@ -203,10 +191,8 @@ func libFileBytes(suffix string) []byte {
return fakeFS.MapFS[match].Data
}
// expectAlbumFolderCover asserts the album resolves to the library image at the given path suffix,
// byte-for-byte. The serve happens before acquisition on purpose: with no state row the request
// path resolves locally through the library FS, whereas a settled folder row is file-backed and
// read with os.Open, which the in-memory FS cannot satisfy.
// Serving before acquiring is deliberate: with no state row the request resolves through the
// library FS, while a settled folder row is read with os.Open, which the in-memory FS cannot serve.
func expectAlbumFolderCover(al model.Album, suffix string) {
GinkgoHelper()
requireNoStateRow(model.KindAlbumArtwork, al.ID)
@ -216,9 +202,7 @@ func expectAlbumFolderCover(al model.Album, suffix string) {
Expect(filepath.ToSlash(ia.SourcePath)).To(HaveSuffix(suffix))
}
// requireNoStateRow guards the byte-level folder assertions: a drain resolves every ready queue
// row, so acquiring one entity can settle others. Once settled, folder art is file-backed and the
// in-memory FS cannot serve it — so these assertions must come before any acquire in a spec.
// A drain settles every ready item, so byte-level folder assertions must precede any acquire.
func requireNoStateRow(kind model.Kind, id string) {
GinkgoHelper()
_, err := rds.Artwork(rctx).GetItemArtwork(kind, id, model.ImageTypePrimary)
@ -226,7 +210,6 @@ func requireNoStateRow(kind model.Kind, id string) {
"assert %s %q before acquiring any other entity in this spec", kind, id)
}
// expectAlbumAbsent asserts the album settled absent (no source resolved) and serves unavailable.
func expectAlbumAbsent(al model.Album) {
GinkgoHelper()
ia := acquire(model.KindAlbumArtwork, al.ID)
@ -234,8 +217,6 @@ func expectAlbumAbsent(al model.Album) {
Expect(serveErr(al.CoverArtID())).To(MatchError(artwork.ErrUnavailable))
}
// expectArtistFolder asserts the worker selected a library folder image (artist.*, album/artist.*)
// as the artist image; like album folder art it is file-backed, so it is asserted on the state row.
func expectArtistFolder(ar model.Artist, suffix string) {
GinkgoHelper()
requireNoStateRow(model.KindArtistArtwork, ar.ID)
@ -245,8 +226,6 @@ func expectArtistFolder(ar model.Artist, suffix string) {
Expect(filepath.ToSlash(ia.SourcePath)).To(HaveSuffix(suffix))
}
// writeUploadedImage drops raw bytes into the per-entity upload folder under DataFolder, matching
// the layout model.UploadedImagePath expects. Uploads are real files on disk, so they serve back.
func writeUploadedImage(entity, filename string, data []byte) {
GinkgoHelper()
dst := model.UploadedImagePath(entity, filename)
@ -254,21 +233,17 @@ func writeUploadedImage(entity, filename string, data []byte) {
Expect(os.WriteFile(dst, data, 0o600)).To(Succeed())
}
// discArtID is the artwork ID for one disc of an album.
func discArtID(al model.Album, disc int) model.ArtworkID {
return model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, disc), &al.UpdatedAt)
}
// expectDiscImage asserts a multi-disc album serves the given disc's art byte-for-byte. Disc art
// is a pure serve-time read through the library FS (no worker/state row), so this serves it live.
// Disc art is a pure serve-time read through the library FS: no worker, no state row.
func expectDiscImage(al model.Album, disc int, label string) {
GinkgoHelper()
Expect(serveBytes(discArtID(al, disc))).To(Equal(pngBytes(label)))
}
// gridQuadrants decodes a generated 2x2 playlist cover and samples the center of each quadrant,
// in rect() order: top-left, top-right, bottom-left, bottom-right. Each tile is a solid color, so
// the samples identify which album art landed where (and whether tiles were mirrored).
// Samples in rect() order: top-left, top-right, bottom-left, bottom-right.
func gridQuadrants(data []byte) [4]color.RGBA {
GinkgoHelper()
img, _, err := image.Decode(bytes.NewReader(data))
@ -282,9 +257,7 @@ func gridQuadrants(data []byte) [4]color.RGBA {
return [4]color.RGBA{at(qw, qh), at(3*qw, qh), at(qw, 3*qh), at(3*qw, 3*qh)}
}
// storedBytes returns the bytes the worker placed in the content-addressed store for a
// store-backed resolution (embedded/generated). Folder/upload sources are file-backed and are
// not in the store; assert those on ia.SourcePath instead.
// Store-backed sources only (embedded/generated); file-backed ones assert on ia.SourcePath.
func storedBytes(ia model.ItemArtwork) []byte {
GinkgoHelper()
art, err := rds.Artwork(rctx).GetImage(ia.Hash)
@ -297,9 +270,7 @@ func storedBytes(ia model.ItemArtwork) []byte {
return data
}
// smallPNG builds a tiny valid PNG whose pixel color is derived from label, so the bytes are
// distinct per label (a resolver picking a different file yields a different hash/path) while
// still decoding cleanly for the worker's blurhash step.
// The pixel color derives from label, so each label yields distinct, still-decodable bytes.
func smallPNG(label string) *fstest.MapFile {
h := fnv.New32a()
_, _ = h.Write([]byte(label))
@ -316,13 +287,11 @@ func smallPNG(label string) *fstest.MapFile {
return &fstest.MapFile{Data: buf.Bytes()}
}
// pngBytes returns the bytes smallPNG(label) writes, for byte-for-byte serve assertions.
func pngBytes(label string) []byte {
GinkgoHelper()
return smallPNG(label).Data
}
// trackFile builds a fake MP3 entry with optional tag overrides (album, disc, discsubtitle, ...).
func trackFile(num int, title string, extra ...map[string]any) *fstest.MapFile {
tags := storagetest.Track(num, title)
for _, e := range extra {
@ -333,10 +302,8 @@ func trackFile(num int, title string, extra ...map[string]any) *fstest.MapFile {
return storagetest.MP3(tags)
}
// embeddedArtFixture is a real MP3 with an embedded picture; FakeFS's JSON-encoded tags aren't
// taglib-readable, so embedded-art scenarios swap these bytes in after scanning. embeddedArtBytes
// is the exact image taglib extracts from it. Both load lazily (after tests.Init chdirs to the
// project root and registers Gomega), via loadEmbeddedFixture from setupResolutionHarness.
// FakeFS's JSON-encoded tags aren't taglib-readable, so embedded-art specs swap in these real MP3
// bytes after scanning. Loaded lazily: tests.Init must chdir to the project root first.
var (
embeddedFixtureOnce sync.Once
embeddedArtFixture []byte
@ -367,8 +334,6 @@ func extractEmbeddedArt(mp3 []byte) []byte {
return data
}
// replaceWithRealMP3 swaps the fake entry at relPath for the real embedded-art MP3, so the
// library FS returns a taglib-parseable stream during resolution.
func replaceWithRealMP3(relPath string) {
GinkgoHelper()
fakeFS.MapFS[relPath] = &fstest.MapFile{Data: embeddedArtFixture}

View File

@ -21,8 +21,6 @@ import (
)
const (
// maxArtistFolderTraversalDepth defines how many directory levels to search
// when looking for artist images (artist folder + parent directories)
maxArtistFolderTraversalDepth = 3
)
@ -35,8 +33,7 @@ func fromArtistFolder(ctx context.Context, libFS fs.FS, libPath, artistFolder, p
if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
return nil, "", fmt.Errorf(`artist folder '%s' is outside library '%s'`, artistFolder, libPath)
}
// fs.Glob / path.Join below expect forward-slash paths; filepath.Rel may
// return backslash separators on Windows.
// fs.Glob needs forward slashes; filepath.Rel returns backslashes on Windows.
rel = filepath.ToSlash(rel)
current := artistFolder
var unreadable error
@ -49,7 +46,7 @@ func fromArtistFolder(ctx context.Context, libFS fs.FS, libPath, artistFolder, p
unreadable = err
}
if rel == "." {
break // reached library root; don't traverse above it
break // reached library root
}
rel = path.Dir(rel)
current = filepath.Dir(current)
@ -61,10 +58,8 @@ func fromArtistFolder(ctx context.Context, libFS fs.FS, libPath, artistFolder, p
}
}
// findImageInFolder globs libFS at relFolder for pattern and returns the first
// matching image. absFolder is used only for the returned display path and log
// messages so callers see absolute-looking paths consistent with the rest of
// the artwork pipeline.
// findImageInFolder returns the first image matching pattern; absFolder is only used for
// the returned display path and log messages.
func findImageInFolder(ctx context.Context, libFS fs.FS, relFolder, absFolder, pattern string) (io.ReadCloser, string, error) {
log.Trace(ctx, "Artwork: Looking for artist image", "pattern", pattern, "folder", absFolder)
globPattern := pattern
@ -77,7 +72,6 @@ func findImageInFolder(ctx context.Context, libFS fs.FS, relFolder, absFolder, p
return nil, "", err
}
// Filter to valid image files
var imagePaths []string
for _, m := range matches {
if !model.IsImageFile(m) {
@ -86,8 +80,7 @@ func findImageInFolder(ctx context.Context, libFS fs.FS, relFolder, absFolder, p
imagePaths = append(imagePaths, m)
}
// Sort image files by prioritizing base filenames without numeric
// suffixes (e.g., artist.jpg before artist.1.jpg)
// Prefer base filenames over numeric-suffixed ones (artist.jpg before artist.1.jpg)
slices.SortFunc(imagePaths, compareImageFiles)
var openErr error
@ -125,7 +118,7 @@ func loadArtistFolder(ctx context.Context, ds model.DataStore, albums model.Albu
if len(albums) == 0 {
return "", time.Time{}, nil
}
libID := albums[0].LibraryID // Just need one of the albums, as they should all be in the same Library - for now! TODO: Support multiple libraries
libID := albums[0].LibraryID // TODO: Support albums spanning multiple libraries
folderPath := str.LongestCommonPrefix(paths)
if !strings.HasSuffix(folderPath, string(filepath.Separator)) {
@ -133,15 +126,13 @@ func loadArtistFolder(ctx context.Context, ds model.DataStore, albums model.Albu
}
folderPath = filepath.Dir(folderPath)
// Manipulate the path to get the folder ID
// TODO: This is a bit hacky, but it's the easiest way to get the folder ID, ATM
// TODO: Hacky, but the easiest way to get the folder ID ATM
libPath := core.AbsolutePath(ctx, ds, libID, "")
folderID := model.FolderID(model.Library{ID: libID, Path: libPath}, folderPath)
log.Trace(ctx, "Artwork: Calculating artist folder details", "folderPath", folderPath, "folderID", folderID,
"libPath", libPath, "libID", libID, "albumPaths", paths)
// Get the last update time for the folder
folders, err := ds.Folder(ctx).GetAll(model.QueryOptions{Filters: squirrel.Eq{"folder.id": folderID, "missing": false}})
if err != nil || len(folders) == 0 {
log.Warn(ctx, "Artwork: Could not find folder for artist", "folderPath", folderPath, "id", folderID,
@ -151,8 +142,7 @@ func loadArtistFolder(ctx context.Context, ds model.DataStore, albums model.Albu
return folderPath, folders[0].ImagesUpdatedAt, nil
}
// findImageInArtistFolder scans a folder for an image file matching the artist's MBID or name
// (case-insensitive). Returns the full path, or empty string if not found.
// findImageInArtistFolder matches an image by MBID or artist name (case-insensitive), "" if none.
func findImageInArtistFolder(folder, mbzArtistID, artistName string) string {
entries, err := os.ReadDir(folder)
if err != nil {

View File

@ -10,9 +10,8 @@ import (
. "github.com/onsi/gomega"
)
// unreadableFS globs like its embedded MapFS but refuses to open anything, standing in for a
// stale mount or a permissions failure. Injecting the error keeps this independent of the
// filesystem: os.Chmod does not restrict read access on Windows.
// unreadableFS globs like its embedded MapFS but refuses to open anything. Injecting the error
// keeps this independent of the filesystem: os.Chmod does not restrict read access on Windows.
type unreadableFS struct{ fstest.MapFS }
func (u unreadableFS) Open(string) (fs.File, error) { return nil, fs.ErrPermission }

View File

@ -21,36 +21,33 @@ const (
var errBreakerOpen = errors.New("artwork: external circuit breaker open")
// gateFunc gates one named external fetch (rate limit + circuit breaker per name).
// resolveItem defaults to passthroughGate; the worker injects the per-agent gate.
type gateFunc = func(name string, f func() (io.ReadCloser, string, error)) (io.ReadCloser, string, error)
func passthroughGate(_ string, f func() (io.ReadCloser, string, error)) (io.ReadCloser, string, error) {
return f()
}
// isTransientExternal reports whether an external step failed in a way worth retrying;
// a not-found (from either package) is a definitive answer, not a fault.
// isTransientExternal reports whether an external failure is worth retrying; a not-found
// (from either package) is a definitive answer, not a fault.
func isTransientExternal(err error) bool {
return err != nil && !errors.Is(err, agents.ErrNotFound) && !errors.Is(err, model.ErrNotFound)
}
// extGate is one agent's rate limiter + circuit breaker; each external agent gets its
// own so a provider whose API or CDN is down backs off in isolation from the others.
// extGate is one agent's rate limiter and circuit breaker, so a failing provider backs off
// in isolation from the others.
type extGate struct {
limiter *rate.Limiter
breaker *breaker
}
// gate wraps a named external step with that agent's own rate limiter and circuit
// breaker, matching gateFunc so it can be handed to the processor's resolver.
// gate runs a named external step through that agent's rate limiter and circuit breaker.
func (w *Worker) gate(name string, f func() (io.ReadCloser, string, error)) (io.ReadCloser, string, error) {
g := w.gateFor(name)
if !g.breaker.allow() {
log.Debug(w.runCtx, "Artwork: Skipping agent, circuit breaker open", "agent", name)
return nil, "", errBreakerOpen
}
// Waiting for the rate-limit permit is counted separately: a slow agent and a throttled one
// look identical from the drain, but only one of them is the provider's fault.
// Timed separately so a throttled agent isn't mistaken for a slow provider.
waitStart := time.Now()
if err := g.limiter.Wait(w.runCtx); err != nil {
return nil, "", err
@ -63,8 +60,7 @@ func (w *Worker) gate(name string, f func() (io.ReadCloser, string, error)) (io.
return r, path, err
}
// gateFor lazily creates the per-name gate on first use, each with its own limiter at
// ArtworkExternalMaxRPS and its own breaker.
// gateFor lazily creates the per-name gate on first use.
func (w *Worker) gateFor(name string) *extGate {
w.gatesMu.Lock()
defer w.gatesMu.Unlock()
@ -81,8 +77,8 @@ func (w *Worker) gateFor(name string) *extGate {
return g
}
// breaker opens after breakerThreshold consecutive external errors and admits a
// single probe once breakerProbeAfter has elapsed; a success re-closes it.
// breaker opens after breakerThreshold consecutive errors and admits a single probe once
// breakerProbeAfter has elapsed; a success re-closes it.
type breaker struct {
mu sync.Mutex
failures int
@ -107,8 +103,7 @@ func (b *breaker) allow() bool {
func (b *breaker) record(name string, err error) {
b.mu.Lock()
defer b.mu.Unlock()
// A not-found (from either package) is a definitive answer, not a fault; only real
// errors trip the breaker. Must stay consistent with isTransientExternal.
// A not-found is a definitive answer, not a fault; keep in sync with isTransientExternal.
if err == nil || errors.Is(err, model.ErrNotFound) || errors.Is(err, agents.ErrNotFound) {
if b.failures >= breakerThreshold {
log.Info("Artwork: Circuit breaker closed for agent", "agent", name)

View File

@ -15,28 +15,23 @@ import (
"github.com/navidrome/navidrome/model"
)
// staleAbsentAge is how old an absent resolution must be before the recheck job retries it.
const staleAbsentAge = 24 * time.Hour
// recheckKinds are the item kinds eligible for the periodic recheck jobs (stale-absent and
// missing-row). Media files are excluded: they resolve embedded-only, at scan or on view.
// recheckKinds omits media files: they resolve embedded-only, at scan or on view.
var recheckKinds = []model.Kind{
model.KindArtistArtwork, model.KindAlbumArtwork, model.KindPlaylistArtwork, model.KindRadioArtwork,
}
// hasRecheckPath reports whether a periodic job will revisit this kind, which is what makes
// settling absent on an exhausted retry budget recoverable rather than permanent.
// hasRecheckPath reports whether a periodic job will revisit this kind, making an absent settle recoverable.
func hasRecheckPath(prefix string) bool {
kind, ok := model.ParseKind(prefix)
return ok && slices.Contains(recheckKinds, kind)
}
// artworkEpoch invalidates all resolution state when bumped; bump it in the same change that
// alters resolution semantics. Deliberately not the server version, which changes every build.
// artworkEpoch invalidates all resolution state when bumped; bump it whenever resolution semantics change.
const artworkEpoch = 1
// fingerprint summarizes the inputs that affect artwork resolution outcomes; a
// change means previously resolved (or absent) state may no longer be correct.
// fingerprint covers the inputs that affect resolution outcomes; a change invalidates stored state.
func fingerprint() string {
raw := fmt.Sprintf("%s|%s|%s|%s|%t|%t|%d",
conf.Server.CoverArtPriority, conf.Server.ArtistArtPriority, conf.Server.ArtistImageFolder,
@ -45,8 +40,7 @@ func fingerprint() string {
return hex.EncodeToString(sum[:])
}
// backfill enqueues artwork resolution for every entity when the config fingerprint changed
// (or was never stored), artists first so those pages resolve before the larger backlog.
// backfill enqueues artwork resolution for every entity when the config fingerprint changed.
func backfill(ctx context.Context, ds model.DataStore) (bool, error) {
start := time.Now()
ctx = auth.WithAdminUser(ctx, ds)
@ -60,7 +54,7 @@ func backfill(ctx context.Context, ds model.DataStore) (bool, error) {
return false, nil
}
// Artists first: few entities, most external-dependent, so they get queue headstart.
// Artists first: few entities, most external-dependent, so they get a queue headstart.
kinds := []struct {
kind model.Kind
fetch func() ([]string, error)
@ -100,8 +94,6 @@ func enqueueBackfillKind(ctx context.Context, ds model.DataStore, kind model.Kin
return ds.ArtworkQueue(ctx).Enqueue(items...)
}
// enqueueStaleAbsentAll requeues absent-state entries older than staleAbsentAge, across
// every artwork-bearing kind, for the periodic recheck job.
func enqueueStaleAbsentAll(ctx context.Context, ds model.DataStore) error {
cutoff := time.Now().Add(-staleAbsentAge)
queue := ds.ArtworkQueue(ctx)
@ -113,8 +105,7 @@ func enqueueStaleAbsentAll(ctx context.Context, ds model.DataStore) error {
return nil
}
// enqueueMissingAll requeues entities that have no item_artwork row yet, across every recheck
// kind: the safety net for entities a scan never enqueued (added between scans, or scanner off).
// enqueueMissingAll is the safety net for entities a scan never enqueued (added between scans, or scanner off).
func enqueueMissingAll(ctx context.Context, ds model.DataStore) error {
queue := ds.ArtworkQueue(ctx)
for _, kind := range recheckKinds {
@ -125,8 +116,7 @@ func enqueueMissingAll(ctx context.Context, ds model.DataStore) error {
return nil
}
// Refresh clears an item's resolved artwork state and re-queues it at Bump priority, so a
// deliberate refresh (image upload or manual re-resolve) drops the current pick and re-resolves it.
// Refresh drops an item's resolved artwork state and re-queues it at Bump priority.
func Refresh(ctx context.Context, ds model.DataStore, kind model.Kind, id string) error {
if err := ds.Artwork(ctx).DeleteForItem(kind, id); err != nil {
return fmt.Errorf("clearing artwork state: %w", err)

View File

@ -213,9 +213,7 @@ var _ = Describe("Housekeeping", func() {
artRepo.ItemData["al-stale"] = model.ItemArtwork{ItemKind: "al", ItemID: "al1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old}
artRepo.ItemData["pl-stale"] = model.ItemArtwork{ItemKind: "pl", ItemID: "pl1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old}
artRepo.ItemData["ra-stale"] = model.ItemArtwork{ItemKind: "ra", ItemID: "ra1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: old}
// Not stale: too recent.
artRepo.ItemData["ar-recent"] = model.ItemArtwork{ItemKind: "ar", ItemID: "ar2", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: recent}
// Not absent: has a resolved hash.
artRepo.ItemData["al-resolved"] = model.ItemArtwork{ItemKind: "al", ItemID: "al2", ImageType: model.ImageTypePrimary, Hash: "somehash", AttemptedAt: old}
err := enqueueStaleAbsentAll(ctx, ds)
@ -250,7 +248,6 @@ var _ = Describe("Housekeeping", func() {
})
It("enqueues only entities that have no item_artwork row, across all kinds", func() {
// al1 is already resolved and ar1 already absent: both must be skipped.
artRepo.ItemData["al-resolved"] = model.ItemArtwork{ItemKind: "al", ItemID: "al1", ImageType: model.ImageTypePrimary, Hash: "somehash", AttemptedAt: time.Now()}
artRepo.ItemData["ar-absent"] = model.ItemArtwork{ItemKind: "ar", ItemID: "ar1", ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()}

View File

@ -16,8 +16,7 @@ import (
"github.com/zeebo/xxh3"
)
// ImageStore is the content-addressed store for artwork images that have no
// library file backing them (external downloads, embedded extractions, generated).
// ImageStore is the content-addressed store for artwork images with no library file backing them.
type ImageStore struct {
root string
}
@ -26,14 +25,12 @@ func NewImageStore(rootDir string) *ImageStore {
return &ImageStore{root: rootDir}
}
// GetImageStore roots the store in its own subtree under the data folder, so
// Prune's recursive sweep never reaches the per-entity upload folders next to it.
// GetImageStore roots the store in its own subtree so Prune's sweep never reaches the upload folders beside it.
func GetImageStore() *ImageStore {
return NewImageStore(filepath.Join(conf.Server.DataFolder.String(), consts.ArtworkFolder, "store"))
}
// 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.
// extForMime must stay stable across OSes: extensions are baked into stored paths and re-derived on Open.
func extForMime(m string) string {
switch m {
case "image/jpeg":
@ -56,8 +53,7 @@ func hashImage(r io.Reader) (string, error) {
return fmt.Sprintf("%016x", d.Sum64()), nil
}
// validHash rejects anything but 16 lowercase hex chars: known-absent states carry "",
// and malformed persisted hashes must never reach path sharding (slice panics, separators).
// validHash guards path sharding: a malformed hash would slice-panic or inject path separators.
func validHash(hash string) bool {
if len(hash) != 16 {
return false
@ -85,7 +81,7 @@ func (s *ImageStore) Write(hash, mimeType string, r io.Reader) error {
if err := os.Chtimes(dst, now, now); err == nil {
return nil
}
// touch failed (file likely pruned concurrently) — fall through and write it
// touch failed (likely pruned concurrently) — fall through and rewrite it
}
if err := os.MkdirAll(filepath.Dir(dst), 0755); err != nil {
return err
@ -112,8 +108,7 @@ func (s *ImageStore) Open(hash, mimeType string) (io.ReadCloser, error) {
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.
// Remove skips files newer than olderThan: an overlapping acquisition may not have committed its row yet.
func (s *ImageStore) Remove(hash, mimeType string, olderThan time.Time) error {
if !validHash(hash) {
return fmt.Errorf("imagestore: invalid hash %q", hash)
@ -136,9 +131,8 @@ func (s *ImageStore) Remove(hash, mimeType string, olderThan time.Time) error {
return err
}
// 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.
// The walk is cancellable: it runs under the worker's prune lock, which shutdown waits on.
// Sweep removes store files not accepted by keep. Files modified after cutoff are always
// kept: their acquisition row may not be committed yet.
func (s *ImageStore) Sweep(ctx context.Context, 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 {

View File

@ -18,9 +18,7 @@ import (
const tileSize = 600
// findPlaylistSidecarPath scans the directory of the playlist file for a sidecar
// image file with the same base name (case-insensitive). Returns empty string if
// no matching image is found or if plsPath is empty.
// findPlaylistSidecarPath finds an image beside plsPath with the same base name (case-insensitive).
func findPlaylistSidecarPath(ctx context.Context, plsPath string) string {
if plsPath == "" {
return ""
@ -59,25 +57,21 @@ func rect(pos int) image.Rectangle {
return r
}
// fillCenter crops the source image from the center and scales it to fill dstW x dstH exactly,
// equivalent to imaging.Fill with Center anchor.
// fillCenter center-crops src and scales it to fill dstW x dstH exactly.
func fillCenter(src image.Image, dstW, dstH int) image.Image {
srcBounds := src.Bounds()
srcW := srcBounds.Dx()
srcH := srcBounds.Dy()
// Calculate crop rectangle (center crop to match destination aspect ratio)
srcAspect := float64(srcW) / float64(srcH)
dstAspect := float64(dstW) / float64(dstH)
var cropRect image.Rectangle
if srcAspect > dstAspect {
// Source is wider — crop horizontally
cropW := int(float64(srcH) * dstAspect)
cropX := (srcW - cropW) / 2
cropRect = image.Rect(srcBounds.Min.X+cropX, srcBounds.Min.Y, srcBounds.Min.X+cropX+cropW, srcBounds.Max.Y)
} else {
// Source is taller — crop vertically
cropH := int(float64(srcW) / dstAspect)
cropY := (srcH - cropH) / 2
cropRect = image.Rect(srcBounds.Min.X, srcBounds.Min.Y+cropY, srcBounds.Max.X, srcBounds.Min.Y+cropY+cropH)
@ -88,10 +82,7 @@ func fillCenter(src image.Image, dstW, dstH int) image.Image {
return dst
}
// decodeTile and assembleTiles mirror playlistArtworkReader's createTile/
// createTiledImage, reusing the same rect/fillCenter cropping helpers.
// decodeTile runs on every sampled album's resolved bytes before the processor's
// own maxImageBytes/maxImagePixels guards apply, so it enforces them itself too.
// decodeTile runs before the processor's size guards apply, so it enforces the caps itself.
func decodeTile(r io.ReadCloser) (image.Image, error) {
data, err := readCapped(r)
if err != nil {

View File

@ -18,14 +18,13 @@ import (
xdraw "golang.org/x/image/draw"
)
// outcome tells the worker what to do with the queue row: found/absent
// delete it, failed reschedules it via MarkFailed.
// outcome tells the worker whether to delete the queue row (found/absent) or reschedule it.
type outcome int
const (
outcomeFound outcome = iota
// outcomeFoundStale: state was written and is served, but a higher-priority external
// step failed, so the row must retry (via MarkFailed) to give that source another chance.
// source failed, so the row must retry to give it another chance.
outcomeFoundStale
outcomeAbsent
outcomeFailed
@ -47,12 +46,12 @@ func (o outcome) String() string {
// thumbnailSize is the max dimension fed to blurhash.
const thumbnailSize = 128
// maxImageBytes caps a resolved image read: a user-editable ExternalImageURL could
// point at an arbitrarily large endpoint, and 20MB is generous for any real cover.
// maxImageBytes caps a resolved image read: a user-editable ExternalImageURL could point at
// an arbitrarily large endpoint.
const maxImageBytes = 20 << 20
// maxImagePixels caps declared dimensions: a tiny compressed file can declare a
// huge canvas that image.Decode would expand into gigabytes (decompression bomb).
// maxImagePixels guards against decompression bombs: a tiny file can declare a canvas that
// image.Decode would expand into gigabytes.
const maxImagePixels = 64 << 20
// acquired is what a successful acquire persisted, handed back so the caller can warm the resize
@ -63,8 +62,7 @@ type acquired struct {
data []byte
}
// processor turns one queue item into stored artwork. It owns acquisition only — settling the
// queue row afterwards is the Worker's job. pruneLock is nil in tests, where nothing prunes.
// processor turns one queue item into stored artwork; settling the queue row is the Worker's job.
type processor struct {
ds model.DataStore
store *ImageStore
@ -76,8 +74,6 @@ type processor struct {
// blurhash it, place its bytes, and persist the resulting state.
func (p *processor) acquire(ctx context.Context, item model.ArtworkQueueItem) (out outcome, got *acquired) {
repo := p.ds.Artwork(ctx)
// Timed on every exit, failures included: an item that is slow is usually one that failed
// slowly, and the outcome alone doesn't say whether the cost was the network or the decode.
start := time.Now()
defer func() {
log.Debug(ctx, "Artwork: Acquisition finished", "kind", item.ItemKind, "id", item.ItemID,
@ -91,8 +87,7 @@ func (p *processor) acquire(ctx context.Context, item model.ArtworkQueueItem) (o
}
if res.reader == nil {
if res.extError || res.localError {
// A source errored/timed out rather than answering "no image": never settle on
// absent, keep serving old state.
// A fault is not a definitive "no image": never settle absent, keep serving old state.
log.Debug(ctx, "Artwork: No image, but a source faulted; keeping previous state",
"kind", item.ItemKind, "id", item.ItemID, "extError", res.extError, "localError", res.localError)
return outcomeFailed, nil
@ -101,7 +96,6 @@ func (p *processor) acquire(ctx context.Context, item model.ArtworkQueueItem) (o
}
defer res.reader.Close()
// Times the download too: res.reader is the provider's response body for external sources.
readStart := time.Now()
data, err := readCapped(res.reader)
if err != nil {
@ -123,7 +117,6 @@ func (p *processor) acquire(ctx context.Context, item model.ArtworkQueueItem) (o
art, err := repo.GetImage(hash)
switch {
case err == nil:
// Dedup hit: identical bytes already known, reuse dims/mime/blurhash.
log.Debug(ctx, "Artwork: Reusing a known image, skipping decode", "kind", item.ItemKind,
"id", item.ItemID, "hash", hash)
case errors.Is(err, model.ErrNotFound):
@ -148,7 +141,6 @@ func (p *processor) acquire(ctx context.Context, item model.ArtworkQueueItem) (o
}
got = &acquired{ia: ia, mime: art.Mime, data: data}
if res.extError {
// Served, but a higher-priority external source errored: the pick may improve on retry.
log.Debug(ctx, "Artwork: Serving a lower-priority source after an external failure",
"kind", item.ItemKind, "id", item.ItemID, "source", res.source)
return outcomeFoundStale, got
@ -156,9 +148,8 @@ func (p *processor) acquire(ctx context.Context, item model.ArtworkQueueItem) (o
return outcomeFound, got
}
// persist places the bytes and commits the rows referencing them. Only this window excludes
// Prune, which reclaims store files no row points at; resolution stays outside so a slow fetch
// cannot hold prune off.
// persist places the bytes and commits the rows referencing them, excluding Prune for that
// window only so a slow resolution can never hold it off.
func (p *processor) persist(repo model.ArtworkRepository, item model.ArtworkQueueItem,
art *model.Artwork, res resolution, data []byte,
) (*model.ItemArtwork, error) {
@ -183,14 +174,14 @@ func (p *processor) persist(repo model.ArtworkRepository, item model.ArtworkQueu
RefMtime: refMtime,
AttemptedAt: time.Now(),
}
// PutItemArtwork stamps UpdatedAt on ia, so what it holds now matches the persisted row.
// PutItemArtwork stamps UpdatedAt on ia, so the returned struct matches the persisted row.
if err := repo.PutItemArtwork(ia); err != nil {
return nil, fmt.Errorf("persisting item artwork state: %w", err)
}
return ia, nil
}
// writeAbsent records a known-absent state: every local/external source answered definitively "no".
// writeAbsent records a known-absent state: every source answered definitively "no".
func writeAbsent(ctx context.Context, repo model.ArtworkRepository, item model.ArtworkQueueItem) outcome {
err := repo.PutItemArtwork(&model.ItemArtwork{
ItemKind: item.ItemKind,
@ -207,7 +198,6 @@ func writeAbsent(ctx context.Context, repo model.ArtworkRepository, item model.A
return outcomeAbsent
}
// readCapped reads r, rejecting anything over maxImageBytes.
func readCapped(r io.Reader) ([]byte, error) {
data, err := io.ReadAll(io.LimitReader(r, maxImageBytes+1))
if err != nil {
@ -219,15 +209,13 @@ func readCapped(r io.Reader) ([]byte, error) {
return data, nil
}
// decodeCapped rejects declared dimensions over maxImagePixels BEFORE the
// full-decode allocation, then decodes.
// decodeCapped rejects declared dimensions over maxImagePixels before the full-decode allocation.
func decodeCapped(data []byte) (image.Image, string, error) {
cfg, format, err := image.DecodeConfig(bytes.NewReader(data))
if err != nil {
return nil, "", fmt.Errorf("decode image config: %w", err)
}
// Compared by division so the cap holds for any dimensions a decoder might report, without
// depending on a multiplication staying inside int64.
// Compared by division so the cap cannot be defeated by an int64 overflow.
if cfg.Width <= 0 || cfg.Height <= 0 || cfg.Width > maxImagePixels/cfg.Height {
return nil, "", fmt.Errorf("image dimensions %dx%d exceed pixel cap %d", cfg.Width, cfg.Height, maxImagePixels)
}
@ -238,8 +226,7 @@ func decodeCapped(data []byte) (image.Image, string, error) {
return img, format, nil
}
// decodeArtwork builds a new Artwork row from raw bytes: dimensions, mime and a
// blurhash computed from a downscaled thumbnail.
// decodeArtwork builds a new Artwork row from raw bytes: dimensions, mime and blurhash.
func decodeArtwork(ctx context.Context, hash string, data []byte) (*model.Artwork, error) {
img, format, err := decodeCapped(data)
if err != nil {
@ -263,8 +250,7 @@ func decodeArtwork(ctx context.Context, hash string, data []byte) (*model.Artwor
}, nil
}
// makeThumbnail downscales img to fit within maxSize on its longest side.
// Images within bounds are returned as-is (no upscaling).
// makeThumbnail downscales img to fit within maxSize on its longest side; it never upscales.
func makeThumbnail(img image.Image, maxSize int) image.Image {
b := img.Bounds()
w, h := b.Dx(), b.Dy()
@ -277,14 +263,14 @@ func makeThumbnail(img image.Image, maxSize int) image.Image {
return dst
}
// isFileBacked reports whether a resolution's bytes already live in a library/upload
// file, so the acquisition must not duplicate them into the content-addressed store.
// isFileBacked reports whether the bytes already live in a library/upload file, so the
// content-addressed store must not duplicate them.
func isFileBacked(source string) bool {
return source == "folder" || source == "upload"
}
// placeBytes reports the item's backing-file provenance (folder/upload: image, embedded: audio,
// external/generated: none) and writes the bytes into the store for the non-file-backed sources.
// placeBytes reports the item's backing-file provenance and writes the bytes into the store
// for the sources that have none.
func placeBytes(store *ImageStore, art *model.Artwork, res resolution, data []byte) (sourcePath string, refMtime int64, err error) {
if isFileBacked(res.source) {
return res.sourcePath, res.refMtime, nil
@ -295,8 +281,7 @@ func placeBytes(store *ImageStore, art *model.Artwork, res resolution, data []by
return sourcePath, refMtime, store.Write(art.Hash, art.Mime, bytes.NewReader(data))
}
// mimeForFormat maps an image.Decode format name to its MIME type; extForMime
// in image_store.go performs the inverse for content-addressed file paths.
// mimeForFormat maps an image.Decode format name to its MIME type; extForMime is the inverse.
func mimeForFormat(format string) string {
switch format {
case "jpeg":

View File

@ -22,8 +22,7 @@ import (
. "github.com/onsi/gomega"
)
// pngHeaderWithDims builds just a PNG signature + IHDR chunk declaring w×h. DecodeConfig
// reads the header without touching pixel data, so the body can be omitted entirely.
// DecodeConfig reads only the header, so the pixel data can be omitted entirely.
func pngHeaderWithDims(w, h uint32) []byte {
ihdr := make([]byte, 13)
binary.BigEndian.PutUint32(ihdr[0:], w)
@ -124,8 +123,8 @@ var _ = Describe("processor.acquire", func() {
Expect(lock.held()).To(BeFalse(), "the window must close before acquire returns")
})
// Resolution can reach the network under its own timeout, so holding the lock across it
// would let one slow provider block prune, and every drain queued behind prune's writer.
// Resolution can reach the network, so holding the lock across it would let one slow
// provider block prune, and every drain queued behind prune's writer.
It("never takes it while only resolving", func() {
folderRepo.result = nil
ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{
@ -179,8 +178,7 @@ var _ = Describe("processor.acquire", func() {
It("failed-on-unreadable-local: a listed cover that will not open never records absent", func() {
conf.Server.CoverArtPriority = "cover.jpg"
// A healthy library whose folder listing names a cover the FS will not hand over —
// what a stale mount looks like from here.
// A folder listing that names a cover the FS will not hand over: a stale mount.
libRoot := GinkgoT().TempDir()
Expect(os.MkdirAll(filepath.Join(libRoot, "an-album"), 0o755)).To(Succeed())
libRepo.SetData(model.Libraries{{ID: 0, Path: testFileLibPath(libRoot)}})
@ -196,12 +194,10 @@ var _ = Describe("processor.acquire", func() {
Expect(err).To(MatchError(model.ErrNotFound), "an I/O fault must not be recorded as absent")
})
// An upload outranks every other source, so an unreadable one must neither settle absent
// nor let a lower-priority image take its place.
// An upload outranks every source, so an unreadable one must not let a lower one take over.
It("failed-on-unreadable-upload: an upload that will not open never records absent", func() {
if runtime.GOOS == "windows" {
// os.Chmod cannot revoke read access there, so the file would open and the spec
// would pass on the decode error instead of the unreadable source.
// The file would still open, so the spec would pass on the decode error instead.
Skip("chmod does not restrict read access on Windows")
}
radioRepo := tests.CreateMockedRadioRepo()
@ -277,7 +273,6 @@ var _ = Describe("processor.acquire", func() {
Expect(ia.Source).To(Equal("external:deezerFake"))
Expect(ia.Hash).ToNot(BeEmpty())
// External art is content-addressed into the store, not file-backed.
art, err := artRepo.GetImage(ia.Hash)
Expect(err).ToNot(HaveOccurred())
rc, err := store.Open(ia.Hash, art.Mime)
@ -300,8 +295,7 @@ var _ = Describe("processor.acquire", func() {
ia1, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "al5", model.ImageTypePrimary)
Expect(err).ToNot(HaveOccurred())
// Poison the stored blurhash: if the second item re-decodes instead of
// deduping on hash, this sentinel gets overwritten by a real computed value.
// A re-decode instead of a hash dedup would overwrite this sentinel.
poisoned := artRepo.Data[ia1.Hash]
poisoned.BlurHash = "SENTINEL"
artRepo.Data[ia1.Hash] = poisoned
@ -318,8 +312,7 @@ var _ = Describe("processor.acquire", func() {
})
It("two items, two files, identical bytes: each item keeps its own provenance; the shared artwork row is written once", func() {
// Two distinct library files with byte-identical content resolve to the same
// hash. Provenance is per-item, so neither file's path may overwrite the other.
// Byte-identical files share one hash, but provenance is per item.
libRoot := GinkgoT().TempDir()
imgBytes, err := os.ReadFile(filepath.Join(repoRoot, "tests/fixtures/artist/an-album/cover.jpg"))
Expect(err).ToNot(HaveOccurred())
@ -345,7 +338,7 @@ var _ = Describe("processor.acquire", func() {
Expect(filepath.ToSlash(iaA.SourcePath)).To(HaveSuffix("album-a/cover.jpg"))
Expect(iaA.RefMtime).To(Equal(time.Unix(1000, 0).UnixNano()))
// Poison the shared row's blurhash: the second item must dedup on hash, not re-decode.
// A re-decode instead of a hash dedup would overwrite this sentinel.
poisoned := artRepo.Data[iaA.Hash]
poisoned.BlurHash = "SENTINEL"
artRepo.Data[iaA.Hash] = poisoned
@ -359,13 +352,11 @@ var _ = Describe("processor.acquire", func() {
Expect(filepath.ToSlash(iaB.SourcePath)).To(HaveSuffix("album-b/cover.jpg"))
Expect(iaB.RefMtime).To(Equal(time.Unix(2000, 0).UnixNano()))
// The first item's provenance survives the second item processing identical bytes.
iaAafter, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "alA", model.ImageTypePrimary)
Expect(err).ToNot(HaveOccurred())
Expect(filepath.ToSlash(iaAafter.SourcePath)).To(HaveSuffix("album-a/cover.jpg"))
Expect(iaAafter.RefMtime).To(Equal(time.Unix(1000, 0).UnixNano()))
// One shared artwork row, and dedup preserved it untouched.
Expect(artRepo.Data).To(HaveLen(1))
reused, err := artRepo.GetImage(iaA.Hash)
Expect(err).ToNot(HaveOccurred())
@ -411,9 +402,8 @@ var _ = Describe("processor.acquire", func() {
Expect(err).To(MatchError(model.ErrNotFound))
})
// The cap is compared by division, so it cannot be slipped by dimensions whose product
// would overflow. No supported format can declare such dimensions today — image/png caps
// them at 2^30-1 and the rest are 16-bit — so this pins the arithmetic, not a live hole.
// The cap is compared by division so an overflowing product cannot slip past it; no supported
// format can declare these dimensions, so this pins the arithmetic, not a live hole.
DescribeTable("rejects out-of-range declared dimensions",
func(w, h uint32) {
_, err := decodeArtwork(ctx, "bomb", pngHeaderWithDims(w, h))
@ -451,8 +441,6 @@ var _ = Describe("processor.acquire", func() {
})
})
// countingLocker stands in for the worker's prune read-lock, recording how often and how long
// acquire holds it.
type countingLocker struct {
locks int
unlocks int

View File

@ -33,8 +33,7 @@ func prune(ctx context.Context, ds model.DataStore, store *ImageStore) error {
log.Info(ctx, "Artwork: Purged dangling queue rows", "count", queuePurged)
}
// 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.
// Files younger than the grace window may belong to acquisitions whose rows aren't committed yet.
cutoff := time.Now().Add(-pruneMinAge)
candidates, err := repo.GetOrphanHashes(cutoff)
if err != nil {
@ -48,8 +47,7 @@ func prune(ctx context.Context, ds model.DataStore, store *ImageStore) error {
if err := repo.DeleteOrphans(cutoff, candidates); err != nil {
return err
}
// DeleteOrphans may spare candidates reacquired since the snapshot; only remove files
// for rows actually gone (absent from the post-delete re-read).
// DeleteOrphans may spare candidates reacquired since the snapshot; only drop files whose rows are gone.
survivors, err := repo.GetImages(candidates)
if err != nil {
return err
@ -59,8 +57,6 @@ func prune(ctx context.Context, ds model.DataStore, store *ImageStore) error {
if _, ok := survivors[h]; ok {
continue
}
// A spared fresh file is at worst a stray a later sweep reclaims;
// Worker.RunPrune serializes prune against in-flight acquisitions.
if err := store.Remove(h, arts[h].Mime, cutoff); err != nil {
log.Warn(ctx, "Artwork: Could not remove orphan file", "hash", h, err)
}

View File

@ -107,7 +107,6 @@ var _ = Describe("Prune", func() {
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",
ImageType: model.ImageTypePrimary, Hash: h, Source: "folder"})).To(Succeed())
@ -144,8 +143,7 @@ var _ = Describe("Prune", func() {
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.
// The row is orphaned, but a concurrent acquisition just touched the file's mtime.
Expect(prune(context.Background(), ds, store)).To(Succeed())
@ -213,24 +211,19 @@ var _ = Describe("Prune", func() {
Expect(os.Chmod(shardDir, 0500)).To(Succeed())
DeferCleanup(func() { _ = os.Chmod(shardDir, 0755) })
// hb (blocked) is processed first: if store.Remove's failure aborted the loop
// instead of warning and continuing, hg would never be reached.
// hb is processed first: aborting on its Remove failure would leave hg unreached.
awRepo.OrphanHashes = []string{hb, hg}
// Prune still errors: Sweep independently revisits hb's leftover file and,
// unlike the loop below, has no warn-and-continue fallback of its own.
// Prune still errors: Sweep revisits hb's leftover file with no warn-and-continue of its own.
err := prune(context.Background(), ds, store)
Expect(err).To(HaveOccurred())
// hg: reached and fully pruned despite being queued after the failing hb -
// proof the loop didn't return/break on the first Remove error.
_, err = awRepo.GetImage(hg)
Expect(err).To(MatchError(model.ErrNotFound))
_, err = store.Open(hg, "image/jpeg")
Expect(os.IsNotExist(err)).To(BeTrue())
// hb: row still purged (DeleteOrphans doesn't depend on file removal), but the
// file itself survives since store.Remove failed and only warned.
// The row purge does not depend on file removal, so only the file survives.
_, err = awRepo.GetImage(hb)
Expect(err).To(MatchError(model.ErrNotFound))
rc, err := store.Open(hb, "image/jpeg")

View File

@ -21,12 +21,8 @@ import (
func init() {
conf.AddHook(func() {
// gen2brain/webp selects native (purego/libwebp) vs WASM in its own
// package init() and exposes the result only via webp.Dynamic(); there is
// no runtime way to switch back. On 32-bit ARM/x86 the purego callback path
// crashes (issue #5597), so those builds must be compiled with the
// "nodynamic" tag (see Dockerfile), which makes webp.Dynamic() report an
// error here and forces the safe WASM path.
// gen2brain/webp picks native vs WASM in its own init(), with no way to switch at
// runtime: 32-bit builds need the "nodynamic" tag (see Dockerfile) to force WASM.
if err := webp.Dynamic(); err != nil {
log.Debug("Artwork: Using WASM WebP encoder/decoder", "reason", err)
} else {
@ -49,7 +45,6 @@ func resizeImageData(ctx context.Context, ffm ffmpeg.FFmpeg, data []byte, size i
log.Trace(ctx, "Artwork: Resized image", "bytes", len(data), "size", size, "square", square,
"elapsed", time.Since(start))
}()
// Preserve animation for animated images
if isAnimatedGIF(data) {
if ffm.IsAvailable() {
// Animated GIF: convert to animated WebP via ffmpeg (with optional resize)
@ -67,11 +62,8 @@ func resizeImageData(ctx context.Context, ffm ffmpeg.FFmpeg, data []byte, size i
return resizeStaticImage(data, size, square)
}
// toFastScaleType converts images whose concrete type has no optimized scaler
// in x/image/draw (e.g. *image.NYCbCrA from WebP, *image.Paletted from indexed
// PNGs) into *image.RGBA, which has a fast path. Without this, CatmullRom.Scale
// falls back to a generic per-pixel At()/RGBA() loop that is several times
// slower. Fast-path types are returned unchanged.
// toFastScaleType converts types x/image/draw has no optimized scaler for (e.g. *image.NYCbCrA,
// *image.Paletted) to *image.RGBA, avoiding CatmullRom.Scale's generic per-pixel fallback.
func toFastScaleType(img image.Image) image.Image {
switch img.(type) {
case *image.RGBA, *image.NRGBA, *image.Gray, *image.YCbCr:

View File

@ -24,21 +24,19 @@ type resolution struct {
source string // model.ItemArtwork.Source value: "folder", "embedded", "external", "upload", "generated"
sourcePath string // backing library/upload file (folder/upload: the image; embedded: the audio file); "" otherwise
refMtime int64 // sourcePath mtime (unix-nanoseconds) at resolution; 0 when no sourcePath
// external source errored/timed out. With no reader: forces failed (never absent).
// On a hit: a higher-priority external step failed—serve this, but retry later.
// external source errored/timed out. With no reader it forces failed (never absent);
// on a hit a higher-priority external step failed—serve this, but retry later.
extError bool
// a local source that should have been readable wasn't (stale mount, permissions).
// With no reader: forces failed, so a transient I/O fault never records absent.
// a local source that should have been readable wasn't. With no reader it forces failed,
// so a transient I/O fault never records absent.
localError bool
}
// chainState carries what a priority walk has seen so far. A hit takes extErr with it so a
// transient external failure still schedules a retry, but not localErr: a local candidate a
// later source recovered from is re-listed by the scanner when it actually changes.
// transient external failure still retries; localErr is dropped, as the scanner re-lists changes.
type chainState struct{ extErr, localErr bool }
// try reports a hit, stamping the accumulated external failure onto it, and otherwise records
// the miss. Folding both into one call is what keeps the OR from being forgotten at a new site.
// try stamps the accumulated external failure onto a hit, and records the miss otherwise.
func (c *chainState) try(res resolution, ok bool) (resolution, bool) {
if ok {
res.extError = c.extErr
@ -53,15 +51,13 @@ func (c *chainState) exhausted() resolution {
return resolution{extError: c.extErr, localError: c.localErr}
}
// externalSource is the ability to reach the network: which agents to ask, and the per-agent
// rate limiter and circuit breaker to ask them through.
// externalSource holds the agents to ask and the rate limiter/circuit breaker to ask them through.
type externalSource struct {
agents *agents.Agents
gate gateFunc
}
// resolver walks a kind's priority chain and returns the first hit. A nil ext means local-only,
// so "may this resolution go external" is one fact rather than two that could disagree.
// resolver walks a kind's priority chain and returns the first hit; a nil ext means local-only.
type resolver struct {
ds model.DataStore
ffmpeg ffmpeg.FFmpeg
@ -75,8 +71,8 @@ func newResolver(ds model.DataStore, ag *agents.Agents, ffm ffmpeg.FFmpeg, gate
return &resolver{ds: ds, ffmpeg: ffm, ext: &externalSource{agents: ag, gate: gate}}
}
// newLocalResolver offers no parameter through which an external source could be supplied, so
// a request can neither reach the network nor sample album art for the worker-built grid.
// newLocalResolver builds a resolver that can neither reach the network nor sample album art
// for the worker-built grid.
func newLocalResolver(ds model.DataStore, ffm ffmpeg.FFmpeg) *resolver {
return &resolver{ds: ds, ffmpeg: ffm}
}
@ -115,8 +111,7 @@ func (r *resolver) fetchExternalArtist(ctx context.Context, ar model.Artist) (io
return fetchArtistImage(ctx, r.ext.agents, r.ext.gate, ar)
}
// resolveAlbum ports the folder/embedded/external selection from
// reader_album.go, walking conf.Server.CoverArtPriority.
// resolveAlbum walks conf.Server.CoverArtPriority over the folder, embedded and external sources.
func (r *resolver) resolveAlbum(ctx context.Context, albumID string) (resolution, error) {
al, err := r.ds.Album(ctx).Get(albumID)
if err != nil {
@ -154,8 +149,7 @@ func (r *resolver) resolveAlbum(ctx context.Context, albumID string) (resolution
return chain.exhausted(), nil
}
// resolveArtist ports the upload/folder/external selection from
// reader_artist.go: upload always wins, then conf.Server.ArtistArtPriority.
// resolveArtist tries the uploaded image first, then walks conf.Server.ArtistArtPriority.
func (r *resolver) resolveArtist(ctx context.Context, artistID string) (resolution, error) {
ar, err := r.ds.Artist(ctx).Get(artistID)
if err != nil {
@ -171,7 +165,7 @@ func (r *resolver) resolveArtist(ctx context.Context, artistID string) (resoluti
return upload, nil
}
// Only consider albums where the artist is the sole album artist, same as reader_artist.go.
// Only consider albums where the artist is the sole album artist.
als, err := r.ds.Album(ctx).GetAll(model.QueryOptions{
Filters: squirrel.And{
squirrel.Eq{"album_artist_id": artistID},
@ -230,8 +224,7 @@ func (r *resolver) resolveArtist(ctx context.Context, artistID string) (resoluti
return chain.exhausted(), nil
}
// resolvePlaylist ports reader_playlist.go's chain: uploaded image, sidecar,
// ExternalImageURL, then the generated 2x2 grid sourced through resolveAlbum.
// resolvePlaylist tries the uploaded image, the sidecar and ExternalImageURL, then a generated grid.
func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (resolution, error) {
pl, err := r.ds.Playlist(ctx).Get(playlistID)
if err != nil {
@ -252,8 +245,7 @@ func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (reso
return res, nil
}
}
// A local ExternalImageURL is a file-backed reference: serve it in place (staleness-checked,
// and available even on the request path). Only http(s) URLs need the gated remote fetch.
// A local ExternalImageURL is file-backed and served in place; only http(s) needs the gated fetch.
localImg, remoteImg := classifyPlaylistImage(pl.ExternalImageURL)
if localImg != "" {
res, ok := resolveLocalFile(localImg, "folder")
@ -265,8 +257,7 @@ func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (reso
}
}
if r.ext == nil {
// The remote ExternalImageURL fetch and the 2x2 grid are worker-only; a request must
// not fetch remotely nor sample album art synchronously.
// The remote fetch and the generated grid are worker-only; a request must do neither.
return resolution{}, nil
}
if remoteImg != nil && conf.Server.EnableM3UExternalAlbumArt {
@ -309,14 +300,13 @@ func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (reso
}
}
if len(tiles) == 0 {
// A tile-level failure must never resolve as a clean absent: propagate
// internal errors, and force extError for external ones.
// A tile-level failure must never resolve as a clean absent.
if tileErr != nil {
return resolution{}, fmt.Errorf("resolvePlaylist: sampled album art failed: %w", tileErr)
}
return resolution{extError: extErr}, nil
}
// Grow to 4 tiles by repeating what we have, mirroring reader_playlist.go's loadTiles.
// Grow to 4 tiles by repeating what we have.
switch len(tiles) {
case 2:
tiles = append(tiles, tiles[1], tiles[0])
@ -330,7 +320,7 @@ func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (reso
return resolution{reader: grid, source: "generated", extError: extErr}, nil
}
// resolveRadio ports reader_radio.go: only an uploaded image, no fallback.
// resolveRadio serves only an uploaded image; there is no fallback.
func (r *resolver) resolveRadio(ctx context.Context, radioID string) (resolution, error) {
radio, err := r.ds.Radio(ctx).Get(radioID)
if err != nil {
@ -340,8 +330,8 @@ func (r *resolver) resolveRadio(ctx context.Context, radioID string) (resolution
return res, nil
}
// resolveMediaFile resolves a track's own embedded art only; there is no folder or
// external fallback, so disabled/missing cover art is a definitive absent.
// resolveMediaFile resolves a track's own embedded art only, so disabled or missing cover art
// is a definitive absent.
func (r *resolver) resolveMediaFile(ctx context.Context, id string) (resolution, error) {
mf, err := r.ds.MediaFile(ctx).Get(id)
if err != nil {
@ -358,9 +348,8 @@ func (r *resolver) resolveMediaFile(ctx context.Context, id string) (resolution,
return res, nil
}
// resolveExternalStep runs a single external sourceFunc through the named gate; used by
// the playlist ExternalImageURL step. ok reports a hit; extErr reports a non-not-found
// error (a not-found is a definitive "no", not a failure).
// resolveExternalStep runs a single external sourceFunc through the named gate. extErr excludes
// a not-found, which is a definitive "no" rather than a failure.
func resolveExternalStep(gate gateFunc, name string, sf sourceFunc) (res resolution, ok bool, extErr bool) {
r, path, err := gate(name, sf)
if r != nil {
@ -369,8 +358,8 @@ func resolveExternalStep(gate gateFunc, name string, sf sourceFunc) (res resolut
return resolution{}, false, err != nil && !errors.Is(err, model.ErrNotFound)
}
// classifyPlaylistImage splits a playlist ExternalImageURL into a local filesystem path
// (served file-backed) or a remote http(s) URL (fetched and stored); at most one is set.
// classifyPlaylistImage splits a playlist ExternalImageURL into a local filesystem path or a
// remote http(s) URL; at most one is set.
func classifyPlaylistImage(imageURL string) (localPath string, remote *url.URL) {
if imageURL == "" {
return "", nil
@ -429,8 +418,8 @@ func resolveArtistFolderPattern(ctx context.Context, lib libraryView, artistFold
return resolution{reader: r, source: "folder", sourcePath: path, refMtime: mtimeOf(path)}, true
}
// resolveLocalFile opens an absolute path directly (uploads, image-folder). A missing path is
// "no source"; any other open failure says nothing about whether the image exists.
// resolveLocalFile opens an absolute path directly. A missing path is "no source"; any other
// open failure says nothing about whether the image exists.
func resolveLocalFile(path, source string) (resolution, bool) {
if path == "" {
return resolution{}, false
@ -450,8 +439,7 @@ func mtimeOf(path string) int64 {
return info.ModTime().UnixNano()
}
// mtimeViaFS stats through the library FS instead of a joined absolute path,
// since library roots in tests may not be real OS paths (e.g. testfile://).
// mtimeViaFS stats through the library FS, since library roots in tests may not be real OS paths.
func mtimeViaFS(fsys fs.FS, name string) int64 {
if fsys == nil || name == "" {
return 0

View File

@ -387,8 +387,7 @@ var _ = Describe("resolveItem", func() {
Expect(img.Bounds().Dx()).To(Equal(expectedSize))
Expect(img.Bounds().Dy()).To(Equal(expectedSize))
},
// tileSize-1: the 4-tile canvas is built as [0, tileSize-1], matching
// reader_playlist.go's createTiledImage exactly.
// tileSize-1: the 4-tile canvas is built as [0, tileSize-1].
Entry("1 album -> single tile", []string{"t1"}, tileSize/2),
Entry("2 albums -> duplicated to 4 tiles", []string{"t1", "t2"}, tileSize-1),
Entry("3 albums -> duplicated to 4 tiles", []string{"t1", "t2", "t3"}, tileSize-1),
@ -508,8 +507,7 @@ var _ = Describe("resolveItem", func() {
Expect(res.extError).To(BeFalse())
})
// A local resolver holds no agents, and the external branch dereferences them before the
// gate is ever consulted, so reaching it at all would panic rather than degrade.
// A local resolver holds no agents: reaching the external branch would panic, not degrade.
It("skips the external step instead of dereferencing absent agents", func() {
conf.Server.CoverArtPriority = "external"
ds.MockedAlbum = tests.CreateMockAlbumRepo()
@ -521,8 +519,7 @@ var _ = Describe("resolveItem", func() {
Expect(res.extError).To(BeFalse(), "a skipped step is not a failed one")
})
// The request path must never reach the network nor sample album art synchronously. The
// worker resolving the same playlist is asserted alongside, so this cannot pass vacuously.
// The worker resolving the same playlist is asserted alongside, so this cannot pass vacuously.
It("resolves a playlist locally without fetching remotely or building the grid", func() {
conf.Server.EnableM3UExternalAlbumArt = true
var hits atomic.Int32
@ -587,7 +584,6 @@ var _ = Describe("resolveItem", func() {
})
It("skips a grid tile whose declared dimensions are a decompression bomb", func() {
// End-to-end regression: a bomb-declaring tile must not break the grid.
libRoot := GinkgoT().TempDir()
Expect(os.MkdirAll(filepath.Join(libRoot, "bomb"), 0755)).To(Succeed())
Expect(os.WriteFile(filepath.Join(libRoot, "bomb", "cover.jpg"), pngHeaderWithDims(50000, 50000), 0600)).To(Succeed())
@ -606,8 +602,7 @@ var _ = Describe("resolveItem", func() {
})
It("does not resolve as absent when every sampled album fails to resolve", func() {
// "missing1"/"missing2" are not in MockAlbumRepo's data, so resolveAlbum
// returns a genuine (non-external) error for every sampled tile.
// The album ids are absent from MockAlbumRepo, so every tile fails non-externally.
plRepo := tests.CreateMockPlaylistRepo()
plRepo.SetData(model.Playlists{{ID: "pl3", Name: "Playlist"}})
plRepo.TracksRepo = &tests.MockPlaylistTrackRepo{AlbumIDs: []string{"missing1", "missing2"}}
@ -620,8 +615,7 @@ var _ = Describe("resolveItem", func() {
})
})
// decodeTile runs on every sampled album's resolved bytes before the processor's
// own guards apply, so it must enforce the same caps independently.
// decodeTile runs before the processor's own guards, so it must enforce the caps itself.
var _ = Describe("decodeTile", func() {
It("rejects a decompression bomb before the full decode", func() {
data := pngHeaderWithDims(50000, 50000) // 2.5 gigapixels, far above the cap

View File

@ -187,9 +187,8 @@ func fromURL(ctx context.Context, imageUrl *url.URL) (io.ReadCloser, string, err
if err != nil {
return nil, "", err
}
// A dead image URL is a definitive miss, not a transient fault: agents (e.g. Last.fm) can
// advertise an image URL that 404s. Map it to ErrNotFound so it settles absent instead of
// retrying forever and tripping the artwork breaker.
// An agent-advertised URL that 404s is a definitive miss, not a fault: settle absent
// instead of retrying forever and tripping the artwork breaker.
if resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusGone {
resp.Body.Close()
return nil, "", model.ErrNotFound

View File

@ -15,8 +15,7 @@ import (
"github.com/navidrome/navidrome/utils"
)
// MaxImageUploadSize returns the configured MaxImageUploadSize in bytes, or the built-in default
// when it's unset/invalid. Shared by every API that accepts image uploads.
// MaxImageUploadSize returns the configured max upload size in bytes, or the built-in default.
func MaxImageUploadSize() int64 {
if size, err := humanize.ParseBytes(conf.Server.MaxImageUploadSize); err == nil && size > 0 {
return int64(size)
@ -25,18 +24,15 @@ func MaxImageUploadSize() int64 {
return int64(size)
}
// Uploader stores a user-uploaded entity image and invalidates that entity's artwork state so the
// upload becomes the served cover.
// Uploader stores a user-uploaded entity image and invalidates that entity's artwork state.
type Uploader interface {
SetImage(ctx context.Context, entityType string, entityID string, name string, oldPath string, reader io.Reader, ext string) (filename string, err error)
RemoveImage(ctx context.Context, path string) error
// EnqueueArtwork clears an item's resolved state and re-queues it at Bump priority. Callers
// must invoke it AFTER persisting the new filename, so the worker never resolves the old one.
// EnqueueArtwork re-resolves the item's artwork. Call it AFTER persisting the new
// filename, or the worker resolves the old one.
EnqueueArtwork(ctx context.Context, entityType, entityID string)
}
// uploadEntityKind maps an upload's entity type to its artwork kind prefix, so a
// successful upload can clear and re-queue that item's artwork state.
var uploadEntityKind = map[string]model.Kind{
consts.EntityArtist: model.KindArtistArtwork,
consts.EntityPlaylist: model.KindPlaylistArtwork,
@ -59,14 +55,12 @@ func (s *uploader) SetImage(ctx context.Context, entityType string, entityID str
return "", fmt.Errorf("creating image directory: %w", err)
}
// Remove old image if it exists
if oldPath != "" {
if err := os.Remove(oldPath); err != nil && !os.IsNotExist(err) {
log.Warn(ctx, "Artwork: Failed to remove old image", "path", oldPath, err)
}
}
// Save new image
f, err := os.Create(absPath)
if err != nil {
return "", fmt.Errorf("creating image file: %w", err)
@ -79,8 +73,6 @@ func (s *uploader) SetImage(ctx context.Context, entityType string, entityID str
return filename, nil
}
// EnqueueArtwork clears the item's resolved state and re-queues it at Bump priority: the
// upload is now the top-priority source, so the worker re-resolves and the UI swaps.
func (s *uploader) EnqueueArtwork(ctx context.Context, entityType, id string) {
kind, ok := uploadEntityKind[entityType]
if !ok {

View File

@ -22,13 +22,13 @@ import (
const (
workerPollInterval = 5 * time.Second
backoffBase = 5 * time.Second
// giveUpAfter bounds the retry budget from enqueue: past it the worker stops retrying and
// hands the item to the periodic stale-absent recheck (settling absent on a bare failure).
// giveUpAfter bounds the retry budget from enqueue; past it the item falls to the
// periodic stale-absent recheck.
giveUpAfter = 12 * time.Hour
)
// drainPool drains one class of work with its own slot budget, so a kind whose resolution
// blocks cannot occupy slots another kind needs.
// drainPool drains one class of work with its own slot budget, so a blocking kind cannot
// occupy slots another kind needs.
type drainPool struct {
name string
kinds []string
@ -36,9 +36,8 @@ type drainPool struct {
wake chan struct{}
}
// Worker drains the artwork queue through the processor: each external agent is rate-limited
// and circuit-broken independently, and prune is serialized against the store-write window
// via pruneMu.
// Worker drains the artwork queue: each external agent is rate-limited and circuit-broken
// independently, and pruneMu serializes prune against the store-write window.
type Worker struct {
proc *processor
cache cache.FileCache
@ -67,8 +66,8 @@ func NewWorker(ds model.DataStore, store *ImageStore, ag *agents.Agents, ffmpeg
return w
}
// newDrainPools splits the drain by what bounds it: gate() waits for its rate-limit permit
// while holding a slot, so a sleeping lookup would otherwise crowd out a cover sitting on disk.
// newDrainPools splits the drain by what bounds it: gate() holds a slot while waiting for its
// rate-limit permit, so a sleeping lookup would crowd out a cover sitting on disk.
func newDrainPools() []*drainPool {
budget := conf.MaxOpenConns() // floored at 4, so both remainders below stay positive
local := min(max(1, conf.Server.ArtworkWorkerConcurrency), budget-1)
@ -80,8 +79,7 @@ func newDrainPools() []*drainPool {
}
}
// Kind is a proxy for cost: an album whose chain reaches "external" still costs a local slot,
// but only the few with no local art do.
// Kind is a proxy for cost: an album that reaches an external agent still costs a local slot.
var (
externalDrainKinds = []string{model.KindArtistArtwork.Prefix()}
localDrainKinds = []string{
@ -92,8 +90,7 @@ var (
}
)
// Run blocks draining the queue until ctx is cancelled. It exits cleanly with no
// leaked goroutines: each drain waits for its batch before the loop can return.
// Run blocks draining the queue until ctx is cancelled.
func (w *Worker) Run(ctx context.Context) error {
w.runCtx = ctx
var wg sync.WaitGroup
@ -104,7 +101,6 @@ func (w *Worker) Run(ctx context.Context) error {
return nil
}
// runPool drains one pool's kinds until ctx is cancelled.
func (w *Worker) runPool(ctx context.Context, p *drainPool) {
ticker := time.NewTicker(workerPollInterval)
defer ticker.Stop()
@ -117,7 +113,7 @@ func (w *Worker) runPool(ctx context.Context, p *drainPool) {
return
}
if n > 0 {
continue // keep draining while this pool has ready work
continue
}
select {
case <-ctx.Done():
@ -128,8 +124,7 @@ func (w *Worker) runPool(ctx context.Context, p *drainPool) {
}
}
// Bump enqueues an item at the highest priority and wakes the drain loop. It is
// non-blocking: a wake already pending is enough.
// Bump enqueues an item at the highest priority and wakes the drain loops.
func (w *Worker) Bump(kind, id string) {
item := model.ArtworkQueueItem{
ItemKind: kind,
@ -141,8 +136,7 @@ func (w *Worker) Bump(kind, id string) {
log.Warn("Artwork: Could not bump queue item", "kind", kind, "id", id, err)
return
}
// Waking all beats routing by kind: a spurious wake costs one empty dequeue, while an
// unrouted kind would never wake at all.
// Wake every pool: a spurious wake only costs an empty dequeue.
for _, p := range w.pools {
select {
case p.wake <- struct{}{}:
@ -159,27 +153,25 @@ func (w *Worker) RunPrune(ctx context.Context) error {
return prune(ctx, w.proc.ds, w.proc.store)
}
// Backfill enqueues every entity for re-resolution when the artwork config fingerprint has
// changed since the last run, artists first. It reports whether anything was enqueued.
// Backfill enqueues every entity for re-resolution when the artwork config fingerprint changed,
// artists first. It reports whether anything was enqueued.
func (w *Worker) Backfill(ctx context.Context) (bool, error) {
return backfill(ctx, w.proc.ds)
}
// EnqueueStaleAbsentAll requeues known-absent entries older than staleAbsentAge, so artwork
// that appeared since the last attempt is eventually picked up.
// EnqueueStaleAbsentAll requeues known-absent entries older than staleAbsentAge.
func (w *Worker) EnqueueStaleAbsentAll(ctx context.Context) error {
return enqueueStaleAbsentAll(ctx, w.proc.ds)
}
// EnqueueMissingAll requeues entities that have no artwork state row at all: the safety net
// for anything a scan never enqueued.
// EnqueueMissingAll requeues entities with no artwork state row: the safety net for anything
// a scan never enqueued.
func (w *Worker) EnqueueMissingAll(ctx context.Context) error {
return enqueueMissingAll(ctx, w.proc.ds)
}
func (w *Worker) drain(ctx context.Context, concurrency int, kinds ...string) (int, error) {
// Dequeue well past the worker pool so a slow item (an external lookup burning its
// timeout) never idles the other slots: the pool stays fed until the batch runs out.
// Dequeue well past the pool size so a slow external lookup never idles the other slots.
// DequeueBatch does not mark rows taken, so this is one query per pass, not per slot.
items, err := w.proc.ds.ArtworkQueue(ctx).DequeueBatch(max(16, 4*concurrency), kinds...)
if err != nil {
@ -189,8 +181,8 @@ func (w *Worker) drain(ctx context.Context, concurrency int, kinds ...string) (i
return 0, nil
}
drainStart := time.Now()
// Resolved only once there is work, and per drain rather than per item: the worker needs an
// admin identity for private playlists, and can start before any admin exists.
// Private playlists need an admin identity; resolved per drain because the worker can
// start before any admin exists.
ctx = auth.WithAdminUser(ctx, w.proc.ds)
sem := make(chan struct{}, concurrency)
var wg sync.WaitGroup
@ -206,16 +198,15 @@ func (w *Worker) drain(ctx context.Context, concurrency int, kinds ...string) (i
wg.Go(func() {
defer func() { <-sem }()
out, got := w.process(ctx, item)
// Refresh clients on any visible state change: found/foundStale (new art) and absent
// (removed art — clients must drop a previously-served immutable cover). foundStale
// also wrote a served state row.
// Absent counts as a visible change too: clients must drop a previously-served
// immutable cover.
if out == outcomeFound || out == outcomeFoundStale || out == outcomeAbsent {
refreshMu.Lock()
refresh = append(refresh, item)
refreshMu.Unlock()
}
// Precache only actual images. Post-outcome only: the queue row was already settled
// by process, so warming the resize cache here can never block or alter queue ops.
// Post-outcome: the queue row is already settled, so warming the cache can't
// block or alter queue ops.
if got != nil {
w.precache(ctx, got)
}
@ -228,8 +219,8 @@ func (w *Worker) drain(ctx context.Context, concurrency int, kinds ...string) (i
return len(items), nil
}
// artworkKindToResource maps an artwork kind to the UI resource name carried in the refresh
// event; note media_file maps to "song", so this can't derive from Kind.String().
// artworkKindToResource maps a kind to its UI resource name; media_file maps to "song", so
// this can't derive from Kind.String().
var artworkKindToResource = map[model.Kind]string{
model.KindAlbumArtwork: "album",
model.KindArtistArtwork: "artist",
@ -238,8 +229,8 @@ var artworkKindToResource = map[model.Kind]string{
model.KindMediaFileArtwork: "song",
}
// broadcastRefresh emits one coalesced RefreshResource for the batch's newly-acquired
// artwork, so connected UIs re-fetch the affected records (and pick up the new coverArt id).
// broadcastRefresh emits one coalesced RefreshResource for the batch, so UIs re-fetch the
// affected records and pick up the new coverArt id.
func (w *Worker) broadcastRefresh(ctx context.Context, found []model.ArtworkQueueItem) {
if len(found) == 0 {
return
@ -270,16 +261,16 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outc
queue := w.proc.ds.ArtworkQueue(ctx)
switch out {
case outcomeFound, outcomeAbsent:
// DeleteIfUnchanged, not Delete: a scan that re-enqueued this row mid-flight reset
// its retry_at, so the row survives here and the next drain re-resolves it.
// A scan that re-enqueued this row mid-flight reset its retry_at, so the row survives
// here and the next drain re-resolves it.
if err := queue.DeleteIfUnchanged(item.ItemKind, item.ItemID, item.ImageType, item.RetryAt); err != nil {
log.Warn(ctx, "Artwork: Could not delete processed queue item", "kind", item.ItemKind, "id", item.ItemID, err)
}
case outcomeFoundStale, outcomeFailed:
retryAt := time.Now().Add(backoff(item.Attempts))
if retryAt.Before(item.EnqueuedAt.Add(giveUpAfter)) {
// MarkFailedIfUnchanged, not MarkFailed: a scan that re-enqueued this row mid-flight reset
// retry_at, so stale backoff must not stomp its fresh, immediate eligibility.
// A mid-flight re-enqueue reset retry_at; stale backoff must not stomp its
// fresh, immediate eligibility.
if err := queue.MarkFailedIfUnchanged(item.ItemKind, item.ItemID, item.ImageType, item.RetryAt, retryAt); err != nil {
log.Warn(ctx, "Artwork: Could not reschedule failed queue item", "kind", item.ItemKind, "id", item.ItemID, err)
}
@ -288,10 +279,8 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outc
"budgetLeft", time.Until(item.EnqueuedAt.Add(giveUpAfter)))
break
}
// Retry budget exhausted: stop retrying. Absent is only recoverable where a periodic
// recheck will revisit it, so kinds without one keep no row at all; and art already
// being served is kept, since exhaustion means the source stayed unreachable rather
// than that the entity lost its cover.
// Absent is only recoverable where a periodic recheck revisits it, so other kinds keep
// no row; art already being served is kept, as exhaustion means unreachable, not removed.
settled := "kept previous state"
if out == outcomeFailed && hasRecheckPath(item.ItemKind) && !w.hasResolvedArtwork(ctx, item) {
writeAbsent(ctx, w.proc.ds.Artwork(ctx), item)
@ -306,7 +295,6 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outc
return out, got
}
// hasResolvedArtwork reports whether the item already has a hash-bearing state row.
func (w *Worker) hasResolvedArtwork(ctx context.Context, item model.ArtworkQueueItem) bool {
kind, ok := model.ParseKind(item.ItemKind)
if !ok {
@ -317,15 +305,14 @@ func (w *Worker) hasResolvedArtwork(ctx context.Context, item model.ArtworkQueue
}
// precache warms the resize cache at the UI cover size from the bytes just acquired, so the
// first UI request is a cache hit without re-reading the rows or the file. Skipped when
// disabled; failures are debug-only.
// first UI request hits without re-reading the rows or the file.
func (w *Worker) precache(ctx context.Context, got *acquired) {
if !conf.Server.EnableArtworkPrecache || w.cache == nil || w.cache.Disabled(ctx) {
return
}
precacheStart := time.Now()
// Same key as the serving path (hash/size/square); only the source of the bytes differs.
// square matches what the list surfaces request, otherwise this warms a key nothing reads.
// Same key as the serving path: square must match what the list surfaces request, or this
// warms a key nothing reads.
item := &resizedItem{
hash: got.ia.Hash,
size: conf.Server.UICoverArtSize,

View File

@ -17,13 +17,10 @@ import (
. "github.com/onsi/gomega"
)
// soakCycles is deliberately >2000: this is a leak regression guard, not a
// performance benchmark, so it favors a stable signal over raw speed.
// Deliberately >2000: a leak guard favors a stable signal over speed.
const soakCycles = 2200
var _ = Describe("Worker soak", func() {
// Runs acquisition over many cycles across a mix of sources, asserting
// goroutines/heap plateau instead of growing unbounded (a leak guard). Skipped under -short.
It("does not leak goroutines, heap, or fds over many acquisition cycles", func() {
if testing.Short() {
Skip("skipping soak test in short mode")
@ -57,8 +54,7 @@ var _ = Describe("Worker soak", func() {
proc := &processor{ds: ds, store: store, resolver: newResolver(ds, ag, ffm, nil)}
conf.Server.CoverArtPriority = "cover.jpg, embedded"
// Dangling refs (al/ra ids the repos don't know about) mirror an entity
// deleted after being enqueued; ds.Radio auto-provisions an empty mock repo.
// Dangling refs mirror an entity deleted after being enqueued.
items := []model.ArtworkQueueItem{
{ItemKind: "al", ItemID: "al-folder"},
{ItemKind: "al", ItemID: "al-embed"},
@ -78,8 +74,7 @@ var _ = Describe("Worker soak", func() {
}
settleGoroutines := func() int {
// Background goroutines (GC workers, etc.) can take a moment to wind down;
// poll for two consecutive equal samples instead of trusting a single one.
// Background goroutines wind down late, so poll for two consecutive equal samples.
prev := -1
for range 100 {
runtime.GC()
@ -102,8 +97,7 @@ var _ = Describe("Worker soak", func() {
it := items[i%len(items)]
out, _ := proc.acquire(context.Background(), it)
// "Serve-adjacent" read-back: exercise the Phase 2 surfaces a caller would
// use after acquisition, not the old serving pipeline.
// Read-back exercises the surfaces a caller would use after acquisition.
if out == outcomeFound {
kind, _ := model.ParseKind(it.ItemKind)
ia, err := artRepo.GetItemArtwork(kind, it.ItemID, model.ImageTypePrimary)

View File

@ -22,8 +22,6 @@ import (
"go.uber.org/goleak"
)
// recordingCache captures the keys passed to Get so precache warming can be asserted,
// and can be forced Disabled to exercise the skip path.
type recordingCache struct {
cache.FileCache
mu sync.Mutex
@ -48,8 +46,8 @@ func (c *recordingCache) getKeys() []string {
return append([]string(nil), c.keys...)
}
// reenqueueOnDequeue simulates a concurrent scan Enqueue between DequeueBatch and the
// worker's delete by bumping retry_at, so a DeleteIfUnchanged on the dequeued value no-ops.
// Simulates a concurrent Enqueue between DequeueBatch and the worker's delete, so
// DeleteIfUnchanged on the dequeued value no-ops.
type reenqueueOnDequeue struct {
*tests.MockArtworkQueueRepo
done bool
@ -303,8 +301,7 @@ var _ = Describe("Worker", func() {
Expect(err).ToNot(HaveOccurred())
Expect(n).To(Equal(1))
// The concurrent re-enqueue reset retry_at; the failure path must not stomp it
// with stale backoff nor bump attempts, so the row stays immediately eligible.
// The re-enqueue reset retry_at; the failure path must not stomp it nor bump attempts.
it := findQueued(queueRepo, "al", "al8")
Expect(it).ToNot(BeNil())
Expect(it.Attempts).To(BeZero())
@ -317,7 +314,7 @@ var _ = Describe("Worker", func() {
imageAgents(&fakeImageAgent{name: "failAgent", err: errors.New("agent timed out")})
w = NewWorker(ds, store, ag, ffm, broker, imgCache)
Expect(queueRepo.Enqueue(model.ArtworkQueueItem{ItemKind: "al", ItemID: "al9"})).To(Succeed())
// Age the row past the budget so the next retry would land beyond enqueued_at+giveUpAfter.
// Age the row past the retry budget.
for k, v := range queueRepo.Data {
if v.ItemID == "al9" {
v.EnqueuedAt = time.Now().Add(-(giveUpAfter + time.Hour))
@ -329,7 +326,6 @@ var _ = Describe("Worker", func() {
Expect(err).ToNot(HaveOccurred())
Expect(n).To(Equal(1))
// Row removed (stops retrying) and the failure settles absent for the periodic sweep.
Expect(findQueued(queueRepo, "al", "al9")).To(BeNil())
ia, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "al9", model.ImageTypePrimary)
Expect(err).ToNot(HaveOccurred())
@ -363,8 +359,8 @@ var _ = Describe("Worker", func() {
Expect(ia.Hash).To(Equal("cafebabe"), "a persistent outage must not discard served art")
})
// Media files are excluded from recheckKinds, so an absent row written here would never
// be revisited: a transient read error would look like "this track has no cover" forever.
// Media files are excluded from recheckKinds, so an absent row here would never be
// revisited: a transient read error would look permanent.
It("does not settle absent on exhaustion for a kind with no recheck path", func() {
conf.Server.EnableMediaFileCoverArt = true
ds.MockedMediaFile = tests.CreateMockMediaFileRepo()
@ -403,7 +399,6 @@ var _ = Describe("Worker", func() {
Expect(err).ToNot(HaveOccurred())
Expect(n).To(Equal(1))
// Resolved as absent (no art) and removed — not stuck failing on ErrNotFound forever.
Expect(findQueued(queueRepo, "pl", "plPriv")).To(BeNil())
ia, err := artRepo.GetItemArtwork(model.KindPlaylistArtwork, "plPriv", model.ImageTypePrimary)
Expect(err).ToNot(HaveOccurred())
@ -526,9 +521,8 @@ var _ = Describe("Worker", func() {
})
It("does not open the breaker on a run of agent not-found misses", func() {
// Regression: agents.ErrNotFound is a definitive miss, not a fault. A run of
// artless items must never trip the breaker, or they'd loop in retry instead of
// settling absent. Uses the real gate, not passthroughGate.
// agents.ErrNotFound is a definitive miss, not a fault: artless items must not
// trip the breaker, or they would loop in retry instead of settling absent.
notFound := func() (io.ReadCloser, string, error) { return nil, "", agents.ErrNotFound }
for range breakerThreshold + 3 {
_, _, err := w.gate("A", notFound)
@ -601,8 +595,7 @@ var _ = Describe("Worker", func() {
Expect(imgCache.getKeys()).To(BeEmpty())
})
// It warms from the bytes the acquisition already held, so no state row or store file
// needs to exist for it to work.
// It warms from the bytes acquisition already held, so no state row or store file is needed.
It("warms from the acquired bytes without reading them back", func() {
conf.Server.EnableArtworkPrecache = true
ia := &model.ItemArtwork{
@ -615,9 +608,8 @@ var _ = Describe("Worker", func() {
w.precache(ctx, &acquired{ia: ia, mime: "image/jpeg", data: data})
// Nothing backs that hash on disk or in the store, so the entry can only have come
// from the bytes handed in. Probing with a source that refuses to open proves it
// is really cached rather than re-read on demand.
// Nothing backs that hash on disk, so a hit can only come from the bytes handed in;
// the probe refuses to open, proving nothing is re-read.
probe := &resizedItem{
hash: ia.Hash, size: 300, square: true, ffmpeg: ffm,
open: func() (io.ReadCloser, error) { return nil, errors.New("precache must not re-read the source") },
@ -642,9 +634,8 @@ var _ = Describe("Worker", func() {
Expect(pooled).To(HaveLen(len(artworkKindToResource)), "a kind is claimed by more than one pool")
})
// A first backfill enqueues artists before albums, and artists resolve through a
// rate-limited agent that holds its slot while waiting. Sharing one pool let ~29k
// sleeping lookups sit in front of every album for hours.
// Artists resolve through a rate-limited agent that holds its slot while waiting, so one
// shared pool would park every album behind them.
It("resolves albums while artists are stuck on a slow agent", func() {
conf.Server.CoverArtPriority = "cover.jpg"
conf.Server.ArtistArtPriority = "external"
@ -677,15 +668,13 @@ var _ = Describe("Worker", func() {
runCtx, cancel := context.WithCancel(ctx)
done := make(chan struct{})
go func() { defer close(done); _ = w.Run(runCtx) }()
// Wait for Run to return: a leaked pool goroutine outlives the spec and races the
// config snapshot Ginkgo restores on cleanup.
// Join Run: a leaked pool goroutine would race the config snapshot Ginkgo restores.
DeferCleanup(func() {
cancel()
close(block) // unpark the blocked lookups so the pools can unwind
<-done
})
// The album must land while every artist is still parked in the agent.
Eventually(func() bool {
ia, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "alx", model.ImageTypePrimary)
return err == nil && ia.Hash != ""
@ -698,8 +687,6 @@ var _ = Describe("Worker", func() {
})
Describe("batching", func() {
// A cancelled drain leaves its undispatched rows untouched in the queue, so a later
// drain picks them up unchanged.
It("leaves undispatched items queued when cancelled mid-batch", func() {
for i := range 8 {
id := fmt.Sprintf("alc%d", i)
@ -720,8 +707,6 @@ var _ = Describe("Worker", func() {
}
})
// The pool is fed from one dequeue per pass: a batch sized to the pool would make a
// single slow item idle the other slots for as long as it runs.
It("dequeues past the worker pool so one drain covers many items", func() {
for i := range 16 {
ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{{ID: fmt.Sprintf("alb%d", i), Name: "Album"}})

View File

@ -41,8 +41,6 @@ func TestArtworkBreakerHalfOpen(t *testing.T) {
})
}
// Drives the worker's per-name gate map with the fake clock: one agent's open breaker
// must neither block another agent nor short-circuit the other's probe recovery.
func TestArtworkGatePerAgentBreakerIsolation(t *testing.T) {
synctest.Test(t, func(t *testing.T) {
g := NewWithT(t)

View File

@ -15,20 +15,17 @@ type Artwork struct {
const ImageTypePrimary = "primary"
// ItemImage is per-entity artwork state hydrated at query time; never persisted
// (structs:"-" keeps it out of upserts).
// ItemImage is per-entity artwork state hydrated at query time; never persisted.
type ItemImage struct {
ImageHash string `structs:"-" json:"imageHash,omitempty"`
ImageAbsent bool `structs:"-" json:"imageAbsent,omitempty"`
BlurHash string `structs:"-" json:"blurHash,omitempty"`
// Dimensions of the original image. A blurhash carries no aspect ratio, so a client
// needs these to decode the placeholder into the shape the real image will occupy.
// A blurhash carries no aspect ratio, so clients need these to shape the placeholder.
ImageWidth int `structs:"-" json:"imageWidth,omitempty"`
ImageHeight int `structs:"-" json:"imageHeight,omitempty"`
}
// AspectRatio is the image's width/height, or nil when there is no image or its dimensions are
// unknown (an unresolved item). Never guesses: a wrong ratio mis-shapes a client's placeholder.
// AspectRatio is the image's width/height, or nil when the image or its dimensions are unknown.
func (i ItemImage) AspectRatio() *float64 {
if i.ImageAbsent || i.ImageWidth <= 0 || i.ImageHeight <= 0 {
return nil
@ -47,8 +44,7 @@ type ItemArtwork struct {
SourcePath string `structs:"source_path"`
// RefMtime is SourcePath's mtime (unix-nanoseconds) 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.
// Nullable in the schema, but every insert must set them: these non-pointer fields cannot scan NULL.
AttemptedAt time.Time `structs:"attempted_at"`
UpdatedAt time.Time `structs:"updated_at"`
}
@ -65,8 +61,7 @@ type ItemArtworkInfo struct {
// Absent reports a known-absent artwork state (resolved, no image).
func (i ItemArtworkInfo) Absent() bool { return i.Hash == "" }
// Image projects the hydration entry onto the entity-facing struct, so every hydration
// site copies the same set of fields.
// Image projects the hydration entry onto the entity-facing struct.
func (i ItemArtworkInfo) Image() ItemImage {
return ItemImage{
ImageHash: i.Hash,
@ -96,50 +91,43 @@ const (
)
type ArtworkRepository interface {
// Image identity (artwork table)
GetImage(hash string) (*Artwork, error)
PutImage(a *Artwork) error
GetImages(hashes []string) (map[string]Artwork, error)
// GetOrphanHashes returns hashes referenced by no item_artwork row and older than cutoff.
GetOrphanHashes(createdBefore time.Time) ([]string, error)
// DeleteOrphans deletes the given hashes only if still unreferenced and older than cutoff (atomic re-check).
// DeleteOrphans deletes the given hashes only if still unreferenced and older than cutoff.
DeleteOrphans(createdBefore time.Time, hashes []string) error
// Per-item state (item_artwork table)
GetItemArtwork(kind Kind, id, imageType string) (*ItemArtwork, error)
PutItemArtwork(ia *ItemArtwork) error
DeleteForItem(kind Kind, id string) error
// DeleteForItems removes state rows for the given ids of one kind, in chunks.
DeleteForItems(kind Kind, ids []string) error
// GetInfoForItems hydrates a page: one batched query, item_artwork joined to artwork.
// GetInfoForItems hydrates a page in one batched query.
GetInfoForItems(kind Kind, ids []string) (map[string]ItemArtworkInfo, error)
// GetAllMimes returns hash -> current mime for every stored artwork, for sweep retention checks.
// GetAllMimes returns hash -> current mime for every stored artwork.
GetAllMimes() (map[string]string, error)
// PurgeDanglingItemArtwork removes state rows whose entity no longer exists.
PurgeDanglingItemArtwork() (int64, error)
}
type ArtworkQueueRepository interface {
// Enqueue upserts; an existing row keeps the higher of the two priorities and has its
// retry_at reset (a detected change wants immediate re-resolution).
// Enqueue upserts; an existing row keeps the higher priority and has its retry_at reset.
Enqueue(items ...ArtworkQueueItem) error
// EnqueueBump upserts like Enqueue but preserves an existing row's retry_at, so a
// request-triggered read-through never resets a failed resolution's backoff.
EnqueueBump(items ...ArtworkQueueItem) error
// DequeueBatch returns up to n items with retry_at <= now, priority desc, enqueued_at asc.
// Restricted to the given item kinds when any are passed, so a drain pool sees only its own
// work and cannot be held up behind another kind's backlog.
// Restricted to the given kinds when any are passed, so one kind cannot block another's drain.
DequeueBatch(n int, kinds ...string) ([]ArtworkQueueItem, error)
// MarkFailedIfUnchanged applies the failure backoff only while retry_at still matches
// seenRetryAt; a concurrent re-enqueue (which resets retry_at) keeps its fresh eligibility.
// seenRetryAt, so a concurrent re-enqueue keeps its fresh eligibility.
MarkFailedIfUnchanged(kind, id, imageType string, seenRetryAt, retryAt time.Time) error
// DeleteIfUnchanged deletes the row only if its retry_at still matches retryAt, so a
// concurrent re-enqueue (which resets retry_at) survives instead of being erased.
// DeleteIfUnchanged deletes only while retry_at still matches, sparing a concurrent re-enqueue.
DeleteIfUnchanged(kind, id, imageType string, retryAt time.Time) error
Count() (int64, error)
// EnqueueStaleAbsent inserts queue rows (priority Recheck) for absent states older than cutoff.
EnqueueStaleAbsent(kind Kind, attemptedBefore time.Time) (int64, error)
// EnqueueMissing inserts queue rows (priority Recheck) for entities of the kind that have no
// item_artwork row at all, so a never-processed entity is eventually resolved even without a scan.
// EnqueueMissing inserts queue rows (priority Recheck) for entities with no item_artwork row.
EnqueueMissing(kind Kind) (int64, error)
// PurgeDangling removes queue rows whose entity no longer exists.
PurgeDangling() (int64, error)

View File

@ -23,8 +23,7 @@ var _ = Describe("ItemImage JSON", func() {
Expect(out).To(HaveKeyWithValue("blurHash", "LEHV6nWB2yk8"))
})
// Clients decode the blurhash into a bitmap of their choosing, so without the dimensions they
// cannot know the placeholder's shape and default to a square.
// Without the dimensions, clients cannot know the placeholder's shape and default to a square.
It("exposes the image dimensions alongside the blurhash", func() {
al := model.Album{ID: "al-3", Name: "Album"}
al.BlurHash = "LEHV6nWB2yk8"

View File

@ -202,8 +202,7 @@ func (r *albumRepository) CountAll(options ...model.QueryOptions) (int64, error)
}
func (r *albumRepository) Exists(id string) (bool, error) {
// Filtered like CountAll: the plain exists() helper applies no library filter, so it
// would report a row in a library the caller cannot see.
// The exists() helper applies no library filter, so it would report rows the caller cannot see.
c, err := r.count(r.applyLibraryFilter(r.newSelect().Where(Eq{"album.id": id})))
return c > 0, err
}
@ -259,7 +258,6 @@ func (r *albumRepository) GetAll(options ...model.QueryOptions) (model.Albums, e
return albums, nil
}
// hydrateArtwork fills each album's ImageHash/ImageAbsent from one batched item_artwork lookup.
func (r *albumRepository) hydrateArtwork(albums model.Albums) {
if len(albums) == 0 {
return
@ -271,9 +269,7 @@ func (r *albumRepository) hydrateArtwork(albums model.Albums) {
}
}
// GetAllIDs returns just the album IDs for the same row set as GetAll, skipping the
// heavy column projection and JSON post-processing. Used by bulk enumeration (artwork backfill)
// and as GetCursor's id pre-pass.
// GetAllIDs returns the IDs of GetAll's row set, skipping its column projection and JSON decoding.
func (r *albumRepository) GetAllIDs(options ...model.QueryOptions) ([]string, error) {
sq := r.applyLibraryFilter(r.newSelect(options...).Columns("album.id"))
if filtersNeedAnnotation(sq) {

View File

@ -993,8 +993,7 @@ var _ = Describe("AlbumRepository", func() {
})
})
// Exists used the unfiltered helper, so it reported albums in libraries the caller
// cannot see -- the same leak Get/GetAll/CountAll already guard against.
// Exists must apply the same library filter as Get/GetAll/CountAll.
Describe("Exists library visibility", func() {
It("hides an album the user has no library access to", func() {
Expect(albumRepo.Put(&model.Album{ID: "vis-album", Name: "Vis", LibraryID: 1})).To(Succeed())

View File

@ -266,8 +266,7 @@ func (r *artistRepository) GetAll(options ...model.QueryOptions) (model.Artists,
}
// GetAllIDs returns just the artist IDs for the same row set as GetAll, skipping the
// heavy stats columns and JSON post-processing. Used by bulk enumeration (artwork backfill)
// and as GetCursor's id pre-pass.
// heavy stats columns and JSON post-processing.
func (r *artistRepository) GetAllIDs(options ...model.QueryOptions) ([]string, error) {
sq := r.applyLibraryFilterToArtistQuery(r.newSelect(options...).Columns("artist.id")).GroupBy("artist.id")
if filtersNeedAnnotation(sq) {

View File

@ -12,12 +12,12 @@ import (
"github.com/pocketbase/dbx"
)
// artworkChunkSize bounds each chunk fetch's id IN-list (under SQLite's bound-parameter limit); a
// whole multiple of artworkBatchSize so a page re-chunks into even hydration batches.
// Keeps the id IN-list under SQLite's bound-parameter limit; a whole multiple of artworkBatchSize
// so a page re-chunks into even hydration batches.
const artworkChunkSize = artworkBatchSize * 3
// streamByIDs yields the rows of ids in chunks, fetching each chunk through the caller's hydrating
// fetch. Resolving ids first keeps OFFSET out of the joined query (spec §6).
// streamByIDs yields rows in id chunks through the caller's hydrating fetch. Resolving ids first
// keeps OFFSET out of the joined query.
func streamByIDs[S ~[]T, T any](ids []string, fetch func(chunk []string) (S, error)) iter.Seq2[T, error] {
return func(yield func(T, error) bool) {
for chunk := range slices.Chunk(ids, artworkChunkSize) {
@ -36,8 +36,8 @@ func streamByIDs[S ~[]T, T any](ids []string, fetch func(chunk []string) (S, err
}
}
// chunkOptions narrows the caller's options to a chunk of ids, dropping Max/Offset (the id pre-pass
// already consumed them) and reusing its Sort, which may be a seeded random expression by now.
// chunkOptions narrows options to a chunk of ids, dropping Max/Offset (the id pre-pass already
// consumed them) and reusing Sort, which may be a seeded random expression by now.
func chunkOptions(options []model.QueryOptions, idField string) func([]string) model.QueryOptions {
var base model.QueryOptions
if len(options) > 0 {
@ -53,8 +53,8 @@ func chunkOptions(options []model.QueryOptions, idField string) func([]string) m
}
}
// hydrateItemImages returns per-item artwork info for a fetched page via one batched query per kind
// (never a join, see spec §6). On error it logs and returns an empty map so the page still renders.
// hydrateItemImages returns per-item artwork info in one batched query per kind. On error it logs
// and returns an empty map, so the page still renders.
func hydrateItemImages(ctx context.Context, db dbx.Builder, kind model.Kind, ids []string) map[string]model.ItemArtworkInfo {
if len(ids) == 0 {
return map[string]model.ItemArtworkInfo{}
@ -75,7 +75,7 @@ func applyItemImage(infos map[string]model.ItemArtworkInfo, id string, img *mode
}
// hydrateMediaFileArtwork mirrors MediaFile.CoverArtID: an embedded-eligible file with resolved own
// art uses it, else it falls back to the album's. Two batched item_artwork lookups, never a join.
// art uses it, else it falls back to the album's.
func hydrateMediaFileArtwork(ctx context.Context, db dbx.Builder, mfs model.MediaFiles) {
if len(mfs) == 0 {
return
@ -96,26 +96,20 @@ func hydrateMediaFileArtwork(ctx context.Context, db dbx.Builder, mfs model.Medi
eligible := mf.HasCoverArt && conf.Server.EnableMediaFileCoverArt
ownInfo, ownResolved := mfInfos[mf.ID]
if eligible && ownResolved && !ownInfo.Absent() {
mf.ItemImage = ownInfo.Image() // own resolved art wins
mf.ItemImage = ownInfo.Image()
continue
}
ownWontResolve := !eligible || (ownResolved && ownInfo.Absent())
// Fallback (see MediaFile.CoverArtID): inherit a found album hash for optimistic caching,
// but only when the album's bytes are what serving will actually return. A multi-disc track
// emits a dc- id served from disc art, and an eligible-but-unresolved track still extracts
// its own embedded image — stamping the album hash on either advertises a content-version
// (and a blurhash) belonging to a different image.
// Inherit the album hash only when serving returns those exact bytes: a multi-disc track is
// served disc art, and an eligible-but-unresolved one still extracts its own embedded image.
if album, ok := albumInfos[mf.AlbumID]; ok && !album.Absent() {
if mf.DiscNumber == 0 && ownWontResolve {
mf.ItemImage = album.Image()
}
continue
}
// Nothing found. Mark absent only when serving would definitively yield a placeholder:
// a single-disc track whose album is known-absent and whose own art won't resolve. A
// multi-disc track resolves disc art provisionally (never known-absent), and an
// eligible-but-unresolved track can still extract its own embedded art — both stay
// requestable.
// Mark absent only when serving would definitively yield a placeholder; disc art and
// still-extractable embedded art both keep a track requestable.
if mf.DiscNumber > 0 {
continue
}
@ -125,8 +119,7 @@ func hydrateMediaFileArtwork(ctx context.Context, db dbx.Builder, mfs model.Medi
}
}
// hydrateCursor buffers a streamed page into batches and hydrates each before yielding, so a
// cursor carries the same artwork state as a fetched page without a per-row query.
// hydrateCursor hydrates a streamed cursor in batches, avoiding a per-row query.
func hydrateCursor[T any](cursor iter.Seq2[T, error], hydrate func([]T)) iter.Seq2[T, error] {
return func(yield func(T, error) bool) {
buf := make([]T, 0, artworkBatchSize)
@ -157,8 +150,7 @@ func hydrateCursor[T any](cursor iter.Seq2[T, error], hydrate func([]T)) iter.Se
}
}
// hydratePlaylistTrackArtwork hydrates the MediaFile embedded in each playlist track, so a track
// reached through a playlist carries the same artwork state as one reached through the songs list.
// hydratePlaylistTrackArtwork hydrates the MediaFile embedded in each playlist track.
func hydratePlaylistTrackArtwork(ctx context.Context, db dbx.Builder, tracks model.PlaylistTracks) {
if len(tracks) == 0 {
return

View File

@ -18,8 +18,7 @@ import (
"github.com/pocketbase/dbx"
)
// seedAnnotations gives each id a distinct rating, play count, play date and starred_at for the
// current user, dropping the rows again when the spec ends. It writes them directly, so that
// seedAnnotations gives each id distinct annotation values, writing them directly so that
// SetRating's average_rating update does not outlive the cleanup.
func seedAnnotations(itemType string, ids ...string) {
GinkgoHelper()
@ -103,7 +102,6 @@ var _ = Describe("Artwork hydration", func() {
al.ImageAbsent = true
Expect(repo.(*albumRepository).Put(&al)).To(Succeed())
// No item_artwork rows exist, so a fresh read must observe zero values.
got, err := repo.Get(al.ID)
Expect(err).ToNot(HaveOccurred())
Expect(got.ImageHash).To(BeEmpty())
@ -179,8 +177,6 @@ var _ = Describe("Artwork hydration", func() {
Expect(got.ImageHash).To(Equal("plget8888888888"))
})
// A track reached through a playlist must carry the same artwork state as one reached
// through the songs list, or its cover id has no hash to serve immutably or blur.
It("hydrates the tracks reached through a playlist", func() {
Expect(aw.PutImage(&model.Artwork{Hash: "pltrackhash1234", Mime: "image/jpeg", BlurHash: "LPLBLURhash"})).To(Succeed())
putInfo("al", songDayInALife.AlbumID, "pltrackhash1234")
@ -279,16 +275,13 @@ var _ = Describe("Artwork hydration", func() {
byID := getByID()
// eligible + own hash -> own hash
Expect(byID["1001"].ImageHash).To(Equal("mfh1001xxxxxxxx"))
Expect(byID["1001"].ImageAbsent).To(BeFalse())
// eligible + embedded absent -> falls through to album 102 info
Expect(byID["1002"].ImageHash).To(Equal("alh102xxxxxxxxxx"))
Expect(byID["1002"].ImageAbsent).To(BeFalse())
// not eligible (no embedded cover) -> album 103 info (known-absent)
Expect(byID["1003"].ImageHash).To(BeEmpty())
Expect(byID["1003"].ImageAbsent).To(BeTrue())
// not eligible, album has no row -> zero values (unresolved)
// 2002: not eligible, and its album has no row at all -> unresolved
Expect(byID["2002"].ImageHash).To(BeEmpty())
Expect(byID["2002"].ImageAbsent).To(BeFalse())
})
@ -304,15 +297,12 @@ var _ = Describe("Artwork hydration", func() {
byID := getByID()
// own-art-wins: ImageHash is the track's own, but AlbumImage still carries the album's.
Expect(byID["1001"].ImageHash).To(Equal("mfh1001albimgxxx"))
Expect(byID["1001"].AlbumImage.ImageHash).To(Equal("alh101albimgxxxx"))
// single-disc album-inheritance: not eligible, so ImageHash mirrors the album's hash.
Expect(byID["1002"].ImageHash).To(Equal("alh102albimgxxxx"))
Expect(byID["1002"].AlbumImage.ImageHash).To(Equal("alh102albimgxxxx"))
// multi-disc bailout: ImageHash stays bare, but AlbumImage still reflects the absence.
Expect(byID["2002"].ImageHash).To(BeEmpty())
Expect(byID["2002"].ImageAbsent).To(BeFalse())
Expect(byID["2002"].AlbumImage.ImageAbsent).To(BeTrue())
@ -341,18 +331,16 @@ var _ = Describe("Artwork hydration", func() {
})
It("keeps an eligible file optimistic when its own art is unresolved, even if the album is absent", func() {
// 1004 is eligible (has embedded cover) with no mf state row yet; its album (103) is absent.
setCover("1004", true)
DeferCleanup(func() { setCover("1004", false) })
putInfo("al", "103", "") // album known-absent
byID := getByID()
// The track's own embedded art is still unresolved, so it must NOT inherit the album's
// absence: coverArt stays requestable so serving can extract the embedded art.
// 1004's own embedded art is still unresolved, so coverArt must stay requestable.
Expect(byID["1004"].ImageAbsent).To(BeFalse())
Expect(byID["1004"].ImageHash).To(BeEmpty())
// A non-eligible sibling on the same absent album still inherits the absence.
// 1003 is not eligible, so it still inherits the album's absence.
Expect(byID["1003"].ImageAbsent).To(BeTrue())
})
@ -361,8 +349,6 @@ var _ = Describe("Artwork hydration", func() {
byID := getByID()
// 2002 is multi-disc (DiscNumber>0); CoverArtID emits a dc- id served from disc art of
// unknown identity, so it must not advertise the album's hash as its content-version.
Expect(byID["2002"].ImageHash).To(BeEmpty())
Expect(byID["2002"].ImageAbsent).To(BeFalse())
})
@ -372,14 +358,12 @@ var _ = Describe("Artwork hydration", func() {
byID := getByID()
// 2002 is a multi-disc track (DiscNumber>0); CoverArtID points at disc art, which
// resolves provisionally, so it must never inherit the album's absence.
Expect(byID["2002"].ImageAbsent).To(BeFalse())
Expect(byID["2002"].ImageHash).To(BeEmpty())
})
// serveMediaFile extracts this track's own embedded art (provisionalEmbedded), so the
// album's hash would advertise a content-version for bytes nobody will serve.
// serveMediaFile serves this track's own embedded art, so the album's hash would
// advertise a content-version for bytes nobody will serve.
It("leaves the hash bare for an eligible file whose own art is unresolved", func() {
setCover("1004", true)
DeferCleanup(func() { setCover("1004", false) })
@ -438,16 +422,15 @@ var _ = Describe("Artwork hydration", func() {
albumRadioactivity.ID, albumMultiDisc.ID, albumCJK.ID, albumPunctuation.ID}}
onlyArtists = squirrel.Eq{"artist.id": []string{artistKraftwerk.ID, artistBeatles.ID,
artistCJK.ID, artistPunctuation.ID}}
// Both fixture playlists belong to the same owner, so the owner_name sort would have a
// single value to order by. This one is also private and owned by a third user, which
// is what the non-admin visibility spec needs to see hidden.
// Both fixture playlists share an owner, leaving the owner_name sort a single value to
// order by; this one is also private, which the non-admin visibility spec needs.
foreign := model.Playlist{Name: "Foreign", OwnerID: thirdUser.ID, OwnerName: thirdUser.UserName}
Expect(playlistRepo.Put(&foreign)).To(Succeed())
DeferCleanup(func() { Expect(playlistRepo.Delete(foreign.ID)).To(Succeed()) })
onlyPlaylists = squirrel.Eq{"playlist.id": []string{plsBest.ID, plsCool.ID, foreign.ID}}
// The suite annotates a single album and artist, so the annotation-backed sorts would
// have nothing to order. Seed distinct values, and drop them again after each spec.
// The suite annotates a single album and artist, leaving the annotation-backed sorts
// nothing to order.
seedAnnotations("album", albumSgtPeppers.ID, albumAbbeyRoad.ID)
seedAnnotations("artist", artistKraftwerk.ID, artistCJK.ID)
@ -515,8 +498,7 @@ var _ = Describe("Artwork hydration", func() {
To(Equal(slice.Map(want, func(a model.Album) string { return a.ID })))
})
// The sorts and filters the Jellyfin list endpoints issue: the id pre-pass must resolve each
// of them exactly like the full query. Comparing sort keys, not ids, keeps ties out of it.
// The sorts the Jellyfin list endpoints issue; comparing sort keys, not ids, keeps ties out.
DescribeTable("orders albums like GetAll",
func(opts model.QueryOptions, key func(model.Album) string) {
opts = scoped(opts, onlyAlbums)
@ -612,8 +594,7 @@ var _ = Describe("Artwork hydration", func() {
repo := NewPlaylistRepository(otherCtx, GetDBXBuilder())
opts := model.QueryOptions{Sort: "name", Filters: onlyPlaylists}
// Both phases must filter on their own: the id pre-pass, and the chunk fetch that would
// otherwise re-read an id whose visibility changed in between.
// Both phases must filter on their own: the id pre-pass and the chunk fetch.
Expect(repo.GetAllIDs(opts)).To(ConsistOf(plsBest.ID))
all, err := repo.GetAll(model.QueryOptions{Filters: onlyPlaylists})
Expect(err).ToNot(HaveOccurred())
@ -633,15 +614,13 @@ var _ = Describe("Artwork hydration", func() {
BeforeEach(func() {
mfRepo = NewMediaFileRepository(ctx, GetDBXBuilder())
putInfo("al", albumSgtPeppers.ID, "curhash11111111")
// Distinct titles keep the ordering/paging specs tie-free; other fixture songs share
// titles (e.g. "Antenna") or albums, which would make row order ambiguous.
// Distinct titles only: other fixture songs share titles (e.g. "Antenna" x3), which
// would make the positional comparisons against GetAll pass by tie-order coincidence.
onlySongs = squirrel.Eq{"media_file.id": []string{songDayInALife.ID, songComeTogether.ID,
songRadioactivity.ID, songAntenna.ID, songDisc1Track01.ID, songCJK.ID, songPunctuation.ID}}
})
It("hydrates artwork onto every streamed track, unlike GetCursor", func() {
// Scoped to onlySongs (distinct titles): the full fixture has title ties (e.g. "Antenna"
// x3), so positional comparison against GetAll would only pass by tie-order coincidence.
opts := model.QueryOptions{Sort: "title", Filters: onlySongs}
want, err := mfRepo.GetAll(opts)
Expect(err).ToNot(HaveOccurred())

View File

@ -11,7 +11,7 @@ import (
"github.com/pocketbase/dbx"
)
// enqueueChunkSize keeps each multi-row insert under SQLite's bind-variable limit (7 cols -> 700 vars).
// Keeps each multi-row insert under SQLite's bind-variable limit (7 cols -> 700 vars).
const enqueueChunkSize = 100
type artworkQueueRepository struct {
@ -26,16 +26,13 @@ func NewArtworkQueueRepository(ctx context.Context, db dbx.Builder) model.Artwor
return r
}
// Enqueue also restarts the retry budget the worker measures from enqueued_at, so a fresh
// request never inherits an old row's spent window and give up on its first attempt.
// Enqueue also resets enqueued_at, so a fresh request does not inherit an old row's spent retry budget.
func (r *artworkQueueRepository) Enqueue(items ...model.ArtworkQueueItem) error {
return r.enqueue(`ON CONFLICT (item_kind, item_id, image_type) DO UPDATE SET
priority = MAX(priority, excluded.priority), retry_at = excluded.retry_at,
attempts = 0, enqueued_at = excluded.enqueued_at`, items)
}
// EnqueueBump raises priority like Enqueue but leaves an existing row's retry_at intact, so a
// request-triggered read-through never resets a failed resolution's backoff. New rows insert eligible.
func (r *artworkQueueRepository) EnqueueBump(items ...model.ArtworkQueueItem) error {
return r.enqueue(`ON CONFLICT (item_kind, item_id, image_type) DO UPDATE SET
priority = MAX(priority, excluded.priority)`, items)
@ -72,8 +69,6 @@ func (r *artworkQueueRepository) DequeueBatch(n int, kinds ...string) ([]model.A
return res, err
}
// MarkFailedIfUnchanged applies the backoff only while retry_at still equals seenRetryAt;
// a concurrent Enqueue resets retry_at, so its fresh eligibility survives untouched.
func (r *artworkQueueRepository) MarkFailedIfUnchanged(kind, id, imageType string, seenRetryAt, retryAt time.Time) error {
upd := Update(r.tableName).
Set("attempts", Expr("attempts + 1")).
@ -83,13 +78,10 @@ func (r *artworkQueueRepository) MarkFailedIfUnchanged(kind, id, imageType strin
return err
}
// DeleteIfUnchanged deletes the row only while its retry_at still equals the dequeued
// value; a concurrent Enqueue resets retry_at, so the row survives to be re-resolved.
func (r *artworkQueueRepository) DeleteIfUnchanged(kind, id, imageType string, retryAt time.Time) error {
return r.delete(Eq{"item_kind": kind, "item_id": id, "image_type": imageType, "retry_at": retryAt})
}
// PurgeDangling removes queue rows whose entity no longer exists, per kind.
func (r *artworkQueueRepository) PurgeDangling() (int64, error) {
return purgeDangling(r.executeSQL, r.tableName)
}

View File

@ -19,8 +19,8 @@ var _ = Describe("ArtworkQueueRepository", func() {
ImageType: model.ImageTypePrimary, Priority: prio}
}
// backOff puts a row into the state a failed resolution leaves behind. Production only ever
// reaches that state through MarkFailedIfUnchanged, which needs the retry_at it dequeued.
// Writes the post-failure state directly: the production path, MarkFailedIfUnchanged, needs
// a retry_at only a dequeue can hand it.
backOff := func(kind, id string, retryAt time.Time) {
GinkgoHelper()
r := repo.(*artworkQueueRepository)
@ -63,11 +63,9 @@ var _ = Describe("ArtworkQueueRepository", func() {
It("EnqueueBump raises priority without resetting a backing-off row's retry_at", func() {
Expect(repo.Enqueue(item("al", "b1", model.ArtworkPriorityScan))).To(Succeed())
// Push retry_at into the future so the row is backing off and hidden from dequeue.
backOff("al", "b1", time.Now().Add(time.Hour))
Expect(repo.DequeueBatch(10)).To(BeEmpty())
// A request-triggered bump raises priority but must leave the backoff intact.
Expect(repo.EnqueueBump(item("al", "b1", model.ArtworkPriorityBump))).To(Succeed())
Expect(repo.DequeueBatch(10)).To(BeEmpty(), "bump must not reset retry_at")
@ -101,7 +99,7 @@ var _ = Describe("ArtworkQueueRepository", func() {
It("MarkFailedIfUnchanged applies backoff only while retry_at is unchanged", func() {
Expect(repo.Enqueue(item("al", "m1", model.ArtworkPriorityScan))).To(Succeed())
// Anchor retry_at in the past (attempts -> 1) so it can never collide with the re-enqueue's now.
// Anchor retry_at in the past so it can never collide with the re-enqueue's now.
backOff("al", "m1", time.Now().Add(-time.Hour))
got, err := repo.DequeueBatch(10)
Expect(err).ToNot(HaveOccurred())
@ -111,7 +109,6 @@ var _ = Describe("ArtworkQueueRepository", func() {
// A concurrent scan re-enqueues, resetting retry_at to now.
Expect(repo.Enqueue(item("al", "m1", model.ArtworkPriorityScan))).To(Succeed())
// Failing with the stale retry_at is a no-op: the re-enqueued row keeps its fresh state.
future := time.Now().Add(48 * time.Hour)
Expect(repo.MarkFailedIfUnchanged("al", "m1", model.ImageTypePrimary, original, future)).To(Succeed())
got, _ = repo.DequeueBatch(10)
@ -119,7 +116,6 @@ var _ = Describe("ArtworkQueueRepository", func() {
Expect(got[0].Attempts).To(BeZero(), "re-enqueue clears attempts, and the stale failure must not bump them")
current := got[0].RetryAt
// Failing with the current retry_at applies the backoff and bumps attempts.
Expect(repo.MarkFailedIfUnchanged("al", "m1", model.ImageTypePrimary, current, future)).To(Succeed())
got, _ = repo.DequeueBatch(10)
Expect(got).To(BeEmpty(), "backed-off row is hidden until the future retry_at")
@ -171,7 +167,6 @@ var _ = Describe("ArtworkQueueRepository", func() {
n, _ := repo.Count()
Expect(n).To(Equal(int64(1)))
// Deleting with the current retry_at removes it.
got, _ = repo.DequeueBatch(10)
Expect(got).To(HaveLen(1))
Expect(repo.DeleteIfUnchanged("al", "d1", model.ImageTypePrimary, got[0].RetryAt)).To(Succeed())
@ -225,8 +220,6 @@ var _ = Describe("ArtworkQueueRepository", func() {
It("enqueues entities that have no item_artwork row at all", func() {
awRepo := NewArtworkRepository(context.Background(), GetDBXBuilder())
// albumSgtPeppers has a resolved row and albumAbbeyRoad an absent row: both already
// processed, so neither should be enqueued as "missing".
Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: albumSgtPeppers.ID, ImageType: model.ImageTypePrimary, Hash: "hX", AttemptedAt: time.Now()})).To(Succeed())
Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: albumAbbeyRoad.ID, ImageType: model.ImageTypePrimary, Hash: "", AttemptedAt: time.Now()})).To(Succeed())

View File

@ -11,7 +11,6 @@ import (
"github.com/pocketbase/dbx"
)
// 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"} {

View File

@ -163,8 +163,7 @@ func (r *mediaFileRepository) CountBySuffix(options ...model.QueryOptions) (map[
}
func (r *mediaFileRepository) Exists(id string) (bool, error) {
// Filtered like CountAll: the plain exists() helper applies no library filter, so it
// would report a row in a library the caller cannot see.
// The exists() helper applies no library filter, so it would report rows the caller cannot see.
c, err := r.count(r.applyLibraryFilter(r.newSelect().Where(Eq{"media_file.id": id})))
return c > 0, err
}
@ -226,7 +225,6 @@ func (r *mediaFileRepository) GetAll(options ...model.QueryOptions) (model.Media
return mfs, nil
}
// hydrateArtwork hydrates a fetched page in place.
func (r *mediaFileRepository) hydrateArtwork(mfs model.MediaFiles) {
hydrateMediaFileArtwork(r.ctx, r.db, mfs)
}
@ -301,8 +299,7 @@ func (r *mediaFileRepository) GetCursor(options ...model.QueryOptions) (model.Me
return wrapMediaFileCursor(cursor), nil
}
// GetAllIDs returns just the media_file IDs for the same row set as GetAll, skipping the heavy
// column projection. Used as GetCursorWithArtwork's id pre-pass.
// GetAllIDs returns the IDs of GetAll's row set, skipping its wide column projection.
func (r *mediaFileRepository) GetAllIDs(options ...model.QueryOptions) ([]string, error) {
sq := r.applyLibraryFilter(r.newSelect(options...).Columns("media_file.id"))
if filtersNeedAnnotation(sq) {
@ -313,8 +310,7 @@ func (r *mediaFileRepository) GetAllIDs(options ...model.QueryOptions) ([]string
return ids, err
}
// GetCursorWithArtwork streams the same rows as GetCursor, hydrated, via the id pre-pass used by
// the other cursors, rather than the scanner's bounded, unhydrated GetCursor itself.
// GetCursorWithArtwork streams the same rows as GetCursor, hydrated, via an id pre-pass.
func (r *mediaFileRepository) GetCursorWithArtwork(options ...model.QueryOptions) (model.MediaFileCursor, error) {
ids, err := r.GetAllIDs(options...)
if err != nil {

View File

@ -1119,8 +1119,7 @@ var _ = Describe("MediaRepository", func() {
})
})
// Exists used the unfiltered helper, so it reported tracks in libraries the caller
// cannot see -- the same leak Get/GetAll/CountAll already guard against.
// Exists must apply the same library filter as Get/GetAll/CountAll.
Describe("Exists library visibility", func() {
It("hides a track the user has no library access to", func() {
restricted := model.User{ID: "restricted_mf_user", UserName: "rm", Name: "RM", Email: "rm@t.com"}

View File

@ -134,8 +134,8 @@ func (r *playlistRepository) Put(p *model.Playlist, cols ...string) error {
}
pls.ID = id // r.put assigns the generated id to p, not to this copy
if isNew {
// A brand-new playlist has art to find even with no tracks (an imported m3u can carry an
// ExternalImageURL). An update landing here changed only metadata, so leave its cover be.
// Even a trackless new playlist has art to find (an imported m3u can carry an
// ExternalImageURL); an update landing here changed only metadata, so leave its cover be.
r.enqueueCoverRebuild(id)
}
return r.refreshCounters(&pls.Playlist)
@ -184,7 +184,6 @@ func (r *playlistRepository) findBy(sql Sqlizer) (*model.Playlist, error) {
return &list[0], nil
}
// hydrateArtwork fills each playlist's ImageHash/ImageAbsent from one batched item_artwork lookup.
func (r *playlistRepository) hydrateArtwork(playlists model.Playlists) {
if len(playlists) == 0 {
return
@ -211,12 +210,10 @@ func (r *playlistRepository) GetAll(options ...model.QueryOptions) (model.Playli
return playlists, err
}
// GetAllIDs returns just the playlist IDs for the same row set as GetAll (honoring userFilter),
// skipping its per-row processing. Used by bulk enumeration (artwork backfill) and as GetCursor's
// id pre-pass.
// GetAllIDs returns the IDs of GetAll's row set, skipping its per-row processing.
func (r *playlistRepository) GetAllIDs(options ...model.QueryOptions) ([]string, error) {
// Joins a projection of user, not the table: its name/created_at columns would otherwise make
// an ORDER BY on the playlist's own ambiguous.
// Joins a projection of user, not the table: its name/created_at columns would make an ORDER BY
// on the playlist's own ambiguous.
sq := r.newSelect(options...).Columns("playlist.id", "user.user_name as owner_name").
Join("(select id, user_name from user) user on user.id = owner_id").Where(r.userFilter())
if filtersNeedAnnotation(sq) {
@ -228,8 +225,7 @@ func (r *playlistRepository) GetAllIDs(options ...model.QueryOptions) ([]string,
}
func (r *playlistRepository) GetCursor(options ...model.QueryOptions) (model.PlaylistCursor, error) {
// GetAllIDs and GetAll both apply userFilter: a cursor must not widen visibility beyond
// public/owned playlists, even if visibility changes between the two queries.
// Both passes apply userFilter, so a visibility change between them cannot widen the cursor.
ids, err := r.GetAllIDs(options...)
if err != nil {
return nil, err
@ -338,10 +334,8 @@ func (r *playlistRepository) refreshCounters(pls *model.Playlist) error {
return nil
}
// enqueueCoverRebuild re-resolves the generated 2x2 grid, which depends on the track set. Called
// only where that set actually changes: the grid samples albums at random, so rebuilding after a
// mere rename would hand the playlist a different cover. No clear -- the old cover keeps serving
// until the worker rebuilds, so there is no flicker.
// enqueueCoverRebuild re-resolves the generated 2x2 grid. Call it only when the track set changes:
// the grid samples albums at random, so rebuilding after a mere rename would change the cover.
func (r *playlistRepository) enqueueCoverRebuild(id string) {
item := model.ArtworkQueueItem{ItemKind: model.KindPlaylistArtwork.Prefix(), ItemID: id,
ImageType: model.ImageTypePrimary, Priority: model.ArtworkPriorityScan}

View File

@ -273,8 +273,7 @@ var _ = Describe("PlaylistRepository", func() {
Expect(queued).ToNot(ContainElement(HaveField("ItemID", "")), "must not enqueue an empty playlist id")
})
// The grid samples albums at random, so re-resolving after a rename would silently hand the
// playlist a different cover.
// The grid samples albums at random, so re-resolving after a rename would change the cover.
It("does not enqueue artwork when only metadata changes", func() {
ctx := request.WithUser(log.NewContext(GinkgoT().Context()), model.User{ID: "userid", UserName: "userid", IsAdmin: true})
newPls := model.Playlist{Name: "Rename Me", OwnerID: "userid"}
@ -411,9 +410,8 @@ var _ = Describe("PlaylistRepository", func() {
})
})
// Exists is ctx-sensitive through userFilter: a private playlist is invisible to anyone but
// its owner or an admin. Callers that only want "does it still exist" -- the public image
// route serving a share -- must elevate, or a shared private playlist looks deleted.
// Exists is ctx-sensitive through userFilter, so callers that only want "does it still exist"
// -- the public image route serving a share -- must elevate, or a private playlist looks gone.
Describe("Exists visibility", func() {
It("hides a private playlist from an unauthenticated context", func() {
// "userid" is the fixture user; playlist.owner_id has a FK to user(id).

View File

@ -1,6 +1,5 @@
// Package imghttp holds the shared HTTP caching contract for artwork responses, so the
// subsonic, public, and jellyfin image handlers apply identical headers without importing
// each other.
// Package imghttp holds the HTTP caching contract shared by the subsonic, public, and jellyfin
// image handlers, so they apply identical headers without importing each other.
package imghttp
import (
@ -11,33 +10,30 @@ import (
)
// WriteImageHeaders applies the artwork caching contract and reports whether a 304 was written
// (in which case the caller must not write a body). requestedHash is the hash the client asserted
// (id suffix / JWT payload / jellyfin tag param), or "" when the request carried no hash.
// (the caller must then not write a body). requestedHash is the hash the client asserted, or "".
func WriteImageHeaders(w http.ResponseWriter, r *http.Request, img *artwork.Image, requestedHash string) (wrote304 bool) {
h := w.Header()
// Placeholders are transient stand-ins for not-yet-resolved art: never cached, no validators.
// Placeholders are transient stand-ins for unresolved art: never cached, no validators.
if img.Placeholder {
h.Set("Cache-Control", "no-store")
return false
}
// The validator identifies the served representation (resized/re-encoded bytes version it via
// ETag), so a CoverArtQuality/EnableWebPEncoding change invalidates a revalidating client's
// cache. Falls back to the pixel hash for full-size originals (bytes == the hash).
// The ETag versions the served representation, so a re-encoding config change invalidates a
// revalidating client's cache; full-size originals are their own pixel hash.
etag := img.ETag
if etag == "" {
etag = img.Hash
}
// An empty validator is not one: emitting it would hand every such response the same ETag,
// and matching it would 304 a client that echoed it back even after the bytes changed.
// An empty validator would be shared by every such response, 304ing clients that echo it back.
if etag != "" {
h.Set("ETag", `"`+etag+`"`)
}
if !img.LastUpdated.IsZero() {
h.Set("Last-Modified", img.LastUpdated.UTC().Format(http.TimeFormat))
}
// Immutable only when the client asked for the exact current pixel hash; bare/legacy/mismatched
// requests get cheap ETag revalidation instead, which fixes stale art after re-resolution.
// Immutable only when the client asked for the current pixel hash; anything else revalidates,
// so art that gets re-resolved is never pinned.
if requestedHash != "" && requestedHash == img.Hash {
h.Set("Cache-Control", "public, max-age=31536000, immutable")
} else {
@ -51,8 +47,7 @@ func WriteImageHeaders(w http.ResponseWriter, r *http.Request, img *artwork.Imag
return false
}
// ifNoneMatch reports whether the If-None-Match header asserts the given hash, using weak
// comparison (RFC 9110): "*" matches any current representation and W/ prefixes are ignored.
// ifNoneMatch reports whether If-None-Match asserts hash, using RFC 9110 weak comparison.
func ifNoneMatch(header, hash string) bool {
header = strings.TrimSpace(header)
if header == "" {

View File

@ -30,8 +30,8 @@ func placeholder() *artwork.Image {
return &artwork.Image{ReadCloser: io.NopCloser(strings.NewReader("PH")), Placeholder: true}
}
// resized carries a representation ETag distinct from the pixel hash (as a resized/re-encoded
// response does), so the validator versions with the encode settings.
// A re-encoded response carries a representation ETag distinct from the pixel hash, so the
// validator versions with the encode settings.
func resized() *artwork.Image {
return &artwork.Image{
ReadCloser: io.NopCloser(strings.NewReader("IMG")),
@ -41,7 +41,6 @@ func resized() *artwork.Image {
}
}
// unvalidated stands for a response with no content hash and no representation tag.
func unvalidated() *artwork.Image {
return &artwork.Image{ReadCloser: io.NopCloser(strings.NewReader("IMG")), LastUpdated: lastMod}
}
@ -107,8 +106,8 @@ var _ = Describe("WriteImageHeaders", func() {
Entry("placeholder ignores If-None-Match and never 304s",
testCase{img: placeholder(), ifNoneMatch: "*", want304: false, wantCache: "no-store"}),
// An image with neither ETag nor Hash has no validator. Emitting one would give every
// such response the same empty tag, and matching it would 304 changed bytes.
// With no validator, an emitted ETag would be the same empty tag on every such response,
// and matching it would 304 changed bytes.
Entry("omits the ETag entirely when there is no validator",
testCase{img: unvalidated(), wantCache: "public, no-cache", wantLastMod: true}),
Entry("never 304s an empty validator echoed back by the client",

View File

@ -193,17 +193,15 @@ func SongToBaseItem(mf model.MediaFile, fields Fields) BaseItemDto {
} else if mf.Genre != "" {
item.Genres = []string{mf.Genre}
}
// A track's own cover wins in Finamp's precedence (ImageTags.Primary before AlbumId).
// Clients prefer ImageTags.Primary over AlbumId, so a track's own cover must win here.
if mf.ImageHash != "" && mf.ImageHash != mf.AlbumImage.ImageHash {
tag, blurs, ratio := primaryImage(mf.ItemImage, mf.ID, fields)
item.ImageTags = map[string]string{"Primary": tag}
item.ImageBlurHashes = blurs
item.PrimaryImageAspectRatio = ratio
} else if embeddedArtPending(mf) {
// Nothing enqueues media files, so an unresolved track only resolves when someone asks
// for its image. Advertising the id here is what makes a Jellyfin client ask; the
// request extracts the embedded art and queues the track for the worker. No blurhash:
// there is no resolved image to have one yet, and a fake would be cached forever.
// Nothing enqueues media files: advertising the id is what makes a client ask, and that
// request is what extracts the embedded art and queues the track.
item.ImageTags = map[string]string{"Primary": mf.ID}
} else if mf.AlbumID != "" {
if tag, blurs, ratio := primaryImage(mf.AlbumImage, mf.AlbumID, fields); tag != "" {
@ -215,15 +213,13 @@ func SongToBaseItem(mf model.MediaFile, fields Fields) BaseItemDto {
return item
}
// embeddedArtPending reports a track whose own art is eligible but not resolved yet, and not
// known to be absent.
func embeddedArtPending(mf model.MediaFile) bool {
return mf.HasCoverArt && conf.Server.EnableMediaFileCoverArt &&
mf.ImageHash == "" && !mf.ItemImage.ImageAbsent
}
// primaryImage derives all a mapper advertises about one image, so Primary is chosen once. It never
// fakes a blurhash: Finamp keys its cover cache on the value, pinning a stale cover forever (#5798).
// primaryImage never fakes a blurhash: clients key their cover cache on the value, which would
// pin a stale cover forever.
func primaryImage(img model.ItemImage, fallback string, fields Fields) (tag string, blurs map[string]map[string]string, ratio *float64) {
if img.ImageAbsent {
return "", nil, nil

View File

@ -290,8 +290,6 @@ var _ = Describe("mappers", func() {
Expect(string(b)).ToNot(ContainSubstring("NormalizationGain"))
})
// Real Jellyfin only attaches it when the client asks (DtoService.ContainsField), and derives
// it from the image's real dimensions.
Describe("PrimaryImageAspectRatio", func() {
nonSquare := func() model.Album {
al := model.Album{ID: "al1", Name: "Album"}
@ -457,9 +455,8 @@ var _ = Describe("mappers", func() {
Expect(after.ImageTags["Primary"]).To(Equal(before.ImageTags["Primary"]))
})
// Nothing enqueues media files, so a track's own art only resolves when something requests
// it. A Jellyfin client that is only told about the album image never asks, so the track's
// own cover would stay unreachable for Jellyfin-only users.
// Nothing enqueues media files, so a track's own art only resolves when a client requests it;
// advertising just the album image would leave that cover unreachable.
Describe("unresolved embedded art", func() {
BeforeEach(func() {
DeferCleanup(configtest.SetupConfig())

View File

@ -215,8 +215,8 @@ var _ = Describe("Playlists", func() {
To(Equal(http.StatusNotImplemented))
})
// Guards the whole chain: an upload must clear any previously-resolved artwork state, or a
// stale tag stays live under clients' blurhash-keyed cover cache until the next scan (#5798).
// An upload must clear the resolved artwork state, or clients keep serving the stale cover
// from their tag-keyed cache until the next scan.
It("clears the resolved image tag after a cover upload", func() {
plID := createPlaylist("Cover Tag", nil)
Expect(ds.Artwork(ctx).PutItemArtwork(&model.ItemArtwork{
@ -233,8 +233,7 @@ var _ = Describe("Playlists", func() {
Expect(upload(adminUser, "/Items/"+enc(plID)+"/Images/Primary", "image/jpeg", jpeg).Code).
To(Equal(http.StatusNoContent))
// The upload re-queues resolution instead of resolving inline, so the tag falls back to
// unresolved rather than carrying over the pre-upload hash.
// The upload re-queues resolution instead of resolving inline, so the tag goes bare.
Expect(imageTag()).ToNot(Equal("1111111111111111"))
})
})

View File

@ -75,8 +75,7 @@ var _ = Describe("Images", func() {
Expect(fa.recvId).To(ContainSubstring("a1"))
})
// resolveArtworkID probes the entity tables, so a deleted item cannot produce an artwork id
// at all -- which is what stops artwork state outliving its entity from being served here.
// resolveArtworkID probes the entity tables, so a deleted item yields no artwork id at all.
It("asks for no artwork once the item is deleted, rather than its lingering state", func() {
ds := &tests.MockDataStore{} // no albums/artists/tracks/playlists at all
fa := &fakeArtwork{}

View File

@ -9,7 +9,6 @@ import (
"github.com/navidrome/navidrome/model"
)
// refreshableArtworkKinds are the entity kinds a manual re-resolve accepts.
var refreshableArtworkKinds = map[model.Kind]bool{
model.KindAlbumArtwork: true,
model.KindArtistArtwork: true,
@ -22,7 +21,6 @@ func (api *Router) addArtworkRoute(r chi.Router) {
r.Post("/artwork/{kind}/{id}/refresh", api.refreshArtwork())
}
// refreshArtwork clears an item's resolved artwork state and re-queues it at Bump priority.
// State is deliberately cleared so a wrong pick disappears immediately (placeholder until re-resolved).
func (api *Router) refreshArtwork() http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {

View File

@ -39,9 +39,8 @@ func (pub *Router) handleImages(w http.ResponseWriter, r *http.Request) {
http.Error(w, "invalid request", http.StatusBadRequest)
return
}
// Elevated like the Jellyfin image route: the token is the authorization, so the service's
// entity check asks "is it still there", not "may this user see it" -- the latter would hide
// a shared private playlist, which is the case shares exist to serve.
// The token is the authorization, so the entity check must ask "is it still there", not
// "may this user see it" -- the latter would hide a shared private playlist.
ctx = request.WithUser(ctx, model.User{IsAdmin: true})
size := p.IntOr("size", 0)
square := p.BoolOr("square", false)

View File

@ -53,8 +53,7 @@ var _ = Describe("handleImages", func() {
return httptest.NewRequest("GET", "/img?:id="+url.QueryEscape(token), nil)
}
// The handler re-checks that the entity behind the token still exists, so every spec needs
// a store where "1" is a live album.
// The handler re-checks that the entity behind the token still exists, so "1" must be live.
var ds *tests.MockDataStore
BeforeEach(func() {

View File

@ -43,9 +43,8 @@ import (
"go.senan.xyz/taglib"
)
// The artwork serving path streams folder-backed originals via os.Open, which a fake FS cannot
// back, so this suite scans a small REAL on-disk library and drives the real acquisition worker
// and artwork.Artwork through the Subsonic and public image handlers.
// The serving path streams folder-backed originals via os.Open, which a fake FS cannot back, so
// this suite scans a small REAL on-disk library and drives the real worker and artwork.Artwork.
var _ = Describe("Artwork Serving", Ordered, func() {
var (
artRouter *subsonic.Router
@ -105,9 +104,8 @@ var _ = Describe("Artwork Serving", Ordered, func() {
ds = &tests.MockDataStore{RealDS: persistence.New(db.Db())}
auth.Init(ds)
// A re-scan of already-populated data deadlocks the parallel album-refresh phase at the
// harness's MaxOpenConns=1, so wipe the golden content and import ONLY the real artwork
// library fresh (the import path never enters that refresh phase).
// Re-scanning already-populated data deadlocks the parallel album-refresh phase at the
// harness's MaxOpenConns=1, so wipe the golden content and import this library fresh.
wipeScannedContent()
artLib := model.Library{Name: "Artwork Library", Path: musicDir}
Expect(ds.Library(ctx).Put(&artLib)).To(Succeed())
@ -201,9 +199,8 @@ var _ = Describe("Artwork Serving", Ordered, func() {
Expect(w.Header().Get("Cache-Control")).To(Equal("no-store"))
})
// The BeforeAll grants the artwork library to adminUser only, so regularUser cannot see
// this album -- but its artwork state and bytes are perfectly servable by id. The service
// resolves the entity through the caller's repositories, so the filter still applies.
// Only adminUser was granted this library, so regularUser must get the placeholder even
// though the artwork state and bytes are perfectly servable by id.
It("serves the placeholder for an album in a library the caller cannot see", func() {
w := httptest.NewRecorder()
artRouter.ServeHTTP(w, buildReq(regularUser, "getCoverArt", "id", "al-"+artfulID))
@ -212,12 +209,10 @@ var _ = Describe("Artwork Serving", Ordered, func() {
Expect(w.Body.Bytes()).To(Equal(placeholder), "must not leak the real cover")
Expect(w.Header().Get("Cache-Control")).To(Equal("no-store"))
// ...while the admin, who does have access, gets the real bytes for the same id.
Expect(getCover("id", "al-"+artfulID).Body.Bytes()).ToNot(Equal(placeholder))
})
// An album with no art and an id naming no album are different answers; only the former
// is a placeholder.
// An id naming no entity is a different answer from an album with no art: no placeholder.
It("answers error 70 for an id that matches no entity", func() {
w := getCover("id", "al-nosuchalbum")
Expect(w.Code).To(Equal(http.StatusOK))
@ -261,8 +256,8 @@ func buildArtworkRouter(art artwork.Artwork) *subsonic.Router {
)
}
// wipeScannedContent clears all scanned library content (including the golden fake library) so the
// next scan is a clean import. FKs are disabled for the bulk delete, mirroring harness.Restore.
// wipeScannedContent clears all scanned content so the next scan is a clean import. FKs are
// disabled for the bulk delete, mirroring harness.Restore.
func wipeScannedContent() {
GinkgoHelper()
_, err := db.Db().Exec("PRAGMA foreign_keys = OFF")
@ -283,8 +278,7 @@ func albumIDByName(name string) string {
return albums[0].ID
}
// writeArtworkTrack lays down one tag-distinct track (and optionally a folder cover) so the scanner
// creates a self-contained album; distinct ALBUM/ALBUMARTIST tags keep the two albums from merging.
// Distinct ALBUM/ALBUMARTIST tags keep the generated albums from merging into one.
func writeArtworkTrack(root, artist, album, title string, withCover bool) {
GinkgoHelper()
dir := filepath.Join(root, artist, album)
@ -306,9 +300,8 @@ func readArtworkFixture(name string) []byte {
return data
}
// newDummyImageCache backs the artwork.Artwork's resize cache. size=0 requests stream originals
// and never invoke the reader, so it only needs to satisfy the constructor; resize behavior is
// covered by the artwork package's own suites.
// size=0 requests stream originals and never invoke the reader, so this resize cache only has
// to satisfy the constructor.
func newDummyImageCache(ctx context.Context) cache.FileCache {
GinkgoHelper()
c := cache.NewFileCache("SubsonicArtworkE2E", "100MB", "images", 0,
@ -319,7 +312,6 @@ func newDummyImageCache(ctx context.Context) cache.FileCache {
return c
}
// runWorkerUntil drives the real worker loop until a condition holds, then cancels and joins it.
func runWorkerUntil(ctx context.Context, worker *artwork.Worker, until func() bool) {
GinkgoHelper()
runCtx, cancel := context.WithCancel(ctx)

View File

@ -349,8 +349,7 @@ var _ = Describe("helpers", func() {
})
Describe("childFromMediaFile", func() {
// The album id carries the album's hash, which hydration puts in AlbumImage; the
// track's own ItemImage describes its own art and must not be stamped onto an al- id.
// An al- id must carry the album's hash (AlbumImage), never the track's own.
It("suffixes coverArt with the album's content hash when resolved", func() {
mf := model.MediaFile{ID: "mf-1", AlbumID: "al-1", AlbumImage: model.ItemImage{ImageHash: hash}}
Expect(childFromMediaFile(ctx, mf).CoverArt).To(Equal("al-al-1_" + hash))

View File

@ -67,9 +67,8 @@ var _ = Describe("MediaRetrievalController", func() {
Expect(w.Body.String()).To(Equal(artwork.data))
})
// Visibility now lives in the service, which resolves the entity through the
// request-scoped repositories. The handler's whole contribution is handing over the
// caller's context unchanged -- elevating here would bypass the library filter.
// The service applies the library filter from the caller's context, so elevating here
// would bypass it.
It("passes the caller's context to the service rather than elevating", func() {
r := newGetRequest("id=al-34")
usr := model.User{ID: "u1", UserName: "u1"}

View File

@ -86,9 +86,8 @@ func SetupDB(ctx context.Context, users ...*model.User) *DB {
return h
}
// ResettableTables lists the tables a per-spec reset may write. FTS shadow tables are excluded;
// they are kept in sync by their content tables' triggers, and writing them directly corrupts the
// index.
// ResettableTables lists the tables a per-spec reset may write. FTS shadow tables are excluded:
// their content tables' triggers keep them in sync, and writing them directly corrupts the index.
func ResettableTables() []string {
rows, err := db.Db().Query("SELECT name FROM main.sqlite_master WHERE type='table' AND name NOT LIKE 'sqlite_%' AND name NOT LIKE '%_fts' AND name NOT LIKE '%_fts_%'")
Expect(err).ToNot(HaveOccurred())
@ -103,9 +102,8 @@ func ResettableTables() []string {
return tables
}
// TruncateDB empties every resettable table, leaving the migrated schema in place. Suites whose
// specs each build their own library use this instead of a golden snapshot, since re-migrating the
// schema per spec costs ~400ms.
// TruncateDB empties every resettable table, leaving the migrated schema in place — for suites
// whose specs each build their own library, so the schema is not re-migrated per spec.
func TruncateDB(tables []string) {
sqlDB := db.Db()
_, err := sqlDB.Exec("PRAGMA foreign_keys = OFF")

View File

@ -11,13 +11,13 @@ import (
type MockArtworkQueueRepo struct {
model.ArtworkQueueRepository
// mu guards Data so the worker's concurrent drain can hit this mock race-free.
// mu guards Data: the worker drains this mock concurrently.
mu sync.Mutex
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
// ExistingIDs, keyed by item_kind, backs PurgeDangling; a nil per-kind map keeps that kind.
// ExistingIDs is keyed by item_kind; a nil per-kind map means PurgeDangling keeps that kind.
ExistingIDs map[string]map[string]bool
}
@ -191,8 +191,7 @@ func (m *MockArtworkQueueRepo) EnqueueStaleAbsent(kind model.Kind, attemptedBefo
return inserted, nil
}
// EnqueueMissing enqueues entities in ExistingIDs[kind] that have no item_artwork row in
// ItemArtworkSource and are not already queued, mirroring the SQL set-difference insert.
// EnqueueMissing mirrors the SQL set-difference insert: ExistingIDs[kind] minus ItemArtworkSource.
func (m *MockArtworkQueueRepo) EnqueueMissing(kind model.Kind) (int64, error) {
m.mu.Lock()
defer m.mu.Unlock()

View File

@ -232,8 +232,7 @@ const LoadedAlbumGrid = ({ ids, data, basePath, width }) => {
}
const AlbumGridView = ({ albumListType, loaded, loading, seed, ...props }) => {
// A re-roll replaces every album, so the previous roll must not linger while it loads. Blanking
// on any load instead collapsed the grid to a spinner on each search keystroke.
// A re-roll replaces every album, so the previous roll must not linger while it loads.
const rerolling = useRollChanged(seed, loading) && albumListType === 'random'
const hide = rerolling || !props.data || !props.ids
return hide ? <Loading /> : <LoadedAlbumGrid {...props} />

View File

@ -8,8 +8,8 @@ describe('useRollChanged', () => {
initialProps: props,
})
// A re-roll redirects and remounts the grid, so "mounted with a load in flight" is exactly the
// case where the store still holds the previous roll. Nothing is settled until that load lands.
// A re-roll remounts the grid, so on mount a load in flight means the store still holds the
// previous roll.
it('reports a change while a load is in flight on a fresh mount', () => {
const { result } = setup({ seed: 's1', loading: true })
expect(result.current).toBe(true)
@ -20,15 +20,12 @@ describe('useRollChanged', () => {
expect(result.current).toBe(false)
})
// Typing in the search box refetches with the same seed: the albums on screen still belong to
// the roll being loaded, so they must stay put.
it('stays false while loading a filter change on the same roll', () => {
const { result, rerender } = setup({ seed: 's1', loading: false })
rerender({ seed: 's1', loading: true })
expect(result.current).toBe(false)
})
// A new seed is a new roll, so what is on screen is about to be replaced wholesale.
it('goes true while loading after the seed changes', () => {
const { result, rerender } = setup({ seed: 's1', loading: false })
rerender({ seed: 's2', loading: true })
@ -43,8 +40,7 @@ describe('useRollChanged', () => {
expect(result.current).toBe(false)
})
// The seed can land a render before loading flips, which would otherwise record the new roll as
// already shown and skip the blank entirely.
// The seed can land a render before loading flips; the new roll must not be recorded as shown.
it('still reports a change when the seed arrives before loading starts', () => {
const { result, rerender } = setup({ seed: 's1', loading: false })
rerender({ seed: 's2', loading: false })

View File

@ -11,7 +11,7 @@ import { BlurHashCanvas } from './BlurHashCanvas'
const fadeMs = 500
const useStyles = makeStyles({
// className supplies the size and shape; overflow:hidden clips the fills to a rounded shape.
// className supplies the size and shape; overflow:hidden clips the fills to it.
root: {
position: 'relative',
display: 'inline-flex',
@ -34,9 +34,8 @@ const useStyles = makeStyles({
imgInstant: { opacity: 1, transition: 'none' },
})
// Artwork renders an entity's cover through the shared useImageUrl blob cache, so it survives
// React remounts without re-fetching. The blurhash is the loading placeholder; the image is only
// mounted once its blob is ready, so an unresolved cover never renders as a broken <img>.
// Renders a cover through the shared useImageUrl blob cache, so it survives remounts without
// re-fetching. The image mounts only once its blob is ready, so it never renders broken.
export const Artwork = ({
record,
size = config.uiCoverArtSize,
@ -50,8 +49,7 @@ export const Artwork = ({
const url = record ? subsonic.getCoverArtUrl(record, size, square) : ''
const { imgUrl } = useImageUrl(url)
// A blob already cached when this instance mounted paints on the first frame, so it skips the
// fade; anything fetched later cross-fades over the blurhash.
// A blob already cached at mount paints on the first frame, so it skips the fade.
const cachedOnMount = useRef(null)
if (cachedOnMount.current === null) {
cachedOnMount.current = !!imgUrl
@ -63,8 +61,7 @@ export const Artwork = ({
setFaded(false)
}, [url])
// Retire the blurhash on a timer rather than transitionend: under prefers-reduced-motion the
// transition is none, so the event never fires and the placeholder would stay up forever.
// Timer, not transitionend: under prefers-reduced-motion there is no transition to end.
useEffect(() => {
if (!decoded || faded) return undefined
const timer = setTimeout(() => setFaded(true), fadeMs)
@ -74,11 +71,9 @@ export const Artwork = ({
if (!record) return null
const instant = cachedOnMount.current
// The blurhash stays mounted under the image until the fade ends. Swapping them the moment the
// blob arrives would expose the empty container for the length of the fade.
// Kept mounted until the fade ends; swapping on blob arrival would flash an empty container.
const showBlurHash = !!record.blurHash && !instant && !faded
// A square request is padded, not cropped, so its content is already aspect-fit inside the square
// the server returns; `contain` is what keeps placeholder and image on the same pixels.
// A square request is padded, not cropped, so `contain` keeps placeholder and image aligned.
const effectiveFit = square ? 'contain' : fit
const ratio = record.imageWidth / record.imageHeight
const handleClick = imgUrl && onClick ? onClick : undefined

View File

@ -56,8 +56,7 @@ describe('Artwork', () => {
expect(canvas.style.objectFit).toBe('contain')
})
// A square request is padded, not cropped, so the artwork still sits letterboxed inside the
// square the server returns and the placeholder has to letterbox with it.
// A square request is padded, not cropped, so the placeholder has to letterbox with it.
it('letterboxes the blurhash when the server pads a non-square image to a square', () => {
useImageUrl.mockReturnValue({ imgUrl: null, loading: true })
const nonSquare = { ...withArt, imageWidth: 1200, imageHeight: 800 }
@ -68,8 +67,7 @@ describe('Artwork', () => {
expect(canvas.style.objectFit).toBe('contain')
})
// The padded square the server returns is aspect-fit, so cropping it would disagree with the
// placeholder. Both renderers have to read `square` the same way.
// The padded square is aspect-fit, so `square` overrides fit="cover" as it does for the canvas.
it('fits the image itself with contain when the server padded to a square', () => {
useImageUrl.mockReturnValue({ imgUrl: 'blob:abc', loading: false })
const { container } = render(
@ -109,15 +107,13 @@ describe('Artwork', () => {
useImageUrl.mockReturnValue({ imgUrl: null, loading: true })
const { container, rerender } = render(<Artwork record={withArt} />)
// Blob arrives: the image mounts transparent, with the blurhash still behind it.
useImageUrl.mockReturnValue({ imgUrl: 'blob:abc', loading: false })
rerender(<Artwork record={withArt} />)
const img = container.querySelector('img')
expect(img).not.toBeNull()
expect(container.querySelector('canvas')).not.toBeNull()
// Decoding starts the cross-fade; the blurhash only goes away once it finishes. The clock
// drives it, not transitionend, which never fires under prefers-reduced-motion.
// The clock ends the fade, not transitionend, which never fires under reduced-motion.
act(() => {
fireEvent.load(img)
})
@ -138,7 +134,6 @@ describe('Artwork', () => {
it('does not fade an image that was already cached on mount', () => {
useImageUrl.mockReturnValue({ imgUrl: 'blob:abc', loading: false })
const { container } = render(<Artwork record={withArt} />)
// No placeholder to cross-fade from, so it paints at once.
expect(container.querySelector('canvas')).toBeNull()
expect(container.querySelector('img').className).toContain('imgInstant')
})

View File

@ -92,7 +92,6 @@ describe('BlurHashCanvas', () => {
rerender(<BlurHashCanvas hash="!!!not-a-blurhash!!!" />)
expect(ctxMock.clearRect).toHaveBeenCalledTimes(2)
// No new pixels drawn after the clear, so the stale frame stays gone.
expect(ctxMock.putImageData).toHaveBeenCalledTimes(1)
})
})

View File

@ -12,8 +12,7 @@ describe('useScrollToTop', () => {
expect(window.scrollTo).toHaveBeenCalledWith({ top: 0 })
})
// Navigating straight from one detail page to another (an album's artist link, say) reuses the
// component, so only the key change tells us we are looking at something new.
// Detail-to-detail navigation reuses the component, so only the key change signals a new page.
it('scrolls again when the key changes', () => {
const { rerender } = renderHook(({ id }) => useScrollToTop(id), {
initialProps: { id: 'al-1' },
@ -32,8 +31,7 @@ describe('useScrollToTop', () => {
expect(window.scrollTo).toHaveBeenCalledTimes(1)
})
// The record arrives after the first render, so the key starts undefined; scrolling then would
// fire before the page has its content and read as a no-op.
// The record arrives after the first render, so the key starts undefined.
it('waits for a key rather than scrolling on an empty record', () => {
const { rerender } = renderHook(({ id }) => useScrollToTop(id), {
initialProps: { id: undefined },

View File

@ -342,8 +342,7 @@ var _ = Describe("File Caches", func() {
})
It("re-fetches when an adopted entry's data file vanished", func() {
// Entries adopted on startup take a different code path than ones
// created in-process, so cover both.
// Entries adopted on startup take a different code path than in-process ones.
var calls atomic.Int32
fc := callNewFileCache("test", "10MB", "test", 0, func(ctx context.Context, arg Item) (io.Reader, error) {
calls.Add(1)

View File

@ -44,8 +44,7 @@ var _ = Describe("Spread FS", func() {
Describe("Create", func() {
It("leaves an already-open reader's bytes intact", func() {
// A re-created entry must not shrink the file an older stream is still
// serving: its reader would spin forever at the premature EOF.
// Shrinking a file an older stream still serves spins its reader at a premature EOF.
if runtime.GOOS == "windows" {
Skip("Windows cannot unlink a file with open handles, so Create reuses the inode")
}