Skip to content

Commit 0de4c6b

Browse files
committed
refresh_listing: short-circuit on watcher-backed (M1)
- `refresh_listing` now checks the listing's volume `listing_is_watched(path)` and returns `TimedOut { timed_out: false }` immediately when the volume is keeping the cache fresh via `notify_mutation`. Eliminates the redundant post-transfer full re-read that wedged MTP (17 s + USB collision after every cancel/complete/error). - Logs at debug under `target: "refresh_listing"` with `listing_id`, `volume_id`, and `path` when the short-circuit fires. - New `caching::get_listing_volume_id_and_path` helper reads both fields in one lock acquisition. - Tests: short-circuit on watched, fall-through on unwatched, fall-through on missing listing, fall-through on unregistered volume. - Docs: extended `refresh_listing` rustdoc, updated `commands/CLAUDE.md` file map, added `refresh_listing` as the third consumer of `listing_is_watched` in `volume/CLAUDE.md`. - Watcher-driven `handle_directory_change` callers (FSEvents debouncer, incremental fallback, full re-read fallback) are intentionally NOT gated — they're how the cache stays in sync.
1 parent eb36037 commit 0de4c6b

6 files changed

Lines changed: 312 additions & 28 deletions

File tree

apps/desktop/src-tauri/src/commands/CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ immediately to business-logic modules. No significant logic lives here.
99
|------|--------|-------|
1010
| `mod.rs` | Re-exports | `mtp`, `network` gated behind `#[cfg(any(target_os = "macos", target_os = "linux"))]`; `volumes` behind `#[cfg(target_os = "macos")]`; `volumes_linux` behind `#[cfg(target_os = "linux")]` |
1111
| `util.rs` | Shared helpers | `TimedOut<T>`, `IpcError`, `blocking_with_timeout`, `blocking_with_timeout_flag`, `blocking_result_with_timeout`. See "Timeout-aware return types" below. |
12-
| `file_system/` | File listing & writes | Directory module split by operation type. `mod.rs` has `expand_tilde()`, re-exports, and tests. `listing.rs`: streaming + virtual-scroll listing API, path queries, `find_first_fuzzy_match` (type-to-jump), benchmarking, `get_brief_column_text_widths` (per-column widest-filename text widths for Brief mode; replaces the removed `get_max_filename_width`). `write_ops.rs`: create, copy, move, delete, trash, scan preview, conflict resolution, synthetic diff helpers. `volume_copy.rs`: cross-volume copy/move/scan, `SourceItemInput`. `drag.rs`: native drag, self-drag overlay. `e2e_support.rs`: feature-gated E2E/debug commands. |
12+
| `file_system/` | File listing & writes | Directory module split by operation type. `mod.rs` has `expand_tilde()`, re-exports, and tests. `listing.rs`: streaming + virtual-scroll listing API, path queries, `find_first_fuzzy_match` (type-to-jump), benchmarking, `get_brief_column_text_widths` (per-column widest-filename text widths for Brief mode; replaces the removed `get_max_filename_width`). `refresh_listing` short-circuits on watcher-backed listings (`Volume::listing_is_watched(path) == true`): the cache is already kept fresh by `notify_mutation`, so a redundant full re-read after every transfer outcome (the FE's `refreshPanesAfterTransfer`) used to wedge slow volumes (MTP 17 s + USB session collision). Logs at debug `target: "refresh_listing"` when the short-circuit fires. `write_ops.rs`: create, copy, move, delete, trash, scan preview, conflict resolution, synthetic diff helpers. `volume_copy.rs`: cross-volume copy/move/scan, `SourceItemInput`. `drag.rs`: native drag, self-drag overlay. `e2e_support.rs`: feature-gated E2E/debug commands. |
1313
| `volumes.rs` | Volume management (macOS) | `list_volumes`, `get_default_volume_id`, `get_volume_space`, `resolve_path_volume` (statfs-based, no volume enumeration) |
1414
| `volumes_linux.rs` | Volume management (Linux) | Same interface as `volumes.rs`, delegates to `volumes_linux` module |
1515
| `mtp.rs` | MTP devices | Full MTP command surface (connect, disconnect, list, download, upload, delete, rename, move, scan) |

apps/desktop/src-tauri/src/commands/file_system/listing.rs

Lines changed: 238 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,9 +322,42 @@ pub fn list_directory_end(listing_id: String) {
322322

323323
/// Force a re-read of a watched directory listing, emitting any diff.
324324
/// Used after write operations (move) when the file watcher may not fire promptly.
325+
///
326+
/// Short-circuits when the listing's volume reports `listing_is_watched(path) == true`.
327+
/// In that case the cache is already being kept fresh by the volume's `notify_mutation`
328+
/// pipeline (per-file `Added` / `Removed` / `Modified` events patched into `LISTING_CACHE`
329+
/// after every successful mutation), so a full `list_directory` re-read is redundant
330+
/// and costs a lot on slow backends: a 1k-entry MTP folder takes ~17 s and holds the
331+
/// USB session, colliding with the user's next op. Returns `TimedOut { data: (),
332+
/// timed_out: false }` immediately when the short-circuit fires, matching the
333+
/// `timed_out: false` shape the FE already handles on the fast-path.
334+
///
335+
/// Note: only this user-triggered command is gated. The FSEvents/SMB/MTP watcher
336+
/// callbacks call `handle_directory_change` directly and are intentionally left
337+
/// alone — they're how the cache stays in sync in the first place.
325338
#[tauri::command]
326339
#[specta::specta]
327340
pub async fn refresh_listing(listing_id: String) -> TimedOut<()> {
341+
// Short-circuit on watcher-backed listings: the volume's `notify_mutation`
342+
// pipeline keeps `LISTING_CACHE` fresh, so a full re-read here is pure
343+
// redundancy. See the doc comment above for why this matters on MTP/SMB.
344+
if let Some((volume_id, path)) = crate::file_system::listing::get_listing_volume_id_and_path(&listing_id)
345+
&& let Some(volume) = get_volume_manager().get(&volume_id)
346+
&& volume.listing_is_watched(&path)
347+
{
348+
log::debug!(
349+
target: "refresh_listing",
350+
"refresh_listing: short-circuit, watcher-backed listing (listing_id={}, volume_id={}, path={})",
351+
listing_id,
352+
volume_id,
353+
path.display(),
354+
);
355+
return TimedOut {
356+
data: (),
357+
timed_out: false,
358+
};
359+
}
360+
328361
let timed_out = tokio::time::timeout(Duration::from_secs(2), async {
329362
crate::file_system::watcher::handle_directory_change(&listing_id).await;
330363
})
@@ -368,3 +401,208 @@ pub fn benchmark_log(message: String) {
368401
eprintln!("{}", message);
369402
}
370403
}
404+
405+
#[cfg(test)]
406+
mod refresh_listing_tests {
407+
//! Tests for the `refresh_listing` short-circuit on watcher-backed listings (M1
408+
//! of the cancel-settled plan). Pattern adapted from
409+
//! `write_operations::delete_volume_reuse_tests` — a counter-wrapping
410+
//! `InMemoryVolume` whose `listing_is_watched` is flipped per test, seeded into
411+
//! `LISTING_CACHE` and `VolumeManager`, then we call `refresh_listing` and
412+
//! assert `list_directory` was or wasn't invoked.
413+
use super::*;
414+
use crate::file_system::get_volume_manager;
415+
use crate::file_system::listing::caching::{CachedListing, LISTING_CACHE};
416+
use crate::file_system::listing::metadata::FileEntry;
417+
use crate::file_system::listing::sorting::{DirectorySortMode, SortColumn, SortOrder};
418+
use crate::file_system::volume::{InMemoryVolume, Volume, VolumeError};
419+
use std::future::Future;
420+
use std::path::Path;
421+
use std::pin::Pin;
422+
use std::sync::Arc;
423+
use std::sync::atomic::{AtomicBool, AtomicU64, AtomicUsize, Ordering};
424+
425+
/// Wraps an `InMemoryVolume` and counts `list_directory` calls. `watched` is
426+
/// flipped per test to pin both short-circuit and fall-through behaviour.
427+
struct CountingVolume {
428+
inner: InMemoryVolume,
429+
watched: AtomicBool,
430+
list_dir_calls: AtomicUsize,
431+
}
432+
433+
impl CountingVolume {
434+
fn new(name: &str, watched: bool) -> Self {
435+
Self {
436+
inner: InMemoryVolume::new(name),
437+
watched: AtomicBool::new(watched),
438+
list_dir_calls: AtomicUsize::new(0),
439+
}
440+
}
441+
442+
fn list_dir_count(&self) -> usize {
443+
self.list_dir_calls.load(Ordering::Relaxed)
444+
}
445+
}
446+
447+
impl Volume for CountingVolume {
448+
fn name(&self) -> &str {
449+
self.inner.name()
450+
}
451+
fn root(&self) -> &Path {
452+
self.inner.root()
453+
}
454+
455+
fn list_directory<'a>(
456+
&'a self,
457+
path: &'a Path,
458+
on_progress: Option<&'a (dyn Fn(usize) + Sync)>,
459+
) -> Pin<Box<dyn Future<Output = Result<Vec<FileEntry>, VolumeError>> + Send + 'a>> {
460+
self.list_dir_calls.fetch_add(1, Ordering::Relaxed);
461+
self.inner.list_directory(path, on_progress)
462+
}
463+
464+
fn get_metadata<'a>(
465+
&'a self,
466+
path: &'a Path,
467+
) -> Pin<Box<dyn Future<Output = Result<FileEntry, VolumeError>> + Send + 'a>> {
468+
self.inner.get_metadata(path)
469+
}
470+
471+
fn exists<'a>(&'a self, path: &'a Path) -> Pin<Box<dyn Future<Output = bool> + Send + 'a>> {
472+
self.inner.exists(path)
473+
}
474+
475+
fn is_directory<'a>(
476+
&'a self,
477+
path: &'a Path,
478+
) -> Pin<Box<dyn Future<Output = Result<bool, VolumeError>> + Send + 'a>> {
479+
self.inner.is_directory(path)
480+
}
481+
482+
fn listing_is_watched(&self, _path: &Path) -> bool {
483+
self.watched.load(Ordering::Relaxed)
484+
}
485+
}
486+
487+
fn unique(suffix: &str) -> String {
488+
static N: AtomicU64 = AtomicU64::new(0);
489+
format!(
490+
"refresh_listing_{}_{}_{}",
491+
suffix,
492+
std::process::id(),
493+
N.fetch_add(1, Ordering::Relaxed)
494+
)
495+
}
496+
497+
fn insert_listing(listing_id: &str, volume_id: &str, path: &str) {
498+
let mut cache = LISTING_CACHE.write().unwrap();
499+
cache.insert(
500+
listing_id.to_string(),
501+
CachedListing {
502+
volume_id: volume_id.to_string(),
503+
path: PathBuf::from(path),
504+
entries: Vec::new(),
505+
sort_by: SortColumn::Name,
506+
sort_order: SortOrder::Ascending,
507+
directory_sort_mode: DirectorySortMode::LikeFiles,
508+
sequence: AtomicU64::new(1),
509+
created_at: std::time::Instant::now(),
510+
},
511+
);
512+
}
513+
514+
fn remove_listing(listing_id: &str) {
515+
let _ = LISTING_CACHE.write().unwrap().remove(listing_id);
516+
}
517+
518+
/// Watched volume: short-circuit fires, `list_directory` never called.
519+
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
520+
async fn refresh_listing_short_circuits_on_watched_volume() {
521+
let vid = unique("short_circuit_vid");
522+
let lid = unique("short_circuit_lid");
523+
let path = "/dcim";
524+
525+
let vol = Arc::new(CountingVolume::new("watched-vol", true));
526+
get_volume_manager().register(&vid, vol.clone() as Arc<dyn Volume>);
527+
insert_listing(&lid, &vid, path);
528+
529+
let result = refresh_listing(lid.clone()).await;
530+
531+
assert!(!result.timed_out, "short-circuit returns timed_out=false");
532+
assert_eq!(
533+
vol.list_dir_count(),
534+
0,
535+
"watched-backed refresh_listing must skip list_directory",
536+
);
537+
538+
remove_listing(&lid);
539+
get_volume_manager().unregister(&vid);
540+
}
541+
542+
/// Unwatched volume: fall-through path runs (`handle_directory_change` calls
543+
/// `list_directory`). The InMemoryVolume's directory exists so we get a real
544+
/// listing rather than NotFound.
545+
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
546+
async fn refresh_listing_falls_through_on_unwatched() {
547+
let vid = unique("fallthrough_vid");
548+
let lid = unique("fallthrough_lid");
549+
let path = "/dcim";
550+
551+
let vol = Arc::new(CountingVolume::new("unwatched-vol", false));
552+
// Populate one file so `list_directory` succeeds.
553+
vol.inner.create_file(Path::new("/dcim/a.jpg"), b"alpha").await.unwrap();
554+
get_volume_manager().register(&vid, vol.clone() as Arc<dyn Volume>);
555+
insert_listing(&lid, &vid, path);
556+
557+
let result = refresh_listing(lid.clone()).await;
558+
559+
assert!(!result.timed_out, "fast InMemory list_directory shouldn't time out");
560+
assert!(
561+
vol.list_dir_count() >= 1,
562+
"unwatched volume must fall through to list_directory (count was {})",
563+
vol.list_dir_count(),
564+
);
565+
566+
remove_listing(&lid);
567+
get_volume_manager().unregister(&vid);
568+
}
569+
570+
/// No cache entry for the listing_id: today's behaviour is a clean no-op
571+
/// (`handle_directory_change` early-returns). The short-circuit must NOT
572+
/// suppress that path or panic; we just assert the call completes cleanly.
573+
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
574+
async fn refresh_listing_falls_through_on_missing_listing() {
575+
let lid = unique("missing_lid");
576+
// No insert_listing call; no register call.
577+
let result = refresh_listing(lid).await;
578+
assert!(
579+
!result.timed_out,
580+
"missing listing should resolve quickly without timeout"
581+
);
582+
}
583+
584+
/// Cache has the listing but the volume isn't registered: short-circuit
585+
/// can't ask `listing_is_watched`, so we fall through to today's behaviour
586+
/// (`handle_directory_change` finds no volume, falls back to local std::fs
587+
/// for the path which doesn't exist, and returns cleanly without panic).
588+
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
589+
async fn refresh_listing_falls_through_when_volume_not_registered() {
590+
let vid = unique("unregistered_vid");
591+
let lid = unique("unregistered_lid");
592+
// Use a path that doesn't exist on disk so the std::fs fallback returns
593+
// NotFound and the function exits cleanly.
594+
let path = "/tmp/cmdr-refresh-listing-test-nonexistent-path-xyz123";
595+
596+
// Note: NO get_volume_manager().register() call.
597+
insert_listing(&lid, &vid, path);
598+
599+
let result = refresh_listing(lid.clone()).await;
600+
601+
assert!(
602+
!result.timed_out,
603+
"unregistered-volume fallthrough should resolve quickly"
604+
);
605+
606+
remove_listing(&lid);
607+
}
608+
}

