chore(recovery): Improve error messages (#2122)

Added a suggestion for errors during creation of the recovery directory
folders.

I also tried to make the fatal recovery errors more readable.
Comparison:

**Before**:
<img width="573" alt="Screenshot 2024-10-18 at 00 32 16"
src="https://github.com/user-attachments/assets/b434d6f2-5688-4ef9-a7ac-8fd40db6f888">


**After**: 
<img width="572" alt="Screenshot 2024-10-18 at 00 31 34"
src="https://github.com/user-attachments/assets/380890d9-7001-4f68-b7a6-e508c90ed236">

Author: dist1ll

rs/recovery/src/app_subnet_recovery.rs

@@ -3,7 +3,7 @@ use crate::{
         consent_given, print_height_info, read_optional, read_optional_node_ids,
         read_optional_subnet_id, read_optional_version, wait_for_confirmation,
     },
-    error::RecoveryError,
+    error::{GracefulExpect, RecoveryError},
     recovery_iterator::RecoveryIterator,
     registry_helper::RegistryPollingStrategy,
     NeuronArgs, Recovery, RecoveryArgs, RecoveryResult, Step, CUPS_DIR,
@@ -168,7 +168,7 @@ impl AppSubnetRecovery {
             recovery_args.nns_url.clone(),
             RegistryPollingStrategy::OnlyOnInit,
         )
-        .expect("Failed to init recovery");
+        .expect_graceful("Failed to init recovery");
 
         Self {
             step_iterator: StepType::iter().peekable(),

rs/recovery/src/cli.rs

@@ -2,6 +2,7 @@
 use crate::{
     app_subnet_recovery::{AppSubnetRecovery, AppSubnetRecoveryArgs},
     args_merger::merge,
+    error::GracefulExpect,
     get_node_heights_from_metrics,
     nns_recovery_failover_nodes::{NNSRecoveryFailoverNodes, NNSRecoveryFailoverNodesArgs},
     nns_recovery_same_nodes::{NNSRecoverySameNodes, NNSRecoverySameNodesArgs},
@@ -299,7 +300,7 @@ pub fn read_and_maybe_update_state<T: Serialize + DeserializeOwned + Clone + Par
     subcommand_args: Option<T>,
 ) -> RecoveryState<T> {
     let state = RecoveryState::<T>::read(&recovery_args.dir)
-        .expect("Failed to read the recovery state file");
+        .expect_graceful("Failed to read the recovery state file");
 
     if let Some(state) = state {
         info!(

rs/recovery/src/error.rs

@@ -60,7 +60,6 @@ impl RecoveryError {
             e,
         )
     }
-
     pub fn validation_failed(message: impl Display, error: impl Display) -> Self {
         RecoveryError::ValidationFailed(format!("{}: {}", message, error))
     }
@@ -70,40 +69,57 @@ impl fmt::Display for RecoveryError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
             RecoveryError::IoError(msg, e) => {
-                write!(f, "IO error, message: {}, error: {}", msg, e)
+                write!(f, "IO error: {}\nError: {}", msg, e)
             }
             RecoveryError::CommandError(code, msg) => {
-                write!(f, "Command error, message: {}, code: {:?}", msg, code)
+                write!(f, "Command error: {}\nCode: {:?}", msg, code)
             }
             RecoveryError::OutputError(msg) => {
-                write!(f, "Output error, message: {}", msg)
+                write!(f, "Output error: {}", msg)
             }
             RecoveryError::DownloadError(msg, e) => {
-                write!(f, "Download error, message: {}, error: {}", msg, e)
+                write!(f, "Download error: {}\nError: {}", msg, e)
             }
             RecoveryError::UnexpectedError(msg) => {
-                write!(f, "Unexpected error, message: {}", msg)
+                write!(f, "Unexpected error: {}", msg)
             }
             RecoveryError::StepSkipped => {
                 write!(f, "Recovery step skipped.")
             }
             RecoveryError::ParsingError(e) => {
-                write!(f, "Parsing error, error: {}", e)
+                write!(f, "Parsing error: {}", e)
             }
             RecoveryError::SerializationError(e) => {
-                write!(f, "Serialization error, error: {}", e)
+                write!(f, "Serialization error: {}", e)
             }
             RecoveryError::CheckpointError(msg, e) => {
-                write!(f, "Checkpoint error, message: {}, error: {}", msg, e)
+                write!(f, "Checkpoint error: {}\nError: {}", msg, e)
             }
-            RecoveryError::RegistryError(msg) => write!(f, "Registry error, message: {}", msg),
-            RecoveryError::StateToolError(msg) => write!(f, "State tool error, message: {}", msg),
+            RecoveryError::RegistryError(msg) => write!(f, "Registry error: {}", msg),
+            RecoveryError::StateToolError(msg) => write!(f, "State tool error: {}", msg),
             RecoveryError::ValidationFailed(msg) => {
-                write!(f, "Validation failed, message: {}", msg)
+                write!(f, "Validation failed: {}", msg)
             }
-            RecoveryError::AgentError(msg) => write!(f, "ic-agent error, message: {}", msg),
+            RecoveryError::AgentError(msg) => write!(f, "ic-agent error: {}", msg),
         }
     }
 }
 
 impl Error for RecoveryError {}
+
+pub trait GracefulExpect<T> {
+    /// Print a human-readable error message, instead of a debug dump.
+    fn expect_graceful(self, context: &str) -> T;
+}
+
+impl<T> GracefulExpect<T> for RecoveryResult<T> {
+    fn expect_graceful(self, context: &str) -> T {
+        match self {
+            Ok(inner) => inner,
+            Err(e) => {
+                println!("\x1b[1;31mFatal error\x1B[0m: {context}\n{e}");
+                std::process::exit(1)
+            }
+        }
+    }
+}

rs/recovery/src/lib.rs

@@ -25,6 +25,7 @@ use registry_helper::RegistryPollingStrategy;
 use serde::{Deserialize, Serialize};
 use slog::{info, warn, Logger};
 use ssh_helper::SshHelper;
+use std::io::ErrorKind;
 use std::{
     net::IpAddr,
     path::{Path, PathBuf},
@@ -148,7 +149,22 @@ impl Recovery {
         let local_store_path = work_dir.join("data").join(IC_REGISTRY_LOCAL_STORE);
         let nns_pem = recovery_dir.join("nns.pem");
 
-        Recovery::create_dirs(&[&binary_dir, &data_dir, &work_dir, &local_store_path])?;
+        match Recovery::create_dirs(&[&binary_dir, &data_dir, &work_dir, &local_store_path]) {
+            Err(RecoveryError::IoError(s, err)) => match err.kind() {
+                ErrorKind::PermissionDenied => Err(RecoveryError::IoError(
+                    format!(
+                        "No permission to create recovery directory. Consider manually \
+                        creating the directory with the right permissions by running:\n\n  \
+                        sudo mkdir -p {} && sudo chown $USER {}\n",
+                        recovery_dir.display(),
+                        recovery_dir.display()
+                    ),
+                    err,
+                )),
+                _ => Err(RecoveryError::IoError(s, err)),
+            },
+            x => x,
+        }?;
 
         let registry_helper = RegistryHelper::new(
             logger.clone(),
@@ -976,6 +992,7 @@ pub fn get_member_ips(
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::error::GracefulExpect;
     use ic_test_utilities_tmpdir::tmpdir;
 
     #[test]
@@ -991,7 +1008,7 @@ mod tests {
 
         let (name, height) =
             Recovery::get_latest_checkpoint_name_and_height(checkpoints_dir.path())
-                .expect("Failed getting the latest checkpoint name and height");
+                .expect_graceful("Failed getting the latest checkpoint name and height");
 
         assert_eq!(name, "000000000000fd84");
         assert_eq!(height, Height::from(64900));

rs/recovery/src/nns_recovery_failover_nodes.rs

@@ -2,7 +2,7 @@ use crate::{
     admin_helper::RegistryParams,
     cli::{print_height_info, read_optional, read_optional_node_ids, read_optional_version},
     command_helper::pipe_all,
-    error::RecoveryError,
+    error::{GracefulExpect, RecoveryError},
     recovery_iterator::RecoveryIterator,
     registry_helper::RegistryPollingStrategy,
     NeuronArgs, Recovery, RecoveryArgs, RecoveryResult, Step, CUPS_DIR, IC_REGISTRY_LOCAL_STORE,
@@ -121,7 +121,7 @@ impl NNSRecoveryFailoverNodes {
             subnet_args.validate_nns_url.clone(),
             RegistryPollingStrategy::OnlyOnInit,
         )
-        .expect("Failed to init recovery");
+        .expect_graceful("Failed to init recovery");
 
         let new_registry_local_store = recovery.work_dir.join(IC_REGISTRY_LOCAL_STORE);
         Self {

rs/recovery/src/nns_recovery_same_nodes.rs

@@ -1,6 +1,6 @@
 use crate::{
     cli::{print_height_info, read_optional, read_optional_version},
-    error::RecoveryError,
+    error::{GracefulExpect, RecoveryError},
     file_sync_helper::create_dir,
     recovery_iterator::RecoveryIterator,
     registry_helper::RegistryPollingStrategy,
@@ -96,10 +96,10 @@ impl NNSRecoverySameNodes {
             recovery_args.nns_url.clone(),
             RegistryPollingStrategy::OnlyOnInit,
         )
-        .expect("Failed to init recovery");
+        .expect_graceful("Failed to init recovery");
 
         let new_state_dir = recovery.work_dir.join("new_ic_state");
-        create_dir(&new_state_dir).expect("Failed to create state directory for upload.");
+        create_dir(&new_state_dir).expect_graceful("Failed to create state directory for upload.");
         Self {
             step_iterator: StepType::iter().peekable(),
             params: subnet_args,

rs/recovery/src/recovery_state.rs

@@ -130,6 +130,7 @@ mod tests {
     use std::{fs, str::FromStr};
 
     use crate::app_subnet_recovery::AppSubnetRecoveryArgs;
+    use crate::error::GracefulExpect;
 
     use super::*;
     use ic_base_types::{PrincipalId, SubnetId};
@@ -146,7 +147,7 @@ mod tests {
         assert!(tmp.path().join("recovery/recovery_state.json").exists());
 
         let deserialized_state =
-            RecoveryState::read(tmp.path()).expect("Failed to deserialize the state");
+            RecoveryState::read(tmp.path()).expect_graceful("Failed to deserialize the state");
 
         assert_eq!(deserialized_state, Some(state));
     }