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

From: Jeff King <peff@peff.net>
Date: May 9, 2026, 17:52 UTC
Message-ID: <20260509175249.GA2336928@coredump.intra.peff.net>
In-Reply-To: <20260508180341.GB737125@coredump.intra.peff.net>

On Fri, May 08, 2026 at 02:03:41PM -0400, Jeff King wrote:

Show 14 quoted lines

> Also, should background maintenance be locking to avoid multiple runs?
> It does not seem to do so, and if I run:
> 
>   git init
>   for i in $(seq 10000); do
>     echo $i >>file
>     git add file
>     git commit -m "commit $i"
>   done
> 
> I get several concurrent pack-objects processes. After a few thousand
> commits I got bored and hit ^C, and the resulting repo was corrupt!
> Which is not too surprising, as multiple simultaneous repacks are known
> to be unsafe, but means we should probably avoid them.

There is a pretty awful bug here, and it got much worse in v2.54.

First, the locking for "git maintenance run --detach" is totally broken. We take the lock as the first step of maintenance_run_tasks(), then run foreground tasks, then daemonize(), and then run background tasks. But daemonize() forks and exits the parent process, leaving the child process to do the real work. But the lock subsystem registers the lockfile as a tempfile to be cleaned up at exit. So the moment we daemonize, we release the lock, even though we're still going to run sub-commands that should happen under lock.

I don't think this affected the old "git gc --detach" because it takes the lock after daemonizing[1]. We can't do the same here, though, since we need to hold the lock for the foreground tasks. So either we need to release and re-take the lock between foreground and background tasks, or we need to teach the daemonize() function to update the "owner" field on all of the tempfiles to the new child[2].

AFAICT this has been broken since a6affd3343 (builtin/maintenance: add a `--detach` flag, 2024-08-16). But it was mostly harmless because under the hood we were running the "gc" task, which ran git-gc itself in no-detach mode. So git-gc took its own lock, and we never ended up with more than one repack running at a time.

But that changed in 452b12c2e0 (builtin/maintenance: use "geometric" strategy by default, 2026-02-24), which is new in v2.54. Now we are running repack directly, with no additional locking. So because of the lock bug from a6affd3343, that means lots of repacks running at the same time.

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

-Peff

[1] git-gc does have a mode where it will take the lock and run some
    foreground tasks. That mode probably has the same bug, but I'm not
    sure if it existed before we switched to "maintenance run --auto".
    At this point it's moot.

[2] You can't just disable the atexit handler in the parent process
    here, because then the child wouldn't actually clean them up either
    (because its pid will not match the owner field). So we really need
    to transfer ownership during the daemonize() process, which probably
    needs a new tempfile.[ch] function to walk the list and update all
    of the owner fields.