Skip to content

Commit bed59db

Browse files
vdavidvdavid
committed
Bugfix: Never block UI on network FS ops
- Move validation (`validate_sources`, `validate_destination`, etc.) from `*_files_start` into `spawn_blocking` handler closures so `operationId` returns immediately — dialog is always cancellable, even on stalled mounts - Fix `start_write_operation` to emit `write-error` for handler errors (previously only caught panics, silently dropped validation failures) - Add `remove_file_in_background` and `remove_dir_all_in_background` helpers for fire-and-forget cleanup on detached threads - Add `CopyTransaction::rollback_in_background` for non-blocking rollback on cancel (sync `rollback()` stays for error paths) - Use async cleanup in `chunked_copy` cancel path, `copy` cancel path, and `move_op` staging dir failure paths - Background cleanup is best-effort — partial files may remain if mount disconnects (uses `.cmdr-` prefix for recognition) Entire-Checkpoint: 12a05651ab05
1 parent ced9d25 commit bed59db

8 files changed

Lines changed: 392 additions & 63 deletions

File tree

‎apps/desktop/src-tauri/src/file_system/write_operations/CLAUDE.md‎

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,10 @@ network mounts, cross-filesystem moves, and name/path length limits.
1212

1313
| File | Responsibility |
1414
|------|----------------|
15-
| `mod.rs` | Public API: `copy_files_start`, `move_files_start`, `delete_files_start`, `trash_files_start`. Each validates inputs, then delegates to `start_write_operation` which handles state creation, spawn lifecycle, cleanup, and panic recovery. |
15+
| `mod.rs` | Public API: `copy_files_start`, `move_files_start`, `delete_files_start`, `trash_files_start`. Each delegates to `start_write_operation` which handles state creation, spawn lifecycle, cleanup, and error/panic recovery. Validation runs inside the handler closure on the blocking thread pool — never on the async executor. |
1616
| `types.rs` | All serializable types: events, config, errors, results. `WriteOperationConfig`, `ConflictResolution`, `WriteOperationError`, `DryRunResult`, scan preview events. |
1717
| `state.rs` | Two `LazyLock<RwLock<HashMap>>` caches (`WRITE_OPERATION_STATE`, `OPERATION_STATUS_CACHE`). `WriteOperationState`, `CopyTransaction`, `ScanResult`, `FileInfo`. |
18-
| `helpers.rs` | Validation (`validate_sources`, `validate_destination_writable` via `libc::access`, `validate_disk_space` via `statvfs`). Conflict resolution (condvar wait for Stop mode). `safe_overwrite_file`/`safe_overwrite_dir` (temp+rename). `find_unique_name`. `run_cancellable`. `is_same_filesystem` (device IDs). |
18+
| `helpers.rs` | Validation (`validate_sources`, `validate_destination_writable` via `libc::access`, `validate_disk_space` via `statvfs`). Conflict resolution (condvar wait for Stop mode). `safe_overwrite_file`/`safe_overwrite_dir` (temp+rename). `find_unique_name`. `run_cancellable`. `is_same_filesystem` (device IDs). Background cleanup helpers: `remove_file_in_background`, `remove_dir_all_in_background`. |
1919
| `scan.rs` | `scan_sources` (recursive walk, emits progress), `dry_run_scan`, scan preview subsystem (`start_scan_preview`, `cancel_scan_preview`). |
2020
| `copy.rs` | `copy_files_with_progress`: scan → disk space check → per-file copy via `copy_single_item`. `CopyTransaction` for rollback. |
2121
| `move_op.rs` | Same-fs: `fs::rename`. Cross-fs: copy to `.cmdr-staging-<uuid>`, atomic rename, delete sources. |
@@ -32,18 +32,20 @@ network mounts, cross-filesystem moves, and name/path length limits.
3232

