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

builtin/mktree.c

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

builtin/pack-objects.c

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

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;
 	}
 
@@ -589,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". */
@@ -1032,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;

midx.h

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

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,

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

odb/source-packed.c

@@ -17,13 +17,18 @@
 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;
@@ -35,6 +40,33 @@ static int find_pack_entry(struct odb_source_packed *store,
 		}
 	}
 
+	/*
+	 * 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 (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&
+	    (flags & OBJECT_INFO_SECOND_READ)) {
+		struct multi_pack_index *m = store->midx;
+		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;
 }
 
@@ -57,7 +89,7 @@ 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
@@ -105,7 +137,7 @@ 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);
@@ -611,7 +643,7 @@ 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;
@@ -798,8 +830,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;
 

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

replay.c

@@ -327,6 +327,13 @@ static struct commit *pick_regular_commit(struct repository *repo,
 	merge_opt->ancestor = NULL;
 	merge_opt->branch2 = NULL;
 
+	if (result->clean < 0) {
+		error(_("merge of %s onto %s failed"),
+		      oid_to_hex(&pickme->object.oid),
+		      oid_to_hex(&replayed_base->object.oid));
+		return NULL;
+	}
+
 	if (!result->clean)
 		return NULL;
 

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