Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack

From: Jeff King <peff@peff.net>
Date: Aug 24, 2026, 06:55 UTC
Message-ID: <20260824065539.GA149254@coredump.intra.peff.net>
In-Reply-To: <aovTA4F04aX8SPTU@pks.im>

On Mon, Aug 24, 2026 at 07:13:39AM +0200, Patrick Steinhardt wrote:

Show 14 quoted lines

> On Mon, Aug 24, 2026 at 12:48:22AM -0400, Jeff King wrote:
> > So between the two cases, it sounds like things (or at least the
> > low-level lookups) are working as designed, and there is no bug. Or am I
> > misunderstanding something?
> 
> I agree that QUICK is working as designed, and that callers that pass it
> without being able to accommodate for false negatives are buggy. But the
> patch sent by Elijah still fixes an actual bug where we may not find an
> object that is contained in two MIDXd packs where the preferred pack for
> a respective object vanishes concurrently. Filling the packfile entry
> via the MIDX will fail because the pack vanished, and the lookup via the
> non-preferred pack will fail, too, because we skip over any packs that
> are covered by the MIDX when doing the non-MIDX lookup. Consequently, we
> won't find the object at all.

Ah, OK. I get it now. Thanks for explaining.

It feels like the midx is foiling the usual reprepare strategy (well, SECOND_READ these days) because we don't actually flush it for the second read. Assuming the writing side always generates a new midx (that no longer references the to-be-deleted pack) before deleting the pack itself, then we'd be able to find the object by refreshing the midx. Just like we find new objects by refreshing the pack list and finding the new .idx files.

And I guess that's what the original commit message was saying here:

  This recovers the object without touching the multi-pack-index itself.
  Reloading the stale index would be a more complete fix but would be much
  more involved: other code (pack bitmaps, object name disambiguation)
  borrows and caches the "struct multi_pack_index *" across object reads,
  so freeing it underneath them would be a use-after-free.  Refreshing the
  index with proper invalidation of those borrowers is left for future
  work.

That's not a problem for packs because we _don't_ free the packfile structs. We keep them around forever. So presumably we'd have to do the same for stale midxs. But I agree that it might end up more complicated than we'd like (especially because there's so much "there is only one midx" assumption baked into various parts of the code). So working around it in a more immediate way makes some sense.

> That case is broken no matter whether we pass QUICK or not.

Right. It would be OK to skip Elijah's fallback workaround when SECOND_READ is not set; the QUICK callers are prepared to accept the false negative. But since it is cheap-ish to do the fallback check, it is perhaps OK to just do it on the first pass?

I wonder how true that is. Imagine you had a midx covering a million packs, and you notice an object is missing, but you're in QUICK mode. Do you really want to individually check each of those million pack idx files (that were otherwise not even opened or mmap'd because they're covered by the midx!).

I think it's mostly academic. You'd have to do the million-pack search if we are not in QUICK mode. And the point of QUICK mode is mostly avoiding tons of fruitless searches for objects we don't actually have. The bsearch() conditional means that we _know_ this is a racy negative and not just some object we never even had. So it would trigger generally only when the search is useful.

-Peff