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 [email protected], 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 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 |
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.
Trace2 logging has been added to track retry attempts, delays, and
error conditions. This enables monitoring and debugging of rate limit
scenarios in production environments.
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 <[email protected]>
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 <[email protected]>
vaidas-shopify
force-pushed
the
retry-after
branch
from
5b859d1 to
d4a5896
Compare
3 participants
No description provided.