[PATCH 0/1] Fix zombie children when git is PID 1 in containers

[PATCH 0/1] Fix zombie children when git is PID 1 in containers

[Andrew Au]

When git runs as PID 1 inside a container and exits via exit(128) on
transport errors, child processes (git-remote-https, ssh, proxy) are
never waited on because the normal cleanup paths (disconnect_helper,
finish_connect) are bypassed. Since PID 1 has no parent to reap its
children, these become zombies that persist for the container's lifetime.

This patch registers atexit handlers in transport-helper.c and connect.c
to ensure children are reaped on any exit path, and clears them on the
normal cleanup paths to avoid double-waiting.

Blog post with detailed investigation: https://cshung.github.io/posts/zombie-git/

Andrew Au (1):
  transport-helper, connect: add atexit handler to reap children on
    abnormal exit

 connect.c          | 17 +++++++++++++++++
 transport-helper.c | 11 +++++++++++
 2 files changed, 28 insertions(+)

-- 
2.43.0

[PATCH 1/1] transport-helper, connect: add atexit handler to reap children on abnormal exit

[Andrew Au]

From: Andrew Au <3410332+cshung@users.noreply.github.com>

When git exits via exit(128) on transport errors, child processes
(git-remote-https, ssh, proxy) are never waited on because the normal
cleanup paths (disconnect_helper, finish_connect) are bypassed. When
git is PID 1 in a container, these un-reaped children become zombies.

Register atexit handlers in both transport-helper.c and connect.c to
ensure children are reaped on any exit path. Clear the handlers on the
normal cleanup paths to avoid double-waiting.

Signed-off-by: Andrew Au <cshung@gmail.com>
---
 connect.c          | 17 +++++++++++++++++
 transport-helper.c | 11 +++++++++++
 2 files changed, 28 insertions(+)

diff --git a/connect.c b/connect.c
index eef752f14..322b1f816 100644
--- a/connect.c
+++ b/connect.c
@@ -20,6 +20,18 @@ static char *server_capabilities_v1;
 static struct strvec server_capabilities_v2 = STRVEC_INIT;
 static const char *next_server_feature_value(const char *feature, int *len, int *offset);
 
