Re: [PATCH 2/4] http: add exponential backoff without Retry-After

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

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

> From: Ravi Mistry <rmistry@google.com>
>
> When a server returns HTTP 429 without a Retry-After header, Git waits
> the same fixed http.retryAfter delay on every retry. If that delay is
> short (e.g., 1s), Git burns through all of its retries in just a few
> seconds before a busy server can recover. If that delay is long, even a
> brief glitch has to wait a long time on the first retry.
>
> Pass the 1-based retry attempt count to handle_rate_limit_retry() and
> double http.retryAfter on each subsequent retry (1s, 2s, 4s, ..., via
> http.retryAfter * 2^(attempt - 1)), clamping the delay at
> http.maxRetryTime. This keeps the first retry fast while giving the
> server more time to recover on later attempts, matching curl's own
> --retry behavior. When the server provides an explicit Retry-After
> header, continue to honor the server-provided delay (or fail fast if it
> exceeds http.maxRetryTime).

The logic makes perfect sense (I am taking the above description at
face value without checking if this really matches cURL's behavior
myself, though).

>  http.retryAfter::
> +	Initial wait time in seconds before retrying when a server returns
>  	HTTP 429 (Too Many Requests) without a Retry-After header.
> +	Defaults to 0 (retry immediately). When set to a positive value and
> +	no Retry-After header is present, the delay doubles on each
> +	subsequent retry attempt (`http.retryAfter * 2^(attempt - 1)`),
> +	capped at `http.maxRetryTime`. When a Retry-After header is

It somehow feels that the default value of 0 (immediate retry) does
not mesh well with exponential backoff.  Will we keep retrying
immediately after failing until we run out of retries?

Also, it feels somewhat inconsistent with how maxretrytime is
currently treated.  Specifically, when the other side tells us via
Retry-After, or the configuration specifies via http.retryAfter, to
wait longer than maxretrytime, instead of capping the wait, we
immediately give up without retrying.  But when retryAfter grows
with exponential backoff, the wait is capped at maxretrytime instead
of resulting in an immediate failure.

The handling of maxretrytime in this definition, where it caps the
waiting time before retrying, is definitely more useful than the
traditional "if we are told to wait longer than this, we fail
instead of capping", I would think.

> @@ -348,7 +351,9 @@ http.maxRetryTime::
>  	handling HTTP 429 (Too Many Requests) responses. If the server
>  	requests a delay (via Retry-After header) or if `http.retryAfter`
>  	is configured with a value that exceeds this maximum, Git will fail
> -	immediately rather than waiting. Default is 300 seconds (5 minutes).
> +	immediately rather than waiting. Exponentially backed-off delays
> +	computed from `http.retryAfter` are capped at this maximum. Default
> +	is 300 seconds (5 minutes).

> -static long handle_rate_limit_retry(long slot_retry_after)
> +static long handle_rate_limit_retry(long slot_retry_after, int attempt)
>  {
>  	/* Use the slot-specific retry_after value or configured default */
>  	if (slot_retry_after >= 0) {
> @@ -2427,6 +2427,8 @@ static long handle_rate_limit_retry(long slot_retry_after)
>  		}
>  		return slot_retry_after;
>  	} else {
> +		long delay = http_retry_after;
> +
>  		/* No Retry-After header provided, use configured default */
>  		if (http_retry_after > http_max_retry_time) {
>  			error(_("configured http.retryAfter exceeds http.maxRetryTime (%ld > %ld seconds)"),
> @@ -2437,7 +2439,14 @@ static long handle_rate_limit_retry(long slot_retry_after)
>  		}
>  		trace2_data_string("http", the_repository,
>  			"http/429-retry-source", "config-default");
> -		return http_retry_after;
> +		for (int a = 1; a < attempt && delay > 0; a++) {
> +			if (delay >= http_max_retry_time - delay) {
> +				delay = http_max_retry_time;
> +				break;
> +			}
> +			delay *= 2;
> +		}
> +		return delay;
>  	}
>  }

I guess what is in the loop is a tricky arithmetic to compute

	delay = ((delay * 2) > http_max_retry_time)
		? http_max_retry_time
		: (delay * 2);

without integer overflow?  Very effective.


> @@ -2449,6 +2458,7 @@ static int http_request_recoverable(const char *url,
>  	int i = 3;
>  	int ret;
>  	int rate_limit_retries = http_max_retries;
> +	int attempt = 0;
>  
>  	if (!options)
>  		options = &empty_opts;
> @@ -2498,7 +2508,7 @@ static int http_request_recoverable(const char *url,
>  			BUG("Unknown http_request target");
>  		}
>  		if (ret == HTTP_RATE_LIMITED) {
> -			retry_delay = handle_rate_limit_retry(options->retry_after);
> +			retry_delay = handle_rate_limit_retry(options->retry_after, ++attempt);
>  			if (retry_delay < 0)
>  				return HTTP_ERROR;

Interesting.

We already have to deal with the maximum number of retries to honor
http_max_retries, so we must be counting how many times we have
retried so far.  But we still need to introduce another variable to
count the number of attempts.

Or would it suffice to pass (http_max_retries - rate_limit_retries)
in place of the 'attempt' parameter?

Not complaining.  Just making an observation.