Skip to content

Conversation

@spacebear21
Copy link
Collaborator

As we now have AsyncSessionPersister to enable a sync persistence (#1235), we need to be able to replay the event log asynchronously too.

There's still considerable duplication between the replay_events functions on the send and receive sides, but I'm not convinced whether we should refactor further by introducing a SessionEvent trait that both the receive and sender SessionEvent types could both implement (and the same on Receive/SendSession).

Pull Request Checklist

Please confirm the following before requesting review:

As we now have AsyncSessionPersister to enable async persistence, we
need to be able to replay the event log asynchronously too.
@coveralls
Copy link
Collaborator

coveralls commented Jan 15, 2026

Pull Request Test Coverage Report for Build 21076949429

Details

  • 228 of 236 (96.61%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 83.357%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin/src/core/receive/v2/session.rs 131 135 97.04%
payjoin/src/core/send/v2/session.rs 97 101 96.04%
Totals Coverage Status
Change from base Build 21044982881: 0.3%
Covered Lines: 9937
Relevant Lines: 11921

💛 - Coveralls

Copy link
Collaborator

@arminsabouri arminsabouri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cACK be7d040

Just had one question. Otherwise code looks good.


for event in logs {
session_events.push(event.clone());
receiver = receiver.process_event(event)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the previous code if process_event failed we close the session. Is this a regression?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks the close was move to the replay_events call side. Is that right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replay_events does no IO so that it can be reused in the async version of replay_event_log. So it just returns an error here and that's handled with a session close in replay_event_log/_async:

    let (receiver, session_events) = match replay_events(logs.map(|e| e.into())) {
        Ok(r) => r,
        Err(e) => {
            persister.close().await.map_err(|ce| {
                InternalReplayError::PersistenceFailure(ImplementationError::new(ce))
            })?;
            return Err(e);
        }
    };

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a second commit that adds test coverage for this scenario to confirm no regression

When `process_event` fails we close the session. This adds test coverage
for that scenario.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants