myxce/.kiro/specs/ha-commit-engine/research.md
Rongsong Shen 20e59d0b42 checkpoint: bug-851 root cause + raft TLA+ conformance spec family
Investigation & fixes (failover committed-job loss):
- root-cause bug-851 on an owned 3-node cluster: chaotic failover causes
  silent committed-prefix divergence (WAL hdr.term always 0; two leaders
  per term; no (term,lsn) reconcile). Evidence in repro-851/ + INVESTIGATION.
- live-validated fixes 848/849/850 (recovery-on-promotion, advertise real
  applied_lsn, persist-layer contiguity guard); add waldump WAL inspector.

Specs (strict raft.tla conformance — library owns the log):
- TLA-gap-analysis.md: impl vs raft/raft.tla, gaps G1-G6.
- raft-log-replication, raft-commit-safety, raft-sim-conformance: extend
  src/lib/raft to own log/commit/verification via callbacks (specs generated,
  cross-spec review passed).
- ha-term-safety: controller-side adapter + config marker (R1-R7); design done.
- config-upgrade-snapshot: R8 descoped here (deps entity-bearing-snapshots, bug-857).
- roadmap + buglog (851/854/857) + .wolf knowledge updated.
2026-06-21 14:02:15 +00:00

98 lines
13 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# Research Log: ha-commit-engine
Date: 2026-06-11. Discovery status: **complete via umbrella** — this sub-spec inherits the ha-consistency-invariants discovery (`.kiro/specs/ha-consistency-invariants/research.md`: gap analysis, Option C selection, deep-dives §A–§C, locked decisions). No decisions are re-derived here. This log records only the slice-scoped code verification performed for this spec's design.
## Inherited Locked Decisions (umbrella — do not re-litigate)
- Commit engine as single authority; WAIT/RETAIN completion modes (research synthesis)
- Admission cap relative to `n_vcpus` (never absolute); `COMMIT_WAIT_RESERVED` default 2 (research item 3(d–e))
- Blocking condvar on vtask threads accepted; async continuation deferred; no liveness risk (ACKs on iomgr) (research item 3(b))
- Catch-up standbys excluded from quorum accounting and commit counting until `repl_complete_catchup` (umbrella §Quorum Accounting + commit_on_membership_change contract)
- Commit state volatile (no persisted commitIndex); convergence/rejoin owned by ha-convergence-recovery (research §C)
- Own-term gate via WAL_NOOP at promotion — API (`commit_set_writable_lsn`) ships in this spec, consumers in ha-single-leader-role (research item 4)
## Slice Verification (2026-06-11, this spec)
| Fact | Evidence | Implication |
|------|----------|-------------|
| `standby_conn_t.acked_lsn` atomic exists, updated by `repl_on_ack` | src/include/repl.h:24; repl.c:176, store at repl.c:187 | matchIndex bookkeeping ready; engine consumes read-only |
| Dishonest counting: `acked_lsn > 0` | repl.c:419 | replaced by per-LSN engine evaluation |
| `n_active == 0 → success` short-circuit | repl.c:432-435 | removed for n_peers>1 via engine (engine knows n_peers; repl does not) |
| Quorum denominator is `n_active`, not `n_peers` | repl.c:437 | engine uses configured `n_peers` per umbrella commit rule |
| `ha_has_write_quorum` counts `standby_endpoints->count` (includes catch-up) | ha.c:1507-1526; gate call at ha.c:1537 | fix via new `repl_active_standby_count` (ep && !in_catch_up); `in_catch_up` at repl.h:26 |
| Sync barrier: `persist_wal_sync_append` | persist.c:1003-1049; committed_lsn store at 1035; `has_standbys → -ENODEV` degraded-success at ~1027-1031; truncation store at persist.c:1438 | adapter passes `written_lsn`; 1035 store moves into engine; -ENODEV case replaced by engine decision; 1438 store stays |
| ACKs processed on iomgr | `peer_ctrl_msg_handler` → `repl_ack_decode_and_dispatch` (ha.c:2632) → `repl_on_ack` call at ha.c:2748 | `commit_on_ack` feed point; must never block |
| Membership mutation points | repl.h:50-51 (`repl_add_standby`/`repl_remove_standby`), repl.h:76/79 (`repl_start_catchup`/`repl_complete_catchup`); removal call sites ha.c:3176, 3866 | `commit_on_membership_change` call sites |
| vtask pool default 8, `--vcpus` override | cluster.c:1076-1083 | `n_vcpus` input to `commit_engine_init`; init point after vtaskmgr sizing |
| `replicator_t` embedded in `persist_t` (`p->repl`); cluster reaches via `c->persist.repl` | persist.c:1032; ha.c:538, 2748 | engine reaches standby registry via its `persist_t*`; `commit_engine_init(ce, p, ...)` signature suffices |
| `ha_state_t` has `n_peers` and `raft_c`, but no repl pointer | ha.h:180-185 | add explicit `replicator_t *repl` back-pointer (raft_c only set by election path — not reliable for the gate) |
| repl.h/persist.h live in src/include (lib-visible); controller-local headers in src/controller/include | find results; cluster.h, worker.h | `commit.h` goes to src/controller/include; repl.h uses forward-declared `struct commit_engine *` to avoid dependency inversion |
| `repl_wait_majority_ack` callers: persist.c:1032 (walAck), config_repl.c:916 (configAck) | grep | signature gains `entry_lsn`; configAck branch untouched (out of boundary) |
| Existing tests encode old semantics | testsuite/test_repl_majority.c (walAck cases at :85-:177); meson controller_unit_test_src list at meson.build:74-110 | walAck cases must be updated; new test file registered in meson |
| `XCE_ERR_NOT_READY` defined | ha.c:35 (`= E_XCE_NOT_READY`) | reuse for admission-cap rejection |
## Design Synthesis Outcomes (this slice)
- **Adapter placement** (brief-mandated): `repl_wait_majority_ack` stays as the shell; gains `entry_lsn` param because the replicator has no LSN knowledge; engine-less fallback retained ONLY for unit-test contexts and explicitly retired by ha-commit-barrier-rollout. Production walAck always goes through the engine.
- **Engine reachability**: forward-declared `struct commit_engine *ce` on `replicator_t` (keeps src/include/repl.h free of controller headers); engine instance embedded in `cluster_t`; init after vtask sizing + peer count known.
- **Standalone behavior change (intentional)**: with the engine wired, `n_peers <= 1` sync appends now advance `committed_lsn` (previously the -ENODEV path left it unmoved). Raft-correct (self-quorum), satisfies spec req 6.3; flagged in design.
- **Membership-shrink semantics**: re-evaluation = wake + re-check; no forced early-fail (umbrella 4.3 requires fail-not-confirm, satisfied by timeout; reconnection within the deadline may still legitimately commit).
- Review gate: design verified against the umbrella contract — interface verbatim, no contradiction with locked decisions, all 20 slice requirement IDs traced.
---
## Gap Re-Audit (2026-06-18)
**Type:** Post-implementation gap audit (not a pre-impl gap analysis). Roadmap declares `ha-commit-engine` IMPLEMENTED 12/12 (2026-06-12, validate-impl GO, 467 tests green). `tasks.md` shows 6 `[ ]` checkboxes; all 6 are **section/group headers** ("1.", "2.", "3.", "4.", "5.", "6."), not leaf tasks — every leaf sub-task (1.1, 1.2, 2.1, 2.2, 3.1, 3.2, 4.1, 4.2, 5.1, 5.2, 5.3, 6.1) is `[x]`. The "12/12" count is the leaf count. The unchecked group headers are an artifact of the checklist format, documented as such in §Implementation Notes — they are **not** deferred work and **not** core gaps.
### Verdict
**STATUS: CLOSED.** All 7 requirements (20 EARS acceptance criteria) are implemented and wired in production. Verified against current on-disk source (codegraph verbatim reads + grep, 2026-06-18). The engine has since been extended by `ha-commit-durability` (durable self-cap) — that extension strengthens, and does not regress, the ha-commit-engine contract.
### Requirement-to-Asset Map (verified, file:line)
| Req | Criterion | Asset (verified) | Status |
|-----|-----------|------------------|--------|
| 1.1 | confirm only after per-entry majority | `commit_submit(WAIT)` predicate `wait_done_locked` commit.c:184-189; sync path persist.c:1897-1900 → `repl_wait_majority_ack` walAck delegation repl.c:447 → `commit_submit` | MET |
| 1.2 | count ACK toward N only if acked_lsn >= N | `majority_candidate_locked` reads `s->acked_lsn`, sorts desc, candidate = `scratch[n_peers/2-1]` commit.c:107-134 | MET |
| 1.3 | connectivity never substitutes for ACKs | ACK-time eval only; watermarks read from registry at `commit_on_ack` commit.c:366-381; gate (ha_has_write_quorum) is fast-reject only, engine re-verifies | MET |
| 2.1 | no commit advance without majority | `commit_advance_locked` stores only `majority_candidate_locked` result commit.c:145-168 | MET |
| 2.2 | monotonic committed_lsn | `if (cand <= cur) return;` + `atomic_store(max)` commit.c:150-165; XCE_TESTING monotonic assert | MET |
| 2.3 | contiguous, bounded by appended log | candidate clamped to `cap` (durable, ≤ `highest_appended_lsn`) commit.c:136-137, 52-55; cumulative watermarks → contiguous | MET (strengthened: cap is now durable_lsn per ha-commit-durability) |
| 3.1/3.2/3.3 | admission quorum counts only members able to receive; catch-up + disconnected excluded | `repl_active_standby_count` (ep != NULL && !in_catch_up) repl.c:668-686; consumed by `ha_has_write_quorum` ha.c:2049-2058 | MET |
| 4.1 | post-acceptance shrink → fail not confirm | WAIT times out (`-ETIMEDOUT`) when majority never reached; no false-success path; `commit_on_membership_change` re-evals monotonically (shrink never regresses) commit.c:397-409 | MET |
| 4.2 | membership change re-evaluates pending waits | `commit_on_membership_change` wired at 4 mutators: repl.c:130 (add), :162 (remove), :598 (start_catchup), :621 (complete_catchup) | MET |
| 4.3/4.4 | zero peer ACKs never majority (n_peers>1); no zero-active short-circuit | engine derives from `n_peers` not n_active; `k = n_peers/2-1`, absent peers zero-filled commit.c:117-134; legacy `n_active==0→success` short-circuit removed (sync path persist.c:1897 engine branch; configAck has explicit "no n_active shortcut" repl.c:506-510) | MET |
| 5.1 | lag never blocks majority | majority is the only threshold; far-lagging standby sorts low, does not affect `scratch[n_peers/2-1]` commit.c:129-134 | MET |
| 6.1 | reject without quorum | `ha_has_write_quorum` false → `ha_dispatch_gate` -XCE_ERR_NOT_READY ha.c:2031-2059 | MET |
| 6.2 | auto-resume on quorum restore | gate is stateless per request (recomputes `repl_active_standby_count` each call); no reset needed | MET |
| 6.3 | standalone self-quorum | `n_peers <= 1` → `commit_advance_locked` immediate, returns cap commit.c:100-102, 330-332; sync path -ENODEV special case removed on engine branch | MET |
| 7.1/7.2 | n_vcpus-relative admission cap, NOT_READY beyond | `wait_cap = max(1, n_vcpus - COMMIT_WAIT_RESERVED)` commit.c:175-180; `inflight_waits >= cap → -XCE_ERR_NOT_READY` commit.c:313-320 | MET |
| 7.3 | ACK processing unaffected at cap | `commit_on_ack` takes only ce->lock, never the inflight gate; advances + broadcasts regardless of cap commit.c:366-381 | MET |
### The 6 Open `[ ]` Tasks — Classified
All 6 are group-header checkboxes whose every child is `[x]`. None is deferred work; none is a core gap. They are a **checklist-rendering artifact** (parent headers left unchecked while leaves complete).
| Header | Children | Classification | Effort/Risk |
|--------|----------|----------------|-------------|
| 1. Commit engine core | 1.1 [x], 1.2 [x] | DONE (artifact) | none |
| 2. WAIT/self-quorum/zero-peer/cap/gate | 2.1 [x], 2.2 [x] | DONE (artifact) | none |
| 3. Membership re-evaluation | 3.1 [x], 3.2 [x] | DONE (artifact; jointly completed — RED unachievable, §Notes) | none |
| 4. Quorum-accounting fix | 4.1 [x], 4.2 [x] | DONE (artifact) | none |
| 5. Integration (wire engine behind job submit) | 5.1 [x], 5.2 [x], 5.3 [x] | DONE (artifact) | none |
| 6. Validation / dishonest-path audit | 6.1 [x] | DONE (artifact; 6.1 audit record in §Notes) | none |
**Bottom line: 0 deferred follow-ups remain as core gaps. The "deferred follow-ups in §Implementation Notes" referenced by the roadmap are advisory/out-of-scope items, not open tasks** — see the genuine residuals below; all are explicitly out-of-spec-boundary and owned elsewhere.
### Genuine Residuals (advisory, all out-of-boundary or already resolved downstream)
These are the §Implementation Notes "deferred follow-ups". None blocks CLOSED status; each is owned by a later spec or is cosmetic.
1. **LSN-0 first-entry legacy edge** (Notes 5.1, 6.1 audit). The first-ever WAL entry (LSN 0) routes to the legacy branch (persist.c:1903-1929) because `commit_submit` rejects `lsn==0`. — **RESOLVED downstream:** ha-single-leader-role's promotion no-op entry makes LSN 0 never a client write in practice; repl.c:438-440 now confirms LSN 0 immediately. Once-per-cluster-lifetime, no consistency loss. *Effort: 0 (closed). Risk: none.*
2. **`ce->n_peers` not re-derived on dynamic membership change** (Notes 6.1(b)). If `ha->n_peers` is reassigned post-init (e.g. ha.c RDHUP re-election), the engine's `n_peers` is stale. — **Out of boundary** (dynamic membership = future work); current cluster topology is fixed at init. *Option A: assert n_peers stability at re-election callback; Option B: re-derive `ce->n_peers` in a `commit_set_n_peers` setter called from the election path; Option C: leave as documented latent gap. Effort: A ~0.5d, B ~1d (+revalidate barrier-rollout/durability). Risk: LOW today (no dynamic membership in production); MEDIUM if dynamic reconfiguration is ever added without addressing it.*
3. **Pre-existing ASAN leaks in 6 test groups** (Notes 6.1 LEAK FOLLOW-UP: worker_join 33KB, rdhup_integration, failover, start_election_async, liveness, connection_reuse). — **Out of scope** (pre-existing, production-side allocations; suite runs with `detect_leaks=0`). *Option: dedicated leak-cleanup pass. Effort: ~1-2d. Risk: LOW (test-harness hygiene, not a correctness gap).*
4. **Stale comment persist.c:1151-1154 / pre-engine contract text** (Notes 6.1). One-line doc follow-up. *Effort: trivial. Risk: none.*
5. **Untracked `src/lib/testsuite/test_wal_wire.c`** referenced by committed meson.build (Notes 5.1). Repo hygiene. *Effort: trivial (git add). Risk: none.*
### Drift Note (informational)
The engine source has been extended by **ha-commit-durability** (implemented 2026-06-18) AFTER ha-commit-engine: `majority_candidate_locked` now caps the self-contribution at `persist_durable_lsn` (commit.c:93) rather than the appended tip, and a `commit_on_durable_advance` flusher wake was added (commit.c:218). This is additive — it tightens Req 2.3 (committed never beyond durable, not merely never beyond appended) and closes bug-661. The ha-commit-engine contract is unchanged and remains fully satisfied. Original research-table anchors (e.g. repl.c:432, persist.c:1035, ha.c:1507) have drifted; verified-current anchors are in the Requirement-to-Asset Map above.