From 42044e92e2785b1ac096e372ef98c429cba96479 Mon Sep 17 00:00:00 2001 From: Raphael Maenle Date: Wed, 10 Jun 2026 19:08:37 +0200 Subject: [PATCH] ci(interop): one-command external-validation suite + CI lanes for the daemon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tools/run_interop.sh runs ALL nine validation steps with a PASS/FAIL summary: build, unit (464), daemon-unit (41), secsgem-py host vs server (31 checks), secs_conformance (47), gRPC+secsgem-py daemon bridge, spool persistence across restart, tshark HSMS dissector, secs4java8 (55 checks). Verified green end-to-end. The unit suite is partly self-referential (our parsers validate our builders); these external validators are the real oracle — now they run with one command instead of by hand. Two bugs found by running it: unbounded ninja at -O3 OOM-kills cc1plus in memory-constrained Docker VMs (build with -j 2) and bash-3.2 lacks negative array subscripts. CI: grpc deps added to the build job so secs_gemd + secs_gemd_tests build and RUN in CI (previously the daemon silently dropped out — now fails loudly if missing), plus a python-interop lane running py-host/conformance/daemon harnesses against localhost in one container (no docker-in-docker). Service hardening while in there: reject proto Values with no kind set at the RPC edge (previously silently became ASCII ""), TODO markers for list element formats and daemon graceful shutdown. New tests: unset-Value guard + a property test iterating ALL configured variables via gRPC asserting each keeps its declared SECS-II format (daemon tests 16 -> 41 assertions). Co-Authored-By: Claude Fable 5 --- .gitea/workflows/ci.yml | 84 ++++++++++++++++++++++ apps/equipment_service.hpp | 20 ++++++ apps/secs_gemd.cpp | 4 ++ docs/DAEMON_ROADMAP.md | 21 +++--- tests/test_daemon_service.cpp | 52 ++++++++++++++ tools/run_interop.sh | 130 ++++++++++++++++++++++++++++++++++ 6 files changed, 303 insertions(+), 8 deletions(-) create mode 100755 tools/run_interop.sh diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index bc6a462..53fb252 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -35,6 +35,10 @@ jobs: ninja-build \ libasio-dev \ libyaml-cpp-dev \ + libprotobuf-dev \ + protobuf-compiler \ + protobuf-compiler-grpc \ + libgrpc++-dev \ python3 \ python3-yaml @@ -47,6 +51,14 @@ jobs: - name: Unit tests (Release) run: build/secsgem_tests + # The daemon's gRPC service tests (in-process channel). The grpc deps + # above make secs_gemd_tests build; fail loudly if it didn't, so the + # daemon can never silently drop out of CI again. + - name: Daemon tests (Release) + run: | + test -x build/secs_gemd_tests || { echo "secs_gemd_tests not built"; exit 1; } + build/secs_gemd_tests + thread-sanitizer: runs-on: ubuntu-latest container: @@ -176,6 +188,78 @@ jobs: - name: secs4j cross-validation run: bash interop/secs4j_validate.sh + python-interop: + # secsgem-py (the Python reference implementation) judging our wire + # behaviour: its GemHostHandler drives our secs_server (31 checks), the + # C++ conformance host drives it too (47 checks), and the dual-face + # daemon harness (gRPC tool + secsgem-py host vs secs_gemd) proves the + # gRPC<->HSMS bridge. Localhost processes in one container — no + # docker-in-docker. + runs-on: ubuntu-latest + container: + image: ubuntu:24.04 + steps: + - name: Bootstrap (node + git for actions/checkout) + run: | + export DEBIAN_FRONTEND=noninteractive + apt-get update + apt-get install -y --no-install-recommends \ + git ca-certificates nodejs + + - uses: actions/checkout@v4 + + - name: Install toolchain + python + run: | + export DEBIAN_FRONTEND=noninteractive + apt-get install -y --no-install-recommends \ + build-essential cmake ninja-build \ + libasio-dev libyaml-cpp-dev \ + libprotobuf-dev protobuf-compiler protobuf-compiler-grpc libgrpc++-dev \ + python3 python3-yaml python3-venv python3-pip + + - name: Build + run: | + cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Release + cmake --build build + + - name: Python deps (secsgem-py + grpcio, same pins as interop image) + run: | + python3 -m venv /tmp/venv + /tmp/venv/bin/pip install --no-cache-dir \ + secsgem==0.3.0 grpcio==1.60.0 grpcio-tools==1.60.0 "setuptools<80" + + - name: secsgem-py host vs secs_server + run: | + build/secs_server --port 5001 & + SERVER=$! + sleep 1 + /tmp/venv/bin/python interop/host_vs_cpp_server.py --host 127.0.0.1 --port 5001 + rc=$? + kill $SERVER 2>/dev/null || true + exit $rc + + - name: secs_conformance vs secs_server + run: | + build/secs_server --port 5002 & + SERVER=$! + sleep 1 + build/secs_conformance --host 127.0.0.1 --port 5002 + rc=$? + kill $SERVER 2>/dev/null || true + exit $rc + + - name: daemon interop (gRPC tool + secsgem-py host vs secs_gemd) + run: | + build/secs_gemd --port 5003 --grpc 127.0.0.1:50051 & + GEMD=$! + sleep 1 + /tmp/venv/bin/python interop/daemon_interop.py \ + --grpc 127.0.0.1:50051 --hsms-host 127.0.0.1 --hsms-port 5003 \ + --proto-dir proto/secsgem/v1 + rc=$? + kill $GEMD 2>/dev/null || true + exit $rc + libfuzzer: runs-on: ubuntu-latest container: diff --git a/apps/equipment_service.hpp b/apps/equipment_service.hpp index 6557c5a..995955c 100644 --- a/apps/equipment_service.hpp +++ b/apps/equipment_service.hpp @@ -59,15 +59,25 @@ inline s2::Item to_item(const pb::Value& v, s2::Format want) { } } case pb::Value::kList: { + // TODO(daemon): list elements inherit the variable's scalar format; + // honouring per-element formats needs the declared Item's nested shape. s2::Item::List items; for (const auto& e : v.list().items()) items.push_back(to_item(e, want)); return s2::Item::list(std::move(items)); } default: + // Unreachable: callers reject KIND_NOT_SET before converting (see + // value_is_set). Kept as a safe fallback for future oneof additions. return s2::Item::ascii(""); } } +// Validate a client-supplied Value before conversion. An unset oneof would +// otherwise silently become ASCII "" — reject it at the API edge instead. +inline bool value_is_set(const pb::Value& v) { + return v.kind_case() != pb::Value::KIND_NOT_SET; +} + inline pb::ControlState::State to_proto_state(gem::ControlState s) { switch (s) { case gem::ControlState::EquipmentOffline: return pb::ControlState::EQUIPMENT_OFFLINE; @@ -101,6 +111,11 @@ class EquipmentService final : public pb::Equipment::Service { resp->set_message("no variable named '" + kv.first + "'"); return grpc::Status::OK; } + if (!value_is_set(kv.second)) { + resp->set_code(pb::Ack::PARAMETER_INVALID); + resp->set_message("value for '" + kv.first + "' has no kind set"); + return grpc::Status::OK; + } rt_.set_variable(it->second.vid, to_item(kv.second, it->second.format)); } resp->set_code(pb::Ack::ACCEPT); @@ -117,6 +132,11 @@ class EquipmentService final : public pb::Equipment::Service { resp->set_message("no variable named '" + kv.first + "'"); return grpc::Status::OK; } + if (!value_is_set(kv.second)) { + resp->set_code(pb::Ack::PARAMETER_INVALID); + resp->set_message("value for '" + kv.first + "' has no kind set"); + return grpc::Status::OK; + } rt_.set_variable(it->second.vid, to_item(kv.second, it->second.format)); } auto ev = events_.find(req->name()); diff --git a/apps/secs_gemd.cpp b/apps/secs_gemd.cpp index b3c4ccd..6b37c43 100644 --- a/apps/secs_gemd.cpp +++ b/apps/secs_gemd.cpp @@ -68,6 +68,10 @@ int main(int argc, char** argv) { } std::cout << "[gemd] gRPC on " << grpc_addr << "; HSMS equipment on :" << hsms_port << std::endl; + // TODO(daemon): graceful shutdown — handle SIGTERM/SIGINT by calling + // server->Shutdown() and rt->stop() so in-flight RPCs drain and the spool + // journal flushes, instead of dying mid-write when supervised (systemd/ + // docker stop). See DAEMON_ROADMAP Phase E. server->Wait(); return 0; } diff --git a/docs/DAEMON_ROADMAP.md b/docs/DAEMON_ROADMAP.md index 064f635..84ccbd5 100644 --- a/docs/DAEMON_ROADMAP.md +++ b/docs/DAEMON_ROADMAP.md @@ -92,11 +92,14 @@ debts tax every later phase, and the most valuable tests aren't automated. (WatchHealth foundation). ⬜ Remaining: roll the same 3-line pattern onto the other single-slot classes (comm-state, EPT, exceptions, substrates, modules, carriers, E84) as each phase needs them — mechanical now that the type exists. -2. ⬜ **CI the interop + conformance harnesses** (`tools/run_interop.sh` + lane: - host_vs_cpp_server, daemon_interop, secs4j, tshark, secs_conformance). The - unit suite is partly self-referential (our parsers validate our builders); - the external validators are the real oracle and currently run only by hand. - Highest leverage-per-effort item in the repo. +2. 🚧 **CI the interop + conformance harnesses.** `tools/run_interop.sh` ✅ — + one command runs ALL nine validation steps (build, unit, daemon-unit, + py-host 31 checks, conformance 47, daemon bridge, spool restart, tshark, + secs4j 55) with a PASS/FAIL summary; verified green end-to-end 2026-06-10. + CI ✅ added but UNVERIFIED until pushed: grpc deps + `secs_gemd_tests` in + the build job (fails loudly if the daemon silently drops out), and a new + `python-interop` lane (py-host + conformance + daemon harness against + localhost, no docker-in-docker). ⬜ Verify the lanes on the first push. 3. ✅ **Fix `CompleteCommand` proto comment** — it described the rejected blocking model; now states the HCACK-4 contract. 4. ⬜ **Table-driven handler conformance test** ((request, expected-reply-shape) @@ -113,9 +116,11 @@ debts tax every later phase, and the most valuable tests aren't automated. 7. ⬜ Move `apps/equipment_service.hpp` into the library tree (`include/secsgem/daemon/`) once Phase B grows it; add a TSan-built `run_async` + concurrent-RPC daemon test (today's daemon tests only poll()). -8. ⬜ Validate names are identifier-safe in `ConfigValidator` (the Python - client's kwargs API depends on it); generalize the format-compliance test - to iterate ALL configured variables (property-style, same cost). +8. 🚧 Validate names are identifier-safe in `ConfigValidator` (the Python + client's kwargs API depends on it) — ⬜. Generalized format-compliance + property test (iterates ALL configured variables via gRPC, asserts each + keeps its declared wire format) — ✅, plus an unset-`Value` guard at the + RPC edge (was silently writing ASCII ""). ### Phase A — finish the universal daemon surface (small, unblock vendors) 1. ⬜ `GetVariables` — needs the reverse `Item → proto Value` conversion diff --git a/tests/test_daemon_service.cpp b/tests/test_daemon_service.cpp index 7d6da4d..6c41ccc 100644 --- a/tests/test_daemon_service.cpp +++ b/tests/test_daemon_service.cpp @@ -73,6 +73,58 @@ TEST_CASE("Equipment gRPC service over an in-process channel") { CHECK(resp.code() == pb::Ack::PARAMETER_INVALID); } + SUBCASE("SetVariables rejects a Value with no kind set (silent-'' guard)") { + grpc::ClientContext ctx; + pb::VariableUpdate req; + pb::Ack resp; + (*req.mutable_values())["ChamberPressure"]; // map entry, kind never set + auto st = stub->SetVariables(&ctx, req, &resp); + CHECK(st.ok()); + CHECK(resp.code() == pb::Ack::PARAMETER_INVALID); + // The previous value must be untouched (no silent ASCII "" write). + rt.poll(); + CHECK(rt.model().dvids.value(101)->format() == s2::Format::F4); + } + + SUBCASE("property: every configured variable keeps its declared format") { + // Iterate ALL SVIDs+DVIDs from the live config: set each via gRPC with a + // type-appropriate plain value and assert the stored Item keeps the + // declared wire format. Catches any future variable whose format the + // conversion table doesn't handle — not just the two pinned above. + auto check_all = [&](const auto& entries) { + for (const auto& v : entries) { + const auto declared = v.value.format(); + grpc::ClientContext ctx; + pb::VariableUpdate req; + pb::Ack resp; + pb::Value val; + switch (declared) { + case s2::Format::ASCII: case s2::Format::JIS8: + val.set_text("x"); break; + case s2::Format::Boolean: + val.set_boolean(true); break; + case s2::Format::Binary: + val.set_binary("\x01"); break; + case s2::Format::F4: case s2::Format::F8: + val.set_real(1.5); break; + default: // all integer widths + val.set_integer(1); break; + } + (*req.mutable_values())[v.name] = val; + REQUIRE(stub->SetVariables(&ctx, req, &resp).ok()); + REQUIRE_MESSAGE(resp.code() == pb::Ack::ACCEPT, v.name); + rt.poll(); + auto stored = rt.model().vid_value(v.id); + REQUIRE_MESSAGE(stored.has_value(), v.name); + CHECK_MESSAGE(stored->format() == declared, + v.name, ": declared ", static_cast(declared), + " stored ", static_cast(stored->format())); + } + }; + check_all(rt.model().svids.all()); + check_all(rt.model().dvids.all()); + } + SUBCASE("FireEvent accepts a known event and rejects an unknown one") { { grpc::ClientContext ctx; diff --git a/tools/run_interop.sh b/tools/run_interop.sh new file mode 100755 index 0000000..bdae89b --- /dev/null +++ b/tools/run_interop.sh @@ -0,0 +1,130 @@ +#!/usr/bin/env bash +# Run EVERY external-validation harness in one shot, with a PASS/FAIL summary. +# +# The unit suite is partly self-referential (our parsers validate our +# builders); these harnesses are the real oracle — independent implementations +# (secsgem-py, secs4java8, Wireshark) judging our wire behaviour. This script +# exists so they run routinely, not "when someone remembers". +# +# tools/run_interop.sh # everything +# SKIP_SECS4J=1 tools/run_interop.sh # skip the (slow) Java image build +# +# Steps (each independently PASS/FAIL, all attempted, nonzero exit if any fail): +# build compose builder (cmake + ninja, includes secs_gemd) +# unit secsgem_tests (full C++ suite) +# daemon-unit secs_gemd_tests (in-process gRPC service tests) +# py-host secsgem-py active host vs C++ secs_server (31 checks) +# conformance secs_conformance C++ host vs C++ secs_server (47 checks) +# daemon gRPC tool + secsgem-py host vs secs_gemd (bridge proof) +# spool spool persistence across a server restart +# tshark Wireshark HSMS dissector on a live capture +# secs4j secs4java8 Java host vs C++ secs_server (55 checks) +# +# Requires: docker compose. Runs from any cwd. +set -u +cd "$(dirname "${BASH_SOURCE[0]}")/.." + +declare -a NAMES RESULTS +overall=0 + +note() { printf '\n\033[1m== %s ==\033[0m\n' "$*"; } +record() { # record + NAMES+=("$1"); RESULTS+=("$2") + if [ "$2" -ne 0 ]; then overall=1; fi +} + +compose() { docker compose "$@"; } + +stop_services() { compose stop server gemd server-spool >/dev/null 2>&1 || true; } +trap stop_services EXIT + +# ---- build ----------------------------------------------------------------- +# -j 2: unbounded ninja at -O3 OOM-kills cc1plus in memory-constrained Docker +# VMs (macOS Docker Desktop). Two jobs is reliably safe and still fast. +note "build (compose builder, -j 2)" +compose run --rm builder bash -lc \ + "cmake -S /app -B /app/build -G Ninja -DCMAKE_BUILD_TYPE=Release && \ + cmake --build /app/build -j 2" +build_rc=$? +record build $build_rc +if [ "$build_rc" -ne 0 ]; then + echo "build failed — aborting (everything below needs the binaries)" + exit 1 +fi + +# ---- unit suites ----------------------------------------------------------- +note "unit: secsgem_tests" +compose run --rm builder /app/build/secsgem_tests; record unit $? + +note "daemon-unit: secs_gemd_tests" +if compose run --rm builder test -x /app/build/secs_gemd_tests; then + compose run --rm builder /app/build/secs_gemd_tests; record daemon-unit $? +else + echo "secs_gemd_tests not built (grpc missing from toolchain?) — FAIL" + record daemon-unit 1 +fi + +# ---- secsgem-py host vs C++ server ------------------------------------------ +note "py-host: secsgem-py GemHostHandler vs secs_server" +compose up -d --no-deps server +sleep 2 +compose run --rm --no-deps interop python3 host_vs_cpp_server.py --host server +record py-host $? +compose stop server >/dev/null 2>&1 + +# ---- C++ conformance host vs C++ server ------------------------------------- +note "conformance: secs_conformance vs secs_server" +compose up -d --no-deps server +sleep 2 +compose run --rm builder /app/build/secs_conformance --host server --port 5000 +record conformance $? +compose stop server >/dev/null 2>&1 + +# ---- daemon: gRPC tool + secsgem-py host vs secs_gemd ------------------------ +note "daemon: gRPC + secsgem-py vs secs_gemd" +compose up -d --no-deps gemd +sleep 2 +compose run --rm --no-deps interop \ + python3 daemon_interop.py --grpc gemd:50051 --hsms-host gemd +record daemon $? +compose stop gemd >/dev/null 2>&1 + +# ---- spool persistence across restart --------------------------------------- +note "spool: persistence across server restart" +( + set -e + compose up -d --no-deps server-spool + sleep 2 + compose run --rm --no-deps interop \ + python3 spool_persistence_test.py --phase enqueue --host server-spool + compose restart server-spool + sleep 2 + compose run --rm --no-deps interop \ + python3 spool_persistence_test.py --phase drain --host server-spool +) +record spool $? +compose stop server-spool >/dev/null 2>&1 + +# ---- Wireshark dissector ------------------------------------------------------ +note "tshark: Wireshark HSMS dissector" +compose run --rm builder bash interop/tshark_validate.sh +record tshark $? + +# ---- secs4java8 --------------------------------------------------------------- +if [ "${SKIP_SECS4J:-0}" = "1" ]; then + echo "secs4j skipped (SKIP_SECS4J=1)" +else + note "secs4j: secs4java8 host vs secs_server" + bash interop/secs4j_validate.sh; record secs4j $? +fi + +# ---- summary ----------------------------------------------------------------- +printf '\n\033[1m== interop summary ==\033[0m\n' +for i in "${!NAMES[@]}"; do + if [ "${RESULTS[$i]}" -eq 0 ]; then + printf ' \033[32mPASS\033[0m %s\n' "${NAMES[$i]}" + else + printf ' \033[31mFAIL\033[0m %s (exit %s)\n' "${NAMES[$i]}" "${RESULTS[$i]}" + fi +done +exit $overall