diff --git a/grafana-alertcheck/cmd/grafana-alertcheck/check.go b/grafana-alertcheck/cmd/grafana-alertcheck/check.go index f4e9dab26..3f2d295d6 100644 --- a/grafana-alertcheck/cmd/grafana-alertcheck/check.go +++ b/grafana-alertcheck/cmd/grafana-alertcheck/check.go @@ -17,28 +17,27 @@ const checkUsage = "usage: grafana-alertcheck check [--in ] [--pidfile F] "[--alerts ...] [--folder F] [--states ...] [--preexisting ...] [--min-observed N] [--allow-paused] " + "[--nodata-is-unobservable] [--concurrency N] [--output json]" -// runCheck is the classify step's CLI surface: parse flags into a -// gate.Config, run gate.Check, and translate its (Result, error) into -// §20.2/§20.3's output and exit code. All of the correctness lives in -// gate.Check (P9) and decide (P8) — this file's only job is presentation and -// the H6/H7 exit-code mapping, which exitCode below keeps as one pure -// function so it can be tested without a network. +// runCheck is the classify step's CLI surface: parse flags into a gate.Config, +// run gate.Check, and translate its (Result, error) into output and an exit +// code. All of the correctness lives in the gate package — this file's only job +// is presentation and the exit-code mapping, which exitCode below keeps as one +// pure function so it can be tested without a network. func runCheck(args []string, stdin io.Reader, stdout, stderr io.Writer) int { fs := flag.NewFlagSet("check", flag.ContinueOnError) fs.SetOutput(stderr) fs.Usage = func() { fmt.Fprintln(stderr, checkUsage) } common := registerCommon(fs) - in := fs.String("in", "", "path of a log recorded by watch; empty selects single-step mode (§9)") + in := fs.String("in", "", "path of a log recorded by watch; empty selects single-step mode") pidfile := fs.String("pidfile", "", "pidfile of the recorder to stop before reading --in (default .pid)") - from := fs.String("from", "", "the moment the deploy finished, RFC3339 (required in recorder mode, §7)") + from := fs.String("from", "", "the moment the deploy finished, RFC3339 (required with --in)") to := fs.String("to", "", "the end of the window to classify, RFC3339 (required)") - states := fs.String("states", "", "comma-separated bad states to classify against (default: firing, §13)") - preexisting := fs.String("preexisting", "", "how to judge an instance already bad at `from` (default: fail-unless-recovered, §11.7)") - minObserved := fs.Int("min-observed", 0, "minimum rules that must be observed (default: every resolved rule, §12)") - allowPaused := fs.Bool("allow-paused", false, "do not count a rule paused before the window against --min-observed (§12.1)") - nodataIsUnobservable := fs.Bool("nodata-is-unobservable", false, "treat a sustained health=nodata as unobservable rather than a note (§10.2)") - output := fs.String("output", "", `"json" writes the machine-readable Result to stdout in addition to the table (§20.2); default is the table alone`) + states := fs.String("states", "", "comma-separated bad states to classify against (default: firing)") + preexisting := fs.String("preexisting", "", "how to judge an instance already bad at `from` (default: fail-unless-recovered)") + minObserved := fs.Int("min-observed", 0, "minimum rules that must be observed (default: every resolved rule)") + allowPaused := fs.Bool("allow-paused", false, "do not count a rule paused before the window against --min-observed") + nodataIsUnobservable := fs.Bool("nodata-is-unobservable", false, "treat a sustained health=nodata as unobservable rather than a note") + output := fs.String("output", "", `"json" writes the machine-readable Result to stdout in addition to the table; default is the table alone`) if err := fs.Parse(args); err != nil { return 2 @@ -86,7 +85,7 @@ func runCheck(args []string, stdin io.Reader, stdout, stderr io.Writer) int { Notes: stderr, } if *to == "" { - fmt.Fprintln(stderr, "check: --to is required (§7)") + fmt.Fprintln(stderr, "check: --to is required") return 2 } t, err := time.Parse(time.RFC3339, *to) @@ -131,11 +130,11 @@ func runCheck(args []string, stdin io.Reader, stdout, stderr io.Writer) int { return exitCode(result, checkErr) } -// exitCode is §20.3/H6/H7's whole mapping, kept as one pure function of +// exitCode is the whole exit-code mapping, kept as one pure function of // exactly what Check returns so it is testable without a network: err != nil // is exit 2 UNCONDITIONALLY — never 0 and never 1, even alongside real -// violations, because inability beats violation (H6) and an error is never a -// pass (H7). Violations without an error is exit 1. Neither is exit 0. +// violations, because an inability to check beats a violation and an error is +// never a pass. Violations without an error is exit 1. Neither is exit 0. func exitCode(res gate.Result, err error) int { switch { case err != nil: diff --git a/grafana-alertcheck/cmd/grafana-alertcheck/check_test.go b/grafana-alertcheck/cmd/grafana-alertcheck/check_test.go index dbf81d3a9..99e8ebc3a 100644 --- a/grafana-alertcheck/cmd/grafana-alertcheck/check_test.go +++ b/grafana-alertcheck/cmd/grafana-alertcheck/check_test.go @@ -10,9 +10,9 @@ import ( "github.com/smartcontractkit/chainlink-testing-framework/grafana-alertcheck/internal/gate" ) -// TestExitCode pins §20.3/H6/H7's mapping directly against exitCode, with no -// network involved: err != nil is exit 2 even alongside violations (H6 — -// inability beats violation), violations alone are exit 1, and neither is 0. +// The exit-code mapping, pinned directly against exitCode with no network +// involved: err != nil is exit 2 even alongside violations (an inability to +// check beats a violation), violations alone are exit 1, and neither is 0. func TestExitCode(t *testing.T) { tests := []struct { name string @@ -43,10 +43,10 @@ func writeTempAlerts(t *testing.T) string { return path } -// TestRunCheck_FlagValidation is the flag-validation matrix: every one of -// these must fail before any network call, because Config.validate() (P9) -// runs first — an unreachable GRAFANA_URL succeeding or timing out is a -// different test than these, which check pure input validation. +// The flag-validation matrix: every one of these must fail before any network +// call, because gate.Config.validate() runs first — an unreachable GRAFANA_URL +// succeeding or timing out is a different test than these, which check pure +// input validation. func TestRunCheck_FlagValidation(t *testing.T) { tests := []struct { name string @@ -73,9 +73,9 @@ func TestRunCheck_FlagValidation(t *testing.T) { return []string{"--to", "2026-01-01T00:00:00Z", "--states", "bogus", "--alerts", writeTempAlerts(t)} }, "--states"}, {"states normal is rejected", true, func(t *testing.T) []string { - // normal is the good state, never a state to classify AS bad - // (R1): accepting it would make --states normal fail every - // healthy instance, the fail-open shape H7 exists to prevent. + // normal is the good state, never a state to classify AS bad: + // accepting it would make --states normal fail every healthy + // instance. return []string{"--to", "2026-01-01T00:00:00Z", "--states", "normal", "--alerts", writeTempAlerts(t)} }, "--states"}, {"bad preexisting", true, func(t *testing.T) []string { @@ -110,9 +110,8 @@ func TestRunCheck_FlagValidation(t *testing.T) { } } -// TestRunCheck_ToInPastNoLog pins §4.2's refusal: a `to` already in the past -// with no recorded log cannot be classified from anything, because nothing -// ever observed the window. +// A `to` already in the past with no recorded log cannot be classified from +// anything, because nothing ever observed the window. func TestRunCheck_ToInPastNoLog(t *testing.T) { t.Setenv("GRAFANA_URL", "http://example.invalid") t.Setenv("GRAFANA_TOKEN", "test-token") @@ -126,13 +125,13 @@ func TestRunCheck_ToInPastNoLog(t *testing.T) { t.Fatalf("code = %d, want 2; stderr = %q", code, stderr.String()) } if !strings.Contains(stderr.String(), "already passed") { - t.Fatalf("stderr = %q, want the §4.2 refusal", stderr.String()) + t.Fatalf("stderr = %q, want the past-`to` refusal", stderr.String()) } } -// TestRunCheck_NoResultOnConfigError pins §20.2: --output json never writes -// to stdout when Check was never reached, because there is no Result to -// encode — only the table (on stderr) can report a configuration failure. +// --output json never writes to stdout when Check was never reached, because +// there is no Result to encode — only the table (on stderr) can report a +// configuration failure. func TestRunCheck_NoResultOnConfigError(t *testing.T) { t.Setenv("GRAFANA_URL", "") t.Setenv("GRAFANA_TOKEN", "") diff --git a/grafana-alertcheck/cmd/grafana-alertcheck/common.go b/grafana-alertcheck/cmd/grafana-alertcheck/common.go index e605c029b..3c56cd63f 100644 --- a/grafana-alertcheck/cmd/grafana-alertcheck/common.go +++ b/grafana-alertcheck/cmd/grafana-alertcheck/common.go @@ -12,11 +12,11 @@ import ( ) // commonFlags is registerCommon's result: the exactly three flags watch and -// check share (§20). Connection details are never flags (§20.2) and states / -// poll-interval are deliberately NOT here — states is check-only because -// recording is unfiltered (P6), and poll-interval is watch-only because check -// reads the cadence from the log header (P5). Putting either here would -// silently reinstate a knob this plan removed. +// check share. Connection details are never flags, and states / poll-interval +// are deliberately NOT here — states is check-only because recording is +// unfiltered, and poll-interval is watch-only because check reads the cadence +// from the log header. Putting either here would give both commands an opinion +// about a value only one of them may set. type commonFlags struct { folder *string concurrency *int @@ -25,13 +25,13 @@ type commonFlags struct { func registerCommon(fs *flag.FlagSet) *commonFlags { return &commonFlags{ - folder: fs.String("folder", "", "default folder to scope an unqualified alert name to (§17)"), + folder: fs.String("folder", "", "default folder to scope an unqualified alert name to"), concurrency: fs.Int("concurrency", 1, "maximum concurrent requests to Grafana"), alerts: fs.String("alerts", "", "path to a file of alert names, one per line, or - for stdin"), } } -// readAlerts reads §17's alert names, one per line, from a file or from +// readAlerts reads alert names, one per line, from a file or from // stdin when path is "-". An empty path is not an error here — watch and // check each decide for themselves whether an empty list is allowed // (log mode never wants one; single-step / record mode always does). @@ -62,16 +62,15 @@ func readAlerts(stdin io.Reader, path string) ([]string, error) { } // parseStates parses check's --states flag: a comma-separated list of the -// "bad" state vocabulary Config.States matches against (§13, classify.go's +// "bad" state vocabulary Config.States matches against (classify.go's // badStateSet). An empty string is not resolved here — it means "use the // library default of {firing}" — so this returns nil, nil for "" rather than // an error. // -// normal is deliberately NOT accepted: the v2 plan fixes this vocabulary to -// firing | pending | nodata | error (line 378) precisely because "normal" is -// the good state, never a bad one to classify against. Accepting it here -// would let --states normal turn every healthy instance into a violation and -// fail every healthy fleet — the exact fail-open shape H7 exists to prevent. +// normal is deliberately NOT accepted. The vocabulary is fixed to +// firing | pending | nodata | error precisely because "normal" is the good +// state, never a bad one to classify against: --states normal would turn every +// healthy instance into a violation and fail every healthy fleet. func parseStates(s string) ([]gate.State, error) { if strings.TrimSpace(s) == "" { return nil, nil @@ -95,7 +94,7 @@ func parseStates(s string) ([]gate.State, error) { return out, nil } -// parsePreexisting parses check's --preexisting flag (§11.7). +// parsePreexisting parses check's --preexisting flag. func parsePreexisting(s string) (gate.PreexistingPolicy, error) { switch gate.PreexistingPolicy(s) { case "": diff --git a/grafana-alertcheck/cmd/grafana-alertcheck/env.go b/grafana-alertcheck/cmd/grafana-alertcheck/env.go index e5702a6d1..02a125f1d 100644 --- a/grafana-alertcheck/cmd/grafana-alertcheck/env.go +++ b/grafana-alertcheck/cmd/grafana-alertcheck/env.go @@ -7,8 +7,7 @@ import ( // grafanaEnv reads the connection details from the environment only, never // from a flag — a flag value lands in the process argv and in CI logs, and -// the token must never be logged or otherwise surface in an error string -// (§20.2). +// the token must never be logged or otherwise surface in an error string. func grafanaEnv() (url, token string, err error) { url = os.Getenv("GRAFANA_URL") if url == "" { diff --git a/grafana-alertcheck/cmd/grafana-alertcheck/list.go b/grafana-alertcheck/cmd/grafana-alertcheck/list.go index 687e5dd9f..c1d0e8532 100644 --- a/grafana-alertcheck/cmd/grafana-alertcheck/list.go +++ b/grafana-alertcheck/cmd/grafana-alertcheck/list.go @@ -11,11 +11,10 @@ import ( ) // runList reads every rule definition from the ruler endpoint and prints one -// line per rule: its kind, its Folder/Group/Title, and its uid. This is what -// makes the gate runnable end to end before any coverage logic exists (§9 -// rule 4) — it validates auth, the ruler parse, and the shapes Resolve -// matches against, all against a real Grafana. It is also the "did you mean" -// surface §17.2's no-match error points operators at. +// line per rule: its kind, its Folder/Group/Title, and its uid. It validates +// auth, the ruler parse, and the shapes Resolve matches against, all against a +// real Grafana, and it is the surface Resolve's no-match error points operators +// at. func runList(args []string, stdout, stderr io.Writer) int { if len(args) != 0 { fmt.Fprintf(stderr, "list takes no arguments, got %v\n", args) @@ -34,7 +33,7 @@ func runList(args []string, stdout, stderr io.Writer) int { // always terminates. It can still take minutes end-to-end under repeated // transient failures (5 retries * up to 30s backoff each, per call) — an // acceptable wait for an interactive `list`, not for `watch`/`check`, - // which get their own deadlines from `--until`/`to` in P10. + // which get their own deadlines from `--until`/`--to`. src := gate.NewHTTPSource(url, token, gate.SystemClock{}) version, err := src.Version(context.Background()) if err != nil { diff --git a/grafana-alertcheck/cmd/grafana-alertcheck/main.go b/grafana-alertcheck/cmd/grafana-alertcheck/main.go index 73d30f3f3..7ab5d6397 100644 --- a/grafana-alertcheck/cmd/grafana-alertcheck/main.go +++ b/grafana-alertcheck/cmd/grafana-alertcheck/main.go @@ -1,5 +1,5 @@ -// Command grafana-alertcheck is the CLI entry point for the gate: `list` -// (P3), `watch` (record, P10) and `check` (classify, P10). +// Command grafana-alertcheck is the CLI entry point for the gate: `list`, +// `watch` (record) and `check` (classify). package main import ( @@ -16,9 +16,9 @@ const usage = "usage: grafana-alertcheck " // run is the whole of main's testable surface: parse the subcommand, dispatch, // return the process exit code. Exit codes below 2 (pass/violations) belong to -// `check` alone (§20.3, P10); every failure reachable from here — a missing -// subcommand, a bad flag, a transport or auth failure — is a could-not-check -// condition and maps to 2, never to 0 or 1 (H7). +// `check` alone; every failure reachable from here — a missing subcommand, a +// bad flag, a transport or auth failure — is a could-not-check condition and +// maps to 2, never to 0 or 1. // // Requested help (-h/--help) is not a failure — it is the one exception to // that rule. Convention (and every stdlib flag.FlagSet default) is exit 0 to diff --git a/grafana-alertcheck/cmd/grafana-alertcheck/table.go b/grafana-alertcheck/cmd/grafana-alertcheck/table.go index 0e4c93c67..11b72a074 100644 --- a/grafana-alertcheck/cmd/grafana-alertcheck/table.go +++ b/grafana-alertcheck/cmd/grafana-alertcheck/table.go @@ -10,27 +10,25 @@ import ( "github.com/smartcontractkit/chainlink-testing-framework/grafana-alertcheck/internal/gate" ) -// renderTable is §20.2's required human table. It always writes to the -// writer it is given, which the caller (runCheck) always points at -// stderr — the human table is not the machine output §20.2 reserves stdout -// for. +// renderTable is the human table. It always writes to the writer it is given, +// which the caller (runCheck) always points at stderr — stdout is reserved for +// the machine-readable --output json. // // Three sections, in order: // // 1. one line per rule: outcome, BadFor, pollEvery, proved-or-not with the // largest gap; -// 2. one line per Violation (R2): a rule's worst-of outcome does not carry -// the State/Health of the instance that actually caused it — Violation -// does — so this is also where those two columns appear, sorted after -// the rule table rather than folded into it, and it is the only place an -// operator running WITHOUT --output json sees the §12.1 --allow-paused -// hint that Violation.Note already carries (classify.go); -// 3. a footer with the per-rule thresholds and the run-wide numbers §20.2 -// says are the answer to "why" on exit 2: each non-skipped rule's -// maxGap/healthGrace/evalStaleAfter, the global transitionGrace and +// 2. one line per Violation: a rule's worst-of outcome does not carry the +// State/Health of the instance that actually caused it — Violation does — +// so this is also where those two columns appear, sorted after the rule +// table rather than folded into it, and it is the only place an operator +// running WITHOUT --output json sees the --allow-paused hint that +// Violation.Note already carries (classify.go); +// 3. a footer with the numbers that answer "why" on exit 2: each non-skipped +// rule's maxGap/healthGrace/evalStaleAfter, the global transitionGrace and // drainTimeout, and the largest measured clock skew alongside its own -// error bound (RTT/2) — SkewHardLimit is a separate, fixed input -// threshold and is reported next to it, never as if it were that bound. +// error bound (RTT/2) — SkewHardLimit is a separate, fixed input threshold +// and is reported next to it, never as if it were that bound. func renderTable(w io.Writer, res gate.Result) error { alertOf := make(map[string]string, len(res.Verdicts)) for _, v := range res.Verdicts { @@ -77,7 +75,7 @@ func renderTable(w io.Writer, res gate.Result) error { // provedLabel is the table's PROVED column: "yes" for a clean coverage // proof, "no" with the reason and largest gap for an unobservable rule, and // "-" for a rule decide never asked proveCoverage about at all (skipped — -// paused before the window opened, §12). +// paused before the window opened). func provedLabel(cov gate.CoverageResult) string { if cov.Reason == "" && !cov.Unobservable && !cov.Proved { return "-" diff --git a/grafana-alertcheck/cmd/grafana-alertcheck/table_test.go b/grafana-alertcheck/cmd/grafana-alertcheck/table_test.go index 106760fbb..e585e9b61 100644 --- a/grafana-alertcheck/cmd/grafana-alertcheck/table_test.go +++ b/grafana-alertcheck/cmd/grafana-alertcheck/table_test.go @@ -9,10 +9,9 @@ import ( "github.com/smartcontractkit/chainlink-testing-framework/grafana-alertcheck/internal/gate" ) -// TestRenderTable is the golden table test: a fixed Result renders a -// deterministic, ordered rule table, a violations section (R2) and a footer -// carrying the per-rule and global thresholds plus the skew and its bound -// (R3) — with no live Check involved. +// The golden table test: a fixed Result renders a deterministic, ordered rule +// table, a violations section and a footer carrying the per-rule and global +// thresholds plus the skew and its bound — with no live Check involved. func TestRenderTable(t *testing.T) { gapAt := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) res := gate.Result{ @@ -64,13 +63,13 @@ func TestRenderTable(t *testing.T) { t.Fatalf("out = %q, want Zebra's clean row", out) } - // Violations section (R2): must show up even without --output json, and - // must carry the §12.1 --allow-paused hint text verbatim. + // The violations section must show up even without --output json, and must + // carry the --allow-paused hint text verbatim. if !strings.Contains(out, "VIOLATIONS") { t.Fatalf("out = %q, want a VIOLATIONS section", out) } if !strings.Contains(out, "--allow-paused") { - t.Fatalf("out = %q, want the §12.1 --allow-paused hint in the human table", out) + t.Fatalf("out = %q, want the --allow-paused hint in the human table", out) } if !strings.Contains(out, "STATE") || !strings.Contains(out, "HEALTH") { t.Fatalf("out = %q, want the violations table to have STATE and HEALTH columns", out) @@ -79,8 +78,8 @@ func TestRenderTable(t *testing.T) { t.Fatalf("out = %q, want Ape's violation State/Health", out) } - // Footer (R3): per-rule thresholds, global thresholds, and skew with its - // own bound rather than the fixed hard limit. + // The footer: per-rule thresholds, global thresholds, and skew with its own + // bound rather than the fixed hard limit. if !strings.Contains(out, "Ape Alert: maxGap=1m0s healthGrace=2m0s evalStaleAfter=1m0s") { t.Fatalf("out = %q, want Ape's per-rule thresholds", out) } @@ -88,7 +87,7 @@ func TestRenderTable(t *testing.T) { t.Fatalf("out = %q, want Zebra's per-rule thresholds", out) } if strings.Contains(out, "Paused Alert: maxGap") { - t.Fatalf("out = %q, a skipped rule must not report thresholds it never had (§12)", out) + t.Fatalf("out = %q, a skipped rule must not report thresholds it never had", out) } if !strings.Contains(out, "global: transitionGrace=5m0s (source: Ape Alert (for=5m)) drainTimeout=2m0s") { t.Fatalf("out = %q, want the global thresholds line", out) @@ -104,9 +103,9 @@ func TestRenderTable(t *testing.T) { } } -// TestProvedLabel_Skipped pins the "-" case: a rule decide never asked -// proveCoverage about (paused before the window opened, §12) has an empty -// CoverageResult and must not be reported as either proved or unobservable. +// The "-" case: a rule decide never asked proveCoverage about (paused before +// the window opened) has an empty CoverageResult and must not be reported as +// either proved or unobservable. func TestProvedLabel_Skipped(t *testing.T) { if got := provedLabel(gate.CoverageResult{}); got != "-" { t.Fatalf("provedLabel(zero value) = %q, want \"-\"", got) diff --git a/grafana-alertcheck/cmd/grafana-alertcheck/watch.go b/grafana-alertcheck/cmd/grafana-alertcheck/watch.go index f3ad57d92..df2c50029 100644 --- a/grafana-alertcheck/cmd/grafana-alertcheck/watch.go +++ b/grafana-alertcheck/cmd/grafana-alertcheck/watch.go @@ -15,19 +15,17 @@ const watchUsage = "usage: grafana-alertcheck watch --out [--pidfile F] [ // runWatch is the record step's entire CLI surface, split in two by one flag // set — gate.DaemonChildFlag ("--daemon-child") and gate.ReadyFDFlag -// ("--ready-fd") select which side of P6's parent/child split this -// invocation is: +// ("--ready-fd") select which side of the parent/child split this invocation +// is: // // - without them: the record command an operator types. It parses --out, // --alerts and the rest, builds a gate.WatchConfig and calls gate.Watch, // which resolves, records the first observation of every rule, and -// detaches the recorder before returning (§4.3). +// detaches the recorder before returning. // - with them: the detached recorder itself. gate.Watch's own childArgs // (watch_unix.go) is the only thing that ever sets them — an operator -// never types "--daemon-child" and it does not appear in watchUsage — -// and this dispatches straight to gate.RunDaemonChild. P6's integration -// test already covers the spawn; this is the one new test P10 owns: that -// seeing the flag reaches RunDaemonChild. +// never types "--daemon-child" and it does not appear in watchUsage — and +// this dispatches straight to gate.RunDaemonChild. // // Both flags live in the SAME flag set as the operator-facing ones rather // than a second, hidden set: the child is started with childArgs' exact @@ -106,11 +104,10 @@ func runWatch(args []string, stdin io.Reader, stdout, stderr io.Writer) int { return 0 } -// runDaemonChild is the detached recorder's whole entry point (P6's -// obligation on this phase). Its stdout and stderr are already the daemon -// log file — spawnChild (watch_unix.go) redirects both before Start — so -// writing to stderr here lands exactly where waitForChildReady's failure -// path quotes from. +// runDaemonChild is the detached recorder's whole entry point. Its stdout and +// stderr are already the daemon log file — spawnChild (watch_unix.go) +// redirects both before Start — so writing to stderr here lands exactly where +// waitForChildReady's failure path quotes from. func runDaemonChild(out, until string, concurrency, readyFD int, stderr io.Writer) int { url, token, err := grafanaEnv() if err != nil { diff --git a/grafana-alertcheck/cmd/grafana-alertcheck/watch_test.go b/grafana-alertcheck/cmd/grafana-alertcheck/watch_test.go index 267a312a2..6d8c8c1a6 100644 --- a/grafana-alertcheck/cmd/grafana-alertcheck/watch_test.go +++ b/grafana-alertcheck/cmd/grafana-alertcheck/watch_test.go @@ -7,9 +7,8 @@ import ( "testing" ) -// TestRunWatch_FlagValidation is the record step's flag-validation matrix. -// Every case fails inside gate.WatchConfig.validate() (P6) or before it, so -// none needs a reachable Grafana. +// The record step's flag-validation matrix. Every case fails inside +// gate.WatchConfig.validate() or before it, so none needs a reachable Grafana. func TestRunWatch_FlagValidation(t *testing.T) { tests := []struct { name string @@ -58,9 +57,8 @@ func TestRunWatch_FlagValidation(t *testing.T) { } } -// TestRunWatch_DaemonChildDispatch pins P6's obligation on this phase: seeing -// gate.DaemonChildFlag must dispatch to gate.RunDaemonChild, and the flag -// must never appear in watchUsage (an operator never types it). +// Seeing gate.DaemonChildFlag must dispatch to gate.RunDaemonChild, and the +// flag must never appear in watchUsage (an operator never types it). func TestRunWatch_DaemonChildDispatch(t *testing.T) { t.Setenv("GRAFANA_URL", "http://example.invalid") t.Setenv("GRAFANA_TOKEN", "test-token") diff --git a/grafana-alertcheck/internal/gate/check.go b/grafana-alertcheck/internal/gate/check.go index 710bedef9..0f21b4d86 100644 --- a/grafana-alertcheck/internal/gate/check.go +++ b/grafana-alertcheck/internal/gate/check.go @@ -11,46 +11,29 @@ import ( "time" ) -// Obligations this phase leaves for P10, carried forward the way P6 and P7 -// carried theirs so a later review has something concrete to check against: +// Check returns (Result, error) and no exit code: the code is a presentation +// decision the CLI makes. err != nil is exit 2 unconditionally, even alongside +// real violations; violations with err == nil is exit 1; neither is exit 0. // -// - Exit codes are the CLI's (§20.3, §9.1, H6/H7). Check returns -// (Result, error) and nothing else: err != nil is exit 2 unconditionally, -// never 0 and never 1, even alongside real violations; len(Violations) > 0 -// with err == nil is exit 1; both empty is exit 0. Check deliberately does -// not return a code, because a code is a presentation decision and the -// library must not make it. -// - §12.1 wants the paused rule AND --allow-paused both named to the user. -// decide names both in the shortfall Violation's Note (classify.go), and -// P10's renderer prints every Violation, including Note, in the human -// table — not only in --output json. -// - Config.Notes carries the running commentary (§13.2's planned run time, -// the countdown, the blind-interval warning). §20.2 puts the human output -// on stderr and reserves stdout for --output json, so the CLI must pass -// stderr here. -// - Check never reads the environment. GRAFANA_URL and GRAFANA_TOKEN are -// read by the CLI and passed in as fields, and the token must never reach -// a *flag.FlagSet (§20.2). +// Check never reads the environment either. The URL and token are read by the +// CLI and passed in as fields, and the token must never reach a *flag.FlagSet. // countdownEvery is how often the collection loop reports what it is waiting -// for (§13.2: "then print a countdown at regular intervals"). A silent wait is -// indistinguishable from a hung process, and the wait after `to` is the -// longest silence in the whole run. +// for. A silent wait is indistinguishable from a hung process, and the wait +// after `to` is the longest silence in the whole run. const countdownEvery = 30 * time.Second -// recorderStopTimeout bounds §4.4 step 3, the wait for the recorder's exit. -// Not in the source plan's table of values — a judgment call, on the same -// reasoning as childReadyTimeout (P6): everything the recorder does after -// SIGTERM is local (finish the in-flight write, append the sentinel, fsync) -// and an in-flight poll aborts through the child's own context, so the real -// figure is milliseconds. Loose enough for an overloaded runner, and a -// timeout is a hard error rather than a longer wait — a log a writer may -// still hold cannot be read at all (§4.4 step 4). +// recorderStopTimeout bounds the wait for the recorder's exit. Everything the +// recorder does after SIGTERM is local (finish the in-flight write, append the +// sentinel, fsync) and an in-flight poll aborts through the child's own +// context, so the real figure is milliseconds; this is loose enough for an +// overloaded runner. The timeout is a hard error rather than a longer wait — a +// log a writer may still hold cannot be read at all. const recorderStopTimeout = 30 * time.Second // recorderStopPoll is how often that wait re-checks the pid. There is no // wait(2) available: the recorder is a detached session leader, not this -// process's child (P6), so its exit can only be observed by polling. +// process's child, so its exit can only be observed by polling. const recorderStopPoll = 100 * time.Millisecond // Config is check's whole input. It is the CLI's view of a run, and it is @@ -59,14 +42,13 @@ const recorderStopPoll = 100 * time.Millisecond // never cross that line. type Config struct { // URL and Token are the connection details, read from the environment by - // the CLI and never registered as flags (§20.2). Token never enters the - // pure layer, an error string, or a Result. + // the CLI and never registered as flags. Token never enters the pure layer, + // an error string, or a Result. URL, Token string // Alerts is REQUIRED in single-step mode and must be EMPTY in log mode: - // with a log, the header IS the alert set (§19.1 step 3), and there is - // nothing to compare a second list against. Both directions are encoded, - // resolving the source plan's §19.1 step 1 / step 3 contradiction. + // with a log, the header IS the alert set, and there is nothing to compare + // a second list against. Alerts []string Folder string @@ -76,17 +58,16 @@ type Config struct { AllowPaused bool NodataIsUnobservable bool - // From is the moment the deploy finished and To is the end of the work - // (§7). They are different moments and both come from the work. In - // recorder mode an absent From is a hard error; in single-step mode it - // falls back to the start of this step, with the blind-interval warning - // §4.2 requires. + // From is the moment the deploy finished and To is the end of the work. + // They are different moments and both come from the work. In recorder mode + // an absent From is a hard error; in single-step mode it falls back to the + // start of this step, with a blind-interval warning. From, To time.Time // Log is the path of a recording made by watch; "" selects single-step // mode. PidFile defaults to .pid, the convention watch's parent - // writes (P6) and the only way check can reach the recorder it must stop - // before it may read the log (§4.4 steps 1-4). + // writes and the only way check can reach the recorder it must stop before + // it may read the log. Log string PidFile string @@ -94,16 +75,15 @@ type Config struct { // --poll-interval flag. In log mode the cadence comes from the header — // the cadence the recording actually used — and a second authority would // let an operator silently widen maxGap over evidence that was recorded at - // a different rate (P5, "two authorities"); in single-step mode the same - // process records and classifies, so §5's default is the only cadence - // there is. + // a different rate; in single-step mode the same process records and + // classifies, so the default cadence is the only cadence there is. Concurrency int Clock Clock // Notes is where the shell prints what an operator has to see while the // run is in progress: the planned run time, the grace and its source, the // countdown, the blind-interval warning. nil discards them. The library - // renders no table — the CLI owns presentation (§20.2). + // renders no table — the CLI owns presentation. Notes io.Writer } @@ -123,7 +103,7 @@ func (cfg Config) withDefaults() Config { return cfg } -// namedAlerts returns the alert names that survive §17.3's trim-and-discard, +// namedAlerts returns the alert names that survive Resolve's trim-and-discard, // so validation counts what Resolve will actually see rather than what the // caller happened to pass (a file ending in a newline yields an empty line). func (cfg Config) namedAlerts() []string { @@ -139,12 +119,12 @@ func (cfg Config) namedAlerts() []string { // Check is the I/O shell: HTTP, signals, the pidfile, file reads, the // countdown print. Every correctness question it touches is answered // elsewhere — by proveCoverage and decide, which are pure — and that split is -// the most important seam in the project (§2). Check therefore needs two +// the most important seam in the project. Check therefore needs two // integration tests; decide carries the suite. // -// H7 governs the return: a pass is exactly len(Violations) == 0 && err == nil. -// Every error path below leaves err non-nil, and no path anywhere in this file -// converts an error into an empty Result with a nil error. +// A pass is exactly len(Violations) == 0 && err == nil. Every error path below +// leaves err non-nil, and no path anywhere in this file converts an error into +// an empty Result with a nil error. func Check(ctx context.Context, cfg Config) (Result, error) { cfg = cfg.withDefaults() if err := cfg.validate(); err != nil { @@ -152,32 +132,31 @@ func Check(ctx context.Context, cfg Config) (Result, error) { } // The Source is built here and injected into check() so every behaviour // below is testable against a scripted fake — the same seam prepareWatch - // uses (P6), and the reason this file needs no test-only setter. + // uses, and the reason this file needs no test-only setter. return check(ctx, cfg, NewHTTPSource(cfg.URL, cfg.Token, cfg.Clock)) } -// validate is §19.1 step 1. It runs before any network call, so a -// configuration mistake costs nothing and, more importantly, is never -// discovered after a ten-minute wait. +// validate runs before any network call, so a configuration mistake costs +// nothing and, more importantly, is never discovered after a ten-minute wait. func (cfg Config) validate() error { if cfg.URL == "" { return errors.New("check: no grafana url") } if cfg.To.IsZero() { - return errors.New("check: no `to`: the end of the window is required (§7)") + return errors.New("check: no `to`: the end of the window is required") } named := cfg.namedAlerts() if cfg.Log == "" { - // §19.1 step 1: an empty Alerts is an error — but only without a log. + // An empty Alerts is an error — but only without a log. if len(named) == 0 { return errors.New("check: no alert names given and no recorded log to take them from") } } else if len(named) > 0 { - // §19.1 step 3, the other direction: the alert set comes from the log. - // Accepting both would mean reconciling two sets, which is the subset - // arithmetic the source plan removes by making the log the one source. - return fmt.Errorf("check: --alerts is refused with a recorded log: %s already names the alert set it recorded (§19.1 step 3)", cfg.Log) + // The other direction: with a log, the alert set comes from the log. + // Accepting both would mean reconciling two sets, which the log being + // the one source removes entirely. + return fmt.Errorf("check: --alerts is refused with a recorded log: %s already names the alert set it recorded", cfg.Log) } now := cfg.Clock.Now() @@ -188,13 +167,13 @@ func (cfg Config) validate() error { from := cfg.From switch { case from.IsZero() && cfg.Log != "": - // §7, and never a warning-and-continue: falling back to the start of - // the check step reinstates exactly the blind interval the recorder - // exists to remove, which is the fail-open shape this design refuses. - return errors.New("check: no `from` in recorder mode: the deploy step must emit a completion timestamp (§7)") + // Never a warning-and-continue: falling back to the start of the check + // step reinstates exactly the blind interval the recorder exists to + // remove, which is the fail-open shape this design refuses. + return errors.New("check: no `from` in recorder mode: the deploy step must emit a completion timestamp") case from.IsZero(): // Single-step only. The caller sees the resulting blind interval named - // exactly, once the first observation has fixed its end (§4.2). + // exactly, once the first observation has fixed its end. from = now } @@ -202,40 +181,39 @@ func (cfg Config) validate() error { return fmt.Errorf("check: `to` %s is before `from` %s", cfg.To.Format(time.RFC3339), from.Format(time.RFC3339)) } if from.After(now.Add(fromFutureTolerance)) { - return fmt.Errorf("check: `from` %s is more than %s ahead of this runner's clock %s (§7)", + return fmt.Errorf("check: `from` %s is more than %s ahead of this runner's clock %s", from.Format(time.RFC3339), fromFutureTolerance, now.Format(time.RFC3339)) } - // A `to` already in the past is not a special mode WITH a log (§7, §24.3): - // the collection loop's condition is simply already true and the evidence - // is classified immediately. Without one it is a different thing entirely - // — a request to prove a window that nothing observed. Refusing it is not + // A `to` already in the past is not a special mode WITH a log: the + // collection loop's condition is simply already true and the evidence is + // classified immediately. Without one it is a different thing entirely — a + // request to prove a window that nothing observed. Refusing it is not // pedantry: the coverage window would end before the first observation, // every heartbeat gap inside it would measure negative, and the run would // report a proved window it never saw. if cfg.Log == "" && !cfg.To.After(now) { - return fmt.Errorf("check: `to` %s has already passed and there is no recorded log: a window that ended before check started can only be classified from a recording (§4.2)", + return fmt.Errorf("check: `to` %s has already passed and there is no recorded log: a window that ended before check started can only be classified from a recording", cfg.To.Format(time.RFC3339)) } return nil } -// check is Check with the Source injected. Its body is §19.1 steps 1-9, one -// commented block each and in that order, so a review can diff it against the -// source plan line by line. +// check is Check with the Source injected, and its body is one commented block +// per stage of a run, in the order a run performs them. func check(ctx context.Context, cfg Config, src Source) (Result, error) { - // ---- §19.1 step 1 — validate the configuration. ----------------------- + // ---- Validate the configuration. -------------------------------------- // Done by Check before this function is reached, except for the one part // that needs a clock reading kept for later: the single-step fallback for // an absent `from`. from := cfg.From if from.IsZero() { from = cfg.Clock.Now() - fmt.Fprintf(cfg.Notes, "note: no `from` given; the window starts at the start of this step, %s (§4.2)\n", + fmt.Fprintf(cfg.Notes, "note: no `from` given; the window starts at the start of this step, %s\n", from.Format(time.RFC3339)) } - // ---- §19.1 step 2 — resolve the definitions from the ruler API. ------- + // ---- Resolve the definitions from the ruler API. ---------------------- // Unconditional, in BOTH modes. A log's header supplies the alert set as // UIDs and the recording facts, never the rule facts: `for`, // intervalSeconds and Kind always come from a fresh ruler read, which is @@ -249,17 +227,16 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { } allDefs, err := src.Definitions(ctx) if err != nil { - // §19.3 case 2: resolution of the definitions failed. return Result{}, fmt.Errorf("read rule definitions: %w", err) } - // ---- §19.1 step 3 — with a log, validate its identity. ---------------- + // ---- With a log, validate its identity. ------------------------------- // The header is read early — line 1 only, the one line a writer can never // change (ReadLogHeader) — so a wrong URL or a rule that no longer // resolves fails closed NOW rather than after the whole window has // elapsed. It is advisory: the authoritative header comes from the single - // full ReadLog in step 6, after the writer has exited, and the identity is - // validated again against that one. + // full ReadLog once collection is over and the writer has exited, and the + // identity is validated again against that one. var ( resolved []Definition notes []string @@ -272,7 +249,6 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { if cfg.Log != "" { earlyHdr, err = ReadLogHeader(cfg.Log) if err != nil { - // §19.3 case 3. return Result{}, fmt.Errorf("log identity: %w", err) } logHasHdr = true @@ -293,12 +269,12 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { fmt.Fprintf(cfg.Notes, "note: %s\n", n) } - // ---- §19.1 step 4 — derive the timings, print the plan, fit the budget. + // ---- Derive the timings, print them, fit the request budget. ---------- if logHasHdr { // The header is the authority for the cadence actually recorded at; // re-deriving it from defs would compare gaps recorded at an override // cadence against thresholds computed from the default — fail-open in - // the faster-override direction (P5). + // the faster-override direction. rt, gt, err = DeriveTimingsFromLog(earlyHdr, resolved) if err != nil { return Result{}, fmt.Errorf("log identity: %w", err) @@ -316,10 +292,10 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { } // The measurement pass and the budget check belong to single-step mode - // alone (§5.2): in recorder mode watch already took one observation of - // every rule and checked the budget against those measured latencies - // before it detached, and repeating it here would spend a second poll of - // every rule to re-answer a question already answered. + // alone: in recorder mode watch already took one observation of every rule + // and checked the budget against those measured latencies before it + // detached, and repeating it here would spend a second poll of every rule + // to re-answer a question already answered. var ( header Header initial []Poll @@ -328,8 +304,8 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { if !logHasHdr { // StartedAt is fixed before the pass rather than after it, so the // interval it claims to have observed can only be wider than the one - // it really saw — and the first heartbeat's own boundary gap (P7 check - // 3) is what proves that interval, not this timestamp. + // it really saw — and the first heartbeat's own boundary gap is what + // proves that interval, not this timestamp. startedAt := cfg.Clock.Now() active := activeRules(resolved) var measured map[string]time.Duration @@ -341,10 +317,10 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { return Result{}, err } - // Single-step synthesis — how the pure layer stays unconditional (§2). - // The shell builds the Header and later stamps the sentinel itself, so - // P7 checks 1 and 2 run exactly as they do over a recording and no - // mode flag ever reaches proveCoverage or decide. + // Single-step synthesis — how the pure layer stays unconditional. The + // shell builds the Header and later stamps the sentinel itself, so the + // sentinel and from-bounds coverage checks run exactly as they do over + // a recording and no mode flag ever reaches proveCoverage or decide. header = Header{ SchemaVersion: LogSchemaVersion, URL: cfg.URL, @@ -353,31 +329,31 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { Rules: loggedRules(resolved, rt), } if from.Before(startedAt) { - // §4.2/§22.4's declared blind interval: in single-step mode this - // is a warning and a pass, and ONLY here. Recorder mode keeps P7 - // check 2 strict (§22.9), because there the recorder was supposed - // to be watching and the gap means it was not. - fmt.Fprintf(cfg.Notes, "warning: cannot see [%s, %s) — %s before the first observation; the window is classified from %s (§4.2)\n", + // The declared blind interval: in single-step mode this is a + // warning and a pass, and ONLY here. Recorder mode keeps the + // from-bounds coverage check strict, because there the recorder + // was supposed to be watching and the gap means it was not. + fmt.Fprintf(cfg.Notes, "warning: cannot see [%s, %s) — %s before the first observation; the window is classified from %s\n", from.Format(time.RFC3339), startedAt.Format(time.RFC3339), startedAt.Sub(from).Round(time.Second), startedAt.Format(time.RFC3339)) from = startedAt } } - // ---- §19.1 step 5 — apply MinObserved. -------------------------------- - // Its default is the resolved rule count AFTER the collapse (§17.3), which - // is len(resolved) by construction. decide defaults it identically; it is - // resolved here as well so the value the run will judge against is printed - // before the wait rather than inferred from the verdict afterwards. + // ---- Apply MinObserved. ----------------------------------------------- + // Its default is the resolved rule count AFTER duplicate names collapse, + // which is len(resolved) by construction. decide defaults it identically; + // it is resolved here as well so the value the run will judge against is + // printed before the wait rather than inferred from the verdict afterwards. minObserved := cfg.MinObserved if minObserved == 0 { minObserved = len(resolved) } fmt.Fprintf(cfg.Notes, "min-observed: %d of %d resolved rule(s)\n", minObserved, len(resolved)) - // ---- §19.1 step 6 — collect the evidence. ----------------------------- + // ---- Collect the evidence. -------------------------------------------- // Collect ONLY. No classification happens here and there is no early exit, - // even once a violation is certain (H5, §19.2): the loop always runs to + // even once a violation is certain: the loop always runs to // to + transitionGrace, which is what makes "did the early exit lose the // coverage proof?" a question that cannot be asked. windowEnd := cfg.To.Add(gt.transitionGrace) @@ -388,11 +364,10 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { } collected, err := collectUntil(ctx, cfg, windowEnd, poller) if err != nil { - // §19.3 case 1: the failure limit was exceeded (retryTransport already - // gave every transient failure its backoff), or the context ended. - // Nothing collected is classified — the count is there so an operator - // can tell a run that failed at once from one that failed at minute - // nine. + // The failure limit was exceeded (retryTransport already gave every + // transient failure its backoff), or the context ended. Nothing + // collected is classified — the count is there so an operator can tell + // a run that failed at once from one that failed at minute nine. return Result{}, fmt.Errorf("collect evidence after %d poll(s): %w", len(collected), err) } @@ -401,10 +376,10 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { sentinel *time.Time ) if logHasHdr { - // §4.4 steps 2-4, in this order and no other: signal the writer, wait - // for its exit, and only THEN read the log once. A log read while a - // writer can still append can only yield a shorter window than the one - // that was actually recorded. + // In this order and no other: signal the writer, wait for its exit, + // and only THEN read the log once. A log read while a writer can still + // append can only yield a shorter window than the one that was + // actually recorded. heldLog, err := stopRecorder(ctx, cfg) if err != nil { return Result{}, err @@ -442,23 +417,23 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { polls = append(polls, collected...) // The shell stamps the sentinel itself, when the collection loop // exits: by construction that is at or after to + transitionGrace, so - // P7 check 1 passes for the same reason a clean recorder stop does, - // and for no other. + // the sentinel check passes for the same reason a clean recorder stop + // does, and for no other. stoppedAt := cfg.Clock.Now() sentinel = &stoppedAt } - // ---- §19.1 step 7 — the drain wait. ----------------------------------- - // The last instance of the liveness check (§14.6): did this rule evaluate - // through the end of the window? It is I/O and it is deliberately NOT part - // of proveCoverage — adding it there would put HTTP inside the pure layer - // and destroy the seam §2 depends on. + // ---- The drain wait. -------------------------------------------------- + // The last instance of the liveness check: did this rule evaluate through + // the end of the window? It is I/O and it is deliberately NOT part of + // proveCoverage — adding it there would put HTTP inside the pure layer and + // destroy the seam this design depends on. drained, err := drainWait(ctx, cfg, src, resolved, header.pausedAtStart(), rt, polls, windowEnd, gt.drainTimeout) if err != nil { return Result{}, err } - // ---- §19.1 step 8 — classify. ----------------------------------------- + // ---- Classify. -------------------------------------------------------- pol := Policy{ States: cfg.States, Preexisting: cfg.Preexisting, @@ -471,33 +446,34 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { result, decideErr := decide(header, polls, sentinel, resolved, rt, gt, pol) result, drainErr := mergeDrainTimeouts(result, drained) - // ---- §19.1 step 9 — return Result. ------------------------------------ + // ---- Return the Result. ----------------------------------------------- // Both errors are joined rather than one shadowing the other: each names // rules the other does not, and on exit 2 that list IS the answer to - // "why". H7 needs only that err be non-nil when either fired. + // "why". return result, errors.Join(decideErr, drainErr) } -// resolveFromLog turns a log header into the resolved definitions, and is -// §19.1 step 3's identity check in practice. Three things are verified: the -// URL matches, the schema version matches (ReadLog/ReadLogHeader own that), -// and every header UID still resolves against the fresh ruler read. The alert -// set is TAKEN from the log, never compared — with Alerts required empty in -// log mode there is nothing to compare it against, and §22.4's "different -// alert set" refusal is exactly this URL-and-UID failure. +// resolveFromLog turns a log header into the resolved definitions, and is the +// log's identity check in practice. Three things are verified: the URL +// matches, the schema version matches (ReadLog/ReadLogHeader own that), and +// every header UID still resolves against the fresh ruler read. The alert set +// is TAKEN from the log, never compared — with Alerts required empty in log +// mode there is nothing to compare it against, and refusing a log recorded +// against a different alert set is exactly this URL-and-UID failure. // // Resolving through Resolve, by uid:, rather than by a private lookup, keeps -// one implementation of §17: a header naming a recording or datasource-managed -// rule gets the same specific refusal an operator would, and a header naming -// the same UID twice collapses with a note (DeriveTimingsFromLog rejects that -// case outright, so the note is belt and braces). +// one implementation of the resolution rules: a header naming a recording or +// datasource-managed rule gets the same specific refusal an operator would, +// and a header naming the same UID twice collapses with a note +// (DeriveTimingsFromLog rejects that case outright, so the note is belt and +// braces). // // Only the header-to-defs direction needs checking. The opposite direction // cannot fail here: resolved is BUILT from the header, so no resolved // definition can be absent from it. func resolveFromLog(allDefs []Definition, h Header, cfg Config) ([]Definition, []string, error) { if h.URL != cfg.URL { - return nil, nil, fmt.Errorf("log identity: %s recorded url %q but this run is configured for %q (§19.1 step 3)", + return nil, nil, fmt.Errorf("log identity: %s recorded url %q but this run is configured for %q", cfg.Log, h.URL, cfg.URL) } names := make([]string, 0, len(h.Rules)) @@ -506,16 +482,15 @@ func resolveFromLog(allDefs []Definition, h Header, cfg Config) ([]Definition, [ } resolved, notes, err := Resolve(allDefs, names, "") if err != nil { - return nil, nil, fmt.Errorf("log identity: %s names a rule that no longer resolves: %w (§19.1 step 3)", cfg.Log, err) + return nil, nil, fmt.Errorf("log identity: %s names a rule that no longer resolves: %w", cfg.Log, err) } return resolved, notes, nil } -// activeRules drops the rules whose DEFINITION says paused. They are skipped -// (§12): never polled, never waited for, and reported from the definitions -// alone — a skipped rule has no poll records at all, so it has no heartbeats -// to prove and no IsPaused poll to detect (P6 deviation 4, and the obligation -// it left P7/P8). +// activeRules drops the rules whose DEFINITION says paused. They are skipped: +// never polled, never waited for, and reported from the definitions alone — a +// skipped rule has no poll records at all, so it has no heartbeats to prove +// and no IsPaused poll to detect. func activeRules(defs []Definition) []Definition { out := make([]Definition, 0, len(defs)) for _, d := range defs { @@ -527,8 +502,9 @@ func activeRules(defs []Definition) []Definition { } // activeTimingsOf narrows the timings map to the rules that will actually be -// polled, which is what §5.2's budget is spent on: a skipped rule consumes -// none of the capacity, so counting it would refuse schedules that fit. +// polled, which is what the request budget is spent on: a skipped rule +// consumes none of the capacity, so counting it would refuse schedules that +// fit. func activeTimingsOf(active []Definition, rt map[string]ruleTimings) map[string]ruleTimings { out := make(map[string]ruleTimings, len(active)) for _, d := range active { @@ -538,14 +514,14 @@ func activeTimingsOf(active []Definition, rt map[string]ruleTimings) map[string] } // livePoller is single-step mode's collection engine: the same per-rule -// scheduler and the same Reducer the recorder uses (P4/P6), writing into -// memory instead of a log. Log mode has none — the recorder is doing this -// work in another process — and collectUntil takes a nil poller for it. +// scheduler and the same Reducer the recorder uses, writing into memory +// instead of a log. Log mode has none — the recorder is doing this work in +// another process — and collectUntil takes a nil poller for it. type livePoller struct { src Source reducer *Reducer sched *Scheduler - titles map[string]string // uid -> title: poll by title, select by UID (§14.5) + titles map[string]string // uid -> title: poll by title, select by UID concurrency int } @@ -573,10 +549,10 @@ func newLivePoller(src Source, reducer *Reducer, active []Definition, rt map[str // The successes are NOT kept for the reason watchLoopConfig.pollBatch keeps // its own: those go into a durable log that a later check will read, so // dropping one would turn a single rule's transport failure into a coverage -// gap for the others. Here there is no later reader. A terminal failure -// during collection is exit 2 (§19.3 case 1) and check discards the whole -// collection, so these come back only to let the error say how far the run -// got before it stopped — which is the one part of it an operator can act on. +// gap for the others. Here there is no later reader. A terminal failure during +// collection is exit 2 and check discards the whole collection, so these come +// back only to let the error say how far the run got before it stopped — +// which is the one part of it an operator can act on. func (p *livePoller) poll(ctx context.Context, uids []string) ([]Poll, error) { observed, obsErr := observeAll(ctx, p.src, p.titles, uids, p.concurrency) out := make([]Poll, 0, len(uids)) @@ -590,13 +566,13 @@ func (p *livePoller) poll(ctx context.Context, uids []string) ([]Poll, error) { return out, obsErr } -// collectUntil is §19.1 step 6's loop, shared by both modes. With a poller it +// collectUntil is the collection loop, shared by both modes. With a poller it // polls each rule on its own cadence; with nil it only waits, because in // recorder mode the evidence is being written by another process. Both print // the same countdown, because both are the same silence to an operator -// watching a job (§13.2). +// watching a job. // -// It never classifies and never exits early (H5). +// It never classifies and never exits early. func collectUntil(ctx context.Context, cfg Config, deadline time.Time, p *livePoller) ([]Poll, error) { var ( polls []Poll @@ -643,9 +619,9 @@ func collectUntil(ctx context.Context, cfg Config, deadline time.Time, p *livePo } } -// stopRecorder is §4.4 steps 2 and 3. Nothing here is best-effort: the log may -// not be read until the writer has provably gone, so every failure to reach -// that state is a hard error. +// stopRecorder signals the recorder and waits for it to go. Nothing here is +// best-effort: the log may not be read until the writer has provably gone, so +// every failure to reach that state is a hard error. // // It returns the log held under an exclusive flock. The caller must keep that // file open across ReadLog and close it afterwards — the lock is the proof @@ -654,12 +630,11 @@ func collectUntil(ctx context.Context, cfg Config, deadline time.Time, p *livePo // // Two authorities, and only one of them is evidence: // -// - The PIDFILE says whether a recording was ever started. An absent or -// unparseable one is the load-bearing case, and P6's obligation on this -// phase: it must never read as "there was nothing to stop". The parent -// writes the pidfile only AFTER the child reports that it holds the log -// and is polling, and removes it on every failing path, so a missing one -// means watch failed and this run has no evidence at all. +// - The PIDFILE says whether a recording was ever started, and an absent or +// unparseable one must never read as "there was nothing to stop". The +// parent writes the pidfile only AFTER the child reports that it holds the +// log and is polling, and removes it on every failing path, so a missing +// one means watch failed and this run has no evidence at all. // - The FLOCK says whether a writer exists RIGHT NOW. Nothing removes the // pidfile when a recorder exits cleanly — the parent has long returned and // the child never learns the path — so after a --until run, a supported @@ -673,7 +648,7 @@ func collectUntil(ctx context.Context, cfg Config, deadline time.Time, p *livePo func stopRecorder(ctx context.Context, cfg Config) (*os.File, error) { pid, err := ReadPidFile(cfg.PidFile) if err != nil { - return nil, fmt.Errorf("cannot stop the recorder: %w; a pidfile is written only once a recorder reports that it is running, so an unreadable one means the recording never started (§4.4)", err) + return nil, fmt.Errorf("cannot stop the recorder: %w; a pidfile is written only once a recorder reports that it is running, so an unreadable one means the recording never started", err) } log, err := os.Open(cfg.Log) @@ -690,7 +665,7 @@ func stopRecorder(ctx context.Context, cfg Config) (*os.File, error) { // No writer. Send no signal, whatever the pidfile says — the pid may // belong to somebody else entirely by now. Which of --until, a clean // stop and a death ended the recording is the sentinel's question, - // answered by P7 check 1 over the log this unblocks. + // answered by the coverage proof over the log this unblocks. fmt.Fprintf(cfg.Notes, "note: no writer holds %s; the recorder has already finished\n", cfg.Log) return log, nil } @@ -732,7 +707,7 @@ func stopRecorder(ctx context.Context, cfg Config) (*os.File, error) { } if !cfg.Clock.Now().Before(deadline) { log.Close() - return nil, fmt.Errorf("recorder pid %d still holds %s %s after SIGTERM; refusing to read a log a writer can still append to (§4.4 step 4)", + return nil, fmt.Errorf("recorder pid %d still holds %s %s after SIGTERM; refusing to read a log a writer can still append to", pid, cfg.Log, recorderStopTimeout) } } @@ -743,34 +718,33 @@ func stopRecorder(ctx context.Context, cfg Config) (*os.File, error) { // are genuinely different faults: drain_timeout means the rule is still there // and still behind, rule_absent means it is gone. Collapsing both into // drain_timeout would name the wait instead of the fault, and Reason is a -// published vocabulary that reaches the action's JSON (§19.0). +// published vocabulary that reaches the JSON output. type drainVerdict struct { reason UnobservableReason note string } -// drainWait is §19.1 step 7 and §14.6: the final instance of the liveness -// check, asking each rule the last question — did you evaluate through the end -// of the window? A rule that cannot answer within drainTimeout is -// unobservable, never a pass. +// drainWait is the final instance of the liveness check, asking each rule the +// last question — did you evaluate through the end of the window? A rule that +// cannot answer within drainTimeout is unobservable, never a pass. // // It returns one verdict per rule it could not clear, keyed by UID, which the // caller folds into the Result. It returns an error only for a hard failure of -// the wait itself (§19.3 case 1); a rule that simply never catches up is -// reported, not raised. +// the wait itself; a rule that simply never catches up is reported, not +// raised. // -// Two rules are excluded from the wait before it starts, and both are -// exclusions of work that could not change a verdict: +// Two kinds of rule are excluded before the wait starts, both because draining +// them could not change a verdict: // -// - a rule the HEADER says was already paused when the recording opened -// (§12): it is skipped, it was not evaluating, and it never was — there is -// no evaluation to wait for. The header and not the definition, for -// decide's reason (Header.pausedAtStart): a rule the header says was -// active must be drained or faulted, because a pause somebody applied -// after the window is not evidence about the window; -// - a rule whose last poll says Found == false: P7 check 8 already makes it -// unobservable, so the only thing draining it could add is drainTimeout of -// waiting before the same answer. +// - a rule the HEADER says was already paused when the recording opened: it +// is skipped, it was not evaluating, and it never was — there is no +// evaluation to wait for. The header and not the definition, for decide's +// reason (Header.pausedAtStart): a rule the header says was active must be +// drained or faulted, because a pause somebody applied after the window is +// not evidence about the window; +// - a rule whose last poll says Found == false: the rule-absent coverage +// check already makes it unobservable, so the only thing draining it could +// add is drainTimeout of waiting before the same answer. func drainWait(ctx context.Context, cfg Config, src Source, defs []Definition, pausedAtStart map[string]bool, rt map[string]ruleTimings, polls []Poll, windowEnd time.Time, timeout time.Duration) (map[string]drainVerdict, error) { @@ -817,15 +791,16 @@ func drainWait(ctx context.Context, cfg Config, src Source, defs []Definition, p } rule := stateRuleByUID(obs.Rules, uid) if rule == nil { - // §14.5: a 2xx that parsed and carries no matching rule is an - // authoritative "the rule is gone" — P2 retried every transport - // failure long before this Observation existed. It is knowable - // on the FIRST poll, so waiting the rest of drainTimeout would - // spend two minutes to reach the same verdict under a name that - // describes the wait rather than the fault. + // A 2xx that parsed and carries no matching rule is an + // authoritative "the rule is gone" — the transport retried + // every transient failure long before this Observation + // existed. It is knowable on the FIRST poll, so waiting the + // rest of drainTimeout would spend two minutes to reach the + // same verdict under a name that describes the wait rather + // than the fault. verdicts[uid] = drainVerdict{ reason: ReasonRuleAbsent, - note: fmt.Sprintf("rule %q: absent from the state endpoint during the drain wait; there is no evaluation to wait for (§14.5)", + note: fmt.Sprintf("rule %q: absent from the state endpoint during the drain wait; there is no evaluation to wait for", pending[uid]), } delete(pending, uid) @@ -835,11 +810,11 @@ func drainWait(ctx context.Context, cfg Config, src Source, defs []Definition, p // A paused rule does not evaluate, so this one can never catch // up and the rest of drainTimeout would buy nothing. The reason // stays drain_timeout: UnobservableReason is a published - // vocabulary that reaches the action's JSON (§19.0), and the - // prose below is where the detail belongs. + // vocabulary that reaches the JSON output, and the prose below + // is where the detail belongs. verdicts[uid] = drainVerdict{ reason: ReasonDrainTimeout, - note: fmt.Sprintf("rule %q: paused before it evaluated through %s, so it never will (§14.8)", + note: fmt.Sprintf("rule %q: paused before it evaluated through %s, so it never will", pending[uid], windowEnd.Format(time.RFC3339)), } delete(pending, uid) @@ -858,7 +833,7 @@ func drainWait(ctx context.Context, cfg Config, src Source, defs []Definition, p for uid, title := range pending { verdicts[uid] = drainVerdict{ reason: ReasonDrainTimeout, - note: fmt.Sprintf("rule %q: did not evaluate through %s within the %s drain limit (§19.1 step 7)", + note: fmt.Sprintf("rule %q: did not evaluate through %s within the %s drain limit", title, windowEnd.Format(time.RFC3339), timeout), } } @@ -867,8 +842,8 @@ func drainWait(ctx context.Context, cfg Config, src Source, defs []Definition, p // Re-ask no faster than the tightest cadence among the rules still // pending: a rule evaluating every 60s cannot answer differently 200ms - // later, and hammering it would spend the request budget §5 accounts - // for on nothing. + // later, and hammering it would spend the run's request budget on + // nothing. wait := deadline.Sub(now) for uid := range pending { if every := rt[uid].pollEvery; every > 0 { @@ -897,16 +872,16 @@ func anyPollEvaluatedThrough(polls []Poll, windowEnd time.Time) bool { return false } -// evaluatedThrough is the drain wait's one comparison, and it is cross-domain -// (§16): lastEvaluation is a Grafana timestamp and windowEnd is runner-domain, -// so the Grafana value is translated by its own poll's skew. The skew BOUND is -// then subtracted rather than added — the pessimistic end of the uncertainty — -// so an evaluation that only might have reached the end of the window does not +// evaluatedThrough is the drain wait's one comparison, and it is cross-domain: +// lastEvaluation is a Grafana timestamp and windowEnd is runner-domain, so the +// Grafana value is translated by its own poll's skew. The skew BOUND is then +// subtracted rather than added — the pessimistic end of the uncertainty — so +// an evaluation that only might have reached the end of the window does not // count as one that did. Understating it costs a few more seconds of waiting; // overstating it would pass an unproven window. // // A zero lastEvaluation never satisfies the wait: only a paused rule may -// legitimately report it (§2.3), and a paused rule has nothing to drain. +// legitimately report it, and a paused rule has nothing to drain. func evaluatedThrough(lastEval time.Time, skew, bound time.Duration, windowEnd time.Time) bool { if lastEval.IsZero() { return false @@ -915,19 +890,15 @@ func evaluatedThrough(lastEval time.Time, skew, bound time.Duration, windowEnd t } // mergeDrainTimeouts folds the I/O drain wait's verdicts into the pure layer's -// Result. P7 places this merge "before decide runs"; it cannot be, because -// decide owns proveCoverage and therefore builds the Coverage map itself — so -// the merge happens immediately after, which is the same thing from every -// caller's point of view and keeps decide's signature a pure function of its -// arguments. +// Result. It runs immediately after decide rather than before it, because +// decide owns proveCoverage and therefore builds the Coverage map itself; that +// keeps decide a pure function of its arguments. // // It returns its own error rather than mutating decide's, so neither hides the // other: a run with one rule unobservable from the coverage proof and another -// from the drain wait must name both (H6 — inability beats violation, and it -// beats a second inability being dropped from the message too). The error says -// "at the drain wait" for that reason: the two are joined into one message, and -// two counts under one identical phrase read as a contradiction rather than as -// two findings. +// from the drain wait must name both. The error says "at the drain wait" for +// that reason — the two are joined into one message, and two counts under one +// identical phrase read as a contradiction rather than as two findings. func mergeDrainTimeouts(res Result, drained map[string]drainVerdict) (Result, error) { if len(drained) == 0 { return res, nil diff --git a/grafana-alertcheck/internal/gate/check_process.go b/grafana-alertcheck/internal/gate/check_process.go index 580a2a143..86dd73148 100644 --- a/grafana-alertcheck/internal/gate/check_process.go +++ b/grafana-alertcheck/internal/gate/check_process.go @@ -6,7 +6,7 @@ import ( "syscall" ) -// signalRecorder asks the recorder to stop (§4.4 step 2). +// signalRecorder asks the recorder to stop. // // The caller must have established that a writer is alive — by taking the // log's flock and being refused — before it calls this. Nothing removes the diff --git a/grafana-alertcheck/internal/gate/check_test.go b/grafana-alertcheck/internal/gate/check_test.go index 8cd94468f..d13914a8e 100644 --- a/grafana-alertcheck/internal/gate/check_test.go +++ b/grafana-alertcheck/internal/gate/check_test.go @@ -21,7 +21,7 @@ import ( // from those three numbers, and the tests assert against them by name rather // than by magic constant: // -// pollEvery 30s (§5: intervalSeconds/2) +// pollEvery 30s (intervalSeconds/2) // maxGap 60s (2 x pollEvery) // healthGrace 60s (max(maxGap, interval)) // evalStaleAfter 120s (2 x interval) @@ -99,9 +99,9 @@ var _ Source = (*checkSource)(nil) // checkStateRule builds one state-endpoint rule whose totals agree with the // instances it carries. That agreement is load-bearing: a totals map claiming -// normal instances that the instance list does not contain fails §3.2's -// verification (VerifyNormalInstancesVisible), which is a different failure -// from the one most of these tests are about. +// normal instances that the instance list does not contain fails +// VerifyNormalInstancesVisible, which is a different failure from the one most +// of these tests are about. func checkStateRule(lastEval time.Time, insts ...Instance) StateRule { totals := map[string]int{} for _, i := range insts { @@ -143,7 +143,7 @@ func baseConfig(t *testing.T, clock Clock) Config { func notesOf(cfg Config) string { return cfg.Notes.(*strings.Builder).String() } // --------------------------------------------------------------------------- -// §19.1 step 1 — configuration validation +// Configuration validation // --------------------------------------------------------------------------- func TestCheckValidateRejectsBadConfigurations(t *testing.T) { @@ -173,7 +173,7 @@ func TestCheckValidateRejectsBadConfigurations(t *testing.T) { wantErr: "no `to`", }, { - // §19.1 step 1: an empty Alerts is an error — but only without a log. + // An empty Alerts is an error — but only without a log. name: "single-step without alerts", mutate: func(c *Config) {}, wantErr: "no alert names given", @@ -185,13 +185,13 @@ func TestCheckValidateRejectsBadConfigurations(t *testing.T) { wantErr: "no alert names given", }, { - // §19.1 step 3, the other direction: the log names the alert set. + // The other direction: with a log, the log names the alert set. name: "log mode with alerts", mutate: func(c *Config) { c.Log = "log.jsonl"; c.Alerts = []string{"A"} }, wantErr: "--alerts is refused with a recorded log", }, { - // §7 — never a warning-and-continue. + // Never a warning-and-continue. name: "log mode without from", mutate: func(c *Config) { c.Log = "log.jsonl"; c.From = time.Time{} }, wantErr: "the deploy step must emit a completion timestamp", @@ -238,9 +238,9 @@ func TestCheckValidateRejectsBadConfigurations(t *testing.T) { } } -// A past `to` WITH a log is explicitly not a special mode (§7, §24.3): the -// collection loop's condition is already true and the evidence classifies -// immediately. No branch, and no refusal. +// A past `to` WITH a log is not a special mode: the collection loop's condition +// is already true and the evidence classifies immediately. No branch, and no +// refusal. func TestCheckValidateAcceptsAPastToWithALog(t *testing.T) { cfg := Config{ URL: "https://grafana.example.com", @@ -273,7 +273,7 @@ func TestCheckSingleStepCleanWindowPasses(t *testing.T) { if err != nil { t.Fatalf("check() = %v, want nil\nnotes:\n%s", err, notesOf(cfg)) } - // H7: a pass is exactly this shape. + // A pass is exactly this shape. if len(res.Violations) != 0 { t.Fatalf("Violations = %+v, want none", res.Violations) } @@ -284,7 +284,7 @@ func TestCheckSingleStepCleanWindowPasses(t *testing.T) { t.Fatalf("Coverage = %+v, want proved", cov) } - // The collection loop ran to to+transitionGrace and no further (H5). + // The collection loop ran to to+transitionGrace and no further. windowEnd := cfg.To.Add(checkGrace) if clock.Now().Before(windowEnd) { t.Errorf("stopped collecting at %s, before to+grace %s", clock.Now(), windowEnd) @@ -297,15 +297,13 @@ func TestCheckSingleStepCleanWindowPasses(t *testing.T) { t.Errorf("polled %d times, want at least the ~13 a full 6-minute window at 30s implies", got) } if notes := notesOf(cfg); !strings.Contains(notes, "planned run time") { - t.Errorf("§13.2 requires the planned run time at start; notes were:\n%s", notes) + t.Errorf("the planned run time must be printed at start; notes were:\n%s", notes) } } -// §22.2: the collapse-note-plus-satisfied-MinObserved path (resolve_test.go's -// TestResolve_CollapseByUIDGivesNoteNotError and -// TestResolve_MinObservedCountIsPostCollapse) is proven only at Resolve() -// directly; this drives the same shape through check() end to end — the two -// input names must collapse to one verdict, the run must pass, and the +// resolve_test.go proves the collapse-note-plus-satisfied-MinObserved path at +// Resolve() directly; this drives the same shape through check() end to end — +// the two input names must collapse to one verdict, the run must pass, and the // collapse note must reach the run's own notes, not just Resolve()'s return // value. func TestCheckSingleStepDuplicateAlertNamesCollapseWithNote(t *testing.T) { @@ -331,11 +329,11 @@ func TestCheckSingleStepDuplicateAlertNamesCollapseWithNote(t *testing.T) { } } -// §22.1's highest-priority regression: a rule with health=error for the -// whole window is unobservable, exit 2 — using the real "[JD] No Job -// Proposals" capture (testdata/README.md), not a synthetic Poll table, so a -// change in how the real payload shapes health/lastError cannot slip past a -// hand-built fixture that happens to still look right. +// A rule with health=error for the whole window is unobservable, exit 2 — +// driven from the real "[JD] No Job Proposals" capture (testdata/README.md), +// not a synthetic Poll table, so a change in how the real payload shapes +// health/lastError cannot slip past a hand-built fixture that happens to still +// look right. func TestCheckSingleStepContinuousHealthErrorIsUnobservable(t *testing.T) { body := readFixture(t, "state_health_error.json") rules, err := ParseState(body) @@ -358,8 +356,8 @@ func TestCheckSingleStepContinuousHealthErrorIsUnobservable(t *testing.T) { src := newCheckSource(func(_ string, _ int) (Observation, error) { // Every field but LastEvaluation stays exactly as the real capture // shaped it (health=error, the real lastError text, the real Error - // instance); LastEvaluation tracks the poll so staleness (a - // different coverage check, §14) never becomes the actual cause. + // instance); LastEvaluation tracks the poll so staleness — a + // different coverage check — never becomes the actual cause. r := base r.LastEvaluation = clock.Now() return Observation{Rules: []StateRule{r}, GrafanaNow: clock.Now(), Latency: 200 * time.Millisecond}, nil @@ -368,7 +366,7 @@ func TestCheckSingleStepContinuousHealthErrorIsUnobservable(t *testing.T) { res, err := check(context.Background(), cfg, src) if err == nil { - t.Fatalf("check() = nil, want an error: continuous health=error must be unobservable (§22.1, H6/H7)\nnotes:\n%s", notesOf(cfg)) + t.Fatalf("check() = nil, want an error: continuous health=error must be unobservable\nnotes:\n%s", notesOf(cfg)) } if len(res.Verdicts) != 1 || res.Verdicts[0].Outcome != OutcomeUnobservable { t.Fatalf("Verdicts = %+v, want one unobservable verdict", res.Verdicts) @@ -378,8 +376,8 @@ func TestCheckSingleStepContinuousHealthErrorIsUnobservable(t *testing.T) { } } -// H5: a certain violation does not release the runner early, and it does not -// stop the gate reporting exit-1 shape — violations with a nil error. +// A certain violation does not release the runner early, and it does not stop +// the gate reporting exit-1 shape — violations with a nil error. func TestCheckSingleStepFiringInstanceReportsWithoutExitingEarly(t *testing.T) { clock := newVirtualClock(testNow) cfg := baseConfig(t, clock) @@ -403,15 +401,14 @@ func TestCheckSingleStepFiringInstanceReportsWithoutExitingEarly(t *testing.T) { t.Errorf("Outcome = %q, want %q", got, OutcomePersistentlyBad) } if windowEnd := cfg.To.Add(checkGrace); clock.Now().Before(windowEnd) { - t.Errorf("exited early at %s; H5 requires collecting to %s", clock.Now(), windowEnd) + t.Errorf("exited early at %s; collection must run to %s", clock.Now(), windowEnd) } } -// §22.8: "newly_bad at from+30s gives exit 1, but ONLY after -// to+transition_grace." The test above pins H5 for a rule already bad -// before the window opened (persistently_bad); this pins the anti-fail-fast -// case the plan names explicitly — a fresh onset just inside the window -// must not release the runner the instant it is first observed. +// A newly_bad instance at from+30s gives exit 1, but ONLY after +// to+transitionGrace. The test above covers a rule already bad before the +// window opened (persistently_bad); this covers a fresh onset just inside the +// window, which must not release the runner the instant it is first observed. func TestCheckSingleStepNewOnsetDoesNotExitEarly(t *testing.T) { clock := newVirtualClock(testNow) cfg := baseConfig(t, clock) @@ -437,13 +434,13 @@ func TestCheckSingleStepNewOnsetDoesNotExitEarly(t *testing.T) { t.Fatalf("Violations = %+v, want exactly one newly_bad", res.Violations) } if windowEnd := cfg.To.Add(checkGrace); clock.Now().Before(windowEnd) { - t.Errorf("exited early at %s; H5 requires collecting to %s even for a fresh onset at from+30s", clock.Now(), windowEnd) + t.Errorf("exited early at %s; collection must run to %s even for a fresh onset at from+30s", clock.Now(), windowEnd) } } -// §22.9: an ABSENT `from` in single-step mode (as opposed to recorder mode, -// which hard-errors — TestCheckValidateRejectsBadConfigurations's "log mode -// without from") falls back to the start of this check step, with the same +// An ABSENT `from` in single-step mode (as opposed to recorder mode, which +// hard-errors — TestCheckValidateRejectsBadConfigurations's "log mode without +// from") falls back to the start of this check step, with the same // declared-blind-interval warning as an explicit early `from`. func TestCheckSingleStepAbsentFromFallsBackToStepStart(t *testing.T) { clock := newVirtualClock(testNow) @@ -459,16 +456,16 @@ func TestCheckSingleStepAbsentFromFallsBackToStepStart(t *testing.T) { } notes := notesOf(cfg) if !strings.Contains(notes, "no `from` given") { - t.Errorf("want the §4.2 fallback note; notes were:\n%s", notes) + t.Errorf("want the step-start fallback note; notes were:\n%s", notes) } if !res.From.Equal(testNow) { t.Errorf("Result.From = %s, want the step-start fallback %s", res.From, testNow) } } -// §4.2/§22.4: in single-step mode an explicit `from` earlier than the first -// observation is a DECLARED blind interval — a warning and a pass, naming the -// exact interval it cannot see. Recorder mode keeps P7 check 2 strict. +// In single-step mode an explicit `from` earlier than the first observation is +// a DECLARED blind interval — a warning and a pass, naming the exact interval +// it cannot see. Recorder mode keeps the from-bounds coverage check strict. func TestCheckSingleStepFromBeforeFirstObservationWarnsAndPasses(t *testing.T) { clock := newVirtualClock(testNow) cfg := baseConfig(t, clock) @@ -492,9 +489,9 @@ func TestCheckSingleStepFromBeforeFirstObservationWarnsAndPasses(t *testing.T) { } } -// §19.3 case 1: the failure limit was exceeded. The measurement pass succeeds -// and the collection loop then hits a terminal failure, so this exercises the -// path a live run really takes. +// The failure limit was exceeded. The measurement pass succeeds and the +// collection loop then hits a terminal failure, so this exercises the path a +// live run really takes. func TestCheckFailClosedOnExhaustedRetries(t *testing.T) { clock := newVirtualClock(testNow) cfg := baseConfig(t, clock) @@ -517,8 +514,8 @@ func TestCheckFailClosedOnExhaustedRetries(t *testing.T) { } } -// §19.3 case 2: the resolution of the definitions failed. Both shapes — the -// ruler read itself failing, and a name that resolves to nothing. +// The resolution of the definitions failed. Both shapes — the ruler read +// itself failing, and a name that resolves to nothing. func TestCheckFailClosedOnDefinitionResolution(t *testing.T) { t.Run("ruler read fails", func(t *testing.T) { clock := newVirtualClock(testNow) @@ -545,8 +542,8 @@ func TestCheckFailClosedOnDefinitionResolution(t *testing.T) { }) } -// The version gate (§2.7 control 2): an unsupported Grafana is exit 2 before -// anything else is attempted. +// The version gate: an unsupported Grafana is exit 2 before anything else is +// attempted. func TestCheckRefusesUnsupportedGrafanaVersion(t *testing.T) { clock := newVirtualClock(testNow) cfg := baseConfig(t, clock) @@ -559,9 +556,9 @@ func TestCheckRefusesUnsupportedGrafanaVersion(t *testing.T) { } } -// §5.2: the budget is checked against the latencies the measurement pass -// actually measured, and a schedule that cannot fit errors at START rather -// than producing a gap-riddled recording nobody can classify. +// The budget is checked against the latencies the measurement pass actually +// measured, and a schedule that cannot fit errors at START rather than +// producing a gap-riddled recording nobody can classify. func TestCheckSingleStepRefusesAScheduleThatDoesNotFit(t *testing.T) { clock := newVirtualClock(testNow) cfg := baseConfig(t, clock) @@ -577,7 +574,7 @@ func TestCheckSingleStepRefusesAScheduleThatDoesNotFit(t *testing.T) { } for _, want := range []string{"raising concurrency", "raising poll-interval", "watching fewer alerts"} { if !strings.Contains(err.Error(), want) { - t.Errorf("err = %q, want it to name the control %q (§5.1)", err, want) + t.Errorf("err = %q, want it to name the control %q", err, want) } } } @@ -691,16 +688,15 @@ func TestCheckRecorderModeCleanWindowPasses(t *testing.T) { if res.GrafanaVersion != "13.1.0" { t.Errorf("GrafanaVersion = %q, want the recorded one", res.GrafanaVersion) } - // The collection loop still waited out to+transitionGrace (H5) even though - // the recorder had already finished. + // The collection loop still waited out to+transitionGrace even though the + // recorder had already finished. if clock.Now().Before(windowEnd) { t.Errorf("returned at %s, before to+grace %s", clock.Now(), windowEnd) } } -// §19.3 case 3: the identity of the log is not correct. The check runs against -// the header read EARLY, so it fails before the window's wait rather than -// after it. +// The identity of the log is not correct. The check runs against the header +// read EARLY, so it fails before the window's wait rather than after it. func TestCheckFailClosedOnWrongLogIdentity(t *testing.T) { t.Run("different url", func(t *testing.T) { dir := t.TempDir() @@ -736,8 +732,8 @@ func TestCheckFailClosedOnWrongLogIdentity(t *testing.T) { }) } -// §19.3 case 4: the coverage proof failed. A hole in the middle of the -// recording is not saved by healthy data at both ends (§22.4). +// The coverage proof failed: a hole in the middle of the recording is not +// saved by healthy data at both ends. func TestCheckFailClosedOnCoverageGap(t *testing.T) { dir := t.TempDir() windowEnd := testNow.Add(5*time.Minute + checkGrace) @@ -782,13 +778,12 @@ func TestCheckFailClosedOnCoverageGap(t *testing.T) { } } -// §22.4: "an episode fully between the deploy and the start of the check" — -// recorder mode must find this at the LEADING edge of the window too, right -// after `from` (the deploy's completion), not only in the middle -// (TestCheckFailClosedOnCoverageGap above). No poll exists for -// [from, from+3m): whatever happened there is invisible to every per-poll -// check, so only the coverage gap itself can catch it — the reason this -// two-phase recorder model exists at all (§4.2). +// An episode fully between the deploy and the start of the check: recorder +// mode must find this at the LEADING edge of the window too, right after `from` +// (the deploy's completion), not only in the middle +// (TestCheckFailClosedOnCoverageGap above). No poll exists for [from, from+3m), +// so whatever happened there is invisible to every per-poll check and only the +// coverage gap itself can catch it — the reason the recorder exists at all. func TestCheckRecorderModeFindsAGapImmediatelyAfterTheDeploy(t *testing.T) { dir := t.TempDir() windowEnd := testNow.Add(5*time.Minute + checkGrace) @@ -827,14 +822,14 @@ func TestCheckRecorderModeFindsAGapImmediatelyAfterTheDeploy(t *testing.T) { } } -// §19.3 case 5: the drain limit passed. The recording itself is clean, so this -// isolates the drain wait — the rule simply never evaluates through the end of -// the window, and a rule that cannot answer that question is unobservable. +// The drain limit passed. The recording itself is clean, so this isolates the +// drain wait — the rule simply never evaluates through the end of the window, +// and a rule that cannot answer that question is unobservable. func TestCheckFailClosedOnDrainTimeout(t *testing.T) { dir := t.TempDir() windowEnd := testNow.Add(5*time.Minute + checkGrace) - // A 45s lag keeps every poll inside evalStaleAfter (120s), so P7 check 6 - // is silent and only the drain wait can fail. + // A 45s lag keeps every poll inside evalStaleAfter (120s), so the liveness + // coverage check is silent and only the drain wait can fail. logPath := recordedLog(t, dir, "https://grafana.example.com", testNow.Add(-time.Minute), testNow.Add(-time.Minute), windowEnd, windowEnd.Add(30*time.Second), 45*time.Second) writePid(t, logPath+".pid", fmt.Sprintf("%d\n", deadPid(t))) @@ -868,8 +863,8 @@ func TestCheckFailClosedOnDrainTimeout(t *testing.T) { } } -// §14.5: a rule the state endpoint no longer serves is knowable on the FIRST -// drain poll, and the answer is rule_absent — the fault — rather than +// A rule the state endpoint no longer serves is knowable on the FIRST drain +// poll, and the answer is rule_absent — the fault — rather than // drain_timeout, which would only name the wait. It must not spend the whole // drain limit to reach it. func TestCheckDrainWaitNamesADeletedRuleAtOnce(t *testing.T) { @@ -881,9 +876,9 @@ func TestCheckDrainWaitNamesADeletedRuleAtOnce(t *testing.T) { clock := newVirtualClock(testNow) cfg := recorderConfig(t, clock, logPath) - // An authoritative 2xx that parsed and carries no matching rule. P2 - // retried every transport failure long before an Observation exists, so - // this is a deletion, not a hiccup. + // An authoritative 2xx that parsed and carries no matching rule. The + // transport retried every transient failure long before an Observation + // exists, so this is a deletion, not a hiccup. src := newCheckSource(func(_ string, _ int) (Observation, error) { return Observation{GrafanaNow: clock.Now()}, nil }) @@ -1016,8 +1011,7 @@ func TestCheckHeaderPausedRuleStaysSkipped(t *testing.T) { t.Fatalf("NewWriter: %v", err) } // Named in the header, is_paused true, and no poll records at all — the - // shape watch writes for a rule paused before the window opened (P6 - // deviation 4). + // shape watch writes for a rule paused before the window opened. if err := w.WriteHeader(Header{ URL: "https://grafana.example.com", GrafanaVersion: "13.1.0", StartedAt: testNow.Add(-time.Minute), Rules: []LoggedRule{{ @@ -1054,7 +1048,7 @@ func TestCheckHeaderPausedRuleStaysSkipped(t *testing.T) { t.Errorf("Coverage[%s] present, want absent: a skipped rule has no coverage to prove", checkUID) } if len(res.Violations) != 1 { - t.Errorf("Violations = %+v, want the MinObserved shortfall (§12.1)", res.Violations) + t.Errorf("Violations = %+v, want the MinObserved shortfall", res.Violations) } res, err = run(true) @@ -1089,7 +1083,7 @@ func TestCheckDrainWaitConcludesAtOnceOnAPausedRule(t *testing.T) { t.Errorf("polled %d times, want exactly 1: a paused rule can never catch up", got) } if got := res.Coverage[checkUID].Reason; got != ReasonDrainTimeout { - t.Errorf("Reason = %q, want %q — the vocabulary is published (§19.0), so the detail goes in the note", got, ReasonDrainTimeout) + t.Errorf("Reason = %q, want %q — the vocabulary is published, so the detail goes in the note", got, ReasonDrainTimeout) } if !strings.Contains(res.Verdicts[0].Note, "paused before it evaluated through") { t.Errorf("Note = %q, want it to say the rule was paused", res.Verdicts[0].Note) @@ -1099,10 +1093,10 @@ func TestCheckDrainWaitConcludesAtOnceOnAPausedRule(t *testing.T) { } } -// P6's obligation on this phase: an absent or unparseable pidfile is never -// "there was nothing to stop". The parent writes the pidfile only once the -// child reports that it is recording, so a missing one means the recording -// never started — and the log must not be read at all. +// An absent or unparseable pidfile is never "there was nothing to stop". The +// parent writes the pidfile only once the child reports that it is recording, +// so a missing one means the recording never started — and the log must not be +// read at all. func TestCheckRefusesToReadALogItCannotStop(t *testing.T) { windowEnd := testNow.Add(5*time.Minute + checkGrace) @@ -1159,8 +1153,8 @@ func startLockHolder(t *testing.T, logPath string) int { return cmd.Process.Pid } -// §4.4 step 4: a recorder that will not let go of the log means the log may -// still be appended to, and a log a writer can change cannot be read at all. +// A recorder that will not let go of the log means the log may still be +// appended to, and a log a writer can change cannot be read at all. func TestCheckFailsWhenTheRecorderWillNotExit(t *testing.T) { dir := t.TempDir() windowEnd := testNow.Add(5*time.Minute + checkGrace) @@ -1209,9 +1203,9 @@ func TestCheckDoesNotSignalABystanderHoldingAReusedPid(t *testing.T) { } } -// §22.5: a dead pidfile (the recorder process has already exited, holding no -// flock) with NO sentinel in the log — the shape a killed `watch` leaves -// behind — must not hang the stop wait: the flock is free immediately, so +// A dead pidfile (the recorder process has already exited, holding no flock) +// with NO sentinel in the log — the shape a killed `watch` leaves behind — +// must not hang the stop wait: the flock is free immediately, so // check reads the log at once, finds no sentinel, and fails closed. func TestCheckDeadPidWithNoSentinelIsUnobservable(t *testing.T) { dir := t.TempDir() @@ -1254,12 +1248,11 @@ func TestCheckDeadPidWithNoSentinelIsUnobservable(t *testing.T) { } } -// §22.5: "an incomplete last line gives exit 2" is otherwise proven only -// indirectly — log_test.go's TestReadLogRejectsBadLogs pins ReadLog's own -// error, and TestExitCode pins that any non-nil error maps to exit 2 — but -// nothing feeds a genuinely truncated log through check() itself. This closes -// that seam: a raw file with a valid header and poll, then a torn JSON tail, -// exactly what a recorder killed mid-write leaves behind. +// An incomplete last line gives exit 2. log_test.go's TestReadLogRejectsBadLogs +// pins ReadLog's own error and TestExitCode pins that any non-nil error maps to +// exit 2, but only this feeds a genuinely truncated log through check() itself: +// a raw file with a valid header and poll, then a torn JSON tail, exactly what +// a recorder killed mid-write leaves behind. func TestCheckRecorderModeTruncatedLogFailsClosed(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "log.jsonl") @@ -1291,10 +1284,10 @@ func TestCheckRecorderModeTruncatedLogFailsClosed(t *testing.T) { } } -// P5's "two authorities", from check's side: maxGap comes from the cadence the -// header records, never from a re-derivation off intervalSeconds. The -// fail-open direction is the one asserted — a log recorded at 5s on a 60s rule -// must still fail on a hole a re-derived 30s maxGap would have forgiven. +// One authority for the cadence, from check's side: maxGap comes from the +// cadence the header records, never from a re-derivation off intervalSeconds. +// The fail-open direction is the one asserted — a log recorded at 5s on a 60s +// rule must still fail on a hole a re-derived 30s maxGap would have forgiven. func TestCheckDerivesMaxGapFromTheRecordedCadence(t *testing.T) { dir := t.TempDir() windowEnd := testNow.Add(5*time.Minute + checkGrace) @@ -1341,8 +1334,8 @@ func TestCheckDerivesMaxGapFromTheRecordedCadence(t *testing.T) { // The pieces, in isolation // --------------------------------------------------------------------------- -// The drain wait's one comparison is cross-domain (§16), and its uncertainty -// is spent in the fail-closed direction: an evaluation that only MIGHT have +// The drain wait's one comparison is cross-domain, and its uncertainty is +// spent in the fail-closed direction: an evaluation that only MIGHT have // reached the end of the window does not count as one that did. func TestEvaluatedThroughSpendsItsUncertaintyFailingClosed(t *testing.T) { end := testNow @@ -1381,8 +1374,8 @@ func TestEvaluatedThroughSpendsItsUncertaintyFailingClosed(t *testing.T) { } } -// H6 through the merge: a drain timeout on one rule and a coverage failure on -// another must both reach the message. Neither error may shadow the other. +// A drain timeout on one rule and a coverage failure on another must both +// reach the message. Neither error may shadow the other. func TestMergeDrainTimeoutsNamesEveryUnobservableRule(t *testing.T) { res := Result{ Coverage: map[string]CoverageResult{ diff --git a/grafana-alertcheck/internal/gate/classify.go b/grafana-alertcheck/internal/gate/classify.go index d15abaeba..1a9207339 100644 --- a/grafana-alertcheck/internal/gate/classify.go +++ b/grafana-alertcheck/internal/gate/classify.go @@ -7,19 +7,19 @@ import ( "time" ) -// ReasonNodata is decide's own unobservable reason (§10.1/§10.2): proveCoverage -// (P7) deliberately never sets it — health=nodata is a note there, never fatal, -// because escalating it needs Policy.NodataIsUnobservable, and the pure -// coverage layer has no Policy to consult (coverage.go, check 5). decide is -// the seam that DOES have a Policy, so the escalation lives here. +// ReasonNodata is decide's own unobservable reason: proveCoverage deliberately +// never sets it — health=nodata is a note there, never fatal, because +// escalating it needs Policy.NodataIsUnobservable, and the pure coverage layer +// has no Policy to consult (coverage.go, check 5). decide is the seam that DOES +// have a Policy, so the escalation lives here. const ReasonNodata UnobservableReason = "nodata" // Outcome is the verdict of one instance's timeline, and — after decide takes -// the worst across a rule's instances — of the rule itself (§9). It is a -// published JSON output (§19.0): the three fail values stay distinct even -// though v1 maps all three to exit 1, because a later reason string cannot -// recover the information a single "fail" value would have thrown away, and -// because splitting them later would break a published interface for no gain. +// the worst across a rule's instances — of the rule itself. It is a published +// JSON output: the three fail values stay distinct even though v1 maps all +// three to exit 1, because a later reason string cannot recover the +// information a single "fail" value would have thrown away, and because +// splitting them later would break a published interface for no gain. type Outcome string const ( @@ -34,16 +34,15 @@ const ( // PreexistingPolicy governs only the ONE ambiguous case in the outcome table: // an instance that was already bad when the window opened. A newly_bad or -// flapping instance is a fail under every policy (§11.3) — the plan lists -// them among the outcomes that "do not change" — so this type only ever +// flapping instance is a fail under every policy, so this type only ever // changes how `recovered` and `persistently_bad` are judged (isViolation // below). type PreexistingPolicy string const ( - // PreexistingFailUnlessRecovered is the default (§11.7): a preexisting - // instance that clears and stays clear is a pass (`recovered`); one that - // never clears is still a fail (`persistently_bad`). + // PreexistingFailUnlessRecovered is the default: a preexisting instance + // that clears and stays clear is a pass (`recovered`); one that never + // clears is still a fail (`persistently_bad`). PreexistingFailUnlessRecovered PreexistingPolicy = "fail-unless-recovered" // PreexistingFail makes ANY preexisting instance a fail, even one that // recovers — for a user who wants no benefit of the doubt for a @@ -61,9 +60,9 @@ type Violation struct { Alert, RuleUID string Outcome Outcome State State - Health string // raw, reporting-only, like Poll.Health (P1.2a) + Health string // raw, reporting-only, like Poll.Health LastError string - // FirstSeen is the episode's onset, in the runner domain (§16): activeAt + // FirstSeen is the episode's onset, in the runner domain: activeAt // translated by its poll's own skew when the episode opened strictly // inside the window, or `from` itself when the instance was already bad // at window-open (preexisting) — never a raw, untranslated Grafana @@ -74,14 +73,14 @@ type Violation struct { ClearedAt time.Time InstanceLabels map[string]string // Note carries an explanation for a Violation that has no instance - // behind it — the synthetic MinObserved shortfall entry decide emits - // when the deficit exceeds what any named paused rule explains (§12). - // LastError is reporting-only rule state from a real poll and must not - // double as a message field for a Violation that never touched one. + // behind it — the synthetic MinObserved shortfall entry decide emits when + // the deficit exceeds what any named paused rule explains. LastError is + // reporting-only rule state from a real poll and must not double as a + // message field for a Violation that never touched one. Note string } -// RuleVerdict is one rule's worst-of outcome (§9), always present for every +// RuleVerdict is one rule's worst-of outcome, always present for every // resolved rule — Verdicts includes the passes, not only the failures — so a // human reading the table sees every alert that was asked for, not only the // ones that misbehaved. @@ -93,9 +92,9 @@ type RuleVerdict struct { Note string } -// Policy is decide's narrowed, pure-layer view of Config/Cfg (§9's P9 -// comment): the classification knobs and the window, nothing else. No URL, -// no token, no I/O handles — those never reach the pure layer. +// Policy is decide's narrowed, pure-layer view of a Config: the classification +// knobs and the window, nothing else. No URL, no token, no I/O handles — those +// never reach the pure layer. type Policy struct { States []State Preexisting PreexistingPolicy @@ -104,37 +103,35 @@ type Policy struct { From, To time.Time } -// RuleThresholds is one non-skipped rule's resolved coverage thresholds -// (§5/§10.1/§14.1), carried on Result so the CLI's table (P10, §20.2) can -// print the numbers that answer "why" on exit 2 without decide exposing the -// unexported ruleTimings type itself. +// RuleThresholds is one non-skipped rule's resolved coverage thresholds, +// carried on Result so the CLI's table can print the numbers that answer "why" +// on exit 2 without decide exposing the unexported ruleTimings type itself. type RuleThresholds struct { MaxGap time.Duration HealthGrace time.Duration EvalStaleAfter time.Duration } -// GlobalThresholds is the run-wide half of the same information (§13.1, -// §19): transitionGrace and drainTimeout apply once, across every -// non-skipped watched rule, not per rule (globalTimings). +// GlobalThresholds is the run-wide half of the same information: +// transitionGrace and drainTimeout apply once, across every non-skipped +// watched rule, not per rule (globalTimings). type GlobalThresholds struct { TransitionGrace time.Duration - // GraceSource names, and already carries the `for` value of, the rule - // that set TransitionGrace (§13.2 requires printing both). "none" when no - // rule contributed (TransitionGrace is then 0). + // GraceSource names, and already carries the `for` value of, the rule that + // set TransitionGrace — an operator has to see both. "none" when no rule + // contributed (TransitionGrace is then 0). GraceSource string DrainTimeout time.Duration } -// Result is decide's whole answer: everything §20.2's table and the action's -// JSON outputs need. Coverage carries one CoverageResult per non-skipped -// rule — no separate Interval type anywhere in the project (§2's -// simplification table). +// Result is decide's whole answer: everything the human table and the JSON +// output need. Coverage carries one CoverageResult per non-skipped rule — +// there is deliberately no separate Interval type anywhere in the project. type Result struct { From, To time.Time GrafanaVersion string ClockSkew time.Duration // the largest |skew| across every poll decide was given, not only the ones a rule's window actually used - // ClockSkewBound is the skew BOUND (RTT/2, §16) of that SAME poll — not + // ClockSkewBound is the skew BOUND (RTT/2) of that SAME poll — not // the largest bound seen overall, which would pair a wide bound from an // unrelated slow request with the worst skew and misstate how tightly // that skew is actually known. SkewHardLimit is a separate, fixed input @@ -143,9 +140,9 @@ type Result struct { ClockSkewBound time.Duration Coverage map[string]CoverageResult // Thresholds carries one RuleThresholds per rule Coverage also covers — - // every non-skipped rule, keyed by UID. A skipped rule has neither: it - // was never scheduled, so it has no maxGap/healthGrace/evalStaleAfter to - // report (§12). + // every non-skipped rule, keyed by UID. A skipped rule has neither: it was + // never scheduled, so it has no maxGap/healthGrace/evalStaleAfter to + // report. Thresholds map[string]RuleThresholds Global GlobalThresholds Verdicts []RuleVerdict @@ -154,19 +151,19 @@ type Result struct { // episode is one contiguous, policy-bad span of one instance's timeline, // already resolved to the runner domain and clamped to [from, windowEnd]. It -// never crosses a genuine Cleared event (H2): a Vanished marker freezes the -// state instead of closing the episode, which is what keeps a vanish from -// ever reading as a recovery. +// never crosses a genuine Cleared event: a Vanished marker freezes the state +// instead of closing the episode, which is what keeps a vanish from ever +// reading as a recovery. type episode struct { start, end time.Time closedByRealClear bool } // instanceTimeline accumulates one instance's walk across a rule's in-window -// polls. preexisting is decided once, the first time this key is seen bad: -// by the translated ActiveAt against `from` (§16), never by which poll -// happened to report it first — a poll's own cadence is not evidence of when -// the condition actually began (F1/F2). +// polls. preexisting is decided once, the first time this key is seen bad: by +// the translated ActiveAt against `from`, never by which poll happened to +// report it first — a poll's own cadence is not evidence of when the condition +// actually began. type instanceTimeline struct { labels map[string]string preexisting bool @@ -179,27 +176,27 @@ type instanceTimeline struct { episodes []episode } -// runnerTime translates a Grafana-domain timestamp recorded on poll p into -// the runner domain, undoing that poll's own measured skew (§16). GrafanaNow -// and ActiveAt come from the same response, so the same poll's skew applies -// to both. This is the single implementation of that translation for the -// package (same drift argument as pollsForRule, F5): coverage.go's window -// membership test and heartbeat boundary segments call it too, rather than -// each keeping its own copy of `p.GrafanaNow.Add(-p.Skew())` that could -// silently diverge from this one. +// runnerTime translates a Grafana-domain timestamp recorded on poll p into the +// runner domain, undoing that poll's own measured skew. GrafanaNow and +// ActiveAt come from the same response, so the same poll's skew applies to +// both. This is the single implementation of that translation for the package +// (same drift argument as pollsForRule): coverage.go's window membership test +// and heartbeat boundary segments call it too, rather than each keeping its own +// copy of `p.GrafanaNow.Add(-p.Skew())` that could silently diverge from this +// one. func runnerTime(p Poll, grafanaDomain time.Time) time.Time { return grafanaDomain.Add(-p.Skew()) } // classifyRule builds every instance timeline for one rule across -// [from, windowEnd] and reduces them to the rule's worst outcome (§9), its -// merged BadFor, and the Violations the preexisting policy actually charges -// against the run. It is PURE: no I/O, no clock reads (§2) — decide supplies -// windowEnd (to + transitionGrace) rather than this function deriving it, so -// a test can pin the boundary directly. +// [from, windowEnd] and reduces them to the rule's worst outcome, its merged +// BadFor, and the Violations the preexisting policy actually charges against +// the run. It is PURE: no I/O, no clock reads — decide supplies windowEnd +// (to + transitionGrace) rather than this function deriving it, so a test can +// pin the boundary directly. // // polls need not be pre-filtered to this rule, matching proveCoverage's own -// contract (§14.5): selection is by def.UID. +// contract: selection is by def.UID. func classifyRule(def Definition, polls []Poll, from, windowEnd time.Time, badStates map[State]bool, pol PreexistingPolicy) (Outcome, time.Duration, []Violation) { rulePolls := pollsForRule(polls, def.UID) inWindow := inWindowPolls(rulePolls, from, windowEnd) @@ -207,8 +204,8 @@ func classifyRule(def Definition, polls []Poll, from, windowEnd time.Time, badSt timelines := make(map[string]*instanceTimeline) order := make([]string, 0) - // get backfills labels the first time a real Instance is seen (F4): a key - // can be created earlier by a bare Cleared/Vanished marker, which carries + // get backfills labels the first time a real Instance is seen: a key can + // be created earlier by a bare Cleared/Vanished marker, which carries // no labels of its own, and the instance later re-firing must not report // an empty InstanceLabels just because of which event happened to create // the timeline first. @@ -233,7 +230,7 @@ func classifyRule(def Definition, polls []Poll, from, windowEnd time.Time, badSt closeEpisode := func(tl *instanceTimeline, end time.Time, real bool) { // inWindowPolls admits a poll whose translated time is up to its own // skew bound PAST windowEnd (the membership test widens the boundary - // outward, §16). Without this clamp a genuine Cleared event on such a + // outward). Without this clamp a genuine Cleared event on such a // poll would produce an episode.end slightly beyond windowEnd, // contradicting the episode type's own "clamped to // [from, windowEnd]" contract. @@ -276,12 +273,12 @@ func classifyRule(def Definition, polls []Poll, from, windowEnd time.Time, badSt case !tl.seen: tl.seen = true if bad { - // Fail-closed (§16): only call an onset "preexisting" - // when even the worst-case skew error still puts it at - // or before `from`. An onset that might really have - // landed just inside the window must classify as a new - // episode, never earn the `recovered` benefit of the - // doubt it would get if it later clears (F1/F2). + // Fail-closed: only call an onset "preexisting" when + // even the worst-case skew error still puts it at or + // before `from`. An onset that might really have landed + // just inside the window must classify as a new episode, + // never earn the `recovered` benefit of the doubt it + // would get if it later clears. activeAtRunner := runnerTime(p, inst.ActiveAt) tl.preexisting = !activeAtRunner.Add(p.SkewBound()).After(from) if tl.preexisting { @@ -315,7 +312,7 @@ func classifyRule(def Definition, polls []Poll, from, windowEnd time.Time, badSt tl.lastHealth, tl.lastError = p.Health, p.LastError } - // Vanished is a deliberate no-op (H2): freeze whatever badOpen/preexisting + // Vanished is a deliberate no-op: freeze whatever badOpen/preexisting // already holds. An instance that vanishes while bad must stay bad, and // one that vanishes while never having been bad must stay uninteresting. for _, key := range p.Vanished { @@ -358,8 +355,8 @@ func classifyRule(def Definition, polls []Poll, from, windowEnd time.Time, badSt } default: // A genuinely new onset always fails, whether or not it later - // clears within the window (§11.4 point 3): only a PREEXISTING - // condition earns the benefit of `recovered`. + // clears within the window: only a PREEXISTING condition earns + // the benefit of `recovered`. instOutcome = OutcomeNewlyBad } @@ -392,9 +389,9 @@ func classifyRule(def Definition, polls []Poll, from, windowEnd time.Time, badSt } // isViolation decides whether one instance's outcome counts against the run, -// once the preexisting policy is applied. newly_bad and flapping always do -// (§11.3): both contain a genuinely new bad episode, so no policy forgives -// them. recovered and persistently_bad are, by classifyRule's construction, +// once the preexisting policy is applied. newly_bad and flapping always do: +// both contain a genuinely new bad episode, so no policy forgives them. +// recovered and persistently_bad are, by classifyRule's construction, // ALWAYS preexisting (a non-preexisting single episode is newly_bad instead, // regardless of whether it clears) — so these are the only two policy can // change, and isViolation needs no separate preexisting flag to know that. @@ -412,15 +409,16 @@ func isViolation(o Outcome, pol PreexistingPolicy) bool { } // outcomeRank orders outcomes for classifyRule's worst-of reduction across a -// rule's instances (§9). The three fail values, and recovered above clean, -// give it exactly the ordering the table requires — -// "unobservable > {flapping, persistently_bad, newly_bad} > recovered > -// skipped > clean" — with unobservable and skipped applied outside this -// function (decide owns both: unobservable from CoverageResult, skipped from -// Definition.IsPaused). The table does not distinguish among the three fail -// values, so their relative order here (flapping above persistently_bad -// above newly_bad) is an arbitrary but fixed and documented tie-break, not a -// claim that one is worse than another. +// rule's instances: +// +// unobservable > {flapping, persistently_bad, newly_bad} > recovered > +// skipped > clean +// +// with unobservable and skipped applied outside this function (decide owns +// both: unobservable from CoverageResult, skipped from the log header). The +// three fail values are not ranked against each other by anything that reads +// this, so their relative order here is an arbitrary but fixed tie-break, not +// a claim that one is worse than another. func outcomeRank(o Outcome) int { switch o { case OutcomeFlapping: @@ -463,12 +461,11 @@ func mergeDurations(eps []episode) time.Duration { } // pollsForRule filters polls to one rule and sorts them by GrafanaNow, the -// same selection proveCoverage uses (§14.5: selection is by UID, never by -// title) — stable, because two polls sharing a coarse Date header must not -// reorder nondeterministically in a pure function. This is the single -// filter+sort implementation for the package (F5): proveCoverage calls it -// too, rather than keeping its own copy that could silently drift from this -// one's membership test. +// same selection proveCoverage uses (by UID, never by title) — stable, because +// two polls sharing a coarse Date header must not reorder nondeterministically +// in a pure function. This is the single filter+sort implementation for the +// package: proveCoverage calls it too, rather than keeping its own copy that +// could silently drift from this one's membership test. func pollsForRule(polls []Poll, uid string) []Poll { var out []Poll for _, p := range polls { @@ -481,9 +478,9 @@ func pollsForRule(polls []Poll, uid string) []Poll { } // badStateSet turns Policy.States into a lookup set, defaulting to {firing} -// (§13) when the caller leaves States empty — decide applies the default -// itself so a test can pass a zero-value Policy and get v1's real default, -// rather than relying on a CLI layer that does not exist yet. +// when the caller leaves States empty — decide applies the default itself so a +// test can pass a zero-value Policy and get the real default, rather than +// depending on the CLI to have filled it in. func badStateSet(states []State) map[State]bool { if len(states) == 0 { states = []State{StateFiring} @@ -496,17 +493,16 @@ func badStateSet(states []State) map[State]bool { } // decide is the pure seam between the collected evidence and the CLI's exit -// code: nearly every §22 test targets this function, not Check (P9). It -// combines proveCoverage's nine checks with classifyRule's timelines under -// one Policy, and OWNS the H6 mapping: any unobservable rule makes decide -// return a non-nil error, which P10's CLI maps to exit 2 unconditionally -// (H7) — never to 0 or 1, and never suppressed by a real violation found -// alongside it. +// code, and carries nearly the whole test suite because of it. It combines +// proveCoverage's nine checks with classifyRule's timelines under one Policy, +// and owns the inability-beats-violation rule: any unobservable rule makes +// decide return a non-nil error, which the CLI maps to exit 2 unconditionally +// — never to 0 or 1, and never suppressed by a real violation found alongside +// it. // -// Result is fully populated even when the returned error is non-nil: H7's -// "err != nil, the violation list is irrelevant" means the CALLER must not -// use Violations to second-guess the error, not that Result stops being -// useful for the human table on exit 2. +// Result is fully populated even when the returned error is non-nil. A caller +// must not use Violations to second-guess the error, but Result stays useful +// for the human table on exit 2. func decide(h Header, polls []Poll, sentinel *time.Time, defs []Definition, rt map[string]ruleTimings, gt globalTimings, pol Policy) (Result, error) { @@ -557,7 +553,7 @@ func decide(h Header, polls []Poll, sentinel *time.Time, defs []Definition, unobservableNames []string ) - // `skipped` is decided from the header, never from defs (§12). defs are + // `skipped` is decided from the header, never from defs. defs are // resolved after the window has closed, so Definition.IsPaused describes // the present; Header.pausedAtStart describes the moment the recording // opened, which is the only moment "paused before the window opened" can @@ -614,14 +610,14 @@ func decide(h Header, polls []Poll, sentinel *time.Time, defs []Definition, }) } - // MinObserved (§12): default len(defs) after the collapse (already done - // by Resolve before decide ever sees defs). skipped rules count against - // it unless AllowPaused says otherwise. A shortfall counts toward exit 1 - // (§9.1), never exit 2 — decide never returns an error for this — and H7 - // requires it to surface through Violations like any other fail reason, - // so a shortfall always produces at least one, even when no rule is - // paused at all (an operator-supplied MinObserved that simply exceeds - // what could ever be resolved). + // MinObserved defaults to len(defs) after duplicate names collapse + // (already done by Resolve before decide ever sees defs). Skipped rules + // count against it unless AllowPaused says otherwise. A shortfall counts + // toward exit 1, never exit 2 — decide never returns an error for this — + // and it has to surface through Violations like any other fail reason, so + // a shortfall always produces at least one, even when no rule is paused at + // all (an operator-supplied MinObserved that simply exceeds what could ever + // be resolved). counted := watchedCount var attributable []Definition if pol.AllowPaused { @@ -635,10 +631,10 @@ func decide(h Header, polls []Poll, sentinel *time.Time, defs []Definition, if attributed >= shortfall { break } - // §12.1 requires the paused rule and --allow-paused both be - // named to the user; both live in this one Violation, in Note — - // P10's renderer prints Note verbatim rather than re-deriving - // the hint, so the exact wording here is what an operator reads. + // The paused rule and --allow-paused must both be named to the + // user; both live in this one Violation, in Note — the renderer + // prints Note verbatim rather than re-deriving the hint, so the + // exact wording here is what an operator reads. result.Violations = append(result.Violations, Violation{ Alert: def.Title, RuleUID: def.UID, Outcome: OutcomeSkipped, Note: "paused before the window opened; counts against --min-observed unless --allow-paused is set", diff --git a/grafana-alertcheck/internal/gate/classify_test.go b/grafana-alertcheck/internal/gate/classify_test.go index 6522f6e38..017d7ae6b 100644 --- a/grafana-alertcheck/internal/gate/classify_test.go +++ b/grafana-alertcheck/internal/gate/classify_test.go @@ -83,8 +83,7 @@ func TestClassifyRule_NewOnsetInsideWindowIsNewlyBad(t *testing.T) { } } -// TestClassifyRule_NewOnsetThatClearsStillFails pins §11.4 point 3: a -// genuinely new bad episode fails even if it clears again before the window +// A genuinely new bad episode fails even if it clears again before the window // ends — only a PREEXISTING condition earns the benefit of `recovered`. func TestClassifyRule_NewOnsetThatClearsStillFails(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) @@ -134,9 +133,9 @@ func TestClassifyRule_PreexistingThatRecoversIsRecoveredAndNotAViolation(t *test } } -// §22.2's "late condition": bad for 58 of a 60-minute window, clear at -// minute 58, still passes with a large BadFor — never a fail against some -// derived deadline (e.g. "must clear before 90% of the window"). +// The late condition: bad for 58 of a 60-minute window, clear at minute 58, +// still passes with a large BadFor — never a fail against some derived +// deadline (e.g. "must clear before 90% of the window"). func TestClassifyRule_LateRecoveryPassesRegardlessOfHowLateItIs(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) to := from.Add(60 * time.Minute) @@ -205,9 +204,9 @@ func TestClassifyRule_ClearThenBadAgainIsFlapping(t *testing.T) { } } -// §22.2: "a clear and then a second bad state gives flapping, at each -// possible time of the second bad state." A table over where the second -// onset lands — immediately after the clear, mid-window, and right at the +// A clear and then a second bad state gives flapping, wherever the second bad +// state lands. A table over where the second onset falls — immediately after +// the clear, mid-window, and right at the // last instant before windowEnd — closes the boundary this single fixed // timing above cannot. func TestClassifyRule_FlappingAtEveryTimingOfTheSecondOnset(t *testing.T) { @@ -244,7 +243,7 @@ func TestClassifyRule_FlappingAtEveryTimingOfTheSecondOnset(t *testing.T) { } } -// --- H2: vanished is a discontinuity, never a clear --- +// --- vanished is a discontinuity, never a clear --- func TestClassifyRule_VanishedWhileBadStaysPersistentlyBad(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) @@ -259,7 +258,7 @@ func TestClassifyRule_VanishedWhileBadStaysPersistentlyBad(t *testing.T) { } outcome, badFor, viols := classifyRule(def, polls, from, to, defaultBad, PreexistingFailUnlessRecovered) if outcome != OutcomePersistentlyBad { - t.Fatalf("outcome = %v, want persistently_bad: a vanish must never read as a recovery (H2)", outcome) + t.Fatalf("outcome = %v, want persistently_bad: a vanish must never read as a recovery", outcome) } if badFor != to.Sub(from) { t.Fatalf("badFor = %v, want the full window %v: the freeze must hold the episode open to windowEnd", badFor, to.Sub(from)) @@ -375,7 +374,7 @@ func TestClassifyRule_WorstOfMultipleInstancesWins(t *testing.T) { } } -// --- decide(): skipped rules, unobservable (H6), MinObserved, exit mapping (H7) --- +// --- decide(): skipped rules, unobservable, MinObserved, exit mapping --- func TestDecide_SkippedRuleNeverReachesProveCoverage(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) @@ -387,7 +386,7 @@ func TestDecide_SkippedRuleNeverReachesProveCoverage(t *testing.T) { pol := Policy{From: from, To: to, AllowPaused: true} // No polls, no sentinel at all: a heartbeat_gap/no_sentinel misclassification - // here would mean proveCoverage ran for a skipped rule (§4.3's obligation). + // here would mean proveCoverage ran for a skipped rule. // The HEADER is what says paused — decide reads skipped from there, not // from def.IsPaused, which is a post-window reading (Header.pausedAtStart). res, err := decide(pausedHeader(from.Add(-time.Hour), "r1"), nil, nil, defs, rt, gt, pol) @@ -415,16 +414,15 @@ func TestDecide_UnobservableRuleAlwaysReturnsAnError(t *testing.T) { // regardless of anything else. res, err := decide(Header{StartedAt: from.Add(-time.Hour)}, nil, nil, defs, rt, gt, pol) if err == nil { - t.Fatalf("err = nil, want non-nil: H6/H7 require an unobservable rule to always fail the run") + t.Fatalf("err = nil, want non-nil: an unobservable rule must always fail the run") } if len(res.Verdicts) != 1 || res.Verdicts[0].Outcome != OutcomeUnobservable { t.Fatalf("Verdicts = %+v, want exactly one unobservable verdict", res.Verdicts) } } -// TestDecide_UnobservableWinsEvenAlongsideARealViolation pins H6 exactly: -// "Any unobservable rule -> exit 2, no exception, even alongside a real -// newly_bad." +// Any unobservable rule means exit 2, with no exception — even alongside a +// real newly_bad. func TestDecide_UnobservableWinsEvenAlongsideARealViolation(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) to := from.Add(10 * time.Minute) @@ -468,18 +466,17 @@ func TestDecide_UnobservableWinsEvenAlongsideARealViolation(t *testing.T) { t.Fatalf("broken.Outcome = %v, want unobservable", gotBroken) } if gotBad != OutcomeNewlyBad { - t.Fatalf("bad.Outcome = %v, want newly_bad: classification still runs and is still visible in Verdicts (H5)", gotBad) + t.Fatalf("bad.Outcome = %v, want newly_bad: classification still runs and is still visible in Verdicts", gotBad) } if len(res.Violations) == 0 { t.Fatalf("Violations empty, want the newly_bad instance still reported even though the run fails on the unobservable rule") } } -// §22.10: "a clean verdict with a coverage gap ... must never give exit 0", -// and "a recovered verdict and a skipped verdict also need proved coverage -// of the full window." One genuinely unobservable rule ("broken", zero -// polls) alongside a rule with each of the three favorable outcomes — none -// of them may waive the run. +// A clean verdict with a coverage gap must never give exit 0, and recovered +// and skipped verdicts need proved coverage of the full window just as much. +// One genuinely unobservable rule ("broken", zero polls) alongside a rule with +// each of the three favorable outcomes — none of them may waive the run. func TestDecide_UnobservableRuleWinsOverEveryFavorableOutcome(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) to := from.Add(10 * time.Minute) @@ -575,8 +572,8 @@ func TestDecide_UnobservableRuleWinsOverEveryFavorableOutcome(t *testing.T) { } } -// §22.10: the table above puts the coverage gap on a DIFFERENT rule from the -// one with the favorable outcome. This pins the tighter claim: a rule that +// The table above puts the coverage gap on a DIFFERENT rule from the one with +// the favorable outcome. This pins the tighter claim: a rule that // itself recovers, but ALSO itself has a coverage gap, is still overridden to // unobservable — the favorable classification of a rule is never a reason to // skip that same rule's own coverage check. @@ -637,21 +634,20 @@ func TestDecide_CleanWindowIsAPass(t *testing.T) { t.Fatalf("err = %v, want nil", err) } if len(res.Violations) != 0 { - t.Fatalf("Violations = %+v, want none: H7 says a pass is exactly len(Violations)==0 && err==nil", res.Violations) + t.Fatalf("Violations = %+v, want none: a pass is exactly len(Violations)==0 && err==nil", res.Violations) } if res.Verdicts[0].Outcome != OutcomeClean { t.Fatalf("Outcome = %v, want clean", res.Verdicts[0].Outcome) } } -// §22.7's second of the plan's "if only three tests could exist" cases: a -// pause and then an unpause inside the window, with an episode that would +// A pause and then an unpause inside the window, with an episode that would // fire and resolve entirely inside the blind interval. A drain wait alone — // "did the rule eventually evaluate through windowEnd?" — would see // lastEvaluation catch up after the unpause and answer yes, a pass. decide() -// never runs a drain wait (that is check.go's I/O concern, §14.6); this pins -// that proveCoverage's own per-poll checks already refuse the window without -// one, so a live drain wait is not what is saving this case. +// never runs a drain wait (that is check.go's I/O concern); this pins that +// proveCoverage's own per-poll checks already refuse the window without one, +// so a live drain wait is not what is saving this case. func TestDecide_PauseThenUnpauseWithHiddenEpisodeGivesUnobservableNotClean(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) to := from.Add(20 * time.Minute) @@ -671,7 +667,7 @@ func TestDecide_PauseThenUnpauseWithHiddenEpisodeGivesUnobservableNotClean(t *te for ts := pauseStart; !ts.After(pauseEnd); ts = ts.Add(30 * time.Second) { // No fire/resolve is ever observed here: the rule was not // evaluating, so any real episode inside this stretch is invisible - // to every poll (§14.7). + // to every poll. polls = append(polls, Poll{RuleUID: "r1", GrafanaNow: ts, Found: true, Health: "ok", IsPaused: true, LastEvaluation: pauseStart}) } for ts := pauseEnd.Add(30 * time.Second); !ts.After(to); ts = ts.Add(30 * time.Second) { @@ -693,7 +689,7 @@ func TestDecide_PauseThenUnpauseWithHiddenEpisodeGivesUnobservableNotClean(t *te } } -// --- MinObserved shortfall (§12) --- +// --- MinObserved shortfall --- func TestDecide_SkippedOnlyShortfallProducesAViolationWithoutAnError(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) @@ -718,13 +714,13 @@ func TestDecide_SkippedOnlyShortfallProducesAViolationWithoutAnError(t *testing. res, err := decide(pausedHeader(from.Add(-time.Hour), "paused"), polls, &sentinel, defs, rt, gt, pol) if err != nil { - t.Fatalf("err = %v, want nil: a shortfall caused only by a skipped rule is exit 1, not exit 2 (§9.1)", err) + t.Fatalf("err = %v, want nil: a shortfall caused only by a skipped rule is exit 1, not exit 2", err) } if len(res.Violations) != 1 { - t.Fatalf("Violations = %+v, want exactly one: H7 needs the shortfall visible through Violations to keep its equivalence", res.Violations) + t.Fatalf("Violations = %+v, want exactly one: a shortfall must be visible through Violations like any other fail reason", res.Violations) } if v := res.Violations[0]; v.Outcome != OutcomeSkipped || v.RuleUID != "paused" || v.Alert != "Paused" { - t.Fatalf("Violations[0] = %+v, want Outcome=skipped naming the paused rule (§12.1: the message names the paused rule)", v) + t.Fatalf("Violations[0] = %+v, want Outcome=skipped naming the paused rule", v) } if res.Violations[0].Note == "" { t.Fatalf("Violations[0].Note is empty, want an explanation: the shortfall reason must not be smuggled into LastError, " + @@ -732,10 +728,9 @@ func TestDecide_SkippedOnlyShortfallProducesAViolationWithoutAnError(t *testing. } } -// TestDecide_ExplicitMinObservedShortfallWithNoPausedRuleStillProducesAViolation -// pins F3: an operator-supplied MinObserved that exceeds what could ever be -// resolved is still a shortfall, even with zero paused rules to blame it on -// — H7 must not let this silently read as a pass. +// An operator-supplied MinObserved that exceeds what could ever be resolved is +// still a shortfall, even with zero paused rules to blame it on — it must not +// silently read as a pass. func TestDecide_ExplicitMinObservedShortfallWithNoPausedRuleStillProducesAViolation(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) to := from.Add(10 * time.Minute) @@ -758,7 +753,7 @@ func TestDecide_ExplicitMinObservedShortfallWithNoPausedRuleStillProducesAViolat } if len(res.Violations) != 2 { t.Fatalf("Violations = %+v, want two: the shortfall (3-1=2) is not explained by any paused rule, "+ - "so H7 requires it to surface directly rather than pass silently", res.Violations) + "so it must surface directly rather than pass silently", res.Violations) } for _, v := range res.Violations { if v.Outcome != OutcomeSkipped { @@ -846,13 +841,12 @@ func TestDecide_NodataIsANoteByDefault(t *testing.T) { } } -// --- F1/F2 regressions: preexisting is decided by ActiveAt, not poll timing --- +// --- preexisting is decided by ActiveAt, not poll timing --- -// TestClassifyRule_OnsetBetweenFromAndFirstPollIsNewlyBadNotRecovered pins -// F1: an instance whose true onset (ActiveAt) falls strictly inside the -// window — even though the first poll that happens to observe it already -// shows it bad — must never be treated as preexisting. If it then clears, -// the plan requires newly_bad (exit 1), not recovered (exit 0). +// An instance whose true onset (ActiveAt) falls strictly inside the window — +// even though the first poll that happens to observe it already shows it bad — +// must never be treated as preexisting. If it then clears, that is newly_bad +// (exit 1), not recovered (exit 0). func TestClassifyRule_OnsetBetweenFromAndFirstPollIsNewlyBadNotRecovered(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) to := from.Add(10 * time.Minute) @@ -870,13 +864,13 @@ func TestClassifyRule_OnsetBetweenFromAndFirstPollIsNewlyBadNotRecovered(t *test outcome, badFor, viols := classifyRule(def, polls, from, to, defaultBad, PreexistingFailUnlessRecovered) if outcome != OutcomeNewlyBad { t.Fatalf("outcome = %v, want newly_bad: the onset is after `from`, so it is not preexisting even though "+ - "the FIRST in-window poll already observes it bad (F1)", outcome) + "the FIRST in-window poll already observes it bad", outcome) } if len(viols) != 1 || viols[0].Outcome != OutcomeNewlyBad { t.Fatalf("viols = %+v, want one newly_bad violation: a policy=fail-unless-recovered default must still fail this", viols) } if want := clearAt.Sub(onset); badFor != want { - t.Fatalf("badFor = %v, want %v: BadFor must count from the true onset, not from `from` (F1's overcount bug)", badFor, want) + t.Fatalf("badFor = %v, want %v: BadFor must count from the true onset, not from `from`", badFor, want) } } @@ -909,11 +903,9 @@ func TestClassifyRule_OnsetJustBeforeFromIsPreexisting(t *testing.T) { } } -// TestClassifyRule_SkewTranslatesActiveAtAcrossTheWindowBoundary pins F2: a -// poll carrying a nonzero skew must have its ActiveAt (and GrafanaNow) +// A poll carrying a nonzero skew must have its ActiveAt (and GrafanaNow) // translated to the runner domain before comparing against `from` — a raw, -// untranslated comparison would land on the wrong side of the F1 boundary -// check. +// untranslated comparison would land on the wrong side of that boundary. func TestClassifyRule_SkewTranslatesActiveAtAcrossTheWindowBoundary(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) to := from.Add(10 * time.Minute) @@ -937,14 +929,14 @@ func TestClassifyRule_SkewTranslatesActiveAtAcrossTheWindowBoundary(t *testing.T outcome, badFor, _ := classifyRule(def, []Poll{poll, stillBad}, from, to, defaultBad, PreexistingFailUnlessRecovered) if outcome != OutcomePersistentlyBad { - t.Fatalf("outcome = %v, want persistently_bad: a +90s skew must translate ActiveAt back to exactly `from` (F2)", outcome) + t.Fatalf("outcome = %v, want persistently_bad: a +90s skew must translate ActiveAt back to exactly `from`", outcome) } if badFor != to.Sub(from) { t.Fatalf("badFor = %v, want the full window %v", badFor, to.Sub(from)) } } -// --- F4: InstanceLabels must survive a timeline first created by a bare marker --- +// --- InstanceLabels must survive a timeline first created by a bare marker --- func TestClassifyRule_LabelsSurviveWhenTimelineStartsFromAClearedMarker(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) @@ -967,12 +959,11 @@ func TestClassifyRule_LabelsSurviveWhenTimelineStartsFromAClearedMarker(t *testi } if viols[0].InstanceLabels == nil || viols[0].InstanceLabels["instance"] != "a" { t.Fatalf("InstanceLabels = %+v, want {instance: a}: labels must backfill even though the "+ - "timeline was first created by a label-less Cleared marker (F4)", viols[0].InstanceLabels) + "timeline was first created by a label-less Cleared marker", viols[0].InstanceLabels) } } -// TestClassifyRule_ViolationFieldsArePrecise pins FirstSeen/ClearedAt exactly, -// not just that a violation exists (F7). +// FirstSeen/ClearedAt are pinned exactly, not just that a violation exists. func TestClassifyRule_ViolationFieldsArePrecise(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) to := from.Add(10 * time.Minute) @@ -1002,10 +993,9 @@ func TestClassifyRule_ViolationFieldsArePrecise(t *testing.T) { } } -// TestClassifyRule_ClearedEventPastWindowEndClampsToWindowEnd pins the -// episode.end clamp: inWindowPolls admits a poll up to its own skew bound -// past windowEnd (§16's widened membership test), so a genuine Cleared event -// on such a poll must not leave the episode extending beyond windowEnd. +// The episode.end clamp: inWindowPolls admits a poll up to its own skew bound +// past windowEnd, so a genuine Cleared event on such a poll must not leave the +// episode extending beyond windowEnd. func TestClassifyRule_ClearedEventPastWindowEndClampsToWindowEnd(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) to := from.Add(10 * time.Minute) @@ -1069,7 +1059,7 @@ func TestClassifyRule_OnsetJustPastWindowEndIsNewlyBadNotClean(t *testing.T) { } } -// §22.2: "a clear after `to` gives persistently_bad." classifyRule filters +// A clear after `to` gives persistently_bad. classifyRule filters // its input to [from, windowEnd] itself (inWindowPolls), so a Cleared event // GENUINELY past windowEnd — well beyond any skew bound, unlike the clamp // case above — never reaches the timeline at all: the instance is still bad diff --git a/grafana-alertcheck/internal/gate/coverage.go b/grafana-alertcheck/internal/gate/coverage.go index 4eac5eac1..f8c4e1ec4 100644 --- a/grafana-alertcheck/internal/gate/coverage.go +++ b/grafana-alertcheck/internal/gate/coverage.go @@ -5,32 +5,28 @@ import ( "time" ) -// keepLastReason is the instance Reason that check 9 watches for (§10.2). +// keepLastReason is the instance Reason that check 9 watches for. const keepLastReason = "KeepLast" -// Two things this file deliberately does not do, and where they are done -// instead — both were open obligations when P7 was written, and both are now -// discharged: +// Two things this file deliberately leaves to its callers: // -// - §7's second clause, "from more than fromFutureTolerance ahead is a hard -// error", is once-per-run input validation rather than a per-rule -// coverage check, and this function has no error return. Discharged by -// P9: the constant is fromFutureTolerance (schedule.go) and Config.validate -// (check.go) applies it. Check 2 below still owns the first clause, -// "from < StartedAt". -// - A rule paused before the window opened is never scheduled or polled -// (§4.3), so it reaches this function with zero polls and reads as one -// large heartbeat_gap, not as skipped (pinned by -// TestProveCoverage_SkippedRuleWithZeroPollsPinnedAsHeartbeatGap). -// Discharged by P8: decide returns before it ever calls proveCoverage for -// such a rule (classify.go). It reads skipped from the log header -// (Header.pausedAtStart), NOT from Definition.IsPaused — the definitions -// are re-resolved after the window closed, so they cannot answer what was -// paused when it opened. +// - "from more than fromFutureTolerance ahead of the runner's clock" is a +// hard error, but it is once-per-run input validation rather than a +// per-rule coverage check, and this function has no error return. +// Config.validate (check.go) applies it; check 2 below owns only the +// "from < StartedAt" half. +// - A rule paused before the window opened is never scheduled or polled, so +// it would reach this function with zero polls and read as one large +// heartbeat_gap rather than as skipped (pinned by +// TestProveCoverage_SkippedRuleWithZeroPollsPinnedAsHeartbeatGap). decide +// returns before it ever calls proveCoverage for such a rule, reading +// skipped from the log header (Header.pausedAtStart) and NOT from +// Definition.IsPaused — the definitions are re-resolved after the window +// closed, so they cannot answer what was paused when it opened. // UnobservableReason names why proveCoverage could not prove a rule's window. -// It is machine-readable — this reaches the action's JSON outputs, so it is a -// published vocabulary like Outcome (§19.0); prose belongs in Notes. +// It is machine-readable — this reaches the JSON output, so it is a published +// vocabulary like Outcome; prose belongs in Notes. type UnobservableReason string const ( @@ -42,16 +38,16 @@ const ( ReasonStaleEvaluation UnobservableReason = "stale_evaluation" ReasonPausedInWindow UnobservableReason = "paused_in_window" ReasonRuleAbsent UnobservableReason = "rule_absent" - // ReasonDrainTimeout is set by check.go's drain wait (a later phase), - // never by proveCoverage: the wait is I/O and must not be added to this - // pure function — that would put HTTP inside the pure layer and destroy - // the seam §2's architecture depends on. + // ReasonDrainTimeout is set by check.go's drain wait, never by + // proveCoverage: the wait is I/O and must not be added to this pure + // function — that would put HTTP inside the pure layer and destroy the + // seam this design depends on. ReasonDrainTimeout UnobservableReason = "drain_timeout" ) // CoverageResult is proveCoverage's whole answer for one rule. No interval // list: proved-or-not plus the largest gap and where is everything a human -// reads on exit 2, and everything §20.2's table needs. +// reads on exit 2, and everything the rendered table needs. type CoverageResult struct { Proved bool LargestGap time.Duration @@ -64,21 +60,21 @@ type CoverageResult struct { BlindFor time.Duration } -// proveCoverage applies the nine coverage checks (§6, §10, §14) to one rule's -// polls and is PURE: no HTTP, no files, no clock reads — everything it needs -// arrives as an argument, which is what lets §22's tests build []Poll literals -// instead of a fixture server (§2). +// proveCoverage applies the nine coverage checks to one rule's polls and is +// PURE: no HTTP, no files, no clock reads — everything it needs arrives as an +// argument, which is what lets its tests build []Poll literals instead of a +// fixture server. // // polls need not be pre-filtered to this rule: proveCoverage selects by -// def.UID itself, exactly as Reduce selects by UID rather than by title -// (§14.5) — a caller handing it a whole log's polls must not have to -// pre-filter to get a correct answer. +// def.UID itself, exactly as Reduce selects by UID rather than by title — a +// caller handing it a whole log's polls must not have to pre-filter to get a +// correct answer. // // Every check always runs, even once an earlier one has already set // Unobservable: LargestGap and the notes are diagnostics an operator reads on -// exit 2 regardless of which check actually failed (§20.2). Reason names the -// FIRST check, in the order below, that failed; a later failure still adds -// its own Note. +// exit 2 regardless of which check actually failed. Reason names the FIRST +// check, in the order below, that failed; a later failure still adds its own +// Note. func proveCoverage(h Header, polls []Poll, sentinel *time.Time, t ruleTimings, def Definition, from, to time.Time, grace time.Duration) CoverageResult { @@ -100,9 +96,9 @@ func proveCoverage(h Header, polls []Poll, sentinel *time.Time, t ruleTimings, d res.Notes = append(res.Notes, fmt.Sprintf("rule %q: %s", def.Title, note)) } - // Check 1 — sentinel (§4.5). Present and At >= to+grace -> coverage - // provable; absent, or short of it, is never a pass. A recorder that died - // early must look exactly like a coverage gap, because it is one. + // Check 1 — sentinel. Present and At >= to+grace -> coverage provable; + // absent, or short of it, is never a pass. A recorder that died early must + // look exactly like a coverage gap, because it is one. switch { case sentinel == nil: fail(ReasonNoSentinel, "no stopped sentinel: the recorder never reported finishing") @@ -111,13 +107,12 @@ func proveCoverage(h Header, polls []Poll, sentinel *time.Time, t ruleTimings, d sentinel.Format(time.RFC3339), windowEnd.Format(time.RFC3339))) } - // Check 2 — from bounds (§7), first sentence only: from < StartedAt makes - // coverage unprovable, no matter how healthy the polls that DO exist look. - // Both are runner-domain clock reads (the recorder's own Clock.Now()), so - // no cross-domain translation applies here. The second sentence — from - // more than fromFutureTolerance ahead is a hard error — is Check's input - // validation, once per run rather than per rule, and belongs to a later - // phase: this function has no error return, only a per-rule verdict. + // Check 2 — from bounds: from < StartedAt makes coverage unprovable, no + // matter how healthy the polls that DO exist look. Both are runner-domain + // clock reads (the recorder's own Clock.Now()), so no cross-domain + // translation applies here. The other half of the bound — from too far + // ahead of the runner's clock — is Check's input validation, once per run + // rather than per rule. if from.Before(h.StartedAt) { fail(ReasonFromBeforeRecord, fmt.Sprintf( "requested from %s is before recording started at %s", from.Format(time.RFC3339), h.StartedAt.Format(time.RFC3339))) @@ -129,18 +124,18 @@ func proveCoverage(h Header, polls []Poll, sentinel *time.Time, t ruleTimings, d // drifting from the other's membership test. inWindow := inWindowPolls(rulePolls, from, windowEnd) - // Check 3 — heartbeat continuity (§6). Data at both ends with a hole in - // between is not enough (§22.4): this scans every gap inside the window, - // not just its edges. + // Check 3 — heartbeat continuity. Data at both ends with a hole in between + // is not enough: this scans every gap inside the window, not just its + // edges. res.LargestGap, res.LargestGapAt = ruleHeartbeatGap(inWindow, from, windowEnd) if res.LargestGap > t.maxGap { fail(ReasonHeartbeatGap, fmt.Sprintf( "gap of %s starting at %s exceeds maxGap %s", res.LargestGap, res.LargestGapAt.Format(time.RFC3339), t.maxGap)) } - // Check 4 — health=="error" (§10.1). A short blip is a note only (§22.1: - // one failed evaluation must not exit 2 over an otherwise clean window); - // only a run longer than healthGrace consumes coverage. + // Check 4 — health=="error". A short blip is a note only — one failed + // evaluation must not exit 2 over an otherwise clean window; only a run + // longer than healthGrace consumes coverage. if runLen, sawAny := longestHealthRun(inWindow, "error"); sawAny { res.Notes = append(res.Notes, fmt.Sprintf("rule %q: health=error observed (longest run %s)", def.Title, runLen)) if runLen > t.healthGrace { @@ -148,25 +143,25 @@ func proveCoverage(h Header, polls []Poll, sentinel *time.Time, t ruleTimings, d } } - // Check 5 — health=="nodata" (§10.1/§10.2). Never fatal here: 96% of the - // fleet runs no_data_state:OK, so treating this as fatal by default would - // block nearly every healthy deploy in an idle environment. Escalating it - // under Policy.NodataIsUnobservable is decide's job (a later phase), - // applied directly against the raw polls — this pure function has no - // Policy to consult and must not invent one. + // Check 5 — health=="nodata". Never fatal here: 96% of the fleet runs + // no_data_state:OK, so treating this as fatal by default would block + // nearly every healthy deploy in an idle environment. Escalating it under + // Policy.NodataIsUnobservable is decide's job, applied directly against + // the raw polls — this pure function has no Policy to consult and must not + // invent one. if _, sawAny := longestHealthRun(inWindow, "nodata"); sawAny { res.Notes = append(res.Notes, fmt.Sprintf("rule %q: health=nodata observed (not fatal; see --nodata-is-unobservable)", def.Title)) } - // Check 6 — liveness (H3). Absolute only, per poll: GrafanaNow and + // Check 6 — liveness. Absolute only, per poll: GrafanaNow and // LastEvaluation are both Grafana-domain reads off the SAME response, so // this is a same-domain comparison and uses raw values — never a delta // against a previous poll, which reports stale on ~half the polls of a // perfectly healthy rule (polling runs at intervalSeconds/2). // // Skipped only for a poll whose own flags SAY there is nothing to check: - // IsPaused (a zero LastEvaluation is legal only while paused, §2.3; check - // 7 is its detector) or !Found (no rule, no evaluation; check 8 is its + // IsPaused (a zero LastEvaluation is legal only while paused; check 7 is + // its detector) or !Found (no rule, no evaluation; check 8 is its // detector). Deliberately NOT skipped merely because LastEvaluation is // zero: ReadLog does no field validation, so a corrupted or hand-edited // log line can claim found:true, is_paused:false and still carry a zero @@ -193,11 +188,10 @@ func proveCoverage(h Header, polls []Poll, sentinel *time.Time, t ruleTimings, d staleCount, worstStale, t.evalStaleAfter, worstStaleAt.Format(time.RFC3339))) } - // Check 7 — isPaused in-window (§12.2, §14.8). The PRIMARY pause - // detector: liveness (check 6) is only the backup for what IsPaused - // cannot show (a deleted rule, a stopped scheduler, a blocked - // evaluation). This is what catches pause-then-unpause, which the drain - // wait alone passes (§14.7). + // Check 7 — isPaused in-window. The PRIMARY pause detector: liveness + // (check 6) is only the backup for what IsPaused cannot show (a deleted + // rule, a stopped scheduler, a blocked evaluation). This is what catches + // pause-then-unpause, which the drain wait alone passes. var pausedCount int var pausedAt time.Time for _, p := range inWindow { @@ -212,8 +206,8 @@ func proveCoverage(h Header, polls []Poll, sentinel *time.Time, t ruleTimings, d fail(ReasonPausedInWindow, fmt.Sprintf("observed paused on %d poll(s), first at %s", pausedCount, pausedAt.Format(time.RFC3339))) } - // Check 8 — rule absent (§14.5). Found==false is authoritative (P2 - // already retried every transport failure before a Poll record ever + // Check 8 — rule absent. Found==false is authoritative (the transport + // already retried every transient failure before a Poll record ever // exists): the rule resolved at resolve time but the state endpoint // stopped serving it. Never drop a watched rule from the verdict set // silently. @@ -231,21 +225,20 @@ func proveCoverage(h Header, polls []Poll, sentinel *time.Time, t ruleTimings, d fail(ReasonRuleAbsent, fmt.Sprintf("state endpoint returned no rule on %d poll(s), first at %s", absentCount, absentAt.Format(time.RFC3339))) } - // Check 9 — KeepLast (§10.2). Two distinct notes, both non-fatal: + // Check 9 — KeepLast. Two distinct notes, both non-fatal: // // DECLARED: the rule's own no_data_state/exec_err_state is configured as - // KeepLast — a standing blind spot (§10.2's "unclear condition") whether - // or not it is ever exercised during this particular window. This reads - // def, not polls, so it fires exactly once regardless of poll content. + // KeepLast — a standing blind spot whether or not it is ever exercised + // during this particular window. This reads def, not polls, so it fires + // exactly once regardless of poll content. if def.NoDataState == keepLastReason || def.ExecErrState == keepLastReason { res.Notes = append(res.Notes, fmt.Sprintf( - "rule %q: configured with no_data_state/exec_err_state=KeepLast — a stale state can continue past a real fault (§10.2)", def.Title)) + "rule %q: configured with no_data_state/exec_err_state=KeepLast — a stale state can continue past a real fault", def.Title)) } // OBSERVED: an instance actually reported the KeepLast reason during the - // window. It surfaces only as an instance Reason after P1.2a's parsing, - // and Reasons keys can be comma-joined composites, so membership - // (reasonsContain) is required — indexing "KeepLast" directly would miss - // "KeepLast, MissingSeries". + // window. It surfaces only as an instance Reason, and Reasons keys can be + // comma-joined composites, so membership (reasonsContain) is required — + // indexing "KeepLast" directly would miss "KeepLast, MissingSeries". for _, p := range inWindow { if reasonsContain(p.Reasons, keepLastReason) { res.Notes = append(res.Notes, fmt.Sprintf( @@ -259,16 +252,16 @@ func proveCoverage(h Header, polls []Poll, sentinel *time.Time, t ruleTimings, d } // inWindowPolls filters polls to those inside [from, windowEnd] using the -// CROSS-DOMAIN membership test (§16): each poll's Grafana-domain GrafanaNow -// is translated to the runner domain by its OWN skew, and its own skew bound -// is the membership tolerance, so a poll that is genuinely inside the window -// is never excluded by ordinary clock imprecision. +// CROSS-DOMAIN membership test: each poll's Grafana-domain GrafanaNow is +// translated to the runner domain by its OWN skew, and its own skew bound is +// the membership tolerance, so a poll that is genuinely inside the window is +// never excluded by ordinary clock imprecision. // -// Everything downstream of this filter (health runs, liveness, pause, -// absence) reads the poll's raw fields: GrafanaNow paired with -// LastEvaluation on the SAME response, or one poll's GrafanaNow against the -// next's, are same-domain comparisons and need no translation (§16, "Clock -// domains" — only window membership and check 3's two boundary segments do). +// Everything downstream of this filter (health runs, liveness, pause, absence) +// reads the poll's raw fields: GrafanaNow paired with LastEvaluation on the +// SAME response, or one poll's GrafanaNow against the next's, are same-domain +// comparisons and need no translation. Only window membership and check 3's +// two boundary segments cross domains. func inWindowPolls(polls []Poll, from, windowEnd time.Time) []Poll { var out []Poll for _, p := range polls { @@ -282,22 +275,22 @@ func inWindowPolls(polls []Poll, from, windowEnd time.Time) []Poll { return out } -// ruleHeartbeatGap finds the largest unobserved span inside [from, windowEnd] -// (§6), including the two boundary segments — which is why "data at both -// ends with a hole in the middle" still fails (§22.4): the segment between -// the polls just inside each edge is exactly what this measures. in must -// already be filtered to this window (inWindowPolls) and sorted by -// GrafanaNow — proveCoverage computes that filter once and threads it through -// every check, this one included, rather than each check re-filtering. +// ruleHeartbeatGap finds the largest unobserved span inside [from, windowEnd], +// including the two boundary segments — which is why "data at both ends with a +// hole in the middle" still fails: the segment between the polls just inside +// each edge is exactly what this measures. in must already be filtered to this +// window (inWindowPolls) and sorted by GrafanaNow — proveCoverage computes that +// filter once and threads it through every check, this one included, rather +// than each check re-filtering. // // The two boundary segments compare a Grafana-domain poll time against the // runner-domain from/windowEnd, so each is translated by its own poll's skew -// AND widened by that same poll's skew bound (§16: "with that poll's bound as -// the tolerance") — on the side that makes the segment larger, never smaller, -// so an uncertain boundary reads as at least as big a gap as it might really -// be. Understating it by up to the bound would be fail-open. The spacing -// BETWEEN consecutive polls compares two Grafana-domain reads to each other — -// same domain — and uses the raw GrafanaNow difference, no bound needed. +// AND widened by that same poll's skew bound — on the side that makes the +// segment larger, never smaller, so an uncertain boundary reads as at least as +// big a gap as it might really be. Understating it by up to the bound would be +// fail-open. The spacing BETWEEN consecutive polls compares two Grafana-domain +// reads to each other — same domain — and uses the raw GrafanaNow difference, +// no bound needed. func ruleHeartbeatGap(in []Poll, from, windowEnd time.Time) (largestGap time.Duration, largestGapAt time.Time) { if len(in) == 0 { return windowEnd.Sub(from), from @@ -321,16 +314,15 @@ func ruleHeartbeatGap(in []Poll, from, windowEnd time.Time) (largestGap time.Dur return largestGap, largestGapAt } -// longestHealthRun returns the longest contiguous wall-clock span (§10.1) -// during which polls — already sorted by GrafanaNow, same-domain spacing -// (§16) — read the given rule-level Health, and whether any poll matched it -// at all. +// longestHealthRun returns the longest contiguous wall-clock span during which +// polls — already sorted by GrafanaNow, same-domain spacing — read the given +// rule-level Health, and whether any poll matched it at all. // // It detects the span as it accumulates rather than waiting for the run to // end, so an open-ended run that is still failing at the last poll in the // window is measured correctly without needing data past the window: waiting // for the run to "end" would have to assume the best case about what happens -// next, which is exactly what this gate must not do (§1). +// next, which is exactly what this gate must not do. func longestHealthRun(polls []Poll, health string) (longest time.Duration, sawAny bool) { var runStart time.Time for _, p := range polls { diff --git a/grafana-alertcheck/internal/gate/coverage_test.go b/grafana-alertcheck/internal/gate/coverage_test.go index 9dbba6594..cef99782b 100644 --- a/grafana-alertcheck/internal/gate/coverage_test.go +++ b/grafana-alertcheck/internal/gate/coverage_test.go @@ -45,7 +45,7 @@ func TestProveCoverage_FiltersPollsByUID(t *testing.T) { } } -// --- Check 1: sentinel (§4.5) --- +// --- Check 1: sentinel --- func TestProveCoverage_NoSentinelIsUnobservable(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) @@ -109,7 +109,7 @@ func TestProveCoverage_SentinelExactlyAtGraceIsFine(t *testing.T) { } } -// --- Check 2: from bounds (§7) --- +// --- Check 2: from bounds --- func TestProveCoverage_FromBeforeRecordIsUnobservable(t *testing.T) { started := time.Date(2026, 1, 1, 1, 0, 0, 0, time.UTC) @@ -141,10 +141,10 @@ func TestProveCoverage_FromBeforeRecordIsUnobservable(t *testing.T) { } } -// --- Check 3: heartbeat continuity (§6) --- +// --- Check 3: heartbeat continuity --- -// TestProveCoverage_HeartbeatGapBetweenBoundariesIsUnobservable is §22.4's -// core regression: data at both ends with a hole between is not enough. +// The core heartbeat regression: data at both ends with a hole between is not +// enough. func TestProveCoverage_HeartbeatGapBetweenBoundariesIsUnobservable(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) to := from.Add(10 * time.Minute) @@ -159,7 +159,7 @@ func TestProveCoverage_HeartbeatGapBetweenBoundariesIsUnobservable(t *testing.T) res := proveCoverage(Header{StartedAt: from.Add(-time.Hour)}, polls, &sentinel, rt, def, from, to, 0) if res.Reason != ReasonHeartbeatGap { - t.Fatalf("Reason = %q, want heartbeat_gap: healthy edges with a hole in the middle must still fail (§22.4)", res.Reason) + t.Fatalf("Reason = %q, want heartbeat_gap: healthy edges with a hole in the middle must still fail", res.Reason) } // The gap is the SPACING between the two polls (598s), not either // boundary segment (1s each) — pin the actual values, not just the verdict. @@ -172,7 +172,7 @@ func TestProveCoverage_HeartbeatGapBetweenBoundariesIsUnobservable(t *testing.T) } } -// --- Check 4/5: health (§10.1/§10.2) --- +// --- Check 4/5: health --- func TestProveCoverage_HealthErrorShortBlipPassesWithNote(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) @@ -193,7 +193,7 @@ func TestProveCoverage_HealthErrorShortBlipPassesWithNote(t *testing.T) { res := proveCoverage(Header{StartedAt: from.Add(-time.Hour)}, polls, &sentinel, rt, def, from, to, 0) if !res.Proved { - t.Fatalf("Proved = false, want true: one failed evaluation must not fail an otherwise clean window (§22.1): %+v", res) + t.Fatalf("Proved = false, want true: one failed evaluation must not fail an otherwise clean window: %+v", res) } if !anyContains(res.Notes, "health=error") { t.Fatalf("Notes = %v, want a health=error note even though it did not fail the window", res.Notes) @@ -238,20 +238,19 @@ func TestProveCoverage_HealthNodataNeverFatalHere(t *testing.T) { res := proveCoverage(Header{StartedAt: from.Add(-time.Hour)}, polls, &sentinel, rt, def, from, to, 0) if !res.Proved { t.Fatalf("Proved = false, want true: health=nodata for the WHOLE window must still not be fatal by itself "+ - "(escalating it is Policy.NodataIsUnobservable's job, applied by decide in a later phase): %+v", res) + "(escalating it is Policy.NodataIsUnobservable's job, applied by decide): %+v", res) } if !anyContains(res.Notes, "health=nodata") { t.Fatalf("Notes = %v, want a health=nodata note", res.Notes) } } -// --- Check 6: liveness / H3 --- +// --- Check 6: liveness --- -// TestProveCoverage_LivenessAbsoluteNeverFalseStale is §22.7's disproportionate -// test: a healthy rule polled at intervalSeconds/2, across the full window, -// must show zero staleness violations. lastEvaluation only advances once per -// full evaluation interval here — the realistic shape a delta check -// misreads as stale on roughly half of all polls (H3). +// A healthy rule polled at intervalSeconds/2, across the full window, must +// show zero staleness violations. lastEvaluation only advances once per full +// evaluation interval here — the realistic shape a delta check misreads as +// stale on roughly half of all polls. func TestProveCoverage_LivenessAbsoluteNeverFalseStale(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) pollEvery := 30 * time.Second @@ -272,7 +271,7 @@ func TestProveCoverage_LivenessAbsoluteNeverFalseStale(t *testing.T) { res := proveCoverage(Header{StartedAt: from.Add(-time.Hour)}, polls, &sentinel, rt, def, from, windowEnd, 0) if res.Reason == ReasonStaleEvaluation || res.BlindFor != 0 { - t.Fatalf("proveCoverage flagged staleness on a healthy rule polled at intervalSeconds/2 — H3 must be absolute, "+ + t.Fatalf("proveCoverage flagged staleness on a healthy rule polled at intervalSeconds/2 — liveness must be absolute, "+ "never a delta against a previous poll: %+v", res) } if !res.Proved { @@ -312,9 +311,8 @@ func TestProveCoverage_ZeroLastEvaluationNeverFalseStale(t *testing.T) { rt := newRuleTimings(30*time.Second, 60) def := Definition{UID: "r1", Title: "R1"} - // A paused rule legitimately reports the zero time (§2.3); check 6 must - // not read that as an enormous staleness violation. Check 7 is its - // detector. + // A paused rule legitimately reports the zero time; check 6 must not read + // that as an enormous staleness violation. Check 7 is its detector. polls := []Poll{ {RuleUID: "r1", GrafanaNow: from.Add(time.Minute), Found: true, IsPaused: true}, } @@ -326,7 +324,7 @@ func TestProveCoverage_ZeroLastEvaluationNeverFalseStale(t *testing.T) { } } -// --- Check 7: isPaused in-window (§12.2, §14.8) --- +// --- Check 7: isPaused in-window --- func TestProveCoverage_PausedInWindowIsUnobservable(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) @@ -341,7 +339,7 @@ func TestProveCoverage_PausedInWindowIsUnobservable(t *testing.T) { for i := range polls { if polls[i].GrafanaNow.Equal(pausedAt) { polls[i].IsPaused = true - polls[i].LastEvaluation = time.Time{} // legal only while paused, §2.3 + polls[i].LastEvaluation = time.Time{} // legal only while paused } } sentinel := to @@ -379,7 +377,7 @@ func TestProveCoverage_PausedAfterWindowIsFine(t *testing.T) { } } -// --- Check 8: rule absent (§14.5) --- +// --- Check 8: rule absent --- func TestProveCoverage_RuleAbsentIsUnobservable(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) @@ -418,7 +416,7 @@ func denseHealthyPolls(uid string, from, to time.Time, every time.Duration) []Po return out } -// --- Check 9: KeepLast (§10.2) --- +// --- Check 9: KeepLast --- func TestProveCoverage_KeepLastObservedIsNoteOnly(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) @@ -431,7 +429,7 @@ func TestProveCoverage_KeepLastObservedIsNoteOnly(t *testing.T) { polls = append(polls, Poll{ RuleUID: "r1", GrafanaNow: ts, Found: true, Health: "ok", LastEvaluation: ts, // A comma-joined composite — reasonsContain must match by - // membership, never by an exact key, per P5's markers. + // membership, never by an exact key. Reasons: map[string]int{"KeepLast, MissingSeries": 1}, }) } @@ -446,8 +444,8 @@ func TestProveCoverage_KeepLastObservedIsNoteOnly(t *testing.T) { } } -// §22.2/§10.2: "KeepLast in the configuration gives a note" — a DIFFERENT -// claim from the observed-reason test above. A rule DECLARED with +// KeepLast in the CONFIGURATION gives a note — a different claim from the +// observed-reason test above. A rule DECLARED with // no_data_state or exec_err_state = KeepLast is a standing blind spot // whether or not any poll ever actually reports the reason, so the note // must fire off the definition alone, over an otherwise perfectly healthy @@ -480,12 +478,11 @@ func TestProveCoverage_KeepLastConfiguredIsNoteOnly(t *testing.T) { } } -// --- Clock domains (§16) --- +// --- Clock domains --- -// TestProveCoverage_SkewTranslationAtWindowBoundary pins §16's "Clock -// domains" rule: a constant clock skew on every poll must not itself read as -// a coverage gap or a from-before-record violation, because every -// cross-domain comparison translates by that poll's own skew first. +// A constant clock skew on every poll must not itself read as a coverage gap +// or a from-before-record violation, because every cross-domain comparison +// translates by that poll's own skew first. func TestProveCoverage_SkewTranslationAtWindowBoundary(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) to := from.Add(10 * time.Minute) @@ -507,15 +504,14 @@ func TestProveCoverage_SkewTranslationAtWindowBoundary(t *testing.T) { res := proveCoverage(Header{StartedAt: from.Add(-time.Hour)}, polls, &sentinel, rt, def, from, to, 0) if !res.Proved { - t.Fatalf("res = %+v, want proved: a constant clock skew must not itself read as a coverage gap (§16)", res) + t.Fatalf("res = %+v, want proved: a constant clock skew must not itself read as a coverage gap", res) } } -// --- Override round-trip (P5's "two authorities") --- +// --- Override round-trip: one authority for the cadence --- -// TestProveCoverage_OverrideRoundTrip is P7's other disproportionate done-gate -// test: it exercises DeriveTimingsFromLog and proveCoverage together, exactly -// as check will, to prove maxGap tracks the RECORDED cadence, never a +// This exercises DeriveTimingsFromLog and proveCoverage together, exactly as +// check does, to prove maxGap tracks the RECORDED cadence and never a // re-derivation from the rule's own evaluation interval. func TestProveCoverage_OverrideRoundTrip(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) @@ -577,7 +573,7 @@ func TestProveCoverage_OverrideRoundTrip(t *testing.T) { res := proveCoverage(h, polls, &sentinel, rt["r1"], defs[0], from, windowEnd, 0) if res.Reason != ReasonHeartbeatGap { t.Fatalf("Reason = %q, want heartbeat_gap: if maxGap had been re-derived from the 300s definition instead of "+ - "the recorded 5s cadence, this 250s gap would pass silently — the fail-open direction P5 warns about", res.Reason) + "the recorded 5s cadence, this 250s gap would pass silently — the fail-open direction", res.Reason) } }) } @@ -598,7 +594,7 @@ func anyContains(notes []string, substr string) bool { // found:true, is_paused:false and still carry a zero LastEvaluation (a // corrupted write, a hand-edited fixture, a future log format bug). That // combination must read as maximally stale, not be waved through the way a -// legitimately paused poll's zero time is (§2.3) — the skip must key off +// legitimately paused poll's zero time is — the skip must key off // IsPaused/Found, never off LastEvaluation being zero. func TestProveCoverage_ZeroLastEvaluationWithoutPauseIsStale(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) @@ -624,10 +620,9 @@ func TestProveCoverage_ZeroLastEvaluationWithoutPauseIsStale(t *testing.T) { // --- Check 3, tightened: the boundary segments must widen by the skew bound --- -// TestProveCoverage_BoundaryGapWidensBySkewBound pins §16's "with that -// poll's bound as the tolerance" for the two boundary segments specifically: -// a boundary gap that lands EXACTLY at maxGap must still fail once the -// poll's own skew bound is added, because the translation is only a best +// The two boundary segments take their own poll's bound as the tolerance: a +// boundary gap that lands EXACTLY at maxGap must still fail once the poll's +// own skew bound is added, because the translation is only a best // estimate and understating the gap by up to the bound would be fail-open. func TestProveCoverage_BoundaryGapWidensBySkewBound(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) @@ -647,17 +642,16 @@ func TestProveCoverage_BoundaryGapWidensBySkewBound(t *testing.T) { res := proveCoverage(Header{StartedAt: from.Add(-time.Hour)}, polls, &sentinel, rt, def, from, to, 0) if res.Reason != ReasonHeartbeatGap { t.Fatalf("Reason = %q, want heartbeat_gap: the leading boundary segment sits at EXACTLY maxGap (60s) before "+ - "widening; the poll's own %s skew bound must push it past the threshold (§16), not just the skew translation", res.Reason, bound) + "widening; the poll's own %s skew bound must push it past the threshold, not just the skew translation", res.Reason, bound) } } // --- Multi-failure contract --- -// TestProveCoverage_MultipleFailuresReasonIsFirstButAllNoted exercises two -// checks failing in the same rule: check 7 (paused in-window) precedes check -// 8 (rule absent) in the §5 order, so Reason must name the pause even though -// the rule also goes absent later — and the later failure must still add its -// own Note rather than being swallowed once Reason is set. +// Two checks failing in the same rule: check 7 (paused in-window) runs before +// check 8 (rule absent), so Reason must name the pause even though the rule +// also goes absent later — and the later failure must still add its own Note +// rather than being swallowed once Reason is set. func TestProveCoverage_MultipleFailuresReasonIsFirstButAllNoted(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) to := from.Add(10 * time.Minute) @@ -682,7 +676,7 @@ func TestProveCoverage_MultipleFailuresReasonIsFirstButAllNoted(t *testing.T) { res := proveCoverage(Header{StartedAt: from.Add(-time.Hour)}, polls, &sentinel, rt, def, from, to, 0) if res.Reason != ReasonPausedInWindow { - t.Fatalf("Reason = %q, want paused_in_window (the FIRST check to fail, in §5's order)", res.Reason) + t.Fatalf("Reason = %q, want paused_in_window: the FIRST check to fail names the reason", res.Reason) } if !anyContains(res.Notes, "paused") { t.Fatalf("Notes = %v, want a note about the pause", res.Notes) @@ -693,18 +687,16 @@ func TestProveCoverage_MultipleFailuresReasonIsFirstButAllNoted(t *testing.T) { } } -// --- Skipped rules (P6/P8 obligation) --- +// --- Skipped rules --- -// TestProveCoverage_SkippedRuleWithZeroPollsPinnedAsHeartbeatGap pins a known -// gap in this function's contract, not a bug in it: a rule paused BEFORE the -// window opened is never scheduled or polled (watch.go, §4.3), so it reaches -// proveCoverage with zero polls at all. proveCoverage has no notion of +// A known limit of this function's contract, not a bug in it: a rule paused +// BEFORE the window opened is never scheduled or polled (watch.go), so it +// reaches proveCoverage with zero polls at all. proveCoverage has no notion of // "skipped" — that classification belongs to the definitions -// (LoggedRule.IsPaused / Definition.IsPaused), never to the polls — so today -// it reports the whole window as one big heartbeat_gap instead. decide (P8) -// MUST read skipped status from the definitions and either skip calling this -// function for that rule entirely, or override this result — this test pins -// today's behavior so that review has something concrete to check against. +// (LoggedRule.IsPaused / Definition.IsPaused), never to the polls — so it +// reports the whole window as one big heartbeat_gap instead. decide is what +// reads skipped status from the header and never calls this function for such +// a rule; this pins the behavior it relies on not reaching. func TestProveCoverage_SkippedRuleWithZeroPollsPinnedAsHeartbeatGap(t *testing.T) { from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) to := from.Add(10 * time.Minute) @@ -715,7 +707,7 @@ func TestProveCoverage_SkippedRuleWithZeroPollsPinnedAsHeartbeatGap(t *testing.T res := proveCoverage(Header{StartedAt: from.Add(-time.Hour)}, nil, &sentinel, rt, def, from, to, 0) if res.Reason != ReasonHeartbeatGap { t.Fatalf("Reason = %q, want heartbeat_gap (pinned, not the desired end state): proveCoverage has no "+ - "'skipped' concept, so decide (P8) must handle a skipped rule's classification itself, before or "+ + "'skipped' concept, so decide must handle a skipped rule's classification itself, before or "+ "instead of calling this function", res.Reason) } } diff --git a/grafana-alertcheck/internal/gate/duration.go b/grafana-alertcheck/internal/gate/duration.go index 011c27f14..3690a2549 100644 --- a/grafana-alertcheck/internal/gate/duration.go +++ b/grafana-alertcheck/internal/gate/duration.go @@ -29,7 +29,7 @@ var promDurationUnits = []promDurationUnit{ } // ParsePromDuration parses a Grafana/Prometheus-style duration ("1h30m", "1d", "1w"). -// Unlike time.ParseDuration, it accepts "d" and "w" (§11.8). "" and "0" are 0. +// Unlike time.ParseDuration, it accepts "d" and "w". "" and "0" are 0. func ParsePromDuration(s string) (time.Duration, error) { if s == "" || s == "0" { return 0, nil diff --git a/grafana-alertcheck/internal/gate/flock.go b/grafana-alertcheck/internal/gate/flock.go index 865c6019a..3d006a85b 100644 --- a/grafana-alertcheck/internal/gate/flock.go +++ b/grafana-alertcheck/internal/gate/flock.go @@ -8,7 +8,7 @@ import ( ) // lockExclusive takes a non-blocking exclusive lock on f. Non-blocking is the -// point (§8): a second writer must fail immediately with an error the operator +// point: a second writer must fail immediately with an error the operator // sees, not queue behind the first and start appending to a log somebody else // already finished. func lockExclusive(f *os.File) error { @@ -24,7 +24,7 @@ func lockExclusive(f *os.File) error { // // check needs that distinction where NewWriter does not. NewWriter is entitled // to treat any refusal as "another writer has it", because it wants the lock; -// check only wants to know whether a writer EXISTS (§4.4). The lock answers +// check only wants to know whether a writer EXISTS. The lock answers // that directly, where a pid can only infer it — the kernel releases a flock // when the holder exits, crash included, and pids get reused. func tryLockExclusive(f *os.File) (held bool, err error) { diff --git a/grafana-alertcheck/internal/gate/jsonreq.go b/grafana-alertcheck/internal/gate/jsonreq.go index cea19b6fe..24ea183ba 100644 --- a/grafana-alertcheck/internal/gate/jsonreq.go +++ b/grafana-alertcheck/internal/gate/jsonreq.go @@ -8,7 +8,7 @@ import ( // req decodes m[key] into *dst. It returns an error when key is absent from m // or explicitly JSON null, so a caller can never mistake absence for a zero -// value (H1) — json.Unmarshal treats "null" as a documented no-op for +// value — json.Unmarshal treats "null" as a documented no-op for // non-pointer targets (string, bool, int, ...), so without this check a // required field sent as null would silently pass through as its zero value. func req[T any](m map[string]json.RawMessage, key string, dst *T) error { diff --git a/grafana-alertcheck/internal/gate/log.go b/grafana-alertcheck/internal/gate/log.go index fa9838aed..eb240aafa 100644 --- a/grafana-alertcheck/internal/gate/log.go +++ b/grafana-alertcheck/internal/gate/log.go @@ -13,11 +13,11 @@ import ( // LogSchemaVersion is the version stamped into every log header. A log with // any other value is a read error, never a best-effort read: the log is the -// gate's only evidence, and misreading a stale shape is a fail-open (§5). +// gate's only evidence, and misreading a stale shape is a fail-open. const LogSchemaVersion = 1 // RecordType tags each JSONL line. There are exactly three, and a poll record -// IS the heartbeat — there is deliberately no separate heartbeat type (§4.6). +// IS the heartbeat — there is deliberately no separate heartbeat type. type RecordType string const ( @@ -28,13 +28,13 @@ const ( // missingSeriesReason is the reason Grafana parks a disappearing series at // ("Normal (MissingSeries)") for a couple of evaluations before deleting the -// instance. Reading that as a recovery is H2's named bug, so the markers below -// route it to Vanished (P1.2a). +// instance. Reading that as a recovery would turn a disappearing series into a +// fake recovery, so the markers below route it to Vanished. const missingSeriesReason = "MissingSeries" // LoggedRule is the per-rule identity written into the header. Together with -// the header URL it IS the log's identity, which check validates (§19.1 step -// 3), and it supplies the alert set in check mode. +// the header URL it IS the log's identity, which check validates, and it +// supplies the alert set in check mode. type LoggedRule struct { UID string `json:"uid"` Title string `json:"title"` @@ -42,17 +42,17 @@ type LoggedRule struct { Group string `json:"group"` // ForSeconds, IntervalSeconds, NoDataState and ExecErrState are purely // forensic: a resolve-time snapshot that makes the uploaded artifact - // self-describing to a human reading it after the runner is gone (§21.3). - // check never converts them back into a Definition — it always re-resolves - // definitions from the ruler API (§19.1 step 2). + // self-describing to a human reading it after the runner is gone. check + // never converts them back into a Definition — it always re-resolves + // definitions from the ruler API. ForSeconds float64 `json:"for_seconds"` IntervalSeconds int `json:"interval_seconds"` // IsPaused is NOT forensic, and is the second load-bearing field here // beside PollEverySeconds. It is the pause state at record start, which is - // the only moment `skipped` can honestly mean (§12), and decide reads it - // through Header.pausedAtStart rather than reading Definition.IsPaused off - // a ruler read taken after the window had already closed. See that method - // for what goes wrong the other way. + // the only moment `skipped` can honestly mean, and decide reads it through + // Header.pausedAtStart rather than reading Definition.IsPaused off a ruler + // read taken after the window had already closed. See that method for what + // goes wrong the other way. IsPaused bool `json:"is_paused"` NoDataState string `json:"no_data_state"` ExecErrState string `json:"exec_err_state"` @@ -60,34 +60,34 @@ type LoggedRule struct { // --poll-interval override. Load-bearing, not forensic: check derives // maxGap from it and never re-derives it from the definitions. Getting // that wrong is fail-open in the faster-override direction — a real - // recorder gap would pass silently (see "Two authorities", P5). + // recorder gap would pass silently. PollEverySeconds float64 `json:"poll_every_seconds"` } // Header is the log's first line: what was recorded, from where, and when the // recording started. It carries no States field — recording is deliberately // unfiltered, so the same log can be re-classified under different --states -// without re-recording (P6). +// without re-recording. type Header struct { SchemaVersion int `json:"schema_version"` - URL string `json:"url"` // the log's identity (§19.1 step 3) + URL string `json:"url"` // the log's identity GrafanaVersion string `json:"grafana_version"` - StartedAt time.Time `json:"started_at"` // the record start (§7 validation) - Rules []LoggedRule `json:"rules"` // THE alert set (§19.1 step 3) + StartedAt time.Time `json:"started_at"` // the record start + Rules []LoggedRule `json:"rules"` // THE alert set } // pausedAtStart reports, per rule UID, whether the rule was paused when the -// recording opened. That instant — and no other — is what `skipped` means -// (§12): a rule nobody was watching on purpose. +// recording opened. That instant — and no other — is what `skipped` means: a +// rule nobody was watching on purpose. // // It is the authority for `skipped` in BOTH modes, and the reason is that no // other source knows the right moment. `check` re-resolves the definitions -// AFTER the window closed (§19.1 step 2), so Definition.IsPaused there -// describes the present, not the window: a rule that fired and was then -// paused would read as skipped, its firing would never be classified, and -// under --allow-paused the run would pass. The header cannot drift that way, -// because watch stamps it before the deploy step runs and single-step check -// stamps it from definitions resolved at the start of its own step. +// AFTER the window closed, so Definition.IsPaused there describes the present, +// not the window: a rule that fired and was then paused would read as skipped, +// its firing would never be classified, and under --allow-paused the run would +// pass. The header cannot drift that way, because watch stamps it before the +// deploy step runs and single-step check stamps it from definitions resolved at +// the start of its own step. // // A UID the header does not name is reported NOT paused, which is the safe // direction: it then reaches proveCoverage with no polls and fails closed as @@ -104,7 +104,7 @@ func (h Header) pausedAtStart() map[string]bool { // only input the pure coverage and classification layers ever see. type Poll struct { RuleUID string `json:"rule_uid"` - GrafanaNow time.Time `json:"grafana_now"` // the Date header — H4 + GrafanaNow time.Time `json:"grafana_now"` // the response's Date header // SkewMS, SkewBoundMS and LatencyMS are milliseconds for JSONL // compactness ONLY. The pure layer never touches raw ms: it reads // Skew(), SkewBound() and Latency() below, which convert at the @@ -112,21 +112,21 @@ type Poll struct { SkewMS int64 `json:"skew_ms"` SkewBoundMS int64 `json:"skew_bound_ms"` LatencyMS int64 `json:"latency_ms"` - // Found false means an authoritative 2xx in which this rule was absent - // (§14.5) — never a transport failure, which P2 retried and never turns - // into a Poll. P7 check 8 turns it into unobservable. + // Found false means an authoritative 2xx in which this rule was absent — + // never a transport failure, which the transport retries and never turns + // into a Poll. The coverage proof turns it into unobservable. Found bool `json:"found"` // State, Health and LastError are the raw rule-level strings, reporting - // only and never classified (P1.2a). + // only and never classified. State string `json:"state,omitempty"` Health string `json:"health,omitempty"` LastError string `json:"last_error,omitempty"` - // omitzero, not omitempty: a not-found poll (and a paused rule, §2.3) has - // no evaluation time, and writing "0001-01-01T00:00:00Z" into an artifact - // humans and jq read (§21.3) invites reading it as a real timestamp. + // omitzero, not omitempty: a not-found poll (and a paused rule) has no + // evaluation time, and writing "0001-01-01T00:00:00Z" into an artifact + // humans and jq read invites reading it as a real timestamp. LastEvaluation time.Time `json:"last_evaluation,omitzero"` IsPaused bool `json:"is_paused"` - Histogram map[string]int `json:"histogram,omitempty"` // §4.9 — written, never analysed + Histogram map[string]int `json:"histogram,omitempty"` // written, never analysed // Reasons counts this poll's non-empty instance reasons, e.g. // {"NoData":1091,"Error":14}; nil when none. Reporting-only, and the ONLY // place composite states stay visible: they are canonical normal (so they @@ -134,37 +134,37 @@ type Poll struct { // // The KEYS are raw reason strings and can be comma-joined composites // ("KeepLast, MissingSeries") — newer Grafana versions join several - // reasons into one. So any consumer, P7 check 9's KeepLast note included, - // must test membership across the keys with reasonNames and must NEVER - // index a literal key: reasons["KeepLast"] misses every composite. + // reasons into one. So any consumer, the coverage proof's KeepLast note + // included, must test membership across the keys with reasonNames and must + // NEVER index a literal key: reasons["KeepLast"] misses every composite. Reasons map[string]int `json:"reasons,omitempty"` - // Abnormal holds the instances whose CANONICAL state is not normal - // (§4.6). "Normal (NoData)" and "Normal (Error)" are canonical normal and - // are deliberately not retained here (P1.2a). + // Abnormal holds the instances whose CANONICAL state is not normal. + // "Normal (NoData)" and "Normal (Error)" are canonical normal and are + // deliberately not retained here. Abnormal []Instance `json:"abnormal,omitempty"` - // Cleared and Vanished are instance keys (§4.7): keys that left the - // abnormal set, resolved against the SAME response — a clear and a - // discontinuity are not the same fact (H2). + // Cleared and Vanished are instance keys that left the abnormal set, + // resolved against the SAME response — a clear and a discontinuity are not + // the same fact. Cleared []string `json:"cleared,omitempty"` Vanished []string `json:"vanished,omitempty"` } -// Skew is the signed clock skew of this poll (§16). +// Skew is the signed clock skew of this poll. func (p Poll) Skew() time.Duration { return time.Duration(p.SkewMS) * time.Millisecond } // SkewBound is the uncertainty on Skew — the tolerance every cross-domain -// comparison in P7 applies alongside it. +// comparison applies alongside it. func (p Poll) SkewBound() time.Duration { return time.Duration(p.SkewBoundMS) * time.Millisecond } -// Latency is the wall time this poll's request took, feeding §5.2's budget check. +// Latency is the wall time this poll's request took, feeding the budget check. func (p Poll) Latency() time.Duration { return time.Duration(p.LatencyMS) * time.Millisecond } // Reducer turns each Observation into the single Poll record that goes into // the log. It holds the previous poll's abnormal instance keys per rule, which -// is all the state the transition markers need (§4.7). +// is all the state the transition markers need. // // A Reducer is safe for concurrent use: watch polls a fleet of rules -// concurrently (P6) and every one of those goroutines reduces through the same +// concurrently and every one of those goroutines reduces through the same // instance, because the per-rule marker state has to live in one place. The // lock is per-Reducer rather than per-rule — Reduce only touches maps and // slices, so it never blocks on I/O while holding it. @@ -179,12 +179,12 @@ func NewReducer() *Reducer { // Reduce selects the rule identified by uid out of obs and reduces it to a // Poll. Selection is BY UID, never by title: a filtered response can carry -// several rules sharing one title (the known 2-way collision, §14.5), and -// picking the first would silently watch the wrong rule. +// several rules sharing one title, and picking the first would silently watch +// the wrong rule. // -// The reduction (§4.6) keeps the rule-level fields, the raw totals histogram, -// the reason counts, and only the instances whose canonical state is not -// normal. That makes per-poll size independent of NORMAL cardinality — not of +// The reduction keeps the rule-level fields, the raw totals histogram, the +// reason counts, and only the instances whose canonical state is not normal. +// That makes per-poll size independent of NORMAL cardinality — not of // cardinality outright: a rule with 449 firing instances still stores all 449. func (r *Reducer) Reduce(uid string, obs Observation) Poll { r.mu.Lock() @@ -216,8 +216,8 @@ func (r *Reducer) Reduce(uid string, obs Observation) Poll { p.Histogram = rule.Totals // present indexes every instance in THIS response, normal ones included — - // the markers below must resolve a departed key against the same response - // (H2), which is impossible from the abnormal subset alone. + // the markers below must resolve a departed key against the same response, + // which is impossible from the abnormal subset alone. present := make(map[string]Instance, len(rule.Instances)) curAbnormal := make(map[string]struct{}) for _, inst := range rule.Instances { @@ -246,7 +246,7 @@ func (r *Reducer) Reduce(uid string, obs Observation) Poll { p.Vanished = append(p.Vanished, key) case reasonNames(inst.Reason, missingSeriesReason): // The vanish in disguise, caught one poll earlier than the fully - // absent case — H2's named bug. + // absent case. p.Vanished = append(p.Vanished, key) default: // Present as canonical normal without a MissingSeries reason. @@ -268,10 +268,10 @@ func (r *Reducer) Reduce(uid string, obs Observation) Poll { // // It exists for the one place a recording changes hands: watch's parent takes // the first observation of every rule and its detached child continues from -// there (P6). Without the seed, an instance that is abnormal in the parent's +// there. Without the seed, an instance that is abnormal in the parent's // observation and gone by the child's first poll produces no marker at all — -// it leaves the record as though it had never been bad, which is H2's -// fail-open reached through the handoff rather than through a reason string. +// it leaves the record as though it had never been bad, the same fail-open a +// misread MissingSeries causes, reached through the handoff instead. // // Not-found polls are skipped, mirroring Reduce: an absent rule leaves the // previous abnormal set untouched rather than emptying it. @@ -291,7 +291,7 @@ func (r *Reducer) seedFrom(polls []Poll) { } // stateRuleByUID picks one rule out of a state-endpoint response BY UID, and -// nil means the response is an authoritative "the rule is absent" (§14.5). +// nil means the response is an authoritative "the rule is absent". // // Never by title: the ?rule_name= filter is a title filter, and a filtered // response can carry several rules sharing one title (the known 2-way @@ -311,7 +311,7 @@ func stateRuleByUID(rules []StateRule, uid string) *StateRule { // reasonNames reports whether reason names want. Newer Grafana versions // comma-join several reasons into one string, so this tests membership rather -// than equality (P7 check 9 needs the same test for KeepLast). +// than equality. func reasonNames(reason, want string) bool { for part := range strings.SplitSeq(reason, ",") { if strings.TrimSpace(part) == want { @@ -321,12 +321,12 @@ func reasonNames(reason, want string) bool { return false } -// VerifyNormalInstancesVisible checks §3.2's assumption on a first -// observation: that the state endpoint really does return normal instances, -// not only the abnormal ones. If it ever stops doing so, the reduction's -// "keep the non-normal instances" becomes "keep everything the API happened to -// send" and the transition markers lose their ground truth — a silent -// fail-open. So this is verified at start, never assumed. +// VerifyNormalInstancesVisible checks, on a first observation, that the state +// endpoint really does return normal instances and not only the abnormal ones. +// If it ever stops doing so, the reduction's "keep the non-normal instances" +// becomes "keep everything the API happened to send" and the transition markers +// lose their ground truth — a silent fail-open. So this is verified at start, +// never assumed. // // The counts are summed over every totals key whose LOWERCASED name is // "normal" or "inactive". Never index one literal key: the captured @@ -351,7 +351,7 @@ func VerifyNormalInstancesVisible(rules []StateRule) error { } return fmt.Errorf( "rule %q (%s): totals claim %d normal instances but the response returned none — "+ - "the state endpoint no longer returns normal instances, which the §3.2 reduction depends on", + "the state endpoint no longer returns normal instances, which the reduction depends on", r.Title, r.UID, claimed) } return nil @@ -384,10 +384,10 @@ type stoppedRecord struct { At time.Time `json:"at"` } -// Writer appends records to the JSONL log. It is append-only by construction -// (§8): O_APPEND|O_CREATE|O_WRONLY, never O_TRUNC, so no writer can ever -// destroy evidence a previous one recorded. An exclusive non-blocking flock -// makes a second writer fail immediately rather than interleave. +// Writer appends records to the JSONL log. It is append-only by construction — +// O_APPEND|O_CREATE|O_WRONLY, never O_TRUNC — so no writer can ever destroy +// evidence a previous one recorded. An exclusive non-blocking flock makes a +// second writer fail immediately rather than interleave. type Writer struct { mu sync.Mutex f *os.File @@ -415,8 +415,8 @@ func NewWriter(path string, clock Clock) (*Writer, error) { // WriteHeader writes line 1 and stamps the current schema version, so no // caller can leave it at zero. It refuses a non-empty file: the log already // has a header, and a second one would make ReadLog's "header is line 1" -// contract a lie. In the P6 handoff the parent writes the header and the child -// only appends polls. +// contract a lie. In watch's handoff the parent writes the header and the +// detached child only appends polls. func (w *Writer) WriteHeader(h Header) error { w.mu.Lock() defer w.mu.Unlock() @@ -450,15 +450,14 @@ func (w *Writer) WritePoll(p Poll) error { return nil } -// Stop finishes recording in the fixed §4.4 order, which must not be -// reordered: let the in-flight write finish (the mutex), append the stopped -// sentinel, fsync, then release. Any other order can leave a log whose last -// durable byte is a sentinel that was never actually preceded by the polls it -// vouches for. +// Stop finishes recording in a fixed order that must not be rearranged: let +// the in-flight write finish (the mutex), append the stopped sentinel, fsync, +// then release. Any other order can leave a log whose last durable byte is a +// sentinel that was never actually preceded by the polls it vouches for. // // Stop writes the sentinel with the recorder's OWN stop time and makes no // comparison against `to` — watch never knows `to` or the transition grace. -// check does that comparison, after this writer has exited (§4.5). +// check does that comparison, after this writer has exited. // // Calling Stop twice is a no-op: watch reaches it from both a signal handler // and a defer, and a second sentinel would be indistinguishable from a second @@ -488,9 +487,8 @@ func (w *Writer) Stop() error { // Close releases the file and the lock WITHOUT writing a sentinel. It exists // for exactly one caller: watch's parent, which writes the header and then -// hands the log to the detached child that will finish it (P6). A sentinel -// here would tell check the recording ended before the child had even -// started. +// hands the log to the detached child that will finish it. A sentinel here +// would tell check the recording ended before the child had even started. func (w *Writer) Close() error { w.mu.Lock() defer w.mu.Unlock() @@ -507,15 +505,15 @@ func (w *Writer) Close() error { // ReadLogHeader reads ONLY line 1 and is the one read of a log that a writer // may still hold. That is safe for exactly one line and for no other: the // header is written once, by watch's parent, before any child appends a byte, -// the file is opened O_APPEND and never O_TRUNC (§8), so line 1 is complete -// and immutable for the whole life of the recording. +// and the file is opened O_APPEND and never O_TRUNC, so line 1 is complete and +// immutable for the whole life of the recording. // -// It exists so check can fail closed EARLY (§19.1 steps 3-4): the log's -// identity, the rule set and the cadences are all knowable at the start, and -// discovering a wrong URL or an unresolvable rule after a ten-minute wait -// helps nobody. It is advisory only — the authoritative read is still ReadLog, -// once, after the writer has exited (§4.4 step 4), and check re-validates the -// identity against that header rather than trusting this one. +// It exists so check can fail closed EARLY: the log's identity, the rule set +// and the cadences are all knowable at the start, and discovering a wrong URL +// or an unresolvable rule after a ten-minute wait helps nobody. It is advisory +// only — the authoritative read is still ReadLog, once, after the writer has +// exited, and check re-validates the identity against that header rather than +// trusting this one. func ReadLogHeader(path string) (Header, error) { f, err := os.Open(path) if err != nil { @@ -550,12 +548,11 @@ func ReadLogHeader(path string) (Header, error) { // recording never finished — check turns that into unobservable, never a // pass). // -// Call this only after the writer has exited (§4.4 step 4). Reading a log a -// writer can still append to can only produce a shorter window than the one -// that was recorded. +// Call this only after the writer has exited. Reading a log a writer can still +// append to can only produce a shorter window than the one that was recorded. // -// The parse rules are deliberately the crudest possible (§24.2): the header -// must be line 1 with a matching schema version, and ANY unparseable line — +// The parse rules are deliberately the crudest possible: the header must be +// line 1 with a matching schema version, and ANY unparseable line — // including the last one, and including a last line that follows a sentinel — // is an error, full stop. No heuristics, no discarding an untidy tail: a // truncated log is evidence that something killed the recorder, which is diff --git a/grafana-alertcheck/internal/gate/log_test.go b/grafana-alertcheck/internal/gate/log_test.go index 494c9535c..179adb5fe 100644 --- a/grafana-alertcheck/internal/gate/log_test.go +++ b/grafana-alertcheck/internal/gate/log_test.go @@ -48,7 +48,7 @@ func TestLogReduceKeepsOnlyAbnormalInstances(t *testing.T) { Instances: []Instance{ testInstance(StateNormal, "", "a"), testInstance(StateFiring, "", "b"), - // Both composites are canonical normal (P1.2a): they must NOT be + // Both composites are canonical normal: they must NOT be // retained as abnormal, and their reasons must still be counted. testInstance(StateNormal, "NoData", "c"), testInstance(StateNormal, "Error", "d"), @@ -67,11 +67,11 @@ func TestLogReduceKeepsOnlyAbnormalInstances(t *testing.T) { t.Errorf("Reasons = %v, want %v", p.Reasons, want) } // The histogram is a verbatim copy of the response totals — raw keys, no - // normalization (§4.9). + // normalization. if want := map[string]int{"alerting": 1, "normal": 2}; !reflect.DeepEqual(p.Histogram, want) { t.Errorf("Histogram = %v, want %v", p.Histogram, want) } - // Rule-level state and health stay raw and unnormalized (P1.2a). + // Rule-level state and health stay raw and unnormalized. if p.State != "firing" || p.Health != "ok" { t.Errorf("State/Health = %q/%q, want firing/ok", p.State, p.Health) } @@ -83,8 +83,8 @@ func TestLogReduceKeepsOnlyAbnormalInstances(t *testing.T) { } } -// A filtered response can hold several rules sharing one title (the known -// 2-way collision, §14.5), so the reducer must select by UID. +// A filtered response can hold several rules sharing one title, so the reducer +// must select by UID. func TestLogReduceSelectsRuleByUID(t *testing.T) { first := StateRule{UID: "ruleA", Title: "Same Title", Health: "ok", State: "inactive", LastEvaluation: testNow} second := StateRule{ @@ -120,7 +120,7 @@ func TestLogReduceRuleAbsentIsAuthoritative(t *testing.T) { } } -// H2: an instance that leaves the abnormal set is resolved against the SAME +// An instance that leaves the abnormal set is resolved against the SAME // response, and MissingSeries is a vanish, never a recovery. func TestTransitionMarkersClearedVersusVanished(t *testing.T) { badKey := instanceKey(testInstance(StateFiring, "", "b").Labels) @@ -249,8 +249,8 @@ func TestTransitionMarkersAreSortedAndPerRule(t *testing.T) { } } -// §3.2: the reduction depends on the state endpoint returning normal instances. -// If it ever stops, that must fail loudly at start, never be assumed. +// The reduction depends on the state endpoint returning normal instances. If it +// ever stops, that must fail loudly at start, never be assumed. func TestLogVerifyNormalInstancesVisible(t *testing.T) { cases := []struct { fixture string @@ -275,8 +275,8 @@ func TestLogVerifyNormalInstancesVisible(t *testing.T) { if err == nil { t.Fatalf("VerifyNormalInstancesVisible: want an error, got nil") } - if !strings.Contains(err.Error(), "§3.2") { - t.Errorf("error does not name §3.2: %v", err) + if !strings.Contains(err.Error(), "no longer returns normal instances") { + t.Errorf("error does not say the endpoint stopped returning normal instances: %v", err) } return } @@ -371,7 +371,7 @@ func TestLogModeCadenceComesFromTheHeader(t *testing.T) { } } -// watch polls a fleet concurrently through one Reducer (P6), so the marker +// watch polls a fleet concurrently through one Reducer, so the marker // state it holds per rule must be safe under -race — a latent data race here // surfaces as a wrong transition, which is the one thing markers exist to get // right. @@ -405,7 +405,7 @@ func TestLogReduceIsSafeForConcurrentUse(t *testing.T) { } // A not-found poll has no evaluation time, and the artifact is read by humans -// and jq (§21.3) — the zero time must not appear as though it were real. +// and jq — the zero time must not appear as though it were real. func TestLogPollOmitsTheZeroEvaluationTime(t *testing.T) { absent := NewReducer().Reduce("rule1", observation(testNow)) b, err := json.Marshal(pollRecord{Type: RecordPoll, Poll: absent}) @@ -504,13 +504,13 @@ func TestWriterReadLogRoundTrip(t *testing.T) { t.Fatalf("sentinel is nil after Stop") } // Stop stamps the recorder's own stop time and makes no comparison - // against `to` — watch never knows it (§4.5). + // against `to` — watch never knows it. if !sentinel.Equal(testNow.Add(2 * time.Minute)) { t.Errorf("sentinel = %s, want the writer's stop time %s", sentinel, testNow.Add(2*time.Minute)) } } -// §8: the log is append-only. A second run against the same path must never +// The log is append-only. A second run against the same path must never // destroy the evidence the first one recorded. func TestWriterAppendsAndNeverTruncates(t *testing.T) { path := filepath.Join(t.TempDir(), "log.jsonl") @@ -529,7 +529,7 @@ func TestWriterAppendsAndNeverTruncates(t *testing.T) { t.Fatalf("read: %v", err) } - // The P6 handoff: the parent wrote the header and closed; the child + // The handoff: the parent wrote the header and closed; the child // reopens the same path and appends without a second header. child, _ := newTestWriter(t, path) if err := child.WritePoll(Poll{RuleUID: "rule1", Found: true, GrafanaNow: testNow.Add(time.Minute)}); err != nil { @@ -633,8 +633,8 @@ func TestSentinelStopIsIdempotentAndLast(t *testing.T) { } } -// Close is the parent's handoff path in P6: a sentinel there would tell check -// the recording ended before the child had even started. +// Close is the parent's handoff path: a sentinel there would tell check the +// recording ended before the child had even started. func TestSentinelCloseWritesNone(t *testing.T) { path := filepath.Join(t.TempDir(), "log.jsonl") w, _ := newTestWriter(t, path) @@ -658,8 +658,9 @@ func TestSentinelCloseWritesNone(t *testing.T) { } // An unfinished recording reads cleanly with a nil sentinel — ReadLog reports -// the absence and P7 turns it into unobservable. It is never ReadLog's job to -// call that a failure, and never anyone's job to call it a pass. +// the absence and the coverage proof turns it into unobservable. It is never +// ReadLog's job to call that a failure, and never anyone's job to call it a +// pass. func TestReadLogWithoutASentinel(t *testing.T) { path := filepath.Join(t.TempDir(), "log.jsonl") w, _ := newTestWriter(t, path) @@ -682,7 +683,7 @@ func TestReadLogWithoutASentinel(t *testing.T) { } } -// The read rules are deliberately the crudest possible (§24.2): any unparseable +// The read rules are deliberately the crudest possible: any unparseable // line is an error, full stop — including the last one, and including a last // line that follows a sentinel. func TestReadLogRejectsBadLogs(t *testing.T) { @@ -778,7 +779,7 @@ func TestReadLogMissingFile(t *testing.T) { } } -// §22.3: per-poll log size must not grow across polls on a high-cardinality +// Per-poll log size must not grow across polls on a high-cardinality // rule, and the one firing instance among 2446 must still be attributed by its // labels. The reduction makes size independent of NORMAL cardinality — the // firing instances are still stored, which is why a clear shrinks the record. @@ -851,8 +852,8 @@ func TestLogSizeIsFlatAcrossPollsOnAHighCardinalityRule(t *testing.T) { } // The log must stay readable by anything that reads JSONL, one flat object per -// line with its type tag — an uploaded artifact (§21.3) is read by humans and -// by jq, not only by ReadLog. +// line with its type tag — an uploaded artifact is read by humans and by jq, +// not only by ReadLog. func TestLogRecordsAreFlatOneLineObjects(t *testing.T) { path := filepath.Join(t.TempDir(), "log.jsonl") w, _ := newTestWriter(t, path) diff --git a/grafana-alertcheck/internal/gate/parse_ruler.go b/grafana-alertcheck/internal/gate/parse_ruler.go index 06f51407e..afeaf9c22 100644 --- a/grafana-alertcheck/internal/gate/parse_ruler.go +++ b/grafana-alertcheck/internal/gate/parse_ruler.go @@ -7,9 +7,9 @@ import ( "time" ) -// RuleKind classifies a ruler-endpoint rule by shape, not by name (P1.3). -// P3 rejects KindDatasourceManaged and KindRecording, but only for rules a -// user actually named — ParseDefinitions itself never rejects. +// RuleKind classifies a ruler-endpoint rule by shape, not by name. Resolve +// rejects KindDatasourceManaged and KindRecording, but only for rules a user +// actually named — ParseDefinitions itself never rejects. type RuleKind int const ( @@ -22,8 +22,8 @@ const ( // (/api/ruler/grafana/api/v1/rules). IntervalSeconds, NoDataState and // ExecErrState live inside the grafana_alert block and are only populated for // KindGrafanaManaged — a datasource-managed rule has no such block by -// definition (§11.6 drops relativeTimeRange/keep_firing_for entirely; neither -// is parsed here). +// definition. relativeTimeRange and keep_firing_for are deliberately not +// parsed: nothing in the gate reads them. type Definition struct { UID, Title, Folder, FolderUID, Group string For time.Duration @@ -43,8 +43,8 @@ func ParseDefinitions(body []byte) ([]Definition, error) { } // Map iteration order is nondeterministic; sort namespace names so - // ParseDefinitions' output order is stable across calls (P3's candidate - // listings and any golden test depend on that). + // ParseDefinitions' output order is stable across calls — Resolve's + // candidate listings and the golden tests depend on that. names := make([]string, 0, len(namespaces)) for name := range namespaces { names = append(names, name) @@ -134,11 +134,11 @@ func parseDefinition(raw json.RawMessage, folder, group string) (Definition, err // Classify by the presence of "record" before requiring anything else. // no_data_state/exec_err_state/is_paused/intervalSeconds are alerting-only // concepts a recording rule may not carry at all — its real shape is - // unverified (none exist in the fleet capture) — and P3 refuses this - // Kind categorically before any of this would gate a release. Strict- - // parsing a recording rule into a hard error over fields it was never - // going to use would brick `list` and every resolve for rules nobody - // named (§11.6, "do not reject here"). + // unverified, none exist in the fleet capture — and Resolve refuses this + // Kind categorically before any of this would gate a release. + // Strict-parsing a recording rule into a hard error over fields it was + // never going to use would brick `list` and every resolve for rules nobody + // named. var record json.RawMessage if err := opt(ga, "record", &record); err != nil { return Definition{}, fmt.Errorf("rule %q: grafana_alert: %w", uid, err) diff --git a/grafana-alertcheck/internal/gate/parse_ruler_test.go b/grafana-alertcheck/internal/gate/parse_ruler_test.go index 56d35b663..c2b9a47af 100644 --- a/grafana-alertcheck/internal/gate/parse_ruler_test.go +++ b/grafana-alertcheck/internal/gate/parse_ruler_test.go @@ -20,7 +20,7 @@ func TestParseDefinitions_RulerRules(t *testing.T) { } // The real 2-way duplicate title: same folder, same group, same title, - // distinct UIDs (§17, §22.2). + // distinct UIDs — only uid: can tell them apart. a, ok := byUID["rule0000006a"] if !ok { t.Fatalf("missing rule0000006a") @@ -81,7 +81,8 @@ func TestParseDefinitions_DatasourceManaged(t *testing.T) { } // A datasource-managed rule has no uid in this shape; its only identity // is the Prometheus "alert" name — a synthetic UID would be invented - // shape, and an empty Title would make P3's refusal-by-name unreachable. + // shape, and an empty Title would make Resolve's refusal-by-name + // unreachable. if defs[0].Title != "ExampleTargetDown" { t.Errorf("Title = %q, want ExampleTargetDown", defs[0].Title) } diff --git a/grafana-alertcheck/internal/gate/parse_state.go b/grafana-alertcheck/internal/gate/parse_state.go index 8b2464aef..a66ab6325 100644 --- a/grafana-alertcheck/internal/gate/parse_state.go +++ b/grafana-alertcheck/internal/gate/parse_state.go @@ -8,7 +8,7 @@ import ( "time" ) -// State is the canonical instance state (P1.2a). It is distinct from the raw, +// State is the canonical instance state. It is distinct from the raw, // unnormalized vocabularies the API uses at the rule level and at the instance // level — see normalizeInstanceState. type State string @@ -23,12 +23,12 @@ const ( // Instance is one entry of a rule's alerts[]. State is always canonical; Reason // is the opaque suffix of a "State (Reason)" composite ("" when the API gave a -// bare state). Reason is reporting-only except for the H2 MissingSeries routing +// bare state). Reason is reporting-only except for the MissingSeries routing // done downstream in the log markers. // -// The json tags are for the JSONL log's abnormal-instance list (P5) only — -// parsing an API response never goes through them, because parseInstance -// decodes field by field through req/opt to keep H1's presence checks explicit. +// The json tags are for the JSONL log's abnormal-instance list only — parsing +// an API response never goes through them, because parseInstance decodes field +// by field through req/opt to keep the presence checks explicit. type Instance struct { Labels map[string]string `json:"labels"` State State `json:"state"` @@ -38,12 +38,12 @@ type Instance struct { } // StateRule is one rule from the state endpoint -// (/api/prometheus/grafana/api/v1/rules), fully and strictly parsed (H1). +// (/api/prometheus/grafana/api/v1/rules), fully and strictly parsed. type StateRule struct { UID, Title, Folder, Group string Interval time.Duration // State and Health are raw, lowercase, and reporting-only — never - // classified (P1.2a). State in particular is never normalized. + // classified. State in particular is never normalized. State, Health string LastError string LastEvaluation time.Time @@ -54,7 +54,7 @@ type StateRule struct { // ParseState strictly parses a state-endpoint response body into its rules. // A missing or unparseable required field (health, state, lastEvaluation on -// each rule; interval on each group) is an error, never a zero value (H1). +// each rule; interval on each group) is an error, never a zero value. func ParseState(body []byte) ([]StateRule, error) { var top map[string]json.RawMessage if err := json.Unmarshal(body, &top); err != nil { @@ -134,11 +134,10 @@ func parseStateRule(raw json.RawMessage, folder, group string, interval time.Dur if err := req(m, "health", &r.Health); err != nil { return StateRule{}, fmt.Errorf("rule %q: %w", uid, err) } - // isPaused is not one of H1's four named required fields, but this parser - // extends that contract to it: the zero-time rule below can't tell a - // paused rule from a broken one without it, and it's the primary - // in-window pause detector (H2/§12.2) — a silent false default would be - // exactly the fail-open bug H1 exists to kill. + // isPaused is required rather than optional: the zero-time rule below + // can't tell a paused rule from a broken one without it, and it's the + // primary in-window pause detector — a silent false default would be + // exactly the fail-open this parser's strictness exists to kill. if err := req(m, "isPaused", &r.IsPaused); err != nil { return StateRule{}, fmt.Errorf("rule %q: %w", uid, err) } @@ -151,7 +150,7 @@ func parseStateRule(raw json.RawMessage, folder, group string, interval time.Dur if err != nil { return StateRule{}, fmt.Errorf("rule %q: lastEvaluation: %w", uid, err) } - // The zero-time rule (§2.3): only a paused rule may report the zero time. + // Only a paused rule may report the zero time. if lastEval.IsZero() && !r.IsPaused { return StateRule{}, fmt.Errorf("rule %q: lastEvaluation is the zero time but isPaused is false", uid) } @@ -199,10 +198,10 @@ func parseInstance(raw json.RawMessage) (Instance, error) { return Instance{}, err } - // activeAt is also not in H1's named list, extended here for the same - // reason as StateRule.IsPaused: it's the onset time BadFor (P8) measures - // from, so a silently zeroed one would misclassify how long an instance - // has been bad rather than failing loudly. + // activeAt is required for the same reason as StateRule.IsPaused: it's the + // onset time BadFor measures from, so a silently zeroed one would + // misclassify how long an instance has been bad rather than failing + // loudly. var activeAtStr string if err := req(m, "activeAt", &activeAtStr); err != nil { return Instance{}, err @@ -225,8 +224,8 @@ func parseInstance(raw json.RawMessage) (Instance, error) { } // baseInstanceStates is the strict 5-value allowlist for the base of an -// instance state (P1.2a). Anything else — including an unrecognized base -// inside a "Base (Reason)" composite — is a parse error (H1, §2.7 control 3). +// instance state. Anything else — including an unrecognized base inside a +// "Base (Reason)" composite — is a parse error. var baseInstanceStates = map[string]State{ "Normal": StateNormal, "Alerting": StateFiring, diff --git a/grafana-alertcheck/internal/gate/parse_state_test.go b/grafana-alertcheck/internal/gate/parse_state_test.go index 1ab6c620d..6e5a24297 100644 --- a/grafana-alertcheck/internal/gate/parse_state_test.go +++ b/grafana-alertcheck/internal/gate/parse_state_test.go @@ -167,7 +167,7 @@ func TestParseState_HappyPaths(t *testing.T) { t.Fatalf("Instances = %+v, want one firing instance", r.Instances) } if r.Totals["normal"] == 0 { - t.Errorf(`Totals["normal"] = 0, want >0 (this is the §3.2 mismatch the fixture exists to capture)`) + t.Errorf(`Totals["normal"] = 0, want >0 (the totals/instances mismatch this fixture exists to capture)`) } }, }, @@ -192,11 +192,11 @@ func TestParseState_HappyPaths(t *testing.T) { } } -// TestParseState_MustError is the H1 regression suite: it doesn't just check -// err != nil (a stray comma in a fixture would keep that green forever while -// the actual check regressed) — it asserts the error names the specific -// offending field or value, so a real H1 check going missing fails loudly -// here instead of surviving unnoticed. +// The strict-parsing regression suite: it doesn't just check err != nil (a +// stray comma in a fixture would keep that green forever while the actual +// check regressed) — it asserts the error names the specific offending field +// or value, so a check going missing fails loudly here instead of surviving +// unnoticed. func TestParseState_MustError(t *testing.T) { cases := []struct { fixture string @@ -283,7 +283,7 @@ func TestInstanceKey(t *testing.T) { } } -// minimalStateBody is the smallest H1-legal state response: one group, one +// minimalStateBody is the smallest legal state response: one group, one // rule, no optional keys at all, plus whatever extra is spliced in verbatim // before the rule's closing brace — for isolating one optional key at a time // rather than relying on a fixture that removes several together. @@ -294,10 +294,10 @@ func minimalStateBody(extraRuleJSON string) []byte { `"lastEvaluation":"2026-01-01T00:00:00Z"%s}]}]}}`, extraRuleJSON) } -// §22.2: keepFiringFor is named alongside alerts/totals/labels as an optional -// key (§3.1), but state_missing_optional.json removes it together with -// everything else — never in isolation, so a regression that made it -// required specifically would not be caught by that fixture alone. +// keepFiringFor is optional alongside alerts/totals/labels, but +// state_missing_optional.json removes it together with everything else — never +// in isolation, so a regression that made it required specifically would not +// be caught by that fixture alone. func TestParseState_KeepFiringForIsOptional(t *testing.T) { tests := []struct { name string @@ -319,7 +319,7 @@ func TestParseState_KeepFiringForIsOptional(t *testing.T) { } } -// §22.2: labels is optional at the INSTANCE level (opt(m, "labels", ...) in +// labels is optional at the INSTANCE level (opt(m, "labels", ...) in // parseInstance), distinct from the rule-level labels state_missing_optional.json // already covers — an instance can exist with no labels of its own. func TestParseState_InstanceWithoutLabelsParses(t *testing.T) { @@ -339,9 +339,9 @@ func TestParseState_InstanceWithoutLabelsParses(t *testing.T) { // synthesizeHighCardinalityState builds a state response with a single rule // holding `alerting` Alerting instances and `normal` Normal instances, by // cloning the one real instance in state_one_instance.json. It is never -// committed (§3.2, §22.3, §22.6) — the 2446-instance rule this stands in for -// is ~600 KB and exists only to prove the parser and (in later phases) the -// reducer don't choke on real fleet cardinality. +// committed — the 2446-instance rule this stands in for is ~600 KB and exists +// only to prove the parser and the reducer don't choke on real fleet +// cardinality. func synthesizeHighCardinalityState(t *testing.T, alerting, normal int) []byte { t.Helper() base := readFixture(t, "state_one_instance.json") diff --git a/grafana-alertcheck/internal/gate/resolve.go b/grafana-alertcheck/internal/gate/resolve.go index 3c3710038..329644f95 100644 --- a/grafana-alertcheck/internal/gate/resolve.go +++ b/grafana-alertcheck/internal/gate/resolve.go @@ -7,8 +7,8 @@ import ( "strings" ) -// Resolve turns the operator-supplied alert names into resolved Definitions -// (§17). Order is load-bearing (§17.3): +// Resolve turns the operator-supplied alert names into resolved Definitions. +// Order is load-bearing: // // 1. Trim each name. // 2. Discard empty lines. @@ -18,9 +18,9 @@ import ( // user less than a failure). // // The caller-visible consequence: len(resolved) is the count *after* the -// collapse. A later phase's MinObserved must default from that length, never -// from len(names) — using the input line count would make one rule named -// twice turn an achievable default into an unsatisfiable one (§17.3). +// collapse, and MinObserved must default from that length, never from +// len(names) — using the input line count would make one rule named twice turn +// an achievable default into an unsatisfiable one. func Resolve(defs []Definition, names []string, folder string) (resolved []Definition, notes []string, err error) { seenUID := map[string]string{} // uid -> the first input name that resolved to it for _, raw := range names { @@ -45,15 +45,15 @@ func Resolve(defs []Definition, names []string, folder string) (resolved []Defin return resolved, notes, nil } -// resolveOne resolves a single trimmed, non-empty name against defs (§17.1): -// one match wins outright, zero is an error with suggestions, two or more is -// an error listing every candidate. folder scopes a bare title (no "/" in the -// name) to one folder; it is ignored for the "Folder/Title" and -// "Folder/Group/Title" forms, which already name their own folder. +// resolveOne resolves a single trimmed, non-empty name against defs: one match +// wins outright, zero is an error with suggestions, two or more is an error +// listing every candidate. folder scopes a bare title (no "/" in the name) to +// one folder; it is ignored for the "Folder/Title" and "Folder/Group/Title" +// forms, which already name their own folder. // -// Policy on unsupported kinds (datasource-managed, recording) — decided here -// because §17.1 only says to refuse them, not how they interact with the -// no-match/ambiguous surfaces: a name can still match an unsupported rule (so +// Unsupported kinds (datasource-managed, recording) are refused, and how that +// interacts with the no-match/ambiguous surfaces is decided here: a name can +// still match an unsupported rule (so // naming one by title still gets the specific, named refusal, not a bare "no // match"), but only *supported* candidates count for ambiguity — an // unsupported rule sharing a title with a supported one is resolved silently @@ -73,7 +73,7 @@ func resolveOne(defs []Definition, name, folder string) (Definition, error) { } // uid == "" falls through to the same message as "not found": several // Definition kinds legitimately carry UID == "" (datasource-managed - // rules have no uid at all, P1.3), so matching on an empty suffix + // rules have no uid at all), so matching on an empty suffix // would silently hit one of those and report a misleading // kind-specific refusal for what is really an empty/typo'd uid. This // deliberately does not go through noMatchError: that function's @@ -118,9 +118,9 @@ func resolveOne(defs []Definition, name, folder string) (Definition, error) { } } -// supportedDefs filters out the two kinds §17.1 refuses. Only these -// participate in name-based matching, the no-match rule count, and substring -// suggestions (see the policy note on resolveOne). +// supportedDefs filters out the two refused kinds. Only these participate in +// name-based matching, the no-match rule count, and substring suggestions (see +// the policy note on resolveOne). func supportedDefs(defs []Definition) []Definition { out := make([]Definition, 0, len(defs)) for _, d := range defs { @@ -132,7 +132,7 @@ func supportedDefs(defs []Definition) []Definition { } // classifyForm splits name into the Title | Folder/Title | Folder/Group/Title -// forms (§17). A bare title is scoped by folder when the caller supplied one; +// forms. A bare title is scoped by folder when the caller supplied one; // the two- and three-segment forms already carry their own folder and ignore // it. // @@ -158,8 +158,8 @@ func classifyForm(name, folder string) (wantFolder, wantGroup, wantTitle string, } } -// refuseUnsupportedKind rejects the two kinds §17.1 names explicitly with a -// clear, specific error — distinct from "no match" and from "ambiguous" — so +// refuseUnsupportedKind rejects the two unsupported kinds with a clear, +// specific error — distinct from "no match" and from "ambiguous" — so // an operator who names a recording or datasource-managed rule learns why, // not just that nothing matched. func refuseUnsupportedKind(name string, d Definition) (Definition, error) { @@ -174,8 +174,7 @@ func refuseUnsupportedKind(name string, d Definition) (Definition, error) { } // noMatchError reports a no-match with the count of rules the gate could see -// and, per Context decision 4, case-insensitive substring matches in place of -// the source plan's cut Levenshtein suggestions (§17.2). +// and case-insensitive substring matches as suggestions. func noMatchError(defs []Definition, name, wantTitle string) error { msg := fmt.Sprintf("no rule matched %q (%d rules available; run 'grafana-alertcheck list' to see titles)", name, len(defs)) @@ -194,8 +193,8 @@ func noMatchError(defs []Definition, name, wantTitle string) error { } // ambiguousError lists every candidate with its folder, its group, and the -// full copyable Folder/Group/Title (§17.1) — including the uid: form, which -// resolves unambiguously on the next attempt. +// full copyable Folder/Group/Title — including the uid: form, which resolves +// unambiguously on the next attempt. func ambiguousError(name string, candidates []Definition) error { sorted := append([]Definition(nil), candidates...) sort.Slice(sorted, func(i, j int) bool { return sorted[i].UID < sorted[j].UID }) diff --git a/grafana-alertcheck/internal/gate/resolve_test.go b/grafana-alertcheck/internal/gate/resolve_test.go index 192ed3dc2..d58ec73bd 100644 --- a/grafana-alertcheck/internal/gate/resolve_test.go +++ b/grafana-alertcheck/internal/gate/resolve_test.go @@ -143,8 +143,8 @@ func TestResolve_RejectsEmptySegments(t *testing.T) { } func TestResolve_UIDEmptySuffix(t *testing.T) { - // ruler_datasource_managed.json's only rule has UID == "" (P1.3: this - // shape has no uid at all). "uid:" with an empty suffix must not match it + // ruler_datasource_managed.json's only rule has UID == "" — that shape has + // no uid at all. "uid:" with an empty suffix must not match it // — that would report the misleading "datasource-managed rule, not // supported" for what is really a typo'd/empty uid. defs, err := ParseDefinitions(readFixture(t, "ruler_datasource_managed.json")) @@ -216,8 +216,7 @@ func TestResolve_UnsupportedHomonymResolvesSupportedSilently(t *testing.T) { func TestResolve_CollapseByUIDGivesNoteNotError(t *testing.T) { defs := rulerDefs(t) // The bare title and its Folder/Group/Title spelling both name the same - // rule (rule0000007) — a duplicate-name copy mistake, not an error - // (§17.3). + // rule (rule0000007) — a duplicate-name copy mistake, not an error. resolved, notes, err := Resolve(defs, []string{ "example_workflow_paused_rule", "ExampleObservability/Example Auth Production/example_workflow_paused_rule", @@ -233,9 +232,8 @@ func TestResolve_CollapseByUIDGivesNoteNotError(t *testing.T) { } } -// §22.2: "the same rule with two identical names ... must collapse to one -// rule" — the literal exact-duplicate-string case, distinct from the -// different-spellings case above. +// The same rule named twice with the identical string must collapse to one +// rule — distinct from the different-spellings case above. func TestResolve_IdenticalDuplicateNameCollapsesWithNote(t *testing.T) { defs := rulerDefs(t) resolved, notes, err := Resolve(defs, []string{ @@ -264,7 +262,7 @@ func TestResolve_MinObservedCountIsPostCollapse(t *testing.T) { if err != nil { t.Fatalf("Resolve: unexpected error: %v", err) } - // §17.3: the default MinObserved must come from len(resolved) (2 distinct + // The default MinObserved must come from len(resolved) (2 distinct // rules) — never len(names) (3 input lines), which would be unsatisfiable. if len(resolved) != 2 { t.Fatalf("resolved = %+v, want 2 distinct rules after collapse", resolved) diff --git a/grafana-alertcheck/internal/gate/schedule.go b/grafana-alertcheck/internal/gate/schedule.go index 32fd93df4..c2d5e7c12 100644 --- a/grafana-alertcheck/internal/gate/schedule.go +++ b/grafana-alertcheck/internal/gate/schedule.go @@ -8,35 +8,30 @@ import ( "time" ) -// SkewHardLimit is one of §5's filled-in values (basis: §16; §22.11 asserts -// 120s errors, 30s does not). Defined here, in schedule.go's named-constants -// block, per §5's instruction — it moved out of source.go now that P4 exists; -// P2 needed it before this file did, so it started there. Exported (P10) so -// the CLI can report it verbatim next to a measured skew instead of keeping -// its own mirrored copy. +// SkewHardLimit is the largest clock skew between this runner and Grafana that +// a run tolerates before it errors out. Exported so the CLI can report it +// verbatim next to a measured skew instead of keeping its own mirrored copy. const SkewHardLimit = 60 * time.Second // fromFutureTolerance is how far ahead of the runner's own clock a supplied -// `from` may sit before check refuses it (§7: "from in the future, more than -// the skew tolerance — error"). §7 names no number, so this is the judgment -// call §5's table records: the same 60s as SkewHardLimit, because the only -// legitimate reason for a `from` in the future is clock disagreement between -// the deploy step and the check step, and that is bounded by the same figure. -// It is once-per-run input validation, not a per-rule coverage check, so -// Check applies it (P9) and proveCoverage does not. +// `from` may sit before check refuses it: the same 60s as SkewHardLimit, +// because the only legitimate reason for a `from` in the future is clock +// disagreement between the deploy step and the check step, and that is bounded +// by the same figure. It is once-per-run input validation, not a per-rule +// coverage check, so Check applies it and proveCoverage does not. const fromFutureTolerance = 60 * time.Second -// minDrainTimeout is §5's floor on drainTimeout: max(2 x max(intervalSeconds), -// 2m). Without the floor, a fleet of very tight rules would derive a -// drainTimeout too short to let a healthy in-flight poll land. +// minDrainTimeout is the floor on drainTimeout, which is otherwise +// 2 x max(intervalSeconds). Without the floor, a fleet of very tight rules +// would derive a drainTimeout too short to let a healthy in-flight poll land. const minDrainTimeout = 2 * time.Minute -// graceWarnFraction is §13.2's threshold for warning that transitionGrace eats -// too much of the requested window: "approximately one quarter of the window". +// graceWarnFraction is the share of the requested window above which +// transitionGrace is worth warning about. const graceWarnFraction = 0.25 -// ruleTimings groups the per-rule threshold values §5/§10.1/§14.1 derive from -// a rule's poll cadence and its own evaluation interval. +// ruleTimings groups the per-rule thresholds derived from a rule's poll +// cadence and its own evaluation interval. type ruleTimings struct { pollEvery time.Duration maxGap time.Duration @@ -45,26 +40,24 @@ type ruleTimings struct { } // globalTimings groups the values that apply to the whole run rather than to -// one rule: §13.1's transitionGrace and §19's drainTimeout are each derived -// once, across every non-skipped watched rule, not per rule. +// one rule: transitionGrace and drainTimeout are each derived once, across +// every non-skipped watched rule, not per rule. type globalTimings struct { transitionGrace time.Duration - // graceSource names, and already carries the `for` value of, the rule - // that set transitionGrace (§13.2 requires printing both) — one string - // field rather than a second (rule, duration) pair, matching this - // struct's fixed shape. "none" when no rule contributed (transitionGrace - // is then 0). + // graceSource names, and already carries the `for` value of, the rule that + // set transitionGrace — one string field rather than a second + // (rule, duration) pair, matching this struct's fixed shape. "none" when no + // rule contributed (transitionGrace is then 0). graceSource string drainTimeout time.Duration } // newRuleTimings derives one rule's thresholds from its fully-resolved poll -// cadence and its evaluation interval (§5, §10.1, §14.1). pollEvery arrives -// already resolved for the caller's mode — the §5 default, the operator's -// --poll-interval override, or (in log mode, a later phase) the cadence -// recorded in the log header. Deriving pollEvery inline here, instead of -// accepting it as an input, would let a caller in the wrong mode compute -// maxGap against the wrong authority — see the "Two authorities" note in P5. +// cadence and its evaluation interval. pollEvery arrives already resolved for +// the caller's mode — the default, the operator's --poll-interval override, or +// (in log mode) the cadence recorded in the log header. Deriving pollEvery +// inline here, instead of accepting it as an input, would let a caller in the +// wrong mode compute maxGap against the wrong authority. func newRuleTimings(pollEvery time.Duration, intervalSeconds int) ruleTimings { interval := time.Duration(intervalSeconds) * time.Second maxGap := 2 * pollEvery @@ -77,20 +70,20 @@ func newRuleTimings(pollEvery time.Duration, intervalSeconds int) ruleTimings { } } -// defaultPollEvery is §5's default per-rule cadence: half the rule's own +// defaultPollEvery is the default per-rule cadence: half the rule's own // evaluation interval. func defaultPollEvery(intervalSeconds int) time.Duration { return time.Duration(intervalSeconds) * time.Second / 2 } -// DeriveTimings computes every resolved rule's ruleTimings, keyed by UID, -// plus the shared globalTimings, from resolved definitions and watch's -// optional --poll-interval override (0 = no override: use each rule's §5 -// default of half its own interval). Per §5.1, a supplied override is used -// verbatim for every rule and is never clamped down to the default even when -// it exceeds intervalSeconds/2 — that case is reported back as a note, not -// silently corrected or refused, because clamping would defeat the one knob -// §5.1 gives an operator for making a tight schedule fit. +// DeriveTimings computes every resolved rule's ruleTimings, keyed by UID, plus +// the shared globalTimings, from resolved definitions and watch's optional +// --poll-interval override (0 = no override: use each rule's default of half +// its own interval). A supplied override is used verbatim for every rule and is +// never clamped down to the default even when it exceeds intervalSeconds/2 — +// that case is reported back as a note, not silently corrected or refused, +// because clamping would defeat the one knob an operator has for making a tight +// schedule fit. func DeriveTimings(defs []Definition, override time.Duration) (rules map[string]ruleTimings, global globalTimings, notes []string) { rules = make(map[string]ruleTimings, len(defs)) for _, d := range defs { @@ -124,14 +117,14 @@ func pausedSet(defs []Definition) map[string]bool { return paused } -// DeriveTimingsFromLog is DeriveTimings' log-mode counterpart, and the two -// authorities of P5 are the whole reason it exists as a separate function. -// pollEvery comes from the header — the cadence the recording ACTUALLY used, -// after any --poll-interval override — and maxGap and healthGrace follow from -// it. Re-deriving pollEvery from defs here would compare gaps recorded at the -// override cadence against thresholds computed from the default: exit 2 on a -// clean window when the override was slower, and, worse, a real recorder gap -// passing silently when it was faster. +// DeriveTimingsFromLog is DeriveTimings' log-mode counterpart, and having one +// authority for the cadence is the whole reason it exists as a separate +// function. pollEvery comes from the header — the cadence the recording +// ACTUALLY used, after any --poll-interval override — and maxGap and +// healthGrace follow from it. Re-deriving pollEvery from defs here would +// compare gaps recorded at the override cadence against thresholds computed +// from the default: exit 2 on a clean window when the override was slower, and, +// worse, a real recorder gap passing silently when it was faster. // // evalStaleAfter still comes from defs (2 x intervalSeconds): it is a property // of the rule's own evaluation cadence and is unaffected by how often the gate @@ -151,12 +144,12 @@ func pausedSet(defs []Definition) map[string]bool { // // It checks only the header-to-defs direction. The opposite direction — a // resolved definition absent from the header — is NOT this function's to -// judge: it is §19.1 step 3's log-identity validation, and it belongs to P9's -// Check, which is the only caller that knows both sets and can name the -// mismatch. Without that check a definition simply gets no timings entry, and -// a downstream lookup would read a zero maxGap: fail-closed (every gap -// exceeds it) but silent, so P9 must reject the set mismatch by name rather -// than let a rule fail for an unexplained reason. +// judge: it belongs to Check's log-identity validation, the only caller that +// knows both sets and can name the mismatch. Without that check a definition +// simply gets no timings entry, and a downstream lookup would read a zero +// maxGap: fail-closed (every gap exceeds it) but silent, so Check must reject +// the set mismatch by name rather than let a rule fail for an unexplained +// reason. func DeriveTimingsFromLog(h Header, defs []Definition) (rules map[string]ruleTimings, global globalTimings, err error) { byUID := make(map[string]Definition, len(defs)) for _, d := range defs { @@ -187,15 +180,13 @@ func DeriveTimingsFromLog(h Header, defs []Definition) (rules map[string]ruleTim return rules, deriveGlobalTimings(defs, h.pausedAtStart()), nil } -// deriveGlobalTimings computes transitionGrace and drainTimeout over defs -// (§5, §13.1, §19). +// deriveGlobalTimings computes transitionGrace and drainTimeout over defs. // -// A rule paused before the window opened — skipped, §12 — is excluded from the +// A rule paused before the window opened — skipped — is excluded from the // transitionGrace max: its `for` value can never fire during the window, so // counting it would only inflate the wait past what any watched rule actually -// needs (a judgment call the v2 plan makes explicitly for this formula; §19's -// drainTimeout carries no such exclusion, so it still runs over every resolved -// rule). +// needs. drainTimeout carries no such exclusion and still runs over every +// resolved rule. // // "Before the window opened" is the whole content of that exclusion, so the // authority is pausedAtStart and NEVER Definition.IsPaused: in log mode the @@ -226,9 +217,9 @@ func deriveGlobalTimings(defs []Definition, pausedAtStart map[string]bool) globa return g } -// Scheduler drives one per-rule schedule, never a global cycle (§5): a rule -// at intervalSeconds=10 alongside twenty at 300 keeps its own 5s cadence -// without forcing the same cadence onto the other twenty. +// Scheduler drives one per-rule schedule, never a global cycle: a rule at +// intervalSeconds=10 alongside twenty at 300 keeps its own 5s cadence without +// forcing the same cadence onto the other twenty. type Scheduler struct { next map[string]time.Time every map[string]time.Duration @@ -236,9 +227,9 @@ type Scheduler struct { // NewScheduler builds a Scheduler over per-rule cadences (keyed by UID), // staggering each rule's initial next-due time across [0, pollEvery) so the -// fleet does not start phase-aligned (§5's burst-bound proof depends on this: -// an already-staggered fleet only re-aligns by chance, briefly, not by -// construction). +// fleet does not start phase-aligned. The burst bound CheckBudget enforces +// depends on that: an already-staggered fleet only re-aligns by chance, +// briefly, not by construction. // // It takes cadences rather than whole ruleTimings on purpose: a scheduler // decides when to poll and nothing else, so it must not be handed maxGap, @@ -262,12 +253,12 @@ func NewScheduler(every map[string]time.Duration, now time.Time) *Scheduler { } // Due returns the UIDs whose next-due time has arrived, earliest-due-first. -// Ties (equal next-due time) break by tightest cadence first: the burst-bound -// proof in §5 assumes a newly-due tight rule waits at most for one in-flight -// request, which only holds if a simultaneous batch serves the tightest rule -// ahead of slacker ones. A tie-break that instead followed map iteration -// order would silently void that proof — nothing else would fail until a -// phase-aligned fleet opened a mid-run gap in production. +// Ties (equal next-due time) break by tightest cadence first: the burst bound +// assumes a newly-due tight rule waits at most for one in-flight request, which +// only holds if a simultaneous batch serves the tightest rule ahead of slacker +// ones. A tie-break that instead followed map iteration order would silently +// void that assumption — nothing else would fail until a phase-aligned fleet +// opened a mid-run gap in production. func (s *Scheduler) Due(now time.Time) []string { var due []string for uid, t := range s.next { @@ -295,11 +286,10 @@ func (s *Scheduler) Mark(uid string, now time.Time) { } // earliestDue returns the earliest scheduled next-due time, and false when the -// scheduler holds no rules at all. The recorder's loop (P6) waits exactly that -// long instead of waking on a fixed tick: a fixed tick either polls a slack -// rule early — spending request budget the §5 formulas already accounted for — -// or wakes too late for the tightest rule and opens a gap inside its own -// maxGap. +// scheduler holds no rules at all. The recorder's loop waits exactly that long +// instead of waking on a fixed tick: a fixed tick either polls a slack rule +// early — spending request budget the schedule already accounted for — or wakes +// too late for the tightest rule and opens a gap inside its own maxGap. func (s *Scheduler) earliestDue() (time.Time, bool) { var earliest time.Time for _, t := range s.next { @@ -310,23 +300,21 @@ func (s *Scheduler) earliestDue() (time.Time, bool) { return earliest, !earliest.IsZero() } -// CheckBudget applies §5's error-at-start check to a fully resolved schedule. -// t and measured are both keyed by rule UID; measured must carry every UID in -// t; a rule this run never measured can't have its budget proved, and a -// silent zero-duration default would be exactly the kind of pass-on-an- -// unproven-window bug §5 exists to catch. CheckBudget fails when any of three -// conditions holds (sanity-checked against §22.3's mixed-interval regression -// in this phase's tests): +// CheckBudget proves at start that a fully resolved schedule can actually be +// served. t and measured are both keyed by rule UID, and measured must carry +// every UID in t: a rule this run never measured cannot have its budget +// proved, and a silent zero-duration default would be exactly the kind of +// pass-on-an-unproven-window bug this check exists to catch. It fails when any +// of three conditions holds: // // - utilization: the long-run request rate exceeds what concurrency serves; // - a single rule's own request cannot fit inside its own cadence; -// - the burst bound: the slowest measured request is slower than the -// fleet's tightest cadence, which — even under earliest-due-first -// ordering — can open a mid-run gap bigger than that rule's maxGap. +// - the burst bound: the slowest measured request is slower than the fleet's +// tightest cadence, which — even under earliest-due-first ordering — can +// open a mid-run gap bigger than that rule's maxGap. // -// The message never suggests a single interval (§5.1) — only the three -// controls an operator actually has: concurrency, poll-interval, and the -// alert list. +// The message names only the three controls an operator actually has: +// concurrency, poll-interval, and the alert list. func CheckBudget(t map[string]ruleTimings, measured map[string]time.Duration, concurrency int) error { if len(t) == 0 { return nil @@ -393,10 +381,11 @@ func CheckBudget(t map[string]ruleTimings, measured map[string]time.Duration, co return fmt.Errorf("%s", b.String()) } -// StartupSummary formats §13.2's required pre-run print: the total planned -// run time and the rule (with its `for` value) that set transitionGrace, plus -// a warning when the grace eats more than graceWarnFraction of the requested -// window. from/to are the requested classification window. +// StartupSummary formats the pre-run print an operator sees before the wait: +// the total planned run time and the rule (with its `for` value) that set +// transitionGrace, plus a warning when the grace eats more than +// graceWarnFraction of the requested window. from/to are the requested +// classification window. func StartupSummary(from, to time.Time, global globalTimings) (summary, warning string) { window := to.Sub(from) total := window + global.transitionGrace + global.drainTimeout diff --git a/grafana-alertcheck/internal/gate/schedule_test.go b/grafana-alertcheck/internal/gate/schedule_test.go index 4e4023152..9a235bc7f 100644 --- a/grafana-alertcheck/internal/gate/schedule_test.go +++ b/grafana-alertcheck/internal/gate/schedule_test.go @@ -69,7 +69,7 @@ func TestDeriveTimings_TransitionGraceExcludesSkippedRule(t *testing.T) { } } -// §22.2/§22.3: `for: 1d` and `for: 1w` parse correctly (parse_ruler_test.go), +// `for: 1d` and `for: 1w` parse correctly (parse_ruler_test.go), // but that alone never proves they flow into transitionGrace — a Prometheus // duration parser that silently truncated to time.Duration's other units, or // a transitionGrace derivation that only ever saw hand-built values, could @@ -148,7 +148,7 @@ func TestDeriveTimingsFromLog_TransitionGraceFollowsTheHeaderNotTheDefinition(t }) t.Run("drainTimeout counts every rule either way", func(t *testing.T) { - // §19 puts no pause exclusion on drainTimeout, so both headers give the + // drainTimeout carries no pause exclusion, so both headers give the // same floor-bound value. for _, pausedAtStart := range []bool{false, true} { h := Header{Rules: []LoggedRule{loggedRule("r1", pausedAtStart)}} @@ -192,9 +192,8 @@ func TestDeriveTimings_DrainTimeoutAboveFloor(t *testing.T) { } } -// TestScheduler_DueOrderingTiesBreakByTightestCadence pins the ordering -// invariant the burst bound depends on (§5): when several rules become due at -// the exact same instant, Due must serve the tightest cadence first, not +// The ordering invariant the burst bound depends on: when several rules become +// due at the exact same instant, Due must serve the tightest cadence first, not // whatever order the underlying map happens to iterate in. A refactor that // loses this ordering must fail here, not in a production phase-aligned gap. func TestScheduler_DueOrderingTiesBreakByTightestCadence(t *testing.T) { @@ -244,8 +243,8 @@ func TestScheduler_MarkAdvancesNextDue(t *testing.T) { } // TestScheduler_PerRuleCadenceOverTime simulates a run and counts how often -// each rule comes due, pinning §5's core claim: schedules are per rule, never -// a global cycle. A tight rule must be polled at its own cadence regardless +// each rule comes due: schedules are per rule, never a global cycle. A tight +// rule must be polled at its own cadence regardless // of what slower rules in the same fleet need, and a slack rule must never be // forced onto the tight rule's cadence. func TestScheduler_PerRuleCadenceOverTime(t *testing.T) { @@ -291,9 +290,8 @@ func TestNewScheduler_StaggersWithinPollEvery(t *testing.T) { } } -// TestCheckBudget_MixedIntervalRegression is §22.3's sanity check from the -// plan: one rule at 10s beside twenty at 300s, all measured ~1.8s, must not -// error at any reasonable concurrency — the exact case a naive worst-case-slot +// One rule at 10s beside twenty at 300s, all measured ~1.8s, must not error at +// any reasonable concurrency — the exact case a naive worst-case-slot // simulation would wrongly fail. func TestCheckBudget_MixedIntervalRegression(t *testing.T) { timings := map[string]ruleTimings{"tight": {pollEvery: 5 * time.Second}} @@ -386,9 +384,9 @@ func TestCheckBudget_EmptyScheduleIsFine(t *testing.T) { } } -// assertBudgetMessage checks §5.1's required message contents: a measured -// duration is present, and all three controls are named — never a single -// suggested interval. +// assertBudgetMessage checks the message contents: a measured duration is +// present, and all three controls are named — never a single suggested +// interval. func assertBudgetMessage(t *testing.T, msg string) { t.Helper() for _, want := range []string{"measured", "concurrency", "poll-interval", "fewer"} { @@ -418,13 +416,10 @@ func TestStartupSummary_WarningWhenGraceTooLarge(t *testing.T) { } } -// §22.3: "a rule with for: 15m in a 10-minute window gives the warning about -// a large grace period" — no such rule exists in the real capture -// (testdata/README.md), so the test above pins the mechanism with a -// hand-built globalTimings. This drives the same warning off the real -// ruler_rules.json fixture's for:1w rule instead, tying ParseDefinitions and -// DeriveTimings into the warning end to end, not just the warning formula in -// isolation. +// The test above pins the warning formula with a hand-built globalTimings. +// This drives the same warning off the real ruler_rules.json fixture's for:1w +// rule instead, tying ParseDefinitions and DeriveTimings into the warning end +// to end. func TestStartupSummary_RealForOneWeekRuleTriggersWarning(t *testing.T) { defs := rulerDefs(t) _, global, notes := DeriveTimings(defs, 0) diff --git a/grafana-alertcheck/internal/gate/source.go b/grafana-alertcheck/internal/gate/source.go index 58008087f..3a3c7587c 100644 --- a/grafana-alertcheck/internal/gate/source.go +++ b/grafana-alertcheck/internal/gate/source.go @@ -14,8 +14,8 @@ import ( "time" ) -// Clock is the seam that lets tests advance time without sleeping (§22) — the -// only two operations the gate ever needs from a clock. +// Clock is the seam that lets tests advance time without sleeping — the only +// two operations the gate ever needs from a clock. type Clock interface { Now() time.Time After(d time.Duration) <-chan time.Time @@ -29,9 +29,9 @@ func (SystemClock) After(d time.Duration) <-chan time.Time { return time.After(d // Observation is one successful poll of the state endpoint for a single rule. type Observation struct { - Rules []StateRule // may be empty — an authoritative 2xx saying the rule is absent (§14.5) - GrafanaNow time.Time // the Date header — H4 - Skew time.Duration // serverDate - (t_send+t_headers)/2, signed (§16) + Rules []StateRule // may be empty — an authoritative 2xx saying the rule is absent + GrafanaNow time.Time // the response's Date header + Skew time.Duration // serverDate - (t_send+t_headers)/2, signed SkewBound time.Duration // (t_headers-t_send)/2 — RTT/2 to the response headers Latency time.Duration // t_send through the full body read — see requestResult.Latency } @@ -40,7 +40,7 @@ type Observation struct { // network failure, or a body that failed to parse. It is never a deleted rule // (an authoritative 2xx with no matching rule is not this) and never a clock // problem (a missing/unparseable Date header or an out-of-bounds skew is a -// hard error instead — see doRequest). Never conflate them (§14.5). +// hard error instead — see doRequest). Never conflate them. type TransportError struct { Err error Status int // 0 when the failure never got a status (network/transport failure) @@ -59,10 +59,10 @@ func (e *TransportError) Unwrap() error { return e.Err } // too many sequential *TransportError failures. It deliberately does not // implement Unwrap into the underlying *TransportError: once retries are // exhausted the result is a hard, terminal failure, and -// errors.AsType[*TransportError] must never re-classify it as retryable — -// that is the exact conflation §19.3 case 1 forbids. Cause is still exposed -// as a plain field (and folded into Error()'s text) so a caller can log or -// inspect it; it just cannot flow back into the retry classification. +// errors.AsType[*TransportError] must never re-classify it as retryable. +// Cause is still exposed as a plain field (and folded into Error()'s text) so +// a caller can log or inspect it; it just cannot flow back into the retry +// classification. type RetryExhaustedError struct { Failures int Cause error @@ -73,7 +73,7 @@ func (e *RetryExhaustedError) Error() string { } // Source is everything the gate reads from Grafana. httpSource is the one -// production implementation; every later phase's tests use a scripted fake +// production implementation; the tests use a scripted fake // (source_fake_test.go) instead of real HTTP. type Source interface { Version(ctx context.Context) (string, error) @@ -129,7 +129,7 @@ func parseGrafanaVersion(s string) (grafanaVersion, error) { } // supportedGrafanaMin and supportedGrafanaMax bound the platform this gate is -// verified against (§2.7 control 2, §21.5): >= 13.0.0, < 14.0.0. +// verified against: >= 13.0.0, < 14.0.0. var ( supportedGrafanaMin = grafanaVersion{13, 0, 0} supportedGrafanaMax = grafanaVersion{14, 0, 0} // exclusive @@ -137,8 +137,9 @@ var ( // CheckGrafanaVersion enforces the supported range. An unparseable or // out-of-range version is a hard error naming both what was found and what is -// supported — trusting an unverified schema is exactly the deprecation risk -// §2.7 control 2 exists to catch. +// supported: the response schemas this gate parses are only verified against +// that range, and trusting an unverified one is how a deprecation turns into a +// silent misread. func CheckGrafanaVersion(version string) error { v, err := parseGrafanaVersion(version) if err != nil { @@ -152,12 +153,12 @@ func CheckGrafanaVersion(version string) error { return nil } -// httpSource is the production Source: stdlib net/http only, bearer auth -// from a token supplied at construction (the caller reads it from the -// environment — §20.2 — this type never touches env itself), and manual -// strict decoding via ParseState/ParseDefinitions (H1). The retry limit and -// backoff parameters are struct fields with production defaults set here, -// not package constants, so a test can shrink them without a hook. +// httpSource is the production Source: stdlib net/http only, bearer auth from +// a token supplied at construction (the caller reads it from the environment; +// this type never touches env itself), and manual strict decoding via +// ParseState/ParseDefinitions. The retry limit and backoff parameters are +// struct fields with production defaults set here, not package constants, so a +// test can shrink them without a hook. type httpSource struct { baseURL string token string @@ -169,9 +170,8 @@ type httpSource struct { backoffCap time.Duration } -// NewHTTPSource builds the production Source. token is never logged and -// never enters an error string (§20.2) — it is used only to set the -// Authorization header. +// NewHTTPSource builds the production Source. token is never logged and never +// enters an error string — it is used only to set the Authorization header. func NewHTTPSource(baseURL, token string, clock Clock) Source { return &httpSource{ baseURL: strings.TrimSuffix(baseURL, "/"), @@ -228,8 +228,8 @@ func (s *httpSource) RuleState(ctx context.Context, title string) (Observation, if parseErr != nil { // Treated as transient, not a schema break: an unparseable 2xx // is far more likely a mid-stream hiccup than a permanent shape - // change, and H1's strict parser already turns a real shape - // change into a loud per-field error the moment it's visible. + // change, and the strict parser already turns a real shape change + // into a loud per-field error the moment it's visible. return Observation{}, &TransportError{Err: fmt.Errorf("parse rule state: %w", parseErr)} } return Observation{ @@ -244,33 +244,32 @@ func (s *httpSource) RuleState(ctx context.Context, title string) (Observation, // requestResult is the outcome of one successful HTTP attempt in doRequest: // the raw body plus everything derived from timing the round trip against -// the response's own clock (§16). +// the response's own clock. type requestResult struct { Body []byte - ServerDate time.Time // the Date header — H4 + ServerDate time.Time // the response's Date header Skew time.Duration // serverDate - (t_send+t_headers)/2, signed SkewBound time.Duration // (t_headers-t_send)/2 — RTT/2 to the response headers - // Latency spans t_send through the full body read (§5.2's budget check - // needs the whole poll's wall time, or a schedule feasibility check that - // only sees header latency goes optimistic — fail-open). It does not - // include the caller's subsequent JSON parse (ParseState/ParseDefinitions - // run outside doRequest); if P4's budget accounting needs parse time - // folded in too, extend here rather than approximating it at the call - // site. + // Latency spans t_send through the full body read: the budget check needs + // the whole poll's wall time, or a schedule feasibility check that only + // sees header latency goes optimistic — fail-open. It does not include the + // caller's subsequent JSON parse (ParseState/ParseDefinitions run outside + // doRequest); if the budget accounting ever needs parse time folded in too, + // extend here rather than approximating it at the call site. Latency time.Duration } -// doRequest performs one HTTP GET and classifies the outcome (§14.5, §16): -// a network failure, a non-2xx status, or a body-read failure is retryable +// doRequest performs one HTTP GET and classifies the outcome: a network +// failure, a non-2xx status, or a body-read failure is retryable // (*TransportError); a missing or unparseable Date header, or a skew beyond // SkewHardLimit, is a hard error — retrying can never fix either, so neither -// may enter the backoff loop (H4). +// may enter the backoff loop. // // The Date-header/skew check runs for every endpoint this hits, including -// /api/health — broader than §16's own scope, which only discusses the state -// endpoint. Deliberate: a skewed clock discovered only once RuleState starts -// polling is a skew that has already masked whatever /api/health and the -// ruler read reported; failing closed at the first response catches it +// /api/health, and not only the state endpoint whose timestamps the gate +// actually compares. Deliberate: a skewed clock discovered only once RuleState +// starts polling is a skew that has already masked whatever /api/health and +// the ruler read reported; failing closed at the first response catches it // before any of that is trusted, and every response comes with a Date header // for free. func (s *httpSource) doRequest(ctx context.Context, path string) (requestResult, error) { @@ -303,7 +302,7 @@ func (s *httpSource) doRequest(ctx context.Context, path string) (requestResult, dateHeader := resp.Header.Get("Date") if dateHeader == "" { - return requestResult{}, fmt.Errorf("%s: response has no Date header (H4)", path) + return requestResult{}, fmt.Errorf("%s: response has no Date header", path) } serverDate, parseErr := http.ParseTime(dateHeader) if parseErr != nil { @@ -318,7 +317,7 @@ func (s *httpSource) doRequest(ctx context.Context, path string) (requestResult, absSkew = -absSkew } if absSkew > SkewHardLimit { - return requestResult{}, fmt.Errorf("%s: clock skew %s exceeds hard limit %s (§16)", path, absSkew, SkewHardLimit) + return requestResult{}, fmt.Errorf("%s: clock skew %s exceeds hard limit %s", path, absSkew, SkewHardLimit) } return requestResult{Body: b, ServerDate: serverDate, Skew: signedSkew, SkewBound: bound, Latency: latency}, nil @@ -327,9 +326,9 @@ func (s *httpSource) doRequest(ctx context.Context, path string) (requestResult, // retryTransport runs fn, retrying with backoff only while it fails with a // *TransportError — any other error is a hard error and returns immediately, // never retried. failures counts consecutive *TransportError results; -// exceeding maxFailures gives up with a wrapped hard error (§19.3 case 1). -// The wait between attempts goes through clock.After so a test with a fake -// Clock never sleeps on real time (§22). +// exceeding maxFailures gives up with a wrapped hard error. The wait between +// attempts goes through clock.After so a test with a fake Clock never sleeps on +// real time. func retryTransport[T any](ctx context.Context, clock Clock, maxFailures int, backoffBase, backoffCap time.Duration, fn func() (T, error)) (T, error) { var zero T failures := 0 @@ -354,7 +353,7 @@ func retryTransport[T any](ctx context.Context, clock Clock, maxFailures int, ba } // backoffDelay is 1s base, doubling per failure, capped at maxDelay, with -// ±20% jitter (§5's filled-in value for maxSequentialFailures). +// ±20% jitter. func backoffDelay(base, maxDelay time.Duration, failureCount int) time.Duration { d := base for i := 1; i < failureCount && d < maxDelay; i++ { diff --git a/grafana-alertcheck/internal/gate/source_fake_test.go b/grafana-alertcheck/internal/gate/source_fake_test.go index 5f8cbc9e4..e804bd6a8 100644 --- a/grafana-alertcheck/internal/gate/source_fake_test.go +++ b/grafana-alertcheck/internal/gate/source_fake_test.go @@ -8,15 +8,13 @@ import ( ) // fakeClock is a manually-advanced Clock — no test in this package sleeps on -// real time (§22). It is goroutine-safe (a concurrent fleet under -race must -// not trip on the double itself), but After always fires immediately, -// regardless of the requested duration or whether Advance was ever called. -// That is sufficient here: every retry/backoff test in this phase only needs -// to avoid a real sleep. It is NOT sufficient for a test that must prove a -// wait did not fire early — e.g. a P4 scheduler test asserting Due() doesn't -// return a rule before its next-due time. Use virtualClock below for that: it -// is the clock P6's recorder-loop tests needed, and it makes a wait and the -// passage of time the same event. +// real time. It is goroutine-safe (a concurrent fleet under -race must not trip +// on the double itself), but After always fires immediately, regardless of the +// requested duration or whether Advance was ever called. That is enough for the +// retry/backoff tests, which only need to avoid a real sleep. It is NOT enough +// for a test that must prove a wait did not fire early — e.g. asserting Due() +// does not return a rule before its next-due time. Use virtualClock below for +// that: it makes a wait and the passage of time the same event. type fakeClock struct { mu sync.Mutex now time.Time @@ -112,12 +110,11 @@ type scriptedObservation struct { err error } -// fakeSource is a scripted Source with no HTTP, goroutine-safe so a phase -// that polls several rules concurrently (P6) can share one instance across -// goroutines without tripping -race. P3 through at least P5 can construct -// one directly instead of talking to HTTP; a phase that needs it to behave -// like a live server under concurrent load beyond simple locking should -// verify that assumption rather than take this comment's word for it. +// fakeSource is a scripted Source with no HTTP, goroutine-safe so a test that +// polls several rules concurrently can share one instance across goroutines +// without tripping -race. A test that needs it to behave like a live server +// under concurrent load beyond simple locking should verify that assumption +// rather than take this comment's word for it. type fakeSource struct { mu sync.Mutex diff --git a/grafana-alertcheck/internal/gate/source_test.go b/grafana-alertcheck/internal/gate/source_test.go index 2a4b9dc71..9bad3a578 100644 --- a/grafana-alertcheck/internal/gate/source_test.go +++ b/grafana-alertcheck/internal/gate/source_test.go @@ -90,7 +90,7 @@ func TestCheckGrafanaVersion(t *testing.T) { } for _, want := range c.wantContains { if !strings.Contains(err.Error(), want) { - t.Errorf("CheckGrafanaVersion(%q): error %q does not mention %q (the plan requires naming both what was found and what is supported)", c.version, err.Error(), want) + t.Errorf("CheckGrafanaVersion(%q): error %q does not mention %q — it must name both what was found and what is supported", c.version, err.Error(), want) } } } @@ -170,7 +170,7 @@ func TestHTTPSource_RuleState_EmptyIsNotAnError(t *testing.T) { t.Fatalf("Rules = %+v, want empty (an authoritative 2xx is not a transport error)", obs.Rules) } if obs.GrafanaNow.IsZero() { - t.Fatalf("GrafanaNow is zero, want the response's Date header value (H4)") + t.Fatalf("GrafanaNow is zero, want the response's Date header value") } } @@ -274,7 +274,7 @@ func TestHTTPSource_MissingDateHeader(t *testing.T) { src := NewHTTPSource(srv.URL, "", clock) _, err := src.Version(context.Background()) if err == nil { - t.Fatalf("Version(): want error, got nil (H4: a missing Date header is a hard error)") + t.Fatalf("Version(): want error, got nil: a missing Date header is a hard error") } if calls.Load() != 1 { t.Fatalf("calls = %d, want 1 — a missing Date header must never be retried", calls.Load()) @@ -294,7 +294,7 @@ func TestHTTPSource_UnparseableDateHeader(t *testing.T) { src := NewHTTPSource(srv.URL, "", clock) _, err := src.Version(context.Background()) if err == nil { - t.Fatalf("Version(): want error, got nil (H4: an unparseable Date header is a hard error)") + t.Fatalf("Version(): want error, got nil: an unparseable Date header is a hard error") } if calls.Load() != 1 { t.Fatalf("calls = %d, want 1 — an unparseable Date header must never be retried", calls.Load()) @@ -343,14 +343,14 @@ func TestHTTPSource_ObservationTiming(t *testing.T) { t.Errorf("SkewBound = %v, want 1s (RTT/2 with a 2s round trip to headers)", obs.SkewBound) } if obs.Latency != 4*time.Second { - t.Errorf("Latency = %v, want 4s (send through full body read, §5.2) — not just the 2s header round trip", obs.Latency) + t.Errorf("Latency = %v, want 4s (send through full body read) — not just the 2s header round trip", obs.Latency) } }) } } -// §22.7/§16: a genuinely discriminating regression for "the gate compares -// staleness against the Date header, never the runner's clock." lastEvaluation +// A discriminating regression for "the gate compares staleness against the +// Date header, never the runner's clock". lastEvaluation // sits 100s behind Grafana's TRUE now (obs.GrafanaNow, from the Date header) // — under the 120s evalStaleAfter limit — but 130s behind the RUNNER's clock. // An implementation that leaked the runner's clock into the staleness @@ -531,8 +531,7 @@ func TestHTTPSource_NetworkFailureRetries(t *testing.T) { // it names how many failures it gave up after, and — the regression this // pins — it is never itself classified as a *TransportError. If it were, // something one layer up that also retries on *TransportError would treat an -// already-exhausted give-up as retryable again, the exact conflation §19.3 -// case 1 forbids. +// already-exhausted give-up as retryable again. func assertRetryExhausted(t *testing.T, err error, wantFailures int) { t.Helper() var reErr *RetryExhaustedError diff --git a/grafana-alertcheck/internal/gate/testdata/README.md b/grafana-alertcheck/internal/gate/testdata/README.md index fb437b3ae..1a6bf5877 100644 --- a/grafana-alertcheck/internal/gate/testdata/README.md +++ b/grafana-alertcheck/internal/gate/testdata/README.md @@ -1,6 +1,6 @@ # Fixture provenance -All fixtures are sanitized slices of the real Grafana 13.1.0 payloads captured next to the plan in +All fixtures are sanitized slices of real Grafana 13.1.0 payloads captured into `tmp/` (`tmp/state_all.json`, `tmp/ruler_all.json`, `tmp/health.json` — gitignored, never committed). Renames are consistent across files: the same real folder/rule keeps the same fake identity everywhere it appears (e.g. `folder0000002`/`rule0000002` is the same real paused rule in both @@ -23,45 +23,38 @@ here instead, for every fixture, for consistency. `rule0000002`/"Example Paused Rule". Unmodified: `isPaused:true`, zero `lastEvaluation`, `health:ok`, `state:inactive`, absent `alerts`/`labels`. - **state_health_error.json** — real `health:error` rule ("[JD] No Job Proposals", folder - `job-distributor`), highest priority per §22.1. Renamed to folder `ExampleService`/`folder0000003`, + `job-distributor`). Renamed to folder `ExampleService`/`folder0000003`, rule `rule0000003`/"Example No Data Source". Unmodified: `health:error`, `lastError` text, the single `Error` instance. - **state_health_nodata.json** — real `health:nodata` rule ("ARE test", folder `diegos_playground`). Renamed to folder `ExamplePlayground`/`folder0000004`, rule `rule0000004`/"Example NoData Rule". Unmodified: `health:nodata`, the single `NoData` instance. -- **state_reason_composite.json** — composite of two real instances combined under one rule for P1.2a - coverage: a real `"Normal (Error)"` instance (from a Flux-reconciliation rule; 14 of that state exist +- **state_reason_composite.json** — two real instances combined under one rule to cover composite + state parsing: a real `"Normal (Error)"` instance (from a Flux-reconciliation rule; 14 of that state exist in the capture) and a real `"Normal (NoData)"` instance (from a pod-liveness rule; 1091 of that state exist), plus one plain `"Normal"` instance for contrast. Renamed to folder `ExampleInfra`/`folder0000005`, rule `rule0000005`/"Example Composite Reasons". - **state_missing_optional.json** — derived from `state_one_instance.json`: `alerts`, `totals`, `totalsFiltered` and `labels` all removed. Must parse with `Instances=nil`, `Totals=nil`. -- **state_missing_health.json** — derived from `state_one_instance.json`: the required `health` key - removed. Must be a parse error (H1). -- **state_missing_lasteval.json** — derived from `state_one_instance.json`: the required - `lastEvaluation` key removed. Must be a parse error (H1). -- **state_missing_state.json** — derived from `state_one_instance.json`: the required rule-level - `state` key removed. Must be a parse error (H1). Closes must-error coverage for H1's four required - fields — a review pass found `health`/`lastEvaluation` covered but `state`/`interval` weren't, even - though the code already `req`'d them correctly. -- **state_missing_interval.json** — derived from `state_one_instance.json`: the required group-level - `interval` key removed. Must be a parse error (H1); same review-pass gap as above. +- **state_missing_health.json**, **state_missing_lasteval.json**, **state_missing_state.json**, + **state_missing_interval.json** — derived from `state_one_instance.json`, each with one of the four + required keys removed (`health`, `lastEvaluation`, rule-level `state`, group-level `interval`). Each + must be a parse error. - **state_missing_file.json** / **state_missing_name.json** — derived from `state_one_instance.json`: - the group-level `file`/`name` keys removed respectively. Not part of H1's four (those are `health`, - `state`, `lastEvaluation`, `interval`), but the code treats group identity as strict too, and the same - review pass flagged the gap — closed rather than deferred to a later §22 sweep since the fixture is - the same 10-line edit. + the group-level `file`/`name` keys removed respectively. Not among the four required fields above, + but the parser treats group identity as strict too. - **state_zerotime_unpaused.json** — derived from `state_one_instance.json`: `lastEvaluation` set to - the zero time while `isPaused` stays `false`. Must be a parse error (§2.3). + the zero time while `isPaused` stays `false`. Must be a parse error — only a paused rule may report + the zero time. - **state_unknown_state.json** — derived from `state_one_instance.json`: the instance state hand-edited to `"Weird (NoData)"`, a syntactically valid composite whose base isn't in the 5-value allowlist. Must - be a parse error (P1.2a). + be a parse error. - **state_only_active_instances.json** — derived from a real rule that genuinely had 1 `Alerting` + 22 `Normal` instances (`totals: {alerting:1, normal:22}`, rule `dfhp1t5pkosu8f`, folder `BCM`). `alerts[]` - trimmed to the single `Alerting` instance only, while `totals` is left **unchanged** — reproducing the - §3.2 violation shape (instance list says "only active" while totals disagrees). Renamed to folder - `ExampleTeam`/`folder0000001`, rule `rule0000006`. `ParseState` itself parses this fine; the §3.2 - verification lives in a later phase (P5/P9). + trimmed to the single `Alerting` instance only, while `totals` is left **unchanged** — the shape a + state endpoint that stopped returning normal instances would produce (the instance list says "only + active" while totals disagrees). Renamed to folder `ExampleTeam`/`folder0000001`, rule `rule0000006`. + `ParseState` itself parses this fine; `VerifyNormalInstancesVisible` is what rejects it. ## Ruler endpoint (`/api/ruler/grafana/api/v1/rules`) @@ -69,7 +62,7 @@ here instead, for every fixture, for consistency. - The real true 2-way title collision: namespace `CRE-BCM-Prod-Zone-A`, group `Gateway`, identical folder+group+title, distinct UIDs (`ffvabtvvbozcwf`/`efvabtwbxlvk0b`) — renamed to namespace `Example-Zone-A`, rules `rule0000006a`/`rule0000006b`, both titled "Example No Gateways Available". - Folder/Group/Title alone does **not** disambiguate this pair (§17, §22.2). + Folder/Group/Title alone does **not** disambiguate this pair; only `uid:` does. - The 3 real `is_paused:true` rules, renamed to `rule0000002`/`rule0000007`/`rule0000008`. `rule0000002` intentionally shares its identity (`folder0000002`) with `state_paused.json`. - A real `for:1d` rule (`afs438kjd4v7kd` → `rule0000009`). @@ -81,7 +74,7 @@ here instead, for every fixture, for consistency. Grafana represents a datasource-managed (native Prometheus-format) alerting rule. `ParseDefinitions` must classify it as `KindDatasourceManaged`, parse `Title` from `alert`, and leave `UID` empty (this shape has no uid at all — inventing one would be inventing shape) without rejecting the rule - (rejection is P3's job, only for rules a user actually named). + (rejection is `Resolve`'s job, and only for rules a user actually named). - **ruler_recording.json** — **DERIVED**, no recording rule exists in the capture (verified: 0 rules carry `grafana_alert.record`). Hand-built: a `grafana_alert` block with a `record` sub-object but deliberately *without* `no_data_state`/`exec_err_state`/`is_paused`/`intervalSeconds`/`namespace_uid` diff --git a/grafana-alertcheck/internal/gate/watch.go b/grafana-alertcheck/internal/gate/watch.go index 67f60954d..7c663025f 100644 --- a/grafana-alertcheck/internal/gate/watch.go +++ b/grafana-alertcheck/internal/gate/watch.go @@ -14,7 +14,7 @@ import ( ) // DaemonChildFlag is the hidden flag the parent passes when it re-execs itself -// as the detached recorder (§4.4). It is deliberately absent from the CLI's +// as the detached recorder. It is deliberately absent from the CLI's // usage text: an operator never types it, and a child started by hand against // a log no parent prepared fails immediately on the header read. const DaemonChildFlag = "--daemon-child" @@ -26,7 +26,7 @@ const DaemonChildFlag = "--daemon-child" // loop — and not an assumption drawn from surviving a timer. A timer cannot // tell a healthy child from one that is about to die on a slow runner, and // getting that wrong means watch returns success over a recording that never -// happened (§4.3). +// happened. const ReadyFDFlag = "--ready-fd" // childReadyTimeout bounds that wait. Everything before the signal is local — @@ -43,7 +43,7 @@ const daemonLogTailBytes = 4096 // // It has no To field and must never gain one: watch writes the stopped // sentinel with its OWN stop time and makes no comparison against `to`, which -// only check knows (§4.5). Passing `to` here would give two components an +// only check knows. Passing `to` here would give two components an // opinion about the same comparison, and the recorder's opinion is the one // that cannot be trusted — it exits before the grace it would have to wait for. // @@ -55,18 +55,18 @@ const daemonLogTailBytes = 4096 // Header carries no States field for the same reason. type WatchConfig struct { // URL and Token are the connection details. The CLI reads both from the - // environment and never from a flag (§20.2); Token is never logged and - // never enters an error string. + // environment and never from a flag; Token is never logged and never + // enters an error string. URL, Token string - // Alerts are the operator-supplied names, one per line, in any of §17's - // forms. Empty lines are discarded by Resolve. + // Alerts are the operator-supplied names, one per line, in any of the forms + // Resolve accepts. Empty lines are discarded by Resolve. Alerts []string Folder string // Out is the JSONL log path. PidFile and DaemonLog default to // .pid and .daemon.log — the same convention check uses to find - // the recorder it must stop (P9), so nothing has to be wired by hand. + // the recorder it must stop, so nothing has to be wired by hand. Out string PidFile string DaemonLog string @@ -77,11 +77,10 @@ type WatchConfig struct { Until time.Time // PollEvery is the --poll-interval override, used verbatim for every rule - // and never clamped (§5.1). Zero means each rule polls at half its own - // evaluation interval. Whatever this resolves to is written into the header - // as the cadence actually used, and that header value — never a - // re-derivation from the definitions — is what check derives maxGap from - // (P5, "two authorities"). + // and never clamped. Zero means each rule polls at half its own evaluation + // interval. Whatever this resolves to is written into the header as the + // cadence actually used, and that header value — never a re-derivation from + // the definitions — is what check derives maxGap from. PollEvery time.Duration Concurrency int @@ -90,7 +89,7 @@ type WatchConfig struct { // Notes is where the parent prints what an operator has to see before the // deploy step runs: resolve notes, the cadence per rule, the rules it will // not wait for. nil discards them. The library prints nothing else — the - // CLI owns presentation (§20.2). + // CLI owns presentation. Notes io.Writer } @@ -138,20 +137,20 @@ func (cfg WatchConfig) validate() error { return nil } -// Watch is the record step's parent process (§4.3). It returns only once the -// window is genuinely being recorded: +// Watch is the record step's parent process. It returns only once the window +// is genuinely being recorded: // // version gate -> resolve definitions and names -> derive timings -> // open the log and write the header -> ONE observation of every non-skipped -// rule -> verify §3.2 -> check the schedule budget -> detach the child -> -// wait for the child to report that it is recording -> write the pidfile -> -// return. +// rule -> verify normal instances are visible -> check the schedule budget -> +// detach the child -> wait for the child to report that it is recording -> +// write the pidfile -> return. // // The first-observation wait is not a convenience. Returning before it would // leave the deploy inside [from, first_poll] with no evidence — the exact -// blind interval the two-phase model exists to remove — and it is also what -// surfaces auth, name-resolution and parse failures BEFORE deploy.sh runs -// rather than ten minutes later. +// blind interval the record-then-check split exists to remove — and it is +// also what surfaces auth, name-resolution and parse failures BEFORE +// deploy.sh runs rather than ten minutes later. func Watch(ctx context.Context, cfg WatchConfig) error { cfg = cfg.withDefaults() if err := cfg.validate(); err != nil { @@ -165,8 +164,8 @@ func Watch(ctx context.Context, cfg WatchConfig) error { } // Hand the log over with Close, never Stop: a sentinel here would tell - // check the recording ended before the child had even started (§4.5). - // Closing also releases the flock the child is about to take. + // check the recording ended before the child had even started. Closing + // also releases the flock the child is about to take. if err := prep.writer.Close(); err != nil { return err } @@ -181,8 +180,8 @@ func Watch(ctx context.Context, cfg WatchConfig) error { // The PARENT writes the pidfile, not the child: check must find the pid the // instant Watch returns, and a child writing its own would race the very - // next step of the pipeline. A deviation from P6's argv list, and the - // reason the child is never given --pidfile at all. + // next step of the pipeline. That is why the child is never given + // --pidfile at all. // // It is written only once the child has reported ready, so no path through // this function leaves a pidfile naming a process that is not recording. @@ -276,8 +275,8 @@ type preparedWatch struct { // prepareWatch is everything the parent does before it detaches. It takes a // Source rather than building one so the paused-rule, first-observation, -// §3.2 and budget behaviours are all testable with a scripted fake — only the -// process spawning needs a real binary. +// instance-visibility and budget behaviours are all testable with a scripted +// fake — only the process spawning needs a real binary. func prepareWatch(ctx context.Context, cfg WatchConfig, src Source) (*preparedWatch, error) { version, err := src.Version(ctx) if err != nil { @@ -306,7 +305,7 @@ func prepareWatch(ctx context.Context, cfg WatchConfig, src Source) (*preparedWa for _, d := range resolved { // A cadence of zero would make the child spin: every rule is due the // instant it was marked. It also cannot be written into the header, - // where check requires a positive value to derive maxGap from (P5). + // where check requires a positive value to derive maxGap from. if rt[d.UID].pollEvery <= 0 { return nil, fmt.Errorf("rule %q (%s) reports intervalSeconds=%d: there is no poll cadence to record at", d.Title, d.UID, d.IntervalSeconds) @@ -344,18 +343,18 @@ func openRecording(ctx context.Context, cfg WatchConfig, src Source, writer *Wri return nil, err } - // A rule whose DEFINITION says is_paused is skipped (§12): it is not - // waited for, not scheduled and never polled. Waiting for one either hangs - // forever or errors before the deploy (§4.3), and recording polls for it - // would report an in-window pause (coverage check 7) for a rule that was - // already paused when the window opened — turning §12's exit 1 into an - // exit 2. The header still names it, with is_paused true, so check reports - // it as skipped from the definitions. + // A rule whose DEFINITION says is_paused is skipped: it is not waited for, + // not scheduled and never polled. Waiting for one either hangs forever or + // errors before the deploy, and recording polls for it would report an + // in-window pause (coverage check 7) for a rule that was already paused + // when the window opened — turning a skipped rule's exit 1 into an exit 2. + // The header still names it, with is_paused true, so check reports it as + // skipped from the definitions. var active []Definition activeTimings := make(map[string]ruleTimings, len(resolved)) for _, d := range resolved { if d.IsPaused { - fmt.Fprintf(cfg.Notes, "note: rule %q (%s) is paused: recorded as skipped, not waited for (§4.3)\n", d.Title, d.UID) + fmt.Fprintf(cfg.Notes, "note: rule %q (%s) is paused: recorded as skipped, not waited for\n", d.Title, d.UID) continue } active = append(active, d) @@ -373,9 +372,9 @@ func openRecording(ctx context.Context, cfg WatchConfig, src Source, writer *Wri } } - // Budget last, on the latencies just measured — never on a fixed estimate - // (§5.2). Only the active rules count: a skipped rule is never polled and - // consumes none of the capacity. + // Budget last, on the latencies just measured — never on a fixed estimate. + // Only the active rules count: a skipped rule is never polled and consumes + // none of the capacity. if err := CheckBudget(activeTimings, measured, cfg.Concurrency); err != nil { return nil, err } @@ -385,9 +384,9 @@ func openRecording(ctx context.Context, cfg WatchConfig, src Source, writer *Wri // loggedRules snapshots the resolved definitions into the header's rule list. // Every field but PollEverySeconds is forensic — a resolve-time snapshot that -// makes an uploaded log self-describing (§21.3) — while PollEverySeconds is +// makes an uploaded log self-describing — while PollEverySeconds is // load-bearing: it is the cadence this recording actually used, and check -// derives maxGap from it rather than from the definitions (P5). +// derives maxGap from it rather than from the definitions. func loggedRules(defs []Definition, rt map[string]ruleTimings) []LoggedRule { out := make([]LoggedRule, 0, len(defs)) for _, d := range defs { @@ -408,17 +407,17 @@ func loggedRules(defs []Definition, rt map[string]ruleTimings) []LoggedRule { } // firstObservations takes one observation of every rule in active, verifies -// §3.2 against those very responses, and reduces each into the poll record -// that IS the window's first heartbeat — plus the measured latency of each, -// which is the only honest input to §5.2's budget check (a fixed estimate is -// worthless when one rule's payload is ~230x another's). +// that normal instances are visible in those very responses, and reduces each +// into the poll record that IS the window's first heartbeat — plus the measured +// latency of each, which is the only honest input to the budget check (a fixed +// estimate is worthless when one rule's payload is ~230x another's). // -// Both entry paths share it: watch's parent, before it detaches (§4.3), and +// Both entry paths share it: watch's parent, before it detaches, and // single-step check's measurement pass, which keeps the polls as evidence -// rather than writing them to a log (P9). Keeping one implementation is the -// point — the §3.2 verification and the "absent is a warning, not an error" -// rule are exactly the places where two copies would silently drift, and a -// drift in either direction is fail-open. +// rather than writing them to a log. Keeping one implementation is the point — +// the instance-visibility verification and the "absent is a warning, not an +// error" rule are exactly the places where two copies would silently drift, and +// a drift in either direction is fail-open. // // polls come back in `active` order, so a log written from them is byte-stable // for a given set of observations. @@ -437,7 +436,7 @@ func firstObservations(ctx context.Context, src Source, active []Definition, red return nil, nil, err } - // Verify §3.2 before anything downstream relies on it: if the state + // Verify this before anything downstream relies on it: if the state // endpoint ever stops returning normal instances, the reduction's "keep // the non-normal ones" silently becomes "keep everything it happened to // send" and the transition markers lose their ground truth. @@ -454,12 +453,12 @@ func firstObservations(ctx context.Context, src Source, active []Definition, red measured[d.UID] = obs.Latency poll := reducer.Reduce(d.UID, obs) if !poll.Found { - // Authoritative, not transient (P2 already retried transport - // failures): the rule resolved in the ruler API but the state - // endpoint does not serve it. Recorded as Found=false, which P7 - // check 8 turns into unobservable — a note rather than an error - // here, because the state endpoint can lag a freshly created rule - // and the coverage proof fails closed either way. + // Authoritative, not transient (the transport already retried + // every transient failure): the rule resolved in the ruler API but + // the state endpoint does not serve it. Recorded as Found=false, + // which the coverage proof turns into unobservable — a note rather + // than an error here, because the state endpoint can lag a freshly + // created rule and the coverage proof fails closed either way. fmt.Fprintf(notes, "warning: rule %q (%s) is absent from the state endpoint; recorded as not found\n", d.Title, d.UID) } polls = append(polls, poll) @@ -469,8 +468,9 @@ func firstObservations(ctx context.Context, src Source, active []Definition, red // observeAll polls every rule in uids concurrently, bounded by concurrency, // and returns one Observation per rule that answered. Every rule is polled by -// TITLE (the ?rule_name= filter, §2.8) and selected out of the response by -// UID (§14.5) — a filtered response can carry several rules sharing one title. +// TITLE (the ?rule_name= filter is a title filter) and selected out of the +// response by UID — a filtered response can carry several rules sharing one +// title. // // It returns the successful observations alongside the first error in UID // order, so a caller that wants to keep the good heartbeats can, and the error @@ -518,9 +518,9 @@ func observeAll(ctx context.Context, src Source, titles map[string]string, uids // parent already wrote — one source of truth, no parent/child drift, and it // exercises ReadLog's header path — and the connection details come from the // inherited environment. Only the run facts the header does not carry travel -// in argv (§4.4). +// in argv. type DaemonChildConfig struct { - URL, Token string // from the inherited environment, never from argv (§20.2) + URL, Token string // from the inherited environment, never from argv Out string Until time.Time Concurrency int @@ -549,7 +549,7 @@ func RunDaemonChild(ctx context.Context, cfg DaemonChildConfig) error { } // Safe to read: the parent closed its writer before spawning this process, - // and no other writer can hold the log's flock (§4.4 step 4). + // and no other writer can hold the log's flock. header, polls, sentinel, err := ReadLog(cfg.Out) if err != nil { return err @@ -557,7 +557,7 @@ func RunDaemonChild(ctx context.Context, cfg DaemonChildConfig) error { if sentinel != nil { return fmt.Errorf("log %s already carries a stopped sentinel: another recorder finished it", cfg.Out) } - // The header's URL is the log's identity (§19.1 step 3). Checking it here + // The header's URL is the log's identity. Checking it here // catches a child that inherited an environment pointing somewhere else, // before it appends a single poll from the wrong Grafana. if header.URL != cfg.URL { @@ -577,7 +577,7 @@ func RunDaemonChild(ctx context.Context, cfg DaemonChildConfig) error { reducer := NewReducer() reducer.seedFrom(polls) - // SIGTERM is how check stops the recorder (§4.4 step 1); SIGINT is the + // SIGTERM is how check stops the recorder; SIGINT is the // same request from a human at a terminal. Both are clean stops, so both // end with a sentinel. Registered before the readiness report, so a signal // arriving the moment the parent unblocks is already handled. @@ -623,10 +623,10 @@ func reportReady(fd int) error { // childSchedule derives what the child polls, and how often, from the header // alone. The cadence comes from PollEverySeconds — the cadence the recording -// actually uses — and is never re-derived from the rule's evaluation interval: -// that is P5's "two authorities", and getting it wrong is fail-open in the -// faster-override direction. Paused rules are excluded here for the same -// reason the parent never polls them (§4.3, §12). +// actually uses — and is never re-derived from the rule's evaluation interval, +// which would be a second authority for the same value and is fail-open in the +// faster-override direction. Paused rules are excluded here for the same reason +// the parent never polls them. // // It returns cadences and nothing else. maxGap, healthGrace and evalStaleAfter // are coverage thresholds applied by the pure layer at classification time, so @@ -653,7 +653,7 @@ func childSchedule(h Header) (titles map[string]string, cadence map[string]time. // watchLoopConfig is the child's working state: what to poll, how often, and // where to append it. There is no threshold in here and no policy — the child -// records and classifies nothing (H5). +// records and classifies nothing. type watchLoopConfig struct { Src Source Writer *Writer @@ -671,7 +671,7 @@ type watchLoopConfig struct { // // The sentinel policy is the load-bearing part. A clean stop (a signal, or // Until) writes it; a hard error does NOT. A recorder that died must look -// exactly like a coverage gap to check, because it is one (§4.5) — writing a +// exactly like a coverage gap to check, because it is one — writing a // sentinel on the way out of a failure would hand check a "recording finished" // claim about a window that stopped being observed. func watchLoop(ctx context.Context, cfg watchLoopConfig) error { @@ -716,8 +716,8 @@ func watchLoop(ctx context.Context, cfg watchLoopConfig) error { pollErr := cfg.pollBatch(ctx, due) if ctx.Err() != nil { // Signalled while a poll was in flight. The aborted poll's error is - // not a recorder failure, and a clean stop wins over it (§4.4 step - // 1: finish the in-flight write, then the sentinel). + // not a recorder failure, and a clean stop wins over it: finish the + // in-flight write, then the sentinel. return cfg.Writer.Stop() } if pollErr != nil { @@ -761,7 +761,7 @@ func untilNextPoll(sched *Scheduler, until, now time.Time) (time.Duration, bool) return max(next.Sub(now), 0), true } -// writePidFile records the child's pid where check looks for it (P9's +// writePidFile records the child's pid where check looks for it (its // --pidfile, default .pid). The format is the decimal pid and a newline, // so `kill $(cat log.jsonl.pid)` works and ReadPidFile stays trivial. func writePidFile(path string, pid int) error { @@ -772,7 +772,7 @@ func writePidFile(path string, pid int) error { } // ReadPidFile is the other side of that contract: the pid of the recorder -// check must stop before it may read the log (§4.4 steps 1-4). +// check must stop before it may read the log. func ReadPidFile(path string) (int, error) { b, err := os.ReadFile(path) if err != nil { diff --git a/grafana-alertcheck/internal/gate/watch_daemon_test.go b/grafana-alertcheck/internal/gate/watch_daemon_test.go index db35c505b..c3a91cd12 100644 --- a/grafana-alertcheck/internal/gate/watch_daemon_test.go +++ b/grafana-alertcheck/internal/gate/watch_daemon_test.go @@ -21,7 +21,7 @@ import ( // os.Executable(), which under `go test` is this binary, so the one integration // test below exercises the real thing — a real fork/exec, a real setsid, a real // inherited environment, a real SIGTERM — with this function standing in for -// the CLI's `watch --daemon-child` dispatch, which lands in P10. +// the CLI's `watch --daemon-child` dispatch. func TestMain(m *testing.M) { if path := os.Getenv(lockHolderEnv); path != "" { os.Exit(runTestLockHolder(path)) @@ -66,8 +66,8 @@ func runTestLockHolder(path string) int { } // runTestDaemonChild parses the child argv childArgs() writes, and reads the -// connection details from the environment — never from argv (§20.2). P10's -// `watch` FlagSet does the same four flags. +// connection details from the environment — never from argv. The CLI's `watch` +// FlagSet does the same four flags. func runTestDaemonChild(args []string) int { cfg := DaemonChildConfig{ URL: os.Getenv("GRAFANA_URL"), @@ -116,7 +116,7 @@ func runTestDaemonChild(args []string) int { } // testBearerToken is what every request to grafanaTestServer must carry. The -// child never receives it in argv (§20.2), so a request that arrives +// child never receives it in argv, so a request that arrives // authenticated is proof that the token reached the detached process through // the inherited environment — and a 401 is what a test sees if that ever // breaks. @@ -147,7 +147,7 @@ func grafanaTestServer(t *testing.T) *httptest.Server { _, _ = w.Write(ruler) case strings.HasPrefix(r.URL.Path, "/api/prometheus/"): if r.URL.Query().Get("rule_name") == "" { - // §2.8: the gate must never read the state endpoint unfiltered. + // The gate must never read the state endpoint unfiltered. http.Error(w, "unfiltered state read", http.StatusBadRequest) return } @@ -212,14 +212,14 @@ func waitFor(t *testing.T, what string, timeout time.Duration, cond func() bool) t.Fatalf("timed out after %s waiting for %s", timeout, what) } -// TestWatchSpawnsADetachedRecorder is P6's one integration test: everything -// from the version gate to the sentinel, through a real detached process. +// The one watch integration test: everything from the version gate to the +// sentinel, through a real detached process. // // It asserts the four things only a real spawn can show — the pidfile points // at a live process, that process is in its own session (setsid, not a bare // `&`), it keeps appending after Watch returned, and SIGTERM makes it finish -// the log in the §4.4 order — and it uses a 200ms --poll-interval to do it in -// about a second, which also exercises the unclamped-override path (§5.1). +// the log in the stop order — and it uses a 200ms --poll-interval to do it in +// about a second, which also exercises the unclamped-override path. func TestWatchSpawnsADetachedRecorder(t *testing.T) { srv := grafanaTestServer(t) t.Setenv("GRAFANA_URL", srv.URL) @@ -263,7 +263,7 @@ func TestWatchSpawnsADetachedRecorder(t *testing.T) { t.Errorf("recorder pgid = %d, want %d: it did not get its own session", pgid, pid) } - // The parent already wrote the first heartbeat before it returned (§4.3); + // The parent already wrote the first heartbeat before it returned; // these later ones prove the detached child is the one appending now. waitFor(t, "the detached recorder to append its own polls", 10*time.Second, func() bool { _, polls, _, err := ReadLog(out) @@ -294,7 +294,7 @@ func TestWatchSpawnsADetachedRecorder(t *testing.T) { t.Fatalf("poll %d = %+v, want a found observation of %s", i, p, watchActiveUID) } if p.GrafanaNow.IsZero() { - t.Fatalf("poll %d has no grafana_now; H4 needs the Date header of its own response", i) + t.Fatalf("poll %d has no grafana_now; every poll needs the Date header of its own response", i) } } if sentinel.Before(header.StartedAt) { diff --git a/grafana-alertcheck/internal/gate/watch_process.go b/grafana-alertcheck/internal/gate/watch_process.go index 629606188..169a4f65a 100644 --- a/grafana-alertcheck/internal/gate/watch_process.go +++ b/grafana-alertcheck/internal/gate/watch_process.go @@ -23,7 +23,7 @@ type detachedChild struct { logOffset int64 } -// spawnChild re-execs this binary as the detached recorder (§4.4). A trailing +// spawnChild re-execs this binary as the detached recorder. A trailing // `&` is NOT sufficient: the child would keep the parent's session and process // group, so it would still take the terminal's signals and, on a runner, die // with the step that started it. Setsid gives it a new session AND a new @@ -52,7 +52,7 @@ func spawnChild(cfg WatchConfig) (detachedChild, error) { logOffset = info.Size() } - // The readiness pipe (§ReadyFDFlag): the child gets the write end as + // The readiness pipe: the child gets the write end as // descriptor 3 and reports on it once it holds the log and is polling. readyRead, readyWrite, err := os.Pipe() if err != nil { @@ -64,9 +64,9 @@ func spawnChild(cfg WatchConfig) (detachedChild, error) { cmd.Stdout = logFile cmd.Stderr = logFile cmd.ExtraFiles = []*os.File{readyWrite} // descriptor 3 in the child - // The environment is how the connection details reach the child (§20.2): - // the token must never appear in argv, where it would land in the process - // table and in CI logs. + // The environment is how the connection details reach the child: the token + // must never appear in argv, where it would land in the process table and + // in CI logs. cmd.Env = os.Environ() cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true} diff --git a/grafana-alertcheck/internal/gate/watch_test.go b/grafana-alertcheck/internal/gate/watch_test.go index 85d9f5185..f0d4168ce 100644 --- a/grafana-alertcheck/internal/gate/watch_test.go +++ b/grafana-alertcheck/internal/gate/watch_test.go @@ -92,9 +92,9 @@ func countPolls(polls []Poll, uid string) int { return n } -// TestWatchLoopPollsEachRuleAtItsOwnCadence is §5's per-rule schedule seen -// from the recorder: a 10s rule beside a 300s one keeps its own 5s cadence -// instead of dragging the slack rule along with it or being slowed to its pace. +// The per-rule schedule seen from the recorder: a 10s rule beside a 300s one +// keeps its own 5s cadence instead of dragging the slack rule along with it or +// being slowed to its pace. func TestWatchLoopPollsEachRuleAtItsOwnCadence(t *testing.T) { const tightUID, slackUID = "tight", "slack" path := filepath.Join(t.TempDir(), "log.jsonl") @@ -148,9 +148,8 @@ func TestWatchLoopPollsEachRuleAtItsOwnCadence(t *testing.T) { } } -// TestWatchLoopHardErrorLeavesNoSentinel is §4.5's fail-closed rule from the -// recorder's side: a recorder that dies must look exactly like a coverage gap, -// so it must not sign off the log on its way out. +// Fail-closed from the recorder's side: a recorder that dies must look exactly +// like a coverage gap, so it must not sign off the log on its way out. func TestWatchLoopHardErrorLeavesNoSentinel(t *testing.T) { path := filepath.Join(t.TempDir(), "log.jsonl") clock := newVirtualClock(testNow) @@ -191,10 +190,9 @@ func TestWatchLoopHardErrorLeavesNoSentinel(t *testing.T) { } } -// TestWatchLoopSignalDuringPollIsACleanStop pins §4.4 step 1: SIGTERM arriving -// while a poll is in flight is a clean stop, so the aborted poll's error must -// not suppress the sentinel — otherwise every normal check run, which stops the -// recorder exactly this way, would end unobservable. +// SIGTERM arriving while a poll is in flight is a clean stop, so the aborted +// poll's error must not suppress the sentinel — otherwise every normal check +// run, which stops the recorder exactly this way, would end unobservable. func TestWatchLoopSignalDuringPollIsACleanStop(t *testing.T) { path := filepath.Join(t.TempDir(), "log.jsonl") clock := newVirtualClock(testNow) @@ -311,7 +309,7 @@ func TestWatchLoopPollBatchKeepsTheHeartbeatsItGot(t *testing.T) { } } -// TestReducerSeedFromKeepsMarkersAcrossTheHandoff is H2 at the one seam P6 +// The vanish-versus-clear distinction at the one seam the parent/child handoff // introduces. The parent observes a firing instance; the child starts with a // fresh Reducer and sees the instance gone. Seeded, that is a vanish — a // discontinuity. Unseeded, it is nothing at all, and the instance silently @@ -321,7 +319,7 @@ func TestReducerSeedFromKeepsMarkersAcrossTheHandoff(t *testing.T) { key := instanceKey(firing.Labels) parentPoll := Poll{RuleUID: "r1", Found: true, Abnormal: []Instance{firing}} // The child's first response: the instance is gone from the response - // entirely, which is a vanish and never a clear (§4.7). + // entirely, which is a vanish and never a clear. childObs := observation(testNow, testStateRule("r1", "Example", time.Minute, testNow)) t.Run("seeded", func(t *testing.T) { @@ -385,10 +383,9 @@ func liveObservation(grafanaNow time.Time) Observation { testInstance(StateNormal, "", "a"))) } -// TestPrepareWatchDoesNotWaitForPausedRules is §22.4's regression test: a rule -// paused in its definition is skipped, never waited for. Waiting for one either -// hangs forever or errors before the deploy — and the header must still name -// it, so check can report it as skipped rather than lose it. +// A rule paused in its definition is skipped, never waited for. Waiting for one +// either hangs forever or errors before the deploy — and the header must still +// name it, so check can report it as skipped rather than lose it. func TestPrepareWatchDoesNotWaitForPausedRules(t *testing.T) { var notes strings.Builder cfg := watchTestConfig(t, ¬es, "uid:"+watchActiveUID, "uid:"+watchPausedUID) @@ -423,16 +420,16 @@ func TestPrepareWatchDoesNotWaitForPausedRules(t *testing.T) { } // One poll, for the live rule only — and it is already in the log before - // prepareWatch returned, which is the whole point of §4.3. + // prepareWatch returned, which is the whole point of the record step. if len(polls) != 1 || polls[0].RuleUID != watchActiveUID { t.Fatalf("polls = %+v, want exactly one first observation of %s", polls, watchActiveUID) } if !polls[0].Found || !polls[0].GrafanaNow.Equal(testNow) { t.Errorf("first poll = %+v, want a found observation at %s", polls[0], testNow) } - // §22.3: "the poll record holds the state histogram. Assert that watch - // writes it" — through a real prepareWatch()/Reducer call, not just - // log_test.go's hand-built Writer/ReadLog round trip. + // The poll record holds the state histogram, asserted through a real + // prepareWatch()/Reducer call rather than log_test.go's hand-built + // Writer/ReadLog round trip. if want := map[string]int{"normal": 1}; !maps.Equal(polls[0].Histogram, want) { t.Errorf("Histogram = %v, want %v: watch must record the state histogram on every poll it writes", polls[0].Histogram, want) } @@ -441,9 +438,9 @@ func TestPrepareWatchDoesNotWaitForPausedRules(t *testing.T) { } } -// TestPrepareWatchHeaderRecordsTheOverriddenCadence is P5's "two authorities" -// from the writing side: whatever --poll-interval resolves to is what the -// header records, because that is the only value check may derive maxGap from. +// One authority for the cadence, from the writing side: whatever +// --poll-interval resolves to is what the header records, because that is the +// only value check may derive maxGap from. func TestPrepareWatchHeaderRecordsTheOverriddenCadence(t *testing.T) { var notes strings.Builder cfg := watchTestConfig(t, ¬es, "uid:"+watchActiveUID) @@ -467,8 +464,8 @@ func TestPrepareWatchHeaderRecordsTheOverriddenCadence(t *testing.T) { } } -// TestPrepareWatchFailsWhenTheScheduleDoesNotFit: the budget check runs on the -// latencies the parent just measured, before the deploy runs (§5.2). +// The budget check runs on the latencies the parent just measured, before the +// deploy runs. func TestPrepareWatchFailsWhenTheScheduleDoesNotFit(t *testing.T) { var notes strings.Builder cfg := watchTestConfig(t, ¬es, "uid:"+watchActiveUID) @@ -484,9 +481,9 @@ func TestPrepareWatchFailsWhenTheScheduleDoesNotFit(t *testing.T) { assertBudgetMessage(t, err.Error()) } -// TestPrepareWatchVerifiesNormalInstancesAreVisible is the §3.2 check at the -// one place it can still be cheap: the first observation. If the state endpoint -// stops returning normal instances, the reduction's predicate quietly inverts. +// Normal instances are verified visible at the one place it is still cheap: +// the first observation. If the state endpoint stops returning them, the +// reduction's predicate quietly inverts. func TestPrepareWatchVerifiesNormalInstancesAreVisible(t *testing.T) { var notes strings.Builder cfg := watchTestConfig(t, ¬es, "uid:"+watchActiveUID) @@ -499,8 +496,8 @@ func TestPrepareWatchVerifiesNormalInstancesAreVisible(t *testing.T) { if err == nil { t.Fatal("prepareWatch: no error when totals claim normal instances the response omitted") } - if !strings.Contains(err.Error(), "3.2") { - t.Errorf("error does not name §3.2: %v", err) + if !strings.Contains(err.Error(), "no longer returns normal instances") { + t.Errorf("error does not say the endpoint stopped returning normal instances: %v", err) } // The failure happens before any poll is appended, so the log holds a @@ -525,9 +522,9 @@ func TestPrepareWatchRejectsAnUnsupportedGrafana(t *testing.T) { } } -// TestPrepareWatchNotesAnAbsentRule: a rule that resolved in the ruler API but -// is absent from the state endpoint is recorded as Found=false — authoritative -// evidence P7 turns into unobservable — not silently dropped. +// A rule that resolved in the ruler API but is absent from the state endpoint +// is recorded as Found=false — authoritative evidence the coverage proof turns +// into unobservable — not silently dropped. func TestPrepareWatchNotesAnAbsentRule(t *testing.T) { var notes strings.Builder cfg := watchTestConfig(t, ¬es, "uid:"+watchActiveUID) @@ -601,8 +598,8 @@ func TestWatchConfigValidation(t *testing.T) { }) } -// TestChildScheduleUsesTheRecordedCadence is P5's fail-open direction, checked -// on the child's side: a log recorded at 5s on a 300s rule must schedule at 5s. +// The fail-open direction, checked on the child's side: a log recorded at 5s on +// a 300s rule must schedule at 5s. // Re-deriving from the interval would give 150s — and every real 250s hole in // that recording would pass. func TestChildScheduleUsesTheRecordedCadence(t *testing.T) { @@ -616,7 +613,7 @@ func TestChildScheduleUsesTheRecordedCadence(t *testing.T) { t.Fatalf("childSchedule: %v", err) } if _, ok := titles["paused"]; ok { - t.Error("the child scheduled a rule that was paused when the window opened (§4.3)") + t.Error("the child scheduled a rule that was paused when the window opened") } if got := cadence["fast"]; got != 5*time.Second { t.Errorf("pollEvery = %s, want 5s from the header, not %s from the interval", got, defaultPollEvery(300))