+/*
+ * Ensure the connection child (ssh, proxy, or local git) is reaped on
+ * any exit path, mirroring the transport-helper.c atexit pattern.
+ */
+static struct child_process *conn_to_reap;
+
+static void cleanup_conn_on_exit(void)
+{
+	if (conn_to_reap)
+		finish_command(conn_to_reap);
+}
+
 static int check_ref(const char *name, unsigned int flags)
 {
 	if (!flags)
@@ -991,6 +1003,8 @@ static struct child_process *git_proxy_connect(int fd[2], char *host)
 	proxy->out = -1;
 	if (start_command(proxy))
 		die(_("cannot start proxy %s"), git_proxy_command);
+	conn_to_reap = proxy;
+	atexit(cleanup_conn_on_exit);
 	fd[0] = proxy->out; /* read from proxy stdout */
 	fd[1] = proxy->in;  /* write to proxy stdin */
 	return proxy;
@@ -1449,6 +1463,8 @@ struct child_process *git_connect(int fd[2], const char *url,
 
 		if (start_command(conn))
 			die(_("unable to fork"));
+		conn_to_reap = conn;
+		atexit(cleanup_conn_on_exit);
 
 		fd[0] = conn->out; /* read from child's stdout */
 		fd[1] = conn->in;  /* write to child's stdin */
@@ -1466,6 +1482,7 @@ int finish_connect(struct child_process *conn)
 		return 0;
 
 	code = finish_command(conn);
+	conn_to_reap = NULL;
 	free(conn);
 	return code;
 }
diff --git a/transport-helper.c b/transport-helper.c
index e95267a4a..cdfd40dfc 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -17,6 +17,14 @@
 
 static int debug;
 
+static struct child_process *helper_to_reap;
+
+static void cleanup_helper_on_exit(void)
+{
+	if (helper_to_reap)
+		finish_command(helper_to_reap);
+}
+
 struct helper_data {
 	const char *name;
 	struct child_process *helper;
@@ -147,6 +155,8 @@ static struct child_process *get_helper(struct transport *transport)
 		exit(code);
 
 	data->helper = helper;
+	helper_to_reap = helper;
+	atexit(cleanup_helper_on_exit);
 	data->no_disconnect_req = 0;
 	refspec_init(&data->rs, REFSPEC_FETCH);
 
@@ -249,6 +259,7 @@ static int disconnect_helper(struct transport *transport)
 		close(data->helper->out);
 		fclose(data->out);
 		res = finish_command(data->helper);
+		helper_to_reap = NULL;
 		FREE_AND_NULL(data->helper);
 	}
 	return res;
-- 
2.43.0

Re: [PATCH 1/1] transport-helper, connect: add atexit handler to reap children on abnormal exit

[Kristoffer Haugsbakk]

On Mon, Feb 23, 2026, at 17:51, Andrew Au wrote:
> From: Andrew Au <3410332+cshung@users.noreply.github.com>

This email here,

>[snip]
>
> Signed-off-by: Andrew Au <cshung@gmail.com>

has to match with the email here. Or probably vice versa since a real
email address is better than a GitHub noreply email in this context.

Maybe this was just an automatic don’t-use-my-real-email-address thing
that GitHub did?

> ---
>[snip]

Re: [PATCH 1/1] transport-helper, connect: add atexit handler to reap children on abnormal exit

[Andrew Au]

Yes, this is forced by the GitHub don't use my real email address
option. I won't be allowed to push if I used my real address there.

I can change the commit author to use my real email address if that's
preferred for upstream.

On Mon, Feb 23, 2026 at 9:15 AM Kristoffer Haugsbakk
<kristofferhaugsbakk@fastmail.com> wrote:
>
> On Mon, Feb 23, 2026, at 17:51, Andrew Au wrote:
> > From: Andrew Au <3410332+cshung@users.noreply.github.com>
>
> This email here,
>
> >[snip]
> >
> > Signed-off-by: Andrew Au <cshung@gmail.com>
>
> has to match with the email here. Or probably vice versa since a real
> email address is better than a GitHub noreply email in this context.
>
> Maybe this was just an automatic don’t-use-my-real-email-address thing
> that GitHub did?
>
> > ---
> >[snip]

[PATCH v2] transport-helper, connect: add atexit handler to reap children on abnormal exit

[Andrew Au]

When git exits via exit(128) on transport errors, child processes
(git-remote-https, ssh, proxy) are never waited on because the normal
cleanup paths (disconnect_helper, finish_connect) are bypassed. When
git is PID 1 in a container, these un-reaped children become zombies.

Register atexit handlers in both transport-helper.c and connect.c to
ensure children are reaped on any exit path. Clear the handlers on the
normal cleanup paths to avoid double-waiting.

Signed-off-by: Andrew Au <cshung@gmail.com>
---
 connect.c          | 17 +++++++++++++++++
 transport-helper.c | 11 +++++++++++
 2 files changed, 28 insertions(+)

diff --git a/connect.c b/connect.c
index eef752f14..322b1f816 100644
--- a/connect.c
+++ b/connect.c
@@ -20,6 +20,18 @@ static char *server_capabilities_v1;
 static struct strvec server_capabilities_v2 = STRVEC_INIT;
 static const char *next_server_feature_value(const char *feature, int *len, int *offset);
 
+/*
+ * Ensure the connection child (ssh, proxy, or local git) is reaped on
+ * any exit path, mirroring the transport-helper.c atexit pattern.
+ */
+static struct child_process *conn_to_reap;
+
+static void cleanup_conn_on_exit(void)
+{
+	if (conn_to_reap)
+		finish_command(conn_to_reap);
+}
+
 static int check_ref(const char *name, unsigned int flags)
 {
 	if (!flags)
@@ -991,6 +1003,8 @@ static struct child_process *git_proxy_connect(int fd[2], char *host)
 	proxy->out = -1;
 	if (start_command(proxy))
 		die(_("cannot start proxy %s"), git_proxy_command);
+	conn_to_reap = proxy;
+	atexit(cleanup_conn_on_exit);
 	fd[0] = proxy->out; /* read from proxy stdout */
 	fd[1] = proxy->in;  /* write to proxy stdin */
 	return proxy;
@@ -1449,6 +1463,8 @@ struct child_process *git_connect(int fd[2], const char *url,
 
 		if (start_command(conn))
 			die(_("unable to fork"));
+		conn_to_reap = conn;
+		atexit(cleanup_conn_on_exit);
 
 		fd[0] = conn->out; /* read from child's stdout */
 		fd[1] = conn->in;  /* write to child's stdin */
@@ -1466,6 +1482,7 @@ int finish_connect(struct child_process *conn)
 		return 0;
 
 	code = finish_command(conn);
+	conn_to_reap = NULL;
 	free(conn);
 	return code;
 }
diff --git a/transport-helper.c b/transport-helper.c
index e95267a4a..cdfd40dfc 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -17,6 +17,14 @@
 
 static int debug;
 
+static struct child_process *helper_to_reap;
+
+static void cleanup_helper_on_exit(void)
+{
+	if (helper_to_reap)
+		finish_command(helper_to_reap);
+}
+
 struct helper_data {
 	const char *name;
 	struct child_process *helper;
@@ -147,6 +155,8 @@ static struct child_process *get_helper(struct transport *transport)
 		exit(code);
 
 	data->helper = helper;
+	helper_to_reap = helper;
+	atexit(cleanup_helper_on_exit);
 	data->no_disconnect_req = 0;
 	refspec_init(&data->rs, REFSPEC_FETCH);
 
@@ -249,6 +259,7 @@ static int disconnect_helper(struct transport *transport)
 		close(data->helper->out);
 		fclose(data->out);
 		res = finish_command(data->helper);
+		helper_to_reap = NULL;
 		FREE_AND_NULL(data->helper);
 	}
 	return res;
-- 
2.43.0

Re: [PATCH v2] transport-helper, connect: add atexit handler to reap children on abnormal exit

[Junio C Hamano]

Andrew Au <cshung@gmail.com> writes:

> When git exits via exit(128) on transport errors, child processes
> (git-remote-https, ssh, proxy) are never waited on because the normal
> cleanup paths (disconnect_helper, finish_connect) are bypassed. When
> git is PID 1 in a container, these un-reaped children become zombies.

Could you tell me more about the real use case behind such a set-up.

These children become zombies, and then what will be done to the
container that lost the "git" process, running of which presumably
was the primary reason why the container was brought up in the first
place?  Wouldn't these zombies go away when the container that
finished its sole purpose of running "git" gets dismantled?

Thanks.

Re: [PATCH v2] transport-helper, connect: add atexit handler to reap children on abnormal exit

[Andrew Au]

Thank you for the feedback.

The use case is a long-running service running as PID 1 inside a
container. The service continuously spawns git to detect repository
changes — it is not a one-shot container where git itself is the
primary process. Because the service is meant to stay alive
indefinitely, any zombies git leaves behind accumulate over time
rather than being cleaned up when the container exits.

In my specific case, I observed over 6,500 zombie processes before
identifying this as the root cause. The blog post linked in the cover
letter documents the investigation in detail.

The fix ensures git cleans up its own children on abnormal exit paths,
which is the right behavior regardless of whether the parent is PID 1
or not.

On Wed, Mar 11, 2026 at 10:58 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Andrew Au <cshung@gmail.com> writes:
>
> > When git exits via exit(128) on transport errors, child processes
> > (git-remote-https, ssh, proxy) are never waited on because the normal
> > cleanup paths (disconnect_helper, finish_connect) are bypassed. When
> > git is PID 1 in a container, these un-reaped children become zombies.
>
> Could you tell me more about the real use case behind such a set-up.
>
> These children become zombies, and then what will be done to the
> container that lost the "git" process, running of which presumably
> was the primary reason why the container was brought up in the first
> place?  Wouldn't these zombies go away when the container that
> finished its sole purpose of running "git" gets dismantled?
>
> Thanks.

Re: [PATCH v2] transport-helper, connect: add atexit handler to reap children on abnormal exit

[Jeff King]

On Wed, Mar 11, 2026 at 02:20:21PM +0000, Andrew Au wrote:

> +/*
> + * Ensure the connection child (ssh, proxy, or local git) is reaped on
> + * any exit path, mirroring the transport-helper.c atexit pattern.
> + */
> +static struct child_process *conn_to_reap;
> +
> +static void cleanup_conn_on_exit(void)
> +{
> +	if (conn_to_reap)
> +		finish_command(conn_to_reap);
> +}

This waits for the command to exit. Are we sure it will always do so,
and it won't sometimes be waiting on us to do something (like close a
pipe that is feeding it)? If not, then we can get deadlocks.

I think you actually want to kill(), then wait. There is already support
for this in run-command.[ch]. You just need to set the clean_on_exit
flag of the child_process struct.

I actually wonder if clean_on_exit should become the default behavior.
It should be rare for our subprocesses to outlive us. Commit afe19ff7b5
(run-command: optionally kill children on exit, 2012-01-07) mentions the
pager, but I don't think that was true even back then (we wait around
for the pager to finish). There are a few cases where we spawn daemon
programs, which would need to be marked as survivable. I think mostly we
have not looked into it because somebody would have to look at each
run_command() callsite.

