[PATCH v3 5/5] http: default http.retryAfter to 10 seconds

["Ravi Mistry via GitGitGadget" <gitgitgadget@gmail.com>]

From: Ravi Mistry <rmistry@google.com>

When http.maxRetries is enabled and a server returns HTTP 429 (or an
opt-in status code via http.retryStatusCodes) without a Retry-After
header, http.retryAfter currently defaults to 0. That causes Git to
retry immediately in a tight loop on every attempt, defeating
exponential backoff and continuing to hammer a rate-limiting server
right after it asked the client to back off.

Default http.retryAfter to 10 seconds (backing off as 10s, 20s, 40s,
..., capped at http.maxRetryTime) so that overloaded Git servers get
meaningful breathing room while keeping interactive commands reasonably
responsive. Callers or tests that want immediate retries can still set
http.retryAfter=0 explicitly.

Because http.retryAfter can now exceed a lowered http.maxRetryTime even
when http.retryAfter itself was not explicitly configured, drop the word
"configured" from the error message and config documentation. Also
update the past HTTP-date test in t5584: CURLINFO_RETRY_AFTER only
yields a positive delay for future timestamps, so past dates fall back
to http.retryAfter (which previously waited 0s only because the default
was 0).

Signed-off-by: Ravi Mistry <rmistry@google.com>
---
 Documentation/config/http.adoc | 25 +++++++++++++------------
 http.c                         |  4 ++--
 t/t5584-http-429-retry.sh      | 26 ++++++++++++++++++++------
 3 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/Documentation/config/http.adoc b/Documentation/config/http.adoc
index e7fa251b65..aa86bc68d5 100644
--- a/Documentation/config/http.adoc
+++ b/Documentation/config/http.adoc
@@ -332,16 +332,17 @@ http.retryAfter::
 	set of retryable status codes is configured by
 	`http.retryStatusCodes`, which includes only HTTP 429 (Too Many
 	Requests) by default.
-	Defaults to 0, which retries immediately without waiting on every
-	attempt (unless a Retry-After header is present). When set to a
-	positive value and no Retry-After header is present, the first
-	retry waits `http.retryAfter` seconds and the delay doubles on
-	each subsequent retry attempt (`http.retryAfter * 2^(attempt - 1)`),
-	capped at `http.maxRetryTime`. When a Retry-After header is
-	present, its value takes precedence over this setting; however,
-	automatic use of the server-provided `Retry-After` header requires
-	libcurl 7.66.0 or later. On older versions, configure this setting
-	manually to control the retry delay. Can be overridden by the
+	Defaults to 10. When positive and no Retry-After header is
+	present, the first retry waits `http.retryAfter` seconds and the
+	delay doubles on each subsequent retry attempt
+	(`http.retryAfter * 2^(attempt - 1)`), capped at
+	`http.maxRetryTime`. Set to 0 to retry immediately without waiting
+	on every attempt (unless a Retry-After header is present). When a
+	Retry-After header with a positive delay is present, its value
+	takes precedence over this setting; however, automatic use of the
+	server-provided `Retry-After` header requires libcurl 7.66.0 or
+	later. On older versions, `Retry-After` headers are ignored and
+	`http.retryAfter` is always used. Can be overridden by the
 	`GIT_HTTP_RETRY_AFTER` environment variable.
 	See also `http.maxRetries`, `http.maxRetryTime`, and
 	`http.retryStatusCodes`.
@@ -360,8 +361,8 @@ http.maxRetryTime::
 	handling a retryable HTTP status code. The set of retryable
 	status codes is configured by `http.retryStatusCodes`, which
 	includes only HTTP 429 (Too Many Requests) by default.
-	If the server requests a delay (via Retry-After header) or if the
-	configured `http.retryAfter` value itself exceeds this maximum,
+	If the server requests a delay (via Retry-After header) or if
+	`http.retryAfter` itself exceeds this maximum,
 	Git will fail immediately rather than waiting. Exponentially
 	backed-off delays computed from `http.retryAfter`, however, are
 	capped at this maximum rather than causing an immediate failure.
diff --git a/http.c b/http.c
index ea4f9245c4..4ce662ffe4 100644
--- a/http.c
+++ b/http.c
@@ -153,7 +153,7 @@ static char *http_ssl_backend;
 
 static int http_schannel_check_revoke = 1;
 
-static long http_retry_after = 0;
+static long http_retry_after = 10;
 static long http_max_retries = 0;
 static long http_max_retry_time = 300;
 static char *http_retry_status_codes;
