Cleanup pass over the three preceding commits.
Tabulate the cosine terms in the UI blurhash decoder instead of calling Math.cos
per pixel per component: 248us -> 75us for a 32x32 decode, and an album grid
mounts one decoder per tile. Output is unchanged, which the pinned pixel specs
enforce. The Go encoder already tabulated the same terms.
Drop the dead paths that deriving components inside Encode left behind: the
zero-size guard in components, the post-downscale empty check, and the
no-AC-factor branch, which cannot be reached now that the counts are always at
least 1x9. The empty-image check moves ahead of the derivation, where it belongs.
In the queue mock, look up item_artwork by its existing iaKey rather than
scanning the map, and hold the lock across EnqueueIfMissing through a shared
unlocked helper instead of releasing it mid-operation. Extract the duplicated
drain-and-resolve block in the scanner specs into one helper.
Components had a single caller, which only ever fed it the bounds of the image
it then passed to Encode. Exporting it gave callers two ways to get it wrong —
components mismatched with the image, or out of the 1..9 range — in exchange for
a knob nobody turned.
Derive them from img.Bounds() at the top of Encode and unexport the helper. The
counts must come from the pre-downscale bounds: downscale's integer rounding can
shift the ratio across a component boundary, and the hash is a client-side cache
key. Verified byte-identical over 18 hashes spanning 9 aspect ratios.
The out-of-range validation goes with it, being unreachable once the counts are
always derived. The aspect-ratio table now asserts through Encode's size flag,
which encodes (x-1)+(y-1)*9.
A full scan re-imports every track, so the per-entity artwork enqueue in
persistChanges fired for every album and artist in the library — measured as
100% of both on a live instance, and ~8.5k artists / ~16.7k Deezer fetches on a
96k-file library. Re-importing a track is no evidence the art changed: artist
art has no track-content source at all, and the albums re-derived to identical
hashes across three consecutive scans.
Enqueue through EnqueueIfMissing on a full scan, which anti-joins item_artwork
so only entities that never resolved are queued. Incremental scans keep the
unconditional Enqueue, where a re-import does mean the file changed. Doing this
at the enqueue site rather than skipping it wholesale keeps a first scan filling
in covers as it walks, instead of stalling every cover until the scan ends.
EnqueueMissing grows a priority argument and runs once at end of scan, so an
entity phase 1 never saw still resolves, at Scan priority rather than dropping
to Recheck behind the backfill.
Mechanical cleanups, no behavior change:
- 15 copies of the same id-extraction loop collapse to slice.Map (5 repo
mocks, the scanner's track sweep, 4 wantIDs assertions) and slice.ToMap
(6 index-by-id loops in the hydration specs).
- disc.go built a map[string]bool purely to dedup folder ids and then
walked it back into a slice; slice.Unique says that directly.
- folders_artist.go's image filter is slice.Filter over model.IsImageFile.
- mock_artwork_repo deleted from a map while ranging it; maps.DeleteFunc
states the intent.
- sort.Slice -> slices.SortFunc + cmp.Or; math.Min/Max -> builtin min/max;
make+copy -> bytes.Clone; strings.Split -> SplitSeq on a per-request
path; three-clause pixel loops -> for range.
- Reuse utils.BaseName where a stem was recomputed by hand. Not at
playlist_cover.go:27: that path is a full OS path and utils.BaseName
uses path.Base, which does not split backslashes.
- Drop a dead nil-guard in agents.go: getAgent returns a bare nil
interface, and a type assertion on nil already yields ok == false.
cmp.Or was rejected for the gate fallback (func types are not comparable,
does not compile) and for ItemArtwork.AttemptedAt (cmp.Or compares
time.Time with ==, which includes loc; IsZero does not).
The follow-up loop could never fail: once the first "ar" index is asserted
to be 0, every non-"ar" element is necessarily at an index greater than 0.
A sequence like ["ar", "al", "ar"] passed both assertions, which is exactly
the interleaving the check exists to forbid.
Assert the partition directly instead: nothing after the first non-artist
call may be an artist. Verified by mutation — enqueueing artists a second
time after albums now fails the spec.
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.
Artwork state and its bytes outlive a deleted entity until the next
prune (@daily), and the serving path consulted only item_artwork, so a
Subsonic id or a signed public token kept serving a removed entity's
image in the meantime. Master's readers loaded the entity first, so this
was a regression.
The check goes in serveHash, the one path that can hand back a found
row's bytes: absent rows are already unavailable, and the provisional
and disc paths load their entity to resolve at all. Doing it there
instead of per-handler also settles who owns the invariant. Subsonic had
worked around it with artworkAccessible, whose comment described the
service "bypassing the library and private-playlist filters"; that
workaround is now deleted, since the service resolves through the
request-scoped repositories and enforces the filters itself.
Because those repositories are ctx-scoped, each caller says what it
wants by what it passes: Subsonic hands over the request context and so
gets visibility as well as existence, while the public image route
elevates like the Jellyfin one already did -- a token is the
authorization there, and a visibility check would hide a shared private
playlist, the very case shares exist for.
Two supporting fixes. albumRepository.Exists and mediaFileRepository
.Exists used the plain exists() helper, which applies no library filter,
so they reported rows in libraries the caller cannot see; they now count
through applyLibraryFilter as CountAll and artistRepository.Exists
already do. Neither had a production caller. RadioRepository gained the
Exists it lacked.
Tests follow the layers: the service refuses a found row whose entity is
gone, the repositories hide rows the caller may not see, and the
handlers only assert the context they hand over. Jellyfin needed no
change -- resolveArtworkID probes the entity tables, so a deleted item
yields an empty artwork id -- but that protection was incidental and
untested, so it is pinned now.
fmt.Sprintf ran on every newly-decoded image even with Debug off, since
Go evaluates log arguments regardless of level. Separate width/height
fields also query better than a "300x300" string.
Correction to e0f1acd1a's message: it said "all 40" messages carry the
prefix. The package has 68 log call sites and 62 distinct messages; 40
was only what the test suite happened to exercise. The sweep itself was
complete -- zero unprefixed messages remain.
Prefix: 22 messages still logged unprefixed, so a line from this package
was indistinguishable from any other subsystem's. All 40 now carry
"Artwork: ", matching Scanner:/API:/Watcher: -- which earns its place
because DevLogSourceLine is off by default.
Timing on what can actually be slow: total per acquisition (on every
exit, failures included), the read that also covers the provider
download, hashing, decode+blurhash, resize, drain batch, precache,
prune, backfill, and the external agent call -- with the rate-limiter
wait counted separately, since a throttled agent and a slow one look
identical from the drain.
Debug coverage for states that were previously silent: dedup hit vs
decode, settling absent, serving a lower-priority source after an
external failure, retry scheduling with attempts and budget left, giving
up when the budget runs out, breaker open/close per agent, provisional
read-through, dangling state rows, and the mtime mismatch that makes art
appear to vanish. outcome gained a String() so it reads as a name.
The three methods shared one comment attached to Backfill, so godoc
rendered the other two undocumented and the one it did show described
the group rather than the call. Each now opens with its own name and
says what that call does, including Backfill's bool return.
Every other core interface is named for what it is -- Playlists,
Library, Scrobbler, MediaStreamer -- while this one was artwork.Service,
the only X.Service in the tree. It becomes artwork.Artwork/NewArtwork,
and serving.go follows the type to artwork.go.
ProvideImageStore was likewise the only "func Provide" in the repo; it
is GetImageStore now, matching GetImageCache beside it. cmd/wire_gen.go
regenerated via make wire.
scheduleArtworkHousekeeping received a *artwork.Worker and then called
CreateDataStore() for a second handle onto the state that Worker already
owns, because Backfill, EnqueueStaleAbsentAll and EnqueueMissingAll were
free functions taking a DataStore.
They are now Worker methods over unexported implementations, the same
shape prune/RunPrune already uses: one public path, and the specs keep
calling the plain function with a mock store instead of standing up a
Worker. Fingerprint is unexported too -- nothing outside the package
used it.
Every source in the album and artist chains repeated the same five
lines: stamp the accumulated external failure onto a hit, or OR the
local fault into the running total on a miss. Each new source was a
chance to forget the OR, which is how the local-I/O-settles-absent bug
happened.
chainState.try does both, so each case drops to three lines and the
omission is no longer expressible. Semantics are unchanged: a hit still
carries extErr only. Also drops localErr from resolvePlaylist, which
declared it but never assigned it.
refresh.go was a 21-line file for one function that clears artwork state
and enqueues -- the same thing Backfill, EnqueueStaleAbsentAll and
EnqueueMissingAll already do next door.
Pure move.
resizedItem is the only implementation of artworkReader, which is
declared in image_cache.go, and it is used by the worker's precache as
well as the serving path -- so worker.go was reaching into serving.go
for a cache type. representationTag stays in serving.go, where the
HTTP validator belongs.
Pure move.
decodeTile and assembleTiles were in resolve.go while the geometry they
depend on -- rect, fillCenter, tileSize -- was in playlist_cover.go and
used nowhere else, so one file held the grid's helpers and another its
assembly.
Pure move.
gateFunc, passthroughGate and isTransientExternal sat in agent_images.go
while every implementation lived in worker.go: extGate, breaker,
Worker.gate, gateFor. isTransientExternal even carries a comment saying
it must stay consistent with breaker.record, which was in the other
file -- a rule spanning two files with only a comment holding it
together.
Pure move into gate.go: no symbol added or removed.
workerDeps was a parameter bag threaded into two free functions that
nothing outside the worker calls. It becomes the processor type, with
processItem and persist as acquire and persist methods on it, and Worker
holds one collaborator instead of reaching through a bag.
Kept as a separate type rather than folding onto Worker: acquisition
takes a queue item and returns bytes, while Worker.process settles the
queue row around it. That boundary is what keeps retry policy out of the
image pipeline, and what lets the acquisition specs build a three-field
value instead of a Worker with drain pools, gates, a broker and a
real on-disk cache.
The bag carried cache, which processItem never reads and only the
worker's precache uses, and carried agents/ffmpeg/gate solely to
reconstruct a resolver on every queue item. cache and ffmpeg move to
Worker, where precache actually uses them, and the resolver is built
once in NewWorker.
persist's hash parameter was redundant: decodeArtwork sets Hash and
GetImage selects it, so art.Hash already holds it on both paths.
The type itself now lives beside Worker, which owns it, rather than in
the file of the function it is passed to.
The package logged under three spellings of its own name -- "artwork: "
lowercase, "Prune: " and one "Artwork: " -- and the lowercase ones
carried lowercase message text, against 568 capitalized to 48 lowercase
elsewhere.
Prefixing itself is the convention here (Scanner:, API:, Watcher:) and
it earns its place: DevLogSourceLine is off by default, so without it a
line does not say which subsystem emitted it. So this normalizes the
spelling rather than dropping the prefix. Error strings stay lowercase
and unprefixed per Go convention.
serveHash, serveBytes and serveDisc each hand-copied the same five steps
-- test for full size, stream or build a resizedItem, call the cache,
wrap with a validator -- with a different error policy bolted on. The
ETag rule was restated at four sites and applied inconsistently.
serveSource now states it once: full size streams open() directly, and
an ETag is attached only when the bytes are resized or there is no hash
to validate against. Each caller keeps just its own error policy, and
serveBytes folds into its single caller.
Two behavior changes fall out, both narrowing an aborted request's blast
radius: serveDisc propagates context.Canceled instead of falling back to
a full album resolution, and serveHash's full-size path propagates it
instead of going dangling, which would have enqueued a re-resolution for
a request nobody is waiting on.
The resolve* chain walkers each took seven parameters -- ds, agents,
ffmpeg, gate, localOnly -- while the package already had workerDeps
bundling the same collaborators for processItem. They are now methods on
a resolver.
The external capability is one nilable field instead of three values
that had to agree. Previously a local-only resolution passed agents=nil,
gate=denyGate and localOnly=true, and only the localOnly check actually
protected anything: the external branch dereferences agents in the loop
header, before the gate closure runs, so denyGate could never fire. It
is deleted. A nil ext now both marks the resolution local-only and
removes the agents there were to dereference, and newLocalResolver takes
no parameter that could supply one.
The playlist tile loop hardcoded localOnly=false, safe only because an
early return 22 lines above it made that unreachable; it now inherits
the resolver's capability.
resolveItemLocal's guard against the remote ExternalImageURL fetch and
the 2x2 grid had no coverage: deleting it left the whole suite green
while putting synchronous network calls on the request path.
The worker resolving the same playlist is asserted alongside, so the
spec cannot pass by simply resolving nothing.
artworkReader.LastUpdated had no callers: invalidation rides entirely on
the cache key, so the interface member, the resizedItem field and its
four assignments were vestigial. Reader's second return value was
likewise discarded at all three call sites.
resizedItem.Key duplicated representationTag's format string over
identical inputs, where drift would serve a wrong-keyed entry under a
right-looking validator; it now derives from it. newResizedItem had one
caller and a doc comment claiming a sharing with worker.precache that
never existed -- precache builds its own literal.
Also unexport Prune, which no caller outside the package used while
RunPrune documented itself as the only sanctioned path, drop a
single-call placeholder wrapper, and delete five fakeFolderRepo fields
no spec ever set.
The 12h retry budget hands a bare failure to the periodic stale-absent
sweep, which is what makes the resulting absent row recoverable. Media
files are deliberately excluded from that sweep -- they resolve embedded
only, at scan or on view -- so exhausting the budget on a transient read
error recorded a "this track has no cover" verdict that nothing would
ever revisit.
Settle absent only for kinds a recheck job covers. Without a row the
track stays unresolved, so the next view re-enqueues it.
The prune read-lock wrapped all of processItem, including external fetches under
their own timeout. Since a pending RWMutex writer blocks new readers, one prune
arriving behind a slow provider stalled every subsequent item in both drain
pools. Extract persist() so the lock covers only the window it protects: store
placement plus the two row writes.
The in-flight set guarded against a queue row appearing twice in one batch, but
artwork_queue's primary key makes that impossible, drains are serial per pool,
and the pools' kind lists are disjoint. Removing it also retires the cancellation
unwind loop that existed only to release those claims.
precache built its resizedItem without setting square, so it warmed
h-<hash>.<size>.false.<quality>. The list surfaces - album grid, artwork avatars,
playlist and radio details - all request square covers, so the warmed entry was
never read and every grid cover stayed a cold miss on first view.
Set square on the precache item so the key matches the request path. The existing
specs asserted the '.300.false.' key and were updated accordingly.
serveHash routed every non-cancel cache error into dangling(), which returns
ErrUnavailable and is then rendered as a placeholder at 200 OK. A cache-layer
fault was therefore indistinguishable from an album genuinely having no artwork,
and left no trace: a broken resize cache silently served placeholders for a
quarter of the library while the logs stayed clean.
Log the error before falling back, so the cause is recoverable from the logs.
Defence in depth rather than a live hole: the reported crafted PNG
(0xffffffff square) never reaches the multiplication, because
image/png rejects it at DecodeConfig, and the largest dimensions any
supported format can declare — 2^30-1 for PNG, 16-bit for JPEG and
GIF, 14-bit for WebP — cannot overflow the int64 product.
decodeCapped is format-agnostic though, so the guard should not depend
on a decoder's own limits staying where they are. Comparing by division
holds for any dimensions a decoder might report, and non-positive ones
are now rejected outright.
os.Chmod cannot revoke read access on Windows — it only toggles the
read-only attribute — so the findImageInFolder spec opened the file
happily and failed there, and the upload spec passed for the wrong
reason: outcomeFailed came from the 1-byte payload failing to decode,
not from the source being unreadable.
findImageInFolder takes an fs.FS, so the failure is now injected and
the spec is filesystem-independent. The upload path goes through
os.Open directly and has nothing to inject, so it skips on Windows
rather than pretend to cover it.
Regression from 04d5a556. Keying the resize cache on identity meant the
disc response no longer carried a content hash, and the full-size branch
set no ETag either — so WriteImageHeaders fell back to the empty hash
and emitted `ETag: ""` for every full-size disc image. Since ifNoneMatch
compares the unquoted value, a client echoing that back matched, and got
a 304 even after the image was replaced.
The full-size branch now carries the same identity validator the sized
branch uses, so it moves when the folder's images change.
WriteImageHeaders is hardened against the class as well: an empty
validator is no validator, so it is neither emitted nor matched.
Reported by Codex on #5847.
Third and last site in this class: findImageInFolder logged and skipped
an image the glob had already matched, so a permissions or mount failure
during the artist-folder traversal read as "no image here" and let
processItem settle the artist absent, discarding any artwork already
resolved.
A matched-but-unreadable file now propagates through fromArtistFolder
and lands as localError, the same as album folder art, embedded art and
uploads. A folder with no match stays a definitive miss.
Also normalizes the e2e path assertions with filepath.ToSlash: the
stored SourcePath is OS-native, so the forward-slash suffixes failed
all 23 folder specs on Windows.
Reported by Codex on #5847.
Media files are never enqueued — the scanner drops their state without
queueing them and the recheck kinds exclude them — so an unresolved
track keeps an empty ImageHash and SongToBaseItem always fell through to
AlbumPrimaryImageTag. Finamp then asks for the album image, nothing ever
requests mf-, and the read-through that resolves the track never fires:
the own-cover branch was unreachable for Jellyfin-only users.
An eligible, unresolved, not-known-absent track now advertises its id as
the Primary tag, which is what makes the client ask. The request serves
the embedded art and queues the track so the worker persists a real
hash. No blurhash is sent, since none exists yet and a fake would be
cached against that tag forever.
Serving gains the album fallback that made this safe to advertise: an
eligible track whose frame will not extract now falls back the way
CoverArtID does instead of answering with a placeholder.
Reported by Codex on #5847.
claim() reserves the whole batch before dispatch, but the cancellation
path returned without releasing what it had not yet started, leaving
those items in the in-flight set permanently — no later drain could
claim them again.
Harmless until the batch grew past the pool size; now a cancel strands
up to a full batch. The e2e harness cancels mid-drain after every
acquire, so it surfaced there first: one spec timed out waiting for an
item that had been claimed and abandoned, and the suite went from 59s
to 87s on CI.
The identity cache key used album.UpdatedAt alone, which a replaced
disc image does not necessarily move — the sized response would then
serve the old image indefinitely. The legacy reader folded ImportedAt
and the folder's ImagesUpdatedAt into its key for exactly this reason,
and loadAlbumFoldersPaths already returns that timestamp; the disc
reader was discarding it.
The resize-cache key was the content hash, which cannot be computed
without reading the file, so a warm cache never prevented the I/O: every
sized disc request read up to 20MB and hashed it before the lookup. The
legacy reader keyed on the id and the album's mtime and touched the file
only on a miss.
Disc art has no state row and therefore no stored hash, so the key is
that same identity — id, album mtime, DiscArtPriority — and the
selection chain now runs only when the cache misses. Full-size requests
stream the source instead of buffering and hashing it.
This matters more now that single-disc albums keep running the disc
chain, which puts every disc-tagged track without embedded art on this
path.
CI runs the suite under -race, which the local `make test` does not, so
this only showed up there: 230 specs passed and the detector still
failed the run.
The drain-pools spec started Run and never waited for it, so pool
goroutines outlived the spec and raced the config snapshot Ginkgo
restores on cleanup. It now cancels, unparks the blocked lookups and
waits for Run to return.
Two test doubles also had to become concurrency-safe, since the spec is
the first to resolve several artists at once: fakeImageAgent's call
counters, and MockAlbumRepo.GetAll, which records the last query options
on a read path. MockDataStore's lazy accessors get the same treatment —
only MediaFile was guarded before, and two pools now reach them
concurrently. ArtworkQueue takes an unlocked helper for its internal
Artwork call, since repoMu is not reentrant.
resolveLocalFile swallowed every os.Open error, so uploads, playlist
sidecars, a local M3U image and the artist image folder still had the
bug that was fixed for folder and embedded sources: a permission or
transient I/O error on a file that exists read as "no image here". The
worker then settled the item absent and dropped its queue row.
Uploads outrank every other source, so an unreadable one now stops the
chain rather than letting a lower-priority image be persisted in its
place. A genuinely missing file stays a clean miss.
Reported by Codex on #5847.
A first backfill enqueues artists before albums at a single priority, so
the drain took them in that order. Artists resolve through a
rate-limited agent, and gate() waits for its permit while holding a
worker slot, so the whole pool sat asleep in the limiter with every
album queued behind it.
Measured on a 96k-track library (29,115 artists to 6,949 albums, 4:1):
zero albums resolved in seven minutes, and roughly 3.3 hours before the
first album cover would have appeared. Splitting the drain gives each
class its own slots: albums now finish in under eight minutes while
artists trickle at the same 2/s they were always limited to.
The two budgets are carved out of MaxOpenConns so a second pool cannot
take connections the scanner and the UI need. Dequeue filters by kind,
and the drain index leads with item_kind so each pool seeks to its own
work instead of scanning past the other's backlog.
The pool was fed from a batch sized to the pool itself (2x concurrency)
with a WaitGroup barrier before the next dequeue, so one item burning
its external timeout idled every other slot until it finished. The
legacy cache warmer had no such barrier: it streamed through a pipeline
of 4.
Dequeuing well past the pool keeps the slots fed for the whole pass at
no extra cost, since DequeueBatch does not mark rows taken and was
already one query per pass. Acquiring a slot now also observes
cancellation, so a larger batch cannot delay shutdown.
Warming the resize cache re-read the two rows and the file the
acquisition had just written, so every acquired image cost two extra
queries and a second full read of a file whose bytes were still in
memory. processItem now hands back what it persisted and precache warms
from that, under the same cache key the serving path computes.
Resolving the admin user also moves behind the empty-queue check: it is
needed only to resolve private playlists, so an idle server no longer
runs a user lookup on every poll.
Sweep walked the whole store with no context, and RunPrune holds the
prune write lock for its full duration. In-flight acquisitions park on
the read lock, drain's WaitGroup never returns, and Run never reaches
its ctx.Err() check — so a SIGTERM during a daily prune over a large
store on slow storage waits out the container's grace period and dies
mid-remove.
GetOrPlaceholder folded model.ErrNotFound in with ErrUnavailable, so an
id matching no entity returned 200 and the placeholder PNG. That made
the ErrorDataNotFound branch in getCoverArt unreachable: Subsonic went
from error 70 to a successful placeholder, and Jellyfin's Primary image
endpoint from 404 to 200. Neither is ours to change.
An entity with no art and an id with no entity are different answers;
only the first is a placeholder.
Exhausting the 12h budget called writeAbsent unconditionally, so an
entity whose art was already resolved and serving lost it to a long
upstream outage: the hash went empty, clients fell back to the
placeholder, and the now-unreferenced bytes were freed by the next
prune even though nothing about the image had changed.
Exhaustion means the source stayed unreachable, not that the cover
disappeared, so absent is now recorded only when there is nothing to
keep.
Local sources swallowed their open errors, so a stale NFS/SMB mount was
indistinguishable from "this entity has no artwork": the chain returned
no reader, processItem took the absent branch, and the upsert replaced a
good content hash with the empty string. Clients then saw a placeholder
until the 1h request recheck or the 24h stale-absent sweep, and the
orphaned bytes became eligible for the next prune.
A candidate the resolver knows about — a file in the folder listing, a
track's own audio file — failing to open is not evidence of absence, so
it now forces a retry the same way an external agent error does.
The deleted artwork.go carried blank imports for image/gif and
x/image/webp. WebP came back via resize.go's gen2brain/webp, which
self-registers, but GIF did not: core/artwork claims GIF support in
mimeForFormat and extForMime while relying on an unrelated server
package to have imported the decoder.
The guard lives in the e2e suite because that test binary has no other
image/gif importer; a spec in core/artwork would pass regardless, since
animation_test.go imports the package non-blank.
