Repository navigation
Fix concurrent token refresh marking healthy accounts for re-authentication - #1
Open
rayan-mazouz wants to merge 3 commits into
Open
rayan-mazouz wants to merge 3 commits into
rayan-mazouz wants to merge 3 commits into
Conversation
Both the fast path and the locked path decrypted the stored access token and flagged the account for re-authentication on failure. Share one helper so later changes to the locked path stay small.
ensure_fresh_token locked the row with SELECT ... FOR UPDATE and then re-checked the expiry, but the query returned the identity-mapped object with its previously loaded attributes. A worker that loaded the account before another worker rotated its tokens therefore saw the old expiry and spent the already-rotated refresh token. The provider rejected it and the healthy account was marked as needing re-authentication. Re-read the row with populate_existing() (flushing pending changes first so they are not discarded), and stop if the account was marked for re-authentication in the meantime.
When several in-flight requests hit a 401 on the same revoked token, each forced its own refresh, spending one refresh token per request. Pass the rejected access token instead of a force flag: under the row lock, a token that no longer matches it was already replaced by another worker and is returned as-is, so only the first request refreshes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Concurrent token refreshes could mark healthy accounts as needing re-authentication.
ensure_fresh_tokenlocks the account row withSELECT ... FOR UPDATEand re-checks the expiry before refreshing. But SQLAlchemy returns the identity-mapped object already in the session, without reloading its attributes. A worker that loaded the account before another worker rotated its tokens therefore still saw the old expiry and the old refresh token. It spent a refresh token that had already been rotated, the provider rejected it, and the account was flaggedREAUTH_REQUIREDwith an "authentication expired" notification. Two concurrent requests near token expiry are enough to trigger it, as is a request racing the quota refresher.Commits
populate_existing(), flushing pending changes first so they are not discarded. Also stops if the account was marked for re-authentication in the meantime.force_refresh=Trueis replaced byrejected_token=<the token that got the 401>. When several requests hit a 401 on the same token, only the first refreshes; the rest get its replacement.Testing
mainand pass on this branch. Before the fix, the stale worker spentupstream-refresh-tokeninstead of the rotated one.