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

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

From: Elijah Newren <[email protected]>

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.

---
 replay.c                 |  7 +++++++
 t/t3650-replay-basics.sh | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

index 463c900d6c..33e21b2032 100644
@@ -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;
 

index 3353bc4a4d..d66b8edb95 100755
@@ -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

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

Oops, you're right.

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

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.

From: Elijah Newren <[email protected]>

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.

---
 odb/source-packed.c         | 29 +++++++++++++++++++++++++++
 t/t5319-multi-pack-index.sh | 40 +++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

index 0890704e76..de96215069 100644
@@ -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;
 }
 

index 68143cb5b7..2b8ff6f3ed 100755
@@ -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

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

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

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

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

Yep.

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

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

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

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

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

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

So between the two cases, it sounds like things (or at least the low-level lookups) are working as designed, and there is no bug. Or am I misunderstanding something?

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

-Peff

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

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

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

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

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

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

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

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

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

-Peff

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

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

-Peff

I implemented that in v2.

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

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

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

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

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

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

-Peff

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.

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

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.

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

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 <[email protected]>

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

From: Elijah Newren <[email protected]>

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.

---
 replay.c                 |  7 +++++++
 t/t3650-replay-basics.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

index 463c900d6c..33e21b2032 100644
@@ -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;
 

index 3353bc4a4d..12348b4a5f 100755
@@ -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

From: Elijah Newren <[email protected]>

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.

---
 builtin/mktree.c | 3 +++
 1 file changed, 3 insertions(+)

index 4084e32476..dc2d293c3d 100644
@@ -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

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

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

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

#leftoverbits, perhaps?

-Peff

From: Elijah Newren <[email protected]>

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.

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

index 37f082dbdd..942505ac41 100644
@@ -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;
 	}
 

index 6bbea64033..4bb9662c65 100644
@@ -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,

index 1264d4ce7d..8b91e6f8ba 100644
@@ -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),
 

index 1a12a605db..b6c1d8fdf4 100644
@@ -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;
 

index cd38be088d..bc8587d185 100644
@@ -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. */

index 2133c840da..28b63c486c 100644
@@ -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',

gitgitgadget

Maybe I am still being dense, but this description does not make any sense to me at all.

The _point_ of QUICK is to accept those false negatives. It is the right thing for upload-pack to do, to avoid re-scans for objects which we simply don't have (and don't necessarily expect to have).

It sounds like mktree is wrong to be using QUICK at all. It comes from 817b0f6027 (mktree: do not check type of remote objects, 2022-06-21) which rewrote a call to vanilla oid_object_info(). From the description there it probably should be using SKIP_FETCH_OBJECT but not QUICK. Or possibly it should use neither unless --missing is given.

So I don't see QUICK itself here violating any contract (even if it _could_ find the object in some cases with just a little more work, as in the case that we were discussing for v1).

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

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

It's far more likely that I am the one being dense. My rough line of thinking:

* We see "packfile ... index unavailable" in our logging
* There's only one thing that remove packfiles
* Investigate the mechanism
* Look for other affected callers (e.g. mktree --batch)
* Consider corrective measures

Steps 1-4 above are probably fine, and step 5 may have been where I went off the rails. My thinking there, wrong or right, was:

* It makes sense that we don't want to reprepare most of the time
* ...but _if_ we know of the existence of some specific packfile in this process and that packfile has since disappeared by the time we go to open or read it, is that a special case? Should it be?

I'll drop this patch and instead send a small mktree change that stops passing OBJECT_INFO_QUICK (keeping SKIP_FETCH_OBJECT), so mktree recovers via the normal reprepare like every other non-QUICK reader. That removes the packfile.c changes entirely, so both the reload-under-QUICK hack and the .idx/.pack raciness you noted in pack_index_is_missing() go away with them.

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

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

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

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

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

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

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

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

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

But I think the key thing is separating:

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

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

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

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

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

-Peff

From: Elijah Newren <[email protected]>

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.

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

index 399acd0f22..30ad7d822c 100644
@@ -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;

index 942505ac41..6b585f3c1a 100644
@@ -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;

