diff --git a/src/skillfs/crates/skillfs-cli/tests/cli_startup_tests.rs b/src/skillfs/crates/skillfs-cli/tests/cli_startup_tests.rs index a1fbe7b78..81cfdc18c 100644 --- a/src/skillfs/crates/skillfs-cli/tests/cli_startup_tests.rs +++ b/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 { diff --git a/src/skillfs/crates/skillfs-fuse/src/lib.rs b/src/skillfs/crates/skillfs-fuse/src/lib.rs index 116a7128f..bce3dd2f8 100644 --- a/src/skillfs/crates/skillfs-fuse/src/lib.rs +++ b/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" ); }