Merge pull request #595 from Nezreka/fix/deezer-contributors-cache-pollution

Fix Deezer contributors tagging silently dropping for cache-polluted …

core/deezer_client.py

@@ -89,6 +89,33 @@ def _upgrade_deezer_cover_url(url: str, target_size: int = _DEEZER_MAX_COVER_SIZ
     return _DEEZER_CDN_SIZE_PATTERN.sub(f'/{target_size}x{target_size}-', url, count=1)
 
 
+def _is_full_track_payload(payload: Optional[Dict[str, Any]]) -> bool:
+    """Distinguish a full `/track/<id>` cache hit from partial album-tracks data.
+
+    Three Deezer endpoints feed the per-track cache:
+      - `/track/<id>` — full record, includes both `track_position` AND
+        `contributors` (the multi-artist list the contributors-upgrade
+        path reads).
+      - `/album/<id>/tracks` — partial; includes `track_position` but
+        omits `contributors`.
+      - `/search/track` — minimal; lacks `track_position`.
+
+    Pre-fix `get_track_details` only checked `track_position`, so
+    partial album-tracks payloads were treated as full hits and the
+    contributors-upgrade silently fell back to single-artist tagging
+    whenever an album had been fetched before its individual tracks
+    were post-processed (issue #588).
+
+    `contributors` key presence is the load-bearing distinction —
+    `[]` is a valid value for genuinely single-artist tracks fetched
+    via the per-track endpoint, so test for key membership not
+    truthiness.
+    """
+    if not isinstance(payload, dict):
+        return False
+    return 'track_position' in payload and 'contributors' in payload
+
+
 # ==================== Dataclasses (match iTunesClient / SpotifyClient format) ====================
 
 @dataclass
@@ -546,14 +573,9 @@ class DeezerClient:
         """Get detailed track info — returns Spotify-compatible dict (metadata source interface)"""
         cache = get_metadata_cache()
         cached = cache.get_entity('deezer', 'track', str(track_id))
-        if cached and cached.get('title'):
-            # Search results are cached with minimal data (no track_position).
-            # Only use cache if it has track_position — the key field from /track/{id}.
-            # Search results include 'isrc' and 'release_date' but NOT track_position,
-            # so those fields alone are not sufficient to distinguish full from partial data.
-            if 'track_position' in cached:
-                return self._build_enhanced_track(cached)
-            # Otherwise fall through to fetch full data from API
+        if cached and cached.get('title') and _is_full_track_payload(cached):
+            return self._build_enhanced_track(cached)
+        # Otherwise fall through to fetch full data from API
 
         data = self._api_get(f'track/{track_id}')
         if not data:

tests/metadata/test_deezer_track_cache_validity.py

@@ -0,0 +1,208 @@
+"""Pin the Deezer per-track cache validity check.
+
+Issue #588: contributors tagging worked for some tracks and not others.
+Root cause was cache pollution — `/album/<id>/tracks` cached partial
+records under the same key as `/track/<id>`, and `get_track_details`
+was using `track_position` alone as the "full payload" sentinel. Both
+endpoints set track_position; only `/track/<id>` sets contributors.
+
+These tests pin the corrected sentinel (`_is_full_track_payload`) so
+the regression can't silently come back.
+"""
+
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+from core.deezer_client import _is_full_track_payload
+
+
+# ────────────────────────────────────────────────────────────────────
+# Pure helper — payload-shape classification
+# ────────────────────────────────────────────────────────────────────
+
+def test_full_track_endpoint_payload_is_valid():
+    payload = {
+        'id': 12345,
+        'title': 'Erased',
+        'track_position': 1,
+        'contributors': [{'name': 'Whipped Cream'}, {'name': 'Andrea Botez'}],
+        'artist': {'name': 'Whipped Cream'},
+        'album': {'id': 1, 'title': 'Erased'},
+    }
+    assert _is_full_track_payload(payload) is True
+
+
+def test_full_track_with_empty_contributors_list_is_valid():
+    # Single-artist track from /track/<id> still emits contributors=[]
+    # The KEY presence is what matters, not truthiness.
+    payload = {
+        'id': 12345,
+        'title': 'Solo Track',
+        'track_position': 1,
+        'contributors': [],
+        'artist': {'name': 'Solo Artist'},
+    }
+    assert _is_full_track_payload(payload) is True
+
+
+def test_album_tracks_payload_missing_contributors_is_partial():
+    # The exact shape /album/<id>/tracks returns per item — has
+    # track_position but no contributors. Pre-fix this passed the
+    # `track_position in cached` check; post-fix it correctly falls
+    # through to a fresh /track/<id> fetch.
+    payload = {
+        'id': 12345,
+        'title': 'Sacrifice',
+        'track_position': 1,
+        'duration': 180,
+        'artist': {'name': 'Andrea Botez'},
+    }
+    assert _is_full_track_payload(payload) is False
+
+
+def test_search_payload_without_track_position_is_partial():
+    payload = {
+        'id': 12345,
+        'title': 'Sacrifice',
+        'artist': {'name': 'Andrea Botez'},
+        'isrc': 'XX1234567890',
+    }
+    assert _is_full_track_payload(payload) is False
+
+
+def test_none_or_non_dict_payload_is_partial():
+    assert _is_full_track_payload(None) is False
+    assert _is_full_track_payload([]) is False
+    assert _is_full_track_payload('string') is False
+    assert _is_full_track_payload(0) is False
+
+
+def test_empty_dict_is_partial():
+    assert _is_full_track_payload({}) is False
+
+
+# ────────────────────────────────────────────────────────────────────
+# get_track_details — cache + fetch interaction
+# ────────────────────────────────────────────────────────────────────
+
+@pytest.fixture
+def deezer_client():
+    """Build a DeezerClient with mocked HTTP + cache. Bypasses __init__
+    auth/config requirements."""
+    from core.deezer_client import DeezerClient
+    client = DeezerClient.__new__(DeezerClient)
+    client._api_get = MagicMock()
+    return client
+
+
+def _patch_cache(cached_payload):
+    """Patch the module-level cache lookup. Returns the patched cache
+    mock so callers can assert on store_entity calls."""
+    cache = MagicMock()
+    cache.get_entity.return_value = cached_payload
+    cache.store_entity = MagicMock()
+    return patch('core.deezer_client.get_metadata_cache', return_value=cache), cache
+
+
+def test_cache_hit_with_full_payload_skips_api_call(deezer_client):
+    full = {
+        'id': 12345,
+        'title': 'Erased',
+        'track_position': 1,
+        'contributors': [{'name': 'Whipped Cream'}, {'name': 'Andrea Botez'}],
+        'artist': {'name': 'Whipped Cream'},
+        'album': {'id': 1, 'title': 'Erased', 'nb_tracks': 1},
+    }
+    cache_patch, cache = _patch_cache(full)
+    with cache_patch:
+        result = deezer_client.get_track_details('12345')
+
+    assert result is not None
+    assert result['artists'] == ['Whipped Cream', 'Andrea Botez']
+    deezer_client._api_get.assert_not_called()
+    cache.store_entity.assert_not_called()
+
+
+def test_cache_hit_with_partial_album_tracks_payload_refetches(deezer_client):
+    """The bug from #588 — partial album-tracks data should NOT be
+    treated as a full hit. Post-fix the client re-fetches."""
+    partial = {
+        'id': 12345,
+        'title': 'Sacrifice',
+        'track_position': 1,
+        'artist': {'name': 'Andrea Botez'},
+    }
+    fresh = {
+        'id': 12345,
+        'title': 'Sacrifice',
+        'track_position': 1,
+        'contributors': [{'name': 'Andrea Botez'}, {'name': 'Grabbitz'}],
+        'artist': {'name': 'Andrea Botez'},
+        'album': {'id': 1, 'title': 'Sacrifice', 'nb_tracks': 1},
+    }
+    cache_patch, cache = _patch_cache(partial)
+    deezer_client._api_get.return_value = fresh
+    with cache_patch:
+        result = deezer_client.get_track_details('12345')
+
+    assert result is not None
+    assert result['artists'] == ['Andrea Botez', 'Grabbitz']
+    deezer_client._api_get.assert_called_once_with('track/12345')
+    cache.store_entity.assert_called_once_with('deezer', 'track', '12345', fresh)
+
+
+def test_cache_miss_fetches_fresh(deezer_client):
+    cache_patch, cache = _patch_cache(None)
+    fresh = {
+        'id': 12345,
+        'title': 'Sacrifice',
+        'track_position': 1,
+        'contributors': [{'name': 'Andrea Botez'}, {'name': 'Grabbitz'}],
+        'artist': {'name': 'Andrea Botez'},
+        'album': {'id': 1, 'title': 'Sacrifice', 'nb_tracks': 1},
+    }
+    deezer_client._api_get.return_value = fresh
+    with cache_patch:
+        result = deezer_client.get_track_details('12345')
+
+    assert result is not None
+    assert result['artists'] == ['Andrea Botez', 'Grabbitz']
+    deezer_client._api_get.assert_called_once_with('track/12345')
+    cache.store_entity.assert_called_once()
+
+
+def test_cache_hit_with_search_shape_refetches(deezer_client):
+    """Search results lack track_position — same fall-through path as
+    partial album-tracks data."""
+    search_shape = {
+        'id': 12345,
+        'title': 'Sacrifice',
+        'artist': {'name': 'Andrea Botez'},
+        'isrc': 'XX1234567890',
+    }
+    fresh = {
+        'id': 12345,
+        'title': 'Sacrifice',
+        'track_position': 1,
+        'contributors': [{'name': 'Andrea Botez'}, {'name': 'Grabbitz'}],
+        'artist': {'name': 'Andrea Botez'},
+        'album': {'id': 1, 'title': 'Sacrifice', 'nb_tracks': 1},
+    }
+    cache_patch, _ = _patch_cache(search_shape)
+    deezer_client._api_get.return_value = fresh
+    with cache_patch:
+        result = deezer_client.get_track_details('12345')
+
+    assert result is not None
+    assert result['artists'] == ['Andrea Botez', 'Grabbitz']
+    deezer_client._api_get.assert_called_once()
+
+
+def test_api_failure_returns_none(deezer_client):
+    cache_patch, _ = _patch_cache(None)
+    deezer_client._api_get.return_value = None
+    with cache_patch:
+        result = deezer_client.get_track_details('12345')
+
+    assert result is None

webui/static/helper.js

@@ -3416,6 +3416,7 @@ const WHATS_NEW = {
     '2.5.2': [
         // --- May 13, 2026 — 2.5.2 release ---
         { date: 'May 13, 2026 — 2.5.2 release' },
+        { title: 'Deezer: Contributing Artist Tagging Now Consistent', desc: 'github issue #588: contributors tagging worked for some tracks but silently dropped them for others — most reproducibly for tracks whose ALBUM was fetched before the per-track post-process ran. trace: `core/deezer_client.py:get_track_details` cache check used `track_position` as the "full payload" sentinel, but BOTH `/track/<id>` AND `/album/<id>/tracks` set that field. only `/track/<id>` sets the `contributors` array. when album-tracks data hit the cache first, `get_track_details` returned the partial record → `_build_enhanced_track` found no contributors → the metadata-source contributors-upgrade silently fell back to single-artist. fix: lifted cache-validity to a pure helper `_is_full_track_payload` that requires BOTH `track_position` AND `contributors` key presence (empty list `[]` is valid — single-artist tracks fetched via `/track/<id>` carry it explicitly). partial cache hits now fall through to a fresh `/track/<id>` fetch. 11 boundary tests pin every shape: full payload, single-artist with empty contributors list, partial album-tracks shape, search-result shape, none/non-dict, cache-hit/cache-miss/api-failure paths.', page: 'downloads' },
         { title: 'Server Playlists: Find & Add Now Persists As A Permanent Match', desc: 'github issue #585: when a spotify track name had a versioned suffix not present in the local file (e.g. "Iron Man - 2012 - Remaster" vs "Iron Man") the auto-matcher missed the pair. user could click Find & Add to manually pick the right local file — that worked, file got added to the plex playlist — but the source spotify track stayed in Missing while the added file showed up under Extra, because the matcher had no record of the user-confirmed pairing. on the next sync the source track would re-quarantine and try to download all over again. fix: every Find & Add selection now writes a `(spotify_track_id → server_track_id)` override into `sync_match_cache` at confidence=1.0. the matching algorithm runs an override pass BEFORE the existing exact and fuzzy passes, so any user-confirmed pair short-circuits straight to "matched" without going through normalization at all. covers every kind of mismatch — dash-suffix remasters, covers / karaoke versions, alt masters, cross-language titles, typo\'d local files, anything. logic lifted to `core/sync/match_overrides.py` (pure helpers `resolve_match_overrides` + `record_manual_match`). 18 boundary tests pin: cache-hit pairs, cache-miss falls through, stale-cache (server track removed) handled gracefully, two sources pointing at same server track (UNIQUE-violation defense), str/int id coercion, partial cache hits, defensive against non-dict inputs and DB exceptions. legacy entries without `source_track_id` (non-mirrored playlists) just skip the override path. works across plex / jellyfin / navidrome.', page: 'sync' },
         { title: 'Quarantine Management — See, Approve, Delete Files Without Touching The Filesystem', desc: 'github issue #584: quarantined files used to just sit in `ss_quarantine/` with a thin sidecar — no UI, no recovery, no way to see what got dropped or why. new **Quarantine** tab on the existing Library History modal (downloads page → Download History button) lists every quarantined file with the same row chrome as the Downloads + Server Imports tabs: thumb placeholder, expected track + artist, original filename, trigger badge (Duration / AcoustID / Bit Depth), relative time, expandable details panel showing the full failure reason. three per-row actions: **Approve** (restores the file, re-runs post-processing with ONLY the failing check skipped, lands in your library with full tags + lyrics + scan), **Recover** (legacy fallback for entries quarantined before this PR with thin sidecars — moves to Staging so you finish via Import flow), **Delete** (permanent removal of file + sidecar). all three use the themed soulsync confirm modal + toast feedback (no native browser alert / confirm). per-check bypass means approving a duration-mismatch file still runs AcoustID; approving an AcoustID failure still runs bit-depth — other quality gates stay live so you can only override one trigger at a time. files that fail a different check after approval get re-quarantined with the new trigger label so you can decide again. sidecar now persists the full json-safe context so approve has everything the pipeline needs to re-process. download modal status differentiates "🛡️ Quarantined" from "❌ Failed" so recoverable files are visible at a glance. logic lifted to pure helpers in `core/imports/quarantine.py` (list / delete / approve / recover_to_staging / serialize_quarantine_context) with 27 boundary tests covering orphan files / orphan sidecars / corrupt sidecars / collision-safe filename restoration / full-context vs thin-sidecar dispatch / json round-trip safety. four new endpoints. pipeline change is per-check conditionals at the existing quarantine sites — no blanket skip-all flag.', page: 'downloads' },
         { title: 'Configurable Duration Tolerance For Quarantined Tracks', desc: 'discord question: tracks were quarantining when their actual length drifted by a few seconds from what spotify/musicbrainz reported (3s tolerance hardcoded, 5s for tracks >10min). live recordings, alternate masterings, and some legitimate uploads routinely drift more than that. new setting on settings → metadata → post-processing: "duration tolerance (seconds)". `0 = auto` (preserves the existing 3s/5s defaults). raise it to 10 / 15 / 20 if your library has a lot of drift-prone material. capped at 60s — past that the check is effectively off. applies to ALL matched downloads (soulseek / tidal / qobuz / hifi / youtube / deezer-direct) since they all flow through the same post-process integrity check. logic lifted to a pure helper `core/imports/file_integrity.py:resolve_duration_tolerance` that coerces the config value (none / empty / 0 / negative / unparseable / above-cap) to either a float override or `None` for the auto-scaled default. 12 tests pin every input shape.', page: 'settings' },