lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

checkpoint: bug-851 root cause + raft TLA+ conformance spec family

Browse source 
Investigation & fixes (failover committed-job loss):
- root-cause bug-851 on an owned 3-node cluster: chaotic failover causes
  silent committed-prefix divergence (WAL hdr.term always 0; two leaders
  per term; no (term,lsn) reconcile). Evidence in repro-851/ + INVESTIGATION.
- live-validated fixes 848/849/850 (recovery-on-promotion, advertise real
  applied_lsn, persist-layer contiguity guard); add waldump WAL inspector.

Specs (strict raft.tla conformance — library owns the log):
- TLA-gap-analysis.md: impl vs raft/raft.tla, gaps G1-G6.
- raft-log-replication, raft-commit-safety, raft-sim-conformance: extend
  src/lib/raft to own log/commit/verification via callbacks (specs generated,
  cross-spec review passed).
- ha-term-safety: controller-side adapter + config marker (R1-R7); design done.
- config-upgrade-snapshot: R8 descoped here (deps entity-bearing-snapshots, bug-857).
- roadmap + buglog (851/854/857) + .wolf knowledge updated.
This commit is contained in:
Rongsong Shen 2026-06-21 14:02:15 +00:00
parent 7d70820aac
commit 20e59d0b42
85 changed files with 10525 additions and 1993 deletions
Download patch file Download diff file Expand all files Collapse all files

73
.kiro/specs/ha-commit-durability/brief.md Normal file
View file

