meta: range-lease expiry and renewal for leader election (#223) - #232
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: a7efee782c
ℹ️ 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".
| let range_key = MetaKey::Range { | ||
| topic_uuid, | ||
| range_uuid, | ||
| } | ||
| .encode(); | ||
| let Some(MetaValue::Range(range)) = self.records.get_mut(&range_key) else { |
There was a problem hiding this comment.
When the holder has already been transitioned to Dead while its lease is still recorded, renewal still succeeds because this path never validates holder_node_uuid against the node record. A delayed or rogue RenewRangeLease with the old epoch can keep pushing expires_at_ms forward, and then AcquireRangeLease by another active node rejects the takeover as still live; that defeats the failover this expiry path is meant to enable. Please reject at least dead holders before extending the deadline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. You and cubic both caught it: grant and acquire validate the holder against the node record and renewal did not, so a node transitioned to Dead could hold its range indefinitely through heartbeats alone.
Renewal now applies the same active-node check. Test a_dead_holder_cannot_renew marks the holder Dead, asserts the transition actually took (an earlier draft of the test silently passed a stale expected_generation, so the node was never Dead and the test proved nothing), then asserts the renewal is refused.
| let extended = match lease.expires_at_ms { | ||
| Some(current) => current.max(expires_at_ms), | ||
| None => expires_at_ms, | ||
| }; | ||
| lease.expires_at_ms = Some(extended); |
There was a problem hiding this comment.
When RenewRangeLease is applied to a range granted by GrantRangeLease, lease.expires_at_ms is None. This branch turns that administrative, never-expiring lease into Some(expires_at_ms), so a later AcquireRangeLease from another holder can take it after the new deadline; that violates the contract that admin grants cannot be quietly undone by elections. Renewals should reject deadline-less leases or leave None unchanged.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by rejecting rather than preserving. A renewal against a deadline-less lease is now refused outright, because there is no coherent thing for it to mean: an administrative grant is an operator saying "this holder, permanently", and silently accepting a heartbeat for it invites the caller to believe it is keeping something alive that was never at risk.
Leaving None unchanged would have been the quieter fix, but it makes a successful response a lie. The error names the alternatives — release or re-grant. Test an_administrative_lease_cannot_be_renewed_into_an_expiring_one asserts both the refusal and that the lease stays never-expiring.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
a7efee7 to
28869a0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28869a06d4
ℹ️ 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".
| // that lost track of its own epoch recovers, and it still pays a new | ||
| // epoch for the privilege. | ||
| if let Some(existing) = range.lease.as_ref() { | ||
| if existing.holder_node_uuid != holder_node_uuid && existing.is_live_at(now_ms) { |
There was a problem hiding this comment.
When an operator has used GrantRangeLease and the same holder later issues AcquireRangeLease, this check only rejects live leases held by someone else, so the branch below overwrites the None deadline with Some(expires_at_ms). Fresh evidence beyond the resolved renewal thread is that this acquire path can still convert an administrative, never-expiring grant into an expiring lease, after which another node can take it once the new deadline passes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — the same reasoning as the renewal thread, applied to the acquire path. acquire_range_lease now rejects ANY administrative (deadline-less) lease outright, including the holder's own: letting the holder "re-acquire" it would trade the operator's permanent lease for an expiring one that a rival could take once it lapses. The only ways out of an administrative grant remain an explicit release or a fresh grant. Test an_administrative_lease_cannot_be_acquired_into_an_expiring_one asserts both the refusal and that the lease record is byte-identical afterwards.
1bb726b to
8b10522
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b10522691
ℹ️ 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".
| unreachable!("the lease was present a moment ago") | ||
| }; | ||
| lease.expires_at_ms = Some(extended); | ||
| range.generation += 1; |
There was a problem hiding this comment.
When a live holder renews, this increments RangeRecord.generation even though the fencing epoch does not change. I checked register_sealed_segment: it rejects unless expected_range_generation matches the current range generation, so a renewal that lands between sealing a segment and registering it turns an otherwise valid publish under the same lease epoch into a GenerationMismatch; steady heartbeats can make the leader repeatedly fail its own metadata publishes unless every in-flight command is retried after refetching/tracking this hidden CAS bump. Since renewal only extends the deadline and returns no generation, leave the range CAS unchanged or explicitly return/propagate the new generation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — renewal no longer touches range.generation. You are right that it was a hidden CAS bump: a renewal changes neither holder nor epoch, returns no generation for the caller to re-learn, and every heartbeat would have silently invalidated the leader's own in-flight RegisterSealedSegment. Acquisition keeps its bump (holder and epoch genuinely change, and the agent re-reads before acquiring). Test renewal_does_not_consume_the_range_cas pins it.
First slice of #223. The metadata plane already granted range leases with strictly monotonic fencing epochs, so a grant fences the previous holder by construction. What it had no concept of was expiry — a dead leader held its range forever and no follower could ever take over, which is exactly why killing a range leader stops the range today. Adds two commands rather than fields on the existing one, so every pinned golden vector for `GrantRangeLease` stays byte-exact: * `AcquireRangeLease` is the election path. It refuses to displace a lease that is still live and held by someone else, and mints `fencing_epoch + 1` when it succeeds. * `RenewRangeLease` extends the holder's deadline WITHOUT minting an epoch, so a live leader keeps serving across renewals. A new epoch per heartbeat would fence the leader against its own in-flight produce requests. `GrantRangeLease` keeps its meaning untouched: an administrative grant that never expires, because an operator naming a holder explicitly should not have that decision quietly undone by an election. The deadline is computed from the envelope's `issued_at_ms` plus the requested duration — both data in the replicated log — so every replica derives the same expiry and the state machine never reads a local clock. The property worth being explicit about: **expiry is liveness, not safety.** Safety comes from the epoch, which acquisition always advances. A clock-skewed candidate can therefore acquire early and be disruptive, but can never produce two brokers that each believe they may write. That is what lets #215's clock-skew scenario hold without the cluster needing agreed time, and it is the difference from an ISR model with an unclean-election knob to misconfigure. Two rejections are load-bearing rather than defensive. A renewal must name both the holder AND the current epoch, or a partitioned old leader could keep a lease alive it no longer holds. And a renewal never shortens a deadline, so one that arrives out of order behind a longer one cannot pull the expiry in and trigger an election against a leader that is renewing correctly. Durably, `LeaseRecord` gains `expires_at_ms: Option<i64>` behind a third presence byte. A lease with no deadline still encodes with the pre-#223 byte, so snapshot vectors and mixed-version replicas stay byte-exact; tag 2 appears only once a lease actually carries an expiry. A pinned test asserts that. Next slices: the node-side acquire/renew agent that publishes grants into `MetaFencingEpoch` (the gap #224's readiness probe documents), verified promotion from a quorum of replica statuses, and the live failover scenario.
8b10522 to
7b21b78
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Second slice of #223, stacked on #232. ## Why a candidate cannot decide without this The admin transport offered propose, status, and membership — **no read of applied state at all**. So an election loop could only guess: propose an acquisition and learn from the rejection. That conflates two very different situations. A `GenerationMismatch` tells a candidate its CAS token was stale; it does **not** tell it whether the incumbent is healthy and renewing, or dead and expired. Acting on that ambiguity is exactly how a candidate fences a leader that was doing nothing wrong. `AdminReadRangeLease` returns the lease view **together with** the `range_generation` acquisition must CAS against — reading the lease without the token it has to be paired with would just move the guessing one step along. ## The ordering is the point `ensure_linearizable()` runs **before** the state is read, not after. A deposed node serving its own lagging copy could report an expired lease that the real leader has already renewed, and a candidate acting on that would fence a healthy leader. Fencing first makes that impossible: a node that has lost leadership fails the read instead of answering it. ## Three distinctions the response keeps - a range that **does not exist** vs. one that exists with **nobody leading it** - an **administrative** lease with no deadline vs. an **election** lease with one — the codec mirrors the durable presence-byte encoding from #232 so the two representations cannot drift apart in meaning - the **applied index** the read was fenced at, so a caller can tell an answer from a newer state machine apart from a replayed older one ## Notes `OpenraftConsensus` gains an optional store handle — optional because the existing harnesses build the façade from a bare Raft handle and have no applied state to offer; a read against one of those reports the store as unavailable rather than inventing an answer. `start_meta_node` attaches it. openraft containment holds: the new wire types and trait are consensus-library-free, and the only openraft call sits inside `raft/`. The policy test passes. Refs #223. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Adds a linearizable range-lease read on the admin transport so candidates can see the current lease and the CAS token (`range_generation`) before acquiring, preventing fences of a healthy leader. Supports #223. - New Features - Admin endpoint `read_range_lease` fences with Raft `ensure_linearizable()` before reading; implemented via `AdminReadRangeLease` on `OpenraftConsensus` and wired through `AdminHandler`. - Response includes `found`, `range_generation`, `fencing_epoch`, `AdminLeaseView` (optional `expires_at_ms`), and `read_at_applied_index`. - Wiring: new wire kinds and codecs; server dispatch; `AdminClient::read_range_lease(...)`; `OpenraftConsensus::with_store(...)` enables reads and `start_meta_node` attaches the store; nodes without a store return an error; crate re-exports `AdminLeaseView` and `AdminReadRangeLease*` types. <sup>Written for commit b180097. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/allamiro/vtop-engine/pull/233?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
First slice of #223. Metadata-only; no behaviour change to any running node yet.
The gap
The metadata plane already grants range leases with strictly monotonic fencing epochs, so a grant fences the previous holder by construction. What it had no concept of was expiry — a dead leader held its range forever and no follower could ever take over. That is precisely why killing a range leader stops the range today, and why #215's kill-test validates durability rather than failover.
What this adds
Two new commands rather than fields on the existing one, so every pinned golden vector for
GrantRangeLeasestays byte-exact:AcquireRangeLease— the election path. Refuses to displace a lease that is still live and held by someone else; mintsfencing_epoch + 1when it succeeds.RenewRangeLease— extends the holder's deadline without minting an epoch, so a live leader keeps serving across renewals. A new epoch per heartbeat would fence the leader against its own in-flight produce requests.GrantRangeLeasekeeps its meaning untouched: an administrative grant that never expires, because an operator naming a holder explicitly should not have that decision quietly undone by an election.The property worth arguing about
Expiry is liveness, not safety. Safety comes from the epoch, which acquisition always advances. The deadline is computed from the envelope's
issued_at_msplus the requested duration — both data in the replicated log — so every replica derives the same expiry and the state machine never reads a local clock.A clock-skewed candidate can therefore acquire early and be disruptive, but can never produce two brokers that each believe they may write. That is what lets #215's clock-skew scenario hold without the cluster needing agreed time, and it is the difference from an ISR model with an unclean-election knob to misconfigure.
Two rejections that are load-bearing, not defensive
Durable format
LeaseRecordgainsexpires_at_ms: Option<i64>behind a third presence byte. A lease with no deadline still encodes with the pre-#223 byte, so snapshot vectors and mixed-version replicas stay byte-exact; tag 2 appears only once a lease actually carries an expiry. A pinned test asserts exactly that, and the existing golden-vector suite passes unchanged.Verification
cargo fmt --check,cargo clippy --workspace --all-targets -- -D warningsclean, fullcargo test --workspacegreen. Seven new tests cover expiry, steal refusal, renewal without epoch change, stale-epoch renewal, out-of-order renewal, the never-expiring administrative grant, and the encoding contract.Next slices
The node-side acquire/renew agent that publishes grants into
MetaFencingEpoch— closing the caveat #224 documented — then verified promotion from a quorum of replica statuses, then the live failover scenario.Refs #223, #215.
Summary by cubic
Adds lease expiry and renewal to the metadata layer for deterministic leader election and failover. Introduces
AcquireRangeLeaseandRenewRangeLease, and adds a backward‑compatibleexpires_at_mstoLeaseRecord; metadata-only with no node behavior change yet.AcquireRangeLease: mintsfencing_epoch + 1; computes expiry asenv.issued_at_ms + lease_duration_ms(replicated, no local clocks); refuses if another node’s lease is live; rejects zero duration; requires Active holder; blocks over an administrative (no‑deadline) lease; guards against deadline overflow.RenewRangeLease: extends the deadline without changing the epoch orrange.generation; requires matching holder and epoch; rejects expired leases; requires Active holder; never shortens the deadline; cannot renew an administrative lease.LeaseRecordaddsexpires_at_ms: Option<i64>with presence tag0/1/2;Nonekeeps the pre‑broker: metadata-lease leader election and verified failover for the data plane #223 byte‑exact encoding; tests cover admin‑grant invariants, acquire/renew refusals (including dead holders and admin-lease blocks), never‑shorten behavior, CAS stability on renewals, and codec round‑trips.Written for commit 7b21b78. Summary will update on new commits.