From: Jeff King <peff@peff.net>
Date: Wed, 11 Mar 2026 18:42:06 GMT
Subject: Re: [PATCH v2] transport-helper, connect: add atexit handler to reap children on abnormal exit
Message-ID: <20260311184206.GA1911377@coredump.intra.peff.net>
In-Reply-To: <20260311142021.3464789-1-cshung@gmail.com>

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