From 9e55f09274360a84d0521a63ee861155589e2317 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 27 Jun 2026 09:38:06 +0000 Subject: [PATCH] docs(raft-ha-core-safety): retarget Phase 2 to cross-term payload loss (post-1.3) Task 1.3 refuted the term-blind hypothesis; real defect = cross-term catch-up payload loss (streamer ships PLEN=0 on short pread/malloc-fail; (term,lsn)-only guard is payload-blind). Retarget: - design: new Catch-up Payload Integrity component (config_repl.c core fix); Persist guard -> (term,lsn,payload-crc); term-blind demoted to defensive; flows/traceability/testing/hypothesis sections updated. - tasks: Phase 2 retargeted (streamer fail-loud + payload-aware guard); new 1.5 (release-log-buffer soak robustness); 7.2 _Depends 1.5_. - research: old root-cause + synthesis D1/D3 marked SUPERSEDED. Independent sanity review PASS after adding the 1.5 dependency edge. --- .kiro/specs/raft-ha-core-safety/design.md | 128 ++++++++++++-------- .kiro/specs/raft-ha-core-safety/research.md | 6 +- .kiro/specs/raft-ha-core-safety/tasks.md | 61 ++++++---- 3 files changed, 120 insertions(+), 75 deletions(-) diff --git a/.kiro/specs/raft-ha-core-safety/design.md b/.kiro/specs/raft-ha-core-safety/design.md index 1b904567..4fcbbc07 100644 --- a/.kiro/specs/raft-ha-core-safety/design.md +++ b/.kiro/specs/raft-ha-core-safety/design.md @@ -4,7 +4,7 @@ This feature makes repeated and chaotic primary failover on an XCE controller HA cluster provably safe and live. It is a **correction-and-verification** spec, not a build-from-scratch one: the `(term, lsn)` commit-identity, conflict-reconcile, Figure-8 commit, and vote-freshness machinery already exist and are validated (`raft-log-replication`, `raft-commit-safety`, `ha-term-safety`), yet the live chaotic soak still produces committed-prefix divergence (LSN 9), standby-rejoin failure, and intermittent election stalls — on this branch and on baseline `21e26a1`. -Discovery root-caused the residual divergence to a **split catch-up path**: term-carrying entries route through the single Raft reconcile seam (`raft_on_append_entries_request` → logOk → `on_truncate_from` → `persist_set_term`), but a **term-blind branch** (`entry->term == 0` → `ha_apply_single_entry` → `persist_wal_write_replicated` direct) bypasses logOk entirely, stamps the standby's own term instead of the leader's, and the persistence layer's **LSN-only idempotency guard silently no-ops** when the correct term-carrying entry later arrives for an already-written LSN — so a divergent committed entry can never be repaired. +Discovery (task 1.3, live real-fs release soak, independently verified — see `research.md`) root-caused the residual divergence to **cross-term catch-up payload loss**, *not* the term-blind-bypass hypothesis the early analysis proposed (that hypothesis is **refuted**: the divergent entry carries the *correct* term and arrives *through* the reconcile seam). At committed LSN 9 all members agree on `term`, but the minority member holds an **empty payload** (`PLEN=0`, `pcrc=0`) where the majority holds the real entry (`PLEN=3560`). The bisect: when a restarted standby catches up an entry whose stored term is below the current leader's term (the bug-1067 cross-term path), the leader-side streamer `ha_stream_wal_to_standby` (`config_repl.c` ~2154-2163) **silently ships `PLEN=0`** on a short `pread` or `malloc` failure instead of failing loud; the receiver faithfully persists the empty body; and the persistence layer's **`(term,lsn)`-only idempotency guard is payload-blind**, so it never detects that an existing-LSN entry now has a divergent payload. The result is a permanently divergent committed prefix that carries the right term but the wrong (empty) data. The design therefore: (1) collapses every entry-ingest path into the one reconcile seam and makes persistence `(term, lsn)`-conflict-aware instead of LSN-only-idempotent; (2) closes the snapshot-boundary and standby-reconvergence gaps; (3) guarantees election liveness under churn; and (4) delivers a deterministic chaotic harness that forces each defect RED before its fix and a green 6+ round real-fs soak as the acceptance gate. @@ -54,13 +54,14 @@ The design therefore: (1) collapses every entry-ingest path into the one reconci ## Architecture ### Existing Architecture Analysis -- **Single reconcile seam (preserve, do not duplicate):** all conflict detection/repair lives inside `raft_on_append_entries_request` (raft.c:646–765). The defect is that not every ingest path enters it. +- **The real defect (task 1.3, verified):** the cross-term catch-up *payload* is lost at its source. `ha_stream_wal_to_standby` (`config_repl.c` ~2154-2163) reads the entry body with `pread` into a `malloc`'d buffer; on a short read or allocation failure it frees the buffer and ships the entry with `PLEN=0` **silently** (no error, no retry). The receiver persists the empty body, and the `(term,lsn)`-only persistence guard cannot tell that an existing-LSN entry now carries a divergent (empty) payload. Term and ingest-path are correct; the data is gone. +- **Single reconcile seam (preserve, do not duplicate):** all `(term,lsn)` conflict detection/repair lives inside `raft_on_append_entries_request` (raft.c:646–765). This seam is sound and is preserved as an invariant — but it is *not* where this bug lives (the divergent entry already passes through it with the correct term). The fix is upstream (streamer payload integrity) and underneath (payload-aware persistence guard). - **Layering (must hold):** Raft library (pure, callback-driven) ← controller HA adapter (`ha.c`, `ha_convergence.c`, `config_repl.c`) ← persistence (`persist.c`, WAL). The adapter calls into the library and implements callbacks; the library never calls the adapter except through callbacks. The fix routes adapter ingest *into* the library, never around it. - **Two distinct truncate contracts (must not be conflated):** - **Promotion-truncate** (a node becoming leader): clamps at `max(safe_committed_lsn, durable_tip)` (ha.c:999–1000, bug-1060) — a fresh leader must not discard its own durable tip. - **Follower-AE-conflict-truncate** (a follower/standby receiving an entry that conflicts at `(term, lsn)`): obeys Raft Log Matching — truncates the conflicting suffix **regardless of local `durable_tip`**. The bug-1060 promotion clamp does **not** apply here. Rationale: an entry that conflicts with the leader's `(term, lsn)` was, by Raft's Leader Completeness, never majority-committed, so it is safe — and required — to remove even if the local node had marked it durable/committed. Applying the promotion clamp on this path would block legitimate repair and convert repairable divergence into a permanent fence. - The only entries the follower-conflict path must never cut are those that match the leader (no conflict) — guaranteed by logOk before any `on_truncate_from`. -- **Technical debt addressed:** the term-blind ingest branch and the LSN-only idempotency guard are the debt this spec removes. +- **Technical debt addressed:** the silent empty-payload ship in the cross-term catch-up streamer and the payload-blind persistence idempotency guard are the primary debt this spec removes. The term-blind ingest branch is retired as *defensive* hardening (it is a latent hazard, not the cause of the observed divergence). ### Architecture Pattern & Boundary Map @@ -75,13 +76,14 @@ graph TB LOGOK --> PERSISTCB end subgraph Adapter [Controller HA Adapter] + STREAM[Catchup Streamer payload integrity] INGEST[Unified Entry Ingest] SNAP[Snapshot Boundary Reconcile] CONV[Standby Reconvergence] ELECT[Election Liveness] end subgraph Persist [Persistence WAL] - GUARD[term lsn conflict aware guard] + GUARD[term lsn payload conflict guard] SETTERM[persist_set_term leader term] TERMIDX[per lsn term index] end @@ -90,6 +92,7 @@ graph TB WALCMP[walcompare term lsn pcrc] SOAK[xce-test-ha chaotic soak] end + STREAM --> INGEST INGEST --> AE SNAP --> AE CONV --> INGEST @@ -102,10 +105,10 @@ graph TB ``` **Architecture Integration** -- **Selected pattern:** funnel-into-single-seam — every ingest path (live replication, catch-up batch, snapshot post-install, rejoin) terminates in `raft_on_append_entries_request`; persistence enforces `(term,lsn)` conflict semantics underneath. -- **Boundaries:** ingest-routing (Adapter), persistence-conflict (Persist), liveness (Adapter election), verification (test-only) are four separable seams under one acceptance gate. +- **Selected pattern:** payload-integrity-at-source + payload-aware-persistence, with the single reconcile seam preserved as an invariant. The cross-term catch-up streamer must never ship a non-empty entry as empty (fail-loud on short `pread`/`malloc`-fail); persistence enforces `(term, lsn, payload-crc)` conflict semantics so a divergent-payload entry at an existing LSN is detected, not idempotent-skipped. +- **Boundaries:** catch-up payload integrity (Adapter streamer), persistence-conflict (Persist), reconvergence + liveness (Adapter), verification (test-only) are separable seams under one acceptance gate. - **Patterns preserved:** single reconcile seam, callback-driven library, truncate-clamp, persist-before-ACK. -- **New components rationale:** only the harness is genuinely new code; everything else removes a bypass or hardens an existing guard. +- **New components rationale:** the harness is genuinely new code; the streamer fix hardens a silent-failure path; the persistence change extends an existing guard from `(term,lsn)` to `(term,lsn,payload-crc)`. - **Steering compliance:** async-I/O (no blocking in vtask handlers; file I/O + restart on delaymgr, never callback thread), fail-loud over silent skip, controller-never-self-terminates-on-operational-condition. ### Technology Stack @@ -114,51 +117,56 @@ graph TB |-------|------------------|-----------------|-------| | Consensus core | `src/lib/raft` (in-repo) | reconcile seam + callbacks (consumed) | no library changes beyond callback use | | Controller HA adapter | C (`ha.c`, `ha_convergence.c`, `config_repl.c`) | ingest routing, snapshot/rejoin, election | primary change surface | -| Persistence | C (`persist.c`, WAL segments) | `(term,lsn)`-aware guard, term index | harden guard, no format change | +| Persistence | C (`persist.c`, WAL segments) | `(term,lsn,payload-crc)`-aware guard, term index | harden guard, no format change | | Verification | C unit (`controller_unit_test`), `walcompare`, `xce-test-ha` bash | RED→GREEN + soak gate | new `test_ha_core_safety.c`; chaotic soak mode | | Runtime | real filesystem (ext4) release build | acceptance soak | overlayfs excluded (bug-924) | ## File Structure Plan ### Modified Files -- `src/controller/ha.c` — Collapse the term-blind ingest branch: `ha_apply_wal_catchup_entry` (ha.c:2233) and `ha_apply_single_entry` (ha.c:2097) route **all** entries through `ha_apply_term_entry_via_library`/`ha_consume_append_entries` (the seam), never `persist_wal_write_replicated` directly. Snapshot post-install (`ha_handle_snapshot_chunk` ha.c:2492) establishes the `(term,lsn)` boundary. Standby reconvergence completion (applied/log_len sync, healthy gating). Election-liveness guarantee on the failover/vote path. -- `src/controller/persist.c` — Make the idempotency/contiguity guard (`persist_wal_write_replicated` persist.c:802, guard at :822/:839) **`(term,lsn)`-conflict-aware**: same LSN + different term ⇒ surface a conflict to the reconcile seam (truncate-then-repersist) rather than `return 0`. Ensure no ingest writes a non-leader term. -- `src/controller/config_repl.c` — Leader-side streamer (`ha_stream_wal_to_standby` config_repl.c:2032) always emits term-carrying entries (no `entry->term == 0` on the wire from a live leader); walAck continuation feeds only the seam path. +- `src/controller/config_repl.c` — **Core fix.** Leader-side catch-up streamer `ha_stream_wal_to_standby` (~2154-2163): on a short `pread` or `malloc` failure when reading an entry body, **fail loud** (error + retry/abort the stream) instead of silently freeing the buffer and shipping `PLEN=0`. A non-empty entry must never be transmitted with an empty payload. Disambiguate and handle both send-side-empty (short read/alloc fail here) and any transitive-empty-propagation source uncovered during implementation. +- `src/controller/persist.c` — Extend the replicated-write guard (`persist_wal_write_replicated` persist.c:802, idempotency guard at :822/:839) from `(term,lsn)`-only to **`(term, lsn, payload-crc)`-aware**: an entry arriving for an existing LSN whose payload-crc differs (e.g. a real entry over a previously-stored empty one, or vice versa) is a conflict to surface for repair, not an idempotent `return 0`. Never accept an empty payload as the durable form of a known-non-empty entry. Preserve persist-before-ACK. +- `src/controller/ha.c` — Receiver-side handling so a recovered/divergent-payload entry is repaired through the reconcile seam (`ha_consume_append_entries` ha.c:2410-2440 body copy; `ha_apply_wal_catchup_entry` ha.c:2233). Snapshot post-install (`ha_handle_snapshot_chunk` ha.c:2492) establishes the `(term,lsn)` boundary. Standby reconvergence completion (applied/log_len sync, healthy gating). Election-liveness guarantee on the failover/vote path. **Defensive:** retire the term-blind `ha_apply_single_entry` (ha.c:2097) direct-persist branch so no future path can bypass the seam. - `src/controller/ha_convergence.c` — Reuse the `ha_rejoin_reconcile` intake hook for chaotic-rejoin reconciliation through the seam; retain the `ha_self_restart` convergence primitive and once-guard. - `src/controller/testsuite/meson.build` — Register `test_ha_core_safety.c`. - `src/controller/testsuite/walcompare.c` — Extend to emit a machine-checkable `(lsn → term, pcrc, present)` per-member report consumable by the harness verdict (committed-vs-uncommitted classification). -- `.claude/skills/xce-test-ha/run-ha-soak.sh` (and `SKILL.md`) — Add a **chaotic mode** (overlapping kills inside election windows) and a verdict that isolates bug-854/bug-924 from the divergence/job-loss result. +- `.claude/skills/xce-test-ha/run-ha-soak.sh` (and `SKILL.md`) — Chaotic mode + isolated verdict (done, tasks 1.1/1.2). **Plus (task 1.3 follow-up):** make primary-detection robust to the release build's block-buffered `--logfile` — switch the soak's primary/convergence detection to a client probe (`ctrlstatus`) rather than log-grep, so the chaotic soak runs unattended on release binaries. +- `src/controller/` log init (only if the client-probe approach is insufficient) — ensure the release build flushes (line-buffers) its `--logfile` so operator-observable progress is not lost until signal. Prefer the soak-side client-probe fix first; treat a controller log-flush change as a fallback that must not alter production logging semantics. ### New Files -- `src/controller/testsuite/test_ha_core_safety.c` — Deterministic RED→GREEN harness: forces the term-blind divergence, snapshot-boundary divergence, standby reconvergence-failure, and election-stall paths using existing gate/probe seams (pattern: `test_ha_dispatch_integration.c`); no sleeps. +- `src/controller/testsuite/test_ha_core_safety.c` — Deterministic RED→GREEN harness: forces the **cross-term catch-up payload-loss** divergence (a non-empty entry streamed/persisted as empty at an existing committed LSN), snapshot-boundary divergence, standby reconvergence-failure, and election-stall paths using existing gate/probe seams (pattern: `test_ha_dispatch_integration.c`); no sleeps. > Dependency direction (enforced): `persist.c` (term/lsn storage) ← `ha.c`/`config_repl.c`/`ha_convergence.c` (adapter ingest/election) → Raft library seam. Tests depend on all; production never depends on tests. ## System Flows -### Divergence repair via unified seam (the fix) +### Cross-term catch-up payload integrity (the fix) ```mermaid sequenceDiagram - participant L as Leader + participant L as Leader streamer participant S as Standby ingest participant Seam as raft_on_append_entries_request - participant P as persist term lsn guard - L->>S: replicate or catch-up entry at LSN N - S->>Seam: route ALL entries no term-blind branch - Seam->>Seam: UpdateTerm then logOk prev term lsn - alt conflict same LSN different term - Seam->>P: on_truncate_from N - Seam->>P: on_persist_entry N leader_term - P->>P: guard sees term conflict repersist not no-op - else match - Seam->>P: on_persist_entry N leader_term + participant P as persist term lsn payload guard + L->>L: read entry body pread into malloc buffer + alt short read or malloc fail + L->>L: FAIL LOUD error retry or abort stream + Note over L: never ship a non empty entry as PLEN 0 + else full body + L->>S: stream entry at LSN N with full payload and term end - P-->>S: WAL holds leader term at N - Note over S,P: walcompare term pcrc identical across members + S->>Seam: route through reconcile seam + Seam->>P: on_persist_entry N term payload + alt existing LSN payload crc differs + P->>P: conflict surface for repair not idempotent skip + else identical term and payload crc + P->>P: idempotent no op safe + end + P-->>S: WAL holds full payload at N + Note over S,P: walcompare term and pcrc identical across members ``` -Key decisions: the term-blind branch is removed (single path); the persist guard distinguishes "same LSN same term" (idempotent no-op, safe) from "same LSN different term" (conflict → truncate+repersist). Truncation never cuts below `max(safe_committed_lsn, durable_tip)`. +Key decisions: the streamer fails loud on a short `pread`/`malloc` failure instead of silently shipping `PLEN=0`; the persistence guard compares `(term, lsn, payload-crc)` so an empty-over-real (or real-over-empty) write at an existing LSN is a conflict to repair, not an idempotent skip. The single reconcile seam is preserved; truncation (when a genuine `(term,lsn)` conflict exists) never cuts below `max(safe_committed_lsn, durable_tip)` on the promotion path and follows the follower-conflict contract on the receive path. ### Standby reconvergence gating @@ -178,8 +186,8 @@ A node is reported healthy/3-of-3 only when committed **and** applied state matc | Requirement | Summary | Components | Key Contracts | Flows | |-------------|---------|------------|---------------|-------| -| 1.1–1.4 | Committed prefix identical; no divergent entry at a position; divergent treated non-authoritative; TLA+ State-Machine/Commit safety | Unified Entry Ingest; Persist Conflict Guard | seam routing; `(term,lsn)` guard | Divergence repair | -| 2.1–2.4 | Entry stamped leader term; never term-0 committed; identity distinguishes conflicts; identity survives restart | Persist Conflict Guard; Unified Entry Ingest | `persist_set_term`; term index | Divergence repair | +| 1.1–1.4 | Committed prefix identical; no divergent entry at a position; divergent treated non-authoritative; TLA+ State-Machine/Commit safety | Catch-up Payload Integrity; Persist Conflict Guard | streamer fail-loud; `(term,lsn,payload-crc)` guard | Payload integrity | +| 2.1–2.4 | Entry carries faithful identity+payload; never empty-over-real at a committed LSN; identity distinguishes conflicts; survives restart | Persist Conflict Guard; Catch-up Payload Integrity | `(term,lsn,payload-crc)` guard; `persist_set_term`; term index | Payload integrity | | 3.1–3.4 | Detect conflict; repair to majority prefix; surface unrecoverable; never cut majority-committed | Unified Entry Ingest; Snapshot Boundary | `on_truncate_from`; follower-conflict truncate contract (logOk-gated) | Divergence repair | | 4.1–4.3 | At most one committing primary/term; step-down on higher term; Election-Safety/Leader-Completeness | Election Liveness (step-down) | vote-once; pre-vote; vote freshness | — | | 5.1–5.4 | Rejoin → 3-of-3 in bound; applied+log_len sync; catch-up completeness; not converged until applied matches | Standby Reconvergence | healthy-gating contract | Reconvergence gating | @@ -191,8 +199,9 @@ A node is reported healthy/3-of-3 only when committed **and** applied state matc | Component | Layer | Intent | Req Coverage | Key Dependencies | Contracts | |-----------|-------|--------|--------------|------------------|-----------| -| Unified Entry Ingest | Adapter | Route every entry through the one reconcile seam | 1,2,3 | Raft seam (P0); Persist guard (P0) | Service, State | -| Persist Conflict Guard | Persist | `(term,lsn)`-conflict-aware write; never non-leader term | 1,2,3 | term index (P0); `persist_set_term` (P0) | State | +| Catch-up Payload Integrity | Adapter | Stream a non-empty entry never as empty; fail-loud on short read/alloc-fail | 1,2,3 | catch-up streamer (P0); Persist guard (P0) | Service, State | +| Persist Conflict Guard | Persist | `(term,lsn,payload-crc)`-conflict-aware write; never accept empty-over-real | 1,2,3 | term index (P0); `persist_set_term` (P0) | State | +| Unified Entry Ingest | Adapter | Route every entry through the one reconcile seam; retire term-blind bypass (defensive) | 1,2,3 | Raft seam (P0); Persist guard (P0) | Service, State | | Snapshot Boundary Reconcile | Adapter | Establish `(term,lsn)` at checkpoint; post-install reconcile | 3,5 | snapshot install (P0); seam (P1) | State | | Standby Reconvergence | Adapter | Gate healthy/3-of-3 on committed+applied match | 5 | catch-up apply (P0); `ha_rejoin_reconcile` (P1) | Service, State | | Election Liveness | Adapter | Always complete an election under churn; step-down on higher term | 4,6 | vote/election path (P0) | Service | @@ -201,14 +210,36 @@ A node is reported healthy/3-of-3 only when committed **and** applied state matc ### Adapter +#### Catch-up Payload Integrity +| Field | Detail | +|-------|--------| +| Intent | The cross-term catch-up streamer must transmit every non-empty entry with its full payload, or fail loud — never silently ship `PLEN=0`. | +| Requirements | 1.1, 1.2, 2.1, 2.3, 3.1 | + +**Responsibilities & Constraints** +- In `ha_stream_wal_to_standby` (`config_repl.c` ~2154-2163): when reading an entry body (`pread` into a `malloc`'d buffer), a short read or allocation failure must be handled as an error (log + retry/abort the stream), **not** by freeing the buffer and sending the entry with an empty payload. +- Disambiguate the two possible sources the reviewer flagged: (a) **send-side empty** — the short-read/alloc-fail here; (b) **transitive-empty propagation** — the streamed source WAL already holds an empty body (itself produced by an earlier instance of this defect). The fix must close (a) directly and, if (b) is observed, trace it to its own streamer instance rather than masking it. +- Cross-term entries (stored term < leader current term, the bug-1067 path) are the trigger seen live — ensure they are read and shipped identically to same-term entries. + +**Dependencies**: Inbound: walAck catch-up continuation (P0). Outbound: Persist Conflict Guard (P0) on the receiver; the reconcile seam (P1). + +**Contracts**: Service [x] / State [x] +- Precondition: the source WAL holds a non-empty body for the entry being streamed. +- Postcondition: the receiver persists the same `(term, lsn, payload-crc)` the leader holds; a transmit failure is surfaced, never silently degraded to an empty entry. +- Invariant: `walcompare` reports identical `(term, pcrc)` for the streamed LSN across leader and standby. + +**Implementation Notes** +- Validation: the RED test reproduces the live signature — a cross-term catch-up of an entry whose body read short/fails must NOT result in a persisted `PLEN=0` entry at a committed LSN; instead the stream errors and retries. Pin send-side vs transitive via an instrumented `payload_len` capture at send and at `on_persist_entry`. +- Risks: failing loud on the stream must not wedge catch-up permanently — pair the error with a bounded retry so a transient short read recovers, while a persistent failure is surfaced (fail-loud over silent skip; controller never self-terminates). + #### Unified Entry Ingest | Field | Detail | |-------|--------| -| Intent | Every replicated/catch-up entry passes `raft_on_append_entries_request` (logOk + `(term,lsn)` conflict + leader-term stamp). No term-blind branch. | -| Requirements | 1.1, 1.2, 2.1, 2.3, 3.1, 3.2, 3.4 | +| Intent | Every replicated/catch-up entry passes `raft_on_append_entries_request` (logOk + `(term,lsn)` conflict + leader-term stamp). Defensively retire the term-blind direct-persist branch so no path can bypass the seam. | +| Requirements | 1.2, 2.3, 3.1, 3.2, 3.4 | **Responsibilities & Constraints** -- Remove the `entry->term == 0` direct-persist branch; route batch/single/catch-up ingest through the seam. +- Defensive: remove the `entry->term == 0` direct-persist branch (`ha_apply_single_entry`); route batch/single/catch-up ingest through the seam. (Not the cause of the observed divergence, but a latent bypass hazard.) - Conflict repair on this (follower/standby) path uses the **follower-AE-conflict-truncate** contract (truncate the conflicting suffix per Raft Log Matching) — **not** the promotion clamp. logOk gates the truncate, so only genuinely-conflicting (never-majority-committed) entries are cut. The promotion clamp applies only to the leader-promotion path, which this component does not touch. - File I/O / truncation / restart never on a callback or iomgr thread — defer to delaymgr (existing pattern). @@ -223,7 +254,7 @@ A node is reported healthy/3-of-3 only when committed **and** applied state matc **Implementation Notes** - Integration: reuse `ha_apply_term_entry_via_library`/`ha_consume_append_entries`; retire `ha_apply_single_entry`'s direct-persist path. -- Validation: RED unit test injects a term-blind/divergent entry at an LSN and asserts the seam repairs it and `persist_set_term` stamped the leader term. +- Validation: RED unit test injects an entry through the term-blind branch and asserts it now routes through the seam and is reconciled. - Risks: the safety boundary on this path is **logOk**, not a durable-tip clamp — only entries that fail the `(term, lsn)` match are truncated, and a failed match proves the entry was never majority-committed (Leader Completeness). A unit test must assert that an entry which *matches* the leader is never truncated, and that a *conflicting* entry below local `durable_tip` *is* truncated. #### Standby Reconvergence @@ -266,22 +297,22 @@ A node is reported healthy/3-of-3 only when committed **and** applied state matc #### Persist Conflict Guard | Field | Detail | |-------|--------| -| Intent | `(term,lsn)`-aware write: same-LSN/same-term is an idempotent no-op; same-LSN/different-term is a conflict surfaced for truncate+repersist; never stamp a non-leader term. | +| Intent | `(term, lsn, payload-crc)`-aware write: identical `(term, payload-crc)` at an LSN is an idempotent no-op; a *differing* payload-crc (or term) at an existing LSN is a conflict surfaced for repair — never a silent `return 0`. An empty payload is never accepted as the durable form of a known-non-empty entry. | | Requirements | 1.1, 1.2, 2.1, 2.2, 2.4, 3.1, 3.4 | -**Responsibilities & Constraints**: replace the LSN-only idempotency check (persist.c:822) with `(term,lsn)` semantics; preserve persist-before-ACK; preserve the term index and `WAL_LEGACY_TERM` mapping. +**Responsibilities & Constraints**: extend the LSN-only idempotency check (persist.c:822) to compare `(term, payload-crc)`; preserve persist-before-ACK; preserve the term index and `WAL_LEGACY_TERM` mapping. **Dependencies**: Inbound: `on_persist_entry`/`on_truncate_from` (P0). Outbound: term index `record_entry_term`/`persist_entry_term` (P0). **Contracts**: State [x] - Precondition: caller has passed the entry through the seam (leader term known via `persist_set_term`; logOk already decided match vs conflict). -- Postcondition: WAL at LSN N holds exactly the leader's `(term, payload)`; no silent retention of a stale-term entry. -- Invariant: never write `term == 0` for a live-replicated entry. On the seam-driven conflict path the guard honors `on_truncate_from` (follower-AE-conflict contract) — it does **not** re-apply the promotion durable-tip clamp, which belongs to the leader-promotion path only. The guard never *initiates* a truncate on its own; it only executes the seam's decision. +- Postcondition: WAL at LSN N holds exactly the leader's `(term, payload)`; no silent retention of a stale-term or empty-payload entry. +- Invariant: never write `term == 0` for a live-replicated entry, and never let a `PLEN=0` write overwrite or shadow a known-non-empty entry at the same LSN. On the seam-driven conflict path the guard honors `on_truncate_from` (follower-AE-conflict contract) — it does **not** re-apply the promotion durable-tip clamp, which belongs to the leader-promotion path only. The guard never *initiates* a truncate on its own; it only executes the seam's decision. **Implementation Notes** -- Integration: the guard distinguishes "already have identical entry" from "have a different entry at this LSN"; the latter routes to the seam's truncate path instead of `return 0`. -- Validation: unit test writes a wrong-term entry at LSN N, then the correct one, and asserts the WAL ends with the leader term (no silent skip). -- Risks: mis-tagging same-term-same-payload as a conflict would cause needless truncation — the guard compares both term and payload-crc. +- Integration: the guard distinguishes "already have identical entry (same term + payload-crc)" from "have a different-payload entry at this LSN"; the latter is surfaced for repair instead of `return 0`. +- Validation: unit test writes an empty-payload entry at LSN N, then the correct full-payload entry, and asserts the WAL ends with the full payload (no silent idempotent skip); and the reverse (full then empty) never degrades a stored entry. +- Risks: mis-tagging an identical `(term, payload-crc)` entry as a conflict would cause needless work — the guard compares both term and payload-crc and treats a true match as idempotent. ### Verify @@ -298,9 +329,9 @@ A node is reported healthy/3-of-3 only when committed **and** applied state matc - Idempotency & recovery: deterministic gate/probe seams (pattern `test_ha_dispatch_integration.c`); teardown drains pending tasks (avoid the known teardown-race UAF class, bug-1224). **Hypothesis-validation discipline (mandatory):** -- The term-blind bypass is the *primary* hypothesis, not a proven sole cause. The RED gate (7.1) must **first reproduce the actual live divergence** — a `walcompare` divergence from the chaotic soak — and bisect it to a concrete seam, **before** the synthetic `(term,lsn)`-injection unit test is accepted as representative. A green synthetic test over a still-red soak is not a pass. -- Note baseline `21e26a1` already carries ha-term-safety 3.4 (retired `term==0` repl_broadcast), so a *live leader* may never emit `term==0`. If the live divergence is therefore **not** the term-blind path, the harness's `walcompare (term,lsn,pcrc)` classification (committed vs uncommitted; which member, which term) drives re-diagnosis before any fix is committed. -- **Contingency**: if the soak still diverges after the term-blind path is closed, treat it as a new finding — classify via `walcompare`, record in `research.md`, and add the next fix task; do **not** mark R1/R8 done on the strength of green unit tests alone. +- The live-repro gate (task 1.3) has **already run and refuted the term-blind hypothesis** (verified): the real defect is cross-term catch-up payload loss (`research.md` §"Task 1.3"). The RED unit/integration cases must therefore reproduce the *observed* signature — a non-empty entry persisted as `PLEN=0` at a committed LSN under cross-term catch-up — not the synthetic term-conflict the earlier draft assumed. +- A green synthetic unit test over a still-red soak is not a pass: R1/R8 close only when the chaotic real-fs soak (task 7.2) reports zero `walcompare` divergence with the streamer + guard fixes in place. +- **Contingency**: if the soak still diverges after the streamer payload-integrity fix and the payload-aware guard land, treat it as a new finding — classify via `walcompare (term,lsn,pcrc)`, record in `research.md`, and add the next fix task; do **not** mark R1/R8 done on the strength of green unit tests alone. (The reviewer flagged one still-open sub-question: send-side-empty vs transitive-empty propagation — disambiguate during the streamer fix.) #### Acceptance Soak Gate | Field | Detail | @@ -327,8 +358,9 @@ A node is reported healthy/3-of-3 only when committed **and** applied state matc ## Testing Strategy ### Unit Tests (`test_ha_core_safety.c`, `controller_unit_test`) -- Wrong-term entry at LSN N then correct entry ⇒ WAL ends with leader term; persist guard did not silently skip (2.1, 2.2, 3.1). -- Term-blind ingest path removed: a `term==0`-shaped catch-up entry still routes through the seam and is reconciled (1.2, 3.2). +- Streamer payload integrity: a cross-term catch-up whose entry-body read returns short / `malloc` fails must NOT persist a `PLEN=0` entry at a committed LSN — the stream errors and retries instead (1.1, 2.1, 3.1). +- Payload-aware guard: an empty-payload write at existing LSN N followed by the correct full-payload entry ⇒ WAL ends with the full payload (guard did not silently idempotent-skip); the reverse (full then empty) never degrades the stored entry (1.1, 1.2, 2.1, 2.2). +- Defensive term-blind retirement: a `term==0`-shaped catch-up entry routes through the seam and is reconciled (no direct-persist bypass) (1.2, 3.2). - Follower-conflict truncate contract: a *conflicting* entry below local `durable_tip` **is** truncated and repaired (the promotion clamp does not block it); a *matching* entry is **never** truncated (3.2, 3.4). - Reconvergence gate: injected apply gap ⇒ node not reported converged until closed (5.2, 5.3, 5.4). diff --git a/.kiro/specs/raft-ha-core-safety/research.md b/.kiro/specs/raft-ha-core-safety/research.md index b6151bb1..2a364d55 100644 --- a/.kiro/specs/raft-ha-core-safety/research.md +++ b/.kiro/specs/raft-ha-core-safety/research.md @@ -85,7 +85,9 @@ Extend/correct in place (A), but build the **deterministic chaotic harness** (R7 # Design-Phase Discovery (kiro-spec-design) -## ROOT CAUSE (term-blind catch-up bypass) — primary hypothesis, harness-confirmable +## ROOT CAUSE (term-blind catch-up bypass) — ⚠️ SUPERSEDED / REFUTED by task 1.3 + +> **This hypothesis was tested live (task 1.3) and REFUTED.** The real defect is cross-term catch-up *payload* loss (see §"Task 1.3 — Live divergence reproduction & bisect" below). The divergent entry carries the correct term and arrives through the reconcile seam; the term-blind path is a latent hazard but not the observed cause. Retained below for the diagnostic record. Deep-trace of the standby catch-up/rejoin paths found a **split** where one branch bypasses the single reconcile seam: @@ -106,6 +108,8 @@ Deep-trace of the standby catch-up/rejoin paths found a **split** where one bran ## Synthesis +> ⚠️ **D1/D3 partially superseded by task 1.3.** The "eliminate every bypass of the one seam / collapse Path 2 into Path 1" framing assumed the term-blind hypothesis, now refuted. The single-seam invariant (D2) and harness-first discipline (D4) still hold; but the actual fix is the cross-term catch-up streamer payload integrity + a `(term,lsn,payload-crc)`-aware guard (see §"Task 1.3"), not seam-bypass elimination. Term-blind retirement remains as defensive hardening only. + - **Generalization (D1):** R1/R2/R3/R4 are one underlying invariant — *every* entry that enters a member's log must pass the single `(term,lsn)` reconcile seam. The fix is not four mechanisms but **eliminating every bypass of the one seam** + making persistence `(term,lsn)`-conflict-aware rather than LSN-only-idempotent. - **Build vs adopt (D2):** Adopt the existing library seam (`raft_on_append_entries_request` + `on_truncate_from` + `persist_set_term`) — already validated (raft-log-replication/raft-commit-safety). Build nothing new in the consensus core; only route into it and harden the persist guard. Reuse `ha_rejoin_reconcile` intake hook (currently stub) rather than a second reconcile path. - **Simplification (D3):** Do not add a separate "divergence repair subsystem" (gap-analysis Option B rejected) — that would create the exact second-reconcile-path class of bug being fixed. Collapse Path 2 into Path 1. diff --git a/.kiro/specs/raft-ha-core-safety/tasks.md b/.kiro/specs/raft-ha-core-safety/tasks.md index 17b87fea..18ea0101 100644 --- a/.kiro/specs/raft-ha-core-safety/tasks.md +++ b/.kiro/specs/raft-ha-core-safety/tasks.md @@ -1,10 +1,10 @@ # Implementation Plan: raft-ha-core-safety > **TDD (project steering, mandatory):** every implementation sub-task is preceded by a testsuite sub-task that lands failing (RED) tests first, then implements to GREEN. The deterministic harness (Phase 1) is the RED infrastructure that pins every later fix to a `walcompare`/invariant violation. -> **No `(P)` markers:** Phases 2–6 all modify `ha.c` (and `persist.c`), and the RED cases share `test_ha_core_safety.c`, so the plan is sequential — concurrent edits would contend on the same files (same constraint as `ha-convergence-recovery`). Foundation Phase 1 establishes shared prerequisites and is also sequential. +> **No `(P)` markers:** Phases 2–6 modify shared files (`ha.c`, `persist.c`, `config_repl.c`) and the RED cases share `test_ha_core_safety.c`, so the plan is sequential — concurrent edits would contend (same constraint as `ha-convergence-recovery`). Foundation Phase 1 establishes shared prerequisites and is also sequential. > **Correctness oracle:** verify every change against `raft/raft.tla` and the invariant checklist in `src/controller/CLAUDE.md` (Election Safety, Leader Completeness, State Machine Safety, Commit Ordering/Durability). > **Verify anchors before editing** (files drift): the design and `research.md` carry 2026-06-27 file:line anchors; re-grep before each edit. -> **Hypothesis discipline:** the term-blind bypass is the primary hypothesis, not proven sole cause. Task 1.3 must reproduce the *live* divergence first; a green synthetic unit test over a still-red soak does not satisfy 1.1/8.1. +> **Root cause (task 1.3, DONE + verified):** the term-blind hypothesis is **refuted**. The real defect is **cross-term catch-up payload loss** — `ha_stream_wal_to_standby` silently ships `PLEN=0` on a short `pread`/`malloc`-fail, and the `(term,lsn)`-only persist guard is payload-blind. Phase 2 targets the streamer + payload-aware guard (`research.md` §"Task 1.3"). A green synthetic unit test over a still-red soak does not satisfy 1.1/8.1 — closure requires the real-fs chaotic soak (7.2) clean. - [ ] 1. Foundation: deterministic chaotic harness and live-divergence reproduction - [x] 1.1 Extend the divergence detector to a machine-checkable per-member report @@ -31,32 +31,41 @@ - _Requirements: 7.1, 7.4_ - _Boundary: Deterministic Chaotic Harness_ -- [ ] 2. Committed-prefix divergence fix: persistence conflict guard and single-seam ingest -- [ ] 2.1 RED: persistence `(term, lsn)`-conflict tests - - Add cases: a wrong-term entry written at an LSN followed by the correct term-carrying entry must end with the leader's term (today the LSN-only idempotency guard silently no-ops); a conflicting entry below local `durable_tip` must be truncatable; a matching entry must never be truncated - - Observable completion: the new persistence cases FAIL on the baseline because the guard is LSN-only and silently skips the corrective write +- [ ] 1.5 Make soak primary-detection robust to the release-build log buffer + - Task 1.3 found the release build block-buffers its `--logfile` and flushes only on signal, so the soak's log-grep primary/convergence detection cannot see an elected primary and false-aborts. Switch the soak's primary + convergence detection to a client probe (`ctrlstatus`/`xce_test`) instead of log-grep, so the chaotic soak runs unattended on release binaries. (Only if a client probe is insufficient, fall back to a controller release log-flush change that does not alter production logging semantics.) + - Observable completion: `run-ha-soak.sh --chaotic` on the `~/tmp` real-fs path with the `src/build.release` binaries detects the elected primary each round via the client probe and no longer false-aborts on "no primary within Ns"; a short (2-round) run reaches its verdict line + - _Requirements: 7.5, 8.3_ + - _Boundary: Acceptance Soak Gate, Deterministic Chaotic Harness_ + - _Depends: 1.2_ + +- [ ] 2. Committed-prefix divergence fix: cross-term catch-up payload integrity + payload-aware guard + - _Root cause (task 1.3, verified): the cross-term catch-up streamer silently ships `PLEN=0` on a short `pread`/`malloc`-fail, and the `(term,lsn)`-only persist guard is payload-blind. Term and ingest-path are correct — the payload is lost._ +- [ ] 2.1 RED: payload-aware persistence guard tests + - Add cases: an empty-payload (`PLEN=0`) entry written at an existing LSN N followed by the correct full-payload entry must end with the FULL payload (today the `(term,lsn)`-only idempotency guard silently no-ops the corrective write); the reverse (full then empty at the same `(term,lsn)`) must NOT degrade the stored entry to empty; an identical `(term, payload-crc)` re-write is a safe idempotent no-op + - Observable completion: the new persistence cases FAIL on the baseline because the guard compares only `(term, lsn)` and cannot see the payload difference - _Requirements: 1.1, 1.2, 2.1, 2.2, 3.1, 3.4_ - _Boundary: Persist Conflict Guard_ -- [ ] 2.2 Make the persistence guard `(term, lsn)`-conflict-aware - - Replace the LSN-only idempotency check: same LSN + same term is an idempotent no-op; same LSN + different term is surfaced as a conflict for the seam to truncate-then-repersist (never a silent no-op); never persist a non-leader term for a live-replicated entry; preserve persist-before-ACK and the term index - - Observable completion: the 2.1 cases pass; the WAL at a re-written LSN holds exactly the leader's `(term, payload)`; persist-before-ACK behavior unchanged - - _Requirements: 1.1, 1.2, 2.1, 2.2, 3.1_ +- [ ] 2.2 Make the persistence guard `(term, lsn, payload-crc)`-aware + - Extend the LSN-only idempotency check (persist.c:822): identical `(term, payload-crc)` at an LSN is an idempotent no-op; a differing payload-crc (or term) at an existing LSN is surfaced for repair, never a silent `return 0`; never accept a `PLEN=0` write as the durable form of a known-non-empty entry; preserve persist-before-ACK, the term index, and `WAL_LEGACY_TERM` + - Observable completion: the 2.1 cases pass; an empty-over-real write at an existing committed LSN no longer silently survives; identical re-writes stay idempotent + - _Requirements: 1.1, 1.2, 2.1, 2.2, 2.4, 3.1_ - _Boundary: Persist Conflict Guard_ -- [ ] 2.3 RED: single-seam ingest and unrecoverable-fence tests - - Add cases asserting that every replicated/catch-up entry — including a `term==0`-shaped legacy entry — passes through the reconcile seam (logOk + conflict-truncate + leader-term stamp) and is reconciled, with no direct-persist bypass branch remaining; and a case where a node that cannot obtain the majority-agreed committed prefix takes the loud non-promotable fence and refuses promotion instead of serving divergent committed state - - Observable completion: the ingest cases FAIL on the baseline because the term-blind branch bypasses the seam; the fence case FAILs because no unrecoverable-divergence fence exists yet - - _Requirements: 1.2, 3.2, 3.3, 7.1_ - - _Boundary: Unified Entry Ingest_ - -- [ ] 2.4 Collapse the term-blind ingest path into the single reconcile seam - - Route batch/single/catch-up ingest through `raft_on_append_entries_request`; retire the direct-persist branch; apply the follower-AE-conflict-truncate contract (logOk-gated, no promotion clamp) so a conflicting entry is repaired regardless of local `durable_tip`; surface genuinely-unrecoverable divergence as a loud non-promotable fence (never self-terminate); defer file I/O/truncation to delaymgr (never the callback/iomgr thread) - - Observable completion: 2.3 passes (including the fence case — an unrepairable node refuses promotion and stays alive retrying); an injected divergent `(term, lsn)` is repaired through the seam; `walcompare` is clean on the 1.3 reproduction path - - _Requirements: 1.1, 1.2, 1.3, 2.1, 2.3, 3.1, 3.2, 3.3, 3.4_ - - _Boundary: Unified Entry Ingest_ +- [ ] 2.3 RED: cross-term catch-up streamer payload-integrity tests + - Add a case reproducing the live signature: a cross-term catch-up (stored term < leader current term, bug-1067 path) whose entry-body `pread` returns short / `malloc` fails must NOT result in a persisted `PLEN=0` entry at a committed LSN — instead the stream must error and retry. Also add the defensive case that a `term==0`-shaped catch-up entry routes through the reconcile seam (no direct-persist bypass), and the unrecoverable-fence case (a node that cannot obtain the majority-agreed committed prefix refuses promotion instead of serving divergent state). Instrument `payload_len` at send and at `on_persist_entry` to disambiguate send-side-empty vs transitive-empty propagation + - Observable completion: the streamer case FAILs on the baseline because the short-read/malloc-fail path silently ships `PLEN=0`; the seam-routing and fence cases FAIL because those paths/fence do not exist yet + - _Requirements: 1.1, 1.2, 2.1, 3.1, 3.2, 3.3, 7.1_ + - _Boundary: Catch-up Payload Integrity, Unified Entry Ingest_ - _Depends: 2.2_ +- [ ] 2.4 Fix the streamer payload integrity + defensive seam routing + fence + - Core: in `ha_stream_wal_to_standby` (config_repl.c ~2154-2163), fail loud on a short `pread`/`malloc`-fail with a bounded retry instead of freeing the buffer and shipping `PLEN=0`; a non-empty entry is never transmitted empty. Disambiguate and handle send-side-empty vs transitive-empty propagation per the 2.3 instrumentation. Defensive: retire the term-blind `ha_apply_single_entry` direct-persist branch so all ingest routes through the seam; surface genuinely-unrecoverable divergence as a loud non-promotable fence (never self-terminate); defer file I/O to delaymgr (never the callback/iomgr thread) + - Observable completion: 2.3 passes (streamer never ships empty; seam-routing and fence cases green); on the 1.3 reproduction path (real-fs release chaotic soak) `walcompare` reports zero committed divergence at the previously-divergent LSNs + - _Requirements: 1.1, 1.2, 1.3, 2.1, 2.3, 3.1, 3.2, 3.3, 3.4_ + - _Boundary: Catch-up Payload Integrity, Unified Entry Ingest_ + - _Depends: 2.2, 2.3_ + - [ ] 3. Snapshot-boundary reconciliation - [ ] 3.1 RED: snapshot post-install reconcile test - Add a case where a divergent pre-snapshot entry must be reconciled against the installed checkpoint's `(term, lsn)` boundary (today snapshot install sets applied-lsn without a `(term, lsn)` reconcile) @@ -99,11 +108,11 @@ - _Boundary: Election Liveness_ - [ ] 6. Integration: single-seam invariant end-to-end -- [ ] 6.1 Route live leader replication and catch-up streaming through the seam only - - Ensure the leader-side streamer always emits term-carrying entries (no `term==0` on the wire from a live leader) and the walAck catch-up continuation feeds only the seam ingest path - - Observable completion: live leader replication carries the leader's term end-to-end; no term-0 entries reach a standby from a live leader; streamed catch-up entries reconcile through the seam +- [ ] 6.1 Verify leader→standby replication carries (term, payload) faithfully end-to-end + - Integration check that the live leader replication AND the cross-term catch-up streaming path transmit each entry's full `(term, payload)` to the receiver: no `term==0` on the wire from a live leader (defensive), and — the real concern — no non-empty entry ever arrives with `PLEN=0`; the walAck catch-up continuation feeds only the seam ingest path + - Observable completion: an end-to-end test (or instrumented soak round) shows leader and standby hold identical `(term, payload-crc)` for every replicated and cross-term-caught-up LSN; `walcompare --report` clean across members - _Requirements: 2.1, 2.3, 3.2_ - - _Boundary: Unified Entry Ingest, Persist Conflict Guard_ + - _Boundary: Catch-up Payload Integrity, Unified Entry Ingest_ - _Depends: 2.4_ - [ ] 6.2 Verify the single-seam invariant and restart identity @@ -123,7 +132,7 @@ - Run the chaotic soak (≥6 rounds, overlapping kills) against real-fs release binaries and assert zero `walcompare` divergence, zero committed-job loss, and 3-of-3 reconvergence plus a new primary after every kill round, with bug-854/bug-924 isolated from the verdict - Observable completion: the soak exits PASS over ≥6 chaotic rounds with zero divergence and zero job loss — the green result that gates the `raft-drive-async` production flag-flip - _Requirements: 1.4, 7.4, 8.1, 8.2, 8.3, 8.4_ - - _Depends: 6.2, 5.2_ + - _Depends: 1.5, 6.2, 5.2_ - [ ] 7.3 Hypothesis-validation closeout - Confirm the live soak divergence is eliminated by the implemented fixes (not by synthetic unit tests alone); if any residual divergence remains, classify it via `walcompare (term, lsn, pcrc)`, record the new finding in `research.md`, and add a follow-up fix task rather than declaring closure