fix: keep reconnecting across a long outage instead of evicting on NeighborDown
Field test of the long-outage path failed: ~31s into a Wi-Fi drop the peer
vanished with no "Reconnecting..." indicator and never came back. Logs showed
the chain `Gossip NeighborDown -> Removed peer -> Transport: stopped
supervising peer`.
Root cause: a transient gossip NeighborDown was routed to RoomEvent::PeerLeft,
same as a graceful leave, so core called disconnect_peer -> supervisor.abort().
That aborted the very reconnect supervisor that was meant to redial -- before
its own reconnect loop (which re-emits Connecting and re-dials the retained
09acefd address with backoff) ever ran. The supervisor + retained-address fix
were effectively dead code in the field, which is also why the loopback tests
(they drive the supervisor directly) never caught it.
Fix: decouple a transient drop from a graceful leave.
- gossip.rs: NeighborDown now emits the new RoomEvent::PeerConnectionLost
instead of PeerLeft. A graceful GossipMessage::Leave still emits PeerLeft.
- core: on PeerConnectionLost, do NOT disconnect the peer. Keep its supervisor
alive (it redials the retained address and drives the yellow indicator) and
arm a per-peer reconnect grace timer (RECONNECT_GRACE = 45s, comfortably past
the ~30s QUIC idle timeout). The peer is evicted only if the link hasn't
recovered when the timer fires. A gossip rejoin (PeerJoined/PeerUpdated) or a
transport reconnect (ConnEvent::Connected) cancels the timer first; session
shutdown aborts all pending timers so none fire a stray eviction.
Builds clean, clippy clean, 3 transport tests pass. NOT yet field-verified --
re-test the long-outage path on a real call.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
+76
-6
@@ -46,6 +46,25 @@ impl CoreController {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// How long a peer may stay "reconnecting" after a transient drop before we give
|
||||||
|
/// up and evict it. Comfortably past the QUIC idle timeout (~30s) so a genuine
|
||||||
|
/// reconnect has time to complete, but short enough that a crashed/departed peer
|
||||||
|
/// clears from the room promptly.
|
||||||
|
const RECONNECT_GRACE: Duration = Duration::from_secs(45);
|
||||||
|
|
||||||
|
/// Per-peer reconnect grace timers (see [`RECONNECT_GRACE`]). Shared between the
|
||||||
|
/// room-event task (which arms one on a transient drop and cancels it on a
|
||||||
|
/// gossip rejoin) and the conn-event task (which cancels it when the audio link
|
||||||
|
/// actually comes back).
|
||||||
|
type GraceTimers = Arc<std::sync::Mutex<HashMap<EndpointId, tokio::task::JoinHandle<()>>>>;
|
||||||
|
|
||||||
|
/// Cancel and forget a peer's pending grace timer, if any. No-op if none is armed.
|
||||||
|
fn cancel_grace_timer(timers: &GraceTimers, peer_id: &EndpointId) {
|
||||||
|
if let Some(handle) = timers.lock().unwrap().remove(peer_id) {
|
||||||
|
handle.abort();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
struct ActiveSession {
|
struct ActiveSession {
|
||||||
endpoint: Endpoint,
|
endpoint: Endpoint,
|
||||||
router: Router,
|
router: Router,
|
||||||
@@ -55,6 +74,7 @@ struct ActiveSession {
|
|||||||
mixer_task: tokio::task::JoinHandle<()>,
|
mixer_task: tokio::task::JoinHandle<()>,
|
||||||
event_task: tokio::task::JoinHandle<()>,
|
event_task: tokio::task::JoinHandle<()>,
|
||||||
conn_event_task: tokio::task::JoinHandle<()>,
|
conn_event_task: tokio::task::JoinHandle<()>,
|
||||||
|
grace_timers: GraceTimers,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl ActiveSession {
|
impl ActiveSession {
|
||||||
@@ -64,6 +84,11 @@ impl ActiveSession {
|
|||||||
self.mixer_task.abort();
|
self.mixer_task.abort();
|
||||||
self.event_task.abort();
|
self.event_task.abort();
|
||||||
self.conn_event_task.abort();
|
self.conn_event_task.abort();
|
||||||
|
// Abort any pending reconnect grace timers so they can't fire a stray
|
||||||
|
// eviction (or touch a torn-down transport) after the session is gone.
|
||||||
|
for (_, handle) in self.grace_timers.lock().unwrap().drain() {
|
||||||
|
handle.abort();
|
||||||
|
}
|
||||||
crate::log_msg("Aborted tasks");
|
crate::log_msg("Aborted tasks");
|
||||||
|
|
||||||
let audio_backend_clone = audio_backend.clone();
|
let audio_backend_clone = audio_backend.clone();
|
||||||
@@ -426,10 +451,15 @@ async fn run_core_loop(
|
|||||||
let ui_tx_events = ui_tx.clone();
|
let ui_tx_events = ui_tx.clone();
|
||||||
let jitter_events = jitter.clone();
|
let jitter_events = jitter.clone();
|
||||||
let transport_events = transport.clone();
|
let transport_events = transport.clone();
|
||||||
|
let grace_timers: GraceTimers = Arc::new(std::sync::Mutex::new(HashMap::new()));
|
||||||
|
let grace_timers_events = grace_timers.clone();
|
||||||
let event_task = tokio::spawn(async move {
|
let event_task = tokio::spawn(async move {
|
||||||
while let Some(event) = room_events.recv().await {
|
while let Some(event) = room_events.recv().await {
|
||||||
match event {
|
match event {
|
||||||
RoomEvent::PeerJoined(peer_id, state) => {
|
RoomEvent::PeerJoined(peer_id, state) => {
|
||||||
|
// A (re)join means the peer is back — cancel any
|
||||||
|
// pending reconnect grace timer before re-adding it.
|
||||||
|
cancel_grace_timer(&grace_timers_events, &peer_id);
|
||||||
// Establish the audio connection as soon as the peer
|
// Establish the audio connection as soon as the peer
|
||||||
// is known (the transport dedupes the full-mesh race).
|
// is known (the transport dedupes the full-mesh race).
|
||||||
// Hand over the full address so reconnects can dial
|
// Hand over the full address so reconnects can dial
|
||||||
@@ -438,19 +468,52 @@ async fn run_core_loop(
|
|||||||
let _ = ui_tx_events.send(UiEvent::PeerJoined { id: peer_id, state }).await;
|
let _ = ui_tx_events.send(UiEvent::PeerJoined { id: peer_id, state }).await;
|
||||||
}
|
}
|
||||||
RoomEvent::PeerLeft(peer_id) => {
|
RoomEvent::PeerLeft(peer_id) => {
|
||||||
|
// Graceful leave — evict immediately.
|
||||||
|
cancel_grace_timer(&grace_timers_events, &peer_id);
|
||||||
transport_events.disconnect_peer(peer_id).await;
|
transport_events.disconnect_peer(peer_id).await;
|
||||||
jitter_events.lock().await.remove(&peer_id);
|
jitter_events.lock().await.remove(&peer_id);
|
||||||
let _ = ui_tx_events.send(UiEvent::PeerLeft { id: peer_id }).await;
|
let _ = ui_tx_events.send(UiEvent::PeerLeft { id: peer_id }).await;
|
||||||
}
|
}
|
||||||
RoomEvent::PeerUpdated(peer_id, state) => {
|
RoomEvent::PeerUpdated(peer_id, state) => {
|
||||||
// A re-announce may carry a fresh address (peer back
|
// A re-announce means the peer is alive — cancel any
|
||||||
// on a new network). Refresh the retained dial target
|
// pending grace timer. It may also carry a fresh
|
||||||
// so a later reconnect re-reaches it. Idempotent: an
|
// address (peer back on a new network); refresh the
|
||||||
// ordinary mute/unmute update just re-records the same
|
// retained dial target so a later reconnect re-reaches
|
||||||
// address.
|
// it. Idempotent: an ordinary mute/unmute update just
|
||||||
|
// re-records the same address.
|
||||||
|
cancel_grace_timer(&grace_timers_events, &peer_id);
|
||||||
transport_events.connect_peer(state.addr.clone()).await;
|
transport_events.connect_peer(state.addr.clone()).await;
|
||||||
let _ = ui_tx_events.send(UiEvent::PeerUpdated { id: peer_id, state }).await;
|
let _ = ui_tx_events.send(UiEvent::PeerUpdated { id: peer_id, state }).await;
|
||||||
}
|
}
|
||||||
|
RoomEvent::PeerConnectionLost(peer_id) => {
|
||||||
|
// Transient drop: do NOT tear down the peer. Its audio
|
||||||
|
// supervisor stays alive and keeps redialing the
|
||||||
|
// retained address, so show "reconnecting" and arm a
|
||||||
|
// grace timer that evicts the peer only if the link
|
||||||
|
// hasn't recovered within RECONNECT_GRACE. A gossip
|
||||||
|
// rejoin (PeerJoined/PeerUpdated) or a transport
|
||||||
|
// reconnect (ConnEvent::Connected) cancels it first.
|
||||||
|
let _ = ui_tx_events.send(UiEvent::PeerConnecting { id: peer_id }).await;
|
||||||
|
let transport_evict = transport_events.clone();
|
||||||
|
let jitter_evict = jitter_events.clone();
|
||||||
|
let ui_evict = ui_tx_events.clone();
|
||||||
|
let timers_evict = grace_timers_events.clone();
|
||||||
|
let handle = tokio::spawn(async move {
|
||||||
|
tokio::time::sleep(RECONNECT_GRACE).await;
|
||||||
|
crate::log_msg(&format!(
|
||||||
|
"Reconnect grace expired; evicting peer {:?}", peer_id
|
||||||
|
));
|
||||||
|
transport_evict.disconnect_peer(peer_id).await;
|
||||||
|
jitter_evict.lock().await.remove(&peer_id);
|
||||||
|
let _ = ui_evict.send(UiEvent::PeerLeft { id: peer_id }).await;
|
||||||
|
timers_evict.lock().unwrap().remove(&peer_id);
|
||||||
|
});
|
||||||
|
// Replace (and abort) any timer already pending for
|
||||||
|
// this peer so repeated drops don't stack up.
|
||||||
|
if let Some(old) = grace_timers_events.lock().unwrap().insert(peer_id, handle) {
|
||||||
|
old.abort();
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
@@ -465,11 +528,17 @@ async fn run_core_loop(
|
|||||||
}
|
}
|
||||||
};
|
};
|
||||||
let ui_tx_conn = ui_tx.clone();
|
let ui_tx_conn = ui_tx.clone();
|
||||||
|
let grace_timers_conn = grace_timers.clone();
|
||||||
let conn_event_task = tokio::spawn(async move {
|
let conn_event_task = tokio::spawn(async move {
|
||||||
while let Some(event) = conn_events.recv().await {
|
while let Some(event) = conn_events.recv().await {
|
||||||
let msg = match event {
|
let msg = match event {
|
||||||
ConnEvent::Connecting(id) => UiEvent::PeerConnecting { id },
|
ConnEvent::Connecting(id) => UiEvent::PeerConnecting { id },
|
||||||
ConnEvent::Connected(id) => UiEvent::PeerConnected { id },
|
ConnEvent::Connected(id) => {
|
||||||
|
// The audio link came back — the peer recovered
|
||||||
|
// within the grace window, so cancel its eviction.
|
||||||
|
cancel_grace_timer(&grace_timers_conn, &id);
|
||||||
|
UiEvent::PeerConnected { id }
|
||||||
|
}
|
||||||
};
|
};
|
||||||
if ui_tx_conn.send(msg).await.is_err() {
|
if ui_tx_conn.send(msg).await.is_err() {
|
||||||
break;
|
break;
|
||||||
@@ -486,6 +555,7 @@ async fn run_core_loop(
|
|||||||
mixer_task,
|
mixer_task,
|
||||||
event_task,
|
event_task,
|
||||||
conn_event_task,
|
conn_event_task,
|
||||||
|
grace_timers,
|
||||||
};
|
};
|
||||||
|
|
||||||
let self_id = endpoint.id().to_string();
|
let self_id = endpoint.id().to_string();
|
||||||
|
|||||||
@@ -193,10 +193,17 @@ impl RoomState for IrohGossipState {
|
|||||||
}
|
}
|
||||||
Ok(iroh_gossip::api::Event::NeighborDown(peer_id)) => {
|
Ok(iroh_gossip::api::Event::NeighborDown(peer_id)) => {
|
||||||
crate::log_msg(&format!("Gossip event: NeighborDown={:?}", peer_id));
|
crate::log_msg(&format!("Gossip event: NeighborDown={:?}", peer_id));
|
||||||
|
// A NeighborDown is a *transient* loss, not a graceful
|
||||||
|
// leave: emit PeerConnectionLost so the core marks the peer
|
||||||
|
// "reconnecting" and keeps its audio supervisor redialing,
|
||||||
|
// rather than tearing everything down. (Treating this as a
|
||||||
|
// PeerLeft is exactly what defeated reconnect in the field —
|
||||||
|
// it aborted the supervisor ~30s in.) We still drop our
|
||||||
|
// cached presence entry; a rejoin re-announces as new.
|
||||||
let removed = peers.lock().unwrap().remove(&peer_id).is_some();
|
let removed = peers.lock().unwrap().remove(&peer_id).is_some();
|
||||||
if removed {
|
if removed {
|
||||||
crate::log_msg(&format!("Removed peer due to NeighborDown: {:?}", peer_id));
|
crate::log_msg(&format!("Peer connection lost (NeighborDown): {:?}", peer_id));
|
||||||
let _ = event_tx.send(RoomEvent::PeerLeft(peer_id)).await;
|
let _ = event_tx.send(RoomEvent::PeerConnectionLost(peer_id)).await;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
Ok(other) => {
|
Ok(other) => {
|
||||||
|
|||||||
@@ -32,8 +32,16 @@ pub struct PeerState {
|
|||||||
#[derive(Debug, Clone)]
|
#[derive(Debug, Clone)]
|
||||||
pub enum RoomEvent {
|
pub enum RoomEvent {
|
||||||
PeerJoined(EndpointId, PeerState),
|
PeerJoined(EndpointId, PeerState),
|
||||||
|
/// A peer left *gracefully* (it broadcast a `Leave`). Evict it immediately.
|
||||||
PeerLeft(EndpointId),
|
PeerLeft(EndpointId),
|
||||||
PeerUpdated(EndpointId, PeerState),
|
PeerUpdated(EndpointId, PeerState),
|
||||||
|
/// A peer's gossip neighbour link dropped without a graceful `Leave` (network
|
||||||
|
/// blip, crash, walked out of range). This is transient: the audio supervisor
|
||||||
|
/// keeps redialing, so the core marks the peer "reconnecting" and only evicts
|
||||||
|
/// it if the link hasn't recovered within the reconnect grace window. Distinct
|
||||||
|
/// from `PeerLeft` precisely so a momentary `NeighborDown` can't tear down the
|
||||||
|
/// reconnect path the way it used to.
|
||||||
|
PeerConnectionLost(EndpointId),
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Transport-level link state for a peer, surfaced so the UI can show when a
|
/// Transport-level link state for a peer, surfaced so the UI can show when a
|
||||||
|
|||||||
Reference in New Issue
Block a user