I think this code here is breaking the emergency_recover test. Previously each message was followed by an io_wait, but now we only io_wait after 5 messages have been queued. So while the messages stay in order, the timing expectations of the test are different.

@rustyrussell I think I was able to reproduce the issue where we respond to a different ping mentioned here in #8308. Here's how I set it up:

• 20 concurrent workers
• 5 pings per worker
• Different ping sizes
• Very short delays (1 - 10ms) between pings

So for each ping we expected the pong to be of the same (similar-ish) size. We receive the corresponding pong (with the corect size) most of the time (98/100 times) however we encountered a race condition the other 2 times where we received the pong for a separate ping and the CLN protection did not kick in?

So the 3 scenarios were:

Clean Success:

• Worker 19.1 sends ping → CLN processes → correct pong returned

Race Condition:

• Worker 12.1 sends ping requesting 2000 bytes
• Worker X sends ping requesting 1000 bytes
• Pongs get mixed up
• Worker 12.1 gets Worker X's pong (1004 bytes instead of 2004)

Complete Rejection

• Worker 1.2 sends ping command
• CLN's protection finally kicks in: "expecting_pong != PONG_UNEXPECTED"
• lightning-cli gets failure response

I think the issue could be that the check on line 1423 is not atomic, so multiple workers can pass this check?

	if (peer->expecting_pong != PONG_UNEXPECTED) {
		daemon_conn_send(daemon->master,
				 take(towire_connectd_ping_reply(NULL,
								 false, 0)));
		return;
	}

And while the message queue itself is thread safe in inject_peer_msg(peer, take(ping)); I imagine multiple pings get queued to the same peer, but since we don't track which ping belongs to which pong we just deliver the first pong that arrives to whoever is waiting?

No, that's fine. This is the "we're already sending a ping, refuse to send another" case.

I have also reproduced this issue. I cannot (and neither can ChatGPT) see where we would dispatch more than one ping at once. I will have to put some data into the pings to try to trace what is happening!

Found it. subd_req() is inappropriate here, since replies from connectd are NOT FIFO. See final commit.