From 2b81bd286f37f19f7f1e4fb83594d45f2c21f40b Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Wed, 19 Aug 2026 14:16:14 +0200 Subject: [PATCH] fix(clients): the Deck learns a host's wake MAC, so Wake-on-LAN can fire there at all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every wake gate in the codebase reads `!host.mac.is_empty()` against the saved record — `ConnectPlan::wake`, the console's `can_wake`, `punktfunk wake`. That MAC only ever reached the store through `trust::learn_mac`, and `learn_mac` had exactly two callers: the GTK hosts page and the WinUI one. Neither runs on a Steam Deck. Gaming Mode has only the Decky panel (which drives the headless CLI) and the console home — and those learned the management port alone, never the MAC. So a Deck's records stayed MAC-less forever, every wake gate stayed false, and Wake-on-LAN was skipped silently: no packet, no error, nothing to see. It worked on desktop purely because those two hosts pages learn on each discovery tick. (#322) Rather than add the missing call twice, collapse the three per-field learners (`learn_mac`, `learn_os`, `learn_mgmt_port` — three `pub fn`s, three load/save cycles) into one `learn_from_advert`, and call it at every site where an advert meets a saved record: both desktop hosts pages, the console home, and the CLI's `discover`. Remembering one call is not a thing a front-end can half-do; remembering three is what produced this. It takes the three fields rather than a `DiscoveredHost` because there are two of those — core's and the WinUI shell's verbatim port. `discover` is where the panel-only flow is fixed: it is the one verb the Decky panel runs that ever sees an advert. It keeps `KnownHosts::read()`, so it still mints no ids and cannot join the race that comment warns about, and `learn_from_advert` writes only when an advert genuinely taught the record something — a steady-state panel refresh touches no disk. Two things fall out of the same root cause: the console home now persists the OS chain too, so a Deck host's icon stops vanishing the moment mDNS goes quiet; and `punktfunk wake`'s "connect to it once while it's awake" is replaced, since a MAC comes from an advert and never from a connect — that wording sent this diagnosis looking in the wrong place. The magic-packet sender itself was never at fault (`punktfunk-core::wol` passes its 7 tests) and neither was the flatpak sandbox (`--share=network`). Nothing reached them. Closes #322 --- clients/cli/src/main.rs | 23 +++-- clients/linux/src/ui_hosts.rs | 41 +++------ clients/session/src/console.rs | 18 +++- clients/windows/src/app/hosts.rs | 38 ++++---- clients/windows/src/discovery.rs | 2 +- clients/windows/src/trust.rs | 4 +- crates/pf-client-core/src/trust.rs | 136 +++++++++++++++++------------ 7 files changed, 146 insertions(+), 116 deletions(-) diff --git a/clients/cli/src/main.rs b/clients/cli/src/main.rs index 53e1f6c1..184ccfb1 100644 --- a/clients/cli/src/main.rs +++ b/clients/cli/src/main.rs @@ -370,15 +370,26 @@ from the config directory for a true factory reset." .unwrap_or(DISCOVER_DEFAULT_SECS) .min(DISCOVER_MAX_SECS); let found = pf_client_core::discovery::discover_for(Duration::from_secs_f64(secs)); - // `read`, not `load`: this verb only LOOKS at the records to annotate what it found, and - // never hands their ids back. `load` would mint ids for a pre-mint store and save them — - // a write from a read-only verb, and one that races the `hosts list` a caller is very - // likely running at the same moment (the Decky panel issues both together). + // `read`, not `load`: this verb never hands a record's id back, so it has no business + // MINTING one. `load` would mint ids for a pre-mint store and save them, racing the + // `hosts list` a caller is very likely running at the same moment (the Decky panel issues + // both together) — after which the ids one of them already handed out no longer resolve. let known = KnownHosts::read(); let rows: Vec<( &pf_client_core::discovery::DiscoveredHost, Option<&KnownHost>, )> = found.iter().map(|d| (d, match_saved(&known, d))).collect(); + // The one write this verb does make, and why it doesn't contradict the above: an advert + // is the only place a host's wake MAC is ever published, and this verb is the only one + // the Decky panel runs that ever sees one. Without it a Deck in Gaming Mode never learns + // a MAC at all and Wake-on-LAN cannot fire, with nothing to show for it (#322). + // `learn_from_advert` mints nothing either, and writes only when an advert genuinely + // taught the record something new — so a steady-state panel refresh touches no disk. + for (d, saved) in &rows { + if let Some(k) = saved { + trust::learn_from_advert(&k.fp_hex, &k.addr, k.port, &d.mac, &d.os, d.mgmt_port); + } + } if has(args, "--json") { let hosts: Vec = rows .iter() @@ -733,7 +744,9 @@ from the config directory for a true factory reset." }; let host = &known.hosts[i]; if host.mac.is_empty() { - eprintln!("no Wake-on-LAN address known for {} — connect to it once while it's awake so the client can learn it", host.name); + // A MAC is learned from the host's mDNS advert, never from a connect — say so, since + // "connect to it once" sent at least one Deck owner looking in the wrong place (#322). + eprintln!("no Wake-on-LAN address known for {} — run `punktfunk discover` while it's awake (the Deck panel does this every time it opens) so the client learns it from the host's advert", host.name); return UNRESOLVED; } if !has(args, "--wait") { diff --git a/clients/linux/src/ui_hosts.rs b/clients/linux/src/ui_hosts.rs index 04531839..904a11ad 100644 --- a/clients/linux/src/ui_hosts.rs +++ b/clients/linux/src/ui_hosts.rs @@ -1087,33 +1087,20 @@ impl HostsPage { // Online = advertising on mDNS OR proven reachable by the last probe sweep. let online = self.adverts.values().any(|a| matches(k, a)) || self.probed.get(&saved_key(k)).copied().unwrap_or(false); - // Learn this host's wake MAC(s) from its live advert while it's online. - if let Some(a) = self - .adverts - .values() - .find(|a| matches(k, a) && !a.mac.is_empty()) - { - crate::trust::learn_mac(&k.fp_hex, &k.addr, k.port, &a.mac); - } - // Same for its OS chain — the icon then survives the host going offline. - if let Some(a) = self - .adverts - .values() - .find(|a| matches(k, a) && !a.os.is_empty()) - { - crate::trust::learn_os(&k.fp_hex, &k.addr, k.port, &a.os); - } - // Same for its management port — and this one is not cosmetic: without it a host - // that moved off 47990 loses its library the moment mDNS is unavailable, because - // the advert was the only place the real port ever lived. - if let Some(a) = self - .adverts - .values() - .find(|a| matches(k, a) && a.mgmt_port.is_some()) - { - if let Some(p) = a.mgmt_port { - crate::trust::learn_mgmt_port(&k.fp_hex, &k.addr, k.port, p); - } + // Learn what this host's live advert teaches while it's online: its wake MAC(s), + // its OS chain (so the icon survives it going offline), and its management port + // — the last one not cosmetic, since a host that moved off 47990 loses its + // library the moment mDNS is unavailable and the advert is the only place the + // real port ever lived. + if let Some(a) = self.adverts.values().find(|a| matches(k, a)) { + crate::trust::learn_from_advert( + &k.fp_hex, + &k.addr, + k.port, + &a.mac, + &a.os, + a.mgmt_port, + ); } saved.push_back(HostCard { connecting: self.connecting.as_deref() == Some(k.fp_hex.as_str()), diff --git a/clients/session/src/console.rs b/clients/session/src/console.rs index d3f173a5..5f3bd57b 100644 --- a/clients/session/src/console.rs +++ b/clients/session/src/console.rs @@ -764,11 +764,21 @@ impl ServiceState { || (d.addr == h.addr && d.port == h.port) }); let online = advert.is_some() || probed.get(&key).copied().unwrap_or(false); - // Write the advertised mgmt port down while the host is visible, so this console - // keeps working against a moved port once it is not. No-op (and no disk write) + // Write down everything the advert teaches while the host is visible: the mgmt + // port (so this console keeps working against a moved one once it is not), the + // OS chain, and the wake MAC — which matters most here, because this console and + // the Decky panel are the only surfaces a Deck in Gaming Mode ever runs, and a + // record that never learned a MAC can never be woken. No-op (and no disk write) // when unchanged, so this is safe on every refresh tick. - if let Some(p) = advert.and_then(|d| d.mgmt_port) { - pf_client_core::trust::learn_mgmt_port(&h.fp_hex, &h.addr, h.port, p); + if let Some(a) = advert { + pf_client_core::trust::learn_from_advert( + &h.fp_hex, + &h.addr, + h.port, + &a.mac, + &a.os, + a.mgmt_port, + ); } let row = HostRow { key: key.clone(), diff --git a/clients/windows/src/app/hosts.rs b/clients/windows/src/app/hosts.rs index 2ad01b74..92126a4f 100644 --- a/clients/windows/src/app/hosts.rs +++ b/clients/windows/src/app/hosts.rs @@ -700,31 +700,23 @@ pub(crate) fn hosts_page(props: &HostsProps, cx: &mut RenderCx) -> Element { .iter() .any(|h| h.fp_hex == k.fp_hex || (h.addr == k.addr && h.port == k.port)) || props.probed.get(&k.fp_hex).copied().unwrap_or(false); - // Learn this host's wake MAC(s) from its live advert while it's online, so we can wake - // it once it sleeps (no-op / no disk write when unchanged). - if let Some(a) = hosts.iter().find(|h| { - (h.fp_hex == k.fp_hex || (h.addr == k.addr && h.port == k.port)) - && !h.mac.is_empty() - }) { - crate::trust::learn_mac(&k.fp_hex, &k.addr, k.port, &a.mac); - } - // Same for its OS chain — the tile's mark then survives the host going offline. - if let Some(a) = hosts.iter().find(|h| { - (h.fp_hex == k.fp_hex || (h.addr == k.addr && h.port == k.port)) && !h.os.is_empty() - }) { - crate::trust::learn_os(&k.fp_hex, &k.addr, k.port, &a.os); - } - // Same for its management port — load-bearing, unlike the two above: a host moved off - // 47990 loses its library entirely once mDNS is gone unless we write the port down. - if let Some(p) = hosts + // Learn what this host's live advert teaches while it's online: its wake MAC(s) (so we + // can wake it once it sleeps), its OS chain (so the tile's mark survives it going + // offline), and its management port — the last load-bearing rather than cosmetic, as + // a host moved off 47990 loses its library entirely once mDNS is gone unless we write + // the port down. No-op, and no disk write, when unchanged. + if let Some(a) = hosts .iter() - .find(|h| { - (h.fp_hex == k.fp_hex || (h.addr == k.addr && h.port == k.port)) - && h.mgmt_port.is_some() - }) - .and_then(|h| h.mgmt_port) + .find(|h| h.fp_hex == k.fp_hex || (h.addr == k.addr && h.port == k.port)) { - crate::trust::learn_mgmt_port(&k.fp_hex, &k.addr, k.port, p); + crate::trust::learn_from_advert( + &k.fp_hex, + &k.addr, + k.port, + &a.mac, + &a.os, + a.mgmt_port, + ); } let can_wake = !online && !k.mac.is_empty(); let menu = { diff --git a/clients/windows/src/discovery.rs b/clients/windows/src/discovery.rs index 94dd0afe..d4655628 100644 --- a/clients/windows/src/discovery.rs +++ b/clients/windows/src/discovery.rs @@ -29,7 +29,7 @@ pub struct DiscoveredHost { /// persisted like `mac`. Empty if absent (older host). pub os: String, /// The management API's port from the mDNS `mgmt` TXT — where the game library is served. - /// Persisted like `mac` (`trust::learn_mgmt_port`), and load-bearing rather than cosmetic: + /// Persisted like `mac` (`trust::learn_from_advert`), and load-bearing rather than cosmetic: /// a host moved off 47990 loses its library once mDNS is gone unless we write this down. /// `None` if absent (older host) — resolve via `library::DEFAULT_MGMT_PORT`. pub mgmt_port: Option, diff --git a/clients/windows/src/trust.rs b/clients/windows/src/trust.rs index 96976620..557db83d 100644 --- a/clients/windows/src/trust.rs +++ b/clients/windows/src/trust.rs @@ -8,6 +8,6 @@ //! still load via a serde alias in core. pub use pf_client_core::trust::{ - hex, learn_mac, learn_mgmt_port, learn_os, load_or_create_identity, pair_error_message, - parse_hex32, KnownHost, KnownHosts, Settings, + hex, learn_from_advert, load_or_create_identity, pair_error_message, parse_hex32, KnownHost, + KnownHosts, Settings, }; diff --git a/crates/pf-client-core/src/trust.rs b/crates/pf-client-core/src/trust.rs index 435c2ee7..0fc48274 100644 --- a/crates/pf-client-core/src/trust.rs +++ b/crates/pf-client-core/src/trust.rs @@ -675,10 +675,10 @@ pub fn forget_placeholder(addr: &str, port: u16) { } } -/// The record [`learn_mac`]/[`learn_os`] should write what an advert taught them onto: -/// the fingerprint match if there is one, else whatever the address resolves to. Fingerprint -/// FIRST — a single pass that took "either" would hand a stale record at the same address the -/// data the live host advertised, purely because it came earlier in the file. +/// The record an advert's lesson should land on: the fingerprint match if there is one, else +/// whatever the address resolves to. Fingerprint FIRST — a single pass that took "either" would +/// hand a stale record at the same address the data the live host advertised, purely because it +/// came earlier in the file. fn learn_target<'a>( known: &'a mut KnownHosts, fp_hex: &str, @@ -692,61 +692,62 @@ fn learn_target<'a>( known.hosts.get_mut(i) } -/// Learn/refresh a saved host's Wake-on-LAN MAC(s) from its live advert (called while the host -/// is online, matched by fingerprint or address). No-op — and no disk write — when unchanged, so -/// the hosts page can call it on every discovery tick without churning the store. -pub fn learn_mac(fp_hex: &str, addr: &str, port: u16, mac: &[String]) { - if mac.is_empty() { - return; - } - let mut known = KnownHosts::load(); - let Some(h) = learn_target(&mut known, fp_hex, addr, port) else { - return; - }; - if h.mac == mac { - return; - } - h.mac = mac.to_vec(); - let _ = known.save(); -} - -/// Learn/refresh a saved host's OS-identity chain from its live advert (mDNS `os` TXT), matched -/// like [`learn_mac`]: by fingerprint or address. No-op — and no disk write — when unchanged, so -/// the hosts page can call it on every discovery tick without churning the store. -pub fn learn_os(fp_hex: &str, addr: &str, port: u16, os: &str) { - if os.is_empty() { - return; - } - let mut known = KnownHosts::load(); - let Some(h) = learn_target(&mut known, fp_hex, addr, port) else { - return; - }; - if h.os == os { - return; - } - h.os = os.to_string(); - let _ = known.save(); -} - -/// Learn/refresh a saved host's management-API port from its live advert (mDNS `mgmt` TXT), -/// matched like [`learn_mac`]: by fingerprint or address. No-op — and no disk write — when -/// unchanged, so the hosts page can call it on every discovery tick without churning the store. +/// Copy everything an advert can teach onto a saved record — wake MAC(s), OS-identity chain, +/// management port — and report whether anything actually moved, so the caller writes only when +/// there is something to write. Pure (no disk, no clock), which is what makes it testable. /// -/// This is what makes a moved mgmt port outlive mDNS. Until it existed the port was read straight -/// off the live advert and thrown away, so the library worked on the LAN and went blank over a VPN. -pub fn learn_mgmt_port(fp_hex: &str, addr: &str, port: u16, mgmt_port: u16) { - if mgmt_port == 0 { - return; +/// A field the advert does not carry is left alone, never cleared: an older host simply omits the +/// TXT, and forgetting a MAC already learned would cost the user their wake. +fn apply_advert(h: &mut KnownHost, mac: &[String], os: &str, mgmt_port: Option) -> bool { + let mut changed = false; + if !mac.is_empty() && h.mac != mac { + h.mac = mac.to_vec(); + changed = true; } - let mut known = KnownHosts::load(); + if !os.is_empty() && h.os != os { + h.os = os.to_string(); + changed = true; + } + // 0 is how "not advertised" reaches us from a caller whose own type has no `Option`. + if mgmt_port.is_some_and(|p| p != 0 && h.mgmt_port != Some(p)) { + h.mgmt_port = mgmt_port; + changed = true; + } + changed +} + +/// Write down everything a live advert teaches the saved record it matched — wake MAC(s), OS +/// chain, management port — matched by fingerprint or address. No-op, and no disk write, when +/// the record already says all three, so a surface can call this on every discovery tick. +/// +/// ONE call rather than three. Each field used to be learned by its own function, which meant +/// every front-end had to remember all three, and only the two desktop hosts pages ever did: +/// the console home and the headless CLI learned the management port alone. On a Steam Deck, +/// whose Gaming Mode runs nothing but those two, that left every saved host with no MAC forever +/// — and every wake gate in the codebase reads `!mac.is_empty()` against this record, so +/// Wake-on-LAN there could not fire at all, with no error to show for it (#322). +/// +/// [`KnownHosts::read`], not [`KnownHosts::load`]: `punktfunk discover` calls this, and that verb +/// is deliberately not an id-minter (see [`KnownHosts::read`] for the race that avoids). Learning +/// a MAC is no reason to become one. +/// +/// Takes the three learned fields rather than a `DiscoveredHost` because there are two of those +/// — core's and the WinUI shell's verbatim port — and this has to serve both. +pub fn learn_from_advert( + fp_hex: &str, + addr: &str, + port: u16, + mac: &[String], + os: &str, + mgmt_port: Option, +) { + let mut known = KnownHosts::read(); let Some(h) = learn_target(&mut known, fp_hex, addr, port) else { return; }; - if h.mgmt_port == Some(mgmt_port) { - return; + if apply_advert(h, mac, os, mgmt_port) { + let _ = known.save(); } - h.mgmt_port = Some(mgmt_port); - let _ = known.save(); } /// Re-key a saved host's address/port after it rediscovered on a new DHCP lease (matched by @@ -785,7 +786,7 @@ pub fn touch_last_used(fp_hex: &str) { /// Save a host's management-API port learned from the **session's own `Welcome`**, keyed by /// fingerprint alone — the identity a just-connected client is certain of. /// -/// This is the mDNS-free path, and the one that matters most: [`learn_mgmt_port`] can only fire +/// This is the mDNS-free path, and the one that matters most: [`learn_from_advert`] can only fire /// where an advert is visible, whereas this fires on any successful connect, including a host /// added by IP on a network where discovery has never worked. No-op — and no disk write — when /// the fingerprint isn't stored or the value is unchanged, so it is safe on every connect. @@ -2293,6 +2294,33 @@ mod tests { assert!(learn_target(&mut k, &fp('e'), "10.0.0.9", 9777).is_none()); } + /// What an advert carries lands on the record; what it omits is left alone; and a repeat of + /// the same advert reports no change — which is what lets every surface call this on every + /// discovery tick without churning the store. + #[test] + fn apply_advert_learns_what_it_carries_and_keeps_what_it_omits() { + let mut h = KnownHost::default(); + let mac = vec!["aa:bb:cc:dd:ee:ff".to_string()]; + assert!(apply_advert(&mut h, &mac, "linux/arch", Some(47991))); + assert_eq!(h.mac, mac); + assert_eq!(h.os, "linux/arch"); + assert_eq!(h.mgmt_port, Some(47991)); + // The same advert a tick later: nothing moved, so there is nothing to persist. + assert!(!apply_advert(&mut h, &mac, "linux/arch", Some(47991))); + // An older host advertises none of the three. Clearing a learned MAC here is exactly what + // would cost the user their wake, so an absent field must never overwrite a known one. + assert!(!apply_advert(&mut h, &[], "", None)); + assert_eq!(h.mac, mac); + assert_eq!(h.os, "linux/arch"); + assert_eq!(h.mgmt_port, Some(47991)); + // 0 is how "not advertised" reaches us from a consumer that has no Option — not a port. + assert!(!apply_advert(&mut h, &[], "", Some(0))); + assert_eq!(h.mgmt_port, Some(47991)); + // A host that genuinely moved: the new value wins. + assert!(apply_advert(&mut h, &[], "", Some(47992))); + assert_eq!(h.mgmt_port, Some(47992)); + } + /// Pins render in card order, deduplicated, with deleted profiles simply gone — a pin is /// presentation state, so a dangling one is never an error surface. #[test] -- 2.54.0