Re: Race condition between repack and loose-objects maintenance task

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 6, 2022, 08:50 UTC
Message-ID: <220706.86bku2a9px.gmgdl@evledraar.gmail.com>
In-Reply-To: <CAKQoGakLzF+bvSnU7JD7yBibmYe0-8j3eo+CgN7EchUiNc2NkA@mail.gmail.com>

On Tue, Jul 05 2022, Gregory Szorc wrote:

> Thinking about this some more, it is clear that running `git gc` /
> `git repack` *and* `git maintenance` simultaneously is prone to
> badness since their locking mechanisms aren't aware of the other.

Yes, there are outstanding issues with the "maintenance" and "gc" locks, they should be unified, and the gc.lock has bugs & race conditions that need to be fixed. See these past threads:

    https://lore.kernel.org/git/87d02fi75p.fsf@evledraar.gmail.com/
    https://lore.kernel.org/git/87ef3o7ws1.fsf@evledraar.gmail.com/

I also have some old WIP patches to add a "gc testing" mode to our test suite, similar to the "commit graph" instrumentation if you're interested in working on this.

I.e. almost every command will fork off a "gi gc --auto", and we'll see based on the errors whether our locking is still buggy/racy.

Show 11 quoted lines

> I think it makes sense to consolidate onto `git maintenace` going
> forward as this seems to be where the inertia is. However, a barrier
> to that for me is the objects/maintenance.lock file has no acquisition
> timeout and will wait indefinitely. So in scenarios like mine where
> you have multiple processes looping over repos invoking `git
> maintenance run`, you can have extended execution stalls during
> long-running operations like `gc`.
> [...]
> There should probably be a configurable timeout for the maintenance
> lock like there is for other locks. This shouldn't be too much work
> and I may throw up a patch if others feel this is a good idea.

...but while I think this is all worthwhile I think you're on the wrong path if you think this will help much or at all with the issue being reported here.

Fixing the gc.lock (and maintenance lock...) would be nice because we'd have "gc" be less dumb, and it wouldn't get itself into lock races etc.

But you cannot hope to fix the underlying "object expiry as things become reachable" race condition that way, because that's those tasks racing with *other* object/ref creation.

Such a fix would either need some repository-global lock (which would entail all sorts of nastyness), or other workarounds around the inherent race between different object store & ref operations, see Taylor's recent "cruft pack write-out" series & my links to some past discussions of the race here:

    https://lore.kernel.org/git/220630.86y1xeeeik.gmgdl@evledraar.gmail.com/