Objects treated as missing despite being present, due to race with geometric repacking - #2207
Open
newren wants to merge 4 commits into
Open
Conversation
Author
|
/submit |
|
Submitted as pull.2207.git.1787092446.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
|
User |
|
This branch is now known as |
|
This patch series was integrated into seen via git@8ef1c4d. |
|
There was a status update in the "New Topics" section about the branch The object lookup machinery has been taught to gracefully recover when a multi-pack-index points to an owning pack that was removed during a concurrent geometric repack, and 'git replay' has been fixed to not segfault when reading such missing objects. Waiting for response. cf. <xmqqfr0augls.fsf@gitster.g> cf. <aoayppoxHAkcFTBN@pks.im> source: <pull.2207.git.1787092446.gitgitgadget@gmail.com> |
|
User |
newren
force-pushed
the
midx-removed-pack-recovery
branch
from
August 21, 2026 18:29
5792c08 to
a912b8c
Compare
|
This patch series is no longer integrated into seen. |
|
User |
|
There was a status update in the "Cooking" section about the branch The object lookup machinery has been taught to gracefully recover when a multi-pack-index points to an owning pack that was removed during a concurrent geometric repack, and 'git replay' has been fixed to not segfault when reading such missing objects. Waiting for response. cf. <xmqqfr0augls.fsf@gitster.g> cf. <aoayppoxHAkcFTBN@pks.im> source: <pull.2207.git.1787092446.gitgitgadget@gmail.com> |
|
User |
|
This patch series was integrated into seen via git@e4a2bf3. |
newren
force-pushed
the
midx-removed-pack-recovery
branch
from
August 25, 2026 07:29
a912b8c to
60dc2ad
Compare
When objects involved in the merge cannot be read, the merge machinery will return early with result.clean = -1, and result.tree left as NULL. pick_regular_commit() tested only "if (!result->clean)", ignoring the case where "clean < 0". That causes the code to try to use result->tree, resulting in a SIGSEGV. Handle clean < 0 explicitly; the merge machinery will already have printed messages such as "Could not read <object>" and "collecting merge info failed for trees...", so we don't need to add much detail beyond the fact that the merge failed. Signed-off-by: Elijah Newren <newren@gmail.com>
In --batch mode "git mktree" reuses its entry buffer across trees, resetting `used` to 0 after writing each tree. It never frees the `treeent` structures the previous tree appended, though, so once the next tree overwrites those slots the earlier allocations are leaked. A single-tree invocation hides this, as the entries stay reachable through the `entries` global until exit. Free each entry when resetting the buffer, and free the buffer itself before returning. Signed-off-by: Elijah Newren <newren@gmail.com>
When a reader opens a pack it discovered on disk, open_packed_git_1()
first mmaps the pack's `.idx`. A `git repack` running alongside us
consolidates existing packs into a new one and then removes the
redundant packs, deleting each pack's `.idx` before its `.pack` (see the
ordering in unlink_pack_path()). A reader that had just enumerated one
of those packs -- most easily through a multi-pack-index -- can race with
the removal and find the pack gone.
Two things go wrong in that window:
1. open_pack_index() fails, so we print
error: packfile <path> index unavailable
and report the pack as unusable, even though the object still lives
in the replacement pack.
2. A normal lookup recovers: odb_read_object_info_extended() issues a
second read that reloads the on-disk pack state and finds the object
in its new home, making the message above mere noise. But an
OBJECT_INFO_QUICK lookup deliberately skips that second read to stay
fast on a genuine miss, so it does *not* recover: it reports the
object as absent even though it still lives in the replacement pack.
A resident reader that resolves objects with a QUICK lookup -- such
as the `git mktree --batch` process the tests below drive -- then
produces wrong results. Even where a spurious miss is not fatal it
is not harmless: `git upload-pack` checks a client's "have" lines
with a QUICK lookup, and a dropped "have" removes a common object
from the negotiation, so the client is sent more than it needs.
Recovering without giving up that speed is the trick: we keep QUICK's
fast path for a genuine miss and force the extra read only when a pack
we were already using has provably vanished.
Fix both. Record that a pack disappeared out from under us by setting
object_database.stale_packs_detected at the three points where a reader
can notice a pack vanish beneath it:
- In open_packed_git_1(), when open_pack_index() fails because the
index simply vanished (its open fails with ENOENT). Here we also
stay silent instead of printing "index unavailable"; a genuinely
unreadable index that is still present keeps the error, since that is
a real problem worth surfacing.
- In open_packed_git_1() again, from the other side of the race: when
the `.idx` was already mapped -- so open_pack_index() returns without
touching the filesystem -- yet opening the `.pack` fails with ENOENT.
A reader that prepared its pack list before the repack only trips
over the removal when it finally opens the pack file.
- In prepare_midx_pack(), when packfile_store_load_pack() cannot open a
pack the midx still references at all. If both the `.idx` and the
`.pack` are already gone -- as happens when the redundant pack is
removed outright rather than index-first -- we never reach
open_pack_index(), so this is the only place the vanished pack is
observed.
Then, in odb_read_object_info_extended(), issue the second read -- which
asks the sources to reload their on-disk state (for packs, a reprepare)
and retry -- not only for non-QUICK lookups but also whenever
stale_packs_detected is set, even under OBJECT_INFO_QUICK. An ordinary
QUICK miss, with no vanished pack, still skips the second read and stays
fast; we pay for the rescan only when we have positive evidence that the
on-disk pack set changed beneath us. The flag is reset when the
packfiles are reprepared, in odb_source_packed_prepare().
Add t5336, regression tests that reproduce the race deterministically:
they drive a resident `git mktree --batch` reader -- which resolves each
tree entry with OBJECT_INFO_QUICK -- across both removal windows, one
removing a pack's `.idx` first while a midx routes the lookup to the
doomed pack, the other removing a pack's `.pack` after its `.idx` was
already mapped. Each confirms the reader recovers the relocated object
instead of dying.
Assisted-by: Claude Opus 4.8 & GPT-5.6 Sol
Signed-off-by: Elijah Newren <newren@gmail.com>
A geometric repack writes a new pack and multi-pack-index and then deletes the packs the new one subsumes. A process still using the previous MIDX keeps seeing a removed pack listed as the owner of some objects. Since a MIDX attributes each object to exactly one pack, such an object is served only through its recorded owner; if that owner was just removed, find_pack_entry() cannot serve it -- fill_midx_entry() routes to the missing pack, and the regular pack fallback deliberately skips every MIDX-covered pack, so a surviving copy in another covered pack (e.g. a kept base pack) is never consulted. Unlike the ordinary "a pack's .idx is mapped but its .pack is gone" race, the second read does not rescue us -- and not only for OBJECT_INFO_QUICK callers. Reloading the on-disk pack set does not reload the borrowed, cached MIDX (freeing it under the code that caches the "struct multi_pack_index *" would be a use-after-free), so the stale MIDX keeps routing to the removed pack and the surviving copy stays hidden behind the covered-pack skip. cat-file, rev-list and pack-objects can thus all spuriously fail with "unable to read object". Teach find_pack_entry() to recover. fill_midx_entry() now returns a tri-state, distinguishing "absent from the MIDX" from "present but the owning pack is unavailable"; in the latter case, once the regular fallback has also missed, scan the MIDX's packs directly for a surviving copy. Do the scan only on the second read (OBJECT_INFO_SECOND_READ): by then the cheaper on-disk reload has run, so an object merely relocated into a new (non-covered) pack has already been found by the regular fallback, and only a genuine hidden duplicate reaches the rescan. QUICK callers that would skip the second read are steered into it by the preceding commit's stale_packs_detected flag, which prepare_midx_pack() sets when it cannot open the owning pack. Reloading the stale MIDX would be a more complete fix but is much more involved (the borrowers above need proper invalidation), so leave that for later. Assisted-by: Claude Opus 4.8 & GPT-5.6 Sol Helped-by: Jeff King <peff@peff.net> Signed-off-by: Elijah Newren <newren@gmail.com>
newren
force-pushed
the
midx-removed-pack-recovery
branch
from
August 25, 2026 17:03
60dc2ad to
eacf6ba
Compare
Author
|
/submit |
|
Submitted as pull.2207.v2.git.1787684429.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
There was a status update in the "Cooking" section about the branch The object lookup machinery has been taught to gracefully recover when a multi-pack-index points to an owning pack that was removed during a concurrent geometric repack, and 'git replay' has been fixed to not segfault when reading such missing objects. Needs review. source: <pull.2207.v2.git.1787684429.gitgitgadget@gmail.com> |
| @@ -200,8 +200,11 @@ int cmd_mktree(int ac, | |||
| puts(oid_to_hex(&oid)); | |||
There was a problem hiding this comment.
Jeff King wrote on the Git mailing list (how to reply to this email):
On Tue, Aug 25, 2026 at 07:00:27PM +0000, Elijah Newren via GitGitGadget wrote:
> In --batch mode "git mktree" reuses its entry buffer across trees,
> resetting `used` to 0 after writing each tree. It never frees the
> `treeent` structures the previous tree appended, though, so once the
> next tree overwrites those slots the earlier allocations are leaked. A
> single-tree invocation hides this, as the entries stay reachable through
> the `entries` global until exit.
>
> Free each entry when resetting the buffer, and free the buffer itself
> before returning.
Yikes. It is sad that we did not catch this in our leak-checking builds,
as it implies that we do not test "mktree --batch" with multiple inputs.
Or grepping for "mktree.*--batch" implies that we do not test the
feature at all!
Looks like that feature comes from f1cf2d8b14 (mktree --batch: build
more than one tree object, 2009-05-14), so I am not surprised that test
coverage was a bit more spotty back then.
I guess you are going to add some coverage incidentally (or else you
would not have found this). That's better than nothing, but I suspect a
few basic directed "mktree --batch" tests would be a good thing to have
in t1010.
#leftoverbits, perhaps?
-Peff
| @@ -475,6 +475,12 @@ int prepare_midx_pack(struct multi_pack_index *m, | |||
|
|
|||
There was a problem hiding this comment.
Jeff King wrote on the Git mailing list (how to reply to this email):
On Tue, Aug 25, 2026 at 07:00:28PM +0000, Elijah Newren via GitGitGadget wrote:
> 1. open_pack_index() fails, so we print
>
> error: packfile <path> index unavailable
>
> and report the pack as unusable, even though the object still lives
> in the replacement pack.
>
> 2. A normal lookup recovers: odb_read_object_info_extended() issues a
> second read that reloads the on-disk pack state and finds the object
> in its new home, making the message above mere noise. But an
> OBJECT_INFO_QUICK lookup deliberately skips that second read to stay
> fast on a genuine miss, so it does *not* recover: it reports the
> object as absent even though it still lives in the replacement pack.
> A resident reader that resolves objects with a QUICK lookup -- such
> as the `git mktree --batch` process the tests below drive -- then
> produces wrong results. Even where a spurious miss is not fatal it
> is not harmless: `git upload-pack` checks a client's "have" lines
> with a QUICK lookup, and a dropped "have" removes a common object
> from the negotiation, so the client is sent more than it needs.
Maybe I am still being dense, but this description does not make any
sense to me at all.
The _point_ of QUICK is to accept those false negatives. It is the right
thing for upload-pack to do, to avoid re-scans for objects which we
simply don't have (and don't necessarily expect to have).
It sounds like mktree is wrong to be using QUICK at all. It comes from
817b0f6027 (mktree: do not check type of remote objects, 2022-06-21)
which rewrote a call to vanilla oid_object_info(). From the description
there it probably should be using SKIP_FETCH_OBJECT but not QUICK. Or
possibly it should use neither unless --missing is given.
So I don't see QUICK itself here violating any contract (even if it
_could_ find the object in some cases with just a little more work, as
in the case that we were discussing for v1).
The much more interesting case is the non-QUICK one that Patrick
outlined earlier in the thread. Where we say "nope, we don't have that
object" even though we could find it with a little more work. But that
doesn't seem to be described here either. But I think that is not even
what this patch is about; that's in patch 4.
If the "error:" message is scary and gross (especially because we may
retry and correct it anyway) and happens due to routine races, we might
consider suppressing it.
> + /*
> + * Set when a lookup finds that a pack we already know about has
> + * vanished -- its ".idx" or ".pack" removed out from under us, the
> + * signature of a concurrent "git repack". It tells
> + * odb_read_object_info_extended() to reprepare and retry even for an
> + * OBJECT_INFO_QUICK lookup, which normally skips that rescan to stay
> + * fast on a genuine miss. Reset when the packfiles are reprepared
> + * (see odb_source_packed_prepare()).
> + */
> + unsigned stale_packs_detected : 1;
So this is a way of hackily triggering SECOND_READ for QUICK queries,
even though the point of QUICK is to suppress that second read! Again,
maybe I'm just being dense, but I don't get it.
> @@ -535,8 +550,20 @@ static int open_packed_git_1(struct packed_git *p)
> ssize_t read_result;
> const unsigned hashsz = p->repo->hash_algo->rawsz;
>
> - if (open_pack_index(p))
> + if (open_pack_index(p)) {
> + /*
> + * A concurrent repack may have removed this pack, deleting its
> + * ".idx" before its ".pack" (see unlink_pack_path()). If the
> + * index simply vanished, note the stale pack set and stay
> + * quiet; the pack is still reported unusable. Only a
> + * still-present but unreadable index is worth an error.
> + */
> + if (pack_index_is_missing(p)) {
> + p->repo->objects->stale_packs_detected = 1;
> + return -1;
> + }
> return error("packfile %s index unavailable", p->pack_name);
> + }
And this seems racy. We might catch the .idx but miss the .pack file.
That would cause a failed read, but not trigger sale_packs_detected.
-PeffThere was a problem hiding this comment.
Elijah Newren wrote on the Git mailing list (how to reply to this email):
On Wed, Aug 26, 2026 at 10:57 PM Jeff King <peff@peff.net> wrote:
>
> On Tue, Aug 25, 2026 at 07:00:28PM +0000, Elijah Newren via GitGitGadget wrote:
>
> > 1. open_pack_index() fails, so we print
> >
> > error: packfile <path> index unavailable
> >
> > and report the pack as unusable, even though the object still lives
> > in the replacement pack.
> >
> > 2. A normal lookup recovers: odb_read_object_info_extended() issues a
> > second read that reloads the on-disk pack state and finds the object
> > in its new home, making the message above mere noise. But an
> > OBJECT_INFO_QUICK lookup deliberately skips that second read to stay
> > fast on a genuine miss, so it does *not* recover: it reports the
> > object as absent even though it still lives in the replacement pack.
> > A resident reader that resolves objects with a QUICK lookup -- such
> > as the `git mktree --batch` process the tests below drive -- then
> > produces wrong results. Even where a spurious miss is not fatal it
> > is not harmless: `git upload-pack` checks a client's "have" lines
> > with a QUICK lookup, and a dropped "have" removes a common object
> > from the negotiation, so the client is sent more than it needs.
>
> Maybe I am still being dense, but this description does not make any
> sense to me at all.
>
> The _point_ of QUICK is to accept those false negatives. It is the right
> thing for upload-pack to do, to avoid re-scans for objects which we
> simply don't have (and don't necessarily expect to have).
It's far more likely that I am the one being dense. My rough line of thinking:
* We see "packfile ... index unavailable" in our logging
* There's only one thing that remove packfiles
* Investigate the mechanism
* Look for other affected callers (e.g. mktree --batch)
* Consider corrective measures
Steps 1-4 above are probably fine, and step 5 may have been where I
went off the rails. My thinking there, wrong or right, was:
* It makes sense that we don't want to reprepare most of the time
* ...but _if_ we know of the existence of some specific packfile in
this process and that packfile has since disappeared by the time we go
to open or read it, is that a special case? Should it be?
> It sounds like mktree is wrong to be using QUICK at all. It comes from
> 817b0f6027 (mktree: do not check type of remote objects, 2022-06-21)
> which rewrote a call to vanilla oid_object_info(). From the description
> there it probably should be using SKIP_FETCH_OBJECT but not QUICK. Or
> possibly it should use neither unless --missing is given.
>
> So I don't see QUICK itself here violating any contract (even if it
> _could_ find the object in some cases with just a little more work, as
> in the case that we were discussing for v1).
I'll drop this patch and instead send a small mktree change that stops
passing OBJECT_INFO_QUICK (keeping SKIP_FETCH_OBJECT), so mktree
recovers via the normal reprepare like every other non-QUICK reader.
That removes the packfile.c changes entirely, so both the
reload-under-QUICK hack and the .idx/.pack raciness you noted in
pack_index_is_missing() go away with them.
| @@ -1786,7 +1786,7 @@ static int want_object_in_pack_mtime(const struct object_id *oid, | |||
| struct multi_pack_index *m = get_multi_pack_index(files->packed); | |||
There was a problem hiding this comment.
Jeff King wrote on the Git mailing list (how to reply to this email):
On Tue, Aug 25, 2026 at 07:00:29PM +0000, Elijah Newren via GitGitGadget wrote:
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 399acd0f22..30ad7d822c 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1786,7 +1786,7 @@ static int want_object_in_pack_mtime(const struct object_id *oid,
> struct multi_pack_index *m = get_multi_pack_index(files->packed);
> struct pack_entry e;
>
> - if (m && fill_midx_entry(m, oid, &e, NULL)) {
> + if (m && fill_midx_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {
> want = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);
> if (want != -1)
> return want;
We've changed the return value semantics without changing the signature
(or name). So we need to make sure we adjust all callers, as here.
That's _probably_ OK in practice for such a specialized function. But we
could also rename it if we wanted to be paranoid (especially about
new callers added on parallel branches).
> +enum midx_fill_result fill_midx_entry(struct multi_pack_index *m,
> + const struct object_id *oid,
> + struct pack_entry *e,
> + struct packed_git **bad_pack)
OK, so this is our tri-state fix. Mostly looks as expected, though:
> if (prepare_midx_pack(m, pack_int_id))
> - return 0;
> + goto owner_unavailable;
I'd have expected just "return MIDX_FILL_OWNER_UNAVAILABLE" here. But
then, I'm not sure I buy the need for this stale_packs_detected stuff
from patch 3.
> p = m->packs[pack_int_id - m->num_packs_in_base];
>
> - /*
> - * We are about to tell the caller where they can locate the
> - * requested object. We better make sure the packfile is
> - * still here and can be accessed before supplying that
> - * answer, as it may have been deleted since the MIDX was
> - * loaded!
> - */
> + /* Make sure the pack is still present before pointing at it. */
> if (!is_pack_valid(p))
> - return 0;
> + goto owner_unavailable;
This comment rewrite seems superfluous at best. Can we try to keep such
patch fluff to a minimum?
> + /*
> + * Recovery for a concurrent-repack race: a stale MIDX may still name a
> + * vanished owning pack even though the object survives in another pack
> + * the same MIDX covers. The regular fallback above skips MIDX-covered
> + * packs, and repreparing the on-disk pack set does not reload the
> + * borrowed, cached MIDX, so scan its packs directly for the survivor.
> + *
> + * Do this only on the second read, by which point repreparing packs has
> + * already had a chance to find an object merely relocated into a new,
> + * uncovered pack; only a genuine hidden duplicate reaches here.
> + */
> + if (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&
> + (flags & OBJECT_INFO_SECOND_READ)) {
> + struct multi_pack_index *m = store->midx;
> + uint32_t i;
> +
> + for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {
> + struct packed_git *p;
> +
> + if (prepare_midx_pack(m, i))
> + continue;
> + p = nth_midxed_pack(m, i);
> + if (p && packfile_fill_entry(p, oid, e, bad_pack))
> + return 1;
> + }
> + }
OK, and this is as-before but now gated on the SECOND_READ flag. As
expected in this revision.
-Peff
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes since v1:
git mktree --batchsince I use it in new testcases and don't want the *-leaks jobs failingCover letter addendum/update:
A geometric repack writes a new pack plus multi-pack-index and then deletes the packs the new one subsumes. Readers running alongside it can be told an object is missing when it is in fact still present. The v1 series fixed one race of this shape (the object didn't move and was in a second pack referenced by the multi-pack-index); v2 added a new patch fixing others in the same class but of a different shape (the object moved to a brand new pack).
Note here that Stolee's suggestion to defer pack deletion via
git multi-pack-index expireseems like a good complementary mitigation; it would reduce how often we fall into recovery, while this series tries to fix recovery to work more robustly.Original cover letter (focused on the final patch):
When an object is found in multiple packs that are in a multi-pack-index, and a subsequent geometric repacking creates a new multi-pack-index and removes the pack that was considered the owner of the object in the old multi-pack-index, then an already-running process that had opened the old multi-pack-index and hadn't yet opened the removed packfile will not be able to access the object -- lookups will return it as missing. Additionally, replay has a separate bug where a missing object causes a SIGSEGV rather than an error message.
This appears to affect a very small percentage of git operations in production since it is a tiny window, but I've found evidence of it occurring in at least eight distinct server-side operations, covering seven different git commands:
There are also commands that could be changing behavior without throwing an error -- e.g. object negotiation thinking an object doesn't exist and instead negotiating based on an older common commit, or cat-file --batch reporting that some objects don't exist.
This series fixes the replay bug first, since it's simpler; investigating it, together with my other recent repacking work, is what led me to the underlying multi-pack-index issue that 2/2 addresses.
cc: Patrick Steinhardt ps@pks.im
cc: Elijah Newren newren@gmail.com
cc: Jeff King peff@peff.net
cc: Derrick Stolee stolee@gmail.com