improve error handling

Incl. fix tests.

Signed-off-by: Lars Wirzenius <liw@liw.fi>

4 files changed, 69 insertions, 27 deletions

diff --git a/src/bin/radicle-native-ci.rs b/src/bin/radicle-native-ci.rs
index ef05814..67ba887 100644
--- a/src/bin/radicle-native-ci.rs
+++ b/src/bin/radicle-native-ci.rs
@@ -20,12 +20,15 @@ use std::{
 use uuid::Uuid;
 
 use radicle::prelude::Profile;
-use radicle_ci_broker::msg::{Id, Oid, Request, Response, RunId, RunResult};
+use radicle_ci_broker::msg::{Id, Oid, Request, RunId, RunResult};
 
 use radicle_native_ci::{
     config::{Config, ConfigError},
     logfile::{LogError, LogFile},
-    msg::{read_request, write_response, write_triggered, NativeMessageError},
+    msg::{
+        read_request, write_errored, write_failed, write_succeeded, write_triggered,
+        NativeMessageError,
+    },
     report,
     runcmd::{runcmd, RunCmdError},
     runinfo::{RunInfo, RunInfoBuilder, RunInfoError},
@@ -57,12 +60,16 @@ fn fallible_main() -> Result<(), NativeError> {
     let mut builder = RunInfo::builder();
 
     let result = fallible_main_inner(&config, &mut logfile, &mut builder);
-    if result.is_ok() {
-        builder.result(RunResult::Success);
-    } else {
-        builder.result(RunResult::Failure);
+    let ri = builder.build()?;
+
+    match &ri.result {
+        RunResult::Success => write_succeeded()?,
+        RunResult::Failure => write_failed()?,
+        RunResult::Error(s) => write_errored(s)?,
+        _ => write_errored(&format!("unknown result {}", ri.result))?,
     }
-    builder.build()?.write()?;
+
+    ri.write()?;
 
     logfile.writeln(&format!("update report page in {}", config.state.display()))?;
     if let Err(e) = report::build_report(&config.state) {
@@ -114,12 +121,12 @@ fn fallible_main_inner(
         let result = runner.run();
         if let Err(e) = result {
             logfile.writeln(&format!("CI failed: {:?}", e))?;
-            builder.result(RunResult::Failure);
+            builder.result(RunResult::Error(format!("{}", e)));
             return Err(e);
         }
         logfile.writeln("CI run exited zero")?;
+        builder.result(RunResult::Success);
     } else {
-        write_response(&Response::error("first request was not Trigger\n"))?;
         builder.result(RunResult::Error("first request was not Trigger".into()));
     };
 
@@ -200,20 +207,25 @@ impl<'a> Runner<'a> {
 
         self.log.writeln("run shell snippet in repository")?;
         let snippet = format!("set -xeuo pipefail\n{}", &runspec.shell);
-        if let Some(timeout) = self.timeout {
+        let runcmd_result = if let Some(timeout) = self.timeout {
             let timeout = format!("{}", timeout);
             runcmd(
                 &mut self.run_log,
                 &["timeout", &timeout, "bash", "-c", &snippet],
                 &self.src,
-            )?;
+            )
         } else {
-            runcmd(&mut self.run_log, &["bash", "-c", &snippet], &self.src)?;
-        }
-
-        let result = RunResult::Success;
-
-        write_response(&Response::finished(result.clone()))?;
+            runcmd(&mut self.run_log, &["bash", "-c", &snippet], &self.src)
+        };
+
+        let result = if runcmd_result.is_ok() {
+            RunResult::Success
+        } else if let Err(RunCmdError::CommandFailed(exit, argv)) = &runcmd_result {
+            let msg = format!("command failed: exit: {}, argv: {:?}", exit, argv);
+            RunResult::Error(msg)
+        } else {
+            RunResult::Failure
+        };
 
         std::fs::remove_dir_all(&self.src)
             .map_err(|e| NativeError::RemoveDir(self.src.clone(), e))?;
@@ -225,7 +237,11 @@ impl<'a> Runner<'a> {
                 .writeln(&format!("failed to write run log: {}", e))?;
         }
 
-        Ok(())
+        if let Err(e) = runcmd_result {
+            Err(e.into())
+        } else {
+            Ok(())
+        }
     }
 }
 
diff --git a/src/msg.rs b/src/msg.rs
index be8336d..6d1c90f 100644
--- a/src/msg.rs
+++ b/src/msg.rs
@@ -1,6 +1,6 @@
 use std::path::PathBuf;
 
-use radicle_ci_broker::msg::{MessageError, Request, Response, RunId};
+use radicle_ci_broker::msg::{MessageError, Request, Response, RunId, RunResult};
 
 use crate::{
     config::ConfigError, logfile::LogError, report, runcmd::RunCmdError, runinfo::RunInfoError,
@@ -13,8 +13,8 @@ pub fn read_request() -> Result<Request, NativeMessageError> {
     Ok(req)
 }
 
-/// Write response to stdout.
-pub fn write_response(resp: &Response) -> Result<(), NativeMessageError> {
+// Write response to stdout.
+fn write_response(resp: &Response) -> Result<(), NativeMessageError> {
     resp.to_writer(std::io::stdout())
         .map_err(|e| NativeMessageError::WriteResponse(resp.clone(), e))?;
     Ok(())
@@ -28,6 +28,30 @@ pub fn write_triggered(run_id: &RunId) -> Result<(), NativeMessageError> {
     Ok(())
 }
 
+/// Write a message indicating failure to stdout.
+pub fn write_failed() -> Result<(), NativeMessageError> {
+    write_response(&Response::Finished {
+        result: RunResult::Failure,
+    })?;
+    Ok(())
+}
+
+/// Write a message indicating error to stdout.
+pub fn write_errored(s: &str) -> Result<(), NativeMessageError> {
+    write_response(&Response::Finished {
+        result: RunResult::Error(s.into()),
+    })?;
+    Ok(())
+}
+
+/// Write a message indicating success to stdout.
+pub fn write_succeeded() -> Result<(), NativeMessageError> {
+    write_response(&Response::Finished {
+        result: RunResult::Success,
+    })?;
+    Ok(())
+}
+
 #[derive(Debug, thiserror::Error)]
 pub enum NativeMessageError {
     #[error("failed to read request from stdin: {0:?}")]
diff --git a/src/runcmd.rs b/src/runcmd.rs
index 1674a5d..831c72a 100644
--- a/src/runcmd.rs
+++ b/src/runcmd.rs
@@ -29,10 +29,6 @@ pub fn runcmd(run_log: &mut RunLog, argv: &[&str], cwd: &Path) -> Result<(), Run
     );
 
     if !exit.success() {
-        let error = Response::error(&format!("command failed: {:?}", argv));
-        error
-            .to_writer(std::io::stdout())
-            .map_err(|e| RunCmdError::WriteResponse(error.clone(), e))?;
         return Err(RunCmdError::CommandFailed(
             exit.code().unwrap(),
             argv.iter().map(|s| s.to_string()).collect(),
diff --git a/test-suite b/test-suite
index b9f34a9..97989fa 100755
--- a/test-suite
+++ b/test-suite
@@ -195,15 +195,21 @@ class Suite:
     def test_native_yaml_has_no_shell(self):
         exit, resps, stderr = self._test_case("no-shell", None)
         assert exit != 0
-        assert len(resps) == 1
+        assert len(resps) == 2
         self.assert_triggered(resps[0])
+        self.assert_error(resps[1])
+        error = resps[1]["result"]["error"]
+        assert "YAML" in error
         assert "shell" in stderr
 
     def test_native_yaml_shell_is_not_string(self):
         exit, resps, stderr = self._test_case("shell-not-string", {"foo": "bar"})
         assert exit != 0
-        assert len(resps) == 1
+        assert len(resps) == 2
         self.assert_triggered(resps[0])
+        self.assert_error(resps[1])
+        error = resps[1]["result"]["error"]
+        assert "YAML" in error
         assert "shell" in stderr
         assert "string" in stderr