[Umbrella] Remove the remaining environment mutation from tests

[sehkone]

Remove the remaining environment mutation from tests

Summary

This crate's tests steer production code by mutating the process environment. There are 84 std::env::set_var / std::env::remove_var calls under #[cfg(test)], spread over nine files, each wrapped in an unsafe block whose // SAFETY: comment cites a shared Mutex.

In Rust 2024 those two functions are unsafe for a reason the Mutex does not address. Outside Windows, another thread reading the environment while one mutates it is undefined behaviour, and a lock excludes only the code that takes it. A tokio runtime, an HTTP client, an env-filtered tracing subscriber and the libc resolver all read the environment from threads that hold nothing. Every // SAFETY: comment in this set is therefore wrong, and the crate has 84 of them.

The tests mutate for two distinct reasons, and the two need different fixes:

• Value steering. A production function reads a named variable — POSTGRES_USER, COMPOSE_PROJECT_NAME, OPENBAO_ROOT_TOKEN, a *_HOST_PORT key — and the test sets it to choose a branch. The fix is to make the value a parameter and leave one thin wrapper at the composition root reading the environment once.
• Executable shadowing. The test writes a shell script named docker (or bootroot-agent) into a tempdir and prepends that directory to PATH so production spawns the fake. The fix needs production to change first: the executable has to become something a caller can supply, because production is what constructs the Command. A test cannot redirect a child it does not spawn.

src/config.rs already carries the shape of the first fix. Settings::from_file (line 284) loads a configuration file without layering the BOOTROOT_* Environment source that Settings::new (line 255) adds, so a test asserting on file contents does not have to empty the environment first — it asserts that the file alone is sufficient. The doc comment there explains why emptying the environment was not an option.

When the effort is done, grep -rn "set_var\|remove_var" src returns exactly two lines: the production set_var in src/commands/dotenv.rs:96, whose justification is genuinely sound and which stays, and the word set_var in the Settings::from_file rustdoc at src/config.rs:280, which is prose about why that function exists. No call under #[cfg(test)] survives, and no environment-serialisation lock remains anywhere in the crate.

Children

• Pass environment-derived values into the resolvers their tests steer #806
• Make the docker executable a value the caller supplies #807
• Convert the rotate tests to the injected docker executable #808
• Convert the init-step and verify tests, and remove the last environment mutation #809

The work splits along the two reasons above, and the split is what makes it parallelisable at the start.

Two children can begin at once. One parameterises the value-steered resolvers — that is 67 of the 84 mutation sites, in files whose tests never fake an executable. The other is a production-only refactor that makes the docker executable a caller-supplied value at the four places the crate spawns it on a path some PATH-faking test exercises; it converts no test and removes no mutation, so it can land while the first is still in review.

They are independent in intent but not quite in text. Both touch src/commands/compose_project.rs, in regions that do not overlap: the first owns compose_project_name_override (line 101) and the test_env module (line 315), the second owns DOCKER_BIN (line 23) and ComposeInvocation::command (line 266). Whichever merges second rebases across a textual conflict in one file, not a disagreement about what the file should do. Each body says so, and neither may edit the other's region.

The two conversion children run after those. Each takes one tree of executable-shadowing tests and rewrites them onto the seam. They are ordered rather than parallel because the shared test-support module they both draw on can only be emptied once its last consumer is gone, and because the final child carries the crate-wide assertion that nothing is left.

The critical path is therefore three deep: seam → rotate conversion → init/verify conversion. The schedule risk sits in the middle child and the last one, where a converted test can silently stop asserting what it used to assert. Both carry an explicit criterion against that: corrupt the expected argv, confirm the test fails.

Shared background

AGENTS.md, testing section, is the rule this effort enforces. In substance it bans mutating the process environment in tests, names the shared-Mutex workaround as not making the call sound, says to remove an existing mutation rather than add another lock, and says to keep env::var at a thin composition boundary so the logic underneath receives its values instead of reading them. Read it in the file rather than from a quotation: it is a shared block that gets reworded upstream, and no child should go stale against it.

Two rules apply to every child:

• Do not replace one lock with another. A Mutex, a OnceLock, an RAII guard or a semaphore around the mutation is the same unsound pattern under a different name.
• Do not reach for --test-threads=1. It serialises test functions and says nothing about the runtime, client and subscriber threads that do the concurrent reading.

Where an exact process environment genuinely must be asserted, assert it in a child process with Command::env, Command::env_remove or Command::env_clear. Setting a variable on a child the test spawns is always fine; setting one on this process never is.

Note that src/commands/ lives in the binary crate (src/main.rs:6), while src/host_port.rs and src/db.rs live in the library. cargo test --lib runs none of the commands::* tests. Use cargo test --bin bootroot.

Two test-support modules hold the machinery being removed:

• src/commands/rotate.rs:266, test_support — ENV_LOCK (line 275), env_lock() (308), the ScopedEnvVar RAII guard (279) whose set and Drop are that file's three mutation sites, path_with_prepend (335), write_fake_docker_script (314), and the TEST_DOCKER_ARGS_ENV / TEST_DOCKER_EXIT_ENV names (276, 277). Its items are pub(in crate::commands), and the init-step tests import them too — this module, not the one below, is where all the fake-docker plumbing lives.
• src/commands/init/steps.rs:861, test_support — only ENV_LOCK (870), env_lock() (872), default_init_args (879) and re-exported test_messages / test_cert_pem. Its env_lock() has exactly one caller: src/commands/init/steps/database.rs.

Neither module is deleted outright. Both hold helpers that touch nothing global and that modules outside this effort import.

Execution order

Issues in the same wave have no unmet dependencies among these children and can run in parallel.

• Wave 1: Pass environment-derived values into the resolvers their tests steer #806, Make the docker executable a value the caller supplies #807
• Wave 2: Convert the rotate tests to the injected docker executable #808
• Wave 3: Convert the init-step and verify tests, and remove the last environment mutation #809

graph TD
  issue806["#806 Pass environment-derived values into the resolvers their tests steer [phase: unphased]"]
  issue807["#807 Make the docker executable a value the caller supplies [phase: unphased]"]
  issue808["#808 Convert the rotate tests to the injected docker executable [phase: unphased]"]
  issue809["#809 Convert the init-step and verify tests, and remove the last environment mutation [phase: unphased]"]
  issue807 --> issue808
  issue806 --> issue808
  issue806 --> issue809
  issue807 --> issue809
  issue808 --> issue809

Loading