Flexible dynamic exponential backoff of requests #272
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Client as BigDBMClient
participant HTTP as HTTP Layer
Caller->>Client: __request(...)
rect rgb(220,235,255)
Note over Client: Retry loop (up to max_request_attempts)
loop Each Attempt
Client->>HTTP: Send request (timeout: request_timeout_seconds)
alt Success
HTTP-->>Client: Response
Client-->>Caller: Return response
else RequestException
HTTP-->>Client: Exception
Client->>Client: Log warning (attempt N)
Client->>Client: Sleep (exponential backoff)
end
end
end
rect rgb(255,220,220)
Note over Client: All attempts failed
Client->>Client: Log error (final)
Client-->>Caller: Raise last exception
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
real_intent/client.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
real_intent/client.py (1)
real_intent/internal_logging.py (1)
log(16-17)
🪛 Ruff (0.14.5)
real_intent/client.py
135-135: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (1)
real_intent/client.py (1)
34-46: LGTM! Constructor parameters are well-designed.The addition of configurable
request_timeout_secondsandmax_request_attemptsparameters provides the intended flexibility for exponential backoff behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
real_intent/client.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
real_intent/client.py (1)
real_intent/internal_logging.py (1)
log(16-17)
🪛 Ruff (0.14.5)
real_intent/client.py
126-126: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
142-142: Undefined name e
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (2)
real_intent/client.py (2)
34-40: LGTM!The new parameters are well-named, properly typed, and have sensible defaults. The signature change maintains backward compatibility for existing code using positional arguments.
45-46: LGTM!The attributes are properly initialized from the constructor parameters, and the rename from
timeout_secondstorequest_timeout_secondsimproves clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
real_intent/client.py (1)
113-148: Critical: Exception scoping bugs still present.The retry loop still has the critical exception handling bugs flagged in previous reviews:
Line 145:
eis undefined - In Python 3, exception variables are automatically deleted at the end of theexceptblock (PEP 3110). Theelseblock cannot accesse, causing aNameError.Line 148: Bare
raisefails - A bareraisestatement only works inside an exception handler to re-raise the active exception. In thefor...elseblock, there's no active exception, causing aRuntimeError.These issues will cause the code to crash when all retry attempts are exhausted.
Apply this diff to fix the exception handling:
+ last_exception: Exception | None = None for n_attempt in range(1, self.max_request_attempts+1): try: with Session() as session: response = session.send( request.prepare(), timeout=self.request_timeout_seconds ) response.raise_for_status() break except RequestException as e: + last_exception = e + if n_attempt == self.max_request_attempts: - continue # on last failed attempt, skip to else block + break # Don't sleep after final failure # Log it, sleep, loop again to try again sleep_time = round( number=random.uniform(30 * n_attempt, (30 * n_attempt) + 10), ndigits=2 ) log( "warn", ( f"Request attempt {n_attempt} of {self.max_request_attempts} " f"failed. Retrying in {sleep_time}s. Error: {e}" ) ) time.sleep(sleep_time) else: log( "error", ( f"Request failed after {self.max_request_attempts} " - f"attempts. Error: {e}" + f"attempts. Error: {last_exception}" ) ) - raise + raise last_exception
🧹 Nitpick comments (1)
real_intent/client.py (1)
34-46: Consider adding parameter validation.The new parameters lack validation. Invalid values like
max_request_attempts < 1orrequest_timeout_seconds <= 0could cause confusing runtime behavior.Apply this diff to add validation:
def __init__( self, client_id: str, client_secret: str, request_timeout_seconds: int = 30, max_request_attempts: int = 3 ) -> None: """Initialize the BigDBM client.""" + if max_request_attempts < 1: + raise ValueError("max_request_attempts must be at least 1") + if request_timeout_seconds <= 0: + raise ValueError("request_timeout_seconds must be positive") + self.client_id: str = client_id self.client_secret: str = client_secret self.request_timeout_seconds: int = request_timeout_seconds self.max_request_attempts: int = max_request_attempts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
real_intent/client.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
real_intent/client.py (1)
real_intent/internal_logging.py (1)
log(16-17)
🪛 Ruff (0.14.5)
real_intent/client.py
129-129: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
145-145: Undefined name e
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
real_intent/client.py (1)
88-88: LGTM!The updated docstring accurately reflects the behavior of the method.
There was a problem hiding this comment.
Pull request overview
This PR replaces hardcoded retry logic with a configurable retry mechanism for API requests. The changes make the client more resilient to transient API failures by allowing customization of timeout and retry attempts, though the implementation uses linear backoff rather than true exponential backoff as described.
Key Changes
- Added configurable
request_timeout_seconds(default: 30s) andmax_request_attempts(default: 3) parameters toBigDBMClient.__init__ - Replaced one-time retry with sleep of 7-13 seconds with a loop-based retry mechanism
- Implemented linear backoff with delays of approximately 30s, 60s, 90s for successive attempts (not exponential as stated in description)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
real_intent/client.py (1)
85-90: Docstring should mention retry/backoff behavior.
__requestnow performs multiple attempts with backoff, not just a single call that “raises for status automatically.” Consider updating the docstring to describe that it may retry up tomax_request_attemptswith incremental delays before propagating aRequestException, so callers understand the potential latency and failure mode.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
real_intent/client.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
real_intent/client.py (1)
real_intent/internal_logging.py (1)
log(16-17)
🪛 Ruff (0.14.5)
real_intent/client.py
131-131: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
real_intent/client.py (2)
146-146: Clarify: the backoff is linear, not exponential.The PR description claims "exponential backoff," but the formula
30 * n_attemptproduces linear delays: approximately 30-40s, 60-70s, 90-100s for attempts 1, 2, 3. If true exponential backoff is intended, use30 * (2 ** (n_attempt - 1))for delays of approximately 30-40s, 60-70s, 120-130s. Otherwise, update the PR description to accurately describe this as "linear backoff."Additionally, silence the S311 static analysis warning since
random.uniformis appropriate for non-cryptographic jitter.Option 1: Keep linear backoff and silence S311 (if current behavior is acceptable):
sleep_time = round( - number=random.uniform(30 * n_attempt, (30 * n_attempt) + 10), + number=random.uniform(30 * n_attempt, (30 * n_attempt) + 10), # noqa: S311 ndigits=2 )Option 2: Implement true exponential backoff:
sleep_time = round( - number=random.uniform(30 * n_attempt, (30 * n_attempt) + 10), + number=random.uniform(30 * (2 ** (n_attempt - 1)), (30 * (2 ** (n_attempt - 1))) + 10), # noqa: S311 ndigits=2 )
38-38: Add validation forrequest_timeout_secondsto prevent immediate timeout failures.The parameter accepts any integer value, including 0 or negative numbers, which would cause immediate timeout failures or undefined behavior in the requests library.
Apply this diff to add validation:
""" if max_request_attempts < 1: raise ValueError("max_request_attempts must be at least 1") + if request_timeout_seconds <= 0: + raise ValueError("request_timeout_seconds must be positive") + self.client_id: str = client_id self.client_secret: str = client_secret
🧹 Nitpick comments (2)
real_intent/client.py (2)
141-142: Consider removing the unnecessarycontinuestatement.The
continuestatement when on the last attempt is unnecessary and could be confusing. The loop would naturally end anyway, and theelseblock would execute. A clearer pattern is to checkif n_attempt < self.max_request_attempts:before the sleep logic.Apply this diff for clearer intent:
except RequestException as e: last_exception: RequestException = e - if n_attempt == self.max_request_attempts: - continue # on last failed attempt, skip to else block - - # Log it, sleep, loop again to try again - sleep_time = round( - number=random.uniform(30 * n_attempt, (30 * n_attempt) + 10), - ndigits=2 - ) - log( - "warn", - ( - f"Request attempt {n_attempt} of {self.max_request_attempts} " - f"failed. Retrying in {sleep_time}s. Error: {e}" + if n_attempt < self.max_request_attempts: + # Log it, sleep, loop again to try again + sleep_time = round( + number=random.uniform(30 * n_attempt, (30 * n_attempt) + 10), + ndigits=2 ) - ) - time.sleep(sleep_time) + log( + "warn", + ( + f"Request attempt {n_attempt} of {self.max_request_attempts} " + f"failed. Retrying in {sleep_time}s. Error: {e}" + ) + ) + time.sleep(sleep_time)
145-147: Consider extracting magic numbers as constants.The hardcoded values
30(base delay in seconds) and10(jitter in seconds) make the backoff calculation less maintainable. Consider extracting them as class-level constants likeBASE_RETRY_DELAY_SECONDS = 30andRETRY_JITTER_SECONDS = 10, or making them configurable via__init__parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
real_intent/client.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
real_intent/client.py (1)
real_intent/internal_logging.py (1)
log(16-17)
🪛 Ruff (0.14.5)
real_intent/client.py
56-56: Avoid specifying long messages outside the exception class
(TRY003)
146-146: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
[skip ci]
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
real_intent/client.py (2)
34-63: Good additions: validation and documentation.The constructor improvements are solid:
- Parameter validation for
max_request_attempts >= 1prevents theUnboundLocalErroridentified in previous reviews.- The docstring clearly documents both new parameters and their defaults.
Based on previous review feedback from Copilot, consider adding validation for
request_timeout_secondsto ensure it's positive:""" # Validate input + if request_timeout_seconds <= 0: + raise ValueError("request_timeout_seconds must be positive") if max_request_attempts < 1: raise ValueError("max_request_attempts must be at least 1")
130-167: Retry logic is now correct; consider optional cleanup.The retry implementation properly addresses the critical bugs from earlier reviews:
- Success path breaks out of the loop immediately.
- Exception scoping uses
last_exceptionvariable correctly.- The for-else pattern ensures final failures are logged and raised.
A few optional refinements remain from previous feedback:
- Line 144: Unnecessary
continue– As Copilot noted, thecontinueon the last attempt doesn't add clarity. Consider restructuring:except RequestException as e: last_exception: RequestException = e - - if n_attempt == self.max_request_attempts: - continue # on last failed attempt, skip to else block - - # Log it, sleep, loop again to try again - sleep_time = round( - number=random.uniform(30 * n_attempt, (30 * n_attempt) + 10), - ndigits=2 - ) - log( - "warn", - ( - f"Request attempt {n_attempt} of {self.max_request_attempts} " - f"failed. Retrying in {sleep_time}s. Error: {e}" - ) - ) - time.sleep(sleep_time) + + # Only retry if not on the last attempt + if n_attempt < self.max_request_attempts: + sleep_time = round( + number=random.uniform(30 * n_attempt, (30 * n_attempt) + 10), # noqa: S311 + ndigits=2 + ) + log( + "warn", + ( + f"Request attempt {n_attempt} of {self.max_request_attempts} " + f"failed. Retrying in {sleep_time}s. Error: {e}" + ) + ) + time.sleep(sleep_time)
Line 148: Linear vs exponential backoff – Copilot correctly noted the backoff is linear (
30s, 60s, 90s) not exponential. Either update the PR title/description or adjust to true exponential (30 * 2^(n-1)→30s, 60s, 120s).Line 148: S311 warning – Suppress the non-cryptographic random warning with
# noqa: S311as shown above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
real_intent/client.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
real_intent/client.py (1)
real_intent/internal_logging.py (1)
log(16-17)
🪛 Ruff (0.14.5)
real_intent/client.py
57-57: Avoid specifying long messages outside the exception class
(TRY003)
148-148: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
real_intent/client.py (3)
143-144: Remove unnecessarycontinuestatement.When
n_attempt == self.max_request_attempts, the loop naturally ends after this iteration regardless of thecontinuestatement. The code then falls through to theelseblock as intended. Thecontinueadds no functional value and creates confusion about the control flow.Apply this diff to simplify:
except RequestException as e: last_exception: RequestException = e - if n_attempt == self.max_request_attempts: - continue # on last failed attempt, skip to else block - - # Log it, sleep, loop again to try again - sleep_time = round( - number=random.uniform(30 * n_attempt, (30 * n_attempt) + 10), - ndigits=2 - ) - log( - "warn", - ( - f"Request attempt {n_attempt} of {self.max_request_attempts} " - f"failed. Retrying in {sleep_time}s. Error: {e}" - ) - ) - time.sleep(sleep_time) + # Only retry if not on the last attempt + if n_attempt < self.max_request_attempts: + # Log it, sleep, loop again to try again + sleep_time = round( + number=random.uniform(30 * n_attempt, (30 * n_attempt) + 10), + ndigits=2 + ) + log( + "warn", + ( + f"Request attempt {n_attempt} of {self.max_request_attempts} " + f"failed. Retrying in {sleep_time}s. Error: {e}" + ) + ) + time.sleep(sleep_time)
34-64: Add validation forrequest_timeout_secondsparameter.The constructor validates
max_request_attemptsbut doesn't validaterequest_timeout_seconds. A value of 0 or negative would cause immediate timeout failures or undefined behavior in the requests library.Apply this diff to add validation:
""" # Validate input + if not isinstance(request_timeout_seconds, int) or request_timeout_seconds <= 0: + raise ValueError("request_timeout_seconds must be an int and greater than 0") + if not isinstance(max_request_attempts, int) or max_request_attempts < 1: raise ValueError("max_request_attempts must be an int and at least 1")
147-149: Clarify: This implements linear backoff, not exponential.The PR title and description claim "exponential backoff," but the formula
30 * n_attemptproduces linear delays: approximately 30-40s, 60-70s, 90-100s for attempts 1, 2, 3.True exponential backoff would use
30 * (2 ** (n_attempt - 1)), yielding approximately 30-40s, 60-70s, 120-130s.Either update the PR description to accurately describe this as "linear backoff," or apply this diff for true exponential backoff:
# Log it, sleep, loop again to try again sleep_time = round( - number=random.uniform(30 * n_attempt, (30 * n_attempt) + 10), + number=random.uniform(30 * (2 ** (n_attempt - 1)), (30 * (2 ** (n_attempt - 1))) + 10), # noqa: S311 ndigits=2 )Note: I've also added
# noqa: S311to suppress the Ruff warning aboutrandom.uniformin cryptographic contexts—backoff jitter is not a security-sensitive use case.
🧹 Nitpick comments (1)
real_intent/client.py (1)
148-148: Suppress false-positive S311 warning for non-cryptographic random usage.The
random.uniformcall is used only for backoff jitter timing, not for cryptographic purposes. The Ruff S311 warning is a false positive here.Apply this diff to suppress the warning:
sleep_time = round( - number=random.uniform(30 * n_attempt, (30 * n_attempt) + 10), + number=random.uniform(30 * n_attempt, (30 * n_attempt) + 10), # noqa: S311 ndigits=2 )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
real_intent/client.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
real_intent/client.py (1)
real_intent/internal_logging.py (1)
log(16-17)
🪛 Ruff (0.14.5)
real_intent/client.py
57-57: Avoid specifying long messages outside the exception class
(TRY003)
148-148: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
Instead of a hardcoded double backoff with (7, 13) delay, create a loop-based exponential backoff with a parameter at instantiation
max_request_attemptsdefaulting to 3. Each request fail increments in30 * n_attemptseconds, more forgiving to target API.Unfortunately the double backoff was failing the entire system a few times due to a brittle provider API. This should be much more robust.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.