Fix resume succeding prematurely by zealsham · Pull Request #1573 · payjoin/rust-payjoin

zealsham · 2026-05-22T14:18:13Z

This addresses #1545, upon resuming a session where the sender posted the original proposal but went ofline, the cli succeds prematurely and outputs "payjoin transaction detected in mempool" because the cli treats any Ok(_) as sucess.

This also addresses a subtle issue where after a certain period without tx detected in the mempool the the monitor_proposal function returned
Err("Timeout waiting for payment confirmation after 5s").
That looked like a failure even though the session should stay open in Monitor. And finally the missleading "All resumed sessions completed" message.

my initial analysis of the issue in #1545 was wrong, as the intended solution witht that did not fix it. i had LLM (gemini) help with some of the root cause analysis

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.

coveralls · 2026-05-22T14:27:22Z

Coverage Report for CI Build 26514540156

Coverage increased (+0.05%) to 85.19%

Details

Coverage increased (+0.05%) from the base build.
Patch coverage: 8 uncovered changes across 1 file (26 of 34 lines covered, 76.47%).
No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
payjoin-cli/src/app/v2/mod.rs	33	25	75.76%
Total (2 files)	34	26	76.47%

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	13720
Covered Lines:	11688
Line Coverage:	85.19%
Coverage Strength:	394.62 hits per line

💛 - Coveralls

xstoicunicornx

I like how you are breaking out session level information but I think this solution is a bit over engineered and it seems unnecessary to introduce additional state.

My comments below are a bit general but feel free to take more concrete direction from my branch https://github.com/xstoicunicornx/rust-payjoin/tree/pr1573-alt

xstoicunicornx

A bit more feedback.

