feat(ha-commit-durability): gate standby ACK on fdatasync success + persist-before-ACK regression test (task 2.3)

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

@@ -37,7 +37,7 @@
   - _Requirements: 1.2, 1.4_
   - _Boundary: DurableCommit_
 
-- [ ] 2.3 Pin standby persist-before-acknowledge with a regression test
+- [x] 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_
@@ -125,3 +125,4 @@
 - 1.3: Deterministic mode = `bash src/tools/test-commit-durability.sh --deterministic` proves the durability GAP via strace, NOT joblist data-loss. SCOPE CLARIFICATION (review-ratified): single-host SIGKILL committed-job loss is physically NON-deterministic — `persist_wal_async` pwrites inline (page cache survives process death) and standbys fdatasync-before-WalAck (persist.c:515), so a committed job is always durable off the primary; a data-loss run flips GREEN/RED/RED. The deterministic slice: with `--wal-sync-ms 30000` the primary acks "jobsubmit ok" with ZERO primary fdatasync in the submit→ack window (exec-wrap `strace -f -tt -e trace=fdatasync,fsync`; PTRACE_ATTACH `-p` is yama-blocked). RED = in-window primary syncs==0 (15/15 across 5 runs; primary=0 vs standby=2). This is a valid RED→GREEN gate: the task-2.x fsync-before-ack fix makes the primary count ≥1 ⇒ GREEN. Original racy soak retained as the default mode. Boundary: only `src/tools/test-commit-durability.sh` (flag plumbed via deploy-ha.sh start.sh `$*` passthrough — no C edits). For task 5.2's E2E, prefer this strace gap-proof over joblist-loss for a deterministic durability assertion.
 - 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).

src/controller/persist.c

@@ -730,8 +730,27 @@ int persist_wal_write_replicated(persist_t* p, uint64_t lsn, uint32_t type,
      * persist_wal_async defers fsync to the background thread / persist_flush;
      * here we sync inline because the entry feeds the primary's commit
      * majority and must survive a standby restart.  fdatasync is sufficient
-     * (entry bytes + segment metadata are pre-allocated by open_new_segment). */
-    fdatasync(p->wal_fd);
+     * (entry bytes + segment metadata are pre-allocated by open_new_segment).
+     *
+     * Fail-loud rc-gate (ha-commit-durability Req 1.3, symmetric with the
+     * append-path gate at task 2.1): the ACK is sent by the caller ONLY when
+     * this returns success, and the ACK claims the entry is durable. So if
+     * the fdatasync FAILS (EIO/ENOSPC, or the forced-fail test seam), we must
+     * NOT return success — we log loudly and return -EIO so the caller's
+     * existing error path (ha_apply_single_entry) does NOT apply or ACK. */
+    int fsrc = fdatasync(p->wal_fd);
+#ifdef XCE_TESTING
+    atomic_fetch_add_explicit(&p->test_replicated_fdatasync_count, 1,
+                              memory_order_relaxed);
+#endif
+    if (fsrc != 0 || flusher_fsync_forced_fail(p)) {
+        int e = (fsrc != 0) ? errno : EIO;
+        ERROR("persist: persist_wal_write_replicated fdatasync FAILED for "
+              "lsn=%" PRIu64 " (errno=%d %s) — entry NOT durable; returning "
+              "error so the standby does NOT ACK", lsn, e, strerror(e));
+        pthread_mutex_unlock(&p->lock);
+        return -EIO;
+    }
 
     /* Rotate segment when full (same threshold as the primary append). */
     if (p->seg_offset >= WAL_SEGMENT_SIZE_BYTES) {
@@ -799,6 +818,14 @@ void persist_test_set_force_fsync_fail(persist_t* p, int fail)
                               memory_order_release);
 }
 
