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