diff --git a/core/metadata/artwork.py b/core/metadata/artwork.py index a3c38c98..489472a5 100644 --- a/core/metadata/artwork.py +++ b/core/metadata/artwork.py @@ -289,13 +289,15 @@ def _upgrade_art_url(art_url: str) -> str: elif "coverartarchive.org" in art_url: # MusicBrainz art arrives as Cover Art Archive thumbnails # (/front-250 — see musicbrainz_search._cover_art_url). Upgrade to the - # 1200px thumbnail: a huge jump from 240p yet still served by CAA's own - # CDN. Deliberately NOT the bare /front original — that redirects to - # archive.org, which is flaky (intermittent 500s/timeouts) and can be - # multi-MB, nasty to embed in every track. 1200 is the sweet spot of - # quality + reliability. `_fetch_art_bytes` falls back to the original - # sized URL if /front-1200 is ever refused. - return re.sub(r"/front-\d+", "/front-1200", art_url) + # bare /front ORIGINAL — native resolution, frequently 3000px+ (#806: + # the old /front-1200 cap left MusicBrainz as the one source still + # below native while iTunes already shipped 3000x3000 — and bare + # /front URLs from release-group lookups bypassed the cap anyway, + # so the policy was inconsistent in practice). The original redirects + # to archive.org, which can be flaky, so `_fetch_art_bytes` inserts a + # /front-1200 midpoint fallback before the original-size URL: + # flakiness degrades to the old 1200px behavior, never below it. + return re.sub(r"/front(-\d+)?$", "/front", art_url) return art_url @@ -312,22 +314,27 @@ def _fetch_art_bytes(art_url: str): if not art_url: return None, None upgraded = _upgrade_art_url(art_url) - try: - with urllib.request.urlopen(upgraded, timeout=10) as response: - return response.read(), (response.info().get_content_type() or "image/jpeg") - except Exception as fetch_err: - if upgraded != art_url: - logger.info( - "Upgraded art URL refused (%s); retrying original size", fetch_err - ) - try: - with urllib.request.urlopen(art_url, timeout=10) as response: - return response.read(), (response.info().get_content_type() or "image/jpeg") - except Exception as retry_err: - logger.error("Art fetch failed after fallback: %s", retry_err) - return None, None - logger.error("Art fetch failed: %s", fetch_err) - return None, None + attempts = [upgraded] + # CAA's bare /front original redirects to archive.org, which can be flaky. + # Midpoint fallback: the 1200px CDN thumbnail (the pre-#806 behavior), + # tried BEFORE the original sized URL so a flaky archive.org degrades to + # 1200px — never all the way down to the 250px thumbnail. + if "coverartarchive.org" in upgraded and upgraded.endswith("/front"): + attempts.append(upgraded + "-1200") + if art_url not in attempts: + attempts.append(art_url) + + last_err = None + for i, candidate in enumerate(attempts): + try: + with urllib.request.urlopen(candidate, timeout=10) as response: + return response.read(), (response.info().get_content_type() or "image/jpeg") + except Exception as fetch_err: + last_err = fetch_err + if i < len(attempts) - 1: + logger.info("Art URL refused (%s); falling back to next size", fetch_err) + logger.error("Art fetch failed after %d attempt(s): %s", len(attempts), last_err) + return None, None def _min_size_art_validator(min_px): diff --git a/tests/metadata/test_artwork_resolution.py b/tests/metadata/test_artwork_resolution.py index e07c56d0..d92526b6 100644 --- a/tests/metadata/test_artwork_resolution.py +++ b/tests/metadata/test_artwork_resolution.py @@ -45,26 +45,24 @@ class TestUpgradeArtUrl: url = 'https://cdn-images.dzcdn.net/images/cover/abc/1000x1000-000000-80-0-0.jpg' assert '1900x1900' in _upgrade_art_url(url) - def test_caa_thumbnail_upgraded_to_1200(self): - # MusicBrainz art arrives as the /front-250 thumbnail; upgrade to the - # 1200px CDN thumbnail (NOT the flaky bare /front original). + def test_caa_thumbnail_upgraded_to_native_original(self): + # #806: MusicBrainz art arrives as the /front-250 thumbnail; upgrade + # to the bare /front ORIGINAL (native res, frequently 3000px+). + # Flakiness of the original is handled by _fetch_art_bytes' 1200px + # midpoint fallback, not by capping the URL here. url = 'https://coverartarchive.org/release/abc-123/front-250' - assert _upgrade_art_url(url) == 'https://coverartarchive.org/release/abc-123/front-1200' + assert _upgrade_art_url(url) == 'https://coverartarchive.org/release/abc-123/front' - def test_caa_500_scope_and_idempotent(self): + def test_caa_all_sizes_and_scopes_upgrade_to_original(self): assert _upgrade_art_url('https://coverartarchive.org/release/x/front-500') \ - == 'https://coverartarchive.org/release/x/front-1200' - # release-group scope works the same. + == 'https://coverartarchive.org/release/x/front' + assert _upgrade_art_url('https://coverartarchive.org/release/x/front-1200') \ + == 'https://coverartarchive.org/release/x/front' + # release-group scope works the same (the URL shape from #806). assert _upgrade_art_url('https://coverartarchive.org/release-group/y/front-250') \ - == 'https://coverartarchive.org/release-group/y/front-1200' - # Idempotent — an already-1200 URL stays put. - assert _upgrade_art_url('https://coverartarchive.org/release-group/y/front-1200') \ - == 'https://coverartarchive.org/release-group/y/front-1200' + == 'https://coverartarchive.org/release-group/y/front' - def test_caa_bare_front_left_alone(self): - # The bare /front original is intentionally NOT what we want; the - # sized-thumbnail regex doesn't touch it (and it never reaches the - # helper in practice — _cover_art_url always emits /front-250). + def test_caa_bare_front_idempotent(self): url = 'https://coverartarchive.org/release/abc/front' assert _upgrade_art_url(url) == url @@ -163,3 +161,60 @@ class TestFetchArtBytes: def test_empty_url_returns_none(self): assert _fetch_art_bytes('') == (None, None) assert _fetch_art_bytes(None) == (None, None) + + # ── #806: CAA native-res chain — original → 1200px midpoint → thumbnail ── + + def test_caa_fetches_native_original_first(self, monkeypatch): + calls = [] + + def fake_urlopen(url, timeout=None): + calls.append(url) + return _FakeResponse(b'native-3000px') + + monkeypatch.setattr('core.metadata.artwork.urllib.request.urlopen', fake_urlopen) + + data, _ = _fetch_art_bytes('https://coverartarchive.org/release/r1/front-250') + + assert data == b'native-3000px' + assert calls == ['https://coverartarchive.org/release/r1/front'] + + def test_caa_flaky_original_degrades_to_1200_not_250(self, monkeypatch): + """archive.org flakiness lands on the old 1200px behavior — the + midpoint sits BEFORE the tiny original thumbnail in the chain.""" + calls = [] + + def fake_urlopen(url, timeout=None): + calls.append(url) + if url.endswith('/front'): + raise Exception('503 archive.org is having a day') + return _FakeResponse(b'cdn-1200px') + + monkeypatch.setattr('core.metadata.artwork.urllib.request.urlopen', fake_urlopen) + + data, _ = _fetch_art_bytes('https://coverartarchive.org/release/r1/front-250') + + assert data == b'cdn-1200px' + assert calls == [ + 'https://coverartarchive.org/release/r1/front', + 'https://coverartarchive.org/release/r1/front-1200', + ] + + def test_caa_full_chain_ends_at_the_original_thumbnail(self, monkeypatch): + calls = [] + + def fake_urlopen(url, timeout=None): + calls.append(url) + if url.endswith('/front') or url.endswith('-1200'): + raise Exception('refused') + return _FakeResponse(b'tiny-250px') + + monkeypatch.setattr('core.metadata.artwork.urllib.request.urlopen', fake_urlopen) + + data, _ = _fetch_art_bytes('https://coverartarchive.org/release/r1/front-250') + + assert data == b'tiny-250px' + assert calls == [ + 'https://coverartarchive.org/release/r1/front', + 'https://coverartarchive.org/release/r1/front-1200', + 'https://coverartarchive.org/release/r1/front-250', + ]