From 4e9e94435751b173e5f3b6c576f4a28638070ce8 Mon Sep 17 00:00:00 2001 From: Smit Barmase Date: Tue, 10 Mar 2026 03:56:45 +0530 Subject: [PATCH] fs: Fix no-overwrite rename races (#51090) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #46661 This PR changes `fs.rename` to use the platform’s atomic no-overwrite rename on all platforms when `overwrite` is `false`. This fixes a case where concurrent renames to the same target could race past a separate metadata check and end up overwriting each other. In Project Panel, we can still rename entries in parallel without worrying about OS internals not handling it correctly or making these renames sequential. Release Notes: - Fixed an issue in the Project Panel where conflicting file moves could overwrite each other instead of leaving the losing file in place. --- crates/fs/src/fs.rs | 118 ++++++++++++++++++++++++++++-- crates/fs/tests/integration/fs.rs | 59 +++++++++++++++ 2 files changed, 170 insertions(+), 7 deletions(-) diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index 0fde444171042eda859edcac7915c456ab91e265..6c7074d2139068d2ea581ea6343de4d4c1f09030 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -15,10 +15,14 @@ use gpui::Global; use gpui::ReadGlobal as _; use gpui::SharedString; use std::borrow::Cow; +#[cfg(unix)] +use std::ffi::CString; use util::command::new_command; #[cfg(unix)] use std::os::fd::{AsFd, AsRawFd}; +#[cfg(unix)] +use std::os::unix::ffi::OsStrExt; #[cfg(unix)] use std::os::unix::fs::{FileTypeExt, MetadataExt}; @@ -506,6 +510,63 @@ impl RealFs { } } +#[cfg(any(target_os = "macos", target_os = "linux"))] +fn rename_without_replace(source: &Path, target: &Path) -> io::Result<()> { + let source = path_to_c_string(source)?; + let target = path_to_c_string(target)?; + + #[cfg(target_os = "macos")] + let result = unsafe { libc::renamex_np(source.as_ptr(), target.as_ptr(), libc::RENAME_EXCL) }; + + #[cfg(target_os = "linux")] + let result = unsafe { + libc::syscall( + libc::SYS_renameat2, + libc::AT_FDCWD, + source.as_ptr(), + libc::AT_FDCWD, + target.as_ptr(), + libc::RENAME_NOREPLACE, + ) + }; + + if result == 0 { + Ok(()) + } else { + Err(io::Error::last_os_error()) + } +} + +#[cfg(target_os = "windows")] +fn rename_without_replace(source: &Path, target: &Path) -> io::Result<()> { + use std::os::windows::ffi::OsStrExt; + + use windows::Win32::Storage::FileSystem::{MOVE_FILE_FLAGS, MoveFileExW}; + use windows::core::PCWSTR; + + let source: Vec = source.as_os_str().encode_wide().chain(Some(0)).collect(); + let target: Vec = target.as_os_str().encode_wide().chain(Some(0)).collect(); + + unsafe { + MoveFileExW( + PCWSTR(source.as_ptr()), + PCWSTR(target.as_ptr()), + MOVE_FILE_FLAGS::default(), + ) + } + .map_err(|_| io::Error::last_os_error()) +} + +#[cfg(any(target_os = "macos", target_os = "linux"))] +fn path_to_c_string(path: &Path) -> io::Result { + CString::new(path.as_os_str().as_bytes()).map_err(|_| { + io::Error::new( + io::ErrorKind::InvalidInput, + format!("path contains interior NUL: {}", path.display()), + ) + }) +} + #[async_trait::async_trait] impl Fs for RealFs { async fn create_dir(&self, path: &Path) -> Result<()> { @@ -588,7 +649,56 @@ impl Fs for RealFs { } async fn rename(&self, source: &Path, target: &Path, options: RenameOptions) -> Result<()> { - if !options.overwrite && smol::fs::metadata(target).await.is_ok() { + if options.create_parents { + if let Some(parent) = target.parent() { + self.create_dir(parent).await?; + } + } + + if options.overwrite { + smol::fs::rename(source, target).await?; + return Ok(()); + } + + let use_metadata_fallback = { + #[cfg(any(target_os = "macos", target_os = "linux", target_os = "windows"))] + { + let source = source.to_path_buf(); + let target = target.to_path_buf(); + match self + .executor + .spawn(async move { rename_without_replace(&source, &target) }) + .await + { + Ok(()) => return Ok(()), + Err(error) if error.kind() == io::ErrorKind::AlreadyExists => { + if options.ignore_if_exists { + return Ok(()); + } + return Err(error.into()); + } + Err(error) + if error.raw_os_error().is_some_and(|code| { + code == libc::ENOSYS + || code == libc::ENOTSUP + || code == libc::EOPNOTSUPP + }) => + { + // For case when filesystem or kernel does not support atomic no-overwrite rename. + true + } + Err(error) => return Err(error.into()), + } + } + + #[cfg(not(any(target_os = "macos", target_os = "linux", target_os = "windows")))] + { + // For platforms which do not have an atomic no-overwrite rename yet. + true + } + }; + + if use_metadata_fallback && smol::fs::metadata(target).await.is_ok() { if options.ignore_if_exists { return Ok(()); } else { @@ -596,12 +706,6 @@ impl Fs for RealFs { } } - if options.create_parents { - if let Some(parent) = target.parent() { - self.create_dir(parent).await?; - } - } - smol::fs::rename(source, target).await?; Ok(()) } diff --git a/crates/fs/tests/integration/fs.rs b/crates/fs/tests/integration/fs.rs index dd5e694e23c99716a81b27afd487e3a6ea648209..b688d5e2c243ede5eb3f499ad2956feaec01a965 100644 --- a/crates/fs/tests/integration/fs.rs +++ b/crates/fs/tests/integration/fs.rs @@ -523,6 +523,65 @@ async fn test_rename(executor: BackgroundExecutor) { ); } +#[gpui::test] +#[cfg(any(target_os = "macos", target_os = "linux", target_os = "windows"))] +async fn test_realfs_parallel_rename_without_overwrite_preserves_losing_source( + executor: BackgroundExecutor, +) { + let temp_dir = TempDir::new().unwrap(); + let root = temp_dir.path(); + let source_a = root.join("dir_a/shared.txt"); + let source_b = root.join("dir_b/shared.txt"); + let target = root.join("shared.txt"); + + std::fs::create_dir_all(source_a.parent().unwrap()).unwrap(); + std::fs::create_dir_all(source_b.parent().unwrap()).unwrap(); + std::fs::write(&source_a, "from a").unwrap(); + std::fs::write(&source_b, "from b").unwrap(); + + let fs = RealFs::new(None, executor); + let (first_result, second_result) = futures::future::join( + fs.rename(&source_a, &target, RenameOptions::default()), + fs.rename(&source_b, &target, RenameOptions::default()), + ) + .await; + + assert_ne!(first_result.is_ok(), second_result.is_ok()); + assert!(target.exists()); + assert_eq!(source_a.exists() as u8 + source_b.exists() as u8, 1); +} + +#[gpui::test] +#[cfg(any(target_os = "macos", target_os = "linux", target_os = "windows"))] +async fn test_realfs_rename_ignore_if_exists_leaves_source_and_target_unchanged( + executor: BackgroundExecutor, +) { + let temp_dir = TempDir::new().unwrap(); + let root = temp_dir.path(); + let source = root.join("source.txt"); + let target = root.join("target.txt"); + + std::fs::write(&source, "from source").unwrap(); + std::fs::write(&target, "from target").unwrap(); + + let fs = RealFs::new(None, executor); + let result = fs + .rename( + &source, + &target, + RenameOptions { + ignore_if_exists: true, + ..Default::default() + }, + ) + .await; + + assert!(result.is_ok()); + + assert_eq!(std::fs::read_to_string(&source).unwrap(), "from source"); + assert_eq!(std::fs::read_to_string(&target).unwrap(), "from target"); +} + #[gpui::test] #[cfg(unix)] async fn test_realfs_broken_symlink_metadata(executor: BackgroundExecutor) {