When an object is found in multiple packs that are in a multi-pack-index, and a subsequent geometric repacking creates a new multi-pack-index and removes the pack that was considered the owner of the object in the old multi-pack-index, then an already-running process that had opened the old multi-pack-index and hadn't yet opened the removed packfile will not be able to access the object -- lookups will return it as missing. Additionally, replay has a separate bug where a missing object causes a SIGSEGV rather than an error message.

This appears to affect a very small percentage of git operations in production since it is a tiny window, but I've found evidence of it occurring in at least eight distinct server-side operations, covering seven different git commands:

git operation                        symptom
-----------------------------------  -----------------------------
git replay (server-side rebase)      SIGSEGV (this series, 1/2)
git merge-tree                       spurious read-miss failure
git diff (raw and tree-vs-tree)      spurious read-miss failure
git rev-list --count                 spurious read-miss failure
git merge-base                       spurious read-miss failure
object/rev resolution (rev-parse,    spurious read-miss failure
  cat-file)
repository repair (fsck/repack)      spurious read-miss failure

There are also commands that could be changing behavior without throwing an error -- e.g. object negotiation thinking an object doesn't exist and instead negotiating based on an older common commit, or cat-file --batch reporting that some objects don't exist.

This series fixes the replay bug first, since it's simpler; investigating it, together with my other recent repacking work, is what led me to the underlying multi-pack-index issue that 2/2 addresses.

Elijah Newren (2):
  replay: fail gracefully when a merge input is unreadable
  packfile: recover when a multi-pack-index names a removed pack

 odb/source-packed.c         | 29 +++++++++++++++++++++++++++
 replay.c                    |  7 +++++++
 t/t3650-replay-basics.sh    | 35 ++++++++++++++++++++++++++++++++
 t/t5319-multi-pack-index.sh | 40 +++++++++++++++++++++++++++++++++++++
 4 files changed, 111 insertions(+)

base-commit: 18e66859d87fb4b76599f73460b54f0848c76b16
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2207%2Fnewren%2Fmidx-removed-pack-recovery-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2207/newren/midx-removed-pack-recovery-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2207

From: Elijah Newren <newren@gmail.com>

When objects involved in the merge cannot be read, the merge machinery will return early with result.clean = -1, and result.tree left as NULL. pick_regular_commit() tested only "if (!result->clean)", ignoring the case where "clean < 0". That causes the code to try to use result->tree, resulting in a SIGSEGV.

Handle clean < 0 explicitly; the merge machinery will already have printed messages such as "Could not read <object>" and "collecting merge info failed for trees...", so we don't need to add much detail beyond the fact that the merge failed.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 replay.c                 |  7 +++++++
 t/t3650-replay-basics.sh | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

From: Elijah Newren <newren@gmail.com>

When a geometric repack runs concurrently with other git processes, it can write a new pack and multi-pack-index and then delete older packs that the new one subsumes. One or more of those older packs may have been indexed by the previous multi-pack-index. A process that already had the previous multi-pack-index open keeps using it, and that stale index still records the removed pack(s) as owning some objects.

Because a multi-pack-index attributes each object to exactly one pack, an object that exists in multiple covered packs is served only through its recorded owner. If that owner is the pack a concurrent repack just removed, find_pack_entry() cannot serve the object: fill_midx_entry() routes the lookup to the missing pack (prepare_midx_pack() fails), and the regular pack fallback deliberately skips every multi-pack-index covered pack. The object is reported missing even though a perfectly good copy survives in another covered pack -- for example a large "base" pack that geometric repacking intentionally kept.

The false negative is not limited to one caller. Any reader (cat-file, rev-list, pack-objects, ...) can spuriously fail with "unable to read object", and callers that only ask whether an object exists get a wrong answer too, since the OBJECT_INFO_QUICK path never retries. Writers that merge in-core, such as "git replay", are hit hardest: merge-ort treats the unreadable tree as a premature abort, sets result.clean < 0, and returns without a result tree.

Teach find_pack_entry() to recover. After the normal multi-pack-index lookup and the regular pack fallback both miss, check whether the object is nonetheless present in a covered multi-pack-index (bsearch_midx()). If it is, its recorded owner must have become unavailable, so scan that index's packs directly for a surviving copy. The bsearch gate keeps genuine misses (i.e. objects absent from the index) on the fast path, and because the recovery lives in find_pack_entry() itself it also fixes the OBJECT_INFO_QUICK callers that never reprepare.

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 odb/source-packed.c         | 29 +++++++++++++++++++++++++++
 t/t5319-multi-pack-index.sh | 40 +++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

Hmph, so anything but "0 < result->clean" is a failure, but we by mistake took any non-zero value as OK? That is an obvious mistake. Well spotted and fixed.

"test_must_fail" means "the tested command must fail voluntarily and in a controlled way", so a segfaulting git-replay invocation would not pass test_must_fail. Hence, there is no need to separately test "test_grep ! '[sS]egmentation'".

Besides, the spelling used by strsignal() is implementation-defined, so you cannot reliably grep for it anyway.

Thanks.

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

I'll prepare an evil-merge to rewrite this line to

			if (p && packfile_fill_entry(p, oid, e, bad_pack))

to adjust to the API change another topic in-flight brings in when merging these patches to 'seen'.

This is strictly FYI. You do not need to rebase on top of the other topic, until I and/or the author of the other topic ask you.

Thanks.

On Tue, Aug 18, 2026 at 10:34:06PM +0000, Elijah Newren via GitGitGadget wrote:

Okay. Rephrasing in my own words: the object in question exists in two packs covered by the MIDX. We rewrite one of those two packs, and the MIDX used to reference the object via the pack we're about to rewrite. Consequently, the MIDX is stale now and it cannot be used to find the object anymore because its pack has disappeared. And as we know to skip searching packfiles for the object that are already covered by the MIDX we won't be able to find it via the second packfile, either.

Hm. Isn't there a slight variant of the race though for any caller that does not use OBJECT_INFO_QUICK?

Namely, the packfile containing our object disappears and is being written to a new packfile, and that file is the only one containing it. Without OBJECT_INFO_QUICK we would be fine: we notice the object could not be found, and then we perform a second read that makes the "packed" backend reload its packfiles. It would find the new packfile, and because it's not covered by its MIDX it would use it to surface the object. But without OBJECT_INFO_QUICK that's not the case, as we would skip reloading packfiles altogether, and hence we would not be able to find that object at all.

As far as I can see though, we don't seem to pass OBJECT_INFO_QUICK in any of the mentioned readers. I could very well be missing something here, but I would have thought that those readers are fine in this scenario?

Okay. I was initially worried that we now unconditionally search through all packfiles a second time, as that could have an impact on performance. But we really only do this in case we have a MIDX and we know that the MIDX _should_ have contained the object, but didn't yield it.

And here we now loop through all packs covered by the MIDX and manually try to look up the object in those. Makes sense.

I was wondering whether a preferable fix would be to eagerly load any packfile referenced by the MIDX when loading the MIDX itself. And if that fails, we'd ignore the MIDX altogether. This would guarantee that the MIDX remains valid, and we wouldn't have to worry about any disappearing packfiles.

The downside is of course that we now eagerly open packfiles, and we didn't have to do that before. So I think your fix is preferable, as we can rather easily detect the case where the MIDX should've yielded the object but didn't, and consequently the additional search only triggers in very specific edge cases.

Overall I think this patch looks good to me. The one thing that I'm a bit puzzled about is the above discussion around OBJECT_INFO_QUICK. I feel like I'm missing something there.

Thanks!

Patrick

On Thu, Aug 20, 2026 at 12:54 AM Patrick Steinhardt <ps@pks.im> wrote:

Yep.

Nicely caught -- and you're right that the readers named above are fine: they're all non-QUICK, so the second read reloads the packfiles and finds the object in its new, non-MIDX-covered home, exactly as you describe.

But the variant you describe is a real bug for QUICK callers that don't get that second read -- e.g. upload-pack's object-existence checks and mktree --batch. I have three more race-condition patches to clean up and submit, and this is one of them: it forces the reload even under OBJECT_INFO_QUICK once we notice a pack has vanished out from under us.

Your wording also makes me realize that my fix in this unsubmitted patch still has a hole: it triggers when opening the pack .idx fails, but if the timing is such that the .idx is already mmapped and only the .pack has gone missing, it won't fire. I'll look into that before submitting...and then clean up/submit my two other race fixes as well.

On Wed, Aug 19, 2026 at 11:09 AM Junio C Hamano <gitster@pobox.com> wrote:

Thanks, but the bug was also caused by me -- e787e664da64 (replay: introduce pick_regular_commit(), 2023-11-24) -- so not sure I should get much credit for finding it three years later.

Oops, you're right.

