sdk: retry all requests which previous response was a plain 429 without an errcode #3606
Conversation
…ut an errcode This can happen if there's a load-balancer or any modification of the response by a reverse proxy (e.g. rewrite 5XX errors into 429, to not let a reverse proxy mark the upstream server as being down, as Cloudflare seems to do). As a result, such requests will be retried in multiple places, including when sending something with the send queue. Also, the send queue will mark these errors as recoverable instead of unrecoverable. No test, because the change really is trivial and a regression test didn't seem worth it, for once.
bnjbvr
requested review from
Hywan,
a team and
poljar
and removed request for
a team and
Hywan
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3606 +/- ##
==========================================
+ Coverage 83.85% 83.87% +0.01%
==========================================
Files 254 254
Lines 26319 26319
==========================================
+ Hits 22069 22074 +5
+ Misses 4250 4245 -5 ☔ View full report in Codecov by Sentry. |
| }; | ||
|
|
||
| if let Some(status_code) = status_code { | ||
| if status_code.is_server_error() { | ||
| // If the status code is 429, this is requesting a retry in HTTP, without the |
There was a problem hiding this comment.
MDN tells us that the 429 might contain a retry-after value in the HTTP header1, are we sure that we don't want to attempt to fish that out of the headers?
Footnotes
There was a problem hiding this comment.
At this point we're operating from the HttpError, which contains only the status code and response body, so no more headers unfortunately.
There was a problem hiding this comment.
Ah right, I had trouble with this for the QR login as well. We probably should do something about this, one of these days... 🥲
This can happen if there's a load-balancer or any modification of the response by a reverse proxy (e.g. rewrite 5XX errors into 429, to not let a reverse proxy mark the upstream server as being down, as Cloudflare seems to do).
As a result, such requests will be retried in multiple places, including when sending something with the send queue. Also, the send queue will mark these errors as recoverable instead of unrecoverable.
No test, because the change really is trivial and a regression test didn't seem worth it, for once.
Part of #3361.