Skip to content

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

tagged this 13 Aug 01:05
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. For example, 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 fails. Two requests both pass
its "test -f one-time-script" check; one removes the marker; the other then
fails to exec it, emits an empty body, and the server answers 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, since 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 check-and-set. Its retry flow is mostly sequential, so it seems
less likely to fail, but it is the same latent race.

Each helper replaces a non-atomic "test -f" check and separate follow-up
action with a single atomic operation whose exit status decides the outcome:
apply-one-time-script.sh consumes its one-shot marker with "rm" (without
"-f"), and http-429.sh elects the first request with "mkdir".

 * 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 next to where t/lib-httpd.sh installs
   the CGI scripts, so the guidance is in front of anyone adding another
   helper.

Changes since v2:

 * Patch 1 now consumes the marker with a plain "rm" (without "-f") instead
   of a rename. "rm" without "-f" already fails once the marker is gone,
   which is the atomicity the helper needs. A new comment explains why the
   helper discards the one-time script's stderr: a losing request can find
   the marker already removed.

 * Patch 3 is now specific to the lib-httpd CGI helpers and lives beside
   their install site in t/lib-httpd.sh, rather than as a general section in
   t/README.

 * Reworded several helper comments and the patch 1 and 2 log messages for
   clarity and to match the code; no 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                       | 13 ++++
 t/lib-httpd/apply-one-time-script.sh | 50 +++++++++++----
 t/lib-httpd/http-429.sh              | 30 +++++----
 t/meson.build                        |  1 +
 t/t5567-one-time-script.sh           | 96 ++++++++++++++++++++++++++++
 5 files changed, 164 insertions(+), 26 deletions(-)
 create mode 100755 t/t5567-one-time-script.sh

base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc

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