feat(ha-term-safety): drive ACK path through raft_on_append_entries_reply + advance_commit (task 3.3)

Includes fix for bug-960: endpoint-type-aware peer-index resolution
(promoted-vote relative vs heartbeat-standby absolute) with fail-loud
bounds check, preventing an out-of-bounds match_index[] write / wrong-peer
false-quorum on the replication ACK path.
This commit is contained in:
Rongsong Shen 2026-06-22 09:25:44 +00:00
parent 754e586f8f
commit f1783f7952
4 changed files with 466 additions and 2 deletions

View file

@ -78,7 +78,7 @@
- _Boundary: ha intake + ha_convergence_
- _Depends: 2.5_
- [ ] 3.3 Drive `raft_on_append_entries_reply` and `raft_advance_commit` on the ACK path
- [x] 3.3 Drive `raft_on_append_entries_reply` and `raft_advance_commit` on the ACK path
- On a replication ACK, route it through `raft_on_append_entries_reply` to update `match_index[]`/`next_index[]`, then drive `raft_advance_commit` so the library recomputes the current-term-gated commit and may fire `on_commit_advanced`
- Observable completion: a quorum ACK advances the library `commit_index` only when the agreed entry is current-term, which then triggers the barrier; a stale-term reply is dropped
- _Requirements: 4.1, 4.5_
@ -177,3 +177,8 @@
- **3.2 done.** Heartbeat/replication intake routes through the library. NEW `ha_consume_append_entries` (ha.c) = library-route intake; `ha_consume_heartbeat_committed` (~5691) now sends a 0-entry AE through `raft_on_append_entries_request` for UpdateTerm (G5) BEFORE the monotonic committed-learn — old term-blind `if(hb->term<myterm)return` DELETED; reachable on the LIVE heartbeat path (cluster.c:326). `ha_apply_wal_catchup_entry` (~2034) routes term-carrying single-entry AE through the library (logOk G2 + (term,lsn) truncate G3 via on_truncate_from/on_persist_entry). `ha_rejoin_reconcile` (ha_convergence.c) RETIRED to no-op; dead `ha_rejoin_reconcile_fn`/`_ctx_t` removed (the watermark committed-boundary truncate was the bug-851-unsafe heuristic). Wire gap CLOSED: `WalCatchupEntry.leaderCommit @7` (controller.capnp, append-only, no collision w/ term@4/prevLogIndex@5/prevLogTerm@6) encoded (request.c/repl.c) + decoded into typemap `leader_commit` + passed to the library (library currently `(void)`s it — commit-advance is 3.3). 3 new intake tests + 3 rewritten rejoin tests (test_demotion_convergence.c — preserve safety asserts, pin the retired contract, NOT weakened). 156 Tests 0 Failures; reviewer APPROVED (verified live-reachability, replace-not-parallel, leaderCommit end-to-end, baseline-stash proved bug-924 cluster pre-existing).
- **CRITICAL for 3.3 (the live-path gap):** live steady-state replication still uses TERM-0 `repl_broadcast` (legacy direct-apply in ha_apply_wal_catchup_entry, gated `entry->term != 0`). So the term-carrying single-entry AE RECEIVE route is UNIT-TEST-ONLY in production until 3.3 wires the library SEND loop (`raft_on_append_entries_reply` + `raft_advance_commit`) AND RETIRES the term-0 `repl_broadcast`. The higher-term-heartbeat UpdateTerm IS live (heartbeats are 0-entry → always go through the library route). Live divergent-tail convergence is validated in 5.2/5.4.
- **LOW cleanup (5.5 / cleanup pass):** (1) ha_convergence.c ~292-297 doc-comment on `cluster_apply_ctrlhello_reply` still describes the old "clamped truncate + restart" — stale. (2) `ha.h:197 rejoin_reconciled` atomic is now a dead struct field (once-guard unused). Both harmless/cosmetic. Add to the growing 5.5 cleanup list alongside the `ha_cb_*_stub` rename + stale stub-header comment.
- **3.3 done (REJECTED round 1, fixed, APPROVED round 2).** ACK path: NEW `ha_raft_on_repl_ack(c, abs_peer_idx, acked_lsn, reply_term)` (ha.c ~2914) calls `raft_on_append_entries_reply` then `raft_advance_commit` → fires `on_commit_advanced` on a strict current-term advance (activates the 2.4 commit gate). Wired into the production walAck success branch (`repl_ack_decode_and_dispatch`, ha.c ~4327); `reply_term = war_raw.term`. §4.1/§4.5 current-term gate owned by library (raft.c G4); stale-term reply dropped library-first. 6 ACK tests; reviewer APPROVED round 2 after a full re-verify.
- **bug-960 (CRITICAL, caught by review — logged in buglog.json):** `ha_raft_on_repl_ack` first applied the votes-index→absolute remap UNCONDITIONALLY, but the walAck call site fires for BOTH promoted-vote eps (`vctx->ctx != NULL`, relative peer_idx) AND heartbeat standby eps (`vctx->ctx == NULL`, ALREADY-absolute peer_idx — see authoritative ha.c ~6172-6189 / ~5808). Over-shift → OOB `match_index[]` write (raft.c:802 has NO bounds check) in a 3-node cluster / wrong-peer FALSE-QUORUM data-loss in larger. FIX: `ha_repl_ack_abs_peer_idx(ha, ep_is_vote, peer_idx)` resolver (vote→remap, heartbeat→passthrough) resolved at the call site via `vctx->ctx != NULL`; `ha_raft_on_repl_ack` now takes ABSOLUTE idx + fail-loud `[0,n_peers)` self-excluding bounds check (logs+returns, never crashes/self-terminates). Unit tests had MASKED it (only exercised votes_idx under my_priority==0). LESSON: any peer-index passed to a library `match_index[]`/`next_index[]` write MUST be resolved per endpoint-type (vote-relative vs heartbeat-absolute) — never blind-remap; test BOTH endpoint types at my_priority 0 AND non-0.
- **DO-NOT-REPEAT (cerebrum):** an implementer ran `clang-format -i` (whole-file reformat) then `git checkout HEAD -- <files>` to undo it, which WIPED the uncommitted task diff (HEAD was at the prior task's commit). It reconstructed the diff from context and re-review confirmed fidelity, but: never `git checkout HEAD --` to undo a reformat on a file holding uncommitted work; reformat only the changed hunks.
- **STILL DEFERRED to 5.2/5.4 (carried 3.2→3.3):** live steady-state send is still TERM-0 `repl_broadcast`; the library send loop (`on_send_append_entries`) is not yet the live sender, so the ACK→commit path is proven end-to-end in UNIT TESTS but the first LIVE `on_commit_advanced(>=1)` requires retiring term-0 `repl_broadcast` + driving the library send. Live chaotic convergence (and this retirement) is validated/owned by 5.2 (clean failover) + 5.4 (chaotic regression). Phase 3 (drive library entry points) is structurally complete; phase 4 (config marker) + phase 5 (verification) remain.