t/lib-httpd: make CGI test helpers concurrency-safe

[mmontalbo]

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

cc: Patrick Steinhardt ps@pks.im

[gitgitgadget]

There is an issue in commit f8d8372:
t/lib-httpd: add cgi-lib.sh for concurrency-safe CGI helpers

• Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
  Indented lines, and lines without whitespace, are exempt

[gitgitgadget]

There is an issue in commit e46f718:
t/lib-httpd: fix apply-one-time-script race using cgi-lib

• Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
  Indented lines, and lines without whitespace, are exempt

[gitgitgadget]

There is an issue in commit ac4d384:
t5567: test lib-httpd CGI helpers under concurrent requests

• Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
  Indented lines, and lines without whitespace, are exempt

[gitgitgadget]

There is an issue in commit 56503c0:
t/lib-httpd: add cgi-lib.sh for concurrency-safe CGI helpers

• Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
  Indented lines, and lines without whitespace, are exempt

[gitgitgadget]

There is an issue in commit 517865e:
t/lib-httpd: fix apply-one-time-script race using cgi-lib

• Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
  Indented lines, and lines without whitespace, are exempt

[gitgitgadget]

There is an issue in commit f4e5366:
t5567: test lib-httpd CGI helpers under concurrent requests

• Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
  Indented lines, and lines without whitespace, are exempt

[gitgitgadget]

There is an issue in commit b8239cb:
t/lib-httpd: add cgi-lib.sh for concurrency-safe CGI helpers

• Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
  Indented lines, and lines without whitespace, are exempt

[gitgitgadget]

There is an issue in commit 2953b76:
t/lib-httpd: fix apply-one-time-script race using cgi-lib

• Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
  Indented lines, and lines without whitespace, are exempt

[gitgitgadget]

There is an issue in commit 234a4c5:
t5567: test lib-httpd CGI helpers under concurrent requests

• Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
  Indented lines, and lines without whitespace, are exempt

[mmontalbo]

/preview

[gitgitgadget]

Preview email sent as pull.2171.git.1783479090.gitgitgadget@gmail.com

[mmontalbo]

/submit

[gitgitgadget]

Submitted as pull.2171.git.1783479584.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v1

To fetch this version to local tag pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v1

[gitgitgadget]

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Michael Montalbo <mmontalbo@gmail.com>
>
> apply-one-time-script.sh checks for the "one-time-script" marker, runs
> it, captures the git-http-backend response in the fixed-name files "out"
> and "out_modified", and removes the marker only after it has finished
> serving the modified response. Because the client receives the response
> body before that removal, it can start its next request while the marker
> still exists. Apache can then run this CGI for two requests 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 in
> flight. The second request passes the marker check, the first request
> then removes the marker, and the second fails to exec the now-missing
> marker, emits no output, and the server answers HTTP 500:
>
>   fatal: ... The requested URL returned error: 500
>   fatal: could not fetch <oid> from promisor remote
>
> This has been seen as a flaky failure of t5616.47 on the macOS CI
> runners.

Thanks for this detailed write-up.  The analysis looks good.

> Claim the marker atomically with a rename, and only once the one-time
> script has succeeded and actually changed the response; give the scratch
> files per-request names. A request that loses the rename, or whose
> script fails or leaves the response unchanged, serves the unmodified
> body and keeps the marker for a later request. No path emits an empty
> body, so the HTTP 500 no longer occurs.

Hmph.  

> +#
> +# Apache can run this CGI for concurrent requests (for example a partial fetch
> +# that lazily fetches a missing object while the first response is still in
> +# flight), so the helper claims the marker atomically with a rename, and only
> +# once it has decided to modify the response. A request that loses the race
> +# finds the marker already gone and serves its response unchanged; no request
> +# 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"
>  
> -	"$GIT_EXEC_PATH/git-http-backend" >out
> -	./one-time-script out >out_modified
> +LC_ALL=C
> +export LC_ALL

The original was somehow inconsistent in that it forced C locale
only when one-time-script munged the output, and otherwise the
backend was run in the original locale.  I am not sure if that
matters very much.

> +out=out.$$
> +modified=out-modified.$$
> +"$GIT_EXEC_PATH/git-http-backend" >"$out"
> +
> +if ./one-time-script "$out" 2>/dev/null >"$modified" &&
> +   ! cmp -s "$out" "$modified" &&
> +   mv one-time-script one-time-script.$$ 2>/dev/null
> +then
> +	cat "$modified"
>  else
> +	cat "$out"
>  fi

