From d15b3a185d2f7be1369c9861300b76697ce6b712 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Mon, 15 Jun 2026 23:35:48 -0700 Subject: [PATCH] Track "01" bug: recover real track position instead of fabricating 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Single tracks (esp. Deezer-sourced) imported as "01 - Title" regardless of their real album position — e.g. Fly Away (track 2 of Greatest Hits) landed as 01, littering album folders with duplicate "01" files. Root cause: a Deezer single track is matched via /search/track, which omits track_position, so the context never carried the real number; then service.py + context.py fabricated a confident track_number=1 from that gap. Because the resolver puts that first, the fake 1 beat the source. It is source-agnostic (slskd-with-Deezer-metadata hits it too) — albums work because /album//tracks DOES include positions. Fix (at the shared import funnel, strictly additive): - track_number.py: new read_embedded_track_number() (mutagen, local, no network) + an optional embedded_track_number arg on resolve_track_number. The downloaded file already carries the source-written position (deemix wrote it); consult it LAST — only when metadata AND the "NN - Title" filename both come up empty — so it can only fill the gap that would otherwise hit the default-1 floor. Never overrides a value the pre-fix resolver produced (no regression for correctly-named/mistagged files). - pipeline.py: read the file tag at the resolve step and pass it in. - De-poison: service.py:217 + context.py default to 0 (the existing "unknown" sentinel, like total_tracks), NOT 1 — so the fake 1 no longer blocks recovery. Frontend already treats falsy track_number as unknown (omits it), so this also drops the bogus "1." in the UI. 13 new resolver tests incl. the no-regression precedence guards; full imports + wishlist suites green (583), no behavior change for albums. --- core/imports/context.py | 6 +- core/imports/pipeline.py | 12 ++- core/imports/track_number.py | 74 +++++++++++-- core/wishlist/service.py | 6 +- tests/imports/test_track_number_resolver.py | 112 +++++++++++++++++++- 5 files changed, 196 insertions(+), 14 deletions(-) diff --git a/core/imports/context.py b/core/imports/context.py index f718f014..024c3f36 100644 --- a/core/imports/context.py +++ b/core/imports/context.py @@ -319,7 +319,11 @@ def build_import_album_info( (album_info or {}).get("track_number") or track_info.get("track_number") or original_search.get("track_number") - or 1 + # "Track 01" bug: default to 0 (the codebase's "unknown" sentinel, + # same as total_tracks below), NOT 1. A fabricated 1 looks + # authoritative and blocks the pipeline's downstream recovery + # (embedded file tag / resolve chain); 0 lets it fall through. + or 0 ) disc_number = ( (album_info or {}).get("disc_number") diff --git a/core/imports/pipeline.py b/core/imports/pipeline.py index b6a077a9..ad6a0801 100644 --- a/core/imports/pipeline.py +++ b/core/imports/pipeline.py @@ -629,9 +629,17 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta # See ``core/imports/track_number.py`` for the resolution # chain — pure function, unit-tested in isolation, single # place to fix the rule. - from core.imports.track_number import resolve_track_number + from core.imports.track_number import resolve_track_number, read_embedded_track_number track_info_for_resolve = context.get('track_info') if isinstance(context, dict) else None - track_number = resolve_track_number(album_info, track_info_for_resolve, file_path) + # "Track 01" bug: a single Deezer track is matched via an endpoint + # that omits track_position, so the context never carried the real + # number. The downloaded file itself does (deemix/source wrote it), + # so read it as a source between metadata and the filename guess. + embedded_track_number = read_embedded_track_number(file_path) + track_number = resolve_track_number( + album_info, track_info_for_resolve, file_path, + embedded_track_number=embedded_track_number, + ) logger.debug( "Final track_number processing: source=%s album_info=%s resolved=%s", album_info.get('source', 'unknown'), diff --git a/core/imports/track_number.py b/core/imports/track_number.py index 43c98029..63b53b03 100644 --- a/core/imports/track_number.py +++ b/core/imports/track_number.py @@ -65,17 +65,64 @@ def _coerce_spotify_data(track_info: Any) -> dict: return {} +def read_embedded_track_number(file_path: str) -> Optional[int]: + """Read the track position from a downloaded audio file's own tags. + + Streaming sources (Deezer/deemix, Qobuz, Tidal) and most Soulseek + uploads write the correct album position into the file itself. That + tag is authoritative for the *source's* idea of the track's place on + its album — more reliable than a filename guess — so the resolver + consults it before falling back to the filename / default-1 floor. + + Issue #874-adjacent / "Track 01" bug: a single Deezer track is matched + via Deezer's ``/search/track`` endpoint, which omits ``track_position`` + (core/deezer_client.py), so the metadata context never carried the + real number — but the downloaded file *does* (deemix wrote it). This + recovers it with no network call. + + Returns a positive int, or None when the file has no usable + tracknumber tag / can't be read. Never raises. Handles the common + ``"2/15"`` (number/total) form by taking the leading number. + """ + if not file_path: + return None + try: + from mutagen import File as MutagenFile + audio = MutagenFile(file_path, easy=True) + if audio is None: + return None + raw = audio.get('tracknumber') + if isinstance(raw, list): + raw = raw[0] if raw else None + if raw is None: + return None + # "2/15" -> "2"; bare "2" -> "2". + text = str(raw).split('/', 1)[0].strip() + return _coerce_positive(text) + except Exception: + return None + + def resolve_track_number( album_info: Any, track_info: Any, file_path: str, + embedded_track_number: Any = None, ) -> Optional[int]: """Walk the resolution chain and return the first valid positive int found, or None when every source is missing / unusable. - Caller is responsible for the final default-1 floor — leaving - that out of this function so tests can pin "everything missing + Order: album_info -> track_info -> nested spotify_data -> filename -> + ``embedded_track_number`` (the source-written file tag, when the caller + supplies it). Caller is responsible for the final default-1 floor — + leaving that out of this function so tests can pin "everything missing returns None" separate from the floor behaviour. + + ``embedded_track_number`` is passed in (not read here) so this stays a + pure function — the file I/O lives in :func:`read_embedded_track_number`. + It is consulted **last**, only when every other source came up empty, so + it can never override a value the pre-fix resolver already produced — it + only fills the gap that would otherwise hit the default-1 floor. """ album_info = album_info if isinstance(album_info, dict) else {} track_info = track_info if isinstance(track_info, dict) else {} @@ -96,10 +143,19 @@ def resolve_track_number( # default-1 floor is the single source of that fallback — # otherwise this resolver would silently fill 1 and the # downstream floor logic would have no effect. - if not file_path: - return None - try: - from_filename = extract_explicit_track_number(file_path) - except Exception: - from_filename = None - return _coerce_positive(from_filename) + if file_path: + try: + from_filename = extract_explicit_track_number(file_path) + except Exception: + from_filename = None + ff = _coerce_positive(from_filename) + if ff is not None: + return ff + + # Embedded source-written file tag is consulted LAST — only when every + # other source (metadata + the ripped-album "NN - Title" filename) came + # up empty. This is deliberate: it can ONLY fill the gap that would + # otherwise hit the caller's default-1 floor, so it never overrides a + # value the pre-fix resolver would have used. A correctly-named file + # with a stale/wrong embedded tag is therefore never regressed. + return _coerce_positive(embedded_track_number) diff --git a/core/wishlist/service.py b/core/wishlist/service.py index 6dc8c859..df3be19a 100644 --- a/core/wishlist/service.py +++ b/core/wishlist/service.py @@ -214,7 +214,11 @@ class WishlistService: "preview_url": track_data.get("preview_url") if isinstance(track_data, dict) else None, "external_urls": track_data.get("external_urls", {}) if isinstance(track_data, dict) else {}, "popularity": track_data.get("popularity", 0) if isinstance(track_data, dict) else 0, - "track_number": track_data.get("track_number", 1) if isinstance(track_data, dict) else 1, + # "Track 01" bug: 0 = "unknown position", NOT a fabricated 1. + # A fake 1 looks authoritative and blocks the import + # pipeline's track-number recovery; 0 lets it recover the + # real position (file tag / source lookup) before the floor. + "track_number": track_data.get("track_number", 0) if isinstance(track_data, dict) else 0, "disc_number": track_data.get("disc_number", 1) if isinstance(track_data, dict) else 1, } diff --git a/tests/imports/test_track_number_resolver.py b/tests/imports/test_track_number_resolver.py index 088a4bcb..381d5f86 100644 --- a/tests/imports/test_track_number_resolver.py +++ b/tests/imports/test_track_number_resolver.py @@ -14,7 +14,7 @@ from __future__ import annotations from unittest.mock import patch -from core.imports.track_number import resolve_track_number +from core.imports.track_number import read_embedded_track_number, resolve_track_number # --------------------------------------------------------------------------- @@ -226,3 +226,113 @@ def test_non_dict_track_info_treated_as_empty(): """Defensive — non-dict track_info won't crash the resolver.""" result = resolve_track_number({}, 'not a dict', '/dir/file.flac') assert result is None + + +# --------------------------------------------------------------------------- +# "Track 01" bug (Deezer single tracks): embedded file-tag source. +# --------------------------------------------------------------------------- + + +def test_embedded_tag_used_when_metadata_missing(): + """The Deezer single-track case: context carried no position (search + endpoint omits track_position), but the downloaded file did. With the + metadata-context de-poisoned to 0/None, the embedded tag is consulted + BEFORE the filename guess.""" + result = resolve_track_number( + album_info={'track_number': 0}, # de-poisoned "unknown" + track_info={'track_number': 0}, + file_path='/dl/Lenny Kravitz - Fly Away.flac', # no number prefix + embedded_track_number=2, + ) + assert result == 2 + + +def test_real_metadata_still_beats_embedded_tag(): + """No regression for album downloads: an authoritative album_info + position wins over the file tag (they normally agree, but the + context is the source of truth when present).""" + result = resolve_track_number( + album_info={'track_number': 5}, + track_info={}, + file_path='/dl/file.flac', + embedded_track_number=2, + ) + assert result == 5 + + +def test_filename_beats_embedded_tag_no_regression(): + """SAFETY: embedded tag is consulted LAST, so a correctly-named ripped + file ('05 - Song') with a stale/wrong embedded tag is NOT regressed — + the filename still wins, exactly as before the fix.""" + result = resolve_track_number( + album_info={}, + track_info={}, + file_path='/dl/09 - Mislabelled.flac', + embedded_track_number=2, # wrong/stale tag must NOT win + ) + assert result == 9 + + +def test_embedded_only_fills_the_floor_gap(): + """When metadata AND filename are both empty (the Deezer case), the + embedded tag fills what would otherwise be the blind default-1 floor.""" + result = resolve_track_number( + album_info={}, track_info={}, + file_path='/dl/Lenny Kravitz - Fly Away.flac', # no number prefix + embedded_track_number=2, + ) + assert result == 2 + + +def test_resolver_without_embedded_arg_is_backwards_compatible(): + """The new parameter is optional — existing 3-arg callers unaffected.""" + assert resolve_track_number({}, {'track_number': 4}, '/x.flac') == 4 + + +# read_embedded_track_number — mutagen-backed file tag reader. + +def _fake_mutagen(tag_value): + """Patch target factory: returns a callable standing in for + ``mutagen.File`` that yields an object whose .get('tracknumber') + returns tag_value (mimicking easy=True list-valued tags).""" + class _Audio(dict): + pass + def _factory(path, easy=False): + a = _Audio() + if tag_value is not None: + a['tracknumber'] = tag_value + return a + return _factory + + +def test_read_embedded_handles_number_slash_total(): + with patch('mutagen.File', _fake_mutagen(['2/15'])): + assert read_embedded_track_number('/x.flac') == 2 + + +def test_read_embedded_handles_bare_number(): + with patch('mutagen.File', _fake_mutagen(['7'])): + assert read_embedded_track_number('/x.flac') == 7 + + +def test_read_embedded_none_when_tag_absent(): + with patch('mutagen.File', _fake_mutagen(None)): + assert read_embedded_track_number('/x.flac') is None + + +def test_read_embedded_none_when_file_unreadable(): + def _factory(path, easy=False): + return None + with patch('mutagen.File', _factory): + assert read_embedded_track_number('/x.flac') is None + + +def test_read_embedded_never_raises(): + def _boom(path, easy=False): + raise RuntimeError('corrupt') + with patch('mutagen.File', _boom): + assert read_embedded_track_number('/x.flac') is None + + +def test_read_embedded_empty_path_returns_none(): + assert read_embedded_track_number('') is None