Mirrored playlist: stop Playlist Pipeline from reverting manual Fix-popup matches
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.
This commit is contained in:
Broque Thomas
parent
acc5eb77ea
commit
39f582a690
4 changed files with 81 additions and 5 deletions
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -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
|
||||
)
|
||||
|
||||
|
|
|
|||
|
|
@ -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.' },
|
||||
|
|
|
|||
Loading…
Reference in a new issue