Re: [PATCH 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 8, 2026, 19:54 UTC
Message-ID: <xmqqpl0xtfyz.fsf@gitster.g>
In-Reply-To: <9f48aa6d6ddea681b700f689f0509c4b30a7007d.1783479584.git.gitgitgadget@gmail.com>

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

Show 19 quoted lines

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

Show 6 quoted lines

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

Show 16 quoted lines

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

Show 12 quoted lines

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