fix: look tags up by artist name rather than by MusicBrainz id

Tag lookups asked by MusicBrainz id whenever Lidarr had one, which is always.
The reasoning was that an id cannot be ambiguous the way a name can. In practice
Last.fm's mbid index is stale and partial, and it answered "the artist you
supplied could not be found" for Devo, Escape the Fate, Blasterjaxx, Frank
Carter & the Rattlesnakes and a long tail of others -- artists whose Last.fm
pages plainly exist and carry precisely the tags the moods are built from. Devo
is tagged new wave, post-punk and 80s; Escape the Fate is tagged post-hardcore,
screamo and emocore. Both were skipped.

Ask by name first, which is the index Last.fm's own site runs on, and keep the
id only as a fallback for a name Lidarr spells differently.

Failures were also being dropped without recording the attempt, so every one of
those artists was re-queried on every subsequent pass, indefinitely. They are now
distinguished: an artist that neither key resolves is recorded as fetched with
no tags and not asked about again, while a genuine failure -- a rate limit, a
bad key -- is deliberately left unrecorded so the next pass retries it. Telling
the two apart needed the service's own error number, so LastfmError now carries
it.

README.md

@@ -159,11 +159,20 @@ listening history. MusicBrainz genres arrive free with the Lidarr index and are
 no use for this: they are sparse and formal, and will not tell you a record is
 screamo or synthwave. People typing tags will.
 
-Tags are fetched once per artist — one `artist.getTopTags` call each, by
-MusicBrainz id where Lidarr has one — and refreshed every ninety days. An
-artist Last.fm has never heard of is recorded as fetched with no tags, so it is
-not asked about again on every pass. `--tag-limit` spreads the first sweep over
-several passes.
+Tags are fetched once per artist — one `artist.getTopTags` call each — and
+refreshed every ninety days. `--tag-limit` spreads the first sweep over several
+passes.
+
+Artists are looked up **by name**, not by MusicBrainz id, despite Lidarr having
+an id for every one of them. Last.fm's mbid index is stale and partial: it
+answers "the artist you supplied could not be found" for Devo, Escape the Fate,
+Blasterjaxx and a few hundred others whose pages plainly exist and carry exactly
+the tags wanted. Its name index is the one its own site runs on. The id is kept
+only as a fallback, for a name Lidarr spells differently.
+
+An artist neither key resolves is recorded as fetched with no tags, so the next
+pass does not spend a request on it again. A genuine failure — a rate limit, a
+bad key — is *not* recorded, so that one is retried.
 
 The built-in moods are chosen for this library rather than as a general
 taxonomy:

music_curator.py

@@ -266,7 +266,16 @@ WHITESPACE = re.compile(r"\s+")
 
 
 class LastfmError(Exception):
-    """A Last.fm request that failed in a way retrying will not fix."""
+    """A Last.fm request that failed in a way retrying will not fix.
+
+    `code` is the service's own error number where the failure came from the
+    API rather than the transport. Callers need it to tell "this thing does not
+    exist", which is final, from "something went wrong", which is not.
+    """
+
+    def __init__(self, message, code=None):
+        super().__init__(message)
+        self.code = code
 
 
 class LidarrError(Exception):
@@ -447,7 +456,7 @@ class Lastfm:
                 return payload
             detail = f"error {code}: {payload.get('message', '')}".strip()
             if code not in RETRYABLE_ERRORS:
-                raise LastfmError(f"{method}: {detail}")
+                raise LastfmError(f"{method}: {detail}", code=code)
             self._retry_or_raise(method, attempt, detail, None)
 
         raise LastfmError(f"{method}: gave up after {self.attempts} attempts")
@@ -1323,6 +1332,34 @@ def parse_tags(payload):
     return pairs
 
 
