From ca5bbfa9daa41f74854ad1d18217a828cd1cf685 Mon Sep 17 00:00:00 2001 From: Jonathan Date: Mon, 20 Jul 2026 16:45:37 +0200 Subject: [PATCH] fix(worker): retry transient MusicBrainz failures with backoff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The matching stage makes up to three musicbrainzngs calls per job, none of which retried. A dropped TLS connection (surfacing as NetworkError, "") or a 503 rate-limit (ResponseError) failed the entire pipeline for the job, which then re-queued and re-ran from scratch — ~36% of jobs churned this way over a 6h window. Wrap the three MB calls in _with_retry (4 attempts, 1s/2s/4s exponential backoff), catching only the two transient musicbrainzngs error types so real bugs still propagate. musicbrainzngs stays lazily imported to keep module import offline. Co-Authored-By: Claude Opus 4.8 (1M context) --- worker/lyra_worker/_musicbrainz.py | 32 +++++++++++-- worker/tests/test_musicbrainz.py | 75 +++++++++++++++++++++++++++++- 2 files changed, 102 insertions(+), 5 deletions(-) diff --git a/worker/lyra_worker/_musicbrainz.py b/worker/lyra_worker/_musicbrainz.py index f34180d..063e853 100644 --- a/worker/lyra_worker/_musicbrainz.py +++ b/worker/lyra_worker/_musicbrainz.py @@ -1,3 +1,4 @@ +import time from difflib import SequenceMatcher from lyra_worker.types import MBTarget @@ -5,6 +6,29 @@ from lyra_worker.types import MBTarget _MIN_TITLE_RATIO = 0.6 +def _with_retry(fn, *, attempts: int = 4, base_delay: float = 1.0, sleep=time.sleep): + """Call `fn`, retrying transient MusicBrainz failures with exponential backoff. + + A dropped TLS connection to the MusicBrainz webservice surfaces as + musicbrainzngs.NetworkError (``), + and MusicBrainz's own 503 rate-limit surfaces as ResponseError; both are + transient, so we retry rather than failing the whole pipeline for the job. + Any other exception (bug, non-transient) propagates immediately. + + musicbrainzngs is imported lazily so importing this module stays offline. + """ + import musicbrainzngs + + transient = (musicbrainzngs.NetworkError, musicbrainzngs.ResponseError) + for i in range(attempts): + try: + return fn() + except transient: + if i == attempts - 1: + raise + sleep(base_delay * (2**i)) + + def _ratio(a: str, b: str) -> float: return SequenceMatcher(None, a.strip().casefold(), b.strip().casefold()).ratio() @@ -68,7 +92,7 @@ class MusicBrainzResolver: musicbrainzngs.set_useragent(self._app, self._version, self._contact) - res = musicbrainzngs.search_release_groups(query=album, artist=artist, limit=5) + res = _with_retry(lambda: musicbrainzngs.search_release_groups(query=album, artist=artist, limit=5)) groups = res.get("release-group-list", []) rg = _best_release_group(artist, album, groups) if rg is None: @@ -93,11 +117,11 @@ class MusicBrainzResolver: titles: tuple[str, ...] = () rgid = rg.get("id") if rgid: - rgfull = musicbrainzngs.get_release_group_by_id(rgid, includes=["releases"]) + rgfull = _with_retry(lambda: musicbrainzngs.get_release_group_by_id(rgid, includes=["releases"])) releases = rgfull.get("release-group", {}).get("release-list", []) if releases: - rel = musicbrainzngs.get_release_by_id( - releases[0]["id"], includes=["recordings"] + rel = _with_retry( + lambda: musicbrainzngs.get_release_by_id(releases[0]["id"], includes=["recordings"]) ).get("release", {}) tracks = [] for medium in rel.get("medium-list", []): diff --git a/worker/tests/test_musicbrainz.py b/worker/tests/test_musicbrainz.py index 6d1821c..0dbdfbd 100644 --- a/worker/tests/test_musicbrainz.py +++ b/worker/tests/test_musicbrainz.py @@ -1,4 +1,7 @@ -from lyra_worker._musicbrainz import _best_release_group +import musicbrainzngs +import pytest + +from lyra_worker._musicbrainz import _best_release_group, _with_retry # Offline unit tests for the pure release-group selection logic. The live resolve() # path (network) is covered by test_musicbrainz_live.py. @@ -60,3 +63,73 @@ def test_prefers_credited_artist_over_same_titled_other_artist_album(): ] best = _best_release_group("Lake Street Dive", "Fun Machine", groups) assert best["artist-credit"][0]["artist"]["name"] == "Lake Street Dive" + + +# --- _with_retry: transient MusicBrainz failures (SSL EOF, 503 rate-limit) --- + + +def test_retry_returns_value_without_backoff_when_first_call_succeeds(): + sleeps: list[float] = [] + calls = {"n": 0} + + def fn(): + calls["n"] += 1 + return "ok" + + assert _with_retry(fn, sleep=sleeps.append) == "ok" + assert calls["n"] == 1 + assert sleeps == [] # no backoff when nothing failed + + +def test_retry_recovers_from_transient_network_error(): + # The real symptom: arrives + # as musicbrainzngs.NetworkError. A couple of retries clears it. + sleeps: list[float] = [] + calls = {"n": 0} + + def fn(): + calls["n"] += 1 + if calls["n"] < 3: + raise musicbrainzngs.NetworkError("SSL: UNEXPECTED_EOF_WHILE_READING") + return "recovered" + + assert _with_retry(fn, sleep=sleeps.append) == "recovered" + assert calls["n"] == 3 + assert sleeps == [1.0, 2.0] # exponential backoff before each retry + + +def test_retry_recovers_from_transient_response_error(): + # MusicBrainz's own 503 rate-limit surfaces as ResponseError. + calls = {"n": 0} + + def fn(): + calls["n"] += 1 + if calls["n"] < 2: + raise musicbrainzngs.ResponseError("503 Service Unavailable") + return "ok" + + assert _with_retry(fn, sleep=lambda _: None) == "ok" + assert calls["n"] == 2 + + +def test_retry_reraises_after_exhausting_attempts(): + sleeps: list[float] = [] + + def fn(): + raise musicbrainzngs.NetworkError("still down") + + with pytest.raises(musicbrainzngs.NetworkError): + _with_retry(fn, attempts=4, sleep=sleeps.append) + assert sleeps == [1.0, 2.0, 4.0] # 3 backoffs across 4 attempts + + +def test_retry_does_not_swallow_non_transient_errors(): + calls = {"n": 0} + + def fn(): + calls["n"] += 1 + raise ValueError("bug, not a network blip") + + with pytest.raises(ValueError): + _with_retry(fn, sleep=lambda _: None) + assert calls["n"] == 1 # non-transient errors propagate immediately, no retry