http: add support for HTTP 429 rate limit retries#2008
Open
vaidas-shopify wants to merge 4 commits intogitgitgadget:masterfrom
Open
http: add support for HTTP 429 rate limit retries#2008vaidas-shopify wants to merge 4 commits intogitgitgadget:masterfrom
vaidas-shopify wants to merge 4 commits intogitgitgadget:masterfrom
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
dscho
reviewed
Nov 24, 2025
|
|
||
| /* Parse Retry-After header for rate limiting */ | ||
| if (skip_iprefix_mem(ptr, size, "retry-after:", &val, &val_len)) { | ||
| strbuf_add(&buf, val, val_len); |
Member
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.
Author
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
Member
There was a problem hiding this comment.
Good catch! And sorry for guiding you in the wrong direction :-(
Author
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
Member
Range-diff
@vaidas-shopify without addressing the |
vaidas-shopify
force-pushed
the
retry-after
branch
4 times, most recently
from
eb66a3a to
adbcc02
Compare
Author
|
/preview |
|
Preview email sent as pull.2008.git.1764159573.gitgitgadget@gmail.com |
Author
|
/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}
| @@ -315,6 +315,30 @@ http.keepAliveCount:: | |||
| 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,
TaylorThere was a problem hiding this comment.
On the Git mailing list, Vaidas Pilkauskas wrote (reply to this):
On Wed, Dec 10, 2025 at 1:15 AM Taylor Blau <me@ttaylorr.com> wrote:
> > +/*
> > + * 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.
This is a very good catch - I'll rewrite this to a non-blocking wait.
Thanks for the review, Taylor, I'll work to address this and other
comments in the next version of the patch.
On Wed, Dec 10, 2025 at 1:15 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> 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 |
remote-curl.c
Outdated
| @@ -371,26 +371,32 @@ 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,
Taylor
vaidas-shopify
force-pushed
the
retry-after
branch
from
adbcc02 to
163f2ea
Compare
|
There are issues in commit 163f2ea: |
vaidas-shopify
force-pushed
the
retry-after
branch
from
163f2ea to
c40e68a
Compare
|
There are issues in commit c40e68a: |
vaidas-shopify
force-pushed
the
retry-after
branch
2 times, most recently
from
f7dc975 to
efac711
Compare
strbuf_attach(sb, buf, len, alloc) requires alloc > len (the buffer must have at least len+1 bytes to hold the NUL). Several call sites passed alloc == len, relying on strbuf_grow(sb, 0) inside strbuf_attach to reallocate. Fix these in mailinfo, am, refs/files-backend, fast-import, and trailer by passing len+1 when the buffer is a NUL-terminated string (or from strbuf_detach). Signed-off-by: Vaidas Pilkauskas <vaidas.pilkauskas@shopify.com>
Several code paths in remote-curl.c follow the same pattern of calling show_http_message() to display server error messages followed by die() to terminate with an error. This duplication makes the code more verbose and harder to maintain. Introduce a new show_http_message_fatal() helper function that combines these two operations. This function: 1. Displays any HTTP error message from the server via show_http_message() 2. Calls die() with the provided error message 3. Returns NORETURN to help the compiler with control flow analysis Refactor existing call sites in remote-curl.c to use this new helper, reducing code duplication and improving readability. This pattern will also be used by upcoming HTTP 429 rate limiting support. Suggested-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Vaidas Pilkauskas <vaidas.pilkauskas@shopify.com>
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, http.retryAfter, and http.maxRetryTime) which are documented in git-config(1). 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>
vaidas-shopify
force-pushed
the
retry-after
branch
from
00e19e0 to
bfee1f1
Compare
Author
|
/submit |
|
Submitted as pull.2008.v5.git.1771856405.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This branch is now known as |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Vaidas Pilkauskas via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> Changes since v4:
>
> * fix only strbuf_attach() calls which don't need reallocation
> * remove patch, which enforces strbuf_attach() contract via BUG()
> ...
> Vaidas Pilkauskas (4):
> strbuf: pass correct alloc to strbuf_attach() in strbuf_reencode()
> strbuf_attach: fix call sites to pass correct alloc
> remote-curl: introduce show_http_message_fatal() helper
These three patches looked quite reasonable to me.
> http: add support for HTTP 429 rate limit retries
I'd feel comfortable to see somebody more familiar with the HTTP
transport code base to take a look at this step before we declare
victory.
Thanks. |
|
This patch series was integrated into seen via git@1373782. |
|
This patch series was integrated into seen via git@7f75b82. |
|
There was a status update in the "Cooking" section about the branch The HTTP transport learned to react to "429 Too Many Requests". Comments? source: <pull.2008.v5.git.1771856405.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@3f80e99. |
|
There was a status update in the "Cooking" section about the branch The HTTP transport learned to react to "429 Too Many Requests". Needs review. cf. <xmqq5x7nknrd.fsf@gitster.g> source: <pull.2008.v5.git.1771856405.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@29e706f. |
|
This patch series was integrated into seen via git@d7a75bf. |
|
There was a status update in the "Cooking" section about the branch The HTTP transport learned to react to "429 Too Many Requests". Needs review. cf. <xmqq5x7nknrd.fsf@gitster.g> source: <pull.2008.v5.git.1771856405.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@3f3f534. |
|
There was a status update in the "Cooking" section about the branch The HTTP transport learned to react to "429 Too Many Requests". Needs review. cf. <xmqq5x7nknrd.fsf@gitster.g> source: <pull.2008.v5.git.1771856405.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@bea5b1d. |
|
This patch series was integrated into seen via git@f38a5e4. |
|
There was a status update in the "Cooking" section about the branch The HTTP transport learned to react to "429 Too Many Requests". Needs review. cf. <xmqq5x7nknrd.fsf@gitster.g> source: <pull.2008.v5.git.1771856405.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@7827fa3. |
|
There was a status update in the "Cooking" section about the branch The HTTP transport learned to react to "429 Too Many Requests". Needs review. cf. <xmqq5x7nknrd.fsf@gitster.g> source: <pull.2008.v5.git.1771856405.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@254d084. |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Junio C Hamano <gitster@pobox.com> writes:
> "Vaidas Pilkauskas via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> Changes since v4:
>>
>> * fix only strbuf_attach() calls which don't need reallocation
>> * remove patch, which enforces strbuf_attach() contract via BUG()
>> ...
>> Vaidas Pilkauskas (4):
>> strbuf: pass correct alloc to strbuf_attach() in strbuf_reencode()
>> strbuf_attach: fix call sites to pass correct alloc
>> remote-curl: introduce show_http_message_fatal() helper
>
> These three patches looked quite reasonable to me.
>
>> http: add support for HTTP 429 rate limit retries
>
> I'd feel comfortable to see somebody more familiar with the HTTP
> transport code base to take a look at this step before we declare
> victory.
Any volunteers?
Thanks. |
|
There was a status update in the "Cooking" section about the branch The HTTP transport learned to react to "429 Too Many Requests". Needs review. cf. <xmqq5x7nknrd.fsf@gitster.g> source: <pull.2008.v5.git.1771856405.gitgitgadget@gmail.com> |
| @@ -367,31 +367,42 @@ static void free_discovery(struct discovery *d) | |||
| } | |||
There was a problem hiding this comment.
Jeff King wrote on the Git mailing list (how to reply to this email):
On Mon, Feb 23, 2026 at 02:20:04PM +0000, Vaidas Pilkauskas via GitGitGadget wrote:
> From: Vaidas Pilkauskas <vaidas.pilkauskas@shopify.com>
>
> Several code paths in remote-curl.c follow the same pattern of calling
> show_http_message() to display server error messages followed by die()
> to terminate with an error. This duplication makes the code more verbose
> and harder to maintain.
>
> Introduce a new show_http_message_fatal() helper function that combines
> these two operations. This function:
>
> 1. Displays any HTTP error message from the server via show_http_message()
> 2. Calls die() with the provided error message
> 3. Returns NORETURN to help the compiler with control flow analysis
>
> Refactor existing call sites in remote-curl.c to use this new helper,
> reducing code duplication and improving readability. This pattern will
> also be used by upcoming HTTP 429 rate limiting support.
I'm unconvinced that this actually makes things simpler. It doesn't save
any lines, the show_http_message() becomes less flexible, and we now
encode die-logic like the numeric code for exit() in the curl code.
> +static NORETURN 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();
If we got the die_routine here and not the die_message_routine, we
wouldn't have to exit() ourselves. But sadly there is not a
get_die_routine() helper yet, so we'd have to add one.
> /*
> * 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 {
OK, we jump to the end since now we have to make sure to hit the exit
bits.
> @@ -391,7 +393,16 @@ 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);
This cleanup seems pointless. These strbufs came from the caller, so
they are not our responsibility to release. But also, we're about to
call die(), so there is no need to clean them up.
> + va_start(ap, fmt);
> + die_message_routine(fmt, ap);
> + va_end(ap);
> + exit(128);
This part is the advertised change for the function, which looks correct.
> static int get_protocol_http_header(enum protocol_version version,
> @@ -518,21 +529,21 @@ 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));
So this is what the refactoring bought us: we trade 3 lines for 3 lines. ;)
If we could roll in policy decisions like anonymizing the URL, or
showing the curl_errorstr, I think it might be worth it. But we can't do
that easily because they are varargs for the %s format.
IMHO we should just drop this patch, and patch 4 should do the usual
show_http_message() and die().
-Peff
| @@ -315,6 +315,29 @@ http.keepAliveCount:: | |||
| unset, curl's default value is used. Can be overridden by the | |||
There was a problem hiding this comment.
Jeff King wrote on the Git mailing list (how to reply to this email):
On Mon, Feb 23, 2026 at 02:20:05PM +0000, Vaidas Pilkauskas via GitGitGadget wrote:
> @@ -257,6 +264,50 @@ static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p UN
> goto exit;
> }
>
> +#ifndef GIT_CURL_HAVE_CURLINFO_RETRY_AFTER
> + /* Parse Retry-After header for rate limiting (for curl < 7.66.0) */
> + if (skip_iprefix_mem(ptr, size, "retry-after:", &val, &val_len)) {
> + struct active_request_slot *slot = (struct active_request_slot *)p;
> +
> + strbuf_add(&buf, val, val_len);
> + 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, ×tamp, &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);
> + }
> + }
> + }
> +
> + goto exit;
> + }
> +#endif
> +
> /*
> * 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
I don't think this meshes well with the existing code for parsing
www-authenticate, especially with respect to header continuation:
1. If we see continuation like "Retry-After:\n <date>", we won't find
<date>. Instead, we'll just think it's blank (or worse, do a
partial parse if the line break happens at whitespace in the middle
of the date).
2. We don't reset http_auth.header_is_last_match, so
"WWW-Authenticate: foo\nRetry-After:\n <date>" will attribute
<date> to the WWW-Authenticate header.
3. We don't clear the value like we do for www-authenticate headers,
as we do in the lower code:
/*
* If this is a HTTP status line and not a header field, this signals
* a different HTTP response. libcurl writes all the output of all
* response headers of all responses, including redirects.
* We only care about the last HTTP request response's headers so clear
* the existing array.
*/
if (skip_iprefix_mem(ptr, size, "http/", &val, &val_len))
strvec_clear(values);
It's probably unlikely to get a retry-after _and_ a redirect in
practice, though.
I suspect these are all quite uncommon cases, but it worries me a little
that a malicious response could inject values into the wrong header.
And worse, that these cases will go largely untested, as most developers
have recent enough versions of libcurl.
Could we drop this hunk entirely, and just document that the Retry-After
feature only works if you have a recent version of libcurl? Curl 7.66.0
is almost 7 years old now (and you'd still get manual-timed retries if
you configure them).
> @@ -342,6 +393,17 @@ static void finish_active_slot(struct active_request_slot *slot)
>
> curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,
> &slot->results->http_connectcode);
> +
> +#ifdef GIT_CURL_HAVE_CURLINFO_RETRY_AFTER
> + if (slot->results->http_code == 429) {
> + curl_off_t retry_after;
> + CURLcode res = curl_easy_getinfo(slot->curl,
> + CURLINFO_RETRY_AFTER,
> + &retry_after);
> + if (res == CURLE_OK && retry_after > 0)
> + slot->results->retry_after = (long)retry_after;
> + }
> +#endif
This hunk makes sense. It is a little funny that we cast to long after
getting a curl_off_t, but I suspect we have to cast somewhere unless we
want to consistently pass around a curl_off_t (including via
http_request).
Though since it seems like http_request() is the only code path that
handles retries, do we need to grab it here in finish_active_slot()?
I.e., would it make more sense to do it in http_request() where we are
copying it out from the results field anyway?
Pushing it down to this level would make sense if we wanted to handle
retries on other requests outside of http_request (like dumb-http walker
request), but I don't think your patch does that.
> @@ -1886,6 +1971,9 @@ 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;
> +
This is an unusual spot for per-request setup. Usually this happens in
get_active_slot(), which is called before making a request.
I think you are putting it here because we will make several requests on
the same handle via http_request_recoverable(). But in that case, would
setting it there make more sense? In fact, we seem to _reset_ it there
already.
> static int http_request(const char *url,
> void *result, int target,
> - const struct http_get_options *options)
> + const struct http_get_options *options,
> + long *retry_after_out)
> {
Why add this as a new argument when we already have many other optional
out-parameters in http_get_options? I.e., I'd have expected this:
diff --git a/http.h b/http.h
index eb40456450..4e36e41432 100644
--- a/http.h
+++ b/http.h
@@ -155,12 +155,14 @@ struct http_get_options {
/*
* If not NULL, contains additional HTTP headers to be sent with the
* request. The strings in the list must not be freed until after the
* request has completed.
*/
struct string_list *extra_headers;
+
+ long *retry_after;
};
/* Return values for http_get_*() */
#define HTTP_OK 0
#define HTTP_MISSING_TARGET 1
#define HTTP_ERROR 2
We could possibly even just make it a regular "long", not a pointer. The
reason the other fields in http_get_options are pointers is that we only
want to fill them in if the caller is interested, because:
1. There is a cost to getting/computing the information.
2. They allocate memory, so the caller has to know to clean them up.
But here we just have a plain integer, so we could just always assign
it. It would mean dropping the "const" from the struct parameter, but I
think that's fine.
Looking further in the patch, I guess one reason is that the retry stuff
is handled internally by http_request(), so a caller never needs or
wants to see it. I wonder if that is always true, though. For example,
when we print the rate-limited message at the top-level of
remote-curl.c, might it be useful for us to mention the retry-after field?
If you want retries to work for all calls, we might have to use a dummy
http_get_options struct. But I think that would actually make the code
cleaner in general (no more "if (options && options->effective_url)"; it
would just "if (options->effective_url)".
> @@ -2148,7 +2237,8 @@ static int http_request(const char *url,
> fwrite_buffer);
> }
>
> - curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_wwwauth);
> + curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_headers);
> + curl_easy_setopt(slot->curl, CURLOPT_HEADERDATA, slot);
This hunk goes away if we drop the fwrite_headers() bit.
> +static long handle_rate_limit_retry(int *rate_limit_retries, long slot_retry_after)
> [...]
> + /* Use the slot-specific retry_after value or configured default */
> + if (slot_retry_after >= 0) {
> + /* Check if retry delay exceeds maximum allowed */
> + if (slot_retry_after > http_max_retry_time) {
> + error(_("response requested a delay greater than http.maxRetryTime (%ld > %ld seconds)"),
> + slot_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", slot_retry_after);
> + return -1;
> + }
> + return slot_retry_after;
OK. I had imagine that the max-time would clamp how long we were willing
to wait, but instead we just bail. I guess what you have is probably more
friendly to a server that says "please try again after 600 seconds" as
opposed to trying again faster than they'd prefer.
> + } else {
> + /* 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)"),
> + http_retry_after, http_max_retry_time);
> + trace2_data_string("http", the_repository,
> + "http/429-error", "config-exceeds-max-retry-time");
> + return -1;
> + }
And this one is a misconfiguration on the part of the user, so it should
certainly be an error. ;)
> +static int http_request_recoverable(const char *url,
> void *result, int target,
> struct http_get_options *options)
> {
> int i = 3;
> int ret;
> + int rate_limit_retries = http_max_retries;
> + long slot_retry_after = -1; /* Per-slot retry_after value */
Hmm, what is "i" here? Previously we used it to avoid REAUTH looping too
many times:
> - while (ret == HTTP_REAUTH && --i) {
> + while ((ret == HTTP_REAUTH || ret == HTTP_RATE_LIMITED) && --i) {
But now we are looping on both rate-limits and auth. If max_retries is
greater than 2, won't we bail even though there are retries left?
Also, what if we retry once due to RATE_LIMITED, but then need multiple
attempts to do auth? It feels like we should have two separate counters:
while ((ret == HTTP_REAUTH && --i) ||
(ret == HTTP_RATE_LIMITED && --rate_limit_retries)
Side note: I think I am reading the "--i" count right. It feels like it
would be simpler as "i--", but I guess it is counting the initial
http_request() we made. I suspect this might make more sense as a
do-while loop, but it is hairy enough that trying to refactor it further
might be risky.
> @@ -2301,10 +2454,23 @@ static int http_request_reauth(const char *url,
> default:
> BUG("Unknown http_request target");
> }
> + if (ret == HTTP_RATE_LIMITED) {
> + retry_delay = handle_rate_limit_retry(&rate_limit_retries, slot_retry_after);
> + if (retry_delay < 0)
> + return HTTP_ERROR;
> +
> + if (retry_delay > 0) {
> + warning(_("rate limited, waiting %ld seconds before retry"), retry_delay);
> + trace2_data_intmax("http", the_repository,
> + "http/retry-sleep-seconds", retry_delay);
> + sleep(retry_delay);
> + }
> + slot_retry_after = -1; /* Reset after use */
> + } else if (ret == HTTP_REAUTH) {
> + credential_fill(the_repository, &http_auth, 1);
> + }
OK. In the loop condition I suggested above, we decrement
rate_limit_retries there. But obviously it is also happening here via
handle_rate_limit_retry(), so it has to be one or the other.
I wondered about the case where we are out of retries, and whether we
might sleep before bailing. But that function checks that case and
returns -1, so we'd bail immediately. Good.
> +test_expect_success 'HTTP 429 retry delays are respected' '
> + # Enable retries
> + test_config http.maxRetries 3 &&
> +
> + # Time the operation - it should take at least 2 seconds due to retry delay
> + start=$(test-tool date getnanos) &&
> + git ls-remote "$HTTPD_URL/http_429/retry-delays-respected/2/repo.git" >output 2>err &&
> + duration=$(test-tool date getnanos $start) &&
> +
> + # Verify it took at least 2 seconds (allowing some tolerance)
> + duration_int=${duration%.*} &&
> + test "$duration_int" -ge 1 &&
> + test_grep "refs/heads/" output
> +'
Nice, the duration-checking here looks nice and simple. The 2-second
check here should be non-racy, since we'll wait at least that long. We
may get a false-success on a heavily-loaded system, but that's OK.
> +test_expect_success 'HTTP 429 fails immediately if Retry-After exceeds http.maxRetryTime' '
> + # Configure max retry time to 3 seconds (much less than requested 100)
> + test_config http.maxRetries 3 &&
> + test_config http.maxRetryTime 3 &&
> +
> + # Should fail immediately without waiting
> + start=$(test-tool date getnanos) &&
> + test_must_fail git ls-remote "$HTTPD_URL/http_429/retry-after-exceeds-max-time/100/repo.git" 2>err &&
> + duration=$(test-tool date getnanos $start) &&
> +
> + # Should fail quickly (less than 2 seconds, no 100 second wait)
> + duration_int=${duration%.*} &&
> + test "$duration_int" -lt 2 &&
> + test_grep "greater than http.maxRetryTime" err
> +'
This one is the opposite, though. On a heavily loaded system, we may get
a false failure if the test takes longer than 2s to run. Can we bump
this to something much less likely to trigger, like 99? Or even 100
would work, I'd think, since we know we'll wait at least 100 seconds if
we actually tried to use that retry-time.
> +test_expect_success 'HTTP 429 fails if configured http.retryAfter exceeds http.maxRetryTime' '
> + # Test misconfiguration: retryAfter > maxRetryTime
> + # Configure retryAfter larger than maxRetryTime
> + test_config http.maxRetries 3 &&
> + test_config http.retryAfter 100 &&
> + test_config http.maxRetryTime 5 &&
> +
> + # Should fail immediately with configuration error
> + start=$(test-tool date getnanos) &&
> + test_must_fail git ls-remote "$HTTPD_URL/http_429/config-retry-after-exceeds-max-time/none/repo.git" 2>err &&
> + duration=$(test-tool date getnanos $start) &&
> +
> + # Should fail quickly
> + duration_int=${duration%.*} &&
> + test "$duration_int" -lt 2 &&
> + test_grep "configured http.retryAfter.*exceeds.*http.maxRetryTime" err
> +'
Same here; the key thing is that we don't wait 100 seconds, but we might
accidentally take 2 seconds on a slow system.
I think there are a few more below, just looking for "-lt".
-Peff
|
Jeff King wrote on the Git mailing list (how to reply to this email): On Mon, Mar 09, 2026 at 04:34:25PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Vaidas Pilkauskas via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> >
> >> Changes since v4:
> >>
> >> * fix only strbuf_attach() calls which don't need reallocation
> >> * remove patch, which enforces strbuf_attach() contract via BUG()
> >> ...
> >> Vaidas Pilkauskas (4):
> >> strbuf: pass correct alloc to strbuf_attach() in strbuf_reencode()
> >> strbuf_attach: fix call sites to pass correct alloc
> >> remote-curl: introduce show_http_message_fatal() helper
> >
> > These three patches looked quite reasonable to me.
> >
> >> http: add support for HTTP 429 rate limit retries
> >
> > I'd feel comfortable to see somebody more familiar with the HTTP
> > transport code base to take a look at this step before we declare
> > victory.
>
> Any volunteers?
Sorry, I'm way underwater on things I could/should be reviewing, and
this one was quite non-trivial. ;)
I just left some comments. A lot of it was about how to more cleanly
integrate with the http code (which I admit is a mess, especially with
respect to which "layer" things should happen at). Some of that may be
debatable, though I hope we can make things a bit cleaner.
But I think there may be a logic error in how http_request_recoverable()
loops, which certainly needs to be fixed.
-Peff |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Jeff King <peff@peff.net> writes:
> On Mon, Mar 09, 2026 at 04:34:25PM -0700, Junio C Hamano wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > "Vaidas Pilkauskas via GitGitGadget" <gitgitgadget@gmail.com>
>> > writes:
>> >
>> >> Changes since v4:
>> >>
>> >> * fix only strbuf_attach() calls which don't need reallocation
>> >> * remove patch, which enforces strbuf_attach() contract via BUG()
>> >> ...
>> >> Vaidas Pilkauskas (4):
>> >> strbuf: pass correct alloc to strbuf_attach() in strbuf_reencode()
>> >> strbuf_attach: fix call sites to pass correct alloc
>> >> remote-curl: introduce show_http_message_fatal() helper
>> >
>> > These three patches looked quite reasonable to me.
>> >
>> >> http: add support for HTTP 429 rate limit retries
>> >
>> > I'd feel comfortable to see somebody more familiar with the HTTP
>> > transport code base to take a look at this step before we declare
>> > victory.
>>
>> Any volunteers?
>
> Sorry, I'm way underwater on things I could/should be reviewing, and
> this one was quite non-trivial. ;)
>
> I just left some comments. A lot of it was about how to more cleanly
> integrate with the http code (which I admit is a mess, especially with
> respect to which "layer" things should happen at). Some of that may be
> debatable, though I hope we can make things a bit cleaner.
>
> But I think there may be a logic error in how http_request_recoverable()
> loops, which certainly needs to be fixed.
Thanks.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes since v4:
Changes since v3:
Clean up of all strbuf_attach() call sites
Add strbuf_attach() contract enforcement via BUG()
Changes since v2:
New preparatory patch: Introduced show_http_message_fatal() helper
function to reduce code duplication in remote-curl.c (suggested by
Taylor Blau)
Removed specific HTTP_RATE_LIMITED error handling from http-push.c
and http-walker.c for the obsolete "dumb" protocol, allowing generic
error handling to take over (suggested by Jeff King)
Added support for CURLINFO_RETRY_AFTER on curl >= 7.66.0, falling
back to manual header parsing on older versions
Simplified retry/delay architecture: replaced complex non-blocking
"delayed slot" mechanism with simple blocking sleep() call in the
retry loop, removing ~66 lines of timing logic (suggested by Jeff King)
Fixed Retry-After: 0 handling to allow immediate retry as specified
by RFC 9110
Changed http.retryAfter default from -1 to 0, so Git will retry
immediately when encountering HTTP 429 without a Retry-After header,
rather than failing with a configuration error
Improved error messages: shortened to be more concise
Fixed coding style issues: removed unnecessary curly braces, changed
x == 0 to !x (per CodingGuidelines)
Improved test portability: replaced non-portable date(1) commands
with test-tool date, added nanosecond-precision timing with getnanos,
replaced cut(1) with POSIX shell parameter expansion
Split out strbuf.c bugfix into separate preparatory patch (the
strbuf_reencode alloc size fix is unrelated to HTTP 429 support)
Squashed separate trace2 logging patch into main HTTP 429 retry
support commit
Kept header_is_last_match assignment for Retry-After to prevent
incorrect handling of HTTP header continuation lines
The implementation includes:
A bug fix in strbuf_reencode() that corrects the allocation size
passed to strbuf_attach(), passing len+1 instead of len so that
the existing buffer is reused rather than immediately reallocated.
A cleanup of strbuf_attach() call sites that were passing
alloc == len, leaving no room for the NUL terminator. Sites with
a known-NUL-terminated buffer now pass len+1; sites where the
source buffer has no trailing NUL (ll_merge output) are converted
to use strbuf_add() instead.
A new show_http_message_fatal() helper in remote-curl.c that
combines the repeated pattern of show_http_message() followed by
die() into a single NORETURN function, reducing boilerplate at
existing call sites and providing a clean hook for the retry logic.
The main feature: HTTP 429 retry logic with support for the
Retry-After header (both delay-seconds and HTTP-date formats),
configurable via http.maxRetries, http.retryAfter, and
http.maxRetryTime options. If any computed delay exceeds
maxRetryTime the request fails immediately with a clear diagnostic
rather than capping and retrying silently.
cc: Taylor Blau me@ttaylorr.com
cc: Jeff King peff@peff.net
cc: Junio C Hamano gitster@pobox.com