mattp5657 opened a new issue, #4075:
URL: https://github.com/apache/iggy/issues/4075
## Summary
A partition backup's replication gap check accepts or drops an incoming
Prepare based on
one comparison: whether its op number is exactly one past the local
sequencer. It has no
way to tell "I already have this, it's a stale duplicate" apart from "my
sequencer was
advanced past this during a view change, but the body never arrived." After
a view change
that inherits an uncommitted op, and once anything else advances the
sequencer past that
op, every future retransmission of it fails the same check, permanently. The
op can never
be acked, so the commit point freezes below it, and every later write is
blocked behind it,
including writes that have themselves reached full quorum. Producers get
silence until
their read timeout expires. The group does not recover on its own.
This was discovered when
```
cluster::fast_primary_rejoin::given_a_quic_producer_when_its_first_roster_hop_is_down_should_reach_the_partition_primary
```
continually failed for #3873
## Reproduction
Fails roughly 70 to 90 percent of runs locally, and on all 4 nextest
attempts in CI. Not
branch specific; reproduces on unrelated PRs.
Failing test:
```
cluster::fast_primary_rejoin::given_a_quic_producer_when_its_first_roster_hop_is_down_should_reach_the_partition_primary
```
**1. Build the server binary first**
```bash
cargo build --bin iggy-server
```
**2. Build and run the test:**
```bash
cargo test -p integration --no-run
# produces target/debug/deps/mod-<hash>
./target/debug/deps/mod-<hash> --exact \
cluster::fast_primary_rejoin::given_a_quic_producer_when_its_first_roster_hop_is_down_should_reach_the_partition_primary
```
Failing runs take about 32s. Passing runs take about 10s.
**3. Capture diagnostics on failure:**
```bash
RUST_LOG=info,iggy=debug ./target/debug/deps/mod-<hash> --exact <test>
```
Per-test logs land in
`test_logs/cluster__fast_primary_rejoin__<testname>_<hash>/`:
`test_stdout.log` (client SDK tracing) and `server_{0,1,2}_stdout.log`
(servers, including
the `iggy.sim` VSR event trace). Note a `RUST_LOG` value that filters out
the server's own
targets breaks harness startup; `info,iggy=debug` is known to work.
Failure output:
```
post-dead-roster-hop: an attempt outlived the whole 30s budget
(0/1 acked, 2 attempts, last error: Some(QuicError))
```
## Evidence
Node-0 is the restarted original primary, node-1 is stopped, node-2 is the
new partition
primary.
View change correctly inherits an uncommitted op, node-0 adopts it but
repair stops one op
short and never widens:
```
view-change quorum merged; repairing up to the merged log before starting
the view
replica=2 view=2 op_head=21 commit_max=20
adopting view from StartView replica=0 old_view=2 new_view=2 op=21 commit=20
partition behind the group frontier; requesting repair from_op=1
commit_to_op=20 fetch_to_op=20 peer=2
partition journal repair complete shard=1 through_op=20
```
The primary retransmits the inherited op for the rest of the run; every
retransmit is
rejected:
```
(node-2) prepare timeout: retransmitting un-acked prepares replica=2 view=2
targets=1 first_op=21
(node-0) WARN iggy.partitions.diag: op=21 sequence=22 ... dropping
out-of-order prepare (gap)
```
The client's write reaches full quorum and still never commits:
```
PrepareAcked ... op=22 ack_from_replica=0 ack_count=2 quorum=2
quorum_reached=true
```
No `OperationCommitted` ever follows; at shutdown `commit_max` is still 20
on both survivors.
The client itself is a bystander: it reconnects to the correct new primary,
sends, then logs
nothing for about 24 seconds until its read timeout fires. It never dials
the stopped node.
## Root cause
The backup admission check is one comparison
(`core/partitions/src/iggy_partition.rs:3333-3352`):
```rust
if is_backup && header.op != current_op + 1 {
// "dropping out-of-order prepare (gap)"
return;
}
```
`current_op` comes from the sequencer, not the journal. The check's own
comment states its
limitation directly:
> `sequence` is what separates the two shapes this line covers: a forward
gap (op above the
> sequencer, the hole the repair driver closes) and a retransmit of an op
this replica
> already sequenced. Without it they read identically.
Both shapes fail the same comparison for the same reason, and the check
cannot tell them
apart. It does not fall back to the journal to disambiguate, and the comment
explains why
not: on this plane the journal is memory-only and reads empty right after
every restart, so
checking it here would misfire during ordinary post-restart catch-up.
This is a reasonable tradeoff on its own. It breaks because two things
compound:
1. **Repair narrows its fetch window under a commit lag, which a
just-restarted node
always has** (`core/shard/src/lib.rs:8324`): `let fetch_to_op = if
commit_lag {
commit_to_op } else { head };`. The adopted op's body is never requested.
2. **Once a later op is accepted, the sequencer advances past the hollow op,
permanently.** `Prepare(22)` passes the check (`22 == 21+1`), node-0 acks
it and its
sequencer moves to 22. Every later retransmission of op 21 now fails the
identical check
for the identical reason op 22 passed it. The window in which op 21 was
still
recoverable closes the instant anything else moves past it.
`ack_quorum_reached` (`core/consensus/src/plane_helpers.rs:399`) advances
the commit point
by walking forward from `commit_max` and breaking at the first entry without
quorum. Op 21
never gets a second ack, so the walk breaks there on every attempt, and op
22's own full
quorum is never reached. Replies are generated on commit, so the client is
never answered.
With 3 configured replicas and 2 live, quorum has no spare capacity: one
wedged backup
takes down the whole group.
## Fix
Close the window rather than trying to fix the check's disambiguation
problem. The repair
request already fetches a specific op range, not "everything": `from_op =
commit_min + 1`,
bounded by `fetch_to_op`. The fix computes `fetch_to_op` correctly
(`core/shard/src/lib.rs:8313-8324`), using `missing_suffix`, which the code
already
computed two lines above but never consulted in this branch:
```rust
let fetch_to_op = if commit_lag && !missing_suffix { commit_to_op } else {
head };
```
The cost of widening under a commit lag, refetching the committed prefix, is
already being
paid today regardless, since `from_op` is unconditionally `commit_min + 1`.
This only
extends the endpoint to cover the inherited suffix, bounded by pipeline
capacity. If the
body arrives before anything else can advance the sequencer past it, the gap
check never
has to make a distinction it cannot reliably make.
Fixing the gap check itself would be a harder, separate problem: the obvious
version,
checking the journal, is the exact thing its own comment rules out for the
post-restart
case. Any fix there would need to recognize the specific
adopted-but-unfilled range rather
than querying journal presence directly. Not needed here since the fix above
closes the
window the gap check would otherwise have to reason about.
--
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]
