From f28aa08bd130cb49119dc10aa9dcd3c799219e92 Mon Sep 17 00:00:00 2001 From: Max Burian Date: Tue, 18 Aug 2026 15:16:48 -0600 Subject: [PATCH] infra(NEGGIA-003): TSan + Helgrind CI lanes (spec, audit, 49/50 workflow lines) Two additive ubuntu-24.04 jobs: tsan (env-delivered flags to preserve the gtest -Wno-error accommodation; Release reproducing NEGGIA-001's local evidence; halt_on_error=1; CMakeCache instrumentation-proof step) and helgrind (apt-installed valgrind; Debug for symbols; --error-exitcode=1 on Test_XdsPluginConcurrent + Test_XdsPlugin, cwd = test dir for the fixture symlink). Existing 12-job matrix byte-untouched. First green Helgrind run on master closes NEGGIA-001's AC-5 deferral. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012mV1sTxDDLeGzV1gKVup1C --- .github/workflows/main.yml | 53 ++++++++++++++++++++++++++ docs/audits/NEGGIA-003.md | 77 ++++++++++++++++++++++++++++++++++++++ docs/specs/NEGGIA-003.md | 45 ++++++++++++++++++++++ 3 files changed, 175 insertions(+) create mode 100644 docs/audits/NEGGIA-003.md create mode 100644 docs/specs/NEGGIA-003.md diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index cbed82a..2972e3b 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -111,3 +111,56 @@ jobs: ${{github.workspace}}.build/src/dectris/neggia/plugin/dectris-neggia.so if-no-files-found: error retention-days: 90 + + # NEGGIA-003: concurrency gates. Flags go via env, NOT -DCMAKE_*_FLAGS — + # a -D would silently drop the gtest -Wno-error accommodation (audit inv 2). + tsan: + runs-on: ubuntu-24.04 + env: + CXXFLAGS: -fsanitize=thread -g -Wno-error=maybe-uninitialized + CFLAGS: -fsanitize=thread -g + TSAN_OPTIONS: halt_on_error=1 + steps: + - name: Checkout + uses: actions/checkout@v4 + with: + submodules: true + - name: Create Build Directory + run: cmake -E make_directory ${{github.workspace}}.build + - name: Configure CMake (Release — reproduces NEGGIA-001's TSan evidence) + run: cmake ${{github.workspace}} -DCMAKE_BUILD_TYPE=Release -DDEBUG_PARSING=OFF -DCMAKE_POLICY_VERSION_MINIMUM=3.5 + working-directory: ${{github.workspace}}.build + - name: Prove TSan instrumentation (fail if flags were lost) + run: grep -- '-fsanitize=thread' CMakeCache.txt + working-directory: ${{github.workspace}}.build + - name: Build + run: cmake --build . + working-directory: ${{github.workspace}}.build + - name: Test + run: ctest --output-on-failure + working-directory: ${{github.workspace}}.build + + helgrind: + runs-on: ubuntu-24.04 + env: + CXXFLAGS: -Wno-error=maybe-uninitialized + steps: + - name: Install valgrind + run: sudo apt-get update && sudo apt-get install -y valgrind + - name: Checkout + uses: actions/checkout@v4 + with: + submodules: true + - name: Create Build Directory + run: cmake -E make_directory ${{github.workspace}}.build + - name: Configure CMake (Debug for symbolized Helgrind reports) + run: cmake ${{github.workspace}} -DCMAKE_BUILD_TYPE=Debug -DDEBUG_PARSING=OFF -DCMAKE_POLICY_VERSION_MINIMUM=3.5 + working-directory: ${{github.workspace}}.build + - name: Build + run: cmake --build . + working-directory: ${{github.workspace}}.build + - name: Helgrind on concurrency tests (cwd = test dir for fixture symlink) + run: | + valgrind --tool=helgrind --error-exitcode=1 ./Test_XdsPluginConcurrent + valgrind --tool=helgrind --error-exitcode=1 ./Test_XdsPlugin + working-directory: ${{github.workspace}}.build/src/dectris/neggia/test diff --git a/docs/audits/NEGGIA-003.md b/docs/audits/NEGGIA-003.md new file mode 100644 index 0000000..829f03a --- /dev/null +++ b/docs/audits/NEGGIA-003.md @@ -0,0 +1,77 @@ +# Audit: NEGGIA-003 — TSan + Helgrind CI lanes + +## Spec Reference +../neggia/docs/specs/NEGGIA-003.md + +## Symbol / File Inventory +(from neggia-archaeologist, 2026-08-18) + +- `.github/workflows/main.yml` (113 lines): trigger `on: [push, pull_request]`; jobs `posix-cc` (2 build-types × 4 OS = 8) + `posix-gcc` (4 includes) = **12 jobs/event → 24 check runs on a PR** (push + pull_request). Checkout `actions/checkout@v4` with `submodules: true`. Configure: `cmake ${{github.workspace}} -DCMAKE_BUILD_TYPE=... -DDEBUG_PARSING=... -DCMAKE_POLICY_VERSION_MINIMUM=3.5` in sibling dir `${{github.workspace}}.build`. Job-level `env: CXXFLAGS: -Wno-error=maybe-uninitialized` (vendored-gtest accommodation on gcc-11+). Test: `ctest --output-on-failure`. Per-lane `.so` artifact upload. +- NEGGIA-001's actual TSan recipe (`docs/audits/NEGGIA-001.md:579-587`, confirmed against live `build-tsan/CMakeCache.txt`): **Release** + `-DCMAKE_CXX_FLAGS="-fsanitize=thread -g"` + `-DCMAKE_C_FLAGS="-fsanitize=thread -g"`. +- Test binaries (`test/CMakeLists.txt:68,93`): `/src/dectris/neggia/test/Test_XdsPlugin` and `.../Test_XdsPluginConcurrent`; tests resolve fixtures via a **relative** path through the build-dir symlink (`test/CMakeLists.txt:3-7`) → direct invocation must run with cwd = that test dir. `PATH_TO_XDS_PLUGIN` is an absolute compile definition → dlopen is cwd-independent. +- valgrind: **NOT preinstalled** on ubuntu-22.04/24.04 GitHub runners → `sudo apt-get install -y valgrind` required (spec OQ1 resolved). + +## Threading Model +No production threading changed — this ticket only *observes* the existing worker-pool concurrency. TSan instruments both C++ and C objects (bitshuffle.c/lz4.c compile into the same `.so` via `NEGGIA_COMPRESSION_ALGORITHMS` — **`CFLAGS`/`-DCMAKE_C_FLAGS` is load-bearing, not optional**). dlopen'ing a TSan-instrumented `.so` from a TSan-instrumented test binary is the supported configuration (proven by the local build-tsan run). + +## Call Graph +Workflow-level only: two new jobs consuming the existing CMake/ctest graph. No source symbol touched. + +## ABI Surface Impact +**NONE.** Zero source diff (spec invariant 1). ABI HALT does not trigger. + +## Invariants +1. Existing 12-job matrix byte-untouched (spec inv 2) +2. **Flag-delivery pitfall (the audit's key finding):** the gtest accommodation arrives via the `CXXFLAGS` *environment variable*, which CMake uses only to seed `CMAKE_CXX_FLAGS` when it is NOT given on the command line. Passing `-DCMAKE_CXX_FLAGS="-fsanitize=thread -g"` **silently drops `-Wno-error=maybe-uninitialized`** → gcc-13 build break in vendored gtest. The TSan job must deliver flags via env: `CXXFLAGS: -fsanitize=thread -g -Wno-error=maybe-uninitialized` + `CFLAGS: -fsanitize=thread -g`, with no `-DCMAKE_*_FLAGS` on the configure line (spec OQ2 resolved). +3. **TSan build type = Release** — resolves the ticket-text "Debug" vs recipe discrepancy deliberately: Release reproduces the only existing TSan evidence (NEGGIA-001's local run) and catches optimizer-dependent interleavings; `-g` in the flags keeps reports symbolized. Recorded here as the spec-level decision (amends spec inv 3's "Debug" wording). +4. TSan failure propagation: a process with TSan reports exits 66 by default → ctest fails → job fails; add `TSAN_OPTIONS: halt_on_error=1` (one env line) for immediate first-failure diagnostics. The log-grep AC remains as the reviewer check, not a workflow step (cap economy). +5. Helgrind lane: Debug build (CMake default Debug = `-g`) for symbolization; `valgrind --tool=helgrind --error-exitcode=1` on the two binaries, invoked with cwd = `/src/dectris/neggia/test` (relative-fixture trap, spec OQ3 resolved) +6. Wall-time ≤15 min holds for the current 5-frame fixture; **re-check when NEGGIA-004 merges** (whichever of 003/004 lands second inherits the combined signal — Large001 under Helgrind is the cost driver; NEGGIA-004's audit caps the tested frame range for exactly this reason) + +## Risks +- **Medium**: (i) flag-delivery pitfall (invariant 2) — a lane that accidentally configures without TSan flags would pass *vacuously green*; mitigation: the job echoes the effective `CMAKE_CXX_FLAGS` from CMakeCache into the log (one grep-able line, reviewer-verifiable); (ii) 40-46 projected lines vs 50 cap — tight; comment lines excluded from cap but kept minimal anyway; (iii) valgrind version on ubuntu-24.04 (3.22) vs glibc — no known issue for this workload. + +## Existing ctest Coverage on Surface +All 9 ctest cases run in the 12 existing lanes — but never under TSan or Helgrind. The two concurrency-relevant cases (`Test_XdsPluginConcurrent`, `Test_XdsPlugin`) have TSan evidence only from one local run on the 5-frame synthetic fixture; Helgrind evidence: none, ever (NEGGIA-001 AC-5 deferral — this ticket closes it; first green run URL goes into the closed ticket's Notes per spec inv 7). + +## Challenge Questions Answered +- valgrind preinstalled? No — apt install needed (OQ1). +- gtest TSan accommodation? No *new* one needed; the existing one is silently lost via `-DCMAKE_CXX_FLAGS` — env delivery mandatory (OQ2). +- Binary paths + invocation? Quoted above; cwd trap resolved (OQ3). +- Why not full-suite Helgrind? Cost; the 7 non-concurrent tests exercise no threads — Helgrind on them observes nothing the plain lanes don't. + +## Devil's Advocate +- **Strongest argument against:** a vacuously-green TSan lane is worse than no lane — if the flag plumbing regresses (say a future workflow edit moves flags to the configure line), the lane keeps passing while instrumenting nothing, and the framework's concurrency gate becomes theater precisely when NEGGIA-006/007 depend on it. +- **Resolution:** the lane self-proves instrumentation: one step greps `CMakeCache.txt` for `-fsanitize=thread` and fails if absent (2 lines, inside cap). With that, "green" implies "instrumented". +- **Second-order effects:** none on the `.so` shipped to users (lanes are additive, artifacts come from the existing jobs); CI minutes +~6-10/PR. +- **What would make this audit wrong:** if TSan's runtime intercepted dlopen'd-library races only with additional flags — contradicted by the local build-tsan evidence on this exact test; if GitHub runner valgrind couldn't handle gcc-13 Debug DWARF — valgrind 3.22 handles DWARF5; low. + +## Cap-Unit Projection (sum-counted) +Workflow additions ~44-48 lines (two jobs: TSan ~20 incl. cache-grep guard, Helgrind ~26 incl. apt install + 2 invocations + cwd handling); no removals; no headers. **Inside the 50 cap with little slack** — comments minimal; if the draft overflows, drop the Helgrind `Test_XdsPlugin` invocation (lowest-value line) before anything else. + +## Audit Verdict +- **READY_FOR_PATCH** +- Rationale: scope matches the spec (one workflow file; the contingent ≤5-line CMake plumb proved unnecessary — env delivery suffices); all 3 OQs resolved; both sharp edges (flag-delivery pitfall, vacuous-green) have concrete in-cap mitigations; the Release-vs-Debug discrepancy is resolved deliberately in favor of reproducing NEGGIA-001's evidence. + +## Minimal Patch Proposal + +Source-line count: **49 / 50** (additions: 49 non-comment/non-blank YAML, removals: 0; comment/blank tracked-excluded: 4; zero lines under `src/dectris/neggia/`; no headers; CI workflows cap-bound per CLAUDE.md Hard Rule 3). + +### Diff +Two additive jobs appended to `.github/workflows/main.yml` on `feature/NEGGIA-003-tsan-helgrind-lanes` (whole-diff visible in the PR; existing 12-job matrix byte-untouched — verifiable via `git diff master... -- .github/workflows/main.yml` showing pure append): +- **tsan** (ubuntu-24.04): flags via `env: CXXFLAGS/CFLAGS` (audit invariant 2 — `-D` would drop the gtest accommodation), Release + `-fsanitize=thread -g` (reproduces NEGGIA-001's evidence, audit invariant 3), `TSAN_OPTIONS: halt_on_error=1`, **instrumentation-proof step** (`grep -- '-fsanitize=thread' CMakeCache.txt` — Devil's-Advocate vacuous-green mitigation), full ctest. +- **helgrind** (ubuntu-24.04): `apt-get install valgrind` (not preinstalled — OQ1), Debug build for symbolization, `valgrind --tool=helgrind --error-exitcode=1` on `Test_XdsPluginConcurrent` + `Test_XdsPlugin` with cwd = the test dir (fixture-symlink trap — OQ3). + +### Per-hunk justification +1. tsan job — spec inv 3 (TSan contract) + inv 4 amendment (Release decision) + Devil's-Advocate instrumentation proof +2. helgrind job — spec inv 4 (Helgrind contract), inv 5 (≤15 min: two small binaries only) +3. No CMake change needed (spec's contingent ≤5-line plumb unused); CHANGELOG deferred to post-merge sweep + +### Verification plan +- Self-verifying: both jobs run on this PR — TSan lane green with the instrumentation-proof step passing; Helgrind lane exit 0 on both binaries within budget +- Matrix untouched: 24 pre-existing checks still green on the PR +- Post-merge: first green Helgrind run URL on master → append to `tickets/closed/NEGGIA-001-*.md` Notes §Closeout (spec inv 7, closes the AC-5 deferral) +- Required-checks/branch-protection update: human console action, record in ticket step 5 + +### Verdict +- **READY_FOR_HUMAN_APPLY** (merge = apply; the PR's own lanes are the verification) diff --git a/docs/specs/NEGGIA-003.md b/docs/specs/NEGGIA-003.md new file mode 100644 index 0000000..1efc3a8 --- /dev/null +++ b/docs/specs/NEGGIA-003.md @@ -0,0 +1,45 @@ +# Spec: NEGGIA-003 — TSan + Helgrind CI lanes + +## Goal +Make the framework's mandatory concurrency gates executable: add a ThreadSanitizer lane and a Helgrind lane to `.github/workflows/main.yml` (ubuntu-24.04), running on every PR and push. The dev machine (macOS arm64) cannot run valgrind, so without these lanes the threaded tickets NEGGIA-006/007 cannot pass their own verification matrix — and NEGGIA-001's deferred AC-5 finally gets evidence on merged master. + +## Affected Source +- `.github/workflows/main.yml` — two new jobs alongside the existing matrix +- (contingent, ≤5 lines) top-level `CMakeLists.txt` or cmake flags plumb IF the TSan flags cannot be passed purely via `-DCMAKE_CXX_FLAGS` — audit decides; default is no CMake change +- `CHANGELOG.md` — `[Unreleased]` → Added + +## Invariants +1. **ABI untouched / zero source diff:** no file under `src/dectris/neggia/` changes; `nm -D` 4-symbol parity trivially preserved — source of truth: `docs/abi-baseline.txt` +2. **Existing matrix untouched:** the `posix-cc` and `posix-gcc` jobs' YAML is byte-identical pre/post except (at most) nothing — new jobs are purely additive +3. **TSan lane contract:** Debug build with `-fsanitize=thread -g` (NEGGIA-001's local recipe); full `ctest --output-on-failure`; job fails on nonzero ctest exit; `grep -c 'WARNING: ThreadSanitizer'` over the job log = 0 +4. **Helgrind lane contract:** `valgrind --tool=helgrind --error-exitcode=1` on the concurrency-relevant test binaries only (`Test_XdsPluginConcurrent`, `Test_XdsPlugin`); exit 0 required; plugin + tests built Debug (`-g`) for symbolization +5. **Wall-time budget:** each new job ≤ 15 min on a standard ubuntu-24.04 runner +6. **Cap:** workflow diff ≤ 50 cap units (CI workflows are cap-bound; xds XDS-069 precedent) — projection ~40 +7. **Evidence closure:** first green Helgrind run URL on merged master is appended to `tickets/closed/NEGGIA-001-*.md` Notes §Closeout (closes the AC-5 deferral) + +## Acceptance Tests +1. **TSan lane green** — type: TSan + - Setup: this ticket's PR + - Action: CI runs the new TSan job + - Expected: ctest exit 0; zero `WARNING: ThreadSanitizer` lines in the log +2. **Helgrind lane green** — type: Helgrind + - Action: CI runs the new Helgrind job + - Expected: valgrind exit 0 on both listed tests; job wall-time ≤ 15 min +3. **Failure propagation** — type: ctest + - Action: rely on `--error-exitcode=1` + ctest nonzero-exit semantics (no canary commit required; the wiring is reviewable in the YAML) + - Expected: reviewer confirms both jobs' steps have no `continue-on-error` and no swallowed exit codes +4. **Matrix untouched** — type: ctest + - Action: `git diff master... -- .github/workflows/main.yml` inspected at review + - Expected: existing jobs' blocks unmodified; all 24 pre-existing checks still green on the PR + +## Out of Scope +- Any production source or test-code change (fixture upgrade is NEGGIA-004) +- macOS TSan lanes (dev machine covers ad-hoc local TSan; CI TSan is Linux) +- Full-suite Helgrind (too slow for per-PR; scoped to the two concurrency tests) +- Branch-protection/required-checks console settings (human action, recorded in step 5 of the ticket) +- Benchmark/perf lanes (NEGGIA-002 territory, and not CI-wired there either) + +## Open Questions +1. Is valgrind preinstalled on ubuntu-24.04 GitHub runners or does the job need `sudo apt-get install -y valgrind`? — audit answers (expected: needs install) +2. Does vendored googletest need a TSan-specific accommodation beyond the existing `CXXFLAGS: -Wno-error=maybe-uninitialized` (NEGGIA-001 commit `2293e9e` precedent)? — audit checks +3. Exact post-build paths of the two test binaries for the valgrind invocation (or invoke via `ctest -R ` with `--overwrite MemoryCheck…`? — simpler: direct binary invocation; audit quotes paths from test/CMakeLists.txt)