[Issue #9021] Add retries if Login.gov fails when we ask for token details#9023
Open
[Issue #9021] Add retries if Login.gov fails when we ask for token details#9023
Conversation
chouinar
reviewed
Mar 13, 2026
Comment on lines
+11
to
14
| self.retries: dict[str, int] = {} | ||
|
|
||
| def add_token_response(self, code: str, response: OauthTokenResponse) -> None: | ||
| self.responses[code] = response |
Collaborator
There was a problem hiding this comment.
Rather than setting the retries by using the dict directly, what if we add a param to the add token function to do it?
Suggested change
| self.retries: dict[str, int] = {} | |
| def add_token_response(self, code: str, response: OauthTokenResponse) -> None: | |
| self.responses[code] = response | |
| self.retries: dict[str, int] = {} | |
| def add_token_response(self, code: str, response: OauthTokenResponse, retry_count: int = 0) -> None: | |
| self.responses[code] = response | |
| self.retries[code] = retry_count |
Then that simplifies the logic below a bit as it'll always be set.
Comment on lines
+178
to
+183
| else: | ||
| logger.info( | ||
| "Retrying call to Login.gov after receiving error", | ||
| extra={"tries": tries, "limit": limit}, | ||
| ) | ||
| continue |
Collaborator
There was a problem hiding this comment.
I think you meant to do this - as written it would always call 3 times as the loop never breaks before 3 tries.
Suggested change
| else: | |
| logger.info( | |
| "Retrying call to Login.gov after receiving error", | |
| extra={"tries": tries, "limit": limit}, | |
| ) | |
| continue | |
| else: | |
| logger.info( | |
| "Retrying call to Login.gov after receiving error", | |
| extra={"tries": tries, "limit": limit}, | |
| ) | |
| break |
Alternatively, if we wanted to use tenacity, seems they have a TryAgain exception we could use - although I don't think it's that big a deal.
| # If this request failed, we'll assume we're the issue and 500 | ||
| if response.is_error_response(): | ||
| raise_flask_error(500, response.error_description) | ||
| # If this request failed, we'll assume we're the issue and 500 |
Collaborator
There was a problem hiding this comment.
Probably should update this as with the retries it isn't quite right?
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
Fixes #9021
Changes proposed
Introduce a retry of up to 3 times for our call to Login.gov to fetch information about the token holder
Context for reviewers
We see occasional failures, once every few weeks, where upon a callback from Login.gov our request back to Login to get information about the holder of the token fails with a unspecified 500 error. This currently results in the user not successfully signing in. This change would give that user an opportunity to still get logged in.
Validation steps