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.
1 issue 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>
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>
There was a problem hiding this comment.
Half fixed in code, half deliberately documented. The dangerous direction — the boundary sitting ABOVE what this leader's own log covers, which is what lets a re-promoted broker acknowledge or expose data it does not hold — is now refused outright (Promotion::LeaderBehind, see the Codex thread). The remaining direction, an in-memory watermark already above a newly proven boundary, is kept deliberately: within one process lifetime the watermark only ever advanced by quorum acknowledgement or quorum proof, so everything below it was committed, and commitment is permanent — rewinding it would hide records acknowledged to producers, which is the exact failure this module exists to prevent. A probe that reaches a different majority proves a lower floor; it does not disprove the higher one. The genuinely unsound case — same offset, different records across epochs — is the epoch-qualification gap, and no watermark arithmetic papers over it; it needs KIP-101-style truncation, tracked with the recovery arc. The module doc now states all of this under "Why an inherited watermark is never lowered" instead of leaving it implied.
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 👍 / 👎.
There was a problem hiding this comment.
Fixed, and this was the sharpest finding of the round — the 50/90/90 case turns the established boundary into a write-acknowledgement machine for records the leader never had, via exactly the flush_produce_group fast path you traced. establish() now takes the candidate's own id and refuses with a new Promotion::LeaderBehind whenever the quorum-proven boundary exceeds the leader's own reported offset (or its own probe is absent): nothing is published, the loss is announced, and the lease is left to lapse so a caught-up replica can win the range instead. Catching a behind leader up in place is the recovery-protocol arc the module docs already track. Tests: a_leader_behind_the_proven_boundary_is_refused (your exact 50/90/90), a_leader_at_the_proven_boundary_promotes (only strictly behind refuses), a_leader_that_did_not_answer_its_own_probe_is_refused, and promoter-level a_leader_behind_the_boundary_refuses_and_demotes.
444fef0 to
de26283
Compare
f044ca9 to
028ce54
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 028ce544fe
ℹ️ 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".
| // branch that normally demotes is guarded on `Held` — | ||
| // so nothing would ever clear it and this node would | ||
| // keep passing /readyz as a deposed leader. | ||
| self.lost(fencing_epoch); |
There was a problem hiding this comment.
When promotion is refused for a transient quorum miss, this calls lost(), which routes through BrokerLeasePublisher::demote and MetaFencingEpoch::clear_lease; that records the same fencing_epoch in released_through. If the followers recover before metadata's lease expires, the next round sees our live lease, re-probes successfully, and calls promote() for the same epoch, but MetaFencingEpoch::set leaves lease_active false for epochs already released. The agent then caches verified_epoch and keeps renewing while the broker remains fenced indefinitely, so retryable promotion failures can wedge the range until an external epoch change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed, and the mechanism was exactly as you traced it: clear_lease records the epoch in released_through, so the recovered quorum's re-promotion at the same epoch hit set()'s already-released check and the broker stayed fenced under its own live lease. The fix separates the two meanings that were conflated in one primitive. MetaFencingEpoch::suspend(epoch) deactivates the CURRENT epoch without recording a release — for refusals that are retryable (quorum miss, and leader-behind, since probes are a snapshot) — while demote/clear_lease remains for epochs metadata has actually finished (rival grant, refused renewal). The Promoter's refusal paths now suspend; verified_epoch still resets so the next round re-probes. Tests: a_suspended_epoch_reactivates_on_the_same_grant_where_a_release_would_not (the view-level distinction, both directions), a_refused_promotion_suspends_rather_than_stranding_or_poisoning, and a_transient_quorum_miss_does_not_wedge_the_epoch — the full arc against a real fencing view: refusal fences, quorum returns, the SAME epoch serves again.
de26283 to
4771930
Compare
028ce54 to
dcbc9e2
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
4771930 to
3f036fd
Compare
dcbc9e2 to
bab96ce
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
bab96ce to
8e2e4d2
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…#236) Fifth and final slice of #223, stacked on #235. The mechanism exists; this is the evidence it works. ## Deterministic races `lease_election_races.rs` decides the races against the real metadata state machine with explicit `issued_at_ms` values. "Two candidates at the same instant" means two commands carrying the same timestamp applied in log order — which is what Raft delivers to every replica. **Nothing depends on wall-clock timing or on tests racing each other.** The five cases are the ones a timing-based design gets wrong: | Case | What it proves | |---|---| | two simultaneous candidates | exactly one wins — the loser's CAS token is stale by the time its command applies | | a candidate an hour fast | it takes the range early (the disruption the design admits to) **and** the epoch it mints still fences the old holder, whose broker is then refused on the data path | | a late renewal from a displaced holder | refused — this is what a partitioned leader produces when its heartbeats land after the range moved | | a holder that keeps renewing | never displaced, and its epoch never churns; a new epoch per heartbeat would fence a leader against its own in-flight produce | | re-acquisition by the current holder | mints exactly one epoch, so a retrying agent cannot ratchet itself out of its own range | Together they state the claim the design rests on: **expiry is liveness, the fencing epoch is safety.** ## Live failover `09-range-leader-failover.sh` runs the same thing on real processes. Every earlier data-plane scenario validated **durability** — kill the leader, and what was acknowledged survives — but none validated **failover**, because until now there was nothing to fail over to and the range simply stopped. It kills a leader under sustained quorum produce, restarts a follower as a lease-driven leader over the data it already replicated, and asserts: 1. the follower acquires the lease within the TTL, at a strictly higher epoch 2. every acknowledged record is still readable byte-exactly 3. the restarted old leader is refused under its stale epoch 4. every surviving artifact verifies offline ## Supporting CLI `vtopctl meta range-lease` exposes the linearizable read, so a scenario — or an operator mid-incident — can see who holds a range and until when. `vtopctl meta create-topic` fills the gap that made a range unleasable from the CLI at all. Refs #223. With this merged, #223's acceptance is met. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Deterministic lease-election races and a live failover scenario prove lease-driven promotion and fencing work end to end. Adds `vtopctl` commands to create a topic and read a range lease, fulfilling #223. - **New Features** - Tests: `vtop-broker` adds `lease_election_races.rs` with five deterministic cases using explicit `issued_at_ms`, including data-path fencing of stale epochs. - Chaos: `09-range-leader-failover.sh` kills a leader under sustained quorum produce, promotes the follower with the highest committed offset via the lease, asserts a higher epoch, intact acknowledged data, a restarted old leader that stays unready and is fenced, and verifies artifacts; lease helpers in `scripts/live-chaos/lib.sh`; lease env vars in `LIVE_CHAOS_VALIDATION.md`. - CLI: `vtopctl meta range-lease` returns a linearizable view (found, holder, fencing epoch, deadline, range generation, applied index; with JSON output); `vtopctl meta create-topic` creates a topic and root range for leasing. <sup>Written for commit 42f00bd. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/allamiro/vtop-engine/pull/236?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
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 serving by probing replicas and only adopting the epoch if a quorum proves the floor; publish the established offset to the broker watermark. On failure, suspend serving without releasing the lease so a retry can reactivate; advances #223.
New Features
promotion.rs: computes the quorum floor via the k-th largest offset; majority from the configured replication factor; unreachable replicas ignored; refuse if the leader is behind; standalone ranges promote without a probe.QuorumProbeandReplicaPlaneProbeusing concurrent replication-plane RPCs; the leader reads its own disk with a blocking accessor.LeasePublisher:promote(fencing_epoch, committed_offset)advances the broker’s committed watermark and adopts the epoch; addedsuspend(fencing_epoch)to fence locally without releasing;demoteunchanged.MetaFencingEpoch::suspend, wired throughBrokerLeasePublisher::suspend; added aPromoterthat verifies once per epoch transition, publishes suspensions on refusal, and integrates withLeaseAgentso renewals are skipped until verified.Dependencies
async-trait,futures.Written for commit 8e2e4d2. Summary will update on new commits.