fix: token cache exits after exhaustive retries#4551
fix: token cache exits after exhaustive retries#4551alvarowolfx merged 23 commits intogoogleapis:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4551 +/- ##
==========================================
- Coverage 95.03% 95.02% -0.02%
==========================================
Files 195 195
Lines 7473 7475 +2
==========================================
+ Hits 7102 7103 +1
- Misses 371 372 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/auth/src/token_cache.rs
Outdated
| Err(err) => { | ||
| // The retry policy has been used already by the inner token provider. | ||
| // If it ended in an error, just quit the background task. | ||
| break; | ||
| // If it ended in an error, the background task will wait for a while | ||
| // and try again. This allows the task to eventually recover if the | ||
| // error was transient but exhausted all the retries. |
There was a problem hiding this comment.
This comment does not read right.
| Err(err) => { | |
| // The retry policy has been used already by the inner token provider. | |
| // If it ended in an error, just quit the background task. | |
| break; | |
| // If it ended in an error, the background task will wait for a while | |
| // and try again. This allows the task to eventually recover if the | |
| // error was transient but exhausted all the retries. | |
| Err(err) => { | |
| // On transient errors, even if the retry policy is exhausted, | |
| // we want to continue running this retry loop. | |
| // This loop cannot stop because that may leave the | |
| // credentials in an unrecoverable state (see #...). | |
| // We considered using a notification to wake up the next time | |
| // a caller wants to retrieve a token, but that seemed prone to | |
| // deadlocks. We may implement this as an improvement (#....). | |
| // On permanent errors, then there is really no point in trying | |
| // again, by definition of "permanent". If the error was misclassified | |
| // as permanent, that is a bug in the retry policy and better fixed | |
| // there than implemented as a workaround here. |
With prompts the question: are we certain our default policy does not misclassify some errors as permanent? Can you check?
There was a problem hiding this comment.
The retry loop always map errors to gax:authentication errors, so I think in that part is correct, so I don't any other gax error being raised. If was not that I would be worried because https://github.com/googleapis/google-cloud-rust/pull/4551/changes#diff-43612f24f6413f8e5304d6af3e912553d7913ceea55c203c549db570b52d0d30L133 treat all non auth errors as permanent.
In the auth side, I was doing some audit on every place that creates CredentialsError::from_msg(false, ...), CredentialsError::new(false,...), CredentialsError::from_source( or CredentialsError::from_http_response and they are mostly consider non transient errors the ones related to transport and decoding errors when fetching data from remote services, so I think that's correct.
I could not find yet any case of misclassification, but you raised a valid point
There was a problem hiding this comment.
I updated the comment and created an issue to track future improvement on the retry loop.
There was a problem hiding this comment.
I am not sure the parsing errors are permanent. We sent a request, got a bad response (sometimes the load balancers like to return silly things). The next attempt may succeed. OTOH, maybe the endpoint is configured wrong and we got a nice cocktail recipe 🤷
The error classification is too close to the source, we should let the retry policy decide what errors are transient vs. permanent, instead of hard-coding that decision in each function.
There was a problem hiding this comment.
Can you open a bug to track these things? It may be too late to change, but we should think it through.
coryan
left a comment
There was a problem hiding this comment.
I think this is better than what we have today, so approved. It could also get some improvements.
src/auth/src/token_cache.rs
Outdated
| Err(err) => { | ||
| // The retry policy has been used already by the inner token provider. | ||
| // If it ended in an error, just quit the background task. | ||
| break; | ||
| // If it ended in an error, the background task will wait for a while | ||
| // and try again. This allows the task to eventually recover if the | ||
| // error was transient but exhausted all the retries. |
There was a problem hiding this comment.
I am not sure the parsing errors are permanent. We sent a request, got a bad response (sometimes the load balancers like to return silly things). The next attempt may succeed. OTOH, maybe the endpoint is configured wrong and we got a nice cocktail recipe 🤷
The error classification is too close to the source, we should let the retry policy decide what errors are transient vs. permanent, instead of hard-coding that decision in each function.
dbolduc
left a comment
There was a problem hiding this comment.
(not important. I just like doing puzzles)
| let expiry = token_result.as_ref().ok().map(|t| t.expires_at); | ||
| let tagged = token_result.map(|token| { | ||
| let expiry = token_result.as_ref().map(|t| t.expires_at); | ||
| let tagged = token_result.clone().map(|token| { |
There was a problem hiding this comment.
silly nit: you can avoid the clone.
enum Refresh {
Deadline(Instant),
Never,
Soon,
}
let refresh = match token_result.as_ref() {
Ok(t) => {
match t.expires_at {
Some(d) => Refresh::Deadline(d),
None => Refresh::Never,
}
},
Err(e) if e.is_transient() => Refresh::Soon,
Err(_) => Refresh::Never,
};But maybe you think saving one clone is silly for a token that is cloned on every request anyway
Towards #4541