From f32c3aaa717c96ae852f3ae34d32eba668da6c16 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Wed, 29 Jul 2026 01:04:09 +0200 Subject: [PATCH] =?UTF-8?q?feat(session):=20spec=20mode=20=E2=80=94=20the?= =?UTF-8?q?=20renderer=20stops=20resolving=20policy=20and=20stops=20writin?= =?UTF-8?q?g=20settings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C2 of design/client-architecture-split.md, and the last of the session's overreach. `--resolved-spec ` hands the session everything it needs already resolved — effective settings, the host's clipboard decision, the profile's name — and in that mode it performs ZERO store reads. It had been re-deriving all three, which meant policy was being evaluated inside the thing that draws pixels, and that the spawner and the child could disagree about a file either of them might have written in between. First-party spawns (the shells, the CLI) always pass one now. The compat path stays for hand-run `punktfunk-session --connect` and old Decky scripts — but it calls the SAME helper, so the two modes cannot drift; it is the identical function invoked in-process instead of by the parent. A spec that is named but unreadable fails loudly rather than quietly falling back: a spawner that asked for exact settings must not get store-derived ones instead. The match-window write-back is gone too. The callback used to load-modify-save the shared settings file from inside the renderer — one of that file's five concurrent writers, for a value only the parent needs. It now reports `{"window":{w,h}}` on stdout and the spawner persists it, on a real change only. A hand-run session still persists its own window, because nobody is listening to its stdout there and the event alone would drop the value. Verified on .21: a spec naming a profile that doesn't exist in the catalog is honoured (proving no lookup happened), a missing spec errors instead of falling back, and the CLI's spec file is written and cleaned up per launch. Co-Authored-By: Claude Opus 5 (1M context) --- clients/cli/src/main.rs | 2 + clients/linux/src/spawn.rs | 8 + clients/session/src/console.rs | 6 +- clients/session/src/main.rs | 78 +++++++--- crates/pf-client-core/src/orchestrate.rs | 180 ++++++++++++++++++++++- 5 files changed, 250 insertions(+), 24 deletions(-) diff --git a/clients/cli/src/main.rs b/clients/cli/src/main.rs index eace847b..6e7eb3a7 100644 --- a/clients/cli/src/main.rs +++ b/clients/cli/src/main.rs @@ -571,6 +571,8 @@ punktfunk:// link takes. Exit codes: 0 ok, 2 connect, 3 trust, 4 renderer, 5 not trust_rejected, } => failure = Some((msg, trust_rejected)), SessionEvent::Ended(reason) => eprintln!("{reason}"), + // Persisted by the brain on the way past; nothing to report here. + SessionEvent::Window { .. } => {} SessionEvent::Exited(code) => { return match failure { Some((msg, true)) => { diff --git a/clients/linux/src/spawn.rs b/clients/linux/src/spawn.rs index ce41bd2a..3530745f 100644 --- a/clients/linux/src/spawn.rs +++ b/clients/linux/src/spawn.rs @@ -78,6 +78,12 @@ pub fn spawn_session( wake: false, connect_timeout_secs: opts.connect_timeout_secs, tofu, + // The per-host clipboard decision, resolved here so the child doesn't look it up + // again — matched by address, the way every other per-host lookup matches. + clipboard: pf_client_core::trust::KnownHosts::load() + .hosts + .iter() + .any(|h| h.addr == req.addr && h.port == req.port && h.clipboard_sync), }; let persist_paired = opts.persist_paired; @@ -96,6 +102,8 @@ pub fn spawn_session( trust_rejected, } => error = Some((msg, trust_rejected)), SessionEvent::Ended(msg) => ended = Some(msg), + // The brain persists the window size; the shell has nothing to do with it. + SessionEvent::Window { .. } => {} SessionEvent::Exited(code) => { let _ = sender.send(AppMsg::SessionExited { req: req.clone(), diff --git a/clients/session/src/console.rs b/clients/session/src/console.rs index 07e896a7..0c24fc32 100644 --- a/clients/session/src/console.rs +++ b/clients/session/src/console.rs @@ -183,7 +183,9 @@ pub fn run(target: Option<&str>) -> u8 { window_size: crate::session_main::window_size(&settings_at_start), // Latched at console start (like the stats tier above): toggling Match window in // the console's settings screen applies from the next console launch. - match_window: crate::session_main::match_window(&settings_at_start), + // The console owns its own window across every launch, and no parent is listening to + // its stdout, so it keeps persisting the size itself. + match_window: crate::session_main::match_window(&settings_at_start, true), render_scale: settings_at_start.render_scale, render_scale_max_dim: punktfunk_core::render_scale::max_dimension(&settings_at_start.codec), }; @@ -218,6 +220,8 @@ pub fn run(target: Option<&str>) -> u8 { let mut params = session_params( &settings, profile.map(|p| p.name), + // In-process launch: no spawner resolved a clipboard decision for us. + None, addr.clone(), port, pin, diff --git a/clients/session/src/main.rs b/clients/session/src/main.rs index 3d32640f..aec0d583 100644 --- a/clients/session/src/main.rs +++ b/clients/session/src/main.rs @@ -159,6 +159,7 @@ mod session_main { pub(crate) fn session_params( settings: &trust::Settings, profile: Option, + clipboard_override: Option, addr: String, port: u16, pin: [u8; 32], @@ -169,14 +170,16 @@ mod session_main { force_software: Arc, vulkan: Option, ) -> SessionParams { - // Per-host clipboard opt-in (design/clipboard-and-file-transfer.md §5.3), resolved - // here rather than passed in so every caller — a direct connect and the console's - // own launches — honors the same stored decision. `addr` is moved into the struct + // Per-host clipboard opt-in (design/clipboard-and-file-transfer.md §5.3). In spec + // mode the spawner already resolved it; otherwise this looks it up itself, which is + // the last store read the compat path still owes. `addr` is moved into the struct // below, so read it first. - let clipboard = trust::KnownHosts::load() - .hosts - .iter() - .any(|h| h.addr == addr && h.port == port && h.clipboard_sync); + let clipboard = clipboard_override.unwrap_or_else(|| { + trust::KnownHosts::load() + .hosts + .iter() + .any(|h| h.addr == addr && h.port == port && h.clipboard_sync) + }); // Re-apply the shell-persisted forwarded-controller pin (stable `vid:pid:name` // key) to OUR gamepad service — the shells' in-process services can't reach this // process. Applied per params-build (idempotent; browse re-launches included) so @@ -280,14 +283,24 @@ mod session_main { /// debounced resize→`Reconfigure` machinery on; the callback stores each resize-end's /// logical window size (load-modify-save, like the console settings screen) so the /// next launch opens at it. - pub(crate) fn match_window(settings: &trust::Settings) -> Option> { + /// The Match-window policy hook (design/midstream-resolution-resize.md D1/D2). The + /// callback used to load-modify-save the shared settings file from inside the renderer — + /// one of that file's five concurrent writers, for a value only the parent needs. It now + /// REPORTS the size on stdout and the spawner persists it + /// (design/client-architecture-split.md §5). + /// + /// `persist_locally` keeps a hand-run session remembering its own window: nobody is + /// listening to stdout there, so the event alone would drop the value. A spawned session + /// leaves the write to its parent, which is the whole point. + pub(crate) fn match_window( + settings: &trust::Settings, + persist_locally: bool, + ) -> Option> { settings.match_window.then(|| { - Box::new(|w: u32, h: u32| { - let mut s = trust::Settings::load(); - if (s.last_window_w, s.last_window_h) != (w, h) { - s.last_window_w = w; - s.last_window_h = h; - s.save(); + Box::new(move |w: u32, h: u32| { + println!("{{\"window\":{{\"w\":{w},\"h\":{h}}}}}"); + if persist_locally { + pf_client_core::orchestrate::persist_window_size(w, h); } }) as Box }) @@ -473,12 +486,31 @@ mod session_main { return EXIT_CONNECT_FAILED; } }; - // Global defaults with this host's settings profile overlaid — the binding on the - // host record, or `--profile ` for a one-off (`--profile ""` forces the - // defaults). Resolved through the shared helper, exactly like the console's launches. - let (settings, profile) = trust::effective_settings(&addr, port, profile_arg().as_deref()); - if let Some(p) = &profile { - tracing::info!(profile = %p.name, id = %p.id, "streaming with a settings profile"); + // `--resolved-spec `: the spawner already did the resolving, so this process + // performs ZERO store reads (design/client-architecture-split.md §5) — no Settings + // load, no known-hosts lookup, no profile resolution. Without it (a hand-run + // `--connect`, an old Decky script) the session resolves for itself through the SAME + // helper, so the two modes cannot drift. + let spec = arg_value("--resolved-spec").map(std::path::PathBuf::from); + let (settings, profile_name, clipboard_override) = match &spec { + Some(path) => match pf_client_core::orchestrate::ResolvedSpec::read(path) { + Ok(s) => { + tracing::info!(path = %path.display(), "running from a resolved spec"); + (s.settings, s.profile, Some(s.clipboard)) + } + Err(e) => { + json_line("error", &format!("resolved spec: {e}"), None); + return EXIT_CONNECT_FAILED; + } + }, + None => { + let (settings, profile) = + trust::effective_settings(&addr, port, profile_arg().as_deref()); + (settings, profile.map(|p| p.name), None) + } + }; + if let Some(name) = &profile_name { + tracing::info!(profile = %name, "streaming with a settings profile"); } // Trust follows the GTK client's `--connect` rules: a stored (or `--fp`) pin @@ -540,7 +572,8 @@ mod session_main { #[cfg(not(feature = "ui"))] overlay: None, window_size: window_size(&settings), - match_window: match_window(&settings), + // A spawned session (spec mode) reports its window; a hand-run one persists it. + match_window: match_window(&settings, spec.is_none()), render_scale: settings.render_scale, render_scale_max_dim: punktfunk_core::render_scale::max_dimension(&settings.codec), }; @@ -549,7 +582,8 @@ mod session_main { pf_presenter::run_session(opts, move |gamepad, native, force_software, vulkan| { session_params( &settings, - profile.map(|p| p.name), + profile_name, + clipboard_override, addr, port, pin, diff --git a/crates/pf-client-core/src/orchestrate.rs b/crates/pf-client-core/src/orchestrate.rs index f4568cef..643b2c08 100644 --- a/crates/pf-client-core/src/orchestrate.rs +++ b/crates/pf-client-core/src/orchestrate.rs @@ -23,6 +23,7 @@ use crate::deeplink::{DeepLink, HostResolution, Route}; use crate::profiles::{ProfilesFile, Resolution, StreamProfile}; use crate::trust::{effective_settings, KnownHost, KnownHosts, Settings}; +use serde::{Deserialize, Serialize}; use std::process::{Child, Command, Stdio}; use std::sync::{Arc, Mutex}; @@ -79,6 +80,10 @@ pub struct ConnectPlan { /// The pin came from an advert rather than the store: persist it once the session reports /// ready (ready proves the host really holds that identity). pub tofu: bool, + /// Share this machine's clipboard with this host — a trust decision about the HOST, so it + /// lives on the record rather than in a profile, and the spawner resolves it once here + /// instead of the renderer looking it up again. + pub clipboard: bool, } impl ConnectPlan { @@ -101,6 +106,7 @@ impl ConnectPlan { settings, connect_timeout_secs: None, tofu: false, + clipboard: host.clipboard_sync, } } @@ -140,6 +146,17 @@ impl ConnectPlan { settings, connect_timeout_secs: None, tofu: false, + clipboard: host.clipboard_sync, + } + } + + /// This plan as a [`ResolvedSpec`] — what a first-party spawner hands the session so it + /// performs no store reads of its own. + pub fn spec(&self, clipboard: bool) -> ResolvedSpec { + ResolvedSpec { + settings: self.settings.clone(), + clipboard, + profile: self.profile.as_ref().map(|p| p.name.clone()), } } @@ -448,6 +465,49 @@ pub enum ConnectOutcome { // Session spawn + the stdout contract. // --------------------------------------------------------------------------------------- +/// Everything a session needs, resolved by the caller — the spec `--resolved-spec` carries +/// (design/client-architecture-split.md §5). +/// +/// The session binary is a renderer: given this, it performs ZERO store reads. Its old habit of +/// re-deriving state (loading `Settings`, looking up the host's `clipboard_sync`, resolving the +/// profile) meant policy was being evaluated inside the thing that draws pixels, and that the +/// spawner and the child could disagree about a file either of them might have written since. +/// +/// The compat path — a hand-run `punktfunk-session --connect` with no spec — still resolves for +/// itself, but through the *same* helper (`effective_settings`), so the two modes cannot drift. +#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] +pub struct ResolvedSpec { + /// Effective settings: the global defaults with the chosen profile already applied. + pub settings: Settings, + /// Whether this host may share the clipboard — a per-host trust decision, resolved by the + /// spawner rather than re-looked-up here. + pub clipboard: bool, + /// The profile's name, for the stats overlay. `None` = the global defaults. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub profile: Option, +} + +impl ResolvedSpec { + /// Write the spec somewhere the child can read it, returning the path. Temp files rather + /// than a pipe because the session already takes a file path elsewhere and a crashed + /// spawner leaves something inspectable; the name carries the pid so concurrent launches + /// (a shell and a CLI, or two Decky invocations) never overwrite each other's spec. + pub fn write_temp(&self) -> std::io::Result { + let path = std::env::temp_dir().join(format!("punktfunk-spec-{}.json", std::process::id())); + let json = serde_json::to_vec_pretty(self) + .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; + std::fs::write(&path, json)?; + Ok(path) + } + + /// Read a spec written by the spawner. + pub fn read(path: &std::path::Path) -> std::io::Result { + let text = std::fs::read_to_string(path)?; + serde_json::from_str(&text) + .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e)) + } +} + /// One event from the session child's stdout contract (`{"ready":true}`, `{"error":…}`, /// `{"ended":…}`, then EOF and an exit code). Parsed in one place so a shell, the console and /// the CLI cannot disagree about what "ready" or "trust rejected" means. @@ -460,6 +520,13 @@ pub enum SessionEvent { trust_rejected: bool, }, Ended(String), + /// The session window's logical size settled at this, under the match-window policy. The + /// SPAWNER persists it (design §5): a renderer that load-modify-saves the shared settings + /// file was one of its five concurrent writers, for a value only the parent needs. + Window { + w: u32, + h: u32, + }, /// EOF: the child is gone. `-1` = killed by a signal. Exited(i32), } @@ -480,9 +547,26 @@ pub fn parse_session_line(line: &str) -> Option { if let Some(msg) = v.get("ended").and_then(|m| m.as_str()) { return Some(SessionEvent::Ended(msg.to_string())); } + if let Some(win) = v.get("window") { + let dim = |k: &str| win.get(k).and_then(|n| n.as_u64()).map(|n| n as u32); + if let (Some(w), Some(h)) = (dim("w"), dim("h")) { + return Some(SessionEvent::Window { w, h }); + } + } None } +/// Persist a window size the session reported. The spawner's job now, not the renderer's — and +/// it writes only on a real change, so a session that never resizes never touches the file. +pub fn persist_window_size(w: u32, h: u32) { + let mut s = Settings::load(); + if (s.last_window_w, s.last_window_h) != (w, h) { + s.last_window_w = w; + s.last_window_h = h; + s.save(); + } +} + /// The session binary: installed next to this executable, else `$PATH` (a dev run out of /// `target/…` lands on the sibling). pub fn session_binary() -> std::path::PathBuf { @@ -525,7 +609,23 @@ pub fn spawn_session( on_event: impl FnMut(SessionEvent) + Send + 'static, ) -> Result { let mut cmd = Command::new(session_binary()); - cmd.args(plan.session_args()) + let mut args = plan.session_args(); + // Spec mode: hand the child the settings we already resolved, so it reads no stores and + // cannot disagree with us about a file either of us might write (design §5). A spec we + // fail to write is not fatal — the child's compat path resolves the same values through + // the same helper, which is exactly why that path was kept. + let spec_path = match plan.spec(plan.clipboard).write_temp() { + Ok(path) => { + args.push("--resolved-spec".into()); + args.push(path.to_string_lossy().into_owned()); + Some(path) + } + Err(e) => { + tracing::warn!(error = %e, "couldn't write the resolved spec; the session will resolve for itself"); + None + } + }; + cmd.args(args) .stdin(Stdio::null()) .stdout(Stdio::piped()) .stderr(Stdio::inherit()); // the session's logs interleave with the front-end's @@ -550,9 +650,19 @@ pub fn spawn_session( for line in std::io::BufReader::new(stdout).lines() { let Ok(line) = line else { break }; if let Some(ev) = parse_session_line(&line) { + // The window size is the spawner's to persist — the renderer only reports + // it. Front-ends still see the event; they just don't have to act on it. + if let SessionEvent::Window { w, h } = ev { + persist_window_size(w, h); + } on_event(ev); } } + // The spec has done its job the moment the child has read it; a leftover temp file + // in %TEMP% is litter, and one per launch adds up. + if let Some(path) = &spec_path { + let _ = std::fs::remove_file(path); + } // EOF — reap (a cancel-killed child lands here too; -1 = died on a signal). let code = reader_slot .0 @@ -675,6 +785,7 @@ mod tests { wake: true, connect_timeout_secs: None, tofu: false, + clipboard: false, }; assert_eq!( plan.session_args(), @@ -807,6 +918,65 @@ mod tests { )); } + /// The spec is the whole of what a session needs, and it round-trips — a field lost here + /// is a setting the stream silently doesn't get. + #[test] + fn resolved_spec_round_trips() { + let spec = ResolvedSpec { + settings: Settings { + width: 2560, + height: 1440, + bitrate_kbps: 55000, + codec: "av1".into(), + ..Default::default() + }, + clipboard: true, + profile: Some("Work".into()), + }; + let json = serde_json::to_string(&spec).unwrap(); + assert_eq!(serde_json::from_str::(&json).unwrap(), spec); + + // A spec without a profile is the defaults, and the key is simply absent. + let plain = ResolvedSpec { + profile: None, + ..spec.clone() + }; + let json = serde_json::to_string(&plain).unwrap(); + assert!(!json.contains("profile")); + assert_eq!(serde_json::from_str::(&json).unwrap(), plain); + } + + /// A plan's spec carries the settings the plan resolved — including the profile's name for + /// the overlay, and the host's clipboard decision the renderer no longer looks up. + #[test] + fn plan_spec_carries_what_the_session_may_not_re_derive() { + let h = KnownHost { + name: "Desk".into(), + addr: "192.168.1.50".into(), + fp_hex: "a".repeat(64), + clipboard_sync: true, + profile_id: Some("aaaaaaaaaaaa".into()), + ..Default::default() + }; + let catalog = ProfilesFile { + version: 1, + profiles: vec![crate::profiles::StreamProfile { + id: "aaaaaaaaaaaa".into(), + name: "Game".into(), + overrides: crate::profiles::SettingsOverlay { + bitrate_kbps: Some(80000), + ..Default::default() + }, + ..crate::profiles::StreamProfile::new("") + }], + }; + let plan = ConnectPlan::resolve(&h, None, None, &catalog, &Settings::default()); + let spec = plan.spec(plan.clipboard); + assert_eq!(spec.settings.bitrate_kbps, 80000, "the overlay is baked in"); + assert_eq!(spec.profile.as_deref(), Some("Game")); + assert!(spec.clipboard, "the host's decision, resolved once"); + } + /// The stdout contract, parsed once for every front-end. #[test] fn session_contract_lines() { @@ -832,6 +1002,14 @@ mod tests { parse_session_line(r#"{"ended":"Host ended the session"}"#), Some(SessionEvent::Ended("Host ended the session".into())) ); + // The window report the spawner persists on the session's behalf. + assert_eq!( + parse_session_line(r#"{"window":{"w":1600,"h":900}}"#), + Some(SessionEvent::Window { w: 1600, h: 900 }) + ); + // A half-formed window line is not an event — persisting half a size would be worse + // than ignoring it. + assert_eq!(parse_session_line(r#"{"window":{"w":1600}}"#), None); // Stats lines and stray output are never events. assert_eq!(parse_session_line("stats: 1280×800@60 · 60 fps"), None); assert_eq!(parse_session_line(""), None);