feat(execution-engine)!: fail :error: now bubbles the original error up [fixes VM-342] (#714)

air/src/execution_step/errors/catchable_errors.rs

@@ -32,7 +32,7 @@ use std::rc::Rc;
 /// Catchable errors arisen during AIR script execution. Catchable here means that these errors
 /// could be handled by a xor instruction and their error_code could be used in a match
 /// instruction.
-#[derive(ThisError, EnumDiscriminants, Debug)]
+#[derive(ThisError, EnumDiscriminants, Debug, Clone)]
 #[strum_discriminants(derive(EnumIter))]
 pub enum CatchableError {
     /// An error is occurred while calling local service via call_service.

air/src/execution_step/execution_context/context.rs

@@ -195,6 +195,8 @@ impl ExecutionCtx<'_> {
 
         self.error_descriptor
             .try_to_set_error_from_exec_error(error, instruction, peer_id, tetraplet.clone());
+
+        self.error_descriptor.disable_error_setting();
     }
 }
 

air/src/execution_step/execution_context/instruction_error/error_descriptor.rs

@@ -18,10 +18,12 @@ use super::no_error;
 use super::InstructionError;
 use crate::execution_step::ErrorAffectable;
 use crate::execution_step::RcSecurityTetraplet;
+use crate::ExecutionError;
 use crate::ToErrorCode;
 
 pub(crate) struct ErrorDescriptor {
     error: InstructionError,
+    error_can_be_set: bool,
 }
 
 impl ErrorDescriptor {
@@ -34,7 +36,8 @@ impl ErrorDescriptor {
     ) {
         use super::get_instruction_error_from_exec_error;
 
-        if !error.affects_error() {
+        // returns early if there is an error to bubble up or the error is Uncatchable.
+        if !self.error_can_be_set || !error.affects_error() {
             return;
         }
 
@@ -45,15 +48,36 @@ impl ErrorDescriptor {
         &self.error
     }
 
-    pub(crate) fn clear_error(&mut self) {
-        self.error = no_error();
+    pub(crate) fn clear_error_object_if_needed(&mut self) {
+        if self.error_can_be_set {
+            self.error = no_error();
+        }
+    }
+
+    pub(crate) fn set_original_execution_error(&mut self, e: &ExecutionError) {
+        self.error.orig_catchable = match e {
+            ExecutionError::Catchable(err) => Some(err.as_ref().clone()),
+            _ => None,
+        };
+    }
+
+    pub(crate) fn enable_error_setting(&mut self) {
+        self.error_can_be_set = true;
+    }
+
+    pub(crate) fn disable_error_setting(&mut self) {
+        self.error_can_be_set = false;
     }
 }
 
 impl Default for ErrorDescriptor {
     fn default() -> Self {
         let error = no_error();
+        let error_can_be_set = true;
 
-        Self { error }
+        Self {
+            error,
+            error_can_be_set,
+        }
     }
 }

air/src/execution_step/execution_context/instruction_error/errors_utils.rs

@@ -65,9 +65,11 @@ pub(crate) fn get_instruction_error_from_error_object(
     tetraplet: Option<RcSecurityTetraplet>,
     provenance: Provenance,
 ) -> InstructionError {
+    let orig_catchable = None;
     InstructionError {
         error,
         tetraplet,
         provenance,
+        orig_catchable,
     }
 }

air/src/execution_step/execution_context/instruction_error/instruction_error_definition.rs

@@ -16,11 +16,10 @@
 
 use super::ErrorObjectError;
 use crate::execution_step::RcSecurityTetraplet;
+use crate::CatchableError;
 use crate::JValue;
 
 use air_interpreter_data::Provenance;
-use serde::Deserialize;
-use serde::Serialize;
 use serde_json::json;
 
 use std::rc::Rc;
@@ -37,7 +36,7 @@ pub const NO_ERROR_ERROR_CODE: i64 = 0;
 /// There are some differences b/w mentioned errors and an ordinary scalar:
 ///  - they are set to not-an-error value by default
 ///  - these are global scalars, meaning that fold and new scopes do not apply for it
-#[derive(Debug, Clone, Serialize, Deserialize)]
+#[derive(Debug, Clone)]
 pub struct InstructionError {
     /// Error object that represents the last occurred error.
     pub error: Rc<JValue>,
@@ -45,8 +44,12 @@ pub struct InstructionError {
     /// Tetraplet that identify host where the error occurred.
     pub tetraplet: Option<RcSecurityTetraplet>,
 
-    /// Value provenance
+    /// Value provenance.
     pub provenance: Provenance,
+
+    /// This is an original Catchable that is used to bubble an original error up,
+    /// when `fail :error:` is called in the right branch of `xor`.
+    pub orig_catchable: Option<CatchableError>,
 }
 
 pub(crate) fn error_from_raw_fields_w_peerid(
@@ -146,5 +149,6 @@ pub fn no_error() -> InstructionError {
         error: Rc::new(no_error_object()),
         tetraplet: None,
         provenance: Provenance::literal(),
+        orig_catchable: None,
     }
 }

air/src/execution_step/instructions/fail.rs

@@ -109,6 +109,7 @@ fn fail_with_last_error(exec_ctx: &mut ExecutionCtx<'_>) -> ExecutionResult<()>
         error,
         tetraplet,
         provenance,
+        ..
     } = exec_ctx.last_error_descriptor.error();
 
     check_error_object(error).map_err(CatchableError::InvalidErrorObjectError)?;
@@ -127,11 +128,21 @@ fn fail_with_error(exec_ctx: &mut ExecutionCtx<'_>) -> ExecutionResult<()> {
         error,
         tetraplet,
         provenance,
+        orig_catchable,
     } = exec_ctx.error_descriptor.error();
 
     check_error_object(error).map_err(CatchableError::InvalidErrorObjectError)?;
 
-    fail_with_error_object(exec_ctx, error.clone(), tetraplet.clone(), provenance.clone())
+    let result = match orig_catchable {
+        Some(orig_catchable) => {
+            let catchable_to_return = orig_catchable.clone();
+            let _ = fail_with_error_object(exec_ctx, error.clone(), tetraplet.clone(), provenance.clone());
+            Err(ExecutionError::Catchable(catchable_to_return.into()))
+        }
+        None => fail_with_error_object(exec_ctx, error.clone(), tetraplet.clone(), provenance.clone()),
+    };
+    exec_ctx.error_descriptor.disable_error_setting();
+    result
 }
 
 fn fail_with_error_object(

air/src/execution_step/instructions/xor.rs

@@ -34,11 +34,19 @@ impl<'i> super::ExecutableInstruction<'i> for Xor<'i> {
                 exec_ctx.flush_subgraph_completeness();
                 exec_ctx.last_error_descriptor.meet_xor_right_branch();
 
+                exec_ctx.error_descriptor.set_original_execution_error(&e);
+                exec_ctx.error_descriptor.enable_error_setting();
+
                 let right_subgraph_result = self.1.execute(exec_ctx, trace_ctx);
                 // This sets :error: to a no-error state.
                 // Please note the right_subgraph_result might be an Error that bubbles up to an :error:
                 // above this execute().
-                exec_ctx.error_descriptor.clear_error();
+                exec_ctx.error_descriptor.clear_error_object_if_needed();
+
+                if right_subgraph_result.is_ok() {
+                    exec_ctx.error_descriptor.enable_error_setting();
+                }
+
                 right_subgraph_result
             }
             res => res,

air/src/execution_step/resolver/resolvable_impl.rs

@@ -74,6 +74,7 @@ fn resolve_errors(
         error,
         tetraplet,
         provenance,
+        ..
     } = instruction_error;
 
     let jvalue = match lens {

air/tests/test_module/features/errors/error.rs

@@ -44,9 +44,9 @@ fn fail_with_rebubble_error() {
     let mut cid_tracker: ExecutionCidState = ExecutionCidState::new();
     let expected_error_json = {
         json!({
-          "error_code": 10006,
-          "instruction": "xor",
-          "message": "fail with '{\"error_code\":10001,\"instruction\":\"match 1 2\",\"message\":\"compared values do not match\"}' is used without corresponding xor"
+          "error_code": 10001,
+          "instruction": "match 1 2",
+          "message": "compared values do not match"
         })
     };
 
@@ -71,3 +71,77 @@ fn fail_with_rebubble_error() {
 
     assert_eq!(actual_trace, expected_trace,);
 }
+
+#[test]
+fn rebubble_error_from_xor_right_branch() {
+    let peer_id = "peer_id";
+    let script = r#"
+    (seq
+        (xor
+            (xor
+                (xor
+                    (match 1 2 (null) )
+                    (fail :error:)
+                )
+                (seq
+                    (call "peer_id" ("m" "f1") [:error:] scalar1) ; behaviour = echo
+                    (match 3 2 (null) )
+                )
+            )
+            (call "peer_id" ("m" "f2") [:error:] scalar2) ; behaviour = echo
+        )
+        (call "peer_id" ("m" "f3") [:error:] scalar3) ; behaviour = echo
+    )
+    "#
+    .to_string();
+
+    let executor = AirScriptExecutor::from_annotated(TestRunParameters::from_init_peer_id(peer_id), &script)
+        .expect("invalid test AIR script");
+    let result = executor.execute_all(peer_id).unwrap();
+    let actual_trace = trace_from_result(&result.last().unwrap());
+
+    let mut cid_tracker: ExecutionCidState = ExecutionCidState::new();
+    let inner_expected_error_json = {
+        json!({
+          "error_code": 10001,
+          "instruction": "match 1 2",
+          "message": "compared values do not match"
+        })
+    };
+    let outer_expected_error_json = {
+        json!({
+          "error_code": 10001,
+          "instruction": "match 3 2",
+          "message": "compared values do not match"
+        })
+    };
+
+    let expected_trace: Vec<ExecutedState> = vec![
+        scalar_tracked!(
+            inner_expected_error_json.clone(),
+            cid_tracker,
+            peer = peer_id,
+            service = "m..0",
+            function = "f1",
+            args = [inner_expected_error_json]
+        ),
+        scalar_tracked!(
+            outer_expected_error_json.clone(),
+            cid_tracker,
+            peer = peer_id,
+            service = "m..1",
+            function = "f2",
+            args = [outer_expected_error_json]
+        ),
+        scalar_tracked!(
+            no_error_object(),
+            cid_tracker,
+            peer = peer_id,
+            service = "m..2",
+            function = "f3",
+            args = [no_error_object()]
+        ),
+    ];
+
+    assert_eq!(actual_trace, expected_trace,);
+}

air/tests/test_module/instructions/fail.rs

@@ -62,15 +62,9 @@ fn fail_with_error() {
     );
 
     let result = call_vm!(vm, <_>::default(), script, "", "");
+    let err_message = r#""failed result from fallible_call_service""#.to_string();
+    let expected_error = CatchableError::LocalServiceError(1i32, err_message.into());
 
-    let expected_error = CatchableError::UserError {
-        error: rc!(json!({
-            "error_code": 10000i64,
-            "instruction": r#"call "local_peer_id" ("service_id_1" "local_fn_name") [] result_1"#,
-            "message": r#"Local service error, ret_code is 1, error message is '"failed result from fallible_call_service"'"#,
-            "peer_id": "local_peer_id",
-        })),
-    };
     assert!(check_error(&result, expected_error));
 }
 

crates/testing-framework/src/asserts/behavior.rs

@@ -66,7 +66,10 @@ impl Behavior {
         use Behavior::*;
 
         match self {
-            Echo => echo_call_service()(params),
+            Echo => {
+                println!("echo_call_service() {:#?}", params);
+                echo_call_service()(params)
+            }
             Unit => unit_call_service()(params),
             Function => CallServiceResult::ok(params.function_name.into()),
             Service => CallServiceResult::ok(params.service_id.into()),