Re: [PATCH v2] promisor-remote: prevent lazy-fetch recursion in child fetch

[Patrick Steinhardt <ps@pks.im>]

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