From a5c9b7b8650a03bf9cec0030a5aee654ff4f1811 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Thu, 13 Aug 2026 15:41:43 +0200 Subject: [PATCH] fix(pf-vdisplay): serialize Mutter monitor rebuilds end-to-end, not just our D-Bus calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chaining two clients through a kept (keep-alive) Mutter display segfaults gnome-shell in meta_monitor_manager_rebuild (libmutter-18) and takes the whole desktop down; every later session then fails RemoteDesktop.CreateSession: ServiceUnknown until GDM restarts. A/B'd on .21: byte-identical on released 0.27.0 and the 0.28.0 RC, so it was never a regression — the trigger has been there all along. TOPOLOGY_LOCK already serialized every topology-mutating D-Bus call, but two gaps still let Mutter's REBUILDS overlap: - Teardown was fire-and-forget: StopGuard::drop set a flag and returned, and the session thread only noticed on its ≤200 ms park tick. The A2 dead-reuse path (reused kept display dead on first frame → mark_failed → re-create) therefore issued its fresh RecordVirtual with the doomed monitor's removal still pending — the fresh session could even win the lock BEFORE the old thread had woken to take it, adding a monitor while the dead one still stood. The drop now waits (bounded, 20 s) for the session thread to finish. - The lock was released while the shell was still rebuilding: Stop / RecordVirtual / ApplyMonitorsConfig all return mid-rebuild, and a temporary (APPLY_TEMPORARY) config auto-reverts asynchronously on top. Every locked mutation section now ends with settle_topology() — poll GetCurrentState until a removed connector is actually gone and the config serial holds still across two consecutive reads — before the guard drops. Bounded at 4 s and best-effort (a read error means the shell is gone; a hotplug storm must not park sessions), degrading to exactly the old behavior. Cost when Mutter is already quiet: one confirming read plus one 150 ms recheck per setup/teardown. The live_mutter_create_drop harness sheds its grace sleep — the synchronous drop IS the teardown confirmation now. Not fixed here, documented on TOPOLOGY_LOCK: the mid-stream mode-switch rebuild is create-before-drop by design (H2), so its RecordVirtual still lands while the superseded monitor exists; the settle makes Mutter quiescent at that point but cannot remove the coexistence itself. --- .../pf-vdisplay/src/vdisplay/linux/mutter.rs | 165 +++++++++++++++++- 1 file changed, 157 insertions(+), 8 deletions(-) diff --git a/crates/pf-vdisplay/src/vdisplay/linux/mutter.rs b/crates/pf-vdisplay/src/vdisplay/linux/mutter.rs index a4101c4d..171697c8 100644 --- a/crates/pf-vdisplay/src/vdisplay/linux/mutter.rs +++ b/crates/pf-vdisplay/src/vdisplay/linux/mutter.rs @@ -75,6 +75,20 @@ const CURSOR_EMBEDDED: u32 = 1; /// appearing at once, "the connector absent from MY pre-snapshot" can name a sibling's monitor. /// Each session runs on its own dedicated thread (see [`session_thread`]), so blocking on a std /// mutex — including across the awaits of its single-threaded setup future — is safe. +/// +/// The lock alone is NOT enough, because Mutter's rebuilds outlive our D-Bus calls: `Stop` / +/// `RecordVirtual` / `ApplyMonitorsConfig` return while the shell is still rebuilding (and, for a +/// session whose config was applied `APPLY_TEMPORARY`, still auto-reverting it). Releasing the lock +/// at that point hands the next session a NON-QUIESCENT Mutter, and its first mutation rebuilds +/// concurrently with the leftover one — the exact `meta_monitor_manager_rebuild` SIGSEGV again, +/// reproduced on 2026-08-08 (mid-bringup mode switch: two `RecordVirtual`s ~1 s apart) and +/// 2026-08-13 (keep-alive reuse dead on first frame → teardown + immediate re-create; A/B'd +/// identical on 0.27.0 and the 0.28.0 RC, so it was never a regression). So every locked mutation +/// section ends with [`settle_topology`] — poll DisplayConfig until the change is visible and the +/// config serial stops moving — BEFORE the guard drops. And because ordering across sessions runs +/// through the keepalive drop, [`StopGuard`]'s `Drop` must be SYNCHRONOUS (wait for the session +/// thread to finish its Stop + settle): a fire-and-forget flag let the A2 re-create win the lock +/// before the doomed session's thread had even woken to take it. static TOPOLOGY_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); /// The Mutter virtual-display driver. Each [`create`](VirtualDisplay::create) spins up a @@ -183,11 +197,18 @@ impl VirtualDisplay for MutterDisplay { let (setup_tx, setup_rx) = std::sync::mpsc::channel::>(); let stop = Arc::new(AtomicBool::new(false)); let stop_thread = stop.clone(); + // Teardown confirmation: the sender lives exactly as long as the session thread, so the + // guard's `Drop` can WAIT on `Disconnected` for the thread to finish its Stop + settle + // (see TOPOLOGY_LOCK — the drop is the only happens-before edge ordering "old monitor + // removed" against "next monitor created"). + let (done_tx, done_rx) = std::sync::mpsc::channel::<()>(); let first_in_group = self.first_in_group; let hw_cursor = self.hw_cursor; thread::Builder::new() .name("punktfunk-mutter-vout".into()) .spawn(move || { + // Dropped when the thread returns — every exit path signals `done_rx`. + let _done = done_tx; session_thread( setup_tx, stop_thread, @@ -204,7 +225,10 @@ impl VirtualDisplay for MutterDisplay { // that finishes after we gave up then parks for at most one 200 ms tick. `report_node` is // the primary defence (it stops the session outright); this is the belt-and-braces half, // and it also covers a thread that is somewhere else entirely when the timeout fires. - let guard = StopGuard(stop); + let guard = StopGuard { + stop, + done: done_rx, + }; // 45 s (was 20 s): setups now queue on TOPOLOGY_LOCK, so a session behind a slow sibling // (whose guard spans up to a ~10 s stream wait + 6 s connector wait + the apply) must @@ -230,11 +254,35 @@ impl VirtualDisplay for MutterDisplay { /// Dropping this ends the keepalive thread, closing the D-Bus connection — Mutter then tears /// the remote-desktop + screencast sessions (and the virtual monitor) down. -struct StopGuard(Arc); +/// +/// The drop is SYNCHRONOUS: it waits (bounded) for the session thread to confirm the teardown — +/// Stop issued, the monitor removal settled under [`TOPOLOGY_LOCK`]. The registry drops these +/// outside its pool lock and documents that the drop may block, and the callers that immediately +/// re-create (the A2 dead-reuse teardown, a mode-switch retire) are exactly the ones that NEED the +/// wait: with the old fire-and-forget flag, the fresh session's `RecordVirtual` could win +/// `TOPOLOGY_LOCK` before this session's thread had woken (≤200 ms park tick) to take it, adding a +/// monitor while the doomed one still stood — gnome-shell then died rebuilding the monitor manager +/// (`meta_monitor_manager_rebuild`, 2026-08-13, byte-identical on 0.27.0 and the 0.28.0 RC). +struct StopGuard { + stop: Arc, + /// Signals `Disconnected` when the session thread — which owns the paired sender — returns. + done: std::sync::mpsc::Receiver<()>, +} impl Drop for StopGuard { fn drop(&mut self) { - self.0.store(true, Ordering::Relaxed); + self.stop.store(true, Ordering::Relaxed); + // Generous: teardown is one ~200 ms park tick + Stop + a ≤4 s settle, but the thread may + // first have to outwait a sibling's setup holding TOPOLOGY_LOCK (up to ~16 s of stream + + // connector waits). Timing out is degraded-but-safe: the next mutation still queues on the + // lock; only the wake-up ordering guarantee is lost. + match self.done.recv_timeout(Duration::from_secs(20)) { + Ok(()) | Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => {} + Err(std::sync::mpsc::RecvTimeoutError::Timeout) => tracing::warn!( + "mutter: virtual-output teardown did not confirm within 20 s — proceeding; the \ + next topology mutation may race the shell's rebuild" + ), + } } } @@ -318,6 +366,10 @@ fn session_thread( Ok(s) => s, Err(e) => { let _ = setup_tx.send(Err(format!("{e:#}"))); + // A half-built session can still have ADDED the monitor (`RecordVirtual` succeeded, + // the node-id wait didn't) — its connections dropped inside `connect`, so Mutter is + // now removing it. Settle that rebuild before the guard releases the lock. + settle_topology(None, None).await; return; } }; @@ -325,6 +377,10 @@ fn session_thread( // mutates the operator's desktop topology on behalf of a session that, past this point, // would have no way to undo it. if !report_node(&setup_tx, &session).await { + // `report_node` already stopped the session — the virtual monitor is being removed. + // Settle under the still-held lock (same reasoning as the teardown below). + drop(session); + settle_topology(None, None).await; return; } // The send can also LAND in the moment the opener's `recv_timeout` gives up — the value sits @@ -338,6 +394,8 @@ fn session_thread( the desktop topology" ); let _ = session.rd_session.call_method("Stop", &()).await; + drop(session); + settle_topology(None, None).await; return; } @@ -382,6 +440,11 @@ fn session_thread( } } + // The lock's promise is "one rebuild at a time", which holds only if the rebuilds THIS + // setup caused — the `RecordVirtual` add, the `ApplyMonitorsConfig`, and Mutter's own + // auto-revert of any sibling's temporary config — are finished before it is released. + // Cheap when Mutter is already quiet (one confirming read + one 150 ms recheck). + settle_topology(tracked.as_ref().map(|(dc, _, _)| dc), None).await; drop(topology_guard); // Park, keeping `session` (and its zbus connection) alive until told to stop. Every ~5 s, @@ -414,13 +477,96 @@ fn session_thread( // the virtual output disappears and our DisplayConfig connection (in `tracked`) closes — so we // just drop it here and let the revert happen Mutter-side, never touching the layout ourselves. // The Stop (+ the revert it triggers) is a topology mutation too — take TOPOLOGY_LOCK so a - // sibling's teardown or setup can't interleave with the rebuild it causes. + // sibling's teardown or setup can't interleave with the rebuild it causes. And HOLD it + // until the removal has actually settled: `Stop` returns while the shell is still + // rebuilding, and the very next thing after this teardown is often a fresh create (the A2 + // dead-reuse re-create, a mode-switch retire) whose `RecordVirtual` must not land in that + // window — that overlap is the reproduced `meta_monitor_manager_rebuild` SIGSEGV. let _topology_guard = TOPOLOGY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); let _ = session.rd_session.call_method("Stop", &()).await; + let vconn = tracked.as_ref().map(|(_, _, v)| v.clone()); + // Close our own handles FIRST — the APPLY_TEMPORARY revert waits on the DisplayConfig + // connection in `tracked` closing (see above) — then observe the settle on a fresh one. drop(tracked); + drop(session); + settle_topology(None, vconn.as_deref()).await; }); } +/// Wait, bounded, for Mutter's monitor topology to go QUIET — called at the end of every +/// [`TOPOLOGY_LOCK`]-holding mutation section, before the guard drops (see the lock's docs for the +/// two field crashes this closes). Two phases, both polled over `GetCurrentState`: +/// +/// 1. when `gone` names a just-removed virtual connector, wait until it is actually absent (its +/// `Stop` returned before the shell finished the removal rebuild); +/// 2. wait until the config serial holds still across two consecutive reads — the rebuilds we +/// caused (add/remove/apply, plus Mutter's own auto-revert of a temporary config) each bump it. +/// +/// `dc` reuses the session's open DisplayConfig proxy when it has one; otherwise a fresh +/// short-lived connection is opened (the teardown path deliberately closes its own first — the +/// APPLY_TEMPORARY revert waits on that close). Best-effort by design: a read error usually means +/// the shell is gone (crashed or logging out), and the deadline keeps an unrelated hotplug storm +/// from parking a session forever — both degrade to "proceed", which is exactly the old behavior. +async fn settle_topology(dc: Option<&zbus::Proxy<'_>>, gone: Option<&str>) { + let fresh; + let dc = match dc { + Some(p) => p, + None => match display_config().await { + Ok(p) => { + fresh = p; + &fresh + } + Err(_) => { + // Nothing to observe (no DisplayConfig — a crashed shell?): a fixed grace still + // beats returning into the next mutation instantly. + tokio::time::sleep(Duration::from_millis(300)).await; + return; + } + }, + }; + let started = Instant::now(); + let deadline = started + Duration::from_secs(4); + if let Some(conn) = gone { + loop { + match get_state(dc).await { + Ok(s) if !connectors(&s).contains(conn) => break, + Ok(_) if Instant::now() < deadline => { + tokio::time::sleep(Duration::from_millis(100)).await; + } + _ => break, // read error (shell gone) or deadline — proceed either way + } + } + } + let mut last: Option = None; + loop { + match get_state(dc).await { + Ok(s) => { + if last == Some(s.0) { + break; + } + last = Some(s.0); + } + Err(_) => break, + } + if Instant::now() >= deadline { + tracing::warn!( + "mutter: the monitor topology did not settle within 4 s — proceeding (a concurrent \ + hotplug?)" + ); + break; + } + tokio::time::sleep(Duration::from_millis(150)).await; + } + let waited = started.elapsed(); + if waited > Duration::from_millis(600) { + tracing::info!( + waited_ms = waited.as_millis() as u64, + removed = gone.is_some(), + "mutter: waited out a monitor-topology rebuild before releasing the lock" + ); + } +} + /// Record an **existing** monitor by connector — the monitor-mirror path /// (`design/per-monitor-portal-capture.md` L2). Returns the PipeWire node id and the keepalive /// whose drop stops the recording. @@ -1543,10 +1689,13 @@ mod tests { ); std::thread::sleep(std::time::Duration::from_secs(3)); + // The keepalive's Drop is synchronous: it returns only once the session thread has run the + // Stop and settled the removal rebuild (see `StopGuard`), so no grace sleep is needed. + let dropped_at = std::time::Instant::now(); drop(out); - // The keepalive's Drop only SIGNALS the thread; give it more than one 200 ms tick to run - // the Stop + topology revert before the harness exits and takes the process with it. - std::thread::sleep(std::time::Duration::from_secs(2)); - println!("dropped — gnome-shell should have removed the monitor and reverted the topology"); + println!( + "dropped in {:?} — gnome-shell should have removed the monitor and reverted the topology", + dropped_at.elapsed() + ); } } -- 2.54.0