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

[Patrick Steinhardt <ps@pks.im>]

On Tue, May 12, 2026 at 05:14:22PM -0400, Taylor Blau wrote:
> 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.)

Hmm. I was initially aiming for a more minimal fix, and that's why I
decided to make only this one callsite reassign tempfiles. But there's
only two callsites right now, and thinking a bit more about the problem
makes me agree with your take on it.

Will adapt, thanks for pushing back!

Patrick