Repository: shell-pool/shpool
Pull request #67 (2024-06-25 15:51:36)

link to the troubleshoting guide from the README
This patch links to the troubleshooting wiki page from the README. It to help address #64.


Patch:
diff --git a/Cargo.lock b/Cargo.lock
index 741a6686..0dee2324 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -86,6 +86,12 @@ version = "0.7.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "96d30a06541fbafbc7f82ed10c06164cfbd2c401138f6addd8404629c4b16711"
 
+[[package]]
+name = "assert_matches"
+version = "1.5.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "9b34d609dfbaf33d6889b2b7106d3ca345eacad44200913df5ba02bfd31d2ba9"
+
 [[package]]
 name = "autocfg"
 version = "1.1.0"
@@ -264,6 +270,27 @@ version = "0.8.19"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "248e3bacc7dc6baa3b21e405ee045c3047101a49145e7e9eca583ab4c2ca5345"
 
+[[package]]
+name = "directories"
+version = "5.0.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "9a49173b84e034382284f27f1af4dcbbd231ffa358c0fe316541a7337f376a35"
+dependencies = [
+ "dirs-sys",
+]
+
+[[package]]
+name = "dirs-sys"
+version = "0.4.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "520f05a5cbd335fae5a99ff7a6ab8627577660ee5cfd6a94a6a929b52ff0321c"
+dependencies = [
+ "libc",
+ "option-ext",
+ "redox_users",
+ "windows-sys 0.48.0",
+]
+
 [[package]]
 name = "either"
 version = "1.10.0"
@@ -334,6 +361,17 @@ dependencies = [
  "libc",
 ]
 
+[[package]]
+name = "getrandom"
+version = "0.2.14"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "94b22e06ecb0110981051723910cbf0b5f5e09a2062dd7663334ee79a9d1286c"
+dependencies = [
+ "cfg-if",
+ "libc",
+ "wasi",
+]
+
 [[package]]
 name = "glob"
 version = "0.3.1"
@@ -497,16 +535,28 @@ dependencies = [
  "libc",
 ]
 
