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.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
bf53e31af8