fix(skillfs): prevent FUSE test mount leaks

src/skillfs/crates/skillfs-cli/tests/cli_startup_tests.rs

@@ -14,12 +14,64 @@
 //! wiring in `cmd_mount`.
 
 use std::path::Path;
-use std::process::Command;
+use std::process::{Child, Command};
+use std::time::Duration;
 
 fn bin_path() -> &'static str {
     env!("CARGO_BIN_EXE_skillfs")
 }
 
+/// True when `path` is a live mountpoint per `/proc/mounts`. Authoritative
+/// even for a dead FUSE endpoint (where `metadata()` would misbehave).
+fn is_mounted(path: &Path) -> bool {
+    let Ok(mounts) = std::fs::read_to_string("/proc/mounts") else {
+        return false;
+    };
+    let target = path.to_string_lossy();
+    mounts
+        .lines()
+        .any(|line| line.split_whitespace().nth(1) == Some(&*target))
+}
+
+/// Bounded, best-effort force unmount: `fusermount3 -u`, then lazy `-z`, then
+/// `umount -l`, retried until the path leaves `/proc/mounts` or the budget is
+/// exhausted. Never panics.
+fn force_unmount(path: &Path) {
+    for _ in 0..50 {
+        if !is_mounted(path) {
+            return;
+        }
+        let mp = path.to_string_lossy();
+        let _ = Command::new("fusermount3").args(["-u", &mp]).output();
+        let _ = Command::new("fusermount3").args(["-u", "-z", &mp]).output();
+        let _ = Command::new("umount").args(["-l", &mp]).output();
+        std::thread::sleep(Duration::from_millis(100));
+    }
+    if is_mounted(path) {
+        eprintln!("WARN: leaked SkillFS FUSE mount at {}", path.display());
+    }
+}
+
+/// Stop a spawned `skillfs mount` child without leaking its FUSE mount.
+///
+/// `child.kill()` sends SIGKILL, which the binary cannot catch, so the FUSE
+/// endpoint would survive under the (possibly workspace-rooted) mountpoint.
+/// Instead send SIGTERM — the mount command unmounts cleanly on SIGTERM —
+/// wait a bounded time for graceful exit, then force-unmount as a fallback
+/// and SIGKILL to guarantee the child is reap-able by the caller.
+fn stop_mount_child(child: &mut Child, mountpoint: &Path) {
+    let pid = child.id().to_string();
+    let _ = Command::new("kill").args(["-TERM", &pid]).status();
+    for _ in 0..50 {
+        if matches!(child.try_wait(), Ok(Some(_))) {
+            break;
+        }
+        std::thread::sleep(Duration::from_millis(100));
+    }
+    force_unmount(mountpoint);
+    let _ = child.kill();
+}
+
 fn empty_source() -> tempfile::TempDir {
     let dir = tempfile::tempdir().expect("source tempdir");
     // Make the source path a real directory so the "Source directory
@@ -695,18 +747,20 @@ fn unique_leaf(prefix: &str) -> String {
     format!("{prefix}-{}-{}", std::process::id(), nanos)
 }
 
-/// Spawn the binary, let startup run briefly, then kill it and return the
-/// combined stdout+stderr. Used for configs that pass the new gate and
-/// would otherwise block on the FUSE mount.
-fn run_briefly(args: &[&str]) -> String {
+/// Spawn the binary, let startup run briefly, then stop it cleanly and return
+/// the combined stdout+stderr. Used for configs that pass the new gate and
+/// would otherwise block on the FUSE mount. `mountpoint` must match the mount
+/// path passed in `args` so the FUSE mount is always torn down (never leaked
+/// under the workspace).
+fn run_briefly(mountpoint: &Path, args: &[&str]) -> String {
     let mut child = Command::new(bin_path())
         .args(args)
         .stderr(std::process::Stdio::piped())
         .stdout(std::process::Stdio::piped())
         .spawn()
         .expect("spawn skillfs");
-    std::thread::sleep(std::time::Duration::from_secs(1));
-    let _ = child.kill();
+    std::thread::sleep(Duration::from_secs(1));
+    stop_mount_child(&mut child, mountpoint);
     let out = child.wait_with_output().expect("wait for child");
     format!(
         "{}{}",
@@ -886,16 +940,19 @@ fn mountpoint_under_tmp_not_rejected_when_source_is_non_tmp() {
     // must not fire.
     let source = non_tmp_dir();
     let mount = tempfile::tempdir().expect("mount tempdir"); // under /tmp
-    let combined = run_briefly(&[
-        "mount",
-        source.path().to_str().unwrap(),
-        mount.path().to_str().unwrap(),
-        "--security",
-        "--activation-mode",
-        "file",
-        "--notify-socket",
-        "/run/skillfs-privtmp-test.sock",
-    ]);
+    let combined = run_briefly(
+        mount.path(),
+        &[
+            "mount",
+            source.path().to_str().unwrap(),
+            mount.path().to_str().unwrap(),
+            "--security",
+            "--activation-mode",
+            "file",
+            "--notify-socket",
+            "/run/skillfs-privtmp-test.sock",
+        ],
+    );
     assert!(
         !combined.contains("PrivateTmp=true"),
         "PrivateTmp gate must not fire for an agent-visible /tmp mountpoint \
@@ -911,18 +968,21 @@ fn non_tmp_daemon_facing_root_passes_gate() {
     // branch without requiring CAP_SYS_ADMIN.
     let source = non_tmp_dir();
     let mount = tempfile::tempdir().expect("mount tempdir");
-    let combined = run_briefly(&[
-        "mount",
-        source.path().to_str().unwrap(),
-        mount.path().to_str().unwrap(),
-        "--security",
-        "--activation-mode",
-        "file",
-        "--notify-socket",
-        "/run/skillfs-privtmp-test.sock",
-        "--ledger-backing-root",
-        source.path().to_str().unwrap(),
-    ]);
+    let combined = run_briefly(
+        mount.path(),
+        &[
+            "mount",
+            source.path().to_str().unwrap(),
+            mount.path().to_str().unwrap(),
+            "--security",
+            "--activation-mode",
+            "file",
+            "--notify-socket",
+            "/run/skillfs-privtmp-test.sock",
+            "--ledger-backing-root",
+            source.path().to_str().unwrap(),
+        ],
+    );
     assert!(
         !combined.contains("PrivateTmp=true") && !combined.contains("resolves under /tmp"),
         "PrivateTmp gate must not fire for a non-tmp backing root, got: {combined}"
@@ -1086,18 +1146,21 @@ fn non_tmp_transport_paths_pass_gate() {
     let mount = non_tmp_dir();
     let events_dir = non_tmp_dir();
     let events_log = events_dir.path().join("events.jsonl");
-    let combined = run_briefly(&[
-        "mount",
-        source.path().to_str().unwrap(),
-        mount.path().to_str().unwrap(),
-        "--security",
-        "--activation-mode",
-        "file",
-        "--activation-events-log",
-        events_log.to_str().unwrap(),
-        "--notify-socket",
-        "/run/skillfs-privtmp-test.sock",
-    ]);
+    let combined = run_briefly(
+        mount.path(),
+        &[
+            "mount",
+            source.path().to_str().unwrap(),
+            mount.path().to_str().unwrap(),
+            "--security",
+            "--activation-mode",
+            "file",
+            "--activation-events-log",
+            events_log.to_str().unwrap(),
+            "--notify-socket",
+            "/run/skillfs-privtmp-test.sock",
+        ],
+    );
     assert!(
         !combined.contains("PrivateTmp=true") && !combined.contains("resolves under /tmp"),
         "PrivateTmp gate must not fire for non-tmp transport paths, got: {combined}"
@@ -1277,7 +1340,7 @@ staging_patterns = [".openclaw-install-stage-*"]
         .expect("spawn skillfs");
 
     std::thread::sleep(std::time::Duration::from_secs(2));
-    let _ = child.kill();
+    stop_mount_child(&mut child, mount.path());
     let out = child.wait_with_output().expect("wait for child");
     let combined = format!(
         "{}{}",
@@ -1331,7 +1394,7 @@ staging_patterns = [".openclaw-install-stage-*"]
         .expect("spawn skillfs");
 
     std::thread::sleep(std::time::Duration::from_secs(2));
-    let _ = child.kill();
+    stop_mount_child(&mut child, mount.path());
     let out = child.wait_with_output().expect("wait for child");
     let combined = format!(
         "{}{}",
@@ -1694,7 +1757,7 @@ fn control_socket_created_and_accepts_ping() {
         false
     };
 
-    let _ = child.kill();
+    stop_mount_child(&mut child, mount.path());
     let _ = child.wait();
 
     if socket_exists {

src/skillfs/crates/skillfs-fuse/src/lib.rs

@@ -117,10 +117,23 @@ impl MountHandle {
                     info!("unmount successful");
                 }
                 Ok(output) => {
+                    // A plain `fusermount3 -u` can fail transiently (busy /
+                    // lazy). Best-effort force cleanup so we never leave the
+                    // mountpoint dangling, but still surface the original
+                    // failure to the caller for explicit `unmount()`.
+                    //
+                    // Detach the session thread first so a later `Drop` sees
+                    // `session == None` and skips a second `unmount_inner`
+                    // (which would run `force_unmount_path` again — up to a
+                    // full 5s each on a genuinely stuck mount).
                     let stderr = String::from_utf8_lossy(&output.stderr);
+                    self.session.take();
+                    Self::force_unmount_path(&self.mountpoint);
                     return Err(FuseError::UnmountFailed(stderr.to_string()));
                 }
                 Err(e) => {
+                    self.session.take();
+                    Self::force_unmount_path(&self.mountpoint);
                     return Err(FuseError::IoError(e));
                 }
             }
@@ -141,13 +154,83 @@ impl MountHandle {
     pub fn is_mounted(&self) -> bool {
         std::fs::metadata(&self.mountpoint).is_ok()
     }
+
+    /// Return `true` when `path` currently appears as a mountpoint in
+    /// `/proc/mounts`. This is the authoritative signal (unlike
+    /// `std::fs::metadata`, which can succeed on a dead FUSE endpoint).
+    #[cfg(target_os = "linux")]
+    fn path_is_mounted(path: &std::path::Path) -> bool {
+        let Ok(mounts) = std::fs::read_to_string("/proc/mounts") else {
+            return false;
+        };
+        let target = path.to_string_lossy();
+        mounts
+            .lines()
+            .any(|line| line.split_whitespace().nth(1) == Some(&*target))
+    }
+
+    /// One best-effort unmount pass: plain `fusermount3 -u`, then lazy
+    /// `fusermount3 -u -z`, then `umount -l`. All failures are ignored; the
+    /// caller re-checks `/proc/mounts` to decide whether to retry.
+    #[cfg(target_os = "linux")]
+    fn try_unmount_once(path: &std::path::Path) {
+        let mountpoint = path.to_string_lossy();
+        let _ = std::process::Command::new("fusermount3")
+            .args(["-u", &mountpoint])
+            .output();
+        let _ = std::process::Command::new("fusermount3")
+            .args(["-u", "-z", &mountpoint])
+            .output();
+        let _ = std::process::Command::new("umount")
+            .args(["-l", &mountpoint])
+            .output();
+    }
+
+    /// Bounded, non-panicking force cleanup of a mountpoint. Returns as soon
+    /// as the path is no longer mounted; otherwise retries a fixed number of
+    /// times before giving up with a warning. Used by both `Drop` and the
+    /// explicit `unmount()` error path so every test/handle teardown route is
+    /// covered — a leaked FUSE mount under a workspace directory is far worse
+    /// than a slow teardown.
+    #[cfg(target_os = "linux")]
+    fn force_unmount_path(path: &std::path::Path) {
+        for _ in 0..50 {
+            if !Self::path_is_mounted(path) {
+                return;
+            }
+            Self::try_unmount_once(path);
+            std::thread::sleep(std::time::Duration::from_millis(100));
+        }
+
+        if Self::path_is_mounted(path) {
+            eprintln!(
+                "WARN: leaked SkillFS FUSE mount at {} (force cleanup exhausted)",
+                path.display()
+            );
+        }
+    }
 }
 
 impl Drop for MountHandle {
     fn drop(&mut self) {
+        // Best-effort teardown. Must never panic (a panic while unwinding a
+        // failing test would abort the process) and must never block
+        // unboundedly.
+        //
+        // On a clean unmount `unmount_inner` already joins the session thread.
+        // We deliberately do NOT force a join afterwards: if the mountpoint
+        // could only be torn down lazily, the FUSE session thread may never
+        // return, and joining it would hang. Dropping the `JoinHandle` simply
+        // detaches the thread, which is safe once the mount is gone.
         if self.session.is_some() {
             let _ = self.unmount_inner();
         }
+
+        // Cover every path — including tests that just `drop(handle)` — by
+        // force-cleaning the mountpoint even when `unmount_inner` bailed out
+        // early on a `fusermount3` error. Bounded (see `force_unmount_path`).
+        #[cfg(target_os = "linux")]
+        Self::force_unmount_path(&self.mountpoint);
     }
 }
 
@@ -284,16 +367,19 @@ mod tests {
     }
 
     #[test]
-    fn unmount_inner_skips_join_when_fusermount_fails() {
+    fn unmount_inner_detaches_session_when_fusermount_fails() {
         let mut handle = MountHandle {
             mountpoint: PathBuf::from("/nonexistent/mount/point"),
             session: Some(std::thread::spawn(|| {})),
         };
         let result = handle.unmount_inner();
         assert!(result.is_err(), "bogus mountpoint must produce an error");
+        // The error path detaches the session (takes it without joining) so a
+        // subsequent `Drop` sees `session == None` and does not run a second
+        // `unmount_inner` / `force_unmount_path` pass.
         assert!(
-            handle.session.is_some(),
-            "session must remain when fusermount3 fails (join skipped)"
+            handle.session.is_none(),
+            "session must be detached when fusermount3 fails so Drop skips re-unmount"
         );
     }