threads / patch / 62347

patchbuiltin/credential-cache--daemon: fix error when "exit"ing on Cygwin

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

## tl;dr

22 messages between Oct 16, 2024 and May 8, 2025. Diffs are folded; open one to read it.

replies: 21people: 6as markdown or json

Patrick Steinhardt· Oct 16, 2024, 14:21 UTC · lore

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, see for example [1], but the exact root cause is not known.

As it turns out, we can avoid the issue by explicitly closing the client streams via fclose(3p). I'm not sure at all where the claimed atexit(3p) handler mentioned in the comment is supposed to live, but from all I can see we do not have any installed that would close the sockets for us. So this leaves me with a bit of a sour taste overall.

That being said, I couldn't spot anything obviously wrong with closing the streams ourselves, and it does fix the issue on Cygwin without any regressions on other platforms as far as I can see. So let's go for this fix, even though I cannot properly explain it.

[1]: https://github.com/cygporter/git/issues/51
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---

I've Cc'd Adam, who is the maintainer of the Git package in Cygwin, as well as Peff, who is the original author of the below comment. I'd be really happy if one of you could enlighten me here :)

Patrick
 builtin/credential-cache--daemon.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
Show changes to builtin/credential-cache--daemon.c +4 −6
diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index bc22f5c6d24..5a09df5c167 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -156,13 +156,11 @@ static void serve_one_client(FILE *in, FILE *out)
 	}
 	else if (!strcmp(action.buf, "exit")) {
 		/*
-		 * It's important that we clean up our socket first, and then
-		 * signal the client only once we have finished the cleanup.
-		 * Calling exit() directly does this, because we clean up in
-		 * our atexit() handler, and then signal the client when our
-		 * process actually ends, which closes the socket and gives
-		 * them EOF.
+		 * We must close our file handles before we exit such that the
+		 * client will receive an EOF.
 		 */
+		fclose(in);
+		fclose(out);
 		exit(0);
 	}
 	else if (!strcmp(action.buf, "erase"))
-- 
2.47.0.72.gef8ce8f3d4.dirty
Jeff King· Oct 16, 2024, 14:55 UTC · re: Patrick Steinhardt · lore

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

On Wed, Oct 16, 2024 at 04:21:36PM +0200, Patrick Steinhardt wrote:
Show 21 quoted lines
> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> index bc22f5c6d24..5a09df5c167 100644
> --- a/builtin/credential-cache--daemon.c
> +++ b/builtin/credential-cache--daemon.c
> @@ -156,13 +156,11 @@ static void serve_one_client(FILE *in, FILE *out)
>  	}
>  	else if (!strcmp(action.buf, "exit")) {
>  		/*
> -		 * It's important that we clean up our socket first, and then
> -		 * signal the client only once we have finished the cleanup.
> -		 * Calling exit() directly does this, because we clean up in
> -		 * our atexit() handler, and then signal the client when our
> -		 * process actually ends, which closes the socket and gives
> -		 * them EOF.
> +		 * We must close our file handles before we exit such that the
> +		 * client will receive an EOF.
>  		 */
> +		fclose(in);
> +		fclose(out);
>  		exit(0);
>  	}

This breaks the thing the comment was trying to protect against. We want to unlink() the socket file before closing the descriptors.

From 7d5e9c9849 (which you can find with blame or "git log --follow -Satexit builtin/credential-cache--daemon.c"):

    credential-cache--daemon: clarify "exit" action semantics
    When this code was originally written, there wasn't much
    thought given to the timing between a client asking for
    "exit", the daemon signaling that the action is done (with
    EOF), and the actual cleanup of the socket.
    However, we need to care about this so that our test scripts
    do not end up racy (e.g., by asking for an exit and checking
    that the socket was cleaned up). The code that is already
    there happens to behave very reasonably; let's add a comment
    to make it clear that any changes should retain the same
    behavior.

So with the proposed change, t0301 will now fail racily. We need that unlink() to happen before the fclose(). Just calling exit() does things in the right order, but it should also be OK to do an explicit:

  delete_tempfile(&socket_file);
  fclose(in);
  fclose(out);