Anyway, that is a bit of a tangent. I think it would be safe to mark the
spots in this patch as clean_on_exit.

-Peff

Re: [PATCH v2] transport-helper, connect: add atexit handler to reap children on abnormal exit

[Junio C Hamano]

Andrew Au <cshung@gmail.com> writes:

> Thank you for the feedback.
>
> The use case is a long-running service running as PID 1 inside a
> container. The service continuously spawns git to detect repository
> changes — it is not a one-shot container where git itself is the
> primary process. Because the service is meant to stay alive
> indefinitely, any zombies git leaves behind accumulate over time
> rather than being cleaned up when the container exits.

Wait, the proposed log message said "Git is the pid 1", and now the
above is "there is some long-running service that runs Git".  Which
one is it?  Wouldn't that long-running service be the right process
that should reap these unwaited children?  Or is "git" used as that
long-running service somehow?

Re: [PATCH 0/1] Fix zombie children when git is PID 1 in containers

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]

On 2026-02-23 at 16:51:46, Andrew Au wrote:
> When git runs as PID 1 inside a container and exits via exit(128) on
> transport errors, child processes (git-remote-https, ssh, proxy) are
> never waited on because the normal cleanup paths (disconnect_helper,
> finish_connect) are bypassed. Since PID 1 has no parent to reap its
> children, these become zombies that persist for the container's lifetime.
> 
> This patch registers atexit handlers in transport-helper.c and connect.c
> to ensure children are reaped on any exit path, and clears them on the
> normal cleanup paths to avoid double-waiting.
> 
> Blog post with detailed investigation: https://cshung.github.io/posts/zombie-git/

