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

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.

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

diff --git a/odb/source-packed.c b/odb/source-packed.c
index 0890704e76..de96215069 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -31,6 +31,35 @@ static int find_pack_entry(struct odb_source_packed *store,
 		}
 	}
 
+	/*
+	 * Recovery for a concurrent-repack race: a MIDX can name an owning
+	 * pack for an object that a simultaneous repack has since deleted,
+	 * even though the object still exists in another pack the same MIDX
+	 * covers (e.g. a kept base pack that geometric repack did not rewrite).
+	 * If the object is present in a MIDX yet none of the paths above could
+	 * serve it, its recorded owning pack has become unavailable.  The
+	 * regular fallback above deliberately skips MIDX-covered packs, so
+	 * scan this MIDX's packs directly to find the surviving copy.  The
+	 * bsearch gate keeps genuine misses (objects absent from the MIDX) on
+	 * the fast path.
+	 */
+	if (store->midx) {
+		struct multi_pack_index *m = store->midx;
+		uint32_t midx_pos, i;
+
+		if (bsearch_midx(oid, m, &midx_pos)) {
+			for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {
+				struct packed_git *p;
+
+				if (prepare_midx_pack(m, i))
+					continue;
+				p = nth_midxed_pack(m, i);
+				if (p && packfile_fill_entry(p, oid, e))
+					return 1;
+			}
+		}
+	}
+
 	return 0;
 }
 
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 68143cb5b7..2b8ff6f3ed 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -1393,4 +1393,44 @@ test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '
 	)
 '
 
+test_expect_success 'lookup recovers object whose midx-owning pack was removed' '
+	test_when_finished "rm -fr repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		# "keep" ends up only in the big pack; "dup" is deliberately
+		# placed in two packs so the midx has to choose an owner.
+		test_commit keep &&
+		echo duplicated-content >dup &&
+		git add dup &&
+		git commit -m dup &&
+		dup_oid=$(git rev-parse HEAD:dup) &&
+
+		# Roll every object, including dup, into a single big pack.
+		git repack -adq &&
+
+		# Build a second, "moderate" pack that also contains dup, so dup
+		# now lives in two packs that the midx will cover.
+		moderate=$(echo "$dup_oid" |
+			git pack-objects --quiet $objdir/pack/pack) &&
+
+		# Attribute dup to the moderate pack in the midx.
+		git multi-pack-index write \
+			--preferred-pack="pack-$moderate.idx" &&
+
+		# Simulate a concurrent "git repack" retiring the moderate pack:
+		# its files disappear, but the now-stale midx still names it as
+		# the owner of dup.  A valid copy of dup survives in the big pack.
+		rm -f $objdir/pack/pack-$moderate.* &&
+
+		# The midx routes the lookup to the deleted pack, and the regular
+		# pack fallback skips midx-covered packs, so without recovery dup
+		# would appear missing even though it is physically present.
+		echo blob >expect &&
+		git cat-file -t "$dup_oid" >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done
-- 
gitgitgadget