From 6d446b117b435702d67da2157f8eef572cacdd9b Mon Sep 17 00:00:00 2001 From: Luna Date: Mon, 3 Aug 2026 21:30:07 -0300 Subject: [PATCH 1/2] retry request on connection faults --- lib/dexcord/api.ex | 53 ++++++++++++++++++++++++++- lib/dexcord/supervisor.ex | 4 +- test/dexcord/api_integration_test.exs | 31 ++++++++++++++++ test/support/fake_rest.ex | 27 ++++++++++++-- 4 files changed, 108 insertions(+), 7 deletions(-) diff --git a/lib/dexcord/api.ex b/lib/dexcord/api.ex index 99bbc46..5563c74 100644 --- a/lib/dexcord/api.ex +++ b/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) diff --git a/lib/dexcord/supervisor.ex b/lib/dexcord/supervisor.ex index 2f97179..cb0cc84 100644 --- a/lib/dexcord/supervisor.ex +++ b/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} ] ++ diff --git a/test/dexcord/api_integration_test.exs b/test/dexcord/api_integration_test.exs index 3934804..fa842b2 100644 --- a/test/dexcord/api_integration_test.exs +++ b/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}) diff --git a/test/support/fake_rest.ex b/test/support/fake_rest.ex index d35183f..1b256fb 100644 --- a/test/support/fake_rest.ex +++ b/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 From 854f202f760370d7836fb3c1aaa62b2dab79e9d3 Mon Sep 17 00:00:00 2001 From: Luna Date: Wed, 7 Oct 2026 16:03:15 -0300 Subject: [PATCH 2/2] Fix after-cursor message history pagination --- lib/dexcord/api.ex | 11 +++++++---- test/dexcord/pagination_test.exs | 30 +++++++++++++++++++++++++++--- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/lib/dexcord/api.ex b/lib/dexcord/api.ex index 5563c74..9bfdb77 100644 --- a/lib/dexcord/api.ex +++ b/lib/dexcord/api.ex @@ -531,10 +531,13 @@ defmodule Dexcord.Api do after_id, @history_page_size, fn cursor -> - Dexcord.Api.Messages.get_channel_messages(channel_id, - limit: @history_page_size, - after: cursor - ) + case Dexcord.Api.Messages.get_channel_messages(channel_id, + limit: @history_page_size, + after: cursor + ) do + {:ok, messages} -> {:ok, Enum.reverse(messages)} + {:error, _} = error -> error + end end, fn last -> last.id end ) diff --git a/test/dexcord/pagination_test.exs b/test/dexcord/pagination_test.exs index 85d63dd..ee09dba 100644 --- a/test/dexcord/pagination_test.exs +++ b/test/dexcord/pagination_test.exs @@ -89,8 +89,8 @@ defmodule Dexcord.PaginationTest do end test "after: flips to ascending paging with after= cursor" do - page1_ids = Enum.to_list(101..200) - page2_ids = [201, 202, 203] + page1_ids = Enum.to_list(200..101//-1) + page2_ids = [203, 202, 201] assert length(page1_ids) == 100 FakeRest.stub( @@ -107,7 +107,7 @@ defmodule Dexcord.PaginationTest do result = Api.message_history(2, after: 100) |> Enum.to_list() - assert Enum.map(result, & &1.id) == page1_ids ++ page2_ids + assert Enum.map(result, & &1.id) == Enum.to_list(101..203) assert_receive {:rest_hit, %{path: "/channels/2/messages", query_string: q1}} assert q1 =~ "after=100" @@ -116,6 +116,30 @@ defmodule Dexcord.PaginationTest do refute q2 =~ "before=" end + test "after: advances past each full descending page without overlapping messages" do + # Discord returns each after page newest first. Using its last wire item + # as the cursor would request overlapping messages on the next page. + for ids <- [200..101//-1, 300..201//-1, 350..301//-1] do + FakeRest.stub( + :get, + "/channels/4/messages", + FakeRest.resp(200, body: messages_json(ids)) + ) + end + + ids = Api.message_history(4, after: 100) |> Enum.map(& &1.id) + + assert ids == Enum.to_list(101..350) + assert length(Enum.uniq(ids)) == 250 + + for cursor <- [100, 200, 300] do + assert_receive {:rest_hit, %{path: "/channels/4/messages", query_string: query}} + assert URI.decode_query(query) == %{"after" => to_string(cursor), "limit" => "100"} + end + + refute_receive {:rest_hit, %{path: "/channels/4/messages"}}, 50 + end + test "limit: caps the total number of messages via Stream.take" do full_page = Enum.to_list(300..201//-1)