diff --git a/core/downloads/staging.py b/core/downloads/staging.py index bd115cc6..6a06091a 100644 --- a/core/downloads/staging.py +++ b/core/downloads/staging.py @@ -167,15 +167,32 @@ def try_staging_match(task_id, batch_id, track, deps: StagingDeps): Returns True if a match was found and the file was moved to the transfer folder. Returns False to fall through to normal download. + + Every silent-False exit point logs at INFO with the rejection reason + so #706 / #708-class "track staged but never imported, ends up + re-added to wishlist" loops can be diagnosed from app.log without + a re-instrumentation round-trip. Per-candidate skips log at DEBUG + so the noise stays out of INFO unless explicitly turned up. """ + track_title = (track.name or '').strip() if hasattr(track, 'name') else '' + track_artist = (track.artists[0] if (hasattr(track, 'artists') and track.artists) else '').strip() + # Compact identifier for the log lines below so a multi-batch + # wishlist run can be greppable per-track. + _track_label = f"'{track_title}' by '{track_artist}'" + staging_files = deps.get_staging_file_cache(batch_id or task_id) if not staging_files: + logger.info( + "[Staging] No match attempted for %s — staging cache empty for batch %s", + _track_label, batch_id or task_id, + ) return False - track_title = track.name or '' - track_artist = track.artists[0] if track.artists else '' - if not track_title: + logger.info( + "[Staging] No match attempted for task %s — track has empty title", + task_id, + ) return False from difflib import SequenceMatcher @@ -186,12 +203,20 @@ def try_staging_match(task_id, batch_id, track, deps: StagingDeps): best_match = None best_score = 0.0 + # Track per-candidate scoring so the rejection log can show the + # near-miss that DID exist (useful when title-sim is 0.79 — one + # point below the threshold). + candidate_scores: list = [] for sf in staging_files: sf_title_variants = _staging_title_variants(sf['title'], normalize) sf_norm_artist = normalize(sf['artist']) if not sf_title_variants: + logger.debug( + "[Staging] Skip candidate %s — no usable title variants", + os.path.basename(sf.get('full_path', '?')), + ) continue # Title similarity (primary) @@ -201,6 +226,12 @@ def try_staging_match(task_id, batch_id, track, deps: StagingDeps): for candidate in sf_title_variants ) if title_sim < 0.80: + logger.debug( + "[Staging] Skip candidate %s — title_sim=%.2f below 0.80 threshold (%s vs %s)", + os.path.basename(sf.get('full_path', '?')), + title_sim, norm_title, '|'.join(sf_title_variants[:3]), + ) + candidate_scores.append((sf, title_sim, None, 0.0)) continue # Artist similarity (secondary) @@ -221,12 +252,37 @@ def try_staging_match(task_id, batch_id, track, deps: StagingDeps): else: combined = (title_sim * 0.80) + (artist_sim * 0.20) + candidate_scores.append((sf, title_sim, artist_sim, combined)) + if combined > best_score: best_score = combined best_match = sf # Require high confidence to avoid false positives if not best_match or best_score < 0.75: + # Log the rejection with the best near-miss so we can see why + # the staged files didn't claim this wishlist track. Pre-fix + # this returned False silently and the loop "download album, + # stage files, never claim them, re-add to wishlist" was + # impossible to debug from logs alone. + if candidate_scores: + near_miss = max(candidate_scores, key=lambda c: c[3]) + sf, title_sim, artist_sim, combined = near_miss + logger.info( + "[Staging] No match for %s in batch %s — best candidate %s " + "(title_sim=%.2f, artist_sim=%s, combined=%.2f) below 0.75 threshold", + _track_label, batch_id or task_id, + os.path.basename(sf.get('full_path', '?')), + title_sim, + f"{artist_sim:.2f}" if artist_sim is not None else 'n/a', + combined, + ) + else: + logger.info( + "[Staging] No match for %s in batch %s — %d staging files " + "but none had usable title variants", + _track_label, batch_id or task_id, len(staging_files), + ) return False logger.info(f"[Staging] Match found for '{track_title}' by '{track_artist}': " diff --git a/core/imports/context.py b/core/imports/context.py index c302891e..73459949 100644 --- a/core/imports/context.py +++ b/core/imports/context.py @@ -418,8 +418,12 @@ def detect_album_info_web(context, artist_context=None): context, album_info={ "album_name": album_name, - "track_number": track_info.get("track_number", 1), - "disc_number": track_info.get("disc_number", 1), + # Preserve missing numbers as None so the import pipeline + # can fall through to ``extract_track_number_from_filename`` + # at ``core/imports/pipeline.py:652`` instead of locking + # to track/disc 01 for every wishlist re-attempt. + "track_number": track_info.get("track_number"), + "disc_number": track_info.get("disc_number"), "album_image_url": album_ctx.get("image_url", ""), "confidence": 0.5, }, diff --git a/core/imports/pipeline.py b/core/imports/pipeline.py index 2f2f177d..1e2f1659 100644 --- a/core/imports/pipeline.py +++ b/core/imports/pipeline.py @@ -642,7 +642,15 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta album_info=album_info, default=original_search.get('title', 'Unknown Track'), ) - track_number = album_info.get('track_number', 1) + # Read with explicit None default — the upstream payload helpers + # (``core/wishlist/payloads.py``) now preserve missing track + # numbers as None instead of pre-filling 1. That lets the + # filename-extract fallback below actually fire on wishlist + # re-attempts whose source payload lost the position. Pre-fix, + # ``.get('track_number', 1)`` filled 1, the None-check below + # never matched, and every wishlist re-try imported as ``01 -`` + # regardless of the source file's real track number. + track_number = album_info.get('track_number') logger.debug( "Final track_number processing: source=%s album_info_track_number=%s track_number=%s", album_info.get('source', 'unknown'), diff --git a/core/wishlist/payloads.py b/core/wishlist/payloads.py index 50f3a0e7..1b6c5932 100644 --- a/core/wishlist/payloads.py +++ b/core/wishlist/payloads.py @@ -118,8 +118,15 @@ def ensure_wishlist_track_format(track_info): 'artists': artists_list, 'album': album, 'duration_ms': track_info.get('duration_ms', 0), - 'track_number': track_info.get('track_number', 1), - 'disc_number': track_info.get('disc_number', 1), + # track/disc numbers preserved as-is (no default-to-1 fallback). + # Defaulting to 1 here poisons the downstream chain — the import + # pipeline at ``core/imports/pipeline.py:645`` only runs its + # ``extract_track_number_from_filename`` fallback when this + # value is None, so a pre-filled 1 would lock every wishlist + # re-attempt to track 01 regardless of source filename. Leave + # missing values explicit so the pipeline can detect-and-recover. + 'track_number': track_info.get('track_number'), + 'disc_number': track_info.get('disc_number'), 'preview_url': track_info.get('preview_url'), 'external_urls': track_info.get('external_urls', {}), 'popularity': track_info.get('popularity', 0), @@ -156,18 +163,33 @@ def build_cancelled_task_wishlist_payload(task, profile_id: int = 1): album_raw = track_info.get('album', {}) if isinstance(album_raw, dict): + # Full-dict shape — copy as-is so release_date / id / total_tracks + # / artists survive the round-trip. Earlier this only ``setdefault``ed + # name/album_type/images, but ``dict(album_raw)`` already + # preserves every other key the source carries; the setdefaults + # only fill blanks. release_date in particular MUST survive so + # the import path-template renders the year in the folder name. album_data = dict(album_raw) album_data.setdefault('name', 'Unknown Album') album_data.setdefault('album_type', track_info.get('album_type', 'album')) if 'images' not in album_data and track_info.get('album_image_url'): album_data['images'] = [{'url': track_info.get('album_image_url')}] else: + # String-shape album — preserve every adjacent track_info field + # we know about so the constructed dict is still usable + # downstream. Pre-fix this only set name + album_type, dropping + # release_date / total_tracks / id even when the caller had them. album_data = { 'name': str(album_raw) if album_raw else 'Unknown Album', 'album_type': track_info.get('album_type', 'album'), } if track_info.get('album_image_url'): album_data['images'] = [{'url': track_info.get('album_image_url')}] + if track_info.get('album_release_date') or track_info.get('release_date'): + album_data['release_date'] = ( + track_info.get('album_release_date') + or track_info.get('release_date') + ) track_data = { 'id': track_info.get('id'), @@ -175,6 +197,13 @@ def build_cancelled_task_wishlist_payload(task, profile_id: int = 1): 'artists': formatted_artists, 'album': album_data, 'duration_ms': track_info.get('duration_ms'), + # Preserve track / disc position so a cancellation→re-add doesn't + # lock the next attempt to track 01. ``None`` is intentional — + # downstream ``core/imports/pipeline.py:645`` reads None as + # "extract from filename instead", which is what we want when + # the position genuinely isn't known. + 'track_number': track_info.get('track_number'), + 'disc_number': track_info.get('disc_number'), } source_context = { @@ -279,8 +308,11 @@ def track_object_to_dict(track_object) -> Dict[str, Any]: "preview_url": getattr(track_object, "preview_url", None), "external_urls": getattr(track_object, "external_urls", {}), "popularity": getattr(track_object, "popularity", 0), - "track_number": getattr(track_object, "track_number", 1), - "disc_number": getattr(track_object, "disc_number", 1), + # See ``ensure_wishlist_track_format`` — preserve missing + # values as None so the import pipeline's filename fallback + # can fire instead of locking to track/disc 01. + "track_number": getattr(track_object, "track_number", None), + "disc_number": getattr(track_object, "disc_number", None), } logger.debug( diff --git a/tests/wishlist/test_payloads.py b/tests/wishlist/test_payloads.py index b8cbd499..103a8026 100644 --- a/tests/wishlist/test_payloads.py +++ b/tests/wishlist/test_payloads.py @@ -106,6 +106,92 @@ def test_extract_spotify_track_from_modal_info_reconstructs_from_slskd_result(): assert out["album"]["name"] == "Album Three" +# --------------------------------------------------------------------------- +# track_number / disc_number preservation through the wishlist payload +# helpers — pins the bug A fix from PR 2/4. Pre-fix the helpers +# defaulted missing numbers to 1, which locked every wishlist retry +# to track 01 because the import pipeline's filename-extract fallback +# only fires when the value is None (not the pre-filled 1). +# --------------------------------------------------------------------------- + + +def test_ensure_wishlist_track_format_preserves_real_track_number(): + """Real track positions must survive the format helper. Pre-fix + the helper read ``track_info.get('track_number', 1)`` which always + returned 1 if the upstream payload had dropped the key — the + desired number was lost on every round-trip.""" + track = { + "name": "No Sleep Till Brooklyn", + "artist": "Beastie Boys", + "album": {"name": "Licensed to Ill", "release_date": "1986-11-15"}, + "track_number": 8, + "disc_number": 1, + } + out = payloads.ensure_wishlist_track_format(track) + assert out["track_number"] == 8 + assert out["disc_number"] == 1 + + +def test_ensure_wishlist_track_format_keeps_missing_track_number_as_none(): + """When the upstream payload doesn't carry a track number, the + helper must NOT pre-fill 1 — that poisons the chain and locks the + file to track 01. Leave None so the import pipeline's filename + fallback at ``core/imports/pipeline.py:652`` can fire.""" + track = { + "name": "Mystery Track", + "artist": "Artist", + "album": {"name": "Album"}, + } + out = payloads.ensure_wishlist_track_format(track) + assert out["track_number"] is None + assert out["disc_number"] is None + + +def test_build_cancelled_task_wishlist_payload_preserves_track_number(): + """Cancellation→re-add path was the worst offender — the payload + builder dropped track_number from the saved data entirely (didn't + even include the key). Next wishlist cycle saw missing key → + helper defaulted to 1 → file imported as 01. Now both the + cancellation payload AND the helper preserve real positions.""" + task = { + "track_info": { + "id": "trk-1", "name": "Brass Monkey", + "artists": [{"name": "Beastie Boys"}], + "album": {"name": "Licensed to Ill", "release_date": "1986-11-15"}, + "track_number": 11, + "disc_number": 1, + }, + "playlist_name": "Wishlist", + "playlist_id": "p1", + } + out = payloads.build_cancelled_task_wishlist_payload(task) + td = out["track_data"] + assert td["track_number"] == 11 + assert td["disc_number"] == 1 + # Album release_date survives the round-trip so the path template + # renders the year in the folder name. + assert td["album"]["release_date"] == "1986-11-15" + + +def test_build_cancelled_task_wishlist_payload_string_album_pulls_release_date_from_track_info(): + """When the source ``album`` field is a bare string, the payload + builder constructs an album dict from scratch — it must pull + release_date / album_image_url / etc. from the adjacent + track_info fields rather than dropping them silently.""" + task = { + "track_info": { + "id": "trk-2", "name": "Song", + "artists": [{"name": "Artist"}], + "album": "Bare String Album", + "release_date": "2020-06-01", + }, + } + out = payloads.build_cancelled_task_wishlist_payload(task) + album = out["track_data"]["album"] + assert album["name"] == "Bare String Album" + assert album["release_date"] == "2020-06-01" + + def test_ensure_wishlist_track_format_defaults_non_dict_album_to_album_type(): """When ``album`` arrives as a non-dict (legacy/reconstruction path) we must not stamp ``album_type='single'`` — that lies about the origin