Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config

[Derrick Stolee <stolee@gmail.com>]

On 5/9/26 5:52 PM, Taylor Blau wrote:
> On Sat, May 09, 2026 at 01:52:49PM -0400, Jeff King wrote:
>> I don't think this affected the old "git gc --detach" because it takes
>> the lock after daemonizing[1]. We can't do the same here, though, since we
>> need to hold the lock for the foreground tasks. So either we need to
>> release and re-take the lock between foreground and background tasks, or
>> we need to teach the daemonize() function to update the "owner" field on
>> all of the tempfiles to the new child[2].
> 
> I agree. We can't simply do what was done in 329e6e8794c (gc: save log
> from daemonized gc --auto and print it next time, 2015-09-19), for
> exactly the reason that you stated.
> 
> Dropping and re-acquiring the lock is possible, but it's racy since
> there is a gap during the critical window while we fork(). I believe
> that the only airtight way to do this is to update the owner field of
> the tempfiles we want to pass down during daemonization.

I agree that this drop/reacquire pattern would not be a safe choice.

> (As an aside, do we want to do that for all tempfiles? It might be nice
> to have a "->reassign_on_fork" flag or something on the tempfile struct
> in case there are instances where the parent wants to retain ownership
> of the tempfile after fork()-ing, but I can't think of any off the top
> of my head. If we do introduce such a field, it should probably default
> to "true" to avoid any foot-guns.)
> 
>> Ultimately fixing the lock bug will solve that. Though if doing so is
>> too complicated for a quick maint release, I'm tempted to say we should
>> consider reverting 452b12c2e0 for a potential v2.54.1 (as there were a
>> few other regression fixes so far, I assume we'll have one soon-ish).
> 
> I think something like the following (untested) would do the trick:
> 
> --- 8< ---
> diff --git a/setup.c b/setup.c
> index 7ec4427368a..c07aeac4f7d 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -22,6 +22,7 @@
>   #include "chdir-notify.h"
>   #include "path.h"
>   #include "quote.h"
> +#include "tempfile.h"
>   #include "trace.h"
>   #include "trace2.h"
>   #include "worktree.h"
> @@ -2162,12 +2163,17 @@ int daemonize(void)
>   	errno = ENOSYS;
>   	return -1;
>   #else
> -	switch (fork()) {
> +	pid_t ppid = getpid();
> +	pid_t pid;
> +
> +	switch ((pid = fork())) {
>   		case 0:
> +			reassign_tempfile_ownership(ppid, getpid());
>   			break;
>   		case -1:
>   			die_errno(_("fork failed"));
>   		default:
> +			reassign_tempfile_ownership(ppid, pid);
>   			exit(0);
>   	}
>   	if (setsid() == -1)
> diff --git a/tempfile.c b/tempfile.c
> index 82dfa3d82f2..f0fdf582794 100644
> --- a/tempfile.c
> +++ b/tempfile.c
> @@ -373,3 +373,15 @@ int delete_tempfile(struct tempfile **tempfile_p)
> 
>   	return err ? -1 : 0;
>   }
> +
> +void reassign_tempfile_ownership(pid_t from, pid_t to)
> +{
> +	volatile struct volatile_list_head *pos;
> +
> +	list_for_each(pos, &tempfile_list) {
> +		struct tempfile *p = list_entry(pos, struct tempfile, list);
> +
> +		if (is_tempfile_active(p) && p->owner == from)
> +			p->owner = to;
> +	}
> +}

I worry that we're assigning _all_ tempfiles to the child in this
case, not just specific ones like the maintenance lock. But since
this is specifically called only in daemonize() then it may be
fine.

Is there any concern that since the child didn't create a tempfile
that the atexit() may not be initialized in the child process? Or
will that carry over from the fork() automatically? (This is hard
to test without something causing a die() in the detached process.)
> That's not too terrible to write, and I would feel OK about putting it
> in a 2.54.1 release soon-ish provided that others think it is reasonable.

I'd rather revert the geometric maintenance default for a point
release and let something more complicated like this percolate
until the next release window.

> Simply reverting 452b12c2e0 (builtin/maintenance: use "geometric"
> strategy by default, 2026-02-24) feels somewhat unsatisfying, since it
> is merely making the bug less likely rather than eliminating it
> entirely.

It does limit the behavior to those who previously chose to opt-in to
this behavior, and likely that's a low number or else we'd have more
bug reports.

> So in that sense I would prefer to "fix forward" here rather than to
> mask over the bug. But even the relatively short diff above is not so
> straightforward to reason through, review, or test, so I'm open to other
> ideas on how to proceed here.

I initially worried about cross-platform support, thinking that we
needed to pass file descriptors / handles and Windows always has
issues with file handles. But we aren't actually keeping a handle
open but instead a record that we created the lock and should delete
it when everything resolves.

For me to be convinced that this forward fix is the right direction,
I'd need to see a test that proves the detached process will clean up
the locks on a normal process end and an early exit.

Thanks,
-Stolee