Skip to content

Commit b401889

Browse files
committed
Delete: emit write-cancelled when scan phase aborts
`scan_volume_recursive`'s top-of-function cancel check returned `Err(Cancelled)` without emitting the terminal event, so a mid-scan cancel of a volume delete propagated up silently and the FE never saw `write-cancelled`. With M4's settle event in place the dialog still closed via the safety-net path, but the selection-recovery / log trail / dialog state-machine all want the proper event. The cancel check fires at every recursion level, so emitting from inside `scan_volume_recursive` would multi-fire. Instead: add a small `emit_cancelled_if_aborted` helper next to the entry point, call it at each of the three `scan_volume_recursive(...).await?` sites in `delete_volume_files_with_progress_inner`. The pattern mirrors the five existing emit-then-return cancel sites elsewhere in the file. Regression test in `delete_volume_reuse_tests.rs` pre-sets `state.intent` to `Stopped`, runs a delete, asserts the result is `Cancelled` AND the event sink received a `write-cancelled` with `rolled_back: false`.
1 parent f894e60 commit b401889

2 files changed

Lines changed: 97 additions & 8 deletions

File tree

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

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,12 @@ impl VolumeScanTracker {
223223
/// photos can take ~17 s of USB roundtrips.
224224
/// 2. **After each subtree** (this function's tail), as a final snapshot for that branch. Throttled
225225
/// by the shared `VolumeScanTracker` so it doesn't race with the per-entry callback.
226+
///
227+
/// **Cancel contract**: this function does NOT emit `write-cancelled` before returning
228+
/// `Err(Cancelled)`. The cancel check fires at every recursion level, so emitting here would
229+
/// multi-fire. Top-level callers must call `emit_cancelled_if_aborted` (or otherwise emit
230+
/// `write-cancelled`) on the returned result before propagating it, so the FE sees the proper
231+
/// terminal event for the cancel flow.
226232
#[allow(
227233
clippy::too_many_arguments,
228234
reason = "Matches the parameter pattern of other write operation functions"
@@ -409,6 +415,26 @@ async fn scan_volume_recursive(
409415
Ok(())
410416
}
411417

418+
/// Emits `write-cancelled` when `result` is `Err(Cancelled)`. Use this after
419+
/// every top-level `scan_volume_recursive` call so the FE sees the proper
420+
/// terminal event before the error propagates up. See `scan_volume_recursive`'s
421+
/// "Cancel contract" doc for why this is the caller's responsibility.
422+
fn emit_cancelled_if_aborted(
423+
result: &Result<(), WriteOperationError>,
424+
events: &dyn OperationEventSink,
425+
operation_id: &str,
426+
tracker: &VolumeScanTracker,
427+
) {
428+
if matches!(result, Err(WriteOperationError::Cancelled { .. })) {
429+
events.emit_cancelled(WriteCancelledEvent {
430+
operation_id: operation_id.to_string(),
431+
operation_type: WriteOperationType::Delete,
432+
files_processed: tracker.files_so_far.load(std::sync::atomic::Ordering::Relaxed),
433+
rolled_back: false,
434+
});
435+
}
436+
}
437+
412438
/// Deletes files on a non-local volume (like MTP) with progress reporting.
413439
///
414440
/// Uses `volume.list_directory()` for scanning and `volume.delete()` per item.
@@ -506,7 +532,7 @@ pub(super) async fn delete_volume_files_with_progress_inner(
506532
// `is_dir_hint = Some(true)` so the recursion never re-probes
507533
// the top-level. Any subtree that's open in another pane and
508534
// watcher-fresh gets cache-fed inside the walker.
509-
scan_volume_recursive(
535+
let result = scan_volume_recursive(
510536
&*volume,
511537
volume_id,
512538
source,
@@ -518,13 +544,15 @@ pub(super) async fn delete_volume_files_with_progress_inner(
518544
operation_id,
519545
&tracker,
520546
)
521-
.await?;
547+
.await;
548+
emit_cancelled_if_aborted(&result, events, operation_id, &tracker);
549+
result?;
522550
}
523551
None => {
524552
// Scan preview was non-volume (local-FS) or didn't include
525553
// this source. Fall back to the no-preview shape for this
526554
// path: oracle-aware walker resolves the type.
527-
scan_volume_recursive(
555+
let result = scan_volume_recursive(
528556
&*volume,
529557
volume_id,
530558
source,
@@ -536,7 +564,9 @@ pub(super) async fn delete_volume_files_with_progress_inner(
536564
operation_id,
537565
&tracker,
538566
)
539-
.await?;
567+
.await;
568+
emit_cancelled_if_aborted(&result, events, operation_id, &tracker);
569+
result?;
540570
}
541571
}
542572

@@ -600,7 +630,7 @@ pub(super) async fn delete_volume_files_with_progress_inner(
600630
};
601631

602632
if is_dir {
603-
scan_volume_recursive(
633+
let result = scan_volume_recursive(
604634
&*volume,
605635
volume_id,
606636
source,
@@ -612,7 +642,9 @@ pub(super) async fn delete_volume_files_with_progress_inner(
612642
operation_id,
613643
&tracker,
614644
)
615-
.await?;
645+
.await;
646+
emit_cancelled_if_aborted(&result, events, operation_id, &tracker);
647+
result?;
616648
} else {
617649
// Top-level file: size unknown without listing the parent, use 0.
618650
// Progress still tracks file count accurately, and individual file

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

Lines changed: 59 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@ use std::sync::atomic::{AtomicBool, AtomicU64, AtomicUsize, Ordering};
1616
use std::time::Duration;
1717

1818
use super::delete::delete_volume_files_with_progress_inner;
19-
use super::state::{CachedScanResult, SCAN_PREVIEW_RESULTS, WriteOperationState};
20-
use super::types::{CollectorEventSink, WriteOperationConfig};
19+
use super::state::{CachedScanResult, OperationIntent, SCAN_PREVIEW_RESULTS, WriteOperationState};
20+
use super::types::{CollectorEventSink, WriteOperationConfig, WriteOperationError};
2121
use crate::file_system::get_volume_manager;
2222
use crate::file_system::listing::caching::{CachedListing, LISTING_CACHE};
2323
use crate::file_system::listing::metadata::FileEntry;
@@ -436,3 +436,60 @@ async fn delete_mid_scan_listing_close() {
436436
remove_listing(&parent_lid_inserted);
437437
get_volume_manager().unregister(&vid);
438438
}
439+
440+
/// Regression test: cancel landing during the scan phase of a volume delete
441+
/// must emit `write-cancelled` before propagating `Err(Cancelled)` up. Prior
442+
/// to the fix, `scan_volume_recursive`'s top-of-function cancel check returned
443+
/// `Cancelled` without emitting, and the `?` propagation at the call site
444+
/// passed it through silently — the FE never saw the terminal cancel event.
445+
///
446+
/// The pattern: pre-set `state.intent` to `Stopped` so the recursion's cancel
447+
/// check fires on first entry. Direct `intent.store(...)` is acceptable here
448+
/// because we're testing the emit behaviour on Cancelled return, not the
449+
/// state-machine transition itself.
450+
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
451+
async fn delete_cancel_during_scan_emits_write_cancelled() {
452+
let vid = unique("cancel_scan_emits");
453+
454+
let vol = Arc::new(CountingVolume::new("cancel-scan-vol", false));
455+
vol.inner.create_file(Path::new("/dir/a.jpg"), b"alpha").await.unwrap();
456+
vol.inner.create_file(Path::new("/dir/b.jpg"), b"betabb").await.unwrap();
457+
get_volume_manager().register(&vid, vol.clone() as Arc<dyn Volume>);
458+
459+
let events = Arc::new(CollectorEventSink::new());
460+
let state = make_state();
461+
// Pre-cancel: the very first cancel check inside `scan_volume_recursive`
462+
// will fire and return `Cancelled`. Without the fix, no `write-cancelled`
463+
// event would be emitted before the `?` propagates the error.
464+
state.intent.store(OperationIntent::Stopped as u8, Ordering::Relaxed);
465+
466+
let config = WriteOperationConfig::default(); // preview_id: None
467+
468+
let result = delete_volume_files_with_progress_inner(
469+
vol.clone() as Arc<dyn Volume>,
470+
&vid,
471+
events.as_ref(),
472+
"test-op-cancel-scan-emits",
473+
&state,
474+
&[PathBuf::from("/dir")],
475+
&config,
476+
)
477+
.await;
478+
479+
assert!(
480+
matches!(result, Err(WriteOperationError::Cancelled { .. })),
481+
"scan-time cancel must propagate as Cancelled: {:?}",
482+
result
483+
);
484+
let cancelled = events.cancelled.lock().unwrap();
485+
assert!(
486+
!cancelled.is_empty(),
487+
"write-cancelled must be emitted before Cancelled propagates from scan",
488+
);
489+
assert!(
490+
!cancelled.first().unwrap().rolled_back,
491+
"scan-time cancel is a Stopped (not RollingBack) outcome"
492+
);
493+
494+
get_volume_manager().unregister(&vid);
495+
}

0 commit comments

Comments
 (0)