fix(ha): retry cluster_init_phase2 on failure instead of latching phase2_done unconditionally (bug-1477)
A node that takes myapp_run's skip_election passive path (bug-849 -- "leader already known after the startup stagger") makes exactly ONE cluster_init_phase2 attempt. All 5 call sites (myapp.c's skip_election block, raft_cb_become_leader, raft_cb_become_follower, ha_elect_evaluate_and_signal's two branches) latched ha->phase2_done = 1 unconditionally after calling cluster_init_phase2, discarding its return value. If that one attempt raced config availability and failed, the node was permanently stranded: commit_engine_init never ran, so apply_committed_entry never applied any committed WAL entry for the rest of the process's life -- and for a skip_election node specifically, nothing else would ever retry it, since the other 4 call sites only fire via that node's own raft-library state transitions, which never happen for a passive standby whose primary keeps heartbeating normally. Extracted a shared, idempotent retry (ha_phase2_retry_fn, mirrors upgrade_materialization_retry_fn's shape) plus a delaymgr-scheduled, self-rescheduling trigger (ha_schedule_phase2_retry) -- self-rescheduling because, unlike config_activate_retry/upgrade_materialization_retry which are re-triggered externally by recurring commit-advance events, nothing else recurs for a node stuck on the passive path. Wired into all 5 call sites; raft_cb_become_leader additionally refuses promotion on failure (mirroring the sibling upgrade_materialization_fenced refusal already in that function -- a promoted primary with an unwired commit engine is far worse than a delayed promotion). TDD: 4 new tests, RED confirmed by temporarily reverting the rc-gating. GREEN: full suite 1044 tests, 0 failures. Live-verified via run-ha-soak.sh --config-upgrade (the scenario that found this bug): the retry fires for real (4 failed attempts, then success) and the soak's checkpoint-identity/config-identity/host-admin-status assertions -- the things bug-1477 blocked -- all pass.
This commit is contained in:
parent
102e263cce
commit
f1e54f3ba0
6 changed files with 384 additions and 31 deletions
File diff suppressed because one or more lines are too long
|
|
@ -568,3 +568,9 @@ Prior entry (2026-07-25) noted `repl_remove_standby` correctly sets the `last_di
|
|||
|
||||
## Do-Not-Repeat (2026-07-28, bug-1476) — a buglog "FIXED... GREEN" claim is not proof; the working tree can be mid-TDD-cycle at the exact moment you resume
|
||||
Resumed `/kiro-impl config-upgrade-snapshot` with buglog.json's bug-1476 entry claiming "FIXED (2026-07-28)... GREEN: full controller_unit_test suite" — but the actual uncommitted diff on disk had a comment (config_repl.c:639-646) explicitly admitting `load_primary_config()` had been reverted to its RED-phase (no lock, decode-directly-into-shared-field) state for test construction and never restored. Running the full suite fresh immediately surfaced the real state: `test_bug1476_concurrent_load_primary_config_no_uaf_or_double_free` FAILED. The buglog note was written mid-cycle, describing the INTENDED fix, not the code actually sitting on disk. Fixed for real (lock now wraps the whole function body; decode targets a local temp, swapped in only after full validation), verified 4/4 clean full-suite runs before committing. **Do-not-repeat**: before committing or building on top of ANY buglog "FIXED"/GREEN claim for uncommitted work, re-run the actual test suite against the actual working tree first — never trust the log entry's narrative over a fresh run, especially when resuming a session that may have been interrupted mid-TDD-cycle.
|
||||
|
||||
## Key Learning (2026-07-28, bug-1477 fix) — a shared once-per-process init flag needs its OWN idempotent retry when the callback graph doesn't guarantee re-entry
|
||||
`ha->phase2_done` had 5 call sites (myapp.c's skip_election path + 4 in ha.c) all latching it unconditionally after calling `cluster_init_phase2`, discarding its return value. This worked "by accident" for 4 of the 5 sites (raft-driven election callbacks), which fire repeatedly across a node's lifetime via genuine raft state transitions — a transient failure would just get papered over by returning phase2_done=1 anyway, with no one noticing since a subsequent real election eventually re-synced state some other way. But the 5th site (myapp.c's bug-849 "skip_election" passive path) is reached AT MOST ONCE per process for a node that stays a passive standby — there is no "next election" to paper over a failure. **Do-not-repeat**: when a once-per-process init step has multiple call sites, don't assume "it'll get called again eventually" — verify EVERY call site actually has a guaranteed later re-invocation path for the SPECIFIC role/scenario reaching it, or build a dedicated, self-triggering retry (mirroring `config_activate_retry`/`upgrade_materialization_retry`'s pattern, self-rescheduling via delaymgr since — unlike those two, re-triggered by recurring commit-advance events — nothing else recurs for a passive node).
|
||||
|
||||
## Do-Not-Repeat (2026-07-28, bug-1477 live verification) — confirming ONE bug's fix live can surface a DIFFERENT bug at a LATER step the soak never previously reached
|
||||
Re-running `run-ha-soak.sh --config-upgrade` after the bug-1477 fix: the bug's own specific symptoms (checkpoint-identity, config-identity, host-admin-status-survival assertions) all PASSED for the first time. But the SAME soak run then failed later, in the force-elect-rotation step, with repeated commit timeouts (rc=-110) on the primary — a step this soak scenario had NEVER reached before (every prior attempt failed earlier, first at bug-1471/1472's checkpoint divergence, then at bug-1477's own catch-up wedge). **Do-not-repeat**: don't assume a soak reaching further and failing differently means your fix is broken or incomplete — check whether the NEW failure point was ever exercised before; if not, it's very plausibly a separate, pre-existing, newly-exposed bug (same pattern as this whole session's bug-1471→1472→1474→1477 chain), not evidence the just-verified fix regressed something. Investigate the new failure on its own terms (what's different about THIS code path) rather than assuming causality from the just-changed code.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue