[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

[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

[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

[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