Skip to content

Convert the init-step and verify tests off process-global state (#809) - #824

Merged
AcoPiper merged 1 commit into
mainfrom
AcoPiper/issue-809
Aug 13, 2026
Merged

Convert the init-step and verify tests off process-global state (#809)#824
AcoPiper merged 1 commit into
mainfrom
AcoPiper/issue-809

Conversation

@AcoPiper

@AcoPiper AcoPiper commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Converts the last nine init step tests and the six verify.rs lock-scaffolded tests off process-global state, then deletes the machinery that kept it alive. No env::set_var/env::remove_var under #[cfg(test)] remains in the crate.

Closes #809

Changes

The init step tree. The nine tests that wrote a fake docker into a tempdir and prepended it to PATH now write a self-contained fake with init::steps::test_support::write_self_contained_fake_docker and 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 for issue_openbao_tls_cert_aborts_when_the_chown_fails, which needs a non-zero fake; the zero-exit writer is now that function's exit_code = 0 case. 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 local write_appending_fake_docker goes 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 manual runtime.block_on for #[tokio::test], since the comment justifying it named the environment lock. No assertion in the init tree changes form.

One test is deleted: restarting_the_stepca_sidecar_names_the_container_it_is_given. Converted, it was line-for-line restarting_the_stepca_sidecar_runs_the_supplied_executable beside 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_path now takes the PATH value (Option<&OsStr>, so an unset variable stays distinguishable in the error) and the directory an empty entry means; both thread through resolve_agent_binary. The single production caller reads std::env::var_os("PATH") once and passes Path::new("."), so the three resolution stages and the empty-entry semantics are unchanged. That is what lets ENV_LOCK, the fourteen mutation sites, the ten unsafe blocks and the set_current_dir pair go. The current_exe() sibling stage is untouched. One assertion changes: resolve_agent_binary_uses_cwd_for_empty_path_segment names the agent inside its own tempdir; the relative ./bootroot-agent rendering stays pinned by resolve_agent_binary_error_lists_empty_path_segment_as_cwd, which keeps the production . base.

Legacy machinery. rotate::test_support's ENV_LOCK, TEST_DOCKER_ARGS_ENV, ScopedEnvVar, env_lock(), write_fake_docker_script and path_with_prepend are deleted, the module doc no longer claims the init tree shares the harness, and pub(super) mod test_support narrows to mod test_support. test_messages, the self-contained writers and decode_fake_docker_log stay. No production behaviour changes and the diff adds no unsafe block.

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 an ifconfig-backed shim with OPENBAO_BIND_HOST/STEPCA_BIND_HOST pointed at a real interface, and passed.

The matrix then stops at openbao-tls-reown, which refuses to start without non-interactive sudo (sudo -n). That gate also blocks the two hosts-mode lifecycles and the two-instance stage behind it, and test-core fails separately because it writes /etc/hosts inside the non-root step-ca container — dead code regardless, since docker-compose.test.yml already 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-instance and 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 bootroot and cargo test --lib both pass
  • grep -rn "set_var\|remove_var" src returns exactly two lines: src/commands/dotenv.rs:111 and the rustdoc mention in src/config.rs:280
  • grep -rn "ENV_LOCK\|env_lock\|ScopedEnvVar\|path_with_prepend\|write_fake_docker_script\|TEST_DOCKER_ARGS_ENV\|BOOTROOT_TEST_DOCKER\|set_current_dir" src returns nothing
  • grep -rn '"PATH"' src returns exactly one line, in src/commands/verify.rs
  • grep -rn "unsafe {" src returns exactly thirteen lines: ten in src/cert_group.rs, the dotenv.rs production call, and the two libc::umask calls in orchestrator.rs; grep -rn "unsafe fn\|unsafe impl\|unsafe trait" src returns nothing
  • Reading the diff confirms it introduces no unsafe block
  • For at least one converted test per file, corrupting the expected argv or pointing the fake somewhere wrong makes the assertion fail — and all three negative tests fail when production is made to run docker where it should not
  • cargo clippy --all-targets -- -D warnings is clean
  • cargo fmt -- --check --config group_imports=StdExternalCrate is clean
  • CI green, including the Docker E2E matrix and the extended E2E workflow — the four preflight stages this host could not run

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
@AcoPiper

Copy link
Copy Markdown
Contributor Author

[Reviewer Round 1]

No blocking findings. The resolver now receives both the captured PATH value and the empty-entry base, while run_verify remains the sole composition boundary that reads PATH (src/commands/verify.rs:38-44); its override, sibling, and search-path lookup order remains intact. The tests keep the important empty-entry behavior without changing this process working directory.

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 set_var and the PATH read in verify.rs.

@AcoPiper

Copy link
Copy Markdown
Contributor Author

[Review Verdict Round 1: APPROVED]

@AcoPiper
AcoPiper merged commit 2313fcf into main Aug 13, 2026
21 checks passed
@AcoPiper
AcoPiper deleted the AcoPiper/issue-809 branch August 13, 2026 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Convert the init-step and verify tests, and remove the last environment mutation

1 participant