myxce/.kiro/specs/ha-commit-engine/tasks.md

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

• 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

• 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

• 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

• 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

• 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

• 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

• 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

• 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.
• (5.1) LSN-0 edge for 5.2/5.3 (reviewer-verified): commit_submit rejects lsn==0 with -EINVAL (commit.c:229), but the FIRST-EVER WAL entry is LSN 0 (current_lsn starts at 0; written_lsn = current_lsn - 1 at persist.c:1020). When 5.3 passes written_lsn, the first sync append on a fresh cluster would get -EINVAL. 5.3 must handle written_lsn==0 (options: treat entry 0 as valid in the engine, or shift LSN numbering, or special-case — decide per design). Tests 32-34 sidestep via a priming async append; production must not.
• (5.1) persist.c currently passes entry_lsn=0 to the adapter — if 5.2 lands walAck delegation before 5.3 passes written_lsn, the wired sync path returns -EINVAL. Land 5.2's delegation gated on ce!=NULL AND entry_lsn!=0 falling back to legacy, OR land 5.2+5.3 wiring in lockstep.
• (5.1) Baseline failure root causes identified: test_committed_lsn_does_not_advance_on_timeout (test_repl_unit.c:294) expects the OLD contract (rc==0 + sync_rc out-param) but persist_wal_sync_append now rejects-on-timeout — STALE TEST EXPECTATION, fix the test at 6.1, not the code. test_p2_integration.c:test_single_node_committed_lsn_unchanged pins the -ENODEV degraded path that 5.3 replaces — update it at 5.3/6.1.
• (5.1) Repo hygiene (pre-existing, out of scope): untracked src/lib/testsuite/test_wal_wire.c is referenced by committed src/lib/meson.build:176 — should be added to git eventually.
• (5.2) commit.c linked into production controller_src ONE TASK EARLY (meson.build:3) — necessary (repl.c references engine symbols) and reviewer-verified inert (zero production callers of commit_engine_init/repl_set_commit_engine until 5.3; repl_init memsets ce=NULL → all new paths dormant).
• (5.2) repl_broadcast's over-limit eviction (in_catch_up=1, repl.c:268-277) deliberately does NOT feed commit_on_membership_change — reviewer-proven no-op for waiter correctness (shrink can never raise the majority candidate; monotonic store). Design lists exactly four mutators; eviction recovery fires the hook at repl_complete_catchup.
• (5.2) r->ce is read unlocked under a set-once-before-traffic contract (repl.h:47-52). 5.3 must wire it at cluster init BEFORE replication traffic; never NULL it concurrent with traffic (fini ordering is 5.3's responsibility).
• (5.2) Remaining RED going into 5.3: test 33 (Expected -110 Was 0 — persist passes entry_lsn=0 → legacy fallback) and test 34 (Expected 0 Was -19 — -ENODEV special case). These plus test 32's honest-delegation green are 5.3's completion evidence. [RESOLVED at 5.3: both green]
• (5.3) For 6.1 audit: (a) baseline failure #1 (test_committed_lsn_does_not_advance_on_timeout) PHYSICALLY lives in test_p2_integration.c:277 — Unity misattributes it across groups (bug-411 artifact); its root cause is a stale expectation (old rc==0+sync_rc contract). (b) Legacy cluster_init() wires the engine with n_peers==0 (self-quorum) — confirmed test-only (zero production callers; production = myapp_run two-phase), but ce->n_peers is never re-derived if ha->n_peers is reassigned post-init (ha.c:3484 RDHUP re-election) — latent gap for future dynamic membership; consider an assert or re-derivation at the first-boot election callback. (c) Pre-existing teardown race (not 5.3's): persist_fini runs before vtaskmgr_fini in cluster_fini.
• (5.3) cluster_t lives in src/include/xce/cluster_def.h (not cluster.h); engine embedded there. Init point: cluster_init_phase2 after vtaskmgr guard, idempotence-guarded by c->commit.p==NULL (phase2 can run twice). n_vcpus = c->vtaskmgr.n_vcpus (resolved).
• (6.1) AUDIT RECORD (reviewer-confirmed): delegation guard repl.c:435-437 (is_wal_ack && ce && entry_lsn!=0 → commit_submit); legacy acked_lsn>0 counting (repl.c:469) and n_active==0→success (repl.c:482-484) strictly below it, DEPRECATED-marked, walAck-reachable only when ce==NULL || entry_lsn==0; persist-side mirror guard persist.c:1037 with DEPRECATED legacy branch (1044-1064); production always wires ce at cluster_init_phase2 (cluster.c:1134-1148) — sole production legacy exposure = documented LSN-0 first entry (once per cluster lifetime; eliminated by ha-single-leader-role no-op); configAck never delegates (is_wal_ack false; barrier-rollout hardens that track). Census: 46 groups / 467 tests / 0 failures (standalone default-ASAN per group; meson harness uses pre-existing detect_leaks=0 from commit 8468a83). Baseline fixes: stale timeout expectation → reject-on-timeout contract (engine-wired fixture); 2 gate fixtures honestly populated with standby_endpoints. LEAK FOLLOW-UP (out of spec scope): 6 groups leak production-side allocations under default ASAN — worker_join 33,648B/357; rdhup_integration 1,776B/24 (ha.c:3457 elect_ctx deliberate, ha.c:2293/2299 delaymgr objects); failover 232B/2; start_election_async 272B/1; liveness 216B/5; connection_reuse 48B/2. Stale comment persist.c:1151-1154 (pre-engine contract text) — one-line follow-up.