diff --git a/README.md b/README.md index 3c8766e..3717b4f 100644 --- a/README.md +++ b/README.md @@ -44,12 +44,28 @@ 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. +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 diff --git a/music_curator.py b/music_curator.py index ebeccae..03c6b55 100644 --- a/music_curator.py +++ b/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 - # 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) + albums_by_artist, album_failures = fetch_albums(client, artists) 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" diff --git a/tests/test_music_curator.py b/tests/test_music_curator.py index ab7b570..e793de6 100644 --- a/tests/test_music_curator.py +++ b/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 - hydrate; the unfiltered one skips such albums instead.""" + """One request, and the only path that skips albums it cannot hydrate.""" 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)])