Skip to content

Commit f894e60

Browse files
committed
Write ops: per-op settle event for honest cancel UX (M4)
- New `write-settled` Tauri event, emitted exactly once per op after the spawned background task fully returns. Implemented via a `WriteSettledGuard` RAII struct whose `Drop` fires the event — runs even on handler panic, so the FE never hangs waiting for a settle that never comes. - Wired into all five spawn-task entry points: `start_write_operation` (local copy/move/delete/trash), the volume-delete branch in `delete_files_start`, `copy_between_volumes`, `move_between_volumes`, and `move_within_same_volume`. Cache cleanup happens before the guard drops; ordering guarantee: terminal event (`write-cancelled` / `write-complete` / `write-error`) → cache cleanup → `write-settled`. - FE `TransferProgressDialog` gates the cancel close on BOTH `write-cancelled` AND `write-settled`. The dialog stays in "Cancelling…" until settle lands, then applies the existing 400 ms display floor and closes. Defensive against out-of-order delivery. Complete / error paths are unchanged. - After 200 ms of waiting for settle, the label gains "(finishing USB transfers)" so users see real work, not a generic spinner. - 4 backend tests pin the guard invariants (single fire, panic safety, ordering). 3 backend integration tests pin volume-delete cancel / complete / error each emit settle. 4 Vitest tests pin the FE dialog's two-condition close, out-of-order ordering protection, and operationId filtering. 1 Playwright spec covers the full MTP cancel-then-F8-again flow. - Docs: new "Settle contract" section in `write_operations/CLAUDE.md`; "Cancel close is two-condition" gotcha added in `file-operations/CLAUDE.md`.
1 parent 1696355 commit f894e60

16 files changed

Lines changed: 1433 additions & 42 deletions