+unsigned persist_test_replicated_fdatasync_count(persist_t* p)
+{
+    if (!p)
+        return 0;
+    return atomic_load_explicit(&p->test_replicated_fdatasync_count,
+                                memory_order_relaxed);
+}
+
 /* ------------------------------------------------------------------ *
  * Durability-verification test seam (ha-commit-durability Req 3.2/3.3)*
  *                                                                     *

src/controller/testsuite/test_commit_durability.c

@@ -497,6 +497,112 @@ static void test_durable_advance_drives_commit_single_member(void)
     printf("test_durable_advance_drives_commit_single_member: PASS\n");
 }
 
+/* ================================================================== *
+ * task 2.3 — standby persist-before-ACK regression (Req 1.3)          *
+ *                                                                     *
+ * Req 1.3: a standby durably persists a replicated entry to local      *
+ * stable storage BEFORE sending its acknowledgment. The ACK is sent by *
+ * the CALLER (ha_apply_single_entry) only after                        *
+ * persist_wal_write_replicated returns success — so to pin "persist     *
+ * before ACK" we prove a durable fdatasync ran INSIDE the persist call  *
+ * before it returned.                                                  *
+ *                                                                     *
+ * Mechanism: a disk re-read (persist_test_durable_lsn_on_disk) cannot   *
+ * distinguish an fsync'd page from a page-cache-only page, so it CANNOT *
+ * prove the sync ran. Instead we read the XCE_TESTING fdatasync counter *
+ * incremented at the inline sync site: snapshot it before the call,     *
+ * assert it incremented after — the sync provably ran within the call,  *
+ * before the caller could ACK. The test FAILS if someone removed or     *
+ * moved the fdatasync out of persist_wal_write_replicated.             *
+ * ------------------------------------------------------------------ */
+static void test_replicated_persist_syncs_before_return(void)
+{
+    char tmp[256];
+    strcpy(tmp, "/tmp/xce_replsync_XXXXXX");
+    TEST_ASSERT_NOT_NULL(mkdtemp(tmp));
+
+    persist_t* p = make_durability_persist(tmp);
+    TEST_ASSERT_NOT_NULL(p);
+
+    /* Snapshot the sync counter, then write a replicated entry. The standby
+     * path must fdatasync inline BEFORE returning — so the counter must
+     * advance during the single call. */
+    unsigned before = persist_test_replicated_fdatasync_count(p);
+
+    int rc = persist_wal_write_replicated(p, /*lsn=*/1, WAL_JOB_UPDATE,
+                                          "x", 1);
+    TEST_ASSERT_EQUAL_INT(0, rc);
+
+    unsigned after = persist_test_replicated_fdatasync_count(p);
+    TEST_ASSERT_TRUE_MESSAGE(
+        after > before,
+        "persist_wal_write_replicated must fdatasync inline (persist-before-"
+        "ACK): the sync counter did not advance during the call");
+
+    /* And the entry is genuinely on stable storage by the time the call
+     * returned (would survive an immediate process loss → recoverable ACK). */
+    TEST_ASSERT_TRUE(persist_test_durable_lsn_on_disk(p) >= 1);
+
+    teardown_durability_persist(p);
+    printf("test_replicated_persist_syncs_before_return: PASS\n");
+}
+
+/* ------------------------------------------------------------------ *
+ * task 2.3 — forced standby-sync failure ⇒ error, NO success/ACK      *
+ *                                                                     *
+ * Closes the Req-1.3 hole: if the inline fdatasync FAILS, the standby   *
+ * must NOT report success — otherwise the caller would ACK an entry     *
+ * that is not durable (durable AFTER, not BEFORE, the ACK). With the     *
+ * force-fail seam ON, persist_wal_write_replicated must return a         *
+ * negative error; the caller (ha_apply_single_entry) already treats any  *
+ * negative return as "do not apply, do not ACK".                        *
+ *                                                                     *
+ * RED evidence: against the pre-2.3 code (which ignored the fdatasync    *
+ * return at the standby path) this returned 0 (success) under forced     *
+ * failure — a non-durable ACK. The rc-gate makes it return -EIO.        *
+ * ------------------------------------------------------------------ */
+static void test_replicated_sync_failure_no_ack(void)
+{
+    char tmp[256];
+    strcpy(tmp, "/tmp/xce_replsyncfail_XXXXXX");
+    TEST_ASSERT_NOT_NULL(mkdtemp(tmp));
+
+    persist_t* p = make_durability_persist(tmp);
+    TEST_ASSERT_NOT_NULL(p);
+
+    /* Force the fdatasync to be treated as failed, then attempt a replicated
+     * write. The durable sync cannot succeed, so the call MUST fail (no ACK).
+     * A negative return is what ha_apply_single_entry routes to its
+     * "not applying/acking" path. */
+    persist_test_set_force_fsync_fail(p, 1);
+
+    int rc = persist_wal_write_replicated(p, /*lsn=*/1, WAL_JOB_UPDATE,
+                                          "x", 1);
+    TEST_ASSERT_TRUE_MESSAGE(
+        rc < 0,
+        "forced standby fdatasync failure must make persist_wal_write_"
+        "replicated FAIL (negative rc) so the standby does NOT ACK a "
+        "non-durable entry");
+
+    /* The sync was still attempted (counter advanced) — we fail loud on the
+     * RESULT, not by skipping the sync. */
+    TEST_ASSERT_TRUE(persist_test_replicated_fdatasync_count(p) >= 1);
+
+    /* Recovery: clear the fault; a fresh replicated write (new lsn, not the
+     * deduped lsn=1) now succeeds and is durable — proving the failure path
+     * did not corrupt persist state and the sync runs again on success.
+     * (lsn=1 already advanced current_lsn to 2, so re-using 1 would dedup;
+     * lsn=2 is the genuine next entry.) */
+    persist_test_set_force_fsync_fail(p, 0);
+    int rc2 = persist_wal_write_replicated(p, /*lsn=*/2, WAL_JOB_UPDATE,
+                                           "x", 1);
+    TEST_ASSERT_EQUAL_INT(0, rc2);
+    TEST_ASSERT_TRUE(persist_test_durable_lsn_on_disk(p) >= 2);
+
+    teardown_durability_persist(p);
+    printf("test_replicated_sync_failure_no_ack: PASS\n");
+}
+
 #endif /* XCE_TESTING */
 
 /* ------------------------------------------------------------------ */
