Uh oh!
There was an error while loading. Please reload this page.
Synchronize public AFT state queries (leader_id, leadership_state) - #8181
Closed
Eddy Ashton (eddyashton) wants to merge 8 commits into
Closed
Synchronize public AFT state queries (leader_id, leadership_state)#8181Eddy Ashton (eddyashton) wants to merge 8 commits into
Eddy Ashton (eddyashton) wants to merge 8 commits into
Conversation
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>
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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a real data race in AFT consensus public query getters (eg primary(), is_primary()) by introducing a dedicated lock to synchronize reads/writes of leadership-related public state, and updates the Raft unit tests and changelog accordingly.
Changes:
- Add
public_state_lockand route public consensus queries through lock-protected accessors. - Introduce “published” copies for selected log-state query fields and publish updates from consensus code paths.
- Add a new
raft_testtest case and document the fix inCHANGELOG.md.
Custom instructions used
- None (no repository instruction files from
.github/copilot-instructions.mdor.github/instructions/were loaded via tools during this review)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/consensus/aft/raft.h | Introduces public_state_lock, synchronized accessors, and “published” query state to remove public-query data races. |
src/consensus/aft/test/main.cpp | Adds a new unit test scenario describing leadership transitions interleaved with public getters. |
CHANGELOG.md | Adds an Unreleased entry documenting the data race fix (with PR reference). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+275
to
+279
| std::lock_guard<ccf::pal::Mutex> guard(public_state_lock); | ||
| published_last_idx = state->last_idx; | ||
| published_commit_idx = state->commit_idx; | ||
| published_view_history = state->view_history; | ||
| } |
Comment on lines
+90
to
+100
| // 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". |
Eddy Ashton (eddyashton)
marked this pull request as draft
August 19, 2026 13:07
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>
… 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>
…tency 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>
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>
Eddy Ashton (eddyashton)
commented
Aug 19, 2026
MemberAuthor
I think this suggested fix is a misreading - #8184 outlines a more principled distinction. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
aft::Aft::primary(),is_primary()and related public getters readleader_idandstate->leadership_statewithout any locking, while leadership transitions (become_leader(),become_follower()) that mutate these fields are only synchronized incidentally, by whatever lock (if any) the caller happens to hold (e.g.state->lockheld aroundperiodic()/recv_append_entries()).This is a genuine data race: endpoints such as
/node/networkcall these getters from the RPC/worker threads concurrently with consensus threads driving leadership transitions. It was originally caught by TSAN (primary()read vsbecome_follower()write) while working on #8117 (RPC connection manager), but is unrelated to that PR's actual RPC/connection-management changes, so it's split out here as its own fix.This PR:
public_state_lockguardingleader_idandstate->leadership_state, and routes all reads/writes of these fields through synchronized accessors (set_leader_id,reset_leader_id,set_leadership_state,primary(),is_primary(), etc.). This makes each individual read or write of these fields well-defined (no more UB from an unsynchronized concurrent read/write).is_primary()followed byprimary()) atomic with respect to each other. The primary is legitimately allowed to change while an endpoint is executing, so callers must not assume multiple calls observe a single consistent snapshot - only that each individual call is race-free.raft_test, "Public state reads are data-race-free across a leadership transition") that reproduces the shape of the original bug without needing TSAN or real threading: it manually interleaves abecome_follower()transition between two endpoint-style getter reads. It demonstrates both that each read/write is now race-free, and that the pair of calls can still legitimately observe different points in time (i.e. is_primary() may report true just before a transition, then primary() reports no-one, afterwards) - this is expected behaviour, not something the fix is meant to prevent.Evidence
nodes_test(rotationsubtest) exactly matching this race: read inprimary()(raft.h) vs write inbecome_follower().CR_FILTER=cft ./tests.sh --timeout 600 -R nodes_testunder a TSAN build fails consistently without this fix and passes consistently with it (and 6+ passes on plainmainwithout a reliable local repro of the exact race, so this is proactive - the race is real but rare/hard to trigger without workload changes elsewhere).raft_testunit test demonstrates the inconsistency deterministically, independent of TSAN or thread scheduling.Testing
raft_test: all 16 test cases / ~1,001,000 assertions pass, including the new test.e2e_logging,e2e_redirects(previously run under TSAN on the originating branch prior to the split): passed.