From 902eb38fb8f9500ad70715e84e02e16c76744c65 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Sun, 7 Jun 2026 23:53:18 -0700 Subject: [PATCH 01/45] Downloads: fix collapsed-batch overflow (#814) + Retry Failed result feedback (#815) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #814 — the collapsed Batches rail (44px) hid .adl-batch-active and .adl-batch-history-section but NOT the JS-rendered .adl-batch-summary line ("N batches · M downloading · …"), so it overflowed as clipped text. Added it to the collapsed hide rule. #815 — "Retry Failed" only toasted "Retrying N…" at the start and a generic "Discovery complete!" at the end, with no sense of how many of the retried tracks actually progressed. retryFailedMirroredDiscovery now stamps a baseline (matches-before + retry count) on the state, and a shared completion toast reports "Retry complete: X of N newly found[, Y still not found]" instead of the generic message. Normal (non-retry) discovery still shows "Discovery complete!". JS syntax clean, 70 script-split/style tests pass. --- webui/static/stats-automations.js | 6 ++++++ webui/static/style.css | 6 +++++- webui/static/sync-services.js | 22 ++++++++++++++++++++-- 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/webui/static/stats-automations.js b/webui/static/stats-automations.js index 01aab02a..68e1adea 100644 --- a/webui/static/stats-automations.js +++ b/webui/static/stats-automations.js @@ -1678,6 +1678,12 @@ async function retryFailedMirroredDiscovery(urlHash) { state.phase = 'discovering'; state.status = 'discovering'; state.discovery_progress = 0; + // #815: stamp a baseline so the completion toast can report how many + // of these retried tracks were newly found (not just overall matched). + state._retryDiscovery = { + matchesBefore: state.spotify_matches || 0, + retryCount: data.retry_count, + }; } // Update modal buttons to show discovering state diff --git a/webui/static/style.css b/webui/static/style.css index f6a2452e..b78855dc 100644 --- a/webui/static/style.css +++ b/webui/static/style.css @@ -59389,7 +59389,11 @@ body.reduce-effects #page-particles-canvas { } .adl-batch-panel.collapsed .adl-batch-active, -.adl-batch-panel.collapsed .adl-batch-history-section { +.adl-batch-panel.collapsed .adl-batch-history-section, +/* The active-batches summary line ("N batches · M downloading · …") is shown + via JS and was NOT hidden on collapse, so it overflowed the 44px rail as + clipped text (#814). Hide it with the rest of the panel body. */ +.adl-batch-panel.collapsed .adl-batch-summary { display: none; } diff --git a/webui/static/sync-services.js b/webui/static/sync-services.js index 69dd0cef..f375bdd3 100644 --- a/webui/static/sync-services.js +++ b/webui/static/sync-services.js @@ -9248,6 +9248,24 @@ async function startYouTubeDiscovery(urlHash) { } } +// Discovery-complete toast. For a "Retry Failed" run (#815) it reports how many +// of the retried tracks were newly found this attempt, instead of the generic +// message — the baseline is stamped on the state by retryFailedMirroredDiscovery. +function _discoveryCompleteToast(urlHash) { + const st = youtubePlaylistStates[urlHash]; + const retry = st && st._retryDiscovery; + if (retry) { + const found = Math.max(0, (st.spotify_matches || 0) - (retry.matchesBefore || 0)); + const stillFailed = Math.max(0, (retry.retryCount || 0) - found); + delete st._retryDiscovery; + let msg = `Retry complete: ${found} of ${retry.retryCount} newly found`; + if (stillFailed > 0) msg += `, ${stillFailed} still not found`; + showToast(msg, found > 0 ? 'success' : 'info'); + return; + } + showToast('Discovery complete!', 'success'); +} + function startYouTubeDiscoveryPolling(urlHash) { // Stop any existing polling if (activeYouTubePollers[urlHash]) { @@ -9274,7 +9292,7 @@ function startYouTubeDiscoveryPolling(urlHash) { if (st) st.phase = 'discovered'; updateYouTubeCardPhase(urlHash, 'discovered'); updateYouTubeModalButtons(urlHash, 'discovered'); - showToast('Discovery complete!', 'success'); + _discoveryCompleteToast(urlHash); } }; } @@ -9322,7 +9340,7 @@ function startYouTubeDiscoveryPolling(urlHash) { updateYouTubeModalButtons(urlHash, 'discovered'); console.log('✅ Discovery complete:', urlHash); - showToast('Discovery complete!', 'success'); + _discoveryCompleteToast(urlHash); } } catch (error) { From 1ca14d1c1983c9b0d6fa693a3d9fd1c9af58e2b9 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Mon, 8 Jun 2026 08:34:36 -0700 Subject: [PATCH 02/45] Cover art: surface read-only on the cover.jpg sidecar write too (Sokhi, #804 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sokhi still hit the read-only error after the statvfs fix (6c3e285a). Root cause was a gap that fix didn't cover: read_only_fs was only set from the per-file EMBED write, but download_cover_art SWALLOWS its own cover.jpg EROFS (logs "Error downloading cover.jpg" and returns). So when an album's tracks already have embedded art, the embed loop is skipped, only the cover.jpg sidecar write runs, its EROFS is swallowed, and the filler reported success while spamming the log — exactly Sokhi's case. Fix (no blast radius — download_cover_art still never re-raises, since its import-pipeline callers aren't wrapped): on EROFS it now records '_cover_read_only' on the passed context instead of just logging; apply_art_to_ album_files passes a capture dict and promotes that to read_only_fs. So a cover-only read-only album now correctly surfaces the read-only message instead of a silent success. Tests: +1 — embed skipped (track already arted) + cover.jpg read-only → read_only_fs True. 472 cover/art/repair tests pass. --- core/metadata/art_apply.py | 11 ++++++++++- core/metadata/artwork.py | 13 ++++++++++++- tests/test_art_apply.py | 21 +++++++++++++++++++++ 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/core/metadata/art_apply.py b/core/metadata/art_apply.py index 448ff0f2..f81a35cd 100644 --- a/core/metadata/art_apply.py +++ b/core/metadata/art_apply.py @@ -168,11 +168,20 @@ def apply_art_to_album_files( target_dir = folder or (os.path.dirname(paths[0]) if paths else None) if target_dir and os.path.isdir(target_dir): + # download_cover_art swallows its own write errors, so a read-only mount + # (EROFS) on the cover.jpg sidecar would otherwise go undetected here — + # the exact gap that left cover-only albums reporting success on a + # read-only filesystem (Sokhi: tracks already had embedded art, so the + # embed loop above never tripped EROFS). Pass a capture dict and read + # the read-only flag it sets back. + cover_ctx = context if isinstance(context, dict) else {} try: - download_cover_art(album_info, target_dir, context) + download_cover_art(album_info, target_dir, cover_ctx) result["cover_written"] = folder_has_cover_sidecar(target_dir) except Exception as exc: if getattr(exc, "errno", None) == errno.EROFS: result["read_only_fs"] = True logger.warning("cover.jpg write failed for %s: %s", target_dir, exc) + if cover_ctx.get("_cover_read_only"): + result["read_only_fs"] = True return result diff --git a/core/metadata/artwork.py b/core/metadata/artwork.py index 3698a064..091ee1c4 100644 --- a/core/metadata/artwork.py +++ b/core/metadata/artwork.py @@ -2,6 +2,7 @@ from __future__ import annotations +import errno import os import re import time @@ -566,4 +567,14 @@ def download_cover_art(album_info: dict, target_dir: str, context: dict = None): handle.write(image_data) logger.info("Cover art downloaded to: %s", cover_path) except Exception as exc: - logger.error("Error downloading cover.jpg: %s", exc) + # A read-only mount (EROFS) is a "can't write" condition the caller + # needs to surface (cover-art filler #804/Tim/Sokhi) — but we must NOT + # re-raise (import callers aren't wrapped here). Record it on the + # context so callers that care can detect it, instead of just spamming + # the log with a swallowed error. + if getattr(exc, "errno", None) == errno.EROFS: + if isinstance(context, dict): + context["_cover_read_only"] = True + logger.warning("cover.jpg write blocked — read-only filesystem: %s", cover_path) + else: + logger.error("Error downloading cover.jpg: %s", exc) diff --git a/tests/test_art_apply.py b/tests/test_art_apply.py index 2be5d9e4..3a590ef1 100644 --- a/tests/test_art_apply.py +++ b/tests/test_art_apply.py @@ -194,6 +194,27 @@ def test_apply_flags_erofs_from_actual_write(tmp_path, monkeypatch): assert res['failed'] == 2 # first EROFS fails it + bails the rest +def test_cover_only_read_only_is_detected(tmp_path, monkeypatch): + """The gap that left Sokhi stuck (#804): tracks already have art so the + embed loop is skipped, but the cover.jpg write hits a read-only mount. + download_cover_art swallows that EROFS onto the context — apply must still + surface read_only_fs (not report a silent success).""" + f = tmp_path / 'a.flac'; f.write_bytes(b'') + audio = SimpleNamespace(pictures=['pic'], tags={'ok': 1}) # already has art → embed skipped + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + + def _fake_download(album_info, target_dir, ctx): + # mimic download_cover_art's EROFS handling: record on context, swallow + if isinstance(ctx, dict): + ctx['_cover_read_only'] = True + monkeypatch.setattr(aa, 'download_cover_art', _fake_download) + + res = aa.apply_art_to_album_files([str(f)], {}, {}, folder=str(tmp_path)) + + assert res['embedded'] == 0 and res['skipped'] == 1 # nothing embedded + assert res['read_only_fs'] is True # but read-only surfaced + + def test_apply_normal_failure_not_flagged_read_only(tmp_path, monkeypatch): def _boom(): raise PermissionError(13, 'Permission denied') From 41f4eeb91efe5f27d4075f4f68583efe64a78771 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Mon, 8 Jun 2026 08:52:49 -0700 Subject: [PATCH 03/45] Watchlist: log when the release-type filter skips an album/EP/single MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sokhi reported "14 singles, 0 albums" from a scan and setting lookback to "All" didn't help — because it was never lookback. The cause is the per-artist release-type filter: with "Albums" toggled off, _should_include_release drops every 7+-track release (the full discography is still fetched, then filtered). That skip was completely silent, so there was no way to see it in the log. Now logs each skipped release with its type + the artist's albums/eps/singles toggles, so "why didn't my albums get added" is answerable from app.log. No behaviour change — diagnostics only. --- core/watchlist_scanner.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/core/watchlist_scanner.py b/core/watchlist_scanner.py index 0ce55e89..e2a345aa 100644 --- a/core/watchlist_scanner.py +++ b/core/watchlist_scanner.py @@ -1313,6 +1313,19 @@ class WatchlistScanner: logger.info("Skipping album with placeholder tracks: %s", album_name) continue if not self._should_include_release(len(tracks), artist): + # Make the type-filter skip visible — otherwise a user + # with "Albums" toggled off just sees missing tracks + # with no explanation (Sokhi #815-adjacent: 14 singles, + # 0 albums because include_albums was off). + _n = len(tracks) + _kind = 'album' if _n >= 7 else ('EP' if _n >= 4 else 'single') + logger.info( + "Skipping %s '%s' (%d tracks) — release type filter " + "(albums=%s, eps=%s, singles=%s) excludes it", + _kind, album_name, _n, + getattr(artist, 'include_albums', True), + getattr(artist, 'include_eps', True), + getattr(artist, 'include_singles', True)) continue album_image_url = '' From c83d4862e804208f3dc266d2b6d0e6c7c646d463 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Mon, 8 Jun 2026 09:06:37 -0700 Subject: [PATCH 04/45] Cover art: stop crying "(read-only?)" when files are simply already arted (Sokhi/Boulder) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The real reason Sokhi's cover-art fix "didn't work" and Boulder saw the same message on a WRITABLE Windows box: _fix_missing_cover_art reported "Updated database thumbnail, but could not write art to files (read-only?)" whenever embedded==0 AND cover_written==False — but the overwhelmingly common cause of that is every file ALREADY having embedded art (skipped, not failed). The message blamed read-only on a perfectly writable library, which sent us chasing a read-only ghost across three commits. Sokhi's /fix returns 200 (success), so he was never hitting the genuine EROFS path at all. Now the message is derived from the art_result breakdown: - embedded/cover written → "Applied cover art: …" - failed>0 (non-EROFS) → "… check file/folder permissions" - skipped>0 (already arted) → "Cover art already present on all N file(s)" - otherwise → "no file artwork was applied" Genuine read-only (EROFS) still hard-fails with the clear mount message. Tests: already-arted→present (not read-only), failed→permissions, EROFS→hard fail, embedded→success. 453 cover/repair/art tests pass. --- core/repair_worker.py | 34 +++++++++++++++---- tests/test_cover_art_targets.py | 60 +++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 6 deletions(-) diff --git a/core/repair_worker.py b/core/repair_worker.py index 311109c9..58dc2517 100644 --- a/core/repair_worker.py +++ b/core/repair_worker.py @@ -1407,12 +1407,34 @@ class RepairWorker: 'mount (NFS/SMB/mergerfs) is read-write, then recreate the ' 'container. (Database thumbnail was still updated.)'), 'art_result': art_result} - msg = f'Applied cover art: embedded into {embedded}/{len(resolved)} file(s)' - if art_result.get('cover_written'): - msg += ' + wrote cover.jpg' - if embedded == 0 and not art_result.get('cover_written'): - # DB updated but nothing reached disk (e.g. permissions). - msg = 'Updated database thumbnail, but could not write art to files (read-only?)' + skipped = art_result.get('skipped', 0) + failed = art_result.get('failed', 0) + cover_written = art_result.get('cover_written') + + wrote_parts = [] + if embedded: + wrote_parts.append(f'embedded into {embedded}/{len(resolved)} file(s)') + if cover_written: + wrote_parts.append('wrote cover.jpg') + + if wrote_parts: + msg = 'Applied cover art: ' + ' + '.join(wrote_parts) + elif failed: + # Real per-file write failures that were NOT a read-only mount + # (genuine EROFS is handled above) — almost always file/folder + # permissions or a locked file. + msg = (f'Updated database thumbnail, but could not write art to ' + f'{failed} file(s) — check file/folder permissions') + elif skipped: + # Every file already had embedded art and no new cover.jpg was + # needed — nothing to do, NOT a failure. This is the case that made + # the old "(read-only?)" message fire on perfectly writable + # libraries (Boulder on Windows, Sokhi): the files were simply + # already arted, so embedded==0 and cover_written==False. + msg = f'Cover art already present on all {skipped} file(s) — database thumbnail updated' + else: + # No file art applied and nothing found to write. + msg = 'Updated database thumbnail (no file artwork was applied)' if artist_result is not None and artist_result.get('success'): msg += ' + applied artist image' return {'success': True, 'action': 'applied_cover_art', 'message': msg, 'art_result': art_result} diff --git a/tests/test_cover_art_targets.py b/tests/test_cover_art_targets.py index 3c4de992..0538fd8d 100644 --- a/tests/test_cover_art_targets.py +++ b/tests/test_cover_art_targets.py @@ -128,3 +128,63 @@ def test_artist_action_without_found_artist_url_fails_cleanly(tmp_path): assert res['success'] is False album_thumb, artist_thumb = _thumbs(w) assert artist_thumb == 'http://old/artist.jpg' # nothing changed + + +# ── apply-result message accuracy (Sokhi/Boulder: "read-only?" on writable fs) ── + +def _add_track(w, path): + conn = w.db._get_connection() + c = conn.cursor() + c.execute("INSERT INTO tracks VALUES ('t1', 'al1', ?)", (str(path),)) + conn.commit() + conn.close() + + +def _apply_returns(monkeypatch, **art_result): + import core.metadata.art_apply as aa + base = {'embedded': 0, 'failed': 0, 'skipped': 0, 'cover_written': False, 'read_only_fs': False} + base.update(art_result) + monkeypatch.setattr(aa, 'apply_art_to_album_files', lambda *a, **k: base) + + +def test_already_arted_reports_present_not_readonly(tmp_path, monkeypatch): + # The bug: all files already had art (skipped) → embedded 0, cover 0 → the + # old message cried "(read-only?)" on a perfectly writable library. + w = _worker(tmp_path) + f = tmp_path / 'song.mp3'; f.write_bytes(b'x') + _add_track(w, f) + _apply_returns(monkeypatch, skipped=1) + res = w._fix_missing_cover_art('album', 'al1', None, {**DETAILS, '_fix_action': 'album'}) + assert res['success'] is True + assert 'already present' in res['message'].lower() + assert 'read-only' not in res['message'].lower() + + +def test_failed_writes_blame_permissions_not_readonly(tmp_path, monkeypatch): + w = _worker(tmp_path) + f = tmp_path / 'song.mp3'; f.write_bytes(b'x') + _add_track(w, f) + _apply_returns(monkeypatch, failed=1) + res = w._fix_missing_cover_art('album', 'al1', None, {**DETAILS, '_fix_action': 'album'}) + assert res['success'] is True + assert 'permission' in res['message'].lower() + assert 'read-only' not in res['message'].lower() + + +def test_genuine_read_only_still_hard_fails(tmp_path, monkeypatch): + w = _worker(tmp_path) + f = tmp_path / 'song.mp3'; f.write_bytes(b'x') + _add_track(w, f) + _apply_returns(monkeypatch, read_only_fs=True) + res = w._fix_missing_cover_art('album', 'al1', None, {**DETAILS, '_fix_action': 'album'}) + assert res['success'] is False + assert 'read-only' in res['error'].lower() + + +def test_embedded_success_message(tmp_path, monkeypatch): + w = _worker(tmp_path) + f = tmp_path / 'song.mp3'; f.write_bytes(b'x') + _add_track(w, f) + _apply_returns(monkeypatch, embedded=1) + res = w._fix_missing_cover_art('album', 'al1', None, {**DETAILS, '_fix_action': 'album'}) + assert res['success'] is True and 'embedded into 1' in res['message'] From 442ced3dbf77a457b5a641f25b315a846a1b5343 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Mon, 8 Jun 2026 09:40:27 -0700 Subject: [PATCH 05/45] Jellyfin/Emby: populate album thumb_url during the library scan (root cause of "flags every album") MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit JellyfinArtist sets self.thumb (→ artists.thumb_url on scan); JellyfinAlbum never did. So for Jellyfin/Emby the library scan read getattr(album, 'thumb', None) == None and albums.thumb_url stayed empty for the WHOLE library. Two downstream effects: blank album art in the web UI, and the Cover Art Filler flagging every album as "missing cover art" (db_missing = empty thumb_url) — making the filler the only thing that ever populated the column, which is backwards. It should come from the scan, like artist thumbs do. Fix: JellyfinAlbum.thumb = /Items//Images/Primary (same shape as the artist thumb, which already normalizes + displays via fix_artist_image_url). Now the scan stores album thumbs, the UI shows art, and the filler only flags albums that are genuinely missing art. Navidrome + Plex already set album thumbs. Tests: album exposes the Primary-image thumb, None without an id, same shape as the artist thumb. --- core/jellyfin_client.py | 16 +++++++++++++-- tests/test_jellyfin_album_thumb.py | 31 ++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) create mode 100644 tests/test_jellyfin_album_thumb.py diff --git a/core/jellyfin_client.py b/core/jellyfin_client.py index 4937c2c0..8ee4eb74 100644 --- a/core/jellyfin_client.py +++ b/core/jellyfin_client.py @@ -65,7 +65,19 @@ class JellyfinAlbum: self.title = jellyfin_data.get('Name', 'Unknown Album') self.addedAt = self._parse_date(jellyfin_data.get('DateCreated')) self._artist_id = jellyfin_data.get('AlbumArtists', [{}])[0].get('Id', '') if jellyfin_data.get('AlbumArtists') else '' - + # Album cover image, mirroring JellyfinArtist.thumb — so the library + # scan stores albums.thumb_url instead of leaving it empty. Without + # this, EVERY album reads back with no thumb, the web UI shows blank + # art, and the Cover Art Filler flags the entire library as "missing + # cover art" (it became the only thing populating the column). + self.thumb = self._get_album_image_url() + + def _get_album_image_url(self) -> Optional[str]: + """Jellyfin/Emby album primary image URL (same shape as the artist one).""" + if not self.ratingKey: + return None + return f"/Items/{self.ratingKey}/Images/Primary" + def _parse_date(self, date_str: Optional[str]) -> Optional[datetime]: if not date_str: return None @@ -73,7 +85,7 @@ class JellyfinAlbum: return datetime.fromisoformat(date_str.replace('Z', '+00:00')) except: return None - + def artist(self) -> Optional[JellyfinArtist]: """Get the album artist""" if self._artist_id: diff --git a/tests/test_jellyfin_album_thumb.py b/tests/test_jellyfin_album_thumb.py new file mode 100644 index 00000000..f04c3d21 --- /dev/null +++ b/tests/test_jellyfin_album_thumb.py @@ -0,0 +1,31 @@ +"""JellyfinAlbum must expose a cover-image thumb so the library scan stores +albums.thumb_url (mirroring JellyfinArtist). Without it the whole library reads +back with empty album thumbs — blank art in the UI + the Cover Art Filler +flagging every album as "missing cover art". +""" + +from __future__ import annotations + +from core.jellyfin_client import JellyfinAlbum, JellyfinArtist + + +class _Client: + pass + + +def test_album_has_primary_image_thumb(): + alb = JellyfinAlbum({'Id': 'abc123', 'Name': 'For You'}, _Client()) + assert alb.thumb == '/Items/abc123/Images/Primary' + + +def test_album_thumb_none_without_id(): + alb = JellyfinAlbum({'Name': 'No Id Album'}, _Client()) + assert alb.thumb is None + + +def test_album_thumb_matches_artist_shape(): + # Same URL shape the artist uses (already proven to display) — just the + # album's own item id. + art = JellyfinArtist({'Id': 'xyz', 'Name': 'A'}, _Client()) + alb = JellyfinAlbum({'Id': 'xyz', 'Name': 'B'}, _Client()) + assert alb.thumb == art.thumb From 827738505191d12e9fe33949cbb70400fa396de2 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Mon, 8 Jun 2026 09:52:17 -0700 Subject: [PATCH 06/45] Cover Art Filler: resolve the rep path before the disk-art check (flags-every-album) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scan checked album_has_art_on_disk() on the RAW DB track path, while the apply (_fix_missing_cover_art) resolves the path first. On any path-mapped setup (docker mounts, a Plex/SoulSync path mismatch) the raw path isn't found, disk art reads as "missing", and EVERY album gets flagged — then the apply resolves the path, finds the art already there, and reports "already present". Scan and apply disagreed purely because only the apply resolved paths. Now the scan resolves the representative path the same way (resolve_library_ file_path, same transfer/download/config inputs the retag job uses) before checking disk art. Unresolvable → treated as no-local-file (not claimed disk-missing) so we never false-flag a file we simply can't reach. Tests: disk check runs on the resolved path (thumb+art → not flagged); unresolvable path → not flagged + art never checked on None. --- core/repair_jobs/missing_cover_art.py | 18 ++++++++++- tests/test_missing_cover_art.py | 45 +++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/core/repair_jobs/missing_cover_art.py b/core/repair_jobs/missing_cover_art.py index eb3a8bf1..bed11419 100644 --- a/core/repair_jobs/missing_cover_art.py +++ b/core/repair_jobs/missing_cover_art.py @@ -4,6 +4,7 @@ import os import re from core.metadata.art_apply import album_has_art_on_disk +from core.library.path_resolver import resolve_library_file_path from core.metadata_service import get_client_for_source, get_primary_source, get_source_priority from core.repair_jobs import register_job from core.repair_jobs.base import JobContext, JobResult, RepairJob @@ -137,6 +138,8 @@ class MissingCoverArtJob(RepairJob): context.update_progress(0, total) logger.info("Found %d albums missing cover art", total) + download_folder = (context.config_manager.get('soulseek.download_path', '') + if context.config_manager else None) if context.report_progress: context.report_progress(phase=f'Searching artwork for {total} albums...', total=total) @@ -160,7 +163,20 @@ class MissingCoverArtJob(RepairJob): # Art can be missing in the DB (no thumb_url) and/or on disk (no # embedded art and no cover.jpg). Skip albums that already have both. db_missing = not (str(album_thumb).strip() if album_thumb else '') - disk_missing = bool(rep_path) and not album_has_art_on_disk(rep_path) + # Resolve the representative path the SAME way the apply does + # (_fix_missing_cover_art) before checking disk art. Checking the raw + # DB path would fail on any path-mapped setup (docker mounts, a + # Plex/SoulSync path mismatch) — the file isn't found, album art + # reads as "missing", and EVERY album gets flagged while the apply + # (which resolves) then finds the art already present. Unresolvable → + # treat as no-local-file (don't claim disk-missing). + resolved_rep = resolve_library_file_path( + rep_path, + transfer_folder=getattr(context, 'transfer_folder', None), + download_folder=download_folder, + config_manager=context.config_manager, + ) if rep_path else None + disk_missing = bool(resolved_rep) and not album_has_art_on_disk(resolved_rep) if not db_missing and not disk_missing: result.skipped += 1 continue diff --git a/tests/test_missing_cover_art.py b/tests/test_missing_cover_art.py index c32ba009..da952e45 100644 --- a/tests/test_missing_cover_art.py +++ b/tests/test_missing_cover_art.py @@ -281,3 +281,48 @@ def test_result_matches_unit(): # no artist on result → require exact title assert m({'title': 'Album'}, 'Album', 'Artist') assert not m({'title': 'Album Deluxe'}, 'Album', 'Artist') + + +# ── disk-art check must run on the RESOLVED path (flags-every-album bug) ── + +def _add_track(conn, path): + conn.execute( + "INSERT INTO tracks (id, album_id, file_path, disc_number, track_number) " + "VALUES (1, 1, ?, 1, 1)", (path,)) + conn.commit() + + +def test_scan_checks_disk_art_on_resolved_path(monkeypatch): + # Album already has a DB thumb (db not missing) and a track whose DB path + # only resolves via mapping. The disk-art check must run on the RESOLVED + # path — checking the raw path would fail on path-mapped setups and flag + # the whole library while the apply (which resolves) finds art present. + conn = _make_db((1, 'Album', 1, 'https://has/thumb', None, None, None, None, None)) + _add_track(conn, '/plex/raw/song.flac') + context = _make_context(conn) + checked = {} + monkeypatch.setattr(mca, 'resolve_library_file_path', + lambda raw, **k: '/resolved/song.flac' if raw == '/plex/raw/song.flac' else None) + monkeypatch.setattr(mca, 'album_has_art_on_disk', + lambda p: checked.update(path=p) or True) + + result = mca.MissingCoverArtJob().scan(context) + + assert checked.get('path') == '/resolved/song.flac' # resolved, not raw + assert result.findings_created == 0 # thumb + disk art → not flagged + + +def test_scan_unresolvable_path_not_flagged_disk_missing(monkeypatch): + # An unreachable file (resolve → None) must NOT be claimed as "missing disk + # art" — we can't know, so don't false-flag. (Album has a thumb already.) + conn = _make_db((1, 'Album', 1, 'https://has/thumb', None, None, None, None, None)) + _add_track(conn, '/gone/song.flac') + context = _make_context(conn) + monkeypatch.setattr(mca, 'resolve_library_file_path', lambda raw, **k: None) + called = [] + monkeypatch.setattr(mca, 'album_has_art_on_disk', lambda p: called.append(p) or False) + + result = mca.MissingCoverArtJob().scan(context) + + assert result.findings_created == 0 # thumb present, disk unknown → not flagged + assert called == [] # never checked art on a None path From 7d481ae02f19e41c687a7eaa727801cac786334f Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Mon, 8 Jun 2026 10:03:43 -0700 Subject: [PATCH 07/45] Cover Art Filler: a local album with file art isn't "missing" just because the DB thumb is empty (Boulder) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Boulder (Plex): "flags every album, but everything has art." His albums show art in the library (served from the embedded file art), but the DB thumb_url cache column is empty — and the scan flagged on db_missing (empty thumb_url), so every local album tripped it despite having perfectly good art in the files. Now: a LOCAL album is flagged only when its files actually lack art (disk_missing). An empty thumb_url is just a stale cache when the files have art — not "missing cover art". db_missing still flags media-server-only albums (no local files), where the DB thumb is the only art there is. Tests: local+file-art+empty-thumb → NOT flagged (the bug); local+no-file-art → still flagged; media-server-only+empty-thumb → still flagged. --- core/repair_jobs/missing_cover_art.py | 17 +++++++++-- tests/test_missing_cover_art.py | 43 +++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/core/repair_jobs/missing_cover_art.py b/core/repair_jobs/missing_cover_art.py index bed11419..a4477c00 100644 --- a/core/repair_jobs/missing_cover_art.py +++ b/core/repair_jobs/missing_cover_art.py @@ -176,8 +176,21 @@ class MissingCoverArtJob(RepairJob): download_folder=download_folder, config_manager=context.config_manager, ) if rep_path else None - disk_missing = bool(resolved_rep) and not album_has_art_on_disk(resolved_rep) - if not db_missing and not disk_missing: + has_local = bool(resolved_rep) + disk_missing = has_local and not album_has_art_on_disk(resolved_rep) + # An album whose FILES already have art is not "missing cover art", + # even if the DB thumb_url cache happens to be empty — the library + # shows the file art either way. So for a local album, flag only when + # the files actually lack art. db_missing alone matters only for + # media-server-only albums (no local files), where the DB thumb is + # the sole art. Without this, every local album with an empty + # thumb_url got flagged despite having perfectly good embedded art + # (Boulder: "flags every album, but everything has art"). + if has_local: + needs_fix = disk_missing + else: + needs_fix = db_missing + if not needs_fix: result.skipped += 1 continue diff --git a/tests/test_missing_cover_art.py b/tests/test_missing_cover_art.py index da952e45..20b4324f 100644 --- a/tests/test_missing_cover_art.py +++ b/tests/test_missing_cover_art.py @@ -326,3 +326,46 @@ def test_scan_unresolvable_path_not_flagged_disk_missing(monkeypatch): assert result.findings_created == 0 # thumb present, disk unknown → not flagged assert called == [] # never checked art on a None path + + +def test_local_album_with_file_art_not_flagged_despite_empty_thumb(monkeypatch): + # Boulder: every album flagged, but "everything has art". Local files HAVE + # embedded art; only the DB thumb_url cache is empty. That is NOT missing + # cover art — must not be flagged. + conn = _make_db((1, 'Album', 1, '', None, None, None, None, None)) # empty thumb + _add_track(conn, '/music/Album/01.flac') + context = _make_context(conn) + monkeypatch.setattr(mca, 'resolve_library_file_path', lambda raw, **k: raw) + monkeypatch.setattr(mca, 'album_has_art_on_disk', lambda p: True) # files have art + + result = mca.MissingCoverArtJob().scan(context) + + assert result.findings_created == 0 # has file art → not "missing" + assert result.skipped == 1 + + +def test_local_album_without_file_art_still_flagged(monkeypatch): + # Local album whose files genuinely lack art → still flagged (real case). + # Give it a source id + findable art so a finding is created when flagged. + conn = _make_db((1, 'Album', 1, '', 'sp-album', None, None, None, None)) + _add_track(conn, '/music/Album/01.flac') + context = _make_context(conn) + monkeypatch.setattr(mca, 'resolve_library_file_path', lambda raw, **k: raw) + monkeypatch.setattr(mca, 'album_has_art_on_disk', lambda p: False) # no art on disk + monkeypatch.setattr(mca, 'get_primary_source', lambda: 'spotify') + monkeypatch.setattr(mca, 'get_client_for_source', lambda s: _FakeClient(album_image='https://img/x')) + + result = mca.MissingCoverArtJob().scan(context) + assert result.findings_created == 1 # files lack art → flagged + + +def test_media_server_only_album_empty_thumb_still_flagged(monkeypatch): + # No local files (media-server-only) + empty thumb → DB thumb is the only + # art, so still flag it. + conn = _make_db((1, 'Album', 1, '', 'sp-album', None, None, None, None)) # no track added + context = _make_context(conn) + monkeypatch.setattr(mca, 'get_primary_source', lambda: 'spotify') + monkeypatch.setattr(mca, 'get_client_for_source', lambda s: _FakeClient(album_image='https://img/x')) + + result = mca.MissingCoverArtJob().scan(context) + assert result.findings_created == 1 # media-server-only + empty thumb → flagged From 3c06bd03c00345887157524651c5385417bd2790 Mon Sep 17 00:00:00 2001 From: Jeff Theroux Date: Mon, 8 Jun 2026 13:50:11 -0400 Subject: [PATCH 08/45] fix(tidal): honour version field in matching and back off on rate limits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three related fixes to Tidal track matching and downloading: 1. version-field handling — Tidal stores remix/live/edit qualifiers in a dedicated `track.version` attribute (e.g. name="Emerge", version="Junkie XL Remix"), not in the track name. The qualifier filter and the matcher only looked at name/album, so the exact recording was discarded. Fold `version` into both the qualifier haystack and the candidate title passed to MusicMatchingEngine. 2. divergent-version penalty — once versions are visible, OTHER cuts of the same base become candidates ("(Shazam Remix)" vs "(southstar Remix)"). Neither title is a prefix of the other, so the prefix-based version check missed them and the raw ratio stayed high off the shared base. Apply a heavy penalty when both titles carry different version descriptors so the wrong cut can't outscore the threshold. 3. rate-limit backoff — the trackManifests endpoint is aggressively rate-limited; a bare request failed 429 instantly, burned the quality tier, re-queued the track and hammered again (a self-amplifying storm). Honour Retry-After / exponential backoff with a bounded retry count and shutdown-aware sleep. Adds unit + end-to-end tests for all three. --- core/matching_engine.py | 65 +++++++++--- core/tidal_download_client.py | 84 +++++++++++++-- tests/matching/test_divergent_version.py | 100 ++++++++++++++++++ tests/test_tidal_qualifier_filter.py | 126 ++++++++++++++++++++++- tests/test_tidal_rate_limit_backoff.py | 114 ++++++++++++++++++++ 5 files changed, 462 insertions(+), 27 deletions(-) create mode 100644 tests/matching/test_divergent_version.py create mode 100644 tests/test_tidal_rate_limit_backoff.py diff --git a/core/matching_engine.py b/core/matching_engine.py index 32113bcc..15f91f02 100644 --- a/core/matching_engine.py +++ b/core/matching_engine.py @@ -214,6 +214,22 @@ class MusicMatchingEngine: # Standard similarity standard_ratio = SequenceMatcher(None, str1, str2).ratio() + # Version vocabulary, shared by the prefix check and the divergent + # check below. + remaster_keywords = ['remaster', 'remastered'] + different_version_keywords = [ + 'remix', 'mix', 'rmx', # Remixes (different song) + 'live', 'live at', 'live from', # Live versions (different recording) + 'acoustic', 'unplugged', # Acoustic versions (different arrangement) + 'slowed', 'reverb', 'sped up', 'speed up', # TikTok edits (different) + 'radio edit', 'radio version', # Radio edits (different cut) + 'single edit', # Single edits (different cut) + 'album edit', # Album edits (different cut) + 'instrumental', 'karaoke', # Instrumental (different) + 'extended', 'extended version', # Extended (different length) + 'demo', 'rough cut', # Demos (different recording) + ] + # STRICT VERSION CHECKING: Different versions should score LOW # This prevents "Song Title" from matching "Song Title (Remix)" during sync shorter, longer = (str1, str2) if len(str1) <= len(str2) else (str2, str1) @@ -223,23 +239,6 @@ class MusicMatchingEngine: # Extract the extra content extra_content = longer[len(shorter):].strip() - # Check if the extra content looks like version info - # Separate remasters from other versions - they should be treated differently - remaster_keywords = ['remaster', 'remastered'] - - different_version_keywords = [ - 'remix', 'mix', 'rmx', # Remixes (different song) - 'live', 'live at', 'live from', # Live versions (different recording) - 'acoustic', 'unplugged', # Acoustic versions (different arrangement) - 'slowed', 'reverb', 'sped up', 'speed up', # TikTok edits (different) - 'radio edit', 'radio version', # Radio edits (different cut) - 'single edit', # Single edits (different cut) - 'album edit', # Album edits (different cut) - 'instrumental', 'karaoke', # Instrumental (different) - 'extended', 'extended version', # Extended (different length) - 'demo', 'rough cut', # Demos (different recording) - ] - # Normalize extra content for comparison extra_normalized = extra_content.lower().strip(' -()[]') @@ -261,6 +260,38 @@ class MusicMatchingEngine: logger.debug(f"Version mismatch detected: '{str1}' vs '{str2}' (keyword: '{keyword}') - applying heavy penalty") return 0.30 + # STRICT VERSION CHECKING (divergent case): two DIFFERENT versions of + # the same base — e.g. "Song (Shazam Remix)" vs "Song (southstar + # Remix)", or "...live at pukkelpop" vs "...live at wembley". Both + # carry a version descriptor, so neither is a prefix of the other and + # the prefix check above misses them; the raw ratio then stays high off + # the shared base. Without this, when the requested version is absent a + # different cut of the same song can outscore the threshold and get + # downloaded. A correct same-version match is identical after + # normalisation and already returned 1.0 above, so a both-versioned + # pair that survives to here with high base overlap is a genuinely + # different cut. (Remasters are intentionally excluded — the prefix + # branch gives them the lenient 0.75 so re-mastered cuts still match.) + def _versions_in(s: str) -> frozenset: + return frozenset( + kw for kw in different_version_keywords + if re.search(r'\b' + re.escape(kw) + r'\b', s)) + + v1, v2 = _versions_in(str1), _versions_in(str2) + if v1 and v2 and standard_ratio >= 0.5: + # Strip the version words; what remains is base + distinguishing + # descriptor (remixer / performance / year). + def _strip_versions(s: str) -> str: + for kw in different_version_keywords: + s = re.sub(r'\b' + re.escape(kw) + r'\b', ' ', s) + return re.sub(r'\s+', ' ', s).strip() + + if v1 != v2 or _strip_versions(str1) != _strip_versions(str2): + logger.debug( + f"Divergent version detected: '{str1}' vs '{str2}' " + f"- applying heavy penalty") + return 0.30 + return standard_ratio def duration_similarity(self, duration1: int, duration2: int) -> float: diff --git a/core/tidal_download_client.py b/core/tidal_download_client.py index 6ae2027e..4192606a 100644 --- a/core/tidal_download_client.py +++ b/core/tidal_download_client.py @@ -302,18 +302,21 @@ class TidalDownloadClient(DownloadSourcePlugin): @classmethod def _track_matches_qualifiers(cls, track, qualifiers: List[str]) -> bool: - """Issue #589 — qualifier check must inspect both track.name AND - track.album.name. For MTV Unplugged-style releases the live / - unplugged signal lives in the album title, not the track title. - A track passes if every required qualifier appears as a whole - word in either the track name OR its album name. + """Issue #589 — qualifier check must inspect track.name, track.version + AND track.album.name. Tidal stores remix/live/edit qualifiers in a + dedicated `version` attribute (e.g. name="Emerge", version="Junkie XL + Remix"), and for MTV Unplugged-style releases the live / unplugged + signal lives in the album title. A track passes if every required + qualifier appears as a whole word in the track name, its version, OR + its album name. """ if not qualifiers: return True track_name = (getattr(track, 'name', '') or '').lower() + version = (getattr(track, 'version', '') or '').lower() album = getattr(track, 'album', None) album_name = (getattr(album, 'name', '') or '').lower() if album else '' - haystack = f"{track_name} {album_name}".strip() + haystack = f"{track_name} {version} {album_name}".strip() if not haystack: return False for kw in qualifiers: @@ -475,6 +478,17 @@ class TidalDownloadClient(DownloadSourcePlugin): def _tidal_to_track_result(self, track, quality_info: dict) -> TrackResult: artist_name = track.artist.name if track.artist else 'Unknown Artist' title = track.name or 'Unknown Title' + # Tidal keeps remix/live/edition qualifiers in a separate `version` + # attribute (e.g. name="Emerge", version="Junkie XL Remix"). The + # matcher only scores `title`, so without folding the version in, + # every remix/live/edit candidate presents as the bare base track + # and MusicMatchingEngine's strict version check rejects it against + # a "... (Junkie XL Remix)" request. Append the version (unless it's + # already in the name) so the candidate title is the full + # "Title (Version)". + version = getattr(track, 'version', None) + if version and version.strip() and version.strip().lower() not in title.lower(): + title = f"{title} ({version.strip()})" album_name = track.album.name if track.album else None duration_ms = int(track.duration * 1000) if track.duration else None @@ -548,6 +562,62 @@ class TidalDownloadClient(DownloadSourcePlugin): return init_uri, segment_uris + # Tidal aggressively rate-limits the trackManifests endpoint. When a + # batch fans out, a bare request fails 429 instantly, the quality tier is + # burned, the track is re-queued, and the retry hammers again — a + # self-amplifying storm (thousands of 429s, downloads stalled, only the + # lowest tier squeaking through). Honour the server's Retry-After (or back + # off exponentially) so downloads pace themselves to whatever rate Tidal + # allows instead of slamming the wall and instant-failing. + _MANIFEST_MAX_RETRIES = 5 + _MANIFEST_BACKOFF_BASE = 2.0 # seconds → 2, 4, 8, 16, 32 (capped) + _MANIFEST_BACKOFF_CAP = 30.0 + + @classmethod + def _retry_after_seconds(cls, response, attempt: int) -> float: + """Backoff delay for a rate-limited response: honour a numeric + Retry-After header when present, else exponential backoff (capped).""" + retry_after = response.headers.get('Retry-After') if response is not None else None + if retry_after: + try: + return min(max(float(retry_after), 0.0), cls._MANIFEST_BACKOFF_CAP) + except (TypeError, ValueError): + pass + return min(cls._MANIFEST_BACKOFF_BASE * (2 ** attempt), cls._MANIFEST_BACKOFF_CAP) + + def _sleep_with_shutdown(self, delay: float) -> bool: + """Sleep up to `delay` seconds in small slices. Returns True if a + shutdown was requested mid-sleep so the caller can bail early.""" + slept = 0.0 + while slept < delay: + if self.shutdown_check and self.shutdown_check(): + return True + chunk = min(0.5, delay - slept) + time.sleep(chunk) + slept += chunk + return False + + def _get_with_rate_limit_retry(self, url: str, *, params=None, headers=None, + timeout: int = 20): + """HTTP GET that backs off and retries on 429 (and transient 5xx), + honouring Retry-After. Returns the final Response (the caller still + calls raise_for_status); a persistent rate-limit after all retries + returns the last 429 response so existing error handling applies.""" + response = None + for attempt in range(self._MANIFEST_MAX_RETRIES + 1): + response = http_requests.get(url, params=params, headers=headers, timeout=timeout) + if response.status_code != 429 and response.status_code < 500: + return response + if attempt >= self._MANIFEST_MAX_RETRIES: + return response + delay = self._retry_after_seconds(response, attempt) + logger.warning( + f"Tidal returned {response.status_code} on manifest fetch — backing off " + f"{delay:.1f}s (attempt {attempt + 1}/{self._MANIFEST_MAX_RETRIES})") + if self._sleep_with_shutdown(delay): + return response + return response + def _get_hls_manifest(self, track_id: int, quality: str = 'lossless') -> Optional[Dict]: q_info = HLS_QUALITY_MAP.get(quality, HLS_QUALITY_MAP['lossless']) formats = q_info['formats'] @@ -573,7 +643,7 @@ class TidalDownloadClient(DownloadSourcePlugin): } try: - response = http_requests.get(url, params=params, headers=headers, timeout=20) + response = self._get_with_rate_limit_retry(url, params=params, headers=headers, timeout=20) response.raise_for_status() data = response.json() except http_requests.HTTPError as e: diff --git a/tests/matching/test_divergent_version.py b/tests/matching/test_divergent_version.py new file mode 100644 index 00000000..f3d5764d --- /dev/null +++ b/tests/matching/test_divergent_version.py @@ -0,0 +1,100 @@ +"""Divergent-version matching: two DIFFERENT versions of the same base +title must NOT match. + +Context: Tidal stores remix/live/edit qualifiers in a dedicated `version` +field which `_tidal_to_track_result` now folds into the candidate title so +the matcher can see it. That fix made the *correct* version win — but it +also makes OTHER versions of the same song visible ("We Are The People +(Shazam Remix)" vs "(southstar Remix)"). Neither title is a prefix of the +other, so the original prefix-based version check missed them and the raw +ratio stayed high (~0.8) off the shared base. Without discrimination, when +the requested version is absent a different remix could outscore the +threshold and the wrong cut would be downloaded. + +These pin: different descriptors reject, the correct one still wins, and +the existing original-vs-version / remaster behaviour is preserved. +""" + +from __future__ import annotations + +import pytest + +from core.matching_engine import MusicMatchingEngine + +me = MusicMatchingEngine() + + +# ── similarity_score: divergent version tails (already-normalised input) ── + +def test_different_remix_descriptors_rejected(): + assert me.similarity_score( + 'we are the people shazam remix', + 'we are the people southstar remix', + ) == 0.30 + + +def test_different_live_performances_rejected(): + assert me.similarity_score( + 'all night live at pukkelpop', + 'all night live at wembley', + ) == 0.30 + + +def test_different_version_types_same_base_rejected(): + # Same song, different version TYPE (remix vs live) — different cut. + assert me.similarity_score('song title remix', 'song title live') == 0.30 + + +def test_same_base_non_version_tails_not_penalised(): + # "one" / "two" are not version words — leave the raw ratio alone. + assert me.similarity_score('song one', 'song two') != 0.30 + + +# ── regression: original-vs-version + remaster behaviour preserved ── + +def test_original_vs_remix_still_rejected(): + assert me.similarity_score('we are the people', 'we are the people remix') == 0.30 + + +def test_remaster_still_light_penalty(): + assert me.similarity_score('song title', 'song title remastered') == 0.75 + + +def test_identical_titles_still_perfect(): + assert me.similarity_score('emerge junkie xl remix', 'emerge junkie xl remix') == 1.0 + + +# ── end-to-end via score_track_match (raw titles, real weighting) ── + +_ARTIST = 'Empire Of The Sun' + + +def test_wrong_remix_scored_below_threshold(): + # Requested Shazam Remix, only a different remix available → must land + # well under the 0.55/0.60 acceptance gate so it is never downloaded. + conf, _ = me.score_track_match( + 'We Are The People (Shazam Remix)', [_ARTIST], 344_000, + 'We Are The People (southstar Remix)', [_ARTIST], 236_000, + ) + assert conf < 0.55, f'wrong remix scored {conf:.2f}, should be < 0.55' + + +def test_correct_remix_still_wins(): + conf, _ = me.score_track_match( + 'We Are The People (Shazam Remix)', [_ARTIST], 344_000, + 'We Are The People (Shazam Remix)', [_ARTIST], 344_000, + ) + assert conf >= 0.90, f'correct remix scored {conf:.2f}, should be >= 0.90' + + +@pytest.mark.parametrize('wanted,candidate', [ + ('We Are The People (Shazam Remix)', 'We Are The People (ARTBAT Remix)'), + ('All Night (Live @ Pukkelpop)', 'All Night (Umek Remix)'), + ('Emerge (Junkie XL Remix)', 'Emerge (DFA Version)'), +]) +def test_wrong_version_below_correct(wanted, candidate): + artist = 'X' + wrong, _ = me.score_track_match(wanted, [artist], 0, candidate, [artist], 0) + right, _ = me.score_track_match(wanted, [artist], 0, wanted, [artist], 0) + assert right > wrong + assert wrong < 0.60, f'{candidate!r} scored {wrong:.2f} vs {wanted!r}' diff --git a/tests/test_tidal_qualifier_filter.py b/tests/test_tidal_qualifier_filter.py index e97fe51f..b4394580 100644 --- a/tests/test_tidal_qualifier_filter.py +++ b/tests/test_tidal_qualifier_filter.py @@ -19,12 +19,14 @@ from unittest.mock import MagicMock from core.tidal_download_client import TidalDownloadClient -def _make_track(name: str, album_name: str = ''): +def _make_track(name: str, album_name: str = '', version: str = ''): """Build a minimal duck-typed track object matching what the Tidal - SDK returns: a `name` attribute and an `album` attribute with its - own `name`.""" + SDK returns: a `name` attribute, a `version` attribute (remix/live/ + edit qualifier — separate from the title), and an `album` attribute + with its own `name`.""" track = MagicMock() track.name = name + track.version = version track.album = MagicMock() track.album.name = album_name return track @@ -121,3 +123,121 @@ def test_extract_qualifiers_picks_up_live_unplugged(): quals = TidalDownloadClient._extract_qualifiers('Shy Away (MTV Unplugged Live)') assert 'live' in quals assert 'unplugged' in quals + + +# ────────────────────────────────────────────────────────────────────── +# track.version — Tidal stores the remix/live/edit qualifier in a +# dedicated `version` attribute, NOT in track.name. Real-world: the +# exact recording is present in the search results but was discarded +# because neither the qualifier filter nor the matcher looked at +# `version`. Cases below are real Tidal tracks (id in comments). +# ────────────────────────────────────────────────────────────────────── + +def test_qualifier_in_version_field_passes(): + # Tidal 124341 — name="Emerge", version="Junkie XL Remix" + track = _make_track('Emerge', album_name='#1', version='Junkie XL Remix') + assert TidalDownloadClient._track_matches_qualifiers(track, ['remix']) is True + + +def test_qualifier_in_version_field_radio_version(): + # Tidal 122127 — name="Black Horse And The Cherry Tree", + # version="Radio Version" + track = _make_track('Black Horse And The Cherry Tree', version='Radio Version') + assert TidalDownloadClient._track_matches_qualifiers(track, ['version']) is True + + +def test_qualifier_missing_from_name_version_and_album_fails(): + # The studio cut: nothing carries "remix" anywhere — must still reject. + track = _make_track('Emerge', album_name='#1', version='') + assert TidalDownloadClient._track_matches_qualifiers(track, ['remix']) is False + + +def test_empty_version_does_not_pollute_haystack(): + # Regression guard: a falsy version must not let unrelated qualifiers + # match (e.g. via a stringified mock). + track = _make_track('Emerge', album_name='#1', version='') + assert TidalDownloadClient._track_matches_qualifiers(track, ['live']) is False + + +# ────────────────────────────────────────────────────────────────────── +# _tidal_to_track_result — folds `version` into the candidate title so +# the matcher (which scores title only) can see the qualifier. +# ────────────────────────────────────────────────────────────────────── + +def _make_full_track(name, artist, version='', album='', duration=240, + isrc='X', tid=1): + track = MagicMock() + track.id = tid + track.name = name + track.version = version + track.artist = MagicMock() + track.artist.name = artist + track.artist.id = 99 + track.album = MagicMock() + track.album.name = album + track.duration = duration + track.track_num = 1 + track.isrc = isrc + track.bpm = 0 + track.copyright = '' + return track + + +_QINFO = {'codec': 'flac', 'bitrate': 1411} + + +def test_version_folded_into_title(): + track = _make_full_track('Emerge', 'Fischerspooner', version='Junkie XL Remix') + result = TidalDownloadClient._tidal_to_track_result(None, track, _QINFO) + assert result.title == 'Emerge (Junkie XL Remix)' + + +def test_no_version_leaves_title_unchanged(): + track = _make_full_track('Emerge', 'Fischerspooner', version='') + result = TidalDownloadClient._tidal_to_track_result(None, track, _QINFO) + assert result.title == 'Emerge' + + +def test_version_already_in_name_not_duplicated(): + # Some Tidal tracks redundantly carry the version in the name too; + # don't produce "Song (Remix) (Remix)". + track = _make_full_track('Song (Remix)', 'Artist', version='Remix') + result = TidalDownloadClient._tidal_to_track_result(None, track, _QINFO) + assert result.title == 'Song (Remix)' + + +# ────────────────────────────────────────────────────────────────────── +# End-to-end: folding version in is what lets MusicMatchingEngine accept +# the exact recording. Before the fix the bare name scores below the +# 0.60 gate; after it, the full title matches. Real repro cases. +# ────────────────────────────────────────────────────────────────────── + +import pytest +from core.matching_engine import MusicMatchingEngine + + +# (wanted title, artist, tidal name, tidal version) +_REPROS = [ + ('Emerge (Junkie XL Remix)', 'Fischerspooner', 'Emerge', 'Junkie XL Remix'), + ('We Are The People (Shazam Remix)', 'Empire Of The Sun', 'We Are The People', 'Shazam Remix'), + ('Black Horse And The Cherry Tree (Radio Version)', 'KT Tunstall', 'Black Horse And The Cherry Tree', 'Radio Version'), + ('All Night (Live @ Pukkelpop)', 'Parov Stelar', 'All Night', 'Live @ Pukkelpop'), + ('Fleur de Lille (Extended)', 'Parov Stelar', 'Fleur de Lille', 'Extended'), +] + + +@pytest.mark.parametrize('wanted,artist,tidal_name,tidal_version', _REPROS) +def test_version_fold_lets_matcher_accept(wanted, artist, tidal_name, tidal_version): + me = MusicMatchingEngine() + track = _make_full_track(tidal_name, artist, version=tidal_version) + folded = TidalDownloadClient._tidal_to_track_result(None, track, _QINFO).title + + # Before the fix the candidate title was the bare Tidal name → rejected. + bare_conf, _ = me.score_track_match(wanted, [artist], 0, tidal_name, [artist], 0) + # After the fix it's "Name (Version)" → accepted. + folded_conf, _ = me.score_track_match(wanted, [artist], 0, folded, [artist], 0) + + assert folded_conf >= 0.60, f'{wanted!r}: folded {folded_conf:.2f} should clear 0.60' + assert folded_conf > bare_conf, ( + f'{wanted!r}: folding version in ({folded_conf:.2f}) must beat bare name ' + f'({bare_conf:.2f})') diff --git a/tests/test_tidal_rate_limit_backoff.py b/tests/test_tidal_rate_limit_backoff.py new file mode 100644 index 00000000..f55fceee --- /dev/null +++ b/tests/test_tidal_rate_limit_backoff.py @@ -0,0 +1,114 @@ +"""Tidal manifest fetch backs off on HTTP 429 instead of instant-failing. + +Tidal aggressively rate-limits the trackManifests endpoint. The bare +request previously failed 429 immediately, burning the quality tier and +re-queueing the track, which hammered Tidal again — a self-amplifying +storm (thousands of 429s, downloads stalled). These pin the backoff: +retry on 429, honour Retry-After, give up after a bounded number of +attempts, bail on shutdown, and never retry a normal 4xx. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +from core.tidal_download_client import TidalDownloadClient + + +def _client(): + # Bypass __init__ (which builds a tidalapi session + mkdirs); we only + # exercise the pure HTTP-retry helpers. + c = TidalDownloadClient.__new__(TidalDownloadClient) + c.shutdown_check = None + return c + + +def _resp(status, retry_after=None): + r = MagicMock() + r.status_code = status + r.headers = {'Retry-After': str(retry_after)} if retry_after is not None else {} + return r + + +# ── _retry_after_seconds ────────────────────────────────────────────── + +def test_exponential_backoff_without_header(): + assert TidalDownloadClient._retry_after_seconds(_resp(429), 0) == 2.0 + assert TidalDownloadClient._retry_after_seconds(_resp(429), 1) == 4.0 + assert TidalDownloadClient._retry_after_seconds(_resp(429), 2) == 8.0 + + +def test_backoff_capped(): + # 2 * 2**10 = 2048 → capped at 30. + assert TidalDownloadClient._retry_after_seconds(_resp(429), 10) == 30.0 + + +def test_retry_after_header_honoured(): + assert TidalDownloadClient._retry_after_seconds(_resp(429, retry_after=7), 0) == 7.0 + + +def test_retry_after_header_capped(): + assert TidalDownloadClient._retry_after_seconds(_resp(429, retry_after=999), 0) == 30.0 + + +def test_garbage_retry_after_falls_back_to_exponential(): + assert TidalDownloadClient._retry_after_seconds(_resp(429, retry_after='soon'), 1) == 4.0 + + +# ── _get_with_rate_limit_retry ──────────────────────────────────────── + +def test_retries_then_succeeds(): + c = _client() + responses = [_resp(429), _resp(429), _resp(200)] + with patch('core.tidal_download_client.http_requests.get', + side_effect=responses) as g, \ + patch('core.tidal_download_client.time.sleep') as sleep: + out = c._get_with_rate_limit_retry('http://x') + assert out.status_code == 200 + assert g.call_count == 3 + assert sleep.call_count >= 2 # backed off before each retry + + +def test_non_429_4xx_returns_immediately_no_retry(): + c = _client() + with patch('core.tidal_download_client.http_requests.get', + side_effect=[_resp(403)]) as g, \ + patch('core.tidal_download_client.time.sleep') as sleep: + out = c._get_with_rate_limit_retry('http://x') + assert out.status_code == 403 + assert g.call_count == 1 + sleep.assert_not_called() + + +def test_gives_up_after_max_retries(): + c = _client() + with patch('core.tidal_download_client.http_requests.get', + side_effect=[_resp(429)] * 20) as g, \ + patch('core.tidal_download_client.time.sleep'): + out = c._get_with_rate_limit_retry('http://x') + assert out.status_code == 429 + # initial try + MAX_RETRIES retries + assert g.call_count == TidalDownloadClient._MANIFEST_MAX_RETRIES + 1 + + +def test_transient_5xx_is_retried(): + c = _client() + with patch('core.tidal_download_client.http_requests.get', + side_effect=[_resp(503), _resp(200)]) as g, \ + patch('core.tidal_download_client.time.sleep'): + out = c._get_with_rate_limit_retry('http://x') + assert out.status_code == 200 + assert g.call_count == 2 + + +def test_shutdown_aborts_backoff(): + c = _client() + c.shutdown_check = lambda: True # shutdown requested + with patch('core.tidal_download_client.http_requests.get', + side_effect=[_resp(429), _resp(200)]) as g, \ + patch('core.tidal_download_client.time.sleep'): + out = c._get_with_rate_limit_retry('http://x') + # First call 429 → enters backoff → shutdown → returns the 429 without + # making the second request. + assert out.status_code == 429 + assert g.call_count == 1 From cdee7b3550f5ce00342774f61909596bdc4d45e8 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Mon, 8 Jun 2026 11:07:00 -0700 Subject: [PATCH 09/45] Cover Art Filler: always write the cover.jpg sidecar (Sokhi) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sokhi: filler works but doesn't write cover.jpg. Cause: the sidecar write (download_cover_art) respects the import-time "Download cover.jpg to album folder" toggle, while embedding ignores it — so with that toggle off, art embeds but no sidecar is written. A job literally called Cover Art Filler should produce the complete art when you explicitly run it. download_cover_art gains force=True (bypasses the toggle) and the filler's apply path passes it. The import pipeline still calls without force, so it keeps honoring the user's setting. Tests: filler passes force=True; existing cover-write mocks updated for the new kwarg. 1732 art/cover/repair/import tests pass. --- core/metadata/art_apply.py | 5 ++++- core/metadata/artwork.py | 12 ++++++++++-- tests/test_art_apply.py | 20 ++++++++++++++++++-- 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/core/metadata/art_apply.py b/core/metadata/art_apply.py index f81a35cd..cee020ed 100644 --- a/core/metadata/art_apply.py +++ b/core/metadata/art_apply.py @@ -176,7 +176,10 @@ def apply_art_to_album_files( # the read-only flag it sets back. cover_ctx = context if isinstance(context, dict) else {} try: - download_cover_art(album_info, target_dir, cover_ctx) + # force=True: the Cover Art Filler always writes the cover.jpg sidecar, + # regardless of the import-time "Download cover.jpg" toggle — running + # the art filler is an explicit request for the complete art (Sokhi). + download_cover_art(album_info, target_dir, cover_ctx, force=True) result["cover_written"] = folder_has_cover_sidecar(target_dir) except Exception as exc: if getattr(exc, "errno", None) == errno.EROFS: diff --git a/core/metadata/artwork.py b/core/metadata/artwork.py index 091ee1c4..758fe206 100644 --- a/core/metadata/artwork.py +++ b/core/metadata/artwork.py @@ -470,9 +470,17 @@ def embed_album_art_metadata(audio_file, metadata: dict): return False -def download_cover_art(album_info: dict, target_dir: str, context: dict = None): +def download_cover_art(album_info: dict, target_dir: str, context: dict = None, force: bool = False): + """Write cover.jpg into ``target_dir``. + + ``force`` bypasses the import-time "Download cover.jpg to album folder" + toggle — used by the Cover Art Filler, whose whole job is to add cover art + (if you explicitly run the filler you want the sidecar regardless of the + auto-import preference). The import pipeline calls this WITHOUT force, so it + still honors the user's setting. + """ cfg = get_config_manager() - if cfg.get("metadata_enhancement.cover_art_download", True) is False: + if not force and cfg.get("metadata_enhancement.cover_art_download", True) is False: return try: diff --git a/tests/test_art_apply.py b/tests/test_art_apply.py index 3a590ef1..4e3d2470 100644 --- a/tests/test_art_apply.py +++ b/tests/test_art_apply.py @@ -109,7 +109,7 @@ def test_apply_embeds_into_each_file_and_writes_cover(tmp_path, monkeypatch): monkeypatch.setattr(aa, 'embed_album_art_metadata', lambda a, m: embed_calls.append(m) or True) # download_cover_art is the standard cover.jpg writer — stub it to drop one. monkeypatch.setattr(aa, 'download_cover_art', - lambda album_info, folder, ctx=None: open(f"{folder}/cover.jpg", 'wb').close()) + lambda album_info, folder, ctx=None, **k: open(f"{folder}/cover.jpg", 'wb').close()) meta = {'artist': 'A', 'album': 'B', 'album_art_url': 'http://x/y.jpg'} res = aa.apply_art_to_album_files([str(f1), str(f2)], meta, {'album_name': 'B'}, folder=str(tmp_path)) @@ -203,7 +203,7 @@ def test_cover_only_read_only_is_detected(tmp_path, monkeypatch): audio = SimpleNamespace(pictures=['pic'], tags={'ok': 1}) # already has art → embed skipped monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) - def _fake_download(album_info, target_dir, ctx): + def _fake_download(album_info, target_dir, ctx, **k): # mimic download_cover_art's EROFS handling: record on context, swallow if isinstance(ctx, dict): ctx['_cover_read_only'] = True @@ -231,3 +231,19 @@ def test_apply_normal_failure_not_flagged_read_only(tmp_path, monkeypatch): assert res['failed'] == 1 + + +def test_filler_forces_cover_sidecar_write(tmp_path, monkeypatch): + # The Cover Art Filler must write cover.jpg regardless of the import-time + # "Download cover.jpg" toggle — so apply passes force=True (Sokhi). + f = tmp_path / 'a.mp3'; f.write_bytes(b'') + audio = SimpleNamespace(pictures=[], tags={'ok': 1}, save=lambda: None) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + monkeypatch.setattr(aa, 'embed_album_art_metadata', lambda *a, **k: True) + captured = {} + monkeypatch.setattr(aa, 'download_cover_art', + lambda album_info, target_dir, ctx, **k: captured.update(k)) + + aa.apply_art_to_album_files([str(f)], {}, {}, folder=str(tmp_path)) + + assert captured.get('force') is True From 72c62aec4570450295b0d874910dc893b7de10a2 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Mon, 8 Jun 2026 11:29:00 -0700 Subject: [PATCH 10/45] CSS: fix dashboard hover-flicker (#816), Automations tile clutter (#816), and onboarding badge overlap (#817) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #816 hover-flicker — .dash-card:hover and .qa-tile:hover both did transform: translateY(-Npx). Hovering a card's bottom edge lifted it off the cursor → un-hover → drop → re-hover, an infinite rapid loop. Since every dashboard card is a .dash-card, all of them flickered ("all elements affected"). Removed the translateY lift; the hover's stronger shadow + border glow already reads as "raised" without moving the hit box. (qa-tile has overflow:hidden so a pseudo-element gap-buffer can't help — removal is the clean, consistent fix.) #816 Automations "looks strange" — the .qa-tile__flow decoration sits in the bottom row directly behind the green "Open →" CTA; at 0.45 opacity the accent nodes/line competed with the CTA (green-on-green clutter). Toned to 0.22 so it reads as faint background texture; still brightens on hover. #817 badge overlap — .helper-first-launch-tip was right:84px, only ~4px clear of the ? float button's 8px pulse ring (button left edge ~72px from right), so the "New here?" tip touched the button. Moved to right:96px. CSS values/comments only — no structural changes (brace delta unchanged). --- webui/static/style.css | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/webui/static/style.css b/webui/static/style.css index b78855dc..cc22d3b2 100644 --- a/webui/static/style.css +++ b/webui/static/style.css @@ -2673,7 +2673,10 @@ body.helper-mode-active #dashboard-activity-feed:hover { .helper-first-launch-tip { position: fixed; bottom: 34px; - right: 84px; + /* Clear the ? float button (right:24 + 48 wide → left edge ~72px from the + right) AND its 8px pulse ring, with margin, so the tip never covers the + button (#817). Was 84px — only ~4px off the pulse ring, so they touched. */ + right: 96px; padding: 8px 16px; background: rgba(16, 16, 16, 0.95); border: 1px solid rgba(var(--accent-rgb), 0.3); @@ -61935,7 +61938,10 @@ body.reduce-effects .dash-card::after { } .dash-card:hover { border-color: rgba(var(--accent-rgb), 0.35); - transform: translateY(-3px); + /* No translateY lift: moving the card up on hover pulls its bottom edge off + the cursor when you hover that edge, which un-hovers → drops → re-hovers + in a rapid flicker loop (#816). The stronger shadow + glow below already + reads as "raised" without moving the hit box. */ box-shadow: 0 12px 40px rgba(0, 0, 0, 0.40), 0 4px 14px rgba(0, 0, 0, 0.22), @@ -62137,7 +62143,9 @@ body.reduce-effects .dash-card::after { .qa-tile:hover, .qa-tile:focus-visible { - transform: translateY(-2px); + /* No translateY lift — see .dash-card:hover (#816 hover-flicker). overflow + is hidden here so a pseudo-element gap-buffer can't help; the glow/shadow + below carries the hover state. */ border-color: rgba(var(--accent-rgb), 0.32); box-shadow: 0 16px 40px rgba(0, 0, 0, 0.5), @@ -62310,7 +62318,11 @@ body.reduce-effects .dash-card::after { align-items: center; justify-content: space-between; gap: clamp(4px, 0.5cqw, 8px); - opacity: 0.45; + /* The flow sits in the bottom row, directly behind the green "Open →" CTA — + at 0.45 the accent nodes/line competed with the CTA and read as clutter + (#816 "automations looks a bit strange"). Toned to a faint background + texture; it still brightens on hover. */ + opacity: 0.22; transition: opacity 0.35s ease; } From df898b52120c957b2c89015b3f1a9c3d7397005b Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Mon, 8 Jun 2026 13:46:28 -0700 Subject: [PATCH 11/45] Import: atomic tag saves so an interrupted/OOM save can't destroy the file (#819) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CubeComming: manual imports of large hi-res Qobuz FLACs came out as empty shells — no audio, no tags, no length/bitrate. Root cause: mutagen's in-place save() rewrites the file, and it's NOT atomic; if the process is interrupted or OOM-killed mid-write (he also reported the memory-growth issue #802), the file is left truncated — audio and tags gone. save_audio_file (the chokepoint both the enrichment tag-write AND wipe_source_ tags route through) now saves atomically: copy the original to a temp in the same dir, write the modified tags into the copy, verify it's still valid audio (duration > 0), then os.replace() it in. The original is untouched until that atomic swap, so a crash can only orphan the temp — never destroy the user's file. Falls back to the plain in-place save (byte-identical to before) when the atomic path can't run, so nothing is ever left worse off. tag_writer's write_tags_to_file routes through the same helper. Verified the atomic path works with REAL mutagen on a real FLAC (audio length preserved 1.0s→1.0s, tag written, temp cleaned). Tests: replace-on-success, original-survives-save-failure, corrupt-temp-rejected, no-filename-plain-save, + a real-FLAC round-trip (skips without ffmpeg). 2443 import/metadata/tag tests pass. --- core/metadata/common.py | 59 +++++++++++++- core/tag_writer.py | 13 ++- tests/test_atomic_audio_save.py | 137 ++++++++++++++++++++++++++++++++ 3 files changed, 198 insertions(+), 11 deletions(-) create mode 100644 tests/test_atomic_audio_save.py diff --git a/core/metadata/common.py b/core/metadata/common.py index ebf6ee5d..407de668 100644 --- a/core/metadata/common.py +++ b/core/metadata/common.py @@ -3,6 +3,7 @@ from __future__ import annotations import os +import shutil import threading import weakref from types import SimpleNamespace @@ -142,13 +143,63 @@ def is_vorbis_like(audio_file: Any, symbols: Any) -> bool: return bool(vorbis_classes) and isinstance(audio_file, vorbis_classes) or is_ogg_opus(audio_file) -def save_audio_file(audio_file: Any, symbols: Any) -> None: +def _raw_audio_save(audio_file: Any, symbols: Any, target: Any = None) -> None: + """The plain mutagen save with the format-specific kwargs. ``target`` None → + save in place (the exact call used before #819, byte-for-byte unchanged); a + path → save into that file (the atomic temp copy).""" if isinstance(audio_file.tags, symbols.ID3): - audio_file.save(v1=0, v2_version=4) + audio_file.save(v1=0, v2_version=4) if target is None else audio_file.save(target, v1=0, v2_version=4) elif isinstance(audio_file, symbols.FLAC): - audio_file.save(deleteid3=True) + audio_file.save(deleteid3=True) if target is None else audio_file.save(target, deleteid3=True) else: - audio_file.save() + audio_file.save() if target is None else audio_file.save(target) + + +def save_audio_file(audio_file: Any, symbols: Any) -> None: + """Persist mutagen tag changes ATOMICALLY where possible (#819). + + mutagen's in-place ``save()`` rewrites the file; if the process is + interrupted or OOM-killed mid-write, the file is left truncated — audio AND + tags gone (CubeComming's large hi-res FLACs imported to an empty shell). + Instead: copy the original to a temp in the same directory, write the + modified tags into that copy, verify it's still a valid audio file, then + ``os.replace`` it in atomically. The original is never touched until that + final swap, so a crash can only ever orphan the temp — never destroy the + user's file. + + Falls back to the plain in-place save if the atomic path can't run (no + filename, copy fails, or a format mutagen can't save-to-path) so the file + is never left worse off than it is today. + """ + path = getattr(audio_file, "filename", None) + try: + path = os.fspath(path) if path else None + except TypeError: + path = None + if not path or not os.path.isfile(path): + _raw_audio_save(audio_file, symbols) + return + + tmp = f"{path}.sstmp" + try: + shutil.copy2(path, tmp) # snapshot original (audio + tags) + _raw_audio_save(audio_file, symbols, target=tmp) # write new tags into the copy + check = symbols.File(tmp) # verify it's still real audio + if check is None or getattr(getattr(check, "info", None), "length", 0) <= 0: + raise ValueError("atomic save produced a file with no audio") + os.replace(tmp, path) # atomic swap — original safe until here + except Exception as atomic_err: + # Original untouched (only tmp was written). Clean up + fall back to the + # original in-place save so any format/edge the atomic path can't handle + # behaves exactly as before. + try: + if os.path.exists(tmp): + os.remove(tmp) + except OSError: + pass + logger.warning("[Atomic Save] atomic path failed (%s) — in-place fallback for %s", + atomic_err, os.path.basename(path)) + _raw_audio_save(audio_file, symbols) def get_image_dimensions(data: bytes): diff --git a/core/tag_writer.py b/core/tag_writer.py index 707454ac..e0f0ea88 100644 --- a/core/tag_writer.py +++ b/core/tag_writer.py @@ -390,13 +390,12 @@ def write_tags_to_file(file_path: str, db_data: Dict[str, Any], if art_ok: written.append('cover_art') - # Save - if isinstance(audio.tags, ID3): - audio.save(v1=0, v2_version=4) - elif isinstance(audio, FLAC): - audio.save(deleteid3=True) - else: - audio.save() + # Save — atomically (#819): write into a temp copy + atomic replace so an + # interrupted/OOM-killed save can never truncate the user's file. Same + # format kwargs as before, just routed through the shared atomic helper. + from types import SimpleNamespace + from core.metadata.common import save_audio_file + save_audio_file(audio, SimpleNamespace(ID3=ID3, FLAC=FLAC, File=MutagenFile)) return {'success': True, 'written_fields': written} diff --git a/tests/test_atomic_audio_save.py b/tests/test_atomic_audio_save.py new file mode 100644 index 00000000..396dafb8 --- /dev/null +++ b/tests/test_atomic_audio_save.py @@ -0,0 +1,137 @@ +"""Atomic tag saves: an interrupted/OOM-killed save must never destroy the +user's file (#819 — CubeComming's hi-res FLACs imported to an empty shell). + +save_audio_file writes the modified tags into a temp COPY, verifies it's still +valid audio, then os.replace()s it in — the original is untouched until that +atomic swap. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +from types import SimpleNamespace + +import pytest + +from core.metadata.common import save_audio_file + + +def _symbols(file_length): + """Symbols whose File() reports the given decoded length (None → invalid).""" + return SimpleNamespace( + ID3=type("ID3", (), {}), FLAC=type("FLAC", (), {}), + File=lambda p: (None if file_length is None + else SimpleNamespace(info=SimpleNamespace(length=file_length))), + ) + + +def test_atomic_replace_on_success(tmp_path): + f = tmp_path / "song.flac" + f.write_bytes(b"ORIGINAL-AUDIO") + saved_to = [] + + class Audio: + filename = str(f) + tags = None + + def save(self, target=None, **k): + saved_to.append(target) + # mimic mutagen writing modified tags into the temp copy + with open(target, "ab") as h: + h.write(b"+TAGS") + + save_audio_file(Audio(), _symbols(180.0)) + + assert f.read_bytes() == b"ORIGINAL-AUDIO+TAGS" # replaced with the tagged copy + assert saved_to == [str(f) + ".sstmp"] # wrote the temp, NOT in place + assert not (tmp_path / "song.flac.sstmp").exists() # temp cleaned up + + +def test_original_survives_save_failure(tmp_path): + # The #819 scenario: the save blows up mid-write. The original must be intact. + f = tmp_path / "song.flac" + f.write_bytes(b"ORIGINAL") + inplace = [] + + class Audio: + filename = str(f) + tags = None + + def save(self, target=None, **k): + if target is not None: + raise OSError("simulated interrupted/OOM save") + inplace.append(True) # fallback in-place save (writes nothing here) + + save_audio_file(Audio(), _symbols(180.0)) + + assert f.read_bytes() == b"ORIGINAL" # never destroyed + assert not (tmp_path / "song.flac.sstmp").exists() # temp removed + assert inplace == [True] # fell back to in-place + + +def test_corrupt_temp_rejected(tmp_path): + # save-to-temp "succeeds" but produces a file with no audio → must NOT + # replace the original; fall back instead. + f = tmp_path / "song.flac" + f.write_bytes(b"ORIGINAL") + inplace = [] + + class Audio: + filename = str(f) + tags = None + + def save(self, target=None, **k): + if target is None: + inplace.append(True) + + save_audio_file(Audio(), _symbols(0)) # File().info.length == 0 → invalid + + assert f.read_bytes() == b"ORIGINAL" + assert not (tmp_path / "song.flac.sstmp").exists() + assert inplace == [True] + + +def test_no_filename_plain_save(): + saves = [] + + class Audio: + filename = None + tags = None + + def save(self, target=None, **k): + saves.append(target) + + save_audio_file(Audio(), _symbols(180.0)) + assert saves == [None] # nothing to be atomic about → plain in-place + + +# ── real mutagen round-trip (only if ffmpeg can make a FLAC) ── + +def _make_flac(path): + ff = shutil.which("ffmpeg") + if not ff: + return False + r = subprocess.run( + [ff, "-f", "lavfi", "-i", "sine=frequency=440:duration=1", "-y", str(path)], + capture_output=True) + return r.returncode == 0 and os.path.getsize(path) > 0 + + +def test_real_flac_atomic_save_preserves_audio(tmp_path): + from mutagen.flac import FLAC + f = tmp_path / "real.flac" + if not _make_flac(f): + pytest.skip("ffmpeg unavailable — cannot build a real FLAC") + orig_len = FLAC(str(f)).info.length + + audio = FLAC(str(f)) + audio["title"] = "Atomic Test" + save_audio_file(audio, SimpleNamespace(ID3=type("ID3", (), {}), FLAC=FLAC, + File=__import__("mutagen").File)) + + reread = FLAC(str(f)) + assert reread.info.length == pytest.approx(orig_len, abs=0.05) # audio intact + assert reread["title"] == ["Atomic Test"] # tag written + assert not (tmp_path / "real.flac.sstmp").exists() From b2de64e87df4b147f03a8d6ab3a5cd254ee719eb Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Mon, 8 Jun 2026 14:37:48 -0700 Subject: [PATCH 12/45] Search: extend pasted-link resolution to Discogs (#813) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #775 already resolves pasted Spotify / Apple / MusicBrainz / Deezer links to an exact artist/album/track on the Search page. Added Discogs to that set (the one source in the request not already covered; Amazon left out per request). - by_id resolver: discogs in SUPPORTED_SOURCES + _KNOWN_HOSTS; parse /artist/-slug, /release/-slug, /master/-slug (master→album), and strip the slug to the leading numeric id. Discogs has no standalone track URLs (tracks live inside a release), so artist + album resolve. - Fetch dispatch: discogs albums use get_album(include_tracks=False) like itunes/musicbrainz; artists use get_artist; both already return the common normalized card shape. Updated the not-a-link hint to mention Discogs. Frontend needs no change — it adopts whatever source the resolver returns and Discogs is already a search source. Tests: parse release/master/artist (slug-stripped, scheme-less) + resolve release→get_album(numeric id) and artist→get_artist. 43 by_id tests pass. --- core/search/by_id.py | 25 ++++++++++++---- tests/search/test_search_by_id.py | 50 +++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/core/search/by_id.py b/core/search/by_id.py index 7b8f73d1..032c87f4 100644 --- a/core/search/by_id.py +++ b/core/search/by_id.py @@ -33,6 +33,7 @@ an injected ``client_resolver`` (defaulting to the orchestrator's from __future__ import annotations import logging +import re from typing import Any, Callable, NamedTuple, Optional from urllib.parse import parse_qs, urlparse @@ -42,13 +43,13 @@ logger = logging.getLogger(__name__) # providers whose public links a user would paste AND whose get-by-id returns # the common Spotify-shaped dict. Streaming download backends (Tidal/Qobuz) # return raw API shapes and aren't metadata-link sources, so they're omitted. -SUPPORTED_SOURCES = ('spotify', 'itunes', 'musicbrainz', 'deezer') +SUPPORTED_SOURCES = ('spotify', 'itunes', 'musicbrainz', 'deezer', 'discogs') # Domains we recognize — used to detect a pasted URL even when the user # omitted the scheme (e.g. "open.spotify.com/album/…"). _KNOWN_HOSTS = ( 'open.spotify.com', 'music.apple.com', 'itunes.apple.com', - 'musicbrainz.org', 'deezer.com', + 'musicbrainz.org', 'deezer.com', 'discogs.com', ) @@ -72,7 +73,7 @@ class LookupTarget(NamedTuple): def _kind_from_keyword(keyword: str) -> Optional[str]: """Map a URL/URI path keyword to a lookup kind.""" - if keyword in ('album', 'release', 'release-group'): + if keyword in ('album', 'release', 'release-group', 'master'): return 'album' if keyword in ('track', 'recording', 'song'): return 'track' @@ -130,6 +131,18 @@ def _parse_url(raw: str) -> list[LookupTarget]: # redirect; only handle canonical /album/ /track/ paths. return _by_keyword('deezer') + if 'discogs.com' in host: + # Discogs paths are /artist/-Slug, /release/-Slug, + # /master/-Slug — the id is embedded with a slug, so strip to the + # leading number. (Discogs has no standalone track URLs; tracks live + # inside a release, so only artist/album resolve.) + out = [] + for t in _by_keyword('discogs'): + m = re.match(r'(\d+)', t.id) + if m: + out.append(t._replace(id=m.group(1))) + return out + return [] @@ -254,7 +267,7 @@ def _fetch_album(client: Any, source: str, identifier: str) -> Optional[dict]: (the modal re-fetches the full tracklist on open).""" if source == 'deezer': return client.get_album_metadata(identifier, include_tracks=False) - if source in ('itunes', 'musicbrainz'): + if source in ('itunes', 'musicbrainz', 'discogs'): return client.get_album(identifier, include_tracks=False) return client.get_album(identifier) # spotify @@ -274,8 +287,8 @@ def _fetch_artist(client: Any, source: str, identifier: str) -> Optional[dict]: # Shown in the dropdown's empty state so the user knows what to do next. _MSG_NOT_A_LINK = ( - 'Paste a full link from Spotify, Apple Music, MusicBrainz, or Deezer ' - '(a bare ID is ambiguous).' + 'Paste a full link from Spotify, Apple Music, Deezer, Discogs, or ' + 'MusicBrainz (a bare ID is ambiguous).' ) _MSG_NOT_FOUND = "Couldn't resolve that link — double-check it's correct." diff --git a/tests/search/test_search_by_id.py b/tests/search/test_search_by_id.py index 172cb5b8..07f1a425 100644 --- a/tests/search/test_search_by_id.py +++ b/tests/search/test_search_by_id.py @@ -449,3 +449,53 @@ def test_resolve_get_album_returning_none_yields_not_found(): ) assert res['available'] is False assert res['albums'] == [] + + +# ── Discogs (#813 — extend paste-link to Discogs) ────────────────────────── + +def test_parse_discogs_release_url_strips_slug(): + out = by_id.parse_metadata_identifier( + 'https://www.discogs.com/release/678910-Some-Album-Title') + assert out == [by_id.LookupTarget('discogs', 'album', '678910')] + + +def test_parse_discogs_master_url_is_album(): + out = by_id.parse_metadata_identifier( + 'https://www.discogs.com/master/555-A-Master') + assert out == [by_id.LookupTarget('discogs', 'album', '555')] + + +def test_parse_discogs_artist_url(): + out = by_id.parse_metadata_identifier( + 'https://www.discogs.com/artist/12345-Some-Artist') + assert out == [by_id.LookupTarget('discogs', 'artist', '12345')] + + +def test_parse_discogs_url_no_scheme(): + out = by_id.parse_metadata_identifier('discogs.com/release/999-X') + assert out == [by_id.LookupTarget('discogs', 'album', '999')] + + +def test_resolve_discogs_release_uses_get_album_with_numeric_id(): + client = _FakeClient(album={'id': '678910', 'name': 'Some Album', + 'artists': [{'name': 'Some Artist'}]}) + res = by_id.resolve_identifier( + 'https://www.discogs.com/release/678910-Some-Album-Title', deps=None, + client_resolver=_resolver_from({'discogs': client})) + assert res['available'] is True and res['source'] == 'discogs' + assert res['albums'] and res['albums'][0]['name'] == 'Some Album' + assert client.album_calls == ['678910'] # numeric id, slug stripped + + +def test_resolve_discogs_artist(): + client = _FakeClient(artist={'id': '12345', 'name': 'Some Artist'}) + res = by_id.resolve_identifier( + 'https://www.discogs.com/artist/12345-Some-Artist', deps=None, + client_resolver=_resolver_from({'discogs': client})) + assert res['available'] is True and res['source'] == 'discogs' + assert res['artists'] and res['artists'][0]['name'] == 'Some Artist' + assert client.artist_calls == ['12345'] + + +def test_discogs_in_supported_sources(): + assert 'discogs' in by_id.SUPPORTED_SOURCES From cea0e9d63c5c9ea8807cce28553518e8d14e08d5 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Mon, 8 Jun 2026 15:03:54 -0700 Subject: [PATCH 13/45] Manual download search: paste a Tidal/Qobuz track link to grab the exact version (#813) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a track shows "Not found", the manual search now accepts a pasted Tidal or Qobuz track link, not just a typed query (CubeComming: the fuzzy search misses versions; he can find the track on Tidal but can't get it to appear). How it works (robust, reuses the proven path): parse the link → (source, track_id) → fetch the track via the source client's get_track → build a clean "artist title (version)" query → run THAT source's normal search → bubble the result whose id matches the link to the top. So the candidate is a normal, already-downloadable streaming result — no hand-built download encoding — and it downloads through the existing verified flow. Degrades gracefully: if the source isn't connected or the link can't be resolved, it falls back to a normal text search of the raw input — the user is never worse off than typing it themselves. Scoped to Tidal + Qobuz (the streaming sources that download by track id, with public track URLs); Soulseek can't take a link (P2P, no ids), YouTube/SoundCloud are URL-native via a different path (future). - core/downloads/track_link.py: pure parse_download_track_link (tidal/qobuz /track/, slug/region suffixes, scheme-less) + query_from_track_payload (per-source title/artist, Tidal version-append). - manual-search endpoint: link detection → resolve → restrict to that source → id-match bubble. - placeholder hint mentions pasting a link; maxlength 200→300 for long URLs. Tests: 14 (parser shapes + payload extraction incl. remix version-append + qobuz performer/album-artist fallback). JS valid. --- core/downloads/track_link.py | 97 ++++++++++++++++++++++++++++++ tests/downloads/test_track_link.py | 80 ++++++++++++++++++++++++ web_server.py | 49 +++++++++++++++ webui/static/downloads.js | 4 +- 4 files changed, 228 insertions(+), 2 deletions(-) create mode 100644 core/downloads/track_link.py create mode 100644 tests/downloads/test_track_link.py diff --git a/core/downloads/track_link.py b/core/downloads/track_link.py new file mode 100644 index 00000000..e6dec8bd --- /dev/null +++ b/core/downloads/track_link.py @@ -0,0 +1,97 @@ +"""Recognize a pasted streaming-source track link in the manual download +search (#813). + +A user pastes e.g. ``https://tidal.com/track/434945950/u`` instead of typing a +query, to grab the exact version. We only recognize sources that download by +track ID (Tidal, Qobuz) — the manual search then resolves the link to that +track and runs the source's own search so the result is a normal, downloadable +candidate (no hand-built download encoding). + +Pure + import-safe: parsing only, no network. +""" + +from __future__ import annotations + +import re +from typing import Any, Optional, Tuple +from urllib.parse import urlparse + +# host substring → download source id. Only ID-downloadable streaming sources. +_HOSTS = ( + ('tidal.com', 'tidal'), + ('qobuz.com', 'qobuz'), +) + + +def parse_download_track_link(raw: str) -> Optional[Tuple[str, str]]: + """Parse a pasted Tidal/Qobuz track URL into ``(source, track_id)``. + + Returns None when the input isn't a recognized track link (so the caller + falls back to a normal text search). Handles the common URL shapes: + ``tidal.com/track/[/u]``, ``listen.tidal.com/track/``, + ``tidal.com/browse/track/``, ``open.qobuz.com/track/``, + ``play.qobuz.com/track/`` — with or without the scheme. + """ + raw = (raw or '').strip() + if not raw: + return None + + lowered = raw.lower() + if '://' not in raw and not any(h in lowered for h, _ in _HOSTS): + return None # not even a URL we care about + + url = raw if '://' in raw else f'https://{raw}' + parsed = urlparse(url) + host = (parsed.netloc or '').lower() + + source = next((sid for h, sid in _HOSTS if h in host), None) + if not source: + return None + + segs = [s for s in (parsed.path or '').split('/') if s] + for i, seg in enumerate(segs): + if seg.lower() == 'track' and i + 1 < len(segs): + m = re.match(r'(\d+)', segs[i + 1]) # id may carry a slug/suffix + if m: + return (source, m.group(1)) + return None + + +def _first_artist_name(value: Any) -> str: + """First artist name from a list of {'name': ...}/strings, or a single + {'name': ...}/string.""" + if isinstance(value, list): + value = value[0] if value else None + if isinstance(value, dict): + return str(value.get('name') or '') + return str(value or '') + + +def query_from_track_payload(source: str, raw: Any) -> Optional[str]: + """Build a clean ``"artist title"`` search query from a source ``get_track`` + payload — pure, so the per-source shape parsing is unit-testable without a + live client. + + - Tidal: attributes dict (``title`` + optional ``version`` + maybe + ``artists``/``artist``). The version is appended so a remix link searches + for the remix. + - Qobuz: track dict (``title`` + ``performer``/``album.artist``). + """ + if not isinstance(raw, dict): + return None + title = (raw.get('title') or '').strip() + artist = '' + + if source == 'tidal': + version = (raw.get('version') or '').strip() + if version and version.lower() not in title.lower(): + title = f"{title} ({version})" if title else version + artist = _first_artist_name(raw.get('artists') or raw.get('artist')) + elif source == 'qobuz': + artist = _first_artist_name(raw.get('performer')) + if not artist: + album = raw.get('album') if isinstance(raw.get('album'), dict) else {} + artist = _first_artist_name(album.get('artist')) + + query = f"{artist} {title}".strip() + return query or (title or None) diff --git a/tests/downloads/test_track_link.py b/tests/downloads/test_track_link.py new file mode 100644 index 00000000..39dfe8a1 --- /dev/null +++ b/tests/downloads/test_track_link.py @@ -0,0 +1,80 @@ +"""Parse pasted Tidal/Qobuz track links for the manual download search (#813).""" + +from core.downloads.track_link import parse_download_track_link as p + + +def test_tidal_track_url_with_region_suffix(): + assert p('https://tidal.com/track/434945950/u') == ('tidal', '434945950') + + +def test_tidal_browse_and_listen_hosts(): + assert p('https://tidal.com/browse/track/434945950') == ('tidal', '434945950') + assert p('https://listen.tidal.com/track/434945950') == ('tidal', '434945950') + + +def test_qobuz_track_urls(): + assert p('https://open.qobuz.com/track/12345678') == ('qobuz', '12345678') + assert p('https://play.qobuz.com/track/12345678') == ('qobuz', '12345678') + + +def test_scheme_less(): + assert p('tidal.com/track/999') == ('tidal', '999') + + +def test_id_with_slug_suffix(): + assert p('https://www.qobuz.com/track/555-some-slug') == ('qobuz', '555') + + +def test_non_track_links_rejected(): + assert p('https://tidal.com/album/123') is None # album, not track + assert p('https://tidal.com/artist/123') is None + assert p('https://open.spotify.com/track/abc') is None # unsupported source + assert p('https://example.com/track/123') is None + + +def test_garbage_rejected(): + assert p('') is None + assert p('just some text') is None + assert p('Habbit (T-Mass Remix)') is None + + +# ── query_from_track_payload (pure per-source parsing) ── + +from core.downloads.track_link import query_from_track_payload as q + + +def test_tidal_payload_appends_version(): + # Tidal attributes: title + version → remix link searches for the remix. + raw = {'title': 'Habbit', 'version': 'T-Mass Remix', + 'artists': [{'name': 'Rain Man'}, {'name': 'Krysta Youngs'}]} + assert q('tidal', raw) == 'Rain Man Habbit (T-Mass Remix)' + + +def test_tidal_payload_no_version_no_artist(): + assert q('tidal', {'title': 'Bloom'}) == 'Bloom' + + +def test_tidal_payload_singular_artist(): + assert q('tidal', {'title': 'X', 'artist': {'name': 'Jinco'}}) == 'Jinco X' + + +def test_tidal_version_already_in_title_not_doubled(): + raw = {'title': 'Bloom (Nurko Remix)', 'version': 'Nurko Remix', + 'artists': [{'name': 'Dabin'}]} + assert q('tidal', raw) == 'Dabin Bloom (Nurko Remix)' + + +def test_qobuz_payload_performer(): + raw = {'title': "What's Good For Me", 'performer': {'name': 'Jinco'}} + assert q('qobuz', raw) == "Jinco What's Good For Me" + + +def test_qobuz_payload_falls_back_to_album_artist(): + raw = {'title': 'Song', 'album': {'artist': {'name': 'Some Artist'}}} + assert q('qobuz', raw) == 'Some Artist Song' + + +def test_payload_non_dict_or_empty(): + assert q('tidal', None) is None + assert q('tidal', {}) is None + assert q('qobuz', 'garbage') is None diff --git a/web_server.py b/web_server.py index 56b1566e..3d1961c6 100644 --- a/web_server.py +++ b/web_server.py @@ -7083,6 +7083,28 @@ def download_selected_candidate(task_id): return jsonify({"error": str(e)}), 500 +def _resolve_link_track_query(source: str, track_id: str): + """Resolve a pasted (source, track_id) to a clean "artist title" search + query via the source client's get_track (#813). Returns (query, None) or + (None, error). Used so a pasted Tidal/Qobuz link runs the source's normal + search (proven-downloadable candidates) instead of a hand-built one.""" + from core.downloads.track_link import query_from_track_payload + client = download_orchestrator.client(source) if download_orchestrator else None + if not client or not hasattr(client, 'get_track'): + return None, f"{source.title()} is not connected" + try: + raw = client.get_track(track_id) + except Exception as e: + return None, f"Could not resolve {source.title()} track: {e}" + if not raw: + return None, f"{source.title()} track {track_id} not found" + + query = query_from_track_payload(source, raw) + if not query: + return None, f"Could not read the track title from {source.title()}" + return query, None + + @app.route('/api/downloads/task//manual-search', methods=['POST']) def manual_search_for_task(task_id): """Run a user-driven search against one (or all) configured download @@ -7122,6 +7144,26 @@ def manual_search_for_task(task_id): download_mode, available_sources = _list_available_download_sources() valid_source_ids = {s['id'] for s in available_sources} + # Pasted streaming-source track link (#813): resolve it to a clean + # "artist title" query and search ONLY that source, then bubble the + # exact track to the top. Falls back to a normal text search if the + # source isn't connected or the link can't be resolved — so the user is + # never worse off than typing the query themselves. + from core.downloads.track_link import parse_download_track_link + link = parse_download_track_link(query) + link_source = None + link_track_id = None + if link: + _src, _tid = link + if _src in valid_source_ids: + clean_q, link_err = _resolve_link_track_query(_src, _tid) + if clean_q: + query = clean_q + source = _src + link_source, link_track_id = _src, _tid + else: + logger.info(f"[Manual Search] link resolve fell back: {link_err}") + if source != 'all': if source not in valid_source_ids: return jsonify({ @@ -7181,6 +7223,13 @@ def manual_search_for_task(task_id): "error": error, }) + "\n" continue + # Pasted-link exact match: bubble the track whose id matches + # the link to the top so the user sees the exact version + # first (graceful no-op if ids don't line up). + if src_name == link_source and link_track_id and tracks: + tracks = sorted( + tracks, + key=lambda t: str(getattr(t, 'id', '')) != str(link_track_id)) serialized = [] for t in tracks: s = _serialize_candidate(t, source_override=src_name) diff --git a/webui/static/downloads.js b/webui/static/downloads.js index 5d5ad688..aeca1bd1 100644 --- a/webui/static/downloads.js +++ b/webui/static/downloads.js @@ -3127,8 +3127,8 @@ function _renderCandidatesModal(data) { + placeholder="Search, or paste a Tidal / Qobuz track link..." + maxlength="300" /> ${sourceControl} +
+ +
+
Runs the background metadata enrichment worker on the no-auth Spotify source instead of this connected account — keeping bulk enrichment off your official API quota (and dodging rate-limit bans), so your account is reserved for interactive search and playlist sync. On by default. Turn off to enrich through your connected account instead. Works even with no account connected. Note: the no-auth source can't supply artist genres.
+
+
diff --git a/webui/static/settings.js b/webui/static/settings.js index d9d1e33c..f1212f2e 100644 --- a/webui/static/settings.js +++ b/webui/static/settings.js @@ -1062,7 +1062,9 @@ async function loadSettingsData() { ? 'spotify_free' : _fbSrc; document.getElementById('metadata-fallback-source').value = _metaSel; const _efEl = document.getElementById('metadata-spotify-free-enrichment'); - if (_efEl) _efEl.checked = settings.metadata?.spotify_free_enrichment === true; + // Default ON: unset (undefined) reads as enabled, matching the worker's + // config default (metadata.spotify_free_enrichment defaults True). + if (_efEl) _efEl.checked = settings.metadata?.spotify_free_enrichment !== false; // Populate Hydrabase settings const hbConfig = settings.hydrabase || {}; From 26368a80ab24f25f25bb924db9b40b637dde3fd0 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 9 Jun 2026 13:08:41 -0700 Subject: [PATCH 30/45] Dead File Cleaner: don't flag a whole library when paths just aren't reachable (#828) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit macstainless: a Plex-on-macOS user running SoulSync in Docker had all 5,250 tracks flagged "dead" even though the files exist and play in Plex. Root cause: the DB stores paths as Plex reported them (/Volumes/Core/Music/...), which don't exist inside SoulSync's container. resolve_library_file_path() returns None for "couldn't find it at any known base dir" — and for a mis-mounted library that's EVERY track, not a deletion. The job treated None as "file deleted" and created a finding per track. Fix mirrors the existing transfer-folder abort: collect unresolvable tracks, and if at least max_unresolved_fraction (default 0.5) of the library is unresolvable once it's past min_tracks_for_guard (default 25), treat it as a path-mapping/ mount problem — abort with an actionable message (Docker mount / Settings → Library → Music Paths) and create ZERO findings. A small fraction unresolvable is still reported as genuine dead files, and tiny libraries (< min) report as before. Both thresholds are configurable per the job's settings. Tests: mass-unresolvable aborts with no findings; a lone dead file among real ones is still reported; a small all-dead library still reports; thresholds configurable. 54 repair tests pass. --- core/repair_jobs/dead_file_cleaner.py | 132 ++++++++++++++++++------- tests/test_dead_file_cleaner_guard.py | 135 ++++++++++++++++++++++++++ 2 files changed, 232 insertions(+), 35 deletions(-) create mode 100644 tests/test_dead_file_cleaner_guard.py diff --git a/core/repair_jobs/dead_file_cleaner.py b/core/repair_jobs/dead_file_cleaner.py index f0b3fb9d..ac9a0263 100644 --- a/core/repair_jobs/dead_file_cleaner.py +++ b/core/repair_jobs/dead_file_cleaner.py @@ -36,7 +36,17 @@ class DeadFileCleanerJob(RepairJob): icon = 'repair-icon-deadfile' default_enabled = True default_interval_hours = 24 - default_settings = {} + default_settings = { + # Mass-false-positive guard: if at least this fraction of tracks resolve + # to no file on disk, treat it as a path-mapping/mount problem (SoulSync + # can't SEE the library — e.g. Docker, or library.music_paths unset for + # this environment) rather than thousands of individually-deleted files, + # and abort without creating findings. Mirrors the transfer-folder abort. + 'max_unresolved_fraction': 0.5, + # ...but only once the library is at least this big — a small library can + # legitimately have a high dead fraction. + 'min_tracks_for_guard': 25, + } auto_fix = False def scan(self, context: JobContext) -> JobResult: @@ -87,9 +97,30 @@ class DeadFileCleanerJob(RepairJob): if context.config_manager: download_folder = context.config_manager.get('soulseek.download_path', '') + # Mass-false-positive guard thresholds (see default_settings). + max_unresolved_fraction = 0.5 + min_tracks_for_guard = 25 + if context.config_manager: + try: + max_unresolved_fraction = float(context.config_manager.get( + self.get_config_key('max_unresolved_fraction'), 0.5)) + except (TypeError, ValueError): + max_unresolved_fraction = 0.5 + try: + min_tracks_for_guard = int(context.config_manager.get( + self.get_config_key('min_tracks_for_guard'), 25)) + except (TypeError, ValueError): + min_tracks_for_guard = 25 + if context.report_progress: context.report_progress(phase=f'Checking {total} tracks...', total=total) + # Collect unresolvable tracks first; decide whether they're genuine dead + # files or a systemic path problem AFTER the full pass (below). A "None" + # from the resolver means "couldn't find it at any known base dir" — which + # for a mis-mounted library is EVERY track, not a real deletion. + dead_rows = [] + for i, row in enumerate(tracks): if context.check_stop(): return result @@ -112,44 +143,75 @@ class DeadFileCleanerJob(RepairJob): config_manager=context.config_manager) if resolved is None: - # File is truly missing — create finding - if context.report_progress: - context.report_progress( - log_line=f'Missing: {title or "Unknown"} — {os.path.basename(file_path)}', - log_type='error' - ) - if context.create_finding: - try: - inserted = context.create_finding( - job_id=self.job_id, - finding_type='dead_file', - severity='warning', - entity_type='track', - entity_id=str(track_id), - file_path=file_path, - title=f'Missing file: {title or "Unknown"}', - description=f'Track "{title}" by {artist_name or "Unknown"} points to a file that no longer exists', - details={ - 'track_id': track_id, - 'title': title, - 'artist': artist_name, - 'album': album_title, - 'original_path': file_path, - 'album_thumb_url': album_thumb or None, - 'artist_thumb_url': artist_thumb or None, - } - ) - if inserted: - result.findings_created += 1 - else: - result.findings_skipped_dedup += 1 - except Exception as e: - logger.debug("Error creating dead file finding for track %s: %s", track_id, e) - result.errors += 1 + dead_rows.append(row) if context.update_progress and (i + 1) % 100 == 0: context.update_progress(i + 1, total) + # Mass-false-positive guard: a large fraction of unresolvable paths almost + # always means SoulSync can't SEE the library (Docker mount, or + # Settings → Library → Music Paths not set for this environment), NOT that + # thousands of files were individually deleted. Refuse to flag and say so + # — same principle as the transfer-folder abort above. (#828: a Plex-on- + # macOS user in Docker had all 5,250 tracks flagged because their stored + # /Volumes/... paths don't exist inside the container.) + if (dead_rows + and result.scanned >= min_tracks_for_guard + and len(dead_rows) >= result.scanned * max_unresolved_fraction): + logger.error( + "Dead file scan: %d/%d tracks unresolvable (>= %.0f%%) — aborting without " + "creating findings; this is a path-mapping/mount problem, not deleted files.", + len(dead_rows), result.scanned, max_unresolved_fraction * 100) + result.errors += 1 + if context.report_progress: + context.report_progress( + phase='Aborted — too many unreachable paths', + log_line=(f"{len(dead_rows)} of {result.scanned} tracks point to paths SoulSync " + f"can't reach — almost always a path-mapping issue (Docker mount, or " + f"Settings → Library → Music Paths), not deleted files. No findings created."), + log_type='error' + ) + if context.update_progress: + context.update_progress(total, total) + return result + + # A small fraction unresolvable — treat as genuine dead files and report. + for row in dead_rows: + track_id, title, artist_name, album_title, file_path, album_thumb, artist_thumb = row + if context.report_progress: + context.report_progress( + log_line=f'Missing: {title or "Unknown"} — {os.path.basename(file_path)}', + log_type='error' + ) + if context.create_finding: + try: + inserted = context.create_finding( + job_id=self.job_id, + finding_type='dead_file', + severity='warning', + entity_type='track', + entity_id=str(track_id), + file_path=file_path, + title=f'Missing file: {title or "Unknown"}', + description=f'Track "{title}" by {artist_name or "Unknown"} points to a file that no longer exists', + details={ + 'track_id': track_id, + 'title': title, + 'artist': artist_name, + 'album': album_title, + 'original_path': file_path, + 'album_thumb_url': album_thumb or None, + 'artist_thumb_url': artist_thumb or None, + } + ) + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 + except Exception as e: + logger.debug("Error creating dead file finding for track %s: %s", track_id, e) + result.errors += 1 + if context.update_progress: context.update_progress(total, total) diff --git a/tests/test_dead_file_cleaner_guard.py b/tests/test_dead_file_cleaner_guard.py new file mode 100644 index 00000000..1ea90afe --- /dev/null +++ b/tests/test_dead_file_cleaner_guard.py @@ -0,0 +1,135 @@ +"""Dead File Cleaner — mass-false-positive guard (#828). + +macstainless: a Plex-on-macOS user running SoulSync in Docker had all 5,250 +tracks flagged "dead" because their stored /Volumes/... paths don't exist inside +the container. The resolver returning None means "couldn't find it at any known +base dir" — for a mis-mounted library that's EVERY track, not a real deletion. +The job now refuses to flag when a large fraction is unresolvable (a path-mapping +problem) and reports it as such, mirroring the existing transfer-folder abort. +""" + +from __future__ import annotations + +from core.repair_jobs.base import JobContext +from core.repair_jobs.dead_file_cleaner import DeadFileCleanerJob + + +class _Cur: + def __init__(self, rows): + self._rows = rows + + def execute(self, *a, **k): + pass + + def fetchall(self): + return self._rows + + def fetchone(self): + return [len(self._rows)] + + def close(self): + pass + + +class _Conn: + def __init__(self, rows): + self._rows = rows + + def cursor(self): + return _Cur(self._rows) + + def close(self): + pass + + +class _Db: + def __init__(self, rows): + self._rows = rows + + def _get_connection(self): + return _Conn(self._rows) + + +class _Cfg: + def __init__(self, overrides=None): + self._o = overrides or {} + + def get(self, key, default=None): + return self._o.get(key, default) + + +def _row(i, path): + # (track_id, title, artist, album, file_path, album_thumb, artist_thumb) + return (i, f"Track {i}", "Yellowcard", "Ocean Avenue", path, None, None) + + +def _run(rows, transfer_folder, cfg_overrides=None): + findings = [] + cfg = _Cfg({'soulseek.download_path': '', **(cfg_overrides or {})}) + ctx = JobContext( + db=_Db(rows), + transfer_folder=str(transfer_folder), + config_manager=cfg, + create_finding=lambda **kw: (findings.append(kw) or True), + ) + res = DeadFileCleanerJob().scan(ctx) + return res, findings + + +def test_mass_unresolvable_aborts_without_findings(tmp_path): + # 30 tracks all pointing to a /Volumes path that doesn't exist in this env + # -> systemic path problem -> abort, zero findings, one error. + rows = [_row(i, f"/Volumes/Core/Music/Plex/Yellowcard/{i}.mp3") for i in range(30)] + res, findings = _run(rows, tmp_path) + assert findings == [] + assert res.findings_created == 0 + assert res.errors >= 1 + assert res.scanned == 30 + + +def test_few_unresolvable_creates_findings(tmp_path): + # 4 real (resolvable) files + 1 genuinely missing -> fraction 0.2 < 0.5 -> + # the one dead file IS reported. + rows = [] + for i in range(4): + f = tmp_path / f"real_{i}.mp3" + f.write_text("x") + rows.append(_row(i, str(f))) + rows.append(_row(99, "/no/such/path/dead.mp3")) + res, findings = _run(rows, tmp_path, + {'repair.jobs.dead_file_cleaner.min_tracks_for_guard': 4}) + assert res.findings_created == 1 + assert len(findings) == 1 + assert findings[0]['entity_id'] == '99' + assert res.errors == 0 + + +def test_small_library_all_dead_still_reports(tmp_path): + # 3 dead tracks, below the default min_tracks_for_guard (25) -> guard doesn't + # apply -> all 3 reported (a tiny library can legitimately be all-dead). + rows = [_row(i, f"/no/such/{i}.mp3") for i in range(3)] + res, findings = _run(rows, tmp_path) + assert res.findings_created == 3 + + +def test_guard_thresholds_configurable(tmp_path): + # Lower min to 4; all 4 dead -> fraction 1.0 >= 0.5 -> abort. + rows = [_row(i, f"/no/such/{i}.mp3") for i in range(4)] + res, findings = _run(rows, tmp_path, + {'repair.jobs.dead_file_cleaner.min_tracks_for_guard': 4}) + assert res.findings_created == 0 + assert res.errors >= 1 + + +def test_healthy_library_no_abort_no_findings(tmp_path): + # 30 fully-resolvable tracks -> 0 dead -> neither aborts nor flags anything. + rows = [] + for i in range(30): + f = tmp_path / f"ok_{i}.mp3" + f.write_text("x") + rows.append(_row(i, str(f))) + res, findings = _run(rows, tmp_path) + assert res.findings_created == 0 + assert res.errors == 0 + assert res.scanned == 30 + assert findings == [] From 1d16ac79783d3fee0694a7696ec84c88dd8f00f5 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 9 Jun 2026 13:47:25 -0700 Subject: [PATCH 31/45] Downloads: reuse an album's existing folder so batches don't split it (#829) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tacobell444: when tracks land in an album across multiple batches (a wishlist run, the Album Completeness job, a missed track re-downloaded later), the folder is rebuilt from API metadata each time — so when $albumtype or $year come back blank/different on a later batch, the folder NAME changes and the album splits, forcing a Reorganize. Fix: build_final_path_for_track now checks whether the album already lives in a single folder on disk and, if so, drops the new track there instead of a freshly templated folder. Match (chosen): exact stored Spotify album id first, then a STRICT >=0.85 name+artist match (vs the 0.7 used elsewhere) — a wrong match here misplaces a file. New core/library/existing_album_folder.resolve_existing_album_folder holds the logic; always-on with template fallback. Safety rails: only returns a folder UNDER the transfer dir (never a read-only library/NAS mount), only when the album lives in EXACTLY ONE folder (multiple = disc subfolders, which DatabaseTrack can't disambiguate — those defer to the template), and any failure falls through to the template path. Added MusicDatabase.get_album_by_spotify_album_id for the id-first lookup. Tests: single-folder reuse, no-match, below-threshold, multi-folder defer, outside-transfer reject, id-first, missing transfer dir, no-files-on-disk. 8 tests; 1556 path/import/download tests pass (only the known soundcloud failures remain). --- core/imports/paths.py | 39 ++++++++ core/library/existing_album_folder.py | 129 ++++++++++++++++++++++++++ database/music_database.py | 40 +++++++- tests/test_existing_album_folder.py | 114 +++++++++++++++++++++++ 4 files changed, 320 insertions(+), 2 deletions(-) create mode 100644 core/library/existing_album_folder.py create mode 100644 tests/test_existing_album_folder.py diff --git a/core/imports/paths.py b/core/imports/paths.py index 5744744e..6546509f 100644 --- a/core/imports/paths.py +++ b/core/imports/paths.py @@ -587,6 +587,45 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext, cr disc_label = _get_config_manager().get("file_organization.disc_label", "Disc") folder_path, filename_base = get_file_path_from_template(template_context, "album_path") + + # #829: if this album already lives in a single folder on disk, drop the + # new track there instead of a freshly-templated folder — this is what + # keeps an album from splitting when $albumtype/$year drift between + # batches (wishlist, Album Completeness, a missed track later). Strict + # match + transfer-dir-only + single-folder-only inside the resolver; + # any miss falls through to the template path below. Best-effort. + reuse_folder = None + if filename_base: + try: + from core.library.existing_album_folder import resolve_existing_album_folder + from database.music_database import get_database + try: + _active_server = _get_config_manager().get_active_media_server() + except Exception: + _active_server = None + _spotify_album_id = (album_context.get("id") + if album_context and str(source).startswith("spotify") else None) + _expected_tracks = None + if album_context and album_context.get("total_tracks"): + _expected_tracks = _coerce_int(album_context.get("total_tracks"), 0) or None + reuse_folder = resolve_existing_album_folder( + db=get_database(), + transfer_dir=transfer_dir, + album_name=album_info.get("album_name"), + album_artist=template_context.get("albumartist"), + spotify_album_id=_spotify_album_id, + active_server=_active_server, + expected_track_count=_expected_tracks, + config_manager=_get_config_manager(), + ) + except Exception as _reuse_err: + logger.debug("[Existing Album Folder] lookup failed: %s", _reuse_err) + reuse_folder = None + if reuse_folder and filename_base: + final_path = os.path.join(reuse_folder, filename_base + file_ext) + _ensure_dir(reuse_folder, exist_ok=True) + return final_path, True + if folder_path and filename_base: if total_discs > 1 and not user_controls_disc: disc_folder = f"{disc_label} {disc_number}" diff --git a/core/library/existing_album_folder.py b/core/library/existing_album_folder.py new file mode 100644 index 00000000..926ab173 --- /dev/null +++ b/core/library/existing_album_folder.py @@ -0,0 +1,129 @@ +"""Reuse an album's existing on-disk folder for new downloads (#829). + +When tracks are added to an album across multiple batches (a wishlist run, the +Album Completeness job, a missed track re-downloaded later), the destination +folder is normally rebuilt from API metadata each time. If ``$albumtype`` or +``$year`` come back blank/different on a later batch, the folder *name* changes +and the album splits across folders — forcing a Reorganize afterwards. + +This resolves the folder the album *already* lives in so the new track joins its +existing files instead. Matching is deliberately conservative: the exact stored +Spotify album id first (definitive), then a STRICT (>= 0.85) name+artist match — +higher than the 0.7 used elsewhere, because a wrong match here misplaces a file. + +Safety rails: + * Only ever returns a folder UNDER the transfer dir (the managed download + tree) — never a read-only library/NAS mount the resolver happens to find. + * Only reuses when the album lives in EXACTLY ONE folder on disk. Multiple + folders means disc subfolders (DatabaseTrack carries no disc number, so we + can't safely pick the right one) — those defer to the template path. + * Any failure returns None — the caller falls back to the normal template. +""" + +from __future__ import annotations + +import os +from typing import Any, Optional + +from core.library.path_resolver import resolve_library_file_path +from utils.logging_config import get_logger + +logger = get_logger("library.existing_album_folder") + +# Strict — a wrong album match drops the file in the wrong folder. +_STRICT_ALBUM_CONFIDENCE = 0.85 + + +def _is_under(child: str, parent: str) -> bool: + """True if ``child`` is the same as or inside ``parent`` (normalized).""" + try: + child_n = os.path.normcase(os.path.normpath(os.path.abspath(child))) + parent_n = os.path.normcase(os.path.normpath(os.path.abspath(parent))) + return child_n == parent_n or child_n.startswith(parent_n + os.sep) + except Exception: + return False + + +def _find_album(db: Any, spotify_album_id: Optional[str], album_name: Optional[str], + album_artist: Optional[str], active_server: Optional[str], + expected_track_count: Optional[int]): + """Stored Spotify id first, then a strict name+artist match. None on no match.""" + if spotify_album_id: + try: + album = db.get_album_by_spotify_album_id(spotify_album_id) + if album: + return album + except Exception as e: + logger.debug("album-by-spotify-id lookup failed: %s", e) + if album_name and album_artist: + try: + match, confidence = db.check_album_exists_with_editions( + title=album_name, artist=album_artist, + confidence_threshold=_STRICT_ALBUM_CONFIDENCE, + expected_track_count=expected_track_count, + server_source=active_server, + ) + if match and confidence >= _STRICT_ALBUM_CONFIDENCE: + return match + except Exception as e: + logger.debug("strict album name+artist match failed: %s", e) + return None + + +def resolve_existing_album_folder( + *, + db: Any, + transfer_dir: Optional[str], + album_name: Optional[str] = None, + album_artist: Optional[str] = None, + spotify_album_id: Optional[str] = None, + active_server: Optional[str] = None, + expected_track_count: Optional[int] = None, + config_manager: Any = None, + resolver=resolve_library_file_path, +) -> Optional[str]: + """Return the on-disk folder an existing album lives in (so a new track joins + it) or None to fall back to the templated path. See module docstring.""" + if not transfer_dir or not os.path.isdir(transfer_dir): + return None + if not db: + return None + + album = _find_album(db, spotify_album_id, album_name, album_artist, + active_server, expected_track_count) + if not album: + return None + + try: + tracks = db.get_tracks_by_album(album.id) + except Exception as e: + logger.debug("get_tracks_by_album(%s) failed: %s", getattr(album, 'id', '?'), e) + return None + + folders = set() + for t in tracks: + file_path = getattr(t, 'file_path', None) + if not file_path: + continue + try: + resolved = resolver(file_path, transfer_folder=transfer_dir, + config_manager=config_manager) + except Exception: + resolved = None + if not resolved: + continue + folder = os.path.dirname(resolved) + if _is_under(folder, transfer_dir): + folders.add(os.path.normpath(folder)) + + # Single folder under the transfer dir → reuse it. Zero (nothing on disk yet) + # or many (disc subfolders) → let the template decide. + if len(folders) == 1: + reuse = next(iter(folders)) + logger.info("[Existing Album Folder] Reusing '%s' for album '%s'", + reuse, getattr(album, 'title', album_name)) + return reuse + return None + + +__all__ = ["resolve_existing_album_folder"] diff --git a/database/music_database.py b/database/music_database.py index 5acbf0af..202b2d4c 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -6260,11 +6260,47 @@ class MusicDatabase: )) return tracks - + except Exception as e: logger.error(f"Error getting tracks for album {album_id}: {e}") return [] - + + def get_album_by_spotify_album_id(self, spotify_album_id: str) -> Optional[DatabaseAlbum]: + """Fetch a single album by its (enriched) Spotify album id, or None. + + Used by the download path builder (#829) to reuse an album's existing + on-disk folder when re-downloading into the same album — matching the + exact stored Spotify id before falling back to fuzzy name+artist. + """ + if not spotify_album_id: + return None + try: + conn = self._get_connection() + cursor = conn.cursor() + cursor.execute(""" + SELECT albums.*, artists.name as artist_name + FROM albums + JOIN artists ON albums.artist_id = artists.id + WHERE albums.spotify_album_id = ? + LIMIT 1 + """, (spotify_album_id,)) + row = cursor.fetchone() + if not row: + return None + genres = json.loads(row['genres']) if row['genres'] else None + album = DatabaseAlbum( + id=row['id'], artist_id=row['artist_id'], title=row['title'], + year=row['year'], thumb_url=row['thumb_url'], genres=genres, + track_count=row['track_count'], duration=row['duration'], + created_at=datetime.fromisoformat(row['created_at']) if row['created_at'] else None, + updated_at=datetime.fromisoformat(row['updated_at']) if row['updated_at'] else None, + ) + album.artist_name = row['artist_name'] + return album + except Exception as e: + logger.error(f"Error getting album by spotify_album_id {spotify_album_id}: {e}") + return None + def search_artists(self, query: str, limit: int = 50, server_source: str = None) -> List[DatabaseArtist]: """Search artists by name, optionally filtered by server source. Uses diacritic-insensitive matching so 'Tiesto' finds 'Tiësto'.""" diff --git a/tests/test_existing_album_folder.py b/tests/test_existing_album_folder.py new file mode 100644 index 00000000..24b6f82b --- /dev/null +++ b/tests/test_existing_album_folder.py @@ -0,0 +1,114 @@ +"""Reuse an album's existing on-disk folder for new downloads (#829). + +Tacobell444: tracks added to an album across batches split into different folders +when $albumtype/$year drift. The resolver finds the album's existing single +folder (under the transfer dir) so the new track joins it. These pin the safety +rails: strict match, transfer-dir-only, single-folder-only, id-first. +""" + +from __future__ import annotations + +import os +from types import SimpleNamespace + +from core.library.existing_album_folder import resolve_existing_album_folder + + +class _FakeDb: + def __init__(self, album=None, album_conf=0.0, tracks=None, by_spotify=None): + self._album = album + self._album_conf = album_conf + self._tracks = tracks or [] + self._by_spotify = by_spotify + + def get_album_by_spotify_album_id(self, sid): + return self._by_spotify + + def check_album_exists_with_editions(self, title, artist, confidence_threshold=0.8, + expected_track_count=None, server_source=None, **kw): + return (self._album, self._album_conf) + + def get_tracks_by_album(self, album_id): + return self._tracks + + +def _track(path): + return SimpleNamespace(file_path=path) + + +def _album(id=1, title="Ocean Avenue"): + return SimpleNamespace(id=id, title=title) + + +def _mkfile(folder, name): + folder.mkdir(parents=True, exist_ok=True) + f = folder / name + f.write_text("x") + return str(f) + + +def test_reuses_single_folder_under_transfer(tmp_path): + album_dir = tmp_path / "Yellowcard - Ocean Avenue" + f1 = _mkfile(album_dir, "01 - Way Away.mp3") + f2 = _mkfile(album_dir, "02 - Breathing.mp3") + db = _FakeDb(album=_album(), album_conf=0.95, tracks=[_track(f1), _track(f2)]) + out = resolve_existing_album_folder( + db=db, transfer_dir=str(tmp_path), + album_name="Ocean Avenue", album_artist="Yellowcard") + assert out == os.path.normpath(str(album_dir)) + + +def test_no_match_returns_none(tmp_path): + db = _FakeDb(album=None, album_conf=0.0) + assert resolve_existing_album_folder( + db=db, transfer_dir=str(tmp_path), album_name="X", album_artist="Y") is None + + +def test_below_strict_threshold_returns_none(tmp_path): + f = _mkfile(tmp_path / "A", "1.mp3") + # 0.80 < the resolver's 0.85 strict gate -> not reused. + db = _FakeDb(album=_album(), album_conf=0.80, tracks=[_track(f)]) + assert resolve_existing_album_folder( + db=db, transfer_dir=str(tmp_path), album_name="A", album_artist="B") is None + + +def test_multi_folder_defers_to_template(tmp_path): + f1 = _mkfile(tmp_path / "Album" / "Disc 01", "1.mp3") + f2 = _mkfile(tmp_path / "Album" / "Disc 02", "1.mp3") + db = _FakeDb(album=_album(), album_conf=0.95, tracks=[_track(f1), _track(f2)]) + assert resolve_existing_album_folder( + db=db, transfer_dir=str(tmp_path), album_name="A", album_artist="B") is None + + +def test_folder_outside_transfer_returns_none(tmp_path): + f = _mkfile(tmp_path / "outside", "1.mp3") + transfer = tmp_path / "transfer" + transfer.mkdir() + db = _FakeDb(album=_album(), album_conf=0.95, tracks=[_track(f)]) + assert resolve_existing_album_folder( + db=db, transfer_dir=str(transfer), album_name="A", album_artist="B") is None + + +def test_id_first_match_skips_name_lookup(tmp_path): + album_dir = tmp_path / "Album" + f = _mkfile(album_dir, "1.mp3") + # name match would FAIL (album=None); the stored spotify id hits. + db = _FakeDb(album=None, album_conf=0.0, tracks=[_track(f)], by_spotify=_album()) + out = resolve_existing_album_folder( + db=db, transfer_dir=str(tmp_path), spotify_album_id="sp123", + album_name="X", album_artist="Y") + assert out == os.path.normpath(str(album_dir)) + + +def test_missing_transfer_dir_returns_none(tmp_path): + db = _FakeDb(album=_album(), album_conf=0.95, tracks=[]) + assert resolve_existing_album_folder( + db=db, transfer_dir=str(tmp_path / "nope"), album_name="A", album_artist="B") is None + + +def test_album_with_no_files_on_disk_returns_none(tmp_path): + # Album matched but its tracks have no resolvable file -> nothing to reuse. + db = _FakeDb(album=_album(), album_conf=0.95, + tracks=[_track("/gone/1.mp3"), _track(None)]) + assert resolve_existing_album_folder( + db=db, transfer_dir=str(tmp_path), album_name="A", album_artist="B") is None From db65e783c7acdabcf0a7a9f99db8a4104a0a1e2c Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 9 Jun 2026 14:35:54 -0700 Subject: [PATCH 32/45] Discography: keep collab tracks credited as one combined string (#830) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vicky-2418: Download Discography skipped some albums/singles as "No New Track" even on a brand-new artist, while manual one-by-one worked. The log showed the real reason wasn't ownership at all — it was "N skipped (artist mismatch)". Root cause (confirmed against the iTunes API, not guessed): iTunes returns a collab as ONE combined string. Narvent's "Miss You (Ambient Remix)" is credited 'TRVNSPORTER, Narvent & SKVLENT'. track_artist_matches did an exact full-string compare ('trvnsporter, narvent & skvlent' == 'narvent' → False), so every collaborator's discography entry was dropped — despite the #559 comment claiming it "keeps features" (it didn't, because features arrive combined, not as a list). Fix: match the requested artist as a COMPONENT of the credit — split on the common separators (, & ; / feat ft featuring vs x), while still including the full string so band names with internal separators ("Florence + the Machine") match exactly. Component matching stays exact per name, so true contamination (the #559 case — artist not credited at all) is still dropped, and substrings ("Drakeo the Ruler" ≠ "Drake") still don't match. Also corrects an over-conservative #559 test that asserted "Drake & Future" shouldn't match "Drake" — it should; Drake is a credited collaborator. The guard drops uncredited artists, not legit collabs packed into one string. Tests: the exact real case (Narvent in the combined credit) + each collaborator, contamination still dropped, feat/ft/featuring/x forms, no substring false positive. 36 filter tests + 747 discography/metadata tests pass. --- core/metadata/discography_filters.py | 42 ++++++++++++++++++---- tests/metadata/test_discography_filters.py | 41 ++++++++++++++++++--- 2 files changed, 72 insertions(+), 11 deletions(-) diff --git a/core/metadata/discography_filters.py b/core/metadata/discography_filters.py index 4e1e0ecd..c5985fdf 100644 --- a/core/metadata/discography_filters.py +++ b/core/metadata/discography_filters.py @@ -28,8 +28,31 @@ single source of truth. from __future__ import annotations +import re from typing import Any, Dict, List, Optional +# Split a combined artist credit into its individual artists. Sources like +# iTunes return collabs as ONE string ("TRVNSPORTER, Narvent & SKVLENT"), not a +# list — so an exact full-string compare drops every collaborator's discography +# entry (#830). Split on the common credit separators; " and " / " with " are +# deliberately excluded (too many real band names contain them). +_ARTIST_CREDIT_SPLIT_RE = re.compile( + r"\s*[,&;/]\s*|\s+(?:feat\.?|ft\.?|featuring|vs\.?|x)\s+", + re.IGNORECASE, +) + + +def _artist_credit_components(name: str) -> List[str]: + """Return the individual artist names within a (possibly combined) credit, + always including the full string itself (so exact band names with internal + separators still match).""" + name = (name or "").strip() + if not name: + return [] + parts = [name] + parts.extend(p.strip() for p in _ARTIST_CREDIT_SPLIT_RE.split(name) if p.strip()) + return parts + from core.watchlist_scanner import ( is_acoustic_version, is_instrumental_version, @@ -47,10 +70,12 @@ def track_artist_matches(track_artists: Any, requested_artist_name: str) -> bool what the discography fetch returns), or the list-of-dicts shape that some upstreams pass directly. Both are accepted. - Returns True for primary-artist tracks AND feature appearances — - the requested artist need only be one of the listed artists. Only - drops tracks where the requested artist isn't named at all (the - cross-artist compilation case from #559). + Returns True for primary-artist tracks AND feature/collab appearances — + the requested artist need only be one of the credited artists, INCLUDING + when a source (iTunes, etc.) packs the collab into one combined string like + "TRVNSPORTER, Narvent & SKVLENT" (#830). Only drops tracks where the + requested artist isn't credited at all (the cross-artist compilation case + from #559). """ if not requested_artist_name: # No artist to compare against — don't filter; let the caller @@ -70,8 +95,13 @@ def track_artist_matches(track_artists: Any, requested_artist_name: str) -> bool name = entry.get('name', '') or '' else: name = str(entry or '') - if name.strip().lower() == target: - return True + # Match the requested artist as a component of the credit, so combined + # collab strings ("A, B & C") keep B's discography entry. Component + # matching is still exact per-name, so true contamination (the artist + # genuinely absent) is dropped exactly as before. + for component in _artist_credit_components(name): + if component.strip().lower() == target: + return True return False diff --git a/tests/metadata/test_discography_filters.py b/tests/metadata/test_discography_filters.py index 400e0885..8c297d05 100644 --- a/tests/metadata/test_discography_filters.py +++ b/tests/metadata/test_discography_filters.py @@ -61,6 +61,35 @@ class TestTrackArtistMatches: assert track_artist_matches(['DRAKE'], 'Drake') is True assert track_artist_matches(['Drake'], 'drake') is True + def test_combined_collab_credit_string_matches_component(self): + """#830 (Vicky-2418): iTunes packs a collab into ONE string. The + requested artist is one of several credited — must match. This is the + exact real case from the report (Narvent's 'Miss You (Ambient Remix)').""" + credit = ['TRVNSPORTER, Narvent & SKVLENT'] + assert track_artist_matches(credit, 'Narvent') is True + assert track_artist_matches(credit, 'TRVNSPORTER') is True + assert track_artist_matches(credit, 'SKVLENT') is True + + def test_combined_credit_still_drops_absent_artist(self): + """The #559 contamination guard survives: an artist genuinely absent + from a combined credit is still dropped.""" + assert track_artist_matches(['TRVNSPORTER, Narvent & SKVLENT'], 'Drake') is False + + def test_feat_forms_match_component(self): + for credit in (['Drake feat. Narvent'], ['Drake ft. Narvent'], + ['Drake featuring Narvent'], ['Drake x Narvent']): + assert track_artist_matches(credit, 'Narvent') is True + + def test_no_substring_false_positive(self): + """Component matching is exact per-name — a longer name that merely + contains the target must NOT match.""" + assert track_artist_matches(['Narventos'], 'Narvent') is False + + def test_band_name_with_internal_separator_still_matches_exactly(self): + """A real band name containing a separator still matches as the full + string (we keep the whole credit as a candidate alongside the split).""" + assert track_artist_matches(['Florence + the Machine'], 'Florence + the Machine') is True + def test_match_handles_whitespace_padding(self): """Trailing whitespace in either side mustn't break the match.""" assert track_artist_matches([' Drake '], 'Drake') is True @@ -87,12 +116,14 @@ class TestTrackArtistMatches: assert track_artist_matches([{'name': 'Drake'}], 'Drake') is True assert track_artist_matches([{'name': 'Random'}], 'Drake') is False - def test_substring_does_not_match(self): - """A song by "Drake & Future" should not match "Drake" via - substring — that's exactly the false-positive case the bug - report describes. Exact full-name match only.""" - assert track_artist_matches(['Drake & Future'], 'Drake') is False + def test_substring_does_not_match_but_component_does(self): + """No SUBSTRING matching — "Drakeo the Ruler" must not match "Drake". + But a combined collab credit IS component-matched (#830): "Drake & + Future" matches "Drake" because Drake is genuinely one of the credited + artists. The #559 guard drops artists who aren't credited AT ALL, not + legit collaborators packed into one string by sources like iTunes.""" assert track_artist_matches(['Drakeo the Ruler'], 'Drake') is False + assert track_artist_matches(['Drake & Future'], 'Drake') is True # --------------------------------------------------------------------------- From ca040d2c1074314fb4de1781f3cadf6f901b2738 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 9 Jun 2026 14:53:30 -0700 Subject: [PATCH 33/45] Discography UI: show WHY tracks were skipped, not a flat "No new tracks" (#830) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-album status only looked at added + the generic wishlist-skip count, so anything else (other-artist credit, already owned, content-filtered) showed the misleading "No new tracks" — which is what made Vicky-2418's artist-mismatch skips look like "you already have it." The backend already streams the full breakdown (tracks_skipped_artist/owned/filter); the UI just ignored it. New shared _discogItemStatus(data) builds an accurate line from all the counts: "4 by other artists", "13 already owned", "1 added, 2 already owned", etc. Replaces the duplicated 4-line block at both render sites. Frontend only; JS syntax + per-case output verified. --- webui/static/library.js | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/webui/static/library.js b/webui/static/library.js index eb8dc6b0..34823ce6 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -2794,10 +2794,7 @@ async function startDiscographyDownload() { item.classList.remove('active'); if (data.status === 'done') { - const parts = []; - if (data.tracks_added > 0) parts.push(`${data.tracks_added} added`); - if (data.tracks_skipped > 0) parts.push(`${data.tracks_skipped} skipped`); - statusEl.textContent = parts.join(', ') || 'No new tracks'; + statusEl.textContent = _discogItemStatus(data); iconEl.innerHTML = data.tracks_added > 0 ? '✓' : '—'; item.classList.add(data.tracks_added > 0 ? 'done' : 'skipped'); } else if (data.status === 'error') { @@ -2814,6 +2811,22 @@ async function startDiscographyDownload() { } } +// Build a clear per-album status from the discography stream payload. The +// backend already reports WHY tracks weren't added — other-artist credit, +// already owned/queued, or content-filtered — so surface that instead of a +// misleading "No new tracks" (#830: collab tracks dropped for "artist mismatch" +// looked identical to "you already have it"). +function _discogItemStatus(data) { + const parts = []; + const added = data.tracks_added || 0; + if (added > 0) parts.push(`${added} added`); + if ((data.tracks_skipped_owned || 0) > 0) parts.push(`${data.tracks_skipped_owned} already owned`); + if ((data.tracks_skipped || 0) > 0) parts.push(`${data.tracks_skipped} already queued`); + if ((data.tracks_skipped_artist || 0) > 0) parts.push(`${data.tracks_skipped_artist} by other artists`); + if ((data.tracks_skipped_filter || 0) > 0) parts.push(`${data.tracks_skipped_filter} filtered out`); + return parts.join(', ') || 'No tracks'; +} + function _handleDiscogProgress(data) { if (data.type === 'album') { const item = document.getElementById(`discog-prog-${data.album_id}`); @@ -2826,10 +2839,7 @@ function _handleDiscogProgress(data) { statusEl.textContent = `Processing ${data.tracks_total} tracks...`; item.classList.add('active'); } else if (data.status === 'done') { - const parts = []; - if (data.tracks_added > 0) parts.push(`${data.tracks_added} added`); - if (data.tracks_skipped > 0) parts.push(`${data.tracks_skipped} skipped`); - statusEl.textContent = parts.join(', ') || 'No new tracks'; + statusEl.textContent = _discogItemStatus(data); iconEl.innerHTML = data.tracks_added > 0 ? '✓' : '—'; item.classList.remove('active'); item.classList.add(data.tracks_added > 0 ? 'done' : 'skipped'); From 0939585620d14cefe07f796b3ffb8d945988ce51 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 9 Jun 2026 15:38:49 -0700 Subject: [PATCH 34/45] Matcher: bracketed subtitles no longer read as different songs (#825) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit carlosjfcasero round 2 (manual-add fix didn't help — different path). His log pinned it: the mirrored sync auto-added 'Llamando a la tierra (Serenade From the Stars)' by M-Clan every run even though his library has the song (stored bare). Reproduced exactly: the subtitle restates no album context, so the #808 context strip keeps it, and the length-ratio penalty in _calculate_track_confidence crushes the pair to 0.142 (needs 0.7). Sync → "missing" → wishlist, forever; and the cleanup uses the SAME matcher, so it deterministically never removed it. Self-reinforcing. Fix at the matcher seam (benefits sync, cleanup, downloads, discography alike): core/text/title_match.strip_subtitle_qualifiers(title, other) strips a bracketed qualifier only when it (a) isn't restated in the other title, (b) contains no version-marker token (EN + ES: live/remix/acoustic/version/ dueto/directo/vivo/...), and (c) introduces no new digit token ('(Pt. 2)', '(2007)' stay different releases). Wired as a third comparison variant in _calculate_track_confidence with its own length guard. Verified against his log's other unmatched tracks: '(Live)' 0.15, '(Dueto 2007)' 0.179, '(Versión 1988)' 0.167 all still correctly blocked — version qualifiers keep their meaning; the M-Clan case goes 0.142 → 1.0 in both directions. Also: sync's check_track_exists call now passes album= (cleanup already did), enabling the album-aware fallback for multi-artist albums during sync. Tests: tests/test_subtitle_qualifier_match.py — the reported case verbatim (end-to-end through check_track_exists, both directions, batched candidate path included), EN+ES version qualifiers still blocked, numeric guard, '#769 Dani California' and '#808 OurVinyl' guards still hold. 1396 matcher/wishlist/sync tests pass. --- core/text/title_match.py | 72 +++++++++++ database/music_database.py | 18 +++ services/sync_service.py | 4 + tests/test_subtitle_qualifier_match.py | 158 +++++++++++++++++++++++++ 4 files changed, 252 insertions(+) create mode 100644 tests/test_subtitle_qualifier_match.py diff --git a/core/text/title_match.py b/core/text/title_match.py index 95f232f2..85e1c10b 100644 --- a/core/text/title_match.py +++ b/core/text/title_match.py @@ -115,6 +115,77 @@ def strip_redundant_context_qualifiers(title: str, *context_texts: str) -> str: return re.sub(r"\s+", " ", out).strip() +# Qualifier tokens that mark a genuinely DIFFERENT recording/cut — these must +# keep blocking a match. Union of the matching-engine keyword lists plus the +# Spanish markers seen in real libraries (#825: 'En Directo…', 'Versión 1988', +# 'Dueto 2007'). Titles reaching the matcher are unidecode-normalized, so the +# ASCII forms ('version') cover the accented ones ('versión'). +_VERSION_MARKER_TOKENS = frozenset({ + # English + "remix", "mix", "rmx", "live", "acoustic", "unplugged", "instrumental", + "karaoke", "demo", "demos", "edit", "version", "versions", "remaster", + "remastered", "slowed", "reverb", "sped", "spedup", "speedup", "extended", + "club", "mashup", "bootleg", "cover", "covers", "reprise", "session", + "sessions", "mono", "stereo", "duet", "rework", "dub", "vip", "single", + "radio", "alt", "alternate", "alternative", "take", "edition", "orchestral", + "symphonic", "piano", "acapella", "cappella", "nightcore", + # Distinct-track qualifiers — '(Interlude)' etc. are SEPARATE short tracks + # that share the base name with the full song; never treat as subtitles. + "interlude", "intro", "outro", "skit", "freestyle", "medley", "snippet", + # Part/volume markers whose number can be non-numeric ('Pt. II') — the + # digit guard below only catches actual digits. + "pt", "part", "vol", "ii", "iii", "iv", "vi", "vii", "viii", + # Spanish (unidecode-normalized; 'versión' → 'version' is covered above) + "directo", "vivo", "dueto", +}) + + +def strip_subtitle_qualifiers(title: str, other_title: str) -> str: + """Remove bracketed qualifiers that are SUBTITLES, not version markers. + + #825 (carlosjfcasero): the wishlist held 'Llamando a la tierra (Serenade + From the Stars)' — the song's official subtitle — while the library track + was the bare 'Llamando a la tierra'. The qualifier appears in no album or + counterpart title, so :func:`strip_redundant_context_qualifiers` keeps it, + and the length-ratio penalty then crushes an obviously-same song to ~0.14. + The sync matcher reported it missing on every run (re-adding it to the + wishlist) and the cleanup — same matcher — could never remove it. + + A qualifier is stripped only when ALL of: + * its text does not appear in ``other_title`` (if it does, the direct + comparison already handles it); + * it contains no version-marker token ('(Live)', '(Versión 1988)', + '(Dueto 2007)' keep blocking — they are different recordings); + * it introduces no digit token absent from ``other_title`` ('(Pt. 2)', + '(2007)' are different releases, never subtitles). + + Inputs should be normalized the same way the caller compares them + (lowercased / unidecode'd), like strip_redundant_context_qualifiers. + """ + if not title: + return title + + other = (other_title or "").casefold() + other_tokens = set(_TOKEN_RE.findall(other)) + + def _drop(match: re.Match) -> str: + inner = match.group(1).strip().casefold() + if not inner: + return " " + # Restated in the counterpart title — leave for the direct comparison. + if re.search(r"\b" + re.escape(inner) + r"\b", other): + return match.group(0) + tokens = _TOKEN_RE.findall(inner) + if any(t in _VERSION_MARKER_TOKENS for t in tokens): + return match.group(0) + if any(any(c.isdigit() for c in t) and t not in other_tokens for t in tokens): + return match.group(0) + return " " + + out = _QUALIFIER_RE.sub(_drop, title) + return re.sub(r"\s+", " ", out).strip() + + def numeric_tokens_differ(title_a: str, title_b: str) -> bool: """True when the digit-bearing tokens of two titles differ — 'Vol.4' vs 'Vol.4.5', 'Album' vs 'Album 2'. A numeric difference is a different @@ -133,5 +204,6 @@ def numeric_tokens_differ(title_a: str, title_b: str) -> bool: __all__ = [ "titles_plausibly_same", "strip_redundant_context_qualifiers", + "strip_subtitle_qualifiers", "numeric_tokens_differ", ] diff --git a/database/music_database.py b/database/music_database.py index 202b2d4c..0fc68c4c 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -7668,6 +7668,24 @@ class MusicDatabase: ctx_sim *= ctx_ratio # 'Believe' vs 'Believe In Me' still penalised best_title_similarity = max(best_title_similarity, ctx_sim) + # #825: a bracketed qualifier that is a SUBTITLE — not a version + # marker and not numeric — is the same song. 'Llamando a la tierra + # (Serenade From the Stars)' vs the library's bare 'Llamando a la + # tierra': the subtitle restates nothing (so #808 keeps it) and the + # length penalty crushed the pair to ~0.14 — sync re-added it to + # the wishlist forever and cleanup (same matcher) never removed it. + # Version qualifiers ('(Live)', '(Versión 1988)', '(Dueto 2007)') + # are kept by the helper, so their mismatch penalty still stands. + from core.text.title_match import strip_subtitle_qualifiers + sub_search = strip_subtitle_qualifiers(search_title_norm, db_title_norm) + sub_db = strip_subtitle_qualifiers(db_title_norm, search_title_norm) + if (sub_search, sub_db) != (search_title_norm, db_title_norm) and sub_search and sub_db: + sub_sim = self._string_similarity(sub_search, sub_db) + sub_ratio = min(len(sub_search), len(sub_db)) / max(len(sub_search), len(sub_db)) + if sub_ratio < 0.7: + sub_sim *= sub_ratio # stripped forms still length-guarded + best_title_similarity = max(best_title_similarity, sub_sim) + # Word-level guard: SequenceMatcher's char ratio over-credits # different songs that share a long substring or only a stopword # ("Dani California" vs "Californication" = 0.67; "Under The Bridge" diff --git a/services/sync_service.py b/services/sync_service.py index 803f2e8e..b48aa051 100644 --- a/services/sync_service.py +++ b/services/sync_service.py @@ -665,6 +665,10 @@ class PlaylistSyncService: _try_title, _try_artist, confidence_threshold=0.7, server_source=active_server, candidate_tracks=artist_candidates, + # #825: album context enables the album-aware fallback + # (multi-artist albums filed under another artist) — + # the cleanup path already passes it; sync didn't. + album=getattr(spotify_track, 'album', None) or None, ) if _cand_conf > confidence: db_track, confidence = _cand_track, _cand_conf diff --git a/tests/test_subtitle_qualifier_match.py b/tests/test_subtitle_qualifier_match.py new file mode 100644 index 00000000..a96150dc --- /dev/null +++ b/tests/test_subtitle_qualifier_match.py @@ -0,0 +1,158 @@ +"""#825: bracketed SUBTITLES must not block library-presence matching. + +carlosjfcasero's case (round 2): the mirrored-playlist sync auto-added +'Llamando a la tierra (Serenade From the Stars)' by M-Clan to the wishlist on +every run, even though his library has the song (stored bare). The subtitle is +the song's official parenthetical — it restates no album context, so the #808 +strip kept it, and the length-ratio penalty crushed the pair to ~0.14. The +sync matcher reported it missing forever AND the wishlist cleanup (the same +matcher) could never remove it. + +Fix: a bracketed qualifier with no version-marker token and no new numeric +token is a subtitle — compare with it stripped. Version qualifiers ('(Live)', +'(Versión 1988)', '(Dueto 2007)') still block, both EN and ES. +""" + +from __future__ import annotations + +import pytest + +from core.text.title_match import strip_subtitle_qualifiers +from database.music_database import MusicDatabase + + +# ── the pure helper ────────────────────────────────────────────────────────── + +def test_subtitle_is_stripped(): + out = strip_subtitle_qualifiers( + 'llamando a la tierra (serenade from the stars)', 'llamando a la tierra') + assert out == 'llamando a la tierra' + + +def test_version_markers_kept_english(): + for q in ('live', 'remix', 'acoustic', 'instrumental', 'demo', 'radio edit'): + assert strip_subtitle_qualifiers(f'song ({q})', 'song') == f'song ({q})' + + +def test_version_markers_kept_spanish(): + assert strip_subtitle_qualifiers('song (version 1988)', 'song') == 'song (version 1988)' + assert strip_subtitle_qualifiers('song (dueto 2007)', 'song') == 'song (dueto 2007)' + assert strip_subtitle_qualifiers('song (en directo en el liceu / 2008)', 'song') \ + == 'song (en directo en el liceu / 2008)' + + +def test_new_numeric_token_kept(): + # '(Pt. 2)' / '(2007)' are different releases, never subtitles. + assert strip_subtitle_qualifiers('song (pt. 2)', 'song') == 'song (pt. 2)' + assert strip_subtitle_qualifiers('song (2007)', 'song') == 'song (2007)' + + +def test_distinct_track_qualifiers_kept(): + # '(Interlude)' etc. are SEPARATE short tracks sharing the base name — + # treating them as subtitles would wrongly count the full song as owned. + for q in ('interlude', 'intro', 'outro', 'skit', 'freestyle'): + assert strip_subtitle_qualifiers(f'song ({q})', 'song') == f'song ({q})' + + +def test_roman_numeral_parts_kept(): + # No digits, so the numeric guard alone can't catch these. + assert strip_subtitle_qualifiers('song (pt. ii)', 'song') == 'song (pt. ii)' + assert strip_subtitle_qualifiers('song (part two)', 'song') == 'song (part two)' + assert strip_subtitle_qualifiers('song (vol. iii)', 'song') == 'song (vol. iii)' + + +def test_numeric_token_shared_with_other_title_is_fine(): + # The digit appears on the other side too — not a new release marker. + assert strip_subtitle_qualifiers('song 2007 (the ballad)', 'song 2007') == 'song 2007' + + +def test_qualifier_restated_in_other_title_left_for_direct_compare(): + full = 'song (the ballad)' + assert strip_subtitle_qualifiers(full, 'song (the ballad)') == full + + +def test_empty_and_plain_untouched(): + assert strip_subtitle_qualifiers('', 'x') == '' + assert strip_subtitle_qualifiers('plain title', 'other') == 'plain title' + + +# ── end to end through check_track_exists (sync + cleanup contract) ────────── + +@pytest.fixture() +def lib_db(tmp_path): + db = MusicDatabase(str(tmp_path / 'm.db')) + conn = db._get_connection() + c = conn.cursor() + c.execute("INSERT INTO artists (id, name, server_source) VALUES ('a1', 'M-Clan', 'jellyfin')") + c.execute("""INSERT INTO albums (id, title, artist_id, server_source) + VALUES ('al1', 'Usar y tirar', 'a1', 'jellyfin')""") + c.execute("""INSERT INTO tracks (id, album_id, artist_id, title, file_path, server_source) + VALUES ('t1', 'al1', 'a1', 'Llamando a la tierra', '/m/llamando.mp3', 'jellyfin')""") + c.execute("""INSERT INTO tracks (id, album_id, artist_id, title, file_path, server_source) + VALUES ('t2', 'al1', 'a1', 'Carolina', '/m/carolina.mp3', 'jellyfin')""") + conn.commit() + conn.close() + return db + + +def test_825_subtitled_search_matches_bare_library_track(lib_db): + """The reported case verbatim: playlist title carries the subtitle, the + library stores the bare title — must match (sync) and clean (cleanup).""" + match, conf = lib_db.check_track_exists( + 'Llamando a la tierra (Serenade From the Stars)', 'M-Clan', + confidence_threshold=0.7, server_source='jellyfin', + ) + assert match is not None and conf >= 0.7 + assert match.title == 'Llamando a la tierra' + + +def test_825_reverse_direction_matches(lib_db): + """Library could equally store the FULL title while the playlist has the + bare one — both directions must match.""" + conn = lib_db._get_connection() + c = conn.cursor() + c.execute("""INSERT INTO tracks (id, album_id, artist_id, title, file_path, server_source) + VALUES ('t3', 'al1', 'a1', 'Maggie (despierta)', '/m/maggie.mp3', 'jellyfin')""") + conn.commit() + conn.close() + match, conf = lib_db.check_track_exists( + 'Maggie', 'M-Clan', confidence_threshold=0.7, server_source='jellyfin') + assert match is not None and conf >= 0.7 + + +def test_live_version_still_blocked(lib_db): + match, conf = lib_db.check_track_exists( + 'Llamando a la tierra (Live)', 'M-Clan', + confidence_threshold=0.7, server_source='jellyfin', + ) + assert conf < 0.7 + + +def test_spanish_version_qualifiers_still_blocked(lib_db): + for title in ('Carolina (Versión 1988)', 'Carolina (Dueto 2007)', + 'Carolina (En Directo / 2005)'): + match, conf = lib_db.check_track_exists( + title, 'M-Clan', confidence_threshold=0.7, server_source='jellyfin') + assert conf < 0.7, title + + +def test_different_song_prefix_still_blocked(lib_db): + """Non-bracketed extensions are untouched — the length penalty stands.""" + match, conf = lib_db.check_track_exists( + 'Carolina en mi mente y otras cosas', 'M-Clan', + confidence_threshold=0.7, server_source='jellyfin', + ) + assert conf < 0.7 + + +def test_batched_candidate_path_also_fixed(lib_db): + """The sync matcher uses the candidate_tracks (batched) path — the fix must + apply there too, not just the SQL-variation path.""" + candidates = lib_db.search_tracks(artist='M-Clan', limit=50, server_source='jellyfin') + assert candidates + match, conf = lib_db.check_track_exists( + 'Llamando a la tierra (Serenade From the Stars)', 'M-Clan', + confidence_threshold=0.7, server_source='jellyfin', + candidate_tracks=candidates, + ) + assert match is not None and conf >= 0.7 From e32e2e5e14e327af3b52b32a26f624bc2733fc2a Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 9 Jun 2026 16:31:05 -0700 Subject: [PATCH 35/45] =?UTF-8?q?Sync:=20append=20mode=20actually=20dedupe?= =?UTF-8?q?s=20=E2=80=94=20stop=20re-adding=20the=20whole=20playlist=20(#8?= =?UTF-8?q?23)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit carlosjfcasero round 2: after 6fa956d6 append stopped recreating the playlist, but every sync re-appended ALL matched tracks — every track N times. His log shows it plainly: "added 22 new tracks to 'Disney' (skipped 0 already present)" on a playlist that already had those 22. Root cause: the dedupe read `{t.id for t in get_playlist_tracks(...)}` — but JellyfinTrack only defines `ratingKey`, never `id`, so the existing-ids set was ALWAYS empty and everything looked new. NavidromeTrack is also ratingKey-only, so the Navidrome append had the identical bug. Plex (plexapi ratingKey) was fine. The existing tests were green because they mocked existing tracks as SimpleNamespace(id=...) — encoding the same wrong assumption as the code. Fix: - New pure planner plan_playlist_append(current, desired) in core/sync/playlist_edit.py (next to the reconcile planner): order-preserving, drops already-present ids, dedupes within desired, stringifies (Emby numeric vs string safe). - Jellyfin/Emby: existing ids fetched from the canonical /Playlists/{id}/Items endpoint (same as reconcile — works for Jellyfin GUIDs and Emby numeric ids), ratingKey fallback if that request fails. - Navidrome: dedupe on ratingKey (the attribute that actually exists). Tests: planner (skip-present incl. the reporter's unchanged-playlist case, desired-order, dupes-within-desired, int/str ids, empties) + the append-mode suite rewritten to pin the REAL shapes (raw Items dicts for Jellyfin, ratingKey objects for Navidrome) + a new fallback-path test. 524 playlist/sync/jellyfin/navidrome tests pass. --- core/jellyfin_client.py | 34 +++++++++++++---- core/navidrome_client.py | 19 +++++++--- core/sync/playlist_edit.py | 29 ++++++++++++++ tests/test_playlist_edit.py | 35 +++++++++++++++++ tests/test_server_playlist_append_mode.py | 46 ++++++++++++++++++----- 5 files changed, 140 insertions(+), 23 deletions(-) diff --git a/core/jellyfin_client.py b/core/jellyfin_client.py index 8ee4eb74..f24020d5 100644 --- a/core/jellyfin_client.py +++ b/core/jellyfin_client.py @@ -1566,20 +1566,40 @@ class JellyfinClient(MediaServerClient): return self.create_playlist(playlist_name, tracks) playlist_id = existing_playlist.id - existing_tracks = self.get_playlist_tracks(playlist_id) - existing_ids = { - str(t.id) for t in existing_tracks if hasattr(t, 'id') and t.id - } + # #823 round 2: the old dedupe read `t.id` off get_playlist_tracks() + # results — but JellyfinTrack only defines `ratingKey`, so the + # existing-ids set was ALWAYS empty and every sync re-appended the + # whole matched list ("added 22 ... skipped 0" on a playlist that + # already had them, every track N times). Fetch the playlist's items + # from the canonical /Playlists/{id}/Items endpoint (the same one + # reconcile uses — works on Jellyfin GUIDs and Emby numeric ids) and + # dedupe on the raw item Id; fall back to ratingKey if that fails. + existing_ids = set() + items_resp = self._make_request( + f'/Playlists/{playlist_id}/Items', {'UserId': self.user_id}) + if items_resp: + for item in items_resp.get('Items', []): + iid = str(item.get('Id') or '') + if iid: + existing_ids.add(iid) + else: + existing_ids = { + str(getattr(t, 'ratingKey', '') or '') + for t in self.get_playlist_tracks(playlist_id) + } - {''} - new_track_ids = [] + desired_ids = [] for t in tracks: tid = None if hasattr(t, 'id'): tid = str(t.id) if t.id else None elif isinstance(t, dict): tid = str(t.get('Id') or t.get('id') or '') - if tid and tid not in existing_ids and self._is_valid_guid(tid): - new_track_ids.append(tid) + if tid and self._is_valid_guid(tid): + desired_ids.append(tid) + + from core.sync.playlist_edit import plan_playlist_append + new_track_ids = plan_playlist_append(existing_ids, desired_ids) if not new_track_ids: logger.info( diff --git a/core/navidrome_client.py b/core/navidrome_client.py index 085a4257..5a0677ee 100644 --- a/core/navidrome_client.py +++ b/core/navidrome_client.py @@ -1102,12 +1102,16 @@ class NavidromeClient(MediaServerClient): return self.create_playlist(playlist_name, tracks) primary = existing_playlists[0] - existing_tracks = self.get_playlist_tracks(primary.id) + # #823 round 2: the old dedupe read `t.id` — but NavidromeTrack only + # defines `ratingKey`, so the existing-ids set was ALWAYS empty and + # every sync re-appended the whole matched list (every track N + # times). Same bug as the Jellyfin append; dedupe on ratingKey. existing_ids = { - str(t.id) for t in existing_tracks if hasattr(t, 'id') and t.id - } + str(getattr(t, 'ratingKey', '') or '') + for t in self.get_playlist_tracks(primary.id) + } - {''} - new_track_ids = [] + desired_ids = [] for t in tracks: tid = None if hasattr(t, 'ratingKey'): @@ -1116,8 +1120,11 @@ class NavidromeClient(MediaServerClient): tid = str(t.id) if t.id else None elif isinstance(t, dict): tid = str(t.get('id') or '') - if tid and tid not in existing_ids: - new_track_ids.append(tid) + if tid: + desired_ids.append(tid) + + from core.sync.playlist_edit import plan_playlist_append + new_track_ids = plan_playlist_append(existing_ids, desired_ids) if not new_track_ids: logger.info( diff --git a/core/sync/playlist_edit.py b/core/sync/playlist_edit.py index 80325238..a3423302 100644 --- a/core/sync/playlist_edit.py +++ b/core/sync/playlist_edit.py @@ -109,6 +109,34 @@ def plan_playlist_reconcile( return {"add": add, "remove": remove} +def plan_playlist_append( + current_ids: List[str], + desired_ids: List[str], +) -> List[str]: + """Plan an append: which desired ids are NOT already in the playlist. + + Used by ``sync_mode='append'`` (#823 round 2): the Jellyfin/Emby and + Navidrome appends deduped with ``{t.id for t in existing}`` — but their + track wrappers only define ``ratingKey``, never ``id``, so the existing-ids + set was ALWAYS empty and every sync re-appended the full matched list + (every track N times, "skipped 0 already present"). Pure planner so the + dedupe logic is testable; the caller fetches current ids however its + server API works and applies the returned adds. + + Order-preserving and duplicate-safe: desired order is kept, ids already + present are dropped, and duplicates WITHIN desired are emitted once. + """ + current_set = {str(t) for t in current_ids} + out: List[str] = [] + seen = set() + for d in desired_ids: + tid = str(d) + if tid and tid not in current_set and tid not in seen: + seen.add(tid) + out.append(tid) + return out + + VALID_SYNC_MODES = ("replace", "append", "reconcile") @@ -129,6 +157,7 @@ __all__ = [ "plan_playlist_add", "remove_one_occurrence", "plan_playlist_reconcile", + "plan_playlist_append", "normalize_sync_mode", "VALID_SYNC_MODES", ] diff --git a/tests/test_playlist_edit.py b/tests/test_playlist_edit.py index 66caae40..7c402992 100644 --- a/tests/test_playlist_edit.py +++ b/tests/test_playlist_edit.py @@ -161,3 +161,38 @@ def test_normalize_falls_back_for_unknown(): def test_normalize_all_real_modes_pass_through(): for m in ('replace', 'append', 'reconcile'): assert normalize_sync_mode(m, 'replace') == m + + +# ── plan_playlist_append (#823 round 2) ────────────────────────────────────── +# The Jellyfin/Emby + Navidrome appends deduped on `t.id`, but their track +# wrappers only define `ratingKey` — the existing-ids set was always empty, so +# every sync re-appended the full matched list (every track N times, +# "skipped 0 already present"). The planner is the now-testable dedupe. + +from core.sync.playlist_edit import plan_playlist_append # noqa: E402 + + +def test_append_skips_already_present(): + # carlosjfcasero's case: second sync of an unchanged playlist adds NOTHING. + assert plan_playlist_append(['a', 'b', 'c'], ['a', 'b', 'c']) == [] + + +def test_append_adds_only_new_in_desired_order(): + assert plan_playlist_append(['a', 'c'], ['a', 'b', 'c', 'd']) == ['b', 'd'] + + +def test_append_to_empty_playlist_adds_all(): + assert plan_playlist_append([], ['a', 'b']) == ['a', 'b'] + + +def test_append_dedupes_within_desired(): + assert plan_playlist_append(['x'], ['a', 'a', 'x', 'b', 'a']) == ['a', 'b'] + + +def test_append_stringifies_ids(): + # Emby numeric ids may arrive as ints on one side and strings on the other. + assert plan_playlist_append([16607838], ['16607838', '999']) == ['999'] + + +def test_append_ignores_empty_ids(): + assert plan_playlist_append(['a'], ['', 'b']) == ['b'] diff --git a/tests/test_server_playlist_append_mode.py b/tests/test_server_playlist_append_mode.py index 8624fb1a..97af0c83 100644 --- a/tests/test_server_playlist_append_mode.py +++ b/tests/test_server_playlist_append_mode.py @@ -173,13 +173,19 @@ class TestJellyfinAppendToPlaylist: mock_create.assert_called_once_with("Test", new_tracks) def test_filters_out_already_present_tracks(self): - """Reporter's exact case for Jellyfin — only new GUIDs go in.""" + """Reporter's exact case for Jellyfin — only new GUIDs go in. + + #823 round 2: existing ids now come from the canonical + /Playlists/{id}/Items request ({'Items':[{'Id': ...}]}) — the old + get_playlist_tracks path returned JellyfinTrack objects that only + define `ratingKey`, so the previous `t.id` dedupe was always empty and + every sync re-appended everything. These mocks pin the REAL shape.""" client = _make_jellyfin_client() existing_playlist = SimpleNamespace(id='pl-1') - existing_tracks = [ - SimpleNamespace(id='aaaaaaaa-bbbb-cccc-dddd-000000000001'), - SimpleNamespace(id='aaaaaaaa-bbbb-cccc-dddd-000000000002'), - ] + items_resp = {'Items': [ + {'Id': 'aaaaaaaa-bbbb-cccc-dddd-000000000001'}, + {'Id': 'aaaaaaaa-bbbb-cccc-dddd-000000000002'}, + ]} incoming = [ SimpleNamespace(id='aaaaaaaa-bbbb-cccc-dddd-000000000001'), # present SimpleNamespace(id='aaaaaaaa-bbbb-cccc-dddd-000000000003'), # NEW @@ -194,7 +200,7 @@ class TestJellyfinAppendToPlaylist: with patch.object(client, 'ensure_connection', return_value=True), \ patch.object(client, 'get_playlist_by_name', return_value=existing_playlist), \ - patch.object(client, 'get_playlist_tracks', return_value=existing_tracks), \ + patch.object(client, '_make_request', return_value=items_resp), \ patch.object(client, '_is_valid_guid', return_value=True), \ patch('core.jellyfin_client.requests.post', side_effect=fake_post): result = client.append_to_playlist("Test", incoming) @@ -206,9 +212,26 @@ class TestJellyfinAppendToPlaylist: def test_short_circuits_when_no_new_tracks(self): client = _make_jellyfin_client() existing_playlist = SimpleNamespace(id='pl-1') - existing_tracks = [SimpleNamespace(id='guid-1')] + items_resp = {'Items': [{'Id': 'guid-1'}]} with patch.object(client, 'ensure_connection', return_value=True), \ patch.object(client, 'get_playlist_by_name', return_value=existing_playlist), \ + patch.object(client, '_make_request', return_value=items_resp), \ + patch.object(client, '_is_valid_guid', return_value=True), \ + patch('core.jellyfin_client.requests.post') as mock_post: + result = client.append_to_playlist("Test", [SimpleNamespace(id='guid-1')]) + assert result is True + mock_post.assert_not_called() + + def test_falls_back_to_ratingkey_tracks_when_items_request_fails(self): + """When /Playlists/{id}/Items fails, dedupe falls back to + get_playlist_tracks — reading ratingKey (the attr that EXISTS on + JellyfinTrack), not the never-present `.id` of the original bug.""" + client = _make_jellyfin_client() + existing_playlist = SimpleNamespace(id='pl-1') + existing_tracks = [SimpleNamespace(ratingKey='guid-1')] + with patch.object(client, 'ensure_connection', return_value=True), \ + patch.object(client, 'get_playlist_by_name', return_value=existing_playlist), \ + patch.object(client, '_make_request', return_value=None), \ patch.object(client, 'get_playlist_tracks', return_value=existing_tracks), \ patch.object(client, '_is_valid_guid', return_value=True), \ patch('core.jellyfin_client.requests.post') as mock_post: @@ -221,7 +244,7 @@ class TestJellyfinAppendToPlaylist: existing_playlist = SimpleNamespace(id='pl-1') with patch.object(client, 'ensure_connection', return_value=True), \ patch.object(client, 'get_playlist_by_name', return_value=existing_playlist), \ - patch.object(client, 'get_playlist_tracks', return_value=[]), \ + patch.object(client, '_make_request', return_value={'Items': []}), \ patch.object(client, '_is_valid_guid', return_value=True), \ patch('core.jellyfin_client.requests.post', return_value=SimpleNamespace(status_code=500, text='server error')): @@ -256,9 +279,12 @@ class TestNavidromeAppendToPlaylist: mock_create.assert_called_once() def test_filters_out_already_present_tracks_and_calls_subsonic(self): + # #823 round 2: existing tracks are NavidromeTrack objects, which only + # define `ratingKey` — the old `t.id` dedupe was always empty. These + # mocks pin the REAL attribute so the dedupe is actually exercised. client = _make_navidrome_client() existing_playlists = [SimpleNamespace(id='pl-1', title='Test')] - existing_tracks = [SimpleNamespace(id='100'), SimpleNamespace(id='101')] + existing_tracks = [SimpleNamespace(ratingKey='100'), SimpleNamespace(ratingKey='101')] incoming = [ SimpleNamespace(id='100'), # present SimpleNamespace(id='102'), # NEW @@ -287,7 +313,7 @@ class TestNavidromeAppendToPlaylist: def test_short_circuits_when_no_new_tracks(self): client = _make_navidrome_client() existing_playlists = [SimpleNamespace(id='pl-1', title='Test')] - existing_tracks = [SimpleNamespace(id='100')] + existing_tracks = [SimpleNamespace(ratingKey='100')] with patch.object(client, 'ensure_connection', return_value=True), \ patch.object(client, 'get_playlists_by_name', return_value=existing_playlists), \ patch.object(client, 'get_playlist_tracks', return_value=existing_tracks), \ From bcd69c8baa8be29a0eb0558007d09db54aedc460 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 9 Jun 2026 17:10:03 -0700 Subject: [PATCH 36/45] =?UTF-8?q?Multi-artist=20tags:=20Search=20=E2=86=92?= =?UTF-8?q?=20Download=20Now=20finally=20knows=20its=20metadata=20source?= =?UTF-8?q?=20(Netti93)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third round of the multi-artist report. The earlier fixes (Deezer contributors upgrade, _artists_list, feat_in_title/artist_separator) were all in place and correct — but gated on source == 'deezer', and on the real Search → Download Now path NOTHING carried the source: core/search/sources.py serialized tracks with no source field, search.js's enrichedTrack didn't add one, so get_import_source() resolved '' and the whole Deezer-specific block silently skipped. Files were tagged with only the primary artist until a Retag (which rebuilds context with the source set — exactly why retagging always fixed it). The earlier tests passed because they set context['source'] directly — the one field the real flow never had (same mock-drift as the #823 append tests). Reproduced with Netti93's exact track (deezer 3966840171) through the real extract_source_metadata: before — source '', artists ['August Burns Red']; after — source 'deezer', contributors fetched, artists ['August Burns Red', 'Polaris'], title 'Sonic Salvation (feat. Polaris)' per feat_in_title. Fix, three layers: - core/search/sources.py: serialized tracks/albums/artists carry "source" (the canonical name the orchestrator already passes; '' when unnamed). - core/imports/context.py get_import_source: also reads '_source' from the nested dicts (track_info/original_search/album/artist) — additionally fixes the discography/wishlist flows, which always passed '_source' that nothing read. - search.js: enrichedTrack + the album-download path carry source through to the download task. Tests: real-payload staging-shaped contexts (source in track_info, '_source' shape, and the pre-fix sourceless shape staying safe — mocked Deezer client), serializer source-field tests, resolver fallback tests; exact-shape serializer tests updated for the new key. 1977 import/metadata/search tests pass (the only 2 failures are the known soundcloud ones). --- core/imports/context.py | 16 ++-- core/search/sources.py | 17 ++++ tests/imports/test_import_context.py | 17 ++++ .../test_multi_artist_tag_settings.py | 90 +++++++++++++++++++ tests/search/test_search_sources.py | 45 ++++++++++ webui/static/search.js | 14 ++- 6 files changed, 193 insertions(+), 6 deletions(-) diff --git a/core/imports/context.py b/core/imports/context.py index 73459949..f718f014 100644 --- a/core/imports/context.py +++ b/core/imports/context.py @@ -129,30 +129,36 @@ def get_import_search_result(context: Optional[Dict[str, Any]]) -> Dict[str, Any def get_import_source(context: Optional[Dict[str, Any]]) -> str: + # Several track payloads carry the metadata source under "_source" rather + # than "source" (the discography/wishlist dicts, frontend search results). + # Only the context-level "_source" was honored (normalize_import_context); + # the nested dicts were checked for "source" alone, so a Deezer-sourced + # Download Now resolved to '' and source-specific metadata logic (the + # Deezer contributors upgrade for multi-artist tags) never ran (Netti93). if not isinstance(context, dict): return "" - source = context.get("source") + source = context.get("source") or context.get("_source") if source: return str(source) track_info = get_import_track_info(context) - source = _first_value(track_info, "source", default="") + source = _first_value(track_info, "source", "_source", default="") if source: return str(source) original_search = get_import_original_search(context) - source = _first_value(original_search, "source", default="") + source = _first_value(original_search, "source", "_source", default="") if source: return str(source) album = get_import_context_album(context) - source = _first_value(album, "source", default="") + source = _first_value(album, "source", "_source", default="") if source: return str(source) artist = get_import_context_artist(context) - source = _first_value(artist, "source", default="") + source = _first_value(artist, "source", "_source", default="") return str(source) if source else "" diff --git a/core/search/sources.py b/core/search/sources.py index 006bf8f0..e5d66805 100644 --- a/core/search/sources.py +++ b/core/search/sources.py @@ -35,6 +35,7 @@ def search_kind(client, query: str, kind: str, source_name: Optional[str] = None artists.append({ "id": artist.id, "name": artist.name, + "source": source_name or "", "image_url": artist.image_url, "external_urls": artist.external_urls or {}, }) @@ -52,6 +53,7 @@ def search_kind(client, query: str, kind: str, source_name: Optional[str] = None "id": album.id, "name": album.name, "artist": artist_name, + "source": source_name or "", "image_url": album.image_url, "release_date": album.release_date, "total_tracks": album.total_tracks, @@ -78,6 +80,21 @@ def search_kind(client, query: str, kind: str, source_name: Optional[str] = None "id": track.id, "name": track.name, "artist": artist_name, + # The REAL artist list, not the joined display string above. + # Spotify/Tidal/iTunes searches return collabs as a list; + # collapsing them to one "A, B" string made the import + # pipeline tag downloads with a single combined artist + # (resolve_track_artists saw one value). The frontend keeps + # using "artist" for display. + "artists": list(track.artists or []), + # Which metadata source this result came from. Travels with + # the payload through Download Now -> download task -> + # import context, where extract_source_metadata needs it to + # run source-specific logic (the Deezer contributors + # upgrade for multi-artist tags — Netti93's report: without + # it get_import_source() resolved '' and collab tracks + # were tagged with only the primary artist until a Retag). + "source": source_name or "", "album": track.album, "duration_ms": track.duration_ms, "image_url": track.image_url, diff --git a/tests/imports/test_import_context.py b/tests/imports/test_import_context.py index e71584b7..5ce80835 100644 --- a/tests/imports/test_import_context.py +++ b/tests/imports/test_import_context.py @@ -338,3 +338,20 @@ def test_detect_album_info_web_forces_album_when_track_and_artist_differ(): assert album_info["album_name"] == "Album One" assert album_info["track_number"] == 4 assert album_info["disc_number"] == 2 + + +def test_get_import_source_reads_underscore_source_from_nested_dicts(): + """Netti93 multi-artist fix: many track payloads carry '_source' (the + discography/wishlist dicts) or 'source' only inside track_info (search + results). get_import_source must resolve all of them — previously only + the context-level keys worked, so direct downloads resolved '' and + source-specific metadata logic never ran.""" + from core.imports.context import get_import_source + + assert get_import_source({"track_info": {"source": "deezer"}}) == "deezer" + assert get_import_source({"track_info": {"_source": "deezer"}}) == "deezer" + assert get_import_source({"original_search_result": {"_source": "itunes"}}) == "itunes" + assert get_import_source({"_source": "tidal"}) == "tidal" + # context-level 'source' still wins over nested + assert get_import_source({"source": "spotify", "track_info": {"_source": "deezer"}}) == "spotify" + assert get_import_source({}) == "" diff --git a/tests/metadata/test_multi_artist_tag_settings.py b/tests/metadata/test_multi_artist_tag_settings.py index b8de390f..dc835bed 100644 --- a/tests/metadata/test_multi_artist_tag_settings.py +++ b/tests/metadata/test_multi_artist_tag_settings.py @@ -561,3 +561,93 @@ class TestDeezerDirectDownloadFlow: assert meta["_artists_list"] == ["FAYAN", "Dalton"] assert meta["artist"] == "FAYAN;Dalton" assert meta["title"] == "VERLIEBT IN MICH" + + +class TestSourceResolutionFromRealDownloadPayload: + """Netti93 round 3: the prior tests set context['source'] — a field the + REAL Search → Download Now flow never had. The serialized search results + carried no source at all, so get_import_source() resolved '' and the + Deezer contributors upgrade never ran on direct downloads (single-artist + tags until a Retag). These pin the actual payload shapes end to end.""" + + def _staging_context(self, track_info): + # Shape built by core/downloads/staging.py:457 for a stream download. + return { + "track_info": track_info, + "spotify_artist": {"name": "August Burns Red", "id": None}, + "spotify_album": {"name": "Death Below"}, + "original_search_result": { + "username": "tidal", "filename": "/x.flac", + "title": "Sonic Salvation", "artist": "August Burns Red", + "spotify_clean_title": "Sonic Salvation", + "spotify_clean_album": "Death Below", + "spotify_clean_artist": "August Burns Red", + "track_number": 1, "disc_number": 1, + }, + "is_album_download": False, + "staging_source": True, + } + + def _fake_deezer(self): + return SimpleNamespace(get_track_details=MagicMock(return_value={ + "id": "3966840171", "name": "Sonic Salvation", + "artists": ["August Burns Red", "Polaris"], + })) + + def test_source_in_track_info_triggers_upgrade(self): + """The fixed flow: serialized search result carries source='deezer' + inside track_info (no context-level source) → upgrade fires.""" + from core.metadata import source as src_module + + track_info = { + "id": "3966840171", "name": "Sonic Salvation", + "source": "deezer", + "artists": ["August Burns Red"], + "album": {"name": "Death Below", "id": None}, + } + fake_deezer = self._fake_deezer() + with patch.object(src_module, "get_config_manager", + return_value=_make_cfg({"metadata_enhancement.tags.write_multi_artist": True})), \ + patch("core.metadata.get_deezer_client", return_value=fake_deezer): + meta = src_module.extract_source_metadata( + self._staging_context(track_info), {"name": "August Burns Red", "id": ""}, {}) + + assert meta["_artists_list"] == ["August Burns Red", "Polaris"] + fake_deezer.get_track_details.assert_called_once_with("3966840171") + + def test_underscore_source_shape_also_triggers_upgrade(self): + """Discography/wishlist payloads carry '_source' instead of 'source' — + get_import_source must honor that shape too.""" + from core.metadata import source as src_module + + track_info = { + "id": "3966840171", "name": "Sonic Salvation", + "_source": "deezer", + "artists": ["August Burns Red"], + } + fake_deezer = self._fake_deezer() + with patch.object(src_module, "get_config_manager", return_value=_make_cfg()), \ + patch("core.metadata.get_deezer_client", return_value=fake_deezer): + meta = src_module.extract_source_metadata( + self._staging_context(track_info), {"name": "August Burns Red", "id": ""}, {}) + + assert meta["_artists_list"] == ["August Burns Red", "Polaris"] + fake_deezer.get_track_details.assert_called_once_with("3966840171") + + def test_sourceless_payload_still_no_upgrade(self): + """A payload with no source anywhere (pre-fix shape) must not crash — + and must not call the Deezer API blindly.""" + from core.metadata import source as src_module + + track_info = { + "id": "3966840171", "name": "Sonic Salvation", + "artists": ["August Burns Red"], + } + fake_deezer = self._fake_deezer() + with patch.object(src_module, "get_config_manager", return_value=_make_cfg()), \ + patch("core.metadata.get_deezer_client", return_value=fake_deezer): + meta = src_module.extract_source_metadata( + self._staging_context(track_info), {"name": "August Burns Red", "id": ""}, {}) + + assert meta["_artists_list"] == ["August Burns Red"] + fake_deezer.get_track_details.assert_not_called() diff --git a/tests/search/test_search_sources.py b/tests/search/test_search_sources.py index f6949027..eb5fb7fd 100644 --- a/tests/search/test_search_sources.py +++ b/tests/search/test_search_sources.py @@ -84,6 +84,7 @@ def test_search_kind_artists_returns_normalized_dicts(): assert result == [{ 'id': 'id1', 'name': 'Pink Floyd', + 'source': 'spotify', 'image_url': 'thumb.jpg', 'external_urls': {'spotify': 'url'}, }] @@ -137,6 +138,8 @@ def test_search_kind_tracks_returns_full_shape(): 'id': 't1', 'name': 'Money', 'artist': 'Pink Floyd', + 'artists': ['Pink Floyd'], + 'source': '', 'album': 'DSOTM', 'duration_ms': 383000, 'image_url': 'm.jpg', @@ -201,3 +204,45 @@ def test_search_source_all_fail_returns_empty_lists(): client = _Client(fail={'artists', 'albums', 'tracks'}) result = sources.search_source('q', client, 'spotify') assert result == {'artists': [], 'albums': [], 'tracks': [], 'available': True} + + +# ── source field on serialized results (Netti93 multi-artist fix) ─────────── +# Search results must carry which metadata source they came from: the payload +# travels Download Now → download task → import context, where the Deezer +# contributors upgrade (multi-artist tags) is gated on source == 'deezer'. +# Without it get_import_source() resolved '' and collab tracks were tagged +# with only the primary artist until a Retag. + +def _full_client(): + return _Client( + artists=[_Artist('id1', 'A')], + albums=[_Album('a1', 'Album', artists=['A'])], + tracks=[_Track('t1', 'T', artists=['A'], album='Album')], + ) + + +def test_serialized_tracks_carry_source(): + out = sources.search_kind(_full_client(), "q", "tracks", source_name="deezer") + assert out and all(t["source"] == "deezer" for t in out) + + +def test_serialized_albums_and_artists_carry_source(): + albums = sources.search_kind(_full_client(), "q", "albums", source_name="deezer") + artists = sources.search_kind(_full_client(), "q", "artists", source_name="deezer") + assert albums and all(a["source"] == "deezer" for a in albums) + assert artists and all(a["source"] == "deezer" for a in artists) + + +def test_serialized_source_empty_when_unnamed(): + # hydrabase path calls without a source_name — emit '' not the class name. + out = sources.search_kind(_full_client(), "q", "tracks") + assert out and all(t["source"] == "" for t in out) + + +def test_serialized_tracks_carry_real_artists_list(): + """Collabs must survive as a LIST — the joined "A, B" display string made + downloads tag a single combined artist (Marcus's report).""" + client = _Client(tracks=[_Track('t1', 'Collab', artists=['Artist A', 'Artist B'], album='X')]) + out = sources.search_kind(client, 'q', 'tracks', source_name='spotify') + assert out[0]['artists'] == ['Artist A', 'Artist B'] + assert out[0]['artist'] == 'Artist A, Artist B' # display string unchanged diff --git a/webui/static/search.js b/webui/static/search.js index 54110af1..721df256 100644 --- a/webui/static/search.js +++ b/webui/static/search.js @@ -750,6 +750,9 @@ function initializeSearchModeToggle() { // Enrich each track with full album object (needed for wishlist functionality) const enrichedTracks = albumData.tracks.map(track => ({ ...track, + // Carry the metadata source for source-specific import logic + // (Deezer contributors upgrade for multi-artist tags). + source: track.source || album.source || null, album: { name: albumData.name, id: albumData.id, @@ -902,7 +905,16 @@ function initializeSearchModeToggle() { const enrichedTrack = { id: track.id, name: track.name, - artists: [track.artist], // Convert string to array for modal compatibility + // Carry the metadata source so the import pipeline can run + // source-specific logic (Deezer contributors upgrade for + // multi-artist tags) on the downloaded file. + source: track.source || null, + // Prefer the real artist list (Spotify/Tidal collabs) over the + // joined "A, B" display string, so downloads get proper + // multi-artist tags instead of one combined artist. + artists: (Array.isArray(track.artists) && track.artists.length) + ? track.artists + : [track.artist], album: { name: track.album, id: null, From e8cde40d222f10dc7d64d9aa60d155adb313a019 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 9 Jun 2026 20:14:02 -0700 Subject: [PATCH 37/45] Watchlist: show WHICH tracks a scan found/added + group Download Origins (#831) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tacobell444 (#707 follow-up): the scan summary said "New tracks: 19 • Added to wishlist: 10" with no way to see which tracks those were — you had to scan your wishlist and guess what was new. Scan ledger: the scanner now records a per-run scan_track_events list (track, artist, album, thumb, status added|skipped — skipped = found-new but declined by add_to_wishlist: already queued or blocklisted; capped at 500). The status endpoint already serializes scan_state, so the payload flows free. The completed (and cancelled) scan summary on the Watchlist page gets a "Show tracks" toggle expanding a styled list — Added section + Skipped section with badges, reusing the live-feed row styling. Download Origins grouping: the modal now groups entries by what triggered them (watchlist artist / playlist name) with collapsible headers + counts instead of a flat list with a per-row badge. Entries arrive newest-first so groups order themselves by their newest download. Same row markup, checkboxes/delete intact. Provenance: watchlist adds now stamp scan_run_id into wishlist source_info, so per-run grouping is queryable later (future "what did run X add" views). Tests: per-run ledger seam test (added + skipped statuses, album/artist fields, FIFO unchanged). 316 watchlist/wishlist tests pass; JS syntax-checked. --- core/watchlist_scanner.py | 37 +++++++-- tests/test_watchlist_scanner_scan.py | 54 ++++++++++++++ web_server.py | 9 ++- webui/static/api-monitor.js | 46 ++++++++++++ webui/static/origin-history.js | 35 ++++++++- webui/static/style.css | 108 +++++++++++++++++++++++++++ 6 files changed, 277 insertions(+), 12 deletions(-) diff --git a/core/watchlist_scanner.py b/core/watchlist_scanner.py index 239cfba0..c99841eb 100644 --- a/core/watchlist_scanner.py +++ b/core/watchlist_scanner.py @@ -1376,14 +1376,34 @@ class WatchlistScanner: if scan_state is not None: scan_state['tracks_found_this_scan'] += 1 - if self.add_track_to_wishlist(track, album_data, artist): + added = self.add_track_to_wishlist( + track, album_data, artist, + scan_run_id=(scan_state or {}).get('scan_run_id', ''), + ) + + track_artists = track.get('artists', []) + track_artist_name = track_artists[0].get('name', 'Unknown Artist') if track_artists else 'Unknown Artist' + + # #831: per-run ledger so the completed-scan + # summary can list WHICH tracks the counts mean. + # 'skipped' = found-new but add_to_wishlist + # declined (already queued in the wishlist, or + # the artist is blocklisted). Capped for sanity. + if scan_state is not None: + events = scan_state.setdefault('scan_track_events', []) + if len(events) < 500: + events.append({ + 'track_name': track_name, + 'artist_name': track_artist_name, + 'album_name': album_name, + 'album_image_url': album_image_url, + 'status': 'added' if added else 'skipped', + }) + + if added: artist_added_tracks += 1 if scan_state is not None: scan_state['tracks_added_this_scan'] += 1 - - track_artists = track.get('artists', []) - track_artist_name = track_artists[0].get('name', 'Unknown Artist') if track_artists else 'Unknown Artist' - if scan_state is not None: scan_state['recent_wishlist_additions'].insert(0, { 'track_name': track_name, 'artist_name': track_artist_name, @@ -2224,7 +2244,8 @@ class WatchlistScanner: logger.warning(f"Error checking if track exists: {track_name}: {e}") return True # Assume missing if we can't check - def add_track_to_wishlist(self, track, album, watchlist_artist: WatchlistArtist) -> bool: + def add_track_to_wishlist(self, track, album, watchlist_artist: WatchlistArtist, + scan_run_id: str = '') -> bool: """Add a missing track to the wishlist""" try: # Handle both dict and object track/album formats @@ -2306,7 +2327,9 @@ class WatchlistScanner: 'watchlist_artist_name': watchlist_artist.artist_name, 'watchlist_artist_id': watchlist_artist.spotify_artist_id, 'album_name': album_name, - 'scan_timestamp': datetime.now().isoformat() + 'scan_timestamp': datetime.now().isoformat(), + # #831: groups wishlist rows by the scan run that added them. + 'scan_run_id': scan_run_id or '', }, profile_id=getattr(watchlist_artist, 'profile_id', 1) ) diff --git a/tests/test_watchlist_scanner_scan.py b/tests/test_watchlist_scanner_scan.py index 6c08efa0..f02eab45 100644 --- a/tests/test_watchlist_scanner_scan.py +++ b/tests/test_watchlist_scanner_scan.py @@ -1177,3 +1177,57 @@ def test_match_to_spotify_uses_strict_lookup(): assert result is None assert spotify_client.search_calls == [("Artist One", 5, False)] + + +def test_scan_records_per_run_track_ledger(monkeypatch): + """#831: the scan keeps a full per-run ledger (scan_track_events) of found + tracks — 'added' vs 'skipped' (wishlist dup / blocklisted) — so the UI can + list WHICH tracks the 'New tracks / Added to wishlist' counts refer to.""" + monkeypatch.setattr(watchlist_scanner_module, "DELAY_BETWEEN_ARTISTS", 0) + monkeypatch.setattr(watchlist_scanner_module, "DELAY_BETWEEN_ALBUMS", 0) + + artist = _build_artist() + album = types.SimpleNamespace(id="album-1", name="Album One") + album_data = { + "name": "Album One", + "images": [{"url": "https://example.com/album.jpg"}], + "tracks": { + "items": [ + {"id": "t1", "name": "Added Track", "track_number": 1, + "disc_number": 1, "artists": [{"name": "Artist One"}]}, + {"id": "t2", "name": "Skipped Track", "track_number": 2, + "disc_number": 1, "artists": [{"name": "Artist One"}]}, + ] + }, + } + scanner = _build_scanner(album_data, [artist]) + scanner._database.has_fresh_similar_artists = lambda *args, **kwargs: False + + monkeypatch.setattr(scanner, "_backfill_missing_ids", lambda *args, **kwargs: None) + monkeypatch.setattr(scanner, "get_artist_image_url", lambda *_a, **_k: "") + monkeypatch.setattr(scanner, "get_artist_discography_for_watchlist", lambda *_a, **_k: [album]) + monkeypatch.setattr(scanner, "_get_lookback_period_setting", lambda: "30") + monkeypatch.setattr(scanner, "_get_rescan_cutoff", lambda: None) + monkeypatch.setattr(scanner, "_should_include_release", lambda *_a, **_k: True) + monkeypatch.setattr(scanner, "_should_include_track", lambda *_a, **_k: True) + monkeypatch.setattr(scanner, "is_track_missing_from_library", lambda *_a, **_k: True) + # First add succeeds, second is declined (already queued / blocklisted). + _add_results = iter([True, False]) + monkeypatch.setattr(scanner, "add_track_to_wishlist", + lambda *_a, **_k: next(_add_results)) + monkeypatch.setattr(scanner, "update_artist_scan_timestamp", lambda *_a, **_k: True) + monkeypatch.setattr(scanner, "update_similar_artists", lambda *_a, **_k: True) + monkeypatch.setattr(scanner, "_backfill_similar_artists_fallback_ids", lambda *_a, **_k: 0) + + scan_state = {"scan_run_id": "20260609-1"} + scanner.scan_watchlist_artists([artist], scan_state=scan_state) + + events = scan_state["scan_track_events"] + assert [(e["track_name"], e["status"]) for e in events] == [ + ("Added Track", "added"), + ("Skipped Track", "skipped"), + ] + assert events[0]["album_name"] == "Album One" + assert events[0]["artist_name"] == "Artist One" + # The 10-item live FIFO only carries the ADDED one, as before. + assert [a["track_name"] for a in scan_state["recent_wishlist_additions"]] == ["Added Track"] diff --git a/web_server.py b/web_server.py index bd396d6c..4f4aba3b 100644 --- a/web_server.py +++ b/web_server.py @@ -25952,9 +25952,14 @@ def start_watchlist_scan(): 'current_track_name': '', 'tracks_found_this_scan': 0, 'tracks_added_this_scan': 0, - 'recent_wishlist_additions': [] + 'recent_wishlist_additions': [], + # #831: full per-run ledger of found tracks (added vs + # skipped) so the completed-scan summary can list WHICH + # tracks the "New tracks / Added to wishlist" counts mean. + 'scan_track_events': [], + 'scan_run_id': datetime.now().strftime('%Y%m%d-%H%M%S'), }) - + scan_results = [] # Pause enrichment workers during scan to reduce API contention diff --git a/webui/static/api-monitor.js b/webui/static/api-monitor.js index 0d51d85b..c5b4d2a5 100644 --- a/webui/static/api-monitor.js +++ b/webui/static/api-monitor.js @@ -3189,6 +3189,50 @@ async function startWatchlistScan() { /** * Poll watchlist scan status */ +// #831 (Tacobell444): the scan summary said "New tracks: 19 • Added to +// wishlist: 10" with no way to see WHICH tracks. The scan now ships a per-run +// ledger (scan_track_events: track/artist/album/thumb + added|skipped) and +// this renders it as an expandable list under the completion summary. +function renderWatchlistScanTrackLedger(events) { + if (!Array.isArray(events) || events.length === 0) return ''; + const added = events.filter(e => e.status === 'added'); + const skipped = events.filter(e => e.status !== 'added'); + + const row = (e) => ` +
+ +
+
${escapeHtml(e.track_name || '')}
+
${escapeHtml(e.artist_name || '')}${e.album_name ? ' — ' + escapeHtml(e.album_name) : ''}
+
+ ${e.status === 'added' + ? 'added' + : 'skipped'} +
`; + + const section = (label, list) => list.length + ? `
${label} (${list.length})
${list.map(row).join('')}` + : ''; + + return ` + + `; +} + +function toggleWatchlistScanTracks(btn) { + const list = btn.parentElement.querySelector('.watchlist-scan-tracks'); + if (!list) return; + const open = list.style.display !== 'none'; + list.style.display = open ? 'none' : 'block'; + btn.innerHTML = (open ? 'Show tracks ' : 'Hide tracks ') + + `${open ? '▾' : '▴'}`; +} + function handleWatchlistScanData(data) { const button = document.getElementById('scan-watchlist-btn'); const liveActivity = document.getElementById('watchlist-live-activity'); @@ -3295,6 +3339,7 @@ function handleWatchlistScanData(data) { • Added to wishlist: ${addedTracks} + ${renderWatchlistScanTrackLedger(data.scan_track_events)} `; } @@ -3341,6 +3386,7 @@ function handleWatchlistScanData(data) { • Added to wishlist: ${addedTracks} + ${renderWatchlistScanTrackLedger(data.scan_track_events)} `; } diff --git a/webui/static/origin-history.js b/webui/static/origin-history.js index 090e2980..c525588d 100644 --- a/webui/static/origin-history.js +++ b/webui/static/origin-history.js @@ -99,7 +99,8 @@ function _renderOriginEntries() { return; } const ctxLabel = _originActiveTab === 'watchlist' ? 'Watchlist artist' : 'Playlist'; - body.innerHTML = _originEntries.map(e => { + + const entryRow = (e) => { const checked = _originSelected.has(e.id) ? 'checked' : ''; const thumb = e.thumb_url ? `${escapeHtml(e.title || 'Unknown')}
${escapeHtml(e.artist_name || '')}${e.album_name ? ' — ' + escapeHtml(e.album_name) : ''}
- ${escapeHtml(e.origin_context || '—')} ${e.quality ? `${escapeHtml(e.quality)}` : ''}
${escapeHtml(_originFormatTime(e.created_at))}
+
${entries.map(entryRow).join('')}
+ `).join(''); _updateOriginDeleteButton(); } +function toggleOriginGroup(btn) { + const bodyEl = btn.parentElement.querySelector('.origin-group-body'); + const caret = btn.querySelector('.origin-group-caret'); + if (!bodyEl) return; + const open = bodyEl.style.display !== 'none'; + bodyEl.style.display = open ? 'none' : ''; + if (caret) caret.textContent = open ? '▸' : '▾'; +} + function toggleOriginEntry(id, on) { if (on) _originSelected.add(id); else _originSelected.delete(id); _updateOriginDeleteButton(); diff --git a/webui/static/style.css b/webui/static/style.css index cc22d3b2..28942dda 100644 --- a/webui/static/style.css +++ b/webui/static/style.css @@ -19891,6 +19891,65 @@ body.helper-mode-active #dashboard-activity-feed:hover { margin-bottom: 10px; } +/* #831: expandable per-run track ledger under the scan summary */ +.watchlist-scan-tracks-toggle { + margin-top: 10px; + padding: 5px 14px; + background: rgba(255, 255, 255, 0.06); + border: 1px solid rgba(255, 255, 255, 0.12); + border-radius: 14px; + color: #ccc; + font-size: 12px; + cursor: pointer; + transition: background 0.15s ease; +} + +.watchlist-scan-tracks-toggle:hover { + background: rgba(255, 255, 255, 0.12); + color: #fff; +} + +.watchlist-scan-tracks { + margin-top: 10px; + max-height: 280px; + overflow-y: auto; + text-align: left; +} + +.watchlist-scan-tracks-section { + font-size: 11px; + font-weight: 600; + text-transform: uppercase; + letter-spacing: 0.4px; + color: #888; + margin: 10px 0 4px; +} + +.watchlist-scan-tracks .watchlist-live-addition-item { + position: relative; +} + +.watchlist-scan-track-badge { + margin-left: auto; + flex-shrink: 0; + padding: 2px 8px; + border-radius: 10px; + font-size: 10px; + font-weight: 600; + text-transform: uppercase; + letter-spacing: 0.3px; +} + +.watchlist-scan-track-badge.added { + background: rgba(80, 200, 120, 0.15); + color: #6fd99a; +} + +.watchlist-scan-track-badge.skipped { + background: rgba(255, 255, 255, 0.08); + color: #999; +} + /* Watchlist Search */ .watchlist-search-input { @@ -66708,6 +66767,55 @@ body.em-scroll-lock { overflow: hidden; } color: #f87171 !important; } +/* #831: origins grouped by what triggered them (artist / playlist) */ +.origin-group { + margin-bottom: 6px; +} + +.origin-group-header { + display: flex; + align-items: center; + gap: 8px; + width: 100%; + padding: 8px 12px; + background: rgba(255, 255, 255, 0.04); + border: 1px solid rgba(255, 255, 255, 0.08); + border-radius: 8px; + cursor: pointer; + text-align: left; + transition: background 0.15s ease; +} + +.origin-group-header:hover { + background: rgba(255, 255, 255, 0.08); +} + +.origin-group-caret { + color: rgba(255, 255, 255, 0.45); + font-size: 11px; + width: 12px; +} + +.origin-group-name { + flex: 1; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + font-size: 13px; + font-weight: 600; + color: rgb(var(--accent-light-rgb)); +} + +.origin-group-count { + flex: 0 0 auto; + font-size: 11px; + color: rgba(255, 255, 255, 0.45); +} + +.origin-group-body { + padding: 4px 0 2px 6px; +} + /* ── Blocklist modal (artist/album/track bans) ── */ .blocklist-modal { position: relative; From 9f12bdfef6532ef9f10633864a9d09bd023d94fe Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 9 Jun 2026 20:35:16 -0700 Subject: [PATCH 38/45] Watchlist: bespoke live scan deck + persistent per-run Scan History (#831 round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Boulder: the live display was a cramped ~600px box showing a fraction of the data the scan already tracks, with no animation and no history. Live scan deck (replaces the three-column box, full width): - Header: pulsing live dot, "x / y artists" progress text, and two live counter chips (found / added) that pop when they change. - Animated progress bar (artist index / total) with a shimmer sweep. - Stage: artist avatar with accent glow + name + readable phase line ("Checking album 2 of 5"), album art + album + current track. - "Added to wishlist this run" feed: taller, bigger art, slide-in animation that plays once per new track (feed re-renders only when it changes). - All data was already in scan_state (current_artist_index, total_artists, tracks_found/added_this_scan, current_phase) — just never displayed. The legacy fullscreen-modal markup shares element ids and lacks the new ones, so it keeps working untouched. Scan History (persistent): - New watchlist_scan_runs table — one row per run (status, timestamps, artists/found/added counts) + the full track ledger JSON. Saved at scan completion AND cancellation; idempotent on run_id; pruned to the last 100 runs. Wishlist rows erode as tracks download, so this is the durable record. - GET /api/watchlist/scan/history (runs) + /history//tracks (ledger). - New History button on the Watchlist page → modal in the origins/blocklist house style: run cards (date, cancelled chip, artists/found/added stats) expanding into the Added / Skipped track lists with art and badges. Tests: save+fetch with ledger, idempotent re-save, prune keeps newest, unknown-run empty, cancelled runs recorded. 398 watchlist/wishlist/history tests pass; JS syntax-checked; all rendered strings escaped. --- database/music_database.py | 90 ++++++++ tests/test_watchlist_scan_history.py | 68 ++++++ web_server.py | 46 ++++ webui/index.html | 56 +++-- webui/static/api-monitor.js | 78 +++++-- webui/static/style.css | 331 +++++++++++++++++++++++++++ webui/static/watchlist-history.js | 149 ++++++++++++ 7 files changed, 792 insertions(+), 26 deletions(-) create mode 100644 tests/test_watchlist_scan_history.py create mode 100644 webui/static/watchlist-history.js diff --git a/database/music_database.py b/database/music_database.py index 0fc68c4c..d87e92de 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -649,6 +649,27 @@ class MusicDatabase: logger.info(f"Added {_col} column to library_history") cursor.execute("CREATE INDEX IF NOT EXISTS idx_lh_origin ON library_history (origin, created_at DESC)") + # Watchlist scan history (#831 round 2) — one row per scan run with + # its full track ledger (added/skipped), so the Watchlist page can + # show what every past run did. Wishlist rows erode as tracks + # download, so this is the durable record. + cursor.execute(""" + CREATE TABLE IF NOT EXISTS watchlist_scan_runs ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + run_id TEXT NOT NULL UNIQUE, + profile_id INTEGER DEFAULT 1, + status TEXT NOT NULL, + started_at TIMESTAMP, + completed_at TIMESTAMP, + total_artists INTEGER DEFAULT 0, + artists_scanned INTEGER DEFAULT 0, + tracks_found INTEGER DEFAULT 0, + tracks_added INTEGER DEFAULT 0, + track_events TEXT + ) + """) + cursor.execute("CREATE INDEX IF NOT EXISTS idx_wsr_completed ON watchlist_scan_runs (completed_at DESC)") + # Auto-import history — tracks auto-import scan results and processing status cursor.execute(""" CREATE TABLE IF NOT EXISTS auto_import_history ( @@ -12544,6 +12565,75 @@ class MusicDatabase: logger.debug(f"Error adding library history entry: {e}") return False + def save_watchlist_scan_run(self, run_id, profile_id=1, status='completed', + started_at=None, completed_at=None, + total_artists=0, artists_scanned=0, + tracks_found=0, tracks_added=0, + track_events=None, keep_last=100) -> bool: + """Persist one watchlist scan run + its track ledger (#831 round 2). + + Idempotent on run_id (re-saving a run replaces it). Prunes the table to + the most recent ``keep_last`` runs so history can't grow unbounded.""" + try: + conn = self._get_connection() + cursor = conn.cursor() + cursor.execute(""" + INSERT OR REPLACE INTO watchlist_scan_runs + (run_id, profile_id, status, started_at, completed_at, + total_artists, artists_scanned, tracks_found, tracks_added, + track_events) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + """, (run_id, profile_id, status, started_at, completed_at, + total_artists, artists_scanned, tracks_found, tracks_added, + json.dumps(track_events or []))) + cursor.execute(""" + DELETE FROM watchlist_scan_runs WHERE id NOT IN ( + SELECT id FROM watchlist_scan_runs + ORDER BY completed_at DESC, id DESC LIMIT ? + ) + """, (keep_last,)) + conn.commit() + return True + except Exception as e: + logger.error(f"Error saving watchlist scan run {run_id}: {e}") + return False + + def get_watchlist_scan_runs(self, limit=30, profile_id=None): + """Recent watchlist scan runs, newest first — WITHOUT track ledgers + (fetch those per-run via get_watchlist_scan_run_events).""" + try: + conn = self._get_connection() + cursor = conn.cursor() + where = "WHERE profile_id = ?" if profile_id is not None else "" + params = ([profile_id] if profile_id is not None else []) + [limit] + cursor.execute(f""" + SELECT run_id, profile_id, status, started_at, completed_at, + total_artists, artists_scanned, tracks_found, tracks_added + FROM watchlist_scan_runs {where} + ORDER BY completed_at DESC, id DESC LIMIT ? + """, params) + return [dict(row) for row in cursor.fetchall()] + except Exception as e: + logger.error(f"Error getting watchlist scan runs: {e}") + return [] + + def get_watchlist_scan_run_events(self, run_id): + """The track ledger (added/skipped events) for one scan run.""" + try: + conn = self._get_connection() + cursor = conn.cursor() + cursor.execute( + "SELECT track_events FROM watchlist_scan_runs WHERE run_id = ?", + (run_id,)) + row = cursor.fetchone() + if not row or not row['track_events']: + return [] + events = json.loads(row['track_events']) + return events if isinstance(events, list) else [] + except Exception as e: + logger.error(f"Error getting watchlist scan run events for {run_id}: {e}") + return [] + def get_origin_cleanup_candidates(self): """Origin-tracked downloads (watchlist/playlist) annotated with the matching library track's play_count, for the Expired Download Cleaner. diff --git a/tests/test_watchlist_scan_history.py b/tests/test_watchlist_scan_history.py new file mode 100644 index 00000000..3b6f4924 --- /dev/null +++ b/tests/test_watchlist_scan_history.py @@ -0,0 +1,68 @@ +"""Watchlist scan history persistence (#831 round 2). + +Every scan run is saved to watchlist_scan_runs with its track ledger so the +Watchlist History modal can show what each past run added — wishlist rows +erode as tracks download, so this table is the durable record. +""" + +from __future__ import annotations + +import pytest + +from database.music_database import MusicDatabase + + +@pytest.fixture() +def db(tmp_path): + return MusicDatabase(str(tmp_path / 'm.db')) + + +def _events(n_added=2, n_skipped=1): + evs = [{'track_name': f'Added {i}', 'artist_name': 'A', 'album_name': 'Al', + 'album_image_url': '', 'status': 'added'} for i in range(n_added)] + evs += [{'track_name': f'Skipped {i}', 'artist_name': 'A', 'album_name': 'Al', + 'album_image_url': '', 'status': 'skipped'} for i in range(n_skipped)] + return evs + + +def test_save_and_fetch_run_with_ledger(db): + assert db.save_watchlist_scan_run( + 'run-1', status='completed', + started_at='2026-06-09T20:00:00', completed_at='2026-06-09T20:05:00', + total_artists=63, artists_scanned=63, tracks_found=19, tracks_added=10, + track_events=_events()) + runs = db.get_watchlist_scan_runs() + assert len(runs) == 1 + r = runs[0] + assert (r['run_id'], r['status'], r['tracks_found'], r['tracks_added']) == \ + ('run-1', 'completed', 19, 10) + events = db.get_watchlist_scan_run_events('run-1') + assert [e['status'] for e in events] == ['added', 'added', 'skipped'] + + +def test_resave_is_idempotent_on_run_id(db): + db.save_watchlist_scan_run('run-1', tracks_added=10) + db.save_watchlist_scan_run('run-1', tracks_added=11) + runs = db.get_watchlist_scan_runs() + assert len(runs) == 1 and runs[0]['tracks_added'] == 11 + + +def test_prune_keeps_most_recent(db): + for i in range(1, 8): + db.save_watchlist_scan_run( + f'run-{i}', completed_at=f'2026-06-09T20:0{i}:00', keep_last=5) + runs = db.get_watchlist_scan_runs() + assert len(runs) == 5 + assert runs[0]['run_id'] == 'run-7' # newest first + assert all(r['run_id'] != 'run-1' for r in runs) # oldest pruned + + +def test_events_for_unknown_run_empty(db): + assert db.get_watchlist_scan_run_events('nope') == [] + + +def test_cancelled_run_recorded(db): + db.save_watchlist_scan_run('run-c', status='cancelled', tracks_added=3, + track_events=_events(1, 0)) + r = db.get_watchlist_scan_runs()[0] + assert r['status'] == 'cancelled' diff --git a/web_server.py b/web_server.py index 4f4aba3b..922bb85a 100644 --- a/web_server.py +++ b/web_server.py @@ -25996,6 +25996,29 @@ def start_watchlist_scan(): else: logger.warning("Watchlist scan cancelled — skipping post-scan steps") + # #831 round 2: persist this run + its track ledger so the + # Watchlist History modal can show what every past scan did. + try: + _state = watchlist_scan_state + get_database().save_watchlist_scan_run( + run_id=_state.get('scan_run_id') or datetime.now().strftime('%Y%m%d-%H%M%S'), + profile_id=scan_profile_id, + status='cancelled' if was_cancelled else 'completed', + started_at=(_state.get('started_at').isoformat() + if _state.get('started_at') else None), + completed_at=(_state.get('completed_at') or datetime.now()).isoformat() + if not isinstance(_state.get('completed_at'), str) + else _state.get('completed_at'), + total_artists=(_state.get('summary') or {}).get('total_artists', + _state.get('total_artists', 0)), + artists_scanned=(_state.get('summary') or {}).get('successful_scans', 0), + tracks_found=_state.get('tracks_found_this_scan', 0), + tracks_added=_state.get('tracks_added_this_scan', 0), + track_events=_state.get('scan_track_events') or [], + ) + except Exception as _hist_err: + logger.error(f"Failed to persist watchlist scan run: {_hist_err}") + # Post-scan steps — skip if cancelled if not was_cancelled: # Populate discovery pool from similar artists @@ -26152,6 +26175,29 @@ def get_watchlist_scan_status(): logger.error(f"Error getting watchlist scan status: {e}") return jsonify({"success": False, "error": str(e)}), 500 +@app.route('/api/watchlist/scan/history', methods=['GET']) +def get_watchlist_scan_history(): + """Recent watchlist scan runs (counts only — ledgers fetched per run).""" + try: + limit = min(int(request.args.get('limit', 30) or 30), 100) + runs = get_database().get_watchlist_scan_runs(limit=limit) + return jsonify({"success": True, "runs": runs}) + except Exception as e: + logger.error(f"Error getting watchlist scan history: {e}") + return jsonify({"success": False, "error": str(e)}), 500 + + +@app.route('/api/watchlist/scan/history//tracks', methods=['GET']) +def get_watchlist_scan_history_tracks(run_id): + """The track ledger (added/skipped) for one past scan run.""" + try: + events = get_database().get_watchlist_scan_run_events(run_id) + return jsonify({"success": True, "events": events}) + except Exception as e: + logger.error(f"Error getting watchlist scan history tracks: {e}") + return jsonify({"success": False, "error": str(e)}), 500 + + @app.route('/api/watchlist/scan/cancel', methods=['POST']) def cancel_watchlist_scan(): """Cancel a running watchlist scan""" diff --git a/webui/index.html b/webui/index.html index a0c5722b..2ebf113d 100644 --- a/webui/index.html +++ b/webui/index.html @@ -6975,20 +6975,45 @@