From cdac4a027e7f8042418e8cf164b8361deadab01f Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Fri, 14 Aug 2026 10:10:04 +0000 Subject: [PATCH 1/8] Synchronize public AFT state queries RPC task workers can query consensus state concurrently with Raft message processing. Publish a coherent query snapshot without taking the Raft lock from KV-backed endpoints, avoiding both data races and KV/Raft lock inversion.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/consensus/aft/raft.h | 221 ++++++++++++++++++++++++++++++--------- 1 file changed, 169 insertions(+), 52 deletions(-) diff --git a/src/consensus/aft/raft.h b/src/consensus/aft/raft.h index 286a3e0ebb8..1b122791ea0 100644 --- a/src/consensus/aft/raft.h +++ b/src/consensus/aft/raft.h @@ -124,7 +124,15 @@ namespace aft // Volatile std::optional voted_for = std::nullopt; + // Public consensus queries may run on task workers while Raft messages are + // processed on the enclave main thread. Keep these small query fields + // independently synchronized so endpoint transactions do not need to take + // state->lock and invert the KV/Raft lock order. + mutable ccf::pal::Mutex public_state_lock; std::optional leader_id = std::nullopt; + Index published_last_idx = 0; + Index published_commit_idx = 0; + ViewHistory published_view_history; // Keep track of votes in each active configuration struct Votes @@ -201,6 +209,105 @@ namespace aft // pre-deserialisation, without an additional header. static constexpr size_t max_terms_per_append_entries = 1; + // Called while state->lock is held. + void set_leader_id(const ccf::NodeId& leader) + { + std::lock_guard guard(public_state_lock); + leader_id = leader; + } + + // Called while state->lock is held. + void reset_leader_id() + { + std::lock_guard guard(public_state_lock); + leader_id.reset(); + } + + // Called while state->lock is held. + void set_leadership_state(ccf::kv::LeadershipState leadership_state) + { + std::lock_guard guard(public_state_lock); + state->leadership_state = leadership_state; + } + + // Called while state->lock is held. + void set_current_view(Term view) + { + std::lock_guard guard(public_state_lock); + state->current_view = view; + } + + // Called while state->lock is held. + void advance_current_view(Term increment) + { + std::lock_guard guard(public_state_lock); + state->current_view += increment; + } + + // Called while state->lock is held. + void initialise_log_state( + Index last_idx, + Index commit_idx, + const std::vector& terms, + std::optional> view_update = std::nullopt) + { + state->last_idx = last_idx; + state->commit_idx = commit_idx; + state->view_history.initialise(terms); + if (view_update.has_value()) + { + state->view_history.update(view_update->first, view_update->second); + } + publish_log_state(); + } + + // Called while state->lock is held. + void publish_replicated_entry(Index index, Term view) + { + state->last_idx = index; + state->view_history.update(index, view); + publish_log_state(); + } + + // Called while state->lock is held, or during construction. + void publish_log_state() + { + std::lock_guard guard(public_state_lock); + published_last_idx = state->last_idx; + published_commit_idx = state->commit_idx; + published_view_history = state->view_history; + } + + // Called while state->lock is held. + void update_view_history(Index index, Term view) + { + state->view_history.update(index, view); + } + + // Called while state->lock is held. + void rollback_view_history(Index index) + { + state->view_history.rollback(index); + } + + // Called while state->lock is held. + void set_last_idx(Index index) + { + state->last_idx = index; + } + + // Called while state->lock is held. + void decrement_last_idx() + { + state->last_idx--; + } + + // Called while state->lock is held. + void set_commit_idx(Index index) + { + state->commit_idx = index; + } + public: static constexpr size_t append_entries_size_limit = 20000; std::unique_ptr ledger; @@ -239,6 +346,7 @@ namespace aft ledger(std::move(ledger_)), channels(std::move(channels_)) { + publish_log_state(); if (commit_callbacks != nullptr) { commit_callbacks->set_consensus(this); @@ -249,6 +357,7 @@ namespace aft std::optional primary() override { + std::lock_guard guard(public_state_lock); return leader_id; } @@ -259,11 +368,13 @@ namespace aft bool is_primary() override { + std::lock_guard guard(public_state_lock); return state->leadership_state == ccf::kv::LeadershipState::Leader; } bool is_candidate() override { + std::lock_guard guard(public_state_lock); return state->leadership_state == ccf::kv::LeadershipState::Candidate; } @@ -305,6 +416,7 @@ namespace aft bool is_backup() override { + std::lock_guard guard(public_state_lock); return state->leadership_state == ccf::kv::LeadershipState::Follower; } @@ -404,14 +516,14 @@ namespace aft { // This is unsafe and should only be called when the node is certain // there is no leader and no other node will attempt to force leadership. + std::lock_guard guard(state->lock); if (leader_id.has_value()) { throw std::logic_error( "Can't force leadership if there is already a leader"); } - std::lock_guard guard(state->lock); - state->current_view += starting_view_change; + advance_current_view(starting_view_change); become_leader(true); } @@ -423,19 +535,16 @@ namespace aft { // This is unsafe and should only be called when the node is certain // there is no leader and no other node will attempt to force leadership. + std::lock_guard guard(state->lock); if (leader_id.has_value()) { throw std::logic_error( "Can't force leadership if there is already a leader"); } - std::lock_guard guard(state->lock); - state->current_view = term; - state->last_idx = index; - state->commit_idx = commit_idx_; - state->view_history.initialise(terms); - state->view_history.update(index, term); - state->current_view += starting_view_change; + initialise_log_state( + index, commit_idx_, terms, std::make_pair(index, term)); + set_current_view(term + starting_view_change); become_leader(true); } @@ -449,10 +558,7 @@ namespace aft // before it has received any append entries. std::lock_guard guard(state->lock); - state->last_idx = index; - state->commit_idx = index; - - state->view_history.initialise(term_history); + initialise_log_state(index, index, term_history); ledger->init(index, recovery_start_index); @@ -461,44 +567,50 @@ namespace aft Index get_last_idx() { - return state->last_idx; + std::lock_guard guard(public_state_lock); + return published_last_idx; } Index get_committed_seqno() override { - std::lock_guard guard(state->lock); - return get_commit_idx_unsafe(); + std::lock_guard guard(public_state_lock); + return published_commit_idx; } Term get_view() override { - std::lock_guard guard(state->lock); + std::lock_guard guard(public_state_lock); return state->current_view; } std::pair get_committed_txid() override { - std::lock_guard guard(state->lock); - ccf::SeqNo commit_idx = get_commit_idx_unsafe(); - return {get_term_internal(commit_idx), commit_idx}; + std::lock_guard guard(public_state_lock); + return { + published_view_history.view_at(published_commit_idx), + published_commit_idx}; } Term get_view(Index idx) override { - std::lock_guard guard(state->lock); - return get_term_internal(idx); + std::lock_guard guard(public_state_lock); + if (idx > published_last_idx) + { + return ccf::VIEW_UNKNOWN; + } + return published_view_history.view_at(idx); } std::vector get_view_history(Index idx) override { - // This should only be called when the spin lock is held. - return state->view_history.get_history_until(idx); + std::lock_guard guard(public_state_lock); + return published_view_history.get_history_until(idx); } std::vector get_view_history_since(Index idx) override { - // This should only be called when the spin lock is held. - return state->view_history.get_history_since(idx); + std::lock_guard guard(public_state_lock); + return published_view_history.get_history_since(idx); } // Same as ccfraft.tla GetServerSet/IsInServerSet @@ -706,13 +818,12 @@ namespace aft should_sign = false; } - state->last_idx = index; ledger->put_entry( *data, globally_committable, state->current_view, index); entry_size_not_limited += data->size(); entry_count++; - state->view_history.update(index, state->current_view); + publish_replicated_entry(index, state->current_view); if (entry_size_not_limited >= append_entries_size_limit) { update_batch_size(); @@ -1203,7 +1314,7 @@ namespace aft restart_election_timeout(); if (!leader_id.has_value() || leader_id.value() != from) { - leader_id = from; + set_leader_id(from); RAFT_DEBUG_FMT( "Node {} thinks leader is {}", state->node_id, leader_id.value()); } @@ -1377,10 +1488,11 @@ namespace aft if (apply_success == ccf::kv::ApplyResult::FAIL) { ledger->truncate(i - 1); + publish_log_state(); send_append_entries_response_nack(from); return; } - state->last_idx = i; + set_last_idx(i); for (auto& hook : ds->get_hooks()) { @@ -1404,7 +1516,7 @@ namespace aft case ccf::kv::ApplyResult::FAIL: { RAFT_FAIL_FMT("Follower failed to apply log entry: {}", i); - state->last_idx--; + decrement_last_idx(); ledger->truncate(state->last_idx); send_append_entries_response_nack(from); break; @@ -1428,7 +1540,7 @@ namespace aft // happened in sig_term. We reflect this in the history. if (r.term_of_idx == aft::ViewHistory::InvalidView) { - state->view_history.update(1, r.term); + update_view_history(1, r.term); } else { @@ -1439,7 +1551,7 @@ namespace aft max_terms_per_append_entries == 1, "AppendEntries processing for term updates assumes single " "term"); - state->view_history.update(r.prev_idx + 1, ds->get_term()); + update_view_history(r.prev_idx + 1, ds->get_term()); } commit_if_possible(r.leader_commit_idx); @@ -1465,6 +1577,7 @@ namespace aft } execute_append_entries_finish(r, from); + publish_log_state(); } void execute_append_entries_finish( @@ -1483,7 +1596,7 @@ namespace aft // occurred, when processing a heartbeat at index 0, which does not // happen in a real node (due to the genesis transaction executing // before ticks start), but may happen in tests. - state->view_history.update(1, r.term); + update_view_history(1, r.term); } else { @@ -1492,7 +1605,7 @@ namespace aft // after the previous signature we saw (lci, last committable index). if (r.idx > lci) { - state->view_history.update(lci + 1, r.term_of_idx); + update_view_history(lci + 1, r.term_of_idx); } } @@ -1825,7 +1938,7 @@ namespace aft { // If we grant our vote to a candidate, then an election is in progress restart_election_timeout(); - leader_id.reset(); + reset_leader_id(); voted_for = from; } @@ -2102,8 +2215,8 @@ namespace aft return; } - state->leadership_state = ccf::kv::LeadershipState::PreVoteCandidate; - leader_id.reset(); + set_leadership_state(ccf::kv::LeadershipState::PreVoteCandidate); + reset_leader_id(); reset_votes_for_me(); restart_election_timeout(); @@ -2147,12 +2260,12 @@ namespace aft return; } - state->leadership_state = ccf::kv::LeadershipState::Candidate; - leader_id.reset(); + set_leadership_state(ccf::kv::LeadershipState::Candidate); + reset_leader_id(); voted_for = state->node_id; reset_votes_for_me(); - state->current_view++; + advance_current_view(1); restart_election_timeout(); reset_last_ack_timeouts(); @@ -2204,8 +2317,8 @@ namespace aft store->initialise_term(state->current_view); } - state->leadership_state = ccf::kv::LeadershipState::Leader; - leader_id = state->node_id; + set_leadership_state(ccf::kv::LeadershipState::Leader); + set_leader_id(state->node_id); should_sign = true; using namespace std::chrono_literals; @@ -2254,11 +2367,11 @@ namespace aft // primary node has not received a majority of acks (CheckQuorum) void become_follower() { - leader_id.reset(); + reset_leader_id(); restart_election_timeout(); reset_last_ack_timeouts(); - state->leadership_state = ccf::kv::LeadershipState::Follower; + set_leadership_state(ccf::kv::LeadershipState::Follower); RAFT_INFO_FMT( "Becoming follower {}: {}.{}", state->node_id, @@ -2286,7 +2399,7 @@ namespace aft { voted_for.reset(); } - state->current_view = term; + set_current_view(term); reset_votes_for_me(); become_follower(); is_new_follower = true; @@ -2380,8 +2493,8 @@ namespace aft { nominate_successor(); - leader_id.reset(); - state->leadership_state = ccf::kv::LeadershipState::None; + reset_leader_id(); + set_leadership_state(ccf::kv::LeadershipState::None); } state->membership_state = ccf::kv::MembershipState::Retired; @@ -2518,6 +2631,7 @@ namespace aft if (term_of_new == state->current_view) { commit(new_commit_idx.value()); + publish_log_state(); } else { @@ -2591,7 +2705,7 @@ namespace aft compact_committable_indices(idx); - state->commit_idx = idx; + set_commit_idx(idx); if ( is_retired() && state->retirement_phase == ccf::kv::RetirementPhase::Signed && @@ -2650,7 +2764,9 @@ namespace aft if (changed) { create_and_remove_node_state(); - if (retired_node_cleanup && is_primary()) + if ( + retired_node_cleanup && + state->leadership_state == ccf::kv::LeadershipState::Leader) { retired_node_cleanup->cleanup(); } @@ -2693,10 +2809,11 @@ namespace aft RAFT_DEBUG_FMT("Setting term in store to: {}", state->current_view); ledger->truncate(idx); - state->last_idx = idx; + set_last_idx(idx); RAFT_DEBUG_FMT("Rolled back at {}", idx); - state->view_history.rollback(idx); + rollback_view_history(idx); + publish_log_state(); while (!state->committable_indices.empty() && (state->committable_indices.back() > idx)) From 07879b570d79f919ce5f49ce2bc1b80a9899a02a Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Fri, 14 Aug 2026 11:46:51 +0000 Subject: [PATCH 2/8] Refactor raft.h to improve code clarity and reduce complexity --- src/consensus/aft/raft.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/consensus/aft/raft.h b/src/consensus/aft/raft.h index 1b122791ea0..86f5c4e572b 100644 --- a/src/consensus/aft/raft.h +++ b/src/consensus/aft/raft.h @@ -1315,8 +1315,7 @@ namespace aft if (!leader_id.has_value() || leader_id.value() != from) { set_leader_id(from); - RAFT_DEBUG_FMT( - "Node {} thinks leader is {}", state->node_id, leader_id.value()); + RAFT_DEBUG_FMT("Node {} thinks leader is {}", state->node_id, from); } // Third, check index consistency, making sure entries are not in the past From 5a9c72ba701fb679a400c1de21cab508cd047cad Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Wed, 19 Aug 2026 12:53:25 +0000 Subject: [PATCH 3/8] Add unit test demonstrating unsynchronized Raft public state reads Adds a deterministic, single-threaded reproduction (no TSAN/threading required) showing that primary(), is_primary() and become_follower() can be combined by a caller (e.g. an HTTP endpoint building a status response) into an internally inconsistent snapshot if a leadership transition happens between reads. This complements the CI TSAN failure and local isolated e2e repro (CR_FILTER=cft ./tests.sh -R nodes_test) that motivated the preceding AFT public state synchronization fix. --- src/consensus/aft/test/main.cpp | 62 +++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/src/consensus/aft/test/main.cpp b/src/consensus/aft/test/main.cpp index 317ec0668ba..14bd3746751 100644 --- a/src/consensus/aft/test/main.cpp +++ b/src/consensus/aft/test/main.cpp @@ -83,6 +83,68 @@ DOCTEST_TEST_CASE("Single node commit" * doctest::test_suite("single")) } } +DOCTEST_TEST_CASE( + "Splicing public state reads across a leadership transition" * + doctest::test_suite("single")) +{ + // This reproduces, without any threading or TSAN, the shape of bug that + // motivated the (reverted) AFT public-state synchronization change: none + // of primary(), is_primary() and get_view() are protected by a single + // lock that also guards leadership transitions (become_leader() / + // become_follower()), so two calls made "in sequence" by a caller (e.g. + // an HTTP endpoint building a status response) are not actually a + // consistent snapshot if a transition happens between them. In the real + // system that gap is filled by a second thread; here we fill it by hand + // to show the resulting combination can violate invariants an endpoint + // might reasonably assume, e.g. "if is_primary() was true a moment ago, + // primary() should still identify this node". + ccf::NodeId node_id = ccf::kv::test::PrimaryNodeId; + auto kv_store = std::make_shared(node_id); + + TRaft r0( + raft_settings, + std::make_unique(kv_store), + std::make_unique(node_id), + std::make_shared(), + std::make_shared(node_id), + nullptr); + r0.start_ticking(); + + ccf::kv::Configuration::Nodes config; + config.try_emplace(node_id); + r0.add_configuration(0, config); + + r0.periodic(election_timeout * 2); + DOCTEST_REQUIRE(r0.is_primary()); + DOCTEST_REQUIRE(r0.primary() == node_id); + + // An endpoint-style caller observes this node is currently primary... + const bool was_primary = r0.is_primary(); + DOCTEST_REQUIRE(was_primary); + + // ...but before it gets around to reading primary() to report who that + // is, something else (in production: a concurrent thread handling a + // CheckQuorum failure or a higher-term message) drives a leadership + // transition. become_follower() is a public method that mutates + // leader_id and state->leadership_state without acquiring state->lock + // itself (it relies on callers, e.g. periodic()/recv_append_entries(), + // to already hold it) - so nothing prevents this from happening between + // the two reads. + r0.become_follower(); + + // The caller's combined snapshot is now internally inconsistent: it + // believes this node is (or very recently was) primary, yet primary() + // no longer identifies it as such. An endpoint that assumed + // "is_primary() implies primary() == self" would report a + // contradiction (or otherwise misuse the stale information) to a + // client. + const auto primary_after = r0.primary(); + DOCTEST_REQUIRE(was_primary); + DOCTEST_REQUIRE_FALSE(r0.is_primary()); + DOCTEST_REQUIRE_FALSE(primary_after.has_value()); + DOCTEST_REQUIRE_FALSE(primary_after == node_id); +} + DOCTEST_TEST_CASE( "Multiple nodes startup and election" * doctest::test_suite("multiple")) { From 7ce853891d801201d4db91c4a1ee5699332b9df3 Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Wed, 19 Aug 2026 12:54:13 +0000 Subject: [PATCH 4/8] Add CHANGELOG entry for AFT public state synchronization fix (#8181) --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66f1fa7b445..c83aec98400 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed + +- Fixed a data race where `primary()`/`is_primary()` could read Raft's `leader_id`/`leadership_state` concurrently with a leadership transition writing them, by guarding both with a dedicated lock (#8181). + ## [7.0.12] [7.0.12]: https://github.com/microsoft/CCF/releases/tag/ccf-7.0.12 From 3e8b8ab0fabb2ffdef65dff8377bb99c85154558 Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Wed, 19 Aug 2026 13:27:59 +0000 Subject: [PATCH 5/8] Document and extend public_state_lock coverage in AFT Raft Add explicit comments naming exactly which fields public_state_lock protects, in preparation for future clang thread-safety annotations. While auditing coverage, found that get_details() (which backs the /node/consensus RPC endpoint) still read state->membership_state, state->retirement_phase, and ticking under the heavier state->lock, reintroducing the endpoint-takes-heavy-lock pattern the original fix was meant to eliminate. Extend public_state_lock to also guard these three fields (via new set_membership_state()/set_retirement_phase()/ set_ticking() wrappers, mirroring the existing set_leadership_state() pattern), and update get_details(), is_active(), is_retired(), is_retired_committed(), and is_retired_completed() accordingly. get_details() still needs to take state->lock afterwards to read configurations and all_other_nodes, which remain out of scope for this lightweight published-state set; this is documented in a comment. Also documented a pre-existing, orthogonal gap: set_retired_committed() is called directly from a KV commit hook and does not appear to take state->lock before mutating membership/retirement state, unlike other mutators in this class. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/consensus/aft/raft.h | 104 +++++++++++++++++++++++++++++++++------ 1 file changed, 89 insertions(+), 15 deletions(-) diff --git a/src/consensus/aft/raft.h b/src/consensus/aft/raft.h index 86f5c4e572b..3ecd6a24641 100644 --- a/src/consensus/aft/raft.h +++ b/src/consensus/aft/raft.h @@ -128,6 +128,34 @@ namespace aft // processed on the enclave main thread. Keep these small query fields // independently synchronized so endpoint transactions do not need to take // state->lock and invert the KV/Raft lock order. + // + // Guarded by public_state_lock (read and written only through the + // wrappers below, e.g. set_leader_id()/primary(), + // set_leadership_state()/is_primary()/is_candidate()/is_backup(), + // set_membership_state()/is_active()/is_retired()/..., set_ticking()): + // - leader_id + // - published_last_idx, published_commit_idx, published_view_history + // (published snapshots of state->last_idx/commit_idx/view_history) + // - state->leadership_state + // - state->membership_state, state->retirement_phase + // - ticking + // state->current_view is also guarded by public_state_lock, via + // set_current_view()/advance_current_view()/get_view(). + // + // All mutations of these fields happen while state->lock is already + // held (see the "Called while state->lock is held" comments below), so + // state->lock continues to order writes with respect to each other; + // public_state_lock only needs to order writes with respect to the + // lightweight readers below, and must never be taken while already + // holding state->lock's writer nested inside another public_state_lock + // acquisition (i.e. treat it as a leaf lock, acquired alone). + // + // Note: get_details() still takes state->lock (not just + // public_state_lock) because it also reads `configurations` and + // `all_other_nodes`, which are not yet part of this lightweight + // published-state set. Removing that remaining state->lock acquisition + // from the endpoint-facing get_details() path would require publishing + // those fields too, which is a larger change out of scope here. mutable ccf::pal::Mutex public_state_lock; std::optional leader_id = std::nullopt; Index published_last_idx = 0; @@ -230,6 +258,28 @@ namespace aft state->leadership_state = leadership_state; } + // Called while state->lock is held. + void set_membership_state(ccf::kv::MembershipState membership_state) + { + std::lock_guard guard(public_state_lock); + state->membership_state = membership_state; + } + + // Called while state->lock is held. + void set_retirement_phase( + std::optional retirement_phase) + { + std::lock_guard guard(public_state_lock); + state->retirement_phase = retirement_phase; + } + + // Called while state->lock is held. + void set_ticking(bool ticking_) + { + std::lock_guard guard(public_state_lock); + ticking = ticking_; + } + // Called while state->lock is held. void set_current_view(Term view) { @@ -422,26 +472,36 @@ namespace aft bool is_active() const { + std::lock_guard guard(public_state_lock); return state->membership_state == ccf::kv::MembershipState::Active; } bool is_retired() const { + std::lock_guard guard(public_state_lock); return state->membership_state == ccf::kv::MembershipState::Retired; } bool is_retired_committed() const { + std::lock_guard guard(public_state_lock); return state->membership_state == ccf::kv::MembershipState::Retired && state->retirement_phase == ccf::kv::RetirementPhase::RetiredCommitted; } bool is_retired_completed() const { + std::lock_guard guard(public_state_lock); return state->membership_state == ccf::kv::MembershipState::Retired && state->retirement_phase == ccf::kv::RetirementPhase::Completed; } + // NOTE: unlike most mutators in this class, this is called directly from + // a KV commit hook (see node_state.h) and does not appear to hold + // state->lock. This is a pre-existing gap, orthogonal to + // public_state_lock (which only orders these fields' writes against the + // lightweight readers below, not against other writers) - not addressed + // here. void set_retired_committed( ccf::SeqNo seqno, const std::vector& node_ids) override { @@ -674,9 +734,10 @@ namespace aft } } + // Called while state->lock is held. void start_ticking() { - ticking = true; + set_ticking(true); using namespace std::chrono_literals; timeout_elapsed = 0ms; RAFT_INFO_FMT("Election timer has become active"); @@ -710,16 +771,25 @@ namespace aft ccf::kv::ConsensusDetails get_details() override { ccf::kv::ConsensusDetails details; - std::lock_guard guard(state->lock); - details.primary_id = leader_id; - details.current_view = state->current_view; - details.ticking = ticking; - details.leadership_state = state->leadership_state; - details.membership_state = state->membership_state; - if (is_retired()) - { - details.retirement_phase = state->retirement_phase; + // These fields are all guarded by public_state_lock (see its + // declaration above), so this does not need to take the heavier + // state->lock or block on Raft message processing. + { + std::lock_guard guard(public_state_lock); + details.primary_id = leader_id; + details.current_view = state->current_view; + details.ticking = ticking; + details.leadership_state = state->leadership_state; + details.membership_state = state->membership_state; + if (state->membership_state == ccf::kv::MembershipState::Retired) + { + details.retirement_phase = state->retirement_phase; + } } + // configurations and all_other_nodes are not part of the + // public_state_lock-guarded set, so state->lock is still required to + // read them safely. + std::lock_guard guard(state->lock); for (auto const& conf : configurations) { details.configs.push_back(conf); @@ -2455,6 +2525,10 @@ namespace aft channels->send_authenticated( successor, ccf::NodeMsgType::consensus_msg, prv); } + // Called while state->lock is held (writes state->retirement_idx / + // state->retirement_committable_idx / state->retired_committed_idx, + // which are not part of the public_state_lock-guarded set and so still + // rely on state->lock for synchronization). void become_retired(Index idx, ccf::kv::RetirementPhase phase) { RAFT_INFO_FMT( @@ -2496,8 +2570,8 @@ namespace aft set_leadership_state(ccf::kv::LeadershipState::None); } - state->membership_state = ccf::kv::MembershipState::Retired; - state->retirement_phase = phase; + set_membership_state(ccf::kv::MembershipState::Retired); + set_retirement_phase(phase); } void add_vote_for_me(const ccf::NodeId& from) @@ -2833,7 +2907,7 @@ namespace aft if (retirement_committable > idx) { state->retirement_committable_idx = std::nullopt; - state->retirement_phase = ccf::kv::RetirementPhase::Ordered; + set_retirement_phase(ccf::kv::RetirementPhase::Ordered); } } } @@ -2851,8 +2925,8 @@ namespace aft if (retirement > idx) { state->retirement_idx = std::nullopt; - state->retirement_phase = std::nullopt; - state->membership_state = ccf::kv::MembershipState::Active; + set_retirement_phase(std::nullopt); + set_membership_state(ccf::kv::MembershipState::Active); RAFT_DEBUG_FMT("Becoming Active after rollback"); } } From bcc9eb5f5f09d7cccbab71b493add8b537577321 Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Wed, 19 Aug 2026 13:29:49 +0000 Subject: [PATCH 6/8] Move changelog entry to 7.0.13, describe test as long-term regression check Moved the public_state_lock fix from Unreleased into a new 7.0.13 section, bumping python/pyproject.toml to match. Reworded the raft_test comment introducing the splicing regression test to describe the long-term property it guards (endpoints see a consistent snapshot of Raft's public state) rather than transient history about the fix being reverted/reinstated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 4 ++++ python/pyproject.toml | 2 +- src/consensus/aft/test/main.cpp | 19 ++++++++++--------- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c83aec98400..34f7a6bcdf6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased] +## [7.0.13] + +[7.0.13]: https://github.com/microsoft/CCF/releases/tag/ccf-7.0.13 + ### Fixed - Fixed a data race where `primary()`/`is_primary()` could read Raft's `leader_id`/`leadership_state` concurrently with a leadership transition writing them, by guarding both with a dedicated lock (#8181). diff --git a/python/pyproject.toml b/python/pyproject.toml index e8ee11b20a3..18462482f03 100644 --- a/python/pyproject.toml +++ b/python/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "ccf" -version = "7.0.12" +version = "7.0.13" authors = [ { name="CCF Team", email="CCF-Sec@microsoft.com" }, ] diff --git a/src/consensus/aft/test/main.cpp b/src/consensus/aft/test/main.cpp index 14bd3746751..7d4f16b97d0 100644 --- a/src/consensus/aft/test/main.cpp +++ b/src/consensus/aft/test/main.cpp @@ -87,15 +87,16 @@ DOCTEST_TEST_CASE( "Splicing public state reads across a leadership transition" * doctest::test_suite("single")) { - // This reproduces, without any threading or TSAN, the shape of bug that - // motivated the (reverted) AFT public-state synchronization change: none - // of primary(), is_primary() and get_view() are protected by a single - // lock that also guards leadership transitions (become_leader() / - // become_follower()), so two calls made "in sequence" by a caller (e.g. - // an HTTP endpoint building a status response) are not actually a - // consistent snapshot if a transition happens between them. In the real - // system that gap is filled by a second thread; here we fill it by hand - // to show the resulting combination can violate invariants an endpoint + // This is a regression test confirming that application endpoints see a + // consistent snapshot of Raft's public state, rather than one spliced + // together from multiple separate calls made around a leadership + // transition. primary(), is_primary() and get_view() are each + // individually synchronized against become_leader()/become_follower(), + // but a caller that reads several of them in sequence (e.g. an HTTP + // endpoint building a status response) has no guarantee that a + // transition did not happen between those reads. In the real system + // that gap is filled by a second thread; here we fill it by hand to + // show the resulting combination can violate invariants an endpoint // might reasonably assume, e.g. "if is_primary() was true a moment ago, // primary() should still identify this node". ccf::NodeId node_id = ccf::kv::test::PrimaryNodeId; From ec336b893fefb1d62302e585ea8b1a139d2224b9 Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Wed, 19 Aug 2026 13:32:04 +0000 Subject: [PATCH 7/8] Correct overclaim: locking fixes the data race, not cross-call consistency Rename the raft_test case and rework its comments: public_state_lock makes each individual read/write of leader_id/leadership_state data-race-free, but it cannot and does not make a sequence of separate getter calls (e.g. is_primary() then primary()) appear atomic. The primary is legitimately allowed to change while an endpoint executes, so a caller observing different results across two calls is expected behaviour, not a bug the fix is meant to prevent. The test now asserts this explicitly rather than implying the fix eliminates the interleaving. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/consensus/aft/test/main.cpp | 54 +++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 23 deletions(-) diff --git a/src/consensus/aft/test/main.cpp b/src/consensus/aft/test/main.cpp index 7d4f16b97d0..d0f7f1a86db 100644 --- a/src/consensus/aft/test/main.cpp +++ b/src/consensus/aft/test/main.cpp @@ -84,21 +84,27 @@ DOCTEST_TEST_CASE("Single node commit" * doctest::test_suite("single")) } DOCTEST_TEST_CASE( - "Splicing public state reads across a leadership transition" * + "Public state reads are data-race-free across a leadership transition" * doctest::test_suite("single")) { - // This is a regression test confirming that application endpoints see a - // consistent snapshot of Raft's public state, rather than one spliced - // together from multiple separate calls made around a leadership - // transition. primary(), is_primary() and get_view() are each - // individually synchronized against become_leader()/become_follower(), - // but a caller that reads several of them in sequence (e.g. an HTTP - // endpoint building a status response) has no guarantee that a - // transition did not happen between those reads. In the real system - // that gap is filled by a second thread; here we fill it by hand to - // show the resulting combination can violate invariants an endpoint - // might reasonably assume, e.g. "if is_primary() was true a moment ago, - // primary() should still identify this node". + // This is a regression test for the data race that public_state_lock + // fixes: leader_id and state->leadership_state used to be read (by + // primary()/is_primary()/get_view()) and written (by become_leader()/ + // become_follower()) without any shared lock, which is undefined + // behaviour under concurrent access (and was caught by TSAN). + // public_state_lock makes each individual read/write of these fields + // well-defined. + // + // What this fix does NOT do, and cannot do, is make several separate + // getter calls appear as a single atomic snapshot. The primary is + // legitimately allowed to change while an endpoint is executing - e.g. + // between an is_primary() call and a later primary() call, a + // leadership transition may genuinely occur on another thread. Callers + // must tolerate that a sequence of these calls may observe different + // points in time, not treat them as one consistent view. This test + // demonstrates that: even with public_state_lock in place, a + // transition interleaved between two calls still changes what they + // report - each call is race-free, but the pair is not atomic. ccf::NodeId node_id = ccf::kv::test::PrimaryNodeId; auto kv_store = std::make_shared(node_id); @@ -127,18 +133,20 @@ DOCTEST_TEST_CASE( // is, something else (in production: a concurrent thread handling a // CheckQuorum failure or a higher-term message) drives a leadership // transition. become_follower() is a public method that mutates - // leader_id and state->leadership_state without acquiring state->lock - // itself (it relies on callers, e.g. periodic()/recv_append_entries(), - // to already hold it) - so nothing prevents this from happening between - // the two reads. + // leader_id and state->leadership_state while holding state->lock + // (which its caller, e.g. periodic()/recv_append_entries(), is + // expected to already hold) - public_state_lock only orders this + // against the getters below, it does not delay or serialize it with + // them into a single transaction. r0.become_follower(); - // The caller's combined snapshot is now internally inconsistent: it - // believes this node is (or very recently was) primary, yet primary() - // no longer identifies it as such. An endpoint that assumed - // "is_primary() implies primary() == self" would report a - // contradiction (or otherwise misuse the stale information) to a - // client. + // The two calls together are not a consistent snapshot: this node was + // primary a moment ago, but no longer is by the time primary() is + // called. This is expected and unavoidable - it reflects a real + // transition that happened between the calls, not a bug. An endpoint + // must not assume "is_primary() implies primary() == self" holds + // across separate calls; each call is only guaranteed to report an + // accurate, race-free value at the instant it runs. const auto primary_after = r0.primary(); DOCTEST_REQUIRE(was_primary); DOCTEST_REQUIRE_FALSE(r0.is_primary()); From 66284be6f5bc64a68f44a5a4ee56a350a80ea23e Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Wed, 19 Aug 2026 13:56:44 +0000 Subject: [PATCH 8/8] Replace contrived splice test with a real concurrent TSAN stress test The previous "Public state reads are data-race-free..." test was single-threaded: it manually called become_follower() between two getter reads, so there was no actual concurrency for TSAN to ever detect a regression against, and its comment was far longer than the (weak) signal it provided. Replace it with a genuinely concurrent test: one driver thread cycles a node through real leadership transitions via force_become_primary()/ become_aware_of_new_term(), while several reader threads spin calling the public getters with no lock of their own - mirroring how endpoints actually call into Raft. This gives TSAN, under -DTSAN=ON, real concurrent access to public_state_lock-guarded fields to catch a regression if the lock is ever removed or bypassed. Verified locally that this test is clean under TSAN with the fix in place, and reliably reports a data race in get_view() when public_state_lock guards are removed. Also make become_follower() private again: it has no legitimate public callers (only periodic() and become_aware_of_new_term(), both members of Aft, call it), so there's no reason to expose it. The new test drives transitions through become_aware_of_new_term(), which was already used elsewhere in this test file as the public entry point for forcing a step-down. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/consensus/aft/raft.h | 2 +- src/consensus/aft/test/main.cpp | 110 ++++++++++++++++++-------------- 2 files changed, 62 insertions(+), 50 deletions(-) diff --git a/src/consensus/aft/raft.h b/src/consensus/aft/raft.h index 3ecd6a24641..ed9f028c93c 100644 --- a/src/consensus/aft/raft.h +++ b/src/consensus/aft/raft.h @@ -2431,7 +2431,6 @@ namespace aft } } - public: // Called when a replica becomes follower in the same term, e.g. when the // primary node has not received a majority of acks (CheckQuorum) void become_follower() @@ -2457,6 +2456,7 @@ namespace aft #endif } + public: // Called when a replica becomes aware of the existence of a new term // If retired already, state remains unchanged, but the replica otherwise // becomes a follower in the new term. diff --git a/src/consensus/aft/test/main.cpp b/src/consensus/aft/test/main.cpp index d0f7f1a86db..b79ce7ba486 100644 --- a/src/consensus/aft/test/main.cpp +++ b/src/consensus/aft/test/main.cpp @@ -5,7 +5,9 @@ #define DOCTEST_CONFIG_NO_SHORT_MACRO_NAMES #define DOCTEST_CONFIG_IMPLEMENT +#include #include +#include using ms = std::chrono::milliseconds; @@ -84,27 +86,26 @@ DOCTEST_TEST_CASE("Single node commit" * doctest::test_suite("single")) } DOCTEST_TEST_CASE( - "Public state reads are data-race-free across a leadership transition" * - doctest::test_suite("single")) + "Concurrent public state reads under a driving leadership transition" * + doctest::test_suite("concurrency")) { - // This is a regression test for the data race that public_state_lock - // fixes: leader_id and state->leadership_state used to be read (by - // primary()/is_primary()/get_view()) and written (by become_leader()/ - // become_follower()) without any shared lock, which is undefined - // behaviour under concurrent access (and was caught by TSAN). - // public_state_lock makes each individual read/write of these fields - // well-defined. + // Regression test for a genuine data race: leader_id and + // state->leadership_state used to be read (by + // primary()/is_primary()/is_candidate()/is_backup()/get_details()) and + // written (by become_leader()/become_aware_of_new_term(), driven here via + // force_become_primary()/become_aware_of_new_term()) without any lock + // shared between readers and writers. That is undefined behaviour under + // concurrent access, and is exactly what TSAN caught in nodes_test. // - // What this fix does NOT do, and cannot do, is make several separate - // getter calls appear as a single atomic snapshot. The primary is - // legitimately allowed to change while an endpoint is executing - e.g. - // between an is_primary() call and a later primary() call, a - // leadership transition may genuinely occur on another thread. Callers - // must tolerate that a sequence of these calls may observe different - // points in time, not treat them as one consistent view. This test - // demonstrates that: even with public_state_lock in place, a - // transition interleaved between two calls still changes what they - // report - each call is race-free, but the pair is not atomic. + // A single background thread repeatedly forces this node through real + // leadership transitions, while several foreground threads spin calling + // the public getters. This does not (and cannot) assert anything about + // what values the readers observe - the primary is legitimately allowed + // to change while a reader is running, so there is no "correct" snapshot + // to check for. The value of this test is solely to give TSAN, run under + // `-DTSAN=ON`, enough real concurrent access to these fields to catch a + // regression if public_state_lock is ever removed or bypassed; it is not + // expected to fail without TSAN instrumentation. ccf::NodeId node_id = ccf::kv::test::PrimaryNodeId; auto kv_store = std::make_shared(node_id); @@ -121,37 +122,48 @@ DOCTEST_TEST_CASE( config.try_emplace(node_id); r0.add_configuration(0, config); - r0.periodic(election_timeout * 2); - DOCTEST_REQUIRE(r0.is_primary()); - DOCTEST_REQUIRE(r0.primary() == node_id); + std::atomic stop = false; - // An endpoint-style caller observes this node is currently primary... - const bool was_primary = r0.is_primary(); - DOCTEST_REQUIRE(was_primary); - - // ...but before it gets around to reading primary() to report who that - // is, something else (in production: a concurrent thread handling a - // CheckQuorum failure or a higher-term message) drives a leadership - // transition. become_follower() is a public method that mutates - // leader_id and state->leadership_state while holding state->lock - // (which its caller, e.g. periodic()/recv_append_entries(), is - // expected to already hold) - public_state_lock only orders this - // against the getters below, it does not delay or serialize it with - // them into a single transaction. - r0.become_follower(); - - // The two calls together are not a consistent snapshot: this node was - // primary a moment ago, but no longer is by the time primary() is - // called. This is expected and unavoidable - it reflects a real - // transition that happened between the calls, not a bug. An endpoint - // must not assume "is_primary() implies primary() == self" holds - // across separate calls; each call is only guaranteed to report an - // accurate, race-free value at the instant it runs. - const auto primary_after = r0.primary(); - DOCTEST_REQUIRE(was_primary); - DOCTEST_REQUIRE_FALSE(r0.is_primary()); - DOCTEST_REQUIRE_FALSE(primary_after.has_value()); - DOCTEST_REQUIRE_FALSE(primary_after == node_id); + constexpr size_t transition_count = 2000; + std::thread driver([&]() { + aft::Term term = 1; + for (size_t i = 0; i < transition_count; ++i) + { + r0.force_become_primary(); + // Advancing the term is what become_aware_of_new_term uses to step + // back down to follower - it is the same public entry point used + // when a real node hears from a more up-to-date peer. + r0.become_aware_of_new_term(++term); + } + stop = true; + }); + + constexpr size_t reader_thread_count = 8; + std::vector readers; + for (size_t i = 0; i < reader_thread_count; ++i) + { + readers.emplace_back([&]() { + while (!stop) + { + // The return values are deliberately unchecked - any interleaving + // of these calls with the driver's transitions is valid. Only + // TSAN's instrumentation, not any assertion here, is meant to + // catch a regression. + r0.is_primary(); + r0.is_candidate(); + r0.is_backup(); + r0.primary(); + r0.get_view(); + r0.get_details(); + } + }); + } + + driver.join(); + for (auto& reader : readers) + { + reader.join(); + } } DOCTEST_TEST_CASE(