Discogs: fix master/release ID collision fetching the wrong album

Discogs masters (/masters/{id}) and releases (/releases/{id}) share one
numeric ID namespace, so release N and master N are different albums.
search_albums() (type=release) yields RELEASE ids, but get_album,
get_album_tracks and _fetch_and_cache_album all tried /masters first and
only fell back to /releases. Whenever a release id collided with a real
master id, the master lookup succeeded and returned a valid-but-wrong
album — the fallback never fired.

Tag the object type into the id string ('r12345' / 'm12345') at the single
parse point (Album.from_discogs_release) and route each fetch to the
matching endpoint. Legacy untagged ids are tried release-first (then master),
which self-heals pre-existing bad matches without a migration. The
enrichment worker now persists the tagged id too.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

core/discogs_client.py

@@ -79,6 +79,51 @@ def _clean_discogs_artist_name(name: Optional[str]) -> str:
     return _DISCOGS_DISAMBIG_RE.sub('', name).strip()
 
 
+# --- Discogs album ID typing -------------------------------------------------
+# Discogs has two album object types — masters (/masters/{id}) and releases
+# (/releases/{id}) — whose numeric IDs share one space, so release N and master
+# N are DIFFERENT albums. A bare numeric ID is therefore ambiguous. We tag the
+# type into the ID string ('m12345' / 'r12345') at the point we parse it, so the
+# correct endpoint can be chosen later without guessing. (Artist IDs are a single
+# namespace and stay untagged.)
+
+def _discogs_album_kind(data: Dict[str, Any]) -> str:
+    """Classify a Discogs album payload as 'master' or 'release'.
+
+    Search results and artist-discography items carry an explicit ``type``;
+    full detail responses don't, but only master detail has ``main_release``."""
+    t = (data.get('type') or '').lower()
+    if t in ('master', 'release'):
+        return t
+    return 'master' if 'main_release' in data else 'release'
+
+
+def _tag_discogs_album_id(raw_id: Any, kind: str) -> str:
+    """``'12345'`` + ``'master'`` -> ``'m12345'``; empty input -> ``''``."""
+    s = str(raw_id or '').strip()
+    if not s:
+        return ''
+    return f"{'m' if kind == 'master' else 'r'}{s}"
+
+
+def _discogs_album_endpoints(album_id: Any) -> List[str]:
+    """Map a (possibly tagged) album ID to the API path(s) to try, in order.
+
+      ``'m12345'`` -> ``['/masters/12345']``
+      ``'r12345'`` -> ``['/releases/12345']``
+      ``'12345'``  (legacy untagged) -> ``['/releases/12345', '/masters/12345']``
+
+    Legacy bare IDs are tried release-first because stored IDs originate
+    overwhelmingly from search / manual-match / collection sync (all releases);
+    this also self-heals pre-fix bad matches. Returns ``[]`` for unusable input."""
+    s = str(album_id or '').strip()
+    if len(s) > 1 and s[0] in ('m', 'r') and s[1:].isdigit():
+        return [f"/{'masters' if s[0] == 'm' else 'releases'}/{s[1:]}"]
+    if s.isdigit():
+        return [f'/releases/{s}', f'/masters/{s}']
+    return []
+
+
 # --- Shared dataclasses (same shape as iTunes/Deezer/Spotify) ---
 
 @dataclass