@ -0,0 +1,73 @@
# Brief: ha-commit-durability
## Problem
A 3-node HA controller failover test **permanently lost committed jobs**. The client submitted 10 jobs across several forced primary kills; the 7 earliest (JobIds `0-0-1781708993..1781709035`, WAL LSN ≤20) — which had been replicated and majority-ACKed, and survived the *first* failover — were gone by the end, while only the 3 latest survived. Losing an acknowledged, committed job is a correctness (data-loss) failure, the most severe class for this system.
## Current State
The HA consistency machinery is implemented (commit engine, WAL replication, convergence/truncate-on-demotion, single-leader own-term gate, PreVote, vote-freshness bug-339). But **"committed" does not imply "durable on a majority"**:
- **WAL fsync is asynchronous** — a timer thread (`persist.c:173 fsync_worker`) fdatasyncs every `wal_sync_ms`. `persist_append` does **not** fsync before returning; the standby does **not** fsync before sending its `WalAck`.
- **The commit engine advances `committed_lsn` on majority *ACK*** (`commit.c:303 commit_on_ack`, `repl_wait_majority_ack`). An ACK only proves the entry reached the OS page cache — it can be durable on **zero** disks at the moment it is declared committed.
- **Recovery silently truncates to nothing.** When killed nodes restart, `persist_recover` hits a CRC chain break at a zeroed/never-fsync'd region (`ctrl-3.log:1597` `prev_crc=00000000` at offset 40) and stops replay at ~0 entries — but the node still becomes eligible.
- **A hollow node wins and overwrites the good timeline.** With durability lost on the killed nodes and failover quorum collapsed to `required=1`, the freshness gate cannot fence the stale candidate. ctrl-3 became PRIMARY term=4 advertising `committed_lsn=21` over an empty job table (`ctrl-3.log:1612`); the node that *did* hold the data (ctrl-1, applied_lsn=22) converged **down** to it (`ctrl-3.log:1618`). LSN ≤20 discarded forever.
This violates the Leader-Completeness assumption documented in `src/controller/CLAUDE.md` ("committed entries are on a majority" presumes majority-ACK ⇒ durable-on-a-majority) and the TLA+ mapping `log[i] = WAL on disk`.
## Desired Outcome
1. An operation reported committed is **durable on a majority of nodes** before it is declared committed — surviving simultaneous crash + restart of any minority.
2. A node that cannot recover its WAL to a valid committed tip is **fenced from leadership** (fail-loud), never silently truncating to zero and then leading; the cluster never converges onto a timeline below its known committed high-water mark.
## Approach
Two coupled requirements (chosen scope: *Durability + promotion fence*):
- **R1 Durable commit (fsync-before-ACK):** the standby `fdatasync`s the appended WAL bytes **before** emitting `WalAck`; the primary counts its own vote toward majority only after its local `fdatasync`. `committed_lsn` advances only on a **durable** majority. Preserve throughput via group-commit / batched fdatasync (amortize the sync across pending appends) rather than one sync per entry. Keep `wal_sync_ms` async path only for non-committing/best-effort writes, if any remain.
- **R2 Recovery & promotion fence:** a WAL CRC/replay failure that prevents reconstructing a contiguous committed tip makes the node **ineligible to become primary** until it has caught up from a healthy peer (FATAL or fence, never silent truncate-to-zero-then-lead). A node must not win election or self-promote advertising a `committed_lsn` below the cluster's known committed high-water mark, even under degraded single-voter quorum.
Honor the user's fail-loud preference: surface the real durability/recovery failure rather than masking it.
## Scope
- **In**: fsync-before-ACK on the standby apply path and primary self-count; durable-majority gating of `committed_lsn`; group-commit batching to bound the latency cost; fencing an un-recovered/corrupt-WAL node from promotion; preventing a sub-high-water-mark leader from being elected under degraded quorum; unit tests for each; the 3-node live E2E that reproduces this incident (kill-primary-under-load, assert zero committed-job loss).
- **Out**: WAL wire format / ACK message shape (`ha-wal-replication`); per-LSN majority math (`ha-commit-engine` — consumed, the *durability precondition* of its ACK input is what changes); demotion truncate/self-restart and rejoin reconciliation of an *intact* log (`ha-convergence-recovery`); promotion no-op / own-term gate (`ha-single-leader-role`); raw vote-grant arithmetic (`raft-library` — this spec adds the durability+high-water *precondition*, not new vote math); entity-bearing snapshots (roadmap); client-side failover (`libxce-failover`).
## Boundary Candidates
- Durable-commit seam: standby `fdatasync`-before-`WalAck` + primary self-fdatasync before majority self-count (the "ACK ⇒ durable" guarantee).
- Promotion-eligibility seam: recovery-integrity + committed-high-water check gating `raft_cb_become_leader` / PreVote eligibility.
## Out of Boundary
- Tuning the async `fsync_worker` cadence as a "fix" (it is the defect for committing writes, not a knob to turn).
- Changing how a majority is *counted* (commit-engine) or how votes are *granted* (raft-library) beyond adding the durability/high-water precondition.
- Worker-reported fact resync across restart (`worker-rejoin-resync`, roadmap backstop).
## Upstream / Downstream
- **Upstream**: `ha-commit-engine` (committed_lsn semantics it now gates on durable ACKs), `ha-wal-replication` (ACK/catch-up machinery this makes durability-correct), `persist.c` fsync infrastructure, `ha-prevote` + vote-freshness (bug-339) eligibility path it strengthens.
- **Downstream**: `ha-convergence-recovery` E2E (its single-outcome guarantee depends on this), `component-failover-adoption` and any real failover deployment.
## Existing Spec Touchpoints
- **Extends/strengthens**: `ha-wal-replication` (durability precondition on ACK), `ha-commit-engine` (durable-majority), `ha-convergence-recovery` (assumes intact persisted committed state — this spec makes that assumption true).
- **Adjacent (avoid overlapping)**: `ha-single-leader-role`, `raft-library`, `ha-prevote`.
## Constraints
- C / gnu17, `src/controller/persist.c`, `commit.c`, `repl.c`, `ha.c`, `cluster.c`; TDD mandatory (tests in `testsuite/` before C changes).
- Async-I/O rule: never block the iomgr thread — fdatasync must not stall heartbeat/vote handling; use the dedicated persist/fsync thread + group-commit and signal the commit waiter on completion.
- Must verify against `raft/raft.tla` and the `src/controller/CLAUDE.md` Protocol Verification Checklist (commitIndex monotonic; leader holds all committed entries).
- Live E2E needs `/xce-deploy-ha` (cluster at `/tmp/xce-ha-deploy`); reproduction harness = `xce_test` driving jobsub + primary kills. An automated soak reproduction now exists at `src/tools/test-commit-durability.sh` (rolling SIGKILL+restart under load, asserts every acked job survives).
- Performance: durable-majority adds sync latency to the submit path — group-commit is required, not optional.
## Reproduction Findings (2026-06-17, from building the E2E)
- **bug-661 is a RACE, not a deterministic failure.** `wal_sync_ms` is hardcoded to `DEFAULT_WAL_SYNC_MS = 10` ms (`config.h:86`) with **no CLI/env/config override**. The committed-but-not-fsync'd window is ~10ms, so a single-node SIGKILL+restart only loses a committed job when the kill lands inside that window AND the truncated node re-leads before catch-up repairs its WAL. The soak reproduced real loss once (4 committed jobs) but passed an 8-round run clean — confirming the rarity.
- **Required test seam (NEW spec item): a configurable fsync interval** (e.g. `--wal-sync-ms`, plumbed into `persist_config_t.wal_sync_ms`). Setting it to several seconds makes every committed entry provably un-fsync'd at kill time, turning the racy soak into a **deterministic** RED. This seam is also the natural way to unit-test fsync-before-ACK. None exists today — add it as part of Requirement 1's testability (cf. the convergence spec's XCE_TESTING restart seam).
- **Acceptance asymmetry**: a soak FAIL proves the bug; a soak PASS does not prove durability (the race may simply not have fired). After the fix, the soak must pass *deterministically regardless of timing* — that, plus the deterministic `--wal-sync-ms` test, is the real acceptance bar.
- **Amplifier confirmed**: loss needs a killed+restarted node to re-lead with a truncated WAL *before* replication catch-up repairs it. The fix must therefore also fence a node that has not caught up to the committed high-water mark from leading (Requirement 2).
## Design Decision — durability mechanism (2026-06-17, with user)
**Invariant (user-stated, authoritative):** when the controller replies **ok** to a client, the operation's WAL entry MUST already be persisted (fdatasync'd) on a fault-tolerant majority — self + ⌊N/2⌋ standbys.
**Exact defect located:** `commit.c:majority_candidate_locked` counts the primary's own contribution as `cap = highest_appended_lsn(p)` — every *appended* (page-cache) entry, not every *fdatasync'd* one. On 3 nodes a commit needs self + 1 standby; the standby ACK is durable (`persist_wal_write_replicated:515` fdatasyncs before replying) but the primary's copy is **not** (it relies on the `fsync_worker` timer). So a "committed" entry is durable on **exactly 1 node** → zero durability fault-tolerance. That is bug-661.
**Chosen direction (user): synchronous persistence — eliminate `fsync_worker` as the durability mechanism.**
- The submit path **already blocks off the iomgr thread** waiting for majority replication (`job.c:568-569`; `persist_commit_barrier → commit_submit(COMMIT_MODE_WAIT)`), so adding the primary's own `fdatasync` on that path is liveness-safe — it does not stall heartbeats.
- Minimal correctness change: (1) primary `fdatasync`s its entry before its self-contribution covers that LSN (symmetric with the standby path at `persist.c:515`); (2) `commit.c` self-contribution uses a `durable_lsn` (highest fdatasync'd), not `highest_appended_lsn`; (3) reply stays gated on the commit barrier → reply-ok ⇒ durable-majority.
- **Throughput**: per-entry fdatasync. The dominant amplifier (a job array fanning out to N entries → N fsyncs) is removed by the roadmap item **job-array-single-wal-record** (one WAL record per array → one fsync). Cross-reference that item; this spec assumes it lands or is acceptable interim.
- **Group-commit IS in scope (revised 2026-06-18 at design-review).** Initially deferred, but `/kiro-validate-design` found the highest-volume WAL path is `WAL_MODE_RETAIN` facts (job start/finish/fail/rusage) — not array-collapsible, not covered by job-array-single-record — so a pure per-entry synchronous design penalizes the dominant writes. Decision (with user): repurpose `fsync_worker` into a demand-driven group-commit flusher (batch `fdatasync`, advance `durable_lsn`, wake commit-eval) as part of this spec. The roadmap "WAL group-commit batching" item is absorbed here.
- **Open for design:** whether to delete the `fsync_worker` thread entirely or retain a minimal best-effort flush for any non-client-acked / local-only writes (e.g. checkpoint markers already fsync inline; audit `wal_classify` for any write that gates no reply). Confirm no client-acked op bypasses the commit barrier.