Convert the init-step and verify tests off process-global state (#809) - #824
Conversation
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
|
[Reviewer Round 1] No blocking findings. The resolver now receives both the captured The nine init conversions consistently name a self-contained fake through the existing docker seam. In particular, the negative tests still supply fakes that would log, so an absent log remains evidence that no invocation occurred (src/commands/init/steps.rs:893-925 and src/commands/init/steps/openbao_transition.rs:581-612). The non-zero fake variant preserves the init tree’s existing appended, space-joined log format. I also confirmed the removed StepCA test duplicates the retained supplied-executable test (src/commands/init/steps/stepca_setup.rs:587-603), and the legacy environment scaffolding is gone. The acceptance searches leave only the intentional production |
|
[Review Verdict Round 1: APPROVED] |
Converts the last nine
initstep tests and the sixverify.rslock-scaffolded tests off process-global state, then deletes the machinery that kept it alive. Noenv::set_var/env::remove_varunder#[cfg(test)]remains in the crate.Closes #809
Changes
The init step tree. The nine tests that wrote a fake
dockerinto a tempdir and prepended it toPATHnow write a self-contained fake withinit::steps::test_support::write_self_contained_fake_dockerand hand production that path through the executable seam it already carried (run_docker_with_exec/run_compose_with_exec,InitRollback::docker,OpenBaoTlsTransition::docker,restart_stepca_openbao_agent's direct spawn). No new injection mechanism, and the seam was not extended — every call site already took the executable.write_self_contained_fake_docker_exiting(path, args_log, exit_code)is added beside the existing writer forissue_openbao_tls_cert_aborts_when_the_chown_fails, which needs a non-zero fake; the zero-exit writer is now that function'sexit_code = 0case. It differs in exit status and nothing else — same appended, space-joined line per invocation, same single-quoted log path with the same'-free assertion.openbao_tls.rs's localwrite_appending_fake_dockergoes away with its last caller.The three negative tests (
rollback_skips_openbao_recreate_when_this_run_did_not_recreate,no_available_source_fails_before_any_docker_call,issue_openbao_tls_cert_refuses_a_symlinked_output_dir) still hand production a fake that would log, so an absent or empty log remains evidence.openbao_transition.rs's pair drop their manualruntime.block_onfor#[tokio::test], since the comment justifying it named the environment lock. No assertion in theinittree changes form.One test is deleted:
restarting_the_stepca_sidecar_names_the_container_it_is_given. Converted, it was line-for-linerestarting_the_stepca_sidecar_runs_the_supplied_executablebeside it — same call, same container name, same fake, same argv assertion — so the older one goes and the newer keeps the coverage. This is the only test that disappears.verify.rs.find_on_pathnow takes thePATHvalue (Option<&OsStr>, so an unset variable stays distinguishable in the error) and the directory an empty entry means; both thread throughresolve_agent_binary. The single production caller readsstd::env::var_os("PATH")once and passesPath::new("."), so the three resolution stages and the empty-entry semantics are unchanged. That is what letsENV_LOCK, the fourteen mutation sites, the tenunsafeblocks and theset_current_dirpair go. Thecurrent_exe()sibling stage is untouched. One assertion changes:resolve_agent_binary_uses_cwd_for_empty_path_segmentnames the agent inside its own tempdir; the relative./bootroot-agentrendering stays pinned byresolve_agent_binary_error_lists_empty_path_segment_as_cwd, which keeps the production.base.Legacy machinery.
rotate::test_support'sENV_LOCK,TEST_DOCKER_ARGS_ENV,ScopedEnvVar,env_lock(),write_fake_docker_scriptandpath_with_prependare deleted, the module doc no longer claims theinittree shares the harness, andpub(super) mod test_supportnarrows tomod test_support.test_messages, the self-contained writers anddecode_fake_docker_logstay. No production behaviour changes and the diff adds nounsafeblock.Preflight: what was verified locally, and what is left to CI
The suite does not pass in full on this macOS host, but every stage the host can reach is green. Verified locally:
ci/check.sh(rustfmt, clippy, Python, Biome, markdown, docs build,cargo audit),validate-deploy-compose.sh,validate-compose-instance-names.sh,extra/deploy-no-build-smoke.sh, and six of the ten E2E matrix stages — the local lifecycle (no-hosts), the remote lifecycle (no-hosts), the rotation/recovery full matrix, reinit-recovery, step-ca certificate SANs and the OpenBao TLS no-delta transition.The last three of those need
ip -4 -o addr show, which macOS lacks; they were run against anifconfig-backed shim withOPENBAO_BIND_HOST/STEPCA_BIND_HOSTpointed at a real interface, and passed.The matrix then stops at
openbao-tls-reown, which refuses to start without non-interactivesudo(sudo -n). That gate also blocks the two hosts-mode lifecycles and thetwo-instancestage behind it, andtest-corefails separately because it writes/etc/hostsinside the non-root step-ca container — dead code regardless, sincedocker-compose.test.ymlalready carries the three network aliases. None of these are related to this change.Everything the host could not run is covered by CI on this PR, all green: the full Docker E2E matrix, including
openbao-tls-reown,two-instanceand both hosts-mode lifecycles. The extended E2E workflow never triggers on a pull request, so it was dispatched against this branch separately and passed — run 31664260453.Test plan
cargo test --bin bootrootandcargo test --libboth passgrep -rn "set_var\|remove_var" srcreturns exactly two lines:src/commands/dotenv.rs:111and the rustdoc mention insrc/config.rs:280grep -rn "ENV_LOCK\|env_lock\|ScopedEnvVar\|path_with_prepend\|write_fake_docker_script\|TEST_DOCKER_ARGS_ENV\|BOOTROOT_TEST_DOCKER\|set_current_dir" srcreturns nothinggrep -rn '"PATH"' srcreturns exactly one line, insrc/commands/verify.rsgrep -rn "unsafe {" srcreturns exactly thirteen lines: ten insrc/cert_group.rs, thedotenv.rsproduction call, and the twolibc::umaskcalls inorchestrator.rs;grep -rn "unsafe fn\|unsafe impl\|unsafe trait" srcreturns nothingunsafeblockcargo clippy --all-targets -- -D warningsis cleancargo fmt -- --check --config group_imports=StdExternalCrateis clean