fix: address review feedback - race condition, test portability (PR #350)

Author: karthiknadig

crates/pet-conda/src/lib.rs

@@ -168,13 +168,15 @@ impl CondaLocator for Conda {
                 self.managers.insert(conda_dir.clone(), manager.clone());
 
                 // Also check for a mamba/micromamba manager in the same directory and report it.
-                if !self.mamba_managers.contains_key(&conda_dir) {
-                    if let Some(mamba_mgr) = get_mamba_manager(&conda_dir) {
-                        self.mamba_managers
-                            .insert(conda_dir.clone(), mamba_mgr.clone());
-                        reporter.report_manager(&mamba_mgr.to_manager());
-                    }
-                }
+                let _ = self
+                    .mamba_managers
+                    .get_or_insert_with(conda_dir.clone(), || {
+                        let mgr = get_mamba_manager(&conda_dir);
+                        if let Some(ref m) = mgr {
+                            reporter.report_manager(&m.to_manager());
+                        }
+                        mgr
+                    });
 
                 // Find all the environments in the conda install folder. (under `envs` folder)
                 for conda_env in
@@ -349,14 +351,16 @@ impl Locator for Conda {
                         reporter.report_environment(&env);
 
                         // Also check for a mamba/micromamba manager in the same directory and report it.
-                        let is_new_mamba = !self.mamba_managers.contains_key(conda_dir);
-                        if let Some(mamba_mgr) = self.mamba_managers.get_or_insert_with(conda_dir.clone(), || {
-                            get_mamba_manager(conda_dir)
-                        }) {
-                            if is_new_mamba {
-                                reporter.report_manager(&mamba_mgr.to_manager());
+                        // Reporting inside the closure minimizes the TOCTOU window compared to a
+                        // separate contains_key check, though concurrent threads may still
+                        // briefly both invoke the closure before the write-lock double-check.
+                        let _ = self.mamba_managers.get_or_insert_with(conda_dir.clone(), || {
+                            let mgr = get_mamba_manager(conda_dir);
+                            if let Some(ref m) = mgr {
+                                reporter.report_manager(&m.to_manager());
                             }
-                        }
+                            mgr
+                        });
                     } else {
                         // We will still return the conda env even though we do not have the manager.
                         // This might seem incorrect, however the tool is about discovering environments.

crates/pet-conda/tests/manager_test.rs

@@ -140,18 +140,24 @@ fn is_mamba_executable_identifies_mamba_binaries() {
     use pet_conda::manager::is_mamba_executable;
     use std::path::Path;
 
+    // Cross-platform: forward-slash paths work on all platforms
     assert!(is_mamba_executable(Path::new("/usr/bin/mamba")));
     assert!(is_mamba_executable(Path::new("/usr/bin/micromamba")));
-    assert!(is_mamba_executable(Path::new("C:\\Scripts\\mamba.exe")));
-    assert!(is_mamba_executable(Path::new(
-        "C:\\Scripts\\micromamba.exe"
-    )));
     assert!(is_mamba_executable(Path::new("/opt/miniforge3/bin/mamba")));
 
     // Should NOT match conda or python
     assert!(!is_mamba_executable(Path::new("/usr/bin/conda")));
     assert!(!is_mamba_executable(Path::new("/usr/bin/python")));
-    assert!(!is_mamba_executable(Path::new("C:\\Scripts\\conda.exe")));
+
+    // Windows-specific paths with backslashes (only valid on Windows)
+    #[cfg(windows)]
+    {
+        assert!(is_mamba_executable(Path::new("C:\\Scripts\\mamba.exe")));
+        assert!(is_mamba_executable(Path::new(
+            "C:\\Scripts\\micromamba.exe"
+        )));
+        assert!(!is_mamba_executable(Path::new("C:\\Scripts\\conda.exe")));
+    }
 }
 
 /// Test that get_mamba_manager finds a mamba manager from a conda install with mamba.