retry request on connection faults

Reviewed-on: #1

lib/dexcord/api.ex

@@ -11,7 +11,10 @@ defmodule Dexcord.Api do
   flows through the limiter: acquire a token (sleeping in the caller's process
   while `{:wait, ms}`), issue the request, feed the response headers back for
   bucket learning, and on a 429 honor `retry_after` / the global scope and retry
-  up to #{3} times.
+  up to #{3} times. Connection-level transport faults (`%Mint.TransportError{}`,
+  e.g. a stale pooled keep-alive the server already closed) share that retry
+  budget with a short backoff; timeouts and pool errors are NOT retried, since
+  a timed-out request may have been processed and retrying could double-post.
 
   All bodies are string-keyed maps; JSON encode/decode uses the built-in `JSON`
   module. Returns `{:ok, map}`, `{:ok, nil}` (204 / empty 2xx body),
@@ -45,6 +48,16 @@ defmodule Dexcord.Api do
   @default_base_url "https://discord.com/api/v10"
   @user_agent "DiscordBot (https://github.com/luna/dexcord, 0.1.0)"
   @max_retries 3
+  # Backoff (ms) before transport-error retry N. The first retry is immediate:
+  # the dominant fault is a stale pooled keep-alive, and the retry opens a
+  # fresh connection anyway.
+  @transport_retry_ms {0, 250, 1000}
+  # Connection-fault reasons safe to retry: the request (almost certainly)
+  # never reached the server. Deliberately excludes :timeout — a timed-out
+  # request may have been fully processed, and retrying it can double-post.
+  # Finch wraps Mint's error as `%Finch.TransportError{reason: ..., source: ...}`
+  # with the same reason atoms.
+  @retryable_transport_reasons [:closed, :econnrefused, :econnreset, :epipe]
   # Small cushion added to every computed sleep so we wake just after a window
   # or retry-after boundary rather than a hair before it.
   @wait_padding_ms 20
@@ -72,6 +85,9 @@ defmodule Dexcord.Api do
       rate-limit deadline (`status: nil`, message `"rate limit deadline exceeded"`)
       when the internal waits would exceed `:api_deadline_ms`. For a non-JSON
       error body a bounded snippet of the raw body is kept in `:message`.
