From 9f48aa6d6ddea681b700f689f0509c4b30a7007d Mon Sep 17 00:00:00 2001 From: Michael Montalbo Date: Mon, 6 Jul 2026 20:06:57 -0700 Subject: [PATCH 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests apply-one-time-script.sh checks for the "one-time-script" marker, runs it, captures the git-http-backend response in the fixed-name files "out" and "out_modified", and removes the marker only after it has finished serving the modified response. Because the client receives the response body before that removal, it can start its next request while the marker still exists. Apache can then run this CGI for two requests at once: a partial fetch that receives a REF_DELTA against a missing promisor object lazily fetches that base while the first response is still in flight. The second request passes the marker check, the first request then removes the marker, and the second fails to exec the now-missing marker, emits no output, and the server answers HTTP 500: fatal: ... The requested URL returned error: 500 fatal: could not fetch from promisor remote This has been seen as a flaky failure of t5616.47 on the macOS CI runners. Claim the marker atomically with a rename, and only once the one-time script has succeeded and actually changed the response; give the scratch files per-request names. A request that loses the rename, or whose script fails or leaves the response unchanged, serves the unmodified body and keeps the marker for a later request. No path emits an empty body, so the HTTP 500 no longer occurs. Add t5567 to lock this down. The overlap depends on timing, so a live httpd test such as t5616.47 (the real code path) passes almost every time even against the buggy helper; t5567 instead drives the helper directly with a fake git-http-backend and forces the overlap with FIFOs. Against the pre-fix helper it fails with the same shell error seen in the field: ./one-time-script: No such file or directory Signed-off-by: Michael Montalbo --- t/lib-httpd/apply-one-time-script.sh | 38 +++++++---- t/meson.build | 1 + t/t5567-one-time-script.sh | 96 ++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 14 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 b1682944e280e2..a298ae89ae90cf 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 -then - LC_ALL=C - export LC_ALL +# +# Apache can run this CGI for concurrent requests (for example a partial fetch +# that lazily fetches a missing object while the first response is still in +# flight), so the helper claims the marker atomically with a rename, and only +# once it has decided to modify the response. A request that loses the race +# finds the marker already gone and serves its response unchanged; no request +# is left emitting an empty body, which the server would report as HTTP 500. +# Scratch files are per-request ($$) so concurrent requests do not clobber each +# other. + +test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend" - "$GIT_EXEC_PATH/git-http-backend" >out - ./one-time-script out >out_modified +LC_ALL=C +export LC_ALL - if cmp -s out out_modified - then - cat out - else - cat out_modified - rm one-time-script - fi +out=out.$$ +modified=out-modified.$$ +"$GIT_EXEC_PATH/git-http-backend" >"$out" + +if ./one-time-script "$out" 2>/dev/null >"$modified" && + ! cmp -s "$out" "$modified" && + mv one-time-script one-time-script.$$ 2>/dev/null +then + cat "$modified" else - "$GIT_EXEC_PATH/git-http-backend" + cat "$out" fi +rm -f "$out" "$modified" one-time-script.$$ diff --git a/t/meson.build b/t/meson.build index 3219264fe7d497..a118a4d7196b17 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 00000000000000..cd8e6560056160 --- /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 'concurrent requests: one rewritten, one passed through, neither empty' ' + mkdir workdir fakebin && + ENTERED="$PWD/entered" && + GATE="$PWD/gate" && + export ENTERED GATE && + mkfifo "$ENTERED" "$GATE" && + + # Stand in for git-http-backend. The modify role returns a response + # containing "packfile", which the one-time script rewrites. The + # passthrough role returns a response that is left untouched, but first + # announces that it has entered the helper and then blocks, so that it + # is still in flight when the modify role claims and removes the marker. + 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 + + # The transform that replace_packfile would install as one-time-script: + # rewrite responses that contain "packfile", leave the rest alone. + 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 && + + # Hold GATE open read-write on fd 9 for the duration, so releasing the + # passthrough request below cannot block even if that request has + # already exited (it keeps a reader on the FIFO). + exec 9<>"$GATE" && + + # Launch the passthrough request in the background. It enters the + # helper, signals us through ENTERED, then blocks on GATE inside the + # fake backend. The braces keep the && chain intact while backgrounding + # only the subshell, so "wait" can reap it by pid; kill it on any exit + # so a stray blocked child cannot hold the test output open and stall a + # reader such as prove. + { ( + 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 past the marker check. + read -r entered <"$ENTERED" && + + # Run the modifying request to completion while the passthrough request + # is still blocked. + ( + cd workdir && + ROLE=modify sh "$HELPER" >../modify.out 2>../modify.err + ) && + + # Release the passthrough request and let it finish. Ignore the helper + # exit status here so a broken helper is diagnosed by the assertions + # below rather than aborting the test. + echo released >&9 && + { wait "$passthrough_pid" || :; } && + + # Neither request may error out or produce an empty (HTTP 500) body, + # and each must have played its role: the modify request rewrote its + # response and the passthrough request came through untouched. + 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 efd34c17157b3183cdc851c8b17e7967b6c85506 Mon Sep 17 00:00:00 2001 From: Michael Montalbo Date: Mon, 6 Jul 2026 20:32:10 -0700 Subject: [PATCH 2/3] t/lib-httpd: make http-429 first-request check atomic 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 --- 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 c97b16145b7f92..d9bbedf1ad3f2d 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" From 771d264d2999a780e0c93e64bb4451a05214ab75 Mon Sep 17 00:00:00 2001 From: Michael Montalbo Date: Mon, 6 Jul 2026 20:32:10 -0700 Subject: [PATCH 3/3] t/README: document writing concurrency-safe helpers The apply-one-time-script.sh and http-429.sh fixes addressed the same underlying problem: a test helper assuming it has exclusive access to a file when the web server can run it for several requests at once. The atomic idioms that avoid this are not specific to CGI or to HTTP, so document them generally, alongside the other guidance for writing tests, and leave a pointer from the lib-httpd helper list rather than a local comment. The note covers the anti-pattern (a "test -f" then a separate act) and the two safe operations (mkdir to elect a winner, rename to consume a one-shot marker), citing Git's own lockfile machinery and make_symlink() as precedent. Signed-off-by: Michael Montalbo --- t/README | 32 ++++++++++++++++++++++++++++++++ t/lib-httpd.sh | 3 +++ 2 files changed, 35 insertions(+) diff --git a/t/README b/t/README index 085921be4b6c2a..a9d425f392115c 100644 --- a/t/README +++ b/t/README @@ -854,6 +854,38 @@ from the test harness library. At the end of the script, call 'test_done'. +Writing concurrency-safe helpers +-------------------------------- + +Some test code runs concurrently: a test may background work with '&', +and the helper scripts installed for the web server (in t/lib-httpd) are +run once per request, so the same script can execute for several +requests at once. Such code cannot assume it has exclusive access to a +file. + +When exactly one of several concurrent processes needs to "win" a +decision, a single atomic filesystem operation can make it, rather than +a check followed by a separate action. A "test -f X" then "touch X" +(or "rm X") races: two processes can both pass the check before either +acts. Two atomic operations avoid this: + + - "mkdir dir", which fails if the directory already exists, so that + exactly one caller wins, electing a first or only request (see + t/lib-httpd/http-429.sh). + + - "mv src dst" (rename), which fails if the source is gone, so that + exactly one caller consumes it, claiming a planted one-shot marker + (see t/lib-httpd/apply-one-time-script.sh). + +A "$$" suffix on per-request scratch files keeps concurrent invocations +from clobbering each other's fixed-name files. + +This is a standard shell locking idiom, and the same reasoning behind +Git's own lockfile machinery, which creates its lock with O_CREAT|O_EXCL, +and make_symlink() in t/test-lib.sh, which uses an mkdir lock: an atomic +operation whose failure indicates that another process got there first. + + Test harness library -------------------- diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index fc646447d5c038..d64f9c8c2d0045 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -159,6 +159,9 @@ prepare_httpd() { mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH" cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH" cp "$TEST_PATH"/proxy-passwd "$HTTPD_ROOT_PATH" + # The web server can run any of these CGI scripts for two requests at + # once; a helper that keeps state between requests must do so with an + # atomic operation. See "Writing concurrency-safe helpers" in t/README. install_script incomplete-length-upload-pack-v2-http.sh install_script incomplete-body-upload-pack-v2-http.sh install_script error-no-report.sh