Re: [PATCH v2 3/4] packfile: recover object lookups racing a concurrent repack

From: Elijah Newren <newren@gmail.com>
Date: Aug 27, 2026, 22:23 UTC
Message-ID: <CABPp-BEmReAR-f-aweM=f=5QhRPxG1K-KLTsbyRt2aDQD_QnVA@mail.gmail.com>
In-Reply-To: <20260827055743.GB189659@coredump.intra.peff.net>

On Wed, Aug 26, 2026 at 10:57 PM Jeff King <peff@peff.net> wrote:

Show 29 quoted lines

>
> On Tue, Aug 25, 2026 at 07:00:28PM +0000, Elijah Newren via GitGitGadget wrote:
>
> >   1. open_pack_index() fails, so we print
> >
> >         error: packfile <path> index unavailable
> >
> >      and report the pack as unusable, even though the object still lives
> >      in the replacement pack.
> >
> >   2. A normal lookup recovers: odb_read_object_info_extended() issues a
> >      second read that reloads the on-disk pack state and finds the object
> >      in its new home, making the message above mere noise.  But an
> >      OBJECT_INFO_QUICK lookup deliberately skips that second read to stay
> >      fast on a genuine miss, so it does *not* recover: it reports the
> >      object as absent even though it still lives in the replacement pack.
> >      A resident reader that resolves objects with a QUICK lookup -- such
> >      as the `git mktree --batch` process the tests below drive -- then
> >      produces wrong results.  Even where a spurious miss is not fatal it
> >      is not harmless: `git upload-pack` checks a client's "have" lines
> >      with a QUICK lookup, and a dropped "have" removes a common object
> >      from the negotiation, so the client is sent more than it needs.
>
> Maybe I am still being dense, but this description does not make any
> sense to me at all.
>
> The _point_ of QUICK is to accept those false negatives. It is the right
> thing for upload-pack to do, to avoid re-scans for objects which we
> simply don't have (and don't necessarily expect to have).

It's far more likely that I am the one being dense.  My rough line of thinking:

* We see "packfile ... index unavailable" in our logging
* There's only one thing that remove packfiles
* Investigate the mechanism
* Look for other affected callers (e.g. mktree --batch)
* Consider corrective measures

Steps 1-4 above are probably fine, and step 5 may have been where I went off the rails. My thinking there, wrong or right, was:

* It makes sense that we don't want to reprepare most of the time
* ...but _if_ we know of the existence of some specific packfile in
this process and that packfile has since disappeared by the time we go
to open or read it, is that a special case?  Should it be?

Show 9 quoted lines

> It sounds like mktree is wrong to be using QUICK at all. It comes from
> 817b0f6027 (mktree: do not check type of remote objects, 2022-06-21)
> which rewrote a call to vanilla oid_object_info(). From the description
> there it probably should be using SKIP_FETCH_OBJECT but not QUICK. Or
> possibly it should use neither unless --missing is given.
>
> So I don't see QUICK itself here violating any contract (even if it
> _could_ find the object in some cases with just a little more work, as
> in the case that we were discussing for v1).

I'll drop this patch and instead send a small mktree change that stops passing OBJECT_INFO_QUICK (keeping SKIP_FETCH_OBJECT), so mktree recovers via the normal reprepare like every other non-QUICK reader. That removes the packfile.c changes entirely, so both the reload-under-QUICK hack and the .idx/.pack raciness you noted in pack_index_is_missing() go away with them.