Data race: Aft::is_primary() and is_candidate() read leadership_state without state->lock

[achamayou]

Describe the bug

Aft::is_primary() reads state->leadership_state without holding state->lock, while every write to that field takes the lock. It is a data race, reported by ThreadSanitizer against a concurrent election.

src/consensus/aft/raft.h:

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

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

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

The unsynchronised accessors sit directly next to can_replicate(), which does take the lock, so this reads as an oversight rather than a deliberate relaxed-read design.

leadership_state is written on every election transition, all under state->lock (raft.h ~2120, ~2165, ~2222, ~2276, ~2399):

state->leadership_state = ccf::kv::LeadershipState::PreVoteCandidate;
state->leadership_state = ccf::kv::LeadershipState::Candidate;
state->leadership_state = ccf::kv::LeadershipState::Leader;
state->leadership_state = ccf::kv::LeadershipState::Follower;
state->leadership_state = ccf::kv::LeadershipState::None;

To Reproduce

Observed under ThreadSanitizer while driving a real Store, MerkleTxHistory and aft::Aft concurrently with elections. The most easily reachable racing reader is ccf::kv::Store::commit():

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

which races an election thread writing leadership_state under state->lock.

Expected behavior

Reads of leadership_state should be synchronised with the writes.

Additional context

This is worth care, because the two obvious fixes both have a catch.

1. Taking state->lock inside is_primary() would deadlock.

Store::commit() calls is_primary() while holding 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(state->current_view); (in become_leader(), three lines before the leadership_state = Leader write)
• raft.h:2625 store->compact(idx);
• raft.h:2707 store->rollback({get_term_internal(idx), idx}, state->current_view);

So locking the accessor inverts an existing order.

2. Making the member std::atomic ripples into serialisation.

State is JSON-serialised, and leadership_state is a required field:

DECLARE_JSON_TYPE_WITH_OPTIONAL_FIELDS(State);
DECLARE_JSON_REQUIRED_FIELDS(
  State, node_id, current_view, last_idx, commit_idx, leadership_state, membership_state, pre_vote_enabled);

std::atomic<LeadershipState> has no nlohmann serialiser, so this needs to_json/from_json adapters (or a non-atomic projection for serialisation). std::atomic is also non-copyable, which may affect State construction/copying.

A third option is std::atomic_ref at the access sites, leaving the member type and serialisation untouched, but that only works if every access is converted; there are readers beyond is_primary(), e.g. details.leadership_state = state->leadership_state; (raft.h:605).

Affected readers are not limited to Store::commit(). is_primary() has call sites in src/node/node_state.h (4), src/kv/store.h (2, including Store::compact() deciding generate_snapshot), and src/node/rpc/node_interface.h.

For context: this was found while working on #8242, and an earlier revision of that PR simply dropped the && get_consensus()->is_primary() conjunct from Store::commit(). That was removed again, because it silences one caller of a racy accessor rather than fixing the accessor, and it changes commit behaviour on backups, which that PR does not need. Filing this separately so the accessor can be fixed on its own merits.