better handle undo actions that fail

.github/workflows/rust.yml

@@ -33,13 +33,11 @@ jobs:
       run: cargo clippy -- --version
     - name: Run Clippy Lints
       #
-      # Clippy overrides should go into src/lib.rs so that developers running
-      # `cargo clippy` see the same behavior as we see here in the GitHub
-      # Action.  In some cases, the overrides also need to appear here because
-      # clippy does not always honor the crate-wide overrides.  See
-      # rust-lang/rust-clippy#6610.
+      # Clippy's style nits are useful, but not worth keeping in CI.  This
+      # override belongs in src/lib.rs, and it is there, but that doesn't
+      # reliably work due to rust-lang/rust-clippy#6610.
       #
-      run: cargo clippy -- -A clippy::style -D warnings
+      run: cargo clippy --all-targets -- --deny warnings --allow clippy::style
 
   build-and-test:
     runs-on: ${{ matrix.os }}

CHANGELOG.adoc

@@ -15,6 +15,10 @@
 
 https://github.com/oxidecomputer/steno/compare/v0.3.1\...HEAD[Full list of commits]
 
+=== Breaking changes
+
+* https://github.com/oxidecomputer/steno/pull/138[#138] Steno no longer panics when an undo action fails.  `SagaResultErr` has a new optional field describing whether any undo action failed during unwinding.  **You should check this.**  If an undo action fails, then the program has failed to provide the usual guarantee that a saga either runs to completion or completely unwinds.  What to do next is application-specific but in general this cannot be automatically recovered from.  (If there are steps that can automatically recover in this case, the undo action that failed should probably do that instead.)
+
 == 0.3.1 (released 2023-01-06)
 
 https://github.com/oxidecomputer/steno/compare/v0.3.0\...v0.3.1[Full list of commits]

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "steno"
-version = "0.3.2-dev"
+version = "0.4.0-dev"
 edition = "2021"
 license = "Apache-2.0"
 repository = "https://github.com/oxidecomputer/steno"

examples/demo-provision.rs

@@ -18,6 +18,7 @@ use steno::ExampleSagaType;
 use steno::SagaDag;
 use steno::SagaId;
 use steno::SagaLog;
+use steno::SagaResultErr;
 use steno::SagaSerialized;
 use structopt::StructOpt;
 use uuid::Uuid;
@@ -165,6 +166,10 @@ struct RunArgs {
     #[structopt(long)]
     inject_error: Vec<String>,
 
+    /// simulate an error at the named saga node's undo action
+    #[structopt(long)]
+    inject_undo_error: Vec<String>,
+
     /// do not print to stdout
     #[structopt(long)]
     quiet: bool,
@@ -237,6 +242,18 @@ async fn cmd_run(args: &RunArgs) -> Result<(), anyhow::Error> {
         }
     }
 
+    for node_name in &args.inject_undo_error {
+        let node_id = dag.get_index(node_name).with_context(|| {
+            format!("bad argument for --inject-undo-error: {:?}", node_name)
+        })?;
+        sec.saga_inject_error_undo(saga_id, node_id)
+            .await
+            .context("injecting error")?;
+        if !args.quiet {
+            println!("will inject error at node \"{}\" undo action", node_name);
+        }
+    }
+
     if !args.quiet {
         println!("*** running saga ***");
     }
@@ -263,10 +280,19 @@ async fn cmd_run(args: &RunArgs) -> Result<(), anyhow::Error> {
                     success_case.saga_output::<String>().unwrap()
                 );
             }
-            Err(error_case) => {
-                println!("FAILURE");
-                println!("failed at node:    {:?}", error_case.error_node_name);
-                println!("failed with error: {:#}", error_case.error_source);
+            Err(SagaResultErr {
+                error_node_name,
+                error_source,
+                undo_failure,
+            }) => {
+                println!("ACTION FAILURE");
+                println!("failed at node:    {:?}", error_node_name);
+                println!("failed with error: {:#}", error_source);
+                if let Some((undo_node_name, undo_error)) = undo_failure {
+                    println!("FOLLOWED BY UNDO ACTION FAILURE");
+                    println!("failed at node:    {:?}", undo_node_name);
+                    println!("failed with error: {:#}", undo_error);
+                }
             }
         }
     }

examples/trip.rs

@@ -21,6 +21,7 @@ use steno::Node;
 use steno::SagaDag;
 use steno::SagaId;
 use steno::SagaName;
+use steno::SagaResultErr;
 use steno::SagaType;
 use steno::SecClient;
 use uuid::Uuid;
@@ -116,9 +117,14 @@ async fn book_trip(
             );
             println!("\nraw summary:\n{:?}", success.saga_output::<Summary>());
         }
-        Err(error) => {
-            println!("action failed: {}", error.error_node_name.as_ref());
-            println!("error: {}", error.error_source);
+        Err(SagaResultErr { error_node_name, error_source, undo_failure }) => {
+            println!("action failed: {}", error_node_name.as_ref());
+            println!("error: {}", error_source);
+            if let Some((undo_node_name, undo_error_source)) = undo_failure {
+                println!("additionally:");
+                println!("undo action failed: {}", undo_node_name.as_ref());
+                println!("error: {}", undo_error_source);
+            }
         }
     }
 }

src/dag.rs

@@ -1060,7 +1060,7 @@ mod test {
         builder.append(Node::constant("a", serde_json::Value::Null));
         builder.append_parallel(vec![
             Node::constant("c", serde_json::Value::Null),
-            Node::subsaga("b", subsaga_dag.clone(), "c"),
+            Node::subsaga("b", subsaga_dag, "c"),
         ]);
         let error = builder.build().unwrap_err();
         println!("{:?}", error);

src/lib.rs

@@ -60,6 +60,7 @@ pub use dag::SagaDag;
 pub use dag::SagaId;
 pub use dag::SagaName;
 pub use saga_action_error::ActionError;
+pub use saga_action_error::UndoActionError;
 pub use saga_action_func::new_action_noop_undo;
 pub use saga_action_func::ActionFunc;
 pub use saga_action_func::ActionFuncResult;

src/saga_action_error.rs

@@ -142,3 +142,21 @@ impl ActionError {
         ActionError::SubsagaCreateFailed { message }
     }
 }
+
+/// An error produced by a failed undo action
+///
+/// **Returning an error from an undo action should be avoided if at all
+/// possible.**  If undo actions experience transient issues, they should
+/// generally retry until the undo action completes successfully.  That's
+/// because by definition, failure of an undo action means that the saga's
+/// actions cannot be unwound.  The system cannot move forward to the desired
+/// saga end state nor backward to the initial state.  It's left forever in some
+/// partially-updated state.  This should really only happen because of a bug.
+/// It should be expected that human intervention will be required to repair the
+/// result of an undo action that has failed.
+#[derive(Clone, Debug, Deserialize, Error, JsonSchema, Serialize)]
+pub enum UndoActionError {
+    /// Undo action failed due to a consumer-specific error
+    #[error("undo action failed permanently: {source_error:#}")]
+    PermanentFailure { source_error: serde_json::Value },
+}

src/saga_action_generic.rs

@@ -165,8 +165,9 @@ impl<UserType: SagaType> Action<UserType> for ActionInjectError {
     }
 
     fn undo_it(&self, _: ActionContext<UserType>) -> BoxFuture<'_, UndoResult> {
-        // We should never undo an action that failed.
-        unimplemented!();
+        // We should never undo an action that failed.  But this same impl is
+        // plugged into a saga when an "undo action" error is injected.
+        Box::pin(futures::future::err(anyhow::anyhow!("error injected")))
     }
 
     fn name(&self) -> ActionName {