Clean command: removes installation from state if directory missing.

crates/criticalup-cli/src/arg.rs

@@ -24,7 +24,7 @@ pub(crate) struct Instrumentation {
     pub(crate) log_level: Vec<Directive>,
 }
 
-impl<'a> Instrumentation {
+impl Instrumentation {
     pub(crate) fn log_level(&self) -> String {
         match self.verbose {
             0 => "info",
@@ -34,7 +34,7 @@ impl<'a> Instrumentation {
         .to_string()
     }
 
-    pub(crate) async fn setup<'b: 'a>(&'b self, binary_name: &str) -> Result<(), crate::Error> {
+    pub(crate) async fn setup(&self, binary_name: &str) -> Result<(), crate::Error> {
         let filter_layer = self.filter_layer(binary_name)?;
 
         if self.verbose != 0 {

crates/criticalup-cli/src/commands/clean.rs

@@ -35,10 +35,23 @@ async fn delete_cache_directory(cache_dir: &Path) -> Result<(), Error> {
 /// Deletes installation from `State` with `InstallationId`s that have empty manifest section, and
 /// deletes the installation directory from the disk if present.
 async fn delete_unused_installations(installations_dir: &Path, state: &State) -> Result<(), Error> {
+    // We need to list all the available installations on the disk first, so we can check which
+    // installations in state file are absent from the disk.
+    let mut all_installations_on_disk: Vec<InstallationId> = Vec::new();
+    let mut entries = fs::read_dir(installations_dir).await?;
+    while let Some(item) = entries.next_entry().await? {
+        if item.file_type().await?.is_dir() {
+            let installation_dir_name = item.file_name();
+            if let Some(name) = installation_dir_name.to_str() {
+                all_installations_on_disk.push(InstallationId(name.into()));
+            }
+        }
+    }
+
     let unused_installations: Vec<InstallationId> = state
         .installations()
         .iter()
-        .filter(|item| item.1.manifests().is_empty())
+        .filter(|item| item.1.manifests().is_empty() || !all_installations_on_disk.contains(item.0))
         .map(|item| item.0.to_owned())
         .collect();
 
@@ -59,8 +72,8 @@ async fn delete_unused_installations(installations_dir: &Path, state: &State) ->
         // Remove installation directory from physical location.
         let installation_dir_to_delete = installations_dir.join(&installation.0);
         if installation_dir_to_delete.exists() {
-            tracing::info!(
-                "deleting unused installation directory {}",
+            tracing::debug!(
+                "Deleting unused installation directory {}",
                 &installation_dir_to_delete.display()
             );
             fs::remove_dir_all(&installation_dir_to_delete)
@@ -90,7 +103,7 @@ async fn delete_untracked_installation_dirs(
                 if !installations_in_state.contains_key(&InstallationId(name.into())) {
                     are_untracked_installation_dirs_present = true;
                     tracing::info!(
-                        "deleting untracked installation directory {}",
+                        "Deleting untracked installation directory {}",
                         item.path().to_path_buf().display()
                     );
 

crates/criticalup-cli/tests/cli/auth_set.rs

@@ -36,9 +36,12 @@ macro_rules! run_cmd {
             .expect("failed to execute command");
         match &$expected {
             Some(expected) => {
-                // this regex replacement dance is required because this nested macro tests
+                // Since 1.84, stable Rust Clippy will complain about Regex inside a for loop.
+                // Unblocking ourselves by ignoring this warning in tests.
+                #[allow(clippy::regex_creation_in_loops)]
+                // This regex replacement dance is required because this nested macro tests
                 // set is instantiating the test server twice which means each run gives
-                // a different local port. we replace with a stable port just for this test.
+                // a different local port. We replace with a stable port just for this test.
                 let re = Regex::new(r"127.0.0.1:\d+").expect("regex creation failed.");
                 let left_str = String::from_utf8(out.stderr.clone())
                     .expect("string creation from bytes failed.");

crates/criticalup-cli/tests/cli/clean.rs

@@ -17,13 +17,16 @@ async fn help_message() {
 #[tokio::test]
 async fn clean_deletes_only_unused_installations() {
     let test_env = TestEnvironment::prepare().await;
-
+    let root = test_env.root();
+    let toolchains_dir = root.join("toolchains");
+    fs::create_dir_all(&toolchains_dir).unwrap();
     let installation_id_1 = "installation_id_1";
     let installation_id_2 = "installation_id_2";
     let installation_id_3 = "installation_id_3";
 
     let root = test_env.root().join("state.json");
     let mut state_file = std::fs::File::create(&root).unwrap();
+
     let content = json!({
         "version": 1,
         "authentication_token": "criticalup_token_45_hahaha",
@@ -54,6 +57,14 @@ async fn clean_deletes_only_unused_installations() {
     .to_string();
     state_file.write_all(content.as_bytes()).unwrap();
 
+    fs::create_dir_all(toolchains_dir.join(installation_id_1)).unwrap();
+    fs::create_dir_all(toolchains_dir.join(installation_id_2)).unwrap();
+    fs::create_dir_all(toolchains_dir.join(installation_id_3)).unwrap();
+
+    assert!(toolchains_dir.join(installation_id_1).exists());
+    assert!(toolchains_dir.join(installation_id_2).exists());
+    assert!(toolchains_dir.join(installation_id_3).exists());
+
     assert_output!(test_env.cmd().args(["clean"]));
 
     let state_file_actual: Value =
@@ -255,3 +266,93 @@ async fn removes_unused_installations_from_disk_that_do_not_have_state() {
     assert!(!toolchains_dir.join(installation_id_2).exists()); // Does not exist.
     assert!(!toolchains_dir.join(installation_id_3).exists()); // Does not exist.
 }
+
+/// Remove the installation from the state, if the installation directory does not exist. This is
+/// true even if the installation in the state has a manifest.
+#[tokio::test]
+async fn clean_deletes_only_unused_installations_that_are_not_on_disk() {
+    let test_env = TestEnvironment::prepare().await;
+    let root = test_env.root();
+    let toolchains_dir = root.join("toolchains");
+    fs::create_dir_all(&toolchains_dir).unwrap();
+
+    let installation_id_1 = "installation_id_1";
+    let installation_id_2 = "installation_id_2";
+    let installation_id_3 = "installation_id_3";
+
+    let state_file_in_root = root.join("state.json");
+    let mut state_file = std::fs::File::create(&state_file_in_root).unwrap();
+    let content = json!({
+        "version": 1,
+        "authentication_token": "criticalup_token_45_hahaha",
+        "installations": {
+            installation_id_1: {
+                "binary_proxies": {
+                    "cargo": "/path/toolchains/bin/cargo"
+                },
+                "manifests": [
+                    "/path/to/proj/1/criticalup.toml",
+                    "/path/to/proj/2/criticalup.toml"
+                ]
+            },
+            installation_id_2: {
+                "binary_proxies": {
+                    "cargo": "/path/toolchains/bin/cargo"
+                },
+                "manifests": ["/path/to/proj/3/criticalup.toml"]
+            },
+            installation_id_3: {
+                "binary_proxies": {
+                    "cargo": "/path/toolchains/bin/rustc"
+                },
+                "manifests": []
+            }
+        }
+    })
+    .to_string();
+    state_file.write_all(content.as_bytes()).unwrap();
+
+    // Create only one of the corresponding physical directories of installations. In this case we
+    // create only Installation ID 1.
+    // TODO: We have to generate these by running `install` command, once tests for those are setup.
+    fs::create_dir_all(toolchains_dir.join(installation_id_1)).unwrap();
+
+    assert!(toolchains_dir.join(installation_id_1).exists());
+    assert!(!toolchains_dir.join(installation_id_2).exists());
+    assert!(!toolchains_dir.join(installation_id_3).exists());
+
+    // Run the `clean` command.
+    assert_output!(test_env.cmd().args(["clean"]));
+
+    // Test the actual values.
+    let state_file_actual: Value =
+        serde_json::from_reader(BufReader::new(File::open(&state_file_in_root).unwrap())).unwrap();
+
+    // "installation_id_2" is not present.
+    assert_eq!(
+        state_file_actual.pointer("/installations/installation_id_2"),
+        None
+    );
+    // "installation_id_3" is not present.
+    assert_eq!(
+        state_file_actual.pointer("/installations/installation_id_3"),
+        None
+    );
+    // "installation_id_1" is still present with correct values.
+    assert_eq!(
+        state_file_actual
+            .pointer("/installations/installation_id_1")
+            .unwrap(),
+        &json!({
+            "binary_proxies": {
+                "cargo": "/path/toolchains/bin/cargo"
+            },
+            "manifests": [
+                "/path/to/proj/1/criticalup.toml",
+                "/path/to/proj/2/criticalup.toml"
+            ]
+        })
+    );
+
+    assert!(toolchains_dir.join(installation_id_1).exists());
+}

crates/criticalup-cli/tests/snapshots/cli__clean__clean_deletes_only_unused_installations.snap

@@ -2,13 +2,13 @@
 source: crates/criticalup-cli/tests/cli/clean.rs
 expression: repr
 ---
-exit: exit status: 1
+exit: exit status: 0
 
 empty stdout
 
 stderr
 ------
  INFO Deleting unused installation installation_id_2
  INFO Deleting unused installation installation_id_3
-error: No such file or directory (os error 2)
+ INFO no untracked installation directories found
 ------

crates/criticalup-cli/tests/snapshots/cli__clean__clean_deletes_only_unused_installations_also_from_disk.snap

@@ -1,7 +1,6 @@
 ---
 source: crates/criticalup-cli/tests/cli/clean.rs
 expression: repr
-snapshot_kind: text
 ---
 exit: exit status: 0
 
@@ -10,8 +9,6 @@ empty stdout
 stderr
 ------
  INFO Deleting unused installation installation_id_2
- INFO deleting unused installation directory /path/to/toolchain/installation/installation_id_2/
  INFO Deleting unused installation installation_id_3
- INFO deleting unused installation directory /path/to/toolchain/installation/installation_id_3/
  INFO no untracked installation directories found
 ------

crates/criticalup-cli/tests/snapshots/cli__clean__clean_deletes_only_unused_installations_that_are_not_on_disk.snap

@@ -0,0 +1,14 @@
+---
+source: crates/criticalup-cli/tests/cli/clean.rs
+expression: repr
+---
+exit: exit status: 0
+
+empty stdout
+
+stderr
+------
+ INFO Deleting unused installation installation_id_2
+ INFO Deleting unused installation installation_id_3
+ INFO no untracked installation directories found
+------

crates/criticalup-cli/tests/snapshots/cli__clean__removes_unused_installations_from_disk_that_do_not_have_state.snap

@@ -9,6 +9,5 @@ empty stdout
 stderr
 ------
  INFO Deleting unused installation installation_id_2
- INFO deleting unused installation directory /path/to/toolchain/installation/installation_id_2/
- INFO deleting untracked installation directory /path/to/toolchain/installation/installation_id_3/
+ INFO Deleting untracked installation directory /path/to/toolchain/installation/installation_id_3/
 ------