[v2] builtin/credential-cache--daemon: fix error when "exit"ing on Cygwin

Commit Message

Patrick Steinhardt Oct. 18, 2024, 4:36 a.m. UTC

Clients can signal the git-credential-cache(1) daemon that it is
supposed to exit by sending it an "exit" command. The details around
how exactly the daemon exits seem to be rather intricate as spelt out by
a comment surrounding our call to exit(3p), as we need to be mindful
around closing the client streams before we signal the client.

The logic is broken on Cygwin though: when a client asks the daemon to
exit, they won't see the EOF and will instead get an error message:

  fatal: read error from cache daemon: Software caused connection abort

This issue is known in Cygwin for quite a while already [1] [2] and
seems to come from a change in Cygwin itself. While we haven't figured
out what exact change that was, the effect is that we see ECONNABORTED
now instead of ECONNRESET when trying to read from the socket connected
to the daemon when the daemon has shut down. It is somewhat dubious that
errno changed in this way: read(3p) does not mention ECONNABORTED as a
possible error code, so the behaviour seems to not be POSIX-compliant.

We have discussed a couple of workarounds:

  - Explicitly close file streams when handling the "exit" action in
    `serve_one_client()`. This may easily lead to races because we need
    to be very careful about the order in which we delete sockets.

  - We can adapt `serve_one_client()` to not use exit(3) anymore, and
    instead handle the shutdown in the outer loop. This allows us to
    handle the shutdown elsewhere and be less intimate with the calling
    context.

  - Start handling ECONNABORTED.

The last option seems like it is both the easiest and least-risky fix.
It is unlikely that it would negatively affect any other platforms given
that read(3p) shouldn't even set the code in the first place, and it
fixes the issue on Cygwin.

Fix the issue by handling both ECONNRESET and ECONNABORTED.

[1]: https://github.com/cygporter/git/issues/51
[2]: https://lore.kernel.org/git/[e-mail]/

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


base-commit: a7c6fbb48a5849db1bb1f841ef5403476fed0c0e

Patch

diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c
index 5de8b9123bf..7789d57d3e1 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)