3333
```
3434
Frontend
35-
→ validate (sources exist, dest writable, not same location, dest not inside source)
3635
→ WriteOperationState created (AtomicBool cancelled, Condvar for Stop conflicts)
3736
→ stored in WRITE_OPERATION_STATE + OPERATION_STATUS_CACHE
37+
→ operationId returned to frontend immediately (dialog opens, cancel is possible)
3838
→ tokio::spawn (async wrapper)
3939
→ tokio::task::spawn_blocking (all blocking I/O here)
40+
→ validate (sources exist, dest writable, not same location, dest not inside source)
4041
→ scan phase: walk_dir_recursive, emit scan-progress events
4142
→ disk space check (statvfs)
4243
→ execute phase: per-file copy/delete
4344
→ throttled write-progress events (200ms default)
4445
→ success: CopyTransaction::commit(), emit write-complete
45-
→ cancel: CopyTransaction::rollback(), emit write-cancelled
46+
→ cancel: CopyTransaction::rollback_in_background(), emit write-cancelled
4647
→ error: CopyTransaction::rollback(), emit write-error
48+
→ safety net: start_write_operation emits write-error for unhandled handler errors
4749
→ state removed from both caches
4850
```
4951

@@ -54,10 +56,11 @@ Frontend
5456
**Two-layer cancellation.** `AtomicBool` for fast in-loop checks. `run_cancellable` wraps blocking operations (e.g.,
5557
network-mount copies that may block indefinitely) in a separate thread, polling the flag every 100ms via `mpsc::channel`.
5658

57-
**`CopyTransaction` rollback with auto-rollback on panic.** Records created files and dirs in creation order. Rollback
58-
deletes files in reverse order first, then dirs in reverse order (deepest first). `commit()` sets a `committed` flag and
59-
drops the vecs. `Drop` impl checks `committed` — if false (panic unwind or forgotten commit), it auto-rolls back.
60-
Delete operations are not rollbackable.
59+
**`CopyTransaction` rollback: sync vs async.** Two rollback paths: `rollback()` (synchronous, for error paths where
60+
cleanup must complete before the error event) and `rollback_in_background()` (fire-and-forget on a detached thread, for
61+
user-initiated cancel where the UI must respond instantly). `rollback_in_background` sets `committed = true` to prevent
62+
`Drop` from triggering a synchronous double-rollback, then moves the file/dir lists into the detached thread. The
63+
synchronous `rollback()` and auto-rollback-on-panic via `Drop` remain unchanged. Delete operations are not rollbackable.
6164

6265
**Symlinks never dereferenced.** All stat calls use `symlink_metadata`. Symlink loop detection uses a `HashSet<PathBuf>`
6366
of canonicalized paths.
@@ -99,6 +102,20 @@ operations when the frontend is destroyed.
99102

100103
**Special files skipped.** Sockets, FIFOs, and device files are filtered out during scan.
101104

105+
**Validation runs inside `spawn_blocking`.** The `*_files_start` functions return an `operationId` immediately, before
106+
any filesystem I/O. Validation (`validate_sources`, `validate_destination_writable`, etc.) runs inside the handler
107+
closure on the blocking thread pool. This keeps the Tauri IPC handler non-blocking, so the frontend can always open
108+
the progress dialog and offer cancel, even if a network mount is stalled.
109+
110+
**`start_write_operation` emits `write-error` for handler errors.** The spawn wrapper matches on the handler's
111+
`Result`: `Ok(Ok(()))` and `Ok(Err(Cancelled))` are no-ops (handlers already emitted the right events), `Ok(Err(e))`
112+
emits `write-error` as a safety net, and `Err(join_error)` handles panics. Double-emit is harmless because the
113+
frontend's `handleError` removes all listeners on first receipt.
114+
115+
**Background cleanup is best-effort.** `rollback_in_background`, `remove_file_in_background`, and
116+
`remove_dir_all_in_background` run on detached threads. If the network mount disconnects or the app exits, partial
117+
files or staging directories may remain on disk. These use the `.cmdr-` prefix, so they're recognizable.
118+
102119
**`volume_copy` path is incomplete.** The three `volume_*` files are Phase 5 work, but are publicly re-exported from `mod.rs` and at least partially wired up.
103120

104121
## Events emitted
@@ -109,7 +126,7 @@ operations when the frontend is destroyed.
109126
| `write-conflict` | Stop mode hit a conflicting destination file |
110127
| `write-complete` | Operation finished successfully |
111128
| `write-cancelled` | Operation cancelled (includes `rolled_back` flag) |
112-
| `write-error` | Operation failed |
129+
| `write-error` | Operation failed (emitted by handler and/or `start_write_operation` safety net) |
113130
| `write-source-item-done` | All files for a top-level source item processed (for gradual deselection) |
114131
| `dry-run-complete` | `config.dry_run == true` (returns `DryRunResult`) |
115132
| `scan-preview-progress` | During `start_scan_preview` |

