test: cover immediate client disconnects for `ListenConnections` by xyzconstant · Pull Request #310 · bitcoin-core/libmultiprocess

xyzconstant · 2026-07-17T00:42:33Z

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 (review) and its fix is available in the v2 branch.

DrahtBot · 2026-07-17T00:42:43Z

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  into the comment that the bot should ignore.

xyzconstant · 2026-07-17T01:09:36Z

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.

ViniciusCestarii

ACK 38391eb 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.~~

xyzconstant · 2026-07-17T16:05:12Z

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.

ryanofsky

Code review 38391eb

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 · 2026-07-17T17:21:00Z

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

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

xyzconstant · 2026-07-20T18:58:38Z

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

re: #310 (review)

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 · 2026-07-23T23:24:23Z

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

ryanofsky

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

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.

xyzconstant · 2026-07-30T15:52:24Z

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.

b5b357f -> 496fb84 (compare)

ryanofsky

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

ryanofsky · 2026-08-03T15:33:24Z

+        // The event loop then reports this as an uncaught task exception. We catch and ignore
+        // this specific error here so that the corresponding CI job does not fail.
+        //
+        // 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


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

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

c39c785 doc: note construct() call in valid init interface test (xyzconstant) b9c36c6 test: close sockets unconditionally and check errors with KJ_SYSCALL (xyzconstant) 7eb741e test: drop unnecessary KJ_EXPECT(true) (xyzconstant) 113f1d4 test: join server thread unconditionally in connect tests (xyzconstant) 44bc463 test: drop mp:: prefixes in connect tests (xyzconstant) 038d33e test: share DefaultLogHandler between test files (xyzconstant) b54a163 test: drop TestSetup socket members in connect tests (xyzconstant) 70467c5 test: add m_ prefix to TestSetup members in connect tests (xyzconstant) cc260f2 test: replace capnp fix link with upstream PR (xyzconstant) Pull request description: Addresses review suggestions left (all of them made by ryanofsky) in the now-merged PRs #298 and #310. These are non-critical test cleanups (naming, simplification, comments, etc.) with zero changes to library code. ACKs for top commit: ryanofsky: Code review ACK c39c785. Thanks for the followup! Tree-SHA512: e703d4508f46978c0be7831d3be04c831829aa5de08873e93fbe4ba02b0a5db71554fd80c30058336a4b3f3a620f496d5fb4bfee66f99fc24234073fa98db4c4

