diff --git a/crates/pf-vdisplay/src/vdisplay/linux/hyprland.rs b/crates/pf-vdisplay/src/vdisplay/linux/hyprland.rs index ab4a57e9..d0f847a3 100644 --- a/crates/pf-vdisplay/src/vdisplay/linux/hyprland.rs +++ b/crates/pf-vdisplay/src/vdisplay/linux/hyprland.rs @@ -5,9 +5,10 @@ //! protocols, so it shares the wlr virtual-input path with sway — but it needs its own IPC and //! portal, so it is a **distinct backend** from [`super::wlroots`], not a branch inside it (D1): //! -//! 1. `hyprctl output create headless PF-` adds a named headless output — Hyprland supports +//! 1. `hyprctl output create headless PF--` adds a named headless output — Hyprland supports //! **explicit names**, so no before/after diffing like sway's `HEADLESS-N` (D6). We poll -//! `hyprctl -j monitors` until the name shows up. +//! `hyprctl -j monitors` until the name shows up. The creator's pid rides in the name so a +//! crashed host's leftovers are attributable, and only those (see [`reclaim_leftovers_once`]). //! 2. A monitor rule sets the client's exact mode. [`set_monitor_rule`] uses `hyprctl keyword //! monitor NAME,WxH@Hz,auto,1` (the hyprlang path — the default config manager on every current //! release, ≥0.55 included) and falls back to the Lua `hyprctl eval 'hl.monitor{…}'` only for a @@ -69,12 +70,46 @@ fn picker_selection_line(name: &str) -> String { format!("[SELECTION]screen:{name}\n") } -/// Monotonic per-process counter for headless output names (`PF-1`, `PF-2`, …). Named outputs kill -/// the before/after diff race sway needs (D6). +/// Monotonic per-process counter for headless output names (`PF--1`, `PF--2`, …). Named +/// outputs kill the before/after diff race sway needs (D6). static OUTPUT_SEQ: AtomicU32 = AtomicU32::new(0); +/// The name for our next headless output: `PF--`. +/// +/// The pid is not decoration. `OutputGuard::drop` is the only thing that removes an output, so a +/// host that was SIGKILLed leaves its outputs in the compositor — and a bare `PF-` counter starts +/// again at `PF-1` in the next process, colliding with the corpses it just inherited. Stamping the +/// creator's pid into the name makes a leftover both recognisable and *attributable*, which is what +/// lets [`reclaim_leftovers_once`] remove only the ones whose owner is gone. fn next_output_name() -> String { - format!("PF-{}", OUTPUT_SEQ.fetch_add(1, Ordering::Relaxed) + 1) + format!( + "PF-{}-{}", + std::process::id(), + OUTPUT_SEQ.fetch_add(1, Ordering::Relaxed) + 1 + ) +} + +/// Is `name` an output some punktfunk host created (`PF--`, or a legacy `PF-`)? Pure — +/// this is what [`list_monitors`] reports as `managed`, so a user's own monitor called `PF-office` +/// must not qualify. +fn is_managed_output(name: &str) -> bool { + let Some(rest) = name.strip_prefix("PF-") else { + return false; + }; + !rest.is_empty() + && rest + .split('-') + .all(|part| !part.is_empty() && part.bytes().all(|b| b.is_ascii_digit())) +} + +/// The pid of the host that created `name`, for `PF--` only. `None` for anything else — +/// including a legacy `PF-` from a host older than this naming scheme, which carries no owner and +/// therefore may not be reclaimed on a guess. +fn output_owner_pid(name: &str) -> Option { + let rest = name.strip_prefix("PF-")?; + let (pid, seq) = rest.split_once('-')?; + seq.parse::().ok()?; + pid.parse::().ok() } /// The Hyprland virtual-display driver. Stateless — each [`create`](VirtualDisplay::create) adds one @@ -100,11 +135,24 @@ impl HyprlandDisplay { /// under `$XDG_RUNTIME_DIR/hypr/*/.socket.sock` (so the systemd `--user` host works without env /// import, unlike sway's `SWAYSOCK`; the signature is then exported by `apply_session_env`). Cheap, /// side-effect-free — safe on the enumeration path. +/// +/// Both env reads take [`crate::with_env_lock`] — in ONE scope, so the pair is sampled from a single +/// consistent view. This runs on a management worker (`/host/compositors` → [`crate::available`]) +/// concurrently with another connect's `apply_session_env`, which `set_var`s the signature for a +/// live Hyprland session and `remove_var`s it for anything else; a glibc `getenv` racing that +/// `setenv`/`unsetenv` is the `environ` realloc data race ENV_LOCK exists for. No caller holds the +/// lock (it is not reentrant), and the `read_dir` below deliberately runs outside it. pub fn is_available() -> bool { - if std::env::var_os("HYPRLAND_INSTANCE_SIGNATURE").is_some() { + let (sig, runtime) = crate::with_env_lock(|| { + ( + std::env::var_os("HYPRLAND_INSTANCE_SIGNATURE"), + std::env::var_os("XDG_RUNTIME_DIR"), + ) + }); + if sig.is_some() { return true; } - let dir = match std::env::var_os("XDG_RUNTIME_DIR") { + let dir = match runtime { Some(d) => std::path::PathBuf::from(d).join("hypr"), None => return false, }; @@ -147,6 +195,9 @@ impl VirtualDisplay for HyprlandDisplay { fn create(&mut self, mode: Mode) -> Result { // Log the permission-system caveat once per process (silent black frames otherwise). preflight_once(); + // Remove any output a PREVIOUS host left in this compositor, before we mint our first. + reclaim_leftovers_once(); + warn_topology_is_extend_only(); let name = next_output_name(); hyprctl_dispatch(&["output", "create", "headless", &name]).with_context(|| { @@ -181,7 +232,7 @@ impl VirtualDisplay for HyprlandDisplay { remote_fd: Some(fd), preferred_mode: Some((mode.width, mode.height, mode.refresh_hz)), keepalive: Box::new(Keepalive { - _stop: StopGuard(stop), + _stop: stop, _output: output, }), // Owned (the compositor output is ours to tear down), but not registry-poolable: the @@ -212,6 +263,62 @@ impl Drop for StopGuard { } } +/// Remove the `PF--` outputs left behind by host processes that are **gone**, once per +/// process before we create our first. +/// +/// [`OutputGuard::drop`] is the only unplug path there is, so a host that was SIGKILLed, OOM-killed +/// or crashed leaves its headless outputs in the compositor for as long as the Hyprland session +/// lives — a dead `PF-…` head in the operator's layout, forever, with the next host start happily +/// adding more beside it. Reclaim is keyed on the OWNER pid in the name and only removes an output +/// whose creator no longer exists, so a second live host on the same session (or this very process) +/// can never have its output pulled out from under it. `Once` puts the sweep strictly before this +/// process owns anything, and blocks a concurrent first `create` until it is done. +fn reclaim_leftovers_once() { + static RECLAIMED: Once = Once::new(); + RECLAIMED.call_once(|| { + let Ok(names) = monitor_names() else { return }; + for name in names { + let Some(pid) = output_owner_pid(&name) else { + // Either not ours, or a legacy `PF-` with no owner recorded — which we must not + // remove on a guess, because a still-running older host may be streaming it. + if is_managed_output(&name) { + tracing::debug!(output = %name, "a managed headless output with no owner pid in \ + its name (an older host build) — left alone"); + } + continue; + }; + if pid == std::process::id() || std::path::Path::new(&format!("/proc/{pid}")).exists() { + continue; + } + match hyprctl_dispatch(&["output", "remove", &name]) { + Ok(()) => tracing::info!(output = %name, owner_pid = pid, "removed a headless \ + output left behind by a host that is no longer running"), + Err(e) => tracing::warn!(output = %name, owner_pid = pid, error = %format!("{e:#}"), + "could not remove a leftover headless output"), + } + } + }); +} + +/// The configured [`crate::policy::Topology`] is not implemented on this backend — say so once per +/// create instead of leaving the management API's echo as the only signal that the pin was dropped +/// (sweep 13.18). The Hyprland headless output is always an EXTENSION: nothing here promotes it to +/// primary or disables the operator's heads. +fn warn_topology_is_extend_only() { + let topology = crate::effective_topology(); + if !matches!( + topology, + crate::policy::Topology::Extend | crate::policy::Topology::Auto + ) { + tracing::warn!( + ?topology, + "hyprland: this backend implements EXTEND only — the headless output is added beside \ + the operator's heads and nothing is promoted or disabled. Configure `topology: extend` \ + to stop the console promising otherwise." + ); + } +} + /// Owns the created headless output; dropping it removes it from Hyprland. struct OutputGuard(String); @@ -226,14 +333,25 @@ impl Drop for OutputGuard { } } +/// Budget for one `hyprctl` call ([`crate::proc`]). +/// +/// `hyprctl` is a client of the compositor it drives — it connects to the instance socket and waits +/// for a reply, so against a wedged Hyprland it never returns. These calls run on the session's +/// stream thread, whose only way to end a session is to return, so one hung query used to wedge the +/// session for good. Generous next to a healthy call (single-digit milliseconds), and every call +/// site already has a failed-query path. +const HYPRCTL_BUDGET: Duration = Duration::from_secs(5); + +/// Budget for the one-shot xdph restart. `systemctl --user try-restart` waits for the user manager's +/// job to settle, so it is the slowest helper on this path — and its result is already ignored. +const PORTAL_RESTART_BUDGET: Duration = Duration::from_secs(10); + /// Run `hyprctl `, returning stdout. `hyprctl` reads `HYPRLAND_INSTANCE_SIGNATURE` from the /// env (exported by `apply_session_env`) to reach the right instance socket. It exits non-zero on a /// hard failure, but for dispatch commands it can print an error with status 0 — see /// [`hyprctl_dispatch`]. fn hyprctl(args: &[&str]) -> Result { - let out = Command::new("hyprctl") - .args(args) - .output() + let out = crate::proc::output_within(Command::new("hyprctl").args(args), HYPRCTL_BUDGET) .context("run hyprctl (is Hyprland installed?)")?; if !out.status.success() { bail!( @@ -251,12 +369,36 @@ fn hyprctl(args: &[&str]) -> Result { /// write between ours and xdph's read would silently steer capture at the other session's output. static SELECTION_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); +/// The per-session selection file, removed when the handshake it steers is over. +/// +/// Its lifetime is the HANDSHAKE, not the session: the shim cats it once, inside +/// [`select_and_cast`]'s critical section, and everything after that is the cast's own business. +/// Left behind (as it was) the stale `[SELECTION]screen:PF-…` outlives the output `Drop` has since +/// removed, and it permanently shadows xdph's documented empty-read fallback — every later capture +/// that reaches the picker without a session of ours is steered at an output that is gone. Tying +/// removal to the CAST instead would be worse: the file is one per user, so a session ending hours +/// later would delete a *sibling's* selection out from under its picker. +struct SelectionFile(String); + +impl Drop for SelectionFile { + fn drop(&mut self) { + if let Err(e) = std::fs::remove_file(&self.0) { + if e.kind() != std::io::ErrorKind::NotFound { + tracing::debug!(path = %self.0, error = %e, "could not remove the xdph selection file"); + } + } + } +} + /// Point xdph's custom picker at `output` and run the ScreenCast handshake, returning the portal fd /// + node id and the guard that stops the cast. The caller must hold [`SELECTION_LOCK`]. -fn select_and_cast(output: &str, hw_cursor: bool) -> Result<(OwnedFd, u32, Arc)> { +fn select_and_cast(output: &str, hw_cursor: bool) -> Result<(OwnedFd, u32, StopGuard)> { ensure_xdph_config()?; let sel = selection_file(); std::fs::write(&sel, picker_selection_line(output)).with_context(|| format!("write {sel}"))?; + // Owned from the write on: every arm below (and every `?`) leaves the handshake, which is the + // only thing that reads it. + let _sel_file = SelectionFile(sel); let (setup_tx, setup_rx) = std::sync::mpsc::channel::>(); let stop = Arc::new(AtomicBool::new(false)); let stop_thread = stop.clone(); @@ -264,8 +406,16 @@ fn select_and_cast(output: &str, hw_cursor: bool) -> Result<(OwnedFd, u32, Arc Ok((fd, node_id, stop)), + Ok(Ok((fd, node_id))) => Ok((fd, node_id, guard)), Ok(Err(e)) => bail!("ScreenCast portal on {output} failed: {e}"), Err(_) => bail!("timed out waiting for the ScreenCast portal on {output}"), } @@ -285,7 +435,7 @@ pub(crate) fn stream_existing_output( Ok(crate::mirror::MirrorStream { node_id, remote_fd: Some(fd), - keepalive: Box::new(StopGuard(stop)), + keepalive: Box::new(stop), }) } @@ -330,11 +480,12 @@ pub(crate) fn list_monitors() -> Result> { .unwrap_or(1.0), primary: m.get("focused").and_then(|v| v.as_bool()).unwrap_or(false), enabled: !m.get("disabled").and_then(|v| v.as_bool()).unwrap_or(false), - // Our headless outputs are named `PF-` (see `next_output_name`). + // Our headless outputs are named `PF--` (see `next_output_name`); the shape + // is checked, not just the prefix, so a user's own `PF-office` stays theirs. managed: m .get("name") .and_then(|v| v.as_str()) - .is_some_and(|n| n.starts_with("PF-")), + .is_some_and(is_managed_output), }) }) .collect(); @@ -382,6 +533,23 @@ fn wait_monitor_ready(name: &str, timeout: Duration) -> Result<()> { } } +/// Every monitor name Hyprland reports, **disabled ones included** (`-j monitors all`) — a leftover +/// output from a dead host may well have ended up disabled, and [`reclaim_leftovers_once`] must see +/// it anyway. +fn monitor_names() -> Result> { + let out = hyprctl(&["-j", "monitors", "all"])?; + let monitors: serde_json::Value = + serde_json::from_str(&out).context("parse hyprctl -j monitors all")?; + Ok(monitors + .as_array() + .map(|a| { + a.iter() + .filter_map(|m| m.get("name").and_then(|n| n.as_str()).map(str::to_owned)) + .collect() + }) + .unwrap_or_default()) +} + /// Is a monitor named `name` present in `hyprctl -j monitors` (JSON)? fn monitor_exists(name: &str) -> Result { let out = hyprctl(&["-j", "monitors"])?; @@ -417,17 +585,33 @@ fn set_monitor_rule(name: &str, mode: Mode) -> Result<()> { ); let keyword: Vec<&str> = vec!["keyword", "monitor", &spec]; let eval: Vec<&str> = vec!["eval", &lua]; + // What each form actually said. hyprctl reports a rejection in its OUTPUT TEXT ("eval is only + // supported with the lua config manager", "invalid monitor rule", a permission denial), and + // dropping it on the floor with `.is_err()` is what left the failure below guessing at GBM when + // the compositor had already named the real cause. + let mut attempts: Vec = Vec::new(); for a in [&keyword, &eval] { // A wrong-era command errors (`keyword` gone under Lua, or `eval` under hyprlang) — skip to // the other form. A command that's accepted then has up to the timeout to take effect. - if hyprctl_dispatch(a).is_err() { + if let Err(e) = hyprctl_dispatch(a) { + let said = format!("{e:#}"); + tracing::debug!(output = %name, cmd = ?a, error = %said, "hyprctl rejected this monitor-rule form — trying the other config era"); + attempts.push(said); continue; } if wait_exact_mode(name, mode, Duration::from_millis(1500)) { tracing::debug!(output = %name, cmd = ?a, w = mode.width, h = mode.height, "monitor adopted the requested mode"); return Ok(()); } + attempts.push(format!( + "hyprctl {a:?} was accepted but the mode never took effect" + )); } + let said = if attempts.is_empty() { + "nothing (no form was attempted)".to_string() + } else { + attempts.join("; ") + }; // Neither form produced the exact mode. Distinguish "usable but different size" (proceed with a // warning — a working stream beats none) from "0×0 / gone" (the output has no framebuffer at all). match monitor_size(name)? { @@ -436,14 +620,20 @@ fn set_monitor_rule(name: &str, mode: Mode) -> Result<()> { output = %name, requested = %format!("{}x{}", mode.width, mode.height), got = %format!("{w}x{h}"), + hyprctl = %said, "Hyprland did not adopt the exact requested mode — streaming at the output's current size" ); Ok(()) } + // The output has no framebuffer at all. Lead with what hyprctl SAID: if every form was + // rejected the cause is named right there (wrong config era, a permission denial, a bad + // rule) and no allocation was ever attempted; only a form that was accepted and still left + // the output at 0×0 points at the compositor failing to back the mode. _ => bail!( "headless output {name} never got a framebuffer (stayed 0x0) after the monitor rule for \ - {}x{}@{hz} — the compositor could not back the mode, likely a headless GBM/dmabuf \ - allocation failure (GPU driver; cf. Sunshine#4197). Check the Hyprland log.", + {}x{}@{hz}. hyprctl said: {said}. If a form was accepted, the compositor could not back \ + the mode — likely a headless GBM/dmabuf allocation failure (GPU driver; cf. \ + Sunshine#4197). Check the Hyprland log.", mode.width, mode.height ), @@ -574,13 +764,17 @@ fn ensure_xdph_config() -> Result<()> { return Ok(()); } tracing::info!(path = %path.display(), "pointed xdg-desktop-portal-hyprland at the managed picker shim"); - let _ = Command::new("systemctl") - .args([ + // Bounded: `systemctl --user` blocks on the user manager's job queue, and this runs on the + // session's stream thread. Its result was already ignored — a timeout just means xdph picks the + // new config up whenever it next starts. + let _ = crate::proc::status_within( + Command::new("systemctl").args([ "--user", "try-restart", "xdg-desktop-portal-hyprland.service", - ]) - .status(); + ]), + PORTAL_RESTART_BUDGET, + ); Ok(()) } @@ -702,6 +896,28 @@ mod tests { assert_ne!(a, b); } + /// The name carries the creating host's pid, which is what makes a leftover attributable — a + /// reclaim that could not tell whose output it was would have to remove a LIVE sibling's or + /// nothing at all. + #[test] + fn a_name_carries_its_owner_pid_and_only_ours_does() { + let mine = next_output_name(); + assert_eq!(output_owner_pid(&mine), Some(std::process::id())); + assert!(is_managed_output(&mine)); + + // A legacy `PF-` from an older host: recognisably managed, but with no owner recorded — + // so it may be reported, never reclaimed on a guess. + assert!(is_managed_output("PF-1")); + assert_eq!(output_owner_pid("PF-1"), None); + + // Not ours: a user's own monitor name that happens to start with the prefix, and the + // connectors every wlr-family compositor mints. + for theirs in ["PF-office", "PF-", "PF-12-abc", "HEADLESS-1", "DP-1", ""] { + assert!(!is_managed_output(theirs), "{theirs:?} is not ours"); + assert_eq!(output_owner_pid(theirs), None, "{theirs:?} has no owner"); + } + } + #[test] fn picker_line_carries_the_selection_marker() { // xdph requires the `[SELECTION]` prefix; a bare `screen:NAME` is rejected as strange output. diff --git a/crates/pf-vdisplay/src/vdisplay/linux/mutter.rs b/crates/pf-vdisplay/src/vdisplay/linux/mutter.rs index 9a0dde2c..a4101c4d 100644 --- a/crates/pf-vdisplay/src/vdisplay/linux/mutter.rs +++ b/crates/pf-vdisplay/src/vdisplay/linux/mutter.rs @@ -122,8 +122,14 @@ impl MutterDisplay { /// `XDG_SESSION_DESKTOP` alongside would resurrect the bug that scrub exists to prevent — a stale /// `gnome` there after a gnome-shell crash reports Mutter usable and routes the next client into a /// dead session (45 s create timeouts instead of a crisp handshake error). +/// +/// The read takes [`crate::with_env_lock`]: this runs on a management worker (`/host/compositors` → +/// [`crate::available`]) concurrently with another connect's `apply_session_env`, which `set_var`s +/// this key for a live session and `remove_var`s it when nothing is — and a glibc `getenv` racing +/// that is the `environ` realloc data race ENV_LOCK exists for, torn answer at best and a host +/// segfault mid-connect at worst. Read-then-drop; no caller holds the lock (it is not reentrant). pub fn is_available() -> bool { - std::env::var("XDG_CURRENT_DESKTOP") + crate::with_env_lock(|| std::env::var("XDG_CURRENT_DESKTOP")) .map(|d| d.to_ascii_uppercase().contains("GNOME")) .unwrap_or(false) } @@ -718,13 +724,24 @@ async fn connect( } // --------------------------------------------------------------------------------------------- -// Optional: make the per-session virtual output the PRIMARY monitor (PUNKTFUNK_MUTTER_VIRTUAL_PRIMARY). +// Optional: make the per-session virtual output the PRIMARY monitor. // // `RecordVirtual` adds the virtual monitor as an *extended* desktop. On a headless host that's the // only display, so the shell + windows live there. But when a physical monitor is attached, GNOME // keeps it primary and the virtual output is an empty extension — the stream shows only the -// wallpaper. We fix that by promoting the virtual output to primary (physical kept on, secondary) -// via `org.gnome.Mutter.DisplayConfig.ApplyMonitorsConfig`, and restore on teardown. +// wallpaper. We fix that by promoting the virtual output via +// `org.gnome.Mutter.DisplayConfig.ApplyMonitorsConfig`. +// +// Which shape is `crate::effective_topology()`'s call, not this module's: the console policy first, +// then the legacy `PUNKTFUNK_{KWIN,MUTTER}_VIRTUAL_PRIMARY` env, then the Auto default. `Primary` +// keeps the physicals on as secondaries; `Exclusive` omits them, so Mutter disables them for the +// session; `Extend` skips this block entirely. +// +// Applied at APPLY_TEMPORARY, and **MUTTER ITSELF REVERTS IT** when the virtual monitor disappears +// and our DisplayConfig connection closes. We must never re-assert the layout on teardown: the +// banner used to promise a "restore on teardown" that the teardown deliberately does not do, and +// issuing that ApplyMonitorsConfig is what SIGSEGVed gnome-shell on Mutter 50 + NVIDIA and wedged a +// box at the GDM greeter (see the teardown comment in `session_thread`). // --------------------------------------------------------------------------------------------- /// `org.gnome.Mutter.DisplayConfig.GetCurrentState` reply shapes (see the interface XML): @@ -811,7 +828,9 @@ fn current_mode(state: &CurrentState, connector: &str) -> Option<(String, i32, i /// Pure mode-pick for a KEPT physical (unit-tested). Given the physical's PRE-connect mode /// (`pre_mode = (id, w, h, refresh)`; `None` when the connector is new since the snapshot) and the /// mode list Mutter reports for it in the POST-virtual state -/// (`(id, w, h, refresh, is_current, is_preferred)`), return the `(mode_id, width)` to re-apply. +/// (`(id, w, h, refresh, is_current, is_preferred)`), return the `(mode_id, width, height)` to +/// re-apply. The height is not decoration: a head rotated 90°/270° is as wide on the desktop as its +/// mode is tall, and the caller lays the kept heads out side by side. /// /// Mutter re-derives its layout when the `RecordVirtual` output appears and can silently drop a /// 120 Hz panel to its EDID-preferred 60 Hz — so the post-virtual `is-current` is *already* 60 Hz. @@ -821,40 +840,40 @@ fn current_mode(state: &CurrentState, connector: &str) -> Option<(String, i32, i fn pick_keep_mode( pre_mode: Option<(String, i32, i32, f64)>, state_modes: &[(String, i32, i32, f64, bool, bool)], -) -> Option<(String, i32)> { +) -> Option<(String, i32, i32)> { let state_current = || { state_modes .iter() .find(|m| m.4) .or_else(|| state_modes.iter().find(|m| m.5)) .or_else(|| state_modes.first()) - .map(|m| (m.0.clone(), m.1)) + .map(|m| (m.0.clone(), m.1, m.2)) }; let Some((pre_id, w, h, hz)) = pre_mode else { return state_current(); }; // The exact pre mode id, if the connector still offers it (same session ⇒ usually true). if state_modes.iter().any(|m| m.0 == pre_id) { - return Some((pre_id, w)); + return Some((pre_id, w, h)); } // Else a re-keyed id with the same geometry + refresh (still the real 120 Hz). if let Some(m) = state_modes .iter() .find(|m| m.1 == w && m.2 == h && (m.3 - hz).abs() < 0.5) { - return Some((m.0.clone(), m.1)); + return Some((m.0.clone(), m.1, m.2)); } // The physical genuinely no longer offers that mode — use whatever is valid now. state_current() } -/// The `(mode_id, width)` a kept physical should be RE-APPLIED at — its PRE-connect mode preserved -/// across Mutter's virtual-output layout re-derive. See [`pick_keep_mode`]. +/// The `(mode_id, width, height)` a kept physical should be RE-APPLIED at — its PRE-connect mode +/// preserved across Mutter's virtual-output layout re-derive. See [`pick_keep_mode`]. fn physical_keep_mode( pre: &CurrentState, state: &CurrentState, conn: &str, -) -> Option<(String, i32)> { +) -> Option<(String, i32, i32)> { let pre_mode = current_mode_full(pre, conn); let state_modes: Vec<(String, i32, i32, f64, bool, bool)> = state .1 @@ -1044,13 +1063,57 @@ fn snap_integral_scale(want: f64, width: u32, height: u32) -> f64 { .unwrap_or(want) } -/// The scale of the logical monitor carrying `connector`, if present. -fn logical_scale(state: &CurrentState, connector: &str) -> Option { +/// The `(scale, transform)` of the logical monitor carrying `connector`. `None` means **no logical +/// monitor carries it** — which is how Mutter reports a head the operator has DISABLED, and is the +/// distinction [`keep_head_layout`] turns into "leave it off". +fn logical_placement(state: &CurrentState, connector: &str) -> Option<(f64, u32)> { state .2 .iter() .find(|l| l.5.iter().any(|spec| spec.0 == connector)) - .map(|l| l.2) + .map(|l| (l.2, l.3)) +} + +/// The scale of the logical monitor carrying `connector`, if present. +fn logical_scale(state: &CurrentState, connector: &str) -> Option { + logical_placement(state, connector).map(|(scale, _)| scale) +} + +/// Whether a kept physical should be re-applied at all, and with what `(scale, transform)`. Pure — +/// unit-tested, because getting it wrong is invisible on a headless lab box and very visible on the +/// operator's desk. +/// +/// The rebuild used to hardcode `scale = 1.0`, `transform = 0` and to list every connector Mutter +/// reported, so one connect un-rotated a portrait panel, dropped a 2×-scaled 4K head to native +/// pixels, and switched a deliberately-dark monitor back on. All three facts are in the PRE-connect +/// snapshot: `pre_logical` is the head's logical-monitor entry there, and Mutter reports a disabled +/// head by omitting it from `logical_monitors` entirely. So: carry the pre values when the head was +/// on; leave it out when the connector existed pre-connect and carried no logical monitor (disabled +/// on purpose); and for a connector that was not in the snapshot at all — a hotplug inside our +/// window — keep it on at whatever Mutter has just derived for it, which is the friendlier reading +/// of "the operator plugged this in while we were connecting". +fn keep_head_layout( + existed_pre: bool, + pre_logical: Option<(f64, u32)>, + state_logical: Option<(f64, u32)>, +) -> Option<(f64, u32)> { + // A non-finite or non-positive scale would fail the whole ApplyMonitorsConfig, taking the + // primary switch down with it. + let sane = |(scale, transform): (f64, u32)| { + ( + if scale.is_finite() && scale > 0.0 { + scale + } else { + 1.0 + }, + transform, + ) + }; + match (pre_logical, existed_pre) { + (Some(l), _) => Some(sane(l)), + (None, true) => None, + (None, false) => Some(sane(state_logical.unwrap_or((1.0, 0)))), + } } /// Every head Mutter reports, for [`crate::monitors::list`]. @@ -1142,16 +1205,20 @@ fn build_exclusive_config(vconn: &str, vmode: &str, scale: f64) -> Vec, key: &str, value: &str) - /// Read `path`, set `key` in `block`, write it back — and back the original up ONCE, the first time /// we touch a file we did not write. Returns `true` when the file changed (the caller restarts the /// portal only then). +/// +/// The read is matched EXPLICITLY, and only [`ErrorKind::NotFound`](std::io::ErrorKind::NotFound) +/// may mean "empty". This used to be `read_to_string(path).unwrap_or_default()`, which folded every +/// read failure into an empty string — and an empty string is the one input for which this function +/// destroys data: `upsert("")` yields a file holding ONLY our block, the backup below is skipped +/// because there is nothing to back up, and the write replaces the user's config. One non-UTF-8 byte +/// in a comment (a Latin-1 character, an 8-bit paste) or a transient EIO on an NFS/overlay config +/// dir was enough, and the result was exactly the silent, permanent loss this module exists to +/// prevent. A config we cannot read is a config we refuse to rewrite. pub(crate) fn ensure_key(path: &Path, block: Block<'_>, key: &str, value: &str) -> Result { - let existing = std::fs::read_to_string(path).unwrap_or_default(); + // Read BYTES: whether a backup is owed is a question about what is on disk, not about what + // decoded — and the decode failure below is itself one of the cases that must not be silent. + let raw = match std::fs::read(path) { + Ok(b) => Some(b), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => None, + Err(e) => { + return Err(e).with_context(|| { + format!( + "read {} (refusing to rewrite a portal config we could not read)", + path.display() + ) + }) + } + }; + let existing = match &raw { + Some(bytes) => std::str::from_utf8(bytes) + .with_context(|| { + format!( + "{} is not UTF-8 — refusing to rewrite it (the one key we own is not worth \ + losing the rest of the file for; fix or move the file and reconnect)", + path.display() + ) + })? + .to_string(), + None => String::new(), + }; let updated = upsert(&existing, block, key, value); if updated == existing { return Ok(false); @@ -108,9 +142,9 @@ pub(crate) fn ensure_key(path: &Path, block: Block<'_>, key: &str, value: &str) if let Some(dir) = path.parent() { std::fs::create_dir_all(dir).with_context(|| format!("mkdir {}", dir.display()))?; } - // One-time backup. `create_new` makes this genuinely once: a later edit must not overwrite the - // user's ORIGINAL with our own previous output. - if !existing.is_empty() { + // One-time backup, of the bytes we actually read. `create_new` makes this genuinely once: a + // later edit must not overwrite the user's ORIGINAL with our own previous output. + if let Some(bytes) = raw.as_deref().filter(|b| !b.is_empty()) { let backup = path.with_extension("punktfunk-backup"); match std::fs::OpenOptions::new() .write(true) @@ -119,7 +153,7 @@ pub(crate) fn ensure_key(path: &Path, block: Block<'_>, key: &str, value: &str) { Ok(mut f) => { use std::io::Write; - let _ = f.write_all(existing.as_bytes()); + let _ = f.write_all(bytes); tracing::info!( backup = %backup.display(), "backed up the existing portal config before editing it" @@ -133,10 +167,49 @@ pub(crate) fn ensure_key(path: &Path, block: Block<'_>, key: &str, value: &str) ), } } - std::fs::write(path, &updated).with_context(|| format!("write {}", path.display()))?; + write_atomic(path, updated.as_bytes())?; Ok(true) } +/// Replace `path`'s contents with `bytes` **atomically**: fill a temp file beside it, then rename +/// over it. `fs::write` truncates first and fills afterwards, so a crash, a full disk or a killed +/// host between the two leaves the user's config truncated — the same loss this module exists to +/// prevent, arrived at from the other side. The temp file goes in the SAME directory because a +/// rename is only atomic within one filesystem, and it inherits the original's permission bits so +/// an operator's 0600 config does not come back at the umask default. +fn write_atomic(path: &Path, bytes: &[u8]) -> Result<()> { + use std::io::Write; + let dir = path.parent().unwrap_or_else(|| Path::new(".")); + let stem = path + .file_name() + .map(|n| n.to_string_lossy().into_owned()) + .unwrap_or_else(|| "config".to_string()); + // Per-process name: two hosts editing the same config must not fill one another's temp file. + let tmp = dir.join(format!(".{stem}.punktfunk-{}.tmp", std::process::id())); + let write = || -> Result<()> { + { + let mut f = + std::fs::File::create(&tmp).with_context(|| format!("create {}", tmp.display()))?; + f.write_all(bytes) + .with_context(|| format!("write {}", tmp.display()))?; + // The rename must not publish a name whose contents are still in the page cache only. + f.sync_all() + .with_context(|| format!("sync {}", tmp.display()))?; + } // closed before the rename — Windows is far happier renaming a file nobody holds open. + if let Ok(md) = std::fs::metadata(path) { + let _ = std::fs::set_permissions(&tmp, md.permissions()); + } + std::fs::rename(&tmp, path) + .with_context(|| format!("rename {} -> {}", tmp.display(), path.display())) + }; + let r = write(); + if r.is_err() { + // Never leave a half-written dotfile beside the user's config. + let _ = std::fs::remove_file(&tmp); + } + r +} + #[cfg(test)] mod tests { use super::*; @@ -237,3 +310,166 @@ mod tests { ); } } + +/// [`ensure_key`] itself — the half that touches the user's disk. +/// +/// The merge above was pinned by seven cases while the I/O wrapper around it, which is where the +/// destructive behaviour lives (the read, the once-only backup, the replacing write), had none. That +/// is backwards: `upsert` can at worst return a wrong string, `ensure_key` can delete a config. +/// Filesystem-only — no compositor, no portal — so these run on every platform, like the merge tests. +#[cfg(test)] +mod io_tests { + use super::*; + + /// A scratch directory removed on drop. `tempfile` is deliberately not a dependency of this + /// crate; the temp-dir + pid + counter convention is the one `proc.rs`'s fixtures already use. + struct Scratch(std::path::PathBuf); + + impl Scratch { + fn new(tag: &str) -> Self { + static N: std::sync::atomic::AtomicU32 = std::sync::atomic::AtomicU32::new(0); + let n = N.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let dir = std::env::temp_dir() + .join(format!("pf-vd-portalcfg-{tag}-{}-{n}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("scratch dir"); + Self(dir) + } + fn path(&self, name: &str) -> std::path::PathBuf { + self.0.join(name) + } + } + + impl Drop for Scratch { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } + } + + fn backup_of(p: &Path) -> std::path::PathBuf { + p.with_extension("punktfunk-backup") + } + + /// The data-loss case. A config that cannot be decoded must be left EXACTLY as it is: the old + /// `unwrap_or_default()` turned it into an empty string, wrote a file holding only our block, + /// skipped the backup (nothing to back up, as far as it could tell) and returned `Ok(true)`. + #[test] + fn a_non_utf8_config_is_refused_not_replaced() { + let s = Scratch::new("nonutf8"); + let p = s.path("config"); + // A Latin-1 'ÿ' in a comment — the whole file is otherwise perfectly ordinary. + let raw: &[u8] = b"[screencast]\n# r\xffgler\nchooser_type=simple\noutput_name=DP-1\n"; + std::fs::write(&p, raw).expect("seed"); + let err = ensure_key(&p, Block::Ini("screencast"), "chooser_cmd", "cat x") + .expect_err("an unreadable config must not be rewritten"); + assert!( + format!("{err:#}").contains("not UTF-8"), + "the error must name the real cause: {err:#}" + ); + assert_eq!( + std::fs::read(&p).expect("still there"), + raw, + "byte-identical" + ); + assert!( + !backup_of(&p).exists(), + "nothing was edited, so nothing is owed a backup" + ); + } + + /// The ordinary first-connect path: no file yet, so one is created — and there is no original + /// to preserve, so no backup is left lying beside it. + #[test] + fn a_missing_file_is_created_without_a_backup() { + let s = Scratch::new("missing"); + let p = s.path("nested").join("config"); + assert!(ensure_key(&p, Block::Ini("screencast"), "chooser_cmd", "cat x").expect("write")); + assert_eq!( + std::fs::read_to_string(&p).expect("created"), + "[screencast]\nchooser_cmd=cat x\n" + ); + assert!(!backup_of(&p).exists()); + } + + /// `create_new` is what makes the backup once-only, and this is the invariant it buys: after a + /// second edit (a new `$XDG_RUNTIME_DIR`, so a new value) the backup must still hold the user's + /// PRISTINE file — not our own previous output. + #[test] + fn the_backup_holds_the_original_across_two_edits() { + let s = Scratch::new("backup"); + let p = s.path("config"); + let pristine = "[screencast]\nchooser_type=simple\noutput_name=DP-1\n"; + std::fs::write(&p, pristine).expect("seed"); + assert!( + ensure_key(&p, Block::Ini("screencast"), "chooser_cmd", "cat /run/a").expect("1st") + ); + assert!( + ensure_key(&p, Block::Ini("screencast"), "chooser_cmd", "cat /run/b").expect("2nd") + ); + assert_eq!( + std::fs::read_to_string(backup_of(&p)).expect("backup"), + pristine + ); + let now = std::fs::read_to_string(&p).expect("edited"); + assert!( + now.contains("chooser_cmd=cat /run/b"), + "the second value won" + ); + assert!( + now.contains("output_name=DP-1"), + "the user's other keys survived" + ); + } + + /// Idempotence at the I/O level: an already-correct file is not rewritten and reports `false`, + /// because the caller RESTARTS the portal on `true` — a spurious `true` restarts xdpw/xdph on + /// every connect. + #[test] + fn an_unchanged_file_returns_false_and_does_not_rewrite() { + let s = Scratch::new("unchanged"); + let p = s.path("config"); + assert!(ensure_key(&p, Block::Ini("screencast"), "chooser_cmd", "cat x").expect("1st")); + let after_first = std::fs::read_to_string(&p).expect("written"); + let mtime = std::fs::metadata(&p) + .and_then(|m| m.modified()) + .expect("mtime"); + assert!( + !ensure_key(&p, Block::Ini("screencast"), "chooser_cmd", "cat x").expect("2nd"), + "an unchanged config must report no change" + ); + assert_eq!( + std::fs::read_to_string(&p).expect("still there"), + after_first + ); + assert_eq!( + std::fs::metadata(&p) + .and_then(|m| m.modified()) + .expect("mtime"), + mtime, + "the file must not have been touched at all" + ); + } + + /// The write publishes the WHOLE new file or nothing (temp + rename), and it leaves no debris + /// beside the config — a stray dotfile in `~/.config/hypr` is the kind of thing that outlives + /// several releases. + #[test] + fn the_write_is_atomic_and_leaves_no_temp_behind() { + let s = Scratch::new("atomic"); + let p = s.path("config"); + std::fs::write(&p, "[other]\nkeep=me\n").expect("seed"); + assert!(ensure_key(&p, Block::Ini("screencast"), "chooser_cmd", "cat x").expect("write")); + let names: Vec = std::fs::read_dir(&s.0) + .expect("dir") + .flatten() + .map(|e| e.file_name().to_string_lossy().into_owned()) + .collect(); + assert!( + !names.iter().any(|n| n.ends_with(".tmp")), + "temp file left behind: {names:?}" + ); + assert!(std::fs::read_to_string(&p) + .expect("edited") + .contains("keep=me")); + } +} diff --git a/crates/pf-vdisplay/src/vdisplay/linux/wlroots.rs b/crates/pf-vdisplay/src/vdisplay/linux/wlroots.rs index 9d1a868d..7f37d307 100644 --- a/crates/pf-vdisplay/src/vdisplay/linux/wlroots.rs +++ b/crates/pf-vdisplay/src/vdisplay/linux/wlroots.rs @@ -40,7 +40,11 @@ fn chooser_file() -> String { } /// The chooser command xdpw runs via `/bin/sh -c`, reading stdout. The `|| echo` fallback keeps -/// plain portal capture (`--source portal`) working when no session has written the chooser file. +/// plain portal capture (`--source portal`) working when no session of ours is mid-handshake — it +/// is a GUESS at sway's own first headless output, right on a box whose sway loads the headless +/// backend with one output of its own and wrong (a cast of nothing) otherwise. It is reachable +/// again: the per-session file is removed with the handshake it steers ([`ChooserFile`]), so it no +/// longer sits there naming an output we have since unplugged. fn chooser_cmd() -> String { format!( "cat {} 2>/dev/null || echo 'Monitor: HEADLESS-1'", @@ -68,8 +72,14 @@ impl WlrootsDisplay { /// wlroots/Sway is usable when the host runs inside a Sway session — signalled by `SWAYSOCK` /// (the IPC socket `swaymsg create_output` needs). Cheap env check for the enumeration path. +/// +/// Under [`crate::with_env_lock`]: this runs on a management worker (`/host/compositors` → +/// [`crate::available`]) concurrently with another connect's `apply_session_env`, which `set_var`s +/// — and, when no sway session is live, `remove_var`s — this very key. A glibc `getenv` racing a +/// `setenv` is the `environ` realloc data race ENV_LOCK exists for, and it is UB whichever key each +/// side names. No caller holds the lock (the mutex is not reentrant). pub fn is_available() -> bool { - std::env::var_os("SWAYSOCK").is_some() + crate::with_env_lock(|| std::env::var_os("SWAYSOCK")).is_some() } impl VirtualDisplay for WlrootsDisplay { @@ -86,13 +96,33 @@ impl VirtualDisplay for WlrootsDisplay { } fn create(&mut self, mode: Mode) -> Result { - let before = output_names() - .context("swaymsg get_outputs (is the host inside the sway session env — SWAYSOCK?)")?; - swaymsg(&["create_output"]) - .context("swaymsg create_output (sway needs the headless backend loaded)")?; - // The output appears synchronously in practice; poll briefly to be safe, and own it - // from here on so error unwinding unplugs it. - let output = OutputGuard(wait_new_output(&before, Duration::from_secs(5))?); + warn_topology_is_extend_only(); + // Snapshot → create → identify, all under CREATE_LOCK. sway names the headless output + // itself (`HEADLESS-N`), so the only way to know which one is ours is "the name that was not + // there before" — and two concurrent creates each picking the other's output is a silent + // mis-capture, not a failure (mutter's TOPOLOGY_LOCK exists for exactly this class). The + // lock also gives the failure path somewhere safe to unplug from: the output already exists + // by the time `wait_new_output` can fail, and nothing else may have created one meanwhile. + let output = { + let _create = CREATE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let before = output_names().context( + "swaymsg get_outputs (is the host inside the sway session env — SWAYSOCK?)", + )?; + swaymsg(&["create_output"]) + .context("swaymsg create_output (sway needs the headless backend loaded)")?; + // The output appears synchronously in practice; poll briefly to be safe, and own it + // from here on so error unwinding unplugs it. + match wait_new_output(&before, Duration::from_secs(5)) { + Ok(name) => OutputGuard(name), + Err(e) => { + // `create_output` reported success, so an output very probably exists — it just + // never showed up in time (or showed up a moment after we gave up). Unowned, it + // would sit in the operator's sway layout forever. + unplug_strays(&before); + return Err(e); + } + } + }; let name = output.0.clone(); // The client's exact mode (also the refresh clock that makes the output produce frames). @@ -128,7 +158,7 @@ impl VirtualDisplay for WlrootsDisplay { remote_fd: Some(fd), preferred_mode: Some((mode.width, mode.height, mode.refresh_hz)), keepalive: Box::new(Keepalive { - _stop: StopGuard(stop), + _stop: stop, _output: output, }), // Owned (the compositor output is ours to tear down), but not registry-poolable: the @@ -159,6 +189,52 @@ impl Drop for StopGuard { } } +/// Serializes **snapshot → `create_output` → identify-the-new-name**, process-wide. sway names its +/// headless outputs itself, so ownership is established by a before/after diff and two concurrent +/// creates would each adopt the other's output — which does not fail, it silently streams the wrong +/// one. Mutter's `TOPOLOGY_LOCK` is the same guard for the same reason; Hyprland needs none because +/// it lets us NAME the output (D6). +static CREATE_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + +/// Unplug any headless output that appeared since `before` and that nothing owns — the cleanup for a +/// `create_output` whose output we could not identify in time. Only `HEADLESS-*` is touched: a +/// physical hotplug in the same window is the operator's, not ours, and `unplug` on a real connector +/// would take their screen away. Best-effort by construction, and it runs with [`CREATE_LOCK`] held +/// so nothing else in this process can have created the strays it sees. +fn unplug_strays(before: &[String]) { + let Ok(now) = output_names() else { return }; + for name in now + .into_iter() + .filter(|n| n.starts_with("HEADLESS-") && !before.iter().any(|b| b == n)) + { + match swaymsg(&["output", &name, "unplug"]) { + Ok(_) => tracing::warn!(output = %name, "unplugged a headless output we created but \ + could not identify in time"), + Err(e) => tracing::warn!(output = %name, error = %format!("{e:#}"), "could not unplug \ + the headless output left behind by a failed create"), + } + } +} + +/// The configured [`crate::policy::Topology`] is not implemented on this backend — say so once per +/// create instead of leaving the management API's echo as the only signal that the pin was dropped +/// (sweep 13.18). sway's virtual output is always an EXTENSION: nothing here promotes it to primary +/// or disables the operator's heads. +fn warn_topology_is_extend_only() { + let topology = crate::effective_topology(); + if !matches!( + topology, + crate::policy::Topology::Extend | crate::policy::Topology::Auto + ) { + tracing::warn!( + ?topology, + "wlroots: this backend implements EXTEND only — the headless output is added beside the \ + operator's heads and nothing is promoted or disabled. Configure `topology: extend` to \ + stop the console promising otherwise." + ); + } +} + /// Owns the created headless output; dropping it unplugs it from sway. struct OutputGuard(String); @@ -171,15 +247,26 @@ impl Drop for OutputGuard { } } +/// Budget for one `swaymsg` call ([`crate::proc`]). +/// +/// swaymsg is a CLIENT of the compositor it drives: against a wedged sway it blocks in its own +/// connect to the IPC socket and never returns — and these calls run on the session's stream thread, +/// whose only way to end a session is to return, so one hung query used to wedge the session +/// permanently. Generous next to a healthy call (single-digit milliseconds), and every call site +/// here already has a failed-query path, so a timeout lands on behaviour that already exists. +const SWAYMSG_BUDGET: Duration = Duration::from_secs(5); + +/// Budget for the one-shot xdpw restart. `systemctl --user try-restart` waits for the unit's job to +/// settle, so it is the slowest helper on this path — and its result is already ignored. +const PORTAL_RESTART_BUDGET: Duration = Duration::from_secs(10); + /// Run `swaymsg -- `, returning stdout (`--` so command tokens like `--custom` reach /// sway instead of swaymsg's own getopt). swaymsg exits non-zero (with the error on stderr/ /// stdout) when the command fails, so checking the status covers `{"success": false}` too. fn swaymsg(args: &[&str]) -> Result { - let out = Command::new("swaymsg") - .arg("--") - .args(args) - .output() - .context("run swaymsg (is sway installed?)")?; + let out = + crate::proc::output_within(Command::new("swaymsg").arg("--").args(args), SWAYMSG_BUDGET) + .context("run swaymsg (is sway installed?)")?; if !out.status.success() { bail!( "swaymsg {:?} failed: {}{}", @@ -197,10 +284,11 @@ fn swaymsg(args: &[&str]) -> Result { /// *command*, which is right for `create_output` and wrong for a query — `-t` after `--` comes back /// as `Unknown/invalid command '-t'` (caught on-glass writing the monitor enumeration). fn swaymsg_query(kind: &str) -> Result { - let out = Command::new("swaymsg") - .args(["-t", kind, "--raw"]) - .output() - .context("run swaymsg (is sway installed?)")?; + let out = crate::proc::output_within( + Command::new("swaymsg").args(["-t", kind, "--raw"]), + SWAYMSG_BUDGET, + ) + .context("run swaymsg (is sway installed?)")?; if !out.status.success() { bail!( "swaymsg -t {kind} failed: {}", @@ -230,13 +318,37 @@ fn output_names() -> Result> { /// handshake, not just the write, because the read happens inside it. static SELECTION_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); +/// The per-session chooser file, removed when the handshake it steers is over. +/// +/// Its lifetime is the HANDSHAKE, not the session: xdpw reads it once, inside +/// [`select_and_cast`]'s critical section, and everything after that is the cast's own business. +/// Left behind (as it was) the stale `Monitor: HEADLESS-3` outlives the output `Drop` has since +/// unplugged, and it permanently shadows [`chooser_cmd`]'s `|| echo` fallback — so a later +/// `--source portal` capture with no session of ours running steers at a connector that is gone. +/// Tying removal to the CAST instead would be worse still: the file is one per user, so a session +/// ending hours later would delete a *sibling's* selection out from under its picker. +struct ChooserFile(String); + +impl Drop for ChooserFile { + fn drop(&mut self) { + if let Err(e) = std::fs::remove_file(&self.0) { + if e.kind() != std::io::ErrorKind::NotFound { + tracing::debug!(path = %self.0, error = %e, "could not remove the xdpw chooser file"); + } + } + } +} + /// Point xdpw's chooser at `output` and run the ScreenCast handshake, returning the portal fd + /// node id and the guard that stops the cast. The caller must hold [`SELECTION_LOCK`]. -fn select_and_cast(output: &str, hw_cursor: bool) -> Result<(OwnedFd, u32, Arc)> { +fn select_and_cast(output: &str, hw_cursor: bool) -> Result<(OwnedFd, u32, StopGuard)> { ensure_xdpw_config()?; let chooser = chooser_file(); std::fs::write(&chooser, format!("Monitor: {output}\n")) .with_context(|| format!("write {chooser}"))?; + // Owned from the write on: every arm below (and every `?`) leaves the handshake, which is the + // only thing that reads it. + let _chooser = ChooserFile(chooser); let (setup_tx, setup_rx) = std::sync::mpsc::channel::>(); let stop = Arc::new(AtomicBool::new(false)); let stop_thread = stop.clone(); @@ -244,8 +356,16 @@ fn select_and_cast(output: &str, hw_cursor: bool) -> Result<(OwnedFd, u32, Arc Ok((fd, node_id, stop)), + Ok(Ok((fd, node_id))) => Ok((fd, node_id, guard)), Ok(Err(e)) => bail!("ScreenCast portal on {output} failed: {e}"), Err(_) => bail!("timed out waiting for the ScreenCast portal on {output}"), } @@ -266,7 +386,7 @@ pub(crate) fn stream_existing_output( Ok(crate::mirror::MirrorStream { node_id, remote_fd: Some(fd), - keepalive: Box::new(StopGuard(stop)), + keepalive: Box::new(stop), }) } @@ -374,9 +494,13 @@ fn ensure_xdpw_config() -> Result<()> { return Ok(()); } tracing::info!(path = %path.display(), "pointed xdg-desktop-portal-wlr at the managed output chooser"); - let _ = Command::new("systemctl") - .args(["--user", "try-restart", "xdg-desktop-portal-wlr.service"]) - .status(); + // Bounded: `systemctl --user` blocks on the user manager's job queue, and this runs on the + // session's stream thread. Its result was already ignored — a timeout just means the portal + // picks the new config up whenever it next starts. + let _ = crate::proc::status_within( + Command::new("systemctl").args(["--user", "try-restart", "xdg-desktop-portal-wlr.service"]), + PORTAL_RESTART_BUDGET, + ); Ok(()) }