diff --git a/crates/pf-vdisplay/src/vdisplay/layout.rs b/crates/pf-vdisplay/src/vdisplay/layout.rs index e7d71eb1..f3d258ee 100644 --- a/crates/pf-vdisplay/src/vdisplay/layout.rs +++ b/crates/pf-vdisplay/src/vdisplay/layout.rs @@ -10,8 +10,16 @@ //! deterministic. //! * **manual** — per-identity-slot offsets from [`Layout::positions`] (console-arranged): a member //! whose stable identity slot has a stored position sits there; a member with no pin (no stored -//! position, or a shared/anonymous identity that has no slot) falls back to its auto-row origin, so -//! a half-arranged group never collapses everything onto the origin. +//! position, or a shared/anonymous identity that has no slot) is **packed clear of the pins** — +//! rowed left-to-right starting past the rightmost pinned edge — so a half-arranged group neither +//! collapses everything onto the origin nor drops an unpinned display exactly on top of a pinned +//! one. The pins themselves are reproduced verbatim: where two of them overlap, that is the +//! operator's own arrangement and not ours to second-guess. +//! +//! Members carry no height, so "clear of the pins" is decided on the x axis alone and every pin +//! counts regardless of its `y` — a vertically-stacked arrangement therefore packs further right than +//! it strictly needs to. That is the conservative direction: a gap is a cosmetic waste of desktop +//! coordinate space, an overlap is two desktops fighting over the same pixels. //! //! Group membership + acquire order live in the registry ([`super::registry`]); this file only turns //! that ordered member list into positions. @@ -24,8 +32,18 @@ pub struct Member { /// Stable per-client identity slot — the manual-layout key. `None` for a shared/anonymous /// identity (no per-client slot), which can't carry a manual pin and therefore always auto-rows. pub identity_slot: Option, - /// Pixel width, for auto-row `x` accumulation. Clamped at 0 (a bogus negative never shifts a - /// sibling left). + /// The member's width **in the same coordinate space the resulting [`Placement`] is expressed + /// in**, for row `x` accumulation. Clamped at 0 (a bogus negative never shifts a sibling left). + /// + /// ⚠ Every fill site currently uses the requested *mode* width, i.e. pixels. On Windows + /// that is also the desktop space (CCD geometry is pixels), so the two agree; on KWin the + /// placement is handed to `config.position()`, which is the compositor's **logical** space — the + /// two coincide only at scale 1.0, and a per-output scale is exactly what the identity machinery + /// exists to make KDE reapply. A 150 %-scaled 2560-wide output occupies 1707 logical px, so + /// auto-rowing past it by 2560 leaves an 853-px dead band. Fixing that means dividing by the + /// output's applied scale at the KWin fill site (`kwin_output_mgmt` already reads `scale` into + /// its device state); this type stays unit-agnostic, and the contract is that whoever fills it + /// speaks the consumer's space. pub width: i32, } @@ -37,30 +55,64 @@ pub struct Placement { } /// The auto-row origin of member `i`: the summed width of every prior member, top-aligned. +/// `saturating_add` because the widths are client-supplied through the requested mode — an absurd +/// one must produce an absurd coordinate, not a debug-build panic inside the state readout. fn auto_row_x(members: &[Member], i: usize) -> i32 { - members[..i].iter().map(|m| m.width.max(0)).sum() + members[..i] + .iter() + .fold(0i32, |x, m| x.saturating_add(m.width.max(0))) +} + +/// The manual pin for `m`, if its identity slot carries one. The lookup is an exact string match on +/// the canonical decimal slot id — `DisplayPolicy::sanitized` re-keys the table to that form on +/// write, so a `"01"` typed into a hand-edited settings file still resolves here. +fn pin_of(m: &Member, layout: &Layout) -> Option { + m.identity_slot + .and_then(|slot| layout.positions.get(&slot.to_string())) + .map(|p| Placement { x: p.x, y: p.y }) } /// Arrange `members` (in acquire order) per `layout`, returning one [`Placement`] per member in the /// same order. Pure — the single source of truth for auto-row / manual placement, shared by the /// state readout and (KWin) the per-backend position apply. pub fn arrange(members: &[Member], layout: &Layout) -> Vec { - members - .iter() - .enumerate() - .map(|(i, m)| { - let auto = Placement { + match layout.mode { + LayoutMode::AutoRow => (0..members.len()) + .map(|i| Placement { x: auto_row_x(members, i), y: 0, - }; - match layout.mode { - LayoutMode::AutoRow => auto, - // A pinned member sits at its stored offset; an unpinned one falls back to auto-row. - LayoutMode::Manual => m - .identity_slot - .and_then(|slot| layout.positions.get(&slot.to_string())) - .map(|p| Placement { x: p.x, y: p.y }) - .unwrap_or(auto), + }) + .collect(), + LayoutMode::Manual => arrange_manual(members, layout), + } +} + +/// Manual placement: pins verbatim, everything else rowed out past them. +/// +/// The unpinned fallback used to be the unconditional auto-row prefix sum — computed as if the pins +/// did not exist — so an unpinned display could land exactly on top of a pinned sibling with nothing +/// downstream noticing (the arrangement is only ever *reported* and *applied*, never validated). One +/// number in this crate's own fixture separated the tested case from that collision. Rowing the +/// unpinned members from the rightmost pinned edge instead makes the overlap unrepresentable while +/// keeping every property the fallback had: deterministic, acquire-ordered, and identical to plain +/// auto-row when nothing is pinned. +fn arrange_manual(members: &[Member], layout: &Layout) -> Vec { + let pins: Vec> = members.iter().map(|m| pin_of(m, layout)).collect(); + // Start the unpinned row at the desktop origin, or past the rightmost pinned edge when there is + // one. `max(0)` on the width keeps a bogus negative from pulling the cursor back over a pin. + let mut cursor = pins + .iter() + .zip(members) + .filter_map(|(pin, m)| pin.map(|p| p.x.saturating_add(m.width.max(0)))) + .fold(0i32, i32::max); + pins.iter() + .zip(members) + .map(|(pin, m)| match pin { + Some(p) => *p, + None => { + let at = Placement { x: cursor, y: 0 }; + cursor = cursor.saturating_add(m.width.max(0)); + at } }) .collect() @@ -115,14 +167,153 @@ mod tests { } #[test] - fn manual_unpinned_and_slotless_fall_back_to_auto_row() { + fn manual_unpinned_and_slotless_pack_clear_of_the_pins() { let members = [m(Some(1), 2560), m(Some(9), 1920), m(None, 1280)]; // Only slot 1 is pinned; slot 9 has no stored pin; the third has no slot at all. let layout = manual(&[("1", 100, 50)]); let out = arrange(&members, &layout); assert_eq!(out[0], Placement { x: 100, y: 50 }, "pinned"); - assert_eq!(out[1], Placement { x: 2560, y: 0 }, "unpinned → auto-row"); - assert_eq!(out[2], Placement { x: 4480, y: 0 }, "slotless → auto-row"); + // The pin occupies [100, 2660); the unpinned members row out from its right edge in acquire + // order, NOT from the pin-blind prefix sum (which would have put the first one at 2560 — + // inside the pin). + assert_eq!( + out[1], + Placement { x: 2660, y: 0 }, + "unpinned → past the pin" + ); + assert_eq!(out[2], Placement { x: 4580, y: 0 }, "slotless → past both"); + } + + #[test] + fn manual_with_no_pins_at_all_is_plain_auto_row() { + // The fallback must not drift from auto-row when the manual table happens to be empty (the + // state a group is in the instant `manual` is selected and nothing has been arranged yet). + let members = [m(Some(1), 2560), m(Some(2), 1920), m(None, 1280)]; + let out = arrange(&members, &manual(&[])); + assert_eq!(out, arrange(&members, &Layout::default())); + } + + #[test] + fn a_manual_pin_that_would_collide_with_an_auto_row_sibling_is_packed_clear() { + // The exact geometry §13 11.8 names: a pin sitting where the pin-blind auto-row would have + // put the unpinned sibling. Two displays on one origin = two desktops on the same pixels. + let members = [m(Some(1), 2560), m(Some(9), 1920)]; + let layout = manual(&[("1", 2560, 0)]); + let out = arrange(&members, &layout); + assert_eq!(out[0], Placement { x: 2560, y: 0 }, "pin honored verbatim"); + assert_ne!( + out[1], out[0], + "the unpinned sibling must not land on the pin" + ); + assert_eq!( + out[1], + Placement { x: 5120, y: 0 }, + "past the pin's right edge" + ); + } + + #[test] + fn a_pin_left_of_the_origin_still_leaves_the_unpinned_row_at_zero() { + // A negative pin is legal (KWin's global space extends left of 0). Its right edge is what + // matters: at -3000+2560 = -440 it constrains nothing, so the row still starts at the origin. + let members = [m(Some(1), 2560), m(Some(9), 1920)]; + let out = arrange(&members, &manual(&[("1", -3000, 0)])); + assert_eq!(out[0], Placement { x: -3000, y: 0 }); + assert_eq!(out[1], Placement { x: 0, y: 0 }); + } + + #[test] + fn absurd_widths_saturate_instead_of_panicking() { + // Widths originate in the client-requested mode; a hostile or corrupt one must produce an + // absurd coordinate, not an overflow panic inside the `/display/state` readout. + let members = [m(Some(1), i32::MAX), m(Some(2), i32::MAX), m(None, 4096)]; + let out = arrange(&members, &Layout::default()); + assert_eq!(out[2], Placement { x: i32::MAX, y: 0 }); + let out = arrange(&members, &manual(&[("1", i32::MAX, 0)])); + assert_eq!(out[2], Placement { x: i32::MAX, y: 0 }); + } + + /// Property (deterministic seeded walk): across arbitrary member widths, slot assignments and pin + /// tables, **no unpinned member may share desktop space with any sibling**. Overlap between two + /// *pins* is excluded from the invariant — that is the operator's own arrangement, faithfully + /// reproduced. Members carry no height, so "share space" is decided on the x interval alone, + /// which is the strictest reading available here. + #[test] + fn no_unpinned_member_overlaps_a_sibling_under_any_layout() { + // Tiny deterministic LCG (Numerical Recipes) — reproducible, no dependency. Same shape as + // `lifecycle`'s property walk. + let mut rng: u64 = 0x0bad_f00d_dead_beef; + let mut next = || { + rng = rng + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + (rng >> 33) as u32 + }; + + for _ in 0..20_000 { + let count = (next() % 6) as usize; + let members: Vec = (0..count) + .map(|_| { + // A slot only sometimes, and from a small pool so collisions with the pin table + // are frequent; widths include 0 and the odd negative. + let slot = match next() % 4 { + 0 => None, + _ => Some(next() % 6 + 1), + }; + let width = match next() % 8 { + 0 => 0, + 1 => -((next() % 4000) as i32), + _ => (next() % 4000) as i32, + }; + m(slot, width) + }) + .collect(); + let mut pairs: Vec<(String, i32, i32)> = Vec::new(); + for slot in 1..=6u32 { + if next() % 2 == 0 { + let x = (next() % 8000) as i32 - 2000; + let y = ((next() % 3) * 1440) as i32; + pairs.push((slot.to_string(), x, y)); + } + } + let borrowed: Vec<(&str, i32, i32)> = + pairs.iter().map(|(k, x, y)| (k.as_str(), *x, *y)).collect(); + + for layout in [Layout::default(), manual(&borrowed)] { + let out = arrange(&members, &layout); + assert_eq!(out.len(), members.len()); + let pinned: Vec = members + .iter() + .map(|mem| pin_of(mem, &layout).is_some()) + .collect(); + for i in 0..out.len() { + for j in (i + 1)..out.len() { + if pinned[i] && pinned[j] { + continue; // the operator's own arrangement + } + let span = |k: usize| { + let x = out[k].x as i64; + (x, x + members[k].width.max(0) as i64) + }; + let (ai, bi) = span(i); + let (aj, bj) = span(j); + // Empty spans (a zero/negative width) can't collide with anything. + if ai >= bi || aj >= bj { + continue; + } + assert!( + bi <= aj || bj <= ai, + "members {i} {:?} and {j} {:?} overlap under {layout:?} \ + (widths {} / {})", + out[i], + out[j], + members[i].width, + members[j].width + ); + } + } + } + } } #[test] diff --git a/crates/pf-vdisplay/src/vdisplay/policy.rs b/crates/pf-vdisplay/src/vdisplay/policy.rs index ee64b4ea..edf25f53 100644 --- a/crates/pf-vdisplay/src/vdisplay/policy.rs +++ b/crates/pf-vdisplay/src/vdisplay/policy.rs @@ -4,10 +4,14 @@ //! This is the pure config layer that sits **above** the per-compositor [`VirtualDisplay`](super) //! backends: a small set of orthogonal options ([`DisplayPolicy`]) with safe defaults and named //! [`Preset`]s, persisted to `/display-settings.json` and editable from the web console. -//! The lifecycle/registry that *acts* on this policy lands in later stages; **Stage 0** (this file -//! plus the mgmt endpoints) stands up the surface and wires the two behaviors the existing code can -//! already express — the Windows monitor linger duration and the Linux "make the streamed output -//! the sole desktop" topology — through it. +//! Every axis here is now *acted on*, so nothing in this file is a stored-but-inert knob: `keep_alive` +//! by the display lifecycle (`lifecycle` + [`super::registry`]), `topology` by each backend's +//! [`super::effective_topology`] apply, `mode_conflict` by [`super::admission`] before the Welcome, +//! `identity` by the `identity` slot table (whose carriers are the Windows EDID serial + IddCx +//! connector index, KWin's per-slot output name and the host-persisted Mutter scale map), and +//! `layout` by `layout::arrange` — on Linux the *position apply* is KWin-only, everywhere else the +//! arrangement is the `/display/state` readout. This file plus the mgmt endpoints remain the single +//! surface the console writes. //! //! Precedence, mirroring the GPU preference (`console preference > env pin > default`): a present, //! valid `display-settings.json` (console-written) **wins**; when it is absent the host keeps its @@ -43,18 +47,25 @@ pub enum KeepAlive { /// Keep the display for `seconds` after the last session leaves, then tear it down; a reconnect /// inside the window reuses it. Duration { - /// Linger window in seconds. + /// Linger window in seconds, clamped to `0..=86400` on write (see + /// [`DisplayPolicy::sanitized`]): a window longer than a day is `forever` by any honest + /// reading, and `u32` seconds is ~136 years — a deadline the reaper would never reach and a + /// nonsense `expires_in_ms` in `/display/state`. seconds: u32, }, /// Keep the display until host shutdown or an explicit release (the `Pinned` lifecycle state). - /// **Not honored until the display-lifecycle stage** — rejected by the mgmt PUT at Stage 0. + /// Honored end-to-end: the registry resolves it to `Release::Pin`, so the display survives every + /// disconnect — free it with `POST /display/release` (which force-releases `Pinned` exactly like + /// a `Lingering` display). This is what the `gaming-rig` preset selects. Forever, } impl Default for KeepAlive { fn default() -> Self { - // The historical Windows behavior, made explicit; the Linux backends had no linger and map - // `Off`/short-duration onto their (nonexistent) keep-alive as a no-op until the lifecycle stage. + // The historical Windows behavior, made explicit: 10 s is long enough for a client's own + // reconnect (a mode change, a network blip) to reuse the display instead of re-minting it, + // short enough that a walk-away leaves no ghost head on the desktop. Every backend now runs + // the same lifecycle, so this is the linger on Linux too. KeepAlive::Duration { seconds: 10 } } } @@ -100,7 +111,8 @@ pub enum Topology { } /// Admission when a *different* client connects while a display/session is already live and asks for -/// a different mode. Stored at Stage 0; enforced from the mode-conflict admission stage. +/// a different mode. Enforced by [`super::admission`] before the Welcome is sent, so a `reject` is a +/// clean handshake error rather than a half-built session. #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize, ToSchema)] #[serde(rename_all = "snake_case")] pub enum ModeConflict { @@ -115,8 +127,9 @@ pub enum ModeConflict { Reject, } -/// Stable display identity, so desktop environments persist per-display config (KDE scaling). Stored -/// at Stage 0; carriers wired from the identity stage. +/// Stable display identity, so desktop environments persist per-display config (KDE scaling). The +/// slot this resolves to is carried per backend: the Windows EDID serial + IddCx connector index, +/// KWin's per-slot output name, and the host-persisted Mutter scale map. #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize, ToSchema)] #[serde(rename_all = "kebab-case")] pub enum Identity { @@ -130,8 +143,9 @@ pub enum Identity { PerClientMode, } -/// How group members are arranged in the desktop coordinate space. Stored at Stage 0; applied from -/// the multi-monitor stage. +/// How group members are arranged in the desktop coordinate space, resolved by `layout::arrange` — +/// which both the `/display/state` readout and (on Linux, KWin only) the per-backend position apply +/// consume, so the answer is computed in exactly one place. #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize, ToSchema)] #[serde(rename_all = "kebab-case")] pub enum LayoutMode { @@ -155,6 +169,11 @@ pub struct Position { pub struct Layout { #[serde(default)] pub mode: LayoutMode, + /// Keys are the **canonical decimal** identity-slot id (`"1"`..`"15"`) — the exact string + /// `arrange` looks a member up by. [`DisplayPolicy::sanitized`] re-canonicalizes them on write + /// (`"01"` → `"1"`) and drops anything that is not a slot id, because a key that never matches is + /// a pin the operator can see in the console and in `GET /display/settings` while every session + /// silently auto-rows past it. #[serde(default)] pub positions: BTreeMap, } @@ -201,7 +220,9 @@ pub enum Preset { /// single [`EffectivePolicy`]. #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, ToSchema)] pub struct DisplayPolicy { - /// Schema version (currently 1) — lets a future field addition migrate rather than reject. + /// Schema version (currently 1) — lets a future field addition migrate rather than reject. Read + /// at load time ([`DisplayPolicyStore::load_from`] warns when a file claims a version this host + /// does not know, then reads it best-effort) and pinned back to the current version on write. #[serde(default = "one")] pub version: u32, #[serde(default)] @@ -258,6 +279,22 @@ pub struct DisplayPolicy { pub capture_monitor: Option, } +/// The schema version this host writes and understands. A file carrying anything else is still read +/// (every field is `#[serde(default)]`, so a newer document degrades to "the axes we know"), but the +/// mismatch is logged — silently treating a future document as ours is how a migration gets skipped. +const CURRENT_VERSION: u32 = 1; + +/// Upper bound on `KeepAlive::Duration.seconds` (24 h). Anything longer is `forever` in every +/// practical sense, and the unclamped `u32` a PUT could carry (~136 years) produced a deadline the +/// lifecycle reaper never reaches plus a ~4.29e12 ms `expires_in_ms` in the `/display/state` readout. +/// `forever` is the honest way to say "keep it": it is releasable by design via `POST /display/release`. +const MAX_KEEP_ALIVE_SECS: u32 = 24 * 60 * 60; + +/// The highest identity-slot id `identity`'s slot table can ever hand out (its `MAX_ID`) — the upper +/// bound on a usable [`Layout::positions`] key. Mirrored rather than imported because the slot table +/// is a private module; a key above it can never match a member and is dropped at write time. +const MAX_IDENTITY_SLOT: u32 = 15; + fn one() -> u32 { 1 } @@ -268,9 +305,9 @@ fn default_max_displays() -> u32 { impl Default for DisplayPolicy { fn default() -> Self { // Bit-for-bit today's behavior (the `default` preset expanded), so an unconfigured host reads - // the same policy the Stage-0 call sites already produce. + // the same policy the un-overridden call sites already produce. DisplayPolicy { - version: 1, + version: CURRENT_VERSION, preset: Preset::Custom, keep_alive: KeepAlive::default(), topology: Topology::Auto, @@ -286,16 +323,28 @@ impl Default for DisplayPolicy { } } -/// The six resolved fields after preset expansion — what the lifecycle/registry and the Stage-0 call +/// The six resolved fields after preset expansion — what the lifecycle/registry and the policy call /// sites read, and what the mgmt API echoes as the "currently in force" policy. Pure output of /// [`DisplayPolicy::effective`]. +/// +/// Every field is `#[serde(default)]` for the same reason [`DisplayPolicy`]'s are: this shape is also +/// *persisted*, inside each entry of the custom-preset catalog, so a document written before an axis +/// existed (or by a hand edit that dropped one) must still load. Without the defaults a single +/// missing `max_displays` failed the whole `Vec` deserialize and took the operator's +/// entire catalog with it. #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, ToSchema)] pub struct EffectivePolicy { + #[serde(default)] pub keep_alive: KeepAlive, + #[serde(default)] pub topology: Topology, + #[serde(default)] pub mode_conflict: ModeConflict, + #[serde(default)] pub identity: Identity, + #[serde(default)] pub layout: Layout, + #[serde(default = "default_max_displays")] pub max_displays: u32, } @@ -322,11 +371,15 @@ impl DisplayPolicy { } } - /// Clamp fields to their valid ranges (called on write). `max_displays` to `1..=16` (the - /// pf-vdisplay connector ceiling / a sane Linux bound). + /// Clamp fields to their valid ranges (called on write, and on load so a hand-edited file gets + /// the same treatment as a console PUT). `max_displays` to `1..=16` (the pf-vdisplay connector + /// ceiling / a sane Linux bound), the linger window to `MAX_KEEP_ALIVE_SECS`, and the manual + /// layout keys to canonical slot ids. pub fn sanitized(mut self) -> Self { - self.version = 1; + self.version = CURRENT_VERSION; self.max_displays = self.max_displays.clamp(1, 16); + self.keep_alive = clamp_keep_alive(self.keep_alive); + self.layout.positions = canonical_positions(std::mem::take(&mut self.layout.positions)); // A picker that clears its selection sends `""`; that means "no pin", not "match the // monitor named empty string" — same normalization the env knob does. self.capture_monitor = self @@ -337,6 +390,60 @@ impl DisplayPolicy { } } +/// Clamp a keep-alive's linger window to `MAX_KEEP_ALIVE_SECS`. Shared by [`DisplayPolicy::sanitized`] +/// and [`sanitize_preset_fields`] so a custom preset can never smuggle in a window a direct PUT is +/// refused; `Off`/`Forever` carry no window and pass through untouched. +fn clamp_keep_alive(keep_alive: KeepAlive) -> KeepAlive { + match keep_alive { + KeepAlive::Duration { seconds } if seconds > MAX_KEEP_ALIVE_SECS => KeepAlive::Duration { + seconds: MAX_KEEP_ALIVE_SECS, + }, + other => other, + } +} + +/// Re-key a manual layout table to canonical identity-slot ids, dropping (loudly) what can never +/// match a member. +/// +/// `layout::arrange` looks a pin up by `u32::to_string()` — an exact string match — so `"01"`, +/// `"slot1"`, `" 1"` or `"99"` are accepted by the PUT, echoed back by `GET /display/settings` and +/// then silently ignored at arrange time, leaving the operator with a manual arrangement that never +/// takes effect and no signal anywhere. Parsing here makes `"01"` *work* and makes junk visible in +/// the log at write time rather than invisible at stream time. A key already in canonical form wins +/// over an equivalent non-canonical spelling of the same slot, so the result never depends on +/// `BTreeMap` iteration order. +fn canonical_positions(positions: BTreeMap) -> BTreeMap { + use std::collections::btree_map::Entry; + let mut out: BTreeMap = BTreeMap::new(); + for (key, pos) in positions { + let id = key.parse::().ok().filter(|id| { + // `identity`'s slot table only ever hands out 1..=MAX_ID; a pin outside that range is + // addressed to a display that cannot exist. + (1..=MAX_IDENTITY_SLOT).contains(id) + }); + let Some(id) = id else { + tracing::warn!( + key = %key, + "display layout pin keyed by something that is not an identity slot \ + (1..={MAX_IDENTITY_SLOT}) — dropping it; it could never have been applied" + ); + continue; + }; + let canonical = id.to_string(); + let already_canonical = key == canonical; + match out.entry(canonical) { + Entry::Vacant(v) => { + v.insert(pos); + } + Entry::Occupied(mut o) if already_canonical => { + o.insert(pos); + } + Entry::Occupied(_) => {} + } + } + out +} + impl EffectivePolicy { /// Build a persistable `Custom` [`DisplayPolicy`] that keeps THIS effective behavior but replaces /// the arrangement with a **manual** layout at `positions` — the `/display/layout` endpoint's @@ -352,7 +459,7 @@ impl EffectivePolicy { capture_monitor: Option, ) -> DisplayPolicy { DisplayPolicy { - version: 1, + version: CURRENT_VERSION, preset: Preset::Custom, keep_alive: self.keep_alive, topology: self.topology, @@ -434,29 +541,111 @@ pub fn preset_fields(preset: Preset) -> Option { pub struct DisplayPolicyStore { path: PathBuf, /// `Some` only when a valid `display-settings.json` was loaded / written — the "console has - /// configured this host" signal that gates whether Stage-0 call sites override their historical - /// env/default behavior. + /// configured this host" signal that gates whether the policy call sites override their + /// historical env/default behavior. cur: Mutex>, + /// Serializes the whole write transaction (serialize → temp-write → rename → publish to `cur`). + /// Without it two concurrent `PUT /display/settings` can rename in one order and publish to + /// memory in the other, leaving the file and the in-memory value disagreeing — exactly what + /// [`Self::set`]'s contract promises cannot happen. Held *around* the `cur` lock rather than + /// instead of it, so a reader (`get`, on the acquire path) never blocks on disk I/O. + write: Mutex<()>, } impl DisplayPolicyStore { - /// Load from `path`. A missing file ⇒ unconfigured (`None`); a corrupt file ⇒ unconfigured with a - /// warning (never fail host startup over a settings file). + /// Load from `path`. A missing file ⇒ unconfigured (`None`); a corrupt file ⇒ best-effort + /// per-axis salvage, and only a file we cannot make any sense of falls back to unconfigured with + /// a warning (never fail host startup over a settings file). pub fn load_from(path: PathBuf) -> Self { let cur = match std::fs::read(&path) { - Ok(bytes) => match serde_json::from_slice::(&bytes) { - Ok(p) => Some(p), - Err(e) => { - tracing::warn!(path = %path.display(), - "display-settings.json unreadable — using built-in defaults: {e}"); - None - } - }, - Err(_) => None, + Ok(bytes) => Self::parse(&path, &bytes), + // A settings file that exists but cannot be READ (EACCES after a bad chown, EIO on a + // failing disk) is NOT the same thing as an unconfigured host, and the two used to be + // folded into one silent `None` — the console's entire configuration reverting to + // built-in defaults with nothing in the log to say why. Only NotFound is quiet. + Err(e) if e.kind() == std::io::ErrorKind::NotFound => None, + Err(e) => { + tracing::warn!(path = %path.display(), + "display-settings.json exists but could not be read ({e}) — this host is \ + running on BUILT-IN DEFAULTS, not on its configured policy"); + None + } }; DisplayPolicyStore { path, cur: Mutex::new(cur), + write: Mutex::new(()), + } + } + + /// Parse the settings document, salvaging what we can. Split out of [`Self::load_from`] so the + /// recovery rules are unit-tested without touching the filesystem. + /// + /// Three layers, because the failure that used to discard the whole policy was almost never + /// "the file is garbage": (1) the strict parse, which is what a console-written file always + /// takes; (2) a `version` check, so a document from a future host is read best-effort but + /// *announced* rather than silently treated as ours; (3) a per-axis salvage — every field is + /// `#[serde(default)]`, so a member that does not deserialize on its own (an enum variant this + /// build has never heard of, a hand-typed `"max_displays": "four"`) is dropped and the other + /// eleven survive. Dropping one axis to its default is a much smaller lie than reverting the + /// operator's entire configuration. + fn parse(path: &std::path::Path, bytes: &[u8]) -> Option { + let value: serde_json::Value = match serde_json::from_slice(bytes) { + Ok(v) => v, + Err(e) => { + tracing::warn!(path = %path.display(), + "display-settings.json is not valid JSON ({e}) — this host is running on \ + BUILT-IN DEFAULTS, not on its configured policy"); + return None; + } + }; + let claimed = value + .get("version") + .and_then(serde_json::Value::as_u64) + .unwrap_or(CURRENT_VERSION as u64); + if claimed != CURRENT_VERSION as u64 { + tracing::warn!(path = %path.display(), claimed, current = CURRENT_VERSION, + "display-settings.json claims a schema version this host does not know — reading it \ + best-effort (unknown axes are ignored); the next write pins it back to the current \ + version"); + } + match serde_json::from_value::(value.clone()) { + Ok(p) => Some(p.sanitized()), + Err(e) => { + let mut obj = match value { + serde_json::Value::Object(o) => o, + _ => { + tracing::warn!(path = %path.display(), + "display-settings.json is not a JSON object ({e}) — this host is running \ + on BUILT-IN DEFAULTS, not on its configured policy"); + return None; + } + }; + // Probe each member on its own: because every field defaults, a one-key document + // parses iff that key's value is valid, which localizes the failure with no + // hand-maintained field list to drift when an axis is added. + obj.retain(|key, member| { + let one = serde_json::Value::Object( + std::iter::once((key.clone(), member.clone())).collect(), + ); + let ok = serde_json::from_value::(one).is_ok(); + if !ok { + tracing::warn!(path = %path.display(), field = %key, + "display-settings.json carries an unreadable value for this setting — \ + falling back to its built-in default and keeping the rest of the policy"); + } + ok + }); + match serde_json::from_value::(serde_json::Value::Object(obj)) { + Ok(p) => Some(p.sanitized()), + Err(e) => { + tracing::warn!(path = %path.display(), + "display-settings.json unreadable even per-setting ({e}) — this host is \ + running on BUILT-IN DEFAULTS, not on its configured policy"); + None + } + } + } } } @@ -498,20 +687,40 @@ impl DisplayPolicyStore { } /// Persist + adopt a new policy (sanitized first). The in-memory value changes only if the disk - /// write succeeds, so a full disk can't leave memory and file disagreeing. + /// write succeeds, so a full disk can't leave memory and file disagreeing — and the whole + /// transaction runs under [`Self::write`], so neither can two concurrent PUTs. pub fn set(&self, policy: DisplayPolicy) -> Result<()> { let policy = policy.sanitized(); + let _tx = self.write.lock().unwrap_or_else(|e| e.into_inner()); if let Some(dir) = self.path.parent() { pf_paths::create_private_dir(dir)?; } - let tmp = self.path.with_extension("json.tmp"); + let tmp = unique_tmp_path(&self.path); pf_paths::write_secret_file(&tmp, &serde_json::to_vec_pretty(&policy)?)?; - std::fs::rename(&tmp, &self.path)?; + if let Err(e) = std::fs::rename(&tmp, &self.path) { + // The rename is what publishes the write; if it fails the temp file is ours alone + // (unique name) and would otherwise sit in the config dir forever. + let _ = std::fs::remove_file(&tmp); + return Err(e.into()); + } *self.cur.lock().unwrap() = Some(policy); Ok(()) } } +/// A temp path for a temp-write + atomic-rename that no other writer can be using: `...tmp`. +/// A *fixed* `.json.tmp` is safe only while one thread writes at a time — two host processes over the +/// same config dir (a service plus a hand-run binary, an upgrade overlap) would otherwise interleave +/// their partial writes into one file and rename the mixture over the real one. +fn unique_tmp_path(path: &std::path::Path) -> PathBuf { + use std::sync::atomic::{AtomicU64, Ordering}; + static SEQ: AtomicU64 = AtomicU64::new(0); + let n = SEQ.fetch_add(1, Ordering::Relaxed); + let mut name = path.file_name().unwrap_or_default().to_os_string(); + name.push(format!(".{}.{n}.tmp", std::process::id())); + path.with_file_name(name) +} + /// The process-wide display-policy store (config-dir file), loaded once on first access — the same /// global-accessor shape as `pf_gpu::prefs`, because display setup happens deep in the /// capture/vdisplay path where no app state is threaded. @@ -564,64 +773,187 @@ fn custom_presets_path() -> PathBuf { } /// Clamp a saved preset's fields to their valid ranges — the same bounds [`DisplayPolicy::sanitized`] -/// enforces, so a preset can never carry an out-of-range `max_displays` that a later apply would reject. +/// enforces, so a preset can never carry an out-of-range `max_displays` or linger window that a later +/// apply would silently smuggle past the direct PUT's checks. fn sanitize_preset_fields(mut fields: EffectivePolicy) -> EffectivePolicy { fields.max_displays = fields.max_displays.clamp(1, 16); + fields.keep_alive = clamp_keep_alive(fields.keep_alive); + fields.layout.positions = canonical_positions(std::mem::take(&mut fields.layout.positions)); fields } -/// Load the saved custom presets (empty + non-fatal if the file is absent or malformed — a bad -/// catalog never breaks the console's settings GET). -pub fn load_custom_presets() -> Vec { +/// What a catalog read recovered: the entries we could make sense of, plus whether anything was lost +/// getting there. `lossy` is the flag the CRUD path checks before it overwrites the file — a save +/// that would drop entries must preserve the original first. +struct CatalogRead { + presets: Vec, + lossy: bool, +} + +/// Parse the catalog **entry-wise**. Pure (no I/O) so the recovery rules are unit-tested. +/// +/// The whole-document `from_slice::>` this replaces made every entry hostage to +/// every other: one hand-edited preset missing a field, or naming an enum variant this build does not +/// know, returned `Vec::new()` — and because the CRUD path is load → mutate → save, the very next +/// "create preset" atomically renamed that one-element vector over the operator's entire catalog. +/// Here a bad entry costs exactly itself, and the caller learns (via `lossy`) that the file on disk +/// holds more than we understood. +fn parse_catalog(bytes: &[u8]) -> CatalogRead { + let entries: Vec = match serde_json::from_slice(bytes) { + Ok(v) => v, + Err(e) => { + tracing::warn!(error = %e, + "display-presets.json is not a JSON array of presets — ignoring the custom-preset \ + catalog; it is preserved as display-presets.json.bad if anything overwrites it"); + return CatalogRead { + presets: Vec::new(), + lossy: true, + }; + } + }; + let mut presets = Vec::with_capacity(entries.len()); + let mut lossy = false; + for (i, entry) in entries.into_iter().enumerate() { + // Keep the id/name for the log even when the body is unreadable — "entry 3" is useless to an + // operator staring at a console list of names. + let named = entry + .get("name") + .and_then(serde_json::Value::as_str) + .unwrap_or("") + .to_string(); + match serde_json::from_value::(entry) { + Ok(mut p) => { + p.fields = sanitize_preset_fields(p.fields); + presets.push(p); + } + Err(e) => { + lossy = true; + tracing::warn!(index = i, name = %named, error = %e, + "display-presets.json entry is unreadable — skipping just this preset, the rest \ + of the catalog is kept"); + } + } + } + CatalogRead { presets, lossy } +} + +/// Read + parse the catalog file. `Ok(CatalogRead)` for "absent" (an empty catalog) and for +/// "readable, possibly lossy"; `Err` only when the file exists and the OS refused to hand it over +/// (EACCES/EIO) — which the CRUD path must NOT paper over, because writing back what we could read +/// (nothing) would erase a catalog that is merely unreachable. +fn read_catalog() -> Result { match std::fs::read(custom_presets_path()) { - Ok(bytes) => serde_json::from_slice(&bytes).unwrap_or_else(|e| { - tracing::warn!(error = %e, "display-presets.json malformed — ignoring custom presets"); - Vec::new() + Ok(bytes) => Ok(parse_catalog(&bytes)), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(CatalogRead { + presets: Vec::new(), + lossy: false, }), - Err(_) => Vec::new(), + Err(e) => Err(e.into()), } } +/// Copy the catalog aside to `display-presets.json.bad` before a save that would not round-trip it. +/// A copy, not a rename: the original stays in place until the atomic rename replaces it, so a crash +/// in between still leaves a catalog where the host looks for one. Best-effort — failing to preserve +/// a file we already could not fully read must not fail the operator's write. +fn quarantine_catalog() { + let path = custom_presets_path(); + let bad = path.with_extension("json.bad"); + match std::fs::copy(&path, &bad) { + Ok(_) => tracing::warn!(path = %bad.display(), + "the custom-preset catalog held entries this host could not read; the original was \ + copied aside before being rewritten"), + Err(e) => tracing::warn!(error = %e, path = %bad.display(), + "could not preserve the unreadable custom-preset catalog before rewriting it"), + } +} + +/// Serializes the catalog's read → mutate → save transaction. The three CRUD entry points are free +/// functions over one shared file, so without this a concurrent add + delete each write back the +/// catalog they loaded and one of the two edits vanishes wholesale. +static CATALOG_LOCK: Mutex<()> = Mutex::new(()); + /// Persist the catalog (private dir, temp-write + atomic rename — the [`DisplayPolicyStore::set`] -/// discipline, so a crash mid-write never truncates it). +/// discipline, so a crash mid-write never truncates it). Callers hold [`CATALOG_LOCK`]. fn save_custom_presets(presets: &[CustomPreset]) -> Result<()> { let path = custom_presets_path(); if let Some(dir) = path.parent() { pf_paths::create_private_dir(dir)?; } - let tmp = path.with_extension("json.tmp"); + let tmp = unique_tmp_path(&path); pf_paths::write_secret_file(&tmp, &serde_json::to_vec_pretty(presets)?)?; - std::fs::rename(&tmp, &path)?; + if let Err(e) = std::fs::rename(&tmp, &path) { + let _ = std::fs::remove_file(&tmp); + return Err(e.into()); + } Ok(()) } -/// 12 hex chars from the name + wall-clock nanos — collision-free in practice, no uuid dep (the -/// the host `library` custom-entry id scheme). -fn new_preset_id(name: &str) -> String { +/// Load the saved custom presets (empty + non-fatal if the file is absent, unreadable or malformed — +/// a bad catalog never breaks the console's settings GET). +pub fn load_custom_presets() -> Vec { + match read_catalog() { + Ok(c) => c.presets, + Err(e) => { + tracing::warn!(error = %e, + "display-presets.json exists but could not be read — the console will show no custom \ + presets; the file itself is untouched"); + Vec::new() + } + } +} + +/// 12 hex chars from the name + wall-clock nanos + a `nonce` — no uuid dep (the host `library` +/// custom-entry id scheme). The nonce exists because the name+nanos pair is NOT unique: two creates +/// of the same name inside one clock tick (a double-clicked Save, two console tabs, a clock that +/// stepped back) hash identically, after which `update_custom_preset`'s `find(|p| p.id == id)` +/// silently edits whichever landed first. +fn preset_id(name: &str, nanos: u128, nonce: u64) -> String { + hex::encode(&Sha256::digest(format!("{name}:{nanos}:{nonce}").as_bytes())[..6]) +} + +/// The first id not already taken in `presets`, re-rolling the nonce against a **single** clock read +/// — 48 bits is collision-free in practice only if something actually checks, and re-reading the +/// clock per attempt would make the retry indistinguishable from luck (and untestable). +fn free_preset_id_at(presets: &[CustomPreset], name: &str, nanos: u128) -> String { + (0u64..) + .map(|nonce| preset_id(name, nanos, nonce)) + .find(|id| presets.iter().all(|p| &p.id != id)) + .expect("the nonce space is unbounded, so some id is always free") +} + +fn free_preset_id(presets: &[CustomPreset], name: &str) -> String { let nanos = SystemTime::now() .duration_since(UNIX_EPOCH) .map(|d| d.as_nanos()) .unwrap_or(0); - hex::encode(&Sha256::digest(format!("{name}:{nanos}").as_bytes())[..6]) + free_preset_id_at(presets, name, nanos) } /// Create a custom preset, returning it with its assigned id. pub fn add_custom_preset(input: CustomPresetInput) -> Result { - let mut presets = load_custom_presets(); + let _tx = CATALOG_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let catalog = read_catalog()?; + let mut presets = catalog.presets; let preset = CustomPreset { - id: new_preset_id(&input.name), + id: free_preset_id(&presets, &input.name), name: input.name, fields: sanitize_preset_fields(input.fields), game_session: input.game_session, }; presets.push(preset.clone()); + if catalog.lossy { + quarantine_catalog(); + } save_custom_presets(&presets)?; Ok(preset) } /// Replace a custom preset's fields (id preserved). `None` ⇒ no preset with that id. pub fn update_custom_preset(id: &str, input: CustomPresetInput) -> Result> { - let mut presets = load_custom_presets(); + let _tx = CATALOG_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let catalog = read_catalog()?; + let mut presets = catalog.presets; let Some(slot) = presets.iter_mut().find(|p| p.id == id) else { return Ok(None); }; @@ -629,18 +961,26 @@ pub fn update_custom_preset(id: &str, input: CustomPresetInput) -> Result