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

[Jeff King <peff@peff.net>]

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