diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 54112f4a7..3be12e6c3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -85,6 +85,27 @@ env: # the cache itself -- jobs still read each other's entries. FASTCACHE_PREFETCH_GROUP: "${{ github.run_id }}-${{ github.job }}" CLANG_VERSION: "22" + # The Catch2 the clang-tidy-diff job analyses against. Not a version this + # workflow installs -- `apt-get install -y catch2` takes whatever + # ubuntu-24.04 ships -- but a record of what that has been measured to be, + # which the "Assert the Catch2 this job analyses against" step below reads + # back off the runner and fails on if it has moved. + # + # It is worth recording because it is load-bearing and invisible. Which + # Catch2 is on the include path decides whether + # readability-function-cognitive-complexity findings on a TEST_CASE body + # reach this job at all, so a workstation with a different one can run the + # same clang-tidy over the same diff with the same flags and exit 0 where + # this job exits 1 -- silently, reporting nothing rather than reporting less + # (morph#666). An unpinned package that decides a gate's outcome and moves + # without notice is the shape this repository keeps getting caught by. + # + # ubuntu-24.04's package is 3.4.0-1build1 (Launchpad, noble Release pocket), + # and the clang-tidy-report artifact of run 35581623269 prints + # `/usr/include/catch2/internal/catch_test_registry.hpp:121` with + # `INTERNAL_CATCH_TESTCASE2( INTERNAL_CATCH_UNIQUE_NAME( dummyFunction ) )` + # and no `, __VA_ARGS__` -- v3.4.0's text exactly, and not v3.5.3's. + CATCH2_VERSION: "3.4.0" # MORPH_BUILD_FORMS_QML needs Qt 6.5+; ubuntu-24.04 apt still ships 6.4.2. QT_VERSION: "6.8.1" @@ -2193,6 +2214,20 @@ jobs: - name: Self-test the clang-tidy suppression-scope checker run: bash scripts/test_check_tidy_suppression_scope.sh clang-tidy-${{ env.CLANG_VERSION }} + # The Catch2 on this runner's include path decides whether a + # readability-function-cognitive-complexity finding on a TEST_CASE body + # reaches this job, and `apt-get install -y catch2` above pins nothing. + # This step reads the version out of the headers the step above just + # installed and fails if it is not the one CATCH2_VERSION records, so a + # move in the runner image is a red job rather than a quiet change of + # what this gate measures (morph#666). Its self-test runs first, for the + # same reason the suppression-scope checker's does. + - name: Self-test the catch2-pin checker + run: bash scripts/test_check_catch2_pin.sh + + - name: Assert the Catch2 this job analyses against + run: bash scripts/check_catch2_pin.sh . --strict + # Catches morph#632's bug class: tests/.clang-tidy's thirteen # suppressions are argued as Catch2 and raw-syscall idiom, which is true # of test sources and says nothing about include/morph/** -- yet diff --git a/examples/bank/tests/.clang-tidy b/examples/bank/tests/.clang-tidy index 1bbb5028c..d7ad0d3f1 100644 --- a/examples/bank/tests/.clang-tidy +++ b/examples/bank/tests/.clang-tidy @@ -11,9 +11,12 @@ # logical operator -- would defeat the expression decomposition that makes a # failing assertion print its two operands. Catch2 says so itself; the macro # definition carries `/* NOLINT(bugprone-chained-comparison) */` on that very -# line in 3.15.3. The clang-tidy-diff job installs ubuntu-24.04's catch2, -# 3.4.0, which predates that comment -- which is why the finding reaches CI -# here and not on a workstation with a current Catch2. +# line in 3.15.3. CI pins catch2 3.4.0 -- ubuntu-24.04's package, which +# predates that comment -- which is why the finding reaches CI here and not on +# a workstation with a current Catch2. That number is checked rather than +# asserted: scripts/check_catch2_pin.sh reads the runner's installed version +# back off the include path and fails this line and the clang-tidy-diff job +# together if the package moves (morph#666). # # Directory-scoped, and no wider. clang-tidy resolves configuration by walking # up from the file it is analysing and offers no finer granularity than a diff --git a/examples/bookmarks/tests/.clang-tidy b/examples/bookmarks/tests/.clang-tidy index 1bbb5028c..d7ad0d3f1 100644 --- a/examples/bookmarks/tests/.clang-tidy +++ b/examples/bookmarks/tests/.clang-tidy @@ -11,9 +11,12 @@ # logical operator -- would defeat the expression decomposition that makes a # failing assertion print its two operands. Catch2 says so itself; the macro # definition carries `/* NOLINT(bugprone-chained-comparison) */` on that very -# line in 3.15.3. The clang-tidy-diff job installs ubuntu-24.04's catch2, -# 3.4.0, which predates that comment -- which is why the finding reaches CI -# here and not on a workstation with a current Catch2. +# line in 3.15.3. CI pins catch2 3.4.0 -- ubuntu-24.04's package, which +# predates that comment -- which is why the finding reaches CI here and not on +# a workstation with a current Catch2. That number is checked rather than +# asserted: scripts/check_catch2_pin.sh reads the runner's installed version +# back off the include path and fails this line and the clang-tidy-diff job +# together if the package moves (morph#666). # # Directory-scoped, and no wider. clang-tidy resolves configuration by walking # up from the file it is analysing and offers no finer granularity than a diff --git a/examples/common/testkit/.clang-tidy b/examples/common/testkit/.clang-tidy index 759365510..21f86b275 100644 --- a/examples/common/testkit/.clang-tidy +++ b/examples/common/testkit/.clang-tidy @@ -11,9 +11,12 @@ # logical operator -- would defeat the expression decomposition that makes a # failing assertion print its two operands. Catch2 says so itself; the macro # definition carries `/* NOLINT(bugprone-chained-comparison) */` on that very -# line in 3.15.3. The clang-tidy-diff job installs ubuntu-24.04's catch2, -# 3.4.0, which predates that comment -- which is why the finding reaches CI -# here and not on a workstation with a current Catch2. +# line in 3.15.3. CI pins catch2 3.4.0 -- ubuntu-24.04's package, which +# predates that comment -- which is why the finding reaches CI here and not on +# a workstation with a current Catch2. That number is checked rather than +# asserted: scripts/check_catch2_pin.sh reads the runner's installed version +# back off the include path and fails this line and the clang-tidy-diff job +# together if the package moves (morph#666). # # Directory-scoped, and no wider. clang-tidy resolves configuration by walking # up from the file it is analysing and offers no finer granularity than a diff --git a/examples/crm/tests/.clang-tidy b/examples/crm/tests/.clang-tidy index 1bbb5028c..d7ad0d3f1 100644 --- a/examples/crm/tests/.clang-tidy +++ b/examples/crm/tests/.clang-tidy @@ -11,9 +11,12 @@ # logical operator -- would defeat the expression decomposition that makes a # failing assertion print its two operands. Catch2 says so itself; the macro # definition carries `/* NOLINT(bugprone-chained-comparison) */` on that very -# line in 3.15.3. The clang-tidy-diff job installs ubuntu-24.04's catch2, -# 3.4.0, which predates that comment -- which is why the finding reaches CI -# here and not on a workstation with a current Catch2. +# line in 3.15.3. CI pins catch2 3.4.0 -- ubuntu-24.04's package, which +# predates that comment -- which is why the finding reaches CI here and not on +# a workstation with a current Catch2. That number is checked rather than +# asserted: scripts/check_catch2_pin.sh reads the runner's installed version +# back off the include path and fails this line and the clang-tidy-diff job +# together if the package moves (morph#666). # # Directory-scoped, and no wider. clang-tidy resolves configuration by walking # up from the file it is analysing and offers no finer granularity than a diff --git a/examples/kanban/tests/.clang-tidy b/examples/kanban/tests/.clang-tidy index 1bbb5028c..d7ad0d3f1 100644 --- a/examples/kanban/tests/.clang-tidy +++ b/examples/kanban/tests/.clang-tidy @@ -11,9 +11,12 @@ # logical operator -- would defeat the expression decomposition that makes a # failing assertion print its two operands. Catch2 says so itself; the macro # definition carries `/* NOLINT(bugprone-chained-comparison) */` on that very -# line in 3.15.3. The clang-tidy-diff job installs ubuntu-24.04's catch2, -# 3.4.0, which predates that comment -- which is why the finding reaches CI -# here and not on a workstation with a current Catch2. +# line in 3.15.3. CI pins catch2 3.4.0 -- ubuntu-24.04's package, which +# predates that comment -- which is why the finding reaches CI here and not on +# a workstation with a current Catch2. That number is checked rather than +# asserted: scripts/check_catch2_pin.sh reads the runner's installed version +# back off the include path and fails this line and the clang-tidy-diff job +# together if the package moves (morph#666). # # Directory-scoped, and no wider. clang-tidy resolves configuration by walking # up from the file it is analysing and offers no finer granularity than a diff --git a/examples/ledger/tests/.clang-tidy b/examples/ledger/tests/.clang-tidy index 1bbb5028c..d7ad0d3f1 100644 --- a/examples/ledger/tests/.clang-tidy +++ b/examples/ledger/tests/.clang-tidy @@ -11,9 +11,12 @@ # logical operator -- would defeat the expression decomposition that makes a # failing assertion print its two operands. Catch2 says so itself; the macro # definition carries `/* NOLINT(bugprone-chained-comparison) */` on that very -# line in 3.15.3. The clang-tidy-diff job installs ubuntu-24.04's catch2, -# 3.4.0, which predates that comment -- which is why the finding reaches CI -# here and not on a workstation with a current Catch2. +# line in 3.15.3. CI pins catch2 3.4.0 -- ubuntu-24.04's package, which +# predates that comment -- which is why the finding reaches CI here and not on +# a workstation with a current Catch2. That number is checked rather than +# asserted: scripts/check_catch2_pin.sh reads the runner's installed version +# back off the include path and fails this line and the clang-tidy-diff job +# together if the package moves (morph#666). # # Directory-scoped, and no wider. clang-tidy resolves configuration by walking # up from the file it is analysing and offers no finer granularity than a diff --git a/examples/lims/tests/.clang-tidy b/examples/lims/tests/.clang-tidy index 1bbb5028c..d7ad0d3f1 100644 --- a/examples/lims/tests/.clang-tidy +++ b/examples/lims/tests/.clang-tidy @@ -11,9 +11,12 @@ # logical operator -- would defeat the expression decomposition that makes a # failing assertion print its two operands. Catch2 says so itself; the macro # definition carries `/* NOLINT(bugprone-chained-comparison) */` on that very -# line in 3.15.3. The clang-tidy-diff job installs ubuntu-24.04's catch2, -# 3.4.0, which predates that comment -- which is why the finding reaches CI -# here and not on a workstation with a current Catch2. +# line in 3.15.3. CI pins catch2 3.4.0 -- ubuntu-24.04's package, which +# predates that comment -- which is why the finding reaches CI here and not on +# a workstation with a current Catch2. That number is checked rather than +# asserted: scripts/check_catch2_pin.sh reads the runner's installed version +# back off the include path and fails this line and the clang-tidy-diff job +# together if the package moves (morph#666). # # Directory-scoped, and no wider. clang-tidy resolves configuration by walking # up from the file it is analysing and offers no finer granularity than a diff --git a/examples/pastebin/tests/.clang-tidy b/examples/pastebin/tests/.clang-tidy index 1bbb5028c..d7ad0d3f1 100644 --- a/examples/pastebin/tests/.clang-tidy +++ b/examples/pastebin/tests/.clang-tidy @@ -11,9 +11,12 @@ # logical operator -- would defeat the expression decomposition that makes a # failing assertion print its two operands. Catch2 says so itself; the macro # definition carries `/* NOLINT(bugprone-chained-comparison) */` on that very -# line in 3.15.3. The clang-tidy-diff job installs ubuntu-24.04's catch2, -# 3.4.0, which predates that comment -- which is why the finding reaches CI -# here and not on a workstation with a current Catch2. +# line in 3.15.3. CI pins catch2 3.4.0 -- ubuntu-24.04's package, which +# predates that comment -- which is why the finding reaches CI here and not on +# a workstation with a current Catch2. That number is checked rather than +# asserted: scripts/check_catch2_pin.sh reads the runner's installed version +# back off the include path and fails this line and the clang-tidy-diff job +# together if the package moves (morph#666). # # Directory-scoped, and no wider. clang-tidy resolves configuration by walking # up from the file it is analysing and offers no finer granularity than a diff --git a/examples/polls/tests/.clang-tidy b/examples/polls/tests/.clang-tidy index 1bbb5028c..d7ad0d3f1 100644 --- a/examples/polls/tests/.clang-tidy +++ b/examples/polls/tests/.clang-tidy @@ -11,9 +11,12 @@ # logical operator -- would defeat the expression decomposition that makes a # failing assertion print its two operands. Catch2 says so itself; the macro # definition carries `/* NOLINT(bugprone-chained-comparison) */` on that very -# line in 3.15.3. The clang-tidy-diff job installs ubuntu-24.04's catch2, -# 3.4.0, which predates that comment -- which is why the finding reaches CI -# here and not on a workstation with a current Catch2. +# line in 3.15.3. CI pins catch2 3.4.0 -- ubuntu-24.04's package, which +# predates that comment -- which is why the finding reaches CI here and not on +# a workstation with a current Catch2. That number is checked rather than +# asserted: scripts/check_catch2_pin.sh reads the runner's installed version +# back off the include path and fails this line and the clang-tidy-diff job +# together if the package moves (morph#666). # # Directory-scoped, and no wider. clang-tidy resolves configuration by walking # up from the file it is analysing and offers no finer granularity than a diff --git a/scripts/check_catch2_pin.sh b/scripts/check_catch2_pin.sh new file mode 100755 index 000000000..b1a8197d4 --- /dev/null +++ b/scripts/check_catch2_pin.sh @@ -0,0 +1,218 @@ +#!/usr/bin/env bash +# Usage: bash scripts/check_catch2_pin.sh [REPO_ROOT] [--strict] +# +# Two halves, both about the same fact: which Catch2 the clang-tidy-diff job +# analyses against. +# +# A. Textual. Every line in the tree asserting `CI pins catch2 ` must name +# the version .github/workflows/ci.yml records in CATCH2_VERSION, and any +# other line naming a Catch2 version beside a CI reference is rejected as +# a phrasing this gate cannot check. +# B. Behavioural. The Catch2 headers actually installed here are read and +# compared against that same pin. Under `--strict` (how the CI job runs +# it) a mismatch, or no Catch2 at all, fails. Without it -- a workstation +# run -- a mismatch prints a divergence notice instead, because a +# workstation is not required to carry the runner's package, only to know +# that it does not. +# +# Why this gate exists (morph#666): a local `clang-tidy-diff` over the same +# diff, with the same clang-tidy version and the same job flags, can exit 0 on +# a diff the CI job fails -- silently, reporting nothing rather than reporting +# less. The variable is the Catch2 on the include path: +# `readability-function-cognitive-complexity` computes the *same* score under +# both, and what differs is whether `ClangTidyDiagnosticConsumer` classifies +# the finding as user code, which depends on which notes a given Catch2 +# release's `TEST_CASE` expansion produces and where it puts them. That is how +# morph#656's branch shipped a NOLINT reason asserting a neighbouring TEST_CASE +# "scores under the threshold" while it scored 87 against a threshold of 25: +# the local gate agreed with it. +# +# What this gate does and does not do, stated plainly, because the distinction +# is the whole point of the ticket: +# +# * It makes CI's own Catch2 a *decision* rather than an accident. `apt-get +# install -y catch2` is unpinned; if the runner image's package moves, the +# clang-tidy job's measurement changes with nothing anywhere saying so. +# Half B under `--strict` turns that silent move into a failed job. +# * It does not make a local clang-tidy-diff agree with CI's. Nothing short +# of the job not depending on the runner's Catch2 at all does that -- +# morph#666's first closing condition, filed separately as a follow-up +# because it needs a CMake change in a file this change does not own. +# What half B does on a workstation is report, every time it is run, that +# the two measurements are not the same one. +# +# Requires git and grep; compiles nothing. +set -euo pipefail + +repo_root="" +strict=0 +for arg in "$@"; do + case "$arg" in + --strict) strict=1 ;; + *) repo_root="$arg" ;; + esac +done +if [ -z "$repo_root" ]; then + repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +fi +readonly repo_root strict + +readonly ci_workflow=".github/workflows/ci.yml" +readonly self="scripts/check_catch2_pin.sh" +readonly self_test="scripts/test_check_catch2_pin.sh" + +# `catch2 3.4.0`, `Catch2-3.4.0`, `Catch2 v3.8.1`. Not `Catch2.git` (no +# separator), and not a bare `3.4.0` with no product name on the line. +readonly catch2_version_re='[Cc]atch2[ -]v?[0-9]+\.[0-9]+(\.[0-9]+)?' +readonly ci_ref_re='(\bCI\b|ci\.yml|CATCH2_VERSION)' +readonly canonical_re='CI pins catch2 [0-9]+\.[0-9]+\.[0-9]+' +readonly historical_marker='catch2-pin: historical' + +failures=0 +canonical_sites=0 + +note() { printf 'ok: %s\n' "$*"; } +fail() { printf 'error: %s\n' "$*" >&2; failures=$((failures + 1)); } + +cd "$repo_root" + +# -- The source of truth ----------------------------------------------------- +if [ ! -f "$ci_workflow" ]; then + printf 'error: %s not found under %s\n' "$ci_workflow" "$repo_root" >&2 + exit 1 +fi + +pinned="$(sed -nE 's/^[[:space:]]*CATCH2_VERSION:[[:space:]]*"?([0-9]+\.[0-9]+\.[0-9]+)"?[[:space:]]*$/\1/p' \ + "$ci_workflow" | head -n 1)" + +if [ -z "$pinned" ]; then + printf 'error: no `CATCH2_VERSION: ""` found in %s -- this gate reads\n' \ + "$ci_workflow" >&2 + printf ' its expected value from there and cannot check anything without it\n' >&2 + exit 1 +fi + +note "${ci_workflow} pins catch2 ${pinned}" + +# -- A. The textual half ----------------------------------------------------- +mapfile -t files < <(git ls-files \ + | grep -vFx "$ci_workflow" \ + | grep -vFx "$self" \ + | grep -vFx "$self_test" \ + | { grep -v '^$' || true; }) + +if [ "${#files[@]}" -eq 0 ]; then + printf 'error: git ls-files returned nothing under %s\n' "$repo_root" >&2 + exit 1 +fi + +while IFS= read -r hit; do + [ -n "$hit" ] || continue + location="${hit%%:*}" + rest="${hit#*:}" + lineno="${rest%%:*}" + text="${rest#*:}" + + case "$text" in + *"$historical_marker"*) + note "${location}:${lineno}: marked historical, not checked" + continue + ;; + esac + + if printf '%s' "$text" | grep -qE "$canonical_re"; then + while IFS= read -r stated; do + canonical_sites=$((canonical_sites + 1)) + if [ "$stated" = "$pinned" ]; then + note "${location}:${lineno}: states catch2 ${stated}" + else + fail "${location}:${lineno}: states 'CI pins catch2 ${stated}', but ${ci_workflow} pins catch2 ${pinned}: + ${text}" + fi + done < <(printf '%s' "$text" | grep -oE "$canonical_re" \ + | grep -oE '[0-9]+\.[0-9]+\.[0-9]+') + continue + fi + + fail "${location}:${lineno}: names a Catch2 version beside a CI reference in a + phrasing this gate cannot check. Write it as 'CI pins catch2 ${pinned}', or + append the marker '${historical_marker}' if it is a dated record rather + than a claim about the pin now: + ${text}" +done < <(grep -nHIE "$catch2_version_re" -- "${files[@]}" 2>/dev/null \ + | grep -E "$ci_ref_re" || true) + +if [ "$canonical_sites" -eq 0 ]; then + fail "no 'CI pins catch2 ' assertion found anywhere in the tree. Either + the documentation stopped saying which Catch2 the clang-tidy job analyses + against, or it was reworded out of the shape this gate reads -- both leave + the gate checking nothing while still exiting 0, so it fails instead." +fi + +# -- B. The behavioural half ------------------------------------------------- +# Read the version out of the headers that are actually on this machine's +# include path, the same ones clang-tidy would expand TEST_CASE from. +# +# MORPH_CATCH2_INCLUDE_DIR, when set, *replaces* the default search rather than +# preceding it: the self-test needs a run in which no Catch2 is found, and a +# fallback to /usr/include would make that case pass or fail depending on what +# the machine running the self-test happens to have installed. +installed="" +installed_dir="" +if [ -n "${MORPH_CATCH2_INCLUDE_DIR:-}" ]; then + search_prefixes="${MORPH_CATCH2_INCLUDE_DIR}" +else + search_prefixes="/usr/include /usr/local/include" +fi +for prefix in $search_prefixes; do + header="${prefix}/catch2/catch_version_macros.hpp" + [ -f "$header" ] || continue + major="$(sed -nE 's/^#define CATCH_VERSION_MAJOR ([0-9]+).*$/\1/p' "$header" | head -n 1)" + minor="$(sed -nE 's/^#define CATCH_VERSION_MINOR ([0-9]+).*$/\1/p' "$header" | head -n 1)" + patch="$(sed -nE 's/^#define CATCH_VERSION_PATCH ([0-9]+).*$/\1/p' "$header" | head -n 1)" + if [ -n "$major" ] && [ -n "$minor" ] && [ -n "$patch" ]; then + installed="${major}.${minor}.${patch}" + installed_dir="$prefix" + break + fi +done + +if [ -z "$installed" ]; then + if [ "$strict" -eq 1 ]; then + fail "no Catch2 headers found on this machine, but --strict says this run *is* + the clang-tidy job's own environment. The job installs catch2 from apt + before this step; if that stopped happening, the measurement below this + step is no longer the one the pin describes." + else + note "no Catch2 headers found here -- nothing to compare against the pin" + fi +elif [ "$installed" = "$pinned" ]; then + note "installed Catch2 ${installed} (${installed_dir}) matches the pin" +elif [ "$strict" -eq 1 ]; then + fail "installed Catch2 is ${installed} (${installed_dir}) but ${ci_workflow} pins + catch2 ${pinned}. The runner image's package moved. Every clang-tidy-diff + result from this job is now a measurement against ${installed}, and the + nine examples/*/tests/.clang-tidy comments that explain why a local run + disagrees with CI describe ${pinned}. Update CATCH2_VERSION and those + comments together, having checked that the suppressions they argue for + still apply to ${installed}." +else + printf '\n' >&2 + printf 'WARNING: this machine'"'"'s Catch2 is %s (%s); CI pins catch2 %s.\n' \ + "$installed" "$installed_dir" "$pinned" >&2 + printf ' A local clang-tidy-diff run is therefore NOT the measurement the\n' >&2 + printf ' clang-tidy-diff job makes. For checks whose evidence lives inside\n' >&2 + printf ' Catch2 macro expansions -- readability-function-cognitive-complexity\n' >&2 + printf ' on a TEST_CASE body is the known one -- it can exit 0 on a diff CI\n' >&2 + printf ' fails, reporting nothing rather than reporting less (morph#666).\n' >&2 + printf ' A green local run is not evidence for those checks. It is still\n' >&2 + printf ' evidence for every check whose finding lands on a line you wrote.\n' >&2 + printf '\n' >&2 +fi + +if [ "$failures" -ne 0 ]; then + printf '\n%s catch2-pin check(s) failed\n' "$failures" >&2 + exit 1 +fi + +note "all ${canonical_sites} catch2-pin assertion(s) agree with ${ci_workflow}" diff --git a/scripts/test_check_catch2_pin.sh b/scripts/test_check_catch2_pin.sh new file mode 100755 index 000000000..d1f35fefe --- /dev/null +++ b/scripts/test_check_catch2_pin.sh @@ -0,0 +1,226 @@ +#!/usr/bin/env bash +# Usage: bash scripts/test_check_catch2_pin.sh +# +# Self-test for scripts/check_catch2_pin.sh, the gate that keeps the Catch2 the +# clang-tidy-diff job analyses against a recorded, checked fact rather than +# whatever apt shipped that morning. +# +# A lint gate nobody tests reports green whether or not it still detects +# anything, and this one is doubly exposed: the tree it guards is correct +# today, and the runner's package is the pinned one today, so the gate passes +# today whether or not either half is looking at anything. Both halves are +# therefore driven from the wrong side as well as the right one -- every drift +# the gate claims to catch is reintroduced into a scratch copy of the tree, or +# into a synthetic include directory, and must be caught for the stated reason +# rather than merely with a nonzero exit. +# +# One mutation at a time: applied together, a single detection would mask +# every other. +# +# The checker enumerates the tree with `git ls-files`, so each case runs +# against a throwaway git repository holding a copy of this one's tracked +# files rather than against a directory argument. +set -euo pipefail + +readonly repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +readonly checker="scripts/check_catch2_pin.sh" + +failures=0 + +note() { printf 'ok: %s\n' "$*"; } +fail() { printf 'error: %s\n' "$*" >&2; failures=$((failures + 1)); } + +scratch="$(mktemp -d)" +trap 'rm -rf "$scratch"' EXIT + +readonly pristine="${scratch}/pristine" +mkdir -p "$pristine" +while IFS= read -r -d '' tracked; do + mkdir -p "${pristine}/$(dirname "$tracked")" + cp "${repo_root}/${tracked}" "${pristine}/${tracked}" +done < <(cd "$repo_root" && git ls-files -z) +git -C "$pristine" init -q +git -C "$pristine" add -A + +make_tree() { + local dest="$1" + rm -rf "$dest" + mkdir -p "$dest" + cp -R "${pristine}/." "$dest" +} + +# `sed -i` is not portable between GNU and BSD sed; edit through a temp file. +edit() { + local file="$1"; shift + sed "$@" "$file" > "${file}.new" + mv "${file}.new" "$file" +} + +# A synthetic Catch2 include tree carrying exactly the three version macros the +# checker reads. This is what lets the behavioural half be driven from a +# version the machine running the self-test does not have installed. +make_catch2() { + local dir="$1" version="$2" + local major="${version%%.*}" rest="${version#*.}" + local minor="${rest%%.*}" patch="${rest#*.}" + rm -rf "$dir" + mkdir -p "${dir}/catch2" + { + printf '#define CATCH_VERSION_MAJOR %s\n' "$major" + printf '#define CATCH_VERSION_MINOR %s\n' "$minor" + printf '#define CATCH_VERSION_PATCH %s\n' "$patch" + } > "${dir}/catch2/catch_version_macros.hpp" +} + +# An include directory with no Catch2 in it at all. +readonly empty_include="${scratch}/no-catch2" +mkdir -p "$empty_include" + +# `$expected` is a substring the resulting diagnostic must contain; without it +# a mutation that broke the tree in some unrelated way -- a mangled sed, a file +# the mutator emptied -- would count as a detection, and this self-test would +# report a gate that no longer detects anything as fully working. +expect_caught() { + local description="$1" mutator="$2" expected="$3" extra_args="${4:-}" catch2_dir="${5:-}" + local tree="${scratch}/case" output + make_tree "$tree" + if ! ( cd "$tree" && eval "$mutator" ); then + fail "mutator failed to apply: ${description}" + return + fi + if output="$( cd "$tree" && MORPH_CATCH2_INCLUDE_DIR="$catch2_dir" \ + bash "$checker" . $extra_args 2>&1 )"; then + fail "NOT caught: ${description} -- the gate passed a tree it should reject" + printf '%s\n' "$output" >&2 + return + fi + if printf '%s' "$output" | grep -qF "$expected"; then + note "caught: ${description}" + else + fail "caught for the WRONG reason: ${description} -- no diagnostic containing '${expected}':" + printf '%s\n' "$output" >&2 + fi +} + +# The mirror, for the false positives this gate must not manufacture. A rule +# that rejected every mention of a Catch2 version anywhere would "catch" every +# case above while being useless. +expect_accepted() { + local description="$1" mutator="$2" expected="${3:-}" extra_args="${4:-}" catch2_dir="${5:-}" + local tree="${scratch}/case" output + make_tree "$tree" + if ! ( cd "$tree" && eval "$mutator" ); then + fail "mutator failed to apply: ${description}" + return + fi + if ! output="$( cd "$tree" && MORPH_CATCH2_INCLUDE_DIR="$catch2_dir" \ + bash "$checker" . $extra_args 2>&1 )"; then + fail "FALSE POSITIVE: ${description} -- the gate rejected a tree it should accept:" + printf '%s\n' "$output" >&2 + return + fi + if [ -z "$expected" ] || printf '%s' "$output" | grep -qF "$expected"; then + note "accepted: ${description}" + else + fail "accepted but SILENT: ${description} -- no output containing '${expected}':" + printf '%s\n' "$output" >&2 + fi +} + +readonly pinned_catch2="${scratch}/catch2-3.4.0" +readonly moved_catch2="${scratch}/catch2-3.5.3" +make_catch2 "$pinned_catch2" 3.4.0 +make_catch2 "$moved_catch2" 3.5.3 + +# -- The unmodified tree must pass ------------------------------------------- +make_tree "${scratch}/clean" +if output="$( cd "${scratch}/clean" && MORPH_CATCH2_INCLUDE_DIR="$pinned_catch2" \ + bash "$checker" . --strict 2>&1 )"; then + note "the unmodified tree passes against the pinned Catch2" +else + fail "the unmodified tree was rejected by the gate:" + printf '%s\n' "$output" >&2 +fi + +# -- A. The textual half ----------------------------------------------------- +expect_caught "a doc asserting CI pins catch2 3.5.3 while ci.yml pins 3.4.0" \ + "printf '%s\n' 'The lint leg reproduces because CI pins catch2 3.5.3 there.' \ + >> docs/spec/testing_charter.md" \ + "states 'CI pins catch2 3.5.3', but .github/workflows/ci.yml pins catch2 3.4.0" + +# The direction morph#666 will actually take: the runner image moves, someone +# updates CATCH2_VERSION, and the nine .clang-tidy copies stay where they are. +expect_caught "ci.yml bumped to 3.5.3 while the nine copies still say 3.4.0" \ + "edit .github/workflows/ci.yml -e 's/^ CATCH2_VERSION: \"3.4.0\"/ CATCH2_VERSION: \"3.5.3\"/'" \ + "but .github/workflows/ci.yml pins catch2 3.5.3" + +# Rule B: the rewording that would defeat rule A alone. The nine copies said +# the same sentence nine times; a tenth site wording it differently is exactly +# how the number went unchecked in the first place. +expect_caught "a CI Catch2 claim in an unrecognised phrasing, even with the right version" \ + "printf '%s\n' 'The CI lint job installs catch2 3.4.0 from apt.' \ + >> docs/spec/testing_charter.md" \ + "phrasing this gate cannot check" + +expect_caught "a CI Catch2 claim in an unrecognised phrasing with the wrong version" \ + "printf '%s\n' 'Measured against Catch2 3.16.0 while CI has catch2-3.5.3.' \ + >> docs/spec/testing_charter.md" \ + "phrasing this gate cannot check" + +expect_caught "every canonical assertion removed from the tree" \ + "for f in \$(git grep -lF 'CI pins catch2' -- . ':!scripts/check_catch2_pin.sh' \ + ':!scripts/test_check_catch2_pin.sh'); do + edit \"\$f\" -e 's/CI pins catch2 [0-9.]*/the pinned Catch2/g' + done" \ + "no 'CI pins catch2 ' assertion found anywhere in the tree" + +expect_caught "ci.yml with no CATCH2_VERSION to read" \ + "edit .github/workflows/ci.yml -e 's/^ CATCH2_VERSION: \"3.4.0\"/ UNRELATED_CATCH2: \"3.4.0\"/'" \ + 'no `CATCH2_VERSION: ""` found' + +# A Catch2 version discussed without invoking CI in the same breath is not this +# gate's business; rejecting it would make the gate unsatisfiable for any +# document recording a local measurement or a FetchContent tag. +expect_accepted "a Catch2 version named with no CI reference on the line" \ + "printf '%s\n' 'Reproduced against Catch2 3.16.0 on this workstation.' \ + >> docs/spec/testing_charter.md" + +expect_accepted "a historical record carrying the documented marker" \ + "printf '%s\n' 'Before noble, CI had catch2 2.13.10 (catch2-pin: historical).' \ + >> docs/spec/testing_charter.md" + +# -- B. The behavioural half ------------------------------------------------- +# The whole reason this gate is not just another prose checker: the runner's +# package moving under an unpinned `apt-get install -y catch2` must fail the +# job rather than quietly change what it measures. +expect_caught "--strict against a runner whose Catch2 package has moved" \ + "true" \ + "installed Catch2 is 3.5.3" \ + "--strict" "$moved_catch2" + +expect_caught "--strict with no Catch2 installed at all" \ + "true" \ + "no Catch2 headers found on this machine" \ + "--strict" "$empty_include" + +# Without --strict -- a workstation run -- a divergence is reported rather than +# failed, because a workstation is not required to carry the runner's package. +# But it must be *reported*: silence here is the defect morph#666 is about. +expect_accepted "a workstation whose Catch2 differs is warned, not failed" \ + "true" \ + "A local clang-tidy-diff run is therefore NOT the measurement" \ + "" "$moved_catch2" + +# And a workstation that does match must not be warned, or the notice becomes +# noise that gets filtered out. +expect_accepted "a workstation whose Catch2 matches the pin is not warned" \ + "true" \ + "matches the pin" \ + "" "$pinned_catch2" + +if [ "$failures" -ne 0 ]; then + printf '\n%s self-test check(s) failed\n' "$failures" >&2 + exit 1 +fi + +note "all catch2-pin checker self-tests passed" diff --git a/tests/test_strand_race.cpp b/tests/test_strand_race.cpp index 1df6a3919..ace091859 100644 --- a/tests/test_strand_race.cpp +++ b/tests/test_strand_race.cpp @@ -1,8 +1,10 @@ // SPDX-License-Identifier: Apache-2.0 +#include #include #include #include +#include #include #include #include @@ -19,10 +21,26 @@ // both strands dispatched tasks for that key concurrently — breaking the // per-model serialisation guarantee. // -// The test hammers post() on a single key from many threads. Every task bumps a -// per-key in-flight counter on entry and drops it on exit; if two tasks for the -// same key ever run concurrently the counter exceeds 1 and the test fails. Very -// short tasks maximise the drain/re-arm interleaving that triggered the bug. +// This file holds two cases, and they cover different things. This first one is +// a *load* test: it hammers post() on a single key from many threads, and every +// task bumps a per-key in-flight counter on entry and drops it on exit, so if +// two tasks for the same key ever run concurrently the counter exceeds 1 and the +// test fails. +// +// What it does **not** cover is the drain-and-erase boundary the bug above lives +// on. The erase only fires when a drain finds the pending queue empty, and eight +// threads posting 3200 tasks back-to-back onto one key keep it non-empty almost +// throughout: the quiet moment the defect needs never arrives. Measured, not +// assumed -- with the pre-fix two-step drain restored in `scheduleNext` (flip +// `running` under `strand->mtx`, release it, then erase under `_mapMtx` in a +// separate critical section), this case passed 10/10 under ThreadSanitizer on +// x86-64 Linux / clang 22.1.8 (morph#668). Short tasks maximise *re-arm*; they do +// not produce a drain. Turning the thread or post counts up makes that worse, +// not better. +// +// The second case below produces the shape this one cannot, and is the one that +// fails against that mutant. Keep both: saturation and the drain boundary are +// different failure modes of the same invariant. TEST_CASE("StrandExecutor never runs two tasks for one key concurrently under contention", "[strand][race]") { constexpr int kThreads = 8; constexpr int kPostsPerThread = 400; @@ -115,6 +133,169 @@ TEST_CASE("StrandExecutor never runs two tasks for one key concurrently under co } } +// The drain-and-re-arm boundary (morph#668), which the case above never reaches. +// +// Shape, not volume. The defect needs a strand to reach *empty* while a post is +// arriving, so this case manufactures that rendezvous instead of hoping for it: +// +// 1. A pilot task is posted alone on the key. Its last act is to publish the +// round number, so the chaser threads learn the strand is about to drain. +// 2. `kChasers` threads spin on that publication and post the instant it +// flips -- that is, while the drain block following the pilot's body is +// deciding "keep running vs. erase". A per-thread stagger walks each post +// across the handful of instructions that decision spans, so the window is +// sampled at many offsets rather than one. +// 3. The round ends only once every one of its tasks has run, so the strand +// really does empty before the next pilot. The gap is the point of the +// test, and is exactly what sustained saturation destroys. +// +// Three detectors, because the defect and its symptom are not the same event: +// +// * `maxInFlight` -- the *symptom*, as in the case above: two tasks for one +// key running at the same wall-clock moment. +// * plain, non-atomic state touched by every task -- the *defect*. Two strands +// for one key leave those accesses unordered by any happens-before edge, +// which ThreadSanitizer reports whether or not the two tasks ever overlap in +// wall clock. Lost updates to the same state are visible without a sanitizer +// at all, which is why the cells are checked as well as raced on. +// * per-producer FIFO -- a strand orphaned mid-burst can run one producer's +// later task before its earlier one, and an ordinary build sees that too. +// +// The tasks deliberately do a little plain work rather than none: an empty task +// gives an overlap a window a few instructions wide, which is why the case above +// can be wrong about serialisation and still report `maxInFlight == 1`. +TEST_CASE("StrandExecutor keeps one strand per key when a post races the drain", "[strand][race]") { + constexpr int kChasers = 4; + constexpr int kBurst = 3; + constexpr int kRounds = 600; + constexpr int kIterations = 6; + constexpr int kCells = 24; + constexpr int kPerRound = 1 + (kChasers * kBurst); + + morph::exec::detail::ModelId const key{7}; + + for (int iter = 0; iter < kIterations; ++iter) { + // Same ordering rule as the case above: the pool outlives the strand. + morph::exec::ThreadPoolExecutor pool{4}; + + // Deliberately plain -- no atomic, no mutex. Under the invariant this + // file exists for, the strand *is* the synchronisation: every handoff + // between two tasks for one key passes through `_mapMtx`/`strand->mtx`, + // so each task's writes happen-before the next task's reads and these + // are data-race-free. A second strand for the same key breaks that + // chain, and then they are not. + std::vector cells(static_cast(kCells), 0); + std::vector lastSeq(static_cast(kChasers) + 1, -1); + long long executedPlain = 0; + + std::atomic inFlight{0}; + std::atomic maxInFlight{0}; + std::atomic outOfOrder{0}; + std::atomic completed{0}; + // Round number whose pilot task has finished its body. Release/acquire: + // the chasers must not start posting for round r before it is set. + std::atomic gate{0}; + + auto body = [&](int producer, int seq) { + int const cur = inFlight.fetch_add(1) + 1; + int prev = maxInFlight.load(); + while (cur > prev && !maxInFlight.compare_exchange_weak(prev, cur)) { + } + auto const slot = static_cast(producer); + if (seq <= lastSeq[slot]) { + outOfOrder.fetch_add(1); + } + lastSeq[slot] = seq; + for (auto& cell : cells) { + cell += 1; + } + ++executedPlain; + inFlight.fetch_sub(1); + completed.fetch_add(1, std::memory_order_release); + }; + + { + morph::exec::detail::StrandExecutor strand{pool}; + + std::vector chasers; + chasers.reserve(kChasers); + for (int chaser = 0; chaser < kChasers; ++chaser) { + chasers.emplace_back([&, chaser] { + int const producer = chaser + 1; + // Per-thread LCG, so the stagger below differs per thread + // and per round without pulling in or a shared + // engine that would itself synchronise the threads. + auto rng = (static_cast(chaser) * 2654435761U) + 1U; + for (int round = 0; round < kRounds; ++round) { + // Spin rather than yield: the window this case aims at + // is a few instructions wide, and a yield overshoots it + // by orders of magnitude. The periodic yield is only a + // starvation guard for hosts with fewer cores than this + // case has threads (a CI runner has four); it fires once + // per 4096 spins, so it costs the rendezvous nothing. + for (unsigned spins = 0; gate.load(std::memory_order_acquire) <= round; ++spins) { + if ((spins & 0xFFFU) == 0xFFFU) { + std::this_thread::yield(); + } + } + for (int post = 0; post < kBurst; ++post) { + rng = (rng * 1664525U) + 1013904223U; + int const stagger = static_cast((rng >> 16U) & 0x3FU); + for (int step = 0; step < stagger; ++step) { + // Busy work, folded back into `rng` so it cannot + // be optimised away, walking this post to a + // different offset inside the drain window. + rng = (rng * 1103515245U) + 12345U; + } + int const seq = (round * kBurst) + post; + strand.post(key, [&body, producer, seq] { body(producer, seq); }); + } + } + }); + } + + for (int round = 0; round < kRounds; ++round) { + strand.post(key, [&body, &gate, round] { + body(0, round); + // Published last: the chasers' posts have to arrive while + // the drain that follows this body is running, not before. + gate.store(round + 1, std::memory_order_release); + }); + // Wait out the round rather than pipelining it. This is the + // quiet moment -- the strand drains to empty here, which is the + // only state from which the erase can fire at all. + while (completed.load(std::memory_order_acquire) < (round + 1) * kPerRound) { + std::this_thread::yield(); + } + } + + for (auto& chaser : chasers) { + chaser.join(); + } + // Closing this scope runs `~StrandExecutor`, which blocks until + // `_inFlight == 0`; see the case above for why that is a complete + // drain and not a deadline. + } + + constexpr int kExpected = kRounds * kPerRound; + INFO("iteration " << iter << ": completed " << completed.load() << " of " << kExpected); + // `CHECK`, not `REQUIRE`, for everything but the last line: each of + // these answers a different question about the same run and stopping at + // the first one would hide the others (see the case above). + CHECK(completed.load() == kExpected); + // Lost updates to the plain state: the sanitizer-free reading of the + // same defect the TSan legs see as a data race on it. + CHECK(executedPlain == static_cast(kExpected)); + auto const [lowest, highest] = std::ranges::minmax_element(cells); + CHECK(*lowest == kExpected); + CHECK(*highest == kExpected); + // FIFO per key is part of the contract, and an orphaned strand breaks it + // without any two tasks having to overlap. + CHECK(outOfOrder.load() == 0); + REQUIRE(maxInFlight.load() == 1); + } +} + // Regression test for ThreadPoolExecutor(0): a zero-worker pool used to accept // tasks that could never run, hanging every post() forever. The constructor now // clamps the worker count to at least 1, so a pool built with 0 is still usable.