# Implementation Plan: ha-commit-engine

> TDD ordering per project steering (`testing.md`): every implementation sub-task is preceded by a testsuite sub-task that lands failing (RED) tests first. All new tests live in `testsuite/test_commit_engine.c` (brief-scoped single test file) plus targeted updates to `test_repl_majority.c` — this shared-file reality is why no `(P)` markers are applied: the test file and `repl.c` are touched by multiple groups, so the plan is sequential.
> Verify exact line numbers with grep before editing (research.md table has the verified anchors; files drift).

- [ ] 1. Commit engine core: per-LSN evaluation and commit-index integrity
- [x] 1.1 Write RED tests for per-LSN majority evaluation and commit-index integrity
  - Create `testsuite/test_commit_engine.c` against a stub `persist_t` + `replicator_t` (XCE_TESTING pattern, AAA structure) and register it in `controller_unit_test_src` (meson.build); create `src/controller/include/commit.h` with the umbrella-verbatim interface (types + prototypes only) so tests compile
  - Tests: with n_peers=3, a standby ACK for LSN M does not commit entry N>M (kills `acked_lsn > 0` counting); ACK watermark >= N from one standby plus self commits N; cumulative/out-of-order watermarks advance `committed_lsn` to the majority median, never past the highest appended LSN, never backwards; one far-lagging standby does not block majority commit; RETAIN mode returns 0 immediately without blocking
  - Observable completion: build succeeds with the new header and test file; all new test cases FAIL (red) against a stub `commit.c` or unlinked implementation
  - _Requirements: 1.2, 2.1, 2.2, 2.3, 5.1_

