R-344: restore the idle-connection timeout our hand-rolled transports lost
gates / gates (push) Successful in 7s
gates / gates (push) Successful in 7s
Every client here pins TLS, so none can use http.DefaultTransport and each hand-rolls its own. A composite literal takes IdleConnTimeout ZERO, which means retain idle connections forever -- not "use a sane default". pbsTargetsFromPVE builds a fresh pbs.Client every cycle and drops the previous one, and an abandoned http.Transport does not close its connections. One stranded socket per cycle, on both sides, forever. Measured: 388 established connections on ep0 over 46 h, 194 per box, zero closed in a 31-minute window. pvestatd and proxmox-backup-client made 162,404 requests in the same window and leaked none. New leaf package internal/httpx owns the default (90s, http.DefaultTransport's own value) and NewTransport, which returns a FRESH transport per call and treats <=0 as "use the default", never "no timeout". pbs.Config gains IdleConnTimeout for tests only. hub and proxmox carried the same missing default and are corrected here for consistency. Neither contributed to the ep0 leak -- both are built once per process and neither talks to ep0:8007. Tests count connections SERVER-side and model the abandonment, so they pin the consequence rather than the field. Two red-proofs, both seen failing: removing the timeout -> "still holds 5 open connection(s), want 0"; DisableKeepAlives -> "3 sequential requests over 3 connection(s), want 1" (the leak test PASSES under that one -- it is the worse-fix guard that catches it). Not released: hand-installed on demo-hp only so demo-felhom stays the control. CHANGELOG heading stays UNRELEASED until the publish is authorised.
This commit is contained in:
@@ -0,0 +1,59 @@
|
||||
// Package httpx holds the one HTTP-transport default this repo may not lose.
|
||||
//
|
||||
// Every client here pins TLS — PBS and PVE by leaf-cert SHA-256, the hub by an optional CA file —
|
||||
// so none of them can use http.DefaultTransport and each hand-rolls its own. Hand-rolling silently
|
||||
// discards DefaultTransport's settings, and one of them is load-bearing:
|
||||
//
|
||||
// Transport: &http.Transport{TLSClientConfig: tlsCfg} // IdleConnTimeout == 0 == NO timeout
|
||||
//
|
||||
// A zero IdleConnTimeout means idle keep-alive connections are retained FOREVER, not "use a sane
|
||||
// default". Combined with a client that is rebuilt on a schedule and dropped (pbsTargetsFromPVE
|
||||
// builds a fresh pbs.Client per cycle), every cycle strands one connection that nothing will ever
|
||||
// close: the abandoned Transport becomes unreachable but its persistConn read-loop goroutine keeps
|
||||
// the socket alive, and an unreachable Transport does not close its connections.
|
||||
//
|
||||
// Measured cost, live: 388 established connections accumulated on ep0's PBS proxy between
|
||||
// 2026-08-18 09:51:22Z and 2026-08-20 08:02:13Z — 194 from each of the two boxes, held open on BOTH
|
||||
// sides, one per agent poll cycle, on a proxy whose descriptor ceiling is 65536. See R-344 and
|
||||
// felhom.eu/documentation/audits/SPIKE-ep0-established-connections-2026-08-20.md.
|
||||
package httpx
|
||||
|
||||
import (
|
||||
"crypto/tls"
|
||||
"net/http"
|
||||
"time"
|
||||
)
|
||||
|
||||
// DefaultIdleConnTimeout is how long an idle keep-alive connection is retained before it is closed.
|
||||
//
|
||||
// It is 90s because that is http.DefaultTransport's own value: the fix for R-344 restores a
|
||||
// standard-library default rather than inventing a number, so there is nothing here to tune and
|
||||
// nothing to justify. It is comfortably shorter than every cadence that drives these clients (the
|
||||
// 15-minute live-snapshot collect and the 6-hour verify loop), so a connection abandoned by one
|
||||
// cycle is closed long before the next.
|
||||
const DefaultIdleConnTimeout = 90 * time.Second
|
||||
|
||||
// NewTransport builds a FRESH *http.Transport pinned to tlsCfg, with the idle-connection timeout
|
||||
// applied.
|
||||
//
|
||||
// Fresh, never shared: each caller pins a different endpoint, and a shared transport would pool
|
||||
// connections across differently pinned servers. Reusing http.DefaultTransport for the same reason
|
||||
// is not an option — it would drop the pin entirely.
|
||||
//
|
||||
// idleConnTimeout <= 0 means USE THE DEFAULT. It deliberately does not mean "no timeout": no-timeout
|
||||
// is the bug this package exists to prevent, and an unset field must never be able to reintroduce
|
||||
// it. Callers pass their configured value straight through; only tests pass a short one.
|
||||
//
|
||||
// Only IdleConnTimeout is set. The other DefaultTransport settings this transport also lacks
|
||||
// (MaxIdleConns, TLSHandshakeTimeout, ExpectContinueTimeout) are deliberately left alone: none of
|
||||
// them accumulates anything, every client bounds its whole request with http.Client.Timeout, and
|
||||
// widening the change would have made the R-344 measurement unattributable.
|
||||
func NewTransport(tlsCfg *tls.Config, idleConnTimeout time.Duration) *http.Transport {
|
||||
if idleConnTimeout <= 0 {
|
||||
idleConnTimeout = DefaultIdleConnTimeout
|
||||
}
|
||||
return &http.Transport{
|
||||
TLSClientConfig: tlsCfg,
|
||||
IdleConnTimeout: idleConnTimeout,
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,74 @@
|
||||
package httpx
|
||||
|
||||
import (
|
||||
"crypto/tls"
|
||||
"net/http"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
// TestNewTransport_ZeroMeansDefaultNeverForever is the whole point of this package.
|
||||
//
|
||||
// http.Transport's zero IdleConnTimeout means "retain idle connections FOREVER". Any code path that
|
||||
// can reach that zero reintroduces R-344, so an unset, zero or negative value must all land on the
|
||||
// default. If someone later "simplifies" NewTransport by passing the argument straight through,
|
||||
// this fails.
|
||||
func TestNewTransport_ZeroMeansDefaultNeverForever(t *testing.T) {
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
in time.Duration
|
||||
want time.Duration
|
||||
}{
|
||||
{"zero", 0, DefaultIdleConnTimeout},
|
||||
{"negative", -time.Hour, DefaultIdleConnTimeout},
|
||||
{"explicit short value (tests)", 50 * time.Millisecond, 50 * time.Millisecond},
|
||||
{"explicit long value", time.Hour, time.Hour},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
got := NewTransport(&tls.Config{MinVersion: tls.VersionTLS12}, tc.in).IdleConnTimeout
|
||||
if got != tc.want {
|
||||
t.Fatalf("IdleConnTimeout = %v, want %v", got, tc.want)
|
||||
}
|
||||
if got == 0 {
|
||||
t.Fatal("IdleConnTimeout is 0 — that is 'never expire', which is the R-344 defect itself")
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestDefaultIdleConnTimeout_MatchesTheStandardLibrary pins the number to its justification.
|
||||
//
|
||||
// 90s is not a tuned value; it is what http.DefaultTransport uses. Reading it off the standard
|
||||
// library rather than hardcoding 90 means the constant cannot drift away from the reason given for
|
||||
// it in the package doc.
|
||||
func TestDefaultIdleConnTimeout_MatchesTheStandardLibrary(t *testing.T) {
|
||||
std, ok := http.DefaultTransport.(*http.Transport)
|
||||
if !ok {
|
||||
t.Skip("http.DefaultTransport is not an *http.Transport in this Go build")
|
||||
}
|
||||
if DefaultIdleConnTimeout != std.IdleConnTimeout {
|
||||
t.Fatalf("DefaultIdleConnTimeout = %v but http.DefaultTransport uses %v — the doc comment's justification no longer holds",
|
||||
DefaultIdleConnTimeout, std.IdleConnTimeout)
|
||||
}
|
||||
}
|
||||
|
||||
// TestNewTransport_IsFreshEveryCall guards the pooling property the pinning relies on.
|
||||
//
|
||||
// Each caller pins a DIFFERENT endpoint. A shared transport would pool connections across
|
||||
// differently pinned servers, so returning a package-level singleton would be a security change
|
||||
// dressed as a tidy-up.
|
||||
func TestNewTransport_IsFreshEveryCall(t *testing.T) {
|
||||
a := NewTransport(&tls.Config{MinVersion: tls.VersionTLS12}, 0)
|
||||
b := NewTransport(&tls.Config{MinVersion: tls.VersionTLS12}, 0)
|
||||
if a == b {
|
||||
t.Fatal("NewTransport returned the SAME transport twice — connections would be pooled across differently pinned endpoints")
|
||||
}
|
||||
}
|
||||
|
||||
// TestNewTransport_KeepsTheTLSConfig — the transport gains a field; it must lose nothing.
|
||||
func TestNewTransport_KeepsTheTLSConfig(t *testing.T) {
|
||||
cfg := &tls.Config{MinVersion: tls.VersionTLS12, InsecureSkipVerify: true} //nolint:gosec // test only
|
||||
if got := NewTransport(cfg, 0).TLSClientConfig; got != cfg {
|
||||
t.Fatalf("TLSClientConfig = %p, want the config passed in (%p) — the pin would be dropped", got, cfg)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user