swarm-controller: make the homeserver default fn pure, fixing test race

homeserver_or_configured_default read DEFAULT_HOMESERVER_ENV internally,
so its three unit tests raced each other by set_var/remove_var-ing the
same process env var with no synchronization under cargo test's default
parallelism (argus, PR #4443 review).

Take the default as a plain parameter instead of reading the env var
inside the function. The one env read moves to a new
configured_default_homeserver() helper, called once at the edge
(main.rs's startup diagnostic); homeserver_or_configured_default itself
is now pure and its tests need no env mutation at all.

Refs #4345

swarm-controller/src/main.rs

@@ -1625,7 +1625,12 @@ async fn main() -> Result<()> {
     // own doc comment), so this cannot change any request's outcome. Logged
     // so an operator can tell the config landed before the slice that
     // consults it does.
-    if matrix_account::homeserver_or_configured_default(None).is_none() {
+    if matrix_account::homeserver_or_configured_default(
+        None,
+        matrix_account::configured_default_homeserver(),
+    )
+    .is_none()
+    {
         tracing::debug!(
             "{}; matrix-account requests still need their own homeserver",
             matrix_account::DEFAULT_HOMESERVER_ENV

swarm-controller/src/matrix_account.rs

@@ -38,18 +38,36 @@ fn default_mode() -> String {
 /// Env var the controller's NixOS module sets from
 /// `services.hyperhive.deploy.swarm-controller.matrixHomeserverUrl` — the
 /// swarm-wide default `PutMatrixAccountRequest::homeserver` falls back to
-/// when a caller omits one.
+/// when a caller omits one. Read via [`configured_default_homeserver`], not
+/// directly — see that fn's doc.
 ///
 /// Not yet consulted by [`put_matrix_account`]: `homeserver_or_configured_default`
 /// below exists for a later slice of hyperhive#4345 to call; this one only
 /// wires the config through.
 pub(crate) const DEFAULT_HOMESERVER_ENV: &str = "SWARM_CONTROLLER_MATRIX_HOMESERVER_URL";
 
-/// `caller`'s own homeserver, or [`DEFAULT_HOMESERVER_ENV`] when the caller
+/// `caller`'s own homeserver, or `default` when the caller left it unset.
-/// left it unset. `caller` always wins — this only fills a gap it left,
+/// `caller` always wins — this only fills a gap it left, never replaces a
-/// never replaces a value it gave.
+/// value it gave.
-pub(crate) fn homeserver_or_configured_default(caller: Option<String>) -> Option<String> {
+///
-    caller.or_else(|| std::env::var(DEFAULT_HOMESERVER_ENV).ok())
+/// Takes `default` as a plain parameter rather than reading
+/// [`DEFAULT_HOMESERVER_ENV`] itself: a caller wants that env read done once,
+/// at the edge (see [`configured_default_homeserver`]), and keeping this fn
+/// pure makes it testable without mutating shared process env — three tests
+/// doing exactly that raced each other under `cargo test`'s default
+/// parallelism in review.
+pub(crate) fn homeserver_or_configured_default(
+    caller: Option<String>,
+    default: Option<String>,
+) -> Option<String> {
+    caller.or(default)
+}
+
+/// Reads [`DEFAULT_HOMESERVER_ENV`] fresh each call — the one place this
+/// crate touches that env var, so [`homeserver_or_configured_default`] above
+/// can stay a pure function.
+pub(crate) fn configured_default_homeserver() -> Option<String> {
+    std::env::var(DEFAULT_HOMESERVER_ENV).ok()
 }
 
 /// The credential to store for one agent's external matrix account.
@@ -356,8 +374,8 @@ async fn matrix_password_login(
 #[cfg(test)]
 mod tests {
     use super::{
-        DEFAULT_HOMESERVER_ENV, PutMatrixAccountRequest, homeserver_or_configured_default,
+        PutMatrixAccountRequest, homeserver_or_configured_default, is_reserved_account,
-        is_reserved_account, password_fields, resolve_credential, token_credential,
+        password_fields, resolve_credential, token_credential,
     };
 
     fn request(mode: &str) -> PutMatrixAccountRequest {
@@ -370,41 +388,30 @@ mod tests {
         }
     }
 
-    /// SAFETY: single-threaded mutation of a process env var no other test
+    // `homeserver_or_configured_default` takes its default as a plain
-    /// in this crate reads; restored (removed) before returning.
+    // parameter rather than reading the env var itself, so these are pure
+    // — no process env mutation, and so no risk of racing each other (or
+    // any other test in the crate) under `cargo test`'s default parallelism.
+
     #[test]
     fn caller_supplied_homeserver_wins_even_with_a_default_configured() {
-        unsafe {
+        let result = homeserver_or_configured_default(
-            std::env::set_var(DEFAULT_HOMESERVER_ENV, "https://default.example.org");
+            Some("https://caller.example.org".to_owned()),
-        }
+            Some("https://default.example.org".to_owned()),
-        let result =
+        );
-            homeserver_or_configured_default(Some("https://caller.example.org".to_owned()));
-        unsafe {
-            std::env::remove_var(DEFAULT_HOMESERVER_ENV);
-        }
         assert_eq!(result.as_deref(), Some("https://caller.example.org"));
     }
 
-    /// SAFETY: same as above.
     #[test]
     fn falls_back_to_the_configured_default_when_the_caller_omits_one() {
-        unsafe {
+        let result =
-            std::env::set_var(DEFAULT_HOMESERVER_ENV, "https://default.example.org");
+            homeserver_or_configured_default(None, Some("https://default.example.org".to_owned()));
-        }
-        let result = homeserver_or_configured_default(None);
-        unsafe {
-            std::env::remove_var(DEFAULT_HOMESERVER_ENV);
-        }
         assert_eq!(result.as_deref(), Some("https://default.example.org"));
     }
 
-    /// SAFETY: same as above.
     #[test]
     fn none_when_neither_caller_nor_default_is_set() {
-        unsafe {
+        assert_eq!(homeserver_or_configured_default(None, None), None);
-            std::env::remove_var(DEFAULT_HOMESERVER_ENV);
-        }
-        assert_eq!(homeserver_or_configured_default(None), None);
     }
 
     #[test]