Fix SMB streaming-write deadlock; bump smb2 to 0.9.0

.oxfmtrc.json

@@ -24,6 +24,7 @@
     "**/playwright-report/",
     "**/test-results/",
     "**/coverage/",
+    "apps/desktop/test/smb-servers/.compose/",
     "apps/website/src/pages/pricing.astro"
   ]
 }

apps/desktop/src-tauri/Cargo.toml

@@ -170,7 +170,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.8.0"
+smb2 = "0.9.1"
 # NFD normalization for APFS collation and SMB path normalization
 unicode-normalization = "0.1"
 

apps/desktop/src-tauri/src/commands/ui.rs

@@ -118,6 +118,24 @@ pub fn show_breadcrumb_context_menu<R: Runtime>(window: Window<R>, shortcut: Str
 #[tauri::command]
 #[specta::specta]
 pub fn show_main_window<R: Runtime>(window: Window<R>) -> Result<(), String> {
+    // E2E: on macOS, use `orderFront:` instead of `makeKeyAndOrderFront:` so the
+    // window appears without stealing focus from whatever the user is currently
+    // working in. `window.show()` calls the latter on macOS, which always grabs
+    // OS focus. Linux/Windows test runs happen in headless containers, so the
+    // standard show is fine there.
+    #[cfg(target_os = "macos")]
+    if crate::test_mode::is_e2e_mode() {
+        use objc2::msg_send;
+        use objc2::runtime::AnyObject;
+        let ns_window = window.ns_window().map_err(|e| e.to_string())? as *mut AnyObject;
+        if ns_window.is_null() {
+            return Err("NSWindow pointer is null".into());
+        }
+        unsafe {
+            let _: () = msg_send![ns_window, orderFront: std::ptr::null_mut::<AnyObject>()];
+        }
+        return Ok(());
+    }
     window.show().map_err(|e| e.to_string())
 }
 

apps/desktop/src-tauri/src/file_system/volume/CLAUDE.md

@@ -128,7 +128,7 @@ When adding a new volume, add a column for it and fill in each row. The matrix d
 Reads and writes have different shapes because the consumer relationship is different:
 
 - **Reads** return a `VolumeReadStream` that an external caller polls. The download handle has to live past the function call and cross async contexts. That's where the lifetime/ownership gymnastics below come from.
-- **Writes** consume a stream (or a local file) inside the method itself. The chunk loop is the consumer, so there's nothing to hand off. Just hold the session lock for the duration, pull chunks from the source, push them into the backend's chunk-by-chunk writer. `SmbVolume::import_single_file_with_progress` and `SmbVolume::write_from_stream` are the reference implementations: open the smb2 `FileWriter`, loop `write_chunk`, call `finish()` on success or `abort()` on cancel. No task spawn, no channel, no self-referential struct.
+- **Writes** consume a stream (or a local file) inside the method itself. The chunk loop is the consumer, so there's nothing to hand off. For backends with a `'static` writer (smb2 0.9's owned `FileWriter`, mtp-rs's `upload_stream`), drive the writer directly on a cloned session handle — no lock held across I/O. For backends whose writer borrows from the session, hold the session lock for the chunk loop's duration. `SmbVolume::write_from_stream` is the reference implementation: clone the session once, open the smb2 `FileWriter` on the clone, loop `write_chunk`, call `finish()` on success or `abort()` on cancel. No task spawn, no channel, no self-referential struct, no client mutex held while WRITEs are in flight.
 
 The rest of this section is about **read-side** lifetime handling. Which pattern to pick depends on whether your protocol SDK's download handle is `'static` or borrowed.
 
@@ -387,7 +387,7 @@ spawned detached task. This is safe because the stream always lives in an async
 **Why**: `SmbVolume` now handles listing updates via `notify_mutation` using its own smb2 `get_metadata`. The old `std::fs`-based synthetic diff path (`emit_synthetic_entry_diff`) is redundant and goes through the slow OS mount. Returning `false` skips it.
 
 **Decision**: `SmbVolume` splits session storage: `Arc<Mutex<Option<SmbClient>>>` + `Arc<RwLock<Option<Arc<Tree>>>>`
-**Why**: Phase 4 Fix 2 unblocks concurrency on the hot copy path. Previously the session lived in one `Mutex<Option<(SmbClient, Tree)>>`, which the streaming-read producer and the compound read/write fast-paths held for the entire transfer, serializing every concurrent copy through the mutex. With smb2 0.7.x, `Connection` is `Clone` (cheap `Arc::clone`, all clones multiplex frames over one SMB session). Splitting the Tree out lets us briefly lock the client, clone its `Connection`, and release the lock, then drive `Tree::download` / `Tree::read_file_compound` / `Tree::write_file_compound` on the cloned `Connection` with no lock held. N concurrent copies on one `SmbVolume` now pipeline N operations over the single session instead of queuing on the mutex. Tree lives in a `RwLock` because we only take read locks in the hot path (cloning an `Arc<Tree>`) and only write on disconnect. The legacy large-file streaming writer (`FileWriter<'a>`) still takes the client mutex for its duration because its lifetime borrows `&'a mut Connection` from the `SmbClient`; that's documented as a Gotcha below. Large files are rare in the hot path, the compound fast-path covers every small write.
+**Why**: Phase 4 Fix 2 unblocks concurrency on the hot copy path. Previously the session lived in one `Mutex<Option<(SmbClient, Tree)>>`, which the streaming-read producer and the compound read/write fast-paths held for the entire transfer, serializing every concurrent copy through the mutex. With smb2 0.7.x, `Connection` is `Clone` (cheap `Arc::clone`, all clones multiplex frames over one SMB session). Splitting the Tree out lets us briefly lock the client, clone its `Connection`, and release the lock, then drive `Tree::download` / `Tree::read_file_compound` / `Tree::write_file_compound` on the cloned `Connection` with no lock held. N concurrent copies on one `SmbVolume` now pipeline N operations over the single session instead of queuing on the mutex. Tree lives in a `RwLock` because we only take read locks in the hot path (cloning an `Arc<Tree>`) and only write on disconnect. Since smb2 0.9, the streaming-write path also uses this clone-and-release shape (see the `write_from_stream` Decision below), so the client mutex is no longer held across any I/O.
 
 **Decision**: `SmbVolume::local_path()` returns `None`
 **Why**: `local_path()` is checked in `volume_copy.rs` to decide whether to use native OS copy APIs. If SmbVolume returned `Some(mount_path)`, copies would go through the slow OS mount, which is exactly what we're trying to avoid. `root()` still returns the mount path for frontend path resolution.
@@ -410,8 +410,11 @@ spawned detached task. This is safe because the stream always lives in an async
 **Gotcha**: Watcher filenames are NFC (from server) but macOS mount paths are NFD
 **Why**: SMB servers return NFC-normalized filenames. macOS filesystem paths use NFD. The watcher NFD-normalizes filenames before constructing display paths used for cache lookups.
 
-**Gotcha**: SMB write streaming fallback still holds the client mutex for the whole upload
-**Why**: `SmbClient::create_file_writer` / `Tree::create_file_writer` both return `FileWriter<'a>` which borrows `&'a mut Connection`. We can't release the client mutex while the writer is alive, so the streaming (large-file) write path serializes concurrent writes on one `SmbVolume`. This is acceptable because the compound fast-path in `write_from_stream` handles every file ≤ `max_write_size` (typically 1 MB on QNAP, the SMB2 spec ceiling is 8 MB) without touching the client mutex for the actual write. Large files are rare in the hot copy path; if this ever becomes a bottleneck, the fix is a future `smb2` release that exposes a `FileWriter` built from a cloned `Connection` + `Arc<Tree>` (both owned inside the writer) rather than borrowing.
+**Gotcha (no longer applicable as of smb2 0.9)**: SMB write streaming fallback used to hold the client mutex for the whole upload
+**Why**: Historically `FileWriter<'a>` borrowed `&'a mut Connection` from the `SmbClient`, so `write_from_stream` had to hold the client mutex for the duration of the streaming write. Under sustained concurrent pressure this two-phase pattern (brief `clone_session` fast-path probe → drop → long mutex-guarded streaming fallback) deadlocked. smb2 0.9 rebuilt `FileWriter` to own its `Connection` and `Arc<Tree>`, removing the borrow. The regression is pinned by `smb_integration_concurrent_streaming_writes_no_deadlock`. See the new Decision below (`write_from_stream` uses a cloned Connection + Arc<Tree>) for the current design.
+
+**Decision**: `write_from_stream` uses a cloned `Connection` + `Arc<Tree>` via smb2 0.9's owned `FileWriter`
+**Why**: smb2 0.9 made `FileWriter` own its `Connection` (cheap `Arc::clone`) and `Arc<Tree>` instead of borrowing `&'a mut Connection`. `write_from_stream` now calls `clone_session` once up front and drives both the compound fast-path AND the streaming fallback on the same owned `Connection` clone. The client mutex is held only for the few microseconds of `clone_session()`, never across I/O. The previous shape — fast-path on a clone, then drop and re-acquire the client for the streaming fallback — was the deadlock reproducer in Phase C against QNAP. The architectural property we get for free: N concurrent streaming writes on one `SmbVolume` pipeline N WRITE chains over a single SMB session, multiplexed by `MessageId` in smb2's receiver task. No external locking, no mutex contention on the hot copy path.
 
 **Decision**: `SmbVolume` overrides `scan_for_copy_batch` to pipeline per-path stats over a single SMB session
 **Why**: The copy pipeline's scan phase used to loop `scan_for_copy` per top-level source, N sequential RTTs on the wire before the copy phase could even start. For a 100-file copy over a ~60 ms Tailscale link that's ~5 s of serial stats. Fix 4 overrides `scan_for_copy_batch` to clone `smb2::Connection` per path under a brief client-mutex acquire (cheap `Arc::clone`, all clones multiplex over the same SMB session), release the lock, then drive `tree.stat(&mut conn, path)` on each clone inside a `FuturesUnordered`. Empty root paths skip the stat. Single-path batches fall through to `scan_recursive` so one-file drag-drops don't pay the batch machinery cost. Directories found during the stat phase recurse sequentially afterward. Parallel directory recursion is a future "Fix 5". Measured 6.5× wall-clock win at 100 × 10 KB: 6.11 s → 947 ms. See `docs/notes/phase4-rtt-investigation.md` for the wire trace.

apps/desktop/src-tauri/src/file_system/volume/friendly_error/kinds.rs

@@ -206,6 +206,29 @@ pub(super) fn not_supported(raw_detail: String) -> FriendlyError {
     }
 }
 
+/// `STATUS_DELETE_PENDING`: the file has been marked for deletion on the server
+/// but at least one open handle is keeping it alive. The file disappears the
+/// moment the last handle closes, so retry-after-a-moment is the right hint.
+pub(super) fn delete_pending(path_display: &str, raw_detail: String) -> FriendlyError {
+    FriendlyError {
+        category: ErrorCategory::Transient,
+        title: "File is being removed".into(),
+        explanation: format!(
+            "`{}` is on its way out. The server marked it for deletion, but another \
+            open handle is keeping it around until that handle closes.",
+            path_display
+        ),
+        suggestion: "Here's what to try:\n\
+            - Wait a moment and try again — once the last handle closes, the file disappears\n\
+            - Close any other apps that might have this file open\n\
+            - If it sticks around, restart Cmdr to drop any handles it might still hold"
+            .into(),
+        raw_detail,
+        retry_hint: true,
+        action_kind: None,
+    }
+}
+
 pub(super) fn io_serious(path_display: &str, message: &str, raw_detail: String) -> FriendlyError {
     FriendlyError {
         category: ErrorCategory::Serious,

apps/desktop/src-tauri/src/file_system/volume/friendly_error/mod.rs

@@ -211,6 +211,7 @@ mod tests {
                 true,
             ),
             (VolumeError::Cancelled("x".into()), ErrorCategory::Transient, true),
+            (VolumeError::DeletePending("x".into()), ErrorCategory::Transient, true),
             (
                 VolumeError::IoError {
                     message: "x".into(),
@@ -296,6 +297,7 @@ mod tests {
             VolumeError::NotFound("x".into()),
             VolumeError::PermissionDenied("x".into()),
             VolumeError::ConnectionTimeout("x".into()),
+            VolumeError::DeletePending("x".into()),
             VolumeError::IoError {
                 message: "x".into(),
                 raw_os_error: None,
@@ -357,6 +359,36 @@ mod tests {
         }
     }
 
+    #[test]
+    fn delete_pending_uses_dedicated_copy() {
+        let path = Path::new("/Volumes/share/photo.jpg");
+        let err = VolumeError::DeletePending("Protocol error: STATUS_DELETE_PENDING during Create".into());
+        let friendly = friendly_error_from_volume_error(&err, path);
+
+        assert_eq!(friendly.category, ErrorCategory::Transient);
+        assert!(
+            friendly.retry_hint,
+            "DeletePending is transient — user should see a retry hint"
+        );
+        assert!(
+            friendly.title.contains("being removed"),
+            "DeletePending title should say the file is being removed, got: {:?}",
+            friendly.title,
+        );
+        // The path is interpolated into the explanation so the user knows which file.
+        assert!(
+            friendly.explanation.contains("photo.jpg"),
+            "DeletePending explanation should include the path, got: {:?}",
+            friendly.explanation,
+        );
+        // raw_detail preserves the underlying NTSTATUS for the technical-details disclosure.
+        assert!(
+            friendly.raw_detail.contains("DELETE_PENDING"),
+            "raw_detail should preserve the NTSTATUS code, got: {:?}",
+            friendly.raw_detail,
+        );
+    }
+
     // ── action_kind tests ───────────────────────────────────────────────
 
     #[cfg(target_os = "macos")]

apps/desktop/src-tauri/src/file_system/volume/friendly_error/volume_error.rs

@@ -49,6 +49,7 @@ pub fn friendly_error_from_volume_error(err: &VolumeError, path: &Path) -> Frien
         VolumeError::StorageFull { .. } => kinds::storage_full(raw),
         VolumeError::ConnectionTimeout(_) => kinds::connection_timeout(raw),
         VolumeError::Cancelled(_) => kinds::cancelled(raw),
+        VolumeError::DeletePending(_) => kinds::delete_pending(&path_display, raw),
         VolumeError::IsADirectory(_) => FriendlyError {
             category: ErrorCategory::NeedsAction,
             title: "This is a folder, not a file".into(),

apps/desktop/src-tauri/src/file_system/volume/friendly_error/write_error.rs

@@ -142,6 +142,7 @@ pub fn friendly_from_write_error(err: &crate::file_system::write_operations::Wri
             retry_hint: false,
             action_kind: None,
         },
+        W::DeletePending { path } => kinds::delete_pending(path, raw),
         W::IoError { path, message } => kinds::io_serious(path, message, raw),
     }
 }

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

@@ -166,6 +166,11 @@ pub enum VolumeError {
     Cancelled(String),
     /// The path is a directory, not a file (for example, SMB STATUS_FILE_IS_A_DIRECTORY).
     IsADirectory(String),
+    /// The file is in `STATUS_DELETE_PENDING`: a delete has been requested on the server
+    /// but at least one open handle is keeping the file alive. The file will disappear
+    /// once the last handle closes; any new `Create` (stat, open, write) on the path
+    /// fails with this status in the meantime. SMB-only today.
+    DeletePending(String),
     IoError {
         message: String,
         raw_os_error: Option<i32>,
@@ -194,6 +199,7 @@ impl std::fmt::Display for VolumeError {
             Self::ConnectionTimeout(msg) => write!(f, "Connection timed out: {}", msg),
             Self::Cancelled(msg) => write!(f, "Cancelled: {}", msg),
             Self::IsADirectory(path) => write!(f, "Is a directory: {}", path),
+            Self::DeletePending(path) => write!(f, "Delete pending: {}", path),
             Self::IoError { message, .. } => write!(f, "I/O error: {}", message),
             Self::FriendlyGit(err) => write!(f, "git: {}", err),
         }