Fix data races on Aft leadership state accessors

[Amaury Chamayou (achamayou)]

Closes #8247.

Independent of the #8242..#8246 concurrency stack - based on main, touches no files that stack touches.

The race

Aft::is_primary() reads mutex-protected state without the mutex:

bool is_primary() override
{
  return state->leadership_state == ccf::kv::LeadershipState::Leader;   // no lock
}

bool can_replicate() override
{
  std::unique_lock<ccf::pal::Mutex> guard(state->lock);                 // takes the lock

leadership_state is written on every election transition, all under state->lock (PreVoteCandidate, Candidate, Leader, Follower, None). The unsynchronised accessors sit directly beside one that does lock, so this reads as an oversight rather than a deliberate relaxed read. ThreadSanitizer reports it against a concurrent election.

The most easily reached racing reader is Store::commit():

std::lock_guard<ccf::pal::Mutex> vguard(version_lock);
if (txid.view != term_of_next_version && get_consensus()->is_primary())

Why not just take the lock

That deadlocks. Store::commit() calls is_primary() under version_lock, which would create version_lock -> state->lock. The opposite edge already exists - Aft calls into the Store from under state->lock, and those entry points take version_lock:

• raft.h:2219 store->initialise_term(...) in become_leader(), three lines before the leadership_state = Leader write
• raft.h:2625 store->compact(idx)
• raft.h:2707 store->rollback(...)

So the value has to be made safe to read without the lock, rather than moved under it.

The change

leadership_state becomes atomic, and all 31 access sites become explicit load()/store(). This fixes all seven is_primary() call sites - four in node_state.h, two in store.h (including Store::compact() deciding generate_snapshot), one in node_interface.h - not just the one TSAN happened to hit.

std::atomic is neither copyable nor movable, so it cannot be a field of a type declared with the DECLARE_JSON_* macros: DECLARE_JSON_REQUIRED_FIELDS expands to t.field = it->get<decltype(TYPE::field)>(), which must return by value. A small wrapper restores value semantics for serialisation while keeping every access atomic, which leaves State's declaration untouched - so no other field's serialisation can have changed.

Acquire/release ordering, so a reader that observes Leader also observes the writes the new leader made before the transition.

Why this is minimal

• No behaviour change, and no change to the serialised form.
• No new locks, and no change to lock ordering.
• State's DECLARE_JSON_* declaration is unchanged.
• Cost is a relaxed atomic load in place of a plain load, on paths that were already doing a virtual call.

Testing

• raft_test (1,001,014 assertions), kv_test, map_test, history_test, snapshotter_test, snapshot_test, frontend_test all pass.
• Compiles under both CCF_RAFT_TRACING and VERBOSE_RAFT_LOGGING, which are the only builds that serialise and log this field.
• Serialisation verified byte-identical to main. Serialising a State before and after gives exactly:

  {"commit_idx":5,"current_view":3,"last_idx":7,"leadership_state":"Leader","membership_state":"Active","node_id":"n0","pre_vote_enabled":true}
  

  and the from_json round-trip is preserved. This matters because the CCF_RAFT_TRACING JSON feeds TLA+ trace validation.
• clang-tidy and clang-format clean.

No CHANGELOG entry: no user-facing API or behaviour change.

Labelled run-long-test.

Follow-up from review

Aft::primary() had the same defect in the adjacent accessor: it returned leader_id unsynchronised, while every write to it (raft.h 1213, 1850, 2130, 2175, 2232, 2281, 2407) happens under state->lock, and get_details() already reads it under that lock at 604.

leader_id is a std::optional<NodeId>, so it cannot be made atomic. It does not need to be: unlike is_primary(), primary() is not called from Store::commit() under version_lock, and is never called from inside Aft, so taking state->lock there introduces no ordering risk. The adjacent can_replicate() already takes the same lock from the same callers.

[Copilot]

Pull request overview

This PR fixes a ThreadSanitizer-reported data race in the AFT consensus implementation by making State::leadership_state safe to read from unsynchronised accessors (eg. Aft::is_primary()), while preserving existing lock ordering and JSON serialisation behavior used by raft tracing.

Changes:

• Introduce AtomicLeadershipState (atomic-backed, JSON round-trippable wrapper) and switch State::leadership_state to use it.
• Update AFT code paths to use explicit load()/store() for leadership-state reads/writes (including logging/tracing).

Custom instructions used:

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

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/consensus/aft/raft.h	Update all leadership-state reads/writes to atomic load()/store() to remove racy unsynchronised reads.
src/consensus/aft/impl/state.h	Add AtomicLeadershipState wrapper and update State to keep JSON serialisation intact while making leadership-state access atomic.

---

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