Skip to content

pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v2

tagged this 10 Jul 17:30
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

Submitted-As: https://lore.kernel.org/git/pull.2171.v2.git.1783704657.gitgitgadget@gmail.com
In-Reply-To: https://lore.kernel.org/git/pull.2171.git.1783479584.gitgitgadget@gmail.com
Assets 2
Loading