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

["Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>]

From: Elijah Newren <newren@gmail.com>

When a reader opens a pack it discovered on disk, open_packed_git_1()
first mmaps the pack's `.idx`.  A `git repack` running alongside us
consolidates existing packs into a new one and then removes the
redundant packs, deleting each pack's `.idx` before its `.pack` (see the
ordering in unlink_pack_path()).  A reader that had just enumerated one
of those packs -- most easily through a multi-pack-index -- can race with
the removal and find the pack gone.

Two things go wrong in that window:

  1. open_pack_index() fails, so we print

        error: packfile <path> index unavailable

     and report the pack as unusable, even though the object still lives
     in the replacement pack.

  2. A normal lookup recovers: odb_read_object_info_extended() issues a
     second read that reloads the on-disk pack state and finds the object
     in its new home, making the message above mere noise.  But an
     OBJECT_INFO_QUICK lookup deliberately skips that second read to stay
     fast on a genuine miss, so it does *not* recover: it reports the
     object as absent even though it still lives in the replacement pack.
     A resident reader that resolves objects with a QUICK lookup -- such
     as the `git mktree --batch` process the tests below drive -- then
     produces wrong results.  Even where a spurious miss is not fatal it
     is not harmless: `git upload-pack` checks a client's "have" lines
     with a QUICK lookup, and a dropped "have" removes a common object
     from the negotiation, so the client is sent more than it needs.

Recovering without giving up that speed is the trick: we keep QUICK's
fast path for a genuine miss and force the extra read only when a pack
we were already using has provably vanished.

Fix both.  Record that a pack disappeared out from under us by setting
object_database.stale_packs_detected at the three points where a reader
can notice a pack vanish beneath it:

  - In open_packed_git_1(), when open_pack_index() fails because the
    index simply vanished (its open fails with ENOENT).  Here we also
    stay silent instead of printing "index unavailable"; a genuinely
    unreadable index that is still present keeps the error, since that is
    a real problem worth surfacing.

  - In open_packed_git_1() again, from the other side of the race: when
    the `.idx` was already mapped -- so open_pack_index() returns without
    touching the filesystem -- yet opening the `.pack` fails with ENOENT.
    A reader that prepared its pack list before the repack only trips
    over the removal when it finally opens the pack file.

  - In prepare_midx_pack(), when packfile_store_load_pack() cannot open a
    pack the midx still references at all.  If both the `.idx` and the
    `.pack` are already gone -- as happens when the redundant pack is
    removed outright rather than index-first -- we never reach
    open_pack_index(), so this is the only place the vanished pack is
    observed.

Then, in odb_read_object_info_extended(), issue the second read -- which
asks the sources to reload their on-disk state (for packs, a reprepare)
and retry -- not only for non-QUICK lookups but also whenever
stale_packs_detected is set, even under OBJECT_INFO_QUICK.  An ordinary
QUICK miss, with no vanished pack, still skips the second read and stays
fast; we pay for the rescan only when we have positive evidence that the
on-disk pack set changed beneath us.  The flag is reset when the
packfiles are reprepared, in odb_source_packed_prepare().

Add t5336, regression tests that reproduce the race deterministically:
they drive a resident `git mktree --batch` reader -- which resolves each
tree entry with OBJECT_INFO_QUICK -- across both removal windows, one
removing a pack's `.idx` first while a midx routes the lookup to the
doomed pack, the other removing a pack's `.pack` after its `.idx` was
already mapped.  Each confirms the reader recovers the relocated object
instead of dying.

Assisted-by: Claude Opus 4.8 & GPT-5.6 Sol
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 midx.c                        |   6 ++
 odb.c                         |   8 +-
 odb.h                         |  16 +++-
 odb/source-packed.c           |   9 ++-
 packfile.c                    |  39 ++++++++-
 t/meson.build                 |   1 +
 t/t5336-repack-reader-race.sh | 148 ++++++++++++++++++++++++++++++++++
 7 files changed, 221 insertions(+), 6 deletions(-)
 create mode 100755 t/t5336-repack-reader-race.sh

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