feat(core: improve auto troubleshooting logic

espanso/src/cli/daemon/mod.rs

@@ -24,7 +24,6 @@ use crossbeam::{
   select,
 };
 use espanso_ipc::IPCClient;
-use espanso_path::Paths;
 use log::{error, info, warn};
 
 use crate::{
@@ -85,14 +84,26 @@ fn daemon_main(args: CliModuleArgs) -> i32 {
 
   // TODO: we might need to check preconditions: accessibility on macOS, presence of binaries on Linux, etc
 
-  let config_store =
+  // This variable holds the current troubleshooter guard.
-    match troubleshoot::load_config_or_troubleshoot(&paths, &paths_overrides, false) {
+  // When a guard is dropped, the troubleshooting GUI is killed
-      Ok((load_result, _)) => load_result.config_store,
+  // so this ensures that there is only one troubleshooter running
-      Err(fatal_err) => {
+  // at a given time.
-        error!("critical error while loading config: {}", fatal_err);
+  let mut _current_troubleshoot_guard = None;
-        return DAEMON_FATAL_CONFIG_ERROR;
+
-      }
+  let config_store = match troubleshoot::load_config_or_troubleshoot(&paths, &paths_overrides) {
-    };
+    troubleshoot::LoadResult::Correct(load_result) => load_result.config_store,
+    troubleshoot::LoadResult::Warning(load_result, guard) => {
+      _current_troubleshoot_guard = guard;
+      load_result.config_store
+    }
+    troubleshoot::LoadResult::Fatal(mut guard) => {
+      guard
+        .wait()
+        .expect("unable to wait for troubleshooting guard");
+      error!("critical error while loading config");
+      return DAEMON_FATAL_CONFIG_ERROR;
+    }
+  };
 
   info!("espanso version: {}", VERSION);
   // TODO: print os system and version? (with os_info crate)
@@ -106,7 +117,6 @@ fn daemon_main(args: CliModuleArgs) -> i32 {
   let mut worker_run_count = 0;
 
   spawn_worker(
-    &paths,
     &paths_overrides,
     exit_notify.clone(),
     &mut worker_run_count,
@@ -132,37 +142,57 @@ fn daemon_main(args: CliModuleArgs) -> i32 {
       recv(watcher_signal) -> _ => {
         info!("configuration change detected, restarting worker process...");
 
-        match create_ipc_client_to_worker(&paths.runtime) {
+        // Before killing the previous worker, we make sure there is no fatal error
-          Ok(mut worker_ipc) => {
+        // in the configs.
-            if let Err(err) = worker_ipc.send_async(IPCEvent::Exit) {
+        let should_restart_worker = match troubleshoot::load_config_or_troubleshoot(&paths, &paths_overrides) {
-              error!(
+          troubleshoot::LoadResult::Correct(_) => {
-                "unable to send termination signal to worker process: {}",
+            _current_troubleshoot_guard = None;
-                err
+            true
-              );
+          },
+          troubleshoot::LoadResult::Warning(_, guard) => {
+            _current_troubleshoot_guard = guard;
+            true
+          }
+          troubleshoot::LoadResult::Fatal(guard) => {
+            _current_troubleshoot_guard = Some(guard);
+            error!("critical error while loading config, could not restart worker");
+            false
+          }
+        };
+
+        if should_restart_worker {
+          match create_ipc_client_to_worker(&paths.runtime) {
+            Ok(mut worker_ipc) => {
+              if let Err(err) = worker_ipc.send_async(IPCEvent::Exit) {
+                error!(
+                  "unable to send termination signal to worker process: {}",
+                  err
+                );
+              }
+            }
+            Err(err) => {
+              error!("could not establish IPC connection with worker: {}", err);
             }
           }
-          Err(err) => {
-            error!("could not establish IPC connection with worker: {}", err);
-          }
-        }
 
-        // Wait until the worker process has terminated
+          // Wait until the worker process has terminated
-        let start = Instant::now();
+          let start = Instant::now();
-        let mut has_timed_out = true;
+          let mut has_timed_out = true;
-        while start.elapsed() < std::time::Duration::from_secs(30) {
+          while start.elapsed() < std::time::Duration::from_secs(30) {
-          let lock_file = acquire_worker_lock(&paths.runtime);
+            let lock_file = acquire_worker_lock(&paths.runtime);
-          if lock_file.is_some() {
+            if lock_file.is_some() {
-            has_timed_out = false;
+              has_timed_out = false;
-            break;
+              break;
+            }
+
+            std::thread::sleep(std::time::Duration::from_millis(100));
           }
 
-          std::thread::sleep(std::time::Duration::from_millis(100));
+          if !has_timed_out {
-        }
+            spawn_worker(&paths_overrides, exit_notify.clone(), &mut worker_run_count, false);
-
+          } else {
-        if !has_timed_out {
+            error!("could not restart worker, as the exit process has timed out");
-          spawn_worker(&paths, &paths_overrides, exit_notify.clone(), &mut worker_run_count, false);
+          }
-        } else {
-          error!("could not restart worker, as the exit process has timed out");
         }
       }
       recv(exit_signal) -> code => {
@@ -175,7 +205,7 @@ fn daemon_main(args: CliModuleArgs) -> i32 {
               }
               WORKER_RESTART => {
                 info!("worker requested a restart, spawning a new one...");
-                spawn_worker(&paths, &paths_overrides, exit_notify.clone(), &mut worker_run_count, true);
+                spawn_worker(&paths_overrides, exit_notify.clone(), &mut worker_run_count, true);
               }
               _ => {
                 error!("received unexpected exit code from worker {}, exiting", code);
@@ -233,7 +263,6 @@ fn terminate_worker_if_already_running(runtime_dir: &Path) {
 }
 
 fn spawn_worker(
-  paths: &Paths,
   paths_overrides: &PathsOverrides,
   exit_notify: Sender<i32>,
   worker_run_count: &mut i32,
@@ -244,28 +273,6 @@ fn spawn_worker(
   let espanso_exe_path =
     std::env::current_exe().expect("unable to obtain espanso executable location");
 
-  // Before starting the worker, check if the configuration has any error, and if so, show the troubleshooting GUI
-  let troubleshoot_guard =
-    match troubleshoot::load_config_or_troubleshoot(paths, paths_overrides, true) {
-      Ok((_, troubleshoot_guard)) => troubleshoot_guard,
-      Err(fatal_err) => {
-        error!(
-          "critical configuration error detected, unable to restart worker: {:?}",
-          fatal_err
-        );
-
-        // TODO: we should show a "Reload & Retry" button in the troubleshooter to retry
-        // loading the configuration and:
-        //   - if the configuration is good -> start the worker
-        //   - if the configuration is bad -> show the window again
-        // Until we have this logic, we choose the safest option and kill the daemon
-        // (otherwise, we would have a dangling daemon running after closing the troubleshooting).
-
-        unimplemented!();
-        return;
-      }
-    };
-
   let mut command = Command::new(&espanso_exe_path.to_string_lossy().to_string());
 
   let string_worker_run_count = format!("{}", worker_run_count);
@@ -273,7 +280,7 @@ fn spawn_worker(
     "worker",
     "--monitor-daemon",
     "--run-count",
-    &string_worker_run_count
+    &string_worker_run_count,
   ];
   if manual_start {
     args.push("--manual");
@@ -290,8 +297,6 @@ fn spawn_worker(
   std::thread::Builder::new()
     .name("worker-status-monitor".to_string())
     .spawn(move || {
-      let _guard = troubleshoot_guard;
-
       let result = child.wait();
       if let Ok(status) = result {
         if let Some(code) = status.code() {

espanso/src/cli/daemon/troubleshoot.rs

@@ -19,7 +19,7 @@
 
 use std::process::{Child, Command};
 
-use anyhow::{Result, bail};
+use anyhow::{Result};
 use espanso_path::Paths;
 
 use crate::cli::util::CommandExt;
@@ -28,9 +28,7 @@ use crate::config::ConfigLoadResult;
 use crate::error_eprintln;
 use crate::preferences::Preferences;
 
-pub fn launch_troubleshoot(
+pub fn launch_troubleshoot(paths_overrides: &PathsOverrides) -> Result<TroubleshootGuard> {
-  paths_overrides: &PathsOverrides,
-) -> Result<TroubleshootGuard> {
   let espanso_exe_path = std::env::current_exe()?;
   let mut command = Command::new(&espanso_exe_path.to_string_lossy().to_string());
   command.args(&["modulo", "troubleshoot"]);
@@ -41,12 +39,6 @@ pub fn launch_troubleshoot(
   Ok(TroubleshootGuard::new(child))
 }
 
-pub fn launch_troubleshoot_blocking(paths_overrides: &PathsOverrides) -> Result<()> {
-  let mut guard = launch_troubleshoot(paths_overrides)?;
-  guard.wait()?;
-  Ok(())
-}
-
 pub struct TroubleshootGuard {
   child: Child,
 }
@@ -67,16 +59,20 @@ impl Drop for TroubleshootGuard {
   }
 }
 
-pub fn load_config_or_troubleshoot(
+pub enum LoadResult {
-  paths: &Paths,
+  Correct(ConfigLoadResult),
-  paths_overrides: &PathsOverrides,
+  Warning(ConfigLoadResult, Option<TroubleshootGuard>),
-  troubleshoot_non_fatal: bool,
+  Fatal(TroubleshootGuard),
-) -> Result<(ConfigLoadResult, Option<TroubleshootGuard>)> {
+}
+
+pub fn load_config_or_troubleshoot(paths: &Paths, paths_overrides: &PathsOverrides) -> LoadResult {
   match crate::load_config(&paths.config, &paths.packages) {
     Ok(load_result) => {
-      let mut troubleshoot_handle = None;
+      if load_result.non_fatal_errors.is_empty() {
+        return LoadResult::Correct(load_result);
+      } else {
+        let mut troubleshoot_handle = None;
 
-      if troubleshoot_non_fatal && !load_result.non_fatal_errors.is_empty() {
         let preferences =
           crate::preferences::get_default(&paths.runtime).expect("unable to get preferences");
 
@@ -84,19 +80,20 @@ pub fn load_config_or_troubleshoot(
           match launch_troubleshoot(paths_overrides) {
             Ok(handle) => {
               troubleshoot_handle = Some(handle);
-            },
+            }
             Err(err) => {
               error_eprintln!("unable to launch troubleshoot GUI: {}", err);
             }
           }
         }
-      }
-      Ok((load_result, troubleshoot_handle))
-    }
-    Err(fatal_err) => {
-      launch_troubleshoot_blocking(paths_overrides)?;
 
-      bail!("fatal error while loading config: {}", fatal_err);
+        return LoadResult::Warning(load_result, troubleshoot_handle);
+      }
+    }
+    Err(_) => {
+      return LoadResult::Fatal(
+        launch_troubleshoot(paths_overrides).expect("unable to launch troubleshoot GUI"),
+      );
     }
   }
 }