From 941f9ef458f564de55a050e858b9f35bf9881646 Mon Sep 17 00:00:00 2001 From: Raphael Maenle Date: Wed, 10 Jun 2026 18:57:53 +0200 Subject: [PATCH] docs: add Phase 0 (structural debts from design review); fix CompleteCommand contract comment Phase 0 captures the 2026-06-10 review: multi-observer callbacks (done for the critical three), CI for the interop/conformance harnesses (the unit suite is partly self-referential; the external validators are the real oracle), table-driven handler conformance + message-level golden frames, register_default_handlers decomposition per GEM capability + YAML role bindings for today's magic constants, the post+future mutable-read pattern, service relocation + TSan run_async daemon test, identifier-safe name validation. CompleteCommand's proto comment described the rejected blocking model; it now states the settled HCACK-4 contract. Co-Authored-By: Claude Fable 5 --- docs/DAEMON_ROADMAP.md | 48 +++++++++++++++++++++++++++----- proto/secsgem/v1/equipment.proto | 10 +++++-- 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/docs/DAEMON_ROADMAP.md b/docs/DAEMON_ROADMAP.md index 7ae0df2..064f635 100644 --- a/docs/DAEMON_ROADMAP.md +++ b/docs/DAEMON_ROADMAP.md @@ -34,13 +34,11 @@ host and stays conformant while the tool software restarts/upgrades/crashes. ## Known issues (found in the 2026-06-10 audit; honest list) -- 🚧 **`GetControlState` cross-thread read.** The gRPC handler reads the control - FSM's state enum while the io thread may transition it — a narrow data race - (single enum read; benign on every real ABI, but TSan-visible and sloppy). - Fix: an atomic state mirror in `EquipmentRuntime`. NOTE: cannot piggyback on - `ControlStateMachine::set_state_change_handler` — that is a single slot - already owned by `register_default_handlers`; either add multi-handler - support or update the mirror inside the runtime's own wiring. +- ✅ ~~**`GetControlState` cross-thread read.**~~ Fixed 2026-06-10: the runtime + keeps an atomic control-state mirror updated via an `add_state_change_handler` + observer (`HandlerSlot` primary+observers pattern), so the mirror survives + `register_default_handlers` claiming the primary slot. `control_state()` is + now safe from any thread. - ⬜ **Alarms have no name key.** `equipment.yaml` alarms carry only numeric `id` + freetext `text` (matches SEMI: ALID/ALTX; there is no standard short name). The name-based `SetAlarm`/`ClearAlarm` RPCs need an optional local @@ -83,6 +81,42 @@ Open sub-decisions to settle while implementing: ## Plan — ordered next steps +### Phase 0 — structural debts (from the 2026-06-10 design review; pay before sprinting) +The review's verdict: architecture and API bets are sound, but two structural +debts tax every later phase, and the most valuable tests aren't automated. + +1. 🚧 **Multi-observer callbacks** (THE structural blocker — hit twice already). + `HandlerSlot` (primary slot keeps legacy set_ semantics; append-only add_ + observers survive it) — done for `ControlStateMachine` + PJ/CJ stores, plus + runtime atomic control-state mirror (race retired) and `add_link_observer` + (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. +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) + pairs through `router.dispatch` for broad coverage of the 56 handlers) + + message-level golden wire frames (codec KATs exist; message-level don't). +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 + (`control_state_svid:`, `cj_executing_ceid:` …). Gradual; aligns with the + capability structure GEM itself defines (S1F19) and enables vendor subsetting. +6. ⬜ **Standardize the mutable-read pattern** for daemon RPCs: post-to-io + + future with deadline (always truthful; latency irrelevant at SECS rates). + First consumer: `GetVariables` (Phase A1) — set the precedent there. +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). + ### Phase A — finish the universal daemon surface (small, unblock vendors) 1. ⬜ `GetVariables` — needs the reverse `Item → proto Value` conversion (read via post-to-io + future, or serve from a daemon-side cache of last diff --git a/proto/secsgem/v1/equipment.proto b/proto/secsgem/v1/equipment.proto index 5333440..4363d1e 100644 --- a/proto/secsgem/v1/equipment.proto +++ b/proto/secsgem/v1/equipment.proto @@ -59,8 +59,14 @@ service Equipment { // HostRequest messages for as long as the call stays open. rpc Subscribe(SubscribeRequest) returns (stream HostRequest); - // Answer a remote Command delivered on the stream, quoting its `id`. Until you - // call this (or the reply window elapses) the host's transaction stays open. + // Report the outcome of a Command delivered on the stream, quoting its `id`. + // NOTE the contract (SEMI-conformant, non-blocking): the daemon has ALREADY + // answered the host with S2F42 HCACK=4 ("accepted, will finish later") when + // it pushed the command onto the stream — the host's transaction is closed. + // CompleteCommand therefore correlates/audits the command lifecycle; the + // host learns the real outcome via the events/alarms you fire (FireEvent / + // SetAlarm), exactly as E30 intends. A synchronous gating mode (tool decides + // the HCACK before S2F42 goes out) is a possible v2 extension. rpc CompleteCommand(CommandResult) returns (Ack); // ---- Jobs / Carriers: report progress of work the host asked for --------