http: add support for HTTP 429 rate limit retries #2008
Conversation
Welcome to GitGitGadgetHi @vaidas-shopify, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
vaidas-shopify
force-pushed
the
retry-after
branch
from
7c923b1 to
43a58bd
Compare
|
/allow |
|
User vaidas-shopify is now allowed to use GitGitGadget. |
vaidas-shopify
force-pushed
the
retry-after
branch
from
43a58bd to
2352f80
Compare
|
|
||
| /* Parse Retry-After header for rate limiting */ | ||
| if (skip_iprefix_mem(ptr, size, "retry-after:", &val, &val_len)) { | ||
| strbuf_add(&buf, val, val_len); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
-leaksjobs leaves a lot to be desired.
I got tests passing with the following fix: d4a5896
There was a problem hiding this comment.
Good catch! And sorry for guiding you in the wrong direction :-(
There was a problem hiding this comment.
Good catch! And sorry for guiding you in the wrong direction :-(
@dscho, no worries, very appreciate you looking into the issue. Thank you!
|
There are issues in commit 293af7e: |
vaidas-shopify
force-pushed
the
retry-after
branch
3 times, most recently
from
c9faf53 to
5b859d1
Compare
Range-diff
@vaidas-shopify without addressing the |
vaidas-shopify
force-pushed
the
retry-after
branch
3 times, most recently
from
84a40b2 to
eb66a3a
Compare
Add retry logic for HTTP 429 (Too Many Requests) responses to handle
server-side rate limiting gracefully. When Git's HTTP client receives
a 429 response, it can now automatically retry the request after an
appropriate delay, respecting the server's rate limits.
The implementation supports the RFC-compliant Retry-After header in
both delay-seconds (integer) and HTTP-date (RFC 2822) formats. If a
past date is provided, Git retries immediately without waiting.
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.
The retry logic implements a fail-fast approach: if any delay
(whether from server header or configuration) exceeds maxRetryTime,
Git fails immediately with a clear error message rather than capping
the delay. This provides better visibility into rate limiting issues.
The implementation includes extensive test coverage for basic retry
behavior, Retry-After header formats (integer and HTTP-date),
configuration combinations, maxRetryTime limits, invalid header
handling, environment variable overrides, and edge cases.
Signed-off-by: Vaidas Pilkauskas <vaidas.pilkauskas@shopify.com>
Fix a memory leak in show_http_message() that was triggered when displaying HTTP error messages before die(). The function would call strbuf_reencode() which modifies the caller's strbuf in place, allocating new memory for the re-encoded string. Since this function is only called immediately before die(), the allocated memory was never explicitly freed, causing leak detectors to report it. The leak became visible when HTTP 429 rate limit retry support was added, which introduced the HTTP_RATE_LIMITED error case. However, the issue existed in pre-existing error paths as well (HTTP_MISSING_TARGET, HTTP_NOAUTH, HTTP_NOMATCHPUBLICKEY) - the new retry logic just made it more visible in tests because retries exercise the error paths more frequently. The leak was detected by LeakSanitizer in t5584 tests that enable retries (maxRetries > 0). Tests with retries disabled passed because they took a different code path or timing. Fix this by making show_http_message() work on a local copy of the message buffer instead of modifying the caller's buffer in place: 1. Create a local strbuf and copy the message into it 2. Perform re-encoding on the local copy if needed 3. Display the message from the local copy 4. Properly release the local copy before returning This ensures all memory allocated by strbuf_reencode() is freed before the function returns, even though die() is called immediately after, eliminating the leak. Signed-off-by: Vaidas Pilkauskas <vaidas.pilkauskas@shopify.com>
Add trace2 instrumentation to HTTP 429 retry operations to enable
monitoring and debugging of rate limit scenarios in production
environments.
The trace2 logging captures:
* Retry attempt numbers (http/429-retry-attempt) to track retry
progression and identify how many attempts were needed
* Retry-After header values (http/429-retry-after) from server
responses to understand server-requested delays
* Actual sleep durations (http/retry-sleep-seconds) within trace2
regions (http/retry-sleep) to measure time spent waiting
* Error conditions (http/429-error) such as "retries-exhausted",
"exceeds-max-retry-time", "no-retry-after-config", and
"config-exceeds-max-retry-time" for diagnosing failures
* Retry source (http/429-retry-source) indicating whether delay
came from server header or config default
This instrumentation provides complete visibility into retry behavior,
enabling operators to monitor rate limiting patterns, diagnose retry
failures, and optimize retry configuration based on real-world data.
Signed-off-by: Vaidas Pilkauskas <vaidas.pilkauskas@shopify.com>
vaidas-shopify
force-pushed
the
retry-after
branch
from
eb66a3a to
adbcc02
Compare
|
/preview |
|
Preview email sent as pull.2008.git.1764159573.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.2008.git.1764160227.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
I've just created my first PR, could someone please /allow me? git#2111 |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| http.keepAliveCount:: | ||
| Specifies how many TCP keepalive probes to send before giving up | ||
| and terminating the connection (if supported by the OS). If | ||
| unset, curl's default value is used. Can be overridden by the |
There was a problem hiding this comment.
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
|
User |
| } | ||
|
|
||
| static int show_http_message(struct strbuf *type, struct strbuf *charset, | ||
| struct strbuf *msg) |
There was a problem hiding this comment.
On the Git mailing list, Taylor Blau wrote (reply to this):
On Wed, Nov 26, 2025 at 12:30:26PM +0000, Vaidas Pilkauskas via GitGitGadget wrote:
> diff --git a/remote-curl.c b/remote-curl.c
> index 5959461cd3..dd0680e5ae 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -371,6 +371,7 @@ static int show_http_message(struct strbuf *type, struct strbuf *charset,
> struct strbuf *msg)
> {
> const char *p, *eol;
> + struct strbuf msgbuf = STRBUF_INIT;
>
> /*
> * We only show text/plain parts, as other types are likely
> @@ -378,19 +379,24 @@ static int show_http_message(struct strbuf *type, struct strbuf *charset,
> */
> if (strcmp(type->buf, "text/plain"))
> return -1;
> +
> + strbuf_addbuf(&msgbuf, msg);
Hmm. Looking at the list of show_http_message() callers, it looks like
they all follow the pattern of constructing a strbuf "msg", passing it
to this function, and then calling die() with some user-friendly
message.
I agree that the patch here does address that leak, but I wonder if we
should do it in a way that doesn't involve copying the "msg" buffer. One
thing we could do is rename 'show_http_message()' to make it clear that
it's fatal and then free the re-encoded buffer ourselves (along with the
other buffers type and charset), perhaps like so (on top of the previous
patch in lieu of this one):
--- 8< ---
diff --git a/remote-curl.c b/remote-curl.c
index 5959461cd34..9d8359665ee 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -367,23 +367,25 @@ static void free_discovery(struct discovery *d)
}
}
-static int show_http_message(struct strbuf *type, struct strbuf *charset,
- struct strbuf *msg)
+static void show_http_message_fatal(struct strbuf *type, struct strbuf *charset,
+ struct strbuf *msg, const char *fmt, ...)
{
const char *p, *eol;
+ va_list ap;
+ report_fn die_message_routine = get_die_message_routine();
/*
* We only show text/plain parts, as other types are likely
* to be ugly to look at on the user's terminal.
*/
if (strcmp(type->buf, "text/plain"))
- return -1;
+ goto out;
if (charset->len)
strbuf_reencode(msg, charset->buf, get_log_output_encoding());
strbuf_trim(msg);
if (!msg->len)
- return -1;
+ goto out;
p = msg->buf;
do {
@@ -391,7 +393,15 @@ static int show_http_message(struct strbuf *type, struct strbuf *charset,
fprintf(stderr, "remote: %.*s\n", (int)(eol - p), p);
p = eol + 1;
} while(*eol);
- return 0;
+
+out:
+ strbuf_release(type);
+ strbuf_release(charset);
+ strbuf_release(msg);
+
+ va_start(ap, fmt);
+ die_message_routine(fmt, ap);
+ va_end(ap);
}
static int get_protocol_http_header(enum protocol_version version,
@@ -518,25 +528,27 @@ static struct discovery *discover_refs(const char *service, int for_push)
case HTTP_OK:
break;
case HTTP_MISSING_TARGET:
- show_http_message(&type, &charset, &buffer);
- die(_("repository '%s' not found"),
- transport_anonymize_url(url.buf));
+ show_http_message_fatal(&type, &charset, &buffer,
+ _("repository '%s' not found"),
+ transport_anonymize_url(url.buf));
--- >8 ---
(...and so on for the remaining cases).
Thanks,
Taylor4 participants
This patch series adds support for handling HTTP 429 (Too Many Requests)
responses in Git's HTTP client with automatic retry logic.
Git hosting services can implement rate limiting to protect their
infrastructure. When these limits are reached, servers respond with HTTP
429 status codes, potentially including a Retry-After header to indicate when
the client should retry. Currently, Git treats these responses as fatal
errors, forcing users to manually retry their operations.
This series implements automatic retry support with three new configuration
options:
http.maxRetries: Controls the maximum number of retry attempts
(default: 0, opt-in behavior)
http.retryAfter: Provides a fallback delay when the server doesn't
include a Retry-After header (default: -1, fail if no header)
http.maxRetryTime: Sets an upper limit on any single retry delay
(default: 300 seconds) to prevent indefinite blocking
The implementation includes:
Patch 1: Core HTTP 429 retry logic with support for RFC-compliant
Retry-After headers (both delay-seconds and HTTP-date formats),
comprehensive configuration options, and fail-fast behavior for
excessive delays. Includes extensive test coverage.
Patch 2: Fixes a pre-existing memory leak in show_http_message() that
became more visible with the new retry logic.
Patch 3: Adds trace2 instrumentation to enable monitoring and debugging
of retry operations in production environments.
The retry behavior is disabled by default (maxRetries = 0), requiring
explicit opt-in, ensuring backward compatibility while providing a robust
solution for environments that need rate limit handling.
There was a previous attempt to add retry support [1], which was not merged. It
had support for 50x status codes. Should they be supported here too?
[1] https://lore.kernel.org/git/20201012184806.166251-3-smcallis@google.com/
Vaidas Pilkauskas (3):
http: add support for HTTP 429 rate limit retries
remote-curl: fix memory leak in show_http_message()
http: add trace2 logging for retry operations
Documentation/config/http.adoc | 24 ++
http-push.c | 8 +
http-walker.c | 5 +
http.c | 171 ++++++++++++-
http.h | 2 +
remote-curl.c | 18 +-
t/meson.build | 1 +
t/t5584-http-429-retry.sh | 429 +++++++++++++++++++++++++++++++++
8 files changed, 650 insertions(+), 8 deletions(-)
create mode 100755 t/t5584-http-429-retry.sh
--
2.50.1
cc: Taylor Blau me@ttaylorr.com