Feature/Add retry to with backoff to policy download #638
Conversation
WalkthroughThe changes in this pull request significantly modify the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@SafetyQuincyF has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
SafetyQuincyF
changed the title
![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
🧹 Outside diff range and nitpick comments (1)
safety/scan/render.py (1)
340-344: Consider using thetenacitylibrary for retry logicTo simplify the retry mechanism and improve maintainability, consider using the
tenacitylibrary. It provides robust features for retrying operations with exponential backoff and can handle more complex retry strategies.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
safety/scan/render.py (2)
340-343: Consider moving retry configuration to a dedicated constants file.The retry mechanism constants could be moved to a dedicated configuration file or constants module for better maintainability and reusability across the codebase.
- # can replace with tenacity in the future - MAX_RETRIES = 3 - INITIAL_BACKOFF = 1 # seconds - BACKOFF_MULTIPLIER = 2 # Exponential factor for backoff + from safety.constants import ( + POLICY_MAX_RETRIES, + POLICY_INITIAL_BACKOFF, + POLICY_BACKOFF_MULTIPLIER + )
349-363: Enhance error messaging for better debugging.While the retry logic is well-implemented, the error message could be more informative by including the specific error details.
- console.print("Not using cloud policy file.") + console.print(f"Not using cloud policy file. Failed to download after {MAX_RETRIES} attempts. Last error: {str(e)}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
safety/scan/render.py(1 hunks)
🔇 Additional comments (1)
safety/scan/render.py (1)
340-369: Verify integration with error handling system.
The retry mechanism is well-implemented, but we should verify its integration with the overall error handling system.
✅ Verification successful
The retry mechanism aligns well with the existing error handling patterns
The implementation follows the established error handling patterns in the codebase:
- Uses consistent logging patterns with
LOG.exceptionfor detailed error tracking - Properly handles exceptions without exposing sensitive information
- Gracefully degrades by falling back to "Not using cloud policy file" when retries are exhausted
- Integrates with the existing
SafetyExceptionhierarchy used throughout the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent error handling patterns across the codebase
rg -A 2 "SafetyException|LOG\.(error|warning|exception)" safety/
Length of output: 9018
SafetyQuincyF
force-pushed
the
feature/add-retry-to-policy
branch
from
9c28e27 to
dee078d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
safety/scan/render.py (1)
340-343: Consider moving constants to configuration and using tenacity.The retry configuration constants could be moved to a dedicated configuration file for better maintainability. Also, as noted in the comment, consider using the
tenacitylibrary which provides a robust retry mechanism with features like jitter and various backoff strategies.- # can replace with tenacity in the future - MAX_RETRIES = 3 - INITIAL_BACKOFF = 1 # seconds - BACKOFF_MULTIPLIER = 2 # Exponential factor for backoff + from safety.config import POLICY_MAX_RETRIES, POLICY_INITIAL_BACKOFF, POLICY_BACKOFF_MULTIPLIER
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
safety/scan/render.py(1 hunks)
🔇 Additional comments (2)
safety/scan/render.py (2)
349-363: LGTM! Well-implemented retry mechanism.
The retry implementation follows best practices with:
- Proper exception handling
- Clear user feedback
- Exponential backoff
- Appropriate logging
365-369: 🛠️ Refactor suggestion
Improve warning message and fix potential race condition.
The warning message could be more specific about the impact of failing to fetch the policy, and there's a potential race condition between the success message and the warning log.
if policy:
wait_msg = "Policy fetched from Safety Platform."
else:
- LOG.warning("Failed to fetch policy after retries.")
+ LOG.warning("Failed to fetch policy after %d retries. Falling back to default policies.", MAX_RETRIES)
+ wait_msg = "Not using cloud policy file."Likely invalid or redundant comment.
| console.print(f"Retrying in {backoff_time:.1f} seconds...") | ||
| time.sleep(backoff_time) # Wait before next attempt | ||
| else: | ||
| console.print("Not using cloud policy file.") |
There was a problem hiding this comment.
My only question here is why is this failing? Is this a retry/timeout issue?
There was a problem hiding this comment.
Not sure on platform side no error was logged in sentry.
| @@ -337,22 +337,36 @@ def print_wait_policy_download(console: Console, closure: Tuple[Any, Dict[str, A | |||
| Returns: | |||
| Optional[PolicyFileModel]: The downloaded policy file model. | |||
| """ | |||
| # can replace with tenacity in the future | |||
| MAX_RETRIES = 3 | |||
| INITIAL_BACKOFF = 1 # seconds | |||
There was a problem hiding this comment.
Can we start with lower initial retry i.e 0.2 which is 200 ms and bump the retries to 5.
| LOG.exception(f"Attempt {attempt} failed: {e}") | ||
| if attempt < MAX_RETRIES: | ||
| backoff_time = INITIAL_BACKOFF * (BACKOFF_MULTIPLIER ** (attempt - 1)) | ||
| console.print(f"Retrying in {backoff_time:.1f} seconds...") |
There was a problem hiding this comment.
I don't think we need to notify the user about it, if there is a way to make this a debug log I would prefer that, but not in the normal log output.
Description
Adds 3 retries with backoff when trying to download the policy file.
Type of Change
Testing
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation