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) } }