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 |
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