apps/desktop/src-tauri/src/file_system/listing/caching.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,17 @@ pub fn get_listing_path(listing_id: &str) -> Option<PathBuf> {
188188
cache.get(listing_id).map(|listing| listing.path.clone())
189189
}
190190

191+
/// Returns `(volume_id, path)` for a cached listing in one read-lock acquisition.
192+
///
193+
/// Used by `refresh_listing` so the short-circuit check can ask the volume
194+
/// `listing_is_watched(path)` without two separate cache reads.
195+
pub fn get_listing_volume_id_and_path(listing_id: &str) -> Option<(String, PathBuf)> {
196+
let cache = LISTING_CACHE.read().ok()?;
197+
cache
198+
.get(listing_id)
199+
.map(|listing| (listing.volume_id.clone(), listing.path.clone()))
200+
}
201+
191202
/// Removes an entry by its path from the cached listing.
192203
///
193204
/// Returns `(old_index, removed_entry)` or `None` if the listing or entry wasn't found.

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,8 @@ pub use operations::{get_files_at_indices, get_paths_at_indices};
2929

3030
// Internal re-exports for file_system module internals (pub(crate) for crate-internal use)
3131
pub(crate) use caching::{
32-
ModifyResult, find_listings_for_path, get_listing_path, has_entry, increment_sequence, insert_entry_sorted,
33-
remove_entry_by_path, update_entry_sorted,
32+
ModifyResult, find_listings_for_path, get_listing_path, get_listing_volume_id_and_path, has_entry,
33+
increment_sequence, insert_entry_sorted, remove_entry_by_path, update_entry_sorted,
3434
};
3535
// Notification API for volume mutations
3636
#[cfg(any(target_os = "macos", target_os = "linux"))]

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,11 @@ Optional methods default to `Err(VolumeError::NotSupported)` or `false`, so new
4949
- `on_unmount()`: lifecycle hook called before unregistration. `SmbVolume` uses it to disconnect its smb2 session. Default is no-op.
5050
- `scanner()` / `watcher()`: drive indexing hooks; `None` by default.
5151
- `space_poll_interval()`: recommended interval for the live disk-space poller (`space_poller.rs`). Default 2 s (local volumes). `SmbVolume` and `MtpVolume` override to 5 s. `InMemoryVolume` returns `None` (no polling). The poller uses this to tick each volume at its own cadence.
52-
- `listing_is_watched(path)`: returns `true` when this volume's cached listing for `path` is being kept in sync by a live watcher. Used by `file_system::listing::caching::try_get_watched_listing` (the "fresh-listing oracle") to decide whether write-op pre-flight scans can reuse a cached listing instead of re-reading. Default `false` so a new backend without a real watcher won't accidentally claim freshness. **Freshness contract**: a `true` result does NOT mean the cache is byte-perfect with the device right now. Every backend has a debounce or settling window between a real change and the cache reflecting it: local FS ≈ 10 ms (FSEvents coalesce), SMB 200 ms (watcher debounce; > 50 events/dir triggers a `FullRefresh`), MTP 500 ms (event debouncer plus per-device polling; many cameras emit no events at all, so on those `true` means only "the device is reachable"). Callers must treat the result as "fresh as our most recent observation" — the same guarantee a `list_directory` call gives. The MTP and SMB checks are volume-level, not path-level: when the gate flips true, every path on that volume becomes oracle-eligible.
52+
- `listing_is_watched(path)`: returns `true` when this volume's cached listing for `path` is being kept in sync by a live watcher. Three consumers today:
53+
1. `file_system::listing::caching::try_get_watched_listing` (the "fresh-listing oracle") — write-op pre-flight scans reuse a cached listing instead of re-reading.
54+
2. `write_operations::delete::scan_volume_recursive` (the oracle-aware delete walker) — same idea, per-recursion-level.
55+
3. The `refresh_listing` Tauri command (`commands/file_system/listing.rs`) — short-circuits the post-transfer redundant `list_directory` re-read entirely when the volume is keeping the cache fresh via `notify_mutation`. Without this, a 1k-entry MTP folder paid ~17 s + USB session collision after every transfer outcome, wedging the next user op.
56+
Default `false` so a new backend without a real watcher won't accidentally claim freshness. **Freshness contract**: a `true` result does NOT mean the cache is byte-perfect with the device right now. Every backend has a debounce or settling window between a real change and the cache reflecting it: local FS ≈ 10 ms (FSEvents coalesce), SMB 200 ms (watcher debounce; > 50 events/dir triggers a `FullRefresh`), MTP 500 ms (event debouncer plus per-device polling; many cameras emit no events at all, so on those `true` means only "the device is reachable"). Callers must treat the result as "fresh as our most recent observation" — the same guarantee a `list_directory` call gives. The MTP and SMB checks are volume-level, not path-level: when the gate flips true, every path on that volume becomes oracle-eligible.
5357

5458
## Building a new volume
5559

0 commit comments

Comments
 (0)