You said on 2/2 that I don't need to rebase because you're putting together an evil merge. Do you want me to resubmit with this line removed (without changing the series' base), or would you rather I avoid that to prevent merging work for you?

Elijah Newren <newren@gmail.com> writes:

I can remove that line myself, or you can resubmit on the same base. The evil-merge machinery uses the usual 3-way merge, so I do not think removal of that "test_grep !" line would break it either way.

Thanks.

On Thu, Aug 20, 2026 at 06:36:09PM -0700, Elijah Newren wrote:

OK, so do I understand correctly that you _can't_ get the "unable to read object" result that the commit message claims? I.e., the reprepare / packfile reload is helps us (just like it does for the non-midx case when an idx has been mapped but the pack disappears before we open it).

So there is no bug there for non-QUICK callers. But then...

This seems wrong. The whole point of the QUICK flag is that the caller is OK producing a false negative for an object lookup, and it would prefer that outcome to spending the time to reload. If there are callers passing QUICK that aren't OK with false negatives, they are broken and the fix should be there. But repreparing the packs for a QUICK miss is going to reintroduce the performance problems that QUICK was introduced to help.

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 think it would be fine, for the same reason that regular idx lookups are fine. In packfile_fill_entry() we call is_pack_valid(), checking that the pack is still there (and relying on its side effect of leaving the fd/mmap open so that it remains accessible even if the file is deleted).

-Peff

On Tue, Aug 18, 2026 at 10:34:06PM +0000, Elijah Newren via GitGitGadget wrote:

You don't even have to pay the bsearch() again. We'd already have looked in the midx earlier in the function. We just need to distinguish three cases:

  1. it was not in the midx (or there is no midx)

  2. it was in the midx but we could not load it (pack invalid, or
     object in the bad_objects list)

  3. it was in the midx and is available

In fill_midx_entry() we return a boolean that lumps cases 1+2 together, versus case 3. It could return a tri-state that would let us distinguish all three. And then your fallback would kick in only for case 2 (case 3 already returned with success, and case 1 means the midx does not even mention the object).

This is all assuming the fallback is worth pursuing. I'm still puzzled why this specific case would matter when we have the same (already solved) problem of reading a regular .idx whose .pack has gone away.

-Peff

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

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.

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

Patrick

On Mon, Aug 24, 2026 at 12:55:29AM -0400, Jeff King wrote:

I've tried to clarify in a parallel message already, but the issue is that we skip over any packfiles that covered by a MIDX when doing the lookup. So any secondary packfiles that contain the object would be completely ignored, and that's why we don't find the object there.

But this mail here suggests an alternative fix: instead of re-scanning all packfiles like the patch proposes, wouldn't the proper fix be to not ignore _all_ MIDX'd packs, but only the pack that _should_ have contained the object?

Ultimately though, this would be equivalent to turning the function's return value into a tri-state as suggested by Peff here. The only case where the issue can occur is in case (2), and in that case we should not skip MIDX'd packs at all as the MIDX'd pack that should've contained the pack does not exist anyway.

Patrick

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

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.

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

On Mon, Aug 24, 2026 at 07:40:22AM +0200, Patrick Steinhardt wrote:

Yes, thanks. Your other message cleared it up for me.

Do you mean in the main code path, or in the fallback?

In the main code path we definitely don't want to do this. Imagine we have a midx that covers a million packs, and says object X is in pack P. A simultaneous writer deletes P and rewrites the midx, and the object is now in a new pack Q (which might be covered by the new midx, but we don't know because we're working with the stale one).

We definitely want to look in Q for the object after the midx can't find it. But we probably don't want to immediately search in the other million midx packs. Most objects won't have such a duplicate and the search is fruitless.

-Peff

On Mon, Aug 24, 2026 at 02:55:39AM -0400, Jeff King wrote:

Actually, thinking on this more: we _don't_ usually scan the million packs for an object we actually have. If the object is available in a new pack, the SECOND_READ scan should find that pack and put it at the front of the packfile list (because they sort by reverse mtime), and we'd find the object immediately, without having to open the new packs.

It's only the case that this patch is helping (when the object is not moved at all, but an existing duplicate is hidden in the midx) where we have to re-scan all of those packs. But we don't know which case is which until we get to the SECOND_READ stage. So I think this probably should only kick in for SECOND_READ.

-Peff

On Mon, Aug 24, 2026 at 03:06:01AM -0400, Jeff King wrote:

Er, this final sentence should be "without having to open the (million) old packs".

-Peff

On 8/18/2026 6:34 PM, Elijah Newren via GitGitGadget wrote:

This kind of race is why 'git multi-pack-index expire' exists, to delete packfiles whose objects are all referenced within other packfiles. The inclusion of these "stale" packs in the multi-pack-index helps halt reads of those packfiles by new processes while allowing them to be read by existing processes.

This is currently used in the incremental repacks done by 'git multi-pack-index repack' and maybe could be used again in this kind of geometric repack.

(This dance is more important on Windows platforms where read handles prevent deletions, so it's common to have a foreground operation prevent a packfile deletion in background maintenance.)

I do think your attempts to be more robust to missing packs is good, but the comment thread does show that it's a complicated situation that we may want to avoid whenever possible. Leaving some redundant data around for some time interval can reduce the number of times that the fallback logic is triggered.

Thanks, -Stolee

On 8/18/2026 6:34 PM, Elijah Newren via GitGitGadget wrote:

This kind of race is why 'git multi-pack-index expire' exists, to delete packfiles whose objects are all referenced within other packfiles. The inclusion of these "stale" packs in the multi-pack-index helps halt reads of those packfiles by new processes while allowing them to be read by existing processes.

This is currently used in the incremental repacks done by 'git multi-pack-index repack' and maybe could be used again in this kind of geometric repack.

(This dance is more important on Windows platforms where read handles prevent deletions, so it's common to have a foreground operation prevent a packfile deletion in background maintenance.)

I do think your attempts to be more robust to missing packs is good, but the comment thread does show that it's a complicated situation that we may want to avoid whenever possible. Leaving some redundant data around for some time interval can reduce the number of times that the fallback logic is triggered.

Thanks, -Stolee

On Sun, Aug 23, 2026 at 9:55 PM Jeff King <peff@peff.net> wrote:

You know, I considered putting the logic in fill_midx_entry() as well as putting where it is. You'd think based on that, that I'd have thought about just changing fill_midx_entry()'s return type to get the best of both worlds. You'd be wrong though. ;-)

This sounds much nicer; I adopted it and made fill_midx_entry() return MIDX_FILL_MISS / MIDX_FILL_HIT / MIDX_FILL_OWNER_UNAVAILABLE in v2.

On Mon, Aug 24, 2026 at 12:06 AM Jeff King <peff@peff.net> wrote:

I implemented that in v2.

On Mon, Aug 24, 2026 at 7:45 AM Derrick Stolee <stolee@gmail.com> wrote:

Oh, good pointer. It may make sense to teach geometric repacking about "git multi-pack-index expire", which I think would be complementary and reduce how often we fall into recovery, while the changes in this patch help keep us correct when we do fall into recovery.

Changes since v1:

 * Rebased on top of ps/odb-generic-corrupt-objects, and conflicts with it
   resolved
 * Removed useless test_grep line spotted by Junio in PATCH 1
 * Switched fill_midx_entry() to a tri-state to avoid duplicate
   bsearch_midx(), as suggested by Peff
 * Only do the re-read on SECOND_READ, as suggested by Peff
 * Handle multiple objects shared across multiple packs correctly (issue
   caught & corrected & new testcase by deeper AI review)
 * Inserted two new patches:
   * 2/4: Fix a leak in git mktree --batch since I use it in new testcases
     and don't want the *-leaks jobs failing
   * 3/4: Demonstrate and fix QUICK reader problems, while keeping expected
     QUICK performance for normal cases (we've already been discussing this
     patch in this thread a bunch anyway, and it's logically related)

Cover letter addendum/update:

A geometric repack writes a new pack plus multi-pack-index and then deletes the packs the new one subsumes. Readers running alongside it can be told an object is missing when it is in fact still present. The v1 series fixed one race of this shape (the object didn't move and was in a second pack referenced by the multi-pack-index); v2 added a new patch fixing others in the same class but of a different shape (the object moved to a brand new pack).

Note here that Stolee's suggestion to defer pack deletion via git multi-pack-index expire seems like a good complementary mitigation; it would reduce how often we fall into recovery, while this series tries to fix recovery to work more robustly.

Original cover letter (focused on the final patch):

When an object is found in multiple packs that are in a multi-pack-index, and a subsequent geometric repacking creates a new multi-pack-index and removes the pack that was considered the owner of the object in the old multi-pack-index, then an already-running process that had opened the old multi-pack-index and hadn't yet opened the removed packfile will not be able to access the object -- lookups will return it as missing. Additionally, replay has a separate bug where a missing object causes a SIGSEGV rather than an error message.

This appears to affect a very small percentage of git operations in production since it is a tiny window, but I've found evidence of it occurring in at least eight distinct server-side operations, covering seven different git commands:

git operation                        symptom
-----------------------------------  -----------------------------
git replay (server-side rebase)      SIGSEGV (this series, 1/2)
git merge-tree                       spurious read-miss failure
git diff (raw and tree-vs-tree)      spurious read-miss failure
git rev-list --count                 spurious read-miss failure
git merge-base                       spurious read-miss failure
object/rev resolution (rev-parse,    spurious read-miss failure
  cat-file)
repository repair (fsck/repack)      spurious read-miss failure

There are also commands that could be changing behavior without throwing an error -- e.g. object negotiation thinking an object doesn't exist and instead negotiating based on an older common commit, or cat-file --batch reporting that some objects don't exist.

This series fixes the replay bug first, since it's simpler; investigating it, together with my other recent repacking work, is what led me to the underlying multi-pack-index issue that 2/2 addresses.

Elijah Newren (4):
  replay: fail gracefully when a merge input is unreadable
  mktree: plug per-tree leak in --batch mode
  packfile: recover object lookups racing a concurrent repack
  packfile: recover when a multi-pack-index names a removed pack

 builtin/mktree.c              |   3 +
 builtin/pack-objects.c        |   2 +-
 midx.c                        |  44 ++++++----
 midx.h                        |  21 ++++-
 odb.c                         |   8 +-
 odb.h                         |  16 +++-
 odb/source-packed.c           |  51 ++++++++++--
 packfile.c                    |  39 ++++++++-
 replay.c                      |   7 ++
 t/meson.build                 |   1 +
 t/t3650-replay-basics.sh      |  34 ++++++++
 t/t5319-multi-pack-index.sh   |  80 ++++++++++++++++++
 t/t5336-repack-reader-race.sh | 148 ++++++++++++++++++++++++++++++++++
 13 files changed, 423 insertions(+), 31 deletions(-)
 create mode 100755 t/t5336-repack-reader-race.sh

base-commit: 2135b14863642bbcec02996e7f5e54ac1f77b03a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2207%2Fnewren%2Fmidx-removed-pack-recovery-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2207/newren/midx-removed-pack-recovery-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2207

Range-diff vs v1:

 1:  321af575e0 ! 1:  36bf2ce17b replay: fail gracefully when a merge input is unreadable
     @@ t/t3650-replay-basics.sh: test_expect_success '--onto with --ref rejects multipl
      +
      +		# Ensure replay gracefully handles the missing object
      +		test_must_fail git replay --onto onto base..side 2>err &&
     -+		test_grep ! "[Ss]egmentation" err &&
     -+		test_grep "Could not read\|collecting merge info failed" err
     ++		test_grep -e "Could not read" -e "collecting merge info failed" err
      +	)
      +'
      +
 -:  ---------- > 2:  3f3b75690e mktree: plug per-tree leak in --batch mode
 -:  ---------- > 3:  fc98f48ddb packfile: recover object lookups racing a concurrent repack
 2:  5792c08f4e ! 4:  eacf6ba4b1 packfile: recover when a multi-pack-index names a removed pack
     @@ Metadata
       ## Commit message ##
          packfile: recover when a multi-pack-index names a removed pack
      
     -    When a geometric repack runs concurrently with other git processes, it
     -    can write a new pack and multi-pack-index and then delete older packs
     -    that the new one subsumes.  One or more of those older packs may have
     -    been indexed by the previous multi-pack-index.  A process that already
     -    had the previous multi-pack-index open keeps using it, and that stale
     -    index still records the removed pack(s) as owning some objects.
     +    A geometric repack writes a new pack and multi-pack-index and then
     +    deletes the packs the new one subsumes.  A process still using the
     +    previous MIDX keeps seeing a removed pack listed as the owner of some
     +    objects.  Since a MIDX attributes each object to exactly one pack, such
     +    an object is served only through its recorded owner; if that owner was
     +    just removed, find_pack_entry() cannot serve it -- fill_midx_entry()
     +    routes to the missing pack, and the regular pack fallback deliberately
     +    skips every MIDX-covered pack, so a surviving copy in another covered
     +    pack (e.g. a kept base pack) is never consulted.
      
     -    Because a multi-pack-index attributes each object to exactly one pack,
     -    an object that exists in multiple covered packs is served only through
     -    its recorded owner.  If that owner is the pack a concurrent repack just
     -    removed, find_pack_entry() cannot serve the object: fill_midx_entry()
     -    routes the lookup to the missing pack (prepare_midx_pack() fails), and
     -    the regular pack fallback deliberately skips every multi-pack-index
     -    covered pack.  The object is reported missing even though a perfectly
     -    good copy survives in another covered pack -- for example a large "base"
     -    pack that geometric repacking intentionally kept.
     +    Unlike the ordinary "a pack's .idx is mapped but its .pack is gone"
     +    race, the second read does not rescue us -- and not only for
     +    OBJECT_INFO_QUICK callers.  Reloading the on-disk pack set does not
     +    reload the borrowed, cached MIDX (freeing it under the code that caches
     +    the "struct multi_pack_index *" would be a use-after-free), so the stale
     +    MIDX keeps routing to the removed pack and the surviving copy stays
     +    hidden behind the covered-pack skip.  cat-file, rev-list and pack-objects
     +    can thus all spuriously fail with "unable to read object".
      
     -    The false negative is not limited to one caller.  Any reader
     -    (cat-file, rev-list, pack-objects, ...) can spuriously fail with
     -    "unable to read object", and callers that only ask whether an object
     -    exists get a wrong answer too, since the OBJECT_INFO_QUICK path never
     -    retries.  Writers that merge in-core, such as "git replay", are hit
     -    hardest: merge-ort treats the unreadable tree as a premature abort, sets
     -    result.clean < 0, and returns without a result tree.
     +    Teach find_pack_entry() to recover.  fill_midx_entry() now returns a
     +    tri-state, distinguishing "absent from the MIDX" from "present but the
     +    owning pack is unavailable"; in the latter case, once the regular
     +    fallback has also missed, scan the MIDX's packs directly for a surviving
     +    copy.
      
     -    Teach find_pack_entry() to recover.  After the normal multi-pack-index
     -    lookup and the regular pack fallback both miss, check whether the object
     -    is nonetheless present in a covered multi-pack-index (bsearch_midx()).
     -    If it is, its recorded owner must have become unavailable, so scan that
     -    index's packs directly for a surviving copy.  The bsearch gate keeps
     -    genuine misses (i.e. objects absent from the index) on the fast path, and
     -    because the recovery lives in find_pack_entry() itself it also fixes the
     -    OBJECT_INFO_QUICK callers that never reprepare.
     +    Do the scan only on the second read (OBJECT_INFO_SECOND_READ): by then
     +    the cheaper on-disk reload has run, so an object merely relocated into a
     +    new (non-covered) pack has already been found by the regular fallback,
     +    and only a genuine hidden duplicate reaches the rescan.  QUICK callers
     +    that would skip the second read are steered into it by the preceding
     +    commit's stale_packs_detected flag, which prepare_midx_pack() sets when
     +    it cannot open the owning pack.
      
     -    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.
     +    Reloading the stale MIDX would be a more complete fix but is much more
     +    involved (the borrowers above need proper invalidation), so leave that
     +    for later.
      
     +    Assisted-by: Claude Opus 4.8 & GPT-5.6 Sol
     +    Helped-by: Jeff King <peff@peff.net>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
     + ## builtin/pack-objects.c ##
     +@@ builtin/pack-objects.c: static int want_object_in_pack_mtime(const struct object_id *oid,
     + 		struct multi_pack_index *m = get_multi_pack_index(files->packed);
     + 		struct pack_entry e;
     + 
     +-		if (m && fill_midx_entry(m, oid, &e, NULL)) {
     ++		if (m && fill_midx_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {
     + 			want = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);
     + 			if (want != -1)
     + 				return want;
     +
     + ## midx.c ##
     +@@ midx.c: uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos)
     + 					       (off_t)pos * MIDX_CHUNK_OFFSET_WIDTH);
     + }
     + 
     +-int fill_midx_entry(struct multi_pack_index *m,
     +-		    const struct object_id *oid,
     +-		    struct pack_entry *e,
     +-		    struct packed_git **bad_pack)
     ++enum midx_fill_result fill_midx_entry(struct multi_pack_index *m,
     ++				      const struct object_id *oid,
     ++				      struct pack_entry *e,
     ++				      struct packed_git **bad_pack)
     + {
     + 	uint32_t pos;
     + 	uint32_t pack_int_id;
     + 	struct packed_git *p;
     + 
     + 	if (!bsearch_midx(oid, m, &pos))
     +-		return 0;
     ++		return MIDX_FILL_MISS;
     + 
     + 	midx_for_object(&m, pos);
     + 	pack_int_id = nth_midxed_pack_int_id(m, pos);
     + 
     + 	if (prepare_midx_pack(m, pack_int_id))
     +-		return 0;
     ++		goto owner_unavailable;
     + 	p = m->packs[pack_int_id - m->num_packs_in_base];
     + 
     +-	/*
     +-	* We are about to tell the caller where they can locate the
     +-	* requested object.  We better make sure the packfile is
     +-	* still here and can be accessed before supplying that
     +-	* answer, as it may have been deleted since the MIDX was
     +-	* loaded!
     +-	*/
     ++	/* Make sure the pack is still present before pointing at it. */
     + 	if (!is_pack_valid(p))
     +-		return 0;
     ++		goto owner_unavailable;
     + 
     + 	if (oidset_size(&p->bad_objects) &&
     + 	    oidset_contains(&p->bad_objects, oid)) {
     + 		if (bad_pack && !*bad_pack)
     + 			*bad_pack = p;
     +-		return 0;
     ++		return MIDX_FILL_MISS;
     + 	}
     + 
     + 	e->offset = nth_midxed_offset(m, pos);
     + 	e->p = p;
     + 
     +-	return 1;
     ++	return MIDX_FILL_HIT;
     ++
     ++owner_unavailable:
     ++	/*
     ++	 * Re-arm stale_packs_detected on every such lookup, not just the
     ++	 * first: prepare_midx_pack() caches the failure, so without this a
     ++	 * later lookup of the same vanished pack would leave the flag clear
     ++	 * and a QUICK reader would skip its recovering second read.
     ++	 */
     ++	m->source->base.odb->stale_packs_detected = 1;
     ++	return MIDX_FILL_OWNER_UNAVAILABLE;
     + }
     + 
     + /* Match "foo.idx" against either "foo.pack" _or_ "foo.idx". */
     +@@ midx.c: int verify_midx_file(struct odb_source_packed *source, unsigned flags)
     + 
     + 		nth_midxed_object_oid(&oid, m, pairs[i].pos);
     + 
     +-		if (!fill_midx_entry(m, &oid, &e, NULL)) {
     ++		if (fill_midx_entry(m, &oid, &e, NULL) != MIDX_FILL_HIT) {
     + 			midx_report(_("failed to load pack entry for oid[%d] = %s"),
     + 				    pairs[i].pos, oid_to_hex(&oid));
     + 			continue;
     +
     + ## midx.h ##
     +@@ midx.h: uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos);
     + struct object_id *nth_midxed_object_oid(struct object_id *oid,
     + 					struct multi_pack_index *m,
     + 					uint32_t n);
     +-int fill_midx_entry(struct multi_pack_index *m, const struct object_id *oid,
     +-		    struct pack_entry *e, struct packed_git **bad_pack);
     ++/*
     ++ * Result of looking an object up in a multi-pack-index.  MIDX_FILL_HIT means
     ++ * "e was filled in"; the two miss variants distinguish an object the midx does
     ++ * not know about (MIDX_FILL_MISS) from one it does know about but whose owning
     ++ * pack we can no longer open (MIDX_FILL_OWNER_UNAVAILABLE -- the signature of a
     ++ * concurrent repack having removed that pack).  A known-bad (corrupt) object
     ++ * reports MIDX_FILL_MISS but also sets *bad_pack, if provided, to the owning
     ++ * pack so the caller can tell "corrupt" apart from "absent".
     ++ */
     ++enum midx_fill_result {
     ++	MIDX_FILL_MISS = 0,
     ++	MIDX_FILL_HIT,
     ++	MIDX_FILL_OWNER_UNAVAILABLE,
     ++};
     ++
     ++enum midx_fill_result fill_midx_entry(struct multi_pack_index *m,
     ++				      const struct object_id *oid,
     ++				      struct pack_entry *e,
     ++				      struct packed_git **bad_pack);
     + int midx_contains_pack(struct multi_pack_index *m,
     + 		       const char *idx_or_pack_name);
     + int midx_layer_contains_pack(struct multi_pack_index *m,
     +
       ## odb/source-packed.c ##
     +@@
     + static int find_pack_entry(struct odb_source_packed *store,
     + 			   const struct object_id *oid,
     + 			   struct pack_entry *e,
     ++			   enum object_info_flags flags,
     + 			   struct packed_git **bad_pack)
     + {
     + 	struct packfile_list_entry *l;
     ++	enum midx_fill_result midx_result = MIDX_FILL_MISS;
     + 
     + 	odb_source_prepare(&store->base, 0);
     +-	if (store->midx && fill_midx_entry(store->midx, oid, e, bad_pack))
     +-		return 1;
     ++	if (store->midx) {
     ++		midx_result = fill_midx_entry(store->midx, oid, e, bad_pack);
     ++		if (midx_result == MIDX_FILL_HIT)
     ++			return 1;
     ++	}
     + 
     + 	for (l = store->packs.head; l; l = l->next) {
     + 		struct packed_git *p = l->pack;
      @@ odb/source-packed.c: static int find_pack_entry(struct odb_source_packed *store,
       		}
       	}
       
      +	/*
     -+	 * Recovery for a concurrent-repack race: a MIDX can name an owning
     -+	 * pack for an object that a simultaneous repack has since deleted,
     -+	 * even though the object still exists in another pack the same MIDX
     -+	 * covers (e.g. a kept base pack that geometric repack did not rewrite).
     -+	 * If the object is present in a MIDX yet none of the paths above could
     -+	 * serve it, its recorded owning pack has become unavailable.  The
     -+	 * regular fallback above deliberately skips MIDX-covered packs, so
     -+	 * scan this MIDX's packs directly to find the surviving copy.  The
     -+	 * bsearch gate keeps genuine misses (objects absent from the MIDX) on
     -+	 * the fast path.
     ++	 * Recovery for a concurrent-repack race: a stale MIDX may still name a
     ++	 * vanished owning pack even though the object survives in another pack
     ++	 * the same MIDX covers.  The regular fallback above skips MIDX-covered
     ++	 * packs, and repreparing the on-disk pack set does not reload the
     ++	 * borrowed, cached MIDX, so scan its packs directly for the survivor.
     ++	 *
     ++	 * Do this only on the second read, by which point repreparing packs has
     ++	 * already had a chance to find an object merely relocated into a new,
     ++	 * uncovered pack; only a genuine hidden duplicate reaches here.
      +	 */
     -+	if (store->midx) {
     ++	if (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&
     ++	    (flags & OBJECT_INFO_SECOND_READ)) {
      +		struct multi_pack_index *m = store->midx;
     -+		uint32_t midx_pos, i;
     -+
     -+		if (bsearch_midx(oid, m, &midx_pos)) {
     -+			for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {
     -+				struct packed_git *p;
     -+
     -+				if (prepare_midx_pack(m, i))
     -+					continue;
     -+				p = nth_midxed_pack(m, i);
     -+				if (p && packfile_fill_entry(p, oid, e))
     -+					return 1;
     -+			}
     ++		uint32_t i;
     ++
     ++		for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {
     ++			struct packed_git *p;
     ++
     ++			if (prepare_midx_pack(m, i))
     ++				continue;
     ++			p = nth_midxed_pack(m, i);
     ++			if (p && packfile_fill_entry(p, oid, e, bad_pack))
     ++				return 1;
      +		}
      +	}
      +
       	return 0;
       }
       
     +@@ odb/source-packed.c: static enum odb_read_status odb_source_packed_read_object_info(struct odb_source
     + 	if (flags & OBJECT_INFO_SECOND_READ)
     + 		odb_source_prepare(source, ODB_PREPARE_FLUSH_CACHES);
     + 
     +-	if (!find_pack_entry(packed, oid, &e, &bad_pack)) {
     ++	if (!find_pack_entry(packed, oid, &e, flags, &bad_pack)) {
     + 		/*
     + 		 * The lookup may have failed because the object is known to be
     + 		 * corrupt in one of the packfiles. Report the object as
     +@@ odb/source-packed.c: static int odb_source_packed_read_object_stream(struct odb_read_stream **out,
     + 	struct odb_source_packed *packed = odb_source_packed_downcast(source);
     + 	struct pack_entry e;
     + 
     +-	if (!find_pack_entry(packed, oid, &e, NULL))
     ++	if (!find_pack_entry(packed, oid, &e, 0, NULL))
     + 		return -1;
     + 
     + 	return packfile_read_object_stream(out, oid, e.p, e.offset);
     +@@ odb/source-packed.c: static int odb_source_packed_freshen_object(struct odb_source *source,
     + 		timesp = &times;
     + 	}
     + 
     +-	if (!find_pack_entry(packed, oid, &e, NULL))
     ++	if (!find_pack_entry(packed, oid, &e, 0, NULL))
     + 		return 0;
     + 	if (e.p->is_cruft)
     + 		return 0;
      
       ## t/t5319-multi-pack-index.sh ##
      @@ t/t5319-multi-pack-index.sh: test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '
     @@ t/t5319-multi-pack-index.sh: test_expect_success 'pack.preferBitmapTips interpre
      +		test_cmp expect actual
      +	)
      +'
     ++
     ++test_expect_success 'repeated QUICK lookups recover after owning pack removed' '
     ++	test_when_finished "rm -fr repo" &&
     ++	git init repo &&
     ++	(
     ++		cd repo &&
     ++
     ++		# Two blobs, each duplicated across packs so the midx must pick
     ++		# an owning pack, and each attributed to the same moderate pack.
     ++		echo one >f1 &&
     ++		echo two >f2 &&
     ++		git add f1 f2 &&
     ++		git commit -m dups &&
     ++		d1=$(git rev-parse HEAD:f1) &&
     ++		d2=$(git rev-parse HEAD:f2) &&
     ++
     ++		# Roll every object, including d1 and d2, into one big pack,
     ++		# then build a moderate pack that also holds both blobs.
     ++		git repack -adq &&
     ++		moderate=$(printf "%s\n%s\n" "$d1" "$d2" |
     ++			git pack-objects --quiet $objdir/pack/pack) &&
     ++
     ++		git multi-pack-index write \
     ++			--preferred-pack="pack-$moderate.idx" &&
     ++
     ++		# Retire the moderate pack; the stale midx still names it as the
     ++		# owner of both blobs, each of which survives in the big pack.
     ++		rm -f $objdir/pack/pack-$moderate.* &&
     ++
     ++		# One resident QUICK reader ("git mktree --batch") resolves both
     ++		# blobs.  The first lookup recovers d1 and caches the owning
     ++		# packs failure; unless that failure keeps re-arming the second
     ++		# read, the lookup of d2 skips its recovering read and the reader
     ++		# dies reporting d2 as missing.
     ++		printf "100644 blob %s\tf1\n\n100644 blob %s\tf2\n\n" \
     ++			"$d1" "$d2" |
     ++			git mktree --batch >trees &&
     ++		test_line_count = 2 trees
     ++	)
     ++'
      +
       test_done

From: Elijah Newren <newren@gmail.com>

When objects involved in the merge cannot be read, the merge machinery will return early with result.clean = -1, and result.tree left as NULL. pick_regular_commit() tested only "if (!result->clean)", ignoring the case where "clean < 0". That causes the code to try to use result->tree, resulting in a SIGSEGV.

Handle clean < 0 explicitly; the merge machinery will already have printed messages such as "Could not read <object>" and "collecting merge info failed for trees...", so we don't need to add much detail beyond the fact that the merge failed.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 replay.c                 |  7 +++++++
 t/t3650-replay-basics.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

From: Elijah Newren <newren@gmail.com>

In --batch mode "git mktree" reuses its entry buffer across trees, resetting `used` to 0 after writing each tree. It never frees the `treeent` structures the previous tree appended, though, so once the next tree overwrites those slots the earlier allocations are leaked. A single-tree invocation hides this, as the entries stay reachable through the `entries` global until exit.

Free each entry when resetting the buffer, and free the buffer itself before returning.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/mktree.c | 3 +++
 1 file changed, 3 insertions(+)

From: Elijah Newren <newren@gmail.com>

When a reader opens a pack it discovered on disk, open_packed_git_1() first mmaps the pack's `.idx`. A `git repack` running alongside us consolidates existing packs into a new one and then removes the redundant packs, deleting each pack's `.idx` before its `.pack` (see the ordering in unlink_pack_path()). A reader that had just enumerated one of those packs -- most easily through a multi-pack-index -- can race with the removal and find the pack gone.

Two things go wrong in that window:

  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.

Recovering without giving up that speed is the trick: we keep QUICK's fast path for a genuine miss and force the extra read only when a pack we were already using has provably vanished.

Fix both. Record that a pack disappeared out from under us by setting object_database.stale_packs_detected at the three points where a reader can notice a pack vanish beneath it:

  - In open_packed_git_1(), when open_pack_index() fails because the
    index simply vanished (its open fails with ENOENT).  Here we also
    stay silent instead of printing "index unavailable"; a genuinely
    unreadable index that is still present keeps the error, since that is
    a real problem worth surfacing.

  - In open_packed_git_1() again, from the other side of the race: when
    the `.idx` was already mapped -- so open_pack_index() returns without
    touching the filesystem -- yet opening the `.pack` fails with ENOENT.
    A reader that prepared its pack list before the repack only trips
    over the removal when it finally opens the pack file.

  - In prepare_midx_pack(), when packfile_store_load_pack() cannot open a
    pack the midx still references at all.  If both the `.idx` and the
    `.pack` are already gone -- as happens when the redundant pack is
    removed outright rather than index-first -- we never reach
    open_pack_index(), so this is the only place the vanished pack is
    observed.

Then, in odb_read_object_info_extended(), issue the second read -- which asks the sources to reload their on-disk state (for packs, a reprepare) and retry -- not only for non-QUICK lookups but also whenever stale_packs_detected is set, even under OBJECT_INFO_QUICK. An ordinary QUICK miss, with no vanished pack, still skips the second read and stays fast; we pay for the rescan only when we have positive evidence that the on-disk pack set changed beneath us. The flag is reset when the packfiles are reprepared, in odb_source_packed_prepare().

Add t5336, regression tests that reproduce the race deterministically: they drive a resident `git mktree --batch` reader -- which resolves each tree entry with OBJECT_INFO_QUICK -- across both removal windows, one removing a pack's `.idx` first while a midx routes the lookup to the doomed pack, the other removing a pack's `.pack` after its `.idx` was already mapped. Each confirms the reader recovers the relocated object instead of dying.

Assisted-by: Claude Opus 4.8 & GPT-5.6 Sol
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 midx.c                        |   6 ++
 odb.c                         |   8 +-
 odb.h                         |  16 +++-
 odb/source-packed.c           |   9 ++-
 packfile.c                    |  39 ++++++++-
 t/meson.build                 |   1 +
 t/t5336-repack-reader-race.sh | 148 ++++++++++++++++++++++++++++++++++
 7 files changed, 221 insertions(+), 6 deletions(-)
 create mode 100755 t/t5336-repack-reader-race.sh

From: Elijah Newren <newren@gmail.com>

A geometric repack writes a new pack and multi-pack-index and then deletes the packs the new one subsumes. A process still using the previous MIDX keeps seeing a removed pack listed as the owner of some objects. Since a MIDX attributes each object to exactly one pack, such an object is served only through its recorded owner; if that owner was just removed, find_pack_entry() cannot serve it -- fill_midx_entry() routes to the missing pack, and the regular pack fallback deliberately skips every MIDX-covered pack, so a surviving copy in another covered pack (e.g. a kept base pack) is never consulted.

Unlike the ordinary "a pack's .idx is mapped but its .pack is gone" race, the second read does not rescue us -- and not only for OBJECT_INFO_QUICK callers. Reloading the on-disk pack set does not reload the borrowed, cached MIDX (freeing it under the code that caches the "struct multi_pack_index *" would be a use-after-free), so the stale MIDX keeps routing to the removed pack and the surviving copy stays hidden behind the covered-pack skip. cat-file, rev-list and pack-objects can thus all spuriously fail with "unable to read object".

Teach find_pack_entry() to recover. fill_midx_entry() now returns a tri-state, distinguishing "absent from the MIDX" from "present but the owning pack is unavailable"; in the latter case, once the regular fallback has also missed, scan the MIDX's packs directly for a surviving copy.

Do the scan only on the second read (OBJECT_INFO_SECOND_READ): by then the cheaper on-disk reload has run, so an object merely relocated into a new (non-covered) pack has already been found by the regular fallback, and only a genuine hidden duplicate reaches the rescan. QUICK callers that would skip the second read are steered into it by the preceding commit's stale_packs_detected flag, which prepare_midx_pack() sets when it cannot open the owning pack.

Reloading the stale MIDX would be a more complete fix but is much more involved (the borrowers above need proper invalidation), so leave that for later.

Assisted-by: Claude Opus 4.8 & GPT-5.6 Sol
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/pack-objects.c      |  2 +-
 midx.c                      | 38 ++++++++++--------
 midx.h                      | 21 +++++++++-
 odb/source-packed.c         | 42 ++++++++++++++++---
 t/t5319-multi-pack-index.sh | 80 +++++++++++++++++++++++++++++++++++++
 5 files changed, 158 insertions(+), 25 deletions(-)

On Tue, Aug 25, 2026 at 07:00:27PM +0000, Elijah Newren via GitGitGadget wrote:

Yikes. It is sad that we did not catch this in our leak-checking builds, as it implies that we do not test "mktree --batch" with multiple inputs. Or grepping for "mktree.*--batch" implies that we do not test the feature at all!

Looks like that feature comes from f1cf2d8b14 (mktree --batch: build more than one tree object, 2009-05-14), so I am not surprised that test coverage was a bit more spotty back then.

I guess you are going to add some coverage incidentally (or else you would not have found this). That's better than nothing, but I suspect a few basic directed "mktree --batch" tests would be a good thing to have in t1010.

#leftoverbits, perhaps?

-Peff

On Tue, Aug 25, 2026 at 07:00:28PM +0000, Elijah Newren via GitGitGadget wrote:

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

The much more interesting case is the non-QUICK one that Patrick outlined earlier in the thread. Where we say "nope, we don't have that object" even though we could find it with a little more work. But that doesn't seem to be described here either. But I think that is not even what this patch is about; that's in patch 4.

If the "error:" message is scary and gross (especially because we may retry and correct it anyway) and happens due to routine races, we might consider suppressing it.

So this is a way of hackily triggering SECOND_READ for QUICK queries, even though the point of QUICK is to suppress that second read! Again, maybe I'm just being dense, but I don't get it.

And this seems racy. We might catch the .idx but miss the .pack file. That would cause a failed read, but not trigger sale_packs_detected.

-Peff

On Tue, Aug 25, 2026 at 07:00:29PM +0000, Elijah Newren via GitGitGadget wrote:

We've changed the return value semantics without changing the signature (or name). So we need to make sure we adjust all callers, as here. That's _probably_ OK in practice for such a specialized function. But we could also rename it if we wanted to be paranoid (especially about new callers added on parallel branches).

OK, so this is our tri-state fix. Mostly looks as expected, though:

I'd have expected just "return MIDX_FILL_OWNER_UNAVAILABLE" here. But then, I'm not sure I buy the need for this stale_packs_detected stuff from patch 3.

This comment rewrite seems superfluous at best. Can we try to keep such patch fluff to a minimum?

OK, and this is as-before but now gated on the SECOND_READ flag. As expected in this revision.

-Peff

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

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?

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.

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

Any suggestions for alternate names?  fill_midx_entry_result?  midx_fill_entry?

Yeah, with the drop of patch 3 it becomes that.

Yes, sorry.

Thanks for taking a look!

Changes since v2:

 * Ripped out the old 3/4 dealing with QUICK readers; QUICK readers are left
   alone
 * Insert a new 3/4 fixing git mktree --batch to stop passing QUICK (with
   new testcase in t1010)
 * undo bad paragraph comment change
 * renamed fill_midx_entry() -> midx_fill_entry(), so that we catch any
   other new callers and appropriately check their return value (caught one
   in test-read-midx.c)

Changes since v1:

 * Rebased on top of ps/odb-generic-corrupt-objects, and conflicts with it
   resolved
 * Removed useless test_grep line spotted by Junio in PATCH 1
 * Switched fill_midx_entry() to a tri-state to avoid duplicate
   bsearch_midx(), as suggested by Peff
 * Only do the re-read on SECOND_READ, as suggested by Peff
 * Handle multiple objects shared across multiple packs correctly (issue
   caught & corrected & new testcase by deeper AI review)
 * Inserted two new patches:
   * 2/4: Fix a leak in git mktree --batch since I use it in new testcases
     and don't want the *-leaks jobs failing
   * 3/4: Demonstrate and fix QUICK reader problems, while keeping expected
     QUICK performance for normal cases (we've already been discussing this
     patch in this thread a bunch anyway, and it's logically related)

Cover letter addendum/update:

We also fix git mktree --batch to no longer erroneously pass QUICK.

Note here that Stolee's suggestion to defer pack deletion via git multi-pack-index expire seems like a good complementary mitigation; it would reduce how often we fall into recovery, while this series tries to fix recovery to work more robustly.

Original cover letter (focused on the final patch):

When an object is found in multiple packs that are in a multi-pack-index, and a subsequent geometric repacking creates a new multi-pack-index and removes the pack that was considered the owner of the object in the old multi-pack-index, then an already-running process that had opened the old multi-pack-index and hadn't yet opened the removed packfile will not be able to access the object -- lookups will return it as missing. Additionally, replay has a separate bug where a missing object causes a SIGSEGV rather than an error message.

This appears to affect a very small percentage of git operations in production since it is a tiny window, but I've found evidence of it occurring in at least eight distinct server-side operations, covering seven different git commands:

git operation                        symptom
-----------------------------------  -----------------------------
git replay (server-side rebase)      SIGSEGV (this series, 1/2)
git merge-tree                       spurious read-miss failure
git diff (raw and tree-vs-tree)      spurious read-miss failure
git rev-list --count                 spurious read-miss failure
git merge-base                       spurious read-miss failure
object/rev resolution (rev-parse,    spurious read-miss failure
  cat-file)
repository repair (fsck/repack)      spurious read-miss failure

There are also commands that could be changing behavior without throwing an error -- e.g. object negotiation thinking an object doesn't exist and instead negotiating based on an older common commit, or cat-file --batch reporting that some objects don't exist.

This series fixes the replay bug first, since it's simpler; investigating it, together with my other recent repacking work, is what led me to the underlying multi-pack-index issue that 2/2 addresses.

Elijah Newren (4):
  replay: fail gracefully when a merge input is unreadable
  mktree: plug per-tree leak in --batch mode
  mktree: do not use OBJECT_INFO_QUICK when checking objects
  packfile: recover when a multi-pack-index names a removed pack

 builtin/mktree.c            |  4 +++-
 builtin/pack-objects.c      |  2 +-
 midx.c                      | 20 ++++++++--------
 midx.h                      | 21 ++++++++++++++--
 odb/source-packed.c         | 42 ++++++++++++++++++++++++++++----
 replay.c                    |  7 ++++++
 t/helper/test-read-midx.c   |  2 +-
 t/t1010-mktree.sh           | 48 +++++++++++++++++++++++++++++++++++++
 t/t3650-replay-basics.sh    | 34 ++++++++++++++++++++++++++
 t/t5319-multi-pack-index.sh | 40 +++++++++++++++++++++++++++++++
 10 files changed, 200 insertions(+), 20 deletions(-)

base-commit: 2135b14863642bbcec02996e7f5e54ac1f77b03a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2207%2Fnewren%2Fmidx-removed-pack-recovery-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2207/newren/midx-removed-pack-recovery-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2207

Range-diff vs v2:

 1:  36bf2ce17b = 1:  36bf2ce17b replay: fail gracefully when a merge input is unreadable
 2:  3f3b75690e = 2:  3f3b75690e mktree: plug per-tree leak in --batch mode
 3:  fc98f48ddb < -:  ---------- packfile: recover object lookups racing a concurrent repack
 -:  ---------- > 3:  79ce753c68 mktree: do not use OBJECT_INFO_QUICK when checking objects
 4:  eacf6ba4b1 ! 4:  9b0966df9a packfile: recover when a multi-pack-index names a removed pack
     @@ Commit message
          previous MIDX keeps seeing a removed pack listed as the owner of some
          objects.  Since a MIDX attributes each object to exactly one pack, such
          an object is served only through its recorded owner; if that owner was
     -    just removed, find_pack_entry() cannot serve it -- fill_midx_entry()
     -    routes to the missing pack, and the regular pack fallback deliberately
     -    skips every MIDX-covered pack, so a surviving copy in another covered
     -    pack (e.g. a kept base pack) is never consulted.
     +    just removed, find_pack_entry() cannot serve it -- the MIDX lookup routes
     +    to the missing pack, and the regular pack fallback deliberately skips
     +    every MIDX-covered pack, so a surviving copy in another covered pack
     +    (e.g. a kept base pack) is never consulted.
      
          Unlike the ordinary "a pack's .idx is mapped but its .pack is gone"
     -    race, the second read does not rescue us -- and not only for
     -    OBJECT_INFO_QUICK callers.  Reloading the on-disk pack set does not
     -    reload the borrowed, cached MIDX (freeing it under the code that caches
     -    the "struct multi_pack_index *" would be a use-after-free), so the stale
     -    MIDX keeps routing to the removed pack and the surviving copy stays
     +    race, the second read does not rescue us.  Reloading the on-disk pack set
     +    does not reload the borrowed, cached MIDX (freeing it under the code that
     +    caches the "struct multi_pack_index *" would be a use-after-free), so the
     +    stale MIDX keeps routing to the removed pack and the surviving copy stays
          hidden behind the covered-pack skip.  cat-file, rev-list and pack-objects
          can thus all spuriously fail with "unable to read object".
      
     -    Teach find_pack_entry() to recover.  fill_midx_entry() now returns a
     -    tri-state, distinguishing "absent from the MIDX" from "present but the
     -    owning pack is unavailable"; in the latter case, once the regular
     -    fallback has also missed, scan the MIDX's packs directly for a surviving
     -    copy.
     +    Teach find_pack_entry() to recover.  The MIDX lookup now returns a
     +    tri-state, distinguishing an object absent from the MIDX from one it owns
     +    via a pack that can no longer be opened; in the latter case, once the
     +    regular fallback has also missed, scan the MIDX's packs directly for a
     +    surviving copy.  Because the return value is no longer a boolean, rename
     +    fill_midx_entry() to midx_fill_entry() so callers must reckon with the
     +    new enum rather than silently treat MIDX_FILL_OWNER_UNAVAILABLE as a hit.
      
          Do the scan only on the second read (OBJECT_INFO_SECOND_READ): by then
          the cheaper on-disk reload has run, so an object merely relocated into a
     -    new (non-covered) pack has already been found by the regular fallback,
     -    and only a genuine hidden duplicate reaches the rescan.  QUICK callers
     -    that would skip the second read are steered into it by the preceding
     -    commit's stale_packs_detected flag, which prepare_midx_pack() sets when
     -    it cannot open the owning pack.
     +    new (uncovered) pack has already been found by the regular fallback, and
     +    only a genuine hidden duplicate reaches the rescan.  A QUICK caller that
     +    skips the second read simply accepts the false negative, as QUICK is
     +    designed to.
      
          Reloading the stale MIDX would be a more complete fix but is much more
          involved (the borrowers above need proper invalidation), so leave that
     @@ builtin/pack-objects.c: static int want_object_in_pack_mtime(const struct object
       		struct pack_entry e;
       
      -		if (m && fill_midx_entry(m, oid, &e, NULL)) {
     -+		if (m && fill_midx_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {
     ++		if (m && midx_fill_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {
       			want = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);
       			if (want != -1)
       				return want;
     @@ midx.c: uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos
      -		    const struct object_id *oid,
      -		    struct pack_entry *e,
      -		    struct packed_git **bad_pack)
     -+enum midx_fill_result fill_midx_entry(struct multi_pack_index *m,
     ++enum midx_fill_result midx_fill_entry(struct multi_pack_index *m,
      +				      const struct object_id *oid,
      +				      struct pack_entry *e,
      +				      struct packed_git **bad_pack)
     @@ midx.c: uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos
       
       	if (prepare_midx_pack(m, pack_int_id))
      -		return 0;
     -+		goto owner_unavailable;
     ++		return MIDX_FILL_OWNER_UNAVAILABLE;
       	p = m->packs[pack_int_id - m->num_packs_in_base];
       
     --	/*
     --	* We are about to tell the caller where they can locate the
     --	* requested object.  We better make sure the packfile is
     --	* still here and can be accessed before supplying that
     --	* answer, as it may have been deleted since the MIDX was
     --	* loaded!
     --	*/
     -+	/* Make sure the pack is still present before pointing at it. */
     + 	/*
     +@@ midx.c: int fill_midx_entry(struct multi_pack_index *m,
     + 	* loaded!
     + 	*/
       	if (!is_pack_valid(p))
      -		return 0;
     -+		goto owner_unavailable;
     ++		return MIDX_FILL_OWNER_UNAVAILABLE;
       
       	if (oidset_size(&p->bad_objects) &&
       	    oidset_contains(&p->bad_objects, oid)) {
     @@ midx.c: uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos
       
      -	return 1;
      +	return MIDX_FILL_HIT;
     -+
     -+owner_unavailable:
     -+	/*
     -+	 * Re-arm stale_packs_detected on every such lookup, not just the
     -+	 * first: prepare_midx_pack() caches the failure, so without this a
     -+	 * later lookup of the same vanished pack would leave the flag clear
     -+	 * and a QUICK reader would skip its recovering second read.
     -+	 */
     -+	m->source->base.odb->stale_packs_detected = 1;
     -+	return MIDX_FILL_OWNER_UNAVAILABLE;
       }
       
       /* Match "foo.idx" against either "foo.pack" _or_ "foo.idx". */
     @@ midx.c: int verify_midx_file(struct odb_source_packed *source, unsigned flags)
       		nth_midxed_object_oid(&oid, m, pairs[i].pos);
       
      -		if (!fill_midx_entry(m, &oid, &e, NULL)) {
     -+		if (fill_midx_entry(m, &oid, &e, NULL) != MIDX_FILL_HIT) {
     ++		if (midx_fill_entry(m, &oid, &e, NULL) != MIDX_FILL_HIT) {
       			midx_report(_("failed to load pack entry for oid[%d] = %s"),
       				    pairs[i].pos, oid_to_hex(&oid));
       			continue;
     @@ midx.h: uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos
      +	MIDX_FILL_OWNER_UNAVAILABLE,
      +};
      +
     -+enum midx_fill_result fill_midx_entry(struct multi_pack_index *m,
     ++enum midx_fill_result midx_fill_entry(struct multi_pack_index *m,
      +				      const struct object_id *oid,
      +				      struct pack_entry *e,
      +				      struct packed_git **bad_pack);
     @@ odb/source-packed.c
      -	if (store->midx && fill_midx_entry(store->midx, oid, e, bad_pack))
      -		return 1;
      +	if (store->midx) {
     -+		midx_result = fill_midx_entry(store->midx, oid, e, bad_pack);
     ++		midx_result = midx_fill_entry(store->midx, oid, e, bad_pack);
      +		if (midx_result == MIDX_FILL_HIT)
      +			return 1;
      +	}
     @@ odb/source-packed.c: static int odb_source_packed_freshen_object(struct odb_sour
       	if (e.p->is_cruft)
       		return 0;
      
     + ## t/helper/test-read-midx.c ##
     +@@ t/helper/test-read-midx.c: static int read_midx_file(const char *object_dir, const char *checksum,
     + 		for (i = 0; i < m->num_objects; i++) {
     + 			nth_midxed_object_oid(&oid, m,
     + 					      i + m->num_objects_in_base);
     +-			fill_midx_entry(m, &oid, &e, NULL);
     ++			midx_fill_entry(m, &oid, &e, NULL);
     + 
     + 			printf("%s %"PRIu64"\t%s\n",
     + 			       oid_to_hex(&oid), e.offset, e.p->pack_name);
     +
       ## t/t5319-multi-pack-index.sh ##
      @@ t/t5319-multi-pack-index.sh: test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '
       	)
     @@ t/t5319-multi-pack-index.sh: test_expect_success 'pack.preferBitmapTips interpre
      +		test_cmp expect actual
      +	)
      +'
     -+
     -+test_expect_success 'repeated QUICK lookups recover after owning pack removed' '
     -+	test_when_finished "rm -fr repo" &&
     -+	git init repo &&
     -+	(
     -+		cd repo &&
     -+
     -+		# Two blobs, each duplicated across packs so the midx must pick
     -+		# an owning pack, and each attributed to the same moderate pack.
     -+		echo one >f1 &&
     -+		echo two >f2 &&
     -+		git add f1 f2 &&
     -+		git commit -m dups &&
     -+		d1=$(git rev-parse HEAD:f1) &&
     -+		d2=$(git rev-parse HEAD:f2) &&
     -+
     -+		# Roll every object, including d1 and d2, into one big pack,
     -+		# then build a moderate pack that also holds both blobs.
     -+		git repack -adq &&
     -+		moderate=$(printf "%s\n%s\n" "$d1" "$d2" |
     -+			git pack-objects --quiet $objdir/pack/pack) &&
     -+
     -+		git multi-pack-index write \
     -+			--preferred-pack="pack-$moderate.idx" &&
     -+
     -+		# Retire the moderate pack; the stale midx still names it as the
     -+		# owner of both blobs, each of which survives in the big pack.
     -+		rm -f $objdir/pack/pack-$moderate.* &&
     -+
     -+		# One resident QUICK reader ("git mktree --batch") resolves both
     -+		# blobs.  The first lookup recovers d1 and caches the owning
     -+		# packs failure; unless that failure keeps re-arming the second
     -+		# read, the lookup of d2 skips its recovering read and the reader
     -+		# dies reporting d2 as missing.
     -+		printf "100644 blob %s\tf1\n\n100644 blob %s\tf2\n\n" \
     -+			"$d1" "$d2" |
     -+			git mktree --batch >trees &&
     -+		test_line_count = 2 trees
     -+	)
     -+'
      +
       test_done

From: Elijah Newren <newren@gmail.com>

When objects involved in the merge cannot be read, the merge machinery will return early with result.clean = -1, and result.tree left as NULL. pick_regular_commit() tested only "if (!result->clean)", ignoring the case where "clean < 0". That causes the code to try to use result->tree, resulting in a SIGSEGV.

Handle clean < 0 explicitly; the merge machinery will already have printed messages such as "Could not read <object>" and "collecting merge info failed for trees...", so we don't need to add much detail beyond the fact that the merge failed.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 replay.c                 |  7 +++++++
 t/t3650-replay-basics.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

From: Elijah Newren <newren@gmail.com>

In --batch mode "git mktree" reuses its entry buffer across trees, resetting `used` to 0 after writing each tree. It never frees the `treeent` structures the previous tree appended, though, so once the next tree overwrites those slots the earlier allocations are leaked. A single-tree invocation hides this, as the entries stay reachable through the `entries` global until exit.

Free each entry when resetting the buffer, and free the buffer itself before returning.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/mktree.c | 3 +++
 1 file changed, 3 insertions(+)

From: Elijah Newren <newren@gmail.com>

mktree_line() checks each referenced object's type with odb_read_object_info_extended() under OBJECT_INFO_QUICK. QUICK skips the reprepare-and-retry that reloads the on-disk pack set, so a resident "git mktree --batch" reader reports an object that a concurrent repack just relocated into a new pack as missing, and rejects the entry.

QUICK entered this lookup in 817b0f602710 (mktree: do not check type of remote objects, 2022-06-21) only to avoid lazily fetching promisor objects; OBJECT_INFO_SKIP_FETCH_OBJECT already provides that. Drop OBJECT_INFO_QUICK and keep OBJECT_INFO_SKIP_FETCH_OBJECT, so mktree still avoids a promisor fetch but recovers an object that was merely repacked.

Add a regression test driving a resident mktree --batch reader across a concurrent repack that retires a pack.

Assisted-by: Claude Opus 4.8 & GPT-5.6 Sol
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/mktree.c  |  1 -
 t/t1010-mktree.sh | 48 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)

From: Elijah Newren <newren@gmail.com>

A geometric repack writes a new pack and multi-pack-index and then deletes the packs the new one subsumes. A process still using the previous MIDX keeps seeing a removed pack listed as the owner of some objects. Since a MIDX attributes each object to exactly one pack, such an object is served only through its recorded owner; if that owner was just removed, find_pack_entry() cannot serve it -- the MIDX lookup routes to the missing pack, and the regular pack fallback deliberately skips every MIDX-covered pack, so a surviving copy in another covered pack (e.g. a kept base pack) is never consulted.

Unlike the ordinary "a pack's .idx is mapped but its .pack is gone" race, the second read does not rescue us. Reloading the on-disk pack set does not reload the borrowed, cached MIDX (freeing it under the code that caches the "struct multi_pack_index *" would be a use-after-free), so the stale MIDX keeps routing to the removed pack and the surviving copy stays hidden behind the covered-pack skip. cat-file, rev-list and pack-objects can thus all spuriously fail with "unable to read object".

Teach find_pack_entry() to recover. The MIDX lookup now returns a tri-state, distinguishing an object absent from the MIDX from one it owns via a pack that can no longer be opened; in the latter case, once the regular fallback has also missed, scan the MIDX's packs directly for a surviving copy. Because the return value is no longer a boolean, rename fill_midx_entry() to midx_fill_entry() so callers must reckon with the new enum rather than silently treat MIDX_FILL_OWNER_UNAVAILABLE as a hit.

Do the scan only on the second read (OBJECT_INFO_SECOND_READ): by then the cheaper on-disk reload has run, so an object merely relocated into a new (uncovered) pack has already been found by the regular fallback, and only a genuine hidden duplicate reaches the rescan. A QUICK caller that skips the second read simply accepts the false negative, as QUICK is designed to.

Reloading the stale MIDX would be a more complete fix but is much more involved (the borrowers above need proper invalidation), so leave that for later.

Assisted-by: Claude Opus 4.8 & GPT-5.6 Sol
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/pack-objects.c      |  2 +-
 midx.c                      | 20 +++++++++---------
 midx.h                      | 21 +++++++++++++++++--
 odb/source-packed.c         | 42 ++++++++++++++++++++++++++++++++-----
 t/helper/test-read-midx.c   |  2 +-
 t/t5319-multi-pack-index.sh | 40 +++++++++++++++++++++++++++++++++++
 6 files changed, 108 insertions(+), 19 deletions(-)

On Thu, Aug 27, 2026 at 03:23:30PM -0700, Elijah Newren wrote:

I think we should consider the log message independently from whether we eventually return a value (whether QUICK or not). It seems like the log message is often unnecessarily scary, because we either recover via SECOND_READ, or we are in QUICK mode and the false negative is OK. So the message is informative at best, and probably just noise in those cases.

But it perhaps _is_ helpful when a non-QUICK lookup ends up returning failure. We'll end up with some other error() message, but it may be useful context to know that we _thought_ we had the object available and then the rug was pulled out from under us. But we don't have a good way of queuing up an error that is shown conditionally.

So I dunno. We could consider moving that message into trace/trace2, making it more of a "debug" message. And then people digging into a problem can turn on traces. But I have a feeling that is not very helpful, since it is mostly a racy situation (so you can't just easily replay your failure with tracing turned on).

So now we can consider the actual return value, aside from the logged message. For non-QUICK requests, I think this case is uninteresting (we already do a reprepare and follow-up read). For mktree, I think the core of the problem is using QUICK when it should not.

I think the current behavior of QUICK is _correct_, in the sense that false negatives are OK. But can we make it better? Possibly. To me the argument for this patch's direction is something like:

  The point of QUICK was to avoid lots of reprepare effort when we are
  looking up objects that we might reasonably not have. This has
  historically been about things like fetch speculatively looking for
  stuff the other side mentioned. But there we are mostly concerned
  about objects we _never_ had, and avoiding tons of reprepare work that
  will almost certainly not help us. But in some races, we might learn
  that we _did_ have the object at one point (because we opened its idx,
  or a midx) but the lookup still failed (because the pack couldn't be
  accessed).

  We can cheaply notice this case by differentiating true idx misses
  from failure to access the pack contents. And these items _are_ worth
  a reprepare, because they were almost certainly caused by a repacking
  race (or a true repo corruption or object pruning, but that is rare
  enough not to worry about for optimization purposes).

  So even though QUICK is not _wrong_ to say "we do not have that
  object", it is a good tradeoff to spend a little bit of time calling
  reprepare in order to produce fewer false negative "no such object"
  responses (because tools like fetch then have a chance to optimize
  their own task more as a result).

Maybe that argument was somewhere in your original commit message. I admit I got lost about half-way through. ;)

But I think the key thing is separating:

  - is the logging confusing or useful? What should we do about it?

  - is mktree racily broken because of QUICK? I think so.

  - even though QUICK is not wrong to skip the second read for this
    case, it might be a good tradeoff for it to detect and try harder
    here (i.e., the argument above).

Which sounds like three patches to me, each of which can be motivated and argued on its own.

I am also happy with this direction. Then we can consider the other questions separately (or not at all if nobody cares enough).

-Peff

On Fri, Aug 28, 2026 at 12:29:49AM -0700, Elijah Newren wrote:

I did not have a good suggestion, but midx_fill_entry (which it looks like your new series uses) seems reasonable. It is probably the better name anyway, as it fits the subsystem_verb_the_thing() ordering.

I'll take a look at the new series and comment further there (if needed; my fingers are crossed for perfection).

-Peff

On Sat, Aug 29, 2026 at 07:00:30AM +0000, Elijah Newren via GitGitGadget wrote:

I think this line of reasoning is fine.

We probably _could_ use QUICK when the caller specified --missing, which would optimize out the SECOND_READ effort if the caller told us they expect (or at least allow) some items to be missing. But:

  1. It's not clear how people use --missing. If you are just trying to
     be gentle with an occasional missing entry, then the optimization
     is not that interesting. If you run mktree all the time to make
     synthetic trees full of objects you don't have, then maybe you do
     care about the optimization. But if you are doing that then you
     probably are better off with an option that avoids the lookup
     entirely (i.e., we should just trust the type found in the input).

     So there's maybe room for a --yolo argument to mktree, though I
     guess in practice you could just use "hash-object" for that. But
     either way that is way out of scope for this patch.

  2. Prior to 817b0f602710 we were not QUICK either! And that commit was
     only trying to trigger SKIP_FETCH_OBJECT. So whether there is an
     argument for linking --missing and QUICK or not, it should be made
     separately. This patch is just fixing the extra flag that probably
     should not have been added by 817b0f602710.

OK. I was hoping we could test this without all of the PIPE complexity, but I don't think we can. We really need a case where the first lookup fails but SECOND_READ succeeds, which is inherently a race. Feeding one entry at a time lets us implement that in a deterministic way, and I think is the simplest we can get.

So the patch looks good to me overall.

-Peff

On Sat, Aug 29, 2026 at 07:00:31AM +0000, Elijah Newren via GitGitGadget wrote:

So I think this workaround is fine to do (as long as we are not going to actually refresh the midx on SECOND_READ, which I agree is probably a bigger change).

I always get confused about m->num_packs and m->num_packs_in_base, and whether we are looking at the packs in a midx slice versus the whole thing. I _think_ what you have here is correct, because we are iterating from 0 up to the total number of packs, and prepare_midx_pack() etc will look back through the incremental slices as necessary.

But I wonder if it would be simpler to just iterate over the actual pack list in the usual way, since we already do that in this function. I _thought_ this would work:

Jeff King <peff@peff.net> writes:

Thanks for a very informative and well reasoned write-up in support of the series.

Shall we mark it for 'next' then?

On Sun, Aug 30, 2026 at 01:53:51PM -0700, Junio C Hamano wrote:

Here's my a lot less well reasoned +1, for what it's worth. Thanks!

Patrick

On Sun, Aug 30, 2026 at 01:53:51PM -0700, Junio C Hamano wrote:

Yeah, that sounds good to me.

-Peff

On 8/29/2026 3:00 AM, Elijah Newren via GitGitGadget wrote:

I'm late in reviewing this patch, so forgive me responding inline as I discover how it works.

tl;dr: Good patch. LGTM.

This tri-state is valuable!
 

One major benefit to the rename is that we can guarantee that all callers are updated to reflect the new tri-state response.

It also has a better naming convention, overall.

(reordered header file diff up)

This is good documentation that will help future uses know how to react to the different modes.

Obviously correct: this OID isn't in the sorted list.

Obviously correct: we tried to open the pack index but failed.

Same: Pack is invalid somehow, likely that the .pack disappeared.

This one is tricky, but makes sense: we have marked this as a "bad" object so we should act like it doesn't exist. Good.

finally: success!>  }

This looks good. On a hit, we return. Act like a MIDX-miss if we don't have a midx.

Outside of the patch context is the "reprepare packfiles" to pick up a copy from a packfile that doesn't exist within the current (stale) midx.

This comment does a lot of important context-setting to show that we are in a very narrow case: the stale MIDX has multiple packs that contain the requested object, but the "newer" one was deleted without creating a new packfile, so we need to look at each contained pack for the object from its pack-index.

This is hopefully a very rare case, but it's good to have this "fall back to O(num packs)" situation.

This use of preferred pack is a good way of getting around mtimes that could be equal. We could also consider updating mtimes, but this works so don't change it.

Thanks for adding this test so we can keep this narrow case working in perpetuity.

Thanks, -Stolee

On 8/30/2026 4:53 PM, Junio C Hamano wrote:

I'm late in responding, but I support the series, too!

thanks, -Stolee

Derrick Stolee <stolee@gmail.com> writes:

Thanks, all.

On Tue, Sep 1, 2026 at 8:26 AM Derrick Stolee <stolee@gmail.com> wrote:

Thanks for taking a look; I wanted to point out two minor clarifications...

Actually, a new packfile is typically created, it just doesn't have the object in question -- and doesn't need to, because a pre-existing (also midx-covered) pack already has it.

It's actually a fall back to O(num_packs_in_the_midx); on developer laptops that's probably about the same as O(num_packs), but on busy servers constantly receiving pushes, the total number of packs often dwarfs the number of packs in the midx.

On 9/1/2026 12:47 PM, Elijah Newren wrote:

Thanks. That helps me understand why this can occur regularly enough to be triggered in the wild.

Thanks. You're absolutely right that I was not specific enough and in server situations this loop will be very short.

Thanks, -Stolee