feat(ha-commit-durability): gate legacy sync-append commit store on durable_lsn + commit<=durable invariant (task 2.4)

.kiro/specs/ha-commit-durability/tasks.md

@@ -43,7 +43,7 @@
   - _Requirements: 1.3_
   - _Boundary: DurableCommit_
 
-- [ ] 2.4 Reconcile the dormant synchronous-append commit path
+- [x] 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_
@@ -126,3 +126,4 @@
 - 2.1: `persist_t.durable_lsn` (`_Atomic uint64_t`, accessor `persist_durable_lsn()`) = prefix watermark advanced ONLY after a successful `fdatasync`. `fsync_worker` is now a demand-driven group-commit flusher: `persist_wal_async` pwrites under `p->lock`, releases, then signals `fsync_cond`. Lost-wakeup guard is LEVEL-triggered on `flusher_work_pending_locked` (`durable_lsn < current_lsn-1`), re-checked under `p->lock` before wait and after each flush (reviewer-confirmed sound). Invariants: monotonic, `durable_lsn <= current_lsn-1`, clamped on truncate (`clamp_durable_lsn_locked` at both truncate paths). fail-loud rc-gate: `durable_lsn` does NOT advance on `fdatasync` error (all 4 sites: flusher main/fallback, `persist_stop`, `persist_flush`) — logs ERROR. Test fault-injection seam: `#ifdef XCE_TESTING` toggle `persist_test_set_force_fsync_fail()` (production-excluded, nm-verified). NOT yet consumed by commit (that is 2.2). FOLLOW-UP (non-blocking, out of scope for 2.1): bound the flusher retry on SUSTAINED fdatasync failure (backoff / rate-limited logging) — currently tight-spins (releases `p->lock` each iter, so liveness OK, but CPU/log burn on a degrading disk). NOTE: a HANGING (not erroring) fdatasync blocks appends since it runs under `p->lock` — pre-existing, not 2.1's boundary.
 - 2.2: `majority_candidate_locked` (commit.c:93) self-cap = `persist_durable_lsn(ce->p)` (was `highest_appended_lsn`); appended tip kept only for the `assert(cand <= highest_appended_lsn)` never-beyond-log check. So `committed_lsn` cannot advance over an entry until it is durable locally (Req 1.2/1.4). Durable-advance WAKE (the wire 2.1 deferred): persist defines a callback hook `on_durable_advance(void*)`/`durable_cb_ctx` + `persist_set_durable_cb()`; commit registers `commit_on_durable_advance` (ctx=engine) in `commit_engine_init`, unregisters in `commit_engine_fini`. Flusher invokes it ONLY when `durable_lsn` advanced, AFTER releasing `p->lock` → callback takes `ce->lock` + broadcasts `ce->cond`. Respects commit→persist dependency direction (persist never includes commit). Lock order p->lock→ce->lock only (cycle-free). Lost-wakeup-free: broadcast follows the release-store; waiter re-reads durable under ce->lock; timedwait backstop. Shutdown UAF closed: `persist_stop` JOINS the flusher BEFORE `commit_engine_fini` (cluster.c:1675 vs 1695); final stop-flush does NOT fire the callback; fini also unregisters defensively. GOTCHA (bug-704): self-cap broke 4 existing commit-path fixtures (test_repl_majority/coldstart_lsn0/commit_barrier_rollout/demotion_convergence) that primed `current_lsn` but not `durable_lsn` — fix = prime `durable_lsn = appended tip` in any in-memory fixture that sets current_lsn without writing real WAL bytes (the flusher can't advance durable for a fake log). Any future commit-engine test must prime durable_lsn too.
 - 2.3: Standby persist-before-ACK pinned + Req-1.3 hole closed. `persist_wal_write_replicated` (persist.c:741) now rc-gates the inline `fdatasync`: on failure (or `flusher_fsync_forced_fail`) it logs ERROR + `return -EIO` (symmetric with the 2.1 flusher gate) — the standby will NOT ACK a non-durable entry. Caller `ha_apply_single_entry` (ha.c:1135-1166) ALREADY routes a negative return to no-apply/no-ACK (`ha_send_wal_ack_reply` reached only on rc==0), so NO ha.c change was needed (boundary stayed persistence-only). Ordering pin = `#ifdef XCE_TESTING atomic_uint test_replicated_fdatasync_count` incremented at the sync site; `test_replicated_persist_syncs_before_return` asserts it increments inside the call (page-cache means a disk re-read can't prove fsync — use the counter). `test_replicated_sync_failure_no_ack` REDs without the gate. Seam production-excluded (nm-verified). HOUSEKEEPING: controller tests `mkdtemp` into `/tmp/xce_*` and don't clean up — thousands accumulate and eventually make `mkdtemp` fail → misleading suite-wide "Expected Non-NULL" cascade. Run `rm -rf /tmp/xce_*` if that appears (pre-existing test pattern, not a regression).
+- 2.4: SPEC INACCURACY corrected — `persist_wal_sync_append` is NOT dormant; it is the LIVE job-submit path (`persist_job_hook_fun` calls it at persist.c:1911). Took the "route through durable-majority eval" option (delete infeasible). Production path (ce!=NULL, lsn!=0) was already durable-safe via 2.2's `majority_candidate_locked` self-cap. Closed the hazard in the LEGACY branch: the former unconditional `atomic_store(committed_lsn, written_lsn)` on majority ACK is now `sync_append_legacy_commit_store()` which advances ONLY when `written_lsn <= persist_durable_lsn(p)` (LSN-0 edge: store of 0 valid). Added the design's debug invariant `committed_lsn <= durable_lsn` (`#ifdef XCE_TESTING` assert at commit.c:162 + inside the legacy store) as the future-proof guard. Fixed the stale "after local fsync via persist_wal_async" comment (2.1 deferred the fsync). CODE-SEARCH (authoritative): the ONLY `committed_lsn` stores are — engine advance (commit.c, durable-capped by 2.2), legacy sync_append (now gated), and the two `persist_truncate_wal_above` cutoff stores (NOT ACK-driven; each immediately `clamp_durable_lsn_locked`). ha.c/ha_convergence.c stores are `last_known_committed_lsn` (DIFFERENT field). So no path advances commit on ACK without the durable precondition. Seam `persist_test_sync_append_legacy_commit_store` is production-excluded.

src/controller/commit.c

@@ -154,6 +154,12 @@ static void commit_advance_locked(commit_engine_t* ce)
 #ifdef XCE_TESTING
     assert(cand > cur);                              /* monotonic advance only */
     assert(cand <= highest_appended_lsn(ce->p));     /* never beyond the log */
+    /* DURABLE-COMMIT INVARIANT (ha-commit-durability task 2.4, Req 1.1/1.2):
+     * committed_lsn must never advance past what is durable locally. The
+     * candidate is already durable-capped by majority_candidate_locked (self
+     * caps at persist_durable_lsn), so this must hold — it guards against a
+     * future change that reintroduces an ACK-without-durability advance. */
+    assert(cand <= persist_durable_lsn(ce->p));
 #endif
 
     atomic_store(&ce->p->committed_lsn, cand);

src/controller/persist.c

@@ -1580,6 +1580,42 @@ int persist_recover(persist_t* p)
     return 0;
 }
 
+/* Advance committed_lsn to `written_lsn` on a majority ACK — but ONLY if that
+ * entry is already durable on local stable storage (ha-commit-durability task
+ * 2.4, Req 1.1/1.2). This is the legacy (engine-less / LSN-0) sibling of the
+ * commit engine's durable self-cap (commit.c majority_candidate_locked): a
+ * majority ACK no longer implies the local copy is fsync'd, because group-
+ * commit (task 2.1) deferred the append fsync to the demand-driven flusher. So
+ * advancing committed_lsn here on the ACK alone would reintroduce the bug-661
+ * ACK-without-durability hazard — committed could be durable on a single
+ * member and lost across failover. We gate the store on written_lsn <=
+ * durable_lsn so committed_lsn ⇒ durable-on-this-member on every path.
+ * committed_lsn is monotonic, so a max() store; the durable cap can only delay
+ * the advance until the flusher syncs the entry (the durable-advance commit
+ * wake re-drives the engine path; the legacy path is re-evaluated on the next
+ * sync_append). */
+static void sync_append_legacy_commit_store(persist_t* p, uint64_t written_lsn)
+{
+    uint64_t durable = atomic_load_explicit(&p->durable_lsn,
+                                            memory_order_acquire);
+    /* DURABILITY PRECONDITION: never advance committed past what is durable
+     * locally. The LSN-0 first-entry edge stays valid (0 <= 0). */
+    if (written_lsn > durable)
+        return;
+    uint64_t cur = atomic_load_explicit(&p->committed_lsn,
+                                        memory_order_acquire);
+    if (written_lsn <= cur)
+        return; /* monotonic: committed_lsn never regresses */
+    atomic_store_explicit(&p->committed_lsn, written_lsn, memory_order_release);
+#ifdef XCE_TESTING
+    /* INVARIANT (debug guard): committed_lsn must never exceed durable_lsn on
+     * any commit advance. A future caller reintroducing ACK-without-durability
+     * trips this. */
+    assert(atomic_load_explicit(&p->committed_lsn, memory_order_acquire)
+           <= atomic_load_explicit(&p->durable_lsn, memory_order_acquire));
+#endif
+}
+
 int persist_wal_sync_append(persist_t* p, uint32_t type,
                             const void* payload, uint32_t len,
                             int* sync_rc_out)
@@ -1592,7 +1628,12 @@ int persist_wal_sync_append(persist_t* p, uint32_t type,
     }
 
     /* Majority-commit replication (Req 5.1–5.3; ha-commit-engine Req 1.1–1.3).
-     * After local fsync via persist_wal_async, confirm per-entry majority.
+     * persist_wal_async appends to the page cache and signals the demand-driven
+     * flusher; it no longer fsyncs inline (group-commit, task 2.1). Durability
+     * is therefore established by the flusher advancing durable_lsn, and a
+     * commit position advances only once the entry is durable locally — the
+     * engine path via the durable self-cap (commit.c), the legacy path below
+     * via sync_append_legacy_commit_store's written_lsn <= durable_lsn gate.
      * We use current_lsn - 1 because persist_wal_async already incremented it. */
     uint64_t written_lsn;
     pthread_mutex_lock(&p->lock);
@@ -1639,8 +1680,10 @@ int persist_wal_sync_append(persist_t* p, uint32_t type,
                                               p->config.sync_timeout_ms,
                                               (uint8_t)OPCode_walAck);
             if (sync_err == 0)
-                atomic_store_explicit(&p->committed_lsn, written_lsn,
-                                      memory_order_release);
+                /* Durable-gated commit advance (task 2.4): advance committed_lsn
+                 * only if written_lsn is durable locally — never on the ACK
+                 * alone (group-commit removed the inline append fsync). */
+                sync_append_legacy_commit_store(p, written_lsn);
             else
                 CTRL_METRIC_INC(ctrl_repl_degraded_count);
         }
