From d4fbee1644ea7e132d153729a7d3d289971dc0cb Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 2 Sep 2026 20:03:44 -0700 Subject: [PATCH 1/3] fsck: distinguish a pack removed mid-run from a corrupt rev-index "git fsck" enumerates packs up front and later verifies each pack's reverse index. If a concurrent "git repack" removes one of those packs in the meantime, load_pack_revindex_from_disk() fails and fsck reports "unable to load rev-index", implying corruption. That is misleading: the objects are safe in the replacement pack and a quiescent retry succeeds. When the load fails because the ".pack" itself is gone (ENOENT), say so and suggest retrying once maintenance completes, rather than blaming the rev-index. A failure with the pack still present is reported as before, so genuine corruption still surfaces. This can be provoked by looping "git repack -adq" in one process while another loops "git fsck --connectivity-only": occasionally fsck trips over a pack removed after it was enumerated, and now says the pack disappeared instead of reporting a bad rev-index. Assisted-by: Claude Opus 4.8 Signed-off-by: Elijah Newren --- builtin/fsck.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/builtin/fsck.c b/builtin/fsck.c index 6d368f06f36b50..fed632f16fc8ae 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -937,7 +937,13 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress) int load_error = load_pack_revindex_from_disk(p); if (load_error < 0) { - error(_("unable to load rev-index for pack '%s'"), p->pack_name); + if (access(p->pack_name, F_OK) < 0 && errno == ENOENT) + error(_("pack '%s' disappeared while fsck was " + "running; retry after concurrent " + "maintenance completes"), p->pack_name); + else + error(_("unable to load rev-index for pack '%s'"), + p->pack_name); res = ERROR_PACK_REV_INDEX; } else if (!load_error && !load_pack_revindex(r, p) && From 58caf9d8425e47126e1130a2bcd7298bf5599e3f Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 2 Sep 2026 20:05:23 -0700 Subject: [PATCH 2/3] midx: report the concurrent-repack removal race during verify "git multi-pack-index verify" (which "git fsck" always spawns) walks the midx's packs, closing and reopening each by name as it checks object offsets. A concurrent "git repack" that unlinks a redundant pack in that window makes the reopen fail, and verify reports "failed to load pack in position N", "failed to load pack entry for oid[N]", or "failed to load pack-index for packfile ..." -- all of which read like midx corruption, though the objects are safe in the replacement pack and a quiescent retry succeeds. When such a failure is explained by a midx-referenced ".pack" having vanished (repack unlinks a redundant pack's ".idx" before its ".pack", so a missing ".pack" is the tell-tale), add a one-time hint to retry when quiescent. The existing per-failure messages are kept, so genuine corruption is still reported as before. To provoke it, "git multi-pack-index write" a repo with several packs and run "git multi-pack-index verify" while "git repack -adq" loops in the background; verify occasionally fails with one of the messages above, now followed by the retry hint. Assisted-by: Claude Opus 4.8 Signed-off-by: Elijah Newren --- midx.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 63 insertions(+), 3 deletions(-) diff --git a/midx.c b/midx.c index 6d1c548e3dae8e..a3d3f218c38f46 100644 --- a/midx.c +++ b/midx.c @@ -896,6 +896,62 @@ static void midx_report(const char *fmt, ...) va_end(ap); } +/* + * Set once we have blamed a verification failure on a pack that the midx + * references having vanished -- the signature of a concurrent repack. The + * objects are not lost (they moved to the replacement pack) and a quiescent + * retry succeeds, so we say so rather than implying midx corruption. + */ +static int verify_midx_race; + +/* + * Has the ".pack" backing the midx entry at pack_int_id been removed? A + * concurrent repack unlinks a redundant pack's ".idx" before its ".pack", so + * a missing ".pack" is the tell-tale of that removal race. + */ +static int midx_pack_vanished(struct multi_pack_index *m, uint32_t pack_int_id) +{ + struct multi_pack_index *cur = m; + struct strbuf path = STRBUF_INIT; + int vanished; + + pack_int_id = midx_for_pack(&cur, pack_int_id); + strbuf_addf(&path, "%s/pack/%s", cur->source->base.path, + cur->pack_names[pack_int_id]); + strbuf_strip_suffix(&path, ".idx"); + strbuf_addstr(&path, ".pack"); + vanished = access(path.buf, F_OK) < 0 && errno == ENOENT; + strbuf_release(&path); + return vanished; +} + +/* + * Like midx_report(), but for a failure to load the pack for pack_int_id. If + * that pack simply vanished, add a one-time hint to retry when quiescent, so a + * concurrent repack is not mistaken for midx corruption. + */ +__attribute__((format (printf, 3, 4))) +static void midx_report_pack_load(struct multi_pack_index *m, + uint32_t pack_int_id, + const char *fmt, ...) +{ + va_list ap; + + verify_midx_error = 1; + va_start(ap, fmt); + vfprintf(stderr, fmt, ap); + fprintf(stderr, "\n"); + va_end(ap); + + if (!verify_midx_race && midx_pack_vanished(m, pack_int_id)) { + verify_midx_race = 1; + fprintf(stderr, "%s\n", + _("a pack referenced by the multi-pack-index is " + "missing; concurrent maintenance may have replaced " + "it; retry when quiescent")); + } +} + struct pair_pos_vs_id { uint32_t pos; @@ -931,6 +987,7 @@ int verify_midx_file(struct odb_source_packed *source, unsigned flags) struct multi_pack_index *m = load_multi_pack_index(source); struct multi_pack_index *curr; verify_midx_error = 0; + verify_midx_race = 0; if (!m) { int result = 0; @@ -956,7 +1013,8 @@ int verify_midx_file(struct odb_source_packed *source, unsigned flags) m->num_packs + m->num_packs_in_base); for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) { if (prepare_midx_pack(m, i)) - midx_report("failed to load pack in position %d", i); + midx_report_pack_load(m, i, + "failed to load pack in position %d", i); display_progress(progress, i + 1); } @@ -1033,13 +1091,15 @@ int verify_midx_file(struct odb_source_packed *source, unsigned flags) nth_midxed_object_oid(&oid, m, pairs[i].pos); if (midx_fill_entry(m, &oid, &e, NULL) != MIDX_FILL_HIT) { - midx_report(_("failed to load pack entry for oid[%d] = %s"), + midx_report_pack_load(m, pairs[i].pack_int_id, + _("failed to load pack entry for oid[%d] = %s"), pairs[i].pos, oid_to_hex(&oid)); continue; } if (open_pack_index(e.p)) { - midx_report(_("failed to load pack-index for packfile %s"), + midx_report_pack_load(m, pairs[i].pack_int_id, + _("failed to load pack-index for packfile %s"), e.p->pack_name); break; } From 00926d8735911d54dc97acffad44ecc919ad241d Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 2 Sep 2026 20:38:39 -0700 Subject: [PATCH 3/3] midx: pin packs during verification MIDX verification closes packs between object groups and later reopens them by name. A concurrent repack can remove one in that interval, causing verification to fail on an otherwise benign race. When the referenced packs fit within a conservative fd budget, open them before verifying offsets and keep them open through the walk. Most geometrically maintained MIDXes have only O(log N) packs; larger MIDX chains retain the close-as-we-go path and its race-aware diagnostic. Do not use do_not_close for this: find_lru_pack() ignores it, so close_one_pack() may still reclaim the fd under pressure. Assisted-by: Claude Opus 4.8 Signed-off-by: Elijah Newren --- midx.c | 40 +++++++++++++++++++++++++++++++++++++--- packfile.c | 2 +- packfile.h | 3 +++ 3 files changed, 41 insertions(+), 4 deletions(-) diff --git a/midx.c b/midx.c index a3d3f218c38f46..03a69d27da6ec5 100644 --- a/midx.c +++ b/midx.c @@ -952,6 +952,24 @@ static void midx_report_pack_load(struct multi_pack_index *m, } } +/* Pin at most 256 packs, reserving 64 fds and budgeting two per pack. */ +static uint32_t midx_verify_pin_budget(void) +{ + uint32_t budget = 256; + unsigned int max_fds = get_max_fd_limit(); + + if (max_fds > 64) { + uint32_t avail = (max_fds - 64) / 2; + if (avail < budget) + budget = avail; + } else { + budget = 1; + } + if (budget < 1) + budget = 1; + return budget; +} + struct pair_pos_vs_id { uint32_t pos; @@ -983,6 +1001,8 @@ int verify_midx_file(struct odb_source_packed *source, unsigned flags) struct repository *r = source->base.odb->repo; struct pair_pos_vs_id *pairs = NULL; uint32_t i; + uint32_t total_packs; + int pin_packs; struct progress *progress = NULL; struct multi_pack_index *m = load_multi_pack_index(source); struct multi_pack_index *curr; @@ -1007,17 +1027,30 @@ int verify_midx_file(struct odb_source_packed *source, unsigned flags) if (!midx_checksum_valid(m)) midx_report(_("incorrect checksum")); + total_packs = m->num_packs + m->num_packs_in_base; + + /* Reopening by name races with repack pack removal; avoid it when possible. */ + pin_packs = total_packs <= midx_verify_pin_budget(); + if (flags & MIDX_PROGRESS) progress = start_delayed_progress(r, _("Looking for referenced packfiles"), - m->num_packs + m->num_packs_in_base); - for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) { + total_packs); + for (i = 0; i < total_packs; i++) { if (prepare_midx_pack(m, i)) midx_report_pack_load(m, i, "failed to load pack in position %d", i); display_progress(progress, i + 1); } + + if (pin_packs) { + for (i = 0; i < total_packs; i++) { + struct packed_git *p = nth_midxed_pack(m, i); + if (p) + is_pack_valid(p); + } + } stop_progress(&progress); if (m->num_objects == 0) { @@ -1079,7 +1112,8 @@ int verify_midx_file(struct odb_source_packed *source, unsigned flags) struct pack_entry e; off_t m_offset, p_offset; - if (i > 0 && pairs[i-1].pack_int_id != pairs[i].pack_int_id && + if (!pin_packs && + i > 0 && pairs[i-1].pack_int_id != pairs[i].pack_int_id && nth_midxed_pack(m, pairs[i-1].pack_int_id)) { uint32_t pack_int_id = pairs[i-1].pack_int_id; struct packed_git *p = nth_midxed_pack(m, pack_int_id); diff --git a/packfile.c b/packfile.c index 4fa5fd67c8497f..19fa3ddf11e660 100644 --- a/packfile.c +++ b/packfile.c @@ -471,7 +471,7 @@ static int close_one_pack(struct repository *r) return 0; } -static unsigned int get_max_fd_limit(void) +unsigned int get_max_fd_limit(void) { #ifdef RLIMIT_NOFILE { diff --git a/packfile.h b/packfile.h index 6d30d15a0053b3..234b9e4c7b4bd0 100644 --- a/packfile.h +++ b/packfile.h @@ -234,6 +234,9 @@ void close_pack_index(struct packed_git *); int close_pack_fd(struct packed_git *p); +/* Return the platform's maximum number of open file descriptors. */ +unsigned int get_max_fd_limit(void); + uint32_t get_pack_fanout(struct packed_git *p, uint32_t value); struct object_database;