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

[Gregory Szorc <gregory.szorc@gmail.com>]

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.

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.

On Thu, Jun 30, 2022 at 1:12 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Taylor Blau <me@ttaylorr.com> writes:
>
> > On Wed, Jun 29, 2022 at 11:19:09PM -0400, Taylor Blau wrote:
> >> > However, I think there is yet another bug at play: running
> >> > `incremental-repack` appears to be able to repack the cruft packfile.
> >> > In doing so, it deletes its .mtimes file and the metadata inside.
> >>
> >> That sounds like a bug to me. I'll take a look into it and see what I
> >> can find.
> >
> > I actually think that there are two bugs here.
> >
> > One is that we run a MIDX repack and expire, which could lead to us
> > repacking the entire contents of the cruft pack and then expiring the
> > metadata file. This is a bug, and we should exclude cruft packs from
> > this computation.
> >
> > Another bug can happen when the cruft pack gets written into the MIDX
> > and is MIDX-expireable (meaning that no objects are selected from the
> > cruft pack). In that case, the `git multi-pack-index expire` step would
> > remove the cruft pack entirely, which is also incorrect.
> >
> > I'll take a look at fixing both of these, and thanks for pointing them
> > out!
>
> Thanks, both.
>
> The fact that the semantics of the .mtimes file being not equivalent
> to the mtime on individual loose objects does not help thinking
> about the possible ways the "cruft" pack can break, and both of the
> possible issues you discuss above are indeed tricky ones.
>
>