Stop a stuck client reconnect from blocking every new connection

[vzd3v]

Symptom

etserver stops accepting connections entirely while a single client is reconnecting over a dead network path. et host hangs, no matching line ever appears in the server log, ssh to the same host is unaffected, and service only comes back when the stuck clients are killed.

Observed on 7.0.0 (Ubuntu, port 2022). ss -tln showed the listening socket carrying 22-27 queued connections against a backlog of 32, all from a single source address. That address turned out to be one laptop behind NAT running four et clients abandoned days earlier, each still retrying.

Mechanism

TerminalServer::run() does everything on one thread. The same select() loop accepts TCP clients and accepts the router connection that registers a new session's key:

for (int i : serverPortFds) {
  if (FD_ISSET(i, &rfds)) {
    acceptNewConnection(i);
  }
}
if (FD_ISSET(terminalRouter->getServerFd(), &rfds)) {
  auto idKeyPair = terminalRouter->acceptNewConnection();
  if (idKeyPair.id.length()) {
    addClientKey(idKeyPair.id, idKeyPair.key);
  }
}

acceptNewConnection() takes classMutex to enqueue the new descriptor. A returning client is handled on one of the eight pool threads, which took the same lock around the recovery:

lock_guard<std::recursive_mutex> guard(classMutex);
serverClientState->recoverClient(clientSocketFd);

recoverClient() calls Connection::recover(), which writes a SequenceHeader, reads the client's reply, exchanges catchup buffers, and only then returns. All four of those socket operations block. Reads are bounded by SOCKET_IDLE_TIMEOUT_SEC (30s idle) and SOCKET_ABSOLUTE_TIMEOUT_SEC (60s absolute), checked every iteration. Writes are bounded only by the 30s idle timer, which writeAllOrThrow() resets on every partial write, so a peer that accepts a trickle holds a write open indefinitely.

So one recovery against a client that is no longer reachable holds classMutex for as long as its timeouts allow, and for that entire time nothing can be accepted -- not TCP clients, and not the router connection either, since run() handles both on the same thread. ClientConnection::pollReconnect() keeps trying: apart from its own shutdown it gives up only when the server answers INVALID_KEY. With several such clients the stalls chain back to back, the accept queue fills, and further clients never complete a handshake.

The same lock ordering appears inverted in removeClient() and destroyPartialConnection(), which called Connection::shutdown() while holding classMutex; shutdown() waits on connectionMutex, which a reconnect holds across that same blocking I/O.

Changes

Seven commits, independently reviewable:

1. Stop a stuck reconnect from blocking every new connection. Takes the recovery out of classMutex. The lock was providing two things: keeping the connection alive across the call, and serializing concurrent reconnects for one client. The first is already covered, since serverClientState is a shared_ptr copy taken under the lock. The second moves to the connection's own mutex -- recoverClient() now holds connectionMutex across the whole socket swap rather than just its first half, so two reconnects for one client still cannot interleave, while reconnects for different clients stop blocking each other and the accept loop. connectionMutex is recursive, so recover() re-locking it is fine. Holding it for the whole call also closes two pre-existing windows, where another thread could observe socketFd == -1 with no recovery in progress, or where a shutdown() slipping between a failed recover() and the restore would let the restore revive a torn-down connection. removeClient() and destroyPartialConnection() drop their map entry under classMutex and shut the connection down after releasing it. recover() returns early when the connection is already shutting down, since with the global lock gone a session can be torn down while a reconnect for it is in flight.

2. Make the listen backlog configurable. listen() hardcoded 32, which is small for a daemon whose clients tend to reconnect all at once. Defaults to 128, overridable through backlog in the [Networking] section of et.cfg.

3. Bound the wait for a client's initial payload. handleConnection() looped forever on readPacket(), one log line per second. On the affected server one such thread had been logging for over forty minutes. Now gives up when the connection is shutting down or after ten minutes; the initial payload is what starts the terminal, so a client that never sends one has no session to preserve, and a client whose path recovers before the deadline still reaches the same thread through the normal reconnect. Note that giving up erases the client key, so a client returning after the deadline gets INVALID_KEY and stops retrying.

4. Exit that wait when the server is halted. The connection's own shutdown flag is not what stops this server: TerminalServer::shutdown() only sets halt, and it hides ServerConnection::shutdown(), so the unqualified call in run() resolves to the derived one and nothing ever marks these connections as shutting down. Without a halt check the thread kept looping and run() blocked joining it until the deadline. Checks halt under terminalThreadMutex, the way the session loops in runJumpHost() and runTerminal() already do, reading it into a local so the lock is released before removeClient() takes classMutex.

5. Free the address info in TcpSocketHandler::listen(). A pre-existing leak of the getaddrinfo() result, on both the normal return and the bind failure that throws, while connect() frees it on every exit. It went unnoticed because nothing called listen() in process until this branch added a test, and LeakSanitizer is unavailable on macOS. Also logs the backlog that was actually applied and warns when a configured value is discarded.

6. Test the initial-payload deadline directly. The deadline is read through a protected member so a test subclass can lower it, instead of being unreachable behind a ten minute constant.

7. Refuse a reconnect while one is in flight for that client. Freeing classMutex leaves the recovery occupying the handler thread it runs on, and reconnects for one client serialize on that connection's mutex, so overlapping attempts each park a worker behind the ones ahead of them. A real client cannot overlap its own attempts, since pollReconnect() blocks in its own half of the same handshake before retrying, but an unauthenticated peer can: a reconnect needs nothing but a client id. The refused socket is closed before the live session is touched, and a genuine client returns on its next attempt.

Testing

