[PATCH 0/2] Objects treated as missing despite being present, due to race with geometric repacking

[PATCH 0/2] Objects treated as missing despite being present, due to race with geometric repacking

[Elijah Newren via GitGitGadget]

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

[PATCH 1/2] replay: fail gracefully when a merge input is unreadable

[Elijah Newren via GitGitGadget]

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(+)

diff --git a/replay.c b/replay.c
index 463c900d6c..33e21b2032 100644
--- a/replay.c
+++ b/replay.c
@@ -327,6 +327,13 @@ static struct commit *pick_regular_commit(struct repository *repo,
 	merge_opt->ancestor = NULL;
 	merge_opt->branch2 = NULL;
 
+	if (result->clean < 0) {
+		error(_("merge of %s onto %s failed"),
+		      oid_to_hex(&pickme->object.oid),
+		      oid_to_hex(&replayed_base->object.oid));
+		return NULL;
+	}
+
 	if (!result->clean)
 		return NULL;
 
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index 3353bc4a4d..d66b8edb95 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -565,4 +565,39 @@ test_expect_success '--onto with --ref rejects multiple revision ranges' '
 	test_grep "cannot be used with multiple revision ranges" err
 '
 
+test_expect_success 'replay fails without segfault when objects are missing' '
+	test_when_finished "rm -fr unreadable" &&
+	git init unreadable &&
+	(
+		cd unreadable &&
+
+		test_write_lines l1 l2 l3 l4 l5 l6 l7 l8 >f &&
+		git add f &&
+		git commit -m base &&
+		git branch base &&
+
+		test_write_lines l1 l2 l3 l4 l5 l6 l7 CHANGED >f &&
+		git commit -am side &&
+		git branch side &&
+
+		git switch -c onto base &&
+		test_write_lines CHANGED l2 l3 l4 l5 l6 l7 l8 >f &&
+		git commit -am onto &&
+
+		# The replay works while every object is readable.
+		git replay --onto onto base..side &&
+
+		# Removing the onto tree makes parse_tree() fail during the
+		# incore merge, driving clean < 0 with a NULL result tree.
+		onto_tree=$(git rev-parse onto^{tree}) &&
+		obj=$(test_oid_to_path "$onto_tree") &&
+		mv .git/objects/${obj} saved-tree &&
+
+		# 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_done
-- 
gitgitgadget

Re: [PATCH 1/2] replay: fail gracefully when a merge input is unreadable

[Junio C Hamano]

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

> 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(+)
>
> diff --git a/replay.c b/replay.c
> index 463c900d6c..33e21b2032 100644
> --- a/replay.c
> +++ b/replay.c
> @@ -327,6 +327,13 @@ static struct commit *pick_regular_commit(struct repository *repo,
>  	merge_opt->ancestor = NULL;
>  	merge_opt->branch2 = NULL;
>  
> +	if (result->clean < 0) {
> +		error(_("merge of %s onto %s failed"),
> +		      oid_to_hex(&pickme->object.oid),
> +		      oid_to_hex(&replayed_base->object.oid));
> +		return NULL;
> +	}
> +
>  	if (!result->clean)
>  		return NULL;

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.

> +		# 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_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.

> +	)
> +'
> +
>  test_done

Thanks.

Re: [PATCH 1/2] replay: fail gracefully when a merge input is unreadable

[Elijah Newren]

On Wed, Aug 19, 2026 at 11:09 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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(+)
> >
> > diff --git a/replay.c b/replay.c
> > index 463c900d6c..33e21b2032 100644
> > --- a/replay.c
> > +++ b/replay.c
> > @@ -327,6 +327,13 @@ static struct commit *pick_regular_commit(struct repository *repo,
> >       merge_opt->ancestor = NULL;
> >       merge_opt->branch2 = NULL;
> >
> > +     if (result->clean < 0) {
> > +             error(_("merge of %s onto %s failed"),
> > +                   oid_to_hex(&pickme->object.oid),
> > +                   oid_to_hex(&replayed_base->object.oid));
> > +             return NULL;
> > +     }
> > +
> >       if (!result->clean)
> >               return NULL;
>
> 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.

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.

> > +             # 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_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'".

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?

Re: [PATCH 1/2] replay: fail gracefully when a merge input is unreadable

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> > +             # 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_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'".
>
> 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?

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.

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

[Elijah Newren via GitGitGadget]

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(+)

diff --git a/odb/source-packed.c b/odb/source-packed.c
index 0890704e76..de96215069 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -31,6 +31,35 @@ 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.
+	 */
+	if (store->midx) {
+		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;
+			}
+		}
+	}
+
 	return 0;
 }
 
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 68143cb5b7..2b8ff6f3ed 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -1393,4 +1393,44 @@ test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '
 	)
 '
 
+test_expect_success 'lookup recovers object whose midx-owning pack was removed' '
+	test_when_finished "rm -fr repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		# "keep" ends up only in the big pack; "dup" is deliberately
+		# placed in two packs so the midx has to choose an owner.
+		test_commit keep &&
+		echo duplicated-content >dup &&
+		git add dup &&
+		git commit -m dup &&
+		dup_oid=$(git rev-parse HEAD:dup) &&
+
+		# Roll every object, including dup, into a single big pack.
+		git repack -adq &&
+
+		# Build a second, "moderate" pack that also contains dup, so dup
+		# now lives in two packs that the midx will cover.
+		moderate=$(echo "$dup_oid" |
+			git pack-objects --quiet $objdir/pack/pack) &&
+
+		# Attribute dup to the moderate pack in the midx.
+		git multi-pack-index write \
+			--preferred-pack="pack-$moderate.idx" &&
+
+		# Simulate a concurrent "git repack" retiring the moderate pack:
+		# its files disappear, but the now-stale midx still names it as
+		# the owner of dup.  A valid copy of dup survives in the big pack.
+		rm -f $objdir/pack/pack-$moderate.* &&
+
+		# The midx routes the lookup to the deleted pack, and the regular
+		# pack fallback skips midx-covered packs, so without recovery dup
+		# would appear missing even though it is physically present.
+		echo blob >expect &&
+		git cat-file -t "$dup_oid" >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done
-- 
gitgitgadget

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

[Junio C Hamano]

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

> @@ -31,6 +31,35 @@ 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.
> +	 */
> +	if (store->midx) {
> +		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;
> +			}
> +		}
> +	}
> +
>  	return 0;
>  }

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.

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

[Patrick Steinhardt]

On Tue, Aug 18, 2026 at 10:34:06PM +0000, Elijah Newren via GitGitGadget wrote:
> 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.

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.

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

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?

> diff --git a/odb/source-packed.c b/odb/source-packed.c
> index 0890704e76..de96215069 100644
> --- a/odb/source-packed.c
> +++ b/odb/source-packed.c
> @@ -31,6 +31,35 @@ 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.
> +	 */
> +	if (store->midx) {
> +		struct multi_pack_index *m = store->midx;
> +		uint32_t midx_pos, i;
> +
> +		if (bsearch_midx(oid, m, &midx_pos)) {

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.

> +			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;
> +			}

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

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

[Elijah Newren]

On Thu, Aug 20, 2026 at 12:54 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Aug 18, 2026 at 10:34:06PM +0000, Elijah Newren via GitGitGadget wrote:
> > 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.
>
> 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.

Yep.

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

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.

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

[Jeff King]

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

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

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

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

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?

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

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

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

[Patrick Steinhardt]

On Mon, Aug 24, 2026 at 12:48:22AM -0400, Jeff King wrote:
> So between the two cases, it sounds like things (or at least the
> low-level lookups) are working as designed, and there is no bug. Or am I
> misunderstanding something?

I agree that QUICK is working as designed, and that callers that pass it
without being able to accommodate for false negatives are buggy. But the
patch sent by Elijah still fixes an actual bug where we may not find an
object that is contained in two MIDXd packs where the preferred pack for
a respective object vanishes concurrently. Filling the packfile entry
via the MIDX will fail because the pack vanished, and the lookup via the
non-preferred pack will fail, too, because we skip over any packs that
are covered by the MIDX when doing the non-MIDX lookup. Consequently, we
won't find the object at all.

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

Patrick

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

[Jeff King]

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

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

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

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

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

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

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

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

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

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

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

-Peff

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

[Jeff King]

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

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

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

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

[Jeff King]

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

> On Mon, Aug 24, 2026 at 02:55:39AM -0400, Jeff King wrote:
> 
> > 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.
> 
> 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.

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

-Peff

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

[Elijah Newren]

On Mon, Aug 24, 2026 at 12:06 AM Jeff King <peff@peff.net> wrote:
>
> 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.

I implemented that in v2.

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

[Jeff King]

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

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

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

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

[Patrick Steinhardt]

On Mon, Aug 24, 2026 at 12:55:29AM -0400, Jeff King wrote:
> On Tue, Aug 18, 2026 at 10:34:06PM +0000, Elijah Newren via GitGitGadget wrote:
> 
> > 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.
> 
> 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.

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

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

[Jeff King]

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

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

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

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

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

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

[Elijah Newren]

On Sun, Aug 23, 2026 at 9:55 PM Jeff King <peff@peff.net> wrote:
>
> On Tue, Aug 18, 2026 at 10:34:06PM +0000, Elijah Newren via GitGitGadget wrote:
>
> > 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.
>
> 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).

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.

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

