Resolve simultaneous node-to-node connects deterministically

[Amaury Chamayou (achamayou)]

Fixes the node-to-node channel defect investigated in #8232. This is quite a corner case, perhaps we want to convert node to node channels to TLS over TCP first instead, but in case this is still some way away, this may be a good liveness fix.

Closes #8232

The problem

When two nodes dial each other at the same moment, each ends up holding two connections to the same peer: an outgoing one it created, and an incoming one the peer created. Today both nodes unconditionally prefer the incoming connection, so each destroys the socket the other is relying on. Both are left holding a connection whose far end no longer exists.

The existing code knows about this. associate_incoming carries a comment describing exactly this case and resolves it with an assumption:

Assumption is that they progress at different rates, and one of them eventually spots the dead connection and opens a new one which succeeds.

That assumption holds only because closing a socket normally sends a FIN which the peer observes as a disconnect. It does not hold when that notification is lost, which is precisely what a partition does. In the failure that prompted this, the two orphaned sockets were created during partition churn, the FIN was swallowed by an iptables DROP rule, and the pair never spoke again. A node then sat as a PreVoteCandidate for 20 seconds issuing six pre-votes that were encrypted, handed to the host, written into a dead socket and silently discarded, while the peer that could have voted for it logged nothing at all.

This is not test-only. Any silent blackhole - asymmetric firewall state, a NAT idle eviction, a cloud SDN drop - reaches the same state in production, and the result is a stalled election with no diagnostics.

The fix

Make the outcome deterministic instead of racy. Both nodes order the two connections by node ID: the lower ID keeps the connection it opened, the higher ID prefers the one its peer opened. Both sides compute the same answer from information they already have, so exactly one connection survives, both ends agree on which, and no orphan is ever created.

The host is never told its own node ID, but it appears in the sender field of every node_outbound message, and is only needed once an outgoing connection exists - which implies we have already sent. It is cached from there, so this needs no new plumbing or ringbuffer message.

The tie-break only defends a recently created outgoing connection. This qualification is essential rather than incidental. An incoming connection from a peer we already have a settled connection to means that peer believes the link is broken, and it is in a better position to know - it may have observed a failure we cannot see. Defending our own connection indefinitely would deny that peer the only means it has of repairing a link that is dead in a way we cannot detect, which is precisely the failure mode this change exists to prevent. So a connection young enough to still be part of a genuine race is defended, and anything older yields exactly as it does today. A real race is resolved within a round trip; the window is 2s.

Host connection lifecycle events are promoted from LOG_DEBUG_FMT to LOG_INFO_FMT. These fire a handful of times per node lifetime, and their absence at CI log level is what made the original incident require millisecond arithmetic on an idle timer and inference from missing log lines to diagnose.

Compatibility with nodes that do not have this change

There is no wire-format change: no new message types, no altered serialisation, no negotiation. Connection selection is a purely local decision, so an old node and a new node can always talk to each other.

The behaviour in a mixed cluster follows from one observation: the higher-ID node's behaviour is unchanged. It still prefers the incoming connection, exactly as today. Only the lower-ID node behaves differently. So for a pair of nodes:

Lower ID	Higher ID	Outcome
new	new	Fixed - one connection survives
new	old	Fixed - old node prefers incoming, new node keeps its outgoing, they agree
old	new	Unchanged - both prefer incoming, as today
old	old	Unchanged - as today

So the fix engages for exactly those pairs whose lower-ID node has been upgraded, is inert otherwise, and is fully effective once the cluster is upgraded. There is no ordering requirement on a rolling upgrade, and no state in which mixed versions are worse than the current behaviour.

Testing

src/host/test/node_connections.cpp is new, and is the first unit test for this file. NodeConnections is templated on its socket type - mirroring the existing RPCConnections<TCP> - so the test can drive it over a mock network. Production use is unchanged, via using NodeConnections = NodeConnectionsImpl<TCP>.

Testing this needs a mock rather than loopback sockets, and that is the point: on a healthy loopback the race self-corrects, because the FIN is delivered and the peer repairs. Reproducing the real failure requires suppressing connection teardown, which the mock does with a deliver_close flag standing in for the DROP rule. The mock also buffers writes issued before a connect completes, as libuv does via TCPImpl::pending_writes; without that a node's first message would be lost and would mask the behaviour under test.

Two cases, each verified to fail against the specific line of code it covers:

1. Simultaneous connect leaves a usable connection in both directions. Both nodes send before either has accepted, teardown is then suppressed, and a message must still get through both ways. With the tie-break reverted this reproduces the production symptom exactly - the first message arrives in each direction and every subsequent one vanishes:

   ERROR: CHECK( b.received > b_before ) is NOT correct!  values: CHECK( 1 > 1 )
   ERROR: CHECK( a.received > a_before ) is NOT correct!  values: CHECK( 1 > 1 )
   

2. A later incoming connection is accepted, so a peer can repair. A connects to B, then B alone observes that connection fail, leaving A holding a silently dead outgoing connection. B dials A, and A must accept it. With the age check removed this fails, confirming it covers the regression described above rather than passing incidentally:

   ERROR: CHECK( a.received > a_before ) is NOT correct!  values: CHECK( 0 > 0 )
   

Both pass against the fix:

[doctest] test cases:  2 |  2 passed | 0 failed | 0 skipped
[doctest] assertions: 10 | 10 passed | 0 failed |

src/host/run.cpp and src/host/test/rpc_connections.cpp still compile, and clang-format-18 is clean. CI is green across all five jobs, including the partitions job whose failure prompted the investigation.

What this does not do

It does not add liveness detection to an already-established channel. A channel can still be blackholed by other means, and CCF will still not notice, because NodeToNodeChannelManager resets a channel's idle timer on sends as well as receives - so a node talking into a void indefinitely refreshes its own timeout. In the original incident node 1's 16 s self-heal timer was reset six times by the very pre-votes that were failing. That deserves separating "is this channel in use" from "is this channel working", and is left for a follow-up; it is discussed in #8232.

It also does not change partitions_test.py. The test asserts a reasonable liveness property and the two previous stabilisation attempts on these same functions (#8132, #8197) tuned timing rather than fixing the framework.

Related

The throw e exception-slicing bug in messaging::Dispatcher::dispatch, found while writing these tests, was split out into #8235 and has now merged. This branch is rebased on top of it.

[Copilot]

Pull request overview

This PR fixes a liveness bug in CCF’s host-side node-to-node transport where simultaneous cross-connects could leave both nodes holding an orphaned socket (especially when disconnect notifications are blackholed during partitions), by making the “which connection to keep” decision deterministic. It also adds the first focused unit tests for NodeConnections, using a mock network to reproduce the failure mode reliably.

Changes:

• Make NodeConnections deterministic under simultaneous connect by tie-breaking on node ID, with a bounded “simultaneous connect” time window.
• Promote node connection lifecycle logs from DEBUG to INFO to improve diagnosability at CI log levels.
• Add a new mock-based unit test (node_connections_test) and a changelog entry for the fix.

Custom instructions used:

• None (no .github/copilot-instructions.md or .github/instructions/* files were loaded during this review)

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/host/node_connections.h	Deterministic resolution of simultaneous node-to-node connects (templated impl + ID-based tie-break + logging changes).
src/host/test/node_connections.cpp	New doctest unit tests with a mock network to reproduce and validate the fix.
CMakeLists.txt	Adds the new node_connections_test unit test target.
CHANGELOG.md	Documents the node-to-node simultaneous connect fix in the release notes.

Suppressed comments (1)

src/host/node_connections.h:454

• The connection failure log message just above has a duplicated word ("dropping outbound message message"), which looks like a typo and makes diagnostics harder to read.

              outbound_connection = connection_it->second.socket;

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.