‎apps/desktop/src-tauri/src/file_system/write_operations/chunked_copy.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -122,9 +122,9 @@ fn copy_data_chunked(
122122
"chunked_copy: cancellation detected after {} bytes, cleaning up",
123123
total_bytes
124124
);
125-
// Clean up partial file
125+
// Clean up partial file in background (may block on network mounts)
126126
drop(dst_file);
127-
let _ = std::fs::remove_file(dest);
127+
super::helpers::remove_file_in_background(dest.to_path_buf());
128128
return Err(WriteOperationError::Cancelled {
129129
message: "Operation cancelled by user".to_string(),
130130
});
@@ -338,8 +338,8 @@ mod tests {
338338
let result = chunked_copy_with_metadata(&src, &dst, &cancelled, None);
339339

340340
assert!(matches!(result, Err(WriteOperationError::Cancelled { .. })));
341-
// Partial file should be cleaned up
342-
assert!(!dst.exists());
341+
// Partial file cleanup is now async/best-effort (fires on a detached thread),
342+
// so we don't assert file absence here — it may or may not be deleted yet.
343343

344344
cleanup_temp_dir(&temp_dir);
345345
}

‎apps/desktop/src-tauri/src/file_system/write_operations/copy.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,7 @@ pub(super) fn copy_files_with_progress(
268268
operation_id,
269269
transaction.created_files.len()
270270
);
271-
transaction.rollback();
271+
transaction.rollback_in_background();
272272
}
273273

