fix: never retry non-idempotent requests after ambiguous failures

lib/forem/api_requestor.rb

@@ -22,6 +22,12 @@ module Forem
   #   requestor = Forem::APIRequestor.new(config: config)
   #   requestor.request(:get, "/api/articles")
   class APIRequestor
+    # HTTP methods that RFC 9110 defines as idempotent: sending the same
+    # request twice has the same effect on the server as sending it once.
+    # Replaying these after a failure is safe. POST and PATCH are absent
+    # deliberately — a retried POST can create a second resource.
+    IDEMPOTENT_METHODS = %i[get head put delete options trace].freeze
+
     # Create a new APIRequestor.
     #
     # @param config [Configuration] the configuration to use for this
@@ -42,6 +48,13 @@ def initialize(config:)
     # retries honor integer +Retry-After+ seconds; other retries use
     # exponential back-off.
     #
+    # Retries are constrained by idempotency. A network error or a server 5xx
+    # leaves it unknown whether the server processed the request, so those are
+    # replayed only for {IDEMPOTENT_METHODS}; a POST is not retried and the
+    # error is raised to the caller. A 429 is always retried, for any method,
+    # because the server rejected the request without processing it. Override
+    # per request with +opts[:idempotent]+.
+    #
     # @param method [Symbol] the HTTP verb — +:get+, +:post+, +:put+, or
     #   +:delete+.
     # @param path [String] the API path relative to {Configuration#api_base}
@@ -53,6 +66,10 @@ def initialize(config:)
     # @option opts [String] :api_base override the base URL for this request.
     # @option opts [APIRequestor] :requestor an alternative requestor to use
     #   (consumed by higher-level helpers before reaching this method).
+    # @option opts [Boolean] :idempotent override whether replaying this
+    #   request is safe. Defaults to +true+ for {IDEMPOTENT_METHODS} and
+    #   +false+ for POST/PATCH. Set it to +true+ on a POST only when the
+    #   endpoint deduplicates server-side (e.g. an idempotency key).
     # @return [ForemResponse] the parsed response wrapper.
     # @raise [AuthenticationError] on HTTP 401.
     # @raise [AuthorizationError] on HTTP 403.
@@ -76,20 +93,26 @@ def request(method, path, params = {}, opts = {})
       extra_headers = opts.delete(:headers) || {}
       uri = URI("#{api_base}#{path}")
 
+      idempotent = opts.delete(:idempotent)
+      idempotent = IDEMPOTENT_METHODS.include?(method) if idempotent.nil?
+
       retries_left = @config.max_network_retries
       begin
         response = execute_request(method, uri, params, api_key, extra_headers)
         handle_error_response(response) if response.http_status >= 400
         response
       rescue Forem::APIConnectionError
-        if retries_left > 0
+        # A network failure is ambiguous: the request may have reached the
+        # server and been processed before the connection broke. Only replay
+        # it when doing so cannot create a second resource.
+        if retries_left > 0 && idempotent
           retries_left -= 1
           sleep backoff_duration(@config.max_network_retries - retries_left)
           retry
         end
         raise
       rescue Forem::RateLimitError, Forem::APIError => e
-        if retries_left > 0 && retryable_error?(e)
+        if retries_left > 0 && retryable_error?(e, idempotent: idempotent)
           retries_left -= 1
           retry_count = @config.max_network_retries - retries_left
           sleep retry_delay(e, retry_count)
@@ -241,15 +264,21 @@ def headers_to_hash(http_response)
 
     # Determine whether a given error is eligible for an automatic retry.
     #
-    # {RateLimitError} is always retryable. {APIError} is retryable only when
-    # the HTTP status is 500 or greater (server errors).
+    # {RateLimitError} is always retryable, including for non-idempotent
+    # methods: a 429 means the server rejected the request without processing
+    # it, so replaying cannot duplicate anything.
+    #
+    # A server {APIError} (5xx) is ambiguous — the request may have been
+    # processed before the failure — so it is retried only for idempotent
+    # methods.
     #
     # @param error [ForemError] the error to evaluate.
+    # @param idempotent [Boolean] whether replaying this request is safe.
     # @return [Boolean] +true+ if the request should be retried.
-    def retryable_error?(error)
+    def retryable_error?(error, idempotent:)
       case error
       when RateLimitError then true
