broker,node,protocol: record and expose which fencing epoch wrote each stretch of the log (#240) - #258
Conversation
…#240) Promotion establishes a committed boundary by asking replicas where their disks are and taking the offset a majority can vouch for. That is sound arithmetic over unsound inputs: an offset is a bare integer, and two replicas both reporting 90 need not hold the same record at 90. One may have taken 90 from a leader whose writes never reached a quorum before it was deposed, the other from the leader that replaced it. Comparing them by number cannot tell those apart. Kafka shipped exactly this bug and fixed it in KIP-101 with a leader-epoch → start-offset vector, reconciling against the epoch rather than the high-water mark. This is that vector, durable per replica. `(epoch, start_offset)` records that a replica's first record under `epoch` sits at `start_offset`, so the entries partition the log. Two replicas agree on a prefix exactly as far as their vectors agree, and the first differing epoch bounds divergence: below the smaller of the two start offsets, both wrote under identical leadership and hold identical records. `divergence_point` returns that offset. It is the fact truncation needs and a bare offset cannot supply — which is why this lands before the truncation step of #240 rather than after. Truncating against a high-water mark without epoch qualification can discard ACKNOWLEDGED records, which is the failure KIP-101 exists to prevent. Recorded on epoch adoption, on both a leader and a follower, and fsynced before the replica serves under that epoch: a vector that loses its newest entry in a crash is back to comparing bare offsets at exactly the moment the answer matters. An unrecordable start marks the history broken, and a broken history reports EMPTY rather than partial. Absence and breakage are deliberately the same answer, because they license the same decision: this replica's offsets are bare numbers and must not be reconciled by epoch. A partial vector would be worse than none — it reads as authoritative and yields a truncation target computed from a hole. vtop-broker carries no `tracing` dependency and this does not add one; making the unsafe state structurally unusable beats a log line someone has to notice. Verified live: scenario 09 passes and writes real journals — leader (epoch 1, start 0) promoted follower (epoch 2, start 300) other follower empty The promoted follower's entry says epoch 2 begins at 300, the boundary its verified promotion established, so any replica claiming records at 300 or above under epoch 1 is now provably diverged rather than merely differently numbered. The third replica has no lease block in that scenario, never adopts, and correctly reports "unknown". Nothing reads the vector across the wire yet; exposing it on the replica-status plane and truncating against it are the next steps of #240.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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. |
![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/data_node.rs">
<violation number="1" location="crates/vtop-node/src/data_node.rs:292">
P1: Static leaders and followers never record the epoch that writes their initial log, so future promotion sees an empty/unknown vector for the entire preexisting history. Initializing the journal at the current next offset for the initial held epoch is needed in both role paths before serving.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // stretch of this replica's log. Promotion cannot compare two replicas' | ||
| // offsets without it — a bare offset says where a replica is, not whose | ||
| // writes put it there. | ||
| follower.set_fencing_epoch_journal( |
There was a problem hiding this comment.
P1: Static leaders and followers never record the epoch that writes their initial log, so future promotion sees an empty/unknown vector for the entire preexisting history. Initializing the journal at the current next offset for the initial held epoch is needed in both role paths before serving.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/vtop-node/src/data_node.rs, line 292:
<comment>Static leaders and followers never record the epoch that writes their initial log, so future promotion sees an empty/unknown vector for the entire preexisting history. Initializing the journal at the current next offset for the initial held epoch is needed in both role paths before serving.</comment>
<file context>
@@ -285,6 +285,16 @@ async fn run_follower(
+ // stretch of this replica's log. Promotion cannot compare two replicas'
+ // offsets without it — a bare offset says where a replica is, not whose
+ // writes put it there.
+ follower.set_fencing_epoch_journal(
+ vtop_broker::fencing_epochs::FencingEpochJournal::open(
+ config.data_dir.join("fencing-epochs"),
</file context>
All nine were real. The two that mattered most: TORN-TAIL RECOVERY NEVER TRUNCATED THE FILE. The fragment was dropped from memory and left on disk, so the next append landed after it and every entry from then on was misaligned. The failure surfaced on the restart AFTER the recovery — reading (epoch, start) pairs straddling entry boundaries, which either refuses to open (losing a history this code had already recovered once) or yields wrong offsets. Now truncates to the last whole entry and fsyncs, which is what the comment always claimed it did. A test appends a fragment, recovers, records, and reopens — the reopen is where a survivor would show. THE EPOCH BECAME VISIBLE BEFORE ITS START WAS DURABLE. `fetch_max` is what admits writes under a new epoch, and the start was recorded after it. A concurrent write in that window is a record written under an epoch whose start offset is not yet known, so the journal would name a start ABOVE the first record that epoch actually wrote — and a later divergence comparison would attribute that record to the previous epoch, misreading the exact boundary this vector exists to fix. Recording now happens before the epoch is exposed, reading the tail as of before it is servable. The rest: * `epoch_starts` read the broken flag before taking the lock, so a record that failed while the call was waiting could return a now-partial vector as authoritative. Flag is read under the lock. * A failed write or fsync left the file possibly holding a partial record and allowed further appends onto it. The journal poisons and refuses to append until reopened and revalidated. * `divergence_point` reported two EMPTY vectors as a proven common prefix at 0. Empty means "unknown" everywhere else in this API; returning agreement let a caller truncate on mutual ignorance. Now `None`. * `open` allocated on the file's length before checking the entry bound, so a malformed journal could exhaust memory during recovery — at the moment the broker is trying to come back. Bounded first. * Creating the journal fsynced the file but not its directory, so a power loss could take the directory entry and the whole history with it. * A replica with a STATIC epoch never calls `adopt_fencing_epoch`, so it reported "unknown" for its entire history and could never be reconciled. Seeded on install — but only when the log is also empty, which the review's suggested fix did not require. For a replica that already holds records this process cannot know where its held epoch began; claiming it started at the current tail is a fabricated boundary, and a truncation computed from it could discard acknowledged records. "Unknown" is the honest answer there. * Seeding also skips epoch 0: that is the "no grant yet" sentinel a lease-driven replica starts at, not an epoch that ever wrote anything. It showed up live as a leader recording (0,0) before its first real grant. 13 journal tests, three of them new and aimed at the fixes above. Verified live — scenario 09 now produces complete lineage where the first version recorded almost none: leader (1,0) promoted follower (1,0) (2,400) other follower (1,0) The promoted follower carries the whole range's lineage, not just its own tenure, which is what makes it comparable against a peer.
|
All nine findings were real and are fixed in The two that matteredTorn-tail recovery never truncated the file. The fragment was dropped from memory and left on disk, so the next append landed after it and every entry from then on was misaligned. The failure surfaces on the restart after the recovery — reading The epoch became visible before its start was durable. Exactly as described: The rest, all as reported
One place I diverged from the suggested fixStatic replicas never recording their initial epoch — correct, and fixed. But I seed only when the log is also empty, which the suggestion did not require. For a replica that already holds records, this process cannot know where its held epoch began; those records may have been written under an older one. Claiming it started at the current tail is a fabricated boundary, and a truncation computed from it could discard acknowledged records — the failure this whole vector exists to prevent. "Unknown" is the honest answer there, and the API already handles it. Seeding also skips epoch 0, the "no grant yet" sentinel a lease-driven replica starts at. That one showed up live: the first version of the fix recorded Verification13 journal tests (3 new, aimed at the fixes above); workspace tests, clippy and fmt clean. Live, scenario 09 now produces complete lineage where the first version recorded almost none: The promoted follower carries the whole range's lineage rather than just its own tenure — which is what makes it comparable against a peer at all, and the thing the original version silently failed to do. |
allamiro
changed the title
|
Scope note — this PR grew since the review, and the description was stale. Flagging rather than letting it pass unnoticed.
Nothing is wrong with the code, but it deserves review attention it was not flagged for, so here is what was added: New wire messages (kinds 67/68)
A new kind rather than widening kind 66. The decoder rejects trailing bytes, so growing Three things worth checkingThe decoder enforces advancement, not just the writer. A peer's history is a claim; a non-advancing one would hand this replica a truncation target that discards acknowledged records. Rejecting it only on write would leave the receiving side trusting whatever arrives. Bounded before allocating (
Found on the way inThere is a kind allowlist gating the frame header before dispatch, separate from the decode match. I missed it initially and the round-trip test failed with Still trueNothing consumes the vector yet. Promotion reading it and truncating against it is the next step of #240, and it needs both halves of this to exist. Protocol round-trip tests cover a populated history, an empty one (the "unknown" case an older replica produces), and rejection of a non-advancing one. Workspace tests, clippy, fmt clean; scenarios 00 and 09 pass. |
There was a problem hiding this comment.
1 issue found across 6 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-protocol/src/lib.rs">
<violation number="1" location="crates/vtop-protocol/src/lib.rs:353">
P3: The wire bound (4096) is inconsistent with the local journal bound (MAX_ENTRIES = 1<<20) in crates/vtop-broker/src/fencing_epochs.rs. A replica with a valid, locally recoverable journal holding more than 4096 (epoch, start) pairs — which `open()` accepts because it is within MAX_ENTRIES — will fail `encode_message` with a Limit error the moment its `epoch_history` is encoded, so it can never serve its intact vector and silently reports "unknown" on the wire. If 4096 is meant to be a hard product limit, the local writer should refuse to grow past it; if not, the wire cap should match the local MAX_ENTRIES so a valid vector can always be transmitted.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| /// A range needs this many leadership changes to reach it; past that the | ||
| /// cluster has a flapping problem an operator must see, and the reader must | ||
| /// not allocate on a peer's say-so regardless. | ||
| pub const MAX_REPLICA_EPOCH_STARTS: usize = 4096; |
There was a problem hiding this comment.
P3: The wire bound (4096) is inconsistent with the local journal bound (MAX_ENTRIES = 1<<20) in crates/vtop-broker/src/fencing_epochs.rs. A replica with a valid, locally recoverable journal holding more than 4096 (epoch, start) pairs — which open() accepts because it is within MAX_ENTRIES — will fail encode_message with a Limit error the moment its epoch_history is encoded, so it can never serve its intact vector and silently reports "unknown" on the wire. If 4096 is meant to be a hard product limit, the local writer should refuse to grow past it; if not, the wire cap should match the local MAX_ENTRIES so a valid vector can always be transmitted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/vtop-protocol/src/lib.rs, line 353:
<comment>The wire bound (4096) is inconsistent with the local journal bound (MAX_ENTRIES = 1<<20) in crates/vtop-broker/src/fencing_epochs.rs. A replica with a valid, locally recoverable journal holding more than 4096 (epoch, start) pairs — which `open()` accepts because it is within MAX_ENTRIES — will fail `encode_message` with a Limit error the moment its `epoch_history` is encoded, so it can never serve its intact vector and silently reports "unknown" on the wire. If 4096 is meant to be a hard product limit, the local writer should refuse to grow past it; if not, the wire cap should match the local MAX_ENTRIES so a valid vector can always be transmitted.</comment>
<file context>
@@ -317,6 +317,41 @@ pub struct ReplicaStatusResponse {
+/// A range needs this many leadership changes to reach it; past that the
+/// cluster has a flapping problem an operator must see, and the reader must
+/// not allocate on a peer's say-so regardless.
+pub const MAX_REPLICA_EPOCH_STARTS: usize = 4096;
+
/// Lineage-aware durable consumer progress. Bound to topic epoch, range
</file context>
…240) All four were real. THE MIXED-VERSION DEGRADED PATH DID NOT WORK. The doc promised that an older peer which does not know kind 67 yields an empty history rather than an error, and the code only mapped an Error frame to empty. But such a peer fails in its own read_frame, before any handler runs, and drops the connection without writing a reply — so the case arrives as a clean EOF, which was a hard error. The one path the comment existed to guarantee was the one it did not cover. Now handled, which means conflating "too old to answer" with "died mid-request": safe here because telling those apart is not this call's job. Liveness comes from the status probe, which still fails loudly, and both mean the same thing for the value returned — we do not know this replica's history. Unknown only ever disables epoch reconciliation; it never authorises a truncation. A malformed reply or bad peer identity is still an error, because those are faults rather than answers. THE WIRE BOUND AND THE LOCAL BOUND DISAGREED — 4096 against 1<<20. A replica could hold a journal this code accepts on open and be structurally unable to transmit it, reporting "unknown" to every peer forever while its vector sat intact on disk. The bounds are now one constant, taken from the wire, so "locally recoverable" implies "transmittable" by construction. The wire is the right source: a vector exists to be compared against a peer's, so one too large to send cannot do the only job it has. The size branch reserved before enforcing the bound, so an over-long history allocated on the way to being refused. Bounded in the branch encode_frame actually reserves from. sync_parent used the empty path from Path::parent on a bare filename, so a journal opened by relative name failed on create for a reason unrelated to the journal. Normalised to ".". Tests: a journal opens at a bare relative path, and the two bounds are asserted equal so they cannot drift apart again.
|
All four fixed in The mixed-version degraded path did not workYou are right, and this is the finding that mattered. The doc comment guaranteed that an older peer yields an empty history rather than an error, and the code only mapped Now handled at the The two bounds disagreed4096 on the wire against 1<<20 locally, exactly as you describe: a replica could hold a journal I took your first option — one constant — and sourced it from the wire, so "locally recoverable" implies "transmittable" by construction. The wire is the right authority here: this vector exists to be compared against a peer's, so one too large to send cannot do the only job it has. Raising the wire cap to 1<<20 would instead have made a 16MB status response legal, which is not a thing a promotion should be waiting on. There is a test asserting the two are equal so they cannot drift apart again. Worth noting the local bound was never a cliff: hitting it makes Allocating before the boundCorrect, and the fix is yours. The check lived in the writer, which runs after Bare relative pathAlso correct — Workspace tests, clippy ( |
There was a problem hiding this comment.
1 issue found across 3 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/fencing_epochs.rs">
<violation number="1" location="crates/vtop-broker/src/fencing_epochs.rs:627">
P2: The new test mutates the process-wide current directory without synchronization, so a concurrent cwd-sensitive test can resolve relative paths inside this temporary directory and become flaky. Running this check in a subprocess or using a lock shared by all cwd-sensitive tests would isolate the global state.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| let previous = std::env::current_dir().unwrap(); | ||
| // Serialised against other tests only by being the single one that | ||
| // changes the process directory; it restores it before returning. | ||
| std::env::set_current_dir(dir.path()).unwrap(); |
There was a problem hiding this comment.
P2: The new test mutates the process-wide current directory without synchronization, so a concurrent cwd-sensitive test can resolve relative paths inside this temporary directory and become flaky. Running this check in a subprocess or using a lock shared by all cwd-sensitive tests would isolate the global state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/vtop-broker/src/fencing_epochs.rs, line 627:
<comment>The new test mutates the process-wide current directory without synchronization, so a concurrent cwd-sensitive test can resolve relative paths inside this temporary directory and become flaky. Running this check in a subprocess or using a lock shared by all cwd-sensitive tests would isolate the global state.</comment>
<file context>
@@ -602,4 +615,30 @@ mod tests {
+ let previous = std::env::current_dir().unwrap();
+ // Serialised against other tests only by being the single one that
+ // changes the process directory; it restores it before returning.
+ std::env::set_current_dir(dir.path()).unwrap();
+ let opened = FencingEpochJournal::open("fencing-epochs");
+ std::env::set_current_dir(previous).unwrap();
</file context>
…260) v0.1.0's release body was entirely boilerplate — install commands, cosign invocations, a maturity banner — and not one word about what the release contained. Someone deciding whether to upgrade learned nothing from it. The cause: generate_release_notes: true was set, but an explicit body takes precedence over it, so the generated notes were never rendered. The option looked like it was doing its job and was doing nothing. The changelog is now generated from the tag range: one entry per merged pull request, grouped by the component prefix the commit subjects already carry, followed by the issues those pull requests closed — each with a title and a link. Built from the API rather than the commit text. Squash subjects look like 'broker: subject (#240) (#258)', where the trailing number is the pull request and an earlier one may be the issue — but only sometimes: a change with no issue has one number, a change closing two has three. Parsing positionally guesses, and a changelog that mislabels an issue as a pull request is worse than none. A trailing reference that IS closed is dropped because the closes link says it; one that is NOT closed is a different fact and becomes a link rather than being discarded. Install and verification instructions move to docs/RELEASE_VERIFICATION.md, linked from the body. They are reference material: unchanged between releases, wrong to re-read every time, and long enough that they buried the part that varies. The doc now also states what SHA256SUMS and the Sigstore bundle each establish, since checking one and skipping the other is the easy mistake. The generator is linted in CI, path-filtered onto itself. It only ever executes during a tag build, where a failure means a published release with a broken body and no chance to fix it before anyone sees it.
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
First step of #240. Does not close it — the remaining items build on this.
Promotion takes the offset a majority of replicas can vouch for. That is sound arithmetic over unsound inputs: an offset is a bare integer, and two replicas both reporting 90 need not hold the same record at 90. One may have taken 90 from a leader whose writes never reached a quorum before it was deposed; the other from the leader that replaced it. Comparing them by number cannot tell those apart.
Kafka shipped exactly this bug and fixed it in KIP-101 with a leader-epoch → start-offset vector. This is that vector, durable per replica.
Why this lands before truncation
#240's second item is "truncate followers to the boundary". Doing that first would have been actively dangerous: truncating against a high-water mark without epoch qualification can discard acknowledged records. That is the failure KIP-101 exists to prevent, and I would have been writing it fresh.
divergence_point— walk two vectors, stop at the first differing epoch, take the smaller of the two start offsets — is the fact truncation needs and a bare offset cannot supply.One design decision worth reviewing
A start that cannot be recorded marks the history broken, and a broken history reports empty rather than partial.
Absence and breakage are deliberately the same answer because they license the same decision: this replica's offsets are bare numbers and must not be reconciled by epoch. A partial vector is worse than none — it reads as authoritative and yields a truncation target computed from a hole.
vtop-brokercarries notracingdependency and this does not add one. Making the unsafe state structurally unusable beats a log line someone has to notice.Verification
10 unit tests on the journal, covering the properties that actually matter rather than round-trips: re-recording the same epoch is a no-op (a polling watcher does it constantly), the same epoch at a different offset is refused rather than merged, a stale observation is ignored rather than treated as a rewind, a torn tail recovers the whole entries, and a non-advancing file is refused outright.
Live, from scenario 09 — real journals on real disk:
The promoted follower's entry says epoch 2 begins at 300 — the boundary its verified promotion established — so a replica claiming records at 300+ under epoch 1 is now provably diverged rather than merely differently numbered. The third replica has no
leaseblock in that scenario, never adopts, and correctly reports "unknown".Workspace tests, clippy, and fmt clean; scenario 09 passes.
Scope
Nothing reads the vector across the wire yet. Exposing it on the replica-status plane and truncating against it are the next steps of #240, and they need this to exist first.
Summary by cubic
Records per-replica fencing-epoch starts and exposes them over the replication protocol to make log comparison epoch-aware and safe. Adds a durable journal and hardens mixed-version handling and bounds; prepares safe truncation for #240.
New Features
FencingEpochJournal(durable epoch→start-offset) with entries(), latest(), end_of_epoch(), and divergence_point(); appends are fsynced before serving.epoch_starts()returns empty when unknown/broken.ReplicaStatusClient::epoch_history(); handlers on leader/follower; defaults to empty for mixed versions.Bug Fixes
MAX_REPLICA_EPOCH_STARTSbefore reserving and use the same bound in the journal (viavtop_protocol); fix directory sync for bare relative paths; tests assert the local and wire bounds stay equal.Written for commit 04b3bf1. Summary will update on new commits.