Make `RealFs::metadata` not error on recursive/looped symlinks (#45458)

Martin Pool created

Change summary

crates/fs/src/fs.rs | 83 ++++++++++++++++++++++++++++++++++++++--------
1 file changed, 68 insertions(+), 15 deletions(-)

Detailed changes

crates/fs/src/fs.rs 🔗

@@ -886,22 +886,25 @@ impl Fs for RealFs {
         let is_symlink = symlink_metadata.file_type().is_symlink();
         let metadata = if is_symlink {
             let path_buf = path.to_path_buf();
-            let path_exists = self
+            // Read target metadata, if the target exists
+            match self
                 .executor
-                .spawn(async move {
-                    path_buf
-                        .try_exists()
-                        .with_context(|| format!("checking existence for path {path_buf:?}"))
-                })
-                .await?;
-            if path_exists {
-                let path_buf = path.to_path_buf();
-                self.executor
-                    .spawn(async move { std::fs::metadata(path_buf) })
-                    .await
-                    .with_context(|| "accessing symlink for path {path}")?
-            } else {
-                symlink_metadata
+                .spawn(async move { std::fs::metadata(path_buf) })
+                .await
+            {
+                Ok(target_metadata) => target_metadata,
+                Err(err) => {
+                    if err.kind() != io::ErrorKind::NotFound {
+                        // TODO: Also FilesystemLoop when that's stable
+                        log::warn!(
+                            "Failed to read symlink target metadata for path {path:?}: {err}"
+                        );
+                    }
+                    // For a broken or recursive symlink, return the symlink metadata. (Or
+                    // as edge cases, a symlink into a directory we can't read, which is hard
+                    // to distinguish from just being broken.)
+                    symlink_metadata
+                }
             }
         } else {
             symlink_metadata
@@ -3496,4 +3499,54 @@ mod tests {
             ]
         );
     }
+
+    #[gpui::test]
+    #[cfg(unix)]
+    async fn test_realfs_broken_symlink_metadata(executor: BackgroundExecutor) {
+        let tempdir = TempDir::new().unwrap();
+        let path = tempdir.path();
+        let fs = RealFs {
+            bundled_git_binary_path: None,
+            executor,
+            next_job_id: Arc::new(AtomicUsize::new(0)),
+            job_event_subscribers: Arc::new(Mutex::new(Vec::new())),
+        };
+        let symlink_path = path.join("symlink");
+        smol::block_on(fs.create_symlink(&symlink_path, PathBuf::from("file_a.txt"))).unwrap();
+        let metadata = fs
+            .metadata(&symlink_path)
+            .await
+            .expect("metadata call succeeds")
+            .expect("metadata returned");
+        assert!(metadata.is_symlink);
+        assert!(!metadata.is_dir);
+        assert!(!metadata.is_fifo);
+        assert!(!metadata.is_executable);
+        // don't care about len or mtime on symlinks?
+    }
+
+    #[gpui::test]
+    #[cfg(unix)]
+    async fn test_realfs_symlink_loop_metadata(executor: BackgroundExecutor) {
+        let tempdir = TempDir::new().unwrap();
+        let path = tempdir.path();
+        let fs = RealFs {
+            bundled_git_binary_path: None,
+            executor,
+            next_job_id: Arc::new(AtomicUsize::new(0)),
+            job_event_subscribers: Arc::new(Mutex::new(Vec::new())),
+        };
+        let symlink_path = path.join("symlink");
+        smol::block_on(fs.create_symlink(&symlink_path, PathBuf::from("symlink"))).unwrap();
+        let metadata = fs
+            .metadata(&symlink_path)
+            .await
+            .expect("metadata call succeeds")
+            .expect("metadata returned");
+        assert!(metadata.is_symlink);
+        assert!(!metadata.is_dir);
+        assert!(!metadata.is_fifo);
+        assert!(!metadata.is_executable);
+        // don't care about len or mtime on symlinks?
+    }
 }