diff --git a/backlog/tasks/ml-146 - Honour-Retry-After-and-rate-limit-reset-headers-for-precise-snooze.md b/backlog/tasks/ml-146 - Honour-Retry-After-and-rate-limit-reset-headers-for-precise-snooze.md index 712ad4cd..b707ec4d 100644 --- a/backlog/tasks/ml-146 - Honour-Retry-After-and-rate-limit-reset-headers-for-precise-snooze.md +++ b/backlog/tasks/ml-146 - Honour-Retry-After-and-rate-limit-reset-headers-for-precise-snooze.md @@ -1,9 +1,11 @@ --- id: ML-146 title: Honour Retry-After and rate-limit reset headers for precise snooze -status: To Do -assignee: [] +status: Done +assignee: + - Codex created_date: '2026-04-24 11:12' +updated_date: '2026-04-25 06:37' labels: [] dependencies: - ML-21 @@ -46,8 +48,58 @@ Blocked by ML-21 — requires the classification plumbing to exist first. ## Acceptance Criteria -- [ ] #1 Each API module extracts a retry-delay value from its response (Retry-After, X-RateLimit-Reset, x-ratelimit-reset-*) when present -- [ ] #2 Workers emit {:snooze, seconds} with the parsed value for transient errors, falling back to a fixed default when the header is absent or malformed -- [ ] #3 Parsed durations are clamped to a safe range to prevent pathological values -- [ ] #4 Per-API header parsing is covered by unit tests using representative fixture responses +- [x] #1 Each API module extracts a retry-delay value from its response (Retry-After, X-RateLimit-Reset, x-ratelimit-reset-*) when present +- [x] #2 Workers emit {:snooze, seconds} with the parsed value for transient errors, falling back to a fixed default when the header is absent or malformed +- [x] #3 Parsed durations are clamped to a safe range to prevent pathological values +- [x] #4 Per-API header parsing is covered by unit tests using representative fixture responses + +## Implementation Plan + + +# Implementation Plan + +- Add a shared `MusicLibrary.RetryDelay` helper that parses retry/reset headers from `Req.Response` values, clamps parsed provider hints to 5..300 seconds, uses the maximum valid value for multi-window headers, and returns nil for absent or malformed hints. +- Extend HTTP-based API `ErrorResponse` structs with an optional `retry_delay_seconds` field populated at `from_response/1` time. +- Parse provider hints for MusicBrainz and Wikipedia `retry-after`, Brave `x-ratelimit-reset`, and OpenAI `x-ratelimit-reset-requests` / `x-ratelimit-reset-tokens`; keep Discogs and Last.fm on their existing fixed fallbacks. +- Update each affected `retry_delay_seconds/1` implementation to prefer the parsed field when present and preserve existing defaults otherwise. +- Add focused unit coverage for the shared parser, per-API parsed/fallback behavior, and one `ErrorHandler` integration assertion showing parsed values flow through to `{:snooze, seconds}`. + + +## Implementation Notes + + +Implemented precise retry-delay parsing through the existing ErrorResponse callback flow. Added MusicLibrary.RetryDelay with 5..300s clamping and max-window selection for multi-window headers. MusicBrainz/Wikipedia parse Retry-After, Brave parses X-RateLimit-Reset, OpenAI parses request/token reset durations. Discogs and Last.fm remain on fixed fallbacks because they do not expose a reliable retry-delay header in scope. Verification passed: focused API/error-handler tests, full mix test suite, mix format --check-formatted, and git diff --check. + +Addressed review gaps after implementation: Wikipedia Action API error promotion now passes the full Req response to `from_action_api_body/2` so `Retry-After` is preserved; OpenAI retry parsing now includes `Retry-After` and compound reset durations such as `1m30s`; `Retry-After: 0` now clamps to the 5s minimum instead of falling back. Verification passed: focused retry/API tests, full `mix test`, and `mix format --check-formatted`. + + +## Final Summary + + +## Summary + +Added shared retry-delay parsing for provider retry/reset headers and wired it into the existing structured API error flow. HTTP-based ErrorResponse structs for MusicBrainz, Wikipedia, Brave Search, and OpenAI now capture an optional parsed retry delay and prefer it from `retry_delay_seconds/1`; workers automatically emit `{:snooze, parsed_seconds}` through the existing `MusicLibrary.Worker.ErrorHandler` path. + +## Details + +- New `MusicLibrary.RetryDelay` parses `Retry-After`, comma-separated reset-second headers, and OpenAI duration reset headers. +- Parsed provider hints are clamped to 5..300 seconds and multi-window headers use the maximum valid parsed value. +- Existing fixed fallbacks are preserved when hints are absent or malformed. +- Discogs and Last.fm remain on current fallback behavior because they do not provide a reliable retry-delay header in this scope. +- Updated architecture docs for the new shared helper. + +## Tests + +- `mix test test/music_library/retry_delay_test.exs test/music_brainz/api_test.exs test/wikipedia/api_test.exs test/brave_search/api_test.exs test/open_ai/api_test.exs test/music_library/worker/error_handler_test.exs` +- `mix test` +- `mix format --check-formatted` +- `git diff --check` + +## Review follow-up + +- Wikipedia Action API body-error responses now preserve response headers for `Retry-After` parsing. +- OpenAI retry parsing now considers `Retry-After` alongside request/token reset headers and supports compound durations like `1m30s`. +- Zero-second retry hints clamp to the 5s minimum rather than falling back to fixed defaults. +- Architecture docs now mention header-driven snooze delays in the external API integration notes. + diff --git a/docs/architecture.md b/docs/architecture.md index 267cbf87..f28d69f6 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -138,6 +138,7 @@ Last.fm schemas (separate, not Ecto-persisted to main DB): | `ErrorIgnorer` | ErrorTracker.Ignorer implementation: filters non-actionable errors (e.g., NoRouteError from bot scanners) | | `MusicLibrary.ErrorResponse` | Behaviour for structured API error responses (`retryable?/1`, `retry_delay_seconds/1`) — per-API `ErrorResponse` modules implement this | | `MusicLibrary.HttpError` | Default HTTP status → kind mapping (`:rate_limit`, `:server_error`, `:timeout`, `:auth_error`, `:not_found`, `:client_error`, `:unknown`) used as baseline by per-API `ErrorResponse` modules | +| `MusicLibrary.RetryDelay` | Parses and clamps provider retry/reset headers into Oban snooze delays for structured API errors | | `MusicLibrary.Worker.ErrorHandler` | Translates per-API `ErrorResponse` structs into Oban tuples — `{:snooze, seconds}` for retryable, `{:cancel, reason}` for permanent | | `MusicLibraryWeb.RecordsOnThisDayEmail` | Builds and sends daily "records on this day" email with cover images, anniversary styling | | `MusicLibrary.Mailer` | Swoosh mailer (Mailgun in prod, local adapter in dev) | @@ -165,10 +166,11 @@ stubbed via `Req.Test` (configured in `config/test.exs`). Each API also has an `API.ErrorResponse` module (e.g. `MusicBrainz.API.ErrorResponse`, `OpenAI.API.ErrorResponse`) implementing the `MusicLibrary.ErrorResponse` behaviour, so workers can uniformly classify HTTP failures as transient (snooze) or permanent -(cancel) via `MusicLibrary.Worker.ErrorHandler`. Per-API overrides capture -API-specific quirks — e.g. MusicBrainz uses HTTP 503 as the rate-limit signal, and -OpenAI splits HTTP 429 into `:rate_limit` vs `:auth_error` by reading the body -`code` (`insufficient_quota` → permanent). +(cancel) via `MusicLibrary.Worker.ErrorHandler`. Retry/reset headers are parsed into +clamped snooze delays when providers expose them. Per-API overrides capture API-specific +quirks — e.g. MusicBrainz uses HTTP 503 as the rate-limit signal, and OpenAI splits +HTTP 429 into `:rate_limit` vs `:auth_error` by reading the body `code` +(`insufficient_quota` → permanent). --- diff --git a/lib/brave_search/api/error_response.ex b/lib/brave_search/api/error_response.ex index 69e718ff..1c6cf900 100644 --- a/lib/brave_search/api/error_response.ex +++ b/lib/brave_search/api/error_response.ex @@ -16,25 +16,28 @@ defmodule BraveSearch.API.ErrorResponse do @behaviour MusicLibrary.ErrorResponse alias MusicLibrary.HttpError + alias MusicLibrary.RetryDelay @type t :: %__MODULE__{ status: integer() | nil, code: String.t() | nil, message: String.t() | nil, kind: HttpError.kind(), - body: term() + body: term(), + retry_delay_seconds: pos_integer() | nil } - defstruct [:status, :code, :message, :kind, :body] + defstruct [:status, :code, :message, :kind, :body, :retry_delay_seconds] @spec from_response(Req.Response.t() | map()) :: t() - def from_response(%{status: status, body: body} = _response) do + 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 + body: body, + retry_delay_seconds: RetryDelay.reset_seconds(response, "x-ratelimit-reset") } end @@ -47,6 +50,9 @@ defmodule BraveSearch.API.ErrorResponse do @impl MusicLibrary.ErrorResponse @spec retry_delay_seconds(t()) :: pos_integer() + def retry_delay_seconds(%__MODULE__{retry_delay_seconds: seconds}) when is_integer(seconds), + do: seconds + 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 diff --git a/lib/music_brainz/api/error_response.ex b/lib/music_brainz/api/error_response.ex index c4021d9b..0bdb470c 100644 --- a/lib/music_brainz/api/error_response.ex +++ b/lib/music_brainz/api/error_response.ex @@ -16,32 +16,36 @@ defmodule MusicBrainz.API.ErrorResponse do @behaviour MusicLibrary.ErrorResponse alias MusicLibrary.HttpError + alias MusicLibrary.RetryDelay @type t :: %__MODULE__{ status: integer() | nil, message: String.t() | nil, kind: HttpError.kind(), - body: term() + body: term(), + retry_delay_seconds: pos_integer() | nil } - defstruct [:status, :message, :kind, :body] + defstruct [:status, :message, :kind, :body, :retry_delay_seconds] @spec from_response(Req.Response.t() | map()) :: t() - def from_response(%{status: 503, body: body} = _response) do + def from_response(%{status: 503, body: body} = response) do %__MODULE__{ status: 503, message: extract_message(body), kind: :rate_limit, - body: body + body: body, + retry_delay_seconds: RetryDelay.retry_after_seconds(response) } end - def from_response(%{status: status, body: body} = _response) do + def from_response(%{status: status, body: body} = response) do %__MODULE__{ status: status, message: extract_message(body), kind: HttpError.default_kind(status), - body: body + body: body, + retry_delay_seconds: RetryDelay.retry_after_seconds(response) } end @@ -54,6 +58,9 @@ defmodule MusicBrainz.API.ErrorResponse do @impl MusicLibrary.ErrorResponse @spec retry_delay_seconds(t()) :: pos_integer() + def retry_delay_seconds(%__MODULE__{retry_delay_seconds: seconds}) when is_integer(seconds), + do: seconds + 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 diff --git a/lib/music_library/retry_delay.ex b/lib/music_library/retry_delay.ex new file mode 100644 index 00000000..2eff5d97 --- /dev/null +++ b/lib/music_library/retry_delay.ex @@ -0,0 +1,133 @@ +defmodule MusicLibrary.RetryDelay do + @moduledoc """ + Parses provider retry/reset headers into clamped snooze delays. + + Header values come from upstream APIs and should not be trusted blindly. Parsed + values are clamped to keep Oban snoozes useful without allowing pathological + values to churn jobs or stall them for too long. + """ + + alias Req.Response + + @min_seconds 5 + @max_seconds 300 + + @doc """ + Parses a `Retry-After` header that contains seconds. + """ + @spec retry_after_seconds(Response.t() | map()) :: pos_integer() | nil + def retry_after_seconds(response), do: integer_header_seconds(response, "retry-after") + + @doc """ + Parses a reset header containing one or more comma-separated second values. + """ + @spec reset_seconds(Response.t() | map(), String.t()) :: pos_integer() | nil + def reset_seconds(response, header_name), do: integer_header_seconds(response, header_name) + + @doc """ + Parses OpenAI request/token reset duration headers. + """ + @spec openai_reset_seconds(Response.t() | map()) :: pos_integer() | nil + def openai_reset_seconds(response) do + values = + header_values(response, "retry-after") ++ + header_values(response, "x-ratelimit-reset-requests") ++ + header_values(response, "x-ratelimit-reset-tokens") + + values + |> Enum.flat_map(&String.split(&1, ",")) + |> Enum.map(&parse_openai_reset/1) + |> max_clamped() + end + + defp integer_header_seconds(response, header_name) do + response + |> header_values(header_name) + |> Enum.flat_map(&String.split(&1, ",")) + |> Enum.map(&parse_positive_integer/1) + |> max_clamped() + end + + defp header_values(%{headers: _} = response, header_name) do + Response.get_header(response, String.downcase(header_name)) + end + + defp header_values(_response, _header_name), do: [] + + defp parse_positive_integer(value) do + value = String.trim(value) + + case Integer.parse(value) do + {seconds, ""} when seconds >= 0 -> seconds + _ -> nil + end + end + + defp parse_openai_reset(value) do + case parse_positive_integer(value) do + seconds when is_integer(seconds) -> seconds + nil -> parse_duration(value) + end + end + + defp parse_duration(value) do + value = String.trim(value) + + case Regex.scan(~r/(\d+(?:\.\d+)?)(ms|s|m)/i, value) do + [] -> nil + parts -> parts_to_seconds(parts, value) + end + end + + defp parts_to_seconds(parts, value) do + parsed = + Enum.map(parts, fn [_token, amount, unit] -> + {parse_number(amount), String.downcase(unit)} + end) + + rebuilt = + Enum.map_join(parts, fn [token, _amount, _unit] -> String.downcase(token) end) + + normalized = value |> String.downcase() |> String.replace(~r/\s+/, "") + + if rebuilt == normalized do + total_duration_seconds(parsed) + else + nil + end + end + + defp total_duration_seconds(parts) do + parts + |> Enum.map(fn {amount, unit} -> duration_to_seconds(amount, unit) end) + |> Enum.reduce_while(0, fn + nil, _total -> {:halt, nil} + seconds, total -> {:cont, total + seconds} + end) + end + + defp parse_number(amount) do + case Float.parse(amount) do + {number, ""} -> number + _ -> nil + end + end + + defp duration_to_seconds(nil, _unit), do: nil + defp duration_to_seconds(amount, _unit) when amount < 0, do: nil + defp duration_to_seconds(amount, "ms"), do: ceil(amount / 1000) + defp duration_to_seconds(amount, "s"), do: ceil(amount) + defp duration_to_seconds(amount, "m"), do: ceil(amount * 60) + + defp max_clamped(values) do + values + |> Enum.reject(&is_nil/1) + |> Enum.max(fn -> nil end) + |> clamp() + end + + defp clamp(nil), do: nil + defp clamp(seconds) when seconds < @min_seconds, do: @min_seconds + defp clamp(seconds) when seconds > @max_seconds, do: @max_seconds + defp clamp(seconds), do: seconds +end diff --git a/lib/open_ai/api/error_response.ex b/lib/open_ai/api/error_response.ex index ec8c8f00..112c841c 100644 --- a/lib/open_ai/api/error_response.ex +++ b/lib/open_ai/api/error_response.ex @@ -20,6 +20,7 @@ defmodule OpenAI.API.ErrorResponse do @behaviour MusicLibrary.ErrorResponse alias MusicLibrary.HttpError + alias MusicLibrary.RetryDelay @type t :: %__MODULE__{ status: integer() | nil, @@ -27,10 +28,11 @@ defmodule OpenAI.API.ErrorResponse do type: String.t() | nil, message: String.t() | nil, kind: HttpError.kind(), - body: term() + body: term(), + retry_delay_seconds: pos_integer() | nil } - defstruct [:status, :code, :type, :message, :kind, :body] + defstruct [:status, :code, :type, :message, :kind, :body, :retry_delay_seconds] @spec from_response(Req.Response.t() | map()) :: t() def from_response(%{status: 429, body: %{"error" => %{"code" => "insufficient_quota"} = e}} = r) do @@ -40,11 +42,12 @@ defmodule OpenAI.API.ErrorResponse do type: e["type"], message: e["message"], kind: :auth_error, - body: r.body + body: r.body, + retry_delay_seconds: RetryDelay.openai_reset_seconds(r) } end - def from_response(%{status: status, body: %{"error" => err} = body} = _response) + def from_response(%{status: status, body: %{"error" => err} = body} = response) when is_map(err) do %__MODULE__{ status: status, @@ -52,18 +55,20 @@ defmodule OpenAI.API.ErrorResponse do type: err["type"], message: err["message"], kind: HttpError.default_kind(status), - body: body + body: body, + retry_delay_seconds: RetryDelay.openai_reset_seconds(response) } end - def from_response(%{status: status, body: body} = _response) do + 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 + body: body, + retry_delay_seconds: RetryDelay.openai_reset_seconds(response) } end @@ -76,6 +81,9 @@ defmodule OpenAI.API.ErrorResponse do @impl MusicLibrary.ErrorResponse @spec retry_delay_seconds(t()) :: pos_integer() + def retry_delay_seconds(%__MODULE__{retry_delay_seconds: seconds}) when is_integer(seconds), + do: seconds + 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 diff --git a/lib/wikipedia/api.ex b/lib/wikipedia/api.ex index 6f1bfaa6..4ee55e6d 100644 --- a/lib/wikipedia/api.ex +++ b/lib/wikipedia/api.ex @@ -123,7 +123,7 @@ defmodule Wikipedia.API do {request, %{status: 200, body: %{"error" => %{"code" => _, "info" => _}} = body} = response} ) do - error = ErrorResponse.from_action_api_body(body) + error = ErrorResponse.from_action_api_body(body, response) Logger.error(fn -> url = URI.to_string(request.url) diff --git a/lib/wikipedia/api/error_response.ex b/lib/wikipedia/api/error_response.ex index 3ce3958f..7add67ba 100644 --- a/lib/wikipedia/api/error_response.ex +++ b/lib/wikipedia/api/error_response.ex @@ -12,8 +12,8 @@ defmodule Wikipedia.API.ErrorResponse do * **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. + `from_response/1` handles classic HTTP errors. `from_action_api_body/2` is a + dedicated entry point for the HTTP 200 + body-error case. ## Non-error body shapes @@ -32,36 +32,42 @@ defmodule Wikipedia.API.ErrorResponse do @behaviour MusicLibrary.ErrorResponse alias MusicLibrary.HttpError + alias MusicLibrary.RetryDelay @type t :: %__MODULE__{ status: integer() | nil, code: String.t() | nil, message: String.t() | nil, kind: HttpError.kind(), - body: term() + body: term(), + retry_delay_seconds: pos_integer() | nil } - defstruct [:status, :code, :message, :kind, :body] + defstruct [:status, :code, :message, :kind, :body, :retry_delay_seconds] @spec from_response(Req.Response.t() | map()) :: t() - def from_response(%{status: status, body: body} = _response) do + 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 + body: body, + retry_delay_seconds: RetryDelay.retry_after_seconds(response) } end - @spec from_action_api_body(map()) :: t() - def from_action_api_body(%{"error" => %{"code" => code, "info" => info}} = body) do + @spec from_action_api_body(map(), Req.Response.t() | map()) :: t() + def from_action_api_body(body, response \\ %{}) + + def from_action_api_body(%{"error" => %{"code" => code, "info" => info}} = body, response) do %__MODULE__{ status: 200, code: code, message: info, kind: action_api_kind(code), - body: body + body: body, + retry_delay_seconds: RetryDelay.retry_after_seconds(response) } end @@ -74,6 +80,9 @@ defmodule Wikipedia.API.ErrorResponse do @impl MusicLibrary.ErrorResponse @spec retry_delay_seconds(t()) :: pos_integer() + def retry_delay_seconds(%__MODULE__{retry_delay_seconds: seconds}) when is_integer(seconds), + do: seconds + 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 diff --git a/test/brave_search/api_test.exs b/test/brave_search/api_test.exs index 5ad56c65..3e8eb7c1 100644 --- a/test/brave_search/api_test.exs +++ b/test/brave_search/api_test.exs @@ -57,6 +57,7 @@ defmodule BraveSearch.APITest do Req.Test.stub(__MODULE__, fn conn -> conn |> Plug.Conn.put_status(429) + |> Plug.Conn.put_resp_header("x-ratelimit-reset", "12, 60") |> Req.Test.json(%{ "type" => "ErrorResponse", "error" => %{"status" => 429, "code" => "RATE_LIMITED", "detail" => "Too many requests"} @@ -69,6 +70,8 @@ defmodule BraveSearch.APITest do assert err.status == 429 assert err.code == "RATE_LIMITED" assert err.kind == :rate_limit + assert err.retry_delay_seconds == 60 + assert ErrorResponse.retry_delay_seconds(err) == 60 assert ErrorResponse.retryable?(err) end diff --git a/test/music_brainz/api_test.exs b/test/music_brainz/api_test.exs index 48483591..0f2296b2 100644 --- a/test/music_brainz/api_test.exs +++ b/test/music_brainz/api_test.exs @@ -15,6 +15,7 @@ defmodule MusicBrainz.APITest do Req.Test.stub(__MODULE__, fn conn -> conn |> Plug.Conn.put_status(503) + |> Plug.Conn.put_resp_header("retry-after", "42") |> Req.Test.json(%{"error" => "Your requests are exceeding the allowable rate limit."}) end) @@ -23,6 +24,8 @@ defmodule MusicBrainz.APITest do assert err.status == 503 assert err.kind == :rate_limit + assert err.retry_delay_seconds == 42 + assert API.ErrorResponse.retry_delay_seconds(err) == 42 assert API.ErrorResponse.retryable?(err) end diff --git a/test/music_library/retry_delay_test.exs b/test/music_library/retry_delay_test.exs new file mode 100644 index 00000000..89559fa6 --- /dev/null +++ b/test/music_library/retry_delay_test.exs @@ -0,0 +1,77 @@ +defmodule MusicLibrary.RetryDelayTest do + use ExUnit.Case, async: true + + alias MusicLibrary.RetryDelay + + describe "retry_after_seconds/1" do + test "parses integer seconds" do + response = response(%{"retry-after" => ["42"]}) + + assert RetryDelay.retry_after_seconds(response) == 42 + end + + test "clamps parsed values to safe bounds" do + assert RetryDelay.retry_after_seconds(response(%{"retry-after" => ["1"]})) == 5 + assert RetryDelay.retry_after_seconds(response(%{"retry-after" => ["999"]})) == 300 + end + + test "returns nil for missing or malformed values" do + assert RetryDelay.retry_after_seconds(response(%{})) == nil + assert RetryDelay.retry_after_seconds(response(%{"retry-after" => ["tomorrow"]})) == nil + assert RetryDelay.retry_after_seconds(response(%{"retry-after" => ["-1"]})) == nil + end + + test "clamps zero to the safe minimum" do + assert RetryDelay.retry_after_seconds(response(%{"retry-after" => ["0"]})) == 5 + end + end + + describe "reset_seconds/2" do + test "uses the largest valid comma-separated reset window" do + response = response(%{"x-ratelimit-reset" => ["12, 60"]}) + + assert RetryDelay.reset_seconds(response, "x-ratelimit-reset") == 60 + end + + test "ignores malformed windows and clamps the selected value" do + response = response(%{"x-ratelimit-reset" => ["bad, 2, 600"]}) + + assert RetryDelay.reset_seconds(response, "x-ratelimit-reset") == 300 + end + end + + describe "openai_reset_seconds/1" do + test "uses the largest reset across retry-after, request, and token windows" do + response = + response(%{ + "retry-after" => ["10"], + "x-ratelimit-reset-requests" => ["20s"], + "x-ratelimit-reset-tokens" => ["1m30s"] + }) + + assert RetryDelay.openai_reset_seconds(response) == 90 + end + + test "clamps sub-second OpenAI durations to the safe minimum" do + response = response(%{"x-ratelimit-reset-requests" => ["120ms"]}) + + assert RetryDelay.openai_reset_seconds(response) == 5 + end + + test "parses minute durations and clamps long values" do + response = response(%{"x-ratelimit-reset-tokens" => ["10m"]}) + + assert RetryDelay.openai_reset_seconds(response) == 300 + end + + test "returns nil when OpenAI reset headers are absent or malformed" do + assert RetryDelay.openai_reset_seconds(response(%{})) == nil + + assert RetryDelay.openai_reset_seconds( + response(%{"x-ratelimit-reset-requests" => ["soon"]}) + ) == nil + end + end + + defp response(headers), do: Req.Response.new(status: 429, headers: headers) +end diff --git a/test/music_library/worker/error_handler_test.exs b/test/music_library/worker/error_handler_test.exs index 5dad2d42..29f6058c 100644 --- a/test/music_library/worker/error_handler_test.exs +++ b/test/music_library/worker/error_handler_test.exs @@ -22,6 +22,19 @@ defmodule MusicLibrary.Worker.ErrorHandlerTest do assert ErrorHandler.to_oban_result({:error, err}) == {:snooze, 60} end + test "parsed retry headers determine the snooze duration" do + response = + Req.Response.new( + status: 503, + headers: %{"retry-after" => ["42"]}, + body: %{"error" => "rate"} + ) + + err = MusicBrainz.API.ErrorResponse.from_response(response) + + assert ErrorHandler.to_oban_result({:error, err}) == {:snooze, 42} + 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} diff --git a/test/open_ai/api_test.exs b/test/open_ai/api_test.exs index 40415f40..f6db53bd 100644 --- a/test/open_ai/api_test.exs +++ b/test/open_ai/api_test.exs @@ -31,6 +31,9 @@ defmodule OpenAI.APITest do Req.Test.stub(__MODULE__, fn conn -> conn |> Plug.Conn.put_status(429) + |> Plug.Conn.put_resp_header("retry-after", "10") + |> Plug.Conn.put_resp_header("x-ratelimit-reset-requests", "20s") + |> Plug.Conn.put_resp_header("x-ratelimit-reset-tokens", "1m30s") |> Req.Test.json(%{ "error" => %{ "code" => "rate_limit_exceeded", @@ -46,6 +49,8 @@ defmodule OpenAI.APITest do assert err.status == 429 assert err.code == "rate_limit_exceeded" assert err.kind == :rate_limit + assert err.retry_delay_seconds == 90 + assert ErrorResponse.retry_delay_seconds(err) == 90 assert ErrorResponse.retryable?(err) end diff --git a/test/wikipedia/api_test.exs b/test/wikipedia/api_test.exs index b13658be..f6e41673 100644 --- a/test/wikipedia/api_test.exs +++ b/test/wikipedia/api_test.exs @@ -14,7 +14,9 @@ defmodule Wikipedia.APITest 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, %{ + conn + |> Plug.Conn.put_resp_header("retry-after", "42") + |> Req.Test.json(%{ "error" => %{"code" => "ratelimited", "info" => "You've exceeded your rate limit."} }) end) @@ -23,6 +25,8 @@ defmodule Wikipedia.APITest do assert err.status == 200 assert err.code == "ratelimited" assert err.kind == :rate_limit + assert err.retry_delay_seconds == 42 + assert ErrorResponse.retry_delay_seconds(err) == 42 assert ErrorResponse.retryable?(err) end @@ -148,6 +152,7 @@ defmodule Wikipedia.APITest do Req.Test.stub(API, fn conn -> conn |> Plug.Conn.put_status(429) + |> Plug.Conn.put_resp_header("retry-after", "25") |> Req.Test.json(%{ "httpCode" => 429, "httpReason" => "Too Many Requests", @@ -158,6 +163,8 @@ defmodule Wikipedia.APITest do assert {:error, %ErrorResponse{status: 429, kind: :rate_limit} = err} = API.get_article_summary("Steven%20Wilson", @config) + assert err.retry_delay_seconds == 25 + assert ErrorResponse.retry_delay_seconds(err) == 25 assert ErrorResponse.retryable?(err) end end