Merge pull request 'fix: index albums through the endpoint Lidarr does not throw from' (#2) from fix/lidarr-album-endpoint into main

Reviewed-on: #2

README.md

@@ -42,6 +42,21 @@ It leans towards collapsing too much. A false match makes something look
 played; a missed match makes something look abandoned. Only one of those
 deletes music.
 
+### Indexing quirks
+
+Albums are fetched from the **unfiltered** `GET /api/v1/album`, not one call per
+artist. `?artistId=` is Lidarr's unguarded path: it dereferences the album's
+artist metadata with no null check and picks the monitored release with
+`SingleOrDefault`, so it returns a 500 for an album with broken metadata or two
+monitored releases. The unfiltered endpoint skips such albums instead, and costs
+N fewer requests.
+
+Tracks and files have no unfiltered endpoint — Lidarr rejects a call with no
+filter — so they stay per artist. If one artist cannot be served, that artist is
+skipped and the run continues, but the count is recorded and the coverage report
+says so loudly. A missing artist makes their played music look cold, so an
+incomplete index must never be culled against.
+
 ### Reading the coverage report
 
 Matched against unmatched is the wrong comparison — most unmatched listening is

music_curator.py

@@ -665,13 +665,36 @@ class Lidarr:
         try:
             body = self.transport(url, timeout=self.timeout, headers={"X-Api-Key": self.api_key})
         except urllib.error.HTTPError as error:
-            raise LidarrError(f"{path}: HTTP {error.code}") from error
+            detail = error_detail(error)
+            raise LidarrError(f"GET {url}: HTTP {error.code}{': ' + detail if detail else ''}")
         except (urllib.error.URLError, TimeoutError) as error:
-            raise LidarrError(f"{path}: {error}") from error
+            raise LidarrError(f"GET {url}: {error}") from error
         try:
             return json.loads(body)
         except json.JSONDecodeError as error:
-            raise LidarrError(f"{path}: malformed response") from error
+            raise LidarrError(f"GET {url}: malformed response") from error
+
+
+def error_detail(error, limit=300):
+    """Return whatever the server said about a failure, for the log.
+
+    A bare status code sends you looking in the wrong place; Lidarr puts the
+    actual exception in the body.
+    """
+    try:
+        body = error.read().decode("utf-8", "replace").strip()
+    except Exception:  # noqa: BLE001 - diagnostics must never raise
+        return ""
+    if not body:
+        return ""
+    try:
+        payload = json.loads(body)
+    except json.JSONDecodeError:
+        return body[:limit]
+    for key in ("message", "description", "error"):
+        if payload.get(key):
+            return str(payload[key])[:limit]
+    return body[:limit]
 
 
 def parse_added(value):
@@ -693,6 +716,15 @@ def index_library(client, store):
     """
     artists = client.get("artist")
     artist_rows, album_rows, track_rows = [], [], []
+    skipped = []
+
+    # Albums come back in one unfiltered call rather than one per artist. That
+    # endpoint is the only one Lidarr hydrates defensively -- it skips an album
+    # whose artist metadata is missing, where `?artistId=` dereferences it and
+    # returns a 500 -- and it costs N fewer requests into the bargain.
+    albums_by_artist = {}
+    for album in client.get("album"):
+        albums_by_artist.setdefault(album.get("artistId"), []).append(album)
 
     for artist in artists:
         artist_id = artist["id"]
@@ -708,7 +740,7 @@ def index_library(client, store):
             )
         )
 
-        for album in client.get("album", {"artistId": artist_id}):
+        for album in albums_by_artist.get(artist_id, []):
             album_rows.append(
                 (
                     album["id"],
@@ -720,11 +752,24 @@ def index_library(client, store):
                 )
             )
 
+        # Tracks and files have no unfiltered endpoint -- Lidarr rejects a call
+        # with no filter at all -- so these stay per artist. One artist it
+        # cannot serve must not cost the whole index, but it cannot pass
+        # silently either: their tracks end up absent, and every scrobble of
+        # theirs then reads as unmatched.
+        try:
             # Paths and the date a track landed live on the file, not the track.
             files = {
-            handle["id"]: handle for handle in client.get("trackfile", {"artistId": artist_id})
+                handle["id"]: handle
+                for handle in client.get("trackfile", {"artistId": artist_id})
             }
-        for track in client.get("track", {"artistId": artist_id}):
+            tracks = client.get("track", {"artistId": artist_id})
+        except LidarrError as error:
+            logger.warning("could not index %s: %s", name or artist_id, error)
+            skipped.append(name or str(artist_id))
+            continue
+
+        for track in tracks:
             handle = files.get(track.get("trackFileId") or 0) or {}
             title = track.get("title") or ""
             track_rows.append(
@@ -744,6 +789,7 @@ def index_library(client, store):
             )
 
     store.replace_library(artist_rows, album_rows, track_rows)
+    store.set_state("index_skipped", str(len(skipped)))
     logger.info(
         "library indexed: %d artists, %d albums, %d tracks (%d with files)",
         len(artist_rows),
@@ -751,6 +797,12 @@ def index_library(client, store):
         len(track_rows),
         sum(row[7] for row in track_rows),
     )
+    if skipped:
+        logger.warning(
+            "%d artists could not be indexed, so their tracks are missing: %s",
+            len(skipped),
+            ", ".join(sorted(skipped)[:10]),
+        )
     return len(artist_rows), len(track_rows)
 
 
@@ -817,6 +869,13 @@ def coverage_report(store):
         return
 
     logger.info("--- match coverage ---")
+    skipped = int(store.get_state("index_skipped") or 0)
+    if skipped:
+        logger.warning(
+            "the index is incomplete: %d artists are missing their tracks, so every"
+            " figure below is a floor. Do not cull against it.",
+            skipped,
+        )
     for method in ("mbid", "name", "none"):
         row = store.connection.execute(
             "SELECT COUNT(*) AS pairs, COALESCE(SUM(plays), 0) AS plays"

tests/conftest.py

@@ -1,6 +1,8 @@
+import io
 import json
 import os
 import sys
+import urllib.error
 import urllib.parse
 
 import pytest
@@ -128,7 +130,10 @@ class FakeLidarr:
     exercise the same stitching the real thing does.
     """
 
-    def __init__(self, artists=()):
+    def __init__(self, artists=(), fail=()):
+        # (path, artistId) pairs the fake refuses to serve, standing in for the
+        # 500s Lidarr returns on data it cannot hydrate.
+        self.fail = set(fail)
         self.artists, self.albums, self.tracks, self.files = [], [], [], []
         for artist_index, entry in enumerate(artists, start=1):
             artist_id = artist_index
@@ -190,10 +195,26 @@ class FakeLidarr:
         }
         self.calls.append((path, query))
 
