From e4c496833169bffa73b53a2c5666fcfe48c5e2fe Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 18 Jun 2026 06:45:21 +0000 Subject: [PATCH] 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 | 3 +- src/controller/persist.c | 31 ++++- .../testsuite/test_commit_durability.c | 108 ++++++++++++++++++ src/include/persist.h | 16 +++ 4 files changed, 155 insertions(+), 3 deletions(-) diff --git a/.kiro/specs/ha-commit-durability/tasks.md b/.kiro/specs/ha-commit-durability/tasks.md index 701f527..7ed4401 100644 --- a/.kiro/specs/ha-commit-durability/tasks.md +++ b/.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). diff --git a/src/controller/persist.c b/src/controller/persist.c index 0803177..9c22d4e 100644 --- a/src/controller/persist.c +++ b/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)* * * diff --git a/src/controller/testsuite/test_commit_durability.c b/src/controller/testsuite/test_commit_durability.c index 164db05..cffb4f4 100644 --- a/src/controller/testsuite/test_commit_durability.c +++ b/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 diff --git a/src/include/persist.h b/src/include/persist.h index fa82f9c..2b7ca28 100644 --- a/src/include/persist.h +++ b/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 */