14 commits

Author	SHA1	Message	Date
Deluan Quintão	a7365e119b	fix(subsonic): update the playlist changed timestamp when renaming a smart playlist (#6082) ... `buildPlaylist` reported `evaluated_at` as `changed` for smart playlists, so a rename or comment edit was invisible to clients until the next evaluation. A never-evaluated smart playlist also reported the current time on every call, which never settled. Report `updated_at` for every playlist. `refreshCounters` now syncs the stamp it writes back onto the model, and `refreshSmartPlaylist` reuses it for `evaluated_at`. `changed` therefore still equals the evaluation time for a just-evaluated playlist, and `validUntil` stays anchored to it. Original Subsonic always bumps `changed` on any playlist update, so this also aligns the behavior with upstream.	2026-09-03 16:07:53 -04:00
Deluan Quintão	1f3034f022	fix(playlist): block track edits on synced playlists across all APIs (#5984) ... * fix(playlist): block track edits on synced playlists across all APIs A synced playlist's tracks come from its source file, so any track edit made through the UI or an API was silently reverted on the next scan. Track mutations funnel through two service guards, checkTracksEditable (incremental edits) and Create (wholesale replace, used by Subsonic createPlaylist and Jellyfin's replace path), which each duplicated the smart-playlist check. Both now consult a shared model.Playlist.TracksEditable() predicate, so the native, Subsonic, and Jellyfin paths are all locked: track edits return ErrNotAuthorized (403, or Subsonic error 50) instead of being accepted and lost. Metadata-only edits (name, comment, public, the sync flag itself) still go through checkWritable and are unaffected. In the UI, a synced playlist's track list becomes read-only, mirroring how smart playlists already behave. * fix(playlist): return 409 Conflict for non-editable playlist track edits The previous commit rejected track edits on smart and synced playlists with ErrNotAuthorized (403). That conflates two different things: a 403 says the caller lacks permission, but a synced or smart playlist's tracks are immutable for everyone, including the owner and admins. It is a property of the resource, not the caller. Introduce ErrPlaylistNotEditable and return it from both track-edit guards. The Native and Jellyfin APIs now map it to 409 Conflict; Subsonic maps it to error 50, the closest code it has (it has no read-only concept). The Native track handlers previously mapped this rejection inconsistently (400 on add, 500 on remove, 403 on reorder) through a new shared writePlaylistError helper. Genuine authorization failures (non-owner, non-admin) still return ErrNotAuthorized. * fix(playlist): surface synced read-only state in picker, Jellyfin, and OpenSubsonic Follow-up to the track-edit lock: the read-only state was enforced but not advertised consistently, so clients still offered edits that the server rejects. - UI: the Add to Playlist picker filtered targets by isWritable only, offering synced playlists that then 409 on add. It now filters with canChangeTracks. - Jellyfin: addToPlaylist/removeFromPlaylist hard-coded every error to 404, so a locked playlist reported "not found" instead of 409. They now return 409 for ErrPlaylistNotEditable while keeping the deliberate anti-probing 404 for every other error (a non-owner never reaches ErrPlaylistNotEditable, so 409 leaks nothing). - OpenSubsonic: buildOSPlaylist marked only smart playlists readonly; owned synced playlists advertised readonly=false. Readonly now also covers !TracksEditable(), matching the existing smart-playlist treatment. * fix(jellyfin): report CanEdit from playlist editability in permission probes getPlaylistUsers and getPlaylistUser returned CanEdit: true unconditionally, so Finamp (which probes this before showing edit controls) offered track editing on synced/smart playlists whose add/remove requests now return 409. Both handlers now fetch the playlist and set CanEdit from TracksEditable(), keeping the deliberate non-owner looseness (CanEdit stays true for a normal playlist a non-owner views) and mapping any lookup error to 404 like the sibling probes. * fix(playlist): check ownership before editability when replacing tracks Create checked TracksEditable() before ownership, so a non-owner replacing another user's public smart/synced playlist (Jellyfin updatePlaylist with a non-empty Ids list) received a 409 read-only conflict instead of a 403 authorization failure. The incremental guards check ownership first via checkWritable; Create now matches that order. Subsonic is unaffected (both errors map to code 50). Owners of their own smart/synced playlists still get the read-only conflict. * fix(jellyfin): return 403 for locked playlists, matching Jellyfin Jellyfin itself refuses edits on its file-backed playlists with Forbid() (403): PlaylistsController gates every mutation on OwnerUserId == caller or a share with CanEdit, and playlists imported from .m3u files satisfy neither. Its CanEdit is an ACL field, not a read-only marker, and Jellyfin core has no server-managed playlist type at all. Our Jellyfin routes exist to imitate that API, so ErrPlaylistNotEditable now maps to 403 there instead of 409. The native API keeps 409 (a resource-state conflict is the accurate REST answer where we define the contract) and Subsonic keeps error 50, its closest code. * chore(playlist): trim comments added by this branch Several comments ran to three or four lines and carried rationale that belongs in the commit history rather than the code: what Jellyfin does with its own file-backed playlists, and restatements of the expressions directly below them. Each block is now one or two lines covering only the non-obvious why.	2026-08-19 08:47:53 -04:00
Deluan Quintão	944ca3100f	feat(artwork): new artwork pipeline with background resolution and Low Quality Image Placeholders (#5847) ... * feat(artwork): add artwork, item_artwork and artwork_queue tables * feat(artwork): add artwork models, repository interfaces and mocks * feat(artwork): implement artwork repository * feat(artwork): implement item_artwork repository with batched hydration * feat(artwork): implement artwork_queue repository * feat(artwork): add content-addressed originals store * feat(artwork): add artwork prune (orphan cleanup) * fix(artwork): never sweep files on transient DB errors during prune * refactor(artwork): fold originals package into core/artwork as ImageStore * refactor(artwork): merge item artwork state into ArtworkRepository * fix(artwork): chunk unbounded IN clauses and restore interface docs * refactor(artwork): apply simplify-pass cleanups Internal item_artwork sqlRepository helper, toSQLArgs upserts, batched queue enqueue, EnqueueStaleAbsent moved to queue repo, snapshot-based prune sweep, mock/real semantics aligned. * fix(artwork): address review findings on prune/sweep races and mock fidelity Sweep now honors an mtime grace window (in-flight acquisitions and temp files), reacquired orphans reset the prune grace window, and the queue mock implements real stale-absent semantics. * fix(artwork): atomic orphan deletion and timestamp semantics from review DeleteOrphans re-checks age+references at delete time, PutItemArtwork defaults attempted_at, queue mock timestamps mirror SQL. * fix(artwork): guard orphan file removal with the prune grace window Duplicate ImageStore writes refresh the file mtime and Remove skips files newer than the cutoff, so overlapping acquisitions cannot lose their store files to a concurrent prune. * fix(artwork): rewrite vanished duplicates and sweep stale mime variants Write falls through to a real write when the liveness touch fails, and sweep retention now matches the recorded mime's extension so obsolete variants are reclaimed. * fix(artwork): index artwork_queue in dequeue order The previous leading retry_at range column forced a temp B-tree sort of the whole eligible set on every DequeueBatch; ordering the index by (priority DESC, enqueued_at) lets scans stop after the batch size. * fix(artwork): honor the orphan cutoff in the repository mock The mock's DeleteOrphans now applies createdBefore like the SQL implementation, and a new spec covers a freshly reacquired row surviving prune. * fix(artwork): reject malformed hashes in ImageStore operations Known-absent states carry an empty hash and malformed persisted hashes could panic path sharding or inject separators; Write/Open/Remove now return an error for anything but 16 lowercase hex chars. * fix(artwork): mock PutImage refreshes created_at like the SQL repository Prune specs now age fixtures directly instead of seeding stale timestamps through the upsert. * fix(artwork): store backing-file provenance per item, not per hash * feat(artwork): import blurhash encoder from #5797 * feat(artwork): add worker-side artwork resolvers * fix(artwork): propagate playlist tile failures and dedupe external step * feat(artwork): add acquisition processor Resolves one queue item end to end: hash/dedup, decode + 128px thumbnail blurhash, place bytes (store vs source file), and persist found/absent/ failed state for the worker (Task 4) to act on. * style(artwork): tighten processor comments to budget * feat(artwork): add acquisition worker service * feat(artwork): enqueue artwork resolution from scan and CRUD paths * feat(artwork): artwork backfill, fingerprint re-resolution and scheduled jobs * test(artwork): leak/soak coverage and deferred assertions * fix(artwork): propagate transient artist image errors to the worker callGetImage swallowed all agent errors, so an agent outage surfaced as ErrNotFound and the worker settled artist artwork as a definitive absent (and reset the breaker). Add an additive ArtistImageResult path that returns the underlying agent error on transient failure while keeping ArtistImage byte-identical for existing callers; the worker's artist external step uses it via fromArtistExternalResult. * fix(artwork): resolve full playlist source chain resolvePlaylist only built the generated grid, dropping the uploaded-image, sidecar and ExternalImageURL sources the old reader_playlist.go chain serves. Port the full chain before the grid fallback: uploaded (upload), sidecar (folder), and ExternalImageURL routed through extGate with the same extError semantics as the other external steps. Also rewires the artist external step onto ArtistImageResult. * fix(artwork): purge dangling queue rows and guard concurrent re-enqueues Queue rows for deleted entities failed forever (Get -> ErrNotFound -> failed -> capped retries, unbounded). Add ArtworkQueueRepository.PurgeDangling, called from Prune next to the item_artwork purge. Separately, the found/absent path unconditionally deleted the dequeued row, erasing a concurrent scan re-enqueue; switch to DeleteIfUnchanged, which deletes only while retry_at still matches the dequeued value (verified retry_at is the column an Enqueue upsert resets). * style(artwork): fix comment accuracy and budget; fingerprint ArtistImageFolder Correct the inverted workerDeps.extGate comment, trim over-budget doc comments, and add conf.Server.ArtistImageFolder to the resolution fingerprint so an image-folder change re-resolves artist artwork. * fix(artwork): treat missing local playlist cover as definitive, not transient A playlist ExternalImageURL pointing at a local file that fails to open was routed through extError, causing failed/48h-retry loops that burn a rate limiter token forever instead of falling through to the generated grid. * refactor(artwork): deduplicate purge loop, backfill table, and extGate alias * fix(artwork): cap resolved image reads A user-editable ExternalImageURL can point at an arbitrarily large endpoint; a fast server could make the worker buffer hundreds of MB inside the 5s HTTP timeout. Bound the read to a fixed 20MB cap (no config knob) via io.LimitReader and fail the item if it is exceeded. * fix(artwork): retry higher-priority external art after fallback hit With CoverArtPriority="external,cover.jpg", a transient external failure followed by a folder hit dropped the external error: the worker recorded found and deleted the queue row, so the configured higher-priority external art was never retried. Carry extError onto the fallback resolution and add an outcomeFoundStale that persists+serves the art but reschedules via MarkFailed, giving the external source another chance. When external later answers definitively-not-found, the hit is not stale and the row is deleted. * fix(artwork): treat playlist cover URL 404 as definitive miss The playlist ExternalImageURL step used sources.go's fromURL, which maps any non-200 to a generic error, so a stale URL returning 404/410 was classified transient: infinite backoff plus it counted toward the circuit breaker, blocking valid external work. Add a local fetch in resolve.go that maps 404/410 to model.ErrNotFound (definitive) while keeping other non-200s transient. sources.go is left untouched. * test(artwork): move soak test into the Ginkgo suite * test(artwork): make leak and permission tests pass on linux goleak now ignores notify's nonrecursive-tree goroutines (linux uses inotify, which spawns dispatch+internal instead of darwin's recursive dispatch), and the read-only-dir prune spec skips under root, where permission bits cannot make Remove fail. * fix(artwork): reject decompression-bomb dimensions before decoding * fix(artwork): keep fresh re-enqueues ahead of stale failure backoff * fix(artwork): include M3U external art flag in the config fingerprint * fix(artwork): resolve private playlists with an admin context * test(artwork): convert non-synctest timing tests to Ginkgo specs TestArtworkBackoffSchedule and TestArtworkWorkerRunNoLeak needed no real *testing.T (no synctest), so move them into worker_test.go as Ginkgo specs. TestArtworkBreakerHalfOpen stays plain since testing/synctest requires a real *testing.T, matching core/scrobbler's precedent. * fix(artwork): store backing-file provenance per item, not per hash * fix(artwork): apply image limits to playlist tile decoding decodeTile ran image.Decode on every sampled album's resolved bytes before processItem's maxImageBytes/maxImagePixels guards applied, letting an oversized or decompression-bomb tile fully decode unbounded. Enforce both caps inside decodeTile itself. * refactor(artwork): reuse auth.WithAdminUser and dedupe image cap guards * perf(artwork): fetch only IDs for backfill enumeration Backfill enumerated every album, artist, playlist and radio via GetAll and mapped out just the ID. GetAll materializes full entities (library joins, participant/stats/tags JSON, annotation, artwork hydration), so on a large library it loaded tens of thousands of heavy structs only to read one field each — spiking transient RSS to ~1GB during the one-time upgrade backfill, a memory risk on small NAS/Pi hardware. Add GetAllIDs to the album, artist, playlist and radio repositories: it reuses each repo's base row-set filter (library visibility, artist content join, playlist userFilter) but projects only id, skipping the heavy columns and post-processing. A per-repo parity test asserts GetAllIDs returns exactly the same id set as GetAll. Verified on a 727MB / 29k-artist production DB copy: peak RSS during backfill dropped from ~1012MB to ~89MB, file descriptors flat, same 36,138 items enqueued. * feat(artwork): promote worker concurrency and external rate to real configs The artwork worker's drain speed was governed by two hidden Dev flags, DevArtworkWorkerConcurrency and DevArtworkExternalRPS, both defaulting to 2. On a large library's one-time backfill the external rate limiter is the real ceiling: every art-less item waits on it before the (rate-limited) external lookup, so the drain crawls at ~RPS items/sec while local-art items are unaffected. Promote both to documented, supported options: ArtworkWorkerConcurrency (default 4) sets local-resolution parallelism, ArtworkExternalMaxRPS (default 2, 0 = unlimited) caps external-agent lookups to stay polite to Last.fm/Deezer/etc. Operators can now trade first-backfill speed against external-API rate limits. The old Dev names still map for backward compat. * fix(deezer): never return empty-image-id placeholder pictures * feat(agents): enumerate enabled image-retriever agents per capability * feat(artwork): worker fetches agent images directly with per-agent rate limits and breakers * fix(artwork): treat agent not-found as breaker success * feat(model): content-hash artwork id suffix and hydratable per-entity image state * feat(persistence): hydrate artwork hash and absence onto entity pages * feat(artwork): resolve media_file embedded art in the worker, invalidate on rescan * feat(artwork): broadcast refresh events when artwork lands * fix(artwork): broadcast refresh for stale-found artwork too * feat(artwork): state-backed serving path with provisional read-through * feat(server): serve artwork from persisted state with content-hash caching * feat(subsonic): content-hash coverArt ids, omit artwork on known-absent * refactor(artwork): delete the legacy reader chain, cache warmer, and provider image methods * feat(artwork): precache on acquisition, bump on upload/radio changes, manual re-resolve API * test(artwork): end-to-end coverage for the serving cutover * chore(artwork): generic 500 bodies on refresh endpoint, trim stale test comments * fix(artwork): request read-through must not reset the failure backoff The provisional read-through and dangling re-enqueue used Enqueue, whose upsert resets retry_at, so any browse of an unresolved entity that was backing off after an external failure made it immediately eligible again — defeating the exponential backoff during a provider outage. Add EnqueueBump, which raises priority but leaves an existing row's retry_at intact, and route the serving path through it. Scan and manual re-resolve keep Enqueue's reset (a detected change wants immediate retry). * fix(artwork): keep an eligible track's cover requestable when its album is absent An embedded-eligible track with no resolved item_artwork row inherited the album's ImageAbsent, so when the album resolved absent (e.g. CoverArtPriority without 'embedded') the track's coverArt was omitted permanently — the client never requested it, so the lazy mediafile path never resolved it — even though the serving path would extract and serve the track's own embedded art. Hydration now never copies the album's absence onto an eligible-but-unresolved track. * fix(artwork): validate each agent image URL before picking the largest bestImageURL selected the largest by size and only then parsed it, so a malformed largest URL (e.g. a bad percent-escape) returned nil and shadowed a valid smaller candidate, contradicting the documented skip-unparseable behavior. Parse per candidate and compare sizes only among URLs that parse. * fix(artwork): fall back to disc art, not the album, for multi-disc tracks serveMediaFile delegated an absent/ineligible track straight to AlbumCoverArtID, skipping the disc-specific lookup that MediaFile.CoverArtID (and the deleted legacy reader) use. On multi-disc albums with per-disc images that served the album cover instead of the configured disc artwork. Delegate through DiscCoverArtID. * fix(artwork): enqueue uploaded artwork only after the filename is persisted SetImage cleared state and enqueued the bump before the caller stored the new filename, so a worker drain in that window could resolve against the old (already deleted) file and settle absent, leaving the upload unused until a later scan. Move the invalidate+enqueue into EnqueueArtwork, which each caller now invokes after the entity Put. * fix(artwork): keep multi-disc tracks requestable when the album is absent Round-1's hydration fix still copied the album's known-absent onto a non-eligible (or own-absent) track, but MediaFile.CoverArtID routes a multi-disc track to disc art, which resolves provisionally and is never known-absent. Marking it absent made Subsonic omit coverArt so clients never requested a valid disc image. Only mark a single-disc track absent, and only when its own art won't resolve. * fix(artwork): serve a local playlist ExternalImageURL as a file-backed reference A local ExternalImageURL was resolved through the external step and labelled external, so placeBytes copied it into the content-addressed store and dropped its path/mtime — replacing the file never tripped the staleness check. Classify local references as file-backed (resolved in place, even on the request path) and keep store-backed behaviour only for http(s) URLs. * fix(artwork): requeue playlist cover when its track set changes A generated-grid cover went stale after track mutations: nothing re-resolved the playlist's artwork, and the request path deliberately never rebuilds the grid, so serveEntity kept returning the old grid hash indefinitely. Enqueue pl artwork from refreshCounters (the choke point for every track-set change); no clear, so the old cover keeps serving until the worker rebuilds. * fix(artwork): open library-backed artwork through its on-disk root A library configured with a file:// path stored absRoot as the raw URI, so Abs produced strings like file:/music/cover.jpg that os.Open/os.Stat reject — folder, upload and embedded art were treated as dangling on every request, looping forever. Normalize a file:// path to its parsed OS path (the same root os.DirFS uses); non-local schemes are left unchanged (out of scope, per the artwork-musicfs TODO). * fix(artwork): only use disc resolution for multi-disc albums DiscCoverArtID returns a dc- id for any track with DiscNumber>0, so serveDisc ran the full DiscArtPriority chain even for single-disc albums, where a stray disc*/ embedded image could shadow higher-priority album art. Gate disc resolution on the album having more than one disc, matching the legacy reader; single-disc tracks serve album art directly. * fix(artwork): invalidate artwork when an uploaded image is deleted Deleting an artist/radio/playlist upload cleared the filename but left the found item_artwork row and its hash, so lists kept advertising the deleted cover's hash-suffixed immutable URL and clients could display it indefinitely. Call EnqueueArtwork after the delete-side Put, symmetric with upload, so the state is cleared and re-resolved to the next source (or absent). * fix(artwork): restore synthetic-artist guard and unicode normalization in agent lookups Moving agent calls into the worker bypassed two behaviors of the aggregate provider: Agents.GetArtistImages' guard for Unknown/Various Artists (a direct retriever call could assign an unrelated image to a synthetic artist), and auxAlbum/auxArtist.Name's DevPreserveUnicodeInExternalCalls normalization (records with typographic quotes/dashes missed exact-name searches). Re-apply both before enumerating retrievers. * fix(artwork): enforce entity visibility on the Subsonic getCoverArt path serveEntity reads persisted item_artwork by id, bypassing the library and private- playlist filters that the legacy entity-load applied. On the authenticated Subsonic path a user could fetch artwork for an inaccessible album or someone else's private playlist by guessing an id. getCoverArt now resolves the underlying entity through the request-scoped (filtered) repositories and serves the placeholder when it is not visible, so existence isn't leaked and the always-an-image invariant holds. The public share (JWT-authorized) and Jellyfin (admin) paths are intentionally untouched. * fix(artwork): version the artwork ETag with the served representation The ETag was the pixel hash of the original image, so a CoverArtQuality or EnableWebPEncoding change altered the resized bytes without changing the ETag — revalidating clients got a spurious 304 and kept the old encoding. Resized responses now carry a representation ETag (hash + size + square + encode settings) used for the ETag header and If-None-Match, while the immutable decision stays on the pixel hash (URLs remain pixel-identity per the spec, so hash-suffixed clients keep zero-request caching). Full-size originals fall back to the pixel hash as before. * fix(artwork): don't stamp the album hash onto multi-disc tracks The hydration fallback assigned a found album hash to every fallback track, but a multi-disc track's CoverArtID emits a dc- id served from disc-specific art whose hash is unknown at hydration time. Advertising dc-..._<albumHash> gave clients a content- version that never changes when the disc image does, breaking id-based refresh. Only stamp the album hash for single-disc tracks (DiscNumber == 0); multi-disc tracks stay unhashed and rely on the correct ETag returned by the served response. * fix(artwork): enqueue new empty playlists by id, and refresh on absent outcomes Two worker/enqueue fixes from review: - playlistRepository.Put assigned the generated id to the caller's Playlist but passed the stale copy (empty id) to refreshCounters, enqueueing a pl|"" row the worker failed until the daily dangling purge while the real playlist went unresolved. Set the id on the copy before enqueueing. - The drain refresh batch only included found/foundStale, so a cover removed by a scan (found -> absent) never notified clients, leaving the old immutable image displayed. Broadcast absent outcomes too; precache still only warms found/foundStale. * fix(artwork): honor disabled per-track art at serve time; use nanosecond mtime provenance Two serving-correctness fixes from review: - serveMediaFile served a persisted mf embedded image even after EnableMediaFileCoverArt was turned off (the setting isn't in the config fingerprint, so found rows aren't reprocessed). Direct mf- URLs now honor the setting at serve time and fall back to disc/album art. - The file-backed staleness check compared whole-second mtimes, so a same-second content replacement (two writes in one second, or timestamp-preserving tools) could serve different bytes under the old hash + immutable policy. RefMtime is now unix-nanoseconds (no schema change; int64 column), detecting sub-second changes where the filesystem records them. * fix(artwork): preserve the drive when normalizing Windows file:// library paths url.Parse puts the volume of file://C:/Music in Host, not Path, so localOSRoot dropped it and returned /Music — os.Open/os.Stat then failed and folder/embedded art on Windows looped as dangling. Rejoin the host volume, matching core/storage/local's newLocalStorage. * fix(artwork): clamp negative sizes to full-size; convert imghttp test to Ginkgo - A negative size (Subsonic size / Jellyfin maxwidth accept signed ints) reached resizeStaticImage, where the square path builds image.NewNRGBA(Rect(0,0,size,size)) — a giant rectangle that panics/OOMs. Clamp size<0 to 0 (full-size) at the Service entry. Positive sizes were already clamped to the original. - imghttp used a plain func Test with a table; convert to a Ginkgo DescribeTable with the suite entry point in imghttp_suite_test.go (AGENTS.md test-framework requirement). * test(artwork): use renamed ArtworkWorkerConcurrency in e2e tests * feat(artwork): carry blurhash through item image hydration * feat(nativeapi): expose artwork hash, absence and blurhash * feat(artwork): hydrate the parent album's artwork state onto tracks * test(artwork): add hydrateArtwork regression guard for AlbumImage wiring Drives hydrateArtwork itself (not applyItemImage directly) over tracks that take each of the loop's continue branches, so a future edit moving the AlbumImage fill below a continue would fail loudly instead of passing silently. * feat(jellyfin): version album and artist image tags by content hash * fix(jellyfin): trim primaryImageTag comment to why-only, within budget * feat(jellyfin): emit real blurhashes and drop the synthesized fallback * feat(ui): version cover art urls by content hash and skip absent art * feat(ui): add BlurHashCanvas placeholder component * fix(ui): clear stale blurhash pixels and assert the draw path in tests Clear the canvas before each decode attempt so a hash change that fails to decode doesn't leave the previous frame's pixels on screen once this wires into a list that recycles items. Also strengthen the specs to assert createImageData/putImageData were actually invoked (and with what), instead of only checking that a <canvas> element exists. * feat(ui): show the blurhash while an album cover loads * fix(artwork): hydrate cursor streams via an id pre-pass The album, artist and playlist GetCursor built their own select and never called hydrateArtwork, so every Jellyfin list endpoint (all six stream via GetCursor) emitted entity-id image tags and no blurhash. Only GetAll hydrated, which is why Subsonic and the native API were unaffected. Each cursor now resolves its ordered/filtered/paginated id set with the cheap id-only GetAllIDs query, then streams those ids in chunks through the repo's existing GetAll, which already hydrates and applies the full select. Max/Offset are consumed by the pre-pass alone; the chunk query carries only the caller's filters, Sort and Order. This also removes a pre-existing deep-pagination cost: keeping OFFSET out of the joined query makes the pre-pass a covering index scan instead of paying the library and annotation joins for every skipped row. Benchmarked on a synthetic 100k-album DB with the real schema, page=500 at offset 90,000: 3.9ms via the id pre-pass, 52.5ms for the current shape, 192.7ms for a naive join. An unpaginated full stream costs ~24% more, which is the trade. GetAllIDs gains the annotation join whenever the caller's filters or sort reference an annotation column (same gate CountAll uses), otherwise Filters=IsFavorite and SortBy=PlayCount would fail in the pre-pass. The playlist pre-pass repeats GetAll's columns so ORDER BY keeps resolving to playlist.name rather than the joined user.name. * fix(jellyfin): hydrate artwork on the song cursor Jellyfin's listSongs streamed media files via GetCursor, which never hydrates artwork, so songs emitted entity-id image tags and no blurhash. media_file now uses the same id pre-pass as the other three cursors (album/artist/playlist), for consistency, but on a separate method, GetCursorWithArtwork: GetCursor itself must stay untouched, since it's also the scanner's hot path and the scanner never reads artwork. Measured on 1,000,000 tracks, the pre-pass over all ids costs +41.8 MB heap and +298 ms versus GetCursor's bounded +0.0 MB. The Jellyfin path is paginated, though, so in practice it only ever pre-passes a page's worth of ids, not the full library, and doesn't pay that cost. * feat(jellyfin): emit a song's own cover art when it differs from the album's Real Jellyfin fills ImageTags from each item's own images before falling back to the parent album, and Finamp checks imageTags.Primary before AlbumId. Our mapper read only the album's image, so a track with distinct embedded art silently showed the album cover. Emit exactly one entry under ImageBlurHashes.Primary: Go marshals map[string]string in sorted key order rather than insertion order, so a second entry could pair the wrong blurhash with the image imageId resolves to, and Finamp pins that pairing in its cache for 365 days. * test(persistence): scope the GetCursorWithArtwork full-stream spec to tie-free ids The fixture has title ties (e.g. three "Antenna" tracks), so the unscoped positional comparison against GetAll only passed because SQLite's tie order happened to coincide between the full scan and the pre-pass's id IN (...) fetch. Scope it to onlySongs like the sibling ordering specs already do. * refactor(artwork): route song own-art through primaryImageTag; align chunk size Cleanups surfaced by /simplify: the song mapper's own-art branch reimplemented primaryImageTag's tag+blurhash-map construction (and its one-entry invariant) — route it through the helper so that invariant lives in one place. Tie artworkChunkSize to a whole multiple of artworkBatchSize so a cursor page re-chunks into even hydration batches. Hoist a duplicated imageLoading && blurHash boolean in the album grid. * fix(ui): serve the placeholder for known-absent art instead of a broken icon getCoverArtUrl returned '' for an imageAbsent record, so <img src={undefined}> rendered as the browser's broken-image icon on every absent cover. The server already serves a proper placeholder for absent art, so build the url and let it render. * feat(ui): show the blurhash as the loading placeholder across cover surfaces Add a shared CoverImage component (useImageUrl blob cache + blurhash + fade) and render the blurhash while a cover loads on the list thumbnails (CoverArtAvatar, radio) and the artist/album/playlist detail pages. The detail pages now go through CoverImage instead of a plain CardMedia, so their images come from the in-memory blob cache and survive React remounts without re-fetching. BlurHashCanvas gains an optional style prop. * refactor(ui): unify list cover surfaces onto the shared CoverImage component Route the album grid, CoverArtAvatar (artist/playlist lists) and the radio list's cover field through CoverImage instead of each carrying its own useImageUrl + blurhash-overlay wiring. CoverImage gains a default object-fit: cover. Radio keeps its uploaded-image gate and the generic radio placeholder for stations with no art. * fix(ui): address CoverImage review findings Restructure CoverImage so the size/shape lives on the root and the blurhash + image are absolute fills: the <img> mounts only once its blob is ready, so an unresolved cover never flashes a broken <img>. Add a fit prop (default cover) so album/playlist detail keep their letterbox instead of being cropped by a hardcoded object-fit. Remove the orphaned coverLoading styles and an unused subsonic import; add a CoverImage unit test. * perf(ui): only refetch already-loaded records on SSE refresh The artwork worker broadcasts a RefreshResource event per resolved chunk, carrying every id in the chunk. useResourceRefresh was doing a getMany for all of them, so any open list/detail page fetched hundreds of artists it was not displaying. Filter the event ids to records already in the store; the rest load fresh (with their new artwork) when navigated to. * refactor(artwork): scale worker concurrency with CPU count ArtworkWorkerConcurrency now defaults to max(2, NumCPU()/2) instead of a fixed 4, mirroring MaxOpenConns: local resolution scales with the host but stays at half the SQLite pool so it never starves the scanner/UI. External RPS stays a fixed 2 — it gates third-party API calls and is bounded by their tolerance, not the host, so it must not scale with CPUs. Also drop the DevArtworkWorkerConcurrency/DevArtworkExternalRPS deprecated aliases: those names were never released, so there is nothing to migrate. * feat(artwork): re-queue an absent cover when its page is viewed serveEntity now schedules a Bump recheck for an entity whose art was recorded absent, so viewing a missing cover re-triggers resolution (e.g. after an external source that was down during the scan comes back), matching the request-time bump that already covers never-resolved entities. Throttled by attempted_at against requestRecheckAge (1h) so repeatedly opening a genuinely-absent page can't hammer external services. EnqueueBump preserves an existing failed-state backoff via MAX(priority,...) and inserts a fresh, immediately-eligible recheck for a settled-absent row (whose queue row was already deleted). * refactor(artwork): move ImageUploadService to artwork.Uploader Relocate the image-upload service from core to core/artwork as artwork.Uploader, co-locating it with the resolver/worker/serving that own the artwork state it invalidates. MaxImageUploadSize moves too — its only callers are the two image-upload handlers — which lets core/image_upload.go be deleted entirely. Extract the shared "clear resolved state + re-queue at Bump" invalidation into artwork.Refresh and fold nativeapi's refreshArtwork handler onto it, removing the duplicated DeleteForItem+Enqueue block that had drifted into three places. The wire provider moves from core's set to artwork's; the playlists.ImageUploadService binding moves to the top-level injector so core/artwork stays unaware of playlists. Behavior is unchanged. * refactor(artwork): thread model.Kind through the artwork API Entity-level artwork queries now take a typed model.Kind instead of a bare prefix string. GetItemArtwork, DeleteForItem(s), GetInfoForItems, EnqueueStaleAbsent, hydrateItemImages, enqueueBackfillKind and artwork.Refresh convert to the prefix string only at the two real boundaries: the SQL item_kind column (kind.Prefix() inside each repo) and external string inputs (a new model.ParseKind for the nativeapi URL param, which also validates it). The Backfill/stale-absent kind slices, the resolve.go dispatch switch, and the kind→resource / kind→table lookup maps now use the Kind vars directly. The queue lifecycle methods (MarkFailed/Delete*) keep string kinds — they operate on a dequeued item's raw ItemKind column, which stays a string field, always populated via kind.Prefix(). Removes every bare "al"/"ar"/… prefix literal from non-test code (27 -> 0); behavior is unchanged. * tune(artwork): drop backoff base from 5m to 15s The exponential retry (base × 4^attempts, cap 48h) started at 5 minutes, so a single transient failure — a timeout under load, an external blip — parked a cover for 5 minutes even though a retry seconds later would have resolved it. Start at 15s instead: transient failures recover almost immediately (15s → 1m → 4m → 16m …), while persistent failures still escalate to the 48h cap (now at the 8th attempt instead of the 5th). * tune(artwork): 5s backoff base + 12h give-up, drop the cap Retry backoff now starts at 5s (was 15s) so a transient failure recovers on essentially the next drain, and jitter widens to ±40% so a wave of correlated failures doesn't re-clump into one poll. Add a 12h give-up budget measured from enqueued_at: once the next backoff would land past it, the worker stops retrying instead of grinding at a cap forever. A bare failure settles absent (handed to the 24h stale-absent sweep, and still recoverable on a page view); a found-stale keeps its already-served art. The budget bounds the tail, so the separate 48h backoffCap is removed. * fix(lastfm): match album.getInfo on name+artist only, not MBID 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. * fix(lastfm): return agents.ErrNotFound on error 6 (not found) 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. * feat(artwork): log external image-lookup failures at debug 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. * fix(artwork): treat a 404/410 image URL as not-found, not a transient fault 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. * feat(artwork): make artwork re-resolution targeted, not blunt 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. * test(artwork): restore resolution edge-case e2e coverage 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. * fix(artwork): run disc resolution for single-disc albums too 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. * fix(artwork): register the GIF decoder in core/artwork 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. * fix(artwork): never record absent after a local I/O failure 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. * fix(artwork): keep served art when the retry budget runs out 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. * fix(artwork): restart the retry budget on re-enqueue 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. * fix(artwork): keep not-found distinct from artwork-absent 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. * fix(artwork): make the prune sweep cancellable 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. * fix(artwork): hydrate the tracks reached through a playlist 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. * feat(ui): cross-fade the cover over its blurhash 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. * fix(ui): retire the blurhash on a timer, not transitionend 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. * fix(artwork): stop advertising a hash for bytes that won't be served 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. * perf(artwork): precache from the bytes just acquired 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. * perf(artwork): keep the worker pool fed across a drain 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. * perf(artwork): drain local and external artwork in separate pools 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. * fix(artwork): treat an unreadable upload as a failure, not a miss 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. * fix(artwork): make the pool-split worker test race-clean 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. * perf(artwork): stop reading and hashing disc art on every request 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. * fix(artwork): key disc art on folder image changes, not just the album 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. * fix(artwork): return undispatched items when a drain is cancelled 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. * fix(jellyfin): make a track's own cover reachable for Jellyfin clients 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. * fix(artwork): treat an unreadable artist-folder image as a failure 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. * fix(artwork): give full-size disc art a real ETag 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. * test(artwork): inject the unreadable source instead of chmod 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. * fix(artwork): compare the pixel cap without multiplying 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. * refactor(ui): rename cover artwork components * fix(ui): remove Artwork rendering gate Signed-off-by: Deluan <deluan@navidrome.org> * fix(cache): re-fetch when a cache entry outlives its data file 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. * fix(artwork): log why a sized cover fell back to the placeholder 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. * fix(artwork): precache the cover variant the UI actually requests 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. * fix(cache): give a re-created cache entry its own file 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. * fix(cache): keep the in-place truncate on windows 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. * fix(artwork): shape the blurhash placeholder to the artwork's aspect ratio 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. * feat(jellyfin): expose PrimaryImageAspectRatio 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. * refactor(model): give ItemImage an AspectRatio method 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. * fix(ui): anchor detail pages to the top when opened from a list 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. * fix(ui): stop the Random album grid collapsing on every keystroke 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. * refactor(artwork): move fingerprint property key const to `consts` package Signed-off-by: Deluan <deluan@navidrome.org> * refactor(tests): enhance database handling with resettable tables and truncation Signed-off-by: Deluan <deluan@navidrome.org> * refactor(artwork): narrow the prune lock and drop redundant in-flight tracking 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. * fix(artwork): never settle absent for a kind no recheck job revisits 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. * refactor(artwork): remove dead plumbing from the serving path 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. * refactor(artwork): drop queue repository methods only tests called 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. * test(artwork): pin that a request never fetches or samples album art 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. * refactor(artwork): give the resolver a receiver and one capability field 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. * refactor(artwork): funnel every served representation through one path 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. * style(artwork): use one log prefix, spelled the way the codebase does 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. * refactor(artwork): give workerDeps only what the processor uses 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. * refactor(artwork): make acquisition a processor with its own receiver 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. * refactor(artwork): collect the external gate contract in one file 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. * refactor(artwork): put the whole playlist grid in playlist_cover.go 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. * refactor(artwork): move resizedItem next to the interface it implements 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. * refactor(artwork): fold Refresh into housekeeping 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. * refactor(artwork): accumulate priority-chain state in one place 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. * refactor(artwork): expose housekeeping through the Worker 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. * refactor(artwork): name the service for its domain, not its role 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. * docs(artwork): give each Worker housekeeping method its own godoc 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. * refactor(artwork): move fakeFolderRepo to artwork_suite_test.go Signed-off-by: Deluan <deluan@navidrome.org> * refactor(artwork): unexport artwork.HashImage function * fix(log): stop ShortDur eating significant trailing zeros TrimSuffix(s, "0s") was meant to turn "4h0m0s" into "4h", but it strips any trailing "0s"/"0m" -- so 10s logged as "1", 20s as "2", 1m30s as "1m3", 2h30m as "2h3", and a zero duration as the empty string. Every elapsed/duration field in the app was affected. The suffix now has to include the preceding unit, so only a whole zero-valued component is dropped. The existing table only covered values that dodge the bug (4m, 4h, 4m3s); added the ones that don't. * feat(artwork): prefix every log message and time the slow steps 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. * refactor(artwork): log decoded dimensions as fields, not a formatted string 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. * fix(subsonic): stop serving artwork for a deleted radio artworkAccessible checked every kind except radio, which fell through to the default "no per-user access control" branch and returned true without a lookup. Radio deletion removes only the entity row -- item_artwork and the uploaded file survive until the next prune -- so the old ra- id kept serving the removed radio's image for up to a day. This is a regression against master, where newRadioArtworkReader loaded the radio first and returned its error, so deletion took effect at once. Radios stay globally visible; only existence is checked. CreateMockedRadioRepo left Data nil, so its own Put panicked on first use; initialized it. Reported by Codex on #5847. * fix(artwork): stop serving artwork for entities that no longer exist 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. * fix(playlist): rebuild the generated cover only when the tracks change The enqueue sat in refreshCounters, which Put also reaches for an ordinary metadata update, so renaming a playlist or editing its comment re-resolved the cover. The 2x2 grid samples albums with random(), so that silently handed the playlist a different cover for an edit that touched no tracks -- and refetched remote artwork to do it. It now happens where the track set actually changes: addTracks (which Put-with-tracks and updatePlaylist both funnel through) and renumber (reached from removeOrphans). Creation still enqueues even with no tracks, since an imported m3u can carry an ExternalImageURL. Reported by Codex on #5847. * 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. * test(artwork): make the artists-first backfill assertion non-vacuous 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. * refactor: use stdlib slices/maps and utils helpers in artwork code 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). * refactor(persistence): collapse the four hydrateArtwork copies into one generic album/artist/playlist/radio each carried the same eight lines, differing only in element type and model.Kind. hydrateItems takes a ref callback yielding an item's id and the ItemImage to fill, which is all that varied. The len()==0 guards drop out: hydrateItemImages already short-circuits an empty id list, and both loops are no-ops on an empty slice. applyItemImage stays as-is; hydrateMediaFileArtwork and its own specs still use it. * perf(scanner): stop reprocessing album and artist artwork on every full scan 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. * refactor(artwork): derive blurhash components inside Encode 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. * refactor(ui): replace the blurhash package with a local decoder The UI pulled in the `blurhash` dependency for one function, `decode`, called from a single component. The decoder is ~80 lines of well-specified arithmetic, so carrying a dependency for it costs more in supply chain and bundle than it saves. Equivalence was proven against the package before removing it: 84 hashes — three real ones plus every component count from 1x1 to 9x9 — decoded at six sizes, compared byte for byte, plus parity on which malformed inputs throw. Those pixel values are now pinned in the spec, so drift from the reference algorithm fails. The punch parameter is dropped rather than reproduced: no caller passes one, and the package applies `punch | 1`, which silently turns a punch of 2 into 3. * refactor: simplify the artwork enqueue and blurhash paths 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. * fix(artwork): refresh songs when their album's artwork changes A track with no art of its own is served its album's, so hydration copies the album's hash onto the track record. When an album resolution changed, the worker broadcast only an `album` refresh, leaving song and now-playing surfaces holding the previous hash-suffixed URL until something else refetched them. Pair the album refresh with a song one. The dependent id list is unbounded — an album has arbitrarily many tracks and a drained batch arbitrarily many albums — so this refreshes the resource as a whole via the protocol's existing wildcard rather than enumerating ids. Reported by Codex on #5847. * feat(persistence): log SQLite result codes on failed statements SQLite reuses one message for errors that need different responses: "database is locked" is both SQLITE_BUSY, which busy_timeout retries, and SQLITE_BUSY_SNAPSHOT, which it can never retry because the transaction's read snapshot is already stale. Reading only the message, the two are indistinguishable, and a lock error seen in the wild could not be diagnosed without guessing which one it was. Add db.ErrorCodes to unwrap a sqlite3.Error and report its result and extended result codes, and include them in the SQL error log. The helper lives in db because that package already owns the driver, so persistence does not need to import it. Constraint, readonly and disk-full errors share messages the same way, so this applies to every failed statement, not just locks. * fix(ui): keep the Random grid blank while a refresh re-rolls Refresh bumps the list version, which changes the random seed and remounts the grid. useRollChanged tracked the seed on screen in a ref inside the grid, so the remount started it empty and adopted the new seed on the first render, while the refetch had not begun and the store still held the previous roll. The grid painted the old albums for the length of the request and swapped when the new roll arrived. Move the ref up to AlbumList, which a refresh does not remount, and pass it to the grid and the pagination. The seed on screen then survives the remount, so a refresh reads as a re-roll and blanks until the new roll lands. A search keystroke keeps the seed and still leaves the grid in place. * fix(ui): keep the album grid working outside the Random list Hoisting the shown-seed ref into AlbumList made the prop mandatory in practice: ArtistShow renders the same grid through ReferenceManyField and passes no seed tracking, so useRollChanged dereferenced undefined and the artist page died with "Cannot read properties of undefined (reading 'current')". Own a ref in the grid when none is passed. A caller with no roll to track then behaves as it did before, while the Random list keeps the ref that has to outlive the refresh remount. * refactor(config): make the artwork tuning options dev flags ArtworkWorkerConcurrency and ArtworkExternalMaxRPS become DevArtworkWorkerConcurrency and DevArtworkExternalMaxRPS, joining the other DevArtwork* flags. Their defaults should not need tuning, so they do not belong in the documented, user-facing option set. * refactor(artwork): name the hashed image store folder for how it is addressed The content-addressed store sat in artwork/store/, which did not distinguish it from the artist/, playlist/ and radio/ upload folders beside it — those hold artwork too. It is now artwork/hashed/, naming the one property that sets it apart, and the path comes from a consts entry rather than a bare literal, matching how the sibling folders are built. Deliberately not under cache/: that folder holds resizes that rebuild offline from a local source, while this one holds the only local copy of externally fetched images, whose re-fetch depends on a third party still serving them and whose bytes back the stored blurhash and dimensions. No migration: anything left in the old artwork/store/ is orphaned and re-resolved into the new location. * refactor(artwork): call the album e2e helpers by their real names album_test.go aliased expectAlbumFolderCover and expectAlbumAbsent to shorter local names, which cost a lookup to resolve and hid the prefix that distinguishes them from the artist and playlist helpers. * test(artwork): cover the album-root and artist-folder path arithmetic The unit specs for these helpers lived in the readers #5856 patched, both deleted here, leaving the album-root promotion and the artist-folder climb covered only end-to-end. A layout can show which image won but not why, so these pin the parts behind it: that the parent is fetched only when it could qualify as an album root, that a failed fetch propagates or degrades, and that commonDir keeps a shared name fragment from reading as a shared folder. Each spec was checked against a mutant: removing the library-root guard, the other-album audio check, the single-folder short circuit, the single-root climb, or commonDir's separator all turn one red. Master's remaining specs were dropped as redundant — they cover sources this branch already exercises under different names. * fix(ui): stop artwork refreshes reloading the whole page Resolving an album broadcast song:["*"], because a track with no art of its own is served its album's and the dependent ids are unbounded server-side. useResourceRefresh checked for that wildcard across every resource in the payload, not just the ones a component shows, so the album grid called refresh() — a full page reload — for every drained batch. Upgrading a large library reloaded the grid thousands of times. The client knows what the server cannot: which tracks are loaded, and their albumId. So the fan-out moves there, the wildcard check is scoped to watched resources, and the backend now names only what it resolved. The playlist views watch playlistTrack too: their rows carry albumId but are keyed by playlist entry, so a song refresh never reached them — previously masked by the wildcard's page reload. Signed-off-by: Deluan <deluan@navidrome.org> * fix(ui): stop the album grid blanking its covers on refresh Cover takes its height from react-measure, which reports nothing until it re-measures. That was harmless while the cover class sat on the img itself, which has intrinsic size; it now sits on the Artwork root, whose img fills it absolutely and so lends no height. A refresh remounts the tiles, and for the frame before measurement lands the box collapsed to zero and every cover vanished. Give the box its own aspect ratio as a floor. The measured height still wins once it arrives. * refactor(artwork): drop the unused Bump/wake path from the worker Worker.Bump had no production callers. Every real bump writes the queue row through the repository instead: artwork.Refresh (the refresh endpoint and the uploader), service.enqueue (read-through and stale-absent views), and radio_repository. None of them wake a pool. Bump was also the only sender to drainPool.wake, so the select case it fed was unreachable outside tests, and runPool read as if a bump were picked up promptly when it always waited for the 5s poll. The e2e and unit suites used Bump only as a driver, never as the subject, so they now enqueue with EnqueueBump and exercise the path production takes. EnqueueBump rather than Refresh because Refresh also clears the state row, which would change semantics for the re-bump-after-state and dedup specs. Both harnesses start Run after enqueueing, so a fresh pool drains on its first iteration and the wake never affected them: core/artwork/e2e averaged 3.26s before and 3.30s after over 5 runs, within noise. Signed-off-by: Deluan <deluan@navidrome.org> * refactor(artwork): unexport what nothing outside the package calls PlaceholderFor had no callers at all. Its comment offered it to callers that must not consult persisted state, but no such caller was ever written, so it is removed rather than unexported. entityExists and encode83 are only reached from inside their own packages; their tests are package-internal (artwork) or never touch them (blurhash_test). ImageStore stays exported: server/subsonic/e2e constructs one to wire up the worker, and that suite cannot move into package artwork. Signed-off-by: Deluan <deluan@navidrome.org> * refactor(persistence): name the backoff-preserving enqueue for what it does EnqueueBump was named for a priority, but its behaviour is preserving an existing row's retry_at; the priority comes from the item. Its one caller passes ArtworkPriorityScan, which read like a bug at the call site and is not. The three DO NOTHING inserts each repeated the INSERT prefix, the column list and the conflict clause. They now share insertIfNotQueued, with the CTE that EnqueueIfMissing needs passed as a prefix, and the column list lives in one slice used by both squirrel and the raw SQL. EnqueueIfMissing had no test against real SQL, only a mock mirroring the anti-join by hand, so the rewritten statement had nothing verifying it. Two specs now cover it: items with a state row are skipped, and an already-queued row keeps its priority. Signed-off-by: Deluan <deluan@navidrome.org> * test(thumbhash): vendor the reference and generate golden vectors * test(thumbhash): add a faithful reference port as the differential oracle * fix(thumbhash): re-condition alpha fixture and assert solid.png header alpha.png varied only alpha over a constant color, so compositing atop the average canceled L/P/Q exactly like solid.png, leaving it just as float-noise unstable and silently dropping alpha-path coverage. Vary R/G/B with position too so alpha.png carries real signal and reproduces byte-exactly, then replace the blanket solid.png skip with a precise assertion on its well-conditioned header bytes and quantized-zero scales. Also apply a range-over-int modernizer hint in reference_test.go. * test(thumbhash): dither the fixtures so no DCT coefficient is degenerate The gradient fixtures are smooth analytic ramps, and alpha.png's alpha channel varied along x only. Both make whole families of DCT coefficients mathematically zero: 13 of alpha.png's 38 AC nibbles, and at least one in every other ramp fixture. A zero coefficient normalizes to exactly the 0.5 midpoint, i.e. 15*f = 7.5, so which of nibble 7 or 8 it quantizes to is decided by ~1e-16 of float rounding noise. Those nibbles are therefore unstable across any two summation orders -- the vendored JS and the Go port already disagreed on solid.png for this reason -- and they carry no signal, so a bug that transposed two of them would be invisible. A deterministic +/-4 dither, plus an alpha ramp that varies in x and y, leaves every coefficient at least 4.8e-5 from the tie boundary: ~1e9 times the observed inter- implementation noise. solid.png and tiny.png regenerate byte-identical and keep their existing carve-outs. * feat(thumbhash): add a two-pass separable ThumbHash encoder Encode replaces the reference's four w*h scratch arrays (~320 KB at 100x100) and its 40 re-reads of every pixel with a single pixel pass that folds each row into per-frequency sums, then a second fold over rows. The transform drops from O(terms * pixels) to O(nx * pixels + terms * h), nx <= 7. Verified against the vendored JS goldens and, differentially, against the literal Go port on 500 randomized images. On the fixtures the two implementations' AC coefficients agree to 8e-15; the separable inner loop reassociates the additions, so exact agreement holds only where a coefficient is not sitting on a quantization tie, which is why the fixtures are now dithered. * test(thumbhash): fuzz the opaque path and drop an ill-conditioned golden The 500-image randomized differential filled every byte randomly, so hasAlpha (avgA < w*h) was true for all 500 images: the 7x7 no-alpha layout, terms(7,7), nx=7 and the `if hasAlpha` false branch were never fuzzed. Force full opacity on alternating iterations, which splits the run 250/250 with the seed and iteration count unchanged. All 500 still match the reference port. tiny.png is 1x1, so it has no non-zero AC content: 12 of its 37 AC coefficients sit exactly on the round(15*f) = .5 tie and 24 are within 1e-12. It passed only because a single pixel admits no summation reassociation. Give it the same header-only carve-out solid.png already had, in both test files. The four well-conditioned fixtures keep strict full byte equality. Also document the divergence class on Encode itself rather than only in test comments, add a sub-image regression spec, and use the max builtin over math.Max. The toNRGBA Rect.Min gate turns out to protect nothing — SubImage re-slices Pix so Pix[0] is the Rect.Min pixel and the loops read it correctly either way — so its comment, which claimed the opposite, is corrected. The gate is kept for now; removing it is a separate call. * test(thumbhash): benchmark against blurhash at the pipeline input size * feat(artwork): persist a thumbhash alongside the blurhash The artwork table is content-addressed, so this costs one row per unique image rather than per entity. Measured on a 25k-image library: +0.6MB on a 727MB DB. The column is added to the existing add_artwork_tables migration rather than a new one, since #5847 has not been released and the table it extends ships in that same PR. * feat(artwork): hydrate and expose thumbHash on every entity ItemArtworkInfo.Image() is the single projection every hydration branch goes through, so adding the field there covers albums, artists, playlists and both the own-art and inherited media-file branches. * feat(artwork): encode blurhash and thumbhash from one 100px thumbnail thumbnailSize drops 128 -> 100 so a single CatmullRom scale feeds both encoders; thumbhash hard-rejects anything larger and a second downscale would cost more than shrinking the shared one. decodeArtwork on a 1000x1000 JPEG, before -> after: 15.53ms -> 15.45ms/op, 5878122 -> 5014796 B/op, 37 -> 75 allocs/op Time is a wash because the JPEG decode dominates; the 863KB drop is the smaller thumbnail more than paying for thumbhash's added work. This rewrites every blurhash value, so it must land before #5847 reaches a release: Finamp keys its cover cache and download dedup on that value. * feat(ui): use thumbhash for the cover loading placeholder Adds a local decoder rather than the thumbhash npm package, matching the local blurhash decoder it replaces. Pixels are pinned against evanw/thumbhash's reference decoder via the vendored copy already in the encoder's testdata. Unlike a blurhash, a thumbhash carries its own approximate aspect, so a record with no dimensions now falls back to that instead of to a square. The blurHash API field stays: Jellyfin clients and third-party native clients still consume it. * perf(artwork): hand both hash encoders one NRGBA thumbnail makeThumbnail emitted a premultiplied *image.RGBA, so thumbhash allocated and un-premultiplied a full copy on every image while blurhash paid nothing. It now emits *image.NRGBA, which thumbhash wants as-is, and blurhash reads that type directly, premultiplying per pixel to keep its output identical. thumbhash.Encode at the pipeline's 100x100 input: 134200 -> 95300 ns/op, 56188 -> 15163 B/op, 37 -> 35 allocs/op decodeArtwork on a 1000x1000 JPEG: 5014796 -> 4973800 B/op, exactly the conversion that is gone The scaler costs the same into either destination (7.33ms vs 7.34ms measured), so no time is traded for this. * test(artwork): drop the decodeArtwork benchmark It existed to capture the before/after baseline for the 128->100 thumbnail change, which it has served. As an ongoing guard it is misleading: a 1000x1000 JPEG decode is ~97% of the 15ms it measures, so the two encoders it would be reached for are ~2.6% of the signal. It reported a phantom 2.8% regression for the NRGBA thumbnail change that isolating the scaler disproved. The per-encoder benchmarks in blurhash/ and thumbhash/ cover the part that actually changes. * test(artwork): benchmark both hash encoders from one file benchImage and BenchmarkEncodeAtInputSize existed in both encoder packages, and blurhash's copy had drifted into a third inline duplicate once both benches moved to NRGBA input. The head-to-head now lives in core/artwork, which already imports both encoders and can pin the input to the real thumbnailSize constant. The gradient builder moves to tests, which both packages already import via their suite bootstrap. thumbhash keeps a shipped-vs-reference benchmark, since referenceEncode is test-only and cannot be reached from core/artwork. * test(thumbhash): drop the shipped-vs-reference benchmark The two-pass rewrite's advantage over the naive port is already recorded in its commit message; carrying the benchmark to re-derive it has no ongoing use. reference_test.go stays: besides the benchmark baseline it holds the differential oracle the golden-vector and randomised specs assert against. * test(thumbhash): assert against reference goldens, drop the Go port The reference port existed to be a differential oracle, but its packing half was a verbatim copy of production pack(), so it was not as independent as it looked. Only the 500-image randomised spec needed it; the six PNG fixtures cannot reach random sizes, aspects, or both coefficient layouts. gen_generated.mjs now emits 300 vectors from evanw's actual JS. Their pixels are a pure function of their index, so Go rebuilds them byte-for-byte and only the hashes are committed (16KB). The oracle is now the reference itself rather than a hand transcription of it. Also from the cleanup pass: - maxCX/maxCY scanned every term to recover a bound that is just the widest coefficient region - tests.GradientImage duplicated generateGradientImage three files away - BenchmarkHashEncodersAtInputSize was also BenchmarkHashEncoders/*/100x100 - processor had two internal test files with no rule for which gets a new spec - three blurhash specs differing only by alpha became a DescribeTable - the shared mock's GetInfoForItems projection had drifted from the real query * refactor(model): keep the blurhash off native JSON and fold its specs Nothing on the native API consumes blurHash: the web UI reads thumbHash, and Jellyfin's mappers take the Go field directly rather than through this serialization. It was shipping ~40 unused bytes on every row of every list response, on a surface that accretes clients once published. The JSON specs were five marshal-and-check-keys blocks over the same struct; they collapse to one populated case, one bare case, and a guard that the blurhash stays off the wire. * refactor(persistence): simplify the artwork dangling-row purge helper purgeDangling took a bound executeSQL method value and a table name only because its two callers live on unrelated types. Neither parameter was load-bearing: executeSQL is a value receiver on sqlRepository that never reads tableName, and both callers already hold an sqlRepository whose tableName is the table being purged. Taking that struct collapses both parameters into the receiver. itemArtworkSQL wrapped sqlRepository without adding a single method, so the items field becomes a plain sqlRepository. danglingItemArtworkKinds is also read by EnqueueAllMissing, which enqueues entities that have no artwork row yet - neither a purge nor anything specific to item_artwork. Renamed to artworkOwnerTables to describe both of its uses. * fix(artwork): keep sweeping past a store file that cannot be removed Sweep returned the os.Remove error straight out of WalkDir, so a single unremovable file abandoned the rest of the walk. Every later orphan, stale mime variant and abandoned temp file then survived until the next prune, which would abort at the same place. Warn and carry on instead. The orphan-file loop in prune already had this resilience; Sweep is where it belongs, since it is the only step that reaches stray files and superseded variants. * refactor(artwork): let the sweep reclaim orphan files instead of prune Deleting unreferenced artwork rows took five round trips: snapshot the orphan hashes, fetch their mimes, delete, re-fetch to see which rows actually went, then remove each file. The last four exist only to work out which files to delete - but the sweep that runs moments later already derives exactly that from the database, since GetAllMimes is read after the delete and Sweep removes any file whose hash has no row, under the same mtime guard. The sweep is also the more thorough of the two: it reclaims stale mime variants of an orphan hash, which the per-hash removal never looked at. Delete the rows in one predicate-only statement and let the sweep follow. The orphan predicate now runs once per prune instead of once for the snapshot plus once per 200-hash delete chunk, and with no hash list to bind there is nothing left to chunk. GetOrphanHashes and GetImages had no other caller and are gone. Dropping them also removes the mock's OrphanHashes lever, which let a spec declare a hash orphaned while item_artwork still referenced it - a state the SQL repository could never produce, since both predicates were always identical. * refactor(model): name the artwork mime lookup for what it is keyed by GetAllMimes reads as a collection of mime types; it returns a hash -> mime index over every stored artwork. GetMimeByHash names the key, matching the convention the other map-returning repository methods already follow (CountBySuffix, CountByClient). * refactor(artwork): split the repository's deletes from its purges by name Six removal methods across the two artwork repositories used Delete and Purge interchangeably, so neither prefix told you anything. There is a real distinction underneath: Delete methods are caller-directed and take the rows to remove, while Purge methods are garbage collection - the repository finds rows whose referent is gone by joining against the owning table, and returns a count because the caller cannot know one. The signatures already followed that split; only the names did not. DeleteOrphans was the odd one out, taking a grace cutoff rather than a target and returning a count, so it becomes PurgeOrphans. PurgeDanglingItemArtwork loses the Artwork it repeats from its own interface and keeps the Items that says which of that interface's two tables it means. DeleteForItem was DeleteForItems with one id - squirrel emits = for a scalar and IN for a slice against the same predicate - so its one caller passes a slice instead. Dangling and Orphan stay distinct: dangling is a row whose entity is gone, orphan is an image no state row references. * refactor(artwork): drop the orphaned store remover, aggregate sweep failures Review follow-ups to the prune rework. ImageStore.Remove lost its only production caller when the sweep took over orphan-file reclamation. It carried its own copy of the mtime guard that now lives solely in Sweep, so the two could drift, and it stood as an invitation to grow a second reclaim path. Its specs either duplicated Sweep's own coverage or tested nothing else, so they go with it; the one unrelated test that used it to delete a file calls os.Remove directly. Sweep warned once per file it could not remove, which is fine for a stray permission bit and useless for a store mounted read-only - a large library would emit one line per aged file on every prune, and prune would still report success. Count the failures instead and warn once, with the count and the last error, so a systemic failure is legible without drowning the log. Also: the orphan log line counts rows, not files, and now says so; and the blocked-removal spec asserts its two fixtures really do land in different shards, since the assertion is vacuous if they collide. * fix(ui): show the placeholder when a refresh swaps in an uncached cover A cover whose hash changed under a mounted grid went blank for the whole fetch, instead of falling back to its thumbhash. Artwork decided 'this blob was already cached, skip the placeholder' once at mount and never revisited it, so every later url for that record inherited the answer. The fix has to live in useImageUrl. imgUrl is state cleared in an effect, so on the render where url changes it still holds the previous url's blob - any caller inspecting it reads the new url as already cached. Only the hook can see cache.get(url) for the url actually being rendered, so it now reports fromCache and resyncs its state during render rather than one render later. Two shapes were tried and rejected against a live server before this one: a ref recomputed per url, which a render-phase resync discards without rolling back the ref, and the same tracking in Artwork state, which still samples imgUrl before React re-runs the component. Verified on a running instance with cover-art responses held open: the thumbhash now covers the whole window and stays under the image until the fade ends. * fix(ui): keep the placeholder for a cover whose fetch failed useImageUrl caches a failed fetch as an entry with no blob, so treating every cached entry as instantly painted suppressed the placeholder on remount: the cover became an empty box instead of falling back to its thumbhash. Only a cached blob counts as instant. Caught by Codex on the previous commit, which introduced fromCache with the looser predicate. * feat(artwork): capture each image's dominant colour A flat colour is the one placeholder a client can paint with no decoding at all, so it can cover the frames before a thumbhash canvas even renders. Extract it from the same 100px thumbnail the two hash encoders already share, and expose it as dominantColor on the native API. Dominant means presence, not salience: the largest cluster wins, so a white sleeve reports white. That is what a placeholder needs. An accent colour is the opposite question and is deliberately not answered here - it depends on the client's theme, and by the time a client needs one it has the cover pixels and can compute whatever suits it. Bins at 4 bits per channel, then merges perceptually-near bins in Oklab before picking the winner: a gradient splits across adjacent bins and would otherwise lose to a smaller flat region. Measured at 246us per image on the weakest target hardware we care about (Celeron N5105), against ~15.8 images/s for the pipeline around it. The column goes into the feature's original migration rather than a new one: computing it later would mean re-decoding every image in the library. * fix(artwork): ignore image candidates that cannot be fetched bestImageURL only rejected URLs that url.Parse itself refuses, which is almost nothing: relative paths and any scheme parse cleanly. A candidate like images/big.jpg or ftp://host/big.jpg therefore won on size, and since the function returns a single URL whose failure ends that agent's turn, the agent's valid smaller images were never tried. Two details made it easy to hit rather than theoretical. Size is frequently 0 for every candidate, and only a strictly larger one replaces the first, so an unfetchable entry in first position sticks. And these strings come straight from plugins, where a relative path is an ordinary mistake. Accept only absolute http/https URLs with a host. * revert(ui): move detail-page scroll handling to its own branch Reverts 40153bd43. Scroll position carrying over between pages is stock React Router behaviour rather than something this branch introduced, so the fix does not belong in an artwork PR that is otherwise ready for review. The work continues on fix/scroll-restoration, off master, where it grew into per-route restoration: a new page starts at the top and going back returns to where you were, which the scroll-to-top could not do. * fix(ui): fade cover art consistently, and shorten the fade to 150ms The <img> mounts with its blob URL already set, so onLoad often fires in the same frame the element is inserted. The opacity:0 start state never gets painted, the transition has no value to animate from, and the cover pops in instead of fading. Only tiles whose decode happened to straddle a paint boundary actually faded, which made a grid load look ragged: 4 to 6 of 18 covers faded, varying between runs. Deferring the decoded flag by two animation frames guarantees the hidden state is painted first. Measured on a live grid afterwards, 17 of 18 covers fade, the 18th having a cached blob and taking the intentional instant path, and the tail after the last byte arrives drops from ~440ms to ~163ms. The duration also drops from 500ms to 150ms. That 500ms mirrored master's fade, but master paints no placeholder behind the image and needs a slow blend to cover a blank tile. Here a thumbhash already fills the gap, so a long crossfade only delays the grid settling, measured at 145-190ms slower to full opacity than master. --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-08-09 15:03:27 -04:00
Deluan Quintão	85132240e0	feat(smartplaylist): per-playlist refreshDelay for stable daily/weekly playlists (#5790) ... * feat(utils): add ParseDuration/FormatDuration with day and week units * feat(criteria): add per-playlist refreshDelay to smart playlist rules * feat(smartplaylist): honor per-playlist refreshDelay in refresh gate * feat(subsonic): compute smart playlist validUntil from effective refresh delay * fix(playlists): reset smart playlist evaluation window when rules change via API * refactor(utils): flatten FormatDuration recursion, single-pass duration regex * fix(playlists): address PR review feedback - Reset EvaluatedAt to nil instead of zero-time on rules change and NSP re-import, so getPlaylist(s) never reports year-1 Changed/validUntil in the window between an edit and the next owner read - Parse negative d/w durations so they are rejected with the consistent "negative duration" error instead of "unknown unit" - Quote input in the negative-duration error, matching the parse error - Gate per-playlist RefreshDelay behind IsSmartPlaylist, matching its doc	2026-07-16 22:20:35 -04:00
Deluan Quintão	ca27335d06	feat(playlists): per-user starred/rating annotations (backend) (#5749) ... * feat(playlists): add average_rating column to playlist table * feat(playlists): store and read per-user starred/rating annotations * feat(playlists): clean up annotations when a playlist is deleted * feat(subsonic): route star/unstar of a playlist to the playlist repository * feat(subsonic): route setRating of a playlist to the playlist repository * test(subsonic): guard that playlist responses never expose annotations * fix(playlists): clean stale mis-typed annotations on upgrade; cover GetAll read-back * fix(playlists): scope annotation join by item_type and harden delete Address code-review findings on the playlist-annotations branch: - withAnnotation: add an item_type predicate to the LEFT JOIN so a mis-typed annotation row sharing an id can no longer leak into (or duplicate) another entity's read. Correct for every caller since each repo writes annotations with item_type = tableName. Regression test added. - migration: reclassify legacy media_file-typed rows for playlist ids to item_type='playlist' (instead of deleting them), preserving users' prior playlist star/rating; run before the average_rating backfill so those ratings are included. - playlist Delete: replace the per-request full-table cleanAnnotations() anti-join with a targeted, permission-safe (rows-affected gated), best-effort delete so a cleanup failure no longer misreports an already-committed delete as an error. - MockPlaylistRepo: implement GetAll/IncPlayCount/ReassignAnnotation to remove the dead All field and the nil-interface panic traps. - test: use slices.IndexFunc instead of a hand-rolled find loop. * feat(playlists): streamline playlist deletion by relying on annotation sweep * docs(playlists): trim comments in annotation migration and test Condense the verbose comments added in this branch per the project's comment-minimalism guideline, keeping only the non-obvious rationale. The migration's reclassify block is shortened while preserving the safety invariant (playlist and media_file ids never collide, so the item_type rewrite touches only mis-typed rows and cannot violate the unique key) and the ordering note. The redundant 'Populate average_rating' comment is dropped since the UPDATE is self-evident. The repository test's leakage comment is condensed to two lines. No code behavior changes. * refactor(subsonic): resolve setStar targets via GetEntityByID Replace setStar's Album/Artist/Playlist Exists probe chain with a single model.GetEntityByID lookup and a type switch, mirroring setRating. This removes three per-id existence queries and keeps the two annotation paths consistent. An id that resolves to no known entity is logged and skipped rather than filed as a spurious media_file annotation, and a lookup failure on one id no longer aborts the whole batch. Also drop a duplicate empty-ids guard. * refactor(playlists): drop no-op reclassify/backfill from migration The average_rating migration carried two data-fix UPDATEs that are no-ops on any real database: - The media_file->playlist reclassification only matches rows no released build ever created: playlists were never annotatable, so star/setRating of a playlist id was never written as item_type='playlist'. Any stray media_file-typed row for a playlist id is already removed by the media_file annotation GC sweep (item_id not in media_file). - The average_rating backfill runs before any item_type='playlist' row can exist, so it can only ever write the default 0. Going forward SetRating keeps average_rating current via updateAvgRating. Reduce the migration to the column add/drop. * refactor(persistence): bind annotation join params, derive idField from tableName Address PR review: use Squirrel parameter binding for item_type/user_id in the shared withAnnotation join instead of string concatenation, and pass r.tableName+".id" from selectPlaylist so the join field stays consistent with the surrounding r.tableName usage. * fix(subsonic): surface datastore errors in setStar instead of skipping Address PR review: setStar swallowed every GetEntityByID error and continued, so a real datastore failure would still commit the transaction and emit a refresh event as if the star succeeded. Skip only on model.ErrNotFound (an unknown id); return any other error so the request fails and rolls back. * test(subsonic): assert absent JSON keys instead of substring matches Address PR review: substring checks are brittle ("starred" matches "starredAt", "rating" matches "userRating"). Unmarshal the response and assert the annotation keys are absent. * fix(subsonic): skip refresh broadcast when a star request changes nothing Address PR review (Codex): once setStar began skipping unknown ids, a request containing only unresolvable ids left the RefreshResource empty, which SendMessage serializes as a {*:*} wildcard that forces every client to refresh. Only broadcast when at least one id was actually starred. * fix(db): rebase playlist average_rating migration timestamp past master The 20260708011823 migration predated the newest migration merged to master (20260712211040_add_primary_key...), which Goose would silently skip on already-upgraded databases. Rename it to a current timestamp so it applies in order.	2026-07-14 07:38:25 -04:00
Deluan Quintão	259c1a9484	feat(subsonic): add sonicSimilarity extension as plugin capability (#5419) ... * feat(plugins): add sonicSimilarity capability types Defines the SonicSimilarity plugin capability interface with GetSonicSimilarTracks and FindSonicPath methods, along with their request/response types. * feat(sonic): add core sonic similarity service Implements the Sonic service with HasProvider, GetSonicSimilarTracks, and FindSonicPath, delegating to the PluginLoader and using the Matcher for index-preserving library resolution. * test(sonic): add sonic service unit tests Covers HasProvider, GetSonicSimilarTracks, and FindSonicPath with mock plugin loader and provider, verifying error propagation and successful match resolution via the library matcher. * feat(matcher): add MatchSongsToLibraryMap for index-preserving matching Adds a new method alongside MatchSongsToLibrary that returns a map[int]MediaFile keyed by input song index rather than a flat slice, enabling callers to correlate similarity scores back to the original position in the results. * fix(sonic): check provider availability before MediaFile lookup Avoids unnecessary DB call when no plugin is available, and ensures the correct error path is tested. * feat(plugins): add sonic similarity adapter Adds SonicSimilarityAdapter implementing sonic.Provider, bridging the plugin system to the core sonic service via Extism plugin functions. Reuses existing songRefsToAgentSongs helper for SongRef conversion. * feat(plugins): add LoadSonicSimilarity to plugin manager Adds Manager.LoadSonicSimilarity method following the pattern of LoadLyricsProvider, enabling the core sonic service to load a SonicSimilarityAdapter from a named plugin. * feat(subsonic): add sonicMatch response type Add SonicMatch struct with Entry and Similarity fields, and a SonicMatches slice to the Subsonic response struct. These types support the OpenSubsonic sonicSimilarity extension for returning similarity-scored track results. * feat(subsonic): add getSonicSimilarTracks and findSonicPath handlers Add two new Subsonic API handlers for the sonicSimilarity OpenSubsonic extension: GetSonicSimilarTracks returns similarity-scored tracks similar to a given song, and FindSonicPath returns a path of tracks connecting two songs. Both handlers delegate to the sonic core service and map results to SonicMatch response types. * feat(subsonic): advertise sonicSimilarity extension when plugin available Update GetOpenSubsonicExtensions to conditionally include the sonicSimilarity extension only when a sonic similarity plugin provider is available. The nil guard ensures backward compatibility with tests that pass nil for the sonic field. Also update the existing test to pass the new nil parameter. * feat(subsonic): wire sonic similarity service into router Add the sonic.Sonic service to the Router struct and New() constructor, register the getSonicSimilarTracks and findSonicPath routes, and wire sonic.New and its PluginLoader binding into the Wire dependency injection graph. Update all existing test call sites to pass the new nil parameter. Regenerate wire_gen.go. * fix(e2e): add sonic parameter to subsonic.New call in e2e tests * test(subsonic): add sonicSimilarity extension advertisement tests Restructures the GetOpenSubsonicExtensions test into two contexts: one verifying the baseline 5 extensions are returned when no sonic similarity plugin is configured, and one verifying that the sonicSimilarity extension is advertised (making 6 total) when a plugin loader reports an available provider. Adds a mockSonicPluginLoader to satisfy the sonic.PluginLoader interface without requiring a real plugin. * feat(subsonic): add nil guard and e2e tests for sonic similarity endpoints Handlers return ErrorDataNotFound when no sonic service is configured, preventing nil panics. E2e tests verify both endpoints return proper error responses when no plugin is available. * fix(subsonic): return HTTP 404 when no sonic similarity plugin available Endpoints are always registered but return 404 when no provider is available, rather than a subsonic error code 70. * refactor: clean up sonic similarity code after review Extract shared helpers to reduce duplication across the sonic similarity implementation: loadAllMatches in matcher consolidates the 4-phase matching pipeline, songRefToAgentSong eliminates per-iteration slice allocation in the adapter, sonicMatchResponse deduplicates response building in handlers, and a package-level constant replaces raw capability name strings in core/sonic. * fix empty response shapes Signed-off-by: Deluan <deluan@navidrome.org> * test(plugins): add testdata plugin and e2e tests for sonic similarity Add a test-sonic-similarity WASM plugin that implements both GetSonicSimilarTracks and FindSonicPath via the generated sonicsimilarity PDK. The plugin returns deterministic test data with decreasing similarity scores and supports error injection via config. Adapter tests verify the full round-trip through the WASM plugin including error handling. Also includes regenerated PDK code from make gen. * docs: update README to include new capabilities and usage examples for plugins Signed-off-by: Deluan <deluan@navidrome.org> * test(e2e): enhance sonic similarity tests with additional scenarios and mock provider Signed-off-by: Deluan <deluan@navidrome.org> * fix: address PR review feedback for sonic similarity Fix incorrect field names in README documentation ({from, to} → {startSong, endSong}) and remove unnecessary XML serialization test from e2e suite since OpenSubsonic endpoints only use JSON. * refactor: rename Matcher methods for conciseness Rename MatchSongsToLibrary to MatchSongs and MatchSongsToLibraryMap to MatchSongsIndexed. The Matcher receiver already establishes the "to library" context, making that suffix redundant, and "Indexed" better describes the intent (preserving input ordering) than "Map" which describes the data structure. * refactor: standardize variable naming for media files in sonic path methods Signed-off-by: Deluan <deluan@navidrome.org> * refactor: simplify plugin loading by introducing adapter constructors Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-04-27 17:50:09 -04:00
Deluan	23f3556371	fix(subsonic): strip OpenSubsonic extensions from playlists for legacy clients ... buildOSPlaylist was the only OpenSubsonic builder function missing the LegacyClients guard, causing attributes like `validUntil` and `readonly` to appear in playlist XML responses for legacy clients like DSub2000. This caused a crash when DSub2000 tried to parse evaluated smart playlists containing the `validUntil` attribute.	2026-04-02 16:37:52 -04:00
Deluan Quintão	ae1e0ddb11	feat(subsonic): implement OpenSubsonic Transcoding extension (#4990) ... * feat(subsonic): implement transcode decision logic and codec handling for media files Signed-off-by: Deluan <deluan@navidrome.org> * fix(subsonic): update codec limitation structure and decision logic for improved clarity Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcoding): update bitrate handling to use kilobits per second (kbps) across transcode decision logic Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): simplify container alias handling in matchesContainer function Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcoding): enforce POST method for GetTranscodeDecision and handle non-POST requests Signed-off-by: Deluan <deluan@navidrome.org> * feat(transcoding): add enums for protocol, comparison operators, limitations, and codec profiles in transcode decision logic Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): streamline limitation checks and applyLimitation logic for improved readability and maintainability Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): replace strings.EqualFold with direct comparison for protocol and limitation checks Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): rename token methods to CreateTranscodeParams and ParseTranscodeParams for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): enhance logging for transcode decision process and client info conversion Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): rename TranscodeDecision to Decider and update related methods for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): enhance transcoding config lookup logic for audio codecs Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): enhance transcoding options with sample rate support and improve command handling Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): add bit depth support for audio transcoding and enhance related logic Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): enhance AAC command handling and support for audio channels in streaming Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): streamline transcoding logic by consolidating stream parameter handling and enhancing alias mapping Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): update default command handling and add codec support for transcoding Signed-off-by: Deluan <deluan@navidrome.org> * fix: implement noopDecider for transcoding decision handling in tests Signed-off-by: Deluan <deluan@navidrome.org> * fix: address review findings for OpenSubsonic transcoding PR Fix multiple issues identified during code review of the transcoding extension: add missing return after error in shared stream handler preventing nil pointer panic, replace dead r.Body nil check with MaxBytesReader size limit, distinguish not-found from other DB errors, fix bpsToKbps integer truncation with rounding, add "pcm" to isLosslessFormat for consistency with model.IsLossless(), add sampleRate/bitDepth/channels to streaming log, fix outdated test comment, and add tests for conversion functions and GetTranscodeStream parameter passing. * feat(transcoding): add sourceUpdatedAt to decision and validate transcode parameters Signed-off-by: Deluan <deluan@navidrome.org> * fix: small issues Updated mock AAC transcoding command to use the new default (ipod with fragmented MP4) matching the migration, ensuring tests exercise the same buildDynamicArgs code path as production. Improved archiver test mock to match on the whole StreamRequest struct instead of decomposing fields, making it resilient to future field additions. Added named constants for JWT claim keys in the transcode token and wrapped ParseTranscodeParams errors with ErrTokenInvalid for consistency. Documented the IsLossless BitDepth fallback heuristic as temporary until Codec column is populated. Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcoding): adapt transcode claims to struct-based auth.Claims Updated transcode token handling to use the struct-based auth.Claims introduced on master, replacing the previous map[string]any approach. Extended auth.Claims with transcoding-specific fields (MediaID, DirectPlay, UpdatedAt, Channels, SampleRate, BitDepth) and added float64 fallback in ClaimsFromToken for numeric claims that lose their Go type during JWT string serialization. Also added the missing lyrics parameter to all subsonic.New() calls in test files. * feat(model): add ProbeData field and UpdateProbeData repository method Add probe_data TEXT column to media_file for caching ffprobe results. Add UpdateProbeData to MediaFileRepository interface and implementations. Use hash:"ignore" tag so probe data doesn't affect MediaFile fingerprints. * feat(ffmpeg): add ProbeAudioStream for authoritative audio metadata Add ProbeAudioStream to FFmpeg interface, using ffprobe to extract codec, profile, bitrate, sample rate, bit depth, and channels. Parse bits_per_raw_sample as fallback for FLAC/ALAC bit depth. Normalize "unknown" profile to empty string. All parseProbeOutput tests use real ffprobe JSON from actual files. * feat(transcoding): integrate ffprobe into transcode decisions Add ensureProbed to probe media files on first transcode decision, caching results in probe_data. Build SourceStream from probe data with fallback to tag-based metadata. Refactor decision logic to pass StreamDetails instead of MediaFile, enabling codec profile limitations (e.g., audioProfile) to use probe data. Add normalizeProbeCodec to map ffprobe codec names (dsd_lsbf_planar, pcm_s16le) to internal names (dsd, pcm). NewDecider now accepts ffmpeg.FFmpeg; wire_gen.go regenerated. * feat(transcoding): add DevEnableMediaFileProbe config flag Add DevEnableMediaFileProbe (default true) to allow disabling ffprobe- based media file probing as a safety fallback. When disabled, the decider uses tag-based metadata from the scanner instead. * test(transcode): add ensureProbed unit tests Test probing when ProbeData is empty, skipping when already set, error propagation from ffprobe, and DevEnableMediaFileProbe flag. * refactor(ffmpeg): use command constant and select_streams for ProbeAudioStream Move ffprobe arguments to a probeAudioStreamCmd constant, following the same pattern as extractImageCmd and probeCmd. Add -select_streams a:0 to only probe the first audio stream, avoiding unnecessary parsing of video and artwork streams. Derive the ffprobe binary path safely using filepath.Dir/Base instead of replacing within the full path string. * refactor(transcode): decouple transcode token claims from auth.Claims Remove six transcode-specific fields (MediaID, DirectPlay, UpdatedAt, Channels, SampleRate, BitDepth) from auth.Claims, which is shared with session and share tokens. Transcode tokens are signed parameter-passing tokens, not authentication tokens, so coupling them to auth created misleading dependencies. The transcode package now owns its own JWT claim serialization via Decision.toClaimsMap() and paramsFromToken(), using generic auth.EncodeToken/DecodeAndVerifyToken wrappers that keep TokenAuth encapsulated. Wire format (JWT claim keys) is unchanged, so in-flight tokens remain compatible. Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcode): simplify code after review Extract getIntClaim helper to eliminate repeated int/int64/float64 JWT claim extraction pattern in paramsFromToken and ClaimsFromToken. Rewrite checkIntLimitation as a one-liner delegating to applyIntLimitation. Return probe result from ensureProbed to avoid redundant JSON round-trip. Extract toResponseStreamDetails helper and mediaTypeSong constant in the API layer, and use transcode.ProtocolHTTP constant instead of hardcoded string. Signed-off-by: Deluan <deluan@navidrome.org> * fix(ffmpeg): enhance bit_rate parsing logic for audio streams Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcode): improve code review findings across transcode implementation - Fix parseProbeData to return nil on JSON unmarshal failure instead of a zero-valued struct, preventing silent degradation of source stream details - Use probe-resolved codec for lossless detection in buildSourceStream instead of the potentially stale scanner data - Remove MediaFile.IsLossless() (dead code) and consolidate lossless detection in isLosslessFormat(), using codec name only — bit depth is not reliable since lossy codecs like ADPCM report non-zero values - Add "wavpack" to lossless codec list (ffprobe codec_name for WavPack) - Guard bpsToKbps against negative input values - Fix misleading comment in buildTemplateArgs about conditional injection - Avoid leaking internal error details in Subsonic API responses - Add missing test for ErrNotFound branch in GetTranscodeDecision - Add TODO for hardcoded protocol in toResponseStreamDetails * refactor(transcode): streamline transcoding command lookup and format resolution Signed-off-by: Deluan <deluan@navidrome.org> * feat(transcode): implement server-side transcoding override for player formats Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcode): honor bit depth and channel constraints in transcoding selection selectTranscodingOptions only checked sample rate when deciding whether same-format transcoding was needed, ignoring requested bit depth and channel reductions. This caused the streamer to return raw audio when the transcode decision requested downmix or bit-depth conversion. * refactor(transcode): unify streaming decision engine via MakeDecision Move transcoding decision-making out of mediaStreamer and into the subsonic Stream/Download handlers, using transcode.Decider.MakeDecision as the single decision engine. This eliminates selectTranscodingOptions and the mismatch between decision and streaming code paths (decision used LookupTranscodeCommand with built-in fallbacks, while streaming used FindByFormat which only checked the DB). - Add DecisionOptions with SkipProbe to MakeDecision so the legacy streaming path never calls ffprobe - Add buildLegacyClientInfo to translate legacy stream params (format, maxBitRate, DefaultDownsamplingFormat) into a synthetic ClientInfo - Add resolveStreamRequest on the subsonic Router to resolve legacy params into a fully specified StreamRequest via MakeDecision - Simplify DoStream to a dumb executor that receives pre-resolved params - Remove selectTranscodingOptions entirely Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcode): move MediaStreamer into core/transcode and unify StreamRequest Moved MediaStreamer, Stream, TranscodingCache and related types from core/media_streamer.go into core/transcode/, eliminating the duplicate StreamRequest type. The transcode.StreamRequest now carries all fields (ID, Format, BitRate, SampleRate, BitDepth, Channels, Offset) and ResolveStream returns a fully-populated value, removing manual field copying at every call site. Also moved buildLegacyClientInfo into the transcode package alongside ResolveStream, and unexported ParseTranscodeParams since it was only used internally by ValidateTranscodeParams. * refactor(transcode): rename Decider methods and unexport Params type Rename ResolveStream → ResolveRequest and ValidateTranscodeParams → ResolveRequestFromToken for clarity and consistency. The new ResolveRequestFromToken returns a StreamRequest directly (instead of the intermediate Params type), eliminating manual Params→StreamRequest conversion in callers. Unexport Params to params since it is now only used internally for JWT token parsing. * test(transcode): remove redundant tests and use constants Remove tests that duplicate coverage from integration-level tests (toClaimsMap, paramsFromToken round-trips, applyServerOverride direct call, duplicate 410 handler test). Replace raw "http" strings with ProtocolHTTP constant. Consolidate lossy -sample_fmt tests into DescribeTable. * refactor(transcode): split oversized files into focused modules Split transcode.go and transcode_test.go into focused files by concern: - decider.go: decision engine (MakeDecision, direct play/transcode evaluation, probe) - token.go: JWT token encode/decode (params, toClaimsMap, paramsFromToken, CreateTranscodeParams, ResolveRequestFromToken) - legacy_client.go: legacy Subsonic bridge (buildLegacyClientInfo, ResolveRequest) - codec_test.go: isLosslessFormat and normalizeProbeCodec tests - token_test.go: token round-trip and ResolveRequestFromToken tests Moved the Decider interface from types.go to decider.go to keep it near its implementation, and cleaned up types.go to contain only pure type definitions and constants. No public API changes. * refactor(transcode): reorder parameters in applyServerOverride function Signed-off-by: Deluan <deluan@navidrome.org> * test(e2e): add NewTestStream function and implement spyStreamer for testing Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-03-08 23:57:49 -04:00
Deluan Quintão	f03ca44a8e	feat(plugins): add lyrics provider plugin capability (#5126) ... * feat(plugins): add lyrics provider plugin capability Refactor the lyrics system from a static function to an interface-based service that supports WASM plugin providers. Plugins listed in the LyricsPriority config (alongside "embedded" and file extensions) are now resolved through the plugin system. Includes capability definition, Go/Rust PDK, adapter, Wire integration, and tests for plugin fallback behavior. * test(plugins): add lyrics capability integration test with test plugin * fix(plugins): default lyrics language to 'xxx' when plugin omits it Per the OpenSubsonic spec, the server must return 'und' or 'xxx' when the lyrics language is unknown. The lyrics plugin adapter was passing an empty string through when a plugin didn't provide a language value. This defaults the language to 'xxx', consistent with all other callers of model.ToLyrics() in the codebase. * refactor(plugins): rename lyrics import to improve clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor(lyrics): update TrackInfo description for clarity Signed-off-by: Deluan <deluan@navidrome.org> * fix(lyrics): enhance lyrics plugin handling and case sensitivity Signed-off-by: Deluan <deluan@navidrome.org> * fix(plugins): update payload type to string with byte format for task data Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-03-03 15:48:39 -05:00
Deluan Quintão	7ad2907719	refactor: move playlist business logic from repositories to service layer (#5027) ... * refactor: move playlist business logic from repositories to core.Playlists service Move authorization, permission checks, and orchestration logic from playlist repositories to the core.Playlists service, following the existing pattern used by core.Share and core.Library. Changes: - Expand core.Playlists interface with read, mutation, track management, and REST adapter methods - Add playlistRepositoryWrapper for REST Save/Update/Delete with permission checks (follows Share/Library pattern) - Simplify persistence/playlist_repository.go: remove isWritable(), auth checks from Delete()/Put()/updatePlaylist() - Simplify persistence/playlist_track_repository.go: remove isTracksEditable() and permission checks from Add/Delete/Reorder - Update Subsonic API handlers to route through service - Update Native API handlers to accept core.Playlists instead of model.DataStore * test: add coverage for playlist service methods and REST wrapper Add 30 new tests covering the service methods added during the playlist refactoring: - Delete: owner, admin, denied, not found - Create: new playlist, replace tracks, admin bypass, denied, not found - AddTracks: owner, admin, denied, smart playlist, not found - RemoveTracks: owner, smart playlist denied, non-owner denied - ReorderTrack: owner, smart playlist denied - NewRepository wrapper: Save (owner assignment, ID clearing), Update (owner, admin, denied, ownership change, not found), Delete (delegation with permission checks) Expand mockedPlaylistRepo with Get, Delete, Tracks, GetWithTracks, and rest.Persistable methods. Add mockedPlaylistTrackRepo for track operation verification. * fix: add authorization check to playlist Update method Added ownership verification to the Subsonic Update endpoint in the playlist service layer. The authorization check was present in the old repository code but was not carried over during the refactoring to the service layer, allowing any authenticated user to modify playlists they don't own via the Subsonic API. Also added corresponding tests for the Update method's permission logic. * refactor: improve playlist permission checks and error handling, add e2e tests Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename core.Playlists to playlists package and update references Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename playlists_internal_test.go to parse_m3u_test.go and update tests; add new parse_nsp.go and rest_adapter.go files Signed-off-by: Deluan <deluan@navidrome.org> * fix: block track mutations on smart playlists in Create and Update Create now rejects replacing tracks on smart playlists (pre-existing gap). Update now uses checkTracksEditable instead of checkWritable when track changes are requested, restoring the protection that was removed from the repository layer during the refactoring. Metadata-only updates on smart playlists remain allowed. * test: add smart playlist protection tests to ensure readonly behavior and mutation restrictions * refactor: optimize track removal and renumbering in playlists Signed-off-by: Deluan <deluan@navidrome.org> * refactor: implement track reordering in playlists with SQL updates Signed-off-by: Deluan <deluan@navidrome.org> * refactor: wrap track deletion and reordering in transactions for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove unused getTracks method from playlistTrackRepository Signed-off-by: Deluan <deluan@navidrome.org> * refactor: optimize playlist track renumbering with CTE-based UPDATE Replace the DELETE + re-INSERT renumbering strategy with a two-step UPDATE approach using a materialized CTE and ROW_NUMBER() window function. The previous approach (SELECT all IDs, DELETE all tracks, re-INSERT in chunks of 200) required 13 SQL operations for a 2000-track playlist. The new approach uses just 2 UPDATEs: first negating all IDs to clear the positive space, then assigning sequential positions via UPDATE...FROM with a CTE. This avoids the UNIQUE constraint violations that affected the original correlated subquery while reducing per-delete request time from ~110ms to ~12ms on a 2000-track playlist. Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename New function to NewPlaylists for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update mock playlist repository and tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-02-21 19:57:13 -05:00
Kendall Garner	6fb4cd277e	feat(subsonic): add OS readonly and validUntil properties in playlists (#4993) ... * feat(subsonic): add OS readonly and validUntil properties * remove duplicated test * test: fix and enable disabled child smart playlist tests Fixed the XContext("child smart playlists") tests that were disabled with a TODO comment. The tests had several issues: nested playlists were missing Public: true (required by InPlaylist criteria), the criteria matched no test fixtures, the "not expired" test set EvaluatedAt on the parent too (preventing it from refreshing at all), and the "expired" test dereferenced a nil EvaluatedAt. Added proper cleanup with DeferCleanup and config restoration via configtest. * fix(subsonic): always include readonly field in JSON playlist responses Removed omitempty from the JSON tag of the Readonly field in OpenSubsonicPlaylist so that readonly: false is always serialized in JSON responses, per the OpenSubsonic spec requirement that supported fields must be returned with default values. Added a test case with an empty OpenSubsonicPlaylist to verify the behavior. --------- Co-authored-by: Deluan Quintão <deluan@navidrome.org>	2026-02-06 19:35:54 -05:00
Matthew Simpson	9ab0c2dc67	feat: new "Subsonic Minimal Clients" configuration option (#4850) ... * Add `.editorconfig` file Hints to users how to properly indent Go files (my setup was defaulting to 2 spaces). * Add Subsonic API minimal config option This will allow users to specify clients which can operate with or need the minimum required fields as per the [SubSonic API spec](https://subsonic.org/pages/api.jsp). * Return only required fields for Child Objects For a minimal client, only return the required fields for Child Objects. * Return only required fields for Playlist objects * refactor: simplify client list checks and improve playlist response handling Signed-off-by: Deluan <deluan@navidrome.org> * test: add unit tests for client list checks and playlist building logic Signed-off-by: Deluan <deluan@navidrome.org> * fix: revert Child.IsVideo and Playlist.Public fields from pointer to boolean, and add omitempty to XML tag Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> Co-authored-by: Deluan Quintão <deluan@navidrome.org>	2026-01-16 05:55:21 -05:00
Kendall Garner	0cd15c1ddc	feat(prometheus): add metrics to Subsonic API and Plugins (#4266) ... * Add prometheus metrics to subsonic and plugins * address feedback, do not log error if operation is not supported * add missing timestamp and client to stats * remove .view from subsonic route * directly inject DataStore in Prometheus, to avoid having to pass it in every call Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> Co-authored-by: Deluan <deluan@navidrome.org>	2025-06-27 22:13:57 -04:00
Deluan Quintão	aab3223e00	fix(subsonic): clearing playlist comment and public in Subsonic API (#4258) ... * fix(subsonic): allow clearing playlist comment * fix(playlists): simplify comment and public parameter handling Signed-off-by: Deluan <deluan@navidrome.org> * refactor(playlists): streamline fakePlaylists implementation in tests Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2025-06-24 08:50:06 -04:00