Fix FS-related issues that were causing a test failure on linux (#13072)

Max Brunsfeld and Mikayla created 2 years ago

This fixes `project_tests::rescan_and_remote_updates` .

That test was actually correctly failing, revealing two bugs on Linux.

Release Notes:

- Fixed an issue where file renames were not detected on Linux.
- Fixed performance problems caused by excessive file system events on
Linux.

---------

Co-authored-by: Mikayla <[e-mail]>

Change summary

crates/fs/src/fs.rs                 | 34 ++++++++++++++++++++----------
crates/project/src/project_tests.rs |  2 -
crates/worktree/src/worktree.rs     | 25 +++++++++++++---------
3 files changed, 37 insertions(+), 24 deletions(-)

Detailed changes

crates/fs/src/fs.rs

@@ -135,8 +135,6 @@ pub struct RealFs {
 }
 
 pub struct RealWatcher {
-    #[cfg(target_os = "linux")]
-    root_path: PathBuf,
     #[cfg(target_os = "linux")]
     fs_watcher: parking_lot::Mutex<notify::INotifyWatcher>,
 }
@@ -452,25 +450,38 @@ impl Fs for RealFs {
     async fn watch(
         &self,
         path: &Path,
-        _latency: Duration,
+        latency: Duration,
     ) -> (
         Pin<Box<dyn Send + Stream<Item = Vec<PathBuf>>>>,
         Arc<dyn Watcher>,
     ) {
+        use parking_lot::Mutex;
+
         let (tx, rx) = smol::channel::unbounded();
+        let pending_paths: Arc<Mutex<Vec<PathBuf>>> = Default::default();
+        let root_path = path.to_path_buf();
 
         let file_watcher = notify::recommended_watcher({
             let tx = tx.clone();
+            let pending_paths = pending_paths.clone();
             move |event: Result<notify::Event, _>| {
                 if let Some(event) = event.log_err() {
-                    tx.try_send(event.paths).ok();
+                    let mut paths = event.paths;
+                    paths.retain(|path| path.starts_with(&root_path));
+                    if !paths.is_empty() {
+                        paths.sort();
+                        let mut pending_paths = pending_paths.lock();
+                        if pending_paths.is_empty() {
+                            tx.try_send(()).ok();
+                        }
+                        util::extend_sorted(&mut *pending_paths, paths, usize::MAX, PathBuf::cmp);
+                    }
                 }
             }
         })
         .expect("Could not start file watcher");
 
         let watcher = Arc::new(RealWatcher {
-            root_path: path.to_path_buf(),
             fs_watcher: parking_lot::Mutex::new(file_watcher),
         });
 
@@ -484,14 +495,13 @@ impl Fs for RealFs {
         (
             Box::pin(rx.filter_map({
                 let watcher = watcher.clone();
-                move |mut paths| {
-                    paths.retain(|path| path.starts_with(&watcher.root_path));
+                move |_| {
+                    let _ = watcher.clone();
+                    let pending_paths = pending_paths.clone();
                     async move {
-                        if paths.is_empty() {
-                            None
-                        } else {
-                            Some(paths)
-                        }
+                        smol::Timer::after(latency).await;
+                        let paths = std::mem::take(&mut *pending_paths.lock());
+                        (!paths.is_empty()).then_some(paths)
                     }
                 }
             })),

crates/project/src/project_tests.rs

@@ -2977,8 +2977,6 @@ async fn test_save_as(cx: &mut gpui::TestAppContext) {
     assert_eq!(opened_buffer, buffer);
 }
 
-// This test is currently disabled on Linux as it fails fails pretty consistently on that target.
-#[cfg(not(target_os = "linux"))]
 #[gpui::test(retries = 5)]
 async fn test_rescan_and_remote_updates(cx: &mut gpui::TestAppContext) {
     use worktree::WorktreeModelHandle as _;

crates/worktree/src/worktree.rs

@@ -303,7 +303,7 @@ struct BackgroundScannerState {
     /// as part of the current update. These entry ids may be re-used
     /// if the same inode is discovered at a new path, or if the given
     /// path is re-created after being deleted.
-    removed_entry_ids: HashMap<u64, ProjectEntryId>,
+    removed_entry_ids: HashMap<(u64, SystemTime), ProjectEntryId>,
     changed_paths: Vec<Arc<Path>>,
     prev_snapshot: Snapshot,
 }
@@ -2638,10 +2638,13 @@ impl BackgroundScannerState {
     }
 
     fn reuse_entry_id(&mut self, entry: &mut Entry) {
-        if let Some(removed_entry_id) = self.removed_entry_ids.remove(&entry.inode) {
-            entry.id = removed_entry_id;
-        } else if let Some(existing_entry) = self.snapshot.entry_for_path(&entry.path) {
-            entry.id = existing_entry.id;
+        if let Some(mtime) = entry.mtime {
+            if let Some(removed_entry_id) = self.removed_entry_ids.remove(&(entry.inode, mtime)) {
+                eprintln!("detected that entry {entry:?} was renamed from inode and mtime reusing id {removed_entry_id:?}");
+                entry.id = removed_entry_id;
+            } else if let Some(existing_entry) = self.snapshot.entry_for_path(&entry.path) {
+                entry.id = existing_entry.id;
+            }
         }
     }
 
@@ -2732,11 +2735,13 @@ impl BackgroundScannerState {
 
         let mut entries_by_id_edits = Vec::new();
         for entry in removed_entries.cursor::<()>() {
-            let removed_entry_id = self
-                .removed_entry_ids
-                .entry(entry.inode)
-                .or_insert(entry.id);
-            *removed_entry_id = cmp::max(*removed_entry_id, entry.id);
+            if let Some(mtime) = entry.mtime {
+                let removed_entry_id = self
+                    .removed_entry_ids
+                    .entry((entry.inode, mtime))
+                    .or_insert(entry.id);
+                *removed_entry_id = cmp::max(*removed_entry_id, entry.id);
+            }
             entries_by_id_edits.push(Edit::Remove(entry.id));
         }
         self.snapshot.entries_by_id.edit(entries_by_id_edits, &());