Skip to content

fix(consensus): stop view change discarding committed ops - #3824

Open
krishvishal wants to merge 2 commits into
masterfrom
vsr-dvc-headers
Open

fix(consensus): stop view change discarding committed ops#3824
krishvishal wants to merge 2 commits into
masterfrom
vsr-dvc-headers

Conversation

@krishvishal

Copy link
Copy Markdown
Contributor

What this fixes

A committed, client-acknowledged op survived a primary crash only if the round-robin new primary happened to hold it.

DoViewChange carried scalars only, so the new primary took the winner's op number without its log, rebuilt its pipeline from its own journal, hit the local gap, and truncated the range as "decided lost". The op was journaled on a quorum, committed, and already replied to; the next client op reused the number and collided with the stale entry on an up-to-date backup. The comment justifying the truncation cited a DVC merge that did not exist.

DoViewChange now carries the sender's uncommitted suffix, a nack bit and a present bit per entry, and the new primary merges the quorum's headers. Discarding requires a nack quorum, which quorum intersection makes unreachable for anything that may have committed. Otherwise the view does not start: unavailable and saying so beats available and short a reply.

Changes

Independent defects on the same path.

Prepare identity. PrepareHeader.checksum was never set, so parent chained zeros and every merge comparison was 0 == 0. Now sealed on both planes and verified at all four ingress points, not just metadata repair: a frame corrupted in flight was journaled as-is and re-served, which the interior-corruption boot refusal turns into an unbootable node. The WAL scan checks it and the parent chain, since a flipped commit reaches recovery's max(header.commit) watermark. view is excluded, because restamp_prepare_view rewrites it in place.

Zero, enforced two ways. For prepares CHECKSUM_UNSEALED is 0 and means "skip", so existing WALs replay. For control headers a zero is a FrameChecksumMismatch: keying on "looks sealed" lets one flipped bit disable the layer by clearing the field that gates the check. Tolerant on disk, strict on the wire.

Control frame integrity. Canonical selection took the first sender scanned and read a differing header as an implicit nack, so one bit flipped in that sender's suffix turned every honest sender's correct header into a nack against the garbage: a nack quorum on three replicas, above commit_max where the refusal does not fire. Control headers now carry a checksum over every byte past it, verified on the typed parse before validate reads a field. Body verification keys on body presence, not on whether checksum_body looks sealed.

Merge correctness. Canonical selection consults every canonical sender and treats disagreement as undecidable, since senders at the canonical log_view cannot legitimately disagree when a primary prepares one thing per op. Only a sender behind that log_view may nack implicitly, a constraint the comment stated and nothing tested. The decoder recomputes each identity and checks view and timestamp monotonicity and the hash chain. A tripwire that panicked on same-log_view disagreement is gone; it fired on remote bytes.

Quorum sizing. Splitting the replication and view-change quorums to define the nack quorum surfaced a latent bug: at replica_count 4 both were 2, so they could be disjoint.

log_view timing. Raised when the merge parked, before the merged head was installed, so a primary-elect superseded or crashed mid-repair held a durable claim to a view whose headers it never had, then carried its own stale head as sole canonical sender of the next view change.

Parked-log roles. A backup's parked log is a verification reference, not a repair window. It doubled as the view's repair scope, so once traffic passed the view's opening head every repaired op was discarded unnamed and the backup never converged.

Latched quorum flag. Set on reaching a view-change quorum, not on deciding a log, so every non-Ready outcome was terminal: an AwaitingRepair waiting for more DoViewChange messages had guaranteed it would ignore them.

Repair serving. Bounded by the local frontier again. to_op comes from a peer with only from_op <= to_op validated and the skip loop walks op by op with no await, so on the single-threaded shard pump an unclamped u64::MAX ends the shard.

Divergence reconciliation. Journal::truncate_from drops a diverging uncommitted suffix and leaves the snapshot watermark alone. Not drain, which advances the watermark past what it removed: correct for a committed prefix, ruinous for a suffix, marking the ops that must stay refillable as evictable.

Data at rest. Disk polls verify each batch against its batch_checksum and fail closed. Consumer offset files gain a checksum, since the offset is a cursor reloaded unchanged on every restart and a flipped bit silently rewinds or skips a consumer. Bare pre-checksum files still load and upgrade on the next write, and the legacy server stays compatible both ways by reading the first eight bytes and stopping. Separable from the consensus work.

@github-actions github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label Aug 5, 2026
@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.98355% with 409 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.43%. Comparing base (0a23e16) to head (d93940e).

Files with missing lines Patch % Lines
core/shard/src/lib.rs 86.98% 82 Missing and 12 partials ⚠️
core/binary_protocol/src/consensus/header.rs 72.13% 65 Missing and 3 partials ⚠️
core/consensus/src/impls.rs 80.78% 47 Missing and 2 partials ⚠️
core/consensus/src/view_change_quorum.rs 73.33% 44 Missing and 4 partials ⚠️
core/journal/src/prepare_journal.rs 86.14% 27 Missing and 10 partials ⚠️
core/consensus/src/dvc_merge.rs 93.99% 23 Missing and 2 partials ⚠️
core/simulator/src/deps.rs 24.13% 22 Missing ⚠️
core/partitions/src/iggy_partition.rs 81.48% 12 Missing and 3 partials ⚠️
core/server-ng/src/offset_recovery.rs 0.00% 11 Missing ⚠️
core/journal/src/lib.rs 0.00% 8 Missing ⚠️
... and 7 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3824      +/-   ##
============================================
+ Coverage     76.40%   76.43%   +0.02%     
  Complexity     1046     1046              
============================================
  Files          1334     1335       +1     
  Lines        165228   167529    +2301     
  Branches     137583   139990    +2407     
============================================
+ Hits         126242   128045    +1803     
- Misses        35271    35657     +386     
- Partials       3715     3827     +112     
Components Coverage Δ
Rust Core 76.48% <85.98%> (+0.07%) ⬆️
Java SDK 63.67% <ø> (ø)
C# SDK 71.13% <ø> (-1.14%) ⬇️
Python SDK 88.14% <ø> (ø)
PHP SDK 84.50% <ø> (+1.52%) ⬆️
Node SDK 96.36% <ø> (+0.08%) ⬆️
Go SDK 43.08% <ø> (ø)
Files with missing lines Coverage Δ
core/configs/src/server_ng_config/metadata.rs 98.21% <100.00%> (+2.17%) ⬆️
core/configs/src/server_ng_config/partition.rs 100.00% <ø> (ø)
core/configs/src/server_ng_config/validators.rs 85.42% <ø> (+0.02%) ⬆️
core/consensus/src/lib.rs 0.00% <ø> (ø)
core/partitions/src/iggy_partitions.rs 70.10% <100.00%> (+0.07%) ⬆️
core/partitions/src/lib.rs 0.00% <ø> (ø)
core/partitions/src/types.rs 34.48% <ø> (ø)
core/server-ng/src/dispatch.rs 39.73% <100.00%> (+0.08%) ⬆️
core/server-ng/src/partition_reconciler.rs 92.63% <100.00%> (+<0.01%) ⬆️
core/simulator/src/lib.rs 97.33% <100.00%> (+0.14%) ⬆️
... and 18 more

... and 62 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review PR is waiting on a reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant