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

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 v4:

 * Reword advice about chaining (&&) atomic operations like rm so it refers
   to chaining with "the logic guarded by the marker" instead of "the logic
   that claims the marker" since the latter is circular and inaccurate
   (atomic operations like rm are the logic that claims markers).

[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                       | 12 ++++
 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, 143 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-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/2171

Range-diff vs v4:

 1:  e202142f19 = 1:  e202142f19 t/lib-httpd: fix apply-one-time-script race under concurrent requests
 2:  79396d491f = 2:  79396d491f t/lib-httpd: make http-429 first-request check atomic
 3:  d8d11ad246 ! 3:  75a184ca09 t/lib-httpd: document writing concurrency-safe CGI helpers
     @@ t/lib-httpd.sh: prepare_httpd() {
      +	#   - 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).
     ++	#     logic that is guarded by 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

-- 
gitgitgadget