Usually people use a tool like tini as PID 1 in containers, which allows
that process to handle process reaping while still exec'ing the normal
Git or other command.  It's the case that _most_ processes are not
designed to run properly as process 1 in a container or otherwise, so
a specialized init-capable helper program is usually a good idea.

Even with this series to fix some of the process reaping problems, I
expect you'll find other cases in Git where we don't always reap
processes correctly as well, so an init helper would still be useful.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 0/1] Fix zombie children when git is PID 1 in containers

[Andrew Au]

Thank you for the suggestion. You are right that tini is a common and
reasonable approach, and I will look into using it regardless.

That said, I think fixing git to clean up its own children is still
worthwhile — processes should not leave zombies on abnormal exit paths
whether or not an init helper is present. The two approaches are
complementary rather than mutually exclusive.

On Wed, Mar 11, 2026 at 2:15 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2026-02-23 at 16:51:46, Andrew Au wrote:
> > When git runs as PID 1 inside a container and exits via exit(128) on
> > transport errors, child processes (git-remote-https, ssh, proxy) are
> > never waited on because the normal cleanup paths (disconnect_helper,
> > finish_connect) are bypassed. Since PID 1 has no parent to reap its
> > children, these become zombies that persist for the container's lifetime.
> >
> > This patch registers atexit handlers in transport-helper.c and connect.c
> > to ensure children are reaped on any exit path, and clears them on the
> > normal cleanup paths to avoid double-waiting.
> >
> > Blog post with detailed investigation: https://cshung.github.io/posts/zombie-git/
>
> Usually people use a tool like tini as PID 1 in containers, which allows
> that process to handle process reaping while still exec'ing the normal
> Git or other command.  It's the case that _most_ processes are not
> designed to run properly as process 1 in a container or otherwise, so
> a specialized init-capable helper program is usually a good idea.
>
> Even with this series to fix some of the process reaping problems, I
> expect you'll find other cases in Git where we don't always reap
> processes correctly as well, so an init helper would still be useful.
> --
> brian m. carlson (they/them)
> Toronto, Ontario, CA

[PATCH v3] transport-helper, connect: use clean_on_exit to reap children on abnormal exit

[Andrew Au]

When a long-running service (e.g., a source indexer) runs as PID 1
inside a container and repeatedly spawns git, git may in turn spawn
child processes such as git-remote-https or ssh. If git exits abnormally
(e.g., via exit(128) on a transport error), the normal cleanup paths
(disconnect_helper, finish_connect) are bypassed, and these children are
never waited on. The children are reparented to PID 1, which does not
reap them, so they accumulate as zombies over time.

Set clean_on_exit and wait_after_clean on child_process structs in both
transport-helper.c and connect.c so that the existing run-command
cleanup infrastructure handles reaping on any exit path. This avoids
rolling custom atexit handlers that call finish_command(), which could
deadlock if the child is blocked waiting for the parent to close a pipe.

The clean_on_exit mechanism sends SIGTERM first, then waits, ensuring
the child terminates promptly. It also handles signal-based exits, not
just atexit.

Signed-off-by: Andrew Au <cshung@gmail.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---
Thanks to Jeff King for suggesting the clean_on_exit approach,
which is simpler and avoids potential deadlocks from the atexit
handler in v2. Also thanks to Junio for catching the inaccurate
description of the PID 1 scenario.

 connect.c          | 4 ++++
 transport-helper.c | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/connect.c b/connect.c
index eef752f14..5039adca7 100644
--- a/connect.c
+++ b/connect.c
@@ -989,6 +989,8 @@ static struct child_process *git_proxy_connect(int fd[2], char *host)
 	strvec_push(&proxy->args, port);
 	proxy->in = -1;
 	proxy->out = -1;
+	proxy->clean_on_exit = 1;
+	proxy->wait_after_clean = 1;
 	if (start_command(proxy))
 		die(_("cannot start proxy %s"), git_proxy_command);
 	fd[0] = proxy->out; /* read from proxy stdout */
@@ -1447,6 +1449,8 @@ struct child_process *git_connect(int fd[2], const char *url,
 		}
 		strvec_push(&conn->args, cmd.buf);
 
+		conn->clean_on_exit = 1;
+		conn->wait_after_clean = 1;
 		if (start_command(conn))
 			die(_("unable to fork"));
 
