Objects treated as missing despite being present, due to race with geometric repacking

[newren]

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.

cc: Patrick Steinhardt ps@pks.im
cc: Elijah Newren newren@gmail.com
cc: Jeff King peff@peff.net
cc: Derrick Stolee stolee@gmail.com

[newren]

/submit

[gitgitgadget]

Submitted as pull.2207.git.1787092446.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2207/newren/midx-removed-pack-recovery-v1

To fetch this version to local tag pr-2207/newren/midx-removed-pack-recovery-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2207/newren/midx-removed-pack-recovery-v1

[gitgitgadget]

User Patrick Steinhardt <ps@pks.im> has been added to the cc: list.

[gitgitgadget]

This branch is now known as en/midx-missing-pack-fallback.

[gitgitgadget]

This patch series was integrated into seen via git@8ef1c4d.

[gitgitgadget]

There was a status update in the "New Topics" section about the branch en/midx-missing-pack-fallback on the Git mailing list:

The object lookup machinery has been taught to gracefully recover
when a multi-pack-index points to an owning pack that was removed
during a concurrent geometric repack, and 'git replay' has been
fixed to not segfault when reading such missing objects.

Waiting for response.
cf. <xmqqfr0augls.fsf@gitster.g>
cf. <aoayppoxHAkcFTBN@pks.im>
source: <pull.2207.git.1787092446.gitgitgadget@gmail.com>

[gitgitgadget]

User Elijah Newren <newren@gmail.com> has been added to the cc: list.

[gitgitgadget]

This patch series is no longer integrated into seen.

[gitgitgadget]

User Jeff King <peff@peff.net> has been added to the cc: list.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch en/midx-missing-pack-fallback on the Git mailing list:

The object lookup machinery has been taught to gracefully recover
when a multi-pack-index points to an owning pack that was removed
during a concurrent geometric repack, and 'git replay' has been
fixed to not segfault when reading such missing objects.

Waiting for response.
cf. <xmqqfr0augls.fsf@gitster.g>
cf. <aoayppoxHAkcFTBN@pks.im>
source: <pull.2207.git.1787092446.gitgitgadget@gmail.com>

[gitgitgadget]

User Derrick Stolee <stolee@gmail.com> has been added to the cc: list.

[gitgitgadget]

This patch series was integrated into seen via git@e4a2bf3.

[newren]

/submit

[gitgitgadget]

Submitted as pull.2207.v2.git.1787684429.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2207/newren/midx-removed-pack-recovery-v2

To fetch this version to local tag pr-2207/newren/midx-removed-pack-recovery-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2207/newren/midx-removed-pack-recovery-v2

[gitgitgadget]

There was a status update in the "Cooking" section about the branch en/midx-missing-pack-fallback on the Git mailing list:

The object lookup machinery has been taught to gracefully recover
when a multi-pack-index points to an owning pack that was removed
during a concurrent geometric repack, and 'git replay' has been
fixed to not segfault when reading such missing objects.

Needs review.
source: <pull.2207.v2.git.1787684429.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch en/midx-missing-pack-fallback on the Git mailing list:

The object lookup machinery has been taught to gracefully recover
when a multi-pack-index points to an owning pack that was removed
during a concurrent geometric repack, and 'git replay' has been
fixed to not segfault when reading such missing objects.

Expecting a reroll.
cf. <CABPp-BEmReAR-f-aweM=f=5QhRPxG1K-KLTsbyRt2aDQD_QnVA@mail.gmail.com>
source: <pull.2207.v2.git.1787684429.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch en/midx-missing-pack-fallback on the Git mailing list:

The object lookup machinery has been taught to gracefully recover
when a multi-pack-index points to an owning pack that was removed
during a concurrent geometric repack, and 'git replay' has been
fixed to not segfault when reading such missing objects.

Expecting a reroll.
cf. <CABPp-BEmReAR-f-aweM=f=5QhRPxG1K-KLTsbyRt2aDQD_QnVA@mail.gmail.com>
source: <pull.2207.v2.git.1787684429.gitgitgadget@gmail.com>

[newren]

/submit

[gitgitgadget]

Submitted as pull.2207.v3.git.1787986831.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2207/newren/midx-removed-pack-recovery-v3

To fetch this version to local tag pr-2207/newren/midx-removed-pack-recovery-v3:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2207/newren/midx-removed-pack-recovery-v3

[gitgitgadget]

Jeff King wrote on the Git mailing list (how to reply to this email):

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

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

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.

> +test_expect_success PIPE 'mktree --batch survives a concurrent repack retiring a pack' '

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

[gitgitgadget]

Jeff King wrote on the Git mailing list (how to reply to this email):

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

> +	/*
> +	 * 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 (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&
> +	    (flags & OBJECT_INFO_SECOND_READ)) {
> +		struct multi_pack_index *m = store->midx;
> +		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;
> +		}
> +	}

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:

diff --git a/odb/source-packed.c b/odb/source-packed.c
index 90d88c0a12..86e6a80d2f 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -33,40 +33,19 @@ static int find_pack_entry(struct odb_source_packed *store,
 	for (l = store->packs.head; l; l = l->next) {
 		struct packed_git *p = l->pack;
 
-		if (!p->multi_pack_index && packfile_fill_entry(p, oid, e, bad_pack)) {
+		/* ...explain tricky race case here... */
+		if (p->multi_pack_index &&
+		    (midx_result != MIDX_FILL_OWNER_UNAVAILABLE ||
+		     !(flags & OBJECT_INFO_SECOND_READ)))
+			continue;
+
+		if (packfile_fill_entry(p, oid, e, bad_pack)) {
 			if (!store->skip_mru_updates)
 				packfile_list_prepend(&store->packs, p);
 			return 1;
 		}
 	}
 
-	/*
-	 * 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 (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&
-	    (flags & OBJECT_INFO_SECOND_READ)) {
-		struct multi_pack_index *m = store->midx;
-		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;
 }
 

but it doesn't because we don't always load the midx'd packs into the
pack list (we do it on-demand as they become useful to us). So I think
you'd essentially end up needing to do a loop like the one you have
anyway to prepare_midx_pack() on them all.

And we want to avoid doing that if we can find it outside the midx
(since that was the whole point of waiting for SECOND_READ). Which would
happen...in that loop. So we really do want to have our own
midx-specific loop like you have here.

Sorry, I know that was a lot of text to end up at "you have already
written it the best way", but it took me a while to reason through it.

The patch looks good to me. ;)

-Peff