@@ -514,6 +620,8 @@ void test_commit_durability(void)
     RUN_TEST(test_durable_lsn_invariants_under_load);
     RUN_TEST(test_durable_lsn_holds_on_fsync_failure);
     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);
 #else
     printf("test_commit_durability: skipped (no XCE_TESTING)\n");
 #endif

src/include/persist.h

@@ -117,6 +117,14 @@ struct persist_s {
      * must NOT advance and the failure is logged loudly. Compiled out of
      * production (xce_controller is built without -DXCE_TESTING). */
     atomic_int      test_force_fsync_fail;
+    /* fdatasync observability counter (ha-commit-durability task 2.3):
+     * incremented at the standby-path inline sync in
+     * persist_wal_write_replicated, BEFORE that function returns (and thus
+     * before the caller can ACK). A persist-before-ACK regression test reads
+     * this to prove a durable sync ran inside the call — page-cache re-reads
+     * cannot distinguish fsync'd from cache-only, so a direct sync signal is
+     * required. Compiled out of production. */
+    atomic_uint     test_replicated_fdatasync_count;
 #endif
     pthread_t       fsync_thread;
     pthread_mutex_t fsync_mutex;   /* guards the fsync timer wait */
@@ -265,6 +273,14 @@ uint64_t persist_test_durable_lsn_on_disk(persist_t* p);
  * Lock-free atomic write; safe to flip from a test thread at any time.
  * Compiled out of production (xce_controller links without it). */
 void persist_test_set_force_fsync_fail(persist_t* p, int fail);
+
+/* Read the standby-path fdatasync counter (task 2.3, persist-before-ACK
+ * regression). Counts the inline fdatasync calls made by
+ * persist_wal_write_replicated. A test snapshots this before the call and
+ * asserts it incremented after the call returns — proving the durable sync
+ * 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);
 #endif /* XCE_TESTING */
 
 /* Checkpoint */