perf(explore): make the owned-artist backfill yield, mark, and stop
The post-scan backfills share MusicBrainz's rate limiters with every page the user can open, and both were FIFO — so a thousand-artist enrichment put an album page behind an hour of queued work. WithBackgroundLane/WithBackgroundPriority add a slower second lane: a marked wait takes no token while any interactive wait is outstanding. It is a context marker rather than a parameter because a backfill calls the same client methods a detail page does. A long backfill also has to be visible and stoppable, so jobs.KindCatalogEnrich registers both with progress and cancel — after the work is counted, since these passes are a no-op on every launch once the library is covered. What it does not fetch is the point. It ran for hours against a 900-artist library and marked nothing, because three of the four things it did per artist were work nobody asked for: similar artists, which the artist page already resolves on view, and a full GetArtistImage (fanart.tv, TheAudioDB, Wikidata, Wikipedia, ten portraits) reached only to warm the MB artist lookup EnsureArtistRels does alone. It was also serial across artists while every limiter is per-host and idle. The marks are a table rather than more explore_index columns, because artifactimport merges by column list and a flag added there is a second place to remember. BrowseReleaseGroupsAll pages to exhaustion, where the old call silently cut a prolific artist at 100 release groups. One portrait is downloaded now; the rest are remembered as URLs. resolveAllSources downloaded every candidate, up to ten, full size, while nothing reads anything but primary.jpg — 5.3 GB measured on a real cache, 4.1 GB of it unreachable. OrphanedArtistImagesJob is why that survived: it joined the bare MBID onto the images directory, but artist directories are sharded under a two-character prefix, so it named a path that never existed and deleted the rows that were the only record of the files it left behind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDCbcCZQepnpSQYJ6SxxZm
This commit is contained in:
+17
-1
@@ -182,6 +182,16 @@ func NewYellowJacketApp(
|
||||
yjApp.library.SetJobRegistry(yjApp.jobs)
|
||||
yjApp.explore.SetJobRegistry(yjApp.jobs)
|
||||
|
||||
// Let the release prefetch skip albums the user already owns in
|
||||
// full — those open with no catalog call at all, so warming their
|
||||
// tracklists spends the most expensive request in the app on
|
||||
// nothing. Injected because neither package imports the other.
|
||||
yjApp.explore.SetAlbumComplete(func(albumID int64) bool {
|
||||
c, err := yjApp.library.GetAlbumCompleteness(albumID)
|
||||
|
||||
return err == nil && c.Known && c.Complete
|
||||
})
|
||||
|
||||
// create autotag service (depends on explore + tagWriter)
|
||||
yjApp.autotag = autotagservice.NewService(
|
||||
yjApp.logger.WithGroup("autotag"),
|
||||
@@ -697,7 +707,13 @@ func (yj *YellowJacketApp) startJanitor() {
|
||||
yj.database, coversDir, library.CoverArtFileSet,
|
||||
))
|
||||
yj.janitor.Register(maintenance.OrphanedArtistImagesJob(
|
||||
yj.database, filepath.Join(dataDir, explore.ArtistImageDirName),
|
||||
yj.database,
|
||||
filepath.Join(dataDir, explore.ArtistImageDirName),
|
||||
explore.ArtistImageDir,
|
||||
))
|
||||
yj.janitor.Register(maintenance.StrayArtistImageFilesJob(
|
||||
filepath.Join(dataDir, explore.ArtistImageDirName),
|
||||
explore.ArtistImageKeepNames(),
|
||||
))
|
||||
yj.janitor.Register(maintenance.ExpiredProxyCacheJob(
|
||||
filepath.Join(dataDir, explore.CoverArtCacheDirName),
|
||||
|
||||
@@ -193,7 +193,26 @@ var exploreIndexFTSTriggers = []string{
|
||||
INSERT INTO explore_index_fts(explore_index_fts, rowid, title, artist_name, aliases)
|
||||
VALUES ('delete', old.id, old.title, old.artist_name, old.aliases);
|
||||
END`,
|
||||
`CREATE TRIGGER explore_index_au AFTER UPDATE ON explore_index BEGIN
|
||||
// Narrowed to the three columns the FTS table actually indexes, and
|
||||
// guarded on them having changed. An UPDATE that leaves all three
|
||||
// alone has nothing to re-index, and re-indexing it is not free: an
|
||||
// FTS5 delete has to find the old row's postings in a multi-million
|
||||
// row index, which is the ~31 rows/s figure below.
|
||||
//
|
||||
// This is not a micro-optimisation. Every writer here upserts, and
|
||||
// the merge rules keep existing values (`CASE WHEN excluded.title
|
||||
// != '' ...`), so the common write is a row arriving unchanged: the
|
||||
// discography backfill re-browsing a known artist, the incremental
|
||||
// dump refreshing popularity. Each of those used to pay a full
|
||||
// delete + insert against the FTS index while holding the single
|
||||
// writer connection — measured at 91% of the app's CPU, with the
|
||||
// play path queued behind it.
|
||||
`CREATE TRIGGER explore_index_au AFTER UPDATE OF title, artist_name, aliases
|
||||
ON explore_index
|
||||
WHEN old.title IS NOT new.title
|
||||
OR old.artist_name IS NOT new.artist_name
|
||||
OR old.aliases IS NOT new.aliases
|
||||
BEGIN
|
||||
INSERT INTO explore_index_fts(explore_index_fts, rowid, title, artist_name, aliases)
|
||||
VALUES ('delete', old.id, old.title, old.artist_name, old.aliases);
|
||||
INSERT INTO explore_index_fts(rowid, title, artist_name, aliases)
|
||||
@@ -203,7 +222,17 @@ var exploreIndexFTSTriggers = []string{
|
||||
|
||||
// createExploreIndexFTSTriggers installs the sync triggers. Safe to
|
||||
// call on a database that already has them.
|
||||
//
|
||||
// It drops first rather than tolerating "already exists", because a
|
||||
// trigger is a definition and not a row: an install that already has
|
||||
// the old one would otherwise keep it forever, and these definitions
|
||||
// are exactly where this table's write cost is decided. Three DDL
|
||||
// statements against a table with no rows to rewrite, on open.
|
||||
func createExploreIndexFTSTriggers(ctx context.Context, db *sql.DB) error {
|
||||
if err := dropExploreIndexFTSTriggers(ctx, db); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
for _, stmt := range exploreIndexFTSTriggers {
|
||||
if _, err := db.ExecContext(ctx, stmt); err != nil &&
|
||||
!strings.Contains(err.Error(), "already exists") {
|
||||
@@ -214,6 +243,23 @@ func createExploreIndexFTSTriggers(ctx context.Context, db *sql.DB) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
// exploreIndexFTSTriggerNames is what both the drop paths remove.
|
||||
var exploreIndexFTSTriggerNames = []string{
|
||||
"explore_index_ai", "explore_index_ad", "explore_index_au",
|
||||
}
|
||||
|
||||
func dropExploreIndexFTSTriggers(ctx context.Context, db *sql.DB) error {
|
||||
for _, name := range exploreIndexFTSTriggerNames {
|
||||
if _, err := db.ExecContext(
|
||||
ctx, "DROP TRIGGER IF EXISTS "+name,
|
||||
); err != nil {
|
||||
return fmt.Errorf("drop explore FTS trigger %s: %w", name, err)
|
||||
}
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// SuspendExploreIndexFTS drops the FTS sync triggers so a bulk load can
|
||||
// write explore_index without paying per-row FTS maintenance.
|
||||
//
|
||||
@@ -227,12 +273,8 @@ func createExploreIndexFTSTriggers(ctx context.Context, db *sql.DB) error {
|
||||
// Callers MUST pair this with ResumeExploreIndexFTS — while suspended,
|
||||
// explore_index_fts stops tracking the table and search goes stale.
|
||||
func (d *DB) SuspendExploreIndexFTS() error {
|
||||
for _, name := range []string{
|
||||
"explore_index_ai", "explore_index_ad", "explore_index_au",
|
||||
} {
|
||||
if _, err := d.db.ExecContext(d.Ctx, "DROP TRIGGER IF EXISTS "+name); err != nil {
|
||||
return fmt.Errorf("suspend explore FTS: drop %s: %w", name, err)
|
||||
}
|
||||
if err := dropExploreIndexFTSTriggers(d.Ctx, d.db); err != nil {
|
||||
return fmt.Errorf("suspend explore FTS: %w", err)
|
||||
}
|
||||
|
||||
return nil
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
package database
|
||||
|
||||
import (
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
@@ -157,3 +158,160 @@ func TestExploreFTSSuspendIsIdempotent(t *testing.T) {
|
||||
t.Fatalf("resume: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// ftsSegmentCount reports how much the FTS index itself has been
|
||||
// written to. Every delete + insert the update trigger performs
|
||||
// appends to the shadow content table, so this is the observable that
|
||||
// tells "the trigger re-indexed the row" from "the trigger declined
|
||||
// to". Search results cannot: a no-op re-index leaves the same
|
||||
// matches behind.
|
||||
func ftsSegmentCount(t *testing.T, db *DB) int {
|
||||
t.Helper()
|
||||
|
||||
rows, err := db.QueryContext("SELECT COUNT(*) FROM explore_index_fts_data")
|
||||
if err != nil {
|
||||
t.Fatalf("fts data count: %v", err)
|
||||
}
|
||||
|
||||
defer func() { _ = rows.Close() }()
|
||||
|
||||
n := 0
|
||||
|
||||
if rows.Next() {
|
||||
if err := rows.Scan(&n); err != nil {
|
||||
t.Fatalf("scan fts data count: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
return n
|
||||
}
|
||||
|
||||
// The common write in this schema is an upsert whose merge rules keep
|
||||
// every existing value — the discography backfill re-browsing a known
|
||||
// artist, the incremental dump refreshing popularity. Re-indexing
|
||||
// those cost an FTS5 delete against a multi-million row index while
|
||||
// holding the single writer connection, which is what starved the
|
||||
// playback path. An update that leaves title, artist_name and aliases
|
||||
// alone must not touch the FTS index at all.
|
||||
func TestExploreFTSUpdateSkipsUnchangedText(t *testing.T) {
|
||||
db := NewTestDB(t)
|
||||
|
||||
seedExploreRow(t, db, "mbid-1", "Unchanged Title", "Steady Artist")
|
||||
|
||||
before := ftsSegmentCount(t, db)
|
||||
|
||||
// A popularity refresh: an FTS column is not named at all.
|
||||
if _, err := db.ExecContext(
|
||||
"UPDATE explore_index SET popularity = 42 WHERE mbid = 'mbid-1'",
|
||||
); err != nil {
|
||||
t.Fatalf("popularity update: %v", err)
|
||||
}
|
||||
|
||||
// An upsert-shaped write that re-states the text identically, which
|
||||
// is what the merge rules produce for a row that has not changed.
|
||||
if _, err := db.ExecContext(`
|
||||
UPDATE explore_index
|
||||
SET title = 'Unchanged Title', artist_name = 'Steady Artist', popularity = 43
|
||||
WHERE mbid = 'mbid-1'
|
||||
`); err != nil {
|
||||
t.Fatalf("no-op text update: %v", err)
|
||||
}
|
||||
|
||||
if got := ftsSegmentCount(t, db); got != before {
|
||||
t.Errorf(
|
||||
"FTS index written by an update that changed no text: %d rows, want %d",
|
||||
got, before,
|
||||
)
|
||||
}
|
||||
|
||||
if got := ftsMatches(t, db, "Unchanged"); got != 1 {
|
||||
t.Errorf("matches after unchanged updates = %d, want 1", got)
|
||||
}
|
||||
}
|
||||
|
||||
// The other half of the same guard: a real rename still re-indexes,
|
||||
// old term gone and new term found.
|
||||
func TestExploreFTSUpdateReindexesChangedText(t *testing.T) {
|
||||
db := NewTestDB(t)
|
||||
|
||||
seedExploreRow(t, db, "mbid-2", "Original Title", "Some Artist")
|
||||
|
||||
if _, err := db.ExecContext(
|
||||
"UPDATE explore_index SET title = 'Corrected Title' WHERE mbid = 'mbid-2'",
|
||||
); err != nil {
|
||||
t.Fatalf("rename: %v", err)
|
||||
}
|
||||
|
||||
if got := ftsMatches(t, db, "Original"); got != 0 {
|
||||
t.Errorf("matches for the old title = %d, want 0", got)
|
||||
}
|
||||
|
||||
if got := ftsMatches(t, db, "Corrected"); got != 1 {
|
||||
t.Errorf("matches for the new title = %d, want 1", got)
|
||||
}
|
||||
|
||||
// The same for the other two indexed columns.
|
||||
if _, err := db.ExecContext(
|
||||
"UPDATE explore_index SET artist_name = 'Renamed Artist', aliases = 'AKA Thing' WHERE mbid = 'mbid-2'",
|
||||
); err != nil {
|
||||
t.Fatalf("artist rename: %v", err)
|
||||
}
|
||||
|
||||
if got := ftsMatches(t, db, "Renamed"); got != 1 {
|
||||
t.Errorf("matches for the new artist = %d, want 1", got)
|
||||
}
|
||||
|
||||
if got := ftsMatches(t, db, "AKA"); got != 1 {
|
||||
t.Errorf("matches for the new alias = %d, want 1", got)
|
||||
}
|
||||
}
|
||||
|
||||
// An existing install already carries the previous, unguarded trigger,
|
||||
// and a create that tolerated "already exists" would leave it there
|
||||
// forever — so the definition has to be replaced on open, not merely
|
||||
// offered.
|
||||
func TestExploreFTSTriggersAreReplacedOnOpen(t *testing.T) {
|
||||
db := NewTestDB(t)
|
||||
|
||||
if err := db.SuspendExploreIndexFTS(); err != nil {
|
||||
t.Fatalf("suspend: %v", err)
|
||||
}
|
||||
|
||||
// The shape that shipped before: fires on every UPDATE.
|
||||
if _, err := db.ExecContext(`
|
||||
CREATE TRIGGER explore_index_au AFTER UPDATE ON explore_index BEGIN
|
||||
INSERT INTO explore_index_fts(explore_index_fts, rowid, title, artist_name, aliases)
|
||||
VALUES ('delete', old.id, old.title, old.artist_name, old.aliases);
|
||||
INSERT INTO explore_index_fts(rowid, title, artist_name, aliases)
|
||||
VALUES (new.id, new.title, new.artist_name, new.aliases);
|
||||
END
|
||||
`); err != nil {
|
||||
t.Fatalf("install old trigger: %v", err)
|
||||
}
|
||||
|
||||
if err := createExploreIndexFTSTriggers(db.Ctx, db.db); err != nil {
|
||||
t.Fatalf("recreate triggers: %v", err)
|
||||
}
|
||||
|
||||
rows, err := db.QueryContext(
|
||||
"SELECT sql FROM sqlite_master WHERE type = 'trigger' AND name = 'explore_index_au'",
|
||||
)
|
||||
if err != nil {
|
||||
t.Fatalf("read trigger sql: %v", err)
|
||||
}
|
||||
|
||||
defer func() { _ = rows.Close() }()
|
||||
|
||||
definition := ""
|
||||
|
||||
if rows.Next() {
|
||||
if err := rows.Scan(&definition); err != nil {
|
||||
t.Fatalf("scan trigger sql: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
if !strings.Contains(definition, "UPDATE OF") ||
|
||||
!strings.Contains(definition, "WHEN") {
|
||||
t.Errorf("explore_index_au was not replaced; definition is:\n%s", definition)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,31 @@
|
||||
-- Per-artist record of which catalog enrichment passes have completed,
|
||||
-- so the owned-artist backfill knows what is left to do.
|
||||
--
|
||||
-- It is a table rather than more flag columns on `explore_index` for one
|
||||
-- reason: the downloaded catalog artifact is merged into that table by
|
||||
-- column list (see artifactimport.go), so a flag added there is a second
|
||||
-- place to remember, and forgetting it silently wipes every mark on the
|
||||
-- next catalog update. These marks are about *this install's* fetching,
|
||||
-- which the artifact knows nothing about.
|
||||
--
|
||||
-- Each column is a separate fetch with its own failure mode, which is
|
||||
-- why they are not one boolean: an MB browse failing must not claim the
|
||||
-- similar-artists fetch, or vice versa. NULL means "not done" — the
|
||||
-- timestamp is for debugging and for any future re-fetch policy, not
|
||||
-- for expiry. Nothing expires these today.
|
||||
--
|
||||
-- `explore_index.discog_fetched` is deliberately NOT duplicated here: it
|
||||
-- means "this artist's top release groups and recordings are present",
|
||||
-- which the artifact legitimately answers for artists it covers.
|
||||
|
||||
CREATE TABLE IF NOT EXISTS artist_enrichment (
|
||||
artist_mbid TEXT PRIMARY KEY,
|
||||
|
||||
-- The full MusicBrainz browse-by-artist landed: every release group,
|
||||
-- with primary and secondary types. ListenBrainz's top-release-groups
|
||||
-- endpoint gives neither the tail nor the types.
|
||||
browsed_at DATETIME,
|
||||
|
||||
-- similar_artist_map has been filled for this artist.
|
||||
similar_at DATETIME
|
||||
);
|
||||
@@ -26,6 +26,12 @@ type ArtistCreditArtist struct {
|
||||
CreditID int64
|
||||
}
|
||||
|
||||
type ArtistEnrichment struct {
|
||||
ArtistMbid string
|
||||
BrowsedAt sql.NullTime
|
||||
SimilarAt sql.NullTime
|
||||
}
|
||||
|
||||
type ArtistImage struct {
|
||||
ID int64
|
||||
ArtistMbid string
|
||||
|
||||
@@ -113,6 +113,14 @@ var tables = []Table{
|
||||
Name: "artist_credit_artist", Kind: Owned, Lifetime: Swept,
|
||||
Note: "Join table between credits and artists.",
|
||||
},
|
||||
{
|
||||
Name: "artist_enrichment", Kind: Derived, Lifetime: Retained,
|
||||
Note: "Which catalog enrichment passes have run for an artist. " +
|
||||
"Derived: losing it re-runs the fetches, which cost time and " +
|
||||
"someone else's rate limit but no data. Retained because a " +
|
||||
"stale mark for an artist no longer owned is harmless — the " +
|
||||
"backfill only ever asks about artists the library has.",
|
||||
},
|
||||
{
|
||||
Name: "artist_images", Kind: Cache, Lifetime: Swept,
|
||||
Note: "Artist photos from fanart.tv/MusicBrainz. Rows point at " +
|
||||
|
||||
@@ -0,0 +1,181 @@
|
||||
package explore
|
||||
|
||||
import (
|
||||
"context"
|
||||
"strings"
|
||||
"time"
|
||||
)
|
||||
|
||||
// artistEnrichment records which catalog passes have completed for one
|
||||
// artist. See sql/schemas/artist_enrichment.sql for why these live
|
||||
// beside explore_index rather than in it.
|
||||
type artistEnrichment struct {
|
||||
// Browsed is true once the full MusicBrainz browse has landed.
|
||||
Browsed bool
|
||||
// Similar is true once similar_artist_map has been filled.
|
||||
Similar bool
|
||||
}
|
||||
|
||||
// enrichmentFor reads an artist's marks. A missing row is the zero
|
||||
// value — nothing done — so a read error degrades to re-fetching rather
|
||||
// than to skipping, which is the safe direction for a resumable pass.
|
||||
func (si *SearchIndex) enrichmentFor(mbid string) artistEnrichment {
|
||||
var mark artistEnrichment
|
||||
|
||||
rows, err := si.db.QueryContext(
|
||||
"SELECT browsed_at IS NOT NULL, similar_at IS NOT NULL "+
|
||||
"FROM artist_enrichment WHERE artist_mbid = ?",
|
||||
mbid,
|
||||
)
|
||||
if err != nil {
|
||||
return mark
|
||||
}
|
||||
|
||||
defer func() { _ = rows.Close() }()
|
||||
|
||||
if rows.Next() {
|
||||
_ = rows.Scan(&mark.Browsed, &mark.Similar)
|
||||
}
|
||||
|
||||
return mark
|
||||
}
|
||||
|
||||
// artistBrowsed reports whether the full MB browse has run for an
|
||||
// artist.
|
||||
func (si *SearchIndex) artistBrowsed(mbid string) bool {
|
||||
return si.enrichmentFor(mbid).Browsed
|
||||
}
|
||||
|
||||
// markArtistBrowsed records that the full MB browse has landed.
|
||||
func (si *SearchIndex) markArtistBrowsed(mbid string) {
|
||||
si.markArtistEnrichment(mbid, "browsed_at")
|
||||
}
|
||||
|
||||
// markArtistSimilar records that similar artists have been persisted.
|
||||
func (si *SearchIndex) markArtistSimilar(mbid string) {
|
||||
si.markArtistEnrichment(mbid, "similar_at")
|
||||
}
|
||||
|
||||
// markArtistEnrichment stamps one column, leaving the other alone. The
|
||||
// column name is never user input — the two callers above are the only
|
||||
// ones, and each passes a literal.
|
||||
func (si *SearchIndex) markArtistEnrichment(mbid, column string) {
|
||||
if mbid == "" {
|
||||
return
|
||||
}
|
||||
|
||||
now := time.Now().UTC()
|
||||
|
||||
_, err := si.db.ExecContext(
|
||||
"INSERT INTO artist_enrichment (artist_mbid, "+column+") VALUES (?, ?) "+
|
||||
"ON CONFLICT(artist_mbid) DO UPDATE SET "+column+" = excluded."+column,
|
||||
mbid, now,
|
||||
)
|
||||
if err != nil {
|
||||
si.logger.Warn("artist enrichment: mark failed",
|
||||
"mbid", mbid, "column", column, "error", err,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
// SetMusicBrainz wires the shared MB client so the owned-artist backfill
|
||||
// can complete a discography the ListenBrainz endpoints can only sketch.
|
||||
// Without it the backfill still runs; it just skips the browse.
|
||||
func (si *SearchIndex) SetMusicBrainz(mb *MusicBrainzClient) {
|
||||
si.mu.Lock()
|
||||
si.mb = mb
|
||||
si.mu.Unlock()
|
||||
}
|
||||
|
||||
// musicBrainz returns the wired MB client, or nil.
|
||||
func (si *SearchIndex) musicBrainz() *MusicBrainzClient {
|
||||
si.mu.RLock()
|
||||
defer si.mu.RUnlock()
|
||||
|
||||
return si.mb
|
||||
}
|
||||
|
||||
// browseFullDiscography fetches every release group MusicBrainz has for
|
||||
// an artist and merges it into the index, then marks the artist browsed.
|
||||
//
|
||||
// This is what makes an owned artist's discography *complete* and
|
||||
// *typed*: `fetchTopReleaseGroups` takes ListenBrainz's top 50 by listen
|
||||
// count, above a popularity floor, and LB returns no secondary types at
|
||||
// all — so without this an artist's page shows their popular albums as
|
||||
// one undifferentiated list, with the tail missing entirely.
|
||||
//
|
||||
// The MBID-keyed mark, rather than the old "does any row have secondary
|
||||
// types" heuristic, is what makes it run once: an artist whose every
|
||||
// release is a plain album has no secondary types to find, so the
|
||||
// heuristic was permanently unsatisfied and re-browsed forever.
|
||||
func (si *SearchIndex) browseFullDiscography(ctx context.Context, mbid string) bool {
|
||||
mb := si.musicBrainz()
|
||||
if mb == nil || mbid == "" {
|
||||
return false
|
||||
}
|
||||
|
||||
rgs, err := mb.BrowseReleaseGroupsAll(ctx, mbid)
|
||||
if err != nil {
|
||||
si.logger.Debug("discography backfill: browse failed",
|
||||
"mbid", mbid, "error", err,
|
||||
)
|
||||
|
||||
return false
|
||||
}
|
||||
|
||||
// An artist with genuinely no release groups is still browsed —
|
||||
// marking it stops the pass asking again every run. Only an error
|
||||
// above leaves it unmarked.
|
||||
if len(rgs) > 0 {
|
||||
si.AddFromCache(si.browsedArtistName(mbid, rgs), mbid, rgs)
|
||||
}
|
||||
|
||||
si.markArtistBrowsed(mbid)
|
||||
|
||||
return true
|
||||
}
|
||||
|
||||
// browsedArtistName picks the name AddFromCache will stamp onto every
|
||||
// release group a browse returned.
|
||||
//
|
||||
// MB's browse-by-artist does not echo the artist credit on each item
|
||||
// (the artist is the query parameter), so the name has to come from
|
||||
// somewhere. The canonical local name is preferred — the index title,
|
||||
// then the library row, both via artistDisplayName — and a release
|
||||
// group's own credit is the fallback for an artist neither knows.
|
||||
//
|
||||
// The featuring guard is why that fallback is not simply "the first
|
||||
// credit": a release group credited "X feat. Y" names a collaboration,
|
||||
// not the artist whose page this is, and stamping it on every row is
|
||||
// how one artist's discography came to be filed under a collaboration's
|
||||
// name. Only true featuring markers count — "&", "x" and "," appear
|
||||
// inside real artist names.
|
||||
func (si *SearchIndex) browsedArtistName(mbid string, rgs []MBReleaseGroup) string {
|
||||
if name := si.artistDisplayName(mbid); name != "" && name != mbid {
|
||||
return name
|
||||
}
|
||||
|
||||
for _, rg := range rgs {
|
||||
if rg.ArtistCredit != "" && !looksLikeFeaturingCredit(rg.ArtistCredit) {
|
||||
return rg.ArtistCredit
|
||||
}
|
||||
}
|
||||
|
||||
// Empty rather than the MBID: AddFromCache's "non-empty wins" rule
|
||||
// then leaves whatever real name arrives later untouched.
|
||||
return ""
|
||||
}
|
||||
|
||||
// looksLikeFeaturingCredit reports whether a credit string carries a
|
||||
// "featuring" clause — i.e. it names a collaboration rather than a
|
||||
// single artist.
|
||||
func looksLikeFeaturingCredit(credit string) bool {
|
||||
lower := strings.ToLower(credit)
|
||||
for _, sep := range []string{" feat. ", " feat ", " featuring ", " ft. ", " ft "} {
|
||||
if strings.Contains(lower, sep) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
|
||||
return false
|
||||
}
|
||||
@@ -0,0 +1,140 @@
|
||||
package explore
|
||||
|
||||
import (
|
||||
"log/slog"
|
||||
"testing"
|
||||
|
||||
"yellowjacket/backend/database"
|
||||
)
|
||||
|
||||
// seedIndexArtist inserts an explore_index artist row, optionally
|
||||
// already marked as having had its ListenBrainz discography fetched.
|
||||
func seedIndexArtist(t *testing.T, db *database.DB, mbid string, discogFetched int) {
|
||||
t.Helper()
|
||||
|
||||
if _, err := db.ExecContext(
|
||||
upsertIndexSQL,
|
||||
"artist", mbid, "Seeded Artist", "Seeded Artist", mbid, "",
|
||||
0, 0,
|
||||
0, "", "",
|
||||
"", "", "",
|
||||
"", "", "", "",
|
||||
1, 0,
|
||||
0, 0, 0,
|
||||
discogFetched,
|
||||
); err != nil {
|
||||
t.Fatalf("seed explore_index row for %q: %v", mbid, err)
|
||||
}
|
||||
}
|
||||
|
||||
// TestArtistEnrichmentMarksAreIndependent is the reason these are two
|
||||
// columns rather than one boolean: each fetch fails on its own, and one
|
||||
// failing must not claim the other as done.
|
||||
func TestArtistEnrichmentMarksAreIndependent(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
db := database.NewTestDB(t)
|
||||
si := NewSearchIndex(db, nil, nil, slog.Default())
|
||||
|
||||
const mbid = "11111111-1111-1111-1111-111111111111"
|
||||
|
||||
if mark := si.enrichmentFor(mbid); mark.Browsed || mark.Similar {
|
||||
t.Fatalf("unmarked artist reported as enriched: %+v", mark)
|
||||
}
|
||||
|
||||
si.markArtistBrowsed(mbid)
|
||||
|
||||
mark := si.enrichmentFor(mbid)
|
||||
if !mark.Browsed {
|
||||
t.Error("browsed_at was not recorded")
|
||||
}
|
||||
|
||||
if mark.Similar {
|
||||
t.Error("marking browsed also marked similar")
|
||||
}
|
||||
|
||||
// The second mark upserts onto the same row rather than replacing it.
|
||||
si.markArtistSimilar(mbid)
|
||||
|
||||
mark = si.enrichmentFor(mbid)
|
||||
if !mark.Browsed || !mark.Similar {
|
||||
t.Errorf("marking similar lost the browsed mark: %+v", mark)
|
||||
}
|
||||
|
||||
if !si.artistBrowsed(mbid) {
|
||||
t.Error("artistBrowsed disagreed with enrichmentFor")
|
||||
}
|
||||
}
|
||||
|
||||
// TestUnenrichedIncludesBrowsedGap is the query change 011 turns on: an
|
||||
// artist the catalog artifact already covers (discog_fetched = 1) has
|
||||
// still never been browsed, and the artifact's per-artist coverage is
|
||||
// graded — so "the artifact knows them" is not "we have their
|
||||
// discography", and the backfill must still pick them up.
|
||||
func TestUnenrichedIncludesBrowsedGap(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
db := database.NewTestDB(t)
|
||||
si := NewSearchIndex(db, nil, nil, slog.Default())
|
||||
|
||||
const mbid = "22222222-2222-2222-2222-222222222222"
|
||||
|
||||
if _, err := db.ExecContext(
|
||||
"INSERT INTO artists (name, mbid) VALUES (?, ?)", "Owned Artist", mbid,
|
||||
); err != nil {
|
||||
t.Fatalf("seed library artist: %v", err)
|
||||
}
|
||||
|
||||
seedIndexArtist(t, db, mbid, 1)
|
||||
|
||||
mbids := si.unenrichedLibraryArtistMBIDs(10)
|
||||
if !containsMBID(mbids, mbid) {
|
||||
t.Fatalf("artist with discog_fetched=1 but no browse was skipped: %v", mbids)
|
||||
}
|
||||
|
||||
// Both marks set: nothing left to do, so it drops out entirely.
|
||||
si.markArtistBrowsed(mbid)
|
||||
si.markArtistSimilar(mbid)
|
||||
|
||||
if mbids := si.unenrichedLibraryArtistMBIDs(10); containsMBID(mbids, mbid) {
|
||||
t.Errorf("fully enriched artist was returned again: %v", mbids)
|
||||
}
|
||||
}
|
||||
|
||||
// TestUnenrichedIgnoresSimilarMark pins the other half of that query:
|
||||
// the backfill no longer fetches similar artists (the artist page does,
|
||||
// on view), so an artist it has finished with must drop out even though
|
||||
// similar_at is still NULL. Testing a mark nothing in the pass sets
|
||||
// makes every owned artist a candidate on every run, forever — which is
|
||||
// a backfill that reports progress and never converges.
|
||||
func TestUnenrichedIgnoresSimilarMark(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
db := database.NewTestDB(t)
|
||||
si := NewSearchIndex(db, nil, nil, slog.Default())
|
||||
|
||||
const mbid = "33333333-3333-3333-3333-333333333333"
|
||||
|
||||
if _, err := db.ExecContext(
|
||||
"INSERT INTO artists (name, mbid) VALUES (?, ?)", "Owned Artist", mbid,
|
||||
); err != nil {
|
||||
t.Fatalf("seed library artist: %v", err)
|
||||
}
|
||||
|
||||
seedIndexArtist(t, db, mbid, 1)
|
||||
si.markArtistBrowsed(mbid)
|
||||
|
||||
if mbids := si.unenrichedLibraryArtistMBIDs(10); containsMBID(mbids, mbid) {
|
||||
t.Errorf("artist with no similar_at was returned again: %v", mbids)
|
||||
}
|
||||
}
|
||||
|
||||
func containsMBID(mbids []string, want string) bool {
|
||||
for _, m := range mbids {
|
||||
if m == want {
|
||||
return true
|
||||
}
|
||||
}
|
||||
|
||||
return false
|
||||
}
|
||||
+135
-44
@@ -40,6 +40,11 @@ const (
|
||||
artistImageMaxBytes = 2 * 1024 * 1024
|
||||
artistImageMaxSize = 500 // max dimension for stored full-res images
|
||||
maxImagesPerArtist = 10
|
||||
|
||||
// artistMissFile marks an artist directory as having been resolved
|
||||
// with no artwork found, so the sources are not asked again until
|
||||
// the marker ages out.
|
||||
artistMissFile = ".miss"
|
||||
)
|
||||
|
||||
// fanartTVProjectKey is the project API key for fanart.tv.
|
||||
@@ -118,7 +123,14 @@ func NewArtistImageProvider(
|
||||
|
||||
// GetArtistImage returns the primary image as a base64 data URL.
|
||||
// Resolves from all sources if not yet cached.
|
||||
func (p *ArtistImageProvider) GetArtistImage(artistMBID string) string {
|
||||
//
|
||||
// The context is carried purely for its priority marking: the MB
|
||||
// relations fetch below shares a limiter with the rest of the app, and
|
||||
// a backfill resolving a thousand artists' photos has to yield to the
|
||||
// page a user is looking at (see WithBackgroundPriority).
|
||||
func (p *ArtistImageProvider) GetArtistImage(
|
||||
ctx context.Context, artistMBID string,
|
||||
) string {
|
||||
if artistMBID == "" || p.baseDir == "" {
|
||||
return ""
|
||||
}
|
||||
@@ -135,7 +147,7 @@ func (p *ArtistImageProvider) GetArtistImage(artistMBID string) string {
|
||||
}
|
||||
|
||||
// Resolve from all sources and select primary.
|
||||
p.resolveAllSources(artistMBID)
|
||||
p.resolveAllSources(ctx, artistMBID)
|
||||
|
||||
// Try again after resolution.
|
||||
if data := readFileData(primaryPath); data != "" {
|
||||
@@ -148,6 +160,26 @@ func (p *ArtistImageProvider) GetArtistImage(artistMBID string) string {
|
||||
return ""
|
||||
}
|
||||
|
||||
// EnsureArtistRels caches the MusicBrainz artist lookup — type,
|
||||
// country, sort name, aliases and the URL relations — without
|
||||
// resolving or downloading a single image.
|
||||
//
|
||||
// It exists because that lookup is all the discography backfill ever
|
||||
// wanted from the image provider: it calls GetArtistDetails, which
|
||||
// reads the cache this fills. Going through GetArtistImage to get it
|
||||
// meant every owned artist also paid five upstream lookups and an
|
||||
// image download, which is the bulk of what made that pass take hours.
|
||||
// Portraits resolve when a page that shows one asks for them.
|
||||
func (p *ArtistImageProvider) EnsureArtistRels(
|
||||
ctx context.Context, artistMBID string,
|
||||
) {
|
||||
if artistMBID == "" {
|
||||
return
|
||||
}
|
||||
|
||||
p.fetchMBRels(ctx, artistMBID)
|
||||
}
|
||||
|
||||
// GetCachedImage returns the primary image from disk cache only.
|
||||
// No network fetches.
|
||||
func (p *ArtistImageProvider) GetCachedImage(artistMBID string) string {
|
||||
@@ -342,12 +374,48 @@ type mbRelation struct {
|
||||
} `json:"url"`
|
||||
}
|
||||
|
||||
func (p *ArtistImageProvider) resolveAllSources(artistMBID string) {
|
||||
type imageSource struct {
|
||||
source string
|
||||
url string
|
||||
// artistImageCandidate is one image an upstream offered for an artist,
|
||||
// before anything has been downloaded.
|
||||
type artistImageCandidate struct {
|
||||
source string
|
||||
url string
|
||||
}
|
||||
|
||||
// resolveAllSources resolves an artist's image candidates and downloads
|
||||
// exactly one of them.
|
||||
//
|
||||
// It used to download every candidate — up to maxImagesPerArtist, full
|
||||
// size, serially — and keep them all, while nothing in the app has ever
|
||||
// read anything but the primary. Measured on a real cache that was
|
||||
// 4.1 GB of the 5.3 GB on disk, and several seconds of the per-artist
|
||||
// cost the discography backfill pays for every owned artist.
|
||||
//
|
||||
// The candidates that are not downloaded are still *recorded*, with an
|
||||
// empty file_path, so replacing the primary later is one download
|
||||
// rather than a re-resolution of five upstreams.
|
||||
func (p *ArtistImageProvider) resolveAllSources(
|
||||
ctx context.Context, artistMBID string,
|
||||
) {
|
||||
candidates := p.resolveCandidates(ctx, artistMBID)
|
||||
if len(candidates) == 0 {
|
||||
return
|
||||
}
|
||||
|
||||
if len(candidates) > maxImagesPerArtist {
|
||||
candidates = candidates[:maxImagesPerArtist]
|
||||
}
|
||||
|
||||
p.fetchPrimary(artistMBID, candidates)
|
||||
}
|
||||
|
||||
// resolveCandidates asks every upstream what images it has for an
|
||||
// artist, in priority order. It downloads no image bytes — only the
|
||||
// metadata lookups happen here.
|
||||
func (p *ArtistImageProvider) resolveCandidates(
|
||||
ctx context.Context, artistMBID string,
|
||||
) []artistImageCandidate {
|
||||
type imageSource = artistImageCandidate
|
||||
|
||||
var urls []imageSource
|
||||
|
||||
// Source 0 (highest priority): fanart.tv artist thumbnails.
|
||||
@@ -366,7 +434,7 @@ func (p *ArtistImageProvider) resolveAllSources(artistMBID string) {
|
||||
}
|
||||
}
|
||||
|
||||
rels := p.fetchMBRels(artistMBID)
|
||||
rels := p.fetchMBRels(ctx, artistMBID)
|
||||
|
||||
// Source 2: MB direct image relations (Wikimedia Commons).
|
||||
for _, rel := range rels {
|
||||
@@ -424,48 +492,73 @@ func (p *ArtistImageProvider) resolveAllSources(artistMBID string) {
|
||||
}
|
||||
}
|
||||
|
||||
if len(urls) == 0 {
|
||||
return
|
||||
}
|
||||
return urls
|
||||
}
|
||||
|
||||
// Cap at maxImagesPerArtist.
|
||||
if len(urls) > maxImagesPerArtist {
|
||||
urls = urls[:maxImagesPerArtist]
|
||||
}
|
||||
|
||||
// Fetch and store each image.
|
||||
// fetchPrimary downloads candidates in priority order until one
|
||||
// succeeds, makes that one the primary, and records the rest as
|
||||
// known-but-unfetched.
|
||||
//
|
||||
// Taking the *first that succeeds* rather than the first outright is
|
||||
// also a fix: the old loop keyed is_primary on the index, so a failed
|
||||
// download of candidate 0 left the artist with a stored image, no
|
||||
// primary.jpg, and — because GetArtistImage looks for primary.jpg —
|
||||
// a `.miss` marker claiming the artist has no art at all.
|
||||
func (p *ArtistImageProvider) fetchPrimary(
|
||||
artistMBID string, candidates []artistImageCandidate,
|
||||
) {
|
||||
dir := p.artistDir(artistMBID)
|
||||
_ = os.MkdirAll(dir, 0o755)
|
||||
|
||||
for i, u := range urls {
|
||||
imgData, err := p.fetchImageBytes(u.url)
|
||||
if err != nil || len(imgData) == 0 {
|
||||
primaryPath := filepath.Join(dir, "primary.jpg")
|
||||
found := false
|
||||
|
||||
for i, c := range candidates {
|
||||
if found {
|
||||
p.recordCandidate(artistMBID, c, "", false, 0, i)
|
||||
|
||||
continue
|
||||
}
|
||||
|
||||
filename := fmt.Sprintf("%s_%d.jpg", u.source, i)
|
||||
path := filepath.Join(dir, filename)
|
||||
_ = os.WriteFile(path, imgData, 0o644)
|
||||
imgData, err := p.fetchImageBytes(c.url)
|
||||
if err != nil || len(imgData) == 0 {
|
||||
p.recordCandidate(artistMBID, c, "", false, 0, i)
|
||||
|
||||
// Store in DB.
|
||||
isPrimary := 0
|
||||
if i == 0 {
|
||||
isPrimary = 1
|
||||
continue
|
||||
}
|
||||
|
||||
_, _ = p.db.ExecContext(`
|
||||
INSERT OR REPLACE INTO artist_images
|
||||
(artist_mbid, source, source_url, file_path, is_primary, sort_order, file_size)
|
||||
VALUES (?, ?, ?, ?, ?, ?, ?)
|
||||
`, artistMBID, u.source, u.url, path, isPrimary, i, len(imgData))
|
||||
// setPrimary writes primary.jpg itself, so the candidate is not
|
||||
// also written under its own name — that second copy was a
|
||||
// straight duplicate of the largest file in the directory.
|
||||
p.setPrimary(artistMBID, dir, imgData)
|
||||
p.recordCandidate(artistMBID, c, primaryPath, true, len(imgData), i)
|
||||
|
||||
// Generate thumbnails for the primary image.
|
||||
if i == 0 {
|
||||
p.setPrimary(artistMBID, dir, imgData)
|
||||
}
|
||||
found = true
|
||||
}
|
||||
}
|
||||
|
||||
// recordCandidate stores one candidate. An empty filePath means the
|
||||
// image was offered but never downloaded, which is the ordinary state
|
||||
// for everything but the primary.
|
||||
func (p *ArtistImageProvider) recordCandidate(
|
||||
artistMBID string,
|
||||
c artistImageCandidate,
|
||||
filePath string,
|
||||
isPrimary bool,
|
||||
size, order int,
|
||||
) {
|
||||
primary := 0
|
||||
if isPrimary {
|
||||
primary = 1
|
||||
}
|
||||
|
||||
_, _ = p.db.ExecContext(`
|
||||
INSERT OR REPLACE INTO artist_images
|
||||
(artist_mbid, source, source_url, file_path, is_primary, sort_order, file_size)
|
||||
VALUES (?, ?, ?, ?, ?, ?, ?)
|
||||
`, artistMBID, c.source, c.url, filePath, primary, order, size)
|
||||
}
|
||||
|
||||
// setPrimary copies image data to primary.jpg and generates thumbnails.
|
||||
func (p *ArtistImageProvider) setPrimary(artistMBID, dir string, imgData []byte) {
|
||||
primaryPath := filepath.Join(dir, "primary.jpg")
|
||||
@@ -664,7 +757,9 @@ func (p *ArtistImageProvider) fetchAudioDB(artistMBID string) []string {
|
||||
// Source 2-4: MB rels + Wikidata + Wikipedia
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
func (p *ArtistImageProvider) fetchMBRels(artistMBID string) []mbRelation {
|
||||
func (p *ArtistImageProvider) fetchMBRels(
|
||||
ctx context.Context, artistMBID string,
|
||||
) []mbRelation {
|
||||
cacheKey := "mb:artist-rels:" + artistMBID
|
||||
|
||||
if data, ok := p.cache.Get(cacheKey); ok {
|
||||
@@ -682,7 +777,7 @@ func (p *ArtistImageProvider) fetchMBRels(artistMBID string) []mbRelation {
|
||||
artistMBID,
|
||||
)
|
||||
|
||||
if err := p.mbLimiter.Wait(context.Background()); err != nil {
|
||||
if err := p.mbLimiter.Wait(ctx); err != nil {
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -849,11 +944,7 @@ func (p *ArtistImageProvider) fetchWikipediaLeadImage(qid string) string {
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
func (p *ArtistImageProvider) artistDir(mbid string) string {
|
||||
if len(mbid) < 2 {
|
||||
return filepath.Join(p.baseDir, "xx", mbid)
|
||||
}
|
||||
|
||||
return filepath.Join(p.baseDir, mbid[:2], mbid)
|
||||
return ArtistImageDir(p.baseDir, mbid)
|
||||
}
|
||||
|
||||
func (p *ArtistImageProvider) primaryPath(mbid string) string {
|
||||
@@ -861,7 +952,7 @@ func (p *ArtistImageProvider) primaryPath(mbid string) string {
|
||||
}
|
||||
|
||||
func (p *ArtistImageProvider) isMiss(mbid string) bool {
|
||||
missPath := filepath.Join(p.artistDir(mbid), ".miss")
|
||||
missPath := filepath.Join(p.artistDir(mbid), artistMissFile)
|
||||
_, err := os.Stat(missPath)
|
||||
|
||||
return err == nil
|
||||
@@ -870,7 +961,7 @@ func (p *ArtistImageProvider) isMiss(mbid string) bool {
|
||||
func (p *ArtistImageProvider) writeMiss(mbid string) {
|
||||
dir := p.artistDir(mbid)
|
||||
_ = os.MkdirAll(dir, 0o755)
|
||||
_ = os.WriteFile(filepath.Join(dir, ".miss"), []byte{}, 0o644)
|
||||
_ = os.WriteFile(filepath.Join(dir, artistMissFile), []byte{}, 0o644)
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
@@ -0,0 +1,189 @@
|
||||
package explore
|
||||
|
||||
import (
|
||||
"log/slog"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
|
||||
"yellowjacket/backend/database"
|
||||
)
|
||||
|
||||
// onePixelJPEG is the smallest thing image.Decode will accept, so
|
||||
// setPrimary's thumbnail pass runs for real rather than bailing out.
|
||||
//
|
||||
//nolint:gochecknoglobals // fixture bytes, shared by the tests below
|
||||
var onePixelJPEG = []byte{
|
||||
0xff, 0xd8, 0xff, 0xe0, 0x00, 0x10, 0x4a, 0x46, 0x49, 0x46, 0x00, 0x01,
|
||||
0x01, 0x00, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00, 0xff, 0xdb, 0x00, 0x43,
|
||||
0x00, 0x08, 0x06, 0x06, 0x07, 0x06, 0x05, 0x08, 0x07, 0x07, 0x07, 0x09,
|
||||
0x09, 0x08, 0x0a, 0x0c, 0x14, 0x0d, 0x0c, 0x0b, 0x0b, 0x0c, 0x19, 0x12,
|
||||
0x13, 0x0f, 0x14, 0x1d, 0x1a, 0x1f, 0x1e, 0x1d, 0x1a, 0x1c, 0x1c, 0x20,
|
||||
0x24, 0x2e, 0x27, 0x20, 0x22, 0x2c, 0x23, 0x1c, 0x1c, 0x28, 0x37, 0x29,
|
||||
0x2c, 0x30, 0x31, 0x34, 0x34, 0x34, 0x1f, 0x27, 0x39, 0x3d, 0x38, 0x32,
|
||||
0x3c, 0x2e, 0x33, 0x34, 0x32, 0xff, 0xc0, 0x00, 0x0b, 0x08, 0x00, 0x01,
|
||||
0x00, 0x01, 0x01, 0x01, 0x11, 0x00, 0xff, 0xc4, 0x00, 0x1f, 0x00, 0x00,
|
||||
0x01, 0x05, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x00, 0x00, 0x00, 0x00,
|
||||
0x00, 0x00, 0x00, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
|
||||
0x09, 0x0a, 0x0b, 0xff, 0xc4, 0x00, 0x14, 0x10, 0x01, 0x00, 0x00, 0x00,
|
||||
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
|
||||
0xff, 0xda, 0x00, 0x08, 0x01, 0x01, 0x00, 0x00, 0x3f, 0x00, 0x37, 0xff,
|
||||
0xd9,
|
||||
}
|
||||
|
||||
// newTestImageProvider returns a provider writing into a temp directory.
|
||||
func newTestImageProvider(t *testing.T) *ArtistImageProvider {
|
||||
t.Helper()
|
||||
|
||||
db := database.NewTestDB(t)
|
||||
|
||||
return &ArtistImageProvider{
|
||||
db: db,
|
||||
cache: NewCache(db, slog.Default()),
|
||||
client: http.DefaultClient,
|
||||
logger: slog.Default(),
|
||||
baseDir: t.TempDir(),
|
||||
}
|
||||
}
|
||||
|
||||
// candidateRows reports what fetchPrimary recorded, keyed by source URL.
|
||||
func candidateRows(t *testing.T, p *ArtistImageProvider, mbid string) map[string]string {
|
||||
t.Helper()
|
||||
|
||||
rows, err := p.db.QueryContext(
|
||||
"SELECT source_url, file_path FROM artist_images WHERE artist_mbid = ?",
|
||||
mbid,
|
||||
)
|
||||
if err != nil {
|
||||
t.Fatalf("query artist_images: %v", err)
|
||||
}
|
||||
|
||||
defer func() { _ = rows.Close() }()
|
||||
|
||||
out := map[string]string{}
|
||||
|
||||
for rows.Next() {
|
||||
var url, path string
|
||||
if err := rows.Scan(&url, &path); err != nil {
|
||||
t.Fatalf("scan artist_images: %v", err)
|
||||
}
|
||||
|
||||
out[url] = path
|
||||
}
|
||||
|
||||
return out
|
||||
}
|
||||
|
||||
// Exactly one candidate is downloaded, however many were offered — the
|
||||
// rest are recorded as URLs so a later request is one fetch rather than
|
||||
// a re-resolution of every upstream.
|
||||
func TestFetchPrimaryDownloadsOnlyTheWinner(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
var hits atomic.Int64
|
||||
|
||||
srv := httptest.NewServer(http.HandlerFunc(
|
||||
func(w http.ResponseWriter, _ *http.Request) {
|
||||
hits.Add(1)
|
||||
|
||||
_, _ = w.Write(onePixelJPEG)
|
||||
},
|
||||
))
|
||||
defer srv.Close()
|
||||
|
||||
p := newTestImageProvider(t)
|
||||
|
||||
const mbid = "11111111-1111-1111-1111-111111111111"
|
||||
|
||||
candidates := []artistImageCandidate{
|
||||
{source: "fanart", url: srv.URL + "/a.jpg"},
|
||||
{source: "audiodb", url: srv.URL + "/b.jpg"},
|
||||
{source: "wikidata", url: srv.URL + "/c.jpg"},
|
||||
}
|
||||
|
||||
p.fetchPrimary(mbid, candidates)
|
||||
|
||||
if got := hits.Load(); got != 1 {
|
||||
t.Errorf("downloaded %d images, want 1", got)
|
||||
}
|
||||
|
||||
dir := p.artistDir(mbid)
|
||||
|
||||
// The portrait and its tiers, and nothing else: the winning
|
||||
// candidate is not also written under its own name.
|
||||
entries, err := os.ReadDir(dir)
|
||||
if err != nil {
|
||||
t.Fatalf("read artist dir: %v", err)
|
||||
}
|
||||
|
||||
for _, e := range entries {
|
||||
if !ArtistImageKeepNames()[e.Name()] {
|
||||
t.Errorf("unexpected file left on disk: %s", e.Name())
|
||||
}
|
||||
}
|
||||
|
||||
if _, err := os.Stat(filepath.Join(dir, "primary.jpg")); err != nil {
|
||||
t.Errorf("no primary.jpg was written: %v", err)
|
||||
}
|
||||
|
||||
recorded := candidateRows(t, p, mbid)
|
||||
if len(recorded) != len(candidates) {
|
||||
t.Errorf("recorded %d candidates, want %d", len(recorded), len(candidates))
|
||||
}
|
||||
|
||||
if path := recorded[candidates[0].url]; path == "" {
|
||||
t.Error("the winning candidate has no file_path")
|
||||
}
|
||||
|
||||
for _, c := range candidates[1:] {
|
||||
if path := recorded[c.url]; path != "" {
|
||||
t.Errorf("unfetched candidate %s recorded a path %q", c.source, path)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// A failing first candidate must fall through to the next. The old loop
|
||||
// keyed the primary on the index, so this left an artist with a stored
|
||||
// image, no primary.jpg, and a .miss marker claiming it had no art.
|
||||
func TestFetchPrimaryFallsThroughAFailure(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
srv := httptest.NewServer(http.HandlerFunc(
|
||||
func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.URL.Path == "/dead.jpg" {
|
||||
w.WriteHeader(http.StatusNotFound)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
_, _ = w.Write(onePixelJPEG)
|
||||
},
|
||||
))
|
||||
defer srv.Close()
|
||||
|
||||
p := newTestImageProvider(t)
|
||||
|
||||
const mbid = "22222222-2222-2222-2222-222222222222"
|
||||
|
||||
good := srv.URL + "/good.jpg"
|
||||
|
||||
p.fetchPrimary(mbid, []artistImageCandidate{
|
||||
{source: "fanart", url: srv.URL + "/dead.jpg"},
|
||||
{source: "audiodb", url: good},
|
||||
})
|
||||
|
||||
if _, err := os.Stat(p.primaryPath(mbid)); err != nil {
|
||||
t.Fatalf("no primary written after the first candidate failed: %v", err)
|
||||
}
|
||||
|
||||
if p.isMiss(mbid) {
|
||||
t.Error("artist marked as having no artwork despite a successful fetch")
|
||||
}
|
||||
|
||||
if path := candidateRows(t, p, mbid)[good]; path == "" {
|
||||
t.Error("the surviving candidate was not recorded as the stored one")
|
||||
}
|
||||
}
|
||||
@@ -1,5 +1,7 @@
|
||||
package explore
|
||||
|
||||
import "path/filepath"
|
||||
|
||||
// Asset directory names under the user data directory.
|
||||
//
|
||||
// These are exported so the maintenance janitor can sweep them without
|
||||
@@ -18,3 +20,34 @@ const (
|
||||
// the database references these files.
|
||||
CoverArtCacheDirName = "cover-art-cache"
|
||||
)
|
||||
|
||||
// ArtistImageDir returns the directory holding one artist's images.
|
||||
//
|
||||
// Artist directories are sharded under the MBID's first two characters,
|
||||
// which the janitor cannot be expected to know and had got wrong — so
|
||||
// this is the one definition of that layout, and ArtistImageProvider's
|
||||
// own artistDir defers to it.
|
||||
func ArtistImageDir(baseDir, mbid string) string {
|
||||
if len(mbid) < 2 {
|
||||
return filepath.Join(baseDir, "xx", mbid)
|
||||
}
|
||||
|
||||
return filepath.Join(baseDir, mbid[:2], mbid)
|
||||
}
|
||||
|
||||
// ArtistImageKeepNames is every file an artist's directory is meant to
|
||||
// hold: the portrait, its three size tiers, and the marker recording
|
||||
// that no portrait was found. Anything else is a downloaded candidate
|
||||
// from a version that kept all of them (see StrayArtistImageFilesJob).
|
||||
func ArtistImageKeepNames() map[string]bool {
|
||||
keep := map[string]bool{
|
||||
"primary.jpg": true,
|
||||
artistMissFile: true,
|
||||
}
|
||||
|
||||
for _, tier := range artistImageTiers {
|
||||
keep["primary"+tier.Suffix+".jpg"] = true
|
||||
}
|
||||
|
||||
return keep
|
||||
}
|
||||
|
||||
@@ -0,0 +1,112 @@
|
||||
package explore
|
||||
|
||||
import (
|
||||
"context"
|
||||
|
||||
"yellowjacket/backend/jobs"
|
||||
)
|
||||
|
||||
// discogBackfillJobID is the stable registry ID for the owned-artist
|
||||
// discography backfill. Only one runs at a time (the launch and
|
||||
// post-scan triggers both go through the same bounded, resumable call),
|
||||
// so the ID is a constant and a re-run reuses the handle and its log.
|
||||
const discogBackfillJobID = "explore:discography-backfill"
|
||||
|
||||
// rgMBIDBackfillJobID is the same for the release-group MBID
|
||||
// resolution pass.
|
||||
const rgMBIDBackfillJobID = "explore:release-group-mbid-backfill"
|
||||
|
||||
// lyricsBackfillJobID is the same for the LRCLIB lyrics pass.
|
||||
const lyricsBackfillJobID = "explore:lyrics-backfill"
|
||||
|
||||
// backfillJob is the registry side of a background catalog backfill:
|
||||
// a handle, and the cancel func the registry's Cancel control trips.
|
||||
//
|
||||
// Every method is nil-safe, because a nil *backfillJob is the ordinary
|
||||
// state in tests and in any build with no registry wired — the backfill
|
||||
// itself must not care whether anyone is watching.
|
||||
type backfillJob struct {
|
||||
h *jobs.Handle
|
||||
cancel context.CancelFunc
|
||||
}
|
||||
|
||||
// startBackfillJob registers a cancellable job and returns it alongside
|
||||
// a context that the job's Cancel control cancels. A nil registry (or
|
||||
// a zero total) yields a nil job and the original context, so callers
|
||||
// need no branch of their own.
|
||||
//
|
||||
// It is deliberately called *after* the work has been counted: a run
|
||||
// with nothing to do must not put a job in the indicator, and this
|
||||
// backfill is a no-op on every launch once the library is covered.
|
||||
func startBackfillJob(
|
||||
ctx context.Context,
|
||||
reg *jobs.Registry,
|
||||
id, title, subtitle string,
|
||||
total int,
|
||||
) (*backfillJob, context.Context) {
|
||||
if reg == nil || total <= 0 {
|
||||
return nil, ctx
|
||||
}
|
||||
|
||||
jobCtx, cancel := context.WithCancel(ctx)
|
||||
|
||||
b := &backfillJob{cancel: cancel}
|
||||
b.h = reg.Start(jobs.Spec{
|
||||
ID: id,
|
||||
Kind: jobs.KindCatalogEnrich,
|
||||
Title: title,
|
||||
Subtitle: subtitle,
|
||||
Total: int64(total),
|
||||
State: jobs.StateRunning,
|
||||
Caps: jobs.Caps{
|
||||
// Not pausable: the run is bounded and resumable by
|
||||
// construction — each artist is marked as it completes, so
|
||||
// cancelling and re-running is exactly what a pause would
|
||||
// achieve, without a second checkpoint to keep honest.
|
||||
Cancellable: true,
|
||||
},
|
||||
Controls: jobs.Controls{Cancel: cancel},
|
||||
})
|
||||
|
||||
return b, jobCtx
|
||||
}
|
||||
|
||||
// progress reports how far the run has got.
|
||||
func (b *backfillJob) progress(current, total int) {
|
||||
if b == nil || b.h == nil {
|
||||
return
|
||||
}
|
||||
|
||||
b.h.SetProgress(int64(current), int64(total))
|
||||
}
|
||||
|
||||
// logf appends a line to the job's log.
|
||||
func (b *backfillJob) logf(level jobs.Level, message string) {
|
||||
if b == nil || b.h == nil {
|
||||
return
|
||||
}
|
||||
|
||||
b.h.Logf(level, message)
|
||||
}
|
||||
|
||||
// finish closes the job out, reporting cancellation when that is what
|
||||
// stopped it, and always releases the context.
|
||||
func (b *backfillJob) finish(ctx context.Context) {
|
||||
if b == nil {
|
||||
return
|
||||
}
|
||||
|
||||
defer b.cancel()
|
||||
|
||||
if b.h == nil {
|
||||
return
|
||||
}
|
||||
|
||||
if ctx.Err() != nil {
|
||||
b.h.Cancelled()
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
b.h.Complete()
|
||||
}
|
||||
@@ -0,0 +1,156 @@
|
||||
package explore
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strconv"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"yellowjacket/backend/database"
|
||||
)
|
||||
|
||||
// browseServer serves a paged release-group browse for an artist with
|
||||
// `total` release groups, and records how many requests it received.
|
||||
func browseServer(t *testing.T, total int) (*httptest.Server, *int) {
|
||||
t.Helper()
|
||||
|
||||
requests := 0
|
||||
|
||||
srv := httptest.NewServer(http.HandlerFunc(
|
||||
func(w http.ResponseWriter, r *http.Request) {
|
||||
requests++
|
||||
|
||||
offset, _ := strconv.Atoi(r.URL.Query().Get("offset"))
|
||||
limit, _ := strconv.Atoi(r.URL.Query().Get("limit"))
|
||||
|
||||
if limit <= 0 {
|
||||
limit = 25
|
||||
}
|
||||
|
||||
end := min(offset+limit, total)
|
||||
|
||||
groups := make([]map[string]any, 0, max(0, end-offset))
|
||||
|
||||
for i := offset; i < end; i++ {
|
||||
groups = append(groups, map[string]any{
|
||||
"id": fmt.Sprintf("rg-%03d", i),
|
||||
"title": "Release " + strconv.Itoa(i),
|
||||
"primary-type": "Album",
|
||||
"secondary-types": []string{"Live"},
|
||||
"first-release-da": "",
|
||||
})
|
||||
}
|
||||
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
_ = json.NewEncoder(w).Encode(map[string]any{
|
||||
"release-group-count": total,
|
||||
"release-group-offset": offset,
|
||||
"release-groups": groups,
|
||||
})
|
||||
},
|
||||
))
|
||||
|
||||
t.Cleanup(srv.Close)
|
||||
|
||||
return srv, &requests
|
||||
}
|
||||
|
||||
// browseClient points a real MusicBrainzClient at a test server.
|
||||
func browseClient(t *testing.T, srv *httptest.Server) *MusicBrainzClient {
|
||||
t.Helper()
|
||||
|
||||
db := database.NewTestDB(t)
|
||||
// Fast limiter: this test is about paging, not pacing.
|
||||
c := NewMusicBrainzClient(
|
||||
NewCache(db, slog.Default()), NewRateLimiterN(1000), slog.Default(),
|
||||
)
|
||||
c.mb.SetBaseURL(strings.TrimSuffix(srv.URL, "/") + "/ws/2/")
|
||||
|
||||
return c
|
||||
}
|
||||
|
||||
// TestBrowseReleaseGroupsAllPages is the bug 011 is built on: the
|
||||
// single-page browse asks for MaxLimit and takes what comes back, so a
|
||||
// prolific artist's discography was silently cut at 100 — and a hundred
|
||||
// albums looks like a complete answer unless you count.
|
||||
func TestBrowseReleaseGroupsAllPages(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
const total = 237
|
||||
|
||||
srv, requests := browseServer(t, total)
|
||||
c := browseClient(t, srv)
|
||||
|
||||
all, err := c.BrowseReleaseGroupsAll(t.Context(), "artist-mbid")
|
||||
if err != nil {
|
||||
t.Fatalf("BrowseReleaseGroupsAll: %v", err)
|
||||
}
|
||||
|
||||
if len(all) != total {
|
||||
t.Errorf("got %d release groups, want %d", len(all), total)
|
||||
}
|
||||
|
||||
// 100 + 100 + 37: the short third page ends it, with no fourth
|
||||
// request to discover that it is over.
|
||||
if *requests != 3 {
|
||||
t.Errorf("made %d requests, want 3", *requests)
|
||||
}
|
||||
}
|
||||
|
||||
// TestBrowseReleaseGroupsAllExactMultiple covers the boundary the
|
||||
// short-page terminator exists for: a total that is an exact multiple
|
||||
// of the page size needs one more (empty) request to know it is done,
|
||||
// and must not loop past it.
|
||||
func TestBrowseReleaseGroupsAllExactMultiple(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
srv, requests := browseServer(t, 200)
|
||||
c := browseClient(t, srv)
|
||||
|
||||
all, err := c.BrowseReleaseGroupsAll(t.Context(), "artist-mbid")
|
||||
if err != nil {
|
||||
t.Fatalf("BrowseReleaseGroupsAll: %v", err)
|
||||
}
|
||||
|
||||
if len(all) != 200 {
|
||||
t.Errorf("got %d release groups, want 200", len(all))
|
||||
}
|
||||
|
||||
if *requests != 3 {
|
||||
t.Errorf("made %d requests, want 3 (two full pages and an empty one)", *requests)
|
||||
}
|
||||
}
|
||||
|
||||
// TestBrowseReleaseGroupsAllCachesForSinglePageReader checks the half
|
||||
// that makes this worth doing interactively: the complete list is
|
||||
// written under the key the single-page browse reads, so the next
|
||||
// ordinary browse is served all of it without a request.
|
||||
func TestBrowseReleaseGroupsAllCachesForSinglePageReader(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
srv, requests := browseServer(t, 150)
|
||||
c := browseClient(t, srv)
|
||||
|
||||
if _, err := c.BrowseReleaseGroupsAll(t.Context(), "artist-mbid"); err != nil {
|
||||
t.Fatalf("BrowseReleaseGroupsAll: %v", err)
|
||||
}
|
||||
|
||||
before := *requests
|
||||
|
||||
cached, err := c.BrowseReleaseGroups(t.Context(), "artist-mbid")
|
||||
if err != nil {
|
||||
t.Fatalf("BrowseReleaseGroups: %v", err)
|
||||
}
|
||||
|
||||
if len(cached) != 150 {
|
||||
t.Errorf("cached read got %d release groups, want 150", len(cached))
|
||||
}
|
||||
|
||||
if *requests != before {
|
||||
t.Errorf("cached read made %d extra requests, want 0", *requests-before)
|
||||
}
|
||||
}
|
||||
+125
-142
@@ -16,6 +16,12 @@ import (
|
||||
"yellowjacket/backend/jobs"
|
||||
)
|
||||
|
||||
// backgroundMBRate is the sustained requests-per-second granted to
|
||||
// post-scan backfills on the MusicBrainz limiters. It is MB's own
|
||||
// documented ceiling rather than the interactive lane's burst rate,
|
||||
// because a backfill is the one caller that sustains it for an hour.
|
||||
const backgroundMBRate = 1.0
|
||||
|
||||
// Service is the Wails-bound service for the explore feature.
|
||||
// It owns the lifecycle of all explore-related components: the
|
||||
// MusicBrainz client, ListenBrainz client, rate limiter, and
|
||||
@@ -31,9 +37,12 @@ type Service struct {
|
||||
artistImg *ArtistImageProvider
|
||||
libMBID *LibraryMBIDIndex
|
||||
caaLimiter *RateLimiter
|
||||
db *database.DB
|
||||
logger *slog.Logger
|
||||
ctx context.Context
|
||||
// albumComplete answers whether a local album is complete; see
|
||||
// SetAlbumComplete. Nil until wired, which every path tolerates.
|
||||
albumComplete AlbumCompleteFunc
|
||||
db *database.DB
|
||||
logger *slog.Logger
|
||||
ctx context.Context
|
||||
|
||||
// searchMu guards searchCancel, which cancels the currently
|
||||
// in-flight SearchLocal so a superseded query releases the shared
|
||||
@@ -79,9 +88,14 @@ func NewExploreService(logger *slog.Logger, db *database.DB) *Service {
|
||||
// which triggers the library's retry loop (up to 5 × 1s waits).
|
||||
// Staggering avoids the 503 entirely while keeping total phase-1
|
||||
// latency under 1.5s (333ms stagger + ~1s MB response).
|
||||
mbSearchLimiter := NewRateLimiterBurst(3, 1)
|
||||
// The background lane paces post-scan backfills at MusicBrainz's
|
||||
// own documented 1/sec and, more importantly, makes them yield: an
|
||||
// artist page opened mid-backfill is not queued behind it.
|
||||
mbSearchLimiter := NewRateLimiterBurst(3, 1).
|
||||
WithBackgroundLane(backgroundMBRate)
|
||||
// MB background limiter: strict 1/sec for sustained image resolution calls.
|
||||
mbBackgroundLimiter := NewRateLimiter()
|
||||
mbBackgroundLimiter := NewRateLimiter().
|
||||
WithBackgroundLane(backgroundMBRate)
|
||||
mb := NewMusicBrainzClient(cache, mbSearchLimiter, logger.WithGroup("musicbrainz"))
|
||||
lb := NewListenBrainzClient(lbLimiter, cache, logger.WithGroup("listenbrainz"))
|
||||
lrclib := NewLRCLibClient(cache, logger.WithGroup("lrclib"))
|
||||
@@ -90,6 +104,7 @@ func NewExploreService(logger *slog.Logger, db *database.DB) *Service {
|
||||
db, cache, mbBackgroundLimiter, logger.WithGroup("artist-image"),
|
||||
)
|
||||
index := NewSearchIndex(db, lb, artistImg, logger.WithGroup("search-index"))
|
||||
index.SetMusicBrainz(mb)
|
||||
index.MarkReadyIfPopulated() // make index queryable immediately if data exists
|
||||
|
||||
libMBID := NewLibraryMBIDIndex(db)
|
||||
@@ -292,11 +307,31 @@ func (e *Service) backfillReleaseGroupMBIDs(ctx context.Context) {
|
||||
|
||||
_ = rows.Close()
|
||||
|
||||
for _, p := range pending {
|
||||
if len(pending) == 0 {
|
||||
return
|
||||
}
|
||||
|
||||
// Same two rules as the discography backfill: nobody is waiting on
|
||||
// this, so it yields the MB limiter, and it is visible and
|
||||
// stoppable while it runs.
|
||||
ctx = WithBackgroundPriority(ctx)
|
||||
|
||||
job, ctx := startBackfillJob(
|
||||
ctx, e.index.jobRegistry(), rgMBIDBackfillJobID,
|
||||
"Matching albums to the catalog",
|
||||
"Resolving release MBIDs found while scanning",
|
||||
len(pending),
|
||||
)
|
||||
|
||||
defer func() { job.finish(ctx) }()
|
||||
|
||||
for i, p := range pending {
|
||||
if ctx.Err() != nil {
|
||||
return
|
||||
}
|
||||
|
||||
job.progress(i, len(pending))
|
||||
|
||||
release, err := e.mb.LookupRelease(ctx, p.releaseMBID)
|
||||
if err != nil || release.ReleaseGroupMBID == "" {
|
||||
// Left alone rather than cleared: LookupRelease caches its
|
||||
@@ -555,18 +590,10 @@ func (e *Service) BrowseReleaseGroups(artistMBID string) ([]MBReleaseGroup, erro
|
||||
if indexed := e.index.TopReleaseGroupsByArtist(artistMBID, 200); len(indexed) > 0 {
|
||||
out := make([]MBReleaseGroup, 0, len(indexed))
|
||||
|
||||
// Check if ANY row has secondary types — if none do, we need
|
||||
// to refresh from MB to pick them up. This typically happens
|
||||
// on the first visit after an artist's discography was indexed
|
||||
// from the LB top-release-groups endpoint (which doesn't
|
||||
// return secondary types).
|
||||
hasSecondaryTypes := false
|
||||
|
||||
for _, r := range indexed {
|
||||
var secondary []string
|
||||
if r.SecondaryTypes != "" {
|
||||
secondary = strings.Split(r.SecondaryTypes, ",")
|
||||
hasSecondaryTypes = true
|
||||
}
|
||||
|
||||
out = append(out, MBReleaseGroup{
|
||||
@@ -584,16 +611,15 @@ func (e *Service) BrowseReleaseGroups(artistMBID string) ([]MBReleaseGroup, erro
|
||||
})
|
||||
}
|
||||
|
||||
// Fire MB browse in background if we're missing secondary types
|
||||
// so the next visit gets them.
|
||||
if !hasSecondaryTypes {
|
||||
go func() {
|
||||
rgs, err := e.mb.BrowseReleaseGroups(e.ctx, artistMBID)
|
||||
if err == nil && len(rgs) > 0 {
|
||||
artistName := e.resolveArtistName(artistMBID, rgs)
|
||||
e.index.AddFromCache(artistName, artistMBID, rgs)
|
||||
}
|
||||
}()
|
||||
// Complete the discography in the background if the full browse
|
||||
// has never run for this artist, so the next visit has the tail
|
||||
// and the secondary types. Marked per artist rather than
|
||||
// inferred from "does any row carry a secondary type", which is
|
||||
// permanently false for an artist whose releases are all plain
|
||||
// albums — that test re-browsed such an artist on every visit,
|
||||
// forever.
|
||||
if !e.index.artistBrowsed(artistMBID) {
|
||||
go func() { e.index.browseFullDiscography(e.ctx, artistMBID) }()
|
||||
}
|
||||
|
||||
return out, nil
|
||||
@@ -610,61 +636,6 @@ func (e *Service) BrowseReleaseGroups(artistMBID string) ([]MBReleaseGroup, erro
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
// resolveArtistName picks the best available artist name for a list
|
||||
// of release groups returned from MB browse-by-artist. MB browse
|
||||
// doesn't echo back the artist credit on each item (since the artist
|
||||
// is the query parameter), so we need to find a name from somewhere:
|
||||
// 1. First non-empty ArtistCredit on any release group
|
||||
// 2. The local explore_index (if the artist was previously indexed)
|
||||
// 3. A LookupArtist call to MB (last resort)
|
||||
// 4. The MBID itself (worst case fallback)
|
||||
//
|
||||
// looksLikeFeaturingCredit reports whether a credit string carries a
|
||||
// "featuring" clause — i.e. it names a collaboration rather than a
|
||||
// single artist. Only true "featuring" markers count; "&", "x", and
|
||||
// "," are excluded because they appear inside real artist names.
|
||||
func looksLikeFeaturingCredit(credit string) bool {
|
||||
lower := strings.ToLower(credit)
|
||||
for _, sep := range []string{" feat. ", " feat ", " featuring ", " ft. ", " ft "} {
|
||||
if strings.Contains(lower, sep) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
|
||||
return false
|
||||
}
|
||||
|
||||
func (e *Service) resolveArtistName(artistMBID string, rgs []MBReleaseGroup) string {
|
||||
// Try the first single-artist credit from the release groups. A
|
||||
// credit carrying a "featuring" clause names a collaboration, not
|
||||
// the artist whose page this is, so skip those and fall through to
|
||||
// the index / MB lookup for the canonical single-artist name — this
|
||||
// is what AddFromCache stamps onto every release group.
|
||||
for _, rg := range rgs {
|
||||
if rg.ArtistCredit != "" && !looksLikeFeaturingCredit(rg.ArtistCredit) {
|
||||
return rg.ArtistCredit
|
||||
}
|
||||
}
|
||||
|
||||
// Check the index for a previously-indexed artist row.
|
||||
if indexed := e.index.LookupArtistByMBID(
|
||||
artistMBID,
|
||||
); indexed != nil && indexed.Title != "" {
|
||||
return indexed.Title
|
||||
}
|
||||
|
||||
// Last resort: hit MB lookup.
|
||||
if artist, err := e.mb.LookupArtist(
|
||||
e.ctx,
|
||||
artistMBID,
|
||||
); err == nil && artist != nil &&
|
||||
artist.Name != "" {
|
||||
return artist.Name
|
||||
}
|
||||
|
||||
return artistMBID
|
||||
}
|
||||
|
||||
// BrowseReleases fetches releases for a given release group MBID.
|
||||
//
|
||||
// Local-first, non-blocking: a warm response cache is served instantly;
|
||||
@@ -751,6 +722,12 @@ func (e *Service) ensureReleasesAsync(releaseGroupMBID string) {
|
||||
// navigation almost always originates there. Already-cached groups are
|
||||
// skipped; a cap bounds how many live fetches a single artist view can
|
||||
// trigger so the MusicBrainz rate limiter isn't flooded.
|
||||
// A release group the user owns *completely* is skipped outright: since
|
||||
// tag-derived completeness landed, such an album opens with no catalog
|
||||
// call at all — identity from its MBID, tracklist from its own files —
|
||||
// so warming the most expensive request in the app on its behalf buys
|
||||
// nothing. The skip is not merely an optimisation; those slots go to
|
||||
// albums that will actually need the browse.
|
||||
func (e *Service) PrefetchReleases(releaseGroupMBIDs []string) {
|
||||
const maxPrefetch = 8
|
||||
|
||||
@@ -765,6 +742,10 @@ func (e *Service) PrefetchReleases(releaseGroupMBIDs []string) {
|
||||
continue
|
||||
}
|
||||
|
||||
if e.ownedAndComplete(mbid) {
|
||||
continue
|
||||
}
|
||||
|
||||
e.ensureReleasesAsync(mbid)
|
||||
|
||||
fired++
|
||||
@@ -774,6 +755,36 @@ func (e *Service) PrefetchReleases(releaseGroupMBIDs []string) {
|
||||
}
|
||||
}
|
||||
|
||||
// AlbumCompleteFunc answers "is the local album with this id complete",
|
||||
// i.e. do its files declare a track total the library actually has.
|
||||
type AlbumCompleteFunc func(albumID int64) bool
|
||||
|
||||
// SetAlbumComplete injects the completeness check. It is injected
|
||||
// rather than imported because `library` and `explore` do not depend on
|
||||
// each other in either direction today, and one prefetch heuristic is
|
||||
// not a reason to introduce that edge — the alternative, re-deriving
|
||||
// "complete" from SQL here, would be a second definition of it.
|
||||
func (e *Service) SetAlbumComplete(fn AlbumCompleteFunc) {
|
||||
e.albumComplete = fn
|
||||
}
|
||||
|
||||
// ownedAndComplete reports whether a release group is one the user owns
|
||||
// in full. Unknown completeness is not complete: an untagged library
|
||||
// declares no totals at all, and treating that as complete would skip
|
||||
// the prefetch for the libraries that need it most.
|
||||
func (e *Service) ownedAndComplete(releaseGroupMBID string) bool {
|
||||
if e.albumComplete == nil {
|
||||
return false
|
||||
}
|
||||
|
||||
indexed := e.index.LookupReleaseGroupByMBID(releaseGroupMBID)
|
||||
if indexed == nil || indexed.LocalReleaseGroupID == 0 {
|
||||
return false
|
||||
}
|
||||
|
||||
return e.albumComplete(indexed.LocalReleaseGroupID)
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// ListenBrainz
|
||||
// ---------------------------------------------------------------------------
|
||||
@@ -937,6 +948,11 @@ func (e *Service) SimilarArtists(artistMBID string) ([]LBSimilarArtist, error) {
|
||||
// labs API into similar_artist_map in the background and emits
|
||||
// ArtistSimilarReady when done. Concurrent calls for the same artist
|
||||
// collapse into one fetch and one event via the singleflight.
|
||||
//
|
||||
// This is the *only* thing that fills similar_artist_map now — the
|
||||
// owned-artist backfill used to do it for every artist up front, which
|
||||
// is a request each for a section nobody had asked to see. Marking
|
||||
// similar_at here is what keeps that mark meaning what it says.
|
||||
func (e *Service) ensureSimilarArtistsAsync(artistMBID string) {
|
||||
if artistMBID == "" {
|
||||
return
|
||||
@@ -947,6 +963,7 @@ func (e *Service) ensureSimilarArtistsAsync(artistMBID string) {
|
||||
similar, err := e.lb.SimilarArtists(e.ctx, artistMBID)
|
||||
if err == nil {
|
||||
e.index.PersistSimilarArtists(artistMBID, similar)
|
||||
e.index.markArtistSimilar(artistMBID)
|
||||
|
||||
events.Emit(e.ctx, events.ArtistSimilarReady, artistMBID)
|
||||
}
|
||||
@@ -1130,7 +1147,9 @@ func (e *Service) GetThumbnails(requests []ThumbnailRequest) map[string]string {
|
||||
// fetches from Wikimedia Commons, subsequent calls are instant.
|
||||
// Returns "" if no image is available.
|
||||
func (e *Service) GetArtistImageURL(artistMBID string) string {
|
||||
return e.artistImg.GetArtistImage(artistMBID)
|
||||
// Bound method: someone is looking at this artist right now, so it
|
||||
// takes the interactive lane.
|
||||
return e.artistImg.GetArtistImage(e.ctx, artistMBID)
|
||||
}
|
||||
|
||||
// GetArtistImageCached returns a base64 data URL for the artist's
|
||||
@@ -1154,82 +1173,46 @@ func (e *Service) GetArtistImageCachedPath(artistMBID string) string {
|
||||
// CheckLibraryMBIDs returns which of the given MBIDs exist in the
|
||||
// local music library. Returns a map of MBID → entity type
|
||||
// ("artist", "release_group", "recording").
|
||||
//
|
||||
// It has no frontend caller — `downloadcatalog.go` is the one consumer,
|
||||
// asking about a single MBID at a time.
|
||||
func (e *Service) CheckLibraryMBIDs(mbids []string) map[string]string {
|
||||
return e.libMBID.CheckMBIDs(mbids)
|
||||
}
|
||||
|
||||
// PersonalizationResult holds popularity and personalization signals
|
||||
// for a single MBID. Exported for Wails binding.
|
||||
type PersonalizationResult struct {
|
||||
Popularity int `json:"popularity"`
|
||||
ListenerCount int `json:"listenerCount"`
|
||||
InLibrary bool `json:"inLibrary"`
|
||||
SimilarityScore int `json:"similarityScore"`
|
||||
}
|
||||
|
||||
// GetPopularityBatch returns LB popularity and personalization
|
||||
// signals for a batch of MBIDs from the local search index.
|
||||
func (e *Service) GetPopularityBatch(mbids []string) map[string]PersonalizationResult {
|
||||
batch := e.index.GetPopularityBatch(mbids)
|
||||
if batch == nil {
|
||||
return make(map[string]PersonalizationResult)
|
||||
}
|
||||
|
||||
out := make(map[string]PersonalizationResult, len(batch.Popularity))
|
||||
for mbid, pop := range batch.Popularity {
|
||||
out[mbid] = PersonalizationResult{
|
||||
Popularity: pop,
|
||||
ListenerCount: batch.ListenerCount[mbid],
|
||||
InLibrary: batch.InLibrary[mbid],
|
||||
SimilarityScore: batch.SimilarityScores[mbid],
|
||||
}
|
||||
}
|
||||
|
||||
// Include entries that have library/similar flags but no popularity.
|
||||
for mbid := range batch.InLibrary {
|
||||
if _, ok := out[mbid]; !ok {
|
||||
out[mbid] = PersonalizationResult{
|
||||
InLibrary: true,
|
||||
SimilarityScore: batch.SimilarityScores[mbid],
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
for mbid, score := range batch.SimilarityScores {
|
||||
if _, ok := out[mbid]; !ok {
|
||||
out[mbid] = PersonalizationResult{SimilarityScore: score}
|
||||
}
|
||||
}
|
||||
|
||||
return out
|
||||
}
|
||||
|
||||
// GetArtistMBID returns the MusicBrainz ID for a local library
|
||||
// artist by name, or "" if not found or no MBID tagged.
|
||||
func (e *Service) GetArtistMBID(artistName string) string {
|
||||
return e.libMBID.GetArtistMBID(artistName)
|
||||
}
|
||||
|
||||
// GetArtistImages resolves artist images for multiple artists by
|
||||
// name in one call. Returns a map of artist name → base64 data
|
||||
// URL. Only artists with cached images are returned — no network
|
||||
// fetches are triggered (use GetArtistImageURL for on-demand fetch).
|
||||
func (e *Service) GetArtistImages(names []string) map[string]string {
|
||||
result := make(map[string]string, len(names))
|
||||
// GetArtistImagesCachedPaths resolves artist portraits for many MBIDs
|
||||
// in one call, returning MBID → asset-handler path for the medium
|
||||
// thumbnail. Disk existence checks only: no MusicBrainz, no Wikidata,
|
||||
// no Wikimedia, no network of any kind. MBIDs with no cached portrait
|
||||
// are omitted rather than returned empty.
|
||||
//
|
||||
// It exists because the resolving entry point (GetArtistImageURL) was
|
||||
// being used where a cache check belonged: a page rendering a dozen
|
||||
// search results paid a full MB → Wikidata → Wikipedia → Wikimedia
|
||||
// resolution for every artist whose portrait was already on disk. Its
|
||||
// predecessor could not serve that — it keyed on artist *name* through
|
||||
// the library's own MBID map, so it only ever answered for artists the
|
||||
// user already owned, which on a catalog search is nearly none of them.
|
||||
//
|
||||
// Paths rather than base64: a portrait is ~128 kB as a data URL, and
|
||||
// the asset handler serves the same bytes without crossing the IPC
|
||||
// boundary or being retained by a JS string.
|
||||
func (e *Service) GetArtistImagesCachedPaths(mbids []string) map[string]string {
|
||||
result := make(map[string]string, len(mbids))
|
||||
|
||||
// Batch resolve all names → MBIDs from the library DB.
|
||||
allMBIDs := e.libMBID.AllArtistMBIDs()
|
||||
|
||||
for _, name := range names {
|
||||
mbid, ok := allMBIDs[name]
|
||||
if !ok || mbid == "" {
|
||||
for _, mbid := range mbids {
|
||||
if mbid == "" {
|
||||
continue
|
||||
}
|
||||
|
||||
// Only return already-cached images — don't trigger fetches.
|
||||
img := e.artistImg.GetCachedImage(mbid)
|
||||
if img != "" {
|
||||
result[name] = img
|
||||
if _, medium, _, _ := e.artistImg.GetImageURLs(mbid); medium != "" {
|
||||
result[mbid] = medium
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -133,6 +133,18 @@ func (e *Service) BackfillLibraryLyrics() {
|
||||
func (e *Service) backfillLibraryLyrics(ctx context.Context) {
|
||||
total := 0
|
||||
|
||||
// LRCLIB has its own limiter, so this starves nothing — but it is
|
||||
// still work nobody asked for, and it was the last background pass
|
||||
// with no way to see or stop it. The job is registered lazily,
|
||||
// after the first batch proves there is something to do, because on
|
||||
// a covered library every launch would otherwise put an empty job
|
||||
// in the indicator.
|
||||
ctx = WithBackgroundPriority(ctx)
|
||||
|
||||
var job *backfillJob
|
||||
|
||||
defer func() { job.finish(ctx) }()
|
||||
|
||||
for range lyricsBackfillMaxPasses {
|
||||
if ctx.Err() != nil {
|
||||
return
|
||||
@@ -149,13 +161,24 @@ func (e *Service) backfillLibraryLyrics(ctx context.Context) {
|
||||
break
|
||||
}
|
||||
|
||||
if job == nil {
|
||||
job, ctx = startBackfillJob(
|
||||
ctx, e.index.jobRegistry(), lyricsBackfillJobID,
|
||||
"Looking up lyrics",
|
||||
"Tracks in your library with no lyrics yet",
|
||||
len(candidates),
|
||||
)
|
||||
}
|
||||
|
||||
filled := 0
|
||||
|
||||
for _, c := range candidates {
|
||||
for i, c := range candidates {
|
||||
if ctx.Err() != nil {
|
||||
return
|
||||
}
|
||||
|
||||
job.progress(i, len(candidates))
|
||||
|
||||
if e.fetchAndStoreLyrics(ctx, c) != nil {
|
||||
filled++
|
||||
total++
|
||||
|
||||
@@ -328,6 +328,83 @@ func (c *MusicBrainzClient) BrowseReleaseGroups(
|
||||
return out, nil
|
||||
}
|
||||
|
||||
// browseMaxPages bounds BrowseReleaseGroupsAll. At MaxLimit per page
|
||||
// that is 1 000 release groups, which no real artist reaches — it is a
|
||||
// runaway guard for a server that stops honouring the offset, not a
|
||||
// coverage decision.
|
||||
const browseMaxPages = 10
|
||||
|
||||
// BrowseReleaseGroupsAll is BrowseReleaseGroups paged to exhaustion.
|
||||
//
|
||||
// The single-page call above asks for MaxLimit (100) and takes whatever
|
||||
// comes back, which silently truncates a prolific artist's discography
|
||||
// at 100 release groups — invisible unless you count, since a hundred
|
||||
// albums looks like a complete page. This is the call to use when the
|
||||
// answer is meant to be the whole discography rather than a page of it.
|
||||
//
|
||||
// The result is cached under the same key the single-page call reads,
|
||||
// so a later interactive browse is served the complete list.
|
||||
func (c *MusicBrainzClient) BrowseReleaseGroupsAll(
|
||||
ctx context.Context, artistMBID string,
|
||||
) ([]MBReleaseGroup, error) {
|
||||
cacheKey := "mb:browse:release-groups:" + artistMBID
|
||||
|
||||
if data, ok := c.cache.Get(cacheKey); ok {
|
||||
var out []MBReleaseGroup
|
||||
if err := json.Unmarshal(data, &out); err == nil {
|
||||
return out, nil
|
||||
}
|
||||
}
|
||||
|
||||
var out []MBReleaseGroup
|
||||
|
||||
for page := range browseMaxPages {
|
||||
if err := c.limiter.Wait(ctx); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
offset := page * musicbrainzws2.MaxLimit
|
||||
|
||||
c.logger.Info("musicbrainz browse release groups",
|
||||
"artistMBID", artistMBID,
|
||||
"offset", offset,
|
||||
)
|
||||
|
||||
result, err := c.mb.BrowseReleaseGroups(ctx,
|
||||
musicbrainzws2.ReleaseGroupFilter{
|
||||
ArtistMBID: mbtypes.MBID(artistMBID),
|
||||
},
|
||||
musicbrainzws2.Paginator{
|
||||
Limit: musicbrainzws2.MaxLimit,
|
||||
Offset: offset,
|
||||
},
|
||||
)
|
||||
if err != nil {
|
||||
// Pages already fetched are still worth keeping if there are
|
||||
// any: a partial discography beats none, and the caller's
|
||||
// mark is only set on a nil error, so the rest is retried.
|
||||
if len(out) > 0 {
|
||||
return out, nil
|
||||
}
|
||||
|
||||
return nil, err
|
||||
}
|
||||
|
||||
out = append(out, convertReleaseGroups(result.ReleaseGroups)...)
|
||||
|
||||
// A short page is the last page. MB reports the full count too,
|
||||
// but a short page is the condition that terminates correctly
|
||||
// even when the count and the pages disagree.
|
||||
if len(result.ReleaseGroups) < musicbrainzws2.MaxLimit {
|
||||
break
|
||||
}
|
||||
}
|
||||
|
||||
c.cacheJSON(cacheKey, out, cacheTTLEntity, artistMBID, "artist")
|
||||
|
||||
return out, nil
|
||||
}
|
||||
|
||||
// LookupRelease fetches a single release by MBID (with media +
|
||||
// recordings). Used by the autotag paste-URL escape hatch.
|
||||
// Cached for 7 days.
|
||||
|
||||
@@ -4,6 +4,7 @@ package explore
|
||||
|
||||
import (
|
||||
"context"
|
||||
"sync"
|
||||
"time"
|
||||
|
||||
"golang.org/x/time/rate"
|
||||
@@ -14,9 +15,31 @@ import (
|
||||
// requests (not just excess) when the rate is exceeded, so callers
|
||||
// block proactively via Wait rather than retrying reactively.
|
||||
//
|
||||
// A limiter may carry a second, slower **background lane** (see
|
||||
// WithBackgroundLane). A caller marked by WithBackgroundPriority is
|
||||
// paced by that lane *and* yields to interactive callers: while any
|
||||
// interactive Wait is outstanding, background waits do not take a
|
||||
// token at all. This is what keeps a multi-thousand-request backfill
|
||||
// from putting the album page the user is looking at right now behind
|
||||
// hours of queued work.
|
||||
//
|
||||
// RateLimiter is safe for concurrent use.
|
||||
type RateLimiter struct {
|
||||
limiter *rate.Limiter
|
||||
|
||||
// background paces callers marked with WithBackgroundPriority. Nil
|
||||
// means background callers are paced by limiter alone and only the
|
||||
// yield gate below applies.
|
||||
background *rate.Limiter
|
||||
|
||||
mu sync.Mutex
|
||||
// interactive counts Waits currently blocked (or about to block) on
|
||||
// behalf of a user-facing request.
|
||||
interactive int
|
||||
// clear is closed when interactive falls to zero, and is nil while
|
||||
// there are none. Background waiters select on it rather than
|
||||
// polling, so a quiet limiter costs nothing.
|
||||
clear chan struct{}
|
||||
}
|
||||
|
||||
// NewRateLimiter returns a rate limiter that allows exactly one
|
||||
@@ -56,9 +79,151 @@ func NewRateLimiterBurst(n, b int) *RateLimiter {
|
||||
}
|
||||
}
|
||||
|
||||
// WithBackgroundLane gives the limiter a slower sustained rate for
|
||||
// callers marked by WithBackgroundPriority, and returns it so the
|
||||
// call can be chained onto a constructor.
|
||||
//
|
||||
// The interactive lane is deliberately allowed to be faster than the
|
||||
// origin's documented limit (MB's search limiter is 3/s burst 1, which
|
||||
// staggers three concurrent search goroutines rather than raising the
|
||||
// sustained rate). Sustained background work has no such excuse: a
|
||||
// backfill running for an hour at the interactive rate is exactly the
|
||||
// traffic shape that earns an all-or-nothing 503 for every request the
|
||||
// app makes, interactive ones included.
|
||||
func (r *RateLimiter) WithBackgroundLane(perSecond float64) *RateLimiter {
|
||||
r.background = rate.NewLimiter(rate.Limit(perSecond), 1)
|
||||
|
||||
return r
|
||||
}
|
||||
|
||||
// Wait blocks until the rate limiter allows the caller to proceed
|
||||
// or the context is cancelled. Returns ctx.Err() if the context
|
||||
// expires before a token becomes available.
|
||||
//
|
||||
// A context marked by WithBackgroundPriority takes the background
|
||||
// lane; every other caller is interactive.
|
||||
func (r *RateLimiter) Wait(ctx context.Context) error {
|
||||
if isBackgroundPriority(ctx) {
|
||||
return r.waitBackground(ctx)
|
||||
}
|
||||
|
||||
r.enterInteractive()
|
||||
defer r.exitInteractive()
|
||||
|
||||
return r.limiter.Wait(ctx)
|
||||
}
|
||||
|
||||
// waitBackground yields to interactive callers, paces on the background
|
||||
// lane if there is one, and only then takes a token.
|
||||
//
|
||||
// One request of slippage is possible and is accepted rather than
|
||||
// designed out: an interactive caller arriving *during* the final
|
||||
// limiter.Wait below queues behind this one background request. Making
|
||||
// that impossible would mean cancelling an already-granted reservation,
|
||||
// which the token bucket cannot express, in exchange for at most one
|
||||
// request-time of latency.
|
||||
func (r *RateLimiter) waitBackground(ctx context.Context) error {
|
||||
for {
|
||||
if err := r.awaitClear(ctx); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
if r.background != nil {
|
||||
if err := r.background.Wait(ctx); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
|
||||
// An interactive caller may have arrived while the background
|
||||
// lane was pacing us. Go round again — a background task that
|
||||
// never runs while the user is active is the intent, and the
|
||||
// context is what ends the loop.
|
||||
if r.pendingInteractive() {
|
||||
continue
|
||||
}
|
||||
|
||||
return r.limiter.Wait(ctx)
|
||||
}
|
||||
}
|
||||
|
||||
// enterInteractive registers an outstanding interactive wait.
|
||||
func (r *RateLimiter) enterInteractive() {
|
||||
r.mu.Lock()
|
||||
defer r.mu.Unlock()
|
||||
|
||||
if r.interactive == 0 {
|
||||
r.clear = make(chan struct{})
|
||||
}
|
||||
|
||||
r.interactive++
|
||||
}
|
||||
|
||||
// exitInteractive retires one, releasing background waiters when the
|
||||
// last interactive caller is done.
|
||||
func (r *RateLimiter) exitInteractive() {
|
||||
r.mu.Lock()
|
||||
defer r.mu.Unlock()
|
||||
|
||||
r.interactive--
|
||||
|
||||
if r.interactive <= 0 {
|
||||
r.interactive = 0
|
||||
|
||||
if r.clear != nil {
|
||||
close(r.clear)
|
||||
r.clear = nil
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// pendingInteractive reports whether any interactive wait is outstanding.
|
||||
func (r *RateLimiter) pendingInteractive() bool {
|
||||
r.mu.Lock()
|
||||
defer r.mu.Unlock()
|
||||
|
||||
return r.interactive > 0
|
||||
}
|
||||
|
||||
// awaitClear blocks until no interactive wait is outstanding.
|
||||
func (r *RateLimiter) awaitClear(ctx context.Context) error {
|
||||
for {
|
||||
r.mu.Lock()
|
||||
ch := r.clear
|
||||
r.mu.Unlock()
|
||||
|
||||
if ch == nil {
|
||||
return nil
|
||||
}
|
||||
|
||||
select {
|
||||
case <-ctx.Done():
|
||||
return ctx.Err()
|
||||
case <-ch:
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// backgroundPriorityKey types the context marker below.
|
||||
type backgroundPriorityKey struct{}
|
||||
|
||||
// WithBackgroundPriority marks a context as belonging to background
|
||||
// work — a post-scan backfill rather than something a user is waiting
|
||||
// on. Every RateLimiter.Wait made under it yields to interactive
|
||||
// callers and is paced by the background lane.
|
||||
//
|
||||
// It is a context marker rather than a parameter because the marking
|
||||
// has to survive the whole call chain — a backfill calls the same
|
||||
// MusicBrainzClient methods the detail pages call, and threading a
|
||||
// priority argument through all of them would put the decision at
|
||||
// every call site instead of at the one place that knows.
|
||||
func WithBackgroundPriority(ctx context.Context) context.Context {
|
||||
return context.WithValue(ctx, backgroundPriorityKey{}, true)
|
||||
}
|
||||
|
||||
// isBackgroundPriority reports whether ctx was marked by
|
||||
// WithBackgroundPriority.
|
||||
func isBackgroundPriority(ctx context.Context) bool {
|
||||
v, _ := ctx.Value(backgroundPriorityKey{}).(bool)
|
||||
|
||||
return v
|
||||
}
|
||||
|
||||
@@ -38,6 +38,104 @@ func TestRateLimiterBurst(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestRateLimiterBackgroundYields is the property the whole priority
|
||||
// lane exists for: an interactive caller arriving while a backfill is
|
||||
// running is not queued behind the rest of the backfill.
|
||||
func TestRateLimiterBackgroundYields(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// Fast rates: this asserts ordering, not pacing.
|
||||
rl := NewRateLimiterBurst(50, 1).WithBackgroundLane(50)
|
||||
bg := WithBackgroundPriority(context.Background())
|
||||
|
||||
// Hold the gate open for the length of the test by keeping one
|
||||
// interactive wait outstanding.
|
||||
rl.enterInteractive()
|
||||
|
||||
done := make(chan struct{})
|
||||
|
||||
go func() {
|
||||
defer close(done)
|
||||
|
||||
_ = rl.Wait(bg)
|
||||
}()
|
||||
|
||||
select {
|
||||
case <-done:
|
||||
t.Fatal("background Wait proceeded while an interactive wait was outstanding")
|
||||
case <-time.After(100 * time.Millisecond):
|
||||
}
|
||||
|
||||
rl.exitInteractive()
|
||||
|
||||
select {
|
||||
case <-done:
|
||||
case <-time.After(2 * time.Second):
|
||||
t.Fatal("background Wait did not proceed after the interactive wait cleared")
|
||||
}
|
||||
}
|
||||
|
||||
// TestRateLimiterBackgroundLanePaces checks the second half: background
|
||||
// callers are held to their own slower rate even with the gate clear.
|
||||
func TestRateLimiterBackgroundLanePaces(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// Interactive lane is effectively unlimited; the background lane is
|
||||
// the only thing that can slow this down.
|
||||
rl := NewRateLimiterBurst(1000, 1000).WithBackgroundLane(4)
|
||||
bg := WithBackgroundPriority(context.Background())
|
||||
|
||||
start := time.Now()
|
||||
|
||||
for i := range 3 {
|
||||
if err := rl.Wait(bg); err != nil {
|
||||
t.Fatalf("Wait %d: %v", i, err)
|
||||
}
|
||||
}
|
||||
|
||||
// Burst of 1, then two more at 4/sec = ≥500ms.
|
||||
if elapsed := time.Since(start); elapsed < 400*time.Millisecond {
|
||||
t.Errorf("elapsed %v, want ≥ 400ms (background lane not pacing)", elapsed)
|
||||
}
|
||||
}
|
||||
|
||||
// TestRateLimiterBackgroundCancels guards the loop in waitBackground:
|
||||
// a background caller blocked behind a permanently busy interactive
|
||||
// lane must still honour its context.
|
||||
func TestRateLimiterBackgroundCancels(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
rl := NewRateLimiter().WithBackgroundLane(1)
|
||||
rl.enterInteractive() // never cleared
|
||||
|
||||
ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
|
||||
defer cancel()
|
||||
|
||||
err := rl.Wait(WithBackgroundPriority(ctx))
|
||||
if err == nil {
|
||||
t.Fatal("expected error from cancelled context, got nil")
|
||||
}
|
||||
|
||||
if !errors.Is(err, context.DeadlineExceeded) {
|
||||
t.Errorf("error = %v, want context.DeadlineExceeded", err)
|
||||
}
|
||||
}
|
||||
|
||||
// TestRateLimiterInteractiveUnmarked confirms an unmarked context is
|
||||
// interactive — the default has to be the safe one, since every
|
||||
// existing call site is unmarked.
|
||||
func TestRateLimiterInteractiveUnmarked(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
if isBackgroundPriority(context.Background()) {
|
||||
t.Error("an unmarked context reported as background priority")
|
||||
}
|
||||
|
||||
if !isBackgroundPriority(WithBackgroundPriority(context.Background())) {
|
||||
t.Error("a marked context did not report as background priority")
|
||||
}
|
||||
}
|
||||
|
||||
func TestRateLimiterContextCancel(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
+160
-33
@@ -10,6 +10,7 @@ import (
|
||||
"strconv"
|
||||
"strings"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
"time"
|
||||
|
||||
"golang.org/x/sync/singleflight"
|
||||
@@ -93,6 +94,27 @@ const (
|
||||
// across launches instead of running for the better part of an hour.
|
||||
discogBackfillMaxPerRun = 2000
|
||||
|
||||
// discogBackfillWorkers is how many owned artists the backfill has
|
||||
// in flight at once.
|
||||
//
|
||||
// The pass was strictly serial, so an artist's ListenBrainz calls,
|
||||
// its MusicBrainz browse pages and every upstream's latency were
|
||||
// paid end to end before the next artist began — while the limiters
|
||||
// that actually keep us polite are per-host and were idle for most
|
||||
// of it. Concurrency here does not raise the request rate against
|
||||
// any origin; it stops one artist's slowest upstream deciding how
|
||||
// fast the whole run goes.
|
||||
discogBackfillWorkers = 6
|
||||
|
||||
// discogBackfillArtistTimeout bounds one artist's share of a run.
|
||||
//
|
||||
// The MusicBrainz client retries a 503 up to five times, honouring
|
||||
// the server's Retry-After (which it caps at a minute), so a single
|
||||
// throttled artist can otherwise hold a worker for longer than a
|
||||
// hundred healthy ones take. A timed-out artist goes unmarked and
|
||||
// is retried next run, which is what every other failure here does.
|
||||
discogBackfillArtistTimeout = 90 * time.Second
|
||||
|
||||
// labsBaseURL is the base URL for the ListenBrainz labs API.
|
||||
labsBaseURL = "https://labs.api.listenbrainz.org"
|
||||
|
||||
@@ -167,8 +189,12 @@ type lbSitewideArtist struct {
|
||||
// - Post-scan: API discographies for new library artists
|
||||
// - Ongoing: organic growth from user browsing (AddFromCache)
|
||||
type SearchIndex struct {
|
||||
db *database.DB
|
||||
lb *ListenBrainzClient
|
||||
db *database.DB
|
||||
lb *ListenBrainzClient
|
||||
// mb is wired after construction (SetMusicBrainz) because the MB
|
||||
// client is built alongside this one; it is only used by the
|
||||
// owned-artist backfill, which tolerates its absence. Guarded by mu.
|
||||
mb *MusicBrainzClient
|
||||
artistImg *ArtistImageProvider
|
||||
logger *slog.Logger
|
||||
runtimeCtx context.Context // Wails runtime context for event emission
|
||||
@@ -345,23 +371,36 @@ func (si *SearchIndex) artistDiscogFetched(mbid string) bool {
|
||||
|
||||
// unenrichedLibraryArtistMBIDs returns MBIDs for owned artists whose
|
||||
// discography has not yet been fetched — either they have no index row
|
||||
// or their row is still discog_fetched = 0. The LEFT JOIN keys off the
|
||||
// persistent flag, so an artist enriched on a prior run (interactively or
|
||||
// by an earlier backfill) never reappears, giving "new artists only" for
|
||||
// free. Ordered by owned-track count so the artists the user has most of
|
||||
// are enriched first. The limit bounds a single run (see
|
||||
// or their row is still discog_fetched = 0, or the full MusicBrainz
|
||||
// browse has not run. The LEFT JOINs key off
|
||||
// persistent marks, so an artist enriched on a prior run (interactively
|
||||
// or by an earlier backfill) never reappears, giving "new artists only"
|
||||
// for free. Ordered by owned-track count so the artists the user has
|
||||
// most of are enriched first. The limit bounds a single run (see
|
||||
// discogBackfillMaxPerRun).
|
||||
//
|
||||
// The conditions are OR'd because they are different fetches: an
|
||||
// artist covered by the downloaded catalog artifact arrives with
|
||||
// discog_fetched = 1 and has still never been browsed, and the
|
||||
// artifact's own per-artist coverage is graded — so "the artifact knows
|
||||
// this artist" is not "we have their discography".
|
||||
//
|
||||
// similar_at is deliberately not one of them. The backfill no longer
|
||||
// fetches similar artists, so testing the mark it does not set would
|
||||
// make every owned artist a candidate on every run, forever.
|
||||
func (si *SearchIndex) unenrichedLibraryArtistMBIDs(limit int) []string {
|
||||
rows, err := si.db.QueryContext(`
|
||||
SELECT a.mbid
|
||||
FROM artists a
|
||||
LEFT JOIN explore_index ei
|
||||
ON ei.entity_type = 'artist' AND ei.mbid = a.mbid
|
||||
LEFT JOIN artist_enrichment ae ON ae.artist_mbid = a.mbid
|
||||
LEFT JOIN artist_credit_artist aca ON aca.artist_id = a.id
|
||||
LEFT JOIN recordings r ON r.artist_credit_id = aca.credit_id
|
||||
LEFT JOIN audio_files af ON af.recording_id = r.id
|
||||
WHERE a.mbid IS NOT NULL AND a.mbid != ''
|
||||
AND (ei.id IS NULL OR ei.discog_fetched = 0)
|
||||
AND (ei.id IS NULL OR ei.discog_fetched = 0
|
||||
OR ae.browsed_at IS NULL)
|
||||
GROUP BY a.mbid
|
||||
ORDER BY COUNT(DISTINCT af.id) DESC
|
||||
LIMIT ?
|
||||
@@ -386,14 +425,31 @@ func (si *SearchIndex) unenrichedLibraryArtistMBIDs(limit int) []string {
|
||||
return mbids
|
||||
}
|
||||
|
||||
// BackfillLibraryDiscographies fetches top release groups and recordings
|
||||
// for every owned artist that has not been enriched yet, so an artist's
|
||||
// wider catalogue is searchable offline right after a scan instead of
|
||||
// only on first artist-page view. It is bounded (discogBackfillMaxPerRun)
|
||||
// and resumable — each artist is marked discog_fetched on success, so a
|
||||
// cancelled or capped run simply continues on the next call. Runs through
|
||||
// discogSF so it never double-fetches an artist a concurrent interactive
|
||||
// EnsureArtistDiscography is already handling.
|
||||
// BackfillLibraryDiscographies makes an owned artist's page renderable
|
||||
// offline, so their catalogue is there right after a scan instead of
|
||||
// only on first view. Two fetches per artist, each skipped by its own
|
||||
// persistent mark:
|
||||
//
|
||||
// - the ListenBrainz top release groups and recordings (marked by
|
||||
// explore_index.discog_fetched), which is what popularity ordering
|
||||
// and the top-tracks section need;
|
||||
// - the full MusicBrainz browse (artist_enrichment.browsed_at), which
|
||||
// is what makes the discography complete and typed — see
|
||||
// browseFullDiscography.
|
||||
//
|
||||
// Similar artists are deliberately *not* fetched here. Nothing shows
|
||||
// them until someone opens an artist page, and that page already
|
||||
// resolves them on demand through Service.SimilarArtists →
|
||||
// ensureSimilarArtistsAsync, which stamps the same similar_at mark.
|
||||
// Fetching them for every owned artist bought a third of the run's
|
||||
// requests for a section most of those artists will never have shown.
|
||||
//
|
||||
// It is bounded (discogBackfillMaxPerRun) and resumable: a cancelled or
|
||||
// capped run continues on the next call, and each artist's marks are
|
||||
// set as they complete rather than at the end. Each artist runs
|
||||
// through discogSF so it never double-fetches one a concurrent
|
||||
// interactive EnsureArtistDiscography is already handling, and under
|
||||
// its own deadline so no single artist can stall the run.
|
||||
func (si *SearchIndex) BackfillLibraryDiscographies(ctx context.Context) {
|
||||
if si.lb == nil {
|
||||
return
|
||||
@@ -404,38 +460,100 @@ func (si *SearchIndex) BackfillLibraryDiscographies(ctx context.Context) {
|
||||
return
|
||||
}
|
||||
|
||||
// Everything below is work nobody is waiting on, so it yields the
|
||||
// shared MusicBrainz limiters to whatever the user is looking at.
|
||||
ctx = WithBackgroundPriority(ctx)
|
||||
|
||||
job, ctx := startBackfillJob(
|
||||
ctx, si.jobRegistry(), discogBackfillJobID,
|
||||
"Filling in artist details",
|
||||
"Discographies for artists in your library",
|
||||
len(mbids),
|
||||
)
|
||||
|
||||
defer func() { job.finish(ctx) }()
|
||||
|
||||
// One shared rate limiter paces the whole run, unlike the per-call
|
||||
// client EnsureArtistDiscography builds for interactive fetches.
|
||||
indexLB := NewListenBrainzClient(
|
||||
NewRateLimiterN(indexerRate), si.lb.cache, si.logger.WithGroup("indexer"),
|
||||
)
|
||||
|
||||
done := 0
|
||||
var (
|
||||
wg sync.WaitGroup
|
||||
done atomic.Int64
|
||||
work = make(chan string)
|
||||
)
|
||||
|
||||
wg.Add(discogBackfillWorkers)
|
||||
|
||||
for range discogBackfillWorkers {
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
|
||||
for mbid := range work {
|
||||
si.backfillOneArtist(ctx, indexLB, mbid)
|
||||
|
||||
job.progress(int(done.Add(1)), len(mbids))
|
||||
}
|
||||
}()
|
||||
}
|
||||
|
||||
for _, mbid := range mbids {
|
||||
if ctx.Err() != nil {
|
||||
return
|
||||
break
|
||||
}
|
||||
|
||||
_, _, _ = si.discogSF.Do(mbid, func() (any, error) {
|
||||
// Re-check under the singleflight: an interactive fetch may
|
||||
// have enriched this artist since the query above.
|
||||
if si.artistDiscogFetched(mbid) {
|
||||
return nil, nil
|
||||
}
|
||||
work <- mbid
|
||||
}
|
||||
|
||||
si.indexOneArtist(ctx, indexLB, lbSitewideArtist{
|
||||
close(work)
|
||||
wg.Wait()
|
||||
|
||||
total := int(done.Load())
|
||||
|
||||
if ctx.Err() != nil {
|
||||
si.logger.Info("discography backfill stopped",
|
||||
"artists", total, "of", len(mbids),
|
||||
)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
job.logf(jobs.LevelInfo, "Filled in "+strconv.Itoa(total)+" artists")
|
||||
|
||||
si.logger.Info("discography backfill complete", "artists", total)
|
||||
}
|
||||
|
||||
// backfillOneArtist runs one owned artist's fetches, under the
|
||||
// singleflight that keeps it from racing an interactive fetch and under
|
||||
// a deadline of its own (see discogBackfillArtistTimeout).
|
||||
func (si *SearchIndex) backfillOneArtist(
|
||||
ctx context.Context, lb *ListenBrainzClient, mbid string,
|
||||
) {
|
||||
if ctx.Err() != nil {
|
||||
return
|
||||
}
|
||||
|
||||
ctx, cancel := context.WithTimeout(ctx, discogBackfillArtistTimeout)
|
||||
defer cancel()
|
||||
|
||||
_, _, _ = si.discogSF.Do(mbid, func() (any, error) {
|
||||
// Re-checked under the singleflight: an interactive fetch may
|
||||
// have done either of these since the query above.
|
||||
if !si.artistDiscogFetched(mbid) {
|
||||
si.indexOneArtist(ctx, lb, lbSitewideArtist{
|
||||
ArtistMBID: mbid,
|
||||
ArtistName: si.artistDisplayName(mbid),
|
||||
})
|
||||
}
|
||||
|
||||
return nil, nil
|
||||
})
|
||||
if !si.enrichmentFor(mbid).Browsed {
|
||||
si.browseFullDiscography(ctx, mbid)
|
||||
}
|
||||
|
||||
done++
|
||||
}
|
||||
|
||||
si.logger.Info("discography backfill complete", "artists", done)
|
||||
return nil, nil
|
||||
})
|
||||
}
|
||||
|
||||
// artistDisplayName resolves a human-readable name for an artist MBID,
|
||||
@@ -1937,14 +2055,23 @@ func (si *SearchIndex) indexOneArtist(
|
||||
recs = si.fetchTopRecordings(ctx, lb, artist, recLimit)
|
||||
}()
|
||||
|
||||
// MB pipeline: resolve + cache artist image (uses MB rate limiter).
|
||||
// MB pipeline: cache the artist lookup, which is what the details
|
||||
// below are read from (uses MB rate limiter).
|
||||
//
|
||||
// Deliberately *not* GetArtistImage. This function wants the MB
|
||||
// artist response; that entry point additionally queried fanart.tv,
|
||||
// TheAudioDB, Wikidata and Wikipedia and downloaded up to ten
|
||||
// full-size portraits per artist, none of which any caller here
|
||||
// reads. A portrait is resolved when a view asks for one.
|
||||
wg.Add(1)
|
||||
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
|
||||
if si.artistImg != nil {
|
||||
si.artistImg.GetArtistImage(artist.ArtistMBID)
|
||||
// Carries the caller's priority: interactive from
|
||||
// EnsureArtistDiscography, background from the backfill.
|
||||
si.artistImg.EnsureArtistRels(ctx, artist.ArtistMBID)
|
||||
}
|
||||
}()
|
||||
|
||||
|
||||
@@ -26,6 +26,14 @@ const (
|
||||
KindIndexBuild Kind = "index-build"
|
||||
KindDownload Kind = "download"
|
||||
KindAutotagApply Kind = "autotag-apply"
|
||||
|
||||
// KindCatalogEnrich is background catalog work for content the user
|
||||
// already owns — the discography backfills. It is distinct from
|
||||
// KindIndexBuild because the two differ in what cancelling costs:
|
||||
// an index build discards hours of downloading and the frontend
|
||||
// confirms before stopping one, where a backfill is resumable per
|
||||
// artist and stopping it is free.
|
||||
KindCatalogEnrich Kind = "catalog-enrich"
|
||||
)
|
||||
|
||||
// State is the lifecycle position of a job.
|
||||
|
||||
@@ -11,6 +11,7 @@ import (
|
||||
"time"
|
||||
|
||||
"yellowjacket/backend/database"
|
||||
"yellowjacket/backend/explore"
|
||||
)
|
||||
|
||||
// errTestJobFailed stands in for a job returning an error.
|
||||
@@ -287,6 +288,12 @@ func TestOrphanedCoverFilesJob_EmptyLiveSetIsNoOp(t *testing.T) {
|
||||
|
||||
// Artwork for an artist in the library is kept regardless of age;
|
||||
// artwork for a browsed artist ages out.
|
||||
//
|
||||
// The directories are laid out by explore.ArtistImageDir rather than by
|
||||
// this test, which is the point: the job used to join the bare MBID,
|
||||
// name a path that has never existed, delete the rows and leave every
|
||||
// file on disk. A test that invents its own flat layout agrees with
|
||||
// the bug.
|
||||
func TestOrphanedArtistImagesJob(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
@@ -300,7 +307,7 @@ func TestOrphanedArtistImagesJob(t *testing.T) {
|
||||
)
|
||||
|
||||
for _, mbid := range []string{ownedMBID, browsedMBID, recentMBID} {
|
||||
artistDir := filepath.Join(dir, mbid)
|
||||
artistDir := explore.ArtistImageDir(dir, mbid)
|
||||
if err := os.MkdirAll(artistDir, 0o755); err != nil {
|
||||
t.Fatalf("mkdir %s: %v", mbid, err)
|
||||
}
|
||||
@@ -334,27 +341,27 @@ func TestOrphanedArtistImagesJob(t *testing.T) {
|
||||
(artist_mbid, source, source_url, file_path, created_at)
|
||||
VALUES (?, 'test', 'http://x', ?, ?)`,
|
||||
tc.mbid,
|
||||
filepath.Join(dir, tc.mbid, "primary.jpg"),
|
||||
filepath.Join(explore.ArtistImageDir(dir, tc.mbid), "primary.jpg"),
|
||||
tc.created,
|
||||
); err != nil {
|
||||
t.Fatalf("seed artist_images for %s: %v", tc.mbid, err)
|
||||
}
|
||||
}
|
||||
|
||||
if _, err := OrphanedArtistImagesJob(db, dir).
|
||||
if _, err := OrphanedArtistImagesJob(db, dir, explore.ArtistImageDir).
|
||||
Run(context.Background()); err != nil {
|
||||
t.Fatalf("run job: %v", err)
|
||||
}
|
||||
|
||||
if _, err := os.Stat(filepath.Join(dir, ownedMBID)); err != nil {
|
||||
if _, err := os.Stat(explore.ArtistImageDir(dir, ownedMBID)); err != nil {
|
||||
t.Error("artwork for a library artist was evicted")
|
||||
}
|
||||
|
||||
if _, err := os.Stat(filepath.Join(dir, recentMBID)); err != nil {
|
||||
if _, err := os.Stat(explore.ArtistImageDir(dir, recentMBID)); err != nil {
|
||||
t.Error("recently fetched artwork was evicted")
|
||||
}
|
||||
|
||||
if _, err := os.Stat(filepath.Join(dir, browsedMBID)); !os.IsNotExist(err) {
|
||||
if _, err := os.Stat(explore.ArtistImageDir(dir, browsedMBID)); !os.IsNotExist(err) {
|
||||
t.Error("stale browsed artwork survived the sweep")
|
||||
}
|
||||
|
||||
@@ -432,3 +439,85 @@ func TestSweepMissingDirectory(t *testing.T) {
|
||||
t.Errorf("FilesDeleted = %d, want 0", result.FilesDeleted)
|
||||
}
|
||||
}
|
||||
|
||||
// A directory holding an artist's portrait plus the candidates an older
|
||||
// version downloaded keeps the portrait and loses the candidates.
|
||||
func TestStrayArtistImageFilesJob(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
dir := t.TempDir()
|
||||
|
||||
const mbid = "44444444-4444-4444-4444-444444444444"
|
||||
|
||||
artistDir := explore.ArtistImageDir(dir, mbid)
|
||||
if err := os.MkdirAll(artistDir, 0o755); err != nil {
|
||||
t.Fatalf("mkdir: %v", err)
|
||||
}
|
||||
|
||||
keep := []string{
|
||||
"primary.jpg", "primary_sm.jpg", "primary_md.jpg",
|
||||
"primary_lg.jpg", ".miss",
|
||||
}
|
||||
strays := []string{"audiodb_0.jpg", "fanart_1.jpg", "wikidata_3.jpg"}
|
||||
|
||||
for _, name := range append(append([]string{}, keep...), strays...) {
|
||||
if err := os.WriteFile(
|
||||
filepath.Join(artistDir, name), []byte("xx"), 0o600,
|
||||
); err != nil {
|
||||
t.Fatalf("write %s: %v", name, err)
|
||||
}
|
||||
}
|
||||
|
||||
result, err := StrayArtistImageFilesJob(dir, explore.ArtistImageKeepNames()).
|
||||
Run(context.Background())
|
||||
if err != nil {
|
||||
t.Fatalf("run job: %v", err)
|
||||
}
|
||||
|
||||
if result.FilesDeleted != int64(len(strays)) {
|
||||
t.Errorf("FilesDeleted = %d, want %d", result.FilesDeleted, len(strays))
|
||||
}
|
||||
|
||||
for _, name := range keep {
|
||||
if _, err := os.Stat(filepath.Join(artistDir, name)); err != nil {
|
||||
t.Errorf("%s was swept and should not have been", name)
|
||||
}
|
||||
}
|
||||
|
||||
for _, name := range strays {
|
||||
if _, err := os.Stat(
|
||||
filepath.Join(artistDir, name),
|
||||
); !os.IsNotExist(err) {
|
||||
t.Errorf("%s survived the sweep", name)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// An empty keep set would condemn every file, which is never what a
|
||||
// caller means — it is a failed lookup, not an empty live set.
|
||||
func TestStrayArtistImageFilesJobRefusesEmptyKeepSet(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
dir := t.TempDir()
|
||||
|
||||
const mbid = "55555555-5555-5555-5555-555555555555"
|
||||
|
||||
artistDir := explore.ArtistImageDir(dir, mbid)
|
||||
if err := os.MkdirAll(artistDir, 0o755); err != nil {
|
||||
t.Fatalf("mkdir: %v", err)
|
||||
}
|
||||
|
||||
primary := filepath.Join(artistDir, "primary.jpg")
|
||||
if err := os.WriteFile(primary, []byte("xx"), 0o600); err != nil {
|
||||
t.Fatalf("write: %v", err)
|
||||
}
|
||||
|
||||
if _, err := StrayArtistImageFilesJob(dir, nil).
|
||||
Run(context.Background()); err != nil {
|
||||
t.Fatalf("run job: %v", err)
|
||||
}
|
||||
|
||||
if _, err := os.Stat(primary); err != nil {
|
||||
t.Error("an empty keep set emptied the directory")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -140,7 +140,20 @@ func liveCoverFiles(
|
||||
// an artist the user owns is kept indefinitely, and everything else ages
|
||||
// out. Rows whose file has vanished are dropped so the table matches
|
||||
// what is actually on disk.
|
||||
func OrphanedArtistImagesJob(db *database.DB, artistImagesDir string) Job {
|
||||
//
|
||||
// dirFor maps an artist MBID to its directory. It is a parameter, as
|
||||
// OrphanedCoverFilesJob's expandVariants is, because that layout is the
|
||||
// image provider's business and this job had been guessing it: artist
|
||||
// directories are sharded under a two-character prefix, so joining the
|
||||
// bare MBID named a path that has never existed. RemoveAll succeeds on
|
||||
// a missing path, so the job reported success, freed nothing, and
|
||||
// deleted the rows that were the only record of the files it left
|
||||
// behind.
|
||||
func OrphanedArtistImagesJob(
|
||||
db *database.DB,
|
||||
artistImagesDir string,
|
||||
dirFor func(baseDir, mbid string) string,
|
||||
) Job {
|
||||
return Job{
|
||||
Name: "artist-images-sweep",
|
||||
MinInterval: dailyInterval,
|
||||
@@ -162,7 +175,7 @@ func OrphanedArtistImagesJob(db *database.DB, artistImagesDir string) Job {
|
||||
return result, nil
|
||||
}
|
||||
|
||||
dir := filepath.Join(artistImagesDir, mbid)
|
||||
dir := dirFor(artistImagesDir, mbid)
|
||||
|
||||
freed, files := dirSize(dir)
|
||||
|
||||
@@ -188,6 +201,82 @@ func OrphanedArtistImagesJob(db *database.DB, artistImagesDir string) Job {
|
||||
}
|
||||
}
|
||||
|
||||
// StrayArtistImageFilesJob removes downloaded image candidates that
|
||||
// were never the artist's portrait.
|
||||
//
|
||||
// The image provider used to download every candidate an upstream
|
||||
// offered — up to ten, full size — and keep them all, while nothing in
|
||||
// the app has ever read anything but primary.jpg and its three size
|
||||
// tiers. It now downloads candidates in priority order until one
|
||||
// succeeds and records the rest as URLs, so nothing new lands here;
|
||||
// this reclaims what earlier versions left, which on a real cache was
|
||||
// about four fifths of it.
|
||||
//
|
||||
// keep is the set of names an artist directory is allowed to hold. It
|
||||
// is a parameter for the same reason dirFor above is: the provider owns
|
||||
// the naming, and a sweep that guesses it deletes the wrong files.
|
||||
func StrayArtistImageFilesJob(artistImagesDir string, keep map[string]bool) Job {
|
||||
return Job{
|
||||
Name: "artist-images-strays",
|
||||
MinInterval: dailyInterval,
|
||||
Run: func(ctx context.Context) (Result, error) {
|
||||
// An empty keep set would mean "every file is garbage",
|
||||
// which is never the intent — refuse, as the covers sweep
|
||||
// refuses an empty live set.
|
||||
if len(keep) == 0 {
|
||||
return Result{}, nil
|
||||
}
|
||||
|
||||
var result Result
|
||||
|
||||
shards, err := os.ReadDir(artistImagesDir)
|
||||
if err != nil {
|
||||
if os.IsNotExist(err) {
|
||||
return Result{}, nil
|
||||
}
|
||||
|
||||
return Result{}, fmt.Errorf("read %s: %w", artistImagesDir, err)
|
||||
}
|
||||
|
||||
for _, shard := range shards {
|
||||
if !shard.IsDir() {
|
||||
continue
|
||||
}
|
||||
|
||||
shardDir := filepath.Join(artistImagesDir, shard.Name())
|
||||
|
||||
artists, err := os.ReadDir(shardDir)
|
||||
if err != nil {
|
||||
continue
|
||||
}
|
||||
|
||||
for _, artist := range artists {
|
||||
if ctx.Err() != nil {
|
||||
return result, nil
|
||||
}
|
||||
|
||||
if !artist.IsDir() {
|
||||
continue
|
||||
}
|
||||
|
||||
swept, err := sweepDir(ctx,
|
||||
filepath.Join(shardDir, artist.Name()),
|
||||
func(name string) bool { return !keep[name] },
|
||||
)
|
||||
if err != nil {
|
||||
continue
|
||||
}
|
||||
|
||||
result.FilesDeleted += swept.FilesDeleted
|
||||
result.BytesFreed += swept.BytesFreed
|
||||
}
|
||||
}
|
||||
|
||||
return result, nil
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
// staleArtistMBIDs returns artist MBIDs whose cached artwork may be
|
||||
// evicted: fetched before the cutoff and not an artist in the library.
|
||||
func staleArtistMBIDs(
|
||||
|
||||
Reference in New Issue
Block a user