Add test coverage for ConnectStream

[xyzconstant]

ref: #183

This PR adds test coverage for the client method ConnectStream, as suggested by @ryanofsky.

The following cases are tested:

1. Connecting to a socket serving a valid init interface
2. Passing a disconnected socket.
3. Passing a disconnected socket with an Init interface with no construct() method.
4. Passing a disconnected socket with an Init interface with no construct() method and making no calls.
5. Passing a live socket that disconnects after some data is received.
6. Passing a socket from a listening socket (accept()) that disconnects after some data arrives

Additionally, a new FooInit interface is added, and the UnixListener class introduced in #269 to set up listening tests is extracted to a shared file so the new connect_tests.cpp file can consume it.

Bugs found

The added tests exposed a couple of bugs, whose fixes are included in this PR as the first 2 commits. Details about them are below:

Fix 1: Use-after-free race found in the NetBSD 9.4 job

The NetBSD 9.4 job was crashingin the new test connect_tests.cpp:94 with:

assertion "m_loop" failed: file ".../include/mp/proxy.h", line 59,
function "mp::EventLoop& mp::EventLoopRef::operator*() const"
*** Received signal #6: Abort trap

Asked Claude to try to reproduce it locally and came up with placing (in ConnectStream)

std::this_thread::sleep_for(std::chrono::milliseconds(100));

between the loop.sync(...) block and the client construction to widen the race window so that the event loop thread wins the race (as it consistently did in the failing job), thereby exposing the issue.

The sequence of events was as follows: passing a disconnected socket fd causes the RPC system to hit EOF on the first read. The event loop resumes right after the loop.sync(...) block, resolves m_network.onDisconnect(), whose handler (registered inside the same sync block) runs delete connection_ptr. Connection::~Connection resets its EventLoopRef m_loop member, and the memory is freed.

The caller thread then wakes up and constructs the ProxyClient<InitInterface>, whose ProxyContext constructor reads the freed Connection:

libmultiprocess/src/mp/proxy.cpp

Line 82 in 8412fcd

ProxyContext::ProxyContext(Connection* connection) : connection(connection), loop{*connection->m_loop} {}

The fix is included in this PR as the first commit. It moves the onDisconnect handler registration to a second loop.sync(...) call just after the proxy client has been constructed.

Fix 2: Connection leak on failed construct() prevents EventLoop::loop() from exiting