+[[package]]
+name = "libredox"
+version = "0.1.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "c0ff37bd590ca25063e35af745c343cb7a0271906fb7b37e4813e8f79f00268d"
+dependencies = [
+ "bitflags 2.4.2",
+ "libc",
+]
+
 [[package]]
 name = "libshpool"
 version = "0.6.2"
 dependencies = [
  "anyhow",
+ "assert_matches",
  "bincode",
  "byteorder",
  "chrono",
  "clap",
  "crossbeam-channel",
+ "directories",
  "lazy_static",
  "libc",
  "libproc",
@@ -698,6 +748,12 @@ version = "1.19.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "3fdb12b2476b595f9358c5161aa467c2438859caa136dec86c26fdd2efe17b92"
 
+[[package]]
+name = "option-ext"
+version = "0.2.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "04744f49eae99ab78e0d5c0b603ab218f515ea8cfe5a456d7629ad883a3b6e7d"
+
 [[package]]
 name = "pin-project-lite"
 version = "0.2.13"
@@ -746,6 +802,17 @@ dependencies = [
  "bitflags 1.3.2",
 ]
 
+[[package]]
+name = "redox_users"
+version = "0.4.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "bd283d9651eeda4b2a83a43c1c91b266c40fd76ecd39a50a8c630ae69dc72891"
+dependencies = [
+ "getrandom",
+ "libredox",
+ "thiserror",
+]
+
 [[package]]
 name = "regex"
 version = "1.10.3"
@@ -992,6 +1059,26 @@ dependencies = [
  "home",
 ]
 
+[[package]]
+name = "thiserror"
+version = "1.0.61"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "c546c80d6be4bc6a00c0f01730c08df82eaa7a7a61f11d656526506112cc1709"
+dependencies = [
+ "thiserror-impl",
+]
+
+[[package]]
+name = "thiserror-impl"
+version = "1.0.61"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "46c3384250002a6d5af4d114f2845d37b57521033f30d5c3f46c4d70e1197533"
+dependencies = [
+ "proc-macro2",
+ "quote",
+ "syn 2.0.52",
+]
+
 [[package]]
 name = "thread_local"
 version = "1.1.8"
diff --git a/README.md b/README.md
index d087aed1..7b9ae107 100644
--- a/README.md
+++ b/README.md
@@ -49,6 +49,11 @@ This README covers basic usage, but you can also check out
 [the wiki](https://github.com/shell-pool/shpool/wiki) for
 more tips and tricks.
 
+### [Troubleshooting](https://github.com/shell-pool/shpool/wiki/Troubleshooting)
+
+The [troubleshooting](https://github.com/shell-pool/shpool/wiki/Troubleshooting)
+wiki page contains some information about known pitfalls.
+
 ### [Configuration](./CONFIG.md)
 
 You can customize some of `shpool`s behavior by editing your
diff --git a/libshpool/Cargo.toml b/libshpool/Cargo.toml
index 473debbd..a19578df 100644
--- a/libshpool/Cargo.toml
+++ b/libshpool/Cargo.toml
@@ -42,6 +42,7 @@ tempfile = "3" # RAII tmp files
 strip-ansi-escapes = "0.2.0" # cleaning up strings for pager display
 notify = "6" # watch config file for updates
 libproc = "0.14.8" # sniffing shells by examining the subprocess
+directories = "5.0.1" # get paths for configurations
 
 # rusty wrapper for unix apis
 [dependencies.nix]
@@ -55,3 +56,4 @@ features = ["std", "fmt", "tracing-log", "smallvec"]
 
 [dev-dependencies]
 ntest = "0.9" # test timeouts
+assert_matches = "1.5" # assert_matches macro
diff --git a/libshpool/src/config.rs b/libshpool/src/config.rs
index 40de24aa..80fa37d7 100644
--- a/libshpool/src/config.rs
+++ b/libshpool/src/config.rs
@@ -13,18 +13,19 @@
 // limitations under the License.
 
 use std::{
+    borrow::Cow,
     collections::HashMap,
     fs,
-    path::{Path, PathBuf},
+    path::Path,
     sync::{Arc, RwLock, RwLockReadGuard},
 };
 
-use anyhow::Context;
-use notify::Watcher;
+use anyhow::{Context as _, Result};
+use directories::ProjectDirs;
 use serde_derive::Deserialize;
 use tracing::{info, warn};
 
-use super::{daemon::keybindings, user};
+use crate::{config_watcher::ConfigWatcher, daemon::keybindings, test_hooks};
 
 /// Exposes the shpool config file, watching for file updates
 /// so that the user does not need to restart the daemon when
@@ -33,90 +34,108 @@ use super::{daemon::keybindings, user};
 /// Users should never cache the config value directly and always
 /// access the config through the manager. The config may change
 /// at any time, so any cached config value could become stale.
+#[derive(Clone)]
 pub struct Manager {
     /// The config value.
     config: Arc<RwLock<Config>>,
-    watcher: Option<Arc<notify::RecommendedWatcher>>,
+    _watcher: Arc<ConfigWatcher>,
 }
 
 impl Manager {
-    // Create a new config manager.
-    pub fn new(config_file: Option<&str>) -> anyhow::Result<Self> {
-        let user_info = user::info()?;
-        let mut default_config_path = PathBuf::from(user_info.home_dir);
-
-        let (config, config_path) = if let Some(config_path) = config_file {
-            info!("parsing explicitly passed in config ({})", config_path);
-            let config_str = fs::read_to_string(config_path).context("reading config toml (1)")?;
-            let config = toml::from_str(&config_str).context("parsing config file (1)")?;
-
-            (config, Some(String::from(config_path)))
-        } else {
-            default_config_path.push(".config");
-            default_config_path.push("shpool");
-            default_config_path.push("config.toml");
-            if default_config_path.exists() {
-                let config_str =
-                    fs::read_to_string(&default_config_path).context("reading config toml (2)")?;
-                let config = toml::from_str(&config_str).context("parsing config file (2)")?;
-
-                (config, default_config_path.clone().to_str().map(String::from))
-            } else {
-                (Config::default(), None)
+    /// Create a new config manager.
+    ///
+    /// Unless given as the `config_file` argument, config files are read from
+    /// the following paths in the reverse priority order:
+    ///
+    /// - System level config: /etc/shpool/config.toml
+    /// - User level config: $XDG_CONFIG_HOME/shpool/config.toml or
+    ///   $HOME/.config/shpool/config.toml if $XDG_CONFIG_HOME is not set
+    ///
+    /// For each top level field, values read later will overrides those read
+    /// eariler. The exact merging strategy is as defined in
+    /// `Config::merge`.
+    pub fn new(config_file: Option<&str>) -> Result<Self> {
+        let dirs = ProjectDirs::from("", "", "shpool").context("getting ProjectDirs")?;
+
+        let config_files = match config_file {
+            None => {
+                vec![
+                    Cow::from(Path::new("/etc/shpool/config.toml")),
+                    Cow::from(dirs.config_dir().join("config.toml")),
+                ]
+            }
+            Some(config_file) => {
+                info!("parsing explicitly passed in config ({})", config_file);
+                vec![Cow::from(Path::new(config_file))]
             }
         };
-        info!("starting with config: {:?}", config);
 
-        let mut manager = Manager { config: Arc::new(RwLock::new(config)), watcher: None };
-
-        if let Some(watch_path) = config_path {
-            let config_slot = Arc::clone(&manager.config);
-            let reload_path = watch_path.clone();
-            let mut watcher = notify::recommended_watcher(move |res| match res {
-                Ok(event) => {
-                    info!("config file modify event: {:?}", event);
-
-                    let config_str = match fs::read_to_string(&reload_path) {
-                        Ok(s) => s,
-                        Err(e) => {
-                            warn!("error reading config file: {:?}", e);
-                            return;
-                        }
-                    };
-
-                    let config = match toml::from_str(&config_str) {
-                        Ok(c) => c,
-                        Err(e) => {
-                            warn!("error parsing config file: {:?}", e);
-                            return;
-                        }
-                    };
-                    info!("new config: {:?}", config);
-
-                    let mut manager_config = config_slot.write().unwrap();
-                    *manager_config = config;
+        let config = Self::load(&config_files).context("loading initial config")?;
+        info!("starting with config: {:?}", config);
+        let config = Arc::new(RwLock::new(config));
+
+        let watcher = {
+            let config = config.clone();
+            // create a owned version of config_files to move to the watcher thread.
+            let config_files: Vec<_> = config_files.iter().map(|f| f.to_path_buf()).collect();
+            ConfigWatcher::new(move || {
+                info!("reloading config");
+                let mut config = config.write().unwrap();
+                match Self::load(&config_files) {
+                    Ok(c) => {
+                        info!("new config: {:?}", c);
+                        *config = c;
+                    }
+                    Err(err) => warn!("error loading config file: {:?}", err),
                 }
-                Err(e) => warn!("config file watch err: {:?}", e),
+                test_hooks::emit("daemon-reload-config");
             })
-            .context("building watcher")?;
-            watcher
-                .watch(Path::new(&watch_path), notify::RecursiveMode::NonRecursive)
-                .context("registering config file for watching")?;
-            manager.watcher = Some(Arc::new(watcher));
+            .context("building watcher")?
+        };
+        for path in config_files {
+            watcher.watch(path).context("registering config file for watching")?;
         }
+        let manager = Manager { config, _watcher: Arc::new(watcher) };
 
         Ok(manager)
     }
 
-    // Get the current config value.
+    /// Get the current config value.
     pub fn get(&self) -> RwLockReadGuard<'_, Config> {
         self.config.read().unwrap()
     }
-}
 
-impl std::clone::Clone for Manager {
-    fn clone(&self) -> Self {
-        Manager { config: Arc::clone(&self.config), watcher: self.watcher.as_ref().map(Arc::clone) }
+    /// Load config by merging configurations from a list of Paths.
+    ///
+    /// Paths come later in the list takes higher priority.
+    /// Merge strategy is as defined in `Config::merge`.
+    fn load<T>(config_files: T) -> Result<Config>
+    where
+        T: IntoIterator,
+        T::Item: AsRef<Path>,
+    {
+        let mut config = Config::default();
+        for path in config_files {
+            let path = path.as_ref();
+            let config_str = match fs::read_to_string(path) {
+                Err(e) => {
+                    warn!("skip reading config file {}: {:?}", path.display(), e);
+                    continue;
+                }
+                Ok(s) => s,
+            };
+            let new_config: Config = match toml::from_str(&config_str) {
+                Err(e) => {
+                    warn!("error parsing config file: {:?}", e);
+                    return Err(e).with_context(|| {
+                        format!("parsing config toml {}", path.to_string_lossy())
+                    });
+                }
+                Ok(c) => c,
+            };
+            config = new_config.merge(config);
+        }
+        Ok(config)
     }
 }
 
@@ -217,6 +236,35 @@ pub struct Config {
     pub motd_args: Option<Vec<String>>,
 }
 
+impl Config {
+    // Merge with `another` Config instance, with `self` taking higher priority,
+    // i.e. it is not communicative.
+    //
+    // Top level options with simple value are directly taken from `self`.
+    // List or map fields may be handled differently. If so, it will be highlighted
+    // in that field's documentation.
+    pub fn merge(self, another: Config) -> Config {
+        Config {
+            norc: self.norc.or(another.norc),
+            noecho: self.noecho.or(another.noecho),
+            nosymlink_ssh_auth_sock: self
+                .nosymlink_ssh_auth_sock
+                .or(another.nosymlink_ssh_auth_sock),
+            noread_etc_environment: self.noread_etc_environment.or(another.noread_etc_environment),
+            shell: self.shell.or(another.shell),
+            env: self.env.or(another.env),
+            forward_env: self.forward_env.or(another.forward_env),
+            initial_path: self.initial_path.or(another.initial_path),
+            session_restore_mode: self.session_restore_mode.or(another.session_restore_mode),
+            output_spool_lines: self.output_spool_lines.or(another.output_spool_lines),
+            keybinding: self.keybinding.or(another.keybinding),
+            prompt_prefix: self.prompt_prefix.or(another.prompt_prefix),
+            motd: self.motd.or(another.motd),
+            motd_args: self.motd_args.or(another.motd_args),
+        }
+    }
+}
+
 #[derive(Deserialize, Debug, Clone)]
 pub struct Keybinding {
     /// The keybinding to map to an action. The syntax for these keybindings
@@ -277,7 +325,7 @@ mod test {
 
     #[test]
     #[timeout(30000)]
-    fn parse() -> anyhow::Result<()> {
+    fn parse() -> Result<()> {
         let cases = vec![
             r#"
             session_restore_mode = "simple"
@@ -301,4 +349,88 @@ mod test {
 
         Ok(())
     }
+
+    mod merge {
+        use super::*;
+        use assert_matches::assert_matches;
+
+        #[test]
+        #[timeout(30000)]
+        fn simple_value() -> Result<()> {
+            // 4 values are chosen to cover all combinations of None and Some cases.
+            let higher = Config {
+                norc: None,
+                noecho: None,
+                shell: Some("abc".to_string()),
+                session_restore_mode: Some(SessionRestoreMode::Simple),
+                ..Default::default()
+            };
+            let lower = Config {
+                norc: Some(true),
+                noecho: None,
+                shell: None,
+                session_restore_mode: Some(SessionRestoreMode::Lines(42)),
+                ..Default::default()
+            };
+
+            assert_matches!(higher.merge(lower), Config {
+                norc: Some(true),
+                noecho: None,
+                shell: Some(shell),
+                session_restore_mode: Some(SessionRestoreMode::Simple),
+                ..
+            } if shell == "abc");
+            Ok(())
+        }
+
+        #[test]
+        #[timeout(30000)]
+        fn vec_value() -> Result<()> {
+            let higher = Config {
+                forward_env: Some(vec!["abc".to_string(), "efg".to_string()]),
+                motd_args: None,
+                ..Default::default()
+            };
+            let lower = Config {
+                forward_env: None,
+                motd_args: Some(vec!["hij".to_string(), "klm".to_string()]),
+                ..Default::default()
+            };
+
+            let actual = higher.merge(lower);
+            assert_eq!(actual.forward_env, Some(vec!["abc".to_string(), "efg".to_string()]));
+            assert_eq!(actual.motd_args, Some(vec!["hij".to_string(), "klm".to_string()]));
+            Ok(())
+        }
+
+        #[test]
+        #[timeout(30000)]
+        fn map_value() -> Result<()> {
+            let higher = Config {
+                env: Some(HashMap::from([
+                    ("key1".to_string(), "value1".to_string()),
+                    ("key2".to_string(), "value2".to_string()),
+                ])),
+                ..Default::default()
+            };
+            let lower = Config {
+                env: Some(HashMap::from([
+                    ("key3".to_string(), "value3".to_string()),
+                    ("key4".to_string(), "value4".to_string()),
+                ])),
+                ..Default::default()
+            };
+
+            let actual = higher.merge(lower);
+
+            assert_eq!(
+                actual.env,
+                Some(HashMap::from([
+                    ("key1".to_string(), "value1".to_string()),
+                    ("key2".to_string(), "value2".to_string()),
+                ]))
+            );
+            Ok(())
+        }
+    }
 }
diff --git a/libshpool/src/config_watcher.rs b/libshpool/src/config_watcher.rs
new file mode 100644
index 00000000..ee99b9bf
--- /dev/null
+++ b/libshpool/src/config_watcher.rs
@@ -0,0 +1,735 @@
+// Copyright 2024 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use anyhow::{anyhow, Context as _, Result};
+use crossbeam_channel::{bounded, select, unbounded, Receiver, Sender};
+use notify::{
+    event::ModifyKind, recommended_watcher, Event, EventKind, INotifyWatcher, RecursiveMode,
+    Watcher as _,
+};
+use std::{
+    collections::{hash_map::Entry, HashMap},
+    path::{Path, PathBuf},
+    thread::{self, JoinHandle},
+    time::{Duration, Instant},
+};
+use tracing::{debug, error, warn};
+
+/// Watches on `path`, returnes the watched path, which is the closest existing
+/// ancestor of `path`, and the immediate child that is of interest.
+pub fn best_effort_watch<'a>(
+    watcher: &mut INotifyWatcher,
+    path: &'a Path,
+) -> Result<(&'a Path, Option<&'a Path>)> {
+    let mut watched_path = Err(anyhow!("empty path"));
+    // Ok or last Err
+    for watch_path in path.ancestors() {
+        match watcher.watch(watch_path, RecursiveMode::NonRecursive) {
+            Ok(_) => {
+                watched_path = Ok(watch_path);
+                break;
+            }
+            Err(err) => watched_path = Err(err.into()),
+        }
+    }
+    // watched path could be any ancestor of the original path
+    let watched_path = watched_path.context("adding notify watch for config file")?;
+    let remaining_path = path
+        .strip_prefix(watched_path)
+        .expect("watched_path was obtained as an ancestor of path, yet it is not a prefix");
+    let immediate_child = remaining_path.iter().next();
+    debug!("Actually watching {}, ic {:?}", watched_path.display(), &immediate_child);
+    Ok((watched_path, immediate_child.map(Path::new)))
+}
+
+// Note that you can't add doctest for private items.
+// See https://stackoverflow.com/a/76289746
+
+/// Notify watcher to detect config file changes.
+///
+/// Notable features:
+/// - handles non-existing config files
+/// - support watching multiple files
+/// - configurable debounce time for reload
+///
+/// For simplicity, reload doesn't distinguish which file was changed. It is
+/// expected that all config files need to be reload regardless which one
+/// changed.
+///
+/// # Examples
+/// ```ignore
+/// use crate::config_watcher::ConfigWatcher;
+///
+/// let watcher = ConfigWatcher::new(|| println!("RELOAD CONFIG")).unwrap();
+/// watcher.watch("/some/path/config.toml");
+/// ````
+pub struct ConfigWatcher {
+    /// For sending watch requests
+    tx: Sender<Command>,
+
+    /// Handle to worker thread
+    #[allow(unused)]
+    worker: JoinHandle<()>,
+
+    /// For receiving debug info from worker thread, test only
+    #[cfg(test)]
+    debug_rx: Receiver<()>,
+}
+
+impl ConfigWatcher {
+    /// Creates a new [`ConfigWatcher`] with default debounce time 100ms.
+    ///
+    /// Event processing happens in another thread, so the passed in `handler`
+    /// is expected to properly handle synchronization and locking.
+    ///
+    /// # Errors
+    /// Returns error if the creation of underlying `notify` watcher or worker
+    /// thread failed.
+    pub fn new(handler: impl FnMut() + Send + 'static) -> Result<Self> {
+        Self::with_debounce(handler, Duration::from_millis(100))
+    }
+
+    /// Creates a new [`ConfigWatcher`] with default debounce time
+    /// `reload_debounce`.
+    ///
+    /// Event processing happens in another thread, so the passed in `handler`
+    /// is expected to properly handle synchronization and locking.
+    ///
+    /// # Arguments
+    /// * `handler` - The handler is called when the watcher determines there is
+    ///   a need to reload config files
+    /// * `reload_debounce` - Reloads happen within `reload_debounce` time will
+    ///   only trigger the handler once
+    ///
+    /// # Errors
+    /// Returns error if the creation of underlying `notify` watcher or worker
+    /// thread failed.
+    pub fn with_debounce(
+        handler: impl FnMut() + Send + 'static,
+        reload_debounce: Duration,
+    ) -> Result<Self> {
+        let (notify_tx, notify_rx) = unbounded();
+        let (req_tx, req_rx) = unbounded();
+
+        #[cfg(test)]
+        let (debug_tx, debug_rx) = unbounded();
+
+        let watcher = recommended_watcher(notify_tx).context("create notify watcher")?;
+
+        let mut inner = ConfigWatcherInner {
+            reload_debounce,
+            reload_deadline: None,
+            handler,
+            watcher,
+            notify_rx,
+            req_rx,
+            #[cfg(test)]
+            debug_tx,
+            paths: Default::default(),
+        };
+        let worker = thread::Builder::new()
+            .name("config-reload".to_string())
+            .spawn(move || {
+                if let Err(err) = inner.run() {
+                    error!("config reload thread returned error: {:?}", err);
+                }
+            })
+            .context("create config reload thread")?;
+
+        Ok(Self {
+            tx: req_tx,
+            worker,
+            #[cfg(test)]
+            debug_rx,
+        })
+    }
+
+    /// Adds a watch on `path`.
+    ///
+    /// # Errors
+    /// Returns error if the underlying thread is gone, e.g. the worker thread
+    /// encountered fatal error and stopped its event loop.
+    pub fn watch(&self, path: impl AsRef<Path>) -> Result<()> {
+        let (tx, rx) = bounded(1);
+        self.tx
+            .send(Command::AddWatch(path.as_ref().to_owned(), tx))
+            .context("sending AddWatch to ConfigWatcherInner")?;
+        rx.recv()?
+    }
+
+    /// Worker is idle and ready for the next event. Debug/test only.
+    #[cfg(test)]
+    fn worker_ready(&self) {
+        self.debug_rx.recv().unwrap();
+        debug!("worker ready");
+    }
+}
+
+impl Drop for ConfigWatcher {
+    /// Stop watching, shutting down the worker thread.
+    fn drop(&mut self) {
+        if let Err(err) = self.tx.send(Command::Shutdown) {
+            warn!("Config watcher thread already died: {:?}", err);
+        }
+    }
+}
+
+/// Messages sent from `ConfigWatcher` in `ConfigWatcherInner`
+enum Command {
+    AddWatch(PathBuf, Sender<Result<()>>),
+    Shutdown,
+}
+
+struct ConfigWatcherInner<Handler> {
+    /// time to wait before actual reloading
+    reload_debounce: Duration,
+    /// deadline to do a reload
+    reload_deadline: Option<Instant>,
+
+    /// handle is called to signify the need to reload configs
+    handler: Handler,
+
+    /// underlying notify-rs watcher
+    watcher: INotifyWatcher,
+    /// receiving notify events
+    notify_rx: Receiver<Result<notify::Event, notify::Error>>,
+
+    /// receiving watch requests from the outer `ConfigWatcher`
+    req_rx: Receiver<Command>,
+    /// Current watching status, it is a map from target_path to (watched_path,
+    /// immediate_child_path)
+    paths: HashMap<PathBuf, (PathBuf, PathBuf)>,
+
+    /// for sending out debug info
+    #[cfg(test)]
+    debug_tx: Sender<()>,
+}
+
+/// Outcomes of selecting channels in the worker thread
+enum Outcome {
+    /// A notify event occurred
+    Event(notify::Result<notify::Event>),
+    /// A control command from outside
+    AddWatch(PathBuf, Sender<Result<()>>),
+    /// Timeout on notify event, trigger reload
+    Timeout,
+    /// Any channel was disconnected, or a explicit shutdown was requested
+    Shutdown,
+}
+
+impl From<Command> for Outcome {
+    fn from(value: Command) -> Self {
+        match value {
+            Command::AddWatch(path, sender) => Self::AddWatch(path, sender),
+            Command::Shutdown => Self::Shutdown,
+        }
+    }
+}
+
+impl From<notify::Result<notify::Event>> for Outcome {
+    fn from(value: notify::Result<notify::Event>) -> Self {
+        Self::Event(value)
+    }
+}
+
+impl<Handler> ConfigWatcherInner<Handler> {
+    /// get next event to work on
+    fn select(&self) -> Outcome {
+        debug!("now {:?} select with ddl {:?}", Instant::now(), &self.reload_deadline);
+
+        // only impose a deadline if there is pending reload
+        let timeout = self
+            .reload_deadline
+            .map(crossbeam_channel::at)
+            .unwrap_or_else(crossbeam_channel::never);
+
+        #[cfg(test)]
+        {
+            // first try non-blocking recv, to give us a chance to to notify debug_tx about
+            // we are about to go into blocking wait.
+            if let Ok(res) = self.notify_rx.try_recv() {
+                return Outcome::from(res);
+            }
+            if let Ok(res) = self.req_rx.try_recv() {
+                return Outcome::from(res);
+            }
+            if timeout.try_recv().is_ok() {
+                return Outcome::Timeout;
+            }
+
+            // nothing ready to act immediately, notify debug_tx
+            self.debug_tx.send(()).unwrap();
+        }
+
+        // finally blocking wait
+        select! {
+            recv(self.notify_rx) -> res => res.map(Outcome::from).unwrap_or(Outcome::Shutdown),
+            recv(self.req_rx) -> res => res.map(Outcome::from).unwrap_or(Outcome::Shutdown),
+            recv(timeout) -> _ => Outcome::Timeout,
+        }
+    }
+
+    /// Schedule a reload later.
+    ///
+    /// If there is already a pending deadline, it is kept as is, such that
+    /// multiple reloads within `self.reload_debounce` duration only result
+    /// in one actual reload. Otherwise, set the reload deadline to be
+    /// `Instant::now() + self.reload_debounce`.
+    fn trigger_reload(&mut self) {
+        self.reload_deadline =
+            self.reload_deadline.or_else(|| Some(Instant::now() + self.reload_debounce));
+        debug!("defer config reloading to {:?}!", &self.reload_deadline);
+    }
+
+    /// Handle add watch command from `ConfigWatcher`.
+    fn add_watch_by_command(&mut self, path: PathBuf) -> Result<()> {
+        match self.paths.entry(path) {
+            Entry::Occupied(e) => Err(anyhow!("{} is already being watched", e.key().display())),
+            e @ Entry::Vacant(_) => {
+                let reload = watch_and_add(&mut self.watcher, e)?;
+                if reload {
+                    self.trigger_reload();
+                }
+                Ok(())
+            }
+        }
+    }
+
+    /// Do rewatch according to the enum, return whether reload is necessary
+    fn rewatch(&mut self, rewatch: ReWatch) -> bool {
+        let rewatch_paths = match rewatch {
+            ReWatch::Some(rewatch_paths) => rewatch_paths,
+            ReWatch::All => {
+                // drain paths and collect into vec first, to avoid keeping a mutable borrow on
+                // self.paths
+                self.paths.drain().map(|(path, (watched_path, _))| (path, watched_path)).collect()
+            }
+        };
+        rewatch_paths
+            .into_iter()
+            .map(|(path, watched_path)| {
+                if let Err(err) = self.watcher.unwatch(&watched_path) {
+                    // error sometimes is expected if the watched_path was simply removed, in that
+                    // case notify will automatically remove the watch.
+                    error!("error unwatch {:?}", err);
+                } else {
+                    debug!("unwatched {}", watched_path.display());
+                }
+                watch_and_add(&mut self.watcher, self.paths.entry(path))
+                    .map_err(|err| error!("Failed to add watch: {:?}", err))
+                    .unwrap_or(true)
+            })
+            .any(|reload| reload)
+    }
+}
+
+impl<Handler> ConfigWatcherInner<Handler>
+where
+    Handler: FnMut(),
+{
+    /// Loop to reload config, only return when there is error to create any
+    /// watches.
+    fn run(&mut self) -> Result<()> {
+        loop {
+            match self.select() {
+                Outcome::Event(res) => {
+                    let (rewatch, mut reload) = match res {
+                        Err(error) => {
+                            error!("Error: {error:?}");
+                            (ReWatch::All, false)
+                        }
+                        Ok(event) => handle_event(event, &self.paths),
+                    };
+                    debug!("rewatch = {rewatch:?}, reload = {reload}");
+                    reload |= self.rewatch(rewatch);
+                    if reload {
+                        self.trigger_reload();
+                    }
+                }
+                Outcome::AddWatch(path, sender) => {
+                    let _ = sender.send(self.add_watch_by_command(path));
+                }
+                Outcome::Timeout => {
+                    debug!("reloading config now!");
+                    self.reload_deadline = None;
+                    (self.handler)();
+                }
+                Outcome::Shutdown => {
+                    debug!("stopping config watcher thread");
+                    break;
+                }
+            }
+        }
+        Ok(())
+    }
+}
+
+#[derive(Debug, PartialEq, Eq)]
+enum ReWatch {
+    /// rewatch a few (target path, watched path)
+    Some(Vec<(PathBuf, PathBuf)>),
+    /// rewatch all paths
+    All,
+}
+
+/// Return wether need to rewatch, and whether need to reload
+fn handle_event(event: Event, paths: &HashMap<PathBuf, (PathBuf, PathBuf)>) -> (ReWatch, bool) {
+    if event.need_rescan() {
+        debug!("need rescan");
+        return (ReWatch::All, true);
+    }
+
+    // this event is about one of the watched target
+    let is_original = event.paths.iter().any(|p| paths.contains_key(p));
+
+    match event.kind {
+        // create/remove in any segment in path
+        EventKind::Remove(_) | EventKind::Create(_) | EventKind::Modify(ModifyKind::Name(_)) => {
+            debug!("create/remove: {:?}", event);
+            // find all path entries about this event
+            let rewatch = paths
+                .iter()
+                .filter(|(_, (watched_path, immediate_child_path))| {
+                    event.paths.iter().any(|p| p == watched_path || p == immediate_child_path)
+                })
+                .map(|(path, (watched_path, _))| (path.to_owned(), watched_path.to_owned()))
+                .collect();
+            (ReWatch::Some(rewatch), is_original)
+        }
+        // modification in any segment in path
+        EventKind::Modify(_) => {
+            debug!("modify: {:?}", event);
+            (ReWatch::Some(vec![]), is_original)
+        }
+        _ => {
+            debug!("ignore {:?}", event);
+
+            (ReWatch::Some(vec![]), false)
+        }
+    }
+}
+
+/// Add a watch at `path`, update paths `entry` if success, or remove `entry` if
+/// failed. Note that this will overwrite any existing state.
+/// Return whether reload is needed.
+fn watch_and_add(
+    watcher: &mut INotifyWatcher,
+    entry: Entry<PathBuf, (PathBuf, PathBuf)>,
+) -> Result<bool> {
+    // make a version of watch path that doesn't retain a borrow in its return value
+    let best_effort_watch_owned = |watcher: &mut INotifyWatcher, path: &Path| {
+        best_effort_watch(watcher, path)
+            .map(|(w, ic)| (w.to_owned(), w.join(ic.unwrap_or_else(|| Path::new("")))))
+    };
+    match best_effort_watch_owned(watcher, entry.key()) {
+        Ok((watched_path, immediate_child_path)) => {
+            let reload = &watched_path == entry.key();
+            // update entry after `match watch_a_path(...)`, as that takes a borrow on entry
+            // (entry.key())
+            match entry {
+                Entry::Occupied(mut entry) => {
+                    entry.insert((watched_path, immediate_child_path));
+                }
+                Entry::Vacant(entry) => {
+                    entry.insert((watched_path, immediate_child_path));
+                }
+            }
+            if reload {
+                debug!("Force reload since now watching on target file");
+            }
+            Ok(reload)
+        }
+        Err(err) => {
+            let context_msg = format!("best_effort_watch on {}", entry.key().display());
+            if let Entry::Occupied(entry) = entry {
+                entry.remove();
+            }
+            Err(err).context(context_msg)
+        }
+    }
+}
+
+#[cfg(test)]
+#[rustfmt::skip::attributes(test_case)]
+mod test {
+    use super::*;
+    use ntest::timeout;
+    use std::fs;
+    use tempfile::TempDir;
+
+    mod watch {
+        use super::*;
+        use std::fs;
+
+        #[test]
+        #[timeout(30000)]
+        fn all_non_existing() {
+            let mut watcher = recommended_watcher(|_| {}).unwrap();
+
+            let (watched_path, immediate_child) =
+                best_effort_watch(&mut watcher, Path::new("/non_existing/subdir")).unwrap();
+
+            assert_eq!(watched_path, Path::new("/"));
+            assert_eq!(immediate_child, Some(Path::new("non_existing")));
+        }
+
+        #[test]
+        #[timeout(30000)]
+        fn non_existing_parent() {
+            let tmpdir = tempfile::tempdir().unwrap();
+            let target_path = tmpdir.path().join(Path::new("sub1/sub2/c.txt"));
+
+            let parent_path = target_path.parent().unwrap().parent().unwrap();
+
+            fs::create_dir_all(parent_path).unwrap();
+
+            let mut watcher = recommended_watcher(|_| {}).unwrap();
+            let (watched_path, immediate_child) =
+                best_effort_watch(&mut watcher, &target_path).unwrap();
+
+            assert_eq!(watched_path, parent_path);
+            assert_eq!(immediate_child, Some(Path::new("sub2")));
+        }
+
+        #[test]
+        #[timeout(30000)]
+        fn existing_file() {
+            let tmpdir = tempfile::tempdir().unwrap();
+            let target_path = tmpdir.path().join(Path::new("sub1/sub2/c.txt"));
+
+            let parent_path = target_path.parent().unwrap();
+
+            fs::create_dir_all(parent_path).unwrap();
+            fs::write(&target_path, "test").unwrap();
+
+            let mut watcher = recommended_watcher(|_| {}).unwrap();
+            let (watched_path, immediate_child) =
+                best_effort_watch(&mut watcher, &target_path).unwrap();
+
+            assert_eq!(watched_path, target_path);
+            assert_eq!(immediate_child, None);
+        }
+    }
+
+    mod handle_event {
+        use super::*;
+        use assert_matches::assert_matches;
+        use notify::{
+            event::{CreateKind, ModifyKind, RemoveKind, RenameMode},
+            Event, EventKind,
+        };
+        use ntest::test_case;
+
+        fn paths_entry(target: &str, watched: &str) -> (PathBuf, (PathBuf, PathBuf)) {
+            let target = PathBuf::from(target);
+            let base = PathBuf::from(watched);
+            let immediate =
+                base.join(target.strip_prefix(&base).unwrap().iter().next().unwrap_or_default());
+            (target, (base, immediate))
+        }
+
+        // create event from spec:
+        // <create|mv|modify|rm> path1 [path2]
+        // `base` is prepended to all paths
+        fn event_from_spec(base: &str, evt: &str) -> notify::Event {
+            let base = Path::new(base);
+            let (evt, path) = evt.split_once(' ').unwrap_or((evt, ""));
+            match evt {
+                "create" => {
+                    Event::new(EventKind::Create(CreateKind::Any)).add_path(base.join(path))
+                }
+                "mv" => {
+                    let (src, dst) = path.split_once(' ').unwrap();
+                    Event::new(EventKind::Modify(ModifyKind::Name(RenameMode::Both)))
+                        .add_path(base.join(src))
+                        .add_path(base.join(dst))
+                }
+                "mvselfother" => Event::new(EventKind::Modify(ModifyKind::Name(RenameMode::Both)))
+                    .add_path(base.to_owned())
+                    .add_path(PathBuf::from("/some/other/path")),
+                "modify" => {
+                    Event::new(EventKind::Modify(ModifyKind::Any)).add_path(base.join(path))
+                }
+                "modifyself" => {
+                    Event::new(EventKind::Modify(ModifyKind::Any)).add_path(base.to_owned())
+                }
+                "rm" => Event::new(EventKind::Remove(RemoveKind::Any)).add_path(base.join(path)),
+                "rmself" => {
+                    Event::new(EventKind::Remove(RemoveKind::Any)).add_path(base.to_owned())
+                }
+                _ => panic!("malformatted event spec"),
+            }
+        }
+
+        #[test]
+        #[timeout(30000)]
+        fn need_rescan() {
+            let event = notify::Event::default().set_flag(notify::event::Flag::Rescan);
+            let paths = Default::default();
+            let (rewatch, reload) = handle_event(event, &paths);
+            assert_eq!(rewatch, ReWatch::All);
+            assert!(reload);
+        }
+
+        const TARGET: &str = "/base/sub/config.toml";
+
+        #[test_case(TARGET, "/base", "create sub", true, false, name = "base_create_sub")]
+        #[test_case(TARGET, "/base", "create other", false, false, name = "base_create_other")]
+        #[test_case(TARGET, "/base", "mv other sub", true, false, name = "base_other_to_sub")]
+        #[test_case(TARGET, "/base", "mv other another", false, false, name = "base_other_to_another")]
+        #[test_case(TARGET, "/base", "mv sub other", true, false, name = "base_sub_to_other")]
+        #[test_case(TARGET, "/base", "rm sub", true, false, name = "base_rm_sub")]
+        #[test_case(TARGET, "/base", "rm other", false, false, name = "base_rm_other")]
+        #[test_case(TARGET, "/base", "modify other.toml", false, false, name = "base_modify_other")]
+        #[test_case(TARGET, "/base/sub", "create config.toml", true, true, name = "sub_create_cfg")]
+        #[test_case(TARGET, "/base/sub", "mv other.toml config.toml", true, true, name = "sub_other_to_cfg")]
+        #[test_case(TARGET, "/base/sub", "mv other.toml another.toml", false, false, name = "sub_other_to_another")]
+        #[test_case(TARGET, "/base/sub", "modify config.toml", false, true, name = "sub_modify_cfg")]
+        #[test_case(TARGET, "/base/sub", "modify other.toml", false, false, name = "sub_modify_other")]
+        #[test_case(TARGET, "/base/sub", "rmself", true, false, name = "sub_rm_self")]
+        #[test_case(TARGET, "/base/sub/config.toml", "rmself", true, true, name = "cfg_rm_self")]
+        #[test_case(TARGET, "/base/sub/config.toml", "mvselfother", true, true, name = "cfg_self_to_other")]
+        #[test_case(TARGET, "/base/sub/config.toml", "modifyself", false, true, name = "cfg_modify_self")]
+        #[timeout(30000)]
+        fn single_path(
+            target: &str,
+            watched: &str,
+            evt: &str,
+            expected_rewatch: bool,
+            expected_reload: bool,
+        ) {
+            let paths = HashMap::from([paths_entry(target, watched)]);
+            let event = event_from_spec(watched, evt);
+
+            let (rewatch, reload) = handle_event(event, &paths);
+
+            let expected_rewatch = if expected_rewatch {
+                ReWatch::Some(vec![(PathBuf::from(target), PathBuf::from(watched))])
+            } else {
+                ReWatch::Some(vec![])
+            };
+            assert_eq!(rewatch, expected_rewatch);
+            assert_eq!(reload, expected_reload);
+        }
+
+        #[test]
+        #[timeout(30000)]
+        fn both_paths_are_updated() {
+            let paths = HashMap::from([
+                paths_entry("/base/sub/config.toml", "/base"),
+                paths_entry("/base/other/another.toml", "/base"),
+            ]);
+            let event = event_from_spec("/base", "rm /base");
+
+            let (rewatch, reload) = handle_event(event, &paths);
+
+            assert_matches!(rewatch, ReWatch::Some(p) if p.len() == 2);
+            assert!(!reload);
+        }
+    }
+
+    // Smaller debounce time for faster testing
+    const DEBOUNCE_TIME: Duration = Duration::from_millis(50);
+
+    struct WatcherState {
+        #[allow(dead_code)]
+        tmpdir: TempDir,
+        base_path: PathBuf,
+        target_path: PathBuf,
+        rx: Receiver<()>,
+        watcher: ConfigWatcher,
+    }
+
+    // Setup file structure at <tmpdir>/`base`, configure watcher to watch
+    // <tmpdir>/`base`/`target`
+    fn setup(base: &str, target: &str) -> Result<WatcherState> {
+        let tmpdir = tempfile::tempdir()?;
+        let base_path = tmpdir.path().join(base);
+        let target_path = base_path.join(target);
+        assert!(target_path.strip_prefix(&base_path).is_ok());
+
+        fs::create_dir_all(&base_path)?;
+
+        let (tx, rx) = unbounded();
+        let watcher = ConfigWatcher::with_debounce(move || tx.send(()).unwrap(), DEBOUNCE_TIME)?;
+        watcher.watch(&target_path)?;
+
+        Ok(WatcherState { tmpdir, base_path, target_path, rx, watcher })
+    }
+
+    // Wait for watcher to do its work and drop the watcher to close the channel
+    fn drop_watcher(watcher: ConfigWatcher) {
+        // sleep time larger than 1 debounce time
+        thread::sleep(DEBOUNCE_TIME * 2);
+        watcher.worker_ready();
+    }
+
+    #[test]
+    #[timeout(30000)]
+    fn debounce() {
+        let state = setup("base", "sub/config.toml").unwrap();
+
+        fs::create_dir_all(state.target_path.parent().unwrap()).unwrap();
+
+        state.watcher.worker_ready();
+        fs::write(&state.target_path, "test").unwrap();
+
+        state.watcher.worker_ready();
+        fs::write(&state.target_path, "another").unwrap();
+
+        drop_watcher(state.watcher);
+
+        let reloads: Vec<_> = state.rx.into_iter().collect();
+        assert_eq!(reloads.len(), 1);
+    }
+
+    #[test]
+    #[timeout(30000)]
+    fn writes_larger_than_debounce() {
+        let state = setup("base", "sub/config.toml").unwrap();
+
+        fs::create_dir_all(state.target_path.parent().unwrap()).unwrap();
+        state.watcher.worker_ready();
+        fs::write(&state.target_path, "test").unwrap();
+
+        thread::sleep(DEBOUNCE_TIME * 2);
+        state.watcher.worker_ready();
+        fs::write(&state.target_path, "another").unwrap();
+
+        drop_watcher(state.watcher);
+
+        let reloads: Vec<_> = state.rx.into_iter().collect();
+        assert_eq!(reloads.len(), 2);
+    }
+
+    // /base, mv /base/other (with config.toml) /base/sub (with config.toml) =>
+    // rewatch, reload
+    #[test]
+    #[timeout(30000)]
+    fn move_multiple_levels_in_place() {
+        let state = setup("base", "sub/config.toml").unwrap();
+
+        // create /base/other/config.toml
+        fs::create_dir_all(state.base_path.join("other")).unwrap();
+        fs::write(state.base_path.join("other/config.toml"), "test").unwrap();
+
+        // mv /base/other /base/sub
+        fs::rename(state.base_path.join("other"), state.base_path.join("sub")).unwrap();
+
+        drop_watcher(state.watcher);
+
+        let reloads: Vec<_> = state.rx.into_iter().collect();
+        assert_eq!(reloads.len(), 1);
+    }
+}
diff --git a/libshpool/src/lib.rs b/libshpool/src/lib.rs
index c0e4401b..1f219806 100644
--- a/libshpool/src/lib.rs
+++ b/libshpool/src/lib.rs
@@ -30,6 +30,7 @@ use tracing_subscriber::fmt::format::FmtSpan;
 mod attach;
 mod common;
 mod config;
+mod config_watcher;
 mod consts;
 mod daemon;
 mod detach;


Test patch:
diff --git a/shpool/tests/attach.rs b/shpool/tests/attach.rs
index 4ec70f84..2237ec24 100644
--- a/shpool/tests/attach.rs
+++ b/shpool/tests/attach.rs
@@ -1301,10 +1301,17 @@ fn dynamic_config_change() -> anyhow::Result<()> {
         attach_proc.run_cmd("echo $CHANGING_VAR")?;
         line_matcher.scan_until_re("REPLACE_ME$")?;
 
-        // change the config contents on the fly
+        // Create waiter right before changing config file, since reload can also happen
+        // right after daemon startup.
+        let mut waiter = daemon_proc.events.take().unwrap().waiter(["daemon-reload-config"]);
+
+        // Change the config contents on the fly
         let config_contents = config_tmpl.replace("REPLACE_ME", "NEW_VALUE");
         fs::write(&config_file, config_contents)?;
 
+        // Wait for reload to happen since there is debounce time
+        waiter.wait_event("daemon-reload-config")?;
+
         // When we spawn a new session, it should pick up the new value
         let mut attach_proc =
             daemon_proc.attach("sh2", Default::default()).context("starting attach proc")?;
diff --git a/shpool/tests/support/daemon.rs b/shpool/tests/support/daemon.rs
index aec3aab4..42085d2f 100644
--- a/shpool/tests/support/daemon.rs
+++ b/shpool/tests/support/daemon.rs
@@ -278,10 +278,12 @@ impl Proc {
             .arg("--socket")
             .arg(&self.socket_path)
             .env_clear()
-            .env("XDG_RUNTIME_DIR", env::var("XDG_RUNTIME_DIR")?)
             .env("SHPOOL_TEST_HOOK_SOCKET_PATH", &test_hook_socket_path)
             .envs(args.extra_env)
             .arg("attach");
+        if let Ok(xdg_runtime_dir) = env::var("XDG_RUNTIME_DIR") {
+            cmd.env("XDG_RUNTIME_DIR", xdg_runtime_dir);
+        }
         if args.force {
             cmd.arg("-f");
         }