Lock the credentials store when reading or writing (#15610)

Adds locking of the credentials store for concurrency safety. It's
important to hold the lock from read -> write so credentials are not
dropped during concurrent writes.

I opted not to attach the lock to the store itself. Instead, I return
the lock on read and require it on write to encourage safe use. Maybe
attaching the source path to the store struct and adding a `lock(&self)`
method would make sense? but then you can forget to take the lock at the
right time. The main problem with the interface here is to write a _new_
store you have to take the lock yourself, and you could make a mistake
by taking a lock for the wrong path or something. The fix for that would
be to introduce a new `CredentialStoreHandle` type or something, but
that seems overzealous rn. We also don't eagerly drop the lock on token
read, although we could.

Cargo.lock

@@ -5095,6 +5095,7 @@ dependencies = [
  "toml",
  "tracing",
  "url",
+ "uv-fs",
  "uv-keyring",
  "uv-once-map",
  "uv-redacted",

crates/uv-auth/Cargo.toml

@@ -10,6 +10,7 @@ doctest = false
 workspace = true
 
 [dependencies]
+uv-fs = { workspace = true }
 uv-keyring = { workspace = true, features = ["apple-native", "secret-service", "windows-native"] }
 uv-once-map = { workspace = true }
 uv-redacted = { workspace = true }

crates/uv-auth/src/middleware.rs

@@ -71,8 +71,8 @@ impl Default for TextStoreMode {
                 })
                 .ok()?;
 
-            match TextCredentialStore::from_file(&path) {
-                Ok(store) => {
+            match TextCredentialStore::read(&path) {
+                Ok((store, _lock)) => {
                     debug!("Loaded credential file {}", path.display());
                     Some(store)
                 }

crates/uv-auth/src/store.rs

@@ -6,6 +6,7 @@ use rustc_hash::FxHashMap;
 use serde::{Deserialize, Serialize};
 use thiserror::Error;
 use url::Url;
+use uv_fs::{LockedFile, with_added_extension};
 use uv_redacted::DisplaySafeUrl;
 
 use uv_state::{StateBucket, StateStore};
@@ -199,8 +200,17 @@ impl TextCredentialStore {
         Ok(dir.join("credentials.toml"))
     }
 
+    /// Acquire a lock on the credentials file at the given path.
+    pub fn lock(path: &Path) -> Result<LockedFile, TomlCredentialError> {
+        if let Some(parent) = path.parent() {
+            fs::create_dir_all(parent)?;
+        }
+        let lock = with_added_extension(path, ".lock");
+        Ok(LockedFile::acquire_blocking(lock, "credentials store")?)
+    }
+
     /// Read credentials from a file.
-    pub fn from_file<P: AsRef<Path>>(path: P) -> Result<Self, TomlCredentialError> {
+    fn from_file<P: AsRef<Path>>(path: P) -> Result<Self, TomlCredentialError> {
         let content = fs::read_to_string(path)?;
         let credentials: TomlCredentials = toml::from_str(&content)?;
 
@@ -213,8 +223,26 @@ impl TextCredentialStore {
         Ok(Self { credentials })
     }
 
+    /// Read credentials from a file.
+    ///
+    /// Returns [`TextCredentialStore`] and a [`LockedFile`] to hold if mutating the store.
+    ///
+    /// If the store will not be written to following the read, the lock can be dropped.
+    pub fn read<P: AsRef<Path>>(path: P) -> Result<(Self, LockedFile), TomlCredentialError> {
+        let lock = Self::lock(path.as_ref())?;
+        let store = Self::from_file(path)?;
+        Ok((store, lock))
+    }
+
     /// Persist credentials to a file.
-    pub fn write<P: AsRef<Path>>(self, path: P) -> Result<(), TomlCredentialError> {
+    ///
+    /// Requires a [`LockedFile`] from [`TextCredentialStore::lock`] or
+    /// [`TextCredentialStore::read`] to ensure exclusive access.
+    pub fn write<P: AsRef<Path>>(
+        self,
+        path: P,
+        _lock: LockedFile,
+    ) -> Result<(), TomlCredentialError> {
         let credentials = self
             .credentials
             .into_iter()
@@ -390,7 +418,12 @@ password = "pass2"
 
         // Test saving
         let temp_output = NamedTempFile::new().unwrap();
-        store.write(temp_output.path()).unwrap();
+        store
+            .write(
+                temp_output.path(),
+                TextCredentialStore::lock(temp_file.path()).unwrap(),
+            )
+            .unwrap();
 
         let content = fs::read_to_string(temp_output.path()).unwrap();
         assert!(content.contains("example.com"));

crates/uv-fs/src/lib.rs

@@ -1,3 +1,4 @@
+use std::borrow::Cow;
 use std::fmt::Display;
 use std::io;
 use std::path::{Path, PathBuf};
@@ -12,6 +13,24 @@ pub mod cachedir;
 mod path;
 pub mod which;
 
+/// Append an extension to a [`PathBuf`].
+///
+/// Unlike [`Path::with_extension`], this function does not replace an existing extension.
+///
+/// If there is no file name, the path is returned unchanged.
+///
+/// This mimics the behavior of the unstable [`Path::with_added_extension`] method.
+pub fn with_added_extension<'a>(path: &'a Path, extension: &str) -> Cow<'a, Path> {
+    let Some(name) = path.file_name() else {
+        // If there is no file name, we cannot add an extension.
+        return Cow::Borrowed(path);
+    };
+    let mut name = name.to_os_string();
+    name.push(".");
+    name.push(extension.trim_start_matches('.'));
+    Cow::Owned(path.with_file_name(name))
+}
+
 /// Attempt to check if the two paths refer to the same file.
 ///
 /// Returns `Some(true)` if the files are missing, but would be the same if they existed.
@@ -812,3 +831,45 @@ pub fn copy_dir_all(src: impl AsRef<Path>, dst: impl AsRef<Path>) -> std::io::Re
     }
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::path::PathBuf;
+
+    #[test]
+    fn test_with_added_extension() {
+        // Test with simple package name (no dots)
+        let path = PathBuf::from("python");
+        let result = with_added_extension(&path, "exe");
+        assert_eq!(result, PathBuf::from("python.exe"));
+
+        // Test with package name containing single dot
+        let path = PathBuf::from("awslabs.cdk-mcp-server");
+        let result = with_added_extension(&path, "exe");
+        assert_eq!(result, PathBuf::from("awslabs.cdk-mcp-server.exe"));
+
+        // Test with package name containing multiple dots
+        let path = PathBuf::from("org.example.tool");
+        let result = with_added_extension(&path, "exe");
+        assert_eq!(result, PathBuf::from("org.example.tool.exe"));
+
+        // Test with different extensions
+        let path = PathBuf::from("script");
+        let result = with_added_extension(&path, "ps1");
+        assert_eq!(result, PathBuf::from("script.ps1"));
+
+        // Test with path that has directory components
+        let path = PathBuf::from("some/path/to/awslabs.cdk-mcp-server");
+        let result = with_added_extension(&path, "exe");
+        assert_eq!(
+            result,
+            PathBuf::from("some/path/to/awslabs.cdk-mcp-server.exe")
+        );
+
+        // Test with empty path (edge case)
+        let path = PathBuf::new();
+        let result = with_added_extension(&path, "exe");
+        assert_eq!(result, path); // Should return unchanged
+    }
+}

crates/uv-shell/src/runnable.rs

@@ -2,27 +2,10 @@
 
 use std::env::consts::EXE_EXTENSION;
 use std::ffi::OsStr;
-use std::path::{Path, PathBuf};
+use std::path::Path;
 use std::process::Command;
 
-/// Append an extension to a [`PathBuf`].
-///
-/// Unlike [`Path::with_extension`], this function does not replace an existing extension.
-///
-/// If there is no file name, the path is returned unchanged.
-///
-/// This mimics the behavior of the unstable [`Path::with_added_extension`] method.
-fn add_extension_to_path(mut path: PathBuf, extension: &str) -> PathBuf {
-    let Some(name) = path.file_name() else {
-        // If there is no file name, we cannot add an extension.
-        return path;
-    };
-    let mut name = name.to_os_string();
-    name.push(".");
-    name.push(extension.trim_start_matches('.'));
-    path.set_file_name(name);
-    path
-}
+use uv_fs::with_added_extension;
 
 #[derive(Debug)]
 pub enum WindowsRunnable {
@@ -109,7 +92,7 @@ impl WindowsRunnable {
             .map(|script_type| {
                 (
                     script_type,
-                    add_extension_to_path(script_path.clone(), script_type.to_extension()),
+                    with_added_extension(&script_path, script_type.to_extension()),
                 )
             })
             .find(|(_, script_path)| script_path.is_file())
@@ -120,50 +103,16 @@ impl WindowsRunnable {
 
 #[cfg(test)]
 mod tests {
-    use super::*;
-    use std::path::PathBuf;
 
+    #[cfg(target_os = "windows")]
+    use super::WindowsRunnable;
     #[cfg(target_os = "windows")]
     use fs_err as fs;
     #[cfg(target_os = "windows")]
+    use std::ffi::OsStr;
+    #[cfg(target_os = "windows")]
     use std::io;
 
-    #[test]
-    fn test_add_extension_to_path() {
-        // Test with simple package name (no dots)
-        let path = PathBuf::from("python");
-        let result = add_extension_to_path(path, "exe");
-        assert_eq!(result, PathBuf::from("python.exe"));
-
-        // Test with package name containing single dot
-        let path = PathBuf::from("awslabs.cdk-mcp-server");
-        let result = add_extension_to_path(path, "exe");
-        assert_eq!(result, PathBuf::from("awslabs.cdk-mcp-server.exe"));
-
-        // Test with package name containing multiple dots
-        let path = PathBuf::from("org.example.tool");
-        let result = add_extension_to_path(path, "exe");
-        assert_eq!(result, PathBuf::from("org.example.tool.exe"));
-
-        // Test with different extensions
-        let path = PathBuf::from("script");
-        let result = add_extension_to_path(path, "ps1");
-        assert_eq!(result, PathBuf::from("script.ps1"));
-
-        // Test with path that has directory components
-        let path = PathBuf::from("some/path/to/awslabs.cdk-mcp-server");
-        let result = add_extension_to_path(path, "exe");
-        assert_eq!(
-            result,
-            PathBuf::from("some/path/to/awslabs.cdk-mcp-server.exe")
-        );
-
-        // Test with empty path (edge case)
-        let path = PathBuf::new();
-        let result = add_extension_to_path(path.clone(), "exe");
-        assert_eq!(result, path); // Should return unchanged
-    }
-
     /// Helper function to create a temporary directory with test files
     #[cfg(target_os = "windows")]
     fn create_test_environment() -> io::Result<tempfile::TempDir> {

crates/uv/src/commands/auth/login.rs

@@ -115,9 +115,9 @@ pub(crate) async fn login(
         AuthBackend::Keyring(provider) => {
             provider.store(&url, &credentials).await?;
         }
-        AuthBackend::TextStore(mut text_store) => {
-            text_store.insert(service.clone(), credentials);
-            text_store.write(TextCredentialStore::default_file()?)?;
+        AuthBackend::TextStore(mut store, _lock) => {
+            store.insert(service.clone(), credentials);
+            store.write(TextCredentialStore::default_file()?, _lock)?;
         }
     }
 

crates/uv/src/commands/auth/logout.rs

@@ -61,12 +61,12 @@ pub(crate) async fn logout(
                 .await
                 .with_context(|| format!("Unable to remove credentials for {display_url}"))?;
         }
-        AuthBackend::TextStore(mut text_store) => {
-            if text_store.remove(&service).is_none() {
+        AuthBackend::TextStore(mut store, _lock) => {
+            if store.remove(&service).is_none() {
                 bail!("No matching entry found for {display_url}");
             }
-            text_store
-                .write(TextCredentialStore::default_file()?)
+            store
+                .write(TextCredentialStore::default_file()?, _lock)
                 .with_context(|| "Failed to persist changes to credentials after removal")?;
         }
     }

crates/uv/src/commands/auth/mod.rs

@@ -1,5 +1,6 @@
 use uv_auth::{KeyringProvider, TextCredentialStore, TomlCredentialError};
 use uv_configuration::KeyringProviderType;
+use uv_fs::LockedFile;
 use uv_preview::Preview;
 
 pub(crate) mod dir;
@@ -10,7 +11,7 @@ pub(crate) mod token;
 /// The storage backend to use in `uv auth` commands.
 enum AuthBackend {
     Keyring(KeyringProvider),
-    TextStore(TextCredentialStore),
+    TextStore(TextCredentialStore, LockedFile),
 }
 
 impl AuthBackend {
@@ -27,10 +28,14 @@ impl AuthBackend {
         }
 
         // Otherwise, we'll use the plain text credential store
-        match TextCredentialStore::from_file(TextCredentialStore::default_file()?) {
-            Ok(store) => Ok(Self::TextStore(store)),
+        let path = TextCredentialStore::default_file()?;
+        match TextCredentialStore::read(&path) {
+            Ok((store, lock)) => Ok(Self::TextStore(store, lock)),
             Err(TomlCredentialError::Io(err)) if err.kind() == std::io::ErrorKind::NotFound => {
-                Ok(Self::TextStore(TextCredentialStore::default()))
+                Ok(Self::TextStore(
+                    TextCredentialStore::default(),
+                    TextCredentialStore::lock(&path)?,
+                ))
             }
             Err(err) => Err(err),
         }

crates/uv/src/commands/auth/token.rs

@@ -47,7 +47,7 @@ pub(crate) async fn token(
             .fetch(url, Some(&username))
             .await
             .ok_or_else(|| anyhow::anyhow!("Failed to fetch credentials for {display_url}"))?,
-        AuthBackend::TextStore(text_store) => text_store
+        AuthBackend::TextStore(store, _lock) => store
             .get_credentials(url)
             .cloned()
             .ok_or_else(|| anyhow::anyhow!("Failed to fetch credentials for {display_url}"))?,