diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 4418872e..a74f461d 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -77,10 +77,19 @@ repos: # Go quality gate — mirrors the CI workflow and the go-production-engineer standard. - repo: local hooks: + # The exit code is read, and that is the whole fix (#829). The entry used + # to be `test -z "$(gofmt -l .)" || ...`, which discards gofmt's status: a + # gofmt that cannot run produces an empty substitution, `test -z ""` is + # true, and the hook reports **Passed** -- green, asserting it looked. The + # window is a fresh clone before `mise trust`, which is a new + # contributor's first commit. go vet and golangci-lint below never had it, + # because they propagate their own status. + # + # tools/ci's TestTheGofmtHookFailsWhenGofmtCannotRun fails without this. - id: gofmt name: gofmt language: system - entry: bash -c 'test -z "$(gofmt -l .)" || { echo "gofmt needed:"; gofmt -l .; exit 1; }' + entry: bash -c 'out=$(gofmt -l .) || { echo "gofmt could not run" >&2; exit 1; }; test -z "$out" || { echo "gofmt needed:"; echo "$out"; exit 1; }' pass_filenames: false files: \.go$ - id: go-vet diff --git a/tools/ci/balancer_sample_test.go b/tools/ci/balancer_sample_test.go new file mode 100644 index 00000000..e2fffbe1 --- /dev/null +++ b/tools/ci/balancer_sample_test.go @@ -0,0 +1,113 @@ +package ci + +import ( + "math" + "os" + "path/filepath" + "regexp" + "strconv" + "strings" + "testing" +) + +// maxFalseVerdict is the rate this repository is willing to call acceptable for +// a verdict drawn from a fixed sample of coin flips. +// +// 1e-6 is a chosen horizon rather than a measured one, and the choice is the +// usual one: small enough that nobody meets it over the life of the suite, +// large enough that the sample stays cheap. What matters more than the number +// is that it is written down, because the defect it replaces was a number +// nobody had computed. +const maxFalseVerdict = 1e-6 + +var ( + probeCall = regexp.MustCompile(`probe\s+"?\$?\{?([A-Za-z_][A-Za-z0-9_]*|[0-9]+)\}?"?\s*\)`) + probeConstant = regexp.MustCompile(`(?m)^DISTRIBUTION_PROBES=([0-9]+)`) + perMachineCase = regexp.MustCompile(`\*"\$vm_[ab]"\*`) +) + +// TestABalancerVerdictDrawsFromASampleLargeEnoughToMeanIt is cited by +// tools/conformance/outscale/balancer.sh. +// +// ## The defect (#559) +// +// The suite asked six connections whether a balancer reaches both machines. Six +// is a coin flipped six times: a balancer that works sends every connection to +// one side with probability 2 × 0.5⁶ = **3.1 %**, so the gate reddened about +// one run in thirty-two on a subject that was behaving. A gate that reds on a +// working system teaches re-running it, which this repository has written down +// as worse than having no gate at all — and this one sits on the dataplane +// suite, whose subject is the product's own differentiator. +// +// The same arithmetic runs the other way at the unlink site: six probes that +// happen to avoid a machine the unlink never reached is 0.5⁶ = 1.6 %, a false +// GREEN over a real defect. That site's comment already named it "#559's defect +// with a different number". +// +// ## What this asserts, and why it reads the script +// +// The subject *is* the sample size, so the control reads it — but it checks a +// property rather than a value: every `probe` call whose verdict names a +// specific machine must draw from the declared constant, and that constant must +// put the false-verdict rate under maxFalseVerdict. Raising or lowering the +// number stays free; going back to a literal, or to a number that cannot +// support the claim, does not. +// +// A liveness probe ("did every connection get an answer") is deliberately out +// of scope: it has no false negative more probes would remove, and demanding +// the big sample there would buy wall-clock and nothing else. +func TestABalancerVerdictDrawsFromASampleLargeEnoughToMeanIt(t *testing.T) { + path, err := filepath.Abs(filepath.Join("..", "conformance", "outscale", "balancer.sh")) + if err != nil { + t.Fatal(err) + } + body, err := os.ReadFile(path) // #nosec G304 -- a fixed path inside the repository + if err != nil { + t.Fatal(err) + } + script := string(body) + + declared := probeConstant.FindStringSubmatch(script) + if declared == nil { + t.Fatal("balancer.sh declares no DISTRIBUTION_PROBES: the sample a per-machine " + + "verdict is drawn from must be named once, with its arithmetic, not spelled at each site") + } + n, err := strconv.Atoi(declared[1]) + if err != nil { + t.Fatal(err) + } + // Both tails: every connection landing on one side, either side. + rate := 2 * math.Pow(0.5, float64(n)) + if rate > maxFalseVerdict { + t.Errorf("DISTRIBUTION_PROBES=%d puts a working balancer's false verdict at %.3g, "+ + "over the %.0e this repository accepts: a gate that reds on a healthy subject "+ + "teaches the reflex of re-running it", n, rate, maxFalseVerdict) + } + + // Every per-machine verdict draws from that constant. Walk the lines, and + // for each `probe ` look ahead at the case arms it feeds. + lines := strings.Split(script, "\n") + checked := 0 + for i, line := range lines { + call := probeCall.FindStringSubmatch(line) + if call == nil { + continue + } + window := strings.Join(lines[i:min(i+10, len(lines))], "\n") + if !perMachineCase.MatchString(window) { + continue // a liveness probe: out of scope, see above + } + checked++ + if call[1] != "DISTRIBUTION_PROBES" { + t.Errorf("line %d draws a per-machine verdict from %q instead of DISTRIBUTION_PROBES: "+ + "a sample written at the site is one that stops agreeing with the arithmetic "+ + "beside the constant", i+1, call[1]) + } + } + // A denominator, because a loop over nothing passes silently — and would + // have passed before this suite had any per-machine verdict at all. + if checked < 2 { + t.Fatalf("only %d per-machine verdict(s) found; this suite draws two, "+ + "the distribution and the unlink", checked) + } +} diff --git a/tools/ci/gofmt_hook_test.go b/tools/ci/gofmt_hook_test.go new file mode 100644 index 00000000..0d04a83b --- /dev/null +++ b/tools/ci/gofmt_hook_test.go @@ -0,0 +1,146 @@ +package ci + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// gofmtHookEntry reads the `entry:` line of the gofmt hook out of +// .pre-commit-config.yaml. +// +// Out of the file rather than written here, for the reason every gate in this +// repository that quotes a document reads it instead: a copy is a second +// source, and a test holding its own copy of the command proves the copy works. +func gofmtHookEntry(t *testing.T) string { + t.Helper() + path, err := filepath.Abs(filepath.Join("..", "..", ".pre-commit-config.yaml")) + if err != nil { + t.Fatal(err) + } + body, err := os.ReadFile(path) // #nosec G304 -- a fixed path inside the repository + if err != nil { + t.Fatal(err) + } + lines := strings.Split(string(body), "\n") + for i, line := range lines { + if strings.TrimSpace(line) != "- id: gofmt" { + continue + } + for _, next := range lines[i:] { + trimmed := strings.TrimSpace(next) + if after, found := strings.CutPrefix(trimmed, "entry: bash -c "); found { + return strings.Trim(after, "'") + } + } + } + t.Fatal("no gofmt hook with a bash entry in .pre-commit-config.yaml") + return "" +} + +// runHook executes the hook's own command with a PATH of this test's choosing, +// and answers its exit code. A directory holding only what the test put there +// is how "gofmt cannot run" is produced without touching the station. +func runHook(t *testing.T, entry, path, dir string) (int, string) { + t.Helper() + cmd := exec.Command("bash", "-c", entry) + cmd.Dir = dir + cmd.Env = []string{"PATH=" + path, "HOME=" + t.TempDir()} + out, err := cmd.CombinedOutput() + code := 0 + var exit *exec.ExitError + if err != nil { + if !asExitError(err, &exit) { + t.Fatalf("running the hook: %v\n%s", err, out) + } + code = exit.ExitCode() + } + return code, string(out) +} + +// TestTheGofmtHookFailsWhenGofmtCannotRun is cited by .pre-commit-config.yaml. +// +// ## The defect, as it was measured (#829) +// +// The entry was `test -z "$(gofmt -l .)" || { … exit 1; }`. `gofmt -l` writes +// the offending files to stdout and its own errors to stderr, and the verdict +// was `test -z` over the captured stdout alone — the exit code was never read. +// So any failure of gofmt produced an empty substitution, `test -z ""` was +// true, and the hook exited 0. +// +// Not `Skipped`, not red: **Passed, asserting it looked**. Its window is a +// fresh clone before `mise trust`, which is a new contributor's first commit, +// and it is the only gate in this file with that shape — `go vet` and +// `golangci-lint` propagate their own status, and both reddened at the same +// instant on the same cause, which is how the gofmt green was noticed at all. +// +// ## Why this test runs the command instead of reading it +// +// A test asserting that the entry *contains* `||` or `$?` would pass on any +// future rewrite that happens to keep the characters and lose the property. The +// property is behavioural — a hook that cannot measure must not report success +// — so it is measured, by running the hook's own line with a PATH that holds no +// gofmt. +// +// The accepting half is asserted in the same file, because a hook that always +// failed would satisfy the test above and refuse every commit, which this +// repository has written down as worse than no gate at all. +func TestTheGofmtHookFailsWhenGofmtCannotRun(t *testing.T) { + entry := gofmtHookEntry(t) + + // A PATH holding bash's own dependencies and no gofmt. Not an empty PATH: + // the entry needs `test` and `echo`, which are builtins, but a future entry + // may not be, and an empty PATH would then fail for a reason that is not + // the subject. + empty := t.TempDir() + code, out := runHook(t, entry, empty, t.TempDir()) + if code == 0 { + t.Fatalf("the hook reported success with no gofmt on PATH: it asserts it looked "+ + "when it could not look, which is the defect #829 names\n%s", out) + } + if !strings.Contains(out, "could not run") { + t.Errorf("the hook failed without saying it could not measure:\n%s", out) + } +} + +// TestTheGofmtHookPassesOnFormattedCodeAndFailsOnUnformatted is the accepting +// half, and the half that keeps the fix from being a refusal of everything. +// +// Both directions in one test on purpose: a guard satisfied only by its +// refusing case is one nobody can tell from a broken one. +func TestTheGofmtHookPassesOnFormattedCodeAndFailsOnUnformatted(t *testing.T) { + entry := gofmtHookEntry(t) + path := os.Getenv("PATH") + if _, err := exec.LookPath("gofmt"); err != nil { + t.Skip("gofmt is not on PATH, so the accepting half cannot be measured here") + } + + t.Run("formatted", func(t *testing.T) { + dir := t.TempDir() + write(t, dir, "a.go", "package p\n\nfunc a() {}\n") + if code, out := runHook(t, entry, path, dir); code != 0 { + t.Errorf("the hook refused correctly formatted Go (exit %d):\n%s", code, out) + } + }) + + t.Run("unformatted", func(t *testing.T) { + dir := t.TempDir() + write(t, dir, "a.go", "package p\nfunc a() {}\n") + code, out := runHook(t, entry, path, dir) + if code == 0 { + t.Errorf("the hook accepted Go that gofmt rewrites:\n%s", out) + } + if !strings.Contains(out, "a.go") { + t.Errorf("the refusal does not name the file to fix:\n%s", out) + } + }) +} + +func write(t *testing.T, dir, name, body string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o600); err != nil { + t.Fatal(err) + } +} diff --git a/tools/conformance/outscale/balancer.sh b/tools/conformance/outscale/balancer.sh index 0e928dd2..73c7fedc 100755 --- a/tools/conformance/outscale/balancer.sh +++ b/tools/conformance/outscale/balancer.sh @@ -203,14 +203,38 @@ probe() { printf '%s' "$out" } +# How many connections a verdict about DISTRIBUTION is drawn from (#559). +# +# Six was the number here, and six is a coin flipped six times. A balancer that +# works sends every connection to one side with probability 2 x 0.5^6 = 3.1 %, +# so this suite reddened about one run in thirty-two on a subject that was +# behaving — and a gate that reds on a working system teaches the reflex of +# re-running it, which this repository has written down as worse than having no +# gate at all. +# +# The same arithmetic runs the other way after an unlink: six probes that +# happen to avoid a machine the unlink never reached is 0.5^6 = 1.6 %, which is +# a false GREEN over a real defect. The comment at that site already named it +# "#559's defect with a different number"; it is the same number and it is +# fixed in the same place. +# +# 24 puts both at 1.2e-7 and 6.0e-8. The cost is about ten seconds on a leg +# that measures in minutes, which is the whole reason the cheap answer is the +# right one here rather than a cleverer statistic. +# +# What it still does NOT prove, and the suite must not be read as claiming: that +# the distribution is FAIR. Both machines being reached says the balancer has +# more than one backend, nothing about the ratio. +DISTRIBUTION_PROBES=24 + echo "- the balancer distributes to both machines" -hits="$(probe 6)" +hits="$(probe "$DISTRIBUTION_PROBES")" case "$hits" in *TIMEOUT*) fail "the balancer did not answer every probe: $hits" ;; esac -case "$hits" in *"$vm_a"*) ;; *) fail "no connection reached $vm_a: $hits" ;; esac -case "$hits" in *"$vm_b"*) ;; *) fail "no connection reached $vm_b: $hits" ;; esac -ok "6/6 answered, over both machines:$hits" +case "$hits" in *"$vm_a"*) ;; *) fail "no connection reached $vm_a over $DISTRIBUTION_PROBES probes: $hits" ;; esac +case "$hits" in *"$vm_b"*) ;; *) fail "no connection reached $vm_b over $DISTRIBUTION_PROBES probes: $hits" ;; esac +ok "$DISTRIBUTION_PROBES/$DISTRIBUTION_PROBES answered, over both machines:$hits" # The measurement #315 turned on. An address the runtime has to announce outside # the network answers for two minutes and goes dark; an address of the network's @@ -226,11 +250,16 @@ echo "- and it is still there a minute later" # observe nothing changing is to let time pass. Shortening it shortens the # claim. docs/limits.md carries the full curve. sleep 60 -hits="$(probe 6)" +# Six is enough HERE, and the difference is the point: this verdict is about +# liveness, not distribution. "Did every connection get an answer" has no +# false-negative that more probes would remove, so the count stays where the +# wall-clock is already a minute. +liveness=6 +hits="$(probe "$liveness")" case "$hits" in *TIMEOUT*) fail "the balancer went dark within a minute: $hits" ;; esac -ok "6/6 answered again:$hits" +ok "$liveness/$liveness answered again:$hits" # The replace-not-patch rule, measured rather than asserted in a unit test: a # machine the API has stopped listing must stop receiving connections. @@ -238,12 +267,14 @@ echo "- an unlinked machine stops receiving connections" osc UnlinkLoadBalancerBackendMachines --LoadBalancerName "$lb_name" --BackendVmIds "$vm_a" >/dev/null \ || fail "UnlinkLoadBalancerBackendMachines rejected" # waits on silence: the verdict below is that $vm_a NEVER answers again across -# six connections. Nothing announces an unlink reaching the runtime, and the -# only poll available — "six probes that avoided $vm_a" — is satisfied by luck -# one run in sixty-four with the unlink not applied at all. That is #559's -# defect with a different number, so this wait stays fixed. +# $DISTRIBUTION_PROBES connections. Nothing announces an unlink reaching the +# runtime, and the only poll available — "probes that avoided $vm_a" — is +# satisfied by luck when the unlink has not applied at all. At six probes that +# luck ran one run in sixty-four, which is #559 pointing the other way: a false +# GREEN over a real defect. The probe count carries the fix; this wait stays +# fixed, because a verdict drawn from an absence cannot poll for it. sleep 2 -hits="$(probe 6)" +hits="$(probe "$DISTRIBUTION_PROBES")" case "$hits" in *TIMEOUT*) fail "the balancer stopped answering after an unlink: $hits" ;; *"$vm_a"*) fail "$vm_a still receives connections after being unlinked: $hits" ;; diff --git a/tools/falsify/specs/a-hook-that-cannot-measure-fails.json b/tools/falsify/specs/a-hook-that-cannot-measure-fails.json new file mode 100644 index 00000000..e0e340a1 --- /dev/null +++ b/tools/falsify/specs/a-hook-that-cannot-measure-fails.json @@ -0,0 +1,20 @@ +{ + "package": "./tools/ci/", + "note": "The subject is a gate reporting success when it could not look. The gofmt pre-commit hook read `test -z \"$(gofmt -l .)\"` and never its exit code: `gofmt -l` writes offending files to stdout and its own errors to stderr, so any failure of gofmt produced an empty substitution, `test -z \"\"` was true, and the hook exited 0. Not skipped, not red — Passed, asserting it looked. Its window is a fresh clone before `mise trust`, which is a new contributor's first commit (#829).\n\nIt is the gate-level form of the defect this repository names most often: a control that cannot fail is a comment. It was found while running the measurement #739 asks for, which is the detail worth keeping — the harness broke first and the hook agreed with it, and `go vet` and `golangci-lint` reddening at the same instant on the same cause is the only reason the gofmt green stood out.\n\nThe first mutation restores the defect exactly. The second is the over-correction, and it is mutated because a hook that refuses everything passes the first test and refuses every commit, which teaches `--no-verify` and disarms every other hook at once — this repository has that written down as worse than no gate at all.\n\nBoth tests run the hook's own `entry:` line, read out of .pre-commit-config.yaml rather than copied into Go. A test asserting the entry *contains* `||` would pass on any rewrite that kept the characters and lost the property.", + "mutations": [ + { + "label": "the entry goes back to `test -z \"$(gofmt -l .)\"`, which discards gofmt's exit code. This is the defect verbatim: with no gofmt on PATH the substitution is empty, the test is true, and the hook reports success over code nobody formatted", + "file": ".pre-commit-config.yaml", + "find": " entry: bash -c 'out=$(gofmt -l .) || { echo \"gofmt could not run\" >&2; exit 1; }; test -z \"$out\" || { echo \"gofmt needed:\"; echo \"$out\"; exit 1; }'", + "replace": " entry: bash -c 'test -z \"$(gofmt -l .)\" || { echo \"gofmt needed:\"; gofmt -l .; exit 1; }'", + "test": "TestTheGofmtHookFailsWhenGofmtCannotRun" + }, + { + "label": "the hook refuses whatever it is given, which passes the refusing test and rejects every commit including correctly formatted Go. The shape that teaches `--no-verify`, and `--no-verify` disarms every other hook in this file at once", + "file": ".pre-commit-config.yaml", + "find": " entry: bash -c 'out=$(gofmt -l .) || { echo \"gofmt could not run\" >&2; exit 1; }; test -z \"$out\" || { echo \"gofmt needed:\"; echo \"$out\"; exit 1; }'", + "replace": " entry: bash -c 'out=$(gofmt -l .) || { echo \"gofmt could not run\" >&2; exit 1; }; echo \"gofmt needed:\"; echo \"$out\"; exit 1'", + "test": "TestTheGofmtHookPassesOnFormattedCodeAndFailsOnUnformatted" + } + ] +} diff --git a/tools/falsify/specs/a-verdict-drawn-from-six-coin-flips.json b/tools/falsify/specs/a-verdict-drawn-from-six-coin-flips.json new file mode 100644 index 00000000..ef16398c --- /dev/null +++ b/tools/falsify/specs/a-verdict-drawn-from-six-coin-flips.json @@ -0,0 +1,20 @@ +{ + "package": "./tools/ci/", + "note": "The subject is a gate that reds on a subject behaving correctly. The Outscale balancer suite asked SIX connections whether a balancer reaches both of its machines; six is a coin flipped six times, so a working balancer sends every connection to one side with probability 2 x 0.5^6 = 3.1 %. About one run in thirty-two, on the dataplane suite — the one whose subject is this project's own differentiator (#559).\n\nThe same arithmetic runs the other way at the unlink site: six probes that happen to avoid a machine the unlink never reached is 0.5^6 = 1.6 %, a false GREEN over a real defect. That site's own comment named it 'the same defect with a different number' and left the number.\n\nWhy it matters more than its size: a gate that reds on a healthy system teaches the reflex of re-running it, and this repository has that written down as worse than having no gate at all. The fix is 24 probes, which puts both tails at 1.2e-7 and 6.0e-8, for about ten seconds on a leg that measures in minutes.\n\nThe control reads the script, because the subject IS the sample size — but it checks a property rather than a value: every probe call whose verdict names a machine draws from the declared constant, and that constant supports the claim. Raising or lowering the number stays free; going back to a literal does not.", + "mutations": [ + { + "label": "the sample goes back to six, which is the defect verbatim: a balancer that works reds one run in thirty-two, and an unlink that never applied passes one run in sixty-four", + "file": "tools/conformance/outscale/balancer.sh", + "find": "DISTRIBUTION_PROBES=24", + "replace": "DISTRIBUTION_PROBES=6", + "test": "TestABalancerVerdictDrawsFromASampleLargeEnoughToMeanIt" + }, + { + "label": "one per-machine verdict is spelled at its site again instead of drawing from the constant, so the arithmetic written beside the constant stops describing what the suite actually asks. This is how the number drifted in the first place", + "file": "tools/conformance/outscale/balancer.sh", + "find": "case \"$hits\" in *\"$vm_a\"*) ;; *) fail \"no connection reached $vm_a over $DISTRIBUTION_PROBES probes: $hits\" ;; esac", + "replace": "hits=\"$(probe 6)\"; case \"$hits\" in *\"$vm_a\"*) ;; *) fail \"no connection reached $vm_a over $DISTRIBUTION_PROBES probes: $hits\" ;; esac", + "test": "TestABalancerVerdictDrawsFromASampleLargeEnoughToMeanIt" + } + ] +}