From: Taylor Blau Date: Sun, 10 May 2026 20:00:08 GMT Subject: Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config Message-ID: In-Reply-To: <9ddfd37d-7d71-4359-b9be-d993fbfd138c@gmail.com> On Sun, May 10, 2026 at 12:08:14PM -0400, Derrick Stolee wrote: > 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. Right, I had wondered about this above, and I'm not sure what the right behavior is here. I think in our case its fine to pass all tempfiles down to the child, since we're about to exit. The only other spot that uses `daemonize()` is 'gc --detach' and 'git daemon'. I think before considering something like this we would want to reason through what the intended behavior of each of those callers is. If we end up pursuing a fix in this direction, I think the safest thing that we could do would be to have callers opt-in to this behavior so we can do the right thing in git-maintenance, and other callers retain their existing behavior. > 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.) The child will have its own copy of the tempfile list as well as its own copy of the atexit() handlers. POSIX (via atexit(3p)) says[1] that atexit() behaves according to the ISO C standard: The functionality described on this reference page is aligned with the ISO C standard. Any conflict between the requirements described here and the ISO C standard is unintentional. This volume of POSIX.1‐2017 defers to the ISO C standard. , and atexit(3) says[2]: When a child process is created via fork(2), it inherits copies of its parent's registrations. Upon a successful call to one of the exec(3) functions, all registrations are removed. So the child has an identical copy of both the atexit() handlers and the tempfile list. > > 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. I agree with what you're saying. In an ideal world we would be able to apply a fix on top that would prevent this bug from occurring, but if that's not trivial to do then we shouldn't rush it in the 2.54.1 window. I think that not having this bug whatsoever (as opposed to simply reverting 452b12c2e0) would be preferable, but as you note we haven't seen any bug reports in a pre-452b12c2e0 world, so perhaps the risk is low enough that we could merely revert 452b12c2e0. > 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. Yeah, I agree. Testing this is a little tricky, but I played around with it for a while and came up with the following: --- 8< --- diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 4700beacc18..77bfa537385 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -1460,6 +1460,56 @@ test_expect_success '--detach causes maintenance to run in background' ' ) ' +test_expect_success PIPE '--detach holds maintenance lock until daemonized child exits' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + + mkfifo fifo && + + git config maintenance.auto false && + git config core.lockfilepid true && + + git remote add origin /does/not/exist && + git config set remote.origin.uploadpack \ + "echo \$PPID >child && cat \"$(pwd)/fifo\"" && + + # Start a detached prefetch maintenance task. Note that + # we are backgrounding git-maintenance here in order to + # determine its PID to validate that the lockfile was + # created by the parent. + { git maintenance run --task=prefetch --detach & } && + parent="$!" && + + # Open fifo for writing, which will block until the + # upload-pack helper opens it for reading. Once exec + # returns, we know that the daemonized child is alive + # and pinned. + exec 8>fifo && + + test_path_is_file .git/objects/maintenance.lock && + test_path_is_file .git/objects/maintenance~pid.lock && + + # Verify that the maintenance.lock still exists, and + # that it was created by the parent process, not the + # child. + echo "pid $parent" >expect && + test_cmp expect .git/objects/maintenance~pid.lock && + + # Close the write end of the FIFO, causing our upload-pack + # helper to quit. Wait until the grandparent (from the + # perspective of our upload-pack helper, the daemonized + # git-maintenance child) + exec 8>&- && + gpid="$(ps -o ppid= -p $(cat child) | tr -d " ")" && + test -n $gpid && wait $gpid && + + test_path_is_missing .git/objects/maintenance.lock && + test_path_is_missing .git/objects/maintenance~pid.lock + ) +' + test_expect_success 'repacking loose objects is quiet' ' test_when_finished "rm -rf repo" && git init repo && --- >8 --- I'm not sure how portable it is, though. I think that 'ps -o ppid=' is OK, since 'start_git_in_background()' uses it, but I'm not sure if $PPID is available on Windows. It reliably passes with the fix that I shared earlier in the thread, and reliably fails without it. Thanks, Taylor [1]: https://man7.org/linux/man-pages/man3/atexit.3p.html [2]: https://man7.org/linux/man-pages/man3/atexit.3.html