From 671cdce2351496276f0e4ba061620098106b548e Mon Sep 17 00:00:00 2001 From: lineoffligbot <4628864+lineoffligbot@users.noreply.github.com> Date: Sun, 16 Aug 2026 15:36:18 +0200 Subject: [PATCH] Make raise_error work with retriable feature raise_error converts error responses into StatusError inside each attempt, blocking retry_statuses matching from seeing the response. This prevents retries and makes retry_statuses dead configuration when both features are enabled. Recover the response carried by StatusError in try_request, so retry_statuses matching, delay calculation, callbacks, and OutOfRetriesError all see it. Treat StatusError with matching status as retriable in retry_request?. Non-matching StatusErrors still raise immediately. Principle: raise-on-status should act only after retry-on-status is exhausted, but raise_error runs per-attempt inside the retry loop. Co-authored-by: Hakan Ensari --- CHANGELOG.md | 7 ++ lib/http/retriable/performer.rb | 16 ++- sig/http.rbs | 1 + test/http/retriable/performer_test.rb | 146 ++++++++++++++++++++++++++ 4 files changed, 169 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 954aa1ae..ba0e83be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 JVM could garbage-collect llhttp's native callback trampolines while a parser was still in use, after which `llhttp_execute` succeeded without invoking any callbacks. Callback procs are now retained for the lifetime of the parser. +- The `raise_error` and `retriable` features now compose. Previously + `raise_error` converted an error response into a `StatusError` inside each + attempt, so `retry_statuses` never saw the response and no retries happened. + The retry performer now recovers the response carried by a `StatusError`, + so status-based retries, `Retry-After` delay calculation, `on_retry`, and + `OutOfRetriesError#response` all see it. ([#848]) ## [6.0.4] - 2026-07-14 @@ -330,6 +336,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#785]: https://github.com/httprb/http/issues/785 [#826]: https://github.com/httprb/http/issues/826 [#841]: https://github.com/httprb/http/pull/841 +[#848]: https://github.com/httprb/http/pull/848 [unreleased]: https://github.com/httprb/http/compare/v6.0.4...HEAD [6.0.4]: https://github.com/httprb/http/compare/v6.0.3...v6.0.4 [6.0.3]: https://github.com/httprb/http/compare/v6.0.2...v6.0.3 diff --git a/lib/http/retriable/performer.rb b/lib/http/retriable/performer.rb index ec6de21f..445c1cba 100644 --- a/lib/http/retriable/performer.rb +++ b/lib/http/retriable/performer.rb @@ -113,6 +113,11 @@ def try_request res = yield rescue Exception => e err = e + # The raise_error feature converts an error response into a + # StatusError inside the attempt; recover the response so + # retry_statuses, delay calculation and OutOfRetriesError + # still see it. + res = e.response if e.is_a?(StatusError) end [err, res] @@ -127,7 +132,7 @@ def retry_request?(req, err, res, attempt) if @should_retry_proc @should_retry_proc.call(req, err, res, attempt) elsif err - retry_exception?(err) + retry_exception?(err) || retriable_status_error?(err) else retry_response?(res) end @@ -141,6 +146,15 @@ def retry_exception?(err) @exception_classes.any? { |e| err.is_a?(e) } end + # Checks whether the error carries a response that warrants retry + # + # @param [Exception] err + # @api private + # @return [Boolean] + def retriable_status_error?(err) + err.is_a?(StatusError) && retry_response?(err.response) + end + # Checks whether the response status warrants retry # # @api private diff --git a/sig/http.rbs b/sig/http.rbs index 9b1c4a69..75df86aa 100644 --- a/sig/http.rbs +++ b/sig/http.rbs @@ -1601,6 +1601,7 @@ module HTTP def try_request: () { () -> Response? } -> [Exception?, Response?] def retry_request?: (Request req, Exception? err, Response? res, Integer attempt) -> bool def retry_exception?: (Exception err) -> bool + def retriable_status_error?: (Exception err) -> bool def retry_response?: (untyped res) -> bool def wait_for_retry_or_raise: (Request req, Exception? err, Response? res, Integer attempt) -> void def out_of_retries_error: (Request request, Response? response, Exception? exception) -> OutOfRetriesError diff --git a/test/http/retriable/performer_test.rb b/test/http/retriable/performer_test.rb index 3f7b84c3..5b9f07b3 100644 --- a/test/http/retriable/performer_test.rb +++ b/test/http/retriable/performer_test.rb @@ -14,6 +14,12 @@ class CustomSubException < HTTP::TimeoutError end end +# Subclass for testing is_a? vs instance_of? in StatusError handling +unless defined?(CustomStatusError) + class CustomStatusError < HTTP::StatusError + end +end + class HTTPRetriablePerformerTest < Minitest::Test cover "HTTP::Retriable::Performer*" @@ -196,6 +202,146 @@ def test_perform_unexpected_status_does_not_retry assert_equal 1, counter_spy end + # -- StatusError raised inside the attempt (raise_error feature interop) -- + + def error_response + @error_response ||= make_response(status: 503) + end + + def test_perform_retries_status_error_matching_retry_statuses + assert_raises HTTP::OutOfRetriesError do + perform(retry_statuses: [503], tries: 3) do + raise HTTP::StatusError, error_response + end + end + assert_equal 3, counter_spy + end + + def test_perform_retries_status_error_subclasses_and_attaches_response + err = nil + begin + perform(retry_statuses: [503], tries: 2) do + raise CustomStatusError, error_response + end + rescue HTTP::OutOfRetriesError => e + err = e + end + + assert_equal error_response, err.response + assert_equal 2, counter_spy + end + + def test_perform_does_not_retry_status_error_not_matching_retry_statuses + assert_raises HTTP::StatusError do + perform(retry_statuses: [503], tries: 3) do + raise HTTP::StatusError, make_response(status: 404) + end + end + assert_equal 1, counter_spy + end + + def test_perform_does_not_retry_plain_exception_when_retry_statuses_configured + assert_raises CustomException do + perform(retry_statuses: [503], tries: 3) do + raise CustomException + end + end + assert_equal 1, counter_spy + end + + def test_perform_does_not_retry_status_error_without_retry_statuses + assert_raises HTTP::StatusError do + perform(tries: 3) do + raise HTTP::StatusError, error_response + end + end + assert_equal 1, counter_spy + end + + def test_out_of_retries_error_from_status_error_has_response_and_cause + err = nil + begin + perform(retry_statuses: [503], tries: 2) do + raise HTTP::StatusError, error_response + end + rescue HTTP::OutOfRetriesError => e + err = e + end + + assert_equal error_response, err.response + assert_kind_of HTTP::StatusError, err.cause + end + + def test_out_of_retries_error_from_status_error_in_exceptions_list_has_response + err = nil + begin + perform(exceptions: [HTTP::StatusError], tries: 2) do + raise HTTP::StatusError, error_response + end + rescue HTTP::OutOfRetriesError => e + err = e + end + + assert_equal error_response, err.response + end + + def test_on_retry_callback_with_status_error_receives_error_and_response + callback_call_spy = 0 + + callback_spy = proc do |callback_request, error, callback_response| + assert_equal request, callback_request + assert_kind_of HTTP::StatusError, error + assert_equal error_response, callback_response + callback_call_spy += 1 + end + + assert_raises HTTP::OutOfRetriesError do + perform(retry_statuses: [503], tries: 3, on_retry: callback_spy) do + raise HTTP::StatusError, error_response + end + end + + assert_equal 2, callback_call_spy + end + + def test_calculate_delay_receives_status_error_response + responses_seen = [] + + performer = HTTP::Retriable::Performer.new(delay: 0, retry_statuses: [503], tries: 2) + calculator = performer.instance_variable_get(:@delay_calculator) + original_call = calculator.method(:call) + calculator.define_singleton_method(:call) do |iteration, resp| + responses_seen << resp + original_call.call(iteration, resp) + end + + begin + performer.perform(client, request) { raise HTTP::StatusError, error_response } + rescue HTTP::OutOfRetriesError + nil + end + + assert_equal error_response, responses_seen.first + end + + def test_response_flushing_flushes_status_error_response_when_retries_exhausted + flushed = false + error_response.define_singleton_method(:flush) do + flushed = true + self + end + + begin + perform(retry_statuses: [503], tries: 2) do + raise HTTP::StatusError, error_response + end + rescue HTTP::OutOfRetriesError + nil + end + + assert flushed, "expected StatusError response to be flushed on final attempt" + end + # -- on_retry callback -- def test_on_retry_callback_with_exception