From 55a1f15fa8fce90e500a3ddd1e973f197d19942f Mon Sep 17 00:00:00 2001 From: Sandeepa Nadahalli <1698507+snadahalli@users.noreply.github.com> Date: Wed, 26 Aug 2026 14:07:59 +0530 Subject: [PATCH] fix(discovery): sort discovered addresses by connectability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mdns_sd::ServiceInfo::get_addresses` returns a `&HashSet`, and all three browsers collected it straight into a `Vec` with `.iter().copied().collect()`. Callers then dial `addresses.first()`, so the address actually connected to was an arbitrary hash-set element — not merely unsorted but non-deterministic, and re-rolled every time a record refreshed. Observed across a single run, same machine, same service: [192.168.0.97, fe80::1427:c2c8:a235:a115] [fe80::1, ::1, fe80::1427:c2c8:a235:a115, 127.0.0.1, 192.168.0.97] [127.0.0.1, ::1, fe80::1, 192.168.0.97, fe80::1427:c2c8:a235:a115] [::1, fe80::1, 127.0.0.1] So `first()` yielded 192.168.0.97, fe80::1, 127.0.0.1 or ::1 depending on timing, and the fourth record offered no routable address at all. Remote receivers only advertise two addresses so they usually looked fine, which is what kept this hidden; a machine advertising loopback and link-local alongside its LAN address exposes it immediately. Sort at collection instead, most-connectable first: routable IPv4, routable IPv6, loopback, link-local, unspecified — ties broken on the address value so the result is stable run to run. Link-local ranks below loopback because we carry no zone index and cannot reconstruct one, so `fe80::1` is not dialable at all, whereas loopback at least resolves (to the wrong host for a remote receiver, hence still below anything routable). IPv4 is preferred over routable IPv6 since it needs no scope handling. This makes `addresses.first()` correct by construction, so the three `addr()` arms in the sender need no change. Verified against the live network: every discovered receiver now reports a routable IPv4 first, on every refresh. --- crates/openplay-discovery/src/address.rs | 135 ++++++++++++++++++ .../openplay-discovery/src/airplay_browser.rs | 7 +- crates/openplay-discovery/src/browser.rs | 7 +- crates/openplay-discovery/src/lib.rs | 2 + .../src/miracast_browser.rs | 6 +- 5 files changed, 154 insertions(+), 3 deletions(-) create mode 100644 crates/openplay-discovery/src/address.rs diff --git a/crates/openplay-discovery/src/address.rs b/crates/openplay-discovery/src/address.rs new file mode 100644 index 0000000..743cd99 --- /dev/null +++ b/crates/openplay-discovery/src/address.rs @@ -0,0 +1,135 @@ +//! Ordering for the addresses mDNS reports for a discovered receiver. +//! +//! `mdns_sd::ServiceInfo::get_addresses` returns a `&HashSet`, so the +//! order a receiver's addresses arrive in is not merely unsorted — it is +//! non-deterministic, and changes between advertisements of the same service. +//! Since callers connect to the first address, that made the dialled address a +//! lottery, re-rolled every time a record refreshed. +//! +//! Sorting at collection makes the first address the most connectable one, so +//! `addresses.first()` is correct by construction rather than by luck. + +use std::net::IpAddr; + +/// Connectability rank, lowest first. +/// +/// A link-local address is ranked below loopback because we advertise no zone +/// index and cannot reconstruct one, so `fe80::1` is not dialable at all, while +/// loopback at least resolves — to the wrong host for a remote receiver, which +/// is why it still ranks below anything routable. +fn rank(ip: &IpAddr) -> u8 { + let (loopback, link_local, unspecified) = match ip { + IpAddr::V4(v4) => (v4.is_loopback(), v4.is_link_local(), v4.is_unspecified()), + // `Ipv6Addr::is_unicast_link_local` is still unstable, so test fe80::/10 + // directly. + IpAddr::V6(v6) => ( + v6.is_loopback(), + (v6.segments()[0] & 0xffc0) == 0xfe80, + v6.is_unspecified(), + ), + }; + + if unspecified { + return 4; + } + if link_local { + return 3; + } + if loopback { + return 2; + } + // Routable. Prefer IPv4: every receiver we target reachable over IPv6 is + // also reachable over IPv4, and IPv4 needs no scope handling. + match ip { + IpAddr::V4(_) => 0, + IpAddr::V6(_) => 1, + } +} + +/// Sorts discovered addresses most-connectable first. +/// +/// Ties break on the address value so the result is stable across runs, which +/// the `HashSet` iteration order on its own is not. +pub fn sort_by_connectability(addrs: &mut [IpAddr]) { + addrs.sort_by_key(|ip| (rank(ip), *ip)); +} + +#[cfg(test)] +mod tests { + use super::*; + + fn ips(raw: &[&str]) -> Vec { + raw.iter().map(|s| s.parse().unwrap()).collect() + } + + #[test] + fn routable_ipv4_wins() { + // The exact set this machine advertised for itself, in one of the + // orders observed in the wild. + let mut a = ips(&[ + "fe80::1", + "::1", + "fe80::1427:c2c8:a235:a115", + "127.0.0.1", + "192.168.0.97", + ]); + sort_by_connectability(&mut a); + assert_eq!(a[0], "192.168.0.97".parse::().unwrap()); + } + + #[test] + fn link_local_ranks_below_loopback() { + let mut a = ips(&["fe80::1", "127.0.0.1"]); + sort_by_connectability(&mut a); + assert_eq!(a, ips(&["127.0.0.1", "fe80::1"])); + } + + #[test] + fn routable_ipv6_beats_loopback_and_link_local() { + let mut a = ips(&["fe80::1", "::1", "2001:db8::1"]); + sort_by_connectability(&mut a); + assert_eq!(a[0], "2001:db8::1".parse::().unwrap()); + } + + #[test] + fn ipv4_preferred_over_routable_ipv6() { + let mut a = ips(&["2001:db8::1", "192.168.0.97"]); + sort_by_connectability(&mut a); + assert_eq!(a[0], "192.168.0.97".parse::().unwrap()); + } + + #[test] + fn ipv4_link_local_is_demoted_too() { + let mut a = ips(&["169.254.1.1", "192.168.0.97"]); + sort_by_connectability(&mut a); + assert_eq!(a, ips(&["192.168.0.97", "169.254.1.1"])); + } + + /// Every permutation of the observed set must produce the same first + /// address — that is the whole point. + #[test] + fn ordering_is_stable_whatever_the_hashset_yielded() { + let base = ips(&["fe80::1", "::1", "127.0.0.1", "192.168.0.97"]); + let mut sorted: Option> = None; + for rotate in 0..base.len() { + let mut a = base.clone(); + a.rotate_left(rotate); + sort_by_connectability(&mut a); + match &sorted { + None => sorted = Some(a), + Some(first) => assert_eq!(first, &a), + } + } + } + + #[test] + fn empty_and_single_are_fine() { + let mut none: Vec = vec![]; + sort_by_connectability(&mut none); + assert!(none.is_empty()); + + let mut one = ips(&["fe80::1"]); + sort_by_connectability(&mut one); + assert_eq!(one, ips(&["fe80::1"])); + } +} diff --git a/crates/openplay-discovery/src/airplay_browser.rs b/crates/openplay-discovery/src/airplay_browser.rs index 93816f1..e30a597 100644 --- a/crates/openplay-discovery/src/airplay_browser.rs +++ b/crates/openplay-discovery/src/airplay_browser.rs @@ -69,7 +69,12 @@ impl AirPlayBrowser { let receiver_info = AirPlayReceiverInfo { name: info.get_fullname().to_string(), display_name, - addresses: info.get_addresses().iter().copied().collect(), + addresses: { + let mut addrs: Vec<_> = + info.get_addresses().iter().copied().collect(); + crate::address::sort_by_connectability(&mut addrs); + addrs + }, port: info.get_port(), device_id: txt .as_ref() diff --git a/crates/openplay-discovery/src/browser.rs b/crates/openplay-discovery/src/browser.rs index 6f999b4..66ad27b 100644 --- a/crates/openplay-discovery/src/browser.rs +++ b/crates/openplay-discovery/src/browser.rs @@ -67,7 +67,12 @@ impl ReceiverBrowser { let receiver_info = ReceiverInfo { name: info.get_fullname().to_string(), display_name, - addresses: info.get_addresses().iter().copied().collect(), + addresses: { + let mut addrs: Vec<_> = + info.get_addresses().iter().copied().collect(); + crate::address::sort_by_connectability(&mut addrs); + addrs + }, port: info.get_port(), fingerprint: txt .as_ref() diff --git a/crates/openplay-discovery/src/lib.rs b/crates/openplay-discovery/src/lib.rs index 0126202..6ae74ba 100644 --- a/crates/openplay-discovery/src/lib.rs +++ b/crates/openplay-discovery/src/lib.rs @@ -11,6 +11,7 @@ //! On Linux, Miracast peers can additionally be found over Wi-Fi Direct; that //! lives in `openplay-miracast`, not here. +pub mod address; mod advertiser; pub mod airplay_browser; pub mod airplay_record; @@ -18,6 +19,7 @@ mod browser; pub mod miracast_browser; mod record; +pub use address::sort_by_connectability; pub use advertiser::ReceiverAdvertiser; pub use airplay_browser::{AirPlayBrowser, AirPlayReceiverInfo}; pub use airplay_record::AirPlayTxtRecord; diff --git a/crates/openplay-discovery/src/miracast_browser.rs b/crates/openplay-discovery/src/miracast_browser.rs index fc37ae0..f189e43 100644 --- a/crates/openplay-discovery/src/miracast_browser.rs +++ b/crates/openplay-discovery/src/miracast_browser.rs @@ -122,7 +122,11 @@ fn run_browser_thread( let receiver_info = MiracastReceiverInfo { name: info.get_fullname().to_string(), display_name: display_name.clone(), - addresses: info.get_addresses().iter().copied().collect(), + addresses: { + let mut addrs: Vec<_> = info.get_addresses().iter().copied().collect(); + crate::address::sort_by_connectability(&mut addrs); + addrs + }, port, device_info: device_info.clone(), };