+def fetch_tags(client, name, mbid):
+    """Return an artist's tags, asking by name first.
+
+    By name, not by MusicBrainz id, despite Lidarr having an id for everything.
+    Last.fm's mbid index is stale and partial -- it answers "the artist you
+    supplied could not be found" for Devo, Escape the Fate and a few hundred
+    others whose pages plainly exist -- while the name index is the one its own
+    site runs on. The id is kept only as a fallback for a name Lidarr spells
+    differently.
+
+    Returns an empty list when the artist is genuinely unknown, which the caller
+    records so it is not asked again.
+    """
+    attempts = [{"artist": name}] if name else []
+    if mbid:
+        attempts.append({"mbid": mbid})
+
+    for query in attempts:
+        try:
+            return parse_tags(client.call("artist.getTopTags", {**query, "autocorrect": 1}))
+        except LastfmError as error:
+            # Error 6 here means "no such artist", which the next key may still
+            # answer. Anything else is a real failure and belongs to the caller.
+            if error.code != 6:
+                raise
+    return []
+
+
 def sync_tags(client, store, now, limit=0):
     """Fetch crowd tags for library artists that have none, or stale ones.
 
@@ -1348,20 +1385,23 @@ def sync_tags(client, store, now, limit=0):
     else:
         logger.info("fetching tags for %d artists", len(stale))
 
-    tagged = 0
+    resolved = 0
+    unknown = 0
     for artist in stale:
-        query = {"mbid": artist["mbid"]} if artist["mbid"] else {"artist": artist["name"]}
         try:
-            payload = client.call("artist.getTopTags", {**query, "autocorrect": 1})
+            pairs = fetch_tags(client, artist["name"], artist["mbid"])
         except LastfmError as error:
-            # One artist Last.fm cannot answer for is not worth losing the pass.
+            # Something went wrong rather than the artist not existing. Left
+            # unrecorded on purpose, so the next pass tries again.
             logger.warning("no tags for %s: %s", artist["name"], error)
             continue
-        store.replace_tags(artist["norm_name"], parse_tags(payload), now)
-        tagged += 1
+        store.replace_tags(artist["norm_name"], pairs, now)
+        resolved += 1
+        if not pairs:
+            unknown += 1
 
-    logger.info("tagged %d artists", tagged)
-    return tagged
+    logger.info("tagged %d artists (%d with nothing to say about them)", resolved, unknown)
+    return resolved
 
 
 def build_vibe_playlists(store, vibes, mirror_root, library_root, limit, now):

tests/conftest.py

@@ -86,7 +86,13 @@ class FakeLastfm:
             return json.dumps(self._loved(query))
         if method == "artist.gettoptags":
             key = query.get("mbid") or query.get("artist", "")
-            return json.dumps({"toptags": {"tag": self.tags.get(key, [])}})
+            if key not in self.tags:
+                # What the real service says for a key it cannot resolve, which
+                # for mbids is a great many artists whose pages plainly exist.
+                return json.dumps(
+                    {"error": 6, "message": "The artist you supplied could not be found"}
+                )
+            return json.dumps({"toptags": {"tag": self.tags[key]}})
         raise AssertionError(f"unexpected method {method}")
 
     def _recent(self, query):

tests/test_music_curator.py

@@ -1050,8 +1050,8 @@ def test_a_vibe_selects_by_tag(tmp_path):
     store, source, mirror = tagged_store(
         tmp_path,
         {
-            "artist-mbid-1": [{"name": "screamo", "count": 100}],
-            "artist-mbid-2": [{"name": "classic rock", "count": 100}],
+            "Played Band": [{"name": "screamo", "count": 100}],
+            "Silent Band": [{"name": "classic rock", "count": 100}],
         },
     )
     vibes = [{"name": "screamo", "tags": ["screamo"]}]
@@ -1066,7 +1066,7 @@ def test_a_vibe_selects_by_tag(tmp_path):
 def test_a_weakly_tagged_artist_is_below_the_threshold(tmp_path):
     """A single low-weight tag is not a genre, it is somebody's stray opinion."""
     store, source, mirror = tagged_store(
-        tmp_path, {"artist-mbid-1": [{"name": "screamo", "count": 3}]}
+        tmp_path, {"Played Band": [{"name": "screamo", "count": 3}]}
     )
     vibes = [{"name": "screamo", "tags": ["screamo"]}]
 
