Merged
Conversation
When binding a socket address with port 0, the OS will allocate a free port. The --port CLI argument supports doing this, but the bound port number was not used in any way. This change checks that if --port 0 is specified, --pj-endpoint does not specify a port, and sets the port to the assigned to the listener by the OS before generating payjoin payment URI.
nothingmuch
force-pushed
the
e2e-concurrency-fixes
branch
from
763d377 to
c8441f2
Compare
nothingmuch
force-pushed
the
e2e-concurrency-fixes
branch
2 times, most recently
from
b4f1f62 to
04f6c09
Compare
spacebear21
approved these changes
Aug 4, 2025
Collaborator
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
utACK 04f6c09, some minor code suggestions below.
This solves a very annoying issue I was vaguely aware of but could never get to the bottom of. Thank you muchly
This change adds --root-certificate and --certificate-key CLI options, primarily intended for testing. This avoids reading from /tmp/localhost.der as e2e tests previously did, which technically requires `--test-threads 1` to work reliably (in practice this was rarely an issue to to latency in starting the redis test container)
nothingmuch
force-pushed
the
e2e-concurrency-fixes
branch
from
04f6c09 to
e3f1cdc
Compare
Co-authored-by: spacebear <git@spacebear.dev>
Collaborator
Pull Request Test Coverage Report for Build 16698166812Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Collaborator
Pull Request Test Coverage Report for Build 16725717260Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Collaborator
Pull Request Test Coverage Report for Build 16698130213Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
This was referenced Sep 19, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses two race conditions in the e2e tests:
Free Port Assignment
Each test tried to bind a free port, and then remembered the number passing it in to the
--portoption of payjoin-cli. There is a brief window where the port is unbound, allowing the same number to be given to both tests concurrently.Instead the payjoin-cli can be given
--port 0, binding a free port on its own, and will now use this port when generating a payjoin URI.Self Signed Certificate Isolation
The self signed certificate was written to e.g.
/tmp/localhost.der, so concurrentcargo testruns as well as concurrent tests within a test run overwrote each others certificate.This issue was mostly hidden by the redis test container delaying startup for the v2 test, typically long enough for the v1 test's server and clients to be be initialized consistently.