Test persister load and close in FFI bindings by Jolah1 · Pull Request #1835 · payjoin/rust-payjoin

Jolah1 · 2026-08-24T13:36:06Z

Closes #1325.

What

Add load() and close() coverage to the FFI binding test suites — dart, javascript, python and csharp — for the receiver and the sender, sync and async.

Each test asserts load() returns what was saved and the persister is still open, drives a terminal transition, then asserts the persister was closed and load() still returns every event. The receiver's terminal step is cancel(); the sender's is pendingFallback.close(), since cancelling a sender only reaches a pending fallback.

Why

#1287 added save()/save_async() coverage but left load() and close() untested. What these pin down is not the test-utils persister but the binding layer's contract with it: that a terminal save actually calls the foreign close() across the FFI boundary, and that load() is a non-consuming read.

Tests

bash payjoin-ffi/dart/contrib/test.sh — 28 passed
bash payjoin-ffi/python/contrib/test.sh — 21 passed
bash payjoin-ffi/javascript/contrib/test.sh — 39 passed (nodejs and web)
bash payjoin-ffi/csharp/contrib/test.sh — not run locally, no dotnet available in my environment; relying on CI

Disclosure: co-authored by Claude

coveralls · 2026-08-24T13:41:37Z

Coverage Report for CI Build 33742775414

Coverage remained the same at 86.64%

Details

Coverage remained the same as the base build.
Patch coverage: No coverable lines changed in this PR.
No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	16474
Covered Lines:	14273
Line Coverage:	86.64%
Coverage Strength:	343.11 hits per line

💛 - Coveralls

caarloshenriq

Concept nACK

This is a good start toward #1325, but the issue asks for coverage of .load() and .close() more broadly, not only the error paths during replay. Happy-path tests (load returns correct events, close marks the persister as closed) are still missing. I'd reword the PR body to describe this as a partial step rather than Closes #1325.

caarloshenriq · 2026-08-24T18:47:19Z

+#[cfg(test)]
+#[derive(Debug, Clone, Copy)]
+pub(crate) enum PersisterFailure {
+    Load,
+    Close,
+}
+
+#[cfg(test)]
+#[derive(Debug)]
+pub(crate) struct TestStorageError(PersisterFailure);
+
+#[cfg(test)]
+impl fmt::Display for TestStorageError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self.0 {
+            PersisterFailure::Load => write!(f, "load failed"),
+            PersisterFailure::Close => write!(f, "close failed"),
+        }
+    }
+}
+
+#[cfg(test)]
+impl std::error::Error for TestStorageError {}
+
+#[cfg(test)]
+pub(crate) struct FailingPersister<V> {
+    events: Vec<V>,
+    failure: PersisterFailure,
+}
+
+#[cfg(test)]
+impl<V> FailingPersister<V> {
+    pub(crate) fn on_load() -> Self { Self { events: vec![], failure: PersisterFailure::Load } }
+
+    pub(crate) fn on_close(events: Vec<V>) -> Self {
+        Self { events, failure: PersisterFailure::Close }
+    }
+}
+
+#[cfg(test)]
+impl<V> SessionPersister for FailingPersister<V>
+where
+    V: Clone + 'static,
+{
+    type InternalStorageError = TestStorageError;
+    type SessionEvent = V;
+
+    fn save_event(&self, _event: Self::SessionEvent) -> Result<(), Self::InternalStorageError> {
+        Ok(())
+    }
+
+    fn load(
+        &self,
+    ) -> Result<Box<dyn Iterator<Item = Self::SessionEvent>>, Self::InternalStorageError> {
+        match self.failure {
+            PersisterFailure::Load => Err(TestStorageError(PersisterFailure::Load)),
+            PersisterFailure::Close => Ok(Box::new(self.events.clone().into_iter())),
+        }
+    }
+
+    fn close(&self) -> Result<(), Self::InternalStorageError> {
+        match self.failure {
+            PersisterFailure::Load => Ok(()),
+            PersisterFailure::Close => Err(TestStorageError(PersisterFailure::Close)),
+        }
+    }
+}
+
+#[cfg(test)]
+pub(crate) struct FailingAsyncPersister<V>(FailingPersister<V>);
+
+#[cfg(test)]
+impl<V> FailingAsyncPersister<V> {
+    pub(crate) fn on_load() -> Self { Self(FailingPersister::on_load()) }
+
+    pub(crate) fn on_close(events: Vec<V>) -> Self { Self(FailingPersister::on_close(events)) }
+}
+
+#[cfg(test)]
+impl<V> AsyncSessionPersister for FailingAsyncPersister<V>
+where
+    V: Clone + Send + Sync + 'static,
+{
+    type InternalStorageError = TestStorageError;
+    type SessionEvent = V;
+
+    async fn save_event(
+        &self,
+        _event: Self::SessionEvent,
+    ) -> Result<(), Self::InternalStorageError> {
+        Ok(())
+    }
+
+    async fn load(
+        &self,
+    ) -> Result<Box<dyn Iterator<Item = Self::SessionEvent> + Send>, Self::InternalStorageError>
+    {
+        match self.0.failure {
+            PersisterFailure::Load => Err(TestStorageError(PersisterFailure::Load)),
+            PersisterFailure::Close => Ok(Box::new(self.0.events.clone().into_iter())),
+        }
+    }
+
+    async fn close(&self) -> Result<(), Self::InternalStorageError> {
+        match self.0.failure {
+            PersisterFailure::Load => Ok(()),
+            PersisterFailure::Close => Err(TestStorageError(PersisterFailure::Close)),
+        }
+    }
+}
+


