{"thread":{"id":"65950","subject":"[PATCH 0/3] t/lib-httpd: make CGI test helpers concurrency-safe","startedAt":"2026-07-08T02:59:46Z","lastAt":"2026-09-03T05:29:58Z","messageCount":43,"participants":["Michael Montalbo via GitGitGadget","Junio C Hamano","Michael Montalbo","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"547424","messageId":"pull.2171.git.1783479584.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":null,"subject":"[PATCH 0/3] t/lib-httpd: make CGI test helpers concurrency-safe","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-07-08T02:59:40Z","receivedAt":"2026-07-08T02:59:46Z","isPatch":true,"body":"The httpd tests share a handful of CGI helper scripts under t/lib-httpd. Two\nof them keep state between requests in the shared HTTPD_ROOT_PATH on the\nassumption that the web server hands them one request at a time. It does\nnot: Apache serves requests concurrently, and a single Git operation can\nopen more than one request to the same endpoint at once. A partial fetch\nthat receives a REF_DELTA against a missing promisor object lazily fetches\nthat base while the first response is still being served.\n\nUnder that overlap apply-one-time-script.sh loses: two requests both pass\nits \"test -f one-time-script\" check, one removes the marker, and the other\nfails to exec it and emits an empty body, which the server answers as HTTP\n500. In the field this is an occasional failure[1] of:\n\nt5616.47 tolerate server sending REF_DELTA against missing promisor objects\n\non the macOS CI runners, with:\n\nfatal: ... The requested URL returned error: 500 fatal: could not fetch from\npromisor remote\n\nI could not reproduce it against a live server (the window is tiny and\ntiming-dependent), but the macOS CI error log names the exact failure, and\nthe new test reproduces the helper's shell error.\n\nhttp-429.sh keeps its \"already returned 429 once\" state with the same\nnon-atomic test-and-set. Its retry flow is mostly sequential so it seems\nless likely to fail, but it is the same latent race.\n\nEach fix is local: claim/consume the one-shot marker with an atomic rename,\nand elect the first request with an atomic mkdir, rather than a \"test -f\"\nfollowed by a separate remove or touch.\n\n * Patch 1 fixes apply-one-time-script.sh (the actual flake) and adds t5567,\n which drives the helper directly with no web server so the overlap can be\n forced deterministically.\n * Patch 2 makes http-429.sh atomic.\n * Patch 3 documents the atomic idioms generally in t/README (they are not\n specific to CGI or HTTP), citing Git's own lockfile machinery and\n make_symlink(), with a pointer from the lib-httpd list.\n\n[1]\nhttps://github.com/gitgitgadget/git/actions/runs/28756172690/job/85263916762?pr=2169\n\nMichael Montalbo (3):\n t/lib-httpd: fix apply-one-time-script race under concurrent requests\n t/lib-httpd: make http-429 first-request check atomic\n t/README: document writing concurrency-safe helpers\n\n t/README | 32 ++++++++++\n t/lib-httpd.sh | 3 +\n t/lib-httpd/apply-one-time-script.sh | 38 +++++++----\n t/lib-httpd/http-429.sh | 21 +++---\n t/meson.build | 1 +\n t/t5567-one-time-script.sh | 96 ++++++++++++++++++++++++++++\n 6 files changed, 166 insertions(+), 25 deletions(-)\n create mode 100755 t/t5567-one-time-script.sh\n\n\nbase-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2171%2Fmmontalbo%2Fmm%2Flib-httpd-cgi-safe-proto-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2171\n-- \ngitgitgadget\n"},{"id":"547425","messageId":"9f48aa6d6ddea681b700f689f0509c4b30a7007d.1783479584.git.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.git.1783479584.gitgitgadget@gmail.com","subject":"[PATCH 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-07-08T02:59:41Z","receivedAt":"2026-07-08T02:59:48Z","isPatch":true,"body":"From: Michael Montalbo \n\napply-one-time-script.sh checks for the \"one-time-script\" marker, runs\nit, captures the git-http-backend response in the fixed-name files \"out\"\nand \"out_modified\", and removes the marker only after it has finished\nserving the modified response. Because the client receives the response\nbody before that removal, it can start its next request while the marker\nstill exists. Apache can then run this CGI for two requests at once: a\npartial fetch that receives a REF_DELTA against a missing promisor\nobject lazily fetches that base while the first response is still in\nflight. The second request passes the marker check, the first request\nthen removes the marker, and the second fails to exec the now-missing\nmarker, emits no output, and the server answers HTTP 500:\n\n fatal: ... The requested URL returned error: 500\n fatal: could not fetch from promisor remote\n\nThis has been seen as a flaky failure of t5616.47 on the macOS CI\nrunners.\n\nClaim the marker atomically with a rename, and only once the one-time\nscript has succeeded and actually changed the response; give the scratch\nfiles per-request names. A request that loses the rename, or whose\nscript fails or leaves the response unchanged, serves the unmodified\nbody and keeps the marker for a later request. No path emits an empty\nbody, so the HTTP 500 no longer occurs.\n\nAdd t5567 to lock this down. The overlap depends on timing, so a live\nhttpd test such as t5616.47 (the real code path) passes almost every\ntime even against the buggy helper; t5567 instead drives the helper\ndirectly with a fake git-http-backend and forces the overlap with FIFOs.\nAgainst the pre-fix helper it fails with the same shell error seen in\nthe field:\n\n ./one-time-script: No such file or directory\n\nSigned-off-by: Michael Montalbo \n---\n t/lib-httpd/apply-one-time-script.sh | 38 +++++++----\n t/meson.build | 1 +\n t/t5567-one-time-script.sh | 96 ++++++++++++++++++++++++++++\n 3 files changed, 121 insertions(+), 14 deletions(-)\n create mode 100755 t/t5567-one-time-script.sh\n\ndiff --git a/t/lib-httpd/apply-one-time-script.sh b/t/lib-httpd/apply-one-time-script.sh\nindex b1682944e2..a298ae89ae 100644\n--- a/t/lib-httpd/apply-one-time-script.sh\n+++ b/t/lib-httpd/apply-one-time-script.sh\n@@ -6,21 +6,31 @@\n #\n # This can be used to simulate the effects of the repository changing in\n # between HTTP request-response pairs.\n-if test -f one-time-script\n-then\n-\tLC_ALL=C\n-\texport LC_ALL\n+#\n+# Apache can run this CGI for concurrent requests (for example a partial fetch\n+# that lazily fetches a missing object while the first response is still in\n+# flight), so the helper claims the marker atomically with a rename, and only\n+# once it has decided to modify the response. A request that loses the race\n+# finds the marker already gone and serves its response unchanged; no request\n+# is left emitting an empty body, which the server would report as HTTP 500.\n+# Scratch files are per-request ($$) so concurrent requests do not clobber each\n+# other.\n+\n+test -f one-time-script || exec \"$GIT_EXEC_PATH/git-http-backend\"\n \n-\t\"$GIT_EXEC_PATH/git-http-backend\" >out\n-\t./one-time-script out >out_modified\n+LC_ALL=C\n+export LC_ALL\n \n-\tif cmp -s out out_modified\n-\tthen\n-\t\tcat out\n-\telse\n-\t\tcat out_modified\n-\t\trm one-time-script\n-\tfi\n+out=out.$$\n+modified=out-modified.$$\n+\"$GIT_EXEC_PATH/git-http-backend\" >\"$out\"\n+\n+if ./one-time-script \"$out\" 2>/dev/null >\"$modified\" &&\n+ ! cmp -s \"$out\" \"$modified\" &&\n+ mv one-time-script one-time-script.$$ 2>/dev/null\n+then\n+\tcat \"$modified\"\n else\n-\t\"$GIT_EXEC_PATH/git-http-backend\"\n+\tcat \"$out\"\n fi\n+rm -f \"$out\" \"$modified\" one-time-script.$$\ndiff --git a/t/meson.build b/t/meson.build\nindex 3219264fe7..a118a4d719 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -707,6 +707,7 @@ integration_tests = [\n 't5564-http-proxy.sh',\n 't5565-push-multiple.sh',\n 't5566-push-group.sh',\n+ 't5567-one-time-script.sh',\n 't5570-git-daemon.sh',\n 't5571-pre-push-hook.sh',\n 't5572-pull-submodule.sh',\ndiff --git a/t/t5567-one-time-script.sh b/t/t5567-one-time-script.sh\nnew file mode 100755\nindex 0000000000..cd8e656005\n--- /dev/null\n+++ b/t/t5567-one-time-script.sh\n@@ -0,0 +1,96 @@\n+#!/bin/sh\n+\n+test_description='apply-one-time-script CGI helper is safe under concurrent requests'\n+\n+. ./test-lib.sh\n+\n+HELPER=\"$TEST_DIRECTORY/lib-httpd/apply-one-time-script.sh\"\n+\n+test_expect_success PIPE 'concurrent requests: one rewritten, one passed through, neither empty' '\n+\tmkdir workdir fakebin &&\n+\tENTERED=\"$PWD/entered\" &&\n+\tGATE=\"$PWD/gate\" &&\n+\texport ENTERED GATE &&\n+\tmkfifo \"$ENTERED\" \"$GATE\" &&\n+\n+\t# Stand in for git-http-backend. The modify role returns a response\n+\t# containing \"packfile\", which the one-time script rewrites. The\n+\t# passthrough role returns a response that is left untouched, but first\n+\t# announces that it has entered the helper and then blocks, so that it\n+\t# is still in flight when the modify role claims and removes the marker.\n+\twrite_script fakebin/git-http-backend <<-\\EOF &&\n+\tprintf \"Status: 200 OK\\r\\n\"\n+\tprintf \"Content-Type: application/x-git-result\\r\\n\"\n+\tprintf \"\\r\\n\"\n+\tif test \"$ROLE\" = modify\n+\tthen\n+\t\tprintf \"packfile\\n\"\n+\telse\n+\t\techo entered >\"$ENTERED\"\n+\t\tread -r released <\"$GATE\"\n+\t\tprintf \"refs\\n\"\n+\tfi\n+\tEOF\n+\n+\t# The transform that replace_packfile would install as one-time-script:\n+\t# rewrite responses that contain \"packfile\", leave the rest alone.\n+\twrite_script workdir/one-time-script <<-\\EOF &&\n+\tif grep packfile \"$1\" >/dev/null\n+\tthen\n+\t\tsed \"/packfile/q\" \"$1\" &&\n+\t\tprintf \"REPLACED\\n\"\n+\telse\n+\t\tcat \"$1\"\n+\tfi\n+\tEOF\n+\n+\tGIT_EXEC_PATH=\"$PWD/fakebin\" &&\n+\texport GIT_EXEC_PATH &&\n+\n+\t# Hold GATE open read-write on fd 9 for the duration, so releasing the\n+\t# passthrough request below cannot block even if that request has\n+\t# already exited (it keeps a reader on the FIFO).\n+\texec 9<>\"$GATE\" &&\n+\n+\t# Launch the passthrough request in the background. It enters the\n+\t# helper, signals us through ENTERED, then blocks on GATE inside the\n+\t# fake backend. The braces keep the && chain intact while backgrounding\n+\t# only the subshell, so \"wait\" can reap it by pid; kill it on any exit\n+\t# so a stray blocked child cannot hold the test output open and stall a\n+\t# reader such as prove.\n+\t{ (\n+\t\tcd workdir &&\n+\t\tROLE=passthrough sh \"$HELPER\" >../passthrough.out 2>../passthrough.err\n+\t) & } &&\n+\tpassthrough_pid=$! &&\n+\ttest_when_finished \"kill $passthrough_pid 2>/dev/null || :\" &&\n+\n+\t# Wait until the passthrough request is past the marker check.\n+\tread -r entered <\"$ENTERED\" &&\n+\n+\t# Run the modifying request to completion while the passthrough request\n+\t# is still blocked.\n+\t(\n+\t\tcd workdir &&\n+\t\tROLE=modify sh \"$HELPER\" >../modify.out 2>../modify.err\n+\t) &&\n+\n+\t# Release the passthrough request and let it finish. Ignore the helper\n+\t# exit status here so a broken helper is diagnosed by the assertions\n+\t# below rather than aborting the test.\n+\techo released >&9 &&\n+\t{ wait \"$passthrough_pid\" || :; } &&\n+\n+\t# Neither request may error out or produce an empty (HTTP 500) body,\n+\t# and each must have played its role: the modify request rewrote its\n+\t# response and the passthrough request came through untouched.\n+\ttest_must_be_empty passthrough.err &&\n+\ttest_must_be_empty modify.err &&\n+\ttest_grep \"Status: 200 OK\" passthrough.out &&\n+\ttest_grep \"Status: 200 OK\" modify.out &&\n+\ttest_grep REPLACED modify.out &&\n+\ttest_grep ! REPLACED passthrough.out &&\n+\ttest_grep refs passthrough.out\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"547426","messageId":"efd34c17157b3183cdc851c8b17e7967b6c85506.1783479584.git.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.git.1783479584.gitgitgadget@gmail.com","subject":"[PATCH 2/3] t/lib-httpd: make http-429 first-request check atomic","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-07-08T02:59:42Z","receivedAt":"2026-07-08T02:59:49Z","isPatch":true,"body":"From: Michael Montalbo \n\nhttp-429.sh records \"already returned 429 once\" with a \"test -f\"\nfollowed by a \"touch\" of a shared state file. That check-then-act is not\natomic: Apache can run this CGI for several requests at once, and two of\nthem can both pass the \"test -f\" before either \"touch\"es, so both treat\nthemselves as the first request. The retry flow that drives this\nendpoint is mostly sequential, so this has not been seen to fail, but\nthe race is latent.\n\nDecide whether this is the first request with a single atomic mkdir,\nwhich fails if the directory already exists, so exactly one of any\nconcurrent requests is rate-limited and the rest are forwarded.\n\nThere is no accompanying regression test. The check and the set are\nadjacent commands with no external step in between to synchronize on, so\nthe overlap cannot be forced deterministically, only reproduced\nprobabilistically; the fix is preventive.\n\nSigned-off-by: Michael Montalbo \n---\n t/lib-httpd/http-429.sh | 21 ++++++++++-----------\n 1 file changed, 10 insertions(+), 11 deletions(-)\n\ndiff --git a/t/lib-httpd/http-429.sh b/t/lib-httpd/http-429.sh\nindex c97b16145b..d9bbedf1ad 100644\n--- a/t/lib-httpd/http-429.sh\n+++ b/t/lib-httpd/http-429.sh\n@@ -26,14 +26,17 @@ repo_path=\"${remaining#*/}\" # Get rest (repo path)\n # The repo name is the first component before any \"/\"\n repo_name=\"${repo_path%%/*}\"\n \n-# Use current directory (HTTPD_ROOT_PATH) for state file\n-# Create a safe filename from test_context, retry_after and repo_name\n-# This ensures all requests for the same test context share the same state file\n+# Use current directory (HTTPD_ROOT_PATH) for state.\n+# Create a safe name from test_context, retry_after and repo_name so that all\n+# requests for the same test context share the same state.\n safe_name=$(echo \"${test_context}-${retry_after}-${repo_name}\" | tr '/' '_' | tr -cd 'a-zA-Z0-9_-')\n-state_file=\"http-429-state-${safe_name}\"\n+state=\"http-429-state-${safe_name}\"\n \n-# Check if this is the first call (no state file exists)\n-if test -f \"$state_file\"\n+# Apache can run this CGI for concurrent requests, so the script decides\n+# whether this is the first call with a single atomic \"mkdir\": it succeeds for\n+# exactly one of any racing requests and fails for the rest. \"permanent\"\n+# always rate-limits and records no state.\n+if test \"$retry_after\" != permanent && ! mkdir \"$state\" 2>/dev/null\n then\n \t# Already returned 429 once, forward to git-http-backend\n \t# Set PATH_INFO to just the repo path (without retry-after value)\n@@ -52,9 +55,6 @@ then\n \texec \"$GIT_EXEC_PATH/git-http-backend\"\n fi\n \n-# Mark that we've returned 429\n-touch \"$state_file\"\n-\n # Output HTTP 429 response\n printf \"Status: 429 Too Many Requests\\r\\n\"\n \n@@ -67,8 +67,7 @@ case \"$retry_after\" in\n \t\tprintf \"Retry-After: invalid-format-123abc\\r\\n\"\n \t\t;;\n \tpermanent)\n-\t\t# Always return 429, don't set state file for success\n-\t\trm -f \"$state_file\"\n+\t\t# Always return 429\n \t\tprintf \"Retry-After: 1\\r\\n\"\n \t\tprintf \"Content-Type: text/plain\\r\\n\"\n \t\tprintf \"\\r\\n\"\n-- \ngitgitgadget\n\n"},{"id":"547427","messageId":"771d264d2999a780e0c93e64bb4451a05214ab75.1783479584.git.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.git.1783479584.gitgitgadget@gmail.com","subject":"[PATCH 3/3] t/README: document writing concurrency-safe helpers","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-07-08T02:59:43Z","receivedAt":"2026-07-08T02:59:50Z","isPatch":true,"body":"From: Michael Montalbo \n\nThe apply-one-time-script.sh and http-429.sh fixes addressed the same\nunderlying problem: a test helper assuming it has exclusive access to a\nfile when the web server can run it for several requests at once. The\natomic idioms that avoid this are not specific to CGI or to HTTP, so\ndocument them generally, alongside the other guidance for writing tests,\nand leave a pointer from the lib-httpd helper list rather than a local\ncomment. The note covers the anti-pattern (a \"test -f\" then a separate\nact) and the two safe operations (mkdir to elect a winner, rename to\nconsume a one-shot marker), citing Git's own lockfile machinery and\nmake_symlink() as precedent.\n\nSigned-off-by: Michael Montalbo \n---\n t/README | 32 ++++++++++++++++++++++++++++++++\n t/lib-httpd.sh | 3 +++\n 2 files changed, 35 insertions(+)\n\ndiff --git a/t/README b/t/README\nindex 085921be4b..a9d425f392 100644\n--- a/t/README\n+++ b/t/README\n@@ -854,6 +854,38 @@ from the test harness library. At the end of the script, call\n 'test_done'.\n \n \n+Writing concurrency-safe helpers\n+--------------------------------\n+\n+Some test code runs concurrently: a test may background work with '&',\n+and the helper scripts installed for the web server (in t/lib-httpd) are\n+run once per request, so the same script can execute for several\n+requests at once. Such code cannot assume it has exclusive access to a\n+file.\n+\n+When exactly one of several concurrent processes needs to \"win\" a\n+decision, a single atomic filesystem operation can make it, rather than\n+a check followed by a separate action. A \"test -f X\" then \"touch X\"\n+(or \"rm X\") races: two processes can both pass the check before either\n+acts. Two atomic operations avoid this:\n+\n+ - \"mkdir dir\", which fails if the directory already exists, so that\n+ exactly one caller wins, electing a first or only request (see\n+ t/lib-httpd/http-429.sh).\n+\n+ - \"mv src dst\" (rename), which fails if the source is gone, so that\n+ exactly one caller consumes it, claiming a planted one-shot marker\n+ (see t/lib-httpd/apply-one-time-script.sh).\n+\n+A \"$$\" suffix on per-request scratch files keeps concurrent invocations\n+from clobbering each other's fixed-name files.\n+\n+This is a standard shell locking idiom, and the same reasoning behind\n+Git's own lockfile machinery, which creates its lock with O_CREAT|O_EXCL,\n+and make_symlink() in t/test-lib.sh, which uses an mkdir lock: an atomic\n+operation whose failure indicates that another process got there first.\n+\n+\n Test harness library\n --------------------\n \ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex fc646447d5..d64f9c8c2d 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -159,6 +159,9 @@ prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/proxy-passwd \"$HTTPD_ROOT_PATH\"\n+\t# The web server can run any of these CGI scripts for two requests at\n+\t# once; a helper that keeps state between requests must do so with an\n+\t# atomic operation. See \"Writing concurrency-safe helpers\" in t/README.\n \tinstall_script incomplete-length-upload-pack-v2-http.sh\n \tinstall_script incomplete-body-upload-pack-v2-http.sh\n \tinstall_script error-no-report.sh\n-- \ngitgitgadget\n"},{"id":"547527","messageId":"xmqqpl0xtfyz.fsf@gitster.g","threadId":"65950","inReplyTo":"9f48aa6d6ddea681b700f689f0509c4b30a7007d.1783479584.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-08T19:54:12Z","receivedAt":"2026-07-08T19:54:15Z","isPatch":true,"body":"\"Michael Montalbo via GitGitGadget\" writes:\n\n> From: Michael Montalbo \n>\n> apply-one-time-script.sh checks for the \"one-time-script\" marker, runs\n> it, captures the git-http-backend response in the fixed-name files \"out\"\n> and \"out_modified\", and removes the marker only after it has finished\n> serving the modified response. Because the client receives the response\n> body before that removal, it can start its next request while the marker\n> still exists. Apache can then run this CGI for two requests at once: a\n> partial fetch that receives a REF_DELTA against a missing promisor\n> object lazily fetches that base while the first response is still in\n> flight. The second request passes the marker check, the first request\n> then removes the marker, and the second fails to exec the now-missing\n> marker, emits no output, and the server answers HTTP 500:\n>\n> fatal: ... The requested URL returned error: 500\n> fatal: could not fetch from promisor remote\n>\n> This has been seen as a flaky failure of t5616.47 on the macOS CI\n> runners.\n\nThanks for this detailed write-up. The analysis looks good.\n\n> Claim the marker atomically with a rename, and only once the one-time\n> script has succeeded and actually changed the response; give the scratch\n> files per-request names. A request that loses the rename, or whose\n> script fails or leaves the response unchanged, serves the unmodified\n> body and keeps the marker for a later request. No path emits an empty\n> body, so the HTTP 500 no longer occurs.\n\nHmph. \n\n> +#\n> +# Apache can run this CGI for concurrent requests (for example a partial fetch\n> +# that lazily fetches a missing object while the first response is still in\n> +# flight), so the helper claims the marker atomically with a rename, and only\n> +# once it has decided to modify the response. A request that loses the race\n> +# finds the marker already gone and serves its response unchanged; no request\n> +# is left emitting an empty body, which the server would report as HTTP 500.\n> +# Scratch files are per-request ($$) so concurrent requests do not clobber each\n> +# other.\n> +\n> +test -f one-time-script || exec \"$GIT_EXEC_PATH/git-http-backend\"\n> \n> -\t\"$GIT_EXEC_PATH/git-http-backend\" >out\n> -\t./one-time-script out >out_modified\n> +LC_ALL=C\n> +export LC_ALL\n\nThe original was somehow inconsistent in that it forced C locale\nonly when one-time-script munged the output, and otherwise the\nbackend was run in the original locale. I am not sure if that\nmatters very much.\n\n> +out=out.$$\n> +modified=out-modified.$$\n> +\"$GIT_EXEC_PATH/git-http-backend\" >\"$out\"\n> +\n> +if ./one-time-script \"$out\" 2>/dev/null >\"$modified\" &&\n> + ! cmp -s \"$out\" \"$modified\" &&\n> + mv one-time-script one-time-script.$$ 2>/dev/null\n> +then\n> +\tcat \"$modified\"\n> else\n> +\tcat \"$out\"\n> fi\n\nWe may run the one-time script, find that it modified the payload,\nand then another instance of us may start running before we can move\nthe one-time script away, so the second request can see \"ah,\none-time-script is there, nobody has claimed it by renaming\" and run\nit again, no? So this solution may shrink the race window but may\nnot completely eliminate it, unless we have some coordination among\nourselves, perhaps?\n\nAh, we assume running one-time-script itself multiple times is safe\nand does not cause issues. Our objective is to avoid returning\nmodified output twice. So while the first instance of us\nsuccessfully renames one-time-script to one-time-script.$$ and emits\nthe modified result, even if the second instance raced and managed\nto run the script again, it will fail to rename with \"mv\", and\ndiscard the modified output, and instead show the unmodified output\ngenerated by the backend.\n\nOK. It is a bit tricky. It may help future readers if we said\nsomething about this in the proposed log message (i.e., we consider\nthat it is perfectly fine to run one-time-script more than once; we\nonly want to avoid letting the second invocation's output used).\n\nThanks.\n"},{"id":"547528","messageId":"xmqqldbltfrc.fsf@gitster.g","threadId":"65950","inReplyTo":"efd34c17157b3183cdc851c8b17e7967b6c85506.1783479584.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] t/lib-httpd: make http-429 first-request check atomic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-08T19:58:47Z","receivedAt":"2026-07-08T19:58:50Z","isPatch":true,"body":"\"Michael Montalbo via GitGitGadget\" writes:\n\n> From: Michael Montalbo \n>\n> http-429.sh records \"already returned 429 once\" with a \"test -f\"\n> followed by a \"touch\" of a shared state file. That check-then-act is not\n> atomic: Apache can run this CGI for several requests at once, and two of\n> them can both pass the \"test -f\" before either \"touch\"es, so both treat\n> themselves as the first request. The retry flow that drives this\n> endpoint is mostly sequential, so this has not been seen to fail, but\n> the race is latent.\n\nOK. And use of mkdir for atomicity is an obvious solution for such\na situtation.\n\n> -if test -f \"$state_file\"\n> +if test \"$retry_after\" != permanent && ! mkdir \"$state\" 2>/dev/null\n> then\n> \t# Already returned 429 once, forward to git-http-backend\n> \t# Set PATH_INFO to just the repo path (without retry-after value)\n> @@ -52,9 +55,6 @@ then\n> \texec \"$GIT_EXEC_PATH/git-http-backend\"\n> fi\n> \n> -# Mark that we've returned 429\n> -touch \"$state_file\"\n> -\n"},{"id":"547529","messageId":"xmqqh5m9tfpl.fsf@gitster.g","threadId":"65950","inReplyTo":"771d264d2999a780e0c93e64bb4451a05214ab75.1783479584.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] t/README: document writing concurrency-safe helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-08T19:59:50Z","receivedAt":"2026-07-08T19:59:53Z","isPatch":true,"body":"\"Michael Montalbo via GitGitGadget\" writes:\n\n> From: Michael Montalbo \n>\n> The apply-one-time-script.sh and http-429.sh fixes addressed the same\n> underlying problem: a test helper assuming it has exclusive access to a\n> file when the web server can run it for several requests at once. The\n> atomic idioms that avoid this are not specific to CGI or to HTTP, so\n> document them generally, alongside the other guidance for writing tests,\n> and leave a pointer from the lib-httpd helper list rather than a local\n> comment. The note covers the anti-pattern (a \"test -f\" then a separate\n> act) and the two safe operations (mkdir to elect a winner, rename to\n> consume a one-shot marker), citing Git's own lockfile machinery and\n> make_symlink() as precedent.\n>\n> Signed-off-by: Michael Montalbo \n> ---\n> t/README | 32 ++++++++++++++++++++++++++++++++\n> t/lib-httpd.sh | 3 +++\n> 2 files changed, 35 insertions(+)\n\nThanks for a nice finishing touch.\n\n\n\n> diff --git a/t/README b/t/README\n> index 085921be4b..a9d425f392 100644\n> --- a/t/README\n> +++ b/t/README\n> @@ -854,6 +854,38 @@ from the test harness library. At the end of the script, call\n> 'test_done'.\n> \n> \n> +Writing concurrency-safe helpers\n> +--------------------------------\n> +\n> +Some test code runs concurrently: a test may background work with '&',\n> +and the helper scripts installed for the web server (in t/lib-httpd) are\n> +run once per request, so the same script can execute for several\n> +requests at once. Such code cannot assume it has exclusive access to a\n> +file.\n> +\n> +When exactly one of several concurrent processes needs to \"win\" a\n> +decision, a single atomic filesystem operation can make it, rather than\n> +a check followed by a separate action. A \"test -f X\" then \"touch X\"\n> +(or \"rm X\") races: two processes can both pass the check before either\n> +acts. Two atomic operations avoid this:\n> +\n> + - \"mkdir dir\", which fails if the directory already exists, so that\n> + exactly one caller wins, electing a first or only request (see\n> + t/lib-httpd/http-429.sh).\n> +\n> + - \"mv src dst\" (rename), which fails if the source is gone, so that\n> + exactly one caller consumes it, claiming a planted one-shot marker\n> + (see t/lib-httpd/apply-one-time-script.sh).\n> +\n> +A \"$$\" suffix on per-request scratch files keeps concurrent invocations\n> +from clobbering each other's fixed-name files.\n> +\n> +This is a standard shell locking idiom, and the same reasoning behind\n> +Git's own lockfile machinery, which creates its lock with O_CREAT|O_EXCL,\n> +and make_symlink() in t/test-lib.sh, which uses an mkdir lock: an atomic\n> +operation whose failure indicates that another process got there first.\n> +\n> +\n> Test harness library\n> --------------------\n> \n> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\n> index fc646447d5..d64f9c8c2d 100644\n> --- a/t/lib-httpd.sh\n> +++ b/t/lib-httpd.sh\n> @@ -159,6 +159,9 @@ prepare_httpd() {\n> \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n> \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n> \tcp \"$TEST_PATH\"/proxy-passwd \"$HTTPD_ROOT_PATH\"\n> +\t# The web server can run any of these CGI scripts for two requests at\n> +\t# once; a helper that keeps state between requests must do so with an\n> +\t# atomic operation. See \"Writing concurrency-safe helpers\" in t/README.\n> \tinstall_script incomplete-length-upload-pack-v2-http.sh\n> \tinstall_script incomplete-body-upload-pack-v2-http.sh\n> \tinstall_script error-no-report.sh\n"},{"id":"547530","messageId":"xmqqcxwxtfkp.fsf@gitster.g","threadId":"65950","inReplyTo":"efd34c17157b3183cdc851c8b17e7967b6c85506.1783479584.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] t/lib-httpd: make http-429 first-request check atomic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-08T20:02:46Z","receivedAt":"2026-07-08T20:02:49Z","isPatch":true,"body":"\"Michael Montalbo via GitGitGadget\" writes:\n\n> -# Check if this is the first call (no state file exists)\n> -if test -f \"$state_file\"\n> +# Apache can run this CGI for concurrent requests, so the script decides\n> +# whether this is the first call with a single atomic \"mkdir\": it succeeds for\n> +# exactly one of any racing requests and fails for the rest. \"permanent\"\n> +# always rate-limits and records no state.\n> +if test \"$retry_after\" != permanent && ! mkdir \"$state\" 2>/dev/null\n\nI think the last sentence in the above comment was meant to explain\nwhy the new code checks the value of \"$retry_after\", but it is not\nclear if it is needed for correctness (in other words, the original\nwas wrong to do \"test -f && touch\" but also was wrong to do so even\nwhen \"$retry_after\" is set to \"permanent), or if it is a mere\n\"optimization opportunity\" you are taking advantage of. In either\ncase, it would be nice to see it explained in the proposed commit\nlog message.\n\nThanks.\n"},{"id":"547641","messageId":"CAC2QwmKeu5edJ=d_sT5BpT4q_=ch8HhUJaLuT9DkKWB22hhzjg@mail.gmail.com","threadId":"65950","inReplyTo":"xmqqpl0xtfyz.fsf@gitster.g","subject":"Re: [PATCH 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests","fromName":"Michael Montalbo","fromEmail":"mmontalbo@gmail.com","sentAt":"2026-07-09T17:26:59Z","receivedAt":"2026-07-09T17:27:13Z","isPatch":true,"body":"On Wed, Jul 8, 2026 at 12:54 PM Junio C Hamano wrote:\n>\n> \"Michael Montalbo via GitGitGadget\" writes:\n> >\n> > +#\n> > +# Apache can run this CGI for concurrent requests (for example a partial fetch\n> > +# that lazily fetches a missing object while the first response is still in\n> > +# flight), so the helper claims the marker atomically with a rename, and only\n> > +# once it has decided to modify the response. A request that loses the race\n> > +# finds the marker already gone and serves its response unchanged; no request\n> > +# is left emitting an empty body, which the server would report as HTTP 500.\n> > +# Scratch files are per-request ($$) so concurrent requests do not clobber each\n> > +# other.\n> > +\n> > +test -f one-time-script || exec \"$GIT_EXEC_PATH/git-http-backend\"\n> >\n> > - \"$GIT_EXEC_PATH/git-http-backend\" >out\n> > - ./one-time-script out >out_modified\n> > +LC_ALL=C\n> > +export LC_ALL\n>\n> The original was somehow inconsistent in that it forced C locale\n> only when one-time-script munged the output, and otherwise the\n> backend was run in the original locale. I am not sure if that\n> matters very much.\n>\n\nI think it's still the same after the rewrite, though I could be\nmistaken. If the\nfirst `test -f` fails git-http-backend executes with inherited locale\n(analogous to\nthe else branch execution in the original), and if `test -f` succeeds the locale\nis forced to C and the one-time-script / git-http-backend run with the forced\nlocale. That being said, I think forcing the locale to C consistently would\nmake more sense. Depending on what you think, I can integrate that into the\nseries or leave for a future cleanup.\n\n>\n> Ah, we assume running one-time-script itself multiple times is safe\n> and does not cause issues. Our objective is to avoid returning\n> modified output twice. So while the first instance of us\n> successfully renames one-time-script to one-time-script.$$ and emits\n> the modified result, even if the second instance raced and managed\n> to run the script again, it will fail to rename with \"mv\", and\n> discard the modified output, and instead show the unmodified output\n> generated by the backend.\n>\n> OK. It is a bit tricky. It may help future readers if we said\n> something about this in the proposed log message (i.e., we consider\n> that it is perfectly fine to run one-time-script more than once; we\n> only want to avoid letting the second invocation's output used).\n>\n\nYes that is a good call, I will add some detail about this subtlety in the\nlog message and helper comment.\n"},{"id":"547642","messageId":"CAC2QwmKuHUP6_287T9SOLdjLdb=b4EqV4qJ_NnYCkGP0-d6qHA@mail.gmail.com","threadId":"65950","inReplyTo":"xmqqcxwxtfkp.fsf@gitster.g","subject":"Re: [PATCH 2/3] t/lib-httpd: make http-429 first-request check atomic","fromName":"Michael Montalbo","fromEmail":"mmontalbo@gmail.com","sentAt":"2026-07-09T18:10:33Z","receivedAt":"2026-07-09T18:10:46Z","isPatch":true,"body":"On Wed, Jul 8, 2026 at 1:02 PM Junio C Hamano wrote:\n>\n> \"Michael Montalbo via GitGitGadget\" writes:\n>\n> > -# Check if this is the first call (no state file exists)\n> > -if test -f \"$state_file\"\n> > +# Apache can run this CGI for concurrent requests, so the script decides\n> > +# whether this is the first call with a single atomic \"mkdir\": it succeeds for\n> > +# exactly one of any racing requests and fails for the rest. \"permanent\"\n> > +# always rate-limits and records no state.\n> > +if test \"$retry_after\" != permanent && ! mkdir \"$state\" 2>/dev/null\n>\n> I think the last sentence in the above comment was meant to explain\n> why the new code checks the value of \"$retry_after\", but it is not\n> clear if it is needed for correctness (in other words, the original\n> was wrong to do \"test -f && touch\" but also was wrong to do so even\n> when \"$retry_after\" is set to \"permanent), or if it is a mere\n> \"optimization opportunity\" you are taking advantage of. In either\n> case, it would be nice to see it explained in the proposed commit\n> log message.\n>\n\nIt is needed for correctness, and I agree it is not very clear from the log\nmessage / comment. I will spell out the reasoning for the change more\nclearly in both.\n\nThanks for taking a look at this!\n"},{"id":"547782","messageId":"pull.2171.v2.git.1783704657.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.git.1783479584.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] t/lib-httpd: make CGI test helpers concurrency-safe","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-07-10T17:30:54Z","receivedAt":"2026-07-10T17:31:00Z","isPatch":true,"body":"The httpd tests share a handful of CGI helper scripts under t/lib-httpd. Two\nof them keep state between requests in the shared HTTPD_ROOT_PATH on the\nassumption that the web server hands them one request at a time. It does\nnot: Apache serves requests concurrently, and a single Git operation can\nopen more than one request to the same endpoint at once. A partial fetch\nthat receives a REF_DELTA against a missing promisor object lazily fetches\nthat base while the first response is still being served.\n\nUnder that overlap apply-one-time-script.sh loses: two requests both pass\nits \"test -f one-time-script\" check, one removes the marker, and the other\nfails to exec it and emits an empty body, which the server answers as HTTP\n500. In the field this is an occasional failure[1] of:\n\nt5616.47 tolerate server sending REF_DELTA against missing promisor objects\n\non the macOS CI runners, with:\n\nfatal: ... The requested URL returned error: 500 fatal: could not fetch from\npromisor remote\n\nI could not reproduce it against a live server (the window is tiny and\ntiming-dependent), but the macOS CI error log names the exact failure, and\nthe new test reproduces the helper's shell error.\n\nhttp-429.sh keeps its \"already returned 429 once\" state with the same\nnon-atomic test-and-set. Its retry flow is mostly sequential so it seems\nless likely to fail, but it is the same latent race.\n\nEach fix is local: claim/consume the one-shot marker with an atomic rename,\nand elect the first request with an atomic mkdir, rather than a \"test -f\"\nfollowed by a separate remove or touch.\n\n * Patch 1 fixes apply-one-time-script.sh (the actual flake) and adds t5567,\n which drives the helper directly with no web server so the overlap can be\n forced deterministically.\n * Patch 2 makes http-429.sh atomic.\n * Patch 3 documents the atomic idioms generally in t/README (they are not\n specific to CGI or HTTP), citing Git's own lockfile machinery and\n make_symlink(), with a pointer from the lib-httpd list.\n\nChanges since v1:\n\n * Clarify that one-time-script.sh can and should be able to run more than\n once. Explain that the constraint on the script execution is that one and\n only one modified response is guaranteed to be returned to the client.\n\n * The existing behavior w.r.t. inconsistent use of locale C vs. inherited\n locale when executing t/lib-httpd/apply-one-time-script.sh has been\n retained from the original version, and is left as future potential\n cleanup.\n\n * Spell out why the logic changes to the \"permanent mode check\" in\n t/lib-httpd/http-429.sh are needed for correctness, rather than an\n optimization opportunity.\n\n[1]\nhttps://github.com/gitgitgadget/git/actions/runs/28756172690/job/85263916762?pr=2169\n\nMichael Montalbo (3):\n t/lib-httpd: fix apply-one-time-script race under concurrent requests\n t/lib-httpd: make http-429 first-request check atomic\n t/README: document writing concurrency-safe helpers\n\n t/README | 32 ++++++++++\n t/lib-httpd.sh | 3 +\n t/lib-httpd/apply-one-time-script.sh | 44 +++++++++----\n t/lib-httpd/http-429.sh | 28 ++++----\n t/meson.build | 1 +\n t/t5567-one-time-script.sh | 96 ++++++++++++++++++++++++++++\n 6 files changed, 179 insertions(+), 25 deletions(-)\n create mode 100755 t/t5567-one-time-script.sh\n\n\nbase-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2171%2Fmmontalbo%2Fmm%2Flib-httpd-cgi-safe-proto-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/2171\n\nRange-diff vs v1:\n\n 1: 9f48aa6d6d ! 1: 79b56402c0 t/lib-httpd: fix apply-one-time-script race under concurrent requests\n @@ Commit message\n body and keeps the marker for a later request. No path emits an empty\n body, so the HTTP 500 no longer occurs.\n \n + Running the one-time script more than once is fine; the only thing to\n + avoid is serving a second, racing request's modified output. Two\n + requests can both find the marker and run the script before either\n + renames it away, but the rename is atomic, so exactly one of them wins:\n + it serves its modified body and consumes the marker. The loser's rename\n + fails because the marker is already gone, so it discards the modified\n + output it produced and serves the unmodified body instead. The rename,\n + not running the script, is what is serialized.\n +\n Add t5567 to lock this down. The overlap depends on timing, so a live\n httpd test such as t5616.47 (the real code path) passes almost every\n time even against the buggy helper; t5567 instead drives the helper\n @@ t/lib-httpd/apply-one-time-script.sh\n +# is left emitting an empty body, which the server would report as HTTP 500.\n +# Scratch files are per-request ($$) so concurrent requests do not clobber each\n +# other.\n -+\n -+test -f one-time-script || exec \"$GIT_EXEC_PATH/git-http-backend\"\n ++#\n ++# The script may run more than once: the marker is consumed when the response\n ++# actually changes (the rename after \"cmp\"), not when the script runs, so a\n ++# request whose response is not the targeted one runs the script, sees no\n ++# change, and leaves the marker for a later request. That is safe because the\n ++# scripts are stateless filters over the captured response.\n \n -\t\"$GIT_EXEC_PATH/git-http-backend\" >out\n -\t./one-time-script out >out_modified\n -+LC_ALL=C\n -+export LC_ALL\n ++test -f one-time-script || exec \"$GIT_EXEC_PATH/git-http-backend\"\n \n -\tif cmp -s out out_modified\n -\tthen\n @@ t/lib-httpd/apply-one-time-script.sh\n -\t\tcat out_modified\n -\t\trm one-time-script\n -\tfi\n ++LC_ALL=C\n ++export LC_ALL\n ++\n +out=out.$$\n +modified=out-modified.$$\n +\"$GIT_EXEC_PATH/git-http-backend\" >\"$out\"\n 2: efd34c1715 ! 2: 5f56f32a74 t/lib-httpd: make http-429 first-request check atomic\n @@ Commit message\n which fails if the directory already exists, so exactly one of any\n concurrent requests is rate-limited and the rest are forwarded.\n \n + Skipping state for \"permanent\" is required for correctness, not just an\n + optimization. The marker tells a later or concurrent request that a 429\n + has already been served, so that it forwards to git-http-backend instead\n + of rate-limiting. Since \"permanent\" must return 429 to every request,\n + that marker must never become visible to another such request.\n +\n + The original did not achieve this by staying stateless: its \"touch\" of\n + the marker ran unconditionally, and the \"permanent\" case removed it\n + afterward with \"rm -f\". That create-then-remove leaves a window in which\n + a concurrent \"permanent\" request sees the marker and is forwarded. It is\n + the same class of check-then-act race this patch removes from the\n + first-request check, latent for the same reason: the flow is mostly\n + sequential. This version fuses the check and the mark into one atomic\n + mkdir and, rather than recreate the pattern as mkdir-then-rmdir, skips\n + the mkdir for \"permanent\" with a \"!= permanent\" guard. No marker is ever\n + created, so there is no window and every \"permanent\" request\n + rate-limits.\n +\n There is no accompanying regression test. The check and the set are\n adjacent commands with no external step in between to synchronize on, so\n the overlap cannot be forced deterministically, only reproduced\n @@ t/lib-httpd/http-429.sh: repo_path=\"${remaining#*/}\" # Get rest (repo path)\n \n -# Check if this is the first call (no state file exists)\n -if test -f \"$state_file\"\n -+# Apache can run this CGI for concurrent requests, so the script decides\n -+# whether this is the first call with a single atomic \"mkdir\": it succeeds for\n -+# exactly one of any racing requests and fails for the rest. \"permanent\"\n -+# always rate-limits and records no state.\n ++# This endpoint returns 429 to the first request and forwards later ones to\n ++# git-http-backend, so the retry succeeds. Apache can run this CGI for several\n ++# requests at once, so a single atomic \"mkdir\" elects that first request: the\n ++# one whose mkdir succeeds returns 429 and leaves the directory behind as the\n ++# \"already rate-limited\" marker; every later request finds the directory (mkdir\n ++# fails) and is forwarded.\n ++#\n ++# \"permanent\" is the exception: it must return 429 to every request and never\n ++# succeed, so it skips the mkdir and records no state. A leftover directory\n ++# would make its own later requests find the marker and be forwarded, which is\n ++# exactly what \"permanent\" must not do.\n +if test \"$retry_after\" != permanent && ! mkdir \"$state\" 2>/dev/null\n then\n \t# Already returned 429 once, forward to git-http-backend\n 3: 771d264d29 = 3: f158e1f92e t/README: document writing concurrency-safe helpers\n\n-- \ngitgitgadget\n"},{"id":"547783","messageId":"79b56402c0d5d8b709f41b25ca66aed98ebbb007.1783704657.git.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.v2.git.1783704657.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-07-10T17:30:55Z","receivedAt":"2026-07-10T17:31:01Z","isPatch":true,"body":"From: Michael Montalbo \n\napply-one-time-script.sh checks for the \"one-time-script\" marker, runs\nit, captures the git-http-backend response in the fixed-name files \"out\"\nand \"out_modified\", and removes the marker only after it has finished\nserving the modified response. Because the client receives the response\nbody before that removal, it can start its next request while the marker\nstill exists. Apache can then run this CGI for two requests at once: a\npartial fetch that receives a REF_DELTA against a missing promisor\nobject lazily fetches that base while the first response is still in\nflight. The second request passes the marker check, the first request\nthen removes the marker, and the second fails to exec the now-missing\nmarker, emits no output, and the server answers HTTP 500:\n\n fatal: ... The requested URL returned error: 500\n fatal: could not fetch from promisor remote\n\nThis has been seen as a flaky failure of t5616.47 on the macOS CI\nrunners.\n\nClaim the marker atomically with a rename, and only once the one-time\nscript has succeeded and actually changed the response; give the scratch\nfiles per-request names. A request that loses the rename, or whose\nscript fails or leaves the response unchanged, serves the unmodified\nbody and keeps the marker for a later request. No path emits an empty\nbody, so the HTTP 500 no longer occurs.\n\nRunning the one-time script more than once is fine; the only thing to\navoid is serving a second, racing request's modified output. Two\nrequests can both find the marker and run the script before either\nrenames it away, but the rename is atomic, so exactly one of them wins:\nit serves its modified body and consumes the marker. The loser's rename\nfails because the marker is already gone, so it discards the modified\noutput it produced and serves the unmodified body instead. The rename,\nnot running the script, is what is serialized.\n\nAdd t5567 to lock this down. The overlap depends on timing, so a live\nhttpd test such as t5616.47 (the real code path) passes almost every\ntime even against the buggy helper; t5567 instead drives the helper\ndirectly with a fake git-http-backend and forces the overlap with FIFOs.\nAgainst the pre-fix helper it fails with the same shell error seen in\nthe field:\n\n ./one-time-script: No such file or directory\n\nSigned-off-by: Michael Montalbo \n---\n t/lib-httpd/apply-one-time-script.sh | 44 +++++++++----\n t/meson.build | 1 +\n t/t5567-one-time-script.sh | 96 ++++++++++++++++++++++++++++\n 3 files changed, 127 insertions(+), 14 deletions(-)\n create mode 100755 t/t5567-one-time-script.sh\n\ndiff --git a/t/lib-httpd/apply-one-time-script.sh b/t/lib-httpd/apply-one-time-script.sh\nindex b1682944e2..adb9cec528 100644\n--- a/t/lib-httpd/apply-one-time-script.sh\n+++ b/t/lib-httpd/apply-one-time-script.sh\n@@ -6,21 +6,37 @@\n #\n # This can be used to simulate the effects of the repository changing in\n # between HTTP request-response pairs.\n-if test -f one-time-script\n-then\n-\tLC_ALL=C\n-\texport LC_ALL\n+#\n+# Apache can run this CGI for concurrent requests (for example a partial fetch\n+# that lazily fetches a missing object while the first response is still in\n+# flight), so the helper claims the marker atomically with a rename, and only\n+# once it has decided to modify the response. A request that loses the race\n+# finds the marker already gone and serves its response unchanged; no request\n+# is left emitting an empty body, which the server would report as HTTP 500.\n+# Scratch files are per-request ($$) so concurrent requests do not clobber each\n+# other.\n+#\n+# The script may run more than once: the marker is consumed when the response\n+# actually changes (the rename after \"cmp\"), not when the script runs, so a\n+# request whose response is not the targeted one runs the script, sees no\n+# change, and leaves the marker for a later request. That is safe because the\n+# scripts are stateless filters over the captured response.\n \n-\t\"$GIT_EXEC_PATH/git-http-backend\" >out\n-\t./one-time-script out >out_modified\n+test -f one-time-script || exec \"$GIT_EXEC_PATH/git-http-backend\"\n \n-\tif cmp -s out out_modified\n-\tthen\n-\t\tcat out\n-\telse\n-\t\tcat out_modified\n-\t\trm one-time-script\n-\tfi\n+LC_ALL=C\n+export LC_ALL\n+\n+out=out.$$\n+modified=out-modified.$$\n+\"$GIT_EXEC_PATH/git-http-backend\" >\"$out\"\n+\n+if ./one-time-script \"$out\" 2>/dev/null >\"$modified\" &&\n+ ! cmp -s \"$out\" \"$modified\" &&\n+ mv one-time-script one-time-script.$$ 2>/dev/null\n+then\n+\tcat \"$modified\"\n else\n-\t\"$GIT_EXEC_PATH/git-http-backend\"\n+\tcat \"$out\"\n fi\n+rm -f \"$out\" \"$modified\" one-time-script.$$\ndiff --git a/t/meson.build b/t/meson.build\nindex 3219264fe7..a118a4d719 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -707,6 +707,7 @@ integration_tests = [\n 't5564-http-proxy.sh',\n 't5565-push-multiple.sh',\n 't5566-push-group.sh',\n+ 't5567-one-time-script.sh',\n 't5570-git-daemon.sh',\n 't5571-pre-push-hook.sh',\n 't5572-pull-submodule.sh',\ndiff --git a/t/t5567-one-time-script.sh b/t/t5567-one-time-script.sh\nnew file mode 100755\nindex 0000000000..cd8e656005\n--- /dev/null\n+++ b/t/t5567-one-time-script.sh\n@@ -0,0 +1,96 @@\n+#!/bin/sh\n+\n+test_description='apply-one-time-script CGI helper is safe under concurrent requests'\n+\n+. ./test-lib.sh\n+\n+HELPER=\"$TEST_DIRECTORY/lib-httpd/apply-one-time-script.sh\"\n+\n+test_expect_success PIPE 'concurrent requests: one rewritten, one passed through, neither empty' '\n+\tmkdir workdir fakebin &&\n+\tENTERED=\"$PWD/entered\" &&\n+\tGATE=\"$PWD/gate\" &&\n+\texport ENTERED GATE &&\n+\tmkfifo \"$ENTERED\" \"$GATE\" &&\n+\n+\t# Stand in for git-http-backend. The modify role returns a response\n+\t# containing \"packfile\", which the one-time script rewrites. The\n+\t# passthrough role returns a response that is left untouched, but first\n+\t# announces that it has entered the helper and then blocks, so that it\n+\t# is still in flight when the modify role claims and removes the marker.\n+\twrite_script fakebin/git-http-backend <<-\\EOF &&\n+\tprintf \"Status: 200 OK\\r\\n\"\n+\tprintf \"Content-Type: application/x-git-result\\r\\n\"\n+\tprintf \"\\r\\n\"\n+\tif test \"$ROLE\" = modify\n+\tthen\n+\t\tprintf \"packfile\\n\"\n+\telse\n+\t\techo entered >\"$ENTERED\"\n+\t\tread -r released <\"$GATE\"\n+\t\tprintf \"refs\\n\"\n+\tfi\n+\tEOF\n+\n+\t# The transform that replace_packfile would install as one-time-script:\n+\t# rewrite responses that contain \"packfile\", leave the rest alone.\n+\twrite_script workdir/one-time-script <<-\\EOF &&\n+\tif grep packfile \"$1\" >/dev/null\n+\tthen\n+\t\tsed \"/packfile/q\" \"$1\" &&\n+\t\tprintf \"REPLACED\\n\"\n+\telse\n+\t\tcat \"$1\"\n+\tfi\n+\tEOF\n+\n+\tGIT_EXEC_PATH=\"$PWD/fakebin\" &&\n+\texport GIT_EXEC_PATH &&\n+\n+\t# Hold GATE open read-write on fd 9 for the duration, so releasing the\n+\t# passthrough request below cannot block even if that request has\n+\t# already exited (it keeps a reader on the FIFO).\n+\texec 9<>\"$GATE\" &&\n+\n+\t# Launch the passthrough request in the background. It enters the\n+\t# helper, signals us through ENTERED, then blocks on GATE inside the\n+\t# fake backend. The braces keep the && chain intact while backgrounding\n+\t# only the subshell, so \"wait\" can reap it by pid; kill it on any exit\n+\t# so a stray blocked child cannot hold the test output open and stall a\n+\t# reader such as prove.\n+\t{ (\n+\t\tcd workdir &&\n+\t\tROLE=passthrough sh \"$HELPER\" >../passthrough.out 2>../passthrough.err\n+\t) & } &&\n+\tpassthrough_pid=$! &&\n+\ttest_when_finished \"kill $passthrough_pid 2>/dev/null || :\" &&\n+\n+\t# Wait until the passthrough request is past the marker check.\n+\tread -r entered <\"$ENTERED\" &&\n+\n+\t# Run the modifying request to completion while the passthrough request\n+\t# is still blocked.\n+\t(\n+\t\tcd workdir &&\n+\t\tROLE=modify sh \"$HELPER\" >../modify.out 2>../modify.err\n+\t) &&\n+\n+\t# Release the passthrough request and let it finish. Ignore the helper\n+\t# exit status here so a broken helper is diagnosed by the assertions\n+\t# below rather than aborting the test.\n+\techo released >&9 &&\n+\t{ wait \"$passthrough_pid\" || :; } &&\n+\n+\t# Neither request may error out or produce an empty (HTTP 500) body,\n+\t# and each must have played its role: the modify request rewrote its\n+\t# response and the passthrough request came through untouched.\n+\ttest_must_be_empty passthrough.err &&\n+\ttest_must_be_empty modify.err &&\n+\ttest_grep \"Status: 200 OK\" passthrough.out &&\n+\ttest_grep \"Status: 200 OK\" modify.out &&\n+\ttest_grep REPLACED modify.out &&\n+\ttest_grep ! REPLACED passthrough.out &&\n+\ttest_grep refs passthrough.out\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"547784","messageId":"5f56f32a74b3d900148f02901bcd104927c5e088.1783704657.git.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.v2.git.1783704657.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] t/lib-httpd: make http-429 first-request check atomic","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-07-10T17:30:56Z","receivedAt":"2026-07-10T17:31:02Z","isPatch":true,"body":"From: Michael Montalbo \n\nhttp-429.sh records \"already returned 429 once\" with a \"test -f\"\nfollowed by a \"touch\" of a shared state file. That check-then-act is not\natomic: Apache can run this CGI for several requests at once, and two of\nthem can both pass the \"test -f\" before either \"touch\"es, so both treat\nthemselves as the first request. The retry flow that drives this\nendpoint is mostly sequential, so this has not been seen to fail, but\nthe race is latent.\n\nDecide whether this is the first request with a single atomic mkdir,\nwhich fails if the directory already exists, so exactly one of any\nconcurrent requests is rate-limited and the rest are forwarded.\n\nSkipping state for \"permanent\" is required for correctness, not just an\noptimization. The marker tells a later or concurrent request that a 429\nhas already been served, so that it forwards to git-http-backend instead\nof rate-limiting. Since \"permanent\" must return 429 to every request,\nthat marker must never become visible to another such request.\n\nThe original did not achieve this by staying stateless: its \"touch\" of\nthe marker ran unconditionally, and the \"permanent\" case removed it\nafterward with \"rm -f\". That create-then-remove leaves a window in which\na concurrent \"permanent\" request sees the marker and is forwarded. It is\nthe same class of check-then-act race this patch removes from the\nfirst-request check, latent for the same reason: the flow is mostly\nsequential. This version fuses the check and the mark into one atomic\nmkdir and, rather than recreate the pattern as mkdir-then-rmdir, skips\nthe mkdir for \"permanent\" with a \"!= permanent\" guard. No marker is ever\ncreated, so there is no window and every \"permanent\" request\nrate-limits.\n\nThere is no accompanying regression test. The check and the set are\nadjacent commands with no external step in between to synchronize on, so\nthe overlap cannot be forced deterministically, only reproduced\nprobabilistically; the fix is preventive.\n\nSigned-off-by: Michael Montalbo \n---\n t/lib-httpd/http-429.sh | 28 +++++++++++++++++-----------\n 1 file changed, 17 insertions(+), 11 deletions(-)\n\ndiff --git a/t/lib-httpd/http-429.sh b/t/lib-httpd/http-429.sh\nindex c97b16145b..9746ec67ae 100644\n--- a/t/lib-httpd/http-429.sh\n+++ b/t/lib-httpd/http-429.sh\n@@ -26,14 +26,24 @@ repo_path=\"${remaining#*/}\" # Get rest (repo path)\n # The repo name is the first component before any \"/\"\n repo_name=\"${repo_path%%/*}\"\n \n-# Use current directory (HTTPD_ROOT_PATH) for state file\n-# Create a safe filename from test_context, retry_after and repo_name\n-# This ensures all requests for the same test context share the same state file\n+# Use current directory (HTTPD_ROOT_PATH) for state.\n+# Create a safe name from test_context, retry_after and repo_name so that all\n+# requests for the same test context share the same state.\n safe_name=$(echo \"${test_context}-${retry_after}-${repo_name}\" | tr '/' '_' | tr -cd 'a-zA-Z0-9_-')\n-state_file=\"http-429-state-${safe_name}\"\n+state=\"http-429-state-${safe_name}\"\n \n-# Check if this is the first call (no state file exists)\n-if test -f \"$state_file\"\n+# This endpoint returns 429 to the first request and forwards later ones to\n+# git-http-backend, so the retry succeeds. Apache can run this CGI for several\n+# requests at once, so a single atomic \"mkdir\" elects that first request: the\n+# one whose mkdir succeeds returns 429 and leaves the directory behind as the\n+# \"already rate-limited\" marker; every later request finds the directory (mkdir\n+# fails) and is forwarded.\n+#\n+# \"permanent\" is the exception: it must return 429 to every request and never\n+# succeed, so it skips the mkdir and records no state. A leftover directory\n+# would make its own later requests find the marker and be forwarded, which is\n+# exactly what \"permanent\" must not do.\n+if test \"$retry_after\" != permanent && ! mkdir \"$state\" 2>/dev/null\n then\n \t# Already returned 429 once, forward to git-http-backend\n \t# Set PATH_INFO to just the repo path (without retry-after value)\n@@ -52,9 +62,6 @@ then\n \texec \"$GIT_EXEC_PATH/git-http-backend\"\n fi\n \n-# Mark that we've returned 429\n-touch \"$state_file\"\n-\n # Output HTTP 429 response\n printf \"Status: 429 Too Many Requests\\r\\n\"\n \n@@ -67,8 +74,7 @@ case \"$retry_after\" in\n \t\tprintf \"Retry-After: invalid-format-123abc\\r\\n\"\n \t\t;;\n \tpermanent)\n-\t\t# Always return 429, don't set state file for success\n-\t\trm -f \"$state_file\"\n+\t\t# Always return 429\n \t\tprintf \"Retry-After: 1\\r\\n\"\n \t\tprintf \"Content-Type: text/plain\\r\\n\"\n \t\tprintf \"\\r\\n\"\n-- \ngitgitgadget\n\n"},{"id":"547785","messageId":"f158e1f92e9c586fca34faecaef23f9581d65478.1783704657.git.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.v2.git.1783704657.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] t/README: document writing concurrency-safe helpers","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-07-10T17:30:57Z","receivedAt":"2026-07-10T17:31:04Z","isPatch":true,"body":"From: Michael Montalbo \n\nThe apply-one-time-script.sh and http-429.sh fixes addressed the same\nunderlying problem: a test helper assuming it has exclusive access to a\nfile when the web server can run it for several requests at once. The\natomic idioms that avoid this are not specific to CGI or to HTTP, so\ndocument them generally, alongside the other guidance for writing tests,\nand leave a pointer from the lib-httpd helper list rather than a local\ncomment. The note covers the anti-pattern (a \"test -f\" then a separate\nact) and the two safe operations (mkdir to elect a winner, rename to\nconsume a one-shot marker), citing Git's own lockfile machinery and\nmake_symlink() as precedent.\n\nSigned-off-by: Michael Montalbo \n---\n t/README | 32 ++++++++++++++++++++++++++++++++\n t/lib-httpd.sh | 3 +++\n 2 files changed, 35 insertions(+)\n\ndiff --git a/t/README b/t/README\nindex 085921be4b..a9d425f392 100644\n--- a/t/README\n+++ b/t/README\n@@ -854,6 +854,38 @@ from the test harness library. At the end of the script, call\n 'test_done'.\n \n \n+Writing concurrency-safe helpers\n+--------------------------------\n+\n+Some test code runs concurrently: a test may background work with '&',\n+and the helper scripts installed for the web server (in t/lib-httpd) are\n+run once per request, so the same script can execute for several\n+requests at once. Such code cannot assume it has exclusive access to a\n+file.\n+\n+When exactly one of several concurrent processes needs to \"win\" a\n+decision, a single atomic filesystem operation can make it, rather than\n+a check followed by a separate action. A \"test -f X\" then \"touch X\"\n+(or \"rm X\") races: two processes can both pass the check before either\n+acts. Two atomic operations avoid this:\n+\n+ - \"mkdir dir\", which fails if the directory already exists, so that\n+ exactly one caller wins, electing a first or only request (see\n+ t/lib-httpd/http-429.sh).\n+\n+ - \"mv src dst\" (rename), which fails if the source is gone, so that\n+ exactly one caller consumes it, claiming a planted one-shot marker\n+ (see t/lib-httpd/apply-one-time-script.sh).\n+\n+A \"$$\" suffix on per-request scratch files keeps concurrent invocations\n+from clobbering each other's fixed-name files.\n+\n+This is a standard shell locking idiom, and the same reasoning behind\n+Git's own lockfile machinery, which creates its lock with O_CREAT|O_EXCL,\n+and make_symlink() in t/test-lib.sh, which uses an mkdir lock: an atomic\n+operation whose failure indicates that another process got there first.\n+\n+\n Test harness library\n --------------------\n \ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex fc646447d5..d64f9c8c2d 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -159,6 +159,9 @@ prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/proxy-passwd \"$HTTPD_ROOT_PATH\"\n+\t# The web server can run any of these CGI scripts for two requests at\n+\t# once; a helper that keeps state between requests must do so with an\n+\t# atomic operation. See \"Writing concurrency-safe helpers\" in t/README.\n \tinstall_script incomplete-length-upload-pack-v2-http.sh\n \tinstall_script incomplete-body-upload-pack-v2-http.sh\n \tinstall_script error-no-report.sh\n-- \ngitgitgadget\n"},{"id":"549427","messageId":"CAC2QwmLWkk4JS2XKLdj4i4CAtr7zZo=9tV_=pPQ77zR+R=pGUw@mail.gmail.com","threadId":"65950","inReplyTo":"pull.2171.v2.git.1783704657.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] t/lib-httpd: make CGI test helpers concurrency-safe","fromName":"Michael Montalbo","fromEmail":"mmontalbo@gmail.com","sentAt":"2026-08-02T03:02:43Z","receivedAt":"2026-08-02T03:02:56Z","isPatch":true,"body":"Friendly ping. If it makes it any more enticing, I believe the flake fixed in\nthis series is responsible for at least a couple CI failures[1][2] since the\nsubmission occurred.\n\n[1] https://github.com/gitgitgadget/git/actions/runs/28983114431/job/86006743571\n[2] https://github.com/git/git/actions/runs/29063352938/job/86269734698\n"},{"id":"549517","messageId":"xmqq4ihayil0.fsf@gitster.g","threadId":"65950","inReplyTo":"pull.2171.v2.git.1783704657.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] t/lib-httpd: make CGI test helpers concurrency-safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-03T21:55:55Z","receivedAt":"2026-08-03T21:55:58Z","isPatch":true,"body":"\"Michael Montalbo via GitGitGadget\" writes:\n\n> Each fix is local: claim/consume the one-shot marker with an atomic rename,\n> and elect the first request with an atomic mkdir, rather than a \"test -f\"\n> followed by a separate remove or touch.\n>\n> * Patch 1 fixes apply-one-time-script.sh (the actual flake) and adds t5567,\n> which drives the helper directly with no web server so the overlap can be\n> forced deterministically.\n> * Patch 2 makes http-429.sh atomic.\n> * Patch 3 documents the atomic idioms generally in t/README (they are not\n> specific to CGI or HTTP), citing Git's own lockfile machinery and\n> make_symlink(), with a pointer from the lib-httpd list.\n\nI was scanning the \"What's cooking\" report for topics marked as\n\"Needs review\" to see if I could find ones that are relatively easy\nto validate, and I hit this one.\n\nThe key change [1/3] is well thought out and nicely done. [2/3] is\nexplained better than the corresponding step in v1, and [3/3] adds\nhelpful tips to the t/README documentation. They all look quite\ngood.\n\nThanks.\n"},{"id":"549536","messageId":"anGcwAZgbarxi6_k@pks.im","threadId":"65950","inReplyTo":"79b56402c0d5d8b709f41b25ca66aed98ebbb007.1783704657.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-04T08:03:12Z","receivedAt":"2026-08-04T08:03:21Z","isPatch":true,"body":"On Fri, Jul 10, 2026 at 05:30:55PM +0000, Michael Montalbo via GitGitGadget wrote:\n> diff --git a/t/lib-httpd/apply-one-time-script.sh b/t/lib-httpd/apply-one-time-script.sh\n> index b1682944e2..adb9cec528 100644\n> --- a/t/lib-httpd/apply-one-time-script.sh\n> +++ b/t/lib-httpd/apply-one-time-script.sh\n> @@ -6,21 +6,37 @@\n> #\n> # This can be used to simulate the effects of the repository changing in\n> # between HTTP request-response pairs.\n> -if test -f one-time-script\n> -then\n> -\tLC_ALL=C\n> -\texport LC_ALL\n> +#\n> +# Apache can run this CGI for concurrent requests (for example a partial fetch\n> +# that lazily fetches a missing object while the first response is still in\n> +# flight), so the helper claims the marker atomically with a rename, and only\n> +# once it has decided to modify the response. A request that loses the race\n> +# finds the marker already gone and serves its response unchanged; no request\n> +# is left emitting an empty body, which the server would report as HTTP 500.\n> +# Scratch files are per-request ($$) so concurrent requests do not clobber each\n> +# other.\n> +#\n> +# The script may run more than once: the marker is consumed when the response\n> +# actually changes (the rename after \"cmp\"), not when the script runs, so a\n> +# request whose response is not the targeted one runs the script, sees no\n> +# change, and leaves the marker for a later request. That is safe because the\n> +# scripts are stateless filters over the captured response.\n> \n> -\t\"$GIT_EXEC_PATH/git-http-backend\" >out\n> -\t./one-time-script out >out_modified\n> +test -f one-time-script || exec \"$GIT_EXEC_PATH/git-http-backend\"\n> \n> -\tif cmp -s out out_modified\n> -\tthen\n> -\t\tcat out\n> -\telse\n> -\t\tcat out_modified\n> -\t\trm one-time-script\n> -\tfi\n> +LC_ALL=C\n> +export LC_ALL\n> +\n> +out=out.$$\n> +modified=out-modified.$$\n> +\"$GIT_EXEC_PATH/git-http-backend\" >\"$out\"\n> +\n> +if ./one-time-script \"$out\" 2>/dev/null >\"$modified\" &&\n\nIs it intentional that we swallow stderr of this script now? We didn't\nbefore. I assume that this is to swallow the error in case the script\ngot removed by the concurrent request?\n\nPatrick\n"},{"id":"549537","messageId":"anGcx4lRyy3jyS1D@pks.im","threadId":"65950","inReplyTo":"f158e1f92e9c586fca34faecaef23f9581d65478.1783704657.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] t/README: document writing concurrency-safe helpers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-04T08:03:19Z","receivedAt":"2026-08-04T08:03:25Z","isPatch":true,"body":"On Fri, Jul 10, 2026 at 05:30:57PM +0000, Michael Montalbo via GitGitGadget wrote:\n> diff --git a/t/README b/t/README\n> index 085921be4b..a9d425f392 100644\n> --- a/t/README\n> +++ b/t/README\n> @@ -854,6 +854,38 @@ from the test harness library. At the end of the script, call\n> 'test_done'.\n> \n> \n> +Writing concurrency-safe helpers\n> +--------------------------------\n\nNit: this paragraph is quite specific to lib-httpd, so it would make\nsense to mention it in the header here. E.g.\n\n Writing concurrency-safe lib-httpd helpers\n\n> +Some test code runs concurrently: a test may background work with '&',\n> +and the helper scripts installed for the web server (in t/lib-httpd) are\n> +run once per request, so the same script can execute for several\n> +requests at once. Such code cannot assume it has exclusive access to a\n> +file.\n> +\n> +When exactly one of several concurrent processes needs to \"win\" a\n> +decision, a single atomic filesystem operation can make it, rather than\n> +a check followed by a separate action. A \"test -f X\" then \"touch X\"\n> +(or \"rm X\") races: two processes can both pass the check before either\n> +acts. Two atomic operations avoid this:\n> +\n> + - \"mkdir dir\", which fails if the directory already exists, so that\n> + exactly one caller wins, electing a first or only request (see\n> + t/lib-httpd/http-429.sh).\n> +\n> + - \"mv src dst\" (rename), which fails if the source is gone, so that\n> + exactly one caller consumes it, claiming a planted one-shot marker\n> + (see t/lib-httpd/apply-one-time-script.sh).\n\nA simple \"rm\" (without \"-f\") should work as well, right?\n\n> +A \"$$\" suffix on per-request scratch files keeps concurrent invocations\n> +from clobbering each other's fixed-name files.\n\nNit: it might be a bit easier to read if we explicitly mention PIDs\ninstead of assuming that every reader immediately knows that \"$$\" will\nexpand to the PID. E.g.:\n\n Appending a PID to the per-request scratch filenames keeps...\n\nThanks!\n\nPatrick\n"},{"id":"550032","messageId":"CAC2QwmK=K3EqvZWKQpy8ag+A8kMghNB6N=0dW7pjY1xJup4_Xg@mail.gmail.com","threadId":"65950","inReplyTo":"anGcwAZgbarxi6_k@pks.im","subject":"Re: [PATCH v2 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests","fromName":"Michael Montalbo","fromEmail":"mmontalbo@gmail.com","sentAt":"2026-08-07T16:29:32Z","receivedAt":"2026-08-07T16:29:47Z","isPatch":true,"body":"On Tue, Aug 4, 2026 at 1:03 AM Patrick Steinhardt wrote:\n>\n> > +\n> > +out=out.$$\n> > +modified=out-modified.$$\n> > +\"$GIT_EXEC_PATH/git-http-backend\" >\"$out\"\n> > +\n> > +if ./one-time-script \"$out\" 2>/dev/null >\"$modified\" &&\n>\n> Is it intentional that we swallow stderr of this script now? We didn't\n> before. I assume that this is to swallow the error in case the script\n> got removed by the concurrent request?\n>\n\nYes, you are correct on both counts. This is an intentional change\nmeant to swallow (an expected) stderr in case the script got removed\nalready by a concurrent request, but that is not clear on its own. I will\nadd an explanatory comment spelling this out.\n"},{"id":"550034","messageId":"CAC2Qwm+Jni+xU=gaef1AWCMj9+GUQhMrCWX9DFpS3y757pxv=Q@mail.gmail.com","threadId":"65950","inReplyTo":"anGcx4lRyy3jyS1D@pks.im","subject":"Re: [PATCH v2 3/3] t/README: document writing concurrency-safe helpers","fromName":"Michael Montalbo","fromEmail":"mmontalbo@gmail.com","sentAt":"2026-08-07T16:51:34Z","receivedAt":"2026-08-07T16:51:47Z","isPatch":true,"body":"On Tue, Aug 4, 2026 at 1:03 AM Patrick Steinhardt wrote:\n>\n> >\n> > +Writing concurrency-safe helpers\n> > +--------------------------------\n>\n> Nit: this paragraph is quite specific to lib-httpd, so it would make\n> sense to mention it in the header here. E.g.\n>\n> Writing concurrency-safe lib-httpd helpers\n>\n\nOriginally, I did just have this as a blurb in t/lib-httpd.sh. I ended up moving\nit here and trying to make the advice apply more generally, though the only\nother existing example I could find in another domain was the\nmake_symlink() reference. My intention was to make sure someone working\non a test helper with concurrency didn't skip over the section just because\nthey saw \"http\" and thought the advice didn't apply to their use case.\n\nI'm inclined to make the language in the section more http-agnostic rather\nthan changing the title to be specific to http, but I do not feel very strongly\nabout it. If we were to frame this as http-specific advice maybe it should go\nback to t/lib-httpd.sh instead of t/README?\n\n>\n> A simple \"rm\" (without \"-f\") should work as well, right?\n>\n\nYes, definitely. I think I over-corrected in excising \"rm\" from the test helpers\nand the advice given here since I associated it with the flawed patterns that\nallowed for the race issues. I will redo the treatment of \"rm\" in the series\nincluding reverting where \"mv\" replaced \"rm\" unnecessarily in the helpers.\n\n> > +A \"$$\" suffix on per-request scratch files keeps concurrent invocations\n> > +from clobbering each other's fixed-name files.\n>\n> Nit: it might be a bit easier to read if we explicitly mention PIDs\n> instead of assuming that every reader immediately knows that \"$$\" will\n> expand to the PID. E.g.:\n>\n> Appending a PID to the per-request scratch filenames keeps...\n>\n\nAgreed, will fix.\n\n> Thanks!\n>\n\nThank you for taking a look and your feedback!\n"},{"id":"550157","messageId":"anlqeshH0FXaLvF5@pks.im","threadId":"65950","inReplyTo":"CAC2Qwm+Jni+xU=gaef1AWCMj9+GUQhMrCWX9DFpS3y757pxv=Q@mail.gmail.com","subject":"Re: [PATCH v2 3/3] t/README: document writing concurrency-safe helpers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-10T06:06:50Z","receivedAt":"2026-08-10T06:06:56Z","isPatch":true,"body":"On Fri, Aug 07, 2026 at 09:51:34AM -0700, Michael Montalbo wrote:\n> On Tue, Aug 4, 2026 at 1:03 AM Patrick Steinhardt wrote:\n> >\n> > >\n> > > +Writing concurrency-safe helpers\n> > > +--------------------------------\n> >\n> > Nit: this paragraph is quite specific to lib-httpd, so it would make\n> > sense to mention it in the header here. E.g.\n> >\n> > Writing concurrency-safe lib-httpd helpers\n> >\n> \n> Originally, I did just have this as a blurb in t/lib-httpd.sh. I ended up moving\n> it here and trying to make the advice apply more generally, though the only\n> other existing example I could find in another domain was the\n> make_symlink() reference. My intention was to make sure someone working\n> on a test helper with concurrency didn't skip over the section just because\n> they saw \"http\" and thought the advice didn't apply to their use case.\n> \n> I'm inclined to make the language in the section more http-agnostic rather\n> than changing the title to be specific to http, but I do not feel very strongly\n> about it. If we were to frame this as http-specific advice maybe it should go\n> back to t/lib-httpd.sh instead of t/README?\n\nDunno. I'm not sure there's much value outside of httpd, so I'm still\ninclined to make it httpd-specific. And if so, moving it into \"t/\" would\nmake sense.\n\nBut I don't feel overly strong about this, either, so I won't complain\nif this section stays as-is.\n\nPatrick\n"},{"id":"550462","messageId":"pull.2171.v3.git.1786583137.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.git.1783479584.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] t/lib-httpd: make CGI test helpers concurrency-safe","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-13T01:05:33Z","receivedAt":"2026-08-13T01:05:40Z","isPatch":true,"body":"The httpd tests share a handful of CGI helper scripts under t/lib-httpd. Two\nof them keep state between requests in the shared HTTPD_ROOT_PATH, on the\nassumption that the web server hands them one request at a time. It does\nnot: Apache serves requests concurrently, and a single Git operation can\nopen more than one request to the same endpoint at once. For example, a\npartial fetch that receives a REF_DELTA against a missing promisor object\nlazily fetches that base while the first response is still being served.\n\nUnder that overlap apply-one-time-script.sh fails. Two requests both pass\nits \"test -f one-time-script\" check; one removes the marker; the other then\nfails to exec it, emits an empty body, and the server answers HTTP 500. In\nthe field this is an occasional failure[1] of\n\nt5616.47 tolerate server sending REF_DELTA against missing promisor objects\n\non the macOS CI runners, with\n\nfatal: ... The requested URL returned error: 500 fatal: could not fetch from\npromisor remote\n\nI could not reproduce it against a live server, since the window is tiny and\ntiming-dependent, but the macOS CI error log names the exact failure and the\nnew test reproduces the helper's shell error.\n\nhttp-429.sh keeps its \"already returned 429 once\" state with the same\nnon-atomic check-and-set. Its retry flow is mostly sequential, so it seems\nless likely to fail, but it is the same latent race.\n\nEach helper replaces a non-atomic \"test -f\" check and separate follow-up\naction with a single atomic operation whose exit status decides the outcome:\napply-one-time-script.sh consumes its one-shot marker with \"rm\" (without\n\"-f\"), and http-429.sh elects the first request with \"mkdir\".\n\n * Patch 1 fixes apply-one-time-script.sh (the actual flake) and adds t5567,\n which drives the helper directly with no web server so the overlap can be\n forced deterministically.\n * Patch 2 makes http-429.sh atomic.\n * Patch 3 documents the atomic idioms next to where t/lib-httpd.sh installs\n the CGI scripts, so the guidance is in front of anyone adding another\n helper.\n\nChanges since v2:\n\n * Patch 1 now consumes the marker with a plain \"rm\" (without \"-f\") instead\n of a rename. \"rm\" without \"-f\" already fails once the marker is gone,\n which is the atomicity the helper needs. A new comment explains why the\n helper discards the one-time script's stderr: a losing request can find\n the marker already removed.\n\n * Patch 3 is now specific to the lib-httpd CGI helpers and lives beside\n their install site in t/lib-httpd.sh, rather than as a general section in\n t/README.\n\n * Reworded several helper comments and the patch 1 and 2 log messages for\n clarity and to match the code; no behavior change.\n\n[1]\nhttps://github.com/gitgitgadget/git/actions/runs/28756172690/job/85263916762?pr=2169\n\nMichael Montalbo (3):\n t/lib-httpd: fix apply-one-time-script race under concurrent requests\n t/lib-httpd: make http-429 first-request check atomic\n t/lib-httpd: document writing concurrency-safe CGI helpers\n\n t/lib-httpd.sh | 13 ++++\n t/lib-httpd/apply-one-time-script.sh | 50 +++++++++++----\n t/lib-httpd/http-429.sh | 30 +++++----\n t/meson.build | 1 +\n t/t5567-one-time-script.sh | 96 ++++++++++++++++++++++++++++\n 5 files changed, 164 insertions(+), 26 deletions(-)\n create mode 100755 t/t5567-one-time-script.sh\n\n\nbase-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2171%2Fmmontalbo%2Fmm%2Flib-httpd-cgi-safe-proto-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/2171\n\nRange-diff vs v2:\n\n 1: 79b56402c0 ! 1: 862c4258e5 t/lib-httpd: fix apply-one-time-script race under concurrent requests\n @@ Metadata\n ## Commit message ##\n t/lib-httpd: fix apply-one-time-script race under concurrent requests\n \n - apply-one-time-script.sh checks for the \"one-time-script\" marker, runs\n - it, captures the git-http-backend response in the fixed-name files \"out\"\n - and \"out_modified\", and removes the marker only after it has finished\n - serving the modified response. Because the client receives the response\n - body before that removal, it can start its next request while the marker\n - still exists. Apache can then run this CGI for two requests at once: a\n - partial fetch that receives a REF_DELTA against a missing promisor\n - object lazily fetches that base while the first response is still in\n - flight. The second request passes the marker check, the first request\n - then removes the marker, and the second fails to exec the now-missing\n - marker, emits no output, and the server answers HTTP 500:\n + apply-one-time-script.sh is a CGI helper that, when the file\n + \"one-time-script\" is present, runs it to rewrite the git-http-backend\n + response. If \"one-time-script\" generates a response that differs from\n + git-http-backend, the modified response is returned and\n + \"one-time-script\" is deleted. Requests after the deletion return normal\n + git-http-backend responses.\n +\n + The deletion is not safe under concurrency. The helper serves the\n + modified body first and deletes \"one-time-script\" only afterward, so a\n + client can issue its next request while the file still exists. Apache\n + runs the CGI for both requests at once, for example when a partial fetch\n + lazily fetches a missing promisor base while the first response is still\n + in flight. Both requests find the file and try to run it; the first\n + deletes it; the second then fails to exec the now-missing file, produces\n + no output, and the server returns HTTP 500:\n \n fatal: ... The requested URL returned error: 500\n fatal: could not fetch from promisor remote\n \n - This has been seen as a flaky failure of t5616.47 on the macOS CI\n - runners.\n -\n - Claim the marker atomically with a rename, and only once the one-time\n - script has succeeded and actually changed the response; give the scratch\n - files per-request names. A request that loses the rename, or whose\n - script fails or leaves the response unchanged, serves the unmodified\n - body and keeps the marker for a later request. No path emits an empty\n - body, so the HTTP 500 no longer occurs.\n + This is the flaky failure of t5616.47 on the macOS CI runners.\n \n - Running the one-time script more than once is fine; the only thing to\n - avoid is serving a second, racing request's modified output. Two\n - requests can both find the marker and run the script before either\n - renames it away, but the rename is atomic, so exactly one of them wins:\n - it serves its modified body and consumes the marker. The loser's rename\n - fails because the marker is already gone, so it discards the modified\n - output it produced and serves the unmodified body instead. The rename,\n - not running the script, is what is serialized.\n + Fix it by removing the file with \"rm\" only after the script has actually\n + changed the response. Because \"rm\" without \"-f\" fails once the file is\n + gone, exactly one request removes it and serves the modified body. Any\n + other request serves the unmodified body. Running the script more than\n + once is harmless; only its deletion is serialized, so exactly one\n + request's modified response is ever served. Per-request scratch file\n + names keep concurrent runs from overwriting each other, and no path\n + emits an empty response body.\n \n - Add t5567 to lock this down. The overlap depends on timing, so a live\n - httpd test such as t5616.47 (the real code path) passes almost every\n - time even against the buggy helper; t5567 instead drives the helper\n - directly with a fake git-http-backend and forces the overlap with FIFOs.\n - Against the pre-fix helper it fails with the same shell error seen in\n - the field:\n + t5616.47 exercises the real code path but, being timing-dependent,\n + passes against the buggy helper almost every time. Add t5567, which\n + drives the helper directly with a fake git-http-backend and forces the\n + overlap with FIFOs; against the pre-fix helper it fails with the same\n + shell error seen in the field:\n \n ./one-time-script: No such file or directory\n \n @@ t/lib-httpd/apply-one-time-script.sh\n -\tLC_ALL=C\n -\texport LC_ALL\n +#\n -+# Apache can run this CGI for concurrent requests (for example a partial fetch\n -+# that lazily fetches a missing object while the first response is still in\n -+# flight), so the helper claims the marker atomically with a rename, and only\n -+# once it has decided to modify the response. A request that loses the race\n -+# finds the marker already gone and serves its response unchanged; no request\n -+# is left emitting an empty body, which the server would report as HTTP 500.\n -+# Scratch files are per-request ($$) so concurrent requests do not clobber each\n -+# other.\n ++# Apache can run this CGI for several requests at the same time. For example, a\n ++# partial fetch lazily fetches a missing object while the first response is\n ++# still in flight. To stay correct, the helper removes the marker only after\n ++# the response changes, and only with \"rm\" (without \"-f\"). The \"rm\" fails for\n ++# every request except the one that removes the marker first. That request\n ++# serves the modified body. Every other request serves its response unchanged.\n ++# No request emits an empty body, which Apache would report as HTTP 500.\n ++#\n ++# A scratch file name includes the process ID ($$), so concurrent requests do\n ++# not overwrite each other's files.\n +#\n -+# The script may run more than once: the marker is consumed when the response\n -+# actually changes (the rename after \"cmp\"), not when the script runs, so a\n -+# request whose response is not the targeted one runs the script, sees no\n -+# change, and leaves the marker for a later request. That is safe because the\n ++# The helper can run one-time-script more than once. It consumes the marker\n ++# when the response changes (the \"rm\" after \"cmp\"), not when it runs the\n ++# script. A request whose response is not the target runs the script, finds no\n ++# change, and leaves the marker for a later request. This is safe because the\n +# scripts are stateless filters over the captured response.\n \n -\t\"$GIT_EXEC_PATH/git-http-backend\" >out\n @@ t/lib-httpd/apply-one-time-script.sh\n +modified=out-modified.$$\n +\"$GIT_EXEC_PATH/git-http-backend\" >\"$out\"\n +\n ++# one-time-script can be gone here: a concurrent request may have consumed it\n ++# since the \"test -f\" above. Then \"./one-time-script\" fails, the exit status\n ++# selects the unmodified body, and \"2>/dev/null\" discards the expected\n ++# \"no such file\" message.\n +if ./one-time-script \"$out\" 2>/dev/null >\"$modified\" &&\n + ! cmp -s \"$out\" \"$modified\" &&\n -+ mv one-time-script one-time-script.$$ 2>/dev/null\n ++ rm one-time-script 2>/dev/null\n +then\n +\tcat \"$modified\"\n else\n -\t\"$GIT_EXEC_PATH/git-http-backend\"\n +\tcat \"$out\"\n fi\n -+rm -f \"$out\" \"$modified\" one-time-script.$$\n ++rm -f \"$out\" \"$modified\"\n \n ## t/meson.build ##\n @@ t/meson.build: integration_tests = [\n 2: 5f56f32a74 ! 2: 8ed22c02a1 t/lib-httpd: make http-429 first-request check atomic\n @@ Metadata\n ## Commit message ##\n t/lib-httpd: make http-429 first-request check atomic\n \n - http-429.sh records \"already returned 429 once\" with a \"test -f\"\n - followed by a \"touch\" of a shared state file. That check-then-act is not\n - atomic: Apache can run this CGI for several requests at once, and two of\n - them can both pass the \"test -f\" before either \"touch\"es, so both treat\n - themselves as the first request. The retry flow that drives this\n - endpoint is mostly sequential, so this has not been seen to fail, but\n - the race is latent.\n + http-429.sh returns 429 to the first request for an endpoint and\n + forwards later ones to git-http-backend so the retry succeeds. It\n + remembers that it has already answered 429 by checking for a shared\n + state file with \"test -f\" and creating it with \"touch\".\n \n - Decide whether this is the first request with a single atomic mkdir,\n - which fails if the directory already exists, so exactly one of any\n - concurrent requests is rate-limited and the rest are forwarded.\n + That \"check-and-set\" is not atomic. Apache runs the CGI for several\n + requests at once, so two of them can pass the \"test -f\" before either\n + \"touch\"es the file, and both then answer as the first request. The\n + retry flow is mostly sequential, so this has not been observed to fail,\n + but the race is latent. Replace the check and the \"touch\" with a single\n + atomic \"mkdir\", which fails if the directory already exists, so exactly\n + one of the concurrent requests is rate-limited and the rest are\n + forwarded.\n \n - Skipping state for \"permanent\" is required for correctness, not just an\n - optimization. The marker tells a later or concurrent request that a 429\n - has already been served, so that it forwards to git-http-backend instead\n - of rate-limiting. Since \"permanent\" must return 429 to every request,\n - that marker must never become visible to another such request.\n + The \"permanent\" mode needs one extra step, for correctness rather than\n + tidiness. The marker means \"429 already served, now forward\", so it must\n + never be visible to a request that must itself return 429. Since\n + \"permanent\" returns 429 to every request, it must leave no marker. The\n + original did not manage this. It ran the \"touch\" unconditionally and\n + removed the file with \"rm -f\" in the \"permanent\" case, and that\n + \"create-then-remove\" has the same racy window: a concurrent \"permanent\"\n + request can see the marker before the \"rm -f\" and be wrongly forwarded.\n + Skipping the \"mkdir\" entirely for \"permanent\" (the \"!= permanent\" guard)\n + leaves no marker at all, so every \"permanent\" request rate-limits.\n \n - The original did not achieve this by staying stateless: its \"touch\" of\n - the marker ran unconditionally, and the \"permanent\" case removed it\n - afterward with \"rm -f\". That create-then-remove leaves a window in which\n - a concurrent \"permanent\" request sees the marker and is forwarded. It is\n - the same class of check-then-act race this patch removes from the\n - first-request check, latent for the same reason: the flow is mostly\n - sequential. This version fuses the check and the mark into one atomic\n - mkdir and, rather than recreate the pattern as mkdir-then-rmdir, skips\n - the mkdir for \"permanent\" with a \"!= permanent\" guard. No marker is ever\n - created, so there is no window and every \"permanent\" request\n - rate-limits.\n -\n - There is no accompanying regression test. The check and the set are\n - adjacent commands with no external step in between to synchronize on, so\n - the overlap cannot be forced deterministically, only reproduced\n - probabilistically; the fix is preventive.\n + There is no regression test. The check and the set are adjacent commands\n + with nothing in between to synchronize on, so the overlap cannot be\n + forced deterministically, only reproduced by chance; the fix is\n + preventive.\n \n Signed-off-by: Michael Montalbo \n \n ## t/lib-httpd/http-429.sh ##\n +@@\n + # Script to return HTTP 429 Too Many Requests responses for testing retry logic.\n + # Usage: /http_429///\n + #\n +-# The test-context is a unique identifier for each test to isolate state files.\n ++# The test-context is a unique identifier for each test to isolate state directories.\n + # The retry-after-value can be:\n + # - A number (e.g., \"1\", \"2\", \"100\") - sets Retry-After header to that many seconds\n + # - \"none\" - no Retry-After header\n @@ t/lib-httpd/http-429.sh: repo_path=\"${remaining#*/}\" # Get rest (repo path)\n # The repo name is the first component before any \"/\"\n repo_name=\"${repo_path%%/*}\"\n @@ t/lib-httpd/http-429.sh: repo_path=\"${remaining#*/}\" # Get rest (repo path)\n -# Use current directory (HTTPD_ROOT_PATH) for state file\n -# Create a safe filename from test_context, retry_after and repo_name\n -# This ensures all requests for the same test context share the same state file\n -+# Use current directory (HTTPD_ROOT_PATH) for state.\n -+# Create a safe name from test_context, retry_after and repo_name so that all\n -+# requests for the same test context share the same state.\n ++# Store state in the current directory (HTTPD_ROOT_PATH). Build a safe name\n ++# from test_context, retry_after, and repo_name, so that all requests for one\n ++# test context share the same state.\n safe_name=$(echo \"${test_context}-${retry_after}-${repo_name}\" | tr '/' '_' | tr -cd 'a-zA-Z0-9_-')\n -state_file=\"http-429-state-${safe_name}\"\n +state=\"http-429-state-${safe_name}\"\n \n -# Check if this is the first call (no state file exists)\n -if test -f \"$state_file\"\n -+# This endpoint returns 429 to the first request and forwards later ones to\n -+# git-http-backend, so the retry succeeds. Apache can run this CGI for several\n -+# requests at once, so a single atomic \"mkdir\" elects that first request: the\n -+# one whose mkdir succeeds returns 429 and leaves the directory behind as the\n -+# \"already rate-limited\" marker; every later request finds the directory (mkdir\n -+# fails) and is forwarded.\n ++# This endpoint returns 429 to the first request. It forwards every later\n ++# request to git-http-backend, so the retry succeeds. Apache can run this CGI\n ++# for several requests at the same time. A single atomic \"mkdir\" selects the\n ++# first request, because only one \"mkdir\" succeeds. That request returns 429\n ++# and leaves the directory as the \"already rate-limited\" marker. Every later\n ++# \"mkdir\" fails, so the endpoint forwards those requests.\n +#\n -+# \"permanent\" is the exception: it must return 429 to every request and never\n -+# succeed, so it skips the mkdir and records no state. A leftover directory\n -+# would make its own later requests find the marker and be forwarded, which is\n -+# exactly what \"permanent\" must not do.\n ++# \"permanent\" is the exception. It must return 429 to every request, so it\n ++# skips the \"mkdir\" and records no state. A leftover directory would let a\n ++# later \"permanent\" request find the marker. The endpoint would forward that\n ++# request, which \"permanent\" must not allow.\n +if test \"$retry_after\" != permanent && ! mkdir \"$state\" 2>/dev/null\n then\n \t# Already returned 429 once, forward to git-http-backend\n 3: f158e1f92e ! 3: 374d148f43 t/README: document writing concurrency-safe helpers\n @@ Metadata\n Author: Michael Montalbo \n \n ## Commit message ##\n - t/README: document writing concurrency-safe helpers\n + t/lib-httpd: document writing concurrency-safe CGI helpers\n \n - The apply-one-time-script.sh and http-429.sh fixes addressed the same\n - underlying problem: a test helper assuming it has exclusive access to a\n - file when the web server can run it for several requests at once. The\n - atomic idioms that avoid this are not specific to CGI or to HTTP, so\n - document them generally, alongside the other guidance for writing tests,\n - and leave a pointer from the lib-httpd helper list rather than a local\n - comment. The note covers the anti-pattern (a \"test -f\" then a separate\n - act) and the two safe operations (mkdir to elect a winner, rename to\n - consume a one-shot marker), citing Git's own lockfile machinery and\n - make_symlink() as precedent.\n + The apply-one-time-script.sh and http-429.sh fixes share a root cause: a\n + CGI helper assumed it had a file to itself, when Apache can run the\n + helper for several requests at once. Document the atomic idioms that\n + avoid this next to where lib-httpd.sh installs the CGI scripts, so the\n + advice is in front of anyone adding another one.\n \n - Signed-off-by: Michael Montalbo \n + The note describes the anti-pattern, a \"test -f\" check followed by a\n + separate action, and the two atomic alternatives these helpers now use:\n +\n + - \"mkdir\", which fails if the directory exists, to elect the first\n + request (http-429.sh); and\n + - \"rm\" without \"-f\", which fails once the file is gone, to consume a\n + one-shot marker (apply-one-time-script.sh).\n \n - ## t/README ##\n -@@ t/README: from the test harness library. At the end of the script, call\n - 'test_done'.\n - \n - \n -+Writing concurrency-safe helpers\n -+--------------------------------\n -+\n -+Some test code runs concurrently: a test may background work with '&',\n -+and the helper scripts installed for the web server (in t/lib-httpd) are\n -+run once per request, so the same script can execute for several\n -+requests at once. Such code cannot assume it has exclusive access to a\n -+file.\n -+\n -+When exactly one of several concurrent processes needs to \"win\" a\n -+decision, a single atomic filesystem operation can make it, rather than\n -+a check followed by a separate action. A \"test -f X\" then \"touch X\"\n -+(or \"rm X\") races: two processes can both pass the check before either\n -+acts. Two atomic operations avoid this:\n -+\n -+ - \"mkdir dir\", which fails if the directory already exists, so that\n -+ exactly one caller wins, electing a first or only request (see\n -+ t/lib-httpd/http-429.sh).\n -+\n -+ - \"mv src dst\" (rename), which fails if the source is gone, so that\n -+ exactly one caller consumes it, claiming a planted one-shot marker\n -+ (see t/lib-httpd/apply-one-time-script.sh).\n -+\n -+A \"$$\" suffix on per-request scratch files keeps concurrent invocations\n -+from clobbering each other's fixed-name files.\n -+\n -+This is a standard shell locking idiom, and the same reasoning behind\n -+Git's own lockfile machinery, which creates its lock with O_CREAT|O_EXCL,\n -+and make_symlink() in t/test-lib.sh, which uses an mkdir lock: an atomic\n -+operation whose failure indicates that another process got there first.\n -+\n -+\n - Test harness library\n - --------------------\n - \n + Signed-off-by: Michael Montalbo \n \n ## t/lib-httpd.sh ##\n @@ t/lib-httpd.sh: prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/proxy-passwd \"$HTTPD_ROOT_PATH\"\n -+\t# The web server can run any of these CGI scripts for two requests at\n -+\t# once; a helper that keeps state between requests must do so with an\n -+\t# atomic operation. See \"Writing concurrency-safe helpers\" in t/README.\n ++\t# Apache runs each of these CGI scripts once per request. Apache can run one\n ++\t# script for several requests at the same time. A helper that keeps state\n ++\t# between requests must update that state with one atomic operation. A check\n ++\t# and then a separate action is not safe: two requests can both pass the\n ++\t# check before either one acts. Test the exit status of one atomic operation\n ++\t# instead:\n ++\t# - \"mkdir dir\" fails if the directory exists, so only one request\n ++\t# succeeds. http-429.sh selects the first request this way.\n ++\t# - \"rm marker\" (without \"-f\") fails if the marker is gone, so only one\n ++\t# request consumes it. apply-one-time-script.sh claims its one-shot\n ++\t# marker this way.\n ++\t# A scratch file name includes the process ID ($$), so concurrent requests\n ++\t# do not overwrite each other's files.\n \tinstall_script incomplete-length-upload-pack-v2-http.sh\n \tinstall_script incomplete-body-upload-pack-v2-http.sh\n \tinstall_script error-no-report.sh\n\n-- \ngitgitgadget\n"},{"id":"550463","messageId":"862c4258e596e411063808a9a68d0bf4db454ebf.1786583137.git.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.v3.git.1786583137.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-13T01:05:34Z","receivedAt":"2026-08-13T01:05:41Z","isPatch":true,"body":"From: Michael Montalbo \n\napply-one-time-script.sh is a CGI helper that, when the file\n\"one-time-script\" is present, runs it to rewrite the git-http-backend\nresponse. If \"one-time-script\" generates a response that differs from\ngit-http-backend, the modified response is returned and\n\"one-time-script\" is deleted. Requests after the deletion return normal\ngit-http-backend responses.\n\nThe deletion is not safe under concurrency. The helper serves the\nmodified body first and deletes \"one-time-script\" only afterward, so a\nclient can issue its next request while the file still exists. Apache\nruns the CGI for both requests at once, for example when a partial fetch\nlazily fetches a missing promisor base while the first response is still\nin flight. Both requests find the file and try to run it; the first\ndeletes it; the second then fails to exec the now-missing file, produces\nno output, and the server returns HTTP 500:\n\n fatal: ... The requested URL returned error: 500\n fatal: could not fetch from promisor remote\n\nThis is the flaky failure of t5616.47 on the macOS CI runners.\n\nFix it by removing the file with \"rm\" only after the script has actually\nchanged the response. Because \"rm\" without \"-f\" fails once the file is\ngone, exactly one request removes it and serves the modified body. Any\nother request serves the unmodified body. Running the script more than\nonce is harmless; only its deletion is serialized, so exactly one\nrequest's modified response is ever served. Per-request scratch file\nnames keep concurrent runs from overwriting each other, and no path\nemits an empty response body.\n\nt5616.47 exercises the real code path but, being timing-dependent,\npasses against the buggy helper almost every time. Add t5567, which\ndrives the helper directly with a fake git-http-backend and forces the\noverlap with FIFOs; against the pre-fix helper it fails with the same\nshell error seen in the field:\n\n ./one-time-script: No such file or directory\n\nSigned-off-by: Michael Montalbo \n---\n t/lib-httpd/apply-one-time-script.sh | 50 +++++++++++----\n t/meson.build | 1 +\n t/t5567-one-time-script.sh | 96 ++++++++++++++++++++++++++++\n 3 files changed, 133 insertions(+), 14 deletions(-)\n create mode 100755 t/t5567-one-time-script.sh\n\ndiff --git a/t/lib-httpd/apply-one-time-script.sh b/t/lib-httpd/apply-one-time-script.sh\nindex b1682944e2..8ab97e882a 100644\n--- a/t/lib-httpd/apply-one-time-script.sh\n+++ b/t/lib-httpd/apply-one-time-script.sh\n@@ -6,21 +6,43 @@\n #\n # This can be used to simulate the effects of the repository changing in\n # between HTTP request-response pairs.\n-if test -f one-time-script\n-then\n-\tLC_ALL=C\n-\texport LC_ALL\n+#\n+# Apache can run this CGI for several requests at the same time. For example, a\n+# partial fetch lazily fetches a missing object while the first response is\n+# still in flight. To stay correct, the helper removes the marker only after\n+# the response changes, and only with \"rm\" (without \"-f\"). The \"rm\" fails for\n+# every request except the one that removes the marker first. That request\n+# serves the modified body. Every other request serves its response unchanged.\n+# No request emits an empty body, which Apache would report as HTTP 500.\n+#\n+# A scratch file name includes the process ID ($$), so concurrent requests do\n+# not overwrite each other's files.\n+#\n+# The helper can run one-time-script more than once. It consumes the marker\n+# when the response changes (the \"rm\" after \"cmp\"), not when it runs the\n+# script. A request whose response is not the target runs the script, finds no\n+# change, and leaves the marker for a later request. This is safe because the\n+# scripts are stateless filters over the captured response.\n \n-\t\"$GIT_EXEC_PATH/git-http-backend\" >out\n-\t./one-time-script out >out_modified\n+test -f one-time-script || exec \"$GIT_EXEC_PATH/git-http-backend\"\n \n-\tif cmp -s out out_modified\n-\tthen\n-\t\tcat out\n-\telse\n-\t\tcat out_modified\n-\t\trm one-time-script\n-\tfi\n+LC_ALL=C\n+export LC_ALL\n+\n+out=out.$$\n+modified=out-modified.$$\n+\"$GIT_EXEC_PATH/git-http-backend\" >\"$out\"\n+\n+# one-time-script can be gone here: a concurrent request may have consumed it\n+# since the \"test -f\" above. Then \"./one-time-script\" fails, the exit status\n+# selects the unmodified body, and \"2>/dev/null\" discards the expected\n+# \"no such file\" message.\n+if ./one-time-script \"$out\" 2>/dev/null >\"$modified\" &&\n+ ! cmp -s \"$out\" \"$modified\" &&\n+ rm one-time-script 2>/dev/null\n+then\n+\tcat \"$modified\"\n else\n-\t\"$GIT_EXEC_PATH/git-http-backend\"\n+\tcat \"$out\"\n fi\n+rm -f \"$out\" \"$modified\"\ndiff --git a/t/meson.build b/t/meson.build\nindex 3219264fe7..a118a4d719 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -707,6 +707,7 @@ integration_tests = [\n 't5564-http-proxy.sh',\n 't5565-push-multiple.sh',\n 't5566-push-group.sh',\n+ 't5567-one-time-script.sh',\n 't5570-git-daemon.sh',\n 't5571-pre-push-hook.sh',\n 't5572-pull-submodule.sh',\ndiff --git a/t/t5567-one-time-script.sh b/t/t5567-one-time-script.sh\nnew file mode 100755\nindex 0000000000..cd8e656005\n--- /dev/null\n+++ b/t/t5567-one-time-script.sh\n@@ -0,0 +1,96 @@\n+#!/bin/sh\n+\n+test_description='apply-one-time-script CGI helper is safe under concurrent requests'\n+\n+. ./test-lib.sh\n+\n+HELPER=\"$TEST_DIRECTORY/lib-httpd/apply-one-time-script.sh\"\n+\n+test_expect_success PIPE 'concurrent requests: one rewritten, one passed through, neither empty' '\n+\tmkdir workdir fakebin &&\n+\tENTERED=\"$PWD/entered\" &&\n+\tGATE=\"$PWD/gate\" &&\n+\texport ENTERED GATE &&\n+\tmkfifo \"$ENTERED\" \"$GATE\" &&\n+\n+\t# Stand in for git-http-backend. The modify role returns a response\n+\t# containing \"packfile\", which the one-time script rewrites. The\n+\t# passthrough role returns a response that is left untouched, but first\n+\t# announces that it has entered the helper and then blocks, so that it\n+\t# is still in flight when the modify role claims and removes the marker.\n+\twrite_script fakebin/git-http-backend <<-\\EOF &&\n+\tprintf \"Status: 200 OK\\r\\n\"\n+\tprintf \"Content-Type: application/x-git-result\\r\\n\"\n+\tprintf \"\\r\\n\"\n+\tif test \"$ROLE\" = modify\n+\tthen\n+\t\tprintf \"packfile\\n\"\n+\telse\n+\t\techo entered >\"$ENTERED\"\n+\t\tread -r released <\"$GATE\"\n+\t\tprintf \"refs\\n\"\n+\tfi\n+\tEOF\n+\n+\t# The transform that replace_packfile would install as one-time-script:\n+\t# rewrite responses that contain \"packfile\", leave the rest alone.\n+\twrite_script workdir/one-time-script <<-\\EOF &&\n+\tif grep packfile \"$1\" >/dev/null\n+\tthen\n+\t\tsed \"/packfile/q\" \"$1\" &&\n+\t\tprintf \"REPLACED\\n\"\n+\telse\n+\t\tcat \"$1\"\n+\tfi\n+\tEOF\n+\n+\tGIT_EXEC_PATH=\"$PWD/fakebin\" &&\n+\texport GIT_EXEC_PATH &&\n+\n+\t# Hold GATE open read-write on fd 9 for the duration, so releasing the\n+\t# passthrough request below cannot block even if that request has\n+\t# already exited (it keeps a reader on the FIFO).\n+\texec 9<>\"$GATE\" &&\n+\n+\t# Launch the passthrough request in the background. It enters the\n+\t# helper, signals us through ENTERED, then blocks on GATE inside the\n+\t# fake backend. The braces keep the && chain intact while backgrounding\n+\t# only the subshell, so \"wait\" can reap it by pid; kill it on any exit\n+\t# so a stray blocked child cannot hold the test output open and stall a\n+\t# reader such as prove.\n+\t{ (\n+\t\tcd workdir &&\n+\t\tROLE=passthrough sh \"$HELPER\" >../passthrough.out 2>../passthrough.err\n+\t) & } &&\n+\tpassthrough_pid=$! &&\n+\ttest_when_finished \"kill $passthrough_pid 2>/dev/null || :\" &&\n+\n+\t# Wait until the passthrough request is past the marker check.\n+\tread -r entered <\"$ENTERED\" &&\n+\n+\t# Run the modifying request to completion while the passthrough request\n+\t# is still blocked.\n+\t(\n+\t\tcd workdir &&\n+\t\tROLE=modify sh \"$HELPER\" >../modify.out 2>../modify.err\n+\t) &&\n+\n+\t# Release the passthrough request and let it finish. Ignore the helper\n+\t# exit status here so a broken helper is diagnosed by the assertions\n+\t# below rather than aborting the test.\n+\techo released >&9 &&\n+\t{ wait \"$passthrough_pid\" || :; } &&\n+\n+\t# Neither request may error out or produce an empty (HTTP 500) body,\n+\t# and each must have played its role: the modify request rewrote its\n+\t# response and the passthrough request came through untouched.\n+\ttest_must_be_empty passthrough.err &&\n+\ttest_must_be_empty modify.err &&\n+\ttest_grep \"Status: 200 OK\" passthrough.out &&\n+\ttest_grep \"Status: 200 OK\" modify.out &&\n+\ttest_grep REPLACED modify.out &&\n+\ttest_grep ! REPLACED passthrough.out &&\n+\ttest_grep refs passthrough.out\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"550464","messageId":"8ed22c02a192e10ab46c7df61e92a3669faaf25a.1786583137.git.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.v3.git.1786583137.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] t/lib-httpd: make http-429 first-request check atomic","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-13T01:05:35Z","receivedAt":"2026-08-13T01:05:42Z","isPatch":true,"body":"From: Michael Montalbo \n\nhttp-429.sh returns 429 to the first request for an endpoint and\nforwards later ones to git-http-backend so the retry succeeds. It\nremembers that it has already answered 429 by checking for a shared\nstate file with \"test -f\" and creating it with \"touch\".\n\nThat \"check-and-set\" is not atomic. Apache runs the CGI for several\nrequests at once, so two of them can pass the \"test -f\" before either\n\"touch\"es the file, and both then answer as the first request. The\nretry flow is mostly sequential, so this has not been observed to fail,\nbut the race is latent. Replace the check and the \"touch\" with a single\natomic \"mkdir\", which fails if the directory already exists, so exactly\none of the concurrent requests is rate-limited and the rest are\nforwarded.\n\nThe \"permanent\" mode needs one extra step, for correctness rather than\ntidiness. The marker means \"429 already served, now forward\", so it must\nnever be visible to a request that must itself return 429. Since\n\"permanent\" returns 429 to every request, it must leave no marker. The\noriginal did not manage this. It ran the \"touch\" unconditionally and\nremoved the file with \"rm -f\" in the \"permanent\" case, and that\n\"create-then-remove\" has the same racy window: a concurrent \"permanent\"\nrequest can see the marker before the \"rm -f\" and be wrongly forwarded.\nSkipping the \"mkdir\" entirely for \"permanent\" (the \"!= permanent\" guard)\nleaves no marker at all, so every \"permanent\" request rate-limits.\n\nThere is no regression test. The check and the set are adjacent commands\nwith nothing in between to synchronize on, so the overlap cannot be\nforced deterministically, only reproduced by chance; the fix is\npreventive.\n\nSigned-off-by: Michael Montalbo \n---\n t/lib-httpd/http-429.sh | 30 ++++++++++++++++++------------\n 1 file changed, 18 insertions(+), 12 deletions(-)\n\ndiff --git a/t/lib-httpd/http-429.sh b/t/lib-httpd/http-429.sh\nindex c97b16145b..904cdacbd0 100644\n--- a/t/lib-httpd/http-429.sh\n+++ b/t/lib-httpd/http-429.sh\n@@ -3,7 +3,7 @@\n # Script to return HTTP 429 Too Many Requests responses for testing retry logic.\n # Usage: /http_429///\n #\n-# The test-context is a unique identifier for each test to isolate state files.\n+# The test-context is a unique identifier for each test to isolate state directories.\n # The retry-after-value can be:\n # - A number (e.g., \"1\", \"2\", \"100\") - sets Retry-After header to that many seconds\n # - \"none\" - no Retry-After header\n@@ -26,14 +26,24 @@ repo_path=\"${remaining#*/}\" # Get rest (repo path)\n # The repo name is the first component before any \"/\"\n repo_name=\"${repo_path%%/*}\"\n \n-# Use current directory (HTTPD_ROOT_PATH) for state file\n-# Create a safe filename from test_context, retry_after and repo_name\n-# This ensures all requests for the same test context share the same state file\n+# Store state in the current directory (HTTPD_ROOT_PATH). Build a safe name\n+# from test_context, retry_after, and repo_name, so that all requests for one\n+# test context share the same state.\n safe_name=$(echo \"${test_context}-${retry_after}-${repo_name}\" | tr '/' '_' | tr -cd 'a-zA-Z0-9_-')\n-state_file=\"http-429-state-${safe_name}\"\n+state=\"http-429-state-${safe_name}\"\n \n-# Check if this is the first call (no state file exists)\n-if test -f \"$state_file\"\n+# This endpoint returns 429 to the first request. It forwards every later\n+# request to git-http-backend, so the retry succeeds. Apache can run this CGI\n+# for several requests at the same time. A single atomic \"mkdir\" selects the\n+# first request, because only one \"mkdir\" succeeds. That request returns 429\n+# and leaves the directory as the \"already rate-limited\" marker. Every later\n+# \"mkdir\" fails, so the endpoint forwards those requests.\n+#\n+# \"permanent\" is the exception. It must return 429 to every request, so it\n+# skips the \"mkdir\" and records no state. A leftover directory would let a\n+# later \"permanent\" request find the marker. The endpoint would forward that\n+# request, which \"permanent\" must not allow.\n+if test \"$retry_after\" != permanent && ! mkdir \"$state\" 2>/dev/null\n then\n \t# Already returned 429 once, forward to git-http-backend\n \t# Set PATH_INFO to just the repo path (without retry-after value)\n@@ -52,9 +62,6 @@ then\n \texec \"$GIT_EXEC_PATH/git-http-backend\"\n fi\n \n-# Mark that we've returned 429\n-touch \"$state_file\"\n-\n # Output HTTP 429 response\n printf \"Status: 429 Too Many Requests\\r\\n\"\n \n@@ -67,8 +74,7 @@ case \"$retry_after\" in\n \t\tprintf \"Retry-After: invalid-format-123abc\\r\\n\"\n \t\t;;\n \tpermanent)\n-\t\t# Always return 429, don't set state file for success\n-\t\trm -f \"$state_file\"\n+\t\t# Always return 429\n \t\tprintf \"Retry-After: 1\\r\\n\"\n \t\tprintf \"Content-Type: text/plain\\r\\n\"\n \t\tprintf \"\\r\\n\"\n-- \ngitgitgadget\n\n"},{"id":"550465","messageId":"374d148f43036077c31c5a55ddb1b59da4d3a923.1786583137.git.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.v3.git.1786583137.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] t/lib-httpd: document writing concurrency-safe CGI helpers","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-13T01:05:36Z","receivedAt":"2026-08-13T01:05:44Z","isPatch":true,"body":"From: Michael Montalbo \n\nThe apply-one-time-script.sh and http-429.sh fixes share a root cause: a\nCGI helper assumed it had a file to itself, when Apache can run the\nhelper for several requests at once. Document the atomic idioms that\navoid this next to where lib-httpd.sh installs the CGI scripts, so the\nadvice is in front of anyone adding another one.\n\nThe note describes the anti-pattern, a \"test -f\" check followed by a\nseparate action, and the two atomic alternatives these helpers now use:\n\n - \"mkdir\", which fails if the directory exists, to elect the first\n request (http-429.sh); and\n - \"rm\" without \"-f\", which fails once the file is gone, to consume a\n one-shot marker (apply-one-time-script.sh).\n\nSigned-off-by: Michael Montalbo \n---\n t/lib-httpd.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex fc646447d5..f26e1594ab 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -159,6 +159,19 @@ prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/proxy-passwd \"$HTTPD_ROOT_PATH\"\n+\t# Apache runs each of these CGI scripts once per request. Apache can run one\n+\t# script for several requests at the same time. A helper that keeps state\n+\t# between requests must update that state with one atomic operation. A check\n+\t# and then a separate action is not safe: two requests can both pass the\n+\t# check before either one acts. Test the exit status of one atomic operation\n+\t# instead:\n+\t# - \"mkdir dir\" fails if the directory exists, so only one request\n+\t# succeeds. http-429.sh selects the first request this way.\n+\t# - \"rm marker\" (without \"-f\") fails if the marker is gone, so only one\n+\t# request consumes it. apply-one-time-script.sh claims its one-shot\n+\t# marker this way.\n+\t# A scratch file name includes the process ID ($$), so concurrent requests\n+\t# do not overwrite each other's files.\n \tinstall_script incomplete-length-upload-pack-v2-http.sh\n \tinstall_script incomplete-body-upload-pack-v2-http.sh\n \tinstall_script error-no-report.sh\n-- \ngitgitgadget\n"},{"id":"551312","messageId":"xmqq1pbkfyb1.fsf@gitster.g","threadId":"65950","inReplyTo":"pull.2171.v3.git.1786583137.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/3] t/lib-httpd: make CGI test helpers concurrency-safe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-26T19:59:14Z","receivedAt":"2026-08-26T19:59:17Z","isPatch":true,"body":"\"Michael Montalbo via GitGitGadget\" writes:\n\n> * Patch 1 fixes apply-one-time-script.sh (the actual flake) and adds t5567,\n> which drives the helper directly with no web server so the overlap can be\n> forced deterministically.\n> * Patch 2 makes http-429.sh atomic.\n> * Patch 3 documents the atomic idioms next to where t/lib-httpd.sh installs\n> the CGI scripts, so the guidance is in front of anyone adding another\n> helper.\n>\n> Changes since v2:\n>\n> * Patch 1 now consumes the marker with a plain \"rm\" (without \"-f\") instead\n> of a rename. \"rm\" without \"-f\" already fails once the marker is gone,\n> which is the atomicity the helper needs. A new comment explains why the\n> helper discards the one-time script's stderr: a losing request can find\n> the marker already removed.\n>\n> * Patch 3 is now specific to the lib-httpd CGI helpers and lives beside\n> their install site in t/lib-httpd.sh, rather than as a general section in\n> t/README.\n>\n> * Reworded several helper comments and the patch 1 and 2 log messages for\n> clarity and to match the code; no behavior change.\n\nAfter giving a cursory review to the previous round, I was hoping\nthat somebody more clueful than I am about HTTP tests would lend an\neye or two to these patches, but nobody seems interested.\n\nAny takers?\n\nThanks.\n"},{"id":"551517","messageId":"apUqs8N3EnTFngyQ@pks.im","threadId":"65950","inReplyTo":"8ed22c02a192e10ab46c7df61e92a3669faaf25a.1786583137.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/3] t/lib-httpd: make http-429 first-request check atomic","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-31T07:18:11Z","receivedAt":"2026-08-31T07:18:20Z","isPatch":true,"body":"On Thu, Aug 13, 2026 at 01:05:35AM +0000, Michael Montalbo via GitGitGadget wrote:\n> From: Michael Montalbo \n> \n> http-429.sh returns 429 to the first request for an endpoint and\n> forwards later ones to git-http-backend so the retry succeeds. It\n> remembers that it has already answered 429 by checking for a shared\n> state file with \"test -f\" and creating it with \"touch\".\n> \n> That \"check-and-set\" is not atomic. Apache runs the CGI for several\n> requests at once, so two of them can pass the \"test -f\" before either\n> \"touch\"es the file, and both then answer as the first request. The\n> retry flow is mostly sequential, so this has not been observed to fail,\n> but the race is latent. Replace the check and the \"touch\" with a single\n> atomic \"mkdir\", which fails if the directory already exists, so exactly\n> one of the concurrent requests is rate-limited and the rest are\n> forwarded.\n> \n> The \"permanent\" mode needs one extra step, for correctness rather than\n> tidiness. The marker means \"429 already served, now forward\", so it must\n> never be visible to a request that must itself return 429. Since\n> \"permanent\" returns 429 to every request, it must leave no marker. The\n> original did not manage this. It ran the \"touch\" unconditionally and\n> removed the file with \"rm -f\" in the \"permanent\" case, and that\n> \"create-then-remove\" has the same racy window: a concurrent \"permanent\"\n> request can see the marker before the \"rm -f\" and be wrongly forwarded.\n> Skipping the \"mkdir\" entirely for \"permanent\" (the \"!= permanent\" guard)\n> leaves no marker at all, so every \"permanent\" request rate-limits.\n> \n> There is no regression test. The check and the set are adjacent commands\n> with nothing in between to synchronize on, so the overlap cannot be\n> forced deterministically, only reproduced by chance; the fix is\n> preventive.\n\nA lot of AI-fluff in this message that could have otherwise been much\nbriefer, but okay.\n\n> diff --git a/t/lib-httpd/http-429.sh b/t/lib-httpd/http-429.sh\n> index c97b16145b..904cdacbd0 100644\n> --- a/t/lib-httpd/http-429.sh\n> +++ b/t/lib-httpd/http-429.sh\n> @@ -26,14 +26,24 @@ repo_path=\"${remaining#*/}\" # Get rest (repo path)\n> # The repo name is the first component before any \"/\"\n> repo_name=\"${repo_path%%/*}\"\n> \n> -# Use current directory (HTTPD_ROOT_PATH) for state file\n> -# Create a safe filename from test_context, retry_after and repo_name\n> -# This ensures all requests for the same test context share the same state file\n> +# Store state in the current directory (HTTPD_ROOT_PATH). Build a safe name\n> +# from test_context, retry_after, and repo_name, so that all requests for one\n> +# test context share the same state.\n> safe_name=$(echo \"${test_context}-${retry_after}-${repo_name}\" | tr '/' '_' | tr -cd 'a-zA-Z0-9_-')\n> -state_file=\"http-429-state-${safe_name}\"\n> +state=\"http-429-state-${safe_name}\"\n> \n> -# Check if this is the first call (no state file exists)\n> -if test -f \"$state_file\"\n> +# This endpoint returns 429 to the first request. It forwards every later\n> +# request to git-http-backend, so the retry succeeds. Apache can run this CGI\n> +# for several requests at the same time. A single atomic \"mkdir\" selects the\n> +# first request, because only one \"mkdir\" succeeds. That request returns 429\n> +# and leaves the directory as the \"already rate-limited\" marker. Every later\n> +# \"mkdir\" fails, so the endpoint forwards those requests.\n> +#\n> +# \"permanent\" is the exception. It must return 429 to every request, so it\n> +# skips the \"mkdir\" and records no state. A leftover directory would let a\n> +# later \"permanent\" request find the marker. The endpoint would forward that\n> +# request, which \"permanent\" must not allow.\n> +if test \"$retry_after\" != permanent && ! mkdir \"$state\" 2>/dev/null\n> then\n> \t# Already returned 429 once, forward to git-http-backend\n> \t# Set PATH_INFO to just the repo path (without retry-after value)\n> @@ -52,9 +62,6 @@ then\n> \texec \"$GIT_EXEC_PATH/git-http-backend\"\n> fi\n> \n> -# Mark that we've returned 429\n> -touch \"$state_file\"\n> -\n> # Output HTTP 429 response\n> printf \"Status: 429 Too Many Requests\\r\\n\"\n> \n> @@ -67,8 +74,7 @@ case \"$retry_after\" in\n> \t\tprintf \"Retry-After: invalid-format-123abc\\r\\n\"\n> \t\t;;\n> \tpermanent)\n> -\t\t# Always return 429, don't set state file for success\n> -\t\trm -f \"$state_file\"\n> +\t\t# Always return 429\n> \t\tprintf \"Retry-After: 1\\r\\n\"\n> \t\tprintf \"Content-Type: text/plain\\r\\n\"\n> \t\tprintf \"\\r\\n\"\n\nThe changes themselves look sensible.\n\nPatrick\n"},{"id":"551518","messageId":"apUquYUS6AvR5clv@pks.im","threadId":"65950","inReplyTo":"374d148f43036077c31c5a55ddb1b59da4d3a923.1786583137.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/3] t/lib-httpd: document writing concurrency-safe CGI helpers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-31T07:18:17Z","receivedAt":"2026-08-31T07:18:24Z","isPatch":true,"body":"On Thu, Aug 13, 2026 at 01:05:36AM +0000, Michael Montalbo via GitGitGadget wrote:\n> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\n> index fc646447d5..f26e1594ab 100644\n> --- a/t/lib-httpd.sh\n> +++ b/t/lib-httpd.sh\n> @@ -159,6 +159,19 @@ prepare_httpd() {\n> \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n> \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n> \tcp \"$TEST_PATH\"/proxy-passwd \"$HTTPD_ROOT_PATH\"\n> +\t# Apache runs each of these CGI scripts once per request. Apache can run one\n> +\t# script for several requests at the same time. A helper that keeps state\n> +\t# between requests must update that state with one atomic operation. A check\n> +\t# and then a separate action is not safe: two requests can both pass the\n> +\t# check before either one acts. Test the exit status of one atomic operation\n> +\t# instead:\n> +\t# - \"mkdir dir\" fails if the directory exists, so only one request\n> +\t# succeeds. http-429.sh selects the first request this way.\n> +\t# - \"rm marker\" (without \"-f\") fails if the marker is gone, so only one\n> +\t# request consumes it. apply-one-time-script.sh claims its one-shot\n> +\t# marker this way.\n> +\t# A scratch file name includes the process ID ($$), so concurrent requests\n\nNit, not worth rerolling over: s/includes/should include/\n\nPatrick\n"},{"id":"551519","messageId":"apUqvjWbbZCRUS0n@pks.im","threadId":"65950","inReplyTo":"xmqq1pbkfyb1.fsf@gitster.g","subject":"Re: [PATCH v3 0/3] t/lib-httpd: make CGI test helpers concurrency-safe","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-31T07:18:22Z","receivedAt":"2026-08-31T07:18:28Z","isPatch":true,"body":"On Wed, Aug 26, 2026 at 12:59:14PM -0700, Junio C Hamano wrote:\n> \"Michael Montalbo via GitGitGadget\" writes:\n> \n> > * Patch 1 fixes apply-one-time-script.sh (the actual flake) and adds t5567,\n> > which drives the helper directly with no web server so the overlap can be\n> > forced deterministically.\n> > * Patch 2 makes http-429.sh atomic.\n> > * Patch 3 documents the atomic idioms next to where t/lib-httpd.sh installs\n> > the CGI scripts, so the guidance is in front of anyone adding another\n> > helper.\n> >\n> > Changes since v2:\n> >\n> > * Patch 1 now consumes the marker with a plain \"rm\" (without \"-f\") instead\n> > of a rename. \"rm\" without \"-f\" already fails once the marker is gone,\n> > which is the atomicity the helper needs. A new comment explains why the\n> > helper discards the one-time script's stderr: a losing request can find\n> > the marker already removed.\n> >\n> > * Patch 3 is now specific to the lib-httpd CGI helpers and lives beside\n> > their install site in t/lib-httpd.sh, rather than as a general section in\n> > t/README.\n> >\n> > * Reworded several helper comments and the patch 1 and 2 log messages for\n> > clarity and to match the code; no behavior change.\n> \n> After giving a cursory review to the previous round, I was hoping\n> that somebody more clueful than I am about HTTP tests would lend an\n> eye or two to these patches, but nobody seems interested.\n> \n> Any takers?\n\nI think this version is good enough. It's quite a bit puffed up by AI\ngenerated messages that are overly long and use lots of meaningless\njargon, but I don't think that's worth another reroll.\n\nPatrick\n"},{"id":"551563","messageId":"xmqq33vuz6lo.fsf@gitster.g","threadId":"65950","inReplyTo":"apUqs8N3EnTFngyQ@pks.im","subject":"Re: [PATCH v3 2/3] t/lib-httpd: make http-429 first-request check atomic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-31T14:50:59Z","receivedAt":"2026-08-31T14:51:02Z","isPatch":true,"body":"Patrick Steinhardt writes:\n\n> On Thu, Aug 13, 2026 at 01:05:35AM +0000, Michael Montalbo via GitGitGadget wrote:\n>> From: Michael Montalbo \n>> \n>> http-429.sh returns 429 to the first request for an endpoint and\n>> forwards later ones to git-http-backend so the retry succeeds. It\n>> remembers that it has already answered 429 by checking for a shared\n>> state file with \"test -f\" and creating it with \"touch\".\n>> \n>> That \"check-and-set\" is not atomic. Apache runs the CGI for several\n>> requests at once, so two of them can pass the \"test -f\" before either\n>> \"touch\"es the file, and both then answer as the first request. The\n>> retry flow is mostly sequential, so this has not been observed to fail,\n>> but the race is latent. Replace the check and the \"touch\" with a single\n>> atomic \"mkdir\", which fails if the directory already exists, so exactly\n>> one of the concurrent requests is rate-limited and the rest are\n>> forwarded.\n>> \n>> The \"permanent\" mode needs one extra step, for correctness rather than\n>> tidiness. The marker means \"429 already served, now forward\", so it must\n>> never be visible to a request that must itself return 429. Since\n>> \"permanent\" returns 429 to every request, it must leave no marker. The\n>> original did not manage this. It ran the \"touch\" unconditionally and\n>> removed the file with \"rm -f\" in the \"permanent\" case, and that\n>> \"create-then-remove\" has the same racy window: a concurrent \"permanent\"\n>> request can see the marker before the \"rm -f\" and be wrongly forwarded.\n>> Skipping the \"mkdir\" entirely for \"permanent\" (the \"!= permanent\" guard)\n>> leaves no marker at all, so every \"permanent\" request rate-limits.\n>> \n>> There is no regression test. The check and the set are adjacent commands\n>> with nothing in between to synchronize on, so the overlap cannot be\n>> forced deterministically, only reproduced by chance; the fix is\n>> preventive.\n>\n> A lot of AI-fluff in this message that could have otherwise been much\n> briefer, but okay.\n\nI too find it disturbing it that the messages from this author tends\nto contain material that triggers \"it may not be wrong, but is it\nrelevant?\" reactions. More does not mean better.\n\nThe above made me curious enough to ask a near-by Gemini to distill\nit down to quarter of the original length without losing essense of\nthe original.\n\n http-429.sh marks that a 429 response was served by creating a\n state file with \"test -f\" and \"touch\". This check-and-set\n sequence is not atomic and can race under concurrent Apache\n requests, causing multiple requests to claim first-arrival\n status.\n\n Replace the check and \"touch\" with an atomic \"mkdir\", which\n fails if the directory already exists. In \"permanent\" mode,\n skip the \"mkdir\" entirely so no state marker is ever created.\n\n Omit a regression test, as this concurrency window cannot be\n forced deterministically without artificial synchronization\n points.\n\nThis seems readable enough to me, but may still need some manual\nclean-up, but this experiment told me that \"A lot of AI-fluff\" is\nnot something users cannot avoid without some extra work.\n\nThanks.\n\n"},{"id":"551575","messageId":"CAC2Qwm+L01XZgys2NGtZwWfVapWmnqDbsevt3Z4WKpS9EoP65A@mail.gmail.com","threadId":"65950","inReplyTo":"apUqs8N3EnTFngyQ@pks.im","subject":"Re: [PATCH v3 2/3] t/lib-httpd: make http-429 first-request check atomic","fromName":"Michael Montalbo","fromEmail":"mmontalbo@gmail.com","sentAt":"2026-08-31T17:09:14Z","receivedAt":"2026-08-31T17:09:27Z","isPatch":true,"body":"On Mon, Aug 31, 2026 at 12:18 AM Patrick Steinhardt wrote:\n>\n> On Thu, Aug 13, 2026 at 01:05:35AM +0000, Michael Montalbo via GitGitGadget wrote:\n> > From: Michael Montalbo \n> >\n> > http-429.sh returns 429 to the first request for an endpoint and\n> > forwards later ones to git-http-backend so the retry succeeds. It\n> > remembers that it has already answered 429 by checking for a shared\n> > state file with \"test -f\" and creating it with \"touch\".\n...\n> > There is no regression test. The check and the set are adjacent commands\n> > with nothing in between to synchronize on, so the overlap cannot be\n> > forced deterministically, only reproduced by chance; the fix is\n> > preventive.\n>\n> A lot of AI-fluff in this message that could have otherwise been much\n> briefer, but okay.\n>\n\nYou are right. I will go through all the prose in the series and re-write it\nby hand. I apologize for giving you unnecessary AI-fluff to read and will\nnot do it again.\n"},{"id":"551585","messageId":"CAC2QwmJ2AgU0y77tmRhs=Ycx-CuWEzzfKsVMn98Wa1EUvHHsKw@mail.gmail.com","threadId":"65950","inReplyTo":"xmqq33vuz6lo.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] t/lib-httpd: make http-429 first-request check atomic","fromName":"Michael Montalbo","fromEmail":"mmontalbo@gmail.com","sentAt":"2026-08-31T17:31:53Z","receivedAt":"2026-08-31T17:32:06Z","isPatch":true,"body":"On Mon, Aug 31, 2026 at 7:51 AM Junio C Hamano wrote:\n>\n> I too find it disturbing it that the messages from this author tends\n> to contain material that triggers \"it may not be wrong, but is it\n> relevant?\" reactions. More does not mean better.\n>\n\nThank you for this feedback. I agree with it and will avoid relying on AI\nas I have to create and edit prose for documentation and cover\nletters.\n\n> The above made me curious enough to ask a near-by Gemini to distill\n> it down to quarter of the original length without losing essense of\n> the original.\n>\n> http-429.sh marks that a 429 response was served by creating a\n> state file with \"test -f\" and \"touch\". This check-and-set\n> sequence is not atomic and can race under concurrent Apache\n> requests, causing multiple requests to claim first-arrival\n> status.\n>\n> Replace the check and \"touch\" with an atomic \"mkdir\", which\n> fails if the directory already exists. In \"permanent\" mode,\n> skip the \"mkdir\" entirely so no state marker is ever created.\n>\n> Omit a regression test, as this concurrency window cannot be\n> forced deterministically without artificial synchronization\n> points.\n>\n> This seems readable enough to me, but may still need some manual\n> clean-up, but this experiment told me that \"A lot of AI-fluff\" is\n> not something users cannot avoid without some extra work.\n>\n\nI agree, even though I have spent a lot of time trying to \"copy-edit\" what\nis generated, the end result does tend to be verbose and include unnecessary\ndetail. Compared to what I start with based on my initial idea and generated\nrough draft, a lot has been edited away. However, I do think I have regretfully\navoided doing some of that extra work. Apologies for having you all read\nunnecessary AI-fluff, I will write prose for documentation and similar from\nscratch.\n"},{"id":"551611","messageId":"pull.2171.v4.git.1788222476.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.git.1783479584.gitgitgadget@gmail.com","subject":"[PATCH v4 0/3] t/lib-httpd: make CGI test helpers concurrency-safe","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-01T00:27:53Z","receivedAt":"2026-09-01T00:27:58Z","isPatch":true,"body":"t/lib-httpd.sh provides several helpers that can be invoked concurrently by\nApache while exercising tests. Currently, two of these helpers use state\nmanagement logic that fails under certain race conditions.\n\napply-one-time-script.sh is one of those test helpers. It executes a\n\"one-time-script\" responsible for modifying the response normally returned\nby git-http-backend. Sometimes a race between multiple concurrent requests\ncauses apply-one-time-script.sh to misbehave and return multiple modified\nresponses or an empty response that results in:\n\nfatal: ... The requested URL returned error: 500 fatal: could not fetch from\npromisor remote\n\nThis can be seen in the flaky failure of t5616.47 on the macOS CI\nrunners[1].\n\nFix this by chaining (&&) the logic for executing \"one-time-script\" with its\nremoval, rather than running them as separate actions. Add\nt/t5567-one-time-script.sh to verify this fix is effective.\n\nhttp-429.sh is the other helper whose state management logic can fail under\ncertain race conditions. However, these failures do not manifest themselves\ncurrently since http-429.sh is invoked sequentially.\n\nAs a preventive measure, fix http-429.sh's state management logic so it\nrelies on an atomic mkdir operation to mark that a 429 was returned rather\nthan separate \"test -f marker\", \"touch marker\", and \"rm -f marker\" actions\nto manage state. http-429.sh is not as straightforward to test as\napply-one-time-script.sh, which is why no regression test was added for the\nchange.\n\nFinally, document these patterns and anti-patterns in t/lib-httpd.sh for\nfuture developers.\n\nChanges since v3:\n\n * Rewrite all the prose in the series from scratch without AI to remove\n fluff.\n * Fix the lack of clarity around the actual fix applied to\n apply-one-time-script.sh, which ultimately has nothing to do with rm\n itself, but rather how rm is used in conjunction with the surrounding\n state management logic.\n * No logical behavior change.\n\n[1]\nhttps://github.com/gitgitgadget/git/actions/runs/28756172690/job/85263916762?pr=2169\n\nMichael Montalbo (3):\n t/lib-httpd: fix apply-one-time-script race under concurrent requests\n t/lib-httpd: make http-429 first-request check atomic\n t/lib-httpd: document writing concurrency-safe CGI helpers\n\n t/lib-httpd.sh | 11 ++++\n t/lib-httpd/apply-one-time-script.sh | 38 +++++++----\n t/lib-httpd/http-429.sh | 22 +++----\n t/meson.build | 1 +\n t/t5567-one-time-script.sh | 96 ++++++++++++++++++++++++++++\n 5 files changed, 142 insertions(+), 26 deletions(-)\n create mode 100755 t/t5567-one-time-script.sh\n\n\nbase-commit: 5b2471720c93ee30e5764a19f3d3b3ae9ec9712a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2171%2Fmmontalbo%2Fmm%2Flib-httpd-cgi-safe-proto-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/2171\n\nRange-diff vs v3:\n\n 1: 862c4258e5 ! 1: e202142f19 t/lib-httpd: fix apply-one-time-script race under concurrent requests\n @@ Metadata\n ## Commit message ##\n t/lib-httpd: fix apply-one-time-script race under concurrent requests\n \n - apply-one-time-script.sh is a CGI helper that, when the file\n - \"one-time-script\" is present, runs it to rewrite the git-http-backend\n - response. If \"one-time-script\" generates a response that differs from\n - git-http-backend, the modified response is returned and\n - \"one-time-script\" is deleted. Requests after the deletion return normal\n - git-http-backend responses.\n -\n - The deletion is not safe under concurrency. The helper serves the\n - modified body first and deletes \"one-time-script\" only afterward, so a\n - client can issue its next request while the file still exists. Apache\n - runs the CGI for both requests at once, for example when a partial fetch\n - lazily fetches a missing promisor base while the first response is still\n - in flight. Both requests find the file and try to run it; the first\n - deletes it; the second then fails to exec the now-missing file, produces\n - no output, and the server returns HTTP 500:\n + apply-one-time-script.sh is a test helper that executes a\n + \"one-time-script\" responsible for modifying the response normally\n + returned by git-http-backend. apply-one-time-script.sh should run\n + \"one-time-script\" once and return a modified response once. However,\n + sometimes a race between multiple concurrent requests causes\n + apply-one-time-script.sh to misbehave and return multiple modified\n + responses or an empty response that results in:\n \n fatal: ... The requested URL returned error: 500\n fatal: could not fetch from promisor remote\n \n - This is the flaky failure of t5616.47 on the macOS CI runners.\n -\n - Fix it by removing the file with \"rm\" only after the script has actually\n - changed the response. Because \"rm\" without \"-f\" fails once the file is\n - gone, exactly one request removes it and serves the modified body. Any\n - other request serves the unmodified body. Running the script more than\n - once is harmless; only its deletion is serialized, so exactly one\n - request's modified response is ever served. Per-request scratch file\n - names keep concurrent runs from overwriting each other, and no path\n - emits an empty response body.\n + This can be seen in the flaky failure of t5616.47 on the macOS CI\n + runners.\n \n - t5616.47 exercises the real code path but, being timing-dependent,\n - passes against the buggy helper almost every time. Add t5567, which\n - drives the helper directly with a fake git-http-backend and forces the\n - overlap with FIFOs; against the pre-fix helper it fails with the same\n - shell error seen in the field:\n + Fix the logic that checks if \"one-time-script\" has returned its modified\n + response by chaining \"rm one-time-script\" with its execution. This\n + ensures a racing script does not also have the opportunity to execute\n + \"one-time-script\".\n \n - ./one-time-script: No such file or directory\n + Add t/t5567-one-time-script.sh to verify the race is fixed. Implement a\n + stub \"git-http-backend\" that intentionally invokes a concurrent request,\n + and check that only one modified response is returned without error.\n \n Signed-off-by: Michael Montalbo \n \n @@ t/lib-httpd/apply-one-time-script.sh\n -then\n -\tLC_ALL=C\n -\texport LC_ALL\n -+#\n -+# Apache can run this CGI for several requests at the same time. For example, a\n -+# partial fetch lazily fetches a missing object while the first response is\n -+# still in flight. To stay correct, the helper removes the marker only after\n -+# the response changes, and only with \"rm\" (without \"-f\"). The \"rm\" fails for\n -+# every request except the one that removes the marker first. That request\n -+# serves the modified body. Every other request serves its response unchanged.\n -+# No request emits an empty body, which Apache would report as HTTP 500.\n -+#\n -+# A scratch file name includes the process ID ($$), so concurrent requests do\n -+# not overwrite each other's files.\n -+#\n -+# The helper can run one-time-script more than once. It consumes the marker\n -+# when the response changes (the \"rm\" after \"cmp\"), not when it runs the\n -+# script. A request whose response is not the target runs the script, finds no\n -+# change, and leaves the marker for a later request. This is safe because the\n -+# scripts are stateless filters over the captured response.\n ++test -f one-time-script || exec \"$GIT_EXEC_PATH/git-http-backend\"\n ++\n ++LC_ALL=C\n ++export LC_ALL\n \n -\t\"$GIT_EXEC_PATH/git-http-backend\" >out\n -\t./one-time-script out >out_modified\n -+test -f one-time-script || exec \"$GIT_EXEC_PATH/git-http-backend\"\n ++out=out.$$\n ++modified=out-modified.$$\n ++\"$GIT_EXEC_PATH/git-http-backend\" >\"$out\"\n \n -\tif cmp -s out out_modified\n -\tthen\n @@ t/lib-httpd/apply-one-time-script.sh\n -\t\tcat out_modified\n -\t\trm one-time-script\n -\tfi\n -+LC_ALL=C\n -+export LC_ALL\n -+\n -+out=out.$$\n -+modified=out-modified.$$\n -+\"$GIT_EXEC_PATH/git-http-backend\" >\"$out\"\n -+\n -+# one-time-script can be gone here: a concurrent request may have consumed it\n -+# since the \"test -f\" above. Then \"./one-time-script\" fails, the exit status\n -+# selects the unmodified body, and \"2>/dev/null\" discards the expected\n -+# \"no such file\" message.\n ++# Since Apache can execute this script for multiple requests\n ++# concurrently, we chain \"rm one-time-script\" with the logic\n ++# for generating a modified response. If the \"rm\" ran separately,\n ++# a concurrent request could pass the \"test -f\" above and\n ++# erroneously result in multiple modified responses or an empty\n ++# body depending on the race state.\n ++#\n ++# We discard stderr for ./one-time-script since it is possible\n ++# ./one-time-script has been removed already, which is expected\n ++# sometimes. In this case, the unmodified response will be returned.\n +if ./one-time-script \"$out\" 2>/dev/null >\"$modified\" &&\n + ! cmp -s \"$out\" \"$modified\" &&\n + rm one-time-script 2>/dev/null\n @@ t/t5567-one-time-script.sh (new)\n +\n +HELPER=\"$TEST_DIRECTORY/lib-httpd/apply-one-time-script.sh\"\n +\n -+test_expect_success PIPE 'concurrent requests: one rewritten, one passed through, neither empty' '\n ++test_expect_success PIPE 'helper only serves one rewritten response for concurrent requests' '\n +\tmkdir workdir fakebin &&\n +\tENTERED=\"$PWD/entered\" &&\n +\tGATE=\"$PWD/gate\" &&\n +\texport ENTERED GATE &&\n +\tmkfifo \"$ENTERED\" \"$GATE\" &&\n +\n -+\t# Stand in for git-http-backend. The modify role returns a response\n -+\t# containing \"packfile\", which the one-time script rewrites. The\n -+\t# passthrough role returns a response that is left untouched, but first\n -+\t# announces that it has entered the helper and then blocks, so that it\n -+\t# is still in flight when the modify role claims and removes the marker.\n ++\t# A stub git-http-backend that returns a response based on\n ++\t# $ROLE. For $ROLE = modify, return the response string\n ++\t# \"packfile\", which ends up being modified by the example\n ++\t# one-time-script below.\n ++\t#\n ++\t# Otherwise, run the branch returning a response that\n ++\t# should be passed through, and block until released\n ++\t# by \"read -r $GATE\".\n +\twrite_script fakebin/git-http-backend <<-\\EOF &&\n +\tprintf \"Status: 200 OK\\r\\n\"\n +\tprintf \"Content-Type: application/x-git-result\\r\\n\"\n @@ t/t5567-one-time-script.sh (new)\n +\tfi\n +\tEOF\n +\n -+\t# The transform that replace_packfile would install as one-time-script:\n -+\t# rewrite responses that contain \"packfile\", leave the rest alone.\n ++\t# An example one-time-script for apply-one-time-script\n ++\t# to execute. Checks for \"packfile\" in the response\n ++\t# that will be returned, and replaces it with a\n ++\t# modified response. Passes through responses without\n ++\t# \"packfile\" in them.\n +\twrite_script workdir/one-time-script <<-\\EOF &&\n +\tif grep packfile \"$1\" >/dev/null\n +\tthen\n @@ t/t5567-one-time-script.sh (new)\n +\tGIT_EXEC_PATH=\"$PWD/fakebin\" &&\n +\texport GIT_EXEC_PATH &&\n +\n -+\t# Hold GATE open read-write on fd 9 for the duration, so releasing the\n -+\t# passthrough request below cannot block even if that request has\n -+\t# already exited (it keeps a reader on the FIFO).\n ++\t# Ensure $GATE has a reader so the test does not block indefinitely if\n ++\t# the helper is buggy and \"echo released >&9\" below does not unblock\n ++\t# the unmodified response gate.\n +\texec 9<>\"$GATE\" &&\n +\n -+\t# Launch the passthrough request in the background. It enters the\n -+\t# helper, signals us through ENTERED, then blocks on GATE inside the\n -+\t# fake backend. The braces keep the && chain intact while backgrounding\n -+\t# only the subshell, so \"wait\" can reap it by pid; kill it on any exit\n -+\t# so a stray blocked child cannot hold the test output open and stall a\n -+\t# reader such as prove.\n ++\t# Launch the passthrough request in the background. Record its pid\n ++\t# so it can be killed when the test finishes if, for some reason, the\n ++\t# request stays blocked and would stall a test runner.\n +\t{ (\n +\t\tcd workdir &&\n +\t\tROLE=passthrough sh \"$HELPER\" >../passthrough.out 2>../passthrough.err\n @@ t/t5567-one-time-script.sh (new)\n +\tpassthrough_pid=$! &&\n +\ttest_when_finished \"kill $passthrough_pid 2>/dev/null || :\" &&\n +\n -+\t# Wait until the passthrough request is past the marker check.\n ++\t# Wait until the passthrough request is \"in-flight\" and paused\n ++\t# mid-response.\n +\tread -r entered <\"$ENTERED\" &&\n +\n -+\t# Run the modifying request to completion while the passthrough request\n -+\t# is still blocked.\n ++\t# Launch the request for a modified response while the passthrough\n ++\t# request is concurrently \"in-flight\" and paused.\n +\t(\n +\t\tcd workdir &&\n +\t\tROLE=modify sh \"$HELPER\" >../modify.out 2>../modify.err\n +\t) &&\n +\n -+\t# Release the passthrough request and let it finish. Ignore the helper\n -+\t# exit status here so a broken helper is diagnosed by the assertions\n -+\t# below rather than aborting the test.\n ++\t# Unblock the passthrough request, allowing git-http-backend to\n ++\t# complete its response.\n +\techo released >&9 &&\n +\t{ wait \"$passthrough_pid\" || :; } &&\n +\n -+\t# Neither request may error out or produce an empty (HTTP 500) body,\n -+\t# and each must have played its role: the modify request rewrote its\n -+\t# response and the passthrough request came through untouched.\n +\ttest_must_be_empty passthrough.err &&\n +\ttest_must_be_empty modify.err &&\n +\ttest_grep \"Status: 200 OK\" passthrough.out &&\n 2: 8ed22c02a1 ! 2: 79396d491f t/lib-httpd: make http-429 first-request check atomic\n @@ Metadata\n ## Commit message ##\n t/lib-httpd: make http-429 first-request check atomic\n \n - http-429.sh returns 429 to the first request for an endpoint and\n - forwards later ones to git-http-backend so the retry succeeds. It\n - remembers that it has already answered 429 by checking for a shared\n - state file with \"test -f\" and creating it with \"touch\".\n + http-429.sh is a helper for testing retry logic. It uses \"test -f\" to\n + check for the existence of a state file and later uses \"touch\" or\n + \"rm -f\" on that file to determine if it should return a 429. This method\n + of managing state can fail if the helper script is invoked concurrently.\n + However, this failure does not currently manifest itself since the\n + helper is invoked sequentially.\n \n - That \"check-and-set\" is not atomic. Apache runs the CGI for several\n - requests at once, so two of them can pass the \"test -f\" before either\n - \"touch\"es the file, and both then answer as the first request. The\n - retry flow is mostly sequential, so this has not been observed to fail,\n - but the race is latent. Replace the check and the \"touch\" with a single\n - atomic \"mkdir\", which fails if the directory already exists, so exactly\n - one of the concurrent requests is rate-limited and the rest are\n - forwarded.\n -\n - The \"permanent\" mode needs one extra step, for correctness rather than\n - tidiness. The marker means \"429 already served, now forward\", so it must\n - never be visible to a request that must itself return 429. Since\n - \"permanent\" returns 429 to every request, it must leave no marker. The\n - original did not manage this. It ran the \"touch\" unconditionally and\n - removed the file with \"rm -f\" in the \"permanent\" case, and that\n - \"create-then-remove\" has the same racy window: a concurrent \"permanent\"\n - request can see the marker before the \"rm -f\" and be wrongly forwarded.\n - Skipping the \"mkdir\" entirely for \"permanent\" (the \"!= permanent\" guard)\n - leaves no marker at all, so every \"permanent\" request rate-limits.\n -\n - There is no regression test. The check and the set are adjacent commands\n - with nothing in between to synchronize on, so the overlap cannot be\n - forced deterministically, only reproduced by chance; the fix is\n - preventive.\n + As a preventive measure, fix the state management logic so it relies on\n + an atomic mkdir operation to mark that a 429 was returned. When\n + $retry_after is \"permanent\", always return 429 now that we do not rely\n + on a state file that is \"touch\"ed and \"rm\"ed to indicate when to respond\n + with a 429.\n \n Signed-off-by: Michael Montalbo \n \n @@ t/lib-httpd/http-429.sh: repo_path=\"${remaining#*/}\" # Get rest (repo path)\n -# Use current directory (HTTPD_ROOT_PATH) for state file\n -# Create a safe filename from test_context, retry_after and repo_name\n -# This ensures all requests for the same test context share the same state file\n -+# Store state in the current directory (HTTPD_ROOT_PATH). Build a safe name\n -+# from test_context, retry_after, and repo_name, so that all requests for one\n -+# test context share the same state.\n ++# Use current directory (HTTPD_ROOT_PATH) to hold state directory\n ++# Create a safe directory name from test_context, retry_after and repo_name\n ++# This ensures all requests for the same test context share the same state directory\n safe_name=$(echo \"${test_context}-${retry_after}-${repo_name}\" | tr '/' '_' | tr -cd 'a-zA-Z0-9_-')\n -state_file=\"http-429-state-${safe_name}\"\n +state=\"http-429-state-${safe_name}\"\n \n -# Check if this is the first call (no state file exists)\n -if test -f \"$state_file\"\n -+# This endpoint returns 429 to the first request. It forwards every later\n -+# request to git-http-backend, so the retry succeeds. Apache can run this CGI\n -+# for several requests at the same time. A single atomic \"mkdir\" selects the\n -+# first request, because only one \"mkdir\" succeeds. That request returns 429\n -+# and leaves the directory as the \"already rate-limited\" marker. Every later\n -+# \"mkdir\" fails, so the endpoint forwards those requests.\n -+#\n -+# \"permanent\" is the exception. It must return 429 to every request, so it\n -+# skips the \"mkdir\" and records no state. A leftover directory would let a\n -+# later \"permanent\" request find the marker. The endpoint would forward that\n -+# request, which \"permanent\" must not allow.\n ++# Check if this is the first call (no state directory exists), or if\n ++# the retry-after-value is \"permanent\", which indicates a 429 must be\n ++# returned for every request (even if the state directory exists).\n +if test \"$retry_after\" != permanent && ! mkdir \"$state\" 2>/dev/null\n then\n \t# Already returned 429 once, forward to git-http-backend\n 3: 374d148f43 ! 3: d8d11ad246 t/lib-httpd: document writing concurrency-safe CGI helpers\n @@ Metadata\n ## Commit message ##\n t/lib-httpd: document writing concurrency-safe CGI helpers\n \n - The apply-one-time-script.sh and http-429.sh fixes share a root cause: a\n - CGI helper assumed it had a file to itself, when Apache can run the\n - helper for several requests at once. Document the atomic idioms that\n - avoid this next to where lib-httpd.sh installs the CGI scripts, so the\n - advice is in front of anyone adding another one.\n -\n - The note describes the anti-pattern, a \"test -f\" check followed by a\n - separate action, and the two atomic alternatives these helpers now use:\n -\n - - \"mkdir\", which fails if the directory exists, to elect the first\n - request (http-429.sh); and\n - - \"rm\" without \"-f\", which fails once the file is gone, to consume a\n - one-shot marker (apply-one-time-script.sh).\n + Update t/lib-httpd.sh to document the fixes applied to\n + apply-one-time-script.sh and http-429.sh for future developers working\n + on helper scripts. Add concrete examples of patterns and anti-patterns\n + that should be considered when handling state management.\n \n Signed-off-by: Michael Montalbo \n \n @@ t/lib-httpd.sh: prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/proxy-passwd \"$HTTPD_ROOT_PATH\"\n -+\t# Apache runs each of these CGI scripts once per request. Apache can run one\n -+\t# script for several requests at the same time. A helper that keeps state\n -+\t# between requests must update that state with one atomic operation. A check\n -+\t# and then a separate action is not safe: two requests can both pass the\n -+\t# check before either one acts. Test the exit status of one atomic operation\n -+\t# instead:\n -+\t# - \"mkdir dir\" fails if the directory exists, so only one request\n -+\t# succeeds. http-429.sh selects the first request this way.\n -+\t# - \"rm marker\" (without \"-f\") fails if the marker is gone, so only one\n -+\t# request consumes it. apply-one-time-script.sh claims its one-shot\n -+\t# marker this way.\n -+\t# A scratch file name includes the process ID ($$), so concurrent requests\n -+\t# do not overwrite each other's files.\n ++\t# Apache can run the following scripts concurrently per request. Make\n ++\t# sure any state management logic is resilient to race conditions.\n ++\t#\n ++\t# For example:\n ++\t# - use \"mkdir dir\" to ensure only one request \"succeeds\" under some\n ++\t# condition (see http-429.sh).\n ++\t# - chain (&&) atomic operations like \"rm marker\" (no -f) with the\n ++\t# logic that \"claims\" the marker instead of relying on a separate\n ++\t# \"test -f\" and \"rm marker\" check (see apply-one-time-script.sh).\n ++\t# - use scratch file names that include the process ID ($$), so\n ++\t# concurrent requests do not overwrite each other's state.\n \tinstall_script incomplete-length-upload-pack-v2-http.sh\n \tinstall_script incomplete-body-upload-pack-v2-http.sh\n \tinstall_script error-no-report.sh\n\n-- \ngitgitgadget\n"},{"id":"551612","messageId":"e202142f1999a57d485cae0d50a1a7c1afa50763.1788222476.git.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.v4.git.1788222476.gitgitgadget@gmail.com","subject":"[PATCH v4 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-01T00:27:54Z","receivedAt":"2026-09-01T00:28:00Z","isPatch":true,"body":"From: Michael Montalbo \n\napply-one-time-script.sh is a test helper that executes a\n\"one-time-script\" responsible for modifying the response normally\nreturned by git-http-backend. apply-one-time-script.sh should run\n\"one-time-script\" once and return a modified response once. However,\nsometimes a race between multiple concurrent requests causes\napply-one-time-script.sh to misbehave and return multiple modified\nresponses or an empty response that results in:\n\n fatal: ... The requested URL returned error: 500\n fatal: could not fetch from promisor remote\n\nThis can be seen in the flaky failure of t5616.47 on the macOS CI\nrunners.\n\nFix the logic that checks if \"one-time-script\" has returned its modified\nresponse by chaining \"rm one-time-script\" with its execution. This\nensures a racing script does not also have the opportunity to execute\n\"one-time-script\".\n\nAdd t/t5567-one-time-script.sh to verify the race is fixed. Implement a\nstub \"git-http-backend\" that intentionally invokes a concurrent request,\nand check that only one modified response is returned without error.\n\nSigned-off-by: Michael Montalbo \n---\n t/lib-httpd/apply-one-time-script.sh | 38 +++++++----\n t/meson.build | 1 +\n t/t5567-one-time-script.sh | 96 ++++++++++++++++++++++++++++\n 3 files changed, 121 insertions(+), 14 deletions(-)\n create mode 100755 t/t5567-one-time-script.sh\n\ndiff --git a/t/lib-httpd/apply-one-time-script.sh b/t/lib-httpd/apply-one-time-script.sh\nindex b1682944e2..eac21a3a8e 100644\n--- a/t/lib-httpd/apply-one-time-script.sh\n+++ b/t/lib-httpd/apply-one-time-script.sh\n@@ -6,21 +6,31 @@\n #\n # This can be used to simulate the effects of the repository changing in\n # between HTTP request-response pairs.\n-if test -f one-time-script\n-then\n-\tLC_ALL=C\n-\texport LC_ALL\n+test -f one-time-script || exec \"$GIT_EXEC_PATH/git-http-backend\"\n+\n+LC_ALL=C\n+export LC_ALL\n \n-\t\"$GIT_EXEC_PATH/git-http-backend\" >out\n-\t./one-time-script out >out_modified\n+out=out.$$\n+modified=out-modified.$$\n+\"$GIT_EXEC_PATH/git-http-backend\" >\"$out\"\n \n-\tif cmp -s out out_modified\n-\tthen\n-\t\tcat out\n-\telse\n-\t\tcat out_modified\n-\t\trm one-time-script\n-\tfi\n+# Since Apache can execute this script for multiple requests\n+# concurrently, we chain \"rm one-time-script\" with the logic\n+# for generating a modified response. If the \"rm\" ran separately,\n+# a concurrent request could pass the \"test -f\" above and\n+# erroneously result in multiple modified responses or an empty\n+# body depending on the race state.\n+#\n+# We discard stderr for ./one-time-script since it is possible\n+# ./one-time-script has been removed already, which is expected\n+# sometimes. In this case, the unmodified response will be returned.\n+if ./one-time-script \"$out\" 2>/dev/null >\"$modified\" &&\n+ ! cmp -s \"$out\" \"$modified\" &&\n+ rm one-time-script 2>/dev/null\n+then\n+\tcat \"$modified\"\n else\n-\t\"$GIT_EXEC_PATH/git-http-backend\"\n+\tcat \"$out\"\n fi\n+rm -f \"$out\" \"$modified\"\ndiff --git a/t/meson.build b/t/meson.build\nindex a25f37d2f5..e4d0b6dc4e 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -716,6 +716,7 @@ integration_tests = [\n 't5564-http-proxy.sh',\n 't5565-push-multiple.sh',\n 't5566-push-group.sh',\n+ 't5567-one-time-script.sh',\n 't5570-git-daemon.sh',\n 't5571-pre-push-hook.sh',\n 't5572-pull-submodule.sh',\ndiff --git a/t/t5567-one-time-script.sh b/t/t5567-one-time-script.sh\nnew file mode 100755\nindex 0000000000..a8429ef3c3\n--- /dev/null\n+++ b/t/t5567-one-time-script.sh\n@@ -0,0 +1,96 @@\n+#!/bin/sh\n+\n+test_description='apply-one-time-script CGI helper is safe under concurrent requests'\n+\n+. ./test-lib.sh\n+\n+HELPER=\"$TEST_DIRECTORY/lib-httpd/apply-one-time-script.sh\"\n+\n+test_expect_success PIPE 'helper only serves one rewritten response for concurrent requests' '\n+\tmkdir workdir fakebin &&\n+\tENTERED=\"$PWD/entered\" &&\n+\tGATE=\"$PWD/gate\" &&\n+\texport ENTERED GATE &&\n+\tmkfifo \"$ENTERED\" \"$GATE\" &&\n+\n+\t# A stub git-http-backend that returns a response based on\n+\t# $ROLE. For $ROLE = modify, return the response string\n+\t# \"packfile\", which ends up being modified by the example\n+\t# one-time-script below.\n+\t#\n+\t# Otherwise, run the branch returning a response that\n+\t# should be passed through, and block until released\n+\t# by \"read -r $GATE\".\n+\twrite_script fakebin/git-http-backend <<-\\EOF &&\n+\tprintf \"Status: 200 OK\\r\\n\"\n+\tprintf \"Content-Type: application/x-git-result\\r\\n\"\n+\tprintf \"\\r\\n\"\n+\tif test \"$ROLE\" = modify\n+\tthen\n+\t\tprintf \"packfile\\n\"\n+\telse\n+\t\techo entered >\"$ENTERED\"\n+\t\tread -r released <\"$GATE\"\n+\t\tprintf \"refs\\n\"\n+\tfi\n+\tEOF\n+\n+\t# An example one-time-script for apply-one-time-script\n+\t# to execute. Checks for \"packfile\" in the response\n+\t# that will be returned, and replaces it with a\n+\t# modified response. Passes through responses without\n+\t# \"packfile\" in them.\n+\twrite_script workdir/one-time-script <<-\\EOF &&\n+\tif grep packfile \"$1\" >/dev/null\n+\tthen\n+\t\tsed \"/packfile/q\" \"$1\" &&\n+\t\tprintf \"REPLACED\\n\"\n+\telse\n+\t\tcat \"$1\"\n+\tfi\n+\tEOF\n+\n+\tGIT_EXEC_PATH=\"$PWD/fakebin\" &&\n+\texport GIT_EXEC_PATH &&\n+\n+\t# Ensure $GATE has a reader so the test does not block indefinitely if\n+\t# the helper is buggy and \"echo released >&9\" below does not unblock\n+\t# the unmodified response gate.\n+\texec 9<>\"$GATE\" &&\n+\n+\t# Launch the passthrough request in the background. Record its pid\n+\t# so it can be killed when the test finishes if, for some reason, the\n+\t# request stays blocked and would stall a test runner.\n+\t{ (\n+\t\tcd workdir &&\n+\t\tROLE=passthrough sh \"$HELPER\" >../passthrough.out 2>../passthrough.err\n+\t) & } &&\n+\tpassthrough_pid=$! &&\n+\ttest_when_finished \"kill $passthrough_pid 2>/dev/null || :\" &&\n+\n+\t# Wait until the passthrough request is \"in-flight\" and paused\n+\t# mid-response.\n+\tread -r entered <\"$ENTERED\" &&\n+\n+\t# Launch the request for a modified response while the passthrough\n+\t# request is concurrently \"in-flight\" and paused.\n+\t(\n+\t\tcd workdir &&\n+\t\tROLE=modify sh \"$HELPER\" >../modify.out 2>../modify.err\n+\t) &&\n+\n+\t# Unblock the passthrough request, allowing git-http-backend to\n+\t# complete its response.\n+\techo released >&9 &&\n+\t{ wait \"$passthrough_pid\" || :; } &&\n+\n+\ttest_must_be_empty passthrough.err &&\n+\ttest_must_be_empty modify.err &&\n+\ttest_grep \"Status: 200 OK\" passthrough.out &&\n+\ttest_grep \"Status: 200 OK\" modify.out &&\n+\ttest_grep REPLACED modify.out &&\n+\ttest_grep ! REPLACED passthrough.out &&\n+\ttest_grep refs passthrough.out\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"551613","messageId":"79396d491fe15c94a4e4c079d1109b425dcf966a.1788222476.git.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.v4.git.1788222476.gitgitgadget@gmail.com","subject":"[PATCH v4 2/3] t/lib-httpd: make http-429 first-request check atomic","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-01T00:27:55Z","receivedAt":"2026-09-01T00:28:01Z","isPatch":true,"body":"From: Michael Montalbo \n\nhttp-429.sh is a helper for testing retry logic. It uses \"test -f\" to\ncheck for the existence of a state file and later uses \"touch\" or\n\"rm -f\" on that file to determine if it should return a 429. This method\nof managing state can fail if the helper script is invoked concurrently.\nHowever, this failure does not currently manifest itself since the\nhelper is invoked sequentially.\n\nAs a preventive measure, fix the state management logic so it relies on\nan atomic mkdir operation to mark that a 429 was returned. When\n$retry_after is \"permanent\", always return 429 now that we do not rely\non a state file that is \"touch\"ed and \"rm\"ed to indicate when to respond\nwith a 429.\n\nSigned-off-by: Michael Montalbo \n---\n t/lib-httpd/http-429.sh | 22 ++++++++++------------\n 1 file changed, 10 insertions(+), 12 deletions(-)\n\ndiff --git a/t/lib-httpd/http-429.sh b/t/lib-httpd/http-429.sh\nindex c97b16145b..1a5d7987db 100644\n--- a/t/lib-httpd/http-429.sh\n+++ b/t/lib-httpd/http-429.sh\n@@ -3,7 +3,7 @@\n # Script to return HTTP 429 Too Many Requests responses for testing retry logic.\n # Usage: /http_429///\n #\n-# The test-context is a unique identifier for each test to isolate state files.\n+# The test-context is a unique identifier for each test to isolate state directories.\n # The retry-after-value can be:\n # - A number (e.g., \"1\", \"2\", \"100\") - sets Retry-After header to that many seconds\n # - \"none\" - no Retry-After header\n@@ -26,14 +26,16 @@ repo_path=\"${remaining#*/}\" # Get rest (repo path)\n # The repo name is the first component before any \"/\"\n repo_name=\"${repo_path%%/*}\"\n \n-# Use current directory (HTTPD_ROOT_PATH) for state file\n-# Create a safe filename from test_context, retry_after and repo_name\n-# This ensures all requests for the same test context share the same state file\n+# Use current directory (HTTPD_ROOT_PATH) to hold state directory\n+# Create a safe directory name from test_context, retry_after and repo_name\n+# This ensures all requests for the same test context share the same state directory\n safe_name=$(echo \"${test_context}-${retry_after}-${repo_name}\" | tr '/' '_' | tr -cd 'a-zA-Z0-9_-')\n-state_file=\"http-429-state-${safe_name}\"\n+state=\"http-429-state-${safe_name}\"\n \n-# Check if this is the first call (no state file exists)\n-if test -f \"$state_file\"\n+# Check if this is the first call (no state directory exists), or if\n+# the retry-after-value is \"permanent\", which indicates a 429 must be\n+# returned for every request (even if the state directory exists).\n+if test \"$retry_after\" != permanent && ! mkdir \"$state\" 2>/dev/null\n then\n \t# Already returned 429 once, forward to git-http-backend\n \t# Set PATH_INFO to just the repo path (without retry-after value)\n@@ -52,9 +54,6 @@ then\n \texec \"$GIT_EXEC_PATH/git-http-backend\"\n fi\n \n-# Mark that we've returned 429\n-touch \"$state_file\"\n-\n # Output HTTP 429 response\n printf \"Status: 429 Too Many Requests\\r\\n\"\n \n@@ -67,8 +66,7 @@ case \"$retry_after\" in\n \t\tprintf \"Retry-After: invalid-format-123abc\\r\\n\"\n \t\t;;\n \tpermanent)\n-\t\t# Always return 429, don't set state file for success\n-\t\trm -f \"$state_file\"\n+\t\t# Always return 429\n \t\tprintf \"Retry-After: 1\\r\\n\"\n \t\tprintf \"Content-Type: text/plain\\r\\n\"\n \t\tprintf \"\\r\\n\"\n-- \ngitgitgadget\n\n"},{"id":"551614","messageId":"d8d11ad246b2e5ca73ea131e908d74111bb0fcf9.1788222476.git.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.v4.git.1788222476.gitgitgadget@gmail.com","subject":"[PATCH v4 3/3] t/lib-httpd: document writing concurrency-safe CGI helpers","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-01T00:27:56Z","receivedAt":"2026-09-01T00:28:03Z","isPatch":true,"body":"From: Michael Montalbo \n\nUpdate t/lib-httpd.sh to document the fixes applied to\napply-one-time-script.sh and http-429.sh for future developers working\non helper scripts. Add concrete examples of patterns and anti-patterns\nthat should be considered when handling state management.\n\nSigned-off-by: Michael Montalbo \n---\n t/lib-httpd.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex a216e5376f..8ca09fe85b 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -159,6 +159,17 @@ prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/proxy-passwd \"$HTTPD_ROOT_PATH\"\n+\t# Apache can run the following scripts concurrently per request. Make\n+\t# sure any state management logic is resilient to race conditions.\n+\t#\n+\t# For example:\n+\t# - use \"mkdir dir\" to ensure only one request \"succeeds\" under some\n+\t# condition (see http-429.sh).\n+\t# - chain (&&) atomic operations like \"rm marker\" (no -f) with the\n+\t# logic that \"claims\" the marker instead of relying on a separate\n+\t# \"test -f\" and \"rm marker\" check (see apply-one-time-script.sh).\n+\t# - use scratch file names that include the process ID ($$), so\n+\t# concurrent requests do not overwrite each other's state.\n \tinstall_script incomplete-length-upload-pack-v2-http.sh\n \tinstall_script incomplete-body-upload-pack-v2-http.sh\n \tinstall_script error-no-report.sh\n-- \ngitgitgadget\n"},{"id":"551658","messageId":"apa0N7VNNkcKurbi@pks.im","threadId":"65950","inReplyTo":"d8d11ad246b2e5ca73ea131e908d74111bb0fcf9.1788222476.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 3/3] t/lib-httpd: document writing concurrency-safe CGI helpers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-09-01T11:17:11Z","receivedAt":"2026-09-01T11:17:20Z","isPatch":true,"body":"On Tue, Sep 01, 2026 at 12:27:56AM +0000, Michael Montalbo via GitGitGadget wrote:\n> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\n> index a216e5376f..8ca09fe85b 100644\n> --- a/t/lib-httpd.sh\n> +++ b/t/lib-httpd.sh\n> @@ -159,6 +159,17 @@ prepare_httpd() {\n> \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n> \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n> \tcp \"$TEST_PATH\"/proxy-passwd \"$HTTPD_ROOT_PATH\"\n> +\t# Apache can run the following scripts concurrently per request. Make\n> +\t# sure any state management logic is resilient to race conditions.\n> +\t#\n> +\t# For example:\n> +\t# - use \"mkdir dir\" to ensure only one request \"succeeds\" under some\n> +\t# condition (see http-429.sh).\n> +\t# - chain (&&) atomic operations like \"rm marker\" (no -f) with the\n> +\t# logic that \"claims\" the marker instead of relying on a separate\n\nNit: I would have written \"with the logic that is guarded by the marker\"\ninstead of \"claims\".\n\n> +\t# \"test -f\" and \"rm marker\" check (see apply-one-time-script.sh).\n> +\t# - use scratch file names that include the process ID ($$), so\n> +\t# concurrent requests do not overwrite each other's state.\n> \tinstall_script incomplete-length-upload-pack-v2-http.sh\n> \tinstall_script incomplete-body-upload-pack-v2-http.sh\n> \tinstall_script error-no-report.sh\n\nOther than that the whole series reads a lot better now, thanks.\n\nPatrick\n"},{"id":"551674","messageId":"CAC2Qwm+dOZedmzzui5TSKJ1FNEiDypwUwVqGBgttG_OvDWQBBg@mail.gmail.com","threadId":"65950","inReplyTo":"apa0N7VNNkcKurbi@pks.im","subject":"Re: [PATCH v4 3/3] t/lib-httpd: document writing concurrency-safe CGI helpers","fromName":"Michael Montalbo","fromEmail":"mmontalbo@gmail.com","sentAt":"2026-09-01T14:28:38Z","receivedAt":"2026-09-01T14:28:51Z","isPatch":true,"body":"On Tue, Sep 1, 2026 at 4:17 AM Patrick Steinhardt wrote:\n>\n> On Tue, Sep 01, 2026 at 12:27:56AM +0000, Michael Montalbo via GitGitGadget wrote:\n> > diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\n> > index a216e5376f..8ca09fe85b 100644\n> > --- a/t/lib-httpd.sh\n> > +++ b/t/lib-httpd.sh\n> > @@ -159,6 +159,17 @@ prepare_httpd() {\n> > mkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n> > cp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n> > cp \"$TEST_PATH\"/proxy-passwd \"$HTTPD_ROOT_PATH\"\n> > + # Apache can run the following scripts concurrently per request. Make\n> > + # sure any state management logic is resilient to race conditions.\n> > + #\n> > + # For example:\n> > + # - use \"mkdir dir\" to ensure only one request \"succeeds\" under some\n> > + # condition (see http-429.sh).\n> > + # - chain (&&) atomic operations like \"rm marker\" (no -f) with the\n> > + # logic that \"claims\" the marker instead of relying on a separate\n>\n> Nit: I would have written \"with the logic that is guarded by the marker\"\n> instead of \"claims\".\n>\n\nThat makes more sense, the current version is circular (rm is the logic doing\nthe claiming). Will fix.\n\n> > + # \"test -f\" and \"rm marker\" check (see apply-one-time-script.sh).\n> > + # - use scratch file names that include the process ID ($$), so\n> > + # concurrent requests do not overwrite each other's state.\n> > install_script incomplete-length-upload-pack-v2-http.sh\n> > install_script incomplete-body-upload-pack-v2-http.sh\n> > install_script error-no-report.sh\n>\n> Other than that the whole series reads a lot better now, thanks.\n>\n\nThank you for the call out and taking another look. I really appreciate your\nfeedback!\n"},{"id":"551681","messageId":"pull.2171.v5.git.1788277983.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.git.1783479584.gitgitgadget@gmail.com","subject":"[PATCH v5 0/3] t/lib-httpd: make CGI test helpers concurrency-safe","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-01T15:53:00Z","receivedAt":"2026-09-01T15:53:06Z","isPatch":true,"body":"t/lib-httpd.sh provides several helpers that can be invoked concurrently by\nApache while exercising tests. Currently, two of these helpers use state\nmanagement logic that fails under certain race conditions.\n\napply-one-time-script.sh is one of those test helpers. It executes a\n\"one-time-script\" responsible for modifying the response normally returned\nby git-http-backend. Sometimes a race between multiple concurrent requests\ncauses apply-one-time-script.sh to misbehave and return multiple modified\nresponses or an empty response that results in:\n\nfatal: ... The requested URL returned error: 500 fatal: could not fetch from\npromisor remote\n\nThis can be seen in the flaky failure of t5616.47 on the macOS CI\nrunners[1].\n\nFix this by chaining (&&) the logic for executing \"one-time-script\" with its\nremoval, rather than running them as separate actions. Add\nt/t5567-one-time-script.sh to verify this fix is effective.\n\nhttp-429.sh is the other helper whose state management logic can fail under\ncertain race conditions. However, these failures do not manifest themselves\ncurrently since http-429.sh is invoked sequentially.\n\nAs a preventive measure, fix http-429.sh's state management logic so it\nrelies on an atomic mkdir operation to mark that a 429 was returned rather\nthan separate \"test -f marker\", \"touch marker\", and \"rm -f marker\" actions\nto manage state. http-429.sh is not as straightforward to test as\napply-one-time-script.sh, which is why no regression test was added for the\nchange.\n\nFinally, document these patterns and anti-patterns in t/lib-httpd.sh for\nfuture developers.\n\nChanges since v4:\n\n * Reword advice about chaining (&&) atomic operations like rm so it refers\n to chaining with \"the logic guarded by the marker\" instead of \"the logic\n that claims the marker\" since the latter is circular and inaccurate\n (atomic operations like rm are the logic that claims markers).\n\n[1]\nhttps://github.com/gitgitgadget/git/actions/runs/28756172690/job/85263916762?pr=2169\n\nMichael Montalbo (3):\n t/lib-httpd: fix apply-one-time-script race under concurrent requests\n t/lib-httpd: make http-429 first-request check atomic\n t/lib-httpd: document writing concurrency-safe CGI helpers\n\n t/lib-httpd.sh | 12 ++++\n t/lib-httpd/apply-one-time-script.sh | 38 +++++++----\n t/lib-httpd/http-429.sh | 22 +++----\n t/meson.build | 1 +\n t/t5567-one-time-script.sh | 96 ++++++++++++++++++++++++++++\n 5 files changed, 143 insertions(+), 26 deletions(-)\n create mode 100755 t/t5567-one-time-script.sh\n\n\nbase-commit: 5b2471720c93ee30e5764a19f3d3b3ae9ec9712a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2171%2Fmmontalbo%2Fmm%2Flib-httpd-cgi-safe-proto-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2171/mmontalbo/mm/lib-httpd-cgi-safe-proto-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/2171\n\nRange-diff vs v4:\n\n 1: e202142f19 = 1: e202142f19 t/lib-httpd: fix apply-one-time-script race under concurrent requests\n 2: 79396d491f = 2: 79396d491f t/lib-httpd: make http-429 first-request check atomic\n 3: d8d11ad246 ! 3: 75a184ca09 t/lib-httpd: document writing concurrency-safe CGI helpers\n @@ t/lib-httpd.sh: prepare_httpd() {\n +\t# - use \"mkdir dir\" to ensure only one request \"succeeds\" under some\n +\t# condition (see http-429.sh).\n +\t# - chain (&&) atomic operations like \"rm marker\" (no -f) with the\n -+\t# logic that \"claims\" the marker instead of relying on a separate\n -+\t# \"test -f\" and \"rm marker\" check (see apply-one-time-script.sh).\n ++\t# logic that is guarded by the marker instead of relying on a\n ++\t# separate \"test -f\" and \"rm marker\" check\n ++\t# (see apply-one-time-script.sh).\n +\t# - use scratch file names that include the process ID ($$), so\n +\t# concurrent requests do not overwrite each other's state.\n \tinstall_script incomplete-length-upload-pack-v2-http.sh\n\n-- \ngitgitgadget\n"},{"id":"551682","messageId":"e202142f1999a57d485cae0d50a1a7c1afa50763.1788277983.git.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.v5.git.1788277983.gitgitgadget@gmail.com","subject":"[PATCH v5 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-01T15:53:01Z","receivedAt":"2026-09-01T15:53:07Z","isPatch":true,"body":"From: Michael Montalbo \n\napply-one-time-script.sh is a test helper that executes a\n\"one-time-script\" responsible for modifying the response normally\nreturned by git-http-backend. apply-one-time-script.sh should run\n\"one-time-script\" once and return a modified response once. However,\nsometimes a race between multiple concurrent requests causes\napply-one-time-script.sh to misbehave and return multiple modified\nresponses or an empty response that results in:\n\n fatal: ... The requested URL returned error: 500\n fatal: could not fetch from promisor remote\n\nThis can be seen in the flaky failure of t5616.47 on the macOS CI\nrunners.\n\nFix the logic that checks if \"one-time-script\" has returned its modified\nresponse by chaining \"rm one-time-script\" with its execution. This\nensures a racing script does not also have the opportunity to execute\n\"one-time-script\".\n\nAdd t/t5567-one-time-script.sh to verify the race is fixed. Implement a\nstub \"git-http-backend\" that intentionally invokes a concurrent request,\nand check that only one modified response is returned without error.\n\nSigned-off-by: Michael Montalbo \n---\n t/lib-httpd/apply-one-time-script.sh | 38 +++++++----\n t/meson.build | 1 +\n t/t5567-one-time-script.sh | 96 ++++++++++++++++++++++++++++\n 3 files changed, 121 insertions(+), 14 deletions(-)\n create mode 100755 t/t5567-one-time-script.sh\n\ndiff --git a/t/lib-httpd/apply-one-time-script.sh b/t/lib-httpd/apply-one-time-script.sh\nindex b1682944e2..eac21a3a8e 100644\n--- a/t/lib-httpd/apply-one-time-script.sh\n+++ b/t/lib-httpd/apply-one-time-script.sh\n@@ -6,21 +6,31 @@\n #\n # This can be used to simulate the effects of the repository changing in\n # between HTTP request-response pairs.\n-if test -f one-time-script\n-then\n-\tLC_ALL=C\n-\texport LC_ALL\n+test -f one-time-script || exec \"$GIT_EXEC_PATH/git-http-backend\"\n+\n+LC_ALL=C\n+export LC_ALL\n \n-\t\"$GIT_EXEC_PATH/git-http-backend\" >out\n-\t./one-time-script out >out_modified\n+out=out.$$\n+modified=out-modified.$$\n+\"$GIT_EXEC_PATH/git-http-backend\" >\"$out\"\n \n-\tif cmp -s out out_modified\n-\tthen\n-\t\tcat out\n-\telse\n-\t\tcat out_modified\n-\t\trm one-time-script\n-\tfi\n+# Since Apache can execute this script for multiple requests\n+# concurrently, we chain \"rm one-time-script\" with the logic\n+# for generating a modified response. If the \"rm\" ran separately,\n+# a concurrent request could pass the \"test -f\" above and\n+# erroneously result in multiple modified responses or an empty\n+# body depending on the race state.\n+#\n+# We discard stderr for ./one-time-script since it is possible\n+# ./one-time-script has been removed already, which is expected\n+# sometimes. In this case, the unmodified response will be returned.\n+if ./one-time-script \"$out\" 2>/dev/null >\"$modified\" &&\n+ ! cmp -s \"$out\" \"$modified\" &&\n+ rm one-time-script 2>/dev/null\n+then\n+\tcat \"$modified\"\n else\n-\t\"$GIT_EXEC_PATH/git-http-backend\"\n+\tcat \"$out\"\n fi\n+rm -f \"$out\" \"$modified\"\ndiff --git a/t/meson.build b/t/meson.build\nindex a25f37d2f5..e4d0b6dc4e 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -716,6 +716,7 @@ integration_tests = [\n 't5564-http-proxy.sh',\n 't5565-push-multiple.sh',\n 't5566-push-group.sh',\n+ 't5567-one-time-script.sh',\n 't5570-git-daemon.sh',\n 't5571-pre-push-hook.sh',\n 't5572-pull-submodule.sh',\ndiff --git a/t/t5567-one-time-script.sh b/t/t5567-one-time-script.sh\nnew file mode 100755\nindex 0000000000..a8429ef3c3\n--- /dev/null\n+++ b/t/t5567-one-time-script.sh\n@@ -0,0 +1,96 @@\n+#!/bin/sh\n+\n+test_description='apply-one-time-script CGI helper is safe under concurrent requests'\n+\n+. ./test-lib.sh\n+\n+HELPER=\"$TEST_DIRECTORY/lib-httpd/apply-one-time-script.sh\"\n+\n+test_expect_success PIPE 'helper only serves one rewritten response for concurrent requests' '\n+\tmkdir workdir fakebin &&\n+\tENTERED=\"$PWD/entered\" &&\n+\tGATE=\"$PWD/gate\" &&\n+\texport ENTERED GATE &&\n+\tmkfifo \"$ENTERED\" \"$GATE\" &&\n+\n+\t# A stub git-http-backend that returns a response based on\n+\t# $ROLE. For $ROLE = modify, return the response string\n+\t# \"packfile\", which ends up being modified by the example\n+\t# one-time-script below.\n+\t#\n+\t# Otherwise, run the branch returning a response that\n+\t# should be passed through, and block until released\n+\t# by \"read -r $GATE\".\n+\twrite_script fakebin/git-http-backend <<-\\EOF &&\n+\tprintf \"Status: 200 OK\\r\\n\"\n+\tprintf \"Content-Type: application/x-git-result\\r\\n\"\n+\tprintf \"\\r\\n\"\n+\tif test \"$ROLE\" = modify\n+\tthen\n+\t\tprintf \"packfile\\n\"\n+\telse\n+\t\techo entered >\"$ENTERED\"\n+\t\tread -r released <\"$GATE\"\n+\t\tprintf \"refs\\n\"\n+\tfi\n+\tEOF\n+\n+\t# An example one-time-script for apply-one-time-script\n+\t# to execute. Checks for \"packfile\" in the response\n+\t# that will be returned, and replaces it with a\n+\t# modified response. Passes through responses without\n+\t# \"packfile\" in them.\n+\twrite_script workdir/one-time-script <<-\\EOF &&\n+\tif grep packfile \"$1\" >/dev/null\n+\tthen\n+\t\tsed \"/packfile/q\" \"$1\" &&\n+\t\tprintf \"REPLACED\\n\"\n+\telse\n+\t\tcat \"$1\"\n+\tfi\n+\tEOF\n+\n+\tGIT_EXEC_PATH=\"$PWD/fakebin\" &&\n+\texport GIT_EXEC_PATH &&\n+\n+\t# Ensure $GATE has a reader so the test does not block indefinitely if\n+\t# the helper is buggy and \"echo released >&9\" below does not unblock\n+\t# the unmodified response gate.\n+\texec 9<>\"$GATE\" &&\n+\n+\t# Launch the passthrough request in the background. Record its pid\n+\t# so it can be killed when the test finishes if, for some reason, the\n+\t# request stays blocked and would stall a test runner.\n+\t{ (\n+\t\tcd workdir &&\n+\t\tROLE=passthrough sh \"$HELPER\" >../passthrough.out 2>../passthrough.err\n+\t) & } &&\n+\tpassthrough_pid=$! &&\n+\ttest_when_finished \"kill $passthrough_pid 2>/dev/null || :\" &&\n+\n+\t# Wait until the passthrough request is \"in-flight\" and paused\n+\t# mid-response.\n+\tread -r entered <\"$ENTERED\" &&\n+\n+\t# Launch the request for a modified response while the passthrough\n+\t# request is concurrently \"in-flight\" and paused.\n+\t(\n+\t\tcd workdir &&\n+\t\tROLE=modify sh \"$HELPER\" >../modify.out 2>../modify.err\n+\t) &&\n+\n+\t# Unblock the passthrough request, allowing git-http-backend to\n+\t# complete its response.\n+\techo released >&9 &&\n+\t{ wait \"$passthrough_pid\" || :; } &&\n+\n+\ttest_must_be_empty passthrough.err &&\n+\ttest_must_be_empty modify.err &&\n+\ttest_grep \"Status: 200 OK\" passthrough.out &&\n+\ttest_grep \"Status: 200 OK\" modify.out &&\n+\ttest_grep REPLACED modify.out &&\n+\ttest_grep ! REPLACED passthrough.out &&\n+\ttest_grep refs passthrough.out\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"551683","messageId":"79396d491fe15c94a4e4c079d1109b425dcf966a.1788277983.git.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.v5.git.1788277983.gitgitgadget@gmail.com","subject":"[PATCH v5 2/3] t/lib-httpd: make http-429 first-request check atomic","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-01T15:53:02Z","receivedAt":"2026-09-01T15:53:09Z","isPatch":true,"body":"From: Michael Montalbo \n\nhttp-429.sh is a helper for testing retry logic. It uses \"test -f\" to\ncheck for the existence of a state file and later uses \"touch\" or\n\"rm -f\" on that file to determine if it should return a 429. This method\nof managing state can fail if the helper script is invoked concurrently.\nHowever, this failure does not currently manifest itself since the\nhelper is invoked sequentially.\n\nAs a preventive measure, fix the state management logic so it relies on\nan atomic mkdir operation to mark that a 429 was returned. When\n$retry_after is \"permanent\", always return 429 now that we do not rely\non a state file that is \"touch\"ed and \"rm\"ed to indicate when to respond\nwith a 429.\n\nSigned-off-by: Michael Montalbo \n---\n t/lib-httpd/http-429.sh | 22 ++++++++++------------\n 1 file changed, 10 insertions(+), 12 deletions(-)\n\ndiff --git a/t/lib-httpd/http-429.sh b/t/lib-httpd/http-429.sh\nindex c97b16145b..1a5d7987db 100644\n--- a/t/lib-httpd/http-429.sh\n+++ b/t/lib-httpd/http-429.sh\n@@ -3,7 +3,7 @@\n # Script to return HTTP 429 Too Many Requests responses for testing retry logic.\n # Usage: /http_429///\n #\n-# The test-context is a unique identifier for each test to isolate state files.\n+# The test-context is a unique identifier for each test to isolate state directories.\n # The retry-after-value can be:\n # - A number (e.g., \"1\", \"2\", \"100\") - sets Retry-After header to that many seconds\n # - \"none\" - no Retry-After header\n@@ -26,14 +26,16 @@ repo_path=\"${remaining#*/}\" # Get rest (repo path)\n # The repo name is the first component before any \"/\"\n repo_name=\"${repo_path%%/*}\"\n \n-# Use current directory (HTTPD_ROOT_PATH) for state file\n-# Create a safe filename from test_context, retry_after and repo_name\n-# This ensures all requests for the same test context share the same state file\n+# Use current directory (HTTPD_ROOT_PATH) to hold state directory\n+# Create a safe directory name from test_context, retry_after and repo_name\n+# This ensures all requests for the same test context share the same state directory\n safe_name=$(echo \"${test_context}-${retry_after}-${repo_name}\" | tr '/' '_' | tr -cd 'a-zA-Z0-9_-')\n-state_file=\"http-429-state-${safe_name}\"\n+state=\"http-429-state-${safe_name}\"\n \n-# Check if this is the first call (no state file exists)\n-if test -f \"$state_file\"\n+# Check if this is the first call (no state directory exists), or if\n+# the retry-after-value is \"permanent\", which indicates a 429 must be\n+# returned for every request (even if the state directory exists).\n+if test \"$retry_after\" != permanent && ! mkdir \"$state\" 2>/dev/null\n then\n \t# Already returned 429 once, forward to git-http-backend\n \t# Set PATH_INFO to just the repo path (without retry-after value)\n@@ -52,9 +54,6 @@ then\n \texec \"$GIT_EXEC_PATH/git-http-backend\"\n fi\n \n-# Mark that we've returned 429\n-touch \"$state_file\"\n-\n # Output HTTP 429 response\n printf \"Status: 429 Too Many Requests\\r\\n\"\n \n@@ -67,8 +66,7 @@ case \"$retry_after\" in\n \t\tprintf \"Retry-After: invalid-format-123abc\\r\\n\"\n \t\t;;\n \tpermanent)\n-\t\t# Always return 429, don't set state file for success\n-\t\trm -f \"$state_file\"\n+\t\t# Always return 429\n \t\tprintf \"Retry-After: 1\\r\\n\"\n \t\tprintf \"Content-Type: text/plain\\r\\n\"\n \t\tprintf \"\\r\\n\"\n-- \ngitgitgadget\n\n"},{"id":"551684","messageId":"75a184ca09010c1ff75b140ae31c4af32fc33503.1788277983.git.gitgitgadget@gmail.com","threadId":"65950","inReplyTo":"pull.2171.v5.git.1788277983.gitgitgadget@gmail.com","subject":"[PATCH v5 3/3] t/lib-httpd: document writing concurrency-safe CGI helpers","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-09-01T15:53:03Z","receivedAt":"2026-09-01T15:53:10Z","isPatch":true,"body":"From: Michael Montalbo \n\nUpdate t/lib-httpd.sh to document the fixes applied to\napply-one-time-script.sh and http-429.sh for future developers working\non helper scripts. Add concrete examples of patterns and anti-patterns\nthat should be considered when handling state management.\n\nSigned-off-by: Michael Montalbo \n---\n t/lib-httpd.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex a216e5376f..115455784c 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -159,6 +159,18 @@ prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/proxy-passwd \"$HTTPD_ROOT_PATH\"\n+\t# Apache can run the following scripts concurrently per request. Make\n+\t# sure any state management logic is resilient to race conditions.\n+\t#\n+\t# For example:\n+\t# - use \"mkdir dir\" to ensure only one request \"succeeds\" under some\n+\t# condition (see http-429.sh).\n+\t# - chain (&&) atomic operations like \"rm marker\" (no -f) with the\n+\t# logic that is guarded by the marker instead of relying on a\n+\t# separate \"test -f\" and \"rm marker\" check\n+\t# (see apply-one-time-script.sh).\n+\t# - use scratch file names that include the process ID ($$), so\n+\t# concurrent requests do not overwrite each other's state.\n \tinstall_script incomplete-length-upload-pack-v2-http.sh\n \tinstall_script incomplete-body-upload-pack-v2-http.sh\n \tinstall_script error-no-report.sh\n-- \ngitgitgadget\n"},{"id":"551829","messageId":"apkFyvN4hcEOadQq@pks.im","threadId":"65950","inReplyTo":"pull.2171.v5.git.1788277983.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 0/3] t/lib-httpd: make CGI test helpers concurrency-safe","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-09-03T05:29:46Z","receivedAt":"2026-09-03T05:29:58Z","isPatch":true,"body":"On Tue, Sep 01, 2026 at 03:53:00PM +0000, Michael Montalbo via GitGitGadget wrote:\n> Changes since v4:\n> \n> * Reword advice about chaining (&&) atomic operations like rm so it refers\n> to chaining with \"the logic guarded by the marker\" instead of \"the logic\n> that claims the marker\" since the latter is circular and inaccurate\n> (atomic operations like rm are the logic that claims markers).\n\nThis version looks good to me. Thanks!\n\nPatrick\n"}]}