[PATCH 0/3] t/lib-httpd: make CGI test helpers concurrency-safe

[PATCH 0/3] t/lib-httpd: make CGI test helpers concurrency-safe

[Michael Montalbo via GitGitGadget]

The httpd tests share a handful of CGI helper scripts under t/lib-httpd. Two
of them keep state between requests in the shared HTTPD_ROOT_PATH on the
assumption that the web server hands them one request at a time. It does
not: Apache serves requests concurrently, and a single Git operation can
open more than one request to the same endpoint 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 being served.

Under that overlap apply-one-time-script.sh loses: two requests both pass
its "test -f one-time-script" check, one removes the marker, and the other
fails to exec it and emits an empty body, which the server answers as HTTP
500. In the field this is an occasional failure[1] of:

t5616.47 tolerate server sending REF_DELTA against missing promisor objects

on the macOS CI runners, with:

fatal: ... The requested URL returned error: 500 fatal: could not fetch from
promisor remote

I could not reproduce it against a live server (the window is tiny and
timing-dependent), but the macOS CI error log names the exact failure, and
the new test reproduces the helper's shell error.

http-429.sh keeps its "already returned 429 once" state with the same
non-atomic test-and-set. Its retry flow is mostly sequential so it seems
less likely to fail, but it is the same latent race.

Each fix is local: claim/consume the one-shot marker with an atomic rename,
and elect the first request with an atomic mkdir, rather than a "test -f"
followed by a separate remove or touch.

 * Patch 1 fixes apply-one-time-script.sh (the actual flake) and adds t5567,
   which drives the helper directly with no web server so the overlap can be
   forced deterministically.
 * Patch 2 makes http-429.sh atomic.
 * Patch 3 documents the atomic idioms generally in t/README (they are not
   specific to CGI or HTTP), citing Git's own lockfile machinery and
   make_symlink(), with a pointer from the lib-httpd list.

[1]
https://github.com/gitgitgadget/git/actions/runs/28756172690/job/85263916762?pr=2169

Michael Montalbo (3):
  t/lib-httpd: fix apply-one-time-script race under concurrent requests
  t/lib-httpd: make http-429 first-request check atomic
  t/README: document writing concurrency-safe helpers

 t/README                             | 32 ++++++++++
 t/lib-httpd.sh                       |  3 +
 t/lib-httpd/apply-one-time-script.sh | 38 +++++++----
 t/lib-httpd/http-429.sh              | 21 +++---
 t/meson.build                        |  1 +
 t/t5567-one-time-script.sh           | 96 ++++++++++++++++++++++++++++
 6 files changed, 166 insertions(+), 25 deletions(-)
 create mode 100755 t/t5567-one-time-script.sh


base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2171%2Fmmontalbo%2Fmm%2Flib-httpd-cgi-safe-proto-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2171
-- 
gitgitgadget

