Repository navigation
fix(read) #381: LinearizableRead must not be served without quorum in multi-voter cluster - #383
Merged
Merged
Conversation
… multi-voter cluster ## Root cause In Phase 3 of `execute_and_process_raft_rpc`, a multi-voter leader served LinearizableRead immediately when `last_applied >= read_index`, with zero quorum confirmation. A leader isolated in a minority partition would answer stale reads — Raft §8 violation. ## Changes **leader_state.rs — Phase 3** Add `single_voter &&` guard: single-voter self IS the quorum (safe to serve immediately); multi-voter reads always queue into `pending_reads` so they cannot be served without a network round-trip. **leader_state.rs — handle_append_result (Path A drain)** After quorum confirmation, drain `pending_reads` entries whose read_index <= last_applied. Required because pure-read batches never advance commit_index, so handle_apply_completed (Path B) would never fire for them.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR fixes Bug ChangesLinearizable Read Pending Queue Bug Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
d-engine-core/src/raft_role/leader_state.rs (1)
1888-1899: ⚡ Quick winExtract shared pending-read drain helper to prevent future path drift.
Path A and
handle_apply_completedboth implement the samepending_readsdrain loop. Centralizing this into one helper reduces divergence risk for future fixes in read serving behavior.♻️ Suggested refactor
+fn drain_pending_reads_up_to( + &mut self, + last_applied: u64, + ctx: &RaftContext<T>, +) { + let to_serve: Vec<u64> = + self.pending_reads.range(..=last_applied).map(|(k, _)| *k).collect(); + for idx in to_serve { + if let Some(batch) = self.pending_reads.remove(&idx) { + self.execute_pending_reads(batch.requests, ctx); + } + } +}- let reads_to_serve: Vec<_> = - self.pending_reads.range(..=last_index).map(|(k, _)| *k).collect(); - - for read_index in reads_to_serve { - if let Some(batch) = self.pending_reads.remove(&read_index) { - self.execute_pending_reads(batch.requests, ctx); - } - } + self.drain_pending_reads_up_to(last_index, ctx);- let last_applied = ctx.state_machine().last_applied().index; - let to_serve: Vec<u64> = - self.pending_reads.range(..=last_applied).map(|(k, _)| *k).collect(); - for idx in to_serve { - if let Some(batch) = self.pending_reads.remove(&idx) { - self.execute_pending_reads(batch.requests, ctx); - } - } + self.drain_pending_reads_up_to(ctx.state_machine().last_applied().index, ctx);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@d-engine-core/src/raft_role/leader_state.rs` around lines 1888 - 1899, Extract the duplicated pending-reads drain loop into a single helper method (e.g., drain_pending_reads(&mut self, ctx: &mut ContextType)) that iterates self.pending_reads up to ctx.state_machine().last_applied().index, removes entries and calls execute_pending_reads(batch.requests, ctx); then replace the inline loop in the Path A block and the loop inside handle_apply_completed with calls to this new drain_pending_reads helper so both paths share the same logic and avoid drift. Ensure the helper has access to self.pending_reads, calls execute_pending_reads, and uses the same last_applied calculation (ctx.state_machine().last_applied().index).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@d-engine-core/src/raft_role/leader_state_test/client_read_test.rs`:
- Around line 2185-2191: The test currently only checks
resp_rx.try_recv().is_err(), which can be true if the channel closed; after
calling flush() assert that the leader_state.pending_reads (or its accessor used
in the test) contains the queued LinearizableRead instance (e.g., matches the
request id or client token used in this test) to prove the read was actually
enqueued awaiting quorum; locate the pending_reads collection in the same test
scope (or via leader_state.pending_reads) and add a direct assert that its
length increased or that it contains the specific read entry before proceeding
to handle_append_result.
---
Nitpick comments:
In `@d-engine-core/src/raft_role/leader_state.rs`:
- Around line 1888-1899: Extract the duplicated pending-reads drain loop into a
single helper method (e.g., drain_pending_reads(&mut self, ctx: &mut
ContextType)) that iterates self.pending_reads up to
ctx.state_machine().last_applied().index, removes entries and calls
execute_pending_reads(batch.requests, ctx); then replace the inline loop in the
Path A block and the loop inside handle_apply_completed with calls to this new
drain_pending_reads helper so both paths share the same logic and avoid drift.
Ensure the helper has access to self.pending_reads, calls execute_pending_reads,
and uses the same last_applied calculation
(ctx.state_machine().last_applied().index).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4df629a-0e37-4aa2-b8ec-5790e951a240
📒 Files selected for processing (4)
d-engine-core/src/raft_role/leader_state.rsd-engine-core/src/raft_role/leader_state_test/client_read_test.rsd-engine-core/src/raft_role/leader_state_test/mod.rsd-engine-core/src/raft_role/leader_state_test/pending_reads_test.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ued not just unanswered
11 of 17 tasks
Merged
10 of 12 tasks
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
1 participant
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.
Root cause
In Phase 3 of
execute_and_process_raft_rpc, a multi-voter leader servedLinearizableRead immediately when
last_applied >= read_index, with zeroquorum confirmation. A leader isolated in a minority partition would answer
stale reads — Raft §8 violation.
Changes
leader_state.rs — Phase 3
Add
single_voter &&guard: single-voter self IS the quorum (safe to serveimmediately); multi-voter reads always queue into
pending_readsso theycannot be served without a network round-trip.
leader_state.rs — handle_append_result (Path A drain)
After quorum confirmation, drain
pending_readsentries whose read_index <=last_applied. Required because pure-read batches never advance commit_index,
so handle_apply_completed (Path B) would never fire for them.
What Does This PR Do?
Fixes a linearizability violation where a partitioned multi-voter leader could
serve stale reads by bypassing quorum confirmation. After this fix, multi-voter
LinearizableRead is always deferred until either a quorum ACK (Path A) or SM
apply (Path B) confirms the leader is still current.
Type:
Why Is This Needed?
Bug: A multi-voter leader in a minority partition would answer
LinearizableRead immediately from local state, bypassing the Raft §8 requirement
that leadership must be confirmed via a quorum heartbeat before serving reads.
Jepsen
registerworkload withFAULTS=partitionreproduces this as aKnossos linearizability violation.
Fix: Gate Phase 3 fast-path on
single_voter. Multi-voter reads enterpending_readsand are drained only after quorum is confirmed inhandle_append_result.Checklist
Required:
make testpassesTesting
How tested:
Unit tests (deterministic):
pending_reads_test::test_multi_voter_fast_path_linear_read_is_queued(T1) — Phase 3 no longer fast-paths multi-voter; fails without fixpending_reads_test::test_pending_reads_drained_by_quorum_ack(T2) — Path A quorum ACK drains the queue; fails without fixpending_reads_test::test_pending_reads_slow_path_drained_by_apply_completed(T3) — Path B regression guardpending_reads_test::test_pending_reads_expired_by_tick(T4) — partition scenario: reads expire viatick()pending_reads_test::test_pending_reads_cleared_on_stepdown(T5) —drain_read_buffer()sendsUnavailableon step-downclient_read_test::test_linearizable_read_served_without_quorum_in_minority_partition— end-to-end regression throughflush_cmd_buffers; fails without fixIntegration tests:
make run-workload WORKLOAD=register FAULTS=partition TIME_LIMIT=120(probabilistic; unit tests above are the deterministic guarantee)For bug fixes:
Does This Follow d-engine's Principles?
Reviewer Notes
Core logic change is 2 surgical edits (~20 lines). The rest is tests and comments.
Focus review on:
leader_state.rs:~3450— Phase 3single_voter &&guardleader_state.rs:~1885— Path A drain loop inhandle_append_resultEstimated review complexity:
Summary by CodeRabbit
Bug Fixes
#381) where linearizable read operations could be served prematurely without proper quorum confirmation, ensuring consistent read behavior across cluster states.Tests