nik/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-08-31 07:30:32 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(instant-mix): top short mixes up instead of returning what the first source found (#5951)

Browse Source 
* fix(instant-mix): top short mixes up instead of returning what the first source found

SimilarSongs returned the first non-empty source's tracks, however few. For a
thinly-represented artist that meant a 3-track mix no matter the requested count:
the artist agent found nothing, the similar-artists fallback matched 3 library
tracks, and `len(res) > 0` kept seed-track sampling from ever running.

Clients treat that as a failed mix and retry with a bigger limit forever. Finamp
cycles limit 34 through 472 and starts over, ~1 request every 2s indefinitely,
each one re-hitting Last.fm, Deezer and AudioMuse.

Sources are now chained rather than raced: each one tops the mix up until it
holds count tracks, so the agent's picks, the similar-artists fallback and
seed-track sampling all contribute instead of the first one winning outright.

* refactor(external): move similar-songs code to its own file

provider.go held two distinct concerns: artist/album external metadata and the
similar-songs mix pipeline. The mix code was already one contiguous block, and
maxSeeds, maxSimilarSongs and dedupByID were used by nothing else.

Moved SimilarSongs and its helpers to provider_similarsongs.go, matching the
existing provider_similarsongs_test.go. Pure code motion: the moved block is
byte-for-byte unchanged and provider.go has no additions, only deletions.

* fix(instant-mix): dedup before deciding a mix is full

topUp measured res before deduplicating it. Matcher.MatchSongs deliberately
re-emits a library track when the same input song repeats, and the similar-artists
fallback can reach one track through several artists, so len(res) could equal count
while holding fewer unique tracks. That returned a mix with duplicates in it and
stopped the top-up early; the caller then deduplicated and handed back a short mix,
which is the client retry loop this branch set out to fix.

Deduplicate first, so the length check counts what the client will actually receive.

* perf(instant-mix): skip a fallback once the mix is already full

The artist path nested one topUp inside another, so the inner one measured only
similarSongsFallback's own result against the full count. With 49 agent matches and
one fallback match for count=50 the mix was already full, yet seed-track sampling
still ran and fired up to five GetSimilarSongsByTrack calls whose results the outer
topUp then truncated away.

topUp now takes the sources as a variadic list and re-checks the accumulated mix
before each one, so a later, costlier source only runs while the mix is still short.
That also flattens the artist case: the agent, the similar-artists fallback and
seed-track sampling are now three peers in one chain instead of two nested calls.

* fix(instant-mix): count distinct tracks when picking the fallback mix

similarSongsFallback stopped after count picks from the weighted chooser, but a
track can sit in that chooser once per artist listing it in their top songs, and
Pick removes the entry it returns. Repeats therefore consumed pick slots and left
unique candidates stranded, so the batch could come back short of count. On the
track path this is the only source, so that short mix reached the client and kept
the retry loop alive.

Track the ids already picked and keep drawing until count distinct tracks are held
or the chooser is empty.

* fix(instant-mix): match the whole agent response before trimming

MatchSongs was capped at count, and it re-emits a track when the same song repeats,
so [A, A, B] with count=2 returned [A, A] and never reached B. topUp then shrank
that to [A] and, with an empty or overlapping fallback, the mix stayed short even
though B had been available all along. seedMix already matched its full merged set
for this reason; mixFromAgent now does the same and leaves the trim to topUp.

Also drop the capacity hint on the picked-ids map. It was sized from the caller's
count, which CodeQL flags as an allocation sized by user input (go/uncontrolled-
allocation-size). SimilarSongs clamps count to maxSimilarSongs long before this
point, so the hint bought nothing worth the alert.

* refactor(instant-mix): tidy the mix chain and its specs

Quality pass over the new code, no behaviour change:

- topUp: drop the first-vs-last error bookkeeping (the value is only read when the
  mix is empty, so the distinction is unobservable) and the redundant nil guard
  (dedupByID returns nil for an empty result, so both branches already agreed).
- mixFromAgent: assign through the if-scoped err instead of a second error name.
- Hoist the similar-artists fallback closure written verbatim in two switch arms.
- Use map[string]struct{} in the pick loop, matching dedupByID in the same file.
- Trim three comments back within budget; two restated the line below them and one
  carried commit-message rationale.
- Tests: add an ids() helper for the ID assertion repeated seven times, and fold
  the track-entity stub block copied into three specs into stubTrackEntity. The
  block hard-coded .Twice() on GetEntityByID, which pinned an implementation
  detail no spec asserts.

* revert(instant-mix): inline the similar-artists fallback closure again

Hoisting it to a shared artistFallback var moved the call away from the arm that
uses it and saved nothing: each arm reads better spelling out its own sources.
This commit is contained in:
Deluan Quintão 2026-08-13 18:50:34 -04:00 committed by GitHub
parent 6c3e7e268b
commit 757ca783d3
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 515 additions and 263 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
core/external/extdata_helper_test.go vendored
View File

@ -6,6 +6,7 @@ import (
"github.com/navidrome/navidrome/core/agents"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/utils/slice"
"github.com/stretchr/testify/mock"
)
@ -324,3 +325,7 @@ func (m *mockAgents) GetSimilarSongsByArtist(ctx context.Context, id, name, mbid
}
return nil, args.Error(1)
}
func ids(mfs model.MediaFiles) []string {
return slice.Map(mfs, func(mf model.MediaFile) string { return mf.ID })
}

255
core/external/provider.go vendored
View File

@ -4,7 +4,6 @@ import (
"context"
"errors"
"fmt"
"math/rand/v2"
"sort"
"strings"
"time"
@ -15,21 +14,15 @@ import (
"github.com/navidrome/navidrome/core/matcher"
"github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/persistence"
"github.com/navidrome/navidrome/utils"
. "github.com/navidrome/navidrome/utils/gg"
"github.com/navidrome/navidrome/utils/random"
"github.com/navidrome/navidrome/utils/slice"
"github.com/navidrome/navidrome/utils/str"
"golang.org/x/sync/errgroup"
)
const (
maxSimilarArtists = 100
maxSeeds = 5
// Subsonic passes the client's count through unbounded, and it ends up as a SQL limit. 500 is
// what the widest caller (similarAlbums, limit*5) legitimately asks for.
maxSimilarSongs = 500
maxSimilarArtists = 100
refreshDelay = 5 * time.Second
refreshTimeout = 15 * time.Second
refreshQueueLength = 2000
@ -283,252 +276,6 @@ func (e *provider) populateArtistInfo(ctx context.Context, artist auxArtist) (au
return artist, nil
}
func (e *provider) SimilarSongs(ctx context.Context, id string, count int) (model.MediaFiles, error) {
// Subsonic passes the client's count straight through: a non-positive one has no valid
// interpretation, and an enormous one overflows the +1 in the local agent's query limit.
if count <= 0 {
return nil, nil
}
count = min(count, maxSimilarSongs)
entity, err := model.GetEntityByID(ctx, e.ds, id)
if err != nil {
// Genre ids don't resolve via GetEntityByID; look them up before giving up.
if !errors.Is(err, model.ErrNotFound) {
return nil, err
}
genre, err := e.ds.Genre(ctx).Get(id)
if err != nil {
return nil, err
}
return e.seedMix(ctx, count, func() (model.MediaFiles, error) {
return e.sampleGenreTracks(ctx, genre, maxSeeds)
})
}
// Try entity-specific similarity first, then fall back to seed-track sampling.
switch v := entity.(type) {
case *model.MediaFile:
return e.mixFromAgent(ctx, count,
func() ([]agents.Song, error) {
return e.ag.GetSimilarSongsByTrack(ctx, v.ID, v.Title, v.Artist, v.MbzRecordingID, count)
},
func() (model.MediaFiles, error) {
return e.similarSongsFallback(ctx, id, count)
})
case *model.Album:
return e.mixFromAgent(ctx, count,
func() ([]agents.Song, error) {
return e.ag.GetSimilarSongsByAlbum(ctx, v.ID, v.Name, v.AlbumArtist, v.MbzAlbumID, count)
},
func() (model.MediaFiles, error) {
return e.seedMix(ctx, count, func() (model.MediaFiles, error) {
return e.sampleAlbumTracks(ctx, v.ID, maxSeeds)
})
})
case *model.Artist:
return e.mixFromAgent(ctx, count,
func() ([]agents.Song, error) {
return e.ag.GetSimilarSongsByArtist(ctx, v.ID, v.Name, v.MbzArtistID, count)
},
func() (model.MediaFiles, error) {
if res, ferr := e.similarSongsFallback(ctx, id, count); ferr == nil && len(res) > 0 {
return res, nil
}
return e.seedMix(ctx, count, func() (model.MediaFiles, error) {
return e.sampleArtistTracks(ctx, v.ID, maxSeeds)
})
})
case *model.Playlist:
return e.seedMix(ctx, count, func() (model.MediaFiles, error) {
return e.samplePlaylistTracks(ctx, v.ID, maxSeeds)
})
default:
log.Warn(ctx, "Unknown entity type", "id", id, "type", fmt.Sprintf("%T", entity))
return nil, model.ErrNotFound
}
}
// mixFromAgent returns the agent's recommendations matched to library tracks, or the fallback
// when the agent errors or none of its picks are in the library.
func (e *provider) mixFromAgent(ctx context.Context, count int, fetch func() ([]agents.Song, error), fallback func() (model.MediaFiles, error)) (model.MediaFiles, error) {
songs, err := fetch()
if err == nil {
matched, merr := e.matcher.MatchSongs(ctx, songs, count)
if merr != nil {
return nil, merr
}
if len(matched) > 0 {
return matched, nil
}
}
return fallback()
}
// seedMix samples seed tracks, runs each through the agent chain's per-track similarity and merges
// the results, falling back to the seeds themselves so the result is never empty.
func (e *provider) seedMix(ctx context.Context, count int, sample func() (model.MediaFiles, error)) (model.MediaFiles, error) {
seeds, err := sample()
if err != nil {
return nil, err
}
if len(seeds) == 0 {
return nil, nil
}
seeds = seeds[:min(len(seeds), maxSeeds)]
// The per-seed similarity calls are independent and hit the (possibly remote) agent chain, so
// run them concurrently. Best-effort: a seed that errors just contributes nothing.
perSeed := make([][]agents.Song, len(seeds))
var g errgroup.Group
for i, seed := range seeds {
g.Go(func() error {
if s, err := e.ag.GetSimilarSongsByTrack(ctx, seed.ID, seed.Title, seed.Artist, seed.MbzRecordingID, count); err == nil {
perSeed[i] = s
}
return nil
})
}
_ = g.Wait()
var songs []agents.Song
for _, s := range perSeed {
songs = append(songs, s...)
}
// Match the whole merged set, not just count of it: the matcher re-emits a track when two
// seeds recommend it identically, so the duplicates have to be dropped before trimming. Every
// seed reaches the shuffle, so no seed can crowd out the others.
matched, err := e.matcher.MatchSongs(ctx, songs, len(songs))
if err != nil {
return nil, err
}
matched = dedupByID(matched)
if len(matched) == 0 {
matched = seeds
}
rand.Shuffle(len(matched), func(i, j int) { matched[i], matched[j] = matched[j], matched[i] })
if len(matched) > count {
matched = matched[:count]
}
return matched, nil
}
func (e *provider) samplePlaylistTracks(ctx context.Context, playlistID string, n int) (model.MediaFiles, error) {
// Refresh: a smart playlist materializes no tracks until it is evaluated, so skipping it would
// mix an empty seed set. It is a no-op for regular playlists and inside the refresh delay.
repo := e.ds.Playlist(ctx).Tracks(playlistID, true)
if repo == nil {
return nil, model.ErrNotFound
}
// A playlist can hold the same file at several positions, so over-fetch and dedup: a repeated
// seed wastes an agent call and can reach the mix twice through the seed fallback.
tracks, err := repo.GetAll(model.QueryOptions{
Sort: "random",
Max: n * 4,
Filters: squirrel.Eq{"missing": false},
})
if err != nil {
return nil, err
}
mfs := dedupByID(tracks.MediaFiles())
return mfs[:min(len(mfs), n)], nil
}
func dedupByID(mfs model.MediaFiles) model.MediaFiles {
seen := make(map[string]struct{}, len(mfs))
return slice.Filter(mfs, func(mf model.MediaFile) bool {
if _, dup := seen[mf.ID]; dup {
return false
}
seen[mf.ID] = struct{}{}
return true
})
}
func (e *provider) sampleAlbumTracks(ctx context.Context, albumID string, n int) (model.MediaFiles, error) {
return e.sampleTracks(ctx, squirrel.Eq{"album_id": albumID}, n)
}
func (e *provider) sampleArtistTracks(ctx context.Context, artistID string, n int) (model.MediaFiles, error) {
// media_file.artist_id is the deprecated primary artist, so it misses an artist credited only
// on the album, as on compilations. Same filter the artist listings use.
filter := persistence.ParticipantIDFilter("media_file", artistID, model.RoleArtist, model.RoleAlbumArtist)
return e.sampleTracks(ctx, filter, n)
}
func (e *provider) sampleGenreTracks(ctx context.Context, genre *model.Genre, n int) (model.MediaFiles, error) {
return e.sampleTracks(ctx, persistence.SongGenres.ByID(genre.ID), n)
}
// sampleTracks returns up to n random present tracks. Seeds can end up in the mix verbatim, so
// missing files would surface as unplayable entries.
func (e *provider) sampleTracks(ctx context.Context, filter squirrel.Sqlizer, n int) (model.MediaFiles, error) {
return e.ds.MediaFile(ctx).GetRandom(model.QueryOptions{
Filters: squirrel.And{filter, squirrel.Eq{"missing": false}},
Max: n,
})
}
// similarSongsFallback uses the original similar artists + top songs algorithm. The idea is to
// get the artist of the given entity, retrieve similar artists, get their top songs, and pick
// a weighted random selection of songs to return as similar songs.
func (e *provider) similarSongsFallback(ctx context.Context, id string, count int) (model.MediaFiles, error) {
artist, err := e.getArtist(ctx, id)
if err != nil {
return nil, err
}
e.callGetSimilarArtists(ctx, e.ag, &artist, 15, false)
if utils.IsCtxDone(ctx) {
log.Warn(ctx, "SimilarSongs call canceled", ctx.Err())
return nil, ctx.Err()
}
weightedSongs := random.NewWeightedChooser[model.MediaFile]()
addArtist := func(a model.Artist, weightedSongs *random.WeightedChooser[model.MediaFile], count, artistWeight int) error {
if utils.IsCtxDone(ctx) {
log.Warn(ctx, "SimilarSongs call canceled", ctx.Err())
return ctx.Err()
}
topCount := max(count, 20)
topSongs, err := e.getMatchingTopSongs(ctx, e.ag, &auxArtist{Artist: a}, topCount)
if err != nil {
log.Warn(ctx, "Error getting artist's top songs", "artist", a.Name, err)
return nil
}
weight := topCount * (4 + artistWeight)
for _, mf := range topSongs {
weightedSongs.Add(mf, weight)
weight -= 4
}
return nil
}
err = addArtist(artist.Artist, weightedSongs, count, 10)
if err != nil {
return nil, err
}
for _, a := range artist.SimilarArtists {
err := addArtist(a, weightedSongs, count, 0)
if err != nil {
return nil, err
}
}
var similarSongs model.MediaFiles
for len(similarSongs) < count && weightedSongs.Size() > 0 {
s, err := weightedSongs.Pick()
if err != nil {
log.Warn(ctx, "Error getting weighted song", err)
continue
}
similarSongs = append(similarSongs, s)
}
return similarSongs, nil
}
func (e *provider) TopSongs(ctx context.Context, artistName, id string, count int) (model.MediaFiles, error) {
artist, err := e.findArtist(ctx, artistName, id)
if err != nil {

299
core/external/provider_similarsongs.go vendored Normal file
View File

@ -0,0 +1,299 @@
package external
import (
"context"
"errors"
"fmt"
"math/rand/v2"
"github.com/Masterminds/squirrel"
"github.com/navidrome/navidrome/core/agents"
"github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/persistence"
"github.com/navidrome/navidrome/utils"
"github.com/navidrome/navidrome/utils/random"
"github.com/navidrome/navidrome/utils/slice"
"golang.org/x/sync/errgroup"
)
const (
maxSeeds = 5
// Subsonic passes the client's count through unbounded, and it ends up as a SQL limit. 500 is
// what the widest caller (similarAlbums, limit*5) legitimately asks for.
maxSimilarSongs = 500
)
func (e *provider) SimilarSongs(ctx context.Context, id string, count int) (model.MediaFiles, error) {
// Subsonic passes the client's count straight through: a non-positive one has no valid
// interpretation, and an enormous one overflows the +1 in the local agent's query limit.
if count <= 0 {
return nil, nil
}
count = min(count, maxSimilarSongs)
entity, err := model.GetEntityByID(ctx, e.ds, id)
if err != nil {
// Genre ids don't resolve via GetEntityByID; look them up before giving up.
if !errors.Is(err, model.ErrNotFound) {
return nil, err
}
genre, err := e.ds.Genre(ctx).Get(id)
if err != nil {
return nil, err
}
return e.seedMix(ctx, count, func() (model.MediaFiles, error) {
return e.sampleGenreTracks(ctx, genre, maxSeeds)
})
}
// Try entity-specific similarity first, then fall back to seed-track sampling.
switch v := entity.(type) {
case *model.MediaFile:
return e.mixFromAgent(ctx, count,
func() ([]agents.Song, error) {
return e.ag.GetSimilarSongsByTrack(ctx, v.ID, v.Title, v.Artist, v.MbzRecordingID, count)
},
func() (model.MediaFiles, error) {
return e.similarSongsFallback(ctx, id, count)
})
case *model.Album:
return e.mixFromAgent(ctx, count,
func() ([]agents.Song, error) {
return e.ag.GetSimilarSongsByAlbum(ctx, v.ID, v.Name, v.AlbumArtist, v.MbzAlbumID, count)
},
func() (model.MediaFiles, error) {
return e.seedMix(ctx, count, func() (model.MediaFiles, error) {
return e.sampleAlbumTracks(ctx, v.ID, maxSeeds)
})
})
case *model.Artist:
return e.mixFromAgent(ctx, count,
func() ([]agents.Song, error) {
return e.ag.GetSimilarSongsByArtist(ctx, v.ID, v.Name, v.MbzArtistID, count)
},
func() (model.MediaFiles, error) {
return e.similarSongsFallback(ctx, id, count)
},
func() (model.MediaFiles, error) {
return e.seedMix(ctx, count, func() (model.MediaFiles, error) {
return e.sampleArtistTracks(ctx, v.ID, maxSeeds)
})
})
case *model.Playlist:
return e.seedMix(ctx, count, func() (model.MediaFiles, error) {
return e.samplePlaylistTracks(ctx, v.ID, maxSeeds)
})
default:
log.Warn(ctx, "Unknown entity type", "id", id, "type", fmt.Sprintf("%T", entity))
return nil, model.ErrNotFound
}
}
// mixFromAgent returns the agent's recommendations matched to library tracks, topped up from the
// fallbacks in order when they don't fill the mix on their own.
func (e *provider) mixFromAgent(ctx context.Context, count int, fetch func() ([]agents.Song, error), fallbacks ...func() (model.MediaFiles, error)) (model.MediaFiles, error) {
var matched model.MediaFiles
if songs, err := fetch(); err == nil {
// Match the whole response: capping at count can stop before a later unique pick.
matched, err = e.matcher.MatchSongs(ctx, songs, len(songs))
if err != nil {
return nil, err
}
}
return topUp(ctx, matched, count, fallbacks...)
}
// topUp draws on each source in turn until the mix holds count distinct tracks.
func topUp(ctx context.Context, res model.MediaFiles, count int, sources ...func() (model.MediaFiles, error)) (model.MediaFiles, error) {
// The matcher can re-emit a track, so a full-looking res may hold fewer than count unique ones.
res = dedupByID(res)
var lastErr error
for _, more := range sources {
if len(res) >= count {
break
}
extra, err := more()
if err != nil {
log.Debug(ctx, "Could not top up a short mix", "have", len(res), "want", count, err)
lastErr = err
continue
}
res = dedupByID(append(res, extra...))
}
if len(res) == 0 {
return nil, lastErr
}
return res[:min(len(res), count)], nil
}
// seedMix samples seed tracks, runs each through the agent chain's per-track similarity and merges
// the results, falling back to the seeds themselves so the result is never empty.
func (e *provider) seedMix(ctx context.Context, count int, sample func() (model.MediaFiles, error)) (model.MediaFiles, error) {
seeds, err := sample()
if err != nil {
return nil, err
}
if len(seeds) == 0 {
return nil, nil
}
seeds = seeds[:min(len(seeds), maxSeeds)]
// The per-seed similarity calls are independent and hit the (possibly remote) agent chain, so
// run them concurrently. Best-effort: a seed that errors just contributes nothing.
perSeed := make([][]agents.Song, len(seeds))
var g errgroup.Group
for i, seed := range seeds {
g.Go(func() error {
if s, err := e.ag.GetSimilarSongsByTrack(ctx, seed.ID, seed.Title, seed.Artist, seed.MbzRecordingID, count); err == nil {
perSeed[i] = s
}
return nil
})
}
_ = g.Wait()
var songs []agents.Song
for _, s := range perSeed {
songs = append(songs, s...)
}
// Match the whole merged set, not just count of it: the matcher re-emits a track when two
// seeds recommend it identically, so the duplicates have to be dropped before trimming. Every
// seed reaches the shuffle, so no seed can crowd out the others.
matched, err := e.matcher.MatchSongs(ctx, songs, len(songs))
if err != nil {
return nil, err
}
matched = dedupByID(matched)
if len(matched) == 0 {
matched = seeds
}
rand.Shuffle(len(matched), func(i, j int) { matched[i], matched[j] = matched[j], matched[i] })
if len(matched) > count {
matched = matched[:count]
}
return matched, nil
}
func (e *provider) samplePlaylistTracks(ctx context.Context, playlistID string, n int) (model.MediaFiles, error) {
// Refresh: a smart playlist materializes no tracks until it is evaluated, so skipping it would
// mix an empty seed set. It is a no-op for regular playlists and inside the refresh delay.
repo := e.ds.Playlist(ctx).Tracks(playlistID, true)
if repo == nil {
return nil, model.ErrNotFound
}
// A playlist can hold the same file at several positions, so over-fetch and dedup: a repeated
// seed wastes an agent call and can reach the mix twice through the seed fallback.
tracks, err := repo.GetAll(model.QueryOptions{
Sort: "random",
Max: n * 4,
Filters: squirrel.Eq{"missing": false},
})
if err != nil {
return nil, err
}
mfs := dedupByID(tracks.MediaFiles())
return mfs[:min(len(mfs), n)], nil
}
func dedupByID(mfs model.MediaFiles) model.MediaFiles {
seen := make(map[string]struct{}, len(mfs))
return slice.Filter(mfs, func(mf model.MediaFile) bool {
if _, dup := seen[mf.ID]; dup {
return false
}
seen[mf.ID] = struct{}{}
return true
})
}
func (e *provider) sampleAlbumTracks(ctx context.Context, albumID string, n int) (model.MediaFiles, error) {
return e.sampleTracks(ctx, squirrel.Eq{"album_id": albumID}, n)
}
func (e *provider) sampleArtistTracks(ctx context.Context, artistID string, n int) (model.MediaFiles, error) {
// media_file.artist_id is the deprecated primary artist, so it misses an artist credited only
// on the album, as on compilations. Same filter the artist listings use.
filter := persistence.ParticipantIDFilter("media_file", artistID, model.RoleArtist, model.RoleAlbumArtist)
return e.sampleTracks(ctx, filter, n)
}
func (e *provider) sampleGenreTracks(ctx context.Context, genre *model.Genre, n int) (model.MediaFiles, error) {
return e.sampleTracks(ctx, persistence.SongGenres.ByID(genre.ID), n)
}
// sampleTracks returns up to n random present tracks. Seeds can end up in the mix verbatim, so
// missing files would surface as unplayable entries.
func (e *provider) sampleTracks(ctx context.Context, filter squirrel.Sqlizer, n int) (model.MediaFiles, error) {
return e.ds.MediaFile(ctx).GetRandom(model.QueryOptions{
Filters: squirrel.And{filter, squirrel.Eq{"missing": false}},
Max: n,
})
}
// similarSongsFallback uses the original similar artists + top songs algorithm. The idea is to
// get the artist of the given entity, retrieve similar artists, get their top songs, and pick
// a weighted random selection of songs to return as similar songs.
func (e *provider) similarSongsFallback(ctx context.Context, id string, count int) (model.MediaFiles, error) {
artist, err := e.getArtist(ctx, id)
if err != nil {
return nil, err
}
e.callGetSimilarArtists(ctx, e.ag, &artist, 15, false)
if utils.IsCtxDone(ctx) {
log.Warn(ctx, "SimilarSongs call canceled", ctx.Err())
return nil, ctx.Err()
}
weightedSongs := random.NewWeightedChooser[model.MediaFile]()
addArtist := func(a model.Artist, weightedSongs *random.WeightedChooser[model.MediaFile], count, artistWeight int) error {
if utils.IsCtxDone(ctx) {
log.Warn(ctx, "SimilarSongs call canceled", ctx.Err())
return ctx.Err()
}
topCount := max(count, 20)
topSongs, err := e.getMatchingTopSongs(ctx, e.ag, &auxArtist{Artist: a}, topCount)
if err != nil {
log.Warn(ctx, "Error getting artist's top songs", "artist", a.Name, err)
return nil
}
weight := topCount * (4 + artistWeight)
for _, mf := range topSongs {
weightedSongs.Add(mf, weight)
weight -= 4
}
return nil
}
err = addArtist(artist.Artist, weightedSongs, count, 10)
if err != nil {
return nil, err
}
for _, a := range artist.SimilarArtists {
err := addArtist(a, weightedSongs, count, 0)
if err != nil {
return nil, err
}
}
// Count distinct tracks, not picks: a collaboration sits in the chooser once per artist that
// lists it, and letting those repeats consume the budget strands unique candidates.
var similarSongs model.MediaFiles
picked := map[string]struct{}{}
for len(similarSongs) < count && weightedSongs.Size() > 0 {
s, err := weightedSongs.Pick()
if err != nil {
log.Warn(ctx, "Error getting weighted song", err)
continue
}
if _, dup := picked[s.ID]; dup {
continue
}
picked[s.ID] = struct{}{}
similarSongs = append(similarSongs, s)
}
return similarSongs, nil
}

219
core/external/provider_similarsongs_test.go vendored
View File

@ -13,7 +13,6 @@ import (
"github.com/navidrome/navidrome/core/matcher"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/tests"
"github.com/navidrome/navidrome/utils/slice"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/stretchr/testify/mock"
@ -65,6 +64,18 @@ var _ = Describe("Provider - SimilarSongs", func() {
provider = NewProvider(ds, agentsCombined, matcher.New(ds))
})
// Resolves track-1 through the GetEntityByID probe order and on to its artist. Left permissive:
// no spec here asserts how many times the entity is looked up.
stubTrackEntity := func() {
track := model.MediaFile{ID: "track-1", Title: "Track", Artist: "Artist", ArtistID: "artist-1"}
artist := model.Artist{ID: "artist-1", Name: "Artist"}
artistRepo.On("Get", "track-1").Return(nil, model.ErrNotFound).Maybe()
albumRepo.On("Get", "track-1").Return(nil, model.ErrNotFound).Maybe()
mediaFileRepo.On("Get", "track-1").Return(&track, nil).Maybe()
artistRepo.On("Get", "artist-1").Return(&artist, nil).Maybe()
artistRepo.On("GetAll", mock.Anything).Return(model.Artists{artist}, nil).Maybe()
}
Describe("dispatch by entity type", func() {
Context("when ID is a MediaFile (track)", func() {
It("calls GetSimilarSongsByTrack and returns matched songs", func() {
@ -83,7 +94,7 @@ var _ = Describe("Provider - SimilarSongs", func() {
albumRepo.On("Get", "track-1").Return(nil, model.ErrNotFound).Once()
mediaFileRepo.On("Get", "track-1").Return(&track, nil).Once()
agentsCombined.On("GetSimilarSongsByTrack", mock.Anything, "track-1", "Just Can't Get Enough", "Depeche Mode", "track-mbid", 5).
agentsCombined.On("GetSimilarSongsByTrack", mock.Anything, "track-1", "Just Can't Get Enough", "Depeche Mode", "track-mbid", 1).
Return([]agents.Song{
{Name: "Dreaming of Me", MBID: "", Artists: []agents.Artist{{Name: "Depeche Mode", MBID: "artist-mbid"}}},
}, nil).Once()
@ -126,7 +137,7 @@ var _ = Describe("Provider - SimilarSongs", func() {
return false
})).Return(model.MediaFiles{matchedSong}, nil).Maybe()
songs, err := provider.SimilarSongs(ctx, "track-1", 5)
songs, err := provider.SimilarSongs(ctx, "track-1", 1)
Expect(err).ToNot(HaveOccurred())
Expect(songs).To(HaveLen(1))
@ -176,6 +187,76 @@ var _ = Describe("Provider - SimilarSongs", func() {
Expect(songs).To(HaveLen(1))
Expect(songs[0].ID).To(Equal("song-1"))
})
It("tops the mix up with the fallback when the agent's picks alone are too few", func() {
stubTrackEntity()
// The agent knows one track of the three asked for.
agentsCombined.On("GetSimilarSongsByTrack", mock.Anything, "track-1", "Track", "Artist", "", 3).
Return([]agents.Song{{Name: "Agent Pick", MBID: "mbid-agent"}}, nil).Once()
mediaFileRepo.On("GetAll", mock.Anything).
Return(model.MediaFiles{{ID: "agent-1", Title: "Agent Pick", MbzRecordingID: "mbid-agent"}}, nil).Once()
mockAgent.On("GetSimilarArtists", mock.Anything, "artist-1", "Artist", "", 15).
Return([]agents.Artist{}, nil).Once()
mockAgent.On("GetArtistTopSongs", mock.Anything, "artist-1", "Artist", "", mock.Anything).
Return([]agents.Song{{Name: "Song One", MBID: "mbid-1"}}, nil).Once()
mediaFileRepo.On("GetAll", mock.Anything).
Return(model.MediaFiles{{ID: "song-1", Title: "Song One", MbzRecordingID: "mbid-1"}}, nil).Once()
songs, err := provider.SimilarSongs(ctx, "track-1", 3)
Expect(err).ToNot(HaveOccurred())
Expect(ids(songs)).To(ConsistOf("agent-1", "song-1"))
})
It("reaches a unique pick that sits past the count-th repeat", func() {
stubTrackEntity()
mockAgent.On("GetSimilarArtists", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).
Return([]agents.Artist{}, nil).Maybe()
mockAgent.On("GetArtistTopSongs", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).
Return([]agents.Song{}, nil).Maybe()
// "Song B" sits past the second repeat, so a matcher capped at count never reaches it.
repeated := agents.Song{Name: "Song A", MBID: "mbid-a"}
agentsCombined.On("GetSimilarSongsByTrack", mock.Anything, "track-1", "Track", "Artist", "", 2).
Return([]agents.Song{repeated, repeated, {Name: "Song B", MBID: "mbid-b"}}, nil).Once()
mediaFileRepo.On("GetAll", mock.Anything).Return(model.MediaFiles{
{ID: "t-a", Title: "Song A", MbzRecordingID: "mbid-a"},
{ID: "t-b", Title: "Song B", MbzRecordingID: "mbid-b"},
}, nil).Once()
songs, err := provider.SimilarSongs(ctx, "track-1", 2)
Expect(err).ToNot(HaveOccurred())
Expect(ids(songs)).To(ConsistOf("t-a", "t-b"))
})
It("keeps topping up when the agent's picks repeat a track", func() {
stubTrackEntity()
// The matcher re-emits a track when the same input song repeats, so these three
// picks resolve to only two distinct library tracks.
repeated := agents.Song{Name: "Song A", MBID: "mbid-a"}
agentsCombined.On("GetSimilarSongsByTrack", mock.Anything, "track-1", "Track", "Artist", "", 3).
Return([]agents.Song{repeated, repeated, {Name: "Song B", MBID: "mbid-b"}}, nil).Once()
mediaFileRepo.On("GetAll", mock.Anything).Return(model.MediaFiles{
{ID: "t-a", Title: "Song A", MbzRecordingID: "mbid-a"},
{ID: "t-b", Title: "Song B", MbzRecordingID: "mbid-b"},
}, nil).Once()
mockAgent.On("GetSimilarArtists", mock.Anything, "artist-1", "Artist", "", 15).
Return([]agents.Artist{}, nil).Once()
mockAgent.On("GetArtistTopSongs", mock.Anything, "artist-1", "Artist", "", mock.Anything).
Return([]agents.Song{{Name: "Song C", MBID: "mbid-c"}}, nil).Once()
mediaFileRepo.On("GetAll", mock.Anything).
Return(model.MediaFiles{{ID: "t-c", Title: "Song C", MbzRecordingID: "mbid-c"}}, nil).Once()
songs, err := provider.SimilarSongs(ctx, "track-1", 3)
Expect(err).ToNot(HaveOccurred())
Expect(ids(songs)).To(ConsistOf("t-a", "t-b", "t-c"))
})
})
Context("when ID is an Album", func() {
@ -187,7 +268,7 @@ var _ = Describe("Provider - SimilarSongs", func() {
artistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once()
albumRepo.On("Get", "album-1").Return(&album, nil).Once()
agentsCombined.On("GetSimilarSongsByAlbum", mock.Anything, "album-1", "Speak & Spell", "Depeche Mode", "album-mbid", 5).
agentsCombined.On("GetSimilarSongsByAlbum", mock.Anything, "album-1", "Speak & Spell", "Depeche Mode", "album-mbid", 1).
Return([]agents.Song{
{Name: "New Life", MBID: "song-mbid", Artists: []agents.Artist{{Name: "Depeche Mode"}}},
}, nil).Once()
@ -208,7 +289,7 @@ var _ = Describe("Provider - SimilarSongs", func() {
return hasEq
})).Return(model.MediaFiles{matchedSong}, nil).Once()
songs, err := provider.SimilarSongs(ctx, "album-1", 5)
songs, err := provider.SimilarSongs(ctx, "album-1", 1)
Expect(err).ToNot(HaveOccurred())
Expect(songs).To(HaveLen(1))
@ -286,7 +367,7 @@ var _ = Describe("Provider - SimilarSongs", func() {
matchedSong := model.MediaFile{ID: "matched-1", Title: "Enjoy the Silence", Artist: "Depeche Mode", MbzRecordingID: "song-mbid"}
artistRepo.On("Get", "artist-1").Return(&artist, nil).Once()
agentsCombined.On("GetSimilarSongsByArtist", mock.Anything, "artist-1", "Depeche Mode", "artist-mbid", 5).
agentsCombined.On("GetSimilarSongsByArtist", mock.Anything, "artist-1", "Depeche Mode", "artist-mbid", 1).
Return([]agents.Song{
{Name: "Enjoy the Silence", MBID: "song-mbid", Artists: []agents.Artist{{Name: "Depeche Mode"}}},
}, nil).Once()
@ -307,7 +388,7 @@ var _ = Describe("Provider - SimilarSongs", func() {
return hasEq
})).Return(model.MediaFiles{matchedSong}, nil).Once()
songs, err := provider.SimilarSongs(ctx, "artist-1", 5)
songs, err := provider.SimilarSongs(ctx, "artist-1", 1)
Expect(err).ToNot(HaveOccurred())
Expect(songs).To(HaveLen(1))
@ -348,6 +429,72 @@ var _ = Describe("Provider - SimilarSongs", func() {
})
})
Context("when ID is an Artist and the similar-artists fallback can't fill the mix", func() {
It("tops the mix up with the artist's own track-similars", func() {
artist := model.Artist{ID: "ar-1", Name: "Thin Artist"}
topSong := model.MediaFile{ID: "top-1", Title: "Top Song", ArtistID: "ar-1", MbzRecordingID: "mbid-top"}
artistRepo.On("Get", "ar-1").Return(&artist, nil).Maybe()
artistRepo.On("GetAll", mock.Anything).Return(model.Artists{artist}, nil).Maybe()
agentsCombined.On("GetSimilarSongsByArtist", mock.Anything, "ar-1", "Thin Artist", "", 5).
Return([]agents.Song{}, nil).Once()
// No similar artist is in the library, so the fallback yields only the seed artist's
// own matching top song: one track for a mix of five.
mockAgent.On("GetSimilarArtists", mock.Anything, "ar-1", "Thin Artist", "", 15).
Return([]agents.Artist{}, nil).Once()
mockAgent.On("GetArtistTopSongs", mock.Anything, "ar-1", "Thin Artist", "", mock.Anything).
Return([]agents.Song{{Name: "Top Song", MBID: "mbid-top"}}, nil).Once()
mediaFileRepo.On("GetAll", mock.Anything).Return(model.MediaFiles{topSong}, nil).Once()
mediaFileRepo.On("GetRandom", mock.Anything).
Return(model.MediaFiles{{ID: "s1", Title: "Seed", Artist: "Thin Artist"}}, nil).Once()
agentsCombined.On("GetSimilarSongsByTrack", mock.Anything, "s1", "Seed", "Thin Artist", "", mock.Anything).
Return([]agents.Song{{Name: "Mix Song", MBID: "mbid-mix"}}, nil).Once()
mediaFileRepo.On("GetAll", mock.Anything).
Return(model.MediaFiles{{ID: "mix-1", Title: "Mix Song", MbzRecordingID: "mbid-mix"}}, nil).Once()
songs, err := provider.SimilarSongs(ctx, "ar-1", 5)
Expect(err).ToNot(HaveOccurred())
Expect(ids(songs)).To(ConsistOf("top-1", "mix-1"))
})
})
Context("when ID is an Artist and the agent plus the similar-artists fallback already fill the mix", func() {
It("does not pay for seed-track sampling", func() {
artist := model.Artist{ID: "ar-1", Name: "The Artist"}
artistRepo.On("Get", "ar-1").Return(&artist, nil).Maybe()
artistRepo.On("GetAll", mock.Anything).Return(model.Artists{artist}, nil).Maybe()
agentsCombined.On("GetSimilarSongsByArtist", mock.Anything, "ar-1", "The Artist", "", 3).
Return([]agents.Song{{Name: "A", MBID: "mbid-a"}, {Name: "B", MBID: "mbid-b"}}, nil).Once()
mediaFileRepo.On("GetAll", mock.Anything).Return(model.MediaFiles{
{ID: "t-a", Title: "A", MbzRecordingID: "mbid-a"},
{ID: "t-b", Title: "B", MbzRecordingID: "mbid-b"},
}, nil).Once()
// The similar-artists fallback supplies the third track, so the mix is full.
mockAgent.On("GetSimilarArtists", mock.Anything, "ar-1", "The Artist", "", 15).
Return([]agents.Artist{}, nil).Once()
mockAgent.On("GetArtistTopSongs", mock.Anything, "ar-1", "The Artist", "", mock.Anything).
Return([]agents.Song{{Name: "C", MBID: "mbid-c"}}, nil).Once()
mediaFileRepo.On("GetAll", mock.Anything).
Return(model.MediaFiles{{ID: "t-c", Title: "C", MbzRecordingID: "mbid-c"}}, nil).Once()
mediaFileRepo.On("GetRandom", mock.Anything).Return(model.MediaFiles{{ID: "seed"}}, nil).Maybe()
agentsCombined.On("GetSimilarSongsByTrack", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).
Return([]agents.Song{}, nil).Maybe()
songs, err := provider.SimilarSongs(ctx, "ar-1", 3)
Expect(err).ToNot(HaveOccurred())
Expect(ids(songs)).To(ConsistOf("t-a", "t-b", "t-c"))
mediaFileRepo.AssertNotCalled(GinkgoT(), "GetRandom", mock.Anything)
})
})
Context("when ID is a Playlist", func() {
It("samples playlist tracks and returns their track-similars", func() {
pls := model.Playlist{ID: "pl-1", Name: "My List"}
@ -553,7 +700,7 @@ var _ = Describe("Provider - SimilarSongs", func() {
Expect(err).ToNot(HaveOccurred())
Expect(songs).To(HaveLen(3))
ids := slice.Map(songs, func(mf model.MediaFile) string { return mf.ID })
ids := ids(songs)
Expect(ids).To(ContainElement(BeElementOf("b1", "b2")), "seed two must be represented in the mix")
})
@ -750,6 +897,60 @@ var _ = Describe("Provider - SimilarSongs", func() {
}
})
It("keeps picking until the fallback holds count distinct tracks", func() {
// A collaboration in two artists' top songs lands in the chooser twice. Picking a fixed
// count of entries lets those duplicates eat the budget and strand unique candidates.
track := model.MediaFile{ID: "track-1", Title: "Track", Artist: "Artist One", ArtistID: "artist-1"}
artist1 := model.Artist{ID: "artist-1", Name: "Artist One"}
similarArtist := model.Artist{ID: "artist-3", Name: "Similar Artist"}
artistRepo.On("Get", "track-1").Return(nil, model.ErrNotFound).Twice()
albumRepo.On("Get", "track-1").Return(nil, model.ErrNotFound).Twice()
mediaFileRepo.On("Get", "track-1").Return(&track, nil).Twice()
artistRepo.On("Get", "artist-1").Return(&artist1, nil).Maybe()
artistRepo.On("Get", "artist-3").Return(&similarArtist, nil).Maybe()
artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool {
return opt.Max == 1 && opt.Filters != nil
})).Return(model.Artists{artist1}, nil).Maybe()
agentsCombined.On("GetSimilarSongsByTrack", mock.Anything, "track-1", "Track", "Artist One", "", 3).
Return([]agents.Song{}, nil).Once()
mockAgent.On("GetSimilarArtists", mock.Anything, "artist-1", "Artist One", "", 15).
Return([]agents.Artist{{Name: "Similar Artist"}}, nil).Once()
artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool {
_, ok := opt.Filters.(squirrel.Eq)
return opt.Max == 0 && ok
})).Return(model.Artists{}, nil).Once()
artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool {
_, ok := opt.Filters.(squirrel.Or)
return opt.Max == 0 && ok
})).Return(model.Artists{similarArtist}, nil).Once()
shared := model.MediaFile{ID: "t-a", Title: "Shared", MbzRecordingID: "mbid-a"}
mockAgent.On("GetArtistTopSongs", mock.Anything, "artist-1", "Artist One", "", mock.Anything).
Return([]agents.Song{{Name: "Shared", MBID: "mbid-a"}}, nil).Once()
mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).
Return(model.MediaFiles{shared}, nil).Once()
mockAgent.On("GetArtistTopSongs", mock.Anything, "artist-3", "Similar Artist", "", mock.Anything).
Return([]agents.Song{
{Name: "Shared", MBID: "mbid-a"},
{Name: "B", MBID: "mbid-b"},
{Name: "C", MBID: "mbid-c"},
}, nil).Once()
mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{
shared,
{ID: "t-b", Title: "B", MbzRecordingID: "mbid-b"},
{ID: "t-c", Title: "C", MbzRecordingID: "mbid-c"},
}, nil).Once()
songs, err := provider.SimilarSongs(ctx, "track-1", 3)
Expect(err).ToNot(HaveOccurred())
Expect(ids(songs)).To(ConsistOf("t-a", "t-b", "t-c"))
})
It("returns ErrNotFound when artist is not found", func() {
artistRepo.On("Get", "artist-unknown-artist").Return(nil, model.ErrNotFound)
mediaFileRepo.On("Get", "artist-unknown-artist").Return(nil, model.ErrNotFound)
@ -792,7 +993,7 @@ var _ = Describe("Provider - SimilarSongs", func() {
mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1}, nil).Once()
songs, err := provider.SimilarSongs(ctx, "artist-1", 5)
songs, err := provider.SimilarSongs(ctx, "artist-1", 1)
Expect(err).ToNot(HaveOccurred())
Expect(songs).To(HaveLen(1))