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

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

On Mon, Aug 24, 2026 at 12:48:22AM -0400, Jeff King wrote:

quoted

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