From cbd82a5a74bfe0c977c12986596a7482c5de7b5a Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Mon, 10 Aug 2026 11:52:26 -0400 Subject: [PATCH] feat: autotag mixed-bag splitting, search relevance fixes, and multi-library download imports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Autotag: detect "junk drawer" folders with no artist/album consensus and split them into synthetic per-cluster groups instead of forcing one match on an unrelated pile of tracks; repair tagging_items rows left behind by a prior scan orphan-cleanup gap. Explore: fix an exact artist-name search being drowned out by its own catalog entries in intent-prior scoring, and prune stale in_library bookkeeping left behind when a referenced library row is deleted. Download: fix a multi-library regression where every import failed with "no library root configured" — the importer resolved the library root from a legacy single-library config field that nothing populates in the current multi-library model. It now resolves the destination library per-request from the request's own library_id. Also widen the Soulseek search window (12s -> 20s), measured against real request history to be missing available peers on live queries. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Y2Agd9af5hE7qzti2ackiS --- .planning/NOTES.md | 40 +- CLAUDE.md | 51 ++- backend/app.go | 1 - backend/autotag/groupkey.go | 69 ++- backend/autotag/groupkey_test.go | 23 + backend/autotag/local.go | 18 +- backend/autotag/mb.go | 42 +- backend/autotag/mb_test.go | 94 ++++ backend/autotag/mixedbag.go | 215 ++++++++++ backend/autotag/mixedbag_test.go | 246 +++++++++++ backend/autotag/rank.go | 33 +- backend/autotag/rank_test.go | 49 +++ backend/autotag/recommend.go | 11 +- backend/autotag/recommend_test.go | 40 ++ backend/autotag/scorer.go | 2 + backend/autotag/scorer_test.go | 14 + backend/autotag/types.go | 18 + backend/autotagservice/service.go | 345 +++++++++++++-- backend/autotagservice/service_test.go | 406 ++++++++++++++++++ backend/database/database.go | 178 +++++++- backend/database/migrations_test.go | 196 +++++++++ .../0001_tagging_items_synthetic.sql | 10 + .../0002_tagging_items_orphan_cleanup.sql | 24 ++ .../database/sql/queries/artist_credit.sql | 8 + .../sql/queries/artist_credit_artists.sql | 4 + backend/database/sql/queries/artists.sql | 9 + backend/database/sql/queries/recordings.sql | 8 + .../sql/queries/release_group_recordings.sql | 4 + .../database/sql/queries/release_groups.sql | 8 + .../database/sql/queries/tagging_items.sql | 69 ++- .../database/sql/schemas/tagging_items.sql | 29 ++ .../database/sql/sqlcgen/artist_credit.sql.go | 32 ++ .../sql/sqlcgen/artist_credit_artists.sql.go | 10 + backend/database/sql/sqlcgen/artists.sql.go | 33 ++ backend/database/sql/sqlcgen/models.go | 2 + .../database/sql/sqlcgen/recordings.sql.go | 32 ++ .../sqlcgen/release_group_recordings.sql.go | 10 + .../sql/sqlcgen/release_groups.sql.go | 32 ++ .../database/sql/sqlcgen/tagging_items.sql.go | 122 +++++- backend/datamap/datamap.go | 7 + backend/download/importer.go | 6 + backend/download/importer_test.go | 8 + backend/download/manager.go | 8 + backend/download/manager_test.go | 4 +- backend/download/provider.go | 11 + backend/download/provider_slskd.go | 19 +- backend/download/service.go | 49 ++- backend/download/types.go | 34 ++ backend/download/types_test.go | 55 +++ backend/explore/autotagclient.go | 58 ++- backend/explore/explore.go | 64 ++- backend/explore/intentprior_test.go | 113 +++++ backend/explore/prune_test.go | 156 +++++++ backend/explore/searchindex.go | 63 ++- backend/library/crud.go | 10 + backend/library/leak_test.go | 2 + backend/library/library.go | 155 +++++++ backend/library/scan_test.go | 174 ++++++++ .../config-page/download-clients.ts | 101 +++-- .../duplicate-tracks-dialog.ts | 2 +- .../explore-album-details.ts | 46 +- .../explore-artist-details.ts | 8 +- .../src/components/wanted-view/wanted-view.ts | 11 +- frontend/src/store/library-store.ts | 19 + .../wailsjs/go/autotagservice/Service.d.ts | 2 + frontend/wailsjs/go/autotagservice/Service.js | 4 + frontend/wailsjs/go/library/Library.d.ts | 2 + frontend/wailsjs/go/library/Library.js | 4 + frontend/wailsjs/go/models.ts | 12 + go.mod | 2 +- 70 files changed, 3617 insertions(+), 129 deletions(-) create mode 100644 backend/autotag/mixedbag.go create mode 100644 backend/autotag/mixedbag_test.go create mode 100644 backend/autotagservice/service_test.go create mode 100644 backend/database/migrations_test.go create mode 100644 backend/database/sql/migrations/0001_tagging_items_synthetic.sql create mode 100644 backend/database/sql/migrations/0002_tagging_items_orphan_cleanup.sql create mode 100644 backend/download/types_test.go create mode 100644 backend/explore/intentprior_test.go create mode 100644 backend/explore/prune_test.go diff --git a/.planning/NOTES.md b/.planning/NOTES.md index fb37b9b..25248d7 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -59,13 +59,36 @@ yet to test against. Confirm before relying on it. Also worth deciding deliberately: every install pulling from a personal Gitea makes its bandwidth and uptime a user-facing dependency. -## No migration chain +## Migrations came back (2026-08-08), scoped to avoid the old failure mode -`applySchema` creates the whole schema from `sql/schemas/*.sql` on every -open; all DDL is `IF NOT EXISTS`. A database written by an older build is -not supported and there is no upgrade path by design. +The "no migration chain" design below lasted until a real `make sandbox` +DB (schema pre-dating the `tagging_items.synthetic`/`parent_group_key` +columns) hit `no such column: parent_group_key` — `IF NOT EXISTS` had +silently no-op'd the `CREATE TABLE` on the existing table, columns and +all. A database written by an older build genuinely needed an upgrade +path; there wasn't one. -Two things this replaced, worth not reintroducing: +What came back is **not** the old 48-step chain. `sql/schemas/*.sql` +stays the single source of truth for the current shape (still what sqlc +reads, still what a fresh install gets verbatim). `sql/migrations/*.sql` +holds small numbered files — `ALTER TABLE ADD COLUMN`, `CREATE INDEX`, +etc. — that run after the schema files, tracked in `schema_migrations`, +tolerating "duplicate column name" as a no-op so the exact same files run +unconditionally on both a fresh database and an old one and converge on +one shape. See the "Schema changes need two things, not one" section in +CLAUDE.md for the column-order and index-placement gotchas this +implies, and `backend/database/migrations_test.go` for the regression +tests. Squashing `sql/migrations/` back into `sql/schemas/` and deleting +the migration files is fine pre-1.0 (see CLAUDE.md); stop once real user +databases exist. + +The original decision this replaces, kept for why the old chain died: + +`applySchema` created the whole schema from `sql/schemas/*.sql` on every +open; all DDL was `IF NOT EXISTS`. A database written by an older build +was not supported and there was no upgrade path, by design. + +Two things that removal fixed, worth not reintroducing: - The 48-step chain was ~3,700 of `database.go`'s 4,061 lines, plus helpers that existed only to serve it (`backupDatabase`, @@ -78,6 +101,13 @@ Two things this replaced, worth not reintroducing: generating against a stale schema and silently missed columns such as `audio_files.modified_at`. + The new design's answer to this specific risk: `sql/schemas/` is + never edited to describe something migrations already did elsewhere + — it's edited to directly declare the target shape, and migrations + exist only to carry an old on-disk database to that same shape. There + is exactly one hand-maintained description of "what does the schema + look like", same as before; migrations don't add a second one. + **When regenerating schema files from a live database, remember the seed rows.** `file_types` (the four supported extensions), `player_state` and `queue` each carry `INSERT OR IGNORE` rows that `sqlite_master` does not diff --git a/CLAUDE.md b/CLAUDE.md index d9ed9aa..0768503 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -60,10 +60,53 @@ Audio playback integration tests require `YELLOWJACKET_INTEGRATION=1`. - `queue` — Track queue with shuffle (Fisher-Yates), repeat modes, auto-advance, and session persistence. - `library` — Concurrent library scanning, metadata extraction, cover art deduplication, incremental rescan. - `database` — SQLite via pure-Go driver. Schema in `database/sql/schemas/`, queries in `database/sql/queries/`. **sqlc** generates Go code into `database/sql/sqlcgen/` — never edit that directory by hand. - There is **no migration chain**: `applySchema` creates everything from - the schema files on every open (all DDL is `IF NOT EXISTS`), and a - database written by an older build is not supported. Changing the - schema means editing the file in `sql/schemas/`, not adding a step. + + **Schema changes need two things, not one.** `sql/schemas/*.sql` is + `CREATE ... IF NOT EXISTS` and is what sqlc reads — it's the single + source of truth for "what the schema looks like right now", and it's + what a fresh install gets verbatim. But it's a no-op against a + database that already has the table, so an existing install needs a + matching file in `sql/schemas/../migrations/` (e.g. + `NNNN_description.sql`, `ALTER TABLE ... ADD COLUMN ...` / + `CREATE INDEX ...`) to actually reach that shape. Both run on every + open, migrations after schema files, tracked in `schema_migrations` + so each applies once; a migration's `ALTER TABLE ADD COLUMN` failing + with "duplicate column name" on an already-current database is + expected and tolerated, not an error. + + A few things that bite if forgotten: + - **Column order must match between the two paths.** `ALTER TABLE + ADD COLUMN` always appends at the end, so a migrated column must + also be declared *last* in the `CREATE TABLE` in `sql/schemas/` + — otherwise a fresh install and an upgraded install disagree on + column order, and a `SELECT *` query (sqlc binds those + positionally) silently reads the wrong field on one of them. See + `backend/database/migrations_test.go`'s + `TestMigrations_ColumnOrderMatchesFreshInstall`, which is the + regression test for exactly this. + - **Don't put an index on a migrated column in `sql/schemas/`.** + Schema files run before migrations, against a database that may + not have that column yet — the index's predicate would fail + (this is precisely the bug an earlier session shipped and a user + hit at `make sandbox`). Declare it in the migration file instead, + after the `ALTER TABLE` that adds the column. + - This project **had** a 48-step migration chain before and tore it + out (see `.planning/NOTES.md`, "No migration chain") because + `sql/schemas/` had drifted from what the migrations actually + produced and sqlc silently generated against the stale version. + The design here avoids that by keeping `sql/schemas/` as the + literal target shape (not a hand-maintained description of it) + and letting migrations replay tolerantly against it — but the + same drift is possible again if a schema change ships without + updating both files. Don't reintroduce a *second* description of + the schema anywhere else. + - **Squashing is fine pre-1.0.** While this hasn't shipped to real + users, periodically folding `sql/migrations/` into `sql/schemas/` + and deleting the migration files (then wiping your own dev/sandbox + DB) is a legitimate way to keep the migrations directory from + accumulating dev-only churn — same effect as the old "just nuke + it" workflow, opt-in instead of mandatory. Stop doing that once + real user databases exist in the wild. - `metadata` — Tag extraction (ID3v2, Vorbis Comments, FLAC). - `config` — TOML-based settings. Settings page uses HTMX + templ for server-rendered HTML fragments. - `playlist` / `smartplaylist` — Playlist CRUD and rule-based smart playlists. diff --git a/backend/app.go b/backend/app.go index c47a26a..f447726 100644 --- a/backend/app.go +++ b/backend/app.go @@ -270,7 +270,6 @@ func (yj *YellowJacketApp) initDownloadRuntime(ctx context.Context) { } yj.downloads.SetImportOptions(download.ImportOptions{ - LibraryRoot: yj.appConfig.GetLibraryDirectory(), PathTemplate: cfg.PathTemplate, }) yj.downloads.SetMaxConcurrent(cfg.MaxConcurrent) diff --git a/backend/autotag/groupkey.go b/backend/autotag/groupkey.go index d4cee45..86fd18b 100644 --- a/backend/autotag/groupkey.go +++ b/backend/autotag/groupkey.go @@ -19,12 +19,14 @@ import ( // // libraryID || 0 || normalized_parent_dir || 0 || disc_number // -// where the parent directory is lower-cased. The folder is taken -// as the album boundary — including the album tag string would -// fragment albums whose tracks carry slightly different tags -// (`Abbey Road` vs `Abbey Road (Remastered 2009)`, etc.). The -// album name is still surfaced in `tagging_items.album_name` for -// the review UI; it just doesn't decide grouping. +// where the parent directory is lower-cased and disc_number is +// normalized so an untagged disc (0) folds into disc 1 — see +// normalizeDiscNumber. The folder is taken as the album boundary — +// including the album tag string would fragment albums whose tracks +// carry slightly different tags (`Abbey Road` vs `Abbey Road +// (Remastered 2009)`, etc.). The album name is still surfaced in +// `tagging_items.album_name` for the review UI; it just doesn't +// decide grouping. // // Using SHA-1 matches the codebase's existing non-crypto // deterministic-key convention; collision risk at album-group @@ -41,7 +43,60 @@ func GroupKey( h.Write([]byte{0}) h.Write([]byte(parentDir)) h.Write([]byte{0}) - h.Write([]byte(strconv.Itoa(discNumber))) + h.Write([]byte(strconv.Itoa(normalizeDiscNumber(discNumber)))) return hex.EncodeToString(h.Sum(nil)) } + +// SyntheticGroupKey returns a deterministic identifier for a +// tag-clustered sub-group carved out of parentGroupKey by +// SplitMixedFolder — same SHA-1-over-null-separated-fields shape as +// GroupKey, but keyed on the cluster's (album, album-artist) tags +// instead of a directory, since a synthetic group's tracks don't +// share a directory boundary distinct from their siblings left +// behind in the parent folder. +func SyntheticGroupKey(parentGroupKey, albumName, albumArtist string) string { + h := sha1.New() //nolint:gosec // see package doc — grouping only. + h.Write([]byte(parentGroupKey)) + h.Write([]byte{0}) + h.Write([]byte(Normalize(albumName))) + h.Write([]byte{0}) + h.Write([]byte(Normalize(albumArtist))) + + return hex.EncodeToString(h.Sum(nil)) +} + +// SyntheticTrackGroupKey returns a deterministic identifier for a +// single leftover track carved out of a mixed-bag folder by +// SplitMixedFolder's singleton fallback (autotag.SplitPlan). Keyed on +// the track's own audio_files id rather than its tags — two +// untagged leftover tracks would otherwise both normalize to the +// same empty (album, album-artist) pair and collide under +// SyntheticGroupKey. +func SyntheticTrackGroupKey(parentGroupKey string, audioFileID int64) string { + h := sha1.New() //nolint:gosec // see package doc — grouping only. + h.Write([]byte(parentGroupKey)) + h.Write([]byte{0}) + h.Write([]byte("track")) + h.Write([]byte{0}) + h.Write([]byte(strconv.FormatInt(audioFileID, 10))) + + return hex.EncodeToString(h.Sum(nil)) +} + +// normalizeDiscNumber folds a missing/invalid disc tag (<= 0) into +// disc 1 for grouping purposes. Without this, a folder where only +// some tracks carry an explicit "disc 1 of 1" tag — common when +// files were ripped or re-tagged at different times — splits into +// two tagging groups for what is really one single-disc album: the +// untagged tracks hash to disc 0, the tagged ones to disc 1. A +// genuine multi-disc release still separates correctly, since its +// disc-2-and-up tracks carry an explicit non-zero, non-one disc +// number. +func normalizeDiscNumber(discNumber int) int { + if discNumber <= 0 { + return 1 + } + + return discNumber +} diff --git a/backend/autotag/groupkey_test.go b/backend/autotag/groupkey_test.go index ecedde7..ad1612c 100644 --- a/backend/autotag/groupkey_test.go +++ b/backend/autotag/groupkey_test.go @@ -88,6 +88,29 @@ func TestGroupKey_DistinctInputsDiffer(t *testing.T) { } } +func TestGroupKey_UntaggedDiscFoldsIntoDiscOne(t *testing.T) { + t.Parallel() + + // A folder where only some tracks carry an explicit disc tag must + // not split: the untagged tracks (disc 0, dhowden/tag's zero value + // for a missing frame) should group with the ones tagged disc 1. + untagged := autotag.GroupKey(1, "/music/Artist/Album/01.mp3", 0) + tagged := autotag.GroupKey(1, "/music/Artist/Album/02.mp3", 1) + + if untagged != tagged { + t.Fatalf( + "disc 0 and disc 1 in the same folder should share a key, got %q vs %q", + untagged, tagged, + ) + } + + // A genuine disc 2 must still separate from disc 1/untagged. + discTwo := autotag.GroupKey(1, "/music/Artist/Album/01.mp3", 2) + if discTwo == tagged { + t.Fatalf("disc 2 should not share a key with disc 1, got %q", discTwo) + } +} + func TestGroupKey_AmbiguityBoundary(t *testing.T) { t.Parallel() diff --git a/backend/autotag/local.go b/backend/autotag/local.go index 5e46046..759fde7 100644 --- a/backend/autotag/local.go +++ b/backend/autotag/local.go @@ -39,14 +39,16 @@ func (r *LocalResolver) LocalTracksForGroup( out := make([]LocalTrack, 0, len(rows)) for _, row := range rows { out = append(out, LocalTrack{ - AudioFileID: row.ID, - FilePath: row.FilePath, - Title: row.Title, - Artist: row.ArtistName, - TrackNumber: int(row.TrackNumber), - DiscNumber: int(row.DiscNumber), - LengthMillis: row.LengthMilliseconds, - RecordingMBID: row.RecordingMbid, + AudioFileID: row.ID, + FilePath: row.FilePath, + Title: row.Title, + Artist: row.ArtistName, + TrackNumber: int(row.TrackNumber), + DiscNumber: int(row.DiscNumber), + LengthMillis: row.LengthMilliseconds, + RecordingMBID: row.RecordingMbid, + AlbumTag: row.AlbumName, + AlbumArtistTag: row.AlbumArtist, }) } diff --git a/backend/autotag/mb.go b/backend/autotag/mb.go index 80ba3f9..1253553 100644 --- a/backend/autotag/mb.go +++ b/backend/autotag/mb.go @@ -61,6 +61,18 @@ type MBClient interface { query string, limit int, ) ([]MBReleaseGroupHit, int, error) + // SearchReleaseGroupsLocal searches the offline dump-derived + // catalog for release groups matching albumName — no network + // round-trip. ok is false when the local catalog isn't + // populated yet (or the implementation has no offline index), + // telling the caller to rely on the network cascade alone; ok + // true with zero hits means the catalog was consulted and + // genuinely has nothing. + SearchReleaseGroupsLocal( + ctx context.Context, + albumName string, + limit int, + ) (hits []MBReleaseGroupHit, ok bool) SearchRecordings( ctx context.Context, query string, @@ -128,12 +140,40 @@ func (r *MBResolver) ResolveMB(ctx context.Context, g Group) ([]Candidate, error } nArtist := Normalize(groupArtist(g)) - steps := buildMBQueryCascade(nAlbum, nArtist, len(g.Tracks), vaLikely(g)) seen := make(map[string]bool) var merged []Candidate + // Local-index pass: the offline dump-derived catalog covers + // essentially every popular release group, so try it before + // spending any rate-limited search calls. This never skips + // BrowseReleases (the catalog doesn't carry per-release + // tracklists) but it very often means the network Lucene + // cascade below never has to run at all. + if localHits, ok := r.client.SearchReleaseGroupsLocal(ctx, g.AlbumName, r.limit); ok { + added := r.fanOutBrowse(ctx, g, localHits, "index", seen, &merged) + + r.logger.Debug( + "local index search done", + "hits", len(localHits), "new_candidates", added, + ) + + if added > 0 { + ranked := RankCandidates(g, merged) + if len(ranked) > 0 && ranked[0].Score >= cascadeSufficient { + r.logger.Info( + "MB cascade stopped — sufficient local-index candidate", + "score", ranked[0].Score, + ) + + return merged, nil + } + } + } + + steps := buildMBQueryCascade(nAlbum, nArtist, len(g.Tracks), vaLikely(g)) + for _, step := range steps { hits, _, err := r.client.SearchReleaseGroups(ctx, step.query, r.limit) if err != nil { diff --git a/backend/autotag/mb_test.go b/backend/autotag/mb_test.go index 89e500a..0cea6b5 100644 --- a/backend/autotag/mb_test.go +++ b/backend/autotag/mb_test.go @@ -23,6 +23,8 @@ type fakeMBClient struct { lookupRGs map[string]MBReleaseGroupHit searchRecs []MBRecordingHit recRelsByMBID map[string][]MBReleaseRef + localHits []MBReleaseGroupHit + localOK bool } func (f *fakeMBClient) SearchReleaseGroups( @@ -36,6 +38,15 @@ func (f *fakeMBClient) SearchReleaseGroups( return hits, len(hits), nil } +// SearchReleaseGroupsLocal is a no-op by default (ok=false), so +// existing cascade tests exercise the network path unchanged. Set +// localHits / localOK on the fake to exercise the index-first path. +func (f *fakeMBClient) SearchReleaseGroupsLocal( + _ context.Context, _ string, _ int, +) ([]MBReleaseGroupHit, bool) { + return f.localHits, f.localOK +} + func (f *fakeMBClient) BrowseReleases( _ context.Context, mbid string, ) ([]MBRelease, error) { @@ -188,6 +199,89 @@ func TestMBResolver_CascadeStopsWhenSufficient(t *testing.T) { } } +func TestMBResolver_LocalIndexSufficientSkipsNetworkSearch(t *testing.T) { + t.Parallel() + + fake := &fakeMBClient{ + localOK: true, + localHits: []MBReleaseGroupHit{ + {MBID: "rg1", Title: "Abbey Road"}, + }, + browseByMBID: map[string][]MBRelease{ + "rg1": {{ + MBID: "rel1", Title: "Abbey Road", Status: "Official", + Tracks: []CandidateTrack{ + {Position: 1, Title: "Come Together", LengthMillis: 259000}, + }, + }}, + }, + } + + r := NewMBResolver(fake, slog.New(slog.DiscardHandler)) + + cands, err := r.ResolveMB(context.Background(), abbeyRoadGroup()) + if err != nil { + t.Fatalf("ResolveMB: %v", err) + } + + if len(cands) != 1 { + t.Fatalf("expected 1 candidate, got %d", len(cands)) + } + + if cands[0].Provenance != "index" { + t.Errorf("provenance = %q, want 'index'", cands[0].Provenance) + } + + if len(fake.queries) != 0 { + t.Errorf( + "expected zero network search queries when the local index sufficed, got %d: %v", + len(fake.queries), fake.queries, + ) + } +} + +func TestMBResolver_LocalIndexThinFallsThroughToNetwork(t *testing.T) { + t.Parallel() + + // Local index is "ready" but has nothing plausible for this + // album — the cascade must still fall through to the network + // steps exactly as if there were no local index at all. + fake := &fakeMBClient{ + localOK: true, + localHits: nil, + searchByStep: map[int][]MBReleaseGroupHit{ + 1: {{MBID: "rg1", Title: "Abbey Road"}}, + }, + browseByMBID: map[string][]MBRelease{ + "rg1": {{ + MBID: "rel1", Title: "Abbey Road", Status: "Official", + Tracks: []CandidateTrack{ + {Position: 1, Title: "Come Together", LengthMillis: 259000}, + }, + }}, + }, + } + + r := NewMBResolver(fake, slog.New(slog.DiscardHandler)) + + cands, err := r.ResolveMB(context.Background(), abbeyRoadGroup()) + if err != nil { + t.Fatalf("ResolveMB: %v", err) + } + + if len(cands) != 1 { + t.Fatalf("expected 1 candidate, got %d", len(cands)) + } + + if cands[0].Provenance != "no-track-count" { + t.Errorf("provenance = %q, want 'no-track-count'", cands[0].Provenance) + } + + if len(fake.queries) != 2 { //nolint:mnd + t.Errorf("expected the usual 2 network queries, got %d", len(fake.queries)) + } +} + func TestMBResolver_CascadeContinuesPastMediocreHits(t *testing.T) { t.Parallel() diff --git a/backend/autotag/mixedbag.go b/backend/autotag/mixedbag.go new file mode 100644 index 0000000..8872cb6 --- /dev/null +++ b/backend/autotag/mixedbag.go @@ -0,0 +1,215 @@ +package autotag + +// mixedBagMinTracks is the smallest folder IsMixedBag will flag. +// Below this, artist/album divergence is just as likely to be +// sampling noise (a 2-track folder with two different artists could +// easily be a legitimate 2-track EP with a featured artist) as it is +// a genuine junk-drawer folder. +const mixedBagMinTracks = 4 + +// clusterMinSize is the smallest tag-matched group ClusterByAlbumArtist +// will surface as a splittable cluster. A single track sharing no +// album/artist with anything else in the folder gains nothing from +// becoming its own one-track group — it stays in the leftover folder, +// which the existing evidence-scaling (rank.go) already treats +// appropriately harshly for a 1-track match. +const clusterMinSize = 2 + +// IsMixedBag reports whether a group's local tracks look like an +// unrelated pile of songs rather than one release: no artist +// consensus AND no album consensus, across enough tracks that the +// divergence isn't just noise. An explicit, non-VA album-artist tag +// on the folder overrides the heuristic — a user (or a prior tagger) +// who set a real album-artist meant this to read as one release. +func IsMixedBag(g Group) bool { + if len(g.Tracks) < mixedBagMinTracks { + return false + } + + if g.AlbumArtist != "" && !isVAName(g.AlbumArtist) { + return false + } + + return !hasTagConsensus(trackArtistTags(g.Tracks)) && + !hasTagConsensus(trackAlbumTags(g.Tracks)) +} + +// hasTagConsensus reports whether every non-empty value in vals +// normalizes to the same string. Empty values are ignored — missing +// tags are unknown, not disagreement. A folder with zero non-empty +// values has no consensus either way; callers only reach here after +// already requiring enough tracks to matter. +func hasTagConsensus(vals []string) bool { + distinct := make(map[string]bool, 2) //nolint:mnd + + for _, v := range vals { + if v == "" { + continue + } + + distinct[Normalize(v)] = true + + if len(distinct) > 1 { + return false + } + } + + return len(distinct) == 1 +} + +func trackArtistTags(tracks []LocalTrack) []string { + out := make([]string, len(tracks)) + for i, t := range tracks { + out[i] = t.Artist + } + + return out +} + +func trackAlbumTags(tracks []LocalTrack) []string { + out := make([]string, len(tracks)) + for i, t := range tracks { + out[i] = t.AlbumTag + } + + return out +} + +// TrackCluster is a set of local tracks sharing a non-empty (album, +// album-artist) tag pair — a candidate sub-album hiding inside a +// mixed-bag folder. +type TrackCluster struct { + AlbumName string + AlbumArtist string + Tracks []LocalTrack +} + +// ClusterByAlbumArtist groups tracks by normalized (album tag, +// album-artist tag) and returns the clusters with at least +// clusterMinSize members, in first-seen order (the caller typically +// passes tracks already ordered by disc/track/path, so this stays +// deterministic run to run). Tracks with no album tag, or whose +// cluster never reaches clusterMinSize, are omitted — they belong in +// the leftover folder, not a synthetic group of their own. +func ClusterByAlbumArtist(tracks []LocalTrack) []TrackCluster { + type key struct{ album, artist string } + + index := make(map[key]int, 4) //nolint:mnd + + var clusters []TrackCluster + + for _, t := range tracks { + album := Normalize(t.AlbumTag) + if album == "" { + continue + } + + k := key{album: album, artist: Normalize(t.AlbumArtistTag)} + + if i, ok := index[k]; ok { + clusters[i].Tracks = append(clusters[i].Tracks, t) + + continue + } + + index[k] = len(clusters) + clusters = append(clusters, TrackCluster{ + AlbumName: t.AlbumTag, + AlbumArtist: t.AlbumArtistTag, + Tracks: []LocalTrack{t}, + }) + } + + out := clusters[:0] + + for _, c := range clusters { + if len(c.Tracks) >= clusterMinSize { + out = append(out, c) + } + } + + return out +} + +// SplitPlan returns the full set of synthetic groups a mixed-bag +// folder should be torn into: ClusterByAlbumArtist's tag-matched +// sub-albums, plus a one-track cluster for every track that didn't +// share an (album, album-artist) pair with anything else in the +// folder. Unlike ClusterByAlbumArtist alone — which leaves +// unclustered tracks behind in the parent group, where they'd still +// get folded into whatever partial-album match the scorer finds for +// the rest of the pile — this guarantees every track leaves the +// parent, so a folder of entirely unrelated singles (no two tracks +// share an album tag) still gets torn apart instead of being scored +// as one bogus album with a pile of "extra" tracks. Each singleton's +// evidence-scaled score (rank.go) keeps it appropriately humble on +// its own — it just no longer drags an unrelated release's score +// down, or gets dragged down by one. +func SplitPlan(tracks []LocalTrack) []TrackCluster { + type key struct{ album, artist string } + + index := make(map[key]int, 4) //nolint:mnd + + var clusters []TrackCluster + + // memberOf[i] is 1+the cluster index track i was assigned to (by + // album/artist tag match), or 0 if it never matched anything. + // Tracked by slice position rather than any LocalTrack field — + // AudioFileID/FilePath are frequently zero-valued in this + // package's own tests and would collide, wrongly treating + // distinct untagged tracks as duplicates of one another. + memberOf := make([]int, len(tracks)) + + for i, t := range tracks { + album := Normalize(t.AlbumTag) + if album == "" { + continue + } + + k := key{album: album, artist: Normalize(t.AlbumArtistTag)} + + if ci, ok := index[k]; ok { + clusters[ci].Tracks = append(clusters[ci].Tracks, t) + memberOf[i] = ci + 1 + + continue + } + + index[k] = len(clusters) + memberOf[i] = len(clusters) + 1 + clusters = append(clusters, TrackCluster{ + AlbumName: t.AlbumTag, + AlbumArtist: t.AlbumArtistTag, + Tracks: []LocalTrack{t}, + }) + } + + // Clusters that never reached clusterMinSize don't survive as a + // group; their sole member falls through to the singleton pass + // below instead. + kept := make([]TrackCluster, 0, len(clusters)) + keptIndex := make(map[int]int, len(clusters)) + + for oldIdx, c := range clusters { + if len(c.Tracks) >= clusterMinSize { + keptIndex[oldIdx] = len(kept) + kept = append(kept, c) + } + } + + for i, t := range tracks { + if ci := memberOf[i] - 1; ci >= 0 { + if _, ok := keptIndex[ci]; ok { + continue + } + } + + kept = append(kept, TrackCluster{ + AlbumName: t.AlbumTag, + AlbumArtist: t.AlbumArtistTag, + Tracks: []LocalTrack{t}, + }) + } + + return kept +} diff --git a/backend/autotag/mixedbag_test.go b/backend/autotag/mixedbag_test.go new file mode 100644 index 0000000..c53be2b --- /dev/null +++ b/backend/autotag/mixedbag_test.go @@ -0,0 +1,246 @@ +package autotag + +import "testing" + +func junkDrawerTracks() []LocalTrack { + return []LocalTrack{ + { + Title: "Song A", Artist: "Artist One", + AlbumTag: "Album One", AlbumArtistTag: "Artist One", + }, + { + Title: "Song B", Artist: "Artist One", + AlbumTag: "Album One", AlbumArtistTag: "Artist One", + }, + { + Title: "Song C", Artist: "Artist Two", + AlbumTag: "Album Two", AlbumArtistTag: "Artist Two", + }, + { + Title: "Song D", Artist: "Artist Two", + AlbumTag: "Album Two", AlbumArtistTag: "Artist Two", + }, + { + Title: "Song E", Artist: "Artist Three", + AlbumTag: "Album Three", AlbumArtistTag: "Artist Three", + }, + } +} + +func TestIsMixedBag_DetectsJunkDrawer(t *testing.T) { + t.Parallel() + + g := Group{Tracks: junkDrawerTracks()} + + if !IsMixedBag(g) { + t.Fatal("expected a folder with no artist or album consensus to be flagged mixed-bag") + } +} + +func TestIsMixedBag_RealAlbumNotFlagged(t *testing.T) { + t.Parallel() + + g := Group{ + AlbumArtist: "The Beatles", + Tracks: []LocalTrack{ + {Title: "Come Together", Artist: "The Beatles"}, + {Title: "Something", Artist: "The Beatles"}, + {Title: "Maxwell's Silver Hammer", Artist: "The Beatles"}, + {Title: "Oh! Darling", Artist: "The Beatles"}, + }, + } + + if IsMixedBag(g) { + t.Fatal("a coherent single-artist album must not be flagged mixed-bag") + } +} + +func TestIsMixedBag_ExplicitAlbumArtistOverridesHeuristic(t *testing.T) { + t.Parallel() + + // Per-track artists disagree (feat. credits, remixers, etc.) but + // the folder carries a real album-artist tag — trust it. + g := Group{ + AlbumArtist: "Some Artist", + Tracks: []LocalTrack{ + {Title: "Track 1", Artist: "Some Artist"}, + {Title: "Track 2", Artist: "Some Artist feat. Guest"}, + {Title: "Track 3", Artist: "Someone Else"}, + {Title: "Track 4", Artist: "Some Artist"}, + }, + } + + if IsMixedBag(g) { + t.Fatal("explicit non-VA album-artist tag should override the divergence heuristic") + } +} + +func TestIsMixedBag_VACompilationNotFlagged(t *testing.T) { + t.Parallel() + + // Various-artists compilation: artists diverge but every track + // agrees on the album — this is vaLikely's case, not a junk + // drawer, so IsMixedBag must require album divergence too. + g := Group{ + Tracks: []LocalTrack{ + {Title: "Track 1", Artist: "Artist One", AlbumTag: "Now That's What I Call Music"}, + {Title: "Track 2", Artist: "Artist Two", AlbumTag: "Now That's What I Call Music"}, + {Title: "Track 3", Artist: "Artist Three", AlbumTag: "Now That's What I Call Music"}, + {Title: "Track 4", Artist: "Artist Four", AlbumTag: "Now That's What I Call Music"}, + }, + } + + if IsMixedBag(g) { + t.Fatal("a VA compilation with consistent album tags must not be flagged mixed-bag") + } +} + +func TestIsMixedBag_TooFewTracksNotFlagged(t *testing.T) { + t.Parallel() + + g := Group{ + Tracks: []LocalTrack{ + {Title: "Track 1", Artist: "Artist One", AlbumTag: "Album One"}, + {Title: "Track 2", Artist: "Artist Two", AlbumTag: "Album Two"}, + }, + } + + if IsMixedBag(g) { + t.Fatal("a folder below mixedBagMinTracks must not be flagged, even if it diverges") + } +} + +func TestClusterByAlbumArtist_FindsSubAlbums(t *testing.T) { + t.Parallel() + + tracks := junkDrawerTracks() // two 2-track clusters + one true singleton + + clusters := ClusterByAlbumArtist(tracks) + + if len(clusters) != 2 { //nolint:mnd + t.Fatalf("expected 2 clusters (Album One, Album Two), got %d: %+v", len(clusters), clusters) + } + + for _, c := range clusters { + if len(c.Tracks) != 2 { //nolint:mnd + t.Errorf("cluster %q: expected 2 tracks, got %d", c.AlbumName, len(c.Tracks)) + } + } + + total := 0 + for _, c := range clusters { + total += len(c.Tracks) + } + + if total != 4 { //nolint:mnd + t.Errorf( + "expected 4 clustered tracks total (Song E stays unclustered), got %d", + total, + ) + } +} + +func TestClusterByAlbumArtist_NoAlbumTagStaysUnclustered(t *testing.T) { + t.Parallel() + + tracks := []LocalTrack{ + {Title: "Track 1", Artist: "Artist One"}, + {Title: "Track 2", Artist: "Artist One"}, + } + + if clusters := ClusterByAlbumArtist(tracks); len(clusters) != 0 { + t.Fatalf("tracks with no album tag must never cluster, got %+v", clusters) + } +} + +func TestClusterByAlbumArtist_DeterministicOrder(t *testing.T) { + t.Parallel() + + tracks := junkDrawerTracks() + + first := ClusterByAlbumArtist(tracks) + second := ClusterByAlbumArtist(tracks) + + if len(first) != len(second) { + t.Fatalf("non-deterministic cluster count: %d vs %d", len(first), len(second)) + } + + for i := range first { + if first[i].AlbumName != second[i].AlbumName { + t.Errorf( + "non-deterministic cluster order at %d: %q vs %q", + i, + first[i].AlbumName, + second[i].AlbumName, + ) + } + } + + if first[0].AlbumName != "Album One" { + t.Errorf("expected first-seen cluster order, got %q first", first[0].AlbumName) + } +} + +func TestSplitPlan_ClustersPlusSingletonForEveryLeftover(t *testing.T) { + t.Parallel() + + tracks := junkDrawerTracks() // two 2-track clusters + one true singleton (Song E) + + plan := SplitPlan(tracks) + + total := 0 + for _, c := range plan { + total += len(c.Tracks) + } + + if total != len(tracks) { + t.Fatalf("expected every track accounted for, got %d of %d", total, len(tracks)) + } + + var singletons, clustered int + + for _, c := range plan { + switch len(c.Tracks) { + case 1: + singletons++ + case 2: //nolint:mnd + clustered++ + default: + t.Errorf("unexpected cluster size %d: %+v", len(c.Tracks), c) + } + } + + if singletons != 1 { + t.Errorf("expected exactly 1 singleton (Song E), got %d", singletons) + } + + if clustered != 2 { //nolint:mnd + t.Errorf("expected exactly 2 clustered groups, got %d", clustered) + } +} + +func TestSplitPlan_AllUnrelatedTracksAllBecomeSingletons(t *testing.T) { + t.Parallel() + + tracks := []LocalTrack{ + {Title: "Track 1", Artist: "Artist One", AlbumTag: "Album A"}, + {Title: "Track 2", Artist: "Artist Two", AlbumTag: "Album B"}, + {Title: "Track 3", Artist: "Artist Three"}, // no album tag at all + } + + plan := SplitPlan(tracks) + + if len(plan) != len(tracks) { + t.Fatalf( + "expected every unrelated track to become its own singleton, got %d clusters for %d tracks", + len(plan), + len(tracks), + ) + } + + for _, c := range plan { + if len(c.Tracks) != 1 { + t.Errorf("expected singleton cluster, got %d tracks: %+v", len(c.Tracks), c) + } + } +} diff --git a/backend/autotag/rank.go b/backend/autotag/rank.go index ad3c358..1842e64 100644 --- a/backend/autotag/rank.go +++ b/backend/autotag/rank.go @@ -33,6 +33,19 @@ const ( // auto-accept entirely. evidenceFloor = 0.85 evidenceFullTracks = 3 + + // Synthetic groups (SplitMixedFolder's tag-clustered sub-albums) + // are, by construction, a SUBSET of a bigger folder: the folder + // might not have every track from the release the cluster + // belongs to. A candidate with more tracks than the synthetic + // group is therefore expected, not a sign of a wrong match, so + // its trackCountMatch penalty is softened relative to a real + // folder (where a track-count gap usually does mean the wrong + // release). A candidate with FEWER tracks than the group is + // still scored by the normal (harsher) formula — that's a real + // mismatch regardless of source. + syntheticMissingPenaltyScale = 0.35 + syntheticTrackCountFloor = 0.55 ) // vaNames are artist strings that signal "various artists" — used @@ -139,7 +152,7 @@ func ScoreCandidate(g Group, c Candidate) Candidate { trackAgg := ((titleAvg*weightTitle + lengthAvg*weightLength) / trackWeightSum) * coverage - trackCountScore := trackCountMatch(len(targets), len(local)) + trackCountScore := trackCountMatch(len(targets), len(local), g.Synthetic) // Artist fit: compare the folder's artist against the // candidate's release artist-credit. This is a SOFT signal, not @@ -347,8 +360,15 @@ func evidenceFactor(localTrackCount int) float64 { } // trackCountMatch returns 1.0 when equal, 0.0 when off by >= 50%, -// linear between. -func trackCountMatch(a, b int) float64 { +// linear between. When synthetic is true and the candidate (a) has +// MORE tracks than the local group (b) — the group having fewer +// tracks than the full release, exactly what's expected from a +// tag-clustered subset of a folder — the penalty is softened instead +// of using the normal harsh formula. Fewer candidate tracks than +// local (b > a) always uses the normal formula: that pattern means +// the group has tracks the candidate release doesn't, which is a +// real mismatch however the group was built. +func trackCountMatch(a, b int, synthetic bool) float64 { if a == 0 && b == 0 { return 1.0 } @@ -357,6 +377,13 @@ func trackCountMatch(a, b int) float64 { return 0.0 } + if synthetic && a > b { + diff := a - b + frac := float64(diff) / float64(a) + + return max(1.0-frac*syntheticMissingPenaltyScale, syntheticTrackCountFloor) + } + diff := a - b if diff < 0 { diff = -diff diff --git a/backend/autotag/rank_test.go b/backend/autotag/rank_test.go index 53f59b2..affdd7a 100644 --- a/backend/autotag/rank_test.go +++ b/backend/autotag/rank_test.go @@ -49,6 +49,55 @@ func TestRankCandidates_PrefersExactTrackCountMatch(t *testing.T) { } } +func TestScoreCandidate_SyntheticGroupSoftensMissingTrackPenalty(t *testing.T) { + t.Parallel() + + // Two tracks pulled from a mixed-bag folder, tag-clustered as a + // subset of a 5-track release — exactly what SplitMixedFolder + // produces. A candidate release with the other 3 tracks the + // folder simply never had must not be penalized nearly as hard + // as a real folder missing 3 of 5 tracks would be. + local := []autotag.LocalTrack{ + {Title: "A", TrackNumber: 1, LengthMillis: 200000}, + {Title: "B", TrackNumber: 2, LengthMillis: 200000}, + } + + candidate := autotag.Candidate{ + ReleaseMBID: "full-release", + Title: "Album", + Status: "Official", + Tracks: []autotag.CandidateTrack{ + {Position: 1, Title: "A", LengthMillis: 200000}, + {Position: 2, Title: "B", LengthMillis: 200000}, + {Position: 3, Title: "C", LengthMillis: 200000}, + {Position: 4, Title: "D", LengthMillis: 200000}, + {Position: 5, Title: "E", LengthMillis: 200000}, + }, + } + + fromRealFolder := autotag.ScoreCandidate( + autotag.Group{Tracks: local, Synthetic: false}, candidate, + ) + fromSynthetic := autotag.ScoreCandidate( + autotag.Group{Tracks: local, Synthetic: true}, candidate, + ) + + if fromSynthetic.Breakdown.TrackCountFit <= fromRealFolder.Breakdown.TrackCountFit { + t.Errorf( + "synthetic track-count fit (%.3f) should exceed the real-folder fit (%.3f) for the same gap", + fromSynthetic.Breakdown.TrackCountFit, + fromRealFolder.Breakdown.TrackCountFit, + ) + } + + if fromSynthetic.Score <= fromRealFolder.Score { + t.Errorf( + "synthetic group score (%.3f) should exceed the real-folder score (%.3f)", + fromSynthetic.Score, fromRealFolder.Score, + ) + } +} + func TestRankCandidates_PrefersOfficial(t *testing.T) { t.Parallel() diff --git a/backend/autotag/recommend.go b/backend/autotag/recommend.go index ab68afa..98201b1 100644 --- a/backend/autotag/recommend.go +++ b/backend/autotag/recommend.go @@ -59,9 +59,16 @@ func Recommend(g Group, candidates []Candidate) Recommendation { // Cap: missing or unmatched tracks mean the alignment itself is // incomplete, however good the matched tracks look (beets caps - // these penalties at "medium" the same way). + // these penalties at "medium" the same way). A synthetic + // (tag-clustered) group is, by construction, a subset of a + // bigger folder, so AlignmentMissing (the candidate has tracks + // the group doesn't) is the expected shape rather than a defect + // and doesn't cap the recommendation. AlignmentUnmatched (the + // group has a track the candidate doesn't) is still a real + // discrepancy regardless of source. for _, a := range top.Alignments { - if a.Status == AlignmentMissing || a.Status == AlignmentUnmatched { + if a.Status == AlignmentUnmatched || + (a.Status == AlignmentMissing && !g.Synthetic) { rec = minRecommendation(rec, RecommendationMedium) break diff --git a/backend/autotag/recommend_test.go b/backend/autotag/recommend_test.go index 8c38b6e..73f2e94 100644 --- a/backend/autotag/recommend_test.go +++ b/backend/autotag/recommend_test.go @@ -97,6 +97,46 @@ func TestRecommend_AlignmentDefectsCapAtMedium(t *testing.T) { } } +func TestRecommend_SyntheticGroupMissingTracksDoNotCap(t *testing.T) { + t.Parallel() + + top := mkScoredCandidate("rg1", 0.95) + top.Alignments = []TrackAlignment{ + {Status: AlignmentMatched}, + {Status: AlignmentMissing, LocalIndex: -1}, + } + + g := fullGroup() + g.Synthetic = true + + if got := Recommend(g, []Candidate{top}); got != RecommendationStrong { + t.Errorf( + "synthetic group with only missing (not unmatched) tracks: Recommend = %q, want strong", + got, + ) + } +} + +func TestRecommend_SyntheticGroupUnmatchedTracksStillCap(t *testing.T) { + t.Parallel() + + top := mkScoredCandidate("rg1", 0.95) + top.Alignments = []TrackAlignment{ + {Status: AlignmentMatched}, + {Status: AlignmentUnmatched, LocalIndex: 1}, + } + + g := fullGroup() + g.Synthetic = true + + if got := Recommend(g, []Candidate{top}); got != RecommendationMedium { + t.Errorf( + "synthetic group with an unmatched local track: Recommend = %q, want medium", + got, + ) + } +} + func TestRecommend_ThinEvidenceCapsAtMedium(t *testing.T) { t.Parallel() diff --git a/backend/autotag/scorer.go b/backend/autotag/scorer.go index 56c8a1c..04fc46c 100644 --- a/backend/autotag/scorer.go +++ b/backend/autotag/scorer.go @@ -104,6 +104,7 @@ func (s *Scorer) scoreGroup( AlbumName: item.AlbumName, AlbumArtist: item.AlbumArtist, Tracks: locals, + Synthetic: item.Synthetic != 0, } localHits, err := s.local.ResolveLocal(ctx, item.AlbumName) @@ -142,6 +143,7 @@ func (s *Scorer) scoreGroup( LocalTracks: locals, Candidates: candidates, Recommendation: Recommend(g, candidates), + Synthetic: g.Synthetic, }, nil } diff --git a/backend/autotag/scorer_test.go b/backend/autotag/scorer_test.go index 58307f9..1fb9303 100644 --- a/backend/autotag/scorer_test.go +++ b/backend/autotag/scorer_test.go @@ -338,6 +338,12 @@ func (c *idFakeClient) LookupReleaseGroup( return autotag.MBReleaseGroupHit{}, nil } +func (c *idFakeClient) SearchReleaseGroupsLocal( + _ context.Context, _ string, _ int, +) ([]autotag.MBReleaseGroupHit, bool) { + return nil, false +} + func TestScorer_PersistScoreWritesTopMatch(t *testing.T) { t.Parallel() @@ -462,3 +468,11 @@ func (c *countingMBClient) LookupReleaseGroup( return autotag.MBReleaseGroupHit{}, nil } + +// SearchReleaseGroupsLocal is not a network call — it never counts +// against the zero-network-call assertions this fake exists for. +func (c *countingMBClient) SearchReleaseGroupsLocal( + _ context.Context, _ string, _ int, +) ([]autotag.MBReleaseGroupHit, bool) { + return nil, false +} diff --git a/backend/autotag/types.go b/backend/autotag/types.go index fff8aec..f2fec05 100644 --- a/backend/autotag/types.go +++ b/backend/autotag/types.go @@ -12,6 +12,15 @@ type LocalTrack struct { DiscNumber int LengthMillis int64 RecordingMBID string + + // AlbumTag/AlbumArtistTag are this track's OWN album tags (via + // its release_group link), independent of the folder-level + // Group.AlbumName/AlbumArtist below. A coherent album's tracks + // all carry the same values here; a junk-drawer folder's don't. + // Used only by SplitMixedFolder's clustering — the scorer itself + // still ranks against Group.AlbumName/AlbumArtist. + AlbumTag string + AlbumArtistTag string } // Group is the folder-level context candidates are ranked against: @@ -21,6 +30,14 @@ type Group struct { AlbumName string AlbumArtist string Tracks []LocalTrack + + // Synthetic marks a group carved out of a mixed-bag folder by + // SplitMixedFolder rather than corresponding to a real directory. + // Its tracks are a tag-matched subset of a bigger folder, so a + // candidate with MORE tracks than the group is expected, not a + // sign of a bad match — see the synthetic-aware evidence/track- + // count handling in rank.go and recommend.go. + Synthetic bool } // CandidateSource distinguishes candidates served from the local @@ -125,4 +142,5 @@ type GroupScore struct { LocalTracks []LocalTrack Candidates []Candidate // sorted by Score, descending Recommendation Recommendation + Synthetic bool // true for a SplitMixedFolder-derived group } diff --git a/backend/autotagservice/service.go b/backend/autotagservice/service.go index b68eb32..8d6a851 100644 --- a/backend/autotagservice/service.go +++ b/backend/autotagservice/service.go @@ -177,7 +177,7 @@ func NewService( exp *explore.Service, tw *tagwriter.TagWriter, ) *Service { - mbAdapter := explore.NewAutotagClient(exp.MusicBrainz()) + mbAdapter := explore.NewAutotagClient(exp) scorer := autotag.NewScorer(db.Queries, mbAdapter, logger.WithGroup("autotag")) mbr := autotag.NewMBResolver(mbAdapter, logger.WithGroup("autotag-mb")) @@ -307,6 +307,12 @@ func (s *Service) startPrefetch(libraryID int64) { s.mu.Unlock() }() + // Self-heal before enumerating: don't burn a scoring pass on rows + // whose bookkeeping never ran or drifted (see ListPendingFolders). + if err := s.db.Queries.PruneOrphanedTaggingItems(ctx); err != nil { + s.logger.Warn("prefetch: prune orphaned items failed", "err", err) + } + // Find all pending items missing a score. Ordered alphabetically // for stable progress reporting; libraryID=0 fans out to all. const maxPrefetch = 5000 @@ -376,25 +382,30 @@ func (s *Service) startPrefetch(libraryID int64) { continue } + // A folder that looks like a pile of unrelated tracks gets + // torn apart before scoring — otherwise the scorer treats + // the whole pile as one album candidate and every track that + // doesn't fit the best partial match gets counted as an + // "extra" of it, rather than being matched on its own. The + // original group key is gone once every track has moved to a + // synthetic child, so score those instead of key. + if newKeys := s.autoSplitMixedBag(ctx, key); len(newKeys) > 0 { + for _, nk := range newKeys { + s.scoreAndPersist(ctx, nk, "prefetch: score synthetic group") + } + + s.emitEvent(events.AutotagPrefetchProgress, map[string]any{ + "processed": i + 1, + "total": total, + }) + + continue + } + // Local-first: the background sweep skips the MusicBrainz // cascade when a local candidate already scores well, so a // library with cross-library duplicates costs no network here. - score, err := s.scorer.ScoreGroupLocalFirst(ctx, key) - if err != nil { - s.logger.Debug( - "prefetch: score failed — skipping", - "group_key", key, "err", err, - ) - } else { - s.cacheCandidates(key, score.Candidates) - - if perr := s.scorer.PersistScore(ctx, score); perr != nil { - s.logger.Debug( - "prefetch: persist failed", - "group_key", key, "err", perr, - ) - } - } + s.scoreAndPersist(ctx, key, "prefetch: score failed — skipping") s.emitEvent(events.AutotagPrefetchProgress, map[string]any{ "processed": i + 1, @@ -409,6 +420,74 @@ func (s *Service) startPrefetch(libraryID int64) { s.logger.Info("autotag prefetch: done", "groups", total) } +// scoreAndPersist runs the cheap local-first score for one group and +// caches + persists the result, logging (never failing the caller) +// on error. failMsg labels the debug log line when scoring itself +// errors. +func (s *Service) scoreAndPersist(ctx context.Context, groupKey, failMsg string) { + score, err := s.scorer.ScoreGroupLocalFirst(ctx, groupKey) + if err != nil { + s.logger.Debug(failMsg, "group_key", groupKey, "err", err) + + return + } + + s.cacheCandidates(groupKey, score.Candidates) + + if perr := s.scorer.PersistScore(ctx, score); perr != nil { + s.logger.Debug( + "prefetch: persist failed", + "group_key", groupKey, "err", perr, + ) + } +} + +// autoSplitMixedBag detects a folder that looks like a pile of +// unrelated tracks (autotag.IsMixedBag) and, if so, tears it apart +// via the same clustering SplitMixedFolder uses (autotag.SplitPlan) +// before the background sweep scores it — otherwise the scorer +// treats the whole pile as one album candidate and every track that +// doesn't fit the best partial match gets counted as an "extra" of +// it. Returns the new synthetic group keys, or nil when the folder +// isn't a mixed bag (or had nothing to split). +func (s *Service) autoSplitMixedBag(ctx context.Context, groupKey string) []string { + item, err := s.db.Queries.GetTaggingItem(ctx, groupKey) + if err != nil { + return nil + } + + locals, err := s.scorer.LocalTracksForGroup(ctx, groupKey) + if err != nil { + s.logger.Debug("prefetch: auto-split load locals failed", "group_key", groupKey, "err", err) + + return nil + } + + g := autotag.Group{AlbumName: item.AlbumName, AlbumArtist: item.AlbumArtist, Tracks: locals} + if !autotag.IsMixedBag(g) { + return nil + } + + clusters := autotag.SplitPlan(locals) + if len(clusters) <= 1 { + return nil + } + + newKeys, err := s.splitIntoSyntheticGroups(groupKey, item.LibraryID, clusters) + if err != nil { + s.logger.Warn("prefetch: auto-split failed", "group_key", groupKey, "err", err) + + return nil + } + + s.logger.Info( + "autotag prefetch: auto-split mixed-bag folder", + "group_key", groupKey, "into", len(newKeys), + ) + + return newKeys +} + // PendingItem is a projection of tagging_items that's safe to hand // to the frontend. Score is dereferenced to 0 when NULL so TS sees // a plain number. @@ -430,6 +509,19 @@ type PendingItem struct { BestMatchReleaseMbid string `json:"bestMatchReleaseMbid"` Score float64 `json:"score"` Status string `json:"status"` + // Synthetic marks a group SplitMixedFolder carved out of a + // bigger folder by matching tags rather than a directory — the + // review UI labels these distinctly since several may share the + // same FolderSubPath. + Synthetic bool `json:"synthetic"` + // LikelyMixedBag is a cheap SQL-side approximation of autotag. + // IsMixedBag, computed for the whole library in one pass by + // ListPendingFolders (see ListLikelyMixedBagGroupKeys) rather + // than hydrating every group's tracks in Go. It's a badge hint, + // not a guarantee — ScoreView.MixedBag (computed from the real + // track list when a folder is opened) is the authoritative check + // that gates the SplitMixedFolder action itself. + LikelyMixedBag bool `json:"likelyMixedBag"` } // GetNextPending returns the next pending tagging item after the @@ -489,6 +581,15 @@ func (s *Service) GetNextPending() (*PendingItem, error) { func (s *Service) ListPendingFolders(libraryID int64) ([]PendingItem, error) { const maxFolders = 5000 + // Self-heal before listing: a row whose bookkeeping (scan orphan + // cleanup, maybeRebindTaggingGroup, SplitMixedFolder) never ran + // or drifted otherwise lingers here indefinitely, showing as an + // "old/nonexistent" entry with no folder path. Best-effort — a + // failed prune shouldn't block the list itself. + if err := s.db.Queries.PruneOrphanedTaggingItems(s.ctx); err != nil { + s.logger.Warn("list pending folders: prune orphaned items failed", "err", err) + } + rows, err := s.db.Queries.ListPendingTaggingItemsByScore( s.ctx, sqlcgen.ListPendingTaggingItemsByScoreParams{ @@ -502,19 +603,32 @@ func (s *Service) ListPendingFolders(libraryID int64) ([]PendingItem, error) { return nil, fmt.Errorf("list pending folders: %w", err) } + mixedBagKeys, err := s.db.Queries.ListLikelyMixedBagGroupKeys(s.ctx) + if err != nil { + // A cheap badge hint isn't worth failing the whole list for. + s.logger.Warn("list pending folders: mixed-bag triage failed", "err", err) + } + + mixedBag := make(map[string]bool, len(mixedBagKeys)) + for _, k := range mixedBagKeys { + mixedBag[k] = true + } + out := make([]PendingItem, 0, len(rows)) for _, row := range rows { item := PendingItem{ - GroupKey: row.GroupKey, - LibraryID: row.LibraryID, - LibraryName: row.LibraryName, - FolderSubPath: folderSubPath(row.LibraryPath, row.SampleFilePath), - TrackCount: row.TrackCount, - AlbumName: row.AlbumName, - AlbumArtist: row.AlbumArtist, - DiscNumber: row.DiscNumber, - Status: row.Status, + GroupKey: row.GroupKey, + LibraryID: row.LibraryID, + LibraryName: row.LibraryName, + FolderSubPath: folderSubPath(row.LibraryPath, row.SampleFilePath), + TrackCount: row.TrackCount, + AlbumName: row.AlbumName, + AlbumArtist: row.AlbumArtist, + DiscNumber: row.DiscNumber, + Status: row.Status, + Synthetic: row.Synthetic != 0, + LikelyMixedBag: mixedBag[row.GroupKey], } if row.BestMatchReleaseMbid.Valid { @@ -555,6 +669,7 @@ func (s *Service) GetPendingFolder(groupKey string) (*PendingItem, error) { AlbumArtist: row.AlbumArtist, DiscNumber: row.DiscNumber, Status: row.Status, + Synthetic: row.Synthetic != 0, } if row.BestMatchReleaseMbid.Valid { @@ -742,6 +857,15 @@ type ScoreView struct { // raw score it accounts for ambiguity (a rival release group // scoring nearly as high) and alignment defects. Recommendation string `json:"recommendation"` + // MixedBag is true when this group's tracks look like an + // unrelated pile rather than one release (autotag.IsMixedBag) — + // the review UI offers SplitMixedFolder when set. Always false + // for a group that's already Synthetic; a split group doesn't + // get split again. + MixedBag bool `json:"mixedBag"` + // Synthetic mirrors PendingItem.Synthetic for the currently + // open group. + Synthetic bool `json:"synthetic"` } // LocalTrackView mirrors autotag.LocalTrack. @@ -1151,6 +1275,156 @@ func (s *Service) RetagGroup(groupKey string) error { ) } +// errNothingToSplit is returned by SplitMixedFolder when the +// folder's tracks carry no repeated (album, album-artist) tag pair +// to cluster on — nothing to split out. +var errNothingToSplit = errors.New("autotag: no tag-matched sub-albums to split out") + +// SplitMixedFolder is the "this folder is a pile of unrelated +// tracks" escape hatch: it partitions the group's local tracks via +// autotag.SplitPlan — tag-matched sub-albums (see +// autotag.ClusterByAlbumArtist) plus a one-track cluster for every +// track that didn't share an (album, album-artist) pair with +// anything else — and carves each piece out into its own synthetic +// tagging group, reassigning just those audio_files rows (no files +// move on disk). Every track leaves the original group; nothing is +// left behind to be scored as "extra tracks" of whichever piece +// happens to match first. The synthetic groups are scored with +// relaxed missing-track handling (rank.go, recommend.go), since +// they're expected to be an incomplete subset of whatever release +// they belong to. +// +// Returns the resulting PendingItems — the leftover original group +// first (if anything remains in it), then the new synthetic groups +// — so the frontend can splice them into the sidebar without a full +// reload. Errors with errNothingToSplit when the folder is already +// one coherent unit (SplitPlan produces a single cluster covering +// every track); callers should treat that as "nothing to show", not +// a failure. +func (s *Service) SplitMixedFolder(groupKey string) ([]PendingItem, error) { + item, err := s.db.Queries.GetTaggingItem(s.ctx, groupKey) + if err != nil { + return nil, fmt.Errorf("get tagging item: %w", err) + } + + locals, err := s.scorer.LocalTracksForGroup(s.ctx, groupKey) + if err != nil { + return nil, fmt.Errorf("load locals: %w", err) + } + + clusters := autotag.SplitPlan(locals) + if len(clusters) <= 1 { + return nil, errNothingToSplit + } + + newKeys, err := s.splitIntoSyntheticGroups(groupKey, item.LibraryID, clusters) + if err != nil { + return nil, err + } + + out := make([]PendingItem, 0, len(newKeys)+1) + + if leftover, err := s.GetPendingFolder(groupKey); err != nil { + s.logger.Warn("split: reload leftover parent", "group_key", groupKey, "err", err) + } else if leftover != nil { + out = append(out, *leftover) + } + + for _, k := range newKeys { + child, err := s.GetPendingFolder(k) + if err != nil || child == nil { + s.logger.Warn("split: reload synthetic group", "group_key", k, "err", err) + + continue + } + + out = append(out, *child) + } + + return out, nil +} + +// splitIntoSyntheticGroups performs the actual DB migration inside a +// single transaction: each cluster's tracks are reassigned onto a +// deterministic synthetic group key, the parent's track count is +// decremented per track moved, and the parent row is dropped if it +// ends up empty. Returns the new group keys in cluster order. +func (s *Service) splitIntoSyntheticGroups( + parentKey string, libraryID int64, clusters []autotag.TrackCluster, +) ([]string, error) { + tx, err := s.db.BeginTx() + if err != nil { + return nil, fmt.Errorf("begin split tx: %w", err) + } + + defer func() { _ = tx.Rollback() }() + + q := s.db.Queries.WithTx(tx) + newKeys := make([]string, 0, len(clusters)) + + for _, c := range clusters { + var newKey string + if len(c.Tracks) == 1 { + // A lone leftover track from SplitPlan's singleton + // fallback may carry an empty (or shared-but-coincidental) + // album/album-artist tag — key on the track itself so two + // untagged leftovers can't collide. + newKey = autotag.SyntheticTrackGroupKey(parentKey, c.Tracks[0].AudioFileID) + } else { + newKey = autotag.SyntheticGroupKey(parentKey, c.AlbumName, c.AlbumArtist) + } + + newKeys = append(newKeys, newKey) + + for _, t := range c.Tracks { + if err := q.DecrementTaggingItemTrackCount(s.ctx, parentKey); err != nil { + return nil, fmt.Errorf("decrement parent group: %w", err) + } + + upsertParams := sqlcgen.UpsertTaggingItemOnTrackAddParams{ + GroupKey: newKey, + LibraryID: libraryID, + AlbumName: c.AlbumName, + AlbumArtist: c.AlbumArtist, + DiscNumber: 0, + } + if err := q.UpsertTaggingItemOnTrackAdd(s.ctx, upsertParams); err != nil { + return nil, fmt.Errorf("upsert synthetic group: %w", err) + } + + if err := q.SetAudioFileGroupKey(s.ctx, sqlcgen.SetAudioFileGroupKeyParams{ + GroupKey: newKey, + ID: t.AudioFileID, + }); err != nil { + return nil, fmt.Errorf("reassign track %d: %w", t.AudioFileID, err) + } + } + + if err := q.MarkTaggingItemSynthetic(s.ctx, sqlcgen.MarkTaggingItemSyntheticParams{ + ParentGroupKey: parentKey, + GroupKey: newKey, + }); err != nil { + return nil, fmt.Errorf("mark synthetic: %w", err) + } + } + + if err := q.DeleteTaggingItemIfEmpty(s.ctx, parentKey); err != nil { + return nil, fmt.Errorf("cleanup leftover parent: %w", err) + } + + // The parent's cached candidates (if it still exists) no longer + // reflect its track set now that some tracks moved out. + if err := q.DeleteTaggingCandidates(s.ctx, parentKey); err != nil { + s.logger.Warn("split: drop stale parent candidates", "group_key", parentKey, "err", err) + } + + if err := tx.Commit(); err != nil { + return nil, fmt.Errorf("commit split: %w", err) + } + + return newKeys, nil +} + // AckLibraryWarning records that the user has seen the first- // time-apply irreversibility warning for this library. func (s *Service) AckLibraryWarning(libraryID int64) error { @@ -1188,6 +1462,7 @@ func (s *Service) GetCandidatesForPasteURL( AlbumName: score.AlbumName, AlbumArtist: score.AlbumArtist, Tracks: score.LocalTracks, + Synthetic: score.Synthetic, }, pasted) merged := append([]autotag.Candidate{scored}, score.Candidates...) score.Candidates = merged @@ -1357,16 +1632,26 @@ func extractReleaseMBID(url string) string { // top-ranked candidate; pass nil to skip cover art entirely (used // only by paths that don't need art). func scoreToView(s *autotag.GroupScore, exp *explore.Service) *ScoreView { + group := autotag.Group{ + AlbumName: s.AlbumName, + AlbumArtist: s.AlbumArtist, + Tracks: s.LocalTracks, + Synthetic: s.Synthetic, + } + rec := s.Recommendation if rec == "" { // Paths that rebuild a GroupScore from cached candidates // don't run the scorer; derive the tier here. - rec = autotag.Recommend( - autotag.Group{Tracks: s.LocalTracks}, s.Candidates, - ) + rec = autotag.Recommend(group, s.Candidates) } - out := &ScoreView{GroupKey: s.GroupKey, Recommendation: string(rec)} + out := &ScoreView{ + GroupKey: s.GroupKey, + Recommendation: string(rec), + Synthetic: s.Synthetic, + MixedBag: !s.Synthetic && autotag.IsMixedBag(group), + } for _, l := range s.LocalTracks { out.LocalTracks = append(out.LocalTracks, LocalTrackView{ diff --git a/backend/autotagservice/service_test.go b/backend/autotagservice/service_test.go new file mode 100644 index 0000000..e341778 --- /dev/null +++ b/backend/autotagservice/service_test.go @@ -0,0 +1,406 @@ +package autotagservice + +import ( + "database/sql" + "errors" + "log/slog" + "testing" + + "yellowjacket/backend/autotag" + "yellowjacket/backend/database" + "yellowjacket/backend/database/sql/sqlcgen" +) + +// newTestService builds a Service with just enough wired up for +// SplitMixedFolder — no MB client, no tag writer. Constructed +// directly (bypassing NewService) since this package's tests live +// inside the package and don't need the explore/tagwriter +// dependencies that method never touches. +func newTestService(t *testing.T, db *database.DB) *Service { + t.Helper() + + logger := slog.New(slog.DiscardHandler) + + return &Service{ + db: db, + scorer: autotag.NewScorer(db.Queries, nil, logger), + logger: logger, + ctx: db.Ctx, + } +} + +// seedMixedBagFolder drops one physical folder (single group_key) +// containing two 2-track clusters (different album/album-artist tags +// each) plus one leftover track with no album tag at all — the shape +// SplitMixedFolder is meant to untangle. +func seedMixedBagFolder(t *testing.T, db *database.DB, groupKey string, libraryID int64) { + t.Helper() + + ctx := db.Ctx + q := db.Queries + + addTrack := func(filePath, title, artist, album, albumArtist string, trackNum int) { + ac, err := q.UpsertArtistCredit(ctx, artist) + if err != nil { + t.Fatalf("upsert artist credit: %v", err) + } + + rec, err := q.CreateRecordingFull(ctx, sqlcgen.CreateRecordingFullParams{ + Name: title, + ArtistCreditID: ac.ID, + TrackNumber: sql.NullInt64{Int64: int64(trackNum), Valid: true}, + }) + if err != nil { + t.Fatalf("create recording: %v", err) + } + + if album != "" { + albumArtistAC, err := q.UpsertArtistCredit(ctx, albumArtist) + if err != nil { + t.Fatalf("upsert album artist credit: %v", err) + } + + rg, err := q.UpsertReleaseGroup(ctx, sqlcgen.UpsertReleaseGroupParams{ + Name: album, + AlbumArtistCreditID: sql.NullInt64{Int64: albumArtistAC.ID, Valid: true}, + }) + if err != nil { + t.Fatalf("upsert release group: %v", err) + } + + if _, err := q.CreateReleaseGroupRecording( + ctx, + sqlcgen.CreateReleaseGroupRecordingParams{ + ReleaseGroupID: rg.ID, + RecordingID: rec.ID, + TrackNumber: sql.NullInt64{Int64: int64(trackNum), Valid: true}, + }, + ); err != nil { + t.Fatalf("link release group recording: %v", err) + } + } + + if _, err := q.CreateAudioFileWithGroupKey(ctx, sqlcgen.CreateAudioFileWithGroupKeyParams{ + FilePath: filePath, + LengthMilliseconds: 200000, + FileTypeID: 0, + RecordingID: rec.ID, + Basename: filePath, + LibraryID: libraryID, + GroupKey: groupKey, + TagStatus: "untagged", + }); err != nil { + t.Fatalf("create audio file: %v", err) + } + } + + addTrack("/junk/01.mp3", "Song A1", "Artist One", "Album One", "Artist One", 1) + addTrack("/junk/02.mp3", "Song A2", "Artist One", "Album One", "Artist One", 2) + addTrack("/junk/03.mp3", "Song B1", "Artist Two", "Album Two", "Artist Two", 1) + addTrack("/junk/04.mp3", "Song B2", "Artist Two", "Album Two", "Artist Two", 2) + addTrack("/junk/05.mp3", "Lone Song", "Artist Three", "", "", 1) + + if _, err := db.ExecContext(` + INSERT INTO tagging_items (group_key, library_id, track_count, album_name, album_artist, disc_number, status) + VALUES (?, ?, 5, '', '', 0, 'pending') + `, groupKey, libraryID); err != nil { + t.Fatalf("insert tagging item: %v", err) + } +} + +// seedCoherentAlbum drops a single-artist, single-album folder — the +// negative case for the mixed-bag triage query. +func seedCoherentAlbum(t *testing.T, db *database.DB, groupKey string, libraryID int64) { + t.Helper() + + ctx := db.Ctx + q := db.Queries + + ac, err := q.UpsertArtistCredit(ctx, "The Beatles") + if err != nil { + t.Fatalf("upsert artist credit: %v", err) + } + + rg, err := q.UpsertReleaseGroup(ctx, sqlcgen.UpsertReleaseGroupParams{ + Name: "Abbey Road", + AlbumArtistCreditID: sql.NullInt64{Int64: ac.ID, Valid: true}, + }) + if err != nil { + t.Fatalf("upsert release group: %v", err) + } + + titles := []string{"Come Together", "Something", "Maxwell's Silver Hammer", "Oh! Darling"} + for i, title := range titles { + rec, err := q.CreateRecordingFull(ctx, sqlcgen.CreateRecordingFullParams{ + Name: title, + ArtistCreditID: ac.ID, + TrackNumber: sql.NullInt64{Int64: int64(i + 1), Valid: true}, + }) + if err != nil { + t.Fatalf("create recording: %v", err) + } + + if _, err := q.CreateReleaseGroupRecording(ctx, sqlcgen.CreateReleaseGroupRecordingParams{ + ReleaseGroupID: rg.ID, + RecordingID: rec.ID, + TrackNumber: sql.NullInt64{Int64: int64(i + 1), Valid: true}, + }); err != nil { + t.Fatalf("link release group recording: %v", err) + } + + if _, err := q.CreateAudioFileWithGroupKey(ctx, sqlcgen.CreateAudioFileWithGroupKeyParams{ + FilePath: groupKey + "/" + title + ".mp3", + LengthMilliseconds: 200000, + FileTypeID: 0, + RecordingID: rec.ID, + Basename: title + ".mp3", + LibraryID: libraryID, + GroupKey: groupKey, + TagStatus: "untagged", + }); err != nil { + t.Fatalf("create audio file: %v", err) + } + } + + if _, err := db.ExecContext(` + INSERT INTO tagging_items (group_key, library_id, track_count, album_name, album_artist, disc_number, status) + VALUES (?, ?, 4, 'Abbey Road', 'The Beatles', 0, 'pending') + `, groupKey, libraryID); err != nil { + t.Fatalf("insert tagging item: %v", err) + } +} + +func TestListPendingFolders_FlagsLikelyMixedBag(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + seedMixedBagFolder(t, db, "g-junk", 0) + seedCoherentAlbum(t, db, "g-abbey-road", 0) + + s := newTestService(t, db) + + items, err := s.ListPendingFolders(0) + if err != nil { + t.Fatalf("ListPendingFolders: %v", err) + } + + got := make(map[string]bool, len(items)) + for _, it := range items { + got[it.GroupKey] = it.LikelyMixedBag + } + + if !got["g-junk"] { + t.Error("expected g-junk (no artist/album consensus) to be flagged LikelyMixedBag") + } + + if got["g-abbey-road"] { + t.Error( + "expected g-abbey-road (coherent single-artist album) to NOT be flagged LikelyMixedBag", + ) + } +} + +func TestSplitMixedFolder_CarvesOutClustersAndSingletons(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + seedMixedBagFolder(t, db, "g-junk", 0) + + s := newTestService(t, db) + + items, err := s.SplitMixedFolder("g-junk") + if err != nil { + t.Fatalf("SplitMixedFolder: %v", err) + } + + // Every track leaves the parent: 2 clustered groups (2 tracks + // each) + 1 singleton for the unclustered "Lone Song" track. The + // parent is now empty and must not survive as a 4th item. + if len(items) != 3 { //nolint:mnd + t.Fatalf("expected 3 resulting groups, got %d: %+v", len(items), items) + } + + for _, it := range items { + if it.GroupKey == "g-junk" { + t.Fatal("expected the original group to be fully drained and removed") + } + + if !it.Synthetic { + t.Errorf("child group %q: Synthetic = false, want true", it.GroupKey) + } + } + + var ( + clustered []PendingItem + singleton *PendingItem + ) + + for i, it := range items { + if it.TrackCount == 1 { + singleton = &items[i] + + continue + } + + clustered = append(clustered, it) + } + + if singleton == nil { + t.Fatal("expected a singleton child for the unclustered Lone Song track") + } + + if singleton.AlbumName != "" { + t.Errorf( + "singleton child album_name = %q, want empty (Lone Song had no album tag)", + singleton.AlbumName, + ) + } + + if len(clustered) != 2 { //nolint:mnd + t.Fatalf("expected 2 clustered children, got %d", len(clustered)) + } + + seenAlbums := map[string]bool{} + + for _, c := range clustered { + if c.TrackCount != 2 { //nolint:mnd + t.Errorf("child group %q: track_count = %d, want 2", c.GroupKey, c.TrackCount) + } + + seenAlbums[c.AlbumName] = true + } + + if !seenAlbums["Album One"] || !seenAlbums["Album Two"] { + t.Errorf("expected children for Album One and Album Two, got %+v", clustered) + } + + // The physical file paths must be untouched — only group_key + // reassignment happened, no files moved on disk. + locals, err := s.scorer.LocalTracksForGroup(db.Ctx, clustered[0].GroupKey) + if err != nil { + t.Fatalf("load synthetic group tracks: %v", err) + } + + for _, l := range locals { + if l.FilePath == "" { + t.Error("expected non-empty file path preserved on the synthetic group's tracks") + } + } +} + +func TestSplitMixedFolder_NothingToClusterErrors(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + + if _, err := db.Queries.CreateAudioFileWithGroupKey( + db.Ctx, + sqlcgen.CreateAudioFileWithGroupKeyParams{ + FilePath: "/coherent/01.mp3", + FileTypeID: 0, + RecordingID: mustCreateRecording(t, db, "Track"), + Basename: "01.mp3", + LibraryID: 0, + GroupKey: "g-coherent", + TagStatus: "untagged", + }, + ); err != nil { + t.Fatalf("create audio file: %v", err) + } + + if _, err := db.ExecContext(` + INSERT INTO tagging_items (group_key, library_id, track_count, album_name, album_artist, disc_number, status) + VALUES ('g-coherent', 0, 1, '', '', 0, 'pending') + `); err != nil { + t.Fatalf("insert tagging item: %v", err) + } + + s := newTestService(t, db) + + if _, err := s.SplitMixedFolder("g-coherent"); !errors.Is(err, errNothingToSplit) { + t.Fatalf("err = %v, want errNothingToSplit", err) + } +} + +func TestListPendingFolders_PrunesOrphanedEntries(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + + // A real, live folder — must survive. + if _, err := db.Queries.CreateAudioFileWithGroupKey( + db.Ctx, + sqlcgen.CreateAudioFileWithGroupKeyParams{ + FilePath: "/live/01.mp3", + FileTypeID: 0, + RecordingID: mustCreateRecording(t, db, "Track"), + Basename: "01.mp3", + LibraryID: 0, + GroupKey: "g-live", + TagStatus: "untagged", + }, + ); err != nil { + t.Fatalf("create audio file: %v", err) + } + + if _, err := db.ExecContext(` + INSERT INTO tagging_items (group_key, library_id, track_count, album_name, album_artist, disc_number, status) + VALUES ('g-live', 0, 1, '', '', 0, 'pending') + `); err != nil { + t.Fatalf("insert live tagging item: %v", err) + } + + // An orphaned row: no audio_files row points at this group_key + // any more (the file was deleted/moved and the bookkeeping that's + // supposed to clean this up never ran) — this is exactly the + // "old/nonexistent" entry the review UI shouldn't show. + if _, err := db.ExecContext(` + INSERT INTO tagging_items (group_key, library_id, track_count, album_name, album_artist, disc_number, status) + VALUES ('g-orphan', 0, 3, 'Ghost Album', 'Ghost Artist', 0, 'pending') + `); err != nil { + t.Fatalf("insert orphaned tagging item: %v", err) + } + + s := newTestService(t, db) + + items, err := s.ListPendingFolders(0) + if err != nil { + t.Fatalf("ListPendingFolders: %v", err) + } + + got := make(map[string]bool, len(items)) + for _, it := range items { + got[it.GroupKey] = true + } + + if !got["g-live"] { + t.Error("expected g-live (has a real audio_files row) to remain listed") + } + + if got["g-orphan"] { + t.Error("expected g-orphan (no matching audio_files rows) to be pruned, not listed") + } + + if _, err := db.Queries.GetTaggingItem(db.Ctx, "g-orphan"); !errors.Is(err, sql.ErrNoRows) { + t.Errorf("expected g-orphan row to be deleted from tagging_items, got err=%v", err) + } +} + +func mustCreateRecording(t *testing.T, db *database.DB, title string) int64 { + t.Helper() + + ac, err := db.Queries.UpsertArtistCredit(db.Ctx, "Artist") + if err != nil { + t.Fatalf("upsert artist credit: %v", err) + } + + rec, err := db.Queries.CreateRecordingFull(db.Ctx, sqlcgen.CreateRecordingFullParams{ + Name: title, + ArtistCreditID: ac.ID, + }) + if err != nil { + t.Fatalf("create recording: %v", err) + } + + return rec.ID +} diff --git a/backend/database/database.go b/backend/database/database.go index 854b5e1..530872c 100644 --- a/backend/database/database.go +++ b/backend/database/database.go @@ -5,10 +5,13 @@ import ( "context" "database/sql" "embed" + "errors" "fmt" "io/fs" "log/slog" "path" + "sort" + "strconv" "strings" _ "modernc.org/sqlite" // Register sqlite driver. @@ -23,6 +26,9 @@ import ( //go:embed sql/schemas/*.sql var schemas embed.FS +//go:embed sql/migrations/*.sql +var migrations embed.FS + // DB wraps the SQLite database connection and queries. // // Two handles back a single database file. db is the single-writer @@ -253,12 +259,20 @@ func (d *DB) ResumeExploreIndexFTS() error { return nil } -// applySchema creates the full schema on a fresh database. +// applySchema creates the full schema on a fresh database and brings +// an existing one up to date via sql/migrations. // -// Every statement is CREATE ... IF NOT EXISTS, so this is idempotent and -// runs unconditionally at open. There is no migration chain: the files -// in sql/schemas describe the only schema the app has, and a database -// written by an older build is not supported. +// The schema files under sql/schemas are CREATE ... IF NOT EXISTS, +// so on a genuinely new database they create every table already at +// its current, latest shape — that's the fast path new installs +// take. A database that already has an older shape (e.g. a +// tagging_items missing a column a later build added) needs the gap +// closed, which IF NOT EXISTS can't do: it silently no-ops on a +// table that already exists, columns and all. sql/migrations holds +// small, additive, numbered files (ALTER TABLE, CREATE INDEX, etc.) +// for exactly that gap, tracked in schema_migrations so each applies +// at most once — see applyMigrations for how a fresh database's +// already-current tables tolerate replaying them anyway. func applySchema(ctx context.Context, db *sql.DB) error { dirEntries, err := schemas.ReadDir("sql/schemas") if err != nil { @@ -288,9 +302,163 @@ func applySchema(ctx context.Context, db *sql.DB) error { return fmt.Errorf("could not create explore FTS triggers: %w", err) } + if err := applyMigrations(ctx, db); err != nil { + return fmt.Errorf("could not apply migrations: %w", err) + } + return nil } +// schemaMigrationsTable tracks which sql/migrations files have run, +// by their leading numeric prefix. +const schemaMigrationsTable = ` +CREATE TABLE IF NOT EXISTS schema_migrations ( + version INTEGER PRIMARY KEY, + applied_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP +)` + +// applyMigrations runs every sql/migrations file not yet recorded in +// schema_migrations, in filename order (numeric prefix), one +// statement at a time. +// +// Every migration runs on EVERY database, fresh or old — there is no +// "skip on fresh install" branch. A fresh database's tables already +// carry a migration's effect (sql/schemas declares the target shape +// directly), so its statements are expected to sometimes be no-ops +// there: "duplicate column name" from an ALTER TABLE ADD COLUMN is +// tolerated and treated as "already applied", the same way +// createExploreIndexFTSTriggers tolerates "already exists". Any +// other error is fatal. This is deliberately simpler than detecting +// "is this database fresh" — every migration converges both a fresh +// and an upgraded database to the identical final schema (including +// column order — ALTER TABLE ADD COLUMN always appends at the end, +// so sql/schemas must declare a migrated column last too; see the +// comment on tagging_items.sql and the regression test in +// migrations_test.go). +func applyMigrations(ctx context.Context, db *sql.DB) error { + if _, err := db.ExecContext(ctx, schemaMigrationsTable); err != nil { + return fmt.Errorf("create schema_migrations: %w", err) + } + + dirEntries, err := migrations.ReadDir("sql/migrations") + if err != nil { + return fmt.Errorf("could not read migrations directory: %w", err) + } + + sort.Slice(dirEntries, func(i, j int) bool { + return dirEntries[i].Name() < dirEntries[j].Name() + }) + + for _, dirEntry := range dirEntries { + if dirEntry.IsDir() { + continue + } + + version, err := migrationVersion(dirEntry.Name()) + if err != nil { + return err + } + + applied, err := migrationApplied(ctx, db, version) + if err != nil { + return err + } + + if applied { + continue + } + + filePath := path.Join("sql/migrations", dirEntry.Name()) + + sqlContent, err := fs.ReadFile(migrations, filePath) + if err != nil { + return fmt.Errorf("could not read file %s: %w", filePath, err) + } + + if err := execMigrationStatements(ctx, db, string(sqlContent)); err != nil { + return fmt.Errorf("error executing migration %s: %w", dirEntry.Name(), err) + } + + if _, err := db.ExecContext( + ctx, `INSERT INTO schema_migrations (version) VALUES (?)`, version, + ); err != nil { + return fmt.Errorf("record migration %d applied: %w", version, err) + } + } + + return nil +} + +// execMigrationStatements runs a migration file one statement at a +// time — NOT as one multi-statement Exec — so that one statement +// being a tolerable no-op (ALTER TABLE ADD COLUMN on a fresh +// database) doesn't abort the statements after it in the same file +// (e.g. a trailing CREATE INDEX that a fresh database still needs, +// since sql/schemas deliberately doesn't declare an index on a +// migrated column — see the comment on tagging_items.sql). +// +// Splitting on ";" is safe for the simple ALTER/CREATE TABLE/CREATE +// INDEX statements migrations are expected to contain; it is NOT +// safe for statements embedding a literal semicolon (e.g. a CREATE +// TRIGGER body) — write those with executeContext calls in Go +// instead of a sql/migrations file, the same way the explore FTS +// triggers already are. +func execMigrationStatements(ctx context.Context, db *sql.DB, script string) error { + for stmt := range strings.SplitSeq(script, ";") { + stmt = strings.TrimSpace(stmt) + if stmt == "" { + continue + } + + if _, err := db.ExecContext(ctx, stmt); err != nil { + if strings.Contains(err.Error(), "duplicate column name") { + continue + } + + return fmt.Errorf("statement %q: %w", stmt, err) + } + } + + return nil +} + +// migrationVersion extracts the leading integer prefix from a +// migration filename, e.g. "0001_tagging_items_synthetic.sql" -> 1. +func migrationVersion(filename string) (int, error) { + prefix, _, ok := strings.Cut(filename, "_") + if !ok { + return 0, fmt.Errorf("%w: %s", errMigrationFilename, filename) + } + + version, err := strconv.Atoi(prefix) + if err != nil { + return 0, fmt.Errorf("%w: %s", errMigrationFilename, filename) + } + + return version, nil +} + +var errMigrationFilename = errors.New( + "migration filename must start with a numeric prefix followed by '_' (e.g. 0001_description.sql)", +) + +func migrationApplied(ctx context.Context, db *sql.DB, version int) (bool, error) { + var v int + + err := db.QueryRowContext( + ctx, `SELECT version FROM schema_migrations WHERE version = ?`, version, + ).Scan(&v) + + switch { + case errors.Is(err, sql.ErrNoRows): + return false, nil + case err != nil: + return false, fmt.Errorf("check migration %d: %w", version, err) + default: + return true, nil + } +} + // applyPRAGMAs configures SQLite connection settings. Called by both // NewDB and NewTestDB to ensure identical behavior. func applyPRAGMAs(ctx context.Context, db *sql.DB) error { diff --git a/backend/database/migrations_test.go b/backend/database/migrations_test.go new file mode 100644 index 0000000..b31fb7a --- /dev/null +++ b/backend/database/migrations_test.go @@ -0,0 +1,196 @@ +package database + +import ( + "database/sql" + "testing" +) + +// oldTaggingItemsDDL is a frozen snapshot of tagging_items exactly as +// it read before sql/migrations/0001_tagging_items_synthetic.sql — +// i.e. what a real user's existing database looks like today, before +// upgrading to a build that includes that migration. +const oldTaggingItemsDDL = ` +CREATE TABLE IF NOT EXISTS tagging_items ( + group_key TEXT PRIMARY KEY, + library_id INTEGER NOT NULL, + track_count INTEGER NOT NULL DEFAULT 0, + album_name TEXT NOT NULL DEFAULT '', + album_artist TEXT NOT NULL DEFAULT '', + disc_number INTEGER NOT NULL DEFAULT 0, + best_match_release_mbid TEXT, + score REAL, + last_checked_at DATETIME, + status TEXT NOT NULL DEFAULT 'pending' + CHECK(status IN ('pending', 'matched', 'confirmed', 'skipped')), + cleared_at DATETIME, + created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, + FOREIGN KEY(library_id) REFERENCES libraries(id) +); + +CREATE INDEX IF NOT EXISTS idx_tagging_items_library_status + ON tagging_items(library_id, status); + +CREATE INDEX IF NOT EXISTS idx_tagging_items_status_pending + ON tagging_items(library_id) WHERE status = 'pending'; +` + +// tableColumns returns the column names of a table in on-disk +// (positional) order, via PRAGMA table_info — the order sqlc's +// generated `SELECT *` scans bind to positionally. +func tableColumns(t *testing.T, db *sql.DB, table string) []string { + t.Helper() + + rows, err := db.QueryContext(t.Context(), "PRAGMA table_info("+table+")") + if err != nil { + t.Fatalf("PRAGMA table_info(%s): %v", table, err) + } + + defer func() { _ = rows.Close() }() + + var cols []string + + for rows.Next() { + var ( + cid int + name string + ctype string + notnull int + dfltValue sql.NullString + primaryKey int + ) + + if err := rows.Scan(&cid, &name, &ctype, ¬null, &dfltValue, &primaryKey); err != nil { + t.Fatalf("scan table_info row: %v", err) + } + + cols = append(cols, name) + } + + if err := rows.Err(); err != nil { + t.Fatalf("iterate table_info: %v", err) + } + + return cols +} + +func openMemDB(t *testing.T) *sql.DB { + t.Helper() + + db, err := sql.Open("sqlite", ":memory:?_busy_timeout=5000&_journal_mode=WAL") + if err != nil { + t.Fatalf("open in-memory db: %v", err) + } + + db.SetMaxOpenConns(1) + t.Cleanup(func() { _ = db.Close() }) + + if err := applyPRAGMAs(t.Context(), db); err != nil { + t.Fatalf("apply pragmas: %v", err) + } + + return db +} + +// TestMigrations_ColumnOrderMatchesFreshInstall is the regression +// test for the exact failure mode that got the old 48-step migration +// chain torn out (see .planning/NOTES.md, "No migration chain"): +// sql/schemas drifting from what migrations actually produce, so +// sqlc-generated code silently reads the wrong thing. +// +// A fresh install takes tagging_items straight from sql/schemas +// (CREATE TABLE, columns in file order). An existing database takes +// it from sql/schemas (the base shape, unchanged since the table +// already existed) plus sql/migrations/0001 (`ALTER TABLE ADD +// COLUMN`, which SQLite always appends at the END of the column +// list, regardless of where the column sits in the CREATE TABLE +// statement). If sql/schemas ever declares a migrated column +// somewhere other than last, the two paths produce tables with the +// SAME columns in a DIFFERENT order — invisible until a `SELECT *` +// (e.g. GetTaggingItem) silently binds a value to the wrong field. +func TestMigrations_ColumnOrderMatchesFreshInstall(t *testing.T) { + t.Parallel() + + fresh := openMemDB(t) + if err := applySchema(t.Context(), fresh); err != nil { + t.Fatalf("apply schema (fresh): %v", err) + } + + upgraded := openMemDB(t) + + librariesDDL, err := schemas.ReadFile("sql/schemas/libraries.sql") + if err != nil { + t.Fatalf("read libraries schema: %v", err) + } + + if _, err := upgraded.ExecContext(t.Context(), string(librariesDDL)); err != nil { + t.Fatalf("create libraries table: %v", err) + } + + if _, err := upgraded.ExecContext(t.Context(), oldTaggingItemsDDL); err != nil { + t.Fatalf("create pre-migration tagging_items: %v", err) + } + + // sql/schemas no-ops on the pre-existing tagging_items (IF NOT + // EXISTS), then sql/migrations/0001's ALTER TABLE statements + // actually add the missing columns for real this time. + if err := applySchema(t.Context(), upgraded); err != nil { + t.Fatalf("apply schema (upgrade path): %v", err) + } + + freshCols := tableColumns(t, fresh, "tagging_items") + upgradedCols := tableColumns(t, upgraded, "tagging_items") + + if len(freshCols) != len(upgradedCols) { + t.Fatalf( + "column count mismatch: fresh install has %d (%v), upgraded has %d (%v)", + len(freshCols), freshCols, len(upgradedCols), upgradedCols, + ) + } + + for i := range freshCols { + if freshCols[i] != upgradedCols[i] { + t.Errorf( + "column order mismatch at position %d: fresh install has %q, upgraded has %q\nfresh: %v\nupgraded: %v", + i, + freshCols[i], + upgradedCols[i], + freshCols, + upgradedCols, + ) + } + } +} + +// TestMigrations_FreshDatabaseStillRecordsAndGetsIndex confirms a +// brand-new database runs migration 0001 (tolerating "duplicate +// column name" from its ALTER TABLE statements, since sql/schemas +// already declared those columns), records it applied, AND still +// gets the trailing CREATE INDEX statement sql/schemas deliberately +// omits for migrated columns. +func TestMigrations_FreshDatabaseStillRecordsAndGetsIndex(t *testing.T) { + t.Parallel() + + fresh := openMemDB(t) + if err := applySchema(t.Context(), fresh); err != nil { + t.Fatalf("apply schema: %v", err) + } + + var version int + + err := fresh.QueryRowContext( + t.Context(), "SELECT version FROM schema_migrations WHERE version = 1", + ).Scan(&version) + if err != nil { + t.Fatalf("expected migration 1 to be recorded as applied on a fresh db: %v", err) + } + + var indexName string + + err = fresh.QueryRowContext( + t.Context(), + "SELECT name FROM sqlite_master WHERE type = 'index' AND name = 'idx_tagging_items_parent_group_key'", + ).Scan(&indexName) + if err != nil { + t.Fatalf("expected idx_tagging_items_parent_group_key to exist on a fresh db: %v", err) + } +} diff --git a/backend/database/sql/migrations/0001_tagging_items_synthetic.sql b/backend/database/sql/migrations/0001_tagging_items_synthetic.sql new file mode 100644 index 0000000..cdd9e4b --- /dev/null +++ b/backend/database/sql/migrations/0001_tagging_items_synthetic.sql @@ -0,0 +1,10 @@ +-- Adds SplitMixedFolder's synthetic-group bookkeeping to an +-- existing tagging_items table. A fresh database never runs this +-- file: sql/schemas/tagging_items.sql already declares these +-- columns, so applySchema's isFreshDatabase check stamps this +-- version as applied without executing it. +ALTER TABLE tagging_items ADD COLUMN synthetic INTEGER NOT NULL DEFAULT 0; +ALTER TABLE tagging_items ADD COLUMN parent_group_key TEXT NOT NULL DEFAULT ''; + +CREATE INDEX IF NOT EXISTS idx_tagging_items_parent_group_key + ON tagging_items(parent_group_key) WHERE parent_group_key != ''; diff --git a/backend/database/sql/migrations/0002_tagging_items_orphan_cleanup.sql b/backend/database/sql/migrations/0002_tagging_items_orphan_cleanup.sql new file mode 100644 index 0000000..8376f7c --- /dev/null +++ b/backend/database/sql/migrations/0002_tagging_items_orphan_cleanup.sql @@ -0,0 +1,24 @@ +-- Repairs tagging_items rows left behind by a library-scan bug: the +-- rescan's orphan-cleanup phase deleted audio_files rows for files +-- removed from disk without decrementing/clearing their tagging +-- group, so a folder whose contents were fully replaced kept a +-- phantom entry (stale track_count, no matching audio_files) in the +-- autotag queue forever. The library scan code no longer has this +-- gap, but a database written before the fix still carries the +-- damage — this is a one-time repair, not ongoing bookkeeping. +-- +-- Drop groups with no audio_files left at all. +DELETE FROM tagging_items +WHERE group_key NOT IN ( + SELECT DISTINCT group_key FROM audio_files WHERE group_key != '' +); + +-- Reconcile track_count for groups that are still alive but drifted +-- (some, not all, of their tracks were removed without decrementing). +UPDATE tagging_items +SET track_count = ( + SELECT COUNT(*) FROM audio_files WHERE audio_files.group_key = tagging_items.group_key +) +WHERE track_count != ( + SELECT COUNT(*) FROM audio_files WHERE audio_files.group_key = tagging_items.group_key +); diff --git a/backend/database/sql/queries/artist_credit.sql b/backend/database/sql/queries/artist_credit.sql index e80020a..17eff1e 100644 --- a/backend/database/sql/queries/artist_credit.sql +++ b/backend/database/sql/queries/artist_credit.sql @@ -32,3 +32,11 @@ SELECT (SELECT COUNT(*) FROM recordings WHERE artist_credit_id = ?1) + (SELECT COUNT(*) FROM release_groups WHERE album_artist_credit_id = ?1) AS total; + +-- name: GetOrphanedArtistCreditIDs :many +-- Artist credits no longer used by any recording or release group - run +-- after orphaned recordings/release groups are deleted, so a credit +-- that only existed for now-removed tracks is cleaned up too. +SELECT ac.id FROM artist_credit ac +WHERE NOT EXISTS (SELECT 1 FROM recordings r WHERE r.artist_credit_id = ac.id) + AND NOT EXISTS (SELECT 1 FROM release_groups rg WHERE rg.album_artist_credit_id = ac.id); diff --git a/backend/database/sql/queries/artist_credit_artists.sql b/backend/database/sql/queries/artist_credit_artists.sql index 617157e..3cdacac 100644 --- a/backend/database/sql/queries/artist_credit_artists.sql +++ b/backend/database/sql/queries/artist_credit_artists.sql @@ -18,3 +18,7 @@ WHERE id =?; -- name: DeleteAllArtistCreditArtists :exec DELETE FROM artist_credit_artist; +-- name: DeleteArtistCreditArtistByCredit :exec +DELETE FROM artist_credit_artist +WHERE credit_id = ?; + diff --git a/backend/database/sql/queries/artists.sql b/backend/database/sql/queries/artists.sql index 942b910..d3a6530 100644 --- a/backend/database/sql/queries/artists.sql +++ b/backend/database/sql/queries/artists.sql @@ -39,6 +39,15 @@ JOIN artist_credit ac ON ac.id = aca.credit_id JOIN release_groups rg ON rg.album_artist_credit_id = ac.id ORDER BY a.name; +-- name: GetOrphanedArtistIDs :many +-- Artists no longer credited on any recording or release group - left +-- behind when a scan's orphan cleanup removes the audio_files that used +-- to justify them, since deleting an audio_files row doesn't cascade. +SELECT a.id FROM artists a +WHERE NOT EXISTS ( + SELECT 1 FROM artist_credit_artist aca WHERE aca.artist_id = a.id +); + -- name: GetAlbumArtistsByLibrary :many SELECT DISTINCT a.id, a.name, a.mbid FROM artists a diff --git a/backend/database/sql/queries/recordings.sql b/backend/database/sql/queries/recordings.sql index ca03f26..a7a3167 100644 --- a/backend/database/sql/queries/recordings.sql +++ b/backend/database/sql/queries/recordings.sql @@ -37,3 +37,11 @@ ORDER BY name; -- name: CountRecordingsByArtistCredit :one SELECT COUNT(*) FROM recordings WHERE artist_credit_id = ?; + +-- name: GetOrphanedRecordingIDs :many +-- Recordings no longer backed by any audio_files row - left behind +-- when a scan's orphan cleanup deletes the file that used to own them, +-- since deleting audio_files doesn't cascade to recordings. +SELECT r.id FROM recordings r +LEFT JOIN audio_files af ON af.recording_id = r.id +WHERE af.id IS NULL; diff --git a/backend/database/sql/queries/release_group_recordings.sql b/backend/database/sql/queries/release_group_recordings.sql index 6198bce..1a1ce40 100644 --- a/backend/database/sql/queries/release_group_recordings.sql +++ b/backend/database/sql/queries/release_group_recordings.sql @@ -26,3 +26,7 @@ WHERE release_group_id = ? AND recording_id = ?; -- name: DeleteAllReleaseGroupRecordings :exec DELETE FROM release_group_recordings; + +-- name: DeleteReleaseGroupRecordingsByRecording :exec +DELETE FROM release_group_recordings +WHERE recording_id = ?; diff --git a/backend/database/sql/queries/release_groups.sql b/backend/database/sql/queries/release_groups.sql index ed58984..eb9fba7 100644 --- a/backend/database/sql/queries/release_groups.sql +++ b/backend/database/sql/queries/release_groups.sql @@ -167,6 +167,14 @@ ORDER BY rg.name; -- name: CountReleaseGroupRecordings :one SELECT COUNT(*) FROM release_group_recordings WHERE release_group_id = ?; +-- name: GetOrphanedReleaseGroupIDs :many +-- Release groups with no recordings left in them - run after orphaned +-- recordings (and their release_group_recordings rows) are deleted, so +-- a release group whose last owned track was removed is cleaned up too. +SELECT rg.id FROM release_groups rg +LEFT JOIN release_group_recordings rgr ON rgr.release_group_id = rg.id +WHERE rgr.id IS NULL; + -- name: GetAlbumsByArtistByLibrary :many SELECT rg.id, diff --git a/backend/database/sql/queries/tagging_items.sql b/backend/database/sql/queries/tagging_items.sql index 8d130fc..89aef15 100644 --- a/backend/database/sql/queries/tagging_items.sql +++ b/backend/database/sql/queries/tagging_items.sql @@ -24,11 +24,63 @@ WHERE group_key = ?; DELETE FROM tagging_items WHERE group_key = ? AND track_count <= 0; +-- name: PruneOrphanedTaggingItems :exec +-- Self-healing sweep for rows whose track_count bookkeeping (scan +-- orphan cleanup, maybeRebindTaggingGroup, SplitMixedFolder) never +-- ran or drifted: a cancelled scan, a library move/rename the +-- SoftScanAllLibraries disk-count/mtime heuristic did not catch, or +-- a decrement that landed without its paired delete. Rather than +-- trust track_count, this checks the ground truth directly: any +-- group_key no audio_files row still points at is gone, and its +-- tagging_items row (and cascaded tagging_candidates) should be too. +-- Cheap: one indexed (idx_audio_files_group_key) existence check per +-- row. Called opportunistically wherever the pending list is read, +-- so stale entries cannot linger indefinitely between full rescans. +DELETE FROM tagging_items +WHERE NOT EXISTS ( + SELECT 1 FROM audio_files af WHERE af.group_key = tagging_items.group_key +); + +-- name: MarkTaggingItemSynthetic :exec +-- Stamps a group as carved out of parent_group_key by +-- SplitMixedFolder. Idempotent: safe to call every time a track +-- is migrated into the synthetic group, not just on first creation. +UPDATE tagging_items +SET synthetic = 1, + parent_group_key = ? +WHERE group_key = ?; + -- name: GetTaggingItem :one SELECT * FROM tagging_items WHERE group_key = ? LIMIT 1; +-- name: ListLikelyMixedBagGroupKeys :many +-- Cheap, whole-library triage pass for autotag.IsMixedBag: one +-- grouped scan over audio_files (indexed on group_key) rather than +-- hydrating every group's full track list in Go. LOWER/TRIM is an +-- approximation of autotag.Normalize (no unicode fold, no qualifier +-- stripping) so this can flag a false positive Normalize would +-- clear, or miss a true one Normalize would catch. Treat it as a +-- triage filter for which groups are worth a real autotag. +-- IsMixedBag check, or a badge at minimum, not the final word. +SELECT ti.group_key +FROM tagging_items ti +JOIN audio_files af ON af.group_key = ti.group_key +LEFT JOIN recordings r ON af.recording_id = r.id +LEFT JOIN artist_credit ac ON r.artist_credit_id = ac.id +LEFT JOIN release_group_recordings rgr ON rgr.recording_id = r.id +LEFT JOIN release_groups rg ON rg.id = rgr.release_group_id +WHERE ti.synthetic = 0 + AND ti.track_count >= 4 + AND ( + ti.album_artist = '' + OR LOWER(TRIM(ti.album_artist)) IN ('various artists', 'various', 'va', 'v.a.', 'v a', 'unknown') + ) +GROUP BY ti.group_key +HAVING COUNT(DISTINCT CASE WHEN ac.text != '' THEN LOWER(TRIM(ac.text)) END) > 1 + AND COUNT(DISTINCT CASE WHEN rg.name != '' THEN LOWER(TRIM(rg.name)) END) > 1; + -- name: CountPendingTaggingItems :one SELECT COUNT(*) FROM tagging_items WHERE status = 'pending' @@ -71,7 +123,8 @@ SELECT ti.score, ti.last_checked_at, ti.status, - ti.created_at + ti.created_at, + ti.synthetic FROM tagging_items ti LEFT JOIN libraries lb ON lb.id = ti.library_id WHERE (CAST(@library_id AS INTEGER) = 0 OR ti.library_id = @library_id) @@ -102,7 +155,8 @@ SELECT ti.score, ti.last_checked_at, ti.status, - ti.created_at + ti.created_at, + ti.synthetic FROM tagging_items ti LEFT JOIN libraries lb ON lb.id = ti.library_id WHERE ti.group_key = ? @@ -130,6 +184,10 @@ ORDER BY ti.created_at DESC, ti.group_key LIMIT @row_limit OFFSET @row_offset; -- name: ListAudioFilesInTaggingGroup :many +-- album_name/album_artist are the PER-TRACK tags (via each track's +-- own release_group link), not the folder-level tagging_items +-- values. SplitMixedFolder clusters on these to find sub-albums +-- hiding inside a folder full of unrelated tracks. SELECT af.id, af.file_path, @@ -140,10 +198,15 @@ SELECT COALESCE(r.disc_number, 0) AS disc_number, COALESCE(r.name, '') AS title, COALESCE(ac.text, '') AS artist_name, - COALESCE(r.mbid, '') AS recording_mbid + COALESCE(r.mbid, '') AS recording_mbid, + COALESCE(rg.name, '') AS album_name, + COALESCE(rgac.text, '') AS album_artist FROM audio_files af LEFT JOIN recordings r ON af.recording_id = r.id LEFT JOIN artist_credit ac ON r.artist_credit_id = ac.id +LEFT JOIN release_group_recordings rgr ON rgr.recording_id = r.id +LEFT JOIN release_groups rg ON rg.id = rgr.release_group_id +LEFT JOIN artist_credit rgac ON rg.album_artist_credit_id = rgac.id WHERE af.group_key = ? ORDER BY COALESCE(r.disc_number, 0), COALESCE(r.track_number, 0), diff --git a/backend/database/sql/schemas/tagging_items.sql b/backend/database/sql/schemas/tagging_items.sql index fe2753b..49a7a1f 100644 --- a/backend/database/sql/schemas/tagging_items.sql +++ b/backend/database/sql/schemas/tagging_items.sql @@ -17,6 +17,28 @@ CREATE TABLE IF NOT EXISTS tagging_items ( -- review state. cleared_at DATETIME, created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, + -- synthetic marks a group carved out of a "mixed bag" folder by + -- SplitMixedFolder: its tracks share a folder with unrelated + -- tracks (a junk-drawer directory) but were clustered together by + -- matching album/album-artist tags rather than by directory. + -- Scoring relaxes the missing-track penalty for these groups, + -- since they're a subset pulled out of a bigger folder, not a + -- complete rip of their own directory. parent_group_key is the + -- original folder group they were split from. + -- + -- These two columns are declared LAST, after created_at, even + -- though that reads oddly next to the rest of the table: sql/ + -- migrations/0001 brings a pre-existing tagging_items up to date + -- with `ALTER TABLE ADD COLUMN`, which SQLite always appends at + -- the end of the column list. A fresh install (this file) and an + -- upgraded database (this file + the migration) must end up with + -- IDENTICAL column order, because sqlc-generated `SELECT *` scans + -- (e.g. GetTaggingItem) bind columns positionally — see the + -- schema/migration column-order test in database_test.go. Put + -- new columns wherever reads best when adding a table for the + -- first time; append-only from the second migration on. + synthetic INTEGER NOT NULL DEFAULT 0, + parent_group_key TEXT NOT NULL DEFAULT '', FOREIGN KEY(library_id) REFERENCES libraries(id) ); @@ -25,3 +47,10 @@ CREATE INDEX IF NOT EXISTS idx_tagging_items_library_status CREATE INDEX IF NOT EXISTS idx_tagging_items_status_pending ON tagging_items(library_id) WHERE status = 'pending'; + +-- idx_tagging_items_parent_group_key is NOT declared here on +-- purpose: this file runs unconditionally, before migrations, even +-- against a database that hasn't run 0001 yet — an index predicate +-- referencing parent_group_key would fail on that table. It lives +-- solely in sql/migrations/0001_tagging_items_synthetic.sql, which +-- runs after the column exists either way (see database.go). diff --git a/backend/database/sql/sqlcgen/artist_credit.sql.go b/backend/database/sql/sqlcgen/artist_credit.sql.go index 6c69c8c..ae2deb9 100644 --- a/backend/database/sql/sqlcgen/artist_credit.sql.go +++ b/backend/database/sql/sqlcgen/artist_credit.sql.go @@ -78,6 +78,38 @@ func (q *Queries) GetArtistCreditByText(ctx context.Context, text string) (Artis return i, err } +const getOrphanedArtistCreditIDs = `-- name: GetOrphanedArtistCreditIDs :many +SELECT ac.id FROM artist_credit ac +WHERE NOT EXISTS (SELECT 1 FROM recordings r WHERE r.artist_credit_id = ac.id) + AND NOT EXISTS (SELECT 1 FROM release_groups rg WHERE rg.album_artist_credit_id = ac.id) +` + +// Artist credits no longer used by any recording or release group - run +// after orphaned recordings/release groups are deleted, so a credit +// that only existed for now-removed tracks is cleaned up too. +func (q *Queries) GetOrphanedArtistCreditIDs(ctx context.Context) ([]int64, error) { + rows, err := q.db.QueryContext(ctx, getOrphanedArtistCreditIDs) + if err != nil { + return nil, err + } + defer rows.Close() + var items []int64 + for rows.Next() { + var id int64 + if err := rows.Scan(&id); err != nil { + return nil, err + } + items = append(items, id) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const updateArtistCredit = `-- name: UpdateArtistCredit :exec UPDATE artist_credit SET text = ? diff --git a/backend/database/sql/sqlcgen/artist_credit_artists.sql.go b/backend/database/sql/sqlcgen/artist_credit_artists.sql.go index f762fbd..da1ff97 100644 --- a/backend/database/sql/sqlcgen/artist_credit_artists.sql.go +++ b/backend/database/sql/sqlcgen/artist_credit_artists.sql.go @@ -45,6 +45,16 @@ func (q *Queries) DeleteArtistCreditArtist(ctx context.Context, id int64) error return err } +const deleteArtistCreditArtistByCredit = `-- name: DeleteArtistCreditArtistByCredit :exec +DELETE FROM artist_credit_artist +WHERE credit_id = ? +` + +func (q *Queries) DeleteArtistCreditArtistByCredit(ctx context.Context, creditID int64) error { + _, err := q.db.ExecContext(ctx, deleteArtistCreditArtistByCredit, creditID) + return err +} + const getArtistCreditArtist = `-- name: GetArtistCreditArtist :one SELECT id, artist_id, credit_id FROM artist_credit_artist WHERE id = ? LIMIT 1 diff --git a/backend/database/sql/sqlcgen/artists.sql.go b/backend/database/sql/sqlcgen/artists.sql.go index e71f0d1..b4f5f17 100644 --- a/backend/database/sql/sqlcgen/artists.sql.go +++ b/backend/database/sql/sqlcgen/artists.sql.go @@ -166,6 +166,39 @@ func (q *Queries) GetArtistByName(ctx context.Context, name string) (Artist, err return i, err } +const getOrphanedArtistIDs = `-- name: GetOrphanedArtistIDs :many +SELECT a.id FROM artists a +WHERE NOT EXISTS ( + SELECT 1 FROM artist_credit_artist aca WHERE aca.artist_id = a.id +) +` + +// Artists no longer credited on any recording or release group - left +// behind when a scan's orphan cleanup removes the audio_files that used +// to justify them, since deleting an audio_files row doesn't cascade. +func (q *Queries) GetOrphanedArtistIDs(ctx context.Context) ([]int64, error) { + rows, err := q.db.QueryContext(ctx, getOrphanedArtistIDs) + if err != nil { + return nil, err + } + defer rows.Close() + var items []int64 + for rows.Next() { + var id int64 + if err := rows.Scan(&id); err != nil { + return nil, err + } + items = append(items, id) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const updateArtist = `-- name: UpdateArtist :exec UPDATE artists SET name = ? diff --git a/backend/database/sql/sqlcgen/models.go b/backend/database/sql/sqlcgen/models.go index 6af50e8..ed4dd2a 100644 --- a/backend/database/sql/sqlcgen/models.go +++ b/backend/database/sql/sqlcgen/models.go @@ -361,6 +361,8 @@ type TaggingItem struct { Status string ClearedAt sql.NullTime CreatedAt time.Time + Synthetic int64 + ParentGroupKey string } type TrackMetadatum struct { diff --git a/backend/database/sql/sqlcgen/recordings.sql.go b/backend/database/sql/sqlcgen/recordings.sql.go index 5bfda76..2d1193b 100644 --- a/backend/database/sql/sqlcgen/recordings.sql.go +++ b/backend/database/sql/sqlcgen/recordings.sql.go @@ -158,6 +158,38 @@ func (q *Queries) GetAllRecordings(ctx context.Context) ([]Recording, error) { return items, nil } +const getOrphanedRecordingIDs = `-- name: GetOrphanedRecordingIDs :many +SELECT r.id FROM recordings r +LEFT JOIN audio_files af ON af.recording_id = r.id +WHERE af.id IS NULL +` + +// Recordings no longer backed by any audio_files row - left behind +// when a scan's orphan cleanup deletes the file that used to own them, +// since deleting audio_files doesn't cascade to recordings. +func (q *Queries) GetOrphanedRecordingIDs(ctx context.Context) ([]int64, error) { + rows, err := q.db.QueryContext(ctx, getOrphanedRecordingIDs) + if err != nil { + return nil, err + } + defer rows.Close() + var items []int64 + for rows.Next() { + var id int64 + if err := rows.Scan(&id); err != nil { + return nil, err + } + items = append(items, id) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const getRecording = `-- name: GetRecording :one SELECT id, name, artist_credit_id, track_number, disc_number, year, genre, composer, lyrics, comment, mbid FROM recordings WHERE id = ? LIMIT 1 diff --git a/backend/database/sql/sqlcgen/release_group_recordings.sql.go b/backend/database/sql/sqlcgen/release_group_recordings.sql.go index 22b9b48..a8e6b81 100644 --- a/backend/database/sql/sqlcgen/release_group_recordings.sql.go +++ b/backend/database/sql/sqlcgen/release_group_recordings.sql.go @@ -75,6 +75,16 @@ func (q *Queries) DeleteReleaseGroupRecordingByFK(ctx context.Context, arg Delet return err } +const deleteReleaseGroupRecordingsByRecording = `-- name: DeleteReleaseGroupRecordingsByRecording :exec +DELETE FROM release_group_recordings +WHERE recording_id = ? +` + +func (q *Queries) DeleteReleaseGroupRecordingsByRecording(ctx context.Context, recordingID int64) error { + _, err := q.db.ExecContext(ctx, deleteReleaseGroupRecordingsByRecording, recordingID) + return err +} + const getRecordingReleaseGroups = `-- name: GetRecordingReleaseGroups :many SELECT id, release_group_id, recording_id, track_number, disc_number FROM release_group_recordings WHERE recording_id = ? diff --git a/backend/database/sql/sqlcgen/release_groups.sql.go b/backend/database/sql/sqlcgen/release_groups.sql.go index 5e12cb2..f1c75ec 100644 --- a/backend/database/sql/sqlcgen/release_groups.sql.go +++ b/backend/database/sql/sqlcgen/release_groups.sql.go @@ -469,6 +469,38 @@ func (q *Queries) GetAllReleaseGroups(ctx context.Context) ([]ReleaseGroup, erro return items, nil } +const getOrphanedReleaseGroupIDs = `-- name: GetOrphanedReleaseGroupIDs :many +SELECT rg.id FROM release_groups rg +LEFT JOIN release_group_recordings rgr ON rgr.release_group_id = rg.id +WHERE rgr.id IS NULL +` + +// Release groups with no recordings left in them - run after orphaned +// recordings (and their release_group_recordings rows) are deleted, so +// a release group whose last owned track was removed is cleaned up too. +func (q *Queries) GetOrphanedReleaseGroupIDs(ctx context.Context) ([]int64, error) { + rows, err := q.db.QueryContext(ctx, getOrphanedReleaseGroupIDs) + if err != nil { + return nil, err + } + defer rows.Close() + var items []int64 + for rows.Next() { + var id int64 + if err := rows.Scan(&id); err != nil { + return nil, err + } + items = append(items, id) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const getReleaseGroup = `-- name: GetReleaseGroup :one SELECT id, name, cover_art_id, album_artist_credit_id, year, total_tracks, total_discs, mbid, original_year FROM release_groups WHERE id = ? LIMIT 1 diff --git a/backend/database/sql/sqlcgen/tagging_items.sql.go b/backend/database/sql/sqlcgen/tagging_items.sql.go index 514b391..ba2db17 100644 --- a/backend/database/sql/sqlcgen/tagging_items.sql.go +++ b/backend/database/sql/sqlcgen/tagging_items.sql.go @@ -136,7 +136,8 @@ SELECT ti.score, ti.last_checked_at, ti.status, - ti.created_at + ti.created_at, + ti.synthetic FROM tagging_items ti LEFT JOIN libraries lb ON lb.id = ti.library_id WHERE ti.group_key = ? @@ -158,6 +159,7 @@ type GetPendingFolderDetailRow struct { LastCheckedAt sql.NullTime Status string CreatedAt time.Time + Synthetic int64 } func (q *Queries) GetPendingFolderDetail(ctx context.Context, groupKey string) (GetPendingFolderDetailRow, error) { @@ -178,6 +180,7 @@ func (q *Queries) GetPendingFolderDetail(ctx context.Context, groupKey string) ( &i.LastCheckedAt, &i.Status, &i.CreatedAt, + &i.Synthetic, ) return i, err } @@ -197,7 +200,7 @@ func (q *Queries) GetRecordingReleaseGroupID(ctx context.Context, recordingID in } const getTaggingItem = `-- name: GetTaggingItem :one -SELECT group_key, library_id, track_count, album_name, album_artist, disc_number, best_match_release_mbid, score, last_checked_at, status, cleared_at, created_at FROM tagging_items +SELECT group_key, library_id, track_count, album_name, album_artist, disc_number, best_match_release_mbid, score, last_checked_at, status, cleared_at, created_at, synthetic, parent_group_key FROM tagging_items WHERE group_key = ? LIMIT 1 ` @@ -218,6 +221,8 @@ func (q *Queries) GetTaggingItem(ctx context.Context, groupKey string) (TaggingI &i.Status, &i.ClearedAt, &i.CreatedAt, + &i.Synthetic, + &i.ParentGroupKey, ) return i, err } @@ -233,10 +238,15 @@ SELECT COALESCE(r.disc_number, 0) AS disc_number, COALESCE(r.name, '') AS title, COALESCE(ac.text, '') AS artist_name, - COALESCE(r.mbid, '') AS recording_mbid + COALESCE(r.mbid, '') AS recording_mbid, + COALESCE(rg.name, '') AS album_name, + COALESCE(rgac.text, '') AS album_artist FROM audio_files af LEFT JOIN recordings r ON af.recording_id = r.id LEFT JOIN artist_credit ac ON r.artist_credit_id = ac.id +LEFT JOIN release_group_recordings rgr ON rgr.recording_id = r.id +LEFT JOIN release_groups rg ON rg.id = rgr.release_group_id +LEFT JOIN artist_credit rgac ON rg.album_artist_credit_id = rgac.id WHERE af.group_key = ? ORDER BY COALESCE(r.disc_number, 0), COALESCE(r.track_number, 0), @@ -254,8 +264,14 @@ type ListAudioFilesInTaggingGroupRow struct { Title string ArtistName string RecordingMbid string + AlbumName string + AlbumArtist string } +// album_name/album_artist are the PER-TRACK tags (via each track's +// own release_group link), not the folder-level tagging_items +// values. SplitMixedFolder clusters on these to find sub-albums +// hiding inside a folder full of unrelated tracks. func (q *Queries) ListAudioFilesInTaggingGroup(ctx context.Context, groupKey string) ([]ListAudioFilesInTaggingGroupRow, error) { rows, err := q.db.QueryContext(ctx, listAudioFilesInTaggingGroup, groupKey) if err != nil { @@ -276,6 +292,8 @@ func (q *Queries) ListAudioFilesInTaggingGroup(ctx context.Context, groupKey str &i.Title, &i.ArtistName, &i.RecordingMbid, + &i.AlbumName, + &i.AlbumArtist, ); err != nil { return nil, err } @@ -290,6 +308,56 @@ func (q *Queries) ListAudioFilesInTaggingGroup(ctx context.Context, groupKey str return items, nil } +const listLikelyMixedBagGroupKeys = `-- name: ListLikelyMixedBagGroupKeys :many +SELECT ti.group_key +FROM tagging_items ti +JOIN audio_files af ON af.group_key = ti.group_key +LEFT JOIN recordings r ON af.recording_id = r.id +LEFT JOIN artist_credit ac ON r.artist_credit_id = ac.id +LEFT JOIN release_group_recordings rgr ON rgr.recording_id = r.id +LEFT JOIN release_groups rg ON rg.id = rgr.release_group_id +WHERE ti.synthetic = 0 + AND ti.track_count >= 4 + AND ( + ti.album_artist = '' + OR LOWER(TRIM(ti.album_artist)) IN ('various artists', 'various', 'va', 'v.a.', 'v a', 'unknown') + ) +GROUP BY ti.group_key +HAVING COUNT(DISTINCT CASE WHEN ac.text != '' THEN LOWER(TRIM(ac.text)) END) > 1 + AND COUNT(DISTINCT CASE WHEN rg.name != '' THEN LOWER(TRIM(rg.name)) END) > 1 +` + +// Cheap, whole-library triage pass for autotag.IsMixedBag: one +// grouped scan over audio_files (indexed on group_key) rather than +// hydrating every group's full track list in Go. LOWER/TRIM is an +// approximation of autotag.Normalize (no unicode fold, no qualifier +// stripping) so this can flag a false positive Normalize would +// clear, or miss a true one Normalize would catch. Treat it as a +// triage filter for which groups are worth a real autotag. +// IsMixedBag check, or a badge at minimum, not the final word. +func (q *Queries) ListLikelyMixedBagGroupKeys(ctx context.Context) ([]string, error) { + rows, err := q.db.QueryContext(ctx, listLikelyMixedBagGroupKeys) + if err != nil { + return nil, err + } + defer rows.Close() + var items []string + for rows.Next() { + var group_key string + if err := rows.Scan(&group_key); err != nil { + return nil, err + } + items = append(items, group_key) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listLocalReleaseGroupCandidates = `-- name: ListLocalReleaseGroupCandidates :many SELECT rg.id AS release_group_id, @@ -552,7 +620,8 @@ SELECT ti.score, ti.last_checked_at, ti.status, - ti.created_at + ti.created_at, + ti.synthetic FROM tagging_items ti LEFT JOIN libraries lb ON lb.id = ti.library_id WHERE (CAST(?1 AS INTEGER) = 0 OR ti.library_id = ?1) @@ -584,6 +653,7 @@ type ListPendingTaggingItemsByScoreRow struct { LastCheckedAt sql.NullTime Status string CreatedAt time.Time + Synthetic int64 } func (q *Queries) ListPendingTaggingItemsByScore(ctx context.Context, arg ListPendingTaggingItemsByScoreParams) ([]ListPendingTaggingItemsByScoreRow, error) { @@ -615,6 +685,7 @@ func (q *Queries) ListPendingTaggingItemsByScore(ctx context.Context, arg ListPe &i.LastCheckedAt, &i.Status, &i.CreatedAt, + &i.Synthetic, ); err != nil { return nil, err } @@ -629,6 +700,49 @@ func (q *Queries) ListPendingTaggingItemsByScore(ctx context.Context, arg ListPe return items, nil } +const markTaggingItemSynthetic = `-- name: MarkTaggingItemSynthetic :exec +UPDATE tagging_items +SET synthetic = 1, + parent_group_key = ? +WHERE group_key = ? +` + +type MarkTaggingItemSyntheticParams struct { + ParentGroupKey string + GroupKey string +} + +// Stamps a group as carved out of parent_group_key by +// SplitMixedFolder. Idempotent: safe to call every time a track +// is migrated into the synthetic group, not just on first creation. +func (q *Queries) MarkTaggingItemSynthetic(ctx context.Context, arg MarkTaggingItemSyntheticParams) error { + _, err := q.db.ExecContext(ctx, markTaggingItemSynthetic, arg.ParentGroupKey, arg.GroupKey) + return err +} + +const pruneOrphanedTaggingItems = `-- name: PruneOrphanedTaggingItems :exec +DELETE FROM tagging_items +WHERE NOT EXISTS ( + SELECT 1 FROM audio_files af WHERE af.group_key = tagging_items.group_key +) +` + +// Self-healing sweep for rows whose track_count bookkeeping (scan +// orphan cleanup, maybeRebindTaggingGroup, SplitMixedFolder) never +// ran or drifted: a cancelled scan, a library move/rename the +// SoftScanAllLibraries disk-count/mtime heuristic did not catch, or +// a decrement that landed without its paired delete. Rather than +// trust track_count, this checks the ground truth directly: any +// group_key no audio_files row still points at is gone, and its +// tagging_items row (and cascaded tagging_candidates) should be too. +// Cheap: one indexed (idx_audio_files_group_key) existence check per +// row. Called opportunistically wherever the pending list is read, +// so stale entries cannot linger indefinitely between full rescans. +func (q *Queries) PruneOrphanedTaggingItems(ctx context.Context) error { + _, err := q.db.ExecContext(ctx, pruneOrphanedTaggingItems) + return err +} + const setAudioFileTagStatus = `-- name: SetAudioFileTagStatus :exec UPDATE audio_files SET tag_status = ? WHERE id = ? ` diff --git a/backend/datamap/datamap.go b/backend/datamap/datamap.go index fbe5ed1..fd6b646 100644 --- a/backend/datamap/datamap.go +++ b/backend/datamap/datamap.go @@ -268,6 +268,13 @@ var tables = []Table{ Name: "release_to_rg", Kind: Cache, Lifetime: Retained, Note: "Release to release-group mapping from the dump.", }, + { + Name: "schema_migrations", Kind: Derived, Lifetime: Retained, + Note: "Bookkeeping for sql/migrations: which numbered files have " + + "run. Safe to lose — replaying an already-applied migration " + + "tolerates its ALTER TABLE ADD COLUMN as a no-op and just " + + "re-records it.", + }, { Name: "search_clicks", Kind: Authored, Lifetime: Retained, Note: "Which results the user picked, used to rank future " + diff --git a/backend/download/importer.go b/backend/download/importer.go index 00e7c71..ee53af7 100644 --- a/backend/download/importer.go +++ b/backend/download/importer.go @@ -56,11 +56,17 @@ type TagWriterPort interface { type LibraryPort interface { // ScanLibrary triggers a rescan so imported files are ingested. ScanLibrary(id int64) error + + // LibraryPath resolves a library's root directory by id. + LibraryPath(id int64) (string, error) } // ImportOptions configures how imported files are laid out. type ImportOptions struct { // LibraryRoot is the directory imported files are placed under. + // Resolved per-request from the request's LibraryID — never a + // fixed, app-wide directory, since a user can have several + // libraries. LibraryRoot string // PathTemplate lays out the destination path. Supported tokens: diff --git a/backend/download/importer_test.go b/backend/download/importer_test.go index de2a394..2f041ce 100644 --- a/backend/download/importer_test.go +++ b/backend/download/importer_test.go @@ -46,6 +46,7 @@ func (r *recordingTagWriter) WriteUntrackedFileTags( type stubLibrary struct { mu sync.Mutex scanned []int64 + path string } func (s *stubLibrary) ScanLibrary(id int64) error { @@ -57,6 +58,13 @@ func (s *stubLibrary) ScanLibrary(id int64) error { return nil } +func (s *stubLibrary) LibraryPath(int64) (string, error) { + s.mu.Lock() + defer s.mu.Unlock() + + return s.path, nil +} + // importFixture stages a set of files and returns the pieces an import // needs. type importFixture struct { diff --git a/backend/download/manager.go b/backend/download/manager.go index 67b2f6e..ff55142 100644 --- a/backend/download/manager.go +++ b/backend/download/manager.go @@ -790,6 +790,14 @@ func (m *Manager) grab( opts := m.importOptions() opts.WriteTags = true + opts.LibraryRoot, err = m.library.LibraryPath(req.LibraryID) + if err != nil { + m.failItem(ctx, job, item, req.ID, + fmt.Errorf("resolve library root: %w", err)) + + return + } + imported, err = m.importer.Import(ctx, req, result, opts) if err != nil { m.failItem(ctx, job, item, req.ID, err) diff --git a/backend/download/manager_test.go b/backend/download/manager_test.go index be27c4b..d583989 100644 --- a/backend/download/manager_test.go +++ b/backend/download/manager_test.go @@ -31,15 +31,15 @@ func newManagerFixture(t *testing.T) managerFixture { store := NewStore(db) staging := newTestStaging(t) tags := newRecordingTagWriter() - lib := &stubLibrary{} root := t.TempDir() + lib := &stubLibrary{path: root} imp := NewImporter(slogDiscard(), staging, tags, lib) m := NewManager( slogDiscard(), store, NewMemSecretStore(), staging, imp, lib, ) - m.SetImportOptions(ImportOptions{LibraryRoot: root}) + m.SetImportOptions(ImportOptions{}) return managerFixture{ manager: m, diff --git a/backend/download/provider.go b/backend/download/provider.go index f753735..3532ade 100644 --- a/backend/download/provider.go +++ b/backend/download/provider.go @@ -130,6 +130,12 @@ type Config struct { Enabled bool `json:"enabled"` Priority int `json:"priority"` Settings map[string]string `json:"settings"` + + // SetSecrets names which of the descriptor's secret fields already + // have a stored value, without exposing it. Populated only when a + // Config is built for the frontend (see Service.withSecretFlags); + // empty when read from or written to the store. + SetSecrets map[string]bool `json:"setSecrets,omitempty"` } // Setting returns a config value, or fallback when unset. @@ -197,6 +203,11 @@ type Field struct { // provider config row, and rendered as a password input. Secret bool `json:"secret"` + // Path marks a value that names a local filesystem directory, so + // the settings form can offer a native folder picker beside the + // text input rather than making the user type or paste it. + Path bool `json:"path"` + Required bool `json:"required"` Default string `json:"default,omitempty"` } diff --git a/backend/download/provider_slskd.go b/backend/download/provider_slskd.go index c07ad36..a78191d 100644 --- a/backend/download/provider_slskd.go +++ b/backend/download/provider_slskd.go @@ -10,6 +10,8 @@ import ( "path/filepath" "strings" "time" + + "github.com/google/uuid" ) // Soulseek is reached through a user-run slskd daemon rather than the @@ -54,8 +56,14 @@ const ( // slskdSearchWait bounds a single search. Soulseek searches return // results progressively; waiting the full budget gets noticeably - // more peers than bailing at the first response. - slskdSearchWait = 12 * time.Second + // more peers than bailing at the first response. 12s was measured + // to miss real, available peers on real-world queries (roughly 4 of + // 5 attempts for a live search came back empty before this many + // responses had a chance to arrive), so this is generous rather + // than tight. Kept a few seconds under Manager's per-provider + // searchTimeout (25s) so the request/cleanup round-trips around it + // do not get cut off by the context deadline. + slskdSearchWait = 20 * time.Second // slskdTransferPoll is how often transfer state is polled. slskdTransferPoll = 3 * time.Second @@ -102,6 +110,7 @@ func init() { Key: "downloadsPath", Label: "slskd downloads folder", Placeholder: "/var/lib/slskd/downloads", + Path: true, Required: true, Help: "The folder slskd saves to, as this machine sees it. " + "If slskd runs elsewhere, this must be a mounted share.", @@ -270,7 +279,11 @@ func (t slskdTransfer) done() (finished, ok bool) { // actually wants: Soulseek has no album concept, but people organise // their shares by album directory. func (s *slskd) Search(ctx context.Context, req Request) ([]Candidate, error) { - searchID := newID() + // slskd's search endpoint deserializes id as a .NET Guid server-side, + // so it must be a dashed UUID — the app's own newID() (a plain hex + // string, used for request/item IDs elsewhere) is rejected with an + // HTTP 400 before any search happens. + searchID := uuid.NewString() body := map[string]any{ "id": searchID, diff --git a/backend/download/service.go b/backend/download/service.go index ff89344..83d8ebd 100644 --- a/backend/download/service.go +++ b/backend/download/service.go @@ -2,6 +2,7 @@ package download import ( "context" + "errors" "fmt" "log/slog" "strconv" @@ -11,6 +12,12 @@ import ( "yellowjacket/backend/events" ) +// ErrNoLibrary means the request did not name a library to attach the +// download to. Letting it through would hit the download_requests / +// download_wants foreign key on library_id and surface as a raw SQLite +// error, so it is rejected here with a message the UI can show. +var ErrNoLibrary = errors.New("no library selected") + // Service is the frontend-facing surface of the download subsystem. // Its methods are bound into Wails and called from TypeScript, so // signatures use plain types and return errors the UI can render. @@ -71,7 +78,39 @@ func (s *Service) ProviderKinds() []Descriptor { // ListProviders returns the user's configured download clients. func (s *Service) ListProviders() ([]Config, error) { - return s.store.ListProviders(context.Background()) + cfgs, err := s.store.ListProviders(context.Background()) + if err != nil { + return nil, err + } + + for i := range cfgs { + cfgs[i].SetSecrets = s.setSecretFlags(cfgs[i]) + } + + return cfgs, nil +} + +// setSecretFlags reports, for each secret field the provider's kind +// declares, whether a value is already stored — so the settings form +// can distinguish an unset secret from one it simply isn't shown. +func (s *Service) setSecretFlags(cfg Config) map[string]bool { + desc, ok := DescriptorFor(cfg.Kind) + if !ok { + return nil + } + + flags := map[string]bool{} + + for _, field := range desc.Fields { + if !field.Secret { + continue + } + + v, err := s.secrets.Get(cfg.ID, field.Key) + flags[field.Key] = err == nil && v != "" + } + + return flags } // AddProvider creates a provider and stores any secret settings @@ -248,6 +287,10 @@ type StartResult struct { // Start searches for a release and either auto-picks a clear winner or // returns ranked candidates for the user to choose from. func (s *Service) Start(req SearchRequest) (StartResult, error) { + if req.LibraryID <= 0 { + return StartResult{}, ErrNoLibrary + } + r := Request{ ID: newID(), LibraryID: req.LibraryID, @@ -397,6 +440,10 @@ type WantRequest struct { // pass, so the user sees something happen rather than waiting six hours // for the next scheduled one. func (s *Service) AddWant(req WantRequest) (int64, error) { + if req.LibraryID <= 0 { + return 0, ErrNoLibrary + } + entity := Entity(req.Entity) if !entity.Valid() { return 0, fmt.Errorf("%w: entity %q", ErrUnsupported, req.Entity) diff --git a/backend/download/types.go b/backend/download/types.go index d43ea08..5472de7 100644 --- a/backend/download/types.go +++ b/backend/download/types.go @@ -134,14 +134,48 @@ func (r Request) Anchored() bool { } // SearchText returns the string to hand a provider's search endpoint. +// +// Album titles routinely start with the artist name — self-titled +// albums ("Boston" / "Boston") and titles like "Blank Banshee 0" both +// do — so naively concatenating Artist and Album would search for +// "Blank Banshee Blank Banshee 0". That repeated term is enough to +// return zero results on providers that expect every term to appear +// in a match (Soulseek in particular), so the artist is dropped when +// the album title already leads with it. func (r Request) SearchText() string { if r.Query != "" { return r.Query } + if r.Artist != "" && albumLeadsWithArtist(r.Artist, r.Album) { + return strings.TrimSpace(r.Album) + } + return strings.TrimSpace(r.Artist + " " + r.Album) } +// albumLeadsWithArtist reports whether album starts with artist as a +// whole word, case-insensitively, so it is safe to drop the artist +// from a combined query without losing a real search term. A plain +// substring check would misfire on cases like artist "Air" against +// album "Repair". +func albumLeadsWithArtist(artist, album string) bool { + a, b := strings.ToLower(strings.TrimSpace(artist)), strings.ToLower(strings.TrimSpace(album)) + if a == "" || !strings.HasPrefix(b, a) { + return false + } + + rest := b[len(a):] + + return rest == "" || !isWordChar(rune(rest[0])) +} + +// isWordChar reports whether r continues a word for the purposes of +// albumLeadsWithArtist's boundary check. +func isWordChar(r rune) bool { + return (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') +} + // ExpectedTrack is one track of the release the user asked for. type ExpectedTrack struct { Position int `json:"position"` diff --git a/backend/download/types_test.go b/backend/download/types_test.go new file mode 100644 index 0000000..fc96e93 --- /dev/null +++ b/backend/download/types_test.go @@ -0,0 +1,55 @@ +package download + +import "testing" + +func TestRequest_SearchText(t *testing.T) { + tests := []struct { + name string + req Request + want string + }{ + { + name: "query overrides everything", + req: Request{Artist: "Blank Banshee", Album: "0", Query: "raw text"}, + want: "raw text", + }, + { + name: "ordinary album keeps artist and album", + req: Request{Artist: "Pink Floyd", Album: "The Wall"}, + want: "Pink Floyd The Wall", + }, + { + name: "album title leads with artist name", + req: Request{Artist: "Blank Banshee", Album: "Blank Banshee 0"}, + want: "Blank Banshee 0", + }, + { + name: "self-titled album", + req: Request{Artist: "Boston", Album: "Boston"}, + want: "Boston", + }, + { + name: "artist name as a substring, not a word prefix", + req: Request{Artist: "Air", Album: "Repair"}, + want: "Air Repair", + }, + { + name: "case-insensitive match", + req: Request{Artist: "blank banshee", Album: "BLANK BANSHEE 0"}, + want: "BLANK BANSHEE 0", + }, + { + name: "no artist", + req: Request{Album: "Compilation"}, + want: "Compilation", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.req.SearchText(); got != tt.want { + t.Errorf("SearchText() = %q, want %q", got, tt.want) + } + }) + } +} diff --git a/backend/explore/autotagclient.go b/backend/explore/autotagclient.go index f55db38..c406628 100644 --- a/backend/explore/autotagclient.go +++ b/backend/explore/autotagclient.go @@ -6,18 +6,20 @@ import ( "yellowjacket/backend/autotag" ) -// AutotagClient adapts *MusicBrainzClient to autotag.MBClient. +// AutotagClient adapts a *Service (its network MusicBrainzClient +// plus its offline dump-derived SearchIndex) to autotag.MBClient. // Lives in the explore package so autotag stays free of // explore-internal types — callers in app wiring construct one via // NewAutotagClient and hand it to the scorer. type AutotagClient struct { - inner *MusicBrainzClient + svc *Service } -// NewAutotagClient wraps a MusicBrainzClient for use by the -// autotag scorer. -func NewAutotagClient(inner *MusicBrainzClient) *AutotagClient { - return &AutotagClient{inner: inner} +// NewAutotagClient wraps an explore Service for use by the autotag +// scorer: svc.MusicBrainz() serves the network cascade, svc's local +// SearchIndex serves the index-first pass. +func NewAutotagClient(svc *Service) *AutotagClient { + return &AutotagClient{svc: svc} } // SearchReleaseGroups delegates to the wrapped client and projects @@ -25,7 +27,7 @@ func NewAutotagClient(inner *MusicBrainzClient) *AutotagClient { func (c *AutotagClient) SearchReleaseGroups( ctx context.Context, query string, limit int, ) ([]autotag.MBReleaseGroupHit, int, error) { - hits, total, err := c.inner.SearchReleaseGroups(ctx, query, limit) + hits, total, err := c.svc.mb.SearchReleaseGroups(ctx, query, limit) if err != nil { return nil, 0, err } @@ -44,13 +46,45 @@ func (c *AutotagClient) SearchReleaseGroups( return out, total, nil } +// SearchReleaseGroupsLocal searches the offline dump-derived catalog +// (explore_index) for release groups matching albumName — no network +// round-trip. Returns ok=false when the index isn't populated yet, +// telling the resolver to rely on the network cascade alone. +func (c *AutotagClient) SearchReleaseGroupsLocal( + ctx context.Context, albumName string, limit int, +) ([]autotag.MBReleaseGroupHit, bool) { + if !c.svc.index.IsReady() { + return nil, false + } + + hits := c.svc.index.Search(ctx, albumName, limit) + + out := make([]autotag.MBReleaseGroupHit, 0, len(hits)) + + for _, h := range hits { + if h.EntityType != "release_group" { + continue + } + + out = append(out, autotag.MBReleaseGroupHit{ + MBID: h.MBID, + Title: h.Title, + ArtistCredit: h.ArtistName, + FirstDate: h.ReleaseDate, + PrimaryType: h.PrimaryType, + }) + } + + return out, true +} + // BrowseReleases delegates to the wrapped client and projects each // release (and its tracks) into autotag's shape. Length is // millisecond-aligned to match local audio_files. func (c *AutotagClient) BrowseReleases( ctx context.Context, releaseGroupMBID string, ) ([]autotag.MBRelease, error) { - releases, err := c.inner.BrowseReleases(ctx, releaseGroupMBID) + releases, err := c.svc.mb.BrowseReleases(ctx, releaseGroupMBID) if err != nil { return nil, err } @@ -68,7 +102,7 @@ func (c *AutotagClient) BrowseReleases( func (c *AutotagClient) LookupRelease( ctx context.Context, releaseMBID string, ) (autotag.MBRelease, error) { - rel, err := c.inner.LookupRelease(ctx, releaseMBID) + rel, err := c.svc.mb.LookupRelease(ctx, releaseMBID) if err != nil { return autotag.MBRelease{}, err } @@ -81,7 +115,7 @@ func (c *AutotagClient) LookupRelease( func (c *AutotagClient) LookupReleaseGroup( ctx context.Context, releaseGroupMBID string, ) (autotag.MBReleaseGroupHit, error) { - rg, err := c.inner.LookupReleaseGroup(ctx, releaseGroupMBID) + rg, err := c.svc.mb.LookupReleaseGroup(ctx, releaseGroupMBID) if err != nil { return autotag.MBReleaseGroupHit{}, err } @@ -101,7 +135,7 @@ func (c *AutotagClient) LookupReleaseGroup( func (c *AutotagClient) SearchRecordings( ctx context.Context, query string, limit int, ) ([]autotag.MBRecordingHit, int, error) { - recs, total, err := c.inner.SearchRecordings(ctx, query, limit) + recs, total, err := c.svc.mb.SearchRecordings(ctx, query, limit) if err != nil { return nil, 0, err } @@ -125,7 +159,7 @@ func (c *AutotagClient) SearchRecordings( func (c *AutotagClient) LookupRecordingReleases( ctx context.Context, recordingMBID string, ) ([]autotag.MBReleaseRef, error) { - refs, err := c.inner.LookupRecordingReleases(ctx, recordingMBID) + refs, err := c.svc.mb.LookupRecordingReleases(ctx, recordingMBID) if err != nil { return nil, err } diff --git a/backend/explore/explore.go b/backend/explore/explore.go index 37ff06c..d995e60 100644 --- a/backend/explore/explore.go +++ b/backend/explore/explore.go @@ -2595,15 +2595,40 @@ func (e *Service) computeIntentPrior( // have for "the user means this artist". Scale by listener // count so a popular exact match dominates and an obscure // one doesn't move the needle. + // + // As above, a Title match is evidence for the entity's own + // category; an ArtistName match is evidence for the artist + // category specifically, even when the matching row is a + // release_group/recording — otherwise an artist's own catalog + // entries in the local index would outvote the artist itself. + // Take the strongest boost per target instead of multiplying + // once per matching row. + localOwnCategoryBoost := map[string]float64{} + + localArtistNameBoost := 0.0 + for _, m := range exactMatches { - if !isExactNameMatch(q, m.Title) && !isExactNameMatch(q, m.ArtistName) { + titleMatch := isExactNameMatch(q, m.Title) + artistMatch := isExactNameMatch(q, m.ArtistName) + + if !titleMatch && !artistMatch { continue } // Confidence boost scales with log listener count. boost := 1.0 + 1.5*normLog(m.ListenerCount) //nolint:mnd - switch m.EntityType { + if titleMatch && boost > localOwnCategoryBoost[m.EntityType] { + localOwnCategoryBoost[m.EntityType] = boost + } + + if artistMatch && boost > localArtistNameBoost { + localArtistNameBoost = boost + } + } + + for cat, boost := range localOwnCategoryBoost { + switch cat { case "artist": weights.artist *= boost case "release_group": @@ -2613,12 +2638,31 @@ func (e *Service) computeIntentPrior( } } + if localArtistNameBoost > 0 { + weights.artist *= localArtistNameBoost + } + // Signal: exact-match candidates discovered in the MB result // list (Source 1b/2b/3b in gatherTopCandidates). These cover // the case where the local index doesn't have the entity but // MB does — e.g. Blue October's "Calling You" when Blue // October isn't yet a known artist. Same scaling as // index-sourced exact matches. + // + // A title match is evidence for that candidate's own category. + // An artist-credit match is evidence for the *artist* category + // specifically, regardless of what kind of entity carries the + // credit — searching an artist's exact name naturally surfaces + // their whole discography in the release/recording pools, and + // crediting each of those to "album"/"recording" would drown + // out the one true artist candidate. Take the strongest boost + // per target rather than multiplying once per matching item, so + // an artist with many releases doesn't compound the signal. + var ( + ownCategoryBoost = map[string]float64{} + artistCreditBoost float64 + ) + for _, c := range exactCandidates { var listeners int @@ -2633,7 +2677,17 @@ func (e *Service) computeIntentPrior( boost := 1.0 + 1.0*normLog(listeners) //nolint:mnd - switch c.category { + if isExactNameMatch(q, c.topResult.Name) && boost > ownCategoryBoost[c.category] { + ownCategoryBoost[c.category] = boost + } + + if isExactNameMatch(q, c.topResult.ArtistCredit) && boost > artistCreditBoost { + artistCreditBoost = boost + } + } + + for cat, boost := range ownCategoryBoost { + switch cat { case "artist": weights.artist *= boost case "release_group": @@ -2643,6 +2697,10 @@ func (e *Service) computeIntentPrior( } } + if artistCreditBoost > 0 { + weights.artist *= artistCreditBoost + } + // Signal: many recordings in the result list with the same // title as the query → cover-wave pattern → strong recording. titleMatches := 0 diff --git a/backend/explore/intentprior_test.go b/backend/explore/intentprior_test.go new file mode 100644 index 0000000..5f0faba --- /dev/null +++ b/backend/explore/intentprior_test.go @@ -0,0 +1,113 @@ +package explore + +import "testing" + +// TestComputeIntentPrior_ArtistNameNotDrownedByOwnCatalog reproduces a bug +// where searching an artist's exact name (e.g. "blank banshee") failed to +// surface the artist in "top results", even though it appeared correctly +// in the dedicated Artists section. The cause: every album/track by that +// artist in the candidate pool also has ArtistCredit == query, and each +// one multiplied the album/recording category weight, drowning out the +// single true artist candidate which only boosted its own category once. +func TestComputeIntentPrior_ArtistNameNotDrownedByOwnCatalog(t *testing.T) { + svc := &Service{} + q := "blank banshee" + + result := &MBSearchResult{ + Artists: []MBArtist{ + {MBID: "artist-1", Name: "Blank Banshee", ListenerCount: 50000}, + }, + ReleaseGroups: []MBReleaseGroup{ + { + MBID: "rg-1", + Title: "Blank Banshee 0", + ArtistCredit: "Blank Banshee", + ListenerCount: 20000, + }, + { + MBID: "rg-2", + Title: "Blank Banshee 1", + ArtistCredit: "Blank Banshee", + ListenerCount: 18000, + }, + { + MBID: "rg-3", + Title: "Blank Banshee 1.5", + ArtistCredit: "Blank Banshee", + ListenerCount: 15000, + }, + }, + Recordings: []MBRecording{ + { + MBID: "rec-1", + Title: "Teen Pregnancy", + ArtistCredit: "Blank Banshee", + ListenerCount: 12000, + }, + {MBID: "rec-2", Title: "Chase", ArtistCredit: "Blank Banshee", ListenerCount: 11000}, + {MBID: "rec-3", Title: "Ghost", ArtistCredit: "Blank Banshee", ListenerCount: 9000}, + }, + } + + exactCandidates := []topCandidate{ + {category: "artist", topResult: TopResult{MBID: "artist-1", Name: "Blank Banshee"}}, + { + category: "release_group", + topResult: TopResult{ + MBID: "rg-1", + Name: "Blank Banshee 0", + ArtistCredit: "Blank Banshee", + }, + }, + { + category: "release_group", + topResult: TopResult{ + MBID: "rg-2", + Name: "Blank Banshee 1", + ArtistCredit: "Blank Banshee", + }, + }, + { + category: "release_group", + topResult: TopResult{ + MBID: "rg-3", + Name: "Blank Banshee 1.5", + ArtistCredit: "Blank Banshee", + }, + }, + { + category: "recording", + topResult: TopResult{ + MBID: "rec-1", + Name: "Teen Pregnancy", + ArtistCredit: "Blank Banshee", + }, + }, + { + category: "recording", + topResult: TopResult{MBID: "rec-2", Name: "Chase", ArtistCredit: "Blank Banshee"}, + }, + { + category: "recording", + topResult: TopResult{MBID: "rec-3", Name: "Ghost", ArtistCredit: "Blank Banshee"}, + }, + } + + prior := svc.computeIntentPrior(q, result, nil, exactCandidates) + + if prior.artist <= prior.album { + t.Errorf( + "expected artist prior (%v) > album prior (%v) for an exact artist-name search", + prior.artist, + prior.album, + ) + } + + if prior.artist <= prior.recording { + t.Errorf( + "expected artist prior (%v) > recording prior (%v) for an exact artist-name search", + prior.artist, + prior.recording, + ) + } +} diff --git a/backend/explore/prune_test.go b/backend/explore/prune_test.go new file mode 100644 index 0000000..9387be7 --- /dev/null +++ b/backend/explore/prune_test.go @@ -0,0 +1,156 @@ +package explore + +import ( + "log/slog" + "testing" + + "yellowjacket/backend/database" + "yellowjacket/backend/database/sql/sqlcgen" +) + +// TestPruneStaleLocalCrossReferences verifies that an explore_index row +// whose local_*_id points at a library row that no longer exists gets its +// in_library flag and local_*_id cleared, while a row still backed by a +// real library entity is left untouched. This is the fix for stale +// "in_library" bookkeeping surviving a rescan that removed the file it +// was tied to (upsertIndexConflictSQL only ever adds these references, +// never clears them). +func TestPruneStaleLocalCrossReferences(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + si := NewSearchIndex(db, nil, nil, slog.Default()) + + // A library artist that still exists. + artist, err := db.Queries.UpsertArtist(t.Context(), "Still Owned") + if err != nil { + t.Fatalf("upsert artist: %v", err) + } + + // Two explore_index artist rows: one pointing at the still-existing + // artist, one pointing at a local ID that has since been deleted + // (simulating a rescan that removed the owning artist). + seedArtist := func(mbid, title string, localID int64) { + t.Helper() + + if _, err := db.ExecContext( + upsertIndexSQL, + "artist", mbid, title, title, mbid, "", + 0, 0, + 0, "", "", + "", "", "", + "", "", "", "", + 1, 0, + localID, 0, 0, + 0, + ); err != nil { + t.Fatalf("seed explore_index row for %q: %v", mbid, err) + } + } + + seedArtist("still-owned-mbid", "Still Owned", artist.ID) + seedArtist("removed-mbid", "Removed Artist", 999999) + + si.pruneStaleLocalCrossReferences() + + stillOwned := si.LookupArtistByMBID("still-owned-mbid") + if stillOwned == nil { + t.Fatal("expected still-owned artist row to survive pruning") + } + + if !stillOwned.InLibrary || stillOwned.LocalArtistID != artist.ID { + t.Errorf( + "still-owned artist: InLibrary=%v LocalArtistID=%d, want InLibrary=true LocalArtistID=%d", + stillOwned.InLibrary, + stillOwned.LocalArtistID, + artist.ID, + ) + } + + removed := si.LookupArtistByMBID("removed-mbid") + if removed == nil { + t.Fatal("expected removed-artist row to still exist (only cross-references cleared)") + } + + if removed.InLibrary || removed.LocalArtistID != 0 { + t.Errorf( + "removed artist: InLibrary=%v LocalArtistID=%d, want InLibrary=false LocalArtistID=0", + removed.InLibrary, removed.LocalArtistID, + ) + } +} + +// TestUnenrichedLibraryArtistMBIDs_OrdersByOwnedTrackCount verifies the +// backfill queue prioritizes artists by how many tracks the user actually +// owns, not by how many duplicate-mbid artist rows happen to exist (the +// previous "ORDER BY COUNT(*)" grouped on a.mbid, which is nearly always 1 +// per artist and so wasn't really ordering by anything meaningful). +func TestUnenrichedLibraryArtistMBIDs_OrdersByOwnedTrackCount(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + si := NewSearchIndex(db, nil, nil, slog.Default()) + q := db.Queries + ctx := t.Context() + + seedArtistWithTracks := func(name, mbid string, trackCount int) { + t.Helper() + + artist, err := q.UpsertArtist(ctx, name) + if err != nil { + t.Fatalf("upsert artist %q: %v", name, err) + } + + _, err = db.ExecContext("UPDATE artists SET mbid = ? WHERE id = ?", mbid, artist.ID) + if err != nil { + t.Fatalf("set mbid for %q: %v", name, err) + } + + ac, err := q.UpsertArtistCredit(ctx, name) + if err != nil { + t.Fatalf("upsert artist credit %q: %v", name, err) + } + + if _, err := q.CreateArtistCreditArtist(ctx, sqlcgen.CreateArtistCreditArtistParams{ + ArtistID: artist.ID, + CreditID: ac.ID, + }); err != nil { + t.Fatalf("link artist credit artist %q: %v", name, err) + } + + for i := range trackCount { + rec, err := q.CreateRecordingFull(ctx, sqlcgen.CreateRecordingFullParams{ + Name: name, + ArtistCreditID: ac.ID, + }) + if err != nil { + t.Fatalf("create recording for %q: %v", name, err) + } + + if _, err := q.CreateAudioFile(ctx, sqlcgen.CreateAudioFileParams{ + FilePath: name + "/" + string(rune('a'+i)) + ".mp3", + LengthMilliseconds: 180000, + RecordingID: rec.ID, + Basename: string(rune('a'+i)) + ".mp3", + }); err != nil { + t.Fatalf("create audio file for %q: %v", name, err) + } + } + } + + seedArtistWithTracks("Few Tracks", "few-mbid", 1) + seedArtistWithTracks("Many Tracks", "many-mbid", 5) + + mbids := si.unenrichedLibraryArtistMBIDs(10) + if len(mbids) != 2 { + t.Fatalf("unenrichedLibraryArtistMBIDs() = %v, want 2 entries", mbids) + } + + if mbids[0] != "many-mbid" { + t.Errorf( + "unenrichedLibraryArtistMBIDs()[0] = %q, want %q (most owned tracks first)", + mbids[0], + "many-mbid", + ) + } +} diff --git a/backend/explore/searchindex.go b/backend/explore/searchindex.go index 8d9d4a0..69b61e4 100644 --- a/backend/explore/searchindex.go +++ b/backend/explore/searchindex.go @@ -359,10 +359,13 @@ func (si *SearchIndex) unenrichedLibraryArtistMBIDs(limit int) []string { FROM artists a LEFT JOIN explore_index ei ON ei.entity_type = 'artist' AND ei.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) GROUP BY a.mbid - ORDER BY COUNT(*) DESC + ORDER BY COUNT(DISTINCT af.id) DESC LIMIT ? `, limit) if err != nil { @@ -2245,6 +2248,11 @@ func (si *SearchIndex) hasMeta(key string) bool { // MBID-less local content is intentionally excluded — the explore index // is MBID-keyed, and unmatched files are served by the library search. func (si *SearchIndex) PopulateLocalCrossReferences() { + // Clear cross-references for entities no longer owned before adding + // current ones — the upsert below only ever adds/refreshes rows for + // what's currently in the library, so removals need their own pass. + si.pruneStaleLocalCrossReferences() + entries := si.collectLibraryEntities() if len(entries) == 0 { si.logger.Info("library sync: no MB-verified library entities to index") @@ -2267,6 +2275,59 @@ func (si *SearchIndex) PopulateLocalCrossReferences() { si.logger.Info("library sync: upserted library entities into index", "count", len(entries)) } +// pruneStaleLocalCrossReferences clears in_library/local_*_id on +// explore_index rows whose local row no longer exists — e.g. an artist +// whose owned files were swapped out and removed by a rescan. The +// index upsert (upsertIndexConflictSQL) is a one-way ratchet that only +// ever sets these columns, never clears them, so this is the only place +// a removal from the library is ever reflected back into the index. +// The row itself is left in place (it may still be part of the shipped +// catalog, just no longer owned) — only the "this is mine" bookkeeping +// is cleared. +func (si *SearchIndex) pruneStaleLocalCrossReferences() { + type prune struct { + entityType string + column string + table string + } + + for _, p := range []prune{ + {"artist", "local_artist_id", "artists"}, + {"release_group", "local_release_group_id", "release_groups"}, + {"recording", "local_recording_id", "recordings"}, + } { + result, err := si.db.ExecContext( + `UPDATE explore_index + SET in_library = 0, `+p.column+` = NULL + WHERE entity_type = ? + AND `+p.column+` IS NOT NULL + AND `+p.column+` NOT IN (SELECT id FROM `+p.table+`)`, + p.entityType, + ) + if err != nil { + si.logger.Warn( + "library sync: prune stale cross-references failed", + "entityType", + p.entityType, + "error", + err, + ) + + continue + } + + if n, err := result.RowsAffected(); err == nil && n > 0 { + si.logger.Info( + "library sync: cleared stale cross-references", + "entityType", + p.entityType, + "count", + n, + ) + } + } +} + // collectLibraryEntities builds index entries for every library artist, // release group, and recording that carries a MusicBrainz ID. Artist // credit strings and the primary artist MBID are resolved from the local diff --git a/backend/library/crud.go b/backend/library/crud.go index 0540ebb..231b253 100644 --- a/backend/library/crud.go +++ b/backend/library/crud.go @@ -116,6 +116,16 @@ func (l *Library) AddLibrary(path string) (*sqlcgen.Library, error) { return &lib, nil } +// LibraryPath resolves a library's root directory by id. +func (l *Library) LibraryPath(id int64) (string, error) { + lib, err := l.db.ReadQueries.GetLibrary(l.ctx, id) + if err != nil { + return "", fmt.Errorf("could not get library %d: %w", id, err) + } + + return lib.Path, nil +} + // RenameLibrary validates and updates a library's display name. func (l *Library) RenameLibrary(id int64, newName string) error { newName = strings.TrimSpace(newName) diff --git a/backend/library/leak_test.go b/backend/library/leak_test.go index 7cc6aa8..bee12e7 100644 --- a/backend/library/leak_test.go +++ b/backend/library/leak_test.go @@ -16,6 +16,8 @@ var staleTolerated = map[string]string{ "stale entries are filtered by joining track_metadata and are " + "cleared by a full rescan", "lyrics_index": "contentless FTS5, same constraint as search_index", + "schema_migrations": "global migration bookkeeping, not scoped to any " + + "library; removing the only library must not touch it", } // Removing the only library must leave no owned or derived rows behind. diff --git a/backend/library/library.go b/backend/library/library.go index a0de79f..74746de 100644 --- a/backend/library/library.go +++ b/backend/library/library.go @@ -776,6 +776,39 @@ func (l *Library) scanInternal( return true } + // Keep the file's tagging group in sync: drop the group's + // track count and clear it out once empty, mirroring the + // bookkeeping maybeRebindTaggingGroup does for a group_key + // change. Without this, a folder whose files are removed + // and replaced leaves a stale tagging_items row behind — + // its track_count still counts the deleted files, and it + // never clears from the autotag queue. + if audioFile.GroupKey != "" { + if err := l.db.Queries.DecrementTaggingItemTrackCount( + l.ctx, audioFile.GroupKey, + ); err != nil { + l.logger.Warn( + "failed to decrement tagging group for orphan", + "path", path, + "group_key", audioFile.GroupKey, + "err", err, + ) + + metrics.addWarning(path, "orphan", err) + } else if err := l.db.Queries.DeleteTaggingItemIfEmpty( + l.ctx, audioFile.GroupKey, + ); err != nil { + l.logger.Warn( + "failed to clean up emptied tagging group for orphan", + "path", path, + "group_key", audioFile.GroupKey, + "err", err, + ) + + metrics.addWarning(path, "orphan", err) + } + } + // Remove from FTS5 search index. if err := l.db.DeleteSearchIndex( audioFile.ID, @@ -795,6 +828,14 @@ func (l *Library) scanInternal( }) metrics.OrphanCleanup = time.Since(orphanStart) + + // --- Phase 5b: orphaned metadata cleanup --- + // Deleting an audio_files row above doesn't cascade to the + // recording/release_group/artist_credit/artist rows it was the + // last owner of — clean those up too, so a swapped-out artist + // doesn't leave stale rows behind for the Explore index to + // keep pointing at. + l.pruneOrphanedMetadata() } // --- Phase 6: repopulate + resolve phantom playlist tracks --- @@ -944,6 +985,120 @@ func (l *Library) flushStatBackfill( ) } +// pruneOrphanedMetadata removes recording/release_group/artist_credit/ +// artist rows left behind once the audio_files rows that justified them +// are gone — deleting an audio_files row doesn't cascade to any of +// these. Runs in dependency order: recordings first (and their +// release_group_recordings/recording_genres rows), then release groups +// left with no recordings, then artist credits left with no +// recordings/release groups, then artists left with no credits. Best +// effort — logs and continues on error rather than failing the scan. +func (l *Library) pruneOrphanedMetadata() { + tx, err := l.db.BeginTx() + if err != nil { + l.logger.Warn("could not begin orphaned metadata cleanup transaction", "err", err) + + return + } + + defer func() { _ = tx.Rollback() }() // no-op after commit + + txq := l.db.Queries.WithTx(tx) + + recordingIDs, err := txq.GetOrphanedRecordingIDs(l.ctx) + if err != nil { + l.logger.Warn("could not find orphaned recordings", "err", err) + + return + } + + for _, id := range recordingIDs { + if err := txq.DeleteReleaseGroupRecordingsByRecording(l.ctx, id); err != nil { + l.logger.Warn( + "could not delete release group links for orphaned recording", + "id", + id, + "err", + err, + ) + } + + if err := txq.DeleteRecordingGenres(l.ctx, id); err != nil { + l.logger.Warn("could not delete genres for orphaned recording", "id", id, "err", err) + } + + if err := txq.DeleteRecording(l.ctx, id); err != nil { + l.logger.Warn("could not delete orphaned recording", "id", id, "err", err) + } + } + + releaseGroupIDs, err := txq.GetOrphanedReleaseGroupIDs(l.ctx) + if err != nil { + l.logger.Warn("could not find orphaned release groups", "err", err) + + return + } + + for _, id := range releaseGroupIDs { + if err := txq.DeleteReleaseGroup(l.ctx, id); err != nil { + l.logger.Warn("could not delete orphaned release group", "id", id, "err", err) + } + } + + artistCreditIDs, err := txq.GetOrphanedArtistCreditIDs(l.ctx) + if err != nil { + l.logger.Warn("could not find orphaned artist credits", "err", err) + + return + } + + for _, id := range artistCreditIDs { + if err := txq.DeleteArtistCreditArtistByCredit(l.ctx, id); err != nil { + l.logger.Warn( + "could not delete artist links for orphaned artist credit", + "id", + id, + "err", + err, + ) + } + + if err := txq.DeleteArtistCredit(l.ctx, id); err != nil { + l.logger.Warn("could not delete orphaned artist credit", "id", id, "err", err) + } + } + + artistIDs, err := txq.GetOrphanedArtistIDs(l.ctx) + if err != nil { + l.logger.Warn("could not find orphaned artists", "err", err) + + return + } + + for _, id := range artistIDs { + if err := txq.DeleteArtist(l.ctx, id); err != nil { + l.logger.Warn("could not delete orphaned artist", "id", id, "err", err) + } + } + + if err := tx.Commit(); err != nil { + l.logger.Warn("could not commit orphaned metadata cleanup", "err", err) + + return + } + + if len(recordingIDs) > 0 || len(releaseGroupIDs) > 0 || len(artistCreditIDs) > 0 || + len(artistIDs) > 0 { + l.logger.Info( + "pruned orphaned library metadata", + "recordings", len(recordingIDs), + "releaseGroups", len(releaseGroupIDs), + "artistCredits", len(artistCreditIDs), + "artists", len(artistIDs), + ) + } +} + // countAudioFiles performs a fast walk of the library directory, // counting only files with supported audio extensions. No per-file // I/O is performed — this reads only directory entries. diff --git a/backend/library/scan_test.go b/backend/library/scan_test.go index 5b82f45..b902292 100644 --- a/backend/library/scan_test.go +++ b/backend/library/scan_test.go @@ -676,6 +676,180 @@ func TestOrphanDeletion(t *testing.T) { } } +func TestPruneOrphanedMetadata(t *testing.T) { + t.Parallel() + + lib, db := setupTestLibrary(t) + ctx := context.Background() + q := db.Queries + + // Seed a full chain: artist -> artist_credit -> recording -> audio_file, + // plus a release group crediting the same artist. + artist, err := q.UpsertArtist(ctx, "Orphaned Artist") + if err != nil { + t.Fatalf("upsert artist: %v", err) + } + + ac, err := q.UpsertArtistCredit(ctx, "Orphaned Artist") + if err != nil { + t.Fatalf("upsert artist credit: %v", err) + } + + if _, err := q.CreateArtistCreditArtist(ctx, sqlcgen.CreateArtistCreditArtistParams{ + ArtistID: artist.ID, + CreditID: ac.ID, + }); err != nil { + t.Fatalf("link artist credit artist: %v", err) + } + + rec, err := q.CreateRecordingFull(ctx, sqlcgen.CreateRecordingFullParams{ + Name: "Orphaned Song", + ArtistCreditID: ac.ID, + }) + if err != nil { + t.Fatalf("create recording: %v", err) + } + + rg, err := q.CreateReleaseGroupFull(ctx, sqlcgen.CreateReleaseGroupFullParams{ + Name: "Orphaned Album", + AlbumArtistCreditID: sql.NullInt64{Int64: ac.ID, Valid: true}, + }) + if err != nil { + t.Fatalf("create release group: %v", err) + } + + if _, err := q.CreateReleaseGroupRecording(ctx, sqlcgen.CreateReleaseGroupRecordingParams{ + ReleaseGroupID: rg.ID, + RecordingID: rec.ID, + }); err != nil { + t.Fatalf("link release group recording: %v", err) + } + + af, err := q.CreateAudioFile(ctx, sqlcgen.CreateAudioFileParams{ + FilePath: "/music/orphaned.mp3", + LengthMilliseconds: 180000, + FileTypeID: 0, + RecordingID: rec.ID, + Basename: "orphaned.mp3", + }) + if err != nil { + t.Fatalf("create audio file: %v", err) + } + + // Simulate a rescan removing the file: delete the audio_files row + // (what the existing Phase 5 orphan cleanup does), then run the new + // metadata cleanup that's supposed to cascade the rest. + if err := q.DeleteAudioFile(ctx, af.ID); err != nil { + t.Fatalf("delete audio file: %v", err) + } + + lib.pruneOrphanedMetadata() + + if _, err := q.GetRecording(ctx, rec.ID); err == nil { + t.Error("expected orphaned recording to be deleted") + } + + if _, err := q.GetReleaseGroup(ctx, rg.ID); err == nil { + t.Error("expected orphaned release group to be deleted") + } + + if _, err := q.GetArtistCredit(ctx, ac.ID); err == nil { + t.Error("expected orphaned artist credit to be deleted") + } + + if _, err := q.GetArtist(ctx, artist.ID); err == nil { + t.Error("expected orphaned artist to be deleted") + } +} + +// TestPruneOrphanedMetadata_KeepsStillOwnedEntities verifies that pruning +// only removes rows with no remaining audio_files, leaving an artist who +// still owns other tracks untouched. +func TestPruneOrphanedMetadata_KeepsStillOwnedEntities(t *testing.T) { + t.Parallel() + + lib, db := setupTestLibrary(t) + ctx := context.Background() + q := db.Queries + + artist, err := q.UpsertArtist(ctx, "Still Owned Artist") + if err != nil { + t.Fatalf("upsert artist: %v", err) + } + + ac, err := q.UpsertArtistCredit(ctx, "Still Owned Artist") + if err != nil { + t.Fatalf("upsert artist credit: %v", err) + } + + if _, err := q.CreateArtistCreditArtist(ctx, sqlcgen.CreateArtistCreditArtistParams{ + ArtistID: artist.ID, + CreditID: ac.ID, + }); err != nil { + t.Fatalf("link artist credit artist: %v", err) + } + + // Two recordings under the same artist credit; only one loses its file. + recGone, err := q.CreateRecordingFull(ctx, sqlcgen.CreateRecordingFullParams{ + Name: "Removed Song", + ArtistCreditID: ac.ID, + }) + if err != nil { + t.Fatalf("create recording (removed): %v", err) + } + + recKept, err := q.CreateRecordingFull(ctx, sqlcgen.CreateRecordingFullParams{ + Name: "Kept Song", + ArtistCreditID: ac.ID, + }) + if err != nil { + t.Fatalf("create recording (kept): %v", err) + } + + afGone, err := q.CreateAudioFile(ctx, sqlcgen.CreateAudioFileParams{ + FilePath: "/music/gone.mp3", + LengthMilliseconds: 180000, + FileTypeID: 0, + RecordingID: recGone.ID, + Basename: "gone.mp3", + }) + if err != nil { + t.Fatalf("create audio file (gone): %v", err) + } + + if _, err := q.CreateAudioFile(ctx, sqlcgen.CreateAudioFileParams{ + FilePath: "/music/kept.mp3", + LengthMilliseconds: 180000, + FileTypeID: 0, + RecordingID: recKept.ID, + Basename: "kept.mp3", + }); err != nil { + t.Fatalf("create audio file (kept): %v", err) + } + + if err := q.DeleteAudioFile(ctx, afGone.ID); err != nil { + t.Fatalf("delete audio file: %v", err) + } + + lib.pruneOrphanedMetadata() + + if _, err := q.GetRecording(ctx, recGone.ID); err == nil { + t.Error("expected orphaned recording to be deleted") + } + + if _, err := q.GetRecording(ctx, recKept.ID); err != nil { + t.Errorf("expected still-owned recording to survive, got: %v", err) + } + + if _, err := q.GetArtistCredit(ctx, ac.ID); err != nil { + t.Errorf("expected still-referenced artist credit to survive, got: %v", err) + } + + if _, err := q.GetArtist(ctx, artist.ID); err != nil { + t.Errorf("expected still-referenced artist to survive, got: %v", err) + } +} + // --------------------------------------------------------------------------- // Empty/missing metadata tests // --------------------------------------------------------------------------- diff --git a/frontend/src/components/config-page/download-clients.ts b/frontend/src/components/config-page/download-clients.ts index c5f10ac..cc44a3d 100644 --- a/frontend/src/components/config-page/download-clients.ts +++ b/frontend/src/components/config-page/download-clients.ts @@ -11,8 +11,10 @@ import { designTokens } from '../../styles/tokens.css'; import type { DownloadDescriptor, DownloadProvider, + ProviderField, } from '@store/download-store'; import { downloadStore } from '@store/download-store'; +import { DirectoryPicker } from '@go/frontendutil/FrontendUtil'; import './config-section'; /** @@ -162,6 +164,20 @@ export class DownloadClients extends LitElement { .add-row { margin-top: 0.8em; } + + .field-row { + display: flex; + gap: 0.5em; + align-items: flex-end; + } + + .field-row wa-input { + flex: 1; + } + + .field-row .browse-button { + flex-shrink: 0; + } `, ]; @@ -257,7 +273,7 @@ export class DownloadClients extends LitElement { ${this.descriptors.map( (d) => html`${d.name}`, @@ -277,7 +293,7 @@ export class DownloadClients extends LitElement { { + @input=${(e: Event) => { this.draftName = (e.target as HTMLInputElement).value; }} > @@ -311,25 +327,25 @@ export class DownloadClients extends LitElement { { + @input=${(e: Event) => { this.draftName = (e.target as HTMLInputElement).value; }} > - ${descriptor ? this.renderFields(descriptor) : nothing} + ${descriptor ? this.renderFields(descriptor, provider) : nothing} { + @input=${(e: Event) => { this.draft['__priority'] = (e.target as HTMLInputElement).value; }} > { + @change=${(e: Event) => { this.draft['__enabled'] = (e.target as HTMLInputElement) .checked ? '1' @@ -355,28 +371,61 @@ export class DownloadClients extends LitElement { `; } - /** Renders one input per descriptor field. */ - private renderFields(descriptor: DownloadDescriptor) { - return (descriptor.fields ?? []).map( - (field) => html` - { - this.draft = { - ...this.draft, - [field.key]: (e.target as HTMLInputElement).value, - }; - }} - > - ${field.help ? html`${field.help}` : nothing} - - `, - ); + /** Renders one input per descriptor field, plus a folder browse + * button for path fields and an "already set" placeholder for + * secrets the provider already has a stored value for. */ + private renderFields(descriptor: DownloadDescriptor, provider?: DownloadProvider) { + return (descriptor.fields ?? []).map((field) => { + const isSet = field.secret && provider?.setSecrets?.[field.key]; + const placeholder = isSet + ? '•••••••• (unchanged — enter a new value to replace it)' + : (field.placeholder ?? ''); + + return html` +
+ { + this.draft = { + ...this.draft, + [field.key]: (e.target as HTMLInputElement).value, + }; + }} + > + ${field.help ? html`${field.help}` : nothing} + + ${field.path + ? html` + this.browseForFolder(field)} + > + Browse + + ` + : nothing} +
+ `; + }); } + private browseForFolder = async (field: ProviderField) => { + try { + const dir = await DirectoryPicker(); + + if (dir) { + this.draft = { ...this.draft, [field.key]: dir }; + } + } catch (err) { + console.error('Failed to open directory picker:', err); + } + }; + private descriptorFor(kind: string): DownloadDescriptor | undefined { return this.descriptors.find((d) => d.kind === kind); } diff --git a/frontend/src/components/duplicate-tracks-dialog/duplicate-tracks-dialog.ts b/frontend/src/components/duplicate-tracks-dialog/duplicate-tracks-dialog.ts index 39f26dc..a95dae1 100644 --- a/frontend/src/components/duplicate-tracks-dialog/duplicate-tracks-dialog.ts +++ b/frontend/src/components/duplicate-tracks-dialog/duplicate-tracks-dialog.ts @@ -315,7 +315,7 @@ export class DuplicateTracksDialog extends LitElement { { + @change=${(e: Event) => { this.applyToAll = ( e.target as HTMLInputElement ).checked; diff --git a/frontend/src/components/explore-album-details/explore-album-details.ts b/frontend/src/components/explore-album-details/explore-album-details.ts index b0e9015..4de21ff 100644 --- a/frontend/src/components/explore-album-details/explore-album-details.ts +++ b/frontend/src/components/explore-album-details/explore-album-details.ts @@ -123,6 +123,16 @@ export class ExploreAlbumDetails extends LitElement { /** True when this album is already on the wanted list. */ @state() private isWanted = false; + /** + * Library to attach downloads/wants to. The library-filter UI that + * would normally set libraryStore's selection isn't mounted anywhere + * currently, so that selection is always null here — falling back to + * `?? 0` would send a library id that doesn't exist and fail the + * download_requests/download_wants foreign key. Resolved to the + * selected library, or the first one if none is selected. + */ + @state() private targetLibraryId: number | null = null; + /** Unsubscribe handle for the download store. */ private downloadUnsub: (() => void) | null = null; @@ -494,6 +504,8 @@ export class ExploreAlbumDetails extends LitElement { this.syncWanted(); }); + void this.resolveTargetLibraryId(); + // A background BrowseReleases fetch (cold album, versions + // tracklist not cached yet) finished — re-fetch the versions once // per release group so they fill in without the initial request @@ -1511,7 +1523,7 @@ export class ExploreAlbumDetails extends LitElement { size="small" appearance="outlined" @click=${() => { - this.pickerOpen = true; + void this.openPicker(); }} > @@ -1553,6 +1565,11 @@ export class ExploreAlbumDetails extends LitElement { `; } + /** Resolves the library to attach downloads/wants to. */ + private async resolveTargetLibraryId(): Promise { + this.targetLibraryId = await libraryStore.getDefaultLibraryId(); + } + /** Reflects the store's view of whether this album is wanted. */ private syncWanted(): void { this.isWanted = this.releaseGroupMBID @@ -1567,10 +1584,18 @@ export class ExploreAlbumDetails extends LitElement { if (wantId) { await downloadStore.removeWant(wantId); } else { + if (!this.targetLibraryId) { + await this.resolveTargetLibraryId(); + } + if (!this.targetLibraryId) { + console.error('Could not update the wanted list: no library available'); + return; + } + await downloadStore.addWant({ mbid: this.releaseGroupMBID, entity: 'release-group', - libraryId: libraryStore.getSelectedLibraryId() ?? 0, + libraryId: this.targetLibraryId, artist: this.releaseGroup?.artistCredit ?? '', title: this.albumName, scope: 'future', @@ -1584,15 +1609,28 @@ export class ExploreAlbumDetails extends LitElement { this.syncWanted(); } + private async openPicker(): Promise { + if (!this.targetLibraryId) { + await this.resolveTargetLibraryId(); + } + + if (!this.targetLibraryId) { + console.error('Cannot search for downloads: no library available'); + return; + } + + this.pickerOpen = true; + } + private renderPicker() { - if (!this.pickerOpen) return nothing; + if (!this.pickerOpen || !this.targetLibraryId) return nothing; const tracks = this.currentTracks(); return html` { try { + const libraryId = + want.libraryId || (await libraryStore.getDefaultLibraryId()); + if (!libraryId) { + console.error( + 'Could not change what this subscription covers: no library available', + ); + return; + } + await downloadStore.addWant({ mbid: want.mbid, entity: 'artist', - libraryId: want.libraryId || (libraryStore.getSelectedLibraryId() ?? 0), + libraryId, artist: want.artist, title: want.title, scope: want.scope === 'all' ? 'future' : 'all', diff --git a/frontend/src/store/library-store.ts b/frontend/src/store/library-store.ts index 0b7c8e4..449f4d2 100644 --- a/frontend/src/store/library-store.ts +++ b/frontend/src/store/library-store.ts @@ -356,6 +356,25 @@ class LibraryStore { return libs; } + /** + * Resolves a library id to attach a download/want to: the selected + * filter if one is set, otherwise the first known library. Callers + * that need a library id (rather than "all libraries", which is + * what `getSelectedLibraryId()` returning null means for browsing) + * should use this instead of defaulting to 0 — id 0 never exists + * and trips the library_id foreign key on download_requests / + * download_wants. + */ + async getDefaultLibraryId(): Promise { + if (this.selectedLibraryIdValue !== null) { + return this.selectedLibraryIdValue; + } + + const libraries = await this.getLibraries(); + + return libraries[0]?.id ?? null; + } + // =================================================================== // SCROLL POSITION // =================================================================== diff --git a/frontend/wailsjs/go/autotagservice/Service.d.ts b/frontend/wailsjs/go/autotagservice/Service.d.ts index 10a7c01..d5c375e 100755 --- a/frontend/wailsjs/go/autotagservice/Service.d.ts +++ b/frontend/wailsjs/go/autotagservice/Service.d.ts @@ -37,6 +37,8 @@ export function SetContext(arg1:context.Context):Promise; export function Skip(arg1:string):Promise; +export function SplitMixedFolder(arg1:string):Promise>; + export function StartAutotagQueue(arg1:number):Promise; export function StartBackgroundPrefetch():Promise; diff --git a/frontend/wailsjs/go/autotagservice/Service.js b/frontend/wailsjs/go/autotagservice/Service.js index 342a2d0..4515ed2 100755 --- a/frontend/wailsjs/go/autotagservice/Service.js +++ b/frontend/wailsjs/go/autotagservice/Service.js @@ -70,6 +70,10 @@ export function Skip(arg1) { return window['go']['autotagservice']['Service']['Skip'](arg1); } +export function SplitMixedFolder(arg1) { + return window['go']['autotagservice']['Service']['SplitMixedFolder'](arg1); +} + export function StartAutotagQueue(arg1) { return window['go']['autotagservice']['Service']['StartAutotagQueue'](arg1); } diff --git a/frontend/wailsjs/go/library/Library.d.ts b/frontend/wailsjs/go/library/Library.d.ts index 5ffd6df..1e96801 100755 --- a/frontend/wailsjs/go/library/Library.d.ts +++ b/frontend/wailsjs/go/library/Library.d.ts @@ -57,6 +57,8 @@ export function IsScanActive():Promise; export function IsScanPaused():Promise; +export function LibraryPath(arg1:number):Promise; + export function PauseScan():Promise; export function QueuedLibraryNames():Promise>; diff --git a/frontend/wailsjs/go/library/Library.js b/frontend/wailsjs/go/library/Library.js index 5abfac9..411cea3 100755 --- a/frontend/wailsjs/go/library/Library.js +++ b/frontend/wailsjs/go/library/Library.js @@ -106,6 +106,10 @@ export function IsScanPaused() { return window['go']['library']['Library']['IsScanPaused'](); } +export function LibraryPath(arg1) { + return window['go']['library']['Library']['LibraryPath'](arg1); +} + export function PauseScan() { return window['go']['library']['Library']['PauseScan'](); } diff --git a/frontend/wailsjs/go/models.ts b/frontend/wailsjs/go/models.ts index cea8cd0..80113f3 100755 --- a/frontend/wailsjs/go/models.ts +++ b/frontend/wailsjs/go/models.ts @@ -207,6 +207,8 @@ export namespace autotagservice { bestMatchReleaseMbid: string; score: number; status: string; + synthetic: boolean; + likelyMixedBag: boolean; static createFrom(source: any = {}) { return new PendingItem(source); @@ -225,6 +227,8 @@ export namespace autotagservice { this.bestMatchReleaseMbid = source["bestMatchReleaseMbid"]; this.score = source["score"]; this.status = source["status"]; + this.synthetic = source["synthetic"]; + this.likelyMixedBag = source["likelyMixedBag"]; } } @@ -233,6 +237,8 @@ export namespace autotagservice { localTracks: LocalTrackView[]; candidates: CandidateView[]; recommendation: string; + mixedBag: boolean; + synthetic: boolean; static createFrom(source: any = {}) { return new ScoreView(source); @@ -244,6 +250,8 @@ export namespace autotagservice { this.localTracks = this.convertValues(source["localTracks"], LocalTrackView); this.candidates = this.convertValues(source["candidates"], CandidateView); this.recommendation = source["recommendation"]; + this.mixedBag = source["mixedBag"]; + this.synthetic = source["synthetic"]; } convertValues(a: any, classs: any, asMap: boolean = false): any { @@ -443,6 +451,7 @@ export namespace download { enabled: boolean; priority: number; settings: Record; + setSecrets?: Record; static createFrom(source: any = {}) { return new Config(source); @@ -456,6 +465,7 @@ export namespace download { this.enabled = source["enabled"]; this.priority = source["priority"]; this.settings = source["settings"]; + this.setSecrets = source["setSecrets"]; } } export class Field { @@ -464,6 +474,7 @@ export namespace download { placeholder?: string; help?: string; secret: boolean; + path: boolean; required: boolean; default?: string; @@ -478,6 +489,7 @@ export namespace download { this.placeholder = source["placeholder"]; this.help = source["help"]; this.secret = source["secret"]; + this.path = source["path"]; this.required = source["required"]; this.default = source["default"]; } diff --git a/go.mod b/go.mod index f602aba..d6f65ee 100644 --- a/go.mod +++ b/go.mod @@ -12,6 +12,7 @@ require ( github.com/go-flac/go-flac/v2 v2.0.4 github.com/godbus/dbus/v5 v5.1.0 github.com/golang-cz/devslog v0.0.15 + github.com/google/uuid v1.6.0 github.com/gopxl/beep/v2 v2.1.1 github.com/klauspost/compress v1.17.9 github.com/parquet-go/parquet-go v0.30.1 @@ -158,7 +159,6 @@ require ( github.com/google/cel-go v0.26.1 // indirect github.com/google/go-cmp v0.7.0 // indirect github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect - github.com/google/uuid v1.6.0 // indirect github.com/gookit/color v1.6.0 // indirect github.com/gordonklaus/ineffassign v0.2.0 // indirect github.com/gorilla/css v1.0.1 // indirect