Re: [PATCH v2 1/2] builtin/maintenance: fix locking with "--detach"

[Taylor Blau <me@ttaylorr.com>]

On Tue, May 12, 2026 at 10:30:30AM +0200, Patrick Steinhardt wrote:
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 3a71e314c9..d866c19b92 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1810,10 +1810,33 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
>  				   TASK_PHASE_FOREGROUND))
>  			result = 1;
>
> -	/* Failure to daemonize is ok, we'll continue in foreground. */
>  	if (opts->detach > 0) {
> +		pid_t child_pid;
> +
>  		trace2_region_enter("maintenance", "detach", the_repository);
> -		daemonize();
> +
> +		child_pid = daemonize_without_exit();
> +		if (!child_pid) {
> +			/*
> +			 * We're in the child process, so we take ownership of
> +			 * the lockfile.
> +			 */
> +			lock_file_reassign_owner(&lk, getpid());
> +		} else if (child_pid > 0) {
> +			/*
> +			 * We're in the parent process, so we drop ownership of
> +			 * the lockfile to prevent us from removing it upon
> +			 * exit.
> +			 */
> +			lock_file_reassign_owner(&lk, child_pid);
> +			exit(0);
> +		} else {
> +			/*
> +			 * Failure to daemonize is ok, we'll continue in
> +			 * foreground.
> +			 */
> +		}
> +

This works almost identically as the original implementation I shared[1]
original thread discussing this issue, but it takes a slightly different
approach.

Instead of doing this automatically as a part of daemonize(), this patch
only adjusts this singular call-site, and forces us to introduce a new
function daemonize_without_exit() in order to facilitate it.

I personally prefer the other approach, for many of the reasons that
Peff wrote about in [2, 3]. There are two questions in my mind that I
would be curious of your thoughts on:

 * Should we transfer ownership of all tempfiles, or a subset of them?

 * Should we perform that transfer automatically via daemonize(), or
   manually via its callers?

Judging from the other parts of the original thread, I think the answer
to the first question is an unambiguous "yes". The second question I
think there is less of a consensus on, but I'd like to advocate for
doing this automatically via daemonize().

To summarize some of my thoughts after reading [2], I think that because
daemonize() is about letting a process continue on in the background
rather than fork()-ing children and then continuing on ourselves, *not*
automatically handing those off feels like an accident waiting to
happen.

I can't think of a situation where someone would want to daemonize() but
not have the new process assume ownership of its locks and tempfiles. I
think an alternative approach here would be to document the behavior I'm
proposing above clearly in daemonize(). Should a caller arise in the
future that *doesn't* want to this behavior, then we could introduce a
function similar to your daemonize_without_exit() (maybe in this case it
would be daemonize_without_transfer(), since not exiting in a function
that is supposed to daemonize feels awkward).

I guess what I'm saying is that I feel that future daemonize() callers
are much more likely to want this behavior than not, and putting this in
daemonize() feels like the safer option between the two.

FWIW, I feel fairly strongly about ^ this, but I'm of course happy to
discuss more if you feel differently.

(If you do end up taking that approach, that makes the C changes
identical to the ones I shared in [1], so you are free to forge my
Co-authored-by and Signed-off-by lines if you want to take that approach
instead.)

> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 4700beacc1..df0bbc1669 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -1438,6 +1438,64 @@ test_expect_success '--no-detach causes maintenance to not run in background' '
>  	)
>  '
>
> +test_expect_success PIPE '--detach holds maintenance lock until daemonized child exits' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +
> +		git config maintenance.auto false &&
> +		git config core.lockfilepid true &&
> +
> +		git remote add origin /does/not/exist &&
> +		git config set remote.origin.uploadpack "cat fifo-uploadpack" &&
> +
> +		mkfifo fifo-uploadpack fifo-maint &&
> +
> +		# Open the maintenance FIFO, as otherwise spawning
> +		# git-maintenance(1) would block. Note that we need to open it
> +		# as read-write, as otherwise we would block here already.
> +		exec 9<>fifo-maint &&

Very nice. The fifo-maint idea is much more robust than what I was able
to come up with in my original sketch, and it is a nice compliment to
fifo-uploadpack.

The rest of the test is nearly identical to mine and looks good to me.

Thanks,
Taylor

[1]: https://lore.kernel.org/git/af+snTGFeoUUyfPU@nand.local/
[2]: https://lore.kernel.org/git/20260511200112.GA22912@coredump.intra.peff.net/
[3]: https://lore.kernel.org/git/20260511202258.GD22912@coredump.intra.peff.net/