feat(metadata): state transfer for snapshot and client table - #3765
feat(metadata): state transfer for snapshot and client table#3765numinnex wants to merge 19 commits into
Conversation
# Conflicts: # core/common/src/error/iggy_error.rs # core/consensus/src/client_table.rs # core/consensus/src/metadata_helpers.rs # core/integration/tests/cluster/client_table_restart.rs # core/metadata/src/impls/metadata.rs # core/metadata/src/impls/recovery.rs # core/metadata/src/lib.rs # core/sdk/src/vsr.rs # core/server-ng/src/bootstrap.rs # core/server-ng/src/dispatch.rs
# Conflicts: # core/consensus/src/impls.rs # core/shard/src/lib.rs
Master's clients_table_max knob (#3756) calls set_capacity after install_client_table, but WAL-replay recovery repopulates the table before bootstrap reaches either call, so every restart with a registered client tripped the "set_capacity must run before any client registers" assert and the node refused to boot. Order the capacity set before the install, build the recovered table at the configured capacity instead of the compile-time default, and plumb the same knob into the shard's state-transfer decode so an installed table never falls back to the default either. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Checkpoints are node-local, so a healthy serving primary can offer a snapshot behind the receiver's own applied frontier (a backup checkpoints an op or two below the primary it later replaces). The install blindly restored that snapshot in place, rewinding the state machine below commit_min with no way back: the commit walk never revisits ops it already counted as applied, so the rewound-over effects were silently lost. Keep the local STM in that case and install only the client table, which comes from the serving primary's live state and is never behind. Caught by new checkpoint-shaped state-transfer specs (restart exactly at a drained journal, multiple snapshot generations, double restart), which also grow the harness with ANSI-stripped stdout markers and an occurrence counter, and promote the forced-checkpoint log to info so tests can pin checkpoint placement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
server-ng has served HTTP for a while (roster bind, advertised addresses, vsr-only http_rbac/http_tls suites), but two race tests still excluded the transport under vsr behind a stale "server-ng exposes no HTTP listener" note that came back with a stash-pop. Unify the concurrent-addition matrix (80 cells in both modes) and the segment-rotation race to all four transports; the 20 HTTP cells and the rotation race run green against a 3-node vsr cluster. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3765 +/- ##
============================================
- Coverage 75.96% 75.91% -0.05%
Complexity 969 969
============================================
Files 1320 1320
Lines 157608 157516 -92
Branches 130990 130977 -13
============================================
- Hits 119720 119581 -139
+ Misses 34297 34275 -22
- Partials 3591 3660 +69
🚀 New features to boost your workflow:
|
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
two notes with no diff line to hang them on: recovery.rs:437 still carries TODO(state-transfer): there is no metadata state transfer yet - this PR is its trigger condition.
| return; | ||
| }; | ||
|
|
||
| consensus.set_state_transfer_stage(consensus::StateTransferStage::Installing); |
There was a problem hiding this comment.
this set_state_transfer_stage(Installing) is the only unguarded stage set in the shard, and Idle -> Installing is an illegal transition, so it's an assert! panic that takes down shard 0. reachable: the probe-exhausted fallback clears the stage to Idle (legal from Fetching, so silent) but lives in the consensus crate and can't touch metadata_transfer, so the session survives with its nonce intact; none of the receive handlers (on_state_transfer_target, on_state_chunk, request_pending_state_chunk) checks the stage, so the pull completes into this assert. on_state_transfer_target also sets target_accepted = true unconditionally while moving the stage only from AwaitingTarget - same missing invariant from the other side. fix needs both halves: bail out of on_transfer_progress when the stage isn't Fetching, and drop the session wherever the stage gets cleared.
| }, | ||
| )* | ||
| [<$state Command>]::RestoreSnapshot(snapshot) => { | ||
| *self = Self::inner_from_snapshot(snapshot.clone()); |
There was a problem hiding this comment.
inner_from_snapshot mints a fresh Arc::new(StatsRegistry::default()) per call, and this arm runs once per left-right buffer (absorb_first and absorb_second both dispatch it), so after an install the two buffers hold different registries. boot is only safe because from_snapshot builds one inner and new_from_empty clones it. worse, inner_from_snapshot never repopulates the registry's partitions map, so every Arc<PartitionStats> the data plane registered at bootstrap/reconcile is orphaned - partition_get returns none and partition stats read (0,0,0,0) permanently on any node that state-transferred, while stream/topic aggregates alternate between the two arc sets by publish parity. the comment three lines above the Arc::new in stream.rs states exactly the invariant this breaks.
fix: pass the existing shared registry into the rebuild (this arm has self to source it) and prune it to the snapshot's key set while keeping survivors' partition entries - slab keys get reused, so a stale entry would hand a new stream a dead one's counters. note the only test pinning exact counts is cfg(not(feature = "vsr")), so CI can't see this.
| return None; | ||
| } | ||
| let coordinator = self.coordinator.as_ref()?; | ||
| let snapshot = std::fs::read(coordinator.snapshot_path()).ok()?; |
There was a problem hiding this comment.
three problems in this function, one rewrite fixes all of them.
the raw fs::read ships the file including its integrity trailer, and neither end ever runs split_trailer: install decodes and re-persists the bytes raw, so write_durably appends a second trailer (+20 B per transfer generation, tolerated only because rmp-serde ignores trailing bytes - any future strict decode bricks every transferred node at boot). worse, corruption-at-rest that survives the msgpack decode (a flipped bit inside a u64 or a string body) gets re-sealed on the receiver with a fresh valid trailer and a matching pairing: a fault the source node would refuse boot over becomes undetectable on the second node.
both .ok()? here also swallow the error, and the shard-side refusal log then says "not a caught-up primary, or no snapshot persisted" - actively wrong when the real cause is an unreadable or corrupt snapshot.bin.
and the whole thing (read + full msgpack decode of every stream/topic/user to get one u64 + full table encode) runs synchronously on shard 0's pump, rebuilt on every RequestStateTransfer including stall retries.
suggested shape: fs::read with the error logged, split_trailer as verification, keep the decode for sequence_number, ship the stripped payload (the installed file is then byte-shape-identical to a local checkpoint), and serve repeat requests from the cached offer instead of rebuilding. deriving the seq from last_checkpoint().0 instead does not work: write_durably renames before the parent-dir fsync, so a DirSync failure leaves the new file live with the cell stale, and the offer would under-advertise the frontier.
| served.fully_served = true; | ||
| } | ||
| artifact_bytes | ||
| .get(start..end.min(artifact_bytes.len())) |
There was a problem hiding this comment.
get(start..end.min(artifact_bytes.len())) returns Some(&[]) when start == len, so this answers a fully-consumed artifact with an empty chunk instead of dropping the request - and the handler already reset served.idle_ticks above, so the offer never expires. the receiver accepts the empty payload (offset matches, no overrun), extends nothing, resets its own idle_ticks, and immediately re-requests the same offset. both liveness counters pinned at zero, attempts never increments: the transfer has no bound at all, the two nodes ping-pong empty frames at network rate, and the rejoining replica withholds PrepareOk for the life of the process - a 3-node cluster is permanently down to 2 acking replicas.
reachable via the offer rebuild: a stall-retry RequestStateTransfer reuses the session nonce, and on_request_state_transfer rebuilds the offer and replaces the cache entry under that same nonce. the client table is encoded live, so if the first descriptor was delayed rather than lost, the receiver holds manifest A while chunks come from offer B. B.len > A.len is caught by the artifact checksum; B.len < A.len (a client logged out between the two builds) lands the cursor exactly on B.len and enters this loop.
fix wants all three: refuse chunk requests with start >= len, don't reset either idle counter on a zero-byte payload, and make the cached offer first-wins per (requester, nonce) so a rebuild can't replace a manifest the receiver already accepted.
| // snapshot being installed already contains those ops. | ||
| // `commit_max` still advanced inside `handle_commit`, so | ||
| // the post-install repair targets the right frontier. | ||
| if !consensus.is_transferring() { |
There was a problem hiding this comment.
this gate is right, but the same walk is reachable ungated from on_start_view (line 2225) and on_do_view_change (line 2128) - the AwaitingTarget early-return only covers that one stage, so a StartView arriving during Fetching walks the pre-install STM. two consequences: the walk can advance commit_min past the incoming snapshot_seq, flipping snapshot_ahead to false so the install silently degrades to table-only (no STM restore, no persist, no pairing) while still logging success; and if the advance lands inside the install's superblock await, set_commit_floor's anti-rewind assert panics the shard. same one-line gate belongs on both paths.
| pub use client_table::{ | ||
| CachedReply, ClientEntrySnapshot, ClientTable, ClientTableDecodeError, ClientTableSnapshot, | ||
| CommitReply, | ||
| CachedReply, ClientEntrySnapshot, ClientTable, ClientTableCodecError, ClientTableDecodeError, |
There was a problem hiding this comment.
ClientTableCodecError next to the pre-existing ClientTableDecodeError - neither name says which format (msgpack checkpoint vs transfer wire), and "codec" oversells it since encode is infallible. ClientTableWireError or similar would disambiguate.
| // withholds `PrepareOk` on its own (`is_transferring`). Gating it | ||
| // would also wedge the one path that repairs a replica whose gap | ||
| // sits below every peer's floor. | ||
| if consensus.state_transfer_stage() == consensus::StateTransferStage::AwaitingTarget { |
There was a problem hiding this comment.
this arms on stage alone and ignores whether handle_start_view accepted anything - it returns an empty action set for wrong-primary / old-view / below-floor, but the arm still mints a fresh nonce, zeroes attempts, and fires a request at a possibly-non-primary. it self-heals via available = 0, but it resets the retry budget, and a stream of stale StartViews re-mints the nonce each time so the in-flight descriptor gets dropped by the nonce filter. gate on the StartView actually being adopted.
| /// the committed prefix the node missed no longer exists as WAL entries on | ||
| /// the peers that checkpointed it. | ||
| pub fn restart_from_clean_slate(&mut self) -> Result<(), TestBinaryError> { | ||
| let cleanup = self.config.cleanup; |
There was a problem hiding this comment.
the cleanup save/restore dance is dead - ServerConfig::cleanup is never read for behavior anywhere in the harness (the real gate is TestContext::cleanup), and this fn wipes the data dir itself two lines later. copied from the same dead pattern in restart.
| fn validate(&self) -> Result<(), ConsensusError> { | ||
| if self.command != Command2::RequestStateTransfer { | ||
| return Err(ConsensusError::InvalidCommand { | ||
| expected: Command2::RequestStateTransfer, |
There was a problem hiding this comment.
the new header-only frames (RequestStateTransferHeader, RequestStateChunkHeader) don't size-check in validate while EvictionHeader does, and StateTransferTargetHeader constrains size only on the available == 0 branch. the generic Message::try_from bound makes this safe either way, but adding == HEADER_SIZE to the two header-only frames (and a comment that the available == 1 body is bounded downstream by the manifest caps) keeps the validate surface consistent.
| #[cfg(not(feature = "vsr"))] | ||
| // Partially vsr-gated inside the module: the bench-fill test requires | ||
| // PARTITION-plane state transfer (sub-floor stats/offset seeding), which is | ||
| // not implemented yet; the metadata-only deletion/restart test runs under |
There was a problem hiding this comment.
dropping the module-level gate puts should_handle_resource_deletion_and_restart into the vsr lane for the first time, and CI doesn't run that lane - green here doesn't prove it passes. worth pasting a local --features vsr run of it in the PR.
github-actions
Bot
added
S-waiting-on-author
2 participants
No description provided.