Re: [PATCH v3 2/3] t/lib-httpd: make http-429 first-request check atomic
From: Patrick Steinhardt <hidden>
Date: 2026-08-31 07:18:20
On Thu, Aug 13, 2026 at 01:05:35AM +0000, Michael Montalbo via GitGitGadget wrote:
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.
quoted hunk ↗ jump to hunk
diff --git a/t/lib-httpd/http-429.sh b/t/lib-httpd/http-429.sh index c97b16145b..904cdacbd0 100644 --- a/t/lib-httpd/http-429.sh +++ b/t/lib-httpd/http-429.sh@@ -26,14 +26,24 @@ repo_path="${remaining#*/}" # Get rest (repo path) # The repo name is the first component before any "/" repo_name="${repo_path%%/*}" -# Use current directory (HTTPD_ROOT_PATH) for state file -# Create a safe filename from test_context, retry_after and repo_name -# This ensures all requests for the same test context share the same state file +# Store state in the current directory (HTTPD_ROOT_PATH). Build a safe name +# from test_context, retry_after, and repo_name, so that all requests for one +# test context share the same state. safe_name=$(echo "${test_context}-${retry_after}-${repo_name}" | tr '/' '_' | tr -cd 'a-zA-Z0-9_-') -state_file="http-429-state-${safe_name}" +state="http-429-state-${safe_name}" -# Check if this is the first call (no state file exists) -if test -f "$state_file" +# This endpoint returns 429 to the first request. It forwards every later +# request to git-http-backend, so the retry succeeds. Apache can run this CGI +# for several requests at the same time. A single atomic "mkdir" selects the +# first request, because only one "mkdir" succeeds. That request returns 429 +# and leaves the directory as the "already rate-limited" marker. Every later +# "mkdir" fails, so the endpoint forwards those requests. +# +# "permanent" is the exception. It must return 429 to every request, so it +# skips the "mkdir" and records no state. A leftover directory would let a +# later "permanent" request find the marker. The endpoint would forward that +# request, which "permanent" must not allow. +if test "$retry_after" != permanent && ! mkdir "$state" 2>/dev/null then # Already returned 429 once, forward to git-http-backend # Set PATH_INFO to just the repo path (without retry-after value)@@ -52,9 +62,6 @@ then exec "$GIT_EXEC_PATH/git-http-backend" fi -# Mark that we've returned 429 -touch "$state_file" - # Output HTTP 429 response printf "Status: 429 Too Many Requests\r\n"@@ -67,8 +74,7 @@ case "$retry_after" in printf "Retry-After: invalid-format-123abc\r\n" ;; permanent) - # Always return 429, don't set state file for success - rm -f "$state_file" + # Always return 429 printf "Retry-After: 1\r\n" printf "Content-Type: text/plain\r\n" printf "\r\n"
The changes themselves look sensible. Patrick