diff --git a/README.md b/README.md index 10ba4e7..3c8766e 100644 --- a/README.md +++ b/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 diff --git a/music_curator.py b/music_curator.py index 2992d99..ebeccae 100644 --- a/music_curator.py +++ b/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): ) ) - # 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 +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" diff --git a/tests/conftest.py b/tests/conftest.py index e44e1cc..74d8e4f 100644 --- a/tests/conftest.py +++ b/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]) diff --git a/tests/test_music_curator.py b/tests/test_music_curator.py index 56e05e0..ab7b570 100644 --- a/tests/test_music_curator.py +++ b/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