SMB merge fix + move-pipeline logging

Bumps `smb2` to 0.8.0 and wires the new typed error variants into Cmdr:

- `map_smb_error`: `STATUS_OBJECT_NAME_COLLISION` → `VolumeError::AlreadyExists`
  instead of falling through to a generic IoError. This unblocks
  `copy_directory_streaming`'s merge-into-existing-directory path, which already
  swallows AlreadyExists. Without this, retrying a move into a partially-
  populated SMB directory blew up with "Protocol error: STATUS_OBJECT_NAME_COLLISION
  during Create".
- SMB delete fast-path: replaces the substring match against
  `"FILE_IS_A_DIRECTORY"` with a typed `is_file_is_a_directory(&smb2::Error)`
  predicate. Same for the `rename(force)` dest-cleanup path, which previously
  retried delete_directory on ANY delete_file error (could mask a real
  PermissionDenied as a futile second attempt).
- Reworded the misleading "SMB treats already-exists as a merge" comment in
  `volume_strategy.rs` — that wasn't true until smb2 0.8.0. Notes the version
  guard so future regressions are easier to spot.
- Added WARN logging in `move_between_volumes` for the copy and source-delete
  steps, plus the operation-level error path. The user's first failed move
  produced only a generic FE "io_error" with no backend trace; now the file
  path and raw `VolumeError` show up in `cmdr.log`.
- Added WARN logging in `LocalPosixVolume::delete` covering both the stat and
  the remove call, including the `io::ErrorKind` and raw errno. Same
  motivation: the move pipeline silently lost context here.

Tests:
- `map_smb_error_already_exists` and `is_file_is_a_directory_detects_typed_kind`
  pin the new mappings.
- All 1568 existing tests still pass.

Note: This commit does NOT yet address the FE error dialog softening
(`friendly_error_from_volume_error` integration) or the recursive-delete bug
in `move_between_volumes` that's the actual root cause of the original first-
attempt failure — see follow-up discussion.

# Conflicts:
#	apps/desktop/src-tauri/src/file_system/volume/smb.rs

Author: vdavid

Cargo.lock

@@ -163,7 +163,7 @@ mdns-sd = { version = "0.19", features = ["logging"] }
 # SMB2/3 protocol client for share enumeration (pure Rust, pipelined I/O).
 # When bumping: also re-vendor the test containers per
 # apps/desktop/test/smb-servers/.compose/VENDORED.md
-smb2 = "0.7.2"
+smb2 = "0.8.0"
 # NFD normalization for APFS collation and SMB path normalization
 unicode-normalization = "0.1"
 

apps/desktop/src-tauri/Cargo.toml

