From ebb69779eb799d5b6f6e50f7efe67af7b9e8eb71 Mon Sep 17 00:00:00 2001 From: YOMXXX Date: Thu, 28 May 2026 21:45:40 +0800 Subject: [PATCH] fix(reset): release log file handle + queue locked entries on Windows reboot (#2668) --- app/src-tauri/src/lib.rs | 201 ++++++++++-- app/src-tauri/src/reset_reboot_schedule.rs | 358 +++++++++++++++++++++ src/core/logging.rs | 101 +++++- 3 files changed, 636 insertions(+), 24 deletions(-) create mode 100644 app/src-tauri/src/reset_reboot_schedule.rs diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index caea9c3af..5b5100250 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -36,6 +36,8 @@ mod native_notifications; mod notification_settings; mod process_kill; mod process_recovery; +#[cfg(target_os = "windows")] +mod reset_reboot_schedule; mod screen_capture; mod slack_scanner; mod telegram_scanner; @@ -384,6 +386,18 @@ async fn reset_local_data( state.inner().shutdown().await; log::info!("[core] reset_local_data: embedded core stopped"); + // ── 3b. Release the host-process log file handle (issue #1615) ────── + // + // The daily-rotating log appender at `/logs/openhuman-*.log` + // is owned by *this* Tauri host process, not by the embedded core + // tokio task — so `shutdown()` above does not release it. On Windows + // that lingering OS file handle causes `remove_dir_all(.openhuman)` + // below to fail with `ERROR_SHARING_VIOLATION` (os error 32). Drop + // the writer guard now so the background flushing thread exits and + // the file handle is closed before the removal walks the tree. + let log_guard_dropped = openhuman_core::core::logging::shutdown_file_guard(); + log::info!("[core] reset_local_data: shutdown_file_guard dropped guard = {log_guard_dropped}"); + // ── 4. Remove the paths ───────────────────────────────────────────── // // Missing entries are non-fatal: the user may already have manually @@ -450,23 +464,125 @@ fn is_windows_file_lock_error(error: &std::io::Error) -> bool { cfg!(windows) && is_windows_file_lock_raw_os_error(error.raw_os_error()) } +/// Returns: +/// * `Ok(())` — the underlying remove failure should be swallowed (e.g. +/// the path disappeared between the failed `remove_*` call and the +/// reboot-fallback walk, so there is nothing left to clean up). +/// * `Err(msg)` — a user-facing failure message the caller should surface +/// to the UI / propagate up the reset flow. fn reset_local_data_delete_error( label: &str, path: &std::path::Path, error: &std::io::Error, -) -> String { +) -> Result<(), String> { if is_windows_file_lock_error(error) { log::warn!( "[core] reset_local_data: Windows file lock blocked removal of {label} at {}: {error}", path.display() ); - return format!( - "Failed to remove {label} at {} because it is locked by another OpenHuman window or process. Close all OpenHuman windows and try again. ({error})", - path.display() - ); + + // Fallback: queue the still-locked sub-tree for deletion on the + // next Windows boot via MoveFileExW + MOVEFILE_DELAY_UNTIL_REBOOT. + // By this point in `reset_local_data` we have already: + // * shut down the embedded core (drops every SQLite/log handle + // the core task held), and + // * released the host-process log appender via + // `shutdown_file_guard()` (drops the rolling log file handle). + // So any remaining lock now comes from *outside* this process — + // anti-virus / file indexer / sibling app / Explorer — and cannot + // be released by closing more OpenHuman windows. See issue #1615. + #[cfg(target_os = "windows")] + { + return schedule_reboot_delete_or_describe(label, path, error); + } + // `is_windows_file_lock_error` is gated on `cfg!(windows)`, so on + // Linux/macOS this branch is unreachable at runtime — but cargo + // still type-checks the file for those targets and needs a value + // of type `String`. + #[cfg(not(target_os = "windows"))] + { + return Err(format!( + "Failed to remove {label} at {} because it is locked by another OpenHuman window or process. Close all OpenHuman windows and try again. ({error})", + path.display() + )); + } } - format!("Failed to remove {label} at {}: {error}", path.display()) + Err(format!( + "Failed to remove {label} at {}: {error}", + path.display() + )) +} + +/// Windows-only: ask the session manager to delete `path` (and its +/// children if it is a directory) on the next reboot, and return either a +/// user-facing message describing the outcome or `Ok(())` when the +/// underlying failure should be treated as already-cleaned-up. +#[cfg(target_os = "windows")] +fn schedule_reboot_delete_or_describe( + label: &str, + path: &std::path::Path, + original_error: &std::io::Error, +) -> Result<(), String> { + match reset_reboot_schedule::schedule_path_for_reboot_deletion(path) { + Ok(summary) => { + log::info!( + "[core] reset_local_data: scheduled {label} at {} for reboot deletion (files={}, dirs={})", + path.display(), + summary.files, + summary.dirs + ); + Err(format!( + "Couldn't remove {label} at {} right now because another process is holding it open ({original_error}). {} files and {} folders have been queued for deletion the next time you restart Windows — restart soon to finish the reset.", + path.display(), + summary.files, + summary.dirs, + )) + } + // Race condition: the still-locked path disappeared between the + // `remove_*` call that failed with `ERROR_SHARING_VIOLATION` and + // the metadata read inside the reboot-schedule walk. Whoever else + // held the handle has already finished cleaning up, so the reset + // goal is achieved — swallow the original lock error and treat + // this as success. The empty partial schedule (no entries queued + // yet) is what distinguishes "vanished cleanly" from "started + // walking, then hit a real error." + Err(failure) + if failure.error.kind() == std::io::ErrorKind::NotFound + && failure.partial.total() == 0 => + { + log::info!( + "[core] reset_local_data: {label} at {} disappeared between lock failure and reboot fallback; treating as removed", + path.display(), + ); + Ok(()) + } + Err(failure) => { + let partial_total = failure.partial.total(); + log::error!( + "[core] reset_local_data: reboot delete fallback failed for {label} at {}: {} (partial schedule: files={}, dirs={})", + path.display(), + failure.error, + failure.partial.files, + failure.partial.dirs, + ); + if partial_total == 0 { + Err(format!( + "Failed to remove {label} at {} because it is locked by another OpenHuman window or process, and scheduling deletion on next reboot also failed ({}). Close all OpenHuman windows and try again. ({original_error})", + path.display(), + failure.error, + )) + } else { + Err(format!( + "Failed to remove {label} at {} because it is locked by another OpenHuman window or process. {} files and {} folders were queued for the next reboot before scheduling failed ({}); the rest still needs manual cleanup. Close all OpenHuman windows and try again. ({original_error})", + path.display(), + failure.partial.files, + failure.partial.dirs, + failure.error, + )) + } + } + } } /// Call the core's `config_get_data_paths` RPC and parse the response. @@ -536,7 +652,7 @@ async fn remove_path_if_exists(path: &std::path::Path, label: &str) -> Result<() ); Ok(()) } - Err(e) => Err(reset_local_data_delete_error(label, path, &e)), + Err(e) => reset_local_data_delete_error(label, path, &e), } } @@ -557,7 +673,7 @@ async fn remove_dir_if_exists(path: &std::path::Path, label: &str) -> Result<(), ); Ok(()) } - Err(e) => Err(reset_local_data_delete_error(label, path, &e)), + Err(e) => reset_local_data_delete_error(label, path, &e), } } @@ -3620,28 +3736,77 @@ mod tests { #[test] fn reset_local_data_delete_error_keeps_generic_message_for_other_errors() { let err = std::io::Error::from(std::io::ErrorKind::PermissionDenied); - let msg = reset_local_data_delete_error( + let result = reset_local_data_delete_error( "current openhuman dir", std::path::Path::new("/tmp/openhuman"), &err, ); + let msg = result.expect_err("non-lock errors must still surface to the UI"); assert!(msg.starts_with("Failed to remove current openhuman dir at /tmp/openhuman:")); assert!(!msg.contains("Close all OpenHuman windows and try again")); } #[cfg(windows)] #[test] - fn reset_local_data_delete_error_explains_windows_file_locks() { - let err = std::io::Error::from_raw_os_error(32); - let msg = reset_local_data_delete_error( - "current openhuman dir", - std::path::Path::new("C:\\Users\\me\\.openhuman"), - &err, - ); + fn reset_local_data_delete_error_swallows_lock_failure_when_path_disappeared() { + // Race condition the reboot fallback now handles: the locked path + // was gone by the time `schedule_path_for_reboot_deletion` ran its + // `symlink_metadata` probe, so the reset goal is already met. The + // helper must return `Ok(())` rather than surfacing a confusing + // "couldn't remove (it's not there)" toast. + let dir = tempfile::tempdir().expect("tempdir for reset error test"); + let missing = dir.path().join("definitely-not-there"); - assert!(msg.contains("locked by another OpenHuman window or process")); - assert!(msg.contains("Close all OpenHuman windows and try again")); + let err = std::io::Error::from_raw_os_error(32); + let result = reset_local_data_delete_error("current openhuman dir", &missing, &err); + + assert!( + result.is_ok(), + "expected NotFound + empty partial schedule to be swallowed as success, got {result:?}" + ); + } + + #[cfg(windows)] + #[test] + fn reset_local_data_delete_error_reports_reboot_schedule_counts() { + // When the lock fallback can walk a real directory tree, the user + // message should report how much has been queued so the support + // log preserves "what was actually scheduled". Scheduling itself + // may still fail at the MoveFileExW step in unprivileged test + // processes (the registry key write requires administrator); the + // fallback then carries a partial schedule that the error path + // surfaces, so both branches must keep mentioning the lock cause + // *and* expose either the queued counts or the schedule failure. + let dir = tempfile::tempdir().expect("tempdir for reset error test"); + let target = dir.path().join("reset-mock"); + std::fs::create_dir_all(target.join("nested")).expect("mkdir nested"); + std::fs::write(target.join("a.txt"), b"x").expect("write a.txt"); + std::fs::write(target.join("nested").join("b.txt"), b"y").expect("write b.txt"); + + let err = std::io::Error::from_raw_os_error(32); + let result = reset_local_data_delete_error("current openhuman dir", &target, &err); + + // Path exists on disk, so the fallback must surface the outcome — + // either an "all-queued" success-but-needs-reboot message (admin) + // or one of the failure flavours (non-admin). + let msg = result + .expect_err("path exists, fallback must report queued counts or scheduling failure"); + let admin_path = msg.contains("queued for deletion the next time you restart Windows") + && msg.contains("2 files and 2 folders"); + let user_full_fail = msg.contains("scheduling deletion on next reboot also failed"); + let user_partial = msg.contains("queued for the next reboot before scheduling failed"); + assert!( + admin_path || user_full_fail || user_partial, + "expected reboot-scheduled, fully-failed, or partial-fail message, got: {msg}" + ); + // Whatever branch we land on, the user must still be told the lock + // is what blocked the immediate removal. + assert!( + msg.contains("locked by another OpenHuman window or process") + || msg.contains("another process is holding it open"), + "missing lock cause: {msg}" + ); } /// Tests for setup_tray conditional compilation diff --git a/app/src-tauri/src/reset_reboot_schedule.rs b/app/src-tauri/src/reset_reboot_schedule.rs new file mode 100644 index 000000000..5f2110aa2 --- /dev/null +++ b/app/src-tauri/src/reset_reboot_schedule.rs @@ -0,0 +1,358 @@ +//! Windows-only fallback for `reset_local_data` (issue #1615). +//! +//! When the in-process `remove_dir_all` step fails because a third-party +//! process (anti-virus, file-indexer, sibling OpenHuman window) still holds +//! an open handle inside the `.openhuman` tree, Windows returns +//! `ERROR_SHARING_VIOLATION` (os error 32) / `ERROR_LOCK_VIOLATION` (33) +//! and the user is stuck — see PR #2395 / #1811, which surface a "close all +//! OpenHuman windows" prompt but cannot break a foreign lock. +//! +//! This module walks the still-present sub-tree depth-first and asks the +//! Windows Session Manager to delete each entry at next boot via +//! `MoveFileExW(src, NULL, MOVEFILE_DELAY_UNTIL_REBOOT)`. The session +//! manager requires that directories be empty when boot-time deletion +//! runs, so children are scheduled before their parent. +//! +//! Reference: +//! https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-movefileexw +//! +//! Privileges: `MoveFileExW(.., NULL, MOVEFILE_DELAY_UNTIL_REBOOT)` writes +//! to `HKLM\SYSTEM\CurrentControlSet\Control\Session Manager\PendingFileRenameOperations` +//! (the boot-time session manager reads from HKLM, not the per-user hive), +//! so the call **may fail for non-administrator users** with `ERROR_ACCESS_DENIED`. +//! That is by design — Microsoft documents the elevation requirement on the +//! `MOVEFILE_DELAY_UNTIL_REBOOT` flag — and the caller in `lib.rs` handles +//! the failure path gracefully: it preserves the original lock error plus +//! the schedule failure reason and falls back to the "close all OpenHuman +//! windows and try again" guidance from PR #2395 / #1811. + +#![cfg(target_os = "windows")] + +use std::io; +use std::os::windows::ffi::OsStrExt; +use std::path::Path; + +use windows_sys::Win32::Storage::FileSystem::{MoveFileExW, MOVEFILE_DELAY_UNTIL_REBOOT}; + +/// Tally of entries handed off to `MoveFileExW`, returned to the caller so +/// it can log and surface (e.g. "scheduled 142 files / 14 dirs for deletion +/// on next reboot") instead of just an opaque "ok". +/// +/// `partial` is `true` when the walk aborted mid-tree (e.g. a directory +/// became unreadable, or an individual `MoveFileExW` call failed). In that +/// case `files` / `dirs` represent **only** what was queued before the +/// failure point — useful for support logs to distinguish "everything is +/// queued" from "some of the tree is queued but the rest still needs +/// manual cleanup." Pair with the `Result::Err` returned by +/// [`schedule_path_for_reboot_deletion`] for the cause. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub struct RebootDeletionSchedule { + pub files: u32, + pub dirs: u32, + pub partial: bool, +} + +impl RebootDeletionSchedule { + pub fn total(&self) -> u32 { + self.files.saturating_add(self.dirs) + } +} + +/// Schedule `path` (and everything under it if it is a directory) for +/// deletion on the next reboot via `MoveFileExW(_, NULL, MOVEFILE_DELAY_UNTIL_REBOOT)`. +/// +/// Strategy: +/// * Regular files / symlinks → scheduled directly. +/// * Directories → children scheduled first (depth-first), then the +/// directory itself once its contents are queued. +/// +/// `path` not existing on disk yields `Err(RebootDeletionFailure { error: NotFound, .. })` — +/// callers can choose to treat that as a no-op since "nothing to remove" is +/// the same outcome. +/// +/// On error the failure carries a partially-populated `RebootDeletionSchedule` +/// (`partial = true`) so the caller can surface "we queued N files and M +/// folders before scheduling failed" instead of just the bare io error. +/// The walk is depth-first, so the counts reflect entries queued *before* +/// the failing step. +pub fn schedule_path_for_reboot_deletion( + path: &Path, +) -> Result { + schedule_path_with_scheduler(path, &mut schedule_one) +} + +/// Internal seam used by both [`schedule_path_for_reboot_deletion`] (which +/// passes the real `MoveFileExW` step as `scheduler`) and the unit tests +/// (which pass an injectable `Ok(())` stub so the traversal/counting logic +/// can be exercised on every dev machine without needing administrator +/// rights or actually queuing reboot-time deletions). +fn schedule_path_with_scheduler( + path: &Path, + scheduler: &mut F, +) -> Result +where + F: FnMut(&Path) -> io::Result<()>, +{ + let metadata = std::fs::symlink_metadata(path).map_err(|error| RebootDeletionFailure { + error, + partial: RebootDeletionSchedule { + partial: true, + ..RebootDeletionSchedule::default() + }, + })?; + let mut summary = RebootDeletionSchedule::default(); + match schedule_inner(path, &metadata, &mut summary, scheduler) { + Ok(()) => Ok(summary), + Err(error) => { + summary.partial = true; + Err(RebootDeletionFailure { + error, + partial: summary, + }) + } + } +} + +/// Pair of `(io::Error, partial schedule)` returned when the depth-first +/// walk aborts mid-tree. The `partial` field records what was queued via +/// `MoveFileExW` *before* the failure point so the caller can include the +/// counts in user-facing copy and support logs ("123 files / 7 folders +/// were queued for the next reboot before scheduling failed: "). +#[derive(Debug)] +pub struct RebootDeletionFailure { + pub error: io::Error, + pub partial: RebootDeletionSchedule, +} + +impl std::fmt::Display for RebootDeletionFailure { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.error) + } +} + +impl std::error::Error for RebootDeletionFailure { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(&self.error) + } +} + +fn schedule_inner( + path: &Path, + metadata: &std::fs::Metadata, + summary: &mut RebootDeletionSchedule, + scheduler: &mut F, +) -> io::Result<()> +where + F: FnMut(&Path) -> io::Result<()>, +{ + // Symlinked directories must NOT be descended into — the lock lives + // on the link target, not the link itself, and following would queue + // unrelated paths for deletion. Treat symlinks (file or dir) as a + // single leaf entry. + if metadata.is_dir() && !metadata.file_type().is_symlink() { + for entry in std::fs::read_dir(path)? { + let entry = entry?; + let child_meta = entry.metadata()?; + schedule_inner(&entry.path(), &child_meta, summary, scheduler)?; + } + scheduler(path)?; + summary.dirs = summary.dirs.saturating_add(1); + } else { + scheduler(path)?; + summary.files = summary.files.saturating_add(1); + } + Ok(()) +} + +fn schedule_one(path: &Path) -> io::Result<()> { + // `MoveFileExW + MOVEFILE_DELAY_UNTIL_REBOOT` requires absolute paths — + // the session manager runs at boot before any working directory is + // established, so a relative path cannot be resolved. The call sites + // in `reset_local_data` already resolve paths via the core's + // `config_get_data_paths` RPC (which returns absolute paths) so this + // is currently a no-op in release builds; the assert catches a future + // regression that wires a different caller in without thinking. + debug_assert!( + path.is_absolute(), + "MoveFileExW + DELAY_UNTIL_REBOOT requires an absolute path, got {}", + path.display() + ); + let wide: Vec = path + .as_os_str() + .encode_wide() + .chain(std::iter::once(0)) + .collect(); + // SAFETY: `wide` is a NUL-terminated UTF-16 buffer that outlives the + // call. The destination pointer is `NULL`, which (combined with + // `MOVEFILE_DELAY_UNTIL_REBOOT`) tells Windows to delete (rather than + // rename) the source at the next boot. `MoveFileExW` returns BOOL — + // non-zero on success. + let ok = unsafe { MoveFileExW(wide.as_ptr(), std::ptr::null(), MOVEFILE_DELAY_UNTIL_REBOOT) }; + if ok == 0 { + Err(io::Error::last_os_error()) + } else { + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Test-only no-op scheduler. Lets the traversal/counting tests run + /// in any user context (incl. non-administrator) by sidestepping the + /// `MoveFileExW + MOVEFILE_DELAY_UNTIL_REBOOT` call that would + /// otherwise need HKLM write access. We capture the call order so a + /// regression in depth-first ordering would surface as a wrong path + /// sequence here, even though the real OS-side scheduling stays + /// out of the test process. + fn noop_scheduler( + captured: &mut Vec, + ) -> impl FnMut(&Path) -> io::Result<()> + '_ { + move |path: &Path| { + captured.push(path.to_path_buf()); + Ok(()) + } + } + + #[test] + fn schedule_walks_files_then_dirs() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path().join("reset-target"); + std::fs::create_dir_all(root.join("nested")).expect("mkdir nested"); + std::fs::write(root.join("a.txt"), b"a").expect("write a.txt"); + std::fs::write(root.join("nested").join("b.txt"), b"b").expect("write b.txt"); + + let mut captured = Vec::new(); + let summary = + schedule_path_with_scheduler(&root, &mut noop_scheduler(&mut captured)).expect("walk"); + + // root + nested == 2 dirs; a.txt + nested/b.txt == 2 files + assert_eq!(summary.files, 2, "expected 2 files queued, got {summary:?}"); + assert_eq!(summary.dirs, 2, "expected 2 dirs queued, got {summary:?}"); + assert_eq!(summary.total(), 4); + assert!(!summary.partial, "Ok must not flag partial"); + + // Depth-first: a parent must only appear after all of its children. + // Track per-path positions in the call order, then assert each + // directory sits after every entry whose path is rooted inside it. + let position = |needle: &Path| -> usize { + captured + .iter() + .position(|p| p == needle) + .unwrap_or_else(|| panic!("missing {} in {captured:?}", needle.display())) + }; + let root_pos = position(&root); + let nested_pos = position(&root.join("nested")); + let a_pos = position(&root.join("a.txt")); + let b_pos = position(&root.join("nested").join("b.txt")); + assert!( + b_pos < nested_pos, + "b.txt must be scheduled before its parent (nested)" + ); + assert!( + nested_pos < root_pos && a_pos < root_pos, + "nested + a.txt must be scheduled before root" + ); + } + + #[test] + fn schedule_single_file_reports_one_file() { + let dir = tempfile::tempdir().expect("tempdir"); + let file = dir.path().join("solo.txt"); + std::fs::write(&file, b"x").expect("write solo.txt"); + + let mut captured = Vec::new(); + let summary = + schedule_path_with_scheduler(&file, &mut noop_scheduler(&mut captured)).expect("walk"); + + assert_eq!( + summary, + RebootDeletionSchedule { + files: 1, + dirs: 0, + partial: false, + } + ); + assert_eq!(captured, vec![file]); + } + + #[test] + fn schedule_missing_path_yields_not_found() { + let dir = tempfile::tempdir().expect("tempdir"); + let missing = dir.path().join("does-not-exist"); + + let mut captured = Vec::new(); + let failure = schedule_path_with_scheduler(&missing, &mut noop_scheduler(&mut captured)) + .expect_err("missing"); + assert_eq!(failure.error.kind(), io::ErrorKind::NotFound); + // Nothing scheduled, but partial flag still reports "did not + // complete" so callers can distinguish from a clean success. + assert!(failure.partial.partial); + assert_eq!(failure.partial.files, 0); + assert_eq!(failure.partial.dirs, 0); + assert!( + captured.is_empty(), + "no scheduling should have been attempted: {captured:?}" + ); + } + + #[test] + fn schedule_empty_dir_counts_one_dir() { + let dir = tempfile::tempdir().expect("tempdir"); + let empty = dir.path().join("empty-target"); + std::fs::create_dir(&empty).expect("mkdir empty-target"); + + let mut captured = Vec::new(); + let summary = + schedule_path_with_scheduler(&empty, &mut noop_scheduler(&mut captured)).expect("walk"); + + assert_eq!( + summary, + RebootDeletionSchedule { + files: 0, + dirs: 1, + partial: false, + } + ); + assert_eq!(captured, vec![empty]); + } + + #[test] + fn schedule_propagates_scheduler_failure_with_partial_counts() { + // Simulate the non-administrator MoveFileExW failure path: the + // walk visits children successfully, then the third call (the + // parent dir) errors out. The Err must carry the leaf counts + // queued before the failure so the caller can surface "we did + // get X files scheduled before this hit the registry wall." + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path().join("partial-fail"); + std::fs::create_dir_all(&root).expect("mkdir"); + std::fs::write(root.join("a.txt"), b"a").expect("write a.txt"); + std::fs::write(root.join("b.txt"), b"b").expect("write b.txt"); + + let root_path = root.clone(); + let mut count = 0usize; + let mut scheduler = |path: &Path| -> io::Result<()> { + count += 1; + if path == root_path { + Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "simulated non-admin MoveFileExW failure", + )) + } else { + Ok(()) + } + }; + + let failure = + schedule_path_with_scheduler(&root, &mut scheduler).expect_err("scheduler failed"); + assert_eq!(failure.error.kind(), io::ErrorKind::PermissionDenied); + assert!(failure.partial.partial); + // Both leaf files were scheduled before the parent-dir call failed. + assert_eq!(failure.partial.files, 2, "got {:?}", failure.partial); + assert_eq!(failure.partial.dirs, 0, "got {:?}", failure.partial); + // Sanity: scheduler was called for both leaves + the parent. + assert_eq!(count, 3); + } +} diff --git a/src/core/logging.rs b/src/core/logging.rs index b4aba3821..323caaefe 100644 --- a/src/core/logging.rs +++ b/src/core/logging.rs @@ -14,7 +14,7 @@ use std::fmt; use std::io::{self, IsTerminal}; use std::path::{Path, PathBuf}; -use std::sync::{Once, OnceLock}; +use std::sync::{Mutex, Once, OnceLock}; use nu_ansi_term::{Color, Style}; use tracing::{Event, Level}; @@ -28,10 +28,19 @@ use tracing_subscriber::Layer; static INIT: Once = Once::new(); -/// Holds the non-blocking writer guard for the file appender. Must live for -/// the entire process lifetime — dropping it stops the background flushing -/// thread and silently swallows pending log records. -static FILE_GUARD: OnceLock = OnceLock::new(); +/// Holds the non-blocking writer guard for the file appender. Dropping it +/// stops the background flushing thread and releases the OS file handle on +/// the active `openhuman-YYYY-MM-DD.log`, which on Windows is required +/// before the parent `/logs/` directory can be removed (issue +/// #1615 — the file is held by the Tauri host process, not the embedded +/// core, so `CoreProcessHandle::shutdown` alone does not release it). +/// +/// Wrapped in `Mutex>` instead of `OnceLock` so [`shutdown_file_guard`] +/// can `take` and drop the guard mid-process during `reset_local_data`. +/// After a `take`, the file layer's writer becomes a no-op (the background +/// thread has exited); see [`shutdown_file_guard`] docs for the consequence +/// on subsequent log records. +static FILE_GUARD: Mutex> = Mutex::new(None); /// Resolved path to the active log file directory. Populated by /// [`init_for_embedded`] so UI commands (e.g. `reveal_logs_folder`) can find @@ -289,7 +298,9 @@ pub fn init_for_embedded(data_dir: &Path, verbose: bool) { { Ok(()) => { if let Some((_, guard, dir)) = pending_file { - let _ = FILE_GUARD.set(guard); + if let Ok(mut slot) = FILE_GUARD.lock() { + *slot = Some(guard); + } let _ = LOG_DIR.set(dir); } } @@ -315,6 +326,37 @@ pub fn log_directory() -> Option<&'static Path> { LOG_DIR.get().map(PathBuf::as_path) } +/// Drop the file appender's worker guard so the rolling `openhuman-*.log` +/// file handle held by *this* process is released. +/// +/// Returns `true` if a guard was taken (and dropped here), `false` if no +/// guard was installed (CLI run, init failed, or already shut down). After +/// this call: +/// +/// * the non-blocking writer's background thread exits as part of `drop`, +/// * the OS file handle on today's log file is closed, +/// * subsequent `tracing::*` records routed to the file layer are silently +/// discarded (the writer becomes a no-op) until the process restarts. +/// +/// **Used by**: the Tauri shell's `reset_local_data` command, which must be +/// able to `remove_dir_all()` on Windows. Without releasing this +/// guard the host process holds an open handle inside `/logs/` +/// and Windows returns `ERROR_SHARING_VIOLATION` (os error 32). The stderr +/// and Sentry layers stay attached because they don't keep files open. +/// +/// **Not idempotent in the recover-after-call sense**: there is no re-init +/// path. A subsequent `init_for_embedded` is a no-op (the `Once` guard has +/// already fired), so file logging stays off until the next process launch. +/// `reset_local_data` is followed by `ensure_running()` which restarts the +/// embedded core but does *not* re-install the subscriber — by design, the +/// user is expected to restart the app shortly after a reset. +pub fn shutdown_file_guard() -> bool { + let Ok(mut slot) = FILE_GUARD.lock() else { + return false; + }; + slot.take().is_some() +} + fn seed_rust_log(verbose: bool, default_scope: CliLogDefault) { if std::env::var_os("RUST_LOG").is_some() { return; @@ -378,6 +420,14 @@ mod tests { /// concurrent env-var writes would race. static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + /// Serialize tests that mutate the process-global `FILE_GUARD` static. + /// Without this, `shutdown_file_guard_takes_installed_guard` can race + /// any concurrent test that calls `init_for_embedded` (or that itself + /// stashes / takes the guard), making one of them observe a guard it + /// did not install. Mirror of the `SCHEDULE_LOCK` pattern in + /// `app/src-tauri/src/reset_reboot_schedule.rs::tests`. + static FILE_GUARD_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + fn with_clean_rust_log(f: impl FnOnce() -> R) -> R { let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); let prior = std::env::var("RUST_LOG").ok(); @@ -488,4 +538,43 @@ mod tests { assert!(log_directory().is_none()); } } + + #[test] + fn shutdown_file_guard_takes_installed_guard() { + let _g = FILE_GUARD_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + + // Simulate `init_for_embedded` having stashed a writer guard, then + // assert that `shutdown_file_guard` empties the slot and reports + // truthfully. + let dir = tempfile::tempdir().expect("tempdir for guard test"); + let appender = tracing_appender::rolling::Builder::new() + .rotation(tracing_appender::rolling::Rotation::DAILY) + .filename_prefix("guard-test") + .filename_suffix("log") + .build(dir.path()) + .expect("rolling appender for guard test"); + let (_writer, guard) = tracing_appender::non_blocking(appender); + { + let mut slot = FILE_GUARD.lock().expect("file guard mutex poisoned"); + // Save any pre-existing guard (a prior test in this process may + // have installed one) and restore it after the assertion runs. + let prior = slot.replace(guard); + drop(slot); + + assert!( + shutdown_file_guard(), + "expected shutdown_file_guard to take the installed guard" + ); + assert!( + !shutdown_file_guard(), + "second call must be a no-op when the slot is already empty" + ); + + // Restore the prior guard so unrelated tests that depend on + // `init_for_embedded` having installed one are not surprised. + if let Ok(mut slot) = FILE_GUARD.lock() { + *slot = prior; + } + } + } }