Skip to content

Commit f002606

Browse files
committed
Read MTP files in bounded windows so the device session is free between reads
Reworks the MTP read path from one held-open `GetObject` (which holds the single PTP session for the whole file) into a sequence of bounded ~8 MiB `download_partial_64` transactions, each acquiring + releasing the per-device lock. Between windows nothing is in flight and the session is free — the foundation for letting a foreground listing interleave during a transfer (the yield wiring comes next). - `MtpReadSession` (`mtp/connection/file_ops.rs`): `open_read_session` resolves the handle + `Storage` + total size ONCE; `read_window` does only `acquire_device_lock` + `download_partial_64(handle, offset, len)` per window. Neither takes a `foreground_guard` — a transfer is a background user of the device gate, so raising `foreground_pending` would make it yield to itself. - `MtpReadStream` (`volume/backends/mtp.rs`) caches the session and reads bounded windows: `next_chunk` clamps each window to the bytes remaining (`mtp_window_len`), advances the offset by the bytes actually returned, ends cleanly at EOF, and treats a 0-byte read before EOF as a transient error (not a frozen-progress spin). `cancel_and_release` is now a near-noop (nothing held between windows; a dropped mid-window read self-heals via mtp-rs `TransactionScope`). - Reworks the SHARED `MtpReadStream`, so native drag-out gets bounded windows for free; the single-file `download_mtp_file` command is routed through the same `read_window` and loses its whole-file `foreground_guard`. Retires `open_download_stream_at_offset` + the `FileDownload`-wrapping internals. - Window size is the named `MTP_READ_WINDOW` (8 MiB, the throughput-vs-yield-latency knob; tuned on real hardware later). - Proactively fixed the pre-existing `virtual-mtp` tests' device-id derivation (built `mtp-{location_id}`, which never matches the serial-derived id, so they couldn't connect in isolation). Real red→green: byte-exact multi-window assembly, EOF short-final-window, empty-file/offset==total → None, zero-before-EOF → error, short-mid-file advance, cancel keeps-partials. `pnpm check rust` green.
1 parent 7a1e57c commit f002606

9 files changed

Lines changed: 715 additions & 232 deletions

File tree

apps/desktop/src-tauri/src/file_system/volume/DETAILS.md

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -182,19 +182,20 @@ Reads and writes have different shapes because the consumer relationship is diff
182182

183183
The rest of this section is about **read-side** lifetime handling. Which pattern to pick depends on whether your protocol SDK's download handle is `'static` or borrowed.
184184

185-
### Pattern A: own the download (use when the SDK's download type is `'static`)
185+
### Pattern A: cached session + bounded windows (use when the SDK exposes a stateless partial-read primitive)
186186

187-
If the SDK gives you a download handle that owns its session internally and doesn't borrow from anything, store it directly in your stream struct. **Example: `MtpReadStream`** (`backends/mtp.rs`).
187+
If the SDK can read an arbitrary byte range on demand (no held streaming handle), cache the resolved session in your stream struct and issue one bounded read per `next_chunk`, advancing an offset. Nothing is held between reads, so there's no lifetime gymnastics, no task, no channel, and no `Drop` to write. **Example: `MtpReadStream`** (`backends/mtp.rs`), which loops `GetPartialObject64`.
188188

189189
```rust
190190
struct MtpReadStream {
191-
download: Option<mtp_rs::FileDownload>, // 'static, no lifetime parameter
191+
reader: Box<dyn WindowReader>, // caches the MtpReadSession (Storage + handle)
192192
total_size: u64,
193-
bytes_read: u64,
193+
offset: u64, // absolute position of the next window
194+
window: u32, // bytes per window (MTP_READ_WINDOW)
194195
}
195196
```
196197

197-
`next_chunk()` calls `download.as_mut()?.next_chunk().await` directly, no task spawn, no channel. `Drop` cancels the transfer (see the MtpReadStream Drop gotcha in `backends/CLAUDE.md` for the detached-task cancel pattern).
198+
`next_chunk()` issues one `[offset, offset + len)` window via the cached session, advancing `offset` by the bytes actually returned. `cancel_and_release` is the trait default no-op (nothing held); a mid-window drop self-heals via mtp-rs's `TransactionScope`. The windowing/offset rules live in `mtp/connection` (DETAILS § "Bounded-window reads").
198199

