test: cover immediate client disconnects for `ListenConnections` #310

xyzconstant commented at 12:42 AM on July 17, 2026: contributor

Following on testing the reversed direction ryanofsky suggested in #298, this PR adds a test to cover immediate client disconnects on the server side.

Additionally, this test adds DefaultLogHandler and a log_handler parameter to ListenSetup, allowing individual tests to observe logs by passing a custom log handler. The new test takes advantage of this by catching and skipping Uncaught exception in daemonized task. logs.

NOTE: an issue surfaced on the macOS job, the accept() call in ListenConnections fails for the closed connection, this is a Cap'n Proto bug as reported by ViniciusCestarii in #310#pullrequestreview-4723481031 and its fix is available in the v2 branch.

DrahtBot commented at 12:42 AM on July 17, 2026: none

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.

Type	Reviewers
ACK	ryanofsky
Stale ACK	ViniciusCestarii

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

xyzconstant renamed this:
~~ListenConnections: keep listening after a failed accept~~
test: add coverage for immediate client disconnects in `ListenConnections`
on Jul 17, 2026

xyzconstant commented at 1:09 AM on July 17, 2026: contributor

the macos job failed with:

[ TEST ] listen_tests.cpp:292: ListenConnections survives a client that disconnects before being accepted
mp/proxy.cpp:48: error: Uncaught exception in daemonized task.; exception = kj/async-io-unix.c++:1365: failed: setsocketopt(IPPROTO_TCP, TCP_NODELAY): Invalid argument
stack: 1020dc7cb 1020dcaff 1020dcc60 10079556c
/Users/runner/work/libmultiprocess/libmultiprocess/test/mp/test/listen_tests.cpp:189: failed: expected matched
stack: 10078b28b 1007ac7bf

I came across the same issue on my machine (MacBook Pro M2) too. I'll push a fix in a moment.

xyzconstant force-pushed on Jul 17, 2026

xyzconstant renamed this:
~~test: add coverage for immediate client disconnects in `ListenConnections`~~
Keep listening after a failed accept() in `ListenConnections`
on Jul 17, 2026

xyzconstant marked this as ready for review on Jul 17, 2026

