Re: [PATCH] credential-cache: treat ECONNABORTED like ECONNRESET

> On Fri, Oct 18, 2024 at 06:36:11AM +0200, Patrick Steinhardt wrote:
> 
>> Subject: builtin/credential-cache--daemon: fix error when "exit"ing on Cygwin
> 
> I think this commit message has a few unclear or inaccurate bits,
> because it's based on the earlier attempt. E.g., the change is now on
> the client side, not in credential-cache--daemon.
> 
> And I think rather than say "the daemon exit rules are intricate", we
> can actually outline the rules. :)
> 
> So here's what I had written after reading through the old thread. It
> would preferably get Ramsay's Signed-off-by before being applied.

> 
> -- >8 --
> From: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Subject: [PATCH] credential-cache: treat ECONNABORTED like ECONNRESET
> 
> On Cygwin, t0301 fails because "git credential-cache exit" returns a
> non-zero exit code. What's supposed to happen here is:
> 
>   1. The client (the "credential-cache" invocation above) connects to a
>      previously-spawned credential-cache--daemon.
> 
>   2. The client sends an "exit" command to the daemon.
> 
>   3. The daemon unlinks the socket and then exits, closing the
>      descriptor back to the client.
> 
>   4. The client sees EOF on the descriptor and exits successfully.
> 
> That works on most platforms, and even _used_ to work on Cygwin. But
> that changed in Cygwin's ef95c03522 (Cygwin: select: Fix FD_CLOSE
> handling, 2021-04-06). After that commit, the client sees a read error
> with errno set to ECONNABORTED, and it reports the error and dies.
> 
> It's not entirely clear if this is a Cygwin bug. It seems that calling
> fclose() on the filehandles pointing to the sockets is sufficient to
> avoid this error return, even though exiting should in general look the
> same from the client's perspective.
> 
> However, we can't just call fclose() here. It's important in step 3
> above to unlink the socket before closing the descriptor to avoid the
> race mentioned by 7d5e9c9849 (credential-cache--daemon: clarify "exit"
> action semantics, 2016-03-18). The client will exit as soon as it sees
> the descriptor close, and the daemon may or may not have actually
> unlinked the socket by then. That makes test code like this:
> 
>   git credential exit &&
>   test_path_is_missing .git-credential-cache
> 
> racy.
> 
> So we probably _could_ fix this by calling:
> 
>   delete_tempfile(&socket_file);
>   fclose(in);
>   fclose(out);
> 
> before we exit(). Or by replacing the exit() with a return up the stack,
> in which case the fclose() happens as we unwind. But in that case we'd
> still need to call delete_tempfile() here to avoid the race.
> 
> But simpler still is that we can notice that we already special-case
> ECONNRESET on the client side, courtesy of 1f180e5eb9 (credential-cache:
> interpret an ECONNRESET as an EOF, 2017-07-27). We can just do the same
> thing here (I suspect that prior to the Cygwin commit that introduced
> this problem, we were really just seeing ECONNRESET instead of
> ECONNABORTED, so the "new" problem is just the switch of the errno
> values).
> 
> There's loads more debugging in this thread:
> 
>   https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/
> 
> but I've tried to summarize the useful bits in this commit message.
> 
> [jk: commit message]
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/credential-cache.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c
> index 5de8b9123b..7789d57d3e 100644
> --- a/builtin/credential-cache.c
> +++ b/builtin/credential-cache.c
> @@ -30,7 +30,7 @@ static int connection_fatally_broken(int error)
>  
>  static int connection_closed(int error)
>  {
> -	return (error == ECONNRESET);
> +	return error == ECONNRESET || error == ECONNABORTED;
>  }
>  
>  static int connection_fatally_broken(int error)