-      when APIError then error.http_status >= 500
+      when APIError then idempotent && error.http_status >= 500
       else false
       end
     end

test/forem/api_requestor_test.rb

@@ -169,6 +169,71 @@ def test_rate_limit_retry_with_negative_retry_after_uses_jittered_backoff
     connection_manager.verify
   end
 
+  def test_post_is_not_retried_after_a_server_error
+    requestor, attempts, = counting_requestor(
+      http_response(status: 500, body: '{"error":"boom"}'),
+      http_response(status: 201, body: '{"id":1}')
+    )
+
+    assert_raises(Forem::APIError) { requestor.request(:post, "/api/articles", { title: "Hi" }) }
+    assert_equal 1, attempts.length, "a 5xx may already have been processed; replaying a POST can duplicate it"
+  end
+
+  def test_post_is_not_retried_after_a_connection_error
+    requestor, attempts, = counting_requestor(
+      Net::ReadTimeout.new,
+      http_response(status: 201, body: '{"id":1}')
+    )
+
+    assert_raises(Forem::APIConnectionError) { requestor.request(:post, "/api/articles", { title: "Hi" }) }
+    assert_equal 1, attempts.length, "a timed-out POST may have been processed by the server"
+  end
+
+  def test_put_is_still_retried_after_a_server_error
+    requestor, attempts, = counting_requestor(
+      http_response(status: 500, body: '{"error":"boom"}'),
+      http_response(status: 200, body: '{"id":1}')
+    )
+
+    response = requestor.request(:put, "/api/articles/1", { title: "Hi" })
+
+    assert_equal 200, response.http_status
+    assert_equal 2, attempts.length, "PUT is idempotent, so replaying is safe"
+  end
+
+  def test_post_is_retried_after_a_rate_limit
+    requestor, attempts, = counting_requestor(
+      http_response(status: 429, body: '{"error":"rate limited"}', headers: { "Retry-After" => "1" }),
+      http_response(status: 201, body: '{"id":1}')
+    )
+
+    response = requestor.request(:post, "/api/articles", { title: "Hi" })
+
+    assert_equal 201, response.http_status
+    assert_equal 2, attempts.length, "a 429 is rejected without processing, so replaying cannot duplicate"
+  end
+
+  def test_post_is_retried_when_explicitly_marked_idempotent
+    requestor, attempts, = counting_requestor(
+      http_response(status: 500, body: '{"error":"boom"}'),
+      http_response(status: 201, body: '{"id":1}')
+    )
+
+    response = requestor.request(:post, "/api/articles", { title: "Hi" }, { idempotent: true })
+
+    assert_equal 201, response.http_status
+    assert_equal 2, attempts.length
+  end
+
+  def test_idempotent_option_is_not_sent_as_a_header
+    mock_http, captured = stub_http_request(method: :post, path: "/api/articles", status: 201, body: '{"id":1}')
+    requestor = make_requestor(mock_http)
+
+    requestor.request(:post, "/api/articles", { title: "Hi" }, { idempotent: true })
+
+    refute captured[:headers].key?("idempotent")
+  end
+
   def test_per_request_api_key_override
     mock_http, captured = stub_http_request(method: :get, path: "/api/articles", status: 200, body: '[]')
     requestor = make_requestor(mock_http)
@@ -178,6 +243,32 @@ def test_per_request_api_key_override
 
   private
 
+  # Like retrying_requestor, but tolerates the request NOT being replayed and
+  # reports how many attempts actually reached the transport. A queued entry
+  # that is an Exception is raised instead of returned.
+  def counting_requestor(*responses, retries: 1)
+    queued = responses.dup
+    attempts = []
+    http = Object.new
+    http.define_singleton_method(:request) do |request|
+      attempts << request
+      nxt = queued.shift
+      raise nxt if nxt.is_a?(Exception)
+
+      nxt
+    end
+
+    @config.max_network_retries = retries
+    requestor = Forem::APIRequestor.new(config: @config)
+    connection_manager = Object.new
+    connection_manager.define_singleton_method(:connection_for) { |_uri, **_kwargs| http }
+    requestor.instance_variable_set(:@connection_manager, connection_manager)
+
+    sleeps = []
+    requestor.define_singleton_method(:sleep) { |seconds| sleeps << seconds }
+    [requestor, attempts, sleeps]
+  end
+
   def retrying_requestor(*responses)
     queued_responses = responses.dup
     http = Object.new