diff --git a/lib/dexcord/api.ex b/lib/dexcord/api.ex index 5563c74..99bbc46 100644 --- a/lib/dexcord/api.ex +++ b/lib/dexcord/api.ex @@ -11,10 +11,7 @@ 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. 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. + up to #{3} times. All bodies are string-keyed maps; JSON encode/decode uses the built-in `JSON` module. Returns `{:ok, map}`, `{:ok, nil}` (204 / empty 2xx body), @@ -48,16 +45,6 @@ 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 @@ -85,9 +72,6 @@ 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(), @@ -134,9 +118,6 @@ 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 @@ -160,41 +141,11 @@ 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, transport_error(reason)}} + {:done, {:error, %Error{status: nil, code: nil, message: inspect(reason), errors: nil}}} 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 cb0cc84..2f97179 100644 --- a/lib/dexcord/supervisor.ex +++ b/lib/dexcord/supervisor.ex @@ -53,9 +53,7 @@ defmodule Dexcord.Supervisor do [ Dexcord.Session, cache_and_dispatcher(), - # 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]}}, + {Finch, name: Dexcord.Finch}, 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 fa842b2..3934804 100644 --- a/test/dexcord/api_integration_test.exs +++ b/test/dexcord/api_integration_test.exs @@ -143,37 +143,6 @@ 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 1b256fb..d35183f 100644 --- a/test/support/fake_rest.ex +++ b/test/support/fake_rest.ex @@ -49,12 +49,6 @@ 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}) @@ -134,24 +128,11 @@ defmodule Dexcord.FakeRest.Plug do body: body } - case Dexcord.FakeRest.handle_request(info) do - :close -> - abort_connection(conn) + response = Dexcord.FakeRest.handle_request(info) - 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 + conn + |> put_headers(response.headers) + |> send_resp(response.status, response.body) end defp put_headers(conn, headers) do