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

["Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com>]

t/lib-httpd.sh provides several helpers that can be invoked concurrently by
Apache while exercising tests. Currently, two of these helpers use state
management logic that fails under certain race conditions.

apply-one-time-script.sh is one of those test helpers. It executes a
"one-time-script" responsible for modifying the response normally returned
by git-http-backend. Sometimes a race between multiple concurrent requests
causes apply-one-time-script.sh to misbehave and return multiple modified
responses or an empty response that results in:

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

This can be seen in the flaky failure of t5616.47 on the macOS CI
runners[1].

Fix this by chaining (&&) the logic for executing "one-time-script" with its
removal, rather than running them as separate actions. Add
t/t5567-one-time-script.sh to verify this fix is effective.

http-429.sh is the other helper whose state management logic can fail under
certain race conditions. However, these failures do not manifest themselves
currently since http-429.sh is invoked sequentially.

As a preventive measure, fix http-429.sh's state management logic so it
relies on an atomic mkdir operation to mark that a 429 was returned rather
than separate "test -f marker", "touch marker", and "rm -f marker" actions
to manage state. http-429.sh is not as straightforward to test as
apply-one-time-script.sh, which is why no regression test was added for the
change.

Finally, document these patterns and anti-patterns in t/lib-httpd.sh for
future developers.

Changes since v3:

 * Rewrite all the prose in the series from scratch without AI to remove
   fluff.
 * Fix the lack of clarity around the actual fix applied to
   apply-one-time-script.sh, which ultimately has nothing to do with rm
   itself, but rather how rm is used in conjunction with the surrounding
   state management logic.
 * No logical 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                       | 11 ++++
 t/lib-httpd/apply-one-time-script.sh | 38 +++++++----
 t/lib-httpd/http-429.sh              | 22 +++----
 t/meson.build                        |  1 +
 t/t5567-one-time-script.sh           | 96 ++++++++++++++++++++++++++++
 5 files changed, 142 insertions(+), 26 deletions(-)
 create mode 100755 t/t5567-one-time-script.sh


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

Range-diff vs v3:

 1:  862c4258e5 ! 1:  e202142f19 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 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:
     +    apply-one-time-script.sh is a test helper that executes a
     +    "one-time-script" responsible for modifying the response normally
     +    returned by git-http-backend. apply-one-time-script.sh should run
     +    "one-time-script" once and return a modified response once. However,
     +    sometimes a race between multiple concurrent requests causes
     +    apply-one-time-script.sh to misbehave and return multiple modified
     +    responses or an empty response that results in:
      
            fatal: ... The requested URL returned error: 500
            fatal: could not fetch <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.
     +    This can be seen in the flaky failure of t5616.47 on the macOS CI
     +    runners.
      
     -    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:
     +    Fix the logic that checks if "one-time-script" has returned its modified
     +    response by chaining "rm one-time-script" with its execution. This
     +    ensures a racing script does not also have the opportunity to execute
     +    "one-time-script".
      
     -      ./one-time-script: No such file or directory
     +    Add t/t5567-one-time-script.sh to verify the race is fixed. Implement a
     +    stub "git-http-backend" that intentionally invokes a concurrent request,
     +    and check that only one modified response is returned without error.
      
          Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
      
     @@ t/lib-httpd/apply-one-time-script.sh
      -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.
     ++test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend"
     ++
     ++LC_ALL=C
     ++export LC_ALL
       
      -	"$GIT_EXEC_PATH/git-http-backend" >out
      -	./one-time-script out >out_modified
     -+test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend"
     ++out=out.$$
     ++modified=out-modified.$$
     ++"$GIT_EXEC_PATH/git-http-backend" >"$out"
       
      -	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"
     -+
     -+# 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.
     ++# Since Apache can execute this script for multiple requests
     ++# concurrently, we chain "rm one-time-script" with the logic
     ++# for generating a modified response. If the "rm" ran separately,
     ++# a concurrent request could pass the "test -f" above and
     ++# erroneously result in multiple modified responses or an empty
     ++# body depending on the race state.
     ++#
     ++# We discard stderr for ./one-time-script since it is possible
     ++# ./one-time-script has been removed already, which is expected
     ++# sometimes. In this case, the unmodified response will be returned.
      +if ./one-time-script "$out" 2>/dev/null >"$modified" &&
      +   ! cmp -s "$out" "$modified" &&
      +   rm one-time-script 2>/dev/null
     @@ t/t5567-one-time-script.sh (new)
      +
      +HELPER="$TEST_DIRECTORY/lib-httpd/apply-one-time-script.sh"
      +
     -+test_expect_success PIPE 'concurrent requests: one rewritten, one passed through, neither empty' '
     ++test_expect_success PIPE 'helper only serves one rewritten response for concurrent requests' '
      +	mkdir workdir fakebin &&
      +	ENTERED="$PWD/entered" &&
      +	GATE="$PWD/gate" &&
      +	export ENTERED GATE &&
      +	mkfifo "$ENTERED" "$GATE" &&
      +
     -+	# 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.
     ++	# A stub git-http-backend that returns a response based on
     ++	# $ROLE. For $ROLE = modify, return the response string
     ++	# "packfile", which ends up being modified by the example
     ++	# one-time-script below.
     ++	#
     ++	# Otherwise, run the branch returning a response that
     ++	# should be passed through, and block until released
     ++	# by "read -r $GATE".
      +	write_script fakebin/git-http-backend <<-\EOF &&
      +	printf "Status: 200 OK\r\n"
      +	printf "Content-Type: application/x-git-result\r\n"
     @@ t/t5567-one-time-script.sh (new)
      +	fi
      +	EOF
      +
     -+	# The transform that replace_packfile would install as one-time-script:
     -+	# rewrite responses that contain "packfile", leave the rest alone.
     ++	# An example one-time-script for apply-one-time-script
     ++	# to execute. Checks for "packfile" in the response
     ++	# that will be returned, and replaces it with a
     ++	# modified response. Passes through responses without
     ++	# "packfile" in them.
      +	write_script workdir/one-time-script <<-\EOF &&
      +	if grep packfile "$1" >/dev/null
      +	then
     @@ t/t5567-one-time-script.sh (new)
      +	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).
     ++	# Ensure $GATE has a reader so the test does not block indefinitely if
     ++	# the helper is buggy and "echo released >&9" below does not unblock
     ++	# the unmodified response gate.
      +	exec 9<>"$GATE" &&
      +
     -+	# Launch the passthrough request in the background. 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.
     ++	# Launch the passthrough request in the background. Record its pid
     ++	# so it can be killed when the test finishes if, for some reason, the
     ++	# request stays blocked and would stall a test runner.
      +	{ (
      +		cd workdir &&
      +		ROLE=passthrough sh "$HELPER" >../passthrough.out 2>../passthrough.err
     @@ t/t5567-one-time-script.sh (new)
      +	passthrough_pid=$! &&
      +	test_when_finished "kill $passthrough_pid 2>/dev/null || :" &&
      +
     -+	# Wait until the passthrough request is past the marker check.
     ++	# Wait until the passthrough request is "in-flight" and paused
     ++	# mid-response.
      +	read -r entered <"$ENTERED" &&
      +
     -+	# Run the modifying request to completion while the passthrough request
     -+	# is still blocked.
     ++	# Launch the request for a modified response while the passthrough
     ++	# request is concurrently "in-flight" and paused.
      +	(
      +		cd workdir &&
      +		ROLE=modify sh "$HELPER" >../modify.out 2>../modify.err
      +	) &&
      +
     -+	# 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.
     ++	# Unblock the passthrough request, allowing git-http-backend to
     ++	# complete its response.
      +	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 &&
 2:  8ed22c02a1 ! 2:  79396d491f 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 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".
     +    http-429.sh is a helper for testing retry logic. It uses "test -f" to
     +    check for the existence of a state file and later uses "touch" or
     +    "rm -f" on that file to determine if it should return a 429. This method
     +    of managing state can fail if the helper script is invoked concurrently.
     +    However, this failure does not currently manifest itself since the
     +    helper is invoked sequentially.
      
     -    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.
     +    As a preventive measure, fix the state management logic so it relies on
     +    an atomic mkdir operation to mark that a 429 was returned. When
     +    $retry_after is "permanent", always return 429 now that we do not rely
     +    on a state file that is "touch"ed and "rm"ed to indicate when to respond
     +    with a 429.
      
          Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
      
     @@ 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
     -+# 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.
     ++# Use current directory (HTTPD_ROOT_PATH) to hold state directory
     ++# Create a safe directory name from test_context, retry_after and repo_name
     ++# This ensures all requests for the same test context share the same state directory
       safe_name=$(echo "${test_context}-${retry_after}-${repo_name}" | tr '/' '_' | tr -cd 'a-zA-Z0-9_-')
      -state_file="http-429-state-${safe_name}"
      +state="http-429-state-${safe_name}"
       
      -# Check if this is the first call (no state file exists)
      -if test -f "$state_file"
     -+# 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.
     ++# Check if this is the first call (no state directory exists), or if
     ++# the retry-after-value is "permanent", which indicates a 429 must be
     ++# returned for every request (even if the state directory exists).
      +if test "$retry_after" != permanent && ! mkdir "$state" 2>/dev/null
       then
       	# Already returned 429 once, forward to git-http-backend
 3:  374d148f43 ! 3:  d8d11ad246 t/lib-httpd: document writing concurrency-safe CGI helpers
     @@ Metadata
       ## Commit message ##
          t/lib-httpd: document writing concurrency-safe CGI helpers
      
     -    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).
     +    Update t/lib-httpd.sh to document the fixes applied to
     +    apply-one-time-script.sh and http-429.sh for future developers working
     +    on helper scripts. Add concrete examples of patterns and anti-patterns
     +    that should be considered when handling state management.
      
          Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
      
     @@ 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"
     -+	# 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.
     ++	# Apache can run the following scripts concurrently per request. Make
     ++	# sure any state management logic is resilient to race conditions.
     ++	#
     ++	# For example:
     ++	#   - use "mkdir dir" to ensure only one request "succeeds" under some
     ++	#     condition (see http-429.sh).
     ++	#   - chain (&&) atomic operations like "rm marker" (no -f) with the
     ++	#     logic that "claims" the marker instead of relying on a separate
     ++	#     "test -f" and "rm marker" check (see apply-one-time-script.sh).
     ++	#   - use scratch file names that include the process ID ($$), so
     ++	#     concurrent requests do not overwrite each other's state.
       	install_script incomplete-length-upload-pack-v2-http.sh
       	install_script incomplete-body-upload-pack-v2-http.sh
       	install_script error-no-report.sh

-- 
gitgitgadget