That would probably require making socket_file a global variable. (You can't just return out of the serving loop, since that closes the sockets first before deleting the tempfile).

Show 15 quoted lines
> 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, see for example [1], but the exact root
> cause is not known.
> [...]
> [1]: https://github.com/cygporter/git/issues/51

I don't see any details at that issue, but I'm not sure how it would fix things. From the client's perspective, they are going to see the descriptor either way. Unless there is some magic that fclose() does that a normal descriptor-close-on-exit does not do.

That "Software caused connection abort" thing seems like a weird not-standard-Unix errno value. Searching for it mostly yields people complaining about getting it from ssh under cygwin. :)

If the magic that cygwin needs is actually "fclose before unlink", then that is in opposition to other platforms (and I suspect would make t0301 racy there).

Show 5 quoted lines
> As it turns out, we can avoid the issue by explicitly closing the client
> streams via fclose(3p). I'm not sure at all where the claimed atexit(3p)
> handler mentioned in the comment is supposed to live, but from all I can
> see we do not have any installed that would close the sockets for us. So
> this leaves me with a bit of a sour taste overall.

The mention of atexit is a little oblique these days. We moved from a manual atexit handler to using the tempfile.c handler in 9e9033166b (credential-cache--daemon: use tempfile module, 2015-08-10).

> That being said, I couldn't spot anything obviously wrong with closing
> the streams ourselves, and it does fix the issue on Cygwin without any
> regressions on other platforms as far as I can see. So let's go for this
> fix, even though I cannot properly explain it.
Running t0301 with --stress on Linux failed for me after a minute or so.
-Peff
Jeff King· Oct 16, 2024, 15:09 UTC · re: Jeff King · lore

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

On Wed, Oct 16, 2024 at 10:55:40AM -0400, Jeff King wrote:
Show 22 quoted lines
> > 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, see for example [1], but the exact root
> > cause is not known.
> > [...]
> > [1]: https://github.com/cygporter/git/issues/51
> 
> I don't see any details at that issue, but I'm not sure how it would fix
> things. From the client's perspective, they are going to see the
> descriptor either way. Unless there is some magic that fclose() does
> that a normal descriptor-close-on-exit does not do.
> 
> That "Software caused connection abort" thing seems like a weird
> not-standard-Unix errno value. Searching for it mostly yields people
> complaining about getting it from ssh under cygwin. :)
> 
> If the magic that cygwin needs is actually "fclose before unlink", then
> that is in opposition to other platforms (and I suspect would make t0301
> racy there).
This all seemed eerily familiar. Try this thread:
  https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/

It looks like the conclusion was that we should adjust errno handling on the client side, but nobody ever followed up with an actual patch.

-Peff
Ramsay Jones· Oct 16, 2024, 15:39 UTC · re: Jeff King · lore

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

On 16/10/2024 16:09, Jeff King wrote:
Show 31 quoted lines
> On Wed, Oct 16, 2024 at 10:55:40AM -0400, Jeff King wrote:
> 
>>> 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, see for example [1], but the exact root
>>> cause is not known.
>>> [...]
>>> [1]: https://github.com/cygporter/git/issues/51
>>
>> I don't see any details at that issue, but I'm not sure how it would fix
>> things. From the client's perspective, they are going to see the
>> descriptor either way. Unless there is some magic that fclose() does
>> that a normal descriptor-close-on-exit does not do.
>>
>> That "Software caused connection abort" thing seems like a weird
>> not-standard-Unix errno value. Searching for it mostly yields people
>> complaining about getting it from ssh under cygwin. :)
>>
>> If the magic that cygwin needs is actually "fclose before unlink", then
>> that is in opposition to other platforms (and I suspect would make t0301
>> racy there).
> 
> This all seemed eerily familiar. Try this thread:
> 
>   https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/
> 
> It looks like the conclusion was that we should adjust errno handling on
> the client side, but nobody ever followed up with an actual patch.

Heh, our emails crossed. Yes, I was hoping that, given that Adam had identified the cygwin commit that caused the regression, some resolution on the cygwin side would fix things up. I waited ... and then put it on my TODO list! :)

I did look at the cygwin commit and it wasn't at all obvious what happened. In fact there was a comment about making sure that errno values didn't change as a side-effect!

Sorry for being tardy, again ...

ATB, Ramsay Jones

Taylor Blau· Oct 16, 2024, 20:28 UTC · re: Jeff King · lore

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

On Wed, Oct 16, 2024 at 11:09:22AM -0400, Jeff King wrote:
Show 31 quoted lines
> On Wed, Oct 16, 2024 at 10:55:40AM -0400, Jeff King wrote:
>
> > > 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, see for example [1], but the exact root
> > > cause is not known.
> > > [...]
> > > [1]: https://github.com/cygporter/git/issues/51
> >
> > I don't see any details at that issue, but I'm not sure how it would fix
> > things. From the client's perspective, they are going to see the
> > descriptor either way. Unless there is some magic that fclose() does
> > that a normal descriptor-close-on-exit does not do.
> >
> > That "Software caused connection abort" thing seems like a weird
> > not-standard-Unix errno value. Searching for it mostly yields people
> > complaining about getting it from ssh under cygwin. :)
> >
> > If the magic that cygwin needs is actually "fclose before unlink", then
> > that is in opposition to other platforms (and I suspect would make t0301
> > racy there).
>
> This all seemed eerily familiar. Try this thread:
>
>   https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/
>
> It looks like the conclusion was that we should adjust errno handling on
> the client side, but nobody ever followed up with an actual patch.

Thanks for digging. It would be great if you both and Ramsay could unify on an agreeable path forward here.

In the meantime, I picked this up as 'ps/credential-cache-exit-cygwin' in my tree, but let's hold it out from 'seen' as you note it racily fails t0301.

Thanks, Taylor

Jeff King· Oct 17, 2024, 02:33 UTC · re: Taylor Blau · lore

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

On Wed, Oct 16, 2024 at 04:28:49PM -0400, Taylor Blau wrote:
Show 9 quoted lines
> > This all seemed eerily familiar. Try this thread:
> >
> >   https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/
> >
> > It looks like the conclusion was that we should adjust errno handling on
> > the client side, but nobody ever followed up with an actual patch.
> 
> Thanks for digging. It would be great if you both and Ramsay could unify
> on an agreeable path forward here.

I think the patch Ramsay posted elsewhere is the right way forward. Hopefully he can fill out a commit message with the summary and then we can proceed.

(I'm happy to help with explaining the credential-cache side, but I would just be hand-waving on the cygwin parts).

-Peff
Patrick Steinhardt· Oct 17, 2024, 08:46 UTC · re: Jeff King · lore

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

On Wed, Oct 16, 2024 at 10:33:19PM -0400, Jeff King wrote:
Show 18 quoted lines
> On Wed, Oct 16, 2024 at 04:28:49PM -0400, Taylor Blau wrote:
> 
> > > This all seemed eerily familiar. Try this thread:
> > >
> > >   https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/
> > >
> > > It looks like the conclusion was that we should adjust errno handling on
> > > the client side, but nobody ever followed up with an actual patch.
> > 
> > Thanks for digging. It would be great if you both and Ramsay could unify
> > on an agreeable path forward here.
> 
> I think the patch Ramsay posted elsewhere is the right way forward.
> Hopefully he can fill out a commit message with the summary and then we
> can proceed.
> 
> (I'm happy to help with explaining the credential-cache side, but I
> would just be hand-waving on the cygwin parts).

Sounds great to me -- in that case, let's just drop my patch. I was basically just trying to get somebody to have a look at the issue, and it very much seems like I succeeded :)

Ramsay, do you want to polish your patch with a commit message or shall I pick it up and iterate on it?

Thanks all for chiming in!
Patrick
Ramsay Jones· Oct 17, 2024, 22:58 UTC · re: Patrick Steinhardt · lore

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

On 17/10/2024 09:46, Patrick Steinhardt wrote:
Show 26 quoted lines
> On Wed, Oct 16, 2024 at 10:33:19PM -0400, Jeff King wrote:
>> On Wed, Oct 16, 2024 at 04:28:49PM -0400, Taylor Blau wrote:
>>
>>>> This all seemed eerily familiar. Try this thread:
>>>>
>>>>   https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/
>>>>
>>>> It looks like the conclusion was that we should adjust errno handling on
>>>> the client side, but nobody ever followed up with an actual patch.
>>>
>>> Thanks for digging. It would be great if you both and Ramsay could unify
>>> on an agreeable path forward here.
>>
>> I think the patch Ramsay posted elsewhere is the right way forward.
>> Hopefully he can fill out a commit message with the summary and then we
>> can proceed.
>>
>> (I'm happy to help with explaining the credential-cache side, but I
>> would just be hand-waving on the cygwin parts).
> 
> Sounds great to me -- in that case, let's just drop my patch. I was
> basically just trying to get somebody to have a look at the issue, and
> it very much seems like I succeeded :)
> 
> Ramsay, do you want to polish your patch with a commit message or shall
> I pick it up and iterate on it?
I can't get to it before the weekend, at the earliest, sorry! :(
If you fancy picking it up, please be my guest! :)

ATB, Ramsay Jones

Patrick Steinhardt· Oct 18, 2024, 04:36 UTC · re: Ramsay Jones · lore

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

On Thu, Oct 17, 2024 at 11:58:33PM +0100, Ramsay Jones wrote:
Show 33 quoted lines
> 
> 
> On 17/10/2024 09:46, Patrick Steinhardt wrote:
> > On Wed, Oct 16, 2024 at 10:33:19PM -0400, Jeff King wrote:
> >> On Wed, Oct 16, 2024 at 04:28:49PM -0400, Taylor Blau wrote:
> >>
> >>>> This all seemed eerily familiar. Try this thread:
> >>>>
> >>>>   https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/
> >>>>
> >>>> It looks like the conclusion was that we should adjust errno handling on
> >>>> the client side, but nobody ever followed up with an actual patch.
> >>>
> >>> Thanks for digging. It would be great if you both and Ramsay could unify
> >>> on an agreeable path forward here.
> >>
> >> I think the patch Ramsay posted elsewhere is the right way forward.
> >> Hopefully he can fill out a commit message with the summary and then we
> >> can proceed.
> >>
> >> (I'm happy to help with explaining the credential-cache side, but I
> >> would just be hand-waving on the cygwin parts).
> > 
> > Sounds great to me -- in that case, let's just drop my patch. I was
> > basically just trying to get somebody to have a look at the issue, and
> > it very much seems like I succeeded :)
> > 
> > Ramsay, do you want to polish your patch with a commit message or shall
> > I pick it up and iterate on it?
> 
> I can't get to it before the weekend, at the earliest, sorry! :(
> 
> If you fancy picking it up, please be my guest! :)

I've sent v2 using that hack now, noting your original authorship. I want to finally get on top of all of those platform-specific failures :)

Thanks!
Patrick
Taylor Blau· Oct 17, 2024, 20:54 UTC · re: Jeff King · lore

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

On Wed, Oct 16, 2024 at 10:33:19PM -0400, Jeff King wrote:
Show 15 quoted lines
> On Wed, Oct 16, 2024 at 04:28:49PM -0400, Taylor Blau wrote:
>
> > > This all seemed eerily familiar. Try this thread:
> > >
> > >   https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/
> > >
> > > It looks like the conclusion was that we should adjust errno handling on
> > > the client side, but nobody ever followed up with an actual patch.
> >
> > Thanks for digging. It would be great if you both and Ramsay could unify
> > on an agreeable path forward here.
>
> I think the patch Ramsay posted elsewhere is the right way forward.
> Hopefully he can fill out a commit message with the summary and then we
> can proceed.
Yeah, that's exactly what I was hoping for ;-).

Thanks, Taylor

Ramsay Jones· Oct 16, 2024, 15:12 UTC · re: Patrick Steinhardt · lore

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

On 16/10/2024 15:21, Patrick Steinhardt wrote:
Show 61 quoted lines
> 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, see for example [1], but the exact root
> cause is not known.
> 
> As it turns out, we can avoid the issue by explicitly closing the client
> streams via fclose(3p). I'm not sure at all where the claimed atexit(3p)
> handler mentioned in the comment is supposed to live, but from all I can
> see we do not have any installed that would close the sockets for us. So
> this leaves me with a bit of a sour taste overall.
> 
> That being said, I couldn't spot anything obviously wrong with closing
> the streams ourselves, and it does fix the issue on Cygwin without any
> regressions on other platforms as far as I can see. So let's go for this
> fix, even though I cannot properly explain it.
> 
> [1]: https://github.com/cygporter/git/issues/51
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> 
> I've Cc'd Adam, who is the maintainer of the Git package in Cygwin, as
> well as Peff, who is the original author of the below comment. I'd be
> really happy if one of you could enlighten me here :)
> 
> Patrick
> 
>  builtin/credential-cache--daemon.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> index bc22f5c6d24..5a09df5c167 100644
> --- a/builtin/credential-cache--daemon.c
> +++ b/builtin/credential-cache--daemon.c
> @@ -156,13 +156,11 @@ static void serve_one_client(FILE *in, FILE *out)
>  	}
>  	else if (!strcmp(action.buf, "exit")) {
>  		/*
> -		 * It's important that we clean up our socket first, and then
> -		 * signal the client only once we have finished the cleanup.
> -		 * Calling exit() directly does this, because we clean up in
> -		 * our atexit() handler, and then signal the client when our
> -		 * process actually ends, which closes the socket and gives
> -		 * them EOF.
> +		 * We must close our file handles before we exit such that the
> +		 * client will receive an EOF.
>  		 */
> +		fclose(in);
> +		fclose(out);
>  		exit(0);
>  	}
>  	else if (!strcmp(action.buf, "erase"))
Heh, this is very familiar! :)
See, for example, the mailing list discussion here[1].

If memory serves, Jeff was against this solution. For what it is worth, my recommended solution is '[RFC PATCH 1A] credential-cache: also handle ECONNABORTED connection closure'. I still have those patches in my local cygwin repo. Ah, let me just add the patch below (this was last rebased onto v2.46.0, but it should (hopefully) be fine to apply to master - famous last words).

ATB, Ramsay Jones

[1] https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/
-------->8--------
From 89526b2b87e39985dc8cd80661662ff087dc1078 Mon Sep 17 00:00:00 2001
From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Wed, 22 Jun 2022 19:24:44 +0100
Subject: [PATCH] credential-cache: also handle ECONNABORTED connection closure
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 builtin/credential-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/credential-cache.c +1 −1
diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c
index 3db8df70a9..defbcc845c 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)
-- 
2.47.0
Patrick Steinhardt· Oct 18, 2024, 04:36 UTC · re: Patrick Steinhardt · lore

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

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/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@ramsayjones.plus.com/

Original-patch-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/credential-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/credential-cache.c +1 −1
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)

base-commit: a7c6fbb48a5849db1bb1f841ef5403476fed0c0e
-- 
2.47.0.72.gef8ce8f3d4.dirty
Jeff King· Oct 18, 2024, 05:29 UTC · re: Patrick Steinhardt · lore

[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(-)
Show changes to builtin/credential-cache.c +1 −1
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)
-- 
2.47.0.273.g4ed1498264
Patrick Steinhardt· Oct 18, 2024, 05:32 UTC · re: Jeff King · lore

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

On Fri, Oct 18, 2024 at 01:29:52AM -0400, Jeff King wrote:
Show 13 quoted lines
> 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
Ramsay Jones· Oct 18, 2024, 15:33 UTC · re: Jeff King · lore

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

On 18/10/2024 06:29, Jeff King wrote:
Show 13 quoted lines
> 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.
Oh Wow, this is (no surprise) a masterpiece! :)
I would very happily add:
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>

(I don't think I could have produced anything half as good in several weekends!)

Thanks!

ATB, Ramsay Jones

Show 84 quoted lines
> 
> -- >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)
Taylor Blau· Oct 18, 2024, 21:17 UTC · re: Ramsay Jones · lore

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

On Fri, Oct 18, 2024 at 04:33:13PM +0100, Ramsay Jones wrote:
Show 22 quoted lines
>
>
> On 18/10/2024 06:29, 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.
>
> Oh Wow, this is (no surprise) a masterpiece! :)
>
> I would very happily add:
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Thanks, all.

Thanks, Taylor

Kristoffer Haugsbakk· Oct 18, 2024, 21:27 UTC · re: Jeff King · lore

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

