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

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

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