@@ -294,12 +294,34 @@ impl Volume for LocalPosixVolume {
         }
         Box::pin(async move {
             spawn_blocking(move || {
-                let metadata = std::fs::symlink_metadata(&abs_path)?;
-                if metadata.is_dir() {
-                    std::fs::remove_dir(&abs_path)?;
+                let metadata = std::fs::symlink_metadata(&abs_path).map_err(|e| {
+                    log::warn!(
+                        target: "local_posix",
+                        "delete: stat failed for {}: {} (kind={:?}, errno={:?})",
+                        abs_path.display(),
+                        e,
+                        e.kind(),
+                        e.raw_os_error()
+                    );
+                    e
+                })?;
+                let result = if metadata.is_dir() {
+                    std::fs::remove_dir(&abs_path)
                 } else {
-                    std::fs::remove_file(&abs_path)?;
-                }
+                    std::fs::remove_file(&abs_path)
+                };
+                result.map_err(|e| {
+                    log::warn!(
+                        target: "local_posix",
+                        "delete: {} {} failed: {} (kind={:?}, errno={:?})",
+                        if metadata.is_dir() { "remove_dir" } else { "remove_file" },
+                        abs_path.display(),
+                        e,
+                        e.kind(),
+                        e.raw_os_error()
+                    );
+                    e
+                })?;
                 Ok(())
             })
             .await

apps/desktop/src-tauri/src/file_system/volume/local_posix.rs

@@ -129,22 +129,13 @@ fn fs_info_to_space_info(info: &smb2::client::tree::FsInfo) -> SpaceInfo {
     }
 }
 
-/// Returns true if an `smb2::Error`'s display string indicates STATUS_FILE_IS_A_DIRECTORY.
-///
-/// smb2 0.7.x doesn't expose NT_STATUS as a typed enum yet — string match is the only option.
-fn is_file_is_a_directory_error(err: &smb2::Error) -> bool {
-    // allowed-error-string-match: smb2 0.7.x doesn't expose NT_STATUS as a typed enum yet
-    err.to_string().contains("FILE_IS_A_DIRECTORY")
-}
-
 /// Converts an `smb2::Error` to `VolumeError`.
 fn map_smb_error(err: smb2::Error) -> VolumeError {
     use smb2::ErrorKind;
-    if is_file_is_a_directory_error(&err) {
-        return VolumeError::IsADirectory(err.to_string());
-    }
     match err.kind() {
         ErrorKind::NotFound => VolumeError::NotFound(err.to_string()),
+        ErrorKind::AlreadyExists => VolumeError::AlreadyExists(err.to_string()),
+        ErrorKind::IsADirectory => VolumeError::IsADirectory(err.to_string()),
         ErrorKind::AccessDenied | ErrorKind::AuthRequired | ErrorKind::SigningRequired => {
             VolumeError::PermissionDenied(err.to_string())
         }
@@ -1291,7 +1282,7 @@ impl Volume for SmbVolume {
             match file_result {
                 Ok(()) => {} // File deleted successfully
                 Err(VolumeError::IsADirectory(_)) => {
-                    // It's a directory — try delete_directory
+                    // Expected fall-through: path is a directory, retry with delete_directory.
                     let (tree, mut conn) = self.clone_session().await?;
                     let r = tree.delete_directory(&mut conn, &smb_path).await;
                     self.handle_smb_result("delete_directory", r)?;
@@ -1335,16 +1326,26 @@ impl Volume for SmbVolume {
                 };
 
                 if dest_exists {
-                    // Try file delete first; if that fails (it's a dir), try directory delete
+                    // Try file delete first; if it fails specifically because the path is a
+                    // directory, try directory delete. Any other error (PermissionDenied,
+                    // SharingViolation, …) propagates immediately instead of being masked
+                    // by a second futile delete.
                     let file_result = {
                         let (tree, mut conn) = self.clone_session().await?;
                         let r = tree.delete_file(&mut conn, &smb_to).await;
                         self.handle_smb_result("rename(delete_dest_file)", r)
                     };
-                    if file_result.is_err() {
-                        let (tree, mut conn) = self.clone_session().await?;
-                        let r = tree.delete_directory(&mut conn, &smb_to).await;
-                        self.handle_smb_result("rename(delete_dest_dir)", r)?;
+                    match file_result {
+                        Ok(()) => {}
+                        Err(VolumeError::IsADirectory(_)) => {
+                            // Expected fall-through: dest is a directory, retry with delete_directory.
+                            // Any other error (PermissionDenied, SharingViolation, …) propagates immediately
+                            // instead of being masked by a second futile delete.
+                            let (tree, mut conn) = self.clone_session().await?;
+                            let r = tree.delete_directory(&mut conn, &smb_to).await;
+                            self.handle_smb_result("rename(delete_dest_dir)", r)?;
+                        }
+                        Err(e) => return Err(e),
                     }
                 }
             } else {
@@ -2040,29 +2041,42 @@ mod tests {
         assert!(matches!(ve, VolumeError::IoError { .. }));
     }
 
+    #[test]
+    fn map_smb_error_already_exists() {
+        // STATUS_OBJECT_NAME_COLLISION (returned by Create when the name exists) must
+        // surface as AlreadyExists so the volume_strategy merge-directory path can
+        // swallow it instead of bubbling a generic IO error to the user.
+        let err = smb2::Error::Protocol {
+            status: smb2::types::status::NtStatus::OBJECT_NAME_COLLISION,
+            command: smb2::types::Command::Create,
+        };
+        let ve = map_smb_error(err);
+        assert!(matches!(ve, VolumeError::AlreadyExists(_)));
+    }
+
     #[test]
     fn map_smb_error_file_is_a_directory() {
-        // STATUS_FILE_IS_A_DIRECTORY is returned by the server when delete_file is called on a dir.
-        // smb2 0.7.x surfaces this in the Display string, not as a typed ErrorKind variant.
+        // STATUS_FILE_IS_A_DIRECTORY is returned when delete_file is called on a dir.
+        // smb2 0.8.0 exposes this as the typed `ErrorKind::IsADirectory` variant, so
+        // `map_smb_error` surfaces it as `VolumeError::IsADirectory` — the delete
+        // fast-path matches on that to decide whether to retry with delete_directory.
         let err = smb2::Error::Protocol {
             status: smb2::types::status::NtStatus::FILE_IS_A_DIRECTORY,
             command: smb2::types::Command::Create,
         };
-        // Verify the helper correctly identifies the error.
-        assert!(is_file_is_a_directory_error(&err));
-        // Verify map_smb_error returns the typed variant.
         let ve = map_smb_error(err);
         assert!(matches!(ve, VolumeError::IsADirectory(_)));
     }
 
     #[test]
-    fn is_file_is_a_directory_error_negative() {
-        // Non-directory errors must not be misclassified.
+    fn map_smb_error_access_denied_is_not_misclassified() {
+        // Non-directory errors must not be classified as IsADirectory.
         let err = smb2::Error::Protocol {
             status: smb2::types::status::NtStatus::ACCESS_DENIED,
             command: smb2::types::Command::Create,
         };
-        assert!(!is_file_is_a_directory_error(&err));
+        let ve = map_smb_error(err);
+        assert!(matches!(ve, VolumeError::PermissionDenied(_)));
     }
 
     // ── Connection state tests ──────────────────────────────────────

apps/desktop/src-tauri/src/file_system/volume/smb.rs

@@ -197,13 +197,29 @@ pub async fn move_between_volumes(
                     &|| {},
                 )
                 .await
-                .map_err(|e| map_volume_error(&source_path.display().to_string(), e))?;
+                .map_err(|e| {
+                    log::warn!(
+                        target: "move",
+                        "move_between_volumes: copy phase failed for {} -> {}: {}",
+                        source_path.display(),
+                        dest_item.display(),
+                        e
+                    );
+                    map_volume_error(&source_path.display().to_string(), e)
+                })?;
 
-                // Delete source
-                source_volume
-                    .delete(source_path)
-                    .await
-                    .map_err(|e| map_volume_error(&source_path.display().to_string(), e))?;
+                // Delete source. Failures here leave a partial-move state (data at dest,
+                // sources still at origin) — log loudly so the cause is visible in the
+                // file log; without this the FE only sees a generic "io_error".
+                source_volume.delete(source_path).await.map_err(|e| {
+                    log::warn!(
+                        target: "move",
+                        "move_between_volumes: source delete failed for {} after successful copy: {}",
+                        source_path.display(),
+                        e
+                    );
+                    map_volume_error(&source_path.display().to_string(), e)
+                })?;
 
                 files_done += 1;
                 bytes_done += bytes;
@@ -262,6 +278,12 @@ pub async fn move_between_volumes(
                 log::info!("move_between_volumes: operation {} cancelled", operation_id_for_cleanup);
             }
             Err(e) => {
+                log::warn!(
+                    target: "move",
+                    "move operation {} failed: {:?}",
+                    operation_id_for_cleanup,
+                    e
+                );
                 let _ = app_for_error.emit(
                     "write-error",
                     WriteErrorEvent {
@@ -462,6 +484,12 @@ async fn move_within_same_volume(
                 log::info!("move_between_volumes: operation {} cancelled", operation_id_for_cleanup);
             }
             Err(e) => {
+                log::warn!(
+                    target: "move",
+                    "move operation {} failed: {:?}",
+                    operation_id_for_cleanup,
+                    e
+                );
                 let _ = app_for_error.emit(
                     "write-error",
                     WriteErrorEvent {

apps/desktop/src-tauri/src/file_system/write_operations/volume_move.rs

@@ -106,22 +106,20 @@ async fn copy_directory_streaming(
     on_file_progress: &(dyn Fn(u64, u64) -> ControlFlow<()> + Sync),
     on_file_complete: &(dyn Fn() + Sync),
 ) -> Result<u64, VolumeError> {
-    // Ensure the destination directory exists. The create_directory call is
-    // idempotent for in-memory/SMB/MTP (they treat "already exists" as a
-    // merge), but we catch `AlreadyExists` just in case for LocalPosix.
+    // Ensure the destination directory exists. Every backend is expected to
+    // surface "already exists" as `VolumeError::AlreadyExists`; we swallow it
+    // because that's the merge-into-existing-directory signal, not a failure.
+    // (SMB needed smb2 ≥ 0.8.0 to typed-classify STATUS_OBJECT_NAME_COLLISION;
+    // older versions leaked it as IoError and the merge path blew up.)
     match dest_volume.create_directory(dest_path).await {
         Ok(()) => {}
         Err(VolumeError::AlreadyExists(_)) => {}
-        Err(e) => {
-            // LocalPosix uses std::fs::create_dir which fails if the parent
-            // doesn't exist; retry with create_dir_all semantics via the
-            // default trait — but most backends implement this fine. Treat
-            // NotSupported as a signal to skip the explicit mkdir and hope
-            // write_from_stream creates parents as needed.
-            if !matches!(e, VolumeError::NotSupported) {
-                return Err(e);
-            }
+        Err(VolumeError::NotSupported) => {
+            // Backend can't create directories at all; assume `write_from_stream`
+            // will materialize parents on demand (LocalPosix does this via the
+            // default `create_dir_all` semantics).
         }
+        Err(e) => return Err(e),
     }
 
     let entries = source_volume.list_directory(source_path, None).await?;