On Fri, Oct 18, 2024, at 07:29, Jeff King wrote:
> [jk: commit message]
>
> Signed-off-by: Jeff King <peff@peff.net>

Just curious. Why this bracket comment (non-trailer line or whatever it is called) and not a regular trailer? All the bracket comments that I’ve seen give some comment, often about tweaking the patch or the commit message. In this case though the whole commit message is written by one person.

-- 
Kristoffer Haugsbakk
Jeff King· Oct 19, 2024, 21:21 UTC · re: Kristoffer Haugsbakk · lore

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

On Fri, Oct 18, 2024 at 11:27:50PM +0200, Kristoffer Haugsbakk wrote:
Show 10 quoted lines
> On Fri, Oct 18, 2024, at 07:29, Jeff King wrote:
> > [jk: commit message]
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> 
> Just curious.  Why this bracket comment (non-trailer line or whatever it
> is called) and not a regular trailer?  All the bracket comments that
> I’ve seen give some comment, often about tweaking the patch or the
> commit message.  In this case though the whole commit message is written
> by one person.

I assigned authorship to Ramsay, so my name is not otherwise mentioned, but appears in the signoff. So it was a way of mentioning what I contributed (both for credit, but also in case anybody has questions later).

I guess "Commit-message-by:" would work, too. ;) I guess you could likewise argue that I'm a co-author.

I think in the usual trailer order, it would be:
  Signed-off-by: Ramsay
  [jk: add commit message]
  Signed-off-by: me
but I didn't want to forge his S-o-b without asking first.
-Peff
Kristoffer Haugsbakk· Oct 20, 2024, 17:08 UTC · re: Jeff King · lore

Comment trailers vs. bracketed lines

Hi Peff
(editing the subject now which I intended to do from the start)
On Sat, Oct 19, 2024, at 23:21, Jeff King wrote:
Show 9 quoted lines
>> On Fri, Oct 18, 2024, at 07:29, Jeff King wrote:
>> […]
>
> I assigned authorship to Ramsay, so my name is not otherwise mentioned,
> but appears in the signoff. So it was a way of mentioning what I
> contributed (both for credit, but also in case anybody has questions
> later).
>
> I guess "Commit-message-by:" would work, too. ;)
I’ve done that when someone has given me a non-descript diff. :)
Show 7 quoted lines
> I think in the usual trailer order, it would be:
>
>   Signed-off-by: Ramsay
>   [jk: add commit message]
>   Signed-off-by: me
>
> but I didn't want to forge his S-o-b without asking first.

I’ve seen those brackets in the log. They used to happen with some regularity. At first it made sense since you need a free-form area to both comment and tell everyone that you left the comment. And a trailer doesn’t make sense for that, I thought.[1]

But thinking about the signoff requirement: you already have all the information you need from the next trailer, namely the signoff. In other words this:

    [kh: Added tests]
    Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
Has the same information as this:
    Comment: Added tests
    Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>

Because the signoff order tells you who left the comment. So I was wondering to myself why this uniform approach wasn’t used.

† 1: Since the brackets become “non-trailer values” or something
   (git-interpret-trailers(1)), i.e. the discarded parts of the trailer
   block
Kristoffer Haugsbakk· May 7, 2025, 16:41 UTC · re: Kristoffer Haugsbakk · lore

Re: Comment trailers vs. bracketed lines

