Reduce access_token refresh margin delay from 2 days to 10 minutes #76
Conversation
WalkthroughThe changes modify the token validation logic in the TokenAuth class found in trakt/api.py. The validity condition was adjusted to require that tokens have more than ten minutes before expiration, rather than over two days. No changes were made to the signatures of exported or public entities. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant T as TokenAuth
C->>T: validate_token(token)
alt Token expiry > 10 minutes
T-->>C: Return "Token valid"
else Token expiry ≤ 10 minutes
T-->>C: Initiate token refresh
end
Possibly related PRs
Poem
✨ Finishing Touches
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 (
|
simonc56
force-pushed
the
lower-token-expiration-delay
branch
from
8d81412 to
8d135af
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
trakt/api.py (2)
217-217: LGTM! Consider making the margin configurable.The 10-minute margin is a reasonable choice given the new 24-hour token expiration policy. However, consider making this value configurable to easily adjust it if needed.
class TokenAuth(AuthBase): + # Time margin before token expiry when refresh should be triggered + TOKEN_REFRESH_MARGIN = timedelta(minutes=10) + def validate_token(self): """Check if current OAuth token has not expired""" current = datetime.now(tz=timezone.utc) expires_at = datetime.fromtimestamp(self.config.OAUTH_EXPIRES_AT, tz=timezone.utc) - if expires_at - current > timedelta(minutes=10): + if expires_at - current > self.TOKEN_REFRESH_MARGIN:
212-220: Add comments explaining the token validation logic.The token validation logic would benefit from additional comments explaining the margin choice and improved logging for better debugging.
def validate_token(self): - """Check if current OAuth token has not expired""" + """Check if current OAuth token has not expired + + The token is considered valid if it expires in more than TOKEN_REFRESH_MARGIN + (default: 10 minutes). This margin ensures the token doesn't expire during + critical operations while also maximizing the token's useful lifetime. + """ current = datetime.now(tz=timezone.utc) expires_at = datetime.fromtimestamp(self.config.OAUTH_EXPIRES_AT, tz=timezone.utc) - if expires_at - current > timedelta(minutes=10): + margin = expires_at - current + if margin > self.TOKEN_REFRESH_MARGIN: + self.logger.debug("Token valid for %s, exceeds margin of %s", margin, self.TOKEN_REFRESH_MARGIN) self.OAUTH_TOKEN_VALID = True else: + self.logger.debug("Token expires in %s, refreshing (margin: %s)", margin, self.TOKEN_REFRESH_MARGIN) self.refresh_token()
|
It could be calculated automatically from the age the token will expire to account for future token changes. but too annoying to figure out what the logic would be. or maybe some AI could provide example logic? |
2 participants
Starting on March 4, 2025, a Trakt OAuth
access_tokenwill expire in 24 hours, instead of 3 months.The current logic is to refresh the
access_tokenif its expiration date is within next 2 days.With current code, a 24h token would be immediately refreshed after being fetched. We do not want that.
This PR changes this delay from 2 days to 10 minutes.
This is a margin to be sure the token does not expire in the middle of a critical action such as a list sync/fetch/post.
It could be any other delay, but not bigger than 24h.
Refs :