mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-08 02:17:25 +02:00
fix(cli): estimate artwork reprocess external lookups per agent, not per item
The reprocess prompt billed one external lookup per externally-capable item. fetchArtistImage/fetchAlbumImage try every enabled image agent and stop early only on a hit, and resolvePlaylist can fetch the m3u image and then resolve up to four sampled albums for the grid, each walking the album agents again. The number the operator confirmed could understate real provider traffic several fold, in the prompt whose whole job is to stop a provider flood. ExternalLookupsPerItem now multiplies by the visible image-agent count and adds the playlist grid factor. It stays a floor: the CLI never calls Manager.Start(), so the plugin registry is empty and plugin-provided agents are dropped by getEnabledAgentNames. On an install with 5 agents of which 3 are plugins the count is well under the truth, so the wording is now "at least N" rather than "up to N" — a zero visible count still bills one lookup for the same reason. Fixing the plugin visibility is out of scope: Manager.Start() needs a Subsonic router and writes to the DB via syncPlugins, breaking this command group's read-only guarantee.
This commit is contained in:
Deluan
parent
08160f3cc0
commit
d2aa486d0c
4 changed files with 138 additions and 30 deletions
|
|
@ -14,6 +14,7 @@ import (
|
|||
|
||||
"github.com/navidrome/navidrome/conf"
|
||||
"github.com/navidrome/navidrome/consts"
|
||||
"github.com/navidrome/navidrome/core/agents"
|
||||
"github.com/navidrome/navidrome/core/artwork"
|
||||
"github.com/navidrome/navidrome/db"
|
||||
"github.com/navidrome/navidrome/log"
|
||||
|
|
@ -273,7 +274,7 @@ func runReprocess(ctx context.Context) {
|
|||
defer db.Init(ctx)()
|
||||
ds, ctx := getAdminContext(ctx)
|
||||
|
||||
if err := reprocessArtwork(ctx, ds, kinds, repositorySources(reprocessSources),
|
||||
if err := reprocessArtwork(ctx, ds, kinds, repositorySources(reprocessSources), imageAgentCount(ds),
|
||||
reprocessDryRun, reprocessConfirm(reprocessYes, os.Stdin), os.Stdout); err != nil {
|
||||
log.Fatal(ctx, err)
|
||||
}
|
||||
|
|
@ -322,13 +323,19 @@ func reprocessConfirm(yes bool, in io.Reader) confirmFunc {
|
|||
return promptConfirm(in)
|
||||
}
|
||||
|
||||
// externalEstimate is an upper bound: a higher-priority local candidate may win before the walk
|
||||
// ever reaches an agent.
|
||||
// externalEstimate is a floor, not a ceiling: plugin-provided agents are unregistered in a CLI that
|
||||
// never starts the plugin manager, so the calls they would add are invisible here.
|
||||
func externalEstimate(n int64) string {
|
||||
if n == 0 {
|
||||
return "none"
|
||||
}
|
||||
return fmt.Sprintf("up to %d", n)
|
||||
return fmt.Sprintf("at least %d", n)
|
||||
}
|
||||
|
||||
// imageAgentCount counts only the built-in image agents, for the same reason.
|
||||
func imageAgentCount(ds model.DataStore) artwork.ImageAgentCount {
|
||||
ag := agents.GetAgents(ds, getPluginManager())
|
||||
return artwork.ImageAgentCount{Artist: len(ag.ArtistImageAgents()), Album: len(ag.AlbumImageAgents())}
|
||||
}
|
||||
|
||||
func promptConfirm(in io.Reader) confirmFunc {
|
||||
|
|
@ -379,7 +386,7 @@ func validateSources(q model.ArtworkQueueRepository, sources []string) error {
|
|||
// reprocessArtwork previews from CountBySource — rows matched — then reports what EnqueueBySource
|
||||
// actually inserted; the two differ because an already-queued row is left untouched.
|
||||
func reprocessArtwork(ctx context.Context, ds model.DataStore, kinds []model.Kind, sources []string,
|
||||
dryRun bool, confirm confirmFunc, out io.Writer) error {
|
||||
imageAgents artwork.ImageAgentCount, dryRun bool, confirm confirmFunc, out io.Writer) error {
|
||||
q := ds.ArtworkQueue(ctx)
|
||||
if err := validateSources(q, sources); err != nil {
|
||||
return err
|
||||
|
|
@ -394,9 +401,7 @@ func reprocessArtwork(ctx context.Context, ds model.DataStore, kinds []model.Kin
|
|||
}
|
||||
matched[i] = n
|
||||
total += n
|
||||
if artwork.MayFetchExternal(k) {
|
||||
external += n
|
||||
}
|
||||
external += n * artwork.ExternalLookupsPerItem(k, imageAgents)
|
||||
}
|
||||
printReprocessPreview(out, kinds, matched, total, external, sources)
|
||||
|
||||
|
|
|
|||
|
|
@ -269,10 +269,10 @@ var _ = Describe("promptConfirm", func() {
|
|||
|
||||
BeforeEach(func() { out.Reset() })
|
||||
|
||||
It("states the external cost and accepts an explicit yes", func() {
|
||||
It("states the external cost as a floor and accepts an explicit yes", func() {
|
||||
Expect(promptConfirm(strings.NewReader("y\n"))(&out, 42, 7)).To(BeTrue())
|
||||
Expect(out.String()).To(ContainSubstring("re-resolve 42 items"))
|
||||
Expect(out.String()).To(ContainSubstring("7 external lookups"))
|
||||
Expect(out.String()).To(ContainSubstring("at least 7 external lookups"))
|
||||
})
|
||||
|
||||
It("defaults to no on anything else", func() {
|
||||
|
|
@ -309,6 +309,7 @@ var _ = Describe("reprocessArtwork", func() {
|
|||
var art *tests.MockArtworkRepo
|
||||
var queue *tests.MockArtworkQueueRepo
|
||||
var out strings.Builder
|
||||
var imageAgents artwork.ImageAgentCount
|
||||
ctx := context.Background()
|
||||
kinds := []model.Kind{model.KindArtistArtwork, model.KindAlbumArtwork}
|
||||
accept := func(io.Writer, int64, int64) bool { return true }
|
||||
|
|
@ -324,6 +325,7 @@ var _ = Describe("reprocessArtwork", func() {
|
|||
conf.Server.CoverArtPriority = "cover.*, external"
|
||||
conf.Server.ArtistArtPriority = "artist.*, external"
|
||||
conf.Server.EnableM3UExternalAlbumArt = false
|
||||
imageAgents = artwork.ImageAgentCount{Artist: 1, Album: 1}
|
||||
ds = &tests.MockDataStore{}
|
||||
art = ds.Artwork(ctx).(*tests.MockArtworkRepo)
|
||||
queue = ds.ArtworkQueue(ctx).(*tests.MockArtworkQueueRepo)
|
||||
|
|
@ -335,7 +337,7 @@ var _ = Describe("reprocessArtwork", func() {
|
|||
})
|
||||
|
||||
It("previews the per-kind breakdown and queues nothing on a dry run", func() {
|
||||
Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, true, accept, &out)).To(Succeed())
|
||||
Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, imageAgents, true, accept, &out)).To(Succeed())
|
||||
|
||||
Expect(out.String()).To(ContainSubstring("external:deezer"))
|
||||
Expect(out.String()).To(ContainSubstring("artist"))
|
||||
|
|
@ -346,14 +348,14 @@ var _ = Describe("reprocessArtwork", func() {
|
|||
})
|
||||
|
||||
It("queues nothing when the operator declines", func() {
|
||||
Expect(reprocessArtwork(ctx, ds, kinds, nil, false, decline, &out)).To(Succeed())
|
||||
Expect(reprocessArtwork(ctx, ds, kinds, nil, imageAgents, false, decline, &out)).To(Succeed())
|
||||
|
||||
Expect(out.String()).To(ContainSubstring("Aborted"))
|
||||
Expect(queue.Count()).To(BeZero())
|
||||
})
|
||||
|
||||
It("queues the matching items at recheck priority, leaving their artwork state alone", func() {
|
||||
Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, false, accept, &out)).To(Succeed())
|
||||
Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, imageAgents, false, accept, &out)).To(Succeed())
|
||||
|
||||
Expect(queue.Count()).To(Equal(int64(2)))
|
||||
queued, err := queue.Get(model.KindAlbumArtwork, "al-1", model.ImageTypePrimary)
|
||||
|
|
@ -368,7 +370,7 @@ var _ = Describe("reprocessArtwork", func() {
|
|||
})
|
||||
|
||||
It("targets the absent state", func() {
|
||||
Expect(reprocessArtwork(ctx, ds, kinds, []string{""}, false, accept, &out)).To(Succeed())
|
||||
Expect(reprocessArtwork(ctx, ds, kinds, []string{""}, imageAgents, false, accept, &out)).To(Succeed())
|
||||
|
||||
Expect(queue.Count()).To(Equal(int64(1)))
|
||||
_, err := queue.Get(model.KindArtistArtwork, "ar-2", model.ImageTypePrimary)
|
||||
|
|
@ -379,7 +381,7 @@ var _ = Describe("reprocessArtwork", func() {
|
|||
Expect(queue.Enqueue(model.ArtworkQueueItem{ItemKind: "ar", ItemID: "ar-1",
|
||||
ImageType: model.ImageTypePrimary, Priority: model.ArtworkPriorityBump})).To(Succeed())
|
||||
|
||||
Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, false, accept, &out)).To(Succeed())
|
||||
Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, imageAgents, false, accept, &out)).To(Succeed())
|
||||
|
||||
Expect(out.String()).To(ContainSubstring("Queued 1 of 2 matched items"))
|
||||
Expect(out.String()).To(ContainSubstring("Already queued, left unchanged: 1"))
|
||||
|
|
@ -390,7 +392,7 @@ var _ = Describe("reprocessArtwork", func() {
|
|||
})
|
||||
|
||||
It("stops at a selection that matches nothing instead of prompting", func() {
|
||||
Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindRadioArtwork}, nil, false,
|
||||
Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindRadioArtwork}, nil, imageAgents, false,
|
||||
func(io.Writer, int64, int64) bool {
|
||||
Fail("must not prompt when there is nothing to queue")
|
||||
return true
|
||||
|
|
@ -401,23 +403,41 @@ var _ = Describe("reprocessArtwork", func() {
|
|||
})
|
||||
|
||||
It("reports an empty selection as a dry run when one was asked for", func() {
|
||||
Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindRadioArtwork}, nil, true, accept, &out)).To(Succeed())
|
||||
Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindRadioArtwork}, nil, imageAgents, true, accept, &out)).To(Succeed())
|
||||
|
||||
Expect(out.String()).To(ContainSubstring("Nothing matches"))
|
||||
Expect(out.String()).To(ContainSubstring("Dry run"))
|
||||
})
|
||||
|
||||
It("shows the external estimate on a dry run, which never reaches the prompt", func() {
|
||||
Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindAlbumArtwork}, nil, true, accept, &out)).To(Succeed())
|
||||
Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindAlbumArtwork}, nil, imageAgents, true, accept, &out)).To(Succeed())
|
||||
|
||||
Expect(out.String()).To(ContainSubstring("External lookups: up to 2"))
|
||||
Expect(out.String()).To(ContainSubstring("External lookups: at least 2"))
|
||||
})
|
||||
|
||||
It("bills every agent per item, not one lookup per item", func() {
|
||||
imageAgents = artwork.ImageAgentCount{Artist: 2, Album: 3}
|
||||
var external int64
|
||||
capture := func(_ io.Writer, _, e int64) bool { external = e; return false }
|
||||
|
||||
Expect(reprocessArtwork(ctx, ds, kinds, nil, imageAgents, false, capture, &out)).To(Succeed())
|
||||
|
||||
Expect(external).To(Equal(int64(2*2+2*3)), "2 artists at 2 agents plus 2 albums at 3 agents")
|
||||
Expect(out.String()).To(ContainSubstring("External lookups: at least 10"))
|
||||
})
|
||||
|
||||
It("states the estimate as a floor, since plugin agents are invisible to the CLI", func() {
|
||||
Expect(reprocessArtwork(ctx, ds, kinds, nil, imageAgents, true, accept, &out)).To(Succeed())
|
||||
|
||||
Expect(out.String()).To(ContainSubstring("External lookups: at least"))
|
||||
Expect(out.String()).ToNot(ContainSubstring("up to"))
|
||||
})
|
||||
|
||||
It("says so when the selection needs no external lookup", func() {
|
||||
conf.Server.CoverArtPriority = "cover.*"
|
||||
put(model.KindPlaylistArtwork, "pl-1", "playlist")
|
||||
|
||||
Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, true, accept, &out)).To(Succeed())
|
||||
Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, imageAgents, true, accept, &out)).To(Succeed())
|
||||
|
||||
Expect(out.String()).To(ContainSubstring("External lookups: none"))
|
||||
})
|
||||
|
|
@ -429,20 +449,22 @@ var _ = Describe("reprocessArtwork", func() {
|
|||
var external int64
|
||||
capture := func(_ io.Writer, _, e int64) bool { external = e; return false }
|
||||
|
||||
Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, false, capture, &out)).To(Succeed())
|
||||
Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, imageAgents, false, capture, &out)).To(Succeed())
|
||||
|
||||
Expect(external).To(Equal(int64(1)))
|
||||
Expect(out.String()).To(ContainSubstring("External lookups: up to 1"))
|
||||
Expect(out.String()).To(ContainSubstring("External lookups: at least 1"))
|
||||
})
|
||||
|
||||
It("counts playlists as external cost when their grid tiles walk an external album chain", func() {
|
||||
It("bills a playlist for every album its grid samples, at every agent", func() {
|
||||
imageAgents = artwork.ImageAgentCount{Album: 3}
|
||||
put(model.KindPlaylistArtwork, "pl-1", "playlist")
|
||||
var external int64
|
||||
capture := func(_ io.Writer, _, e int64) bool { external = e; return false }
|
||||
|
||||
Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, false, capture, &out)).To(Succeed())
|
||||
Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, imageAgents, false, capture, &out)).To(Succeed())
|
||||
|
||||
Expect(external).To(Equal(int64(1)), "sampling album art for the grid reaches the agents")
|
||||
Expect(external).To(Equal(int64(artwork.PlaylistGridSamples*3)),
|
||||
"one playlist samples 4 albums, each walking all 3 album agents")
|
||||
})
|
||||
|
||||
It("counts only the kinds that call an external agent as external cost", func() {
|
||||
|
|
@ -451,14 +473,14 @@ var _ = Describe("reprocessArtwork", func() {
|
|||
capture := func(_ io.Writer, t, e int64) bool { total, external = t, e; return false }
|
||||
|
||||
Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindAlbumArtwork, model.KindRadioArtwork},
|
||||
nil, false, capture, &out)).To(Succeed())
|
||||
nil, imageAgents, false, capture, &out)).To(Succeed())
|
||||
|
||||
Expect(total).To(Equal(int64(3)))
|
||||
Expect(external).To(Equal(int64(2)), "radio artwork never reaches an external agent")
|
||||
})
|
||||
|
||||
It("rejects an unknown source and names the ones in use", func() {
|
||||
err := reprocessArtwork(ctx, ds, kinds, []string{"externa:deezer"}, true, accept, &out)
|
||||
err := reprocessArtwork(ctx, ds, kinds, []string{"externa:deezer"}, imageAgents, true, accept, &out)
|
||||
|
||||
Expect(err).To(HaveOccurred())
|
||||
Expect(err.Error()).To(ContainSubstring("externa:deezer"))
|
||||
|
|
@ -470,7 +492,7 @@ var _ = Describe("reprocessArtwork", func() {
|
|||
|
||||
It("accepts a source another kind uses, letting the empty selection report itself", func() {
|
||||
Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindArtistArtwork}, []string{"folder"},
|
||||
false, decline, &out)).To(Succeed())
|
||||
imageAgents, false, decline, &out)).To(Succeed())
|
||||
|
||||
Expect(out.String()).To(ContainSubstring("Nothing matches"),
|
||||
"a well-formed filter must not be reported as a typo because of the kinds selected")
|
||||
|
|
|
|||
|
|
@ -129,6 +129,33 @@ func MayFetchExternal(kind model.Kind) bool {
|
|||
}
|
||||
}
|
||||
|
||||
// ImageAgentCount is how many enabled agents provide artist and album images.
|
||||
type ImageAgentCount struct{ Artist, Album int }
|
||||
|
||||
// ExternalLookupsPerItem reports what resolving one item of this kind can cost: every image agent is
|
||||
// tried, and a zero count still bills one, so agents the caller cannot see never read as free.
|
||||
func ExternalLookupsPerItem(kind model.Kind, agents ImageAgentCount) int64 {
|
||||
if !MayFetchExternal(kind) {
|
||||
return 0
|
||||
}
|
||||
switch kind {
|
||||
case model.KindArtistArtwork:
|
||||
return int64(max(agents.Artist, 1))
|
||||
case model.KindAlbumArtwork:
|
||||
return int64(max(agents.Album, 1))
|
||||
case model.KindPlaylistArtwork:
|
||||
var n int64
|
||||
if conf.Server.EnableM3UExternalAlbumArt {
|
||||
n++
|
||||
}
|
||||
if chainFetchesExternal(conf.Server.CoverArtPriority) {
|
||||
n += PlaylistGridSamples * int64(max(agents.Album, 1))
|
||||
}
|
||||
return n
|
||||
}
|
||||
return 0
|
||||
}
|
||||
|
||||
func chainFetchesExternal(priority string) bool {
|
||||
for pattern := range strings.SplitSeq(strings.ToLower(priority), ",") {
|
||||
if strings.TrimSpace(pattern) == externalCandidate {
|
||||
|
|
@ -286,6 +313,9 @@ func (r *resolver) resolveArtist(ctx context.Context, artistID string) (resoluti
|
|||
return chain.exhausted(), nil
|
||||
}
|
||||
|
||||
// PlaylistGridSamples is how many albums resolvePlaylist samples to build the generated grid.
|
||||
const PlaylistGridSamples = 4
|
||||
|
||||
// resolvePlaylist tries the uploaded image, the sidecar and ExternalImageURL, then a generated grid.
|
||||
func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (resolution, error) {
|
||||
pl, err := r.ds.Playlist(ctx).Get(playlistID)
|
||||
|
|
@ -331,7 +361,8 @@ func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (reso
|
|||
}
|
||||
}
|
||||
|
||||
albumIDs, err := r.ds.Playlist(ctx).Tracks(pl.ID, false).GetAlbumIDs(model.QueryOptions{Max: 4, Sort: "random()"})
|
||||
albumIDs, err := r.ds.Playlist(ctx).Tracks(pl.ID, false).
|
||||
GetAlbumIDs(model.QueryOptions{Max: PlaylistGridSamples, Sort: "random()"})
|
||||
if err != nil {
|
||||
return resolution{}, err
|
||||
}
|
||||
|
|
@ -357,7 +388,7 @@ func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (reso
|
|||
if decErr == nil {
|
||||
tiles = append(tiles, tile)
|
||||
}
|
||||
if len(tiles) == 4 {
|
||||
if len(tiles) == PlaylistGridSamples {
|
||||
break
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -707,3 +707,53 @@ var _ = Describe("MayFetchExternal", func() {
|
|||
Expect(MayFetchExternal(model.KindMediaFileArtwork)).To(BeFalse())
|
||||
})
|
||||
})
|
||||
|
||||
var _ = Describe("ExternalLookupsPerItem", func() {
|
||||
count := ImageAgentCount{Artist: 3, Album: 2}
|
||||
|
||||
BeforeEach(func() {
|
||||
DeferCleanup(configtest.SetupConfig())
|
||||
conf.Server.CoverArtPriority = "cover.*, external"
|
||||
conf.Server.ArtistArtPriority = "artist.*, external"
|
||||
conf.Server.EnableM3UExternalAlbumArt = false
|
||||
})
|
||||
|
||||
It("bills one call per agent, since the walk only stops early on a hit", func() {
|
||||
Expect(ExternalLookupsPerItem(model.KindArtistArtwork, count)).To(Equal(int64(3)))
|
||||
Expect(ExternalLookupsPerItem(model.KindAlbumArtwork, count)).To(Equal(int64(2)))
|
||||
})
|
||||
|
||||
It("bills a playlist for every album its grid samples", func() {
|
||||
Expect(ExternalLookupsPerItem(model.KindPlaylistArtwork, count)).
|
||||
To(Equal(int64(PlaylistGridSamples) * 2))
|
||||
})
|
||||
|
||||
It("adds the m3u image fetch on top of the grid", func() {
|
||||
conf.Server.EnableM3UExternalAlbumArt = true
|
||||
Expect(ExternalLookupsPerItem(model.KindPlaylistArtwork, count)).
|
||||
To(Equal(int64(PlaylistGridSamples)*2 + 1))
|
||||
})
|
||||
|
||||
It("bills only the m3u fetch when the album chain stays local", func() {
|
||||
conf.Server.CoverArtPriority = "cover.*"
|
||||
conf.Server.EnableM3UExternalAlbumArt = true
|
||||
Expect(ExternalLookupsPerItem(model.KindPlaylistArtwork, count)).To(Equal(int64(1)))
|
||||
})
|
||||
|
||||
It("still bills a call when no agent is visible, which plugins never are offline", func() {
|
||||
none := ImageAgentCount{}
|
||||
Expect(ExternalLookupsPerItem(model.KindArtistArtwork, none)).To(Equal(int64(1)))
|
||||
Expect(ExternalLookupsPerItem(model.KindAlbumArtwork, none)).To(Equal(int64(1)))
|
||||
Expect(ExternalLookupsPerItem(model.KindPlaylistArtwork, none)).
|
||||
To(Equal(int64(PlaylistGridSamples)))
|
||||
})
|
||||
|
||||
It("is zero whenever the kind reaches no agent at all", func() {
|
||||
conf.Server.CoverArtPriority = "cover.*"
|
||||
conf.Server.ArtistArtPriority = "artist.*"
|
||||
Expect(ExternalLookupsPerItem(model.KindArtistArtwork, count)).To(BeZero())
|
||||
Expect(ExternalLookupsPerItem(model.KindAlbumArtwork, count)).To(BeZero())
|
||||
Expect(ExternalLookupsPerItem(model.KindPlaylistArtwork, count)).To(BeZero())
|
||||
Expect(ExternalLookupsPerItem(model.KindRadioArtwork, count)).To(BeZero())
|
||||
})
|
||||
})
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue