From 0dc82d8aa9afcc0cbeaf128f0d04fefea24e5b4f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 08:23:48 +0000 Subject: [PATCH 1/4] Start fix for #1888 Assisted-by: Claude Code:claude-opus-5-5 From 6fbb64b9e1d99fad5052095d752d7adf883ec3bf Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 08:30:31 +0000 Subject: [PATCH 2/4] Test python site memo keeps env-moved answer Red test for #1888: the Python site query memo files an answer under the old environment key even when the environment changed while the query ran, so a later ask in the old environment is served a stale interpreter answer. Assisted-by: Claude Code:claude-opus-5-5 --- .../src/crawlers/python_crawler.rs | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/crates/socket-patch-core/src/crawlers/python_crawler.rs b/crates/socket-patch-core/src/crawlers/python_crawler.rs index fa6083566..d8a8915d4 100644 --- a/crates/socket-patch-core/src/crawlers/python_crawler.rs +++ b/crates/socket-patch-core/src/crawlers/python_crawler.rs @@ -8094,6 +8094,32 @@ G= assert_eq!(runs.load(Ordering::SeqCst), 5); } + /// The site query memo keeps an answer only if the environment it ran + /// under is still the one it was keyed on: an answer computed while the + /// environment changed (another thread swapped `PATH`) is handed back + /// but not filed under the old key, so the next ask runs again. + #[test] + #[serial_test::serial] + fn site_query_memo_drops_an_answer_the_environment_changed_under() { + const VAR: &str = "SOCKET_PATCH_TEST_SITE_QUERY_KEEP_RULE"; + std::env::remove_var(VAR); + let memo: KeyedMemo = KeyedMemo::new(); + let runs = std::cell::Cell::new(0); + let answer = memo.get_or_run(site_query_key(), || { + runs.set(runs.get() + 1); + std::env::set_var(VAR, "moved"); + Some("ran under a moved environment".to_string()) + }); + std::env::remove_var(VAR); + assert_eq!(answer.as_deref(), Some("ran under a moved environment")); + let again = memo.get_or_run(site_query_key(), || { + runs.set(runs.get() + 1); + Some("fresh".to_string()) + }); + assert_eq!(again.as_deref(), Some("fresh")); + assert_eq!(runs.get(), 2); + } + /// DC-6: the memoized site query is wired to the whole environment. /// Under two different `PYTHONUSERBASE` values (the interpreter's /// `getusersitepackages` follows it) the memoized answer equals the From 9ff63ee5225b240d93d263a01903a8e73df60dea Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 08:37:35 +0000 Subject: [PATCH 3/4] Share one env-keyed probe memo across crawlers Composer, Python and RubyGems discovery each cached a probe answer (composer global home, the Python site query, gem env) keyed on the process environment, with three copies of the key and two memo types. The Python copy kept an answer even when the environment changed while the probe ran, so a later ask could get an interpreter answer from the wrong environment. All three now use utils::process::EnvKeyedMemo and env_key, which keep an answer only if the environment is unchanged after the probe. Ruby keeps its per-environment single-flight cells on the shared key. Fixes #1888 Assisted-by: Claude Code:claude-opus-5-5 --- .../src/crawlers/composer_crawler.rs | 170 +---------------- .../src/crawlers/python_crawler.rs | 114 ++--------- .../src/crawlers/ruby_crawler.rs | 29 +-- crates/socket-patch-core/src/utils/process.rs | 180 ++++++++++++++++++ 4 files changed, 200 insertions(+), 293 deletions(-) diff --git a/crates/socket-patch-core/src/crawlers/composer_crawler.rs b/crates/socket-patch-core/src/crawlers/composer_crawler.rs index 0e7143238..99d65d71d 100644 --- a/crates/socket-patch-core/src/crawlers/composer_crawler.rs +++ b/crates/socket-patch-core/src/crawlers/composer_crawler.rs @@ -7,7 +7,7 @@ use super::types::{CrawledPackage, CrawlerOptions}; use crate::patch::path_safety; use crate::utils::composer_version::composer_versions_equivalent; use crate::utils::fs::{is_dir, is_dir_sync, is_file, run_blocking}; -use crate::utils::process::{CommandRunner, GlobalProbeRunner}; +use crate::utils::process::{CommandRunner, EnvKeyedMemo, GlobalProbeRunner}; use crate::utils::relpath::normalize_lexically; #[cfg(test)] @@ -474,85 +474,10 @@ pub fn parse_composer_home_output(stdout: &str) -> Option { } } -/// A process-wide memo of one subprocess answer, reused only while the -/// whole environment (every variable, and the working directory) is exactly -/// what it was when the answer was produced — the subprocess's only inputs -/// the CLI controls. A different environment re-runs it and replaces the -/// entry; concurrent first callers may each run it. -/// -/// Only an answer is kept. A FAILED probe (spawn error under fd pressure, -/// a non-zero exit, empty stdout) is asked again by the next caller — -/// global discovery runs at least twice per command (the crawl, then the -/// package-lookup pass), so a transient first failure must not stick. So -/// is an answer the environment changed under, which the key would -/// misfile. Same rule as the `gem env` -/// memo in `ruby_crawler`. -struct EnvKeyedMemo { - slot: std::sync::Mutex>, -} - -type EnvKey = ( - Vec<(std::ffi::OsString, std::ffi::OsString)>, - Option, -); - -/// What the subprocess inherits: the (sorted) environment and the working -/// directory. Sorted so a `remove_var`/`set_var` round-trip that restores -/// an identical environment still matches (`vars_os` yields `environ`'s -/// order, which such a round-trip permutes) — the same key `gem_env_key` -/// builds. -fn env_key() -> EnvKey { - let mut vars: Vec<_> = std::env::vars_os().collect(); - vars.sort(); - (vars, std::env::current_dir().ok()) -} - -impl EnvKeyedMemo { - const fn new() -> Self { - Self { - slot: std::sync::Mutex::new(None), - } - } - - fn get_or_run(&self, run: impl FnOnce() -> Option) -> Option { - let key = env_key(); - let ran_under = key.clone(); - self.get_or_run_keyed(key, move || { - let answer = run(); - // Keepable only if the environment the subprocess actually ran - // under is still the one we keyed on. - let env_unchanged = env_key() == ran_under; - (answer, env_unchanged) - }) - } - - /// The memoized ask. `run` hands back its answer and whether that - /// answer is keepable; a `None` answer is never kept. Split from - /// [`Self::get_or_run`] so tests can drive both against a key of their - /// own. - fn get_or_run_keyed( - &self, - key: EnvKey, - run: impl FnOnce() -> (Option, bool), - ) -> Option { - let lock = || self.slot.lock().unwrap_or_else(|e| e.into_inner()); - if let Some((cached_key, answer)) = lock().as_ref() { - if *cached_key == key { - return Some(answer.clone()); - } - } - let (answer, keepable) = run(); - if let (Some(answer), true) = (&answer, keepable) { - *lock() = Some((key, answer.clone())); - } - answer - } -} - /// `composer global config home` boots PHP and Composer (0.3-1 s), and /// global discovery asks for it on every pass (the crawl, then each /// package-lookup pass of the same command). -static COMPOSER_GLOBAL_HOME: EnvKeyedMemo = EnvKeyedMemo::new(); +static COMPOSER_GLOBAL_HOME: EnvKeyedMemo = EnvKeyedMemo::new(); /// Get the Composer home directory. /// @@ -2478,96 +2403,5 @@ mod tests { "vacuous fixtures: {crawled}/{found}" ); } - - /// The `composer global config home` memo re-runs only when the - /// environment changed. - #[test] - fn env_keyed_memo_reuses_only_an_identical_environment() { - use std::ffi::OsString; - let env = |path: &str| -> EnvKey { - ( - vec![(OsString::from("PATH"), OsString::from(path))], - Some(PathBuf::from("/cwd")), - ) - }; - let memo = EnvKeyedMemo::new(); - let mut runs = 0; - let mut run = |answer: Option<&str>| { - runs += 1; - (answer.map(str::to_string), true) - }; - assert_eq!( - memo.get_or_run_keyed(env("/a"), || run(Some("home"))), - Some("home".into()) - ); - assert_eq!( - memo.get_or_run_keyed(env("/a"), || run(Some("x"))), - Some("home".into()) - ); - // A different environment misses and replaces the entry. - assert_eq!( - memo.get_or_run_keyed(env("/b"), || run(Some("other"))), - Some("other".into()) - ); - assert_eq!( - memo.get_or_run_keyed(env("/b"), || run(Some("x"))), - Some("other".into()) - ); - assert_eq!( - memo.get_or_run_keyed(env("/a"), || run(Some("again"))), - Some("again".into()) - ); - assert_eq!(runs, 3); - } - - /// A failed probe is not kept: the asker gets its own `None` and - /// the next caller asks again, as every caller did before the memo - /// (global discovery runs the probe once for the crawl and once for - /// the package-lookup pass). Neither is an answer the environment - /// changed under. - #[test] - fn env_keyed_memo_asks_again_after_a_failed_probe() { - use std::ffi::OsString; - let key: EnvKey = ( - vec![(OsString::from("PATH"), OsString::from("/a"))], - Some(PathBuf::from("/cwd")), - ); - let memo = EnvKeyedMemo::new(); - let runs = std::cell::Cell::new(0); - let run = |answer: Option<&str>, keepable: bool| { - runs.set(runs.get() + 1); - (answer.map(str::to_string), keepable) - }; - - // A failure, then an answer whose environment moved: both are - // handed back, neither is kept. - assert_eq!(memo.get_or_run_keyed(key.clone(), || run(None, true)), None); - assert_eq!( - memo.get_or_run_keyed(key.clone(), || run(Some("moved"), false)), - Some("moved".into()) - ); - assert_eq!(runs.get(), 2); - - // The next caller still asks, and a complete answer is kept. - assert_eq!( - memo.get_or_run_keyed(key.clone(), || run(Some("home"), true)), - Some("home".into()) - ); - assert_eq!( - memo.get_or_run_keyed(key.clone(), || run(None, true)), - Some("home".into()) - ); - assert_eq!(runs.get(), 3); - } - - /// The memo key is order-insensitive: an environment restored to - /// the same variables still hits, whatever order `vars_os` yields. - #[test] - fn env_key_is_sorted() { - let key = super::super::env_key(); - let mut sorted = key.0.clone(); - sorted.sort(); - assert_eq!(key.0, sorted); - } } } diff --git a/crates/socket-patch-core/src/crawlers/python_crawler.rs b/crates/socket-patch-core/src/crawlers/python_crawler.rs index d8a8915d4..a45496ca4 100644 --- a/crates/socket-patch-core/src/crawlers/python_crawler.rs +++ b/crates/socket-patch-core/src/crawlers/python_crawler.rs @@ -6,7 +6,7 @@ use super::types::{CrawledPackage, CrawlerOptions}; use crate::utils::fs::{ is_dir, is_dir_sync, read_regular_to_string, read_regular_to_string_sync, run_blocking, }; -use crate::utils::process::{CommandRunner, SystemCommandRunner}; +use crate::utils::process::{CommandRunner, EnvKeyedMemo, SystemCommandRunner}; #[cfg(test)] mod oracle; @@ -2762,59 +2762,14 @@ fn pipenv_workon_home_venvs( found } -/// What the interpreter query below depends on besides the interpreter -/// itself: the whole process environment (`PATH` picks the interpreter; -/// `HOME`, `PYTHONUSERBASE`, `PYTHONHOME`, … shape its answer) and the -/// working directory. -type SiteQueryKey = ( - Vec<(std::ffi::OsString, std::ffi::OsString)>, - Option, -); - -fn site_query_key() -> SiteQueryKey { - let mut env: Vec<_> = std::env::vars_os().collect(); - env.sort(); - (env, std::env::current_dir().ok()) -} - -/// The last successful `(key, answer)` of an expensive query, re-run -/// whenever the key differs. A failed query (`None`) is never stored, so the -/// next ask runs it again exactly as an unmemoized caller would: a transient -/// spawn failure during the crawl cannot blank the vendor phase's answer. -struct KeyedMemo { - slot: std::sync::Mutex>, -} - -impl KeyedMemo { - const fn new() -> Self { - Self { - slot: std::sync::Mutex::new(None), - } - } - - fn get_or_run(&self, key: K, run: impl FnOnce() -> Option) -> Option { - let lock = || { - self.slot - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - }; - if let Some((cached, value)) = lock().as_ref() { - if *cached == key { - return Some(value.clone()); - } - } - let value = run()?; - *lock() = Some((key, value.clone())); - Some(value) - } -} - /// The interpreter's site-packages answer (the `find_python_command` probe -/// plus the `site` query — two process spawns), per [`SiteQueryKey`]: a +/// plus the `site` query — two process spawns), per environment (`PATH` +/// picks the interpreter; `HOME`, `PYTHONUSERBASE`, `PYTHONHOME`, … shape +/// its answer; see [`EnvKeyedMemo`]): a /// scan asks it once for its crawl and again when it vendors, in the same /// environment, so the second ask re-spawned both processes for the same -/// output. Only a successful answer is kept (see [`KeyedMemo`]). -static SITE_QUERY_MEMO: KeyedMemo = KeyedMemo::new(); +/// output. +static SITE_QUERY_MEMO: EnvKeyedMemo = EnvKeyedMemo::new(); /// The unmemoized interpreter query: probe for a python and ask it for its /// site-packages. [`SITE_QUERY_MEMO`] wraps this; tests call it directly as @@ -2853,8 +2808,7 @@ pub async fn get_global_python_site_packages() -> Vec { } // 1. Ask Python for site-packages (subprocesses: on the blocking pool) - let site_output = - run_blocking(|| SITE_QUERY_MEMO.get_or_run(site_query_key(), run_site_query)).await; + let site_output = run_blocking(|| SITE_QUERY_MEMO.get_or_run(run_site_query)).await; if let Some(stdout) = site_output { for p in parse_python_site_packages_output(&stdout) { add_path(p, &mut seen, &mut results); @@ -8048,52 +8002,6 @@ G= } } - /// DC-6: the interpreter query memo answers from its slot only for the - /// exact key it was filled under, re-runs on any other key (a changed - /// environment or working directory), and never keeps a failed query's - /// `None`: the next ask with the same key runs the query again, as an - /// unmemoized caller would. - #[test] - fn keyed_memo_reruns_only_on_a_different_key() { - use std::sync::atomic::{AtomicUsize, Ordering}; - let memo: KeyedMemo<(u32, &str), String> = KeyedMemo::new(); - let runs = AtomicUsize::new(0); - let run = |out: Option<&str>| { - runs.fetch_add(1, Ordering::SeqCst); - out.map(str::to_string) - }; - assert_eq!( - memo.get_or_run((1, "a"), || run(Some("x"))), - Some("x".into()) - ); - assert_eq!( - memo.get_or_run((1, "a"), || run(Some("stale"))), - Some("x".into()) - ); - assert_eq!(runs.load(Ordering::SeqCst), 1); - // A failure is returned but not stored: the same key re-runs. - assert_eq!(memo.get_or_run((2, "a"), || run(None)), None); - assert_eq!(runs.load(Ordering::SeqCst), 2); - assert_eq!( - memo.get_or_run((2, "a"), || run(Some("y"))), - Some("y".into()) - ); - assert_eq!(runs.load(Ordering::SeqCst), 3); - assert_eq!( - memo.get_or_run((2, "a"), || run(Some("stale"))), - Some("y".into()) - ); - assert_eq!(runs.load(Ordering::SeqCst), 3); - // A failure under a new key leaves nothing cached for the old one - // to be served from, and the old key re-runs. - assert_eq!(memo.get_or_run((3, "a"), || run(None)), None); - assert_eq!( - memo.get_or_run((1, "a"), || run(Some("z"))), - Some("z".into()) - ); - assert_eq!(runs.load(Ordering::SeqCst), 5); - } - /// The site query memo keeps an answer only if the environment it ran /// under is still the one it was keyed on: an answer computed while the /// environment changed (another thread swapped `PATH`) is handed back @@ -8103,16 +8011,16 @@ G= fn site_query_memo_drops_an_answer_the_environment_changed_under() { const VAR: &str = "SOCKET_PATCH_TEST_SITE_QUERY_KEEP_RULE"; std::env::remove_var(VAR); - let memo: KeyedMemo = KeyedMemo::new(); + let memo: EnvKeyedMemo = EnvKeyedMemo::new(); let runs = std::cell::Cell::new(0); - let answer = memo.get_or_run(site_query_key(), || { + let answer = memo.get_or_run(|| { runs.set(runs.get() + 1); std::env::set_var(VAR, "moved"); Some("ran under a moved environment".to_string()) }); std::env::remove_var(VAR); assert_eq!(answer.as_deref(), Some("ran under a moved environment")); - let again = memo.get_or_run(site_query_key(), || { + let again = memo.get_or_run(|| { runs.set(runs.get() + 1); Some("fresh".to_string()) }); @@ -8149,7 +8057,7 @@ G= } } } - let memoized = || SITE_QUERY_MEMO.get_or_run(site_query_key(), run_site_query); + let memoized = || SITE_QUERY_MEMO.get_or_run(run_site_query); let base_a = tempfile::tempdir().unwrap(); let base_b = tempfile::tempdir().unwrap(); diff --git a/crates/socket-patch-core/src/crawlers/ruby_crawler.rs b/crates/socket-patch-core/src/crawlers/ruby_crawler.rs index f2643b0b5..ed2c63057 100644 --- a/crates/socket-patch-core/src/crawlers/ruby_crawler.rs +++ b/crates/socket-patch-core/src/crawlers/ruby_crawler.rs @@ -5,7 +5,7 @@ use std::path::{Path, PathBuf}; use super::types::{CrawledPackage, CrawlerOptions}; use crate::patch::path_safety; use crate::utils::fs::{entry_is_dir, home_dir, is_dir, is_file, list_dir_entries, run_blocking}; -use crate::utils::process::{CommandRunner, SystemCommandRunner}; +use crate::utils::process::{env_key, CommandRunner, EnvKey, SystemCommandRunner}; use crate::utils::relpath::normalize_lexically; use crate::vendor::lock_inventory::ProjectView; @@ -1013,11 +1013,11 @@ impl RubyCrawler { async fn gem_env_homes() -> GemEnvHomes { static MEMO: once_cell::sync::Lazy = once_cell::sync::Lazy::new(Default::default); - let key = gem_env_key(); + let key = env_key(); let cell = gem_env_cell(&MEMO, key.clone()); memoize_gem_env_homes(&cell, || async move { let homes = tokio::join!(Self::run_gem_env("gemdir"), Self::run_gem_env("gempath")); - let env_unchanged = gem_env_key() == key; + let env_unchanged = env_key() == key; (homes, env_unchanged) }) .await @@ -1625,26 +1625,14 @@ pub fn parse_gem_env_output(stdout: &str) -> Option { /// `(gemdir, gempath)` as [`parse_gem_env_output`] reads each `gem env` answer. type GemEnvHomes = (Option, Option); -/// What a `gem env` subprocess inherits: the (sorted) environment and the -/// working directory. -type GemEnvKey = ( - Vec<(std::ffi::OsString, std::ffi::OsString)>, - Option, -); - +/// One single-flight cell per inherited environment ([`EnvKey`]). type GemEnvMemo = - std::sync::Mutex>>>; - -fn gem_env_key() -> GemEnvKey { - let mut vars: Vec<_> = std::env::vars_os().collect(); - vars.sort(); - (vars, std::env::current_dir().ok()) -} + std::sync::Mutex>>>; /// The memo cell for `key`, created empty on first sight. fn gem_env_cell( memo: &GemEnvMemo, - key: GemEnvKey, + key: EnvKey, ) -> std::sync::Arc> { let mut memo = memo.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); std::sync::Arc::clone(memo.entry(key).or_default()) @@ -5755,7 +5743,7 @@ mod tests { #[test] fn gem_env_cells_are_keyed_on_the_inherited_environment() { let memo = GemEnvMemo::default(); - let key = |path: &str| -> GemEnvKey { + let key = |path: &str| -> EnvKey { ( vec![("PATH".into(), path.into())], Some(PathBuf::from("/work")), @@ -5766,9 +5754,6 @@ mod tests { let swapped = gem_env_cell(&memo, key("/tmp/fake-bin")); assert!(std::sync::Arc::ptr_eq(&a, &again)); assert!(!std::sync::Arc::ptr_eq(&a, &swapped)); - let (vars, cwd) = gem_env_key(); - assert!(vars.windows(2).all(|w| w[0] <= w[1]), "sorted env snapshot"); - assert_eq!(cwd, std::env::current_dir().ok()); } // ── gem env gempath splitting (OS path separator) ───────────── diff --git a/crates/socket-patch-core/src/utils/process.rs b/crates/socket-patch-core/src/utils/process.rs index 2e5e12812..59824ef4a 100644 --- a/crates/socket-patch-core/src/utils/process.rs +++ b/crates/socket-patch-core/src/utils/process.rs @@ -339,10 +339,190 @@ fn run_resolved_within( } } +/// What a probe subprocess inherits: the (sorted) environment and the +/// working directory. Sorted so a `remove_var`/`set_var` round-trip that +/// restores an identical environment still matches (`vars_os` yields +/// `environ`'s order, which such a round-trip permutes). +pub(crate) type EnvKey = (Vec<(OsString, OsString)>, Option); + +/// The [`EnvKey`] of the process right now. +pub(crate) fn env_key() -> EnvKey { + let mut vars: Vec<_> = std::env::vars_os().collect(); + vars.sort(); + (vars, std::env::current_dir().ok()) +} + +/// A process-wide memo of one subprocess answer, reused only while the +/// whole environment (every variable, and the working directory) is exactly +/// what it was when the answer was produced: the subprocess's only inputs +/// the CLI controls. A different environment re-runs it and replaces the +/// entry; concurrent first callers may each run it. +/// +/// Only an answer is kept. A failed probe (spawn error under fd pressure, +/// a non-zero exit, empty stdout) is asked again by the next caller, so a +/// transient first failure doesn't stick. Neither is an answer the +/// environment changed under while it ran, which the key would misfile. +pub(crate) struct EnvKeyedMemo { + slot: std::sync::Mutex>, +} + +impl EnvKeyedMemo { + pub(crate) const fn new() -> Self { + Self { + slot: std::sync::Mutex::new(None), + } + } + + pub(crate) fn get_or_run(&self, run: impl FnOnce() -> Option) -> Option { + let key = env_key(); + let ran_under = key.clone(); + self.get_or_run_keyed(key, move || { + let answer = run(); + // Keepable only if the environment the subprocess actually ran + // under is still the one we keyed on. + (answer, env_key() == ran_under) + }) + } + + /// The memoized ask. `run` hands back its answer and whether that + /// answer is keepable; a `None` answer is never kept. Split from + /// [`Self::get_or_run`] so tests can drive it against a key of their + /// own. + fn get_or_run_keyed(&self, key: EnvKey, run: impl FnOnce() -> (Option, bool)) -> Option { + let lock = || { + self.slot + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + }; + if let Some((cached_key, answer)) = lock().as_ref() { + if *cached_key == key { + return Some(answer.clone()); + } + } + let (answer, keepable) = run(); + if let (Some(answer), true) = (&answer, keepable) { + *lock() = Some((key, answer.clone())); + } + answer + } +} + #[cfg(test)] pub(crate) mod tests { use super::*; + /// The memo re-runs only when the environment changed. + #[test] + fn env_keyed_memo_reuses_only_an_identical_environment() { + let env = |path: &str| -> EnvKey { + ( + vec![(OsString::from("PATH"), OsString::from(path))], + Some(PathBuf::from("/cwd")), + ) + }; + let memo = EnvKeyedMemo::new(); + let mut runs = 0; + let mut run = |answer: Option<&str>| { + runs += 1; + (answer.map(str::to_string), true) + }; + assert_eq!( + memo.get_or_run_keyed(env("/a"), || run(Some("home"))), + Some("home".into()) + ); + assert_eq!( + memo.get_or_run_keyed(env("/a"), || run(Some("x"))), + Some("home".into()) + ); + // A different environment misses and replaces the entry. + assert_eq!( + memo.get_or_run_keyed(env("/b"), || run(Some("other"))), + Some("other".into()) + ); + assert_eq!( + memo.get_or_run_keyed(env("/b"), || run(Some("x"))), + Some("other".into()) + ); + assert_eq!( + memo.get_or_run_keyed(env("/a"), || run(Some("again"))), + Some("again".into()) + ); + assert_eq!(runs, 3); + } + + /// A failed probe is not kept: the asker gets its own `None` and the + /// next caller asks again. Neither is an answer the environment + /// changed under. + #[test] + fn env_keyed_memo_asks_again_after_a_failed_probe() { + let key: EnvKey = ( + vec![(OsString::from("PATH"), OsString::from("/a"))], + Some(PathBuf::from("/cwd")), + ); + let memo = EnvKeyedMemo::new(); + let runs = std::cell::Cell::new(0); + let run = |answer: Option<&str>, keepable: bool| { + runs.set(runs.get() + 1); + (answer.map(str::to_string), keepable) + }; + + // A failure, then an answer whose environment moved: both are + // handed back, neither is kept. + assert_eq!(memo.get_or_run_keyed(key.clone(), || run(None, true)), None); + assert_eq!( + memo.get_or_run_keyed(key.clone(), || run(Some("moved"), false)), + Some("moved".into()) + ); + assert_eq!(runs.get(), 2); + + // The next caller still asks, and a complete answer is kept. + assert_eq!( + memo.get_or_run_keyed(key.clone(), || run(Some("home"), true)), + Some("home".into()) + ); + assert_eq!( + memo.get_or_run_keyed(key.clone(), || run(None, true)), + Some("home".into()) + ); + assert_eq!(runs.get(), 3); + } + + /// The keep rule end to end: an answer computed while the real process + /// environment changed is handed back but not kept, so the next ask + /// in the original environment runs again. + #[test] + #[serial_test::serial] + fn env_keyed_memo_drops_an_answer_the_environment_changed_under() { + const VAR: &str = "SOCKET_PATCH_TEST_ENV_KEYED_MEMO_KEEP_RULE"; + std::env::remove_var(VAR); + let memo = EnvKeyedMemo::new(); + let runs = std::cell::Cell::new(0); + let answer = memo.get_or_run(|| { + runs.set(runs.get() + 1); + std::env::set_var(VAR, "moved"); + Some("moved".to_string()) + }); + std::env::remove_var(VAR); + assert_eq!(answer.as_deref(), Some("moved")); + let fresh = || { + runs.set(runs.get() + 1); + Some("fresh".to_string()) + }; + assert_eq!(memo.get_or_run(fresh).as_deref(), Some("fresh")); + // Now kept: an unchanged environment is served from the slot. + assert_eq!(memo.get_or_run(|| None).as_deref(), Some("fresh")); + assert_eq!(runs.get(), 2); + } + + /// The memo key is order-insensitive: an environment restored to the + /// same variables still hits, whatever order `vars_os` yields. + #[test] + fn env_key_is_sorted() { + let (vars, cwd) = env_key(); + assert!(vars.windows(2).all(|w| w[0] <= w[1]), "sorted env snapshot"); + assert_eq!(cwd, std::env::current_dir().ok()); + } + /// Confirm the real runner returns Some for a tiny command we /// know is on every Unix PATH — `echo`. Skipped on Windows where /// `echo` isn't a real binary. From baa250a6be363d365d384f0c7dd3f3e99c27214a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 08:59:04 +0000 Subject: [PATCH 4/4] Address review on the shared probe memo Skip the environment clone on a cache hit: the key is passed to the probe only on a miss. Drop the Python copy of the keep-rule test (the shared helper's test covers it, and SITE_QUERY_MEMO is that type), and drop test assertions that raced other tests changing the environment or working directory. Assisted-by: Claude Code:claude-opus-5-5 --- .../src/crawlers/python_crawler.rs | 26 --------- crates/socket-patch-core/src/utils/process.rs | 57 ++++++++++--------- 2 files changed, 30 insertions(+), 53 deletions(-) diff --git a/crates/socket-patch-core/src/crawlers/python_crawler.rs b/crates/socket-patch-core/src/crawlers/python_crawler.rs index a45496ca4..bdacc4ad5 100644 --- a/crates/socket-patch-core/src/crawlers/python_crawler.rs +++ b/crates/socket-patch-core/src/crawlers/python_crawler.rs @@ -8002,32 +8002,6 @@ G= } } - /// The site query memo keeps an answer only if the environment it ran - /// under is still the one it was keyed on: an answer computed while the - /// environment changed (another thread swapped `PATH`) is handed back - /// but not filed under the old key, so the next ask runs again. - #[test] - #[serial_test::serial] - fn site_query_memo_drops_an_answer_the_environment_changed_under() { - const VAR: &str = "SOCKET_PATCH_TEST_SITE_QUERY_KEEP_RULE"; - std::env::remove_var(VAR); - let memo: EnvKeyedMemo = EnvKeyedMemo::new(); - let runs = std::cell::Cell::new(0); - let answer = memo.get_or_run(|| { - runs.set(runs.get() + 1); - std::env::set_var(VAR, "moved"); - Some("ran under a moved environment".to_string()) - }); - std::env::remove_var(VAR); - assert_eq!(answer.as_deref(), Some("ran under a moved environment")); - let again = memo.get_or_run(|| { - runs.set(runs.get() + 1); - Some("fresh".to_string()) - }); - assert_eq!(again.as_deref(), Some("fresh")); - assert_eq!(runs.get(), 2); - } - /// DC-6: the memoized site query is wired to the whole environment. /// Under two different `PYTHONUSERBASE` values (the interpreter's /// `getusersitepackages` follows it) the memoized answer equals the diff --git a/crates/socket-patch-core/src/utils/process.rs b/crates/socket-patch-core/src/utils/process.rs index 59824ef4a..1ed58482b 100644 --- a/crates/socket-patch-core/src/utils/process.rs +++ b/crates/socket-patch-core/src/utils/process.rs @@ -360,8 +360,9 @@ pub(crate) fn env_key() -> EnvKey { /// /// Only an answer is kept. A failed probe (spawn error under fd pressure, /// a non-zero exit, empty stdout) is asked again by the next caller, so a -/// transient first failure doesn't stick. Neither is an answer the -/// environment changed under while it ran, which the key would misfile. +/// transient first failure doesn't stick. Neither is an answer whose +/// environment differs, when the subprocess returns, from the one it was +/// keyed on: the key would misfile it. pub(crate) struct EnvKeyedMemo { slot: std::sync::Mutex>, } @@ -374,21 +375,23 @@ impl EnvKeyedMemo { } pub(crate) fn get_or_run(&self, run: impl FnOnce() -> Option) -> Option { - let key = env_key(); - let ran_under = key.clone(); - self.get_or_run_keyed(key, move || { + self.get_or_run_keyed(env_key(), |ran_under| { let answer = run(); - // Keepable only if the environment the subprocess actually ran - // under is still the one we keyed on. - (answer, env_key() == ran_under) + // Keepable only if the environment is, once the subprocess + // returns, still the one we keyed on. + (answer, env_key() == *ran_under) }) } - /// The memoized ask. `run` hands back its answer and whether that - /// answer is keepable; a `None` answer is never kept. Split from - /// [`Self::get_or_run`] so tests can drive it against a key of their - /// own. - fn get_or_run_keyed(&self, key: EnvKey, run: impl FnOnce() -> (Option, bool)) -> Option { + /// The memoized ask. On a miss, `run` gets the key it runs under and + /// hands back its answer and whether that answer is keepable; a `None` + /// answer is never kept. Split from [`Self::get_or_run`] so tests can + /// drive it against a key of their own. + fn get_or_run_keyed( + &self, + key: EnvKey, + run: impl FnOnce(&EnvKey) -> (Option, bool), + ) -> Option { let lock = || { self.slot .lock() @@ -399,7 +402,7 @@ impl EnvKeyedMemo { return Some(answer.clone()); } } - let (answer, keepable) = run(); + let (answer, keepable) = run(&key); if let (Some(answer), true) = (&answer, keepable) { *lock() = Some((key, answer.clone())); } @@ -427,24 +430,24 @@ pub(crate) mod tests { (answer.map(str::to_string), true) }; assert_eq!( - memo.get_or_run_keyed(env("/a"), || run(Some("home"))), + memo.get_or_run_keyed(env("/a"), |_| run(Some("home"))), Some("home".into()) ); assert_eq!( - memo.get_or_run_keyed(env("/a"), || run(Some("x"))), + memo.get_or_run_keyed(env("/a"), |_| run(Some("x"))), Some("home".into()) ); // A different environment misses and replaces the entry. assert_eq!( - memo.get_or_run_keyed(env("/b"), || run(Some("other"))), + memo.get_or_run_keyed(env("/b"), |_| run(Some("other"))), Some("other".into()) ); assert_eq!( - memo.get_or_run_keyed(env("/b"), || run(Some("x"))), + memo.get_or_run_keyed(env("/b"), |_| run(Some("x"))), Some("other".into()) ); assert_eq!( - memo.get_or_run_keyed(env("/a"), || run(Some("again"))), + memo.get_or_run_keyed(env("/a"), |_| run(Some("again"))), Some("again".into()) ); assert_eq!(runs, 3); @@ -468,20 +471,23 @@ pub(crate) mod tests { // A failure, then an answer whose environment moved: both are // handed back, neither is kept. - assert_eq!(memo.get_or_run_keyed(key.clone(), || run(None, true)), None); assert_eq!( - memo.get_or_run_keyed(key.clone(), || run(Some("moved"), false)), + memo.get_or_run_keyed(key.clone(), |_| run(None, true)), + None + ); + assert_eq!( + memo.get_or_run_keyed(key.clone(), |_| run(Some("moved"), false)), Some("moved".into()) ); assert_eq!(runs.get(), 2); // The next caller still asks, and a complete answer is kept. assert_eq!( - memo.get_or_run_keyed(key.clone(), || run(Some("home"), true)), + memo.get_or_run_keyed(key.clone(), |_| run(Some("home"), true)), Some("home".into()) ); assert_eq!( - memo.get_or_run_keyed(key.clone(), || run(None, true)), + memo.get_or_run_keyed(key.clone(), |_| run(None, true)), Some("home".into()) ); assert_eq!(runs.get(), 3); @@ -509,8 +515,6 @@ pub(crate) mod tests { Some("fresh".to_string()) }; assert_eq!(memo.get_or_run(fresh).as_deref(), Some("fresh")); - // Now kept: an unchanged environment is served from the slot. - assert_eq!(memo.get_or_run(|| None).as_deref(), Some("fresh")); assert_eq!(runs.get(), 2); } @@ -518,9 +522,8 @@ pub(crate) mod tests { /// same variables still hits, whatever order `vars_os` yields. #[test] fn env_key_is_sorted() { - let (vars, cwd) = env_key(); + let (vars, _) = env_key(); assert!(vars.windows(2).all(|w| w[0] <= w[1]), "sorted env snapshot"); - assert_eq!(cwd, std::env::current_dir().ok()); } /// Confirm the real runner returns Some for a tiny command we