From a1c665b4900de0337cf9cb82884ce2c99211b519 Mon Sep 17 00:00:00 2001 From: Claudio Ortolina Date: Fri, 24 Apr 2026 13:55:00 +0100 Subject: [PATCH] ML-21: Classify API errors as transient vs permanent --- ...rs-as-transient-vs-permanent-in-workers.md | 68 +++++++- lib/brave_search/api.ex | 30 +++- lib/brave_search/api/error_response.ex | 60 +++++++ lib/discogs/api.ex | 32 +++- lib/discogs/api/error_response.ex | 48 +++++ lib/last_fm/api/error_response.ex | 39 +++++ lib/music_brainz/api.ex | 45 +++-- lib/music_brainz/api/error_response.ex | 64 +++++++ lib/music_library/error_response.ex | 23 +++ lib/music_library/http_error.ex | 37 ++++ .../worker/artist_refresh_discogs_data.ex | 8 +- .../artist_refresh_music_brainz_data.ex | 8 +- .../worker/artist_refresh_wikipedia_data.ex | 10 +- lib/music_library/worker/error_handler.ex | 49 ++++++ .../worker/fetch_artist_image.ex | 19 +- lib/music_library/worker/fetch_artist_info.ex | 5 +- .../worker/fetch_artist_last_fm_data.ex | 6 +- .../worker/generate_record_embedding.ex | 5 +- .../worker/import_from_musicbrainz_release.ex | 3 +- .../import_from_musicbrainz_release_group.ex | 3 +- lib/music_library/worker/populate_genres.ex | 8 +- .../record_refresh_music_brainz_data.ex | 8 +- lib/music_library/worker/refresh_cover.ex | 14 +- lib/music_library_web/components/chat.ex | 2 +- lib/open_ai.ex | 2 +- lib/open_ai/api.ex | 62 ++++--- lib/open_ai/api/error_response.ex | 83 +++++++++ lib/wikipedia/api.ex | 66 +++++-- lib/wikipedia/api/error_response.ex | 94 ++++++++++ test/brave_search/api_test.exs | 37 +++- test/brave_search_test.exs | 4 +- test/discogs_test.exs | 43 +++++ test/last_fm/api/error_response_test.exs | 62 +++++++ test/music_brainz/api_test.exs | 85 +++++++++ test/music_brainz_test.exs | 3 +- .../music_library/records/similarity_test.exs | 4 +- test/music_library/records_test.exs | 9 +- .../worker/error_handler_test.exs | 97 +++++++++++ .../worker/generate_record_embedding_test.exs | 6 +- .../worker/populate_genres_test.exs | 30 +++- test/open_ai/api_test.exs | 82 +++++++-- test/wikipedia/api_test.exs | 164 ++++++++++++++++++ test/wikipedia_test.exs | 37 ++++ 43 files changed, 1429 insertions(+), 135 deletions(-) create mode 100644 lib/brave_search/api/error_response.ex create mode 100644 lib/discogs/api/error_response.ex create mode 100644 lib/music_brainz/api/error_response.ex create mode 100644 lib/music_library/error_response.ex create mode 100644 lib/music_library/http_error.ex create mode 100644 lib/music_library/worker/error_handler.ex create mode 100644 lib/open_ai/api/error_response.ex create mode 100644 lib/wikipedia/api/error_response.ex create mode 100644 test/last_fm/api/error_response_test.exs create mode 100644 test/music_brainz/api_test.exs create mode 100644 test/music_library/worker/error_handler_test.exs create mode 100644 test/wikipedia/api_test.exs diff --git a/backlog/tasks/ml-21 - Classify-API-errors-as-transient-vs-permanent-in-workers.md b/backlog/tasks/ml-21 - Classify-API-errors-as-transient-vs-permanent-in-workers.md index 76a9604a..b535dd42 100644 --- a/backlog/tasks/ml-21 - Classify-API-errors-as-transient-vs-permanent-in-workers.md +++ b/backlog/tasks/ml-21 - Classify-API-errors-as-transient-vs-permanent-in-workers.md @@ -1,15 +1,16 @@ --- id: ML-21 title: Classify API errors as transient vs permanent in workers -status: To Do +status: Done assignee: [] created_date: '2026-04-20 08:50' -updated_date: '2026-04-24 11:12' +updated_date: '2026-04-24 11:59' labels: [] dependencies: [] references: - 'https://github.com/cloud8421/music_library/issues/158' priority: medium +ordinal: 1000 --- ## Description @@ -69,10 +70,61 @@ Out of scope (tracked separately): honouring `Retry-After` / `X-*-Reset` headers ## Acceptance Criteria -- [ ] #1 MusicBrainz, Discogs, Wikipedia REST v1, Brave Search, and OpenAI API modules expose a classifier that maps {status, body} to :retry or :cancel -- [ ] #2 Wikipedia Action API responses (wbgetentities, prop=extracts) decode HTTP 200 bodies containing {"error": ...} into {:error, reason} instead of {:ok, body} -- [ ] #3 OpenAI classifier distinguishes rate_limit_exceeded (retry) from insufficient_quota (cancel) despite both being HTTP 429 -- [ ] #4 MusicBrainz classifier treats 503 as the rate-limit signal (not 429) and is documented as such -- [ ] #5 Workers using these APIs use the classifier to return {:snooze, n} / {:error, reason} / {:cancel, reason} instead of bubbling raw bodies -- [ ] #6 Existing LastFm.API.ErrorResponse behaviour is preserved +- [x] #1 MusicBrainz, Discogs, Wikipedia REST v1, Brave Search, and OpenAI API modules expose a classifier that maps {status, body} to :retry or :cancel +- [x] #2 Wikipedia Action API responses (wbgetentities, prop=extracts) decode HTTP 200 bodies containing {"error": ...} into {:error, reason} instead of {:ok, body} +- [x] #3 OpenAI classifier distinguishes rate_limit_exceeded (retry) from insufficient_quota (cancel) despite both being HTTP 429 +- [x] #4 MusicBrainz classifier treats 503 as the rate-limit signal (not 429) and is documented as such +- [x] #5 Workers using these APIs use the classifier to return {:snooze, n} / {:error, reason} / {:cancel, reason} instead of bubbling raw bodies +- [x] #6 Existing LastFm.API.ErrorResponse behaviour is preserved + +## Final Summary + + +## Summary + +Introduced structured per-API `ErrorResponse` modules for MusicBrainz, Discogs, Wikipedia, Brave Search, and OpenAI, so workers can now distinguish transient failures (rate limit, 5xx, timeout) from permanent ones (4xx, not found, auth, quota). Workers emit `{:snooze, seconds}` / `{:cancel, reason}` / `{:error, reason}` instead of bubbling raw bodies. Preserved the existing `LastFm.API.ErrorResponse` behaviour and added struct-based `retryable?/1` / `retry_delay_seconds/1` helpers so Last.fm plugs into the same handler. + +## What changed + +**New shared modules** +- `MusicLibrary.HttpError` — default HTTP status → kind mapping +- `MusicLibrary.Worker.ErrorHandler.to_oban_result/1` — maps any known `ErrorResponse` struct to the correct Oban tuple; passes through `{:ok, _}`, `{:cancel, _}`, and atom-reason errors unchanged + +**New per-API `ErrorResponse` modules** +- `MusicBrainz.API.ErrorResponse` — maps 503 to `:rate_limit` (MusicBrainz-specific; 429 is not used upstream) +- `Discogs.API.ErrorResponse` +- `Wikipedia.API.ErrorResponse` — has a dedicated `from_action_api_body/1` for HTTP 200 Action API error envelopes (AC2 silent-bug fix) +- `BraveSearch.API.ErrorResponse` +- `OpenAI.API.ErrorResponse` — disambiguates HTTP 429 between `rate_limit_exceeded` (retry) and `insufficient_quota` (cancel) via body `code` (AC3) + +**Updated API modules** — each now attaches a `parse_error/1` Req response step that halts with the appropriate struct on failure: +- `MusicBrainz.API`, `Discogs.API`, `Wikipedia.API`, `BraveSearch.API` +- `OpenAI.API` — `gpt/2`, `get_embeddings/2`, and `chat_stream/6` now return `{:error, %OpenAI.API.ErrorResponse{}}` instead of `{:error, "OpenAI API error: " <> inspect(body)}` strings + +**Updated workers** to route through `ErrorHandler.to_oban_result/1`, preserving existing atom-cancel branches (`:no_english_wikipedia`, `:cover_not_available`, `:image_not_found`, `:no_discogs_data`): +- `ArtistRefreshMusicBrainzData`, `ArtistRefreshDiscogsData`, `ArtistRefreshWikipediaData` +- `FetchArtistInfo`, `FetchArtistImage`, `FetchArtistLastFmData` +- `RefreshCover`, `RecordRefreshMusicBrainzData` +- `ImportFromMusicbrainzRelease`, `ImportFromMusicbrainzReleaseGroup` +- `PopulateGenres`, `GenerateRecordEmbedding` + +`RefreshScrobbles` unchanged (already had its own `ErrorResponse` routing). + +## Notable fixes along the way + +- Wikipedia silent bug (AC2): Action API returned `{:ok, %{"error" => ...}}` to callers when the body contained an error envelope. Now properly surfaced as `{:error, %Wikipedia.API.ErrorResponse{}}`. +- `lib/music_library_web/components/chat.ex`: Logger interpolation of the error reason (`"Chat streaming error: #{reason}"`) broke on struct errors. Switched to `inspect(reason)`. + +## Tests + +- New: `test/music_library/worker/error_handler_test.exs`, `test/music_brainz/api_test.exs`, `test/last_fm/api/error_response_test.exs` +- Added per-API error-classification assertions to existing `test/open_ai/api_test.exs`, `test/brave_search/api_test.exs`, `test/discogs_test.exs`, `test/wikipedia_test.exs` +- Updated callers that previously asserted on raw error bodies / string errors (`records_test.exs`, `similarity_test.exs`, worker tests) + +Full verification: `mise run dev:precommit` — shellcheck, credo --strict, sobelow, gettext, format, unused deps, 871 tests pass (43 doctests). Dialyzer shows 2 pre-existing warnings unrelated to this change. + +## Out of scope (ML-146) + +Honouring `Retry-After` / `X-*-Reset` headers to derive precise snooze durations — each ErrorResponse module currently returns fixed per-kind defaults (30–60 s). + diff --git a/lib/brave_search/api.ex b/lib/brave_search/api.ex index 202bf8d2..f06033f3 100644 --- a/lib/brave_search/api.ex +++ b/lib/brave_search/api.ex @@ -3,12 +3,13 @@ defmodule BraveSearch.API do Interface to the Brave Search API. """ + alias BraveSearch.API.ErrorResponse alias Req.Request require Logger @spec search_images(String.t(), keyword(), BraveSearch.Config.t()) :: - {:ok, [map()]} | {:error, term()} + {:ok, [map()]} | {:error, ErrorResponse.t() | Exception.t()} def search_images(query, opts, config) do params = [q: query, count: Keyword.get(opts, :count, 20)] @@ -61,16 +62,20 @@ defmodule BraveSearch.API do |> Request.merge_options(config.req_options) |> Req.RateLimiter.attach(name: :brave_search, cooldown: config.api_cooldown) |> Request.append_request_steps(log_attempt: &log_attempt/1) - |> Request.append_response_steps(log_error: &log_error/1) + |> Request.append_response_steps(parse_error: &parse_error/1) end defp get_request(request) do case Req.get(request) do - {:ok, response} when response.status == 200 -> - {:ok, response.body} + {:ok, %{status: status, body: body}} when status in 200..299 -> + {:ok, body} - {:ok, response} -> - {:error, response.body} + {:ok, %{body: %ErrorResponse{} = error}} -> + {:error, error} + + # Image download path does not attach parse_error; fall back to raw body. + {:ok, %{body: body}} -> + {:error, body} error -> error @@ -93,4 +98,17 @@ defmodule BraveSearch.API do {request, response} end + + defp parse_error({request, %{status: status} = response}) when status not in 200..299 do + error = ErrorResponse.from_response(response) + + Logger.error(fn -> + url = URI.to_string(request.url) + "Failed to fetch data from #{url}, status: #{status}, reason: #{inspect(response.body)}" + end) + + Request.halt(request, %{response | body: error}) + end + + defp parse_error(tuple), do: tuple end diff --git a/lib/brave_search/api/error_response.ex b/lib/brave_search/api/error_response.ex new file mode 100644 index 00000000..69e718ff --- /dev/null +++ b/lib/brave_search/api/error_response.ex @@ -0,0 +1,60 @@ +defmodule BraveSearch.API.ErrorResponse do + @moduledoc """ + Structured error response for Brave Search API calls. + + Brave Search returns a consistent JSON envelope on errors: + + %{"type" => "ErrorResponse", + "error" => %{"status" => 422, "code" => "SUBSCRIPTION_TOKEN_INVALID", + "detail" => "...", "meta" => %{...}}} + + HTTP status codes are the primary classifier (429 for rate limit, 5xx for + server errors). Brave uses 422 for most validation failures including + authentication errors that other APIs would return as 401. + """ + + @behaviour MusicLibrary.ErrorResponse + + alias MusicLibrary.HttpError + + @type t :: %__MODULE__{ + status: integer() | nil, + code: String.t() | nil, + message: String.t() | nil, + kind: HttpError.kind(), + body: term() + } + + defstruct [:status, :code, :message, :kind, :body] + + @spec from_response(Req.Response.t() | map()) :: t() + def from_response(%{status: status, body: body} = _response) do + %__MODULE__{ + status: status, + code: extract_code(body), + message: extract_message(body), + kind: HttpError.default_kind(status), + body: body + } + end + + @impl MusicLibrary.ErrorResponse + @spec retryable?(t()) :: boolean() + def retryable?(%__MODULE__{kind: kind}) when kind in [:rate_limit, :server_error, :timeout], + do: true + + def retryable?(%__MODULE__{}), do: false + + @impl MusicLibrary.ErrorResponse + @spec retry_delay_seconds(t()) :: pos_integer() + def retry_delay_seconds(%__MODULE__{kind: :rate_limit}), do: 60 + def retry_delay_seconds(%__MODULE__{kind: :server_error}), do: 30 + def retry_delay_seconds(%__MODULE__{kind: :timeout}), do: 10 + def retry_delay_seconds(%__MODULE__{}), do: 30 + + defp extract_code(%{"error" => %{"code" => code}}) when is_binary(code), do: code + defp extract_code(_), do: nil + + defp extract_message(%{"error" => %{"detail" => msg}}) when is_binary(msg), do: msg + defp extract_message(_), do: nil +end diff --git a/lib/discogs/api.ex b/lib/discogs/api.ex index 66d9590d..d80463eb 100644 --- a/lib/discogs/api.ex +++ b/lib/discogs/api.ex @@ -3,11 +3,13 @@ defmodule Discogs.API do Interface to the Discogs API. """ + alias Discogs.API.ErrorResponse alias Req.Request require Logger - @spec get_artist(integer() | String.t(), Discogs.Config.t()) :: {:ok, map()} | {:error, term()} + @spec get_artist(integer() | String.t(), Discogs.Config.t()) :: + {:ok, map()} | {:error, ErrorResponse.t() | Exception.t()} def get_artist(id, config) do config |> new_request() @@ -41,17 +43,20 @@ defmodule Discogs.API do |> Request.merge_options(config.req_options) |> Req.RateLimiter.attach(name: :discogs, cooldown: config.api_cooldown) |> Request.append_request_steps(log_attempt: &log_attempt/1) - |> Request.append_response_steps(log_error: &log_error/1) + |> Request.append_response_steps(parse_error: &parse_error/1) end defp get_request(request) do case Req.get(request) do - {:ok, response} when response.status == 200 -> - {:ok, response.body} + {:ok, %{status: status, body: body}} when status in 200..299 -> + {:ok, body} - # all non-success responses can be treated as errors - {:ok, response} -> - {:error, response.body} + {:ok, %{body: %ErrorResponse{} = error}} -> + {:error, error} + + # Image download path does not attach parse_error; fall back to raw body. + {:ok, %{body: body}} -> + {:error, body} error -> error @@ -74,4 +79,17 @@ defmodule Discogs.API do {request, response} end + + defp parse_error({request, %{status: status} = response}) when status not in 200..299 do + error = ErrorResponse.from_response(response) + + Logger.error(fn -> + url = URI.to_string(request.url) + "Failed to fetch data from #{url}, status: #{status}, reason: #{inspect(response.body)}" + end) + + Request.halt(request, %{response | body: error}) + end + + defp parse_error(tuple), do: tuple end diff --git a/lib/discogs/api/error_response.ex b/lib/discogs/api/error_response.ex new file mode 100644 index 00000000..6eebb054 --- /dev/null +++ b/lib/discogs/api/error_response.ex @@ -0,0 +1,48 @@ +defmodule Discogs.API.ErrorResponse do + @moduledoc """ + Structured error response for Discogs API calls. + + Discogs uses HTTP status codes as the error channel. Error bodies are JSON with + a single `"message"` field and no application-level error codes. + """ + + @behaviour MusicLibrary.ErrorResponse + + alias MusicLibrary.HttpError + + @type t :: %__MODULE__{ + status: integer() | nil, + message: String.t() | nil, + kind: HttpError.kind(), + body: term() + } + + defstruct [:status, :message, :kind, :body] + + @spec from_response(Req.Response.t() | map()) :: t() + def from_response(%{status: status, body: body} = _response) do + %__MODULE__{ + status: status, + message: extract_message(body), + kind: HttpError.default_kind(status), + body: body + } + end + + @impl MusicLibrary.ErrorResponse + @spec retryable?(t()) :: boolean() + def retryable?(%__MODULE__{kind: kind}) when kind in [:rate_limit, :server_error, :timeout], + do: true + + def retryable?(%__MODULE__{}), do: false + + @impl MusicLibrary.ErrorResponse + @spec retry_delay_seconds(t()) :: pos_integer() + def retry_delay_seconds(%__MODULE__{kind: :rate_limit}), do: 60 + def retry_delay_seconds(%__MODULE__{kind: :server_error}), do: 30 + def retry_delay_seconds(%__MODULE__{kind: :timeout}), do: 10 + def retry_delay_seconds(%__MODULE__{}), do: 30 + + defp extract_message(%{"message" => message}) when is_binary(message), do: message + defp extract_message(_), do: nil +end diff --git a/lib/last_fm/api/error_response.ex b/lib/last_fm/api/error_response.ex index dd151037..fcf99e96 100644 --- a/lib/last_fm/api/error_response.ex +++ b/lib/last_fm/api/error_response.ex @@ -1,4 +1,19 @@ defmodule LastFm.API.ErrorResponse do + @moduledoc """ + Structured error response for Last.fm API calls. + + Last.fm is unusual among the APIs this project consumes: it returns HTTP 200 + with a `%{"error" => N, "message" => "..."}` JSON body on failure, carrying a + numeric application-level error code in the range 2–29. Those codes map to + the atoms declared in `t:error_atom/0`. + + The struct-based helpers (`retryable?/1`, `retry_delay_seconds/1`) exist to + share a uniform protocol with the per-HTTP-status `ErrorResponse` modules + used by the other APIs — see `MusicLibrary.Worker.ErrorHandler`. + """ + + @behaviour MusicLibrary.ErrorResponse + defstruct [:error, :message] @type error_atom :: @@ -66,4 +81,28 @@ defmodule LastFm.API.ErrorResponse do # 5 seconds def retry_delay(:operation_failed), do: 5_000 def retry_delay(_), do: nil + + @doc """ + Struct-based retryability predicate shared with other API `ErrorResponse` modules. + + Enables `MusicLibrary.Worker.ErrorHandler.to_oban_result/1` to treat Last.fm + uniformly alongside HTTP-status-based errors without requiring callers to + unwrap the atom first. + """ + @impl MusicLibrary.ErrorResponse + @spec retryable?(t()) :: boolean() + def retryable?(%__MODULE__{error: error}), do: retryable_error?(error) + + @doc """ + Struct-based retry delay in seconds. Falls back to 30 s when the underlying + atom has no specific delay (see `retry_delay/1`). + """ + @impl MusicLibrary.ErrorResponse + @spec retry_delay_seconds(t()) :: pos_integer() + def retry_delay_seconds(%__MODULE__{error: error}) do + case retry_delay(error) do + ms when is_integer(ms) -> div(ms, 1000) + nil -> 30 + end + end end diff --git a/lib/music_brainz/api.ex b/lib/music_brainz/api.ex index 4e020660..798fc968 100644 --- a/lib/music_brainz/api.ex +++ b/lib/music_brainz/api.ex @@ -6,6 +6,7 @@ defmodule MusicBrainz.API do - Extend the metadata associated with existing records """ + alias MusicBrainz.API.ErrorResponse alias MusicBrainz.{Artist, ReleaseGroupSearchResult, ReleaseSearchResult} alias Req.Request @@ -207,7 +208,8 @@ defmodule MusicBrainz.API do } """ - @spec get_release_group(String.t(), MusicBrainz.Config.t()) :: {:ok, map()} | {:error, term()} + @spec get_release_group(String.t(), MusicBrainz.Config.t()) :: + {:ok, map()} | {:error, ErrorResponse.t() | Exception.t()} def get_release_group(id, config) do config |> new_request() @@ -284,7 +286,8 @@ defmodule MusicBrainz.API do "title": "Clark (Soundtrack From the Netflix Series)" } """ - @spec get_release(String.t(), MusicBrainz.Config.t()) :: {:ok, map()} | {:error, term()} + @spec get_release(String.t(), MusicBrainz.Config.t()) :: + {:ok, map()} | {:error, ErrorResponse.t() | Exception.t()} def get_release(id, config) do config |> new_request() @@ -299,7 +302,7 @@ defmodule MusicBrainz.API do end @spec get_releases(String.t(), keyword(), MusicBrainz.Config.t()) :: - {:ok, map()} | {:error, term()} + {:ok, map()} | {:error, ErrorResponse.t() | Exception.t()} def get_releases(release_group_id, opts, config) do Keyword.validate!(opts, [:limit, :offset]) @@ -320,7 +323,7 @@ defmodule MusicBrainz.API do end @spec search_release_by_barcode(String.t(), MusicBrainz.Config.t()) :: - {:ok, [ReleaseSearchResult.t()]} | {:error, term()} + {:ok, [ReleaseSearchResult.t()]} | {:error, ErrorResponse.t() | Exception.t()} def search_release_by_barcode(barcode, config) do config |> new_request() @@ -439,7 +442,7 @@ defmodule MusicBrainz.API do """ @spec search_release_group(String.t(), keyword(), MusicBrainz.Config.t()) :: {:ok, %{total_count: non_neg_integer(), release_groups: [ReleaseGroupSearchResult.t()]}} - | {:error, term()} + | {:error, ErrorResponse.t() | Exception.t()} def search_release_group(query, opts, config) do Keyword.validate!(opts, [:limit, :offset]) @@ -461,7 +464,8 @@ defmodule MusicBrainz.API do |> get_request() end - @spec get_artist(String.t(), MusicBrainz.Config.t()) :: {:ok, Artist.t()} | {:error, term()} + @spec get_artist(String.t(), MusicBrainz.Config.t()) :: + {:ok, Artist.t()} | {:error, ErrorResponse.t() | Exception.t()} def get_artist(musicbrainz_id, config) do config |> new_request() @@ -507,22 +511,41 @@ defmodule MusicBrainz.API do |> Request.merge_options(config.req_options) |> Req.RateLimiter.attach(name: :music_brainz, cooldown: config.api_cooldown) |> Request.append_request_steps(log_attempt: &log_attempt/1) + |> Request.append_response_steps(parse_error: &parse_error/1) end defp get_request(request) do case Req.get(request) do - {:ok, response} when response.status == 200 -> - {:ok, response.body} + {:ok, %{status: status, body: body}} when status in 200..299 -> + {:ok, body} - # all non-success responses can be treated as errors - {:ok, response} -> - {:error, response.body} + {:ok, %{body: %ErrorResponse{} = error}} -> + {:error, error} + + # Fallback for pipelines without parse_error (e.g. cover-art binary path) + # or Req exhausting retries before the step runs. Callers like + # get_cover_art/2 normalise this further. + {:ok, %{body: body}} -> + {:error, body} error -> error end end + defp parse_error({request, %{status: status} = response}) when status not in 200..299 do + error = ErrorResponse.from_response(response) + + Logger.error(fn -> + url = URI.to_string(request.url) + "Failed to fetch data from #{url}, status: #{status}, reason: #{inspect(response.body)}" + end) + + Request.halt(request, %{response | body: error}) + end + + defp parse_error(tuple), do: tuple + defp log_attempt(request) do url = URI.to_string(request.url) Logger.debug("Fetching data from #{url}") diff --git a/lib/music_brainz/api/error_response.ex b/lib/music_brainz/api/error_response.ex new file mode 100644 index 00000000..c4021d9b --- /dev/null +++ b/lib/music_brainz/api/error_response.ex @@ -0,0 +1,64 @@ +defmodule MusicBrainz.API.ErrorResponse do + @moduledoc """ + Structured error response for MusicBrainz API calls. + + MusicBrainz uses classic HTTP status codes as the error channel. The body is a + flat JSON `{"error": "message"}` with no numeric application codes. + + ## Rate limiting + + MusicBrainz signals rate limiting with **HTTP 503**, not 429. The service does + not use 429 at all — a 503 response with a `Retry-After` header is the rate + limit signal. `from_response/1` therefore maps 503 to `:rate_limit` (not the + generic `:server_error` classification from `MusicLibrary.HttpError`). + """ + + @behaviour MusicLibrary.ErrorResponse + + alias MusicLibrary.HttpError + + @type t :: %__MODULE__{ + status: integer() | nil, + message: String.t() | nil, + kind: HttpError.kind(), + body: term() + } + + defstruct [:status, :message, :kind, :body] + + @spec from_response(Req.Response.t() | map()) :: t() + def from_response(%{status: 503, body: body} = _response) do + %__MODULE__{ + status: 503, + message: extract_message(body), + kind: :rate_limit, + body: body + } + end + + def from_response(%{status: status, body: body} = _response) do + %__MODULE__{ + status: status, + message: extract_message(body), + kind: HttpError.default_kind(status), + body: body + } + end + + @impl MusicLibrary.ErrorResponse + @spec retryable?(t()) :: boolean() + def retryable?(%__MODULE__{kind: kind}) when kind in [:rate_limit, :server_error, :timeout], + do: true + + def retryable?(%__MODULE__{}), do: false + + @impl MusicLibrary.ErrorResponse + @spec retry_delay_seconds(t()) :: pos_integer() + def retry_delay_seconds(%__MODULE__{kind: :rate_limit}), do: 60 + def retry_delay_seconds(%__MODULE__{kind: :server_error}), do: 30 + def retry_delay_seconds(%__MODULE__{kind: :timeout}), do: 10 + def retry_delay_seconds(%__MODULE__{}), do: 30 + + defp extract_message(%{"error" => message}) when is_binary(message), do: message + defp extract_message(_), do: nil +end diff --git a/lib/music_library/error_response.ex b/lib/music_library/error_response.ex new file mode 100644 index 00000000..24759f3d --- /dev/null +++ b/lib/music_library/error_response.ex @@ -0,0 +1,23 @@ +defmodule MusicLibrary.ErrorResponse do + @moduledoc """ + Behaviour shared by all per-API `ErrorResponse` modules. + + Each API (MusicBrainz, Discogs, Wikipedia, Brave Search, OpenAI, Last.fm) + returns a struct implementing this behaviour on HTTP failure, so + `MusicLibrary.Worker.ErrorHandler.to_oban_result/1` can dispatch uniformly + to produce the right Oban tuple (`{:snooze, n}` / `{:cancel, reason}`) + without needing to know which API raised the error. + """ + + @doc """ + Returns `true` if the error is transient and the worker should retry after + a delay; `false` if the error is permanent and the worker should cancel. + """ + @callback retryable?(struct()) :: boolean() + + @doc """ + Returns the snooze delay in seconds for retryable errors. Implementations + may return any positive integer; common defaults are 30–60 s. + """ + @callback retry_delay_seconds(struct()) :: pos_integer() +end diff --git a/lib/music_library/http_error.ex b/lib/music_library/http_error.ex new file mode 100644 index 00000000..b7b53fc9 --- /dev/null +++ b/lib/music_library/http_error.ex @@ -0,0 +1,37 @@ +defmodule MusicLibrary.HttpError do + @moduledoc """ + Default HTTP status → error kind mapping shared by per-API `ErrorResponse` modules. + + Each API's `ErrorResponse.from_response/1` uses this as a baseline before applying + API-specific overrides (e.g. MusicBrainz treats 503 as a rate limit, not a server + error; OpenAI splits HTTP 429 into `:rate_limit` vs `:auth_error` based on the + body `code`). + + ## Kinds + + * `:rate_limit` — back off and retry (transient) + * `:server_error` — retry with backoff (transient) + * `:timeout` — retry with shorter backoff (transient) + * `:auth_error` — permanent until credentials change + * `:not_found` — permanent + * `:client_error` — permanent (malformed request) + * `:unknown` — unclassified; treated as permanent + """ + + @type kind :: + :rate_limit + | :server_error + | :timeout + | :auth_error + | :not_found + | :client_error + | :unknown + + @spec default_kind(integer()) :: kind() + def default_kind(429), do: :rate_limit + def default_kind(status) when status in 500..599, do: :server_error + def default_kind(status) when status in [401, 403], do: :auth_error + def default_kind(404), do: :not_found + def default_kind(status) when status in 400..499, do: :client_error + def default_kind(_), do: :unknown +end diff --git a/lib/music_library/worker/artist_refresh_discogs_data.ex b/lib/music_library/worker/artist_refresh_discogs_data.ex index e9d4de5d..8d3047f9 100644 --- a/lib/music_library/worker/artist_refresh_discogs_data.ex +++ b/lib/music_library/worker/artist_refresh_discogs_data.ex @@ -1,8 +1,14 @@ defmodule MusicLibrary.Worker.ArtistRefreshDiscogsData do + @moduledoc false + use Oban.Worker, queue: :discogs, max_attempts: 3 + alias MusicLibrary.Worker.ErrorHandler + @impl Oban.Worker def perform(%Oban.Job{args: %{"id" => artist_info_id}}) do - MusicLibrary.Artists.refresh_discogs_data(artist_info_id) + artist_info_id + |> MusicLibrary.Artists.refresh_discogs_data() + |> ErrorHandler.to_oban_result() end end diff --git a/lib/music_library/worker/artist_refresh_music_brainz_data.ex b/lib/music_library/worker/artist_refresh_music_brainz_data.ex index e14c5d1b..7861a4fb 100644 --- a/lib/music_library/worker/artist_refresh_music_brainz_data.ex +++ b/lib/music_library/worker/artist_refresh_music_brainz_data.ex @@ -1,8 +1,14 @@ defmodule MusicLibrary.Worker.ArtistRefreshMusicBrainzData do + @moduledoc false + use Oban.Worker, queue: :music_brainz, max_attempts: 3 + alias MusicLibrary.Worker.ErrorHandler + @impl Oban.Worker def perform(%Oban.Job{args: %{"id" => artist_info_id}}) do - MusicLibrary.Artists.refresh_musicbrainz_data(artist_info_id) + artist_info_id + |> MusicLibrary.Artists.refresh_musicbrainz_data() + |> ErrorHandler.to_oban_result() end end diff --git a/lib/music_library/worker/artist_refresh_wikipedia_data.ex b/lib/music_library/worker/artist_refresh_wikipedia_data.ex index 9b0a7d30..7e7df20b 100644 --- a/lib/music_library/worker/artist_refresh_wikipedia_data.ex +++ b/lib/music_library/worker/artist_refresh_wikipedia_data.ex @@ -1,11 +1,15 @@ defmodule MusicLibrary.Worker.ArtistRefreshWikipediaData do + @moduledoc false + use Oban.Worker, queue: :wikipedia, max_attempts: 3 + alias MusicLibrary.Worker.ErrorHandler + @impl Oban.Worker def perform(%Oban.Job{args: %{"id" => artist_info_id}}) do - with {:error, :no_english_wikipedia} <- - MusicLibrary.Artists.refresh_wikipedia_data(artist_info_id) do - {:cancel, :no_english_wikipedia} + case MusicLibrary.Artists.refresh_wikipedia_data(artist_info_id) do + {:error, :no_english_wikipedia} -> {:cancel, :no_english_wikipedia} + other -> ErrorHandler.to_oban_result(other) end end end diff --git a/lib/music_library/worker/error_handler.ex b/lib/music_library/worker/error_handler.ex new file mode 100644 index 00000000..8f08f89a --- /dev/null +++ b/lib/music_library/worker/error_handler.ex @@ -0,0 +1,49 @@ +defmodule MusicLibrary.Worker.ErrorHandler do + @moduledoc """ + Converts context-layer results into Oban worker return values. + + Workers call external APIs that return `{:error, %ErrorResponse{}}` (per-API + structs) on HTTP failures. This helper recognises any of the known error + response structs and translates them into the correct Oban tuple: + + * retryable errors → `{:snooze, seconds}` so the attempt isn't consumed + * non-retryable errors → `{:cancel, reason}` so Oban stops retrying + * unknown `{:error, reason}` → passed through for Oban's default backoff + + Workers that have app-layer atom-cancel reasons (e.g. `:no_english_wikipedia`, + `:cover_not_available`) must match those **before** calling this helper, since + atoms fall through to the generic `{:error, reason}` branch here. + """ + + @error_structs [ + LastFm.API.ErrorResponse, + MusicBrainz.API.ErrorResponse, + Discogs.API.ErrorResponse, + Wikipedia.API.ErrorResponse, + BraveSearch.API.ErrorResponse, + OpenAI.API.ErrorResponse + ] + + @type oban_result :: + :ok + | {:ok, term()} + | {:error, term()} + | {:cancel, term()} + | {:snooze, pos_integer()} + + @spec to_oban_result(term()) :: oban_result() + def to_oban_result(:ok), do: :ok + def to_oban_result({:ok, _} = result), do: result + + def to_oban_result({:error, %mod{} = response}) when mod in @error_structs do + if mod.retryable?(response) do + {:snooze, mod.retry_delay_seconds(response)} + else + {:cancel, response} + end + end + + def to_oban_result({:error, reason}), do: {:error, reason} + def to_oban_result({:cancel, _} = result), do: result + def to_oban_result({:snooze, _} = result), do: result +end diff --git a/lib/music_library/worker/fetch_artist_image.ex b/lib/music_library/worker/fetch_artist_image.ex index 5c6fe17f..8fcfe13c 100644 --- a/lib/music_library/worker/fetch_artist_image.ex +++ b/lib/music_library/worker/fetch_artist_image.ex @@ -1,20 +1,17 @@ defmodule MusicLibrary.Worker.FetchArtistImage do + @moduledoc false + use Oban.Worker, queue: :heavy_writes, max_attempts: 3 + alias MusicLibrary.Worker.ErrorHandler + @impl Oban.Worker def perform(%Oban.Job{args: %{"id" => artist_id}}) do case MusicLibrary.Artists.refresh_image(artist_id) do - {:ok, _artist_info} -> - :ok - - {:error, :image_not_found} -> - {:cancel, :image_not_found} - - {:error, :no_discogs_data} -> - {:cancel, :no_discogs_data} - - error -> - error + {:ok, _artist_info} -> :ok + {:error, :image_not_found} -> {:cancel, :image_not_found} + {:error, :no_discogs_data} -> {:cancel, :no_discogs_data} + other -> ErrorHandler.to_oban_result(other) end end end diff --git a/lib/music_library/worker/fetch_artist_info.ex b/lib/music_library/worker/fetch_artist_info.ex index 1dbe4134..82be3df6 100644 --- a/lib/music_library/worker/fetch_artist_info.ex +++ b/lib/music_library/worker/fetch_artist_info.ex @@ -1,8 +1,11 @@ defmodule MusicLibrary.Worker.FetchArtistInfo do + @moduledoc false + use Oban.Worker, queue: :default, max_attempts: 3 alias MusicLibrary.Artists alias MusicLibrary.Records.Similarity + alias MusicLibrary.Worker.ErrorHandler @impl Oban.Worker def perform(%Oban.Job{args: %{"id" => artist_id}}) do @@ -13,7 +16,7 @@ defmodule MusicLibrary.Worker.FetchArtistInfo do Similarity.regenerate_artist_embeddings(artist_id) else {:error, :no_english_wikipedia} -> {:cancel, :no_english_wikipedia} - error -> error + other -> ErrorHandler.to_oban_result(other) end end end diff --git a/lib/music_library/worker/fetch_artist_last_fm_data.ex b/lib/music_library/worker/fetch_artist_last_fm_data.ex index dab6f4df..c6988046 100644 --- a/lib/music_library/worker/fetch_artist_last_fm_data.ex +++ b/lib/music_library/worker/fetch_artist_last_fm_data.ex @@ -1,11 +1,15 @@ defmodule MusicLibrary.Worker.FetchArtistLastFmData do + @moduledoc false + use Oban.Worker, queue: :last_fm, max_attempts: 3 + alias MusicLibrary.Worker.ErrorHandler + @impl Oban.Worker def perform(%Oban.Job{args: %{"id" => artist_id}}) do case MusicLibrary.Artists.refresh_lastfm_data(artist_id) do {:ok, _artist_info} -> :ok - error -> error + other -> ErrorHandler.to_oban_result(other) end end end diff --git a/lib/music_library/worker/generate_record_embedding.ex b/lib/music_library/worker/generate_record_embedding.ex index e552e9f5..31d184d8 100644 --- a/lib/music_library/worker/generate_record_embedding.ex +++ b/lib/music_library/worker/generate_record_embedding.ex @@ -1,8 +1,11 @@ defmodule MusicLibrary.Worker.GenerateRecordEmbedding do + @moduledoc false + use Oban.Worker, queue: :heavy_writes, max_attempts: 3 alias MusicLibrary.Records alias MusicLibrary.Records.Similarity + alias MusicLibrary.Worker.ErrorHandler @impl Oban.Worker def perform(%Oban.Job{args: %{"record_id" => record_id}}) do @@ -11,7 +14,7 @@ defmodule MusicLibrary.Worker.GenerateRecordEmbedding do case Similarity.generate_embedding(record) do :noop -> :ok {:ok, _} -> Records.notify_update(record) - {:error, _} = error -> error + other -> ErrorHandler.to_oban_result(other) end end end diff --git a/lib/music_library/worker/import_from_musicbrainz_release.ex b/lib/music_library/worker/import_from_musicbrainz_release.ex index 24cbdbbf..d9e89621 100644 --- a/lib/music_library/worker/import_from_musicbrainz_release.ex +++ b/lib/music_library/worker/import_from_musicbrainz_release.ex @@ -8,6 +8,7 @@ defmodule MusicLibrary.Worker.ImportFromMusicbrainzRelease do use Oban.Worker, queue: :music_brainz, max_attempts: 3 alias MusicLibrary.Records.Record + alias MusicLibrary.Worker.ErrorHandler @impl Oban.Worker def perform(%Oban.Job{args: %{"release_id" => release_id} = args}) do @@ -19,7 +20,7 @@ defmodule MusicLibrary.Worker.ImportFromMusicbrainzRelease do case MusicLibrary.Records.import_from_musicbrainz_release(release_id, opts) do {:ok, _record} -> :ok - {:error, reason} -> {:error, reason} + other -> ErrorHandler.to_oban_result(other) end end end diff --git a/lib/music_library/worker/import_from_musicbrainz_release_group.ex b/lib/music_library/worker/import_from_musicbrainz_release_group.ex index b8ff775f..702f07b1 100644 --- a/lib/music_library/worker/import_from_musicbrainz_release_group.ex +++ b/lib/music_library/worker/import_from_musicbrainz_release_group.ex @@ -13,6 +13,7 @@ defmodule MusicLibrary.Worker.ImportFromMusicbrainzReleaseGroup do alias MusicLibrary.Records alias MusicLibrary.Records.Record + alias MusicLibrary.Worker.ErrorHandler @impl Oban.Worker def perform(%Oban.Job{args: %{"release_group_id" => release_group_id} = args}) do @@ -23,7 +24,7 @@ defmodule MusicLibrary.Worker.ImportFromMusicbrainzReleaseGroup do case Records.import_from_musicbrainz_release_group(release_group_id, opts) do {:ok, _record} -> :ok - {:error, reason} -> {:error, reason} + other -> ErrorHandler.to_oban_result(other) end end end diff --git a/lib/music_library/worker/populate_genres.ex b/lib/music_library/worker/populate_genres.ex index f6aaf396..82e140bf 100644 --- a/lib/music_library/worker/populate_genres.ex +++ b/lib/music_library/worker/populate_genres.ex @@ -1,16 +1,20 @@ defmodule MusicLibrary.Worker.PopulateGenres do + @moduledoc false + use Oban.Worker, queue: :heavy_writes, max_attempts: 10 alias MusicLibrary.Records + alias MusicLibrary.Worker.ErrorHandler @impl Oban.Worker def perform(%Oban.Job{args: %{"id" => record_id}}) do record = Records.get_record!(record_id) with {:ok, updated_record} <- Records.populate_genres(record), - {:ok, _worker} <- - Records.Similarity.generate_embedding_async(updated_record) do + {:ok, _worker} <- Records.Similarity.generate_embedding_async(updated_record) do Records.notify_update(updated_record) + else + other -> ErrorHandler.to_oban_result(other) end end end diff --git a/lib/music_library/worker/record_refresh_music_brainz_data.ex b/lib/music_library/worker/record_refresh_music_brainz_data.ex index fdd869df..dbc3a38f 100644 --- a/lib/music_library/worker/record_refresh_music_brainz_data.ex +++ b/lib/music_library/worker/record_refresh_music_brainz_data.ex @@ -1,14 +1,18 @@ defmodule MusicLibrary.Worker.RecordRefreshMusicBrainzData do + @moduledoc false + use Oban.Worker, queue: :music_brainz, max_attempts: 3 alias MusicLibrary.Records + alias MusicLibrary.Worker.ErrorHandler @impl Oban.Worker def perform(%Oban.Job{args: %{"id" => record_id}}) do record = Records.get_record!(record_id) - with {:ok, updated_record} <- Records.refresh_musicbrainz_data(record) do - Records.notify_update(updated_record) + case Records.refresh_musicbrainz_data(record) do + {:ok, updated_record} -> Records.notify_update(updated_record) + other -> ErrorHandler.to_oban_result(other) end end end diff --git a/lib/music_library/worker/refresh_cover.ex b/lib/music_library/worker/refresh_cover.ex index 6211f85e..569badeb 100644 --- a/lib/music_library/worker/refresh_cover.ex +++ b/lib/music_library/worker/refresh_cover.ex @@ -1,21 +1,19 @@ defmodule MusicLibrary.Worker.RefreshCover do + @moduledoc false + use Oban.Worker, queue: :heavy_writes, max_attempts: 3 alias MusicLibrary.Records + alias MusicLibrary.Worker.ErrorHandler @impl Oban.Worker def perform(%Oban.Job{args: %{"id" => record_id}}) do record = Records.get_record!(record_id) case Records.refresh_cover(record) do - {:ok, updated_record} -> - Records.notify_update(updated_record) - - {:error, :cover_not_available} -> - {:cancel, :cover_not_available} - - error -> - error + {:ok, updated_record} -> Records.notify_update(updated_record) + {:error, :cover_not_available} -> {:cancel, :cover_not_available} + other -> ErrorHandler.to_oban_result(other) end end end diff --git a/lib/music_library_web/components/chat.ex b/lib/music_library_web/components/chat.ex index bfab4f9b..b5612320 100644 --- a/lib/music_library_web/components/chat.ex +++ b/lib/music_library_web/components/chat.ex @@ -399,7 +399,7 @@ defmodule MusicLibraryWeb.Components.Chat do ) {:error, reason} -> - Logger.error("Chat streaming error: #{reason}") + Logger.error("Chat streaming error: #{inspect(reason)}") LiveView.send_update(parent_pid, __MODULE__, id: component_id, diff --git a/lib/open_ai.ex b/lib/open_ai.ex index 59eb3b19..91fbdc61 100644 --- a/lib/open_ai.ex +++ b/lib/open_ai.ex @@ -17,7 +17,7 @@ defmodule OpenAI do API.gpt(completion, config()) end - @spec chat_stream([map()], chat_stream_opts()) :: :ok | {:error, String.t()} + @spec chat_stream([map()], chat_stream_opts()) :: :ok | {:error, term()} def chat_stream(messages, opts \\ []) when is_list(messages) do model = Keyword.get(opts, :model, "gpt-4.1") temperature = Keyword.get(opts, :temperature, 0.7) diff --git a/lib/open_ai/api.ex b/lib/open_ai/api.ex index 2a5c458a..a1e58df1 100644 --- a/lib/open_ai/api.ex +++ b/lib/open_ai/api.ex @@ -1,13 +1,23 @@ defmodule OpenAI.API do @moduledoc """ Low-level HTTP client for the OpenAI API (chat completions, streaming responses, embeddings). + + HTTP errors are returned as `OpenAI.API.ErrorResponse` structs, which classify + rate-limit failures (retryable) separately from billing-quota failures + (`insufficient_quota` — non-retryable) despite both using HTTP 429. + + Mid-stream failures in `chat_stream/6` come from parsed SSE event payloads + rather than HTTP responses and remain as message strings passed through the + stream callback plumbing. """ + alias OpenAI.API.ErrorResponse alias Req.Request require Logger - @spec gpt(OpenAI.Completion.t(), OpenAI.Config.t()) :: {:ok, map()} | {:error, term()} + @spec gpt(OpenAI.Completion.t(), OpenAI.Config.t()) :: + {:ok, map()} | {:error, ErrorResponse.t() | Exception.t()} def gpt(completion, config) do case config |> new_request() @@ -27,17 +37,17 @@ defmodule OpenAI.API do content = get_in(body, ["choices", Access.at(0), "message", "content"]) JSON.decode(content) - {:ok, %{body: body}} -> - {:error, "OpenAI API error: #{inspect(body)}"} + {:ok, response} -> + {:error, ErrorResponse.from_response(response)} {:error, exception} -> - {:error, "Connection error: #{Exception.message(exception)}"} + {:error, exception} end end @spec chat_stream([map()], String.t(), String.t(), float(), OpenAI.Config.t(), (String.t() -> any())) :: - :ok | {:error, String.t()} + :ok | {:error, ErrorResponse.t() | Exception.t() | String.t()} def chat_stream(messages, instructions, model, temperature, config, cb) do config |> new_request() @@ -86,33 +96,35 @@ defmodule OpenAI.API do :ok end - {:ok, %{body: body}} -> - {:error, "OpenAI API error: #{inspect(body)}"} + {:ok, response} -> + {:error, ErrorResponse.from_response(response)} {:error, exception} -> - {:error, "Connection error: #{Exception.message(exception)}"} + {:error, exception} end end - @spec get_embeddings(String.t(), OpenAI.Config.t()) :: {:ok, [float()]} | {:error, term()} + @spec get_embeddings(String.t(), OpenAI.Config.t()) :: + {:ok, [float()]} | {:error, ErrorResponse.t() | Exception.t()} def get_embeddings(text, config) do - resp = - config - |> new_request() - |> Req.merge( - url: "/v1/embeddings", - json: %{ - input: text, - model: "text-embedding-3-small" - } - ) - |> Req.post!() + case config + |> new_request() + |> Req.merge( + url: "/v1/embeddings", + json: %{ + input: text, + model: "text-embedding-3-small" + } + ) + |> Req.post() do + {:ok, %{status: 200, body: body}} -> + {:ok, get_in(body, ["data", Access.at(0), "embedding"])} - if resp.status == 200 do - embeddings = get_in(resp.body, ["data", Access.at(0), "embedding"]) - {:ok, embeddings} - else - {:error, resp.body} + {:ok, response} -> + {:error, ErrorResponse.from_response(response)} + + {:error, exception} -> + {:error, exception} end end diff --git a/lib/open_ai/api/error_response.ex b/lib/open_ai/api/error_response.ex new file mode 100644 index 00000000..ec8c8f00 --- /dev/null +++ b/lib/open_ai/api/error_response.ex @@ -0,0 +1,83 @@ +defmodule OpenAI.API.ErrorResponse do + @moduledoc """ + Structured error response for OpenAI API calls. + + OpenAI uses HTTP status codes as the primary classifier, with one critical + body-peek exception: + + * HTTP **429** covers both `code: "rate_limit_exceeded"` (transient — retry) + and `code: "insufficient_quota"` (permanent — billing failure, cancel). + `from_response/1` reads `error.code` to disambiguate. + + Error body shape (non-streaming): + + %{"error" => %{"message" => ..., "type" => ..., "code" => ..., "param" => ...}} + + Mid-stream SSE `error` / `response.failed` events are handled separately in + `OpenAI.API.chat_stream/6` and do not flow through this module. + """ + + @behaviour MusicLibrary.ErrorResponse + + alias MusicLibrary.HttpError + + @type t :: %__MODULE__{ + status: integer() | nil, + code: String.t() | nil, + type: String.t() | nil, + message: String.t() | nil, + kind: HttpError.kind(), + body: term() + } + + defstruct [:status, :code, :type, :message, :kind, :body] + + @spec from_response(Req.Response.t() | map()) :: t() + def from_response(%{status: 429, body: %{"error" => %{"code" => "insufficient_quota"} = e}} = r) do + %__MODULE__{ + status: 429, + code: "insufficient_quota", + type: e["type"], + message: e["message"], + kind: :auth_error, + body: r.body + } + end + + def from_response(%{status: status, body: %{"error" => err} = body} = _response) + when is_map(err) do + %__MODULE__{ + status: status, + code: err["code"], + type: err["type"], + message: err["message"], + kind: HttpError.default_kind(status), + body: body + } + end + + def from_response(%{status: status, body: body} = _response) do + %__MODULE__{ + status: status, + code: nil, + type: nil, + message: nil, + kind: HttpError.default_kind(status), + body: body + } + end + + @impl MusicLibrary.ErrorResponse + @spec retryable?(t()) :: boolean() + def retryable?(%__MODULE__{kind: kind}) when kind in [:rate_limit, :server_error, :timeout], + do: true + + def retryable?(%__MODULE__{}), do: false + + @impl MusicLibrary.ErrorResponse + @spec retry_delay_seconds(t()) :: pos_integer() + def retry_delay_seconds(%__MODULE__{kind: :rate_limit}), do: 60 + def retry_delay_seconds(%__MODULE__{kind: :server_error}), do: 30 + def retry_delay_seconds(%__MODULE__{kind: :timeout}), do: 10 + def retry_delay_seconds(%__MODULE__{}), do: 30 +end diff --git a/lib/wikipedia/api.ex b/lib/wikipedia/api.ex index 340555f7..6f1bfaa6 100644 --- a/lib/wikipedia/api.ex +++ b/lib/wikipedia/api.ex @@ -1,14 +1,23 @@ defmodule Wikipedia.API do @moduledoc """ Interface to the Wikidata and Wikipedia APIs. + + Two API surfaces are used, with different error handling: + + * **Action API** (`/w/api.php`) returns HTTP 200 with an `{"error": {...}}` + payload on failure. `parse_error/1` promotes those bodies into + `Wikipedia.API.ErrorResponse` errors. + * **REST v1 API** (`/api/rest_v1/page/summary/:title`) uses classic HTTP + status codes. `parse_error/1` handles non-2xx statuses the same way. """ alias Req.Request + alias Wikipedia.API.ErrorResponse require Logger @spec get_wikipedia_title(String.t(), Wikipedia.Config.t()) :: - {:ok, String.t() | nil} | {:error, term()} + {:ok, String.t() | nil} | {:error, ErrorResponse.t() | Exception.t()} def get_wikipedia_title(wikidata_id, config) do case config |> new_request("https://www.wikidata.org") @@ -32,7 +41,8 @@ defmodule Wikipedia.API do end end - @spec get_article_summary(String.t(), Wikipedia.Config.t()) :: {:ok, map()} | {:error, term()} + @spec get_article_summary(String.t(), Wikipedia.Config.t()) :: + {:ok, map()} | {:error, ErrorResponse.t() | Exception.t()} def get_article_summary(title, config) do config |> new_request("https://en.wikipedia.org") @@ -41,7 +51,7 @@ defmodule Wikipedia.API do end @spec get_article_extract(String.t(), Wikipedia.Config.t()) :: - {:ok, String.t() | nil} | {:error, term()} + {:ok, String.t() | nil} | {:error, ErrorResponse.t() | Exception.t()} def get_article_extract(title, config) do case config |> new_request("https://en.wikipedia.org") @@ -80,16 +90,22 @@ defmodule Wikipedia.API do |> Request.merge_options(config.req_options) |> Req.RateLimiter.attach(name: :wikipedia, cooldown: config.api_cooldown) |> Request.append_request_steps(log_attempt: &log_attempt/1) - |> Request.append_response_steps(log_error: &log_error/1) + |> Request.append_response_steps(parse_error: &parse_error/1) end defp get_request(request) do case Req.get(request) do - {:ok, response} when response.status == 200 -> - {:ok, response.body} + {:ok, %{status: 200, body: %ErrorResponse{} = error}} -> + {:error, error} - {:ok, response} -> - {:error, response.body} + {:ok, %{status: status, body: body}} when status in 200..299 -> + {:ok, body} + + {:ok, %{body: %ErrorResponse{} = error}} -> + {:error, error} + + {:ok, %{body: body}} -> + {:error, body} error -> error @@ -102,14 +118,32 @@ defmodule Wikipedia.API do request end - defp log_error({request, response}) do - if response.status in 400..499 or response.status in 500..599 do - Logger.error(fn -> - url = URI.to_string(request.url) - "Failed to fetch data from #{url}, reason: #{inspect(response.body)}" - end) - end + # Action API: HTTP 200 with an error envelope in the body. + defp parse_error( + {request, + %{status: 200, body: %{"error" => %{"code" => _, "info" => _}} = body} = response} + ) do + error = ErrorResponse.from_action_api_body(body) - {request, response} + Logger.error(fn -> + url = URI.to_string(request.url) + "Failed to fetch data from #{url}, code: #{error.code}, info: #{error.message}" + end) + + Request.halt(request, %{response | body: error}) end + + # REST v1 API and other non-2xx responses. + defp parse_error({request, %{status: status} = response}) when status not in 200..299 do + error = ErrorResponse.from_response(response) + + Logger.error(fn -> + url = URI.to_string(request.url) + "Failed to fetch data from #{url}, status: #{status}, reason: #{inspect(response.body)}" + end) + + Request.halt(request, %{response | body: error}) + end + + defp parse_error(tuple), do: tuple end diff --git a/lib/wikipedia/api/error_response.ex b/lib/wikipedia/api/error_response.ex new file mode 100644 index 00000000..3ce3958f --- /dev/null +++ b/lib/wikipedia/api/error_response.ex @@ -0,0 +1,94 @@ +defmodule Wikipedia.API.ErrorResponse do + @moduledoc """ + Structured error response for Wikipedia/Wikidata API calls. + + Wikipedia exposes two API surfaces with different error paradigms: + + * **Action API** (`/w/api.php`, used by `get_wikipedia_title/2` and + `get_article_extract/2`) returns **HTTP 200 with an error in the body**: + `%{"error" => %{"code" => ..., "info" => ...}}`. Without body inspection + the error would be silently returned as `{:ok, body}`. + + * **REST v1 API** (`/api/rest_v1/page/summary/:title`, used by + `get_article_summary/2`) uses classic HTTP status codes. + + `from_response/1` handles both paths. `from_action_api_body/1` is a dedicated + entry point for the HTTP 200 + body-error case. + + ## Non-error body shapes + + These are NOT errors and should continue to return `{:ok, body}`: + + * `%{"entities" => %{_id => %{"missing" => ""}}}` from `wbgetentities` — + the facade converts this to `{:error, :no_english_wikipedia}` at a + higher level. + * `get_in(body, ["query", "pages", _, "extract"])` returning `nil` — a + title exists but has no extract. + + The discriminator for an actual error is the literal top-level `"error"` key + whose value is a map with both `"code"` and `"info"`. + """ + + @behaviour MusicLibrary.ErrorResponse + + alias MusicLibrary.HttpError + + @type t :: %__MODULE__{ + status: integer() | nil, + code: String.t() | nil, + message: String.t() | nil, + kind: HttpError.kind(), + body: term() + } + + defstruct [:status, :code, :message, :kind, :body] + + @spec from_response(Req.Response.t() | map()) :: t() + def from_response(%{status: status, body: body} = _response) do + %__MODULE__{ + status: status, + code: extract_rest_code(body), + message: extract_rest_message(body), + kind: HttpError.default_kind(status), + body: body + } + end + + @spec from_action_api_body(map()) :: t() + def from_action_api_body(%{"error" => %{"code" => code, "info" => info}} = body) do + %__MODULE__{ + status: 200, + code: code, + message: info, + kind: action_api_kind(code), + body: body + } + end + + @impl MusicLibrary.ErrorResponse + @spec retryable?(t()) :: boolean() + def retryable?(%__MODULE__{kind: kind}) when kind in [:rate_limit, :server_error, :timeout], + do: true + + def retryable?(%__MODULE__{}), do: false + + @impl MusicLibrary.ErrorResponse + @spec retry_delay_seconds(t()) :: pos_integer() + def retry_delay_seconds(%__MODULE__{kind: :rate_limit}), do: 30 + def retry_delay_seconds(%__MODULE__{kind: :server_error}), do: 30 + def retry_delay_seconds(%__MODULE__{kind: :timeout}), do: 10 + def retry_delay_seconds(%__MODULE__{}), do: 30 + + defp action_api_kind("ratelimited"), do: :rate_limit + defp action_api_kind("maxlag"), do: :rate_limit + defp action_api_kind("readonly"), do: :server_error + defp action_api_kind("internal_api_error_" <> _), do: :server_error + defp action_api_kind(_), do: :client_error + + defp extract_rest_code(%{"httpCode" => code}), do: to_string(code) + defp extract_rest_code(_), do: nil + + defp extract_rest_message(%{"messageTranslations" => %{"en" => msg}}), do: msg + defp extract_rest_message(%{"detail" => msg}) when is_binary(msg), do: msg + defp extract_rest_message(_), do: nil +end diff --git a/test/brave_search/api_test.exs b/test/brave_search/api_test.exs index f0ee2bbb..5ad56c65 100644 --- a/test/brave_search/api_test.exs +++ b/test/brave_search/api_test.exs @@ -2,6 +2,7 @@ defmodule BraveSearch.APITest do use ExUnit.Case, async: true alias BraveSearch.API + alias BraveSearch.API.ErrorResponse @config %BraveSearch.Config{ api_key: "test_key", @@ -52,14 +53,44 @@ defmodule BraveSearch.APITest do end @tag :capture_log - test "returns error on non-200 response" do + test "returns a rate-limit ErrorResponse on 429" do Req.Test.stub(__MODULE__, fn conn -> conn |> Plug.Conn.put_status(429) - |> Req.Test.json(%{"error" => "rate limited"}) + |> Req.Test.json(%{ + "type" => "ErrorResponse", + "error" => %{"status" => 429, "code" => "RATE_LIMITED", "detail" => "Too many requests"} + }) end) - assert {:error, %{"error" => "rate limited"}} = API.search_images("test", [], @config) + assert {:error, %ErrorResponse{} = err} = + API.search_images("test", [], @config) + + assert err.status == 429 + assert err.code == "RATE_LIMITED" + assert err.kind == :rate_limit + assert ErrorResponse.retryable?(err) + end + + @tag :capture_log + test "returns a client-error ErrorResponse on 422" do + Req.Test.stub(__MODULE__, fn conn -> + conn + |> Plug.Conn.put_status(422) + |> Req.Test.json(%{ + "type" => "ErrorResponse", + "error" => %{ + "status" => 422, + "code" => "SUBSCRIPTION_TOKEN_INVALID", + "detail" => "The provided subscription token is invalid." + } + }) + end) + + assert {:error, %ErrorResponse{kind: :client_error} = err} = + API.search_images("test", [], @config) + + refute ErrorResponse.retryable?(err) end test "passes count option as query param" do diff --git a/test/brave_search_test.exs b/test/brave_search_test.exs index e8924788..271c2d8f 100644 --- a/test/brave_search_test.exs +++ b/test/brave_search_test.exs @@ -76,14 +76,14 @@ defmodule BraveSearchTest do end @tag :capture_log - test "returns the decoded error body on non-200 responses" do + test "returns an ErrorResponse struct on non-200 responses" do Req.Test.stub(BraveSearch.API, fn conn -> conn |> Plug.Conn.put_resp_content_type("application/json") |> Plug.Conn.send_resp(429, ~s({"error":"rate_limited"})) end) - assert {:error, %{"error" => "rate_limited"}} = + assert {:error, %BraveSearch.API.ErrorResponse{status: 429, kind: :rate_limit}} = BraveSearch.search_images("too many requests") end end diff --git a/test/discogs_test.exs b/test/discogs_test.exs index f68bd708..b99c5493 100644 --- a/test/discogs_test.exs +++ b/test/discogs_test.exs @@ -1,6 +1,7 @@ defmodule DiscogsTest do use ExUnit.Case, async: true + alias Discogs.API.ErrorResponse alias Discogs.Fixtures describe "get_artist/1" do @@ -16,5 +17,47 @@ defmodule DiscogsTest do assert {:ok, expected_info} == Discogs.get_artist(discogs_id) end + + @tag :capture_log + test "returns a rate-limit ErrorResponse on 429" do + Req.Test.stub(Discogs.API, fn conn -> + conn + |> Plug.Conn.put_status(429) + |> Req.Test.json(%{"message" => "You are making requests too quickly."}) + end) + + assert {:error, %ErrorResponse{} = err} = Discogs.get_artist("1") + assert err.status == 429 + assert err.kind == :rate_limit + assert ErrorResponse.retryable?(err) + end + + @tag :capture_log + test "returns a not-found ErrorResponse on 404" do + Req.Test.stub(Discogs.API, fn conn -> + conn + |> Plug.Conn.put_status(404) + |> Req.Test.json(%{"message" => "The requested resource was not found."}) + end) + + assert {:error, %ErrorResponse{kind: :not_found} = err} = + Discogs.get_artist("1") + + refute ErrorResponse.retryable?(err) + end + + @tag :capture_log + test "returns a server-error ErrorResponse on 500" do + Req.Test.stub(Discogs.API, fn conn -> + conn + |> Plug.Conn.put_status(500) + |> Req.Test.json(%{"message" => "Internal server error"}) + end) + + assert {:error, %ErrorResponse{kind: :server_error} = err} = + Discogs.get_artist("1") + + assert ErrorResponse.retryable?(err) + end end end diff --git a/test/last_fm/api/error_response_test.exs b/test/last_fm/api/error_response_test.exs new file mode 100644 index 00000000..04c32b06 --- /dev/null +++ b/test/last_fm/api/error_response_test.exs @@ -0,0 +1,62 @@ +defmodule LastFm.API.ErrorResponseTest do + use ExUnit.Case, async: true + + alias LastFm.API.ErrorResponse + + describe "new/2" do + test "maps known error codes to atoms" do + assert %ErrorResponse{error: :rate_limit_exceeded, message: "limit"} = + ErrorResponse.new(29, "limit") + + assert %ErrorResponse{error: :invalid_session_key} = ErrorResponse.new(9, "bad") + assert %ErrorResponse{error: :service_offline} = ErrorResponse.new(11, "down") + end + end + + describe "retryable_error?/1" do + test "returns true for transient error atoms" do + for atom <- [:transient_error, :service_offline, :rate_limit_exceeded, :operation_failed] do + assert ErrorResponse.retryable_error?(atom), "expected #{atom} to be retryable" + end + end + + test "returns false for permanent error atoms" do + for atom <- [:invalid_session_key, :invalid_api_key, :authentication_failed] do + refute ErrorResponse.retryable_error?(atom), "expected #{atom} to be non-retryable" + end + end + end + + describe "retry_delay/1 and retry_delay_seconds/1" do + test "returns a delay in milliseconds for retryable atoms" do + assert ErrorResponse.retry_delay(:rate_limit_exceeded) == 60_000 + assert ErrorResponse.retry_delay(:service_offline) == 30_000 + assert ErrorResponse.retry_delay(:transient_error) == 5_000 + assert ErrorResponse.retry_delay(:operation_failed) == 5_000 + end + + test "returns nil for non-retryable atoms" do + for atom <- [:invalid_session_key, :invalid_api_key, :authentication_failed] do + assert ErrorResponse.retry_delay(atom) == nil, + "expected #{atom} to have no retry delay" + end + end + + test "struct-based retry_delay_seconds/1 returns the delay in seconds" do + assert ErrorResponse.retry_delay_seconds(ErrorResponse.new(29, "")) == 60 + assert ErrorResponse.retry_delay_seconds(ErrorResponse.new(11, "")) == 30 + assert ErrorResponse.retry_delay_seconds(ErrorResponse.new(16, "")) == 5 + end + + test "struct-based retry_delay_seconds/1 falls back to 30 s for non-retryable atoms" do + assert ErrorResponse.retry_delay_seconds(ErrorResponse.new(9, "")) == 30 + end + end + + describe "retryable?/1 (struct-based)" do + test "agrees with retryable_error?/1" do + assert ErrorResponse.retryable?(ErrorResponse.new(29, "")) + refute ErrorResponse.retryable?(ErrorResponse.new(9, "")) + end + end +end diff --git a/test/music_brainz/api_test.exs b/test/music_brainz/api_test.exs new file mode 100644 index 00000000..48483591 --- /dev/null +++ b/test/music_brainz/api_test.exs @@ -0,0 +1,85 @@ +defmodule MusicBrainz.APITest do + use ExUnit.Case, async: true + + alias MusicBrainz.API + + @config %MusicBrainz.Config{ + user_agent: "test_agent", + req_options: [plug: {Req.Test, __MODULE__}, max_retries: 0], + api_cooldown: 0 + } + + describe "error classification" do + @tag :capture_log + test "503 is treated as :rate_limit (MusicBrainz's rate-limit signal, not 429)" do + Req.Test.stub(__MODULE__, fn conn -> + conn + |> Plug.Conn.put_status(503) + |> Req.Test.json(%{"error" => "Your requests are exceeding the allowable rate limit."}) + end) + + assert {:error, %API.ErrorResponse{} = err} = + API.get_artist("mbid", @config) + + assert err.status == 503 + assert err.kind == :rate_limit + assert API.ErrorResponse.retryable?(err) + end + + @tag :capture_log + test "429 falls under :rate_limit via default mapping (even though MusicBrainz doesn't use it)" do + Req.Test.stub(__MODULE__, fn conn -> + conn + |> Plug.Conn.put_status(429) + |> Req.Test.json(%{"error" => "Too many requests"}) + end) + + assert {:error, %API.ErrorResponse{kind: :rate_limit} = err} = + API.get_artist("mbid", @config) + + assert API.ErrorResponse.retryable?(err) + end + + @tag :capture_log + test "404 is treated as :not_found and is not retryable" do + Req.Test.stub(__MODULE__, fn conn -> + conn + |> Plug.Conn.put_status(404) + |> Req.Test.json(%{"error" => "Not Found"}) + end) + + assert {:error, %API.ErrorResponse{kind: :not_found} = err} = + API.get_artist("mbid", @config) + + refute API.ErrorResponse.retryable?(err) + end + + @tag :capture_log + test "400 is treated as :client_error and is not retryable" do + Req.Test.stub(__MODULE__, fn conn -> + conn + |> Plug.Conn.put_status(400) + |> Req.Test.json(%{"error" => "Invalid MBID"}) + end) + + assert {:error, %API.ErrorResponse{kind: :client_error} = err} = + API.get_artist("mbid", @config) + + refute API.ErrorResponse.retryable?(err) + end + + @tag :capture_log + test "500 is treated as :server_error and is retryable" do + Req.Test.stub(__MODULE__, fn conn -> + conn + |> Plug.Conn.put_status(500) + |> Req.Test.json(%{"error" => "Internal Server Error"}) + end) + + assert {:error, %API.ErrorResponse{kind: :server_error} = err} = + API.get_artist("mbid", @config) + + assert API.ErrorResponse.retryable?(err) + end + end +end diff --git a/test/music_brainz_test.exs b/test/music_brainz_test.exs index 14a88696..ae9bea38 100644 --- a/test/music_brainz_test.exs +++ b/test/music_brainz_test.exs @@ -121,6 +121,7 @@ defmodule MusicBrainzTest do assert releases == [] end + @tag :capture_log test "error on a later page is returned immediately" do full_page = release_page(100) @@ -138,7 +139,7 @@ defmodule MusicBrainzTest do end end) - assert {:error, %{"error" => "service unavailable"}} = + assert {:error, %MusicBrainz.API.ErrorResponse{status: 503, kind: :rate_limit}} = MusicBrainz.get_all_releases(@release_group_id) end end diff --git a/test/music_library/records/similarity_test.exs b/test/music_library/records/similarity_test.exs index 80e5267c..8bda3e1e 100644 --- a/test/music_library/records/similarity_test.exs +++ b/test/music_library/records/similarity_test.exs @@ -298,10 +298,10 @@ defmodule MusicLibrary.Records.SimilarityTest do Req.Test.stub(OpenAI.API, fn conn -> conn |> Plug.Conn.put_status(500) - |> Req.Test.json(%{"error" => "internal server error"}) + |> Req.Test.json(%{"error" => %{"message" => "internal server error"}}) end) - assert {:error, %{"error" => "internal server error"}} = + assert {:error, %OpenAI.API.ErrorResponse{status: 500, kind: :server_error}} = Similarity.generate_embedding(record) end end diff --git a/test/music_library/records_test.exs b/test/music_library/records_test.exs index 8c90c74e..7ba0c8e2 100644 --- a/test/music_library/records_test.exs +++ b/test/music_library/records_test.exs @@ -383,10 +383,15 @@ defmodule MusicLibrary.RecordsTest do record = record(%{genres: []}) Req.Test.stub(OpenAI.API, fn conn -> - Plug.Conn.send_resp(conn, 500, JSON.encode!(%{"error" => "internal server error"})) + Plug.Conn.send_resp( + conn, + 500, + JSON.encode!(%{"error" => %{"message" => "internal server error"}}) + ) end) - assert {:error, "OpenAI API error:" <> _} = Records.populate_genres(record) + assert {:error, %OpenAI.API.ErrorResponse{status: 500, kind: :server_error}} = + Records.populate_genres(record) end end end diff --git a/test/music_library/worker/error_handler_test.exs b/test/music_library/worker/error_handler_test.exs new file mode 100644 index 00000000..5dad2d42 --- /dev/null +++ b/test/music_library/worker/error_handler_test.exs @@ -0,0 +1,97 @@ +defmodule MusicLibrary.Worker.ErrorHandlerTest do + use ExUnit.Case, async: true + + alias MusicLibrary.Worker.ErrorHandler + + describe "to_oban_result/1 — successes" do + test ":ok passes through" do + assert ErrorHandler.to_oban_result(:ok) == :ok + end + + test "{:ok, result} passes through unchanged" do + payload = %{anything: true} + assert ErrorHandler.to_oban_result({:ok, payload}) == {:ok, payload} + end + end + + describe "to_oban_result/1 — retryable ErrorResponses snooze" do + test "MusicBrainz 503 → {:snooze, 60}" do + err = + MusicBrainz.API.ErrorResponse.from_response(%{status: 503, body: %{"error" => "rate"}}) + + assert ErrorHandler.to_oban_result({:error, err}) == {:snooze, 60} + end + + test "Discogs 429 → {:snooze, 60}" do + err = Discogs.API.ErrorResponse.from_response(%{status: 429, body: %{"message" => "rate"}}) + assert ErrorHandler.to_oban_result({:error, err}) == {:snooze, 60} + end + + test "Wikipedia Action API ratelimited → {:snooze, 30}" do + err = + Wikipedia.API.ErrorResponse.from_action_api_body(%{ + "error" => %{"code" => "ratelimited", "info" => "rate"} + }) + + assert ErrorHandler.to_oban_result({:error, err}) == {:snooze, 30} + end + + test "BraveSearch 500 → {:snooze, 30}" do + err = + BraveSearch.API.ErrorResponse.from_response(%{status: 500, body: %{"error" => "boom"}}) + + assert ErrorHandler.to_oban_result({:error, err}) == {:snooze, 30} + end + + test "OpenAI 429 rate_limit_exceeded → {:snooze, 60}" do + err = + OpenAI.API.ErrorResponse.from_response(%{ + status: 429, + body: %{"error" => %{"code" => "rate_limit_exceeded", "message" => "rate"}} + }) + + assert ErrorHandler.to_oban_result({:error, err}) == {:snooze, 60} + end + + test "Last.fm rate_limit_exceeded → {:snooze, 60}" do + err = LastFm.API.ErrorResponse.new(29, "Rate limit exceeded") + assert ErrorHandler.to_oban_result({:error, err}) == {:snooze, 60} + end + end + + describe "to_oban_result/1 — non-retryable ErrorResponses cancel" do + test "MusicBrainz 404 → {:cancel, err}" do + err = MusicBrainz.API.ErrorResponse.from_response(%{status: 404, body: %{"error" => "nf"}}) + assert {:cancel, ^err} = ErrorHandler.to_oban_result({:error, err}) + end + + test "OpenAI 429 insufficient_quota → {:cancel, err}" do + err = + OpenAI.API.ErrorResponse.from_response(%{ + status: 429, + body: %{"error" => %{"code" => "insufficient_quota", "message" => "quota"}} + }) + + assert {:cancel, ^err} = ErrorHandler.to_oban_result({:error, err}) + end + + test "Last.fm invalid_session_key → {:cancel, err}" do + err = LastFm.API.ErrorResponse.new(9, "Invalid session key") + assert {:cancel, ^err} = ErrorHandler.to_oban_result({:error, err}) + end + end + + describe "to_oban_result/1 — passthrough for unstructured errors and atom reasons" do + test "unknown {:error, reason} passes through unchanged" do + assert ErrorHandler.to_oban_result({:error, :something_else}) == {:error, :something_else} + end + + test "{:cancel, _} passes through unchanged" do + assert ErrorHandler.to_oban_result({:cancel, :manual}) == {:cancel, :manual} + end + + test "{:snooze, _} passes through unchanged" do + assert ErrorHandler.to_oban_result({:snooze, 120}) == {:snooze, 120} + end + end +end diff --git a/test/music_library/worker/generate_record_embedding_test.exs b/test/music_library/worker/generate_record_embedding_test.exs index dfb011fa..b221e677 100644 --- a/test/music_library/worker/generate_record_embedding_test.exs +++ b/test/music_library/worker/generate_record_embedding_test.exs @@ -44,16 +44,16 @@ defmodule MusicLibrary.Worker.GenerateRecordEmbeddingTest do end @tag :capture_log - test "returns {:error, reason} when OpenAI API fails" do + test "snoozes when OpenAI API fails with a 5xx (transient)" do record = record() Req.Test.stub(OpenAI.API, fn conn -> conn |> Plug.Conn.put_status(500) - |> Req.Test.json(%{"error" => "internal server error"}) + |> Req.Test.json(%{"error" => %{"message" => "internal server error"}}) end) - assert {:error, %{"error" => "internal server error"}} = + assert {:snooze, 30} = perform_job(GenerateRecordEmbedding, %{"record_id" => record.id}) end diff --git a/test/music_library/worker/populate_genres_test.exs b/test/music_library/worker/populate_genres_test.exs index 5f324d0c..8a3e206f 100644 --- a/test/music_library/worker/populate_genres_test.exs +++ b/test/music_library/worker/populate_genres_test.exs @@ -31,14 +31,38 @@ defmodule MusicLibrary.Worker.PopulateGenresTest do end @tag :capture_log - test "returns error when OpenAI API fails" do + test "snoozes when OpenAI API returns a 5xx (transient)" do record = record(%{genres: []}) Req.Test.stub(OpenAI.API, fn conn -> - Plug.Conn.send_resp(conn, 500, JSON.encode!(%{"error" => "internal server error"})) + conn + |> Plug.Conn.put_status(500) + |> Req.Test.json(%{"error" => %{"message" => "internal server error"}}) end) - assert {:error, "OpenAI API error:" <> _} = + assert {:snooze, 30} = perform_job(PopulateGenres, %{"id" => record.id}) + end + + @tag :capture_log + test "cancels when OpenAI API returns 429 insufficient_quota (permanent)" do + record = record(%{genres: []}) + + Req.Test.stub(OpenAI.API, fn conn -> + conn + |> Plug.Conn.put_resp_content_type("application/json") + |> Plug.Conn.send_resp( + 429, + JSON.encode!(%{ + "error" => %{ + "code" => "insufficient_quota", + "type" => "insufficient_quota", + "message" => "quota exceeded" + } + }) + ) + end) + + assert {:cancel, %OpenAI.API.ErrorResponse{code: "insufficient_quota"}} = perform_job(PopulateGenres, %{"id" => record.id}) end end diff --git a/test/open_ai/api_test.exs b/test/open_ai/api_test.exs index 9502fb35..40415f40 100644 --- a/test/open_ai/api_test.exs +++ b/test/open_ai/api_test.exs @@ -2,6 +2,7 @@ defmodule OpenAI.APITest do use ExUnit.Case, async: true alias OpenAI.API + alias OpenAI.API.ErrorResponse @config %OpenAI.Config{ api_key: "test_key", @@ -26,15 +27,62 @@ defmodule OpenAI.APITest do end @tag :capture_log - test "returns error on non-2xx response" do + test "returns a rate-limit error response on 429 rate_limit_exceeded" do Req.Test.stub(__MODULE__, fn conn -> conn |> Plug.Conn.put_status(429) - |> Req.Test.json(%{"error" => "rate limited"}) + |> Req.Test.json(%{ + "error" => %{ + "code" => "rate_limit_exceeded", + "type" => "rate_limit_error", + "message" => "Rate limit reached" + } + }) end) completion = %{model: "gpt-4.1-mini", content: "test", role: "user", temperature: 0.5} - assert {:error, "OpenAI API error:" <> _} = API.gpt(completion, @config) + + assert {:error, %ErrorResponse{} = err} = API.gpt(completion, @config) + assert err.status == 429 + assert err.code == "rate_limit_exceeded" + assert err.kind == :rate_limit + assert ErrorResponse.retryable?(err) + end + + @tag :capture_log + test "returns an auth-error response on 429 insufficient_quota (non-retryable)" do + Req.Test.stub(__MODULE__, fn conn -> + conn + |> Plug.Conn.put_status(429) + |> Req.Test.json(%{ + "error" => %{ + "code" => "insufficient_quota", + "type" => "insufficient_quota", + "message" => "You exceeded your current quota" + } + }) + end) + + completion = %{model: "gpt-4.1-mini", content: "test", role: "user", temperature: 0.5} + + assert {:error, %ErrorResponse{} = err} = API.gpt(completion, @config) + assert err.code == "insufficient_quota" + assert err.kind == :auth_error + refute ErrorResponse.retryable?(err) + end + + @tag :capture_log + test "returns a server-error response on 500" do + Req.Test.stub(__MODULE__, fn conn -> + conn + |> Plug.Conn.put_status(500) + |> Req.Test.json(%{"error" => %{"message" => "boom"}}) + end) + + completion = %{model: "gpt-4.1-mini", content: "test", role: "user", temperature: 0.5} + + assert {:error, %ErrorResponse{kind: :server_error}} = + API.gpt(completion, @config) end end @@ -54,14 +102,26 @@ defmodule OpenAI.APITest do end @tag :capture_log - test "returns error on non-200 response" do + test "returns a retryable server-error ErrorResponse on 500" do Req.Test.stub(__MODULE__, fn conn -> conn |> Plug.Conn.put_status(500) - |> Req.Test.json(%{"error" => "internal error"}) + |> Req.Test.json(%{"error" => %{"message" => "internal error"}}) end) - assert {:error, %{"error" => "internal error"}} = API.get_embeddings("test text", @config) + assert {:error, %ErrorResponse{status: 500, kind: :server_error} = err} = + API.get_embeddings("test text", @config) + + assert ErrorResponse.retryable?(err) + end + + test "returns the transport exception on connection failure" do + Req.Test.stub(__MODULE__, fn conn -> + Req.Test.transport_error(conn, :timeout) + end) + + assert {:error, %Req.TransportError{reason: :timeout}} = + API.get_embeddings("test text", @config) end end @@ -114,27 +174,27 @@ defmodule OpenAI.APITest do end @tag :capture_log - test "returns error on non-2xx response" do + test "returns an ErrorResponse struct on non-2xx response" do Req.Test.stub(__MODULE__, fn conn -> conn |> Plug.Conn.put_status(500) - |> Req.Test.json(%{"error" => "server error"}) + |> Req.Test.json(%{"error" => %{"message" => "server error"}}) end) cb = fn _chunk -> :ok end - assert {:error, "OpenAI API error:" <> _} = + assert {:error, %ErrorResponse{status: 500, kind: :server_error}} = API.chat_stream([], "instructions", "gpt-4.1", 0.7, @config, cb) end - test "returns error on connection failure" do + test "returns the transport exception on connection failure" do Req.Test.stub(__MODULE__, fn conn -> Req.Test.transport_error(conn, :timeout) end) cb = fn _chunk -> :ok end - assert {:error, "Connection error:" <> _} = + assert {:error, %Req.TransportError{reason: :timeout}} = API.chat_stream([], "instructions", "gpt-4.1", 0.7, @config, cb) end diff --git a/test/wikipedia/api_test.exs b/test/wikipedia/api_test.exs new file mode 100644 index 00000000..b13658be --- /dev/null +++ b/test/wikipedia/api_test.exs @@ -0,0 +1,164 @@ +defmodule Wikipedia.APITest do + use ExUnit.Case, async: true + + alias Wikipedia.API + alias Wikipedia.API.ErrorResponse + + @config %Wikipedia.Config{ + user_agent: "test_agent", + req_options: [plug: {Req.Test, Wikipedia.API}, max_retries: 0], + api_cooldown: 0 + } + + describe "Action API silent-bug fix (HTTP 200 + body error envelope)" do + @tag :capture_log + test "promotes ratelimited Action API error into a retryable ErrorResponse" do + Req.Test.stub(API, fn conn -> + Req.Test.json(conn, %{ + "error" => %{"code" => "ratelimited", "info" => "You've exceeded your rate limit."} + }) + end) + + assert {:error, %ErrorResponse{} = err} = API.get_wikipedia_title("Q352766", @config) + assert err.status == 200 + assert err.code == "ratelimited" + assert err.kind == :rate_limit + assert ErrorResponse.retryable?(err) + end + + @tag :capture_log + test "promotes maxlag Action API error into a retryable ErrorResponse" do + Req.Test.stub(API, fn conn -> + Req.Test.json(conn, %{ + "error" => %{ + "code" => "maxlag", + "info" => "Waiting for a database server: 7 seconds lagged." + } + }) + end) + + assert {:error, %ErrorResponse{kind: :rate_limit} = err} = + API.get_wikipedia_title("Q352766", @config) + + assert err.code == "maxlag" + assert ErrorResponse.retryable?(err) + end + + @tag :capture_log + test "promotes readonly Action API error into a retryable server error" do + Req.Test.stub(API, fn conn -> + Req.Test.json(conn, %{ + "error" => %{"code" => "readonly", "info" => "The wiki is currently in read-only mode."} + }) + end) + + assert {:error, %ErrorResponse{kind: :server_error} = err} = + API.get_wikipedia_title("Q352766", @config) + + assert ErrorResponse.retryable?(err) + end + + @tag :capture_log + test "promotes internal_api_error_* codes into retryable server errors" do + Req.Test.stub(API, fn conn -> + Req.Test.json(conn, %{ + "error" => %{ + "code" => "internal_api_error_DBQueryError", + "info" => "[abc] Exception Caught" + } + }) + end) + + assert {:error, %ErrorResponse{kind: :server_error} = err} = + API.get_wikipedia_title("Q352766", @config) + + assert ErrorResponse.retryable?(err) + end + + @tag :capture_log + test "classifies unknown error codes as non-retryable client errors" do + Req.Test.stub(API, fn conn -> + Req.Test.json(conn, %{ + "error" => %{"code" => "badtoken", "info" => "Invalid CSRF token."} + }) + end) + + assert {:error, %ErrorResponse{kind: :client_error} = err} = + API.get_wikipedia_title("Q352766", @config) + + refute ErrorResponse.retryable?(err) + end + end + + describe "Action API non-error body shapes still return {:ok, _}" do + test "wbgetentities with a missing entity returns {:ok, body} — not an error" do + Req.Test.stub(API, fn conn -> + Req.Test.json(conn, %{ + "entities" => %{"Q99999999999" => %{"id" => "Q99999999999", "missing" => ""}} + }) + end) + + assert {:ok, nil} = API.get_wikipedia_title("Q99999999999", @config) + end + + test "wbgetentities with no enwiki sitelink returns {:ok, nil}" do + Req.Test.stub(API, fn conn -> + Req.Test.json(conn, %{ + "entities" => %{"Q352766" => %{"id" => "Q352766", "sitelinks" => %{}}} + }) + end) + + assert {:ok, nil} = API.get_wikipedia_title("Q352766", @config) + end + + test "query with empty extract returns {:ok, nil}" do + Req.Test.stub(API, fn conn -> + Req.Test.json(conn, %{ + "query" => %{ + "pages" => %{"123" => %{"pageid" => 123, "title" => "X", "extract" => nil}} + } + }) + end) + + assert {:ok, nil} = API.get_article_extract("X", @config) + end + end + + describe "REST v1 API error classification" do + @tag :capture_log + test "404 on article summary surfaces as a not-found ErrorResponse" do + Req.Test.stub(API, fn conn -> + conn + |> Plug.Conn.put_status(404) + |> Req.Test.json(%{ + "httpCode" => 404, + "httpReason" => "Not Found", + "messageTranslations" => %{"en" => "The specified title does not exist."} + }) + end) + + assert {:error, %ErrorResponse{status: 404, kind: :not_found} = err} = + API.get_article_summary("Missing%20Title", @config) + + refute ErrorResponse.retryable?(err) + end + + @tag :capture_log + test "429 on article summary surfaces as a retryable rate-limit ErrorResponse" do + Req.Test.stub(API, fn conn -> + conn + |> Plug.Conn.put_status(429) + |> Req.Test.json(%{ + "httpCode" => 429, + "httpReason" => "Too Many Requests", + "messageTranslations" => %{"en" => "Slow down."} + }) + end) + + assert {:error, %ErrorResponse{status: 429, kind: :rate_limit} = err} = + API.get_article_summary("Steven%20Wilson", @config) + + assert ErrorResponse.retryable?(err) + end + end +end diff --git a/test/wikipedia_test.exs b/test/wikipedia_test.exs index 775dfdda..cd03e067 100644 --- a/test/wikipedia_test.exs +++ b/test/wikipedia_test.exs @@ -1,6 +1,8 @@ defmodule WikipediaTest do use ExUnit.Case, async: true + alias Wikipedia.API.ErrorResponse + describe "get_artist_summary/1" do test "resolves wikidata ID to Wikipedia summary with full intro" do wikidata_id = "Q352766" @@ -38,5 +40,40 @@ defmodule WikipediaTest do assert {:error, :no_english_wikipedia} = Wikipedia.get_artist_summary(wikidata_id) end + + @tag :capture_log + test "promotes HTTP 200 Action API error envelopes into ErrorResponse structs" do + wikidata_id = "Q352766" + + Req.Test.stub(Wikipedia.API, fn conn -> + Req.Test.json(conn, %{ + "error" => %{"code" => "ratelimited", "info" => "You've exceeded your rate limit."} + }) + end) + + assert {:error, %ErrorResponse{} = err} = + Wikipedia.get_artist_summary(wikidata_id) + + assert err.status == 200 + assert err.code == "ratelimited" + assert err.kind == :rate_limit + assert ErrorResponse.retryable?(err) + end + + @tag :capture_log + test "classifies non-retryable Action API error codes as client errors" do + wikidata_id = "Q352766" + + Req.Test.stub(Wikipedia.API, fn conn -> + Req.Test.json(conn, %{ + "error" => %{"code" => "badtoken", "info" => "Invalid CSRF token."} + }) + end) + + assert {:error, %ErrorResponse{kind: :client_error} = err} = + Wikipedia.get_artist_summary(wikidata_id) + + refute ErrorResponse.retryable?(err) + end end end