credential-cache: treat ECONNABORTED like ECONNRESET

Commit Message

Jeff King Oct. 18, 2024, 5:29 a.m. UTC

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 <[e-mail]>
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/[e-mail]/

but I've tried to summarize the useful bits in this commit message.

[jk: commit message]

Signed-off-by: Jeff King <[e-mail]>
---
 builtin/credential-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Patrick Steinhardt Oct. 18, 2024, 5:32 a.m. UTC | #1

On Fri, Oct 18, 2024 at 01:29:52AM -0400, Jeff King wrote:
> 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.

Works for me. I don't really care whose patch lands, I just want the
issue to be fixed :) Thanks!

Patrick

Patch

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)