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

Patrick Steinhardt [off-list ref] writes:

On Thu, Aug 13, 2026 at 01:05:35AM +0000, Michael Montalbo via GitGitGadget wrote:

quoted

From: Michael Montalbo <redacted>

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.