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

[Patrick Steinhardt <ps@pks.im>]

On Tue, May 12, 2026 at 10:19:22AM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index 3a71e314c9..09cb92ac97 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -1810,10 +1810,32 @@ 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 assign ownership
> > +			 * of the lockfile to the child and then exit immediately.
> > +			 */
> > +			lock_file_reassign_owner(&lk, child_pid);
> > +			exit(0);
> 
> The point of reassigning the owner to somebody else is so that we
> won't clean them when we exit as the tempfile.c::remove_tempfile()
> function checks the "owner" is "me" and refrains from unlinking
> those that do not belong to us, so there is nothing wrong in this
> code, but this somehow felt awkward.  In a sense, child_pid here
> does not have to be what fork() returned but anything that is not
> our own pid.  Perhaps "we assign ... to the child" -> "we relinquish
> ... to prevent us removing upon exiting" would convey the intention
> better?  I dunno.

Fair. This is what I got now:

	/*
	 * We're in the parent process, so we drop ownership of
	 * the lockfile to prevent us from removing it upon
	 * exit.
	 */

> > -int daemonize(void)
> > +pid_t daemonize_without_exit(void)
> >  {
> >  #ifdef NO_POSIX_GOODIES
> >  	errno = ENOSYS;
> >  	return -1;
> >  #else
> > -	switch (fork()) {
> > -		case 0:
> > -			break;
> > -		case -1:
> > -			die_errno(_("fork failed"));
> > -		default:
> > -			exit(0);
> > -	}
> > +	pid_t pid = fork();
> > +	if (pid < 0)
> > +		return -1;
> > +	if (pid > 0)
> > +		return pid;
> > +
> >  	if (setsid() == -1)
> >  		die_errno(_("setsid failed"));
> >  	close(0);
> > @@ -2180,6 +2178,21 @@ int daemonize(void)
> >  #endif
> >  }
> >  
> > +int daemonize(void)
> > +{
> > +#ifdef NO_POSIX_GOODIES
> > +	errno = ENOSYS;
> > +	return -1;
> > +#else
> > +	pid_t pid = daemonize_without_exit();
> > +	if (pid < 0)
> > +		die_errno(_("fork failed"));
> > +	if (pid > 0)
> > +		exit(0);
> > +	return 0;
> > +#endif
> > +}
> 
> I was hoping that we can do without the #ifdef in this caller as
> daemonize_without_exit() already has exactly the same condtional
> compilation.  If the NO_POSIX_GOODIES side can just return silently
> wit ENOSYS, shouldn't the callers be also fine if we return failure
> instead of calling die_errno(_("fork failed")), I have to wonder.
> 
> But because (1) as long as we have to call die_errno() here, we must
> keep the conditional compilation in daemonize() as well as
> daemonize_without_exit(), and (2) changing what the callers get when
> fork failed here is totally outside of this topic, I would say that
> the code around here is good as-is.

Yeah, I was also pondering whether I can drop the additional ifdef. But
I eventually decided to aim for the most minimal fix that has the least
potential for additional regressions. So I aimed to keep `daemonize()`
semantically the same as before, and I aimed to only fix the issue with
the lockfile we know about.

I agree though that this is something we should probably clean up in a
subsequent series. We don't have that many callers of `daemonize()`
after all, so it shouldn't be that involved, either.

Thanks!

Patrick