From 2e778e90db600bf83a4f152fa1a77d27317d7ed0 Mon Sep 17 00:00:00 2001 From: slvnlrt Date: Tue, 24 Mar 2026 17:08:31 +0100 Subject: [PATCH 1/4] fix(locations): canonicalize paths and register watcher on location add MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two upstream bugs fixed: 1. LocationManager::add_location() stored paths as-is without canonicalization. Relative paths (e.g. from cwd-dependent contexts) broke the watcher, volume manager, and indexer pipelines. Now calls tokio::fs::canonicalize() on local physical paths before storing, with UNC prefix stripping on Windows. 2. LocationAddAction::execute() never registered new locations with the FsWatcherService. The watcher only discovered locations at startup via load_library_locations(). Any location added at runtime had no filesystem monitoring — creates, deletes, and renames went undetected. Now calls fs_watcher.watch_location() after successful creation. --- core/src/location/manager.rs | 34 ++++++++++++++++++++++++++++ core/src/ops/locations/add/action.rs | 25 ++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/core/src/location/manager.rs b/core/src/location/manager.rs index c57381d19b16..aa8d52c72915 100644 --- a/core/src/location/manager.rs +++ b/core/src/location/manager.rs @@ -46,6 +46,40 @@ impl LocationManager { job_policies: Option, volume_manager: &crate::volume::VolumeManager, ) -> LocationResult<(Uuid, String)> { + // Canonicalize local physical paths to absolute form before storing. + // Relative paths break the watcher, volume resolution, and indexer. + // Only for local device — remote paths can't be resolved locally. + let sd_path = if sd_path.is_local() { + if let crate::domain::addressing::SdPath::Physical { device_slug, path } = sd_path { + let canonical = tokio::fs::canonicalize(&path).await.map_err(|e| { + LocationError::InvalidPath(format!( + "Failed to resolve path {}: {}", + path.display(), + e + )) + })?; + // On Windows, canonicalize() returns UNC paths (\\?\C:\...) which break + // starts_with() matching throughout the codebase. Strip the prefix. + #[cfg(windows)] + let canonical = { + let s = canonical.to_string_lossy(); + if let Some(stripped) = s.strip_prefix(r"\\?\") { + std::path::PathBuf::from(stripped) + } else { + canonical + } + }; + crate::domain::addressing::SdPath::Physical { + device_slug, + path: canonical, + } + } else { + sd_path + } + } else { + sd_path + }; + info!("Adding location: {}", sd_path); // Validate the path based on type diff --git a/core/src/ops/locations/add/action.rs b/core/src/ops/locations/add/action.rs index f77b09df11dc..4d6edec43763 100644 --- a/core/src/ops/locations/add/action.rs +++ b/core/src/ops/locations/add/action.rs @@ -102,6 +102,31 @@ impl LibraryAction for LocationAddAction { .await .map_err(|e| ActionError::Internal(e.to_string()))?; + // Register the new location with the filesystem watcher so changes + // (creates, deletes, renames) are detected in real-time. + // Without this, the watcher only learns about locations at startup. + if let Some(local_path) = self.input.path.as_local_path() { + if let Some(fs_watcher) = context.get_fs_watcher().await { + use crate::ops::indexing::handlers::LocationMeta; + use crate::ops::indexing::RuleToggles; + + // Use canonical path to match what add_location stored in DB + let root_path = tokio::fs::canonicalize(local_path) + .await + .unwrap_or_else(|_| local_path.to_path_buf()); + + let meta = LocationMeta { + id: location_id, + library_id: library.id(), + root_path, + rule_toggles: RuleToggles::default(), + }; + if let Err(e) = fs_watcher.watch_location(meta).await { + tracing::warn!("Failed to register location with watcher: {}", e); + } + } + } + // Parse the job ID from the string returned by add_location let job_id = if !job_id_string.is_empty() { Some( From 9004d868fbc05cf152fb06cf06be65df816f3051 Mon Sep 17 00:00:00 2001 From: slvnlrt Date: Tue, 24 Mar 2026 22:07:29 +0100 Subject: [PATCH 2/4] fix(windows): handle UNC network paths in canonicalize normalization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit canonicalize() on Windows can produce \?\UNC\server\share\... for network paths. The previous strip_prefix(r"\?\") would produce UNC\server\share\... which is invalid. Now handles both forms: - \?\UNC\server\share\... → \server\share\... (network UNC) - \?\C:\... → C:\... (local drive) Applied in both manager.rs (add_location) and add/action.rs (watcher registration) to ensure consistency. Same normalization as volume/fs/refs.rs:contains_path(). Co-Authored-By: Claude Opus 4.6 (1M context) --- core/src/location/manager.rs | 18 +++++++++++++----- core/src/ops/locations/add/action.rs | 17 ++++++++++++++++- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/core/src/location/manager.rs b/core/src/location/manager.rs index aa8d52c72915..7fbd20bbba2e 100644 --- a/core/src/location/manager.rs +++ b/core/src/location/manager.rs @@ -58,13 +58,21 @@ impl LocationManager { e )) })?; - // On Windows, canonicalize() returns UNC paths (\\?\C:\...) which break - // starts_with() matching throughout the codebase. Strip the prefix. + // On Windows, canonicalize() returns extended path prefixes that break + // starts_with() matching throughout the codebase. + // \\?\UNC\server\share\... → \\server\share\... (network UNC) + // \\?\C:\... → C:\... (local drive) + // Same normalization as volume/fs/refs.rs:contains_path() #[cfg(windows)] let canonical = { - let s = canonical.to_string_lossy(); - if let Some(stripped) = s.strip_prefix(r"\\?\") { - std::path::PathBuf::from(stripped) + if let Some(s) = canonical.to_str() { + if s.starts_with(r"\\?\UNC\") { + std::path::PathBuf::from(format!(r"\\{}", &s[8..])) + } else if let Some(stripped) = s.strip_prefix(r"\\?\") { + std::path::PathBuf::from(stripped) + } else { + canonical + } } else { canonical } diff --git a/core/src/ops/locations/add/action.rs b/core/src/ops/locations/add/action.rs index 4d6edec43763..890c34986612 100644 --- a/core/src/ops/locations/add/action.rs +++ b/core/src/ops/locations/add/action.rs @@ -110,10 +110,25 @@ impl LibraryAction for LocationAddAction { use crate::ops::indexing::handlers::LocationMeta; use crate::ops::indexing::RuleToggles; - // Use canonical path to match what add_location stored in DB + // Use canonical path to match what add_location stored in DB, + // with the same Windows prefix normalization as manager.rs let root_path = tokio::fs::canonicalize(local_path) .await .unwrap_or_else(|_| local_path.to_path_buf()); + #[cfg(windows)] + let root_path = { + if let Some(s) = root_path.to_str() { + if s.starts_with(r"\\?\UNC\") { + std::path::PathBuf::from(format!(r"\\{}", &s[8..])) + } else if let Some(stripped) = s.strip_prefix(r"\\?\") { + std::path::PathBuf::from(stripped) + } else { + root_path + } + } else { + root_path + } + }; let meta = LocationMeta { id: location_id, From 1d7097d64c9ba5e4de0f9fe7e16c52699a06b344 Mon Sep 17 00:00:00 2001 From: slvnlrt Date: Wed, 25 Mar 2026 14:44:43 +0100 Subject: [PATCH 3/4] refactor(locations): extract UNC path helper, add unwatch on removal - Extract duplicated Windows extended path normalization into shared `common::utils::strip_windows_extended_prefix()` helper (was copy-pasted in location/manager.rs, locations/add/action.rs, volume/fs/refs.rs) - Add `unwatch_location()` call in LocationRemoveAction to stop the filesystem watcher when a location is deleted (symmetric with the `watch_location()` added in LocationAddAction) Co-Authored-By: Claude Opus 4.6 (1M context) --- core/src/common/utils.rs | 30 +++++++++++++++++++++++++ core/src/location/manager.rs | 20 +---------------- core/src/ops/locations/add/action.rs | 17 +------------- core/src/ops/locations/remove/action.rs | 13 ++++++++++- core/src/volume/fs/refs.rs | 13 +---------- 5 files changed, 45 insertions(+), 48 deletions(-) diff --git a/core/src/common/utils.rs b/core/src/common/utils.rs index ff900fedb38e..c5792abef26d 100644 --- a/core/src/common/utils.rs +++ b/core/src/common/utils.rs @@ -3,3 +3,33 @@ // Note: Device ID management has been moved to device::manager for better // module organization. Import from there instead: // use crate::device::manager::{get_current_device_id, set_current_device_id}; + +/// Strip Windows extended path prefixes produced by `std::fs::canonicalize()`. +/// +/// On Windows, `canonicalize()` returns paths like `\\?\C:\...` (local) or +/// `\\?\UNC\server\share\...` (network). These prefixes break `starts_with()` +/// matching throughout the codebase and must be normalized. +/// +/// - `\\?\UNC\server\share\...` → `\\server\share\...` +/// - `\\?\C:\...` → `C:\...` +/// - All other paths are returned unchanged. +#[cfg(windows)] +pub fn strip_windows_extended_prefix(path: std::path::PathBuf) -> std::path::PathBuf { + if let Some(s) = path.to_str() { + if s.starts_with(r"\\?\UNC\") { + std::path::PathBuf::from(format!(r"\\{}", &s[8..])) + } else if let Some(stripped) = s.strip_prefix(r"\\?\") { + std::path::PathBuf::from(stripped) + } else { + path + } + } else { + path + } +} + +/// No-op on non-Windows platforms. +#[cfg(not(windows))] +pub fn strip_windows_extended_prefix(path: std::path::PathBuf) -> std::path::PathBuf { + path +} diff --git a/core/src/location/manager.rs b/core/src/location/manager.rs index 7fbd20bbba2e..037c61cda413 100644 --- a/core/src/location/manager.rs +++ b/core/src/location/manager.rs @@ -58,25 +58,7 @@ impl LocationManager { e )) })?; - // On Windows, canonicalize() returns extended path prefixes that break - // starts_with() matching throughout the codebase. - // \\?\UNC\server\share\... → \\server\share\... (network UNC) - // \\?\C:\... → C:\... (local drive) - // Same normalization as volume/fs/refs.rs:contains_path() - #[cfg(windows)] - let canonical = { - if let Some(s) = canonical.to_str() { - if s.starts_with(r"\\?\UNC\") { - std::path::PathBuf::from(format!(r"\\{}", &s[8..])) - } else if let Some(stripped) = s.strip_prefix(r"\\?\") { - std::path::PathBuf::from(stripped) - } else { - canonical - } - } else { - canonical - } - }; + let canonical = crate::common::utils::strip_windows_extended_prefix(canonical); crate::domain::addressing::SdPath::Physical { device_slug, path: canonical, diff --git a/core/src/ops/locations/add/action.rs b/core/src/ops/locations/add/action.rs index 890c34986612..ecf1d92cfacd 100644 --- a/core/src/ops/locations/add/action.rs +++ b/core/src/ops/locations/add/action.rs @@ -110,25 +110,10 @@ impl LibraryAction for LocationAddAction { use crate::ops::indexing::handlers::LocationMeta; use crate::ops::indexing::RuleToggles; - // Use canonical path to match what add_location stored in DB, - // with the same Windows prefix normalization as manager.rs let root_path = tokio::fs::canonicalize(local_path) .await .unwrap_or_else(|_| local_path.to_path_buf()); - #[cfg(windows)] - let root_path = { - if let Some(s) = root_path.to_str() { - if s.starts_with(r"\\?\UNC\") { - std::path::PathBuf::from(format!(r"\\{}", &s[8..])) - } else if let Some(stripped) = s.strip_prefix(r"\\?\") { - std::path::PathBuf::from(stripped) - } else { - root_path - } - } else { - root_path - } - }; + let root_path = crate::common::utils::strip_windows_extended_prefix(root_path); let meta = LocationMeta { id: location_id, diff --git a/core/src/ops/locations/remove/action.rs b/core/src/ops/locations/remove/action.rs index 7ac73207e68c..43e722a2bb2e 100644 --- a/core/src/ops/locations/remove/action.rs +++ b/core/src/ops/locations/remove/action.rs @@ -43,13 +43,24 @@ impl LibraryAction for LocationRemoveAction { library: std::sync::Arc, context: Arc, ) -> Result { - // Remove the location + // Remove the location from DB let location_manager = LocationManager::new(context.events.as_ref().clone()); location_manager .remove_location(&library, self.input.location_id) .await .map_err(|e| ActionError::Internal(e.to_string()))?; + // Unwatch the location from the filesystem watcher + if let Some(watcher) = context.get_fs_watcher().await { + if let Err(e) = watcher.unwatch_location(self.input.location_id).await { + tracing::warn!( + "Failed to unwatch location {}: {}", + self.input.location_id, + e + ); + } + } + Ok(LocationRemoveOutput::new(self.input.location_id, None)) } diff --git a/core/src/volume/fs/refs.rs b/core/src/volume/fs/refs.rs index c398268afb8d..cf2496daa6a2 100644 --- a/core/src/volume/fs/refs.rs +++ b/core/src/volume/fs/refs.rs @@ -293,18 +293,7 @@ impl super::FilesystemHandler for RefsHandler { } fn contains_path(&self, volume: &Volume, path: &std::path::Path) -> bool { - // Strip Windows extended path prefix (\\?\) produced by canonicalize() - let normalized_path = if let Some(path_str) = path.to_str() { - if path_str.starts_with("\\\\?\\UNC\\") { - PathBuf::from(format!("\\\\{}", &path_str[8..])) - } else if let Some(stripped) = path_str.strip_prefix("\\\\?\\") { - PathBuf::from(stripped) - } else { - path.to_path_buf() - } - } else { - path.to_path_buf() - }; + let normalized_path = crate::common::utils::strip_windows_extended_prefix(path.to_path_buf()); if normalized_path.starts_with(&volume.mount_point) { return true; From 7b867ef4f1ca1f393e51ec093ecdcd2f435612d6 Mon Sep 17 00:00:00 2001 From: slvnlrt Date: Wed, 25 Mar 2026 15:04:22 +0100 Subject: [PATCH 4/4] fix(windows): only strip \?\ prefix on drive-letter paths Volume GUIDs (\?\Volume{...}\) and other verbatim forms are invalid without the prefix. Only strip when followed by a drive letter (X:\). Co-Authored-By: Claude Opus 4.6 (1M context) --- core/src/common/utils.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/core/src/common/utils.rs b/core/src/common/utils.rs index c5792abef26d..07584b5a73ce 100644 --- a/core/src/common/utils.rs +++ b/core/src/common/utils.rs @@ -17,9 +17,17 @@ pub fn strip_windows_extended_prefix(path: std::path::PathBuf) -> std::path::PathBuf { if let Some(s) = path.to_str() { if s.starts_with(r"\\?\UNC\") { + // \\?\UNC\server\share\... → \\server\share\... std::path::PathBuf::from(format!(r"\\{}", &s[8..])) } else if let Some(stripped) = s.strip_prefix(r"\\?\") { - std::path::PathBuf::from(stripped) + // Only strip \\?\ when followed by a drive letter (e.g. C:\). + // Leave volume GUIDs (\\?\Volume{...}\) and other verbatim + // forms untouched — they are invalid without the prefix. + if stripped.as_bytes().get(1) == Some(&b':') { + std::path::PathBuf::from(stripped) + } else { + path + } } else { path }