`:retry_after` ignored if there is a Retry-After response header

[dmarkow]

The documentation for :retry_delay says:

if not set, which is the default, the retry delay is determined by the value of the Retry-After header on HTTP 429/503 responses. If the header is not set, the default delay follows a simple exponential backoff: 1s, 2s, 4s, 8s, ...

This makes it seem like the priority would be:

• Use the configured :retry_delay option
• If not provided, use the Retry-After header
• If there's no header, use the backoff strategy

But it seems that it's really:

• If the Retry-After header is present, use it
• If there's no header, use the :retry_delay option
• If that's not set, use the backoff strategy

req/lib/req/steps.ex

Lines 2210 to 2238 in ba4de2a

	defp get_retry_delay(request, %Req.Response{status: status} = response, retry_count)
	when status in [429, 503] do
	if delay = Req.Response.get_retry_after(response) do
	{request, delay}
	else
	calculate_retry_delay(request, retry_count)
	end
	end
	defp get_retry_delay(request, _response, retry_count) do
	calculate_retry_delay(request, retry_count)
	end
	defp calculate_retry_delay(request, retry_count) do
	case Req.Request.get_option(request, :retry_delay, &exp_backoff/1) do
	delay when is_integer(delay) ->
	{request, delay}
	fun when is_function(fun, 1) ->
	case fun.(retry_count) do
	delay when is_integer(delay) and delay >= 0 ->
	{request, delay}
	other ->
	raise ArgumentError,
	"expected :retry_delay function to return non-negative integer, got: #{inspect(other)}"
	end
	end
	end

I'm unsure if this is a bug and it should always use the :retry_after option if it's set, or if it's a documentation fix to clarify that :retry_after is ignored when there's a Retry-After header.

[wojtekmach]

@dmarkow I'm so sorry this issue has fallen through the cracks. I agree the behavior does not match the docs and the docs imply:

Use the configured :retry_delay option
If not provided, use the Retry-After header
If there's no header, use the backoff strategy

And I think that's how it should work. For example, we have a https://hexdocs.pm/req/Req.Response.html#get_retry_after/1 so one could do something like this:

retry_delay: fn attempt ->
  server_delay = Req.Response.get_retry_after(resp) || 0
  backoff_delay = (2 ** attempt) * 1000
  max_reasonable = 60_000 # 1 minute cap
  
  min(max(server_delay, backoff_delay), max_reasonable)
end)

(though not sure how realistic it is for people actually wanting to do that)

Do you have a preference how this should behave?