From 33a31427ae998e6a35dc8134caa1ce410fb65c0e Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 28 Jul 2026 19:11:28 +0200 Subject: [PATCH] fix(vdisplay): locks held across seconds-long work, an AB/BA inversion, and a panic that poisons the manager MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 7's contained half. 7.1's writer split and 7.3's reservation are NOT here — see the plan; both need more than a local edit. **Lock-order inversion, gamescope.** `create_managed_session_steamos` held MANAGED_SESSION while taking STEAMOS_TOOK_OVER (and `persist_takeover`'s locks), while `do_restore_tv_session` takes STEAMOS_TOOK_OVER first and MANAGED_SESSION second — AB/BA between a connect and the restore worker, which genuinely run concurrently (`vd.create` runs off the registry lock; the worker is its own thread). The connect path now drops its guard before the pair and re-acquires after `poll_managed_node`, the shape `create_managed_session` already used. And `takeover_live` held all four statics at once — in a `||` chain every `.lock()` temporary lives to the end of the statement — for four reads; scoped bindings drop each guard before the next, taking it out of the ordering graph entirely. **`observe_session_instance` decided and acted under one lock.** It held LAST_INSTANCE across `invalidate_backend` (which drops keep-alive displays through the registry lock) and a `systemctl` shell-out on a 10 s budget, from three call sites including a per-second watcher. It decides under the lock and acts outside now; the baseline is advanced inside it, so a concurrent observer cannot run the action twice. **`admit` held the live-session table across the budget checks** — `manager::snapshot()` blocks on the manager `state` lock, which is itself held across DDC round trips and three 3 s activation ladders. Every connect, disconnect and mgmt read queued behind one slow display operation. The guard is scoped to `decide` alone. **GameStream never registered**, so its display was invisible to both Windows budgets (they gate on `!live.is_empty()`) and a native connect could be admitted past capacity the box had already spent. It registers now, anonymously, for the session's lifetime. **`ensure_exclusive_watch` panicked on a failed thread spawn** while holding BOTH `exclusive_watch` and (via its caller) `state` — poisoning the two locks the whole manager runs on, so every later acquire and mgmt read would panic on `.lock().unwrap()`. It logs and degrades instead: no re-assert watchdog is a degradation, a wedged manager is not. Also fixes two things only a Windows build shows, both mine: Phase 2.3 left `gamescope_route` unused there (every reader is Linux-gated), which `-D warnings` in Windows CI would have rejected — I had only run the host clippy on Linux. And Phase 5.4's helper-safening reaches a FOURTH crate, pf-inject, which xcheck does not lint. Verified on .173: `cargo clippy -p punktfunk-host -p pf-vdisplay --all-targets` clean across the whole tree; local fmt, both xcheck targets and 53 tests green. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/inject/windows/stream_target.rs | 4 +-- crates/pf-vdisplay/src/vdisplay/admission.rs | 17 +++++++++-- .../src/vdisplay/linux/gamescope.rs | 30 ++++++++++++++----- crates/pf-vdisplay/src/vdisplay/session.rs | 15 ++++++++-- .../src/vdisplay/windows/manager.rs | 18 +++++++++-- .../punktfunk-host/src/gamestream/stream.rs | 15 ++++++++++ crates/punktfunk-host/src/native/stream.rs | 4 +++ 7 files changed, 84 insertions(+), 19 deletions(-) diff --git a/crates/pf-inject/src/inject/windows/stream_target.rs b/crates/pf-inject/src/inject/windows/stream_target.rs index 4a119ad2..65c4cf12 100644 --- a/crates/pf-inject/src/inject/windows/stream_target.rs +++ b/crates/pf-inject/src/inject/windows/stream_target.rs @@ -66,9 +66,7 @@ fn stream_rect() -> Option { let fresh = st.queried.is_some_and(|at| at.elapsed() < RECT_TTL); if !fresh { st.queried = Some(Instant::now()); - // SAFETY: read-only QueryDisplayConfig over owned locals (`source_desktop_rect`'s - // contract — the same call the cursor-readback poller makes at spawn). - match unsafe { pf_win_display::win_display::source_desktop_rect(target_id) } { + match pf_win_display::win_display::source_desktop_rect(target_id) { Some(r) => { if st.rect != Some(r) { tracing::info!(target_id, rect = ?r, "stream-target desktop rect resolved"); diff --git a/crates/pf-vdisplay/src/vdisplay/admission.rs b/crates/pf-vdisplay/src/vdisplay/admission.rs index 66c6e389..e8635ff9 100644 --- a/crates/pf-vdisplay/src/vdisplay/admission.rs +++ b/crates/pf-vdisplay/src/vdisplay/admission.rs @@ -124,10 +124,21 @@ pub fn effective_conflict() -> ModeConflict { /// (`design/windows-parallel-virtual-displays.md` §2.5): a display we can't afford is DECLINED here, /// never admitted-then-degrading a live sibling. pub fn admit(req_identity: Option<[u8; 32]>) -> Admission { - let live = table().lock().unwrap(); - let decision = decide(effective_conflict(), req_identity, &live); + // The table guard is scoped to `decide` ALONE. The budget checks below call + // `manager::snapshot()` — which blocks on the manager `state` lock, held across DDC round trips, + // SetupAPI devnode work and three 3 s activation ladders — and `pf_encode`'s NVENC probe. Holding + // the process-wide live-session table across those stalled every other connect, disconnect and + // mgmt read behind one slow display operation. + let (decision, any_live) = { + let live = table().lock().unwrap(); + ( + decide(effective_conflict(), req_identity, &live), + !live.is_empty(), + ) + }; + let _ = any_live; // read only by the Windows budget block below #[cfg(windows)] - if matches!(decision, Admission::Separate) && !live.is_empty() { + if matches!(decision, Admission::Separate) && any_live { let max = policy::prefs().get().effective().max_displays; let slots = super::manager::snapshot().len() as u32; if slots >= max { diff --git a/crates/pf-vdisplay/src/vdisplay/linux/gamescope.rs b/crates/pf-vdisplay/src/vdisplay/linux/gamescope.rs index ed983734..6a5f510a 100644 --- a/crates/pf-vdisplay/src/vdisplay/linux/gamescope.rs +++ b/crates/pf-vdisplay/src/vdisplay/linux/gamescope.rs @@ -963,6 +963,13 @@ fn create_managed_session_steamos(mode: Mode, hdr: bool) -> Result Result bool { - !STOPPED_AUTOLOGIN + // ONE lock at a time. In a `||` chain every `.lock()` temporary lives to the end of the + // statement, so this used to hold all four simultaneously — putting it in the lock-order graph + // for no reason, since each is only read. Scoped bindings drop each guard before the next. + let autologin = !STOPPED_AUTOLOGIN .lock() .unwrap_or_else(|e| e.into_inner()) - .is_empty() - || *STEAMOS_TOOK_OVER.lock().unwrap_or_else(|e| e.into_inner()) - || STOPPED_DM - .lock() - .unwrap_or_else(|e| e.into_inner()) - .is_some() + .is_empty(); + let steamos = *STEAMOS_TOOK_OVER.lock().unwrap_or_else(|e| e.into_inner()); + let dm = STOPPED_DM + .lock() + .unwrap_or_else(|e| e.into_inner()) + .is_some(); + autologin + || steamos + || dm // A managed session that took nothing over (started beside a live desktop — e.g. a client // gamescope pin on a KDE box) still owns the transient SESSION_UNIT: without this arm it // was ORPHANED forever after disconnect ("closing the app does not end the session", diff --git a/crates/pf-vdisplay/src/vdisplay/session.rs b/crates/pf-vdisplay/src/vdisplay/session.rs index f655a3f0..0c25b574 100644 --- a/crates/pf-vdisplay/src/vdisplay/session.rs +++ b/crates/pf-vdisplay/src/vdisplay/session.rs @@ -50,8 +50,18 @@ static LAST_INSTANCE: std::sync::Mutex)>> = /// session (the per-connect resolve, the mid-stream watcher, the capture-loss re-detect). pub fn observe_session_instance(active: &ActiveSession) { let cur = (active.kind, active.compositor_pid); - let mut last = LAST_INSTANCE.lock().unwrap_or_else(|e| e.into_inner()); - if let Some(prev) = *last { + // DECIDE under the lock, ACT outside it. The action below drops keep-alive displays through the + // registry lock and shells out to `systemctl` on a 10 s budget; holding a process-wide mutex + // across that blocks every other detector — and this runs from the per-connect resolve, the + // mid-stream switch watcher AND the capture-loss re-detect. The baseline is advanced inside the + // lock too, so a concurrent observer sees the new instance and cannot run the action twice. + let changed = { + let mut last = LAST_INSTANCE.lock().unwrap_or_else(|e| e.into_inner()); + let prev = *last; + *last = Some(cur); + prev + }; + if let Some(prev) = changed { // Only a **desktop** compositor (KWin / Mutter / wlroots) instance change bumps the epoch + // invalidates its kept displays — its PipeWire node dies with the compositor. A **gamescope** // session (`ActiveKind::Gaming`) is NOT the epoch's subject: the box's game-mode / managed @@ -83,7 +93,6 @@ pub fn observe_session_instance(active: &ActiveSession) { ); } } - *last = Some(cur); } /// Counterpart to [`settle_desktop_portal`]'s `import-environment`: drop the desktop session's diff --git a/crates/pf-vdisplay/src/vdisplay/windows/manager.rs b/crates/pf-vdisplay/src/vdisplay/windows/manager.rs index 74556924..232c9ff9 100644 --- a/crates/pf-vdisplay/src/vdisplay/windows/manager.rs +++ b/crates/pf-vdisplay/src/vdisplay/windows/manager.rs @@ -983,8 +983,22 @@ impl VirtualDisplayManager { // a pair: eviction restarts presentation, the session re-attaches capture. TOPOLOGY_REASSERT_GEN.fetch_add(1, Ordering::Relaxed); } - }) - .expect("spawn vdisplay-exclusive-watch"); + }); + // NOT `.expect()`: this runs holding BOTH `exclusive_watch` and (via the caller) `state`, so + // a panic here poisons the two locks the whole manager runs on — every later `acquire` and + // every mgmt read would then panic on `.lock().unwrap()`. A thread we could not spawn means + // no re-assert watchdog, which is a degradation; wedging the manager is not. + let thread = match thread { + Ok(t) => t, + Err(e) => { + tracing::error!( + error = %e, + "could not spawn the exclusive re-assert watchdog — the isolate will not be \ + re-asserted if something re-lights a physical display" + ); + return; + } + }; *guard = Some(Pinger { stop, thread }); } diff --git a/crates/punktfunk-host/src/gamestream/stream.rs b/crates/punktfunk-host/src/gamestream/stream.rs index 36ea6ead..c7500bba 100644 --- a/crates/punktfunk-host/src/gamestream/stream.rs +++ b/crates/punktfunk-host/src/gamestream/stream.rs @@ -254,6 +254,21 @@ fn run( // Desktop<->Game switch. let (mut capturer, compositor, gamescope_route) = open_gs_virtual_source(cfg, app, target.as_ref(), &life.quit)?; + // Only the Linux `launch_is_nested` reads it; gamescope does not exist on Windows. + #[cfg(not(target_os = "linux"))] + let _ = &gamescope_route; + // Register this session in the admission table. GameStream acquires a REAL display but + // never registered, so `admit`'s Windows budgets — `max_displays` and the NVENC session + // headroom — could not see it: both gate on `!live.is_empty()`, so a Moonlight-held display + // was invisible to them and a native connect could be admitted past a budget the box had + // already spent. Identity is `None` (the compat plane has no cert fingerprint), which is the + // same "anonymous" the conflict policy already handles. Dropped at the end of `run`. + let _admission_guard = crate::vdisplay::admission::register( + None, + (cfg.width, cfg.height, cfg.fps), + life.quit.clone(), + "gamestream".to_string(), + ); tracing::info!( ?compositor, app = ?app.map(|a| &a.title), diff --git a/crates/punktfunk-host/src/native/stream.rs b/crates/punktfunk-host/src/native/stream.rs index 2a5e237e..c1f610e5 100644 --- a/crates/punktfunk-host/src/native/stream.rs +++ b/crates/punktfunk-host/src/native/stream.rs @@ -1107,6 +1107,10 @@ pub(super) fn virtual_stream(ctx: SessionContext, prepared: Option