Nine #[cfg(test)] annotations on individual items in the module's top-level scope. Since these need pub(crate) visibility for cross-module test access, grouping them in a single gated submodule would be cleaner (following 1213 line):

#[cfg(test)] pub(crate) mod test_support { use super::*; // PersisterFailure, TestStorageError, FailingPersister, FailingAsyncPersister }

One gate, one module, no test-only items floating in production scope.

Done — all four items now live in #[cfg(test)] pub(crate) mod test_support. One gate, and callers import from crate::persist::test_support.

caarloshenriq · 2026-08-24T18:50:16Z

+    async fn replay_surfaces_persistence_failures() {
+        let err = replay_event_log(&FailingPersister::<SessionEvent>::on_load())
+            .expect_err("load failure should fail replay");
+        assert_eq!(err.to_string(), "Persistence failure: load failed");


String-based error assertions couple the test to the exact Display formatting of ReplayError. If someone rewords the message without changing behavior, these break. Matching on the variant (matches!(err, ReplayError::Persistence(_))) or destructuring would be more robust.

Agreed. ReplayError wraps a private InternalReplayError, so the session test modules can't match the variant directly — I added a #[cfg(test)] pub(crate) fn persistence_failure() accessor on ReplayError and an
assert_persistence_failure helper that takes it, then downcasts the ImplementationError source to TestStorageError. That pins both the variant and which fault was injected, and no longer depends on Display.

caarloshenriq · 2026-08-24T18:52:25Z

+        let err = replay_event_log(&FailingPersister::on_close(vec![
+            SessionEvent::CheckedBroadcastSuitability(),
+        ]))
+        .expect_err("close failure should replace the invalid-event error");


This test relies on replay_event_log calling close() even on the error path, so the close error replaces the replay error. That's a subtle contract worth a comment explaining the error chain. If a future refactor of replay_event_log stops calling close() on failure, this test breaks for non-obvious reasons.

Split into replay_surfaces_load_failure and replay_surfaces_close_failure, and the latter now carries a doc comment explaining that replay closes the session on an invalid log, that the close failure therefore replaces the invalid-event error, and that the invalid first event is what drives replay into that close() call at all.

Jolah1 · 2026-08-25T03:23:55Z

Concept nACK

This is a good start toward #1325, but the issue asks for coverage of .load() and .close() more broadly, not only the error paths during replay. Happy-path tests (load returns correct events, close marks the persister as closed) are still missing. I'd reword the PR body to describe this as a partial step rather than Closes #1325.

Fair — added direct happy-path coverage in persist.rs: in_memory_persister_load_and_close and its async twin assert that a fresh persister loads nothing, that load() replays events in save order without consuming the log, and
that close() marks the session closed while leaving the log readable. Reworded the body to "Part of #1325" rather than "Closes".

xstoicunicornx · 2026-09-02T20:36:48Z

As far as I can tell, #1325 relates to the FFI tests and their lack of coverage for the persister load and close methods.

payjoin#1287 added save() and save_async() coverage to the bindings but left load() and close() untested. Nothing pinned the binding layer's contract with a foreign persister: that a terminal save reaches through to close(), and that load() is a read the runtime can repeat rather than one that consumes the log. Cover the receiver and the sender in dart, javascript, python and csharp, sync and async. The two reach their terminal state by different routes: cancelling an initialized receiver is itself terminal, while cancelling a sender only moves it to a pending fallback and the session closes a step later, when the broadcast transition is saved. Assert the persister is open beforehand, closed afterwards, and that load() still returns every event.

Jolah1 · 2026-09-03T10:13:14Z

Retargeted to the FFI bindings — @xstoicunicornx is right that #1325 is about the binding suites, not the payjoin crate.

@caarloshenriq your nACK asked for happy-path load()/close() coverage, and that's here now, just in the bindings rather than persist.rs: each test asserts load() returns the saved events and that a terminal save flips the
persister to closed. Receiver and sender, sync and async, in all four bindings.

The Rust replay error-path commit is dropped from this PR. I'll open it separately with your three points already applied — test_support module, variant matching instead of Display strings, and a doc comment on the close-on-error chain.

Jolah1 marked this pull request as ready for review August 24, 2026 13:38

Jolah1 requested review from DanGould, benalleng and spacebear21 as code owners August 24, 2026 13:38

caarloshenriq reviewed Aug 24, 2026

View reviewed changes

Jolah1 force-pushed the test-persistence-replay-failures branch from 1c3af10 to 042c2f2 Compare August 25, 2026 03:21

Jolah1 force-pushed the test-persistence-replay-failures branch from 042c2f2 to c785c8b Compare September 3, 2026 10:09

Jolah1 changed the title ~~Test replay persistence failures~~ Test persister load and close in FFI bindings Sep 3, 2026

This was referenced Sep 3, 2026

Replay persistence failure paths are unreachable in tests #1857

Open

Test replay persistence failure paths #1858

Open

Navigation Menu

Uh oh!

Test persister load and close in FFI bindings - #1835

Test persister load and close in FFI bindings#1835
Jolah1 wants to merge 1 commit into
payjoin:masterfrom
Jolah1:test-persistence-replay-failures

Jolah1 commented Aug 24, 2026 •

edited

Loading

Uh oh!

coveralls commented Aug 24, 2026 •

edited

Loading

Uh oh!

caarloshenriq left a comment

Uh oh!

caarloshenriq Aug 24, 2026

Uh oh!

Jolah1 Aug 25, 2026

Uh oh!

caarloshenriq Aug 24, 2026

Uh oh!

Jolah1 Aug 25, 2026

Uh oh!

caarloshenriq Aug 24, 2026

Uh oh!

Jolah1 Aug 25, 2026

Uh oh!

Jolah1 commented Aug 25, 2026

Uh oh!

xstoicunicornx commented Sep 2, 2026

Uh oh!

Jolah1 commented Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

Jolah1 commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Tests

Uh oh!

coveralls commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 33742775414

Coverage remained the same at 86.64%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

caarloshenriq left a comment

Choose a reason for hiding this comment

Uh oh!

caarloshenriq Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

Jolah1 Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

caarloshenriq Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

Jolah1 Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

caarloshenriq Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

Jolah1 Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

Jolah1 commented Aug 25, 2026

Uh oh!

xstoicunicornx commented Sep 2, 2026

Uh oh!

Jolah1 commented Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Jolah1 commented Aug 24, 2026 •

edited

Loading

coveralls commented Aug 24, 2026 •

edited

Loading