ML-21: Classify API errors as transient vs permanent

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}")

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