From b4dea975fc5245b75638b0e69e5c7192826bb823 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Tue, 28 Jul 2026 19:13:05 +0300 Subject: [PATCH 1/2] =?UTF-8?q?handshake:=20close=20accepted=20streams=20?= =?UTF-8?q?=E2=80=94=20port=20444=20leaked=20a=20goroutine=20per=20connect?= =?UTF-8?q?ion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Close appeared NOWHERE in handshake.go. Every stream accepted on port 444 was leaked, on both the timeout and the success path. The timeout branch is the worse half: ch := make(chan readResult, 1) go func() { buf := make([]byte, maxMsgSize) // 64 KiB n, err := stream.Read(buf) ... }() select { case res := <-ch: ... case <-time.After(handshakeRecvTimeout): // 10s slog.Warn("handshake timeout waiting for message", ...) } On timeout handleConnection returned without closing. The child goroutine stayed parked in stream.Read forever — nothing could ever unblock it, because the only thing that would is a Close and there wasn't one. Its stack retained the 64 KiB buffer, the stream, and the underlying daemon Connection with its RecvBuf/SendBuf channels. Port 444 accepts from any peer. A peer that connects and sends nothing leaked roughly 64 KiB plus a goroutine stack plus a full Connection every 10s, with no cap and no recovery — the same shape as the dead idle-timeout that OOM-killed the service fleet, reached through a different door. Closing is what frees the parked reader: Close reaches CloseConnection, which calls CloseRecvBuf, so the blocked receive returns !ok and the goroutine exits with io.EOF. The handshakeCloseDelay pause is why that constant exists. It was declared at line 80 for exactly this purpose ("delay before closing after send to let data flush") and had NO remaining reference anywhere in the package — stranded when the close path was removed at some point. processMessage writes its reply synchronously, so closing without the pause would truncate an in-flight response. Each handleConnection already runs in its own goroutine (see the accept loop), so the pause delays nothing but its own teardown. Pre-existing unrelated failure TestSendRequestDirectFailsRelaySucceeds fails identically with and without this change; verified against a clean stash. Found during a codebase-wide sweep for safety mechanisms that silently fail to engage. Co-Authored-By: Claude Opus 5 --- handshake.go | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/handshake.go b/handshake.go index 831646b..bd80469 100644 --- a/handshake.go +++ b/handshake.go @@ -528,6 +528,38 @@ func (hm *Manager) handleConnection(stream coreapi.Stream) { slog.Error("handshake: recovered from panic in connection handler", "panic", r) } }() + + // Close the stream on every exit path. Nothing closed it before — Close + // appeared nowhere in this file — so each accepted connection leaked: + // + // * on the timeout branch below, the reader goroutine stays parked in + // stream.Read forever, retaining its 64 KiB buf, the stream, and the + // underlying daemon Connection (RecvBuf/SendBuf channels). Only a + // Close can unblock it, and there wasn't one. + // * on the success path the daemon-side Connection was never released + // either. + // + // Port 444 accepts from any peer, so a peer that connects and sends + // nothing leaked ~64 KiB plus a goroutine stack and a full Connection + // every handshakeRecvTimeout, uncapped. That is the same shape as the + // dead idle-timeout that OOM-killed the service fleet. + // + // Closing here is what frees the parked reader: Close ends at + // CloseConnection, which closes RecvBuf, so the blocked receive returns + // !ok and the goroutine exits with io.EOF. + // + // The handshakeCloseDelay pause is why that constant exists — it was + // declared for exactly this ("delay before closing after send to let + // data flush") and had no remaining reference, stranded when the close + // path was removed. processMessage writes its reply synchronously, so + // without the pause we would close from under an in-flight response. + // Each handleConnection already runs in its own goroutine (see the + // accept loop), so this delays nothing but its own teardown. + defer func() { + time.Sleep(handshakeCloseDelay) + _ = stream.Close() + }() + const maxMsgSize = 64 * 1024 type readResult struct { From 240dff15f46833845258e5f5d5d16452c4c2efb2 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Wed, 29 Jul 2026 14:46:11 +0300 Subject: [PATCH 2/2] test: make the slowStream double's Close idempotent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closing accepted streams (previous commit) made TestHandleConnection_ TimeoutBranch panic: the test double implemented func (s *slowStream) Close() error { close(s.done); return nil } so the test's own `defer stream.Close()` became a SECOND close once handleConnection started closing the stream itself, and close of a closed channel panics. The real transport is idempotent — *Connection guards teardown with a sync.Once (CloseRecvBuf → c.CloseOnce.Do), so double Close is safe in production. The double simply did not model that, so it was less faithful than the thing it stood in for. Fixed with sync.Once to match. The alternative — dropping the test's own defer — would have been wrong: it would leave the test unable to fail if handleConnection ever stopped closing the stream again, which is precisely the regression this guards. Pre-existing unrelated failure TestSendRequestDirectFailsRelaySucceeds fails identically on origin/main; verified by running the suite on a clean checkout. Co-Authored-By: Claude Opus 5 --- zz_ceiling_more_test.go | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/zz_ceiling_more_test.go b/zz_ceiling_more_test.go index bdce072..5368cbf 100644 --- a/zz_ceiling_more_test.go +++ b/zz_ceiling_more_test.go @@ -4,6 +4,7 @@ package handshake import ( "io" + "sync" "testing" "time" @@ -194,7 +195,8 @@ func TestTrustedPeers_ReturnsAllEntries(t *testing.T) { // ----------------------------------------------------------------------- type slowStream struct { - done chan struct{} + done chan struct{} + closeOnce sync.Once } func newSlowStream() *slowStream { return &slowStream{done: make(chan struct{})} } @@ -203,11 +205,24 @@ func (s *slowStream) Read(p []byte) (int, error) { <-s.done return 0, io.EOF } -func (s *slowStream) Write(p []byte) (int, error) { return len(p), nil } -func (s *slowStream) Close() error { close(s.done); return nil } -func (s *slowStream) LocalAddr() coreapi.Addr { return coreapi.Addr{} } +func (s *slowStream) Write(p []byte) (int, error) { return len(p), nil } + +// Close is idempotent, matching the real transport. The production +// *Connection guards teardown with a sync.Once (CloseRecvBuf → +// c.CloseOnce.Do), so calling Close twice is safe there. This double +// stood in for it with a bare close(s.done), which panics on the second +// call — so once handleConnection started closing the stream itself (as +// it must, or every accepted connection leaks a goroutine), the test's +// own `defer stream.Close()` became a second close and paniced. +// +// The bug was in the double's fidelity, not in closing twice. +func (s *slowStream) Close() error { + s.closeOnce.Do(func() { close(s.done) }) + return nil +} +func (s *slowStream) LocalAddr() coreapi.Addr { return coreapi.Addr{} } func (s *slowStream) LocalPort() uint16 { return 0 } -func (s *slowStream) RemoteAddr() coreapi.Addr { return coreapi.Addr{} } +func (s *slowStream) RemoteAddr() coreapi.Addr { return coreapi.Addr{} } func (s *slowStream) RemotePort() uint16 { return 0 } func (s *slowStream) SetDeadline(time.Time) error { return nil } func (s *slowStream) SetReadDeadline(time.Time) error { return nil } @@ -317,4 +332,3 @@ func TestSaveTrust_MkdirAllErrorIsNonFatal(t *testing.T) { hm.mu.Unlock() hm.saveTrust() // logs error, must not panic } -