Skip to content

Add retries on server errors#275

Merged
tra0x merged 5 commits intomainfrom
tra0x/cf-retry-server-error
Apr 16, 2025
Merged

Add retries on server errors#275
tra0x merged 5 commits intomainfrom
tra0x/cf-retry-server-error

Conversation

@tra0x
Copy link
Contributor

@tra0x tra0x commented Mar 5, 2025

Logical flow:

  1. retry_on_connection_errors in provider base class now checks for RecordStore::Provider::RetriableError
  2. We leave calling RetriableError up to provider implementations
  3. Currently only used by Cloudflare provider
  4. Request block checks for a 5xx response with response.is_a?(Net::HTTPServerError) and catches Net::HTTPFatalError and raises RetriableError; also checks for 3xx response and catches Net::HTTPRetriableError

Tests moved to focus on Cloudflare client's use of retries

  1. Test success when <= max_retries
  2. Test fails when > max_retries
  3. Test 5xx response raises RetriableError

@tra0x tra0x marked this pull request as ready for review March 5, 2025 19:18
max_conn_resets -= 1

waiter.wait
rescue RecordStore::Provider::Error => e
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of using the if match, can we raise/rescue a more specific error?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit surprised this retry is at the generic provider level instead of Cloudflare-specific

do other providers have their retry logic built in?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest creating a new error type like RetryableError and raising that from the provider itself with logic in the provider to switch between the current Error and new RetryableError based on the failure scenario

@tra0x tra0x marked this pull request as draft March 6, 2025 17:44
@tra0x tra0x force-pushed the tra0x/cf-retry-server-error branch from b22f985 to 2727ef6 Compare March 6, 2025 19:34
@tra0x tra0x marked this pull request as ready for review March 6, 2025 19:42
@tra0x tra0x requested a review from sbfaulkner March 26, 2025 16:37

begin
response = conn.request(request)
if response.is_a?(Net::HTTPRetriableError)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thank you. I've updated the PR body explaining the fix.

@tra0x tra0x marked this pull request as draft March 26, 2025 18:06
@tra0x tra0x force-pushed the tra0x/cf-retry-server-error branch 3 times, most recently from 2d1d969 to 18bcd8f Compare March 26, 2025 20:45
@tra0x tra0x force-pushed the tra0x/cf-retry-server-error branch from 18bcd8f to e413225 Compare March 27, 2025 13:34
@tra0x tra0x marked this pull request as ready for review March 27, 2025 14:04
Comment on lines +67 to +69
if response.is_a?(Net::HTTPRedirection) || response.is_a?(Net::HTTPServerError)
response.error!
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we only raising for 3XX and 5XX?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed to use response.value

# Raises an HTTP error if the response is not 2xx (success).
def value
  error! unless self.kind_of?(Net::HTTPSuccess)
end

@tra0x tra0x merged commit 4a040bc into main Apr 16, 2025
7 checks passed
@tra0x tra0x deleted the tra0x/cf-retry-server-error branch April 16, 2025 13:06
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.

3 participants