in include/mp/proxy-io.h:909 in 38391ebc19 outdated

 901 | @@ -902,6 +902,12 @@ void _Listen(const std::shared_ptr<Listener>& listener, EventLoop& loop, InitImp
 902 |                  if (resume_accept) _Listen<InitInterface>(listener, loop, init);
 903 |              });
 904 |              _Listen<InitInterface>(listener, loop, init);
 905 | +        },
 906 | +        // Keep listening if a single accept() fails, so the server does not
 907 | +        // stop accepting future connections.
 908 | +        [&loop, &init, listener](const kj::Exception& e) {
 909 | +            MP_LOG(loop, Log::Info) << "IPC server: accept failed.";

ViniciusCestarii commented at 2:19 PM on July 17, 2026:

In "Keep listening after a failed accept in ListenConnections" 38391ebc196d7ece7152a6372993ba104dceb9c6

nit: I believe MP_LOG(loop, Log::Warning) would fit better here and also the kj exception could be logged too

xyzconstant commented at 4:06 PM on July 17, 2026:

Solved at d8e5395ed8e6f21fc847b65148c2ec3c34bce524 (compare)

Thanks!

ViniciusCestarii commented at 3:01 PM on July 17, 2026: contributor

ACK 38391ebc196d7ece7152a6372993ba104dceb9c6 Nice finding! ~~This is nicer than~~ the old code that would just stop listening because of a transient error that KJ didn't catch but also with this change now a non-transient error will spin forever retrying an unrecoverable error which I believe could be fixed in a follow up.

Also I have dug in and I found that this is a KJ bug because it should treat transient errors like this and it was already fixed for capnp v2 which isn't released yet.

~~Given that, I still think this PR is correct and the defensive retry is the right call regardless. The spin forever on permanent failure case remains a valid follow-up on top of this.~~

in test/mp/test/listen_tests.cpp:299 in 38391ebc19 outdated

 294 | +    ListenSetup server;
 295 | +
 296 | +    // Connect and close before the server has a chance to accept() the
 297 | +    // connection.
 298 | +    int fd = server.listener.MakeConnectedSocket();
 299 | +    close(fd);

ryanofsky commented at 3:37 PM on July 17, 2026:

In commit "Keep listening after a failed accept in ListenConnections" (38391ebc196d7ece7152a6372993ba104dceb9c6)

Can this be checked with KJ_SYSCALL?

ryanofsky commented at 4:10 PM on July 17, 2026:

In commit "Keep listening after a failed accept in ListenConnections" (d8e5395ed8e6f21fc847b65148c2ec3c34bce524)

Would be good to add a comment here that this close is a little racy, if the socket not closed before accepted the test could appear to crash when there is still a problem.

xyzconstant force-pushed on Jul 17, 2026

xyzconstant commented at 4:05 PM on July 17, 2026: contributor

Thanks for the review @ViniciusCestarii!

Also I have dug in and I found that this is a KJ bug because it should treat transient errors like this and it was already fixed for capnp v2 which isn't released yet.

Nice catch! Now it's clear why this wasn't failing on other platforms.

Given that, I still think this PR is correct and the defensive retry is the right call regardless. The spin forever on permanent failure case remains a valid follow-up on top of this.

You're right on this, I was considering this too. I'm yet to figure out which exceptions we want to stop the accept loop for and which we want to retry, so a follow-up is the right call to me. If other reviewers would rather it be solved in this PR, I'm happy to tackle it here though.

xyzconstant force-pushed on Jul 17, 2026

ryanofsky commented at 4:12 PM on July 17, 2026: collaborator

Code review 38391ebc196d7ece7152a6372993ba104dceb9c6

Nice catch and elegant test! However, I think the suggested fix is too dangerous because retrying whenever accept fails could lead to an infinite loop if the accept failed for reasons other than this macos/capnproto bug, like file descriptors being closed, or resources being exhausted.

I think it'd be good to add the test but not the fix here, and make the test allow the setsocketopt(IPPROTO_TCP, TCP_NODELAY): Invalid argument error (https://github.com/bitcoin-core/libmultiprocess/actions/runs/29545518652/job/87776862017#step:4:126) that results on macos when the capnproto version is less than whatever capnproto version fixes this to document the bug. Separately we should make sure this is fixed in the capnproto version we are using in Bitcoin Core and consider adding a patch to the Bitcoin Core depends build if needed temporarily.

ViniciusCestarii commented at 5:21 PM on July 17, 2026: contributor

Thinking back here, I agree with @ryanofsky. In my review I understimated the spin forever on permanent failure case and the fix truly belongs to KJ and not here because if we were to try to reliably distinguish a transient error from a non transient error here we would basically be doing what KJ was supposed to do

test: allow custom log handler in `ListenSetup`

Add `DefaultLogHandler` and a `log_handler` parameter so tests can provide
their own handler and observe log output more easily.

140d9ba6ff

xyzconstant force-pushed on Jul 20, 2026

xyzconstant renamed this:
~~Keep listening after a failed accept() in `ListenConnections`~~
test: cover immediate client disconnects for `ListenConnections`
on Jul 20, 2026

xyzconstant force-pushed on Jul 20, 2026

xyzconstant commented at 6:58 PM on July 20, 2026: contributor

Thanks for the reviews, addressed feedback and force-pushed: d8e5395ed8e6f21fc847b65148c2ec3c34bce524 -> b5b357f22a37a59dd019caebe283b92eeb3b76c6 (compare)

re: #310#pullrequestreview-4724053865

I think it'd be good to add the test but not the fix here, and make the test allow the setsocketopt(IPPROTO_TCP, TCP_NODELAY): Invalid argument error (https://github.com/bitcoin-core/libmultiprocess/actions/runs/29545518652/job/87776862017#step:4:126) that results on macos when the capnproto version is less than whatever capnproto version fixes this to document the bug. Separately we should make sure this is fixed in the capnproto version we are using in Bitcoin Core and consider adding a patch to the Bitcoin Core depends build if needed temporarily.

Fair concern and I think you're right. The fix was designed to pass the test, assuming this behavior would be tolerated. So it's better we drop it, even though it means the test has a workaround for this macOS bug, which I think we could be okay with.

xyzconstant commented at 11:24 PM on July 23, 2026: contributor

Just opened #319 to keep track of the macOS accept crash.

in test/mp/test/listen_tests.cpp:327 in b5b357f22a

 322 | +    const auto deadline = std::chrono::steady_clock::now() + FAILURE_TIMEOUT;
 323 | +    auto accept_error_seen = [&] { Lock lock(mutex); return accept_error; };
 324 | +    while (!accept_error_seen() && server.DisconnectedCount() < 1 && std::chrono::steady_clock::now() < deadline) {
 325 | +        std::this_thread::sleep_for(std::chrono::milliseconds(1));
 326 | +    }
 327 | +    KJ_EXPECT(accept_error_seen() || server.DisconnectedCount() >= 1);

ryanofsky commented at 4:33 AM on July 30, 2026:

In commit "test: cover immediate client disconnects for ListenConnections" (b5b357f22a37a59dd019caebe283b92eeb3b76c6)

Would be good to tighten this check to something like server.DisconnectedCount() == (accept_error_seen() ? 0 : 1) . I think this would also clarify what behavior is actually expected.

Also would be good to require accept_error_seen() is false unless compiling for macos ( #ifdef __APPLE__ )

xyzconstant commented at 3:53 PM on July 30, 2026:

Done at 496fb84e69f8dbf8fd9e2d29a03b44e1c73db3d2

Thanks!

ryanofsky approved

ryanofsky commented at 4:34 AM on July 30, 2026: collaborator

Code review ACK b5b357f22a37a59dd019caebe283b92eeb3b76c6. Thanks for the test! Left suggestions below but think this could also be merged as is.

DrahtBot requested review from ViniciusCestarii on Jul 30, 2026

test: cover immediate client disconnects for `ListenConnections`

Connect to the listening socket and `close()` the connection right away,
note that this is racy and the connection might be accepted normally if
the `close()` syscall doesn't win the race.

Additionally, a custom log handler is added to catch "Uncaught exception
in daemonized task." logs, generated by a Cap'n Proto bug that shows up
on macOS, so CI does not fail on them.

496fb84e69

xyzconstant force-pushed on Jul 30, 2026

xyzconstant commented at 3:52 PM on July 30, 2026: contributor

Thanks for the review @ryanofsky!

I force-pushed changes based on your suggestion to tighten the KJ_EXPECT(...) check, plus the KJ_REQUIRE() check on accept_error_seen() for non-macOS builds.

b5b357f22a37a59dd019caebe283b92eeb3b76c6 -> 496fb84e69f8dbf8fd9e2d29a03b44e1c73db3d2 (compare)

in test/mp/test/listen_tests.cpp:307 in 496fb84e69

 302 | +    ListenSetup server(std::nullopt, [&](mp::LogMessage log) {
 303 | +        // On macOS, accept() can fail if the peer closes the connection before it is accepted.
 304 | +        // The event loop then reports this as an uncaught task exception. We catch and ignore
 305 | +        // this specific error here so that the corresponding CI job does not fail.
 306 | +        //
 307 | +        // This is a Cap'n Proto bug, a fix is available in the v2 branch at: https://github.com/capnproto/capnproto/commit/7df5bd078f389ded313479981bd0ae06cbcdfe1b#diff-ec577ad66535f58f6d7396ea51d3e56c0065308aa8fb02751cd6a8cfaa67252fR1358-R1372

ryanofsky commented at 3:33 PM on August 3, 2026:

In commit "test: cover immediate client disconnects for ListenConnections" (496fb84e69f8dbf8fd9e2d29a03b44e1c73db3d2)

Now that the fix is merged would be nice to simplify the comment to something like This a Cap'n Proto bug, fixed by https://github.com/capnproto/capnproto/pull/2748

ryanofsky approved

ryanofsky commented at 3:36 PM on August 3, 2026: collaborator

Code review ACK 496fb84e69f8dbf8fd9e2d29a03b44e1c73db3d2. Since last review just tightened the checks to only allow the error on macos

ryanofsky merged this on Aug 3, 2026

ryanofsky closed this on Aug 3, 2026