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

[Junio C Hamano <gitster@pobox.com>]

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);