+      Connection-level `%Mint.TransportError{}` faults are retried up to
+      #{3} times before surfacing here (`status: nil`, message is the
+      inspected reason).
   """
   @spec request(
           atom(),
@@ -118,6 +134,9 @@ defmodule Dexcord.Api do
       {:retry_429, headers, resp_body} ->
         handle_429(method, path, body, opts, route, attempt, headers, resp_body, deadline)
 
+      {:retry_transport, reason} ->
+        handle_transport_error(method, path, body, opts, route, attempt, reason, deadline)
+
       {:done, done} ->
         done
     end
@@ -141,11 +160,41 @@ defmodule Dexcord.Api do
         Ratelimit.update(route, resp_headers)
         {:done, {:error, error(resp)}}
 
+      {:error, %struct{reason: r} = reason}
+      when struct in [Finch.TransportError, Mint.TransportError] and
+             r in @retryable_transport_reasons ->
+        {:retry_transport, reason}
+
       {:error, reason} ->
-        {:done, {:error, %Error{status: nil, code: nil, message: inspect(reason), errors: nil}}}
+        {:done, {:error, transport_error(reason)}}
     end
   end
 
+  # Connection-level faults (a stale pooled keep-alive the server already
+  # closed, a refused/reset connection) almost always mean the request never
+  # reached Discord, so a bounded retry is safe. Timeouts and pool errors are
+  # NOT retried: a timed-out POST may have been fully processed, and retrying
+  # it can double-post.
+  defp handle_transport_error(method, path, body, opts, route, attempt, _reason, deadline)
+       when attempt < @max_retries do
+    backoff = elem(@transport_retry_ms, min(attempt, tuple_size(@transport_retry_ms) - 1))
+
+    if mono_now() + backoff >= deadline do
+      deadline_error()
+    else
+      if backoff > 0, do: Process.sleep(backoff)
+      do_request(method, path, body, opts, route, attempt + 1, deadline)
+    end
+  end
+
+  defp handle_transport_error(_method, _path, _body, _opts, _route, _attempt, reason, _deadline) do
+    {:error, transport_error(reason)}
+  end
+
+  defp transport_error(reason) do
+    %Error{status: nil, code: nil, message: inspect(reason), errors: nil}
+  end
+
   defp handle_429(method, path, body, opts, route, attempt, headers, resp_body, deadline)
        when attempt < @max_retries do
     retry_ms = retry_after_ms(resp_body, headers)

lib/dexcord/supervisor.ex

@@ -53,7 +53,9 @@ defmodule Dexcord.Supervisor do
       [
         Dexcord.Session,
         cache_and_dispatcher(),
-        {Finch, name: Dexcord.Finch},
+        # conn_max_idle_time keeps pooled keep-alives younger than Cloudflare's
+        # idle reap, so requests rarely check out an already-closed connection.
+        {Finch, name: Dexcord.Finch, pools: %{default: [conn_max_idle_time: 45_000]}},
         Dexcord.Api.Ratelimit,
         {Task.Supervisor, name: Dexcord.TaskSupervisor}
       ] ++

test/dexcord/api_integration_test.exs

@@ -143,6 +143,37 @@ defmodule Dexcord.ApiIntegrationTest do
     assert elapsed_us >= 200_000
   end
 
+  test "a closed connection is retried and then succeeds" do
+    FakeRest.stub(:post, "/channels/5/messages", FakeRest.close())
+
+    FakeRest.stub(
+      :post,
+      "/channels/5/messages",
+      ok_json(~s({"id":"ok"}), bucket: "T", remaining: 5, reset_after: 1.0)
+    )
+
+    assert {:ok, %Dexcord.Message{id: "ok"}} = Api.create_message(5, "hi")
+
+    # Exactly two hits: the aborted attempt and the successful retry.
+    assert_receive {:rest_hit, _}
+    assert_receive {:rest_hit, _}
+    refute_receive {:rest_hit, _}, 50
+  end
+
+  test "transport errors give up after max retries and surface the reason" do
+    # Sticky stub: every attempt gets its connection slammed shut.
+    FakeRest.stub(:post, "/channels/5/messages", FakeRest.close())
+
+    assert {:error, %Error{status: nil, code: nil, message: message}} =
+             Api.create_message(5, "hi")
+
+    assert message =~ ":closed"
+
+    # Initial attempt + 3 retries (backoffs up to 1s), then no more.
+    for _ <- 1..4, do: assert_receive({:rest_hit, _}, 2_000)
+    refute_receive {:rest_hit, _}, 50
+  end
+
   test "a global 429 blocks an unrelated route until the lock expires" do
     global_429 = ~s({"message":"global rate limit","retry_after":0.4,"global":true})
 

test/support/fake_rest.ex

@@ -49,6 +49,12 @@ defmodule Dexcord.FakeRest do
     }
   end
 
+  @doc """
+  A response marker that abruptly closes the TCP connection instead of
+  responding, so the client observes `%Mint.TransportError{reason: :closed}`.
+  """
+  def close, do: :close
+
   @doc false
   # Called by the plug (in the Bandit connection process).
   def handle_request(info), do: GenServer.call(__MODULE__, {:handle, info})
@@ -128,11 +134,24 @@ defmodule Dexcord.FakeRest.Plug do
       body: body
     }
 
-    response = Dexcord.FakeRest.handle_request(info)
+    case Dexcord.FakeRest.handle_request(info) do
+      :close ->
+        abort_connection(conn)
 
-    conn
-    |> put_headers(response.headers)
-    |> send_resp(response.status, response.body)
+      response ->
+        conn
+        |> put_headers(response.headers)
+        |> send_resp(response.status, response.body)
+    end
+  end
+
+  # Slams the TCP connection shut without a response. The raise gets Bandit off
+  # this request; `Plug.BadRequestError`'s 400 status keeps it outside Bandit's
+  # default 500..599 exception-logging range, so the deliberate abort is silent.
+  defp abort_connection(conn) do
+    {Bandit.Adapter, adapter} = conn.adapter
+    ThousandIsland.Socket.close(adapter.transport.socket)
+    raise Plug.BadRequestError
   end
 
   defp put_headers(conn, headers) do