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 <noreply@anthropic.com>
This commit is contained in:
+41
-7
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user