From: Junio C Hamano Date: Tue, 03 Nov 2026 01:03:44 GMT Subject: Re: [PATCH 1/4] http: fix retry count and Retry-After timing Message-ID: In-Reply-To: <80b7f207f42cff0a3dfe23181ee315fdc90e50ff.1793462224.git.gitgitgadget@gmail.com> "Ravi Mistry via GitGitGadget" writes: > From: Ravi Mistry > > Commit 640657ffd0 (http: add support for HTTP 429 rate limit retries, > 2026-03-17) introduced two small bugs in HTTP 429 retry handling: > > 1. In http_request_recoverable(), rate_limit_retries is initialized to > http_max_retries (the number of retries to perform after the initial > failed request) and decremented with pre-decrement > (--rate_limit_retries) in the while condition. Because of the > pre-decrement, setting http.maxRetries=1 performs 0 retries, and > more generally N configured retries only retries N-1 times. In The new test exactly demonstrates it, which is fun. > In > addition, if the first request returns 401 (HTTP_REAUTH) and the > re-authenticated request returns 429 when http.maxRetries is 0, > --rate_limit_retries wraps from 0 to -1 (truthy in C). Hmph, which would mean we would retry essentially forever until the integer wraps around again? That sounds, eh, nasty. > Fix this by checking http_max_retries > 0 && rate_limit_retries-- > 0 > in the loop condition and returning HTTP_ERROR when > http_max_retries <= 0. OK. > 2. In http_request(), CURLINFO_RETRY_AFTER is queried after > run_one_slot() has already called handle_curl_result(), which logs > results->retry_after to Trace2 ("http/429-retry-after") while it is > still -1. Move the CURLINFO_RETRY_AFTER extraction into > finish_active_slot() (initializing slot->results->retry_after = -1 > first) so that both handle_curl_result() and callers of > run_one_slot() observe the parsed Retry-After value. > > Signed-off-by: Ravi Mistry > --- > http.c | 27 +++++++++++++-------------- > t/lib-httpd/http-429.sh | 31 ++++++++++++++++++++++++++----- > t/t5584-http-429-retry.sh | 21 +++++++++++++++++++++ > 3 files changed, 60 insertions(+), 19 deletions(-) > > diff --git a/http.c b/http.c > index d79678d01f..0308f973a6 100644 > --- a/http.c > +++ b/http.c > @@ -345,11 +345,20 @@ static void finish_active_slot(struct active_request_slot *slot) > if (slot->results) { > slot->results->curl_result = slot->curl_result; > slot->results->http_code = slot->http_code; > + slot->results->retry_after = -1; > curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL, > &slot->results->auth_avail); > > curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE, > &slot->results->http_connectcode); > +#ifdef GIT_CURL_HAVE_CURLINFO_RETRY_AFTER > + { > + curl_off_t retry_after; > + if (curl_easy_getinfo(slot->curl, CURLINFO_RETRY_AFTER, > + &retry_after) == CURLE_OK && retry_after > 0) > + slot->results->retry_after = (long)retry_after; > + } > +#endif > } > > /* Run callback if appropriate */ > @@ -2286,15 +2295,6 @@ static int http_request(const char *url, > > ret = run_one_slot(slot, &results); > > -#ifdef GIT_CURL_HAVE_CURLINFO_RETRY_AFTER > - if (ret == HTTP_RATE_LIMITED) { > - curl_off_t retry_after; > - if (curl_easy_getinfo(slot->curl, CURLINFO_RETRY_AFTER, > - &retry_after) == CURLE_OK && retry_after > 0) > - results.retry_after = (long)retry_after; > - } > -#endif > - OK, this is the fix #2 explained in the log message. > @@ -2461,10 +2461,6 @@ static int http_request_recoverable(const char *url, > if (ret != HTTP_OK && ret != HTTP_REAUTH && ret != HTTP_RATE_LIMITED) > return ret; > > - /* If retries are disabled and we got a 429, fail immediately */ > - if (ret == HTTP_RATE_LIMITED && !http_max_retries) > - return HTTP_ERROR; > - I am not sure I get this removal. If it is because the change to the next hunk (the condition to loop with the while statement) already covers it, I guess it makes sense, because we never enter the while() loop and we will return HTTP_ERROR more or less the same way as the above if http_max_retries is false. > while ((ret == HTTP_REAUTH && --i) || > - (ret == HTTP_RATE_LIMITED && --rate_limit_retries)) { > + (ret == HTTP_RATE_LIMITED && http_max_retries > 0 && > + rate_limit_retries-- > 0)) { > long retry_delay = -1; > /* > * The previous request may have put cruft into our output stream; we > @@ -2518,6 +2515,8 @@ static int http_request_recoverable(const char *url, > ret = http_request(url, result, target, options); > } > if (ret == HTTP_RATE_LIMITED) { > + if (http_max_retries <= 0) > + return HTTP_ERROR; > trace2_data_string("http", the_repository, > "http/429-error", "retries-exhausted"); > return HTTP_RATE_LIMITED;