Merge pull request 'fix: survive an album with two monitored releases' (#3) from fix/lidarr-monitored-release-clash into main

Reviewed-on: #3

README.md

@@ -44,12 +44,28 @@ deletes music.
 
 ### Indexing quirks
 
-Albums are fetched from the **unfiltered** `GET /api/v1/album`, not one call per
+Albums are fetched from the **unfiltered** `GET /api/v1/album` first: one
-artist. `?artistId=` is Lidarr's unguarded path: it dereferences the album's
+request, and the only path that skips albums whose artist metadata is missing
-artist metadata with no null check and picks the monitored release with
+rather than dereferencing it.
-`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
+That is not enough on its own. Every album endpoint maps through a resource
-N fewer requests.
+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

music_curator.py

@@ -707,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).
 
@@ -718,13 +752,7 @@ def index_library(client, store):
     artist_rows, album_rows, track_rows = [], [], []
     skipped = []
 
-    # Albums come back in one unfiltered call rather than one per artist. That
+    albums_by_artist, album_failures = fetch_albums(client, artists)
-    # 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"]
@@ -790,6 +818,7 @@ 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),
@@ -803,6 +832,14 @@ def index_library(client, store):
             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)
 
 
@@ -876,6 +913,13 @@ def coverage_report(store):
             " 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"

tests/test_music_curator.py

@@ -503,8 +503,7 @@ def test_matching_is_redone_when_new_scrobbles_arrive(tmp_path):
 
 
 def test_albums_come_from_the_unfiltered_endpoint(tmp_path):
-    """`?artistId=` is Lidarr's unguarded path and 500s on data it cannot
+    """One request, and the only path that skips albums it cannot hydrate."""
-    hydrate; the unfiltered one skips such albums instead."""
     api = FakeLidarr(LIBRARY)
     store = store_at(tmp_path)
 
@@ -515,6 +514,33 @@ def test_albums_come_from_the_unfiltered_endpoint(tmp_path):
     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)])