Objects treated as missing despite being present, due to race with geometric repacking #2207
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
newren marked this conversation as resolved.
There was a problem hiding this comment. Jeff King wrote on the Git mailing list (how to reply to this email): On Sat, Aug 29, 2026 at 07:00:31AM +0000, Elijah Newren via GitGitGadget wrote:
> + /*
> + * 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;
> + }
> + }
So I think this workaround is fine to do (as long as we are not going to
actually refresh the midx on SECOND_READ, which I agree is probably a
bigger change).
I always get confused about m->num_packs and m->num_packs_in_base, and
whether we are looking at the packs in a midx slice versus the whole
thing. I _think_ what you have here is correct, because we are iterating
from 0 up to the total number of packs, and prepare_midx_pack() etc will
look back through the incremental slices as necessary.
But I wonder if it would be simpler to just iterate over the actual pack
list in the usual way, since we already do that in this function. I
_thought_ this would work:
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 90d88c0a12..86e6a80d2f 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -33,40 +33,19 @@ static int find_pack_entry(struct odb_source_packed *store,
for (l = store->packs.head; l; l = l->next) {
struct packed_git *p = l->pack;
- if (!p->multi_pack_index && packfile_fill_entry(p, oid, e, bad_pack)) {
+ /* ...explain tricky race case here... */
+ if (p->multi_pack_index &&
+ (midx_result != MIDX_FILL_OWNER_UNAVAILABLE ||
+ !(flags & OBJECT_INFO_SECOND_READ)))
+ continue;
+
+ if (packfile_fill_entry(p, oid, e, bad_pack)) {
if (!store->skip_mru_updates)
packfile_list_prepend(&store->packs, p);
return 1;
}
}
- /*
- * Recovery for a concurrent-repack race: a stale MIDX may still name a
- * vanished owning pack even though the object survives in another pack
- * the same MIDX covers. The regular fallback above skips MIDX-covered
- * packs, and repreparing the on-disk pack set does not reload the
- * borrowed, cached MIDX, so scan its packs directly for the survivor.
- *
- * Do this only on the second read, by which point repreparing packs has
- * already had a chance to find an object merely relocated into a new,
- * uncovered pack; only a genuine hidden duplicate reaches here.
- */
- if (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&
- (flags & OBJECT_INFO_SECOND_READ)) {
- struct multi_pack_index *m = store->midx;
- uint32_t i;
-
- for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {
- struct packed_git *p;
-
- if (prepare_midx_pack(m, i))
- continue;
- p = nth_midxed_pack(m, i);
- if (p && packfile_fill_entry(p, oid, e, bad_pack))
- return 1;
- }
- }
-
return 0;
}
but it doesn't because we don't always load the midx'd packs into the
pack list (we do it on-demand as they become useful to us). So I think
you'd essentially end up needing to do a loop like the one you have
anyway to prepare_midx_pack() on them all.
And we want to avoid doing that if we can find it outside the midx
(since that was the whole point of waiting for SECOND_READ). Which would
happen...in that loop. So we really do want to have our own
midx-specific loop like you have here.
Sorry, I know that was a lot of text to end up at "you have already
written it the best way", but it took me a while to reason through it.
The patch looks good to me. ;)
-PeffThere was a problem hiding this comment. Junio C Hamano wrote on the Git mailing list (how to reply to this email): Jeff King <peff@peff.net> writes:
> ...
> Sorry, I know that was a lot of text to end up at "you have already
> written it the best way", but it took me a while to reason through it.
>
> The patch looks good to me. ;)
Thanks for a very informative and well reasoned write-up in support
of the series.
Shall we mark it for 'next' then?There was a problem hiding this comment. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Sun, Aug 30, 2026 at 01:53:51PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > ...
> > Sorry, I know that was a lot of text to end up at "you have already
> > written it the best way", but it took me a while to reason through it.
> >
> > The patch looks good to me. ;)
>
> Thanks for a very informative and well reasoned write-up in support
> of the series.
>
> Shall we mark it for 'next' then?
Here's my a lot less well reasoned +1, for what it's worth. Thanks!
PatrickThere was a problem hiding this comment. Jeff King wrote on the Git mailing list (how to reply to this email): On Sun, Aug 30, 2026 at 01:53:51PM -0700, Junio C Hamano wrote:
> Thanks for a very informative and well reasoned write-up in support
> of the series.
>
> Shall we mark it for 'next' then?
Yeah, that sounds good to me.
-PeffThere was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 8/30/2026 4:53 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> ...
>> Sorry, I know that was a lot of text to end up at "you have already
>> written it the best way", but it took me a while to reason through it.
>>
>> The patch looks good to me. ;)
>
> Thanks for a very informative and well reasoned write-up in support
> of the series.
>
> Shall we mark it for 'next' then?
I'm late in responding, but I support the series, too!
thanks,
-Stolee
There was a problem hiding this comment. Junio C Hamano wrote on the Git mailing list (how to reply to this email): Derrick Stolee <stolee@gmail.com> writes:
> On 8/30/2026 4:53 PM, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>>
>>> ...
>>> Sorry, I know that was a lot of text to end up at "you have already
>>> written it the best way", but it took me a while to reason through it.
>>>
>>> The patch looks good to me. ;)
>>
>> Thanks for a very informative and well reasoned write-up in support
>> of the series.
>>
>> Shall we mark it for 'next' then?
>
> I'm late in responding, but I support the series, too!
>
> thanks,
> -Stolee
Thanks, all.There was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 8/29/2026 3:00 AM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
I'm late in reviewing this patch, so forgive me responding inline as
I discover how it works.
tl;dr: Good patch. LGTM.
> Teach find_pack_entry() to recover. The MIDX lookup now returns a
> tri-state, distinguishing an object absent from the MIDX from one it owns
> via a pack that can no longer be opened; in the latter case, once the
> regular fallback has also missed, scan the MIDX's packs directly for a
> surviving copy. Because the return value is no longer a boolean, rename
> fill_midx_entry() to midx_fill_entry() so callers must reckon with the
> new enum rather than silently treat MIDX_FILL_OWNER_UNAVAILABLE as a hit.
This tri-state is valuable!
> 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 (uncovered) pack has already been found by the regular fallback, and
> only a genuine hidden duplicate reaches the rescan. A QUICK caller that
> skips the second read simply accepts the false negative, as QUICK is
> designed to.
>
> 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.
> - if (m && fill_midx_entry(m, oid, &e, NULL)) {
> + if (m && midx_fill_entry(m, oid, &e, NULL) == MIDX_FILL_HIT) {
One major benefit to the rename is that we can guarantee that
all callers are updated to reflect the new tri-state response.
It also has a better naming convention, overall.
(reordered header file diff up)
> +/*
> + * Result of looking an object up in a multi-pack-index. MIDX_FILL_HIT means
> + * "e was filled in"; the two miss variants distinguish an object the midx does
> + * not know about (MIDX_FILL_MISS) from one it does know about but whose owning
> + * pack we can no longer open (MIDX_FILL_OWNER_UNAVAILABLE -- the signature of a
> + * concurrent repack having removed that pack). A known-bad (corrupt) object
> + * reports MIDX_FILL_MISS but also sets *bad_pack, if provided, to the owning
> + * pack so the caller can tell "corrupt" apart from "absent".
> + */
> +enum midx_fill_result {
> + MIDX_FILL_MISS = 0,
> + MIDX_FILL_HIT,
> + MIDX_FILL_OWNER_UNAVAILABLE,
> +};
> +
> +enum midx_fill_result midx_fill_entry(struct multi_pack_index *m,
> + const struct object_id *oid,
> + struct pack_entry *e,
> + struct packed_git **bad_pack);
This is good documentation that will help future uses know how to
react to the different modes.
> -int fill_midx_entry(struct multi_pack_index *m,
> - const struct object_id *oid,
> - struct pack_entry *e,
> - struct packed_git **bad_pack)
> +enum midx_fill_result midx_fill_entry(struct multi_pack_index *m,
> + const struct object_id *oid,
> + struct pack_entry *e,
> + struct packed_git **bad_pack)
> {
> uint32_t pos;
> uint32_t pack_int_id;
> struct packed_git *p;
>
> if (!bsearch_midx(oid, m, &pos))
> - return 0;
> + return MIDX_FILL_MISS;
Obviously correct: this OID isn't in the sorted list.
> midx_for_object(&m, pos);
> pack_int_id = nth_midxed_pack_int_id(m, pos);
>
> if (prepare_midx_pack(m, pack_int_id))
> - return 0;
> + return MIDX_FILL_OWNER_UNAVAILABLE;
Obviously correct: we tried to open the pack index but failed.
> p = m->packs[pack_int_id - m->num_packs_in_base];
>
> /*
> @@ -616,19 +616,19 @@ int fill_midx_entry(struct multi_pack_index *m,
> * loaded!
> */
> if (!is_pack_valid(p))
> - return 0;
> + return MIDX_FILL_OWNER_UNAVAILABLE;
Same: Pack is invalid somehow, likely that the .pack disappeared.
> if (oidset_size(&p->bad_objects) &&
> oidset_contains(&p->bad_objects, oid)) {
> if (bad_pack && !*bad_pack)
> *bad_pack = p;
> - return 0;
> + return MIDX_FILL_MISS;
This one is tricky, but makes sense: we have marked this as a
"bad" object so we should act like it doesn't exist. Good.
> }
>
> e->offset = nth_midxed_offset(m, pos);
> e->p = p;
>
> - return 1;
> + return MIDX_FILL_HIT;
finally: success!> }
> static int find_pack_entry(struct odb_source_packed *store,
> const struct object_id *oid,
> struct pack_entry *e,
> + enum object_info_flags flags,
> struct packed_git **bad_pack)
> {
> struct packfile_list_entry *l;
> + enum midx_fill_result midx_result = MIDX_FILL_MISS;
>
> odb_source_prepare(&store->base, 0);
> - if (store->midx && fill_midx_entry(store->midx, oid, e, bad_pack))
> - return 1;
> + if (store->midx) {
> + midx_result = midx_fill_entry(store->midx, oid, e, bad_pack);
> + if (midx_result == MIDX_FILL_HIT)
> + return 1;
> + }
This looks good. On a hit, we return. Act like a MIDX-miss if we
don't have a midx.
Outside of the patch context is the "reprepare packfiles" to pick
up a copy from a packfile that doesn't exist within the current
(stale) midx.
> + /*
> + * 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.
> + */
This comment does a lot of important context-setting to show
that we are in a very narrow case: the stale MIDX has multiple
packs that contain the requested object, but the "newer" one
was deleted without creating a new packfile, so we need to
look at each contained pack for the object from its pack-index.
> + 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;
> + }
> + }
> +
This is hopefully a very rare case, but it's good to have
this "fall back to O(num packs)" situation.
> +test_expect_success 'lookup recovers object whose midx-owning pack was removed' '
> + test_when_finished "rm -fr repo" &&
> + git init repo &&
> + (
> + cd repo &&
> +
> + # "keep" ends up only in the big pack; "dup" is deliberately
> + # placed in two packs so the midx has to choose an owner.
> + test_commit keep &&
> + echo duplicated-content >dup &&
> + git add dup &&
> + git commit -m dup &&
> + dup_oid=$(git rev-parse HEAD:dup) &&
> +
> + # Roll every object, including dup, into a single big pack.
> + git repack -adq &&
> +
> + # Build a second, "moderate" pack that also contains dup, so dup
> + # now lives in two packs that the midx will cover.
> + moderate=$(echo "$dup_oid" |
> + git pack-objects --quiet $objdir/pack/pack) &&
> +
> + # Attribute dup to the moderate pack in the midx.
> + git multi-pack-index write \
> + --preferred-pack="pack-$moderate.idx" &&
This use of preferred pack is a good way of getting around mtimes
that could be equal. We could also consider updating mtimes, but
this works so don't change it.
> + # Simulate a concurrent "git repack" retiring the moderate pack:
> + # its files disappear, but the now-stale midx still names it as
> + # the owner of dup. A valid copy of dup survives in the big pack.
> + rm -f $objdir/pack/pack-$moderate.* &&
> +
> + # The midx routes the lookup to the deleted pack, and the regular
> + # pack fallback skips midx-covered packs, so without recovery dup
> + # would appear missing even though it is physically present.
> + echo blob >expect &&
> + git cat-file -t "$dup_oid" >actual &&
> + test_cmp expect actual
> + )
> +'
Thanks for adding this test so we can keep this narrow case
working in perpetuity.
Thanks,
-Stolee
There was a problem hiding this comment. Elijah Newren wrote on the Git mailing list (how to reply to this email): On Tue, Sep 1, 2026 at 8:26 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 8/29/2026 3:00 AM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
>
> I'm late in reviewing this patch, so forgive me responding inline as
> I discover how it works.
>
> tl;dr: Good patch. LGTM.
Thanks for taking a look; I wanted to point out two minor clarifications...
> > + /*
> > + * 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.
> > + */
>
> This comment does a lot of important context-setting to show
> that we are in a very narrow case: the stale MIDX has multiple
> packs that contain the requested object, but the "newer" one
> was deleted without creating a new packfile, so we need to
> look at each contained pack for the object from its pack-index.
Actually, a new packfile is typically created, it just doesn't have
the object in question -- and doesn't need to, because a pre-existing
(also midx-covered) pack already has it.
> > + 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;
> > + }
> > + }
> > +
>
> This is hopefully a very rare case, but it's good to have
> this "fall back to O(num packs)" situation.
It's actually a fall back to O(num_packs_in_the_midx); on developer
laptops that's probably about the same as O(num_packs), but on busy
servers constantly receiving pushes, the total number of packs often
dwarfs the number of packs in the midx.There was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 9/1/2026 12:47 PM, Elijah Newren wrote:
> On Tue, Sep 1, 2026 at 8:26 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 8/29/2026 3:00 AM, Elijah Newren via GitGitGadget wrote:
>>> From: Elijah Newren <newren@gmail.com>
>>
>> I'm late in reviewing this patch, so forgive me responding inline as
>> I discover how it works.
>>
>> tl;dr: Good patch. LGTM.
>
> Thanks for taking a look; I wanted to point out two minor clarifications...
>
>>> + /*
>>> + * 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.
>>> + */
>>
>> This comment does a lot of important context-setting to show
>> that we are in a very narrow case: the stale MIDX has multiple
>> packs that contain the requested object, but the "newer" one
>> was deleted without creating a new packfile, so we need to
>> look at each contained pack for the object from its pack-index.
>
> Actually, a new packfile is typically created, it just doesn't have
> the object in question -- and doesn't need to, because a pre-existing
> (also midx-covered) pack already has it.
Thanks. That helps me understand why this can occur regularly
enough to be triggered in the wild.
>>> + 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;
>>> + }
>>> + }
>>> +
>>
>> This is hopefully a very rare case, but it's good to have
>> this "fall back to O(num packs)" situation.
>
> It's actually a fall back to O(num_packs_in_the_midx); on developer
> laptops that's probably about the same as O(num_packs), but on busy
> servers constantly receiving pushes, the total number of packs often
> dwarfs the number of packs in the midx.
Thanks. You're absolutely right that I was not specific enough and
in server situations this loop will be very short.
Thanks,
-Stolee
|
||
| struct pack_entry e; | ||
|
|
||
| if (m && midx_fill_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; | ||
|
|
||
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
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
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
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
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jeff King wrote on the Git mailing list (how to reply to this email):