Re: [PATCH] fsck: snapshot default refs before object walk

>
> On Mon, Dec 29, 2025 at 07:12:29PM +0000, Elijah Newren via GitGitGadget wrote:
>
> > Fsck has a race when operating on live repositories; consider the
> > following simple script that writes new commits as fsck runs:
> >
> >     #!/bin/bash
> >     git fsck &
> >     PID=$!
> >
> >     while ps -p $PID >/dev/null; do
> >         sleep 3
> >         git commit -q --allow-empty -m "Another commit"
> >     done
> >
> > Since fsck reads refs at the beginning, walks those for connectivity,
> > and then reads the refs again at the end to check, this can cause fsck
> > to get confused and think that the new refs refer to missing commits and
> > that new reflog entries are invalid.
>
> I'm not sure if this is entirely accurate. Does fsck read refs at the
> beginning? I think it walks over everything in the object database
> without regard to connectivity, marking each with the HAS_OBJ flag. And
> then we use the refs to find the reachable objects, making sure any that
> are reachable also have HAS_OBJ set (otherwise they are missing).

> So the race is really:
>
>   1. We look at all of the packs and loose objects to set HAS_OBJ.
>
>   2. Somebody else simultaneously adds a new object, which is missed by
>      our step 1, and updates a ref to point to it.
>
>   3. We look at the refs for reachability, see the new object, but
>      think it is missing because we never saw it in step 1.

> And your fix is to snapshot the refs as a "step 0", and use that
> snapshot in step 3. So any new objects that are introduced after step 1
> will never be referenced, since we are using the snapshot values.
>
> Which makes sense as long as we assume objects are only added to the
> repository. I think we'd now have the opposite direction race:
>
>   0. We snapshot the refs.
>
>   1. Somebody else deletes a ref, and then does a pruning git-gc which
>      deletes the object it pointed to.
>
>   2. We look at all of the objects and mark them as HAS_OBJ. We do not
>      include the now-deleted object.
>
>   3. We do a connectivity check with the snapshot, and are dismayed to
>      find that the deleted object (which we believe is still referenced)
>      has gone away.
>
> I think you could argue that this is a much more preferable race,
> though. A busy server will see lots of new objects introduced and refs
> updated, and you do not want to have a stop-the-world lock that prevents
> pushes. But it is much less common to do a pruning gc, and it is
> probably OK to have a mutually exclusive lock between fsck and gc.

> > This problem doesn't occur when refs are specified on the command line
> > for us to check, since we use those specified refs for both walking and
> > checking.  Using the same refs for walking and checking seems to just
> > make sense, so modify the existing code to do the same when refs aren't
> > specified.
>
> So I don't think this part is quite right either, then. We're not using
> the command-line arguments collect the set of objects in the repo. That
> still happens by walking over the odb itself. So if I do:
>
>   git fsck --no-dangling HEAD
>
> in git.git, and while it is running, do this in another terminal:
>
>   git commit --allow-empty -m foo
>
> then I get:
>
>   error: 49a90c19d0cd010ed00fbb1e4256cbefaa8b83e2: object missing
>
> even with your patch. So I think that names given on the command-line
> could benefit from this type of snapshot, because they suffer from the
> same race. You want to lock in the ref resolution (whether from
> iterating or from names on the command-line) before you start walking
> over the odb.

> And one final note on the overall direction of the patch. We are
> assuming that if we look at the refs first and then the odb second, that
> we will be getting a "fresh" view of the odb in that second step. But
> that isn't necessarily so, as we might have loaded the set of packs
> earlier in the process. I don't know if it is possible to trigger that
> during fsck or not, but certainly it is relying on a subtle assumption.
> It probably is worth calling odb_reprepare() after taking the snapshot
> to ensure we are not getting any results cached from before the snapshot
> was taken.

> > +struct ref_snapshot {
> > +     size_t nr;
> > +     size_t name_alloc;
> > +     size_t oid_alloc;
> > +     char **refname;
> > +     struct object_id *oid;
> > +};
>
> Minor nit, but: why keep two arrays and not a single struct with both?
> After all, you even end up sticking them back in a struct at the only
> point of use:
>
> > +     if (the_refs)
> > +             for (size_t i = 0; i < the_refs->nr; i++) {
> > +                     struct reference ref = {
> > +                             .name = the_refs->refname[i],
> > +                             .oid = &the_refs->oid[i],
> > +                     };
> > +                     fsck_handle_ref(&ref, NULL);
> > +             }
>
> So this could really just be an array of "struct reference". You can't
> just hold onto the "struct reference" passed in to the snapshot_refs()
> callback (because it gets reused as we iterate), but you could do a deep
> copy.
>
> That did make me wonder a bit about the other fields in "struct
> reference" (which your snapshot just throws away). But it looks like
> fsck_handle_ref() only cares about the name and oid, so it is OK.

> > @@ -999,6 +1050,19 @@ int cmd_fsck(int argc,
> >       if (check_references)
> >               fsck_refs(the_repository);
> >
> > +     /*
> > +      * Take a snapshot of the refs before walking objects to avoid looking
> > +      * at a set of refs that may be changed by the user while we are walking
> > +      * objects. We can still walk over new objects that are added during the
> > +      * execution of fsck but won't miss any objects that were reachable.
> > +      */
> > +     use_snapshot = !argc;
> > +     if (use_snapshot) {
> > +             now = time(NULL);
> > +             refs_for_each_rawref(get_main_ref_store(the_repository),
> > +                                  snapshot_refs, &default_refs_snapshot);
> > +     }
>
> BTW, one of the reasons I started looking at this is that Coverity
> complained about this segment of code. We set use_snapshot if and only
> if we don't have any argc arguments. And then later...
>
> >       if (!argc) {
> > -             get_default_heads();
> > +             get_default_heads(use_snapshot ? &default_refs_snapshot : NULL);
> >               keep_cache_objects = 1;
> >       }
>
> ...we enter this block only if argc is zero. So we know that
> use_snapshot will be true here, and the NULL path (and thus the fallback
> code in get_default_heads()) will never be used.
>
> That's not wrong exactly, as it's "just" dead code. But it was what led
> me to thinking about whether the case of non-zero argc would benefit
> from the snapshot, too.

> -Peff
>
> PS The other reason I looked at your patch is that I got deja vu from
>    all of this. I thought we had discussed ref snapshotting for fsck
>    before, but I couldn't find anything on the list. It may have been
>    internal GitHub discussions.