fix(vdisplay): locks held across seconds-long work, an AB/BA inversion, and a panic that poisons the manager
ci / web (push) Successful in 1m3s
ci / docs-site (push) Successful in 1m9s
ci / bench (push) Successful in 5m30s
windows-host / package (push) Failing after 7m52s
windows-host / winget-source (push) Skipped
android / android (push) Successful in 12m10s
decky / build-publish (push) Successful in 21s
docker / build-push (--build-arg FEDORA_VERSION=44, ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora44-rpm) (push) Successful in 13s
ci / rust-arm64 (push) Successful in 13m14s
docker / build-push (., web/Dockerfile, punktfunk-web) (push) Successful in 40s
docker / build-push (ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora-rpm) (push) Successful in 14s
docker / build-push (ci, ci/rust-ci-noble.Dockerfile, punktfunk-rust-ci-noble) (push) Successful in 11s
docker / build-push (ci, ci/rust-ci.Dockerfile, punktfunk-rust-ci) (push) Successful in 18s
docker / build-push (docs-site, docs-site/Dockerfile, punktfunk-docs) (push) Successful in 14s
deb / build-publish (push) Successful in 9m5s
docker / build-push-arm64cross (push) Successful in 16s
docker / deploy-docs (push) Successful in 25s
arch / build-publish (push) Failing after 17m12s
deb / build-publish-client-arm64 (push) Successful in 8m56s
ci / rust (push) Failing after 17m13s
deb / build-publish-host (push) Successful in 10m10s
rpm / build-publish (44, fedora-44, punktfunk-fedora44-rpm) (push) Successful in 16m32s
rpm / build-publish (43, bazzite, punktfunk-fedora-rpm) (push) Successful in 21m39s
apple / swift (push) Successful in 5m32s
apple / screenshots (push) Successful in 25m53s
ci / web (push) Successful in 1m3s
ci / docs-site (push) Successful in 1m9s
ci / bench (push) Successful in 5m30s
windows-host / package (push) Failing after 7m52s
windows-host / winget-source (push) Skipped
android / android (push) Successful in 12m10s
decky / build-publish (push) Successful in 21s
docker / build-push (--build-arg FEDORA_VERSION=44, ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora44-rpm) (push) Successful in 13s
ci / rust-arm64 (push) Successful in 13m14s
docker / build-push (., web/Dockerfile, punktfunk-web) (push) Successful in 40s
docker / build-push (ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora-rpm) (push) Successful in 14s
docker / build-push (ci, ci/rust-ci-noble.Dockerfile, punktfunk-rust-ci-noble) (push) Successful in 11s
docker / build-push (ci, ci/rust-ci.Dockerfile, punktfunk-rust-ci) (push) Successful in 18s
docker / build-push (docs-site, docs-site/Dockerfile, punktfunk-docs) (push) Successful in 14s
deb / build-publish (push) Successful in 9m5s
docker / build-push-arm64cross (push) Successful in 16s
docker / deploy-docs (push) Successful in 25s
arch / build-publish (push) Failing after 17m12s
deb / build-publish-client-arm64 (push) Successful in 8m56s
ci / rust (push) Failing after 17m13s
deb / build-publish-host (push) Successful in 10m10s
rpm / build-publish (44, fedora-44, punktfunk-fedora44-rpm) (push) Successful in 16m32s
rpm / build-publish (43, bazzite, punktfunk-fedora-rpm) (push) Successful in 21m39s
apple / swift (push) Successful in 5m32s
apple / screenshots (push) Successful in 25m53s
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) <noreply@anthropic.com>
This commit is contained in:
@@ -66,9 +66,7 @@ fn stream_rect() -> Option<Rect> {
|
||||
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");
|
||||
|
||||
@@ -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 {
|
||||
// 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();
|
||||
let decision = decide(effective_conflict(), req_identity, &live);
|
||||
(
|
||||
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 {
|
||||
|
||||
@@ -963,6 +963,13 @@ fn create_managed_session_steamos(mode: Mode, hdr: bool) -> Result<VirtualOutput
|
||||
write_steamos_dropin(&shim_dir, mode, hdr)?;
|
||||
systemctl_user(&["daemon-reload"]);
|
||||
systemctl_user(&["restart", STEAMOS_SESSION_TARGET]);
|
||||
// LOCK ORDER. Everything below this line must run WITHOUT `MANAGED_SESSION` held: the restore
|
||||
// path takes STEAMOS_TOOK_OVER first and MANAGED_SESSION second (`do_restore_tv_session`), and
|
||||
// `takeover_live` reads the whole set — so taking them in the other order here is an AB/BA
|
||||
// deadlock between a connect and the restore worker, which genuinely run concurrently
|
||||
// (`registry.rs` calls `vd.create` off the registry lock; the worker is its own thread).
|
||||
// Nothing between here and the re-acquire reads the tracked session.
|
||||
drop(guard);
|
||||
*STEAMOS_TOOK_OVER.lock().unwrap_or_else(|e| e.into_inner()) = true;
|
||||
persist_takeover(); // A3: survive a host crash mid-stream
|
||||
// gamescope's node appears within a few seconds of the restart; Steam's first FRAME is slower
|
||||
@@ -980,7 +987,8 @@ fn create_managed_session_steamos(mode: Mode, hdr: bool) -> Result<VirtualOutput
|
||||
// a clean restart rather than a same-mode reuse of a session we just rejected.
|
||||
verify_managed_spawn_flags(hdr)?;
|
||||
point_injector_at_eis();
|
||||
*guard = Some(SessionState {
|
||||
// Re-acquire to record the tracked session — the same shape `create_managed_session` uses.
|
||||
*MANAGED_SESSION.lock().unwrap_or_else(|e| e.into_inner()) = Some(SessionState {
|
||||
width: mode.width,
|
||||
height: mode.height,
|
||||
refresh_hz: mode.refresh_hz,
|
||||
@@ -1889,15 +1897,21 @@ pub fn schedule_restore_tv_session() {
|
||||
/// display manager, a SteamOS target we re-pointed, or a managed session we launched beside a live
|
||||
/// desktop? The precondition for every restore path.
|
||||
fn takeover_live() -> 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
|
||||
.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()
|
||||
.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",
|
||||
|
||||
@@ -50,8 +50,18 @@ static LAST_INSTANCE: std::sync::Mutex<Option<(ActiveKind, Option<u32>)>> =
|
||||
/// 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);
|
||||
// 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());
|
||||
if let Some(prev) = *last {
|
||||
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
|
||||
|
||||
@@ -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 });
|
||||
}
|
||||
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -1107,6 +1107,10 @@ pub(super) fn virtual_stream(ctx: SessionContext, prepared: Option<PreparedDispl
|
||||
bringup,
|
||||
resize_ms,
|
||||
} = ctx;
|
||||
// Only the Linux paths (`launch_is_nested`, `set_gamescope_route`) read it; gamescope does not
|
||||
// exist on Windows, where every one of those call sites is cfg'd out.
|
||||
#[cfg(target_os = "windows")]
|
||||
let _ = &gamescope_route;
|
||||
// Reference point for adopting the launched game's processes: anything the host will call "this
|
||||
// session's game" has to have started after this instant. Taken HERE, before the display (and
|
||||
// therefore before a bare-spawn gamescope's nested child) exists, because a reading taken after
|
||||
|
||||
Reference in New Issue
Block a user