Handle fatal errors in receiver state machine by spacebear21 · Pull Request #1060 · payjoin/rust-payjoin

spacebear21 · 2025-09-11T19:25:56Z

This brings fatal error handling into the receiver state machine.

Supersedes #1045

TODOs:

HasError::process_error_response should terminally close the session if the response is successfuly, or record a fatal error and move to terminal state if another error request should not be attempted (e.g. expired session or bad reply key).
handle_fatal_reject shouldn't close sessions, instead it should only close after an error response has been processed.
SessionEvent::TerminalFailure holds a JsonReply because they are serializable, but this results in a hacky workaroundfor recording Expired sessions since those shouldn't actually be replied to.
Remove SessionHistory::extract_err_req
Implement session.fail() and session.cancel() to allow receiver to explicitly close a session. Maybe fail() should take a Error parameter and transition to HasReplyableError state if appropriate? see discussion here Fix apply_fee_range error handling #1108 (review)

Pull Request Checklist

Please confirm the following before requesting review:

I have disclosed my use of
AI
in the body of this PR.
I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

arminsabouri

Approach ACK.
Just had some high level questions. In general this is moving in the right direction

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

arminsabouri · 2025-09-12T12:37:33Z

payjoin/src/core/persist.rs

 /// Represents a fatal rejection of a state transition.
 /// When this error occurs, the session must be closed and cannot be resumed.
-pub struct RejectFatal<Event, Err>(Event, Err);
+pub struct RejectFatal<Event, Err>(pub(crate) Event, pub(crate) Err);


Was this pub(crate)'d for tests?

Yeah, specifically to match on the rejection details

Would pub(self) work?

Any decision on whether pub(self) or pub(crate) is better here?

FWIW this is a nit.

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

DanGould · 2025-09-12T15:34:12Z

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

-        &mut self,
+        &mut self, // NOTE: does this need to be &mut? e.g. extract_err_req just clones the inner keys


Definitely easier to deal with if not mut. I believe the concern is &mut self.session_context.ohttp_keys is supposed to get mutated if it ohttp_encapsulate ratchets instead of using randomness so that the same OHTTP ephemeral key doesn't get used twice which would allow a relay to correlate requests since they'd be encrypted with the same key & nonce (base nonce + seq).

I'd like @nothingmuch to confirm.

arminsabouri · 2025-09-15T17:35:26Z

From our devcall:
SessionEvent::Close should take a Reason as an argument. Instead of the reason being derived from previous session event.

We should consider adding a pub abort and private close(). In general it should be a hard to be in limbo where we close but dont push an event or vice-versa

coveralls · 2025-09-18T02:11:42Z

Pull Request Test Coverage Report for Build 18170784561

Details

210 of 268 (78.36%) changed or added relevant lines in 4 files are covered.
8 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.2%) to 84.561%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/core/persist.rs	44	51	86.27%
payjoin-cli/src/app/v2/mod.rs	1	14	7.14%
payjoin/src/core/receive/v2/mod.rs	101	139	72.66%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/core/receive/v2/mod.rs	2	90.43%
payjoin-cli/src/app/v2/mod.rs	3	60.57%
payjoin/src/core/persist.rs	3	94.94%

Totals
Change from base Build 18168561417:	-0.2%
Covered Lines:	8654
Relevant Lines:	10234

💛 - Coveralls

spacebear21 · 2025-09-23T22:49:03Z

Re-requesting review because I believe all the main logic changes are in place and the overall design shouldn't change much unless something clearly wrong comes up in review. Leaving in draft status because some cleanup is needed (e.g. extract_err_req from SessionHistory and remove pub process_err_res pure function since it should only be called in HasReplyableError), and it will need additional testing.

arminsabouri

Partial ApporachACK.
I mostly had highlevel questions. Mainly around:

The necessity for a session event trait
How the typestate progresses to the reply error state

payjoin/src/core/persist.rs

arminsabouri · 2025-09-25T14:25:22Z

payjoin/src/core/persist.rs

 /// Represents a fatal rejection of a state transition.
 /// When this error occurs, the session must be closed and cannot be resumed.
-pub struct RejectFatal<Event, Err>(Event, Err);
+pub struct RejectFatal<Event, Err>(pub(crate) Event, pub(crate) Err);


Would pub(self) work?

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

payjoin/src/core/persist.rs

spacebear21 · 2025-09-30T14:44:24Z

The biggest change since the last round of review is ca5da9c which introduces a new state transition method for replyable errors resulting in the HasReplyableError typestate. This approach replaces the SessionEventTrait approach taken previously, so instead of introspecting on the SessionEvent the caller just calls the appropriate transition method. I used Claude Code opus+sonnet to plan and write the majority of the code in that commit.

benalleng

ack on ca5da9c, Just a few more general clarifying questions. As well the terminal_error TODO can be ignored as we are getting rid of that method entirely, but there still remains the added TODO in the test_err_response in the integrations tests, I'm assuming that can safely be handled after 1.0 in a followup

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

