e2e: Fix flakiness in `test_connection_crossing` test

Addresses intermittent failures in the `test_connection_crossing` e2e test,
which were particularly prevalent on slower CI environments (such as
`rust:trixie`).

Previously, the test spawned two threads to make Alice and Bob dial each
other concurrently and strictly asserted that the "preferred" peer (the
one with the higher Node ID) would always win the `Outbound` link direction.
However, this assumption is flawed in real-world, OS-level network execution
due to two race conditions:

1. Thread Scheduling: One thread could execute and fully establish a
   connection before the other thread even began processing its dial command.
2. Reactor Event Ordering: A node's reactor might wake up and process an
   incoming TCP connection from its peer *before* it processes the `Connect`
   command sent by the test. When it finally processes the `Connect` command,
   it sees a session already exists and skips dialing entirely.

In both scenarios, a true "simultaneous crossing" never occurs. Instead, a
standard sequential connection happens, meaning the link direction is dictated
by whoever dialed first, not by the "preferred" peer logic. This caused the
strict `left: Outbound, right: Inbound` assertions to panic.

To fix this, two changes were made:
- Introduced a `std::sync::Barrier` to synchronize the two test threads.
  This forces both threads to wait for each other before calling `.connect()`,
  maximizing the probability of a true simultaneous dial.
- Relaxed the final assertions. Because OS-level TCP handshakes and reactor
  polling can never guarantee perfect simultaneity, we no longer assert
  *which* peer gets the `Outbound` link. Instead, we assert the core invariant:
  that exactly one connection is established between the nodes, and that their
  link directions are opposite (`s1.link != s2.link`).
This commit is contained in:
Adrian Duke 2026-04-21 14:08:36 +01:00 committed by Fintan Halpenny
parent bbb1279604
commit bfb54bf4be
1 changed files with 14 additions and 7 deletions

View File

@ -873,10 +873,14 @@ fn test_connection_crossing() {
log::debug!(target: "test", "Preferred peer is {preferred}");
let barrier = std::sync::Arc::new(std::sync::Barrier::new(2));
let b1 = barrier.clone();
let b2 = barrier.clone();
let t1 = thread::spawn({
let mut alice = alice.handle.clone();
move || {
b1.wait();
alice
.connect(bob.id, bob.addr.into(), ConnectOptions::default())
.unwrap()
@ -885,6 +889,7 @@ fn test_connection_crossing() {
let t2 = thread::spawn({
let mut bob = bob.handle.clone();
move || {
b2.wait();
bob.connect(alice.id, alice.addr.into(), ConnectOptions::default())
.unwrap()
}
@ -913,13 +918,15 @@ fn test_connection_crossing() {
log::debug!(target: "test", "{:?}", alice.handle.sessions());
log::debug!(target: "test", "{:?}", bob.handle.sessions());
if preferred == alice.id {
assert_eq!(s1.link, radicle::node::Link::Outbound);
assert_eq!(s2.link, radicle::node::Link::Inbound);
} else {
assert_eq!(s1.link, radicle::node::Link::Inbound);
assert_eq!(s2.link, radicle::node::Link::Outbound);
}
// We assert that they have opposite link directions.
// In a true simultaneous crossing, the preferred peer wins the Outbound link.
// However, due to OS thread scheduling and reactor event ordering, one peer
// might fully establish the connection before the other even processes the dial command.
// In all valid cases (crossing or sequential), exactly one is Outbound and one is Inbound.
assert_ne!(
s1.link, s2.link,
"One must be Inbound and the other Outbound"
);
assert_eq!(alice_s.len(), 1);
assert_eq!(bob_s.len(), 1);
}