From a17aa61c5a6a5e624e0cec504d1a815e15edfc34 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 26 Jul 2026 16:44:54 +0200 Subject: [PATCH] fix(session/gamelease): four things the first pass got wrong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found reviewing the riskiest paths of the previous commit. - An install dir of `/games/x` matched a process running out of `/games/xyz`. The image-path check was already component-wise and fine; the command-line check compared raw bytes, so it needed the separator. Two library folders where one name prefixes the other is not a contrived case. - Ending a nested game force-releases kept displays, which is not per-display — so with another client streaming it could retire a display that was never ours. Skip it while anyone else has a live session: the failure mode becomes a game that keeps running, which is the default everywhere else anyway. - A lease nothing polls (no detect signals, or a platform without a matcher yet) sat at "launching" forever in the console. Report it as running: the host did just launch it, and with no watcher it is claiming nothing about the exit. - A game ended after its session was already gone reported no `game.exited` at all — its watcher had stopped with the session, so nothing was left to emit it. That is every grace expiry and every `POST /game/end`, i.e. exactly the ones an operator most wants to see. Co-Authored-By: Claude Fable 5 --- crates/punktfunk-host/src/gamelease.rs | 45 ++++++++++++++++++++++---- crates/punktfunk-host/src/procscan.rs | 33 ++++++++++++++++--- 2 files changed, 68 insertions(+), 10 deletions(-) diff --git a/crates/punktfunk-host/src/gamelease.rs b/crates/punktfunk-host/src/gamelease.rs index f7dd7db2..eca76c29 100644 --- a/crates/punktfunk-host/src/gamelease.rs +++ b/crates/punktfunk-host/src/gamelease.rs @@ -335,6 +335,13 @@ pub fn open(req: LeaseRequest, on_exit: OnExit) -> GameLease { } let watcher = spawn_watcher(shared.clone(), child, on_exit); + if watcher.is_none() { + // Nothing is polling this lease (no signals to poll, or a platform without a matcher yet), so + // its state will never advance on its own. Report it as running rather than leaving the + // console showing "launching" forever: the host did just launch it, and with no watcher it is + // making no claim about noticing the exit. The row disappears when the session ends. + shared.set_state(GameState::Running); + } GameLease { shared, watcher } } @@ -564,9 +571,20 @@ pub fn terminate(shared: Arc, why: &'static str) { } tracing::info!(title = %shared.game.title, reason = why, "ending the launched game"); let name = "pf1-gameterm".to_string(); - let _ = std::thread::Builder::new() - .name(name) - .spawn(move || terminate_blocking(&shared)); + let _ = std::thread::Builder::new().name(name).spawn(move || { + terminate_blocking(&shared); + // A lease whose session is still up has a live watcher, which reports the exit itself + // (with the right reason) once it confirms the game is gone. A lease whose session has + // already ended — every grace expiry and every `POST /game/end` — has no watcher left, so + // this is the only place its exit can be reported from. + if shared.cancel.load(Ordering::Relaxed) { + shared.set_state(GameState::Exited); + crate::events::emit(crate::events::EventKind::GameExited { + game: game_event_ref(&shared), + reason: crate::events::GameEndReason::Terminated, + }); + } + }); } /// The ladder itself. Separate so it can be driven synchronously from a test. @@ -575,9 +593,24 @@ fn terminate_blocking(shared: &LeaseShared) { // gamescope owns the game: releasing the display tears the nested session — and therefore // the game — down. One mechanism for both, so a kept display can't outlive an ended game. LeaseKind::Nested => { - // The same force-release the `/display/release` endpoint performs. It refuses displays - // with live sessions, so this only ever retires the *kept* display this ended session - // left behind — which is exactly the one still holding the game. + // The same force-release the `/display/release` endpoint performs: gamescope exits with + // its display and takes its nested game with it. It refuses displays that still have live + // sessions, so this only reaches the *kept* display this ended session left behind. + // + // But it is not slot-targeted — it retires every kept display — and a lease has no handle + // on which one is its own. So while anyone else is streaming, leave it alone: the cost of + // being wrong here is disturbing an unrelated client, and the cost of doing nothing is a + // game that keeps running, which is also the default everywhere else. + let others = crate::session_status::count(); + if others > 0 { + tracing::info!( + live_sessions = others, + title = %shared.game.title, + "not releasing kept displays to end this game — another session is streaming and \ + the release is not per-display" + ); + return; + } let released = crate::vdisplay::registry::release(None); tracing::info!( released, diff --git a/crates/punktfunk-host/src/procscan.rs b/crates/punktfunk-host/src/procscan.rs index 5c7604f7..62d07812 100644 --- a/crates/punktfunk-host/src/procscan.rs +++ b/crates/punktfunk-host/src/procscan.rs @@ -192,11 +192,16 @@ impl Scanner { } } if let Some(dir) = install_dir { + // Require a path separator after the directory, so an install dir of `/games/x` is + // not satisfied by an unrelated `/games/xyz/…` argument. (The image-path check above + // needs no such care — `Path::starts_with` already compares whole components.) let needle = dir.as_os_str().as_encoded_bytes(); - for arg in cmdline.split(|&b| b == 0) { - if arg.starts_with(needle) { - return true; - } + let under_dir = |arg: &[u8]| { + arg.strip_prefix(needle) + .is_some_and(|rest| rest.first() == Some(&b'/')) + }; + if cmdline.split(|&b| b == 0).any(under_dir) { + return true; } } if let Some(want) = exe { @@ -376,6 +381,26 @@ mod tests { ); } + #[test] + fn install_dir_does_not_match_a_sibling_with_the_same_prefix() { + // `/games/x` must not adopt a process running out of `/games/xyz` — a real hazard when one + // library folder's name is a prefix of another's. + let td = fake_proc_root( + 1000.0, + &[ + FakeProc::new(90, 50_000).exe("/games/xyz/other"), + FakeProc::new(91, 50_000).cmdline(&["wrapper", "/games/xyz/other"]), + ], + ); + let s = scanner(td.path()); + assert!(s.find(&DetectSpec::dir("/games/x"), None).is_empty()); + // The genuine directory still matches, by image path and by argument. + assert_eq!( + pids(s.find(&DetectSpec::dir("/games/xyz"), None)), + vec![90, 91] + ); + } + #[test] fn matches_proton_style_game_only_in_the_cmdline() { // The image is the Proton runtime; the game appears only as an argument.