Re: [PATCH 1/4] http: fix retry count and Retry-After timing

[Junio C Hamano <gitster@pobox.com>]

"Ravi Mistry via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ravi Mistry <rmistry@google.com>
>
> 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 <rmistry@google.com>
> ---
>  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;