Fix: Add max retries to token refresh #60
Conversation
WalkthroughThe pull request introduces enhanced error handling for token refresh in the Changes
Sequence DiagramsequenceDiagram
participant Client
participant TokenAuth
participant AuthService
Client->>TokenAuth: refresh_token()
TokenAuth->>TokenAuth: Check refresh_attempts
alt Attempts < MAX_RETRIES
TokenAuth->>AuthService: Request new token
alt Token refresh successful
AuthService-->>TokenAuth: New token
TokenAuth->>TokenAuth: Reset refresh_attempts
else Token refresh failed
TokenAuth->>TokenAuth: Increment refresh_attempts
end
else Attempts >= MAX_RETRIES
TokenAuth->>TokenAuth: Log error
TokenAuth-->>Client: Refresh failed
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between bae4594e3e42364356d51d00907d655fdd79b306 and b6ed4b4. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![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 (1)
trakt/api.py (1)
184-184: Document the new instance variable in the class docstring.The
refresh_attemptsinstance variable should be documented in the class's docstring to maintain API documentation completeness.Add this to the class docstring:
class TokenAuth(AuthBase): """Attaches Trakt.tv token Authentication to the given Request object. + + Attributes: + refresh_attempts (int): Counter tracking the number of token refresh attempts. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between a8e5fba and bae4594e3e42364356d51d00907d655fdd79b306.
📒 Files selected for processing (1)
trakt/api.py(3 hunks)
🔇 Additional comments (2)
trakt/api.py (2)
175-176: LGTM! Well-defined constant with clear documentation.The
MAX_RETRIESconstant with a value of 3 is a reasonable choice for limiting token refresh attempts.
Line range hint
227-243: Verify retry handling in related authentication flows.Let's check if similar retry mechanisms are needed in other authentication-related methods.
✅ Verification successful
Retry mechanism is correctly implemented where needed
The retry mechanism is appropriately implemented in the token refresh flow, which is an automated process. The initial OAuth authentication is a user-interactive flow that doesn't require automatic retries, as it relies on user input and authorization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other authentication-related methods that might benefit from retry mechanisms ast-grep --pattern 'def $_(self, $$$): $$$ /oauth/$$$ $$$'Length of output: 66
Script:
#!/bin/bash # Search for OAuth-related code and authentication flows rg -l "oauth" --type py rg "def.*oauth" --type py -B1 -A5Length of output: 1574
Script:
#!/bin/bash # Get the complete OAuthAdapter implementation rg -A 50 "class OAuthAdapter" trakt/auth/oauth.py # Search for any retry-related code in OAuth files rg "retry|attempt" trakt/auth/Length of output: 2341
trakt/api.py
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the retry mechanism implementation.
The current implementation has several areas for improvement:
- Silent return on max retries might mask issues from callers
- Error logging could be more informative
- Missing retry backoff strategy
- Incomplete reset of refresh counter
Consider these improvements:
def refresh_token(self):
"""Request Trakt API for a new valid OAuth token using refresh_token"""
if self.refresh_attempts >= self.MAX_RETRIES:
- self.logger.error("Max token refresh attempts reached. Manual intervention required.")
- return
+ msg = f"Max token refresh attempts ({self.MAX_RETRIES}) reached. Manual intervention required."
+ self.logger.error(msg)
+ raise OAuthException(msg)
+
self.refresh_attempts += 1
+ retry_delay = min(2 ** (self.refresh_attempts - 1), 60) # Exponential backoff capped at 60s
+ time.sleep(retry_delay)
self.logger.info("OAuth token has expired, refreshing now...")
try:
# ... existing code ...
self.refresh_attempts = 0
except OAuthException:
+ self.refresh_attempts = 0 # Reset counter on permanent failure
self.logger.debug(
"Rejected - Unable to refresh expired OAuth token, "
"refresh_token is invalid"
)
- return
+ raiseChanges explained:
- Raise an exception instead of silently returning on max retries
- Include retry count in error message
- Add exponential backoff between retries
- Reset counter on permanent failures
Also applies to: 243-243
Suggestion posted by coderabbit: - #59 (review)
Suggestion posted by coderabbit: