git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v3 02/10] t7422: fix flaky test caused by buffered stdout

From
Patrick Steinhardt <ps@pks.im>
Date
Jan 7, 2025, 12:30 UTC
Message-ID
<20250107-b4-pks-ci-fixes-v3-2-546a0ebc8481@pks.im>
In-Reply-To
<20250107-b4-pks-ci-fixes-v3-0-546a0ebc8481@pks.im>

One test in t7422 asserts that `git submodule status --recursive` properly handles SIGPIPE. This test is flaky though and may sometimes not see a SIGPIPE at all:

    expecting success of 7422.18 'git submodule status --recursive propagates SIGPIPE':
            { git submodule status --recursive 2>err; echo $?>status; } |
                    grep -q X/S &&
            test_must_be_empty err &&
            test_match_signal 13 "$(cat status)"
    ++ git submodule status --recursive
    ++ grep -q X/S
    ++ echo 0
    ++ test_must_be_empty err
    ++ test 1 -ne 1
    ++ test_path_is_file err
    ++ test 1 -ne 1
    ++ test -f err
    ++ test -s err
    +++ cat status
    ++ test_match_signal 13 0
    ++ test 0 = 141
    ++ test 0 = 269
    ++ return 1
    error: last command exited with $?=1
    not ok 18 - git submodule status --recursive propagates SIGPIPE

The issue is caused by us using grep(1) to terminate the pipe on the first matching line in the recursing git-submodule(1) process. Standard streams are typically buffered though, so this condition is racy and may cause us to terminate the pipe after git-submodule(1) has already exited, and in that case we wouldn't see the expected signal.

Fix the issue by generating a couple thousand nested submodules and matching on the first nested submodule. This ensures that the recursive git-submodule(1) process completely fills its stdout buffer, which makes subsequent writes block until the downstream consumer of the pipe either fully drains it or closes it.

To verify that this works as expected one can apply the following patch to the preimage of this commit, which used to reliably trigger the race:

    diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh
    index 3c5177cc30..df6001f8a0 100755
    --- a/t/t7422-submodule-output.sh
    +++ b/t/t7422-submodule-output.sh
    @@ -202,7 +202,7 @@ test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE'
     		cd repo &&
     		GIT_ALLOW_PROTOCOL=file git submodule add "$(pwd)"/../submodule &&
     		{ git submodule status --recursive 2>err; echo $?>status; } |
    -			grep -q recursive-submodule-path-1 &&
    +			{ sleep 1 && grep -q recursive-submodule-path-1 && sleep 1; } &&
     		test_must_be_empty err &&
     		test_match_signal 13 "$(cat status)"
     	)
With the pipe-stuffing workaround the test runs successfully.
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t7422-submodule-output.sh | 43 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)
diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh
index f21e9203678b94701281d5339ae8bfe53d5de0ed..023a5cbdc44bac2389fca45cf7017750627c4ce9 100755
--- a/t/t7422-submodule-output.sh
+++ b/t/t7422-submodule-output.sh
@@ -167,10 +167,45 @@ do
 done
 
 test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE' '
-	{ git submodule status --recursive 2>err; echo $?>status; } |
-		grep -q X/S &&
-	test_must_be_empty err &&
-	test_match_signal 13 "$(cat status)"
+	# The test setup is somewhat involved because triggering a SIGPIPE is
+	# racy with buffered pipes. To avoid the raciness we thus need to make
+	# sure that the subprocess in question fills the buffers completely,
+	# which requires a couple thousand submodules in total.
+	test_when_finished "rm -rf submodule repo" &&
+	git init submodule &&
+	(
+		cd submodule &&
+		test_commit initial &&
+
+		COMMIT=$(git rev-parse HEAD) &&
+		for i in $(test_seq 2000)
+		do
+			printf "[submodule \"sm-$i\"]\npath = recursive-submodule-path-$i\n" "$i" ||
+			return 1
+		done >gitmodules &&
+		BLOB=$(git hash-object -w --stdin <gitmodules) &&
+
+		printf "100644 blob $BLOB\t.gitmodules\n" >tree &&
+		for i in $(test_seq 2000)
+		do
+			printf "160000 commit $COMMIT\trecursive-submodule-path-%d\n" "$i" ||
+			return 1
+		done >>tree &&
+		TREE=$(git mktree <tree) &&
+
+		COMMIT=$(git commit-tree "$TREE") &&
+		git reset --hard "$COMMIT"
+	) &&
+
+	git init repo &&
+	(
+		cd repo &&
+		GIT_ALLOW_PROTOCOL=file git submodule add "$(pwd)"/../submodule &&
+		{ git submodule status --recursive 2>err; echo $?>status; } |
+			grep -q recursive-submodule-path-1 &&
+		test_must_be_empty err &&
+		test_match_signal 13 "$(cat status)"
+	)
 '
 
 test_done
-- 
2.48.0.rc1.245.gb3e6e7acbc.dirty
Previous: Patrick SteinhardtNext: Jeff King
Message 2 of 12 in “A couple of CI improvements”
  1. 00/10 A couple of CI improvementsPatrick Steinhardt, Jan 7, 2025
  2. 02/10 t7422: fix flaky test caused by buffered stdoutPatrick Steinhardt, Jan 7, 2025
  3. Jeff KingJan 9, 2025
  4. 01/10 t0060: fix EBUSY in MinGW when setting up runtime prefixPatrick Steinhardt, Jan 7, 2025
  5. 03/10 github: adapt containerized jobs to be rootlessPatrick Steinhardt, Jan 7, 2025
  6. 04/10 github: convert all Linux jobs to be containerizedPatrick Steinhardt, Jan 7, 2025
  7. 06/10 gitlab-ci: remove the "linux-old" jobPatrick Steinhardt, Jan 7, 2025
  8. 05/10 github: simplify computation of the job's distroPatrick Steinhardt, Jan 7, 2025
  9. 07/10 gitlab-ci: add linux32 job testing against i386Patrick Steinhardt, Jan 7, 2025
  10. 08/10 ci: stop special-casing for Ubuntu 16.04Patrick Steinhardt, Jan 7, 2025
  11. 09/10 ci: use latest Ubuntu releasePatrick Steinhardt, Jan 7, 2025
  12. 10/10 ci: remove stale code for Azure PipelinesPatrick Steinhardt, Jan 7, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.