diff --git a/.kiro/specs/ha-commit-durability/tasks.md b/.kiro/specs/ha-commit-durability/tasks.md index 7ed44010..1b6e03e6 100644 --- a/.kiro/specs/ha-commit-durability/tasks.md +++ b/.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. diff --git a/src/controller/commit.c b/src/controller/commit.c index 13b1bc41..c2f039d6 100644 --- a/src/controller/commit.c +++ b/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); diff --git a/src/controller/persist.c b/src/controller/persist.c index 9c22d4ec..70b2e228 100644 --- a/src/controller/persist.c +++ b/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", diff --git a/src/controller/testsuite/test_commit_durability.c b/src/controller/testsuite/test_commit_durability.c index cffb4f4c..b0cc1e1a 100644 --- a/src/controller/testsuite/test_commit_durability.c +++ b/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 diff --git a/src/include/persist.h b/src/include/persist.h index 2b7ca28f..b651c197 100644 --- a/src/include/persist.h +++ b/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 */