d0eea62c58 ci: add Windows cross-compilation config using MinGW and Wine ae6cf017d4 test: Add TCP SocketListener and Windows compat to socketlistener.h 1938c199d7 util: make pthreads optional on Windows to enable MSVC builds 97011d4008 util: Fix Windows SpawnProcess hang when child exits before connecting to named pipe 7861351f24 util: Add Windows support ee09c2b269 util: Add Windows CommandLineFromArgv escaping function 860e2ffb5c util, test: guard Unix-only code for Windows build c79b49afe1 util, test: Replace UnixListener with SocketListener 15aa914f03 util, test: Add CloseSocket, use SocketId be5f87655a util: Improve SpawnProcess API and documentation 390b5f901f Merge bitcoin-core/libmultiprocess#344: test: listen_tests and connect_tests follow-ups e18ca520f4 Merge bitcoin-core/libmultiprocess#343: test: fix race in connect_tests disconnect-deferred-failure test c39c7850c6 doc: note construct() call in valid init interface test b9c36c6175 test: close sockets unconditionally and check errors with KJ_SYSCALL 7eb741e635 test: drop unnecessary KJ_EXPECT(true) 113f1d4d28 test: join server thread unconditionally in connect tests 44bc4630bc test: drop mp:: prefixes in connect tests 038d33eb31 test: share DefaultLogHandler between test files b54a163308 test: drop TestSetup socket members in connect tests 70467c5a72 test: add m_ prefix to TestSetup members in connect tests cc260f2526 test: replace capnp fix link with upstream PR 137a6e4e03 test: fix race in connect_tests disconnect-deferred-failure test 8dab0d4bde Merge bitcoin-core/libmultiprocess#341: ci: add -Wextra-semi to llvm config b3b134eed8 ci: add -Wextra-semi to llvm config bdd0cd6941 Merge bitcoin-core/libmultiprocess#339: refactor: add `[[noreturn]]` attributes a779a09764 ci: add -Wmissing-noreturn 636aaff576 refactor: add missing [[noreturn]] attributes cc11c2b1b4 Merge bitcoin-core/libmultiprocess#338: test: check ReadList return value 2d6e863c77 Merge bitcoin-core/libmultiprocess#334: ci: Set CMAKE_BUILD_PARALLEL_LEVEL to enable parallelism by default d4d10ff98a Merge bitcoin-core/libmultiprocess#332: ci: add -Wextra-semi to default config b540e70f25 Merge bitcoin-core/libmultiprocess#324: proxy: Name threads spawned by the event loop e5e367e785 Merge bitcoin-core/libmultiprocess#312: util: report back child errors to parent and throw 2220df68c9 Merge bitcoin-core/libmultiprocess#298: Fix error handling when creating clients (`mp::ConnectStream`) 51defb79ef Merge bitcoin-core/libmultiprocess#340: ci: Update `capnproto` prerequisites on NetBSD 7e94790b08 ci: Update `capnproto` prerequisites on NetBSD 9f25ffca5b test: Cover OS thread names for worker, pool, and async threads 648a18589c proxy: Name threads spawned by the event loop 49834b2609 ci: add -Wextra-semi to default config fae9a637e3 example: Remove unused kj/async.h include bb473690c9 Fix error handling when creating clients 44d191420c Add test coverage for ConnectStream 231361ae5a Correct stale UnixListener doc comment 060c1a50d0 Extract `UnixListener` class to a dedicated file 62f25af06c test: check ReadList return value ce51d73725 ci: Set CMAKE_BUILD_PARALLEL_LEVEL to enable parallism in build jobs by default 67302cd132 Merge bitcoin-core/libmultiprocess#331: Remove code for Cap'n Proto versions before 0.9 f13c64ab54 Merge bitcoin-core/libmultiprocess#330: ci: Compile with minimum supported g++ in olddeps 8e026f6625 Merge bitcoin-core/libmultiprocess#327: build: avoid unnecessary capnp-rpc dependency for mpgen e5206e9eb5 Merge bitcoin-core/libmultiprocess#325: cmake: Remove `QUIET` option from `find_package(CapnProto ...)` 879efea2bc Merge bitcoin-core/libmultiprocess#321: ci: Roll NetBSD releases to 11.0, drop 9.4 abf127a314 Merge bitcoin-core/libmultiprocess#317: ipc: Fix mpgen capnp tool path for vcpkg/Windows builds c437d7f107 Merge bitcoin-core/libmultiprocess#310: test: cover immediate client disconnects for `ListenConnections` 31bff8a673 Merge bitcoin-core/libmultiprocess#307: refactor: memcpy -> std::ranges::copy f355108b0a Merge bitcoin-core/libmultiprocess#303: type-chrono: Add CustomBuildField/CustomReadField overloads for std::chrono::time_point 2d678177c1 Merge bitcoin-core/libmultiprocess#296: ci: Bump channel to nixos-26.05 3f05b11624 util: kill and reap child on SpawnProcess error 4a56c1837a util: report back child error to parent and throw a9e70dbe77 ci: Add NetBSD release 11.0 2d33b14fb0 ci: Switch to default compiler on NetBSD 9.4 36f7400277 ci: Drop NetBSD release 9.4 bd508311b5 refactor: Drop stray semicolons after function definitions 788f17a850 Remove code for Cap'n Proto versions before 0.9 7402affd0c ci: Pin oldeps config to older nixpkgs channel to compile older cmake with older gcc edf6343562 ci: Compile with minimum supported g++-11 in olddeps fa47449afe cmake: avoid unnecessary capnp-rpc dependency for mpgen a494b764de cmake: Remove `QUIET` option from `find_package(CapnProto ...)` 26452e02d7 refactor: memcpy -> std::ranges::copy e1dcc6eb18 Merge bitcoin-core/libmultiprocess#316: cmake: Fix stale codegen when mpgen binary changes 7a72df02e2 type-chrono: Add CustomBuildField/CustomReadField overloads for std::chrono::time_point 45b685c3f5 type-number, type-chrono: Fix static assert signed/unsigned comparisons 45f6255975 type-number: exclude bool from the integral overload 8d6d464948 Merge bitcoin-core/libmultiprocess#315: Fix startup race in example a6fc80d254 Merge bitcoin-core/libmultiprocess#311: bugfix: clear FD_CLOEXEC in child instead of parent before fork 496fb84e69 test: cover immediate client disconnects for `ListenConnections` 36c6c63520 doc: Document reference-counted EventLoop lifetime 3a997e113c Fix startup race in mpexample f5c15ce33f Merge bitcoin-core/libmultiprocess#323: refactor: access ThreadContext through CurrentThread(), ci: switch Bitcoin Core to master 66298c737f ci: Switch back to Bitcoin Core's master branch 86b4810501 refactor: access ThreadContext through CurrentThread() eea9c64f6e cmake: Fix stale codegen when mpgen binary changes a26a08496b cmake: Fix mpgen capnp tool path for vcpkg/Windows builds 140d9ba6ff test: allow custom log handler in `ListenSetup` 1e0c7ff9a5 util: Clear FD_CLOEXEC in child instead of parent before fork 8550ee6a31 util, refactor: Add ChildFail helper for post-fork child errors 17eab90b52 test: Fix typo in listen_tests.cpp ce865a9ba8 refactor: Directly use value in CustomBuildField 3f221b5bfd Merge bitcoin-core/libmultiprocess#274: Add nonunix platform support 1b0f605606 doc: Remove trailing whitespace d8f8ca3119 ipc: Wrap mpgen main() in try-catch to print errors fbe5a14ad4 ci: Check out bitcoin/bitcoin PR #35084 instead of master 39d3690d83 types: Replace SFINAE with requires clauses to avoid MSVC C2039 error ba68520203 proxy, refactor: Fix C4305 truncation warning in Accessor on MSVC 1d81d47811 util, refactor: Fix PtrOrValue constructor for move-only types on MSVC b883fe1e52 proxy: Fix shutdownWrite() exception handling on macOS with dynamic libraries 0012411ccc proxy: Call shutdownWrite() in Connection destructor 38312ad191 proxy, refactor: Change ConnectStream and ServeStream to accept stream objects e96d5d742a proxy, refactor: Replace EventLoop wakeup fd integers with KJ stream objects db4f9a3d73 cmake: Bump minimum required Cap'n Proto version to 0.9 652934fb79 util, refactor: Add SocketPair() and use it in SpawnProcess 1c6ef7a26c util, refactor: Do not fork() and exec() separately 1389cf3132 util, refactor: Add SpawnConnectInfo type alias and use it c7ca1f00b6 util, refactor: Add SocketId type alias and use it be46a35203 util, refactor: Add ProcessId type alias and use it 91a78db780 doc: Bump version 13 > 14 fa2c56ec27 ci: Bump channel to nixos-26.05 git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: d0eea62c58928f9660b8dc5541a62ea395c565ef

