diff --git a/backend/app.go b/backend/app.go index bcb917e..2ea2010 100644 --- a/backend/app.go +++ b/backend/app.go @@ -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), diff --git a/backend/database/database.go b/backend/database/database.go index f51f91e..1366c9b 100644 --- a/backend/database/database.go +++ b/backend/database/database.go @@ -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 diff --git a/backend/database/explorefts_test.go b/backend/database/explorefts_test.go index 1b4e069..a7d375e 100644 --- a/backend/database/explorefts_test.go +++ b/backend/database/explorefts_test.go @@ -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) + } +} diff --git a/backend/database/sql/schemas/artist_enrichment.sql b/backend/database/sql/schemas/artist_enrichment.sql new file mode 100644 index 0000000..f479dfc --- /dev/null +++ b/backend/database/sql/schemas/artist_enrichment.sql @@ -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 +); diff --git a/backend/database/sql/sqlcgen/models.go b/backend/database/sql/sqlcgen/models.go index 4312ed0..627ac48 100644 --- a/backend/database/sql/sqlcgen/models.go +++ b/backend/database/sql/sqlcgen/models.go @@ -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 diff --git a/backend/datamap/datamap.go b/backend/datamap/datamap.go index fd2f20f..ca29bdf 100644 --- a/backend/datamap/datamap.go +++ b/backend/datamap/datamap.go @@ -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 " + diff --git a/backend/explore/artistenrichment.go b/backend/explore/artistenrichment.go new file mode 100644 index 0000000..44662ba --- /dev/null +++ b/backend/explore/artistenrichment.go @@ -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 +} diff --git a/backend/explore/artistenrichment_test.go b/backend/explore/artistenrichment_test.go new file mode 100644 index 0000000..b282353 --- /dev/null +++ b/backend/explore/artistenrichment_test.go @@ -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 +} diff --git a/backend/explore/artistimage.go b/backend/explore/artistimage.go index 5b12704..10a79f2 100644 --- a/backend/explore/artistimage.go +++ b/backend/explore/artistimage.go @@ -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) } // --------------------------------------------------------------------------- diff --git a/backend/explore/artistimage_test.go b/backend/explore/artistimage_test.go new file mode 100644 index 0000000..53269cd --- /dev/null +++ b/backend/explore/artistimage_test.go @@ -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") + } +} diff --git a/backend/explore/assetdirs.go b/backend/explore/assetdirs.go index 02b601a..f5a56ad 100644 --- a/backend/explore/assetdirs.go +++ b/backend/explore/assetdirs.go @@ -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 +} diff --git a/backend/explore/backfilljob.go b/backend/explore/backfilljob.go new file mode 100644 index 0000000..158ce11 --- /dev/null +++ b/backend/explore/backfilljob.go @@ -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() +} diff --git a/backend/explore/browseall_test.go b/backend/explore/browseall_test.go new file mode 100644 index 0000000..b967635 --- /dev/null +++ b/backend/explore/browseall_test.go @@ -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) + } +} diff --git a/backend/explore/explore.go b/backend/explore/explore.go index ba08a3b..526a0cf 100644 --- a/backend/explore/explore.go +++ b/backend/explore/explore.go @@ -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 } } diff --git a/backend/explore/lyrics.go b/backend/explore/lyrics.go index 5ed116b..2b233b1 100644 --- a/backend/explore/lyrics.go +++ b/backend/explore/lyrics.go @@ -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++ diff --git a/backend/explore/musicbrainz.go b/backend/explore/musicbrainz.go index 190e98d..83598e7 100644 --- a/backend/explore/musicbrainz.go +++ b/backend/explore/musicbrainz.go @@ -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. diff --git a/backend/explore/ratelimiter.go b/backend/explore/ratelimiter.go index d9a06ad..1b99f6d 100644 --- a/backend/explore/ratelimiter.go +++ b/backend/explore/ratelimiter.go @@ -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 +} diff --git a/backend/explore/ratelimiter_test.go b/backend/explore/ratelimiter_test.go index 6c80544..0b009a3 100644 --- a/backend/explore/ratelimiter_test.go +++ b/backend/explore/ratelimiter_test.go @@ -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() diff --git a/backend/explore/searchindex.go b/backend/explore/searchindex.go index 37d601e..0f8b551 100644 --- a/backend/explore/searchindex.go +++ b/backend/explore/searchindex.go @@ -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) } }() diff --git a/backend/jobs/jobs.go b/backend/jobs/jobs.go index 0d80fad..1f825ab 100644 --- a/backend/jobs/jobs.go +++ b/backend/jobs/jobs.go @@ -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. diff --git a/backend/maintenance/maintenance_test.go b/backend/maintenance/maintenance_test.go index 246c247..d248323 100644 --- a/backend/maintenance/maintenance_test.go +++ b/backend/maintenance/maintenance_test.go @@ -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") + } +} diff --git a/backend/maintenance/sweeps.go b/backend/maintenance/sweeps.go index 7a0aca6..1f44b0b 100644 --- a/backend/maintenance/sweeps.go +++ b/backend/maintenance/sweeps.go @@ -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(