MarkFailed and Delete had no production caller: the worker only ever uses
MarkFailedIfUnchanged and DeleteIfUnchanged, which refuse to act on a row
a concurrent scan re-enqueued. Keeping the unconditional pair meant the
interface offered the racy variant under the more obvious name.
MarkFailedIfUnchanged does not build on MarkFailed, so nothing in the
implementation needed them either. The repository tests used them to put
a row into a backed-off state; they now do that directly.
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.
The grid was replaced by a spinner whenever the random list was loading, so
each search keystroke collapsed it to spinner height and back. That blanket
blanking is the flicker commit 9e559311a removed for every other list; random
kept the exception so a re-roll would not flash the roll it is replacing.
Blank on a seed change instead of on any load: a re-roll gets a new seed and
still blanks, while a search keeps the seed and leaves the grid in place. The
seed is tracked from empty rather than from the current value because a re-roll
redirects and remounts the grid, which would otherwise look already-settled
with the previous roll still on screen.
Same rule for the pagination, which was hidden on the same condition.
React Router keeps the previous page's scroll offset, so opening an album from
a scrolled list started the detail page mid-song-list. Artist pages had the
same bug; it just shows less because the artist list is rarely long enough to
scroll far.
Keyed on the record id rather than mount, so detail-to-detail navigation (an
album's artist link) resets too, and so the scroll waits for the record instead
of firing against an empty page.
The zero/absent guards around width/height are ItemImage's own invariant, not
the Jellyfin adapter's. Moving them onto the model keeps one definition for
every consumer, so a second one cannot quietly disagree about what an unknown
ratio means.
Real Jellyfin carries width/height of the Primary image on BaseItemDto
(MediaBrowser.Model/Dto/BaseItemDto.cs), attaching it only when the request's
Fields asks for it (DtoService.cs ContainsField). The dimensions are now on
model.ItemImage for the web UI's blurhash placeholder, so the adapter can
report the same thing for free.
Gated behind Fields to match, and omitted rather than defaulted when the item
has no image or unknown dimensions: real Jellyfin falls back to a per-type
default of 1 for music, but a wrong ratio mis-shapes a client's placeholder,
and we only lack dimensions when the artwork is genuinely unresolved.
primaryImageTag becomes primaryImage, returning the tag, blurhashes and ratio
together, so the choice of which image is Primary is made once per mapper
rather than the same ItemImage being threaded through two calls.
ArtistToBaseItem and PlaylistToBaseItem take Fields now, like the album and
song mappers already did.
The UI decoded every blurhash into a 32x32 bitmap and stretched it to fill its
container, so a non-square cover showed a full-box blur that collapsed into a
letterboxed image the moment it loaded. On the album detail page the
placeholder overhung the image by a third of the box height.
A blurhash string carries no aspect ratio of its own, so the dimensions have to
come from the server. artwork.width/height were already stored and read by
nobody; they now surface on ItemImage as imageWidth/imageHeight, hydrated
through the join that was already in place. Existing rows already carry them,
so no migration or rescan is needed.
A square request is padded rather than cropped, which aspect-fits the content
inside the square the server returns. That made the grid a second instance of
the same bug, so `square` now implies contain for the image as well as the
placeholder, instead of the two renderers reading it differently.
Unlinking before re-creating fixes the premature-EOF spin on unix, but Windows
refuses to remove a file another handle still has open and returns a sharing
violation. Because Create surfaces that error, every cache miss on a path with a
live reader would have failed outright - worse than the spin it was meant to fix.
Split the create behind a build tag: unix unlinks for a fresh inode, Windows keeps
truncating in place and stays exposed to the spin, which is the behaviour it
already had. The unix-only spec is skipped there.
Create re-opened the path with O_TRUNC, which shrinks the file out from under an
older stream that may still be serving readers. stream.Reader then hits EOF from
the OS at the new, shorter length while the broadcaster still reports the original
size, so Wait() reports 'more data exists' and the reader retries forever - a tight
pread loop that burns a core and never releases its handle, which in turn blocks
Stream.Remove() indefinitely.
Unlink first and create with O_EXCL so the new entry gets a fresh inode. Existing
readers keep their descriptor on the old inode, see its full contents, and reach a
clean EOF.
Note this does not address the deferred unlink deleting the re-created file, which
is handled separately by the re-fetch in fileCache.Get.
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.
fscache's Remove drops the in-memory entry, releases the lock, and only then
unlinks - blocking until every outstanding reader closes. A Get landing in that
window re-creates the file at the same path under a fresh entry, and the deferred
unlink deletes those new bytes. The entry survives pointing at nothing, and since
a present entry is treated as a hit, every later Get for that key returns ENOENT
for the rest of the process's life. Only a restart, which rebuilds the map from
disk, cleared it.
Get now drops such an entry and retries once, so a vanished data file costs one
re-fetch instead of poisoning the key permanently. This also covers a file
disappearing for reasons unrelated to that race, such as external deletion or a
restored backup.
Specs cover an in-process entry, one adopted at startup, and the deferred-removal
race itself.
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.
Hydration stamped the album's hash and blurhash onto an embedded-eligible
track that had no state row yet. Serving takes provisionalEmbedded for
exactly that case and returns the track's own embedded image, so the id
carried a content-version belonging to a different picture: every such
request fell back to no-cache instead of immutable, and a client keying
its cover cache on the blurhash paired the album's with the track's art.
AlbumCoverArtID had the mirror-image problem, building the album id from
the track's own ItemImage. It happened to work only because hydration
overwrote ImageHash in precisely the fallback cases; a track with its own
resolved art would have stamped that hash onto the album's id.
Under prefers-reduced-motion the img rule sets transition:none, so
toggling opacity fires no transitionend and the handler that unmounts
the blurhash never ran. The placeholder stayed mounted for the life of
the component — visible in the letterbox bars wherever the cover is
rendered with fit="contain", and a live canvas per tile everywhere else.
One duration constant now drives both the CSS transition and the timer,
so they cannot drift.
The blurhash unmounted the moment the blob arrived, so the placeholder
vanished a frame before the image painted. The image now mounts
transparent and fades in over the blurhash, which stays behind it until
the fade completes. A blob already cached when the instance mounts
skips the fade, so a remount does not re-animate.
loadTracks and the playlist-track cursor were the only entity-page
paths that never hydrated artwork state, so a song reached through a
playlist behaved differently from the same song in the songs list:
Subsonic emitted a hashless coverArt id, which imghttp downgrades to
no-cache, and advertised art even for known-absent albums; Jellyfin
emitted AlbumPrimaryImageTag as the bare album id — a tag that never
changes when the cover does — and no blurhash at all.
The media-file hydration moves next to the other hydration helpers so
both paths share one implementation rather than growing a third.
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.
The conflict clause updated only priority and retry_at, so a row that
already existed kept its original attempts and enqueued_at. The worker
measures the 12h give-up budget from enqueued_at, so any row that had
been pending across a longer gap — a server left off, an upgrade, a
laptop asleep — gave up on its very first attempt and settled absent.
The manual re-resolve endpoint is the sharpest case: it clears artwork
state and re-queues, but inherited the old row's spent window, so a
deliberate retry got one shot. A fresh request now gets a fresh budget,
with the mock updated to match.
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.
ed4178a6 gated serveDisc on len(album.Discs) > 1, claiming parity with the legacy
reader. The legacy reader has no such gate: artwork.go dispatches every dc- id to
newDiscArtworkReader, whose Reader() walks DiscArtPriority unconditionally.
The gate also lost art. For a single-disc album whose only image is disc1.jpg,
the disc request skipped the chain and fell through to album art, which does not
match CoverArtPriority — so tracks tagged disc 1 (whose CoverArtID is a dc- id)
served nothing at all, where before they served disc1.jpg.
A single disc can legitimately have its own cover, distinct from the album's, and
DiscArtPriority is what expresses that preference. Drop the gate and restore the
single-disc e2e scenarios that covered it.
The serving cutover removed the album/disc/artist/mediafile/playlist/radio e2e
specs that documented the folder-selection rules and guarded the #5376/#5456/
#5451/#5457 regressions; nothing replaced them, so compareImageFiles and the
parent-fallback logic were left untested.
Restore them driving the real pipeline: a real scanner populates the folder
graph from an in-memory library, the real Worker drains the queue, and the real
Service serves. Folder-backed art is file-backed (served via os.Open, which the
in-memory FS can't satisfy) so its selection is asserted on the persisted state
row; store-backed and real-disk sources are asserted byte-for-byte. Single-disc
disc resolution now serves album art directly, so only multi-disc disc scenarios
are ported.
Two gaps in when the pipeline re-resolves artwork:
The recheck job only requeued absent-state rows (hash=''), so an entity that was
never processed — added between scans, or on a server with the scanner disabled —
had no periodic safety net and stayed without artwork indefinitely. Add
EnqueueMissing(kind): a SQL set-difference enqueueing entities with no
item_artwork row at Recheck priority (ON CONFLICT DO NOTHING, so it never
disturbs a queued row). Run it once at startup and hourly alongside the
stale-absent recheck. Rename staleAbsentKinds -> recheckKinds accordingly.
Conversely, the config fingerprint included consts.Version, which embeds the git
SHA and so changed on every build, re-enqueueing every entity in the library
(~34k here) and re-querying external agents at the configured RPS for anything
without local art. Replace it with an explicit artworkEpoch constant, bumped
deliberately when resolution semantics change. The cases that motivated the
version input — absent art becoming available — are already covered by the
stale-absent and missing-row rechecks; only a corrected wrong-pick needs the
epoch. A test guards against reintroducing the version.
An agent (notably Last.fm's album.getInfo) can advertise a cover URL that is
itself dead — a 404. sources.go's fromURL returned a generic error for any
non-200, so a dead URL was treated as a transient failure: it churned in
backoff and counted toward the circuit breaker, stalling valid lookups.
Map 404/410 to model.ErrNotFound in fromURL so a dead URL settles absent, and
collapse the near-identical fetchPlaylistImageURL (which already did this for
M3U covers) into it.
The worker's res.reader==nil && extError branch returned outcomeFailed with no
log, so a failing external cover lookup (agent error, dead image URL, download
timeout) was undiagnosable. Log the agent, entity, and underlying error at the
fetch site where it's in hand — this surfaced a Last.fm album.getInfo returning
an image URL that itself 404s.
Last.fm returns error 6 for a missing artist/album — a definitive negative —
but the agent returned the raw *lastFMError, so the artwork worker treated
every not-found as a real fault: it counted toward the per-source circuit
breaker (5 in a row opens it, fast-failing all Last.fm calls including valid
ones) and was retried as a transient error instead of settling absent. On a
first scan of a library with many artists Last.fm lacks, this stalled valid
cover lookups and left entities churning in backoff.
Translate error 6 to the shared agents.ErrNotFound at the agent boundary
(callAlbumGetInfo / callArtistGetInfo), matching how the Deezer agent maps its
client's not-found, and log it at Debug instead of Error — which also removes
the not-found log spam.
Last.fm's album.getInfo by MBID is unreliable: a correct MBID can return a
different album, or none. Observed with black midi's "7-eleven" (whose correct
MBID returned a FLEETWOOD release) and both missing The Chats albums (one MBID
404s, the other resolves to a different self-titled release). The worker then
recorded covers absent — or would fetch the wrong art — even though the correct
cover is on Last.fm by name+artist.
Stop passing the MBID to album.getInfo; query by name+artist only, which also
drops the now-dead error-6 MBID-retry fallback. The low-level client keeps its
MBID support for other callers; only the album lookup changes.