- [x] 1.2 Implement the commit engine core to green
  - Create `src/controller/commit.c` (add to meson controller sources): `commit_engine_init`/`commit_engine_fini` (validate args, one mutex + one condvar), `commit_on_ack` (bounded scan of active standbys' `acked_lsn` into the scratch array, sort descending, candidate = `arr[n_peers/2 - 1]` clamped to appended log, monotonic `max()` store into `persist->committed_lsn`, condvar broadcast — never blocks), `commit_submit(RETAIN)` immediate return
  - Catch-up standbys (`in_catch_up`) excluded from the evaluation count (umbrella-locked conservative rule)
  - XCE_TESTING asserts for monotonicity
  - Observable completion: all task 1.1 tests PASS; engine is the only `committed_lsn` writer in `commit.c` paths
  - _Requirements: 1.2, 2.1, 2.2, 2.3, 5.1_

- [ ] 2. WAIT mode, self-quorum, zero-peer rule, admission cap, own-term gate storage
- [x] 2.1 Write RED tests for blocking-wait semantics and admission control
  - Tests in `test_commit_engine.c`: WAIT blocks until majority ACK then returns 0; WAIT returns `-ETIMEDOUT` at deadline with `committed_lsn` unmoved past the entry; n_peers=1 → WAIT returns 0 immediately and `committed_lsn` advances to the entry; n_peers=3 with zero standby ACKs → `-ETIMEDOUT`, never success (short-circuit regression); with n_vcpus=8 and reserved=2, a 7th concurrent WAIT gets `-XCE_ERR_NOT_READY` while the 6 admitted waits proceed; cap floor `max(1, n_vcpus - reserved)` holds for tiny n_vcpus; while the cap is saturated, `commit_on_ack` still advances `committed_lsn` and completes admitted waiters; `writable_lsn` unset (0) → no gating; `commit_set_writable_lsn(G)` with G > entry → waiter completes only once `committed_lsn >= G`
  - Observable completion: new cases FAIL (red) against the task-1 engine
  - _Requirements: 1.1, 4.3, 4.4, 6.3, 7.1, 7.2, 7.3_

- [x] 2.2 Implement WAIT mode, admission counter, and writable_lsn gate to green
  - `commit_submit(WAIT)`: admission check (`inflight_waits >= max(1, n_vcpus - COMMIT_WAIT_RESERVED)` → `-XCE_ERR_NOT_READY`, distinct log message), `pthread_cond_timedwait` loop with per-LSN predicate (`committed_lsn >= lsn && (writable_lsn == 0 || committed_lsn >= writable_lsn)`), `-ETIMEDOUT` on deadline; `n_peers <= 1` immediate self-quorum success advancing `committed_lsn`; `commit_set_writable_lsn` stores the gate (no caller in this spec)
  - XCE_TESTING assert: inflight counter within cap and never negative
  - Observable completion: all task 2.1 tests PASS; cap is expressed only relative to the `n_vcpus` init argument (no absolute constant other than the reserved headroom)
  - _Requirements: 1.1, 4.3, 4.4, 6.3, 7.1, 7.2, 7.3_

- [ ] 3. Membership-change re-evaluation
- [x] 3.1 Write RED tests for membership re-evaluation (completed jointly with 3.2 — RED unachievable, see Implementation Notes)
  - Tests in `test_commit_engine.c`: a catch-up standby's ACK watermark does not count toward a pending entry; after clearing `in_catch_up` and calling `commit_on_membership_change`, the pending waiter completes if the standby's watermark covers it; on standby removal that makes majority unreachable, a pending WAIT never returns success and fails with `-ETIMEDOUT` at its deadline; `commit_on_membership_change` returns without blocking
  - Observable completion: new cases FAIL (red)
  - _Requirements: 4.1, 4.2_

- [x] 3.2 Implement commit_on_membership_change to green (verified no-op — task 1.2 already implemented the full contract; tests 17-20 are the green evidence)
  - Re-run the commit evaluation under the engine mutex against current registry state and broadcast the condvar so all pending waiters re-check their predicates; no forced early-fail (reconnection within the deadline may still commit — design decision)
  - Observable completion: all task 3.1 tests PASS
  - _Requirements: 4.1, 4.2_

- [ ] 4. Quorum-accounting fix: catch-up exclusion at the dispatch gate
- [x] 4.1 Write RED tests for write-admission quorum counting
  - Tests: `repl_active_standby_count` counts only slots with `ep != NULL && !in_catch_up`; `ha_has_write_quorum` with n_peers=3 returns false when the only connected standby is in catch-up, true with one active standby, false with zero connected; after simulated reconnect (registry restored) the gate accepts again with no reset call (stateless re-evaluation); n_peers=1 always true
  - Declare `repl_active_standby_count` in src/include/repl.h so tests compile
  - Observable completion: new cases FAIL (red) — current `ha_has_write_quorum` counts `standby_endpoints->count` including catch-up (grep ha.c for the current anchor near ha.c:1507)
  - _Requirements: 3.1, 3.2, 3.3, 6.1, 6.2_

- [x] 4.2 Implement the quorum-accounting fix to green (incl. reviewer-mandated wired gate test 29; reviewed in main context — reviewer subagent unavailable)
  - Add `repl_active_standby_count(replicator_t *r)` to repl.c (under `r->lock`); add `replicator_t *repl` back-pointer to `ha_state_t` (ha.h); `ha_has_write_quorum` uses the helper when the back-pointer is wired, falls back to the existing `standby_endpoints` count when NULL (legacy test contexts); quorum condition itself unchanged (`active >= n_peers / 2`, `n_peers <= 1 → true`)
  - Observable completion: all task 4.1 tests PASS; existing dispatch-gate tests still green
  - _Requirements: 3.1, 3.2, 3.3, 6.1, 6.2_

- [ ] 5. Integration: wire the engine behind the job-submit sync path
- [ ] 5.1 Write RED integration tests for the ACK feed and the sync-append path
  - Tests in `test_commit_engine.c`: `repl_on_ack` with a wired engine updates `acked_lsn` AND triggers commit evaluation (majority → `committed_lsn` advances, waiter wakes); `repl_start_catchup`/`repl_complete_catchup`/`repl_remove_standby`/`repl_add_standby` trigger re-evaluation; `persist_wal_sync_append` with wired engine returns 0 only on per-entry majority, returns `-ETIMEDOUT` when standbys' `acked_lsn` covers only an EARLIER entry (stale-ACK regression — connectivity/old ACKs never substitute), leaves `committed_lsn` unmoved on timeout, and for n_peers=1 returns 0 with `committed_lsn` advanced (replaces the old `-ENODEV` degraded path)
  - Update `testsuite/test_repl_majority.c` walAck cases to wire an engine and assert per-LSN semantics; configAck cases byte-identical
  - Observable completion: new/updated cases FAIL (red) against unwired production code
  - _Requirements: 1.1, 1.2, 1.3, 2.1, 4.2, 4.4, 6.3_

- [ ] 5.2 Implement the repl-side wiring: ACK feed, membership hooks, adapter
  - repl.h: forward-declare `struct commit_engine`, add `ce` pointer to `replicator_t`, declare `repl_set_commit_engine()`; add `uint64_t entry_lsn` parameter to `repl_wait_majority_ack` (update the config_repl.c:916 caller to pass 0 — configAck branch behavior untouched)
  - repl.c: `repl_on_ack` calls `commit_on_ack(r->ce, slot_idx, acked_lsn)` after the `acked_lsn` store when wired; `repl_add_standby`/`repl_remove_standby`/`repl_start_catchup`/`repl_complete_catchup` call `commit_on_membership_change` when wired; walAck branch of `repl_wait_majority_ack` delegates to `commit_submit(ce, entry_lsn, COMMIT_MODE_WAIT, timeout_ms)` when wired, with the legacy counting kept only for `ce == NULL` test contexts and marked deprecated (retired by ha-commit-barrier-rollout)
  - Observable completion: ACK-feed and membership-hook tests from 5.1 PASS; configAck tests unaffected
  - _Depends: 2.2, 3.2_
  - _Requirements: 1.1, 4.2_

- [ ] 5.3 Implement engine instantiation and the sync-append path switch
  - cluster.h/cluster.c: embed `commit_engine_t commit` in `cluster_t`; call `commit_engine_init(&c->commit, &c->persist, n_peers, n_vcpus)` + `repl_set_commit_engine` at the init point where both the vtask pool size (cluster.c:1082 anchor) and `ha->n_peers` are final; set the `ha_state_t.repl` back-pointer here; `commit_engine_fini` on teardown
  - persist.c `persist_wal_sync_append`: pass `written_lsn` to the adapter; on the engine-wired path remove the `has_standbys → -ENODEV` special case and the `committed_lsn` store (persist.c:1035 anchor — the engine now owns advancement; truncation store at persist.c:1438 stays); return contract to callers: 0 / `-ETIMEDOUT` / `-XCE_ERR_NOT_READY` propagated through the existing job-submit error path
  - Observable completion: all task 5.1 tests PASS, including stale-ACK regression and single-node `committed_lsn` advancement; job submit on a wired cluster confirms only on per-entry majority
  - _Requirements: 1.1, 1.2, 1.3, 2.1, 4.4, 6.3_

- [ ] 6. Validation: full regression and dishonest-path audit
- [ ] 6.1 Run the full controller regression and audit the legacy paths
  - Build and run the complete controller testsuite with ASAN (`/xce-build controller`); fix any regression introduced by the short-circuit removal or the `committed_lsn` ownership move (existing persist/repl/ha suites)
  - Audit: grep confirms no production (engine-wired) walAck path reaches the `acked_lsn > 0` counting or the `n_active == 0 → success` branch; both survive only under the deprecated `ce == NULL` test fallback
  - Observable completion: full testsuite green under ASAN; audit results recorded in the task completion notes
  - _Requirements: 1.1, 1.2, 1.3, 2.1, 2.2, 2.3, 3.1, 3.2, 3.3, 4.1, 4.2, 4.3, 4.4, 5.1, 6.1, 6.2, 6.3, 7.1, 7.2, 7.3_

## Implementation Notes

- (1.1) THREE PRE-EXISTING suite failures, masked until now (bug-411): test_committed_lsn_does_not_advance_on_timeout (-110), test_repl_ack_after_promote_gate_allows_jobsub (-10013), test_rdhup_primary_dispatch_gate_opens (-10013). Masking cause: each test group runs its own UNITY_BEGIN/END so the process exit code reflects only the LAST group. They live in repl/persist sync-append and quorum-gate paths — tasks 1.2/2.x/5.x touch those areas and 6.1 requires full green, so they must be fixed (or proven fixed by the engine) along the way.
- (1.1) Full-suite output under default ASAN dies at exit via LeakSanitizer (pre-existing leaks in worker_join/rdhup_integration/start_election_async/liveness groups) without flushing stdout — use ASAN_OPTIONS=detect_leaks=0 for full-run logs or run groups standalone with -t.
- (1.1) repl_add_standby returns the SLOT INDEX, not 0 (bug-410 note for fixture writers).
- (1.1) commit.c is registered ONLY in controller_unit_test_src for now; production target linkage lands with task 5.3.
- (2.1) WAIT/cap/gate tests (8-16) read `g_ce.inflight_waits` under `g_ce.lock` — task 2.2 must keep that field accurate while waiters block. Timing floors have ~20ms slack vs sleeps (reviewer advisory: watch for flake on loaded boxes). Unity assertions are main-thread-only after joins — keep that discipline in future threaded tests.
- (3.1/3.2) Joint completion: task 1.2's commit_on_membership_change (commit.c:334-346) already implemented the full 3.2 contract, making 3.1's RED unachievable without dishonest tests. Tests 17-20 landed green as regression coverage; reviewer confirmed mutation-killing power (each test fails against the relevant mutant) and zero production changes. Lesson for later RED tasks: when a prior task's "minimal" allowance fully implements a later contract, complete the pair jointly under review rather than weakening tests.
- (4.1) REVIEWER ADVISORY for 4.2: the gate-side fix (ha_state_t.repl back-pointer + catch-up exclusion through ha_has_write_quorum) has NO RED test — 4.1's tests would all pass even if 4.2 only implemented the helper. 4.2 MUST add a wired gate-level case (n_peers=3, catch-up-only standby → NOT_READY via ha_dispatch_gate) as its own green evidence. Tests 26-28 pin the NULL-back-pointer legacy fallback and n_peers<=1 contracts 4.2 must preserve. Stub repl_active_standby_count returns -1 (repl.c:557) — replace, don't wrap.