From 3ad646ad0e08eb2a1ac6c2ef5e060f44d6fe55f2 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 20 Jun 2026 00:33:18 +0000 Subject: [PATCH] spec(ha-commit-durability): REOPEN for bug-795 total-outage cold-restart-all committed-job loss Phase 6 added (6.1 reproduce-on-real-fs gates 6.2 root-cause / 6.3 fix / 6.4 revalidate). Surfaced by the xce-test-e2e-primitives cold-restart-all soak step; Phase 5 only validated rolling single-node failover. CLAUDE.md durability claim qualified pending 6.x. --- .kiro/specs/ha-commit-durability/research.md | 197 +++++++++++++++++++ .kiro/specs/ha-commit-durability/spec.json | 24 +++ .kiro/specs/ha-commit-durability/tasks.md | 27 +++ src/controller/CLAUDE.md | 2 +- 4 files changed, 249 insertions(+), 1 deletion(-) create mode 100644 .kiro/specs/ha-commit-durability/research.md create mode 100644 .kiro/specs/ha-commit-durability/spec.json diff --git a/.kiro/specs/ha-commit-durability/research.md b/.kiro/specs/ha-commit-durability/research.md new file mode 100644 index 0000000..f335cf9 --- /dev/null +++ b/.kiro/specs/ha-commit-durability/research.md @@ -0,0 +1,197 @@ +# Gap Analysis — ha-commit-durability + +> Brownfield analysis against the XCE controller (`src/controller/`). Informs `/kiro-spec-design`. +> Line numbers from the working tree at analysis time (2026-06-18); re-confirm before editing. + +## Current-State Summary + +The HA stack is mature and well-factored: per-LSN majority commit engine (`commit.c`), fan-out WAL replication with ACK/catch-up (`repl.c`, `config_repl.c`), a commit barrier that gates client replies (`persist_commit_barrier`), demotion truncate + rejoin reconciliation (`ha_convergence.c`), single-leader own-term gate, PreVote, and a vote-freshness gate. The durability gap is **beneath** all of it: "committed" (majority-ACKed) does not imply "durable on a majority", and recovery/promotion trust in-memory position rather than the durable log. + +The submit reply path is already commit-gated and runs **off the iomgr thread** (`job.c:568-569` — "the AFTER_EVENT hook writes WAL and waits for majority replication"), so durability work can be added to that path without stalling heartbeats. + +## Requirement-to-Asset Map + +### Requirement 1 — Durable Commit (acknowledgment ⇒ majority persistence) + +| Need | Existing asset | Status | +|------|----------------|--------| +| Reply gated on commit | `persist_commit_barrier` → `commit_submit(COMMIT_MODE_WAIT)` (persist.c); callers `job.c:1024`, `queue.c:144`, `worker.c:713`, `join.c:130` | ✅ Exists — reuse; strengthen meaning of "committed" | +| Standby persists before ACK | `persist_wal_write_replicated` `fdatasync` before return (persist.c:515) | ✅ Exists — **pin with regression test** (Req 1.3) | +| Primary self-counts only when durable | `commit.c:majority_candidate_locked` sets `cap = highest_appended_lsn(ce->p)` (appended, not durable) | ❌ **Missing** — core defect (bug-661) | +| Primary persists before self-count | `persist_wal_async` (persist.c:340) appends via `pwrite`, broadcasts, returns **without fsync**; durability deferred to `fsync_worker` timer (persist.c:173) | ❌ **Missing** | +| Track highest durable LSN | none — `persist_t` has `committed_lsn`/`current_lsn` but no `durable_lsn` | ❌ **Missing** (new field) | +| Liveness during durability work | commit-wait path already off iomgr (`job.c:568-569`) | ✅ Constraint satisfied by placement | + +**Minimal change shape:** add `durable_lsn` to `persist_t`, advanced when the entry's bytes are `fdatasync`'d; `persist_wal_async` fsyncs (inline, synchronous design — see brief decision) and sets `durable_lsn` before returning; `majority_candidate_locked` uses `min(durable_lsn, …)` for the self contribution instead of `highest_appended_lsn`. Reply gate then enforces the invariant unchanged. + +### Requirement 2 — Recovery Integrity + Leadership Fence + +| Need | Existing asset | Status | +|------|----------------|--------| +| No silent truncate-to-zero on corrupt recovery | `replay_wal` breaks on CRC chain break / CRC mismatch / short read (persist.c:1021, 1050, 1042) and returns the replayed count as **success**; only `-ENOMEM` propagates (persist.c:1107, 1152) | ❌ **Missing** — silent truncate-accept is the amplifier | +| Durable floor re-derived on restart | `persist_recover` **never sets `committed_lsn`** — it stays 0 after restart; floor is reconstructed only at promotion (persist.c:1114) | ❌ **Missing** — restart advertises in-memory floor unbacked by WAL | +| Pre-promotion integrity / caught-up gate | `raft_cb_become_leader` (ha.c:551) computes `safe_lsn` from votes (ha.c:572) then `persist_truncate_wal_above(safe_lsn)` (ha.c:577) — **no check that local WAL is intact or ≥ safe_lsn** | ❌ **Missing** — hollow node can promote, truncate collapses tip without backfill | +| Freshness floor from durable WAL | `ha_vote_self_freshness` = `max(applied_lsn, last_known_committed_lsn)` — **in-memory atomics**, not re-derived from durable WAL (ha.c:2127; used cluster.c:380-396 vote, cluster.c:467-474 PreVote) | ❌ **Missing** — truncated candidate advertises a floor it can't back | +| Fence "ineligible until caught up" | `in_catch_up` atomic exists but only a **standby** uses it to deny its **own** outgoing vote (ha.c:325, cluster.c:529); no fence for a freshly-recovered/promoting node | ⚠️ Partial — extend concept | +| Re-eligibility on catch-up | standby catch-up apply `ha_apply_wal_catchup_entry` (ha.c:1192); re-inclusion exists | ✅ Exists — hook the fence release here | + +### Requirement 3 — Deterministic Durability Verification + +| Need | Existing asset | Status | +|------|----------------|--------| +| Configurable durability cadence / disable background flush | `wal_sync_ms` = `DEFAULT_WAL_SYNC_MS` (config.h:86, value 10); `persist_config_defaults` (persist.h:30); **no env/CLI/config override** — callers `myapp.c:164`, `cluster.c:1267` pass defaults unmutated | ❌ **Missing** — must add an override | +| Test-mode seam convention | `#ifdef XCE_TESTING` hook shims (ha.c:2270-2335), test-only `handle_request_vote` (ha.c:2161); cluster.c not linked under XCE_TESTING (cluster.c:67) | ✅ Pattern exists — follow it | + +> Doc drift noted: `persistence-and-ha-design.md:4178` says `wal_sync_ms=50`; code is 10 (config.h:86). Reconcile in design. + +## Implementation Approach Options + +**Option A — Extend existing components (recommended).** All change sites are inside existing, well-scoped functions (`commit.c`, `persist.c`, `ha.c`, `cluster.c`). R1 = `durable_lsn` field + 2 edits (`persist_wal_async` fsync, `majority_candidate_locked` self cap). R2 = make `replay_wal` return an integrity verdict + a pre-promotion gate in `raft_cb_become_leader` + floor-from-durable-WAL in `ha_vote_self_freshness`. R3 = config override + an `XCE_TESTING` seam. +- ✅ Leverages the commit-barrier and `in_catch_up` machinery already present; smallest surface; matches TLA+ mapping in `CLAUDE.md`. +- ❌ Touches correctness-critical election/commit code — demands rigorous TDD and TLA+ re-verification. + +**Option B — New durability module.** Extract a `durability`/`durable_lsn` tracker as a new component. +- ✅ Isolated unit testing. ❌ Over-engineered — `durable_lsn` is one atomic on `persist_t`; a new module adds indirection without separation benefit. Not recommended. + +**Option C — Hybrid (recommended for R2 only).** Extend in place for R1/R3; add a small new **recovery-integrity + promotion-eligibility helper** (e.g. `persist_recovered_intact()` / `ha_promotion_durable_ok()`) for R2 so the integrity verdict and the fence predicate are unit-testable and don't bloat `raft_cb_become_leader`. +- ✅ Keeps the election hot path readable; testable fence predicate. Recommended: **A for R1+R3, C for R2.** + +## Effort & Risk + +| Req | Effort | Risk | Justification | +|-----|--------|------|---------------| +| R1 Durable commit | **M** (3–7d) | **Medium-High** | Localized (`durable_lsn` + 2 edits) but correctness-critical; needs TLA+ re-check (commit rule, leader completeness) and durability TDD. Liveness already safe (off-iomgr path). | +| R2 Recovery + fence | **L** (1–2wk) | **High** | Touches recovery, promotion, and the vote-freshness floor; must **compose with `ha-convergence-recovery`** (truncate/rejoin) and `ha-single-leader-role` (own-term gate) without double-handling, and honor `CLAUDE.md`'s "never truncate from stale ctrlhello" rule. Election-safety sensitive. | +| R3 Verification seam | **S** (1–3d) | **Low** | Config override + `XCE_TESTING` hook following an established pattern. | + +## Recommendations for Design Phase + +**Preferred approach:** Option A (extend) for R1 + R3; Option C (extend + small fence helper) for R2. + +**Key design decisions to resolve:** +1. **Durability mechanism** — already decided (brief): synchronous persist-before-self-count, eliminate `fsync_worker` as the durability mechanism; group-commit batching deferred to roadmap. Design must confirm `fsync_worker`'s fate: delete entirely vs retain a best-effort flush for any non-client-acked / local-only write (audit `wal_classify` for writes that gate no reply; checkpoint markers already fsync inline at persist.c:573). +2. **`durable_lsn` semantics** — single atomic on `persist_t`; advanced under `p->lock` after `fdatasync`; consumed by `majority_candidate_locked`. Confirm interaction with `persist_truncate_wal_above` (must not let `durable_lsn` exceed a post-truncation tip). +3. **Recovery integrity verdict** — `replay_wal`/`persist_recover` must distinguish "clean EOF" from "stopped at corruption". Decide: FATAL on corruption (fail-loud, simplest, honors the user's fail-loud preference) vs flag-and-fence (recover what's intact, fence from leadership until caught up). The fail-loud preference (`[[feedback_fail_loud_over_timeout]]`) leans FATAL unless the node can provably catch up. +4. **Freshness floor source** — derive the candidate's advertised floor from the **durable WAL tip**, not in-memory `applied_lsn`/`last_known_committed_lsn` alone, so a truncated node cannot advertise an unbacked floor. Reconcile with `CLAUDE.md`: never truncate using a stale ctrlhello `last_known_committed_lsn`; the safe-commit point still comes from voter replies at promotion. +5. **Compose with convergence-recovery** — R2's pre-promotion fence must not duplicate `ha_convergence.c`'s demotion-truncate/rejoin logic; define the seam (this spec fences *promotion*; convergence handles *demotion* and *rejoin reconciliation* of an intact log). + +**Research items to carry forward:** +- Measure `fdatasync` latency on the target storage vs the existing replication round-trip, to confirm the synchronous-design latency add is acceptable before group-commit is needed. +- Confirm no client-acked operation class bypasses the commit barrier (`wal_classify` audit) — any that do would violate the invariant. +- Decide the `wal_sync_ms` override surface (env var vs CLI flag vs config field) consistent with existing `--sync-timeout-ms` precedent (main.c `OPT_SYNC_TIMEOUT`). +- Re-verify against `raft/raft.tla` and the `CLAUDE.md` Protocol Verification Checklist after the self-count change (commit rule, leader completeness). + +--- + +## Design Synthesis (2026-06-18) + +Decisions resolved into `design.md`: +- **Durability mechanism = synchronous.** Add `persist_t.durable_lsn` (atomic, advanced after `fdatasync`); `persist_wal_async` persists before return; `commit.c:majority_candidate_locked` self-contribution uses `durable_lsn` not `highest_appended_lsn`. **`fsync_worker` removed** as the durability mechanism (checkpoint markers keep their inline fsync). Group-commit batching deferred to roadmap as a strict, contract-preserving optimization. +- **No new module (Option A) for R1/R3; small fence helper (Option C) for R2.** `durable_lsn` is one atomic — a module would be over-abstraction (build-vs-adopt: extend). +- **Recovery: fail-loud + fence.** `replay_wal` returns a verdict (`RECOVER_CLEAN` vs `RECOVER_TRUNCATED`); a truncated member is fenced from promotion and catches up from a healthy peer; FATAL only when it cannot reach a consistent committed state and is required for quorum. Balances `[[feedback_fail_loud_over_timeout]]` with availability. +- **Freshness floor from durable WAL**, not volatile in-memory `last_known_committed_lsn`; safe-commit point at promotion still from voter replies (honors `CLAUDE.md`). +- **Compose with convergence-recovery:** this spec fences *promotion* + verifies recovery integrity; convergence-recovery owns *demotion* truncate and *rejoin* of an intact log. No shared ownership of the commit index (kept volatile per Raft). +- **Liveness:** the added `fdatasync` lands on the already-off-iomgr commit path (`job.c:568-569`), so heartbeats are unaffected — Option A's only real risk is retired. + +Generalizations/simplifications: reuse the existing `in_catch_up` fence concept (extend from streaming-standby to recovery-truncated promoter) rather than inventing a new fence; reuse `persist_commit_barrier` reply gate unchanged; follow `--sync-timeout-ms` and `XCE_TESTING` hook conventions for R3. + +--- + +## Gap Re-Audit (2026-06-18) + +> **Post-implementation gap audit.** All 17 tasks (1.1–5.3) + the never-self-exit +> rework are marked `[x]` (landed 2026-06-18). This pass re-verifies each EARS +> acceptance criterion against the on-disk code (codegraph + grep), tags each +> MET / Missing / Unknown / Constraint, and records the known residual +> test-thoroughness items called out in the Implementation Notes. Line numbers +> from the working tree at audit time; re-confirm before editing. + +### Requirement-to-Asset Map (verified against code) + +#### Requirement 1 — Durable Commit (acknowledgment ⇒ majority persistence) + +| Crit | Asset (file:line) | Tag | +|------|-------------------|-----| +| 1.1 ack ⇒ durable on majority | Reply gated on commit barrier (unchanged) + self durable-capped (`commit.c:93`) + standby persist-before-ACK (`persist.c:761`); end-to-end test `test_durable_majority_commit_end_to_end` | **MET** | +| 1.2 self counts only when durable | `majority_candidate_locked` self-cap `cap = persist_durable_lsn(ce->p)` (`commit.c:93`, was `highest_appended_lsn`); debug invariant `assert(cand <= persist_durable_lsn)` (`commit.c:162`) | **MET** | +| 1.3 standby persists before ACK | `persist_wal_write_replicated` inline `fdatasync` rc-gated, `return -EIO` on failure so the standby does NOT ACK (`persist.c:761-773`); caller `ha_apply_single_entry` ACKs only on rc==0 (no-apply/no-ACK on negative). Pinned by `test_replicated_persist_syncs_before_return` / `test_replicated_sync_failure_no_ack` (counter `test_replicated_fdatasync_count`) | **MET** | +| 1.4 not reported until durable-majority | committed_lsn cannot advance over a non-durable entry (1.2 cap) → existing WAIT-mode reply gate enforces it unchanged | **MET** | +| 1.5 minority loss survives | Property test `test_sole_durable_contributor_loss_survives` (committed ⇒ ≥⌊N/2⌋+1 durable copies, drop one); live failover proof via soak (5.2) | **MET** (live N=2 fault is decision/property-level — see residual G1) | +| 1.6 liveness during durability work | Group-commit flusher (`fsync_worker`) syncs OFF the iomgr/caller thread; `fdatasync` under `p->lock` only (never with `fsync_mutex`); `test_durability_work_off_iomgr_preserves_liveness` asserts flush on a non-caller thread + heartbeat reads <50ms | **MET** | + +Mechanism note: `persist_wal_async` no longer fsyncs inline (`persist.c:643-652` signals the flusher); durability watermark `persist_t.durable_lsn` (`_Atomic uint64_t`) advanced ONLY after a successful `fdatasync` (flusher `persist.c:281-311`, `persist_flush` `persist.c:801-806`). `fsync_worker` repurposed from fixed-interval timer to demand-driven group-commit flusher; checkpoint markers retain their inline `fdatasync` (`persist.c:162`). Legacy `sync_append` ACK-commit store now durable-gated (`sync_append_legacy_commit_store`, store only when `written_lsn <= durable_lsn`). + +#### Requirement 2 — Recovery Integrity + Leadership Fence + +| Crit | Asset (file:line) | Tag | +|------|-------------------|-----| +| 2.1 no silent truncate-accept | `replay_wal(p, from_lsn, recover_verdict_t*)` classifies CRC chain break / CRC mismatch / short header / short payload → `RECOVER_TRUNCATED`; clean EOF / pre-alloc zero tail / empty WAL → `RECOVER_CLEAN` (`persist.c:1411-1605`); verdict stored on `persist_t.recovery_verdict`, accessors `persist_recovery_verdict`/`persist_recovery_truncated` (production, not XCE_TESTING-gated) | **MET** | +| 2.2 ineligible until caught up | Pre-promotion gate in `raft_cb_become_leader` (`ha.c:952-1010`): REFUSE if `persist_recovery_truncated` OR `durable_floor < promotion_safe_lsn` (read AFTER truncate-clamp), set `in_catch_up`, return WITHOUT becoming primary | **MET** | +| 2.3 below-HWM never authoritative | Durable-backed freshness floor: candidate advertises bare `ha_candidate_advertise_lsn = persist_durable_floor` (`ha.c:2587`), voter floor `max(durable_floor, learned)`; promotion gate compares clamped `durable_floor` vs voter-reply `promotion_safe_lsn`; floor re-derived from recovered WAL tip in `persist_recover` (assign, not max) | **MET** | +| 2.4 loud alert, hold non-promotable, STAY ALIVE, never self-exit | `persist_recover_loud_fence_if_unrecoverable` (`persist.c:1787`): loud ERROR with truncation boundary + sticky `_Atomic bool recovery_unrecoverable`, **NO abort/exit/tlog_exit**; `RECOVER_DECISION_FENCE_UNRECOVERABLE` (non-terminating). grep confirms zero abort/exit on the recovery path; `persist_fatal_hook` removed from production (only a test comment remains) | **MET** | +| 2.5 preserve durable-commit at degraded/minimal quorum | Hollow node refused regardless of quorum size (gate is unconditional on truncated/behind) + 4.2 freshness floor; `test_promotion_gate_hollow_refused_at_minimal_quorum` (decision-level, Req 2.5) | **MET** (decision-level; no dedicated live N=2 fault-injection — residual G1) | +| 2.6 re-eligibility on catch-up | Fence/flag clear automatically when `!truncated && durable_floor >= HWM`: `ha_evaluate_promotion_fence` ELIGIBLE branch clears `in_catch_up` (`ha.c:592-597`); `promotion_fence_timeout_fn` releases + `persist_clear_recovery_unrecoverable` on catch-up (`ha.c:755-763`); gate ELIGIBLE branch clears + cancels timer (`ha.c:986-993`) | **MET** | + +Bounded-window catch-up (task 4.4): `raft_cb_become_leader` NEVER FATALs/aborts synchronously — on FENCE/would-be-unrecoverable it arms a one-shot `promotion_fence_timeout_fn` (`DEFAULT_RECOVERY_CATCHUP_WINDOW_MS=30000`) that re-reads live state, releases on catch-up, re-emits the loud alert + re-arms if still hollow with no peer — never exits (`ha.c:721-807`). UAF-safe cancel; cancelled in `ha_fini`. + +#### Requirement 3 — Deterministic Durability Verification + +| Crit | Asset (file:line) | Tag | +|------|-------------------|-----| +| 3.1 configurable durability cadence | `--wal-sync-ms` flag (`main.c:67,118`) → `myconf.wal_sync_ms` → `persist_config_set_wal_sync_ms` (`persist.h:130`) applied at BOTH persist_init sites (`myapp.c:169` production, `cluster.c:1284` phase2); 0-sentinel = keep `DEFAULT_WAL_SYNC_MS`. Test seam `persist_test_durable_lsn_on_disk` (read-only on-disk CRC-validated durable LSN) + `persist_test_set_force_fsync_fail` fault injection, all XCE_TESTING-gated (`persist.h:445-506`) | **MET** | +| 3.2 ack ⇒ recoverable after ungraceful kill in verify-mode | Deterministic E2E (`test-commit-durability.sh --deterministic`): with `--wal-sync-ms 30000`, in-window primary `fdatasync >= 1` (=2) precedes ack ("durable-before-ack"), flipped RED→GREEN stable 3/3 / 9-9 probes (5.2) | **MET** | +| 3.3 same observable ack semantics with/without verify-mode | Documented as a property in the test header (`test_commit_durability.c:33-34`): verify-mode "changes nothing observable about the acknowledgment contract" — but there is **NO dedicated A/B assertion** running the same op with mode on vs off and asserting identical observable acknowledgment | **PARTIAL** (residual G2) | + +Constraint compliance: `safe_committed_lsn` arithmetic UNTOUCHED (voter-reply-sourced, honors `CLAUDE.md` "never truncate from stale ctrlhello"). raft.tla election/freshness logic maps faithfully; raft.tla does **NOT** model durability (restores `log` intact on restart, `raft.tla:165-174`) — the commit-durability invariant is verified by tests 2.x/5.1/5.2 + the manual Protocol Verification Checklist, NOT by raft.tla. (Tagged **Constraint** — by-design deferral, see residual G3.) + +### Residual Gaps + +| ID | Gap | Severity | Notes | +|----|-----|----------|-------| +| **G1** | Req 2.5 has no dedicated **minimal-quorum live fault-injection** test (N=2, lose the sole durable contributor across a real failover). Current coverage = decision/property-level (`test_promotion_gate_hollow_refused_at_minimal_quorum`, `test_sole_durable_contributor_loss_survives`) + the rolling-SIGKILL soak. | **LOW** | Invariant is enforced by the unconditional gate + freshness floor and the soak observed zero loss; a targeted N=2 case would harden the assertion against a future regression but is not a correctness gap. | +| **G2** | Req 3.3 has no **verify-mode A/B observable-equivalence** assertion (same op, mode on vs off, identical externally observable acknowledgment). Property is asserted only by header comment + the structural fact that the seam is read-only / production-excluded. | **LOW** | The seam (`persist_test_*`, `--wal-sync-ms`) demonstrably changes only timing, not the reply path (reply gate unchanged). An explicit A/B test would close the letter of 3.3. | +| **G3** | raft.tla does **not** model durable-vs-volatile log; the formal model cannot prove the commit-durability invariant. | **LOW (by design)** | Explicitly deferred (task 5.3 note, requirements out-of-scope). Durability is test-backed. A durable-log TLA+ extension is out of scope unless explicitly opted in. | + +No CRITICAL/HIGH residuals. No criterion is **Missing** or **Unknown**: 16/16 verifiable EARS criteria are MET (1.1–1.6, 2.1–2.6, 3.1–3.2) with 3.3 PARTIAL (property held but not A/B-asserted). The never-self-exit invariant (Req 2.4) is upheld — grep/nm confirm no abort/exit/tlog_exit on the recovery/promotion path. + +### Integration Risks + +- **Group-commit flusher latency on a degrading disk** (Implementation Note 2.1 follow-up): the flusher tight-spins on SUSTAINED `fdatasync` failure (releases `p->lock` each iter so liveness holds, but CPU/log burn). A HANGING (not erroring) `fdatasync` blocks appends since it runs under `p->lock` — pre-existing, not introduced here. Non-blocking; carry to roadmap with group-commit batching. +- **Test-fixture durable priming (bug-704 class)**: any commit-engine / election fixture that primes `current_lsn`/`applied_lsn`/`last_known_committed_lsn` but NOT `durable_lsn` will see floor/advertise=0 and committed-stall, because both the self-cap (2.2) and the freshness floor (4.2) now read durable state. Future tests MUST prime `durable_lsn` to the modeled tip. (4 fixtures already corrected; bug-760 caught one adjacent real-flusher test.) +- **fsync-able filesystem for validation**: `/tmp` overlay in some sandboxes EIOs on `fdatasync`; because committed_lsn is now durable-gated, an EIO correctly HOLDS committed_lsn and surfaces as adjacent-test failures. Final validation MUST run on a real fsync-able fs. Gate on ANSI-stripped `:FAIL:` count (meson `Fail:0` masks Unity sub-group failures). + +### Options for the Open Test-Thoroughness Items (G1, G2) + +**Option A — Close both with targeted unit/integration tests (recommended).** +- G1: add an N=2 minimal-quorum integration test reusing the `test_sole_durable_contributor_loss_survives` two-persist-dir harness, but drive it through a real failover-style commit + drop-the-durable-contributor and assert the acked op is present on the survivor. +- G2: add an A/B test that submits the same op with verify-mode (`--wal-sync-ms` slow / forced) and without, asserting byte-identical externally observable acknowledgment (same rc, same reply gate release semantics). +- **Effort: S (1–3d).** **Risk: Low** — test-only, no production change; reuses existing harnesses/seams. + +**Option B — Cover via the soak/E2E harness only.** +- Extend `test-commit-durability.sh` with an N=2 deterministic round (G1) and a mode-on/mode-off ack-equivalence probe (G2). +- **Effort: S (1–2d).** **Risk: Low-Medium** — E2E is less deterministic than unit; the strace-gap probe is the established deterministic surface, so prefer it over joblist-loss for G2. + +**Option C — Accept as documented residuals (status quo).** +- Record G1/G2 as LOW-severity known limitations; rely on the gate/floor invariants + the zero-loss soak already in place. +- **Effort: none.** **Risk: Low** — acceptable given no CRITICAL/HIGH gap, but leaves the letter of Req 3.3 (A/B) and a dedicated Req 2.5 live case unasserted, so a future regression in those exact paths could go uncaught until soak. + +**Recommendation:** Option A — both items are S-effort, test-only, and turn two LOW residuals into fully-asserted criteria; G3 stays a by-design deferral. + +--- + +## REOPENED 2026-06-19 — cold-restart-all (total-cluster-outage) committed-job loss (bug-795) + +**Trigger:** the `xce-test-e2e-primitives` spec converted `test-commit-durability.sh` to exit-code primitives and ADDED a new `run_cold_restart_recovery` phase (kill ALL 3 controllers simultaneously, then restart ALL from disk, then assert every acknowledged job recovers). Running it live on a 3-node cluster surfaced a NON-DETERMINISTIC committed-job loss that this spec's original validation never exercised. + +**What was observed (3 live soak runs, sandbox 3-node cluster):** +- Rolling **single-node** SIGKILL + restart: ZERO loss every round (consistent with this spec's 5.2 result — single-host loss is non-deterministic and standbys fdatasync-before-WalAck, so a committed job is durable off the primary). +- **Cold-restart-all** (kill ALL 3, restart ALL): a job that returned `jobsubmit ok` AND survived every rolling round VANISHED after the full restart (`expect-jobs expected N, actual N-2`; `expect-job not found`). PASSED in one run, FAILED in another → a **race**. Recovery logs show WAL `truncate` on the rejoining nodes. + +**Why this spec did not catch it:** 5.2's soak was rolling **single-node** kills only; the durability guarantee was validated (and CLAUDE.md flipped to "GUARANTEED") against that scenario. The **total simultaneous outage** path — where ALL members lose their volatile/page-cache tail at once and recovery must reconstruct committed state from on-disk WAL alone — was never tested. When every node dies together, the "durable off the primary" assumption that makes single-host loss non-deterministic no longer applies: only what was `fdatasync`'d on a majority before the ack can survive. + +**Open question — REAL durability gap vs OVERLAYFS artifact (must resolve first):** +- This sandbox's `/tmp` is **overlay** (NOT a real fsync-able fs). This spec's own notes repeatedly flag that overlay `/tmp` can EIO on `fdatasync` and that "final validation must run on a real fsync-able fs." +- If `fdatasync` on overlay is weak (returns rc=0 without true durability) OR if recovery's truncation (`replay_wal` / `persist_truncate_wal_above` / durable-floor re-derivation, tasks 3.1/3.4) is too aggressive after a total outage, a committed entry's tail could be lost or truncated on cold restart. +- The durable-gating (2.2/2.4) should make `committed_lsn` HOLD if `fdatasync` failed — so an un-fdatasync'd job should NOT have returned `jobsubmit ok`. Yet these jobs DID get `jobsubmit ok`. So either `fdatasync` returned success on overlay without real durability, or a non-durable-gated ack path exists under the total-outage timing, or recovery truncates committed entries. + +**Disposition:** the `xce-test-e2e-primitives` soak is the reproduction surface and is working correctly (it detects the loss via exit codes). The fix (if real) belongs HERE. See Phase 6. diff --git a/.kiro/specs/ha-commit-durability/spec.json b/.kiro/specs/ha-commit-durability/spec.json new file mode 100644 index 0000000..7edadcc --- /dev/null +++ b/.kiro/specs/ha-commit-durability/spec.json @@ -0,0 +1,24 @@ +{ + "feature_name": "ha-commit-durability", + "created_at": "2026-06-17T23:40:42Z", + "updated_at": "2026-06-19T00:00:00Z", + "language": "en", + "phase": "tasks-generated", + "reopened_at": "2026-06-19T00:00:00Z", + "reopened_reason": "bug-795: total-cluster-outage (cold-restart-all) committed-job loss surfaced by the xce-test-e2e-primitives soak; Phase 6 added (6.1 reproduce-on-real-fs gates the rest).", + "approvals": { + "requirements": { + "generated": true, + "approved": true + }, + "design": { + "generated": true, + "approved": true + }, + "tasks": { + "generated": true, + "approved": true + } + }, + "ready_for_implementation": true +} diff --git a/.kiro/specs/ha-commit-durability/tasks.md b/.kiro/specs/ha-commit-durability/tasks.md index 1bbe185..8072ffc 100644 --- a/.kiro/specs/ha-commit-durability/tasks.md +++ b/.kiro/specs/ha-commit-durability/tasks.md @@ -126,6 +126,32 @@ - _Requirements: 1.2, 2.3_ - _Depends: 5.1_ +- [ ] 6. REOPENED 2026-06-19 — total-cluster-outage (cold-restart-all) committed-job loss (bug-795) + > Surfaced by the `xce-test-e2e-primitives` soak's new cold-restart-all step (kill ALL 3 + restart ALL). Phase 5 only validated rolling SINGLE-node failover; the total simultaneous outage path was never tested. Sequential by design (each task gates the next). DO NOT start a fix before 6.1 resolves real-vs-artifact. +- [ ] 6.1 Reproduce cold-restart-all loss on a REAL fsync-able fs (rule out the overlayfs artifact) + - Run the converted `test-commit-durability.sh` (incl. its `run_cold_restart_recovery` phase) against a live 3-node cluster on a REAL fsync-able filesystem (NOT container overlay `/tmp`) — e.g. a tmpfs-free ext4/xfs deploy dir. Repeat enough rounds to characterize the race (it is non-deterministic: ~1-in-2 in sandbox runs). Capture controller recovery logs (WAL `truncate`, durable-floor, `committed_lsn`) for a losing run. + - Observable: a definitive verdict — EITHER the loss reproduces on a real fs (⇒ genuine total-outage durability gap, proceed to 6.2) OR it does NOT reproduce on a real fs and only occurs on overlay `fdatasync` (⇒ overlayfs artifact; record as an environment limitation, close Phase 6 with a note, and the `xce-test-e2e-primitives` task 7.2 unblocks on a real-fs run). + - _Requirements: 1.1, 1.5_ + - _Boundary: ValidationOnly (no production change) — `src/tools/test-commit-durability.sh` is owned by xce-test-e2e-primitives; consume it as-is._ +- [ ] 6.2 Root-cause the total-outage recovery loss (conditional on 6.1 = REAL) + - Instrument the recovery path for a simultaneous all-node SIGKILL + restart: determine whether (a) a job returned `jobsubmit ok` without being `fdatasync`'d on a fault-tolerant majority (a Req-1 commit-gate hole specific to the total-outage timing), (b) recovery (`replay_wal` / `persist_truncate_wal_above` / durable-floor re-derivation, tasks 3.1/3.4) truncates committed entries on cold restart, or (c) a just-rejoined-then-immediately-cold-killed BEHIND node re-leads with a truncated log and overwrites the good timeline (a promotion-fence/freshness-floor hole under cold restart, tasks 4.2/4.3). + - Observable: the exact mechanism is identified with a captured trace; classified as commit-gate / recovery-truncate / promotion-fence; mapped to the specific Requirement(s) and code path it violates. + - _Requirements: 1.1, 1.4, 1.5, 2.3, 2.6_ + - _Boundary: RecoveryIntegrity / DurableCommit (controller persist/commit/ha)_ + - _Depends: 6.1_ +- [ ] 6.3 Fix the total-outage durability/recovery path + add a deterministic regression (conditional on 6.2) + - Repair the identified mechanism so an acknowledged job survives a simultaneous all-node kill+restart, honoring the never-self-terminate invariant (loud fence, stay alive, keep retrying — see `src/controller/CLAUDE.md` Process Lifecycle). Add a deterministic cold-restart-all regression (prefer a unit/integration harness over the racy soak; the soak's `run_cold_restart_recovery` step stays as the E2E backstop). + - Observable: the cold-restart-all loss no longer reproduces (real fs); the new regression is RED on the unfixed build and GREEN after; the full controller suite passes under ASAN with `:FAIL:`==0. + - _Requirements: 1.1, 1.4, 1.5, 2.3, 2.6_ + - _Boundary: DurableCommit / RecoveryIntegrity / PromotionFence_ + - _Depends: 6.2_ +- [ ] 6.4 Re-validate and reconcile the durability claim + - Re-run the deterministic regression + the cold-restart-all soak on a real fs (GREEN); reconcile `src/controller/CLAUDE.md` "Commit Durability — GUARANTEED" to state the total-outage scenario is now covered (or, if 6.1 found an artifact, qualify the overlay-fs caveat). Unblock `xce-test-e2e-primitives` task 7.2 and re-run its soak to close it. + - Observable: durability guarantee holds for BOTH rolling single-node failover AND total-cluster-outage cold restart, backed by the new regression; CLAUDE.md is accurate; bug-795 closed. + - _Requirements: 1.1, 1.2_ + - _Boundary: DurableCommit + docs_ + - _Depends: 6.3_ + ## 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`. - 1.2: Test seam = `persist_test_durable_lsn_on_disk(persist_t*)` (decl `persist.h`, impl `persist.c`, all `#ifdef XCE_TESTING`) — re-opens WAL segment files from disk and CRC-validates the chain (read-only, state-free) to return the highest fully-persisted LSN; new test `testsuite/test_commit_durability.c` (registered in `meson.build` + `controller_test.c`). Production `xce_controller` links without it (proven via `nm`). DRY NOTE: ~115 lines of segment-scan/CRC-chain logic are duplicated from the static `replay_wal`; tasks 3.1 (recovery verdict) and 3.4 (durable floor from WAL tip) touch the SAME scan/CRC logic — consider extracting a shared read-only `wal_scan_durable_lsn`-style helper there and routing this seam through it (avoid a third copy). `XCE_TESTING` is defined only for `controller_unit_test` (`src/controller/meson.build:167`). @@ -146,3 +172,4 @@ - REWORK (2026-06-18, user invariant — a controller MUST NEVER self-terminate): tasks 3.3/4.3/4.4 had implemented Req 2.4 "fail loud" as a SELF-ABORT (`persist_recover_fatal_if_unrecoverable → tlog_exit(); abort()` + `persist_fatal_hook`). That violated the invariant (a self-exited controller needs MANUAL admin recovery + leaves quorum degraded; a live retrying node self-heals when a peer returns) and was the true root cause of the 5.2 soak quorum collapse. CORRECTED: removed the abort/exit terminal entirely. New: `persist_recover_loud_fence_if_unrecoverable(persist_t*)` (NO abort) logs a loud operator-visible ERROR (truncation boundary vs cluster HWM) + sets sticky `_Atomic bool recovery_unrecoverable` (accessors `persist_recovery_unrecoverable`/`persist_clear_recovery_unrecoverable`); the member STAYS ALIVE, permanently non-promotable/fenced (`in_catch_up` + the sticky flag both consulted by the promotion gate), and KEEPS RETRYING catch-up. Decision enum `RECOVER_DECISION_FATAL` → `RECOVER_DECISION_FENCE_UNRECOVERABLE` (non-terminating). 4.4 timer no longer aborts — it re-emits the loud operator alert + re-arms (operator visibility), never exits. Flag + fence CLEAR automatically on catch-up (not truncated AND durable_floor ≥ HWM) → re-eligible (Req 2.6), fully automatic. `persist_fatal_hook` removed. Verified: grep/nm show NO abort/exit/tlog_exit on the recovery path; ASAN suite :FAIL:==0; reworked tests (row4/4b → loud_fence, unrecoverable-stays-alive-loud, released-on-catchup, cancel-stops-realert) PASS. Spec updated: requirements.md Req 2.4 + design.md (decision table rows 4/4b, RecoveryIntegrity, traceability, FATAL-vs-fence open question) reworded "FATAL/terminate" → "LOUD PERMANENT FENCE (non-promotable, stays alive, keeps retrying — NEVER self-exit)". PROJECT-WIDE GUIDANCE added: `src/controller/CLAUDE.md` §"Process Lifecycle (DESIGN INVARIANT)" + `.kiro/steering/ha-async-io.md` — controllers never self-terminate. (`ha_self_restart` execv-restart in convergence/demotion is a SEPARATE primitive, untouched.) - 5.2: PASS on the reworked (no-self-exit) build. (a) DETERMINISTIC E2E flipped RED→GREEN, stable 3/3 runs / 9-9 probes — in-window primary fdatasync ≥1 (=2) precedes the ack ("durable-before-ack"), the inverse of 1.3's unfixed RED. (b) ROLLING SIGKILL soak (6 rounds) exit 0 = ZERO acknowledged-job loss, cluster stayed continuously available, 0 self-terminations (the no-self-exit rework removed the abort that collapsed quorum in the first attempt). Validation-only (no production code). NOTE: ran in-sandbox via background+poll on a worktree deploy dir (avoids /tmp overlay EIO); the earlier collapse was the abort terminal, now corrected — a host re-run is still good practice but the in-sandbox soak now passes with the fix. - 5.3: Formal re-verification (conceptual — no TLA+ toolchain in env; project practice = manual alignment + the CLAUDE.md Protocol Verification Checklist). Checklist walked against the integrated impl, all hold: votedFor/currentTerm (untouched); `commitIndex`/`committed_lsn` monotonic + never exceeds `durable_lsn` (2.2/2.4 debug-assert); leader commits via per-entry majority ack (unchanged); `safe_committed_lsn` from voter replies (untouched, 4.2/4.3); WAL truncation only above the safe point + `durable_lsn` clamped on truncate; freshness gate term-then-index with durable-backed LSN (4.1 pinned vs raft.tla `mlastLogIndex`/`LastTerm`; candidate bare `durable_floor`, voter floor `max(durable_floor, learned)`); contiguous-prefix commit; `durable_lsn` prefix watermark; causal-order append; and the NEW item — a member counts toward the commit majority only at its durable LSN, acknowledged⇒durable-on-majority — now TRUE. raft.tla alignment: the election/freshness logic maps faithfully; raft.tla does NOT model durability (assumes a durable log, restores `log` intact on restart `raft.tla:165-174`) so it CANNOT prove the commit-durability invariant — that invariant is backed by tests 2.x/5.1 + the deterministic RED→GREEN E2E + the zero-loss/available soak (5.2), NOT by raft.tla. `src/controller/CLAUDE.md`: "Commit Durability — KNOWN GAP (bug-661)" flipped to "GUARANTEED / CLOSED (landed 2026-06-18)" with the mechanism list (incl. the no-self-exit loud fence); the post-`ha-commit-durability` checklist item ticked `[x]`. ALL 17 tasks (1.1–5.3) + the no-self-exit rework COMPLETE. +- VALIDATION FIX (2026-06-18, `/kiro-validate-impl` finding bug-760): the feature-level validation caught a deterministic 6/6 FAIL of the ADJACENT-spec test `test_fact_hook_append_is_retained_and_nonblocking` (`test_commit_barrier_rollout.c:446`, "Expected 1 Was 0"). ROOT CAUSE = task 2.2's durable-gating: committed_lsn now requires the async group-commit flusher to advance durable_lsn, but this real-persist test checked committed_lsn SYNCHRONOUSLY the instant after the covering ACK (before the flusher fdatasync'd). Task 2.2 had primed durable_lsn in the in-memory `barrier_setup_vcpus` stubs but MISSED this real-flusher test. CORRECTION of the earlier impl-notes (3.1/5.1) claim that this was a pre-existing `/tmp`-overlay fdatasync-EIO artifact: that is FALSE in this env — fdatasync returns rc=0 on /tmp here and the test passed pre-spec; it was a genuine durable-gating regression. FIX: insert `persist_flush(g_fp)` + assert `persist_durable_lsn(g_fp) >= fact_lsn` BEFORE `repl_on_ack` (commit_on_ack runs synchronously inside repl_on_ack, so the now-durable self completes the majority deterministically). Production was never affected (RETAIN facts have no client waiter; WAIT-mode replies block on the commit barrier `commit.c:334`). Verified: full controller suite 0 failures across 4 runs + meson `Ok:1 Fail:0`. GATING NOTE: meson `Fail:0` masks Unity sub-group failures — gate on the ANSI-stripped `:FAIL:` count from the test binary stdout. diff --git a/src/controller/CLAUDE.md b/src/controller/CLAUDE.md index 472ce2b..31c4b00 100644 --- a/src/controller/CLAUDE.md +++ b/src/controller/CLAUDE.md @@ -78,7 +78,7 @@ The file `raft/raft.tla` contains Diego Ongaro's TLA+ specification of the Raft **Commit Ordering** (contiguous, in log order — RULE): `committed_lsn` advances only as a contiguous prefix in LSN/append order; entries are never committed or reordered out of log order. The single-writer append lock (`p->lock`) is the serialization point. Durability flushing (incl. group-commit batching) makes a contiguous **prefix** durable only — `durable_lsn` is a prefix watermark, never a per-entry set, so a later entry can never be durable while an earlier one is not. Log order must preserve **causal dependencies** (append a cause before its effect — e.g. job submit before job start); independent concurrent events take an arbitrary but consistent total order, which is linearizable via the single serialization point plus commit-gated reply. Out-of-order / parallel commit is NOT supported by this single-log architecture (it would require per-entity logs with dependency tracking — explicitly out of scope). -**Commit Durability** (acknowledged ⇒ durable on a majority — GUARANTEED): when the controller replies success to a client, the operation's entry is `fdatasync`'d on a fault-tolerant majority (the acking member + ⌊N/2⌋ others); a member counts toward the commit majority for an LSN only once that LSN is durable locally (self caps at `durable_lsn`, not the appended tip). **CLOSED — bug-661, spec `ha-commit-durability` (landed 2026-06-18).** Mechanisms: `majority_candidate_locked` caps the primary self-contribution at `persist_durable_lsn` (not `highest_appended_lsn`); `fsync_worker` is a demand-driven group-commit flusher that advances `durable_lsn` only after a successful `fdatasync` and wakes the commit eval (fail-loud rc-gate — `durable_lsn` never advances on `fdatasync` error); the standby `fdatasync`s before its WalAck and the ACK is gated on that sync's success; recovery distinguishes a clean vs corruption-truncated WAL (`recover_verdict_t`) and re-derives the durable floor (WAL tip); the vote/PreVote freshness floor and a candidate's advertised position are durable-backed (candidate advertises bare `persist_durable_floor`, voter floor = `max(durable_floor, last_known_committed_lsn)`), and a pre-promotion gate fences a recovery-truncated/behind member (it catches up before leading; if genuinely unrecoverable it stays a loud, non-promotable, *alive* fence — never self-exits — and self-heals on peer return). Verified by unit/integration tests (2.x, 5.1), the deterministic ack-before-fdatasync gap E2E (RED→GREEN), and the rolling-failover soak (zero acked-job loss, cluster stays available) — NOT by `raft.tla`, which assumes a durable log (restores `log` intact from stable storage on restart, `raft.tla:165-174`) and so cannot model the durability invariant; that invariant rests on these tests + this checklist. +**Commit Durability** (acknowledged ⇒ durable on a majority — GUARANTEED): when the controller replies success to a client, the operation's entry is `fdatasync`'d on a fault-tolerant majority (the acking member + ⌊N/2⌋ others); a member counts toward the commit majority for an LSN only once that LSN is durable locally (self caps at `durable_lsn`, not the appended tip). **CLOSED — bug-661, spec `ha-commit-durability` (landed 2026-06-18).** Mechanisms: `majority_candidate_locked` caps the primary self-contribution at `persist_durable_lsn` (not `highest_appended_lsn`); `fsync_worker` is a demand-driven group-commit flusher that advances `durable_lsn` only after a successful `fdatasync` and wakes the commit eval (fail-loud rc-gate — `durable_lsn` never advances on `fdatasync` error); the standby `fdatasync`s before its WalAck and the ACK is gated on that sync's success; recovery distinguishes a clean vs corruption-truncated WAL (`recover_verdict_t`) and re-derives the durable floor (WAL tip); the vote/PreVote freshness floor and a candidate's advertised position are durable-backed (candidate advertises bare `persist_durable_floor`, voter floor = `max(durable_floor, last_known_committed_lsn)`), and a pre-promotion gate fences a recovery-truncated/behind member (it catches up before leading; if genuinely unrecoverable it stays a loud, non-promotable, *alive* fence — never self-exits — and self-heals on peer return). Verified by unit/integration tests (2.x, 5.1), the deterministic ack-before-fdatasync gap E2E (RED→GREEN), and the rolling-failover soak (zero acked-job loss, cluster stays available) — NOT by `raft.tla`, which assumes a durable log (restores `log` intact from stable storage on restart, `raft.tla:165-174`) and so cannot model the durability invariant; that invariant rests on these tests + this checklist. **CAVEAT — UNDER INVESTIGATION (bug-795, ha-commit-durability REOPENED 2026-06-19):** the verification above covers **rolling single-node failover**. A new **total-cluster-outage (cold-restart-all: kill ALL members + restart)** test (added by the `xce-test-e2e-primitives` soak) NON-DETERMINISTICALLY lost a committed job on an overlayfs sandbox (recovery logged WAL `truncate`). Real-durability-gap vs overlay-`fdatasync` artifact is being resolved by `ha-commit-durability` Phase 6 (6.1 reproduce-on-a-real-fs first). Until 6.x closes, treat the guarantee as **proven for rolling failover; pending confirmation under simultaneous total outage**. ### Post-Election Safe Commit Point (Critical)