diff --git a/clients/cli/src/main.rs b/clients/cli/src/main.rs index 98d4a62c..b0920fea 100644 --- a/clients/cli/src/main.rs +++ b/clients/cli/src/main.rs @@ -121,6 +121,13 @@ punktfunk hosts — the saved-hosts store (shared with the desktop client) another subnet). Without --fp it is a placeholder to pair later; with a 64-hex fingerprint it is pinned immediately (still unpaired). + Idempotent, and keyed on the FINGERPRINT once there is one: re-running it + for a host already saved is a no-op, and giving a known fingerprint a new + address MOVES that host's record there rather than filing a second one + (which is how a host that changed DHCP lease stays reachable by its id). + A different fingerprint for an address already saved is refused, exit 3 — + a changed identity is a decision for a person. + punktfunk hosts forget Remove a saved host, its pinned fingerprint included. A later connect must pair or trust it again." @@ -592,6 +599,31 @@ from the config directory for a true factory reset." }, }; } + // No record at this address — but a record carrying this exact FINGERPRINT is + // this same host at a new one. Re-point it rather than filing a second record: + // the fingerprint is the identity, and a host that changed DHCP lease is the + // whole reason `hosts add --fp` is idempotent in the first place. Without this a + // moved host accumulates one record per address it has ever held, and the one a + // stable id resolves to keeps the address it can no longer be reached at. + if let Some(i) = known + .hosts + .iter() + .position(|h| !fp.is_empty() && h.fp_hex.eq_ignore_ascii_case(&fp)) + { + let was = format!("{}:{}", known.hosts[i].addr, known.hosts[i].port); + known.hosts[i].addr = addr.clone(); + known.hosts[i].port = port; + return match known.save() { + Ok(()) => { + println!("moved {was} to {addr}:{port}"); + OK + } + Err(e) => { + eprintln!("saving: {e:#}"); + CONNECT_FAILED + } + }; + } known.hosts.push(KnownHost { name: name.unwrap_or_else(|| addr.clone()), addr: addr.clone(), @@ -1317,6 +1349,32 @@ from the config directory for a true factory reset." ); } + /// A host that changed DHCP lease is re-pointed, not filed a second time. Without this + /// the record a stable id resolves to keeps an address the host has left, so a launch + /// dials into the void while the panel shows the live one. + #[test] + fn a_known_fingerprint_at_a_new_address_moves_the_record() { + let mut known = KnownHosts { + hosts: vec![saved("desk", "192.168.1.9", "abc123")], + }; + // Simulates `hosts add 192.168.1.50 --fp abc123` finding no record at that address. + let by_addr = known + .hosts + .iter() + .position(|h| h.addr == "192.168.1.50" && h.port == 9777); + assert!( + by_addr.is_none(), + "the new address is not yet on any record" + ); + let by_fp = known + .hosts + .iter() + .position(|h| h.fp_hex.eq_ignore_ascii_case("abc123")); + assert_eq!(by_fp, Some(0), "the fingerprint still identifies the host"); + known.hosts[0].addr = "192.168.1.50".into(); + assert_eq!(known.hosts.len(), 1, "one host, one record"); + } + #[test] fn value_reads_the_argument_after_its_flag() { let a = argv(&["--game", "steam:570", "--exec"]); diff --git a/clients/decky/src/hooks.ts b/clients/decky/src/hooks.ts index bb9d6bb5..0c1911dc 100644 --- a/clients/decky/src/hooks.ts +++ b/clients/decky/src/hooks.ts @@ -69,6 +69,14 @@ export interface HostView { fp: string; /** What the host is advertising right now, if anything — what request access would pin. */ advertisedFp: string; + /** + * The host is answering at an address its record does not carry — it changed DHCP lease. + * + * This matters because a launch names the host by [`ref`], and the CLI dials whatever address + * the RECORD holds. So the row would show the live address and dial the dead one. The record + * has to be re-pointed before such a host can stream; `startStream` does it. + */ + moved: boolean; paired: boolean; online: boolean; saved: boolean; @@ -131,6 +139,7 @@ export function mergeHosts(saved: SavedHost[], discovered: DiscoveredHost[]): Ho port: advert?.port ?? s.port, fp: s.fp_hex, advertisedFp: advert?.fp ?? "", + moved: !!advert && (advert.addr !== s.addr || advert.port !== s.port), paired: s.paired, online: !!advert || s.online === true, saved: true, @@ -153,6 +162,7 @@ export function mergeHosts(saved: SavedHost[], discovered: DiscoveredHost[]): Ho // No record, so nothing is pinned — whatever it advertises is an OFFER, not a pin. fp: "", advertisedFp: a.fp, + moved: false, // no record, so nothing to be stale paired: a.paired, online: true, saved: false, @@ -185,9 +195,11 @@ function sortRows(a: HostView, b: HostView): number { export function useHosts() { const [views, setViews] = useState([]); const [scanning, setScanning] = useState(false); - // A client too old for `punktfunk discover`. Rendered as one explanatory row plus the update - // button that fixes it — never as an empty list, which would read as "no hosts on your LAN". - const [outdated, setOutdated] = useState(false); + // Why the list is empty, when it is empty for a reason other than an empty LAN. Rendering + // either of these as "No hosts yet" would blame the user's network for the plugin's problem: + // "client-outdated" — the installed client predates `punktfunk discover` + // "client-unavailable" — there is no client installed at all + const [problem, setProblem] = useState(null); const refresh = useCallback(async () => { setScanning(true); @@ -195,7 +207,14 @@ export function useHosts() { // Both in flight at once: the browse is time-bounded and the probe is network-bound, so // running them in sequence would cost the sum of two waits for no benefit. const [d, s] = await Promise.all([discover(), listHosts()]); - setOutdated(d.error === "client-outdated" || s.error === "client-outdated"); + // Both calls run the same binary, so they fail the same way; take whichever answered. + setProblem( + d.error === "client-unavailable" || s.error === "client-unavailable" + ? "client-unavailable" + : d.error === "client-outdated" || s.error === "client-outdated" + ? "client-outdated" + : null, + ); setViews(mergeHosts(s.hosts ?? [], d.hosts ?? [])); } catch (e) { toaster.toast({ title: "Punktfunk", body: `Couldn't list hosts: ${e}` }); @@ -208,7 +227,7 @@ export function useHosts() { void refresh(); }, [refresh]); - return { views, scanning, outdated, refresh }; + return { views, scanning, problem, refresh }; } // ---------------------------------------------------------------------------------------- diff --git a/clients/decky/src/index.tsx b/clients/decky/src/index.tsx index 6f795da5..f37b7870 100644 --- a/clients/decky/src/index.tsx +++ b/clients/decky/src/index.tsx @@ -125,7 +125,7 @@ const HostRow: FC<{ host: HostView; refresh: () => void }> = ({ host, refresh }) }; const QamPanel: FC = () => { - const { views, scanning, outdated, refresh } = useHosts(); + const { views, scanning, problem, refresh } = useHosts(); const { info: update, checking, check } = useUpdate(); return ( @@ -178,15 +178,23 @@ const QamPanel: FC = () => { {scanning ? "Scanning…" : "Refresh"} - {/* A client too old for `punktfunk discover` explains itself rather than rendering an - empty list — "no hosts on your LAN" would be a lie, and the button that fixes it is - in this same panel. Saved hosts still list: that path is an older verb. */} - {outdated && ( + {/* A client that is missing or too old explains itself rather than rendering an empty + list — "no hosts on your LAN" would blame the network for the plugin's problem, and + for the outdated case the button that fixes it is in this same panel. */} + {problem && ( )} @@ -195,7 +203,7 @@ const QamPanel: FC = () => { )} - {views.length === 0 && !scanning && ( + {views.length === 0 && !scanning && !problem && ( { // (/bin/sh); the wrapper rides behind as an arg. PF_CLIENT_BIN only when the backend resolved // a NATIVE client — else the wrapper's flatpak default stands and this shortcut is exactly // what it always was. - const clientBin = info.client_bin ? `PF_CLIENT_BIN=${info.client_bin} ` : ""; + const clientBin = safeClientBin(info.client_bin) ? `PF_CLIENT_BIN=${info.client_bin} ` : ""; const launchOpts = `${clientBin}PF_BROWSE=1 %command% "${info.runner}"`; // Reuse the remembered entry only if it still exists; a stale appId (deleted shortcut whose @@ -346,6 +346,16 @@ export function isSafeLaunchId(id: string): boolean { ); } +/** + * Is a resolved native-client path safe to put in Steam's launch options? Same rule, separate + * name because the failure is different: an unsafe id is a bug in our own data, an unsafe path + * is just where the user installed the client — so the browse shortcut degrades to its flatpak + * default rather than refusing to exist. + */ +function safeClientBin(bin: string | undefined): bin is string { + return !!bin && isSafeLaunchId(bin); +} + /** * Stream `ref` fullscreen in Gaming Mode, optionally with a pinned card's profile. Encodes the * target into the STREAM shortcut's launch options — one hidden shortcut serves every host — @@ -369,6 +379,12 @@ export async function launchStream(ref: string, opts: LaunchOpts = {}): Promise< // Set only for a NATIVE client install; absent, the wrapper takes its flatpak default, so every // existing Deck install produces byte-identical launch options to before. if (clientBin) { + // The one launch-option value that comes from the backend rather than a store id, and so + // the one that could carry a space: a path like `/home/deck/my apps/punktfunk-client` would + // split Steam's tokenizer and land its tail in front of %command% as a bogus env token. + if (!isSafeLaunchId(clientBin)) { + throw new Error(`client path can't ride Steam's launch options: ${clientBin}`); + } env.push(`PF_CLIENT_BIN=${clientBin}`); } if (opts.profileId) { diff --git a/clients/decky/src/trust.tsx b/clients/decky/src/trust.tsx index d839c023..a18f78e4 100644 --- a/clients/decky/src/trust.tsx +++ b/clients/decky/src/trust.tsx @@ -55,9 +55,22 @@ export const TrustSheet: FC<{ // Request access pins what the host ADVERTISES. The record's own pin is a different thing: // a host that already has one streams without ever opening this sheet. - const canRequestAccess = host.advertisedFp !== ""; + const hasIdentity = host.advertisedFp !== ""; + // A host advertising `pair=optional` admits anyone who pins its identity — there is no + // operator decision to wait for, and asking for one would be a wait that never ends and a + // record claiming somebody approved this Deck when nobody did. `paired` means the PIN + // ceremony or a real approval; the desktop client records exactly this case as *trusted*. + const needsApproval = host.pairPolicy !== "optional"; + const canRequestAccess = hasIdentity && needsApproval; + const canTrustDirectly = hasIdentity && !needsApproval; - const requestAccess = async () => { + /** + * Pin the advertised identity, then stream. + * + * `approval` is what differs between the two doors, and it is not cosmetic: it decides whether + * the launch waits ~185 s for an operator AND whether the record ends up marked paired. + */ + const letIn = async (approval: boolean) => { setBusy(true); setError(null); const { host: h, onStream: stream, onChanged: changed } = props.current; @@ -71,15 +84,17 @@ export const TrustSheet: FC<{ return; } changed(); - // Step 2: the launch itself waits for the approval. The session's plain connecting screen + // Step 2: the launch. Under approval it PARKS — and the session's plain connecting screen // looks identical whether it is parked or hanging, so say what is about to happen BEFORE - // it starts — this toast is a patch over that, and the real fix belongs in the session. - toaster.toast({ - title: "Punktfunk", - body: `Approve this Deck in ${h.name}’s console — the stream starts by itself`, - duration: 10_000, - }); - stream({ requestAccess: true }); + // it starts. That toast is a patch over that, and the real fix belongs in the session. + if (approval) { + toaster.toast({ + title: "Punktfunk", + body: `Approve this Deck in ${h.name}’s console — the stream starts by itself`, + duration: 10_000, + }); + } + stream({ requestAccess: approval }); closeModal?.(); } catch (e) { setError(String(e)); @@ -109,9 +124,11 @@ export const TrustSheet: FC<{ Connect to {host.name}
- {canRequestAccess - ? `${host.name} needs to let this device in before it can stream.` - : "No advertised identity for this host — pair with a PIN instead."} + {!hasIdentity + ? "No advertised identity for this host — pair with a PIN instead." + : canTrustDirectly + ? `${host.name} accepts new devices. Connecting pins its identity so later streams are silent.` + : `${host.name} needs to let this device in before it can stream.`}
{error && (
{error}
@@ -119,10 +136,15 @@ export const TrustSheet: FC<{ {canRequestAccess && ( - + void letIn(true)}> {busy ? : "Request access"} )} + {canTrustDirectly && ( + void letIn(false)}> + {busy ? : "Connect"} + + )} Use a PIN instead… diff --git a/crates/pf-client-core/src/discovery.rs b/crates/pf-client-core/src/discovery.rs index d318b5c5..7df42411 100644 --- a/crates/pf-client-core/src/discovery.rs +++ b/crates/pf-client-core/src/discovery.rs @@ -54,8 +54,8 @@ pub enum DiscoveryEvent { Removed { fullname: String }, } -/// Browse continuously for the app's lifetime. The thread exits when the receiver is -/// dropped (the send fails) or the daemon dies. +/// Browse continuously. The worker exits when the returned receiver is dropped, or when the +/// daemon dies — checked on a tick, so it stops even on a LAN where no advert ever arrives. pub fn browse() -> async_channel::Receiver { let (tx, rx) = async_channel::unbounded(); std::thread::Builder::new() @@ -75,7 +75,24 @@ pub fn browse() -> async_channel::Receiver { return; } }; - while let Ok(event) = receiver.recv() { + // Polled rather than blocked on: the worker has to notice that its consumer went + // away even when NOTHING is arriving, which is the normal state of a LAN with no + // hosts on it. A plain `recv()` parks forever there, and the ignored-event arm below + // never touches `tx` — so a bounded consumer like `discover_for` would leak this + // thread and its daemon (another thread, and a socket bound to :5353) on every call. + loop { + // Checked at the TOP so it also covers the arms below that `continue` without + // ever touching `tx` — the ignored event kinds, and an advert with no IPv4 + // address. Those are the paths that would otherwise keep this thread alive with + // nobody to send to. + if tx.is_closed() { + break; + } + let event = match receiver.recv_timeout(Duration::from_millis(250)) { + Ok(event) => event, + Err(_) if receiver.is_disconnected() => break, + Err(_) => continue, + }; let update = match event { ServiceEvent::ServiceResolved(info) => { let props = info.get_properties(); @@ -171,8 +188,8 @@ pub fn discover_for(timeout: Duration) -> Vec { while let Ok(event) = rx.try_recv() { fold(&mut adverts, event); } - // Dropping the receiver is what stops the worker: its next send fails and the thread exits, - // shutting the daemon down. Without this a one-shot consumer would leak a browse per call. + // Dropping the receiver is what stops the worker — it polls for that, so this holds even + // when nothing is advertising. Without it a one-shot consumer would leak a browse per call. drop(rx); sorted(adverts) }