diff --git a/CHANGELOG.md b/CHANGELOG.md index ddc483c..1f51c74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,12 @@ alone. ### Fixed +- **One failed search request no longer strands the albums behind it.** + Searches go out in batches of a hundred, and a batch that failed raised + immediately, so on a 250-album chart a single timed-out request left the + last fifty albums unsearched with nothing said about it. Every batch is + now attempted, and the summary reports how many albums missed out and + why. - **`--search` did not start downloads for the first album of each new artist.** Adding an album was assumed to search it, via Lidarr's `addOptions.searchForNewAlbum`. That flag is read by diff --git a/chartarr/cli.py b/chartarr/cli.py index 14909a1..30029ac 100644 --- a/chartarr/cli.py +++ b/chartarr/cli.py @@ -461,13 +461,14 @@ def events(): line += f" ยท failed {accent(counts['failed'])}" print(line) if args.search and touched: - # the only thing that actually starts a download: one AlbumSearch + # the only thing that actually starts a download: an AlbumSearch # for everything this run added or turned on - try: - api.search_albums(touched) - print(dim(f"asked lidarr to search for {_n(len(touched), 'album')}")) - except lidarr.LidarrError as e: - print(dim(f"search request failed: {e}")) + queued, errors = api.search_albums(touched) + if queued: + print(dim(f"asked lidarr to search for {_n(queued, 'album')}")) + missed = len(touched) - queued + if missed: + print(dim(f"{_n(missed, 'album')} went unsearched: {errors[0]}")) elif touched and not args.dry_run: # without --search the albums sit there monitored and idle, which # looks like a finished job that downloaded nothing diff --git a/chartarr/lidarr.py b/chartarr/lidarr.py index b2f1a4b..84c460f 100644 --- a/chartarr/lidarr.py +++ b/chartarr/lidarr.py @@ -235,7 +235,21 @@ def add_album(self, rgid: str, quality_profile_id: int, time.sleep(0.2) # be gentle return "added", (created or {}).get("id") - def search_albums(self, album_ids: list[int]) -> None: - for i in range(0, len(album_ids), 100): - self._call("command", method="POST", - json={"name": "AlbumSearch", "albumIds": album_ids[i:i + 100]}) + def search_albums(self, album_ids: list[int]) -> tuple[int, list[str]]: + """queue an AlbumSearch for these rows, in batches of 100. + + returns (queued, errors). a batch that fails does not stop the + rest: on a long chart the last fifty albums shouldn't go unsearched + because one request in the middle timed out. + """ + ids = [i for i in album_ids if i is not None] + queued, errors = 0, [] + for i in range(0, len(ids), 100): + chunk = ids[i:i + 100] + try: + self._call("command", method="POST", + json={"name": "AlbumSearch", "albumIds": chunk}) + queued += len(chunk) + except LidarrError as e: + errors.append(str(e)) + return queued, errors diff --git a/tests/test_cli.py b/tests/test_cli.py index 4d6b7d5..c5f84ce 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -380,6 +380,7 @@ def add_album(self, rgid, qp, mp, rf, also_monitor=None): def search_albums(self, album_ids): self.searched.extend(album_ids) + return len(album_ids), [] def _push(monkeypatch, tmp_path, outcomes, **flags): @@ -446,6 +447,24 @@ def test_every_pushed_album_is_searched_exactly_once( assert len(api.searched) == len(set(api.searched)) +def test_a_partly_failed_search_says_how_many_were_missed( + monkeypatch, tmp_path, capsys): + api = _FakeApi({"rg-1": ("added", 1), "rg-2": ("added", 2)}) + api.search_albums = lambda ids: (1, ["HTTP 500: boom"]) + monkeypatch.setattr(cli.lidarr, "Lidarr", lambda url, key: api) + monkeypatch.setattr(cli, "_screen_ok", lambda: False) + items = [{"key": "k1", "row": {"artist": "a", "title": "t"}, "rgid": "rg-1", + "artist_mbid": "am"}, + {"key": "k2", "row": {"artist": "b", "title": "u"}, "rgid": "rg-2", + "artist_mbid": "am"}] + args = cli.build_parser().parse_args(["chart.csv", "--search"]) + cli.stage_push(items, "artist", "title", args, + {"lidarr_url": "http://l:8686", "api_key": "k"}) + out = capsys.readouterr().out + assert "search for 1 album" in out + assert "1 album went unsearched" in out and "boom" in out + + def test_without_search_the_summary_says_nothing_was_searched( monkeypatch, tmp_path, capsys): # monitored-but-idle looks like a finished job that downloaded nothing diff --git a/tests/test_lidarr.py b/tests/test_lidarr.py index 66fd9f2..13621b0 100644 --- a/tests/test_lidarr.py +++ b/tests/test_lidarr.py @@ -243,13 +243,40 @@ def test_find_album_raises_on_5xx(): def test_search_albums_chunks_by_100_and_loses_nothing(): responses.add(responses.POST, API + "/command", json={"id": 1}, status=201) ids = list(range(250)) - api().search_albums(ids) + assert api().search_albums(ids) == (250, []) bodies = [json.loads(c.request.body) for c in responses.calls] assert [b["name"] for b in bodies] == ["AlbumSearch"] * 3 assert [len(b["albumIds"]) for b in bodies] == [100, 100, 50] assert [i for b in bodies for i in b["albumIds"]] == ids +@responses.activate +def test_one_failed_batch_does_not_strand_the_rest(): + # a 250-album chart is three requests; the middle one failing used to + # raise and leave the last fifty albums unsearched and unmentioned + responses.add(responses.POST, API + "/command", json={"id": 1}, status=201) + responses.add(responses.POST, API + "/command", body="boom", status=500) + responses.add(responses.POST, API + "/command", json={"id": 3}, status=201) + queued, errors = api().search_albums(list(range(250))) + assert queued == 150 # the first and third batches + assert len(errors) == 1 + assert len(responses.calls) == 3 # the third was still attempted + + +@responses.activate +def test_every_batch_failing_is_reported_not_raised(): + responses.add(responses.POST, API + "/command", body="boom", status=500) + queued, errors = api().search_albums([1, 2, 3]) + assert queued == 0 + assert len(errors) == 1 + + +def test_search_albums_with_nothing_to_do_makes_no_requests(): + # add_album returns None for the id when lidarr doesn't send one back + assert api().search_albums([]) == (0, []) + assert api().search_albums([None, None]) == (0, []) + + @responses.activate def test_wrong_api_key_says_where_to_find_the_right_one(): responses.add(responses.GET, API + "/system/status",