From 1ca2d640cb15a3fa95e6c967cde67bbd48a890cd Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 18 Jun 2026 04:46:01 +0000 Subject: [PATCH] feat(ha-commit-durability): add --wal-sync-ms flush-cadence override (task 1.1) --- .kiro/specs/ha-commit-durability/tasks.md | 123 ++++++++++++++++++ src/controller/cluster.c | 5 + src/controller/main.c | 13 ++ src/controller/myapp.c | 5 + .../testsuite/test_cluster_persist.c | 41 ++++++ src/include/myconf.h | 1 + src/include/persist.h | 18 +++ 7 files changed, 206 insertions(+) create mode 100644 .kiro/specs/ha-commit-durability/tasks.md diff --git a/.kiro/specs/ha-commit-durability/tasks.md b/.kiro/specs/ha-commit-durability/tasks.md new file mode 100644 index 00000000..5a24a67e --- /dev/null +++ b/.kiro/specs/ha-commit-durability/tasks.md @@ -0,0 +1,123 @@ +# Implementation Plan + +> TDD is mandatory (project rule): write the failing test in `testsuite/` before the C change (red → green → refactor). +> Re-verify HA changes against `raft/raft.tla` and the `src/controller/CLAUDE.md` Protocol Verification Checklist. +> Most core tasks edit the same correctness-critical files (`persist.c`, `commit.c`, `ha.c`, `cluster.c`) and are therefore sequential by design; only genuinely file-disjoint analysis is marked `(P)`. + +- [ ] 1. Foundation: durability verification seam and deterministic reproduction +- [x] 1.1 Add a configurable flush-cadence override + - Parse a `--wal-sync-ms` controller flag (following the existing `--sync-timeout-ms` convention) into the persistence config before persistence starts; default behavior unchanged. + - Observable: starting a controller with `--wal-sync-ms ` sets the flush cadence to N; omitting it preserves the current default. + - _Requirements: 3.1_ + - _Boundary: DurabilityConfig_ + +- [ ] 1.2 Add the test-only durability-verification hook + - Add an `XCE_TESTING`-gated seam that lets a test assert an acknowledged operation is recoverable after an immediate ungraceful termination; compiled out of production builds. + - Observable: in a test build the hook is callable and the production build links without it. + - _Requirements: 3.2, 3.3_ + - _Boundary: DurabilityTestSeam_ + +- [ ] 1.3 Drive a deterministic durability reproduction from the soak harness + - Extend `test-commit-durability.sh` with a deterministic mode that widens the batching window / uses the seam and kills a durable contributor in-window. + - Observable: on the current (unfixed) build the deterministic mode reports DATA LOSS (RED), proving bug-661 without relying on race timing. + - _Requirements: 3.1, 3.2_ + - _Depends: 1.1, 1.2_ + +- [ ] 2. Durable commit mechanism (acknowledgment ⇒ majority persistence) +- [ ] 2.1 Introduce durable-LSN tracking via a group-commit flusher + - Repurpose the background fsync thread from a timer into a demand-driven flusher: an append signals it; it performs one `fdatasync` over all pending appends, advances a durable-LSN watermark, and wakes the commit evaluation. + - Guard against lost wakeups: the signal is level-triggered on the appended-vs-synced gap so an entry is never considered durable before its `fdatasync` (gating item). + - Observable: a unit test shows one `fdatasync` advances the durable-LSN past several appended LSNs under load, and the durable-LSN never exceeds the synced tip. + - _Requirements: 1.1, 1.6_ + - _Boundary: DurableCommit_ + +- [ ] 2.2 Count self toward the commit majority only when durable + - Change the commit-majority evaluation so the local member's self-contribution is capped at the durable-LSN watermark, not the appended tip; clamp the watermark on WAL truncation. + - Observable: a unit test shows `committed_lsn` does not advance over an entry until that entry is durable locally, and the self-cap equals the durable-LSN. + - _Requirements: 1.2, 1.4_ + - _Boundary: DurableCommit_ + +- [ ] 2.3 Pin standby persist-before-acknowledge with a regression test + - Add a regression test asserting the standby apply path `fdatasync`s a replicated entry before sending its acknowledgment. + - Observable: the test fails if the standby ACK is emitted before the durable write and passes against the current behavior. + - _Requirements: 1.3_ + - _Boundary: DurableCommit_ + +- [ ] 2.4 Reconcile the dormant synchronous-append commit path + - The synchronous-append path advances the commit position directly on majority acknowledgment and is built on the premise that the append already fsync'd inline — a premise group-commit removes. Confirm it has no production caller and delete it, or route it through the same durable-majority evaluation so it can never advance the commit position on a non-durable entry. + - Observable: a code search shows no path advances the commit position on majority ACK without the durable precondition; the dormant path is removed or re-routed, and a test guards against a future caller reintroducing an ACK-without-durability commit. + - _Requirements: 1.1, 1.2_ + - _Boundary: DurableCommit_ + - _Depends: 2.1_ + +- [ ] 3. Recovery integrity (no silent truncate; durable floor) +- [ ] 3.1 Return a recovery integrity verdict and mark a truncated member non-promotable + - Make WAL replay distinguish a clean log end from a corruption-stopped end (CRC chain break / mismatch / short read) and surface that verdict; a corruption-truncated recovery is never reported as a clean recovery and sets a member-level "recovery-truncated" flag that the promotion gate (4.3) consumes. + - Observable: a unit test injecting a CRC break at the first entry yields a TRUNCATED verdict and sets the non-promotable flag, while an intact log yields CLEAN; an empty first-boot WAL is CLEAN (not corruption). + - _Requirements: 2.1_ + - _Boundary: RecoveryIntegrity_ + +- [ ] 3.2 Pin the fail-loud vs fence boot policy as a decision with a verification table + - Pin the concrete policy (gating item, decision only — no behavior change yet): a truncated member fences from leadership at boot, attempts catch-up from a healthy peer, and terminates loudly only on a bounded failure to reach a committed-consistent state — never a silent indefinite fence. Encode the decision as a table of input states → expected outcome. + - Observable: the policy and its decision table are recorded, and skeleton tests assert each row's expected outcome (initially failing), establishing the contract 3.3 and 4.3 implement. + - _Requirements: 2.2, 2.4_ + - _Boundary: RecoveryIntegrity_ + +- [ ] 3.3 Implement loud FATAL on bounded-unrecoverable recovery + - Implement the FATAL branch of the 3.2 policy: when a recovery-truncated member cannot reach a committed-consistent state within the bounded catch-up window, terminate loudly with the truncation boundary logged; a reachable healthy peer never triggers FATAL. + - Observable: the 3.2 decision-table rows for the FATAL/no-FATAL cases pass; logs show the truncation boundary on FATAL. + - _Requirements: 2.4_ + - _Depends: 3.1, 3.2_ + - _Boundary: RecoveryIntegrity_ + +- [ ] 3.4 Re-derive the durable floor from the recovered WAL tip + - On recovery, publish the member's durable floor as the highest intact replayed LSN so later advertisement is backed by durable state rather than a volatile in-memory position. + - Precision: this floor is the durable **last-log index** (WAL tip), used by the Raft freshness gate — not the commit position; the post-election safe-commit point continues to come from voter replies, not from this floor. + - Observable: a unit test shows that after a truncating restart the published durable floor equals the highest intact LSN (the WAL tip), not a stale higher in-memory value or the commit position. + - _Requirements: 2.3_ + - _Boundary: RecoveryIntegrity_ + +- [ ] 4. Promotion fence and durable-backed freshness +- [ ] 4.1 (P) Pin the freshness-floor formula and re-verify election safety and liveness + - Define the exact reconciliation of the durable-derived freshness floor against the existing in-memory floor — the durable quantity is the durable **last-log index** (WAL tip), not the commit position — then re-check it against `raft/raft.tla` for Leader Completeness AND liveness (no election livelock) plus the Protocol Verification Checklist (gating item; analysis only, no shared source files). + - Scope of the raft.tla check: it validates the **election/freshness logic** (the `mlastLogIndex`/`LastTerm` semantics this change touches), which raft.tla models directly. It does NOT validate the *durability* of that floor — raft.tla assumes a durable log (see 5.3). The durability of the WAL-tip derivation is verified by tests (3.4), not by raft.tla. + - Observable: the pinned formula is recorded and the model/checklist verification passes for both a truncated-candidate-denied case and a longest-durable-log-still-elects case. + - _Requirements: 2.3_ + - _Boundary: FreshnessFloor_ + +- [ ] 4.2 Derive the vote and PreVote freshness floor from durable WAL state + - Implement the pinned formula so a candidate cannot advertise, and a voter cannot grant against, a position beyond durable WAL state; the post-election safe-commit point continues to come from voter replies, never a stale ctrlhello. + - Observable: a node restarted with a truncated log is denied (vote and PreVote), while the node holding the longest durable log still wins — no livelock. + - Note: 4.2 and 4.3 both edit the same election code (`cluster.c` vote/PreVote and `ha.c` promotion); they are sequential (4.2 before 4.3), not parallel. + - _Requirements: 2.3_ + - _Depends: 4.1, 3.4_ + +- [ ] 4.3 Add the pre-promotion eligibility gate and fence release + - Before a member becomes primary, refuse promotion if it carries the recovery-truncated flag (from 3.1) or its durable position is below the cluster committed high-water mark; extend the existing catch-up fence to cover the promoting/recovered member and release it when catch-up passes the last committed boundary. Reuse the durable-LSN truncate-clamp from 2.2 so the gate evaluates a clamped durable position at the promotion-path truncate site. + - Observable: a recovery-truncated or behind member is denied promotion until catch-up completes, then regains eligibility; a hollow node cannot win even at minimal quorum. + - _Requirements: 2.2, 2.5, 2.6_ + - _Depends: 4.2, 3.1, 3.2, 2.2_ + +- [ ] 5. Integration and validation +- [ ] 5.1 Wire durable-majority commit end to end + - Connect the durable-LSN self-count and the commit barrier so the client reply is released only after the entry is durable on a majority; assert liveness (the flusher does not stall heartbeat/election handling). + - Observable: an integration test confirms reply-ok only after durable-majority, killing the sole durable contributor mid-commit leaves the acknowledged op present on the survivors, and the full controller suite passes under ASAN. + - _Requirements: 1.1, 1.4, 1.5, 1.6, 2.5_ + - _Depends: 2.2, 4.2_ + +- [ ] 5.2 Prove zero acknowledged-job loss across failover + - Run the deterministic durability E2E and the rolling SIGKILL+restart soak; both must show every acknowledged job surviving every failover. + - Observable: the deterministic mode that was RED in 1.3 now passes (GREEN), and the soak reports zero DATA LOSS across rounds. + - _Requirements: 1.1, 1.5, 2.2, 2.3, 2.6_ + - _Depends: 5.1, 4.3, 1.3_ + +- [ ] 5.3 Re-verify the formal safety checklist and flip the durability gap note + - Re-run the `raft/raft.tla` alignment and the Protocol Verification Checklist (commit rule, leader completeness, `commitIndex` monotonic) against the integrated implementation. + - IMPORTANT — raft.tla does NOT model durability: it restores `log` intact from "stable storage" on restart (`raft.tla:165-174`), i.e. it assumes what bug-661 violates. A passing TLC/alignment run on the unmodified spec therefore does NOT prove the commit-durability invariant. That invariant is verified by the unit/integration/E2E tests (2.x, 5.1, 5.2) plus the manual checklist — not by raft.tla. Do not conclude durability is proven from raft.tla alone. Formal durability proof would require extending raft.tla with a durable-vs-volatile log model (out of scope unless explicitly opted in). + - Update `src/controller/CLAUDE.md`: convert the "Commit Durability — KNOWN GAP (bug-661)" note into the now-true guarantee and tick the post-`ha-commit-durability` checklist item. + - Observable: the checklist passes with the durable self-count and the durable-backed freshness floor in place; the durability invariant is backed by passing tests (not by raft.tla); the CLAUDE.md gap note is removed; any deviation is recorded. + - _Requirements: 1.2, 2.3_ + - _Depends: 5.1_ + +## Implementation Notes +- 1.1: Config plumbing routes through `myconf_t.wal_sync_ms` + `persist_config_set_wal_sync_ms()` (0-sentinel = keep `DEFAULT_WAL_SYNC_MS`), NOT a new config.h constant (design File Structure Plan keeps `DEFAULT_WAL_SYNC_MS`). The override is applied at BOTH `persist_init` sites — `myapp_run` step-6 (production path) and `cluster_init_phase2` (guarded by `wal_path[0]=='\0'`, skipped in production). They are mutually exclusive; apply any new persist_config override at both sites or it is silently dropped. Pre-existing latent bug (do NOT fix here): `--sync-timeout-ms` is applied only in phase2, so it is ignored on the production startup path — relevant if a later task relies on `sync_timeout_ms`. diff --git a/src/controller/cluster.c b/src/controller/cluster.c index f17d31a5..2ebcd723 100644 --- a/src/controller/cluster.c +++ b/src/controller/cluster.c @@ -1270,6 +1270,11 @@ int cluster_init_phase2(cluster_t* c) if (c->myconf->sync_timeout_ms > 0) pcfg.sync_timeout_ms = c->myconf->sync_timeout_ms; + /* --wal-sync-ms flush-cadence override (0 = keep the persist + * default). Applied before persist_init so persistence starts at + * the requested cadence (ha-commit-durability Req 3.1). */ + persist_config_set_wal_sync_ms(&pcfg, c->myconf->wal_sync_ms); + persist_set_workdir(&c->persist, workdir, name); rc = persist_init(&c->persist, &pcfg, c); if (rc != 0) { diff --git a/src/controller/main.c b/src/controller/main.c index ab346fbc..fc1fc860 100644 --- a/src/controller/main.c +++ b/src/controller/main.c @@ -30,6 +30,7 @@ const char* argp_program_bug_address = BUG_EMAIL; #define OPT_VCPUS 259 #define OPT_CERTDIR 260 #define OPT_SYNC_TIMEOUT 261 +#define OPT_WAL_SYNC 262 struct arguments { char* myname; @@ -42,6 +43,7 @@ struct arguments { int vcpus; char* certdir; uint32_t sync_timeout_ms; + uint32_t wal_sync_ms; }; static struct argp_option options[] = { @@ -62,6 +64,8 @@ static struct argp_option options[] = { "Directory containing xce.crt and xce.key (default: /certs)" }, { "sync-timeout-ms", OPT_SYNC_TIMEOUT, "MS", 0, "Synchronous majority-replication commit barrier timeout in ms (0 = default)" }, + { "wal-sync-ms", OPT_WAL_SYNC, "MS", 0, + "WAL flush (fsync) cadence in ms (0 = default)" }, { 0 } }; @@ -111,6 +115,13 @@ static error_t parse_opt(int key, char* arg, struct argp_state* state) arguments->sync_timeout_ms = (uint32_t)v; break; } + case OPT_WAL_SYNC: { + int v = atoi(arg); + if (v < 0) + argp_error(state, "--wal-sync-ms must be >= 0 (got: %s)", arg); + arguments->wal_sync_ms = (uint32_t)v; + break; + } default: return ARGP_ERR_UNKNOWN; } @@ -163,6 +174,7 @@ int main(int argc, char* argv[]) arguments.conf_stale_timeout = 86400; arguments.vcpus = 0; /* 0 = not set; use cluster config default */ arguments.sync_timeout_ms = 0; /* 0 = not set; use persist config default */ + arguments.wal_sync_ms = 0; /* 0 = not set; use persist config default */ argp_parse(&argp, argc, argv, 0, 0, &arguments); @@ -212,6 +224,7 @@ int main(int argc, char* argv[]) myconf.conf_stale_timeout = (time_t)arguments.conf_stale_timeout; myconf.vcpus = arguments.vcpus; myconf.sync_timeout_ms = arguments.sync_timeout_ms; + myconf.wal_sync_ms = arguments.wal_sync_ms; memset(&myconf.peers_cfg, 0, sizeof(myconf.peers_cfg)); if (arguments.peers_str) { diff --git a/src/controller/myapp.c b/src/controller/myapp.c index 940dbabe..4ecfc626 100644 --- a/src/controller/myapp.c +++ b/src/controller/myapp.c @@ -163,6 +163,11 @@ int myapp_run(myconf_t *myconf) : "controller"; persist_config_t pcfg = persist_config_defaults(); + /* --wal-sync-ms flush-cadence override (0 = keep the persist + * default). Applied before persist_init so persistence starts at + * the requested cadence (ha-commit-durability Req 3.1). */ + persist_config_set_wal_sync_ms(&pcfg, myconf->wal_sync_ms); + persist_set_workdir(&c.persist, workdir, name); rc = persist_init(&c.persist, &pcfg, &c); if (rc != 0) { diff --git a/src/controller/testsuite/test_cluster_persist.c b/src/controller/testsuite/test_cluster_persist.c index 678045af..95a6cb95 100644 --- a/src/controller/testsuite/test_cluster_persist.c +++ b/src/controller/testsuite/test_cluster_persist.c @@ -68,6 +68,44 @@ static void test_persist_config_defaults_sync_timeout_ms(void) TEST_ASSERT_EQUAL_UINT32(DEFAULT_SYNC_TIMEOUT_MS, cfg.sync_timeout_ms); } +/* ------------------------------------------------------------------ */ +/* --wal-sync-ms flush-cadence override (ha-commit-durability task 1.1) */ +/* */ +/* persist_config_set_wal_sync_ms() is the single override point shared */ +/* by both persist_config_defaults() callers (cluster.c phase2 and */ +/* myapp.c step 6). It mirrors the --sync-timeout-ms convention: a */ +/* value of 0 means "flag not passed" and preserves the default; a */ +/* positive value sets the flush cadence to exactly that. */ +/* ------------------------------------------------------------------ */ + +static void test_wal_sync_ms_override_applies_positive_value(void) +{ + persist_config_t cfg = persist_config_defaults(); + persist_config_set_wal_sync_ms(&cfg, 250); + TEST_ASSERT_EQUAL_UINT32(250, cfg.wal_sync_ms); +} + +static void test_wal_sync_ms_override_zero_preserves_default(void) +{ + persist_config_t cfg = persist_config_defaults(); + /* 0 = flag omitted: cadence must stay byte-for-byte the default + * (Req 3.3 — the seam changes timing controllability only). */ + persist_config_set_wal_sync_ms(&cfg, 0); + TEST_ASSERT_EQUAL_UINT32(DEFAULT_WAL_SYNC_MS, cfg.wal_sync_ms); +} + +static void test_wal_sync_ms_override_leaves_other_fields_intact(void) +{ + persist_config_t cfg = persist_config_defaults(); + persist_config_set_wal_sync_ms(&cfg, 7); + /* Only the cadence changes; every other persistence default is + * preserved (Req 3.3 — no behavioral change beyond timing). */ + TEST_ASSERT_EQUAL_UINT32(7, cfg.wal_sync_ms); + TEST_ASSERT_EQUAL_UINT32(DEFAULT_SNAP_INTERVAL_S, cfg.snap_interval_s); + TEST_ASSERT_EQUAL_UINT32(DEFAULT_SYNC_TIMEOUT_MS, cfg.sync_timeout_ms); + TEST_ASSERT_EQUAL_UINT32(DEFAULT_MAX_WAL_RECORDS, cfg.max_wal_records); +} + /* ------------------------------------------------------------------ */ /* persist_set_workdir derives correct paths */ /* ------------------------------------------------------------------ */ @@ -103,6 +141,9 @@ void test_cluster_persist(void) RUN_TEST(test_persist_config_defaults_snap_keep_n); RUN_TEST(test_persist_config_defaults_finished_retain_s); RUN_TEST(test_persist_config_defaults_sync_timeout_ms); + RUN_TEST(test_wal_sync_ms_override_applies_positive_value); + RUN_TEST(test_wal_sync_ms_override_zero_preserves_default); + RUN_TEST(test_wal_sync_ms_override_leaves_other_fields_intact); RUN_TEST(test_persist_set_workdir_derives_wal_path); RUN_TEST(test_persist_set_workdir_derives_snap_path); } diff --git a/src/include/myconf.h b/src/include/myconf.h index 3ca46047..99af8be9 100644 --- a/src/include/myconf.h +++ b/src/include/myconf.h @@ -36,6 +36,7 @@ typedef struct { int conf_stale; /* 1 when saved config older than conf_stale_timeout */ int vcpus; /* Req 4.3: --vcpus CLI value; 0 = use config default */ uint32_t sync_timeout_ms; /* --sync-timeout-ms; 0 = use persist default */ + uint32_t wal_sync_ms; /* --wal-sync-ms; 0 = use persist default */ } myconf_t; extern cluster_config_t* get_default_conf(void*); diff --git a/src/include/persist.h b/src/include/persist.h index e42c409d..e67ce10a 100644 --- a/src/include/persist.h +++ b/src/include/persist.h @@ -42,6 +42,24 @@ static inline persist_config_t persist_config_defaults(void) return c; } +/* + * persist_config_set_wal_sync_ms — apply the --wal-sync-ms flush-cadence + * override (ha-commit-durability Req 3.1). + * + * Single override point shared by both persist_config_defaults() callers + * (cluster.c phase2 and myapp.c step 6), mirroring the --sync-timeout-ms + * convention: override_ms == 0 means "flag not passed" and leaves the + * default flush cadence untouched (Req 3.3 — the seam changes timing + * controllability only, not the durability contract). A positive value + * sets the cadence to exactly that, applied before persistence starts. + */ +static inline void persist_config_set_wal_sync_ms(persist_config_t *c, + uint32_t override_ms) +{ + if (override_ms > 0) + c->wal_sync_ms = override_ms; +} + /* ============================================================ * Key→index mapping for checkpoint serialization (Part 3.8) * ============================================================ */