diff --git a/transport-helper.c b/transport-helper.c
index e95267a4a..6633a999b 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -140,6 +140,8 @@ static struct child_process *get_helper(struct transport *transport)
 
 	helper->trace2_child_class = helper->args.v[0]; /* "remote-<name>" */
 
+	helper->clean_on_exit = 1;
+	helper->wait_after_clean = 1;
 	code = start_command(helper);
 	if (code < 0 && errno == ENOENT)
 		die(_("unable to find remote helper for '%s'"), data->name);
-- 
2.43.0

Re: [PATCH v3] transport-helper, connect: use clean_on_exit to reap children on abnormal exit

[Jeff King]

On Thu, Mar 12, 2026 at 07:55:59PM +0000, Andrew Au wrote:

> Set clean_on_exit and wait_after_clean on child_process structs in both
> transport-helper.c and connect.c so that the existing run-command
> cleanup infrastructure handles reaping on any exit path. This avoids
> rolling custom atexit handlers that call finish_command(), which could
> deadlock if the child is blocked waiting for the parent to close a pipe.

Ah, right, I forgot about wait_after_clean when I suggested
clean_on_exit. Yes, you definitely want both here.

> The clean_on_exit mechanism sends SIGTERM first, then waits, ensuring
> the child terminates promptly. It also handles signal-based exits, not
> just atexit.
> 
> Signed-off-by: Andrew Au <cshung@gmail.com>
> 
> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

I don't know if we have established a pattern in the project for
AI-assisted work (and whether it is worth marking at all, and if so, if
co-author is the right way to do so). But if we are going to, usually
the trailers would all be together, with no blank line in between.

>  connect.c          | 4 ++++
>  transport-helper.c | 2 ++
>  2 files changed, 6 insertions(+)

The changes here are all a trivial implementation of the idea. So if the
idea is good (and I think it is), then the patch looks correct to me.

-Peff

Re: [PATCH v3] transport-helper, connect: use clean_on_exit to reap children on abnormal exit

[Jeff King]

On Thu, Mar 12, 2026 at 04:40:24PM -0400, Jeff King wrote:

> > Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
> 
> I don't know if we have established a pattern in the project for
> AI-assisted work (and whether it is worth marking at all, and if so, if
> co-author is the right way to do so). But if we are going to, usually
> the trailers would all be together, with no blank line in between.

Trailers aside, we definitely don't want it in the mail's cc headers, as
that address bounces (unsurprisingly).

-Peff

Re: [PATCH v3] transport-helper, connect: use clean_on_exit to reap children on abnormal exit

[Junio C Hamano]

Andrew Au <cshung@gmail.com> writes:

> When a long-running service (e.g., a source indexer) runs as PID 1
> inside a container and repeatedly spawns git, git may in turn spawn
> child processes such as git-remote-https or ssh. If git exits abnormally
> (e.g., via exit(128) on a transport error), the normal cleanup paths
> (disconnect_helper, finish_connect) are bypassed, and these children are
> never waited on. The children are reparented to PID 1, which does not
> reap them, so they accumulate as zombies over time.
>
> Set clean_on_exit and wait_after_clean on child_process structs in both
> transport-helper.c and connect.c so that the existing run-command
> cleanup infrastructure handles reaping on any exit path. This avoids
> rolling custom atexit handlers that call finish_command(), which could
> deadlock if the child is blocked waiting for the parent to close a pipe.
>
> The clean_on_exit mechanism sends SIGTERM first, then waits, ensuring
> the child terminates promptly. It also handles signal-based exits, not
> just atexit.
>
> Signed-off-by: Andrew Au <cshung@gmail.com>
>
> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Do not throw a blank line inside your trailer block.  In addition,
we do not want somebody who cannot stand beind the patch and certify
DCO listed there.  See also SubmittingPatches::[ai].

> Thanks to Jeff King for suggesting the clean_on_exit approach,
> which is simpler and avoids potential deadlocks from the atexit
> handler in v2. Also thanks to Junio for catching the inaccurate
> description of the PID 1 scenario.

This version looks quite simple and clean, taking advantage of
existing machinery to clean these processes up.