@@ -304,7 +349,7 @@ class Album:
             external_urls['discogs_api'] = release_data['resource_url']
 
         return cls(
-            id=str(release_data.get('id', '')),
+            id=_tag_discogs_album_id(release_data.get('id', ''), _discogs_album_kind(release_data)),
             name=title,
             artists=artists,
             release_date=release_date,
@@ -643,10 +688,13 @@ class DiscogsClient:
         if cached and cached.get('title'):
             data = cached
         else:
-            # Try as master first (artist discography returns master IDs)
+            # Hit the endpoint that matches the ID's type (tag-driven, no guessing).
-            data = self._api_get(f'/masters/{release_id}')
+            data = None
-            if not data or not data.get('title'):
+            for path in _discogs_album_endpoints(release_id):
-                data = self._api_get(f'/releases/{release_id}')
+                data = self._api_get(path)
+                if data and data.get('title'):
+                    break
+                data = None
             if not data:
                 return None
             cache.store_entity('discogs', 'album', release_id, data)
@@ -767,10 +815,13 @@ class DiscogsClient:
         if cached:
             return cached
 
-        # Try as master first (master IDs are used in artist discography)
+        # Hit the endpoint that matches the ID's type (tag-driven, no guessing).
-        data = self._api_get(f'/masters/{release_id}')
+        data = None
-        if not data or not data.get('tracklist'):
+        for path in _discogs_album_endpoints(release_id):
-            data = self._api_get(f'/releases/{release_id}')
+            data = self._api_get(path)
+            if data and data.get('tracklist'):
+                break
+            data = None
         if not data or not data.get('tracklist'):
             return None
 
@@ -782,7 +833,7 @@ class DiscogsClient:
             image_url = (primary or images[0]).get('uri')
 
         album_info = {
-            'id': str(data.get('id', release_id)),
+            'id': str(release_id),
             'name': data.get('title', ''),
             'images': [{'url': image_url, 'height': 600, 'width': 600}] if image_url else [],
             'release_date': str(data.get('year', '')) if data.get('year') else '',
@@ -871,9 +922,13 @@ class DiscogsClient:
         cached = cache.get_entity('discogs', 'album', str(release_id))
         if cached and cached.get('title'):
             return cached
-        data = self._api_get(f'/masters/{release_id}')
+        # Hit the endpoint that matches the ID's type (tag-driven, no guessing).
-        if not data or not data.get('title'):
+        data = None
-            data = self._api_get(f'/releases/{release_id}')
+        for path in _discogs_album_endpoints(release_id):
+            data = self._api_get(path)
+            if data and data.get('title'):
+                break
+            data = None
         if data:
             cache.store_entity('discogs', 'album', str(release_id), data)
         return data

core/discogs_worker.py

@@ -17,7 +17,7 @@ from typing import Optional, Dict, Any
 from datetime import datetime, timedelta
 from utils.logging_config import get_logger
 from database.music_database import MusicDatabase
-from core.discogs_client import DiscogsClient
+from core.discogs_client import DiscogsClient, _discogs_album_kind, _tag_discogs_album_id
 from core.worker_utils import accept_artist_match, interruptible_sleep, set_album_api_track_count
 
 logger = get_logger("discogs_worker")
@@ -436,7 +436,9 @@ class DiscogsWorker:
             conn = self.db._get_connection()
             cursor = conn.cursor()
 
-            discogs_id = str(data.get('id', ''))
+            # Tag the ID with its Discogs type so later re-fetches hit the right
+            # endpoint (master vs release share one numeric space).
+            discogs_id = _tag_discogs_album_id(data.get('id', ''), _discogs_album_kind(data))
             genres = json.dumps(data.get('genres', []))
             styles = json.dumps(data.get('styles', []))
             labels = data.get('labels', [])

tests/test_discogs_id_typing.py

@@ -0,0 +1,189 @@
+"""Tests for Discogs master-vs-release ID disambiguation.
+
+Discogs has two album object types — masters (``/masters/{id}``) and releases
+(``/releases/{id}``) — whose numeric IDs share ONE namespace, so release N and
+master N are different albums. The old fetch code tried ``/masters/{id}`` first
+and fell back to ``/releases/{id}``; because ``search_albums`` only ever yields
+RELEASE ids, any release id that happened to collide with a real master id
+returned a valid-but-WRONG album (the fallback never fired). See
+``core.discogs_client`` ID-typing helpers.
+
+The fix tags the type into the id string ('r12345' / 'm12345') at parse time and
+routes each fetch to the matching endpoint, with legacy bare ids tried
+release-first (and master only as a fallback).
+"""
+
+import pytest
+
+from core.discogs_client import (
+    Album,
+    DiscogsClient,
+    _discogs_album_endpoints,
+    _discogs_album_kind,
+    _tag_discogs_album_id,
+)
+
+
+# ---------------------------------------------------------------------------
+# _discogs_album_endpoints — the routing table
+# ---------------------------------------------------------------------------
+
+def test_release_tagged_id_hits_only_releases():
+    assert _discogs_album_endpoints('r12345') == ['/releases/12345']
+
+
+def test_master_tagged_id_hits_only_masters():
+    assert _discogs_album_endpoints('m12345') == ['/masters/12345']
+
+
+def test_legacy_bare_id_tries_release_first_then_master():
+    # The crux of the fix: bare ids are release-first, NOT master-first.
+    assert _discogs_album_endpoints('12345') == ['/releases/12345', '/masters/12345']
+
+
+def test_unusable_ids_return_empty():
+    assert _discogs_album_endpoints('') == []
+    assert _discogs_album_endpoints(None) == []
+    assert _discogs_album_endpoints('not-an-id') == []
+    # A lone letter prefix with no digits is not a valid tagged id.
+    assert _discogs_album_endpoints('r') == []
+
+
+# ---------------------------------------------------------------------------
+# _discogs_album_kind / _tag_discogs_album_id — classification + tagging
+# ---------------------------------------------------------------------------
+
+def test_kind_from_explicit_type():
+    assert _discogs_album_kind({'type': 'master'}) == 'master'
+    assert _discogs_album_kind({'type': 'release'}) == 'release'
+
+
+def test_kind_full_master_detail_has_main_release():
+    # Full /masters/{id} responses carry no `type`, but do carry `main_release`.
+    assert _discogs_album_kind({'id': 1, 'main_release': 42, 'title': 'X'}) == 'master'
+
+
+def test_kind_full_release_detail_defaults_release():
+    # Full /releases/{id} responses carry no `type` and no `main_release`.
+    assert _discogs_album_kind({'id': 1, 'title': 'X', 'master_id': 42}) == 'release'
+
+
+def test_tagging():
+    assert _tag_discogs_album_id('123', 'master') == 'm123'
+    assert _tag_discogs_album_id('123', 'release') == 'r123'
+    assert _tag_discogs_album_id('', 'release') == ''
+    assert _tag_discogs_album_id(None, 'master') == ''
+
+
+# ---------------------------------------------------------------------------
+# Album.from_discogs_release — the single tagging point
+# ---------------------------------------------------------------------------
+
+def test_search_result_tagged_as_release():
+    # search_albums uses type=release; results carry type='release'.
+    album = Album.from_discogs_release({'id': 999, 'title': 'Radiohead - OK Computer',
+                                        'type': 'release'})
+    assert album.id == 'r999'
+
+
+def test_discography_master_tagged_as_master():
+    album = Album.from_discogs_release({'id': 777, 'title': 'OK Computer', 'type': 'master'})
+    assert album.id == 'm777'
+
+
+def test_full_master_detail_tagged_as_master():
+    album = Album.from_discogs_release({'id': 777, 'title': 'OK Computer', 'main_release': 12})
+    assert album.id == 'm777'
+
+
+# ---------------------------------------------------------------------------
+# Fetch routing — the regression lock on the original bug
+# ---------------------------------------------------------------------------
+
+class _FakeCache:
+    """Always-miss metadata cache."""
+    def get_entity(self, *a, **k):
+        return None
+
+    def store_entity(self, *a, **k):
+        pass
+
+    def get_search_results(self, *a, **k):
+        return None
+
+    def store_search_results(self, *a, **k):
+        pass
+
+    def store_entities_bulk(self, *a, **k):
+        pass
+
+
+@pytest.fixture
+def client(monkeypatch):
+    monkeypatch.setattr('core.discogs_client.get_metadata_cache', lambda: _FakeCache())
+    return DiscogsClient(token='test-token')
+
+
+def _record(client, monkeypatch, responses):
+    """Replace _api_get with a recorder that returns `responses[path]`."""
+    calls = []
+
+    def fake_api_get(endpoint, params=None):
+        calls.append(endpoint)
+        return responses.get(endpoint)
+
+    monkeypatch.setattr(client, '_api_get', fake_api_get)
+    return calls
+
+
+def test_get_album_release_id_never_hits_masters(client, monkeypatch):
+    """A release-tagged id must NOT touch /masters — that was the bug."""
+    calls = _record(client, monkeypatch, {
+        '/releases/249504': {'id': 249504, 'title': 'The Real Album', 'artists': [{'name': 'A'}]},
+        '/masters/249504': {'id': 249504, 'title': 'A DIFFERENT Album', 'artists': [{'name': 'B'}]},
+    })
+    result = client.get_album('r249504', include_tracks=False)
+    assert calls == ['/releases/249504']            # master endpoint never consulted
+    assert result['name'] == 'The Real Album'
+
+
+def test_get_album_master_id_hits_masters_only(client, monkeypatch):
+    calls = _record(client, monkeypatch, {
+        '/masters/777': {'id': 777, 'title': 'Master Album', 'main_release': 1},
+    })
+    result = client.get_album('m777', include_tracks=False)
+    assert calls == ['/masters/777']
+    assert result['name'] == 'Master Album'
+
+
+def test_legacy_bare_id_release_first(client, monkeypatch):
+    """Legacy untagged id resolves as a release without ever hitting /masters
+    when the release lookup succeeds."""
+    calls = _record(client, monkeypatch, {
+        '/releases/249504': {'id': 249504, 'title': 'The Real Album'},
+        '/masters/249504': {'id': 249504, 'title': 'A DIFFERENT Album'},
+    })
+    result = client.get_album('249504', include_tracks=False)
+    assert calls == ['/releases/249504']
+    assert result['name'] == 'The Real Album'
+
+
+def test_legacy_bare_id_falls_back_to_master(client, monkeypatch):
+    """If the release lookup yields nothing, the bare id still tries master."""
+    calls = _record(client, monkeypatch, {
+        '/releases/777': None,
+        '/masters/777': {'id': 777, 'title': 'Master Only', 'main_release': 1},
+    })
+    result = client.get_album('777', include_tracks=False)
+    assert calls == ['/releases/777', '/masters/777']
+    assert result['name'] == 'Master Only'
+
+
+def test_fetch_and_cache_release_id_never_hits_masters(client, monkeypatch):
+    calls = _record(client, monkeypatch, {
+        '/releases/249504': {'id': 249504, 'title': 'The Real Album'},
+        '/masters/249504': {'id': 249504, 'title': 'A DIFFERENT Album'},
+    })
+    data = client._fetch_and_cache_album('r249504')
+    assert calls == ['/releases/249504']
+    assert data['title'] == 'The Real Album'