diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 7886887..da43512 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -15,6 +15,16 @@ https://github.com/oxidecomputer/steno/compare/v0.4.0\...HEAD[Full list of commits] +=== Breaking changes + +* The error type for undo actions is now called `UndoActionPermanentError`. It used to be simply `anyhow::Error`. Steno's behavior when undo actions return an error has _not_ changed. This explicit type was added to better communicate that if you return an error from an undo action, the saga will become stuck. For convenience, there are `From` impls to produce an `UndoActionPermanentError` from an `anyhow::Error` or an `ActionError`. ++ +**What you need to do:** For any function implementing an undo action: ++ +** If the return type was `Result<(), anyhow::Error>`, change it to `Result<(), UndoActionError>`. If the return type was `UndoResult`, then you don't need to change the return type. +*** If you were directly returning `Err(some_anyhow_error)`, you can instead return `Err(UndoActionPermanentError::from(some_anyhow_error))`. +*** If you were returning an error using `foo()?` that was _not_ an `anyhow::Error` or an `ActionError`, you can generally use `foo().map_err(anyhow::Error::from).map_err(UndoActionPermanentError::from)`. (Your error type must have been convertible to `anyhow::Error` for it to compile before. Now you're explicitly converting it to that, and then to `UndoActionPermanentError`.) + == 0.4.0 (released 2023-05-25) https://github.com/oxidecomputer/steno/compare/v0.3.1\...v0.4.0[Full list of commits] diff --git a/Cargo.lock b/Cargo.lock index 25fc6bb..d18a9ce 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1036,7 +1036,7 @@ dependencies = [ [[package]] name = "steno" -version = "0.4.1-dev" +version = "0.5.0-dev" dependencies = [ "anyhow", "async-trait", diff --git a/Cargo.toml b/Cargo.toml index f50cca5..36a9ce8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "steno" -version = "0.4.1-dev" +version = "0.5.0-dev" edition = "2021" license = "Apache-2.0" repository = "https://github.com/oxidecomputer/steno" diff --git a/examples/trip.rs b/examples/trip.rs index 26d23dd..00b2f3f 100644 --- a/examples/trip.rs +++ b/examples/trip.rs @@ -24,6 +24,7 @@ use steno::SagaName; use steno::SagaResultErr; use steno::SagaType; use steno::SecClient; +use steno::UndoActionPermanentError; use uuid::Uuid; // This is where we're going: this program will collect payment and book a whole @@ -283,7 +284,7 @@ async fn saga_charge_card( async fn saga_refund_card( action_context: ActionContext, -) -> Result<(), anyhow::Error> { +) -> Result<(), UndoActionPermanentError> { // Fetch the payment confirmation. The undo function is only ever invoked // after the action function has succeeded. This node is called "payment", // so we fetch our own action's output by looking up the data for "payment". @@ -306,7 +307,7 @@ async fn saga_book_hotel( async fn saga_cancel_hotel( action_context: ActionContext, -) -> Result<(), anyhow::Error> { +) -> Result<(), UndoActionPermanentError> { // ... let trip_context = action_context.user_data(); let confirmation: HotelReservation = action_context.lookup("hotel")?; @@ -327,7 +328,7 @@ async fn saga_book_flight( async fn saga_cancel_flight( action_context: ActionContext, -) -> Result<(), anyhow::Error> { +) -> Result<(), UndoActionPermanentError> { // ... let trip_context = action_context.user_data(); let confirmation: FlightReservation = action_context.lookup("flight")?; @@ -348,7 +349,7 @@ async fn saga_book_car( async fn saga_cancel_car( action_context: ActionContext, -) -> Result<(), anyhow::Error> { +) -> Result<(), UndoActionPermanentError> { // ... let trip_context = action_context.user_data(); let confirmation: CarReservation = action_context.lookup("car")?; diff --git a/src/lib.rs b/src/lib.rs index 99a9ab4..558e03a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -60,7 +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_error::UndoActionPermanentError; pub use saga_action_func::new_action_noop_undo; pub use saga_action_func::ActionFunc; pub use saga_action_func::ActionFuncResult; diff --git a/src/saga_action_error.rs b/src/saga_action_error.rs index 7bafa18..e04aee6 100644 --- a/src/saga_action_error.rs +++ b/src/saga_action_error.rs @@ -155,8 +155,19 @@ impl ActionError { /// 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 }, +#[error("undo action failed permanently: {message}")] +pub struct UndoActionPermanentError { + message: String, +} + +impl From for UndoActionPermanentError { + fn from(value: anyhow::Error) -> Self { + UndoActionPermanentError { message: format!("{:#}", value) } + } +} + +impl From for UndoActionPermanentError { + fn from(value: ActionError) -> Self { + UndoActionPermanentError::from(anyhow::Error::from(value)) + } } diff --git a/src/saga_action_generic.rs b/src/saga_action_generic.rs index 7d5afbd..4a139c8 100644 --- a/src/saga_action_generic.rs +++ b/src/saga_action_generic.rs @@ -7,6 +7,7 @@ use crate::saga_action_error::ActionError; use crate::saga_exec::ActionContext; use crate::ActionName; +use crate::UndoActionPermanentError; use futures::future::BoxFuture; use serde::de::DeserializeOwned; use serde::Serialize; @@ -62,10 +63,7 @@ impl ActionData pub type ActionResult = Result, ActionError>; /// Result of a saga undo action -// TODO-design what should the error type here be? Maybe something that can -// encompass "general framework error"? This might put the saga into a "needs -// attention" state? -pub type UndoResult = Result<(), anyhow::Error>; +pub type UndoResult = Result<(), UndoActionPermanentError>; /// Building blocks of sagas /// @@ -167,7 +165,9 @@ impl Action for ActionInjectError { fn undo_it(&self, _: ActionContext) -> BoxFuture<'_, UndoResult> { // 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"))) + Box::pin(futures::future::err(UndoActionPermanentError::from( + anyhow::anyhow!("error injected"), + ))) } fn name(&self) -> ActionName { diff --git a/src/saga_exec.rs b/src/saga_exec.rs index 2578f4e..959dcd6 100644 --- a/src/saga_exec.rs +++ b/src/saga_exec.rs @@ -7,7 +7,6 @@ use crate::dag::InternalNode; use crate::dag::NodeName; use crate::rust_features::ExpectNone; use crate::saga_action_error::ActionError; -use crate::saga_action_error::UndoActionError; use crate::saga_action_generic::Action; use crate::saga_action_generic::ActionConstant; use crate::saga_action_generic::ActionData; @@ -24,6 +23,7 @@ use crate::SagaLog; use crate::SagaNodeEvent; use crate::SagaNodeId; use crate::SagaType; +use crate::UndoActionPermanentError; use anyhow::anyhow; use anyhow::ensure; use anyhow::Context; @@ -41,7 +41,6 @@ use petgraph::Direction; use petgraph::Graph; use petgraph::Incoming; use petgraph::Outgoing; -use serde_json::json; use std::collections::BTreeMap; use std::collections::BTreeSet; use std::convert::TryFrom; @@ -58,7 +57,7 @@ use tokio::task::JoinHandle; struct SgnsDone(Arc); struct SgnsFailed(ActionError); struct SgnsUndone(UndoMode); -struct SgnsUndoFailed(UndoActionError); +struct SgnsUndoFailed(UndoActionPermanentError); struct SagaNode { node_id: NodeIndex, @@ -1272,21 +1271,14 @@ impl SagaExecutor { let action = &task_params.action; let undo_error = futures::stream::iter(0..count) .map(Ok::) - .try_for_each(|i| async move { - action - .undo_it(make_action_context()) - .await - .with_context(|| format!("undo action attempt {}", i + 1)) + .try_for_each(|_| async move { + action.undo_it(make_action_context()).await }) .await; if let Err(error) = undo_error { - let node = Box::new(SagaNode { - node_id, - state: SgnsUndoFailed(UndoActionError::PermanentFailure { - source_error: json!({ "message": format!("{:#}", error) }), - }), - }); + let node = + Box::new(SagaNode { node_id, state: SgnsUndoFailed(error) }); SagaExecutor::finish_task(task_params, node).await; } else { let node = Box::new(SagaNode { @@ -1513,7 +1505,7 @@ struct SagaExecLiveState { /// Errors produced by failed actions. node_errors: BTreeMap, /// Errors produced by failed undo actions. - undo_errors: BTreeMap, + undo_errors: BTreeMap, /// Persistent state sglog: SagaLog, @@ -1741,7 +1733,7 @@ pub struct SagaResultErr { /// details about the action failure pub error_source: ActionError, /// if an undo action also failed, details about that failure - pub undo_failure: Option<(NodeName, UndoActionError)>, + pub undo_failure: Option<(NodeName, UndoActionPermanentError)>, } /// Summarizes in-progress execution state of a saga diff --git a/src/saga_log.rs b/src/saga_log.rs index abb2c6e..58c5513 100644 --- a/src/saga_log.rs +++ b/src/saga_log.rs @@ -1,7 +1,7 @@ //! Persistent state for sagas use crate::saga_action_error::ActionError; -use crate::saga_action_error::UndoActionError; +use crate::saga_action_error::UndoActionPermanentError; use crate::SagaId; use anyhow::anyhow; use anyhow::Context; @@ -86,7 +86,7 @@ pub enum SagaNodeEventType { /// The undo action has finished UndoFinished, /// The undo action has failed - UndoFailed(UndoActionError), + UndoFailed(UndoActionPermanentError), } impl fmt::Display for SagaNodeEventType { @@ -133,7 +133,7 @@ pub enum SagaNodeLoadStatus { /// The undo action has finished successfully UndoFinished, /// The undo action has failed - UndoFailed(UndoActionError), + UndoFailed(UndoActionPermanentError), } impl SagaNodeLoadStatus { diff --git a/src/sec.rs b/src/sec.rs index 8e6eaed..2e47b4b 100644 --- a/src/sec.rs +++ b/src/sec.rs @@ -1450,7 +1450,7 @@ mod test { use super::*; use crate::{ ActionContext, ActionError, ActionFunc, DagBuilder, Node, SagaId, - SagaName, + SagaName, UndoActionPermanentError, }; use serde::{Deserialize, Serialize}; use slog::Drain; @@ -1526,7 +1526,7 @@ mod test { } async fn undo_n1( ctx: ActionContext, - ) -> Result<(), anyhow::Error> { + ) -> Result<(), UndoActionPermanentError> { ctx.user_data().call("undo_n1"); Ok(()) } @@ -1539,7 +1539,7 @@ mod test { } async fn undo_n2( ctx: ActionContext, - ) -> Result<(), anyhow::Error> { + ) -> Result<(), UndoActionPermanentError> { ctx.user_data().call("undo_n2"); Ok(()) } diff --git a/tests/test_smoke_run_recover_stuck_done.out b/tests/test_smoke_run_recover_stuck_done.out index 897a24e..53ed128 100644 --- a/tests/test_smoke_run_recover_stuck_done.out +++ b/tests/test_smoke_run_recover_stuck_done.out @@ -44,6 +44,4 @@ failed at node: "instance_boot" failed with error: error injected FOLLOWED BY UNDO ACTION FAILURE failed at node: "instance_id" -failed with error: undo action failed permanently: { - "message": "undo action attempt 1: error injected" -} +failed with error: undo action failed permanently: error injected diff --git a/tests/test_smoke_run_stuck.out b/tests/test_smoke_run_stuck.out index 8686f97..23e8116 100644 --- a/tests/test_smoke_run_stuck.out +++ b/tests/test_smoke_run_stuck.out @@ -26,6 +26,4 @@ failed at node: "instance_boot" failed with error: error injected FOLLOWED BY UNDO ACTION FAILURE failed at node: "instance_id" -failed with error: undo action failed permanently: { - "message": "undo action attempt 1: error injected" -} +failed with error: undo action failed permanently: error injected