fix(consensus): keep a replica off a hole in its committed prefix - #4073
fix(consensus): keep a replica off a hole in its committed prefix#4073krishvishal wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4073 +/- ##
=============================================
- Coverage 85.52% 70.52% -15.00%
Complexity 1402 1402
=============================================
Files 1240 1239 -1
Lines 186294 167121 -19173
Branches 152599 133426 -19173
=============================================
- Hits 159327 117863 -41464
- Misses 22914 45272 +22358
+ Partials 4053 3986 -67
🚀 New features to boost your workflow:
|
| // The quiet peer may be the thing that died, and nothing else | ||
| // re-targets a journal-repair session, so retrying it forever pins | ||
| // the walk while the rest of the cluster is serveable. | ||
| let peer = next_repair_peer(consensus.replica_count(), consensus.replica(), peer); |
There was a problem hiding this comment.
session.peer is never updated. The rotated value is a shadowed local, so every retry rotates from the original peer and the RepairDone continuation (line 5097) still sends to it.
Rotation is also blind and immediate. A Normal backup rotates on the first stall to any replica, including one lagging below from_op. on_request_prepares (4683-4708) answers a range it never held with RangeEvicted and RepairDone, and the RangeEvicted arm (5106) arms a state transfer against that peer without checking retained_from against commit_min + 1. On the primary-elect path the rotation leaves the pending_view_body_sources set and hits the same conversion mid view change.
Write session.peer. Reuse next_transfer_peer (2518, prefers the primary) instead of adding next_repair_peer. Rotate only after a retry budget, as tick_partitions does (7148). Keep primary-elect rotation inside pending_view_body_sources.
| // applied and durable in the snapshot. | ||
| let repair_floor = journal.handle().snapshot_op(); | ||
| let missing = first_op_not_covered(&pending, repair_floor, |op| { | ||
| let missing = first_op_not_covered(&pending, repair_floor, consensus.commit_min(), |op| { |
There was a problem hiding this comment.
This scan now reports a missing_op below pending.commit_max, but pending_view_body_sources(missing_op) (5829) only looks at DVC suffixes, which span commit..=op per sender. index_of returns None below that, the source list comes back empty, and the view change stalls until the timeout escalates. Every sender with commit >= missing_op holds or has compacted the op.
For missing_op < pending.commit_max, select DVC senders with commit >= missing_op, most recent log_view first. A RangeEvicted from such a peer means this replica cannot serve the committed prefix, so let the view-change timeout escalate rather than arm a state transfer as primary-elect. Test against a real DVC quorum.
| } | ||
| let barrier = barrier.min(head); | ||
| self.recovery_barrier | ||
| .set(if barrier <= self.commit_max.get() { |
There was a problem hiding this comment.
Collapsing the barrier to 0 when barrier <= commit_max opens the HTTP read gate early. await_recovery_barrier (core/server/src/http/reads.rs:279) gates on commit_min because adoption advances commit_max before applying the suffix. With head 105 and commit_min 100, new readers see barrier 0 and serve state from before ops 101..=105 apply. is_caught_up_primary compares commit_max >= barrier and needs no zero.
Set barrier.min(head) and drop the collapse. Update given_a_discarded_suffix_when_adopting_a_view_should_lower_the_barrier to expect 105 and assert commit_max() >= recovery_barrier().
| .filter(|header| header.op <= commit) | ||
| .filter(|header| header.op <= commit)?; | ||
| if head.op != next { | ||
| // Unreachable in debug and the simulator; release reports and waits. |
There was a problem hiding this comment.
"Unreachable in debug and the simulator" is false on the partition plane. IggyPartition::commit_journal walks at most COMMIT_WALK_OPS_MAX (64) ops per call, and a promoted primary's pipeline is seeded from merged.commit_max + 1. A primary-elect whose journal covers the merged log but whose apply lags by more than 64 ops passes the coverage scan, starts the view, and receives quorum acks for commit_max + 1 before the sweep drains the backlog. drain_committable_prefix then sees head_op > commit_min + 1 and the assert fires. Release logs an error per ack until the walk catches up.
Finish the journal walk before the pipeline drain on partition promotion, or downgrade the asserts at 466 and 529 to a log.
| ) -> Option<PrepareHeader> { | ||
| let shard = self.replicas[replica_idx].partition_shard(namespace); | ||
| let partition = shard.plane.partitions().get_by_ns(&namespace)?; | ||
| partition.log.journal().inner.header_by_op(op) |
There was a problem hiding this comment.
header_by_op reads resident headers only. evict_prefix clears them on flush and moves the entries to the repair ring. After every replica flushes, assert_partition_prefixes_agree compares zero ops and the check is vacuous at quiescence.
Use repair_headers_in(1..=commit_min) once per replica per namespace, since state_checker.rs:333 probes op by op and both lookups are linear. Add a post-flush comparison test.
| /// Expected wherever the paired `AckLevel::NoAck` store never replicated, so a | ||
| /// diagnostic and not a fault. Still logged: on a replica that did serve the | ||
| /// store it is the first symptom of a lost apply. | ||
| fn log_absent_offset_delete(&self, kind: &str, id: u64) { |
There was a problem hiding this comment.
Patch coverage on this file is 20%. Add a unit test that commits DeleteConsumerOffset for an absent offset on a primary and asserts the partition stays unfenced.
github-actions
Bot
added
S-waiting-on-author
2 participants
The defect
A replica could be promoted to primary while missing operations that the cluster had already committed. The promotion checks started at the merged commit point, so they did not check for missing operations between the replica’s
commit_minand that point.For example, a replica might have executed operations 1–6 and received operations 8–10, but never received operation 7. If the cluster had committed through operation 10, the promotion scan started at 10 and missed the gap.
After promotion, the replica could serve reads and compute replies from incomplete state. It could also attempt to advance
commit_minpast operations it had not executed, triggering an assertion.Fixes
Promotion coverage (
shard). The coverage scan, repair requests, and repair retries now start at the lower of the merged commit point andcommit_min + 1. This ensures that promotion checks include missing committed operations and that repair requests cover the same range. Replicas with a contiguous committed prefix keep the existing scan range.Repair progress (
shard). Repair sessions could remain active after their requested range was satisfied, or stop making progress when the selected peer crashed. Restarting repair required another commit, which the incomplete repair could itself prevent. Sessions now clear when their range is satisfied, switch peers after a full retry interval without a response, and restart whenever repair is still needed.Recovery barrier (
consensus). At startup, a replica records the recovered journal head as the point it must commit before accepting client requests as primary. A later view change could discard part of that journal without updating the barrier. If the replica then became primary, it would wait for discarded operations to commit and reject the requests needed to make progress. The barrier is now reassessed when the merged log is finalized at view start or aStartViewlog is adopted. It is lowered if the suffix was truncated and retained if the suffix survived.Commit ordering (
consensus, both planes). The commit pipeline now processes only consecutive operations starting atcommit_min + 1. Previously, it could pass a later operation toadvance_commit_mineven though an earlier prepare was missing. A debug assertion detects this in simulation and CI. Release builds report the problem and pause commit advancement until repair fills the gap. This avoids terminating the shard task while the process continues to report itself as healthy.Consumer offset deletion (
partitions). Applying a committedDeleteConsumerOffsetnow logs and succeeds when the offset is already absent. An absent offset is valid:AckLevel::NoAckstores apply only on the primary, so a follower may never have received the offset, and a restart can also lose it. The previous check depended on the replica’s role when the delete was committed, which did not establish whether it had received the earlier store. Treating absence as an error could therefore fence a partition during a valid committed delete. The apply function no longer returnsResult.Simulator
The quiescence checks previously compared partition commit positions but did not compare the committed operations themselves. Content comparisons covered only metadata, where partition-focused runs often committed one operation or none.
The simulator now compares partition journal contents per namespace over the range retained by both replicas. Partition journals evict committed entries as they flush, so the check allows entries that have already been removed. It compares
identity_checksum, because retransmission can change the prepare’s view and therefore its sealed checksum.The recovery-barrier failure did not reproduce in 340 seeds across two fault profiles. A new check fails a run if a metadata primary remains in
Normalstatus below its recovery barrier for 2000 ticks.The simulator also reports a fenced partition task explicitly. Previously, queued frames left by an exited task could be reported as a missed wake. The missed-wake check remains in place for actual scheduling failures.