Re: [PATCH v3 2/3] t/lib-httpd: make http-429 first-request check atomic

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> On Thu, Aug 13, 2026 at 01:05:35AM +0000, Michael Montalbo via GitGitGadget wrote:
>> From: Michael Montalbo <mmontalbo@gmail.com>
>> 
>> http-429.sh returns 429 to the first request for an endpoint and
>> forwards later ones to git-http-backend so the retry succeeds. It
>> remembers that it has already answered 429 by checking for a shared
>> state file with "test -f" and creating it with "touch".
>> 
>> That "check-and-set" is not atomic. Apache runs the CGI for several
>> requests at once, so two of them can pass the "test -f" before either
>> "touch"es the file, and both then answer as the first request. The
>> retry flow is mostly sequential, so this has not been observed to fail,
>> but the race is latent. Replace the check and the "touch" with a single
>> atomic "mkdir", which fails if the directory already exists, so exactly
>> one of the concurrent requests is rate-limited and the rest are
>> forwarded.
>> 
>> The "permanent" mode needs one extra step, for correctness rather than
>> tidiness. The marker means "429 already served, now forward", so it must
>> never be visible to a request that must itself return 429. Since
>> "permanent" returns 429 to every request, it must leave no marker. The
>> original did not manage this. It ran the "touch" unconditionally and
>> removed the file with "rm -f" in the "permanent" case, and that
>> "create-then-remove" has the same racy window: a concurrent "permanent"
>> request can see the marker before the "rm -f" and be wrongly forwarded.
>> Skipping the "mkdir" entirely for "permanent" (the "!= permanent" guard)
>> leaves no marker at all, so every "permanent" request rate-limits.
>> 
>> There is no regression test. The check and the set are adjacent commands
>> with nothing in between to synchronize on, so the overlap cannot be
>> forced deterministically, only reproduced by chance; the fix is
>> preventive.
>
> A lot of AI-fluff in this message that could have otherwise been much
> briefer, but okay.

I too find it disturbing it that the messages from this author tends
to contain material that triggers "it may not be wrong, but is it
relevant?" reactions.  More does not mean better.

The above made me curious enough to ask a near-by Gemini to distill
it down to quarter of the original length without losing essense of
the original.

    http-429.sh marks that a 429 response was served by creating a
    state file with "test -f" and "touch".  This check-and-set
    sequence is not atomic and can race under concurrent Apache
    requests, causing multiple requests to claim first-arrival
    status.

    Replace the check and "touch" with an atomic "mkdir", which
    fails if the directory already exists.  In "permanent" mode,
    skip the "mkdir" entirely so no state marker is ever created.

    Omit a regression test, as this concurrency window cannot be
    forced deterministically without artificial synchronization
    points.

This seems readable enough to me, but may still need some manual
clean-up, but this experiment told me that "A lot of AI-fluff" is
not something users cannot avoid without some extra work.

Thanks.