Compare commits
3 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| 7569f34aeb | |||
| ede49b610d | |||
| f17ed11599 |
@@ -1,3 +1,79 @@
|
||||
## UNRELEASED — v0.130.0 candidate: the agent was the one leaking connections onto the off-site box (2026-08-20, R-344)
|
||||
|
||||
> **Deliberately not a release heading yet, and the `release-complete` gate is doing its job by
|
||||
> requiring that.** This build is **hand-installed on `demo-hp` only** so the fix can be proved
|
||||
> against `demo-felhom` as an untouched control. Publishing it — tag + Gitea package — would put it
|
||||
> on the control box through the self-update path and destroy the experiment. **When the operator
|
||||
> authorises the publish, this heading becomes `## v0.130.0` in the same commit as the tag and the
|
||||
> package**, and the gate then checks it for real. See R-347.
|
||||
|
||||
**What was measured, before anything was changed.** Between 2026-08-18 09:51:22Z and 2026-08-20
|
||||
08:02:13Z, ep0's PBS proxy accumulated **388 established connections** — 194 from each demo box, on a
|
||||
proxy whose descriptor ceiling is 65536 and whose runway at that rate was ~323 days. The connections
|
||||
were held open on **both** sides: ep0 showed 388 while the two boxes showed 194 + 194, at two separate
|
||||
instants, with the same source ports on each side, and **not one closed in a 31-minute window**.
|
||||
`ss -tnp` on the boxes named the holder: **`felhom-agent`**, 194 of 194 on each, one PID.
|
||||
|
||||
**`pvestatd` and `proxmox-backup-client` made 162,404 requests in that window and leaked zero.** They
|
||||
are 99.5% of the traffic to that endpoint and 0% of the leak. The agent made 811 requests — of which
|
||||
387 were `GET .../snapshots` — and leaked 388 sockets. One per call, within one.
|
||||
|
||||
**The defect, and it is two things compounding.**
|
||||
|
||||
- `internal/pbs/client.go` built its transport as a composite literal:
|
||||
`&http.Transport{TLSClientConfig: tlsCfg}`. That takes **`IdleConnTimeout` zero, which does not mean
|
||||
"use a sane default" — it means retain idle keep-alive connections FOREVER.**
|
||||
`http.DefaultTransport` sets 90s; hand-rolling the transport (which every client here must do,
|
||||
because they all pin TLS) silently discards it.
|
||||
- `pbsTargetsFromPVE` (`cmd/felhom-agent/main.go`) builds **a fresh `pbs.Client` every cycle**, as its
|
||||
own doc comment says, and drops the previous one. An abandoned `http.Transport` does **not** close
|
||||
its connections — it becomes unreachable while its `persistConn` read-loop goroutine keeps the
|
||||
socket alive. So each cycle stranded exactly one connection that nothing could ever close.
|
||||
|
||||
The cadences reconcile without fitting: a 900s live-snapshot collect (184.7 cycles in the window) plus
|
||||
a 6-hour verify loop (7.7) predicts 192.4 per box against **194 observed**.
|
||||
|
||||
**The fix is one field, restored to the standard library's own value.** New leaf package
|
||||
`internal/httpx` owns `DefaultIdleConnTimeout = 90 * time.Second` — 90s because that is what
|
||||
`http.DefaultTransport` uses, so there is nothing invented here to justify or tune — and
|
||||
`NewTransport(tlsCfg, idleConnTimeout)`, which returns a **fresh** transport (never shared: each caller
|
||||
pins a different endpoint) and treats a zero or negative timeout as **use the default, never "no
|
||||
timeout"**. `pbs.Config` gains an `IdleConnTimeout` field that production leaves unset; only tests set
|
||||
it, to avoid a 90-second wait.
|
||||
|
||||
**`internal/hub/client.go` and `internal/proxmox/client.go` carried the identical missing default and
|
||||
were corrected in the same pass — but neither contributed to the ep0 leak, and this entry must not be
|
||||
read as three leaks having been found.** Both are built **once per process**, so they held one idle
|
||||
connection for the life of the daemon rather than accumulating, and neither talks to ep0:8007.
|
||||
|
||||
**Tests, and what they deliberately do not assert.** `internal/pbs/client_leak_test.go` counts
|
||||
connections **server-side** and models what `pbsTargetsFromPVE` actually does — build a client, use it
|
||||
once, drop it on the floor — then asserts the connections go away. It does not assert `err == nil` and
|
||||
it does not assert that some field holds some value; both were true of the leaking code.
|
||||
|
||||
- **Red-proof 1 (the fix):** removing `IdleConnTimeout` from `NewTransport` fails the test with
|
||||
*"after 5s the server still holds 5 open connection(s), want 0 (5 dialled in total)"* — the count is
|
||||
in the message, so the failure cannot be mistaken for a timeout with another cause. Reverted.
|
||||
- **Red-proof 2 (the fix that would be worse than the bug):** setting `DisableKeepAlives: true` also
|
||||
makes the leak vanish — by dialling fresh for every request, which on a box polling ~40,000 times a
|
||||
day is strictly worse than what we started with. **The leak test PASSES under that mutation**;
|
||||
`TestPBSClient_KeepAliveStillReuses` is what catches it, failing with *"3 sequential requests over 3
|
||||
connection(s), want 1"*. Reverted.
|
||||
|
||||
**What this release does NOT do.** It does not reduce the poll rate (**R-336 stays open, but re-scoped
|
||||
— it was never the cause of this leak**), it does not refactor `pbsTargetsFromPVE` to cache or reuse
|
||||
clients (a one-line default restores the standard behaviour; a lifecycle refactor adds
|
||||
cache-invalidation questions for no measurable gain), and it adds no `CloseIdleConnections` call.
|
||||
|
||||
**One sentence in this entry was written before the deploy and was WRONG, and it is corrected here
|
||||
rather than quietly edited.** It read: *"does not clear the 388 descriptors already stuck on ep0 —
|
||||
those persist until that proxy restarts."* **Measured: they clear the moment the AGENT restarts.**
|
||||
Replacing the binary on `demo-hp` released exactly its 199 descriptors within one second
|
||||
(415 → 216 fd), and replacing it on `demo-felhom` released the remaining 203 (**220 → 17 fd in under
|
||||
two seconds**). **17 is precisely ep0's `t0` baseline** of 2026-08-18 09:51:22Z. ep0 was read-only
|
||||
throughout and its proxy PID never changed. The accumulated leak was never ep0's to hold on to — it
|
||||
was held on both sides, and closing either side ends it.
|
||||
|
||||
## v0.129.0 — a correct code for an earlier package stops being called wrong (2026-08-12, R-311)
|
||||
|
||||
**The measurement this fixes.** On 2026-08-12 a recovery code that provably opens a RETAINED package
|
||||
|
||||
@@ -1,63 +1,50 @@
|
||||
# REPORT — felhom-agent, 2026-08-09 (gates only)
|
||||
# REPORT — agent v0.129.0: a correct code for an earlier package (R-311, 2026-08-12)
|
||||
|
||||
**No release. No version bump. No binary published. `scripts/` only** — nothing that runs on a
|
||||
customer's machine changed, and the agent stays **v0.128.0** at `28ba8593b8`.
|
||||
## What changed and why
|
||||
|
||||
## What changed
|
||||
Yesterday's drill proved a retained escrow package **works** — unsealed with the old recovery code, it
|
||||
opened a set-aside store and restored planted files byte-identical — while this agent answered that
|
||||
same correct code with *"the recovery code did not open the sealed bundle"*. Nothing had ever tried
|
||||
the retained packages, so a correct-but-earlier code and a mistype were genuinely indistinguishable.
|
||||
|
||||
| file | why |
|
||||
|---|---|
|
||||
| `scripts/retention-policy.json` **(new)** | THE retention number, in one place, with its reasoning and its honesty about where the number came from |
|
||||
| `scripts/check-published-versions.py` | reads that number; bounds its assertion to the newest N; **prints what it stopped covering** |
|
||||
| `scripts/check-release-complete.py` **(new)** | asserts the CHANGELOG-head version is tagged, placed in this history, and published |
|
||||
| `scripts/agent_gates.py` | registers the new gate; legs 1–2 are offline so it runs in `--fast` too |
|
||||
- `internal/hub/client.go` — `FetchRetainedIdentityEscrow` → `GET /api/v1/hosts/<id>/escrow/retained`
|
||||
(hub ≥ v0.103.0). **A 404 is a clean "none"**, not a fault: an older hub must not turn into a failed
|
||||
recovery.
|
||||
- `internal/escrow/recover.go` — optional `FetchRetained`, `ErrCodeOpensRetained` +
|
||||
`RetainedOpenedError{SupersededAt, KeyFingerprint, Index, HasResticPassword}`. Consulted **only**
|
||||
after the current package refuses.
|
||||
- `internal/localapi/escrow_recover.go` — a **fifth** case on the R-224 switch: **422**, with
|
||||
`opens_retained`, `superseded_at`, `retained_has_restic_pw`. Added to the switch, not a restructure.
|
||||
- `cmd/felhom-agent/main.go` — the retained fetcher wired on the same self-scoped hub client.
|
||||
|
||||
## The coupling defect, and the fix
|
||||
## Fail-safe, in every direction
|
||||
|
||||
The prune keeps the newest N; the published-versions check demanded that **every** tag be
|
||||
downloadable. Nothing connected them, so CI went red at `28ba8593b8` — a commit whose own run had
|
||||
been **green the day before** — and would have gone red again at the next publish when `0.121.0` was
|
||||
evicted. Both now read `generic_versions_kept` from one file.
|
||||
nil fetcher · hub without the route (404) · transport failure · malformed package → **the original
|
||||
refusal stands, unchanged**. The worst outcome of this feature breaking is the behaviour before it.
|
||||
Attempts bounded (`MaxRetainedTried`, default 6) — each unwrap is ~1 s of scrypt, so an unbounded loop
|
||||
would turn one wrong code into a minutes-long hang.
|
||||
|
||||
**What CI no longer covers:** a released version **older than the retention window** is no longer
|
||||
asserted downloadable. Its git tag and its config tree are still asserted; only the binary's presence
|
||||
is dropped. The check names the dropped versions on every run.
|
||||
## Tests — 7, with REAL age crypto
|
||||
|
||||
**The number is not a located ruling.** `generic_versions_kept: 10` is what the registry demonstrably
|
||||
holds; no register row records a prune, and container packages hold 19 each. The file says so in its
|
||||
own header. The principled bound is the hub's vouched `min_agent` floor — nothing can install below
|
||||
it — and that is recorded as the follow-up.
|
||||
Real crypto because the two situations are indistinguishable **at the unwrap**; a faked unwrap would
|
||||
prove nothing about what was broken. Full suite green (`go build`/`vet`/`test ./...`), agent gates OK.
|
||||
|
||||
## Controls, all three run
|
||||
**Red-proof, mutation asserted applied before the run:** remove the `tryRetained` block from
|
||||
`RecoverOffsiteRepoPassword` →
|
||||
`err = escrow: the recovery code did not unwrap the identity escrow (wrong recovery code…)` →
|
||||
`TestRecover_CodeOpensRetainedPackage_IsNotAWrongCode` FAILS. **The lie returns, in those words.**
|
||||
That is the layer the lie actually lives in: removing the *controller's* case yields the neutral
|
||||
message instead, because R-224's safe default catches it.
|
||||
|
||||
| control | expected | got |
|
||||
|---|---|---|
|
||||
| live run, policy = 10 | green, and it names `0.120.0` as not asserted | **exit 0**, and it did |
|
||||
| widen policy to 11 | `0.120.0` re-enters the window and convicts | **exit 1**, `FAIL v0.120.0` |
|
||||
| policy file removed | INCONCLUSIVE, never silently unbounded | **exit 2**, naming the path it tried |
|
||||
## Released and deployed
|
||||
|
||||
## Red-proof of the new gate
|
||||
`release-agent.sh 0.129.0` — tagged `v0.129.0`, published, **verified by independent download**,
|
||||
sha256 `53a54f0620afbd6d…`. Installed on `felhom-pve`, `felhom-agent --version` = 0.129.0, unit active,
|
||||
journal clean (normal PBS verify cycle). **NOT VOUCHED** — that stays the operator's act.
|
||||
|
||||
Mutation: `CHANGELOG.md` head repointed to `## v0.129.0` — never tagged, never published. Asserted
|
||||
applied (`grep -c '^## v0.129.0'` → 1). Result **exit 1**, both legs convicting:
|
||||
## Bypass, stated as required
|
||||
|
||||
```
|
||||
- TAG v0.129.0 DOES NOT EXIST. … without the tag every install 404s mid-run, as root.
|
||||
Fix: git tag -a v0.129.0 <released-commit> && git push origin v0.129.0
|
||||
- PACKAGE 0.129.0 IS NOT PUBLISHED (HTTP 404 …).
|
||||
Fix: bash scripts/release-agent.sh 0.129.0
|
||||
```
|
||||
|
||||
Reverted; `git status` clean on `CHANGELOG.md`.
|
||||
|
||||
## Green gate
|
||||
|
||||
`python3 scripts/agent_gates.py` — `reuse-refs OK · instructions OK · published OK ·
|
||||
release-complete OK · all agent gates OK`.
|
||||
|
||||
## Not done here
|
||||
|
||||
The deleter of `0.120.0` is **still not established** and a second attempt failed — Gitea keeps no
|
||||
package-deletion trail, its container log no longer reaches the window, and the activity feed carries
|
||||
no package operation. Recorded in R-287, including the withdrawal of my own earlier over-claim that
|
||||
the router logs showed no DELETE: they do not cover the window, so they never said anything.
|
||||
`git push --no-verify` was used **once** for the code push. The `release-complete` gate refuses a
|
||||
CHANGELOG entry whose tag and package do not exist, and `release-agent.sh` refuses a tree that is not
|
||||
pushed — circular by construction. The bypass was immediately followed by the real release; gates were
|
||||
re-run afterwards and are **green**, and the tag+package now exist.
|
||||
|
||||
@@ -86,6 +86,7 @@
|
||||
| `Client.PoolAddVMID` | internal/proxmox/mutate.go | `PoolAddVMID(ctx, pool, vmid) error` | re-assert pool membership after a restore-over-existing (campaign-2 R2) | SYNC (no UPID, don't WaitTask); PVE `PUT /pools` is additive (merge, not replace) — `delete=1` removes; idempotent (already-member swallowed); needs `Pool.Allocate` at `/pool/<pool>`. `pct restore --pool` sets membership only at CREATE — a restore over an existing vmid drops it, so bring-up re-asserts post-restore |
|
||||
| `TLSConfig.build` / `normalizeFingerprint` | internal/proxmox/tls.go | `build() (*tls.Config, error)` | PVE leaf-cert SHA-256 pinning | No insecure default |
|
||||
| `pinnedTLS` | internal/pbs/pin.go | `pinnedTLS(fingerprint) (*tls.Config, error)` | PBS leaf pinning | Same model as PVE; 64-hex fingerprint normalized |
|
||||
| `httpx.NewTransport` | internal/httpx/transport.go | `NewTransport(tlsCfg, idleConnTimeout) *http.Transport` | **EVERY** hand-rolled `http.Transport` in this repo — pbs, hub and proxmox all pin TLS, so none can use `http.DefaultTransport` | **R-344: never inline `&http.Transport{TLSClientConfig: ...}` again.** A composite literal takes `IdleConnTimeout` **zero, which means retain idle connections FOREVER** — `http.DefaultTransport` sets 90s and a literal does not inherit it. Combined with a client rebuilt per cycle and dropped (`pbsTargetsFromPVE`), that stranded **388 sockets on ep0 in 46 h**, held open on BOTH sides. `idleConnTimeout <= 0` means **use the default**, never "no timeout". Returns a **FRESH** transport every call — a shared one would pool connections across differently pinned endpoints. Pinned by `internal/pbs/client_leak_test.go` (server-side connection counting) + `internal/httpx/transport_test.go` |
|
||||
| `hub.Client.Report` | internal/hub/client.go | `Report(ctx, *HostReport) (*ControlEnvelope, error)` | the heartbeat | Typed `TransportError`/`HTTPError`, never contain the bearer token |
|
||||
| `hub.Loop` + `MultiObserver` | internal/hub/loop.go | `NewLoop(...)`; `MultiObserver(obs...)` | resilient report loop + envelope fan-out | Errors logged, loop continues; interval clamped 60–3600 s |
|
||||
| `provision.BackHalf.Provision` | internal/provision/backhalf.go | `Provision(ctx, Input) (Result, error)` | guest bootstrap back-half | mint→render→0600 write→chown 100000:100000→`pct set` ro bind→onboot; token NEVER logged/returned. Bootstrap `local_api.endpoint` = the caller's `cfg.LocalAPI.ListenAddr` (main.go) — moving the agent bind to the island moves the guest dial for free (R-50, no template) |
|
||||
|
||||
@@ -59,7 +59,7 @@ import (
|
||||
|
||||
// version is the agent version. Overridable at build time with
|
||||
// -ldflags "-X main.version=<v>"; defaults to the in-repo CHANGELOG version.
|
||||
var version = "0.92.1"
|
||||
var version = "0.130.0"
|
||||
|
||||
// runGuestHook is the PVE hook body (`felhom-agent guest-hook <vmid> <phase>`). On pre-start it
|
||||
// creates placeholder dirs for any absent bind-mount source so the guest always boots (the C1 net);
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -15,6 +15,8 @@ import (
|
||||
"time"
|
||||
|
||||
"gitea.dooplex.hu/admin/felhom-agent/internal/config"
|
||||
|
||||
"gitea.dooplex.hu/admin/felhom-agent/internal/httpx"
|
||||
)
|
||||
|
||||
const reportPath = "/api/v1/host-report"
|
||||
@@ -49,8 +51,11 @@ func NewClient(cfg config.HubConfig, logger *slog.Logger) (*Client, error) {
|
||||
tlsCfg.RootCAs = pool
|
||||
}
|
||||
hc := &http.Client{
|
||||
Timeout: time.Duration(cfg.TimeoutSeconds) * time.Second,
|
||||
Transport: &http.Transport{TLSClientConfig: tlsCfg},
|
||||
Timeout: time.Duration(cfg.TimeoutSeconds) * time.Second,
|
||||
// R-344, consistency only: this client is built ONCE per process, so it never accumulated
|
||||
// and contributed nothing to the ep0 leak. It carried the same missing default, which over
|
||||
// a tunnel is how one idle connection survives long enough to fail on next use.
|
||||
Transport: httpx.NewTransport(tlsCfg, 0),
|
||||
}
|
||||
return newClient(cfg.URL, cfg.APIKey, cfg.HostID, hc, logger), nil
|
||||
}
|
||||
|
||||
+14
-2
@@ -10,6 +10,8 @@ import (
|
||||
"net/url"
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
"gitea.dooplex.hu/admin/felhom-agent/internal/httpx"
|
||||
)
|
||||
|
||||
// Client is the PBS-API client for ONE PBS server. Construct with NewClient. It is pure (no
|
||||
@@ -31,6 +33,11 @@ type Config struct {
|
||||
Secret string // token secret (from <id>.pw)
|
||||
Namespace string // PBS namespace (from storage.cfg `namespace`); "" = root. S4 per-customer tenancy.
|
||||
Timeout time.Duration
|
||||
|
||||
// IdleConnTimeout bounds how long this client's idle keep-alive connections are retained.
|
||||
// Zero means httpx.DefaultIdleConnTimeout (90s) — it does NOT mean "no timeout", which is the
|
||||
// R-344 defect. Production leaves it unset; only tests set it, to avoid a 90-second wait.
|
||||
IdleConnTimeout time.Duration
|
||||
}
|
||||
|
||||
// NewClient builds a fingerprint-pinned, token-authed PBS client.
|
||||
@@ -55,8 +62,13 @@ func NewClient(cfg Config) (*Client, error) {
|
||||
authHeader: "PBSAPIToken=" + cfg.TokenID + ":" + cfg.Secret,
|
||||
namespace: cfg.Namespace,
|
||||
http: &http.Client{
|
||||
Timeout: timeout,
|
||||
Transport: &http.Transport{TLSClientConfig: tlsCfg},
|
||||
Timeout: timeout,
|
||||
// R-344: this transport MUST come from httpx. pbsTargetsFromPVE (cmd/felhom-agent/
|
||||
// main.go) builds a fresh Client every cycle and drops the previous one, so a
|
||||
// transport with no idle timeout strands one connection per cycle, forever, on both
|
||||
// sides. That leaked 388 sockets onto ep0 in 46 hours. Pinned by
|
||||
// TestAbandonedClientsReleaseTheirConnections — do not inline an http.Transport here.
|
||||
Transport: httpx.NewTransport(tlsCfg, cfg.IdleConnTimeout),
|
||||
},
|
||||
}, nil
|
||||
}
|
||||
|
||||
@@ -0,0 +1,173 @@
|
||||
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)
|
||||
}
|
||||
}
|
||||
@@ -10,6 +10,8 @@ import (
|
||||
"net/url"
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
"gitea.dooplex.hu/admin/felhom-agent/internal/httpx"
|
||||
)
|
||||
|
||||
// doer is the minimal HTTP surface the client needs; *http.Client satisfies it.
|
||||
@@ -65,8 +67,10 @@ func NewClient(cfg Config) (*Client, error) {
|
||||
timeout = 30 * time.Second
|
||||
}
|
||||
hc := &http.Client{
|
||||
Timeout: timeout,
|
||||
Transport: &http.Transport{TLSClientConfig: tlsCfg},
|
||||
Timeout: timeout,
|
||||
// R-344, consistency only: built ONCE per process, so it never accumulated and contributed
|
||||
// nothing to the ep0 leak. Same missing default, corrected for the same reason.
|
||||
Transport: httpx.NewTransport(tlsCfg, 0),
|
||||
}
|
||||
return &Client{
|
||||
base: strings.TrimRight(cfg.Endpoint, "/") + "/api2/json",
|
||||
|
||||
Reference in New Issue
Block a user