ML-146: honour API retry headers
This commit is contained in:
+58
-6
@@ -1,9 +1,11 @@
|
|||||||
---
|
---
|
||||||
id: ML-146
|
id: ML-146
|
||||||
title: Honour Retry-After and rate-limit reset headers for precise snooze
|
title: Honour Retry-After and rate-limit reset headers for precise snooze
|
||||||
status: To Do
|
status: Done
|
||||||
assignee: []
|
assignee:
|
||||||
|
- Codex
|
||||||
created_date: '2026-04-24 11:12'
|
created_date: '2026-04-24 11:12'
|
||||||
|
updated_date: '2026-04-25 06:37'
|
||||||
labels: []
|
labels: []
|
||||||
dependencies:
|
dependencies:
|
||||||
- ML-21
|
- ML-21
|
||||||
@@ -46,8 +48,58 @@ Blocked by ML-21 — requires the classification plumbing to exist first.
|
|||||||
|
|
||||||
## Acceptance Criteria
|
## Acceptance Criteria
|
||||||
<!-- AC:BEGIN -->
|
<!-- AC:BEGIN -->
|
||||||
- [ ] #1 Each API module extracts a retry-delay value from its response (Retry-After, X-RateLimit-Reset, x-ratelimit-reset-*) when present
|
- [x] #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
|
- [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
|
||||||
- [ ] #3 Parsed durations are clamped to a safe range to prevent pathological values
|
- [x] #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] #4 Per-API header parsing is covered by unit tests using representative fixture responses
|
||||||
<!-- AC:END -->
|
<!-- AC:END -->
|
||||||
|
|
||||||
|
## Implementation Plan
|
||||||
|
|
||||||
|
<!-- SECTION:PLAN:BEGIN -->
|
||||||
|
# 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}`.
|
||||||
|
<!-- SECTION:PLAN:END -->
|
||||||
|
|
||||||
|
## Implementation Notes
|
||||||
|
|
||||||
|
<!-- SECTION:NOTES:BEGIN -->
|
||||||
|
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`.
|
||||||
|
<!-- SECTION:NOTES:END -->
|
||||||
|
|
||||||
|
## Final Summary
|
||||||
|
|
||||||
|
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
|
||||||
|
## 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.
|
||||||
|
<!-- SECTION:FINAL_SUMMARY:END -->
|
||||||
|
|||||||
@@ -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) |
|
| `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.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.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 |
|
| `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 |
|
| `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) |
|
| `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`,
|
Each API also has an `API.ErrorResponse` module (e.g. `MusicBrainz.API.ErrorResponse`,
|
||||||
`OpenAI.API.ErrorResponse`) implementing the `MusicLibrary.ErrorResponse` behaviour,
|
`OpenAI.API.ErrorResponse`) implementing the `MusicLibrary.ErrorResponse` behaviour,
|
||||||
so workers can uniformly classify HTTP failures as transient (snooze) or permanent
|
so workers can uniformly classify HTTP failures as transient (snooze) or permanent
|
||||||
(cancel) via `MusicLibrary.Worker.ErrorHandler`. Per-API overrides capture
|
(cancel) via `MusicLibrary.Worker.ErrorHandler`. Retry/reset headers are parsed into
|
||||||
API-specific quirks — e.g. MusicBrainz uses HTTP 503 as the rate-limit signal, and
|
clamped snooze delays when providers expose them. Per-API overrides capture API-specific
|
||||||
OpenAI splits HTTP 429 into `:rate_limit` vs `:auth_error` by reading the body
|
quirks — e.g. MusicBrainz uses HTTP 503 as the rate-limit signal, and OpenAI splits
|
||||||
`code` (`insufficient_quota` → permanent).
|
HTTP 429 into `:rate_limit` vs `:auth_error` by reading the body `code`
|
||||||
|
(`insufficient_quota` → permanent).
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|||||||
@@ -16,25 +16,28 @@ defmodule BraveSearch.API.ErrorResponse do
|
|||||||
@behaviour MusicLibrary.ErrorResponse
|
@behaviour MusicLibrary.ErrorResponse
|
||||||
|
|
||||||
alias MusicLibrary.HttpError
|
alias MusicLibrary.HttpError
|
||||||
|
alias MusicLibrary.RetryDelay
|
||||||
|
|
||||||
@type t :: %__MODULE__{
|
@type t :: %__MODULE__{
|
||||||
status: integer() | nil,
|
status: integer() | nil,
|
||||||
code: String.t() | nil,
|
code: String.t() | nil,
|
||||||
message: String.t() | nil,
|
message: String.t() | nil,
|
||||||
kind: HttpError.kind(),
|
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()
|
@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__{
|
%__MODULE__{
|
||||||
status: status,
|
status: status,
|
||||||
code: extract_code(body),
|
code: extract_code(body),
|
||||||
message: extract_message(body),
|
message: extract_message(body),
|
||||||
kind: HttpError.default_kind(status),
|
kind: HttpError.default_kind(status),
|
||||||
body: body
|
body: body,
|
||||||
|
retry_delay_seconds: RetryDelay.reset_seconds(response, "x-ratelimit-reset")
|
||||||
}
|
}
|
||||||
end
|
end
|
||||||
|
|
||||||
@@ -47,6 +50,9 @@ defmodule BraveSearch.API.ErrorResponse do
|
|||||||
|
|
||||||
@impl MusicLibrary.ErrorResponse
|
@impl MusicLibrary.ErrorResponse
|
||||||
@spec retry_delay_seconds(t()) :: pos_integer()
|
@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: :rate_limit}), do: 60
|
||||||
def retry_delay_seconds(%__MODULE__{kind: :server_error}), 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__{kind: :timeout}), do: 10
|
||||||
|
|||||||
@@ -16,32 +16,36 @@ defmodule MusicBrainz.API.ErrorResponse do
|
|||||||
@behaviour MusicLibrary.ErrorResponse
|
@behaviour MusicLibrary.ErrorResponse
|
||||||
|
|
||||||
alias MusicLibrary.HttpError
|
alias MusicLibrary.HttpError
|
||||||
|
alias MusicLibrary.RetryDelay
|
||||||
|
|
||||||
@type t :: %__MODULE__{
|
@type t :: %__MODULE__{
|
||||||
status: integer() | nil,
|
status: integer() | nil,
|
||||||
message: String.t() | nil,
|
message: String.t() | nil,
|
||||||
kind: HttpError.kind(),
|
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()
|
@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__{
|
%__MODULE__{
|
||||||
status: 503,
|
status: 503,
|
||||||
message: extract_message(body),
|
message: extract_message(body),
|
||||||
kind: :rate_limit,
|
kind: :rate_limit,
|
||||||
body: body
|
body: body,
|
||||||
|
retry_delay_seconds: RetryDelay.retry_after_seconds(response)
|
||||||
}
|
}
|
||||||
end
|
end
|
||||||
|
|
||||||
def from_response(%{status: status, body: body} = _response) do
|
def from_response(%{status: status, body: body} = response) do
|
||||||
%__MODULE__{
|
%__MODULE__{
|
||||||
status: status,
|
status: status,
|
||||||
message: extract_message(body),
|
message: extract_message(body),
|
||||||
kind: HttpError.default_kind(status),
|
kind: HttpError.default_kind(status),
|
||||||
body: body
|
body: body,
|
||||||
|
retry_delay_seconds: RetryDelay.retry_after_seconds(response)
|
||||||
}
|
}
|
||||||
end
|
end
|
||||||
|
|
||||||
@@ -54,6 +58,9 @@ defmodule MusicBrainz.API.ErrorResponse do
|
|||||||
|
|
||||||
@impl MusicLibrary.ErrorResponse
|
@impl MusicLibrary.ErrorResponse
|
||||||
@spec retry_delay_seconds(t()) :: pos_integer()
|
@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: :rate_limit}), do: 60
|
||||||
def retry_delay_seconds(%__MODULE__{kind: :server_error}), 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__{kind: :timeout}), do: 10
|
||||||
|
|||||||
@@ -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
|
||||||
@@ -20,6 +20,7 @@ defmodule OpenAI.API.ErrorResponse do
|
|||||||
@behaviour MusicLibrary.ErrorResponse
|
@behaviour MusicLibrary.ErrorResponse
|
||||||
|
|
||||||
alias MusicLibrary.HttpError
|
alias MusicLibrary.HttpError
|
||||||
|
alias MusicLibrary.RetryDelay
|
||||||
|
|
||||||
@type t :: %__MODULE__{
|
@type t :: %__MODULE__{
|
||||||
status: integer() | nil,
|
status: integer() | nil,
|
||||||
@@ -27,10 +28,11 @@ defmodule OpenAI.API.ErrorResponse do
|
|||||||
type: String.t() | nil,
|
type: String.t() | nil,
|
||||||
message: String.t() | nil,
|
message: String.t() | nil,
|
||||||
kind: HttpError.kind(),
|
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()
|
@spec from_response(Req.Response.t() | map()) :: t()
|
||||||
def from_response(%{status: 429, body: %{"error" => %{"code" => "insufficient_quota"} = e}} = r) do
|
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"],
|
type: e["type"],
|
||||||
message: e["message"],
|
message: e["message"],
|
||||||
kind: :auth_error,
|
kind: :auth_error,
|
||||||
body: r.body
|
body: r.body,
|
||||||
|
retry_delay_seconds: RetryDelay.openai_reset_seconds(r)
|
||||||
}
|
}
|
||||||
end
|
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
|
when is_map(err) do
|
||||||
%__MODULE__{
|
%__MODULE__{
|
||||||
status: status,
|
status: status,
|
||||||
@@ -52,18 +55,20 @@ defmodule OpenAI.API.ErrorResponse do
|
|||||||
type: err["type"],
|
type: err["type"],
|
||||||
message: err["message"],
|
message: err["message"],
|
||||||
kind: HttpError.default_kind(status),
|
kind: HttpError.default_kind(status),
|
||||||
body: body
|
body: body,
|
||||||
|
retry_delay_seconds: RetryDelay.openai_reset_seconds(response)
|
||||||
}
|
}
|
||||||
end
|
end
|
||||||
|
|
||||||
def from_response(%{status: status, body: body} = _response) do
|
def from_response(%{status: status, body: body} = response) do
|
||||||
%__MODULE__{
|
%__MODULE__{
|
||||||
status: status,
|
status: status,
|
||||||
code: nil,
|
code: nil,
|
||||||
type: nil,
|
type: nil,
|
||||||
message: nil,
|
message: nil,
|
||||||
kind: HttpError.default_kind(status),
|
kind: HttpError.default_kind(status),
|
||||||
body: body
|
body: body,
|
||||||
|
retry_delay_seconds: RetryDelay.openai_reset_seconds(response)
|
||||||
}
|
}
|
||||||
end
|
end
|
||||||
|
|
||||||
@@ -76,6 +81,9 @@ defmodule OpenAI.API.ErrorResponse do
|
|||||||
|
|
||||||
@impl MusicLibrary.ErrorResponse
|
@impl MusicLibrary.ErrorResponse
|
||||||
@spec retry_delay_seconds(t()) :: pos_integer()
|
@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: :rate_limit}), do: 60
|
||||||
def retry_delay_seconds(%__MODULE__{kind: :server_error}), 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__{kind: :timeout}), do: 10
|
||||||
|
|||||||
@@ -123,7 +123,7 @@ defmodule Wikipedia.API do
|
|||||||
{request,
|
{request,
|
||||||
%{status: 200, body: %{"error" => %{"code" => _, "info" => _}} = body} = response}
|
%{status: 200, body: %{"error" => %{"code" => _, "info" => _}} = body} = response}
|
||||||
) do
|
) do
|
||||||
error = ErrorResponse.from_action_api_body(body)
|
error = ErrorResponse.from_action_api_body(body, response)
|
||||||
|
|
||||||
Logger.error(fn ->
|
Logger.error(fn ->
|
||||||
url = URI.to_string(request.url)
|
url = URI.to_string(request.url)
|
||||||
|
|||||||
@@ -12,8 +12,8 @@ defmodule Wikipedia.API.ErrorResponse do
|
|||||||
* **REST v1 API** (`/api/rest_v1/page/summary/:title`, used by
|
* **REST v1 API** (`/api/rest_v1/page/summary/:title`, used by
|
||||||
`get_article_summary/2`) uses classic HTTP status codes.
|
`get_article_summary/2`) uses classic HTTP status codes.
|
||||||
|
|
||||||
`from_response/1` handles both paths. `from_action_api_body/1` is a dedicated
|
`from_response/1` handles classic HTTP errors. `from_action_api_body/2` is a
|
||||||
entry point for the HTTP 200 + body-error case.
|
dedicated entry point for the HTTP 200 + body-error case.
|
||||||
|
|
||||||
## Non-error body shapes
|
## Non-error body shapes
|
||||||
|
|
||||||
@@ -32,36 +32,42 @@ defmodule Wikipedia.API.ErrorResponse do
|
|||||||
@behaviour MusicLibrary.ErrorResponse
|
@behaviour MusicLibrary.ErrorResponse
|
||||||
|
|
||||||
alias MusicLibrary.HttpError
|
alias MusicLibrary.HttpError
|
||||||
|
alias MusicLibrary.RetryDelay
|
||||||
|
|
||||||
@type t :: %__MODULE__{
|
@type t :: %__MODULE__{
|
||||||
status: integer() | nil,
|
status: integer() | nil,
|
||||||
code: String.t() | nil,
|
code: String.t() | nil,
|
||||||
message: String.t() | nil,
|
message: String.t() | nil,
|
||||||
kind: HttpError.kind(),
|
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()
|
@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__{
|
%__MODULE__{
|
||||||
status: status,
|
status: status,
|
||||||
code: extract_rest_code(body),
|
code: extract_rest_code(body),
|
||||||
message: extract_rest_message(body),
|
message: extract_rest_message(body),
|
||||||
kind: HttpError.default_kind(status),
|
kind: HttpError.default_kind(status),
|
||||||
body: body
|
body: body,
|
||||||
|
retry_delay_seconds: RetryDelay.retry_after_seconds(response)
|
||||||
}
|
}
|
||||||
end
|
end
|
||||||
|
|
||||||
@spec from_action_api_body(map()) :: t()
|
@spec from_action_api_body(map(), Req.Response.t() | map()) :: t()
|
||||||
def from_action_api_body(%{"error" => %{"code" => code, "info" => info}} = body) do
|
def from_action_api_body(body, response \\ %{})
|
||||||
|
|
||||||
|
def from_action_api_body(%{"error" => %{"code" => code, "info" => info}} = body, response) do
|
||||||
%__MODULE__{
|
%__MODULE__{
|
||||||
status: 200,
|
status: 200,
|
||||||
code: code,
|
code: code,
|
||||||
message: info,
|
message: info,
|
||||||
kind: action_api_kind(code),
|
kind: action_api_kind(code),
|
||||||
body: body
|
body: body,
|
||||||
|
retry_delay_seconds: RetryDelay.retry_after_seconds(response)
|
||||||
}
|
}
|
||||||
end
|
end
|
||||||
|
|
||||||
@@ -74,6 +80,9 @@ defmodule Wikipedia.API.ErrorResponse do
|
|||||||
|
|
||||||
@impl MusicLibrary.ErrorResponse
|
@impl MusicLibrary.ErrorResponse
|
||||||
@spec retry_delay_seconds(t()) :: pos_integer()
|
@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: :rate_limit}), do: 30
|
||||||
def retry_delay_seconds(%__MODULE__{kind: :server_error}), 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__{kind: :timeout}), do: 10
|
||||||
|
|||||||
@@ -57,6 +57,7 @@ defmodule BraveSearch.APITest do
|
|||||||
Req.Test.stub(__MODULE__, fn conn ->
|
Req.Test.stub(__MODULE__, fn conn ->
|
||||||
conn
|
conn
|
||||||
|> Plug.Conn.put_status(429)
|
|> Plug.Conn.put_status(429)
|
||||||
|
|> Plug.Conn.put_resp_header("x-ratelimit-reset", "12, 60")
|
||||||
|> Req.Test.json(%{
|
|> Req.Test.json(%{
|
||||||
"type" => "ErrorResponse",
|
"type" => "ErrorResponse",
|
||||||
"error" => %{"status" => 429, "code" => "RATE_LIMITED", "detail" => "Too many requests"}
|
"error" => %{"status" => 429, "code" => "RATE_LIMITED", "detail" => "Too many requests"}
|
||||||
@@ -69,6 +70,8 @@ defmodule BraveSearch.APITest do
|
|||||||
assert err.status == 429
|
assert err.status == 429
|
||||||
assert err.code == "RATE_LIMITED"
|
assert err.code == "RATE_LIMITED"
|
||||||
assert err.kind == :rate_limit
|
assert err.kind == :rate_limit
|
||||||
|
assert err.retry_delay_seconds == 60
|
||||||
|
assert ErrorResponse.retry_delay_seconds(err) == 60
|
||||||
assert ErrorResponse.retryable?(err)
|
assert ErrorResponse.retryable?(err)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|||||||
@@ -15,6 +15,7 @@ defmodule MusicBrainz.APITest do
|
|||||||
Req.Test.stub(__MODULE__, fn conn ->
|
Req.Test.stub(__MODULE__, fn conn ->
|
||||||
conn
|
conn
|
||||||
|> Plug.Conn.put_status(503)
|
|> 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."})
|
|> Req.Test.json(%{"error" => "Your requests are exceeding the allowable rate limit."})
|
||||||
end)
|
end)
|
||||||
|
|
||||||
@@ -23,6 +24,8 @@ defmodule MusicBrainz.APITest do
|
|||||||
|
|
||||||
assert err.status == 503
|
assert err.status == 503
|
||||||
assert err.kind == :rate_limit
|
assert err.kind == :rate_limit
|
||||||
|
assert err.retry_delay_seconds == 42
|
||||||
|
assert API.ErrorResponse.retry_delay_seconds(err) == 42
|
||||||
assert API.ErrorResponse.retryable?(err)
|
assert API.ErrorResponse.retryable?(err)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|||||||
@@ -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
|
||||||
@@ -22,6 +22,19 @@ defmodule MusicLibrary.Worker.ErrorHandlerTest do
|
|||||||
assert ErrorHandler.to_oban_result({:error, err}) == {:snooze, 60}
|
assert ErrorHandler.to_oban_result({:error, err}) == {:snooze, 60}
|
||||||
end
|
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
|
test "Discogs 429 → {:snooze, 60}" do
|
||||||
err = Discogs.API.ErrorResponse.from_response(%{status: 429, body: %{"message" => "rate"}})
|
err = Discogs.API.ErrorResponse.from_response(%{status: 429, body: %{"message" => "rate"}})
|
||||||
assert ErrorHandler.to_oban_result({:error, err}) == {:snooze, 60}
|
assert ErrorHandler.to_oban_result({:error, err}) == {:snooze, 60}
|
||||||
|
|||||||
@@ -31,6 +31,9 @@ defmodule OpenAI.APITest do
|
|||||||
Req.Test.stub(__MODULE__, fn conn ->
|
Req.Test.stub(__MODULE__, fn conn ->
|
||||||
conn
|
conn
|
||||||
|> Plug.Conn.put_status(429)
|
|> 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(%{
|
|> Req.Test.json(%{
|
||||||
"error" => %{
|
"error" => %{
|
||||||
"code" => "rate_limit_exceeded",
|
"code" => "rate_limit_exceeded",
|
||||||
@@ -46,6 +49,8 @@ defmodule OpenAI.APITest do
|
|||||||
assert err.status == 429
|
assert err.status == 429
|
||||||
assert err.code == "rate_limit_exceeded"
|
assert err.code == "rate_limit_exceeded"
|
||||||
assert err.kind == :rate_limit
|
assert err.kind == :rate_limit
|
||||||
|
assert err.retry_delay_seconds == 90
|
||||||
|
assert ErrorResponse.retry_delay_seconds(err) == 90
|
||||||
assert ErrorResponse.retryable?(err)
|
assert ErrorResponse.retryable?(err)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|||||||
@@ -14,7 +14,9 @@ defmodule Wikipedia.APITest do
|
|||||||
@tag :capture_log
|
@tag :capture_log
|
||||||
test "promotes ratelimited Action API error into a retryable ErrorResponse" do
|
test "promotes ratelimited Action API error into a retryable ErrorResponse" do
|
||||||
Req.Test.stub(API, fn conn ->
|
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."}
|
"error" => %{"code" => "ratelimited", "info" => "You've exceeded your rate limit."}
|
||||||
})
|
})
|
||||||
end)
|
end)
|
||||||
@@ -23,6 +25,8 @@ defmodule Wikipedia.APITest do
|
|||||||
assert err.status == 200
|
assert err.status == 200
|
||||||
assert err.code == "ratelimited"
|
assert err.code == "ratelimited"
|
||||||
assert err.kind == :rate_limit
|
assert err.kind == :rate_limit
|
||||||
|
assert err.retry_delay_seconds == 42
|
||||||
|
assert ErrorResponse.retry_delay_seconds(err) == 42
|
||||||
assert ErrorResponse.retryable?(err)
|
assert ErrorResponse.retryable?(err)
|
||||||
end
|
end
|
||||||
|
|
||||||
@@ -148,6 +152,7 @@ defmodule Wikipedia.APITest do
|
|||||||
Req.Test.stub(API, fn conn ->
|
Req.Test.stub(API, fn conn ->
|
||||||
conn
|
conn
|
||||||
|> Plug.Conn.put_status(429)
|
|> Plug.Conn.put_status(429)
|
||||||
|
|> Plug.Conn.put_resp_header("retry-after", "25")
|
||||||
|> Req.Test.json(%{
|
|> Req.Test.json(%{
|
||||||
"httpCode" => 429,
|
"httpCode" => 429,
|
||||||
"httpReason" => "Too Many Requests",
|
"httpReason" => "Too Many Requests",
|
||||||
@@ -158,6 +163,8 @@ defmodule Wikipedia.APITest do
|
|||||||
assert {:error, %ErrorResponse{status: 429, kind: :rate_limit} = err} =
|
assert {:error, %ErrorResponse{status: 429, kind: :rate_limit} = err} =
|
||||||
API.get_article_summary("Steven%20Wilson", @config)
|
API.get_article_summary("Steven%20Wilson", @config)
|
||||||
|
|
||||||
|
assert err.retry_delay_seconds == 25
|
||||||
|
assert ErrorResponse.retry_delay_seconds(err) == 25
|
||||||
assert ErrorResponse.retryable?(err)
|
assert ErrorResponse.retryable?(err)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|||||||
Reference in New Issue
Block a user