Skip to content

Make raise_error compose with retriable status retries - #848

Open
lineoffligbot wants to merge 1 commit into
httprb:mainfrom
hakanensari:fix/retriable-raise-error
Open

Make raise_error compose with retriable status retries#848
lineoffligbot wants to merge 1 commit into
httprb:mainfrom
hakanensari:fix/retriable-raise-error

Conversation

@lineoffligbot

Copy link
Copy Markdown
Contributor

Problem

The raise_error feature and the retriable feature silently conflict. Enabling both makes retry_statuses dead configuration:

HTTP.use(:raise_error)
    .retriable(tries: 3, retry_statuses: [503])
    .get(url)
# expected: 3 attempts, then OutOfRetriesError
# actual:   1 attempt, immediate StatusError

raise_error converts an error response into a StatusError inside each attempt (Client#perform_once wraps the response via feature.wrap_response), so Retriable::Performer catches an exception instead of seeing a response. StatusError is not in RETRIABLE_ERRORS, so retry_request? declines and the error is raised on attempt 1. Measured against a local always-503 server:

configuration requests made raised
retriable alone (retry_statuses: [503]) 3 OutOfRetriesError with response
plus raise_error 1 StatusError
workaround: StatusError in exceptions: 3 OutOfRetriesError with nil response
raise_error with ignore: covering every retry status 3 OutOfRetriesError with response

The last row composes correctly but requires keeping two lists in sync by hand forever.

Fix

try_request now recovers the response a StatusError carries:

rescue Exception => e
  err = e
  res = e.response if e.is_a?(StatusError)
end

and retry_request? treats a status-matching StatusError as retriable:

elsif err
  retry_exception?(err) || retriable_status_error?(err)

Populating res early repairs the whole downstream pipeline at once: retry_statuses matching, Retry-After delay calculation, the on_retry callback, response flushing on exhaustion, and OutOfRetriesError#response and #cause. A StatusError whose status is not covered still raises immediately on attempt 1, preserving raise_error semantics. This also closes the nil-response hole in the exceptions: workaround (third row above).

The underlying principle: raise-on-status should act only after retry-on-status is exhausted, but raise_error runs per attempt inside the retry loop, so the performer has to translate the exception back into its response.

Notes on observable changes

  • on_retry and should_retry callbacks now receive the error response alongside the StatusError (previously the response argument was nil in that case).
  • OutOfRetriesError#response is populated when retries were driven by a StatusError, and #cause is the StatusError.

Ten new tests cover the matrix rows, the preserved no-retry paths, StatusError subclasses, delay calculator and callback plumbing, and response flushing, all written to fail before the fix. RBS signature included. Mutant kills 100% of mutations on the diff.

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 <hakanensari@gmail.com>
@hakanensari
hakanensari force-pushed the fix/retriable-raise-error branch from e86a77b to 671cdce Compare August 16, 2026 14:39
@lineoffligbot

Copy link
Copy Markdown
Contributor Author

A note on the failing steep job: it is environment drift, not this patch. The Gemfile does not lock rbs (gem "rbs", ">= 4"), so CI resolves it fresh on every run. The last green run on main (July 14) resolved rbs 4.0.3; runs today resolve rbs 4.1.3, which introduces 8 new warnings in files this PR does not touch, and bundle exec rake steep fails identically on an untouched checkout of main. Happy to send a small separate PR fixing those warnings so CI goes green again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant