Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions Documentation/config/http.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,30 @@ http.keepAliveCount::
unset, curl's default value is used. Can be overridden by the
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Taylor Blau wrote (reply to this):

On Wed, Nov 26, 2025 at 12:30:25PM +0000, Vaidas Pilkauskas via GitGitGadget wrote:
> Retry behavior is controlled by three new configuration options:
>
>   * http.maxRetries: Maximum number of retry attempts (default: 0,
>     meaning retries are disabled by default). Users must explicitly
>     opt-in to retry behavior.
>
>   * http.retryAfter: Default delay in seconds when the server doesn't
>     provide a Retry-After header (default: -1, meaning fail if no
>     header is provided). This serves as a fallback mechanism.
>
>   * http.maxRetryTime: Maximum delay in seconds for a single retry
>     (default: 300). If the server requests a delay exceeding this
>     limit, Git fails immediately rather than waiting. This prevents
>     indefinite blocking on unreasonable server requests.
>
> All three options can be overridden via environment variables:
> GIT_HTTP_MAX_RETRIES, GIT_HTTP_RETRY_AFTER, and
> GIT_HTTP_MAX_RETRY_TIME.

This is great information, and I am glad that it is written down in
http.adoc so that it shows up in git-config(1). I think that it's fine
to omit this level of detail from the commit message, since it
duplicates information from the authoritative source on configuration
knobs.

It might be reasonable to say something like:

    Retry behavior is controlled by three new configuration options
    (http.maxRetries, http.retryAfter, and http.maxRetryTime) which are
    documented in git-config(1).

or something.

> diff --git a/http-push.c b/http-push.c
> index d86ce77119..a602a302ec 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -716,6 +716,10 @@ static int fetch_indices(void)
>  	case HTTP_MISSING_TARGET:
>  		ret = 0;
>  		break;
> +	case HTTP_RATE_LIMITED:
> +		error("rate limited by '%s', please try again later", repo->url);
> +		ret = -1;

Other strings in this file aren't marked for translation, but I think
we can/should mark this one like so:

    error(_("rate limited by %s ..."), repo->url);

> diff --git a/http.c b/http.c
> index 41f850db16..212805cad5 100644
> --- a/http.c
> +++ b/http.c
> @@ -22,6 +22,7 @@
>  #include "object-file.h"
>  #include "odb.h"
>  #include "tempfile.h"
> +#include "date.h"
>
>  static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
>  static int trace_curl_data = 1;
> @@ -149,6 +150,14 @@ static char *cached_accept_language;
>  static char *http_ssl_backend;
>
>  static int http_schannel_check_revoke = 1;
> +
> +/* Retry configuration */
> +static long http_retry_after = -1; /* Default retry-after in seconds when header is missing (-1 means not set, exit with 128) */
> +static long http_max_retries = 0; /* Maximum number of retry attempts (0 means retries are disabled) */
> +static long http_max_retry_time = 300; /* Maximum time to wait for a single retry (default 5 minutes) */

These comments should be OK to drop, the variables indicate what Git
configuration they correspond to (e.g., http_retry_after ->
http.retryAfter), so git-config(1) is the authoritative source for
documentation here.

