diff --git a/CHANGELOG.md b/CHANGELOG.md index 607a1b4..d53f547 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,30 @@ ## Changelog +### v0.74.0 — fix the controller→agent connection leak (per-call agentapi client) (2026-06-22) + +**Bugfix — agent local-API socket leak that took down the whole agent-backed feature set after ~5 days.** +`Server.agentClient()` built a fresh `agentapi.Client` (hence a fresh bare `http.Transport` with +`IdleConnTimeout:0`) on **every** call and discarded it without closing idle connections. The agent's +keep-alive left one idle ESTABLISHED socket per call to `192.168.0.162:8443`; these accumulated +(~5.8k/day, measured 206 in 47 min) until the ephemeral source-port range for that tuple exhausted → +`connect: cannot assign requested address` (EADDRNOTAVAIL), killing storage UI, host-metrics, and +whole-guest backup. (`:8006`/pveproxy was immune — the controller never dials it.) Diagnosis: +`felhom.eu/documentation/tests/unattended-test-campaign-2026-06-22-8443-diagnosis.md`. + +- `internal/web/server.go` — `Server` gains `agentCli *agentapi.Client` + `agentCliErr error` + + `agentCliOnce sync.Once` (and the `agentapi` import). +- `internal/web/agent_disk_handlers.go` — `agentClient()` now memoizes the build via `agentCliOnce` + and **reuses one shared client** (cfg.LocalAPI is static per process — a config-apply self-restarts). + The empty-endpoint "not configured" guard stays OUTSIDE the Once. All 19 call sites unchanged. +- `internal/agentapi/client.go` — `New` Transport hardened: `MaxIdleConns:4`, `MaxIdleConnsPerHost:2`, + `IdleConnTimeout:90s` (was a bare Transport, `IdleConnTimeout:0`). Added optional `Client.Close()` + (CloseIdleConnections) hygiene helper. +- Tests: `TestAgentClient_ReusesSameInstance` (+ `TestAgentClient_UnconfiguredErrors`) and + `TestNew_TransportIdlePoolBounded` — both red-proofed against the pre-fix code. +- Agent, its bridge-IP bind, and firewall rules were **not** touched (controller-only fix). + Separate open item: the defense-in-depth host firewall rule scoping `:8443` to the guest bridge + subnet is still absent (pve-firewall disabled) — to be closed independently. + ### v0.73.0 — DR recipe: emit the secret-free customer+apps half in the hub report (2026-06-16) **DR recipe slice (controller half).** Additive `dr_recipe` section on the controller's hub report — the diff --git a/controller/internal/agentapi/client.go b/controller/internal/agentapi/client.go index e8111e7..bd70215 100644 --- a/controller/internal/agentapi/client.go +++ b/controller/internal/agentapi/client.go @@ -84,12 +84,26 @@ func New(endpoint, token, fingerprintHex string) (*Client, error) { baseURL: "https://" + endpoint, token: token, hc: &http.Client{ - Timeout: 15 * time.Second, - Transport: &http.Transport{TLSClientConfig: tlsCfg}, + Timeout: 15 * time.Second, + // Bound + expire the idle-conn pool. With the controller reusing one Client (so the pool + // stays ~2), IdleConnTimeout also lets idle conns to a RESTARTED agent drain instead of + // lingering as stale ESTABLISHED entries, and caps any future per-call misuse. (The earlier + // bare Transport had IdleConnTimeout:0 = idle keep-alives never expire → the leak.) + Transport: &http.Transport{ + TLSClientConfig: tlsCfg, + MaxIdleConns: 4, + MaxIdleConnsPerHost: 2, + IdleConnTimeout: 90 * time.Second, + }, }, }, nil } +// Close releases the client's idle keep-alive connections. Optional hygiene for any caller that builds +// a short-lived client; the controller reuses one long-lived client, so it relies on the bounded, +// expiring idle pool (above) rather than calling this. +func (c *Client) Close() { c.hc.CloseIdleConnections() } + // Storage calls GET /storage and returns this guest's mounts (connectivity + placement view). func (c *Client) Storage(ctx context.Context) (StorageResponse, error) { var out StorageResponse diff --git a/controller/internal/agentapi/client_transport_test.go b/controller/internal/agentapi/client_transport_test.go new file mode 100644 index 0000000..23fb4f6 --- /dev/null +++ b/controller/internal/agentapi/client_transport_test.go @@ -0,0 +1,30 @@ +package agentapi + +import ( + "net/http" + "strings" + "testing" +) + +// T2: New must build a Transport with a BOUNDED, EXPIRING idle-conn pool. The earlier bare +// &http.Transport{TLSClientConfig:…} had IdleConnTimeout==0 (idle keep-alives never expire) — the +// leak. White-box (package agentapi) so we can read the unexported hc. +// +// Companion red-proof: the bare Transport gives IdleConnTimeout==0 → this test fails (demonstrated, +// then reverted, during implementation). +func TestNew_TransportIdlePoolBounded(t *testing.T) { + c, err := New("127.0.0.1:8443", "tok", strings.Repeat("a", 64)) + if err != nil { + t.Fatalf("New: %v", err) + } + tr, ok := c.hc.Transport.(*http.Transport) + if !ok { + t.Fatalf("unexpected transport type %T", c.hc.Transport) + } + if tr.IdleConnTimeout <= 0 { + t.Fatalf("IdleConnTimeout must be > 0 (idle keep-alives must expire); got %v", tr.IdleConnTimeout) + } + if tr.MaxIdleConnsPerHost <= 0 { + t.Fatalf("MaxIdleConnsPerHost must be > 0 (bounded pool); got %d", tr.MaxIdleConnsPerHost) + } +} diff --git a/controller/internal/web/agent_client_reuse_test.go b/controller/internal/web/agent_client_reuse_test.go new file mode 100644 index 0000000..3bd2620 --- /dev/null +++ b/controller/internal/web/agent_client_reuse_test.go @@ -0,0 +1,47 @@ +package web + +import ( + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/config" +) + +// T1: agentClient() must return the SAME *agentapi.Client across calls — one Client ⇒ one pooled +// http.Transport ⇒ the per-call Transport leak (idle ESTABLISHED sockets to the agent's :8443 that +// exhausted the ephemeral port range after ~5 days) is structurally impossible. cfg.LocalAPI is +// valid-but-non-network (agentapi.New builds the struct without dialing), so this is hermetic. +// +// Companion red-proof: reverting agentClient() to `return agentapi.New(...)` makes the two pointers +// differ → this test fails (demonstrated, then reverted, during implementation). +func TestAgentClient_ReusesSameInstance(t *testing.T) { + s := &Server{cfg: &config.Config{LocalAPI: config.LocalAPIConfig{ + Endpoint: "127.0.0.1:8443", + Token: "tok", + Fingerprint: strings.Repeat("a", 64), // syntactically valid 64-hex; no network touched + }}} + + c1, err := s.agentClient() + if err != nil { + t.Fatalf("agentClient() #1: %v", err) + } + c2, err := s.agentClient() + if err != nil { + t.Fatalf("agentClient() #2: %v", err) + } + if c1 == nil || c2 == nil { + t.Fatal("agentClient() returned a nil client") + } + if c1 != c2 { + t.Fatalf("agentClient() must return the SAME instance (leak fix); got %p then %p", c1, c2) + } +} + +// T1b: the "not configured" semantics are preserved — an empty endpoint still errors, OUTSIDE the +// memoizing Once (so an unprovisioned guest never caches a client). +func TestAgentClient_UnconfiguredErrors(t *testing.T) { + s := &Server{cfg: &config.Config{}} // LocalAPI.Endpoint == "" + if _, err := s.agentClient(); err == nil { + t.Fatal("expected an 'agent not configured' error when Endpoint is empty") + } +} diff --git a/controller/internal/web/agent_disk_handlers.go b/controller/internal/web/agent_disk_handlers.go index 419892e..b2f0133 100644 --- a/controller/internal/web/agent_disk_handlers.go +++ b/controller/internal/web/agent_disk_handlers.go @@ -38,13 +38,25 @@ func (s *Server) ServeDiskAPI(w http.ResponseWriter, r *http.Request) { } } -// agentClient builds a pinned client for the host agent's per-guest local API. +// agentClient returns the shared pinned client for the host agent's per-guest local API. // Returns a clear error if the local API is not configured (unprovisioned guest). +// +// The client is built ONCE and reused (cfg.LocalAPI is static per process — a config-apply triggers a +// graceful self-restart). Reusing one *agentapi.Client (one pooled http.Transport) eliminates the +// per-call Transport leak that accumulated idle ESTABLISHED sockets to the agent's :8443 and exhausted +// the ephemeral source-port range after ~5 days of uptime. *agentapi.Client/*http.Client are safe for +// concurrent use, so the (19) callers need no extra locking. The empty-endpoint guard stays OUTSIDE the +// Once so it is re-checked each call (defensive; the endpoint is static); only a real build result +// (client, or a construction error like a bad fingerprint — neither changes without a restart) is memoized. func (s *Server) agentClient() (*agentapi.Client, error) { if s.cfg.LocalAPI.Endpoint == "" { return nil, errors.New("agent not configured") } - return agentapi.New(s.cfg.LocalAPI.Endpoint, s.cfg.LocalAPI.Token, s.cfg.LocalAPI.Fingerprint) + s.agentCliOnce.Do(func() { + s.agentCli, s.agentCliErr = agentapi.New( + s.cfg.LocalAPI.Endpoint, s.cfg.LocalAPI.Token, s.cfg.LocalAPI.Fingerprint) + }) + return s.agentCli, s.agentCliErr } // writeDiskJSON writes the standard {ok,data,error} envelope used by the disk API. diff --git a/controller/internal/web/server.go b/controller/internal/web/server.go index 2b00c27..3365d59 100644 --- a/controller/internal/web/server.go +++ b/controller/internal/web/server.go @@ -13,6 +13,7 @@ import ( "sync/atomic" "time" + "gitea.dooplex.hu/admin/felhom-controller/internal/agentapi" "gitea.dooplex.hu/admin/felhom-controller/internal/appexport" "gitea.dooplex.hu/admin/felhom-controller/internal/assets" "gitea.dooplex.hu/admin/felhom-controller/internal/backup" @@ -51,6 +52,14 @@ type Server struct { // Guard for FileBrowser sync — prevents concurrent file writes (H5 fix) fileBrowserMu sync.Mutex + // Shared agent local-API client (built once, reused). cfg.LocalAPI is static per process (a + // config-apply triggers a graceful self-restart), so the client is memoized via agentCliOnce — + // this kills the per-call http.Transport leak that exhausted the controller's ephemeral ports to + // the agent's :8443 after ~5 days of uptime (see agentClient()). + agentCli *agentapi.Client + agentCliErr error + agentCliOnce sync.Once + // Hub push status callback — set via SetHubPushStatus for monitoring page hubPushStatusFn func() HubPushStatusData