@matheusbsbs / matheus-bagliano / commits / ffb323e5b7

t/lib-httpd: make http-429 first-request check atomic

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. Decide whether this is the first request with a single atomic mkdir, which fails if the directory already exists, so exactly one of any concurrent requests is rate-limited and the rest are forwarded. Skipping state for "permanent" is required for correctness, not just an optimization. The marker tells a later or concurrent request that a 429 has already been served, so that it forwards to git-http-backend instead of rate-limiting. Since "permanent" must return 429 to every request, that marker must never become visible to another such request. The original did not achieve this by staying stateless: its "touch" of the marker ran unconditionally, and the "permanent" case removed it afterward with "rm -f". That create-then-remove leaves a window in which a concurrent "permanent" request sees the marker and is forwarded. It is the same class of check-then-act race this patch removes from the first-request check, latent for the same reason: the flow is mostly sequential. This version fuses the check and the mark into one atomic mkdir and, rather than recreate the pattern as mkdir-then-rmdir, skips the mkdir for "permanent" with a "!= permanent" guard. No marker is ever created, so there is no window and every "permanent" request rate-limits. There is no accompanying regression test. The check and the set are adjacent commands with no external step in between to synchronize on, so the overlap cannot be forced deterministically, only reproduced probabilistically; the fix is preventive. Signed-off-by: Michael Montalbo <mmontalbo@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Michael Montalbo committed Jul 10, 2026 at 17:30 UTC ffb323e5b76d50310b82d39f294de2aecd5681b8

1 file changed +17 -11

t/lib-httpd/http-429.sh

+17 -11

@@ -26,14 +26,24 @@ repo_path="${remaining#*/}" # Get rest (repo path)

26 # The repo name is the first component before any "/"

27 repo_name="${repo_path%%/*}"

29 -# Use current directory (HTTPD_ROOT_PATH) for state file

30 -# Create a safe filename from test_context, retry_after and repo_name

31 -# This ensures all requests for the same test context share the same state file

29 +# Use current directory (HTTPD_ROOT_PATH) for state.

30 +# Create a safe name from test_context, retry_after and repo_name so that all

31 +# requests for the same test context share the same state.

32 safe_name=$(echo "${test_context}-${retry_after}-${repo_name}" | tr '/' '_' | tr -cd 'a-zA-Z0-9_-')

33 -state_file="http-429-state-${safe_name}"

33 +state="http-429-state-${safe_name}"

35 -# Check if this is the first call (no state file exists)

36 -if test -f "$state_file"

35 +# This endpoint returns 429 to the first request and forwards later ones to

36 +# git-http-backend, so the retry succeeds. Apache can run this CGI for several

37 +# requests at once, so a single atomic "mkdir" elects that first request: the

38 +# one whose mkdir succeeds returns 429 and leaves the directory behind as the

39 +# "already rate-limited" marker; every later request finds the directory (mkdir

40 +# fails) and is forwarded.

41 +#

42 +# "permanent" is the exception: it must return 429 to every request and never

43 +# succeed, so it skips the mkdir and records no state. A leftover directory

44 +# would make its own later requests find the marker and be forwarded, which is

45 +# exactly what "permanent" must not do.

46 +if test "$retry_after" != permanent && ! mkdir "$state" 2>/dev/null

47 then

48 # Already returned 429 once, forward to git-http-backend

49 # Set PATH_INFO to just the repo path (without retry-after value)

@@ -52,9 +62,6 @@ then

62 exec "$GIT_EXEC_PATH/git-http-backend"

63 fi

55 -# Mark that we've returned 429

56 -touch "$state_file"

57 -

65 # Output HTTP 429 response

66 printf "Status: 429 Too Many Requests\r\n"

@@ -67,8 +74,7 @@ case "$retry_after" in

74 printf "Retry-After: invalid-format-123abc\r\n"

75 ;;

76 permanent)

70 - # Always return 429, don't set state file for success

71 - rm -f "$state_file"

77 + # Always return 429

78 printf "Retry-After: 1\r\n"

79 printf "Content-Type: text/plain\r\n"

80 printf "\r\n"