Add Raft/KV regression tests for view-straddling rollback interleavings

[Amaury Chamayou (achamayou)]

Motivation

#8209 (and #8223, stacked on it) set out to fix view-straddling transactions. That problem has since been fixed on main by #8242 and #8295, leaving one question: are the two correctness concerns raised against #8209 real, and is main exposed to them?

1. Review of Raft: Assign a term during commit rather than execution, to fix view-straddling transaction behaviour #8209: rejecting a stale entry in Raft::replicate() rolls back to last_idx, discarding a concurrent current-view transaction which is then replicated anyway. It lands in the ledger but not the KV, and the next transaction reuses its seqno and is never replicated.
2. Fix stale entry being published to ledger history after rollback #8223: a rollback between Store::commit() releasing version_lock and appending to the history publishes a root that is in neither the KV nor the ledger.

This PR pins both invariants with deterministic tests over a real Store and single-node Aft. Both pass on main. Test 1 fails on the #8209 branch exactly as described. Each test was also mutation-checked on main (a #8209-style rollback on term mismatch; dropping the view passed to append_entry()), and each caught its mutation. That should be enough to close #8209 and #8223, and to guard the KV/consensus boundary during #8184.

Implementation summary

view_straddling_common.h holds the harness: single-node Aft over a real Store, reelect() (step down, win a later view, which rolls back to the last committable index), and PausingMovePendingTx, which pauses inside Store::commit() after the view check, modelling the committing thread blocked on aft::State::lock during an election. Waits are bounded and workers are released and joined on any failure.

view_straddling_rollback.cpp:

sequenceDiagram
    participant A as Tx A
    participant S as Store
    participant R as Raft
    participant B as Tx B
    Note over S,R: baseline at 1, view V
    A->>S: apply write, TxID V.2
    A->>S: Store::commit, view check V == V passes
    Note over A: descheduled before consensus
    R->>R: election, wins view W
    R->>S: rollback to 1 under W (A's write discarded)
    B->>S: apply write, TxID W.2
    Note over B: paused before Store::commit
    A->>R: replicate(V.2)
    R-->>A: rejected, V != W, no rollback
    Note over S: B's write must survive
    B->>S: Store::commit, view check W == W passes
    B->>R: replicate(W.2)
    R-->>B: accepted, last_idx 2
    Note over S,R: KV, last_idx and ledger agree, next tx gets W.3

Loading

view_straddling_history.cpp:

sequenceDiagram
    participant A as Tx A
    participant S as Store
    participant H as History
    participant R as Raft
    Note over S,R: baseline at 1, view V, root r1
    A->>S: apply write, TxID V.2
    A->>S: Store::commit, view check V == V under version_lock
    Note over A: version_lock released, descheduled
    R->>R: election, wins view W
    R->>S: rollback to 1 under W
    S->>H: rollback to 1 under W, root back to r1
    A->>H: append_entry(digest, expected view V)
    H-->>A: dropped, V != W
    Note over H: observed root must still be r1
    A->>R: replicate(V.2)
    R-->>A: rejected
    Note over S,R: store 1, root r1, ledger 1

Loading

CMakeLists.txt adds both sources to raft_test, which now links ccf_kv. Test inventory is unchanged.

Safety and compatibility

Test-only; no runtime code changes, so no security, consensus, data-format or mixed-version impact. The tests exercise existing guards on main: no rollback on term mismatch in Raft::replicate(), and the view check in TxHistory::append_entry().

[Copilot]

🟡 Changes recommended

Four unused structured-binding elements fail raft_test under the repository's -Wall -Wextra -Werror settings.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds deterministic C++ regressions for view-straddling rollback behavior across KV, Raft, and ledger history.

Changes:

• Adds shared Store/Aft interleaving harnesses.
• Tests KV/ledger consistency and stale history publication.
• Registers tests and links ccf_kv into raft_test.

Custom instructions used

• .github/copilot-instructions.md
• .github/instructions/reviewing.instructions.md
• Testing and formatting/linting skills

File summaries

File	Description
src/consensus/aft/test/view_straddling_common.h	Shared rollback test harness
src/consensus/aft/test/view_straddling_rollback.cpp	KV/Raft rollback regression
src/consensus/aft/test/view_straddling_history.cpp	Ledger-history regression
CMakeLists.txt	Registers sources and KV linkage

Review details

Suppressed comments (3)

src/consensus/aft/test/view_straddling_history.cpp:134

• The rolled_back_term structured-binding element is never used. With -Wall -Wextra -Werror on raft_test, this produces an unused-variable error; use _ for the ignored tuple element, consistently with the other tuple destructuring in the repository.

    const auto [rolled_back_txid, rolled_back_root, rolled_back_term] =

src/consensus/aft/test/view_straddling_history.cpp:145

• The observed_term structured-binding element is never used. With -Wall -Wextra -Werror on raft_test, this produces an unused-variable error; use _ for the ignored tuple element.

    const auto [observed_txid, observed_root, observed_term] =

src/consensus/aft/test/view_straddling_history.cpp:157

• The final_term structured-binding element is never used. With -Wall -Wextra -Werror on raft_test, this produces an unused-variable error; use _ for the ignored tuple element.

    const auto [final_txid, final_root, final_term] =

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

An unbounded synchronization wait can stall the test process instead of reporting a regression.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/consensus/aft/test/view_straddling_common.h:87

• This cannot model blocking on aft::State::lock: PendingTx::call() runs before the history append and before Consensus::replicate() (src/kv/store.h:1058,1085,1123), so the Raft lock has not been requested at this pause point. Describe this as a deliberate descheduling point before history publication/consensus to avoid claiming lock-order fidelity the harness does not provide.

  // pauses first, modelling the committing thread being descheduled after
  // Store::commit() has checked the transaction's view and released
  // version_lock, but before it reaches consensus. In production this is the
  // thread blocking on aft::State::lock while an election holds it.

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Balanced