index 1f2f2d5321..52fe9c81e9 100644
@@ -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,

gitgitgadget

Any suggestions for alternate names? fill_midx_entry_result? midx_fill_entry?

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

Yes, sorry.

Thanks for taking a look!

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

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

-Peff

Changes since v2:

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

Changes since v1:

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

Cover letter addendum/update:

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

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

Original cover letter (focused on the final patch):

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

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

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

cat-file)
repository repair (fsck/repack) spurious read-miss failure

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

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

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

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

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

Range-diff vs v2:

 1:  36bf2ce17b = 1:  36bf2ce17b replay: fail gracefully when a merge input is unreadable
 2:  3f3b75690e = 2:  3f3b75690e mktree: plug per-tree leak in --batch mode
 3:  fc98f48ddb < -:  ---------- packfile: recover object lookups racing a concurrent repack
 -:  ---------- > 3:  79ce753c68 mktree: do not use OBJECT_INFO_QUICK when checking objects
 4:  eacf6ba4b1 ! 4:  9b0966df9a packfile: recover when a multi-pack-index names a removed pack

@@ Commit message
previous MIDX keeps seeing a removed pack listed as the owner of some objects. Since a MIDX attributes each object to exactly one pack, such an object is served only through its recorded owner; if that owner was
- just removed, find_pack_entry() cannot serve it -- fill_midx_entry()
- routes to the missing pack, and the regular pack fallback deliberately
- skips every MIDX-covered pack, so a surviving copy in another covered
- pack (e.g. a kept base pack) is never consulted.
+ just removed, find_pack_entry() cannot serve it -- the MIDX lookup routes
+ to the missing pack, and the regular pack fallback deliberately skips
+ every MIDX-covered pack, so a surviving copy in another covered pack
+ (e.g. a kept base pack) is never consulted.

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

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

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