spacebear21 · 2025-09-30T18:11:24Z

there still remains the added TODO in the test_err_response in the integrations tests, I'm assuming that can safely be handled after 1.0 in a followup

Yeah this should be getting addressed in #1114

arminsabouri

CodeACK dcfe2f4
Just had some questions and noted follow up items 🚀

Perhaps we first review and merge #1114, resolve the TODO in the integration test so we can get more e2e coverage on the changes in this PR.

arminsabouri · 2025-10-01T14:31:44Z

payjoin/src/core/persist.rs

 /// Represents a fatal rejection of a state transition.
 /// When this error occurs, the session must be closed and cannot be resumed.
-pub struct RejectFatal<Event, Err>(Event, Err);
+pub struct RejectFatal<Event, Err>(pub(crate) Event, pub(crate) Err);


FWIW this is a nit.

arminsabouri · 2025-10-01T14:34:23Z

payjoin/src/core/persist.rs

 pub struct RejectTransient<Err>(pub(crate) Err);
+/// Represents a replyable error that transitions to an error state but keeps the session open.
+/// When this error occurs, the session transitions to the ErrorState.
+pub struct RejectReplyableError<Event, ErrorState, Err>(


Note to follow up with some unit testing for this

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

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

payjoin/src/core/persist.rs

DanGould

In general I think this works. Wild how few changes actually end up in payjoin-cli's implementation after all this internal jostling. Overall it seems like a much more realistic abstraction of reality. It does seem that tests could be a little tighter if InternalPersistedError could be matched on.

I'm also still a bit bugged out by that name since Transient InternalPersistedErrors are not Persisted. They're PersistErrors which arise from persistence attempts, not errors that are persisted by their nature.

DanGould · 2025-10-01T14:53:24Z

payjoin-cli/src/app/v2/mod.rs

+    async fn handle_error(
        &self,
-        ohttp_relay: &payjoin::Url,
-        session_history: &SessionHistory,
+        session: Receiver<HasReplyableError>,
+        persister: &ReceiverPersister,
    ) -> Result<()> {


Other than an Err(anyhow::Error) being propagated if post_request or process_error_response fails, are there circumstances where we want to close the session in this function? for example, if process_error_response fails? Or if the post_request fails for one of a few enumerated reasons as a follow up with the session.fail() implementation?

DanGould · 2025-10-01T15:10:00Z

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

+            .check_broadcast_suitability(None, |_| Ok(false))
+            .save(&persister)
+            .expect_err("Unbroadcastable should error");
+        // NOTE: it would be good to assert against the internal error type but InternalPersistedError is private


InternalPersistedError is pub(crate).

Would making the internal field in PersistedError pub(super) (to the core scope) address this concern?

#[derive(Debug)] pub struct PersistedError< ApiError: std::error::Error, StorageError: std::error::Error, ErrorState: fmt::Debug = (), >(pub(super) InternalPersistedError<ApiError, StorageError, ErrorState>);

DanGould · 2025-10-01T15:11:59Z

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

-            events: vec![
-                SessionEvent::Created(SHARED_CONTEXT.clone()),
-                SessionEvent::CheckedBroadcastSuitability(),
-                SessionEvent::SessionInvalid(mock_err.0.clone(), Some(mock_err.1.clone())),
-            ],


Why change the static vec! definition to mut & .push() instead? nit.

Hmm I don't think I changed that; this old test got deleted and the new test_session_transient_error is being displayed side by side but it's unrelated. I think I just copied to mut events approach from a different test, we seem to mix and match already.

payjoin/src/core/persist.rs

This session event holds the JSON representation of the error reply to the sender that should be attempted by the receiver, as specified in BIP78. Co-authored-by: spacebear <git@spacebear.dev>

This enforces proper handling within the receiver state machine, such that they must attempt replying to the sender with an error response before the session can be considered closed. Co-authored-by: DanGould <d@ngould.dev>

`TerminalFailure` was never a real state in the receiver state machine. Instead, a SessionEvent::Closed simply results in an empty `()` next state.

Previously extract_err_req was exposed as a public method on SessionHistory, and process_err_res as a pure function, for implementers to manually process error responses. Now this can and should be done via the HasReplyableError typestate, so these functions are no longer needed.

Only a subset of fatal errors should result in immediate closure of areceiver session. In many cases an attempt should first be made torespond to the sender with an error as specified in BIP78. Instead of scrutinizing various error types to determine whether itshould be replyable or not, the callsite can simply call the appropriate state transition method: `replyable_error()` for replyable errors or `fatal()` for non-replyable ones. For replyable errors, the resulting HasReplyableError typestate can be obtained from the resulting `PersistedError` with the `error_state()` method.

spacebear21 · 2025-10-01T17:45:53Z

Latest push is a simple rebase on the latest master. There is an outstanding task in the PR description for the fail()/cancel() methods that I will address in a follow-up.

arminsabouri

Re-Ack 9cdb1eb

arminsabouri added this to the payjoin-1.0 milestone Sep 12, 2025

arminsabouri assigned spacebear21 Sep 12, 2025

arminsabouri reviewed Sep 12, 2025

View reviewed changes

DanGould reviewed Sep 12, 2025

View reviewed changes

DanGould mentioned this pull request Sep 12, 2025

Serialize errs #1045

Closed

2 tasks

spacebear21 force-pushed the handle-error-typestate branch 2 times, most recently from 141b916 to 620c509 Compare September 15, 2025 17:06

arminsabouri mentioned this pull request Sep 17, 2025

Process res error handling #1075

Merged

2 tasks

spacebear21 mentioned this pull request Sep 17, 2025

Introduce SessionEvent::Closed #1078

Merged

2 tasks

spacebear21 force-pushed the handle-error-typestate branch 2 times, most recently from 9e52f2e to 779e172 Compare September 18, 2025 02:07

This was referenced Sep 19, 2025

Implement Serialize/Deserialize on error types #796

Closed

Expired sessions should close the session #1049

Closed

spacebear21 force-pushed the handle-error-typestate branch 3 times, most recently from e0d168e to 58c4143 Compare September 23, 2025 22:43

spacebear21 requested review from DanGould and arminsabouri September 23, 2025 22:45

arminsabouri reviewed Sep 25, 2025

View reviewed changes

spacebear21 mentioned this pull request Sep 29, 2025

New Mutants Found #1118

Closed

DanGould mentioned this pull request Sep 29, 2025

Fix apply_fee_range error handling #1108

Merged

2 tasks

spacebear21 force-pushed the handle-error-typestate branch from fa845c8 to ca5da9c Compare September 30, 2025 01:30

spacebear21 marked this pull request as ready for review September 30, 2025 14:39

spacebear21 requested a review from arminsabouri September 30, 2025 14:39

benalleng mentioned this pull request Sep 30, 2025

Remove terminal_error method for sender and receiver #1123

Merged

2 tasks

spacebear21 requested a review from benalleng September 30, 2025 16:48

spacebear21 mentioned this pull request Sep 30, 2025

HPKE encrypt error response #1115

Merged

2 tasks

benalleng approved these changes Sep 30, 2025

View reviewed changes

payjoin/src/core/receive/v2/mod.rs Show resolved Hide resolved

spacebear21 force-pushed the handle-error-typestate branch from ca5da9c to dcfe2f4 Compare September 30, 2025 18:10

arminsabouri approved these changes Oct 1, 2025

View reviewed changes

DanGould approved these changes Oct 1, 2025

View reviewed changes

spacebear21 mentioned this pull request Oct 1, 2025

Update sender process_res to parse and process error response #1114

Merged

2 tasks

DanGould and others added 7 commits October 1, 2025 12:38

Replace SessionInvalid with GotReplyableError

88dbea2

This session event holds the JSON representation of the error reply to the sender that should be attempted by the receiver, as specified in BIP78. Co-authored-by: spacebear <git@spacebear.dev>

Introduce HasReplyableError typestate

c3d65db

This enforces proper handling within the receiver state machine, such that they must attempt replying to the sender with an error response before the session can be considered closed. Co-authored-by: DanGould <d@ngould.dev>

Remove ReceiveSession::TerminalFailure

6986eb9

`TerminalFailure` was never a real state in the receiver state machine. Instead, a SessionEvent::Closed simply results in an empty `()` next state.

Add session history error tests

3b1174a

Add Receiver<HasReplyableError> test

427d2c4

spacebear21 force-pushed the handle-error-typestate branch from dcfe2f4 to 9cdb1eb Compare October 1, 2025 17:45

arminsabouri approved these changes Oct 1, 2025

View reviewed changes

spacebear21 merged commit 4d82e53 into payjoin:master Oct 1, 2025
14 checks passed

spacebear21 mentioned this pull request Oct 1, 2025

Refactor Receiver Replyable Error #793

Closed

		&mut self,
		&mut self, // NOTE: does this need to be &mut? e.g. extract_err_req just clones the inner keys

Conversation

spacebear21 commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arminsabouri commented Sep 15, 2025

Uh oh!

coveralls commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 18170784561

Details

💛 - Coveralls

Uh oh!

spacebear21 commented Sep 23, 2025

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

spacebear21 commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benalleng left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spacebear21 commented Sep 30, 2025

Uh oh!

arminsabouri left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spacebear21 commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spacebear21 commented Sep 11, 2025 •

edited

Loading

coveralls commented Sep 18, 2025 •

edited

Loading

spacebear21 commented Sep 30, 2025 •

edited

Loading

benalleng left a comment •

edited

Loading

arminsabouri left a comment •

edited

Loading

spacebear21 commented Oct 1, 2025 •

edited

Loading