From: Farid Zakaria <farid.m.zakaria@gmail.com>
Date: Wed, 04 Nov 2026 18:28:03 GMT
Subject: Re: [PATCH 2/2] credential-cache--daemon: do not take over a running daemon's socket
Message-ID: <DMMN0K2447KN.36D8GZ1EWAVP0@gmail.com>
In-Reply-To: <aumUyl6MQa43OFBs@pks.im>

On Tue Nov 3, 2026 at 12:01 AM PST, Patrick Steinhardt wrote:
> On Sun, Nov 01, 2026 at 03:29:26PM -0800, Farid Zakaria wrote:
>> When no daemon is running, two "git credential-cache store" run at the
>> same time each try to start one. unix_stream_listen() removes whatever
>> is at the socket path before it binds, so the second daemon takes over
>> the socket of the first. The first daemon keeps running, but no client
>> can reach it:
>> 
>>  - the credential its client stored is lost;
>> 
>>  - "git credential-cache exit" does not stop it, and it keeps the
>>    password in memory until it times out;
>> 
>>  - when it does exit, it removes the socket of the daemon that is
>>    still running, losing that daemon's credentials too.
>> 
>> The fix is to create the socket with unix_ss_create(), which the
>> simple-ipc server already uses.
>
> We usually write commit messages as if instructing the code to change.
> So: "Fix that by using `unix_ss_create()` instead...".
>

When I sent V2 I will fix this.

>> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
>> index 0dfa11e2ec..de6720c276 100644
>> --- a/builtin/credential-cache--daemon.c
>> +++ b/builtin/credential-cache--daemon.c
>> @@ -231,12 +232,26 @@ static int serve_cache_loop(int fd)
>>  static void serve_cache(const char *socket_path, int debug)
>>  {
>>  	struct unix_stream_listen_opts opts = UNIX_STREAM_LISTEN_OPTS_INIT;
>> -	int fd;
>> +	struct unix_ss_socket *server_socket;
>> +	struct tempfile *socket_file;
>> +	int ret;
>>  
>> -	fd = unix_stream_listen(socket_path, &opts);
>> -	if (fd < 0)
>> +	/*
>> +	 * Two clients may each start a daemon at the same time. Bind only
>> +	 * if no other daemon is listening yet; otherwise leave the socket
>> +	 * to that one, and our client will talk to it.
>> +	 */
>
> This explains what you do without giving much context why you do it, so
> this comment is somewhat useless. Most importantly...
>
>> +	ret = unix_ss_create(socket_path, &opts, -1, &server_socket);
>> +	if (ret == -2) {
>> +		printf("ok\n");
>> +		return;
>
> ... you mention that you write "ok" in your commit message, but you
> don't explain why that's the right thing to do. So why should the daemon
> pretend that everything was alright in the case where it fails to start?
>

The client in builtin/credential-cache.c:spawn_daemon() reads the
daemon's stdout and dies with "cache daemon did not start" unless it
gets "ok"

"ok" is part of the protocol between the client and the daemon. The
client expects to see "ok" and then it connects to the socket.

I can put this reasoning as a comment in the code. 
Following Peff's suggestion, I'll also exit(0) there
to make clear that this process is done.

>> +	}
>> +	if (ret < 0)
>>  		die_errno("unable to bind to '%s'", socket_path);
>>  
>> +	/* Remove the socket at exit only now that it is ours. */
>> +	socket_file = register_tempfile(socket_path);
>
> This comment doesn't really add much value, if you ask me.
>
> Patrick