From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 31 Aug 2026 07:18:11 GMT
Subject: Re: [PATCH v3 2/3] t/lib-httpd: make http-429 first-request check atomic
Message-ID: <apUqs8N3EnTFngyQ@pks.im>
In-Reply-To: <8ed22c02a192e10ab46c7df61e92a3669faaf25a.1786583137.git.gitgitgadget@gmail.com>

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