numinnex commented on code in PR #4506:
URL: https://github.com/apache/iggy/pull/4506#discussion_r4270237258
##########
core/shard/src/lib.rs:
##########
@@ -6559,11 +6718,17 @@ where
}
if consensus.is_normal()
&& !consensus.is_transferring()
- && consensus.commit_min() < consensus.commit_max()
+ && (consensus.commit_min() < consensus.commit_max() ||
consensus.view_log_unproven())
&& self.metadata_repair.borrow().is_none()
{
- let to_op = consensus.commit_max();
- let from_op = consensus.commit_min() + 1;
+ let to_op = if consensus.view_log_unproven() {
+ consensus.sequencer().current_sequence()
Review Comment:
**critical**: server panic. While unproven, `to_op` is the sequencer and
`from_op` is `view_log_repair_op` (the adopted head). A backup's `on_replicate`
(metadata.rs:1705, `set_sequence(header.op)`) rewinds the sequencer below that
head when a retransmit fills a journal gap. That builds `RequestPrepares
[38,37]`, and the `expect` at consensus_message.rs:463 ("repair range must be
non-empty and 1-based") aborts the process.
Repro: metadata swarm seed 7002, and `--plane lifecycle --shell --clients 3
--faults swarm --crash-prob 0.003 --restart-prob 0.02 --crash-primary --ticks
40000 --heal-before-quiesce` seed 20037. 32/200 metadata seeds at head, 0 on
the previous head and base; a clamp mutant gives 0.
Fix: `to_op = max(seq, pending.op_head, from_op)`, and never send an empty
range. The partition twin (:9288) is lower risk because partition backups only
move the sequencer forward.
##########
core/server/src/http/forward.rs:
##########
@@ -332,92 +534,69 @@ fn record_relayed_floor(state: &HttpInner, user_id: u32,
response: &Response) {
state.metadata_watermarks.record(user_id, applied);
}
-async fn forward_partition_or_pass(state: HttpState, request: Request, next:
Next) -> Response {
- if !state.forward.active ||
request.headers().contains_key(FORWARDED_HEADER) {
- return next.run(request).await;
- }
- // A read that does not move consumer progress is answered by whichever
- // replica the caller reached, which is the point of reading from one. Only
- // an automatic commit needs a replica that can originate the offset
- // operation, so only that poll pays for the buffering and the credential
- // resolution below.
- if request.method() == Method::GET &&
!wants_auto_commit(request.uri().query()) {
- return next.run(request).await;
- }
+async fn attach_partition_request(state: HttpState, request: Request, next:
Next) -> Response {
+ let received = Instant::now();
+ let hop = match partition_hop(request.headers()) {
+ Ok(hop) => hop,
+ Err(error) => return CustomError::from(error).into_response(),
+ };
+ // Verify-only, like the metadata relay: an unauthenticated caller gets no
+ // buffered body and no relay. The handler authenticates again.
let bearer = match bearer_token(request.headers()) {
Ok(bearer) => bearer,
Err(error) => return CustomError::from(error).into_response(),
};
if let Err(rejection) = resolve_credential(&state, bearer).await {
return rejection.into_response();
}
-
- let (parts, request_body) = request.into_parts();
+ let _guard = if matches!(hop, PartitionHop::Target(_)) {
+ let Some(guard) = ForwardGuard::admit(&state.forward.in_flight) else {
Review Comment:
warning: the Target guard is admitted before the untimed `to_bytes` body
read, and Target comes from client-supplied headers. 128 authenticated
slow-body connections pin `MAX_IN_FLIGHT_FORWARDS`, and this node's own
outbound relays then get `forward_busy` 503, which is passed to clients
unretried. No test pins the bound.
Admit after the body is read (scope the guard to the hold), or give Target
holds their own cap.
##########
core/shard/src/lib.rs:
##########
@@ -5469,6 +5639,8 @@ where
return;
}
partition.apply_repaired_prepare(msg).await;
+ let pending = partition.consensus().pending_view_log();
+ reconcile_partition_view_divergence(self.id, partition,
pending.as_ref()).await;
Review Comment:
warning: every repaired frame, held duplicates included, clones
`pending_view_log` and reruns the full reconcile (O(op_head - gap) walk plus a
linear canonical find), so a gap of G costs O(G^2) CPU on the shard pump.
Reconcile once per RepairDone, or only when the frame filled the missing op.
##########
core/consensus/src/impls.rs:
##########
@@ -3666,6 +3704,9 @@ impl<B: MessageBus, P: Pipeline<Entry = PipelineEntry>>
VsrConsensus<B, P> {
// it: the new primary's numbers are at least as fresh as any probe
// answer.
self.finish_view_probe();
+ let needs_reconciliation = self.view_log_unproven.get()
Review Comment:
nit: the same-view fix (skip the early return while unproven) and the
metadata StartView suffix refresh (shard/lib.rs:5092) are unpinned: mutants
survive consensus (221), shard (183) and sim (242) tests. The new metadata
unproven-repair arming (shard/lib.rs:6724) is also untested, which is how the
empty-range panic got in. Add a metadata twin of partition_io.rs:4174 and a
same-view Commit-while-unproven test.
##########
core/partitions/src/iggy_partition.rs:
##########
@@ -11540,8 +11847,12 @@ where
self.consensus().set_commit_floor(floor);
Review Comment:
warning (suspect, conf L): `complete_repair` can raise the commit floor
while the view log is unproven. That can jump commit_min over durable
dead-branch entries, which then count as proven (impls.rs:4023-4027) and are
served by repair. Refuse the floor jump while unproven, or re-prove first.
##########
core/partitions/src/iggy_partition.rs:
##########
@@ -8542,11 +8764,45 @@ where
}
async fn truncate_resident_from(&mut self, from_op: u64) -> Result<usize,
IggyError> {
+ self.remove_resident_range(from_op..=u64::MAX).await
+ }
+
+ /// Replace a proven mismatch while retaining the canonical tail across
gaps.
+ ///
+ /// # Errors
+ /// Returns an error if the WAL or resident indexes cannot be rewritten.
+ pub async fn remove_divergent_prepare(&mut self, op: u64) -> Result<(),
IggyError> {
Review Comment:
nit: removing a divergent prepare clears `certified_log_view` durably
(partition_journal.rs:565), so a crash before the refill reboots as
materialization-missing and needs a full state transfer
(iggy_partition.rs:1724-1734). Correct, but expensive; worth a comment or
keeping the certificate for the unchanged prefix.
##########
core/simulator/src/bin/workload-fuzz.rs:
##########
@@ -625,6 +653,23 @@ fn run_quiesce_phase(
args.min_metadata_ops_compared, convergence.ops_compared,
));
}
+ if sim.is_shell()
+ && workload.options.weights.draws_fixture_groups()
+ && convergence.owners_compared < args.min_owners_compared
+ {
+ vacuous(format_args!(
+ "--min-owners-compared {}: the run joined fixture groups, but the
owner oracle \
+ compared only {} settled owner(s) (seed={seed:#x})",
+ args.min_owners_compared, convergence.owners_compared,
+ ));
+ }
+ if sim.is_shell() && args.plane == Plane::Lifecycle &&
workload.owner_fence_refusals() == 0 {
Review Comment:
nit: the vacuity gate exits 3 on many lifecycle `--shell` swarm seeds (7/24
and 18/30 in two runs; e.g. seeds 1, 2, 5, 8, 18, 21, 23), all with
`stale_owner_stores=0`. The workload never forces a stale-owner store, so a CI
cell on a random seed would flake. Deterministically schedule one stale store
per run.
##########
core/server/README.md:
##########
@@ -39,6 +39,22 @@ cargo run --bin iggy-server --release -- --replica-id 0
Command-line arguments include `--replica-id`, `--fresh` (`-f`),
`--with-default-root-credentials`, and `--list-config-env-vars`. Other settings
come from configuration.
+## Upgrading
+
+Deploy matching server and SDK releases together. Rolling upgrades and mixed
+server versions are unsupported. Partition control frames from older servers
+carry incarnation zero and are rejected, so a mixed-version cluster cannot
+complete partition view changes. The `IGGY-OWNERSHIP-HISTORY-5` storage gate
also
+rejects older data directories without modifying them. Existing data requires a
+separately validated offline migration or restore procedure before this release
+can use it.
+
+Cluster metadata includes `replica_id`. Java's `ClusterNode` record adds a
leading
+`replicaId` component, changing constructor calls and record patterns. C#
callers
+must initialize the required `ReplicaId` property. The CLI's cluster metadata
list
+appends a replica ID column. Go, Java, Node.js and C# clients use command
105's new
Review Comment:
nit: the upgrade list misses the Swift `ClusterNode(replicaID:)` initializer
(Resources.swift:586), the C++ FFI `ClusterNode.replica_id`
(foreign/cpp/src/lib.rs:338) and the new pub field on Rust `iggy_common`
`ClusterNode`/`ClusterNodeResponse`. The PR body (squash message) still lacks
an SDK paragraph and still says "Simulator checks cover replacement histories".
##########
core/partitions/src/iggy_partition.rs:
##########
@@ -8125,6 +8360,7 @@ where
if self.fatal.is_some()
|| self.materialization_missing
|| self.commit_waits_for_checkpoint()
+ || self.consensus.view_log_unproven()
Review Comment:
**critical**: this gate covers only `commit_journal`. The `resume_io` walk
at :5909-5914 (`select_persistable_commits` /
`collect_committable_from_journal` -> `select_commit` (:10075) ->
`finish_pending_commit` never checks `view_log_unproven` /
`view_log_entry_proven`. The walk is capped at `persistence.head()`, which
still covers dead-branch ops made durable in the old view, so a fenced backup
applies them.
Seed 7009 (7016 flags): r1 restarts, proof `Missing{60}`, then commits its
discarded v7 op 59 while unproven (backtrace through `finish_pending_commit`).
Seed 7056 does the same with op 75.
Committed divergence is worse than before this fix: partition 7000-7299
17/300 (previous head 6/300), mixed 1000-1149 9/150 (previously 0). With
`select_commit` refusing while unproven, both drop to 0.
Fix: gate per op on `view_log_entry_proven` in `select_commit` and
`collect_committable_from_journal`.
##########
core/shard/src/lib.rs:
##########
@@ -8991,32 +9262,32 @@ where
let commit_to_op = consensus.commit_max();
let commit_lag = consensus.commit_min() < commit_to_op;
let head = consensus.sequencer().current_sequence();
- if !commit_lag && head <= commit_to_op {
+ if !commit_lag && head <= commit_to_op &&
!consensus.view_log_unproven() {
return false;
}
let missing_suffix = partition_missing_suffix_through(partition);
// Fetch the adopted suffix even while committed operations lag. Later
// live prepares can advance the head while an adopted body is missing.
- let Some(fetch_to_op) =
+ let Some(fetch_to_op) = consensus.view_log_repair_op().map(|_|
head).or_else(|| {
partition_repair_fetch_to_op(consensus.commit_min(), commit_to_op,
missing_suffix)
- else {
+ }) else {
return false;
};
// A resident window may only be waiting for persistence or a bounded
commit walk.
- if partition
- .log
- .journal()
- .inner
- .repaired_window_shape(consensus.commit_min(), fetch_to_op)
- .complete
+ if !consensus.view_log_unproven()
+ && partition
+ .log
+ .journal()
+ .inner
+ .repaired_window_shape(consensus.commit_min(), fetch_to_op)
+ .complete
{
return false;
}
let nonce = iggy_common::random_id::get_uuid();
- let from_op = consensus.commit_min() + 1;
- let cluster = consensus.cluster();
- let self_id = consensus.replica();
- let namespace = consensus.group();
+ let from_op = consensus
+ .view_log_repair_op()
+ .unwrap_or_else(|| consensus.commit_min() + 1);
Review Comment:
**critical** (liveness): while fenced, repair starts at
`view_log_repair_op`, the HIGHEST missing op, because the proof walks top-down,
and requests up to head. Each round trip fills one hole and re-serves up to 128
bodies already held. A unit probe filled a 2046-op gap in 2076 rounds (258k
bodies, 2.2 s of pump CPU, about quadratic); a ring-sized gap is about 65k
round trips before falling back to state transfer. Base repairs upward from
commit_min+1 in 128-op chunks.
Sim: 7016 flags, seeds 20000-20399, "did not drain" 17/400 at head vs 0/400
on base. On seed 20007, with r0 down, op 300 was served 125 times but never
ingested, so the two live replicas stayed wedged for 50k ticks: the fenced
backup withholds PrepareOk and there is no quorum.
Fix: request the chunk that ends at the hole (`from = max(commit_min+1,
missing-127)`, `to = missing`), so the parent chain proves 128 ops per round
trip. The same applies to the resume path at :5966.
##########
core/metadata/src/impls/metadata.rs:
##########
@@ -4231,6 +4224,9 @@ where
#[allow(clippy::future_not_send)]
pub async fn commit_journal(&self) {
let consensus = self.consensus.as_ref().unwrap();
+ if consensus.view_log_unproven() {
Review Comment:
warning (suspect): commit is fenced here, but the metadata superblock
(:3719, via shard tick) persists the new `log_view` even while
`view_log_unproven`, but the fence itself is not durable (impls.rs init false).
After a crash, entries that were unproven at or below the fence are reported
under the new log_view. The merge refuses on a same-log_view conflict, so this
looks like a stall rather than loss. The partition plane is safe
(certify_log_view requires the WAL head checksum). Consider deferring the
persist until proven.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
