From 39f582a6908d0f8873a1f09e53496ba6bf6a4907 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Wed, 27 May 2026 06:59:58 -0700 Subject: [PATCH] Mirrored playlist: stop Playlist Pipeline from reverting manual Fix-popup matches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit User reported that manually mapping a mirrored-playlist track via the Fix popup (either by search or by pasting an MBID) worked end-to-end once — match saved, library track downloaded — but the next Playlist Pipeline run flipped the track back to "Provider Changed" and forced them to re-do the manual map every cycle. Three independent issues were combining to cause this: 1. Hardcoded `provider: 'spotify'` on manual-fix save `update_youtube_discovery_match` (the endpoint the Fix popup posts to, also used by mirrored playlists since the frontend routes `platform === 'mirrored'` through the YouTube endpoint) always stamped the cached match as Spotify-provided. The Fix-popup cascade actually queries the user's primary metadata source first and falls back to Spotify / Deezer / iTunes / MusicBrainz — so a user on MusicBrainz primary picking an MB result still had it saved as `provider: 'spotify'`. The next prepare-discovery call (which compares cached_provider to the active source) then immediately classified the match as drifted and pending re-discovery. Fixed by deriving `match_source` from `spotify_track.get('source')` (every *_search_tracks endpoint stamps `source` on results) with a fallback to `_get_active_discovery_source()` for the MBID-paste path (which uses the lean flat shape that doesn't carry source). `matched_data['source']` and the mirrored `extra_data['provider']` both now use the derived value. `match_source` is also recomputed in the cache-save except handler so the downstream mirrored-DB save still has it. 2. Discovery worker re-queueing manual matches as "incomplete" `run_playlist_discovery_worker` in `core/discovery/playlist.py` re-adds any track to `undiscovered_tracks` when its `matched_data` lacks `track_number` or `album.id` / `album.release_date`. The check was designed as a legacy-fix backfill for old discoveries that lost those fields to a Track-dataclass stripping bug. But manual fixes from the popup are *intentionally* lean — search- result rows don't include `track_number` (none of the search endpoints return it), and the MBID-lookup flat shape doesn't carry `album.id` / `release_date` (the recording lookup returns only `album.name`). So every manual match looked "incomplete" and got re-discovered every pipeline run, overwriting the user's pick with whatever the auto-search ranked first. Manual matches now short-circuit ahead of the incomplete-data branch. 3. `prepare_mirrored_discovery` ignored the `manual_match` flag Independent of the provider-stamping fix above, the prepare- discovery endpoint that powers the mirrored-playlist UI did its own `cached_provider != current_provider` check and didn't honour manual_match either. Defence in depth — even if a future code path stamps the wrong provider on a manual match, the flag now anchors it as cached. `has_cached` also extended so manual matches with off-provider stamps still count toward the cached tally for phase classification. Tests: - new `test_manual_match_skipped_even_when_matched_data_incomplete` in `tests/discovery/test_discovery_playlist.py` pins the worker short-circuit using a realistic MB-shape matched_data (album dict without id / release_date, no top-level track_number). 16 existing tests still green; 848 across discovery / metadata / automation suites pass. --- core/discovery/playlist.py | 13 +++++++ tests/discovery/test_discovery_playlist.py | 30 ++++++++++++++++ web_server.py | 42 +++++++++++++++++++--- webui/static/helper.js | 1 + 4 files changed, 81 insertions(+), 5 deletions(-) diff --git a/core/discovery/playlist.py b/core/discovery/playlist.py index 4bbbf1c7..f25e2999 100644 --- a/core/discovery/playlist.py +++ b/core/discovery/playlist.py @@ -125,6 +125,19 @@ def run_playlist_discovery_worker(playlists, automation_id=None, deps: PlaylistD if existing_extra.get('wing_it_fallback'): # Wing It stub — always re-attempt to find a real match undiscovered_tracks.append(track) + elif existing_extra.get('manual_match'): + # User explicitly picked this match via the Fix popup. + # Manual fixes are authoritative: they may lack + # track_number / album.id / release_date (the Fix-popup + # save shape is intentionally lean — search-result rows + # don't include track_number, and the MBID-lookup flat + # shape doesn't carry album.id), but re-running discovery + # against the active source would overwrite the user's + # deliberate pick with whatever the auto-search ranks + # first. Skip — pipeline only re-discovers when the user + # has cleared the match. + pl_skipped += 1 + total_skipped += 1 else: # Check if matched_data is complete — old discoveries may be missing # track_number/release_date due to the Track dataclass stripping them. diff --git a/tests/discovery/test_discovery_playlist.py b/tests/discovery/test_discovery_playlist.py index 8ab18063..8a8a8e9c 100644 --- a/tests/discovery/test_discovery_playlist.py +++ b/tests/discovery/test_discovery_playlist.py @@ -232,6 +232,36 @@ def test_unmatched_by_user_respected(): assert deps._db.extra_data_writes == [] +def test_manual_match_skipped_even_when_matched_data_incomplete(): + """manual_match=True must skip the incomplete-matched_data re-discovery + branch. The Fix-popup save shape is intentionally lean — search-result + rows don't carry track_number, and the MBID-lookup flat shape doesn't + carry album.id / release_date — so a manual fix always looks 'incomplete' + to the old check and used to be re-discovered every pipeline run, + overwriting the user's deliberate pick with whatever the auto-search + ranked first. Pin the fix: manual matches stay put.""" + extra = { + 'discovered': True, + 'manual_match': True, + 'provider': 'musicbrainz', + 'matched_data': { + 'id': 'mb-rec-id', + 'name': 'Coffee Break', + 'artists': ['Zeds Dead'], + 'album': {'name': 'Coffee Break'}, # no id, no release_date + 'source': 'musicbrainz', + # no track_number — Fix-popup shape never has it + }, + } + tracks = [_track(track_id=1, extra_data=extra)] + deps = _build_deps(tracks_by_playlist={'p1': tracks}) + + dp.run_playlist_discovery_worker([_playlist('p1')], deps=deps) + + # No extra_data writes — the manual match wasn't overwritten + assert deps._db.extra_data_writes == [] + + # --------------------------------------------------------------------------- # Cache hit short-circuit # --------------------------------------------------------------------------- diff --git a/web_server.py b/web_server.py index 6175cc57..0fd2ff69 100644 --- a/web_server.py +++ b/web_server.py @@ -24349,6 +24349,20 @@ def update_youtube_discovery_match(): album_obj['image_url'] = image_url album_obj['images'] = [{'url': image_url}] + # Manual fixes can come from any metadata source — the Fix-popup + # cascade queries the user's primary first, then Spotify / Deezer / + # iTunes / MusicBrainz as fallbacks. Each search endpoint stamps + # `source` on its results; the MBID-paste lookup doesn't, so fall + # back to the active discovery source. Hardcoding 'spotify' here + # used to make every non-Spotify manual match look like it had + # provider-drifted on the next prepare-discovery, triggering + # automatic re-discovery that overwrote the user's manual pick. + match_source = ( + spotify_track.get('source') + or _get_active_discovery_source() + or 'spotify' + ) + matched_data = { 'id': spotify_track['id'], 'name': spotify_track['name'], @@ -24356,7 +24370,7 @@ def update_youtube_discovery_match(): 'album': album_obj, 'duration_ms': spotify_track.get('duration_ms', 0), 'image_url': image_url, - 'source': 'spotify', + 'source': match_source, } cache_db = get_database() cache_db.save_discovery_cache_match( @@ -24366,6 +24380,13 @@ def update_youtube_discovery_match(): logger.info(f"Manual fix saved to discovery cache: {original_name} by {original_artist}") except Exception as cache_err: logger.error(f"Error saving manual fix to discovery cache: {cache_err}") + # match_source needs to exist for the mirrored-DB block below even + # if cache save failed — use the same fallback chain. + match_source = ( + spotify_track.get('source') + or _get_active_discovery_source() + or 'spotify' + ) # Persist manual fix to DB for mirrored playlists if identifier.startswith('mirrored_'): @@ -24377,7 +24398,7 @@ def update_youtube_discovery_match(): db = get_database() extra_data = { 'discovered': True, - 'provider': 'spotify', + 'provider': match_source, 'confidence': 1.0, 'matched_data': matched_data, 'manual_match': True, @@ -33671,11 +33692,17 @@ def prepare_mirrored_discovery(playlist_id): extra = track.get('extra_data') if extra and extra.get('discovered'): cached_provider = extra.get('provider', 'spotify') + is_manual = bool(extra.get('manual_match')) # If the cached result was discovered by a different provider than the # currently active one, treat it as pending so re-discovery uses the # correct source (IDs, album data, images differ between providers). - if cached_provider != _current_provider: + # Manual matches are exempt: the user explicitly picked that record, + # so re-discovery would overwrite their intentional fix every cycle. + # Without this guard, fixing a track via the Fix popup correctly + # downloads it once, but the next Playlist Pipeline run re-marks it + # "Provider Changed" and reverts the manual map. + if cached_provider != _current_provider and not is_manual: has_pending = True dur = track.get('duration_ms', 0) pre_discovered_results.append({ @@ -33757,11 +33784,16 @@ def prepare_mirrored_discovery(playlist_id): 'confidence': 0, }) - # Only treat as cached if at least one track was discovered by the current provider + # Treat as cached if at least one track was discovered by the current provider, + # OR if any track carries a manual_match (those are exempt from provider-drift + # re-discovery above, so they count as cached regardless of cached_provider). has_cached = any( t.get('extra_data') and (t['extra_data'].get('discovered') or t['extra_data'].get('discovery_attempted')) and - t['extra_data'].get('provider', 'spotify') == _current_provider + ( + t['extra_data'].get('provider', 'spotify') == _current_provider + or t['extra_data'].get('manual_match') + ) for t in tracks ) diff --git a/webui/static/helper.js b/webui/static/helper.js index a34e58b6..e3d96156 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3415,6 +3415,7 @@ function closeHelperSearch() { const WHATS_NEW = { '2.6.3': [ { unreleased: true }, + { title: 'Fix popup: manual matches survive Playlist Pipeline runs', desc: 'manually mapping a mirrored-playlist track via the Fix popup (search or MBID) saved correctly and the download went through, but the next Playlist Pipeline run kept reverting the match — the track flipped back to "Provider Changed" and you had to map it again. three things were ganging up on it: the manual-fix save always stamped `provider: \'spotify\'` even when the match came from MusicBrainz / iTunes / Deezer, so prepare-discovery saw a provider mismatch on the next run; the discovery worker also re-queued any matched_data that lacked `track_number` / `album.id` / `release_date` for re-discovery, and Fix-popup saves never carry those fields by design (search results don\'t include track_number, MBID lookups don\'t include album.id) — so manual fixes always looked "incomplete" and got overwritten; and the prepare-discovery provider check didn\'t honour the `manual_match` flag at all. all three patched — manual matches now stamp the actual source, the worker short-circuits on `manual_match` before the incomplete-data branch, and prepare-discovery treats manual fixes as cached regardless of provider drift.' }, { title: 'Fix popup: artist + track fields no longer surface unrelated covers', desc: 'separate artist / track inputs in the Fix popup used to dump both into a bare MusicBrainz keyword query — and MB\'s scorer heavily favours title matches, so searching "Coffee Break" + "Zeds Dead" surfaced random "Coffee Break" tracks by other artists ahead of the canonical Zeds Dead one. fields-mode now uses a field-scoped Lucene query that actually anchors the artist, with the old bare query kept as a fallback for diacritic mismatches like "Bjork" vs "Björk" where strict phrase match misses. results also stable-prefer entries with known track length so the canonical 3:04 sibling sits above the 0:00 duplicate.' }, { title: 'Groundwork: unified playlist source layer', desc: 'first slice of a refactor that\'ll let ListenBrainz, Last.fm radio, and SoulSync Discovery playlists live as Sync-page tabs alongside Spotify / Tidal / Qobuz / YouTube — so they can be mirrored + scheduled like the rest. this commit adds the shared adapter layer all those sources will plug into; no UI changes yet. nothing to do on your end.' }, { title: 'Auto-Sync refresh now routes through the unified source layer', desc: 'follow-up to the groundwork above. the mirrored-playlist auto-refresh handler used to have a ~190-line if/elif chain branching per source (one branch each for Spotify, Spotify public, Deezer, Tidal, YouTube). now it asks the source registry for the right adapter and calls one refresh method. behavior identical — same matched_data, same Tidal-skip-on-no-auth log, same Spotify-public-prefers-authed-API fallback. unlocks ListenBrainz / Last.fm / SoulSync Discovery as future Sync-page mirror sources without a fresh elif branch each time.' },