diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 4abd110f9..f63b31c93 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -126,11 +126,20 @@ jobs: go install golang.org/x/vuln/cmd/govulncheck@latest govulncheck -tags dev ./... - race-check: - name: Race Detection + # The shards, and then Race Detection over them. The required status check + # is the aggregate, so its name never changes and the ruleset never has to + # be edited when the shard count does. + race-shard: + name: Race Detection (shard ${{ matrix.shard }} of 4) runs-on: ubuntu-latest permissions: contents: read + strategy: + # Every shard runs: stopping the others on the first failure would + # leave the aggregate unable to tell a real gap from a cancelled job. + fail-fast: false + matrix: + shard: [1, 2, 3, 4] env: BASECAMP_NO_KEYRING: "1" steps: @@ -143,12 +152,67 @@ jobs: with: go-version-file: 'go.mod' - - name: Run tests with race detector - # 20 minutes, not Go's default 10 per package. internal/connector runs - # real sockets, real processes and the recovery harness; under -race it - # takes about five minutes on a fast box and roughly twice that on a - # runner. The default was not a hung test, it was the budget. - run: go test -tags dev -race -v -timeout 20m ./... + # 20 minutes, not Go's default 10 per package. internal/connector runs + # real sockets, real processes and the recovery harness; under -race it + # takes about five minutes on a fast box and roughly twice that on a + # runner. The default was not a hung test, it was the budget. + - name: Run this shard under the race detector + run: scripts/race-shard.sh "${{ matrix.shard }}" 4 "shard-records/shard-${{ matrix.shard }}" + + # Uploaded even when the shard failed: the aggregate has to be able to + # tell "this shard ran its tests and one of them failed" from "this + # shard never got as far as knowing what it was meant to run". + - name: Record what this shard was assigned + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + # The name is the directory the aggregate reads: download-artifact + # extracts each one to //, so this has to be the + # shard-N the union check looks for, not a name of its own. + name: shard-${{ matrix.shard }} + path: | + shard-records/shard-${{ matrix.shard }}/all.txt + shard-records/shard-${{ matrix.shard }}/shard.${{ matrix.shard }}.txt + retention-days: 3 + if-no-files-found: error + + race-check: + name: Race Detection + runs-on: ubuntu-latest + needs: [race-shard] + # always(), or a failed shard would skip this job — and a skipped required + # check is not a failed one. + if: always() + permissions: + contents: read + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Download what each shard recorded + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + pattern: shard-* + path: shard-records + + # Naming what came back before judging it: a check that runs over + # nothing is the failure this whole arrangement exists to prevent. + - name: Show the records + run: find shard-records -type f | sort + + - name: Every shard passed + env: + SHARDS: ${{ needs.race-shard.result }} + run: | + echo "shards: $SHARDS" + [ "$SHARDS" = success ] || { + echo "a race-detection shard did not pass" >&2 + exit 1 + } + + - name: The shards between them ran every test, once + run: scripts/race-shard-union.sh shard-records 4 integration: name: Integration Tests diff --git a/Makefile b/Makefile index 72da89f25..910221fd9 100644 --- a/Makefile +++ b/Makefile @@ -407,6 +407,13 @@ replace-check: fi @echo "Replace check passed (no local replace directives)" +# The race job is sharded, and the union check is what makes that safe: it +# refuses unless the shards between them ran every test, once. A check that +# cannot fail would be worse than no check, so it has its own tests. +.PHONY: check-race-shards +check-race-shards: + @scripts/race-shard-union-test.sh + # Verify every leaf command is accounted for in smoke tests .PHONY: check-smoke-coverage check-smoke-coverage: build @@ -429,7 +436,7 @@ check-eval-patterns: # Run all checks (local CI gate) .PHONY: check -check: fmt-check vet lint lint-actions test test-e2e test-sync-skills check-naming check-surface check-skill-drift test-skill-drift check-bare-groups check-lint-lockstep check-smoke-coverage check-eval-patterns provenance-check tidy-check +check: fmt-check vet lint lint-actions test test-e2e test-sync-skills check-naming check-surface check-skill-drift test-skill-drift check-bare-groups check-lint-lockstep check-smoke-coverage check-eval-patterns check-race-shards provenance-check tidy-check # Lint GitHub Actions workflows (requires actionlint + zizmor) .PHONY: lint-actions @@ -650,6 +657,7 @@ help: @echo " coverage Run tests with coverage and open in browser" @echo " record-cassettes Record happy-path cassettes (TOKEN+TARGET+ACCOUNT+PROJECT)" @echo " smoke Run pre-release smoke suite (BASECAMP_TOKEN=...)" + @echo " check-race-shards Test the race shards' union check" @echo " qa-report Show QA coverage report from smoke traces" @echo "" @echo "Performance:" diff --git a/scripts/race-shard-union-test.sh b/scripts/race-shard-union-test.sh new file mode 100755 index 000000000..d12f9fa4c --- /dev/null +++ b/scripts/race-shard-union-test.sh @@ -0,0 +1,94 @@ +#!/usr/bin/env bash +# race-shard-union-test.sh — the union check's own tests. +# +# The union check is the reason it is safe to shard this job at all, so it +# is worth knowing it fails. Each case below is a way the shards could +# silently stop covering the suite; all of them must be refused. +set -euo pipefail +cd "$(dirname "$0")/.." +union=scripts/race-shard-union.sh + +work=$(mktemp -d) +trap 'rm -rf "$work"' EXIT + +plant() { + rm -rf "${work:?}"/* + for i in 1 2 3 4; do + mkdir -p "$work/shard-$i" + printf 'pkg/a\tTestOne\npkg/a\tTestTwo\npkg/b\tTestThree\npkg/b\tTestFour\n' > "$work/shard-$i/all.txt" + done + printf 'pkg/a\tTestOne\n' > "$work/shard-1/shard.1.txt" + printf 'pkg/a\tTestTwo\n' > "$work/shard-2/shard.2.txt" + printf 'pkg/b\tTestThree\n' > "$work/shard-3/shard.3.txt" + printf 'pkg/b\tTestFour\n' > "$work/shard-4/shard.4.txt" +} + +refuses() { + local what="$1" + if "$union" "$work" 4 >/dev/null 2>&1; then + echo "FAIL: the union check accepted $what" >&2 + exit 1 + fi + echo "ok - refuses $what" +} + +plant +"$union" "$work" 4 >/dev/null || { echo "FAIL: the union check refused a complete, disjoint split" >&2; exit 1; } +echo "ok - accepts a complete, disjoint split" + +plant; : > "$work/shard-3/shard.3.txt" +refuses "a shard assigned nothing" + +plant; rm "$work/shard-2/shard.2.txt" +refuses "a shard that recorded no assignment" + +plant; printf 'pkg/a\tTestOne\n' >> "$work/shard-3/shard.3.txt" +refuses "the same test assigned to two shards" + +plant; printf 'pkg/b\tTestFive\n' >> "$work/shard-1/all.txt" +refuses "a test in the enumeration and in no shard" + +plant; printf 'pkg/b\tTestFive\n' >> "$work/shard-2/all.txt" +refuses "shards that enumerated different sets of tests" + +plant; printf 'pkg/z\tTestGhost\n' >> "$work/shard-4/shard.4.txt" +refuses "a test assigned that the repository does not have" + +plant; : > "$work/shard-1/all.txt" +refuses "a shard whose enumeration is empty" + +# The aggregate reads the shards through uploaded artifacts, so a report +# that never arrives looks exactly like a shard with nothing to say. It +# must not: an aggregator that cannot read a shard's report would have an +# opinion about it anyway. +plant; rm -rf "$work/shard-2" +refuses "a shard whose report never arrived" + +plant; mv "$work/shard-3" "$work/race-shard-3" +refuses "a report that arrived under a name the checker does not read" + +# The check must not depend on how seq reads a backwards range. macOS ships +# seq, but BSD seq treats "first larger than last" as counting down, and the +# overlap loop ends on exactly that range. With seq, this case compared the +# last shard against itself and refused every correct split on macOS. +plant +bsd=$work/bsdbin +mkdir -p "$bsd" +cat > "$bsd/seq" <<'SEQ' +#!/usr/bin/env bash +# BSD seq(1): "If first is larger than last the default incr is -1." +first=$1; last=$2 +if [ "$first" -gt "$last" ]; then + for ((n = first; n >= last; n--)); do echo "$n"; done +else + for ((n = first; n <= last; n++)); do echo "$n"; done +fi +SEQ +chmod +x "$bsd/seq" +PATH="$bsd:$PATH" "$union" "$work" 4 >/dev/null || { + echo "FAIL: the union check refused a complete, disjoint split where seq counts down over a backwards range, as BSD seq does on macOS" >&2 + exit 1 +} +echo "ok - accepts a complete, disjoint split under BSD seq semantics" + +echo "All union checks behaved." diff --git a/scripts/race-shard-union.sh b/scripts/race-shard-union.sh new file mode 100755 index 000000000..7cfc8cce3 --- /dev/null +++ b/scripts/race-shard-union.sh @@ -0,0 +1,59 @@ +#!/usr/bin/env bash +# race-shard-union.sh - the aggregate job's check that the shards between +# them ran every test, once. +# +# This is the point of sharding the race job rather than a nicety attached to +# it. A split that silently stops running some tests reports a fast green and +# measures nothing, which is the defect this repository has spent a lot of +# effort closing elsewhere. So every one of these is a failure, not a warning: +# +# - a shard that enumerated a different set of tests than its siblings, +# which means they did not all see the same repository +# - the same test assigned to two shards, which means the split is wrong +# even where it looks complete +# - a test in no shard at all, which is the one that matters +# +# race-shard-union.sh +set -euo pipefail + +dir="${1:?directory holding the shard records}" +total="${2:?number of shards}" + +fail() { echo "SHARD COVERAGE FAILED: $*" >&2; exit 1; } + +sorted() { LC_ALL=C sort -u "$1"; } + +# Compared as sorted files rather than hashed: cmp is POSIX and md5sum is +# not on macOS, and this check is in make check, which people run locally. +# +# The loops count in the shell rather than through seq for the same reason. +# macOS does ship seq, but BSD seq reads "first larger than last" as a +# request to count down, so `seq 5 4` prints "5 4" where GNU seq prints +# nothing. The inner loop below ends on exactly that range, so on macOS +# every correct run was refused, comparing the last shard against itself. +for ((i = 1; i <= total; i++)); do + [ -s "$dir/shard-$i/all.txt" ] || fail "shard $i recorded no enumeration; either it did not get far enough to have one, or its report did not reach here" + sorted "$dir/shard-$i/all.txt" > "$dir/all.$i.sorted" + if [ "$i" != 1 ] && ! cmp -s "$dir/all.1.sorted" "$dir/all.$i.sorted"; then + fail "shard $i enumerated a different set of tests than shard 1, so the shards did not all see the same repository" + fi +done + +for ((i = 1; i <= total; i++)); do + [ -s "$dir/shard-$i/shard.$i.txt" ] || fail "shard $i was assigned no tests" + for ((j = i + 1; j <= total; j++)); do + overlap=$(LC_ALL=C comm -12 <(sorted "$dir/shard-$i/shard.$i.txt") <(sorted "$dir/shard-$j/shard.$j.txt")) + [ -z "$overlap" ] || fail "shards $i and $j were both assigned $(echo "$overlap" | wc -l) test(s), e.g. $(echo "$overlap" | head -1)" + done +done + +cat "$dir"/shard-*/shard.*.txt | LC_ALL=C sort -u > "$dir/union.txt" +cp "$dir/all.1.sorted" "$dir/expected.txt" + +missing=$(LC_ALL=C comm -23 "$dir/expected.txt" "$dir/union.txt") +[ -z "$missing" ] || fail "$(echo "$missing" | wc -l) test(s) were in no shard and so did not run, e.g. $(echo "$missing" | head -1)" + +extra=$(LC_ALL=C comm -13 "$dir/expected.txt" "$dir/union.txt") +[ -z "$extra" ] || fail "$(echo "$extra" | wc -l) test(s) were assigned that the repository does not have, e.g. $(echo "$extra" | head -1)" + +echo "shard coverage OK: $(wc -l < "$dir/expected.txt") tests across $total shards, disjoint and complete" diff --git a/scripts/race-shard.sh b/scripts/race-shard.sh new file mode 100755 index 000000000..7181e5555 --- /dev/null +++ b/scripts/race-shard.sh @@ -0,0 +1,111 @@ +#!/usr/bin/env bash +# race-shard.sh - run one shard of the race-detector suite. +# +# Race detection was one job of nineteen minutes, and seventeen of them were +# one package: internal/connector is 1031s of a 1061s critical path. Sharding +# by package therefore buys nothing — whichever shard gets that package still +# takes seventeen minutes — so the unit here is the test, not the package. +# +# The hazard that comes with that is the one this repository keeps finding: +# a shard scheme that silently stops running some tests is green and measures +# nothing. Two checks answer it. This script refuses to report success unless +# every test it was assigned actually ran, and race-shard-union.sh, in the +# aggregate job, refuses unless the shards between them covered the lot. +# +# race-shard.sh +set -euo pipefail + +index="${1:?shard index, 1-based}" +total="${2:?number of shards}" +out="${3:?directory to write the shard record into}" +mkdir -p "$out" + +all="$out/all.txt" +plan="$out/shard.$index.txt" + +# -json, not a bare -list: a plain listing prints test names with no package, +# and six names in this repository exist in more than one. The union check +# downstream compares package-and-test pairs, so the attribution has to be +# unambiguous here. +go test -tags dev -list '.*' -json ./... | + python3 -c ' +import sys, json +for line in sys.stdin: + try: + event = json.loads(line) + except ValueError: + continue + if event.get("Action") != "output": + continue + name = event.get("Output", "").strip() + # Benchmarks are listed but -run does not run them without -bench, so + # assigning one would be assigning work no shard can do. The race job + # has never run them; it does not start now. + if name and " " not in name and name.startswith(("Test", "Fuzz", "Example")): + print(event["Package"] + "\t" + name) +' | LC_ALL=C sort -u > "$all" + +[ -s "$all" ] || { echo "enumerated no tests at all" >&2; exit 1; } +awk -v i="$index" -v n="$total" 'NR % n == i % n' "$all" > "$plan" +[ -s "$plan" ] || { echo "shard $index of $total was assigned no tests" >&2; exit 1; } + +echo "shard $index of $total: $(wc -l < "$plan") of $(wc -l < "$all") tests" + +# One invocation over ./... keeps go test's own package parallelism. A name +# that exists in two packages runs in both, which costs eight extra tests +# across the repository and keeps the command line to a single regex. +regex="^($(cut -f2 "$plan" | LC_ALL=C sort -u | paste -sd'|'))$" + +set +e +go test -tags dev -race -timeout 20m -json -run "$regex" ./... | tee "$out/events.$index.json" | + python3 -c ' +import sys, json +for line in sys.stdin: + try: + event = json.loads(line) + except ValueError: + sys.stdout.write(line) + continue + if event.get("Action") == "output": + sys.stdout.write(event.get("Output", "")) +' +status=${PIPESTATUS[0]} +set -e + +# A -run regex that matches nothing exits 0 with "no tests to run". That is +# exactly the shape of failure this scheme has to rule out, so what ran is +# compared against what was assigned. +python3 - "$plan" "$out/events.$index.json" <<'PY' +import json, sys + +plan, events = sys.argv[1], sys.argv[2] + +assigned = set() +with open(plan) as handle: + for line in handle: + package, _, name = line.rstrip("\n").partition("\t") + if name: + assigned.add((package, name)) + +ran = set() +with open(events) as handle: + for line in handle: + try: + event = json.loads(line) + except ValueError: + continue + name = event.get("Test", "") + if event.get("Action") == "run" and name and "/" not in name: + ran.add((event["Package"], name)) + +missing = assigned - ran +if missing: + print(f"SHARD RAN {len(assigned) - len(missing)} OF ITS {len(assigned)} TESTS", file=sys.stderr) + for package, name in sorted(missing)[:20]: + print(f" did not run: {package}\t{name}", file=sys.stderr) + sys.exit(1) + +print(f"shard ran all {len(assigned)} tests it was assigned") +PY + +exit "$status"