From f885100d2946679f1e41f32aa76579f6ae5daf74 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Mon, 5 Oct 2026 21:52:05 +0200 Subject: [PATCH] R-271: the channel recovery after a controller restart is reported The agent_channel_unauthorized alert's remedy is a re-bootstrap (a restart), and an unseeded->up first observation was silent, so following the instruction guaranteed no recovery event. A down alert now leaves a marker in the data dir; the first UP after a restart sends the recovery and clears it. A restart with no alert outstanding stays silent. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS --- controller/cmd/controller/main.go | 3 +- controller/internal/channelhealth/checker.go | 47 ++++++++++++- .../r271_restart_recovery_test.go | 69 +++++++++++++++++++ 3 files changed, 117 insertions(+), 2 deletions(-) create mode 100644 controller/internal/channelhealth/r271_restart_recovery_test.go diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index 89af2e3..45d4459 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -2002,7 +2002,8 @@ func main() { // launches the job immediately since the scheduler is already started. if cfg.LocalAPI.Endpoint != "" { chSink := channelSink{notifier: notifier, alertMgr: alertMgr} - chChecker := channelhealth.New(webServer.ProbeAgentChannel, chSink, logger) + chChecker := channelhealth.New(webServer.ProbeAgentChannel, chSink, logger). + WithAlertMarker(filepath.Join(cfg.Paths.DataDir, "channelhealth-down-alerted")) // R-271 sched.Every("agent-channel-health", 60*time.Second, chChecker.Check) } diff --git a/controller/internal/channelhealth/checker.go b/controller/internal/channelhealth/checker.go index 80d0a53..6088e89 100644 --- a/controller/internal/channelhealth/checker.go +++ b/controller/internal/channelhealth/checker.go @@ -18,6 +18,7 @@ package channelhealth import ( "context" "log" + "os" "strings" "sync" "time" @@ -121,7 +122,8 @@ type Checker struct { mu sync.Mutex 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 + markerPath string // R-271: see WithAlertMarker ("" = no persistence) + 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 // just a live up→down transition; re-armed on recovery / reason-change.) } @@ -131,6 +133,41 @@ func New(probe Probe, sink Sink, logger *log.Logger) *Checker { return &Checker{probe: probe, sink: sink, logger: logger} } +// WithAlertMarker makes ONE fact survive a controller restart: "a down alert went out and no recovery +// has been sent since" (R-271). Without it the operator's trail could never close — the 401 alert's +// own remedy is a re-bootstrap, i.e. a restart, which reset the state to unseeded, and an unseeded→up +// first observation is silent by design. With the marker, that first UP sends the recovery the +// operator is waiting for. The file holds the down state's name only (no secret). "" = disabled. +// Pinned by TestR271_RecoveryAfterRestartIsNotified. +func (c *Checker) WithAlertMarker(path string) *Checker { + c.markerPath = path + return c +} + +func (c *Checker) markAlerted(state string) { + if c.markerPath == "" { + return + } + if err := os.WriteFile(c.markerPath, []byte(state+"\n"), 0o600); err != nil { + c.logger.Printf("[WARN] [channel] could not record the down alert for after a restart (%s): %v — a recovery after a restart will not be reported", c.markerPath, err) + } +} + +// takeAlertMarker reports (and clears) a down alert recorded before this process started. +func (c *Checker) takeAlertMarker() (string, bool) { + if c.markerPath == "" { + return "", false + } + b, err := os.ReadFile(c.markerPath) + if err != nil { + return "", false + } + if rerr := os.Remove(c.markerPath); rerr != nil && !os.IsNotExist(rerr) { + c.logger.Printf("[WARN] [channel] could not clear the down-alert marker %s: %v", c.markerPath, rerr) + } + return strings.TrimSpace(string(b)), true +} + // Check runs one probe cycle: classify, debounce, reflect on the dashboard, and notify on a real // transition. Best-effort + idempotent — safe to call on a timer. Never returns an error to the // scheduler (a probe failure IS the signal, not a job failure). @@ -149,9 +186,16 @@ func (c *Checker) Check(ctx context.Context) error { prev := c.state c.state = "up" c.alerted = false // re-arm for the next down-spell + was, marked := "", false + if prev != "up" { + was, marked = c.takeAlertMarker() // R-271: a down alert from before a restart + } if prev != "" && prev != "up" { c.logger.Printf("[INFO] [channel] agent channel recovered (was %s)", prev) c.sink.NotifyRecovered() + } else if marked { + c.logger.Printf("[INFO] [channel] agent channel recovered (was %s before the controller restarted — the down alert sent then is now closed)", was) + c.sink.NotifyRecovered() } return nil // healthy first-obs / steady-up → no notify } @@ -197,6 +241,7 @@ func (c *Checker) Check(ctx context.Context) error { c.logger.Printf("[WARN] [channel] agent channel DOWN (%s->%s): %v", orUnseeded(prev), newState, perr) c.sink.NotifyDown(cls.reason, cls.eventType, cls.severity, cls.english) c.alerted = true + c.markAlerted(newState) return nil } diff --git a/controller/internal/channelhealth/r271_restart_recovery_test.go b/controller/internal/channelhealth/r271_restart_recovery_test.go new file mode 100644 index 0000000..44065fa --- /dev/null +++ b/controller/internal/channelhealth/r271_restart_recovery_test.go @@ -0,0 +1,69 @@ +package channelhealth + +import ( + "errors" + "io" + "log" + "os" + "path/filepath" + "testing" +) + +// R-271: the 401 alert's remedy is a re-bootstrap (a controller restart), and an unseeded→up first +// observation is silent — so following the instruction guaranteed no recovery event. The consequence +// asserted across a simulated restart: a NEW checker over the same marker file notifies recovery on +// its first UP; a restart with no alert outstanding stays silent (the seeding rule is unchanged). +func TestR271_RecoveryAfterRestartIsNotified(t *testing.T) { + marker := filepath.Join(t.TempDir(), "channelhealth-down-alerted") + lg := log.New(io.Discard, "", 0) + + // Process 1: the channel goes down with a 401 and the alert is sent. + s1 := &fakeSink{} + c1 := New(nil, s1, lg).WithAlertMarker(marker) + run(c1, &scriptedProbe{steps: []struct { + cons bool + err error + }{step(false, nil), step(false, errors.New("agentapi: GET /storage: HTTP 401"))}}) + if len(s1.downs) != 1 { + t.Fatalf("setup: want one down alert, got %d", len(s1.downs)) + } + + // Restart (the remedy). Process 2 sees the channel UP on its first probe. + s2 := &fakeSink{} + c2 := New(nil, s2, lg).WithAlertMarker(marker) + run(c2, &scriptedProbe{steps: []struct { + cons bool + err error + }{step(false, nil), step(false, nil)}}) + if s2.recovered != 1 { + t.Fatalf("R-271: the recovery after a restart must be notified exactly once, got %d", s2.recovered) + } + if _, err := os.Stat(marker); !os.IsNotExist(err) { + t.Errorf("the marker must be cleared once the recovery is sent (stat err=%v)", err) + } + + // Process 3: a restart with nothing outstanding stays silent (first observation seeds). + s3 := &fakeSink{} + c3 := New(nil, s3, lg).WithAlertMarker(marker) + run(c3, &scriptedProbe{steps: []struct { + cons bool + err error + }{step(false, nil)}}) + if s3.recovered != 0 { + t.Errorf("a restart with no alert outstanding must not send a recovery, got %d", s3.recovered) + } + + // A live recovery (no restart) clears the marker too, so a later restart is silent. + s4 := &fakeSink{} + c4 := New(nil, s4, lg).WithAlertMarker(marker) + run(c4, &scriptedProbe{steps: []struct { + cons bool + err error + }{step(false, nil), step(false, errors.New("agentapi: GET /storage: HTTP 401")), step(false, nil)}}) + if s4.recovered != 1 { + t.Fatalf("live recovery: want 1, got %d", s4.recovered) + } + if _, err := os.Stat(marker); !os.IsNotExist(err) { + t.Errorf("a live recovery must clear the marker (stat err=%v)", err) + } +}