Fix to bug regarding int32 overflow - #660
Merged
Merged
Conversation
* The peer response timer and connection activity timer return `uint64_t` values in microseconds. All related logic, including the non-responding member checker, should also use `uint64_t`, even when converting the values to milliseconds. * Otherwise, a 32-bit integer overflow could cause incorrect behavior, such as including a non-responding member in the quorum. The overflow occurs at `2^31` milliseconds, or approximately 24.8 days.
There was a problem hiding this comment.
Pull request overview
This PR addresses long-uptime timekeeping overflows in peer response/activity timing by propagating uint64_t (millisecond) values through quorum/non-responding-peer logic and related reconnection checks, matching the underlying microsecond timers that already return uint64_t.
Changes:
- Updated non-responding peer detection paths (
for_each_voting_members,get_not_responding_peers,is_excluded_from_quorum) to useuint64_tmillisecond values end-to-end. - Updated peer-activity timeout logic in vote/append/join-leave handlers to use
uint64_tand corrected logging format specifiers for 64-bit values. - Updated
raft_serverinternal APIs (header) to reflect the newuint64_ttiming types for the touched methods.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/raft_server.cxx |
Switches quorum “non-responding” timing plumbing to uint64_t ms in callbacks and peer iteration. |
src/handle_vote.cxx |
Uses uint64_t for peer activity timing and updates logging format to PRIu64. |
src/handle_join_leave.cxx |
Uses uint64_t for join/leave activity timing and timeout calculations. |
src/handle_append_entries.cxx |
Uses uint64_t for activity timing and updates logging format to PRIu64. |
include/libnuraft/raft_server.hxx |
Updates method signatures to reflect uint64_t timing types for modified APIs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
715
to
+719
| } | ||
|
|
||
| size_t num_not_resp_nodes = 0; | ||
| auto cb = [&num_not_resp_nodes, required_log_idx, expiry] | ||
| (const ptr<peer>& pp, int32_t resp_elapsed_ms) | ||
| (const ptr<peer>& pp, uint64_t resp_elapsed_ms) |
| int32 get_leadership_expiry(); | ||
| std::list<ptr<peer>> get_not_responding_peers(int expiry = 0); | ||
| std::list<ptr<peer>> get_not_responding_peers(uint64_t expiry = 0); | ||
| size_t get_not_responding_peers_count(int expiry = 0, uint64_t required_log_idx = 0); |
Comment on lines
+647
to
651
| std::list<ptr<peer>> raft_server::get_not_responding_peers(uint64_t expiry) { | ||
| // Check if quorum nodes are not responding | ||
| // (i.e., don't respond 20x heartbeat time long or expiry if sent as argument). | ||
| // default argument for expiry is used in case user defines leadership_expiry_. | ||
| ptr<raft_params> params = ctx_->get_params(); |
Comment on lines
+741
to
742
| uint64_t resp_elapsed_ms = peer_ptr->get_resp_timer_us() / 1000; | ||
| callback(peer_ptr, resp_elapsed_ms); |
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 free
to 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.
The peer response timer and connection activity timer return
uint64_tvalues in microseconds. All related logic, including the non-responding member checker, should also useuint64_t, even when converting the values to milliseconds.Otherwise, a 32-bit integer overflow could cause incorrect behavior, such as including a non-responding member in the quorum. The overflow occurs at
2^31milliseconds, or approximately 24.8 days.