>  connect.c          | 4 ++++
>  transport-helper.c | 2 ++
>  2 files changed, 6 insertions(+)
>
> diff --git a/connect.c b/connect.c
> index eef752f14..5039adca7 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -989,6 +989,8 @@ static struct child_process *git_proxy_connect(int fd[2], char *host)
>  	strvec_push(&proxy->args, port);
>  	proxy->in = -1;
>  	proxy->out = -1;
> +	proxy->clean_on_exit = 1;
> +	proxy->wait_after_clean = 1;
>  	if (start_command(proxy))
>  		die(_("cannot start proxy %s"), git_proxy_command);
>  	fd[0] = proxy->out; /* read from proxy stdout */
> @@ -1447,6 +1449,8 @@ struct child_process *git_connect(int fd[2], const char *url,
>  		}
>  		strvec_push(&conn->args, cmd.buf);
>  
> +		conn->clean_on_exit = 1;
> +		conn->wait_after_clean = 1;
>  		if (start_command(conn))
>  			die(_("unable to fork"));
>  
> diff --git a/transport-helper.c b/transport-helper.c
> index e95267a4a..6633a999b 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -140,6 +140,8 @@ static struct child_process *get_helper(struct transport *transport)
>  
>  	helper->trace2_child_class = helper->args.v[0]; /* "remote-<name>" */
>  
> +	helper->clean_on_exit = 1;
> +	helper->wait_after_clean = 1;
>  	code = start_command(helper);
>  	if (code < 0 && errno == ENOENT)
>  		die(_("unable to find remote helper for '%s'"), data->name);

[PATCH v4] transport-helper, connect: use clean_on_exit to reap children on abnormal exit

[Andrew Au]

When a long-running service (e.g., a source indexer) runs as PID 1
inside a container and repeatedly spawns git, git may in turn spawn
child processes such as git-remote-https or ssh. If git exits abnormally
(e.g., via exit(128) on a transport error), the normal cleanup paths
(disconnect_helper, finish_connect) are bypassed, and these children are
never waited on. The children are reparented to PID 1, which does not
reap them, so they accumulate as zombies over time.

Set clean_on_exit and wait_after_clean on child_process structs in both
transport-helper.c and connect.c so that the existing run-command
cleanup infrastructure handles reaping on any exit path. This avoids
rolling custom atexit handlers that call finish_command(), which could
deadlock if the child is blocked waiting for the parent to close a pipe.

The clean_on_exit mechanism sends SIGTERM first, then waits, ensuring
the child terminates promptly. It also handles signal-based exits, not
just atexit.

Signed-off-by: Andrew Au <cshung@gmail.com>
---
 connect.c          | 4 ++++
 transport-helper.c | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/connect.c b/connect.c
index eef752f14..5039adca7 100644
--- a/connect.c
+++ b/connect.c
@@ -989,6 +989,8 @@ static struct child_process *git_proxy_connect(int fd[2], char *host)
 	strvec_push(&proxy->args, port);
 	proxy->in = -1;
 	proxy->out = -1;
+	proxy->clean_on_exit = 1;
+	proxy->wait_after_clean = 1;
 	if (start_command(proxy))
 		die(_("cannot start proxy %s"), git_proxy_command);
 	fd[0] = proxy->out; /* read from proxy stdout */
@@ -1447,6 +1449,8 @@ struct child_process *git_connect(int fd[2], const char *url,
 		}
 		strvec_push(&conn->args, cmd.buf);
 
+		conn->clean_on_exit = 1;
+		conn->wait_after_clean = 1;
 		if (start_command(conn))
 			die(_("unable to fork"));
 
diff --git a/transport-helper.c b/transport-helper.c
index e95267a4a..6633a999b 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -140,6 +140,8 @@ static struct child_process *get_helper(struct transport *transport)
 
 	helper->trace2_child_class = helper->args.v[0]; /* "remote-<name>" */
 
+	helper->clean_on_exit = 1;
+	helper->wait_after_clean = 1;
 	code = start_command(helper);
 	if (code < 0 && errno == ENOENT)
 		die(_("unable to find remote helper for '%s'"), data->name);
-- 
2.43.0

Re: [PATCH v4] transport-helper, connect: use clean_on_exit to reap children on abnormal exit

[Junio C Hamano]

Andrew Au <cshung@gmail.com> writes:

> When a long-running service (e.g., a source indexer) runs as PID 1
> inside a container and repeatedly spawns git, git may in turn spawn
> child processes such as git-remote-https or ssh. If git exits abnormally
> (e.g., via exit(128) on a transport error), the normal cleanup paths
> (disconnect_helper, finish_connect) are bypassed, and these children are
> never waited on. The children are reparented to PID 1, which does not
> reap them, so they accumulate as zombies over time.
>
> Set clean_on_exit and wait_after_clean on child_process structs in both
> transport-helper.c and connect.c so that the existing run-command
> cleanup infrastructure handles reaping on any exit path. This avoids
> rolling custom atexit handlers that call finish_command(), which could
> deadlock if the child is blocked waiting for the parent to close a pipe.
>
> The clean_on_exit mechanism sends SIGTERM first, then waits, ensuring
> the child terminates promptly. It also handles signal-based exits, not
> just atexit.
>
> Signed-off-by: Andrew Au <cshung@gmail.com>
> ---
>  connect.c          | 4 ++++
>  transport-helper.c | 2 ++
>  2 files changed, 6 insertions(+)

Thanks, queued.