File tree

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,13 +209,41 @@ exits, partial files or staging directories may remain on disk. These use the `.
209209
| `write-complete` | Operation finished successfully |
210210
| `write-cancelled` | Operation cancelled (includes `rolled_back` flag) |
211211
| `write-error` | Operation failed. Carries `error: WriteOperationError` (typed) plus `friendly: FriendlyError` (rendered title/explanation/suggestion + category) populated by `WriteErrorEvent::new` via `friendly_from_write_error`. The FE renders the `friendly` payload directly in `TransferErrorDialog` and applies category-based colors. |
212+
| `write-settled` | Emitted once per op after the spawned background task fully returns. See [Settle contract](#settle-contract). |
212213
| `write-source-item-done` | All files for a top-level source item processed (for gradual deselection) |
213214
| `dry-run-complete` | `config.dry_run == true` (returns `DryRunResult`) |
214215
| `scan-preview-progress` | During `start_scan_preview` |
215216
| `scan-preview-complete` | Preview scan finished |
216217
| `scan-preview-error` | Preview scan failed |
217218
| `scan-preview-cancelled` | Preview scan cancelled |
218219

220+
## Settle contract
221+
222+
`write-settled` fires exactly once per operation, after the spawned background task has fully torn down — including
223+
in-flight USB / network teardown that may briefly outlive the `write-cancelled` emit. The FE uses it to gate the
224+
"Cancelling…" dialog close so the user can't dispatch a new op against a still-tearing-down volume (the wedge mode
225+
the M2 cancel propagation already shortens but doesn't eliminate).
226+
227+
**Ordering**: `write-settled` always fires AFTER the terminal outcome event (`write-complete` / `write-cancelled` /
228+
`write-error`) for the same `operation_id`. The BE guarantees this by placing the settle emit in a `WriteSettledGuard`
229+
RAII struct whose `Drop` runs at the very end of the spawn-task scope, AFTER all the conditional terminal-event emits.
230+
231+
**Guard pattern**: every spawn-task entry point (`start_write_operation` in `mod.rs`, the volume-delete branch in
232+
`delete_files_start`, `copy_between_volumes`, `move_between_volumes`, `move_within_same_volume`) constructs a
233+
`WriteSettledGuard` at the top of the spawned task. The guard's `Drop` impl emits the event. This makes the emit
234+
panic-safe: even if the handler closure panics and the task exits via `JoinError`, the guard still drops as part of
235+
stack unwinding, so the FE never hangs waiting for a settle that never comes. See
236+
`settle_event_tests.rs::settled_fires_on_panic_unwind` for the safety-net pin.
237+
238+
**Payload**: `{ operationId: String, operationType, volumeId: Option<String> }`. The `volume_id` is best-effort: filled
239+
with the source volume's display name for volume-aware ops (copy/move between volumes, volume delete), `None` for pure
240+
local-FS operations. The FE currently filters only by `operationId`; `volume_id` is for diagnostics and forward
241+
compatibility.
242+
243+
**Tests**: `settle_event_tests.rs` pins the guard's invariants (single fire, panic safety, ordering relative to the
244+
terminal event). `volume_cancel_tests::volume_*_emits_write_settled_event` pin the integration shape against the
245+
volume-delete handler.
246+
219247
## Key decisions
220248

221249
**Decision**: `walk_dir_recursive` dedupes hardlinks by inode when summing `total_bytes`.

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

Lines changed: 40 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ use helpers::{
5353
use move_op::move_files_with_progress;
5454
#[cfg(not(test))]
5555
use state::WriteOperationState;
56-
use state::{WRITE_OPERATION_STATE, register_operation_status, unregister_operation_status};
56+
use state::{WRITE_OPERATION_STATE, WriteSettledGuard, register_operation_status, unregister_operation_status};
5757
use trash::trash_files_with_progress;
5858

5959
// Re-export public types
@@ -68,7 +68,7 @@ pub use types::{
6868
ScanPreviewCompleteEvent, ScanPreviewErrorEvent, ScanPreviewProgressEvent, ScanPreviewStartResult,
6969
ScanProgressEvent, SortColumn, SortOrder, WriteCancelledEvent, WriteCompleteEvent, WriteConflictEvent,
7070
WriteErrorEvent, WriteOperationConfig, WriteOperationError, WriteOperationPhase, WriteOperationStartResult,
71-
WriteOperationType, WriteProgressEvent,
71+
WriteOperationType, WriteProgressEvent, WriteSettledEvent,
7272
};
7373

7474
// Re-export for tests (these are pub(crate) in helpers.rs and state.rs)
@@ -128,14 +128,19 @@ where
128128
tokio::spawn(async move {
129129
let operation_id_for_cleanup = operation_id_for_spawn.clone();
130130
let app_for_error = app.clone();
131+
// RAII guard: emits `write-settled` when this task exits, no matter
132+
// how (handler success, error, cancel, or panic via JoinError). FE
133+
// gates the "Cancelling…" dialog close on this event so the user
134+
// can't dispatch a new op against a still-tearing-down volume.
135+
let _settled_guard = WriteSettledGuard::new(
136+
app_for_error.clone(),
137+
operation_id_for_cleanup.clone(),
138+
operation_type,
139+
None,
140+
);
131141

132142
let result = tokio::task::spawn_blocking(move || handler(app, operation_id_for_spawn, state)).await;
133143

134-
if let Ok(mut cache) = WRITE_OPERATION_STATE.write() {
135-
cache.remove(&operation_id_for_cleanup);
136-
}
137-
unregister_operation_status(&operation_id_for_cleanup);
138-
139144
use tauri::Emitter;
140145
match result {
141146
Ok(Ok(())) => {} // Handler already emitted write-complete or write-cancelled
@@ -146,15 +151,15 @@ where
146151
// Handler error (validation, I/O, etc.): emit write-error as safety net
147152
let _ = app_for_error.emit(
148153
"write-error",
149-
WriteErrorEvent::new(operation_id_for_cleanup, operation_type, e),
154+
WriteErrorEvent::new(operation_id_for_cleanup.clone(), operation_type, e),
150155
);
151156
}
152157
Err(join_error) => {
153158
// Panic/abort in spawn_blocking
154159
let _ = app_for_error.emit(
155160
"write-error",
156161
WriteErrorEvent::new(
157-
operation_id_for_cleanup,
162+
operation_id_for_cleanup.clone(),
158163
operation_type,
159164
WriteOperationError::IoError {
160165
path: String::new(),
@@ -164,6 +169,14 @@ where
164169
);
165170
}
166171
}
172+
173+
// Cache cleanup happens AFTER terminal events are emitted, BEFORE the
174+
// settle guard drops (the guard is the very last thing in scope).
175+
// Order: terminal event → cache removal → `write-settled` via Drop.
176+
if let Ok(mut cache) = WRITE_OPERATION_STATE.write() {
177+
cache.remove(&operation_id_for_cleanup);
178+
}
179+
unregister_operation_status(&operation_id_for_cleanup);
167180
});
168181

169182
Ok(WriteOperationStartResult {
@@ -272,6 +285,16 @@ pub async fn delete_files_start(
272285
tokio::spawn(async move {
273286
let operation_id_for_cleanup = operation_id_for_spawn.clone();
274287
let app_for_error = app.clone();
288+
// RAII settle guard: fires `write-settled` when the task exits.
289+
// Drop runs last in scope so the FE sees the terminal event
290+
// (write-cancelled / write-error / write-complete) first, then
291+
// settle, then is free to clear the dialog.
292+
let _settled_guard = WriteSettledGuard::new(
293+
app_for_error.clone(),
294+
operation_id_for_cleanup.clone(),
295+
WriteOperationType::Delete,
296+
Some(volume_id_str.clone()),
297+
);
275298

276299
let volume = match crate::file_system::get_volume_manager().get(&volume_id_str) {
277300
Some(v) => v,
@@ -307,22 +330,22 @@ pub async fn delete_files_start(
307330
)
308331
.await;
309332

310-
if let Ok(mut cache) = WRITE_OPERATION_STATE.write() {
311-
cache.remove(&operation_id_for_cleanup);
312-
}
313-
unregister_operation_status(&operation_id_for_cleanup);
314-
315333
use tauri::Emitter;
316334
match result {
317335
Ok(()) => {}
318336
Err(ref e) if matches!(e, WriteOperationError::Cancelled { .. }) => {}
319337
Err(e) => {
320338
let _ = app_for_error.emit(
321339
"write-error",
322-
WriteErrorEvent::new(operation_id_for_cleanup, WriteOperationType::Delete, e),
340+
WriteErrorEvent::new(operation_id_for_cleanup.clone(), WriteOperationType::Delete, e),
323341
);
324342
}
325343
}
344+
345+
if let Ok(mut cache) = WRITE_OPERATION_STATE.write() {
346+
cache.remove(&operation_id_for_cleanup);
347+
}
348+
unregister_operation_status(&operation_id_for_cleanup);
326349
});
327350

328351
Ok(WriteOperationStartResult {
@@ -379,6 +402,8 @@ mod move_integration_test;
379402
#[cfg(test)]
380403
mod scan_preview_oracle_tests;
381404
#[cfg(test)]
405+
mod settle_event_tests;
406+
#[cfg(test)]
382407
mod tests;
383408
#[cfg(test)]
384409
mod transaction_integration_test;
Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
1+
//! Tests for the `write-settled` per-op event (M4 of `cancel-settled-plan.md`).
2+
//!
3+
//! The `WriteSettledGuard` RAII pattern emits exactly one `write-settled`
4+
//! event when the spawned task fully returns — happy path, error path,
5+
//! cancellation, or panic. These tests pin the contract.
6+
//!
7+
//! Production code emits via `app.emit("write-settled", ...)` from inside
8+
//! the guard's `Drop`. Tests use the alternate `new_with_sink` constructor to
9+
//! redirect into a `CollectorEventSink`, so the full lifecycle runs without
10+
//! a Tauri runtime.
11+
12+
use std::sync::Arc;
13+
14+
use super::state::WriteSettledGuard;
15+
use super::types::{CollectorEventSink, OperationEventSink, WriteOperationType, WriteSettledEvent};
16+
17+
/// Bridge sink that keeps a direct handle to the underlying `CollectorEventSink`
18+
/// for inspection. Needed because the guard takes `Arc<dyn OperationEventSink>`,
19+
/// and `dyn OperationEventSink` isn't downcastable.
20+
struct Bridge {
21+
inner: Arc<CollectorEventSink>,
22+
}
23+
24+
impl OperationEventSink for Bridge {
25+
fn emit_progress(&self, e: super::types::WriteProgressEvent) {
26+
self.inner.emit_progress(e);
27+
}
28+
fn emit_complete(&self, e: super::types::WriteCompleteEvent) {
29+
self.inner.emit_complete(e);
30+
}
31+
fn emit_cancelled(&self, e: super::types::WriteCancelledEvent) {
32+
self.inner.emit_cancelled(e);
33+
}
34+
fn emit_error(&self, e: super::types::WriteErrorEvent) {
35+
self.inner.emit_error(e);
36+
}
37+
fn emit_conflict(&self, e: super::types::WriteConflictEvent) {
38+
self.inner.emit_conflict(e);
39+
}
40+
fn emit_source_item_done(&self, e: super::types::WriteSourceItemDoneEvent) {
41+
self.inner.emit_source_item_done(e);
42+
}
43+
fn emit_scan_progress(&self, e: super::types::ScanProgressEvent) {
44+
self.inner.emit_scan_progress(e);
45+
}
46+
fn emit_scan_conflict(&self, c: super::types::ConflictInfo) {
47+
self.inner.emit_scan_conflict(c);
48+
}
49+
fn emit_dry_run_complete(&self, r: super::types::DryRunResult) {
50+
self.inner.emit_dry_run_complete(r);
51+
}
52+
fn emit_settled(&self, e: WriteSettledEvent) {
53+
self.inner.emit_settled(e);
54+
}
55+
}
56+
57+
fn pair() -> (Arc<dyn OperationEventSink>, Arc<CollectorEventSink>) {
58+
let inner = Arc::new(CollectorEventSink::new());
59+
let bridge: Arc<dyn OperationEventSink> = Arc::new(Bridge {
60+
inner: Arc::clone(&inner),
61+
});
62+
(bridge, inner)
63+
}
64+
65+
#[test]
66+
fn settled_fires_once_on_normal_drop() {
67+
let (sink, collector) = pair();
68+
69+
{
70+
let _guard =
71+
WriteSettledGuard::new_with_sink(sink, "op-happy", WriteOperationType::Copy, Some("test-vol".to_string()));
72+
// Guard drops at end of scope.
73+
}
74+
75+
let settled = collector.settled.lock().unwrap();
76+
assert_eq!(settled.len(), 1, "guard must fire exactly one write-settled event");
77+
assert_eq!(settled[0].operation_id, "op-happy");
78+
assert_eq!(settled[0].operation_type, WriteOperationType::Copy);
79+
assert_eq!(settled[0].volume_id.as_deref(), Some("test-vol"));
80+
}
81+
82+
#[test]
83+
fn settled_fires_with_none_volume_for_local_ops() {
84+
let (sink, collector) = pair();
85+
86+
drop(WriteSettledGuard::new_with_sink(
87+
sink,
88+
"op-local",
89+
WriteOperationType::Delete,
90+
None,
91+
));
92+
93+
let settled = collector.settled.lock().unwrap();
94+
assert_eq!(settled.len(), 1);
95+
assert!(
96+
settled[0].volume_id.is_none(),
97+
"local-FS settle event must carry volume_id=None"
98+
);
99+
}
100+
101+
#[test]
102+
fn settled_fires_on_panic_unwind() {
103+
// The guard is in scope when the closure panics. `catch_unwind` swallows
104+
// the panic, but the guard's Drop still runs as part of stack unwinding.
105+
// This is the panic-safety property the FE relies on so the dialog can
106+
// always clear, even on a backend bug.
107+
let (sink, collector) = pair();
108+
109+
let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(move || {
110+
let _guard =
111+
WriteSettledGuard::new_with_sink(sink, "op-panic", WriteOperationType::Move, Some("v".to_string()));
112+
panic!("simulated handler panic");
113+
}));
114+
115+
assert!(result.is_err(), "the closure must have panicked");
116+
let settled = collector.settled.lock().unwrap();
117+
assert_eq!(
118+
settled.len(),
119+
1,
120+
"settle event must fire even when the spawned task panicked: the FE would hang otherwise"
121+
);
122+
assert_eq!(settled[0].operation_id, "op-panic");
123+
}
124+
125+
#[test]
126+
fn settled_event_order_is_after_terminal_outcome_event() {
127+
// The FE depends on this ordering: it sees the terminal event
128+
// (write-cancelled / write-complete / write-error) first, knows the
129+
// outcome, then sees write-settled and knows the volume is ready.
130+
// Reversing would race: the dialog could try to close on settle before
131+
// it knows the outcome.
132+
//
133+
// Mirrors how production spawn tasks emit: handler emits the terminal
134+
// event, then state cleanup runs, then the guard's Drop emits settled.
135+
let (sink, collector) = pair();
136+
let op_id = "op-order".to_string();
137+
138+
{
139+
let _guard = WriteSettledGuard::new_with_sink(
140+
Arc::clone(&sink),
141+
op_id.clone(),
142+
WriteOperationType::Delete,
143+
Some("v1".to_string()),
144+
);
145+
// Simulate the handler's terminal emit.
146+
sink.emit_cancelled(super::types::WriteCancelledEvent {
147+
operation_id: op_id.clone(),
148+
operation_type: WriteOperationType::Delete,
149+
files_processed: 7,
150+
rolled_back: false,
151+
});
152+
// Guard's Drop runs at end of scope, AFTER the cancelled emit above.
153+
}
154+
155+
let cancelled = collector.cancelled.lock().unwrap();
156+
let settled = collector.settled.lock().unwrap();
157+
assert_eq!(cancelled.len(), 1, "cancelled should fire first");
158+
assert_eq!(settled.len(), 1, "settled should fire second");
159+
assert_eq!(settled[0].operation_id, op_id);
160+
}
161+
162+
#[test]
163+
fn settled_fires_for_every_operation_type() {
164+
// Trivial coverage: each op-type variant flows through the same path,
165+
// but we pin it so a future refactor that branches on type won't
166+
// accidentally drop one.
167+
for op_type in [
168+
WriteOperationType::Copy,
169+
WriteOperationType::Move,
170+
WriteOperationType::Delete,
171+
WriteOperationType::Trash,
172+
] {
173+
let (sink, collector) = pair();
174+
drop(WriteSettledGuard::new_with_sink(
175+
sink,
176+
format!("op-{:?}", op_type),
177+
op_type,
178+
None,
179+
));
180+
let settled = collector.settled.lock().unwrap();
181+
assert_eq!(settled.len(), 1, "settle must fire once per op_type={:?}", op_type);
182+
assert_eq!(settled[0].operation_type, op_type);
183+
}
184+
}

0 commit comments

Comments
 (0)