Raft: Assign a term during commit rather than execution, to fix view-straddling transaction behaviour

[Eddy Ashton (eddyashton)]

#8184 describes some related context.

While trying to reason about the Raft API surface and lock-ordering, we've gone down a long rabbit hole triggered from this block, in store.h:commit:

        std::lock_guard<ccf::pal::Mutex> vguard(version_lock);
        if (txid.view != term_of_next_version && get_consensus()->is_primary())
        {
          // This can happen when a transaction started before a view change,
          // but tries to commit after the view change is complete.
          LOG_DEBUG_FMT(
            "Want to commit for term {} but term is {}",
            txid.view,
            term_of_next_version);

          return CommitResult::FAIL_NO_REPLICATE;
        }

Thought process, roughly, for posterity:

• This get_consensus->is_primary() call from the KV back into consensus is risky, needs careful thoughts about locking.
• But actually, why do we gate on is_primary() at all? If we were primary when this transaction started, but there's been a term change and we've now stepped down to backup (or candidate), surely we have the same early-out as we cannot commit this anymore? This only catches the extremely rare cases where you've stepped down and won a subsequent election while the transaction is in-flight.
• But if we just delete the is_primary() check, lots of things start failing. This early-out is invalid, we need to do the rollback (that for the non-primary path previously happens inside replicate()) to undo all of the locally-visible side effects that happen during the commit flow.
• So we delete this block entirely, sounds good.
• But then what is our view-straddling protection? We currently pass a single view to replicate(), but the batch could cross multiple views. Are we safe to commit things from old views?
• We've done a conflict detection, so these things are still valid...
• Why are we calling such a transaction 2.5 anyway? If it reads 2.4, but we're in term 3 by the time it commits, isn't it 3.5?
• If we stare at the code, it looks like we've needlessly grabbed a term-of-next-commit value alongside our opacity version! Surely we can decide the term of a transaction later in its execution?
• If it weren't for serialisation, I'd say we could let Raft itself determine the term of a transaction? It agrees to replicate something under its lock, and the current term at that point is the canonical term of the transaction? But that's speculative, we do need it for serialisation (IVs!), so we need it earlier.
• Surely we can ask the store for a next_txid() then, where we previously had next_version(), and only pin the transaction's term at that point?
• And the robot can write unit tests for this straddling behaviour that we were previously missing.
• But there's a surprising amount of knock-on changes...
• And somehow at the end of all of that, we've restored the early-out, but its now a valid thing to do...

[Eddy Ashton (eddyashton)]

Changed the shape of this function. It no longer takes a single, explicit view, instead each entry in BatchVector has a full TxID, rather than just a Version. That causes a bunch of churn in test cases, which are constructing these function arguments inline.

The return value also changes. It's no longer all-or-nothing replication - we may accept a prefix, and then hit an entry we refuse to replicate. So the return is not a boolean, but a count of how many entries were accepted for replication. If any were not then the replicate() call should have already called rollback() to restore a consistent state.

[Eddy Ashton (eddyashton)]

Lots of Term arguments removed in these interfaces, and some helper functions erased as a result (they're no longer used).

[Copilot]

Pull request overview

This PR refactors the KV-to-consensus replication interface so transactions carry a full TxID (term+seqno) through batching/replication, with the goal of fixing incorrect behavior for “view-straddling” transactions (transactions that begin in one term but attempt to commit/replicate after a view change). It also adds targeted Raft unit tests that exercise the previously missing edge cases.

Changes:

• Change consensus replicate() to accept BatchVector entries keyed by TxID and return a replicated-count (size_t) rather than a boolean.
• Simplify history interfaces by removing “term_of_next_version” threading through TxHistory APIs, and update callers accordingly.
• Add a new AFT unit test suite covering view-straddling transaction scenarios and related rollback/replication interactions.

Custom instructions used:

• .github/copilot-instructions.md
• .github/instructions/reviewing.instructions.md

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/node/test/history.cpp	Update test consensus stubs for new replicate() signature and TxID batch entries.
src/node/test/historical_queries.cpp	Adjust host ledger construction for BatchVector entries keyed by TxID.
src/node/snapshotter.h	Update Tx::commit() call signature usage.
src/node/rpc/frontend.h	Update Tx::commit() usage and simplify replicated-state read TxID handling.
src/node/node_state.h	Adjust replicated-state capture to updated history return tuple.
src/node/history.h	Remove “term_of_next_version” plumbing from TxHistory methods/Null history implementations.
src/kv/tx.cpp	Stop capturing commit term at Tx construction; use current_txid() only.
src/kv/tx_pimpl.h	Remove cached commit_view from Tx pimpl.
src/kv/test/stub_consensus.h	Update stub consensus replication to accept TxID and return replicated count.
src/kv/test/kv_contention.cpp	Update test consensus wrapper for new replicate() signature.
src/kv/store.h	Track pending transactions with TxID, build batched replication using TxID, and handle partial replication via replicated count.
src/kv/kv_types.h	Update core interfaces (BatchVector, Consensus::replicate, history APIs) to use TxID.
src/kv/committable_tx.h	Assign TxID at apply time via next_txid(); switch commit bookkeeping to store applied_txid.
src/kv/apply_changes.h	Replace version resolver with TxIDResolver, returning an optional TxID.
src/indexing/test/common.h	Update indexing test consensus wrapper for new replicate() signature.
src/consensus/aft/test/view_straddling_transactions.cpp	New unit tests covering view-straddling commit/rollback scenarios.
src/consensus/aft/test/main.cpp	Update existing AFT unit tests for TxID-keyed replication entries.
src/consensus/aft/test/driver.h	Update raft test driver replication helper to use TxID.
src/consensus/aft/test/committable_suffix.cpp	Update existing AFT tests for TxID-keyed replication entries.
src/consensus/aft/raft.h	Refactor Raft replicate() to validate term/contiguity per-entry and return replicated count; rollback on partial replication.
CMakeLists.txt	Add new unit test source file and link raft tests with ccf_kv.

---

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