diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 5b7143e81..87311287f 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -6454,7 +6454,11 @@ fn ensure_managed_python( /// resolver somewhere else without touching the process environment. Tests need /// that: `cargo test` runs every test as a thread in one process, so a test that /// overwrote `PATH` to steer this resolver also hid every other PATH-resolved -/// binary from unrelated tests running at the same moment. +/// binary from unrelated tests running at the same moment. In test builds, a +/// test can also install one as a thread-local override (see +/// `OverridePythonResolverEnv`) instead of constructing one to pass directly +/// to [`resolve_python_launcher_in`]. +#[cfg_attr(test, derive(Clone))] struct PythonResolverEnv { /// `ROCM_CLI_PYTHON`: an explicit interpreter that wins over any search. python_override: Option, @@ -6473,11 +6477,67 @@ impl PythonResolverEnv { } } +// Lets a test steer `resolve_python_launcher` at a fixed `PythonResolverEnv` +// instead of the real process environment, scoped to the calling thread only. +// +// Unlike mutating process-wide `PATH`, a thread-local cannot race a sibling +// test thread that spawns a PATH-resolved binary (e.g. bare-name `tar`) +// during the window a test is steering resolution (see issue #650, where +// that mutation intermittently failed unrelated tests under `cargo test`'s +// one-process-many-threads model). +// +// Because the override is thread-local, it only applies when +// `resolve_python_launcher` runs on the thread that installed it. If the code +// under test ever resolves from a spawned thread (or `thread::scope`), that +// thread silently falls back to the real process environment. +#[cfg(test)] +thread_local! { + static PYTHON_RESOLVER_ENV_OVERRIDE: std::cell::RefCell> = + const { std::cell::RefCell::new(None) }; +} + +/// RAII guard installing a fixed [`PythonResolverEnv`] for [`resolve_python_launcher`] +/// on the calling thread only, restoring the prior override on drop (so nested +/// use composes correctly, mirroring [`SuppressProgressOutput`]; the +/// `nested_python_resolver_env_override_restores_the_outer_one_on_drop` test +/// covers the property this claims). +#[cfg(test)] +struct OverridePythonResolverEnv { + previous: Option, +} + +#[cfg(test)] +impl OverridePythonResolverEnv { + fn new(env: PythonResolverEnv) -> Self { + let previous = PYTHON_RESOLVER_ENV_OVERRIDE.with(|cell| cell.replace(Some(env))); + Self { previous } + } +} + +#[cfg(test)] +impl Drop for OverridePythonResolverEnv { + fn drop(&mut self) { + let previous = self.previous.take(); + PYTHON_RESOLVER_ENV_OVERRIDE.with(|cell| *cell.borrow_mut() = previous); + } +} + +#[cfg(test)] +fn python_resolver_env_override() -> Option { + PYTHON_RESOLVER_ENV_OVERRIDE.with(|cell| cell.borrow().clone()) +} + +#[cfg(not(test))] +const fn python_resolver_env_override() -> Option { + None +} + fn resolve_python_launcher( paths: &AppPaths, requirement: PythonRequirement, ) -> Result { - resolve_python_launcher_in(paths, &PythonResolverEnv::from_process_env(), requirement) + let env = python_resolver_env_override().unwrap_or_else(PythonResolverEnv::from_process_env); + resolve_python_launcher_in(paths, &env, requirement) } fn resolve_python_launcher_in( @@ -7729,6 +7789,17 @@ mod tests { // network-free). If `render_update_json` stopped installing the // guard, that call would land in `PROGRESS_LINE_SINK` instead of // being swallowed, and the assertion below would fail. + // + // The PATH-python-unusable branch is reached via a thread-local + // `OverridePythonResolverEnv`, not a real process `PATH` mutation: + // see issue #650, where mutating process-wide `PATH` to steer this + // test raced any sibling test thread that spawned a PATH-resolved + // binary (e.g. bare-name `tar`) during the window. + // + // The lock below is no longer for `PATH`: it's still needed because + // this test mutates the real `ROCM_CLI_DISABLE_MANAGED_PYTHON_BOOTSTRAP` + // process env var below, and every process-env-mutating test in this + // file takes this lock. let _env_guard = PROCESS_ENV_TEST_LOCK.lock().unwrap(); let (root, paths) = test_paths("render-update-json-installs-guard"); @@ -7739,28 +7810,21 @@ mod tests { fs::create_dir_all(&bin_dir)?; fs::write(bin_dir.join("python3"), "not an interpreter")?; - let old_path = std::env::var_os("PATH"); - let old_python_override = std::env::var_os("ROCM_CLI_PYTHON"); let old_bootstrap_disabled = std::env::var_os("ROCM_CLI_DISABLE_MANAGED_PYTHON_BOOTSTRAP"); unsafe { - std::env::set_var("PATH", &bin_dir); - std::env::remove_var("ROCM_CLI_PYTHON"); std::env::set_var("ROCM_CLI_DISABLE_MANAGED_PYTHON_BOOTSTRAP", "1"); } + let resolver_env_guard = OverridePythonResolverEnv::new(PythonResolverEnv { + python_override: None, + search_dirs: vec![bin_dir], + }); PROGRESS_LINE_SINK.with(|sink| sink.borrow_mut().clear()); assert!(!progress_output_suppressed()); let result = render_update_json(&paths, Some(1)); + drop(resolver_env_guard); unsafe { - match old_path { - Some(value) => std::env::set_var("PATH", value), - None => std::env::remove_var("PATH"), - } - match old_python_override { - Some(value) => std::env::set_var("ROCM_CLI_PYTHON", value), - None => std::env::remove_var("ROCM_CLI_PYTHON"), - } match old_bootstrap_disabled { Some(value) => { std::env::set_var("ROCM_CLI_DISABLE_MANAGED_PYTHON_BOOTSTRAP", value); @@ -10417,6 +10481,92 @@ mod tests { Ok(()) } + /// `resolve_python_launcher` must consult an installed + /// `OverridePythonResolverEnv` instead of the real process environment. + /// That is what lets a test steer resolution without overwriting + /// process-wide `PATH`, which raced sibling test threads spawning + /// PATH-resolved binaries (issue #650). + #[cfg(unix)] + #[test] + fn python_resolver_env_override_is_consulted_by_resolve_python_launcher() -> Result<()> { + if current_platform_wheel_tags().is_err() { + // No wheel platform tag for this host (e.g. macOS): every python fails + // the wheel-compatibility check, so resolution always falls through to + // the managed/uv path regardless of PATH. Nothing to assert here. + return Ok(()); + } + let _guard = PROCESS_ENV_TEST_LOCK.lock().unwrap(); + let (root, paths) = test_paths("python-resolver-env-override-hermetic"); + let bin_dir = root.join("bin"); + fs::create_dir_all(&bin_dir)?; + write_fake_python_with_venv(&bin_dir, "python3")?; + + let launcher = { + let _guard = OverridePythonResolverEnv::new(PythonResolverEnv { + python_override: None, + search_dirs: vec![bin_dir.clone()], + }); + resolve_python_launcher(&paths, python_requirement(SourceLayout::Canonical))? + }; + + assert_eq!( + launcher.source, "path", + "resolve_python_launcher must consult the test override, not the real environment" + ); + assert!(launcher.executable.starts_with(&bin_dir)); + + fs::remove_dir_all(root).ok(); + Ok(()) + } + + #[test] + fn nested_python_resolver_env_override_restores_the_outer_one_on_drop() { + // Mirrors `suppress_progress_output_contract`'s bookkeeping check for + // `SuppressProgressOutput`: proves the same restore-previous + // composition for `OverridePythonResolverEnv`, which its doc comment + // claims but nothing else here exercises. Thread-local, so this + // doesn't race with other tests' threads. + assert!(python_resolver_env_override().is_none()); + { + let _outer = OverridePythonResolverEnv::new(PythonResolverEnv { + python_override: Some("outer".to_owned()), + search_dirs: vec![], + }); + assert_eq!( + python_resolver_env_override() + .unwrap() + .python_override + .as_deref(), + Some("outer") + ); + { + let _inner = OverridePythonResolverEnv::new(PythonResolverEnv { + python_override: Some("inner".to_owned()), + search_dirs: vec![], + }); + assert_eq!( + python_resolver_env_override() + .unwrap() + .python_override + .as_deref(), + Some("inner") + ); + } + assert_eq!( + python_resolver_env_override() + .unwrap() + .python_override + .as_deref(), + Some("outer"), + "dropping the inner override must restore the outer one, not clear it" + ); + } + assert!( + python_resolver_env_override().is_none(), + "dropping the outer override must restore the pre-override state" + ); + } + #[cfg(unix)] fn write_fake_python_with_venv(dir: &Path, name: &str) -> Result { use rocm_core::fake_executable::write_fake_executable;