fix: Refresh hook race - #224
Merged
Merged
Conversation
…resh token is updated.
…n. The access token should be kept for the client, but sending both returns structured data and exposing that access key shouldn't impede the operation of the client.
for more information, see https://pre-commit.ci
…tiple concurrent requests can't race to update the access/refresh tokens.
… so a slow IdP authorization doesn't block requests, when used for re-authorization instead of initial auth.
piitaya
requested changes
Sep 1, 2026
piitaya
left a comment
piitaya
left a comment
Collaborator
There was a problem hiding this comment.
Good catch on the race.
Could you call the hook after releasing the lock? A hook that calls back into the client would otherwise wait forever on it.
Can we also keep set_refresh_token sync, since making it async silently breaks existing callers. A test with two concurrent refreshes would be nice too.
Contributor
Author
|
I have updated the actual code, but I will have to wait until I have some more time to do the new test case. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
I can't confidently commit this script as it was AI generate and I haven't checked it is properly safe. Github actions has also identified a potential source of HTML injection in the authentication error message.
…into refresh_hook_race
Contributor
Author
|
@piitaya Is there a chance you could re-review this PR? I would love to integrate this change into my changes to Music Assistant. |
piitaya
approved these changes
Sep 9, 2026
piitaya
left a comment
piitaya
left a comment
Collaborator
There was a problem hiding this comment.
Seems good ! Thank you !
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
3 participants
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.
If two requests,
R1andR2, call intoYotoClient'scheck_and_refresh_token, there is the potential to race to token updating and writing an invalidated token to the cache.Timeline:
R1andR2get the token and initiate a refresh.R1requests a new token and writes to the cache.R2requests a new token with the invalidated refresh token, causing the Yoto IdP to invalidate the refresh token issued in step 2.To combat this, there token has been locked in the three locations it is accessed:
set_refresh_tokenlocks the refresh token so it's not changed during the middle of another change.check_and_refresh_tokenwill hold the lock from the first time it check the token until the return of the function, including a potential change of token.device_code_flow_completepolls for a new token, then locks while updating the internal token and calling the refresh token hook.