fix(controller-status): ctrlstatus_fill must prefer a live registry slot over a stale disconnected one (task 8.6, bug-1819)
Found live during task 8.5's optional catching-up-observation attempt: a killed-then-reconnected standby can end up occupying a NEW replicator_t.standbys[] slot while its OLD slot (host/port still populated, ep == NULL) also matches the same configured peer. ctrlstatus_fill's primary-side peer-status loop stopped at the first host:port match, so it permanently reported that peer dead even minutes after it had genuinely reconnected and fully caught up -- violating Requirement 13.3's "true, currently-known connection status" guarantee. Fix: scan all MAX_STANDBYS slots for a given peer and prefer any slot with ep != NULL over a slot with ep == NULL; fall back to dead only when no matching slot is live. Read-only, lock scope unchanged from task 8.2. TDD RED->GREEN (1216 Tests: 2 Failures -> 0), independently reproduced by the reviewer (exact RED replay) plus two independent live kill/backlog/restart/poll cycles on separate fresh clusters, both confirming the previously-stuck standby now converges to ok. design.md's Locked Decision 3 corrected to document the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC
This commit is contained in:
parent
464489d972
commit
6c437e2bed
5 changed files with 790 additions and 440 deletions
|
|
@ -426,7 +426,7 @@ User enhancement request: `ctrlstatus`'s "Controllers (N):" list should addition
|
|||
### Locked design decisions
|
||||
1. **Wire type: `CtrlPeer.status @3: UInt8`**, a small fixed-value code — not `Text`. Mirrors the existing `CtrlStatus.responderRole @3: UInt8` precedent exactly (backed by a C enum + a `ctrlstatus_..._str()` string renderer at the client layer), rather than introducing a new stringly-typed convention for this one field.
|
||||
2. **Status values, in ordinal order:** `0 = unknown`, `1 = ok`, `2 = catching-up`, `3 = dead`. `unknown` is `0` (the default/zero value) so a peer entry that nothing ever sets defaults to the safe, honest "can't tell" state rather than an implicitly-true `ok`.
|
||||
3. **Primary-side sourcing: match by host+port, not by name or position.** `standby_conn_t` (the primary's live connection-tracking slot) stores `host`/`port` but not a configured peer name or a stable index into `peers_cfg.candidates[]` — so `ctrlstatus_fill`, when the responder is primary, must scan `replicator_t.standbys[]` for a slot whose `host`/`port` match each configured peer's `hostname`/`port`. No match (or a matched slot with `ep == NULL`) → `dead`; a match with `ep != NULL && in_catch_up` → `catching-up`; a match with `ep != NULL && !in_catch_up` → `ok`. This mirrors `repl_active_standby_count`'s own existing `ep != NULL && !in_catch_up` predicate (`repl.c`) — reuse that exact condition rather than inventing a new one.
|
||||
3. **Primary-side sourcing: match by host+port, not by name or position.** `standby_conn_t` (the primary's live connection-tracking slot) stores `host`/`port` but not a configured peer name or a stable index into `peers_cfg.candidates[]` — so `ctrlstatus_fill`, when the responder is primary, must scan `replicator_t.standbys[]` for a slot whose `host`/`port` match each configured peer's `hostname`/`port`. No match (or a matched slot with `ep == NULL`) → `dead`; a match with `ep != NULL && in_catch_up` → `catching-up`; a match with `ep != NULL && !in_catch_up` → `ok`. This mirrors `repl_active_standby_count`'s own existing `ep != NULL && !in_catch_up` predicate (`repl.c`) — reuse that exact condition rather than inventing a new one. **Correction (task 8.6, 2026-09-16, live-E2E finding):** the scan must NOT stop at the first `host`/`port` match. A standby that is killed and later reconnects can end up occupying a NEW `replicator_t.standbys[]` slot while its OLD slot (host/port still populated, `ep == NULL`) also still matches the same configured peer — `repl_add_standby`'s `ep`-identity idempotency check cannot reuse the old slot once its `ep` is gone. A first-match-wins scan then reports that peer `dead` PERMANENTLY, even after it has genuinely reconnected and fully caught up (live-reproduced: `ctrlstatus` stayed `dead` for 5+ minutes after a rejoin the primary's own log showed as fully caught up), violating Req 13.3's "true, currently-known" guarantee. The corrected algorithm scans ALL `MAX_STANDBYS` slots for a given peer and prefers ANY slot with `ep != NULL` (deriving `ok`/`catching-up` from that live slot) over a slot with `ep == NULL`; only falls back to `dead` when no matching slot has `ep != NULL`. See `.wolf/buglog.json` bug-1819.
|
||||
4. **The responder always reports its own entry as `ok`** (Req 13.2) — matched by comparing each configured peer's name against `st->responderName`, the same identity `ctrlstatus_fill` already establishes for itself.
|
||||
5. **Non-primary responder: only self is determinable; every other peer — including the primary — reports `unknown`.** This was an open question at requirements time (whether a standby could additionally report its own primary's status via the liveness signal it already tracks for failover); design-time investigation closes it: that liveness signal (`ha->last_activity`, `liveness_poll_fn`) is a bare elapsed-time scalar with no peer identity attached, and this codebase has no other mechanism (analogous to the vestigial `ha->primary_host`) that would let a standby reliably attribute it to one specific configured peer. Building one would be materially larger than this delta (a genuinely new identity-carrying signal on the heartbeat/connection-accept path) and is explicitly out of boundary here — a future spec's concern, not silently absorbed into this one.
|
||||
6. **Read-only, no role-branch beyond what Decision 3/5 already requires**: `ctrlstatus_fill` performs only reads (of `peers_cfg`, and — only when the responder is primary — of `replicator_t.standbys[]` under its existing lock); no new state is written, no existing connection-management behavior changes.
|
||||
|
|
|
|||
|
|
@ -158,6 +158,8 @@
|
|||
- 6.1: `responderTimeMs @13: UInt64` appended to `CtrlStatus` (after `peerCount @12`, append-only) + matching `uint64_t responder_time_ms` in the hand-authored `xce_ctrlstatus_t` (`src/include/typemap.h`); codec wire offset 48, round-trips. Commit 65ddafe.
|
||||
- 6.2: `ctrlstatus_fill` role-branches committed sourcing — PRIMARY → own `persist.committed_lsn`/`committed_config_tag` (atomic acquire); else → `ha->last_known_committed_lsn` (atomic acquire) + `ha->last_known_committed_config_tag` (plain aligned read, NO `ha->lock` per design — `ha.h:172` is non-atomic, written under lock at `ha_convergence.c:377`). `appliedLsn` unchanged. Timestamp stamped from `CLOCK_REALTIME` (NOT monotonic). Read-only. STALENESS CAVEAT (in-boundary): `last_known_committed_lsn` refreshes at CtrlHello join/RDHUP, not per heartbeat — off-primary committed can lag; live-fresh = OUT of boundary → ha-heartbeat-liveness/ha-repl-hardening. Commit 3799bd6.
|
||||
- 6.3 LEARNING: the controller log timestamp formatter is NOT in `xcelog.h` (an 85-line thin wrapper that just `#include "tlog.h"`); the `YYYY-MM-DD HH:MM:SS,mmm` prefix is assembled inline in vendored tinylog (`tlog.c` `_tlog_root_default_format` ~393/398, via `my_tlog_format` in `src/lib/utils.c:221/228`) — no caller-facing helper, so it was REPLICATED byte-equivalently (`localtime_r` + `strftime "%Y-%m-%d %H:%M:%S"` + `,%03u` of `ms%1000`) in a new header-only `src/controller/testsuite/ctrlstatus_fmt.h` (`ctrlstatus_format_time_ms`), shared by `test.c` (`cmd_ctrlstatus` prints `responderTime:` after `fenced`) and `test_ctrlstatus.c` (TZ=UTC-pinned exact-string unit test). LOCAL time on both sides — comparable to `ctrl-N.log` under the same TZ.
|
||||
- 8.6 FIXED 2026-09-16 (bug-1819, found live during 8.5's optional attempt): `ctrlstatus_fill`'s primary-side peer-status scan stopped at the first `replicator_t.standbys[]` slot matching a peer's `host:port` — after a kill+rejoin, the stale (ep==NULL) slot from before the kill can have a lower index than the new live slot the reconnect landed in, so the peer was reported `dead` PERMANENTLY even after fully reconnecting and catching up. Fixed by scanning all `MAX_STANDBYS` slots and preferring any `ep != NULL` match over an `ep == NULL` one. TDD RED→GREEN (1216 Tests: 2 Failures → 0 Failures), independently reproduced by the reviewer including an exact RED replay. Two independent live re-checks (implementer + reviewer, separate deploy dirs/ports) both confirmed a killed-then-restarted standby now converges to `ok` on every poll instead of sticking at `dead`. Neither live run happened to construct the literal two-distinct-slots scenario (an unrelated, already-tracked bug-1474 angle-a symptom consumed the cluster's one spare slot both times) — the two new unit tests are the deterministic proof of the exact fix mechanism; the live runs are corroborating, not substituting. design.md's Locked Decision 3 corrected in the same pass to document the fix (matches this spec's established as-built-drift-correction convention).
|
||||
|
||||
- 8.4 VERIFIED 2026-09-16: two independent live 3-node-cluster runs (implementer + reviewer, separate deploy dirs/ports) both confirmed the full Req 13 scenario cleanly with ZERO bugs found (unlike 5.1/6.4/7.6, which each found+fixed a real bug) — healthy primary reports all 3 controllers `ok`; killing a standby flips it to `dead` on the primary's view while the other two stay `ok`; querying the surviving standby directly shows `ok` for itself and `unknown` for every other peer, including the still-alive primary, confirming the asymmetric-visibility design decision (Locked Decision 5) holds live. The tasks 8.1-8.3 implementation required no live-only fixes.
|
||||
|
||||
- 8.2 LEARNING: the new `replicator_t.lock` acquisition in `ctrlstatus_fill`'s peer-building loop is held around the whole per-peer loop (calloc + self/primary/non-primary branching for all configured peers), slightly wider than `repl_active_standby_count`'s own lock-and-scan shape that design.md's Constraints-checked note cites as the precedent. Still bounded/safe (n ≤ `MAX_HA_CANDIDATES`, inner scan ≤ `MAX_STANDBYS`, no I/O) and reviewer-approved as non-blocking, but a future touch to this loop should narrow the lock to wrap only the inner `MAX_STANDBYS` registry scan per non-self peer, matching the cited precedent exactly.
|
||||
|
|
@ -200,3 +202,12 @@
|
|||
- Observable: a live run captures at least one `ctrlstatus` query showing catching-up for a rejoining standby before it converges to ok.
|
||||
- _Requirements: 13.1_
|
||||
- _Depends: 8.4_
|
||||
|
||||
- [x] 8.6 Fix: primary-side peer-status matching must prefer a live registry slot over a stale disconnected one
|
||||
- Bug found live during task 8.5's attempt (read-only observation, no code touched at the time): `ctrlstatus_fill`'s primary-side peer-status loop (`src/controller/ctrlstatus.c`) scans `replicator_t.standbys[]` for a `host`/`port` match and stops at the FIRST match by slot index. When a standby is killed, its slot is freed (`ep` cleared) but keeps its `host`/`port` populated (existing bug-1474-documented "guard-holding slot" behavior, unrelated to this delta); when that standby reconnects, `repl_add_standby`'s `ep`-identity idempotency check cannot reuse the old slot (the old `ep` is gone) and allocates a NEW slot instead — so two slots now share the same configured peer's `host:port`, one stale (`ep == NULL`) and one live (`ep != NULL`). Because the stale slot has the lower index, `ctrlstatus_fill`'s first-match-wins scan reports that peer as `dead` PERMANENTLY, even minutes after it has genuinely reconnected and fully caught up — live-reproduced and log-cross-confirmed (see task 8.5's Status Report: primary's own log shows `acked` converging to the target LSN at a specific timestamp, while `ctrlstatus` kept reporting `dead` from restart through +5m45s). This is distinct from bug-1474's already-fixed angle (`on_standby_connected`'s live-duplicate-channel eviction, a different check) — this defect is specific to `ctrlstatus_fill`'s own status-derivation scan and violates Req 13.3's "true, currently-known connection status" guarantee for any standby that has ever been killed and rejoined.
|
||||
- TDD (red first, in `test_ctrlstatus.c`): a fixture with TWO `replicator_t.standbys[]` slots matching the SAME configured peer's `host:port` — a lower-index slot with `ep == NULL` (stale) and a higher-index slot with `ep != NULL && !in_catch_up` (live, caught up) — must report that peer's status as `ok` (sourced from the live slot), not `dead` (the stale slot's would-be status); a sibling case with the live slot `in_catch_up` must report `catching-up`, not `dead`.
|
||||
- Fix: change the matching loop to scan ALL `MAX_STANDBYS` slots for a given configured peer (not stop at the first match) and prefer any slot with `ep != NULL` over a slot with `ep == NULL` when multiple slots match the same `host:port`; if multiple live slots somehow match (shouldn't normally happen), any one of them is an acceptable source since they'd all be observing the same live peer. Keep the read bounded and lock-scoped exactly as task 8.2 left it (no scope regression).
|
||||
- Observable: the new unit test goes red → green; a live re-run of task 8.5's kill-then-restart-under-backlog scenario shows the rejoined standby transition away from a permanent `dead` (ideally observing `catching-up` before settling to `ok`, satisfying 8.5's original optional Observable as a side effect of this fix).
|
||||
- _Requirements: 13.1, 13.3_
|
||||
- _Boundary: ctrlstatus_fill_
|
||||
- _Depends: 8.2_
|
||||
|
|
|
|||
1061
.wolf/buglog.json
1061
.wolf/buglog.json
File diff suppressed because one or more lines are too long
|
|
@ -253,22 +253,43 @@ void ctrlstatus_fill(cluster_t* c, xce_ctrlstatus_t* st)
|
|||
} else if (is_primary) {
|
||||
/* Reuses repl_active_standby_count's exact ep != NULL &&
|
||||
* !in_catch_up predicate (repl.c) rather than inventing a
|
||||
* new one (Locked Decision 3). */
|
||||
uint8_t status = 3u; /* default: no matching live slot */
|
||||
* new one (Locked Decision 3).
|
||||
*
|
||||
* Scans ALL MAX_STANDBYS slots (does NOT stop at the first
|
||||
* host/port match) and prefers any slot with ep != NULL
|
||||
* over a slot with ep == NULL when multiple slots match the
|
||||
* same configured peer's host:port (task 8.6 fix). A killed
|
||||
* standby's slot is freed (ep cleared) but keeps its
|
||||
* host/port populated; when it reconnects, repl_add_standby's
|
||||
* ep-identity idempotency check cannot reuse that now-ep-NULL
|
||||
* slot, so the reconnect lands in a NEW slot -- leaving two
|
||||
* slots that share the same peer's host:port, one stale
|
||||
* (ep == NULL) and one live (ep != NULL). A first-match-wins
|
||||
* scan reported the stale, lower-index slot's dead status
|
||||
* forever, even after the peer genuinely reconnected and
|
||||
* caught up. If more than one live slot somehow matches, any
|
||||
* one of them is an acceptable source -- they would all be
|
||||
* observing the same live peer. */
|
||||
uint8_t status = 3u; /* default: no matching slot at all */
|
||||
int live_match = 0; /* found a slot with ep != NULL */
|
||||
for (int j = 0; j < MAX_STANDBYS; j++) {
|
||||
if (strcmp(repl->standbys[j].host,
|
||||
mc->peers_cfg.candidates[i].hostname) == 0 &&
|
||||
repl->standbys[j].port == p->port) {
|
||||
if (repl->standbys[j].ep == NULL) {
|
||||
status = 3u; /* matched slot, no live conn */
|
||||
} else if (atomic_load_explicit(
|
||||
&repl->standbys[j].in_catch_up,
|
||||
memory_order_acquire)) {
|
||||
status = 2u; /* catching-up */
|
||||
} else {
|
||||
status = 1u; /* ok */
|
||||
}
|
||||
break;
|
||||
mc->peers_cfg.candidates[i].hostname) != 0 ||
|
||||
repl->standbys[j].port != p->port) {
|
||||
continue; /* not a match for this peer */
|
||||
}
|
||||
if (repl->standbys[j].ep == NULL) {
|
||||
if (!live_match)
|
||||
status = 3u; /* stale match; keep scanning for a live one */
|
||||
continue;
|
||||
}
|
||||
if (!live_match) {
|
||||
live_match = 1;
|
||||
status = atomic_load_explicit(
|
||||
&repl->standbys[j].in_catch_up,
|
||||
memory_order_acquire)
|
||||
? 2u /* live, catching-up */
|
||||
: 1u; /* live, caught up: ok */
|
||||
}
|
||||
}
|
||||
p->status = status;
|
||||
|
|
|
|||
|
|
@ -986,6 +986,111 @@ static void test_ctrlstatus_peer_status_self_mismatch_falls_through(void)
|
|||
printf("test_ctrlstatus_peer_status_self_mismatch_falls_through: PASS\n");
|
||||
}
|
||||
|
||||
/* (r) PRIMARY: a killed-then-reconnected standby ends up owning TWO
|
||||
* replicator_t.standbys[] slots for the SAME configured peer -- a
|
||||
* lower-index slot left behind by the kill (ep == NULL, host/port still
|
||||
* populated -- repl_add_standby's ep-identity idempotency check cannot
|
||||
* reuse it, so the reconnect lands in a fresh, higher-index slot) and a
|
||||
* higher-index slot for the genuine live reconnection (ep != NULL). A
|
||||
* first-match-wins scan (pre-fix) reports the stale slot's dead status
|
||||
* PERMANENTLY; the fix must scan every slot and prefer any live
|
||||
* (ep != NULL) match over a stale (ep == NULL) one, regardless of which
|
||||
* index the live slot lands in (Requirements 13.1, 13.3; task 8.6). */
|
||||
static void test_ctrlstatus_peer_status_prefers_live_slot_over_stale_ok(void)
|
||||
{
|
||||
cluster_t c;
|
||||
myconf_t mc;
|
||||
cs_setup_cluster(&c, &mc);
|
||||
|
||||
atomic_store_explicit(&c.ha.role, (int32_t)CTRL_PRIMARY,
|
||||
memory_order_release);
|
||||
|
||||
TEST_ASSERT_EQUAL_INT(0, repl_init(&c.persist.repl));
|
||||
c.ha.repl = &c.persist.repl;
|
||||
|
||||
/* Stale slot (lower index): ctrlB's OLD connection, killed -- ep
|
||||
* cleared but host/port left behind, exactly as repl_remove_standby
|
||||
* leaves it (this fixture models the pre-existing registry state,
|
||||
* regardless of how it got there -- ctrlstatus_fill's boundary is
|
||||
* reading it correctly, not preventing it). */
|
||||
strncpy(c.persist.repl.standbys[0].host, "10.0.0.2",
|
||||
sizeof(c.persist.repl.standbys[0].host) - 1);
|
||||
c.persist.repl.standbys[0].port = 7002;
|
||||
c.persist.repl.standbys[0].ep = NULL;
|
||||
|
||||
/* Live slot (higher index): ctrlB's NEW connection after reconnecting,
|
||||
* fully caught up. */
|
||||
int dummy_ep;
|
||||
strncpy(c.persist.repl.standbys[1].host, "10.0.0.2",
|
||||
sizeof(c.persist.repl.standbys[1].host) - 1);
|
||||
c.persist.repl.standbys[1].port = 7002;
|
||||
c.persist.repl.standbys[1].ep = (repl_ep_t)&dummy_ep;
|
||||
atomic_store_explicit(&c.persist.repl.standbys[1].in_catch_up, 0,
|
||||
memory_order_release);
|
||||
|
||||
/* ctrlC: no matching slot at all -- unaffected, still dead. */
|
||||
|
||||
xce_ctrlstatus_t st;
|
||||
ctrlstatus_fill(&c, &st);
|
||||
|
||||
TEST_ASSERT_EQUAL_INT(3, st.peers.count);
|
||||
TEST_ASSERT_EQUAL_STRING("ctrlA", st.peers.list[0]->name);
|
||||
TEST_ASSERT_EQUAL_UINT8(1, st.peers.list[0]->status); /* self: ok */
|
||||
TEST_ASSERT_EQUAL_STRING("ctrlB", st.peers.list[1]->name);
|
||||
TEST_ASSERT_EQUAL_UINT8(1, st.peers.list[1]->status); /* live slot wins: ok */
|
||||
TEST_ASSERT_EQUAL_STRING("ctrlC", st.peers.list[2]->name);
|
||||
TEST_ASSERT_EQUAL_UINT8(3, st.peers.list[2]->status); /* unaffected: dead */
|
||||
|
||||
ctrlstatus_free_fields(&st);
|
||||
repl_fini(&c.persist.repl);
|
||||
cs_teardown_cluster(&c);
|
||||
printf("test_ctrlstatus_peer_status_prefers_live_slot_over_stale_ok: PASS\n");
|
||||
}
|
||||
|
||||
/* (s) Sibling of (r): the live slot is still catching up (not yet acked up
|
||||
* to the cluster) -- the stale/live preference must still hold, and the
|
||||
* reported status must be catching-up (2), not dead (3) and not the
|
||||
* unconditional ok a naive "any live slot -> ok" fix would wrongly give. */
|
||||
static void test_ctrlstatus_peer_status_prefers_live_slot_catching_up(void)
|
||||
{
|
||||
cluster_t c;
|
||||
myconf_t mc;
|
||||
cs_setup_cluster(&c, &mc);
|
||||
|
||||
atomic_store_explicit(&c.ha.role, (int32_t)CTRL_PRIMARY,
|
||||
memory_order_release);
|
||||
|
||||
TEST_ASSERT_EQUAL_INT(0, repl_init(&c.persist.repl));
|
||||
c.ha.repl = &c.persist.repl;
|
||||
|
||||
/* Stale slot (lower index): ctrlB's OLD connection, killed. */
|
||||
strncpy(c.persist.repl.standbys[0].host, "10.0.0.2",
|
||||
sizeof(c.persist.repl.standbys[0].host) - 1);
|
||||
c.persist.repl.standbys[0].port = 7002;
|
||||
c.persist.repl.standbys[0].ep = NULL;
|
||||
|
||||
/* Live slot (higher index): ctrlB's NEW connection, still catching up. */
|
||||
int dummy_ep;
|
||||
strncpy(c.persist.repl.standbys[1].host, "10.0.0.2",
|
||||
sizeof(c.persist.repl.standbys[1].host) - 1);
|
||||
c.persist.repl.standbys[1].port = 7002;
|
||||
c.persist.repl.standbys[1].ep = (repl_ep_t)&dummy_ep;
|
||||
atomic_store_explicit(&c.persist.repl.standbys[1].in_catch_up, 1,
|
||||
memory_order_release);
|
||||
|
||||
xce_ctrlstatus_t st;
|
||||
ctrlstatus_fill(&c, &st);
|
||||
|
||||
TEST_ASSERT_EQUAL_INT(3, st.peers.count);
|
||||
TEST_ASSERT_EQUAL_STRING("ctrlB", st.peers.list[1]->name);
|
||||
TEST_ASSERT_EQUAL_UINT8(2, st.peers.list[1]->status); /* live slot wins: catching-up */
|
||||
|
||||
ctrlstatus_free_fields(&st);
|
||||
repl_fini(&c.persist.repl);
|
||||
cs_teardown_cluster(&c);
|
||||
printf("test_ctrlstatus_peer_status_prefers_live_slot_catching_up: PASS\n");
|
||||
}
|
||||
|
||||
/* ── suite entry point ─────────────────────────────────────── */
|
||||
|
||||
void test_ctrlstatus(void)
|
||||
|
|
@ -1011,4 +1116,6 @@ void test_ctrlstatus(void)
|
|||
RUN_TEST(test_ctrlstatus_peer_status_primary_sourced_from_registry);
|
||||
RUN_TEST(test_ctrlstatus_peer_status_standby_unknown_except_self);
|
||||
RUN_TEST(test_ctrlstatus_peer_status_self_mismatch_falls_through);
|
||||
RUN_TEST(test_ctrlstatus_peer_status_prefers_live_slot_over_stale_ok);
|
||||
RUN_TEST(test_ctrlstatus_peer_status_prefers_live_slot_catching_up);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue