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

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

[Patrick Steinhardt]

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"))
-- 
2.47.0.72.gef8ce8f3d4.dirty

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

[Jeff King]

On Wed, Oct 16, 2024 at 04:21:36PM +0200, Patrick Steinhardt wrote:

> 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).

> 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).

> 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

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

[Jeff King]

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.

-Peff

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

[Ramsay Jones]

On 16/10/2024 16:09, Jeff King wrote:
> 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

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

[Taylor Blau]

On Wed, Oct 16, 2024 at 11:09:22AM -0400, Jeff King wrote:
> 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

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

[Jeff King]

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).

-Peff

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

[Patrick Steinhardt]

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?

Thanks all for chiming in!

Patrick

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

[Ramsay Jones]

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! :)

ATB,
Ramsay Jones

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

[Patrick Steinhardt]

On Thu, Oct 17, 2024 at 11:58:33PM +0100, Ramsay Jones wrote:
> 
> 
> 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

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

[Taylor Blau]

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.

Yeah, that's exactly what I was hoping for ;-).

Thanks,
Taylor

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

[Ramsay Jones]

On 16/10/2024 15:21, Patrick Steinhardt wrote:
> 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(-)

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

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

[Patrick Steinhardt]

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(-)

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

[PATCH] credential-cache: treat ECONNABORTED like ECONNRESET

[Jeff King]

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)
-- 
2.47.0.273.g4ed1498264

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

[Patrick Steinhardt]

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

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

[Ramsay Jones]

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>

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

Thanks!

ATB,
Ramsay Jones

> 
> -- >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)

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

[Taylor Blau]

On Fri, Oct 18, 2024 at 04:33:13PM +0100, Ramsay Jones wrote:
>
>
> 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

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

[Kristoffer Haugsbakk]

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

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

[Jeff King]

On Fri, Oct 18, 2024 at 11:27:50PM +0200, Kristoffer Haugsbakk wrote:

> 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

Comment trailers vs. bracketed lines

[Kristoffer Haugsbakk]

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:
>> 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

Re: Comment trailers vs. bracketed lines

[Kristoffer Haugsbakk]

Hi, this is an old thread:

On Sun, Oct 20, 2024, at 19:08, Kristoffer Haugsbakk wrote:
> 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.

Re: Comment trailers vs. bracketed lines

[Jeff King]

On Wed, May 07, 2025 at 06:41:27PM +0200, Kristoffer Haugsbakk wrote:

> > 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

Re: Comment trailers vs. bracketed lines

[Junio C Hamano]

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

>> 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 ;-)