> @@ -257,6 +267,47 @@ static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p UN
>  		goto exit;
>  	}
>
> +	/* Parse Retry-After header for rate limiting */
> +	if (skip_iprefix_mem(ptr, size, "retry-after:", &val, &val_len)) {

Makes sense, though I wonder if we should rename this function, since
fwrite_wwwauth is now doing more than just handling WWW-Authenticate
headers.

Perhaps we should have a single top-level function that is registered as
our CURLOPT_HEADERFUNCTION that dispatches calls to header-specific
functions? Otherwise the actual parsing of the Retry-After header looks
good to me.

> @@ -1422,6 +1488,10 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
>  	set_long_from_env(&curl_tcp_keepintvl, "GIT_TCP_KEEPINTVL");
>  	set_long_from_env(&curl_tcp_keepcnt, "GIT_TCP_KEEPCNT");
>
> +	set_long_from_env(&http_retry_after, "GIT_HTTP_RETRY_AFTER");
> +	set_long_from_env(&http_max_retries, "GIT_HTTP_MAX_RETRIES");
> +	set_long_from_env(&http_max_retry_time, "GIT_HTTP_MAX_RETRY_TIME");
> +

The configuration handling and overrides look good to me.

> @@ -2253,19 +2330,36 @@ static int update_url_from_redirect(struct strbuf *base,
>  	return 1;
>  }
>
> +/*
> + * Sleep for the specified number of seconds before retrying.
> + */
> +static void sleep_for_retry(long retry_after)
> +{
> +	if (retry_after > 0) {
> +		unsigned int remaining;
> +		warning(_("rate limited, waiting %ld seconds before retry"), retry_after);
> +		remaining = sleep(retry_after);

What should we do if there are other active request slots? It has been a
couple of years since I have looked at Git's HTTP code, but I imagine
that we should be able to continue processing other requests while
waiting for the retry-after period to elapse here.

> @@ -2302,7 +2396,54 @@ static int http_request_reauth(const char *url,
>  			BUG("Unknown http_request target");
>  		}
>
> -		credential_fill(the_repository, &http_auth, 1);
> +		if (ret == HTTP_RATE_LIMITED) {

Should handling the retry behavior be moved into a separate function? I
think that http_request_reauth() might be clearer if it read:

    if (ret == HTTP_RATE_LIMITED)
      apply_rate_limit(...); /* presumably with a better name */
    else
      credential_fill(...);

, and likewise, should we rename this function as it is no longer just
re-authenticating HTTP requests?

> diff --git a/t/t5584-http-429-retry.sh b/t/t5584-http-429-retry.sh
> new file mode 100755
> index 0000000000..8bcc382763
> --- /dev/null
> +++ b/t/t5584-http-429-retry.sh
> @@ -0,0 +1,429 @@
> +#!/bin/sh
> +
> +test_description='test HTTP 429 Too Many Requests retry logic'
> +
> +. ./test-lib.sh
> +
> +. "$TEST_DIRECTORY"/lib-httpd.sh
> +
> +start_httpd
> +
> +test_expect_success 'setup test repository' '
> +	test_commit initial &&
> +	git clone --bare . "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> +	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" config http.receivepack true
> +'
> +
> +test_expect_success 'HTTP 429 with retries disabled (maxRetries=0) fails immediately' '
> +	write_script "$HTTPD_ROOT_PATH/one-time-script" <<-\EOF &&
> +	printf "Status: 429 Too Many Requests\r\n"
> +	printf "Retry-After: 1\r\n"
> +	printf "Content-Type: text/plain\r\n"
> +	printf "\r\n"
> +	printf "Rate limited\n"
> +	cat "$1" >/dev/null
> +	EOF

To avoid having to write this script multiple write, you can write it as
a separate script in t/lib-httpd and then make sure to list it in
prepare_httpd() (from t/lib-httpd.sh).

You can then list it in the apache.conf in the same directory and invoke
it however you like. If you need to take in arguments to the script
(e.g., to change the Retry-After value), you can use a ScriptAliasMatch
instead of a normal ScriptAlias to pass in extra parameters from the URL.

The one-time-script mechanism here will cause the test harness to delete
the script after its first (and only) use, which can be useful for some
cases but I suspect is not necessary for all of these tests.
> +
> +	# Set maxRetries to 0 (disabled)
> +	test_config http.maxRetries 0 &&
> +	test_config http.retryAfter 1 &&
> +
> +	# Should fail immediately without any retry attempt
> +	test_must_fail git ls-remote "$HTTPD_URL/one_time_script/repo.git" 2>err &&
> +
> +	# Verify no retry happened (no "waiting" message in stderr)
> +	! grep -i "waiting.*retry" err &&

test_grep can be helpful when reading the output of test failures, since
it dumps the contents of the file it was searching. Just make sure to
write "test_grep !" instead of "! test_grep" (there are a few such
instances of the latter that I just wrote patches to clean up).

"! test_grep" isn't *wrong* per-se, but it will pollute the test output
with "couldn't find xyz in abc".

I skimmed through the the remainder of the tests since I imagine that
they will change substantially after writing the script out explicitly
instead of using one-time-script, so I'll hold off on reviewing that
portion in more detail until then.

Thanks,
Taylor

`GIT_HTTP_KEEPALIVE_COUNT` environment variable.

http.retryAfter::
Default wait time in seconds before retrying when a server returns
HTTP 429 (Too Many Requests) without a Retry-After header. If set
to -1 (the default), Git will fail immediately when encountering
a 429 response without a Retry-After header. When a Retry-After
header is present, its value takes precedence over this setting.
Can be overridden by the `GIT_HTTP_RETRY_AFTER` environment variable.
See also `http.maxRetries` and `http.maxRetryTime`.

http.maxRetries::
Maximum number of times to retry after receiving HTTP 429 (Too Many
Requests) responses. Set to 0 (the default) to disable retries.
Can be overridden by the `GIT_HTTP_MAX_RETRIES` environment variable.
See also `http.retryAfter` and `http.maxRetryTime`.

http.maxRetryTime::
Maximum time in seconds to wait for a single retry attempt when
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).
Can be overridden by the `GIT_HTTP_MAX_RETRY_TIME` environment
variable. See also `http.retryAfter` and `http.maxRetries`.

http.noEPSV::
A boolean which disables using of EPSV ftp command by curl.
This can be helpful with some "poor" ftp servers which don't
Expand Down
8 changes: 8 additions & 0 deletions http-push.c
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,10 @@ static int fetch_indices(void)
case HTTP_MISSING_TARGET:
ret = 0;
break;
case HTTP_RATE_LIMITED:
error("rate limited by '%s', please try again later", repo->url);
ret = -1;
break;
default:
ret = -1;
}
Expand Down Expand Up @@ -1548,6 +1552,10 @@ static int remote_exists(const char *path)
case HTTP_MISSING_TARGET:
ret = 0;
break;
case HTTP_RATE_LIMITED:
error("rate limited by '%s', please try again later", url);
ret = -1;
break;
case HTTP_ERROR:
error("unable to access '%s': %s", url, curl_errorstr);
/* fallthrough */
Expand Down
5 changes: 5 additions & 0 deletions http-walker.c
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,11 @@ static int fetch_indices(struct walker *walker, struct alt_base *repo)
repo->got_indices = 1;
ret = 0;
break;
case HTTP_RATE_LIMITED:
error("rate limited by '%s', please try again later", repo->base);
repo->got_indices = 0;
ret = -1;
break;
default:
repo->got_indices = 0;
ret = -1;
Expand Down
171 changes: 167 additions & 4 deletions http.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
#include "object-file.h"
#include "odb.h"
#include "tempfile.h"
#include "date.h"
#include "trace2.h"

static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
static int trace_curl_data = 1;
Expand Down Expand Up @@ -149,6 +151,14 @@ static char *cached_accept_language;
static char *http_ssl_backend;

static int http_schannel_check_revoke = 1;

/* Retry configuration */
static long http_retry_after = -1; /* Default retry-after in seconds when header is missing (-1 means not set, exit with 128) */
static long http_max_retries = 0; /* Maximum number of retry attempts (0 means retries are disabled) */
static long http_max_retry_time = 300; /* Maximum time to wait for a single retry (default 5 minutes) */

/* Store retry_after value from 429 responses for retry logic (-1 = not set, 0 = retry immediately, >0 = delay in seconds) */
static long last_retry_after = -1;
/*
* With the backend being set to `schannel`, setting sslCAinfo would override
* the Certificate Store in cURL v7.60.0 and later, which is not what we want
Expand Down Expand Up @@ -209,13 +219,14 @@ static inline int is_hdr_continuation(const char *ptr, const size_t size)
return size && (*ptr == ' ' || *ptr == '\t');
}

static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p UNUSED)
static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p)
{
size_t size = eltsize * nmemb;
struct strvec *values = &http_auth.wwwauth_headers;
struct strbuf buf = STRBUF_INIT;
const char *val;
size_t val_len;
struct active_request_slot *slot = (struct active_request_slot *)p;

/*
* Header lines may not come NULL-terminated from libcurl so we must
Expand Down Expand Up @@ -257,6 +268,47 @@ static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p UN
goto exit;
}

/* Parse Retry-After header for rate limiting */
if (skip_iprefix_mem(ptr, size, "retry-after:", &val, &val_len)) {
strbuf_add(&buf, val, val_len);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fairly certain that this is the data that's leaked, which is the reason for this test failure.

Sadly, the reporting of these -leaks jobs leaves a lot to be desired.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fairly certain that this is the data that's leaked, which is the reason for this test failure.

Sadly, the reporting of these -leaks jobs leaves a lot to be desired.

I got tests passing with the following fix: d4a5896

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! And sorry for guiding you in the wrong direction :-(

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! And sorry for guiding you in the wrong direction :-(

@dscho, no worries, very appreciate you looking into the issue. Thank you!

strbuf_trim(&buf);

if (slot && slot->results) {
/* Parse the retry-after value (delay-seconds or HTTP-date) */
char *endptr;
long retry_after;

errno = 0;
retry_after = strtol(buf.buf, &endptr, 10);

/* Check if it's a valid integer (delay-seconds format) */
if (endptr != buf.buf && *endptr == '\0' &&
errno != ERANGE && retry_after > 0) {
slot->results->retry_after = retry_after;
} else {
/* Try parsing as HTTP-date format */
timestamp_t timestamp;
int offset;
if (!parse_date_basic(buf.buf, &timestamp, &offset)) {
/* Successfully parsed as date, calculate delay from now */
timestamp_t now = time(NULL);
if (timestamp > now) {
slot->results->retry_after = (long)(timestamp - now);
} else {
/* Past date means retry immediately */
slot->results->retry_after = 0;
}
} else {
/* Failed to parse as either delay-seconds or HTTP-date */
warning(_("unable to parse Retry-After header value: '%s'"), buf.buf);
}
}
}

http_auth.header_is_last_match = 1;
goto exit;
}

/*
* This line could be a continuation of the previously matched header
* field. If this is the case then we should append this value to the
Expand Down Expand Up @@ -575,6 +627,21 @@ static int http_options(const char *var, const char *value,
return 0;
}

if (!strcmp("http.retryafter", var)) {
http_retry_after = git_config_int(var, value, ctx->kvi);
return 0;
}

if (!strcmp("http.maxretries", var)) {
http_max_retries = git_config_int(var, value, ctx->kvi);
return 0;
}

if (!strcmp("http.maxretrytime", var)) {
http_max_retry_time = git_config_int(var, value, ctx->kvi);
return 0;
}

/* Fall back on the default ones */
return git_default_config(var, value, ctx, data);
}
Expand Down Expand Up @@ -1422,6 +1489,10 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
set_long_from_env(&curl_tcp_keepintvl, "GIT_TCP_KEEPINTVL");
set_long_from_env(&curl_tcp_keepcnt, "GIT_TCP_KEEPCNT");

set_long_from_env(&http_retry_after, "GIT_HTTP_RETRY_AFTER");
set_long_from_env(&http_max_retries, "GIT_HTTP_MAX_RETRIES");
set_long_from_env(&http_max_retry_time, "GIT_HTTP_MAX_RETRY_TIME");

curl_default = get_curl_handle();
}

Expand Down Expand Up @@ -1871,6 +1942,12 @@ static int handle_curl_result(struct slot_results *results)
}
return HTTP_REAUTH;
}
} else if (results->http_code == 429) {
/* Store the retry_after value for use in retry logic */
last_retry_after = results->retry_after;
trace2_data_intmax("http", the_repository, "http/429-retry-after",
last_retry_after);
return HTTP_RATE_LIMITED;
} else {
if (results->http_connectcode == 407)
credential_reject(the_repository, &proxy_auth);
Expand All @@ -1886,6 +1963,8 @@ int run_one_slot(struct active_request_slot *slot,
struct slot_results *results)
{
slot->results = results;
/* Initialize retry_after to -1 (not set) */
results->retry_after = -1;
if (!start_active_slot(slot)) {
xsnprintf(curl_errorstr, sizeof(curl_errorstr),
"failed to start HTTP request");
Expand Down Expand Up @@ -2149,6 +2228,7 @@ static int http_request(const char *url,
}

curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_wwwauth);
curl_easy_setopt(slot->curl, CURLOPT_HEADERDATA, slot);

accept_language = http_get_accept_language_header();

Expand Down Expand Up @@ -2253,19 +2333,40 @@ static int update_url_from_redirect(struct strbuf *base,
return 1;
}

/*
* Sleep for the specified number of seconds before retrying.
*/
static void sleep_for_retry(long retry_after)
{
if (retry_after > 0) {
unsigned int remaining;
warning(_("rate limited, waiting %ld seconds before retry"), retry_after);
trace2_region_enter("http", "retry-sleep", the_repository);
trace2_data_intmax("http", the_repository, "http/retry-sleep-seconds",
retry_after);
remaining = sleep(retry_after);
while (remaining > 0) {
/* Sleep was interrupted, continue sleeping */
remaining = sleep(remaining);
}
trace2_region_leave("http", "retry-sleep", the_repository);
}
}

static int http_request_reauth(const char *url,
void *result, int target,
struct http_get_options *options)
{
int i = 3;
int ret;
int rate_limit_retries = http_max_retries;

if (always_auth_proactively())
credential_fill(the_repository, &http_auth, 1);

ret = http_request(url, result, target, options);

if (ret != HTTP_OK && ret != HTTP_REAUTH)
if (ret != HTTP_OK && ret != HTTP_REAUTH && ret != HTTP_RATE_LIMITED)
return ret;

if (options && options->effective_url && options->base_url) {
Expand All @@ -2276,7 +2377,7 @@ static int http_request_reauth(const char *url,
}
}

while (ret == HTTP_REAUTH && --i) {
while ((ret == HTTP_REAUTH || ret == HTTP_RATE_LIMITED) && --i) {
/*
* The previous request may have put cruft into our output stream; we
* should clear it out before making our next request.
Expand All @@ -2302,7 +2403,69 @@ static int http_request_reauth(const char *url,
BUG("Unknown http_request target");
}

credential_fill(the_repository, &http_auth, 1);
if (ret == HTTP_RATE_LIMITED) {
/* Handle rate limiting with retry logic */
int retry_attempt = http_max_retries - rate_limit_retries + 1;

trace2_data_intmax("http", the_repository, "http/429-retry-attempt",
retry_attempt);

if (rate_limit_retries <= 0) {
/* Retries are disabled or exhausted */
if (http_max_retries > 0) {
error(_("too many rate limit retries, giving up"));
trace2_data_string("http", the_repository,
"http/429-error", "retries-exhausted");
}
return HTTP_ERROR;
}

/* Decrement retries counter */
rate_limit_retries--;

/* Use the stored retry_after value or configured default */
if (last_retry_after >= 0) {
/* Check if retry delay exceeds maximum allowed */
if (last_retry_after > http_max_retry_time) {
error(_("rate limited (HTTP 429) requested %ld second delay, "
"exceeds http.maxRetryTime of %ld seconds"),
last_retry_after, http_max_retry_time);
trace2_data_string("http", the_repository,
"http/429-error", "exceeds-max-retry-time");
trace2_data_intmax("http", the_repository,
"http/429-requested-delay", last_retry_after);
last_retry_after = -1; /* Reset after use */
return HTTP_ERROR;
}
sleep_for_retry(last_retry_after);
last_retry_after = -1; /* Reset after use */
} else {
/* No Retry-After header provided */
if (http_retry_after < 0) {
/* Not configured - exit with error */
error(_("rate limited (HTTP 429) and no Retry-After header provided. "
"Configure http.retryAfter or set GIT_HTTP_RETRY_AFTER."));
trace2_data_string("http", the_repository,
"http/429-error", "no-retry-after-config");
return HTTP_ERROR;
}
/* Check if configured default exceeds maximum allowed */
if (http_retry_after > http_max_retry_time) {
error(_("configured http.retryAfter (%ld seconds) exceeds "
"http.maxRetryTime (%ld seconds)"),
http_retry_after, http_max_retry_time);
trace2_data_string("http", the_repository,
"http/429-error", "config-exceeds-max-retry-time");
return HTTP_ERROR;
}
/* Use configured default retry-after value */
trace2_data_string("http", the_repository,
"http/429-retry-source", "config-default");
sleep_for_retry(http_retry_after);
}
} else if (ret == HTTP_REAUTH) {
credential_fill(the_repository, &http_auth, 1);
}

ret = http_request(url, result, target, options);
}
Expand Down
2 changes: 2 additions & 0 deletions http.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ struct slot_results {
long http_code;
long auth_avail;
long http_connectcode;
long retry_after;
};

struct active_request_slot {
Expand Down Expand Up @@ -167,6 +168,7 @@ struct http_get_options {
#define HTTP_REAUTH 4
#define HTTP_NOAUTH 5
#define HTTP_NOMATCHPUBLICKEY 6
#define HTTP_RATE_LIMITED 7

/*
* Requests a URL and stores the result in a strbuf.
Expand Down
Loading
Loading