xstoicunicornx · 2026-05-25T23:34:48Z

            } => {
-                println!("All resumed sessions completed.");
+                let closed = results.iter().filter(|r| matches!(r, Ok(Ok(())))).count();
+                let total = spawned_recv_ids.len() + spawned_send_ids.len();


Suggested change

let total = spawned_recv_ids.len() + spawned_send_ids.len();

let total = results.len();

Why not just this? Then the spawned_recv_ids and spawned_send_ids aren't needed anymore.

xstoicunicornx · 2026-05-25T23:35:31Z

-                wait_for_stdout_match(&mut stdout, |line| {
-                    line.contains("All resumed sessions completed.")
-                }),
+                wait_for_stdout_match(&mut stdout, |line| line.contains("Resume done:")),


Suggested change

wait_for_stdout_match(&mut stdout, |line| line.contains("Resume done:")),

wait_for_stdout_match(&mut stdout, |line| line.contains("1/1 session(s) closed.")),

This is supposed to assert that the resumed session was completed, checking for "Resume done:" only asserts that summary was printed not that the session was actually closed.

xstoicunicornx · 2026-05-25T23:39:14Z

+            Err(_) => {
+                let id = persister.session_id();
+                println!(
+                    "No payjoin transaction seen yet (waited {timeout_duration:?}). Session {id} \


Suggested change

"No payjoin transaction seen yet (waited {timeout_duration:?}). Session {id} \

"No payjoin transaction seen yet, stopping (waited {timeout_duration:?} sec). Session {id} \

xstoicunicornx · 2026-05-25T23:42:39Z

+                let id = persister.session_id();
+                println!(
+                    "No payjoin transaction seen yet (waited {timeout_duration:?}). Session {id} \
+                     remains open; run `payjoin-cli resume` to keep watching."


Suggested change

remains open; run `payjoin-cli resume` to keep watching."

remains open. Run `payjoin-cli resume` to keep watching."

nit: semicolon isn't used anywhere else

thanks for catching that

spacebear21 · 2026-05-26T15:12:09Z

+                    }
+                }
+                println!(
+                    "Resume done: {closed}/{total} session(s) closed. \


I don't think printing the completed/total sessions count is useful or worth the complexity. It seems sufficient to print per-session results (session XYZ either completed, is still polling, or had an error).

spacebear21 · 2026-05-26T15:13:58Z

+            Err(_) => {
+                let id = persister.session_id();
+                println!(
+                    "No payjoin transaction seen yet (waited {timeout_duration:?}). Session {id} \
+                     remains open; run `payjoin-cli resume` to keep watching."
+                );
+                Err(anyhow!(
+                    "No payjoin transaction detected in mempool within {timeout_duration:?}"
+                ))
+            }


this change seems unrelated to the resume fix, if really necessary it should be a separate commit.

Yes , it's not related to resume fix , more like a helpful ux . i'll extract to diff commit or drop it entirely

spacebear21 · 2026-05-26T15:15:37Z

+                    Ok(OptionalTransitionOutcome::Progress(())) => {
                        println!("Payjoin transaction detected in the mempool!");
                        return Ok(());
                    }
-                    Err(_) => {
-                        // keep polling
-
-                        continue;
-                    }
+                    Ok(OptionalTransitionOutcome::Stasis(_)) => continue,
+                    Err(_) => continue,


This is really the meat of the fix (we were previously returning on Stasis). Would be nice to see some test coverage like https://github.com/payjoin/rust-payjoin/pull/1575/changes#diff-162f6469a0779bd5b6f735aa260e95f854b0e8c0d7cc9da278a2c2f51d394c15R316

@xstoicunicornx since your branch already has the needed test coverage, i can borrow that heavily and then credit you as co-author in the commit

This addresses payjoin#1545, upon resuming a session where the sender posted the original proposal but went ofline, the cli succeds prematurely and outputs "payjoin transaction detected in mempool" because the cli treats any Ok(_) as scucess. This also addresses a subtle issue where after a certain period without tx detected in the mempool the the monitor_proposal function returned Err("Timeout waiting for payment confirmation after 5s"). That looked like a failure even though the session should stay open in Monitor. And finally the missleading "All resumed sessions completed" message. Co-authored-by: xstoicunicornx <xstoicunicornx@users.noreply.github.com>

zealsham force-pushed the premature-resume branch from 2303b41 to 56c9bb2 Compare May 22, 2026 14:19

zealsham mentioned this pull request May 22, 2026

Fix payjoin-cli receiver premature resolution #1575

Draft

2 tasks

xstoicunicornx requested changes May 23, 2026

View reviewed changes

Comment thread payjoin-cli/src/app/v2/mod.rs Outdated

Comment thread payjoin-cli/src/app/v2/mod.rs Outdated

Comment thread payjoin-cli/src/app/v2/mod.rs Outdated

zealsham force-pushed the premature-resume branch 2 times, most recently from 0809f0c to 0e3690a Compare May 25, 2026 21:20

xstoicunicornx requested changes May 25, 2026

View reviewed changes

spacebear21 requested changes May 26, 2026

View reviewed changes

zealsham force-pushed the premature-resume branch from 0e3690a to 0ed9646 Compare May 27, 2026 12:47

Add descriptive message to monitor failing to detect TX in mempool

7779c42

zealsham force-pushed the premature-resume branch from 0ed9646 to 7779c42 Compare May 27, 2026 13:35

	let total = spawned_recv_ids.len() + spawned_send_ids.len();
	let total = results.len();

	wait_for_stdout_match(&mut stdout, \|line\| line.contains("Resume done:")),
	wait_for_stdout_match(&mut stdout, \|line\| line.contains("1/1 session(s) closed.")),

	"No payjoin transaction seen yet (waited {timeout_duration:?}). Session {id} \
	"No payjoin transaction seen yet, stopping (waited {timeout_duration:?} sec). Session {id} \

	remains open; run `payjoin-cli resume` to keep watching."
	remains open. Run `payjoin-cli resume` to keep watching."

Conversation

zealsham commented May 22, 2026

Uh oh!

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

Uh oh!

Coverage Report for CI Build 26514540156

Coverage increased (+0.05%) to 85.19%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

xstoicunicornx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xstoicunicornx 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented May 22, 2026 •

edited

Loading