{"thread":{"id":"66192","subject":"[PATCH 0/2] Objects treated as missing despite being present, due to race with geometric repacking","startedAt":"2026-08-18T22:34:09Z","lastAt":"2026-09-01T17:12:48Z","messageCount":49,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Patrick Steinhardt","Elijah Newren","Jeff King","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"550781","messageId":"pull.2207.git.1787092446.gitgitgadget@gmail.com","threadId":"66192","inReplyTo":null,"subject":"[PATCH 0/2] Objects treated as missing despite being present, due to race with geometric repacking","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-18T22:34:04Z","receivedAt":"2026-08-18T22:34:09Z","isPatch":true,"body":"When an object is found in multiple packs that are in a multi-pack-index,\nand a subsequent geometric repacking creates a new multi-pack-index and\nremoves the pack that was considered the owner of the object in the old\nmulti-pack-index, then an already-running process that had opened the old\nmulti-pack-index and hadn't yet opened the removed packfile will not be able\nto access the object -- lookups will return it as missing. Additionally,\nreplay has a separate bug where a missing object causes a SIGSEGV rather\nthan an error message.\n\nThis appears to affect a very small percentage of git operations in\nproduction since it is a tiny window, but I've found evidence of it\noccurring in at least eight distinct server-side operations, covering seven\ndifferent git commands:\n\ngit operation symptom\n----------------------------------- -----------------------------\ngit replay (server-side rebase) SIGSEGV (this series, 1/2)\ngit merge-tree spurious read-miss failure\ngit diff (raw and tree-vs-tree) spurious read-miss failure\ngit rev-list --count spurious read-miss failure\ngit merge-base spurious read-miss failure\nobject/rev resolution (rev-parse, spurious read-miss failure\n cat-file)\nrepository repair (fsck/repack) spurious read-miss failure\n\n\nThere are also commands that could be changing behavior without throwing an\nerror -- e.g. object negotiation thinking an object doesn't exist and\ninstead negotiating based on an older common commit, or cat-file --batch\nreporting that some objects don't exist.\n\nThis series fixes the replay bug first, since it's simpler; investigating\nit, together with my other recent repacking work, is what led me to the\nunderlying multi-pack-index issue that 2/2 addresses.\n\nElijah Newren (2):\n replay: fail gracefully when a merge input is unreadable\n packfile: recover when a multi-pack-index names a removed pack\n\n odb/source-packed.c | 29 +++++++++++++++++++++++++++\n replay.c | 7 +++++++\n t/t3650-replay-basics.sh | 35 ++++++++++++++++++++++++++++++++\n t/t5319-multi-pack-index.sh | 40 +++++++++++++++++++++++++++++++++++++\n 4 files changed, 111 insertions(+)\n\n\nbase-commit: 18e66859d87fb4b76599f73460b54f0848c76b16\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2207%2Fnewren%2Fmidx-removed-pack-recovery-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2207/newren/midx-removed-pack-recovery-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2207\n-- \ngitgitgadget\n"},{"id":"550782","messageId":"321af575e0a9e0c22c70c1809f6fbf0265b05d4c.1787092446.git.gitgitgadget@gmail.com","threadId":"66192","inReplyTo":"pull.2207.git.1787092446.gitgitgadget@gmail.com","subject":"[PATCH 1/2] replay: fail gracefully when a merge input is unreadable","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-18T22:34:05Z","receivedAt":"2026-08-18T22:34:11Z","isPatch":true,"body":"From: Elijah Newren \n\nWhen objects involved in the merge cannot be read, the merge machinery\nwill return early with result.clean = -1, and result.tree left as NULL.\npick_regular_commit() tested only \"if (!result->clean)\", ignoring the\ncase where \"clean < 0\". That causes the code to try to use\nresult->tree, resulting in a SIGSEGV.\n\nHandle clean < 0 explicitly; the merge machinery will already have printed\nmessages such as \"Could not read \" and \"collecting merge info\nfailed for trees...\", so we don't need to add much detail beyond the\nfact that the merge failed.\n\nSigned-off-by: Elijah Newren \n---\n replay.c | 7 +++++++\n t/t3650-replay-basics.sh | 35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+)\n\ndiff --git a/replay.c b/replay.c\nindex 463c900d6c..33e21b2032 100644\n--- a/replay.c\n+++ b/replay.c\n@@ -327,6 +327,13 @@ static struct commit *pick_regular_commit(struct repository *repo,\n \tmerge_opt->ancestor = NULL;\n \tmerge_opt->branch2 = NULL;\n \n+\tif (result->clean < 0) {\n+\t\terror(_(\"merge of %s onto %s failed\"),\n+\t\t oid_to_hex(&pickme->object.oid),\n+\t\t oid_to_hex(&replayed_base->object.oid));\n+\t\treturn NULL;\n+\t}\n+\n \tif (!result->clean)\n \t\treturn NULL;\n \ndiff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh\nindex 3353bc4a4d..d66b8edb95 100755\n--- a/t/t3650-replay-basics.sh\n+++ b/t/t3650-replay-basics.sh\n@@ -565,4 +565,39 @@ test_expect_success '--onto with --ref rejects multiple revision ranges' '\n \ttest_grep \"cannot be used with multiple revision ranges\" err\n '\n \n+test_expect_success 'replay fails without segfault when objects are missing' '\n+\ttest_when_finished \"rm -fr unreadable\" &&\n+\tgit init unreadable &&\n+\t(\n+\t\tcd unreadable &&\n+\n+\t\ttest_write_lines l1 l2 l3 l4 l5 l6 l7 l8 >f &&\n+\t\tgit add f &&\n+\t\tgit commit -m base &&\n+\t\tgit branch base &&\n+\n+\t\ttest_write_lines l1 l2 l3 l4 l5 l6 l7 CHANGED >f &&\n+\t\tgit commit -am side &&\n+\t\tgit branch side &&\n+\n+\t\tgit switch -c onto base &&\n+\t\ttest_write_lines CHANGED l2 l3 l4 l5 l6 l7 l8 >f &&\n+\t\tgit commit -am onto &&\n+\n+\t\t# The replay works while every object is readable.\n+\t\tgit replay --onto onto base..side &&\n+\n+\t\t# Removing the onto tree makes parse_tree() fail during the\n+\t\t# incore merge, driving clean < 0 with a NULL result tree.\n+\t\tonto_tree=$(git rev-parse onto^{tree}) &&\n+\t\tobj=$(test_oid_to_path \"$onto_tree\") &&\n+\t\tmv .git/objects/${obj} saved-tree &&\n+\n+\t\t# Ensure replay gracefully handles the missing object\n+\t\ttest_must_fail git replay --onto onto base..side 2>err &&\n+\t\ttest_grep ! \"[Ss]egmentation\" err &&\n+\t\ttest_grep \"Could not read\\|collecting merge info failed\" err\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"550783","messageId":"5792c08f4ee0f9627ab1432d91299fe676e0a2f5.1787092446.git.gitgitgadget@gmail.com","threadId":"66192","inReplyTo":"pull.2207.git.1787092446.gitgitgadget@gmail.com","subject":"[PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-18T22:34:06Z","receivedAt":"2026-08-18T22:34:13Z","isPatch":true,"body":"From: Elijah Newren \n\nWhen a geometric repack runs concurrently with other git processes, it\ncan write a new pack and multi-pack-index and then delete older packs\nthat the new one subsumes. One or more of those older packs may have\nbeen indexed by the previous multi-pack-index. A process that already\nhad the previous multi-pack-index open keeps using it, and that stale\nindex still records the removed pack(s) as owning some objects.\n\nBecause a multi-pack-index attributes each object to exactly one pack,\nan object that exists in multiple covered packs is served only through\nits recorded owner. If that owner is the pack a concurrent repack just\nremoved, find_pack_entry() cannot serve the object: fill_midx_entry()\nroutes the lookup to the missing pack (prepare_midx_pack() fails), and\nthe regular pack fallback deliberately skips every multi-pack-index\ncovered pack. The object is reported missing even though a perfectly\ngood copy survives in another covered pack -- for example a large \"base\"\npack that geometric repacking intentionally kept.\n\nThe false negative is not limited to one caller. Any reader\n(cat-file, rev-list, pack-objects, ...) can spuriously fail with\n\"unable to read object\", and callers that only ask whether an object\nexists get a wrong answer too, since the OBJECT_INFO_QUICK path never\nretries. Writers that merge in-core, such as \"git replay\", are hit\nhardest: merge-ort treats the unreadable tree as a premature abort, sets\nresult.clean < 0, and returns without a result tree.\n\nTeach find_pack_entry() to recover. After the normal multi-pack-index\nlookup and the regular pack fallback both miss, check whether the object\nis nonetheless present in a covered multi-pack-index (bsearch_midx()).\nIf it is, its recorded owner must have become unavailable, so scan that\nindex's packs directly for a surviving copy. The bsearch gate keeps\ngenuine misses (i.e. objects absent from the index) on the fast path, and\nbecause the recovery lives in find_pack_entry() itself it also fixes the\nOBJECT_INFO_QUICK callers that never reprepare.\n\nThis recovers the object without touching the multi-pack-index itself.\nReloading the stale index would be a more complete fix but would be much\nmore involved: other code (pack bitmaps, object name disambiguation)\nborrows and caches the \"struct multi_pack_index *\" across object reads,\nso freeing it underneath them would be a use-after-free. Refreshing the\nindex with proper invalidation of those borrowers is left for future\nwork.\n\nSigned-off-by: Elijah Newren \n---\n odb/source-packed.c | 29 +++++++++++++++++++++++++++\n t/t5319-multi-pack-index.sh | 40 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 69 insertions(+)\n\ndiff --git a/odb/source-packed.c b/odb/source-packed.c\nindex 0890704e76..de96215069 100644\n--- a/odb/source-packed.c\n+++ b/odb/source-packed.c\n@@ -31,6 +31,35 @@ static int find_pack_entry(struct odb_source_packed *store,\n \t\t}\n \t}\n \n+\t/*\n+\t * Recovery for a concurrent-repack race: a MIDX can name an owning\n+\t * pack for an object that a simultaneous repack has since deleted,\n+\t * even though the object still exists in another pack the same MIDX\n+\t * covers (e.g. a kept base pack that geometric repack did not rewrite).\n+\t * If the object is present in a MIDX yet none of the paths above could\n+\t * serve it, its recorded owning pack has become unavailable. The\n+\t * regular fallback above deliberately skips MIDX-covered packs, so\n+\t * scan this MIDX's packs directly to find the surviving copy. The\n+\t * bsearch gate keeps genuine misses (objects absent from the MIDX) on\n+\t * the fast path.\n+\t */\n+\tif (store->midx) {\n+\t\tstruct multi_pack_index *m = store->midx;\n+\t\tuint32_t midx_pos, i;\n+\n+\t\tif (bsearch_midx(oid, m, &midx_pos)) {\n+\t\t\tfor (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {\n+\t\t\t\tstruct packed_git *p;\n+\n+\t\t\t\tif (prepare_midx_pack(m, i))\n+\t\t\t\t\tcontinue;\n+\t\t\t\tp = nth_midxed_pack(m, i);\n+\t\t\t\tif (p && packfile_fill_entry(p, oid, e))\n+\t\t\t\t\treturn 1;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \treturn 0;\n }\n \ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 68143cb5b7..2b8ff6f3ed 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -1393,4 +1393,44 @@ test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '\n \t)\n '\n \n+test_expect_success 'lookup recovers object whose midx-owning pack was removed' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t# \"keep\" ends up only in the big pack; \"dup\" is deliberately\n+\t\t# placed in two packs so the midx has to choose an owner.\n+\t\ttest_commit keep &&\n+\t\techo duplicated-content >dup &&\n+\t\tgit add dup &&\n+\t\tgit commit -m dup &&\n+\t\tdup_oid=$(git rev-parse HEAD:dup) &&\n+\n+\t\t# Roll every object, including dup, into a single big pack.\n+\t\tgit repack -adq &&\n+\n+\t\t# Build a second, \"moderate\" pack that also contains dup, so dup\n+\t\t# now lives in two packs that the midx will cover.\n+\t\tmoderate=$(echo \"$dup_oid\" |\n+\t\t\tgit pack-objects --quiet $objdir/pack/pack) &&\n+\n+\t\t# Attribute dup to the moderate pack in the midx.\n+\t\tgit multi-pack-index write \\\n+\t\t\t--preferred-pack=\"pack-$moderate.idx\" &&\n+\n+\t\t# Simulate a concurrent \"git repack\" retiring the moderate pack:\n+\t\t# its files disappear, but the now-stale midx still names it as\n+\t\t# the owner of dup. A valid copy of dup survives in the big pack.\n+\t\trm -f $objdir/pack/pack-$moderate.* &&\n+\n+\t\t# The midx routes the lookup to the deleted pack, and the regular\n+\t\t# pack fallback skips midx-covered packs, so without recovery dup\n+\t\t# would appear missing even though it is physically present.\n+\t\techo blob >expect &&\n+\t\tgit cat-file -t \"$dup_oid\" >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"550835","messageId":"xmqqfr0augls.fsf@gitster.g","threadId":"66192","inReplyTo":"321af575e0a9e0c22c70c1809f6fbf0265b05d4c.1787092446.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] replay: fail gracefully when a merge input is unreadable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-19T18:09:51Z","receivedAt":"2026-08-19T18:09:54Z","isPatch":true,"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> When objects involved in the merge cannot be read, the merge machinery\n> will return early with result.clean = -1, and result.tree left as NULL.\n> pick_regular_commit() tested only \"if (!result->clean)\", ignoring the\n> case where \"clean < 0\". That causes the code to try to use\n> result->tree, resulting in a SIGSEGV.\n>\n> Handle clean < 0 explicitly; the merge machinery will already have printed\n> messages such as \"Could not read \" and \"collecting merge info\n> failed for trees...\", so we don't need to add much detail beyond the\n> fact that the merge failed.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> replay.c | 7 +++++++\n> t/t3650-replay-basics.sh | 35 +++++++++++++++++++++++++++++++++++\n> 2 files changed, 42 insertions(+)\n>\n> diff --git a/replay.c b/replay.c\n> index 463c900d6c..33e21b2032 100644\n> --- a/replay.c\n> +++ b/replay.c\n> @@ -327,6 +327,13 @@ static struct commit *pick_regular_commit(struct repository *repo,\n> \tmerge_opt->ancestor = NULL;\n> \tmerge_opt->branch2 = NULL;\n> \n> +\tif (result->clean < 0) {\n> +\t\terror(_(\"merge of %s onto %s failed\"),\n> +\t\t oid_to_hex(&pickme->object.oid),\n> +\t\t oid_to_hex(&replayed_base->object.oid));\n> +\t\treturn NULL;\n> +\t}\n> +\n> \tif (!result->clean)\n> \t\treturn NULL;\n\nHmph, so anything but \"0 < result->clean\" is a failure, but we by\nmistake took any non-zero value as OK? That is an obvious mistake.\nWell spotted and fixed.\n\n> +\t\t# Ensure replay gracefully handles the missing object\n> +\t\ttest_must_fail git replay --onto onto base..side 2>err &&\n> +\t\ttest_grep ! \"[Ss]egmentation\" err &&\n> +\t\ttest_grep \"Could not read\\|collecting merge info failed\" err\n\n\"test_must_fail\" means \"the tested command must fail voluntarily and\nin a controlled way\", so a segfaulting git-replay invocation would\nnot pass test_must_fail. Hence, there is no need to separately\ntest \"test_grep ! '[sS]egmentation'\".\n\nBesides, the spelling used by strsignal() is implementation-defined,\nso you cannot reliably grep for it anyway.\n\n> +\t)\n> +'\n> +\n> test_done\n\nThanks.\n"},{"id":"550836","messageId":"xmqqbjayug34.fsf@gitster.g","threadId":"66192","inReplyTo":"5792c08f4ee0f9627ab1432d91299fe676e0a2f5.1787092446.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-19T18:21:03Z","receivedAt":"2026-08-19T18:21:06Z","isPatch":true,"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> @@ -31,6 +31,35 @@ static int find_pack_entry(struct odb_source_packed *store,\n> \t\t}\n> \t}\n> \n> +\t/*\n> +\t * Recovery for a concurrent-repack race: a MIDX can name an owning\n> +\t * pack for an object that a simultaneous repack has since deleted,\n> +\t * even though the object still exists in another pack the same MIDX\n> +\t * covers (e.g. a kept base pack that geometric repack did not rewrite).\n> +\t * If the object is present in a MIDX yet none of the paths above could\n> +\t * serve it, its recorded owning pack has become unavailable. The\n> +\t * regular fallback above deliberately skips MIDX-covered packs, so\n> +\t * scan this MIDX's packs directly to find the surviving copy. The\n> +\t * bsearch gate keeps genuine misses (objects absent from the MIDX) on\n> +\t * the fast path.\n> +\t */\n> +\tif (store->midx) {\n> +\t\tstruct multi_pack_index *m = store->midx;\n> +\t\tuint32_t midx_pos, i;\n> +\n> +\t\tif (bsearch_midx(oid, m, &midx_pos)) {\n> +\t\t\tfor (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {\n> +\t\t\t\tstruct packed_git *p;\n> +\n> +\t\t\t\tif (prepare_midx_pack(m, i))\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\tp = nth_midxed_pack(m, i);\n> +\t\t\t\tif (p && packfile_fill_entry(p, oid, e))\n> +\t\t\t\t\treturn 1;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> \treturn 0;\n> }\n\nI'll prepare an evil-merge to rewrite this line to\n\n\t\t\tif (p && packfile_fill_entry(p, oid, e, bad_pack))\n\nto adjust to the API change another topic in-flight brings in when\nmerging these patches to 'seen'.\n\nThis is strictly FYI. You do not need to rebase on top of the other\ntopic, until I and/or the author of the other topic ask you.\n\nThanks.\n\n"},{"id":"550871","messageId":"aoayppoxHAkcFTBN@pks.im","threadId":"66192","inReplyTo":"5792c08f4ee0f9627ab1432d91299fe676e0a2f5.1787092446.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-20T07:54:14Z","receivedAt":"2026-08-20T07:54:40Z","isPatch":true,"body":"On Tue, Aug 18, 2026 at 10:34:06PM +0000, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> When a geometric repack runs concurrently with other git processes, it\n> can write a new pack and multi-pack-index and then delete older packs\n> that the new one subsumes. One or more of those older packs may have\n> been indexed by the previous multi-pack-index. A process that already\n> had the previous multi-pack-index open keeps using it, and that stale\n> index still records the removed pack(s) as owning some objects.\n> \n> Because a multi-pack-index attributes each object to exactly one pack,\n> an object that exists in multiple covered packs is served only through\n> its recorded owner. If that owner is the pack a concurrent repack just\n> removed, find_pack_entry() cannot serve the object: fill_midx_entry()\n> routes the lookup to the missing pack (prepare_midx_pack() fails), and\n> the regular pack fallback deliberately skips every multi-pack-index\n> covered pack. The object is reported missing even though a perfectly\n> good copy survives in another covered pack -- for example a large \"base\"\n> pack that geometric repacking intentionally kept.\n\nOkay. Rephrasing in my own words: the object in question exists in two\npacks covered by the MIDX. We rewrite one of those two packs, and the\nMIDX used to reference the object via the pack we're about to rewrite.\nConsequently, the MIDX is stale now and it cannot be used to find the\nobject anymore because its pack has disappeared. And as we know to skip\nsearching packfiles for the object that are already covered by the MIDX\nwe won't be able to find it via the second packfile, either.\n\n> The false negative is not limited to one caller. Any reader\n> (cat-file, rev-list, pack-objects, ...) can spuriously fail with\n> \"unable to read object\", and callers that only ask whether an object\n> exists get a wrong answer too, since the OBJECT_INFO_QUICK path never\n> retries. Writers that merge in-core, such as \"git replay\", are hit\n> hardest: merge-ort treats the unreadable tree as a premature abort, sets\n> result.clean < 0, and returns without a result tree.\n\nHm. Isn't there a slight variant of the race though for any caller that\ndoes not use OBJECT_INFO_QUICK?\n\nNamely, the packfile containing our object disappears and is being\nwritten to a new packfile, and that file is the only one containing it.\nWithout OBJECT_INFO_QUICK we would be fine: we notice the object could\nnot be found, and then we perform a second read that makes the \"packed\"\nbackend reload its packfiles. It would find the new packfile, and\nbecause it's not covered by its MIDX it would use it to surface the\nobject. But without OBJECT_INFO_QUICK that's not the case, as we would\nskip reloading packfiles altogether, and hence we would not be able to\nfind that object at all.\n\nAs far as I can see though, we don't seem to pass OBJECT_INFO_QUICK in\nany of the mentioned readers. I could very well be missing something\nhere, but I would have thought that those readers are fine in this\nscenario?\n\n> diff --git a/odb/source-packed.c b/odb/source-packed.c\n> index 0890704e76..de96215069 100644\n> --- a/odb/source-packed.c\n> +++ b/odb/source-packed.c\n> @@ -31,6 +31,35 @@ static int find_pack_entry(struct odb_source_packed *store,\n> \t\t}\n> \t}\n> \n> +\t/*\n> +\t * Recovery for a concurrent-repack race: a MIDX can name an owning\n> +\t * pack for an object that a simultaneous repack has since deleted,\n> +\t * even though the object still exists in another pack the same MIDX\n> +\t * covers (e.g. a kept base pack that geometric repack did not rewrite).\n> +\t * If the object is present in a MIDX yet none of the paths above could\n> +\t * serve it, its recorded owning pack has become unavailable. The\n> +\t * regular fallback above deliberately skips MIDX-covered packs, so\n> +\t * scan this MIDX's packs directly to find the surviving copy. The\n> +\t * bsearch gate keeps genuine misses (objects absent from the MIDX) on\n> +\t * the fast path.\n> +\t */\n> +\tif (store->midx) {\n> +\t\tstruct multi_pack_index *m = store->midx;\n> +\t\tuint32_t midx_pos, i;\n> +\n> +\t\tif (bsearch_midx(oid, m, &midx_pos)) {\n\nOkay. I was initially worried that we now unconditionally search through\nall packfiles a second time, as that could have an impact on\nperformance. But we really only do this in case we have a MIDX and we\nknow that the MIDX _should_ have contained the object, but didn't yield\nit.\n\n> +\t\t\tfor (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {\n> +\t\t\t\tstruct packed_git *p;\n> +\n> +\t\t\t\tif (prepare_midx_pack(m, i))\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\tp = nth_midxed_pack(m, i);\n> +\t\t\t\tif (p && packfile_fill_entry(p, oid, e))\n> +\t\t\t\t\treturn 1;\n> +\t\t\t}\n\nAnd here we now loop through all packs covered by the MIDX and manually\ntry to look up the object in those. Makes sense.\n\n> +\t\t}\n> +\t}\n\nI was wondering whether a preferable fix would be to eagerly load\nany packfile referenced by the MIDX when loading the MIDX itself. And if\nthat fails, we'd ignore the MIDX altogether. This would guarantee that\nthe MIDX remains valid, and we wouldn't have to worry about any\ndisappearing packfiles.\n\nThe downside is of course that we now eagerly open packfiles, and we\ndidn't have to do that before. So I think your fix is preferable, as we\ncan rather easily detect the case where the MIDX should've yielded the\nobject but didn't, and consequently the additional search only triggers\nin very specific edge cases.\n\nOverall I think this patch looks good to me. The one thing that I'm a\nbit puzzled about is the above discussion around OBJECT_INFO_QUICK. I\nfeel like I'm missing something there.\n\nThanks!\n\nPatrick\n"},{"id":"550970","messageId":"CABPp-BEBbdmE9q+98gWq-wLzDdhJOyazcHF=pP95o5AcmgCv1Q@mail.gmail.com","threadId":"66192","inReplyTo":"aoayppoxHAkcFTBN@pks.im","subject":"Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-08-21T01:36:09Z","receivedAt":"2026-08-21T01:36:22Z","isPatch":true,"body":"On Thu, Aug 20, 2026 at 12:54 AM Patrick Steinhardt wrote:\n>\n> On Tue, Aug 18, 2026 at 10:34:06PM +0000, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > When a geometric repack runs concurrently with other git processes, it\n> > can write a new pack and multi-pack-index and then delete older packs\n> > that the new one subsumes. One or more of those older packs may have\n> > been indexed by the previous multi-pack-index. A process that already\n> > had the previous multi-pack-index open keeps using it, and that stale\n> > index still records the removed pack(s) as owning some objects.\n> >\n> > Because a multi-pack-index attributes each object to exactly one pack,\n> > an object that exists in multiple covered packs is served only through\n> > its recorded owner. If that owner is the pack a concurrent repack just\n> > removed, find_pack_entry() cannot serve the object: fill_midx_entry()\n> > routes the lookup to the missing pack (prepare_midx_pack() fails), and\n> > the regular pack fallback deliberately skips every multi-pack-index\n> > covered pack. The object is reported missing even though a perfectly\n> > good copy survives in another covered pack -- for example a large \"base\"\n> > pack that geometric repacking intentionally kept.\n>\n> Okay. Rephrasing in my own words: the object in question exists in two\n> packs covered by the MIDX. We rewrite one of those two packs, and the\n> MIDX used to reference the object via the pack we're about to rewrite.\n> Consequently, the MIDX is stale now and it cannot be used to find the\n> object anymore because its pack has disappeared. And as we know to skip\n> searching packfiles for the object that are already covered by the MIDX\n> we won't be able to find it via the second packfile, either.\n\nYep.\n\n> > The false negative is not limited to one caller. Any reader\n> > (cat-file, rev-list, pack-objects, ...) can spuriously fail with\n> > \"unable to read object\", and callers that only ask whether an object\n> > exists get a wrong answer too, since the OBJECT_INFO_QUICK path never\n> > retries. Writers that merge in-core, such as \"git replay\", are hit\n> > hardest: merge-ort treats the unreadable tree as a premature abort, sets\n> > result.clean < 0, and returns without a result tree.\n>\n> Hm. Isn't there a slight variant of the race though for any caller that\n> does not use OBJECT_INFO_QUICK?\n>\n> Namely, the packfile containing our object disappears and is being\n> written to a new packfile, and that file is the only one containing it.\n> Without OBJECT_INFO_QUICK we would be fine: we notice the object could\n> not be found, and then we perform a second read that makes the \"packed\"\n> backend reload its packfiles. It would find the new packfile, and\n> because it's not covered by its MIDX it would use it to surface the\n> object. But without OBJECT_INFO_QUICK that's not the case, as we would\n> skip reloading packfiles altogether, and hence we would not be able to\n> find that object at all.\n>\n> As far as I can see though, we don't seem to pass OBJECT_INFO_QUICK in\n> any of the mentioned readers. I could very well be missing something\n> here, but I would have thought that those readers are fine in this\n> scenario?\n\nNicely caught -- and you're right that the readers named above are\nfine: they're all non-QUICK, so the second read reloads the packfiles\nand finds the object in its new, non-MIDX-covered home, exactly as you\ndescribe.\n\nBut the variant you describe is a real bug for QUICK callers that\ndon't get that second read -- e.g. upload-pack's object-existence\nchecks and mktree --batch. I have three more race-condition patches\nto clean up and submit, and this is one of them: it forces the reload\neven under OBJECT_INFO_QUICK once we notice a pack has vanished out\nfrom under us.\n\nYour wording also makes me realize that my fix in this unsubmitted\npatch still has a hole: it triggers when opening the pack .idx fails,\nbut if the timing is such that the .idx is already mmapped and only\nthe .pack has gone missing, it won't fire. I'll look into that before\nsubmitting...and then clean up/submit my two other race fixes as well.\n"},{"id":"550971","messageId":"CABPp-BGFpLi+FEoJOXvT=wBtexXiDmJ9vXQfc5JnBDrUk+zbDA@mail.gmail.com","threadId":"66192","inReplyTo":"xmqqfr0augls.fsf@gitster.g","subject":"Re: [PATCH 1/2] replay: fail gracefully when a merge input is unreadable","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-08-21T01:44:05Z","receivedAt":"2026-08-21T01:44:17Z","isPatch":true,"body":"On Wed, Aug 19, 2026 at 11:09 AM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > From: Elijah Newren \n> >\n> > When objects involved in the merge cannot be read, the merge machinery\n> > will return early with result.clean = -1, and result.tree left as NULL.\n> > pick_regular_commit() tested only \"if (!result->clean)\", ignoring the\n> > case where \"clean < 0\". That causes the code to try to use\n> > result->tree, resulting in a SIGSEGV.\n> >\n> > Handle clean < 0 explicitly; the merge machinery will already have printed\n> > messages such as \"Could not read \" and \"collecting merge info\n> > failed for trees...\", so we don't need to add much detail beyond the\n> > fact that the merge failed.\n> >\n> > Signed-off-by: Elijah Newren \n> > ---\n> > replay.c | 7 +++++++\n> > t/t3650-replay-basics.sh | 35 +++++++++++++++++++++++++++++++++++\n> > 2 files changed, 42 insertions(+)\n> >\n> > diff --git a/replay.c b/replay.c\n> > index 463c900d6c..33e21b2032 100644\n> > --- a/replay.c\n> > +++ b/replay.c\n> > @@ -327,6 +327,13 @@ static struct commit *pick_regular_commit(struct repository *repo,\n> > merge_opt->ancestor = NULL;\n> > merge_opt->branch2 = NULL;\n> >\n> > + if (result->clean < 0) {\n> > + error(_(\"merge of %s onto %s failed\"),\n> > + oid_to_hex(&pickme->object.oid),\n> > + oid_to_hex(&replayed_base->object.oid));\n> > + return NULL;\n> > + }\n> > +\n> > if (!result->clean)\n> > return NULL;\n>\n> Hmph, so anything but \"0 < result->clean\" is a failure, but we by\n> mistake took any non-zero value as OK? That is an obvious mistake.\n> Well spotted and fixed.\n\nThanks, but the bug was also caused by me -- e787e664da64 (replay:\nintroduce pick_regular_commit(), 2023-11-24) -- so not sure I should\nget much credit for finding it three years later.\n\n> > + # Ensure replay gracefully handles the missing object\n> > + test_must_fail git replay --onto onto base..side 2>err &&\n> > + test_grep ! \"[Ss]egmentation\" err &&\n> > + test_grep \"Could not read\\|collecting merge info failed\" err\n>\n> \"test_must_fail\" means \"the tested command must fail voluntarily and\n> in a controlled way\", so a segfaulting git-replay invocation would\n> not pass test_must_fail. Hence, there is no need to separately\n> test \"test_grep ! '[sS]egmentation'\".\n\nOops, you're right.\n\nYou said on 2/2 that I don't need to rebase because you're putting\ntogether an evil merge. Do you want me to resubmit with this line\nremoved (without changing the series' base), or would you rather I\navoid that to prevent merging work for you?\n"},{"id":"550975","messageId":"xmqqh5korvog.fsf@gitster.g","threadId":"66192","inReplyTo":"CABPp-BGFpLi+FEoJOXvT=wBtexXiDmJ9vXQfc5JnBDrUk+zbDA@mail.gmail.com","subject":"Re: [PATCH 1/2] replay: fail gracefully when a merge input is unreadable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-21T03:37:03Z","receivedAt":"2026-08-21T03:37:05Z","isPatch":true,"body":"Elijah Newren writes:\n\n>> > + # Ensure replay gracefully handles the missing object\n>> > + test_must_fail git replay --onto onto base..side 2>err &&\n>> > + test_grep ! \"[Ss]egmentation\" err &&\n>> > + test_grep \"Could not read\\|collecting merge info failed\" err\n>>\n>> \"test_must_fail\" means \"the tested command must fail voluntarily and\n>> in a controlled way\", so a segfaulting git-replay invocation would\n>> not pass test_must_fail. Hence, there is no need to separately\n>> test \"test_grep ! '[sS]egmentation'\".\n>\n> Oops, you're right.\n>\n> You said on 2/2 that I don't need to rebase because you're putting\n> together an evil merge. Do you want me to resubmit with this line\n> removed (without changing the series' base), or would you rather I\n> avoid that to prevent merging work for you?\n\nI can remove that line myself, or you can resubmit on the same base.\nThe evil-merge machinery uses the usual 3-way merge, so I do not\nthink removal of that \"test_grep !\" line would break it either way.\n\nThanks.\n"},{"id":"551102","messageId":"20260824044822.GA142844@coredump.intra.peff.net","threadId":"66192","inReplyTo":"CABPp-BEBbdmE9q+98gWq-wLzDdhJOyazcHF=pP95o5AcmgCv1Q@mail.gmail.com","subject":"Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-24T04:48:22Z","receivedAt":"2026-08-24T04:48:30Z","isPatch":true,"body":"On Thu, Aug 20, 2026 at 06:36:09PM -0700, Elijah Newren wrote:\n\n> > > The false negative is not limited to one caller. Any reader\n> > > (cat-file, rev-list, pack-objects, ...) can spuriously fail with\n> > > \"unable to read object\", and callers that only ask whether an object\n> > > exists get a wrong answer too, since the OBJECT_INFO_QUICK path never\n> > > retries. Writers that merge in-core, such as \"git replay\", are hit\n> > > hardest: merge-ort treats the unreadable tree as a premature abort, sets\n> > > result.clean < 0, and returns without a result tree.\n> >\n> > Hm. Isn't there a slight variant of the race though for any caller that\n> > does not use OBJECT_INFO_QUICK?\n> >\n> > Namely, the packfile containing our object disappears and is being\n> > written to a new packfile, and that file is the only one containing it.\n> > Without OBJECT_INFO_QUICK we would be fine: we notice the object could\n> > not be found, and then we perform a second read that makes the \"packed\"\n> > backend reload its packfiles. It would find the new packfile, and\n> > because it's not covered by its MIDX it would use it to surface the\n> > object. But without OBJECT_INFO_QUICK that's not the case, as we would\n> > skip reloading packfiles altogether, and hence we would not be able to\n> > find that object at all.\n> >\n> > As far as I can see though, we don't seem to pass OBJECT_INFO_QUICK in\n> > any of the mentioned readers. I could very well be missing something\n> > here, but I would have thought that those readers are fine in this\n> > scenario?\n> \n> Nicely caught -- and you're right that the readers named above are\n> fine: they're all non-QUICK, so the second read reloads the packfiles\n> and finds the object in its new, non-MIDX-covered home, exactly as you\n> describe.\n\nOK, so do I understand correctly that you _can't_ get the \"unable to\nread object\" result that the commit message claims? I.e., the reprepare\n/ packfile reload is helps us (just like it does for the non-midx case\nwhen an idx has been mapped but the pack disappears before we open it).\n\nSo there is no bug there for non-QUICK callers. But then...\n\n> But the variant you describe is a real bug for QUICK callers that\n> don't get that second read -- e.g. upload-pack's object-existence\n> checks and mktree --batch. I have three more race-condition patches\n> to clean up and submit, and this is one of them: it forces the reload\n> even under OBJECT_INFO_QUICK once we notice a pack has vanished out\n> from under us.\n\nThis seems wrong. The whole point of the QUICK flag is that the caller\nis OK producing a false negative for an object lookup, and it would\nprefer that outcome to spending the time to reload. If there are callers\npassing QUICK that aren't OK with false negatives, they are broken and\nthe fix should be there. But repreparing the packs for a QUICK miss is\ngoing to reintroduce the performance problems that QUICK was introduced\nto help.\n\nSo between the two cases, it sounds like things (or at least the\nlow-level lookups) are working as designed, and there is no bug. Or am I\nmisunderstanding something?\n\n> Your wording also makes me realize that my fix in this unsubmitted\n> patch still has a hole: it triggers when opening the pack .idx fails,\n> but if the timing is such that the .idx is already mmapped and only\n> the .pack has gone missing, it won't fire. I'll look into that before\n> submitting...and then clean up/submit my two other race fixes as well.\n\nI think it would be fine, for the same reason that regular idx lookups\nare fine. In packfile_fill_entry() we call is_pack_valid(), checking\nthat the pack is still there (and relying on its side effect of leaving\nthe fd/mmap open so that it remains accessible even if the file is\ndeleted).\n\n-Peff\n"},{"id":"551103","messageId":"20260824045529.GB142844@coredump.intra.peff.net","threadId":"66192","inReplyTo":"5792c08f4ee0f9627ab1432d91299fe676e0a2f5.1787092446.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-24T04:55:29Z","receivedAt":"2026-08-24T04:55:31Z","isPatch":true,"body":"On Tue, Aug 18, 2026 at 10:34:06PM +0000, Elijah Newren via GitGitGadget wrote:\n\n> Teach find_pack_entry() to recover. After the normal multi-pack-index\n> lookup and the regular pack fallback both miss, check whether the object\n> is nonetheless present in a covered multi-pack-index (bsearch_midx()).\n> If it is, its recorded owner must have become unavailable, so scan that\n> index's packs directly for a surviving copy. The bsearch gate keeps\n> genuine misses (i.e. objects absent from the index) on the fast path, and\n> because the recovery lives in find_pack_entry() itself it also fixes the\n> OBJECT_INFO_QUICK callers that never reprepare.\n\nYou don't even have to pay the bsearch() again. We'd already have looked\nin the midx earlier in the function. We just need to distinguish three\ncases:\n\n 1. it was not in the midx (or there is no midx)\n\n 2. it was in the midx but we could not load it (pack invalid, or\n object in the bad_objects list)\n\n 3. it was in the midx and is available\n\nIn fill_midx_entry() we return a boolean that lumps cases 1+2 together,\nversus case 3. It could return a tri-state that would let us distinguish\nall three. And then your fallback would kick in only for case 2 (case 3\nalready returned with success, and case 1 means the midx does not even\nmention the object).\n\nThis is all assuming the fallback is worth pursuing. I'm still puzzled\nwhy this specific case would matter when we have the same (already\nsolved) problem of reading a regular .idx whose .pack has gone away.\n\n-Peff\n"},{"id":"551105","messageId":"aovTA4F04aX8SPTU@pks.im","threadId":"66192","inReplyTo":"20260824044822.GA142844@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-24T05:13:39Z","receivedAt":"2026-08-24T05:13:48Z","isPatch":true,"body":"On Mon, Aug 24, 2026 at 12:48:22AM -0400, Jeff King wrote:\n> So between the two cases, it sounds like things (or at least the\n> low-level lookups) are working as designed, and there is no bug. Or am I\n> misunderstanding something?\n\nI agree that QUICK is working as designed, and that callers that pass it\nwithout being able to accommodate for false negatives are buggy. But the\npatch sent by Elijah still fixes an actual bug where we may not find an\nobject that is contained in two MIDXd packs where the preferred pack for\na respective object vanishes concurrently. Filling the packfile entry\nvia the MIDX will fail because the pack vanished, and the lookup via the\nnon-preferred pack will fail, too, because we skip over any packs that\nare covered by the MIDX when doing the non-MIDX lookup. Consequently, we\nwon't find the object at all.\n\nThat case is broken no matter whether we pass QUICK or not.\n\nPatrick\n"},{"id":"551108","messageId":"aovZRjcIbAUqswFT@pks.im","threadId":"66192","inReplyTo":"20260824045529.GB142844@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-24T05:40:22Z","receivedAt":"2026-08-24T05:40:30Z","isPatch":true,"body":"On Mon, Aug 24, 2026 at 12:55:29AM -0400, Jeff King wrote:\n> On Tue, Aug 18, 2026 at 10:34:06PM +0000, Elijah Newren via GitGitGadget wrote:\n> \n> > Teach find_pack_entry() to recover. After the normal multi-pack-index\n> > lookup and the regular pack fallback both miss, check whether the object\n> > is nonetheless present in a covered multi-pack-index (bsearch_midx()).\n> > If it is, its recorded owner must have become unavailable, so scan that\n> > index's packs directly for a surviving copy. The bsearch gate keeps\n> > genuine misses (i.e. objects absent from the index) on the fast path, and\n> > because the recovery lives in find_pack_entry() itself it also fixes the\n> > OBJECT_INFO_QUICK callers that never reprepare.\n> \n> You don't even have to pay the bsearch() again. We'd already have looked\n> in the midx earlier in the function. We just need to distinguish three\n> cases:\n> \n> 1. it was not in the midx (or there is no midx)\n> \n> 2. it was in the midx but we could not load it (pack invalid, or\n> object in the bad_objects list)\n> \n> 3. it was in the midx and is available\n> \n> In fill_midx_entry() we return a boolean that lumps cases 1+2 together,\n> versus case 3. It could return a tri-state that would let us distinguish\n> all three. And then your fallback would kick in only for case 2 (case 3\n> already returned with success, and case 1 means the midx does not even\n> mention the object).\n> \n> This is all assuming the fallback is worth pursuing. I'm still puzzled\n> why this specific case would matter when we have the same (already\n> solved) problem of reading a regular .idx whose .pack has gone away.\n\nI've tried to clarify in a parallel message already, but the issue is\nthat we skip over any packfiles that covered by a MIDX when doing the\nlookup. So any secondary packfiles that contain the object would be\ncompletely ignored, and that's why we don't find the object there.\n\nBut this mail here suggests an alternative fix: instead of re-scanning\nall packfiles like the patch proposes, wouldn't the proper fix be to not\nignore _all_ MIDX'd packs, but only the pack that _should_ have\ncontained the object?\n\nUltimately though, this would be equivalent to turning the function's\nreturn value into a tri-state as suggested by Peff here. The only case\nwhere the issue can occur is in case (2), and in that case we should not\nskip MIDX'd packs at all as the MIDX'd pack that should've contained the\npack does not exist anyway.\n\nPatrick\n"},{"id":"551109","messageId":"20260824065539.GA149254@coredump.intra.peff.net","threadId":"66192","inReplyTo":"aovTA4F04aX8SPTU@pks.im","subject":"Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-24T06:55:39Z","receivedAt":"2026-08-24T06:55:41Z","isPatch":true,"body":"On Mon, Aug 24, 2026 at 07:13:39AM +0200, Patrick Steinhardt wrote:\n\n> On Mon, Aug 24, 2026 at 12:48:22AM -0400, Jeff King wrote:\n> > So between the two cases, it sounds like things (or at least the\n> > low-level lookups) are working as designed, and there is no bug. Or am I\n> > misunderstanding something?\n> \n> I agree that QUICK is working as designed, and that callers that pass it\n> without being able to accommodate for false negatives are buggy. But the\n> patch sent by Elijah still fixes an actual bug where we may not find an\n> object that is contained in two MIDXd packs where the preferred pack for\n> a respective object vanishes concurrently. Filling the packfile entry\n> via the MIDX will fail because the pack vanished, and the lookup via the\n> non-preferred pack will fail, too, because we skip over any packs that\n> are covered by the MIDX when doing the non-MIDX lookup. Consequently, we\n> won't find the object at all.\n\nAh, OK. I get it now. Thanks for explaining.\n\nIt feels like the midx is foiling the usual reprepare strategy\n(well, SECOND_READ these days) because we don't actually flush it for\nthe second read. Assuming the writing side always generates a new midx\n(that no longer references the to-be-deleted pack) before deleting the\npack itself, then we'd be able to find the object by refreshing the\nmidx. Just like we find new objects by refreshing the pack list and\nfinding the new .idx files.\n\nAnd I guess that's what the original commit message was saying here:\n\n This recovers the object without touching the multi-pack-index itself.\n Reloading the stale index would be a more complete fix but would be much\n more involved: other code (pack bitmaps, object name disambiguation)\n borrows and caches the \"struct multi_pack_index *\" across object reads,\n so freeing it underneath them would be a use-after-free. Refreshing the\n index with proper invalidation of those borrowers is left for future\n work.\n\nThat's not a problem for packs because we _don't_ free the packfile\nstructs. We keep them around forever. So presumably we'd have to do the\nsame for stale midxs. But I agree that it might end up more complicated\nthan we'd like (especially because there's so much \"there is only one\nmidx\" assumption baked into various parts of the code). So working\naround it in a more immediate way makes some sense.\n\n> That case is broken no matter whether we pass QUICK or not.\n\nRight. It would be OK to skip Elijah's fallback workaround when\nSECOND_READ is not set; the QUICK callers are prepared to accept the\nfalse negative. But since it is cheap-ish to do the fallback check, it\nis perhaps OK to just do it on the first pass?\n\nI wonder how true that is. Imagine you had a midx covering a million\npacks, and you notice an object is missing, but you're in QUICK mode. Do\nyou really want to individually check each of those million pack idx\nfiles (that were otherwise not even opened or mmap'd because they're\ncovered by the midx!).\n\nI think it's mostly academic. You'd have to do the million-pack search\nif we are not in QUICK mode. And the point of QUICK mode is mostly\navoiding tons of fruitless searches for objects we don't actually have.\nThe bsearch() conditional means that we _know_ this is a racy negative\nand not just some object we never even had. So it would trigger\ngenerally only when the search is useful.\n\n-Peff\n"},{"id":"551110","messageId":"20260824070317.GB149254@coredump.intra.peff.net","threadId":"66192","inReplyTo":"aovZRjcIbAUqswFT@pks.im","subject":"Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-24T07:03:17Z","receivedAt":"2026-08-24T07:03:18Z","isPatch":true,"body":"On Mon, Aug 24, 2026 at 07:40:22AM +0200, Patrick Steinhardt wrote:\n\n> > This is all assuming the fallback is worth pursuing. I'm still puzzled\n> > why this specific case would matter when we have the same (already\n> > solved) problem of reading a regular .idx whose .pack has gone away.\n> \n> I've tried to clarify in a parallel message already, but the issue is\n> that we skip over any packfiles that covered by a MIDX when doing the\n> lookup. So any secondary packfiles that contain the object would be\n> completely ignored, and that's why we don't find the object there.\n\nYes, thanks. Your other message cleared it up for me.\n\n> But this mail here suggests an alternative fix: instead of re-scanning\n> all packfiles like the patch proposes, wouldn't the proper fix be to not\n> ignore _all_ MIDX'd packs, but only the pack that _should_ have\n> contained the object?\n\nDo you mean in the main code path, or in the fallback?\n\nIn the main code path we definitely don't want to do this. Imagine we\nhave a midx that covers a million packs, and says object X is in pack P.\nA simultaneous writer deletes P and rewrites the midx, and the object is\nnow in a new pack Q (which might be covered by the new midx, but we\ndon't know because we're working with the stale one).\n\nWe definitely want to look in Q for the object after the midx can't find\nit. But we probably don't want to immediately search in the other\nmillion midx packs. Most objects won't have such a duplicate and the\nsearch is fruitless.\n\n-Peff\n"},{"id":"551111","messageId":"20260824070601.GC149254@coredump.intra.peff.net","threadId":"66192","inReplyTo":"20260824065539.GA149254@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-24T07:06:01Z","receivedAt":"2026-08-24T07:06:03Z","isPatch":true,"body":"On Mon, Aug 24, 2026 at 02:55:39AM -0400, Jeff King wrote:\n\n> Right. It would be OK to skip Elijah's fallback workaround when\n> SECOND_READ is not set; the QUICK callers are prepared to accept the\n> false negative. But since it is cheap-ish to do the fallback check, it\n> is perhaps OK to just do it on the first pass?\n> \n> I wonder how true that is. Imagine you had a midx covering a million\n> packs, and you notice an object is missing, but you're in QUICK mode. Do\n> you really want to individually check each of those million pack idx\n> files (that were otherwise not even opened or mmap'd because they're\n> covered by the midx!).\n> \n> I think it's mostly academic. You'd have to do the million-pack search\n> if we are not in QUICK mode. And the point of QUICK mode is mostly\n> avoiding tons of fruitless searches for objects we don't actually have.\n> The bsearch() conditional means that we _know_ this is a racy negative\n> and not just some object we never even had. So it would trigger\n> generally only when the search is useful.\n\nActually, thinking on this more: we _don't_ usually scan the million\npacks for an object we actually have. If the object is available in a\nnew pack, the SECOND_READ scan should find that pack and put it at the\nfront of the packfile list (because they sort by reverse mtime), and\nwe'd find the object immediately, without having to open the new packs.\n\nIt's only the case that this patch is helping (when the object is not\nmoved at all, but an existing duplicate is hidden in the midx) where we\nhave to re-scan all of those packs. But we don't know which case is\nwhich until we get to the SECOND_READ stage. So I think this probably\nshould only kick in for SECOND_READ.\n\n-Peff\n"},{"id":"551112","messageId":"20260824072322.GA155433@coredump.intra.peff.net","threadId":"66192","inReplyTo":"20260824070601.GC149254@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-24T07:23:22Z","receivedAt":"2026-08-24T07:23:24Z","isPatch":true,"body":"On Mon, Aug 24, 2026 at 03:06:01AM -0400, Jeff King wrote:\n\n> On Mon, Aug 24, 2026 at 02:55:39AM -0400, Jeff King wrote:\n> \n> > Right. It would be OK to skip Elijah's fallback workaround when\n> > SECOND_READ is not set; the QUICK callers are prepared to accept the\n> > false negative. But since it is cheap-ish to do the fallback check, it\n> > is perhaps OK to just do it on the first pass?\n> > \n> > I wonder how true that is. Imagine you had a midx covering a million\n> > packs, and you notice an object is missing, but you're in QUICK mode. Do\n> > you really want to individually check each of those million pack idx\n> > files (that were otherwise not even opened or mmap'd because they're\n> > covered by the midx!).\n> > \n> > I think it's mostly academic. You'd have to do the million-pack search\n> > if we are not in QUICK mode. And the point of QUICK mode is mostly\n> > avoiding tons of fruitless searches for objects we don't actually have.\n> > The bsearch() conditional means that we _know_ this is a racy negative\n> > and not just some object we never even had. So it would trigger\n> > generally only when the search is useful.\n> \n> Actually, thinking on this more: we _don't_ usually scan the million\n> packs for an object we actually have. If the object is available in a\n> new pack, the SECOND_READ scan should find that pack and put it at the\n> front of the packfile list (because they sort by reverse mtime), and\n> we'd find the object immediately, without having to open the new packs.\n\nEr, this final sentence should be \"without having to open the (million)\nold packs\".\n\n-Peff\n"},{"id":"551133","messageId":"ebaae70f-9e21-4673-b051-09e30420631e@gmail.com","threadId":"66192","inReplyTo":"5792c08f4ee0f9627ab1432d91299fe676e0a2f5.1787092446.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-08-24T14:45:36Z","receivedAt":"2026-08-24T14:45:40Z","isPatch":true,"body":"On 8/18/2026 6:34 PM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> When a geometric repack runs concurrently with other git processes, it\n> can write a new pack and multi-pack-index and then delete older packs\n> that the new one subsumes. One or more of those older packs may have\n> been indexed by the previous multi-pack-index. A process that already\n> had the previous multi-pack-index open keeps using it, and that stale\n> index still records the removed pack(s) as owning some objects.\n\nThis kind of race is why 'git multi-pack-index expire' exists, to\ndelete packfiles whose objects are all referenced within other\npackfiles. The inclusion of these \"stale\" packs in the multi-pack-index\nhelps halt reads of those packfiles by new processes while allowing\nthem to be read by existing processes.\n\nThis is currently used in the incremental repacks done by 'git\nmulti-pack-index repack' and maybe could be used again in this kind\nof geometric repack.\n\n(This dance is more important on Windows platforms where read handles\nprevent deletions, so it's common to have a foreground operation\nprevent a packfile deletion in background maintenance.)\n\nI do think your attempts to be more robust to missing packs is good,\nbut the comment thread does show that it's a complicated situation\nthat we may want to avoid whenever possible. Leaving some redundant\ndata around for some time interval can reduce the number of times\nthat the fallback logic is triggered.\n\nThanks,\n-Stolee\n\n"},{"id":"551134","messageId":"8d1d0729-7c61-4bbf-9cb2-1c2cbd81f143@gmail.com","threadId":"66192","inReplyTo":"5792c08f4ee0f9627ab1432d91299fe676e0a2f5.1787092446.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-08-24T14:46:22Z","receivedAt":"2026-08-24T14:46:24Z","isPatch":true,"body":"On 8/18/2026 6:34 PM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> When a geometric repack runs concurrently with other git processes, it\n> can write a new pack and multi-pack-index and then delete older packs\n> that the new one subsumes. One or more of those older packs may have\n> been indexed by the previous multi-pack-index. A process that already\n> had the previous multi-pack-index open keeps using it, and that stale\n> index still records the removed pack(s) as owning some objects.\n\nThis kind of race is why 'git multi-pack-index expire' exists, to\ndelete packfiles whose objects are all referenced within other\npackfiles. The inclusion of these \"stale\" packs in the multi-pack-index\nhelps halt reads of those packfiles by new processes while allowing\nthem to be read by existing processes.\n\nThis is currently used in the incremental repacks done by 'git\nmulti-pack-index repack' and maybe could be used again in this kind\nof geometric repack.\n\n(This dance is more important on Windows platforms where read handles\nprevent deletions, so it's common to have a foreground operation\nprevent a packfile deletion in background maintenance.)\n\nI do think your attempts to be more robust to missing packs is good,\nbut the comment thread does show that it's a complicated situation\nthat we may want to avoid whenever possible. Leaving some redundant\ndata around for some time interval can reduce the number of times\nthat the fallback logic is triggered.\n\nThanks,\n-Stolee\n\n"},{"id":"551166","messageId":"CABPp-BHxEJ31fxt-hD6XrVfUwPuw+f3zMmhfX86hyCSakj-YbA@mail.gmail.com","threadId":"66192","inReplyTo":"20260824045529.GB142844@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-08-25T07:19:29Z","receivedAt":"2026-08-25T07:19:41Z","isPatch":true,"body":"On Sun, Aug 23, 2026 at 9:55 PM Jeff King wrote:\n>\n> On Tue, Aug 18, 2026 at 10:34:06PM +0000, Elijah Newren via GitGitGadget wrote:\n>\n> > Teach find_pack_entry() to recover. After the normal multi-pack-index\n> > lookup and the regular pack fallback both miss, check whether the object\n> > is nonetheless present in a covered multi-pack-index (bsearch_midx()).\n> > If it is, its recorded owner must have become unavailable, so scan that\n> > index's packs directly for a surviving copy. The bsearch gate keeps\n> > genuine misses (i.e. objects absent from the index) on the fast path, and\n> > because the recovery lives in find_pack_entry() itself it also fixes the\n> > OBJECT_INFO_QUICK callers that never reprepare.\n>\n> You don't even have to pay the bsearch() again. We'd already have looked\n> in the midx earlier in the function. We just need to distinguish three\n> cases:\n>\n> 1. it was not in the midx (or there is no midx)\n>\n> 2. it was in the midx but we could not load it (pack invalid, or\n> object in the bad_objects list)\n>\n> 3. it was in the midx and is available\n>\n> In fill_midx_entry() we return a boolean that lumps cases 1+2 together,\n> versus case 3. It could return a tri-state that would let us distinguish\n> all three. And then your fallback would kick in only for case 2 (case 3\n> already returned with success, and case 1 means the midx does not even\n> mention the object).\n\nYou know, I considered putting the logic in fill_midx_entry() as well\nas putting where it is. You'd think based on that, that I'd have\nthought about just changing fill_midx_entry()'s return type to get the\nbest of both worlds. You'd be wrong though. ;-)\n\nThis sounds much nicer; I adopted it and made fill_midx_entry() return\nMIDX_FILL_MISS / MIDX_FILL_HIT / MIDX_FILL_OWNER_UNAVAILABLE in v2.\n"},{"id":"551167","messageId":"CABPp-BEkqKgJ3WLt323ntLp07n8dojhEoLu80fSmvz2D1ci0bw@mail.gmail.com","threadId":"66192","inReplyTo":"20260824070601.GC149254@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-08-25T07:38:02Z","receivedAt":"2026-08-25T07:38:15Z","isPatch":true,"body":"On Mon, Aug 24, 2026 at 12:06 AM Jeff King wrote:\n>\n> It's only the case that this patch is helping (when the object is not\n> moved at all, but an existing duplicate is hidden in the midx) where we\n> have to re-scan all of those packs. But we don't know which case is\n> which until we get to the SECOND_READ stage. So I think this probably\n> should only kick in for SECOND_READ.\n\nI implemented that in v2.\n"},{"id":"551168","messageId":"CABPp-BHz2EsFvqpcAAiHSa7Lu28pkoai9GLR_ts=b1098d03vg@mail.gmail.com","threadId":"66192","inReplyTo":"ebaae70f-9e21-4673-b051-09e30420631e@gmail.com","subject":"Re: [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-08-25T07:38:12Z","receivedAt":"2026-08-25T07:38:24Z","isPatch":true,"body":"On Mon, Aug 24, 2026 at 7:45 AM Derrick Stolee wrote:\n>\n> On 8/18/2026 6:34 PM, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > When a geometric repack runs concurrently with other git processes, it\n> > can write a new pack and multi-pack-index and then delete older packs\n> > that the new one subsumes. One or more of those older packs may have\n> > been indexed by the previous multi-pack-index. A process that already\n> > had the previous multi-pack-index open keeps using it, and that stale\n> > index still records the removed pack(s) as owning some objects.\n>\n> This kind of race is why 'git multi-pack-index expire' exists, to\n> delete packfiles whose objects are all referenced within other\n> packfiles. The inclusion of these \"stale\" packs in the multi-pack-index\n> helps halt reads of those packfiles by new processes while allowing\n> them to be read by existing processes.\n>\n> This is currently used in the incremental repacks done by 'git\n> multi-pack-index repack' and maybe could be used again in this kind\n> of geometric repack.\n>\n> (This dance is more important on Windows platforms where read handles\n> prevent deletions, so it's common to have a foreground operation\n> prevent a packfile deletion in background maintenance.)\n>\n> I do think your attempts to be more robust to missing packs is good,\n> but the comment thread does show that it's a complicated situation\n> that we may want to avoid whenever possible. Leaving some redundant\n> data around for some time interval can reduce the number of times\n> that the fallback logic is triggered.\n\nOh, good pointer. It may make sense to teach geometric repacking\nabout \"git multi-pack-index expire\", which I think would be\ncomplementary and reduce how often we fall into recovery, while the\nchanges in this patch help keep us correct when we do fall into\nrecovery.\n"},{"id":"551225","messageId":"pull.2207.v2.git.1787684429.gitgitgadget@gmail.com","threadId":"66192","inReplyTo":"pull.2207.git.1787092446.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] Objects treated as missing despite being present, due to race with geometric repacking","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-25T19:00:25Z","receivedAt":"2026-08-25T19:00:32Z","isPatch":true,"body":"Changes since v1:\n\n * Rebased on top of ps/odb-generic-corrupt-objects, and conflicts with it\n resolved\n * Removed useless test_grep line spotted by Junio in PATCH 1\n * Switched fill_midx_entry() to a tri-state to avoid duplicate\n bsearch_midx(), as suggested by Peff\n * Only do the re-read on SECOND_READ, as suggested by Peff\n * Handle multiple objects shared across multiple packs correctly (issue\n caught & corrected & new testcase by deeper AI review)\n * Inserted two new patches:\n * 2/4: Fix a leak in git mktree --batch since I use it in new testcases\n and don't want the *-leaks jobs failing\n * 3/4: Demonstrate and fix QUICK reader problems, while keeping expected\n QUICK performance for normal cases (we've already been discussing this\n patch in this thread a bunch anyway, and it's logically related)\n\nCover letter addendum/update:\n\nA geometric repack writes a new pack plus multi-pack-index and then deletes\nthe packs the new one subsumes. Readers running alongside it can be told an\nobject is missing when it is in fact still present. The v1 series fixed one\nrace of this shape (the object didn't move and was in a second pack\nreferenced by the multi-pack-index); v2 added a new patch fixing others in\nthe same class but of a different shape (the object moved to a brand new\npack).\n\nNote here that Stolee's suggestion to defer pack deletion via git\nmulti-pack-index expire seems like a good complementary mitigation; it would\nreduce how often we fall into recovery, while this series tries to fix\nrecovery to work more robustly.\n\nOriginal cover letter (focused on the final patch):\n\nWhen an object is found in multiple packs that are in a multi-pack-index,\nand a subsequent geometric repacking creates a new multi-pack-index and\nremoves the pack that was considered the owner of the object in the old\nmulti-pack-index, then an already-running process that had opened the old\nmulti-pack-index and hadn't yet opened the removed packfile will not be able\nto access the object -- lookups will return it as missing. Additionally,\nreplay has a separate bug where a missing object causes a SIGSEGV rather\nthan an error message.\n\nThis appears to affect a very small percentage of git operations in\nproduction since it is a tiny window, but I've found evidence of it\noccurring in at least eight distinct server-side operations, covering seven\ndifferent git commands:\n\ngit operation symptom\n----------------------------------- -----------------------------\ngit replay (server-side rebase) SIGSEGV (this series, 1/2)\ngit merge-tree spurious read-miss failure\ngit diff (raw and tree-vs-tree) spurious read-miss failure\ngit rev-list --count spurious read-miss failure\ngit merge-base spurious read-miss failure\nobject/rev resolution (rev-parse, spurious read-miss failure\n cat-file)\nrepository repair (fsck/repack) spurious read-miss failure\n\n\nThere are also commands that could be changing behavior without throwing an\nerror -- e.g. object negotiation thinking an object doesn't exist and\ninstead negotiating based on an older common commit, or cat-file --batch\nreporting that some objects don't exist.\n\nThis series fixes the replay bug first, since it's simpler; investigating\nit, together with my other recent repacking work, is what led me to the\nunderlying multi-pack-index issue that 2/2 addresses.\n\nElijah Newren (4):\n replay: fail gracefully when a merge input is unreadable\n mktree: plug per-tree leak in --batch mode\n packfile: recover object lookups racing a concurrent repack\n packfile: recover when a multi-pack-index names a removed pack\n\n builtin/mktree.c | 3 +\n builtin/pack-objects.c | 2 +-\n midx.c | 44 ++++++----\n midx.h | 21 ++++-\n odb.c | 8 +-\n odb.h | 16 +++-\n odb/source-packed.c | 51 ++++++++++--\n packfile.c | 39 ++++++++-\n replay.c | 7 ++\n t/meson.build | 1 +\n t/t3650-replay-basics.sh | 34 ++++++++\n t/t5319-multi-pack-index.sh | 80 ++++++++++++++++++\n t/t5336-repack-reader-race.sh | 148 ++++++++++++++++++++++++++++++++++\n 13 files changed, 423 insertions(+), 31 deletions(-)\n create mode 100755 t/t5336-repack-reader-race.sh\n\n\nbase-commit: 2135b14863642bbcec02996e7f5e54ac1f77b03a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2207%2Fnewren%2Fmidx-removed-pack-recovery-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2207/newren/midx-removed-pack-recovery-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/2207\n\nRange-diff vs v1:\n\n 1: 321af575e0 ! 1: 36bf2ce17b replay: fail gracefully when a merge input is unreadable\n @@ t/t3650-replay-basics.sh: test_expect_success '--onto with --ref rejects multipl\n +\n +\t\t# Ensure replay gracefully handles the missing object\n +\t\ttest_must_fail git replay --onto onto base..side 2>err &&\n -+\t\ttest_grep ! \"[Ss]egmentation\" err &&\n -+\t\ttest_grep \"Could not read\\|collecting merge info failed\" err\n ++\t\ttest_grep -e \"Could not read\" -e \"collecting merge info failed\" err\n +\t)\n +'\n +\n -: ---------- > 2: 3f3b75690e mktree: plug per-tree leak in --batch mode\n -: ---------- > 3: fc98f48ddb packfile: recover object lookups racing a concurrent repack\n 2: 5792c08f4e ! 4: eacf6ba4b1 packfile: recover when a multi-pack-index names a removed pack\n @@ Metadata\n ## Commit message ##\n packfile: recover when a multi-pack-index names a removed pack\n \n - When a geometric repack runs concurrently with other git processes, it\n - can write a new pack and multi-pack-index and then delete older packs\n - that the new one subsumes. One or more of those older packs may have\n - been indexed by the previous multi-pack-index. A process that already\n - had the previous multi-pack-index open keeps using it, and that stale\n - index still records the removed pack(s) as owning some objects.\n + A geometric repack writes a new pack and multi-pack-index and then\n + deletes the packs the new one subsumes. A process still using the\n + previous MIDX keeps seeing a removed pack listed as the owner of some\n + objects. Since a MIDX attributes each object to exactly one pack, such\n + an object is served only through its recorded owner; if that owner was\n + just removed, find_pack_entry() cannot serve it -- fill_midx_entry()\n + routes to the missing pack, and the regular pack fallback deliberately\n + skips every MIDX-covered pack, so a surviving copy in another covered\n + pack (e.g. a kept base pack) is never consulted.\n \n - Because a multi-pack-index attributes each object to exactly one pack,\n - an object that exists in multiple covered packs is served only through\n - its recorded owner. If that owner is the pack a concurrent repack just\n - removed, find_pack_entry() cannot serve the object: fill_midx_entry()\n - routes the lookup to the missing pack (prepare_midx_pack() fails), and\n - the regular pack fallback deliberately skips every multi-pack-index\n - covered pack. The object is reported missing even though a perfectly\n - good copy survives in another covered pack -- for example a large \"base\"\n - pack that geometric repacking intentionally kept.\n + Unlike the ordinary \"a pack's .idx is mapped but its .pack is gone\"\n + race, the second read does not rescue us -- and not only for\n + OBJECT_INFO_QUICK callers. Reloading the on-disk pack set does not\n + reload the borrowed, cached MIDX (freeing it under the code that caches\n + the \"struct multi_pack_index *\" would be a use-after-free), so the stale\n + MIDX keeps routing to the removed pack and the surviving copy stays\n + hidden behind the covered-pack skip. cat-file, rev-list and pack-objects\n + can thus all spuriously fail with \"unable to read object\".\n \n - The false negative is not limited to one caller. Any reader\n - (cat-file, rev-list, pack-objects, ...) can spuriously fail with\n - \"unable to read object\", and callers that only ask whether an object\n - exists get a wrong answer too, since the OBJECT_INFO_QUICK path never\n - retries. Writers that merge in-core, such as \"git replay\", are hit\n - hardest: merge-ort treats the unreadable tree as a premature abort, sets\n - result.clean < 0, and returns without a result tree.\n + Teach find_pack_entry() to recover. fill_midx_entry() now returns a\n + tri-state, distinguishing \"absent from the MIDX\" from \"present but the\n + owning pack is unavailable\"; in the latter case, once the regular\n + fallback has also missed, scan the MIDX's packs directly for a surviving\n + copy.\n \n - Teach find_pack_entry() to recover. After the normal multi-pack-index\n - lookup and the regular pack fallback both miss, check whether the object\n - is nonetheless present in a covered multi-pack-index (bsearch_midx()).\n - If it is, its recorded owner must have become unavailable, so scan that\n - index's packs directly for a surviving copy. The bsearch gate keeps\n - genuine misses (i.e. objects absent from the index) on the fast path, and\n - because the recovery lives in find_pack_entry() itself it also fixes the\n - OBJECT_INFO_QUICK callers that never reprepare.\n + Do the scan only on the second read (OBJECT_INFO_SECOND_READ): by then\n + the cheaper on-disk reload has run, so an object merely relocated into a\n + new (non-covered) pack has already been found by the regular fallback,\n + and only a genuine hidden duplicate reaches the rescan. QUICK callers\n + that would skip the second read are steered into it by the preceding\n + commit's stale_packs_detected flag, which prepare_midx_pack() sets when\n + it cannot open the owning pack.\n \n - This recovers the object without touching the multi-pack-index itself.\n - Reloading the stale index would be a more complete fix but would be much\n - more involved: other code (pack bitmaps, object name disambiguation)\n - borrows and caches the \"struct multi_pack_index *\" across object reads,\n - so freeing it underneath them would be a use-after-free. Refreshing the\n - index with proper invalidation of those borrowers is left for future\n - work.\n + Reloading the stale MIDX would be a more complete fix but is much more\n + involved (the borrowers above need proper invalidation), so leave that\n + for later.\n \n + Assisted-by: Claude Opus 4.8 & GPT-5.6 Sol\n + Helped-by: Jeff King \n Signed-off-by: Elijah Newren \n \n + ## builtin/pack-objects.c ##\n +@@ builtin/pack-objects.c: static int want_object_in_pack_mtime(const struct object_id *oid,\n + \t\tstruct multi_pack_index *m = get_multi_pack_index(files->packed);\n + \t\tstruct pack_entry e;\n + \n +-\t\tif (m && fill_midx_entry(m, oid, &e, NULL)) {\n ++\t\tif (m && fill_midx_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {\n + \t\t\twant = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);\n + \t\t\tif (want != -1)\n + \t\t\t\treturn want;\n +\n + ## midx.c ##\n +@@ midx.c: uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos)\n + \t\t\t\t\t (off_t)pos * MIDX_CHUNK_OFFSET_WIDTH);\n + }\n + \n +-int fill_midx_entry(struct multi_pack_index *m,\n +-\t\t const struct object_id *oid,\n +-\t\t struct pack_entry *e,\n +-\t\t struct packed_git **bad_pack)\n ++enum midx_fill_result fill_midx_entry(struct multi_pack_index *m,\n ++\t\t\t\t const struct object_id *oid,\n ++\t\t\t\t struct pack_entry *e,\n ++\t\t\t\t struct packed_git **bad_pack)\n + {\n + \tuint32_t pos;\n + \tuint32_t pack_int_id;\n + \tstruct packed_git *p;\n + \n + \tif (!bsearch_midx(oid, m, &pos))\n +-\t\treturn 0;\n ++\t\treturn MIDX_FILL_MISS;\n + \n + \tmidx_for_object(&m, pos);\n + \tpack_int_id = nth_midxed_pack_int_id(m, pos);\n + \n + \tif (prepare_midx_pack(m, pack_int_id))\n +-\t\treturn 0;\n ++\t\tgoto owner_unavailable;\n + \tp = m->packs[pack_int_id - m->num_packs_in_base];\n + \n +-\t/*\n +-\t* We are about to tell the caller where they can locate the\n +-\t* requested object. We better make sure the packfile is\n +-\t* still here and can be accessed before supplying that\n +-\t* answer, as it may have been deleted since the MIDX was\n +-\t* loaded!\n +-\t*/\n ++\t/* Make sure the pack is still present before pointing at it. */\n + \tif (!is_pack_valid(p))\n +-\t\treturn 0;\n ++\t\tgoto owner_unavailable;\n + \n + \tif (oidset_size(&p->bad_objects) &&\n + \t oidset_contains(&p->bad_objects, oid)) {\n + \t\tif (bad_pack && !*bad_pack)\n + \t\t\t*bad_pack = p;\n +-\t\treturn 0;\n ++\t\treturn MIDX_FILL_MISS;\n + \t}\n + \n + \te->offset = nth_midxed_offset(m, pos);\n + \te->p = p;\n + \n +-\treturn 1;\n ++\treturn MIDX_FILL_HIT;\n ++\n ++owner_unavailable:\n ++\t/*\n ++\t * Re-arm stale_packs_detected on every such lookup, not just the\n ++\t * first: prepare_midx_pack() caches the failure, so without this a\n ++\t * later lookup of the same vanished pack would leave the flag clear\n ++\t * and a QUICK reader would skip its recovering second read.\n ++\t */\n ++\tm->source->base.odb->stale_packs_detected = 1;\n ++\treturn MIDX_FILL_OWNER_UNAVAILABLE;\n + }\n + \n + /* Match \"foo.idx\" against either \"foo.pack\" _or_ \"foo.idx\". */\n +@@ midx.c: int verify_midx_file(struct odb_source_packed *source, unsigned flags)\n + \n + \t\tnth_midxed_object_oid(&oid, m, pairs[i].pos);\n + \n +-\t\tif (!fill_midx_entry(m, &oid, &e, NULL)) {\n ++\t\tif (fill_midx_entry(m, &oid, &e, NULL) != MIDX_FILL_HIT) {\n + \t\t\tmidx_report(_(\"failed to load pack entry for oid[%d] = %s\"),\n + \t\t\t\t pairs[i].pos, oid_to_hex(&oid));\n + \t\t\tcontinue;\n +\n + ## midx.h ##\n +@@ midx.h: uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos);\n + struct object_id *nth_midxed_object_oid(struct object_id *oid,\n + \t\t\t\t\tstruct multi_pack_index *m,\n + \t\t\t\t\tuint32_t n);\n +-int fill_midx_entry(struct multi_pack_index *m, const struct object_id *oid,\n +-\t\t struct pack_entry *e, struct packed_git **bad_pack);\n ++/*\n ++ * Result of looking an object up in a multi-pack-index. MIDX_FILL_HIT means\n ++ * \"e was filled in\"; the two miss variants distinguish an object the midx does\n ++ * not know about (MIDX_FILL_MISS) from one it does know about but whose owning\n ++ * pack we can no longer open (MIDX_FILL_OWNER_UNAVAILABLE -- the signature of a\n ++ * concurrent repack having removed that pack). A known-bad (corrupt) object\n ++ * reports MIDX_FILL_MISS but also sets *bad_pack, if provided, to the owning\n ++ * pack so the caller can tell \"corrupt\" apart from \"absent\".\n ++ */\n ++enum midx_fill_result {\n ++\tMIDX_FILL_MISS = 0,\n ++\tMIDX_FILL_HIT,\n ++\tMIDX_FILL_OWNER_UNAVAILABLE,\n ++};\n ++\n ++enum midx_fill_result fill_midx_entry(struct multi_pack_index *m,\n ++\t\t\t\t const struct object_id *oid,\n ++\t\t\t\t struct pack_entry *e,\n ++\t\t\t\t struct packed_git **bad_pack);\n + int midx_contains_pack(struct multi_pack_index *m,\n + \t\t const char *idx_or_pack_name);\n + int midx_layer_contains_pack(struct multi_pack_index *m,\n +\n ## odb/source-packed.c ##\n +@@\n + static int find_pack_entry(struct odb_source_packed *store,\n + \t\t\t const struct object_id *oid,\n + \t\t\t struct pack_entry *e,\n ++\t\t\t enum object_info_flags flags,\n + \t\t\t struct packed_git **bad_pack)\n + {\n + \tstruct packfile_list_entry *l;\n ++\tenum midx_fill_result midx_result = MIDX_FILL_MISS;\n + \n + \todb_source_prepare(&store->base, 0);\n +-\tif (store->midx && fill_midx_entry(store->midx, oid, e, bad_pack))\n +-\t\treturn 1;\n ++\tif (store->midx) {\n ++\t\tmidx_result = fill_midx_entry(store->midx, oid, e, bad_pack);\n ++\t\tif (midx_result == MIDX_FILL_HIT)\n ++\t\t\treturn 1;\n ++\t}\n + \n + \tfor (l = store->packs.head; l; l = l->next) {\n + \t\tstruct packed_git *p = l->pack;\n @@ odb/source-packed.c: static int find_pack_entry(struct odb_source_packed *store,\n \t\t}\n \t}\n \n +\t/*\n -+\t * Recovery for a concurrent-repack race: a MIDX can name an owning\n -+\t * pack for an object that a simultaneous repack has since deleted,\n -+\t * even though the object still exists in another pack the same MIDX\n -+\t * covers (e.g. a kept base pack that geometric repack did not rewrite).\n -+\t * If the object is present in a MIDX yet none of the paths above could\n -+\t * serve it, its recorded owning pack has become unavailable. The\n -+\t * regular fallback above deliberately skips MIDX-covered packs, so\n -+\t * scan this MIDX's packs directly to find the surviving copy. The\n -+\t * bsearch gate keeps genuine misses (objects absent from the MIDX) on\n -+\t * the fast path.\n ++\t * Recovery for a concurrent-repack race: a stale MIDX may still name a\n ++\t * vanished owning pack even though the object survives in another pack\n ++\t * the same MIDX covers. The regular fallback above skips MIDX-covered\n ++\t * packs, and repreparing the on-disk pack set does not reload the\n ++\t * borrowed, cached MIDX, so scan its packs directly for the survivor.\n ++\t *\n ++\t * Do this only on the second read, by which point repreparing packs has\n ++\t * already had a chance to find an object merely relocated into a new,\n ++\t * uncovered pack; only a genuine hidden duplicate reaches here.\n +\t */\n -+\tif (store->midx) {\n ++\tif (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&\n ++\t (flags & OBJECT_INFO_SECOND_READ)) {\n +\t\tstruct multi_pack_index *m = store->midx;\n -+\t\tuint32_t midx_pos, i;\n -+\n -+\t\tif (bsearch_midx(oid, m, &midx_pos)) {\n -+\t\t\tfor (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {\n -+\t\t\t\tstruct packed_git *p;\n -+\n -+\t\t\t\tif (prepare_midx_pack(m, i))\n -+\t\t\t\t\tcontinue;\n -+\t\t\t\tp = nth_midxed_pack(m, i);\n -+\t\t\t\tif (p && packfile_fill_entry(p, oid, e))\n -+\t\t\t\t\treturn 1;\n -+\t\t\t}\n ++\t\tuint32_t i;\n ++\n ++\t\tfor (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {\n ++\t\t\tstruct packed_git *p;\n ++\n ++\t\t\tif (prepare_midx_pack(m, i))\n ++\t\t\t\tcontinue;\n ++\t\t\tp = nth_midxed_pack(m, i);\n ++\t\t\tif (p && packfile_fill_entry(p, oid, e, bad_pack))\n ++\t\t\t\treturn 1;\n +\t\t}\n +\t}\n +\n \treturn 0;\n }\n \n +@@ odb/source-packed.c: static enum odb_read_status odb_source_packed_read_object_info(struct odb_source\n + \tif (flags & OBJECT_INFO_SECOND_READ)\n + \t\todb_source_prepare(source, ODB_PREPARE_FLUSH_CACHES);\n + \n +-\tif (!find_pack_entry(packed, oid, &e, &bad_pack)) {\n ++\tif (!find_pack_entry(packed, oid, &e, flags, &bad_pack)) {\n + \t\t/*\n + \t\t * The lookup may have failed because the object is known to be\n + \t\t * corrupt in one of the packfiles. Report the object as\n +@@ odb/source-packed.c: static int odb_source_packed_read_object_stream(struct odb_read_stream **out,\n + \tstruct odb_source_packed *packed = odb_source_packed_downcast(source);\n + \tstruct pack_entry e;\n + \n +-\tif (!find_pack_entry(packed, oid, &e, NULL))\n ++\tif (!find_pack_entry(packed, oid, &e, 0, NULL))\n + \t\treturn -1;\n + \n + \treturn packfile_read_object_stream(out, oid, e.p, e.offset);\n +@@ odb/source-packed.c: static int odb_source_packed_freshen_object(struct odb_source *source,\n + \t\ttimesp = ×\n + \t}\n + \n +-\tif (!find_pack_entry(packed, oid, &e, NULL))\n ++\tif (!find_pack_entry(packed, oid, &e, 0, NULL))\n + \t\treturn 0;\n + \tif (e.p->is_cruft)\n + \t\treturn 0;\n \n ## t/t5319-multi-pack-index.sh ##\n @@ t/t5319-multi-pack-index.sh: test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '\n @@ t/t5319-multi-pack-index.sh: test_expect_success 'pack.preferBitmapTips interpre\n +\t\ttest_cmp expect actual\n +\t)\n +'\n ++\n ++test_expect_success 'repeated QUICK lookups recover after owning pack removed' '\n ++\ttest_when_finished \"rm -fr repo\" &&\n ++\tgit init repo &&\n ++\t(\n ++\t\tcd repo &&\n ++\n ++\t\t# Two blobs, each duplicated across packs so the midx must pick\n ++\t\t# an owning pack, and each attributed to the same moderate pack.\n ++\t\techo one >f1 &&\n ++\t\techo two >f2 &&\n ++\t\tgit add f1 f2 &&\n ++\t\tgit commit -m dups &&\n ++\t\td1=$(git rev-parse HEAD:f1) &&\n ++\t\td2=$(git rev-parse HEAD:f2) &&\n ++\n ++\t\t# Roll every object, including d1 and d2, into one big pack,\n ++\t\t# then build a moderate pack that also holds both blobs.\n ++\t\tgit repack -adq &&\n ++\t\tmoderate=$(printf \"%s\\n%s\\n\" \"$d1\" \"$d2\" |\n ++\t\t\tgit pack-objects --quiet $objdir/pack/pack) &&\n ++\n ++\t\tgit multi-pack-index write \\\n ++\t\t\t--preferred-pack=\"pack-$moderate.idx\" &&\n ++\n ++\t\t# Retire the moderate pack; the stale midx still names it as the\n ++\t\t# owner of both blobs, each of which survives in the big pack.\n ++\t\trm -f $objdir/pack/pack-$moderate.* &&\n ++\n ++\t\t# One resident QUICK reader (\"git mktree --batch\") resolves both\n ++\t\t# blobs. The first lookup recovers d1 and caches the owning\n ++\t\t# packs failure; unless that failure keeps re-arming the second\n ++\t\t# read, the lookup of d2 skips its recovering read and the reader\n ++\t\t# dies reporting d2 as missing.\n ++\t\tprintf \"100644 blob %s\\tf1\\n\\n100644 blob %s\\tf2\\n\\n\" \\\n ++\t\t\t\"$d1\" \"$d2\" |\n ++\t\t\tgit mktree --batch >trees &&\n ++\t\ttest_line_count = 2 trees\n ++\t)\n ++'\n +\n test_done\n\n-- \ngitgitgadget\n"},{"id":"551226","messageId":"36bf2ce17be1a4da1ba92d5eb89ce49c7e00be9d.1787684429.git.gitgitgadget@gmail.com","threadId":"66192","inReplyTo":"pull.2207.v2.git.1787684429.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] replay: fail gracefully when a merge input is unreadable","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-25T19:00:26Z","receivedAt":"2026-08-25T19:00:33Z","isPatch":true,"body":"From: Elijah Newren \n\nWhen objects involved in the merge cannot be read, the merge machinery\nwill return early with result.clean = -1, and result.tree left as NULL.\npick_regular_commit() tested only \"if (!result->clean)\", ignoring the\ncase where \"clean < 0\". That causes the code to try to use\nresult->tree, resulting in a SIGSEGV.\n\nHandle clean < 0 explicitly; the merge machinery will already have printed\nmessages such as \"Could not read \" and \"collecting merge info\nfailed for trees...\", so we don't need to add much detail beyond the\nfact that the merge failed.\n\nSigned-off-by: Elijah Newren \n---\n replay.c | 7 +++++++\n t/t3650-replay-basics.sh | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 41 insertions(+)\n\ndiff --git a/replay.c b/replay.c\nindex 463c900d6c..33e21b2032 100644\n--- a/replay.c\n+++ b/replay.c\n@@ -327,6 +327,13 @@ static struct commit *pick_regular_commit(struct repository *repo,\n \tmerge_opt->ancestor = NULL;\n \tmerge_opt->branch2 = NULL;\n \n+\tif (result->clean < 0) {\n+\t\terror(_(\"merge of %s onto %s failed\"),\n+\t\t oid_to_hex(&pickme->object.oid),\n+\t\t oid_to_hex(&replayed_base->object.oid));\n+\t\treturn NULL;\n+\t}\n+\n \tif (!result->clean)\n \t\treturn NULL;\n \ndiff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh\nindex 3353bc4a4d..12348b4a5f 100755\n--- a/t/t3650-replay-basics.sh\n+++ b/t/t3650-replay-basics.sh\n@@ -565,4 +565,38 @@ test_expect_success '--onto with --ref rejects multiple revision ranges' '\n \ttest_grep \"cannot be used with multiple revision ranges\" err\n '\n \n+test_expect_success 'replay fails without segfault when objects are missing' '\n+\ttest_when_finished \"rm -fr unreadable\" &&\n+\tgit init unreadable &&\n+\t(\n+\t\tcd unreadable &&\n+\n+\t\ttest_write_lines l1 l2 l3 l4 l5 l6 l7 l8 >f &&\n+\t\tgit add f &&\n+\t\tgit commit -m base &&\n+\t\tgit branch base &&\n+\n+\t\ttest_write_lines l1 l2 l3 l4 l5 l6 l7 CHANGED >f &&\n+\t\tgit commit -am side &&\n+\t\tgit branch side &&\n+\n+\t\tgit switch -c onto base &&\n+\t\ttest_write_lines CHANGED l2 l3 l4 l5 l6 l7 l8 >f &&\n+\t\tgit commit -am onto &&\n+\n+\t\t# The replay works while every object is readable.\n+\t\tgit replay --onto onto base..side &&\n+\n+\t\t# Removing the onto tree makes parse_tree() fail during the\n+\t\t# incore merge, driving clean < 0 with a NULL result tree.\n+\t\tonto_tree=$(git rev-parse onto^{tree}) &&\n+\t\tobj=$(test_oid_to_path \"$onto_tree\") &&\n+\t\tmv .git/objects/${obj} saved-tree &&\n+\n+\t\t# Ensure replay gracefully handles the missing object\n+\t\ttest_must_fail git replay --onto onto base..side 2>err &&\n+\t\ttest_grep -e \"Could not read\" -e \"collecting merge info failed\" err\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"551227","messageId":"3f3b75690eea02960c7edc8d318ce7dff654f1bc.1787684429.git.gitgitgadget@gmail.com","threadId":"66192","inReplyTo":"pull.2207.v2.git.1787684429.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] mktree: plug per-tree leak in --batch mode","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-25T19:00:27Z","receivedAt":"2026-08-25T19:00:34Z","isPatch":true,"body":"From: Elijah Newren \n\nIn --batch mode \"git mktree\" reuses its entry buffer across trees,\nresetting `used` to 0 after writing each tree. It never frees the\n`treeent` structures the previous tree appended, though, so once the\nnext tree overwrites those slots the earlier allocations are leaked. A\nsingle-tree invocation hides this, as the entries stay reachable through\nthe `entries` global until exit.\n\nFree each entry when resetting the buffer, and free the buffer itself\nbefore returning.\n\nSigned-off-by: Elijah Newren \n---\n builtin/mktree.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/builtin/mktree.c b/builtin/mktree.c\nindex 4084e32476..dc2d293c3d 100644\n--- a/builtin/mktree.c\n+++ b/builtin/mktree.c\n@@ -200,8 +200,11 @@ int cmd_mktree(int ac,\n \t\t\tputs(oid_to_hex(&oid));\n \t\t\tfflush(stdout);\n \t\t}\n+\t\tfor (int i = 0; i < used; i++)\n+\t\t\tfree(entries[i]);\n \t\tused=0; /* reset tree entry buffer for re-use in batch mode */\n \t}\n+\tfree(entries);\n \tstrbuf_release(&sb);\n \n \treturn 0;\n-- \ngitgitgadget\n\n"},{"id":"551228","messageId":"fc98f48ddb4d46cad66a40ecdd96c139e1397784.1787684429.git.gitgitgadget@gmail.com","threadId":"66192","inReplyTo":"pull.2207.v2.git.1787684429.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] packfile: recover object lookups racing a concurrent repack","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-25T19:00:28Z","receivedAt":"2026-08-25T19:00:35Z","isPatch":true,"body":"From: Elijah Newren \n\nWhen a reader opens a pack it discovered on disk, open_packed_git_1()\nfirst mmaps the pack's `.idx`. A `git repack` running alongside us\nconsolidates existing packs into a new one and then removes the\nredundant packs, deleting each pack's `.idx` before its `.pack` (see the\nordering in unlink_pack_path()). A reader that had just enumerated one\nof those packs -- most easily through a multi-pack-index -- can race with\nthe removal and find the pack gone.\n\nTwo things go wrong in that window:\n\n 1. open_pack_index() fails, so we print\n\n error: packfile index unavailable\n\n and report the pack as unusable, even though the object still lives\n in the replacement pack.\n\n 2. A normal lookup recovers: odb_read_object_info_extended() issues a\n second read that reloads the on-disk pack state and finds the object\n in its new home, making the message above mere noise. But an\n OBJECT_INFO_QUICK lookup deliberately skips that second read to stay\n fast on a genuine miss, so it does *not* recover: it reports the\n object as absent even though it still lives in the replacement pack.\n A resident reader that resolves objects with a QUICK lookup -- such\n as the `git mktree --batch` process the tests below drive -- then\n produces wrong results. Even where a spurious miss is not fatal it\n is not harmless: `git upload-pack` checks a client's \"have\" lines\n with a QUICK lookup, and a dropped \"have\" removes a common object\n from the negotiation, so the client is sent more than it needs.\n\nRecovering without giving up that speed is the trick: we keep QUICK's\nfast path for a genuine miss and force the extra read only when a pack\nwe were already using has provably vanished.\n\nFix both. Record that a pack disappeared out from under us by setting\nobject_database.stale_packs_detected at the three points where a reader\ncan notice a pack vanish beneath it:\n\n - In open_packed_git_1(), when open_pack_index() fails because the\n index simply vanished (its open fails with ENOENT). Here we also\n stay silent instead of printing \"index unavailable\"; a genuinely\n unreadable index that is still present keeps the error, since that is\n a real problem worth surfacing.\n\n - In open_packed_git_1() again, from the other side of the race: when\n the `.idx` was already mapped -- so open_pack_index() returns without\n touching the filesystem -- yet opening the `.pack` fails with ENOENT.\n A reader that prepared its pack list before the repack only trips\n over the removal when it finally opens the pack file.\n\n - In prepare_midx_pack(), when packfile_store_load_pack() cannot open a\n pack the midx still references at all. If both the `.idx` and the\n `.pack` are already gone -- as happens when the redundant pack is\n removed outright rather than index-first -- we never reach\n open_pack_index(), so this is the only place the vanished pack is\n observed.\n\nThen, in odb_read_object_info_extended(), issue the second read -- which\nasks the sources to reload their on-disk state (for packs, a reprepare)\nand retry -- not only for non-QUICK lookups but also whenever\nstale_packs_detected is set, even under OBJECT_INFO_QUICK. An ordinary\nQUICK miss, with no vanished pack, still skips the second read and stays\nfast; we pay for the rescan only when we have positive evidence that the\non-disk pack set changed beneath us. The flag is reset when the\npackfiles are reprepared, in odb_source_packed_prepare().\n\nAdd t5336, regression tests that reproduce the race deterministically:\nthey drive a resident `git mktree --batch` reader -- which resolves each\ntree entry with OBJECT_INFO_QUICK -- across both removal windows, one\nremoving a pack's `.idx` first while a midx routes the lookup to the\ndoomed pack, the other removing a pack's `.pack` after its `.idx` was\nalready mapped. Each confirms the reader recovers the relocated object\ninstead of dying.\n\nAssisted-by: Claude Opus 4.8 & GPT-5.6 Sol\nSigned-off-by: Elijah Newren \n---\n midx.c | 6 ++\n odb.c | 8 +-\n odb.h | 16 +++-\n odb/source-packed.c | 9 ++-\n packfile.c | 39 ++++++++-\n t/meson.build | 1 +\n t/t5336-repack-reader-race.sh | 148 ++++++++++++++++++++++++++++++++++\n 7 files changed, 221 insertions(+), 6 deletions(-)\n create mode 100755 t/t5336-repack-reader-race.sh\n\ndiff --git a/midx.c b/midx.c\nindex 37f082dbdd..942505ac41 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -475,6 +475,12 @@ int prepare_midx_pack(struct multi_pack_index *m,\n \n \tif (!p) {\n \t\tm->packs[pack_int_id] = MIDX_PACK_ERROR;\n+\t\t/*\n+\t\t * The midx names a pack we can no longer open (its files\n+\t\t * vanished, e.g. a concurrent repack replaced it). Record the\n+\t\t * stale pack set (see stale_packs_detected).\n+\t\t */\n+\t\tpacked->base.odb->stale_packs_detected = 1;\n \t\treturn 1;\n \t}\n \ndiff --git a/odb.c b/odb.c\nindex 6bbea64033..4bb9662c65 100644\n--- a/odb.c\n+++ b/odb.c\n@@ -583,8 +583,14 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *\n \t\t * When the object hasn't been found we try a second read and\n \t\t * tell the sources so. This may cause them to invalidate\n \t\t * caches or reload on-disk state.\n+\t\t *\n+\t\t * A QUICK lookup normally skips this second read to stay fast\n+\t\t * on a genuine miss, but retry anyway when a pack vanished\n+\t\t * mid-lookup (stale_packs_detected): the object likely just\n+\t\t * moved into its replacement pack.\n \t\t */\n-\t\tif (!(flags & OBJECT_INFO_QUICK)) {\n+\t\tif (!(flags & OBJECT_INFO_QUICK) ||\n+\t\t odb->stale_packs_detected) {\n \t\t\tfor (source = odb->sources; source; source = source->next) {\n \t\t\t\tret = odb_source_read_object_info(source, real, oi,\n \t\t\t\t\t\t\t\t flags | OBJECT_INFO_SECOND_READ,\ndiff --git a/odb.h b/odb.h\nindex 1264d4ce7d..8b91e6f8ba 100644\n--- a/odb.h\n+++ b/odb.h\n@@ -93,6 +93,17 @@ struct object_database {\n \tunsigned object_count_flags;\n \tunsigned object_count_valid : 1;\n \n+\t/*\n+\t * Set when a lookup finds that a pack we already know about has\n+\t * vanished -- its \".idx\" or \".pack\" removed out from under us, the\n+\t * signature of a concurrent \"git repack\". It tells\n+\t * odb_read_object_info_extended() to reprepare and retry even for an\n+\t * OBJECT_INFO_QUICK lookup, which normally skips that rescan to stay\n+\t * fast on a genuine miss. Reset when the packfiles are reprepared\n+\t * (see odb_source_packed_prepare()).\n+\t */\n+\tunsigned stale_packs_detected : 1;\n+\n \t/*\n \t * Submodule source paths that will be added as additional sources to\n \t * allow lookup of submodule objects via the main object database.\n@@ -423,8 +434,9 @@ enum object_info_flags {\n \t * whether any on-disk state may have changed that may have caused the\n \t * object to appear.\n \t *\n-\t * This flag is for internal use, only. The second read only occurs\n-\t * when `OBJECT_INFO_QUICK` was not passed.\n+\t * This flag is for internal use, only. The second read occurs when\n+\t * OBJECT_INFO_QUICK was not passed, or when a vanished pack was\n+\t * detected (see stale_packs_detected).\n \t */\n \tOBJECT_INFO_SECOND_READ = (1 << 4),\n \ndiff --git a/odb/source-packed.c b/odb/source-packed.c\nindex 1a12a605db..b6c1d8fdf4 100644\n--- a/odb/source-packed.c\n+++ b/odb/source-packed.c\n@@ -798,8 +798,15 @@ static void odb_source_packed_prepare(struct odb_source *source,\n {\n \tstruct odb_source_packed *packed = odb_source_packed_downcast(source);\n \n-\tif (flags & ODB_PREPARE_FLUSH_CACHES)\n+\tif (flags & ODB_PREPARE_FLUSH_CACHES) {\n \t\tpacked->initialized = false;\n+\t\t/*\n+\t\t * A reprepare re-scans the on-disk pack set, so any pack we\n+\t\t * previously noticed had vanished is accounted for now; clear\n+\t\t * the flag that forced this rescan (see stale_packs_detected).\n+\t\t */\n+\t\tpacked->base.odb->stale_packs_detected = 0;\n+\t}\n \tif (packed->initialized)\n \t\treturn;\n \ndiff --git a/packfile.c b/packfile.c\nindex cd38be088d..bc8587d185 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -522,6 +522,21 @@ const char *pack_basename(struct packed_git *p)\n \treturn ret;\n }\n \n+/* Did the pack's \".idx\" vanish from disk (ENOENT), e.g. via a repack? */\n+static int pack_index_is_missing(struct packed_git *p)\n+{\n+\tchar *idx_name;\n+\tsize_t len;\n+\tint missing;\n+\n+\tif (!strip_suffix(p->pack_name, \".pack\", &len))\n+\t\treturn 0;\n+\tidx_name = xstrfmt(\"%.*s.idx\", (int)len, p->pack_name);\n+\tmissing = access(idx_name, F_OK) < 0 && errno == ENOENT;\n+\tfree(idx_name);\n+\treturn missing;\n+}\n+\n /*\n * Do not call this directly as this leaks p->pack_fd on error return;\n * call open_packed_git() instead.\n@@ -535,8 +550,20 @@ static int open_packed_git_1(struct packed_git *p)\n \tssize_t read_result;\n \tconst unsigned hashsz = p->repo->hash_algo->rawsz;\n \n-\tif (open_pack_index(p))\n+\tif (open_pack_index(p)) {\n+\t\t/*\n+\t\t * A concurrent repack may have removed this pack, deleting its\n+\t\t * \".idx\" before its \".pack\" (see unlink_pack_path()). If the\n+\t\t * index simply vanished, note the stale pack set and stay\n+\t\t * quiet; the pack is still reported unusable. Only a\n+\t\t * still-present but unreadable index is worth an error.\n+\t\t */\n+\t\tif (pack_index_is_missing(p)) {\n+\t\t\tp->repo->objects->stale_packs_detected = 1;\n+\t\t\treturn -1;\n+\t\t}\n \t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n+\t}\n \n \tif (!pack_max_fds) {\n \t\tunsigned int max_fds = get_max_fd_limit();\n@@ -552,8 +579,16 @@ static int open_packed_git_1(struct packed_git *p)\n \t\t; /* nothing */\n \n \tp->pack_fd = git_open(p->pack_name);\n-\tif (p->pack_fd < 0 || fstat(p->pack_fd, &st))\n+\tif (p->pack_fd < 0 || fstat(p->pack_fd, &st)) {\n+\t\t/*\n+\t\t * A concurrent repack removed this pack, but its \".idx\" was\n+\t\t * already mapped (so open_pack_index() above succeeded); the\n+\t\t * removal surfaces only now, when the \".pack\" cannot be opened.\n+\t\t */\n+\t\tif (p->pack_fd < 0 && errno == ENOENT)\n+\t\t\tp->repo->objects->stale_packs_detected = 1;\n \t\treturn -1;\n+\t}\n \tpack_open_fds++;\n \n \t/* If we created the struct before we had the pack we lack size. */\ndiff --git a/t/meson.build b/t/meson.build\nindex 2133c840da..28b63c486c 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -639,6 +639,7 @@ integration_tests = [\n 't5333-pseudo-merge-bitmaps.sh',\n 't5334-incremental-multi-pack-index.sh',\n 't5335-compact-multi-pack-index.sh',\n+ 't5336-repack-reader-race.sh',\n 't5351-unpack-large-objects.sh',\n 't5400-send-pack.sh',\n 't5401-update-hooks.sh',\ndiff --git a/t/t5336-repack-reader-race.sh b/t/t5336-repack-reader-race.sh\nnew file mode 100755\nindex 0000000000..63dad5521a\n--- /dev/null\n+++ b/t/t5336-repack-reader-race.sh\n@@ -0,0 +1,148 @@\n+#!/bin/sh\n+\n+test_description='reader recovery when a concurrent repack retires a pack\n+\n+\"git repack\" consolidates existing packs into a replacement pack and then\n+removes the redundant packs, deleting each pack.idx before its pack.pack (see\n+the ordering in unlink_pack_path()). A reader that discovered one of those\n+packs -- most easily through a multi-pack-index -- can look the pack up in the\n+window where its .idx is gone but its .pack is not.\n+\n+For an OBJECT_INFO_QUICK lookup this is not recovered automatically: QUICK\n+skips the reprepare-and-retry that a normal lookup performs, so a persistent\n+reader whose pack list predates the replacement pack reports the object as\n+missing even though it still lives in the replacement pack. \"git mktree\n+--batch\" is such a persistent QUICK reader: it stays resident across multiple\n+trees and resolves each entry with OBJECT_INFO_QUICK, so before this fix it\n+produced wrong output in this window.\n+\n+The removal can also be observed one step later, from the other side: a reader\n+that already mmapped a pack.idx (so open_pack_index() succeeds without touching\n+the filesystem) but has not yet opened its pack.pack. If the pack.pack is gone\n+by the time the reader opens it, the same QUICK false-negative results unless we\n+notice the vanished .pack and reprepare.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup repo with a multi-pack-index over per-object packs' '\n+\ttest_commit seed &&\n+\ta=$(echo A | git hash-object -w --stdin) &&\n+\tb=$(echo B | git hash-object -w --stdin) &&\n+\techo \"$a\" | git pack-objects .git/objects/pack/pack >pack-a &&\n+\techo \"$b\" | git pack-objects .git/objects/pack/pack >pack-b &&\n+\n+\t# Drop the loose copies so the blobs resolve only through the packs the\n+\t# multi-pack-index references; otherwise the loose object would satisfy\n+\t# the lookup and the pack-removal race could never be observed.\n+\tgit prune-packed &&\n+\tgit multi-pack-index write &&\n+\n+\tprintf \"100644 blob %s\\ta\\n\" \"$a\" >tree-a-input &&\n+\tprintf \"100644 blob %s\\tb\\n\" \"$b\" >tree-b-input\n+'\n+\n+test_expect_success PIPE 'QUICK reader recovers an object whose pack was retired mid-lookup' '\n+\tvictim=\".git/objects/pack/pack-$(cat pack-b)\" &&\n+\tmkfifo in out &&\n+\ttest_when_finished \"rm -f in out\" &&\n+\n+\t# \"git mktree --batch\" is a resident OBJECT_INFO_QUICK reader; start it\n+\t# now so its in-memory pack list / midx predates the replacement pack.\n+\t(git mktree --batch out 2>err &) &&\n+\texec 9>in &&\n+\texec 8&- || :\" &&\n+\ttest_when_finished \"exec 8<&- || :\" &&\n+\n+\t# The first tree forces the reader to prepare its (soon stale) pack view\n+\t# and gives us a synchronization point.\n+\tcat tree-a-input >&9 &&\n+\techo >&9 &&\n+\tread tree_a <&8 &&\n+\n+\t# Reproduce the transient state a concurrent repack creates: a\n+\t# replacement pack holding every object, plus the original pack for b\n+\t# with its .idx removed but its .pack still present.\n+\tgit cat-file --batch-all-objects --batch-check=\"%(objectname)\" >all-oids &&\n+\tgit pack-objects .git/objects/pack/pack /dev/null &&\n+\trm -f \"$victim.idx\" &&\n+\ttest_path_is_file \"$victim.pack\" &&\n+\n+\t# The reader (stale pack list) now resolves b. Without the recovery its\n+\t# QUICK lookup reports b missing and mktree dies; with it, b is found in\n+\t# the replacement pack and the misleading \"index unavailable\" error is\n+\t# not printed.\n+\tcat tree-b-input >&9 &&\n+\techo >&9 &&\n+\tread tree_b <&8 &&\n+\texec 9>&- &&\n+\n+\ttest -n \"$tree_b\" &&\n+\ttest_grep ! \"index unavailable\" err\n+'\n+\n+test_expect_success 'setup a second repo with plain (non-midx) packs' '\n+\tgit init nomidx &&\n+\t(\n+\t\tcd nomidx &&\n+\t\ttest_commit seed &&\n+\t\ta=$(echo A | git hash-object -w --stdin) &&\n+\t\tb=$(echo B | git hash-object -w --stdin) &&\n+\t\techo \"$a\" | git pack-objects .git/objects/pack/pack >pack-a &&\n+\t\techo \"$b\" | git pack-objects .git/objects/pack/pack >pack-b &&\n+\t\tgit prune-packed &&\n+\n+\t\tprintf \"100644 blob %s\\ta\\n\" \"$a\" >tree-a-input &&\n+\t\tprintf \"100644 blob %s\\tb\\n\" \"$b\" >tree-b-input\n+\t)\n+'\n+\n+test_expect_success PIPE 'QUICK reader recovers when a mapped pack loses its .pack mid-lookup' '\n+\t(\n+\t\tcd nomidx &&\n+\t\tvictim=\".git/objects/pack/pack-$(cat pack-b)\" &&\n+\t\tmkfifo in out &&\n+\n+\t\t# We run in a subshell, so leaving the fifos and the reader\n+\t\t# descriptors open is harmless: they are cleaned up when the\n+\t\t# subshell exits (which also lets \"git mktree --batch\" see EOF\n+\t\t# and quit).\n+\t\t(git mktree --batch out 2>err &) &&\n+\t\texec 9>in &&\n+\t\texec 8&9 &&\n+\t\techo >&9 &&\n+\t\tread tree_a <&8 &&\n+\n+\t\t# A concurrent repack writes a replacement pack holding every\n+\t\t# object and removes the now-redundant pack for b. Delete only\n+\t\t# its .pack: the reader keeps the mapped .idx for b, so\n+\t\t# open_pack_index() still succeeds and the failure surfaces when\n+\t\t# we open the vanished .pack.\n+\t\tgit cat-file --batch-all-objects --batch-check=\"%(objectname)\" >all-oids &&\n+\t\tgit pack-objects .git/objects/pack/pack /dev/null &&\n+\t\trm -f \"$victim.pack\" &&\n+\t\ttest_path_is_file \"$victim.idx\" &&\n+\n+\t\t# The reader (stale pack list) now resolves b. Without the\n+\t\t# recovery its QUICK lookup opens the missing .pack, gives up,\n+\t\t# and mktree dies; with it, the vanished .pack forces a reprepare\n+\t\t# and b is found in the replacement pack.\n+\t\tcat tree-b-input >&9 &&\n+\t\techo >&9 &&\n+\t\tread tree_b <&8 &&\n+\t\texec 9>&- &&\n+\n+\t\ttest -n \"$tree_b\"\n+\t)\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"551229","messageId":"eacf6ba4b11e366466da18b7b668e65793c532a9.1787684429.git.gitgitgadget@gmail.com","threadId":"66192","inReplyTo":"pull.2207.v2.git.1787684429.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] packfile: recover when a multi-pack-index names a removed pack","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-25T19:00:29Z","receivedAt":"2026-08-25T19:00:42Z","isPatch":true,"body":"From: Elijah Newren \n\nA geometric repack writes a new pack and multi-pack-index and then\ndeletes the packs the new one subsumes. A process still using the\nprevious MIDX keeps seeing a removed pack listed as the owner of some\nobjects. Since a MIDX attributes each object to exactly one pack, such\nan object is served only through its recorded owner; if that owner was\njust removed, find_pack_entry() cannot serve it -- fill_midx_entry()\nroutes to the missing pack, and the regular pack fallback deliberately\nskips every MIDX-covered pack, so a surviving copy in another covered\npack (e.g. a kept base pack) is never consulted.\n\nUnlike the ordinary \"a pack's .idx is mapped but its .pack is gone\"\nrace, the second read does not rescue us -- and not only for\nOBJECT_INFO_QUICK callers. Reloading the on-disk pack set does not\nreload the borrowed, cached MIDX (freeing it under the code that caches\nthe \"struct multi_pack_index *\" would be a use-after-free), so the stale\nMIDX keeps routing to the removed pack and the surviving copy stays\nhidden behind the covered-pack skip. cat-file, rev-list and pack-objects\ncan thus all spuriously fail with \"unable to read object\".\n\nTeach find_pack_entry() to recover. fill_midx_entry() now returns a\ntri-state, distinguishing \"absent from the MIDX\" from \"present but the\nowning pack is unavailable\"; in the latter case, once the regular\nfallback has also missed, scan the MIDX's packs directly for a surviving\ncopy.\n\nDo the scan only on the second read (OBJECT_INFO_SECOND_READ): by then\nthe cheaper on-disk reload has run, so an object merely relocated into a\nnew (non-covered) pack has already been found by the regular fallback,\nand only a genuine hidden duplicate reaches the rescan. QUICK callers\nthat would skip the second read are steered into it by the preceding\ncommit's stale_packs_detected flag, which prepare_midx_pack() sets when\nit cannot open the owning pack.\n\nReloading the stale MIDX would be a more complete fix but is much more\ninvolved (the borrowers above need proper invalidation), so leave that\nfor later.\n\nAssisted-by: Claude Opus 4.8 & GPT-5.6 Sol\nHelped-by: Jeff King \nSigned-off-by: Elijah Newren \n---\n builtin/pack-objects.c | 2 +-\n midx.c | 38 ++++++++++--------\n midx.h | 21 +++++++++-\n odb/source-packed.c | 42 ++++++++++++++++---\n t/t5319-multi-pack-index.sh | 80 +++++++++++++++++++++++++++++++++++++\n 5 files changed, 158 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 399acd0f22..30ad7d822c 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1786,7 +1786,7 @@ static int want_object_in_pack_mtime(const struct object_id *oid,\n \t\tstruct multi_pack_index *m = get_multi_pack_index(files->packed);\n \t\tstruct pack_entry e;\n \n-\t\tif (m && fill_midx_entry(m, oid, &e, NULL)) {\n+\t\tif (m && fill_midx_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {\n \t\t\twant = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\t\tif (want != -1)\n \t\t\t\treturn want;\ndiff --git a/midx.c b/midx.c\nindex 942505ac41..6b585f3c1a 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -595,46 +595,50 @@ uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos)\n \t\t\t\t\t (off_t)pos * MIDX_CHUNK_OFFSET_WIDTH);\n }\n \n-int fill_midx_entry(struct multi_pack_index *m,\n-\t\t const struct object_id *oid,\n-\t\t struct pack_entry *e,\n-\t\t struct packed_git **bad_pack)\n+enum midx_fill_result fill_midx_entry(struct multi_pack_index *m,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t struct pack_entry *e,\n+\t\t\t\t struct packed_git **bad_pack)\n {\n \tuint32_t pos;\n \tuint32_t pack_int_id;\n \tstruct packed_git *p;\n \n \tif (!bsearch_midx(oid, m, &pos))\n-\t\treturn 0;\n+\t\treturn MIDX_FILL_MISS;\n \n \tmidx_for_object(&m, pos);\n \tpack_int_id = nth_midxed_pack_int_id(m, pos);\n \n \tif (prepare_midx_pack(m, pack_int_id))\n-\t\treturn 0;\n+\t\tgoto owner_unavailable;\n \tp = m->packs[pack_int_id - m->num_packs_in_base];\n \n-\t/*\n-\t* We are about to tell the caller where they can locate the\n-\t* requested object. We better make sure the packfile is\n-\t* still here and can be accessed before supplying that\n-\t* answer, as it may have been deleted since the MIDX was\n-\t* loaded!\n-\t*/\n+\t/* Make sure the pack is still present before pointing at it. */\n \tif (!is_pack_valid(p))\n-\t\treturn 0;\n+\t\tgoto owner_unavailable;\n \n \tif (oidset_size(&p->bad_objects) &&\n \t oidset_contains(&p->bad_objects, oid)) {\n \t\tif (bad_pack && !*bad_pack)\n \t\t\t*bad_pack = p;\n-\t\treturn 0;\n+\t\treturn MIDX_FILL_MISS;\n \t}\n \n \te->offset = nth_midxed_offset(m, pos);\n \te->p = p;\n \n-\treturn 1;\n+\treturn MIDX_FILL_HIT;\n+\n+owner_unavailable:\n+\t/*\n+\t * Re-arm stale_packs_detected on every such lookup, not just the\n+\t * first: prepare_midx_pack() caches the failure, so without this a\n+\t * later lookup of the same vanished pack would leave the flag clear\n+\t * and a QUICK reader would skip its recovering second read.\n+\t */\n+\tm->source->base.odb->stale_packs_detected = 1;\n+\treturn MIDX_FILL_OWNER_UNAVAILABLE;\n }\n \n /* Match \"foo.idx\" against either \"foo.pack\" _or_ \"foo.idx\". */\n@@ -1038,7 +1042,7 @@ int verify_midx_file(struct odb_source_packed *source, unsigned flags)\n \n \t\tnth_midxed_object_oid(&oid, m, pairs[i].pos);\n \n-\t\tif (!fill_midx_entry(m, &oid, &e, NULL)) {\n+\t\tif (fill_midx_entry(m, &oid, &e, NULL) != MIDX_FILL_HIT) {\n \t\t\tmidx_report(_(\"failed to load pack entry for oid[%d] = %s\"),\n \t\t\t\t pairs[i].pos, oid_to_hex(&oid));\n \t\t\tcontinue;\ndiff --git a/midx.h b/midx.h\nindex 1f2f2d5321..52fe9c81e9 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -117,8 +117,25 @@ uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos);\n struct object_id *nth_midxed_object_oid(struct object_id *oid,\n \t\t\t\t\tstruct multi_pack_index *m,\n \t\t\t\t\tuint32_t n);\n-int fill_midx_entry(struct multi_pack_index *m, const struct object_id *oid,\n-\t\t struct pack_entry *e, struct packed_git **bad_pack);\n+/*\n+ * Result of looking an object up in a multi-pack-index. MIDX_FILL_HIT means\n+ * \"e was filled in\"; the two miss variants distinguish an object the midx does\n+ * not know about (MIDX_FILL_MISS) from one it does know about but whose owning\n+ * pack we can no longer open (MIDX_FILL_OWNER_UNAVAILABLE -- the signature of a\n+ * concurrent repack having removed that pack). A known-bad (corrupt) object\n+ * reports MIDX_FILL_MISS but also sets *bad_pack, if provided, to the owning\n+ * pack so the caller can tell \"corrupt\" apart from \"absent\".\n+ */\n+enum midx_fill_result {\n+\tMIDX_FILL_MISS = 0,\n+\tMIDX_FILL_HIT,\n+\tMIDX_FILL_OWNER_UNAVAILABLE,\n+};\n+\n+enum midx_fill_result fill_midx_entry(struct multi_pack_index *m,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t struct pack_entry *e,\n+\t\t\t\t struct packed_git **bad_pack);\n int midx_contains_pack(struct multi_pack_index *m,\n \t\t const char *idx_or_pack_name);\n int midx_layer_contains_pack(struct multi_pack_index *m,\ndiff --git a/odb/source-packed.c b/odb/source-packed.c\nindex b6c1d8fdf4..ae4c4bac40 100644\n--- a/odb/source-packed.c\n+++ b/odb/source-packed.c\n@@ -17,13 +17,18 @@\n static int find_pack_entry(struct odb_source_packed *store,\n \t\t\t const struct object_id *oid,\n \t\t\t struct pack_entry *e,\n+\t\t\t enum object_info_flags flags,\n \t\t\t struct packed_git **bad_pack)\n {\n \tstruct packfile_list_entry *l;\n+\tenum midx_fill_result midx_result = MIDX_FILL_MISS;\n \n \todb_source_prepare(&store->base, 0);\n-\tif (store->midx && fill_midx_entry(store->midx, oid, e, bad_pack))\n-\t\treturn 1;\n+\tif (store->midx) {\n+\t\tmidx_result = fill_midx_entry(store->midx, oid, e, bad_pack);\n+\t\tif (midx_result == MIDX_FILL_HIT)\n+\t\t\treturn 1;\n+\t}\n \n \tfor (l = store->packs.head; l; l = l->next) {\n \t\tstruct packed_git *p = l->pack;\n@@ -35,6 +40,33 @@ static int find_pack_entry(struct odb_source_packed *store,\n \t\t}\n \t}\n \n+\t/*\n+\t * Recovery for a concurrent-repack race: a stale MIDX may still name a\n+\t * vanished owning pack even though the object survives in another pack\n+\t * the same MIDX covers. The regular fallback above skips MIDX-covered\n+\t * packs, and repreparing the on-disk pack set does not reload the\n+\t * borrowed, cached MIDX, so scan its packs directly for the survivor.\n+\t *\n+\t * Do this only on the second read, by which point repreparing packs has\n+\t * already had a chance to find an object merely relocated into a new,\n+\t * uncovered pack; only a genuine hidden duplicate reaches here.\n+\t */\n+\tif (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&\n+\t (flags & OBJECT_INFO_SECOND_READ)) {\n+\t\tstruct multi_pack_index *m = store->midx;\n+\t\tuint32_t i;\n+\n+\t\tfor (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {\n+\t\t\tstruct packed_git *p;\n+\n+\t\t\tif (prepare_midx_pack(m, i))\n+\t\t\t\tcontinue;\n+\t\t\tp = nth_midxed_pack(m, i);\n+\t\t\tif (p && packfile_fill_entry(p, oid, e, bad_pack))\n+\t\t\t\treturn 1;\n+\t\t}\n+\t}\n+\n \treturn 0;\n }\n \n@@ -57,7 +89,7 @@ static enum odb_read_status odb_source_packed_read_object_info(struct odb_source\n \tif (flags & OBJECT_INFO_SECOND_READ)\n \t\todb_source_prepare(source, ODB_PREPARE_FLUSH_CACHES);\n \n-\tif (!find_pack_entry(packed, oid, &e, &bad_pack)) {\n+\tif (!find_pack_entry(packed, oid, &e, flags, &bad_pack)) {\n \t\t/*\n \t\t * The lookup may have failed because the object is known to be\n \t\t * corrupt in one of the packfiles. Report the object as\n@@ -105,7 +137,7 @@ static int odb_source_packed_read_object_stream(struct odb_read_stream **out,\n \tstruct odb_source_packed *packed = odb_source_packed_downcast(source);\n \tstruct pack_entry e;\n \n-\tif (!find_pack_entry(packed, oid, &e, NULL))\n+\tif (!find_pack_entry(packed, oid, &e, 0, NULL))\n \t\treturn -1;\n \n \treturn packfile_read_object_stream(out, oid, e.p, e.offset);\n@@ -611,7 +643,7 @@ static int odb_source_packed_freshen_object(struct odb_source *source,\n \t\ttimesp = ×\n \t}\n \n-\tif (!find_pack_entry(packed, oid, &e, NULL))\n+\tif (!find_pack_entry(packed, oid, &e, 0, NULL))\n \t\treturn 0;\n \tif (e.p->is_cruft)\n \t\treturn 0;\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 68143cb5b7..4041805807 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -1393,4 +1393,84 @@ test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '\n \t)\n '\n \n+test_expect_success 'lookup recovers object whose midx-owning pack was removed' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t# \"keep\" ends up only in the big pack; \"dup\" is deliberately\n+\t\t# placed in two packs so the midx has to choose an owner.\n+\t\ttest_commit keep &&\n+\t\techo duplicated-content >dup &&\n+\t\tgit add dup &&\n+\t\tgit commit -m dup &&\n+\t\tdup_oid=$(git rev-parse HEAD:dup) &&\n+\n+\t\t# Roll every object, including dup, into a single big pack.\n+\t\tgit repack -adq &&\n+\n+\t\t# Build a second, \"moderate\" pack that also contains dup, so dup\n+\t\t# now lives in two packs that the midx will cover.\n+\t\tmoderate=$(echo \"$dup_oid\" |\n+\t\t\tgit pack-objects --quiet $objdir/pack/pack) &&\n+\n+\t\t# Attribute dup to the moderate pack in the midx.\n+\t\tgit multi-pack-index write \\\n+\t\t\t--preferred-pack=\"pack-$moderate.idx\" &&\n+\n+\t\t# Simulate a concurrent \"git repack\" retiring the moderate pack:\n+\t\t# its files disappear, but the now-stale midx still names it as\n+\t\t# the owner of dup. A valid copy of dup survives in the big pack.\n+\t\trm -f $objdir/pack/pack-$moderate.* &&\n+\n+\t\t# The midx routes the lookup to the deleted pack, and the regular\n+\t\t# pack fallback skips midx-covered packs, so without recovery dup\n+\t\t# would appear missing even though it is physically present.\n+\t\techo blob >expect &&\n+\t\tgit cat-file -t \"$dup_oid\" >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'repeated QUICK lookups recover after owning pack removed' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t# Two blobs, each duplicated across packs so the midx must pick\n+\t\t# an owning pack, and each attributed to the same moderate pack.\n+\t\techo one >f1 &&\n+\t\techo two >f2 &&\n+\t\tgit add f1 f2 &&\n+\t\tgit commit -m dups &&\n+\t\td1=$(git rev-parse HEAD:f1) &&\n+\t\td2=$(git rev-parse HEAD:f2) &&\n+\n+\t\t# Roll every object, including d1 and d2, into one big pack,\n+\t\t# then build a moderate pack that also holds both blobs.\n+\t\tgit repack -adq &&\n+\t\tmoderate=$(printf \"%s\\n%s\\n\" \"$d1\" \"$d2\" |\n+\t\t\tgit pack-objects --quiet $objdir/pack/pack) &&\n+\n+\t\tgit multi-pack-index write \\\n+\t\t\t--preferred-pack=\"pack-$moderate.idx\" &&\n+\n+\t\t# Retire the moderate pack; the stale midx still names it as the\n+\t\t# owner of both blobs, each of which survives in the big pack.\n+\t\trm -f $objdir/pack/pack-$moderate.* &&\n+\n+\t\t# One resident QUICK reader (\"git mktree --batch\") resolves both\n+\t\t# blobs. The first lookup recovers d1 and caches the owning\n+\t\t# packs failure; unless that failure keeps re-arming the second\n+\t\t# read, the lookup of d2 skips its recovering read and the reader\n+\t\t# dies reporting d2 as missing.\n+\t\tprintf \"100644 blob %s\\tf1\\n\\n100644 blob %s\\tf2\\n\\n\" \\\n+\t\t\t\"$d1\" \"$d2\" |\n+\t\t\tgit mktree --batch >trees &&\n+\t\ttest_line_count = 2 trees\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"551340","messageId":"20260827053602.GA189659@coredump.intra.peff.net","threadId":"66192","inReplyTo":"3f3b75690eea02960c7edc8d318ce7dff654f1bc.1787684429.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/4] mktree: plug per-tree leak in --batch mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-27T05:36:02Z","receivedAt":"2026-08-27T05:36:05Z","isPatch":true,"body":"On Tue, Aug 25, 2026 at 07:00:27PM +0000, Elijah Newren via GitGitGadget wrote:\n\n> In --batch mode \"git mktree\" reuses its entry buffer across trees,\n> resetting `used` to 0 after writing each tree. It never frees the\n> `treeent` structures the previous tree appended, though, so once the\n> next tree overwrites those slots the earlier allocations are leaked. A\n> single-tree invocation hides this, as the entries stay reachable through\n> the `entries` global until exit.\n> \n> Free each entry when resetting the buffer, and free the buffer itself\n> before returning.\n\nYikes. It is sad that we did not catch this in our leak-checking builds,\nas it implies that we do not test \"mktree --batch\" with multiple inputs.\nOr grepping for \"mktree.*--batch\" implies that we do not test the\nfeature at all!\n\nLooks like that feature comes from f1cf2d8b14 (mktree --batch: build\nmore than one tree object, 2009-05-14), so I am not surprised that test\ncoverage was a bit more spotty back then.\n\nI guess you are going to add some coverage incidentally (or else you\nwould not have found this). That's better than nothing, but I suspect a\nfew basic directed \"mktree --batch\" tests would be a good thing to have\nin t1010.\n\n#leftoverbits, perhaps?\n\n-Peff\n"},{"id":"551341","messageId":"20260827055743.GB189659@coredump.intra.peff.net","threadId":"66192","inReplyTo":"fc98f48ddb4d46cad66a40ecdd96c139e1397784.1787684429.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/4] packfile: recover object lookups racing a concurrent repack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-27T05:57:43Z","receivedAt":"2026-08-27T05:57:44Z","isPatch":true,"body":"On Tue, Aug 25, 2026 at 07:00:28PM +0000, Elijah Newren via GitGitGadget wrote:\n\n> 1. open_pack_index() fails, so we print\n> \n> error: packfile index unavailable\n> \n> and report the pack as unusable, even though the object still lives\n> in the replacement pack.\n> \n> 2. A normal lookup recovers: odb_read_object_info_extended() issues a\n> second read that reloads the on-disk pack state and finds the object\n> in its new home, making the message above mere noise. But an\n> OBJECT_INFO_QUICK lookup deliberately skips that second read to stay\n> fast on a genuine miss, so it does *not* recover: it reports the\n> object as absent even though it still lives in the replacement pack.\n> A resident reader that resolves objects with a QUICK lookup -- such\n> as the `git mktree --batch` process the tests below drive -- then\n> produces wrong results. Even where a spurious miss is not fatal it\n> is not harmless: `git upload-pack` checks a client's \"have\" lines\n> with a QUICK lookup, and a dropped \"have\" removes a common object\n> from the negotiation, so the client is sent more than it needs.\n\nMaybe I am still being dense, but this description does not make any\nsense to me at all.\n\nThe _point_ of QUICK is to accept those false negatives. It is the right\nthing for upload-pack to do, to avoid re-scans for objects which we\nsimply don't have (and don't necessarily expect to have).\n\nIt sounds like mktree is wrong to be using QUICK at all. It comes from\n817b0f6027 (mktree: do not check type of remote objects, 2022-06-21)\nwhich rewrote a call to vanilla oid_object_info(). From the description\nthere it probably should be using SKIP_FETCH_OBJECT but not QUICK. Or\npossibly it should use neither unless --missing is given.\n\nSo I don't see QUICK itself here violating any contract (even if it\n_could_ find the object in some cases with just a little more work, as\nin the case that we were discussing for v1).\n\nThe much more interesting case is the non-QUICK one that Patrick\noutlined earlier in the thread. Where we say \"nope, we don't have that\nobject\" even though we could find it with a little more work. But that\ndoesn't seem to be described here either. But I think that is not even\nwhat this patch is about; that's in patch 4.\n\nIf the \"error:\" message is scary and gross (especially because we may\nretry and correct it anyway) and happens due to routine races, we might\nconsider suppressing it.\n\n> +\t/*\n> +\t * Set when a lookup finds that a pack we already know about has\n> +\t * vanished -- its \".idx\" or \".pack\" removed out from under us, the\n> +\t * signature of a concurrent \"git repack\". It tells\n> +\t * odb_read_object_info_extended() to reprepare and retry even for an\n> +\t * OBJECT_INFO_QUICK lookup, which normally skips that rescan to stay\n> +\t * fast on a genuine miss. Reset when the packfiles are reprepared\n> +\t * (see odb_source_packed_prepare()).\n> +\t */\n> +\tunsigned stale_packs_detected : 1;\n\nSo this is a way of hackily triggering SECOND_READ for QUICK queries,\neven though the point of QUICK is to suppress that second read! Again,\nmaybe I'm just being dense, but I don't get it.\n\n> @@ -535,8 +550,20 @@ static int open_packed_git_1(struct packed_git *p)\n> \tssize_t read_result;\n> \tconst unsigned hashsz = p->repo->hash_algo->rawsz;\n> \n> -\tif (open_pack_index(p))\n> +\tif (open_pack_index(p)) {\n> +\t\t/*\n> +\t\t * A concurrent repack may have removed this pack, deleting its\n> +\t\t * \".idx\" before its \".pack\" (see unlink_pack_path()). If the\n> +\t\t * index simply vanished, note the stale pack set and stay\n> +\t\t * quiet; the pack is still reported unusable. Only a\n> +\t\t * still-present but unreadable index is worth an error.\n> +\t\t */\n> +\t\tif (pack_index_is_missing(p)) {\n> +\t\t\tp->repo->objects->stale_packs_detected = 1;\n> +\t\t\treturn -1;\n> +\t\t}\n> \t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n> +\t}\n\nAnd this seems racy. We might catch the .idx but miss the .pack file.\nThat would cause a failed read, but not trigger sale_packs_detected.\n\n-Peff\n"},{"id":"551342","messageId":"20260827060622.GC189659@coredump.intra.peff.net","threadId":"66192","inReplyTo":"eacf6ba4b11e366466da18b7b668e65793c532a9.1787684429.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/4] packfile: recover when a multi-pack-index names a removed pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-27T06:06:22Z","receivedAt":"2026-08-27T06:06:24Z","isPatch":true,"body":"On Tue, Aug 25, 2026 at 07:00:29PM +0000, Elijah Newren via GitGitGadget wrote:\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 399acd0f22..30ad7d822c 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1786,7 +1786,7 @@ static int want_object_in_pack_mtime(const struct object_id *oid,\n> \t\tstruct multi_pack_index *m = get_multi_pack_index(files->packed);\n> \t\tstruct pack_entry e;\n> \n> -\t\tif (m && fill_midx_entry(m, oid, &e, NULL)) {\n> +\t\tif (m && fill_midx_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {\n> \t\t\twant = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);\n> \t\t\tif (want != -1)\n> \t\t\t\treturn want;\n\nWe've changed the return value semantics without changing the signature\n(or name). So we need to make sure we adjust all callers, as here.\nThat's _probably_ OK in practice for such a specialized function. But we\ncould also rename it if we wanted to be paranoid (especially about\nnew callers added on parallel branches).\n\n> +enum midx_fill_result fill_midx_entry(struct multi_pack_index *m,\n> +\t\t\t\t const struct object_id *oid,\n> +\t\t\t\t struct pack_entry *e,\n> +\t\t\t\t struct packed_git **bad_pack)\n\nOK, so this is our tri-state fix. Mostly looks as expected, though:\n\n> \tif (prepare_midx_pack(m, pack_int_id))\n> -\t\treturn 0;\n> +\t\tgoto owner_unavailable;\n\nI'd have expected just \"return MIDX_FILL_OWNER_UNAVAILABLE\" here. But\nthen, I'm not sure I buy the need for this stale_packs_detected stuff\nfrom patch 3.\n\n> \tp = m->packs[pack_int_id - m->num_packs_in_base];\n> \n> -\t/*\n> -\t* We are about to tell the caller where they can locate the\n> -\t* requested object. We better make sure the packfile is\n> -\t* still here and can be accessed before supplying that\n> -\t* answer, as it may have been deleted since the MIDX was\n> -\t* loaded!\n> -\t*/\n> +\t/* Make sure the pack is still present before pointing at it. */\n> \tif (!is_pack_valid(p))\n> -\t\treturn 0;\n> +\t\tgoto owner_unavailable;\n\nThis comment rewrite seems superfluous at best. Can we try to keep such\npatch fluff to a minimum?\n\n> +\t/*\n> +\t * Recovery for a concurrent-repack race: a stale MIDX may still name a\n> +\t * vanished owning pack even though the object survives in another pack\n> +\t * the same MIDX covers. The regular fallback above skips MIDX-covered\n> +\t * packs, and repreparing the on-disk pack set does not reload the\n> +\t * borrowed, cached MIDX, so scan its packs directly for the survivor.\n> +\t *\n> +\t * Do this only on the second read, by which point repreparing packs has\n> +\t * already had a chance to find an object merely relocated into a new,\n> +\t * uncovered pack; only a genuine hidden duplicate reaches here.\n> +\t */\n> +\tif (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&\n> +\t (flags & OBJECT_INFO_SECOND_READ)) {\n> +\t\tstruct multi_pack_index *m = store->midx;\n> +\t\tuint32_t i;\n> +\n> +\t\tfor (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {\n> +\t\t\tstruct packed_git *p;\n> +\n> +\t\t\tif (prepare_midx_pack(m, i))\n> +\t\t\t\tcontinue;\n> +\t\t\tp = nth_midxed_pack(m, i);\n> +\t\t\tif (p && packfile_fill_entry(p, oid, e, bad_pack))\n> +\t\t\t\treturn 1;\n> +\t\t}\n> +\t}\n\nOK, and this is as-before but now gated on the SECOND_READ flag. As\nexpected in this revision.\n\n-Peff\n"},{"id":"551395","messageId":"CABPp-BEmReAR-f-aweM=f=5QhRPxG1K-KLTsbyRt2aDQD_QnVA@mail.gmail.com","threadId":"66192","inReplyTo":"20260827055743.GB189659@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/4] packfile: recover object lookups racing a concurrent repack","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-08-27T22:23:30Z","receivedAt":"2026-08-27T22:23:49Z","isPatch":true,"body":"On Wed, Aug 26, 2026 at 10:57 PM Jeff King wrote:\n>\n> On Tue, Aug 25, 2026 at 07:00:28PM +0000, Elijah Newren via GitGitGadget wrote:\n>\n> > 1. open_pack_index() fails, so we print\n> >\n> > error: packfile index unavailable\n> >\n> > and report the pack as unusable, even though the object still lives\n> > in the replacement pack.\n> >\n> > 2. A normal lookup recovers: odb_read_object_info_extended() issues a\n> > second read that reloads the on-disk pack state and finds the object\n> > in its new home, making the message above mere noise. But an\n> > OBJECT_INFO_QUICK lookup deliberately skips that second read to stay\n> > fast on a genuine miss, so it does *not* recover: it reports the\n> > object as absent even though it still lives in the replacement pack.\n> > A resident reader that resolves objects with a QUICK lookup -- such\n> > as the `git mktree --batch` process the tests below drive -- then\n> > produces wrong results. Even where a spurious miss is not fatal it\n> > is not harmless: `git upload-pack` checks a client's \"have\" lines\n> > with a QUICK lookup, and a dropped \"have\" removes a common object\n> > from the negotiation, so the client is sent more than it needs.\n>\n> Maybe I am still being dense, but this description does not make any\n> sense to me at all.\n>\n> The _point_ of QUICK is to accept those false negatives. It is the right\n> thing for upload-pack to do, to avoid re-scans for objects which we\n> simply don't have (and don't necessarily expect to have).\n\nIt's far more likely that I am the one being dense. My rough line of thinking:\n\n* We see \"packfile ... index unavailable\" in our logging\n* There's only one thing that remove packfiles\n* Investigate the mechanism\n* Look for other affected callers (e.g. mktree --batch)\n* Consider corrective measures\n\nSteps 1-4 above are probably fine, and step 5 may have been where I\nwent off the rails. My thinking there, wrong or right, was:\n\n* It makes sense that we don't want to reprepare most of the time\n* ...but _if_ we know of the existence of some specific packfile in\nthis process and that packfile has since disappeared by the time we go\nto open or read it, is that a special case? Should it be?\n\n> It sounds like mktree is wrong to be using QUICK at all. It comes from\n> 817b0f6027 (mktree: do not check type of remote objects, 2022-06-21)\n> which rewrote a call to vanilla oid_object_info(). From the description\n> there it probably should be using SKIP_FETCH_OBJECT but not QUICK. Or\n> possibly it should use neither unless --missing is given.\n>\n> So I don't see QUICK itself here violating any contract (even if it\n> _could_ find the object in some cases with just a little more work, as\n> in the case that we were discussing for v1).\n\nI'll drop this patch and instead send a small mktree change that stops\npassing OBJECT_INFO_QUICK (keeping SKIP_FETCH_OBJECT), so mktree\nrecovers via the normal reprepare like every other non-QUICK reader.\nThat removes the packfile.c changes entirely, so both the\nreload-under-QUICK hack and the .idx/.pack raciness you noted in\npack_index_is_missing() go away with them.\n"},{"id":"551402","messageId":"CABPp-BFhPONjNuVZQfgwKuYdgbm5Fjjttz5q5wSYX6j1Zdwdww@mail.gmail.com","threadId":"66192","inReplyTo":"20260827060622.GC189659@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/4] packfile: recover when a multi-pack-index names a removed pack","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-08-28T07:29:49Z","receivedAt":"2026-08-28T07:30:01Z","isPatch":true,"body":"On Wed, Aug 26, 2026 at 11:06 PM Jeff King wrote:\n>\n> On Tue, Aug 25, 2026 at 07:00:29PM +0000, Elijah Newren via GitGitGadget wrote:\n>\n> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> > index 399acd0f22..30ad7d822c 100644\n> > --- a/builtin/pack-objects.c\n> > +++ b/builtin/pack-objects.c\n> > @@ -1786,7 +1786,7 @@ static int want_object_in_pack_mtime(const struct object_id *oid,\n> > struct multi_pack_index *m = get_multi_pack_index(files->packed);\n> > struct pack_entry e;\n> >\n> > - if (m && fill_midx_entry(m, oid, &e, NULL)) {\n> > + if (m && fill_midx_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {\n> > want = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);\n> > if (want != -1)\n> > return want;\n>\n> We've changed the return value semantics without changing the signature\n> (or name). So we need to make sure we adjust all callers, as here.\n> That's _probably_ OK in practice for such a specialized function. But we\n> could also rename it if we wanted to be paranoid (especially about\n> new callers added on parallel branches).\n\nAny suggestions for alternate names? fill_midx_entry_result? midx_fill_entry?\n\n> > +enum midx_fill_result fill_midx_entry(struct multi_pack_index *m,\n> > + const struct object_id *oid,\n> > + struct pack_entry *e,\n> > + struct packed_git **bad_pack)\n>\n> OK, so this is our tri-state fix. Mostly looks as expected, though:\n>\n> > if (prepare_midx_pack(m, pack_int_id))\n> > - return 0;\n> > + goto owner_unavailable;\n>\n> I'd have expected just \"return MIDX_FILL_OWNER_UNAVAILABLE\" here. But\n> then, I'm not sure I buy the need for this stale_packs_detected stuff\n> from patch 3.\n\nYeah, with the drop of patch 3 it becomes that.\n\n> > p = m->packs[pack_int_id - m->num_packs_in_base];\n> >\n> > - /*\n> > - * We are about to tell the caller where they can locate the\n> > - * requested object. We better make sure the packfile is\n> > - * still here and can be accessed before supplying that\n> > - * answer, as it may have been deleted since the MIDX was\n> > - * loaded!\n> > - */\n> > + /* Make sure the pack is still present before pointing at it. */\n> > if (!is_pack_valid(p))\n> > - return 0;\n> > + goto owner_unavailable;\n>\n> This comment rewrite seems superfluous at best. Can we try to keep such\n> patch fluff to a minimum?\n\nYes, sorry.\n\n> > + /*\n> > + * Recovery for a concurrent-repack race: a stale MIDX may still name a\n> > + * vanished owning pack even though the object survives in another pack\n> > + * the same MIDX covers. The regular fallback above skips MIDX-covered\n> > + * packs, and repreparing the on-disk pack set does not reload the\n> > + * borrowed, cached MIDX, so scan its packs directly for the survivor.\n> > + *\n> > + * Do this only on the second read, by which point repreparing packs has\n> > + * already had a chance to find an object merely relocated into a new,\n> > + * uncovered pack; only a genuine hidden duplicate reaches here.\n> > + */\n> > + if (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&\n> > + (flags & OBJECT_INFO_SECOND_READ)) {\n> > + struct multi_pack_index *m = store->midx;\n> > + uint32_t i;\n> > +\n> > + for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {\n> > + struct packed_git *p;\n> > +\n> > + if (prepare_midx_pack(m, i))\n> > + continue;\n> > + p = nth_midxed_pack(m, i);\n> > + if (p && packfile_fill_entry(p, oid, e, bad_pack))\n> > + return 1;\n> > + }\n> > + }\n>\n> OK, and this is as-before but now gated on the SECOND_READ flag. As\n> expected in this revision.\n\nThanks for taking a look!\n"},{"id":"551451","messageId":"pull.2207.v3.git.1787986831.gitgitgadget@gmail.com","threadId":"66192","inReplyTo":"pull.2207.git.1787092446.gitgitgadget@gmail.com","subject":"[PATCH v3 0/4] Objects treated as missing despite being present, due to race with geometric repacking","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-29T07:00:27Z","receivedAt":"2026-08-29T07:00:35Z","isPatch":true,"body":"Changes since v2:\n\n * Ripped out the old 3/4 dealing with QUICK readers; QUICK readers are left\n alone\n * Insert a new 3/4 fixing git mktree --batch to stop passing QUICK (with\n new testcase in t1010)\n * undo bad paragraph comment change\n * renamed fill_midx_entry() -> midx_fill_entry(), so that we catch any\n other new callers and appropriately check their return value (caught one\n in test-read-midx.c)\n\nChanges since v1:\n\n * Rebased on top of ps/odb-generic-corrupt-objects, and conflicts with it\n resolved\n * Removed useless test_grep line spotted by Junio in PATCH 1\n * Switched fill_midx_entry() to a tri-state to avoid duplicate\n bsearch_midx(), as suggested by Peff\n * Only do the re-read on SECOND_READ, as suggested by Peff\n * Handle multiple objects shared across multiple packs correctly (issue\n caught & corrected & new testcase by deeper AI review)\n * Inserted two new patches:\n * 2/4: Fix a leak in git mktree --batch since I use it in new testcases\n and don't want the *-leaks jobs failing\n * 3/4: Demonstrate and fix QUICK reader problems, while keeping expected\n QUICK performance for normal cases (we've already been discussing this\n patch in this thread a bunch anyway, and it's logically related)\n\nCover letter addendum/update:\n\nWe also fix git mktree --batch to no longer erroneously pass QUICK.\n\nNote here that Stolee's suggestion to defer pack deletion via git\nmulti-pack-index expire seems like a good complementary mitigation; it would\nreduce how often we fall into recovery, while this series tries to fix\nrecovery to work more robustly.\n\nOriginal cover letter (focused on the final patch):\n\nWhen an object is found in multiple packs that are in a multi-pack-index,\nand a subsequent geometric repacking creates a new multi-pack-index and\nremoves the pack that was considered the owner of the object in the old\nmulti-pack-index, then an already-running process that had opened the old\nmulti-pack-index and hadn't yet opened the removed packfile will not be able\nto access the object -- lookups will return it as missing. Additionally,\nreplay has a separate bug where a missing object causes a SIGSEGV rather\nthan an error message.\n\nThis appears to affect a very small percentage of git operations in\nproduction since it is a tiny window, but I've found evidence of it\noccurring in at least eight distinct server-side operations, covering seven\ndifferent git commands:\n\ngit operation symptom\n----------------------------------- -----------------------------\ngit replay (server-side rebase) SIGSEGV (this series, 1/2)\ngit merge-tree spurious read-miss failure\ngit diff (raw and tree-vs-tree) spurious read-miss failure\ngit rev-list --count spurious read-miss failure\ngit merge-base spurious read-miss failure\nobject/rev resolution (rev-parse, spurious read-miss failure\n cat-file)\nrepository repair (fsck/repack) spurious read-miss failure\n\n\nThere are also commands that could be changing behavior without throwing an\nerror -- e.g. object negotiation thinking an object doesn't exist and\ninstead negotiating based on an older common commit, or cat-file --batch\nreporting that some objects don't exist.\n\nThis series fixes the replay bug first, since it's simpler; investigating\nit, together with my other recent repacking work, is what led me to the\nunderlying multi-pack-index issue that 2/2 addresses.\n\nElijah Newren (4):\n replay: fail gracefully when a merge input is unreadable\n mktree: plug per-tree leak in --batch mode\n mktree: do not use OBJECT_INFO_QUICK when checking objects\n packfile: recover when a multi-pack-index names a removed pack\n\n builtin/mktree.c | 4 +++-\n builtin/pack-objects.c | 2 +-\n midx.c | 20 ++++++++--------\n midx.h | 21 ++++++++++++++--\n odb/source-packed.c | 42 ++++++++++++++++++++++++++++----\n replay.c | 7 ++++++\n t/helper/test-read-midx.c | 2 +-\n t/t1010-mktree.sh | 48 +++++++++++++++++++++++++++++++++++++\n t/t3650-replay-basics.sh | 34 ++++++++++++++++++++++++++\n t/t5319-multi-pack-index.sh | 40 +++++++++++++++++++++++++++++++\n 10 files changed, 200 insertions(+), 20 deletions(-)\n\n\nbase-commit: 2135b14863642bbcec02996e7f5e54ac1f77b03a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2207%2Fnewren%2Fmidx-removed-pack-recovery-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2207/newren/midx-removed-pack-recovery-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/2207\n\nRange-diff vs v2:\n\n 1: 36bf2ce17b = 1: 36bf2ce17b replay: fail gracefully when a merge input is unreadable\n 2: 3f3b75690e = 2: 3f3b75690e mktree: plug per-tree leak in --batch mode\n 3: fc98f48ddb < -: ---------- packfile: recover object lookups racing a concurrent repack\n -: ---------- > 3: 79ce753c68 mktree: do not use OBJECT_INFO_QUICK when checking objects\n 4: eacf6ba4b1 ! 4: 9b0966df9a packfile: recover when a multi-pack-index names a removed pack\n @@ Commit message\n previous MIDX keeps seeing a removed pack listed as the owner of some\n objects. Since a MIDX attributes each object to exactly one pack, such\n an object is served only through its recorded owner; if that owner was\n - just removed, find_pack_entry() cannot serve it -- fill_midx_entry()\n - routes to the missing pack, and the regular pack fallback deliberately\n - skips every MIDX-covered pack, so a surviving copy in another covered\n - pack (e.g. a kept base pack) is never consulted.\n + just removed, find_pack_entry() cannot serve it -- the MIDX lookup routes\n + to the missing pack, and the regular pack fallback deliberately skips\n + every MIDX-covered pack, so a surviving copy in another covered pack\n + (e.g. a kept base pack) is never consulted.\n \n Unlike the ordinary \"a pack's .idx is mapped but its .pack is gone\"\n - race, the second read does not rescue us -- and not only for\n - OBJECT_INFO_QUICK callers. Reloading the on-disk pack set does not\n - reload the borrowed, cached MIDX (freeing it under the code that caches\n - the \"struct multi_pack_index *\" would be a use-after-free), so the stale\n - MIDX keeps routing to the removed pack and the surviving copy stays\n + race, the second read does not rescue us. Reloading the on-disk pack set\n + does not reload the borrowed, cached MIDX (freeing it under the code that\n + caches the \"struct multi_pack_index *\" would be a use-after-free), so the\n + stale MIDX keeps routing to the removed pack and the surviving copy stays\n hidden behind the covered-pack skip. cat-file, rev-list and pack-objects\n can thus all spuriously fail with \"unable to read object\".\n \n - Teach find_pack_entry() to recover. fill_midx_entry() now returns a\n - tri-state, distinguishing \"absent from the MIDX\" from \"present but the\n - owning pack is unavailable\"; in the latter case, once the regular\n - fallback has also missed, scan the MIDX's packs directly for a surviving\n - copy.\n + Teach find_pack_entry() to recover. The MIDX lookup now returns a\n + tri-state, distinguishing an object absent from the MIDX from one it owns\n + via a pack that can no longer be opened; in the latter case, once the\n + regular fallback has also missed, scan the MIDX's packs directly for a\n + surviving copy. Because the return value is no longer a boolean, rename\n + fill_midx_entry() to midx_fill_entry() so callers must reckon with the\n + new enum rather than silently treat MIDX_FILL_OWNER_UNAVAILABLE as a hit.\n \n Do the scan only on the second read (OBJECT_INFO_SECOND_READ): by then\n the cheaper on-disk reload has run, so an object merely relocated into a\n - new (non-covered) pack has already been found by the regular fallback,\n - and only a genuine hidden duplicate reaches the rescan. QUICK callers\n - that would skip the second read are steered into it by the preceding\n - commit's stale_packs_detected flag, which prepare_midx_pack() sets when\n - it cannot open the owning pack.\n + new (uncovered) pack has already been found by the regular fallback, and\n + only a genuine hidden duplicate reaches the rescan. A QUICK caller that\n + skips the second read simply accepts the false negative, as QUICK is\n + designed to.\n \n Reloading the stale MIDX would be a more complete fix but is much more\n involved (the borrowers above need proper invalidation), so leave that\n @@ builtin/pack-objects.c: static int want_object_in_pack_mtime(const struct object\n \t\tstruct pack_entry e;\n \n -\t\tif (m && fill_midx_entry(m, oid, &e, NULL)) {\n -+\t\tif (m && fill_midx_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {\n ++\t\tif (m && midx_fill_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {\n \t\t\twant = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\t\tif (want != -1)\n \t\t\t\treturn want;\n @@ midx.c: uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos\n -\t\t const struct object_id *oid,\n -\t\t struct pack_entry *e,\n -\t\t struct packed_git **bad_pack)\n -+enum midx_fill_result fill_midx_entry(struct multi_pack_index *m,\n ++enum midx_fill_result midx_fill_entry(struct multi_pack_index *m,\n +\t\t\t\t const struct object_id *oid,\n +\t\t\t\t struct pack_entry *e,\n +\t\t\t\t struct packed_git **bad_pack)\n @@ midx.c: uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos\n \n \tif (prepare_midx_pack(m, pack_int_id))\n -\t\treturn 0;\n -+\t\tgoto owner_unavailable;\n ++\t\treturn MIDX_FILL_OWNER_UNAVAILABLE;\n \tp = m->packs[pack_int_id - m->num_packs_in_base];\n \n --\t/*\n --\t* We are about to tell the caller where they can locate the\n --\t* requested object. We better make sure the packfile is\n --\t* still here and can be accessed before supplying that\n --\t* answer, as it may have been deleted since the MIDX was\n --\t* loaded!\n --\t*/\n -+\t/* Make sure the pack is still present before pointing at it. */\n + \t/*\n +@@ midx.c: int fill_midx_entry(struct multi_pack_index *m,\n + \t* loaded!\n + \t*/\n \tif (!is_pack_valid(p))\n -\t\treturn 0;\n -+\t\tgoto owner_unavailable;\n ++\t\treturn MIDX_FILL_OWNER_UNAVAILABLE;\n \n \tif (oidset_size(&p->bad_objects) &&\n \t oidset_contains(&p->bad_objects, oid)) {\n @@ midx.c: uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos\n \n -\treturn 1;\n +\treturn MIDX_FILL_HIT;\n -+\n -+owner_unavailable:\n -+\t/*\n -+\t * Re-arm stale_packs_detected on every such lookup, not just the\n -+\t * first: prepare_midx_pack() caches the failure, so without this a\n -+\t * later lookup of the same vanished pack would leave the flag clear\n -+\t * and a QUICK reader would skip its recovering second read.\n -+\t */\n -+\tm->source->base.odb->stale_packs_detected = 1;\n -+\treturn MIDX_FILL_OWNER_UNAVAILABLE;\n }\n \n /* Match \"foo.idx\" against either \"foo.pack\" _or_ \"foo.idx\". */\n @@ midx.c: int verify_midx_file(struct odb_source_packed *source, unsigned flags)\n \t\tnth_midxed_object_oid(&oid, m, pairs[i].pos);\n \n -\t\tif (!fill_midx_entry(m, &oid, &e, NULL)) {\n -+\t\tif (fill_midx_entry(m, &oid, &e, NULL) != MIDX_FILL_HIT) {\n ++\t\tif (midx_fill_entry(m, &oid, &e, NULL) != MIDX_FILL_HIT) {\n \t\t\tmidx_report(_(\"failed to load pack entry for oid[%d] = %s\"),\n \t\t\t\t pairs[i].pos, oid_to_hex(&oid));\n \t\t\tcontinue;\n @@ midx.h: uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos\n +\tMIDX_FILL_OWNER_UNAVAILABLE,\n +};\n +\n -+enum midx_fill_result fill_midx_entry(struct multi_pack_index *m,\n ++enum midx_fill_result midx_fill_entry(struct multi_pack_index *m,\n +\t\t\t\t const struct object_id *oid,\n +\t\t\t\t struct pack_entry *e,\n +\t\t\t\t struct packed_git **bad_pack);\n @@ odb/source-packed.c\n -\tif (store->midx && fill_midx_entry(store->midx, oid, e, bad_pack))\n -\t\treturn 1;\n +\tif (store->midx) {\n -+\t\tmidx_result = fill_midx_entry(store->midx, oid, e, bad_pack);\n ++\t\tmidx_result = midx_fill_entry(store->midx, oid, e, bad_pack);\n +\t\tif (midx_result == MIDX_FILL_HIT)\n +\t\t\treturn 1;\n +\t}\n @@ odb/source-packed.c: static int odb_source_packed_freshen_object(struct odb_sour\n \tif (e.p->is_cruft)\n \t\treturn 0;\n \n + ## t/helper/test-read-midx.c ##\n +@@ t/helper/test-read-midx.c: static int read_midx_file(const char *object_dir, const char *checksum,\n + \t\tfor (i = 0; i < m->num_objects; i++) {\n + \t\t\tnth_midxed_object_oid(&oid, m,\n + \t\t\t\t\t i + m->num_objects_in_base);\n +-\t\t\tfill_midx_entry(m, &oid, &e, NULL);\n ++\t\t\tmidx_fill_entry(m, &oid, &e, NULL);\n + \n + \t\t\tprintf(\"%s %\"PRIu64\"\\t%s\\n\",\n + \t\t\t oid_to_hex(&oid), e.offset, e.p->pack_name);\n +\n ## t/t5319-multi-pack-index.sh ##\n @@ t/t5319-multi-pack-index.sh: test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '\n \t)\n @@ t/t5319-multi-pack-index.sh: test_expect_success 'pack.preferBitmapTips interpre\n +\t\ttest_cmp expect actual\n +\t)\n +'\n -+\n -+test_expect_success 'repeated QUICK lookups recover after owning pack removed' '\n -+\ttest_when_finished \"rm -fr repo\" &&\n -+\tgit init repo &&\n -+\t(\n -+\t\tcd repo &&\n -+\n -+\t\t# Two blobs, each duplicated across packs so the midx must pick\n -+\t\t# an owning pack, and each attributed to the same moderate pack.\n -+\t\techo one >f1 &&\n -+\t\techo two >f2 &&\n -+\t\tgit add f1 f2 &&\n -+\t\tgit commit -m dups &&\n -+\t\td1=$(git rev-parse HEAD:f1) &&\n -+\t\td2=$(git rev-parse HEAD:f2) &&\n -+\n -+\t\t# Roll every object, including d1 and d2, into one big pack,\n -+\t\t# then build a moderate pack that also holds both blobs.\n -+\t\tgit repack -adq &&\n -+\t\tmoderate=$(printf \"%s\\n%s\\n\" \"$d1\" \"$d2\" |\n -+\t\t\tgit pack-objects --quiet $objdir/pack/pack) &&\n -+\n -+\t\tgit multi-pack-index write \\\n -+\t\t\t--preferred-pack=\"pack-$moderate.idx\" &&\n -+\n -+\t\t# Retire the moderate pack; the stale midx still names it as the\n -+\t\t# owner of both blobs, each of which survives in the big pack.\n -+\t\trm -f $objdir/pack/pack-$moderate.* &&\n -+\n -+\t\t# One resident QUICK reader (\"git mktree --batch\") resolves both\n -+\t\t# blobs. The first lookup recovers d1 and caches the owning\n -+\t\t# packs failure; unless that failure keeps re-arming the second\n -+\t\t# read, the lookup of d2 skips its recovering read and the reader\n -+\t\t# dies reporting d2 as missing.\n -+\t\tprintf \"100644 blob %s\\tf1\\n\\n100644 blob %s\\tf2\\n\\n\" \\\n -+\t\t\t\"$d1\" \"$d2\" |\n -+\t\t\tgit mktree --batch >trees &&\n -+\t\ttest_line_count = 2 trees\n -+\t)\n -+'\n +\n test_done\n\n-- \ngitgitgadget\n"},{"id":"551452","messageId":"36bf2ce17be1a4da1ba92d5eb89ce49c7e00be9d.1787986831.git.gitgitgadget@gmail.com","threadId":"66192","inReplyTo":"pull.2207.v3.git.1787986831.gitgitgadget@gmail.com","subject":"[PATCH v3 1/4] replay: fail gracefully when a merge input is unreadable","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-29T07:00:28Z","receivedAt":"2026-08-29T07:00:37Z","isPatch":true,"body":"From: Elijah Newren \n\nWhen objects involved in the merge cannot be read, the merge machinery\nwill return early with result.clean = -1, and result.tree left as NULL.\npick_regular_commit() tested only \"if (!result->clean)\", ignoring the\ncase where \"clean < 0\". That causes the code to try to use\nresult->tree, resulting in a SIGSEGV.\n\nHandle clean < 0 explicitly; the merge machinery will already have printed\nmessages such as \"Could not read \" and \"collecting merge info\nfailed for trees...\", so we don't need to add much detail beyond the\nfact that the merge failed.\n\nSigned-off-by: Elijah Newren \n---\n replay.c | 7 +++++++\n t/t3650-replay-basics.sh | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 41 insertions(+)\n\ndiff --git a/replay.c b/replay.c\nindex 463c900d6c..33e21b2032 100644\n--- a/replay.c\n+++ b/replay.c\n@@ -327,6 +327,13 @@ static struct commit *pick_regular_commit(struct repository *repo,\n \tmerge_opt->ancestor = NULL;\n \tmerge_opt->branch2 = NULL;\n \n+\tif (result->clean < 0) {\n+\t\terror(_(\"merge of %s onto %s failed\"),\n+\t\t oid_to_hex(&pickme->object.oid),\n+\t\t oid_to_hex(&replayed_base->object.oid));\n+\t\treturn NULL;\n+\t}\n+\n \tif (!result->clean)\n \t\treturn NULL;\n \ndiff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh\nindex 3353bc4a4d..12348b4a5f 100755\n--- a/t/t3650-replay-basics.sh\n+++ b/t/t3650-replay-basics.sh\n@@ -565,4 +565,38 @@ test_expect_success '--onto with --ref rejects multiple revision ranges' '\n \ttest_grep \"cannot be used with multiple revision ranges\" err\n '\n \n+test_expect_success 'replay fails without segfault when objects are missing' '\n+\ttest_when_finished \"rm -fr unreadable\" &&\n+\tgit init unreadable &&\n+\t(\n+\t\tcd unreadable &&\n+\n+\t\ttest_write_lines l1 l2 l3 l4 l5 l6 l7 l8 >f &&\n+\t\tgit add f &&\n+\t\tgit commit -m base &&\n+\t\tgit branch base &&\n+\n+\t\ttest_write_lines l1 l2 l3 l4 l5 l6 l7 CHANGED >f &&\n+\t\tgit commit -am side &&\n+\t\tgit branch side &&\n+\n+\t\tgit switch -c onto base &&\n+\t\ttest_write_lines CHANGED l2 l3 l4 l5 l6 l7 l8 >f &&\n+\t\tgit commit -am onto &&\n+\n+\t\t# The replay works while every object is readable.\n+\t\tgit replay --onto onto base..side &&\n+\n+\t\t# Removing the onto tree makes parse_tree() fail during the\n+\t\t# incore merge, driving clean < 0 with a NULL result tree.\n+\t\tonto_tree=$(git rev-parse onto^{tree}) &&\n+\t\tobj=$(test_oid_to_path \"$onto_tree\") &&\n+\t\tmv .git/objects/${obj} saved-tree &&\n+\n+\t\t# Ensure replay gracefully handles the missing object\n+\t\ttest_must_fail git replay --onto onto base..side 2>err &&\n+\t\ttest_grep -e \"Could not read\" -e \"collecting merge info failed\" err\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"551453","messageId":"3f3b75690eea02960c7edc8d318ce7dff654f1bc.1787986831.git.gitgitgadget@gmail.com","threadId":"66192","inReplyTo":"pull.2207.v3.git.1787986831.gitgitgadget@gmail.com","subject":"[PATCH v3 2/4] mktree: plug per-tree leak in --batch mode","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-29T07:00:29Z","receivedAt":"2026-08-29T07:00:39Z","isPatch":true,"body":"From: Elijah Newren \n\nIn --batch mode \"git mktree\" reuses its entry buffer across trees,\nresetting `used` to 0 after writing each tree. It never frees the\n`treeent` structures the previous tree appended, though, so once the\nnext tree overwrites those slots the earlier allocations are leaked. A\nsingle-tree invocation hides this, as the entries stay reachable through\nthe `entries` global until exit.\n\nFree each entry when resetting the buffer, and free the buffer itself\nbefore returning.\n\nSigned-off-by: Elijah Newren \n---\n builtin/mktree.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/builtin/mktree.c b/builtin/mktree.c\nindex 4084e32476..dc2d293c3d 100644\n--- a/builtin/mktree.c\n+++ b/builtin/mktree.c\n@@ -200,8 +200,11 @@ int cmd_mktree(int ac,\n \t\t\tputs(oid_to_hex(&oid));\n \t\t\tfflush(stdout);\n \t\t}\n+\t\tfor (int i = 0; i < used; i++)\n+\t\t\tfree(entries[i]);\n \t\tused=0; /* reset tree entry buffer for re-use in batch mode */\n \t}\n+\tfree(entries);\n \tstrbuf_release(&sb);\n \n \treturn 0;\n-- \ngitgitgadget\n\n"},{"id":"551454","messageId":"79ce753c6849651cb7497c5e7716f0f1068df4ad.1787986831.git.gitgitgadget@gmail.com","threadId":"66192","inReplyTo":"pull.2207.v3.git.1787986831.gitgitgadget@gmail.com","subject":"[PATCH v3 3/4] mktree: do not use OBJECT_INFO_QUICK when checking objects","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-29T07:00:30Z","receivedAt":"2026-08-29T07:00:43Z","isPatch":true,"body":"From: Elijah Newren \n\nmktree_line() checks each referenced object's type with\nodb_read_object_info_extended() under OBJECT_INFO_QUICK. QUICK skips the\nreprepare-and-retry that reloads the on-disk pack set, so a resident\n\"git mktree --batch\" reader reports an object that a concurrent repack\njust relocated into a new pack as missing, and rejects the entry.\n\nQUICK entered this lookup in 817b0f602710 (mktree: do not check type of\nremote objects, 2022-06-21) only to avoid lazily fetching promisor\nobjects; OBJECT_INFO_SKIP_FETCH_OBJECT already provides that. Drop\nOBJECT_INFO_QUICK and keep OBJECT_INFO_SKIP_FETCH_OBJECT, so mktree still\navoids a promisor fetch but recovers an object that was merely repacked.\n\nAdd a regression test driving a resident mktree --batch reader across a\nconcurrent repack that retires a pack.\n\nAssisted-by: Claude Opus 4.8 & GPT-5.6 Sol\nSigned-off-by: Elijah Newren \n---\n builtin/mktree.c | 1 -\n t/t1010-mktree.sh | 48 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/mktree.c b/builtin/mktree.c\nindex dc2d293c3d..45ae2af3b5 100644\n--- a/builtin/mktree.c\n+++ b/builtin/mktree.c\n@@ -125,7 +125,6 @@ static void mktree_line(struct repository *repo, char *buf, int nul_term_line, i\n \toi.typep = &obj_type;\n \tif (odb_read_object_info_extended(repo->objects, &oid, &oi,\n \t\t\t\t\t OBJECT_INFO_LOOKUP_REPLACE |\n-\t\t\t\t\t OBJECT_INFO_QUICK |\n \t\t\t\t\t OBJECT_INFO_SKIP_FETCH_OBJECT) < 0)\n \t\tobj_type = -1;\n \ndiff --git a/t/t1010-mktree.sh b/t/t1010-mktree.sh\nindex 312fe6717a..cecba55d45 100755\n--- a/t/t1010-mktree.sh\n+++ b/t/t1010-mktree.sh\n@@ -69,4 +69,52 @@ test_expect_success 'mktree refuses to read ls-tree -r output (2)' '\n \ttest_must_fail git mktree pack-a &&\n+\t\techo \"$b\" | git pack-objects .git/objects/pack/pack >pack-b &&\n+\n+\t\t# Drop the loose copies so the blobs resolve only through the\n+\t\t# packs the multi-pack-index names.\n+\t\tgit prune-packed &&\n+\t\tgit multi-pack-index write &&\n+\t\tprintf \"100644 blob %s\\ta\\n\" \"$a\" >tree-a &&\n+\t\tprintf \"100644 blob %s\\tb\\n\" \"$b\" >tree-b &&\n+\n+\t\tvictim=\".git/objects/pack/pack-$(cat pack-b)\" &&\n+\t\tmkfifo in out &&\n+\n+\t\t# mktree --batch stays resident, so its pack view predates the\n+\t\t# repack below; feed it one tree at a time over a fifo. The\n+\t\t# subshell exit closes the fifos, letting mktree see EOF and quit.\n+\t\t(git mktree --batch out 2>err &) &&\n+\t\texec 9>in &&\n+\t\texec 8&9 && echo >&9 && read tree_a <&8 &&\n+\n+\t\t# Mimic a concurrent repack: a replacement pack holds every\n+\t\t# object, and the pack for b loses its .idx (its .pack lingers),\n+\t\t# matching the order in which unlink_pack_path() removes files.\n+\t\tgit cat-file --batch-all-objects --batch-check=\"%(objectname)\" >oids &&\n+\t\tgit pack-objects .git/objects/pack/pack /dev/null &&\n+\t\trm -f \"$victim.idx\" &&\n+\n+\t\t# Resolving b used to fail, as its QUICK lookup accepted the\n+\t\t# miss; without QUICK the reader repreps and finds b in the\n+\t\t# replacement pack.\n+\t\tcat tree-b >&9 && echo >&9 && read tree_b <&8 &&\n+\t\texec 9>&- &&\n+\n+\t\ttest -n \"$tree_b\"\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"551455","messageId":"9b0966df9a060df215d8aec7816875d42651d5bb.1787986831.git.gitgitgadget@gmail.com","threadId":"66192","inReplyTo":"pull.2207.v3.git.1787986831.gitgitgadget@gmail.com","subject":"[PATCH v3 4/4] packfile: recover when a multi-pack-index names a removed pack","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-29T07:00:31Z","receivedAt":"2026-08-29T07:00:46Z","isPatch":true,"body":"From: Elijah Newren \n\nA geometric repack writes a new pack and multi-pack-index and then\ndeletes the packs the new one subsumes. A process still using the\nprevious MIDX keeps seeing a removed pack listed as the owner of some\nobjects. Since a MIDX attributes each object to exactly one pack, such\nan object is served only through its recorded owner; if that owner was\njust removed, find_pack_entry() cannot serve it -- the MIDX lookup routes\nto the missing pack, and the regular pack fallback deliberately skips\nevery MIDX-covered pack, so a surviving copy in another covered pack\n(e.g. a kept base pack) is never consulted.\n\nUnlike the ordinary \"a pack's .idx is mapped but its .pack is gone\"\nrace, the second read does not rescue us. Reloading the on-disk pack set\ndoes not reload the borrowed, cached MIDX (freeing it under the code that\ncaches the \"struct multi_pack_index *\" would be a use-after-free), so the\nstale MIDX keeps routing to the removed pack and the surviving copy stays\nhidden behind the covered-pack skip. cat-file, rev-list and pack-objects\ncan thus all spuriously fail with \"unable to read object\".\n\nTeach find_pack_entry() to recover. The MIDX lookup now returns a\ntri-state, distinguishing an object absent from the MIDX from one it owns\nvia a pack that can no longer be opened; in the latter case, once the\nregular fallback has also missed, scan the MIDX's packs directly for a\nsurviving copy. Because the return value is no longer a boolean, rename\nfill_midx_entry() to midx_fill_entry() so callers must reckon with the\nnew enum rather than silently treat MIDX_FILL_OWNER_UNAVAILABLE as a hit.\n\nDo the scan only on the second read (OBJECT_INFO_SECOND_READ): by then\nthe cheaper on-disk reload has run, so an object merely relocated into a\nnew (uncovered) pack has already been found by the regular fallback, and\nonly a genuine hidden duplicate reaches the rescan. A QUICK caller that\nskips the second read simply accepts the false negative, as QUICK is\ndesigned to.\n\nReloading the stale MIDX would be a more complete fix but is much more\ninvolved (the borrowers above need proper invalidation), so leave that\nfor later.\n\nAssisted-by: Claude Opus 4.8 & GPT-5.6 Sol\nHelped-by: Jeff King \nSigned-off-by: Elijah Newren \n---\n builtin/pack-objects.c | 2 +-\n midx.c | 20 +++++++++---------\n midx.h | 21 +++++++++++++++++--\n odb/source-packed.c | 42 ++++++++++++++++++++++++++++++++-----\n t/helper/test-read-midx.c | 2 +-\n t/t5319-multi-pack-index.sh | 40 +++++++++++++++++++++++++++++++++++\n 6 files changed, 108 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 399acd0f22..751d5d3449 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1786,7 +1786,7 @@ static int want_object_in_pack_mtime(const struct object_id *oid,\n \t\tstruct multi_pack_index *m = get_multi_pack_index(files->packed);\n \t\tstruct pack_entry e;\n \n-\t\tif (m && fill_midx_entry(m, oid, &e, NULL)) {\n+\t\tif (m && midx_fill_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {\n \t\t\twant = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\t\tif (want != -1)\n \t\t\t\treturn want;\ndiff --git a/midx.c b/midx.c\nindex 37f082dbdd..6d1c548e3d 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -589,23 +589,23 @@ uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos)\n \t\t\t\t\t (off_t)pos * MIDX_CHUNK_OFFSET_WIDTH);\n }\n \n-int fill_midx_entry(struct multi_pack_index *m,\n-\t\t const struct object_id *oid,\n-\t\t struct pack_entry *e,\n-\t\t struct packed_git **bad_pack)\n+enum midx_fill_result midx_fill_entry(struct multi_pack_index *m,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t struct pack_entry *e,\n+\t\t\t\t struct packed_git **bad_pack)\n {\n \tuint32_t pos;\n \tuint32_t pack_int_id;\n \tstruct packed_git *p;\n \n \tif (!bsearch_midx(oid, m, &pos))\n-\t\treturn 0;\n+\t\treturn MIDX_FILL_MISS;\n \n \tmidx_for_object(&m, pos);\n \tpack_int_id = nth_midxed_pack_int_id(m, pos);\n \n \tif (prepare_midx_pack(m, pack_int_id))\n-\t\treturn 0;\n+\t\treturn MIDX_FILL_OWNER_UNAVAILABLE;\n \tp = m->packs[pack_int_id - m->num_packs_in_base];\n \n \t/*\n@@ -616,19 +616,19 @@ int fill_midx_entry(struct multi_pack_index *m,\n \t* loaded!\n \t*/\n \tif (!is_pack_valid(p))\n-\t\treturn 0;\n+\t\treturn MIDX_FILL_OWNER_UNAVAILABLE;\n \n \tif (oidset_size(&p->bad_objects) &&\n \t oidset_contains(&p->bad_objects, oid)) {\n \t\tif (bad_pack && !*bad_pack)\n \t\t\t*bad_pack = p;\n-\t\treturn 0;\n+\t\treturn MIDX_FILL_MISS;\n \t}\n \n \te->offset = nth_midxed_offset(m, pos);\n \te->p = p;\n \n-\treturn 1;\n+\treturn MIDX_FILL_HIT;\n }\n \n /* Match \"foo.idx\" against either \"foo.pack\" _or_ \"foo.idx\". */\n@@ -1032,7 +1032,7 @@ int verify_midx_file(struct odb_source_packed *source, unsigned flags)\n \n \t\tnth_midxed_object_oid(&oid, m, pairs[i].pos);\n \n-\t\tif (!fill_midx_entry(m, &oid, &e, NULL)) {\n+\t\tif (midx_fill_entry(m, &oid, &e, NULL) != MIDX_FILL_HIT) {\n \t\t\tmidx_report(_(\"failed to load pack entry for oid[%d] = %s\"),\n \t\t\t\t pairs[i].pos, oid_to_hex(&oid));\n \t\t\tcontinue;\ndiff --git a/midx.h b/midx.h\nindex 1f2f2d5321..4b768769b9 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -117,8 +117,25 @@ uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos);\n struct object_id *nth_midxed_object_oid(struct object_id *oid,\n \t\t\t\t\tstruct multi_pack_index *m,\n \t\t\t\t\tuint32_t n);\n-int fill_midx_entry(struct multi_pack_index *m, const struct object_id *oid,\n-\t\t struct pack_entry *e, struct packed_git **bad_pack);\n+/*\n+ * Result of looking an object up in a multi-pack-index. MIDX_FILL_HIT means\n+ * \"e was filled in\"; the two miss variants distinguish an object the midx does\n+ * not know about (MIDX_FILL_MISS) from one it does know about but whose owning\n+ * pack we can no longer open (MIDX_FILL_OWNER_UNAVAILABLE -- the signature of a\n+ * concurrent repack having removed that pack). A known-bad (corrupt) object\n+ * reports MIDX_FILL_MISS but also sets *bad_pack, if provided, to the owning\n+ * pack so the caller can tell \"corrupt\" apart from \"absent\".\n+ */\n+enum midx_fill_result {\n+\tMIDX_FILL_MISS = 0,\n+\tMIDX_FILL_HIT,\n+\tMIDX_FILL_OWNER_UNAVAILABLE,\n+};\n+\n+enum midx_fill_result midx_fill_entry(struct multi_pack_index *m,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t struct pack_entry *e,\n+\t\t\t\t struct packed_git **bad_pack);\n int midx_contains_pack(struct multi_pack_index *m,\n \t\t const char *idx_or_pack_name);\n int midx_layer_contains_pack(struct multi_pack_index *m,\ndiff --git a/odb/source-packed.c b/odb/source-packed.c\nindex 1a12a605db..90d88c0a12 100644\n--- a/odb/source-packed.c\n+++ b/odb/source-packed.c\n@@ -17,13 +17,18 @@\n static int find_pack_entry(struct odb_source_packed *store,\n \t\t\t const struct object_id *oid,\n \t\t\t struct pack_entry *e,\n+\t\t\t enum object_info_flags flags,\n \t\t\t struct packed_git **bad_pack)\n {\n \tstruct packfile_list_entry *l;\n+\tenum midx_fill_result midx_result = MIDX_FILL_MISS;\n \n \todb_source_prepare(&store->base, 0);\n-\tif (store->midx && fill_midx_entry(store->midx, oid, e, bad_pack))\n-\t\treturn 1;\n+\tif (store->midx) {\n+\t\tmidx_result = midx_fill_entry(store->midx, oid, e, bad_pack);\n+\t\tif (midx_result == MIDX_FILL_HIT)\n+\t\t\treturn 1;\n+\t}\n \n \tfor (l = store->packs.head; l; l = l->next) {\n \t\tstruct packed_git *p = l->pack;\n@@ -35,6 +40,33 @@ static int find_pack_entry(struct odb_source_packed *store,\n \t\t}\n \t}\n \n+\t/*\n+\t * Recovery for a concurrent-repack race: a stale MIDX may still name a\n+\t * vanished owning pack even though the object survives in another pack\n+\t * the same MIDX covers. The regular fallback above skips MIDX-covered\n+\t * packs, and repreparing the on-disk pack set does not reload the\n+\t * borrowed, cached MIDX, so scan its packs directly for the survivor.\n+\t *\n+\t * Do this only on the second read, by which point repreparing packs has\n+\t * already had a chance to find an object merely relocated into a new,\n+\t * uncovered pack; only a genuine hidden duplicate reaches here.\n+\t */\n+\tif (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&\n+\t (flags & OBJECT_INFO_SECOND_READ)) {\n+\t\tstruct multi_pack_index *m = store->midx;\n+\t\tuint32_t i;\n+\n+\t\tfor (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {\n+\t\t\tstruct packed_git *p;\n+\n+\t\t\tif (prepare_midx_pack(m, i))\n+\t\t\t\tcontinue;\n+\t\t\tp = nth_midxed_pack(m, i);\n+\t\t\tif (p && packfile_fill_entry(p, oid, e, bad_pack))\n+\t\t\t\treturn 1;\n+\t\t}\n+\t}\n+\n \treturn 0;\n }\n \n@@ -57,7 +89,7 @@ static enum odb_read_status odb_source_packed_read_object_info(struct odb_source\n \tif (flags & OBJECT_INFO_SECOND_READ)\n \t\todb_source_prepare(source, ODB_PREPARE_FLUSH_CACHES);\n \n-\tif (!find_pack_entry(packed, oid, &e, &bad_pack)) {\n+\tif (!find_pack_entry(packed, oid, &e, flags, &bad_pack)) {\n \t\t/*\n \t\t * The lookup may have failed because the object is known to be\n \t\t * corrupt in one of the packfiles. Report the object as\n@@ -105,7 +137,7 @@ static int odb_source_packed_read_object_stream(struct odb_read_stream **out,\n \tstruct odb_source_packed *packed = odb_source_packed_downcast(source);\n \tstruct pack_entry e;\n \n-\tif (!find_pack_entry(packed, oid, &e, NULL))\n+\tif (!find_pack_entry(packed, oid, &e, 0, NULL))\n \t\treturn -1;\n \n \treturn packfile_read_object_stream(out, oid, e.p, e.offset);\n@@ -611,7 +643,7 @@ static int odb_source_packed_freshen_object(struct odb_source *source,\n \t\ttimesp = ×\n \t}\n \n-\tif (!find_pack_entry(packed, oid, &e, NULL))\n+\tif (!find_pack_entry(packed, oid, &e, 0, NULL))\n \t\treturn 0;\n \tif (e.p->is_cruft)\n \t\treturn 0;\ndiff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c\nindex 27a05da957..9c5e308761 100644\n--- a/t/helper/test-read-midx.c\n+++ b/t/helper/test-read-midx.c\n@@ -82,7 +82,7 @@ static int read_midx_file(const char *object_dir, const char *checksum,\n \t\tfor (i = 0; i < m->num_objects; i++) {\n \t\t\tnth_midxed_object_oid(&oid, m,\n \t\t\t\t\t i + m->num_objects_in_base);\n-\t\t\tfill_midx_entry(m, &oid, &e, NULL);\n+\t\t\tmidx_fill_entry(m, &oid, &e, NULL);\n \n \t\t\tprintf(\"%s %\"PRIu64\"\\t%s\\n\",\n \t\t\t oid_to_hex(&oid), e.offset, e.p->pack_name);\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 68143cb5b7..2b8ff6f3ed 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -1393,4 +1393,44 @@ test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '\n \t)\n '\n \n+test_expect_success 'lookup recovers object whose midx-owning pack was removed' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t# \"keep\" ends up only in the big pack; \"dup\" is deliberately\n+\t\t# placed in two packs so the midx has to choose an owner.\n+\t\ttest_commit keep &&\n+\t\techo duplicated-content >dup &&\n+\t\tgit add dup &&\n+\t\tgit commit -m dup &&\n+\t\tdup_oid=$(git rev-parse HEAD:dup) &&\n+\n+\t\t# Roll every object, including dup, into a single big pack.\n+\t\tgit repack -adq &&\n+\n+\t\t# Build a second, \"moderate\" pack that also contains dup, so dup\n+\t\t# now lives in two packs that the midx will cover.\n+\t\tmoderate=$(echo \"$dup_oid\" |\n+\t\t\tgit pack-objects --quiet $objdir/pack/pack) &&\n+\n+\t\t# Attribute dup to the moderate pack in the midx.\n+\t\tgit multi-pack-index write \\\n+\t\t\t--preferred-pack=\"pack-$moderate.idx\" &&\n+\n+\t\t# Simulate a concurrent \"git repack\" retiring the moderate pack:\n+\t\t# its files disappear, but the now-stale midx still names it as\n+\t\t# the owner of dup. A valid copy of dup survives in the big pack.\n+\t\trm -f $objdir/pack/pack-$moderate.* &&\n+\n+\t\t# The midx routes the lookup to the deleted pack, and the regular\n+\t\t# pack fallback skips midx-covered packs, so without recovery dup\n+\t\t# would appear missing even though it is physically present.\n+\t\techo blob >expect &&\n+\t\tgit cat-file -t \"$dup_oid\" >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"551458","messageId":"20260829113257.GC40814@coredump.intra.peff.net","threadId":"66192","inReplyTo":"CABPp-BEmReAR-f-aweM=f=5QhRPxG1K-KLTsbyRt2aDQD_QnVA@mail.gmail.com","subject":"Re: [PATCH v2 3/4] packfile: recover object lookups racing a concurrent repack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-29T11:32:57Z","receivedAt":"2026-08-29T11:32:59Z","isPatch":true,"body":"On Thu, Aug 27, 2026 at 03:23:30PM -0700, Elijah Newren wrote:\n\n> It's far more likely that I am the one being dense. My rough line of thinking:\n> \n> * We see \"packfile ... index unavailable\" in our logging\n> * There's only one thing that remove packfiles\n> * Investigate the mechanism\n> * Look for other affected callers (e.g. mktree --batch)\n> * Consider corrective measures\n> \n> Steps 1-4 above are probably fine, and step 5 may have been where I\n> went off the rails. My thinking there, wrong or right, was:\n\nI think we should consider the log message independently from whether we\neventually return a value (whether QUICK or not). It seems like the log\nmessage is often unnecessarily scary, because we either recover via\nSECOND_READ, or we are in QUICK mode and the false negative is OK. So\nthe message is informative at best, and probably just noise in those\ncases.\n\nBut it perhaps _is_ helpful when a non-QUICK lookup ends up returning\nfailure. We'll end up with some other error() message, but it may be\nuseful context to know that we _thought_ we had the object available and\nthen the rug was pulled out from under us. But we don't have a good way\nof queuing up an error that is shown conditionally.\n\nSo I dunno. We could consider moving that message into trace/trace2,\nmaking it more of a \"debug\" message. And then people digging into a\nproblem can turn on traces. But I have a feeling that is not very\nhelpful, since it is mostly a racy situation (so you can't just easily\nreplay your failure with tracing turned on).\n\n> * It makes sense that we don't want to reprepare most of the time\n> * ...but _if_ we know of the existence of some specific packfile in\n> this process and that packfile has since disappeared by the time we go\n> to open or read it, is that a special case? Should it be?\n\nSo now we can consider the actual return value, aside from the logged\nmessage. For non-QUICK requests, I think this case is uninteresting (we\nalready do a reprepare and follow-up read). For mktree, I think the core\nof the problem is using QUICK when it should not.\n\nI think the current behavior of QUICK is _correct_, in the sense that\nfalse negatives are OK. But can we make it better? Possibly. To me the\nargument for this patch's direction is something like:\n\n The point of QUICK was to avoid lots of reprepare effort when we are\n looking up objects that we might reasonably not have. This has\n historically been about things like fetch speculatively looking for\n stuff the other side mentioned. But there we are mostly concerned\n about objects we _never_ had, and avoiding tons of reprepare work that\n will almost certainly not help us. But in some races, we might learn\n that we _did_ have the object at one point (because we opened its idx,\n or a midx) but the lookup still failed (because the pack couldn't be\n accessed).\n\n We can cheaply notice this case by differentiating true idx misses\n from failure to access the pack contents. And these items _are_ worth\n a reprepare, because they were almost certainly caused by a repacking\n race (or a true repo corruption or object pruning, but that is rare\n enough not to worry about for optimization purposes).\n\n So even though QUICK is not _wrong_ to say \"we do not have that\n object\", it is a good tradeoff to spend a little bit of time calling\n reprepare in order to produce fewer false negative \"no such object\"\n responses (because tools like fetch then have a chance to optimize\n their own task more as a result).\n\nMaybe that argument was somewhere in your original commit message. I\nadmit I got lost about half-way through. ;)\n\nBut I think the key thing is separating:\n\n - is the logging confusing or useful? What should we do about it?\n\n - is mktree racily broken because of QUICK? I think so.\n\n - even though QUICK is not wrong to skip the second read for this\n case, it might be a good tradeoff for it to detect and try harder\n here (i.e., the argument above).\n\nWhich sounds like three patches to me, each of which can be motivated\nand argued on its own.\n\n> > So I don't see QUICK itself here violating any contract (even if it\n> > _could_ find the object in some cases with just a little more work, as\n> > in the case that we were discussing for v1).\n> \n> I'll drop this patch and instead send a small mktree change that stops\n> passing OBJECT_INFO_QUICK (keeping SKIP_FETCH_OBJECT), so mktree\n> recovers via the normal reprepare like every other non-QUICK reader.\n> That removes the packfile.c changes entirely, so both the\n> reload-under-QUICK hack and the .idx/.pack raciness you noted in\n> pack_index_is_missing() go away with them.\n\nI am also happy with this direction. Then we can consider the other\nquestions separately (or not at all if nobody cares enough).\n\n-Peff\n"},{"id":"551459","messageId":"20260829113441.GD40814@coredump.intra.peff.net","threadId":"66192","inReplyTo":"CABPp-BFhPONjNuVZQfgwKuYdgbm5Fjjttz5q5wSYX6j1Zdwdww@mail.gmail.com","subject":"Re: [PATCH v2 4/4] packfile: recover when a multi-pack-index names a removed pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-29T11:34:41Z","receivedAt":"2026-08-29T11:34:43Z","isPatch":true,"body":"On Fri, Aug 28, 2026 at 12:29:49AM -0700, Elijah Newren wrote:\n\n> > We've changed the return value semantics without changing the signature\n> > (or name). So we need to make sure we adjust all callers, as here.\n> > That's _probably_ OK in practice for such a specialized function. But we\n> > could also rename it if we wanted to be paranoid (especially about\n> > new callers added on parallel branches).\n> \n> Any suggestions for alternate names? fill_midx_entry_result? midx_fill_entry?\n\nI did not have a good suggestion, but midx_fill_entry (which it looks\nlike your new series uses) seems reasonable. It is probably the better\nname anyway, as it fits the subsystem_verb_the_thing() ordering.\n\nI'll take a look at the new series and comment further there (if needed;\nmy fingers are crossed for perfection).\n\n-Peff\n"},{"id":"551460","messageId":"20260829114620.GE40814@coredump.intra.peff.net","threadId":"66192","inReplyTo":"79ce753c6849651cb7497c5e7716f0f1068df4ad.1787986831.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/4] mktree: do not use OBJECT_INFO_QUICK when checking objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-29T11:46:20Z","receivedAt":"2026-08-29T11:46:22Z","isPatch":true,"body":"On Sat, Aug 29, 2026 at 07:00:30AM +0000, Elijah Newren via GitGitGadget wrote:\n\n> mktree_line() checks each referenced object's type with\n> odb_read_object_info_extended() under OBJECT_INFO_QUICK. QUICK skips the\n> reprepare-and-retry that reloads the on-disk pack set, so a resident\n> \"git mktree --batch\" reader reports an object that a concurrent repack\n> just relocated into a new pack as missing, and rejects the entry.\n> \n> QUICK entered this lookup in 817b0f602710 (mktree: do not check type of\n> remote objects, 2022-06-21) only to avoid lazily fetching promisor\n> objects; OBJECT_INFO_SKIP_FETCH_OBJECT already provides that. Drop\n> OBJECT_INFO_QUICK and keep OBJECT_INFO_SKIP_FETCH_OBJECT, so mktree still\n> avoids a promisor fetch but recovers an object that was merely repacked.\n\nI think this line of reasoning is fine.\n\nWe probably _could_ use QUICK when the caller specified --missing, which\nwould optimize out the SECOND_READ effort if the caller told us they\nexpect (or at least allow) some items to be missing. But:\n\n 1. It's not clear how people use --missing. If you are just trying to\n be gentle with an occasional missing entry, then the optimization\n is not that interesting. If you run mktree all the time to make\n synthetic trees full of objects you don't have, then maybe you do\n care about the optimization. But if you are doing that then you\n probably are better off with an option that avoids the lookup\n entirely (i.e., we should just trust the type found in the input).\n\n So there's maybe room for a --yolo argument to mktree, though I\n guess in practice you could just use \"hash-object\" for that. But\n either way that is way out of scope for this patch.\n\n 2. Prior to 817b0f602710 we were not QUICK either! And that commit was\n only trying to trigger SKIP_FETCH_OBJECT. So whether there is an\n argument for linking --missing and QUICK or not, it should be made\n separately. This patch is just fixing the extra flag that probably\n should not have been added by 817b0f602710.\n\n> +test_expect_success PIPE 'mktree --batch survives a concurrent repack retiring a pack' '\n\nOK. I was hoping we could test this without all of the PIPE complexity,\nbut I don't think we can. We really need a case where the first lookup\nfails but SECOND_READ succeeds, which is inherently a race. Feeding one\nentry at a time lets us implement that in a deterministic way, and I\nthink is the simplest we can get.\n\nSo the patch looks good to me overall.\n\n-Peff\n"},{"id":"551461","messageId":"20260829120721.GF40814@coredump.intra.peff.net","threadId":"66192","inReplyTo":"9b0966df9a060df215d8aec7816875d42651d5bb.1787986831.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 4/4] packfile: recover when a multi-pack-index names a removed pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-29T12:07:21Z","receivedAt":"2026-08-29T12:07:22Z","isPatch":true,"body":"On Sat, Aug 29, 2026 at 07:00:31AM +0000, Elijah Newren via GitGitGadget wrote:\n\n> +\t/*\n> +\t * Recovery for a concurrent-repack race: a stale MIDX may still name a\n> +\t * vanished owning pack even though the object survives in another pack\n> +\t * the same MIDX covers. The regular fallback above skips MIDX-covered\n> +\t * packs, and repreparing the on-disk pack set does not reload the\n> +\t * borrowed, cached MIDX, so scan its packs directly for the survivor.\n> +\t *\n> +\t * Do this only on the second read, by which point repreparing packs has\n> +\t * already had a chance to find an object merely relocated into a new,\n> +\t * uncovered pack; only a genuine hidden duplicate reaches here.\n> +\t */\n> +\tif (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&\n> +\t (flags & OBJECT_INFO_SECOND_READ)) {\n> +\t\tstruct multi_pack_index *m = store->midx;\n> +\t\tuint32_t i;\n> +\n> +\t\tfor (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {\n> +\t\t\tstruct packed_git *p;\n> +\n> +\t\t\tif (prepare_midx_pack(m, i))\n> +\t\t\t\tcontinue;\n> +\t\t\tp = nth_midxed_pack(m, i);\n> +\t\t\tif (p && packfile_fill_entry(p, oid, e, bad_pack))\n> +\t\t\t\treturn 1;\n> +\t\t}\n> +\t}\n\nSo I think this workaround is fine to do (as long as we are not going to\nactually refresh the midx on SECOND_READ, which I agree is probably a\nbigger change).\n\nI always get confused about m->num_packs and m->num_packs_in_base, and\nwhether we are looking at the packs in a midx slice versus the whole\nthing. I _think_ what you have here is correct, because we are iterating\nfrom 0 up to the total number of packs, and prepare_midx_pack() etc will\nlook back through the incremental slices as necessary.\n\nBut I wonder if it would be simpler to just iterate over the actual pack\nlist in the usual way, since we already do that in this function. I\n_thought_ this would work:\n\ndiff --git a/odb/source-packed.c b/odb/source-packed.c\nindex 90d88c0a12..86e6a80d2f 100644\n--- a/odb/source-packed.c\n+++ b/odb/source-packed.c\n@@ -33,40 +33,19 @@ static int find_pack_entry(struct odb_source_packed *store,\n \tfor (l = store->packs.head; l; l = l->next) {\n \t\tstruct packed_git *p = l->pack;\n \n-\t\tif (!p->multi_pack_index && packfile_fill_entry(p, oid, e, bad_pack)) {\n+\t\t/* ...explain tricky race case here... */\n+\t\tif (p->multi_pack_index &&\n+\t\t (midx_result != MIDX_FILL_OWNER_UNAVAILABLE ||\n+\t\t !(flags & OBJECT_INFO_SECOND_READ)))\n+\t\t\tcontinue;\n+\n+\t\tif (packfile_fill_entry(p, oid, e, bad_pack)) {\n \t\t\tif (!store->skip_mru_updates)\n \t\t\t\tpackfile_list_prepend(&store->packs, p);\n \t\t\treturn 1;\n \t\t}\n \t}\n \n-\t/*\n-\t * Recovery for a concurrent-repack race: a stale MIDX may still name a\n-\t * vanished owning pack even though the object survives in another pack\n-\t * the same MIDX covers. The regular fallback above skips MIDX-covered\n-\t * packs, and repreparing the on-disk pack set does not reload the\n-\t * borrowed, cached MIDX, so scan its packs directly for the survivor.\n-\t *\n-\t * Do this only on the second read, by which point repreparing packs has\n-\t * already had a chance to find an object merely relocated into a new,\n-\t * uncovered pack; only a genuine hidden duplicate reaches here.\n-\t */\n-\tif (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&\n-\t (flags & OBJECT_INFO_SECOND_READ)) {\n-\t\tstruct multi_pack_index *m = store->midx;\n-\t\tuint32_t i;\n-\n-\t\tfor (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {\n-\t\t\tstruct packed_git *p;\n-\n-\t\t\tif (prepare_midx_pack(m, i))\n-\t\t\t\tcontinue;\n-\t\t\tp = nth_midxed_pack(m, i);\n-\t\t\tif (p && packfile_fill_entry(p, oid, e, bad_pack))\n-\t\t\t\treturn 1;\n-\t\t}\n-\t}\n-\n \treturn 0;\n }\n \n\nbut it doesn't because we don't always load the midx'd packs into the\npack list (we do it on-demand as they become useful to us). So I think\nyou'd essentially end up needing to do a loop like the one you have\nanyway to prepare_midx_pack() on them all.\n\nAnd we want to avoid doing that if we can find it outside the midx\n(since that was the whole point of waiting for SECOND_READ). Which would\nhappen...in that loop. So we really do want to have our own\nmidx-specific loop like you have here.\n\nSorry, I know that was a lot of text to end up at \"you have already\nwritten it the best way\", but it took me a while to reason through it.\n\nThe patch looks good to me. ;)\n\n-Peff\n"},{"id":"551485","messageId":"xmqqjyp71g9s.fsf@gitster.g","threadId":"66192","inReplyTo":"20260829120721.GF40814@coredump.intra.peff.net","subject":"Re: [PATCH v3 4/4] packfile: recover when a multi-pack-index names a removed pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-30T20:53:51Z","receivedAt":"2026-08-30T20:53:54Z","isPatch":true,"body":"Jeff King writes:\n\n> ...\n> Sorry, I know that was a lot of text to end up at \"you have already\n> written it the best way\", but it took me a while to reason through it.\n>\n> The patch looks good to me. ;)\n\nThanks for a very informative and well reasoned write-up in support\nof the series.\n\nShall we mark it for 'next' then?\n"},{"id":"551539","messageId":"apVa72AU-cD4IO46@pks.im","threadId":"66192","inReplyTo":"xmqqjyp71g9s.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] packfile: recover when a multi-pack-index names a removed pack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-31T10:43:59Z","receivedAt":"2026-08-31T10:44:08Z","isPatch":true,"body":"On Sun, Aug 30, 2026 at 01:53:51PM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > ...\n> > Sorry, I know that was a lot of text to end up at \"you have already\n> > written it the best way\", but it took me a while to reason through it.\n> >\n> > The patch looks good to me. ;)\n> \n> Thanks for a very informative and well reasoned write-up in support\n> of the series.\n> \n> Shall we mark it for 'next' then?\n\nHere's my a lot less well reasoned +1, for what it's worth. Thanks!\n\nPatrick\n"},{"id":"551609","messageId":"20260831231005.GA973618@coredump.intra.peff.net","threadId":"66192","inReplyTo":"xmqqjyp71g9s.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] packfile: recover when a multi-pack-index names a removed pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-31T23:10:05Z","receivedAt":"2026-08-31T23:10:12Z","isPatch":true,"body":"On Sun, Aug 30, 2026 at 01:53:51PM -0700, Junio C Hamano wrote:\n\n> Thanks for a very informative and well reasoned write-up in support\n> of the series.\n> \n> Shall we mark it for 'next' then?\n\nYeah, that sounds good to me.\n\n-Peff\n"},{"id":"551679","messageId":"944945ab-dde7-41e5-af92-fc520485fc53@gmail.com","threadId":"66192","inReplyTo":"9b0966df9a060df215d8aec7816875d42651d5bb.1787986831.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 4/4] packfile: recover when a multi-pack-index names a removed pack","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-09-01T15:26:25Z","receivedAt":"2026-09-01T15:26:29Z","isPatch":true,"body":"On 8/29/2026 3:00 AM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n\nI'm late in reviewing this patch, so forgive me responding inline as\nI discover how it works.\n\ntl;dr: Good patch. LGTM.\n\n> Teach find_pack_entry() to recover. The MIDX lookup now returns a\n> tri-state, distinguishing an object absent from the MIDX from one it owns\n> via a pack that can no longer be opened; in the latter case, once the\n> regular fallback has also missed, scan the MIDX's packs directly for a\n> surviving copy. Because the return value is no longer a boolean, rename\n> fill_midx_entry() to midx_fill_entry() so callers must reckon with the\n> new enum rather than silently treat MIDX_FILL_OWNER_UNAVAILABLE as a hit.\n\nThis tri-state is valuable!\n \n> Do the scan only on the second read (OBJECT_INFO_SECOND_READ): by then\n> the cheaper on-disk reload has run, so an object merely relocated into a\n> new (uncovered) pack has already been found by the regular fallback, and\n> only a genuine hidden duplicate reaches the rescan. A QUICK caller that\n> skips the second read simply accepts the false negative, as QUICK is\n> designed to.\n> \n> Reloading the stale MIDX would be a more complete fix but is much more\n> involved (the borrowers above need proper invalidation), so leave that\n> for later.\n\n> -\t\tif (m && fill_midx_entry(m, oid, &e, NULL)) {\n> +\t\tif (m && midx_fill_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {\n\nOne major benefit to the rename is that we can guarantee that\nall callers are updated to reflect the new tri-state response.\n\nIt also has a better naming convention, overall.\n\n(reordered header file diff up)\n> +/*\n> + * Result of looking an object up in a multi-pack-index. MIDX_FILL_HIT means\n> + * \"e was filled in\"; the two miss variants distinguish an object the midx does\n> + * not know about (MIDX_FILL_MISS) from one it does know about but whose owning\n> + * pack we can no longer open (MIDX_FILL_OWNER_UNAVAILABLE -- the signature of a\n> + * concurrent repack having removed that pack). A known-bad (corrupt) object\n> + * reports MIDX_FILL_MISS but also sets *bad_pack, if provided, to the owning\n> + * pack so the caller can tell \"corrupt\" apart from \"absent\".\n> + */\n> +enum midx_fill_result {\n> +\tMIDX_FILL_MISS = 0,\n> +\tMIDX_FILL_HIT,\n> +\tMIDX_FILL_OWNER_UNAVAILABLE,\n> +};\n> +\n> +enum midx_fill_result midx_fill_entry(struct multi_pack_index *m,\n> +\t\t\t\t const struct object_id *oid,\n> +\t\t\t\t struct pack_entry *e,\n> +\t\t\t\t struct packed_git **bad_pack);\n\nThis is good documentation that will help future uses know how to\nreact to the different modes.\n\n> -int fill_midx_entry(struct multi_pack_index *m,\n> -\t\t const struct object_id *oid,\n> -\t\t struct pack_entry *e,\n> -\t\t struct packed_git **bad_pack)\n> +enum midx_fill_result midx_fill_entry(struct multi_pack_index *m,\n> +\t\t\t\t const struct object_id *oid,\n> +\t\t\t\t struct pack_entry *e,\n> +\t\t\t\t struct packed_git **bad_pack)\n> {\n> \tuint32_t pos;\n> \tuint32_t pack_int_id;\n> \tstruct packed_git *p;\n> \n> \tif (!bsearch_midx(oid, m, &pos))\n> -\t\treturn 0;\n> +\t\treturn MIDX_FILL_MISS;\n\nObviously correct: this OID isn't in the sorted list.\n\n> \tmidx_for_object(&m, pos);\n> \tpack_int_id = nth_midxed_pack_int_id(m, pos);\n> \n> \tif (prepare_midx_pack(m, pack_int_id))\n> -\t\treturn 0;\n> +\t\treturn MIDX_FILL_OWNER_UNAVAILABLE;\n\nObviously correct: we tried to open the pack index but failed.\n\n> \tp = m->packs[pack_int_id - m->num_packs_in_base];\n> \n> \t/*\n> @@ -616,19 +616,19 @@ int fill_midx_entry(struct multi_pack_index *m,\n> \t* loaded!\n> \t*/\n> \tif (!is_pack_valid(p))\n> -\t\treturn 0;\n> +\t\treturn MIDX_FILL_OWNER_UNAVAILABLE;\n\nSame: Pack is invalid somehow, likely that the .pack disappeared.\n\n> \tif (oidset_size(&p->bad_objects) &&\n> \t oidset_contains(&p->bad_objects, oid)) {\n> \t\tif (bad_pack && !*bad_pack)\n> \t\t\t*bad_pack = p;\n> -\t\treturn 0;\n> +\t\treturn MIDX_FILL_MISS;\n\nThis one is tricky, but makes sense: we have marked this as a\n\"bad\" object so we should act like it doesn't exist. Good.\n\n> \t}\n> \n> \te->offset = nth_midxed_offset(m, pos);\n> \te->p = p;\n> \n> -\treturn 1;\n> +\treturn MIDX_FILL_HIT;\n\nfinally: success!> }\n\n\n> static int find_pack_entry(struct odb_source_packed *store,\n> \t\t\t const struct object_id *oid,\n> \t\t\t struct pack_entry *e,\n> +\t\t\t enum object_info_flags flags,\n> \t\t\t struct packed_git **bad_pack)\n> {\n> \tstruct packfile_list_entry *l;\n> +\tenum midx_fill_result midx_result = MIDX_FILL_MISS;\n> \n> \todb_source_prepare(&store->base, 0);\n> -\tif (store->midx && fill_midx_entry(store->midx, oid, e, bad_pack))\n> -\t\treturn 1;\n> +\tif (store->midx) {\n> +\t\tmidx_result = midx_fill_entry(store->midx, oid, e, bad_pack);\n> +\t\tif (midx_result == MIDX_FILL_HIT)\n> +\t\t\treturn 1;\n> +\t}\n\nThis looks good. On a hit, we return. Act like a MIDX-miss if we\ndon't have a midx.\n\nOutside of the patch context is the \"reprepare packfiles\" to pick\nup a copy from a packfile that doesn't exist within the current\n(stale) midx.\n> +\t/*\n> +\t * Recovery for a concurrent-repack race: a stale MIDX may still name a\n> +\t * vanished owning pack even though the object survives in another pack\n> +\t * the same MIDX covers. The regular fallback above skips MIDX-covered\n> +\t * packs, and repreparing the on-disk pack set does not reload the\n> +\t * borrowed, cached MIDX, so scan its packs directly for the survivor.\n> +\t *\n> +\t * Do this only on the second read, by which point repreparing packs has\n> +\t * already had a chance to find an object merely relocated into a new,\n> +\t * uncovered pack; only a genuine hidden duplicate reaches here.\n> +\t */\n\nThis comment does a lot of important context-setting to show\nthat we are in a very narrow case: the stale MIDX has multiple\npacks that contain the requested object, but the \"newer\" one\nwas deleted without creating a new packfile, so we need to\nlook at each contained pack for the object from its pack-index.\n\n> +\tif (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&\n> +\t (flags & OBJECT_INFO_SECOND_READ)) {\n> +\t\tstruct multi_pack_index *m = store->midx;\n> +\t\tuint32_t i;\n> +\n> +\t\tfor (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {\n> +\t\t\tstruct packed_git *p;\n> +\n> +\t\t\tif (prepare_midx_pack(m, i))\n> +\t\t\t\tcontinue;\n> +\t\t\tp = nth_midxed_pack(m, i);\n> +\t\t\tif (p && packfile_fill_entry(p, oid, e, bad_pack))\n> +\t\t\t\treturn 1;\n> +\t\t}\n> +\t}\n> +\n\nThis is hopefully a very rare case, but it's good to have\nthis \"fall back to O(num packs)\" situation.\n\n> +test_expect_success 'lookup recovers object whose midx-owning pack was removed' '\n> +\ttest_when_finished \"rm -fr repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\t# \"keep\" ends up only in the big pack; \"dup\" is deliberately\n> +\t\t# placed in two packs so the midx has to choose an owner.\n> +\t\ttest_commit keep &&\n> +\t\techo duplicated-content >dup &&\n> +\t\tgit add dup &&\n> +\t\tgit commit -m dup &&\n> +\t\tdup_oid=$(git rev-parse HEAD:dup) &&\n> +\n> +\t\t# Roll every object, including dup, into a single big pack.\n> +\t\tgit repack -adq &&\n> +\n> +\t\t# Build a second, \"moderate\" pack that also contains dup, so dup\n> +\t\t# now lives in two packs that the midx will cover.\n> +\t\tmoderate=$(echo \"$dup_oid\" |\n> +\t\t\tgit pack-objects --quiet $objdir/pack/pack) &&\n> +\n> +\t\t# Attribute dup to the moderate pack in the midx.\n> +\t\tgit multi-pack-index write \\\n> +\t\t\t--preferred-pack=\"pack-$moderate.idx\" &&\n\nThis use of preferred pack is a good way of getting around mtimes\nthat could be equal. We could also consider updating mtimes, but\nthis works so don't change it.\n\n> +\t\t# Simulate a concurrent \"git repack\" retiring the moderate pack:\n> +\t\t# its files disappear, but the now-stale midx still names it as\n> +\t\t# the owner of dup. A valid copy of dup survives in the big pack.\n> +\t\trm -f $objdir/pack/pack-$moderate.* &&\n> +\n> +\t\t# The midx routes the lookup to the deleted pack, and the regular\n> +\t\t# pack fallback skips midx-covered packs, so without recovery dup\n> +\t\t# would appear missing even though it is physically present.\n> +\t\techo blob >expect &&\n> +\t\tgit cat-file -t \"$dup_oid\" >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\nThanks for adding this test so we can keep this narrow case\nworking in perpetuity.\n\nThanks,\n-Stolee\n\n"},{"id":"551680","messageId":"374bffe1-47ff-4cb6-9d69-f4b7da7292da@gmail.com","threadId":"66192","inReplyTo":"xmqqjyp71g9s.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] packfile: recover when a multi-pack-index names a removed pack","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-09-01T15:27:08Z","receivedAt":"2026-09-01T15:27:12Z","isPatch":true,"body":"On 8/30/2026 4:53 PM, Junio C Hamano wrote:\n> Jeff King writes:\n> \n>> ...\n>> Sorry, I know that was a lot of text to end up at \"you have already\n>> written it the best way\", but it took me a while to reason through it.\n>>\n>> The patch looks good to me. ;)\n> \n> Thanks for a very informative and well reasoned write-up in support\n> of the series.\n> \n> Shall we mark it for 'next' then?\n\nI'm late in responding, but I support the series, too!\n\nthanks,\n-Stolee\n\n"},{"id":"551685","messageId":"xmqqmru1t0u8.fsf@gitster.g","threadId":"66192","inReplyTo":"374bffe1-47ff-4cb6-9d69-f4b7da7292da@gmail.com","subject":"Re: [PATCH v3 4/4] packfile: recover when a multi-pack-index names a removed pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-01T16:04:15Z","receivedAt":"2026-09-01T16:04:18Z","isPatch":true,"body":"Derrick Stolee writes:\n\n> On 8/30/2026 4:53 PM, Junio C Hamano wrote:\n>> Jeff King writes:\n>> \n>>> ...\n>>> Sorry, I know that was a lot of text to end up at \"you have already\n>>> written it the best way\", but it took me a while to reason through it.\n>>>\n>>> The patch looks good to me. ;)\n>> \n>> Thanks for a very informative and well reasoned write-up in support\n>> of the series.\n>> \n>> Shall we mark it for 'next' then?\n>\n> I'm late in responding, but I support the series, too!\n>\n> thanks,\n> -Stolee\n\nThanks, all.\n"},{"id":"551687","messageId":"CABPp-BEK8f4Dh=3z-Q768iBV-d-wdpXGSKhsfFacGwHEFabZKA@mail.gmail.com","threadId":"66192","inReplyTo":"944945ab-dde7-41e5-af92-fc520485fc53@gmail.com","subject":"Re: [PATCH v3 4/4] packfile: recover when a multi-pack-index names a removed pack","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-09-01T16:47:59Z","receivedAt":"2026-09-01T16:48:13Z","isPatch":true,"body":"On Tue, Sep 1, 2026 at 8:26 AM Derrick Stolee wrote:\n>\n> On 8/29/2026 3:00 AM, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n>\n> I'm late in reviewing this patch, so forgive me responding inline as\n> I discover how it works.\n>\n> tl;dr: Good patch. LGTM.\n\nThanks for taking a look; I wanted to point out two minor clarifications...\n\n> > + /*\n> > + * Recovery for a concurrent-repack race: a stale MIDX may still name a\n> > + * vanished owning pack even though the object survives in another pack\n> > + * the same MIDX covers. The regular fallback above skips MIDX-covered\n> > + * packs, and repreparing the on-disk pack set does not reload the\n> > + * borrowed, cached MIDX, so scan its packs directly for the survivor.\n> > + *\n> > + * Do this only on the second read, by which point repreparing packs has\n> > + * already had a chance to find an object merely relocated into a new,\n> > + * uncovered pack; only a genuine hidden duplicate reaches here.\n> > + */\n>\n> This comment does a lot of important context-setting to show\n> that we are in a very narrow case: the stale MIDX has multiple\n> packs that contain the requested object, but the \"newer\" one\n> was deleted without creating a new packfile, so we need to\n> look at each contained pack for the object from its pack-index.\n\nActually, a new packfile is typically created, it just doesn't have\nthe object in question -- and doesn't need to, because a pre-existing\n(also midx-covered) pack already has it.\n\n> > + if (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&\n> > + (flags & OBJECT_INFO_SECOND_READ)) {\n> > + struct multi_pack_index *m = store->midx;\n> > + uint32_t i;\n> > +\n> > + for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {\n> > + struct packed_git *p;\n> > +\n> > + if (prepare_midx_pack(m, i))\n> > + continue;\n> > + p = nth_midxed_pack(m, i);\n> > + if (p && packfile_fill_entry(p, oid, e, bad_pack))\n> > + return 1;\n> > + }\n> > + }\n> > +\n>\n> This is hopefully a very rare case, but it's good to have\n> this \"fall back to O(num packs)\" situation.\n\nIt's actually a fall back to O(num_packs_in_the_midx); on developer\nlaptops that's probably about the same as O(num_packs), but on busy\nservers constantly receiving pushes, the total number of packs often\ndwarfs the number of packs in the midx.\n"},{"id":"551689","messageId":"2729941e-c682-42dd-ac82-9d59c9c9668e@gmail.com","threadId":"66192","inReplyTo":"CABPp-BEK8f4Dh=3z-Q768iBV-d-wdpXGSKhsfFacGwHEFabZKA@mail.gmail.com","subject":"Re: [PATCH v3 4/4] packfile: recover when a multi-pack-index names a removed pack","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-09-01T17:12:44Z","receivedAt":"2026-09-01T17:12:48Z","isPatch":true,"body":"On 9/1/2026 12:47 PM, Elijah Newren wrote:\n> On Tue, Sep 1, 2026 at 8:26 AM Derrick Stolee wrote:\n>>\n>> On 8/29/2026 3:00 AM, Elijah Newren via GitGitGadget wrote:\n>>> From: Elijah Newren \n>>\n>> I'm late in reviewing this patch, so forgive me responding inline as\n>> I discover how it works.\n>>\n>> tl;dr: Good patch. LGTM.\n> \n> Thanks for taking a look; I wanted to point out two minor clarifications...\n> \n>>> + /*\n>>> + * Recovery for a concurrent-repack race: a stale MIDX may still name a\n>>> + * vanished owning pack even though the object survives in another pack\n>>> + * the same MIDX covers. The regular fallback above skips MIDX-covered\n>>> + * packs, and repreparing the on-disk pack set does not reload the\n>>> + * borrowed, cached MIDX, so scan its packs directly for the survivor.\n>>> + *\n>>> + * Do this only on the second read, by which point repreparing packs has\n>>> + * already had a chance to find an object merely relocated into a new,\n>>> + * uncovered pack; only a genuine hidden duplicate reaches here.\n>>> + */\n>>\n>> This comment does a lot of important context-setting to show\n>> that we are in a very narrow case: the stale MIDX has multiple\n>> packs that contain the requested object, but the \"newer\" one\n>> was deleted without creating a new packfile, so we need to\n>> look at each contained pack for the object from its pack-index.\n> \n> Actually, a new packfile is typically created, it just doesn't have\n> the object in question -- and doesn't need to, because a pre-existing\n> (also midx-covered) pack already has it.\n\nThanks. That helps me understand why this can occur regularly\nenough to be triggered in the wild.\n\n>>> + if (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&\n>>> + (flags & OBJECT_INFO_SECOND_READ)) {\n>>> + struct multi_pack_index *m = store->midx;\n>>> + uint32_t i;\n>>> +\n>>> + for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {\n>>> + struct packed_git *p;\n>>> +\n>>> + if (prepare_midx_pack(m, i))\n>>> + continue;\n>>> + p = nth_midxed_pack(m, i);\n>>> + if (p && packfile_fill_entry(p, oid, e, bad_pack))\n>>> + return 1;\n>>> + }\n>>> + }\n>>> +\n>>\n>> This is hopefully a very rare case, but it's good to have\n>> this \"fall back to O(num packs)\" situation.\n> \n> It's actually a fall back to O(num_packs_in_the_midx); on developer\n> laptops that's probably about the same as O(num_packs), but on busy\n> servers constantly receiving pushes, the total number of packs often\n> dwarfs the number of packs in the midx.\n\nThanks. You're absolutely right that I was not specific enough and\nin server situations this loop will be very short.\n\nThanks,\n-Stolee\n\n"}]}