Skip to content

chore: use access token expiration for proactive auth refresh#15545

Merged
celia-oai merged 4 commits intomainfrom
dev/cc/exp
Mar 24, 2026
Merged

chore: use access token expiration for proactive auth refresh#15545
celia-oai merged 4 commits intomainfrom
dev/cc/exp

Conversation

@celia-oai
Copy link
Collaborator

@celia-oai celia-oai commented Mar 23, 2026

Follow up to #15357 by making proactive ChatGPT auth refresh depend on the access token's JWT expiration instead of treating last_refresh age as the primary source of truth.

@celia-oai celia-oai changed the title changes chore: u se access token expiration for proactive auth refresh Mar 23, 2026
@celia-oai celia-oai changed the title chore: u se access token expiration for proactive auth refresh chore: use access token expiration for proactive auth refresh Mar 23, 2026
@celia-oai celia-oai marked this pull request as ready for review March 23, 2026 19:57
@celia-oai celia-oai requested a review from pakrym-oai March 23, 2026 19:57
if let Some(tokens) = auth_dot_json.tokens.as_ref()
&& let Ok(Some(expires_at)) = parse_jwt_expiration(&tokens.access_token)
{
return expires_at <= Utc::now();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want a buffer? TOKEN_REFRESH_INTERVAL?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need a buffer? expired_at is a timestamp? we still have the fallback code path that uses Token_refresh_interval after this in case exp field doesn't exist in token?

Copy link
Collaborator

@pakrym-oai pakrym-oai Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need that codepath, can "exp" not be there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically, jwt token doesn't necessarily have the 'exp' field. In reality, the auth token returned should always have this field. We can also throw an error here if exp doesn't exist, but I think having this silent fallback is safer

}

#[derive(Deserialize)]
struct StandardJwtClaims {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why a separate struct? can we put this onto AuthClaims?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or IdClaims ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function is supposed to be generic so it can parse any jwt, not just id / access tokens I think

@celia-oai celia-oai enabled auto-merge (squash) March 23, 2026 23:58
@celia-oai celia-oai merged commit 7dc2cd2 into main Mar 24, 2026
85 of 89 checks passed
@celia-oai celia-oai deleted the dev/cc/exp branch March 24, 2026 19:34
@github-actions github-actions bot locked and limited conversation to collaborators Mar 24, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants