Fix: MusicMap 404s miscounted as errors in similar-artists worker
The worker's WARNING observability proved the '38 errors' were almost all MusicMap returning 404 (artist has no map page) — a genuine not-found, not a fetch failure. But iter_musicmap_similar_artist_events flattened every RequestException to status_code 502, and the worker maps 400/404 -> not_found / everything-else -> error, so these inflated the error count. Surface the real HTTP status from the exception's response (404 stays 404), falling back to 502 only when there's no response (timeout/connection drop, which is correctly still an error eligible for retry). Regression tests: 404 -> 404 (not_found), timeout -> 502 (error), 500 stays error, plus an end-to-end worker check that a 404 result marks 'not_found' and stores nothing.
This commit is contained in:
parent
5149aca358
commit
f883e99feb
2 changed files with 101 additions and 1 deletions
|
|
@ -269,10 +269,17 @@ def iter_musicmap_similar_artist_events(
|
|||
|
||||
except requests.exceptions.RequestException as exc:
|
||||
logger.debug("Error fetching MusicMap for %s: %s", artist_name, exc)
|
||||
# A 404 from MusicMap means the artist simply has no map page — that's a
|
||||
# not-found, not a fetch failure. Surface the real HTTP status (when the
|
||||
# exception carries a response) so callers classify it correctly instead
|
||||
# of flattening every network-layer error to a 502 "error". Timeouts /
|
||||
# connection errors carry no response → fall back to 502 (still an error).
|
||||
resp = getattr(exc, 'response', None)
|
||||
status_code = getattr(resp, 'status_code', None) or 502
|
||||
yield {
|
||||
'type': 'error',
|
||||
'error': f'Failed to fetch from MusicMap: {exc}',
|
||||
'status_code': 502,
|
||||
'status_code': status_code,
|
||||
}
|
||||
except ValueError as exc:
|
||||
status_code = 404 if 'Could not find artist map on MusicMap' in str(exc) else 400
|
||||
|
|
|
|||
93
tests/test_similar_artists_status_codes.py
Normal file
93
tests/test_similar_artists_status_codes.py
Normal file
|
|
@ -0,0 +1,93 @@
|
|||
"""Regression seam tests for MusicMap fetch status-code classification.
|
||||
|
||||
Root cause (found via the worker's WARNING observability): MusicMap returns
|
||||
HTTP 404 when an artist simply has no map page — a *not-found*, not a failure.
|
||||
`_fetch_musicmap_similar_artist_names` calls `response.raise_for_status()`,
|
||||
which raises `requests.exceptions.HTTPError` carrying the real 404. The error
|
||||
handler in `iter_musicmap_similar_artist_events` used to flatten EVERY network
|
||||
error to `status_code: 502`, so the worker (which maps 400/404 → not_found,
|
||||
everything else → error) miscounted these as errors.
|
||||
|
||||
These tests pin the fix: the real HTTP status is surfaced, so a 404 reads as
|
||||
404 (→ not_found downstream) while response-less failures (timeout, connection
|
||||
drop) still fall back to 502 (→ error, eligible for retry).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import requests
|
||||
|
||||
import core.metadata.similar_artists as sa
|
||||
|
||||
|
||||
def _force_reach_fetch(monkeypatch):
|
||||
"""Get past the source-chain / provider-availability guard so the test
|
||||
exercises the fetch error path, not the 'no providers' branch."""
|
||||
monkeypatch.setattr(sa, "_get_source_chain_for_lookup", lambda _opts: ["spotify"])
|
||||
monkeypatch.setattr(sa.metadata_registry, "get_client_for_source", lambda _src: object())
|
||||
|
||||
|
||||
def _http_error(status_code):
|
||||
"""A requests.HTTPError carrying a response with the given status — exactly
|
||||
what response.raise_for_status() raises on a 4xx/5xx."""
|
||||
resp = requests.Response()
|
||||
resp.status_code = status_code
|
||||
return requests.exceptions.HTTPError(f"{status_code} Client Error", response=resp)
|
||||
|
||||
|
||||
def test_musicmap_404_surfaced_as_404(monkeypatch):
|
||||
"""A 404 from MusicMap → status_code 404 (so the worker calls it not_found)."""
|
||||
_force_reach_fetch(monkeypatch)
|
||||
|
||||
def _raise_404(_name):
|
||||
raise _http_error(404)
|
||||
|
||||
monkeypatch.setattr(sa, "_fetch_musicmap_similar_artist_names", _raise_404)
|
||||
|
||||
result = sa.get_musicmap_similar_artists("Pharooo")
|
||||
assert result["success"] is False
|
||||
assert result["status_code"] == 404 # was wrongly 502 before the fix
|
||||
|
||||
|
||||
def test_musicmap_timeout_falls_back_to_502(monkeypatch):
|
||||
"""A response-less failure (timeout) has no status → 502 (stays an error)."""
|
||||
_force_reach_fetch(monkeypatch)
|
||||
|
||||
def _raise_timeout(_name):
|
||||
raise requests.exceptions.Timeout("timed out")
|
||||
|
||||
monkeypatch.setattr(sa, "_fetch_musicmap_similar_artist_names", _raise_timeout)
|
||||
|
||||
result = sa.get_musicmap_similar_artists("Some Artist")
|
||||
assert result["success"] is False
|
||||
assert result["status_code"] == 502
|
||||
|
||||
|
||||
def test_musicmap_500_stays_an_error(monkeypatch):
|
||||
"""A real upstream 5xx is surfaced as-is → not in (400,404) → error/retry."""
|
||||
_force_reach_fetch(monkeypatch)
|
||||
|
||||
def _raise_500(_name):
|
||||
raise _http_error(500)
|
||||
|
||||
monkeypatch.setattr(sa, "_fetch_musicmap_similar_artist_names", _raise_500)
|
||||
|
||||
result = sa.get_musicmap_similar_artists("Some Artist")
|
||||
assert result["success"] is False
|
||||
assert result["status_code"] == 500
|
||||
|
||||
|
||||
def test_worker_classifies_404_as_not_found():
|
||||
"""End-to-end seam: a 404 fetch result → the worker marks 'not_found',
|
||||
not 'error' (closing the loop the upstream fix enables)."""
|
||||
import core.similar_artists_worker as w
|
||||
|
||||
def fake_fetch(_name, limit=25):
|
||||
return {"success": False, "status_code": 404, "error": "no map page"}
|
||||
|
||||
def fake_store(**_kwargs):
|
||||
raise AssertionError("must not store anything on a not-found")
|
||||
|
||||
status, count, _detail = w.process_artist("sp1", "Pharooo", fake_fetch, fake_store)
|
||||
assert status == "not_found"
|
||||
assert count == 0
|
||||
Loading…
Reference in a new issue