Re: [BUG] clone of large repo fails when server closes idle SSH session during "Resolving deltas"

From: Jeff King <peff@peff.net>
Date: May 19, 2021, 09:57 UTC
Message-ID: <YKTg8nYjSGpKbq8W@coredump.intra.peff.net>
In-Reply-To: <YKTbsByUaPEuDNtR@coredump.intra.peff.net>

On Wed, May 19, 2021 at 05:34:40AM -0400, Jeff King wrote:

Show 7 quoted lines

> Another side issue is that once the protocol conversation has finished,
> I'm not sure if it's really useful for us to detect and complain about
> ssh's exit code. We know the other side completed the conversation
> successfully, and we have nothing left to ask it. So a fix for your
> immediate pain would be to stop noticing that. I think the root issue is
> still worth addressing, though; we are tying up network and local
> resources with a useless to-be-closed ssh connection.

By the way, there's an interesting subtlety / bug related to this. While "git clone" does return a failed exit code in this case, it leaves the repository directory in place! And because no real error occurred with the clone, you can use it as usual (though I think if it's a non-bare clone, you'd need to run "git checkout" fill in the working tree).

Propagating the error code comes from aab179d937 (builtin/clone.c: don't ignore transport_fetch_refs() errors, 2020-12-03). So prior to Git v2.30.0, your case would kind-of work.

But:

  - I think that is just "clone"; a "git fetch" would always have
    propagated the error from transport_fetch_refs()

  - that commit was right to start propagating the error code from
    transport_fetch_refs(). While in this specific case, we happened to
    produce a useful repository directory, most other errors would not.

  - there _is_ a bug in aab179d937, though. When it sees the error it
    should clean up the repo directory. And that even happens
    automatically via an atexit() handler. But because rather than
    calling die() it jumps to cleanup code, it mistakenly sets the flag
    for "leave the directory in place".

  - any logic to ignore errors would have to be inside the transport
    code (i.e., it realizes that ssh exiting non-zero isn't a big deal
    anymore, and then transport_fetch_refs() still returns success).

-Peff