When a capnp init interface declares a construct() method (as in example/init.capnp and Bitcoin Core's Init interface), the generated ProxyClient constructor calls it automatically, making a blocking IPC call while the client is being constructed inside ConnectStream.

If that call fails, such as when connecting to a disconnected socket, the exception escapes ProxyClientBase's constructor. Normally, cleanup occurs in the destructor but if the constructor throws no object is created, and the destructor doesn't run. As a result, cleanup functions are not called, the Connection object is leaked, and its EventLoopRef prevents EventLoop::loop() from exiting.

The second commit addresses this by running cleanup functions before rethrowing, which releases the client capability, unregisters the disconnect callback, and deletes the connection when it is owned by the client.

Alternatively, the Sub::construct() call could be removed from the ProxyClientBase constructor and called by ConnectStream after client creation. I am open to switching to this approach if preferred.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline and AI policy for information on the review process.
A summary of reviews will appear here.

Conflicts

No conflicts as of last run.

[ryanofsky]

Thanks for following up to #183 with these tests. They do seem potentially useful. Here is feedback I'd have:

• The first 3 tests seems like they are mostly in good shape. You would just want to wrap these in try/catch to and assert expected exceptions are thrown. For the second test it would seem fine to accept both "called after disconnect" and "interrupted by disconnect" exceptions since we already have existing tests testing for each of these errors more specifically.

• The first 3 tests do overlap a lot with disconnect tests already in test.cpp, and it's a little questionable if having all 3 tests adds much value. The first test could be is nice because it directly tests connecting to a non-capnp server that disconnects ignoring whatever is sent. But the second and third tests just sending the same disconnect at later points in time and needing MSG_PEEK complexity would not seem to add as much value.

• For the 4th test having the client hang as long as server hangs is probably expected behavior. It would be good to make sure that client can still disconnect or cancel the calls if the the server hangs. Or that client calls are able to time out correctly.

• It could make sense to rebase this on #269 or rebase if that could help with the "Add the case with an actual mkdtemp/socket/bind/listen setup" follow up comment.

• Maybe (not sure) it could be interesting to have tests working in opposite direction with server providing dummy interface and clients connecting and disconnecting suddenly. Maybe it could also be useful to have tests sending garbage bytes and making sure clients and servers handle them cleanly. It might even be useful to use fuzzing for this though it might not be a good use of fuzzing resources since it would mostly only be fuzz testing capnproto code and only a little bit of libmultiprocess exception/cleanup handling code.

Overall the tests here seem reasonable to add. It seems good to have at least 1-2 tests verifying disconnects are processed when a capnproto client connects to a non-capnproto server.

(Relatedly, there are also other disconnect tests that could be added at different points during capnproto connections, which I started to write in #201 (comment) and https://github.com/ryanofsky/libmultiprocess/commits/pr/distest.2 but was never able to really finish due to complexity of trying to set up and cover all of the relevant cases. Just mentioning this for completeness, though. There's probably not an obviously place to follow up with this at the moment.)

[xyzconstant]

Thanks for the feedback @ryanofsky!

I've just rebased to master now that #269 has been merged and added a new commit to move UnixListener to its own dedicated file.

Also, I've made changes to the tests (please check the description), keeping the first 2 tests (Notice that I've dropped the MSG_PEEK setup as you suggested.) now catching errors in a try/catch block, plus a Unix domain socket that disconnects after some data arrives and a successful case connecting to a valid libmultiprocess server.

It would be good to make sure that client can still disconnect or cancel the calls if the the server hangs. Or that client calls are able to time out correctly.

This is my current focus. I'm looking more into it, but I think this work might need its own branch so we keep test coverage scoped to this PR.

Maybe (not sure) it could be interesting to have tests working in opposite direction with server providing dummy interface and clients connecting and disconnecting suddenly. Maybe it could also be useful to have tests sending garbage bytes and making sure clients and servers handle them cleanly. It might even be useful to use fuzzing for this though it might not be a good use of fuzzing resources since it would mostly only be fuzz testing capnproto code and only a little bit of libmultiprocess exception/cleanup handling code.

Good, these ideas are great and definitely worth exploring. Happy to tackle them after this.

[xyzconstant]

This PR is now ready for review. I've updated the description as well!

[xyzconstant]

The netbsd 9.4 job exposed a use-after-free race condition in ConnectStream on early disconnects, included a new commit in this PR that fixes it.

[maflcko]

LLM Linter (✨ experimental)

Possible typos and grammar issues:

Initalize -> Initialize [misspelling in the socket setup comment; intended meaning is clear but the word is misspelled]

2026-07-10 05:29:42

[xyzconstant]

Thanks!

[xyzconstant]

The netbsd 9.4 job exposed a use-after-free race condition in ConnectStream on early disconnects, included a new commit in this PR that fixes it.

Updated the description expanding more on this issue.

[xyzconstant]

Added the FooInit test interface which declares construct() so we could trigger that call against a disconnected socket in tests. It exposed a Connection leak, which, for instance, prevented the EventLoop::loop() from ever exiting.

Updated the description with more details about it and included a fix in the second commit.

[ryanofsky]

Code review 96c32f3

Nice tests and fixes! The changes here look pretty good. I would just suggest a number of things to make this easier to understand and review:

• Since this PR is not only adding test coverage, would change title to something like "Fix error handling when creating clients" and give a description like "Avoid use-after-free if socket is disconnected before ConnectStream connects, and avoid leaks and hangs if client construct calls throw. Also add tests to cover these and other client connection errors"

• Would suggest adding the tests before making the bugfixes, otherwise it's not clear how the tests and bugfixes relate. It looks like both bugs are caught by the "Passing a disconnected socket will throw" test which is a very short test, and none of the other tests depend on fixes. So it would be clearer to add all the new tests except "Passing a disconnected socket will throw" initially, to be clear they are unrelated to the bugs.

• PR description is very long because it goes into detail about the history of the bugs and you how debugged them. I think this information is useful and interesting but it makes it harder to get a quick understanding, so I'd suggest moving this detail into separate github issues describing each bug and adding "Fixes #" to the relevant fix commits so they will be closed when this PR is merged.

• It would be good if description of the bugs mentioned they are not new and have always been present. I found current description of the bug in 91f015e that begins with "Previously," confusing because I thought it was referring to something that was previously working but now broken, when actually it's fixing something that has always been broken.

• I think it might be clearer to fix both bugs and add the short "Passing a disconnected socket will throw" all in a single commit. The first fix 91f015e looks correct but it would seem nicer if it just let ProxyClientBase destructor be fully responsible for destroying the connection when destroy_connection is true instead of requiring the caller to destroy it. So could add something like if (destroy_connection) { sync([&]{ connection->onDisconnect(...); } } the end of the ProxyClientBase constructor to make ConnectStream simpler than it was before, instead of more complicated.

• I think it would be good to add a comment to ConnectStream that it calls the InitInterface.construct method if one is present, so it may block or throw.