[PATCH 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 records "already returned 429 once" with a "test -f"
followed by a "touch" of a shared state file. That check-then-act is not
atomic: Apache can run this CGI for several requests at once, and two of
them can both pass the "test -f" before either "touch"es, so both treat
themselves as the first request. The retry flow that drives this
endpoint is mostly sequential, so this has not been seen to fail, but
the race is latent.

Decide whether this is the first request with a single atomic mkdir,
which fails if the directory already exists, so exactly one of any
concurrent requests is rate-limited and the rest are forwarded.

There is no accompanying regression test. The check and the set are
adjacent commands with no external step in between to synchronize on, so
the overlap cannot be forced deterministically, only reproduced
probabilistically; the fix is preventive.

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

diff --git a/t/lib-httpd/http-429.sh b/t/lib-httpd/http-429.sh
index c97b16145b..d9bbedf1ad 100644
--- a/t/lib-httpd/http-429.sh
+++ b/t/lib-httpd/http-429.sh
@@ -26,14 +26,17 @@ 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
+# Use current directory (HTTPD_ROOT_PATH) for state.
+# Create a safe name from test_context, retry_after and repo_name so that all
+# requests for the same 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"
+# Apache can run this CGI for concurrent requests, so the script decides
+# whether this is the first call with a single atomic "mkdir": it succeeds for
+# exactly one of any racing requests and fails for the rest. "permanent"
+# always rate-limits and records no state.
+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 +55,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 +67,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