Merge pull request 'A compositor pin silently vetoed dedicated game sessions — and a mid-bring-up mode switch was killing GNOME outright' (#121) from worktree-dedicated-session-pin-and-recovery into main

Reviewed-on: #121

crates/pf-vdisplay/src/lib.rs

@@ -130,6 +130,22 @@ impl Compositor {
         }
     }
 
+    /// Does this backend need a compositor that is ALREADY RUNNING for this uid?
+    ///
+    /// Every desktop backend attaches to a live session — it asks Mutter/KWin/sway/Hyprland to mint
+    /// a virtual output over their IPC, so with nothing running there is no one to ask and `create`
+    /// can only fail (on GNOME: `RemoteDesktop.CreateSession:
+    /// org.freedesktop.DBus.Error.ServiceUnknown`). [`Compositor::Gamescope`] is the exception: it
+    /// stands its own session up from nothing (bare headless spawn / managed takeover), which is
+    /// exactly why a headless box pins to it.
+    ///
+    /// Callers use this to tell "the session is up" from "the session is a corpse" BEFORE marching a
+    /// client into a doomed bring-up — the state a compositor crash leaves behind (gnome-shell
+    /// SIGSEGV → GDM greeter, whose auto-login is once-per-boot, so it never returns on its own).
+    pub fn needs_live_session(self) -> bool {
+        !matches!(self, Compositor::Gamescope)
+    }
+
     /// Human label for UIs.
     pub fn label(self) -> &'static str {
         match self {

crates/punktfunk-host/src/native/compositor.rs

@@ -33,6 +33,52 @@ fn pick_compositor(
     }
 }
 
+/// Is this connect pinned at a compositor that is not actually running?
+///
+/// Pure (the I/O shell passes in the observed liveness) so the interaction is unit-tested, because
+/// it is invisible from the outside: an operator pin puts its backend into
+/// [`crate::vdisplay::available`] unconditionally AND skips `apply_session_env`'s
+/// `XDG_CURRENT_DESKTOP` scrub, so [`pick_compositor`] hands back a compositor that may be a corpse
+/// and its `None` (recover) arm can never fire. [`Compositor::Gamescope`] is exempt — it stands its
+/// own session up, which is the whole reason a headless box pins it.
+#[cfg(not(target_os = "windows"))]
+fn pinned_at_a_dead_session(
+    overridden: bool,
+    chosen: crate::vdisplay::Compositor,
+    live: crate::vdisplay::ActiveKind,
+) -> bool {
+    overridden && chosen.needs_live_session() && live == crate::vdisplay::ActiveKind::None
+}
+
+/// The handshake error for "no graphical session is live for this uid" — the state a compositor
+/// crash leaves behind (gnome-shell SIGSEGV → GDM greeter, whose auto-login is once-per-boot, so the
+/// box would otherwise need a walk-up or a reboot).
+///
+/// Fires the operator's recovery hook (debounced) on the way out when one is configured, so the
+/// client's retry a few seconds later lands in a recovered desktop. `pinned` names the
+/// `PUNKTFUNK_COMPOSITOR` value when the pin is what got us here, so the message can say which knob
+/// to change rather than the generic advice to *set* the knob that caused it.
+#[cfg(not(target_os = "windows"))]
+fn no_live_session(pinned: Option<&str>) -> anyhow::Error {
+    if crate::vdisplay::try_recover_session() {
+        return anyhow::anyhow!(
+            "no live graphical session for this uid — host session recovery launched \
+             (PUNKTFUNK_RECOVER_SESSION_CMD); retry in a few seconds"
+        );
+    }
+    match pinned {
+        Some(pin) => anyhow::anyhow!(
+            "PUNKTFUNK_COMPOSITOR={pin} pins this host to a backend that can only attach to an \
+             already-running compositor, and no graphical session is live for this uid — start a \
+             session, pin `gamescope` (it stands its own up), or set PUNKTFUNK_RECOVER_SESSION_CMD"
+        ),
+        None => anyhow::anyhow!(
+            "no usable compositor (no live graphical session for this uid; set \
+             PUNKTFUNK_COMPOSITOR or start a desktop/gaming session)"
+        ),
+    }
+}
+
 /// Resolve the client's compositor preference to a concrete backend (the I/O shell around
 /// [`pick_compositor`]): enumerate what's available, auto-detect the default, pick, and log
 /// whether the explicit request was honored or fell back. Runs blocking probes — call off the
