Better handlng of transient validation errors#111
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves handling of transient validation errors by implementing retry mechanisms and better error classification. The changes distinguish between permanent validation failures and temporary infrastructure issues that should be retried.
- Adds retry logic with exponential backoff for API calls and network timeouts
- Introduces new error types to differentiate transient vs permanent failures
- Implements validation stalling detection to pause imports when validation falls behind
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/ethscriptions_api_client.rb | Adds retry mechanism with custom error types and exponential backoff |
| lib/block_validator.rb | Introduces TransientValidationError and improves error classification |
| config/derive_ethscriptions_blocks.rb | Handles ValidationStalledError to pause imports when validation is behind |
| app/services/eth_block_importer.rb | Separates real validation failures from stalled validation states |
| app/models/validation_result.rb | Updates validation persistence to handle transient errors properly |
| app/jobs/validation_job.rb | Configures retry behavior specifically for transient validation errors |
Comments suppressed due to low confidence (1)
lib/ethscriptions_api_client.rb:1
- Unnecessary parentheses in method call. This should be
@errors.empty?without parentheses.
class EthscriptionsApiClient
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| rescue HttpError, ApiError, NetworkError => e | ||
| # Wrap all internal errors into a single type for callers | ||
| Rails.logger.error "API unavailable for block #{block_number} after retries: #{e.message}" | ||
| raise ApiUnavailableError, "API unavailable after #{ENV.fetch('ETHSCRIPTIONS_API_RETRIES', 7)} retries: #{e.message}" |
There was a problem hiding this comment.
The retry count should be retrieved from the same source as the retry configuration on line 85. Consider storing the retry count in a variable to avoid inconsistency.
| def get_validation_failure_block | ||
| # Get the earliest failed block that's behind current import | ||
| def validation_stalled? | ||
| return false unless ENV.fetch('VALIDATION_ENABLED').casecmp?('true') |
There was a problem hiding this comment.
This will raise a KeyError if VALIDATION_ENABLED environment variable is not set. Use ENV.fetch('VALIDATION_ENABLED', 'false') to provide a default value.
| return false unless ENV.fetch('VALIDATION_ENABLED').casecmp?('true') | |
| return false unless ENV.fetch('VALIDATION_ENABLED', 'false').casecmp?('true') |
| exception_class: e.class.name, | ||
| validated_at: Time.current |
There was a problem hiding this comment.
The validated_at field is being set in both the validation_stats hash and as a separate attribute. Remove the duplicate entry from the validation_stats hash to avoid confusion.
| exception_class: e.class.name, | |
| validated_at: Time.current | |
| exception_class: e.class.name |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| error_result.log_summary | ||
| validation_result.save! | ||
|
|
||
| validation_result.log_summary |
There was a problem hiding this comment.
Bug: Race Condition in Concurrent Validation Jobs
The find_or_initialize_by and save! pattern introduces a race condition. Concurrent validation jobs for the same l1_block can cause uniqueness constraint violations during persistence in both success and error paths. Also, if save! fails for other database reasons, the general rescue block misclassifies it as a validation logic error.
| request['Accept'] = 'application/json' | ||
|
|
||
| begin | ||
| response = http.request(request) |
There was a problem hiding this comment.
Bug: Timeouts and Exception Handling Issues
The HTTP client no longer sets explicit open_timeout and read_timeout, which means network requests will use long default timeouts. This significantly delays retry attempts during network issues. Furthermore, the Retriable configuration redundantly includes Net::OpenTimeout and Net::ReadTimeout, as these exceptions are caught and re-raised as NetworkError before Retriable can process them directly.
No description provided.