felhom-agent/internal/pbs/client_leak_test.go

package pbs

import (
	"context"
	"net"
	"net/http"
	"net/http/httptest"
	"strings"
	"sync"
	"testing"
	"time"

	"gitea.dooplex.hu/admin/felhom-agent/internal/httpx"
)

// connCounter is the SERVER-side observer. It counts what the server actually holds, which is the
// only thing that answers the question this file exists for: a client that believes it closed a
// connection, and a server still holding the socket, is precisely the R-344 shape. Asserting on
// anything client-side would be asserting the mechanism instead of the consequence.
type connCounter struct {
	mu    sync.Mutex
	open  int
	total int // every connection ever accepted — how many times the client DIALLED
}

func (c *connCounter) hook(_ net.Conn, s http.ConnState) {
	c.mu.Lock()
	defer c.mu.Unlock()
	switch s {
	case http.StateNew:
		c.open++
		c.total++
	case http.StateClosed, http.StateHijacked:
		c.open--
	}
}

func (c *connCounter) counts() (open, total int) {
	c.mu.Lock()
	defer c.mu.Unlock()
	return c.open, c.total
}

// waitForOpen polls until the server holds want connections, or fails naming what it still holds.
func (c *connCounter) waitForOpen(t *testing.T, want int, within time.Duration, what string) {
	t.Helper()
	deadline := time.Now().Add(within)
	for {
		open, total := c.counts()
		if open == want {
			return
		}
		if time.Now().After(deadline) {
			t.Fatalf("%s: after %s the server still holds %d open connection(s), want %d (%d dialled in total)",
				what, within, open, want, total)
		}
		time.Sleep(5 * time.Millisecond)
	}
}

// newCountingPBSServer is newPBSTestServer plus a ConnState hook. Kept separate rather than
// changing the shared helper, so the existing tests are untouched by this file.
func newCountingPBSServer(t *testing.T, fn http.HandlerFunc) (*httptest.Server, string, *connCounter) {
	t.Helper()
	cc := &connCounter{}
	ts := httptest.NewUnstartedServer(fn)
	ts.Config.ConnState = cc.hook
	ts.StartTLS()
	t.Cleanup(ts.Close)
	return ts, fingerprintOf(ts), cc
}

// TestAbandonedClientsReleaseTheirConnections is Scenario A, and it is the load-bearing test for
// R-344.
//
// It models what pbsTargetsFromPVE actually does — build a client, use it once, drop it on the
// floor without closing anything — and asserts the CONSEQUENCE on the server: the connections go
// away. Before the fix every one of these stayed established forever on both sides; 388 of them
// accumulated on ep0 in 46 hours.
//
// Deliberately NOT asserted: that err == nil, or that IdleConnTimeout holds some value. Both were
// true of the leaking code.
func TestAbandonedClientsReleaseTheirConnections(t *testing.T) {
	ts, fp, cc := newCountingPBSServer(t, func(w http.ResponseWriter, _ *http.Request) {
		w.Write([]byte(`{"data":[]}`))
	})
	host, port := hostPort(t, ts.URL)

	const cycles = 5
	for i := 0; i < cycles; i++ {
		// One fresh client per "cycle", exactly as pbsTargetsFromPVE builds one per collect.
		c, err := NewClient(Config{
			Server: host, Port: port, Fingerprint: fp, TokenID: "u@pbs!t", Secret: "s",
			IdleConnTimeout: 50 * time.Millisecond, // production uses the 90s default
		})
		if err != nil {
			t.Fatal(err)
		}
		if _, err := c.Snapshots(context.Background(), "ds"); err != nil {
			t.Fatalf("cycle %d: %v", i, err)
		}
		_ = c // dropped here — nothing closes it, nothing can
	}

	if _, total := cc.counts(); total != cycles {
		t.Fatalf("setup is not modelling the leak: want %d separate dials (one per abandoned client), got %d", cycles, total)
	}
	cc.waitForOpen(t, 0, 5*time.Second, "abandoned pbs.Clients")
}

// TestAbandonedClientsReleaseTheirConnections_ProductionDefaultIsUsable pins the value that ships.
//
// The field being settable is exactly how it could silently become zero again — and zero used to
// mean "never expire". This asserts the production path (Config leaving it unset) lands on the
// standard-library default, so the leak cannot return through an unset field.
func TestPBSClient_UnsetIdleTimeoutUsesTheDefault(t *testing.T) {
	for _, tc := range []struct {
		name string
		cfg  time.Duration
		want time.Duration
	}{
		{"unset — the production path", 0, httpx.DefaultIdleConnTimeout},
		{"explicit zero is NOT no-timeout", 0, httpx.DefaultIdleConnTimeout},
		{"negative is NOT no-timeout", -time.Second, httpx.DefaultIdleConnTimeout},
		{"an explicit value is honoured", 3 * time.Second, 3 * time.Second},
	} {
		t.Run(tc.name, func(t *testing.T) {
			c, err := NewClient(Config{
				Server: "pbs.example", Fingerprint: strings.Repeat("ab", 32),
				TokenID: "u@pbs!t", Secret: "s", IdleConnTimeout: tc.cfg,
			})
			if err != nil {
				t.Fatal(err)
			}
			tr, ok := c.http.Transport.(*http.Transport)
			if !ok {
				t.Fatalf("transport is %T, not *http.Transport — the httpx wiring was replaced", c.http.Transport)
			}
			if tr.IdleConnTimeout != tc.want {
				t.Fatalf("IdleConnTimeout = %v, want %v (zero would mean connections are retained FOREVER — that is R-344)", tr.IdleConnTimeout, tc.want)
			}
		})
	}
}

// TestPBSClient_KeepAliveStillReuses is Scenario C, and it is the guard against a "fix" that is
// worse than the bug.
//
// Disabling keep-alive entirely would also make the leak go away — by dialling a fresh connection
// for every single request, which on a box polling ~40,000 times a day is strictly worse than what
// we started with. The fix must retire IDLE connections without stopping reuse.
func TestPBSClient_KeepAliveStillReuses(t *testing.T) {
	ts, fp, cc := newCountingPBSServer(t, func(w http.ResponseWriter, _ *http.Request) {
		w.Write([]byte(`{"data":[]}`))
	})
	host, port := hostPort(t, ts.URL)

	c, err := NewClient(Config{
		Server: host, Port: port, Fingerprint: fp, TokenID: "u@pbs!t", Secret: "s",
		IdleConnTimeout: 30 * time.Second, // long enough that reuse is what is being measured
	})
	if err != nil {
		t.Fatal(err)
	}
	for i := 0; i < 3; i++ {
		if _, err := c.Snapshots(context.Background(), "ds"); err != nil {
			t.Fatalf("request %d: %v", i, err)
		}
	}
	if _, total := cc.counts(); total != 1 {
		t.Fatalf("one client made 3 sequential requests over %d connection(s), want 1 — keep-alive reuse is broken, which would make the poll load WORSE than the leak", total)
	}
}