Re: [PATCH 2/4] http: add exponential backoff without Retry-After
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 3, 2026, 13:21 UTC
- Message-ID
- <xmqqpkwmaw51.fsf@gitster.g>
- In-Reply-To
- <36cef34668b6e2ceb867ca99029e8bc11d9cd63f.1793462224.git.gitgitgadget@gmail.com>
"Ravi Mistry via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 16 quoted lines
> 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).
Show 7 quoted lines
> 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.
Show 8 quoted lines
> @@ -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).
Show 29 quoted lines
> -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.
Show 16 quoted lines
> @@ -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.