feat: retry network errors #34
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant IR as InfisicalRequests
participant DR as with_retry Decorator
participant HTTP as HTTP Request
C->>IR: Call get/post/patch/delete
IR->>DR: Enter retry-decorated method
DR->>HTTP: Attempt request
alt Request Fails with network error
HTTP-->>DR: Exception raised
DR->>DR: Check retry count & delay (exponential backoff)
loop If retries remain
DR->>HTTP: Retry request
end
else Request Succeeds
HTTP-->>DR: Return response
end
DR-->>IR: Return final response or raise final exception
IR-->>C: Return response / error
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
infisical_sdk/infisical_requests.py (2)
236-236: Fix the documentation comment for the delete method.The comment incorrectly states "Make a PATCH request with JSON data" instead of "Make a DELETE request with JSON data".
- """Make a PATCH request with JSON data""" + """Make a DELETE request with JSON data"""
89-101: Consider capturing the original exception for better error reporting.When raising the exception after maximum retries, it would be helpful to capture the original exception details to provide more context to the caller. This can be done using raise-from syntax or by creating a custom exception.
except tuple(network_errors) as error: retry_count += 1 if retry_count > max_retries: print(f"Max retries ({max_retries}) exceeded. Giving up.") - raise + raise InfisicalError(f"Network error after {max_retries} retries: {error.__class__.__name__}: {str(error)}") from error delay = base_delay * (2 ** (retry_count - 1))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infisical_sdk/infisical_requests.py(6 hunks)
🔇 Additional comments (6)
infisical_sdk/infisical_requests.py (6)
1-6: Well-structured imports for the new functionality.The new imports are appropriately added to support the retry mechanism functionality, including
Callable,List,socket,functools, andtime.
10-22: Good implementation of network error types.Comprehensive list of network-related exceptions covering both
requestslibrary and socket module errors, which is ideal for identifying retry-worthy conditions.
151-151: Consistent application of the retry decorator.The decorator is appropriately applied to the
getmethod with 4 retries and a 1-second base delay, aligning with the PR objectives.
178-178: Consistent application of the retry decorator.The decorator is appropriately applied to the
postmethod with the same parameters as other methods for consistent behavior.
203-203: Consistent application of the retry decorator.The decorator is appropriately applied to the
patchmethod with the same parameters as other methods for consistent behavior.
228-228: Consistent application of the retry decorator.The decorator is appropriately applied to the
deletemethod with the same parameters as other methods for consistent behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
infisical_sdk/infisical_requests.py (1)
150-250: 🛠️ Refactor suggestionUpdate all retry decorators with the max_delay parameter once implemented.
After implementing the max_delay parameter in the with_retry decorator, update all decorator calls to include this parameter.
- @with_retry(max_retries=4, base_delay=1.0) + @with_retry(max_retries=4, base_delay=1.0, max_delay=8.0)Apply this change to all four HTTP methods (GET, POST, PATCH, DELETE).
♻️ Duplicate comments (1)
infisical_sdk/infisical_requests.py (1)
70-104: 🛠️ Refactor suggestionAdd maximum delay cap and error logging as per the PR objectives.
The PR objectives mention a maximum delay cap of 8 seconds, but this is missing from the implementation. Additionally, the caught error is unused, and there's no logging to help with debugging or traceability.
def with_retry( max_retries: int = 3, base_delay: float = 1.0, + max_delay: float = 8.0, network_errors: Optional[List[Type[Exception]]] = None ) -> Callable: """ Decorator to add retry logic with exponential backoff to requests methods. + + Args: + max_retries: Maximum number of retry attempts + base_delay: Base delay in seconds between retries + max_delay: Maximum delay cap in seconds for any single retry + network_errors: List of exceptions that should trigger retries """ if network_errors is None: network_errors = NETWORK_ERRORS def decorator(func: Callable) -> Callable: @functools.wraps(func) def wrapper(*args, **kwargs): + import logging + logger = logging.getLogger(__name__) retry_count = 0 while True: try: return func(*args, **kwargs) except tuple(network_errors) as error: retry_count += 1 if retry_count > max_retries: + logger.error(f"Max retries ({max_retries}) exceeded for {func.__name__}. Last error: {error}") raise base_delay_with_backoff = base_delay * (2 ** (retry_count - 1)) + base_delay_with_backoff = min(base_delay_with_backoff, max_delay) # +/-20% jitter jitter = random.uniform(-0.2, 0.2) * base_delay_with_backoff delay = base_delay_with_backoff + jitter + logger.warning( + f"Network error {error.__class__.__name__}: {str(error)}. " + f"Retrying in {delay:.2f}s (attempt {retry_count}/{max_retries})" + ) time.sleep(delay) return wrapper return decorator🧰 Tools
🪛 Ruff (0.8.2)
89-89: Local variable
erroris assigned to but never usedRemove assignment to unused variable
error(F841)
🧹 Nitpick comments (1)
infisical_sdk/infisical_requests.py (1)
227-236: Fix incorrect docstring for delete method.The docstring incorrectly states this is a "PATCH request" when it should be a "DELETE request".
@with_retry(max_retries=4, base_delay=1.0) def delete( self, path: str, model: Type[T], json: Optional[Dict[str, Any]] = None ) -> APIResponse[T]: - """Make a PATCH request with JSON data""" + """Make a DELETE request with JSON data"""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infisical_sdk/infisical_requests.py(6 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
infisical_sdk/infisical_requests.py
89-89: Local variable error is assigned to but never used
Remove assignment to unused variable error
(F841)
🔇 Additional comments (4)
infisical_sdk/infisical_requests.py (4)
12-23: LGTM: Comprehensive list of network errors for retry.The list of network-related exceptions is well-thought-out, covering the common connection, timeout and socket errors that would benefit from retries.
150-150: LGTM: Appropriate retry configuration for GET requests.The retry configuration with max_retries=4 and base_delay=1.0 aligns with the PR objectives for improving network resilience.
177-177: LGTM: Appropriate retry configuration for POST requests.The retry configuration with max_retries=4 and base_delay=1.0 aligns with the PR objectives for improving network resilience.
202-202: LGTM: Appropriate retry configuration for PATCH requests.The retry configuration with max_retries=4 and base_delay=1.0 aligns with the PR objectives for improving network resilience.
This PR implements automatic retries of network related errors such as ECONNRESET or host not found. It will retry 4 times, exponentially increasing the retry delay. The highest retry delay will be 8 seconds, which means the total max retry time will be 15 seconds in total.
Summary by CodeRabbit