3 Commits

Author	SHA1	Message	Date
Deluan Quintão	59448e9283	fix(scrobbler): back off when a provider asks us to, instead of retrying per play (#6028) ... * feat(agents): retry-later error type with optional server delay Add agents.ErrRetryLater and agents.RetryLaterError, which carries the delay requested by an external service (e.g. ListenBrainz's X-RateLimit-Reset-In). scrobbler.ErrRetryLater becomes an alias of the new sentinel, so existing errors.Is checks and the plugin error-string protocol keep working unchanged. Groundwork for honoring server-requested retry delays across scrobbling, metadata agents and artwork. Song.Equals tests moved to song_test.go to enable external test package. * fix(scrobbler): honor backoff window and server-requested retry delay ListenBrainz 429s were decoded into a typed error that classified as unrecoverable, silently discarding the scrobble (a JSON-bodied 429 was measured live). The client now maps any 429 to agents.RetryLaterError, carrying X-RateLimit-Reset-In when present (capped at 1h). Last.fm error 29 (rate limit) is now retryable like 11/16. The buffer's drain loop no longer lets wake signals bypass an active backoff window - new plays enqueue but drain only when the window closes - and the wait honors the server delay via max(backoff, retryIn). * feat(agents): skip cooling-down agents in aggregate calls When an agent reports retry-later, remember a per-agent cooldown deadline (the server-requested delay, or 1 minute when unspecified) and skip that agent in all aggregate metadata calls until it passes. A round that found no data but skipped or saw a throttled agent returns ErrRetryLater instead of ErrNotFound, so callers cannot mistake rate limiting for a definitive 'no data' answer. * feat(artwork): honor server-requested retry delay when rescheduling When an external image lookup fails with a retry-later error carrying a delay (e.g. a 429 with X-RateLimit-Reset-In), the chain trace carries the largest such hint back to the worker, which reschedules the item at max(exponential backoff, server delay) instead of backoff alone. * feat(plugins): retry-later with optional delay for scrobbler and agent plugins Scrobbler plugins can now return scrobbler(retry_later:N) to request a retry in N seconds (capped at 1h); the bare token keeps its old meaning. Metadata-agent plugins, which had no error vocabulary at all, gain the parallel agent(retry_later[:N]) token, mapped to agents.RetryLaterError so the aggregate's cooldown and the artwork worker honor plugin throttling the same way as built-in agents. * fix: address whole-branch review findings for retry-later handling Narrow the aggregate's throttled rule to the spec sentence: core.Agents returns ErrRetryLater only when no agent answered at all (all skipped-cooling or retry-later). An agent that does not implement the called method now returns an internal errUnsupported instead of ErrNotFound, so it counts as "did not run" — without that, the always-appended local agent would answer for biography, URL and images and make ErrRetryLater unreachable. Wire the consequence in core/external: a throttled round no longer stamps ExternalInfoUpdatedAt (artist and album), so the empty result is not cached for the TTL, and TopSongs maps ErrRetryLater to the same empty-200 the not-found path already produced instead of a new client-facing error. Move the Last.fm code-29 mapping into the client's central error construction so every metadata path produces RetryLaterError, and map ListenBrainz's body-level code 429 (sent with a non-429 HTTP status) the same way. Clamp server- and plugin-requested delays in seconds before scaling to a Duration, in all three parse sites: a header of 18446744074 wrapped past 2^64 and came out as a 0.29s delay. Also: extract the artwork worker's reschedule computation into retryDelay() and cover both it and the trace RetryIn wiring with tests; collapse the double regex call in mapScrobblerError; drop capabilities.ScrobblerErrorRetryLaterIn (ndpgen never emits funcs, so plugin authors could not reach it); regenerate the PDKs so MetadataAgentError reaches the Go and Rust SDKs; de-flake the cooldown tests (long RetryIn for the skip case, separate expiry spec); and cover the max() retry-delay aggregation across users in the scrobble buffer. * refactor: dedupe retry-later parsing and simplify error collection - Add agents.NewRetryLater and agents.RetryLaterFromSeconds, with a single 1h cap, replacing the parse+clamp+multiply logic and the maxRetryInSeconds constant duplicated across listenbrainz, plugins and the agent adapter. - Move HTTP header parsing to httpclient.RetryAfter, so the transport layer owns it and stays domain-agnostic; drop retryInFromHeaders from the ListenBrainz client. Covered by a new Ginkgo table in that package. - Collapse the two near-identical plugin retry_later regexes into one parseRetryLater(prefix, msg) shared by the agent and scrobbler adapters. - Fold the duplicated noteRetryIn snippet from fetchArtistImage and fetchAlbumImage into recordAgent, which already branched on the same isTransientExternal condition. - Replace the atomic.Bool + note() closure in populateArtistInfo with errgroup's own error collection; the group carries no context, so a returned error does not cancel its siblings. - Reuse recoveringScrobbler for the per-user delay test instead of a third double, and switch fakeScrobbler's mutex-guarded error to the atomic.Pointer idiom already used in the same package. * refactor(listenbrainz): keep rate-limit header parsing in the adapter The X-RateLimit-Reset-In header is ListenBrainz's own convention, not a shared one: Last.fm sends no rate-limit headers at all and reports its limit as a body code, and no other integration in tree sends Retry-After. A parser in utils/httpclient implied a uniformity across services that does not exist, so it moves back next to the only client that can know which header its service sends. * refactor(agents): collapse the retry-later sentinel and error into one type ErrRetryLater is now the zero-delay RetryLaterError rather than a separate errors.New value, so errors.Is and errors.AsType both match the sentinel and every delay-carrying variant. That removes the trap where a bare sentinel silently skipped the AsType path, and lets every consumer read the delay off the error directly: the RetryIn accessor and the two constructors are gone, with the policy cap applied where untrusted input is parsed. * refactor(agents): split the cooldown store from the per-dispatch tally The cooldown map and mutex become a cooldowns value with active/park, holding no knowledge of errors; agentAttempts records one dispatch's outcomes and owns the classification that noteAgentError used to hide behind a bool. The three dispatch loops now touch a single object: skip folds the cooldown check and the throttled flag into one call, so the store never appears in the loops. * refactor(agents): share one dispatch loop between the agent call helpers callAgentMethod and callAgentSliceMethod ran identical loops, differing only in how they test a result for emptiness: a slice cannot be compared against its zero value, so the two could not share a constraint. Both now delegate to callAgent, which takes that test as a parameter. Keeping the loop in one place matters more than the lines saved: it holds the cooldown skip, the attempt recording and the empty-dispatch verdict, and a fix applied to one copy but not the other would be silent. * test: cover the two retry-later paths a mutation could break silently Both gaps were proven, not guessed: making the artwork worker pass 0 instead of the collected hint left all 386 specs green, and replacing the default agent cooldown with 0 left the agents suite green. The worker test drives a throttled image agent through drain and asserts the persisted retry_at, and the cooldown test parks an agent that asked to be retried without naming a delay, which is what Last.fm does on every rate limit. * refactor(artwork): carry the external failure as an error, not a flag plus a trace field The retry delay was riding on ChainTrace, a diagnostic that gets persisted, while the very same signal — an external source faulted — already travelled by value as resolution.extError. That was two mechanisms for one idea, and it put control-flow state inside a serializable trace. resolution.extError and chainState.extErr become the error itself, so a caller checks err != nil for the fault and errors.AsType for the delay the provider asked for. The agent loops return that error last, per convention, and longerRetry keeps whichever failure wants the longer wait. ChainTrace goes back to holding only steps and no longer imports core/agents. * fix(artwork): check the resolve error before reading its resolution Reading res.extError before the err check was safe only because every error path in resolve returns a bare resolution{}; a future path returning a partly-filled one would have been read silently. The failure path now returns no delay explicitly. * test(artwork): assert the delay acquire reports, not just its downstream effect acquire's retry delay was only covered through the worker's persisted retry_at, one layer away from where the value is computed. Both outcomes are now pinned at the processor: a plain failure asks for nothing, a throttled provider's delay is passed through. * refactor: share the retry-seconds parse and drop the backoff deadline arithmetic The clamp-before-scaling invariant lived in two parsers and was independently re-tested in three files with the same magic number; a fix applied to one copy would have left the others wrapping a huge value down to a fraction of a second. It moves to agents.ParseRetryIn. The buffer tracked an absolute retryDeadline only to re-arm a timer that was already armed for the same instant; a backingOff flag says the same thing without the arithmetic. The plugin token regex now carries its capability in the pattern instead of capturing and comparing, so another capability's token in the same message cannot mask it. resolution.extError becomes extErr, matching its chainState counterpart. * fix(agents): keep the longer cooldown when parks overlap Calls to one agent overlap, so a short cooldown could land after a long one started and cut it short. park now keeps whichever deadline is later, matching the rule longerRetry already applies on the artwork side. No in-tree provider can currently produce two different delays for the same agent, so this is hardening rather than a fix for observed behaviour. * fix(agents): parse the retry delay at a fixed width strconv.Atoi parses into the native int, so on the 32-bit targets we ship (linux/386, windows/386, three ARM variants) a delay above MaxInt32 seconds overflowed and became unspecified instead of being capped. No provider sends a 68-year delay, so this is not user-visible, but the overflow tests asserted the cap and would have failed on those architectures, where tests never run. * fix(plugins): anchor the retry_later regex to a word boundary Prevents a superstring like useragent(retry_later) from matching the agent capability token.	2026-08-29 17:28:29 -04:00
Deluan Quintão	bd9fa1c602	feat(listenbrainz): match collaboration top-songs via all credited artist MBIDs (#5670) ... * refactor(agents): drop single Artist fields from Song, keep only Artists Song now represents credited artists solely via the Artists slice; the single Artist/ArtistMBID fields and the ArtistList passthrough are removed. Equals continues to hash the whole value. * refactor(lastfm,listenbrainz): build Song.Artists in built-in agents Last.fm and ListenBrainz now populate the Artists slice directly. For ListenBrainz top songs, all credited artist MBIDs are mapped (the combined display name on the first credit plus MBID-only collaborators) instead of keeping only the first MBID, feeding the matcher's per-MBID specificity. * refactor(external): set Song.Artists in top-songs enrichment getMatchingTopSongs now seeds an Artists entry from the known artist when a song carries none, replacing the single Artist/ArtistMBID writes. * refactor(plugins): fold single-artist SongRef into Song.Artists SongRef keeps its single Artist/ArtistMBID fields as part of the plugin wire contract; songRefToAgentSong now folds them into a one-element Artists list when a plugin sends no artists array. * refactor(matcher): read Song.Artists directly groupQueries consumes s.Artists now that ArtistList is gone; test inputs build the Artists slice. * fix(matcher): keep MBID-only artists as identity signals Review follow-up: an artist credited only by MBID (empty ID and name) was dropped before resolution in four places, defeating the multi-MBID matching path this PR adds. - matcher.groupQueries: treat a non-empty MBID as a usable artist signal - external.getMatchingTopSongs: backfill the primary credit's name/MBID when the agent left them empty - plugins.songRefToAgentSong: fold a single-artist SongRef when only ArtistMBID is set (not just when Artist name is set) - listenbrainz.topSongArtists: return nil instead of an empty-name placeholder when neither name nor MBIDs are present * fix(external): only backfill top-song artist onto an unnamed credit Review follow-up (codex P2): the previous backfill stamped the queried artist's MBID onto Artists[0] whenever it was empty, even when that credit already named a different (e.g. featured) artist — producing a mismatched name+MBID pair that could mis-rank matches. Now only an unnamed first credit is filled (it is, by construction, the queried artist); an already-named credit is left untouched. Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-06-26 17:06:14 -04:00
Deluan Quintão	7b7721f002	feat(matcher): match similar/top songs by multiple artists (#5668) ... * feat(matcher): add Song.Artists (agents.Artist) and field-wise song dedup * refactor(matcher): make song equality an agents.Song.Equals method via hashstructure Move the sameSong free function from core/matcher into an Equals method on agents.Song, following the model.MediaFile/Album.Equals convention. Uses strict hashstructure hashing (nil opts, no IgnoreZeroValue) to preserve the original whole-value equality contract. Tests moved to core/agents. * feat(matcher): match by multiple artists with overlap ranking and artist-ID fast-path * refactor(matcher): rank artist overlap and specificity above the preferred-track flag Identity signals (specificityLevel, artistOverlap) now outrank the taste signal (preferredMatch) in betterThan. A starred/4-star track that is a worse identity match no longer beats a more specific or higher-overlap track. PreferStarred still breaks ties when specificity and overlap are equal. * fix(matcher): score artist-MBID specificity against all credited artists, not just the last sanitizedTrack.artistMBID (string) replaced with artistMBIDs (map[string]struct{}) so bucketTracks collects all credited owned MBIDs per query instead of last-write-wins. computeSpecificityLevel tests set membership, letting each of a collaboration's MBID-bearing artists reach the proper specificity level (4/5) independently. * feat(plugins): carry multiple artists (with IDs) through SongRef conversions * feat(plugins): regenerate schemas and PDK wrappers for multi-artist SongRef * refactor(matcher): tidy bucketTracks accumulator and artist resolution Replace bucketTracks' two parallel per-track maps (overlapByQuery/mbidsByQuery) with a named queryAccum struct (F2). Collapse resolveArtists' four hand-mutated parallel maps into a pendingArtist slice with derived nameToQueries/mbidToQueries maps (F1). Replace the own() method on resolvedArtists with a package-level addToSet helper that drops the method/receiver indirection (F3). * docs(matcher): trim comments that restate the code * fix(plugins): use Vec::is_empty for slice fields in generated Rust PDK * fix(matcher): treat a resolved artist ID as an identity match for specificity * docs(matcher): reflect artist-ID identity in the specificity ladder	2026-06-26 14:46:09 -04:00