{"thread":{"id":"65536","subject":"regression 2.54.0: test suite hangs indefinitely with mksh","startedAt":"2026-04-22T19:00:32Z","lastAt":"2026-04-22T23:00:22Z","messageCount":3,"participants":["Jan Palus","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"542156","messageId":"aekVSi4iu9YLaPLQ@rock.grzadka","threadId":"65536","inReplyTo":null,"subject":"regression 2.54.0: test suite hangs indefinitely with mksh","fromName":"Jan Palus","fromEmail":"jpalus@fastmail.com","sentAt":"2026-04-22T19:00:30Z","receivedAt":"2026-04-22T19:00:32Z","isPatch":false,"body":"If /bin/sh points to mksh, git's test suite hangs forever and never\ncompletes. An example of process tree for such hung test:\n\n$ pstree -a `pgrep -f ./t5702-protocol-v2.sh`  \nt5702-protocol- ./t5702-protocol-v2.sh\n  └─git -c protocol.version=0 ls-remote -o hello -o worldfile:///home/users/builder/rpm/BUILD/gi\n      └─sh -c git-upload-pack '/home/users/builder/rpm/BUILD/git/t/trash directory.t5702-protocol-v2/file_parent'...\n          └─git-upload-pack /home/users/builder/rpm/BUILD/git/t/trash directory.t5702-protocol-v2/file_parent\n\nbisect points to the following commit:\n\ncommit dd3693eb0859274d62feac8047e1d486b3beaf31 (HEAD)\nAuthor: Andrew Au <cshung@gmail.com>\nDate:   Thu Mar 12 22:49:37 2026\n\n    transport-helper, connect: use clean_on_exit to reap children on abnormal exit\n    \n    When a long-running service (e.g., a source indexer) runs as PID 1\n    inside a container and repeatedly spawns git, git may in turn spawn\n    child processes such as git-remote-https or ssh. If git exits abnormally\n    (e.g., via exit(128) on a transport error), the normal cleanup paths\n    (disconnect_helper, finish_connect) are bypassed, and these children are\n    never waited on. The children are reparented to PID 1, which does not\n    reap them, so they accumulate as zombies over time.\n    \n    Set clean_on_exit and wait_after_clean on child_process structs in both\n    transport-helper.c and connect.c so that the existing run-command\n    cleanup infrastructure handles reaping on any exit path. This avoids\n    rolling custom atexit handlers that call finish_command(), which could\n    deadlock if the child is blocked waiting for the parent to close a pipe.\n    \n    The clean_on_exit mechanism sends SIGTERM first, then waits, ensuring\n    the child terminates promptly. It also handles signal-based exits, not\n    just atexit.\n    \n    Signed-off-by: Andrew Au <cshung@gmail.com>\n    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nFrom commit message alone it seems the change tries to avoid reparenting\nchild processes to PID 1, however this does not seem to work for\nprocesses which were spawned with \"sh -c\".\n\nFor example if /bin/sh points to bash tests pass because bash appears to\nreact to SIGTERM differently -- shell process ends but its children are\nreparented anyway.\n\nmksh on the other hand seems to ignore SIGTERM (or queues it?) but\nwaits for child process and so test suite never ends.\n"},{"id":"542160","messageId":"20260422223542.GA110382@coredump.intra.peff.net","threadId":"65536","inReplyTo":"aekVSi4iu9YLaPLQ@rock.grzadka","subject":"Re: regression 2.54.0: test suite hangs indefinitely with mksh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-04-22T22:35:42Z","receivedAt":"2026-04-22T22:35:50Z","isPatch":false,"body":"On Wed, Apr 22, 2026 at 09:00:30PM +0200, Jan Palus wrote:\n\n> If /bin/sh points to mksh, git's test suite hangs forever and never\n> completes. An example of process tree for such hung test:\n> [...]\n\nThanks, I can reproduce the problem easily here with mksh.\n\n> bisect points to the following commit:\n> \n> commit dd3693eb0859274d62feac8047e1d486b3beaf31 (HEAD)\n> Author: Andrew Au <cshung@gmail.com>\n> Date:   Thu Mar 12 22:49:37 2026\n> \n>     transport-helper, connect: use clean_on_exit to reap children on abnormal exit\n\nYup, same here.\n\n> From commit message alone it seems the change tries to avoid reparenting\n> child processes to PID 1, however this does not seem to work for\n> processes which were spawned with \"sh -c\".\n> \n> For example if /bin/sh points to bash tests pass because bash appears to\n> react to SIGTERM differently -- shell process ends but its children are\n> reparented anyway.\n> \n> mksh on the other hand seems to ignore SIGTERM (or queues it?) but\n> waits for child process and so test suite never ends.\n\nYeah, I agree with your analysis. It looks mksh's signal handler waits\nfor its children. If I run:\n\n  # note start of script\n  date\n\n  # mksh with a slow child\n  mksh -c 'echo before; sleep 3; echo after' &\n\n  # wait for it to have started child, then kill\n  sleep 1\n  kill $!\n\n  # and now wait and mark the time\n  wait $!\n  date\n\nthen we see \"before\" but not \"after\", and the script takes 3 seconds to\nrun (since we wait for mksh to exit). Replacing \"mksh\" with bash or\ndash, it finishes in 1 second.\n\nSo what to do?\n\nI think we could alleviate the symptom by teaching clean_on_exit to\nfollow-up SIGTERM with SIGKILL. But as you note, that patch is not\nreally solving the original problem in the first place. If we use dash\nor bash in the above script and add \"ps u | grep sleep\" to the end, we\ncan see that \"sleep 3\" is still running even after we've returned. So we\nhave not really accomplished our goal anyway.\n\nSo I think just reverting dd3693eb08 is probably the most immediate fix.\n\nWe can try again at the same goal later, though I'm not quite sure how\nto do so. From Git's perspective, we do not even know the pid of the\nchild that the shell spawned, so we cannot really kill it or wait on it.\nConceptually we'd want the shell to propagate the signal to its\nchildren, but doing that automatically in the general case is rather\ntricky. I don't think we want to get into prepending trap commands at\nthe start of each shell we invoke.\n\nFor the specific cases in dd3693eb08, we probably want to signal to the\nchild to exit by closing the pipe its reading from. But we can't do that\nvia the run-command system, so it would have to be a specific signal\nhandler within the transport system. I think that would avoid true\ndeadlocks, but it still isn't quite what we want. The process won't die\nuntil it tries to read from the parent, so if it's waiting on a long\nnetwork timeout, for example, it could mean that the parent sits around\nfor quite a while even after having gotten a signal.\n\nThe fallback position to what dd3693eb08 was trying to do is roughly:\nlet them get reparented to pid 1 and don't worry about (and people in\ncontainers should really use \"tini\" or some other pid-1 replacement).\nThat doesn't seem like the worst place to be, and we can get there\neasily with a revert.\n\n\nThis all does point to some general weaknesses in run-command's\nclean_on_exit. If a shell is involved, we're probably not killing who we\nmeant to. That's usually not the end of the world (if we take \"clean\" to\nbe best-effort), but with wait_after_clean tacked on it's a recipe for a\npotential hang if the shell doesn't exit. So I tried this:\n\ndiff --git a/run-command.c b/run-command.c\nindex c146a56532..97c223bdf1 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -680,6 +680,9 @@ int start_command(struct child_process *cmd)\n \tint failed_errno;\n \tconst char *str;\n \n+\tif (cmd->use_shell && cmd->wait_after_clean)\n+\t\tBUG(\"combining use_shell and wait_after_clean is not reliable\");\n+\n \t/*\n \t * In case of errors we must keep the promise to close FDs\n \t * that have been passed in via ->in and ->out.\n\nand ran the test suite, which results in several failures. In particular\nwe'll run a \"!\" alias as a shell command with those flags, but also for\nthings like exec-ing external commands (like \"git-foo\"). So we should be\nable to produce a \"hang\" like this (built with SHELL_PATH=/bin/mksh):\n\n  ./git -c alias.foo='!echo before; sleep 10; echo after' foo &\n  sleep 1\n  kill $!\n  wait\n\nand we can see that the parent \"git\" process waits 10 seconds, even\nthough it was killed. There are two saving graces here:\n\n  1. This isn't really a deadlock, but just a slow process. We are\n     waiting on the alias to exit anyway, and we do not expect that the\n     alias command is waiting on us. This is very different from the\n     cases in dd3693eb08, where we are spawning something with a pipe\n     which could be waiting to read from us. And it looks like all of\n     the existing uses of wait_after_clean are like this; the parent is\n     conceptually doing an \"exec\" of the child, but for various reasons\n     we want it to hang around.\n\n     The exception is in http-backend, where I think we really are\n     interacting with the child (so if we got a signal and the child\n     didn't, I think we could possibly deadlock as it waits for us to\n     feed more data).\n\n  2. mksh seems eager to automatically exec its child when fed only a\n     single, simple command (like \"mksh -c 'sleep 10'). And that\n     alleviates the problem, since the signal goes straight to the child\n     command, then. So for things like git-foo externals, the shell may\n     have gotten out of the way anyway.\n\nSo I think the existing, pre-dd3693eb08 cases are _probably_ fine, even\nwith mksh, though the signal propagation may not be doing quite what we\nwant it to do. In practice I think most signals end up delivered to the\nwhole process group anyway (e.g., hitting ^C in a terminal) and that\npart doesn't matter so much.\n\n-Peff\n"},{"id":"542163","messageId":"20260422230020.GA1839627@coredump.intra.peff.net","threadId":"65536","inReplyTo":"20260422223542.GA110382@coredump.intra.peff.net","subject":"[PATCH] Revert \"transport-helper, connect: use clean_on_exit to reap children on abnormal exit\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-04-22T23:00:20Z","receivedAt":"2026-04-22T23:00:22Z","isPatch":true,"body":"On Wed, Apr 22, 2026 at 06:35:43PM -0400, Jeff King wrote:\n\n> So I think just reverting dd3693eb08 is probably the most immediate fix.\n\nHere it is in patch form.\n\n-- >8 --\nSubject: Revert \"transport-helper, connect: use clean_on_exit to reap children on abnormal exit\"\n\nThis reverts commit dd3693eb0859274d62feac8047e1d486b3beaf31.\n\nThe goal of that commit was to avoid zombie child processes hanging\naround when the parent git process is killed. But it doesn't quite work\nwhen the child command is run by the shell:\n\n  1. If there is a shell, then we kill and wait for the shell, not the\n     process spawned by the shell. And so the child process, even if it\n     eventually exits, will hang around as a zombie forever. And this is\n     true of most (all?) shells: bash, dash, etc.\n\n     So we are not really accomplishing our goal in the first place.\n\n  2. Not all shells will exit immediately upon receiving a signal. In\n     particular, mksh will wait for its children to exit (but not\n     actually propagate the signal to them!) leaving us with a potential\n     deadlock: git is wait()ing on mksh, which is wait()ing on a child\n     process, but that child process is waiting on git to produce more\n     input (or EOF) over a pipe.\n\n     You can see several examples of this deadlock in the test suite,\n     for example by running:\n\n       make SHELL_PATH=/bin/mksh\n       cd t\n       ./t5702-protocol-v2.sh\n\nBecause this is a regression for mksh users, and because we did not\nachieve our goal even with other shells, let's revert the commit for\nnow. If there is a more clever way of doing the same thing, we can\nconsider applying it separately on top (or do nothing and just accept\nthe zombies and rely on PID 1 to reap them).\n\nReported-by: Jan Palus <jpalus@fastmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n connect.c          | 4 ----\n transport-helper.c | 2 --\n 2 files changed, 6 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex fcd35c5539..a02583a102 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -1054,8 +1054,6 @@ static struct child_process *git_proxy_connect(int fd[2], char *host)\n \tstrvec_push(&proxy->args, port);\n \tproxy->in = -1;\n \tproxy->out = -1;\n-\tproxy->clean_on_exit = 1;\n-\tproxy->wait_after_clean = 1;\n \tif (start_command(proxy))\n \t\tdie(_(\"cannot start proxy %s\"), git_proxy_command);\n \tfd[0] = proxy->out; /* read from proxy stdout */\n@@ -1517,8 +1515,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t}\n \t\tstrvec_push(&conn->args, cmd.buf);\n \n-\t\tconn->clean_on_exit = 1;\n-\t\tconn->wait_after_clean = 1;\n \t\tif (start_command(conn))\n \t\t\tdie(_(\"unable to fork\"));\n \ndiff --git a/transport-helper.c b/transport-helper.c\nindex 4e5d1d914f..4614036c99 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -154,8 +154,6 @@ static struct child_process *get_helper(struct transport *transport)\n \n \thelper->trace2_child_class = helper->args.v[0]; /* \"remote-<name>\" */\n \n-\thelper->clean_on_exit = 1;\n-\thelper->wait_after_clean = 1;\n \tcode = start_command(helper);\n \tif (code < 0 && errno == ENOENT)\n \t\tdie(_(\"unable to find remote helper for '%s'\"), data->name);\n-- \n2.54.0.232.gea82d9008b\n\n"}]}