From 784f437a47e7f3faee75bc68d02ea68c64d854ea Mon Sep 17 00:00:00 2001 From: Aco Piper Date: Wed, 12 Aug 2026 16:18:01 +0900 Subject: [PATCH] Name the fake docker through the seam in init and verify tests The shared Mutex these tests took around env::set_var never discharged its contract: it serialises only the code that takes it, while the tokio runtime, the HTTP client and the libc resolver read the environment from threads holding nothing. The nine init step tests now write a self-contained fake docker into their tempdir and hand production that path through the executable seam it already carries, so no test edits PATH. verify.rs's resolver takes the search path and the empty-entry base as parameters instead of reading PATH and the process working directory, which is what lets its lock and its set_current_dir pair go. The single production caller reads PATH once and passes ".", so the search behaviour is unchanged. With the last init consumer converted, rotate::test_support's global-state machinery has no caller and is deleted. Closes #809 --- src/commands/init/steps.rs | 35 ++-- src/commands/init/steps/http01_admin_tls.rs | 27 +-- src/commands/init/steps/openbao_tls.rs | 80 +++------ src/commands/init/steps/openbao_transition.rs | 55 +++--- src/commands/init/steps/stepca_setup.rs | 34 +--- src/commands/rotate.rs | 81 +-------- src/commands/verify.rs | 165 ++++++++---------- 7 files changed, 153 insertions(+), 324 deletions(-) diff --git a/src/commands/init/steps.rs b/src/commands/init/steps.rs index 1a5330ff..d6cd6c41 100644 --- a/src/commands/init/steps.rs +++ b/src/commands/init/steps.rs @@ -886,27 +886,23 @@ mod rollback_tests { use std::fs; use std::path::PathBuf; - use crate::commands::rotate::test_support::{ - ScopedEnvVar, TEST_DOCKER_ARGS_ENV, env_lock, path_with_prepend, - write_fake_docker_script, - }; + use super::test_support::write_self_contained_fake_docker; let dir = tempfile::tempdir().unwrap(); let messages = crate::i18n::test_messages(); - let bin_dir = dir.path().join("bin"); - fs::create_dir_all(&bin_dir).unwrap(); - write_fake_docker_script(&bin_dir.join("docker")); + // A fake that *would* log is what makes the absent log evidence: + // it is named through the seam, so anything the rollback ran + // would have left a record. + let fake = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake, &args_log); let hcl_path = dir.path().join("openbao.hcl"); fs::write(&hcl_path, "tls_cert_file = ...\n").unwrap(); let runtime = tokio::runtime::Runtime::new().expect("tokio runtime"); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); - let rollback = InitRollback { + docker: Some(fake), hcl_backup: Some(RollbackFile { path: hcl_path.clone(), original: Some("tls_disable = 1\n".to_string()), @@ -987,6 +983,21 @@ pub(super) mod test_support { pub(in crate::commands::init::steps) fn write_self_contained_fake_docker( path: &Path, args_log: &Path, + ) { + write_self_contained_fake_docker_exiting(path, args_log, 0); + } + + /// [`write_self_contained_fake_docker`] whose every invocation exits + /// `exit_code` after logging, so a test can steer the failure path + /// of a docker call production spawns on its behalf. + /// + /// The log it writes is the same one the zero-exit writer produces — + /// one appended, space-joined line per invocation — so a test that + /// swaps one writer for the other keeps its assertions. + pub(in crate::commands::init::steps) fn write_self_contained_fake_docker_exiting( + path: &Path, + args_log: &Path, + exit_code: u8, ) { let log = args_log.display().to_string(); assert!( @@ -994,7 +1005,7 @@ pub(super) mod test_support { "the log path is interpolated into a single-quoted shell word" ); let script = format!( - "#!/bin/sh\nset -eu\n{{ printf '%s ' \"$@\"; printf '\\n'; }} >> '{log}'\nexit 0\n" + "#!/bin/sh\nset -eu\n{{ printf '%s ' \"$@\"; printf '\\n'; }} >> '{log}'\nexit {exit_code}\n" ); fs::write(path, script).expect("fake docker script should be written"); fs::set_permissions(path, fs::Permissions::from_mode(0o700)) diff --git a/src/commands/init/steps/http01_admin_tls.rs b/src/commands/init/steps/http01_admin_tls.rs index 09464529..c683a14f 100644 --- a/src/commands/init/steps/http01_admin_tls.rs +++ b/src/commands/init/steps/http01_admin_tls.rs @@ -267,6 +267,7 @@ pub(crate) fn strip_responder_tls_config(secrets_dir: &Path, messages: &Messages mod tests { use std::collections::BTreeMap; + use super::super::test_support::write_self_contained_fake_docker; use super::*; /// The responder container name a default install renders. const DEFAULT_RESPONDER_CONTAINER: &str = "bootroot-http01"; @@ -301,9 +302,9 @@ mod tests { #[test] fn reissue_falls_back_to_instance_scoped_sans() { let dir = tempfile::tempdir().unwrap(); - let bin_dir = dir.path().join("bin"); - std::fs::create_dir(&bin_dir).unwrap(); - crate::commands::rotate::test_support::write_fake_docker_script(&bin_dir.join("docker")); + let fake = dir.path().join("fake-docker"); + let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake, &args_log); let secrets_dir = dir.path().join("secrets"); let tls_dir = secrets_dir.join("bootroot-http01").join("tls"); @@ -324,26 +325,10 @@ mod tests { expires_at: None, }; - let args_log = dir.path().join("docker_args.log"); let messages = crate::i18n::test_messages(); - let _lock = crate::commands::rotate::test_support::env_lock(); - let _path = crate::commands::rotate::test_support::ScopedEnvVar::set( - "PATH", - crate::commands::rotate::test_support::path_with_prepend(&bin_dir), - ); - let _log = crate::commands::rotate::test_support::ScopedEnvVar::set( - crate::commands::rotate::test_support::TEST_DOCKER_ARGS_ENV, - &args_log, - ); - reissue_http01_admin_tls_cert( - &secrets_dir, - &entry, - "insight-http01", - Path::new("docker"), - &messages, - ) - .expect("re-issuance must succeed against the fake docker"); + reissue_http01_admin_tls_cert(&secrets_dir, &entry, "insight-http01", &fake, &messages) + .expect("re-issuance must succeed against the fake docker"); let log = std::fs::read_to_string(&args_log).unwrap_or_default(); assert!( diff --git a/src/commands/init/steps/openbao_tls.rs b/src/commands/init/steps/openbao_tls.rs index 0d11ee30..a46bb2d0 100644 --- a/src/commands/init/steps/openbao_tls.rs +++ b/src/commands/init/steps/openbao_tls.rs @@ -475,34 +475,14 @@ mod tests { use std::collections::BTreeMap; use std::fs; - use super::super::test_support::write_self_contained_fake_docker; - use super::*; - use crate::commands::rotate::test_support::{ - ScopedEnvVar, TEST_DOCKER_ARGS_ENV, env_lock, path_with_prepend, + use super::super::test_support::{ + write_self_contained_fake_docker, write_self_contained_fake_docker_exiting, }; + use super::*; /// The `OpenBao` container name a default install renders. const DEFAULT_OPENBAO_CONTAINER: &str = "bootroot-openbao"; - /// Fake `docker` that *appends* one line per invocation, unlike the - /// shared helper which truncates: the ordering of the two containers - /// this file runs is exactly what the wiring test below pins. - /// - /// `exit_code` is returned by every invocation, so a non-zero value - /// makes the first container this file runs — the chown — fail. - fn write_appending_fake_docker(path: &Path, exit_code: u8) { - let script = format!( - r#"#!/bin/sh -set -eu -{{ printf '%s ' "$@"; printf '\n'; }} >> "${{BOOTROOT_TEST_DOCKER_ARGS:?missing log path}}" -exit {exit_code} -"# - ); - fs::write(path, script).expect("fake docker script should be written"); - fs::set_permissions(path, std::fs::Permissions::from_mode(0o700)) - .expect("fake docker script should be executable"); - } - #[test] fn build_sans_includes_specific_ip() { let sans = build_openbao_tls_sans("192.168.1.10:8200", None, DEFAULT_OPENBAO_CONTAINER); @@ -532,9 +512,9 @@ exit {exit_code} #[test] fn reissue_falls_back_to_instance_scoped_sans() { let dir = tempfile::tempdir().unwrap(); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).unwrap(); - write_appending_fake_docker(&bin_dir.join("docker"), 0); + let fake = dir.path().join("fake-docker"); + let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake, &args_log); let compose_dir = dir.path().join("compose"); let secrets_dir = dir.path().join("secrets"); @@ -557,18 +537,14 @@ exit {exit_code} expires_at: None, }; - let args_log = dir.path().join("docker_args.log"); let messages = crate::i18n::test_messages(); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); reissue_openbao_tls_cert( &compose_dir, &secrets_dir, &entry, "insight-openbao", - Path::new("docker"), + &fake, &messages, ) .expect("re-issuance must succeed against the fake docker"); @@ -860,9 +836,9 @@ exit {exit_code} #[test] fn issue_openbao_tls_cert_chowns_output_dir_before_creating_the_cert() { let dir = tempfile::tempdir().unwrap(); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).unwrap(); - write_appending_fake_docker(&bin_dir.join("docker"), 0); + let fake = dir.path().join("fake-docker"); + let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake, &args_log); let compose_dir = dir.path().join("compose"); let secrets_dir = dir.path().join("secrets"); @@ -874,18 +850,13 @@ exit {exit_code} fs::write(tls_dir.join("server.crt"), "cert").unwrap(); fs::write(tls_dir.join("server.key"), "key").unwrap(); - let args_log = dir.path().join("docker_args.log"); let messages = crate::i18n::test_messages(); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); - issue_openbao_tls_cert( &compose_dir, &secrets_dir, &["openbao.internal"], - Path::new("docker"), + &fake, &messages, ) .expect("issuing the certificate must succeed against the fake docker"); @@ -936,26 +907,21 @@ exit {exit_code} #[test] fn issue_openbao_tls_cert_aborts_when_the_chown_fails() { let dir = tempfile::tempdir().unwrap(); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).unwrap(); - write_appending_fake_docker(&bin_dir.join("docker"), 1); + let fake = dir.path().join("fake-docker"); + let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker_exiting(&fake, &args_log, 1); let compose_dir = dir.path().join("compose"); let secrets_dir = dir.path().join("secrets"); fs::create_dir_all(&secrets_dir).unwrap(); - let args_log = dir.path().join("docker_args.log"); let messages = crate::i18n::test_messages(); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); - let error = issue_openbao_tls_cert( &compose_dir, &secrets_dir, &["openbao.internal"], - Path::new("docker"), + &fake, &messages, ) .expect_err("a failing chown must fail the issuance"); @@ -988,9 +954,12 @@ exit {exit_code} #[test] fn issue_openbao_tls_cert_refuses_a_symlinked_output_dir() { let dir = tempfile::tempdir().unwrap(); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).unwrap(); - write_appending_fake_docker(&bin_dir.join("docker"), 0); + // The fake would log any invocation it received, so the empty + // log below is evidence that nothing ran rather than evidence + // that nothing could. + let fake = dir.path().join("fake-docker"); + let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake, &args_log); let compose_dir = dir.path().join("compose"); let secrets_dir = dir.path().join("secrets"); @@ -1003,18 +972,13 @@ exit {exit_code} fs::create_dir_all(tls_dir.parent().expect("openbao dir")).unwrap(); std::os::unix::fs::symlink(&elsewhere, &tls_dir).unwrap(); - let args_log = dir.path().join("docker_args.log"); let messages = crate::i18n::test_messages(); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); - let error = issue_openbao_tls_cert( &compose_dir, &secrets_dir, &["openbao.internal"], - Path::new("docker"), + &fake, &messages, ) .expect_err("a symlinked output directory must fail the issuance"); diff --git a/src/commands/init/steps/openbao_transition.rs b/src/commands/init/steps/openbao_transition.rs index 6c756a94..4bf765d8 100644 --- a/src/commands/init/steps/openbao_transition.rs +++ b/src/commands/init/steps/openbao_transition.rs @@ -350,10 +350,8 @@ mod tests { use wiremock::matchers::{method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; + use super::super::test_support::write_self_contained_fake_docker; use super::*; - use crate::commands::rotate::test_support::{ - ScopedEnvVar, TEST_DOCKER_ARGS_ENV, env_lock, path_with_prepend, write_fake_docker_script, - }; use crate::i18n::test_messages; const PROBE_ATTEMPTS: u32 = 1; @@ -577,37 +575,32 @@ mod tests { /// The availability pre-check runs before Docker is touched, so a /// deployment is never knocked into a sealed state `init` cannot /// recover from. - #[test] - fn no_available_source_fails_before_any_docker_call() { + #[tokio::test] + async fn no_available_source_fails_before_any_docker_call() { let dir = tempdir().expect("temp dir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir_all(&bin_dir).expect("bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + // The transition is handed a fake that logs every invocation, so + // the absent log below proves no docker command was emitted. + let fake = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake, &args_log); let compose = dir.path().join("docker-compose.yml"); let override_path = dir.path().join("docker-compose.openbao-exposed.yml"); let default_file = dir.path().join("unseal-keys.txt"); - let runtime = tokio::runtime::Runtime::new().expect("tokio runtime"); - - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); let transition = OpenBaoTlsTransition { + docker: &fake, probe_attempts: PROBE_ATTEMPTS, probe_delay: PROBE_DELAY, ..OpenBaoTlsTransition::new(&compose, &override_path, UNREACHABLE_HTTPS_URL, dir.path()) }; let mut recreated = false; - // `block_on` rather than `#[tokio::test]`: the environment lock - // has to stay held across the whole run so a parallel test - // cannot swap PATH out from under the fake `docker`. - let err = runtime - .block_on(transition.run( + let err = transition + .run( &inputs(&[], None, default_file, false), &mut recreated, &test_messages(), - )) + ) + .await .expect_err("no unseal key source is available"); assert!( @@ -785,39 +778,33 @@ mod tests { /// same failure a listener still answering plaintext produces. /// `state.openbao_url` is advanced by the caller only after this /// returns `Ok`, so the pre-TLS plaintext URL survives. - #[test] - fn tls_probe_failure_stops_before_the_url_is_recorded() { + #[tokio::test] + async fn tls_probe_failure_stops_before_the_url_is_recorded() { let dir = tempdir().expect("temp dir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir_all(&bin_dir).expect("bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + let fake = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake, &args_log); let compose = dir.path().join("docker-compose.yml"); let override_path = dir.path().join("docker-compose.openbao-exposed.yml"); let state_path = dir.path().join("state.json"); let plaintext_state = "{\"openbao_url\":\"http://127.0.0.1:8200\"}\n"; fs::write(&state_path, plaintext_state).expect("write state"); let in_memory = vec!["memory-key".to_string()]; - let runtime = tokio::runtime::Runtime::new().expect("tokio runtime"); - - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); let transition = OpenBaoTlsTransition { + docker: &fake, probe_attempts: PROBE_ATTEMPTS, probe_delay: PROBE_DELAY, ..OpenBaoTlsTransition::new(&compose, &override_path, UNREACHABLE_HTTPS_URL, dir.path()) }; let mut recreated = false; - // See `no_available_source_fails_before_any_docker_call` for why - // this drives the future with `block_on`. - let err = runtime - .block_on(transition.run( + let err = transition + .run( &inputs(&in_memory, None, dir.path().join("unseal-keys.txt"), false), &mut recreated, &test_messages(), - )) + ) + .await .expect_err("nothing is listening on the probed URL"); assert!( diff --git a/src/commands/init/steps/stepca_setup.rs b/src/commands/init/steps/stepca_setup.rs index 3a928c57..81885fb6 100644 --- a/src/commands/init/steps/stepca_setup.rs +++ b/src/commands/init/steps/stepca_setup.rs @@ -572,41 +572,13 @@ mod tests { assert!(!names.contains(&DEFAULT_CA_CONTAINER.to_string())); } + /// The executable seam: the caller names the program, and the child + /// that runs is the one it named. + /// /// The restart bypasses Compose, so nothing but the name it is /// handed decides which install's sidecar goes down. It used to /// inline a default-instance literal, which on a non-default /// instance restarted a co-located default install's sidecar. - #[test] - fn restarting_the_stepca_sidecar_names_the_container_it_is_given() { - use crate::commands::rotate::test_support::{ - ScopedEnvVar, TEST_DOCKER_ARGS_ENV, env_lock, path_with_prepend, - write_fake_docker_script, - }; - - let dir = tempdir().expect("tempdir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("create bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); - let args_log = dir.path().join("docker_args.log"); - - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); - - assert!(restart_stepca_openbao_agent( - "insight-openbao-agent-stepca", - Path::new("docker") - )); - - let logged = fs::read_to_string(&args_log).expect("read docker args"); - assert_eq!( - logged.lines().collect::>(), - vec!["restart", "insight-openbao-agent-stepca"] - ); - } - - /// The executable seam: the caller names the program, and the child - /// that runs is the one it named. /// /// The fake carries its own argv-log path in its script text, so /// nothing here sets a variable on this process or edits `PATH` — diff --git a/src/commands/rotate.rs b/src/commands/rotate.rs index eefc7d99..8f6c4057 100644 --- a/src/commands/rotate.rs +++ b/src/commands/rotate.rs @@ -278,89 +278,18 @@ async fn run_rotate_with_exec( Ok(RotateOutcome::Completed) } -/// Shared test harness for commands that shell out to `docker`. +/// Test harness for the `rotate` commands that shell out to `docker`. /// -/// The fake-`docker` script and the environment scoping around it are -/// `pub(in crate::commands)` rather than `pub(super)` because the -/// `OpenBao` TLS transition in `init` asserts on the docker argv it -/// emits with the same harness. +/// It is private to this module and its descendants: a test names the +/// fake executable through the `docker` seam production already +/// carries, so nothing outside `rotate` reaches in here any more. #[cfg(test)] -pub(super) mod test_support { - use std::env; - use std::ffi::{OsStr, OsString}; +mod test_support { use std::fs; use std::path::Path; - use std::sync::{LazyLock, Mutex, MutexGuard}; pub(super) use crate::i18n::test_messages; - static ENV_LOCK: LazyLock> = LazyLock::new(|| Mutex::new(())); - pub(in crate::commands) const TEST_DOCKER_ARGS_ENV: &str = "BOOTROOT_TEST_DOCKER_ARGS"; - - pub(in crate::commands) struct ScopedEnvVar { - key: &'static str, - previous: Option, - } - - impl ScopedEnvVar { - pub(in crate::commands) fn set(key: &'static str, value: impl AsRef) -> Self { - let previous = env::var_os(key); - // SAFETY: Tests hold ENV_LOCK while mutating process environment. - unsafe { - env::set_var(key, value); - } - Self { key, previous } - } - } - - impl Drop for ScopedEnvVar { - fn drop(&mut self) { - // SAFETY: Tests hold ENV_LOCK while mutating process environment. - unsafe { - if let Some(previous) = &self.previous { - env::set_var(self.key, previous); - } else { - env::remove_var(self.key); - } - } - } - } - - pub(in crate::commands) fn env_lock() -> MutexGuard<'static, ()> { - ENV_LOCK - .lock() - .expect("environment lock must not be poisoned") - } - - pub(in crate::commands) fn write_fake_docker_script(path: &Path) { - let script = r#"#!/bin/sh -set -eu -printf '%s\n' "$@" > "${BOOTROOT_TEST_DOCKER_ARGS:?missing log path}" -if [ -n "${BOOTROOT_TEST_DOCKER_STDERR:-}" ]; then - printf '%s' "${BOOTROOT_TEST_DOCKER_STDERR}" 1>&2 -fi -if [ -n "${BOOTROOT_TEST_DOCKER_EXIT:-}" ]; then - exit "${BOOTROOT_TEST_DOCKER_EXIT}" -fi -exit 0 -"#; - fs::write(path, script).expect("fake docker script should be written"); - #[cfg(unix)] - fs::set_permissions(path, std::fs::Permissions::from_mode(0o700)) - .expect("fake docker script should be executable"); - } - - #[cfg(unix)] - use std::os::unix::fs::PermissionsExt; - - pub(in crate::commands) fn path_with_prepend(bin_dir: &Path) -> OsString { - let mut paths = vec![bin_dir.to_path_buf()]; - if let Some(existing) = env::var_os("PATH") { - paths.extend(env::split_paths(&existing)); - } - env::join_paths(paths).expect("PATH components should be valid") - } - /// Writes a fake `docker` at `path` that appends one record per /// invocation to `args_log` and reads nothing from its environment. /// diff --git a/src/commands/verify.rs b/src/commands/verify.rs index 844951c8..871f9a27 100644 --- a/src/commands/verify.rs +++ b/src/commands/verify.rs @@ -1,5 +1,5 @@ use std::collections::HashSet; -use std::ffi::OsString; +use std::ffi::{OsStr, OsString}; use std::path::{Path, PathBuf}; use std::process::Command; @@ -35,7 +35,13 @@ pub(crate) fn run_verify(args: &VerifyArgs, messages: &Messages) -> Result<()> { print_verify_plan(&entry.service_name, agent_config, messages); - let agent_binary = resolve_agent_binary(args.agent_binary.as_deref(), messages)?; + let search_path = std::env::var_os("PATH"); + let agent_binary = resolve_agent_binary( + args.agent_binary.as_deref(), + search_path.as_deref(), + Path::new("."), + messages, + )?; let status = Command::new(&agent_binary) .args(oneshot_agent_args(entry, agent_config)) .status() @@ -155,7 +161,20 @@ fn oneshot_agent_args(entry: &ServiceEntry, agent_config: &Path) -> Vec, messages: &Messages) -> Result { +/// Resolves the agent executable from the explicit override, then a +/// sibling of the running binary, then `search_path`. +/// +/// `search_path` is the `PATH` value to search — `None` for a variable +/// that is unset, which the error message distinguishes — and +/// `empty_entry_base` is the directory an empty entry stands for. Both +/// are supplied by the caller rather than read here, so the search is +/// exercised without moving anything process-global. +fn resolve_agent_binary( + override_path: Option<&Path>, + search_path: Option<&OsStr>, + empty_entry_base: &Path, + messages: &Messages, +) -> Result { let mut candidates = Vec::new(); if let Some(path) = override_path { @@ -176,7 +195,7 @@ fn resolve_agent_binary(override_path: Option<&Path>, messages: &Messages) -> Re } } - let (found, path_candidates) = find_on_path(AGENT_BINARY_NAME); + let (found, path_candidates) = find_on_path(AGENT_BINARY_NAME, search_path, empty_entry_base); for candidate in &path_candidates { candidates.push(candidate.display().to_string()); } @@ -190,17 +209,27 @@ fn resolve_agent_binary(override_path: Option<&Path>, messages: &Messages) -> Re anyhow::bail!(messages.error_bootroot_agent_not_found(&candidates.join(", "))); } -fn find_on_path(name: &str) -> (Option, Vec) { +/// Searches `search_path` for `name`, returning the first match and +/// every candidate it looked at. +/// +/// `search_path` is the `PATH` value itself rather than a variable name, +/// and `empty_entry_base` is the directory an empty entry stands for — +/// `.` for the production caller, which is what POSIX prescribes. +fn find_on_path( + name: &str, + search_path: Option<&OsStr>, + empty_entry_base: &Path, +) -> (Option, Vec) { let mut checked = Vec::new(); - let Some(path_var) = std::env::var_os("PATH") else { + let Some(path_var) = search_path else { return (None, checked); }; let mut found = None; - for dir in std::env::split_paths(&path_var) { + for dir in std::env::split_paths(path_var) { // POSIX treats an empty PATH entry (leading/trailing/doubled `:`) as // the current working directory, so normalise before searching. let search_dir = if dir.as_os_str().is_empty() { - PathBuf::from(".") + empty_entry_base.to_path_buf() } else { dir }; @@ -386,14 +415,10 @@ fn expected_dns_name(entry: &ServiceEntry, messages: &Messages) -> Result = Mutex::new(()); - fn test_service_entry(delivery_mode: DeliveryMode) -> ServiceEntry { ServiceEntry { service_name: "edge-proxy".to_string(), @@ -635,46 +660,38 @@ mod tests { #[test] fn resolve_agent_binary_prefers_override() { - let _guard = ENV_LOCK.lock().unwrap(); let messages = crate::i18n::test_messages(); let dir = tempdir().unwrap(); let explicit = dir.path().join("bootroot-agent"); write_executable(&explicit); - let resolved = resolve_agent_binary(Some(&explicit), &messages).unwrap(); + // An empty search path is the honest input for a case whose + // whole point is that the override short-circuits the search. + let resolved = + resolve_agent_binary(Some(&explicit), None, Path::new("."), &messages).unwrap(); assert_eq!(resolved, explicit); } #[test] fn resolve_agent_binary_falls_back_to_path() { - let _guard = ENV_LOCK.lock().unwrap(); let messages = crate::i18n::test_messages(); let dir = tempdir().unwrap(); let path_entry = dir.path().to_path_buf(); let binary = path_entry.join(AGENT_BINARY_NAME); write_executable(&binary); - let original_path = std::env::var_os("PATH"); - // SAFETY: Serialized via ENV_LOCK so no other test mutates PATH concurrently. - unsafe { - std::env::set_var("PATH", path_entry.as_os_str()); - } - let resolved = resolve_agent_binary(None, &messages); - // SAFETY: Same serialization guarantee as above. - unsafe { - match original_path { - Some(value) => std::env::set_var("PATH", value), - None => std::env::remove_var("PATH"), - } - } - - let resolved = resolved.unwrap(); + let resolved = resolve_agent_binary( + None, + Some(path_entry.as_os_str()), + Path::new("."), + &messages, + ) + .unwrap(); assert_eq!(resolved, binary); } #[test] fn resolve_agent_binary_error_names_candidates() { - let _guard = ENV_LOCK.lock().unwrap(); let messages = crate::i18n::test_messages(); let dir = tempdir().unwrap(); let missing = dir.path().join("no-such-agent"); @@ -684,19 +701,13 @@ mod tests { std::fs::create_dir_all(&path_dir_b).unwrap(); let path_value = std::env::join_paths([&path_dir_a, &path_dir_b]).unwrap(); - let original_path = std::env::var_os("PATH"); - // SAFETY: Serialized via ENV_LOCK so no other test mutates PATH concurrently. - unsafe { - std::env::set_var("PATH", &path_value); - } - let err = resolve_agent_binary(Some(&missing), &messages).unwrap_err(); - // SAFETY: Same serialization guarantee as above. - unsafe { - match original_path { - Some(value) => std::env::set_var("PATH", value), - None => std::env::remove_var("PATH"), - } - } + let err = resolve_agent_binary( + Some(&missing), + Some(path_value.as_os_str()), + Path::new("."), + &messages, + ) + .unwrap_err(); let rendered = err.to_string(); assert!( @@ -717,7 +728,6 @@ mod tests { #[test] fn resolve_agent_binary_error_lists_empty_path_segment_as_cwd() { - let _guard = ENV_LOCK.lock().unwrap(); let messages = crate::i18n::test_messages(); let dir = tempdir().unwrap(); let missing = dir.path().join("no-such-agent"); @@ -726,19 +736,15 @@ mod tests { // Leading separator yields an empty segment, which POSIX treats as `.`. let path_value = std::env::join_paths([PathBuf::new(), path_dir.clone()]).unwrap(); - let original_path = std::env::var_os("PATH"); - // SAFETY: Serialized via ENV_LOCK so no other test mutates PATH concurrently. - unsafe { - std::env::set_var("PATH", &path_value); - } - let err = resolve_agent_binary(Some(&missing), &messages).unwrap_err(); - // SAFETY: Same serialization guarantee as above. - unsafe { - match original_path { - Some(value) => std::env::set_var("PATH", value), - None => std::env::remove_var("PATH"), - } - } + // The production `.` base, because what is pinned here is how an + // empty entry renders in the error. + let err = resolve_agent_binary( + Some(&missing), + Some(path_value.as_os_str()), + Path::new("."), + &messages, + ) + .unwrap_err(); let rendered = err.to_string(); let cwd_candidate = PathBuf::from(".").join(AGENT_BINARY_NAME); @@ -756,7 +762,6 @@ mod tests { #[cfg(unix)] #[test] fn resolve_agent_binary_uses_cwd_for_empty_path_segment() { - let _guard = ENV_LOCK.lock().unwrap(); let messages = crate::i18n::test_messages(); let dir = tempdir().unwrap(); let agent = dir.path().join(AGENT_BINARY_NAME); @@ -766,48 +771,24 @@ mod tests { // Trailing separator yields an empty segment after the non-empty entry. let path_value = std::env::join_paths([path_dir, PathBuf::new()]).unwrap(); - let original_path = std::env::var_os("PATH"); - let original_cwd = std::env::current_dir().unwrap(); - // SAFETY: Serialized via ENV_LOCK; cwd and PATH restored below. - unsafe { - std::env::set_var("PATH", &path_value); - } - std::env::set_current_dir(dir.path()).unwrap(); - let resolved = resolve_agent_binary(None, &messages); - std::env::set_current_dir(&original_cwd).unwrap(); - // SAFETY: Same serialization guarantee as above. - unsafe { - match original_path { - Some(value) => std::env::set_var("PATH", value), - None => std::env::remove_var("PATH"), - } - } - - let resolved = resolved.expect("empty PATH segment should resolve via cwd"); - // Relative candidate; resolution used the cwd we set above. - assert_eq!(resolved, PathBuf::from(".").join(AGENT_BINARY_NAME)); + // The tempdir stands in for the working directory an empty entry + // means, so the resolution the process-wide `chdir` used to set + // up is exercised without moving this process anywhere. + let resolved = + resolve_agent_binary(None, Some(path_value.as_os_str()), dir.path(), &messages) + .expect("empty PATH segment should resolve via the empty-entry base"); + assert_eq!(resolved, agent); assert!(agent.is_file()); } #[test] fn resolve_agent_binary_error_handles_unset_path() { - let _guard = ENV_LOCK.lock().unwrap(); let messages = crate::i18n::test_messages(); let dir = tempdir().unwrap(); let missing = dir.path().join("no-such-agent"); - let original_path = std::env::var_os("PATH"); - // SAFETY: Serialized via ENV_LOCK so no other test mutates PATH concurrently. - unsafe { - std::env::remove_var("PATH"); - } - let err = resolve_agent_binary(Some(&missing), &messages).unwrap_err(); - // SAFETY: Same serialization guarantee as above. - unsafe { - if let Some(value) = original_path { - std::env::set_var("PATH", value); - } - } + let err = + resolve_agent_binary(Some(&missing), None, Path::new("."), &messages).unwrap_err(); let rendered = err.to_string(); assert!(