xyzconstant changed the title ~~ListenConnections: keep listening after a failed accept~~ test: add coverage for immediate client disconnects in ListenConnections Jul 17, 2026

xyzconstant force-pushed the server-disconnect-tests branch from f44094f to 38391eb Compare July 17, 2026 01:20

xyzconstant changed the title ~~test: add coverage for immediate client disconnects in ListenConnections~~ Keep listening after a failed accept() in ListenConnections Jul 17, 2026

xyzconstant marked this pull request as ready for review July 17, 2026 02:32

ViniciusCestarii reviewed Jul 17, 2026

View reviewed changes

Comment thread include/mp/proxy-io.h Outdated

xyzconstant force-pushed the server-disconnect-tests branch from 38391eb to 92d8f86 Compare July 17, 2026 15:53

xyzconstant force-pushed the server-disconnect-tests branch from 92d8f86 to d8e5395 Compare July 17, 2026 16:05

ryanofsky reviewed Jul 17, 2026

View reviewed changes

Comment thread test/mp/test/listen_tests.cpp Outdated

Comment thread test/mp/test/listen_tests.cpp Outdated

test: allow custom log handler in ListenSetup

140d9ba

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

xyzconstant force-pushed the server-disconnect-tests branch from d8e5395 to 5fc3064 Compare July 20, 2026 18:04

xyzconstant changed the title ~~Keep listening after a failed accept() in ListenConnections~~ test: cover immediate client disconnects for ListenConnections Jul 20, 2026

xyzconstant force-pushed the server-disconnect-tests branch from 5fc3064 to b5b357f Compare July 20, 2026 18:31

xyzconstant mentioned this pull request Jul 23, 2026

Accepting a dead socket kills IPC listeners in macOS (Cap'n proto bug) #319

Open

ryanofsky approved these changes Jul 30, 2026

View reviewed changes

Comment thread test/mp/test/listen_tests.cpp Outdated

DrahtBot requested a review from ViniciusCestarii July 30, 2026 04:34

xyzconstant force-pushed the server-disconnect-tests branch from b5b357f to 496fb84 Compare July 30, 2026 14:52

ryanofsky approved these changes Aug 3, 2026

View reviewed changes

ryanofsky merged commit c437d7f into bitcoin-core:master Aug 3, 2026
13 checks passed

ryanofsky mentioned this pull request Aug 5, 2026

Add windows support #231

Open

xyzconstant mentioned this pull request Aug 13, 2026

test: listen_tests and connect_tests follow-ups #344

Merged

Uh oh!

Conversation

xyzconstant commented Jul 17, 2026 • edited by ryanofsky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

xyzconstant commented Jul 17, 2026

Uh oh!

ViniciusCestarii left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xyzconstant commented Jul 17, 2026

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ViniciusCestarii commented Jul 17, 2026

Uh oh!

xyzconstant commented Jul 20, 2026

Uh oh!

xyzconstant commented Jul 23, 2026

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xyzconstant commented Jul 30, 2026

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xyzconstant commented Jul 17, 2026 •

edited by ryanofsky

Loading

DrahtBot commented Jul 17, 2026 •

edited

Loading

ViniciusCestarii left a comment •

edited

Loading

ryanofsky left a comment •

edited

Loading