From de00d80d33f25fe466a60fba9cc5d7b616503f7c Mon Sep 17 00:00:00 2001 From: mikhailm-coder Date: Thu, 17 Sep 2026 18:35:18 +0200 Subject: [PATCH 1/2] fix(client): recover from a locked tool directory on reinstall When a reinstall/uninstall can't delete a tool's install directory because a process holds a file under it open without FILE_SHARE_DELETE (ERROR_SHARING_VIOLATION, os error 32), remove_directory_with_retry now identifies the lock holders via Restart Manager, evicts the safe ones, retries, and falls back to delete-on-reboot instead of looping. Tool-generic; not gated on any tool_id. Kill safety: never terminate this process + its ancestor chain, PID 0/4, RmCritical, or the critical system-process allowlist; auto-terminate only the orphan-shell class and processes whose image is under the tool dir; SCM-stop a service only when its binary is under the tool dir. Every holder is logged before any action is taken. Also routes the single-tool uninstall through remove_directory_with_retry and adds the Win32_Storage_FileSystem windows feature for MoveFileExW. Co-Authored-By: Claude Opus 4.8 --- clients/openframe-client/Cargo.toml | 2 +- .../src/platform/file_lock.rs | 125 +++++- .../src/platform/lock_recovery.rs | 376 ++++++++++++++++++ .../src/platform/lock_recovery_tests.rs | 178 +++++++++ clients/openframe-client/src/platform/mod.rs | 1 + .../src/platform/uninstall.rs | 28 ++ .../src/services/tool_uninstall_service.rs | 8 +- 7 files changed, 698 insertions(+), 20 deletions(-) create mode 100644 clients/openframe-client/src/platform/lock_recovery.rs create mode 100644 clients/openframe-client/src/platform/lock_recovery_tests.rs diff --git a/clients/openframe-client/Cargo.toml b/clients/openframe-client/Cargo.toml index 69d9a09dcf..a59acf6353 100644 --- a/clients/openframe-client/Cargo.toml +++ b/clients/openframe-client/Cargo.toml @@ -126,7 +126,7 @@ winreg = "0.52" winapi = { version = "0.3", features = ["winuser", "shellapi", "securitybaseapi", "errhandlingapi", "winerror", "wincon", "consoleapi", "processenv", "winbase"] } is_elevated = "0.1" windows-service = "0.8" -windows = { version = "0.52", features = ["Win32_Foundation", "Win32_System_Threading", "Win32_System_RemoteDesktop", "Win32_System_JobObjects", "Win32_UI_WindowsAndMessaging", "Win32_System_RestartManager", "Win32_Security", "Win32_Security_Authorization", "Win32_System_Registry", "Win32_System_Environment"] } +windows = { version = "0.52", features = ["Win32_Foundation", "Win32_System_Threading", "Win32_System_RemoteDesktop", "Win32_System_JobObjects", "Win32_UI_WindowsAndMessaging", "Win32_System_RestartManager", "Win32_Security", "Win32_Security_Authorization", "Win32_System_Registry", "Win32_System_Environment", "Win32_Storage_FileSystem"] } [target.'cfg(unix)'.dependencies] libc = "0.2" diff --git a/clients/openframe-client/src/platform/file_lock.rs b/clients/openframe-client/src/platform/file_lock.rs index 49dd03e8f5..74082f399b 100644 --- a/clients/openframe-client/src/platform/file_lock.rs +++ b/clients/openframe-client/src/platform/file_lock.rs @@ -1,10 +1,12 @@ -//! Windows file lock detection using Restart Manager API. +//! Windows file lock detection and holder enumeration using the Restart Manager API. #[cfg(target_os = "windows")] use std::ffi::OsStr; #[cfg(target_os = "windows")] use std::os::windows::ffi::OsStrExt; #[cfg(target_os = "windows")] +use std::path::{Path, PathBuf}; +#[cfg(target_os = "windows")] use windows::core::{PCWSTR, PWSTR}; #[cfg(target_os = "windows")] use windows::Win32::Foundation::FILETIME; @@ -14,6 +16,10 @@ use windows::Win32::System::RestartManager::{ RM_UNIQUE_PROCESS, }; +/// Cap on files registered with Restart Manager for one directory query. +#[cfg(target_os = "windows")] +const MAX_RM_FILES: usize = 1024; + #[cfg(target_os = "windows")] #[derive(Debug, Clone)] pub struct LockingProcess { @@ -21,6 +27,20 @@ pub struct LockingProcess { pub name: String, } +/// A Restart Manager lock holder with the fields needed for safe eviction decisions. +#[cfg(target_os = "windows")] +#[derive(Debug, Clone)] +pub struct LockHolder { + pub pid: u32, + pub app_name: String, + /// Raw `RM_APP_TYPE` (RmCritical == 1000). + pub app_type: i32, + pub ts_session_id: u32, + pub restartable: bool, + /// Present when Restart Manager classifies the holder as a service. + pub service_short_name: Option, +} + #[cfg(target_os = "windows")] impl std::fmt::Display for LockingProcess { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { @@ -28,9 +48,23 @@ impl std::fmt::Display for LockingProcess { } } -/// Returns processes that have the file open. #[cfg(target_os = "windows")] -pub fn get_locking_processes(file_path: &str) -> Result, String> { +fn wide(s: &OsStr) -> Vec { + s.encode_wide().chain(std::iter::once(0)).collect() +} + +#[cfg(target_os = "windows")] +fn decode_utf16(buf: &[u16]) -> String { + let end = buf.iter().position(|&c| c == 0).unwrap_or(buf.len()); + String::from_utf16_lossy(&buf[..end]) +} + +/// Core Restart Manager query: register the given (NUL-terminated wide) paths and return holders. +#[cfg(target_os = "windows")] +fn query_rm_holders(wide_paths: &[Vec]) -> Result, String> { + if wide_paths.is_empty() { + return Ok(vec![]); + } unsafe { let mut session_handle: u32 = 0; let mut session_key = [0u16; 33]; // CCH_RM_SESSION_KEY + 1 @@ -49,13 +83,9 @@ pub fn get_locking_processes(file_path: &str) -> Result, Str } let _guard = SessionGuard(session_handle); - let wide_path: Vec = OsStr::new(file_path) - .encode_wide() - .chain(std::iter::once(0)) - .collect(); - let file_ptr = PCWSTR(wide_path.as_ptr()); + let ptrs: Vec = wide_paths.iter().map(|w| PCWSTR(w.as_ptr())).collect(); - RmRegisterResources(session_handle, Some(&[file_ptr]), None, None) + RmRegisterResources(session_handle, Some(&ptrs), None, None) .map_err(|e| format!("RmRegisterResources failed: {:?}", e))?; let mut needed: u32 = 0; @@ -110,24 +140,87 @@ pub fn get_locking_processes(file_path: &str) -> Result, Str ) .map_err(|e| format!("RmGetList (data) failed: {:?}", e))?; - let locking: Vec = processes + let holders = processes .iter() .take(count as usize) .map(|p| { - let name = String::from_utf16_lossy(&p.strAppName) - .trim_end_matches('\0') - .to_string(); - LockingProcess { + let service_short_name = { + let s = decode_utf16(&p.strServiceShortName); + if s.is_empty() { + None + } else { + Some(s) + } + }; + LockHolder { pid: p.Process.dwProcessId, - name, + app_name: decode_utf16(&p.strAppName), + app_type: p.ApplicationType.0, + ts_session_id: p.TSSessionId, + restartable: p.bRestartable.as_bool(), + service_short_name, } }) .collect(); - Ok(locking) + Ok(holders) } } +/// Returns processes that have the file open (name + PID only). +#[cfg(target_os = "windows")] +pub fn get_locking_processes(file_path: &str) -> Result, String> { + let wide_path = wide(OsStr::new(file_path)); + let holders = query_rm_holders(std::slice::from_ref(&wide_path))?; + Ok(holders + .into_iter() + .map(|h| LockingProcess { + pid: h.pid, + name: h.app_name, + }) + .collect()) +} + +/// Enumerate the regular files under `dir` (bounded, recursive). +#[cfg(target_os = "windows")] +fn collect_files(dir: &Path, out: &mut Vec) { + let Ok(entries) = std::fs::read_dir(dir) else { + return; + }; + for entry in entries.flatten() { + if out.len() >= MAX_RM_FILES { + return; + } + let path = entry.path(); + match entry.file_type() { + Ok(ft) if ft.is_dir() => collect_files(&path, out), + Ok(_) => out.push(path), + Err(_) => {} + } + } +} + +/// Returns the processes holding any file open under `dir`, with the fields needed to +/// evict them safely. Registers every file under the directory (up to `MAX_RM_FILES`) +/// with a single Restart Manager session. +#[cfg(target_os = "windows")] +pub fn get_directory_lock_holders(dir: &Path) -> Result, String> { + let mut files: Vec = Vec::new(); + collect_files(dir, &mut files); + // Fall back to the directory itself when it holds no files (e.g. only subdirectories). + if files.is_empty() { + files.push(dir.to_path_buf()); + } + + let wide_paths: Vec> = files.iter().map(|p| wide(p.as_os_str())).collect(); + let mut holders = query_rm_holders(&wide_paths)?; + + // Dedupe by PID: a process holding several files appears once per file. + holders.sort_by_key(|h| h.pid); + holders.dedup_by_key(|h| h.pid); + Ok(holders) +} + #[cfg(target_os = "windows")] pub fn format_locking_processes(processes: &[LockingProcess]) -> String { if processes.is_empty() { diff --git a/clients/openframe-client/src/platform/lock_recovery.rs b/clients/openframe-client/src/platform/lock_recovery.rs new file mode 100644 index 0000000000..d117719d7a --- /dev/null +++ b/clients/openframe-client/src/platform/lock_recovery.rs @@ -0,0 +1,376 @@ +//! Lock-aware recovery for a tool directory Windows refuses to delete because a process holds +//! a file open without `FILE_SHARE_DELETE` (`ERROR_SHARING_VIOLATION`, os error 32). +//! +//! The classifier is tool-generic and platform-independent (unit-tested everywhere); the +//! eviction and delete-on-reboot machinery is Windows-only. + +#[cfg(any(target_os = "windows", test))] +use std::collections::HashSet; +#[cfg(any(target_os = "windows", test))] +use std::path::{Component, Path, PathBuf}; + +/// Image names that must never be terminated: killing one destabilizes or bugchecks the host. +#[cfg(any(target_os = "windows", test))] +const CRITICAL_PROCESS_NAMES: &[&str] = &[ + "system", + "services.exe", + "svchost.exe", + "lsass.exe", + "csrss.exe", + "wininit.exe", + "smss.exe", + "winlogon.exe", +]; + +/// Orphan-shell class we auto-terminate when it is the one holding the tool dir open. +#[cfg(any(target_os = "windows", test))] +const EVICTABLE_SHELL_NAMES: &[&str] = &["cmd.exe", "powershell.exe", "pwsh.exe", "conhost.exe"]; + +/// `RM_APP_TYPE` value for a system-critical process. +#[cfg(any(target_os = "windows", test))] +const RM_CRITICAL_APP_TYPE: i32 = 1000; + +/// What to do with a single lock holder. +#[cfg(any(target_os = "windows", test))] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum HolderAction { + /// Force-terminate the process (orphan shell, or a stray process under the tool dir). + Terminate, + /// Stop it through the service control manager, not by killing the process. + StopService, + /// Leave it alone (critical, protected, foreign, or unknown). + Skip, +} + +/// A Restart Manager holder enriched with the process facts the decision needs. +#[cfg(any(target_os = "windows", test))] +#[derive(Debug, Clone)] +pub(crate) struct EnrichedHolder { + pub pid: u32, + /// Lowercased executable file name (e.g. `cmd.exe`); falls back to the RM app name. + pub proc_name: String, + pub image_path: Option, + /// Raw `RM_APP_TYPE`. + pub app_type: i32, + /// Present when Restart Manager classified the holder as a service. + pub service_short_name: Option, +} + +/// Case-insensitive file-name match against a lowercase allowlist. +#[cfg(any(target_os = "windows", test))] +fn name_matches(name: &str, list: &[&str]) -> bool { + let file = Path::new(name) + .file_name() + .and_then(|f| f.to_str()) + .unwrap_or(name) + .to_ascii_lowercase(); + list.iter().any(|c| file == *c) +} + +/// Windows paths are case-insensitive, so compare components ASCII-case-insensitively. +#[cfg(any(target_os = "windows", test))] +fn component_eq(a: &Component, b: &Component) -> bool { + a.as_os_str().eq_ignore_ascii_case(b.as_os_str()) +} + +/// True when `child` is `base` itself or lies beneath it. +#[cfg(any(target_os = "windows", test))] +fn path_under(child: &Path, base: &Path) -> bool { + let mut c = child.components(); + for b in base.components() { + match c.next() { + Some(cc) if component_eq(&cc, &b) => {} + _ => return false, + } + } + true +} + +#[cfg(any(target_os = "windows", test))] +fn image_under_dir(image: Option<&Path>, tool_dir: &Path) -> bool { + image.map(|i| path_under(i, tool_dir)).unwrap_or(false) +} + +/// Decide what to do with one holder of a file under `tool_dir`. Encodes the hard kill-safety +/// rules: never the idle/system PIDs, this process or its ancestors, `RmCritical`, or a critical +/// system process; SCM-stop only a service whose binary is the tool being removed; terminate the +/// orphan-shell class and stray processes running from under the tool dir; skip everything else. +#[cfg(any(target_os = "windows", test))] +pub(crate) fn classify_holder( + h: &EnrichedHolder, + tool_dir: &Path, + protected_pids: &HashSet, +) -> HolderAction { + if h.pid == 0 || h.pid == 4 { + return HolderAction::Skip; + } + if protected_pids.contains(&h.pid) { + return HolderAction::Skip; + } + if h.app_type == RM_CRITICAL_APP_TYPE || name_matches(&h.proc_name, CRITICAL_PROCESS_NAMES) { + return HolderAction::Skip; + } + + let under = image_under_dir(h.image_path.as_deref(), tool_dir); + + if h.service_short_name.is_some() { + // Only touch a service that belongs to the tool being removed; never a foreign one. + return if under { + HolderAction::StopService + } else { + HolderAction::Skip + }; + } + + if name_matches(&h.proc_name, EVICTABLE_SHELL_NAMES) || under { + return HolderAction::Terminate; + } + + HolderAction::Skip +} + +// --------------------------------------------------------------------------- +// Windows-only eviction + delete-on-reboot +// --------------------------------------------------------------------------- + +#[cfg(target_os = "windows")] +use crate::platform::file_lock::{get_directory_lock_holders, LockHolder}; +#[cfg(target_os = "windows")] +use crate::platform::system_service; +#[cfg(target_os = "windows")] +use sysinfo::{Pid, System}; +#[cfg(target_os = "windows")] +use tracing::{info, warn}; +#[cfg(target_os = "windows")] +use windows::core::PCWSTR; +#[cfg(target_os = "windows")] +use windows::Win32::Storage::FileSystem::{MoveFileExW, MOVEFILE_DELAY_UNTIL_REBOOT}; + +/// Depth of the ancestor walk when building the protected-PID set (guards against cycles). +#[cfg(target_os = "windows")] +const MAX_ANCESTOR_HOPS: usize = 64; + +/// Build the enriched holders and the protected-PID set (this process + its ancestor chain) +/// from a live process snapshot. +#[cfg(target_os = "windows")] +fn enrich_and_protect(holders: Vec) -> (Vec, HashSet) { + // new_all() already loads process info, so no extra refresh is needed. + let sys = System::new_all(); + + let mut protected: HashSet = HashSet::new(); + let mut current = Some(Pid::from_u32(std::process::id())); + for _ in 0..MAX_ANCESTOR_HOPS { + let Some(pid) = current else { break }; + if !protected.insert(pid.as_u32()) { + break; + } + current = sys.process(pid).and_then(|p| p.parent()); + } + + let enriched = holders + .into_iter() + .map(|h| { + let proc = sys.process(Pid::from_u32(h.pid)); + let image_path = proc.and_then(|p| p.exe()).map(|p| p.to_path_buf()); + let proc_name = image_path + .as_deref() + .and_then(|p| p.file_name()) + .and_then(|f| f.to_str()) + .map(|s| s.to_ascii_lowercase()) + .or_else(|| proc.map(|p| p.name().to_ascii_lowercase())) + .unwrap_or_else(|| h.app_name.to_ascii_lowercase()); + EnrichedHolder { + pid: h.pid, + proc_name, + image_path, + app_type: h.app_type, + service_short_name: h.service_short_name, + } + }) + .collect(); + + (enriched, protected) +} + +/// Identify the processes locking files under `tool_dir`, report them, and evict the ones that +/// are safe to remove. Best-effort: it logs and returns rather than failing the removal. +#[cfg(target_os = "windows")] +pub(crate) async fn evict_holders_for_removal(tool_dir: &Path) { + let dir = tool_dir.to_path_buf(); + let holders = match tokio::task::spawn_blocking(move || get_directory_lock_holders(&dir)).await + { + Ok(Ok(holders)) => holders, + Ok(Err(e)) => { + warn!( + "Lock recovery: Restart Manager query failed for {}: {}", + tool_dir.display(), + e + ); + return; + } + Err(e) => { + warn!( + "Lock recovery: holder-query task failed for {}: {}", + tool_dir.display(), + e + ); + return; + } + }; + + if holders.is_empty() { + info!( + "Lock recovery: no Restart Manager holders under {}", + tool_dir.display() + ); + return; + } + + // Report every holder before taking any action (ships to Loki). + for h in &holders { + info!( + pid = h.pid, + app_name = %h.app_name, + app_type = h.app_type, + ts_session_id = h.ts_session_id, + restartable = h.restartable, + service = %h.service_short_name.as_deref().unwrap_or(""), + "Lock recovery: holder of {}", + tool_dir.display() + ); + } + + let (enriched, protected) = + match tokio::task::spawn_blocking(move || enrich_and_protect(holders)).await { + Ok(result) => result, + Err(e) => { + warn!("Lock recovery: enrichment task failed: {}", e); + return; + } + }; + + let mut to_terminate: Vec = Vec::new(); + let mut to_stop_service: Vec = Vec::new(); + + for h in &enriched { + let action = classify_holder(h, tool_dir, &protected); + info!( + pid = h.pid, + name = %h.proc_name, + image = %h.image_path.as_deref().map(|p| p.display().to_string()).unwrap_or_default(), + action = ?action, + "Lock recovery: decision for holder" + ); + match action { + HolderAction::Terminate => to_terminate.push(h.pid), + HolderAction::StopService => { + if let Some(svc) = &h.service_short_name { + to_stop_service.push(svc.clone()); + } + } + HolderAction::Skip => {} + } + } + + for svc in to_stop_service { + info!("Lock recovery: stopping service holder {} via SCM", svc); + if let Err(e) = system_service::stop_service(&svc, false).await { + warn!("Lock recovery: failed to stop service {}: {:#}", svc, e); + } + } + + for pid in to_terminate { + terminate_pid(pid).await; + } + + // Let the OS release the handles before the caller retries the removal. + tokio::time::sleep(std::time::Duration::from_millis(1500)).await; +} + +#[cfg(target_os = "windows")] +async fn terminate_pid(pid: u32) { + info!("Lock recovery: terminating lock holder PID {}", pid); + match tokio::process::Command::new("taskkill") + .args(["/F", "/T", "/PID", &pid.to_string()]) + .output() + .await + { + Ok(out) if !out.status.success() => warn!( + "Lock recovery: taskkill for PID {} reported: {}", + pid, + String::from_utf8_lossy(&out.stderr).trim() + ), + Ok(_) => {} + Err(e) => warn!( + "Lock recovery: failed to run taskkill for PID {}: {}", + pid, e + ), + } +} + +/// Schedule every entry under `dir` (and `dir` itself) for deletion on the next boot via +/// `MoveFileEx`, so a directory a refused holder still locks is cleared without a manual wipe. +/// Returns the number of entries scheduled. +#[cfg(target_os = "windows")] +pub(crate) fn schedule_delete_on_reboot(dir: &Path) -> usize { + let mut files: Vec = Vec::new(); + let mut dirs: Vec = Vec::new(); + collect_entries(dir, &mut files, &mut dirs); + dirs.push(dir.to_path_buf()); + // Deepest first, so each directory is empty when its own deletion is applied at boot. + dirs.sort_by_key(|d| std::cmp::Reverse(d.components().count())); + + let mut scheduled = 0usize; + for file in &files { + if move_delete_on_reboot(file) { + scheduled += 1; + } + } + for directory in &dirs { + if move_delete_on_reboot(directory) { + scheduled += 1; + } + } + scheduled +} + +#[cfg(target_os = "windows")] +fn collect_entries(dir: &Path, files: &mut Vec, dirs: &mut Vec) { + let Ok(entries) = std::fs::read_dir(dir) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + match entry.file_type() { + Ok(ft) if ft.is_dir() => { + collect_entries(&path, files, dirs); + dirs.push(path); + } + Ok(_) => files.push(path), + Err(_) => {} + } + } +} + +#[cfg(target_os = "windows")] +fn move_delete_on_reboot(path: &Path) -> bool { + use std::os::windows::ffi::OsStrExt; + let wide: Vec = path + .as_os_str() + .encode_wide() + .chain(std::iter::once(0)) + .collect(); + // A NULL destination with MOVEFILE_DELAY_UNTIL_REBOOT marks the path for deletion at boot. + unsafe { + MoveFileExW( + PCWSTR(wide.as_ptr()), + PCWSTR(std::ptr::null()), + MOVEFILE_DELAY_UNTIL_REBOOT, + ) + .is_ok() + } +} + +#[cfg(test)] +#[path = "lock_recovery_tests.rs"] +mod tests; diff --git a/clients/openframe-client/src/platform/lock_recovery_tests.rs b/clients/openframe-client/src/platform/lock_recovery_tests.rs new file mode 100644 index 0000000000..26d68d6cbd --- /dev/null +++ b/clients/openframe-client/src/platform/lock_recovery_tests.rs @@ -0,0 +1,178 @@ +use super::*; +use std::collections::HashSet; +use std::path::PathBuf; + +fn tool_dir() -> PathBuf { + PathBuf::from(r"C:\ProgramData\OpenFrame\meshcentral-agent") +} + +fn holder(pid: u32, name: &str) -> EnrichedHolder { + EnrichedHolder { + pid, + proc_name: name.to_string(), + image_path: None, + app_type: 0, + service_short_name: None, + } +} + +#[test] +fn terminates_orphan_shells() { + let protected = HashSet::new(); + for shell in [ + "cmd.exe", + "powershell.exe", + "pwsh.exe", + "conhost.exe", + "POWERSHELL.EXE", + ] { + let h = holder(4321, shell); + assert_eq!( + classify_holder(&h, &tool_dir(), &protected), + HolderAction::Terminate, + "{shell} should be terminated" + ); + } +} + +#[test] +fn matches_shell_by_full_image_name() { + // Forward slashes parse as separators on every host, so file_name() resolves to cmd.exe. + let h = holder(4321, "C:/Windows/System32/cmd.exe"); + assert_eq!( + classify_holder(&h, &tool_dir(), &HashSet::new()), + HolderAction::Terminate + ); +} + +#[cfg(windows)] +#[test] +fn matches_shell_by_backslash_image_name() { + let h = holder(4321, r"C:\Windows\System32\cmd.exe"); + assert_eq!( + classify_holder(&h, &tool_dir(), &HashSet::new()), + HolderAction::Terminate + ); +} + +#[test] +fn skips_self_and_ancestors() { + let mut protected = HashSet::new(); + protected.insert(4321); + let h = holder(4321, "cmd.exe"); + assert_eq!( + classify_holder(&h, &tool_dir(), &protected), + HolderAction::Skip + ); +} + +#[test] +fn skips_idle_and_system_pids() { + for pid in [0u32, 4] { + let h = holder(pid, "cmd.exe"); + assert_eq!( + classify_holder(&h, &tool_dir(), &HashSet::new()), + HolderAction::Skip, + "pid {pid} must be skipped" + ); + } +} + +#[test] +fn skips_critical_system_processes() { + for name in [ + "services.exe", + "svchost.exe", + "lsass.exe", + "csrss.exe", + "wininit.exe", + "smss.exe", + "winlogon.exe", + "System", + "SVCHOST.EXE", + ] { + let h = holder(9000, name); + assert_eq!( + classify_holder(&h, &tool_dir(), &HashSet::new()), + HolderAction::Skip, + "{name} must be skipped" + ); + } +} + +#[test] +fn skips_rmcritical_app_type_even_for_a_shell_name() { + let mut h = holder(9000, "cmd.exe"); + h.app_type = RM_CRITICAL_APP_TYPE; + assert_eq!( + classify_holder(&h, &tool_dir(), &HashSet::new()), + HolderAction::Skip + ); +} + +#[test] +fn terminates_stray_process_under_tool_dir() { + let mut h = holder(9000, "osqueryd.exe"); + h.image_path = Some(tool_dir().join("osqueryd").join("osqueryd.exe")); + assert_eq!( + classify_holder(&h, &tool_dir(), &HashSet::new()), + HolderAction::Terminate + ); +} + +#[test] +fn skips_unrelated_foreign_process() { + let mut h = holder(9000, "notepad.exe"); + h.image_path = Some(PathBuf::from(r"C:\Windows\System32\notepad.exe")); + assert_eq!( + classify_holder(&h, &tool_dir(), &HashSet::new()), + HolderAction::Skip + ); +} + +#[test] +fn stops_service_whose_binary_is_under_the_tool_dir() { + let mut h = holder(9000, "meshagent.exe"); + h.image_path = Some(tool_dir().join("agent.exe")); + h.service_short_name = Some("Mesh Agent".to_string()); + assert_eq!( + classify_holder(&h, &tool_dir(), &HashSet::new()), + HolderAction::StopService + ); +} + +#[test] +fn skips_foreign_service_never_terminates_it() { + let mut h = holder(9000, "third-party.exe"); + h.image_path = Some(PathBuf::from(r"C:\Program Files\Other\svc.exe")); + h.service_short_name = Some("OtherVendorSvc".to_string()); + assert_eq!( + classify_holder(&h, &tool_dir(), &HashSet::new()), + HolderAction::Skip + ); +} + +// Drive-letter/case-insensitive path matching is a Windows concept; parse it there. +#[cfg(windows)] +#[test] +fn image_under_dir_is_case_insensitive() { + let child = PathBuf::from(r"c:\programdata\openframe\meshcentral-agent\agent.exe"); + assert!(image_under_dir(Some(child.as_path()), &tool_dir())); +} + +#[test] +fn image_under_dir_matches_nested_child() { + let child = tool_dir().join("logs").join("agent.log"); + assert!(image_under_dir(Some(child.as_path()), &tool_dir())); +} + +#[test] +fn image_not_under_sibling_dir() { + // A sibling that shares the parent but not the leaf must not count as "under". + let sibling = tool_dir() + .parent() + .unwrap() + .join("fleetmdm-agent") + .join("agent.exe"); + assert!(!image_under_dir(Some(sibling.as_path()), &tool_dir())); +} diff --git a/clients/openframe-client/src/platform/mod.rs b/clients/openframe-client/src/platform/mod.rs index 76371fa6a0..b9903c280d 100644 --- a/clients/openframe-client/src/platform/mod.rs +++ b/clients/openframe-client/src/platform/mod.rs @@ -5,6 +5,7 @@ pub mod dmg_extractor; pub mod file_acl; pub mod file_lock; pub mod installation_detector; +pub mod lock_recovery; pub mod machine_info_persistence; pub mod permissions; pub mod system_service; diff --git a/clients/openframe-client/src/platform/uninstall.rs b/clients/openframe-client/src/platform/uninstall.rs index 09b81e2300..d63c2f5fa2 100644 --- a/clients/openframe-client/src/platform/uninstall.rs +++ b/clients/openframe-client/src/platform/uninstall.rs @@ -126,6 +126,19 @@ pub async fn remove_directory_with_retry(path: &Path, max_retries: u32) -> Resul return Ok(()); } Err(e) => { + // Sharing violation (os error 32): a process holds a file open without + // FILE_SHARE_DELETE. Identify the holders, evict the safe ones, and retry. + #[cfg(target_os = "windows")] + if crate::platform::file_lock::is_file_in_use_error(&e) { + warn!( + "Removal of {} blocked by a sharing violation on attempt {}/{}; attempting lock-aware recovery", + path.display(), + attempt, + max_retries + ); + crate::platform::lock_recovery::evict_holders_for_removal(path).await; + } + if attempt < max_retries { let wait_secs = std::cmp::min(2_u64.pow(attempt - 1), 8); // Exponential backoff, max 8 seconds warn!( @@ -151,6 +164,21 @@ pub async fn remove_directory_with_retry(path: &Path, max_retries: u32) -> Resul return Ok(()); } Err(force_err) => { + // A refused/unkillable holder still locks the directory: schedule its + // contents for deletion on the next boot so it clears without a wipe. + #[cfg(target_os = "windows")] + if path.exists() && crate::platform::file_lock::is_file_in_use_error(&e) + { + let scheduled = + crate::platform::lock_recovery::schedule_delete_on_reboot(path); + warn!( + "Directory {} is still locked after {} attempts; scheduled {} entr{} for delete-on-reboot (a reboot is required to fully clear it)", + path.display(), + max_retries, + scheduled, + if scheduled == 1 { "y" } else { "ies" } + ); + } return Err(anyhow::anyhow!( "Failed to remove directory {} after {} attempts. Last error: {}. Force removal error: {}", path.display(), diff --git a/clients/openframe-client/src/services/tool_uninstall_service.rs b/clients/openframe-client/src/services/tool_uninstall_service.rs index 7592e1f895..162f90d9e1 100644 --- a/clients/openframe-client/src/services/tool_uninstall_service.rs +++ b/clients/openframe-client/src/services/tool_uninstall_service.rs @@ -134,9 +134,11 @@ impl ToolUninstallService { let tool_dir = self.directory_manager.app_support_dir().join(tool_agent_id); if tool_dir.exists() { - std::fs::remove_dir_all(&tool_dir).with_context(|| { - format!("Failed to remove tool directory: {}", tool_dir.display()) - })?; + crate::platform::remove_directory_with_retry(&tool_dir, 5) + .await + .with_context(|| { + format!("Failed to remove tool directory: {}", tool_dir.display()) + })?; info!("Removed tool directory: {}", tool_dir.display()); } From 4dc1791e2265c73460ba5a2f73d14c35276d2658 Mon Sep 17 00:00:00 2001 From: mikhailm-coder Date: Mon, 21 Sep 2026 22:13:19 +0200 Subject: [PATCH 2/2] refactor(client): drop the delete-on-reboot fallback from lock recovery MoveFileEx delay-until-reboot schedules a delete by path and never cancels it, so if the tool directory is legitimately recreated by a successful reinstall before the reboot, the next boot would delete the fresh files. The eviction + retry path already recovers the common case (a killable orphan-shell holder), and a refused holder clears on the next reboot via redelivery, so the fallback's marginal value did not justify that footgun. Removes schedule_delete_on_reboot and the Win32_Storage_FileSystem feature; the terminal failure now returns Err as it already did for non-sharing-violation errors. Co-Authored-By: Claude Opus 4.8 --- clients/openframe-client/Cargo.toml | 2 +- .../src/platform/lock_recovery.rs | 69 +------------------ .../src/platform/uninstall.rs | 15 ---- 3 files changed, 2 insertions(+), 84 deletions(-) diff --git a/clients/openframe-client/Cargo.toml b/clients/openframe-client/Cargo.toml index a59acf6353..69d9a09dcf 100644 --- a/clients/openframe-client/Cargo.toml +++ b/clients/openframe-client/Cargo.toml @@ -126,7 +126,7 @@ winreg = "0.52" winapi = { version = "0.3", features = ["winuser", "shellapi", "securitybaseapi", "errhandlingapi", "winerror", "wincon", "consoleapi", "processenv", "winbase"] } is_elevated = "0.1" windows-service = "0.8" -windows = { version = "0.52", features = ["Win32_Foundation", "Win32_System_Threading", "Win32_System_RemoteDesktop", "Win32_System_JobObjects", "Win32_UI_WindowsAndMessaging", "Win32_System_RestartManager", "Win32_Security", "Win32_Security_Authorization", "Win32_System_Registry", "Win32_System_Environment", "Win32_Storage_FileSystem"] } +windows = { version = "0.52", features = ["Win32_Foundation", "Win32_System_Threading", "Win32_System_RemoteDesktop", "Win32_System_JobObjects", "Win32_UI_WindowsAndMessaging", "Win32_System_RestartManager", "Win32_Security", "Win32_Security_Authorization", "Win32_System_Registry", "Win32_System_Environment"] } [target.'cfg(unix)'.dependencies] libc = "0.2" diff --git a/clients/openframe-client/src/platform/lock_recovery.rs b/clients/openframe-client/src/platform/lock_recovery.rs index d117719d7a..b6c711faa5 100644 --- a/clients/openframe-client/src/platform/lock_recovery.rs +++ b/clients/openframe-client/src/platform/lock_recovery.rs @@ -130,7 +130,7 @@ pub(crate) fn classify_holder( } // --------------------------------------------------------------------------- -// Windows-only eviction + delete-on-reboot +// Windows-only eviction // --------------------------------------------------------------------------- #[cfg(target_os = "windows")] @@ -141,10 +141,6 @@ use crate::platform::system_service; use sysinfo::{Pid, System}; #[cfg(target_os = "windows")] use tracing::{info, warn}; -#[cfg(target_os = "windows")] -use windows::core::PCWSTR; -#[cfg(target_os = "windows")] -use windows::Win32::Storage::FileSystem::{MoveFileExW, MOVEFILE_DELAY_UNTIL_REBOOT}; /// Depth of the ancestor walk when building the protected-PID set (guards against cycles). #[cfg(target_os = "windows")] @@ -308,69 +304,6 @@ async fn terminate_pid(pid: u32) { } } -/// Schedule every entry under `dir` (and `dir` itself) for deletion on the next boot via -/// `MoveFileEx`, so a directory a refused holder still locks is cleared without a manual wipe. -/// Returns the number of entries scheduled. -#[cfg(target_os = "windows")] -pub(crate) fn schedule_delete_on_reboot(dir: &Path) -> usize { - let mut files: Vec = Vec::new(); - let mut dirs: Vec = Vec::new(); - collect_entries(dir, &mut files, &mut dirs); - dirs.push(dir.to_path_buf()); - // Deepest first, so each directory is empty when its own deletion is applied at boot. - dirs.sort_by_key(|d| std::cmp::Reverse(d.components().count())); - - let mut scheduled = 0usize; - for file in &files { - if move_delete_on_reboot(file) { - scheduled += 1; - } - } - for directory in &dirs { - if move_delete_on_reboot(directory) { - scheduled += 1; - } - } - scheduled -} - -#[cfg(target_os = "windows")] -fn collect_entries(dir: &Path, files: &mut Vec, dirs: &mut Vec) { - let Ok(entries) = std::fs::read_dir(dir) else { - return; - }; - for entry in entries.flatten() { - let path = entry.path(); - match entry.file_type() { - Ok(ft) if ft.is_dir() => { - collect_entries(&path, files, dirs); - dirs.push(path); - } - Ok(_) => files.push(path), - Err(_) => {} - } - } -} - -#[cfg(target_os = "windows")] -fn move_delete_on_reboot(path: &Path) -> bool { - use std::os::windows::ffi::OsStrExt; - let wide: Vec = path - .as_os_str() - .encode_wide() - .chain(std::iter::once(0)) - .collect(); - // A NULL destination with MOVEFILE_DELAY_UNTIL_REBOOT marks the path for deletion at boot. - unsafe { - MoveFileExW( - PCWSTR(wide.as_ptr()), - PCWSTR(std::ptr::null()), - MOVEFILE_DELAY_UNTIL_REBOOT, - ) - .is_ok() - } -} - #[cfg(test)] #[path = "lock_recovery_tests.rs"] mod tests; diff --git a/clients/openframe-client/src/platform/uninstall.rs b/clients/openframe-client/src/platform/uninstall.rs index d63c2f5fa2..2ddbfd5da6 100644 --- a/clients/openframe-client/src/platform/uninstall.rs +++ b/clients/openframe-client/src/platform/uninstall.rs @@ -164,21 +164,6 @@ pub async fn remove_directory_with_retry(path: &Path, max_retries: u32) -> Resul return Ok(()); } Err(force_err) => { - // A refused/unkillable holder still locks the directory: schedule its - // contents for deletion on the next boot so it clears without a wipe. - #[cfg(target_os = "windows")] - if path.exists() && crate::platform::file_lock::is_file_in_use_error(&e) - { - let scheduled = - crate::platform::lock_recovery::schedule_delete_on_reboot(path); - warn!( - "Directory {} is still locked after {} attempts; scheduled {} entr{} for delete-on-reboot (a reboot is required to fully clear it)", - path.display(), - max_retries, - scheduled, - if scheduled == 1 { "y" } else { "ies" } - ); - } return Err(anyhow::anyhow!( "Failed to remove directory {} after {} attempts. Last error: {}. Force removal error: {}", path.display(),