From ae02c9b2ddc9f432c100fc17e71d152038828caa Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 28 Oct 2026 14:32:01 +0000 Subject: [PATCH 1/3] test(therock): stop the update-json guard test mutating process PATH render_update_json_installs_the_suppression_guard_around_resolution steered render_update_json's Python resolution onto its PATH-unusable branch by overwriting the process-wide PATH env var under PROCESS_ENV_TEST_LOCK. cargo test runs every test as a thread in one process, so any sibling test thread spawning a bare-name PATH-resolved binary (tar, sleep, powershell) during that window could fail with ENOENT/"program not found" -- seen repeatedly (#650), most recently on 2026-10-27 during unrelated verification, and patched around twice more this week (bbd80bf5, 8f0e37e4) by retrofitting yet another tar/PATH-spawning test onto the same lock. That whack-a-mole only serializes around the mutation; it never removes it. #253 fixed this same problem for two other python-resolver tests by passing the resolver's environment in as a value (PythonResolverEnv) instead of reading the process PATH at the use site. Those tests call the low-level resolver directly, so they could just construct one. This test exercises the higher-level render_update_json instead, and the call chain between them has production call sites of its own, so threading an env parameter through it would touch signatures used well beyond this one test. Add a #[cfg(test)] thread-local override for resolve_python_launcher's PythonResolverEnv instead, mirroring the SuppressProgressOutput/ PROGRESS_LINE_SINK seam already in this file: a test can now point resolution at a fixture directory without ever touching the real PATH, and because it's thread-local, nothing about it can race a sibling test thread. Production builds always see None and fall straight through to the real process environment, unchanged. Add a regression test proving the property this closes: resolving through the override never mutates process-wide PATH. Signed-off-by: Jussi Elo --- apps/rocm/src/therock.rs | 120 ++++++++++++++++++++++++++++++++++----- 1 file changed, 107 insertions(+), 13 deletions(-) diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 5b7143e81..3abddf6fb 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -6455,6 +6455,7 @@ fn ensure_managed_python( /// 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. +#[derive(Clone)] struct PythonResolverEnv { /// `ROCM_CLI_PYTHON`: an explicit interpreter that wins over any search. python_override: Option, @@ -6473,11 +6474,60 @@ 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. +#[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`]). +#[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 +7779,12 @@ 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. let _env_guard = PROCESS_ENV_TEST_LOCK.lock().unwrap(); let (root, paths) = test_paths("render-update-json-installs-guard"); @@ -7739,28 +7795,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 +10466,51 @@ mod tests { Ok(()) } + /// This is the property that eliminates issue #650: the only way to steer + /// `resolve_python_launcher` used to be overwriting process-wide `PATH`, + /// which raced any sibling test thread spawning a PATH-resolved binary + /// (e.g. bare-name `tar`) during that window. The override is + /// thread-local, so resolving through it must never call + /// `std::env::set_var`/`remove_var` on `PATH` at all. + #[cfg(unix)] + #[test] + fn python_resolver_env_override_resolves_without_touching_process_path() -> 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 path_before = std::env::var_os("PATH"); + let launcher = { + let _guard = OverridePythonResolverEnv::new(PythonResolverEnv { + python_override: None, + search_dirs: vec![bin_dir.clone()], + }); + resolve_python_launcher(&paths, python_requirement(SourceLayout::Canonical))? + }; + let path_after = std::env::var_os("PATH"); + + assert_eq!( + path_before, path_after, + "resolving through the override must never mutate process-wide PATH" + ); + 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(()) + } + #[cfg(unix)] fn write_fake_python_with_venv(dir: &Path, name: &str) -> Result { use rocm_core::fake_executable::write_fake_executable; From 5148ef60545b37e1d628addee43813d0b461512c Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Wed, 28 Oct 2026 16:19:03 +0000 Subject: [PATCH 2/3] test(therock): clarify the new python-resolver override seam Independent review of the #650 fix surfaced three small gaps: - PythonResolverEnv's Clone derive was unconditional even though the only clone() call is #[cfg(test)]; gate it the same way the rest of the seam already is. - OverridePythonResolverEnv's doc comment claimed nested use composes correctly (mirroring SuppressProgressOutput) with nothing proving it; add the same bookkeeping test SuppressProgressOutput has. - Neither PythonResolverEnv's doc comment nor the rewritten test's comment on PROCESS_ENV_TEST_LOCK mentioned why that lock is still taken now that PATH is no longer mutated there. Signed-off-by: Jussi Elo --- apps/rocm/src/therock.rs | 65 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 3 deletions(-) diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index 3abddf6fb..a61bac5a3 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -6454,8 +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. -#[derive(Clone)] +/// 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, @@ -6490,7 +6493,9 @@ thread_local! { /// 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`]). +/// use composes correctly, mirroring [`SuppressProgressOutput`] — see the +/// `nested_python_resolver_env_override_restores_the_outer_one_on_drop` test +/// for the property this claims). #[cfg(test)] struct OverridePythonResolverEnv { previous: Option, @@ -7785,6 +7790,12 @@ mod tests { // 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, even where (as here) nothing else currently + // reads that specific variable. let _env_guard = PROCESS_ENV_TEST_LOCK.lock().unwrap(); let (root, paths) = test_paths("render-update-json-installs-guard"); @@ -10511,6 +10522,54 @@ mod tests { 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; From bb519ab36373c6bd4355a6bd33f8a790b5fa5373 Mon Sep 17 00:00:00 2001 From: Jussi Elo Date: Thu, 29 Oct 2026 12:29:06 +0000 Subject: [PATCH 3/3] test(therock): tighten the python-resolver override tests and comments Rename the override test and drop its PATH-equality assertion, which could not fail (the override never touches the environment) and overclaimed in the name. Document that the thread-local override only applies on the installing thread, drop an unverifiable claim about other readers of the bootstrap env var, and remove em dashes from the new comments. Signed-off-by: Jussi Elo --- apps/rocm/src/therock.rs | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index a61bac5a3..87311287f 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -6456,7 +6456,7 @@ fn ensure_managed_python( /// overwrote `PATH` to steer this resolver also hid every other PATH-resolved /// 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 +/// `OverridePythonResolverEnv`) instead of constructing one to pass directly /// to [`resolve_python_launcher_in`]. #[cfg_attr(test, derive(Clone))] struct PythonResolverEnv { @@ -6482,9 +6482,14 @@ impl PythonResolverEnv { // // 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 +// 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. +// 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> = @@ -6493,9 +6498,9 @@ thread_local! { /// 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`] — see the +/// use composes correctly, mirroring [`SuppressProgressOutput`]; the /// `nested_python_resolver_env_override_restores_the_outer_one_on_drop` test -/// for the property this claims). +/// covers the property this claims). #[cfg(test)] struct OverridePythonResolverEnv { previous: Option, @@ -7794,8 +7799,7 @@ mod tests { // 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, even where (as here) nothing else currently - // reads that specific variable. + // file takes this lock. let _env_guard = PROCESS_ENV_TEST_LOCK.lock().unwrap(); let (root, paths) = test_paths("render-update-json-installs-guard"); @@ -10477,15 +10481,14 @@ mod tests { Ok(()) } - /// This is the property that eliminates issue #650: the only way to steer - /// `resolve_python_launcher` used to be overwriting process-wide `PATH`, - /// which raced any sibling test thread spawning a PATH-resolved binary - /// (e.g. bare-name `tar`) during that window. The override is - /// thread-local, so resolving through it must never call - /// `std::env::set_var`/`remove_var` on `PATH` at all. + /// `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_resolves_without_touching_process_path() -> Result<()> { + 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 @@ -10498,7 +10501,6 @@ mod tests { fs::create_dir_all(&bin_dir)?; write_fake_python_with_venv(&bin_dir, "python3")?; - let path_before = std::env::var_os("PATH"); let launcher = { let _guard = OverridePythonResolverEnv::new(PythonResolverEnv { python_override: None, @@ -10506,12 +10508,7 @@ mod tests { }); resolve_python_launcher(&paths, python_requirement(SourceLayout::Canonical))? }; - let path_after = std::env::var_os("PATH"); - assert_eq!( - path_before, path_after, - "resolving through the override must never mutate process-wide PATH" - ); assert_eq!( launcher.source, "path", "resolve_python_launcher must consult the test override, not the real environment"