test/unit_tests/AcceptStarvationTest.cpp registers a client, opens a second connection for the same client whose peer never answers the sequence header -- leaving recover() blocked exactly where a dead path leaves it -- and then requires acceptNewConnection() to complete for an unrelated descriptor. Pre-fix the accept is still blocked when the five second deadline expires; post-fix it returns in under a millisecond. Waiting for the sequence header rather than the ConnectResponse is what makes it deterministic: pre-fix, the recovery's classMutex was taken after that response was written. A second case requires a further reconnect for the same client to be refused within five seconds instead of queueing behind the wedged one.

test/integration_tests/HandleConnectionTimeoutTest.cpp covers all three exits: a connection already shutting down, an open but silent client once the server is halted, and the same client against a one second deadline with neither flag set.

test/unit_tests/TcpSocketHandlerTest.cpp covers the backlog default, an explicit value, and the non-positive fallback. Its listen() case only checks that a custom backlog is accepted -- the kernel queue depth is not portably readable.

Every regression test was confirmed to fail on the commit that introduced it and pass after, on macOS and on Linux. None of them hangs the suite when it fails: the starvation tests unblock their stuck worker in teardown, and the timeout tests detach their worker and skip the teardown that would wait on a held mutex.

• macOS, -DDISABLE_VCPKG=ON -DDISABLE_TELEMETRY=ON: 162/162.
• Ubuntu 24.04, GCC 13.3, same flags: 162/162.
• Ubuntu 24.04, -DSANITIZE_ADDRESS=ON (what the linux_ci asan leg runs): 162/162. Reverting only commit 5 flips the backlog test's exit code from 0 to 1 with a getaddrinfo leak report, so that diagnosis is verified rather than inferred.
• ThreadSanitizer on macOS: no races, including the existing recoverClient coverage in SecurityNoticesTest. One unrelated abort at process exit in SocketHandler helpers read/write encoded payloads reproduces identically on the parent commit.

End to end

The unit tests pin the mechanism; this checks the reported symptom against real binaries. On Ubuntu 24.04 with sshd, a real etserver, etterminal and et: one live session, then reconnects for that session which complete the TCP handshake, read RETURNING_CLIENT and the server's sequence header, and then answer nothing -- the state a dead path produces, driven from a script so the timing is deterministic. Meanwhile a client that has never connected before attempts a session five times, at four second intervals, through a two minute window in which four such reconnects arrive every fifteen seconds.

unpatched     hung 45s  hung 45s  13s  0s  0s
commits 1-6   0s        0s        42s  1s  13s
commit 7      0s        0s        0s   0s  1s

Unpatched, the first two attempts never got a session and were killed at the 45 second timeout, which is the reported symptom. The 13 second and 42 second figures are recoveries timing out and releasing what the next attempt was waiting on. With the incident's own shape -- four separate abandoned clients rather than repeated attempts at one session -- commits 1-6 alone already answer every attempt in under a second.

Not addressed here

A recovery still occupies its handler thread for the length of its timeouts, so enough distinct clients recovering at once can still fill an eight thread pool. Commit 7 caps it at one thread per client, which is what makes the measured case behave; removing the exposure entirely would mean not running recovery on a pool thread at all.

ServerConnection::shutdown() still calls Connection::shutdown() and drains the handler pool while holding classMutex, so a worker waiting on classMutex would never be joined. It is left alone because TerminalServer::shutdown() hides it, as commit 4 describes, so the base version is only reached from tests -- AcceptStarvationTest avoids the hazard only because its extra client fails its handshake before touching classMutex.

PipeSocketHandler::listen() still hardcodes a backlog of 5 for the router socket, drained by the same loop this change protects.

Reconnect authentication remains as #784 left it, needing a PROTOCOL_VERSION bump.

[codecov]

Codecov Report

❌ Patch coverage is 91.39344% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.45%. Comparing base (b74a12e) to head (422ec7b).

Files with missing lines	Patch %	Lines
.../integration_tests/HandleConnectionTimeoutTest.cpp	85.88%	12 Missing ⚠️
test/unit_tests/AcceptStarvationTest.cpp	95.50%	4 Missing ⚠️
src/base/Connection.cpp	25.00%	3 Missing ⚠️
src/base/ServerConnection.cpp	88.88%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #798      +/-   ##
==========================================
+ Coverage   87.14%   87.45%   +0.31%     
==========================================
  Files          75       78       +3     
  Lines        6462     6680     +218     
  Branches      610      621      +11     
==========================================
+ Hits         5631     5842     +211     
- Misses        831      838       +7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vzd3v]

Three adjacent things I left out of this PR on purpose. Happy to add any of them here or send them separately -- whichever you prefer.

1. ServerConnection::shutdown() destroys the handler pool while holding classMutex, and that destructor joins the workers, so a worker waiting on classMutex would never be joined. Unreachable in etserver today because TerminalServer::shutdown() hides it and only tests reach the base version. The fix is the pattern this PR already uses twice: take the map entries under the lock, act after releasing it.

2. PipeSocketHandler::listen() hardcodes a backlog of 5 for the router socket, which the same accept loop drains, and it is the one call in that function whose return value goes unchecked. Commit 2 would cover it the same way.

3. Recovery still holds its handler thread for the length of its timeouts. Commit 7 caps that at one thread per client, so eight clients on dead paths at once can still fill the pool. A configurable pool size, or a shorter deadline for the reconnect handshake alone -- a healthy reconnect is a single round trip, so 30s of idle tolerance is generous -- would both help, but both change behaviour, so I would rather ask than guess. Removing the exposure properly means not running recovery on a pool thread at all, which is a much larger change.

One question beyond this PR: is a PROTOCOL_VERSION bump plausible for some future release? Reconnect still proves nothing but knowledge of a client id, as #784 records. That is what let me drive the reproduction in the "End to end" section above from a short script, and it cannot be fixed compatibly.

[MisterTea]

Thanks for a great pr!