fix: iroh start service set construction (#522)

also, use a BTreeSet for services, makes output deterministic

iroh/src/fixture.rs

@@ -152,6 +152,32 @@ fn fixture_get_unwrapped_symlink() -> Api {
     api
 }
 
+fn fixture_start_status_stop() -> Api {
+    let mut api = Api::default();
+    api.expect_check().returning(|| {
+        StatusTable::new(
+            Some(StatusRow::new("gateway", 1, ServiceStatus::Serving)),
+            Some(StatusRow::new("p2p", 1, ServiceStatus::Serving)),
+            Some(StatusRow::new("store", 1, ServiceStatus::Serving)),
+        )
+    });
+    api.expect_check().returning(|| {
+        StatusTable::new(
+            Some(StatusRow::new("gateway", 1, ServiceStatus::Serving)),
+            Some(StatusRow::new("p2p", 1, ServiceStatus::Serving)),
+            Some(StatusRow::new("store", 1, ServiceStatus::Serving)),
+        )
+    });
+    api.expect_check().returning(|| {
+        StatusTable::new(
+            Some(StatusRow::new("gateway", 1, ServiceStatus::Unknown)),
+            Some(StatusRow::new("p2p", 1, ServiceStatus::Unknown)),
+            Some(StatusRow::new("store", 1, ServiceStatus::Unknown)),
+        )
+    });
+    api
+}
+
 fn register_fixtures() -> FixtureRegistry {
     [
         ("lookup".to_string(), fixture_lookup as GetFixture),
@@ -177,6 +203,10 @@ fn register_fixtures() -> FixtureRegistry {
             "get_unwrapped_symlink".to_string(),
             fixture_get_unwrapped_symlink as GetFixture,
         ),
+        (
+            "start_status_stop".to_string(),
+            fixture_start_status_stop as GetFixture,
+        ),
     ]
     .into_iter()
     .collect()

iroh/src/run.rs

@@ -1,4 +1,4 @@
-use std::collections::{HashMap, HashSet};
+use std::collections::{BTreeSet, HashMap};
 use std::path::{Path, PathBuf};
 
 use anyhow::{Context, Result};
@@ -187,7 +187,7 @@ async fn add(api: &Api, path: &Path, no_wrap: bool, recursive: bool, provide: bo
     // hydrating only the root CID to the p2p node for providing if a CID were
     // ingested offline. Offline adding should happen, but this is the current
     // path of least confusion
-    let svc_status = require_services(api, HashSet::from(["store"])).await?;
+    let svc_status = require_services(api, BTreeSet::from(["store"])).await?;
     match (provide, svc_status.p2p.status()) {
         (true, ServiceStatus::Down(_status)) => {
             anyhow::bail!("Add provides content to the IPFS network by default, but the p2p service is not running.\n{}",

iroh/src/services.rs

@@ -3,7 +3,7 @@ use crossterm::terminal::{Clear, ClearType};
 use crossterm::{cursor, style, style::Stylize, QueueableCommand};
 use futures::StreamExt;
 use iroh_util::iroh_cache_path;
-use std::collections::HashSet;
+use std::collections::BTreeSet;
 use std::io::{stdout, Write};
 use std::ops::Deref;
 use std::time::SystemTime;
@@ -18,9 +18,9 @@ const SERVICE_START_TIMEOUT_SECONDS: u64 = 15;
 /// Start any given services that aren't currently running.
 pub async fn start(api: &Api, services: &Vec<String>) -> Result<()> {
     let services = match services.is_empty() {
-        true => HashSet::from(["gateway", "store"]),
+        true => BTreeSet::from(["gateway", "store", "p2p"]),
         false => {
-            let mut hs: HashSet<&str> = HashSet::new();
+            let mut hs: BTreeSet<&str> = BTreeSet::new();
             for s in services {
                 hs.insert(s.as_str());
             }
@@ -31,12 +31,12 @@ pub async fn start(api: &Api, services: &Vec<String>) -> Result<()> {
 }
 
 // TODO(b5) - should check for configuration mismatch between iroh CLI configuration
-// TODO(b5) - services HashSet should be an enum
-async fn start_services(api: &Api, services: HashSet<&str>) -> Result<()> {
+// TODO(b5) - services BTreeSet should be an enum
+async fn start_services(api: &Api, services: BTreeSet<&str>) -> Result<()> {
     // check for any running iroh services
     let table = api.check().await;
 
-    let mut expected_services = HashSet::new();
+    let mut expected_services = BTreeSet::new();
     let expected_services = table
         .iter()
         .fold(&mut expected_services, |accum, status_row| {
@@ -49,7 +49,8 @@ async fn start_services(api: &Api, services: HashSet<&str>) -> Result<()> {
             accum
         });
 
-    let unknown_services: HashSet<&str> = services.difference(expected_services).copied().collect();
+    let unknown_services: BTreeSet<&str> =
+        services.difference(expected_services).copied().collect();
 
     if !unknown_services.is_empty() {
         let u = unknown_services.into_iter().collect::<Vec<&str>>();
@@ -60,7 +61,7 @@ async fn start_services(api: &Api, services: HashSet<&str>) -> Result<()> {
         return Err(anyhow!("{} {}.", e, u.join(", ")));
     }
 
-    let mut missing_services = HashSet::new();
+    let mut missing_services = BTreeSet::new();
     let missing_services = table
         .iter()
         .fold(&mut missing_services, |accum, status_row| {
@@ -81,8 +82,8 @@ async fn start_services(api: &Api, services: HashSet<&str>) -> Result<()> {
             accum
         });
 
-    let missing_services: HashSet<&str> = services
-        .difference(missing_services)
+    let missing_services: BTreeSet<&str> = services
+        .intersection(missing_services)
         .map(Deref::deref)
         .collect();
 
@@ -133,9 +134,9 @@ async fn start_services(api: &Api, services: HashSet<&str>) -> Result<()> {
 /// identified by lockfiles
 pub async fn stop(api: &Api, services: &Vec<String>) -> Result<()> {
     let services = match services.is_empty() {
-        true => HashSet::from(["store", "p2p", "gateway"]),
+        true => BTreeSet::from(["store", "p2p", "gateway"]),
         false => {
-            let mut hs: HashSet<&str> = HashSet::new();
+            let mut hs: BTreeSet<&str> = BTreeSet::new();
             for s in services {
                 hs.insert(s.as_str());
             }
@@ -145,7 +146,7 @@ pub async fn stop(api: &Api, services: &Vec<String>) -> Result<()> {
     stop_services(api, services).await
 }
 
-pub async fn stop_services(api: &Api, services: HashSet<&str>) -> Result<()> {
+pub async fn stop_services(api: &Api, services: BTreeSet<&str>) -> Result<()> {
     for service in services {
         let daemon_name = format!("iroh-{}", service);
         info!("checking daemon {} lock", daemon_name);
@@ -276,7 +277,10 @@ where
 
 /// require a set of services is up. returns the underlying status table of all
 /// services for additional scrutiny
-pub async fn require_services(api: &Api, services: HashSet<&str>) -> Result<iroh_api::StatusTable> {
+pub async fn require_services(
+    api: &Api,
+    services: BTreeSet<&str>,
+) -> Result<iroh_api::StatusTable> {
     let table = api.check().await;
     for service in table.iter() {
         if services.contains(service.name()) && service.status() != iroh_api::ServiceStatus::Serving

iroh/tests/cli_tests.rs

@@ -143,3 +143,11 @@ fn version_test() {
         .case("tests/cmd/version.trycmd")
         .run();
 }
+
+#[test]
+fn start_status_stop_test() {
+    trycmd::TestCases::new()
+        .env("IROH_CTL_FIXTURE", "start_status_stop")
+        .case("tests/cmd/start_status_stop.trycmd")
+        .run();
+}

iroh/tests/cmd/start_status_stop.trycmd

@@ -0,0 +1,16 @@
+```
+$ iroh start
+All iroh daemons are already running. all systems normal.
+
+$ iroh status
+Service			Number	Status
+gateway			1/1	Serving
+p2p			1/1	Serving
+store			1/1	Serving
+
+$ iroh stop
+iroh-gateway is already stopped
+iroh-p2p is already stopped
+iroh-store is already stopped
+
+```