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

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

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

@@ -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),
 

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

@@ -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. */

@@ -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',

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