@@ -1662,6 +1705,19 @@ int persist_wal_sync_append(persist_t* p, uint32_t type,
     return 0;
 }
 
+#ifdef XCE_TESTING
+/* Test seam (task 2.4): invoke the EXACT durable-gated commit store that the
+ * legacy sync_append branch uses on a majority ACK, so a test can prove the
+ * ACK-without-durability hazard is closed without standing up the replication
+ * wire. Pre-fix this branch stored committed_lsn = written_lsn unconditionally
+ * (over-advancing past durable_lsn). */
+void persist_test_sync_append_legacy_commit_store(persist_t* p,
+                                                  uint64_t written_lsn)
+{
+    sync_append_legacy_commit_store(p, written_lsn);
+}
+#endif /* XCE_TESTING */
+
 void persist_log_entry_metadata(const wal_entry_hdr_t* hdr)
 {
     TRACE("WAL entry: type=%u lsn=%lu term=%lu payload_len=%u crc=%08x",

src/controller/testsuite/test_commit_durability.c

@@ -603,6 +603,178 @@ static void test_replicated_sync_failure_no_ack(void)
     printf("test_replicated_sync_failure_no_ack: PASS\n");
 }
 
+/* ================================================================== *
+ * task 2.4 — reconcile the dormant synchronous-append commit path      *
+ *                                                                     *
+ * persist_wal_sync_append's LEGACY branch (ce == NULL, or the LSN-0    *
+ * first entry on the engine path) historically advanced committed_lsn  *
+ * DIRECTLY on a majority ACK (atomic_store committed_lsn = written_lsn) *
+ * with NO durable precondition. Group-commit removed the premise that  *
+ * the append already fsync'd inline, so a majority ACK no longer        *
+ * implies the local entry is durable. This branch is the structural    *
+ * ACK-without-durability hazard task 2.4 must close: NO path may        *
+ * advance committed_lsn on a majority ACK unless the entry is durable   *
+ * locally (committed_lsn <= durable_lsn).                              *
+ *                                                                     *
+ * NOTE: persist_wal_sync_append is NOT dormant — it is the LIVE         *
+ * job-submit path (persist_job_hook_fun, WAL_JOB_SUBMIT). So the fix is *
+ * to ROUTE the legacy direct-store through the durable precondition,    *
+ * not to delete the function.                                          *
+ * ------------------------------------------------------------------ */
+
+/* Build an ENGINE-LESS persist with standbys registered, so
+ * persist_wal_sync_append takes the legacy branch (ce == NULL) and its
+ * has_standbys path: repl_wait_majority_ack returns 0 immediately for
+ * the LSN-0 first entry, but for a NON-zero LSN it surfaces -EINVAL via
+ * commit_submit (engine-less). The hazard we guard is the direct
+ * committed_lsn store that the legacy branch performs on a 0 return. */
+
+/* Drive the legacy branch directly: a single-member commit engine makes
+ * the ENGINE path durable-safe, so to exercise the legacy direct-store
+ * we use an engine-less persist and a registered standby that ACKs.
+ * Rather than stand up the replication wire (out of boundary), we test
+ * the invariant the fix establishes: committed_lsn must never exceed
+ * durable_lsn after a commit advance, on EVERY path. */
+
+/* ------------------------------------------------------------------ *
+ * Test (RED before fix): the legacy direct-store path must NOT advance  *
+ * committed_lsn past durable_lsn. We drive the legacy branch via the    *
+ * LSN-0-style direct store by appending one entry that is NOT yet       *
+ * durable (slow cadence, no flush) and confirming committed_lsn stays   *
+ * at/below durable_lsn.                                                 *
+ *                                                                     *
+ * Mechanism: persist_test_sync_append_legacy_store() is an XCE_TESTING  *
+ * seam that invokes the SAME guarded store the legacy branch performs   *
+ * (advance committed_lsn to written_lsn only if written_lsn <=          *
+ * durable_lsn). Pre-fix the store was unconditional, so this over-       *
+ * advanced committed past durable (RED). Post-fix it holds.             *
+ * ------------------------------------------------------------------ */
+static void test_legacy_sync_append_holds_committed_at_durable(void)
+{
+    char tmp[256];
+    strcpy(tmp, "/tmp/xce_legacycommit_XXXXXX");
+    TEST_ASSERT_NOT_NULL(mkdtemp(tmp));
+
+    persist_t* p = make_durability_persist(tmp);
+    TEST_ASSERT_NOT_NULL(p);
+
+    /* Append one entry (LSN 1) but do NOT flush — durable_lsn lags at 0,
+     * while the appended tip is 1. The legacy direct-store would advance
+     * committed_lsn to the ACK'd written_lsn (1) — over-advancing past
+     * the durable watermark (0). The durable precondition must hold it. */
+    TEST_ASSERT_EQUAL_INT(0, persist_wal_async(p, WAL_JOB_UPDATE, "a", 1));
+    uint64_t written_lsn = p->current_lsn - 1; /* 1 */
+    TEST_ASSERT_EQUAL_UINT64(1, written_lsn);
+    TEST_ASSERT_EQUAL_UINT64(0, persist_durable_lsn(p));
+    TEST_ASSERT_EQUAL_UINT64(0, atomic_load(&p->committed_lsn));
+
+    /* Invoke the guarded legacy store directly (simulates a majority ACK
+     * for an entry that is not yet durable locally). It must NOT advance
+     * committed_lsn past durable_lsn. */
+    persist_test_sync_append_legacy_commit_store(p, written_lsn);
+
+    TEST_ASSERT_TRUE_MESSAGE(
+        atomic_load(&p->committed_lsn) <= persist_durable_lsn(p),
+        "legacy sync-append commit store must NOT advance committed_lsn past "
+        "durable_lsn (ACK-without-durability hazard)");
+    /* Concretely: committed stays at 0 because durable_lsn is still 0. */
+    TEST_ASSERT_EQUAL_UINT64(0, atomic_load(&p->committed_lsn));
+
+    /* Now make the entry durable and re-invoke: the store is now allowed
+     * (written_lsn <= durable_lsn) and committed advances to it. */
+    TEST_ASSERT_EQUAL_INT(0, persist_flush(p));
+    TEST_ASSERT_TRUE(persist_durable_lsn(p) >= written_lsn);
+    persist_test_sync_append_legacy_commit_store(p, written_lsn);
+    TEST_ASSERT_EQUAL_UINT64(written_lsn, atomic_load(&p->committed_lsn));
+    TEST_ASSERT_TRUE(atomic_load(&p->committed_lsn) <= persist_durable_lsn(p));
+
+    teardown_durability_persist(p);
+    printf("test_legacy_sync_append_holds_committed_at_durable: PASS\n");
+}
+
+/* ------------------------------------------------------------------ *
+ * Test: the LSN-0 first-entry edge stays valid. durable_lsn and        *
+ * committed_lsn both start at 0; the legacy store of written_lsn == 0   *
+ * (0 <= 0) is permitted and is a harmless no-op store of 0. This is the *
+ * once-per-cluster-lifetime path the engine branch routes here.         *
+ * ------------------------------------------------------------------ */
+static void test_legacy_sync_append_lsn0_edge_valid(void)
+{
+    char tmp[256];
+    strcpy(tmp, "/tmp/xce_legacylsn0_XXXXXX");
+    TEST_ASSERT_NOT_NULL(mkdtemp(tmp));
+
+    persist_t* p = make_durability_persist(tmp);
+    TEST_ASSERT_NOT_NULL(p);
+
+    /* Fresh store: durable_lsn == 0, committed_lsn == 0. */
+    TEST_ASSERT_EQUAL_UINT64(0, persist_durable_lsn(p));
+    TEST_ASSERT_EQUAL_UINT64(0, atomic_load(&p->committed_lsn));
+
+    /* Storing written_lsn == 0 (0 <= durable 0) is allowed — a no-op. */
+    persist_test_sync_append_legacy_commit_store(p, 0);
+    TEST_ASSERT_EQUAL_UINT64(0, atomic_load(&p->committed_lsn));
+    TEST_ASSERT_TRUE(atomic_load(&p->committed_lsn) <= persist_durable_lsn(p));
+
+    teardown_durability_persist(p);
+    printf("test_legacy_sync_append_lsn0_edge_valid: PASS\n");
+}
+
+/* ------------------------------------------------------------------ *
+ * Guard test (the Observable's "test guards against a future caller"):  *
+ * the committed_lsn <= durable_lsn invariant holds across the commit-   *
+ * engine advance path too. Drives the real engine + flusher and asserts *
+ * that at every observed point committed never exceeds durable — the    *
+ * future-proof guard against reintroducing an ACK-without-durability    *
+ * commit on ANY path. In a debug build the in-code assert would abort   *
+ * if a path ever advanced committed past durable.                       *
+ * ------------------------------------------------------------------ */
+static void test_commit_never_exceeds_durable_invariant(void)
+{
+    char tmp[256];
+    strcpy(tmp, "/tmp/xce_cinvariant_XXXXXX");
+    TEST_ASSERT_NOT_NULL(mkdtemp(tmp));
+
+    persist_t* p = make_durability_persist(tmp);
+    TEST_ASSERT_NOT_NULL(p);
+
+    commit_engine_t ce;
+    TEST_ASSERT_EQUAL_INT(0, commit_engine_init(&ce, p, 1, 8));
+
+    /* Append a burst while sampling the invariant: committed must never
+     * outrun durable, even mid-flight while the flusher advances both. */
+    for (int round = 0; round < 6; round++) {
+        for (int i = 0; i < 4; i++)
+            TEST_ASSERT_EQUAL_INT(
+                0, persist_wal_async(p, WAL_JOB_UPDATE, "q", 1));
+        for (int s = 0; s < 50; s++) {
+            uint64_t committed = atomic_load(&p->committed_lsn);
+            uint64_t durable   = persist_durable_lsn(p);
+            TEST_ASSERT_TRUE_MESSAGE(
+                committed <= durable,
+                "committed_lsn must never exceed durable_lsn on any commit "
+                "advance path");
+            usleep(1000);
+        }
+    }
+
+    /* And eventually committed catches up to the durable tip. */
+    uint64_t tip = p->current_lsn - 1;
+    uint64_t committed = 0;
+    for (int i = 0; i < 5000; i++) {
+        committed = atomic_load(&p->committed_lsn);
+        if (committed >= tip)
+            break;
+        usleep(1000);
+    }
+    TEST_ASSERT_EQUAL_UINT64(tip, committed);
+    TEST_ASSERT_TRUE(atomic_load(&p->committed_lsn) <= persist_durable_lsn(p));
+
+    commit_engine_fini(&ce);
+    teardown_durability_persist(p);
+    printf("test_commit_never_exceeds_durable_invariant: PASS\n");
+}
+
 #endif /* XCE_TESTING */
 
 /* ------------------------------------------------------------------ */
@@ -622,6 +794,9 @@ void test_commit_durability(void)
     RUN_TEST(test_durable_advance_drives_commit_single_member);
     RUN_TEST(test_replicated_persist_syncs_before_return);
     RUN_TEST(test_replicated_sync_failure_no_ack);
+    RUN_TEST(test_legacy_sync_append_holds_committed_at_durable);
+    RUN_TEST(test_legacy_sync_append_lsn0_edge_valid);
+    RUN_TEST(test_commit_never_exceeds_durable_invariant);
 #else
     printf("test_commit_durability: skipped (no XCE_TESTING)\n");
 #endif

src/include/persist.h

@@ -281,6 +281,14 @@ void persist_test_set_force_fsync_fail(persist_t* p, int fail);
  * ran inside the persist primitive, BEFORE the caller can send its WalAck.
  * Lock-free atomic read; compiled out of production. */
 unsigned persist_test_replicated_fdatasync_count(persist_t* p);
+
+/* Invoke the legacy sync-append commit store (task 2.4) with the durable
+ * precondition applied — the same store persist_wal_sync_append performs on a
+ * majority ACK in its engine-less / LSN-0 branch. Lets a test prove
+ * committed_lsn never advances past durable_lsn on the ACK path without
+ * standing up the replication wire. Compiled out of production. */
+void persist_test_sync_append_legacy_commit_store(persist_t* p,
+                                                  uint64_t written_lsn);
 #endif /* XCE_TESTING */
 
 /* Checkpoint */