Fix config and IPC lifecycle issues and remove obsolete code

compositor/src/reload.rs

@@ -1,121 +1,5 @@
 //! Filesystem events wake the loop; only pending edits need a debounce timer.
-#[cfg(test)]
-use std::io::Read;
 use std::path::PathBuf;
-#[cfg(test)]
-use std::sync::mpsc;
-#[cfg(test)]
-use std::time::{Duration, Instant};
-
-#[cfg(test)]
-use notify::{RecommendedWatcher, RecursiveMode, Watcher};
-
-#[cfg(test)]
-fn read_source(path: &std::path::Path) -> Result<String, String> {
-    let file = std::fs::File::open(path).map_err(|e| e.to_string())?;
-    let mut source = String::new();
-    file.take(60 * 1024 + 1)
-        .read_to_string(&mut source)
-        .map_err(|e| e.to_string())?;
-    if source.len() > 60 * 1024 {
-        return Err("configuration exceeds the 60 KiB live-reload limit".into());
-    }
-    Ok(source)
-}
-
-#[cfg(test)]
-pub(crate) struct ConfigMonitor {
-    _watcher: RecommendedWatcher,
-    events: mpsc::Receiver<()>,
-    path: PathBuf,
-    dirty: Option<Instant>,
-    observed: Option<String>,
-}
-
-#[cfg(test)]
-impl ConfigMonitor {
-    #[cfg(test)]
-    pub(crate) fn new(path: PathBuf) -> notify::Result<Self> {
-        Self::with_wakeup(path, None)
-    }
-
-    pub(crate) fn with_wakeup(
-        path: PathBuf,
-        wakeup: Option<smithay::reexports::calloop::LoopSignal>,
-    ) -> notify::Result<Self> {
-        let (sender, events) = mpsc::sync_channel(1);
-        let target = path.clone();
-        let event_wakeup = wakeup.clone();
-        let mut watcher = notify::recommended_watcher(move |event: notify::Result<notify::Event>| {
-            if let Ok(event) = event
-                && !event.kind.is_access()
-                && event.paths.iter().any(|p| p == &target || target.starts_with(p))
-            {
-                let _ = sender.try_send(());
-                if let Some(wakeup) = &event_wakeup {
-                    wakeup.wakeup();
-                }
-            }
-        })?;
-        // Watch the directory, not the inode: atomic editor saves replace it.
-        let mut directory = path.parent().unwrap_or(std::path::Path::new("."));
-        while !directory.is_dir() {
-            directory = directory.parent().unwrap_or(std::path::Path::new("."));
-        }
-        watcher.watch(
-            directory,
-            if Some(directory) == path.parent() {
-                RecursiveMode::NonRecursive
-            } else {
-                RecursiveMode::Recursive
-            },
-        )?;
-        let initial_dirty = path.is_file().then(Instant::now);
-        if initial_dirty.is_some()
-            && let Some(wakeup) = wakeup
-        {
-            wakeup.wakeup();
-        }
-        Ok(Self {
-            _watcher: watcher,
-            events,
-            path,
-            dirty: initial_dirty,
-            observed: None,
-        })
-    }
-
-    pub(crate) fn next_deadline(&self) -> Option<Instant> {
-        self.dirty.map(|since| since + Duration::from_millis(120))
-    }
-
-    pub(crate) fn poll(&mut self, now: Instant) -> Option<Result<String, String>> {
-        if self.events.try_iter().next().is_some() {
-            self.dirty = Some(now);
-        }
-        if self
-            .dirty
-            .is_none_or(|since| now.saturating_duration_since(since) < Duration::from_millis(120))
-        {
-            return None;
-        }
-        self.dirty = None;
-        let source = match read_source(&self.path) {
-            Ok(source) => source,
-            Err(error) => {
-                return Some(Err(format!(
-                    "{}: {error}; retaining active config",
-                    self.path.display()
-                )));
-            }
-        };
-        if self.observed.as_ref() == Some(&source) {
-            return None;
-        }
-        self.observed = Some(source.clone());
-        Some(Ok(source))
-    }
-}
 
 #[derive(Clone)]
 struct Prepared {
@@ -369,6 +253,7 @@ impl crate::Ferese {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use std::time::{Duration, Instant};
     struct Directory(PathBuf);
     impl Directory {
         fn new() -> Self {
@@ -663,45 +548,16 @@ mod tests {
     }
 
     #[test]
-    fn directory_watch_detects_atomic_save_and_does_not_read_during_idle() {
+    fn source_size_limit_uses_the_production_reader_and_parser() {
         let directory = Directory::new();
         let path = directory.0.join("config.kdl");
-        std::fs::write(&path, "animations {\n    speed 1\n}\n").unwrap();
-        let mut monitor = ConfigMonitor::new(path.clone()).unwrap();
-        monitor.dirty = Some(Instant::now() - Duration::from_secs(1));
-        assert!(monitor.poll(Instant::now()).unwrap().is_ok());
-        // With no event, even a deliberately missing path is never read.
-        let original_path = monitor.path.clone();
-        monitor.path = directory.0.join("missing");
-        assert!(monitor.poll(Instant::now()).is_none());
-        monitor.path = original_path;
-        let temporary = directory.0.join("config.kdl.new");
-        std::fs::write(&temporary, "animations {\n    speed 0.75\n}\n").unwrap();
-        std::fs::rename(&temporary, &path).unwrap();
-        let deadline = Instant::now() + Duration::from_secs(3);
-        loop {
-            if let Some(result) = monitor.poll(Instant::now()) {
-                assert_eq!(result.unwrap(), "animations {\n    speed 0.75\n}\n");
-                break;
-            }
-            assert!(Instant::now() < deadline, "atomic save notification never settled");
-            std::thread::sleep(Duration::from_millis(10));
-        }
-    }
-
-    #[test]
-    fn source_size_limit_is_bounded_and_debounce_coalesces_edits() {
-        let directory = Directory::new();
-        let path = directory.0.join("config.kdl");
-        std::fs::write(&path, "x".repeat(60 * 1024 + 1)).unwrap();
-        assert!(read_source(&path).is_err());
-        std::fs::write(&path, "animations {\n    speed 1\n}\n").unwrap();
-        let mut monitor = ConfigMonitor::new(path).unwrap();
-        let now = Instant::now();
-        monitor.dirty = Some(now);
-        assert_eq!(monitor.next_deadline(), Some(now + Duration::from_millis(120)));
-        assert!(monitor.poll(now + Duration::from_millis(119)).is_none());
-        assert!(monitor.poll(now + Duration::from_millis(121)).unwrap().is_ok());
-        assert_eq!(monitor.next_deadline(), None);
+        let source = format!("//{}", "x".repeat(60 * 1024 - 2));
+        std::fs::write(&path, &source).unwrap();
+        assert_eq!(crate::config::read_source(&path).unwrap(), source);
+        assert!(Prepared::new(source.clone(), &directory.0).is_ok());
+        let oversized = source + "x";
+        std::fs::write(&path, &oversized).unwrap();
+        assert!(crate::config::read_source(&path).is_err());
+        assert!(Prepared::new(oversized, &directory.0).is_err());
     }
 }

compositor/src/resume.rs

@@ -216,7 +216,7 @@ async fn listen(
     let mut signals = proxy.receive_signal("PrepareForSleep").await?;
     // AddMatch succeeds even when logind is absent. Only call the listener
     // recovered once the service is present, and retry if its owner changes.
-    connection
+    let owner = connection
         .call_method(
             Some("org.freedesktop.DBus"),
             "/org/freedesktop/DBus",
@@ -236,7 +236,7 @@ async fn listen(
 
     future::race(
         async {
-            owners.next().await;
+            owner_changed_after(&mut owners, owner.recv_position()).await;
             Ok(MonitorExit::Reconnect)
         },
         async {
@@ -253,5 +253,16 @@ async fn listen(
     .await
 }
 
+// Owner changes queued during subscription setup are already reflected in the
+// GetNameOwner reply. Only a later change interrupts this recovered listener.
+async fn owner_changed_after(owners: &mut zbus::proxy::OwnerChangedStream<'_>, baseline: zbus::message::Sequence) {
+    use zbus::export::ordered_stream::OrderedStreamExt;
+    while let Some((sequence, _)) = owners.next_before(None).await.into_tuple() {
+        if sequence > baseline {
+            break;
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests;

compositor/src/resume/tests.rs

@@ -247,3 +247,56 @@ fn shutdown_interrupts_connection_authentication() {
     };
     stop_and_join(monitor);
 }
+
+#[test]
+fn ownership_acquired_before_subscription_baseline_is_not_a_second_outage() {
+    if !crate::startup_tests::private_runtime(
+        "resume::tests::ownership_acquired_before_subscription_baseline_is_not_a_second_outage",
+    ) {
+        return;
+    }
+    let bus = Bus::new();
+    let (connection, mut owners, manager, baseline) = future::block_on(async {
+        let connection = zbus::connection::Builder::address(bus.address.as_str())
+            .unwrap()
+            .build()
+            .await
+            .unwrap();
+        let proxy = zbus::Proxy::new(
+            &connection,
+            "org.freedesktop.login1",
+            "/org/freedesktop/login1",
+            "org.freedesktop.login1.Manager",
+        )
+        .await
+        .unwrap();
+        let owners = proxy.receive_owner_changed().await.unwrap();
+        // Force acquisition after AddMatch but before the listener's baseline.
+        let manager = bus.manager();
+        let reply = connection
+            .call_method(
+                Some("org.freedesktop.DBus"),
+                "/org/freedesktop/DBus",
+                Some("org.freedesktop.DBus"),
+                "GetNameOwner",
+                &("org.freedesktop.login1",),
+            )
+            .await
+            .unwrap();
+        (connection, owners, manager, reply.recv_position())
+    });
+    let (sender, events) = mpsc::channel();
+    let worker = std::thread::spawn(move || {
+        // Keep the same connection alive so receive sequences remain comparable.
+        let _connection = connection;
+        future::block_on(owner_changed_after(&mut owners, baseline));
+        sender.send(()).unwrap();
+    });
+    assert!(
+        events.recv_timeout(Duration::from_millis(100)).is_err(),
+        "queued acquisition must not interrupt the recovered listener"
+    );
+    manager.release_name("org.freedesktop.login1").unwrap();
+    events.recv_timeout(Duration::from_secs(2)).unwrap();
+    worker.join().unwrap();
+}

compositor/src/theme.rs

@@ -149,6 +149,22 @@ pub(crate) struct Engine {
 }
 
 impl Engine {
+    fn files_changed(&mut self, now: Instant) -> bool {
+        if self
+            .events
+            .as_ref()
+            .is_some_and(|events| events.try_iter().next().is_some())
+        {
+            self.dirty = Some(now + DEBOUNCE);
+        }
+        if self.dirty.is_some_and(|deadline| now >= deadline) {
+            self.dirty = None;
+            true
+        } else {
+            false
+        }
+    }
+
     pub(crate) fn matches_files(&self, files: &[PathBuf]) -> bool {
         self.active_files == files
     }
@@ -357,21 +373,7 @@ pub(crate) fn init(
                 let _ = monitor.wait();
             }
         })?;
-    let watcher = notify::recommended_watcher(move |event: notify::Result<notify::Event>| {
-        if let Ok(event) = event
-            && !event.kind.is_access()
-            && event.paths.iter().any(|path| {
-                paths
-                    .lock()
-                    .unwrap()
-                    .iter()
-                    .any(|target| target == path || target.starts_with(path))
-            })
-        {
-            let _ = sender.try_send(());
-            wakeup.wakeup();
-        }
-    })?;
+    let watcher = file_watcher(paths, sender, wakeup)?;
     state.theme_engine.watcher = Some(watcher);
     state.theme_engine.events = Some(events);
     state.theme_engine.active_files = candidate.files.clone();
@@ -387,6 +389,28 @@ pub(crate) fn init(
     Ok(())
 }
 
+fn file_watcher(
+    paths: Arc<Mutex<HashSet<PathBuf>>>,
+    sender: mpsc::SyncSender<()>,
+    wakeup: calloop::LoopSignal,
+) -> notify::Result<RecommendedWatcher> {
+    notify::recommended_watcher(move |event: notify::Result<notify::Event>| {
+        if let Ok(event) = event
+            && !event.kind.is_access()
+            && event.paths.iter().any(|path| {
+                paths
+                    .lock()
+                    .unwrap()
+                    .iter()
+                    .any(|target| target == path || target.starts_with(path))
+            })
+        {
+            let _ = sender.try_send(());
+            wakeup.wakeup();
+        }
+    })
+}
+
 impl Ferese {
     pub(crate) fn refresh_theme(&mut self) {
         if let Err(error) = self.queue_config_reload(true, None) {
@@ -477,16 +501,7 @@ impl Ferese {
 
     pub(crate) fn poll_theme(&mut self) {
         let now = Instant::now();
-        if self
-            .theme_engine
-            .events
-            .as_ref()
-            .is_some_and(|events| events.try_iter().next().is_some())
-        {
-            self.theme_engine.dirty = Some(now + DEBOUNCE);
-        }
-        if self.theme_engine.dirty.is_some_and(|deadline| now >= deadline) {
-            self.theme_engine.dirty = None;
+        if self.theme_engine.files_changed(now) {
             self.refresh_theme();
         }
         let mut start = false;
@@ -618,6 +633,55 @@ impl Ferese {
 mod tests {
     use super::*;
 
+    #[test]
+    fn directory_watch_detects_repeated_atomic_saves() {
+        let directory = tempfile::tempdir().unwrap();
+        let path = directory.path().join("config.kdl");
+        std::fs::write(&path, "// initial").unwrap();
+        let event_loop = calloop::EventLoop::<()>::try_new().unwrap();
+        let (sender, events) = mpsc::sync_channel(1);
+        let mut engine = Engine {
+            events: Some(events),
+            ..Engine::default()
+        };
+        engine.watcher = Some(file_watcher(engine.paths.clone(), sender, event_loop.get_signal()).unwrap());
+        engine.watch_files(std::slice::from_ref(&path));
+        for source in ["// first save", "// second save"] {
+            let temporary = path.with_extension("new");
+            std::fs::write(&temporary, source).unwrap();
+            std::fs::rename(&temporary, &path).unwrap();
+            let deadline = Instant::now() + Duration::from_secs(3);
+            while !engine.files_changed(Instant::now()) {
+                assert!(Instant::now() < deadline, "atomic save notification never settled");
+                std::thread::sleep(Duration::from_millis(10));
+            }
+            assert_eq!(crate::config::read_source(&path).unwrap(), source);
+        }
+        assert!(!engine.files_changed(Instant::now() + Duration::from_secs(1)));
+    }
+
+    #[test]
+    fn file_events_debounce_from_the_last_edit_and_stay_idle_without_events() {
+        let (sender, events) = mpsc::sync_channel(1);
+        let mut engine = Engine {
+            events: Some(events),
+            ..Engine::default()
+        };
+        let now = Instant::now();
+        assert!(!engine.files_changed(now));
+        assert!(engine.dirty.is_none());
+        sender.send(()).unwrap();
+        assert!(!engine.files_changed(now));
+        assert_eq!(engine.dirty, Some(now + DEBOUNCE));
+        let second = now + Duration::from_millis(100);
+        sender.send(()).unwrap();
+        assert!(!engine.files_changed(second));
+        assert!(!engine.files_changed(now + DEBOUNCE));
+        assert!(engine.files_changed(second + DEBOUNCE));
+        assert!(engine.dirty.is_none());
+        assert!(!engine.files_changed(second + Duration::from_secs(1)));
+    }
+
     #[test]
     fn startup_warning_survives_repeated_rejection_without_duplicate_publication() {
         let mut engine = Engine::default();