mirror of
https://github.com/jmcorgan/fips.git
synced 2026-10-05 19:18:25 +00:00
fix(node): keep a half-built link when msg2 hits a transient transport
A send refused because the interface is absent or mid-rebind was treated as a failed handshake: the link was removed, the reverse-address entry dropped, the session index freed, the control machine torn down, the queued PromoteToActive aborted — and the whole thing recorded as RejectReason::Handshake(HandshakeReject::BadState). That counter means "the remote sent something invalid". A local interface flap is not the remote's fault, and an operator reading the rejects would conclude it was. The initiator, meanwhile, resends msg1 into a link that no longer exists and has to rebuild from nothing. The binder is already working to bring the interface back, so the half-built link is now left exactly where it is for that resend to land on. Nothing leaks by staying: an initiator that never resends leaves a stale connection, which `check_timeouts` reaps at `handshake_timeout_secs` like every other abandoned handshake. Only a genuinely terminal error still tears down. This is the first consumer of `TransportError::is_transient`, which is what the central classification was for — before it, the distinction did not survive as far as this call site. The rekey msg1 send site gets the severity half only. Its teardown was already benign: it returns before `set_rekey_state`, so the cycle simply does not start and is retried when rekey next comes due, with nothing torn down and nothing charged to the peer. Only the `warn!` was wrong for a local, self-clearing condition the presence machine has already reported. The test drives a real absent Ethernet transport rather than a stub, so the error under test is the one production raises, from the code path that raises it. Verified against the defect: with the transient branch disabled, the link is destroyed and the assertion fails.
This commit is contained in:
@@ -187,6 +187,31 @@ impl Node {
|
||||
None => None,
|
||||
};
|
||||
if let Some(e) = send_err {
|
||||
// A transient refusal is not a failed handshake. The
|
||||
// interface under this transport is absent or
|
||||
// mid-rebind, and the binder is already working to
|
||||
// bring it back — so the half-built link is left
|
||||
// exactly as it is for the initiator's msg1 resend to
|
||||
// land on, rather than being torn down and rebuilt.
|
||||
//
|
||||
// Tearing down here charged a *local* interface flap
|
||||
// to the remote: the reject counter it recorded means
|
||||
// "the peer sent something invalid", which is a
|
||||
// different thing entirely and one an operator reads
|
||||
// as the peer's fault.
|
||||
//
|
||||
// Nothing leaks by staying. An initiator that never
|
||||
// resends leaves a stale connection, which
|
||||
// `check_timeouts` reaps at `handshake_timeout_secs`
|
||||
// exactly as it reaps every other abandoned handshake.
|
||||
if e.is_transient() {
|
||||
debug!(
|
||||
link_id = %link,
|
||||
error = %e,
|
||||
"Deferred msg2: the transport is between interfaces"
|
||||
);
|
||||
return;
|
||||
}
|
||||
// Restored pre-refactor msg2-send-failure warn!
|
||||
// (`handle_msg1` L665): the send error text is surfaced
|
||||
// at the executor point where the failure is now handled.
|
||||
|
||||
@@ -373,11 +373,26 @@ impl Node {
|
||||
);
|
||||
}
|
||||
Err(e) => {
|
||||
warn!(
|
||||
peer = %self.peer_display_name(node_addr),
|
||||
error = %e,
|
||||
"Failed to send rekey msg1"
|
||||
);
|
||||
// The teardown here is already benign — this returns before
|
||||
// `set_rekey_state`, so the cycle simply does not start and
|
||||
// is retried when rekey next comes due, with nothing torn
|
||||
// down and nothing charged to the peer. Only the severity
|
||||
// is wrong for a transport between interfaces, which is a
|
||||
// local and self-clearing condition the presence machine
|
||||
// has already reported.
|
||||
if e.is_transient() {
|
||||
debug!(
|
||||
peer = %self.peer_display_name(node_addr),
|
||||
error = %e,
|
||||
"Deferred rekey msg1: the transport is between interfaces"
|
||||
);
|
||||
} else {
|
||||
warn!(
|
||||
peer = %self.peer_display_name(node_addr),
|
||||
error = %e,
|
||||
"Failed to send rekey msg1"
|
||||
);
|
||||
}
|
||||
let _ = self.index_allocator.free(our_index);
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -2215,3 +2215,82 @@ async fn a_first_epoch_change_against_a_silent_peering_still_restarts_it() {
|
||||
"the replacement must carry the epoch the msg1 announced"
|
||||
);
|
||||
}
|
||||
|
||||
/// A transient send failure during msg2 leaves the half-built link alone.
|
||||
///
|
||||
/// The interface under the transport is absent or mid-rebind, and the binder
|
||||
/// is already working to bring it back. Tearing the link down here meant the
|
||||
/// initiator's msg1 resend had nothing to land on, and — worse — recorded
|
||||
/// `HandshakeReject::BadState`, a counter whose whole meaning is "the remote
|
||||
/// sent something invalid". A local interface flap is not the remote's fault,
|
||||
/// and an operator reading that counter would conclude it was.
|
||||
///
|
||||
/// Nothing leaks by staying: an initiator that never resends leaves a stale
|
||||
/// connection, which `check_timeouts` reaps at `handshake_timeout_secs` like
|
||||
/// any other abandoned handshake.
|
||||
#[cfg(any(target_os = "linux", target_os = "macos"))]
|
||||
#[tokio::test]
|
||||
async fn a_transient_msg2_failure_keeps_the_link_for_the_retry() {
|
||||
use crate::config::EthernetConfig;
|
||||
use crate::proto::fmp::wire::build_msg1;
|
||||
use crate::transport::TransportHandle;
|
||||
use crate::transport::ethernet::EthernetTransport;
|
||||
|
||||
let mut node_b = make_node();
|
||||
let node_a = make_node();
|
||||
let transport_id = TransportId::new(1);
|
||||
node_b.supervisor.state = NodeState::Running;
|
||||
|
||||
// An interface no host has, so every send off this transport reports
|
||||
// `InterfaceUnavailable` — the real error, from the real code path,
|
||||
// rather than a stub that merely returns something transient.
|
||||
let config = EthernetConfig {
|
||||
interface: "fips-absent-x0".to_string(),
|
||||
ethertype: None,
|
||||
mtu: None,
|
||||
recv_buf_size: None,
|
||||
send_buf_size: None,
|
||||
listen: Some(true),
|
||||
announce: Some(false),
|
||||
auto_connect: None,
|
||||
accept_connections: Some(true),
|
||||
beacon_interval_secs: None,
|
||||
optional: Some(true),
|
||||
};
|
||||
let (tx, _rx) = crate::transport::packet_channel(8);
|
||||
let mut eth = EthernetTransport::new(transport_id, Some("lab".into()), config, tx);
|
||||
eth.start_async()
|
||||
.await
|
||||
.expect("an absent interface is not a start failure");
|
||||
node_b
|
||||
.transports
|
||||
.insert(transport_id, TransportHandle::Ethernet(eth));
|
||||
|
||||
let rejects_before = node_b.stats().handshake.snapshot().bad_state;
|
||||
|
||||
let peer_b_identity = PeerIdentity::from_pubkey_full(node_b.identity().pubkey_full());
|
||||
let mut conn_a = outbound_leg(LinkId::new(1), peer_b_identity, 1000);
|
||||
let noise_msg1 = conn_a
|
||||
.start_handshake(node_a.identity().keypair(), node_a.startup_epoch(), 1000)
|
||||
.unwrap();
|
||||
let wire_msg1 = build_msg1(SessionIndex::new(7), &noise_msg1);
|
||||
let packet = ReceivedPacket::with_timestamp(
|
||||
transport_id,
|
||||
TransportAddr::from_string("aa:bb:cc:dd:ee:ff"),
|
||||
wire_msg1,
|
||||
1000,
|
||||
);
|
||||
|
||||
node_b.handle_msg1(packet).await;
|
||||
|
||||
assert_eq!(
|
||||
node_b.link_count(),
|
||||
1,
|
||||
"the link must survive a transport that is merely between interfaces"
|
||||
);
|
||||
assert_eq!(
|
||||
node_b.stats().handshake.snapshot().bad_state,
|
||||
rejects_before,
|
||||
"a local interface flap must not be recorded as the peer's misbehaviour"
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user