From ca0b6d0c4a3520a87037ba72bb1f1ee10eb504dc Mon Sep 17 00:00:00 2001 From: Bitflash Date: Sat, 1 Aug 2026 21:27:29 -0300 Subject: diag: count the socket leak the way that is true, and say what it found (#100) Windows lists a wildcard-bound socket twice in MSFT_NetTCPConnection, so counting Bound rows makes the number climb whenever the node talks to more peers -- 35 of a node's 38 Bound rows shared a local port with one of its own established connections. Counting orphans instead: a 1.2.10 node at 26h held 1262 and was still taking on 30-90 an hour; nodes on 1.2.11 and 1.2.12 held 3, transient. The leak was real and 1.2.11 ended it. Adds a live-unconn column (getpeername on every tagged socket, reports 1: the listener) and rewrites the notes that said otherwise. --- src/net.cpp | 38 +++++++++++++++++------------ src/sockcount.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 83 insertions(+), 29 deletions(-) (limited to 'src') diff --git a/src/net.cpp b/src/net.cpp index 4fe5f15..b56f9f5 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -64,30 +64,38 @@ static string SockAccountingText() long long nOpened[SOCK_SITES], nClosed[SOCK_SITES], nFailed[SOCK_SITES], nUntagged = 0; BtfSockSnapshot(nOpened, nClosed, nFailed, &nUntagged); + long long nUnconn[SOCK_SITES]; + BtfSockLiveUnconnected(nUnconn); + string str = "\n sockets by where they were created\n"; - str += " site opened closed live close failed\n"; - long long nTotalOpened = 0, nTotalClosed = 0, nTotalFailed = 0; + str += " site opened closed live close failed live unconn\n"; + long long nTotalOpened = 0, nTotalClosed = 0, nTotalFailed = 0, nTotalUnconn = 0; for (int i = 0; i < SOCK_SITES; i++) { nTotalOpened += nOpened[i]; nTotalClosed += nClosed[i]; nTotalFailed += nFailed[i]; + nTotalUnconn += nUnconn[i]; if (nOpened[i] == 0) continue; - str += strprintf(" %-24s %8lld %8lld %8lld %13lld\n", pszSockSite[i], - nOpened[i], nClosed[i], nOpened[i] - nClosed[i], nFailed[i]); + str += strprintf(" %-24s %8lld %8lld %8lld %13lld %12lld\n", pszSockSite[i], + nOpened[i], nClosed[i], nOpened[i] - nClosed[i], nFailed[i], nUnconn[i]); } - str += strprintf(" %-24s %8s %8lld %8s %13s\n", "closed but never tagged", - "-", nUntagged, "-", "-"); - str += strprintf(" %-24s %8lld %8lld %8lld %13lld\n", "TOTAL", - nTotalOpened, nTotalClosed, nTotalOpened - nTotalClosed, nTotalFailed); - - // The live total above is what this program believes it is holding. Compare - // it against what the operating system attributes to the process: on - // Windows, `Get-NetTCPConnection | ? OwningProcess -eq `. A gap there - // is the leak, and the codes below say what the failed closes hit -- 10038 - // (WSAENOTSOCK) means the handle was already gone and this close was a - // second one, which is a different bug from a descriptor that stayed. + str += strprintf(" %-24s %8s %8lld %8s %13s %12s\n", "closed but never tagged", + "-", nUntagged, "-", "-", "-"); + str += strprintf(" %-24s %8lld %8lld %8lld %13lld %12lld\n", "TOTAL", + nTotalOpened, nTotalClosed, nTotalOpened - nTotalClosed, + nTotalFailed, nTotalUnconn); + + // The live total is what this program believes it is holding. Comparing it + // against `Get-NetTCPConnection | ? OwningProcess -eq ` needs the note + // in sockcount.h first: that table lists a wildcard-bound socket twice, so + // a raw row count always looks larger than this. Count orphans, not rows. + // + // "live unconn" is a getpeername on every tagged socket. Anything above 1 + // -- the listening socket -- means this program is holding a socket it + // never connected, which is the shape the leak hunt was looking for and + // never found. if (nTotalFailed > 0) { str += " close failures by code\n"; diff --git a/src/sockcount.h b/src/sockcount.h index a42abb9..ef99414 100644 --- a/src/sockcount.h +++ b/src/sockcount.h @@ -4,15 +4,35 @@ // Socket accounting -- which part of the program is holding the sockets. // // A node on a 32-core machine was found with 1502 sockets in Windows' "bound" -// state: created, never connected, never closed, each holding a distinct -// ephemeral port. That is 9% of the machine's whole dynamic port range gone in -// nineteen hours, and when the range runs out nothing on that machine can open -// an outbound connection -- not just this program. +// state, each apparently holding a distinct ephemeral port. Reading the code +// did not find it: every connect() failure path in this tree closes its socket. +// So count instead -- tag each socket where it is created, untag it where it is +// closed, report the difference per site. // -// Reading the code did not find it. Every connect() failure path in this tree -// closes its socket. So count instead: tag each socket where it is created, -// untag it where it is closed, and report the difference per site. Whichever -// site's live count tracks the leak is the answer. +// The counting is what showed the counting was wrong. Windows lists a socket +// bound to the wildcard address TWICE in MSFT_NetTCPConnection: once for the +// binding (0.0.0.0:port, state "Bound") and once for the connection itself +// (state "Established"). Measured: 35 of a node's 38 "Bound" rows had the same +// local port as one of its established connections. The bound count therefore +// rises because the node is talking to more peers, not because it is leaking, +// which is why a fix that plainly worked looked like it had done nothing. +// +// The honest metric is the ORPHAN: a "Bound" row whose port appears nowhere +// else in that process's table. +// +// $b = $c | ? State -eq 'Bound' +// $rest = ($c | ? State -ne 'Bound').LocalPort +// ($b | ? { $rest -notcontains $_.LocalPort }).Count +// +// By that metric, measured the same evening on three machines: a node on 1.2.10 +// with 26h of uptime had 1262 orphans and was still taking on 30-90 an hour; a +// node on 1.2.11 with 4h40m had 3; a node on 1.2.12 had 3, all from its first +// half minute. At the 1.2.10 rate the 1.2.11 node would have had ~230. +// +// So the leak was real and 1.2.11 ended it -- most likely the 60-second reaper +// for undeliverable nodes, which was written for a different problem. The +// counters below stay: they are how the question got answered, and they are how +// it would be answered again. // // Header-only on purpose. btfrv.cpp is compiled a second time, on its own, into // the standalone rendezvous relay -- a separate tree that does not link net.cpp. @@ -53,12 +73,11 @@ struct BtfSockAccount long long nOpened[SOCK_SITES]; long long nClosed[SOCK_SITES]; long long nCloseFailed[SOCK_SITES]; - // Why the close failed, counted per code per site. Measured on a node with - // 3h51m of uptime: the program believed it held 58 sockets and Windows - // attributed 112 to the process, 47 of them bound to an ephemeral port and - // never connected -- against 55 closes that had returned an error. The - // shapes match, and the error code is the piece that says whether the - // descriptor is still there or was already gone. + // Why the close failed, counted per code per site. Kept because nothing in + // this codebase had ever looked at what closesocket returns, and a close + // that fails leaves the descriptor and its port in place. It is not the + // leak -- see the note at the top of this file -- but it is the one number + // that would say so if it ever became the leak. std::map mapCloseErr[SOCK_SITES]; long long nClosedUntagged; BtfSockAccount() : nClosedUntagged(0) @@ -145,6 +164,33 @@ inline void BtfSockSnapshot(long long* pOpened, long long* pClosed, long long* p *pUntagged = a.nClosedUntagged; } +// Of the sockets still tagged live, how many have no peer on the other end. +// +// This is the check that ended the socket hunt, by answering it with a zero. +// The suspicion was that some site created sockets, never connected them and +// never let go. Run against a live node it reports 1 -- the listening socket, +// which has no peer by definition. Every other tagged socket is connected. +// +// See the note at the top of this file for what the pile actually was. +inline void BtfSockLiveUnconnected(long long* pUnconnected) +{ + for (int i = 0; i < SOCK_SITES; i++) + pUnconnected[i] = 0; + BtfSockAccount& a = BtfSockAccounting(); + std::lock_guard lock(a.mtx); + for (std::map::const_iterator mi = a.mapSite.begin(); mi != a.mapSite.end(); ++mi) + { + struct sockaddr_storage ss; +#ifdef _WIN32 + int len = sizeof(ss); +#else + socklen_t len = sizeof(ss); +#endif + if (getpeername(mi->first, (struct sockaddr*)&ss, &len) != 0) + pUnconnected[mi->second]++; + } +} + // The close-failure codes for one site, newest counts included. Separate call // because the table above is fixed-width and this is a list of unknown length. inline void BtfSockCloseErrors(int nSite, std::map& mapOut) -- cgit v1.2.3