@@ -61,13 +107,21 @@ pub(super) fn resolve_compositor(
         // Explicit operator override (legacy / CI / forcing a backend for a test) wins and is assumed
         // to come with a hand-set env — don't retarget the process env in that case.
         let overridden = pf_host_config::config().compositor.is_some();
+        // Liveness is read on BOTH paths. The auto path retargets the process env at the live
+        // session (below); the PINNED path needs it too, because a pin names a BACKEND, not a
+        // running session — and a pin whose compositor has died used to be indistinguishable from a
+        // healthy one here (it skips `apply_session_env`'s `XDG_CURRENT_DESKTOP` scrub and lands
+        // itself in `available()`, so `pick_compositor` could never return `None`). That combination
+        // marched every client through 8 doomed `create` retries and left the operator's
+        // `PUNKTFUNK_RECOVER_SESSION_CMD` unreachable — see the `needs_live_session` gate below.
+        let active = crate::vdisplay::detect_active_session();
         let detected = if overridden {
             crate::vdisplay::detect().ok()
         } else {
             // Auto: detect the LIVE session (Gaming vs Desktop) and retarget the process env at it so
             // every backend (video capture + input) this connect opens against the active session —
             // this is the state machine that lets one host follow a Bazzite box across Gaming↔Desktop.
-            let active = crate::vdisplay::detect_active_session();
+            //
             // A4: if the compositor instance changed since the last connect (an idle-time Game↔Desktop
             // switch), bump the epoch + invalidate the old backend's kept displays so this connect never
             // reuses a node id from the dead instance.
@@ -84,14 +138,36 @@ pub(super) fn resolve_compositor(
         // under `game_session=dedicated` (gamescope confirmed available) forces its OWN headless
         // gamescope spawn at the client's mode, overriding the detected desktop/game-mode backend. The
         // env was already retargeted above (for XDG_RUNTIME_DIR / the PipeWire daemon); we just pin the
-        // backend + input to the spawn sub-mode. Skipped under an explicit operator compositor pin.
-        if dedicated_launch && !overridden {
-            let route = crate::vdisplay::apply_input_env(Compositor::Gamescope, true);
-            tracing::info!(
-                ?route,
-                "dedicated game session — routing to a headless gamescope spawn at the client mode"
-            );
-            return Ok((Compositor::Gamescope, route));
+        // backend + input to the spawn sub-mode. An explicit operator compositor pin still outranks
+        // it — but says so out loud (below), because a silent veto is indistinguishable from the
+        // feature being broken.
+        if dedicated_launch {
+            if overridden {
+                // The pin still wins (it is the operator's explicit, hand-configured knob), but it
+                // must NEVER win silently: the console goes on displaying `game_session=dedicated`
+                // while every launch lands in the pinned session instead, and nothing in the log
+                // connects the two. That cost a full triage on a box whose `PUNKTFUNK_COMPOSITOR`
+                // was a forgotten validation leftover — the setting had never once taken effect and
+                // the only evidence was the ABSENCE of the info! line below.
+                tracing::warn!(
+                    pin = pf_host_config::config()
+                        .compositor
+                        .as_deref()
+                        .unwrap_or("-"),
+                    "game_session=dedicated asked for this launch's OWN headless gamescope, but \
+                     PUNKTFUNK_COMPOSITOR pins this host to a backend — the operator pin wins and \
+                     the game launches into the pinned session instead. Unset PUNKTFUNK_COMPOSITOR \
+                     to get dedicated game sessions."
+                );
+            } else {
+                let route = crate::vdisplay::apply_input_env(Compositor::Gamescope, true);
+                tracing::info!(
+                    ?route,
+                    "dedicated game session — routing to a headless gamescope spawn at the client \
+                     mode"
+                );
+                return Ok((Compositor::Gamescope, route));
+            }
         }
         let available = crate::vdisplay::available();
         let chosen = match pick_compositor(pref, &available, detected) {
@@ -112,23 +188,18 @@ pub(super) fn resolve_compositor(
                 );
                 Compositor::Gamescope
             }
-            None => {
-                // The state a compositor crash leaves behind (gnome-shell
-                // SIGSEGV → GDM greeter, whose auto-login is once-per-boot). If the operator
-                // configured a recovery hook, fire it (debounced) and tell the client to retry:
-                // its next knock lands in the recovered desktop.
-                if crate::vdisplay::try_recover_session() {
-                    anyhow::bail!(
-                        "no live graphical session for this uid — host session recovery launched \
-                         (PUNKTFUNK_RECOVER_SESSION_CMD); retry in a few seconds"
-                    );
-                }
-                anyhow::bail!(
-                    "no usable compositor (no live graphical session for this uid; set \
-                     PUNKTFUNK_COMPOSITOR or start a desktop/gaming session)"
-                );
-            }
+            None => return Err(no_live_session(None)),
         };
+        // Same dead-session exit, reached the other way: a pin puts its backend in `available()`
+        // unconditionally, so `pick_compositor` above can hand back a compositor that is not
+        // actually running and the `None` arm never fires. Check the backend's own requirement
+        // against observed liveness instead of trusting the pin. Gamescope is exempt — it stands
+        // its own session up, which is the whole point of pinning it on a headless box.
+        if pinned_at_a_dead_session(overridden, chosen, active.kind) {
+            return Err(no_live_session(
+                pf_host_config::config().compositor.as_deref(),
+            ));
+        }
         // Point input at the same backend and resolve the gamescope sub-mode (managed where the
         // session infra exists, attach to a foreign gamescope, else per-session bare spawn). The
         // route travels back to the caller as a VALUE and is carried on the backend instance — an
@@ -170,6 +241,44 @@ mod tests {
     use super::pick_compositor;
     use punktfunk_core::config::CompositorPref;
 
+    /// A pin at a compositor that ISN'T RUNNING must take the recovery exit rather than march the
+    /// client into a bring-up that can only fail.
+    ///
+    /// The regression this pins down: `PUNKTFUNK_COMPOSITOR=mutter` on a box whose gnome-shell had
+    /// segfaulted. The pin put Mutter in `available()` and suppressed the `XDG_CURRENT_DESKTOP`
+    /// scrub, so every connect "resolved" happily and then spent 8 retries on
+    /// `RemoteDesktop.CreateSession: ServiceUnknown` — while the operator's
+    /// `PUNKTFUNK_RECOVER_SESSION_CMD` sat unreachable behind a `None` arm that could never fire.
+    #[cfg(not(target_os = "windows"))]
+    #[test]
+    fn a_pin_at_a_dead_session_recovers_instead_of_retrying() {
+        use super::pinned_at_a_dead_session as dead;
+        use crate::vdisplay::{ActiveKind, Compositor::*};
+        // The bug: pinned to a desktop backend with nothing live for this uid.
+        assert!(dead(true, Mutter, ActiveKind::None));
+        assert!(dead(true, Kwin, ActiveKind::None));
+        assert!(dead(true, Wlroots, ActiveKind::None));
+        assert!(dead(true, Hyprland, ActiveKind::None));
+        // Pinned but the session IS up — the ordinary case, must not bail.
+        assert!(!dead(true, Mutter, ActiveKind::DesktopGnome));
+        // Gamescope stands its own session up from nothing: pinning it on a headless box is a
+        // SUPPORTED setup, not a dead session. (This is the .21 no-login workaround — never break it.)
+        assert!(!dead(true, Gamescope, ActiveKind::None));
+        // Unpinned is untouched: the auto path already reaches `pick_compositor`'s `None` arm via
+        // `compositor_for_kind(ActiveKind::None)`, and it owns the managed-takeover case.
+        assert!(!dead(false, Mutter, ActiveKind::None));
+    }
+
+    /// gamescope is the ONLY backend that can serve a connect with no session already running.
+    #[test]
+    fn only_gamescope_survives_a_dead_session() {
+        use crate::vdisplay::Compositor::*;
+        assert!(!Gamescope.needs_live_session());
+        for c in [Mutter, Kwin, Wlroots, Hyprland] {
+            assert!(c.needs_live_session(), "{c:?} needs a live compositor");
+        }
+    }
+
     #[test]
     fn compositor_resolution_precedence() {
         use crate::vdisplay::Compositor::*;

crates/punktfunk-host/src/native/stream.rs

@@ -1553,6 +1553,46 @@ pub(super) fn virtual_stream(ctx: SessionContext, prepared: Option<PreparedDispl
              encoder supports chunked output"
         );
     }
+    // A mode switch the control task accepted BEFORE the pipeline was built (the client connects at
+    // one mode and immediately asks for its real one — a fractional-scale panel resolving its native
+    // pixel size does exactly this, ~3 s ahead of bring-up finishing) used to be served the long way
+    // round: build the whole pipeline at the now-stale mode, then immediately rebuild at the new one
+    // in the loop below. That wastes a display create + capture attach + encoder open on every such
+    // connect, and on GNOME it is actively destructive — the rebuild is create-before-drop, so two
+    // `RecordVirtual` monitors ~400 ms apart segfault mutter 50.4 inside
+    // `meta_monitor_manager_rebuild`, taking down the whole desktop session (and with it the game
+    // just launched into it, which then looks like the GAME crashed). Adopt the newest queued mode
+    // here and build ONCE.
+    //
+    // Only on the inline path: a PREPARED pipeline is already built at the old mode, so adopting a
+    // new `mode` there would just make this variable disagree with the display that exists. Those
+    // sessions keep the rebuild-in-the-loop behavior. No accept ack is owed either way — the
+    // client's mode slot already flipped when control accepted the switch (it acks on accept, not
+    // on rebuild); the H2/H3 *correction* ack the rebuild would have sent is preserved below.
+    let mut mode = mode;
+    let mut adopted_at_bringup = false;
+    if prepared.is_none() {
+        let mut queued = None;
+        while let Ok(m) = reconfig.try_recv() {
+            queued = Some(m);
+        }
+        if let Some(m) = queued.filter(|m| *m != mode) {
+            adopted_at_bringup = true;
+            tracing::info!(
+                stale = ?mode,
+                adopted = ?m,
+                "a mode switch was accepted before bring-up finished — building at the new mode \
+                 instead of building twice"
+            );
+            mode = m;
+            // Mirror the loop's rebuild: PyroWave's Automatic bitrate is a per-mode ~1.6 bpp pin, so
+            // a resolution change moves the operating point. Explicit client rates stay put.
+            if bitrate_auto && plan.codec == crate::encode::Codec::PyroWave {
+                bitrate_kbps =
+                    resolve_bitrate_kbps_for(plan.codec, 0, &mode, plan.chroma, plan.bit_depth);
+            }
+        }
+    }
     tracing::info!(
         compositor = compositor.id(),
         ?mode,
@@ -1666,6 +1706,20 @@ pub(super) fn virtual_stream(ctx: SessionContext, prepared: Option<PreparedDispl
         &live_bitrate,
         &retarget_tx,
     );
+    // H2/H3 correction, carried over from the rebuild this bring-up replaced: the client APPLIED
+    // the mode when control accepted it, but the backend may have honored a different one (KWin
+    // caps a virtual output's refresh; a fallback delivers the size the source actually produces).
+    // Only for a mode adopted at bring-up — an ordinary connect's mode came from the Welcome, not
+    // from an accept the client has already acted on, so it is not owed a correction here.
+    if adopted_at_bringup {
+        let actual = delivered_mode(frame.width, frame.height, interval);
+        if actual != mode {
+            let _ = reconfig_result_tx.send(Reconfigured {
+                accepted: true,
+                mode: actual,
+            });
+        }
+    }
 
     // Capture is live — launch the requested title so it renders onto the streamed output and
     // grabs focus. Windows spawns the library id into the interactive user session; Linux spawns