Skip to content

Split resume session relays#1399

Open
benalleng wants to merge 2 commits intopayjoin:masterfrom
benalleng:split-resume-session-relays
Open

Split resume session relays#1399
benalleng wants to merge 2 commits intopayjoin:masterfrom
benalleng:split-resume-session-relays

Conversation

@benalleng
Copy link
Collaborator

@benalleng benalleng commented Mar 9, 2026

Closes #1391

This commit ensures that each resumed payjoin-cli session is using a
separate instance of the RelayManager to then check the ohttp connection
independently. This fixes a bug where resuming would converge all
existing sessions to one ohttp relay.

Also, the incompatible_msrv allow seems not needed anymore?

Big pickle wrote most of this code. The free open weight models aren't too shabby now either. Though it did want to recreate the entire config instead of just making the app mutable. I thought this seemed a bit easier to parse as its clear everything else is the same.

Pull Request Checklist

Please confirm the following before requesting review:

This fixes a bug whereby the relay selection was skipped if the config
set a fixed value for the ohttp-keys.
This commit ensures that each resumed payjoin-cli session is using a
separate instance of the RelayManager to then check the ohttp connection
independently.  This fixes a bug where resuming would converge all
existing sessions to one ohttp relay.
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 22861941781

Details

  • 5 of 8 (62.5%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 82.487%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin-cli/src/app/v2/mod.rs 3 6 50.0%
Files with Coverage Reduction New Missed Lines %
payjoin-cli/src/app/v2/mod.rs 1 53.79%
Totals Coverage Status
Change from base Build 22741440695: -0.01%
Covered Lines: 10640
Relevant Lines: 12899

💛 - Coveralls

@benalleng benalleng requested a review from spacebear21 March 9, 2026 15:57
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 cf8b0b7

big pickle? Cool.
Should we / do we already ensure that there is > 2 ohttp relays ?

@spacebear21
Copy link
Collaborator

This PR also includes the commit from #1390, is it meant to supersede that PR? I left a comment there that I think we should address holistically (i.e. is ohttp_keys "cache" config field relevant at all since we refetch every time anyways?)

@benalleng
Copy link
Collaborator Author

I should add blocked as this simply builds on top of #1390

This PR would have simply needed to also add the commit from #1390 as the ohttp_keys have been "cached" by that point so relay selection is skipped and resume would just choose ohttp_relay[0] no matter what. #1390 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

All sessions in resume share the same relay instead of randomizing independently

4 participants