Skip to content

Commit 118ac6b

Browse files
committed
Transfer driver: bound async closures with Pin<Box<dyn Future + Send>>
- `drive_transfer_serial_async` previously used `AsyncFnMut(...)` bounds. The driver's own `#[tokio::test]`s passed, but production callers compile inside `tokio::spawn` and fail the Send check: `AsyncFnMut`'s HRTB-bound `CallRefFuture<'a>` is not provably Send for all `'a` when the closure body captures `&Arc<...>` or similar refs (rust-lang/rust#100013-class). - Switched the three closure bounds to `for<'a> FnMut(...) -> Pin<Box<dyn Future<...> + Send + 'a>>`. `+ Send` on the boxed trait object is what makes the driver's awaiting-this-future state Send. Driver loop body unchanged (each closure call is still `.await`-ed in sequence). - Migrated all 18 async tests from `async ||` syntax to explicit `Box::pin(async move { ... })`. Bodies unchanged; only the closure wrappers changed. Added 3 type aliases (`FetchFut`, `ResolveFut`, `TransferFut`) so the test closures stay readable. - Added `driver_future_is_send_across_spawn` regression test: runs the driver call through an explicit inner `tokio::spawn` boundary that captures an outer `Arc` by ref, which is the smallest setup that reproduced the original failure ("Send is not general enough"). Compiles and passes under the new bound. - Updated the driver's doc comment and the colocated `CLAUDE.md` to capture the rationale. - 30 tests pass (29 existing + 1 new). No production callers touched.
1 parent b6833e2 commit 118ac6b

3 files changed

Lines changed: 290 additions & 126 deletions

File tree

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,9 @@ exits, partial files or staging directories may remain on disk. These use the `.
219219
**Decision**: Volume copy pipeline uses `OperationEventSink` trait instead of `tauri::AppHandle`
220220
**Why**: Decouples the copy/move orchestration from the Tauri framework. `TauriEventSink` wraps AppHandle for production; `CollectorEventSink` stores events for test assertions. Enables testing `copy_volumes_with_progress` end-to-end (multi-file copy, cancellation, conflict resolution, progress tracking) without a Tauri runtime. Currently only the volume copy/move path is migrated; local copy/delete/trash still use AppHandle directly (to be migrated during the async Volume refactor).
221221

222+
**Decision**: `drive_transfer_serial_async` bounds its closures as `for<'a> FnMut(...) -> Pin<Box<dyn Future<...> + Send + 'a>>`, not `AsyncFnMut(...) -> T`.
223+
**Why**: Production callers live inside `tokio::spawn(async move { ... })` (see `volume_copy::copy_between_volumes`), so the driver's returned future must be `Send`. `AsyncFnMut`'s HRTB-bound `CallRefFuture<'a>` is not provably `Send` for all `'a` when the closure body captures `&Arc<...>` or similar refs — the compiler emits "implementation of Send is not general enough" because it can't discharge `for<'a> CallRefFuture<'a>: Send` (rust-lang/rust#100013-class). The explicit boxed-future shape moves the Send obligation inside the per-call return type, where it's discharged at each call site, and `+ Send` on the trait object is what makes the driver's awaiting-this-future state Send. The M2 step 0 prototype used `async ||` + `AsyncFnMut` and passed the driver's own `#[tokio::test]`s; the bug only surfaced when M3 migration started wiring real callers and the spawn-boundary Send check ran. `transfer_driver_tests.rs::driver_future_is_send_across_spawn` pins the contract by routing the driver call through an explicit `tokio::spawn` boundary.
224+
222225
**Decision**: `transfer_driver.rs` ships as two sibling entry points (sync + async), not one generic-over-AsyncFnMut-or-FnMut driver. Conflict resolution lives in the driver for the async path, in the closure for the sync path.
223226
**Why**: `copy_files_with_progress_inner` is sync inside `spawn_blocking`; the three volume ops are async. A single generic driver would either force the sync path through a `Pin<Box<dyn Future>>` per source (allocation per call, no real benefit since the I/O is sync) or use a trait so gnarly that the closures stop reading as straight-line transfer code. Two siblings share `TransferContext`, `TransferOutcome`, `TransferLoopOutcome`, and `build_pre_skip_set` / `emit_progress_and_status` helpers — the duplication is small. For conflict resolution: local-FS conflicts surface mid-flight at parent directories inside `copy_single_item` (a file blocking `create_dir_all`), which the driver can't pre-detect via top-level `dest.get_metadata`; so the sync driver delegates conflict resolution to the closure entirely. Volume ops have only top-level conflicts that always reduce to `resolve_volume_conflict`, so the async driver owns that dispatch (uniform shape across all 3 volume ops, exactly what we want to deduplicate). The data-safety contract (closure never invoked for pre-skipped / resolved-as-Skip / post-cancel) is enforced in both shapes by the driver's loop structure and pinned by `transfer_driver_tests.rs`. See `docs/specs/transfer-driver-refactor-plan.md` § "Design decisions" and § "Concurrent driver scope" for the full rationale; the concurrent path stays inline in `copy_volumes_with_progress` (1-of-4 abstraction not worth its weight).
224227

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

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,9 @@
102102
)]
103103

