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

[mmontalbo]

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

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]

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>

[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.v3.git.1786583137.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.v3.git.1786583137.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.v3.git.1786583137.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.v3.git.1786583137.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.v3.git.1786583137.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:

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

After giving a cursory review to the previous round, I was hoping
that somebody more clueful than I am about HTTP tests would lend an
eye or two to these patches, but nobody seems interested.

Any takers?

Thanks.

[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.v3.git.1786583137.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.v3.git.1786583137.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.v3.git.1786583137.gitgitgadget@gmail.com>

[gitgitgadget]

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

On Thu, Aug 13, 2026 at 01:05:35AM +0000, Michael Montalbo via GitGitGadget wrote:
> From: Michael Montalbo <mmontalbo@gmail.com>
> 
> http-429.sh returns 429 to the first request for an endpoint and
> forwards later ones to git-http-backend so the retry succeeds. It
> remembers that it has already answered 429 by checking for a shared
> state file with "test -f" and creating it with "touch".
> 
> That "check-and-set" is not atomic. Apache runs the CGI for several
> requests at once, so two of them can pass the "test -f" before either
> "touch"es the file, and both then answer as the first request. The
> retry flow is mostly sequential, so this has not been observed to fail,
> but the race is latent. Replace the check and the "touch" with a single
> atomic "mkdir", which fails if the directory already exists, so exactly
> one of the concurrent requests is rate-limited and the rest are
> forwarded.
> 
> The "permanent" mode needs one extra step, for correctness rather than
> tidiness. The marker means "429 already served, now forward", so it must
> never be visible to a request that must itself return 429. Since
> "permanent" returns 429 to every request, it must leave no marker. The
> original did not manage this. It ran the "touch" unconditionally and
> removed the file with "rm -f" in the "permanent" case, and that
> "create-then-remove" has the same racy window: a concurrent "permanent"
> request can see the marker before the "rm -f" and be wrongly forwarded.
> Skipping the "mkdir" entirely for "permanent" (the "!= permanent" guard)
> leaves no marker at all, so every "permanent" request rate-limits.
> 
> There is no regression test. The check and the set are adjacent commands
> with nothing in between to synchronize on, so the overlap cannot be
> forced deterministically, only reproduced by chance; the fix is
> preventive.

A lot of AI-fluff in this message that could have otherwise been much
briefer, but okay.

> diff --git a/t/lib-httpd/http-429.sh b/t/lib-httpd/http-429.sh
> index c97b16145b..904cdacbd0 100644
> --- a/t/lib-httpd/http-429.sh
> +++ b/t/lib-httpd/http-429.sh
> @@ -26,14 +26,24 @@ repo_path="${remaining#*/}"  # Get rest (repo path)
>  # The repo name is the first component before any "/"
>  repo_name="${repo_path%%/*}"
>  
> -# Use current directory (HTTPD_ROOT_PATH) for state file
> -# Create a safe filename from test_context, retry_after and repo_name
> -# This ensures all requests for the same test context share the same state file
> +# Store state in the current directory (HTTPD_ROOT_PATH). Build a safe name
> +# from test_context, retry_after, and repo_name, so that all requests for one
> +# test context share the same state.
>  safe_name=$(echo "${test_context}-${retry_after}-${repo_name}" | tr '/' '_' | tr -cd 'a-zA-Z0-9_-')
> -state_file="http-429-state-${safe_name}"
> +state="http-429-state-${safe_name}"
>  
> -# Check if this is the first call (no state file exists)
> -if test -f "$state_file"
> +# This endpoint returns 429 to the first request. It forwards every later
> +# request to git-http-backend, so the retry succeeds. Apache can run this CGI
> +# for several requests at the same time. A single atomic "mkdir" selects the
> +# first request, because only one "mkdir" succeeds. That request returns 429
> +# and leaves the directory as the "already rate-limited" marker. Every later
> +# "mkdir" fails, so the endpoint forwards those requests.
> +#
> +# "permanent" is the exception. It must return 429 to every request, so it
> +# skips the "mkdir" and records no state. A leftover directory would let a
> +# later "permanent" request find the marker. The endpoint would forward that
> +# request, which "permanent" must not allow.
> +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 +62,6 @@ then
>  	exec "$GIT_EXEC_PATH/git-http-backend"
>  fi
>  
> -# Mark that we've returned 429
> -touch "$state_file"
> -
>  # Output HTTP 429 response
>  printf "Status: 429 Too Many Requests\r\n"
>  
> @@ -67,8 +74,7 @@ case "$retry_after" in
>  		printf "Retry-After: invalid-format-123abc\r\n"
>  		;;
>  	permanent)
> -		# Always return 429, don't set state file for success
> -		rm -f "$state_file"
> +		# Always return 429
>  		printf "Retry-After: 1\r\n"
>  		printf "Content-Type: text/plain\r\n"
>  		printf "\r\n"

The changes themselves look sensible.

Patrick

[gitgitgadget]

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

Patrick Steinhardt <ps@pks.im> writes:

> On Thu, Aug 13, 2026 at 01:05:35AM +0000, Michael Montalbo via GitGitGadget wrote:
>> From: Michael Montalbo <mmontalbo@gmail.com>
>> 
>> http-429.sh returns 429 to the first request for an endpoint and
>> forwards later ones to git-http-backend so the retry succeeds. It
>> remembers that it has already answered 429 by checking for a shared
>> state file with "test -f" and creating it with "touch".
>> 
>> That "check-and-set" is not atomic. Apache runs the CGI for several
>> requests at once, so two of them can pass the "test -f" before either
>> "touch"es the file, and both then answer as the first request. The
>> retry flow is mostly sequential, so this has not been observed to fail,
>> but the race is latent. Replace the check and the "touch" with a single
>> atomic "mkdir", which fails if the directory already exists, so exactly
>> one of the concurrent requests is rate-limited and the rest are
>> forwarded.
>> 
>> The "permanent" mode needs one extra step, for correctness rather than
>> tidiness. The marker means "429 already served, now forward", so it must
>> never be visible to a request that must itself return 429. Since
>> "permanent" returns 429 to every request, it must leave no marker. The
>> original did not manage this. It ran the "touch" unconditionally and
>> removed the file with "rm -f" in the "permanent" case, and that
>> "create-then-remove" has the same racy window: a concurrent "permanent"
>> request can see the marker before the "rm -f" and be wrongly forwarded.
>> Skipping the "mkdir" entirely for "permanent" (the "!= permanent" guard)
>> leaves no marker at all, so every "permanent" request rate-limits.
>> 
>> There is no regression test. The check and the set are adjacent commands
>> with nothing in between to synchronize on, so the overlap cannot be
>> forced deterministically, only reproduced by chance; the fix is
>> preventive.
>
> A lot of AI-fluff in this message that could have otherwise been much
> briefer, but okay.

I too find it disturbing it that the messages from this author tends
to contain material that triggers "it may not be wrong, but is it
relevant?" reactions.  More does not mean better.

The above made me curious enough to ask a near-by Gemini to distill
it down to quarter of the original length without losing essense of
the original.

    http-429.sh marks that a 429 response was served by creating a
    state file with "test -f" and "touch".  This check-and-set
    sequence is not atomic and can race under concurrent Apache
    requests, causing multiple requests to claim first-arrival
    status.

    Replace the check and "touch" with an atomic "mkdir", which
    fails if the directory already exists.  In "permanent" mode,
    skip the "mkdir" entirely so no state marker is ever created.

    Omit a regression test, as this concurrency window cannot be
    forced deterministically without artificial synchronization
    points.

This seems readable enough to me, but may still need some manual
clean-up, but this experiment told me that "A lot of AI-fluff" is
not something users cannot avoid without some extra work.

Thanks.

[gitgitgadget]

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

On Mon, Aug 31, 2026 at 12:18 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Thu, Aug 13, 2026 at 01:05:35AM +0000, Michael Montalbo via GitGitGadget wrote:
> > From: Michael Montalbo <mmontalbo@gmail.com>
> >
> > http-429.sh returns 429 to the first request for an endpoint and
> > forwards later ones to git-http-backend so the retry succeeds. It
> > remembers that it has already answered 429 by checking for a shared
> > state file with "test -f" and creating it with "touch".
...
> > There is no regression test. The check and the set are adjacent commands
> > with nothing in between to synchronize on, so the overlap cannot be
> > forced deterministically, only reproduced by chance; the fix is
> > preventive.
>
> A lot of AI-fluff in this message that could have otherwise been much
> briefer, but okay.
>

You are right. I will go through all the prose in the series and re-write it
by hand. I apologize for giving you unnecessary AI-fluff to read and will
not do it again.

[gitgitgadget]

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

On Mon, Aug 31, 2026 at 7:51 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> I too find it disturbing it that the messages from this author tends
> to contain material that triggers "it may not be wrong, but is it
> relevant?" reactions.  More does not mean better.
>

Thank you for this feedback. I agree with it and will avoid relying on AI
as I have to create and edit prose for documentation and cover
letters.

> The above made me curious enough to ask a near-by Gemini to distill
> it down to quarter of the original length without losing essense of
> the original.
>
>     http-429.sh marks that a 429 response was served by creating a
>     state file with "test -f" and "touch".  This check-and-set
>     sequence is not atomic and can race under concurrent Apache
>     requests, causing multiple requests to claim first-arrival
>     status.
>
>     Replace the check and "touch" with an atomic "mkdir", which
>     fails if the directory already exists.  In "permanent" mode,
>     skip the "mkdir" entirely so no state marker is ever created.
>
>     Omit a regression test, as this concurrency window cannot be
>     forced deterministically without artificial synchronization
>     points.
>
> This seems readable enough to me, but may still need some manual
> clean-up, but this experiment told me that "A lot of AI-fluff" is
> not something users cannot avoid without some extra work.
>

I agree, even though I have spent a lot of time trying to "copy-edit" what
is generated, the end result does tend to be verbose and include unnecessary
detail. Compared to what I start with based on my initial idea and generated
rough draft, a lot has been edited away. However, I do think I have regretfully
avoided doing some of that extra work. Apologies for having you all read
unnecessary AI-fluff, I will write prose for documentation and similar from
scratch.

[gitgitgadget]

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

On Thu, Aug 13, 2026 at 01:05:36AM +0000, Michael Montalbo via GitGitGadget wrote:
> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> index fc646447d5..f26e1594ab 100644
> --- a/t/lib-httpd.sh
> +++ b/t/lib-httpd.sh
> @@ -159,6 +159,19 @@ prepare_httpd() {
>  	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
>  	cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
>  	cp "$TEST_PATH"/proxy-passwd "$HTTPD_ROOT_PATH"
> +	# Apache runs each of these CGI scripts once per request. Apache can run one
> +	# script for several requests at the same time. A helper that keeps state
> +	# between requests must update that state with one atomic operation. A check
> +	# and then a separate action is not safe: two requests can both pass the
> +	# check before either one acts. Test the exit status of one atomic operation
> +	# instead:
> +	#   - "mkdir dir" fails if the directory exists, so only one request
> +	#     succeeds. http-429.sh selects the first request this way.
> +	#   - "rm marker" (without "-f") fails if the marker is gone, so only one
> +	#     request consumes it. apply-one-time-script.sh claims its one-shot
> +	#     marker this way.
> +	# A scratch file name includes the process ID ($$), so concurrent requests

Nit, not worth rerolling over: s/includes/should include/

Patrick

[gitgitgadget]

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

On Wed, Aug 26, 2026 at 12:59:14PM -0700, Junio C Hamano wrote:
> "Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> >  * 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.
> 
> After giving a cursory review to the previous round, I was hoping
> that somebody more clueful than I am about HTTP tests would lend an
> eye or two to these patches, but nobody seems interested.
> 
> Any takers?

I think this version is good enough. It's quite a bit puffed up by AI
generated messages that are overly long and use lots of meaningless
jargon, but I don't think that's worth another reroll.

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.

Expecting a reroll.
cf. <CAC2Qwm+L01XZgys2NGtZwWfVapWmnqDbsevt3Z4WKpS9EoP65A@mail.gmail.com>
source: <pull.2171.v3.git.1786583137.gitgitgadget@gmail.com>

[mmontalbo]

/preview

[gitgitgadget]

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

[mmontalbo]

/submit

[gitgitgadget]

Submitted as pull.2171.v4.git.1788222476.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-v4

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

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

[gitgitgadget]

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

On Tue, Sep 01, 2026 at 12:27:56AM +0000, Michael Montalbo via GitGitGadget wrote:
> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> index a216e5376f..8ca09fe85b 100644
> --- a/t/lib-httpd.sh
> +++ b/t/lib-httpd.sh
> @@ -159,6 +159,17 @@ prepare_httpd() {
>  	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
>  	cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
>  	cp "$TEST_PATH"/proxy-passwd "$HTTPD_ROOT_PATH"
> +	# Apache can run the following scripts concurrently per request. Make
> +	# sure any state management logic is resilient to race conditions.
> +	#
> +	# For example:
> +	#   - 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

Nit: I would have written "with the logic that is guarded by the marker"
instead of "claims".

> +	#     "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
>  	install_script incomplete-body-upload-pack-v2-http.sh
>  	install_script error-no-report.sh

Other than that the whole series reads a lot better now, thanks.

Patrick

[gitgitgadget]

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

On Tue, Sep 1, 2026 at 4:17 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Sep 01, 2026 at 12:27:56AM +0000, Michael Montalbo via GitGitGadget wrote:
> > diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> > index a216e5376f..8ca09fe85b 100644
> > --- a/t/lib-httpd.sh
> > +++ b/t/lib-httpd.sh
> > @@ -159,6 +159,17 @@ prepare_httpd() {
> >       mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
> >       cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
> >       cp "$TEST_PATH"/proxy-passwd "$HTTPD_ROOT_PATH"
> > +     # Apache can run the following scripts concurrently per request. Make
> > +     # sure any state management logic is resilient to race conditions.
> > +     #
> > +     # For example:
> > +     #   - 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
>
> Nit: I would have written "with the logic that is guarded by the marker"
> instead of "claims".
>

That makes more sense, the current version is  circular (rm is the logic doing
the claiming). Will fix.

> > +     #     "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
> >       install_script incomplete-body-upload-pack-v2-http.sh
> >       install_script error-no-report.sh
>
> Other than that the whole series reads a lot better now, thanks.
>

Thank you for the call out and taking another look. I really appreciate your
feedback!

[mmontalbo]

/submit

[gitgitgadget]

Submitted as pull.2171.v5.git.1788277983.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-v5

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

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

[gitgitgadget]

This patch series is no longer integrated into seen.