We may run the one-time script, find that it modified the payload,
and then another instance of us may start running before we can move
the one-time script away, so the second request can see "ah,
one-time-script is there, nobody has claimed it by renaming" and run
it again, no?  So this solution may shrink the race window but may
not completely eliminate it, unless we have some coordination among
ourselves, perhaps?

Ah, we assume running one-time-script itself multiple times is safe
and does not cause issues.  Our objective is to avoid returning
modified output twice.  So while the first instance of us
successfully renames one-time-script to one-time-script.$$ and emits
the modified result, even if the second instance raced and managed
to run the script again, it will fail to rename with "mv", and
discard the modified output, and instead show the unmodified output
generated by the backend.

OK.  It is a bit tricky.  It may help future readers if we said
something about this in the proposed log message (i.e., we consider
that it is perfectly fine to run one-time-script more than once; we
only want to avoid letting the second invocation's output used).

Thanks.

[gitgitgadget]

Michael Montalbo wrote on the Git mailing list (how to reply to this email):

On Wed, Jul 8, 2026 at 12:54 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> > +#
> > +# Apache can run this CGI for concurrent requests (for example a partial fetch
> > +# that lazily fetches a missing object while the first response is still in
> > +# flight), so the helper claims the marker atomically with a rename, and only
> > +# once it has decided to modify the response. A request that loses the race
> > +# finds the marker already gone and serves its response unchanged; no request
> > +# 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"
> >
> > -     "$GIT_EXEC_PATH/git-http-backend" >out
> > -     ./one-time-script out >out_modified
> > +LC_ALL=C
> > +export LC_ALL
>
> The original was somehow inconsistent in that it forced C locale
> only when one-time-script munged the output, and otherwise the
> backend was run in the original locale.  I am not sure if that
> matters very much.
>

I think it's still the same after the rewrite, though I could be
mistaken. If the
first `test -f` fails git-http-backend executes with inherited locale
(analogous to
the else branch execution in the original), and if `test -f` succeeds the locale
is forced to C and the one-time-script / git-http-backend run with the forced
locale. That being said, I think forcing the locale to C consistently would
make more sense. Depending on what you think, I can integrate that into the
series or leave for a future cleanup.

>
> Ah, we assume running one-time-script itself multiple times is safe
> and does not cause issues.  Our objective is to avoid returning
> modified output twice.  So while the first instance of us
> successfully renames one-time-script to one-time-script.$$ and emits
> the modified result, even if the second instance raced and managed
> to run the script again, it will fail to rename with "mv", and
> discard the modified output, and instead show the unmodified output
> generated by the backend.
>
> OK.  It is a bit tricky.  It may help future readers if we said
> something about this in the proposed log message (i.e., we consider
> that it is perfectly fine to run one-time-script more than once; we
> only want to avoid letting the second invocation's output used).
>

Yes that is a good call, I will add some detail about this subtlety in the
log message and helper comment.

[gitgitgadget]

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Michael Montalbo <mmontalbo@gmail.com>
>
> http-429.sh records "already returned 429 once" with a "test -f"
> followed by a "touch" of a shared state file. That check-then-act is not
> atomic: Apache can run this CGI for several requests at once, and two of
> them can both pass the "test -f" before either "touch"es, so both treat
> themselves as the first request. The retry flow that drives this
> endpoint is mostly sequential, so this has not been seen to fail, but
> the race is latent.

OK.  And use of mkdir for atomicity is an obvious solution for such
a situtation.

> -if test -f "$state_file"
> +if test "$retry_after" != permanent && ! mkdir "$state" 2>/dev/null
>  then
>  	# Already returned 429 once, forward to git-http-backend
>  	# Set PATH_INFO to just the repo path (without retry-after value)
> @@ -52,9 +55,6 @@ then
>  	exec "$GIT_EXEC_PATH/git-http-backend"
>  fi
>  
> -# Mark that we've returned 429
> -touch "$state_file"
> -

[gitgitgadget]

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Michael Montalbo <mmontalbo@gmail.com>
>
> The apply-one-time-script.sh and http-429.sh fixes addressed the same
> underlying problem: a test helper assuming it has exclusive access to a
> file when the web server can run it for several requests at once. The
> atomic idioms that avoid this are not specific to CGI or to HTTP, so
> document them generally, alongside the other guidance for writing tests,
> and leave a pointer from the lib-httpd helper list rather than a local
> comment. The note covers the anti-pattern (a "test -f" then a separate
> act) and the two safe operations (mkdir to elect a winner, rename to
> consume a one-shot marker), citing Git's own lockfile machinery and
> make_symlink() as precedent.
>
> Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
> ---
>  t/README       | 32 ++++++++++++++++++++++++++++++++
>  t/lib-httpd.sh |  3 +++
>  2 files changed, 35 insertions(+)

Thanks for a nice finishing touch.



> diff --git a/t/README b/t/README
> index 085921be4b..a9d425f392 100644
> --- a/t/README
> +++ b/t/README
> @@ -854,6 +854,38 @@ from the test harness library.  At the end of the script, call
>  'test_done'.
>  
>  
> +Writing concurrency-safe helpers
> +--------------------------------
> +
> +Some test code runs concurrently: a test may background work with '&',
> +and the helper scripts installed for the web server (in t/lib-httpd) are
> +run once per request, so the same script can execute for several
> +requests at once.  Such code cannot assume it has exclusive access to a
> +file.
> +
> +When exactly one of several concurrent processes needs to "win" a
> +decision, a single atomic filesystem operation can make it, rather than
> +a check followed by a separate action.  A "test -f X" then "touch X"
> +(or "rm X") races: two processes can both pass the check before either
> +acts.  Two atomic operations avoid this:
> +
> + - "mkdir dir", which fails if the directory already exists, so that
> +   exactly one caller wins, electing a first or only request (see
> +   t/lib-httpd/http-429.sh).
> +
> + - "mv src dst" (rename), which fails if the source is gone, so that
> +   exactly one caller consumes it, claiming a planted one-shot marker
> +   (see t/lib-httpd/apply-one-time-script.sh).
> +
> +A "$$" suffix on per-request scratch files keeps concurrent invocations
> +from clobbering each other's fixed-name files.
> +
> +This is a standard shell locking idiom, and the same reasoning behind
> +Git's own lockfile machinery, which creates its lock with O_CREAT|O_EXCL,
> +and make_symlink() in t/test-lib.sh, which uses an mkdir lock: an atomic
> +operation whose failure indicates that another process got there first.
> +
> +
>  Test harness library
>  --------------------
>  
> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> index fc646447d5..d64f9c8c2d 100644
> --- a/t/lib-httpd.sh
> +++ b/t/lib-httpd.sh
> @@ -159,6 +159,9 @@ prepare_httpd() {
>  	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
>  	cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
>  	cp "$TEST_PATH"/proxy-passwd "$HTTPD_ROOT_PATH"
> +	# The web server can run any of these CGI scripts for two requests at
> +	# once; a helper that keeps state between requests must do so with an
> +	# atomic operation. See "Writing concurrency-safe helpers" in t/README.
>  	install_script incomplete-length-upload-pack-v2-http.sh
>  	install_script incomplete-body-upload-pack-v2-http.sh
>  	install_script error-no-report.sh

[gitgitgadget]

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> -# 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.
> +if test "$retry_after" != permanent && ! mkdir "$state" 2>/dev/null

I think the last sentence in the above comment was meant to explain
why the new code checks the value of "$retry_after", but it is not
clear if it is needed for correctness (in other words, the original
was wrong to do "test -f && touch" but also was wrong to do so even
when "$retry_after" is set to "permanent), or if it is a mere
"optimization opportunity" you are taking advantage of.  In either
case, it would be nice to see it explained in the proposed commit
log message.

Thanks.

[gitgitgadget]

Michael Montalbo wrote on the Git mailing list (how to reply to this email):

On Wed, Jul 8, 2026 at 1:02 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > -# 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.
> > +if test "$retry_after" != permanent && ! mkdir "$state" 2>/dev/null
>
> I think the last sentence in the above comment was meant to explain
> why the new code checks the value of "$retry_after", but it is not
> clear if it is needed for correctness (in other words, the original
> was wrong to do "test -f && touch" but also was wrong to do so even
> when "$retry_after" is set to "permanent), or if it is a mere
> "optimization opportunity" you are taking advantage of.  In either
> case, it would be nice to see it explained in the proposed commit
> log message.
>

It is needed for correctness, and I agree it is not very clear from the log
message / comment. I will spell out the reasoning for the change more
clearly in both.

Thanks for taking a look at this!

[gitgitgadget]

This patch series was integrated into seen via git@4cbbabb.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch mm/lib-httpd-cgi-safe on the Git mailing list:

CGI helper scripts used by HTTP-related test scripts have been updated
to use atomic filesystem operations, preventing race conditions when
Apache handles concurrent requests.

Needs review.
source: <pull.2171.v2.git.1783704657.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch mm/lib-httpd-cgi-safe on the Git mailing list:

CGI helper scripts used by HTTP-related test scripts have been updated
to use atomic filesystem operations, preventing race conditions when
Apache handles concurrent requests.

Needs review.
source: <pull.2171.v2.git.1783704657.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch mm/lib-httpd-cgi-safe on the Git mailing list:

CGI helper scripts used by HTTP-related test scripts have been updated
to use atomic filesystem operations, preventing race conditions when
Apache handles concurrent requests.

Needs review.
source: <pull.2171.v2.git.1783704657.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch mm/lib-httpd-cgi-safe on the Git mailing list:

CGI helper scripts used by HTTP-related test scripts have been updated
to use atomic filesystem operations, preventing race conditions when
Apache handles concurrent requests.

Needs review.
source: <pull.2171.v2.git.1783704657.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch mm/lib-httpd-cgi-safe on the Git mailing list:

CGI helper scripts used by HTTP-related test scripts have been updated
to use atomic filesystem operations, preventing race conditions when
Apache handles concurrent requests.

Needs review.
source: <pull.2171.v2.git.1783704657.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch mm/lib-httpd-cgi-safe on the Git mailing list:

CGI helper scripts used by HTTP-related test scripts have been updated
to use atomic filesystem operations, preventing race conditions when
Apache handles concurrent requests.

Needs review.
source: <pull.2171.v2.git.1783704657.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch mm/lib-httpd-cgi-safe on the Git mailing list:

CGI helper scripts used by HTTP-related test scripts have been updated
to use atomic filesystem operations, preventing race conditions when
Apache handles concurrent requests.

Needs review.
source: <pull.2171.v2.git.1783704657.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch mm/lib-httpd-cgi-safe on the Git mailing list:

CGI helper scripts used by HTTP-related test scripts have been updated
to use atomic filesystem operations, preventing race conditions when
Apache handles concurrent requests.

Needs review.
source: <pull.2171.v2.git.1783704657.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch mm/lib-httpd-cgi-safe on the Git mailing list:

CGI helper scripts used by HTTP-related test scripts have been updated
to use atomic filesystem operations, preventing race conditions when
Apache handles concurrent requests.

Needs review.
cf. <pull.2171.v2.git.1783704657.gitgitgadget@gmail.com>
source: <pull.2171.v2.git.1783704657.gitgitgadget@gmail.com>

[gitgitgadget]

Michael Montalbo wrote on the Git mailing list (how to reply to this email):

Friendly ping. If it makes it any more enticing, I believe the flake fixed in
this series is responsible for at least a couple CI failures[1][2] since the
submission occurred.

[1] https://github.com/gitgitgadget/git/actions/runs/28983114431/job/86006743571
[2] https://github.com/git/git/actions/runs/29063352938/job/86269734698

[gitgitgadget]

There was a status update in the "Cooking" section about the branch mm/lib-httpd-cgi-safe on the Git mailing list:

CGI helper scripts used by HTTP-related test scripts have been updated
to use atomic filesystem operations, preventing race conditions when
Apache handles concurrent requests.

Needs review.
cf. <CAC2QwmLWkk4JS2XKLdj4i4CAtr7zZo=9tV_=pPQ77zR+R=pGUw@mail.gmail.com>
source: <pull.2171.v2.git.1783704657.gitgitgadget@gmail.com>

[gitgitgadget]

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

I was scanning the "What's cooking" report for topics marked as
"Needs review" to see if I could find ones that are relatively easy
to validate, and I hit this one.

The key change [1/3] is well thought out and nicely done.  [2/3] is
explained better than the corresponding step in v1, and [3/3] adds
helpful tips to the t/README documentation.  They all look quite
good.

Thanks.

[gitgitgadget]

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Fri, Jul 10, 2026 at 05:30:55PM +0000, Michael Montalbo via GitGitGadget wrote:
> diff --git a/t/lib-httpd/apply-one-time-script.sh b/t/lib-httpd/apply-one-time-script.sh
> index b1682944e2..adb9cec528 100644
> --- a/t/lib-httpd/apply-one-time-script.sh
> +++ b/t/lib-httpd/apply-one-time-script.sh
> @@ -6,21 +6,37 @@
>  #
>  # 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
> +#
> +# Apache can run this CGI for concurrent requests (for example a partial fetch
> +# that lazily fetches a missing object while the first response is still in
> +# flight), so the helper claims the marker atomically with a rename, and only
> +# once it has decided to modify the response. A request that loses the race
> +# finds the marker already gone and serves its response unchanged; no request
> +# 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.
> +#
> +# 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
> +test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend"
>  
> -	if cmp -s out out_modified
> -	then
> -		cat out
> -	else
> -		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"
> +
> +if ./one-time-script "$out" 2>/dev/null >"$modified" &&

Is it intentional that we swallow stderr of this script now? We didn't
before. I assume that this is to swallow the error in case the script
got removed by the concurrent request?

Patrick

[gitgitgadget]

Michael Montalbo wrote on the Git mailing list (how to reply to this email):

On Tue, Aug 4, 2026 at 1:03 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> > +
> > +out=out.$$
> > +modified=out-modified.$$
> > +"$GIT_EXEC_PATH/git-http-backend" >"$out"
> > +
> > +if ./one-time-script "$out" 2>/dev/null >"$modified" &&
>
> Is it intentional that we swallow stderr of this script now? We didn't
> before. I assume that this is to swallow the error in case the script
> got removed by the concurrent request?
>

Yes, you are correct on both counts. This is an intentional change
meant to swallow (an expected) stderr in case the script got removed
already by a concurrent request, but that is not clear on its own. I will
add an explanatory comment spelling this out.

[gitgitgadget]

User Patrick Steinhardt <ps@pks.im> has been added to the cc: list.

[gitgitgadget]

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Fri, Jul 10, 2026 at 05:30:57PM +0000, Michael Montalbo via GitGitGadget wrote:
> diff --git a/t/README b/t/README
> index 085921be4b..a9d425f392 100644
> --- a/t/README
> +++ b/t/README
> @@ -854,6 +854,38 @@ from the test harness library.  At the end of the script, call
>  'test_done'.
>  
>  
> +Writing concurrency-safe helpers
> +--------------------------------

Nit: this paragraph is quite specific to lib-httpd, so it would make
sense to mention it in the header here. E.g.

    Writing concurrency-safe lib-httpd helpers

> +Some test code runs concurrently: a test may background work with '&',
> +and the helper scripts installed for the web server (in t/lib-httpd) are
> +run once per request, so the same script can execute for several
> +requests at once.  Such code cannot assume it has exclusive access to a
> +file.
> +
> +When exactly one of several concurrent processes needs to "win" a
> +decision, a single atomic filesystem operation can make it, rather than
> +a check followed by a separate action.  A "test -f X" then "touch X"
> +(or "rm X") races: two processes can both pass the check before either
> +acts.  Two atomic operations avoid this:
> +
> + - "mkdir dir", which fails if the directory already exists, so that
> +   exactly one caller wins, electing a first or only request (see
> +   t/lib-httpd/http-429.sh).
> +
> + - "mv src dst" (rename), which fails if the source is gone, so that
> +   exactly one caller consumes it, claiming a planted one-shot marker
> +   (see t/lib-httpd/apply-one-time-script.sh).

A simple "rm" (without "-f") should work as well, right?

> +A "$$" suffix on per-request scratch files keeps concurrent invocations
> +from clobbering each other's fixed-name files.

Nit: it might be a bit easier to read if we explicitly mention PIDs
instead of assuming that every reader immediately knows that "$$" will
expand to the PID. E.g.:

    Appending a PID to the per-request scratch filenames keeps...

Thanks!

Patrick

[gitgitgadget]

Michael Montalbo wrote on the Git mailing list (how to reply to this email):

On Tue, Aug 4, 2026 at 1:03 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> >
> > +Writing concurrency-safe helpers
> > +--------------------------------
>
> Nit: this paragraph is quite specific to lib-httpd, so it would make
> sense to mention it in the header here. E.g.
>
>     Writing concurrency-safe lib-httpd helpers
>

Originally, I did just have this as a blurb in t/lib-httpd.sh. I ended up moving
it here and trying to make the advice apply more generally, though the only
other existing example I could find in another domain was the
make_symlink() reference. My intention was to make sure someone working
on a test helper with concurrency didn't skip over the section just because
they saw "http" and thought the advice didn't apply to their use case.

I'm inclined to make the language in the section more http-agnostic rather
than changing the title to be specific to http, but I do not feel very strongly
about it. If we were to frame this as http-specific advice maybe it should go
back to t/lib-httpd.sh instead of t/README?

>
> A simple "rm" (without "-f") should work as well, right?
>

Yes, definitely. I think I over-corrected in excising "rm" from the test helpers
and the advice given here since I associated it with the flawed patterns that
allowed for the race issues. I will redo the treatment of "rm" in the series
including reverting where "mv" replaced "rm" unnecessarily in the helpers.

> > +A "$$" suffix on per-request scratch files keeps concurrent invocations
> > +from clobbering each other's fixed-name files.
>
> Nit: it might be a bit easier to read if we explicitly mention PIDs
> instead of assuming that every reader immediately knows that "$$" will
> expand to the PID. E.g.:
>
>     Appending a PID to the per-request scratch filenames keeps...
>

Agreed, will fix.

> Thanks!
>

Thank you for taking a look and your feedback!

[gitgitgadget]

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Fri, Aug 07, 2026 at 09:51:34AM -0700, Michael Montalbo wrote:
> On Tue, Aug 4, 2026 at 1:03 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > >
> > > +Writing concurrency-safe helpers
> > > +--------------------------------
> >
> > Nit: this paragraph is quite specific to lib-httpd, so it would make
> > sense to mention it in the header here. E.g.
> >
> >     Writing concurrency-safe lib-httpd helpers
> >
> 
> Originally, I did just have this as a blurb in t/lib-httpd.sh. I ended up moving
> it here and trying to make the advice apply more generally, though the only
> other existing example I could find in another domain was the
> make_symlink() reference. My intention was to make sure someone working
> on a test helper with concurrency didn't skip over the section just because
> they saw "http" and thought the advice didn't apply to their use case.
> 
> I'm inclined to make the language in the section more http-agnostic rather
> than changing the title to be specific to http, but I do not feel very strongly
> about it. If we were to frame this as http-specific advice maybe it should go
> back to t/lib-httpd.sh instead of t/README?

Dunno. I'm not sure there's much value outside of httpd, so I'm still
inclined to make it httpd-specific. And if so, moving it into "t/" would
make sense.

But I don't feel overly strong about this, either, so I won't complain
if this section stays as-is.

Patrick

[gitgitgadget]

There was a status update in the "Cooking" section about the branch mm/lib-httpd-cgi-safe on the Git mailing list:

CGI helper scripts used by HTTP-related test scripts have been updated
to use atomic filesystem operations, preventing race conditions when
Apache handles concurrent requests.

Waiting for response.
cf. <anGcwAZgbarxi6_k@pks.im>
cf. <anGcx4lRyy3jyS1D@pks.im>
source: <pull.2171.v2.git.1783704657.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch mm/lib-httpd-cgi-safe on the Git mailing list:

CGI helper scripts used by HTTP-related test scripts have been updated
to use atomic filesystem operations, preventing race conditions when
Apache handles concurrent requests.

Expecting a reroll.
cf. <CAC2QwmK=K3EqvZWKQpy8ag+A8kMghNB6N=0dW7pjY1xJup4_Xg@mail.gmail.com>
cf. <CAC2Qwm+Jni+xU=gaef1AWCMj9+GUQhMrCWX9DFpS3y757pxv=Q@mail.gmail.com>
source: <pull.2171.v2.git.1783704657.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch mm/lib-httpd-cgi-safe on the Git mailing list:

CGI helper scripts used by HTTP-related test scripts have been updated
to use atomic filesystem operations, preventing race conditions when
Apache handles concurrent requests.

Expecting a reroll.
cf. <CAC2Qwm+Jni+xU=gaef1AWCMj9+GUQhMrCWX9DFpS3y757pxv=Q@mail.gmail.com>
source: <pull.2171.v2.git.1783704657.gitgitgadget@gmail.com>

[mmontalbo]

/preview

[gitgitgadget]

Preview email sent as pull.2171.v3.git.1786581335.gitgitgadget@gmail.com

[mmontalbo]

/submit

[gitgitgadget]

Submitted as pull.2171.v3.git.1786583137.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v3

To fetch this version to local tag pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v3:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v3

[gitgitgadget]

There was a status update in the "Cooking" section about the branch mm/lib-httpd-cgi-safe on the Git mailing list:

CGI helper scripts used by HTTP-related test scripts have been updated
to use atomic filesystem operations, preventing race conditions when
Apache handles concurrent requests.

Needs review.
(a newer iteration v3 exists as <pull.2171.v3.git.1786583137.gitgitgadget@gmail.com>)
source: <pull.2171.v2.git.1783704657.gitgitgadget@gmail.com>