Hi, this is an old thread:
On Sun, Oct 20, 2024, at 19:08, Kristoffer Haugsbakk wrote:
Show 44 quoted lines
> On Sat, Oct 19, 2024, at 23:21, Jeff King wrote:
>>> On Fri, Oct 18, 2024, at 07:29, Jeff King wrote:
>>> […]
>>
>> I assigned authorship to Ramsay, so my name is not otherwise mentioned,
>> but appears in the signoff. So it was a way of mentioning what I
>> contributed (both for credit, but also in case anybody has questions
>> later).
>>
>> I guess "Commit-message-by:" would work, too. ;)
>
> I’ve done that when someone has given me a non-descript diff. :)
>
>> I think in the usual trailer order, it would be:
>>
>>   Signed-off-by: Ramsay
>>   [jk: add commit message]
>>   Signed-off-by: me
>>
>> but I didn't want to forge his S-o-b without asking first.
>
> I’ve seen those brackets in the log.  They used to happen with some
> regularity.  At first it made sense since you need a free-form area to
> both comment and tell everyone that you left the comment.  And a trailer
> doesn’t make sense for that, I thought.[1]
>
> But thinking about the signoff requirement: you already have all the
> information you need from the next trailer, namely the signoff.  In
> other words this:
>
>     [kh: Added tests]
>     Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> Has the same information as this:
>
>     Comment: Added tests
>     Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> Because the signoff order tells you who left the comment.  So I was
> wondering to myself why this uniform approach wasn’t used.
>
> † 1: Since the brackets become “non-trailer values” or something
>    (git-interpret-trailers(1)), i.e. the discarded parts of the trailer
>    block
I was just reminded of this: https://lore.kernel.org/git/xmqqikmce67y.fsf@gitster.g/T/#m68c22c9b7dbc9b295e923a913e9d67e3ab28a2a4

I’m just doing a little bump of this topic in case anyone has any thoughts. I hope that is okay.

Jeff King· May 8, 2025, 20:24 UTC · re: Kristoffer Haugsbakk · lore

Re: Comment trailers vs. bracketed lines

On Wed, May 07, 2025 at 06:41:27PM +0200, Kristoffer Haugsbakk wrote:
Show 23 quoted lines
> > But thinking about the signoff requirement: you already have all the
> > information you need from the next trailer, namely the signoff.  In
> > other words this:
> >
> >     [kh: Added tests]
> >     Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> >
> > Has the same information as this:
> >
> >     Comment: Added tests
> >     Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> >
> > Because the signoff order tells you who left the comment.  So I was
> > wondering to myself why this uniform approach wasn’t used.
> >
> > † 1: Since the brackets become “non-trailer values” or something
> >    (git-interpret-trailers(1)), i.e. the discarded parts of the trailer
> >    block
> 
> I was just reminded of this: https://lore.kernel.org/git/xmqqikmce67y.fsf@gitster.g/T/#m68c22c9b7dbc9b295e923a913e9d67e3ab28a2a4
> 
> I’m just doing a little bump of this topic in case anyone has any
> thoughts.  I hope that is okay.

It is the same information _if_ there is another trailer with the ident that comes afterwards. In our project, you'd usually have a sign-off there. But not all projects would, and even in git.git you don't necessarily need one, depending on what is in the comment.

That said, if I were designing trailers from scratch today, I'd probably require something that looks more like "Comment:" just because it would reduce the complexity of the trailer parser (and its "non trailer portions"). But since we live in a world where we support that for historical reasons, I don't mind using it (I find it more natural to read, but possibly my mind is polluted by years of practice).

-Peff
Junio C Hamano· May 7, 2025, 19:20 UTC · re: Kristoffer Haugsbakk · lore

Re: Comment trailers vs. bracketed lines

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:
Show 27 quoted lines
>> I think in the usual trailer order, it would be:
>>
>>   Signed-off-by: Ramsay
>>   [jk: add commit message]
>>   Signed-off-by: me
>>
>> but I didn't want to forge his S-o-b without asking first.
>
> I’ve seen those brackets in the log.  They used to happen with some
> regularity.  At first it made sense since you need a free-form area to
> both comment and tell everyone that you left the comment.  And a trailer
> doesn’t make sense for that, I thought.[1]
>
> But thinking about the signoff requirement: you already have all the
> information you need from the next trailer, namely the signoff.  In
> other words this:
>
>     [kh: Added tests]
>     Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> Has the same information as this:
>
>     Comment: Added tests
>     Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> Because the signoff order tells you who left the comment.  So I was
> wondering to myself why this uniform approach wasn’t used.

Simply because we already saw another project like the kernel use the [initial: comment] convention, I think. The "25% rule" was originally added specifically to accomdate this kind of comments frequently used in the kernel project, if I am not mistaken.

We see too many 'jk' and 'js' in the project so the initial may not be all that meaningful if added and it usually is obvious who did what without, but even with three letter initial,

    [khh: Added tests]
    Comment: Added tests

the existing convention is still shorter than with your "Comment: " prefix ;-)

← back to recent threads