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

["Michael Montalbo via GitGitGadget" <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