From 82a79e47416941fbf45fa659cf7f398388ebe82e Mon Sep 17 00:00:00 2001 From: tsushanth <78000697+tsushanth@users.noreply.github.com> Date: Mon, 6 Jul 2026 20:31:54 -0700 Subject: [PATCH] fix(windows): strip \\?\ prefix from canonicalized paths before disk mount-point matching (#13045) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #13041. ## Root cause Two broken string comparisons, both caused by Windows path canonicalization: **1. `src/config/src/config.rs` — `check_disk_cache_config()`** `Path::canonicalize()` on Windows returns a verbatim extended-length path (`\\?\C:\OpenObserve\data\cache`), but `sysinfo` returns plain mount points (`C:\`). The `starts_with` check `"\\?\C:\...".starts_with("C:\\")` is always `false` → `disk_total = disk_free = 0` on every Windows deployment. **2. `src/job/stats.rs` — `update_node_disk_usage()`** The comparison was also reversed: `mount_point.starts_with(data_dir)` checks whether the mount point path begins with the (much longer) data directory path — `"C:\\".starts_with("C:\\OpenObserve\\data\\")` is always `false`. The correct direction is `data_dir.starts_with(mount_point)`. ## Consequences when disk sizes are 0 - Disk cache `max_size` is computed from near-zero `effective_free`, leaving `result_max_size = 0` - `disk.rs` GC panics at first tick: `max_size / result_max_size` evaluates before `max(1, …)` → divide by zero, GC task dies permanently - `check_disk_circuit_breaker()` always compares `0 > 0` → never trips → no self-protection against disk exhaustion on Windows ## Fix 1. **`config.rs`**: strip the `\\?\` prefix from the canonicalized cache dir string before the `starts_with` lookup against sysinfo mount points. 2. **`stats.rs`**: fix the reversed comparison (`mount_point.starts_with(data_dir)` → `data_dir.starts_with(mount_point)`) and apply the same `\\?\` prefix strip so canonicalized data dirs match correctly. Both fixes are no-ops on Linux/macOS where `canonicalize()` never produces the `\\?\` prefix. --------- Co-authored-by: Hengfei Yang --- src/config/src/config.rs | 58 ++++++++++++++++++++++++++++++++++++++-- src/job/stats.rs | 19 ++++++------- 2 files changed, 66 insertions(+), 11 deletions(-) diff --git a/src/config/src/config.rs b/src/config/src/config.rs index 2b0a1f35e0..13ee08ec49 100644 --- a/src/config/src/config.rs +++ b/src/config/src/config.rs @@ -3252,12 +3252,36 @@ fn check_memory_config(cfg: &mut Config) -> Result<(), anyhow::Error> { Ok(()) } +/// Strip the Windows extended-length prefix (`\\?\`) from a canonicalized path +/// so it can be compared with sysinfo mount points that use the plain DOS form. +/// +/// Uses [`std::path::Prefix`] to detect verbatim prefixes rather than +/// manipulating the string directly, which would silently break on non-ASCII +/// drive letters or UNC paths. +pub fn deverbatim(path: &Path) -> std::borrow::Cow<'_, str> { + #[cfg(windows)] + { + use std::path::{Component, Prefix}; + if let Some(Component::Prefix(p)) = path.components().next() { + if let Prefix::VerbatimDisk(drive) = p.kind() { + // \\?\C:\rest → C:\rest + // p.as_os_str() is "\\?\C:" (6 bytes); the remainder of the + // original string is "\rest", so prepend the plain drive letter. + let after_prefix = &path.to_string_lossy()[p.as_os_str().len()..]; + return format!("{}:{}", drive as char, after_prefix).into(); + } + } + } + path.to_string_lossy() +} + fn check_disk_cache_config(cfg: &mut Config) -> Result<(), anyhow::Error> { std::fs::create_dir_all(&cfg.common.data_cache_dir).expect("create cache dir success"); - let cache_dir = Path::new(&cfg.common.data_cache_dir) + let cache_dir_path = Path::new(&cfg.common.data_cache_dir) .canonicalize() .unwrap(); - let cache_dir = cache_dir.to_str().unwrap(); + let cache_dir_owned = deverbatim(&cache_dir_path).into_owned(); + let cache_dir = cache_dir_owned.as_str(); // disable disk cache for local disk storage if cfg.common.is_local_storage @@ -4333,4 +4357,34 @@ mod tests { let name = get_cluster_name(); assert!(!name.is_empty(), "cluster name should not be empty"); } + + #[test] + fn test_deverbatim_plain_path_unchanged() { + let p = std::path::Path::new("/data/openobserve"); + let result = deverbatim(p); + assert_eq!(result, "/data/openobserve"); + } + + #[test] + fn test_deverbatim_empty_path_unchanged() { + let p = std::path::Path::new(""); + let result = deverbatim(p); + assert_eq!(result, ""); + } + + #[cfg(windows)] + #[test] + fn test_deverbatim_verbatim_disk_stripped() { + let p = std::path::Path::new(r"\\?\C:\data\openobserve"); + let result = deverbatim(p); + assert_eq!(result, r"C:\data\openobserve"); + } + + #[cfg(windows)] + #[test] + fn test_deverbatim_plain_windows_path_unchanged() { + let p = std::path::Path::new(r"C:\data\openobserve"); + let result = deverbatim(p); + assert_eq!(result, r"C:\data\openobserve"); + } } diff --git a/src/job/stats.rs b/src/job/stats.rs index bf61e59fed..75691bbf29 100644 --- a/src/job/stats.rs +++ b/src/job/stats.rs @@ -13,7 +13,7 @@ // You should have received a copy of the GNU Affero General Public License // along with this program. If not, see . -use config::{cluster::LOCAL_NODE, get_config, spawn_pausable_job}; +use config::{cluster::LOCAL_NODE, deverbatim, get_config, spawn_pausable_job}; use tokio::time; use crate::service::{compact::stats::update_stats_from_file_list, db}; @@ -112,21 +112,22 @@ async fn update_node_memory_usage() -> Result<(), anyhow::Error> { // update node disk usage metrics every 60 seconds async fn update_node_disk_usage() -> Result<(), anyhow::Error> { let cfg = get_config(); - let data_dir = std::path::Path::new(&cfg.common.data_dir); + let data_dir_raw = std::path::Path::new(&cfg.common.data_dir) + .canonicalize() + .unwrap_or_else(|_| std::path::PathBuf::from(&cfg.common.data_dir)); + // Strip any Windows extended-length prefix (\\?\) returned by canonicalize + // so comparisons with sysinfo mount points (plain DOS form) work correctly. + let data_dir_norm = deverbatim(&data_dir_raw).into_owned(); loop { let disks = config::utils::sysinfo::disk::get_disk_usage(); - // Sum up all disks that contain subdirectories of the data directory - // This handles cases where multiple disks are mounted at different subpaths + // Sum up all disks whose mount point is a prefix of the data directory. + // This finds the disk(s) that actually host the data directory. let mut total_space = 0_u64; let mut total_used = 0_u64; for disk in disks { - // Check if the disk mount point is under the data directory - if disk - .mount_point - .starts_with(data_dir.to_string_lossy().as_ref()) - { + if data_dir_norm.starts_with(disk.mount_point.as_str()) { total_space += disk.total_space; total_used += disk.total_space - disk.available_space; }