From: Vaidas Pilkauskas <vaidas.pilkauskas@shopify.com>
Date: Fri, 13 Feb 2026 13:41:55 GMT
Subject: Re: [PATCH v2 1/2] http: add support for HTTP 429 rate limit retries
Message-ID: <CAGjQmDMhWFx32M+2DrZ3cF-mt+T==LNEzXRO4z=R73RjZnTQPg@mail.gmail.com>
In-Reply-To: <20260211091333.GA1868492@coredump.intra.peff.net>

On Wed, Feb 11, 2026 at 11:13 AM Jeff King <peff@peff.net> wrote:
> Yeah, I noticed that, too. And all of the parsing actually makes me
> nervous. Surely curl can do some of this for us?
>
> ...studies some manpages...
>
> Ah, indeed. How about:
>
>   curl_off_t wait = 0;
>   curl_easy_getinfo(slot->curl, CURLINFO_RETRY_AFTER, &wait);
>
> You can see how we already dig out similar info in finish_active_slot().
> And more extended (but optional) info in http_request(). It looks like
> CURLINFO_RETRY_AFTER was added in 7.66.0, so this would have to be a
> conditional feature at build-time. But that seems like a reasonable
> trade-off.

I'll add parsing with libcurl under conditional feature.

> Most of the details of this active slot stuff have long been paged out
> of my memory. It's all _so_ messy because of the desire for the
> dumb-http code to handle multiple requests. But for smart-http (and I
> would be perfectly content for this feature to only apply there), we
> could probably just focus on run_one_slot(), I'd think.
>
> I.e., what I'd expect the simplest form of the patch to look like is
> roughly:
>
>   - teach handle_curl_result() to recognize 429 and pull out the
>     retry-after value, returning HTTP_RETRY
>
>   - in run_one_slot(), recognize HTTP_RETRY and if appropriate, sleep
>     and retry
>

This greatly simplifies implementation. I think following similar pattern like
auth handling does makes a lot of sense. So, instead of sleeping in
run_one_slot(), I think it makes sense to sleep in http_request_recoverable()
where HTTP_REAUTH is handled.

> > I may solicit Peff's input here on the remainder of the test changes,
> > since he is much more familiar with the lib-httpd parts of the suite
> > than I am.
>
> The lib-httpd parts looked about as I'd expect (and I found the use of
> custom URL components to encode the retry parameters quite clever).
>
> There were lots of uses of "date" that I suspect may give us portability
> problems. "+%s" is not even in POSIX, but maybe it is universal enough.
> But stuff like '-d "+2 seconds"' seems likely to be a GNU-ism.
>
> Using "test-tool date" might get around some of that. We even understand
> relative dates like "2 seconds ago", but I think only in the past. :-/
> So you'd probably have to do:
>
>   now=$(test-tool date timestamp now | cut -d' ' -f3)
>   then=$((now + 2))
>   test-tool date show:rfc2822 $then
>
> or something.

I was not aware about test-tool, thanks!

> -Peff

Thanks, Peff, for the review!