From f07b887e65cb0d3069852b7d5f45e2a7617ef520 Mon Sep 17 00:00:00 2001 From: prorochestvo Date: Tue, 4 Aug 2026 15:19:19 +0500 Subject: [PATCH 1/3] test(httpapi): pin the overview window case to a relative since The subtest asserted that a hardcoded ?since=2026-07-01T00:00:00Z was forwarded verbatim, but QueryService floors the window at now-31d, so the case started failing once the wall clock walked past that date. Derive the timestamp from time.Now() instead. Co-Authored-By: Claude Opus 5 --- gateway/httpapi/handlers_test.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/gateway/httpapi/handlers_test.go b/gateway/httpapi/handlers_test.go index 498f4de..53d7b8e 100644 --- a/gateway/httpapi/handlers_test.go +++ b/gateway/httpapi/handlers_test.go @@ -516,15 +516,20 @@ func TestServer_handleOverview(t *testing.T) { srv := newTestServer(&fakeRepo{}, "tok") - rec := doRequest(t, srv, http.MethodGet, "/api/v1/overview?since=2026-07-01T00:00:00Z&bucket=2h", "tok") + // relative to now, not a literal date: QueryService floors ?since= at + // now-31d, so a hardcoded timestamp forwards intact only until the + // wall clock walks past it. + since := time.Now().UTC().Add(-24 * time.Hour).Truncate(time.Second).Format(time.RFC3339) + + rec := doRequest(t, srv, http.MethodGet, "/api/v1/overview?since="+since+"&bucket=2h", "tok") if rec.Code != http.StatusOK { t.Fatalf("status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String()) } var body dto.OverviewResponse decodeJSON(t, rec, &body) - if body.Window.Since != "2026-07-01T00:00:00Z" { - t.Fatalf("Window.Since = %q, want the forwarded since", body.Window.Since) + if body.Window.Since != since { + t.Fatalf("Window.Since = %q, want the forwarded since %q", body.Window.Since, since) } if body.Window.Bucket != "2h0m0s" { t.Fatalf("Window.Bucket = %q, want %q", body.Window.Bucket, "2h0m0s") From 92de7ea2adb55a89dec2fee4716997fc87175305 Mon Sep 17 00:00:00 2001 From: prorochestvo Date: Tue, 4 Aug 2026 15:19:19 +0500 Subject: [PATCH 2/3] feat(check): record the request stage a failed domain probe stalled in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every recorded domain-probe failure reads "context deadline exceeded", which does not say whether name resolution, the TCP connect, or the TLS handshake was what hung — three faults with three different remedies. Attach an httptrace.ClientTrace to the probe request and tag the recorded error with the phase the request died in. Refs: #6 Co-Authored-By: Claude Opus 5 --- infrastructure/check.go | 88 +++++++++++++++++++++++++++++++++++- infrastructure/check_test.go | 73 ++++++++++++++++++++++++++---- 2 files changed, 150 insertions(+), 11 deletions(-) diff --git a/infrastructure/check.go b/infrastructure/check.go index b6bc5b5..bb88902 100644 --- a/infrastructure/check.go +++ b/infrastructure/check.go @@ -2,9 +2,11 @@ package infrastructure import ( "context" + "crypto/tls" "fmt" "net" "net/http" + "net/http/httptrace" "net/url" "sync" "time" @@ -93,7 +95,11 @@ func (c *Checker) Gateway(ctx context.Context) domain.TargetResult { // probeHTTP GETs target within c.timeout and accepts any 2xx status (204 for // generate_204, 200 for cdn-cgi/trace) as success; anything else — including // a redirect, which CheckRedirect stops the client from following — is a -// failure with the response status recorded in Error. +// failure with the response status recorded in Error. A failure is tagged with +// the request stage it stalled in (issue #6): every recorded domain failure to +// date reads "context deadline exceeded", which does not say whether name +// resolution, the TCP connect, or the TLS handshake was what hung — and those +// have different causes and different fixes. func (c *Checker) probeHTTP(ctx context.Context, target string) domain.TargetResult { start := time.Now() @@ -102,10 +108,13 @@ func (c *Checker) probeHTTP(ctx context.Context, target string) domain.TargetRes return domain.TargetResult{Target: target, Kind: domain.CheckKindDomain, OK: false, Latency: time.Since(start), Error: err.Error()} } + stage := newStageTracker() + req = req.WithContext(httptrace.WithClientTrace(req.Context(), stage.trace())) + resp, err := c.httpClient.Do(req) latency := time.Since(start) if err != nil { - return domain.TargetResult{Target: target, Kind: domain.CheckKindDomain, OK: false, Latency: latency, Error: err.Error()} + return domain.TargetResult{Target: target, Kind: domain.CheckKindDomain, OK: false, Latency: latency, Error: stage.annotate(err)} } defer resp.Body.Close() @@ -161,3 +170,78 @@ func hostPortFromURL(rawURL string) (string, error) { return net.JoinHostPort(u.Hostname(), port), nil } + +// stageDNS, stageConnect, stageTLS, stageRequest, stageResponse, and +// stageBody name the phases of one HTTP probe request, in the order a request +// passes through them. +const ( + stageDNS = "dns" + stageConnect = "connect" + stageTLS = "tls" + stageRequest = "request" + stageResponse = "response" + stageBody = "body" +) + +// newStageTracker returns a tracker parked in stageDNS, the first phase a +// fresh request enters. A request served from the connection pool skips +// straight past it via GotConn. +func newStageTracker() *stageTracker { + return &stageTracker{stage: stageDNS} +} + +// stageTracker records how far one HTTP probe request got before it failed +// (issue #6). "context deadline exceeded" alone cannot distinguish a stalled +// resolver from a blackholed TCP connect or a hung TLS handshake, and those +// are different faults with different remedies — the tracker turns the +// recorded error into evidence. Its callbacks fire on whichever goroutine the +// transport is using (parallel dials during Happy Eyeballs included), hence +// the mutex. +type stageTracker struct { + mu sync.Mutex + stage string +} + +// annotate tags err with the stage the request was in when it failed. +func (s *stageTracker) annotate(err error) string { + s.mu.Lock() + defer s.mu.Unlock() + return fmt.Sprintf("%s (stage=%s)", err, s.stage) +} + +// set advances the tracker to stage. +func (s *stageTracker) set(stage string) { + s.mu.Lock() + defer s.mu.Unlock() + s.stage = stage +} + +// trace builds the ClientTrace that drives the tracker. Each hook records the +// phase the request is entering, so a request that dies mid-phase leaves that +// phase behind as its epitaph; the *Done hooks that carry an error leave the +// failed phase in place rather than advancing past it. +func (s *stageTracker) trace() *httptrace.ClientTrace { + return &httptrace.ClientTrace{ + DNSStart: func(httptrace.DNSStartInfo) { s.set(stageDNS) }, + DNSDone: func(info httptrace.DNSDoneInfo) { + if info.Err == nil { + s.set(stageConnect) + } + }, + ConnectStart: func(string, string) { s.set(stageConnect) }, + ConnectDone: func(_, _ string, err error) { + if err == nil { + s.set(stageTLS) + } + }, + TLSHandshakeStart: func() { s.set(stageTLS) }, + TLSHandshakeDone: func(_ tls.ConnectionState, err error) { + if err == nil { + s.set(stageRequest) + } + }, + GotConn: func(httptrace.GotConnInfo) { s.set(stageRequest) }, + WroteRequest: func(httptrace.WroteRequestInfo) { s.set(stageResponse) }, + GotFirstResponseByte: func() { s.set(stageBody) }, + } +} diff --git a/infrastructure/check_test.go b/infrastructure/check_test.go index 150ccc2..cd35fd6 100644 --- a/infrastructure/check_test.go +++ b/infrastructure/check_test.go @@ -5,6 +5,7 @@ import ( "net" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -72,8 +73,8 @@ func TestChecker_Check(t *testing.T) { c := newTestChecker(t, []string{up, down}, []string{domainUp}, time.Second) result := c.Check(t.Context()) - if result.Down { - t.Fatalf("Down = true, want false (only one ip target failed): %+v", result.Targets) + if result.State != domain.StateUp { + t.Fatalf("State = %q, want %q (only one ip target failed): %+v", result.State, domain.StateUp, result.Targets) } }) @@ -87,12 +88,12 @@ func TestChecker_Check(t *testing.T) { c := newTestChecker(t, []string{down1, down2}, []string{domainUp}, time.Second) result := c.Check(t.Context()) - if !result.Down { - t.Fatalf("Down = false, want true (all ip targets failed): %+v", result.Targets) + if !result.IsDown() { + t.Fatalf("State = %q, want %q (all ip targets failed): %+v", result.State, domain.StateDown, result.Targets) } }) - t.Run("ip up but all domains down is a down verdict", func(t *testing.T) { + t.Run("ip up but all domains down is degraded, not down", func(t *testing.T) { t.Parallel() up := newReachableListener(t) @@ -102,8 +103,8 @@ func TestChecker_Check(t *testing.T) { c := newTestChecker(t, []string{up}, []string{domainDown1, domainDown2}, time.Second) result := c.Check(t.Context()) - if !result.Down { - t.Fatalf("Down = false, want true (all domain targets failed): %+v", result.Targets) + if !result.IsDegraded() { + t.Fatalf("State = %q, want %q (all domain targets failed while an ip target answered): %+v", result.State, domain.StateDegraded, result.Targets) } }) @@ -118,8 +119,8 @@ func TestChecker_Check(t *testing.T) { c := newTestChecker(t, []string{up, down}, []string{domainUp, domainDown}, time.Second) result := c.Check(t.Context()) - if result.Down { - t.Fatalf("Down = true, want false: %+v", result.Targets) + if result.State != domain.StateUp { + t.Fatalf("State = %q, want %q: %+v", result.State, domain.StateUp, result.Targets) } }) @@ -209,6 +210,60 @@ func TestChecker_Check(t *testing.T) { } }) + t.Run("a failed domain probe records the request stage it stalled in", func(t *testing.T) { + t.Parallel() + + const timeout = 50 * time.Millisecond + hung := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + select { + case <-r.Context().Done(): + case <-time.After(5 * time.Second): + w.WriteHeader(http.StatusNoContent) + } + })) + t.Cleanup(hung.Close) + + cases := []struct { + name string + target string + wantStage string + }{ + { + // RFC 2606 reserves .invalid, so this name resolves nowhere. + name: "unresolvable host stalls in dns", + target: "http://yarddog-issue-6.invalid/", + wantStage: "stage=" + stageDNS, + }, + { + name: "refused connection stalls in connect", + target: "http://" + newClosedAddr(t) + "/", + wantStage: "stage=" + stageConnect, + }, + { + name: "a server that never answers stalls awaiting the response", + target: hung.URL, + wantStage: "stage=" + stageResponse, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + t.Parallel() + + chk := newTestChecker(t, nil, []string{c.target}, timeout) + result := chk.Check(t.Context()) + + got := result.Targets[0] + if got.OK { + t.Fatalf("OK = true, want false: %+v", got) + } + if !strings.Contains(got.Error, c.wantStage) { + t.Fatalf("Error = %q, want it to carry %q", got.Error, c.wantStage) + } + }) + } + }) + t.Run("an already-expired context fails the probe immediately", func(t *testing.T) { t.Parallel() From 7ffcb1744a5a35e679c0b61b7d7a20f70f5bd9d9 Mon Sep 17 00:00:00 2001 From: prorochestvo Date: Tue, 4 Aug 2026 15:19:31 +0500 Subject: [PATCH 3/3] fix(check): stop rebooting the router on transient probe failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Of 29 "internet down" verdicts recorded in 21 days, 28 had both ip targets connecting in 19-1118ms and ICMP answering 4/4 while only the two HTTPS domain probes timed out at exactly CHECK_TIMEOUT. Those two probes share one DNS path — in production, through the very router being rebooted — so they fail together, and the old rule ("all ip fail OR all domain fail") turned that into 11 router reboots that cut the LAN off for minutes each. Only one verdict in the window was a real link loss, and it lasted a single five-minute sample and healed on its own. Verdict is now graded: down (all ip targets failed) is the only state a reboot may follow; a domain-only wipeout while raw connectivity holds is degraded — recorded, announced once, never rebooted, because four reboots in seven hours on 2026-07-30 did not stop the same probes from stalling. Two guards then sit between a down verdict and a reboot. Within the run, the verdict is re-probed CHECK_CONFIRMATIONS times CHECK_CONFIRM_DELAY apart, and one healthy sample ends the run as unconfirmed. Across runs, the previous REBOOT_MIN_STREAK-1 runs must have seen the outage too, or the run ends as unsustained; an unreadable history withholds the reboot rather than granting it. Neither guard sends a message — the run row and the log carry the record. Degraded and cooldown-skip notices became edge-triggered, which also ends the per-run message storm during a long outage. Refs: #6 Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 34 +++- README.md | 3 + cmd/yarddog/main.go | 17 +- domain/check.go | 53 +++++- domain/check_test.go | 174 ++++++++++-------- domain/run.go | 36 +++- domain/watchdog.go | 6 + infrastructure/config.go | 95 ++++++++++ infrastructure/config_test.go | 86 +++++++++ infrastructure/store.go | 46 +++++ infrastructure/store_test.go | 125 +++++++++++++ services/fakes_test.go | 36 +++- services/orchestrator.go | 170 ++++++++++++++++-- services/orchestrator_test.go | 330 +++++++++++++++++++++++++++++++--- services/ports.go | 30 +++- yarddog.env.example | 14 ++ 16 files changed, 1112 insertions(+), 143 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3b0e7f4..8449527 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -91,9 +91,33 @@ recorded: `8.8.8.8:53`. Bypasses DNS; proves raw link. - **`domain`** — HTTP GET to `generate_204`-style endpoints. Exercises the DNS path too. -The verdict is deliberately conservative: **down ⇔ (all `ip` targets fail) OR (all -`domain` targets fail)**. A single failed target is never a trigger — that guards -against rebooting the router because one remote server hiccuped. +`domain.Verdict` grades one round into three states (issue #6): **`down` ⇔ all `ip` +targets fail** (the only verdict a reboot may follow); **`degraded` ⇔ all `domain` +targets fail while an `ip` target still answers** — the DNS/HTTPS path is broken, the +raw link is not; everything else is `up`. A single failed target within a group is never +a trigger, and an empty group is "not applicable" rather than vacuously failed — except +that with no `ip` targets configured at all, the `domain` group becomes the only evidence +there is and a full failure of it counts as `down`. + +The split is measured, not theoretical: of 29 "down" verdicts recorded in 21 days, 28 had +both `ip` targets connecting in 19-1118 ms and ICMP answering 4/4 while only the two +HTTPS probes timed out — they share one DNS path, in production through the very router +being rebooted. Four reboots in seven hours on 2026-07-30 did not stop them. + +Two further guards sit between a `down` verdict and a reboot, both in +`services/orchestrator.go`: + +- **In-run confirmation** — `confirmDown` re-probes `CHECK_CONFIRMATIONS` times, + `CHECK_CONFIRM_DELAY` apart (persisted under `PhaseConfirm`); one healthy sample ends + the run as `unconfirmed`, correcting the `internet_ok` it wrote from the first sample. +- **Cross-run corroboration** — `outageSustained` requires the previous + `REBOOT_MIN_STREAK - 1` runs to have seen the outage too, via + `RunRepository.RecentRuns`. A run that falls short ends as `unsustained`. Fail-closed: + an unreadable history withholds the reboot rather than granting it. + +Neither guard sends a Telegram message — silence is the point; the `runs` row and a log +line carry the record. `degraded` and cooldown-skip notices are edge-triggered +(`shouldAnnounce`), so a state that lasts hours is announced once, not once per run. ### Data model @@ -186,7 +210,8 @@ prepended. `README.md` holds the authoritative table. Required: `LABEL`, `TELEGRAMBOT_DSN`, `ROUTER_USER`, `ROUTER_PASS`. Optional (with defaults): `ROUTER_ADDR`, `ROUTER_KIND`, -`DB_PATH`, `CHECK_IPS`, `CHECK_DOMAINS`, `CHECK_TIMEOUT`, `RECOVERY_INTERVAL`, +`DB_PATH`, `CHECK_IPS`, `CHECK_DOMAINS`, `CHECK_TIMEOUT`, `CHECK_CONFIRMATIONS`, +`CHECK_CONFIRM_DELAY`, `REBOOT_MIN_STREAK`, `RECOVERY_INTERVAL`, `RECOVERY_TIMEOUT`, `REBOOT_COOLDOWN`, `RETENTION_DAYS`, `REBOOT_ENABLED` (monitor-only when `false`: check/record/notify but never reboot), `METRICS_ENABLED`, `METRICS_TEMPERATURE`, `METRICS_FANS`, `METRICS_CPU`, `METRICS_MEMORY`, `METRICS_DISK`, @@ -220,6 +245,7 @@ response (`writeError500`). The contract has these surfaces: | 3 | reboot request failed (login / `reboot.cgi`) | | 4 | reboot done, internet not restored within the timeout | | 5 | reboot skipped due to cooldown | + | 6 | a failure was observed but deliberately not acted on (`degraded`, `unsustained`) | - **Daemon exit codes** — its own disjoint contract: `0` clean shutdown (SIGINT/SIGTERM, drained), `1` configuration error (missing `DAEMON_TOKEN`, unparseable value), `2` the diff --git a/README.md b/README.md index a4e9a50..aacecc4 100644 --- a/README.md +++ b/README.md @@ -13,6 +13,9 @@ API and status dashboard over the LAN. - checks connectivity against independent IP and domain targets - soft mode (default) reboots only when the uplink is actually down; hard mode (`--hard-reboot`) reboots unconditionally +- a down verdict must survive re-probing within the run and be corroborated by the previous + run before anything reboots; a failure of the domain targets alone, while raw IP + connectivity holds, is reported as degraded and never reboots - monitor-only mode (`YARDDOG_REBOOT_ENABLED=false`): watch and record, never reboot - records every run, check and recovery phase in SQLite, with optional per-run host-telemetry and ping metrics diff --git a/cmd/yarddog/main.go b/cmd/yarddog/main.go index e591d9a..3ff8af8 100644 --- a/cmd/yarddog/main.go +++ b/cmd/yarddog/main.go @@ -128,13 +128,16 @@ func run() int { } settings := services.Settings{ - Label: cfg.Label, - RebootCooldown: cfg.RebootCooldown, - RecoveryInterval: cfg.RecoveryInterval, - RecoveryTimeout: cfg.RecoveryTimeout, - RebootEnabled: cfg.RebootEnabled, - MetricsEnabled: cfg.MetricsEnabled, - PingEnabled: len(cfg.PingHosts) > 0, + Label: cfg.Label, + RebootCooldown: cfg.RebootCooldown, + RecoveryInterval: cfg.RecoveryInterval, + RecoveryTimeout: cfg.RecoveryTimeout, + CheckConfirmations: cfg.CheckConfirmations, + CheckConfirmDelay: cfg.CheckConfirmDelay, + RebootMinStreak: cfg.RebootMinStreak, + RebootEnabled: cfg.RebootEnabled, + MetricsEnabled: cfg.MetricsEnabled, + PingEnabled: len(cfg.PingHosts) > 0, } code := services.Execute(ctx, settings, st, st, st, chk, rb, nt, mc, pc, clk, mode) diff --git a/domain/check.go b/domain/check.go index e38ce0e..238aed7 100644 --- a/domain/check.go +++ b/domain/check.go @@ -10,9 +10,31 @@ const ( CheckKindGateway = "gateway" ) -// Verdict applies the conservative down rule (design §4.2): the internet is down when -// every ip target fails OR every domain target fails. A single failed target within a -// group never trips it; an empty group is "not applicable", never vacuously down. +// StateUp, StateDegraded, and StateDown are the tri-state verdict Verdict +// produces (issue #6). The two failure states are deliberately not the same +// event: StateDown means the raw link is gone and a router reboot is on the +// table, while StateDegraded means only the name-resolution/HTTPS path broke +// while raw connectivity provably held — recorded and announced, never +// rebooted. +const ( + StateUp = "up" + StateDegraded = "degraded" + StateDown = "down" +) + +// Verdict grades one round of probes into StateUp, StateDegraded, or StateDown +// (issue #6, superseding design §4.2's single "all ip OR all domain failed" +// rule). Only a fully failed ip group — raw TCP dials to IP literals, which +// need no DNS — is StateDown. A fully failed domain group while any ip target +// still connects is StateDegraded: both domain probes are HTTPS GETs sharing +// one DNS path (in production, through the very router this watchdog reboots), +// so they fail together as a matter of course, and 28 of 29 historical "down" +// verdicts were exactly that with the ip group answering in 19-1118ms. +// +// When no ip targets are configured at all, the domain group is the only +// evidence there is and a fully failed one is StateDown rather than a verdict +// nothing could ever act on. A single failed target within a group never trips +// anything; an empty group is "not applicable", never vacuously failed. func Verdict(targets []TargetResult) Result { var ip, dom []TargetResult for _, t := range targets { @@ -23,16 +45,35 @@ func Verdict(targets []TargetResult) Result { dom = append(dom, t) } } - return Result{Down: allFailed(ip) || allFailed(dom), Targets: targets} + + state := StateUp + switch { + case allFailed(ip): + state = StateDown + case allFailed(dom) && len(ip) == 0: + state = StateDown + case allFailed(dom): + state = StateDegraded + } + + return Result{State: state, Targets: targets} } // Result is the outcome of one connectivity check: every probed target's -// individual result plus the aggregate quorum verdict (design §4.2). +// individual result plus the aggregate verdict (design §4.2, issue #6). type Result struct { - Down bool + State string Targets []TargetResult } +// IsDegraded reports whether the application-layer path is broken while raw +// connectivity holds — worth recording and announcing, never worth a reboot. +func (r Result) IsDegraded() bool { return r.State == StateDegraded } + +// IsDown reports whether the raw link is gone, the only verdict a reboot may +// ever follow from. +func (r Result) IsDown() bool { return r.State == StateDown } + // TargetResult is one probed target's outcome, shaped to map directly onto a // checks row (design §9) once the caller supplies RunID and Phase. type TargetResult struct { diff --git a/domain/check_test.go b/domain/check_test.go index db9763f..9776e7d 100644 --- a/domain/check_test.go +++ b/domain/check_test.go @@ -5,80 +5,106 @@ import "testing" func TestVerdict(t *testing.T) { t.Parallel() - t.Run("one down target among several is not a down verdict", func(t *testing.T) { - t.Parallel() - - targets := []TargetResult{ - {Target: "1.1.1.1:443", Kind: CheckKindIP, OK: true}, - {Target: "8.8.8.8:53", Kind: CheckKindIP, OK: false}, - {Target: "https://example.com/generate_204", Kind: CheckKindDomain, OK: true}, - } - - got := Verdict(targets) - - if got.Down { - t.Fatalf("Down = true, want false (only one ip target failed): %+v", got.Targets) - } - }) - - t.Run("all ip targets down is a down verdict regardless of domains", func(t *testing.T) { - t.Parallel() - - targets := []TargetResult{ - {Target: "1.1.1.1:443", Kind: CheckKindIP, OK: false}, - {Target: "8.8.8.8:53", Kind: CheckKindIP, OK: false}, - {Target: "https://example.com/generate_204", Kind: CheckKindDomain, OK: true}, - } - - got := Verdict(targets) - - if !got.Down { - t.Fatalf("Down = false, want true (all ip targets failed): %+v", got.Targets) - } - }) - - t.Run("ip up but all domains down is a down verdict", func(t *testing.T) { - t.Parallel() - - targets := []TargetResult{ - {Target: "1.1.1.1:443", Kind: CheckKindIP, OK: true}, - {Target: "https://a.example/", Kind: CheckKindDomain, OK: false}, - {Target: "https://b.example/", Kind: CheckKindDomain, OK: false}, - } - - got := Verdict(targets) - - if !got.Down { - t.Fatalf("Down = false, want true (all domain targets failed): %+v", got.Targets) - } - }) - - t.Run("at least one ip and one domain up is an up verdict", func(t *testing.T) { - t.Parallel() - - targets := []TargetResult{ - {Target: "1.1.1.1:443", Kind: CheckKindIP, OK: true}, - {Target: "8.8.8.8:53", Kind: CheckKindIP, OK: false}, - {Target: "https://a.example/", Kind: CheckKindDomain, OK: true}, - {Target: "https://b.example/", Kind: CheckKindDomain, OK: false}, - } - - got := Verdict(targets) - - if got.Down { - t.Fatalf("Down = true, want false: %+v", got.Targets) - } - }) - - t.Run("empty group is not applicable, never vacuously down", func(t *testing.T) { - t.Parallel() - - got := Verdict(nil) - - if got.Down { - t.Fatal("Down = true, want false: an empty target list must never be a vacuous down verdict") - } - }) + cases := []struct { + name string + targets []TargetResult + want string + }{ + { + name: "one down target among several is not a failure verdict", + targets: []TargetResult{ + {Target: "1.1.1.1:443", Kind: CheckKindIP, OK: true}, + {Target: "8.8.8.8:53", Kind: CheckKindIP, OK: false}, + {Target: "https://example.com/generate_204", Kind: CheckKindDomain, OK: true}, + }, + want: StateUp, + }, + { + name: "all ip targets down is a down verdict regardless of domains", + targets: []TargetResult{ + {Target: "1.1.1.1:443", Kind: CheckKindIP, OK: false}, + {Target: "8.8.8.8:53", Kind: CheckKindIP, OK: false}, + {Target: "https://example.com/generate_204", Kind: CheckKindDomain, OK: true}, + }, + want: StateDown, + }, + { + name: "every target down is a down verdict", + targets: []TargetResult{ + {Target: "1.1.1.1:443", Kind: CheckKindIP, OK: false}, + {Target: "8.8.8.8:53", Kind: CheckKindIP, OK: false}, + {Target: "https://a.example/", Kind: CheckKindDomain, OK: false}, + }, + want: StateDown, + }, + { + name: "ip up but all domains down is degraded, not down (issue #6)", + targets: []TargetResult{ + {Target: "1.1.1.1:443", Kind: CheckKindIP, OK: true}, + {Target: "https://a.example/", Kind: CheckKindDomain, OK: false}, + {Target: "https://b.example/", Kind: CheckKindDomain, OK: false}, + }, + want: StateDegraded, + }, + { + name: "one ip up among failures still keeps a domain wipeout at degraded", + targets: []TargetResult{ + {Target: "1.1.1.1:443", Kind: CheckKindIP, OK: false}, + {Target: "8.8.8.8:53", Kind: CheckKindIP, OK: true}, + {Target: "https://a.example/", Kind: CheckKindDomain, OK: false}, + {Target: "https://b.example/", Kind: CheckKindDomain, OK: false}, + }, + want: StateDegraded, + }, + { + name: "with no ip targets configured, all domains down is the only evidence there is", + targets: []TargetResult{ + {Target: "https://a.example/", Kind: CheckKindDomain, OK: false}, + {Target: "https://b.example/", Kind: CheckKindDomain, OK: false}, + }, + want: StateDown, + }, + { + name: "at least one ip and one domain up is an up verdict", + targets: []TargetResult{ + {Target: "1.1.1.1:443", Kind: CheckKindIP, OK: true}, + {Target: "8.8.8.8:53", Kind: CheckKindIP, OK: false}, + {Target: "https://a.example/", Kind: CheckKindDomain, OK: true}, + {Target: "https://b.example/", Kind: CheckKindDomain, OK: false}, + }, + want: StateUp, + }, + { + name: "empty group is not applicable, never vacuously down", + targets: nil, + want: StateUp, + }, + { + name: "a gateway probe alone decides nothing", + targets: []TargetResult{ + {Target: "192.168.1.1:80", Kind: CheckKindGateway, OK: false}, + }, + want: StateUp, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + t.Parallel() + + got := Verdict(c.targets) + + if got.State != c.want { + t.Fatalf("State = %q, want %q: %+v", got.State, c.want, got.Targets) + } + if got.IsDown() != (c.want == StateDown) { + t.Fatalf("IsDown() = %v, want %v", got.IsDown(), c.want == StateDown) + } + if got.IsDegraded() != (c.want == StateDegraded) { + t.Fatalf("IsDegraded() = %v, want %v", got.IsDegraded(), c.want == StateDegraded) + } + }) + } t.Run("targets slice is carried through unmodified", func(t *testing.T) { t.Parallel() diff --git a/domain/run.go b/domain/run.go index 8d9ec3f..13f2a80 100644 --- a/domain/run.go +++ b/domain/run.go @@ -20,22 +20,36 @@ const ( ActionSkippedDisabled = "skipped_disabled" ) -// PhaseInitial and PhaseRecovery are the values allowed in checks.phase -// (design §9). +// PhaseInitial, PhaseConfirm, and PhaseRecovery are the values allowed in +// checks.phase (design §9; PhaseConfirm from issue #6's re-check of a down +// verdict). const ( PhaseInitial = "initial" + PhaseConfirm = "confirm" PhaseRecovery = "recovery" ) // OutcomeOK, OutcomeRebootFailed, OutcomeTimeout, OutcomeSkipped, and // OutcomeRebootDisabled are the values written to runs.outcome (design §9; // plans/003-host-telemetry.md for OutcomeRebootDisabled). +// +// OutcomeDegraded, OutcomeUnconfirmed, and OutcomeUnsustained (issue #6) name +// the three ways a run can see a failure and still, correctly, not reboot: +// the failure was application-layer only (StateDegraded), a down verdict did +// not survive re-probing within the run, or a confirmed outage was not yet +// corroborated by the preceding runs. They are distinct values because an +// operator reads them differently — degraded means "something really is +// broken, just not the link", the other two mean "nothing was broken long +// enough to act on". const ( OutcomeOK = "ok" OutcomeRebootFailed = "reboot_failed" OutcomeTimeout = "timeout" OutcomeSkipped = "skipped" OutcomeRebootDisabled = "reboot_disabled" + OutcomeDegraded = "degraded" + OutcomeUnconfirmed = "unconfirmed" + OutcomeUnsustained = "unsustained" ) // Run is one row of the runs table (design §9), as returned by @@ -43,7 +57,10 @@ const ( // lexicographic and chronological order coincide) rather than an // autoincrement integer. InternetOK is nil for a hard-mode run, which // performs no initial check; every *At field is nil until its phase -// transition has happened. +// transition has happened. Since issue #6 InternetOK tracks StateDown +// specifically — a degraded run (raw link up, domain probes failing) records +// true and says so through its outcome — so it reads as "was the link itself +// gone", the one question the reboot decision turns on. type Run struct { ID string StartedAt time.Time @@ -72,11 +89,24 @@ type Check struct { Error string } +// RunSummary is the slice of a past run the reboot guards read back (issue +// #6): whether that run saw the link as down, and how it ended. Deliberately +// narrower than Run — the guards corroborate a present outage against recent +// history and must not grow a dependency on every column of it. +type RunSummary struct { + InternetOK *bool + Outcome string +} + // RunUpdate carries the phase timestamps, action, outcome, and error to // apply to a runs row. Every field is a pointer so RunRepository.UpdateRun // can tell "leave unchanged" (nil) apart from "set this value". Its SQL // rendering is a persistence detail that stays in infrastructure, not here. +// InternetOK exists so a run whose down verdict is later cleared by +// re-probing (issue #6) can correct the column it wrote from its first, +// mistaken sample. type RunUpdate struct { + InternetOK *bool Action *string RebootStartedAt *time.Time RouterDownAt *time.Time diff --git a/domain/watchdog.go b/domain/watchdog.go index b1b81de..ebdd266 100644 --- a/domain/watchdog.go +++ b/domain/watchdog.go @@ -12,6 +12,11 @@ import ( // ExitConfigError and ExitLockHeld are produced by main itself before Execute // is ever called (a bad .env, or a lock already held) and are declared here // only so every code in the design's table has one canonical home. +// ExitNoAction (issue #6) reports the state the original table had no honest +// code for: the run observed a real failure — a degraded application path, or +// an outage that did not survive the confirmation guards — and deliberately +// took no action. It is not ExitOK, which claims nothing was wrong, and not +// ExitSkippedCooldown, which means a reboot was due and suppressed. const ( ExitOK = 0 ExitConfigError = 1 @@ -19,6 +24,7 @@ const ( ExitRebootFailed = 3 ExitRecoveryTimeout = 4 ExitSkippedCooldown = 5 + ExitNoAction = 6 ) // ReasonNoInternet and ReasonScheduledHardReboot are the two reasons the diff --git a/infrastructure/config.go b/infrastructure/config.go index 9e477ac..2228533 100644 --- a/infrastructure/config.go +++ b/infrastructure/config.go @@ -60,6 +60,7 @@ func LoadConfig(envPath string) (*Config, error) { {"RECOVERY_INTERVAL", &cfg.RecoveryInterval, defaultRecoveryInterval}, {"RECOVERY_TIMEOUT", &cfg.RecoveryTimeout, defaultRecoveryTimeout}, {"REBOOT_COOLDOWN", &cfg.RebootCooldown, defaultRebootCooldown}, + {"CHECK_CONFIRM_DELAY", &cfg.CheckConfirmDelay, defaultCheckConfirmDelay}, } for _, d := range durations { raw := getOrDefault(get, d.key, d.def) @@ -69,6 +70,24 @@ func LoadConfig(envPath string) (*Config, error) { } *d.dst = parsed } + cfg.CheckConfirmDelay = clampCheckConfirmDelay(cfg.CheckConfirmDelay) + + // CheckConfirmations and RebootMinStreak are the two issue #6 guards + // between a down verdict and a reboot; both are sample counts, and both + // clamp rather than fail startup, matching PING_COUNT's philosophy. + rawConfirmations := getOrDefault(get, "CHECK_CONFIRMATIONS", defaultCheckConfirmations) + confirmations, err := strconv.Atoi(rawConfirmations) + if err != nil { + return nil, fmt.Errorf("invalid %s %q: %w", envPrefix+"CHECK_CONFIRMATIONS", rawConfirmations, err) + } + cfg.CheckConfirmations = clampCheckConfirmations(confirmations) + + rawMinStreak := getOrDefault(get, "REBOOT_MIN_STREAK", defaultRebootMinStreak) + minStreak, err := strconv.Atoi(rawMinStreak) + if err != nil { + return nil, fmt.Errorf("invalid %s %q: %w", envPrefix+"REBOOT_MIN_STREAK", rawMinStreak, err) + } + cfg.RebootMinStreak = clampRebootMinStreak(minStreak) // RetentionDays now governs the *archive* tier only (issue #4): a run // stays hot for HotWindowDays regardless of this value, and @@ -165,6 +184,13 @@ type Config struct { PingHosts []string PingCount int PingTimeout time.Duration + + // CheckConfirmations, CheckConfirmDelay, and RebootMinStreak are issue + // #6's false-positive guards; see services.Settings for what each one + // buys. + CheckConfirmations int + CheckConfirmDelay time.Duration + RebootMinStreak int } const ( @@ -183,6 +209,17 @@ const ( defaultPingCount = "5" defaultPingTimeout = "4s" + // defaultCheckConfirmations and defaultCheckConfirmDelay re-probe a down + // verdict twice, 20s apart, so a reboot decision rests on ~45s of evidence + // instead of one 5s sample and still lands well inside a 5-minute cron + // slot. defaultRebootMinStreak then requires the previous run to have seen + // the outage too (issue #6): in 21 days of recorded history exactly one + // down verdict had a down predecessor, and 11 reboots fired on isolated + // samples with the ip targets answering normally throughout. + defaultCheckConfirmations = "2" + defaultCheckConfirmDelay = "20s" + defaultRebootMinStreak = "2" + // minHotWindowDays floors HOT_WINDOW_DAYS (issue #4): there is no // "disable roll-over" mode — unbounded hot growth is the bug this knob // fixes — so the floor is 1, not 0. maxHotWindowDays and maxRetentionDays @@ -214,6 +251,28 @@ const ( // every run, making the operator's setting a no-op. minPingTimeout = 1 * time.Second maxPingTimeout = 10 * time.Second + + // minCheckConfirmations bounds CHECK_CONFIRMATIONS at 0 — "act on the + // first sample", the pre-issue-#6 behaviour, which stays available to an + // operator who wants it — and the ceiling keeps the whole confirmation + // window (attempts x delay, plus each attempt's own CHECK_TIMEOUT) inside + // a single cron slot, so a run can never still be confirming when the next + // one starts and hits the flock. + minCheckConfirmations = 0 + maxCheckConfirmations = 5 + + // minCheckConfirmDelay and maxCheckConfirmDelay bound the gap between + // re-probes: below a second the "re-check" would sample the same stalled + // moment it just failed on, and above a minute the guard would outlast the + // cron cadence it is nested inside. + minCheckConfirmDelay = 1 * time.Second + maxCheckConfirmDelay = 60 * time.Second + + // minRebootMinStreak bounds REBOOT_MIN_STREAK at 1 ("this run alone is + // enough", no cross-run corroboration). The ceiling stops a typo from + // demanding a streak so long the watchdog would never reboot at all. + minRebootMinStreak = 1 + maxRebootMinStreak = 10 ) // clampHotWindowDays floors HOT_WINDOW_DAYS at minHotWindowDays (1) rather @@ -244,6 +303,42 @@ func clampRetentionDays(n int) int { return n } +// clampCheckConfirmations bounds CHECK_CONFIRMATIONS to [minCheckConfirmations, +// maxCheckConfirmations], clamping rather than refusing to start (issue #6). +func clampCheckConfirmations(n int) int { + if n < minCheckConfirmations { + return minCheckConfirmations + } + if n > maxCheckConfirmations { + return maxCheckConfirmations + } + return n +} + +// clampCheckConfirmDelay bounds CHECK_CONFIRM_DELAY to [minCheckConfirmDelay, +// maxCheckConfirmDelay] (issue #6). +func clampCheckConfirmDelay(d time.Duration) time.Duration { + if d < minCheckConfirmDelay { + return minCheckConfirmDelay + } + if d > maxCheckConfirmDelay { + return maxCheckConfirmDelay + } + return d +} + +// clampRebootMinStreak bounds REBOOT_MIN_STREAK to [minRebootMinStreak, +// maxRebootMinStreak] (issue #6). +func clampRebootMinStreak(n int) int { + if n < minRebootMinStreak { + return minRebootMinStreak + } + if n > maxRebootMinStreak { + return maxRebootMinStreak + } + return n +} + // clampPingCount bounds PING_COUNT to [minPingCount, maxPingCount], silently // correcting an out-of-range operator value rather than failing startup over // it (matching QueryService.clampLimit's "clamp, don't error" philosophy for diff --git a/infrastructure/config_test.go b/infrastructure/config_test.go index 8828501..1bb064f 100644 --- a/infrastructure/config_test.go +++ b/infrastructure/config_test.go @@ -396,6 +396,92 @@ func TestLoadConfig(t *testing.T) { } }) + t.Run("issue #6 guards default to two re-checks 20s apart and a two-run streak", func(t *testing.T) { + t.Parallel() + + path := writeConfigFixture(t, requiredOnlyEnv()) + + cfg, err := LoadConfig(path) + if err != nil { + t.Fatalf("LoadConfig() error = %v", err) + } + if cfg.CheckConfirmations != 2 { + t.Errorf("CheckConfirmations = %d, want 2", cfg.CheckConfirmations) + } + if cfg.CheckConfirmDelay != 20*time.Second { + t.Errorf("CheckConfirmDelay = %v, want 20s", cfg.CheckConfirmDelay) + } + if cfg.RebootMinStreak != 2 { + t.Errorf("RebootMinStreak = %d, want 2", cfg.RebootMinStreak) + } + }) + + t.Run("issue #6 guard values are clamped, never fatal", func(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + env map[string]string + wantConfirmations int + wantConfirmDelay time.Duration + wantRebootMinStrek int + }{ + { + name: "below the floor", + env: map[string]string{"CHECK_CONFIRMATIONS": "-1", "CHECK_CONFIRM_DELAY": "10ms", "REBOOT_MIN_STREAK": "0"}, + wantConfirmations: minCheckConfirmations, + wantConfirmDelay: minCheckConfirmDelay, + wantRebootMinStrek: minRebootMinStreak, + }, + { + name: "above the ceiling", + env: map[string]string{"CHECK_CONFIRMATIONS": "99", "CHECK_CONFIRM_DELAY": "10m", "REBOOT_MIN_STREAK": "99"}, + wantConfirmations: maxCheckConfirmations, + wantConfirmDelay: maxCheckConfirmDelay, + wantRebootMinStrek: maxRebootMinStreak, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + t.Parallel() + + env := requiredOnlyEnv() + for k, v := range c.env { + env[k] = v + } + path := writeConfigFixture(t, env) + + cfg, err := LoadConfig(path) + if err != nil { + t.Fatalf("LoadConfig() error = %v", err) + } + if cfg.CheckConfirmations != c.wantConfirmations { + t.Errorf("CheckConfirmations = %d, want %d", cfg.CheckConfirmations, c.wantConfirmations) + } + if cfg.CheckConfirmDelay != c.wantConfirmDelay { + t.Errorf("CheckConfirmDelay = %v, want %v", cfg.CheckConfirmDelay, c.wantConfirmDelay) + } + if cfg.RebootMinStreak != c.wantRebootMinStrek { + t.Errorf("RebootMinStreak = %d, want %d", cfg.RebootMinStreak, c.wantRebootMinStrek) + } + }) + } + }) + + t.Run("non-integer CHECK_CONFIRMATIONS returns an error", func(t *testing.T) { + t.Parallel() + + env := requiredOnlyEnv() + env["CHECK_CONFIRMATIONS"] = "twice" + path := writeConfigFixture(t, env) + + _, err := LoadConfig(path) + if err == nil { + t.Fatal("LoadConfig() error = nil, want error for non-integer CHECK_CONFIRMATIONS") + } + }) + t.Run("PING_COUNT below the floor is clamped to 4", func(t *testing.T) { t.Parallel() diff --git a/infrastructure/store.go b/infrastructure/store.go index 5c311f5..3efa9a5 100644 --- a/infrastructure/store.go +++ b/infrastructure/store.go @@ -989,6 +989,49 @@ func (s *Store) PruneArchive(ctx context.Context, now time.Time, retentionDays i return pruned, nil } +// RecentRuns returns the hot runs immediately preceding beforeID, newest +// first, at most limit of them (issue #6). Ordering and the "preceding" +// predicate both ride on the UUIDv7 primary key (issue #4), which is +// time-ordered, so no secondary index on started_at is needed and the caller's +// own row is excluded by a plain id comparison. It reads the hot table only: +// the guards corroborate against the last few minutes, never against history +// old enough to have rolled into the archive. +func (s *Store) RecentRuns(ctx context.Context, beforeID string, limit int) ([]domain.RunSummary, error) { + rows, err := s.db.QueryContext(ctx, + fmt.Sprintf(`SELECT %s, %s FROM %s WHERE %s < ? ORDER BY %s DESC LIMIT ?`, + colRunsInternetOK, colRunsOutcome, tableRuns, colRunsID, colRunsID), + beforeID, limit, + ) + if err != nil { + return nil, fmt.Errorf("recent runs before %s: %w", beforeID, err) + } + defer rows.Close() + + var out []domain.RunSummary + for rows.Next() { + var ( + internetOK sql.NullBool + outcome sql.NullString + ) + if err := rows.Scan(&internetOK, &outcome); err != nil { + return nil, fmt.Errorf("recent runs before %s: %w", beforeID, err) + } + + var summary domain.RunSummary + if internetOK.Valid { + ok := internetOK.Bool + summary.InternetOK = &ok + } + summary.Outcome = outcome.String + out = append(out, summary) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("recent runs before %s: %w", beforeID, err) + } + + return out, nil +} + // RolloverToArchive moves every run (and all its children) with // started_at older than now - hotWindowDays days from the hot tables into // their *_archive twins, in one transaction (issue #4): this run-boundary @@ -1888,6 +1931,9 @@ func runUpdateAssignments(u domain.RunUpdate) ([]string, []any) { args = append(args, v) } + if u.InternetOK != nil { + add(colRunsInternetOK, boolToInt(*u.InternetOK)) + } if u.Action != nil { add(colRunsAction, *u.Action) } diff --git a/infrastructure/store_test.go b/infrastructure/store_test.go index 529b88a..802706e 100644 --- a/infrastructure/store_test.go +++ b/infrastructure/store_test.go @@ -1674,6 +1674,107 @@ func TestStore_ListRuns(t *testing.T) { }) } +func TestStore_RecentRuns(t *testing.T) { + t.Run("empty store returns an empty slice, not an error", func(t *testing.T) { + t.Parallel() + + s := newTestStore(t) + + got, err := s.RecentRuns(t.Context(), "ffffffff-ffff-ffff-ffff-ffffffffffff", 3) + if err != nil { + t.Fatalf("RecentRuns() error = %v", err) + } + if len(got) != 0 { + t.Fatalf("RecentRuns() = %d rows, want 0 on an empty store", len(got)) + } + }) + + t.Run("returns the runs before the given id, newest first, honouring limit", func(t *testing.T) { + t.Parallel() + + s := newTestStore(t) + down, up := false, true + states := []*bool{&up, &down, &down, &up, nil} + + var ids []string + for _, state := range states { + id, err := s.InsertRun(t.Context(), time.Now(), domain.ModeSoft, state) + if err != nil { + t.Fatalf("InsertRun() error = %v", err) + } + ids = append(ids, id) + } + + // asking from the newest run's own id must never hand it back its + // own row: the guards ask "what did the runs before me see?". + got, err := s.RecentRuns(t.Context(), ids[4], 2) + if err != nil { + t.Fatalf("RecentRuns() error = %v", err) + } + if len(got) != 2 { + t.Fatalf("RecentRuns() = %d rows, want 2 (limit honoured)", len(got)) + } + if got[0].InternetOK == nil || !*got[0].InternetOK { + t.Fatalf("RecentRuns()[0].InternetOK = %v, want true (the run immediately before)", got[0].InternetOK) + } + if got[1].InternetOK == nil || *got[1].InternetOK { + t.Fatalf("RecentRuns()[1].InternetOK = %v, want false", got[1].InternetOK) + } + }) + + t.Run("a hard run's NULL internet_ok reads back as nil, not false", func(t *testing.T) { + t.Parallel() + + s := newTestStore(t) + + if _, err := s.InsertRun(t.Context(), time.Now(), domain.ModeHard, nil); err != nil { + t.Fatalf("InsertRun() error = %v", err) + } + newest, err := s.InsertRun(t.Context(), time.Now(), domain.ModeSoft, nil) + if err != nil { + t.Fatalf("InsertRun() error = %v", err) + } + + got, err := s.RecentRuns(t.Context(), newest, 1) + if err != nil { + t.Fatalf("RecentRuns() error = %v", err) + } + if len(got) != 1 { + t.Fatalf("RecentRuns() = %d rows, want 1", len(got)) + } + if got[0].InternetOK != nil { + t.Fatalf("InternetOK = %v, want nil (a hard run never probed)", *got[0].InternetOK) + } + }) + + t.Run("carries the outcome the run ended on", func(t *testing.T) { + t.Parallel() + + s := newTestStore(t) + + first, err := s.InsertRun(t.Context(), time.Now(), domain.ModeSoft, nil) + if err != nil { + t.Fatalf("InsertRun() error = %v", err) + } + outcome := domain.OutcomeDegraded + if err := s.UpdateRun(t.Context(), first, domain.RunUpdate{Outcome: &outcome}); err != nil { + t.Fatalf("UpdateRun() error = %v", err) + } + newest, err := s.InsertRun(t.Context(), time.Now(), domain.ModeSoft, nil) + if err != nil { + t.Fatalf("InsertRun() error = %v", err) + } + + got, err := s.RecentRuns(t.Context(), newest, 1) + if err != nil { + t.Fatalf("RecentRuns() error = %v", err) + } + if len(got) != 1 || got[0].Outcome != domain.OutcomeDegraded { + t.Fatalf("RecentRuns() = %+v, want one row carrying outcome %q", got, domain.OutcomeDegraded) + } + }) +} + func TestStore_ListUnsentOutboxMessages(t *testing.T) { t.Run("orders oldest first and excludes sent rows", func(t *testing.T) { t.Parallel() @@ -2625,6 +2726,30 @@ func TestStore_RunByID(t *testing.T) { } func TestStore_UpdateRun(t *testing.T) { + t.Run("corrects internet_ok written from a mistaken first sample", func(t *testing.T) { + t.Parallel() + + s := newTestStore(t) + down := false + id, err := s.InsertRun(t.Context(), time.Now(), domain.ModeSoft, &down) + if err != nil { + t.Fatalf("InsertRun() error = %v", err) + } + + up := true + if err := s.UpdateRun(t.Context(), id, domain.RunUpdate{InternetOK: &up}); err != nil { + t.Fatalf("UpdateRun() error = %v", err) + } + + run, err := s.GetRun(t.Context(), id) + if err != nil { + t.Fatalf("GetRun() error = %v", err) + } + if run.InternetOK == nil || !*run.InternetOK { + t.Fatalf("InternetOK = %v, want true (issue #6: the re-check cleared the down verdict)", run.InternetOK) + } + }) + t.Run("applies only the non-nil fields", func(t *testing.T) { t.Parallel() diff --git a/services/fakes_test.go b/services/fakes_test.go index 2fa86b8..b96b199 100644 --- a/services/fakes_test.go +++ b/services/fakes_test.go @@ -115,11 +115,15 @@ func newFakeRunRepo() *fakeRunRepo { // real against SQLite in infrastructure/store_test.go. type fakeRunRepo struct { runs map[string]*domain.Run + checks []domain.Check nextID int64 lastRebootStartedAt time.Time lastRebootOK bool lastRebootErr error + + recentRuns []domain.RunSummary + recentRunsErr error } func (f *fakeRunRepo) GetLastRebootStartedAt(context.Context) (time.Time, bool, error) { @@ -129,10 +133,22 @@ func (f *fakeRunRepo) GetLastRebootStartedAt(context.Context) (time.Time, bool, return f.lastRebootStartedAt, f.lastRebootOK, nil } -func (f *fakeRunRepo) InsertCheck(_ context.Context, _ domain.Check) error { +func (f *fakeRunRepo) InsertCheck(_ context.Context, c domain.Check) error { + f.checks = append(f.checks, c) return nil } +// phases returns the phase of every persisted check in insert order, so a +// test can assert what a run actually recorded (issue #6: the confirmation +// re-probes must land in the record as domain.PhaseConfirm, not vanish). +func (f *fakeRunRepo) phases() []string { + out := make([]string, 0, len(f.checks)) + for _, c := range f.checks { + out = append(out, c.Phase) + } + return out +} + func (f *fakeRunRepo) InsertRun(_ context.Context, startedAt time.Time, mode string, internetOK *bool) (string, error) { f.nextID++ id := strconv.FormatInt(f.nextID, 10) @@ -140,6 +156,21 @@ func (f *fakeRunRepo) InsertRun(_ context.Context, startedAt time.Time, mode str return id, nil } +// RecentRuns serves the canned recentRuns slice (newest first), truncated to +// limit, rather than deriving history from inserted rows — the same "dumb and +// settable" stance as GetLastRebootStartedAt, so a guard test scripts exactly +// the history it wants to exercise. The real id-ordered query is exercised +// against SQLite in infrastructure/store_test.go. +func (f *fakeRunRepo) RecentRuns(_ context.Context, _ string, limit int) ([]domain.RunSummary, error) { + if f.recentRunsErr != nil { + return nil, f.recentRunsErr + } + if limit < len(f.recentRuns) { + return f.recentRuns[:limit], nil + } + return f.recentRuns, nil +} + // run returns a copy of the in-memory row id for readback assertions, // failing the test if it was never inserted. func (f *fakeRunRepo) run(t *testing.T, id string) domain.Run { @@ -158,6 +189,9 @@ func (f *fakeRunRepo) UpdateRun(_ context.Context, id string, u domain.RunUpdate return fmt.Errorf("fakeRunRepo: no run %s", id) } + if u.InternetOK != nil { + run.InternetOK = u.InternetOK + } if u.Action != nil { run.Action = *u.Action } diff --git a/services/orchestrator.go b/services/orchestrator.go index e2b0d26..0148118 100644 --- a/services/orchestrator.go +++ b/services/orchestrator.go @@ -155,6 +155,79 @@ func (r *runner) collectTelemetry(ctx context.Context, runID string) { wg.Wait() } +// confirmDown re-probes a down verdict CHECK_CONFIRMATIONS more times, +// CHECK_CONFIRM_DELAY apart, and returns the last result (issue #6): a single +// five-second sample must never cost the whole LAN a router reboot. It returns +// early on the first sample that is no longer down, and every attempt is +// persisted under domain.PhaseConfirm so the record shows what the decision +// was actually made on. The waiting goes through clock.After, so tests advance +// virtual time instead of sleeping. When the re-check clears the verdict it +// also corrects internet_ok, which InsertRun wrote from the mistaken first +// sample. +func (r *runner) confirmDown(ctx context.Context, runID string, initial domain.Result) domain.Result { + result := initial + for i := 0; i < r.settings.CheckConfirmations; i++ { + <-r.clock.After(r.settings.CheckConfirmDelay) + + result = r.checker.Check(ctx) + r.persistChecks(ctx, runID, domain.PhaseConfirm, result.Targets) + if !result.IsDown() { + break + } + } + + if !result.IsDown() { + internetOK := true + if err := r.repo.UpdateRun(ctx, runID, domain.RunUpdate{InternetOK: &internetOK}); err != nil { + r.logf("update run %s: %v", runID, err) + } + r.logf("run %s: down verdict not confirmed on re-check (state now %s): not rebooting", runID, result.State) + } + + return result +} + +// finishDegraded records a run whose domain (HTTPS) targets all failed while +// raw ip connectivity held (issue #6). The router is never rebooted for this: +// on 2026-07-30 four reboots in seven hours did not stop the same domain +// probes from stalling. The operator is told once per episode — shouldAnnounce +// suppresses the repeat while the state persists — because the fault is real +// (name resolution or the HTTPS path is broken for everything on the LAN) even +// though rebooting is not the answer to it. +func (r *runner) finishDegraded(ctx context.Context, runID string) int { + if r.shouldAnnounce(ctx, runID, domain.OutcomeDegraded) { + r.notify(ctx, "domain checks failing while raw connectivity holds (no reboot)") + } + if err := r.notifier.Flush(ctx); err != nil { + r.logf("flush outbox: %v", err) + } + + outcome := domain.OutcomeDegraded + finishedAt := r.clock.Now() + if err := r.repo.UpdateRun(ctx, runID, domain.RunUpdate{Outcome: &outcome, FinishedAt: &finishedAt}); err != nil { + r.logf("update run %s: %v", runID, err) + } + return domain.ExitNoAction +} + +// finishNoOutage closes a run that reached no reboot: outcome ok (the internet +// was up), unconfirmed (a down verdict the in-run re-check cleared), or +// unsustained (a confirmed outage the preceding runs did not corroborate). The +// last two send no Telegram message by design — silence is the point of the +// guards, and the run row plus the log line carry the record instead (issue +// #6). +func (r *runner) finishNoOutage(ctx context.Context, runID, outcome string, exitCode int) int { + if outcome == domain.OutcomeUnsustained { + r.logf("run %s: outage not corroborated by the previous %d run(s): not rebooting", runID, r.settings.RebootMinStreak-1) + } + + finishedAt := r.clock.Now() + if err := r.repo.UpdateRun(ctx, runID, domain.RunUpdate{Outcome: &outcome, FinishedAt: &finishedAt}); err != nil { + r.logf("update run %s: %v", runID, err) + } + return exitCode +} + // finishRebootDisabled records a run that would otherwise have rebooted but // REBOOT_ENABLED is off (plans/003-host-telemetry.md: monitor-only mode). // Exit stays domain.ExitOK: the reboot path was never entered, so none of @@ -288,6 +361,37 @@ func (r *runner) notify(ctx context.Context, text string) { } } +// outageSustained reports whether the REBOOT_MIN_STREAK-1 runs before runID +// also saw the internet as down (issue #6). One five-minute cron sample is not +// an outage: in 21 days of recorded history exactly one down verdict had a +// down predecessor, while 11 reboots fired on isolated samples. A run with no +// verdict at all (a hard-mode run, internet_ok NULL) does not corroborate — it +// never probed — and neither does a history too short to fill the streak. A +// failed query is fail-closed for the same reason as the cooldown guard: an +// unreadable history must not fall through to an unconditional reboot. +func (r *runner) outageSustained(ctx context.Context, runID string) bool { + need := r.settings.RebootMinStreak - 1 + if need <= 0 { + return true + } + + previous, err := r.repo.RecentRuns(ctx, runID, need) + if err != nil { + r.logf("recent runs: %v", err) + return false + } + if len(previous) < need { + return false + } + + for _, p := range previous { + if p.InternetOK == nil || *p.InternetOK { + return false + } + } + return true +} + // persistCheck writes one probed target as a checks row (design §9). func (r *runner) persistCheck(ctx context.Context, runID, phase string, tr domain.TargetResult) { latencyMS := tr.Latency.Milliseconds() @@ -361,7 +465,11 @@ func (r *runner) recoveryLoop(ctx context.Context, runID string, rebootStartedAt inet := r.checker.Check(ctx) r.persistChecks(ctx, runID, domain.PhaseRecovery, inet.Targets) - if !inet.Down { + // anything short of a down verdict ends the loop: a degraded state + // (issue #6) means the link the reboot was meant to restore is back, + // and waiting out the full RECOVERY_TIMEOUT for DNS to also recover + // would only hold the process open for a fault a reboot cannot fix. + if !inet.IsDown() { return r.finishRecoverySuccess(ctx, runID, now) } if now.Sub(rebootStartedAt) >= r.settings.RecoveryTimeout { @@ -380,11 +488,34 @@ func (r *runner) run(ctx context.Context, mode string) int { return r.softFlow(ctx, startedAt) } +// shouldAnnounce reports whether a repeating state is worth another Telegram +// message: true only when the run before runID did not already end in the same +// outcome (issue #6). A cron collector runs every few minutes, so a state that +// lasts an hour would otherwise send a message per run — the storm visible in +// issue #6's screenshot, where five identical cooldown notices arrived between +// 01:50 and 03:05. A failed history query announces: a duplicate message costs +// the operator far less than a silent outage. +func (r *runner) shouldAnnounce(ctx context.Context, runID, outcome string) bool { + previous, err := r.repo.RecentRuns(ctx, runID, 1) + if err != nil { + r.logf("recent runs: %v", err) + return true + } + if len(previous) == 0 { + return true + } + return previous[0].Outcome != outcome +} + // skipCooldown records a reboot skipped because the last one is still within // REBOOT_COOLDOWN (design §6) — rebooting through a provider-side outage -// would otherwise cycle the router every run for no benefit. +// would otherwise cycle the router every run for no benefit. The notice is +// edge-triggered (shouldAnnounce): the skip repeats every run for as long as +// the cooldown lasts, but it is only news the first time. func (r *runner) skipCooldown(ctx context.Context, runID string, age time.Duration) int { - r.notify(ctx, fmt.Sprintf("no internet, skipping reboot (cooldown: last reboot %s ago)", humanDuration(age))) + if r.shouldAnnounce(ctx, runID, domain.OutcomeSkipped) { + r.notify(ctx, fmt.Sprintf("no internet, skipping reboot (cooldown: last reboot %s ago)", humanDuration(age))) + } action := domain.ActionSkippedCooldown outcome := domain.OutcomeSkipped @@ -416,13 +547,16 @@ func (r *runner) skipCooldownUnknown(ctx context.Context, runID string, queryErr } // softFlow probes the internet first (design §6): up means nothing to do; -// down checks REBOOT_ENABLED before ever consulting the cooldown query, so a -// monitor-only host never touches the router (plans/003-host-telemetry.md). +// down goes through the issue #6 guards — re-probe within the run, then +// require the preceding runs to agree — before REBOOT_ENABLED and the +// cooldown query are ever consulted, so a monitor-only host never touches the +// router (plans/003-host-telemetry.md) and a five-second blip never reboots +// anything. func (r *runner) softFlow(ctx context.Context, startedAt time.Time) int { result := r.checker.Check(ctx) - internetOK := !result.Down + initialOK := !result.IsDown() - runID, err := r.repo.InsertRun(ctx, startedAt, domain.ModeSoft, &internetOK) + runID, err := r.repo.InsertRun(ctx, startedAt, domain.ModeSoft, &initialOK) if err != nil { r.logf("insert run: %v", err) return domain.ExitConfigError @@ -430,19 +564,27 @@ func (r *runner) softFlow(ctx context.Context, startedAt time.Time) int { r.persistChecks(ctx, runID, domain.PhaseInitial, result.Targets) r.collectTelemetry(ctx, runID) - if internetOK { - outcome := domain.OutcomeOK - finishedAt := r.clock.Now() - if err := r.repo.UpdateRun(ctx, runID, domain.RunUpdate{Outcome: &outcome, FinishedAt: &finishedAt}); err != nil { - r.logf("update run %s: %v", runID, err) - } - return domain.ExitOK + if result.IsDown() { + result = r.confirmDown(ctx, runID, result) + } + + switch { + case result.IsDegraded(): + return r.finishDegraded(ctx, runID) + case !result.IsDown() && !initialOK: + return r.finishNoOutage(ctx, runID, domain.OutcomeUnconfirmed, domain.ExitOK) + case !result.IsDown(): + return r.finishNoOutage(ctx, runID, domain.OutcomeOK, domain.ExitOK) } if !r.settings.RebootEnabled { return r.finishRebootDisabled(ctx, runID, "no internet, reboot disabled (monitor-only)") } + if !r.outageSustained(ctx, runID) { + return r.finishNoOutage(ctx, runID, domain.OutcomeUnsustained, domain.ExitNoAction) + } + lastRebootStartedAt, ok, err := r.repo.GetLastRebootStartedAt(ctx) if err != nil { r.logf("get last reboot started at: %v", err) diff --git a/services/orchestrator_test.go b/services/orchestrator_test.go index 56e8fab..1dae3c1 100644 --- a/services/orchestrator_test.go +++ b/services/orchestrator_test.go @@ -16,7 +16,7 @@ func TestExecute(t *testing.T) { t.Parallel() env := newTestEnv(t) - env.chk.checkResults = []domain.Result{{Down: false, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true, Latency: 5 * time.Millisecond}}}} + env.chk.checkResults = []domain.Result{{State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true, Latency: 5 * time.Millisecond}}}} code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) @@ -60,7 +60,7 @@ func TestExecute(t *testing.T) { env := newTestEnv(t) env.settings.RebootCooldown = 2 * time.Hour - env.chk.checkResults = []domain.Result{{Down: true, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false, Error: "dial timeout"}}}} + env.chk.checkResults = []domain.Result{{State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false, Error: "dial timeout"}}}} env.repo.lastRebootStartedAt = env.clk.now.Add(-40 * time.Minute) env.repo.lastRebootOK = true @@ -100,7 +100,7 @@ func TestExecute(t *testing.T) { t.Parallel() env := newTestEnv(t) - env.chk.checkResults = []domain.Result{{Down: true, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false, Error: "dial timeout"}}}} + env.chk.checkResults = []domain.Result{{State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false, Error: "dial timeout"}}}} // fakeRunRepo.lastRebootErr stands in for any transient store error // that makes the cooldown state genuinely unknown (e.g. a corrupted // reboot_started_at column) — the real SQL failure mode is covered @@ -142,8 +142,8 @@ func TestExecute(t *testing.T) { env := newTestEnv(t) env.chk.checkResults = []domain.Result{ - {Down: true, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false, Error: "down"}}}, - {Down: false, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}, + {State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false, Error: "down"}}}, + {State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}, } env.chk.gatewayResults = []domain.TargetResult{{Target: "192.168.1.1:80", Kind: domain.CheckKindGateway, OK: true}} @@ -173,7 +173,7 @@ func TestExecute(t *testing.T) { t.Parallel() env := newTestEnv(t) - env.chk.checkResults = []domain.Result{{Down: false, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} + env.chk.checkResults = []domain.Result{{State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} env.chk.gatewayResults = []domain.TargetResult{{Target: "192.168.1.1:80", Kind: domain.CheckKindGateway, OK: true}} code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeHard) @@ -203,11 +203,11 @@ func TestExecute(t *testing.T) { env := newTestEnv(t) env.chk.checkResults = []domain.Result{ - {Down: true, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, // initial soft check - {Down: true, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, // tick 1 - {Down: true, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, // tick 2 - {Down: true, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, // tick 3 - {Down: false, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}, // tick 4 + {State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, // initial soft check + {State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, // tick 1 + {State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, // tick 2 + {State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, // tick 3 + {State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}, // tick 4 } env.chk.gatewayResults = []domain.TargetResult{ {Target: "gw", Kind: domain.CheckKindGateway, OK: false}, // tick 1: went down @@ -261,9 +261,9 @@ func TestExecute(t *testing.T) { env := newTestEnv(t) env.chk.checkResults = []domain.Result{ - {Down: true, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, // initial - {Down: true, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, // tick 1 - {Down: false, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}, // tick 2 + {State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, // initial + {State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, // tick 1 + {State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}, // tick 2 } // gateway never observed down: the router cycled between polls. env.chk.gatewayResults = []domain.TargetResult{{Target: "gw", Kind: domain.CheckKindGateway, OK: true}} @@ -306,7 +306,7 @@ func TestExecute(t *testing.T) { env := newTestEnv(t) env.settings.RecoveryInterval = time.Minute env.settings.RecoveryTimeout = 15 * time.Minute - env.chk.checkResults = []domain.Result{{Down: true, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}} + env.chk.checkResults = []domain.Result{{State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}} env.chk.gatewayResults = []domain.TargetResult{{Target: "gw", Kind: domain.CheckKindGateway, OK: false}} code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) @@ -334,7 +334,7 @@ func TestExecute(t *testing.T) { t.Parallel() env := newTestEnv(t) - env.chk.checkResults = []domain.Result{{Down: true, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}} + env.chk.checkResults = []domain.Result{{State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}} env.rb.err = errRebootFailed code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) @@ -366,7 +366,7 @@ func TestExecute(t *testing.T) { env := newTestEnv(t) env.settings.RebootEnabled = false - env.chk.checkResults = []domain.Result{{Down: true, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false, Error: "dial timeout"}}}} + env.chk.checkResults = []domain.Result{{State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false, Error: "dial timeout"}}}} code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) @@ -435,7 +435,7 @@ func TestExecute(t *testing.T) { env := newTestEnv(t) env.settings.RebootEnabled = false - env.chk.checkResults = []domain.Result{{Down: false, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} + env.chk.checkResults = []domain.Result{{State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) @@ -461,8 +461,8 @@ func TestExecute(t *testing.T) { env := newTestEnv(t) env.settings.MetricsEnabled = false env.chk.checkResults = []domain.Result{ - {Down: true, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, - {Down: false, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}, + {State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, + {State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}, } env.chk.gatewayResults = []domain.TargetResult{{Target: "gw", Kind: domain.CheckKindGateway, OK: true}} @@ -484,7 +484,7 @@ func TestExecute(t *testing.T) { env := newTestEnv(t) env.mr.err = errors.New("disk full") - env.chk.checkResults = []domain.Result{{Down: false, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} + env.chk.checkResults = []domain.Result{{State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) @@ -505,7 +505,7 @@ func TestExecute(t *testing.T) { t.Parallel() env := newTestEnv(t) - env.chk.checkResults = []domain.Result{{Down: false, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} + env.chk.checkResults = []domain.Result{{State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} // Built directly rather than through Execute (which has no way to // inject a shorter-than-production metricsTimeout): r.run is the @@ -557,7 +557,7 @@ func TestExecute(t *testing.T) { env := newTestEnv(t) env.pc.results = []domain.PingResult{{Host: "1.1.1.1", Sent: 5, Received: 5, AvgMS: 12.5, OK: true}} - env.chk.checkResults = []domain.Result{{Down: false, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} + env.chk.checkResults = []domain.Result{{State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) @@ -577,7 +577,7 @@ func TestExecute(t *testing.T) { env := newTestEnv(t) env.pc.results = []domain.PingResult{{Host: "1.1.1.1", Sent: 5, Received: 5, AvgMS: 12.5, OK: true}} - env.chk.checkResults = []domain.Result{{Down: false, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} + env.chk.checkResults = []domain.Result{{State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} env.chk.gatewayResults = []domain.TargetResult{{Target: "gw", Kind: domain.CheckKindGateway, OK: true}} code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeHard) @@ -599,7 +599,7 @@ func TestExecute(t *testing.T) { env := newTestEnv(t) env.settings.PingEnabled = false - env.chk.checkResults = []domain.Result{{Down: false, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} + env.chk.checkResults = []domain.Result{{State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) @@ -619,7 +619,7 @@ func TestExecute(t *testing.T) { env := newTestEnv(t) env.pr.err = errors.New("disk full") - env.chk.checkResults = []domain.Result{{Down: false, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} + env.chk.checkResults = []domain.Result{{State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) @@ -640,7 +640,7 @@ func TestExecute(t *testing.T) { t.Parallel() env := newTestEnv(t) - env.chk.checkResults = []domain.Result{{Down: false, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} + env.chk.checkResults = []domain.Result{{State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}} // Built directly rather than through Execute (which has no way to // inject a shorter-than-production pingTimeout): r.run is the same @@ -683,6 +683,278 @@ func TestExecute(t *testing.T) { t.Fatalf("ping saves = %+v, want none (the collector never returned within the timeout)", env.pr.calls) } }) + + t.Run("soft, down verdict cleared by the re-check: no reboot, recorded as unconfirmed", func(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + env.settings.CheckConfirmations = 2 + env.settings.CheckConfirmDelay = 20 * time.Second + env.chk.checkResults = []domain.Result{ + {State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, // initial + {State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}, // re-check clears it + } + + code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) + + if code != domain.ExitOK { + t.Fatalf("Execute() = %d, want ExitOK (the outage was noise)", code) + } + if env.rb.calls != 0 { + t.Fatalf("rebooter.Reboot calls = %d, want 0 (a single sample must never reboot)", env.rb.calls) + } + if len(env.nt.messages) != 0 { + t.Fatalf("messages sent = %v, want none (silence is the point of the guard)", env.nt.messages) + } + + run := env.repo.run(t, "1") + if run.Outcome != domain.OutcomeUnconfirmed { + t.Fatalf("Outcome = %q, want %q", run.Outcome, domain.OutcomeUnconfirmed) + } + if run.InternetOK == nil || !*run.InternetOK { + t.Fatalf("InternetOK = %v, want true (the initial sample's verdict was corrected)", run.InternetOK) + } + if got, want := env.repo.phases(), []string{domain.PhaseInitial, domain.PhaseConfirm}; !equalStrings(got, want) { + t.Fatalf("check phases = %v, want %v", got, want) + } + }) + + t.Run("soft, down verdict survives the re-check: reboots on the confirmed outage", func(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + env.settings.CheckConfirmations = 2 + env.settings.CheckConfirmDelay = 20 * time.Second + env.chk.checkResults = []domain.Result{ + {State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, // initial + both re-checks + {State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, + {State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, + {State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}, // recovery tick + } + env.chk.gatewayResults = []domain.TargetResult{{Target: "192.168.1.1:80", Kind: domain.CheckKindGateway, OK: true}} + + code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) + + if code != domain.ExitOK { + t.Fatalf("Execute() = %d, want ExitOK (reboot completed, internet restored)", code) + } + if env.rb.calls != 1 { + t.Fatalf("rebooter.Reboot calls = %d, want 1 (three samples agreed)", env.rb.calls) + } + + run := env.repo.run(t, "1") + if run.InternetOK == nil || *run.InternetOK { + t.Fatalf("InternetOK = %v, want false (the outage was real)", run.InternetOK) + } + if got, want := env.repo.phases()[:3], []string{domain.PhaseInitial, domain.PhaseConfirm, domain.PhaseConfirm}; !equalStrings(got, want) { + t.Fatalf("check phases = %v, want %v", got, want) + } + }) + + t.Run("soft, all domain targets down while raw ip holds: degraded, never rebooted", func(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + env.settings.CheckConfirmations = 2 + env.settings.CheckConfirmDelay = 20 * time.Second + env.chk.checkResults = []domain.Result{{State: domain.StateDegraded, Targets: []domain.TargetResult{ + {Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true, Latency: 24 * time.Millisecond}, + {Target: "https://a.example/", Kind: domain.CheckKindDomain, OK: false, Error: "context deadline exceeded (stage=dns)"}, + }}} + + code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) + + if code != domain.ExitNoAction { + t.Fatalf("Execute() = %d, want ExitNoAction", code) + } + if env.rb.calls != 0 { + t.Fatalf("rebooter.Reboot calls = %d, want 0 (a reboot does not fix a broken DNS path)", env.rb.calls) + } + if env.chk.checkCalls != 1 { + t.Fatalf("checker.Check calls = %d, want 1 (degraded is not a down verdict to re-confirm)", env.chk.checkCalls) + } + if len(env.nt.messages) != 1 || !strings.Contains(env.nt.messages[0], "domain checks failing") { + t.Fatalf("messages = %v, want one degraded notice", env.nt.messages) + } + + run := env.repo.run(t, "1") + if run.Outcome != domain.OutcomeDegraded { + t.Fatalf("Outcome = %q, want %q", run.Outcome, domain.OutcomeDegraded) + } + if run.Action != domain.ActionNone { + t.Fatalf("Action = %q, want %q", run.Action, domain.ActionNone) + } + }) + + t.Run("soft, degraded again right after a degraded run: no repeat message", func(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + env.repo.recentRuns = []domain.RunSummary{{Outcome: domain.OutcomeDegraded}} + env.chk.checkResults = []domain.Result{{State: domain.StateDegraded, Targets: []domain.TargetResult{ + {Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}, + {Target: "https://a.example/", Kind: domain.CheckKindDomain, OK: false}, + }}} + + Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) + + if len(env.nt.messages) != 0 { + t.Fatalf("messages = %v, want none (the previous run already announced this state)", env.nt.messages) + } + }) + + t.Run("soft, confirmed outage the previous run did not see: unsustained, no reboot", func(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + env.settings.RebootMinStreak = 2 + healthy := true + env.repo.recentRuns = []domain.RunSummary{{InternetOK: &healthy, Outcome: domain.OutcomeOK}} + env.chk.checkResults = []domain.Result{{State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}} + + code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) + + if code != domain.ExitNoAction { + t.Fatalf("Execute() = %d, want ExitNoAction", code) + } + if env.rb.calls != 0 { + t.Fatalf("rebooter.Reboot calls = %d, want 0 (one five-minute sample is not an outage)", env.rb.calls) + } + if len(env.nt.messages) != 0 { + t.Fatalf("messages = %v, want none", env.nt.messages) + } + + run := env.repo.run(t, "1") + if run.Outcome != domain.OutcomeUnsustained { + t.Fatalf("Outcome = %q, want %q", run.Outcome, domain.OutcomeUnsustained) + } + if run.InternetOK == nil || *run.InternetOK { + t.Fatalf("InternetOK = %v, want false (the verdict stands, only the action is withheld)", run.InternetOK) + } + }) + + t.Run("soft, confirmed outage the previous run also saw: reboots", func(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + env.settings.RebootMinStreak = 2 + outage := false + env.repo.recentRuns = []domain.RunSummary{{InternetOK: &outage, Outcome: domain.OutcomeUnsustained}} + env.chk.checkResults = []domain.Result{ + {State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, + {State: domain.StateUp, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}}}, + } + env.chk.gatewayResults = []domain.TargetResult{{Target: "192.168.1.1:80", Kind: domain.CheckKindGateway, OK: true}} + + code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) + + if code != domain.ExitOK { + t.Fatalf("Execute() = %d, want ExitOK", code) + } + if env.rb.calls != 1 { + t.Fatalf("rebooter.Reboot calls = %d, want 1 (the outage spans two runs)", env.rb.calls) + } + }) + + t.Run("soft, a hard-mode run in the history does not corroborate an outage", func(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + env.settings.RebootMinStreak = 2 + env.repo.recentRuns = []domain.RunSummary{{InternetOK: nil, Outcome: domain.OutcomeOK}} + env.chk.checkResults = []domain.Result{{State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}} + + code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) + + if code != domain.ExitNoAction { + t.Fatalf("Execute() = %d, want ExitNoAction (a run that never probed cannot corroborate)", code) + } + if env.rb.calls != 0 { + t.Fatalf("rebooter.Reboot calls = %d, want 0", env.rb.calls) + } + }) + + t.Run("soft, unreadable history is fail-closed: no reboot", func(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + env.settings.RebootMinStreak = 2 + env.repo.recentRunsErr = errors.New("store: disk i/o error") + env.chk.checkResults = []domain.Result{{State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}} + + code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) + + if code != domain.ExitNoAction { + t.Fatalf("Execute() = %d, want ExitNoAction", code) + } + if env.rb.calls != 0 { + t.Fatalf("rebooter.Reboot calls = %d, want 0 (an unreadable history must not fall through to a reboot)", env.rb.calls) + } + }) + + t.Run("soft, cooldown skip repeated after a skipped run: no repeat message", func(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + env.repo.lastRebootStartedAt = env.clk.now.Add(-30 * time.Minute) + env.repo.lastRebootOK = true + env.repo.recentRuns = []domain.RunSummary{{Outcome: domain.OutcomeSkipped}} + env.chk.checkResults = []domain.Result{{State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}} + + code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) + + if code != domain.ExitSkippedCooldown { + t.Fatalf("Execute() = %d, want ExitSkippedCooldown", code) + } + if len(env.nt.messages) != 0 { + t.Fatalf("messages = %v, want none (the cooldown skip was already announced)", env.nt.messages) + } + + run := env.repo.run(t, "1") + if run.Outcome != domain.OutcomeSkipped { + t.Fatalf("Outcome = %q, want %q", run.Outcome, domain.OutcomeSkipped) + } + }) + + t.Run("soft, recovery ends on a degraded verdict rather than waiting out the timeout", func(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + env.chk.checkResults = []domain.Result{ + {State: domain.StateDown, Targets: []domain.TargetResult{{Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: false}}}, // initial + {State: domain.StateDegraded, Targets: []domain.TargetResult{ // tick 1: link back, DNS still broken + {Target: "1.1.1.1:443", Kind: domain.CheckKindIP, OK: true}, + {Target: "https://a.example/", Kind: domain.CheckKindDomain, OK: false}, + }}, + } + env.chk.gatewayResults = []domain.TargetResult{{Target: "192.168.1.1:80", Kind: domain.CheckKindGateway, OK: true}} + + code := Execute(t.Context(), env.settings, env.repo, env.mr, env.pr, env.chk, env.rb, env.nt, env.mc, env.pc, env.clk, domain.ModeSoft) + + if code != domain.ExitOK { + t.Fatalf("Execute() = %d, want ExitOK (the link the reboot targeted is back)", code) + } + + run := env.repo.run(t, "1") + if run.InternetRestoredAt == nil { + t.Fatal("InternetRestoredAt is nil, want set") + } + }) +} + +// equalStrings reports whether two string slices hold the same values in the +// same order, so a phase-sequence assertion reads as one comparison instead +// of a hand-rolled loop at every call site. +func equalStrings(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true } // errRebootFailed is a stand-in for gateway/router's real reboot errors; @@ -709,6 +981,10 @@ type testEnv struct { // newTestEnv builds a testEnv with sane defaults (2h cooldown, 60s recovery // interval, 15m recovery timeout, reboot, metrics, and ping all enabled — the // design's own defaults) that a subtest can override before calling Execute. +// The issue #6 false-positive guards are left at their "off" values +// (CheckConfirmations 0, RebootMinStreak 0) so every pre-existing scenario +// keeps asserting the flow it was written for; the guards have their own +// subtests, which set them explicitly. func newTestEnv(t *testing.T) *testEnv { t.Helper() diff --git a/services/ports.go b/services/ports.go index cf71006..184abc7 100644 --- a/services/ports.go +++ b/services/ports.go @@ -56,11 +56,18 @@ type Clock interface { // infrastructure.Store satisfies it. InsertRun's returned id and UpdateRun's // id parameter are opaque UUIDv7 strings (issue #4) that the orchestrator // only ever threads through, never inspects. +// RecentRuns returns the runs immediately preceding beforeID, newest first, +// at most limit of them — the history the issue #6 guards corroborate a +// present outage against. beforeID is the caller's own run id: ids are +// time-ordered UUIDv7 strings (issue #4), so "preceding" is a plain id +// comparison and the caller's own freshly inserted row is never returned to +// it. Fewer than limit rows simply means the history is that short. type RunRepository interface { InsertRun(ctx context.Context, startedAt time.Time, mode string, internetOK *bool) (string, error) UpdateRun(ctx context.Context, id string, u domain.RunUpdate) error InsertCheck(ctx context.Context, c domain.Check) error GetLastRebootStartedAt(ctx context.Context) (time.Time, bool, error) + RecentRuns(ctx context.Context, beforeID string, limit int) ([]domain.RunSummary, error) } // MetricsCollector takes one host telemetry snapshot (design @@ -118,12 +125,21 @@ type Sender interface { // domain types (plans/003-host-telemetry.md Trade-off T1): domain owns the // vocabulary the toggles drive (ActionSkippedDisabled, OutcomeRebootDisabled, // the Collector consts), not the toggle itself. +// CheckConfirmations, CheckConfirmDelay, and RebootMinStreak (issue #6) are +// the two independent guards between a down verdict and a reboot: the first +// pair re-probes within the run, the third requires the preceding runs to have +// seen the outage too. Both are counted in samples rather than wall time +// because the collector is a cron process with no memory of its own — the +// interval between runs is cron's business, not the orchestrator's. type Settings struct { - Label string - RebootCooldown time.Duration - RecoveryInterval time.Duration - RecoveryTimeout time.Duration - RebootEnabled bool - MetricsEnabled bool - PingEnabled bool + Label string + RebootCooldown time.Duration + RecoveryInterval time.Duration + RecoveryTimeout time.Duration + CheckConfirmations int + CheckConfirmDelay time.Duration + RebootMinStreak int + RebootEnabled bool + MetricsEnabled bool + PingEnabled bool } diff --git a/yarddog.env.example b/yarddog.env.example index 41fab73..0269a7f 100644 --- a/yarddog.env.example +++ b/yarddog.env.example @@ -26,6 +26,20 @@ YARDDOG_ROUTER_PASS=change-me #YARDDOG_RECOVERY_INTERVAL=60s #YARDDOG_RECOVERY_TIMEOUT=15m #YARDDOG_REBOOT_COOLDOWN=2h + +# false-positive guards between a "down" verdict and a reboot. +# a down verdict is re-probed this many more times, this far apart, and a single +# healthy sample calls the whole thing off (outcome "unconfirmed", no message). +# 0 confirmations restores the old "act on the first sample" behaviour. +#YARDDOG_CHECK_CONFIRMATIONS=2 +# ^ clamped to [0, 5] +#YARDDOG_CHECK_CONFIRM_DELAY=20s +# ^ clamped to [1s, 60s] +# how many consecutive runs (this one included) must have seen the internet down +# before a reboot is allowed. 2 means the previous run has to agree; 1 disables +# the cross-run check. a run that falls short is recorded as "unsustained". +#YARDDOG_REBOOT_MIN_STREAK=2 +# ^ clamped to [1, 10] # a run stays in the fast "hot" tables while younger than this; older runs # roll into the *_archive tables at every collector startup (clamped to >=1) #YARDDOG_HOT_WINDOW_DAYS=30