From 9056f01fae3b9add387a30ca89064ac792c2d2ef Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Sun, 26 Jul 2026 09:13:52 +0200 Subject: [PATCH] =?UTF-8?q?v0.173.0=20=E2=80=94=20R-77:=20endpoint-drift?= =?UTF-8?q?=20detection,=20samba=20protected-set=20gate,=20channel=20log?= =?UTF-8?q?=20honesty?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Source: felhom.eu/documentation/audits/DIAG-agent-channel-2026-07-26.md bootstrap.DetectEndpointDrift names a controller.yaml vs bootstrap.json local_api.endpoint divergence -- one ERROR carrying BOTH values and BOTH paths, its own event type local_api_endpoint_drift, and its own Hungarian banner shown ABOVE the channel banner because drift is the cause and "agent unreachable" the symptom. It writes NOTHING: reconciling from bootstrap.json would clobber a correct controller.yaml on any half-provisioned or hand-repaired guest, so the authority ruling is deferred to R-78. Fail-safe silent on absent/unparseable/ incomplete bootstrap and on an empty endpoint (ensureLocalAPI's fill-if-missing path is untouched). Fingerprint compared as a BOOLEAN only; token never compared, logged or exposed. EffectiveProtected now gates samba on Enabled && UserSet, mirroring BOTH of reconcileSambaAt's early returns, and the doc comment is corrected in the same change -- it claimed "detection and deployment agree in both directions" while citing only !smb.Enabled, an assertion that went false when !smb.UserSet was added. Not over-suppressed: sharing on WITH a password and a dead container still alarms. Channel log: the debounce placeholder is stateUnconfirmed (rendered "unseeded") instead of "up", so a born-down channel no longer logs "up->down" and orUnseeded stops being dead code. Logging only -- the placeholder is still matched in the re-arm condition, so F2 born-down alerting is byte-for-byte unchanged and all nine pre-existing channelhealth tests pass. Tests 951 -> 959, all green. Red-proofs A (both directions), E and F. MinAgent unchanged; felhom-agent untouched. --- CHANGELOG.md | 50 ++++ CONTEXT.md | 24 +- controller/cmd/controller/main.go | 14 ++ controller/internal/bootstrap/bootstrap.go | 92 ++++++++ controller/internal/bootstrap/drift_test.go | 223 ++++++++++++++++++ controller/internal/channelhealth/checker.go | 30 ++- .../internal/channelhealth/checker_test.go | 98 ++++++++ .../monitor/effective_protected_test.go | 53 ++++- controller/internal/monitor/healthcheck.go | 31 ++- controller/internal/notify/notifier.go | 18 ++ controller/internal/web/alerts.go | 34 ++- 11 files changed, 648 insertions(+), 19 deletions(-) create mode 100644 controller/internal/bootstrap/drift_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 658da2e..84f6768 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,55 @@ ## Changelog +### v0.173.0 — R-77: endpoint-drift detection, samba protected-set gate, channel log honesty (2026-07-26) + +Source: `felhom.eu/documentation/audits/DIAG-agent-channel-2026-07-26.md`. + +**Operational repair first (Part 0).** Both production controllers had been dialling their pre-island +LAN address since 2026-07-25 12:44 — the island migration rewrote `bootstrap.json` and +`controller.yaml` was never updated. `local_api.endpoint` corrected to `169.254.253.1:8443` on +demo-felhom and demo-hp (backups at `controller.yaml.pre-r77.bak`); **fingerprint and token agreed on +both boxes**, so only the address moved. Channel healthy since: zero `[channel]` lines and zero +`agent_channel_*` hub events after restart. + +**Endpoint-drift detection — DETECT AND NAME, never write** (`bootstrap.DetectEndpointDrift`). When +`controller.yaml` and `bootstrap.json` both carry a complete `local_api` block and their endpoints +disagree, the controller emits one ERROR naming **both values and both paths**, raises a **new, +dedicated event type `local_api_endpoint_drift`** (error severity — drift never self-heals), and +shows its own Hungarian banner *above* the channel banner, because drift is the CAUSE and +"agent unreachable" the symptom. It **does not reconcile the files**: the mirror-image failure — +clobbering a correct `controller.yaml` from a stale `bootstrap.json` — is just as bad, fleet-wide. +That authority ruling is **R-78**. Fail-safe to silence on an absent/unparseable/incomplete bootstrap +(an unprovisioned guest is not drifted) and on an empty endpoint (that is `ensureLocalAPI`'s +fill-if-missing path, untouched). The fingerprint is compared and reported as a **boolean only**; the +token is never compared, logged or exposed. + +**Hub allowlist (`felhom.eu` hub v0.74.0) — required, not optional.** `allowedEventTypes` 400s an +unknown `event_type`, so without the one-line entry the new alert would have been silently inert — +the exact seam-wiring failure this project has hit four times. Shipped with the controller. + +**Samba protected-set gate.** `EffectiveProtected` now requires `smb.Enabled && smb.UserSet`, +mirroring **both** of `reconcileSambaAt`'s early returns. Sharing enabled without a household +password means the controller deliberately does not deploy samba, yet the health monitor reported +`fail` for it — demo-hp reported `health=fail` to the hub from the moment sharing was switched on. +**The doc comment was corrected in the same change**: it claimed "detection and deployment agree in +both directions" while citing only `!smb.Enabled`, an assertion that became false when the +`!smb.UserSet` return was added — a comment documenting a guarantee the code no longer provides is +how the bug returns. Not over-suppressed: sharing on **with** a password and a dead container still +alarms. `TestEffectiveProtectedTracksSharingToggle` was updated — its old fixture asserted the buggy +behaviour. + +**Channel log honesty.** The debounce branch seeded an unseeded state to `"up"`, so a **born-down** +channel logged `up->down:` and `orUnseeded` was dead code. On 2026-07-25 that implied a +working channel degrading when neither controller had *ever* reached its agent, and it misdirected +the first read of the incident. The placeholder is now `stateUnconfirmed`, rendered `unseeded`. +**Logging only** — the placeholder is still matched in the re-arm condition, so F2 born-down alerting +is byte-for-byte unchanged; the Scenario-F test asserts sink call **count and arguments**, not just +the string, and all nine pre-existing channelhealth tests still pass. + +Tests 951 → 959, all green. Three red-proofs (A, E, F) recorded in REPORT.md — Scenario A in **both** +failure directions: no-detection, and the auto-correcting variant that trips the byte-identical +assertion. **MinAgent unchanged; felhom-agent untouched** (DIAG refuted H1 — the island is healthy). + ### v0.172.0 — R-75: canonical import root, catalog-derived skeleton, import surfaces (2026-07-26) Spike: `felhom.eu/documentation/audits/SPIKE-catalog-data-paths-2026-07-26.md`. diff --git a/CONTEXT.md b/CONTEXT.md index 5e6eda9..8ca3e08 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -7,7 +7,29 @@ > > Ask Claude Code: "Please update CONTEXT.md with what we did today" -Last updated: 2026-07-26 (v0.172.0 — R-75 canonical import root + catalog-derived skeleton) +Last updated: 2026-07-26 (v0.173.0 — R-77 endpoint-drift detection + samba gate + channel log honesty) + +> **2026-07-26 — v0.173.0 (R-77).** Source: `audits/DIAG-agent-channel-2026-07-26.md`. +> +> **UNRESOLVED AND DELIBERATELY DEFERRED — which file is authoritative for `local_api`?** R-77 ships +> DETECTION ONLY. `controller.yaml` and `bootstrap.json` can disagree; the controller dials +> `controller.yaml`. The obvious "fix" — reconcile from `bootstrap.json` on every boot — has a failure +> mode **as severe as the bug it fixes**: on a guest whose `controller.yaml` is correct and whose +> `bootstrap.json` is stale (a re-provision that half-completed, a hand-repaired guest, a +> setup-wizard box), auto-reconcile would clobber a WORKING channel on the next restart — fleet-wide, +> silently, at the moment of a routine deploy. R-77's position is that **naming the drift is enough**: +> it would have converted the 17.5 h outage into a specific alert on the first health cycle. The +> authority ruling is **R-78** and needs its own spike — do not resolve it opportunistically. +> +> Corollary for anyone editing `bootstrap.MaybeIngest`/`ensureLocalAPI`: `ensureLocalAPI` is the ONLY +> writer, it fires only when the endpoint is EMPTY, and `DetectEndpointDrift` must stay write-free. +> Scenario A's test asserts `controller.yaml` is byte-identical after the check, and its red-proof +> covers the auto-correcting variant precisely because that is the tempting wrong turn. +> +> **Also settled here:** the samba protected-set must mirror EVERY early return in +> `reconcileSambaAt` (currently two: `!smb.Enabled`, `!smb.UserSet`). A third would need the same +> mirror, and the doc comment above `EffectiveProtected` must be updated with it. + > **2026-07-26 — v0.172.0 (R-75).** Spike `felhom.eu/documentation/audits/SPIKE-catalog-data-paths-2026-07-26.md`; > feature doc `felhom.eu/documentation/controller/import-and-data-paths.md`. diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index ca17fbc..843dd28 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -1016,6 +1016,20 @@ func main() { sched.Every("agent-channel-health", 60*time.Second, chChecker.Check) } + // local_api endpoint drift (R-77, from the 2026-07-25 outage): controller.yaml and bootstrap.json + // can disagree indefinitely and silently — the island migration rewrote the latter and the + // controller kept dialling the former for 17.5 h, alerting only "agent unreachable". This NAMES + // the fault; it deliberately does not reconcile the files (R-78 owns which one wins). + // + // Startup-only is sufficient and correct: both files are read at boot and neither changes under a + // running controller, so a periodic re-check would add noise without adding signal. + if d := bootstrap.DetectEndpointDrift(*configPath, cfg, logger); d != nil { + alertMgr.SetEndpointDriftAlert(true, d.HungarianMessage()) + if notifier != nil { + notifier.NotifyEndpointDrift(d.EnglishMessage(), d.FingerprintAgrees) + } + } + // Wire debug callbacks (only in debug mode) if cfg.Logging.Level == "debug" { dc := &web.DebugCallbacks{} diff --git a/controller/internal/bootstrap/bootstrap.go b/controller/internal/bootstrap/bootstrap.go index 051ea63..b4f997f 100644 --- a/controller/internal/bootstrap/bootstrap.go +++ b/controller/internal/bootstrap/bootstrap.go @@ -320,3 +320,95 @@ func writeFileAtomic(path string, b []byte) error { } return os.Rename(tmp, path) } + +// --- Endpoint-drift detection (R-77, from DIAG-agent-channel-2026-07-26) --------------------- +// +// THE OUTAGE THIS EXISTS FOR: the R-50 island migration rewrote bootstrap.json's +// local_api.endpoint to the island address; controller.yaml kept the pre-island LAN address on the +// whole fleet; the agent no longer binds that address. Both controllers went dark for ~17.5 h, and +// the only alert said "agent unreachable" — indistinguishable from a dead agent or a network blip, +// so it read as infrastructure noise rather than a config fault. ensureLocalAPI could not catch it: +// it fills an ABSENT local_api block and returns early on a present one, stale or not. +// +// This is DETECTION AND NAMING ONLY. It deliberately does NOT reconcile the two files: +// +// the mirror-image failure is just as bad — on a guest whose controller.yaml is correct and whose +// bootstrap.json is stale, auto-reconcile would clobber a WORKING channel, fleet-wide, on the next +// restart. Which file is authoritative is a real, unresolved question and is tracked as R-78. +// +// Naming it is enough to have converted that outage into a specific, actionable alert on the first +// health cycle, which is the whole lesson of the incident. + +// EndpointDrift is a detected divergence between the two local_api sources. It carries no secrets: +// the fingerprint is reported as an agreement BOOLEAN and the token is not compared or exposed at +// all (a token mismatch is a different failure — see the field comment). +type EndpointDrift struct { + ConfigPath string // controller.yaml + BootstrapPath string // bootstrap.json + ConfigEndpoint string // what the controller is actually dialling + BootstrapEndpoint string // what the provisioning side last wrote + // FingerprintAgrees is false when the pin ALSO moved. That is a materially different (and worse) + // situation than a moved address — fixing the endpoint alone would then fail closed on the pin — + // so it is surfaced, as a boolean, never as a value. + FingerprintAgrees bool +} + +// DetectEndpointDrift compares controller.yaml's live local_api.endpoint against bootstrap.json's. +// Returns nil (silent, no alert) in every ambiguous or not-applicable case: +// +// - cfg is nil, or its endpoint is EMPTY — that is the fill-if-missing path ensureLocalAPI owns, +// not drift; +// - bootstrap.json is absent, unreadable or unparseable — a legacy / manually-configured / +// unprovisioned guest is not a drifted one; +// - the bootstrap local_api block is incomplete (any of endpoint/fingerprint/token empty) — the +// same completeness bar ensureLocalAPI applies before it will merge; +// - the endpoints agree. +// +// It reads two files and writes NOTHING. Emitting the ERROR here (rather than at the call site) +// keeps the diagnosis in one line of log even when the alert path is unavailable. +func DetectEndpointDrift(configPath string, cfg *config.Config, logger *log.Logger) *EndpointDrift { + if cfg == nil || cfg.LocalAPI.Endpoint == "" { + return nil + } + data, err := os.ReadFile(Path()) + if err != nil { + return nil + } + var b Bootstrap + if err := json.Unmarshal(data, &b); err != nil { + return nil + } + if b.LocalAPI.Endpoint == "" || b.LocalAPI.Fingerprint == "" || b.LocalAPI.Token == "" { + return nil + } + if cfg.LocalAPI.Endpoint == b.LocalAPI.Endpoint { + return nil + } + d := &EndpointDrift{ + ConfigPath: configPath, + BootstrapPath: Path(), + ConfigEndpoint: cfg.LocalAPI.Endpoint, + BootstrapEndpoint: b.LocalAPI.Endpoint, + FingerprintAgrees: cfg.LocalAPI.Fingerprint == b.LocalAPI.Fingerprint, + } + if logger != nil { + logger.Printf("[ERROR] bootstrap: local_api endpoint DRIFT — %s says %q but %s says %q; "+ + "the controller is dialling the FORMER. Pin agrees: %v. Not auto-corrected (R-78 owns the "+ + "authority ruling) — fix the intended file and restart the controller.", + d.ConfigPath, d.ConfigEndpoint, d.BootstrapPath, d.BootstrapEndpoint, d.FingerprintAgrees) + } + return d +} + +// EnglishMessage is the operator-tier alert body (operator events are English by convention). +func (d *EndpointDrift) EnglishMessage() string { + return fmt.Sprintf("local_api endpoint drift: controller.yaml=%s bootstrap.json=%s (pin agrees: %v) "+ + "— the controller is dialling controller.yaml's value; the agent may be listening on the other.", + d.ConfigEndpoint, d.BootstrapEndpoint, d.FingerprintAgrees) +} + +// HungarianMessage is the customer-facing dashboard line, matching channelhealth's tone (short, +// no addresses — the operator gets those in the event and the log). +func (d *EndpointDrift) HungarianMessage() string { + return "A tárolókezelő ügynök címe elavult a beállításokban." +} diff --git a/controller/internal/bootstrap/drift_test.go b/controller/internal/bootstrap/drift_test.go new file mode 100644 index 0000000..ac49508 --- /dev/null +++ b/controller/internal/bootstrap/drift_test.go @@ -0,0 +1,223 @@ +package bootstrap + +import ( + "bytes" + "encoding/json" + "io" + "log" + "os" + "path/filepath" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/config" +) + +// R-77 endpoint-drift detection. The outage this guards is DIAG-agent-channel-2026-07-26: the island +// migration rewrote bootstrap.json, controller.yaml kept the pre-island LAN address, and the fleet's +// controllers dialled the wrong host for 17.5 h while alerting only "agent unreachable". + +const driftYAML = `customer: + id: demo-hp +local_api: + endpoint: %ENDPOINT% + fingerprint: aaaa1111 + token: tok-secret +` + +// writeDriftFixture lays out a controller.yaml + bootstrap.json pair and points Path() at the latter. +func writeDriftFixture(t *testing.T, cfgEndpoint, bsEndpoint, bsFingerprint, bsToken string) (cfgPath string, cfg *config.Config) { + t.Helper() + dir := t.TempDir() + cfgPath = filepath.Join(dir, "controller.yaml") + if err := os.WriteFile(cfgPath, []byte(strings.ReplaceAll(driftYAML, "%ENDPOINT%", cfgEndpoint)), 0o600); err != nil { + t.Fatal(err) + } + bsPath := filepath.Join(dir, "bootstrap.json") + b := Bootstrap{} + b.LocalAPI = BootstrapLocalAPI{Endpoint: bsEndpoint, Fingerprint: bsFingerprint, Token: bsToken} + raw, _ := json.Marshal(b) + if err := os.WriteFile(bsPath, raw, 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("FELHOM_BOOTSTRAP_PATH", bsPath) + + var err error + cfg, err = config.LoadPermissive(cfgPath) + if err != nil { + t.Fatal(err) + } + return cfgPath, cfg +} + +func sha(t *testing.T, p string) []byte { + t.Helper() + b, err := os.ReadFile(p) + if err != nil { + t.Fatal(err) + } + return b +} + +// Scenario A — drift is DETECTED, NAMED, and nothing is written. +// +// The "nothing is written" half is the load-bearing assertion: auto-reconcile is R-78 and would +// clobber a working channel on any guest whose controller.yaml is the correct one. "An error was +// logged" alone would be a hollow test. +func TestScenarioA_DriftDetectedAndNamed_NoWrite(t *testing.T) { + cfgPath, cfg := writeDriftFixture(t, + "192.168.0.87:8443", // the live demo-hp value + "169.254.253.1:8443", // the island value the migration wrote + "aaaa1111", "tok-secret") + before := sha(t, cfgPath) + + var buf bytes.Buffer + d := DetectEndpointDrift(cfgPath, cfg, log.New(&buf, "", 0)) + + if d == nil { + t.Fatal("drift must be DETECTED — this is the exact live shape from the 2026-07-25 outage") + } + if d.ConfigEndpoint != "192.168.0.87:8443" || d.BootstrapEndpoint != "169.254.253.1:8443" { + t.Errorf("wrong values captured: %+v", d) + } + if !d.FingerprintAgrees { + t.Error("fingerprints are identical in this fixture — must report agreement") + } + + // The log line alone must diagnose it: BOTH values AND BOTH paths. + logged := buf.String() + for _, want := range []string{"192.168.0.87:8443", "169.254.253.1:8443", cfgPath, "bootstrap.json", "ERROR"} { + if !strings.Contains(logged, want) { + t.Errorf("the ERROR line must contain %q; got:\n%s", want, logged) + } + } + // Never log a secret. + if strings.Contains(logged, "tok-secret") { + t.Error("the token must NEVER be logged") + } + + // THE assertion: controller.yaml is byte-identical. + if after := sha(t, cfgPath); !bytes.Equal(before, after) { + t.Errorf("controller.yaml was MODIFIED — detection must never write (that is R-78)\nbefore:\n%s\nafter:\n%s", before, after) + } +} + +// Scenario B — agreement is silent. A spurious alert on every healthy boot would be worse than the +// bug: it trains the operator to ignore the banner. +func TestScenarioB_AgreementIsSilent(t *testing.T) { + cfgPath, cfg := writeDriftFixture(t, "169.254.253.1:8443", "169.254.253.1:8443", "aaaa1111", "tok-secret") + before := sha(t, cfgPath) + var buf bytes.Buffer + if d := DetectEndpointDrift(cfgPath, cfg, log.New(&buf, "", 0)); d != nil { + t.Errorf("matching endpoints must not report drift: %+v", d) + } + if strings.Contains(buf.String(), "ERROR") { + t.Errorf("no ERROR on agreement; got: %s", buf.String()) + } + if after := sha(t, cfgPath); !bytes.Equal(before, after) { + t.Error("controller.yaml must not be touched") + } +} + +// Scenario D — an absent / unparseable / incomplete bootstrap is NOT drift. An unprovisioned or +// legacy guest must stay silent, not alarm. +func TestScenarioD_IncompleteBootstrapIsFailSafe(t *testing.T) { + t.Run("bootstrap absent", func(t *testing.T) { + cfgPath, cfg := writeDriftFixture(t, "192.168.0.87:8443", "169.254.253.1:8443", "aaaa1111", "tok") + os.Remove(os.Getenv("FELHOM_BOOTSTRAP_PATH")) + if d := DetectEndpointDrift(cfgPath, cfg, log.New(io.Discard, "", 0)); d != nil { + t.Errorf("a missing bootstrap is not drift: %+v", d) + } + }) + t.Run("bootstrap unparseable", func(t *testing.T) { + cfgPath, cfg := writeDriftFixture(t, "192.168.0.87:8443", "169.254.253.1:8443", "aaaa1111", "tok") + os.WriteFile(os.Getenv("FELHOM_BOOTSTRAP_PATH"), []byte("{not json"), 0o600) + if d := DetectEndpointDrift(cfgPath, cfg, log.New(io.Discard, "", 0)); d != nil { + t.Errorf("an unparseable bootstrap is not drift: %+v", d) + } + }) + // Incomplete = any of endpoint/fingerprint/token empty — the same completeness bar ensureLocalAPI + // applies before it will merge. + for _, tc := range []struct{ name, ep, fp, tok string }{ + {"no endpoint", "", "aaaa1111", "tok"}, + {"no fingerprint", "169.254.253.1:8443", "", "tok"}, + {"no token", "169.254.253.1:8443", "aaaa1111", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + cfgPath, cfg := writeDriftFixture(t, "192.168.0.87:8443", tc.ep, tc.fp, tc.tok) + if d := DetectEndpointDrift(cfgPath, cfg, log.New(io.Discard, "", 0)); d != nil { + t.Errorf("an incomplete bootstrap is not drift: %+v", d) + } + }) + } + t.Run("controller.yaml has no endpoint — that is the fill-if-missing path, not drift", func(t *testing.T) { + cfgPath, cfg := writeDriftFixture(t, `""`, "169.254.253.1:8443", "aaaa1111", "tok") + if cfg.LocalAPI.Endpoint != "" { + t.Skipf("fixture did not produce an empty endpoint (got %q)", cfg.LocalAPI.Endpoint) + } + if d := DetectEndpointDrift(cfgPath, cfg, log.New(io.Discard, "", 0)); d != nil { + t.Errorf("an absent local_api endpoint is ensureLocalAPI's job, not drift: %+v", d) + } + }) + t.Run("nil cfg", func(t *testing.T) { + if d := DetectEndpointDrift("/nonexistent", nil, log.New(io.Discard, "", 0)); d != nil { + t.Error("nil cfg must be silent") + } + }) +} + +// A moved PIN alongside a moved address is a materially different failure — fixing the endpoint +// alone would then fail closed on the pin. It must be surfaced, as a boolean, never as a value. +func TestDrift_FingerprintDisagreementIsSurfacedNotLeaked(t *testing.T) { + cfgPath, cfg := writeDriftFixture(t, "192.168.0.87:8443", "169.254.253.1:8443", "bbbb2222", "tok-secret") + var buf bytes.Buffer + d := DetectEndpointDrift(cfgPath, cfg, log.New(&buf, "", 0)) + if d == nil { + t.Fatal("drift expected") + } + if d.FingerprintAgrees { + t.Error("fingerprints differ in this fixture — must report DISagreement") + } + for _, secret := range []string{"aaaa1111", "bbbb2222", "tok-secret"} { + if strings.Contains(buf.String(), secret) { + t.Errorf("a fingerprint/token value leaked into the log: %q", secret) + } + if strings.Contains(d.EnglishMessage(), secret) { + t.Errorf("a fingerprint/token value leaked into the operator message: %q", secret) + } + } + if !strings.Contains(d.EnglishMessage(), "false") { + t.Errorf("the operator message must carry the pin-agreement boolean: %q", d.EnglishMessage()) + } +} + +// Scenario C — the fill-if-missing path is untouched by any of this (it is the ONLY writer). +func TestScenarioC_AbsentBlockStillMerges(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "controller.yaml") + if err := os.WriteFile(cfgPath, []byte("customer:\n id: demo-hp\n"), 0o600); err != nil { + t.Fatal(err) + } + bsPath := filepath.Join(dir, "bootstrap.json") + b := Bootstrap{} + b.LocalAPI = BootstrapLocalAPI{Endpoint: "169.254.253.1:8443", Fingerprint: "aaaa1111", Token: "tok"} + raw, _ := json.Marshal(b) + os.WriteFile(bsPath, raw, 0o600) + t.Setenv("FELHOM_BOOTSTRAP_PATH", bsPath) + + cfg, err := config.LoadPermissive(cfgPath) + if err != nil { + t.Fatal(err) + } + if cfg.LocalAPI.Endpoint != "" { + t.Fatalf("precondition: fixture should have no local_api, got %q", cfg.LocalAPI.Endpoint) + } + got := ensureLocalAPI(cfgPath, cfg, log.New(io.Discard, "", 0)) + if got == nil || got.LocalAPI.Endpoint != "169.254.253.1:8443" { + t.Fatalf("the fill-if-missing merge must still work; got %+v", got) + } + // And the drift check stays silent on the now-merged config. + if d := DetectEndpointDrift(cfgPath, got, log.New(io.Discard, "", 0)); d != nil { + t.Errorf("after a successful merge the two files agree — no drift: %+v", d) + } +} diff --git a/controller/internal/channelhealth/checker.go b/controller/internal/channelhealth/checker.go index ddedce9..80d0a53 100644 --- a/controller/internal/channelhealth/checker.go +++ b/controller/internal/channelhealth/checker.go @@ -105,6 +105,11 @@ func classify(constructionErr bool, err error) classification { } } +// stateUnconfirmed is the debounce placeholder for a checker that has never observed a healthy +// probe. It is deliberately NOT "up": it must not be reported as an observation. See Check's +// debounce branch and orUnseeded. +const stateUnconfirmed = "unconfirmed" + // Checker holds the in-memory channel state. No persistence — the state is re-derived each run // (mirrors the AlertManager's state-based model). Safe for the single scheduler caller; the mutex // guards against an overlapping run. @@ -114,7 +119,7 @@ type Checker struct { logger *log.Logger mu sync.Mutex - state string // "" (unseeded) | "up" | "down:" + state string // "" (unseeded) | stateUnconfirmed (debounce placeholder) | "up" | "down:" consecutiveDown int alerted bool // have we emitted a down alert for the CURRENT down-spell? (F2: drives // alerting instead of `prev==""`, so a BORN-down — broken at startup/reseed — alerts too, not @@ -156,10 +161,17 @@ func (c *Checker) Check(ctx context.Context) error { c.consecutiveDown++ if cls.debounce && c.consecutiveDown < debounceThreshold { // A transient blip (e.g. the ~1s agent-restart socket gap, or the agent not yet up on a cold - // boot). Hold the previous state — do NOT flip the dashboard or notify. Unseeded → assume up - // until confirmed (so a transient born-down still needs N>=2 before it alerts). + // boot). Hold the previous state — do NOT flip the dashboard or notify. + // + // An unseeded checker is held as "not yet confirmed down" so a transient born-down still needs + // N>=2 before it alerts — the debounce must apply to a cold boot exactly as it does to a live + // blip. R-77: that hold used to be spelled `c.state = "up"`, which made a BORN-DOWN channel log + // `up->down` and left orUnseeded dead code. During the 2026-07-25 outage the log therefore + // implied a working channel degrading, when in truth neither controller had EVER reached its + // agent — which actively misdirected the first read of the incident. The debounce semantics are + // unchanged; only the state label is honest now. if c.state == "" { - c.state = "up" + c.state = stateUnconfirmed } c.logger.Printf("[DEBUG] [channel] transient down (%s, %d/%d) — suppressed pending confirmation: %v", cls.reason, c.consecutiveDown, debounceThreshold, perr) @@ -172,7 +184,10 @@ func (c *Checker) Check(ctx context.Context) error { newState := "down:" + string(cls.reason) c.sink.SetDashboard(true, cls.reason, cls.hungarian) // dashboard reflects current state always prev := c.state - if prev == "" || prev == "up" || prev != newState { + // stateUnconfirmed counts as unseeded for BOTH the re-arm decision and the log label: it is the + // debounce placeholder, never an observed up. Keeping it in this condition preserves the F2 + // born-down alerting behaviour byte-for-byte (it used to be spelled "up" and matched here). + if prev == "" || prev == "up" || prev == stateUnconfirmed || prev != newState { c.alerted = false } c.state = newState @@ -185,8 +200,11 @@ func (c *Checker) Check(ctx context.Context) error { return nil } +// orUnseeded renders a state for the log. Both the never-observed state ("") and the debounce +// placeholder render as "unseeded" — a born-down channel must never be logged as `up->down`, which +// is what R-77 fixed. Before that, the placeholder was literally "up" and this function was dead code. func orUnseeded(s string) string { - if s == "" { + if s == "" || s == stateUnconfirmed { return "unseeded" } return s diff --git a/controller/internal/channelhealth/checker_test.go b/controller/internal/channelhealth/checker_test.go index dc2a6b0..1249920 100644 --- a/controller/internal/channelhealth/checker_test.go +++ b/controller/internal/channelhealth/checker_test.go @@ -1,10 +1,12 @@ package channelhealth import ( + "bytes" "context" "errors" "io" "log" + "strings" "testing" ) @@ -272,3 +274,99 @@ func TestProbe_SeamOnly(t *testing.T) { t.Fatalf("checker should call the probe seam exactly once, got %d", p.calls) } } + +// R-77 Scenario F — the confirmed-down line must distinguish BORN-DOWN from a real transition. +// +// The debounce branch used to seed an unseeded state to "up", so a channel that had NEVER reached +// its agent logged `up->down:unreachable` and orUnseeded was dead code. On 2026-07-25 that made the +// log imply a working channel degrading, when in truth neither controller had ever been up — it +// actively misdirected the first read of the incident. +// +// THE OTHER HALF OF THIS TEST IS THE POINT: alerting, debounce and dashboard behaviour must be +// byte-for-byte unchanged. A logging fix that shifts alerting is a regression in a cosmetic disguise, +// so the sink calls are asserted in COUNT and ARGUMENTS, not just the log string. +func TestScenarioF_BornDownLogsUnseededNotUp(t *testing.T) { + var logbuf bytes.Buffer + sink := &fakeSink{} + c := New(nil, sink, log.New(&logbuf, "", 0)) + // A debounced reason (connection refused → unreachable), failing from the very first probe. + p := &scriptedProbe{steps: []struct { + cons bool + err error + }{step(false, errors.New("dial tcp 192.168.0.87:8443: connect: connection refused"))}} + c.probe = p.fn + + // Probe 1: debounced, suppressed — no dashboard flip, no alert. + if err := c.Check(context.Background()); err != nil { + t.Fatal(err) + } + if sink.dashDown || len(sink.downs) != 0 { + t.Fatalf("probe 1 must be suppressed by debounce: dashDown=%v downs=%d", sink.dashDown, len(sink.downs)) + } + // The placeholder must NOT be the string "up". + if got := c.State(); got == "up" { + t.Error("an unseeded checker must not report state \"up\" after a suppressed first failure") + } + + // Probe 2: confirmed down. + if err := c.Check(context.Background()); err != nil { + t.Fatal(err) + } + + logged := logbuf.String() + if !strings.Contains(logged, "unseeded->down:unreachable") { + t.Errorf("born-down must log \"unseeded->down:unreachable\"; got:\n%s", logged) + } + if strings.Contains(logged, "up->down") { + t.Errorf("a channel that was never up must NOT log \"up->down\"; got:\n%s", logged) + } + + // --- the non-change assertions --- + if len(sink.downs) != 1 { + t.Fatalf("exactly ONE down alert must fire (F2 born-down behaviour unchanged), got %d", len(sink.downs)) + } + got := sink.downs[0] + if got.reason != ReasonUnreachable || got.eventType != "agent_channel_unreachable" || got.severity != "warning" { + t.Errorf("alert arguments changed: %+v — Part 3 is a LOGGING fix only", got) + } + if !sink.dashDown || sink.dashReason != ReasonUnreachable { + t.Errorf("dashboard must be set down/unreachable, got down=%v reason=%q", sink.dashDown, sink.dashReason) + } + if sink.recovered != 0 { + t.Errorf("no recovery must fire, got %d", sink.recovered) + } + if p.calls != 2 { + t.Errorf("debounce threshold unchanged: expected 2 probes, got %d", p.calls) + } +} + +// A REAL up->down transition must still log "up->down" — the fix must not relabel everything. +func TestScenarioF_RealTransitionStillLogsUp(t *testing.T) { + var logbuf bytes.Buffer + sink := &fakeSink{} + c := New(nil, sink, log.New(&logbuf, "", 0)) + p := &scriptedProbe{steps: []struct { + cons bool + err error + }{ + step(false, nil), // observed UP for real + step(false, errors.New("connect: connection refused")), + step(false, errors.New("connect: connection refused")), + }} + c.probe = p.fn + for i := 0; i < 3; i++ { + if err := c.Check(context.Background()); err != nil { + t.Fatal(err) + } + } + logged := logbuf.String() + if !strings.Contains(logged, "up->down:unreachable") { + t.Errorf("a genuine transition must still log \"up->down:unreachable\"; got:\n%s", logged) + } + if strings.Contains(logged, "unseeded->down") { + t.Errorf("an observed-up channel must not log \"unseeded->down\"; got:\n%s", logged) + } + if len(sink.downs) != 1 { + t.Errorf("exactly one down alert, got %d", len(sink.downs)) + } +} diff --git a/controller/internal/monitor/effective_protected_test.go b/controller/internal/monitor/effective_protected_test.go index 7bc7609..8f532a9 100644 --- a/controller/internal/monitor/effective_protected_test.go +++ b/controller/internal/monitor/effective_protected_test.go @@ -42,13 +42,19 @@ func TestEffectiveProtectedDropsCloudflaredWithoutToken(t *testing.T) { // R-7b Scenario E, BOTH directions. Sharing is a customer-toggled feature, so the samba container can // never be in the golden controller.yaml — the effective set must add it dynamically when sharing is -// ON (so a dead sharing service raises the same protected-container issue as a dead traefik) and must +// live (so a dead sharing service raises the same protected-container issue as a dead traefik) and must // leave it out when sharing is OFF (so a box that never enabled it never reports a missing container). -// Red-proof: delete the `if smb.Enabled` append and the enabled case fails. +// Red-proof: delete the samba append and the enabled case fails. +// +// R-77 TIGHTENED THE "ON" CASE, and this test was updated with it: "on" now means +// Enabled AND UserSet, because reconcileSambaAt refuses to deploy without a household password. The +// previous fixture used Enabled alone and therefore asserted the very behaviour that produced the +// live false alarm on demo-hp (2026-07-26). The three-state matrix is in +// TestScenarioE_SambaProtectedOnlyWhenActuallyDeployed below. func TestEffectiveProtectedTracksSharingToggle(t *testing.T) { cfg := &config.Config{Stacks: config.StacksConfig{Protected: []string{"traefik", "felhom-controller"}}} - on := EffectiveProtected(cfg, settings.SMBSettings{Enabled: true}) + on := EffectiveProtected(cfg, settings.SMBSettings{Enabled: true, UserSet: true}) if !contains(on, infra.SambaContainerName) { t.Errorf("sharing ON: %q must be watched, got %v", infra.SambaContainerName, on) } @@ -65,3 +71,44 @@ func TestEffectiveProtectedTracksSharingToggle(t *testing.T) { } } } + +// R-77 Scenario E — the samba protected-set gate must mirror reconcileSambaAt's BOTH early returns. +// +// The live false alarm (demo-hp, 2026-07-26): sharing was enabled without a household password, so +// reconcileSambaAt deliberately did not deploy the container, but EffectiveProtected added it anyway +// and the box reported health=fail for a state the controller itself had chosen. +func TestScenarioE_SambaProtectedOnlyWhenActuallyDeployed(t *testing.T) { + cfg := &config.Config{Stacks: config.StacksConfig{ + Protected: []string{"traefik", "cloudflared", "felhom-controller", "filebrowser"}}} + cfg.Infrastructure.CFTunnelToken = "tok" // keep cloudflared in, so the samba change is isolated + + for _, tc := range []struct { + name string + smb settings.SMBSettings + want bool + why string + }{ + {"(1) sharing OFF", settings.SMBSettings{Enabled: false, UserSet: false}, false, + "a box that never enabled sharing must stay quiet"}, + {"(2) sharing ON, no password", settings.SMBSettings{Enabled: true, UserSet: false}, false, + "THE BUG: reconcileSambaAt refuses to deploy without a password — a deliberate state, not a fault"}, + {"(3) sharing ON, password set", settings.SMBSettings{Enabled: true, UserSet: true}, true, + "the container really should be running — a dead one must STILL alarm (do not over-suppress)"}, + } { + if got := contains(EffectiveProtected(cfg, tc.smb), infra.SambaContainerName); got != tc.want { + t.Errorf("%s: samba protected = %v, want %v — %s", tc.name, got, tc.want, tc.why) + } + } + + // Not over-suppressed: the rest of the protected set is untouched in every sharing state. + for _, smb := range []settings.SMBSettings{ + {Enabled: false}, {Enabled: true}, {Enabled: true, UserSet: true}, + } { + set := EffectiveProtected(cfg, smb) + for _, must := range []string{"traefik", "cloudflared", "felhom-controller", "filebrowser"} { + if !contains(set, must) { + t.Errorf("smb=%+v: %q must always stay protected, got %v", smb, must, set) + } + } + } +} diff --git a/controller/internal/monitor/healthcheck.go b/controller/internal/monitor/healthcheck.go index 618a9f8..9795700 100644 --- a/controller/internal/monitor/healthcheck.go +++ b/controller/internal/monitor/healthcheck.go @@ -251,14 +251,28 @@ func checkDocker() error { // // - cloudflared is dropped when no tunnel token is configured (a LAN-only node legitimately runs // without it, so it must not be reported as a missing protected container forever); -// - the samba container is ADDED when network sharing is switched on (R-7b). Sharing is a -// customer-toggled feature, so it can never appear in the golden controller.yaml — but once it -// IS on, a dead sharing service is exactly as customer-visible as a dead traefik and must raise -// the same protected-container issue → alert → Hungarian degradation e-mail. When sharing is -// off the container is absent from the set, so a box that never enabled it stays quiet. +// - the samba container is ADDED only when network sharing is switched on AND the household +// password has been set (R-7b, tightened by R-77). Sharing is a customer-toggled feature, so it +// can never appear in the golden controller.yaml — but once it is actually RUNNING, a dead +// sharing service is exactly as customer-visible as a dead traefik and must raise the same +// protected-container issue → alert → Hungarian degradation e-mail. // -// The bring-up applies the same conditions (stacks.EnsureBaseStack for cloudflared, ensureSamba's -// `if !smb.Enabled { return }` for samba), so detection and deployment agree in both directions. +// THE COUPLING, and why it is spelled out: this set must mirror EVERY early return in +// stacks.reconcileSambaAt, because that function decides whether the container exists at all. It has +// TWO: +// +// if !smb.Enabled { return } // feature off +// if !smb.UserSet { return } // on, but no household password yet → deliberately NOT deployed +// +// R-77 exists because this comment previously claimed "detection and deployment agree in both +// directions" while citing only the first. The second was added later and never mirrored here, so +// enabling sharing without setting a password made the box report health=fail forever for a state +// the controller had deliberately chosen (observed live on demo-hp, 2026-07-26). A THIRD early +// return in reconcileSambaAt would need the same mirror — and this comment must be updated with it, +// because a comment asserting a guarantee the code no longer provides is how the bug came back. +// +// Deliberately NOT over-suppressed: sharing on WITH a password and a dead container still raises the +// issue. That is the case the protected set exists for. // // NOTE: the entries are CONTAINER names (checkProtectedContainers docker-inspects them). For the // base stacks the container name happens to equal the stack name; for samba it does NOT — the stack @@ -272,7 +286,8 @@ func EffectiveProtected(cfg *config.Config, smb settings.SMBSettings) []string { } out = append(out, name) } - if smb.Enabled { + // Mirrors reconcileSambaAt's two early returns — see the coupling note above. + if smb.Enabled && smb.UserSet { out = append(out, infra.SambaContainerName) } return out diff --git a/controller/internal/notify/notifier.go b/controller/internal/notify/notifier.go index 5233e1e..4e70ffe 100644 --- a/controller/internal/notify/notifier.go +++ b/controller/internal/notify/notifier.go @@ -375,6 +375,24 @@ func (n *Notifier) NotifyAgentChannelRecovered() { "Controller→agent channel recovered — local-API reachable again.", nil) } +// NotifyEndpointDrift reports a local_api endpoint divergence (R-77). Operator-only English, its +// OWN event type — deliberately not folded into agent_channel_*, because during the 2026-07-25 +// outage the generic channel alert was the only signal and it hid a specific, fixable config fault. +// severity=error: unlike a transient unreachable, drift never self-heals. +// +// NOTE: the hub validates event_type against allowedEventTypes and 400s an unknown one, so this +// type MUST exist there too (hub handler.go) or the alert is silently inert. +func (n *Notifier) NotifyEndpointDrift(message string, fingerprintAgrees bool) { + n.PushEvent("local_api_endpoint_drift", "error", message, + EndpointDriftDetails{FingerprintAgrees: fingerprintAgrees}) +} + +// EndpointDriftDetails carries NO addresses and NO secrets — the endpoints are in the message, and +// the pin is a boolean by design. +type EndpointDriftDetails struct { + FingerprintAgrees bool `json:"fingerprint_agrees"` +} + // NotifyAppDeployed sends an app deployment event. func (n *Notifier) NotifyAppDeployed(stackName, displayName string) { n.PushEvent("app_deployed", "info", diff --git a/controller/internal/web/alerts.go b/controller/internal/web/alerts.go index 78168db..169d19c 100644 --- a/controller/internal/web/alerts.go +++ b/controller/internal/web/alerts.go @@ -42,6 +42,10 @@ type AlertManager struct { // out-of-band, state-based, self-clearing model as agentChannelAlert: passing an empty slice when // every deployed app is running clears the banner with no manual dismissal. deadAppAlerts []Alert + // endpointDriftAlert (R-77) is set/cleared at startup by the local_api drift check. Separate from + // agentChannelAlert on purpose: drift is usually the CAUSE and "agent unreachable" the SYMPTOM, + // and during the 2026-07-25 outage only the symptom was visible. + endpointDriftAlert *Alert } // NewAlertManager creates a new AlertManager. @@ -77,6 +81,28 @@ func (am *AlertManager) SetAgentChannelAlert(down bool, msg string) { } } +// SetEndpointDriftAlert sets (drift=true) or clears the local_api endpoint-drift banner (R-77). +// +// It is deliberately a SEPARATE alert from SetAgentChannelAlert: during the 2026-07-25 outage the +// generic "agent unreachable" banner was the ONLY signal, and it looked like a dead agent. The two +// can also be true at once — a drifted endpoint usually CAUSES the channel to be down — so folding +// them together would hide the actionable one behind the symptom. +func (am *AlertManager) SetEndpointDriftAlert(drift bool, msg string) { + am.mu.Lock() + defer am.mu.Unlock() + if !drift { + am.endpointDriftAlert = nil + return + } + am.endpointDriftAlert = &Alert{ + ID: "local-api-endpoint-drift", + Level: "error", + Message: msg, + Link: "/settings", + LinkText: "Beállítások", + } +} + // DeadApp is a deployed app the health loop found not-running (fix-3). State is the container-state // string for the display (e.g. "stopped"/"exited"). type DeadApp struct { @@ -244,7 +270,7 @@ func (am *AlertManager) GetAlerts(excludeIDs ...string) []Alert { am.mu.RLock() defer am.mu.RUnlock() - if len(am.alerts) == 0 && am.agentChannelAlert == nil && len(am.deadAppAlerts) == 0 { + if len(am.alerts) == 0 && am.agentChannelAlert == nil && am.endpointDriftAlert == nil && len(am.deadAppAlerts) == 0 { return nil } @@ -254,6 +280,12 @@ func (am *AlertManager) GetAlerts(excludeIDs ...string) []Alert { } var result []Alert + // Endpoint drift first: it is the actionable CAUSE, and the channel-down banner below is usually + // just its symptom. Showing the symptom above the cause is what made the 2026-07-25 outage read + // as an infrastructure blip for 17.5 h. + if am.endpointDriftAlert != nil && !exclude[am.endpointDriftAlert.ID] { + result = append(result, *am.endpointDriftAlert) + } // Channel-down is prepended (highest priority — the agent link being dead breaks disk/storage UI). if am.agentChannelAlert != nil && !exclude[am.agentChannelAlert.ID] { result = append(result, *am.agentChannelAlert)