274274
let _ = app.emit(

‎apps/desktop/src-tauri/src/file_system/write_operations/helpers.rs‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,24 @@ pub(crate) fn is_same_filesystem(_source: &Path, _destination: &Path) -> std::io
260260
// Async sync for durability
261261
// ============================================================================
262262

263+
/// Deletes a file on a detached thread. Returns immediately. Best-effort.
264+
pub(super) fn remove_file_in_background(path: PathBuf) {
265+
std::thread::spawn(move || {
266+
if let Err(e) = fs::remove_file(&path) {
267+
log::warn!("background cleanup: failed to remove {}: {}", path.display(), e);
268+
}
269+
});
270+
}
271+
272+
/// Deletes a directory tree on a detached thread. Returns immediately. Best-effort.
273+
pub(super) fn remove_dir_all_in_background(path: PathBuf) {
274+
std::thread::spawn(move || {
275+
if let Err(e) = fs::remove_dir_all(&path) {
276+
log::warn!("background cleanup: failed to remove {}: {}", path.display(), e);
277+
}
278+
});
279+
}
280+
263281
/// Spawns a background thread to call sync() for durability.
264282
/// This ensures writes are flushed to disk without blocking the completion event.
265283
pub(super) fn spawn_async_sync() {

‎apps/desktop/src-tauri/src/file_system/write_operations/mod.rs‎

Lines changed: 59 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -129,19 +129,37 @@ where
129129
}
130130
unregister_operation_status(&operation_id_for_cleanup);
131131

132-
if let Err(e) = result {
133-
use tauri::Emitter;
134-
let _ = app_for_error.emit(
135-
"write-error",
136-
WriteErrorEvent {
137-
operation_id: operation_id_for_cleanup,
138-
operation_type,
139-
error: WriteOperationError::IoError {
140-
path: String::new(),
141-
message: format!("Task failed: {}", e),
132+
use tauri::Emitter;
133+
match result {
134+
Ok(Ok(())) => {} // Handler already emitted write-complete or write-cancelled
135+
Ok(Err(ref e)) if matches!(e, WriteOperationError::Cancelled { .. }) => {
136+
// Handler already emitted write-cancelled
137+
}
138+
Ok(Err(e)) => {
139+
// Handler error (validation, I/O, etc.) — emit write-error as safety net
140+
let _ = app_for_error.emit(
141+
"write-error",
142+
WriteErrorEvent {
143+
operation_id: operation_id_for_cleanup,
144+
operation_type,
145+
error: e,
146+
},
147+
);
148+
}
149+
Err(join_error) => {
150+
// Panic/abort in spawn_blocking
151+
let _ = app_for_error.emit(
152+
"write-error",
153+
WriteErrorEvent {
154+
operation_id: operation_id_for_cleanup,
155+
operation_type,
156+
error: WriteOperationError::IoError {
157+
path: String::new(),
158+
message: format!("Task failed: {}", join_error),
159+
},
142160
},
143-
},
144-
);
161+
);
162+
}
145163
}
146164
});
147165

@@ -158,12 +176,6 @@ pub async fn copy_files_start(
158176
destination: PathBuf,
159177
config: WriteOperationConfig,
160178
) -> Result<WriteOperationStartResult, WriteOperationError> {
161-
validate_sources(&sources)?;
162-
validate_destination(&destination)?;
163-
validate_destination_writable(&destination)?;
164-
validate_not_same_location(&sources, &destination)?;
165-
validate_destination_not_inside_source(&sources, &destination)?;
166-
167179
log::info!(
168180
"copy_files_start: sources={:?}, destination={:?}, dry_run={}",
169181
sources,
@@ -175,7 +187,14 @@ pub async fn copy_files_start(
175187
app,
176188
WriteOperationType::Copy,
177189
config.progress_interval_ms,
178-
move |app, op_id, state| copy_files_with_progress(&app, &op_id, &state, &sources, &destination, &config),
190+
move |app, op_id, state| {
191+
validate_sources(&sources)?;
192+
validate_destination(&destination)?;
193+
validate_destination_writable(&destination)?;
194+
validate_not_same_location(&sources, &destination)?;
195+
validate_destination_not_inside_source(&sources, &destination)?;
196+
copy_files_with_progress(&app, &op_id, &state, &sources, &destination, &config)
197+
},
179198
)
180199
.await
181200
}
@@ -190,12 +209,6 @@ pub async fn move_files_start(
190209
destination: PathBuf,
191210
config: WriteOperationConfig,
192211
) -> Result<WriteOperationStartResult, WriteOperationError> {
193-
validate_sources(&sources)?;
194-
validate_destination(&destination)?;
195-
validate_destination_writable(&destination)?;
196-
validate_not_same_location(&sources, &destination)?;
197-
validate_destination_not_inside_source(&sources, &destination)?;
198-
199212
log::info!(
200213
"move_files_start: sources={:?}, destination={:?}, dry_run={}",
201214
sources,
@@ -207,7 +220,14 @@ pub async fn move_files_start(
207220
app,
208221
WriteOperationType::Move,
209222
config.progress_interval_ms,
210-
move |app, op_id, state| move_files_with_progress(&app, &op_id, &state, &sources, &destination, &config),
223+
move |app, op_id, state| {
224+
validate_sources(&sources)?;
225+
validate_destination(&destination)?;
226+
validate_destination_writable(&destination)?;
227+
validate_not_same_location(&sources, &destination)?;
228+
validate_destination_not_inside_source(&sources, &destination)?;
229+
move_files_with_progress(&app, &op_id, &state, &sources, &destination, &config)
230+
},
211231
)
212232
.await
213233
}
@@ -224,19 +244,6 @@ pub async fn delete_files_start(
224244
volume_id: Option<String>,
225245
) -> Result<WriteOperationStartResult, WriteOperationError> {
226246
let volume_id_str = volume_id.unwrap_or_else(|| "root".to_string());
227-
let volume = if volume_id_str != "root" {
228-
Some(
229-
crate::file_system::get_volume_manager()
230-
.get(&volume_id_str)
231-
.ok_or_else(|| WriteOperationError::IoError {
232-
path: volume_id_str.clone(),
233-
message: format!("Volume '{}' not found", volume_id_str),
234-
})?,
235-
)
236-
} else {
237-
validate_sources(&sources)?;
238-
None
239-
};
240247

241248
log::info!(
242249
"delete_files_start: sources={:?}, volume={}, dry_run={}",
@@ -250,9 +257,16 @@ pub async fn delete_files_start(
250257
WriteOperationType::Delete,
251258
config.progress_interval_ms,
252259
move |app, op_id, state| {
253-
if let Some(vol) = volume {
254-
delete_volume_files_with_progress(vol, &app, &op_id, &state, &sources, &config)
260+
if volume_id_str != "root" {
261+
let volume = crate::file_system::get_volume_manager()
262+
.get(&volume_id_str)
263+
.ok_or_else(|| WriteOperationError::IoError {
264+
path: volume_id_str.clone(),
265+
message: format!("Volume '{}' not found", volume_id_str),
266+
})?;
267+
delete_volume_files_with_progress(volume, &app, &op_id, &state, &sources, &config)
255268
} else {
269+
validate_sources(&sources)?;
256270
delete_files_with_progress(&app, &op_id, &state, &sources, &config)
257271
}
258272
},
@@ -271,15 +285,16 @@ pub async fn trash_files_start(
271285
item_sizes: Option<Vec<u64>>,
272286
config: WriteOperationConfig,
273287
) -> Result<WriteOperationStartResult, WriteOperationError> {
274-
validate_sources(&sources)?;
275-
276288
log::info!("trash_files_start: sources={:?}", sources);
277289

278290
start_write_operation(
279291
app,
280292
WriteOperationType::Trash,
281293
config.progress_interval_ms,
282-
move |app, op_id, state| trash_files_with_progress(&app, &op_id, &state, &sources, item_sizes.as_deref()),
294+
move |app, op_id, state| {
295+
validate_sources(&sources)?;
296+
trash_files_with_progress(&app, &op_id, &state, &sources, item_sizes.as_deref())
297+
},
283298
)
284299
.await
285300
}

‎apps/desktop/src-tauri/src/file_system/write_operations/move_op.rs‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,9 @@ use std::sync::atomic::Ordering;
88
use std::time::Instant;
99

1010
use super::copy::copy_single_item;
11-
use super::helpers::{is_same_filesystem, resolve_conflict, safe_overwrite_dir, spawn_async_sync};
11+
use super::helpers::{
12+
is_same_filesystem, remove_dir_all_in_background, resolve_conflict, safe_overwrite_dir, spawn_async_sync,
13+
};
1214
use super::scan::{SourceItemTracker, handle_dry_run, scan_sources};
1315
use super::state::{CopyTransaction, WriteOperationState, update_operation_status};
1416
use super::types::{
@@ -275,8 +277,8 @@ fn move_with_staging(
275277
})();
276278

277279
if let Err(e) = copy_result {
278-
// Cleanup staging directory on failure
279-
let _ = fs::remove_dir_all(&staging_dir);
280+
// Cleanup staging directory in background (may block on network mounts)
281+
remove_dir_all_in_background(staging_dir.clone());
280282
let _ = app.emit(
281283
"write-error",
282284
WriteErrorEvent {
@@ -350,8 +352,8 @@ fn move_with_staging(
350352
})();
351353

352354
if let Err(e) = rename_result {
353-
// Cleanup staging directory on failure
354-
let _ = fs::remove_dir_all(&staging_dir);
355+
// Cleanup staging directory in background (may block on network mounts)
356+
remove_dir_all_in_background(staging_dir);
355357
let _ = app.emit(
356358
"write-error",
357359
WriteErrorEvent {

‎apps/desktop/src-tauri/src/file_system/write_operations/state.rs‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -389,6 +389,33 @@ impl CopyTransaction {
389389
}
390390
}
391391

392+
/// Rolls back on a detached thread. Returns immediately.
393+
/// Use for user-initiated cancel where the calling thread must not block.
394+
/// Best-effort: if the background thread fails, files remain on disk.
395+
pub fn rollback_in_background(mut self) {
396+
let files = std::mem::take(&mut self.created_files);
397+
let dirs = std::mem::take(&mut self.created_dirs);
398+
self.committed = true; // Prevent Drop from synchronous double-rollback
399+
if files.is_empty() && dirs.is_empty() {
400+
return;
401+
}
402+
log::info!(
403+
"rollback_in_background: cleaning up {} files and {} dirs",
404+
files.len(),
405+
dirs.len()
406+
);
407+
std::thread::spawn(move || {
408+
for file in files.iter().rev() {
409+
if let Err(e) = std::fs::remove_file(file) {
410+
log::warn!("rollback: failed to remove {}: {}", file.display(), e);
411+
}
412+
}
413+
for dir in dirs.iter().rev() {
414+
let _ = std::fs::remove_dir(dir);
415+
}
416+
});
417+
}
418+
392419
/// Marks the transaction as committed, preventing rollback on drop.
393420
pub fn commit(mut self) {
394421
self.committed = true;

0 commit comments

Comments
 (0)