[PATCH 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests

[Michael Montalbo via GitGitGadget]

From: Michael Montalbo <mmontalbo@gmail.com>

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 <oid> 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 <mmontalbo@gmail.com>
---
 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 b1682944e2..a298ae89ae 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 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..cd8e656005
--- /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
-- 
gitgitgadget

Re: [PATCH 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests

[Junio C Hamano]

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Michael Montalbo <mmontalbo@gmail.com>
>
> 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 <oid> from promisor remote
>
> This has been seen as a flaky failure of t5616.47 on the macOS CI
> runners.

Thanks for this detailed write-up.  The analysis looks good.

> 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.

Hmph.  

> +#
> +# 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

The original was somehow inconsistent in that it forced C locale
only when one-time-script munged the output, and otherwise the
backend was run in the original locale.  I am not sure if that
matters very much.

> +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
> +	cat "$out"
>  fi

We may run the one-time script, find that it modified the payload,
and then another instance of us may start running before we can move
the one-time script away, so the second request can see "ah,
one-time-script is there, nobody has claimed it by renaming" and run
it again, no?  So this solution may shrink the race window but may
not completely eliminate it, unless we have some coordination among
ourselves, perhaps?

Ah, we assume running one-time-script itself multiple times is safe
and does not cause issues.  Our objective is to avoid returning
modified output twice.  So while the first instance of us
successfully renames one-time-script to one-time-script.$$ and emits
the modified result, even if the second instance raced and managed
to run the script again, it will fail to rename with "mv", and
discard the modified output, and instead show the unmodified output
generated by the backend.

OK.  It is a bit tricky.  It may help future readers if we said
something about this in the proposed log message (i.e., we consider
that it is perfectly fine to run one-time-script more than once; we
only want to avoid letting the second invocation's output used).

Thanks.

Re: [PATCH 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests

[Michael Montalbo]

On Wed, Jul 8, 2026 at 12:54 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> > +#
> > +# 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
>
> The original was somehow inconsistent in that it forced C locale
> only when one-time-script munged the output, and otherwise the
> backend was run in the original locale.  I am not sure if that
> matters very much.
>

I think it's still the same after the rewrite, though I could be
mistaken. If the
first `test -f` fails git-http-backend executes with inherited locale
(analogous to
the else branch execution in the original), and if `test -f` succeeds the locale
is forced to C and the one-time-script / git-http-backend run with the forced
locale. That being said, I think forcing the locale to C consistently would
make more sense. Depending on what you think, I can integrate that into the
series or leave for a future cleanup.

>
> Ah, we assume running one-time-script itself multiple times is safe
> and does not cause issues.  Our objective is to avoid returning
> modified output twice.  So while the first instance of us
> successfully renames one-time-script to one-time-script.$$ and emits
> the modified result, even if the second instance raced and managed
> to run the script again, it will fail to rename with "mv", and
> discard the modified output, and instead show the unmodified output
> generated by the backend.
>
> OK.  It is a bit tricky.  It may help future readers if we said
> something about this in the proposed log message (i.e., we consider
> that it is perfectly fine to run one-time-script more than once; we
> only want to avoid letting the second invocation's output used).
>

Yes that is a good call, I will add some detail about this subtlety in the
log message and helper comment.

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

[Michael Montalbo via GitGitGadget]

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

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

[Junio C Hamano]

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

OK.  And use of mkdir for atomicity is an obvious solution for such
a situtation.

> -if test -f "$state_file"
> +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"
> -

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

[Junio C Hamano]

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> -# 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

I think the last sentence in the above comment was meant to explain
why the new code checks the value of "$retry_after", but it is not
clear if it is needed for correctness (in other words, the original
was wrong to do "test -f && touch" but also was wrong to do so even
when "$retry_after" is set to "permanent), or if it is a mere
"optimization opportunity" you are taking advantage of.  In either
case, it would be nice to see it explained in the proposed commit
log message.

Thanks.

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

[Michael Montalbo]

On Wed, Jul 8, 2026 at 1:02 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > -# 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
>
> I think the last sentence in the above comment was meant to explain
> why the new code checks the value of "$retry_after", but it is not
> clear if it is needed for correctness (in other words, the original
> was wrong to do "test -f && touch" but also was wrong to do so even
> when "$retry_after" is set to "permanent), or if it is a mere
> "optimization opportunity" you are taking advantage of.  In either
> case, it would be nice to see it explained in the proposed commit
> log message.
>

It is needed for correctness, and I agree it is not very clear from the log
message / comment. I will spell out the reasoning for the change more
clearly in both.

Thanks for taking a look at this!

[PATCH 3/3] t/README: document writing concurrency-safe helpers

[Michael Montalbo via GitGitGadget]

From: Michael Montalbo <mmontalbo@gmail.com>

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 <mmontalbo@gmail.com>
---
 t/README       | 32 ++++++++++++++++++++++++++++++++
 t/lib-httpd.sh |  3 +++
 2 files changed, 35 insertions(+)

diff --git a/t/README b/t/README
index 085921be4b..a9d425f392 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 fc646447d5..d64f9c8c2d 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
-- 
gitgitgadget

Re: [PATCH 3/3] t/README: document writing concurrency-safe helpers

[Junio C Hamano]

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Michael Montalbo <mmontalbo@gmail.com>
>
> 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 <mmontalbo@gmail.com>
> ---
>  t/README       | 32 ++++++++++++++++++++++++++++++++
>  t/lib-httpd.sh |  3 +++
>  2 files changed, 35 insertions(+)

Thanks for a nice finishing touch.



> diff --git a/t/README b/t/README
> index 085921be4b..a9d425f392 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 fc646447d5..d64f9c8c2d 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

[PATCH v2 0/3] t/lib-httpd: make CGI test helpers concurrency-safe

[Michael Montalbo via GitGitGadget]

The httpd tests share a handful of CGI helper scripts under t/lib-httpd. Two
of them keep state between requests in the shared HTTPD_ROOT_PATH on the
assumption that the web server hands them one request at a time. It does
not: Apache serves requests concurrently, and a single Git operation can
open more than one request to the same endpoint 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 being served.

Under that overlap apply-one-time-script.sh loses: two requests both pass
its "test -f one-time-script" check, one removes the marker, and the other
fails to exec it and emits an empty body, which the server answers as HTTP
500. In the field this is an occasional failure[1] of:

t5616.47 tolerate server sending REF_DELTA against missing promisor objects

on the macOS CI runners, with:

fatal: ... The requested URL returned error: 500 fatal: could not fetch from
promisor remote

I could not reproduce it against a live server (the window is tiny and
timing-dependent), but the macOS CI error log names the exact failure, and
the new test reproduces the helper's shell error.

http-429.sh keeps its "already returned 429 once" state with the same
non-atomic test-and-set. Its retry flow is mostly sequential so it seems
less likely to fail, but it is the same latent race.

Each fix is local: claim/consume the one-shot marker with an atomic rename,
and elect the first request with an atomic mkdir, rather than a "test -f"
followed by a separate remove or touch.

 * Patch 1 fixes apply-one-time-script.sh (the actual flake) and adds t5567,
   which drives the helper directly with no web server so the overlap can be
   forced deterministically.
 * Patch 2 makes http-429.sh atomic.
 * Patch 3 documents the atomic idioms generally in t/README (they are not
   specific to CGI or HTTP), citing Git's own lockfile machinery and
   make_symlink(), with a pointer from the lib-httpd list.

Changes since v1:

 * Clarify that one-time-script.sh can and should be able to run more than
   once. Explain that the constraint on the script execution is that one and
   only one modified response is guaranteed to be returned to the client.

 * The existing behavior w.r.t. inconsistent use of locale C vs. inherited
   locale when executing t/lib-httpd/apply-one-time-script.sh has been
   retained from the original version, and is left as future potential
   cleanup.

 * Spell out why the logic changes to the "permanent mode check" in
   t/lib-httpd/http-429.sh are needed for correctness, rather than an
   optimization opportunity.

[1]
https://github.com/gitgitgadget/git/actions/runs/28756172690/job/85263916762?pr=2169

Michael Montalbo (3):
  t/lib-httpd: fix apply-one-time-script race under concurrent requests
  t/lib-httpd: make http-429 first-request check atomic
  t/README: document writing concurrency-safe helpers

 t/README                             | 32 ++++++++++
 t/lib-httpd.sh                       |  3 +
 t/lib-httpd/apply-one-time-script.sh | 44 +++++++++----
 t/lib-httpd/http-429.sh              | 28 ++++----
 t/meson.build                        |  1 +
 t/t5567-one-time-script.sh           | 96 ++++++++++++++++++++++++++++
 6 files changed, 179 insertions(+), 25 deletions(-)
 create mode 100755 t/t5567-one-time-script.sh


base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2171%2Fmmontalbo%2Fmm%2Flib-httpd-cgi-safe-proto-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2171

Range-diff vs v1:

 1:  9f48aa6d6d ! 1:  79b56402c0 t/lib-httpd: fix apply-one-time-script race under concurrent requests
     @@ Commit message
          body and keeps the marker for a later request. No path emits an empty
          body, so the HTTP 500 no longer occurs.
      
     +    Running the one-time script more than once is fine; the only thing to
     +    avoid is serving a second, racing request's modified output. Two
     +    requests can both find the marker and run the script before either
     +    renames it away, but the rename is atomic, so exactly one of them wins:
     +    it serves its modified body and consumes the marker. The loser's rename
     +    fails because the marker is already gone, so it discards the modified
     +    output it produced and serves the unmodified body instead. The rename,
     +    not running the script, is what is serialized.
     +
          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
     @@ t/lib-httpd/apply-one-time-script.sh
      +# 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"
     ++#
     ++# The script may run more than once: the marker is consumed when the response
     ++# actually changes (the rename after "cmp"), not when the script runs, so a
     ++# request whose response is not the targeted one runs the script, sees no
     ++# change, and leaves the marker for a later request. That is safe because the
     ++# scripts are stateless filters over the captured response.
       
      -	"$GIT_EXEC_PATH/git-http-backend" >out
      -	./one-time-script out >out_modified
     -+LC_ALL=C
     -+export LC_ALL
     ++test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend"
       
      -	if cmp -s out out_modified
      -	then
     @@ t/lib-httpd/apply-one-time-script.sh
      -		cat out_modified
      -		rm one-time-script
      -	fi
     ++LC_ALL=C
     ++export LC_ALL
     ++
      +out=out.$$
      +modified=out-modified.$$
      +"$GIT_EXEC_PATH/git-http-backend" >"$out"
 2:  efd34c1715 ! 2:  5f56f32a74 t/lib-httpd: make http-429 first-request check atomic
     @@ Commit message
          which fails if the directory already exists, so exactly one of any
          concurrent requests is rate-limited and the rest are forwarded.
      
     +    Skipping state for "permanent" is required for correctness, not just an
     +    optimization. The marker tells a later or concurrent request that a 429
     +    has already been served, so that it forwards to git-http-backend instead
     +    of rate-limiting. Since "permanent" must return 429 to every request,
     +    that marker must never become visible to another such request.
     +
     +    The original did not achieve this by staying stateless: its "touch" of
     +    the marker ran unconditionally, and the "permanent" case removed it
     +    afterward with "rm -f". That create-then-remove leaves a window in which
     +    a concurrent "permanent" request sees the marker and is forwarded. It is
     +    the same class of check-then-act race this patch removes from the
     +    first-request check, latent for the same reason: the flow is mostly
     +    sequential. This version fuses the check and the mark into one atomic
     +    mkdir and, rather than recreate the pattern as mkdir-then-rmdir, skips
     +    the mkdir for "permanent" with a "!= permanent" guard. No marker is ever
     +    created, so there is no window and every "permanent" request
     +    rate-limits.
     +
          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
     @@ t/lib-httpd/http-429.sh: repo_path="${remaining#*/}"  # Get rest (repo path)
       
      -# 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.
     ++# This endpoint returns 429 to the first request and forwards later ones to
     ++# git-http-backend, so the retry succeeds. Apache can run this CGI for several
     ++# requests at once, so a single atomic "mkdir" elects that first request: the
     ++# one whose mkdir succeeds returns 429 and leaves the directory behind as the
     ++# "already rate-limited" marker; every later request finds the directory (mkdir
     ++# fails) and is forwarded.
     ++#
     ++# "permanent" is the exception: it must return 429 to every request and never
     ++# succeed, so it skips the mkdir and records no state. A leftover directory
     ++# would make its own later requests find the marker and be forwarded, which is
     ++# exactly what "permanent" must not do.
      +if test "$retry_after" != permanent && ! mkdir "$state" 2>/dev/null
       then
       	# Already returned 429 once, forward to git-http-backend
 3:  771d264d29 = 3:  f158e1f92e t/README: document writing concurrency-safe helpers

-- 
gitgitgadget

[PATCH v2 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests

[Michael Montalbo via GitGitGadget]

From: Michael Montalbo <mmontalbo@gmail.com>

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 <oid> 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.

Running the one-time script more than once is fine; the only thing to
avoid is serving a second, racing request's modified output. Two
requests can both find the marker and run the script before either
renames it away, but the rename is atomic, so exactly one of them wins:
it serves its modified body and consumes the marker. The loser's rename
fails because the marker is already gone, so it discards the modified
output it produced and serves the unmodified body instead. The rename,
not running the script, is what is serialized.

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 <mmontalbo@gmail.com>
---
 t/lib-httpd/apply-one-time-script.sh | 44 +++++++++----
 t/meson.build                        |  1 +
 t/t5567-one-time-script.sh           | 96 ++++++++++++++++++++++++++++
 3 files changed, 127 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 b1682944e2..adb9cec528 100644
--- a/t/lib-httpd/apply-one-time-script.sh
+++ b/t/lib-httpd/apply-one-time-script.sh
@@ -6,21 +6,37 @@
 #
 # 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.
+#
+# The script may run more than once: the marker is consumed when the response
+# actually changes (the rename after "cmp"), not when the script runs, so a
+# request whose response is not the targeted one runs the script, sees no
+# change, and leaves the marker for a later request. That is safe because the
+# scripts are stateless filters over the captured response.
 
-	"$GIT_EXEC_PATH/git-http-backend" >out
-	./one-time-script out >out_modified
+test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend"
 
-	if cmp -s out out_modified
-	then
-		cat out
-	else
-		cat out_modified
-		rm one-time-script
-	fi
+LC_ALL=C
+export LC_ALL
+
+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 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..cd8e656005
--- /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
-- 
gitgitgadget

Re: [PATCH v2 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests

[Patrick Steinhardt]

On Fri, Jul 10, 2026 at 05:30:55PM +0000, Michael Montalbo via GitGitGadget wrote:
> diff --git a/t/lib-httpd/apply-one-time-script.sh b/t/lib-httpd/apply-one-time-script.sh
> index b1682944e2..adb9cec528 100644
> --- a/t/lib-httpd/apply-one-time-script.sh
> +++ b/t/lib-httpd/apply-one-time-script.sh
> @@ -6,21 +6,37 @@
>  #
>  # 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.
> +#
> +# The script may run more than once: the marker is consumed when the response
> +# actually changes (the rename after "cmp"), not when the script runs, so a
> +# request whose response is not the targeted one runs the script, sees no
> +# change, and leaves the marker for a later request. That is safe because the
> +# scripts are stateless filters over the captured response.
>  
> -	"$GIT_EXEC_PATH/git-http-backend" >out
> -	./one-time-script out >out_modified
> +test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend"
>  
> -	if cmp -s out out_modified
> -	then
> -		cat out
> -	else
> -		cat out_modified
> -		rm one-time-script
> -	fi
> +LC_ALL=C
> +export LC_ALL
> +
> +out=out.$$
> +modified=out-modified.$$
> +"$GIT_EXEC_PATH/git-http-backend" >"$out"
> +
> +if ./one-time-script "$out" 2>/dev/null >"$modified" &&

Is it intentional that we swallow stderr of this script now? We didn't
before. I assume that this is to swallow the error in case the script
got removed by the concurrent request?

Patrick

Re: [PATCH v2 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests

[Michael Montalbo]

On Tue, Aug 4, 2026 at 1:03 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> > +
> > +out=out.$$
> > +modified=out-modified.$$
> > +"$GIT_EXEC_PATH/git-http-backend" >"$out"
> > +
> > +if ./one-time-script "$out" 2>/dev/null >"$modified" &&
>
> Is it intentional that we swallow stderr of this script now? We didn't
> before. I assume that this is to swallow the error in case the script
> got removed by the concurrent request?
>

Yes, you are correct on both counts. This is an intentional change
meant to swallow (an expected) stderr in case the script got removed
already by a concurrent request, but that is not clear on its own. I will
add an explanatory comment spelling this out.

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

[Michael Montalbo via GitGitGadget]

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.

Skipping state for "permanent" is required for correctness, not just an
optimization. The marker tells a later or concurrent request that a 429
has already been served, so that it forwards to git-http-backend instead
of rate-limiting. Since "permanent" must return 429 to every request,
that marker must never become visible to another such request.

The original did not achieve this by staying stateless: its "touch" of
the marker ran unconditionally, and the "permanent" case removed it
afterward with "rm -f". That create-then-remove leaves a window in which
a concurrent "permanent" request sees the marker and is forwarded. It is
the same class of check-then-act race this patch removes from the
first-request check, latent for the same reason: the flow is mostly
sequential. This version fuses the check and the mark into one atomic
mkdir and, rather than recreate the pattern as mkdir-then-rmdir, skips
the mkdir for "permanent" with a "!= permanent" guard. No marker is ever
created, so there is no window and every "permanent" request
rate-limits.

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 | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/t/lib-httpd/http-429.sh b/t/lib-httpd/http-429.sh
index c97b16145b..9746ec67ae 100644
--- a/t/lib-httpd/http-429.sh
+++ b/t/lib-httpd/http-429.sh
@@ -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
+# 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"
+# This endpoint returns 429 to the first request and forwards later ones to
+# git-http-backend, so the retry succeeds. Apache can run this CGI for several
+# requests at once, so a single atomic "mkdir" elects that first request: the
+# one whose mkdir succeeds returns 429 and leaves the directory behind as the
+# "already rate-limited" marker; every later request finds the directory (mkdir
+# fails) and is forwarded.
+#
+# "permanent" is the exception: it must return 429 to every request and never
+# succeed, so it skips the mkdir and records no state. A leftover directory
+# would make its own later requests find the marker and be forwarded, which is
+# exactly what "permanent" must not do.
+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

[PATCH v2 3/3] t/README: document writing concurrency-safe helpers

[Michael Montalbo via GitGitGadget]

From: Michael Montalbo <mmontalbo@gmail.com>

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 <mmontalbo@gmail.com>
---
 t/README       | 32 ++++++++++++++++++++++++++++++++
 t/lib-httpd.sh |  3 +++
 2 files changed, 35 insertions(+)

diff --git a/t/README b/t/README
index 085921be4b..a9d425f392 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 fc646447d5..d64f9c8c2d 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
-- 
gitgitgadget

Re: [PATCH v2 3/3] t/README: document writing concurrency-safe helpers

[Patrick Steinhardt]

On Fri, Jul 10, 2026 at 05:30:57PM +0000, Michael Montalbo via GitGitGadget wrote:
> diff --git a/t/README b/t/README
> index 085921be4b..a9d425f392 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
> +--------------------------------

Nit: this paragraph is quite specific to lib-httpd, so it would make
sense to mention it in the header here. E.g.

    Writing concurrency-safe lib-httpd 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 simple "rm" (without "-f") should work as well, right?

> +A "$$" suffix on per-request scratch files keeps concurrent invocations
> +from clobbering each other's fixed-name files.

Nit: it might be a bit easier to read if we explicitly mention PIDs
instead of assuming that every reader immediately knows that "$$" will
expand to the PID. E.g.:

    Appending a PID to the per-request scratch filenames keeps...

Thanks!

Patrick

Re: [PATCH v2 3/3] t/README: document writing concurrency-safe helpers

[Michael Montalbo]

On Tue, Aug 4, 2026 at 1:03 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> >
> > +Writing concurrency-safe helpers
> > +--------------------------------
>
> Nit: this paragraph is quite specific to lib-httpd, so it would make
> sense to mention it in the header here. E.g.
>
>     Writing concurrency-safe lib-httpd helpers
>

Originally, I did just have this as a blurb in t/lib-httpd.sh. I ended up moving
it here and trying to make the advice apply more generally, though the only
other existing example I could find in another domain was the
make_symlink() reference. My intention was to make sure someone working
on a test helper with concurrency didn't skip over the section just because
they saw "http" and thought the advice didn't apply to their use case.

I'm inclined to make the language in the section more http-agnostic rather
than changing the title to be specific to http, but I do not feel very strongly
about it. If we were to frame this as http-specific advice maybe it should go
back to t/lib-httpd.sh instead of t/README?

>
> A simple "rm" (without "-f") should work as well, right?
>

Yes, definitely. I think I over-corrected in excising "rm" from the test helpers
and the advice given here since I associated it with the flawed patterns that
allowed for the race issues. I will redo the treatment of "rm" in the series
including reverting where "mv" replaced "rm" unnecessarily in the helpers.

> > +A "$$" suffix on per-request scratch files keeps concurrent invocations
> > +from clobbering each other's fixed-name files.
>
> Nit: it might be a bit easier to read if we explicitly mention PIDs
> instead of assuming that every reader immediately knows that "$$" will
> expand to the PID. E.g.:
>
>     Appending a PID to the per-request scratch filenames keeps...
>

Agreed, will fix.

> Thanks!
>

Thank you for taking a look and your feedback!

Re: [PATCH v2 3/3] t/README: document writing concurrency-safe helpers

[Patrick Steinhardt]

On Fri, Aug 07, 2026 at 09:51:34AM -0700, Michael Montalbo wrote:
> On Tue, Aug 4, 2026 at 1:03 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > >
> > > +Writing concurrency-safe helpers
> > > +--------------------------------
> >
> > Nit: this paragraph is quite specific to lib-httpd, so it would make
> > sense to mention it in the header here. E.g.
> >
> >     Writing concurrency-safe lib-httpd helpers
> >
> 
> Originally, I did just have this as a blurb in t/lib-httpd.sh. I ended up moving
> it here and trying to make the advice apply more generally, though the only
> other existing example I could find in another domain was the
> make_symlink() reference. My intention was to make sure someone working
> on a test helper with concurrency didn't skip over the section just because
> they saw "http" and thought the advice didn't apply to their use case.
> 
> I'm inclined to make the language in the section more http-agnostic rather
> than changing the title to be specific to http, but I do not feel very strongly
> about it. If we were to frame this as http-specific advice maybe it should go
> back to t/lib-httpd.sh instead of t/README?

Dunno. I'm not sure there's much value outside of httpd, so I'm still
inclined to make it httpd-specific. And if so, moving it into "t/" would
make sense.

But I don't feel overly strong about this, either, so I won't complain
if this section stays as-is.

Patrick

Re: [PATCH v2 0/3] t/lib-httpd: make CGI test helpers concurrency-safe

[Michael Montalbo]

Friendly ping. If it makes it any more enticing, I believe the flake fixed in
this series is responsible for at least a couple CI failures[1][2] since the
submission occurred.

[1] https://github.com/gitgitgadget/git/actions/runs/28983114431/job/86006743571
[2] https://github.com/git/git/actions/runs/29063352938/job/86269734698

Re: [PATCH v2 0/3] t/lib-httpd: make CGI test helpers concurrency-safe

[Junio C Hamano]

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Each fix is local: claim/consume the one-shot marker with an atomic rename,
> and elect the first request with an atomic mkdir, rather than a "test -f"
> followed by a separate remove or touch.
>
>  * Patch 1 fixes apply-one-time-script.sh (the actual flake) and adds t5567,
>    which drives the helper directly with no web server so the overlap can be
>    forced deterministically.
>  * Patch 2 makes http-429.sh atomic.
>  * Patch 3 documents the atomic idioms generally in t/README (they are not
>    specific to CGI or HTTP), citing Git's own lockfile machinery and
>    make_symlink(), with a pointer from the lib-httpd list.

I was scanning the "What's cooking" report for topics marked as
"Needs review" to see if I could find ones that are relatively easy
to validate, and I hit this one.

The key change [1/3] is well thought out and nicely done.  [2/3] is
explained better than the corresponding step in v1, and [3/3] adds
helpful tips to the t/README documentation.  They all look quite
good.

Thanks.

[PATCH v3 0/3] t/lib-httpd: make CGI test helpers concurrency-safe

[Michael Montalbo via GitGitGadget]

The httpd tests share a handful of CGI helper scripts under t/lib-httpd. Two
of them keep state between requests in the shared HTTPD_ROOT_PATH, on the
assumption that the web server hands them one request at a time. It does
not: Apache serves requests concurrently, and a single Git operation can
open more than one request to the same endpoint at once. For example, a
partial fetch that receives a REF_DELTA against a missing promisor object
lazily fetches that base while the first response is still being served.

Under that overlap apply-one-time-script.sh fails. Two requests both pass
its "test -f one-time-script" check; one removes the marker; the other then
fails to exec it, emits an empty body, and the server answers HTTP 500. In
the field this is an occasional failure[1] of

t5616.47 tolerate server sending REF_DELTA against missing promisor objects

on the macOS CI runners, with

fatal: ... The requested URL returned error: 500 fatal: could not fetch from
promisor remote

I could not reproduce it against a live server, since the window is tiny and
timing-dependent, but the macOS CI error log names the exact failure and the
new test reproduces the helper's shell error.

http-429.sh keeps its "already returned 429 once" state with the same
non-atomic check-and-set. Its retry flow is mostly sequential, so it seems
less likely to fail, but it is the same latent race.

Each helper replaces a non-atomic "test -f" check and separate follow-up
action with a single atomic operation whose exit status decides the outcome:
apply-one-time-script.sh consumes its one-shot marker with "rm" (without
"-f"), and http-429.sh elects the first request with "mkdir".

 * Patch 1 fixes apply-one-time-script.sh (the actual flake) and adds t5567,
   which drives the helper directly with no web server so the overlap can be
   forced deterministically.
 * Patch 2 makes http-429.sh atomic.
 * Patch 3 documents the atomic idioms next to where t/lib-httpd.sh installs
   the CGI scripts, so the guidance is in front of anyone adding another
   helper.

Changes since v2:

 * Patch 1 now consumes the marker with a plain "rm" (without "-f") instead
   of a rename. "rm" without "-f" already fails once the marker is gone,
   which is the atomicity the helper needs. A new comment explains why the
   helper discards the one-time script's stderr: a losing request can find
   the marker already removed.

 * Patch 3 is now specific to the lib-httpd CGI helpers and lives beside
   their install site in t/lib-httpd.sh, rather than as a general section in
   t/README.

 * Reworded several helper comments and the patch 1 and 2 log messages for
   clarity and to match the code; no behavior change.

[1]
https://github.com/gitgitgadget/git/actions/runs/28756172690/job/85263916762?pr=2169

Michael Montalbo (3):
  t/lib-httpd: fix apply-one-time-script race under concurrent requests
  t/lib-httpd: make http-429 first-request check atomic
  t/lib-httpd: document writing concurrency-safe CGI helpers

 t/lib-httpd.sh                       | 13 ++++
 t/lib-httpd/apply-one-time-script.sh | 50 +++++++++++----
 t/lib-httpd/http-429.sh              | 30 +++++----
 t/meson.build                        |  1 +
 t/t5567-one-time-script.sh           | 96 ++++++++++++++++++++++++++++
 5 files changed, 164 insertions(+), 26 deletions(-)
 create mode 100755 t/t5567-one-time-script.sh


base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2171%2Fmmontalbo%2Fmm%2Flib-httpd-cgi-safe-proto-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2171

Range-diff vs v2:

 1:  79b56402c0 ! 1:  862c4258e5 t/lib-httpd: fix apply-one-time-script race under concurrent requests
     @@ Metadata
       ## Commit message ##
          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:
     +    apply-one-time-script.sh is a CGI helper that, when the file
     +    "one-time-script" is present, runs it to rewrite the git-http-backend
     +    response. If "one-time-script" generates a response that differs from
     +    git-http-backend, the modified response is returned and
     +    "one-time-script" is deleted. Requests after the deletion return normal
     +    git-http-backend responses.
     +
     +    The deletion is not safe under concurrency. The helper serves the
     +    modified body first and deletes "one-time-script" only afterward, so a
     +    client can issue its next request while the file still exists. Apache
     +    runs the CGI for both requests at once, for example when a partial fetch
     +    lazily fetches a missing promisor base while the first response is still
     +    in flight. Both requests find the file and try to run it; the first
     +    deletes it; the second then fails to exec the now-missing file, produces
     +    no output, and the server returns HTTP 500:
      
            fatal: ... The requested URL returned error: 500
            fatal: could not fetch <oid> 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.
     +    This is the flaky failure of t5616.47 on the macOS CI runners.
      
     -    Running the one-time script more than once is fine; the only thing to
     -    avoid is serving a second, racing request's modified output. Two
     -    requests can both find the marker and run the script before either
     -    renames it away, but the rename is atomic, so exactly one of them wins:
     -    it serves its modified body and consumes the marker. The loser's rename
     -    fails because the marker is already gone, so it discards the modified
     -    output it produced and serves the unmodified body instead. The rename,
     -    not running the script, is what is serialized.
     +    Fix it by removing the file with "rm" only after the script has actually
     +    changed the response. Because "rm" without "-f" fails once the file is
     +    gone, exactly one request removes it and serves the modified body. Any
     +    other request serves the unmodified body. Running the script more than
     +    once is harmless; only its deletion is serialized, so exactly one
     +    request's modified response is ever served. Per-request scratch file
     +    names keep concurrent runs from overwriting each other, and no path
     +    emits an empty response body.
      
     -    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:
     +    t5616.47 exercises the real code path but, being timing-dependent,
     +    passes against the buggy helper almost every time. Add t5567, which
     +    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
      
     @@ t/lib-httpd/apply-one-time-script.sh
      -	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.
     ++# Apache can run this CGI for several requests at the same time. For example, a
     ++# partial fetch lazily fetches a missing object while the first response is
     ++# still in flight. To stay correct, the helper removes the marker only after
     ++# the response changes, and only with "rm" (without "-f"). The "rm" fails for
     ++# every request except the one that removes the marker first. That request
     ++# serves the modified body. Every other request serves its response unchanged.
     ++# No request emits an empty body, which Apache would report as HTTP 500.
     ++#
     ++# A scratch file name includes the process ID ($$), so concurrent requests do
     ++# not overwrite each other's files.
      +#
     -+# The script may run more than once: the marker is consumed when the response
     -+# actually changes (the rename after "cmp"), not when the script runs, so a
     -+# request whose response is not the targeted one runs the script, sees no
     -+# change, and leaves the marker for a later request. That is safe because the
     ++# The helper can run one-time-script more than once. It consumes the marker
     ++# when the response changes (the "rm" after "cmp"), not when it runs the
     ++# script. A request whose response is not the target runs the script, finds no
     ++# change, and leaves the marker for a later request. This is safe because the
      +# scripts are stateless filters over the captured response.
       
      -	"$GIT_EXEC_PATH/git-http-backend" >out
     @@ t/lib-httpd/apply-one-time-script.sh
      +modified=out-modified.$$
      +"$GIT_EXEC_PATH/git-http-backend" >"$out"
      +
     ++# one-time-script can be gone here: a concurrent request may have consumed it
     ++# since the "test -f" above. Then "./one-time-script" fails, the exit status
     ++# selects the unmodified body, and "2>/dev/null" discards the expected
     ++# "no such file" message.
      +if ./one-time-script "$out" 2>/dev/null >"$modified" &&
      +   ! cmp -s "$out" "$modified" &&
     -+   mv one-time-script one-time-script.$$ 2>/dev/null
     ++   rm 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.$$
     ++rm -f "$out" "$modified"
      
       ## t/meson.build ##
      @@ t/meson.build: integration_tests = [
 2:  5f56f32a74 ! 2:  8ed22c02a1 t/lib-httpd: make http-429 first-request check atomic
     @@ Metadata
       ## Commit message ##
          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.
     +    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".
      
     -    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.
     +    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.
      
     -    Skipping state for "permanent" is required for correctness, not just an
     -    optimization. The marker tells a later or concurrent request that a 429
     -    has already been served, so that it forwards to git-http-backend instead
     -    of rate-limiting. Since "permanent" must return 429 to every request,
     -    that marker must never become visible to another such request.
     +    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.
      
     -    The original did not achieve this by staying stateless: its "touch" of
     -    the marker ran unconditionally, and the "permanent" case removed it
     -    afterward with "rm -f". That create-then-remove leaves a window in which
     -    a concurrent "permanent" request sees the marker and is forwarded. It is
     -    the same class of check-then-act race this patch removes from the
     -    first-request check, latent for the same reason: the flow is mostly
     -    sequential. This version fuses the check and the mark into one atomic
     -    mkdir and, rather than recreate the pattern as mkdir-then-rmdir, skips
     -    the mkdir for "permanent" with a "!= permanent" guard. No marker is ever
     -    created, so there is no window and every "permanent" request
     -    rate-limits.
     -
     -    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.
     +    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 ##
     +@@
     + # 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
      @@ t/lib-httpd/http-429.sh: repo_path="${remaining#*/}"  # Get rest (repo path)
       # The repo name is the first component before any "/"
       repo_name="${repo_path%%/*}"
     @@ t/lib-httpd/http-429.sh: repo_path="${remaining#*/}"  # Get rest (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.
     ++# 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 and forwards later ones to
     -+# git-http-backend, so the retry succeeds. Apache can run this CGI for several
     -+# requests at once, so a single atomic "mkdir" elects that first request: the
     -+# one whose mkdir succeeds returns 429 and leaves the directory behind as the
     -+# "already rate-limited" marker; every later request finds the directory (mkdir
     -+# fails) and is forwarded.
     ++# 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 and never
     -+# succeed, so it skips the mkdir and records no state. A leftover directory
     -+# would make its own later requests find the marker and be forwarded, which is
     -+# exactly what "permanent" must not do.
     ++# "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
 3:  f158e1f92e ! 3:  374d148f43 t/README: document writing concurrency-safe helpers
     @@ Metadata
      Author: Michael Montalbo <mmontalbo@gmail.com>
      
       ## Commit message ##
     -    t/README: document writing concurrency-safe helpers
     +    t/lib-httpd: document writing concurrency-safe CGI 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.
     +    The apply-one-time-script.sh and http-429.sh fixes share a root cause: a
     +    CGI helper assumed it had a file to itself, when Apache can run the
     +    helper for several requests at once. Document the atomic idioms that
     +    avoid this next to where lib-httpd.sh installs the CGI scripts, so the
     +    advice is in front of anyone adding another one.
      
     -    Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
     +    The note describes the anti-pattern, a "test -f" check followed by a
     +    separate action, and the two atomic alternatives these helpers now use:
     +
     +     - "mkdir", which fails if the directory exists, to elect the first
     +       request (http-429.sh); and
     +     - "rm" without "-f", which fails once the file is gone, to consume a
     +       one-shot marker (apply-one-time-script.sh).
      
     - ## t/README ##
     -@@ t/README: 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
     - --------------------
     - 
     +    Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
      
       ## t/lib-httpd.sh ##
      @@ t/lib-httpd.sh: 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.
     ++	# Apache runs each of these CGI scripts once per request. Apache can run one
     ++	# script for several requests at the same time. A helper that keeps state
     ++	# between requests must update that state with one atomic operation. A check
     ++	# and then a separate action is not safe: two requests can both pass the
     ++	# check before either one acts. Test the exit status of one atomic operation
     ++	# instead:
     ++	#   - "mkdir dir" fails if the directory exists, so only one request
     ++	#     succeeds. http-429.sh selects the first request this way.
     ++	#   - "rm marker" (without "-f") fails if the marker is gone, so only one
     ++	#     request consumes it. apply-one-time-script.sh claims its one-shot
     ++	#     marker this way.
     ++	# A scratch file name includes the process ID ($$), so concurrent requests
     ++	# do not overwrite each other's files.
       	install_script incomplete-length-upload-pack-v2-http.sh
       	install_script incomplete-body-upload-pack-v2-http.sh
       	install_script error-no-report.sh

-- 
gitgitgadget

[PATCH v3 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests

[Michael Montalbo via GitGitGadget]

From: Michael Montalbo <mmontalbo@gmail.com>

apply-one-time-script.sh is a CGI helper that, when the file
"one-time-script" is present, runs it to rewrite the git-http-backend
response. If "one-time-script" generates a response that differs from
git-http-backend, the modified response is returned and
"one-time-script" is deleted. Requests after the deletion return normal
git-http-backend responses.

The deletion is not safe under concurrency. The helper serves the
modified body first and deletes "one-time-script" only afterward, so a
client can issue its next request while the file still exists. Apache
runs the CGI for both requests at once, for example when a partial fetch
lazily fetches a missing promisor base while the first response is still
in flight. Both requests find the file and try to run it; the first
deletes it; the second then fails to exec the now-missing file, produces
no output, and the server returns HTTP 500:

  fatal: ... The requested URL returned error: 500
  fatal: could not fetch <oid> from promisor remote

This is the flaky failure of t5616.47 on the macOS CI runners.

Fix it by removing the file with "rm" only after the script has actually
changed the response. Because "rm" without "-f" fails once the file is
gone, exactly one request removes it and serves the modified body. Any
other request serves the unmodified body. Running the script more than
once is harmless; only its deletion is serialized, so exactly one
request's modified response is ever served. Per-request scratch file
names keep concurrent runs from overwriting each other, and no path
emits an empty response body.

t5616.47 exercises the real code path but, being timing-dependent,
passes against the buggy helper almost every time. Add t5567, which
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 <mmontalbo@gmail.com>
---
 t/lib-httpd/apply-one-time-script.sh | 50 +++++++++++----
 t/meson.build                        |  1 +
 t/t5567-one-time-script.sh           | 96 ++++++++++++++++++++++++++++
 3 files changed, 133 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 b1682944e2..8ab97e882a 100644
--- a/t/lib-httpd/apply-one-time-script.sh
+++ b/t/lib-httpd/apply-one-time-script.sh
@@ -6,21 +6,43 @@
 #
 # 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 several requests at the same time. For example, a
+# partial fetch lazily fetches a missing object while the first response is
+# still in flight. To stay correct, the helper removes the marker only after
+# the response changes, and only with "rm" (without "-f"). The "rm" fails for
+# every request except the one that removes the marker first. That request
+# serves the modified body. Every other request serves its response unchanged.
+# No request emits an empty body, which Apache would report as HTTP 500.
+#
+# A scratch file name includes the process ID ($$), so concurrent requests do
+# not overwrite each other's files.
+#
+# The helper can run one-time-script more than once. It consumes the marker
+# when the response changes (the "rm" after "cmp"), not when it runs the
+# script. A request whose response is not the target runs the script, finds no
+# change, and leaves the marker for a later request. This is safe because the
+# scripts are stateless filters over the captured response.
 
-	"$GIT_EXEC_PATH/git-http-backend" >out
-	./one-time-script out >out_modified
+test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend"
 
-	if cmp -s out out_modified
-	then
-		cat out
-	else
-		cat out_modified
-		rm one-time-script
-	fi
+LC_ALL=C
+export LC_ALL
+
+out=out.$$
+modified=out-modified.$$
+"$GIT_EXEC_PATH/git-http-backend" >"$out"
+
+# one-time-script can be gone here: a concurrent request may have consumed it
+# since the "test -f" above. Then "./one-time-script" fails, the exit status
+# selects the unmodified body, and "2>/dev/null" discards the expected
+# "no such file" message.
+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"
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..cd8e656005
--- /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
-- 
gitgitgadget

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

[Michael Montalbo via GitGitGadget]

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

[PATCH v3 3/3] t/lib-httpd: document writing concurrency-safe CGI helpers

[Michael Montalbo via GitGitGadget]

From: Michael Montalbo <mmontalbo@gmail.com>

The apply-one-time-script.sh and http-429.sh fixes share a root cause: a
CGI helper assumed it had a file to itself, when Apache can run the
helper for several requests at once. Document the atomic idioms that
avoid this next to where lib-httpd.sh installs the CGI scripts, so the
advice is in front of anyone adding another one.

The note describes the anti-pattern, a "test -f" check followed by a
separate action, and the two atomic alternatives these helpers now use:

 - "mkdir", which fails if the directory exists, to elect the first
   request (http-429.sh); and
 - "rm" without "-f", which fails once the file is gone, to consume a
   one-shot marker (apply-one-time-script.sh).

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

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index fc646447d5..f26e1594ab 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -159,6 +159,19 @@ prepare_httpd() {
 	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
 	cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
 	cp "$TEST_PATH"/proxy-passwd "$HTTPD_ROOT_PATH"
+	# Apache runs each of these CGI scripts once per request. Apache can run one
+	# script for several requests at the same time. A helper that keeps state
+	# between requests must update that state with one atomic operation. A check
+	# and then a separate action is not safe: two requests can both pass the
+	# check before either one acts. Test the exit status of one atomic operation
+	# instead:
+	#   - "mkdir dir" fails if the directory exists, so only one request
+	#     succeeds. http-429.sh selects the first request this way.
+	#   - "rm marker" (without "-f") fails if the marker is gone, so only one
+	#     request consumes it. apply-one-time-script.sh claims its one-shot
+	#     marker this way.
+	# A scratch file name includes the process ID ($$), so concurrent requests
+	# do not overwrite each other's files.
 	install_script incomplete-length-upload-pack-v2-http.sh
 	install_script incomplete-body-upload-pack-v2-http.sh
 	install_script error-no-report.sh
-- 
gitgitgadget