Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config
- From
Taylor Blau <me@ttaylorr.com>
- Date
- May 10, 2026, 20:00 UTC
- Message-ID
- <agDjyAXxkOIso8FU@nand.local>
- 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.
Show 15 quoted lines
> > 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