From: Jacob Keller Date: Mon, 11 May 2026 20:21:37 GMT Subject: Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config Message-ID: <590781db-7c19-4aa8-8497-e16e5eb5eba1@intel.com> In-Reply-To: <20260511200112.GA22912@coredump.intra.peff.net> 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.