t/lib-httpd: make CGI test helpers concurrency-safe

t/lib-httpd.sh

@@ -159,6 +159,18 @@ prepare_httpd() {
 	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
 	cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
 	cp "$TEST_PATH"/proxy-passwd "$HTTPD_ROOT_PATH"
+	# Apache can run the following scripts concurrently per request. Make
+	# sure any state management logic is resilient to race conditions.
+	#
+	# For example:
+	#   - use "mkdir dir" to ensure only one request "succeeds" under some
+	#     condition (see http-429.sh).
+	#   - chain (&&) atomic operations like "rm marker" (no -f) with the
+	#     logic that is guarded by the marker instead of relying on a
+	#     separate "test -f" and "rm marker" check
+	#     (see apply-one-time-script.sh).
+	#   - use scratch file names that include the process ID ($$), so
+	#     concurrent requests do not overwrite each other's state.
 	install_script incomplete-length-upload-pack-v2-http.sh
 	install_script incomplete-body-upload-pack-v2-http.sh
 	install_script error-no-report.sh

t/lib-httpd/apply-one-time-script.sh

@@ -6,21 +6,31 @@
 #
 # This can be used to simulate the effects of the repository changing in
 # between HTTP request-response pairs.
-if test -f one-time-script
-then
-	LC_ALL=C
-	export LC_ALL
+test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend"
+
+LC_ALL=C
+export LC_ALL
 
-	"$GIT_EXEC_PATH/git-http-backend" >out
-	./one-time-script out >out_modified
+out=out.$$
+modified=out-modified.$$
+"$GIT_EXEC_PATH/git-http-backend" >"$out"
 
-	if cmp -s out out_modified
-	then
-		cat out
-	else
-		cat out_modified
-		rm one-time-script
-	fi
+# Since Apache can execute this script for multiple requests
+# concurrently, we chain "rm one-time-script" with the logic
+# for generating a modified response. If the "rm" ran separately,
+# a concurrent request could pass the "test -f" above and
+# erroneously result in multiple modified responses or an empty
+# body depending on the race state.
+#
+# We discard stderr for ./one-time-script since it is possible
+# ./one-time-script has been removed already, which is expected
+# sometimes. In this case, the unmodified response will be returned.
+if ./one-time-script "$out" 2>/dev/null >"$modified" &&
+   ! cmp -s "$out" "$modified" &&
+   rm one-time-script 2>/dev/null
+then
+	cat "$modified"
 else
-	"$GIT_EXEC_PATH/git-http-backend"
+	cat "$out"
 fi
+rm -f "$out" "$modified"

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,16 @@ 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) to hold state directory
+# Create a safe directory name from test_context, retry_after and repo_name
+# This ensures all requests for the same test context share the same state directory
 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"
+# Check if this is the first call (no state directory exists), or if
+# the retry-after-value is "permanent", which indicates a 429 must be
+# returned for every request (even if the state directory exists).
+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 +54,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 +66,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"

t/meson.build

@@ -716,6 +716,7 @@ integration_tests = [
   't5564-http-proxy.sh',
   't5565-push-multiple.sh',
   't5566-push-group.sh',
+  't5567-one-time-script.sh',
   't5570-git-daemon.sh',
   't5571-pre-push-hook.sh',
   't5572-pull-submodule.sh',

t/t5567-one-time-script.sh

@@ -0,0 +1,96 @@
+#!/bin/sh
+
+test_description='apply-one-time-script CGI helper is safe under concurrent requests'
+
+. ./test-lib.sh
+
+HELPER="$TEST_DIRECTORY/lib-httpd/apply-one-time-script.sh"
+
+test_expect_success PIPE 'helper only serves one rewritten response for concurrent requests' '
+	mkdir workdir fakebin &&
+	ENTERED="$PWD/entered" &&
+	GATE="$PWD/gate" &&
+	export ENTERED GATE &&
+	mkfifo "$ENTERED" "$GATE" &&
+
+	# A stub git-http-backend that returns a response based on
+	# $ROLE. For $ROLE = modify, return the response string
+	# "packfile", which ends up being modified by the example
+	# one-time-script below.
+	#
+	# Otherwise, run the branch returning a response that
+	# should be passed through, and block until released
+	# by "read -r $GATE".
+	write_script fakebin/git-http-backend <<-\EOF &&
+	printf "Status: 200 OK\r\n"
+	printf "Content-Type: application/x-git-result\r\n"
+	printf "\r\n"
+	if test "$ROLE" = modify
+	then
+		printf "packfile\n"
+	else
+		echo entered >"$ENTERED"
+		read -r released <"$GATE"
+		printf "refs\n"
+	fi
+	EOF
+
+	# An example one-time-script for apply-one-time-script
+	# to execute. Checks for "packfile" in the response
+	# that will be returned, and replaces it with a
+	# modified response. Passes through responses without
+	# "packfile" in them.
+	write_script workdir/one-time-script <<-\EOF &&
+	if grep packfile "$1" >/dev/null
+	then
+		sed "/packfile/q" "$1" &&
+		printf "REPLACED\n"
+	else
+		cat "$1"
+	fi
+	EOF
+
+	GIT_EXEC_PATH="$PWD/fakebin" &&
+	export GIT_EXEC_PATH &&
+
+	# Ensure $GATE has a reader so the test does not block indefinitely if
+	# the helper is buggy and "echo released >&9" below does not unblock
+	# the unmodified response gate.
+	exec 9<>"$GATE" &&
+
+	# Launch the passthrough request in the background. Record its pid
+	# so it can be killed when the test finishes if, for some reason, the
+	# request stays blocked and would stall a test runner.
+	{ (
+		cd workdir &&
+		ROLE=passthrough sh "$HELPER" >../passthrough.out 2>../passthrough.err
+	) & } &&
+	passthrough_pid=$! &&
+	test_when_finished "kill $passthrough_pid 2>/dev/null || :" &&
+
+	# Wait until the passthrough request is "in-flight" and paused
+	# mid-response.
+	read -r entered <"$ENTERED" &&
+
+	# Launch the request for a modified response while the passthrough
+	# request is concurrently "in-flight" and paused.
+	(
+		cd workdir &&
+		ROLE=modify sh "$HELPER" >../modify.out 2>../modify.err
+	) &&
+
+	# Unblock the passthrough request, allowing git-http-backend to
+	# complete its response.
+	echo released >&9 &&
+	{ wait "$passthrough_pid" || :; } &&
+
+	test_must_be_empty passthrough.err &&
+	test_must_be_empty modify.err &&
+	test_grep "Status: 200 OK" passthrough.out &&
+	test_grep "Status: 200 OK" modify.out &&
+	test_grep REPLACED modify.out &&
+	test_grep ! REPLACED passthrough.out &&
+	test_grep refs passthrough.out
+'
+
+test_done