net: SocketServer never reaps finished connections, leaking an fd and a thread handle per connection ever accepted

[Yaraslaut]

Summary

Two transport-lifetime defects in morph::net, both verified by reading, neither reproduced.

Verification status

Inferred from reading the code, not reproduced. Revision: origin/master adfe8e5f plus this branch's doc-only commits. I did not construct a stalled-peer scenario or measure fd growth.

---

1. SocketServer leaks an fd and a thread handle per connection ever accepted

grep -n "_clients\b\|_clientThreads" include/morph/net/socket_server.hpp gives exactly three sites each:

169:            clients = _clients;          <- close()
170:            threads.swap(_clientThreads);
171:            _clients.clear();
259:                _clients.push_back(conn);        <- acceptLoop()
260:                _clientThreads.push_back(std::move(clientThread));
476:    std::vector<std::shared_ptr<ClientConnection>> _clients;
477:    std::vector<std::thread> _clientThreads;

There is no reaping anywhere. When clientLoop returns, _clients still holds a shared_ptr<ClientConnection>, so the ClientConnection — and its TcpSocket socket member — is never destroyed and the fd stays open until close(). Each std::thread likewise stays joinable, retaining its OS stack and TLS.

So the accumulation is per connection ever accepted, not per live connection. The class doc (:42-45, "one thread per accepted connection") and the spec (docs/spec/core/backend.md:1387, "joins every thread it started, so destruction leaves no dangling threads") are both true only at teardown, and neither hints at the steady state. tests/net/test_socket_server.cpp:488 and :831 already exercise fd-exhaustion paths, so the ceiling is a known concern.

2. ~SocketBackend can deadlock against a stuck sendAll

~SocketBackend (socket_backend.hpp:89-93):

std::scoped_lock lock{_socketMtx};
if (_socket.valid()) { _socket.shutdownBoth(); }

sendFrame (:380-387) holds _socketMtx across _socket.sendAll(...), and sendAll (tcp_socket.hpp:327-339) loops on a blocking ::send with no timeout. A thread stuck against a stalled peer's full send buffer holds _socketMtx indefinitely; the destructor then blocks acquiring that same mutex — and the only thing that would unblock the send is the shutdownBoth() it cannot reach.

SocketServer::close() documents this exact trap and deliberately does the opposite (socket_server.hpp:174-183):

"Taking it here would deadlock exactly when the shutdown is most needed: a client thread blocked in sendAll against a stalled peer's full socket buffer holds writeMtx for as long as that send is stuck, and close() has no timeout — it is reached from the destructor. shutdownBoth() is documented safe from any thread and is itself the mechanism that unblocks that send … Locking to 'protect' the socket would therefore wait on the very thing it is trying to interrupt."

Every word applies to ~SocketBackend. shutdownBoth() is documented safe from any thread (tcp_socket.hpp:341-342), so the lock buys nothing the io-thread ownership of _socket's writes does not already give.

Also noted, same review, lower severity

• connectTimeout is per resolved address, not a total bound. tcp_socket.hpp:133's ::poll sits inside the for (addrinfo* rp = resolved; …) loop, and hints.ai_family = AF_UNSPEC (:98) makes multiple candidates normal (localhost → ::1 and 127.0.0.1). Worst case blocks N × timeout. The docs at tcp_socket.hpp:90-93 and socket_backend.hpp:82-83 both state it as a single bound.
• That same poll treats EINTR as connect failure (:133-137, if (pollRc <= 0) { close(fd); continue; }), while every other blocking syscall in the file retries EINTR (:237, :289, :311, :332) — and :211-215 argues at length for exactly that retry. An ordinary delivered signal abandons the candidate address.

What would change the verdict

• Close (1) if something outside these two files reaps _clients/_clientThreads — I found nothing, but a grep over src/ and the ladder would settle it.
• Close (2) if sendFrame is shown to be unreachable concurrently with ~SocketBackend. Note the I/O thread's own Pong/Close echo in drainFrames (socket_backend.hpp:583, :589) goes through the same path.
• Raise either by reproducing: a peer that accepts a connection and never reads, then destroy the backend; or a loop of short-lived connections against one SocketServer with ls /proc/<pid>/fd | wc -l sampled over time.

[Yaraslaut]

Triage: triage: rescope, bug, area: core — narrowed to one defect, two split out

Rescoped for bundling, not for being wrong. All three findings verified; they are simply three independent defects in one issue, and this repository's own rule is to file the second rather than fold it in. Retitled to its surviving finding. Split out:

• net: ~SocketBackend takes _socketMtx across shutdownBoth(), the deadlock SocketServer::close() documents avoiding #506 — ~SocketBackend taking _socketMtx across shutdownBoth(), the deadlock SocketServer::close() documents avoiding. Different file, different class, different fix.
• net: TcpSocket::connect applies connectTimeout per resolved address and treats EINTR as connect failure #507 — TcpSocket::connect's per-address connectTimeout and its EINTR handling, from the "also noted" section. Marked unverified: real by reading, but never timed or reproduced, and I did not want it closing silently when this one closes.

The surviving finding, re-checked

grep -n "_clients\b|_clientThreads" include/morph/net/socket_server.hpp returns exactly three sites each — the push in acceptLoop (:259-260), the swap/clear in close() (:169-171), and the declarations (:476-477). There is no reaping anywhere.

When clientLoop returns, _clients still holds a shared_ptr<ClientConnection>, so the connection — and its TcpSocket socket member — is never destroyed and the fd stays open until close(). Each std::thread stays joinable, retaining its stack and TLS. The accumulation is therefore per connection ever accepted, not per live connection.

Invariant 7 is what makes this worth fixing rather than noting. The class doc (:42-45) and the spec (docs/spec/core/backend.md:1387, "joins every thread it started, so destruction leaves no dangling threads") are both true — at teardown. A test that accepts N connections and then destroys the server passes with or without a reaper. A regression test must sample fd count or thread count while the server is still running, after connections have closed. tests/net/test_socket_server.cpp:488 and :831 already exercise fd exhaustion, so the ceiling is a known concern and the harness exists.

Area: core — there is no area: net label, and morph::net is specced under docs/spec/core/backend.md.