diff --git a/apps/Mac/Views/MusicBrainzSearchSheet.swift b/apps/Mac/Views/MusicBrainzSearchSheet.swift index 98e5952..b4962e2 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 14b0bf3..749c81e 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 361219b..911c3d1 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 27ced0e..1bf153e 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 5698e45..7a6b5c4 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 611b3a0..52b2e33 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 b6d54c4..4addf92 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 0000000..474bd96 --- /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"] == []