[Derrick Stolee]

On 8/18/2026 6:34 PM, Elijah Newren via GitGitGadget wrote:
> 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.

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

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

[Elijah Newren]

On Mon, Aug 24, 2026 at 7:45 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 8/18/2026 6:34 PM, Elijah Newren via GitGitGadget wrote:
> > 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.
>
> 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.

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.

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

[Derrick Stolee]

On 8/18/2026 6:34 PM, Elijah Newren via GitGitGadget wrote:
> 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.

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

[PATCH v2 0/4] Objects treated as missing despite being present, due to race with geometric repacking

[Elijah Newren via GitGitGadget]

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

-- 
gitgitgadget

[PATCH v2 1/4] replay: fail gracefully when a merge input is unreadable

[Elijah Newren via GitGitGadget]

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(+)

diff --git a/replay.c b/replay.c
index 463c900d6c..33e21b2032 100644
--- a/replay.c
+++ b/replay.c
@@ -327,6 +327,13 @@ static struct commit *pick_regular_commit(struct repository *repo,
 	merge_opt->ancestor = NULL;
 	merge_opt->branch2 = NULL;
 
+	if (result->clean < 0) {
+		error(_("merge of %s onto %s failed"),
+		      oid_to_hex(&pickme->object.oid),
+		      oid_to_hex(&replayed_base->object.oid));
+		return NULL;
+	}
+
 	if (!result->clean)
 		return NULL;
 
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index 3353bc4a4d..12348b4a5f 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -565,4 +565,38 @@ test_expect_success '--onto with --ref rejects multiple revision ranges' '
 	test_grep "cannot be used with multiple revision ranges" err
 '
 
+test_expect_success 'replay fails without segfault when objects are missing' '
+	test_when_finished "rm -fr unreadable" &&
+	git init unreadable &&
+	(
+		cd unreadable &&
+
+		test_write_lines l1 l2 l3 l4 l5 l6 l7 l8 >f &&
+		git add f &&
+		git commit -m base &&
+		git branch base &&
+
+		test_write_lines l1 l2 l3 l4 l5 l6 l7 CHANGED >f &&
+		git commit -am side &&
+		git branch side &&
+
+		git switch -c onto base &&
+		test_write_lines CHANGED l2 l3 l4 l5 l6 l7 l8 >f &&
+		git commit -am onto &&
+
+		# The replay works while every object is readable.
+		git replay --onto onto base..side &&
+
+		# Removing the onto tree makes parse_tree() fail during the
+		# incore merge, driving clean < 0 with a NULL result tree.
+		onto_tree=$(git rev-parse onto^{tree}) &&
+		obj=$(test_oid_to_path "$onto_tree") &&
+		mv .git/objects/${obj} saved-tree &&
+
+		# Ensure replay gracefully handles the missing object
+		test_must_fail git replay --onto onto base..side 2>err &&
+		test_grep -e "Could not read" -e "collecting merge info failed" err
+	)
+'
+
 test_done
-- 
gitgitgadget

[PATCH v2 2/4] mktree: plug per-tree leak in --batch mode

[Elijah Newren via GitGitGadget]

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(+)

diff --git a/builtin/mktree.c b/builtin/mktree.c
index 4084e32476..dc2d293c3d 100644
--- a/builtin/mktree.c
+++ b/builtin/mktree.c
@@ -200,8 +200,11 @@ int cmd_mktree(int ac,
 			puts(oid_to_hex(&oid));
 			fflush(stdout);
 		}
+		for (int i = 0; i < used; i++)
+			free(entries[i]);
 		used=0; /* reset tree entry buffer for re-use in batch mode */
 	}
