Close session on fatal error only when specified

Only a subset of fatal errors should result in immediate closure of a
receiver session. In many cases an attempt should first be made to
respond to the sender with an error as specified in BIP78.

Instead of scrutinizing various error types to determine whether it
should be replyable or not, the callsite can simply provide the
appropriate SessionEvent::HasReplyableError(JsonReply) for replyable
errors, or SessionEvent::Closed(Failure) for non-replyable ones.

Author: spacebear21

payjoin/src/core/persist.rs

@@ -336,14 +336,25 @@ pub enum OptionalTransitionOutcome<NextState, CurrentState> {
     Stasis(CurrentState),
 }
 
+/// Sealed trait to prevent external implementation of SessionEventTrait.
+pub(crate) mod sealed {
+    pub trait Sealed {}
+}
+
+/// Trait for session events that determines if an event represents a session closure.
+pub trait SessionEventTrait: sealed::Sealed {
+    /// Returns true if this event represents a session closure that should close the persister.
+    fn is_closed_event(&self) -> bool;
+}
+
 /// A session that can persist events to an append-only log.
 /// A session represents a sequence of events generated by the BIP78 state machine.
 /// The events can be replayed from the log to reconstruct the state machine's state.
 pub trait SessionPersister {
     /// Errors that may arise from implementers storage layer
     type InternalStorageError: std::error::Error + Send + Sync + 'static;
     /// Session events types that we are persisting
-    type SessionEvent;
+    type SessionEvent: SessionEventTrait;
 
     /// Appends to list of session updates, Receives generic events
     fn save_event(&self, event: Self::SessionEvent) -> Result<(), Self::InternalStorageError>;
@@ -512,12 +523,14 @@ trait InternalSessionPersister: SessionPersister {
         Err: std::error::Error,
     {
         let RejectFatal(event, error) = reject_fatal;
+        let should_close = event.is_closed_event();
         if let Err(e) = self.save_event(event) {
             return InternalPersistedError::Storage(e);
         }
-        // Session is in a terminal state, close it
-        if let Err(e) = self.close() {
-            return InternalPersistedError::Storage(e);
+        if should_close {
+            if let Err(e) = self.close() {
+                return InternalPersistedError::Storage(e);
+            }
         }
 
         InternalPersistedError::Fatal(error)
@@ -531,14 +544,20 @@ impl<T: SessionPersister> InternalSessionPersister for T {}
 #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)]
 pub struct NoopPersisterEvent;
 
+impl sealed::Sealed for NoopPersisterEvent {}
+
+impl SessionEventTrait for NoopPersisterEvent {
+    fn is_closed_event(&self) -> bool { false }
+}
+
 #[derive(Debug, Clone)]
 pub struct NoopSessionPersister<E = NoopPersisterEvent>(std::marker::PhantomData<E>);
 
 impl<E> Default for NoopSessionPersister<E> {
     fn default() -> Self { Self(std::marker::PhantomData) }
 }
 
-impl<E: 'static> SessionPersister for NoopSessionPersister<E> {
+impl<E: 'static + SessionEventTrait> SessionPersister for NoopSessionPersister<E> {
     type InternalStorageError = std::convert::Infallible;
     type SessionEvent = E;
 
@@ -559,7 +578,7 @@ impl<E: 'static> SessionPersister for NoopSessionPersister<E> {
 pub mod test_utils {
     use std::sync::{Arc, RwLock};
 
-    use crate::persist::SessionPersister;
+    use crate::persist::{SessionEventTrait, SessionPersister};
 
     #[derive(Clone)]
     /// In-memory session persister for testing session replays and introspecting session events
@@ -583,7 +602,7 @@ pub mod test_utils {
 
     impl<V> SessionPersister for InMemoryTestPersister<V>
     where
-        V: Clone + 'static,
+        V: Clone + 'static + SessionEventTrait,
     {
         type InternalStorageError = std::convert::Infallible;
         type SessionEvent = V;
@@ -625,6 +644,12 @@ mod tests {
     #[derive(Debug, Clone, Serialize, Deserialize)]
     pub struct InMemoryTestEvent(String);
 
+    impl crate::persist::sealed::Sealed for InMemoryTestEvent {}
+
+    impl crate::persist::SessionEventTrait for InMemoryTestEvent {
+        fn is_closed_event(&self) -> bool { self.0 == "error close" }
+    }
+
     #[derive(Debug, Clone, PartialEq)]
     /// Dummy error type for testing
     struct InMemoryTestError {}
@@ -783,6 +808,7 @@ mod tests {
     #[test]
     fn test_maybe_success_transition() {
         let event = InMemoryTestEvent("foo".to_string());
+        let error_event = InMemoryTestEvent("error close".to_string());
         let test_cases: Vec<
             TestCase<(), PersistedError<InMemoryTestError, std::convert::Infallible>>,
         > = vec![
@@ -813,17 +839,14 @@ mod tests {
             // Fatal error
             TestCase {
                 expected_result: ExpectedResult {
-                    events: vec![InMemoryTestEvent("error event".to_string())],
+                    events: vec![error_event.clone()],
                     is_closed: true,
                     error: Some(InternalPersistedError::Fatal(InMemoryTestError {}).into()),
                     success: None,
                 },
                 test: Box::new(move |persister| {
-                    MaybeSuccessTransition::fatal(
-                        InMemoryTestEvent("error event".to_string()),
-                        InMemoryTestError {},
-                    )
-                    .save(persister)
+                    MaybeSuccessTransition::fatal(error_event.clone(), InMemoryTestError {})
+                        .save(persister)
                 }),
             },
         ];
@@ -873,7 +896,7 @@ mod tests {
             TestCase {
                 expected_result: ExpectedResult {
                     events: vec![error_event.clone()],
-                    is_closed: true,
+                    is_closed: false,
                     error: Some(InternalPersistedError::Fatal(InMemoryTestError {}).into()),
                     success: None,
                 },
@@ -893,7 +916,7 @@ mod tests {
     #[test]
     fn test_maybe_success_transition_with_no_results() {
         let event = InMemoryTestEvent("foo".to_string());
-        let error_event = InMemoryTestEvent("error event".to_string());
+        let error_event = InMemoryTestEvent("error close".to_string());
         let current_state = "Current state".to_string();
         let success_value = "Success value".to_string();
         let test_cases: Vec<
@@ -1010,7 +1033,7 @@ mod tests {
             TestCase {
                 expected_result: ExpectedResult {
                     events: vec![error_event.clone()],
-                    is_closed: true,
+                    is_closed: false,
                     error: Some(InternalPersistedError::Fatal(InMemoryTestError {}).into()),
                     success: None,
                 },

payjoin/src/core/receive/v2/session.rs

@@ -3,7 +3,7 @@ use serde::{Deserialize, Serialize};
 use super::{ReceiveSession, SessionContext};
 use crate::error::{InternalReplayError, ReplayError};
 use crate::output_substitution::OutputSubstitution;
-use crate::persist::SessionPersister;
+use crate::persist::{SessionEventTrait, SessionPersister};
 use crate::receive::v2::{extract_err_req, SessionError};
 use crate::receive::{common, InputPair, JsonReply, OriginalPayload, PsbtContext};
 use crate::{ImplementationError, IntoUrl, PjUri, Request};
@@ -216,6 +216,12 @@ pub enum SessionOutcome {
     Cancel,
 }
 
+impl crate::persist::sealed::Sealed for SessionEvent {}
+
+impl SessionEventTrait for SessionEvent {
+    fn is_closed_event(&self) -> bool { matches!(self, SessionEvent::Closed(_)) }
+}
+
 #[cfg(test)]
 mod tests {
     use std::time::{Duration, SystemTime};

payjoin/src/core/send/v2/session.rs

@@ -1,6 +1,6 @@
 use super::WithReplyKey;
 use crate::error::{InternalReplayError, ReplayError};
-use crate::persist::SessionPersister;
+use crate::persist::{SessionEventTrait, SessionPersister};
 use crate::send::v2::{PollingForProposal, SendSession};
 use crate::uri::v2::PjParam;
 use crate::ImplementationError;
@@ -98,6 +98,16 @@ pub enum SessionEvent {
     SessionInvalid(String),
 }
 
+impl crate::persist::sealed::Sealed for SessionEvent {}
+
+impl SessionEventTrait for SessionEvent {
+    fn is_closed_event(&self) -> bool {
+        // Sender doesn't have a Closed variant yet, so always return true for now.
+        // This maintains current behavior where all fatal events close the session.
+        true
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use bitcoin::{FeeRate, ScriptBuf};