From 7c8e986b679f6d92a7cb1b4c247c5feb715c9991 Mon Sep 17 00:00:00 2001 From: David Rodger Date: Sun, 13 Sep 2026 12:20:45 -0400 Subject: [PATCH] Retry MusicBrainz work search instead of reporting an outage as no results MusicBrainz sheds load with a 503 ("The MusicBrainz web server is currently busy") a few percent of the time. search_works_multi was the one fetch in the client with no retry: it called raise_for_status(), the HTTPError fell into the broad RequestException handler, and the caller got an empty list. The route then returned a healthy 200 with zero results, so the app told users the song did not exist in MusicBrainz whenever the service hiccuped. Backend: - Add MusicBrainzUnavailable, raised only after the retry budget is spent. - Give the work search three attempts with 1s/2s backoff, shorter than the importers' 2s/4s because a user is waiting on a search sheet. Retry 503, 429, timeouts and connection errors; do not retry other 4xx, which are our own bad query. - Return 503 from /musicbrainz/works/search when MusicBrainz is unreachable, keeping 200-with-empty-results to mean "answered, no matches". - Fix the apostrophe normalization next to it. It read replace("'", "'") with ASCII on both sides, an editor autocorrect having flattened the curly one, so it did nothing. Now maps the five variants using \u escapes, as normalize_title already does. Apps: - searchMusicBrainzWorks returns MusicBrainzSearchResult rather than collapsing every failure into an empty array. - Both search sheets gain a "Search Unavailable" state with Try Again, distinct from the empty state that links out to musicbrainz.org. Old app builds are unaffected: they already treated any non-200 as an empty result, and they pick up the server-side retry without changing. Tests: eight cases covering persistent 503, transient 503, timeout, genuine empty result, un-retried 400, the apostrophe substitution, and both route status codes. Co-Authored-By: Claude Opus 5 --- apps/Mac/Views/MusicBrainzSearchSheet.swift | 45 ++++- apps/Shared/Models/MusicBrainz.swift | 13 ++ apps/Shared/Services/MusicBrainzService.swift | 19 +- apps/iOS/Views/MusicBrainzSearchSheet.swift | 47 ++++- backend/integrations/musicbrainz/client.py | 138 +++++++++++---- backend/integrations/musicbrainz/utils.py | 3 +- backend/routes/musicbrainz.py | 15 +- backend/tests/test_musicbrainz_search.py | 162 ++++++++++++++++++ 8 files changed, 395 insertions(+), 47 deletions(-) create mode 100644 backend/tests/test_musicbrainz_search.py diff --git a/apps/Mac/Views/MusicBrainzSearchSheet.swift b/apps/Mac/Views/MusicBrainzSearchSheet.swift index 98e5952d..b4962e24 100644 --- a/apps/Mac/Views/MusicBrainzSearchSheet.swift +++ b/apps/Mac/Views/MusicBrainzSearchSheet.swift @@ -17,6 +17,8 @@ struct MusicBrainzSearchSheet: View { @State private var searchResults: [MusicBrainzWork] = [] @State private var isSearching = false + /// The search itself failed, as opposed to returning no matches. + @State private var searchFailed = false @State private var selectedWork: MusicBrainzWork? @State private var isSubmitting = false @State private var resultTitle: String? @@ -45,6 +47,8 @@ struct MusicBrainzSearchSheet: View { // Content if isSearching { loadingView + } else if searchFailed { + unavailableView } else if searchResults.isEmpty { emptyView } else { @@ -101,6 +105,38 @@ struct MusicBrainzSearchSheet: View { .frame(maxWidth: .infinity, maxHeight: .infinity) } + private var unavailableView: some View { + VStack(spacing: ApproachNoteTheme.spacingMD) { + Image(systemName: "exclamationmark.triangle") + .font(.system(size: 50)) + .foregroundColor(ApproachNoteTheme.textSecondary.opacity(0.5)) + + Text("Search Unavailable") + .font(ApproachNoteTheme.headline()) + .foregroundColor(ApproachNoteTheme.textPrimary) + + Text("We couldn't reach MusicBrainz just now. This is usually temporary.") + .font(ApproachNoteTheme.subheadline()) + .foregroundColor(ApproachNoteTheme.textSecondary) + .multilineTextAlignment(.center) + + Button { + Task { + await performSearch() + } + } label: { + HStack { + Image(systemName: "arrow.clockwise") + Text("Try Again") + } + } + .buttonStyle(.bordered) + .padding(.top, ApproachNoteTheme.spacingXS) + } + .padding() + .frame(maxWidth: .infinity, maxHeight: .infinity) + } + private var emptyView: some View { VStack(spacing: ApproachNoteTheme.spacingMD) { Image(systemName: "magnifyingglass") @@ -240,7 +276,14 @@ struct MusicBrainzSearchSheet: View { private func performSearch() async { isSearching = true - searchResults = await musicBrainzService.searchMusicBrainzWorks(query: searchQuery) + switch await musicBrainzService.searchMusicBrainzWorks(query: searchQuery) { + case .results(let works): + searchResults = works + searchFailed = false + case .unavailable: + searchResults = [] + searchFailed = true + } isSearching = false } diff --git a/apps/Shared/Models/MusicBrainz.swift b/apps/Shared/Models/MusicBrainz.swift index 14b0bf32..749c81e6 100644 --- a/apps/Shared/Models/MusicBrainz.swift +++ b/apps/Shared/Models/MusicBrainz.swift @@ -82,6 +82,19 @@ struct SongRequestErrorResponse: Codable { let error: String } +/// Outcome of a MusicBrainz work search, surfaced to the UI. +/// +/// The two cases must stay distinct: an empty `results` array means +/// MusicBrainz answered and had no matching work, while `unavailable` means +/// MusicBrainz never answered. Telling a user "no results" when the service +/// was down sends them off to add a song that is already there. +enum MusicBrainzSearchResult { + /// MusicBrainz answered. The array may be empty, meaning no matches. + case results([MusicBrainzWork]) + /// MusicBrainz could not be reached, or the API call failed. + case unavailable +} + /// Outcome of submitting a song request, surfaced to the UI. enum SongRequestResult { /// The request was recorded and is awaiting admin review. diff --git a/apps/Shared/Services/MusicBrainzService.swift b/apps/Shared/Services/MusicBrainzService.swift index 361219ba..911c3d17 100644 --- a/apps/Shared/Services/MusicBrainzService.swift +++ b/apps/Shared/Services/MusicBrainzService.swift @@ -7,8 +7,12 @@ import os @MainActor class MusicBrainzService: ObservableObject { - /// Search MusicBrainz for works (songs) by title - func searchMusicBrainzWorks(query: String) async -> [MusicBrainzWork] { + /// Search MusicBrainz for works (songs) by title. + /// + /// Returns `.unavailable` rather than an empty result set when the search + /// itself failed, so the UI can say "try again" instead of wrongly telling + /// the user MusicBrainz has no such song. + func searchMusicBrainzWorks(query: String) async -> MusicBrainzSearchResult { let startTime = Date() let encodedQuery = query.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) ?? query @@ -18,7 +22,8 @@ class MusicBrainzService: ObservableObject { let (data, response) = try await URLSession.shared.data(from: url) guard let httpResponse = response as? HTTPURLResponse else { - return [] + Log.network.error("Error searching MusicBrainz: non-HTTP response") + return .unavailable } APIClient.logRequest("GET /musicbrainz/works/search", startTime: startTime) @@ -28,14 +33,16 @@ class MusicBrainzService: ObservableObject { if APIClient.diagnosticsEnabled { Log.network.debug("Found \(searchResponse.results.count, privacy: .public) MusicBrainz works") } - return searchResponse.results + return .results(searchResponse.results) } else { + // 503 is the backend reporting that MusicBrainz is down; any + // other status is our own failure. Neither means "no matches". Log.network.error("Error searching MusicBrainz: HTTP \(httpResponse.statusCode, privacy: .public)") - return [] + return .unavailable } } catch { Log.network.error("Error searching MusicBrainz: \(error)") - return [] + return .unavailable } } diff --git a/apps/iOS/Views/MusicBrainzSearchSheet.swift b/apps/iOS/Views/MusicBrainzSearchSheet.swift index 27ced0ed..1bf153e9 100644 --- a/apps/iOS/Views/MusicBrainzSearchSheet.swift +++ b/apps/iOS/Views/MusicBrainzSearchSheet.swift @@ -17,6 +17,8 @@ struct MusicBrainzSearchSheet: View { @State private var searchResults: [MusicBrainzWork] = [] @State private var isSearching = false + /// The search itself failed, as opposed to returning no matches. + @State private var searchFailed = false @State private var selectedWork: MusicBrainzWork? @State private var isSubmitting = false @State private var resultTitle: String? @@ -28,6 +30,8 @@ struct MusicBrainzSearchSheet: View { VStack(spacing: 0) { if isSearching { loadingView + } else if searchFailed { + unavailableView } else if searchResults.isEmpty { emptyView } else { @@ -110,6 +114,40 @@ struct MusicBrainzSearchSheet: View { .frame(maxWidth: .infinity, maxHeight: .infinity) } + private var unavailableView: some View { + VStack(spacing: ApproachNoteTheme.spacingMD) { + Image(systemName: "exclamationmark.triangle") + .font(.system(size: 60)) + .foregroundColor(ApproachNoteTheme.textSecondary.opacity(0.5)) + + Text("Search Unavailable") + .font(ApproachNoteTheme.headline()) + .foregroundColor(ApproachNoteTheme.textPrimary) + + Text("We couldn't reach MusicBrainz just now. This is usually temporary.") + .font(ApproachNoteTheme.subheadline()) + .foregroundColor(ApproachNoteTheme.textSecondary) + .multilineTextAlignment(.center) + .padding(.horizontal) + + Button { + Task { + await performSearch() + } + } label: { + HStack { + Image(systemName: "arrow.clockwise") + Text("Try Again") + } + } + .buttonStyle(.bordered) + .tint(ApproachNoteTheme.brand) + .padding(.top, ApproachNoteTheme.spacingXS) + } + .padding() + .frame(maxWidth: .infinity, maxHeight: .infinity) + } + private var emptyView: some View { VStack(spacing: ApproachNoteTheme.spacingMD) { Image(systemName: "magnifyingglass") @@ -222,7 +260,14 @@ struct MusicBrainzSearchSheet: View { private func performSearch() async { isSearching = true - searchResults = await musicBrainzService.searchMusicBrainzWorks(query: searchQuery) + switch await musicBrainzService.searchMusicBrainzWorks(query: searchQuery) { + case .results(let works): + searchResults = works + searchFailed = false + case .unavailable: + searchResults = [] + searchFailed = true + } isSearching = false } diff --git a/backend/integrations/musicbrainz/client.py b/backend/integrations/musicbrainz/client.py index 5698e45d..7a6b5c47 100644 --- a/backend/integrations/musicbrainz/client.py +++ b/backend/integrations/musicbrainz/client.py @@ -38,6 +38,16 @@ logger = logging.getLogger(__name__) +class MusicBrainzUnavailable(Exception): + """MusicBrainz could not be reached, or kept returning a transient error. + + Raised only after the retry budget is exhausted. Callers must not treat + this as "no matches" — the distinction is the whole point: an empty + result set means MusicBrainz answered and had nothing, this means it + never answered. + """ + + class MusicBrainzSearcher: """Shared MusicBrainz search functionality with caching""" @@ -494,14 +504,25 @@ def search_works_multi(self, title, limit=5): limit: Maximum number of results to return (default 5) Returns: - List of dicts with keys: id, title, composers, score, type, musicbrainz_url + List of dicts with keys: id, title, composers, score, type, musicbrainz_url. + An empty list means MusicBrainz answered and had no matches. + + Raises: + MusicBrainzUnavailable: MusicBrainz never answered successfully. """ self.last_made_api_call = True - self.rate_limit() - # Normalize apostrophes - MusicBrainz typically uses curly apostrophe (') - # Convert straight apostrophe to curly for better matching - normalized_title = title.replace("'", "'") + # Normalize apostrophes to ASCII. MusicBrainz stores most titles with + # a curly apostrophe, but its search index matches either form, so + # this only keeps our query text predictable. + # + # \u escapes are used deliberately: this line previously read + # `title.replace("'", "'")` with an ASCII apostrophe on both sides, + # an editor autocorrect having flattened the curly one, which made it + # a silent no-op. Same hazard as documented in normalize_title(). + normalized_title = title + for variant in ['‘', '’', 'ʼ', '`', '´']: + normalized_title = normalized_title.replace(variant, "'") # Search with the title as a phrase query = f'work:"{normalized_title}"' @@ -509,36 +530,13 @@ def search_works_multi(self, title, limit=5): logger.debug(f"Searching MusicBrainz works (multi): {query}") try: - response = self.session.get( - 'https://musicbrainz.org/ws/2/work/', - params={ - 'query': query, - 'fmt': 'json', - 'limit': limit - }, - timeout=10 - ) - response.raise_for_status() - - data = response.json() + data = self._search_works_request(query, limit) works = data.get('works', []) # If no results with quoted search, try unquoted if not works: logger.debug("No results with quoted search, trying unquoted...") - self.rate_limit() - - response = self.session.get( - 'https://musicbrainz.org/ws/2/work/', - params={ - 'query': normalized_title, - 'fmt': 'json', - 'limit': limit - }, - timeout=10 - ) - response.raise_for_status() - data = response.json() + data = self._search_works_request(normalized_title, limit) works = data.get('works', []) if not works: @@ -575,16 +573,82 @@ def search_works_multi(self, title, limit=5): logger.debug(f"Found {len(results)} MusicBrainz works") return results - except requests.exceptions.Timeout: - logger.warning("MusicBrainz search timed out") - return [] - except requests.exceptions.RequestException as e: - logger.error(f"MusicBrainz search failed: {e}") - return [] + except MusicBrainzUnavailable: + # Never downgrade an outage to "no matches" — let the caller + # tell the user that search is unavailable. + raise except Exception as e: - logger.error(f"Error searching MusicBrainz: {e}") + # A malformed/unexpected payload from an otherwise healthy + # MusicBrainz. Nothing to show, but nothing to retry either. + logger.error(f"Error parsing MusicBrainz search results: {e}") return [] + def _search_works_request(self, query, limit): + """Run one work-search query, retrying transient MusicBrainz errors. + + MusicBrainz sheds load with a 503 ("The MusicBrainz web server is + currently busy") often enough that a single attempt fails a few + percent of the time. Matches the backoff used by get_work_recordings + and the other detail fetches. + + Raises: + MusicBrainzUnavailable: after the retry budget is exhausted. + """ + max_retries = 3 + last_reason = 'unknown' + attempts_used = 0 + + for attempt in range(max_retries): + attempts_used = attempt + 1 + if attempt > 0: + # 1s, 2s. Shorter than the importers' 2s/4s because this + # path is a user waiting on a search sheet, not a batch job. + backoff_time = 2 ** (attempt - 1) + logger.warning(f"BACKOFF: MusicBrainz work search retry " + f"{attempt + 1}/{max_retries}, waiting {backoff_time}s " + f"(query={query})") + time.sleep(backoff_time) + + self.rate_limit() + + try: + response = self.session.get( + 'https://musicbrainz.org/ws/2/work/', + params={ + 'query': query, + 'fmt': 'json', + 'limit': limit + }, + timeout=10 + ) + + if response.status_code == 200: + return response.json() + + if response.status_code in (503, 429): + last_reason = f'HTTP {response.status_code}' + logger.warning(f"MusicBrainz work search {last_reason} (transient)") + continue + + # 4xx other than 429 is our fault, not theirs — a retry + # would just repeat the same bad request. + last_reason = f'HTTP {response.status_code}' + logger.error(f"MusicBrainz work search failed: {last_reason}") + break + + except requests.exceptions.Timeout: + last_reason = 'timeout' + logger.warning("MusicBrainz work search timed out") + continue + except requests.exceptions.RequestException as e: + last_reason = str(e) + logger.warning(f"MusicBrainz work search connection error: {e}") + continue + + raise MusicBrainzUnavailable( + f"MusicBrainz work search failed after {attempts_used} attempt(s) ({last_reason})" + ) + def _escape_lucene_query(self, text): """ Escape special characters for Lucene query syntax diff --git a/backend/integrations/musicbrainz/utils.py b/backend/integrations/musicbrainz/utils.py index 611b3a03..52b2e330 100755 --- a/backend/integrations/musicbrainz/utils.py +++ b/backend/integrations/musicbrainz/utils.py @@ -9,7 +9,7 @@ working — new code should import from the real module. """ -from integrations.musicbrainz.client import MusicBrainzSearcher +from integrations.musicbrainz.client import MusicBrainzSearcher, MusicBrainzUnavailable from integrations.musicbrainz.song_updates import ( update_song_composed_year, update_song_composer, @@ -18,6 +18,7 @@ __all__ = [ 'MusicBrainzSearcher', + 'MusicBrainzUnavailable', 'update_song_composer', 'update_song_wikipedia_url', 'update_song_composed_year', diff --git a/backend/routes/musicbrainz.py b/backend/routes/musicbrainz.py index b6d54c4c..4addf924 100644 --- a/backend/routes/musicbrainz.py +++ b/backend/routes/musicbrainz.py @@ -8,7 +8,7 @@ import db_utils as db_tools from core import research_queue from core.song_research import create_song_and_queue_research -from integrations.musicbrainz.utils import MusicBrainzSearcher +from integrations.musicbrainz.utils import MusicBrainzSearcher, MusicBrainzUnavailable from middleware.auth_middleware import require_auth logger = logging.getLogger(__name__) @@ -43,6 +43,10 @@ def search_musicbrainz_works(): - score: Match score (0-100) - type: Work type (e.g., "Song") - musicbrainz_url: URL to MusicBrainz page + + 200 with an empty results array means MusicBrainz had no matches. + 503 means MusicBrainz itself was unreachable — clients must show + those two cases differently. """ query = request.args.get('q', '').strip() @@ -65,6 +69,15 @@ def search_musicbrainz_works(): 'results': results }), 200 + except MusicBrainzUnavailable as e: + logger.warning(f"MusicBrainz unavailable for query '{query}': {e}") + return jsonify({ + 'error': 'MusicBrainz is temporarily unavailable', + 'detail': str(e), + 'query': query, + 'results': [] + }), 503 + except Exception as e: logger.error(f"Error searching MusicBrainz: {e}", exc_info=True) return jsonify({ diff --git a/backend/tests/test_musicbrainz_search.py b/backend/tests/test_musicbrainz_search.py new file mode 100644 index 00000000..474bd964 --- /dev/null +++ b/backend/tests/test_musicbrainz_search.py @@ -0,0 +1,162 @@ +""" +Tests for /v1/musicbrainz/works/search. + +The case that matters here is the difference between "MusicBrainz answered +and had nothing" and "MusicBrainz never answered". MusicBrainz sheds load +with a 503 often enough that the old behaviour — swallow the error, return +an empty list — told users a standard didn't exist in MusicBrainz whenever +the service hiccuped. + +No database is involved; the search route only talks to MusicBrainz. +""" + +import pytest +import requests + +from integrations.musicbrainz.client import MusicBrainzSearcher, MusicBrainzUnavailable + + +class FakeResponse: + def __init__(self, status_code, payload=None): + self.status_code = status_code + self._payload = payload if payload is not None else {} + + def json(self): + return self._payload + + +@pytest.fixture +def searcher(monkeypatch): + """A searcher with rate-limit sleeps and retry backoff stubbed out.""" + s = MusicBrainzSearcher() + monkeypatch.setattr(s, "rate_limit", lambda: None) + monkeypatch.setattr("integrations.musicbrainz.client.time.sleep", lambda _: None) + return s + + +def _work_payload(title="Body and Soul"): + return { + "works": [ + { + "id": "98fb7d1f-12f5-3878-9fad-73266fedbec8", + "title": title, + "type": "Song", + "score": 100, + } + ] + } + + +def test_search_raises_when_musicbrainz_keeps_returning_503(searcher): + """A persistent 503 must not be reported to the caller as "no matches".""" + calls = [] + + def always_busy(*args, **kwargs): + calls.append(kwargs.get("params")) + return FakeResponse(503) + + searcher.session.get = always_busy + + with pytest.raises(MusicBrainzUnavailable): + searcher.search_works_multi("Body and Soul") + + assert len(calls) == 3, "should exhaust the three-attempt retry budget" + + +def test_search_retries_past_a_transient_503(searcher): + """One 503 followed by a good response yields results, not an empty list.""" + responses = [FakeResponse(503), FakeResponse(200, _work_payload())] + + def flaky(*args, **kwargs): + return responses.pop(0) + + searcher.session.get = flaky + + results = searcher.search_works_multi("Body and Soul") + + assert [r["title"] for r in results] == ["Body and Soul"] + assert responses == [], "both queued responses should have been consumed" + + +def test_search_retries_on_timeout(searcher): + """Timeouts are transient too, and get the same retry treatment.""" + responses = [requests.exceptions.Timeout(), FakeResponse(200, _work_payload())] + + def flaky(*args, **kwargs): + item = responses.pop(0) + if isinstance(item, Exception): + raise item + return item + + searcher.session.get = flaky + + results = searcher.search_works_multi("Body and Soul") + + assert len(results) == 1 + + +def test_empty_result_set_is_not_an_error(searcher): + """A genuine no-match answer still returns an empty list.""" + searcher.session.get = lambda *a, **k: FakeResponse(200, {"works": []}) + + assert searcher.search_works_multi("zzzz no such work") == [] + + +def test_client_error_is_not_retried(searcher): + """A 400 is our bad query, not their outage — retrying just repeats it.""" + calls = [] + + def bad_request(*args, **kwargs): + calls.append(1) + return FakeResponse(400) + + searcher.session.get = bad_request + + with pytest.raises(MusicBrainzUnavailable): + searcher.search_works_multi("Body and Soul") + + assert len(calls) == 1 + + +def test_apostrophe_variants_are_normalized(searcher): + """The curly-apostrophe normalization is a real substitution, not a no-op. + + This line was once flattened by an editor autocorrect into + ``replace("'", "'")``, silently doing nothing. + """ + sent = {} + + def capture(*args, **kwargs): + sent["query"] = kwargs["params"]["query"] + return FakeResponse(200, _work_payload("It’s a Blue World")) + + searcher.session.get = capture + + searcher.search_works_multi("It’s a Blue World") + + assert sent["query"] == 'work:"It\'s a Blue World"' + + +def test_route_reports_503_when_musicbrainz_is_down(client, monkeypatch): + """The endpoint surfaces an outage as 503, not as a successful empty search.""" + def boom(self, title, limit=5): + raise MusicBrainzUnavailable("MusicBrainz work search failed (HTTP 503)") + + monkeypatch.setattr(MusicBrainzSearcher, "search_works_multi", boom) + + response = client.get("/v1/musicbrainz/works/search?q=Body+and+Soul") + + assert response.status_code == 503 + assert response.get_json()["results"] == [] + + +def test_route_reports_200_when_musicbrainz_has_no_matches(client, monkeypatch): + """An honest empty answer stays a 200 so the UI can say "no results".""" + monkeypatch.setattr( + MusicBrainzSearcher, "search_works_multi", lambda self, title, limit=5: [] + ) + + response = client.get("/v1/musicbrainz/works/search?q=zzzz+no+such+work") + + assert response.status_code == 200 + assert response.get_json()["results"] == []