@@ -2460,7 +2460,7 @@ static long handle_rate_limit_retry(long slot_retry_after, int attempt)
 
 		/* 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)"),
+			error(_("http.retryAfter exceeds http.maxRetryTime (%ld > %ld seconds)"),
 			      http_retry_after, http_max_retry_time);
 			trace2_data_string("http", the_repository,
 					"http/429-error", "config-exceeds-max-retry-time");
diff --git a/t/t5584-http-429-retry.sh b/t/t5584-http-429-retry.sh
index e627aa2b63..388c5db1b6 100755
--- a/t/t5584-http-429-retry.sh
+++ b/t/t5584-http-429-retry.sh
@@ -53,12 +53,14 @@ test_expect_success 'HTTP 429 with Retry-After is retried and succeeds' '
 
 test_expect_success 'http.maxRetries=1 retries once and succeeds' '
 	test_config http.maxRetries 1 &&
+	test_config http.retryAfter 0 &&
 	git ls-remote "$HTTPD_URL/http_429/max-retries-one/none/repo.git" >output 2>err &&
 	test_grep "refs/heads/" output
 '
 
 test_expect_success 'http.maxRetries=2 fails after 3 failures and succeeds on 2 failures' '
 	test_config http.maxRetries 2 &&
+	test_config http.retryAfter 0 &&
 	test_must_fail git ls-remote "$HTTPD_URL/http_429/max-retries-two-fail-3/none/repo.git" 2>err &&
 	test_grep "rate limited" err &&
 	git ls-remote "$HTTPD_URL/http_429/max-retries-two-fail-2/none/repo.git" >output 2>err &&
@@ -72,6 +74,13 @@ test_expect_success 'Trace2 logs actual CURLINFO_RETRY_AFTER value instead of -1
 	test_grep "\"key\":\"http/429-retry-after\",\"value\":\"1\"" trace-retry-after.event
 '
 
+test_expect_success 'http.retryAfter defaults to 10 seconds when unset' '
+	test_config http.maxRetries 1 &&
+	test_config http.maxRetryTime 9 &&
+	test_must_fail git ls-remote "$HTTPD_URL/http_429/default-retry-after-10/none/repo.git" 2>err &&
+	test_grep "http.retryAfter exceeds http.maxRetryTime (10 > 9 seconds)" err
+'
+
 test_expect_success 'HTTP 429 without Retry-After uses configured default' '
 	# Enable retries and configure default delay
 	test_config http.maxRetries 3 &&
@@ -161,7 +170,7 @@ test_expect_success 'HTTP 429 fails if configured http.retryAfter exceeds http.m
 	# Should fail quickly (no 100 second wait)
 	duration_int=${duration%.*} &&
 	test "$duration_int" -lt 99 &&
-	test_grep "configured http.retryAfter.*exceeds.*http.maxRetryTime" err
+	test_grep "http.retryAfter.*exceeds.*http.maxRetryTime" err
 '
 
 test_expect_success 'HTTP 429 with Retry-After HTTP-date format' '
@@ -210,7 +219,7 @@ test_expect_success 'HTTP 429 with HTTP-date exceeding maxRetryTime fails immedi
 	test_grep "http.maxRetryTime" err
 '
 
-test_expect_success 'HTTP 429 with past HTTP-date should not wait' '
+test_expect_success 'HTTP 429 with past HTTP-date falls back to http.retryAfter' '
 	raw=$(test-tool date timestamp now) &&
 	now="${raw#* -> }" &&
 	past_time=$((now - 10)) &&
@@ -218,18 +227,21 @@ test_expect_success 'HTTP 429 with past HTTP-date should not wait' '
 	past_date="${raw#* -> }" &&
 	past_date_encoded=$(echo "$past_date" | sed "s/ /%20/g") &&
 
-	# Enable retries
+	# Enable retries with a short fallback delay
 	test_config http.maxRetries 3 &&
+	test_config http.retryAfter 1 &&
 
-	# Git should retry immediately without waiting
+	# CURLINFO_RETRY_AFTER does not report a positive delay for past dates,
+	# so Git falls back to http.retryAfter rather than retrying immediately
 	start=$(test-tool date getnanos) &&
 	git ls-remote "$HTTPD_URL/http_429/past-http-date/$past_date_encoded/repo.git" >output 2>err &&
 	duration=$(test-tool date getnanos $start) &&
 
-	# Should complete quickly (no wait for a past-date Retry-After)
 	duration_int=${duration%.*} &&
+	test "$duration_int" -ge 1 &&
 	test "$duration_int" -lt 5 &&
-	test_grep "refs/heads/" output
+	test_grep "refs/heads/" output &&
+	test_grep "waiting 1 seconds before retry" err
 '
 
 test_expect_success 'HTTP 429 with invalid Retry-After format uses configured default' '
@@ -349,6 +361,7 @@ test_expect_success 'HTTP 503 honors Retry-After header when present' '
 
 test_expect_success 'exhausted 502 retries report HTTP 502 error, not rate limited' '
 	test_config http.maxRetries 2 &&
+	test_config http.retryAfter 0 &&
 	test_config http.retryStatusCodes "429,502" &&
 	test_must_fail git ls-remote "$HTTPD_URL/http_429/exhausted-fail-3-status-502/none/repo.git" 2>err &&
 	test_grep "The requested URL returned error: 502" err &&
@@ -357,6 +370,7 @@ test_expect_success 'exhausted 502 retries report HTTP 502 error, not rate limit
 
 test_expect_success 'GIT_HTTP_RETRY_STATUS_CODES and urlmatch http.<url>.retryStatusCodes' '
 	test_config http.maxRetries 2 &&
+	test_config http.retryAfter 0 &&
 	GIT_HTTP_RETRY_STATUS_CODES="429,502" \
 		git ls-remote "$HTTPD_URL/http_429/env-status-502/none/repo.git" >output 2>err &&
 	test_grep "refs/heads/" output &&
-- 
gitgitgadget