[PATCH] credential-cache: treat ECONNABORTED like ECONNRESET

On Fri, Oct 18, 2024 at 06:36:11AM +0200, Patrick Steinhardt wrote:

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