node: verified promotion before a new leader serves (#223) - #235
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a927cda3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for follower in &self.followers { | ||
| probes.push(crate::promotion::ReplicaProbe { | ||
| node_id: *follower, | ||
| local_committed_offset: replicas.follower_durable_offset(*follower), |
There was a problem hiding this comment.
When a broker has just acquired a lease, follower_durable_offset() is only the NetworkedReplicaSet cache, not a fresh answer from that follower. Those channels are initialized with durable_offset = 0 and are only populated after a status/catch-up exchange or append, so a newly promoted leader can establish HWM 0 even when a majority already durably holds acknowledged records; fetch then hides committed data until another append happens to refresh the cache. Promotion needs an on-demand status probe, and disconnected/unqueried followers should be absent rather than counted as cached zero.
Useful? React with 👍 / 👎.
| if !self.publish_held(fencing_epoch) { | ||
| // Could not verify the boundary; do not renew a lease this | ||
| // process cannot safely serve under. | ||
| return Ok(self.config.poll_interval); |
There was a problem hiding this comment.
In the renewal path, if this node was already serving and publish_held() returns false, the branch just returns after publish_held() sets only the agent's private state to NotHeld; it never calls publisher.demote(). The broker's MetaFencingEpoch therefore remains live for the old epoch, so it can keep accepting reads/writes during the remaining metadata lease even though this round decided the committed boundary could not be verified. This failure path should clear the broker lease when the previous state was held.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/vtop-node/src/lease_agent.rs">
<violation number="1" location="crates/vtop-node/src/lease_agent.rs:120">
P1: Promotion does not actually replace an inherited high-water mark when the verified boundary is lower, because `advance_to` is monotonic. The promotion path needs a boundary-reset operation (while steady-state progression remains monotonic), otherwise a re-promoted broker can expose data beyond the new quorum boundary.</violation>
</file>
<file name="crates/vtop-node/src/promotion.rs">
<violation number="1" location="crates/vtop-node/src/promotion.rs:116">
P3: The majority test comment contradicts its assertion and the following explanation; correcting it would keep the even-replica quorum invariant understandable.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| answered, | ||
| } => { | ||
| if let Some(cluster) = self.broker.cluster_committed() { | ||
| cluster.advance_to(committed_offset); |
There was a problem hiding this comment.
P1: Promotion does not actually replace an inherited high-water mark when the verified boundary is lower, because advance_to is monotonic. The promotion path needs a boundary-reset operation (while steady-state progression remains monotonic), otherwise a re-promoted broker can expose data beyond the new quorum boundary.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/vtop-node/src/lease_agent.rs, line 120:
<comment>Promotion does not actually replace an inherited high-water mark when the verified boundary is lower, because `advance_to` is monotonic. The promotion path needs a boundary-reset operation (while steady-state progression remains monotonic), otherwise a re-promoted broker can expose data beyond the new quorum boundary.</comment>
<file context>
@@ -43,29 +43,107 @@ use vtop_meta::{AdminClient, MetadataCommand, MetadataResponse};
+ answered,
+ } => {
+ if let Some(cluster) = self.broker.cluster_committed() {
+ cluster.advance_to(committed_offset);
+ }
+ tracing::info!(
</file context>
| fn a_majority_needs_more_than_half_even_at_even_sizes() { | ||
| assert_eq!(majority(1), 1); | ||
| assert_eq!(majority(3), 2); | ||
| // 2, not 3-of-4 — but crucially not 2 for a 4-set, which would let two |
There was a problem hiding this comment.
P3: The majority test comment contradicts its assertion and the following explanation; correcting it would keep the even-replica quorum invariant understandable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/vtop-node/src/promotion.rs, line 116:
<comment>The majority test comment contradicts its assertion and the following explanation; correcting it would keep the even-replica quorum invariant understandable.</comment>
<file context>
@@ -0,0 +1,223 @@
+ fn a_majority_needs_more_than_half_even_at_even_sizes() {
+ assert_eq!(majority(1), 1);
+ assert_eq!(majority(3), 2);
+ // 2, not 3-of-4 — but crucially not 2 for a 4-set, which would let two
+ // disjoint groups each call themselves a majority.
+ assert_eq!(majority(4), 3);
</file context>
Fourth slice of #223. Winning the lease is an act of the metadata plane: it says who MAY lead, and nothing about what the range actually contains. A leader that starts serving on that basis alone is guessing at its own high-water mark. Guess too low and fetch hides records that were acknowledged to a producer; guess too high and the range exposes records that never reached a quorum and can still be lost. So promotion is a read before it is a right to write. The new leader asks a quorum of replicas where their disks are and takes the boundary a quorum can prove: the k-th largest reported offset, where k is the majority. The maximum would count a replica holding an append the old leader never managed to acknowledge; the minimum would stall the range behind its slowest member. The probe goes over the replication plane, one `ReplicaStatusClient` RPC per follower. It deliberately does NOT read `NetworkedReplicaSet::follower_durable_offset`, which was the obvious choice and is wrong: that accessor reads a counter advanced by this leader's own replication stream, and returns `None` only when a node id is missing from the configured set — a config mismatch, never an unreachable peer. On a freshly promoted leader that stream has never run, so every follower would report `Some(0)`. A disconnected replica would count as holding nothing, the quorum floor would collapse to zero, `advance_to(0)` would be a no-op, and the refusal path could never fire. It would make verified promotion do nothing precisely on the failover it exists for. The leader reads its own disk with the blocking accessor, not the observation -only one. Promotion is a request handler and may queue behind an append; the non-blocking variant would have the leader abstain from its own quorum under momentary lock contention, which in a 2-replica range turns a lock hold into a refused promotion. The majority comes from the CONFIGURED replication factor, not from how many probes came back. Deriving it from what answered would let a partition shrink the quorum: three reachable replicas out of five would compute a majority of two, and two disjoint halves could each promote. Verification runs once per epoch TRANSITION, not once per renewal. A leader holding a range for hours re-proves nothing by re-probing every few seconds. A refused promotion publishes the LOSS rather than only refusing. Flipping local state alone left the broker's metadata view live while the agent stopped renewing: the lease would lapse, a rival would take it, and the `Wait` branch that normally demotes is guarded on `Held` — so nothing would clear it and a deposed leader would keep passing `/readyz` indefinitely. The module documents three things this does NOT yet do, so nobody reads more safety into it than is here: offsets are not epoch-qualified (Kafka's KIP-101 problem — two replicas reporting 90 may not hold the same record), followers are never truncated (a replica holding uncommitted records above the boundary keeps them, and they resurface if it later wins), and followers are not fenced before being probed (BookKeeper fences the ensemble first, precisely so the read is not a snapshot of a moving target). Raft §5.4.2 adds a fourth: the safe form appends a marker in the new epoch rather than committing prior entries by counting replicas. Closing those needs new wire messages and a marker record type.
0a927cd to
f044ca9
Compare
|
Reworked after review. The central finding was correct and serious: as originally written, this was a no-op on real failover. The bugI probed On a freshly promoted leader that stream has never run. So every follower reported
The right primitive was already in the tree — Also fixed
Documented rather than silently claimedThe module doc overclaimed — it promised "guess too high" was prevented when Closing those needs new wire messages and a marker record type; that is a separate arc, and I would rather have the limitation written down than a doc claiming a property the wiring does not deliver. New tests: refusal demotes rather than stranding; one verification per epoch; the established boundary actually reaches the broker; a standalone range still promotes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f044ca9012
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn promote(&self, fencing_epoch: u64) { | ||
| fn promote(&self, fencing_epoch: u64, committed_offset: Option<u64>) { | ||
| if let (Some(offset), Some(cluster)) = (committed_offset, self.broker.cluster_committed()) { | ||
| cluster.advance_to(offset); |
There was a problem hiding this comment.
In the failover case where this broker's own disk is behind the quorum-established boundary (for example probes at 50/90/90), advancing cluster_committed here publishes an HWM beyond the leader's local tail. The existing LocalBroker::flush_produce_group fast path then treats new appends below that HWM as already quorum-committed (cluster.get() >= leader_committed) and returns success without fan-out, so the lagging leader can acknowledge writes at offsets that are occupied by already-committed records before it has caught up. Promotion should refuse/block writes or catch the local log up until local_committed_offset covers the established boundary before raising the broker HWM.
Useful? React with 👍 / 👎.
1 participant
Fourth slice of #223, stacked on #234.
Winning the lease is an act of the metadata plane: it says who may lead, and nothing about what the range actually contains. A leader that starts serving on that basis alone is guessing at its own high-water mark — and both ways of guessing are wrong:
So promotion is a read before it is a right to write.
The arithmetic
The new leader asks a quorum where their disks are and takes the k-th largest reported offset, where k is the majority size:
This is the same arithmetic the replication path already uses to advance the watermark during steady-state produce — applied once, from a standing start, to state written by someone else.
Ordering
The boundary is established before the epoch is adopted. Adopting first would leave the broker servable for the width of the call while still holding whatever high-water mark it inherited — precisely the guess this removes.
LeasePublisher::promotecan now refuse, and the agent honours that: a leader that cannot reach a quorum does not renew, so metadata's deadline hands the range on rather than leaving a leader serving numbers nobody confirmed.Cases the tests pin
Each is a plausible wrong answer someone could implement:
Refs #223.
Summary by cubic
Verify a new leader’s committed boundary before it serves by probing replicas and only adopting the epoch if a quorum proves the boundary. Prevents serving below or above the true high‑water mark and advances #223.
New Features
promotion.rsto compute the quorum floor via the k‑th largest offset (majority isn/2 + 1); unreachable replicas are ignored.QuorumProbeandReplicaPlaneProbeto query followers over the replication plane; the leader reads its own disk with a blocking accessor; standalone ranges skip probing.LeasePublisher::promotenow takescommitted_offset: Option<u64>;BrokerLeasePublisheradvances the cluster committed offset first, then adopts the epoch.Dependencies
async-traitandfutures.Written for commit f044ca9. Summary will update on new commits.