From 4021a93366f1e88031dbd11e967890fef5e453b4 Mon Sep 17 00:00:00 2001 From: Michael Montalbo Date: Tue, 1 Sep 2026 00:27:54 +0000 Subject: [PATCH 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests apply-one-time-script.sh is a test helper that executes a "one-time-script" responsible for modifying the response normally returned by git-http-backend. apply-one-time-script.sh should run "one-time-script" once and return a modified response once. However, sometimes a race between multiple concurrent requests causes apply-one-time-script.sh to misbehave and return multiple modified responses or an empty response that results in: fatal: ... The requested URL returned error: 500 fatal: could not fetch from promisor remote This can be seen in the flaky failure of t5616.47 on the macOS CI runners. Fix the logic that checks if "one-time-script" has returned its modified response by chaining "rm one-time-script" with its execution. This ensures a racing script does not also have the opportunity to execute "one-time-script". Add t/t5567-one-time-script.sh to verify the race is fixed. Implement a stub "git-http-backend" that intentionally invokes a concurrent request, and check that only one modified response is returned without error. Signed-off-by: Michael Montalbo Signed-off-by: Junio C Hamano --- t/lib-httpd/apply-one-time-script.sh | 40 +++++++----- t/meson.build | 1 + t/t5567-one-time-script.sh | 96 ++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 15 deletions(-) create mode 100755 t/t5567-one-time-script.sh diff --git a/t/lib-httpd/apply-one-time-script.sh b/t/lib-httpd/apply-one-time-script.sh index b1682944e2..eac21a3a8e 100644 --- a/t/lib-httpd/apply-one-time-script.sh +++ b/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 +test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend" + +LC_ALL=C +export LC_ALL + +out=out.$$ +modified=out-modified.$$ +"$GIT_EXEC_PATH/git-http-backend" >"$out" + +# 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 - LC_ALL=C - export LC_ALL - - "$GIT_EXEC_PATH/git-http-backend" >out - ./one-time-script out >out_modified - - if cmp -s out out_modified - then - cat out - else - cat out_modified - rm one-time-script - fi + cat "$modified" else - "$GIT_EXEC_PATH/git-http-backend" + cat "$out" fi +rm -f "$out" "$modified" diff --git a/t/meson.build b/t/meson.build index 3219264fe7..a118a4d719 100644 --- a/t/meson.build +++ b/t/meson.build @@ -707,6 +707,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', diff --git a/t/t5567-one-time-script.sh b/t/t5567-one-time-script.sh new file mode 100755 index 0000000000..a8429ef3c3 --- /dev/null +++ b/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 From e43647bf0efc1c155e74dd37145a91753fe4a381 Mon Sep 17 00:00:00 2001 From: Michael Montalbo Date: Tue, 1 Sep 2026 00:27:55 +0000 Subject: [PATCH 2/3] t/lib-httpd: make http-429 first-request check atomic http-429.sh is a helper for testing retry logic. It uses "test -f" to check for the existence of a state file and later uses "touch" or "rm -f" on that file to determine if it should return a 429. This method of managing state can fail if the helper script is invoked concurrently. However, this failure does not currently manifest itself since the helper is invoked sequentially. As a preventive measure, fix the state management logic so it relies on an atomic mkdir operation to mark that a 429 was returned. When $retry_after is "permanent", always return 429 now that we do not rely on a state file that is "touch"ed and "rm"ed to indicate when to respond with a 429. Signed-off-by: Michael Montalbo Signed-off-by: Junio C Hamano --- t/lib-httpd/http-429.sh | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/t/lib-httpd/http-429.sh b/t/lib-httpd/http-429.sh index c97b16145b..1a5d7987db 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/// # -# 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" From e324fdf46d4b37fb97a8d92a0b22364a0219b67a Mon Sep 17 00:00:00 2001 From: Michael Montalbo Date: Tue, 1 Sep 2026 00:27:56 +0000 Subject: [PATCH 3/3] t/lib-httpd: document writing concurrency-safe CGI helpers Update t/lib-httpd.sh to document the fixes applied to apply-one-time-script.sh and http-429.sh for future developers working on helper scripts. Add concrete examples of patterns and anti-patterns that should be considered when handling state management. Signed-off-by: Michael Montalbo Signed-off-by: Junio C Hamano --- t/lib-httpd.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index fc646447d5..9d422daccb 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -159,6 +159,17 @@ 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 "claims" 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