Skip to content

Commit ab98ee8

Browse files
committed
Bugfix: upsert in notify_added so post-write size wins the watcher race
After the previous commit, the SMB watcher and `write_from_stream`'s `notify_mutation` both stat the file and call `notify_directory_changed(Added(entry))`. The watcher's stat fires mid-write and catches a partial size; `write_from_stream`'s fires post-close and gets the final size. `notify_added` used `insert_entry_sorted`, which silently early-returns on duplicate path — so whichever Added landed in the cache first stuck, and the second observation was dropped. Result on MTP→SMB: 3 of 9 files copied with the correct bytes on disk but a cached size frozen at the partial mid-write value, visible in the FE until the user navigated away and back. Confirmed in the log: msg_id 266-269 stat returned `size=2359284` at T+213 ms (file still streaming); msg_id 313+ stat at T+330 ms returned 4989168 (post-Close) but was silently dropped because the path already had an entry. Fix: `notify_added` now upserts. If `has_entry(listing_id, path)` is true, it routes to `notify_modified` (which updates the entry and emits the right diff type to the FE). Otherwise the existing insert path runs. The semantic — "this entry exists with this state at this point in time" — is the same; the cache now reflects the latest observation rather than the first. Pinned by `notify_added_upserts_when_entry_already_present` in `caching_test.rs`: two Added calls with the same path and different sizes — final cache must hold the second size. TDD-verified red before the upsert: assertion failed with exactly the partial-size left-over (`Some(2359284)` vs expected `Some(4989168)`). `notify_added` exposed as `pub(super)` so the test can drive it directly.
1 parent 1dea24e commit ab98ee8

2 files changed

Lines changed: 57 additions & 3 deletions

File tree

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

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -337,12 +337,28 @@ pub fn notify_directory_changed(volume_id: &str, parent_path: &Path, change: Dir
337337

338338
/// Inserts an entry into the cache and queues a single-add change for the next
339339
/// coalesced `directory-diff` flush.
340-
fn notify_added(listing_id: &str, entry: FileEntry) {
340+
///
341+
/// **Upsert semantics**: if a cached entry with the same path already exists,
342+
/// delegates to `notify_modified` so the cache reflects the latest observation
343+
/// instead of dropping it. This matters when the SMB / MTP watcher fires an
344+
/// Add event mid-write (the watcher's stat catches a partial file size), then
345+
/// `Volume::write_from_stream` fires its own Add post-close with the final
346+
/// size. Without upsert, the partial size from the watcher sticks and the FE
347+
/// shows a wrong size until the next manual refresh. Concretely seen on
348+
/// MTP→SMB copies: 9 files copied, 3 stuck at half size (watcher stat'd
349+
/// mid-write, self-notify lost the race against `insert_entry_sorted`'s
350+
/// duplicate guard).
351+
pub(super) fn notify_added(listing_id: &str, entry: FileEntry) {
341352
use crate::file_system::listing::diff_emitter::enqueue_diff;
342353
use crate::file_system::watcher::DiffChange;
343354

355+
if has_entry(listing_id, &entry.path) {
356+
notify_modified(listing_id, entry);
357+
return;
358+
}
359+
344360
let Some(index) = insert_entry_sorted(listing_id, entry.clone()) else {
345-
return; // Already exists or listing gone
361+
return; // Listing gone (or, harmless: lost a TOCTOU race against another add — Modified would no-op).
346362
};
347363

348364
enqueue_diff(

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

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use std::path::PathBuf;
44

55
use super::caching::{
66
CachedListing, LISTING_CACHE, ModifyResult, find_listings_for_path, find_listings_for_path_on_volume, has_entry,
7-
insert_entry_sorted, remove_entry_by_path, update_entry_sorted,
7+
insert_entry_sorted, notify_added, remove_entry_by_path, update_entry_sorted,
88
};
99
use super::metadata::FileEntry;
1010
use super::sorting::{DirectorySortMode, SortColumn, SortOrder};
@@ -228,6 +228,44 @@ fn test_insert_entry_sorted_returns_none_for_missing_listing() {
228228
assert_eq!(result, None);
229229
}
230230

231+
#[test]
232+
fn notify_added_upserts_when_entry_already_present() {
233+
// Race that motivated the upsert: SMB watcher fires an Added event mid-write
234+
// (stat catches the file at partial size), then `write_from_stream`'s own
235+
// post-close `notify_mutation` fires its Added with the final size. Without
236+
// upsert the first-write wins (Samba's mid-write partial size sticks) and
237+
// the FE shows the wrong size until the next manual refresh. With upsert
238+
// the second observation updates the cached entry to the final size.
239+
let id = insert_test_listing(
240+
"notify_added_upsert",
241+
"/test",
242+
SortColumn::Name,
243+
SortOrder::Ascending,
244+
DirectorySortMode::LikeFiles,
245+
vec![],
246+
);
247+
248+
// First observation: partial size (what the watcher would see mid-write).
249+
notify_added("notify_added_upsert", make_entry("photo.jpg", false, Some(2_359_284)));
250+
// Second observation: final size (what the post-close stat sees).
251+
notify_added("notify_added_upsert", make_entry("photo.jpg", false, Some(4_989_168)));
252+
253+
let cache = LISTING_CACHE.read().unwrap();
254+
let listing = cache.get("notify_added_upsert").unwrap();
255+
assert_eq!(
256+
listing.entries.len(),
257+
1,
258+
"should still be exactly one entry, not duplicated"
259+
);
260+
assert_eq!(
261+
listing.entries[0].size,
262+
Some(4_989_168),
263+
"second (final) size must overwrite the partial-size observation"
264+
);
265+
drop(cache);
266+
cleanup_listing(&id);
267+
}
268+
231269
// ============================================================================
232270
// remove_entry_by_path tests
233271
// ============================================================================

0 commit comments

Comments
 (0)