From 148627a08ac68dc785099acf4a6b77f17fe57757 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Thu, 6 Aug 2026 10:31:17 -0700 Subject: [PATCH 1/3] fix: validate contextual executable cache aliases (Fixes #448) Unify relative and absolute cache identities while preserving short caller-facing aliases. Invalidate missing tracked executables and cover memory, disk, and fast-path behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/pet-python-utils/src/cache.rs | 183 +++++++++++++++++++----- crates/pet-python-utils/src/env.rs | 21 ++- crates/pet-python-utils/src/fs_cache.rs | 93 +++++++++--- 3 files changed, 244 insertions(+), 53 deletions(-) diff --git a/crates/pet-python-utils/src/cache.rs b/crates/pet-python-utils/src/cache.rs index a1bc9c66..d6237523 100644 --- a/crates/pet-python-utils/src/cache.rs +++ b/crates/pet-python-utils/src/cache.rs @@ -13,7 +13,10 @@ use std::{ use crate::{ env::ResolvedPythonEnv, - fs_cache::{delete_cache_file, get_cache_from_file, store_cache_in_file}, + fs_cache::{ + delete_cache_file, executable_cache_key, executable_cache_key_from, get_cache_from_file, + store_cache_in_file, + }, }; lazy_static! { @@ -22,6 +25,10 @@ lazy_static! { pub trait CacheEntry: Send + Sync { fn get(&self) -> Option; + fn get_for_executable(&self, executable: &std::path::Path) -> Option { + self.get() + .map(|environment| environment.for_executable_alias(executable)) + } fn store(&self, environment: ResolvedPythonEnv); fn track_symlinks(&self, symlinks: Vec); } @@ -102,6 +109,7 @@ impl CacheImpl { } } fn create_cache(&self, executable: PathBuf) -> LockableCacheEntry { + let cache_key = executable_cache_key(&executable); let cache_directory = self .cache_dir .lock() @@ -111,11 +119,11 @@ impl CacheImpl { .locks .lock() .expect("locks mutex poisoned") - .entry(executable.clone()) + .entry(cache_key.clone()) { Entry::Occupied(lock) => lock.get().clone(), Entry::Vacant(lock) => { - let cache = Box::new(CacheEntryImpl::create(cache_directory.clone(), executable)) + let cache = Box::new(CacheEntryImpl::create(cache_directory.clone(), cache_key)) as Box; lock.insert(Arc::new(Mutex::new(cache))).clone() } @@ -129,6 +137,16 @@ impl CacheImpl { /// See: https://github.com/microsoft/python-environment-tools/issues/223 type FilePathWithMTimeCTime = (PathBuf, SystemTime, Option); +fn current_dir_for_aliases(aliases: &[PathBuf]) -> Option { + aliases + .iter() + .any(|alias| alias.is_relative()) + .then(std::env::current_dir) + .transpose() + .ok() + .flatten() +} + struct CacheEntryImpl { cache_directory: Option, executable: PathBuf, @@ -146,37 +164,35 @@ impl CacheEntryImpl { } } pub fn verify_in_memory_cache(&self) { - // Check if any of the exes have changed since we last cached this. - for symlink_info in self + let cache_is_valid = self .symlinks .lock() .expect("symlinks mutex poisoned") .iter() - { - if let Ok(metadata) = symlink_info.0.metadata() { - let mtime_changed = metadata.modified().ok() != Some(symlink_info.1); - // Only check ctime if we have it stored (may be None on Linux) - let ctime_changed = match symlink_info.2 { - Some(stored_ctime) => metadata.created().ok() != Some(stored_ctime), - None => false, // Can't check ctime if we don't have it - }; - if mtime_changed || ctime_changed { - trace!( - "Symlink {:?} has changed since we last cached it. original mtime & ctime {:?}, {:?}, current mtime & ctime {:?}, {:?}", - symlink_info.0, - symlink_info.1, - symlink_info.2, - metadata.modified().ok(), - metadata.created().ok() - ); - self.envoronment - .lock() - .expect("envoronment mutex poisoned") - .take(); - if let Some(cache_directory) = &self.cache_directory { - delete_cache_file(cache_directory, &self.executable); - } + .all(|symlink_info| { + if let Ok(metadata) = symlink_info.0.metadata() { + let mtime_changed = metadata.modified().ok() != Some(symlink_info.1); + let ctime_changed = match symlink_info.2 { + Some(stored_ctime) => metadata.created().ok() != Some(stored_ctime), + None => false, + }; + !mtime_changed && !ctime_changed + } else { + false } + }); + + if !cache_is_valid { + trace!( + "Tracked executable changed or disappeared for {:?}", + self.executable + ); + self.envoronment + .lock() + .expect("envoronment mutex poisoned") + .take(); + if let Some(cache_directory) = &self.cache_directory { + delete_cache_file(cache_directory, &self.executable); } } } @@ -215,14 +231,17 @@ impl CacheEntry for CacheEntryImpl { fn store(&self, environment: ResolvedPythonEnv) { // Get hold of the mtimes and ctimes of the symlinks. + let aliases = environment.symlinks.clone().unwrap_or_default(); + let current_dir = current_dir_for_aliases(&aliases); let mut symlinks = vec![]; - for symlink in environment.symlinks.clone().unwrap_or_default().iter() { + for alias in &aliases { + let symlink = executable_cache_key_from(alias, current_dir.as_deref()); if let Ok(metadata) = symlink.metadata() { // We require mtime, but ctime is optional (not available on all Linux filesystems) // See: https://github.com/microsoft/python-environment-tools/issues/223 if let Ok(modified) = metadata.modified() { let created = metadata.created().ok(); // May be None on Linux - symlinks.push((symlink.clone(), modified, created)); + symlinks.push((symlink, modified, created)); } } } @@ -259,8 +278,12 @@ impl CacheEntry for CacheEntryImpl { .iter() .map(|x| x.0.clone()) .collect(); - - if symlinks.iter().all(|x| known_symlinks.contains(x)) { + let current_dir = current_dir_for_aliases(&symlinks); + if symlinks + .iter() + .map(|alias| executable_cache_key_from(alias, current_dir.as_deref())) + .all(|key| known_symlinks.contains(&key)) + { return; } @@ -283,3 +306,97 @@ impl CacheEntry for CacheEntryImpl { } } } + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::tempdir_in; + + fn environment(executable: PathBuf, aliases: Vec) -> ResolvedPythonEnv { + ResolvedPythonEnv { + executable, + prefix: PathBuf::from("prefix"), + version: "3.12.0".to_string(), + is64_bit: true, + symlinks: Some(aliases), + } + } + + fn aliases() -> (tempfile::TempDir, PathBuf, PathBuf) { + let current_dir = std::env::current_dir().unwrap(); + let temp_dir = tempdir_in(¤t_dir).unwrap(); + let absolute = temp_dir.path().join("python"); + std::fs::write(&absolute, "python").unwrap(); + let relative = absolute.strip_prefix(¤t_dir).unwrap().to_path_buf(); + (temp_dir, relative, absolute) + } + + #[test] + fn relative_and_absolute_aliases_share_in_memory_entry() { + let (_temp_dir, relative, absolute) = aliases(); + let cache = CacheImpl::new(None); + + let relative_entry = cache.create_cache(relative); + let absolute_entry = cache.create_cache(absolute); + + assert!(Arc::ptr_eq(&relative_entry, &absolute_entry)); + } + + #[test] + fn cache_hit_uses_current_alias_and_preserves_shorter_aliases() { + let (_temp_dir, relative, absolute) = aliases(); + let cache = CacheImpl::new(None); + let entry = cache.create_cache(relative.clone()); + let entry = entry.lock().unwrap(); + entry.store(environment( + relative.clone(), + vec![relative.clone(), absolute.clone()], + )); + + let relative_hit = entry.get_for_executable(&relative).unwrap(); + assert_eq!(relative_hit.executable, relative); + + let absolute_hit = entry.get_for_executable(&absolute).unwrap(); + assert_eq!(absolute_hit.executable, absolute); + let hit_aliases = absolute_hit.symlinks.unwrap(); + assert!(hit_aliases.contains(&relative)); + assert!(hit_aliases.contains(&absolute)); + } + + #[test] + fn disk_cache_reuses_relative_entry_for_absolute_alias() { + let (temp_dir, relative, absolute) = aliases(); + let cache_directory = temp_dir.path().join("cache"); + { + let cache = CacheImpl::new(Some(cache_directory.clone())); + let entry = cache.create_cache(relative.clone()); + entry.lock().unwrap().store(environment( + relative.clone(), + vec![relative.clone(), absolute.clone()], + )); + } + + let cache = CacheImpl::new(Some(cache_directory)); + let entry = cache.create_cache(absolute.clone()); + let hit = entry.lock().unwrap().get_for_executable(&absolute).unwrap(); + + assert_eq!(hit.executable, absolute); + let hit_aliases = hit.symlinks.unwrap(); + assert!(hit_aliases.contains(&relative)); + assert!(hit_aliases.contains(&absolute)); + } + + #[test] + fn missing_tracked_executable_invalidates_in_memory_entry() { + let (temp_dir, _relative, absolute) = aliases(); + let cache = CacheImpl::new(Some(temp_dir.path().join("cache"))); + let entry = cache.create_cache(absolute.clone()); + let entry = entry.lock().unwrap(); + entry.store(environment(absolute.clone(), vec![absolute.clone()])); + assert!(entry.get().is_some()); + + std::fs::remove_file(&absolute).unwrap(); + + assert!(entry.get().is_none()); + } +} diff --git a/crates/pet-python-utils/src/env.rs b/crates/pet-python-utils/src/env.rs index 9d712028..1ac49b6a 100644 --- a/crates/pet-python-utils/src/env.rs +++ b/crates/pet-python-utils/src/env.rs @@ -41,6 +41,25 @@ pub struct ResolvedPythonEnv { } impl ResolvedPythonEnv { + pub(crate) fn for_executable_alias(mut self, executable: &Path) -> Self { + let alias_is_current = self.executable == executable + && self + .symlinks + .as_ref() + .is_some_and(|aliases| aliases.iter().any(|alias| alias == executable)); + if alias_is_current { + return self; + } + + let mut symlinks = self.symlinks.take().unwrap_or_default(); + symlinks.push(executable.to_path_buf()); + symlinks.sort(); + symlinks.dedup(); + self.executable = executable.to_path_buf(); + self.symlinks = Some(symlinks); + self + } + pub fn to_python_env(&self) -> PythonEnv { let mut env = PythonEnv::new( self.executable.clone(), @@ -84,7 +103,7 @@ impl ResolvedPythonEnv { ) -> Option { let cache = create_cache(executable.to_path_buf()); let entry = cache.lock().expect("cache mutex poisoned"); - if let Some(env) = entry.get() { + if let Some(env) = entry.get_for_executable(executable) { Some(env) } else if let Some(env) = get_interpreter_details(executable) { entry.store(env.clone()); diff --git a/crates/pet-python-utils/src/fs_cache.rs b/crates/pet-python-utils/src/fs_cache.rs index cf93fee5..b0feae46 100644 --- a/crates/pet-python-utils/src/fs_cache.rs +++ b/crates/pet-python-utils/src/fs_cache.rs @@ -6,8 +6,9 @@ use pet_fs::path::norm_case; use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use std::{ + env, fs::{self, File}, - io::BufReader, + io::{self, BufReader}, path::{Path, PathBuf}, time::SystemTime, }; @@ -27,34 +28,28 @@ struct CacheEntry { pub symlinks: Vec, } -pub fn generate_cache_file(cache_directory: &Path, executable: &PathBuf) -> PathBuf { - // Version 4: Changed ctime from required to optional for Linux compatibility - // See: https://github.com/microsoft/python-environment-tools/issues/223 - cache_directory.join(format!("{}.4.json", generate_hash(executable))) +pub fn generate_cache_file(cache_directory: &Path, executable: &Path) -> PathBuf { + // Version 5: Relative and absolute aliases share an absolute cache identity. + cache_directory.join(format!("{}.5.json", generate_hash(executable))) } -pub fn delete_cache_file(cache_directory: &Path, executable: &PathBuf) { +pub fn delete_cache_file(cache_directory: &Path, executable: &Path) { let cache_file = generate_cache_file(cache_directory, executable); let _ = fs::remove_file(cache_file); } pub fn get_cache_from_file( cache_directory: &Path, - executable: &PathBuf, + executable: &Path, ) -> Option<(ResolvedPythonEnv, Vec)> { let cache_file = generate_cache_file(cache_directory, executable); let file = File::open(cache_file.clone()).ok()?; let reader = BufReader::new(file); let cache: CacheEntry = serde_json::from_reader(reader).ok()?; - // Account for conflicts in the cache file - // i.e. the hash generated is same for another file, remember we only take the first 16 chars. - if !cache - .environment - .clone() - .symlinks - .unwrap_or_default() - .contains(executable) - { + let cache_key = executable_cache_key(executable); + // Account for conflicts in the cache file. The tracked paths are stored as + // absolute identities, so this remains valid when the caller uses a relative alias. + if !cache.symlinks.iter().any(|symlink| symlink.0 == cache_key) { trace!( "Cache file {:?} {:?}, does not match executable {:?} (possible hash collision)", cache_file, @@ -91,7 +86,7 @@ pub fn get_cache_from_file( pub fn store_cache_in_file( cache_directory: &Path, - executable: &PathBuf, + executable: &Path, environment: &ResolvedPythonEnv, symlinks_with_times: Vec, ) { @@ -120,18 +115,48 @@ pub fn store_cache_in_file( } } -fn generate_hash(executable: &PathBuf) -> String { +fn generate_hash(executable: &Path) -> String { let mut hasher = Sha256::new(); - hasher.update(norm_case(executable).to_string_lossy().as_bytes()); + hasher.update( + executable_cache_key(executable) + .to_string_lossy() + .as_bytes(), + ); let h_bytes = hasher.finalize(); // Convert 256 bits => Hext and then take 16 of the hex chars (that should be unique enough) // We will handle collisions if they happen. format!("{h_bytes:x}")[..16].to_string() } +pub(crate) fn executable_cache_key(executable: &Path) -> PathBuf { + executable_cache_key_with(executable, env::current_dir) +} + +pub(crate) fn executable_cache_key_from(executable: &Path, current_dir: Option<&Path>) -> PathBuf { + if executable.is_absolute() { + norm_case(executable) + } else if let Some(current_dir) = current_dir { + norm_case(current_dir.join(executable)) + } else { + norm_case(executable) + } +} + +fn executable_cache_key_with( + executable: &Path, + current_dir: impl FnOnce() -> io::Result, +) -> PathBuf { + if executable.is_absolute() { + executable_cache_key_from(executable, None) + } else { + executable_cache_key_from(executable, current_dir().ok().as_deref()) + } +} + #[cfg(test)] mod tests { use super::*; + use std::sync::atomic::{AtomicUsize, Ordering}; #[test] #[cfg(unix)] @@ -165,4 +190,34 @@ mod tests { "c3694bfb39d7065b" ); } + + #[test] + fn absolute_cache_key_does_not_query_current_directory() { + let current_dir_calls = AtomicUsize::new(0); + let absolute = std::env::current_dir().unwrap().join("python"); + + let key = executable_cache_key_with(&absolute, || { + current_dir_calls.fetch_add(1, Ordering::Relaxed); + std::env::current_dir() + }); + + assert_eq!(key, norm_case(&absolute)); + assert_eq!(current_dir_calls.load(Ordering::Relaxed), 0); + } + + #[test] + fn relative_and_absolute_aliases_share_cache_file() { + let current_dir = std::env::current_dir().unwrap(); + let relative = PathBuf::from("workspace") + .join(".venv") + .join("bin") + .join("python"); + let absolute = current_dir.join(&relative); + let cache_directory = current_dir.join("cache"); + + assert_eq!( + generate_cache_file(&cache_directory, &relative), + generate_cache_file(&cache_directory, &absolute) + ); + } } From 06c4c792f1073ecdc85dd1cdfe9c934630421279 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Thu, 6 Aug 2026 10:55:59 -0700 Subject: [PATCH 2/3] fix: validate cached aliases in the current context (PR #502) Drop stale relative aliases when a disk cache is reused from another working directory while retaining validated short aliases. Keep the common absolute-cache-hit path free of current-directory lookups. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/pet-python-utils/src/cache.rs | 130 ++++++++++++++++++++++++++- crates/pet-python-utils/src/env.rs | 19 ---- 2 files changed, 127 insertions(+), 22 deletions(-) diff --git a/crates/pet-python-utils/src/cache.rs b/crates/pet-python-utils/src/cache.rs index d6237523..0b1329a3 100644 --- a/crates/pet-python-utils/src/cache.rs +++ b/crates/pet-python-utils/src/cache.rs @@ -6,7 +6,7 @@ use log::{trace, warn}; use std::{ collections::{hash_map::Entry, HashMap, HashSet}, io, - path::PathBuf, + path::{Path, PathBuf}, sync::{Arc, Mutex}, time::SystemTime, }; @@ -25,9 +25,9 @@ lazy_static! { pub trait CacheEntry: Send + Sync { fn get(&self) -> Option; - fn get_for_executable(&self, executable: &std::path::Path) -> Option { + fn get_for_executable(&self, executable: &Path) -> Option { self.get() - .map(|environment| environment.for_executable_alias(executable)) + .map(|environment| bind_environment_to_executable(environment, executable)) } fn store(&self, environment: ResolvedPythonEnv); fn track_symlinks(&self, symlinks: Vec); @@ -147,6 +147,79 @@ fn current_dir_for_aliases(aliases: &[PathBuf]) -> Option { .flatten() } +fn bind_environment_to_executable( + mut environment: ResolvedPythonEnv, + executable: &Path, +) -> ResolvedPythonEnv { + let aliases = environment.symlinks.get_or_insert_with(Vec::new); + if !aliases.iter().any(|alias| alias == executable) { + aliases.push(executable.to_path_buf()); + aliases.sort(); + aliases.dedup(); + } + if environment.executable != executable { + environment.executable = executable.to_path_buf(); + } + environment +} + +fn current_dir_for_cached_aliases( + environment: &ResolvedPythonEnv, + executable: &Path, +) -> Option { + current_dir_for_cached_aliases_with(environment, executable, std::env::current_dir) +} + +fn current_dir_for_cached_aliases_with( + environment: &ResolvedPythonEnv, + executable: &Path, + current_dir: impl FnOnce() -> io::Result, +) -> Option { + environment + .symlinks + .as_ref() + .is_some_and(|aliases| { + aliases + .iter() + .any(|alias| alias.is_relative() && alias != executable) + }) + .then(current_dir) + .transpose() + .ok() + .flatten() +} + +fn bind_validated_environment_to_executable( + mut environment: ResolvedPythonEnv, + executable: &Path, + tracked_aliases: &[FilePathWithMTimeCTime], + current_dir: Option<&Path>, +) -> ResolvedPythonEnv { + let aliases = environment.symlinks.get_or_insert_with(Vec::new); + aliases.retain(|alias| { + if alias == executable { + return true; + } + if tracked_aliases.iter().any(|tracked| tracked.0 == *alias) { + return true; + } + if alias.is_relative() && current_dir.is_none() { + return false; + } + let key = executable_cache_key_from(alias, current_dir); + tracked_aliases.iter().any(|tracked| tracked.0 == key) + }); + if !aliases.iter().any(|alias| alias == executable) { + aliases.push(executable.to_path_buf()); + } + aliases.sort(); + aliases.dedup(); + if environment.executable != executable { + environment.executable = executable.to_path_buf(); + } + environment +} + struct CacheEntryImpl { cache_directory: Option, executable: PathBuf, @@ -229,6 +302,18 @@ impl CacheEntry for CacheEntryImpl { } } + fn get_for_executable(&self, executable: &Path) -> Option { + let environment = self.get()?; + let current_dir = current_dir_for_cached_aliases(&environment, executable); + let tracked_aliases = self.symlinks.lock().expect("symlinks mutex poisoned"); + Some(bind_validated_environment_to_executable( + environment, + executable, + &tracked_aliases, + current_dir.as_deref(), + )) + } + fn store(&self, environment: ResolvedPythonEnv) { // Get hold of the mtimes and ctimes of the symlinks. let aliases = environment.symlinks.clone().unwrap_or_default(); @@ -310,6 +395,7 @@ impl CacheEntry for CacheEntryImpl { #[cfg(test)] mod tests { use super::*; + use std::sync::atomic::{AtomicUsize, Ordering}; use tempfile::tempdir_in; fn environment(executable: PathBuf, aliases: Vec) -> ResolvedPythonEnv { @@ -386,6 +472,44 @@ mod tests { assert!(hit_aliases.contains(&absolute)); } + #[test] + fn stale_relative_alias_from_another_working_directory_is_dropped() { + let (temp_dir, relative, absolute) = aliases(); + let metadata = absolute.metadata().unwrap(); + let tracked_aliases = vec![( + absolute.clone(), + metadata.modified().unwrap(), + metadata.created().ok(), + )]; + let stale_working_directory = temp_dir.path().join("another-workspace"); + + let hit = bind_validated_environment_to_executable( + environment(absolute.clone(), vec![relative.clone(), absolute.clone()]), + &absolute, + &tracked_aliases, + Some(&stale_working_directory), + ); + + let hit_aliases = hit.symlinks.unwrap(); + assert!(!hit_aliases.contains(&relative)); + assert!(hit_aliases.contains(&absolute)); + } + + #[test] + fn absolute_cache_hit_does_not_query_current_directory() { + let (_temp_dir, _relative, absolute) = aliases(); + let current_dir_calls = AtomicUsize::new(0); + let environment = environment(absolute.clone(), vec![absolute.clone()]); + + let current_dir = current_dir_for_cached_aliases_with(&environment, &absolute, || { + current_dir_calls.fetch_add(1, Ordering::Relaxed); + std::env::current_dir() + }); + + assert!(current_dir.is_none()); + assert_eq!(current_dir_calls.load(Ordering::Relaxed), 0); + } + #[test] fn missing_tracked_executable_invalidates_in_memory_entry() { let (temp_dir, _relative, absolute) = aliases(); diff --git a/crates/pet-python-utils/src/env.rs b/crates/pet-python-utils/src/env.rs index 1ac49b6a..83fee1d9 100644 --- a/crates/pet-python-utils/src/env.rs +++ b/crates/pet-python-utils/src/env.rs @@ -41,25 +41,6 @@ pub struct ResolvedPythonEnv { } impl ResolvedPythonEnv { - pub(crate) fn for_executable_alias(mut self, executable: &Path) -> Self { - let alias_is_current = self.executable == executable - && self - .symlinks - .as_ref() - .is_some_and(|aliases| aliases.iter().any(|alias| alias == executable)); - if alias_is_current { - return self; - } - - let mut symlinks = self.symlinks.take().unwrap_or_default(); - symlinks.push(executable.to_path_buf()); - symlinks.sort(); - symlinks.dedup(); - self.executable = executable.to_path_buf(); - self.symlinks = Some(symlinks); - self - } - pub fn to_python_env(&self) -> PythonEnv { let mut env = PythonEnv::new( self.executable.clone(), From 172d887b9cdcb1ee04ebd1521ffa20b883fe7f32 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Thu, 6 Aug 2026 14:38:57 -0700 Subject: [PATCH 3/3] fix: preserve canonical executable on cache hits (PR #502) Keep Python's sys.executable for locator identification while updating only validated caller aliases. This restores macOS python.org classification without changing short user-facing alias selection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/pet-python-utils/src/cache.rs | 40 ++++++++++++++-------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/crates/pet-python-utils/src/cache.rs b/crates/pet-python-utils/src/cache.rs index 0b1329a3..2b206481 100644 --- a/crates/pet-python-utils/src/cache.rs +++ b/crates/pet-python-utils/src/cache.rs @@ -27,7 +27,7 @@ pub trait CacheEntry: Send + Sync { fn get(&self) -> Option; fn get_for_executable(&self, executable: &Path) -> Option { self.get() - .map(|environment| bind_environment_to_executable(environment, executable)) + .map(|environment| add_executable_alias(environment, executable)) } fn store(&self, environment: ResolvedPythonEnv); fn track_symlinks(&self, symlinks: Vec); @@ -147,7 +147,7 @@ fn current_dir_for_aliases(aliases: &[PathBuf]) -> Option { .flatten() } -fn bind_environment_to_executable( +fn add_executable_alias( mut environment: ResolvedPythonEnv, executable: &Path, ) -> ResolvedPythonEnv { @@ -157,9 +157,6 @@ fn bind_environment_to_executable( aliases.sort(); aliases.dedup(); } - if environment.executable != executable { - environment.executable = executable.to_path_buf(); - } environment } @@ -189,7 +186,7 @@ fn current_dir_for_cached_aliases_with( .flatten() } -fn bind_validated_environment_to_executable( +fn bind_validated_executable_alias( mut environment: ResolvedPythonEnv, executable: &Path, tracked_aliases: &[FilePathWithMTimeCTime], @@ -214,9 +211,6 @@ fn bind_validated_environment_to_executable( } aliases.sort(); aliases.dedup(); - if environment.executable != executable { - environment.executable = executable.to_path_buf(); - } environment } @@ -306,7 +300,7 @@ impl CacheEntry for CacheEntryImpl { let environment = self.get()?; let current_dir = current_dir_for_cached_aliases(&environment, executable); let tracked_aliases = self.symlinks.lock().expect("symlinks mutex poisoned"); - Some(bind_validated_environment_to_executable( + Some(bind_validated_executable_alias( environment, executable, &tracked_aliases, @@ -429,36 +423,41 @@ mod tests { } #[test] - fn cache_hit_uses_current_alias_and_preserves_shorter_aliases() { - let (_temp_dir, relative, absolute) = aliases(); + fn cache_hit_preserves_canonical_executable_and_current_aliases() { + let (temp_dir, relative, absolute) = aliases(); + let canonical = temp_dir.path().join("canonical-python"); + std::fs::write(&canonical, "python").unwrap(); let cache = CacheImpl::new(None); let entry = cache.create_cache(relative.clone()); let entry = entry.lock().unwrap(); entry.store(environment( - relative.clone(), - vec![relative.clone(), absolute.clone()], + canonical.clone(), + vec![relative.clone(), absolute.clone(), canonical.clone()], )); let relative_hit = entry.get_for_executable(&relative).unwrap(); - assert_eq!(relative_hit.executable, relative); + assert_eq!(relative_hit.executable, canonical); let absolute_hit = entry.get_for_executable(&absolute).unwrap(); - assert_eq!(absolute_hit.executable, absolute); + assert_eq!(absolute_hit.executable, canonical); let hit_aliases = absolute_hit.symlinks.unwrap(); assert!(hit_aliases.contains(&relative)); assert!(hit_aliases.contains(&absolute)); + assert!(hit_aliases.contains(&canonical)); } #[test] fn disk_cache_reuses_relative_entry_for_absolute_alias() { let (temp_dir, relative, absolute) = aliases(); + let canonical = temp_dir.path().join("canonical-python"); + std::fs::write(&canonical, "python").unwrap(); let cache_directory = temp_dir.path().join("cache"); { let cache = CacheImpl::new(Some(cache_directory.clone())); let entry = cache.create_cache(relative.clone()); entry.lock().unwrap().store(environment( - relative.clone(), - vec![relative.clone(), absolute.clone()], + canonical.clone(), + vec![relative.clone(), absolute.clone(), canonical.clone()], )); } @@ -466,10 +465,11 @@ mod tests { let entry = cache.create_cache(absolute.clone()); let hit = entry.lock().unwrap().get_for_executable(&absolute).unwrap(); - assert_eq!(hit.executable, absolute); + assert_eq!(hit.executable, canonical); let hit_aliases = hit.symlinks.unwrap(); assert!(hit_aliases.contains(&relative)); assert!(hit_aliases.contains(&absolute)); + assert!(hit_aliases.contains(&canonical)); } #[test] @@ -483,7 +483,7 @@ mod tests { )]; let stale_working_directory = temp_dir.path().join("another-workspace"); - let hit = bind_validated_environment_to_executable( + let hit = bind_validated_executable_alias( environment(absolute.clone(), vec![relative.clone(), absolute.clone()]), &absolute, &tracked_aliases,