fix(consensus): close metadata and consensus plane TODOs - #3870
fix(consensus): close metadata and consensus plane TODOs#3870krishvishal wants to merge 11 commits into
Conversation
f9c2839 to
86d27cc
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3870 +/- ##
=============================================
- Coverage 82.72% 69.07% -13.65%
Complexity 1296 1296
=============================================
Files 1199 1198 -1
Lines 160319 148794 -11525
Branches 129923 118503 -11420
=============================================
- Hits 132625 102785 -29840
- Misses 24188 42501 +18313
- Partials 3506 3508 +2
🚀 New features to boost your workflow:
|
| /// lowering it back could re-stamp a value a peer already observed. | ||
| /// | ||
| /// See [`PrepareRollback`] for the outcomes. | ||
| pub fn rollback_pipelined_prepare(&self, header: &PrepareHeader) -> PrepareRollback { |
There was a problem hiding this comment.
[blocking] rollback_pipelined_prepare runs after the append await with no view or status fence, and the tail match is by op alone. If a view change completes while the append is parked and this replica is re-elected, start_pending_view rewinds the sequencer to the merged head, so a new request can re-project the same op number with a different checksum. The old append's failure then passes both guards here: it pops the new view's live entry, rewinds the sequencer and parent chain under it, the live entry's acks die at pipeline_holds_entry, the op number is minted again, and backups panic in panic_if_hash_chain_would_break_in_same_view. Suggest refusing unless header.view == self.view() and status is Normal, and matching the tail by (op, checksum).
| return PrepareRollback::Overtaken { sequence }; | ||
| } | ||
| let removed = self.pipeline.borrow_mut().remove_tail(header.op); | ||
| debug_assert!( |
There was a problem hiding this comment.
[blocking] In release builds a None from remove_tail falls through this debug_assert and the sequencer and parent-chain rewind below still run, mutating state exactly when the invariant they rely on is violated. Returning a refusal on None without mutating keeps release behavior aligned with the assertion.
| /// Version 2: `status` sits at reply-header offset 216 (version 1 carried a | ||
| /// `namespace` word before it), which the client table's cached replies embed as raw | ||
| /// wire bytes msgpack cannot introspect. | ||
| pub const SNAPSHOT_FORMAT_VERSION: u32 = 2; |
There was a problem hiding this comment.
[blocking] The serialized shape changes in this PR (release_format appended, #[serde(default)] dropped from client_table) but the version stays 2, and master already writes 6-element version-2 files. On the first cross-build boot the peek passes and rmp_serde fails with an invalid-length error, so the refusal surfaces as SnapshotError::Deserialize, the corruption-shaped diagnosis this gate exists to prevent. Both upgrade and downgrade directions misreport. This violates the bump rule documented above; bump to 3.
| replica_id = consensus.replica(), | ||
| op = header.op, | ||
| sequence, | ||
| "journal append failed after a sibling prepare was projected off this op; \ |
There was a problem hiding this comment.
[warning] Nothing triggers the view change this log waits on. The failed op was never broadcast, backups gap-drop the sibling at the hole check, and this primary keeps sending Commit heartbeats that reset the backups' election timers, so the group wedges permanently while looking healthy. A solo group has no peer at all. The wedge itself predates this PR (any append failure did this), but the log text is new and promises repair that cannot come. Either escalate here (cede primaryship or fail-stop, per the removed TODO) or reword to state that no election will follow without operator action.
| .push_request(consensus::RequestEntry::new(message)); | ||
| if consensus.pipeline_is_full() { | ||
| let push_result = | ||
| consensus.push_queued_request(consensus::RequestEntry::new(message)); |
There was a problem hiding this comment.
[warning] Parity gap: the partitions-plane primary has the same pre-advance in push_prepare_entry, but its append-failure arm still warns and returns without rollback_pipelined_prepare, leaving the sequencer ahead of the WAL on the data plane. The new rollback is plane-agnostic and the failure arm has no half-applied state, so mirroring the metadata call looks safe. Fine as a follow-up since it predates this PR, but worth a tracking issue before merge.
| if empty { | ||
| timeouts.stop(TimeoutKind::Prepare); | ||
| } else { | ||
| timeouts.reset(TimeoutKind::Prepare); |
There was a problem hiding this comment.
[nit] Timeout::reset never sets ticking, so on a stopped timer with a non-empty pipeline this branch leaves it armed-but-dead and the doc's "restarts it" does not hold. Every current caller happens to be preceded by a start-arm (advance_commit_max, become-primary), but the bootstrap re-pipeline gap flagged separately would land on this branch. start when not ticking, reset otherwise; same trap advance_commit_max documents.
| drop(removed); | ||
| self.sequencer.set_sequence(header.op.saturating_sub(1)); | ||
| self.set_last_prepare_checksum(header.parent); | ||
| PrepareRollback::Unwound |
There was a problem hiding this comment.
[nit] The Unwound path pops the tail without sync_prepare_timeout, so unwinding the sole in-flight entry leaves the Prepare timer ticking on an empty pipeline, against the invariant this PR introduces. The backstop in handle_prepare_timeout caps the cost at one spurious fire.
| } | ||
|
|
||
| #[compio::test] | ||
| async fn recover_pairs_the_checkpoint_against_the_bytes_on_disk() { |
There was a problem hiding this comment.
[nit] This test writes canonical bytes, which re-encode byte-identically (the byte-stability tests pin that), so the recomputed checksum equals the on-disk one under either checksum source and the test passes even if recovery re-encoded the decoded snapshot. Feeding a noncanonical but decodable file, for example the version encoded as a uint32 marker (accepted by read_int, canonicalized on re-encode), restores the discrimination the deleted test had.
| entry.add_ack(topology.self_replica_id); | ||
| pipeline.push(entry); | ||
| } | ||
| consensus.with_pipeline_mut(|pipeline| { |
There was a problem hiding this comment.
[nit] This block pushes recovered entries without arming the Prepare timeout, and init no longer arms it. The branch is unreachable today (clustered restarts cede primaryship, solo forces the watermark to the restored op), but if it reactivates the recovered suffix never retransmits. Needs a sync_prepare_timeout here, which only works once the reset-vs-start issue in that helper is fixed.
| Unwound, | ||
| /// This replica never pre-advanced for the prepare, so there is nothing to | ||
| /// undo. A backup advances only AFTER its append succeeds. | ||
| NotPreAdvanced, |
There was a problem hiding this comment.
[nit] Both labels overclaim. NotPreAdvanced is returned for any non-primary, including a demoted ex-primary that did pre-advance (the view-change reset erased it, so the name is operationally true but literally false). Overtaken also covers sequence < header.op after a state-transfer rewind, where the sibling story in this doc and in the caller's log does not apply. Worth documenting both directions or branching the log text.
| /// shifted remainder. That is not hypothetical: the raw-PAT reply shipped once | ||
| /// without the prefix and broke SDK decoding. This is the only way to emit a | ||
| /// success reply with a body, so the prefix cannot be forgotten again. | ||
| fn build_result_framed_reply( |
There was a problem hiding this comment.
[nit] Two overstatements in the doc: the SDK strips the result section only for is_result_framed() operations (plus a non-empty Register), not every metadata reply, and build_reply_from_bytes still emits success read replies with bodies, so this is not the only path. As written it could lead someone to result-frame a read reply and break decoding in the other direction. Minor: the payload could encode straight into out[RESULT_COUNT_LEN..] instead of a temporary BytesMut, though this path is cold.
| /// [`VsrConsensus::next_monotonic_timestamp`]: this must not consume the | ||
| /// prepare-stamping monotonic sequence, or parking a request would perturb | ||
| /// the timestamps replicated to every backup. | ||
| pub received_at: u64, |
There was a problem hiding this comment.
[nit] The documented reader pattern clock_realtime_micros() - received_at on u64 underflows if the realtime clock steps back between park and promotion (the old i64 kept the sign). No reader exists yet; worth stating the saturating_sub requirement here before one lands.
| /// [`ConsensusHeader`] method, which is why this is a match and not a trait | ||
| /// call. `RepairPrepare` reads through its wrapped prepare. | ||
| #[must_use] | ||
| pub fn routing(&self) -> (Operation, u64) { |
There was a problem hiding this comment.
[nit] header() runs a checked bytemuck cast per call and each arm here reads it twice; the Request and Prepare arms in on_message then re-derive the same tuple, so a hot frame pays four casts where one let header = message.header(); per arm suffices. Same pattern in body(), which re-reads the header for size right after the STM macro's scalar read.
| /// | ||
| /// A build constant, identical on every replica, so two replicas holding identical | ||
| /// state still serialize identically (see [`MetadataSnapshot`]). | ||
| pub const SNAPSHOT_RELEASE_FORMAT: u32 = IGGY_PROTOCOL_VERSION; |
There was a problem hiding this comment.
[nit] The name says format but the content is the packed release semver, and it sits next to a real format version; the doc has to argue against the confusion the name creates. writer_release or similar would remove the ambiguity.
| /// capacity, which moves nothing and proves nothing. | ||
| #[cfg(any(test, feature = "simulator"))] | ||
| #[must_use] | ||
| pub fn buffer_addr(&self) -> usize { |
There was a problem hiding this comment.
[nit] The safety comment argues read-only-ness, but the actual soundness precondition is that no pump-side &mut is live when this forms a &Vec through the UnsafeCell; a shared reborrow aliasing a live &mut is UB regardless of whether it only reads. Current callers uphold it; worth stating directly.
| /// status writes in [`Self::enter_view_change`]. Routing it here instead would | ||
| /// leave a window where the replica has entered a view change without counting | ||
| /// itself, which on a solo group is the entire quorum. | ||
| pub(crate) fn push_loopback(&self, message: Message<GenericHeader>) { |
There was a problem hiding this comment.
[nit] Pre-existing, same class this PR removes on the router hop: send_or_loopback erases the self-addressed PrepareOk to generic and the drain re-types it with a full frame-checksum verify per self-ack, once per op per plane on the primary. Typing the loopback queue as VecDeque<Message<PrepareOkHeader>> would close it.
2 participants
Clears ten
TODOmarkers across the metadata, consensus, and shard planes. One commit per item.Correctness
Roll back the sequencer on a WAL append failure.
push_prepare_entryclaims an op before the journal append, so a failed append left the sequencer one ahead of the WAL forever and the next prepare chained over a hole no repair path refills.rollback_pipelined_preparehands the op back and drops the pipeline entry. Refused when a sibling prepare was already projected off the failed op, since unwinding there would hand a live op number out twice.Snapshot format versioning.
versionwas a hardcoded1with no read-side check. Now exact-equality with fail-stop, plus arelease_formatprovenance stamp, TigerBeetle's pair. The version is peeked off the msgpack prefix before the payload is deserialized, so a foreign layout is refused by version rather than by whichever field misparsed. Covers boot recovery and both state-transfer paths.Performance
Zero-alloc op construction in STM apply.
Bytes::copy_from_sliceper op body, then borrowed as&[u8]and dropped.WireDecode::decode_fromtakes a slice, so theBytesbought nothing. Decodes off the backing buffer via a newMessage::body(). Also drops a 256-byte header copy per op.One frame parse per hop. Frames were classified twice, once in the router for routing and once in the shard for dispatch.
ShardFrame::Consensuscarries theMessageBag;MessageBag::routing()/into_generic()replace the router's 60-lineextract_routing.ShardFramestays 160 B, pinned by aconstassert.Structure
Stop leaking
&RefCell<Pipeline>. 49 call sites move to named accessors pluswith_pipeline/with_pipeline_mut, whoseFnOncebound makes a borrow-across-.awaitunrepresentable. Same shape asIggyPartitions::with_partition.Prepare-timeout lifecycle. "Ticking iff the pipeline is non-empty" now holds:
sync_prepare_timeoutdisarms on empty and restarts otherwise, so a remaining head is timed from now instead of inheriting the drained entry's elapsed ticks.init()no longer arms it; a fresh primary has nothing to retransmit.One result-section framing.
build_login_register_replyandbuild_raw_pat_replyeach hand-assembled[u32 count=0][payload]; the raw-PAT reply shipped once without the prefix and broke SDK decoding. One helper is now the only way to emit a success reply with a body.Stamp request-queue arrival.
received_atpopulated at park fromclock_realtime_micros(), notnext_monotonic_timestamp(). The latter advances the prepare-stamping sequence, so parking a request would perturb timestamps replicated to every backup.Fold the duplicated view-change entry. Three sites inlined the same 12-line sequence, differing only in
reason. Extracted toenter_view_change.Tests
Pump realloc under a live partition borrow. The borrow tripwire and deterministic executor already covered a concurrent
remove; this adds the realloc half, where a growingpushrelocates every element. Guarded against passing vacuously by a newbuffer_addr()and anassert_ne!across the insert, and falsification-checked. Swapping the bad read forwith_partitionmakes it fail with "tripwire did not fire".Also: a real-
PrepareJournalappend-failure rollback test, four rollback-outcome tests, two timer-lifecycle tests, a park-stamp test, snapshot version/peek/refusal tests, and a login-register framing test.