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

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

From: Michael Montalbo <mmontalbo@gmail.com>

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 can be seen in the flaky failure of t5616.47 on the macOS CI
runners.

Fix the logic that checks if "one-time-script" has returned its modified
response by chaining "rm one-time-script" with its execution. This
ensures a racing script does not also have the opportunity to execute
"one-time-script".

Add t/t5567-one-time-script.sh to verify the race is fixed. Implement a
stub "git-http-backend" that intentionally invokes a concurrent request,
and check that only one modified response is returned without error.

Signed-off-by: Michael Montalbo <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..eac21a3a8e 100644
--- a/t/lib-httpd/apply-one-time-script.sh
+++ b/t/lib-httpd/apply-one-time-script.sh
@@ -6,21 +6,31 @@
 #
 # This can be used to simulate the effects of the repository changing in
 # between HTTP request-response pairs.
-if test -f one-time-script
-then
-	LC_ALL=C
-	export LC_ALL
+test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend"
+
+LC_ALL=C
+export LC_ALL
 
-	"$GIT_EXEC_PATH/git-http-backend" >out
-	./one-time-script out >out_modified
+out=out.$$
+modified=out-modified.$$
+"$GIT_EXEC_PATH/git-http-backend" >"$out"
 
-	if cmp -s out out_modified
-	then
-		cat out
-	else
-		cat out_modified
-		rm one-time-script
-	fi
+# Since Apache can execute this script for multiple requests
+# concurrently, we chain "rm one-time-script" with the logic
+# for generating a modified response. If the "rm" ran separately,
+# a concurrent request could pass the "test -f" above and
+# erroneously result in multiple modified responses or an empty
+# body depending on the race state.
+#
+# We discard stderr for ./one-time-script since it is possible
+# ./one-time-script has been removed already, which is expected
+# sometimes. In this case, the unmodified response will be returned.
+if ./one-time-script "$out" 2>/dev/null >"$modified" &&
+   ! cmp -s "$out" "$modified" &&
+   rm one-time-script 2>/dev/null
+then
+	cat "$modified"
 else
-	"$GIT_EXEC_PATH/git-http-backend"
+	cat "$out"
 fi
+rm -f "$out" "$modified"
diff --git a/t/meson.build b/t/meson.build
index a25f37d2f5..e4d0b6dc4e 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -716,6 +716,7 @@ integration_tests = [
   't5564-http-proxy.sh',
   't5565-push-multiple.sh',
   't5566-push-group.sh',
+  't5567-one-time-script.sh',
   't5570-git-daemon.sh',
   't5571-pre-push-hook.sh',
   't5572-pull-submodule.sh',
diff --git a/t/t5567-one-time-script.sh b/t/t5567-one-time-script.sh
new file mode 100755
index 0000000000..a8429ef3c3
--- /dev/null
+++ b/t/t5567-one-time-script.sh
@@ -0,0 +1,96 @@
+#!/bin/sh
+
+test_description='apply-one-time-script CGI helper is safe under concurrent requests'
+
+. ./test-lib.sh
+
+HELPER="$TEST_DIRECTORY/lib-httpd/apply-one-time-script.sh"
+
+test_expect_success PIPE 'helper only serves one rewritten response for concurrent requests' '
+	mkdir workdir fakebin &&
+	ENTERED="$PWD/entered" &&
+	GATE="$PWD/gate" &&
+	export ENTERED GATE &&
+	mkfifo "$ENTERED" "$GATE" &&
+
+	# A stub git-http-backend that returns a response based on
+	# $ROLE. For $ROLE = modify, return the response string
+	# "packfile", which ends up being modified by the example
+	# one-time-script below.
+	#
+	# Otherwise, run the branch returning a response that
+	# should be passed through, and block until released
+	# by "read -r $GATE".
+	write_script fakebin/git-http-backend <<-\EOF &&
+	printf "Status: 200 OK\r\n"
+	printf "Content-Type: application/x-git-result\r\n"
+	printf "\r\n"
+	if test "$ROLE" = modify
+	then
+		printf "packfile\n"
+	else
+		echo entered >"$ENTERED"
+		read -r released <"$GATE"
+		printf "refs\n"
+	fi
+	EOF
+
+	# An example one-time-script for apply-one-time-script
+	# to execute. Checks for "packfile" in the response
+	# that will be returned, and replaces it with a
+	# modified response. Passes through responses without
+	# "packfile" in them.
+	write_script workdir/one-time-script <<-\EOF &&
+	if grep packfile "$1" >/dev/null
+	then
+		sed "/packfile/q" "$1" &&
+		printf "REPLACED\n"
+	else
+		cat "$1"
+	fi
+	EOF
+
+	GIT_EXEC_PATH="$PWD/fakebin" &&
+	export GIT_EXEC_PATH &&
+
+	# Ensure $GATE has a reader so the test does not block indefinitely if
+	# the helper is buggy and "echo released >&9" below does not unblock
+	# the unmodified response gate.
+	exec 9<>"$GATE" &&
+
+	# Launch the passthrough request in the background. Record its pid
+	# so it can be killed when the test finishes if, for some reason, the
+	# request stays blocked and would stall a test runner.
+	{ (
+		cd workdir &&
+		ROLE=passthrough sh "$HELPER" >../passthrough.out 2>../passthrough.err
+	) & } &&
+	passthrough_pid=$! &&
+	test_when_finished "kill $passthrough_pid 2>/dev/null || :" &&
+
+	# Wait until the passthrough request is "in-flight" and paused
+	# mid-response.
+	read -r entered <"$ENTERED" &&
+
+	# Launch the request for a modified response while the passthrough
+	# request is concurrently "in-flight" and paused.
+	(
+		cd workdir &&
+		ROLE=modify sh "$HELPER" >../modify.out 2>../modify.err
+	) &&
+
+	# Unblock the passthrough request, allowing git-http-backend to
+	# complete its response.
+	echo released >&9 &&
+	{ wait "$passthrough_pid" || :; } &&
+
+	test_must_be_empty passthrough.err &&
+	test_must_be_empty modify.err &&
+	test_grep "Status: 200 OK" passthrough.out &&
+	test_grep "Status: 200 OK" modify.out &&
+	test_grep REPLACED modify.out &&
+	test_grep ! REPLACED passthrough.out &&
+	test_grep refs passthrough.out
+'
+
+test_done
-- 
gitgitgadget