Make the docker executable a caller-supplied value (#807)

[sehkone]

Summary

Makes the docker executable a value the caller supplies at the four spawn sites a PATH-faking test reaches, so a test can redirect the child without mutating the process environment. This creates the seam only — no test is converted and no PATH mutation is removed here.

• DOCKER_BIN (compose_project.rs) widens to pub(crate) and becomes the only spelling of the literal the seam uses. No spawn site reads it.
• infra::run_docker, infra::run_compose, infra::run_compose_with_env and ComposeInvocation::command keep their present signatures and become one-line delegations to *_with_exec siblings that take docker: &Path, so all thirty-nine-odd call sites outside the carrier table compile untouched.
• The value travels as a &Path parameter named docker, placed immediately before messages, except where the carrier is a method on a value a test already constructs. There it is a field: RotateContext::docker: PathBuf (set once in run_rotate), OpenBaoTlsTransition::docker: &'a Path (set in new), and InitRollback::docker: Option<PathBuf> with a single docker() accessor resolving None to DOCKER_BIN. InitRollback keeps its #[derive(Default)]; the Option is what keeps a defaulted value from spawning the empty path.
• Carriers threaded: rotate::helpers::restart_openbao_agent/restart_container, rotate::approle's entry points (via the context field, avoiding an eighth argument and the #[allow(clippy::too_many_arguments)] it would need), rotate::stepca_password::change_stepca_passphrase, rotate::infra_cert::execute_reload_strategy/try_signal_container, infra::run_compose, init::steps::openbao_tls::issue_openbao_tls_cert, init::steps::http01_admin_tls::issue_http01_admin_tls_cert, and init::steps::stepca_setup::restart_stepca_openbao_agent. orchestrator passes rollback.docker() for the three init spawns it drives directly.
• Deliberately left ambient, per the issue: infra::docker_output, the two clean.rs helpers and rotate::helpers::try_restart_container — including execute_reload_strategy's ContainerRestart arm, whose &dyn Fn(&str) -> Result<()> seam in rotate/ca.rs must not move.
• rollback gains #[allow(clippy::too_many_lines)], with a comment saying the teardown is one linear sequence that splitting would only scatter. It is the single allow in the diff, and the one the issue's acceptance criteria name as permitted.
• Eight new tests prove the seam, none of them touching process-global state. Four drive a fake executable end to end through a real production path: rollback_runs_the_executable_it_was_given supplies it through InitRollback::docker, runs the real rollback, and asserts the fake ran for both spawn shapes (the compose recreate and the sidecar restart); issue_openbao_tls_cert_runs_the_supplied_executable, restarting_the_stepca_sidecar_runs_the_supplied_executable and container_signal_runs_the_supplied_executable do the same at the three remaining carriers that reach a spawn. The other four pin the defaults and the plumbing — rollback_defaults_to_the_docker_executable, rollback_resolves_the_executable_it_was_given, the_transition_defaults_to_docker_and_honours_a_named_executable, and compose_command_runs_the_supplied_executable, which also asserts the argument vector is identical to the default path's. Every fake is self-contained: its argv-log path is interpolated into the script text at write time, so it reads no environment variable at runtime.
• That fake is written twice — write_self_contained_fake_docker in init::steps::test_support, and a private twin in infra_cert.rs's test module. The one module both trees can reach, rotate::test_support, holds the PATH-based fake every unconverted test still depends on and has to stay untouched until the conversion issues land. The duplication is what that costs, and it goes away with them.

Behaviour with nothing supplied is unchanged: ProcessCommand::new(Path::new("docker")) resolves against PATH exactly as ProcessCommand::new("docker") does, and the argument vectors, error contexts and the BOOTROOT_INSTANCE pin ComposeInvocation::command applies last are all untouched.

Closes #807

Test plan

• cargo test --bin bootroot passes (979 passed, 0 failed), including every test still faking docker through PATH
• cargo test --lib passes (398 passed, 0 failed)
• The four end-to-end seam tests pass with BOOTROOT_TEST_DOCKER_ARGS and BOOTROOT_TEST_DOCKER_EXIT unset — their fakes carry their own log paths — and each fails if the executable is pointed somewhere wrong (the log read expects the fake ran)
• A test builds an InitRollback with ..Default::default() and docker left None, and the resolved program is still docker, so the empty-path trap a bare PathBuf would open cannot reopen
• grep -rn 'Command::new("docker")\|ProcessCommand::new("docker")\|ProcessCommand::new(DOCKER_BIN)' src returns only infra.rs's docker_output, the two clean.rs helpers and rotate/helpers.rs's try_restart_container
• git diff --name-only lists no change to src/commands/monitoring.rs or src/commands/dns_alias.rs; src/commands/rotate/ca.rs changes by exactly three lines (the ctx_for_instance field plus the two imports it compels)
• grep -rn "set_var\|remove_var" src returns 86 lines, exactly as on main — this change removes none of them, and write_fake_docker_script, ScopedEnvVar, path_with_prepend and verify.rs's ENV_LOCK are unchanged
• No new unsafe block anywhere in the diff
• cargo clippy --all-targets -- -D warnings is clean
• cargo fmt -- --check --config group_imports=StdExternalCrate is clean
• Full CI green, including the Docker E2E matrix and the extended E2E workflow — these run the real docker, so they are the check that the default path is genuinely unchanged (the extended workflow has no PR trigger; dispatched by hand on this branch and green on 073e05d)

[sehkone]

[Reviewer Round 1]

No blocking findings.

The implementation follows the issue's deliberately narrow contract: the default wrappers in src/commands/infra.rs and ComposeInvocation::command retain their callers while forwarding &Path to executable-taking siblings; the four in-scope spawn paths receive the value through the three specified context carriers; and InitRollback::docker() remains the sole None-to-default resolution point. I also checked that DOCKER_BIN is read only at the permitted default/construction sites and that the explicitly out-of-scope Docker spawns remain untouched.

The tests include end-to-end, self-contained fake executables for the rollback compose/restart path, the direct signal/restart spawns, and certificate issuance, so the seam is exercised without changing process-global environment state. PR hygiene is in order as well: it closes #807, links the parent work, and includes a checklist test plan.

[sehkone]

[Review Verdict Round 1: APPROVED]

[sehkone]

Suggested squash commit

Title

Make the docker executable a caller-supplied value

Body

Tests that substitute a fake `docker` write a script by that name into
a temporary directory and prepend it to `PATH`. In Rust 2024
`std::env::set_var` is `unsafe`, and the mutex those tests share does
not discharge its contract — it serialises only the code that takes
it, while a tokio runtime, an HTTP client and an env-filtered tracing
subscriber read the environment from threads that hold nothing. They
cannot stop mutating `PATH` until production offers another way to
name the executable, because production is what spawns the child.

This creates that seam and nothing else. The executable travels as a
`&Path` parameter named `docker`, or as a field where the carrier is a
method on a value a test already builds: `RotateContext::docker`,
`OpenBaoTlsTransition::docker`, and `InitRollback::docker`, whose
`Option` keeps a value taken from the derived `Default` from spawning
the empty path. `run_docker`, `run_compose`, `run_compose_with_env`
and `ComposeInvocation::command` keep their signatures and delegate to
executable-taking siblings, so every call site outside the carrier
table compiles untouched.

Behaviour with nothing supplied is unchanged: a bare program name is
still resolved against `PATH` by `Command` itself, and the argument
vectors, error contexts and instance pin are as they were. No test is
converted and no `PATH` mutation is removed here, and the four spawn
sites no conversion reaches stay hard-coded.

Closes #807

[sehkone]

Rebased onto main at 8c33567 ("Pass environment values into the resolvers").

One conflict, in src/commands/init/steps.rs (test_support module): main removed the ENV_LOCK static and its env_lock() helper as part of passing environment values into the resolvers, while this branch had added write_self_contained_fake_docker alongside them. Resolved by keeping main's removal and retaining the new helper, dropping the now-unused Mutex/MutexGuard/OnceLock imports. The env_lock references remaining in steps.rs and stepca_setup.rs resolve to commands::rotate::test_support::env_lock, which is unaffected.

git range-diff against the pre-rebase commit shows that absorbed removal as the only change; the rest of the commit is unchanged.

Verified locally: cargo fmt --check (with group_imports=StdExternalCrate), cargo clippy --all-targets -D warnings, cargo build, and the full cargo test suite — all green, 0 failures.