From be818467ddf46430609e153a9c2c50f56e484664 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sun, 31 May 2026 23:29:26 -0400 Subject: [PATCH] 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 --- src/core/mod.rs | 82 +++++++++++++++++++++++++++++++++++++++---- src/network/gossip.rs | 11 ++++-- src/network/mod.rs | 8 +++++ 3 files changed, 93 insertions(+), 8 deletions(-) diff --git a/src/core/mod.rs b/src/core/mod.rs index 91af23a..03f6252 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -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>>>; + +/// 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 { endpoint: Endpoint, router: Router, @@ -55,6 +74,7 @@ struct ActiveSession { mixer_task: tokio::task::JoinHandle<()>, event_task: tokio::task::JoinHandle<()>, conn_event_task: tokio::task::JoinHandle<()>, + grace_timers: GraceTimers, } impl ActiveSession { @@ -64,6 +84,11 @@ impl ActiveSession { self.mixer_task.abort(); self.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"); let audio_backend_clone = audio_backend.clone(); @@ -426,10 +451,15 @@ async fn run_core_loop( let ui_tx_events = ui_tx.clone(); let jitter_events = jitter.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 { while let Some(event) = room_events.recv().await { match event { 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 // is known (the transport dedupes the full-mesh race). // 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; } RoomEvent::PeerLeft(peer_id) => { + // Graceful leave — evict immediately. + cancel_grace_timer(&grace_timers_events, &peer_id); transport_events.disconnect_peer(peer_id).await; jitter_events.lock().await.remove(&peer_id); let _ = ui_tx_events.send(UiEvent::PeerLeft { id: peer_id }).await; } RoomEvent::PeerUpdated(peer_id, state) => { - // A re-announce may carry a fresh address (peer back - // on a new network). Refresh the retained dial target - // so a later reconnect re-reaches it. Idempotent: an - // ordinary mute/unmute update just re-records the same - // address. + // A re-announce means the peer is alive — cancel any + // pending grace timer. It may also carry a fresh + // address (peer back on a new network); refresh the + // retained dial target so a later reconnect re-reaches + // 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; 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 grace_timers_conn = grace_timers.clone(); let conn_event_task = tokio::spawn(async move { while let Some(event) = conn_events.recv().await { let msg = match event { 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() { break; @@ -486,6 +555,7 @@ async fn run_core_loop( mixer_task, event_task, conn_event_task, + grace_timers, }; let self_id = endpoint.id().to_string(); diff --git a/src/network/gossip.rs b/src/network/gossip.rs index c7b341b..fb2498d 100644 --- a/src/network/gossip.rs +++ b/src/network/gossip.rs @@ -193,10 +193,17 @@ impl RoomState for IrohGossipState { } Ok(iroh_gossip::api::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(); if removed { - crate::log_msg(&format!("Removed peer due to NeighborDown: {:?}", peer_id)); - let _ = event_tx.send(RoomEvent::PeerLeft(peer_id)).await; + crate::log_msg(&format!("Peer connection lost (NeighborDown): {:?}", peer_id)); + let _ = event_tx.send(RoomEvent::PeerConnectionLost(peer_id)).await; } } Ok(other) => { diff --git a/src/network/mod.rs b/src/network/mod.rs index 4903a69..fd7afe9 100644 --- a/src/network/mod.rs +++ b/src/network/mod.rs @@ -32,8 +32,16 @@ pub struct PeerState { #[derive(Debug, Clone)] pub enum RoomEvent { PeerJoined(EndpointId, PeerState), + /// A peer left *gracefully* (it broadcast a `Leave`). Evict it immediately. PeerLeft(EndpointId), 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