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

[Jacob Keller <jacob.e.keller@intel.com>]

On 5/11/2026 1:01 PM, Jeff King wrote:
> 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).
> 
>> (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.
> 
>>> 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:
> 
>> +	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.
> 
Ya, that seems really ugly. My first thought was some way to disable
signals temporarily, but I am guessing that either has no good way to do
it or would introduce even more issues. Plus there is always kill -9...

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

Yea, I agree that this might qualify as unlikely enough to not worry.