node,broker,protocol: fence replicas before reading them (#240) - #262
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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Third item of #240. Promotion asked each replica where its disk was and took the boundary a quorum could prove — but nothing had stopped the deposed leader, which can still be appending to a follower while the new leader takes its measurement. The boundary a quorum "proved" could be stale before it was published. BookKeeper fences the ensemble before ledger recovery for exactly this reason; this is that fence. ONE ROUND TRIP, NOT TWO. Fencing and reading are a single message. Offsets and the epoch history are what a truncation target is computed from, and taking them in separate calls would describe two different moments of a log that may have moved in between — arithmetic over two different logs. WHAT CHANGES THE SAFETY PROPERTY: a replica that could not be fenced reports ABSENT rather than its last known offset, so it does not count toward the quorum. Its offset is a measurement of something that may still be moving, and counting it is the bug the fence exists to close. An offset now either comes from a log that has been stopped, or it does not come at all. A REPLICA DOES NOT FENCE ITSELF ON A CALLER'S WORD. The claim is not evidence. A replica that adopted a bare claim could be fenced to any epoch by anything that reaches its port, and would then refuse every append until metadata reached a number that may never arrive — a permanent outage from one compromised or buggy peer. The ceiling is the replica's own metadata view, which the lease watcher maintains (#239, which is why this was blocked on it). A legitimately promoted leader only has to wait for the replica to see the same grant. Fencing also never moves backwards, so a stale leader cannot un-fence a replica it has already lost. THE HANDLER DEFAULT REFUSES, unlike epoch_history which defaults to empty. The asymmetry is deliberate: an unknown history is a fact a caller can act on — "do not reconcile me by epoch" — but an unknown fence is not. A handler that silently reported success without fencing anything would put the moving target back into the quorum by another route. The client has no degraded path either, for the same reason. KINDS 75/76, NOT THE FREE-LOOKING 69/70. Kind 70 is a live legacy alias that still decodes as CommitCursorRequest (the form before it carried an operation identity), so taking it would make an old client's commit request decode as a fence response — silently, since both are well-formed. A test pins that. Promotion's module docs now separate what is closed from what is open rather than listing three gaps of which one is fixed. 12 tests: 6 on the follower (the deposed leader is genuinely shut out after the fence; an ungranted epoch is refused; a below-held epoch is refused; a retry at the held epoch succeeds; offsets and history come from one instant), 2 on promotion (the fence uses the epoch being promoted; an unfenced replica is absent from the quorum), and protocol round-trips including the legacy-70 collision. Live-chaos 09 passes; full suite unchanged from baseline.
0a024d3 to
0c082e7
Compare
![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
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
All five were real; two changed behaviour. A FENCE AT A NEWER EPOCH WAS ACCEPTED AS SUCCESS. The client checked the reported epoch was at least the requested one. Lower is obviously wrong, but higher is just as disqualifying and I had not thought it through: it means something granted a newer epoch between the replica adopting and answering, so this candidate has already been superseded and the snapshot describes a log fenced under someone else's grant. Counting it establishes a boundary from a measurement taken for a different leader. Now an exact match. THE COLLISION TEST DID NOT TEST THE COLLISION. It encoded a kind-74 frame and asserted it round-tripped as a cursor request — which stays true no matter what kind 70 is reassigned to. It would have passed through exactly the collision it existed to prevent. It now decodes an actual kind-70 payload. The fence-response decoder was a copy of the epoch-history decoder, and the copies had already drifted: one rejected a non-advancing history as InvalidFrame, the other as Limit, so the same malformed payload was indistinguishable to a caller depending on which message carried it. Both now share one `decode_epoch_starts`, which is what stops that recurring. The test expectation moves to InvalidFrame — the payload is malformed, not oversized. No test covered the mapping from "the fence RPC failed" to "this replica is absent", which is the safety property the whole change exists for: every promotion test used a QuorumProbe double, so an edit turning a fence failure back into a counted offset would have left them all green. Added a test driving the REAL `ReplicaPlaneProbe` against a follower with nothing listening — the cheapest genuine failure, and the same arm an older peer, a refused grant, or a crashed replica takes. It needs rcgen as a vtop-node dev-dependency because the client must be constructible, not because the handshake gets anywhere. Scenario 09 passes; workspace tests, clippy -D warnings, fmt clean.
|
All five fixed in P1 — a fence at a newer epoch was accepted as successYou are right, and I had not thought the upper side through. I checked P2 — the collision test did not test the collisionAlso right, and this one is worth stating plainly: the test encoded a kind-74 frame and asserted it round-tripped as a cursor request. That stays true no matter what kind 70 is reassigned to. It would have passed through exactly the collision it existed to prevent — I wrote a test for the hazard I had just avoided and it asserted nothing about that hazard. It now decodes an actual kind-70 payload via P3 — duplicated history parser, and the drift it had already causedCorrect, and the copies had already diverged, which is the best argument for your suggestion: kind 68 rejected a non-advancing history as Both now share one P3 — the fence-error path was untestedThe sharpest of the three. Every promotion test used a Added Scenario 09 passes; workspace tests, clippy |
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
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-broker/src/replication/network.rs">
<violation number="1" location="crates/vtop-broker/src/replication/network.rs:1461">
P1: A follower that reports a newer epoch is treated as an ordinary missing replica, so a superseded candidate can still publish a boundary at the old epoch when the remaining replicas form a majority. Preserve a distinct newer-epoch outcome that aborts or suspends promotion instead of feeding it into the absent-replica quorum path.</violation>
</file>
<file name="crates/vtop-node/src/lease_agent.rs">
<violation number="1" location="crates/vtop-node/src/lease_agent.rs:1269">
P2: This test can fail or stop exercising the intended unfenceable-follower path when another process owns 127.0.0.1:1. Binding an ephemeral loopback listener and closing the accepted connection would provide a test-owned, deterministic fence failure.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // snapshot describes a log fenced under someone else's | ||
| // grant. Counting it would establish a boundary from a | ||
| // measurement taken for a different leader. | ||
| if response.fencing_epoch != fencing_epoch { |
There was a problem hiding this comment.
P1: A follower that reports a newer epoch is treated as an ordinary missing replica, so a superseded candidate can still publish a boundary at the old epoch when the remaining replicas form a majority. Preserve a distinct newer-epoch outcome that aborts or suspends promotion instead of feeding it into the absent-replica quorum path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/vtop-broker/src/replication/network.rs, line 1461:
<comment>A follower that reports a newer epoch is treated as an ordinary missing replica, so a superseded candidate can still publish a boundary at the old epoch when the remaining replicas form a majority. Preserve a distinct newer-epoch outcome that aborts or suspends promotion instead of feeding it into the absent-replica quorum path.</comment>
<file context>
@@ -1448,13 +1448,20 @@ impl ReplicaStatusClient {
+ // snapshot describes a log fenced under someone else's
+ // grant. Counting it would establish a boundary from a
+ // measurement taken for a different leader.
+ if response.fencing_epoch != fencing_epoch {
return Err(crate::BrokerError::InvalidConfig(format!(
- "replica reported epoch {} after being asked to fence at {fencing_epoch}",
</file context>
| .unwrap() | ||
| .with_timeout(std::time::Duration::from_millis(200)); | ||
|
|
||
| // Port 1 on loopback: reserved, and nothing this test could race with. |
There was a problem hiding this comment.
P2: This test can fail or stop exercising the intended unfenceable-follower path when another process owns 127.0.0.1:1. Binding an ephemeral loopback listener and closing the accepted connection would provide a test-owned, deterministic fence failure.
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 1269:
<comment>This test can fail or stop exercising the intended unfenceable-follower path when another process owns 127.0.0.1:1. Binding an ephemeral loopback listener and closing the accepted connection would provide a test-owned, deterministic fence failure.</comment>
<file context>
@@ -1229,6 +1229,76 @@ mod tests {
+ .unwrap()
+ .with_timeout(std::time::Duration::from_millis(200));
+
+ // Port 1 on loopback: reserved, and nothing this test could race with.
+ let unreachable = "127.0.0.1:1".parse().unwrap();
+ let probe = ReplicaPlaneProbe::new(
</file context>
Bumps the workspace to 0.2.0. The release workflow cross-checks the tag against this value, so it lands before the tag is pushed. v0.1.0 shipped verified promotion as a quorum-proven floor. Everything since closes the ways that floor could be computed from unsound inputs, or acted on in ways that lost acknowledged data: #258 replicas record which fencing epoch wrote each stretch of their log and can be asked for it, so two replicas reporting offset 90 are no longer indistinguishable when only one holds the same record there. #259 a diverged replica is truncated instead of stranded, bounded so it can never discard acknowledged records. #262 replicas are fenced and read in one round trip, and a replica that could not be fenced does not count toward the quorum: an offset now either comes from a log that has been stopped, or it does not come at all. #263 a replica reconciles against the candidate while fenced, so it agrees before it answers. Closed #261, where a diverged replica acked a new leader's writes as duplicates and could be counted toward a quorum for bytes it did not hold. #266 committed high-water marks stop being droppable. They rode a try_send whose result was discarded, so a loaded follower silently stopped learning what had been acknowledged, and that mark is the bound that stops truncation from discarding acknowledged records. Release notes now carry a real changelog with linked issues and pull requests instead of install boilerplate, which moved to docs/RELEASE_VERIFICATION.md (#260). The generator's linkifier is anchored to standalone references so it cannot rewrite a URL fragment or nest a link a title already carried, and its trailer cleanup only runs on a trailer it actually edited. Not closed, and stated rather than left to be discovered: #240 stays open for the Raft 5.4.1/5.4.2 question of whether the fence plus the acknowledged-records bound substitute for an election restriction, given metadata grants the lease with no log-completeness condition on the candidate. An attempt at a new-epoch marker (#265) was withdrawn: it could not be encoded, and it did not close the hazard it was written for. The signed leadership-transition record is also outstanding and wants #255's segment transfer first.
1 participant
Third item of #240. Independent of #259 — that one adds the truncation primitive, this one makes the promotion read trustworthy. Either can land first.
Promotion asked each replica where its disk was and took the boundary a quorum could prove. Nothing had stopped the deposed leader, which can still be appending to a follower while the new leader takes its measurement — so the boundary a quorum "proved" could be stale before it was published. BookKeeper fences the ensemble before ledger recovery for exactly this reason.
The property this actually changes
A replica that could not be fenced reports absent, not its last known offset. It does not count toward the quorum. Its offset is a measurement of something that may still be moving, and counting it is the whole bug. An offset now either comes from a log that has been stopped, or it does not come at all.
That is the assertion worth reading first:
a_replica_that_could_not_be_fenced_is_absent_from_the_quorum.One round trip, not two
Fencing and reading are a single message. Offsets and the epoch history are what a truncation target is computed from, and taking them in separate calls would describe two different moments of a log that may have moved in between — arithmetic over two different logs.
A replica does not fence itself on a caller's word
The claim is not evidence. A replica that adopted a bare claim could be fenced to any epoch by anything that can reach its port, and would then refuse every append until metadata reached a number that may never arrive — a permanent outage caused by one compromised or buggy peer.
So the ceiling is the replica's own metadata view, maintained by the lease watcher — which is why this item was blocked on #239. A legitimately promoted leader only has to wait for the replica to see the same grant; a replica that has not seen it refuses, correctly, since it is not fenced until it has. Fencing also never moves backwards, so a stale leader cannot un-fence a replica it already lost.
The handler default refuses
Unlike
epoch_history, which defaults to empty. The asymmetry is deliberate: an unknown history is a fact a caller can act on — "do not reconcile me by epoch" — but an unknown fence is not. A handler that silently reported success without fencing anything would put the moving target back into the quorum by another route. The client has no degraded path either, so an older peer, an unreachable one, and one that has not seen the grant all fail alike.This does mean a rolling upgrade needs a quorum of upgraded replicas to fail over. That is the safe direction and it is temporary; the alternative is counting replicas nothing has stopped.
A collision I nearly shipped
I allocated kinds 69/70 first because they looked free. Kind 70 is a live legacy alias that still decodes as
CommitCursorRequest— the form from before it carried an operation identity. Taking it would have made an old client's commit request decode as a fence response, silently, since both are well-formed. Moved to 75/76, above every kind in use, with a test pinning it. 69 is left unused rather than splitting the pair across the cursor block.Docs
promotion.rslisted three known gaps. Two are now closed and one of the remaining ones was overstated (replicas can now say which epoch wrote what; promotion just does not use it yet). Rewritten as closed-vs-open rather than editing one bullet and leaving the count wrong.Verification
12 tests. The follower ones drive the real thing: append under the old epoch, fence, then assert the old leader's next append is refused and the measurement has not moved. Plus promotion-side tests that the fence uses the epoch being promoted (easy to thread wrongly, impossible to notice from outside) and that an unfenced replica is absent from the quorum.
Live-chaos scenario 09 passes. Full suite unchanged from baseline — the four failures (05, 05b, 06, 07) are Linux-only, needing
unshareand a C shim that will not build on macOS, and are green in CI. Workspace tests, clippy-D warnings, fmt clean.Summary by cubic
Fence replicas before reading during leader promotion to make the boundary trustworthy. Implements #240 with a single fence+read RPC and excludes unfenced replicas from the quorum.
New Features
vtop-protocol:ReplicaFenceRequest/ReplicaFenceResponse(kinds 75/76). Returns offsets and epoch history together.vtop-nodepromotion probe now fences followers at the promotion epoch and treats failures as absent; the leader reads locally without self-RPC.Bug Fixes
ReplicaStatusClient.fencenow requires an exact epoch match; replies at a newer epoch are rejected to avoid counting a snapshot taken under a different grant.decode_epoch_starts, and a non-advancing history is rejected asInvalidFrame; collision test now decodes a real kind-70 payload to pin the legacy alias.rcgenadded as avtop-nodedev-dependency to generate TLS material for the test.Migration
Written for commit 6f832d5. Summary will update on new commits.