protocol,broker,node,cli,chaos: a leader seals its tail on demand — repair reaches its whole position (#306) - #341
Conversation
…epair reaches its whole position (#306) Only sealed segments transfer, and that stays true: the fix is not to transfer a moving tail but to stop it moving, deliberately. SealTail (wire kinds 81/82 — nothing below 81 is free; 70 is the live legacy cursor alias) asks the leader to roll now: the tail seals, a successor opens at its end, and the freshly sealed segment is transferable the moment the call returns. The arithmetic the issue states — a repaired replica up to 8 GiB behind a leader whose retransmission buffer holds 8 MiB — closes because the prefix now reaches the leader's position at the seal, not wherever the range last happened to roll. The issue's four questions, answered: WHO ASKS — the replica-plane RPC, driven by vtopctl node repair --seal-tail, gated by the same transfer allowlist (sealing exists FOR the transfer; a peer that may not pull the bytes has no business reshaping the leader's segments). FENCING — checked before the seal with the transfer plane's own one-snapshot check; a deposed leader refuses rather than sealing a tail the cluster moved past, and the broker-level seal runs under the append path's own lock because rolling belongs to the append critical section. REFUSALS — an empty tail over a sealed prefix is an idempotent no-op reported as zero records sealed, distinguishable from progress by a retrying repair; a never-written range is refused with the reason. COST — one shorter segment per repair, taken only on demand, and retention runs after the seal exactly as it runs after an ordinary roll. LocalBroker::seal_tail is the first production caller of SegmentSet::roll. The handler default refuses, in the fence camp: a peer that silently succeeded without sealing would report a tail transferable that is still moving. Scenario 12's inverted pin fires positive: the replacement must now reach the FULL acknowledged floor — the bound moves from the sealed- prefix end to , the repair must exit 0, and a run that stops at the old boundary fails the scenario instead of being tolerated with a caveat. Proven live: committed through 1500 >= 1500, eight sealed segments, across the leader transition. The byte-for-byte check now matches segments BY NAME — with the sealed tail in the transfer, 'the last .segment' in the two directories are different segments.
|
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: 0d9cc016a1
ℹ️ 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".
| self.check_fencing(fencing_epoch)?; | ||
| let (sealed_end, records_sealed) = self | ||
| .broker | ||
| .seal_tail() |
There was a problem hiding this comment.
If the leader loses its lease after check_fencing returns but before broker.seal_tail() acquires the broker-state lock, the metadata publisher can clear or advance the lease in that gap and this deposed leader still performs the roll. Unlike the produce path, which holds the metadata and broker locks together from authorization through the durable write, this newly introduced write is therefore not fenced atomically; hold the lease snapshot until the seal completes or revalidate it while holding the state lock.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, and the sharpest finding on this PR — fixed in 462730d. Verified against the produce path, which documents exactly the discipline the seal was missing: metadata lease view first, then broker state, both held through the write, 'so a concurrent grant/release cannot revoke between the fencing check and fsync'. seal_tail now takes range and epoch and validates them INSIDE, under both locks, using the same check_range the produce path uses; the handler's one-shot check_fencing no longer participates. The stale-epoch broker test still pins refusal-without-mutation.
| state.segment.roll_minting().map_err(BrokerError::from)?; | ||
| // A seal adds a sealed segment, and retention reasons in sealed | ||
| // units — the same follow-through the produce path's roll performs. | ||
| self.run_retention(&mut state.segment); |
There was a problem hiding this comment.
When retention is enabled with max_total_bytes below the former tail's content size and the committed floor reaches its end, this retention pass can immediately delete every sealed segment, including the one just created. The RPC then reports sealed_end == tail_next, but the listing used by --seal-tail cannot transfer that prefix, so repair remains behind or has no segment to adopt; retention configuration accepts any nonzero bound, so this is a valid deployment rather than an impossible state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, and fixed in 462730d: retention no longer runs in the seal. A bytes-bound pass could reclaim the very segment the call just sealed — the committed floor covers it by construction — and the RPC would then report a sealed_end the listing cannot reach. The seal exists to make the tail transferable; housekeeping that could unmake it waits for the next ordinary roll, and the doc states the decision.
![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 9 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… retention waits Review round one, both P1s verified real and fixed. The seal's fencing check ran through the transfer handler's one-shot check_fencing, then took the broker state lock separately — leaving a window where a grant or release lands between the check and the roll, and a deposed leader still mutates its layout. The produce path already documents the discipline that closes it: metadata lease view first, then broker state, BOTH held through the write. seal_tail now takes the range and epoch and validates them inside, under both locks, with the same check_range the produce path uses — the handler's job reduces to authorization and transport. And retention no longer runs in the seal: a bytes-bound pass could reclaim the very segment the call just sealed — the committed floor covers it by construction — and the RPC would then report a sealed_end the transfer listing cannot reach. The seal exists to make the tail transferable; housekeeping that could unmake it waits for the next ordinary roll. Also from review: the operator hint after a post-seal gap now says a SECOND repair over the adopted directory is refused and names the fresh-directory remedy; the client method no longer splits transfer_sealed_prefix's doc block; and the never-written refusal's string uses proper continuations instead of embedding an indent run.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 462730d3e1
ℹ️ 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".
| state | ||
| .segment | ||
| .roll_minting() | ||
| .map_err(|problem| (ErrorCode::Storage, problem.to_string()))?; | ||
| Ok((tail_next, records_sealed)) |
There was a problem hiding this comment.
When retention is enabled and the committed former tail exceeds max_total_bytes, an append arriving after this method returns but before the repair's separate listing request can delete the segment that was just sealed: flush_produce_group calls run_retention after every successful append at lines 1673-1677, not only after an ordinary roll. The fresh evidence relative to the earlier comment is that this fix suppresses retention only inside seal_tail, so the cross-RPC window remains; the seal can report success while the following transfer cannot reach sealed_end. Keep the segment pinned until the transfer obtains its snapshot, or combine sealing with that snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified — you're right that the in-call suppression closes only half the window: run_retention fires after every successful append (confirmed at the flush_produce_group call site), so the cross-RPC slice remains. Where I land differently is on the consequence, and 0a84b1b encodes that reasoning rather than a pin: neither end of the window can produce a FALSE success — a shorter listing becomes a measured, reported gap (the #303 exit-1 contract), and a segment reclaimed mid-fetch is a clean resumable refusal, which is the same window every listed segment already lives in until its chunks land. A cross-RPC pin was considered and rejected with the reason now in the doc: the leader cannot know when a repairer is done, and a pin that outlives a crashed repairer is a retention bound that silently stopped being one. What the fix adds instead is DETECTION: the repair compares the seal's promised end with what the fenced listing actually held, and when retention won the race it names the cause to the operator instead of presenting a mysteriously shorter prefix. If a durable pin becomes worth its lifetime management, it deserves its own design slice against #290's retention semantics.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Review round two. Codex is right that suppressing retention inside seal_tail closes only the in-call window: retention runs after every successful append, so a produce landing between the seal RPC and the repair's listing can still reclaim the freshly sealed segment under a bytes bound smaller than it. What does NOT follow is a lie — a shorter listing becomes a measured, reported gap (exit 1), and a segment reclaimed mid-fetch is a clean resumable refusal, the same window every listed segment already lives in until its chunks land. A cross-RPC pin was considered and rejected, with the reason in the doc: the leader cannot know when a repairer is done, and a pin that outlives a crashed repairer is a retention bound that silently stopped being one. What the operator gets instead is the CAUSE, named: the repair compares the seal's promised end with what the fenced listing actually held, and when retention won the race it says so — 'the leader's retention reclaimed sealed segments between the two' — instead of presenting a mysteriously shorter prefix. cubic's doc precision applied: the comment no longer claims the segment persists until a later roll. Two more embedded-whitespace string literals fixed in the same pass.
1 participant
Closes #306.
The arithmetic, closed
Only sealed segments transfer — that contract stays. The fix is not to transfer a moving tail but to stop it moving, deliberately: a new fenced replica-plane RPC,
SealTail(wire kinds 81/82), asks the leader to roll now. The tail seals, a successor opens at its end, and the freshly sealed segment is transferable the moment the call returns. The 8 GiB-tail-vs-8 MiB-retransmission-buffer gap the issue quantifies closes because the transferred prefix now reaches the leader's position at the seal, not wherever the range last happened to roll.The issue's four open questions, answered
vtopctl node repair --seal-tail(sealing happens before the transfer, so the listing that drives it already includes the sealed tail — sealing after would close the gap for the next repair). Gated by the sametransfer_peersallowlist: sealing exists for the transfer, so a peer that may not pull the bytes may not reshape the leader's segments either.check_fencing(the log,broker: a repaired replica has no epoch history, so the first leader transition truncates it back to zero #315/broker,cli: the repair carries the epoch history, so it survives the next leader (#315) #320 discipline), and the broker-level seal runs under the append path's own lock — rolling belongs to the append critical section. A stale-epoch seal request is refused before anything rolls, pinned by test.records_sealed: 0, samesealed_end— so a retrying repair distinguishes it from progress without parsing errors. A never-written range refuses with the reason ("produce to the range before repairing from it"): a degenerate sealed segment would cost a file to say nothing, and adoption would still refuse the empty prefix it decorated.LocalBroker::seal_tailis the first production caller ofSegmentSet::roll(via a newroll_minting, minting from the set's own env like every other minting site). TheReplicaPeerHandlerdefault refuses, in thefencecamp: a peer that silently "succeeded" without sealing would report a tail transferable that is still moving.The inverted pin fires positive
Scenario 12 spent two releases stating the tail gap out loud ("…remain out of its reach until #306 gives the gap a road back"). That statement is now an assertion: the repair must exit 0 (gap fully closed), and the replacement's committed offset must reach the full acknowledged floor — the deadline-poll bound moves from
$SEG_NEXTto$ACKED, and a run that stops at the old sealed-prefix boundary fails the scenario instead of being tolerated with a caveat. The byte-for-byte check now matches segments by name, because with the sealed tail in the transfer, "the last.segment" in the two directories are different segments.Verification
sealed_segment_transfer.rsgrows three assertions — a stale-epoch seal isFencedand mutates nothing; sealing makes the leader's position transferable end-to-end over the wire (seal → idempotent re-seal → transfer → adopt lands exactly at the leader's position → leader still serves appends); a never-written range refuses with the stated reason. 436 tests pass across protocol/log/broker/node, all CLI suites green, clippy-D warningsclean.committed through 1500 >= 1500 with 8 sealed segment(s)across the leader transition.Summary by cubic
Adds a
SealTailRPC and--seal-tailrepair option to seal the leader’s tail before transfer so the sealed prefix reaches the leader’s position and the tail gap is closed. Repair now also reports if retention reclaimed sealed segments between the seal and the listing.New Features
SealTailrequest/response added tovtop-protocol(wire kinds 81/82).LocalBroker::seal_tailrolls under the append lock and returns(sealed_end, records_sealed); usesSegmentSet::roll_minting.ReplicaPeerHandler::seal_tailadded;LeaderSegmentTransferHandlerimplements it;vtop-nodegates sealing by the transfer allowlist.seal_tail;vtopctl node repair --seal-tailseals first so the listing includes the sealed tail; output clarifies that a second repair over an adopted dir is refused and suggests a fresh--seal-tailrepair.Bug Fixes
LocalBroker::seal_tailvalidates range/epoch inside the broker under the metadata + state locks; stale-epoch requests returnFencedwithout mutation.records_sealed: 0; a never-written range is refused with a clear reason.Written for commit 0a84b1b. Summary will update on new commits.