Skip to content

Fix data race on peer config_ in handle_rpc_result - #119

Merged
alexey-milovidov merged 1 commit into
masterfrom
fix-peer-config-race
Jun 14, 2026
Merged

Fix data race on peer config_ in handle_rpc_result#119
alexey-milovidov merged 1 commit into
masterfrom
fix-peer-config-race

Conversation

@alexey-milovidov

Copy link
Copy Markdown
Member

The peer::config_ member is protected by config_mutex_ and is meant to be read only through the locked accessors (get_id, get_endpoint, get_config, ...). Several call sites in peer.cxx still read config_->get_id directly, bypassing the mutex.

ThreadSanitizer reports a data race between the unlocked read of srv_config::get_id in peer::handle_rpc_result (an asio RPC worker thread) and the construction of a fresh srv_config that is published via peer::set_config while cluster_config::deserialize runs inside raft_server::commit_conf on the commit thread. The lock on config_mutex_ is the intended synchronization point, so the unlocked read leaves no happens-before edge.

Route all of these reads through the locked get_id accessor, matching the earlier fix in commit 7a9de82 ("Fix some non dangerous races").

Found by the ClickHouse amd_tsan integration test test_keeper_4lw_reconfiguration::test_precondition_failure.

The `peer::config_` member is protected by `config_mutex_` and is meant
to be read only through the locked accessors (`get_id`, `get_endpoint`,
`get_config`, ...). Several call sites in `peer.cxx` still read
`config_->get_id` directly, bypassing the mutex.

ThreadSanitizer reports a data race between the unlocked read of
`srv_config::get_id` in `peer::handle_rpc_result` (an asio RPC worker
thread) and the construction of a fresh `srv_config` that is published
via `peer::set_config` while `cluster_config::deserialize` runs inside
`raft_server::commit_conf` on the commit thread. The lock on
`config_mutex_` is the intended synchronization point, so the unlocked
read leaves no happens-before edge.

Route all of these reads through the locked `get_id` accessor, matching
the earlier fix in commit 7a9de82 ("Fix some non dangerous races").

@alexey-milovidov alexey-milovidov left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@alexey-milovidov
alexey-milovidov merged commit 131671c into master Jun 14, 2026
1 check passed
alexey-milovidov added a commit to ClickHouse/ClickHouse that referenced this pull request Jun 14, 2026
NuRaft PR #119 has been merged. Move the submodule pointer from the
fix commit to the fork's `master` tip (the merge commit), so it tracks
master rather than a feature branch.

ClickHouse/NuRaft#119

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant