diff --git a/src/bin/test_net.rs b/src/bin/test_net.rs index c4271d8..c80740c 100644 --- a/src/bin/test_net.rs +++ b/src/bin/test_net.rs @@ -63,7 +63,7 @@ async fn main() -> Result<(), Box> { addr: endpoint_a.addr(), sharing: None, }; - room_a.join(&ticket_str, state_a).await?; + room_a.join(&ticket_str, state_a, vec![]).await?; println!("Node A joined topic."); // Subscribe to events on Node A @@ -81,7 +81,7 @@ async fn main() -> Result<(), Box> { addr: endpoint_b.addr(), sharing: None, }; - room_b.join(&ticket_str, state_b).await?; + room_b.join(&ticket_str, state_b, vec![]).await?; println!("Node B joined topic."); // Subscribe to events on Node B diff --git a/src/core/mod.rs b/src/core/mod.rs index d27f26e..4237f3e 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -13,7 +13,7 @@ use crate::core::messages::{CoreCommand, UiEvent}; use crate::config::{NetworkMode, RecordingMode}; use crate::audio::multitrack::MultitrackRecorder; -use iroh::{Endpoint, EndpointId, RelayMode, endpoint::presets, protocol::Router}; +use iroh::{Endpoint, EndpointAddr, EndpointId, RelayMode, endpoint::presets, protocol::Router}; use iroh_gossip::net::Gossip; use tokio::sync::{mpsc, Mutex}; use std::collections::{HashMap, HashSet}; @@ -415,6 +415,15 @@ async fn run_core_loop( ) -> Result<(), anyhow::Error> { let memory_lookup = iroh::address_lookup::memory::MemoryLookup::new(); let secret_key = iroh::SecretKey::generate(); + // Peers seen in the current/most-recent room, retained ACROSS leave so a + // rejoin can bootstrap to them. This is the fix for A8: the room creator's own + // ticket lists only themselves as host, so on rejoin these retained peers are + // their only dial targets. Reset when the joined ticket changes (different / + // brand-new room — see the Join handler). Keyed by id so updates refresh addr. + let known_peers: Arc>> = + Arc::new(std::sync::Mutex::new(HashMap::new())); + // The ticket of the room we last (re)joined, to detect room changes above. + let mut last_join_ticket: Option = None; let audio_backend = Arc::new(PipeWireBackend::new()); @@ -554,8 +563,21 @@ async fn run_core_loop( sharing: None, }; - crate::log_msg(&format!("Attempting room_state.join with self_state={:?}", self_state)); - if let Err(e) = room_state.join(&ticket_str, self_state.clone()).await { + // Retain peers across leave so a rejoin can dial them (A8). A + // different ticket (or a fresh "create", which mints a random + // ticket) shares no peers with the last room, so reset; rejoining + // the SAME ticket keeps the set — that's what gives the room + // creator a dial target on rejoin. Then snapshot it as the extra + // bootstrap set (resolution rides the persistent address book). + if last_join_ticket.as_deref() != Some(ticket_str.as_str()) { + known_peers.lock().unwrap().clear(); + } + last_join_ticket = Some(ticket_str.clone()); + let extra_bootstrap: Vec = + known_peers.lock().unwrap().values().cloned().collect(); + + crate::log_msg(&format!("Attempting room_state.join with self_state={:?}, extra_bootstrap={:?}", self_state, extra_bootstrap.iter().map(|a| a.id).collect::>())); + if let Err(e) = room_state.join(&ticket_str, self_state.clone(), extra_bootstrap).await { crate::log_msg(&format!("Error room_state.join failed: {:?}", e)); let _ = ui_tx.send(UiEvent::Error(format!("Failed to join room: {}", e))).await; let _ = router.shutdown().await; @@ -900,6 +922,7 @@ async fn run_core_loop( let seen_connected_events = seen_connected.clone(); let multitrack_events = multitrack.clone(); let is_multitrack_events = is_multitrack.clone(); + let known_peers_events = known_peers.clone(); let event_task = tokio::spawn(async move { while let Some(event) = room_events.recv().await { match event { @@ -912,6 +935,8 @@ async fn run_core_loop( // Hand over the full address so reconnects can dial // it directly rather than via the gossip lookup. transport_events.connect_peer(state.addr.clone()).await; + // Retain this peer as a future rejoin bootstrap target (A8). + known_peers_events.lock().unwrap().insert(peer_id, state.addr.clone()); // If a multitrack recording is live, give this peer // its own stem track (silence-padded back to t=0). if is_multitrack_events.load(Ordering::Relaxed) @@ -926,6 +951,10 @@ async fn run_core_loop( // Graceful leave — evict immediately. cancel_grace_timer(&grace_timers_events, &peer_id); seen_connected_events.lock().unwrap().remove(&peer_id); + // Graceful leave: drop them as a rejoin dial target + // (a transient PeerConnectionLost deliberately does + // NOT, so we can still re-dial a peer who's still up). + known_peers_events.lock().unwrap().remove(&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; @@ -939,6 +968,8 @@ async fn run_core_loop( // re-records the same address. cancel_grace_timer(&grace_timers_events, &peer_id); transport_events.connect_peer(state.addr.clone()).await; + // Refresh the retained rejoin target with the fresh addr (A8). + known_peers_events.lock().unwrap().insert(peer_id, state.addr.clone()); let _ = ui_tx_events.send(UiEvent::PeerUpdated { id: peer_id, state }).await; } RoomEvent::ChatMessage { name, text, .. } => { diff --git a/src/network/gossip.rs b/src/network/gossip.rs index 9ab154b..b06b1a1 100644 --- a/src/network/gossip.rs +++ b/src/network/gossip.rs @@ -1,5 +1,5 @@ use crate::network::{RoomState, NetError, PeerState, RoomEvent, PeerSpeakTicket}; -use iroh::{Endpoint, EndpointId, SecretKey, Signature}; +use iroh::{Endpoint, EndpointAddr, EndpointId, SecretKey, Signature}; use iroh_gossip::net::Gossip; use iroh_gossip::proto::TopicId; use tokio::sync::mpsc; @@ -102,6 +102,28 @@ fn verify_gossip( Ok(()) } +/// Compute the gossip bootstrap peer set for a (re)join: the ticket's host plus +/// every peer we already knew about (`extra`), with self removed and ids +/// de-duplicated. Without the retained `extra` peers, the ROOM CREATOR rejoining +/// their own room would have an empty list (their ticket names only themselves as +/// host) and so never re-enter the swarm — that was bug A8. Pure → unit-testable. +fn compute_bootstrap( + self_id: EndpointId, + ticket_host: EndpointId, + extra: &[EndpointAddr], +) -> Vec { + let mut out: Vec = Vec::new(); + if ticket_host != self_id { + out.push(ticket_host); + } + for addr in extra { + if addr.id != self_id && !out.contains(&addr.id) { + out.push(addr.id); + } + } + out +} + #[derive(Serialize, Deserialize, Clone, Debug)] pub enum GossipMessage { Announce(PeerState), @@ -157,7 +179,12 @@ impl IrohGossipState { #[async_trait] impl RoomState for IrohGossipState { - async fn join(&self, ticket_str: &str, self_state: PeerState) -> Result<(), NetError> { + async fn join( + &self, + ticket_str: &str, + self_state: PeerState, + extra_bootstrap: Vec, + ) -> Result<(), NetError> { crate::log_msg(&format!("RoomState::join: self_id={:?}, self_name={:?}, ticket={}", self_state.addr.id, self_state.name, ticket_str)); let ticket = ticket_str.parse::()?; let topic_id = TopicId::from_bytes(ticket.topic_id); @@ -167,18 +194,20 @@ impl RoomState for IrohGossipState { // Stop any currently running topic let _ = self.leave().await; - // Add the host to the address book + // Make every dial target resolvable: the ticket host plus any retained + // peers handed in (their addresses may have aged out of a fresh endpoint's + // book even though the persistent lookup usually still holds them). self.address_lookup.add_endpoint_info(ticket.host_addr.clone()); + for addr in &extra_bootstrap { + self.address_lookup.add_endpoint_info(addr.clone()); + } - // Join the gossip topic. If we are the host, bootstrap list will be empty - // or contain ourselves (which is fine), but let's bootstrap to the ticket host. - let bootstrap_peers = if ticket.host_addr.id == self_state.addr.id { - crate::log_msg("We are the host. Bootstrap peers list is empty."); - vec![] - } else { - crate::log_msg(&format!("We are a client. Bootstrapping to host ID={:?}", ticket.host_addr.id)); - vec![ticket.host_addr.id] - }; + // Bootstrap to the ticket host AND every retained peer (minus self). The + // retained peers are what let the room creator rejoin a room they left — + // their own ticket names only themselves as host (bug A8). + let bootstrap_peers = + compute_bootstrap(self_state.addr.id, ticket.host_addr.id, &extra_bootstrap); + crate::log_msg(&format!("Bootstrap peers for join: {:?}", bootstrap_peers)); let gossip_topic = self.gossip.subscribe(topic_id, bootstrap_peers).await .map_err(|e| { @@ -467,6 +496,54 @@ mod tests { } } + fn fresh_id() -> EndpointId { + SecretKey::generate().public() + } + + fn addr_of(id: EndpointId) -> EndpointAddr { + EndpointAddr::from(id) + } + + #[test] + fn bootstrap_client_dials_host() { + // A non-host (client) with no retained peers dials just the ticket host. + let me = fresh_id(); + let host = fresh_id(); + assert_eq!(compute_bootstrap(me, host, &[]), vec![host]); + } + + #[test] + fn bootstrap_host_rejoin_dials_retained_peers() { + // The A8 regression: the room creator's ticket names themselves as host, + // so the ticket-host contributes nothing — the retained peer is the only + // (and essential) dial target. An empty result here would reproduce A8. + let me = fresh_id(); + let peer = fresh_id(); + let bootstrap = compute_bootstrap(me, /* ticket_host = */ me, &[addr_of(peer)]); + assert_eq!(bootstrap, vec![peer]); + } + + #[test] + fn bootstrap_host_rejoin_with_no_retained_peers_is_empty() { + // Host rejoin before anyone was ever seen: nothing to dial (unavoidable), + // but it must not contain self. + let me = fresh_id(); + assert!(compute_bootstrap(me, me, &[]).is_empty()); + } + + #[test] + fn bootstrap_excludes_self_and_dedups_host() { + // Self is never dialed (even if it shows up among retained peers), and a + // peer that equals the ticket host isn't listed twice. + let me = fresh_id(); + let host = fresh_id(); + let other = fresh_id(); + let extra = [addr_of(me), addr_of(host), addr_of(other)]; + let bootstrap = compute_bootstrap(me, host, &extra); + assert_eq!(bootstrap, vec![host, other]); + assert!(!bootstrap.contains(&me)); + } + #[test] fn test_gossip_message_leave_round_trip() { let original = GossipMessage::Leave; diff --git a/src/network/mod.rs b/src/network/mod.rs index bbf4515..5026d9f 100644 --- a/src/network/mod.rs +++ b/src/network/mod.rs @@ -1,4 +1,4 @@ -use iroh::EndpointId; +use iroh::{EndpointId, EndpointAddr}; use bytes::Bytes; use thiserror::Error; use tokio::sync::mpsc::Receiver; @@ -133,7 +133,18 @@ pub trait NetworkTransport: Send + Sync { #[async_trait] pub trait RoomState: Send + Sync { /// Joins a room using a gossip ticket string and announces our state. - async fn join(&self, ticket: &str, self_state: PeerState) -> Result<(), NetError>; + /// + /// `extra_bootstrap` are additional peer addresses to dial when entering the + /// gossip swarm, on top of the ticket's host. This is what lets the ROOM + /// CREATOR rejoin a room they left: their own ticket lists only themselves as + /// host, so without retained peers they'd have no dial target and never + /// re-enter the swarm (bug A8). Callers with no prior peers pass `vec![]`. + async fn join( + &self, + ticket: &str, + self_state: PeerState, + extra_bootstrap: Vec, + ) -> Result<(), NetError>; /// Updates our local state (e.g. when user mutes/unmutes) and broadcasts it. async fn update_self_state(&self, self_state: PeerState) -> Result<(), NetError>;