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

From: Michael Montalbo via GitGitGadget <gitgitgadget@gmail.com>
Date: Aug 13, 2026, 01:05 UTC
Message-ID: <8ed22c02a192e10ab46c7df61e92a3669faaf25a.1786583137.git.gitgitgadget@gmail.com>
In-Reply-To: <pull.2171.v3.git.1786583137.gitgitgadget@gmail.com>

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.

Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
 t/lib-httpd/http-429.sh | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

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
@@ -3,7 +3,7 @@
 # Script to return HTTP 429 Too Many Requests responses for testing retry logic.
 # Usage: /http_429/<test-context>/<retry-after-value>/<repo-path>
 #
-# The test-context is a unique identifier for each test to isolate state files.
+# The test-context is a unique identifier for each test to isolate state directories.
 # The retry-after-value can be:
 #   - A number (e.g., "1", "2", "100") - sets Retry-After header to that many seconds
 #   - "none" - no Retry-After header
@@ -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"

-- 
gitgitgadget