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

[Taylor Blau <me@ttaylorr.com>]

On Wed, Jun 29, 2022 at 05:44:25PM -0700, Gregory Szorc wrote:
> Revising my initial post, not running `loose-objects` is insufficient:
> we also need to prevent `incremental-repack` from running while a `git
> gc` / `git repack` is in progress. If an `incremental-repack` runs
> concurrently with `git repack`, `repack` can error after bitmap
> generation but before the temp packfile is renamed with "could not
> find 'pack-*.pack'." I suspect this has to do with
> `incremental-repack` deleting packs that were initially in place when
> `git repack` started running. But I haven't looked into where exactly
> repack is failing.

Yeah, you would need to prevent other writers from removing packs while
writing a cruft pack.

I think the canonical way to do this would be to let `git
maintenance` use its own locking to ensure that it wasn't trying to
remove packs while simultaneously generating a cruft pack.

But assuming that the cruft pack generation is running independently and
concurrently with the maintenance incremental-repack task (which will
delete packs in the background), then there is definitely a TOCTOU race
there.

The race is that `repack` will signal to `pack-objects` which packs will
stay and which packs will be deleted during the repack. To generate a
cruft pack, the packs that stay are formed by doing a reachability
traversal and writing a pack that contains all reachable objects. That
way anything that is in `all-packs \ reachable` (which form the contents
of the cruft pack) are going to be just the unreachable objects.

But if a pack that was going to be deleted by repack *after* generating
the cruft pack disappears while `repack` is running (particularly after
it starts, but before it generates the cruft pack), then the cruft pack
generation can't proceed, since it has no idea what objects it needs to
add.

Namely, if it is told that pack P is going to be deleted, any objects in
P which don't appear in a pack that *isn't* going to be deleted need to
be saved. If we don't have a copy of P anymore, then there is no way to
come up with what that set of objects is, meaning that it's impossible
to generate the cruft pack.

I should mention, BTW, that this isn't a problem unique to cruft packs.
Geometric repacking, which uses `pack-objects --stdin-packs` has a
similar issue where if a pack being combined is removed from underneath
while `repack` is running, `pack-objects` cannot successfully generate
the pack.

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

Thanks,
Taylor