promisor-remote: prevent recursive lazy-fetch during index-pack#2224
Open
ptarjan wants to merge 1 commit intogit:masterfrom
Open
promisor-remote: prevent recursive lazy-fetch during index-pack#2224ptarjan wants to merge 1 commit intogit:masterfrom
ptarjan wants to merge 1 commit intogit:masterfrom
Conversation
14d1696 to
7b72344
Compare
Contributor
Author
|
/submit |
|
Submitted as pull.2224.git.git.1772643468305.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Paul Tarjan via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Paul Tarjan <github@paulisageek.com>
>
> fetch_objects() spawns a child `git fetch` to lazily fill in missing
> objects. That child's index-pack, when it receives a thin pack
> containing a REF_DELTA against a still-missing base, explicitly
> calls promisor_remote_get_direct() — which is fetch_objects() again.
> If the base is truly unavailable (e.g. because many refs in the
> local store point at objects that have been garbage-collected on the
> server), each recursive lazy-fetch can trigger another, leading to
> unbounded recursion with runaway disk and process consumption.
>
> The GIT_NO_LAZY_FETCH guard (introduced by e6d5479e7a (git: add
> --no-lazy-fetch option, 2021-08-31)) already exists at the top of
> fetch_objects(); the missing piece is propagating it into the child
> fetch's environment. Add that propagation so the child's
> index-pack, if it encounters a REF_DELTA against a missing base,
> hits the guard and fails fast instead of recursing.
>
> Depth-1 lazy fetch (the whole point of fetch_objects()) is
> unaffected: only the child and its descendants see the variable.
> With negotiationAlgorithm=noop the client advertises no "have"
> lines, so a well-behaved server sends requested objects
> un-deltified or deltified only against objects in the same pack;
> the child's index-pack should never need a depth-2 fetch. If it
> does, the server response was broken or the local store is already
> corrupt, and further fetching would not help.
>
> This is the same bug shape that 3a1ea94a49 (commit-graph.c: no lazy
> fetch in lookup_commit_in_graph(), 2022-07-01) addressed at a
> different entry point.
>
> Add a test that verifies the child fetch environment contains
> GIT_NO_LAZY_FETCH=1 via a reference-transaction hook, and that
> only one fetch subprocess is spawned.
>
> Cc: Jonathan Tan <jonathantanmy@google.com>
> Cc: Han Xin <hanxin.hx@bytedance.com>
> Cc: Jeff Hostetler <jeffhostetler@github.com>
> Cc: Christian Couder <christian.couder@gmail.com>
I would suggest dropping these CC: lines from the proposed log
message. As far as I can see, they do not have their intended
effect; [*1*] does not show any of these folks listed on Cc:
*1* https://lore.kernel.org/git/pull.2224.git.git.1772643468305.gitgitgadget@gmail.com/
I am not a GitGitGadget user, but I think ...
> Signed-off-by: Paul Tarjan <github@paulisageek.com>
> ---
> promisor-remote: prevent recursive lazy-fetch during index-pack
>
> fetch_objects() in promisor-remote.c spawns a child git fetch to lazily
> fill missing objects. That child's index-pack --fix-thin, when it hits a
> REF_DELTA against a still-missing base, calls
> promisor_remote_get_direct() — which is fetch_objects() again. Unbounded
> recursion.
>
> We hit this in production: 276 GB of promisor packs written in 90
> minutes against a 100 GB monorepo with ~61K stale prefetch refs pointing
> at GC'd commits. Every thin pack picked a bad delta base, and the
> recursion fanned out until the mount filled.
>
> The fix is one line: propagate GIT_NO_LAZY_FETCH=1 into the child
> fetch's environment. The guard already exists at the top of
> fetch_objects() (added by e6d5479e7a, 2021); nothing was setting it in
> the child. This is the same bug shape that Han Xin's 3a1ea94a49 (2022)
> closed at lookup_commit_in_graph().
>
> Depth-1 lazy fetch (the whole point of fetch_objects()) is unaffected —
> only the child and its descendants see the variable. With
> negotiationAlgorithm=noop the client advertises no "have" lines, so a
> well-behaved server sends objects un-deltified or deltified only against
> objects in the same pack. A depth-2 fetch would not help; if the server
> sends broken thin packs, recursing just makes it worse.
... once I heard that the tool expects list of folks to CC: on this
side, i.e., not in the proposed commit log message, but in the pull
request description. I also do not see much point in duplicating
most of what appears in the proposed log message here after the
three dash line, but that is a separate story.
This is totally an unrelated tangent, but perhaps we'd need a
best-practice document/guide for GitGitGadget users, that covers at
least the following two things?
* The pull-request message appear under three-dash in the e-mailed
patch, where additional information that are not meant to become
part of the log message goes. You do not want to duplicate your
commit log message there.
* Do not write Cc: trailers in your commit log message, as
GitGitGadget does not pay attention to them. If you want to
specify whom to Cc: your patches, write these in your
pull-request message instead, which GitGitGadget does pay
attention to.
> diff --git a/promisor-remote.c b/promisor-remote.c
> index 96fa215b06..35c7aab93d 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -42,6 +42,13 @@ static int fetch_objects(struct repository *repo,
> child.in = -1;
> if (repo != the_repository)
> prepare_other_repo_env(&child.env, repo->gitdir);
> + /*
> + * Prevent the child's index-pack from recursing back into
> + * fetch_objects() when resolving REF_DELTA bases it does not
> + * have. With noop negotiation the server should never need
> + * to sen
8000
d such deltas, so a depth-2 fetch would not help.
> + */
> + strvec_pushf(&child.env, "%s=1", NO_LAZY_FETCH_ENVIRONMENT);
> strvec_pushl(&child.args, "-c", "fetch.negotiationAlgorithm=noop",
> "fetch", remote_name, "--no-tags",
> "--no-write-fetch-head", "--recurse-submodules=no",
Looks good.
> diff --git a/t/meson.build b/t/meson.build
> index e5174ee575..0499533dff 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -141,6 +141,7 @@ integration_tests = [
> 't0303-credential-external.sh',
> 't0410-partial-clone.sh',
> 't0411-clone-from-partial.sh',
> + 't0412-promisor-no-lazy-fetch-recursion.sh',
Hmph, do we really need an entirely new test script file dedicated
for this single liner change, instead of adding to some existing
test script that already covers related topics (like promisors and
lazy fetches from them)?
> 't0450-txt-doc-vs-help.sh',
> 't0500-progress-display.sh',
> 't0600-reffiles-backend.sh',
> diff --git a/t/t0412-promisor-no-lazy-fetch-recursion.sh b/t/t0412-promisor-no-lazy-fetch-recursion.sh
> new file mode 100755
> index 0000000000..ec203543d4
> --- /dev/null
> +++ b/t/t0412-promisor-no-lazy-fetch-recursion.sh
> @@ -0,0 +1,49 @@
> +#!/bin/sh
> +
> +test_description='promisor-remote: no recursive lazy-fetch
> +
> +Verify that fetch_objects() sets GIT_NO_LAZY_FETCH=1 in the child
> +fetch environment, so that index-pack cannot recurse back into
> +fetch_objects() when resolving REF_DELTA bases.
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> + test_create_repo server &&
> + test_commit -C server foo &&
> + git -C server repack -a -d --write-bitmap-index &&
> +
> + git clone "file://$(pwd)/server" client &&
> + HASH=$(git -C client rev-parse foo) &&
> + rm -rf client/.git/objects/* &&
> +
> + git -C client config core.repositoryformatversion 1 &&
> + git -C client config extensions.partialclone "origin"
> +'
> +
> +test_expect_success 'lazy-fetch spawns only one fetch subprocess' '
> + GIT_TRACE="$(pwd)/trace" git -C client cat-file -p "$HASH" &&
> +
> + grep "git fetch" trace >fetches &&
> + test_line_count = 1 fetches
> +'
> +
> +test_expect_success 'child of lazy-fetch has GIT_NO_LAZY_FETCH=1' '
> + rm -rf client/.git/objects/* &&
> +
> + # Install a reference-transaction hook to record the env var
> + # as seen by processes inside the child fetch.
> + test_hook -C client reference-transaction <<-\EOF &&
> + echo "$GIT_NO_LAZY_FETCH" >>../env-in-child
> + EOF
> +
> + rm -f env-in-child &&
> + git -C client cat-file -p "$HASH" &&
> +
> + # The hook runs inside the child fetch, which should have
> + # GIT_NO_LAZY_FETCH=1 in its environment.
> + grep "^1$" env-in-child
> +'
> +
> +test_done
>
> base-commit: 7b2bccb0d58d4f24705bf985de1f4612e4cf06e5 |
7b72344 to
907ca7a
Compare
Contributor
Author
|
/submit |
|
Submitted as pull.2224.v2.git.git.1772648846009.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Paul Tarjan wrote on the Git mailing list (how to reply to this email): On Tue, Mar 4, 2026, Junio C Hamano wrote:
> I would suggest dropping these CC: lines from the proposed log
> message. As far as I can see, they do not have their intended
> effect; [*1*] does not show any of these folks listed on Cc:
Done, moved them to the PR description for GitGitGadget to pick up.
> I also do not see much point in duplicating
> most of what appears in the proposed log message here after the
> three dash line, but that is a separate story.
Cleaned up the PR description to avoid the duplication.
> Hmph, do we really need an entirely new test script file dedicated
> for this single liner change, instead of adding to some existing
> test script that already covers related topics (like promisors and
> lazy fetches from them)?
Moved the test into t0411-clone-from-partial.sh, which already has
the other lazy-fetch tests. Dropped the separate t0412 file and the
meson.build entry. |
|
User |
|
This patch series was integrated into seen via 059c38f. |
|
This branch is now known as |
|
This patch series was integrated into seen via edc1a4b. |
|
There was a status update in the "Cooking" section about the branch The mechanism to avoid recursive lazy-fetch from promisor remotes were not propagated properly to child "git fetch" processes, which has been corrected. Comments? source: <pull.2224.v2.git.git.1772648846009.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 6c26668. |
|
There was a status update in the "Cooking" section about the branch The mechanism to avoid recursive lazy-fetch from promisor remotes were not propagated properly to child "git fetch" processes, which has been corrected. Comments? source: <pull.2224.v2.git.git.1772648846009.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 60595a3. |
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Wed, Mar 04, 2026 at 06:27:25PM +0000, Paul Tarjan via GitGitGadget wrote:
> From: Paul Tarjan <github@paulisageek.com>
>
> fetch_objects() spawns a child `git fetch` to lazily fill in missing
> objects. That child's index-pack, when it receives a thin pack
> containing a REF_DELTA against a still-missing base, explicitly
> calls promisor_remote_get_direct() — which is fetch_objects() again.
> If the base is truly unavailable (e.g. because many refs in the
> local store point at objects that have been garbage-collected on the
> server), each recursive lazy-fetch can trigger another, leading to
> unbounded recursion with runaway disk and process consumption.
Is this a theoretical concern or a practical one? I would expect that
backfill fetches never cause the server side to send a pack with
REF_DELTA objects to nonexistent objects. And if they did they are
broken.
> The GIT_NO_LAZY_FETCH guard (introduced by e6d5479e7a (git: add
> --no-lazy-fetch option, 2021-08-31)) already exists at the top of
> fetch_objects(); the missing piece is propagating it into the child
> fetch's environment. Add that propagation so the child's
> index-pack, if it encounters a REF_DELTA against a missing base,
> hits the guard and fails fast instead of recursing.
>
> Depth-1 lazy fetch (the whole point of fetch_objects()) is
> unaffected: only the child and its descendants see the variable.
> With negotiationAlgorithm=noop the client advertises no "have"
> lines, so a well-behaved server sends requested objects
> un-deltified or deltified only against objects in the same pack;
> the child's index-pack should never need a depth-2 fetch. If it
> does, the server response was broken or the local store is already
> corrupt, and further fetching would not help.
Exactly, this here matches my understanding. The backfill fetches don't
perform negotiation, so we shouldn't ever see a thin pack in the first
place. What I don't yet understand is your comment about the depth-2
fetch -- when would we ever do that?
> This is the same bug shape that 3a1ea94a49 (commit-graph.c: no lazy
> fetch in lookup_commit_in_graph(), 2022-07-01) addressed at a
> different entry point.
I dunno, I think it's quite different overall. In the mentioned commit
we protect against a stale commit-graph, which is something that is
quite plausible to happen on the client side. But here we protect us
against a remote side that sends a packfile that violates specs, as far
as I understand.
> Add a test that verifies the child fetch environment contains
> GIT_NO_LAZY_FETCH=1 via a reference-transaction hook.
Hm. Can we craft a test that shows us the resulting failure in practice?
Testing for the environment variable feels like a bad proxy to me, as
I'd rather want to learn how Git would fail now.
> Signed-off-by: Paul Tarjan <github@paulisageek.com>
> ---
> promisor-remote: prevent recursive lazy-fetch during index-pack
>
> Propagate GIT_NO_LAZY_FETCH=1 into the child fetch spawned by
> fetch_objects() so that index-pack cannot recurse back into lazy-fetch
> when resolving REF_DELTA bases.
>
> We hit this in production: 276 GB of promisor packs written in 90
> minutes against a 100 GB monorepo with ~61K stale prefetch refs pointing
> at GC'd commits.
Okay, so this seems to be an issue that can be hit in the wild. But I
have to wonder whether this really is a bug on the client-side, or
whether this is a bug that actually sits on your server. So ultimately:
why does the server send REF_DELTA objects in the first place? Is it
using git-upload-pack(1), or is it using a different implementation of
Git to serve data?
Note that I'm not arguing that we shouldn't have protection on the
client, too. But I'd first like to understand whether there is a bug
lurking somewhere that causes us to send invalid packfiles.
Patrick |
|
User |
907ca7a to
e1a1ec7
Compare
fetch_objects() spawns a child `git fetch` to lazily fill in missing objects. That child's index-pack, when it receives a thin pack containing a REF_DELTA against a still-missing base, calls promisor_remote_get_direct() -- which is fetch_objects() again. With negotiationAlgorithm=noop the client advertises no "have" lines, so a well-behaved server sends requested objects un-deltified or deltified only against objects in the same pack. A server that nevertheless sends REF_DELTA against a base the client does not have is misbehaving; however the client should not recurse unboundedly in response. Propagate GIT_NO_LAZY_FETCH=1 into the child fetch's environment so that if the child's index-pack encounters such a REF_DELTA, it hits the existing guard at the top of fetch_objects() and fails fast instead of recursing. Depth-1 lazy fetch (the whole point of fetch_objects()) is unaffected: only the child and its descendants see the variable. Add a test that injects a thin pack containing a REF_DELTA against a missing base via HTTP, triggering the recursion path through index-pack's promisor_remote_get_direct() call. With the fix, the child's fetch_objects() sees GIT_NO_LAZY_FETCH=1 and blocks the depth-2 fetch with a "lazy fetching disabled" warning. Signed-off-by: Paul Tarjan <github@paulisageek.com>
e1a1ec7 to
d58ccb8
Compare
Contributor
Author
|
/submit |
|
Submitted as pull.2224.v3.git.git.1773238778894.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Paul Tarjan wrote on the Git mailing list (how to reply to this email): Patrick Steinhardt <ps@pks.im> writes:
> Is this a theoretical concern or a practical one? I would expect that
> backfill fetches never cause the server side to send a pack with
> REF_DELTA objects to nonexistent objects. And if they did they are
> broken.
Practical. We hit this at Anthropic: 276 GB of promisor packs written
by `git maintenance --task=prefetch` in 90 minutes against a ~10 GB
monorepo with ~61K stale prefetch refs pointing at GC'd commits.
> Exactly, this here matches my understanding. The backfill fetches don't
> perform negotiation, so we shouldn't ever see a thin pack in the first
> place. What I don't yet understand is your comment about the depth-2
> fetch -- when would we ever do that?
The code path already exists and is tested: t5616 line 832 ("tolerate
server sending REF_DELTA against missing promisor objects") creates
exactly this scenario. index-pack's fix_unresolved_deltas() calls
promisor_remote_get_direct() when it encounters a REF_DELTA against a
missing base (builtin/index-pack.c:1508). That's the depth-2 fetch.
With noop negotiation a well-behaved server shouldn't send REF_DELTA
against objects the client doesn't have. But partial clones with
blob:none mean the client is missing most blobs, and if the server
sends a thin pack deltified against one of those filtered-out blobs,
index-pack will try to fetch the base.
> I dunno, I think it's quite different overall. In the mentioned commit
> we protect against a stale commit-graph, which is something that is
> quite plausible to happen on the client side. But here we protect us
> against a remote side that sends a packfile that violates specs, as far
> as I understand.
Fair point. The commit-graph case is purely client-side corruption,
while this requires a misbehaving server. The bug shape is the same
(unbounded recursion through fetch_objects()) but the trigger is
different. I'll drop the comparison in the next version.
> Hm. Can we craft a test that shows us the resulting failure in practice?
> Testing for the environment variable feels like a bad proxy to me, as
> I'd rather want to learn how Git would fail now.
Good point. Reworked the test in v3. It now injects a thin pack
containing a REF_DELTA against a missing base via HTTP (using the
replace_packfile pattern from t5616). This triggers the actual
recursion path: index-pack encounters the missing base, calls
promisor_remote_get_direct(), which hits the GIT_NO_LAZY_FETCH=1
guard and fails with "lazy fetching disabled". Without the fix,
the depth-2 fetch would proceed and potentially recurse.
> Okay, so this seems to be an issue that can be hit in the wild. But I
> have to wonder whether this really is a bug on the client-side, or
> whether this is a bug that actually sits on your server. So ultimately:
> why does the server send REF_DELTA objects in the first place? Is it
> using git-upload-pack(1), or is it using a different implementation of
> Git to serve data?
The server is GitHub. I did a blob:none partial clone and after some
further git operations ended up in this state. I don't have
server-side data on why it sent REF_DELTAs against missing bases.
> Note that I'm no
C958
t arguing that we shouldn't have protection on the
> client, too. But I'd first like to understand whether there is a bug
> lurking somewhere that causes us to send invalid packfiles.
Agreed, there may well be a server-side bug here. Regardless, the
client should fail fast rather than consume unbounded resources. |
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Wed, Mar 11, 2026 at 08:18:46AM -0600, Paul Tarjan wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > Is this a theoretical concern or a practical one? I would expect that
> > backfill fetches never cause the server side to send a pack with
> > REF_DELTA objects to nonexistent objects. And if they did they are
> > broken.
>
> Practical. We hit this at Anthropic: 276 GB of promisor packs written
> by `git maintenance --task=prefetch` in 90 minutes against a ~10 GB
> monorepo with ~61K stale prefetch refs pointing at GC'd commits.
I must be misunderstanding something here, but how is it that a commit
can be garbage collected if a ref points to it? That shouldn't ever
happen, as reachable commits should not be pruned.
Or do you mean to say that the commits don't exist on the server side
anymore?
> > Hm. Can we craft a test that shows us the resulting failure in practice?
> > Testing for the environment variable feels like a bad proxy to me, as
> > I'd rather want to learn how Git would fail now.
>
> Good point. Reworked the test in v3. It now injects a thin pack
> containing a REF_DELTA against a missing base via HTTP (using the
> replace_packfile pattern from t5616). This triggers the actual
> recursion path: index-pack encounters the missing base, calls
> promisor_remote_get_direct(), which hits the GIT_NO_LAZY_FETCH=1
> guard and fails with "lazy fetching disabled". Without the fix,
> the depth-2 fetch would proceed and potentially recurse.
Great, thanks.
> > Okay, so this seems to be an issue that can be hit in the wild. But I
> > have to wonder whether this really is a bug on the client-side, or
> > whether this is a bug that actually sits on your server. So ultimately:
> > why does the server send REF_DELTA objects in the first place? Is it
> > using git-upload-pack(1), or is it using a different implementation of
> > Git to serve data?
>
> The server is GitHub. I did a blob:none partial clone and after some
> further git operations ended up in this state. I don't have
> server-side data on why it sent REF_DELTAs against missing bases.
That's certainly curious. Do you maybe have multiple remotes attached to
the repository, or are you dropping/modifying the object filter at some
point?
All subsequent fetches need to use the same object filter as you've used
during the initial clone, otherwise you may run into a situation as you
have described. But in theory, Git knows to continue using the filter.
> > Note that I'm not arguing that we shouldn't have protection on the
> > client, too. But I'd first like to understand whether there is a bug
> > lurking somewhere that causes us to send invalid packfiles.
>
> Agreed, there may well be a server-side bug here. Regardless, the
> client should fail fast rather than consume unbounded resources.
Probably, yes. What I'm trying to figure out is whether there are edge
cases here where it's _valid_ for the server to send a thin pack with a
REF_DELTA. Because if so, unconditionally disabling the lazy fetches
would break such edge cases.
I don't think there are such cases, but I wouldn't consider myself an
expert with partial clones.
Cc'd Peff, as he's implemented a couple fixes in this area a couple
years ago.
Patrick |
|
This patch series was integrated into seen via b9f4303. |
|
There was a status update in the "Cooking" section about the branch The mechanism to avoid recursive lazy-fetch from promisor remotes were not propagated properly to child "git fetch" processes, which has been corrected. Comments? source: <pull.2224.v3.git.git.1773238778894.gitgitgadget@gmail.com> |
|
Jeff King wrote on the Git mailing list (how to reply to this email): On Thu, Mar 12, 2026 at 08:27:05AM +0100, Patrick Steinhardt wrote:
> > > Note that I'm not arguing that we shouldn't have protection on the
> > > client, too. But I'd first like to understand whether there is a bug
> > > lurking somewhere that causes us to send invalid packfiles.
> >
> > Agreed, there may well be a server-side bug here. Regardless, the
> > client should fail fast rather than consume unbounded resources.
>
> Probably, yes. What I'm trying to figure out is whether there are edge
> cases here where it's _valid_ for the server to send a thin pack with a
> REF_DELTA. Because if so, unconditionally disabling the lazy fetches
> would break such edge cases.
>
> I don't think there are such cases, but I wouldn't consider myself an
> expert with partial clones.
>
> Cc'd Peff, as he's implemented a couple fixes in this area a couple
> years ago.
Hmm, I'm not sure I have much wisdom. Here's the most plausible scenario
I could come up with.
A backfill fetch like this is going to have a noop negotiation
algorithm. So the server does not have any idea what the client has, and
therefore shouldn't be sending any thin deltas against it.
But it _can_ send deltas against objects which are part of the backfill
itself. Normally we'd send those as OFS_DELTA, because they're both in
the same output pack. But there are cases where we might not:
- if the client did not tell us it understands ofs-deltas; this would
not be true for any version of Git in the last 15+ years, but maybe
there is a bug in sending or parsing the capability? Or an alternate
Git implementation on the client side which forgets to send it?
- the verbatim pack-reuse code will sometimes rewrite ofs-delta into
ref-delta. I don't remember all of the cases where this might
happen. Certainly if the client hasn't claimed to support
ofs-deltas, but I think maybe some other cases? I'd have to dig into
it.
Now there's a catch: the pack is not really thin, and so index-pack
should not need to do an extra backfill request in order to get the base
object. But depending how index-pack is written, it might try to do so
anyway. If X is a delta against base Y, but Y is itself a delta, we
might not have resolved it yet. And so when we try to resolve X, we
think "aha, let us see if we have Y", and then eagerly attempt a
backfill fetch (probably triggered from odb_has_object() or similar).
When in fact the right thing to do is to queue X, resolve everything we
can, and see if we ended up with Y (actually index-pack works from the
bases up in the final resolution phase, but the effect is the same).
If that's what is happening, then I _think_ Paul's patch will do the
right thing. We'd say "no, we don't have that object" without doing the
backfill, and then eventually find it as part of the final resolution.
It would be nice to confirm that's what's going on, though (and it isn't
really a thin pack). If the problem can be reproduced, I don't suppose
we have a GIT_TRACE_PACKET output from a failing instance? That would
confirm that we're correctly using the noop negotiation.
-Peff |
|
User |
|
Paul Tarjan wrote on the Git mailing list (how to reply to this email): Jeff King <peff@peff.net> writes:
> It would be nice to confirm that's what's going on, though (and it isn't
> really a thin pack). If the problem can be reproduced, I don't suppose
> we have a GIT_TRACE_PACKET output from a failing instance? That would
> confirm that we're correctly using the noop negotiation.
Yeah. I couldn't capture one live during the incident, but I did a
clean-room reproduction afterward: empty bare repo, git remote add
origin pointing at the same GitHub monorepo, then the exact argv
from fetch_objects() with a tree SHA pulled from one of the
.promisor sidecars written during the incident:
$ git -c fetch.negotiationAlgorithm=noop fetch origin \
--no-tags --no-write-fetch-head --recurse-submodules=no \
--filter=blob:none --stdin <<< "$TREE_OID"
packet: fetch< version 2
packet: fetch< agent=git/github-94ec3a8682aa-Linux
packet: fetch< ls-refs=unborn
packet: fetch< fetch=shallow wait-for-done filter
packet: fetch< server-option
packet: fetch< object-format=sha1
packet: fetch< 0000
packet: fetch> command=fetch
packet: fetch> agent=git/2.51.2-Linux
packet: fetch> object-format=sha1
packet: fetch> 0001
packet: fetch> thin-pack
packet: fetch> no-progress
packet: fetch> ofs-delta
packet: fetch> filter blob:none
packet: fetch> want 086faa42a038f2e4a1b5e395cde544f8f3c6a9a5
packet: fetch> done
packet: fetch> 0000
packet: fetch< packfile
packet: sideband< PACK ...
packet: sideband< 0000
Zero have lines, noop negotiation doing its thing.
Two interesting bits:
1. The client sends thin-pack unconditionally (fetch-pack.c doesn't
check what negotiation algorithm is in use). So the server is
allowed to send a thin pack. With zero haves it has nothing to
thin against, but nothing on the wire actually forbids it.
2. filter blob:none is on the wire -- hardcoded by fetch_objects().
Combined with thin-pack, this gives the server a way to end up
omitting an object that happens to be a delta base.
For this particular tree GitHub responded cleanly (5 trees, no
deltas, 2701 bytes), so whatever triggers the REF_DELTA depends on
the specific objects being fetched and how they're stored
server-side.
> But it _can_ send deltas against objects which are part of the backfill
> itself. Normally we'd send those as OFS_DELTA, because they're both in
> the same output pack. But there are cases where we might not:
>
> - the verbatim pack-reuse code will sometimes rewrite ofs-delta into
> ref-delta. I don't remember all of the cases where this might
> happen. Certainly if the client hasn't claimed to support
> ofs-deltas, but I think maybe some other cases? I'd have to dig into
> it.
I think it's (b). What's interesting is that Jonathan Tan flagged
exactly this risk when he introduced the prefetch call.
8a30a1efd1 ("index-pack: prefetch missing REF_DELTA bases",
2019-05-14):
Support for lazy fetching should still generally be turned off
in index-pack because it is used as part of the lazy fetching
process itself (if not, infinite loops may occur), but we do
need to fetch the REF_DELTA bases. (When fetching REF_DELTA
bases, it is unlikely that those are REF_DELTA themselves,
because we do not send "have" when making such fetches.)
So the "infinite loops may occur" door was left open deliberately,
on the argument that it's unlikely. My patch just closes it.
Here's what I think went wrong in practice: fetch_objects()
hardcodes --filter=blob:none. When the want is a tree, the server
walks tree reachability and assembles potentially millions of tree
objects with no blobs. If pack-reuse passes through an object
stored server-side as OFS_DELTA against a blob, it comes out as
REF_DELTA and the blob base gets filtered out by blob:none.
I have a live ps snapshot from the incident showing the chain:
3719679 git -c fetch.negotiationAlgorithm=noop fetch origin \
--no-tags --no-write-fetch-head \
--recurse-submodules=no --filter=blob:none --stdin
3725911 git index-pack --stdin --fix-thin --keep=fetch-pack \
3719679 on <host> --promisor --pack_header=2,7535361
3726936 git -c fetch.negotiationAlgorithm=noop fetch origin \
--no-tags --no-write-fetch-head \
--recurse-submodules=no --filter=blob:none --stdin
--keep embeds the parent PID, so the chain is clear: lazy-fetch
(3719679) -> index-pack (3725911, 7,535,361 objects) -> depth-2
lazy-fetch (3726936). 7.5M objects is about right for wanting a
single tree with no haves and filter=blob:none -- you get the
entire commit+tree closure minus blobs. One REF_DELTA against a
filtered blob in 7.5M objects and you're recursing.
> Now there's a catch: the pack is not really thin, and so index-pack
> should not need to do an extra backfill request in order to get the base
> object.
Actually, the pack IS thin in this case -- the REF_DELTA's base is
a blob that the server filtered out because of blob:none. The base
isn't in the pack at all. fix_unresolved_deltas() correctly sees it
as unresolved and calls promisor_remote_get_direct().
> If that's what is happening, then I _think_ Paul's patch will do the
> right thing. We'd say "no, we don't have that object" without doing the
> backfill, and then eventually find it as part of the final resolution.
Right, that's what happens. The child hits the guard at the top of
fetch_objects() and bails with "lazy fetching disabled". The
depth-1 index-pack can't resolve the delta, which is the correct
outcome -- retrying the same noop fetch would just get the same
response.
I also wrote a deterministic local reproducer (no network needed)
that shows the recursion directly. Two scripts, needs python3 + git,
works on Linux and macOS. Ran it just now:
repro-depth1.sh sets up a local bare server with two similar blobs,
a blob:none partial-clone client, and a hand-crafted 68-byte thin
pack with one REF_DELTA against a promised-but-absent blob:
=== without GIT_NO_LAZY_FETCH (current git) ===
index-pack invocations: 2
lazy-fetch spawns: 1
=== with GIT_NO_LAZY_FETCH=1 (what the patch sets) ===
index-pack invocations: 1
lazy-fetch spawns: 0
repro-unbounded.sh goes further: uploadpack.packObjectsHook always
returns the same thin pack, so every lazy-fetch spawns another.
Watchdog kills it at depth 50:
=== without patch ===
recursion depth reached: 55
index-pack invocations: 55
tmp_pack_* on disk: 55
=== with GIT_NO_LAZY_FETCH=1 (the patch) ===
recursion depth: 0
index-pack invocations: 1
tmp_pack_* on disk: 0
Those tmp_pack_* files are the disk growth -- each index-pack
streams to a tmpfile, blocks in fix_unresolved_deltas() to
lazy-fetch the base, and never gets to rename it. In the real
incident each was ~1.4 GB.
Scripts below.
--- >8 --- repro-depth1.sh --- >8 ---
#!/bin/bash
# Deterministic depth-1 recursion proof for promisor-remote lazy-fetch bug.
# No network. Runs in ~1 second.
#
# Mechanism:
# - Local bare server with two similar blobs; client is a blob:none
# partial clone (both blobs promised, neither local).
# - Craft a 68-byte thin pack by hand: one REF_DELTA object whose base
# SHA is one of the promised-but-absent blobs.
# - Feed to `git -C client index-pack --stdin --fix-thin`.
# - fix_unresolved_deltas() sees the unresolved REF_DELTA, calls
# promisor_remote_get_direct() for the base, which spawns
# `git -c fetch.negotiationAlgorithm=noop fetch ... --stdin`.
# - That child fetch spawns its own index-pack --fix-thin.
# - GIT_TRACE shows the nested spawn.
set -euo pipefail
W="${TMPDIR:-/tmp}/promisor-recursion-demo"
rm -rf "$W"; mkdir -p "$W"; cd "$W"
git -c init.defaultBranch=main init -q --bare server.git
git -C server.git config uploadpack.allowFilter true
git -C server.git config uploadpack.allowAnySHA1InWant true
git -c init.defaultBranch=main init -q work
(
cd work
python3 -c "print('\n'.join('shared line %d ' * 4 % (i,i,i,i) for i in range(200)))" > f
git add f; git commit -qm base
python3 -c "print('\n'.join('shared line %d ' * 4 % (i,i,i,i) for i in range(200))); print('extra')" > f
git add f; git commit -qm delta
git push -q ../server.git main
)
printf '%s\n%s\n' "$(git -C work rev-parse HEAD~1:f)" "$(git -C work rev-parse HEAD:f)" | \
git -C server.git pack-objects --stdout --delta-base-offset > both.pack
python3 << 'PYEOF'
import struct, zlib, hashlib
with open('both.pack', 'rb') as f:
data = f.read()
assert data[:4] == b'PACK' and struct.unpack('>I', data[8:12])[0] == 2
def parse_hdr(data, pos):
b = data[pos]; otype = (b >> 4) & 7; sz = b & 0xf; sh = 4; p = pos
while b & 0x80:
p += 1; b = data[p]; sz |= (b & 0x7f) << sh; sh += 7
return otype, sz, p + 1
def zlib_end(data, pos):
d = zlib.decompressobj(); p = pos
while p < len(data):
chunk = data[p:p+512]; d.decompress(chunk)
if d.unused_data: return p + len(chunk) - len(d.unused_data)
if d.eof: return p + len(chunk)
p += len(chunk)
return p
t1, s1, d1 = parse_hdr(data, 12)
if t1 == 6:
p = d1; b = data[p]; p += 1
while b & 0x80: b = data[p]; p += 1
e1 = zlib_end(data, p)
else:
e1 = zlib_end(data, d1)
t2, s2, d2 = parse_hdr(data, e1)
if t1 == 3 and t2 == 6:
base_raw = zlib.decompress(data[d1:e1])
delta_sz, delta_hdr_end = s2, d2
elif t1 == 6 and t2 == 3:
base_raw = zlib.decompress(data[d2:zlib_end(data, d2)])
delta_sz, delta_hdr_end = s1, d1
else:
raise SystemExit(f"expected one blob + one OFS_DELTA, got types {t1},{t2}")
base_sha = hashlib.sha1(f"blob {len(base_raw)}\0".encode() + base_raw).digest()
p = delta_hdr_end; b = data[p]; p += 1
while b & 0x80: b = data[p]; p += 1
delta_zlib = data[p:zlib_end(data, p)]
sz = delta_sz
hb = [(7 << 4) | (sz & 0xf)]; sz >>= 4
while sz: hb[-1] |= 0x80; hb.append(sz & 0x7f); sz >>= 7
thin = b'PACK' + struct.pack('>II', 2, 1) + bytes(hb) + base_sha + delta_zlib
thin += hashlib.sha1(thin).digest()
with open('thin.pack', 'wb') as f: f.write(thin)
print(f"thin.pack: {len(thin)} bytes, REF_DELTA base={base_sha.hex()}")
PYEOF
git clone -q --no-local --filter=blob:none --no-checkout "file://$W/server.git" client
echo "=== without GIT_NO_LAZY_FETCH (current git) ==="
GIT_TRACE="$W/trace.txt" \
git -C client index-pack --stdin --fix-thin < thin.pack >/dev/null 2>&1 || true
grep -E 'built-in: git (index-pack|fetch)|run_command:.*negotiationAlgorithm' trace.txt
echo "index-pack invocations: $(grep -c 'built-in: git index-pack' trace.txt)"
echo "lazy-fetch spawns: $(grep -c 'run_command:.*negotiationAlgorithm=noop' trace.txt)"
echo ""
echo "=== with GIT_NO_LAZY_FETCH=1 (what the patch sets) ==="
rm -rf client
git clone -q --no-local --filter=blob:none --no-checkout "file://$W/server.git" client
GIT_NO_LAZY_FETCH=1 GIT_TRACE="$W/trace2.txt" \
git -C client index-pack --stdin --fix-thin < thin.pack 2>&1 || true
echo "index-pack invocations: $(grep -c 'built-in: git index-pack' trace2.txt)"
echo "lazy-fetch spawns: $(grep -c 'run_command:.*negotiationAlgorithm=noop' trace2.txt || echo 0)"
--- >8 --- repro-unbounded.sh --- >8 ---
#!/bin/bash
# Unbounded recursion proof. Run repro-depth1.sh first.
# uploadpack.packObjectsHook always returns the same thin pack,
# so every lazy-fetch spawns another. Watchdog kills at depth 50.
set -uo pipefail
W="${TMPDIR:-/tmp}/promisor-recursion-demo"
cd "$W"
test -f thin.pack || { echo "run repro-depth1.sh first"; exit 1; }
cat > evil-pack-objects.sh << EOF
#!/bin/sh
cat > /dev/null
cat "$W/thin.pack"
EOF
chmod +x evil-pack-objects.sh
cat > fake-global.gitconfig << EOF
[uploadpack]
packObjectsHook = $W/evil-pack-objects.sh
EOF
rm -rf client
GIT_CONFIG_GLOBAL= git clone -q --no-local --filter=blob:none --no-checkout \
"file://$W/server.git" client
TRACE="$W/unbounded.trace"
rm -f "$TRACE"
echo "=== spawning (watchdog kills at depth 50) ==="
GIT_TRACE="$TRACE" GIT_CONFIG_GLOBAL="$W/fake-global.gitconfig" \
git -C client index-pack --stdin --fix-thin < thin.pack >/dev/null 2>&1 &
ROOT=$!
LIMIT=50
while kill -0 "$ROOT" 2>/dev/null; do
N=$(grep -c 'run_command:.*negotiationAlgorithm=noop' "$TRACE" 2>/dev/null || true)
N=${N:-0}
if (( N >= LIMIT )); then
echo ">>> depth $N reached, killing process tree $ROOT <<<"
pkill -KILL -f "promisor-recursion-demo/client" 2>/dev/null || true
kill -KILL "$ROOT" 2>/dev/null || true
sleep 0.2
pkill -KILL -f "promisor-recursion-demo/client" 2>/dev/null || true
break
fi
sleep 0.02
done
wait "$ROOT" 2>/dev/null
EC=$?
DEPTH=$(grep -c 'run_command:.*negotiationAlgorithm=noop' "$TRACE" 2>/dev/null || true)
DEPTH=${DEPTH:-0}
IPACKS=$(grep -c 'built-in: git index-pack' "$TRACE" 2>/dev/null || true)
IPACKS=${IPACKS:-0}
TMPS=$(ls client/.git/objects/pack/tmp_pack_* 2>/dev/null | wc -l | tr -d ' ')
echo ""
echo "=== without patch (root exit $EC) ==="
echo "recursion depth reached: $DEPTH"
echo "index-pack invocations: $IPACKS"
echo "tmp_pack_* on disk: $TMPS"
echo ""
echo "first 4 + last 2 spawns (identical line, no termination condition):"
grep -E 'built-in: git index-pack|run_command:.*negotiationAlgorithm=noop' "$TRACE" | head -4
echo "..."
grep -E 'built-in: git index-pack|run_command:.*negotiationAlgorithm=noop' "$TRACE" | tail -2
echo ""
echo "=== with GIT_NO_LAZY_FETCH=1 (the patch) ==="
rm -rf client; rm -f "$TRACE"
GIT_CONFIG_GLOBAL= git clone -q --no-local --filter=blob:none --no-checkout \
"file://$W/server.git" client
GIT_NO_LAZY_FETCH=1 GIT_TRACE="$TRACE" GIT_CONFIG_GLOBAL="$W/fake-global.gitconfig" \
git -C client index-pack --stdin --fix-thin < thin.pack 2>&1 || true
CTRL_DEPTH=$(grep -c 'run_command:.*negotiationAlgorithm=noop' "$TRACE" 2>/dev/null || true)
CTRL_IPACKS=$(grep -c 'built-in: git index-pack' "$TRACE" 2>/dev/null || true)
CTRL_TMPS=$(ls client/.git/objects/pack/tmp_pack_* 2>/dev/null | wc -l | tr -d ' ')
echo "recursion depth: ${CTRL_DEPTH:-0}"
echo "index-pack invocations: ${CTRL_IPACKS:-0}"
echo "tmp_pack_* on disk: $CTRL_TMPS"
rm -rf "$W/client"
Paul |
|
Paul Tarjan wrote on the Git mailing list (how to reply to this email): Patrick Steinhardt <ps@pks.im> writes:
> I must be misunderstanding something here, but how is it that a commit
> can be garbage collected if a ref points to it? That shouldn't ever
> happen, as reachable commits should not be pruned.
>
> Or do you mean to say that the commits don't exist on the server side
> anymore?
Sloppy wording on my part — "GC'd" is wrong. These refs pointed at
commits that were promised but never materialized on the partial
clone. The ~77K broken refs looked like:
error: refs/prefetch/remotes/origin/claude/add-azure-dependencies-EaaDn \
does not point to a valid object!
They were created by git maintenance --task=prefetch, which runs
git fetch --prefetch --prune and writes refs/prefetch/remotes/origin/<branch>
pointing at the remote tip. On a blob:none partial clone it fetches
commit/tree metadata, but some referenced commits were never
actually downloaded before the upstream branches (ephemeral
CI/automation branches, force-pushed and deleted within days)
disappeared.
This is a red herring for the patch though. The stale prefetch refs
explain why the outer fetch got a thin pack — the client advertised
haves from promised-but-absent commits. But the recursion (depth-1
to depth-2+) is entirely inside fetch_objects() with noop
negotiation, independent of any refs.
I'll fix the commit message wording in a v4 if you'd like.
> That's certainly curious. Do you maybe have multiple remotes attached to
> the repository, or are you dropping/modifying the object filter at some
> point?
>
> All subsequent fetches need to use the same object filter as you've used
> during the initial clone, otherwise you may run into a situation as you
> have described. But in theory, Git knows to continue using the filter.
Nobody intentionally changed the filter. What happened is the
lazy-fetch child kept re-writing it. fetch_objects() hardcodes
--filter=blob:none on the child argv, and the child's
builtin/fetch.c writes the active filter to config.
23547c40 ("fetch: do not override partial clone filter", 2020)
guards this write behind a check for an already-set filter. But I
was unsetting remote.origin.partialclonefilter manually trying to
stop the storm, so the guard passed and the next lazy-fetch child
wrote it right back:
21:45 (unset) manual git config --unset
22:05 blob:none re-written by a lazy-fetch child
22:11 (unset) manual unset again
23:11 blob:none re-written again
23:13 (unset) manual unset
23:28 blob:none re-written (caught live by a config-mtime trap)
The actual mitigation was unsetting remote.origin.promisor too —
with no promisor remote, fix_unresolved_deltas() skips the prefetch
entirely.
This is arguably a separate bug: fetch_objects() should probably
pass -c remote.<name>.partialclonefilter=blob:none to override for
the single invocation, rather than --filter=blob:none which
persists to config. Not in scope for this patch, but I could follow
up separately if there's interest.
Paul |
|
This patch series was integrated into seen via 08b15fe. |
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.
Propagate GIT_NO_LAZY_FETCH=1 into the child fetch spawned by fetch_objects() so that index-pack cannot recurse back into lazy-fetch when resolving REF_DELTA bases.
We hit this in production: 276 GB of promisor packs written in 90 minutes against a ~10 GB monorepo with ~61K stale prefetch refs pointing at GC'd commits.
Changes since v2:
Changes since v1:
cc: Christian Couder christian.couder@gmail.com
cc: Han Xin hanxin.hx@bytedance.com
cc: Paul Tarjan paul@paultarjan.com
cc: Patrick Steinhardt ps@pks.im
cc: Jeff King peff@peff.net