fix(controller-status): gate electedTerm/electedAt on elected_at_ms, not primaryName
Live 3-node E2E validation (task 7.6) found that cmd_ctrlstatus's electedTerm/electedAt lines (task 7.5) gated on NULLSTR(primaryName), which is unconditionally empty on every non-primary responder in this build (ctrlstatus_fill only self-reports the primary's own name -- ha->primary_host is vestigial, a pre-existing Req 2 behavior). This made electedTerm/electedAt print "unknown" on every standby, forever, even though elected_at_ms was correctly, non-zero, and convergently populated the whole time (term/committedLsn/appliedLsn, sourced the same unconditional way, DID correctly display and converge on the same query). Fix: gate on status->elected_at_ms == 0 instead -- the field stays exactly 0 until a real promotion stamps it (either road) or a heartbeat carries a learned value from an actual primary, unlike term (which can legitimately bump from mere vote/candidacy activity before any primary is elected). ctrlstatus_fill itself is untouched; this is purely a cmd_ctrlstatus display-layer correction. Also corrects design.md's "Design Delta -- Requirement 12" Locked Decision 3 (and its dependent traceability/service-contract text) to document this live-E2E finding, mirroring the spec's existing delta convention (Req 10/11's own live-E2E correction). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC
This commit is contained in:
parent
d982d34eb5
commit
808396618c
2 changed files with 26 additions and 17 deletions
|
|
@ -320,15 +320,16 @@ No new files (the capnp field is generated; all logic slots into existing TUs).
|
|||
User enhancement request: `ctrlstatus` should additionally report the term in which the current primary was elected and the wall-clock time it was elected, so operators can tell how long the current primary has served and correlate its election with the logs — with the same off-primary (standby) fidelity Req 10 already established for committed position, not a bare "unknown" off-primary. Scope of this delta = Req 12 only. Reqs 1–11 are implemented and unchanged.
|
||||
|
||||
### Boundary Commitments (delta)
|
||||
- **This spec owns (added):** a new `ha_state_t.last_known_elected_at_ms` field (`_Atomic uint64_t`, declared alongside `last_known_committed_lsn`/`last_known_committed_config_tag` in `ha.h`), self-stamped by BOTH promotion roads at the exact point each commits to `CTRL_PRIMARY`; a new `HeartbeatMsg.electedAtMs` wire field the primary echoes on every outgoing heartbeat; the receiving standby's unconditional store of that value into its own `last_known_elected_at_ms`; a new `CtrlStatus.electedAtMs` reply field; `ctrlstatus_fill` populating it (one unconditional read, no role-branch — see Locked Decision 2); `cmd_ctrlstatus`'s two new printed lines (`electedTerm`, `electedAt`), gated on the same primaryName-known/unknown derivation Req 2 already computes.
|
||||
- **This spec owns (added):** a new `ha_state_t.last_known_elected_at_ms` field (`_Atomic uint64_t`, declared alongside `last_known_committed_lsn`/`last_known_committed_config_tag` in `ha.h`), self-stamped by BOTH promotion roads at the exact point each commits to `CTRL_PRIMARY`; a new `HeartbeatMsg.electedAtMs` wire field the primary echoes on every outgoing heartbeat; the receiving standby's unconditional store of that value into its own `last_known_elected_at_ms`; a new `CtrlStatus.electedAtMs` reply field; `ctrlstatus_fill` populating it (one unconditional read, no role-branch — see Locked Decision 2); `cmd_ctrlstatus`'s two new printed lines (`electedTerm`, `electedAt`), gated on `elected_at_ms == 0` (corrected 2026-09-16 from an original primaryName-based gate — see Locked Decision 3).
|
||||
- **Out of Boundary (firm):** `electedTerm` gets **no new field** — it reuses the existing `CtrlStatus.term` (Req 6.1) with CLI-side "unknown" gating (Locked Decision 1); this spec does not change how elections are conducted, how `ha->term` advances, or how/when `ha->role` transitions — every stamp site added here is purely observational at an already-decided promotion instant (Req 12.5).
|
||||
- **Allowed Dependencies (added):** `ha->term` and the existing primaryName derivation (Req 2/3, unchanged); `clock_gettime(CLOCK_REALTIME)` (existing precedent, Req 11); `ctrlstatus_format_time_ms` (`src/include/xce/ctrlstatus_fmt.h`) reused verbatim for rendering — no new formatting code.
|
||||
- **Revalidation Triggers (added):** any change to where `ha->role` finally becomes `CTRL_PRIMARY` on either promotion road (`ha_do_promote` or `raft_cb_become_leader`) → re-check the `last_known_elected_at_ms` stamp site is still past every refusal/eligibility gate; any change to the `HeartbeatMsg`/`heartbeat_msg_t` shape → re-check the `electedAtMs` ordinal and decode; any change to the primaryName-unknown derivation (Req 2) → re-check the electedTerm/electedAtMs "unknown" gating that reuses it.
|
||||
|
||||
### Locked design decisions
|
||||
1. **`electedTerm` reuses `ha->term` (Req 6.1's existing wire field) — no new state.** This codebase's Raft term already propagates correctly to every node via the existing heartbeat/vote term-sync path; there is no "stale/zero" class of bug here the way Req 10 had for `committedLsn` (which read an unrelated *primary-only* marker, `persist.committed_lsn`, that stays 0 off-primary by construction). `cmd_ctrlstatus` derives the displayed `electedTerm` as `primaryName known ? term : unknown` — purely a print-layer decision, no handler change for this half of the requirement. **Accepted trade-off:** in the narrow window where a standby has just voted for a new candidate (bumping its own `ha->term`) before its `voted_for_name`-derived primaryName reflects that same candidate, the displayed `electedTerm` could transiently be a half-step out of step with the displayed primary — the same class of transient standby-hint staleness already accepted for the original `primaryName`/`term` fields (see Architecture's "any-node hint then authoritative-from-primary" flow) and for Req 10's own documented staleness caveat. Not a regression, not new risk surface.
|
||||
1. **`electedTerm` reuses `ha->term` (Req 6.1's existing wire field) — no new state.** This codebase's Raft term already propagates correctly to every node via the existing heartbeat/vote term-sync path; there is no "stale/zero" class of bug here the way Req 10 had for `committedLsn` (which read an unrelated *primary-only* marker, `persist.committed_lsn`, that stays 0 off-primary by construction). `cmd_ctrlstatus` derives the displayed `electedTerm` as `elected_at_ms != 0 ? term : unknown` (corrected 2026-09-16 — see Locked Decision 3; originally `primaryName known ? term : unknown`) — purely a print-layer decision, no handler change for this half of the requirement. **Accepted trade-off:** in the narrow window where a standby has just voted for a new candidate (bumping its own `ha->term`) before its `voted_for_name`-derived primaryName reflects that same candidate, the displayed `electedTerm` could transiently be a half-step out of step with the displayed primary — the same class of transient standby-hint staleness already accepted for the original `primaryName`/`term` fields (see Architecture's "any-node hint then authoritative-from-primary" flow) and for Req 10's own documented staleness caveat. Not a regression, not new risk surface.
|
||||
2. **`electedAtMs` is genuinely new state, written unconditionally (no monotonic/term compare on write).** Both promotion roads write `last_known_elected_at_ms` exactly once, at the instant each road COMMITS to `CTRL_PRIMARY` — i.e., past every refusal/eligibility gate, never at function entry (see Research Items below for the exact confirm-at-implementation lines). The heartbeat-receive site writes it only when already processing a heartbeat from the node's currently-recognized leader — the same precondition `ha_observe_heartbeat_committed`'s existing call site already relies on for `committed_lsn`/`committed_config_tag`. Because every write site is already gated by an existing freshness precondition, no second "which term does my stored value correspond to" comparison is needed. **Simplification vs. Req 10's pattern:** since there is no pre-existing primary-only marker analogous to `persist.committed_lsn` for election time, `ctrlstatus_fill` needs **no role-branch** for this field — one unconditional read of `last_known_elected_at_ms` correctly serves both the primary (self-stamped) and a standby (heartbeat-learned) case.
|
||||
3. **Unknown gating happens at output time, not by proactively zeroing state.** Both `electedTerm` and `electedAtMs` are gated in `ctrlstatus_fill`/`cmd_ctrlstatus` on the SAME primaryName-known/unknown derivation Req 2.2/2.3 already computes. A stale `last_known_elected_at_ms` left over from a since-lost primary is simply never surfaced once primaryName itself reports "unknown" — mirrors how the original design already treats a stale `voted_for_name`/`primary_host`.
|
||||
3. **Unknown gating happens at output time, not by proactively zeroing state.** Both `electedTerm` and `electedAtMs` are gated in `cmd_ctrlstatus` on `elected_at_ms == 0` (see live-E2E correction below), not on the primaryName-known/unknown derivation Req 2.2/2.3 computes. A stale `last_known_elected_at_ms` left over from a since-lost primary is never surfaced because a fresh promotion or heartbeat-learn always overwrites it before it could be observed as stale — the reader never sees an in-between state, so no separate staleness check is needed beyond the zero-check itself.
|
||||
> **Live-E2E correction (2026-09-16, task 7.6 finding):** this decision originally gated on the SAME primaryName-known/unknown derivation Req 2.2/2.3 already computes, reasoning it would mirror how the original design treats a stale `voted_for_name`/`primary_host`. A live 3-node cluster run proved this wrong: `primaryName` is *unconditionally* empty on every non-primary responder (Req 2's own `ctrlstatus_fill` only self-reports the primary's name when the responder itself is primary — `ha->primary_host` is vestigial, per the `ade0081` design note above), so Req 2.3's "standby knows the primary" branch is dead code in the current build via the plain `ctrlstatus` command. Gating on `primaryName` therefore made `electedTerm`/`electedAt` print "unknown" on *every* standby, permanently — even though `elected_at_ms` was correctly, non-zero, and convergently populated the whole time (independently proven live: `term`/`committedLsn`/`appliedLsn`, sourced the identical unconditional way, DID correctly display and converge on the same standby query). Gating on `status->elected_at_ms == 0` instead is both necessary and sufficient: it stays 0 until a real promotion stamps it (either road) or a heartbeat carries a learned value, and — unlike `term`, which can be legitimately nonzero from mere vote/candidacy activity before any primary is actually elected — `elected_at_ms` only ever becomes nonzero via an actual election. This is a `cmd_ctrlstatus`-only correction; `ctrlstatus_fill`'s own unconditional, no-role-branch read (Locked Decision 2) is unaffected and was already correct.
|
||||
4. **Wire type: `electedAtMs @14: UInt64` epoch-milliseconds** — mirrors Req 11's `responderTimeMs` numeric-not-Text precedent exactly (machine-comparable; the client formats it via the same existing `ctrlstatus_format_time_ms`).
|
||||
|
||||
### Research items carried to implementation
|
||||
|
|
@ -369,9 +370,9 @@ No new files (capnp fields are generated; all logic slots into existing TUs) —
|
|||
### Requirements Traceability (delta)
|
||||
| Requirement | Summary | Components | Interfaces |
|
||||
|-------------|---------|------------|------------|
|
||||
| 12.1 | Report the primary's election term + time alongside its identity | `ctrlstatus_fill` (`electedAtMs` read), `cmd_ctrlstatus` (`electedTerm` derived from `term`+`primaryName`) | `CtrlStatus.term` (reused), `CtrlStatus.electedAtMs` (new) |
|
||||
| 12.1 | Report the primary's election term + time alongside its identity | `ctrlstatus_fill` (`electedAtMs` read), `cmd_ctrlstatus` (`electedTerm` derived from `term`, gated on `elected_at_ms`) | `CtrlStatus.term` (reused), `CtrlStatus.electedAtMs` (new) |
|
||||
| 12.2 | Standby reports the LEARNED election term/time, not stale/unset | heartbeat-carried `electedAtMs`; existing term-sync for `electedTerm` | `HeartbeatMsg.electedAtMs`, unconditional store into `last_known_elected_at_ms` |
|
||||
| 12.3 | Unknown when no primary is known | `ctrlstatus_fill`/`cmd_ctrlstatus` gating on primaryName | shares Req 2.2/2.3's existing "unknown" derivation |
|
||||
| 12.3 | Unknown when no primary is known | `cmd_ctrlstatus` gating on `elected_at_ms == 0` (corrected 2026-09-16, see Locked Decision 3) | independent of Req 2.2/2.3's `primaryName` derivation, which is unconditionally empty on a standby in the current build |
|
||||
| 12.4 | Election time on the same clock/format as logs | `ctrlstatus_fill` (`CLOCK_REALTIME` stamp), `cmd_ctrlstatus` render | reuses `ctrlstatus_format_time_ms` (Req 11.2 precedent) |
|
||||
| 12.5 | Read-only; no election/leadership behavior change | stamp sites are observational only; no gating logic altered | n/a |
|
||||
| 12.6 | Test client prints both fields | `cmd_ctrlstatus` | two new printed lines |
|
||||
|
|
@ -381,10 +382,10 @@ No new files (capnp fields are generated; all logic slots into existing TUs) —
|
|||
- **Req 12.5 (read-only invariant) preserved:** the handler itself performs only atomic loads, exactly as every other field; all writes to `last_known_elected_at_ms` happen upstream, at promotion or heartbeat-processing time, never inside `ctrlstatus_fill`.
|
||||
- **`cmd_ctrlstatus` (Req 12.1, 12.3, 12.6):** print
|
||||
```
|
||||
electedTerm: <term> (or "unknown" when primaryName is unknown)
|
||||
electedAt: <ctrlstatus_format_time_ms(elected_at_ms)> (or "unknown")
|
||||
electedTerm: <term> (or "unknown" when elected_at_ms == 0)
|
||||
electedAt: <ctrlstatus_format_time_ms(elected_at_ms)> (or "unknown" when elected_at_ms == 0)
|
||||
```
|
||||
placed after the existing `Primary:` line, grouping the primary's identity with its election term/time.
|
||||
placed after the existing `Primary:` line, grouping the primary's identity with its election term/time. Gated on `elected_at_ms == 0`, not on `primaryName` (corrected 2026-09-16 — see Locked Decision 3).
|
||||
|
||||
### Testing Strategy (delta)
|
||||
**Unit (`test_ctrlstatus.c`):**
|
||||
|
|
|
|||
|
|
@ -3098,15 +3098,23 @@ int cmd_ctrlstatus(int argc, char* argv[])
|
|||
NULLSTR(status->primaryName) ? "unknown" : status->primaryName);
|
||||
|
||||
/* Election term + time of the CURRENT primary (Req 12.1/12.3/12.4/12.6):
|
||||
* gated on the SAME primaryName-known/unknown derivation the Primary:
|
||||
* line above already computes (Req 2.2/2.3) -- a stale/learned
|
||||
* elected_at_ms left over from a since-lost primary must never surface
|
||||
* once primaryName itself reports "unknown" (design.md Locked Decision
|
||||
* 3). electedTerm is DERIVED client-side from the existing `term` field
|
||||
* (no new wire field, design.md Locked Decision 1); electedAt renders
|
||||
* `elected_at_ms` on the same clock/format as responderTime below,
|
||||
* reusing ctrlstatus_format_time_ms (design.md Locked Decision 4). */
|
||||
if (NULLSTR(status->primaryName)) {
|
||||
* gated on elected_at_ms itself, NOT on primaryName (design.md Locked
|
||||
* Decision 3, corrected 2026-09-16 / task 7.6 live E2E finding).
|
||||
* primaryName is unconditionally empty on every non-primary responder
|
||||
* (ctrlstatus_fill only self-reports the primary's own name; a
|
||||
* pre-existing Req 2 behavior, unrelated to this delta), so gating on it
|
||||
* made electedTerm/electedAt read "unknown" on every standby forever,
|
||||
* even once a real election had converged. elected_at_ms stays exactly 0
|
||||
* until a real promotion stamps it (ha_do_promote, task 7.2) or a
|
||||
* heartbeat carries a learned value from an actual primary (task 7.3) --
|
||||
* unlike `term`, which can legitimately bump on mere vote/candidacy
|
||||
* activity (raft term adoption) before any primary is actually elected,
|
||||
* so `term == 0` would be the wrong gate. electedTerm is DERIVED
|
||||
* client-side from the existing `term` field (no new wire field,
|
||||
* design.md Locked Decision 1); electedAt renders `elected_at_ms` on the
|
||||
* same clock/format as responderTime below, reusing
|
||||
* ctrlstatus_format_time_ms (design.md Locked Decision 4). */
|
||||
if (status->elected_at_ms == 0) {
|
||||
printf("electedTerm: unknown\n");
|
||||
printf("electedAt: unknown\n");
|
||||
} else {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue