From 2b84c3b76c477baf0f0c0150570a9204386fb179 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Tue, 18 Aug 2026 13:38:12 -0700 Subject: [PATCH 1/3] replay: fail gracefully when a merge input is unreadable When objects involved in the merge cannot be read, the merge machinery will return early with result.clean = -1, and result.tree left as NULL. pick_regular_commit() tested only "if (!result->clean)", ignoring the case where "clean < 0". That causes the code to try to use result->tree, resulting in a SIGSEGV. Handle clean < 0 explicitly; the merge machinery will already have printed messages such as "Could not read " and "collecting merge info failed for trees...", so we don't need to add much detail beyond the fact that the merge failed. Signed-off-by: Elijah Newren --- replay.c | 7 +++++++ t/t3650-replay-basics.sh | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/replay.c b/replay.c index 463c900d6c7c56..33e21b20320e01 100644 --- a/replay.c +++ b/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; diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh index 3353bc4a4dc6ed..12348b4a5f2e64 100755 --- a/t/t3650-replay-basics.sh +++ b/t/t3650-replay-basics.sh @@ -565,4 +565,38 @@ test_expect_success '--onto with --ref rejects multiple revision ranges' ' test_grep "cannot be used with multiple revision ranges" err ' +test_expect_success 'replay fails without segfault when objects are missing' ' + test_when_finished "rm -fr unreadable" && + git init unreadable && + ( + cd unreadable && + + test_write_lines l1 l2 l3 l4 l5 l6 l7 l8 >f && + git add f && + git commit -m base && + git branch base && + + test_write_lines l1 l2 l3 l4 l5 l6 l7 CHANGED >f && + git commit -am side && + git branch side && + + git switch -c onto base && + test_write_lines CHANGED l2 l3 l4 l5 l6 l7 l8 >f && + git commit -am onto && + + # The replay works while every object is readable. + git replay --onto onto base..side && + + # Removing the onto tree makes parse_tree() fail during the + # incore merge, driving clean < 0 with a NULL result tree. + onto_tree=$(git rev-parse onto^{tree}) && + obj=$(test_oid_to_path "$onto_tree") && + mv .git/objects/${obj} saved-tree && + + # Ensure replay gracefully handles the missing object + test_must_fail git replay --onto onto base..side 2>err && + test_grep -e "Could not read" -e "collecting merge info failed" err + ) +' + test_done From f7edd23d6689954f2fa8cc1ac2dd149f559858e7 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 22 Aug 2026 23:33:32 -0700 Subject: [PATCH 2/3] packfile: recover object lookups racing a concurrent repack 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 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 --- 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 37f082dbdd5558..942505ac414fd2 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 6bbea640334305..4bb9662c65a26f 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 1264d4ce7d116a..8b91e6f8ba31b6 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 1a12a605dbc62e..b6c1d8fdf44b62 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 cd38be088dcee8..bc8587d1856f2b 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 2133c840da63dc..28b63c486cc255 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 00000000000000..63dad5521a76e5 --- /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 out 2>err &) && + exec 9>in && + exec 8&- || :" && + 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 /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 out 2>err &) && + exec 9>in && + exec 8&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 /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 From 60dc2ad975bb6d8403f851e16116c84d9ee59c6c Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Tue, 18 Aug 2026 13:38:12 -0700 Subject: [PATCH 3/3] packfile: recover when a multi-pack-index names a removed pack A geometric repack writes a new pack and multi-pack-index and then deletes the packs the new one subsumes. A process still using the previous MIDX keeps seeing a removed pack listed as the owner of some objects. Since a MIDX attributes each object to exactly one pack, such an object is served only through its recorded owner; if that owner was just removed, find_pack_entry() cannot serve it -- fill_midx_entry() routes to the missing pack, and the regular pack fallback deliberately skips every MIDX-covered pack, so a surviving copy in another covered pack (e.g. a kept base pack) is never consulted. Unlike the ordinary "a pack's .idx is mapped but its .pack is gone" race, the second read does not rescue us -- and not only for OBJECT_INFO_QUICK callers. Reloading the on-disk pack set does not reload the borrowed, cached MIDX (freeing it under the code that caches the "struct multi_pack_index *" would be a use-after-free), so the stale MIDX keeps routing to the removed pack and the surviving copy stays hidden behind the covered-pack skip. cat-file, rev-list and pack-objects can thus all spuriously fail with "unable to read object". Teach find_pack_entry() to recover. fill_midx_entry() now returns a tri-state, distinguishing "absent from the MIDX" from "present but the owning pack is unavailable"; in the latter case, once the regular fallback has also missed, scan the MIDX's packs directly for a surviving copy. Do the scan only on the second read (OBJECT_INFO_SECOND_READ): by then the cheaper on-disk reload has run, so an object merely relocated into a new (non-covered) pack has already been found by the regular fallback, and only a genuine hidden duplicate reaches the rescan. QUICK callers that would skip the second read are steered into it by the preceding commit's stale_packs_detected flag, which prepare_midx_pack() sets when it cannot open the owning pack. Reloading the stale MIDX would be a more complete fix but is much more involved (the borrowers above need proper invalidation), so leave that for later. Assisted-by: Claude Opus 4.8 & GPT-5.6 Sol Helped-by: Jeff King Signed-off-by: Elijah Newren --- builtin/pack-objects.c | 2 +- midx.c | 38 ++++++++++-------- midx.h | 21 +++++++++- odb/source-packed.c | 42 ++++++++++++++++--- t/t5319-multi-pack-index.sh | 80 +++++++++++++++++++++++++++++++++++++ 5 files changed, 158 insertions(+), 25 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 399acd0f225d93..30ad7d822c159e 100644 --- a/builtin/pack-objects.c +++ b/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; diff --git a/midx.c b/midx.c index 942505ac414fd2..6b585f3c1a1cb2 100644 --- a/midx.c +++ b/midx.c @@ -595,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". */ @@ -1038,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; diff --git a/midx.h b/midx.h index 1f2f2d53214da5..52fe9c81e945c0 100644 --- a/midx.h +++ b/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, diff --git a/odb/source-packed.c b/odb/source-packed.c index b6c1d8fdf44b62..ae4c4bac4002db 100644 --- a/odb/source-packed.c +++ b/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 = × } - 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; diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh index 68143cb5b76952..4041805807fdd9 100755 --- a/t/t5319-multi-pack-index.sh +++ b/t/t5319-multi-pack-index.sh @@ -1393,4 +1393,84 @@ 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_expect_success 'repeated QUICK lookups recover after owning pack removed' ' + test_when_finished "rm -fr repo" && + git init repo && + ( + cd repo && + + # Two blobs, each duplicated across packs so the midx must pick + # an owning pack, and each attributed to the same moderate pack. + echo one >f1 && + echo two >f2 && + git add f1 f2 && + git commit -m dups && + d1=$(git rev-parse HEAD:f1) && + d2=$(git rev-parse HEAD:f2) && + + # Roll every object, including d1 and d2, into one big pack, + # then build a moderate pack that also holds both blobs. + git repack -adq && + moderate=$(printf "%s\n%s\n" "$d1" "$d2" | + git pack-objects --quiet $objdir/pack/pack) && + + git multi-pack-index write \ + --preferred-pack="pack-$moderate.idx" && + + # Retire the moderate pack; the stale midx still names it as the + # owner of both blobs, each of which survives in the big pack. + rm -f $objdir/pack/pack-$moderate.* && + + # One resident QUICK reader ("git mktree --batch") resolves both + # blobs. The first lookup recovers d1 and caches the owning + # packs failure; unless that failure keeps re-arming the second + # read, the lookup of d2 skips its recovering read and the reader + # dies reporting d2 as missing. + printf "100644 blob %s\tf1\n\n100644 blob %s\tf2\n\n" \ + "$d1" "$d2" | + git mktree --batch >trees && + test_line_count = 2 trees + ) +' + test_done