>
> diff --git a/connect.c b/connect.c
> index eef752f14..5039adca7 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -989,6 +989,8 @@ static struct child_process *git_proxy_connect(int fd[2], char *host)
>  	strvec_push(&proxy->args, port);
>  	proxy->in = -1;
>  	proxy->out = -1;
> +	proxy->clean_on_exit = 1;
> +	proxy->wait_after_clean = 1;
>  	if (start_command(proxy))
>  		die(_("cannot start proxy %s"), git_proxy_command);
>  	fd[0] = proxy->out; /* read from proxy stdout */
> @@ -1447,6 +1449,8 @@ struct child_process *git_connect(int fd[2], const char *url,
>  		}
>  		strvec_push(&conn->args, cmd.buf);
>  
> +		conn->clean_on_exit = 1;
> +		conn->wait_after_clean = 1;
>  		if (start_command(conn))
>  			die(_("unable to fork"));
>  
> diff --git a/transport-helper.c b/transport-helper.c
> index e95267a4a..6633a999b 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -140,6 +140,8 @@ static struct child_process *get_helper(struct transport *transport)
>  
>  	helper->trace2_child_class = helper->args.v[0]; /* "remote-<name>" */
>  
> +	helper->clean_on_exit = 1;
> +	helper->wait_after_clean = 1;
>  	code = start_command(helper);
>  	if (code < 0 && errno == ENOENT)
>  		die(_("unable to find remote helper for '%s'"), data->name);

Re: [PATCH v4] transport-helper, connect: use clean_on_exit to reap children on abnormal exit

[Jeff King]

On Thu, Mar 12, 2026 at 03:04:17PM -0700, Junio C Hamano wrote:

> Thanks, queued.

Curiously this patch seems to cause a failure in one of the CI leak
jobs, but I don't think it's the culprit. See below for a fix and
explanation.