Reloading the stale MIDX would be a more complete fix but is much more involved (the borrowers above need proper invalidation), so leave that
@@ builtin/pack-objects.c: static int want_object_in_pack_mtime(const struct object
struct pack_entry e;

- if (m && fill_midx_entry(m, oid, &e, NULL)) {
-+ if (m && fill_midx_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {
++ if (m && midx_fill_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {
want = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime); if (want != -1)
return want;
@@ midx.c: uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos
- const struct object_id *oid,
- struct pack_entry *e,
- struct packed_git **bad_pack)
-+enum midx_fill_result fill_midx_entry(struct multi_pack_index *m,
++enum midx_fill_result midx_fill_entry(struct multi_pack_index *m,
+ const struct object_id *oid,
+ struct pack_entry *e,
+ struct packed_git **bad_pack)
@@ midx.c: uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos

if (prepare_midx_pack(m, pack_int_id))
- return 0;
-+ goto owner_unavailable;
++ return MIDX_FILL_OWNER_UNAVAILABLE;
p = m->packs[pack_int_id - m->num_packs_in_base];

-- /*
-- * We are about to tell the caller where they can locate the
-- * requested object. We better make sure the packfile is
-- * still here and can be accessed before supplying that
-- * answer, as it may have been deleted since the MIDX was
-- * loaded!
-- */
-+ /* Make sure the pack is still present before pointing at it. */
+ /*
+@@ midx.c: int fill_midx_entry(struct multi_pack_index *m,
+ * loaded!
+ */
if (!is_pack_valid(p))
- return 0;
-+ goto owner_unavailable;
++ return MIDX_FILL_OWNER_UNAVAILABLE;

if (oidset_size(&p->bad_objects) &&
oidset_contains(&p->bad_objects, oid)) {
@@ midx.c: uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos

- return 1;
+ return MIDX_FILL_HIT;
-+
-+owner_unavailable:
-+ /*
-+ * Re-arm stale_packs_detected on every such lookup, not just the
-+ * first: prepare_midx_pack() caches the failure, so without this a
-+ * later lookup of the same vanished pack would leave the flag clear
-+ * and a QUICK reader would skip its recovering second read.
-+ */
-+ m->source->base.odb->stale_packs_detected = 1;
-+ return MIDX_FILL_OWNER_UNAVAILABLE;
}

/* Match "foo.idx" against either "foo.pack" _or_ "foo.idx". */
@@ midx.c: int verify_midx_file(struct odb_source_packed *source, unsigned flags)
nth_midxed_object_oid(&oid, m, pairs[i].pos);

- if (!fill_midx_entry(m, &oid, &e, NULL)) {
-+ if (fill_midx_entry(m, &oid, &e, NULL) != MIDX_FILL_HIT) {
++ if (midx_fill_entry(m, &oid, &e, NULL) != MIDX_FILL_HIT) {
midx_report(_("failed to load pack entry for oid[%d] = %s"),
pairs[i].pos, oid_to_hex(&oid));
continue;
@@ midx.h: uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos
+ MIDX_FILL_OWNER_UNAVAILABLE,
+};
+
-+enum midx_fill_result fill_midx_entry(struct multi_pack_index *m, ++enum midx_fill_result midx_fill_entry(struct multi_pack_index *m,
+ const struct object_id *oid,
+ struct pack_entry *e,
+ struct packed_git **bad_pack);
@@ odb/source-packed.c
- if (store->midx && fill_midx_entry(store->midx, oid, e, bad_pack))
- return 1;
+ if (store->midx) {
-+ midx_result = fill_midx_entry(store->midx, oid, e, bad_pack);
++ midx_result = midx_fill_entry(store->midx, oid, e, bad_pack);
+ if (midx_result == MIDX_FILL_HIT)
+ return 1;
+ }
@@ odb/source-packed.c: static int odb_source_packed_freshen_object(struct odb_sour
if (e.p->is_cruft)
return 0;

+ ## t/helper/test-read-midx.c ##
+@@ t/helper/test-read-midx.c: static int read_midx_file(const char *object_dir, const char *checksum,
+ for (i = 0; i < m->num_objects; i++) {
+ nth_midxed_object_oid(&oid, m,
+ i + m->num_objects_in_base);
+- fill_midx_entry(m, &oid, &e, NULL);
++ midx_fill_entry(m, &oid, &e, NULL);
+
+ printf("%s %"PRIu64"\t%s\n",
+ oid_to_hex(&oid), e.offset, e.p->pack_name);
+
## t/t5319-multi-pack-index.sh ##
@@ t/t5319-multi-pack-index.sh: test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '
)
@@ t/t5319-multi-pack-index.sh: test_expect_success 'pack.preferBitmapTips interpre
+ test_cmp expect actual
+ )
+'
-+
-+test_expect_success 'repeated QUICK lookups recover after owning pack removed' '
-+ test_when_finished "rm -fr repo" &&
-+ git init repo &&
-+ (
-+ cd repo &&
-+
-+ # Two blobs, each duplicated across packs so the midx must pick
-+ # an owning pack, and each attributed to the same moderate pack.
-+ echo one >f1 &&
-+ echo two >f2 &&
-+ git add f1 f2 &&
-+ git commit -m dups &&
-+ d1=$(git rev-parse HEAD:f1) &&
-+ d2=$(git rev-parse HEAD:f2) &&
-+
-+ # Roll every object, including d1 and d2, into one big pack,
-+ # then build a moderate pack that also holds both blobs.
-+ git repack -adq &&
-+ moderate=$(printf "%s\n%s\n" "$d1" "$d2" |
-+ git pack-objects --quiet $objdir/pack/pack) &&
-+
-+ git multi-pack-index write \
-+ --preferred-pack="pack-$moderate.idx" &&
-+
-+ # Retire the moderate pack; the stale midx still names it as the
-+ # owner of both blobs, each of which survives in the big pack.
-+ rm -f $objdir/pack/pack-$moderate.* &&
-+
-+ # One resident QUICK reader ("git mktree --batch") resolves both
-+ # blobs. The first lookup recovers d1 and caches the owning
-+ # packs failure; unless that failure keeps re-arming the second
-+ # read, the lookup of d2 skips its recovering read and the reader
-+ # dies reporting d2 as missing.
-+ printf "100644 blob %s\tf1\n\n100644 blob %s\tf2\n\n" \
-+ "$d1" "$d2" |
-+ git mktree --batch >trees &&
-+ test_line_count = 2 trees
-+ )
-+'
+
test_done

gitgitgadget

From: Elijah Newren <[email protected]>

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.

---
 replay.c                 |  7 +++++++
 t/t3650-replay-basics.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

index 463c900d6c..33e21b2032 100644
@@ -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;
 

index 3353bc4a4d..12348b4a5f 100755
@@ -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

From: Elijah Newren <[email protected]>

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.

---
 builtin/mktree.c | 3 +++
 1 file changed, 3 insertions(+)

index 4084e32476..dc2d293c3d 100644
@@ -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

From: Elijah Newren <[email protected]>

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

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

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

---
 builtin/mktree.c  |  1 -
 t/t1010-mktree.sh | 48 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)

index dc2d293c3d..45ae2af3b5 100644
@@ -125,7 +125,6 @@ static void mktree_line(struct repository *repo, char *buf, int nul_term_line, i
 	oi.typep = &obj_type;
 	if (odb_read_object_info_extended(repo->objects, &oid, &oi,
 					  OBJECT_INFO_LOOKUP_REPLACE |
-					  OBJECT_INFO_QUICK |
 					  OBJECT_INFO_SKIP_FETCH_OBJECT) < 0)
 		obj_type = -1;
 

gitgitgadget

I think this line of reasoning is fine.

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

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

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

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

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

So the patch looks good to me overall.

-Peff

From: Elijah Newren <[email protected]>

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

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

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

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

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

---
 builtin/pack-objects.c      |  2 +-
 midx.c                      | 20 +++++++++---------
 midx.h                      | 21 +++++++++++++++++--
 odb/source-packed.c         | 42 ++++++++++++++++++++++++++++++++-----
 t/helper/test-read-midx.c   |  2 +-
 t/t5319-multi-pack-index.sh | 40 +++++++++++++++++++++++++++++++++++
 6 files changed, 108 insertions(+), 19 deletions(-)

index 399acd0f22..751d5d3449 100644
@@ -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 && midx_fill_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {
 			want = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);
 			if (want != -1)
 				return want;

index 37f082dbdd..6d1c548e3d 100644
@@ -589,23 +589,23 @@ 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 midx_fill_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;
+		return MIDX_FILL_OWNER_UNAVAILABLE;
 	p = m->packs[pack_int_id - m->num_packs_in_base];
 
 	/*

@@ -616,19 +616,19 @@ int fill_midx_entry(struct multi_pack_index *m,
 	* loaded!
 	*/
 	if (!is_pack_valid(p))
-		return 0;
+		return MIDX_FILL_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;
 }
 
 /* Match "foo.idx" against either "foo.pack" _or_ "foo.idx". */

@@ -1032,7 +1032,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 (midx_fill_entry(m, &oid, &e, NULL) != MIDX_FILL_HIT) {
 			midx_report(_("failed to load pack entry for oid[%d] = %s"),
 				    pairs[i].pos, oid_to_hex(&oid));
 			continue;

index 1f2f2d5321..4b768769b9 100644
@@ -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 midx_fill_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,

index 27a05da957..9c5e308761 100644
@@ -82,7 +82,7 @@ static int read_midx_file(const char *object_dir, const char *checksum,
 		for (i = 0; i < m->num_objects; i++) {
 			nth_midxed_object_oid(&oid, m,
 					      i + m->num_objects_in_base);
-			fill_midx_entry(m, &oid, &e, NULL);
+			midx_fill_entry(m, &oid, &e, NULL);
 
 			printf("%s %"PRIu64"\t%s\n",
 			       oid_to_hex(&oid), e.offset, e.p->pack_name);

index 68143cb5b7..2b8ff6f3ed 100755
@@ -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

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

Shall we mark it for 'next' then?

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

Patrick

Yeah, that sounds good to me.

-Peff

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

thanks,
-Stolee

Thanks, all.

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

tl;dr: Good patch. LGTM.

This tri-state is valuable!

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

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

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

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

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

Thanks,
-Stolee