104104
use std::collections::HashSet;
105+
use std::future::Future;
105106
use std::path::{Path, PathBuf};
107+
use std::pin::Pin;
106108
use std::sync::Arc;
107109
use std::time::Instant;
108110

@@ -521,15 +523,24 @@ pub(super) enum ConflictDecision {
521523
/// e. If Proceed, invoke `transfer_one` with the resolved dest path.
522524
/// 3. Return `PostLoopIntent::Completed` if the loop drained without incident.
523525
///
524-
/// # Why `async ||` (Rust 2024 syntax) is required
526+
/// # Closure-bound shape: boxed future, not `AsyncFnMut`
525527
///
526-
/// The M2 step 0 prototype proved that the older `|ctx| async { ... }` form
527-
/// returns an async block that borrows the closure's `&mut` captures, which
528-
/// `AsyncFnMut` rejects ("returns an `async` block that contains a reference
529-
/// to a captured variable, which then escapes the closure body"). The
530-
/// `async ||` form ties the returned future to `&mut self` of the closure
531-
/// call, which is what `AsyncFnMut` actually expects. The codebase is on
532-
/// edition 2024 so the syntax is available.
528+
/// Each closure returns `Pin<Box<dyn Future<...> + Send + 'a>>` rather than
529+
/// being bound as `AsyncFnMut(...) -> T`. The explicit boxed-future shape is
530+
/// load-bearing for production callers: the driver's returned future must be
531+
/// `Send` so callers can `tokio::spawn` it, and `AsyncFnMut`'s HRTB-bound
532+
/// `CallRefFuture<'a>` is not provably `Send` for all `'a` when the closure
533+
/// body captures `&Arc<...>` or similar refs (rust-lang/rust#100013-class).
534+
/// The `+ Send` on the boxed future moves the Send obligation inside the
535+
/// per-call return type, where it's discharged at each call site.
536+
///
537+
/// `transfer_driver_tests.rs::driver_future_is_send_across_spawn` pins this:
538+
/// the driver call must compile inside a `tokio::spawn(async move { ... })`,
539+
/// which fails under `AsyncFnMut` and passes under the boxed-future shape.
540+
///
541+
/// The driver loop is still single-threaded (one `.await` per closure call
542+
/// in sequence); only the FUTURE returned by each call needs `Send`. The
543+
/// closure itself doesn't need to be `Send`.
533544
#[allow(
534545
clippy::too_many_arguments,
535546
reason = "Driver wraps the full per-iter context; bundling adds ceremony without removing parameters"
@@ -551,9 +562,17 @@ pub(super) async fn drive_transfer_serial_async<DestMetaFetcher, ConflictResolve
551562
mut transfer_one: TransferOne,
552563
) -> TransferLoopOutcome
553564
where
554-
DestMetaFetcher: AsyncFnMut(&Path) -> Option<u64>,
555-
ConflictResolver: AsyncFnMut(ConflictDecisionInput<'_>) -> Result<ConflictDecision, WriteOperationError>,
556-
TransferOne: AsyncFnMut(TransferContext<'_>) -> Result<TransferOutcome, WriteOperationError>,
565+
DestMetaFetcher: for<'a> FnMut(&'a Path) -> Pin<Box<dyn Future<Output = Option<u64>> + Send + 'a>>,
566+
ConflictResolver: for<'a> FnMut(
567+
ConflictDecisionInput<'a>,
568+
) -> Pin<
569+
Box<dyn Future<Output = Result<ConflictDecision, WriteOperationError>> + Send + 'a>,
570+
>,
571+
TransferOne: for<'a> FnMut(
572+
TransferContext<'a>,
573+
) -> Pin<
574+
Box<dyn Future<Output = Result<TransferOutcome, WriteOperationError>> + Send + 'a>,
575+
>,
557576
{
558577
let mut files_done = 0usize;
559578
let mut bytes_done = 0u64;

0 commit comments

Comments
 (0)