+	free(entries);
 	strbuf_release(&sb);
 
 	return 0;
-- 
gitgitgadget

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

[Elijah Newren via GitGitGadget]

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

diff --git a/midx.c b/midx.c
index 37f082dbdd..942505ac41 100644
--- a/midx.c
+++ b/midx.c
@@ -475,6 +475,12 @@ int prepare_midx_pack(struct multi_pack_index *m,
 
 	if (!p) {
 		m->packs[pack_int_id] = MIDX_PACK_ERROR;
+		/*
+		 * The midx names a pack we can no longer open (its files
+		 * vanished, e.g. a concurrent repack replaced it).  Record the
+		 * stale pack set (see stale_packs_detected).
+		 */
+		packed->base.odb->stale_packs_detected = 1;
 		return 1;
 	}
 
diff --git a/odb.c b/odb.c
index 6bbea64033..4bb9662c65 100644
--- a/odb.c
+++ b/odb.c
@@ -583,8 +583,14 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
 		 * When the object hasn't been found we try a second read and
 		 * tell the sources so. This may cause them to invalidate
 		 * caches or reload on-disk state.
+		 *
+		 * A QUICK lookup normally skips this second read to stay fast
+		 * on a genuine miss, but retry anyway when a pack vanished
+		 * mid-lookup (stale_packs_detected): the object likely just
+		 * moved into its replacement pack.
 		 */
-		if (!(flags & OBJECT_INFO_QUICK)) {
+		if (!(flags & OBJECT_INFO_QUICK) ||
+		    odb->stale_packs_detected) {
 			for (source = odb->sources; source; source = source->next) {
 				ret = odb_source_read_object_info(source, real, oi,
 								  flags | OBJECT_INFO_SECOND_READ,
diff --git a/odb.h b/odb.h
index 1264d4ce7d..8b91e6f8ba 100644
--- a/odb.h
+++ b/odb.h
@@ -93,6 +93,17 @@ struct object_database {
 	unsigned object_count_flags;
 	unsigned object_count_valid : 1;
 
+	/*
+	 * Set when a lookup finds that a pack we already know about has
+	 * vanished -- its ".idx" or ".pack" removed out from under us, the
+	 * signature of a concurrent "git repack".  It tells
+	 * odb_read_object_info_extended() to reprepare and retry even for an
+	 * OBJECT_INFO_QUICK lookup, which normally skips that rescan to stay
+	 * fast on a genuine miss.  Reset when the packfiles are reprepared
+	 * (see odb_source_packed_prepare()).
+	 */
+	unsigned stale_packs_detected : 1;
+
 	/*
 	 * Submodule source paths that will be added as additional sources to
 	 * allow lookup of submodule objects via the main object database.
@@ -423,8 +434,9 @@ enum object_info_flags {
 	 * whether any on-disk state may have changed that may have caused the
 	 * object to appear.
 	 *
-	 * This flag is for internal use, only. The second read only occurs
-	 * when `OBJECT_INFO_QUICK` was not passed.
+	 * This flag is for internal use, only. The second read occurs when
+	 * OBJECT_INFO_QUICK was not passed, or when a vanished pack was
+	 * detected (see stale_packs_detected).
 	 */
 	OBJECT_INFO_SECOND_READ = (1 << 4),
 
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 1a12a605db..b6c1d8fdf4 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -798,8 +798,15 @@ static void odb_source_packed_prepare(struct odb_source *source,
 {
 	struct odb_source_packed *packed = odb_source_packed_downcast(source);
 
-	if (flags & ODB_PREPARE_FLUSH_CACHES)
+	if (flags & ODB_PREPARE_FLUSH_CACHES) {
 		packed->initialized = false;
+		/*
+		 * A reprepare re-scans the on-disk pack set, so any pack we
+		 * previously noticed had vanished is accounted for now; clear
+		 * the flag that forced this rescan (see stale_packs_detected).
+		 */
+		packed->base.odb->stale_packs_detected = 0;
+	}
 	if (packed->initialized)
 		return;
 
diff --git a/packfile.c b/packfile.c
index cd38be088d..bc8587d185 100644
--- a/packfile.c
+++ b/packfile.c
@@ -522,6 +522,21 @@ const char *pack_basename(struct packed_git *p)
 	return ret;
 }
 
+/* Did the pack's ".idx" vanish from disk (ENOENT), e.g. via a repack? */
+static int pack_index_is_missing(struct packed_git *p)
+{
+	char *idx_name;
+	size_t len;
+	int missing;
+
+	if (!strip_suffix(p->pack_name, ".pack", &len))
+		return 0;
+	idx_name = xstrfmt("%.*s.idx", (int)len, p->pack_name);
+	missing = access(idx_name, F_OK) < 0 && errno == ENOENT;
+	free(idx_name);
+	return missing;
+}
+
 /*
  * Do not call this directly as this leaks p->pack_fd on error return;
  * call open_packed_git() instead.
@@ -535,8 +550,20 @@ static int open_packed_git_1(struct packed_git *p)
 	ssize_t read_result;
 	const unsigned hashsz = p->repo->hash_algo->rawsz;
 
-	if (open_pack_index(p))
+	if (open_pack_index(p)) {
+		/*
+		 * A concurrent repack may have removed this pack, deleting its
+		 * ".idx" before its ".pack" (see unlink_pack_path()).  If the
+		 * index simply vanished, note the stale pack set and stay
+		 * quiet; the pack is still reported unusable.  Only a
+		 * still-present but unreadable index is worth an error.
+		 */
+		if (pack_index_is_missing(p)) {
+			p->repo->objects->stale_packs_detected = 1;
+			return -1;
+		}
 		return error("packfile %s index unavailable", p->pack_name);
+	}
 
 	if (!pack_max_fds) {
 		unsigned int max_fds = get_max_fd_limit();
@@ -552,8 +579,16 @@ static int open_packed_git_1(struct packed_git *p)
 		; /* nothing */
 
 	p->pack_fd = git_open(p->pack_name);
-	if (p->pack_fd < 0 || fstat(p->pack_fd, &st))
+	if (p->pack_fd < 0 || fstat(p->pack_fd, &st)) {
+		/*
+		 * A concurrent repack removed this pack, but its ".idx" was
+		 * already mapped (so open_pack_index() above succeeded); the
+		 * removal surfaces only now, when the ".pack" cannot be opened.
+		 */
+		if (p->pack_fd < 0 && errno == ENOENT)
+			p->repo->objects->stale_packs_detected = 1;
 		return -1;
+	}
 	pack_open_fds++;
 
 	/* If we created the struct before we had the pack we lack size. */
diff --git a/t/meson.build b/t/meson.build
index 2133c840da..28b63c486c 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -639,6 +639,7 @@ integration_tests = [
   't5333-pseudo-merge-bitmaps.sh',
   't5334-incremental-multi-pack-index.sh',
   't5335-compact-multi-pack-index.sh',
+  't5336-repack-reader-race.sh',
   't5351-unpack-large-objects.sh',
   't5400-send-pack.sh',
   't5401-update-hooks.sh',
diff --git a/t/t5336-repack-reader-race.sh b/t/t5336-repack-reader-race.sh
new file mode 100755
index 0000000000..63dad5521a
--- /dev/null
+++ b/t/t5336-repack-reader-race.sh
@@ -0,0 +1,148 @@
+#!/bin/sh
+
+test_description='reader recovery when a concurrent repack retires a pack
+
+"git repack" consolidates existing packs into a replacement pack and then
+removes the redundant packs, deleting each pack.idx before its pack.pack (see
+the ordering in unlink_pack_path()).  A reader that discovered one of those
+packs -- most easily through a multi-pack-index -- can look the pack up in the
+window where its .idx is gone but its .pack is not.
+
+For an OBJECT_INFO_QUICK lookup this is not recovered automatically: QUICK
+skips the reprepare-and-retry that a normal lookup performs, so a persistent
+reader whose pack list predates the replacement pack reports the object as
+missing even though it still lives in the replacement pack.  "git mktree
+--batch" is such a persistent QUICK reader: it stays resident across multiple
+trees and resolves each entry with OBJECT_INFO_QUICK, so before this fix it
+produced wrong output in this window.
+
+The removal can also be observed one step later, from the other side: a reader
+that already mmapped a pack.idx (so open_pack_index() succeeds without touching
+the filesystem) but has not yet opened its pack.pack.  If the pack.pack is gone
+by the time the reader opens it, the same QUICK false-negative results unless we
+notice the vanished .pack and reprepare.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup repo with a multi-pack-index over per-object packs' '
+	test_commit seed &&
+	a=$(echo A | git hash-object -w --stdin) &&
+	b=$(echo B | git hash-object -w --stdin) &&
+	echo "$a" | git pack-objects .git/objects/pack/pack >pack-a &&
+	echo "$b" | git pack-objects .git/objects/pack/pack >pack-b &&
+
+	# Drop the loose copies so the blobs resolve only through the packs the
+	# multi-pack-index references; otherwise the loose object would satisfy
+	# the lookup and the pack-removal race could never be observed.
+	git prune-packed &&
+	git multi-pack-index write &&
+
+	printf "100644 blob %s\ta\n" "$a" >tree-a-input &&
+	printf "100644 blob %s\tb\n" "$b" >tree-b-input
+'
+
+test_expect_success PIPE 'QUICK reader recovers an object whose pack was retired mid-lookup' '
+	victim=".git/objects/pack/pack-$(cat pack-b)" &&
+	mkfifo in out &&
+	test_when_finished "rm -f in out" &&
+
+	# "git mktree --batch" is a resident OBJECT_INFO_QUICK reader; start it
+	# now so its in-memory pack list / midx predates the replacement pack.
+	(git mktree --batch <in >out 2>err &) &&
+	exec 9>in &&
+	exec 8<out &&
+	test_when_finished "exec 9>&- || :" &&
+	test_when_finished "exec 8<&- || :" &&
+
+	# The first tree forces the reader to prepare its (soon stale) pack view
+	# and gives us a synchronization point.
+	cat tree-a-input >&9 &&
+	echo >&9 &&
+	read tree_a <&8 &&
+
+	# Reproduce the transient state a concurrent repack creates: a
+	# replacement pack holding every object, plus the original pack for b
+	# with its .idx removed but its .pack still present.
+	git cat-file --batch-all-objects --batch-check="%(objectname)" >all-oids &&
+	git pack-objects .git/objects/pack/pack <all-oids >/dev/null &&
+	rm -f "$victim.idx" &&
+	test_path_is_file "$victim.pack" &&
+
+	# The reader (stale pack list) now resolves b.  Without the recovery its
+	# QUICK lookup reports b missing and mktree dies; with it, b is found in
+	# the replacement pack and the misleading "index unavailable" error is
+	# not printed.
+	cat tree-b-input >&9 &&
+	echo >&9 &&
+	read tree_b <&8 &&
+	exec 9>&- &&
+
+	test -n "$tree_b" &&
+	test_grep ! "index unavailable" err
+'
+
+test_expect_success 'setup a second repo with plain (non-midx) packs' '
+	git init nomidx &&
+	(
+		cd nomidx &&
+		test_commit seed &&
+		a=$(echo A | git hash-object -w --stdin) &&
+		b=$(echo B | git hash-object -w --stdin) &&
+		echo "$a" | git pack-objects .git/objects/pack/pack >pack-a &&
+		echo "$b" | git pack-objects .git/objects/pack/pack >pack-b &&
+		git prune-packed &&
+
+		printf "100644 blob %s\ta\n" "$a" >tree-a-input &&
+		printf "100644 blob %s\tb\n" "$b" >tree-b-input
+	)
+'
+
+test_expect_success PIPE 'QUICK reader recovers when a mapped pack loses its .pack mid-lookup' '
+	(
+		cd nomidx &&
+		victim=".git/objects/pack/pack-$(cat pack-b)" &&
+		mkfifo in out &&
+
+		# We run in a subshell, so leaving the fifos and the reader
+		# descriptors open is harmless: they are cleaned up when the
+		# subshell exits (which also lets "git mktree --batch" see EOF
+		# and quit).
+		(git mktree --batch <in >out 2>err &) &&
+		exec 9>in &&
+		exec 8<out &&
+
+		# Resolving the first tree makes the reader prepare its pack
+		# list.  With no multi-pack-index, that scan mmaps every
+		# pack.idx -- including the one for b -- but only opens the
+		# pack.pack it actually reads (the one for a).  b is now in the
+		# exact state we want: its .idx is mapped while its .pack is
+		# still unopened.
+		cat tree-a-input >&9 &&
+		echo >&9 &&
+		read tree_a <&8 &&
+
+		# A concurrent repack writes a replacement pack holding every
+		# object and removes the now-redundant pack for b.  Delete only
+		# its .pack: the reader keeps the mapped .idx for b, so
+		# open_pack_index() still succeeds and the failure surfaces when
+		# we open the vanished .pack.
+		git cat-file --batch-all-objects --batch-check="%(objectname)" >all-oids &&
+		git pack-objects .git/objects/pack/pack <all-oids >/dev/null &&
+		rm -f "$victim.pack" &&
+		test_path_is_file "$victim.idx" &&
+
+		# The reader (stale pack list) now resolves b.  Without the
+		# recovery its QUICK lookup opens the missing .pack, gives up,
+		# and mktree dies; with it, the vanished .pack forces a reprepare
+		# and b is found in the replacement pack.
+		cat tree-b-input >&9 &&
+		echo >&9 &&
+		read tree_b <&8 &&
+		exec 9>&- &&
+
+		test -n "$tree_b"
+	)
+'
+
+test_done
-- 
gitgitgadget

[PATCH v2 4/4] packfile: recover when a multi-pack-index names a removed pack

[Elijah Newren via GitGitGadget]

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

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 399acd0f22..30ad7d822c 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1786,7 +1786,7 @@ 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;
diff --git a/midx.c b/midx.c
index 942505ac41..6b585f3c1a 100644
--- a/midx.c
+++ b/midx.c
@@ -595,46 +595,50 @@ 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". */
@@ -1038,7 +1042,7 @@ 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;
diff --git a/midx.h b/midx.h
index 1f2f2d5321..52fe9c81e9 100644
--- a/midx.h
+++ b/midx.h
@@ -117,8 +117,25 @@ 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,
diff --git a/odb/source-packed.c b/odb/source-packed.c
index b6c1d8fdf4..ae4c4bac40 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -17,13 +17,18 @@
 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;
@@ -35,6 +40,33 @@ static int find_pack_entry(struct odb_source_packed *store,
 		}
 	}
 
+	/*
+	 * 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;
 }
 
@@ -57,7 +89,7 @@ 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
@@ -105,7 +137,7 @@ 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);
@@ -611,7 +643,7 @@ 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;
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 68143cb5b7..4041805807 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -1393,4 +1393,84 @@ test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '
 	)
 '
 
+test_expect_success 'lookup recovers object whose midx-owning pack was removed' '
+	test_when_finished "rm -fr repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		# "keep" ends up only in the big pack; "dup" is deliberately
+		# placed in two packs so the midx has to choose an owner.
+		test_commit keep &&
+		echo duplicated-content >dup &&
+		git add dup &&
+		git commit -m dup &&
+		dup_oid=$(git rev-parse HEAD:dup) &&
+
+		# Roll every object, including dup, into a single big pack.
+		git repack -adq &&
+
+		# Build a second, "moderate" pack that also contains dup, so dup
+		# now lives in two packs that the midx will cover.
+		moderate=$(echo "$dup_oid" |
+			git pack-objects --quiet $objdir/pack/pack) &&
+
+		# Attribute dup to the moderate pack in the midx.
+		git multi-pack-index write \
+			--preferred-pack="pack-$moderate.idx" &&
+
+		# Simulate a concurrent "git repack" retiring the moderate pack:
+		# its files disappear, but the now-stale midx still names it as
+		# the owner of dup.  A valid copy of dup survives in the big pack.
+		rm -f $objdir/pack/pack-$moderate.* &&
+
+		# The midx routes the lookup to the deleted pack, and the regular
+		# pack fallback skips midx-covered packs, so without recovery dup
+		# would appear missing even though it is physically present.
+		echo blob >expect &&
+		git cat-file -t "$dup_oid" >actual &&
+		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
-- 
gitgitgadget