199200
### Pattern B: channel-backed stream (use when the SDK's download type borrows `&mut Connection`)
200201

apps/desktop/src-tauri/src/file_system/volume/backends/CLAUDE.md

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ The trait shape, capability matrix, streaming patterns, and "Building a new volu
88

99
- `local_posix.rs`: `LocalPosixVolume`, real filesystem; delegates listing/indexing/watching to `file_system::listing`
1010
and `indexing`, copy scanning via `walkdir`, space info via `libc::statvfs` FFI.
11-
- `mtp.rs`: `MtpVolume`, MTP device storage; direct async MTP calls, `MtpReadStream` for streaming. Gated
12-
`#[cfg(any(target_os = "macos", target_os = "linux"))]`.
11+
- `mtp.rs`: `MtpVolume`, MTP device storage; direct async MTP calls, `MtpReadStream` (bounded-window reads). macOS/Linux
12+
only.
1313
- `smb.rs`: `SmbVolume`, direct async smb2. Split session storage, `AtomicU8` connection state, cached
1414
`SmbConnectionParams` for reconnect, global `AppHandle` for `smb-connection-changed` events. Same cfg gate.
1515
- `smb_watcher.rs`: background SMB change watcher on a dedicated smb2 session (separate TCP connection).
@@ -33,10 +33,11 @@ The trait shape, capability matrix, streaming patterns, and "Building a new volu
3333
- **`LocalPosixVolume::write_from_stream` `sync_data`s each file (+ best-effort parent-dir fsync) before returning.**
3434
Every cross-volume copy/move landing on local disk flows through this one method; a bare `flush()` leaves bytes only
3535
in the page cache, so an eject/sleep loses data (on a move, from both sides). Don't drop the fsync.
36-
- **`MtpVolume::get_metadata` lists the entire parent directory** (MTP has no single-file stat). Fine for infrequent
37-
`notify_mutation` use; avoid it in hot paths.
38-
- **`MtpReadStream::Drop` spawns a detached cancel task.** mtp-rs's `ReceiveStream` panics on drop if not consumed or
39-
cancelled (USB session corruption guard). Safe because the stream always lives in an async context.
36+
- **`MtpVolume::get_metadata` lists the entire parent directory** (MTP has no single-file stat). Avoid in hot paths.
37+
- **`MtpReadStream` reads in bounded windows, not one held-open stream.** Each `next_chunk` issues one
38+
`GetPartialObject64(offset, MTP_READ_WINDOW)`; the device lock is held per window, freeing the session between
39+
windows (foreground nav slips in). `cancel_and_release` is a no-op; a mid-window drop self-heals (mtp-rs
40+
`TransactionScope`). Offset/EOF rules: `mtp/connection/DETAILS.md` § "Bounded-window reads".
4041
- **SMB watcher filenames need normalizing**: backslashes to forward slashes, and NFC (from server) to NFD (macOS
4142
mount paths) before cache lookups. See [DETAILS.md](DETAILS.md) § "Gotchas".
4243
- **SMB auto-upgrade is gated on `network.directSmbConnection`** and is a no-op when no SMB mounts are present (so it

apps/desktop/src-tauri/src/file_system/volume/backends/DETAILS.md

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ modifying `SmbVolume`, `MtpVolume`, `LocalPosixVolume`, the SMB watcher, or `InM
88
## Key files
99

1010
- **`local_posix.rs`**: `LocalPosixVolume`: real filesystem; delegates listing to `file_system::listing`, indexing to `indexing::scanner`, watching to `indexing::watcher` (FSEvents), copy scanning via `walkdir`. Uses `libc::statvfs` FFI for space info.
11-
- **`mtp.rs`**: `MtpVolume`: MTP device storage; async `Volume` trait with direct async MTP calls. Uses `MtpReadStream` for streaming (calls `FileDownload::next_chunk().await` directly). Gated with `#[cfg(any(target_os = "macos", target_os = "linux"))]`.
11+
- **`mtp.rs`**: `MtpVolume`: MTP device storage; async `Volume` trait with direct async MTP calls. Uses `MtpReadStream`, which reads in bounded `GetPartialObject64` windows over a cached `MtpReadSession` (per-window `WindowReader`; the windowing/offset logic lives in `mtp/connection`). Gated with `#[cfg(any(target_os = "macos", target_os = "linux"))]`.
1212
- **`smb.rs`**: `SmbVolume`: SMB share storage; async `Volume` trait with direct async smb2 calls. Splits session storage into `Arc<Mutex<Option<SmbClient>>>` + `Arc<RwLock<Option<Arc<Tree>>>>` so the hot read/write paths can clone `Connection` under a brief lock and drive compound / download ops without serializing on the client mutex. `AtomicU8` connection state. Caches `SmbConnectionParams` (host, share, port, credentials) so `attempt_reconnect` can rebuild the session in place after a transient disconnect, single-flighted via `reconnect_lock`. Holds a global `AppHandle` (`set_app_handle` in `lib.rs::setup`) for emitting `smb-connection-changed` events (the typed `tauri_specta::Event` struct `SmbConnectionChanged` lives in the always-compiled `network/mod.rs`, not here, so `collect_events!` in `ipc.rs` can reference it on every platform; `emit_state_change` just builds and `.emit()`s it). Also contains `connect_smb_volume()`. Gated with `#[cfg(any(target_os = "macos", target_os = "linux"))]`.
1313
- **`smb_watcher.rs`**: Background SMB change watcher (`run_smb_watcher`). Owns a dedicated smb2 session (separate TCP connection from the volume's primary client) and uses smb2 0.10's `'static` `Watcher` with pipelined CHANGE_NOTIFY (one request kept pre-issued on the wire so events arriving during consumer processing don't fall in a re-arm gap). Debounces events, feeds `notify_directory_changed`. Spawned by `connect_smb_volume()` and respawned by `attempt_reconnect`. No internal reconnect — bails on `next_events` errors and lets `attempt_reconnect` handle session recovery.
1414
- **`in_memory.rs`**: `InMemoryVolume`: `RwLock<HashMap>` store for tests; also used for stress tests (`with_file_count`)
@@ -99,12 +99,8 @@ updated them.
9999

100100
## Gotchas
101101

102-
**Gotcha**: `MtpReadStream::Drop` spawns a detached cancel task
103-
**Why**: When a download is cancelled mid-stream (user presses Cancel during MTP copy), the `MtpReadStream` is dropped
104-
before the `FileDownload` is fully consumed. mtp-rs's `ReceiveStream` panics on drop if not consumed or cancelled
105-
(to prevent USB session corruption). The `Drop` impl calls `download.cancel(DEFAULT_CANCEL_TIMEOUT).await` on a
106-
spawned detached task. This is safe because the stream always lives in an async context (tokio worker thread), so
107-
`Handle::try_current()` succeeds. The detached task runs independently; the drop returns immediately.
102+
**Gotcha**: `MtpReadStream` holds nothing scarce between windows, so dropping it mid-read is safe and needs no `Drop` impl
103+
**Why**: It reads in bounded `GetPartialObject64(offset, MTP_READ_WINDOW)` windows (the windowing + offset accounting live in `mtp/connection`; see that module's DETAILS § "Bounded-window reads"). Between windows nothing is in flight — no held `FileDownload`, no pinned PTP session — so a cancel/pause/drop has nothing to abort or drain (`cancel_and_release` is the trait default no-op). If the stream is dropped WHILE a window read is in flight, mtp-rs's `TransactionScope` flags the pipe and the next op drains it under the operation lock (one ~300 ms self-heal), so an aborted window never desyncs the session. ❌ Don't re-add a `Drop`/cancel here: there's no held `FileDownload`, so mtp-rs's `ReceiveStream` unconsumed-drop panic (the reason a `Drop` cancel was once needed) can't apply.
108104

109105
**Gotcha**: `MtpVolume::get_metadata` is expensive: it lists the entire parent directory
110106
**Why**: MTP has no single-file stat call. `get_metadata` lists the parent directory and searches for the entry by name. This is used by `notify_mutation` after each self-mutation (create, delete, rename) and is acceptable because those are infrequent, but avoid calling it in hot paths.

0 commit comments

Comments
 (0)