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

From: Jeff King <peff@peff.net>
Date: May 11, 2026, 20:01 UTC
Message-ID: <20260511200112.GA22912@coredump.intra.peff.net>
In-Reply-To: <af+snTGFeoUUyfPU@nand.local>

On Sat, May 09, 2026 at 05:52:29PM -0400, Taylor Blau wrote:

> 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.

It's racy in the sense that we might not get the lock back, but I don't think that's _wrong_ exactly. We just care that some instance runs the second half eventually.

It is technically vulnerable to starvation, if processes keep taking the lock, doing the foreground jobs, and then losing the race to take it a second time. But besides the fact that you'd eventually win by chance, unless you have an infinite supply of processes coming along to run the foreground half, the final invocation will run the background tasks.

I agree it feels pretty hacky. It is, however, how "git gc" seems to work (notice that it does a manual delete_tempfile() on the lock right before daemonizing).

Show 6 quoted lines

> (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.)

I think the answer is yes, we want it for all tempfiles. Because it is not a property of the individual tempfiles, but rather of the caller who is forking. The point of that owner field is so that when we spawn children, we don't end up with duplicate or unexpected cleanups.

But daemonize() is not about spawning a child or otherwise splitting the process into two. There is still a single contiguous flow of execution, and the fact that we switch pids and call exit() is an implementation detail. So we would still want to remain the "owner" of any cleanup.

And that is true for any other atexit() handlers besides the tempfile ones, too. If we called run_processes_parallel(), and then while that child was running we called daemonize(), we'd not want to kill the child then, but our atexit handler would do so! But we don't tend to leave processes running while doing stuff like daemonize(), which is why we never bothered to implement an owner field there in the first place.

Show 6 quoted lines

> > 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:

Yeah, that's what I was envisioning. Note that there's a small race here:

Show 10 quoted lines

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

The parent is giving up ownership and the child is taking it. So there may be a moment where both claim ownership, or a moment where the parent has relinquished but the child has not yet taken. We're not going to call exit() in the middle there, but a signal could kill us.

And then either:

  - If both claim ownership, it's mostly OK, because they'll both try to
    unlink() which is idempotent-ish. Though there is a bad sequence
    where we delete somebody _else's_ lock, like:

       1. Parent forks, but has not yet reassigned.

       2. Child calls reassign to take ownership. Now both have
	  ownership.

       3. Signal kills both parent and child, so they enter cleanup
	  code.

       4. One of them (let's say the parent) deletes the lockfile.

       5. Some other unrelated process (let's call it "git other") takes
	  the lock.

       6. The child deletes the lockfile.

    And at that point "git other" thinks it holds the lock, but it
    doesn't. It's quite an unlikely sequence in practice, though, I'd
    think.

  - If neither claims ownership and a signal kills both, then nobody
    cleans up the lock and it is left in place. This is annoying, but
    also something that can happen occasionally anyway (kill -9, etc).

I don't have an easy suggestion for making it more atomic, though. You could choose one or the other direction using some synchronization between the two (e.g., child reassigns only after parent signals over a pipe that it has relinquished), but it's all kind of ugly.

So it may be reasonable to just shrug and assume it's unlikely enough not to worry about.

-Peff