I don't know if you want to apply it separately (since it's really a
totally different topic) or on top (since it is only the application of
Andrew's patch which lets us find the problem).

-- >8 --
Subject: [PATCH] transport: plug leaks in transport_color_config()

We retrieve config values with repo_config_get_string(), which will
allocate a new copy of the string for us. But we don't hold on to those
strings, since they are just fed to git_config_colorbool() and
color_parse(). But nor do we free them, which means they leak.

We can fix this by using the "_tmp" form of repo_config_get_string(),
which just hands us a pointer directly to the internal storage. This is
OK for our purposes, since we don't need it to last for longer than our
parsing calls.

Two interesting side notes here:

  1. Many types already have a repo_config_get_X() variant that handles
     this for us (e.g., repo_config_get_bool()). But neither colorbools
     nor colors themselves have such helpers. We might think about
     adding them, but converting all callers is a larger task, and out
     of scope for this fix.

  2. As far as I can tell, this leak has been there since 960786e761
     (push: colorize errors, 2018-04-21), but wasn't detected by LSan in
     our test suite. It started triggering when we applied dd3693eb08
     (transport-helper, connect: use clean_on_exit to reap children on
     abnormal exit, 2026-03-12) which is mostly unrelated.

     Even weirder, it seems to trigger only with clang (and not gcc),
     and only with GIT_TEST_DEFAULT_REF_FORMAT=reftable. So I think this
     is another odd case where the pointers happened to be hanging
     around in stack memory, but changing the pattern of function calls
     in nearby code was enough for them to be incidentally overwritten.

Signed-off-by: Jeff King <peff@peff.net>
---
 transport.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/transport.c b/transport.c
index 107f4fa5dc..2fb4767821 100644
--- a/transport.c
+++ b/transport.c
@@ -54,14 +54,14 @@ static int transport_color_config(void)
 		return 0;
 	initialized = 1;
 
-	if (!repo_config_get_string(the_repository, key, &value))
+	if (!repo_config_get_string_tmp(the_repository, key, &value))
 		transport_use_color = git_config_colorbool(key, value);
 
 	if (!want_color_stderr(transport_use_color))
 		return 0;
 
 	for (size_t i = 0; i < ARRAY_SIZE(keys); i++)
-		if (!repo_config_get_string(the_repository, keys[i], &value)) {
+		if (!repo_config_get_string_tmp(the_repository, keys[i], &value)) {
 			if (!value)
 				return config_error_nonbool(keys[i]);
 			if (color_parse(value, transport_colors[i]) < 0)
-- 
2.53.0.887.g3d5d06adec

Re: [PATCH v4] transport-helper, connect: use clean_on_exit to reap children on abnormal exit

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   1. Many types already have a repo_config_get_X() variant that handles
>      this for us (e.g., repo_config_get_bool()). But neither colorbools
>      nor colors themselves have such helpers. We might think about
>      adding them, but converting all callers is a larger task, and out
>      of scope for this fix.

That certainly is an interesting #leftoverbits project.

The patch looks good.  Will queue.  Thanks.

Re: [PATCH v4] transport-helper, connect: use clean_on_exit to reap children on abnormal exit

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't know if you want to apply it separately (since it's really a
> totally different topic) or on top (since it is only the application of
> Andrew's patch which lets us find the problem).
> ...
>  transport.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/transport.c b/transport.c
> index 107f4fa5dc..2fb4767821 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -54,14 +54,14 @@ static int transport_color_config(void)
>  		return 0;
>  	initialized = 1;
>  
> -	if (!repo_config_get_string(the_repository, key, &value))
> +	if (!repo_config_get_string_tmp(the_repository, key, &value))
>  		transport_use_color = git_config_colorbool(key, value);
>  
>  	if (!want_color_stderr(transport_use_color))
>  		return 0;
>  
>  	for (size_t i = 0; i < ARRAY_SIZE(keys); i++)
> -		if (!repo_config_get_string(the_repository, keys[i], &value)) {
> +		if (!repo_config_get_string_tmp(the_repository, keys[i], &value)) {
>  			if (!value)
>  				return config_error_nonbool(keys[i]);
>  			if (color_parse(value, transport_colors[i]) < 0)

Regardless of where it goes, we need to change a bit more, it seems?

    CC transport.o
transport.c: In function 'transport_color_config':
transport.c:57:62: error: passing argument 3 of 'repo_config_get_string_tmp' from incompatible pointer type [-Wincompatible-pointer-types]
   57 |         if (!repo_config_get_string_tmp(the_repository, key, &value))
      |                                                              ^~~~~~
      |                                                              |
      |                                                              char **
In file included from transport.c:5:
config.h:644:62: note: expected 'const char **' but argument is of type 'char **'
  644 |                                const char *key, const char **dest);
      |                                                 ~~~~~~~~~~~~~^~~~
transport.c:64:74: error: passing argument 3 of 'repo_config_get_string_tmp' from incompatible pointer type [-Wincompatible-pointer-types]
   64 |                 if (!repo_config_get_string_tmp(the_repository, keys[i], &value)) {
      |                                                                          ^~~~~~
      |                                                                          |
      |                                                                          char **
config.h:644:62: note: expected 'const char **' but argument is of type 'char **'
  644 |                                const char *key, const char **dest);
      |                                                 ~~~~~~~~~~~~~^~~~
gmake: *** [Makefile:2815: transport.o] Error 1


I'll squash an obvious patch in.

 transport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git c/transport.c w/transport.c
index 358bc38585..7985b42a74 100644
--- c/transport.c
+++ w/transport.c
@@ -47,7 +47,7 @@ static int transport_color_config(void)
 		"color.transport.reset",
 		"color.transport.rejected"
 	}, *key = "color.transport";
-	char *value;
+	const char *value;
 	static int initialized;
 
 	if (initialized)

Re: [PATCH v4] transport-helper, connect: use clean_on_exit to reap children on abnormal exit

[Jeff King]

On Mon, Mar 16, 2026 at 01:31:08PM -0700, Junio C Hamano wrote:

> Regardless of where it goes, we need to change a bit more, it seems?
> 
>     CC transport.o
> transport.c: In function 'transport_color_config':
> transport.c:57:62: error: passing argument 3 of 'repo_config_get_string_tmp' from incompatible pointer type [-Wincompatible-pointer-types]
>    57 |         if (!repo_config_get_string_tmp(the_repository, key, &value))
>       |                                                              ^~~~~~
>       |                                                              |
>       |                                                              char **

Huh. Obviously yes, but how did I manage to bungle this so badly?

I _think_ what happened is that I compiled the topic without -Werror,
because of all of the -Wdiscarded-qualifier errors that happen on
'master' with a recent glibc.

My integration cycle would have caught it, since I have another topic
with fixes for the discarded-qualifier issue. But I hadn't run one yet. ;)

None of that is important for you, but just wondering if there was
something more subtle going on, or if I just screwed up something as
simple as typing 'make'.

> I'll squash an obvious patch in.

Yep, that looks good. Thanks for fixing.

-Peff

Re: [PATCH v4] transport-helper, connect: use clean_on_exit to reap children on abnormal exit

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Huh. Obviously yes, but how did I manage to bungle this so badly?
>
> I _think_ what happened is that I compiled the topic without -Werror,
> because of all of the -Wdiscarded-qualifier errors that happen on
> 'master' with a recent glibc.
>
> My integration cycle would have caught it, since I have another topic
> with fixes for the discarded-qualifier issue. But I hadn't run one yet. ;)
>
> None of that is important for you, but just wondering if there was
> something more subtle going on, or if I just screwed up something as
> simple as typing 'make'.
>
>> I'll squash an obvious patch in.
>
> Yep, that looks good. Thanks for fixing.

Thanks.  It is probably a good thing that you are using toolchain
ahead of the version I use.  We catch different kind of errors that
way, even though occasionally we see a gotcha like this one.