From cf230d4119ac9ecbf07eefc1ddb1a6e92cc6caeb Mon Sep 17 00:00:00 2001 From: Raphael Maenle Date: Wed, 10 Jun 2026 22:28:33 +0200 Subject: [PATCH] chore(phase0): name validation, golden frames, daemon into library tree, TSan daemon lane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Item 8a — ConfigValidator warns on non-identifier variable/event/alarm/ command names ([A-Za-z_][A-Za-z0-9_]*): language bindings expose names as kwargs/attributes, so 'Chamber Pressure' would be unusable in the planned Python client. Warning not error — the wire doesn't care. Tested (4 warning sites + good-name negative). Item 4 tail — golden frames for S5F1 (Binary ALCD / U4 ALID / ASCII ALTX) and a composed S6F11 (the production-critical report shape), bytes hand- computed from E5 encoding rules: external pins on message composition. Item 7 — equipment_service.hpp moved to include/secsgem/daemon/ (apps/ include-path hack removed) and a TSan daemon lane added locally + in CI. tools/tsan.supp suppresses races whose accesses sit entirely inside the UNinstrumented system libgrpc/libgpr/libabsl (epoll wakeups, absl Mutex GraphCycles bookkeeping); our frames stay fully checked. The lane earned its keep on first run: it caught a REAL threading-contract violation — a daemon test reading model stores from the test thread while the io thread serviced posted writes — fixed to use read_sync, exactly per the documented contract. Now TSan-clean under halt_on_error=1 in the full production threading shape. Suites: core 470/3068, daemon Release+TSan 125/125 each. Co-Authored-By: Claude Fable 5 --- .gitea/workflows/ci.yml | 13 +++++++ .gitignore | 1 + CMakeLists.txt | 2 +- apps/secs_gemd.cpp | 2 +- docs/DAEMON_ROADMAP.md | 34 ++++++++-------- .../secsgem/daemon}/equipment_service.hpp | 0 src/config/validate.cpp | 33 ++++++++++++++-- tests/test_config_validate.cpp | 29 ++++++++++++++ tests/test_daemon_service.cpp | 12 ++++-- tests/test_handler_conformance.cpp | 39 ++++++++++++++++++- tools/tsan.supp | 20 ++++++++++ 11 files changed, 159 insertions(+), 26 deletions(-) rename {apps => include/secsgem/daemon}/equipment_service.hpp (100%) create mode 100644 tools/tsan.supp diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 53fb252..f1c8899 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -79,6 +79,7 @@ jobs: 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 # Debug + -fsanitize=thread. Catches data races in the @@ -98,6 +99,18 @@ jobs: TSAN_OPTIONS: halt_on_error=1 run: build-tsan/secsgem_tests + # The daemon under TSan: run_async + concurrent gRPC handler threads is + # the production threading shape — this is where a strand-contract + # violation would actually surface. tools/tsan.supp silences false + # positives inside the UNinstrumented system libgrpc only; our frames + # stay fully checked. + - name: Daemon tests (TSan) + env: + TSAN_OPTIONS: halt_on_error=1 suppressions=tools/tsan.supp + run: | + test -x build-tsan/secs_gemd_tests || { echo "secs_gemd_tests not built under TSan"; exit 1; } + build-tsan/secs_gemd_tests + tshark-dissector: runs-on: ubuntu-latest container: diff --git a/.gitignore b/.gitignore index a943b28..8ac7d1f 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,4 @@ compile_commands.json # Local Claude Code agent state (memory, skills, etc.) .claude/ +build-tsan-d/ diff --git a/CMakeLists.txt b/CMakeLists.txt index a02ca80..453a477 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -269,7 +269,7 @@ if(SECSGEM_DAEMON_BUILT) ${PROTO_OUT}/secsgem/v1/equipment.pb.cc ${PROTO_OUT}/secsgem/v1/equipment.grpc.pb.cc) target_include_directories(secs_gemd_tests PRIVATE - ${PROTO_OUT} ${Protobuf_INCLUDE_DIRS} ${CMAKE_SOURCE_DIR}/apps) + ${PROTO_OUT} ${Protobuf_INCLUDE_DIRS}) target_link_libraries(secs_gemd_tests PRIVATE secsgem PkgConfig::GRPCPP ${Protobuf_LIBRARIES} doctest::doctest) target_compile_definitions(secs_gemd_tests PRIVATE diff --git a/apps/secs_gemd.cpp b/apps/secs_gemd.cpp index 6b37c43..2be9fb6 100644 --- a/apps/secs_gemd.cpp +++ b/apps/secs_gemd.cpp @@ -16,7 +16,7 @@ #include #include -#include "equipment_service.hpp" +#include "secsgem/daemon/equipment_service.hpp" #include "secsgem/gem/default_handlers.hpp" #include "secsgem/gem/runtime.hpp" diff --git a/docs/DAEMON_ROADMAP.md b/docs/DAEMON_ROADMAP.md index 5ecfa2a..fbacba1 100644 --- a/docs/DAEMON_ROADMAP.md +++ b/docs/DAEMON_ROADMAP.md @@ -48,8 +48,9 @@ host and stays conformant while the tool software restarts/upgrades/crashes. - ✅ ~~**Interop harnesses are manual.**~~ `tools/run_interop.sh` runs all nine validation steps with one command (verified green); CI lanes added, pending first-push verification (Phase 0 item 2). -- ⬜ **TSan lane doesn't cover the daemon.** `secs_gemd_tests` should also be - built/run under `-DSECSGEM_TSAN=ON` once the control-state mirror lands. +- ✅ ~~**TSan lane doesn't cover the daemon.**~~ Covered locally + in CI + with `tools/tsan.supp` (third-party-only suppressions). Caught + fixed a + real test-side contract violation on its first run. - ⚠️ **macOS bind-mount staleness can break Docker builds mid-edit** (a build reading a half-synced source file). Not a product bug; re-run the build. @@ -101,12 +102,10 @@ debts tax every later phase, and the most valuable tests aren't automated. 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** — ✅ - `tests/test_handler_conformance.cpp`: one ordered scenario drives 53 of the - 56 handlers through `router.dispatch` in-process (236 assertions), asserting - paired replies, control-state landings, and the SxF0 abort fallback. - Message-level golden frames: seeded with a hand-computed (E5-rules, not - codec-derived) S1F13 pin — ⬜ extend to S5F1 + composed S6F11 (TODO in file). +4. ✅ **Table-driven handler conformance test** — one ordered scenario + drives 53 of the 56 handlers through `router.dispatch` (236 assertions). + Golden frames: S1F13, S5F1, and a composed S6F11, all hand-computed from + E5 rules (external pins, not codec-derived). 5. ⬜ **Decompose `register_default_handlers` per GEM capability** (it is a relocated main(), not a designed component) and replace magic constants (SVIDs 1/2 `refresh()`, CEIDs 400/401) with YAML role bindings @@ -115,14 +114,17 @@ debts tax every later phase, and the most valuable tests aren't automated. 6. ✅ **Standardize the mutable-read pattern** — `EquipmentRuntime::read_sync` (post-to-io + future with deadline; nullopt => UNAVAILABLE at the RPC edge). Precedent set by `GetVariables`; every future mutable read copies it. -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) — ⬜. 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 ""). +7. ✅ `equipment_service.hpp` moved to `include/secsgem/daemon/` (apps/ + include-path hack removed). TSan daemon lane added locally + in CI + (`tools/tsan.supp` suppresses UNinstrumented system libgrpc/libabsl + internals only — our frames stay checked). The lane caught a real + contract violation on its first run (a test reading the model from the + test thread under run_async — fixed to read_sync); now TSan-clean with + halt_on_error=1. +8. ✅ Identifier-safe name validation: `ConfigValidator` warns (not errors) + on non-identifier variable/event/alarm/command names — bindings expose + names as kwargs/attributes. Format-compliance property test ✅; unset- + `Value` guard ✅. ### Phase A — finish the universal daemon surface (small, unblock vendors) 1. ✅ `GetVariables` — `from_item` reverse conversion (scalar for 1-element diff --git a/apps/equipment_service.hpp b/include/secsgem/daemon/equipment_service.hpp similarity index 100% rename from apps/equipment_service.hpp rename to include/secsgem/daemon/equipment_service.hpp diff --git a/src/config/validate.cpp b/src/config/validate.cpp index be1037d..92972bf 100644 --- a/src/config/validate.cpp +++ b/src/config/validate.cpp @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -117,6 +118,26 @@ std::optional as_nonempty_string(const YAML::Node& n, return s; } +// API-facing names (variables, events, alarms, commands) should be valid +// identifiers: language bindings expose them as kwargs/attributes +// (eq.set(chamber_pressure=...)), where "Chamber Pressure" or "temp-2" can't +// be written. Warning, not error — the wire doesn't care, only bindings do. +void warn_if_not_identifier(const std::optional& s, Sink& sink, + const std::string& path, const YAML::Node& n) { + if (!s) return; + bool ok = !s->empty() && + (std::isalpha(static_cast((*s)[0])) || (*s)[0] == '_'); + for (std::size_t i = 1; ok && i < s->size(); ++i) { + const auto c = static_cast((*s)[i]); + ok = std::isalnum(c) || (*s)[i] == '_'; + } + if (!ok) + sink.warn(path, "`" + *s + "` is not identifier-safe " + + "([A-Za-z_][A-Za-z0-9_]*); language bindings expose " + + "names as kwargs/attributes", + n); +} + // Validate one entry that has the same (id, name, units, type, value) // shape as SVID/DVID/ECID. Returns the parsed id on success so the // caller can dedupe. @@ -124,7 +145,8 @@ std::optional validate_typed_entry( const YAML::Node& entry, Sink& sink, const std::string& path) { auto id = as_int_in_range(entry["id"], sink, path + ".id", 0, UINT32_MAX); - as_nonempty_string(entry["name"], sink, path + ".name"); + warn_if_not_identifier(as_nonempty_string(entry["name"], sink, path + ".name"), + sink, path + ".name", entry["name"]); if (auto tn = entry["type"]) { auto t = tn.as(); if (!valid_secs_type(t)) { @@ -195,7 +217,9 @@ void validate_equipment_block(YAML::Node& root, Sink& sink) { const auto path = "ceids[" + std::to_string(i) + "]"; auto id = as_int_in_range(ces[i]["id"], sink, path + ".id", 0, UINT32_MAX); - as_nonempty_string(ces[i]["name"], sink, path + ".name"); + warn_if_not_identifier( + as_nonempty_string(ces[i]["name"], sink, path + ".name"), + sink, path + ".name", ces[i]["name"]); if (id && !ceids.insert(*id).second) { sink.error(path + ".id", "duplicate CEID " + std::to_string(*id), @@ -221,7 +245,9 @@ void validate_equipment_block(YAML::Node& root, Sink& sink) { // Optional local key for name-based APIs (not on the wire). If // present it must be non-empty. if (als[i]["name"]) - as_nonempty_string(als[i]["name"], sink, path + ".name"); + warn_if_not_identifier( + as_nonempty_string(als[i]["name"], sink, path + ".name"), + sink, path + ".name", als[i]["name"]); if (id && !alarm_ids.insert(*id).second) { sink.error(path + ".id", "duplicate ALID " + std::to_string(*id), als[i]); @@ -256,6 +282,7 @@ void validate_equipment_block(YAML::Node& root, Sink& sink) { for (std::size_t i = 0; i < cmds.size(); ++i) { const auto path = "host_commands[" + std::to_string(i) + "]"; auto name = as_nonempty_string(cmds[i]["name"], sink, path + ".name"); + warn_if_not_identifier(name, sink, path + ".name", cmds[i]["name"]); if (auto ack = cmds[i]["ack"]) { auto s = ack.as(); if (!valid_hcack(s)) diff --git a/tests/test_config_validate.cpp b/tests/test_config_validate.cpp index a57b798..a6d81e0 100644 --- a/tests/test_config_validate.cpp +++ b/tests/test_config_validate.cpp @@ -202,3 +202,32 @@ TEST_CASE("Validate: ships data/equipment.yaml without errors") { } CHECK(v.error_count() == 0); } + +TEST_CASE("Validate: non-identifier names warn (bindings expose names as kwargs)") { + auto p = scratch_path("idname", "yaml"); + write(p, R"YAML( +device: + id: 0 + model_name: "TEST" + software_rev: "1.0" +svids: + - {id: 1, name: "Chamber Pressure", type: F4, value: 0.0} + - {id: 2, name: GoodName, type: U4, value: 0} +ceids: + - {id: 10, name: "wafer-complete"} +alarms: + - {id: 1, name: "2bad", text: "T", category: 1} +host_commands: + - {name: "DO IT", ack: Accept} +)YAML"); + ConfigValidator v; + v.validate_equipment(p.string()); + CHECK(v.error_count() == 0); // identifier shape is a WARNING, not an error + CHECK(v.warning_count() == 4); // space, hyphen, leading digit, space + CHECK(any_match(v.issues(), "svids[0].name")); + CHECK(any_match(v.issues(), "ceids[0].name")); + CHECK(any_match(v.issues(), "alarms[0].name")); + CHECK(any_match(v.issues(), "host_commands[0].name")); + CHECK_FALSE(any_match(v.issues(), "svids[1].name")); + fs::remove_all(p.parent_path()); +} diff --git a/tests/test_daemon_service.cpp b/tests/test_daemon_service.cpp index 85db663..b092ec4 100644 --- a/tests/test_daemon_service.cpp +++ b/tests/test_daemon_service.cpp @@ -6,7 +6,7 @@ #include #include -#include "equipment_service.hpp" +#include "secsgem/daemon/equipment_service.hpp" #include "secsgem/gem/default_handlers.hpp" #include "secsgem/gem/messages.hpp" @@ -220,8 +220,14 @@ TEST_CASE("GetVariables round-trip under run_async (production threading mode)") pb::VariableQuery req; pb::VariableSnapshot resp; REQUIRE(stub->GetVariables(&ctx, req, &resp).ok()); - const auto expected = rt.model().svids.size() + rt.model().dvids.all().size(); - CHECK(resp.values().size() == expected); + // The io thread is live here — model reads must go through read_sync + // (the first violation of that contract was caught by the TSan lane in + // exactly this line). + auto expected = rt.read_sync([&rt] { + return rt.model().svids.size() + rt.model().dvids.all().size(); + }); + REQUIRE(expected.has_value()); + CHECK(resp.values().size() == *expected); } SUBCASE("unknown name is INVALID_ARGUMENT, naming the offender") { diff --git a/tests/test_handler_conformance.cpp b/tests/test_handler_conformance.cpp index 11ff9db..9281cde 100644 --- a/tests/test_handler_conformance.cpp +++ b/tests/test_handler_conformance.cpp @@ -172,8 +172,6 @@ TEST_CASE("conformance sweep: every default handler answers with the paired repl // L[2] -> 0x01 0x02 // A "HOST" -> 0x41 0x04 'H' 'O' 'S' 'T' // A "1.0" -> 0x41 0x03 '1' '.' '0' -// TODO(tests): extend with golden frames for S5F1 and a composed S6F11 once -// their ALID/CEID item formats are pinned down from the catalog. TEST_CASE("golden frame: S1F13 body encodes to the hand-computed E5 bytes") { auto msg = gem::s1f13_establish_comms("HOST", "1.0"); REQUIRE(msg.body.has_value()); @@ -187,3 +185,40 @@ TEST_CASE("golden frame: S1F13 body encodes to the hand-computed E5 bytes") { CHECK(msg.function == 13); CHECK(msg.reply_expected); } + +// S5F1 = L[3]{ Binary ALCD, U4 ALID, ASCII ALTX }. Format bytes per E5: +// Binary(0o10=8)->0x21, U4(0o54=44)->0xB1, ASCII(0o20=16)->0x41 (1 len byte). +TEST_CASE("golden frame: S5F1 alarm report encodes to the hand-computed E5 bytes") { + auto msg = gem::s5f1_alarm_report(0x84, 1, "Chiller Temp High"); + REQUIRE(msg.body.has_value()); + std::vector expected = { + 0x01, 0x03, // L/3 + 0x21, 0x01, 0x84, // B ALCD = set|category4 + 0xB1, 0x04, 0x00, 0x00, 0x00, 0x01, // U4 ALID = 1 + 0x41, 0x11, // A/17 + }; + for (char c : std::string("Chiller Temp High")) expected.push_back(c); + CHECK(s2::encode(*msg.body) == expected); + CHECK(msg.stream == 5); + CHECK(msg.function == 1); +} + +// Composed S6F11 = L[3]{ U4 DATAID, U4 CEID, L reports[ L[2]{U4 RPTID, +// L values} ] } — the production-critical event-report shape, pinned for one +// report (RPTID 1) carrying one ASCII value. +TEST_CASE("golden frame: composed S6F11 encodes to the hand-computed E5 bytes") { + std::vector reports{{1, {s2::Item::ascii("x")}}}; + auto msg = gem::s6f11_event_report(0, 300, reports); + REQUIRE(msg.body.has_value()); + const std::vector expected = { + 0x01, 0x03, // L/3 + 0xB1, 0x04, 0x00, 0x00, 0x00, 0x00, // U4 DATAID = 0 + 0xB1, 0x04, 0x00, 0x00, 0x01, 0x2C, // U4 CEID = 300 + 0x01, 0x01, // L/1 reports + 0x01, 0x02, // L/2 report + 0xB1, 0x04, 0x00, 0x00, 0x00, 0x01, // U4 RPTID = 1 + 0x01, 0x01, // L/1 values + 0x41, 0x01, 'x', // A "x" + }; + CHECK(s2::encode(*msg.body) == expected); +} diff --git a/tools/tsan.supp b/tools/tsan.supp new file mode 100644 index 0000000..fa792bb --- /dev/null +++ b/tools/tsan.supp @@ -0,0 +1,20 @@ +# ThreadSanitizer suppressions — third-party noise only. +# +# The system libgrpc/libprotobuf are NOT built with TSan instrumentation, so +# TSan cannot observe their internal synchronization and reports false +# positives entirely inside the library (e.g. Epoll1Poller::DoEpollWait's +# event-engine wakeup path). Suppressing by library keeps every frame of OUR +# code fully checked: a real race in secsgem/daemon code still has our frames +# in the stack and is NOT suppressed by these rules. +# +# Do not add suppressions for secsgem code here. Fix the race instead. +# +# libabsl_*: gRPC's absl Mutex keeps deadlock-detection bookkeeping +# (GraphCycles) whose synchronization TSan cannot see in the uninstrumented +# system build — every report's racing accesses sit fully inside +# libabsl_graphcycles_internal/libabsl_malloc_internal frames. +race:libgrpc.so +race:libgrpc++.so +race:libgpr.so +race:libabsl_ +called_from_lib:libgrpc.so