chore(release): v0.2.2

Reviewed-on: #3

README.md

@@ -42,6 +42,37 @@ 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` first: one
+request, and the only path that skips albums whose artist metadata is missing
+rather than dereferencing it.
+
+That is not enough on its own. Every album endpoint maps through a resource
+that picks the release with `SingleOrDefault(x => x.Monitored)`, which throws
+for an album with **two monitored releases** and takes the whole response with
+it:
+
+```
+HTTP 500: Sequence contains more than one element
+```
+
+When the bulk call dies that way, the indexer falls back to one request per
+artist. It cannot avoid the exception, but it confines it to whichever artist
+owns the offending album and names them in the log — which is the only
+practical way to find it in a large library. Open that artist in Lidarr and
+check the Releases tab of each album: exactly one release may be monitored.
+
+Losing an artist's albums does not cost their tracks, which come from a
+different endpoint with a different mapper, so matching is unaffected. A cull
+would not be, and the report says so.
+
+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):
@@ -684,6 +707,40 @@ def parse_added(value):
         return None
 
 
+def fetch_albums(client, artists):
+    """Return albums grouped by artist id, plus the artists whose albums failed.
+
+    Every album endpoint maps through a resource that picks the release with
+    `SingleOrDefault(x => x.Monitored)`, which throws for an album with two
+    monitored releases -- "Sequence contains more than one element" -- and takes
+    the entire response down with it.
+
+    The unfiltered endpoint is one request and is the only one that also skips
+    albums whose artist metadata is missing, so it is tried first. When it dies,
+    asking per artist confines the loss to whichever artist owns the offending
+    album, and names them, which is the only way to find it.
+    """
+    try:
+        grouped = {}
+        for album in client.get("album"):
+            grouped.setdefault(album.get("artistId"), []).append(album)
+        return grouped, []
+    except LidarrError as error:
+        logger.warning("fetching all albums failed (%s)", error)
+        logger.warning("falling back to one request per artist to isolate the bad album")
+
+    grouped, failed = {}, []
+    for artist in artists:
+        artist_id = artist["id"]
+        try:
+            grouped[artist_id] = client.get("album", {"artistId": artist_id})
+        except LidarrError as error:
+            name = artist.get("artistName") or str(artist_id)
+            logger.warning("could not fetch albums for %s: %s", name, error)
+            failed.append(name)
+    return grouped, failed
+
+
 def index_library(client, store):
     """Rebuild the library index from Lidarr. Returns (artists, tracks).
 
@@ -693,6 +750,9 @@ def index_library(client, store):
     """
     artists = client.get("artist")
     artist_rows, album_rows, track_rows = [], [], []
+    skipped = []
+
+    albums_by_artist, album_failures = fetch_albums(client, artists)
 
     for artist in artists:
         artist_id = artist["id"]
@@ -708,7 +768,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 +780,24 @@ def index_library(client, store):
                 )
             )
 
-        # 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})
-        }
-        for track in client.get("track", {"artistId": artist_id}):
+        # 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})
+            }
+            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 +817,8 @@ def index_library(client, store):
             )
 
     store.replace_library(artist_rows, album_rows, track_rows)
+    store.set_state("index_skipped", str(len(skipped)))
+    store.set_state("index_albums_skipped", str(len(album_failures)))
     logger.info(
         "library indexed: %d artists, %d albums, %d tracks (%d with files)",
         len(artist_rows),
@@ -751,6 +826,20 @@ 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]),
+        )
+    if album_failures:
+        logger.warning(
+            "no albums indexed for %d artists: %s."
+            " In Lidarr, open each one and check the Releases tab of their albums:"
+            " exactly one release per album may be monitored.",
+            len(album_failures),
+            ", ".join(sorted(album_failures)[:10]),
+        )
     return len(artist_rows), len(track_rows)
 
 
@@ -817,6 +906,20 @@ 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,
+        )
+    albums_skipped = int(store.get_state("index_albums_skipped") or 0)
+    if albums_skipped:
+        logger.warning(
+            "%d artists have no albums indexed. Matching is unaffected -- it runs off"
+            " tracks -- but a cull cannot be run until this is fixed.",
+            albums_skipped,
+        )
     for method in ("mbid", "name", "none"):
         row = store.connection.execute(
             "SELECT COUNT(*) AS pairs, COALESCE(SUM(plays), 0) AS plays"

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "music-curator"
-version = "0.2.0"
+version = "0.2.2"
 description = "Ingest a Last.fm listening history and curate a music library from it"
 readme = "README.md"
 requires-python = ">=3.11"

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,87 @@ 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):
+    """One request, and the only path that skips albums it cannot hydrate."""
+    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_a_bad_album_falls_back_to_asking_per_artist(tmp_path):
+    """An album with two monitored releases throws in the resource mapper, so
+    the bulk call dies wholesale. Per artist, only its owner is lost."""
+    # The bulk call fails because artist 2 owns the offending album.
+    api = FakeLidarr(LIBRARY, fail=[("album", 0), ("album", 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_album WHERE artist_id = 2") == 0
+    # The other two artists keep their albums.
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_album") == 2
+    assert store.get_state("index_albums_skipped") == "1"
+
+
+def test_a_bad_album_does_not_cost_that_artist_their_tracks(tmp_path):
+    """Tracks come from a different endpoint with a different mapper, so
+    matching survives an album Lidarr cannot serialise."""
+    api = FakeLidarr(LIBRARY, fail=[("album", 0), ("album", 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_track WHERE artist_id = 2") == 1
+    assert store.get_state("index_skipped") == "0"
+
+
+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