Compare commits

..

3 Commits

Author SHA1 Message Date
admin 7569f34aeb CHANGELOG: correct a claim this session made and then disproved
gates / gates (push) Successful in 8s
The v0.130.0 draft said the 388 descriptors already stuck on ep0 would
persist until the PBS proxy restarted. Measured within the hour: they
clear when the AGENT restarts. demo-hp released its 199 in one second
(415 -> 216 fd); demo-felhom released the remaining 203 (220 -> 17 fd in
under two seconds). 17 is precisely ep0's t0 baseline of 2026-08-18.

They were held on both sides. Closing either side ends them. ep0 was
read-only throughout and its proxy PID never changed.
2026-08-20 12:39:12 +02:00
admin ede49b610d R-344: restore the idle-connection timeout our hand-rolled transports lost
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.
2026-08-20 11:09:39 +02:00
admin f17ed11599 REPORT: agent v0.129.0 — the retained-package recovery class, released and deployed
gates / gates (push) Successful in 21s
2026-08-12 18:49:51 +02:00
10 changed files with 449 additions and 58 deletions
+76
View File
@@ -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
+38 -51
View File
@@ -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.
+1
View File
@@ -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) |
+1 -1
View File
@@ -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);
+59
View File
@@ -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,
}
}
+74
View File
@@ -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)
}
}
+7 -2
View File
@@ -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
View File
@@ -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
}
+173
View File
@@ -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)
}
}
+6 -2
View File
@@ -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",