@@ -1079,7 +1079,7 @@ def test_a_vibe_can_be_restricted_by_release_year(tmp_path):
     """What separates eighties synth records from everything else a synthpop
     tag drags in."""
     store, source, mirror = tagged_store(
-        tmp_path, {"artist-mbid-1": [{"name": "synthpop", "count": 100}]}
+        tmp_path, {"Played Band": [{"name": "synthpop", "count": 100}]}
     )
     inside = [{"name": "eighties", "tags": ["synthpop"], "years": [1975, 1992]}]
     outside = [{"name": "nineties", "tags": ["synthpop"], "years": [1993, 1999]}]
@@ -1127,3 +1127,74 @@ def test_a_bad_vibes_file_is_refused_up_front(tmp_path, content):
 
     with pytest.raises(ValueError):
         music_curator.load_vibes(str(path))
+
+
+def test_tags_are_looked_up_by_name_not_by_mbid(tmp_path):
+    """Last.fm's mbid index is stale: it cannot find Devo or Escape the Fate by
+    one, though their pages plainly exist. Its name index can."""
+    api, source, mirror = playlist_library(tmp_path)
+    store = store_at(tmp_path)
+    music_curator.index_library(
+        music_curator.Lidarr("http://lidarr", "key", transport=api), store
+    )
+    lastfm = FakeLastfm(tags={"Played Band": [{"name": "screamo", "count": 90}]})
+
+    music_curator.sync_tags(client_for(lastfm), store, NOW)
+
+    asked = [call for call in lastfm.calls if call.get("method") == "artist.getTopTags"]
+    # The name is always tried first; the mbid only appears as a fallback for
+    # the artist that the name could not resolve.
+    assert "artist" in asked[0]
+    assert [call for call in asked if call.get("artist") == "Played Band"]
+    assert not [call for call in asked if call.get("mbid") == "artist-mbid-1"]
+    assert store.scalar("SELECT COUNT(*) FROM artist_tag WHERE tag = 'screamo'") == 1
+
+
+def test_the_mbid_is_tried_when_the_name_is_not_found(tmp_path):
+    """Kept only for a name Lidarr spells differently to Last.fm."""
+    api, source, mirror = playlist_library(tmp_path)
+    store = store_at(tmp_path)
+    music_curator.index_library(
+        music_curator.Lidarr("http://lidarr", "key", transport=api), store
+    )
+    lastfm = FakeLastfm(tags={"artist-mbid-1": [{"name": "dnb", "count": 80}]})
+
+    music_curator.sync_tags(client_for(lastfm), store, NOW)
+
+    assert store.scalar("SELECT COUNT(*) FROM artist_tag WHERE tag = 'dnb'") == 1
+    asked = [call for call in lastfm.calls if call.get("method") == "artist.getTopTags"]
+    assert any("mbid" in call for call in asked)
+
+
+def test_an_artist_neither_key_resolves_is_recorded_and_not_retried(tmp_path):
+    """Otherwise every pass spends a request on it again, for ever."""
+    api, source, mirror = playlist_library(tmp_path)
+    store = store_at(tmp_path)
+    music_curator.index_library(
+        music_curator.Lidarr("http://lidarr", "key", transport=api), store
+    )
+    lastfm = FakeLastfm(tags={})
+
+    assert music_curator.sync_tags(client_for(lastfm), store, NOW) == 2
+    spent = len(lastfm.calls)
+
+    assert music_curator.sync_tags(client_for(lastfm), store, NOW) == 0
+    assert len(lastfm.calls) == spent
+
+
+def test_a_real_failure_is_not_recorded_so_the_next_pass_retries(tmp_path):
+    """A rate limit is not the same as an artist not existing."""
+    api, source, mirror = playlist_library(tmp_path)
+    store = store_at(tmp_path)
+    music_curator.index_library(
+        music_curator.Lidarr("http://lidarr", "key", transport=api), store
+    )
+    lastfm = FakeLastfm(
+        tags={"Played Band": [], "Silent Band": []},
+        outcomes=[{"error": 10, "message": "Invalid API key"}],
+    )
+
+    music_curator.sync_tags(client_for(lastfm), store, NOW)
+
+    # One artist failed hard and must still be pending.
+    assert store.scalar("SELECT COUNT(*) FROM artist_tag_fetched") == 1