Skip to content

Commit 816e9e1

Browse files
vdavidvdavid
committed
Bugfix: Fix copy cancellation on network mounts
chunked_copy for everyone! (except APFS) - `copyfile(3)` ignores `COPYFILE_QUIT` when source or dest is on a network mount, causing cancellation to take 30+ seconds or silently complete the full copy - On macOS, now use `copyfile(3)` only for same-APFS-volume copies (the only case where clonefile works); use chunked copy for everything else - Check cancellation after `copyfile` returns regardless of result code (safety net for when `copyfile` returns 0 despite `COPYFILE_QUIT`) - Deduplicate cancellation log messages via `AtomicBool` (was logging ~1000 identical lines while buffers drained) - On Linux, check both source and destination for network filesystem (was only checking dest) - Delete macOS `is_network_filesystem` (replaced by `is_same_apfs_volume` in copy strategy) - Evaluated `copyfile` on HFS+, exFAT, FAT32, NTFS-3G — no practical benefit over chunked copy on any of them Entire-Checkpoint: 88f2e62bb2e6
1 parent 3e657bd commit 816e9e1

4 files changed

Lines changed: 156 additions & 107 deletions

File tree

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

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ network mounts, cross-filesystem moves, and name/path length limits.
2424
| `copy_strategy.rs` | Strategy selection per file: network FS → chunked copy; overwrite → temp+rename; macOS → `copyfile(3)`; Linux → `copy_file_range(2)`. |
2525
| `macos_copy.rs` | FFI to macOS `copyfile(3)`. Preserves xattrs, ACLs, resource forks, Finder metadata. Supports APFS `clonefile`. |
2626
| `linux_copy.rs` | Linux `copy_file_range(2)` with reflink support on btrfs/XFS. 4 MB chunks, cancellation between iterations. |
27-
| `chunked_copy.rs` | 1 MB chunked read/write for network mounts. Checks cancellation between chunks. Copies xattrs, ACLs, timestamps. |
27+
| `chunked_copy.rs` | 1 MB chunked read/write — the default copy method for all non-APFS-clonefile copies on macOS and network copies on Linux. Checks cancellation between chunks. Copies xattrs, ACLs, timestamps. |
2828
| `volume_copy.rs`, `volume_conflict.rs`, `volume_strategy.rs` | Volume-to-volume copy (Local↔MTP abstraction). Publicly re-exported from `mod.rs` and at least partially wired up. |
2929
| `tests.rs`, `integration_test.rs` | Unit and integration tests. |
3030