+        artist_id = int(query.get("artistId", 0))
+        if (path, artist_id) in self.fail:
+            raise urllib.error.HTTPError(
+                url, 500, "Internal Server Error", {}, io.BytesIO(b'{"message": "boom"}')
+            )
+
         if path == "artist":
             return json.dumps(self.artists)
-        artist_id = int(query.get("artistId", 0))
         source = {"album": self.albums, "track": self.tracks, "trackfile": self.files}[path]
+        if path == "album" and not artist_id:
+            # Lidarr's unfiltered album endpoint returns the lot.
+            return json.dumps(source)
+        if not artist_id:
+            raise urllib.error.HTTPError(
+                url,
+                400,
+                "Bad Request",
+                {},
+                io.BytesIO(b'{"message": "artistId must be provided"}'),
+            )
         return json.dumps([row for row in source if row["artistId"] == artist_id])
 
 

tests/test_music_curator.py

@@ -500,3 +500,61 @@ def test_matching_is_redone_when_new_scrobbles_arrive(tmp_path):
     music_curator.run_once(client_for(api), store, "lyra", NOW, 0)
 
     assert verdict(store, "Yellowcard", "Transmission Home")[0] == "name"
+
+
+def test_albums_come_from_the_unfiltered_endpoint(tmp_path):
+    """`?artistId=` is Lidarr's unguarded path and 500s on data it cannot
+    hydrate; the unfiltered one skips such albums instead."""
+    api = FakeLidarr(LIBRARY)
+    store = store_at(tmp_path)
+
+    music_curator.index_library(music_curator.Lidarr("http://lidarr", "key", transport=api), store)
+
+    album_calls = [query for path, query in api.calls if path == "album"]
+    assert album_calls == [{}]
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_album") == 3
+
+
+def test_an_artist_lidarr_cannot_serve_does_not_kill_the_index(tmp_path):
+    # Artist 2 is AC/DC in LIBRARY; its track lookup fails.
+    api = FakeLidarr(LIBRARY, fail=[("track", 2)])
+    store = store_at(tmp_path)
+
+    music_curator.index_library(music_curator.Lidarr("http://lidarr", "key", transport=api), store)
+
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_artist") == 3
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_track WHERE artist_id = 2") == 0
+    # The other two artists are indexed in full.
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_track") == 3
+    assert store.get_state("index_skipped") == "1"
+
+
+def test_a_skipped_artist_is_recorded_so_the_report_can_disown_the_numbers(tmp_path):
+    """An incomplete index makes played music look cold. It has to be loud."""
+    api = FakeLidarr(LIBRARY, fail=[("trackfile", 1)])
+    store = store_at(tmp_path)
+
+    music_curator.index_library(music_curator.Lidarr("http://lidarr", "key", transport=api), store)
+    music_curator.match_library(store)
+
+    assert store.get_state("index_skipped") == "1"
+
+
+def test_a_clean_index_records_no_skips(tmp_path):
+    store = indexed(tmp_path, [])
+
+    assert store.get_state("index_skipped") == "0"
+
+
+def test_a_lidarr_error_carries_the_url_and_what_the_server_said():
+    """A bare status code sends you looking at the wrong thing entirely."""
+    api = FakeLidarr(LIBRARY, fail=[("album", 0)])
+    client = music_curator.Lidarr("http://lidarr:8686", "key", transport=api)
+
+    with pytest.raises(music_curator.LidarrError) as raised:
+        client.get("album")
+
+    message = str(raised.value)
+    assert "http://lidarr:8686/api/v1/album" in message
+    assert "HTTP 500" in message
+    assert "boom" in message