@@ -82,10 +82,10 @@ actual `copy_files_start` can consume the cache via `preview_id` in `WriteOperat
8282
`.cmdr-staging-<uuid>` dir at the destination root, then atomic `rename` into place, then source deletion.
8383

8484
**Copy strategy selection** (`copy_strategy.rs`):
85-
- Destination is a network mount → `chunked_copy_with_metadata` (macOS `copyfile` ignores `COPYFILE_QUIT` on network mounts)
86-
- Needs safe overwrite → `safe_overwrite_file`
87-
- macOS → `copy_single_file_native` (macOS `copyfile(3)`, supports `COPYFILE_CLONE` for APFS instant copies)
88-
- Linux → `copy_single_file_linux` (Linux `copy_file_range(2)`, supports reflink on btrfs/XFS)
85+
- macOS, same APFS volume → `copyfile(3)` with `COPYFILE_CLONE` for instant clonefile
86+
- macOS, everything else → `chunked_copy_with_metadata` (1 MB chunks, cancellation between chunks)
87+
- Linux, network → `chunked_copy_with_metadata`
88+
- Linux, local → `copy_single_file_linux` (`copy_file_range(2)`, supports reflink on btrfs/XFS)
8989
- Other platforms → `std::fs::copy` fallback
9090

9191
**Trash has no scan phase.** `trashItemAtURL` is atomic per top-level item (the OS moves the entire tree), so trash
@@ -125,6 +125,27 @@ operations when the frontend is destroyed.
125125
**Decision**: Keep `exacl` crate for ACL copy in chunked copies (not custom FFI bindings).
126126
**Why**: `exacl` adds zero new transitive dependencies (all of its deps — `bitflags`, `log`, `scopeguard`, `uuid` — are already in our tree). It provides cross-platform ACL support (macOS, Linux, FreeBSD) and full ACL parsing/manipulation for potential future UI features. The crate appears unmaintained (last release Feb 2024) but ACL APIs are stable and don't change. Our usage is best-effort with graceful fallback — if `exacl` ever breaks, files still copy, they just lose ACLs. MIT licensed (compatible with BSL).
127127

128+
**Decision**: On macOS, use `copyfile(3)` only for same-APFS-volume copies; use chunked copy for everything else.
129+
**Why**: The only practical benefit of `copyfile(3)` is APFS clonefile (instant copy-on-write, zero extra disk usage),
130+
which only works on the same APFS volume. We evaluated `copyfile` on other filesystems:
131+
- **HFS+**: No clonefile. Marginal metadata edge (birthtime, file flags), but HFS+ is rare since Apple converted all
132+
Macs to APFS in 2017.
133+
- **exFAT / FAT32**: No clonefile, no xattrs, no ACLs, no file flags — the metadata `copyfile` would preserve doesn't
134+
exist on these filesystems. No practical benefit.
135+
- **NTFS-3G**: FUSE-based, so `copyfile` goes through userspace with the same I/O buffering issues as network mounts.
136+
`COPYFILE_QUIT` is unreliable. No benefit.
137+
- **Network mounts (SMB, NFS, AFP, WebDAV)**: `copyfile` ignores `COPYFILE_QUIT` while draining buffered I/O, causing
138+
cancellation to take 30+ seconds or complete the copy entirely. This applies when *either* the source or destination
139+
is on a network mount (for example, NAS-to-local copies).
140+
- **USB / external drives**: Typically exFAT or HFS+ — no clonefile. Different volume from the internal drive, so no
141+
same-volume benefits.
142+
143+
Our chunked copy (1 MB read/write chunks) provides: identical speed for non-clonefile copies, reliable cancellation
144+
between chunks, and granular progress callbacks. It preserves xattrs (including resource forks), ACLs, timestamps, and
145+
permissions. The only metadata it doesn't preserve is birthtime (creation date) and file flags (`chflags`), which
146+
matter only on same-volume copies where we use `copyfile` anyway. Detection uses `st_dev` (device ID) for same-volume
147+
and `statfs.f_fstypename` for APFS. See `copy_strategy.rs` for the implementation.
148+
128149
## Dependencies
129150

130151
- `crate::file_system::volume` — `Volume` trait, `SpaceInfo`, `ScanConflict` (used by `volume_copy`)

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

Lines changed: 3 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -30,57 +30,14 @@ const CHUNK_SIZE: usize = 1024 * 1024;
3030
///
3131
/// Returns `true` for SMB, NFS, AFP, and WebDAV filesystems.
3232
/// Returns `false` for local filesystems (APFS, HFS+, etc.) or if detection fails.
33-
#[cfg(target_os = "macos")]
34-
pub fn is_network_filesystem(path: &Path) -> bool {
35-
use std::ffi::CString;
36-
37-
// For non-existent paths, check the parent directory
38-
let check_path = if path.exists() {
39-
path.to_path_buf()
40-
} else {
41-
match path.parent() {
42-
Some(p) if p.exists() => p.to_path_buf(),
43-
_ => return false,
44-
}
45-
};
46-
47-
let c_path = match CString::new(check_path.to_string_lossy().as_bytes()) {
48-
Ok(p) => p,
49-
Err(_) => return false,
50-
};
51-
52-
let mut stat: libc::statfs = unsafe { std::mem::zeroed() };
53-
if unsafe { libc::statfs(c_path.as_ptr(), &mut stat) } != 0 {
54-
return false;
55-
}
56-
57-
// SAFETY: f_fstypename is a null-terminated C string
58-
let fstype = unsafe { std::ffi::CStr::from_ptr(stat.f_fstypename.as_ptr()).to_string_lossy() };
59-
60-
let is_network = matches!(fstype.as_ref(), "smbfs" | "nfs" | "afpfs" | "webdav");
61-
62-
if is_network {
63-
log::debug!(
64-
"is_network_filesystem: {} is on network filesystem type '{}'",
65-
path.display(),
66-
fstype
67-
);
68-
}
69-
70-
is_network
71-
}
72-
33+
///
34+
/// On macOS, copy strategy uses `is_same_apfs_volume` instead (see `copy_strategy.rs`).
35+
/// This function is only used on Linux.
7336
#[cfg(target_os = "linux")]
7437
pub fn is_network_filesystem(path: &Path) -> bool {
7538
crate::file_system::linux_mounts::is_network_filesystem_linux(path)
7639
}
7740

78-
#[cfg(not(any(target_os = "macos", target_os = "linux")))]
79-
pub fn is_network_filesystem(_path: &Path) -> bool {
80-
// On unsupported platforms, assume local filesystem
81-
false
82-
}
83-
8441
// ============================================================================
8542
// Chunked copy with metadata
8643
// ============================================================================
@@ -347,20 +304,6 @@ mod tests {
347304
let _ = fs::remove_dir_all(path);
348305
}
349306

350-
#[test]
351-
fn test_is_network_filesystem_local() {
352-
// Local paths should return false
353-
assert!(!is_network_filesystem(Path::new("/")));
354-
assert!(!is_network_filesystem(Path::new("/tmp")));
355-
assert!(!is_network_filesystem(Path::new("/Users")));
356-
}
357-
358-
#[test]
359-
fn test_is_network_filesystem_nonexistent() {
360-
// Non-existent paths should check parent
361-
assert!(!is_network_filesystem(Path::new("/tmp/nonexistent_file_12345")));
362-
}
363-
364307
#[test]
365308
fn test_chunked_copy_basic() {
366309
let temp_dir = create_temp_dir("basic");

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

Lines changed: 106 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,22 @@
11
//! Copy strategy selection for file operations.
22
//!
3-
//! Encapsulates the decision of which copy method to use:
4-
//! - Network filesystems: chunked copy for responsive cancellation
5-
//! - Safe overwrite needed: temp file + rename pattern
6-
//! - macOS: native copyfile(3) with APFS clonefile support
7-
//! - Linux: copy_file_range(2) with reflink support on btrfs/XFS
8-
//! - Other platforms: std::fs::copy fallback
3+
//! The only reason to use platform-native copy APIs (`copyfile(3)`, `copy_file_range(2)`) is
4+
//! filesystem-level cloning (APFS clonefile, btrfs/XFS reflink) — instant, zero-cost copies
5+
//! that create a copy-on-write pointer instead of copying bytes. In all other cases, our chunked
6+
//! copy is equivalent in speed and strictly better for progress reporting and cancellation.
7+
//!
8+
//! Strategy (macOS):
9+
//! - Same APFS volume → `copyfile(3)` with `COPYFILE_CLONE` for instant clonefile
10+
//! - Everything else → chunked copy (1 MB chunks, cancellation between chunks)
11+
//!
12+
//! Strategy (Linux):
13+
//! - Local, non-network → `copy_file_range(2)` (kernel handles reflink on btrfs/XFS)
14+
//! - Network → chunked copy
15+
//!
16+
//! We evaluated `copyfile` on non-APFS filesystems (HFS+, exFAT, FAT32, NTFS-3G) and found no
17+
//! practical benefit: no clonefile support, and the metadata advantages (birthtime, file flags)
18+
//! either don't apply (exFAT/FAT32 don't store them) or aren't worth the cancellation tradeoff
19+
//! (NTFS-3G is FUSE-based and has the same buffering issues as network mounts). See CLAUDE.md.
920
1021
#[cfg(not(any(target_os = "macos", target_os = "linux")))]
1122
use std::fs;
@@ -18,20 +29,82 @@ use super::linux_copy::copy_single_file_linux;
1829
#[cfg(target_os = "macos")]
1930
use super::macos_copy::{CopyProgressContext, copy_single_file_native};
2031

21-
use super::chunked_copy::{ChunkedCopyProgressFn, chunked_copy_with_metadata, is_network_filesystem};
32+
use super::chunked_copy::ChunkedCopyProgressFn;
33+
#[cfg(any(target_os = "macos", target_os = "linux"))]
34+
use super::chunked_copy::chunked_copy_with_metadata;
35+
#[cfg(target_os = "linux")]
36+
use super::chunked_copy::is_network_filesystem;
2237
use super::helpers::safe_overwrite_file;
2338
#[cfg(not(any(target_os = "macos", target_os = "linux")))]
2439
use super::types::IoResultExt;
2540
use super::types::WriteOperationError;
2641

27-
/// Copies file contents using the appropriate strategy based on destination type.
42+
// ============================================================================
43+
// macOS: APFS clonefile detection
44+
// ============================================================================
45+
46+
/// Returns true if source and dest are on the same APFS volume (clonefile is possible).
47+
///
48+
/// Checks two things:
49+
/// 1. Same volume via `st_dev` (device ID from `stat`) — same approach as `is_same_filesystem`
50+
/// 2. Filesystem type is APFS via `statfs.f_fstypename`
2851
///
29-
/// Strategy selection:
30-
/// - Network filesystems: chunked copy for responsive cancellation
31-
/// - Safe overwrite needed: temp file + rename pattern to preserve original on failure
32-
/// - macOS: native copyfile(3) with APFS clonefile support
33-
/// - Linux: copy_file_range(2) with reflink support on btrfs/XFS
34-
/// - Other platforms: std::fs::copy fallback
52+
/// Handles non-existent destination paths by checking the parent directory.
53+
#[cfg(target_os = "macos")]
54+
fn is_same_apfs_volume(source: &Path, dest: &Path) -> bool {
55+
use std::os::unix::fs::MetadataExt;
56+
57+
// Check same volume via device ID (works even when dest doesn't exist — we check parent)
58+
let src_dev = match std::fs::metadata(source) {
59+
Ok(m) => m.dev(),
60+
Err(_) => return false,
61+
};
62+
let dest_check_path = if dest.exists() {
63+
dest.to_path_buf()
64+
} else {
65+
match dest.parent() {
66+
Some(p) if p.exists() => p.to_path_buf(),
67+
_ => return false,
68+
}
69+
};
70+
let dest_dev = match std::fs::metadata(&dest_check_path) {
71+
Ok(m) => m.dev(),
72+
Err(_) => return false,
73+
};
74+
if src_dev != dest_dev {
75+
return false;
76+
}
77+
78+
// Same volume — now check if it's APFS (only APFS supports clonefile)
79+
is_apfs(source)
80+
}
81+
82+
/// Returns true if the path is on an APFS volume.
83+
#[cfg(target_os = "macos")]
84+
fn is_apfs(path: &Path) -> bool {
85+
use std::ffi::CString;
86+
87+
let c_path = match CString::new(path.to_string_lossy().as_bytes()) {
88+
Ok(p) => p,
89+
Err(_) => return false,
90+
};
91+
let mut stat: libc::statfs = unsafe { std::mem::zeroed() };
92+
if unsafe { libc::statfs(c_path.as_ptr(), &mut stat) } != 0 {
93+
return false;
94+
}
95+
let fstype = unsafe { std::ffi::CStr::from_ptr(stat.f_fstypename.as_ptr()).to_string_lossy() };
96+
fstype == "apfs"
97+
}
98+
99+
// ============================================================================
100+
// Strategy selection
101+
// ============================================================================
102+
103+
/// Copies file contents using the best strategy for the source/destination combination.
104+
///
105+
/// On macOS, uses `copyfile(3)` only for same-APFS-volume copies (APFS clonefile — instant,
106+
/// zero-cost copy-on-write). All other cases use chunked copy for reliable cancellation and
107+
/// progress reporting.
35108
#[cfg(target_os = "macos")]
36109
pub(super) fn copy_file_with_strategy(
37110
source: &Path,
@@ -40,18 +113,25 @@ pub(super) fn copy_file_with_strategy(
40113
cancelled: &Arc<AtomicBool>,
41114
progress_callback: Option<ChunkedCopyProgressFn>,
42115
) -> Result<u64, WriteOperationError> {
43-
if is_network_filesystem(dest) {
116+
if is_same_apfs_volume(source, dest) {
44117
log::debug!(
45-
"copy_file_with_strategy: using chunked copy for network destination {}",
118+
"copy_file_with_strategy: same APFS volume, using copyfile for clonefile (src={}, dest={})",
119+
source.display(),
46120
dest.display()
47121
);
48-
chunked_copy_with_metadata(source, dest, cancelled, progress_callback)
49-
} else if needs_safe_overwrite {
50122
let context = CopyProgressContext::with_cancellation(Arc::clone(cancelled));
51-
safe_overwrite_file(source, dest, Some(&context))
123+
if needs_safe_overwrite {
124+
safe_overwrite_file(source, dest, Some(&context))
125+
} else {
126+
copy_single_file_native(source, dest, false, Some(&context))
127+
}
52128
} else {
53-
let context = CopyProgressContext::with_cancellation(Arc::clone(cancelled));
54-
copy_single_file_native(source, dest, false, Some(&context))
129+
log::debug!(
130+
"copy_file_with_strategy: different volumes or non-APFS, using chunked copy (src={}, dest={})",
131+
source.display(),
132+
dest.display()
133+
);
134+
chunked_copy_with_metadata(source, dest, cancelled, progress_callback)
55135
}
56136
}
57137

@@ -63,9 +143,10 @@ pub(super) fn copy_file_with_strategy(
63143
cancelled: &Arc<AtomicBool>,
64144
progress_callback: Option<ChunkedCopyProgressFn>,
65145
) -> Result<u64, WriteOperationError> {
66-
if is_network_filesystem(dest) {
146+
if is_network_filesystem(source) || is_network_filesystem(dest) {
67147
log::debug!(
68-
"copy_file_with_strategy: using chunked copy for network destination {}",
148+
"copy_file_with_strategy: using chunked copy for network path (src={}, dest={})",
149+
source.display(),
69150
dest.display()
70151
);
71152
chunked_copy_with_metadata(source, dest, cancelled, progress_callback)
@@ -84,14 +165,8 @@ pub(super) fn copy_file_with_strategy(
84165
cancelled: &Arc<AtomicBool>,
85166
progress_callback: Option<ChunkedCopyProgressFn>,
86167
) -> Result<u64, WriteOperationError> {
87-
let _ = progress_callback; // Unused on this platform
88-
if is_network_filesystem(dest) {
89-
log::debug!(
90-
"copy_file_with_strategy: using chunked copy for network destination {}",
91-
dest.display()
92-
);
93-
chunked_copy_with_metadata(source, dest, cancelled, None)
94-
} else if needs_safe_overwrite {
168+
let _ = (cancelled, progress_callback); // Unused on this platform
169+
if needs_safe_overwrite {
95170
safe_overwrite_file(source, dest)
96171
} else {
97172
fs::copy(source, dest).with_path(source)

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

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,8 @@ type CopyfileCallback = extern "C" fn(
8989
pub struct CopyProgressContext {
9090
/// Cancellation flag - checked during copy
9191
pub cancelled: Arc<AtomicBool>,
92+
/// Whether we've already logged the cancellation (avoids log spam from repeated callbacks)
93+
cancel_logged: AtomicBool,
9294
/// Callback to report progress (bytes_copied for current file)
9395
pub on_progress: Option<ProgressCallback>,
9496
/// Callback when starting a new file
@@ -101,6 +103,7 @@ impl Default for CopyProgressContext {
101103
fn default() -> Self {
102104
Self {
103105
cancelled: Arc::new(AtomicBool::new(false)),
106+
cancel_logged: AtomicBool::new(false),
104107
on_progress: None,
105108
on_file_start: None,
106109
on_file_finish: None,
@@ -113,6 +116,7 @@ impl CopyProgressContext {
113116
pub fn with_cancellation(cancelled: Arc<AtomicBool>) -> Self {
114117
Self {
115118
cancelled,
119+
cancel_logged: AtomicBool::new(false),
116120
on_progress: None,
117121
on_file_start: None,
118122
on_file_finish: None,
@@ -144,9 +148,12 @@ extern "C" fn copy_progress_callback(
144148
// SAFETY: ctx is a pointer to CopyProgressContext that we passed in
145149
let context = unsafe { &*(ctx as *const CopyProgressContext) };
146150

147-
// Check cancellation
151+
// Check cancellation (log only once to avoid spam — macOS may call this hundreds of times
152+
// after COPYFILE_QUIT while draining buffers)
148153
if context.cancelled.load(Ordering::Relaxed) {
149-
log::info!("copyfile callback: cancellation detected, returning COPYFILE_QUIT");
154+
if !context.cancel_logged.swap(true, Ordering::Relaxed) {
155+
log::info!("copyfile callback: cancellation detected, returning COPYFILE_QUIT");
156+
}
150157
return COPYFILE_QUIT;
151158
}
152159

@@ -332,18 +339,21 @@ pub fn copy_file_native(
332339
copyfile_state_free(state);
333340
}
334341

342+
// Check cancellation regardless of result — macOS copyfile(3) can return 0 (success)
343+
// even after the callback returned COPYFILE_QUIT, because it may continue draining
344+
// buffered I/O (especially common when the source is on a network mount).
345+
if let Some(ctx) = context
346+
&& ctx.cancelled.load(Ordering::Relaxed)
347+
{
348+
let _ = std::fs::remove_file(destination);
349+
return Err(WriteOperationError::Cancelled {
350+
message: "Operation cancelled by user".to_string(),
351+
});
352+
}
353+
335354
if result == 0 {
336355
Ok(())
337356
} else {
338-
// Check if cancelled
339-
if let Some(ctx) = context
340-
&& ctx.cancelled.load(Ordering::Relaxed)
341-
{
342-
return Err(WriteOperationError::Cancelled {
343-
message: "Operation cancelled by user".to_string(),
344-
});
345-
}
346-
347357
// Get the actual error
348358
let err = std::io::Error::last_os_error();
349359
Err(map_io_error_with_path(err, source, destination))

0 commit comments

Comments
 (0)