Skip to content

Order chunk metadata and snapshot scheduling with rollback - #8243

Open
Amaury Chamayou (achamayou) wants to merge 1 commit into
achamayou-reject-stale-view-writesfrom
achamayou-order-chunk-metadata-rollback
Open

Order chunk metadata and snapshot scheduling with rollback#8243
Amaury Chamayou (achamayou) wants to merge 1 commit into
achamayou-reject-stale-view-writesfrom
achamayou-order-chunk-metadata-rollback

Conversation

@achamayou

@achamayou Amaury Chamayou (achamayou) commented Aug 30, 2026

Copy link
Copy Markdown
Member

Stacked on #8242. Review the top commit only; the base PR must merge first.

Why this is necessary

LedgerChunker decides where ledger chunks end, and Snapshotter decides where snapshots are taken. Both accumulate state that outlives the transaction which contributes to it, and both are reset by Store::rollback() when a view change discards uncommitted work.

Neither was ordered against the commit path. Store::commit() appended an entry''s size to the chunker after releasing version_lock, so this interleaving was possible:

  1. a batch is assembled and its entries applied;
  2. a view change rolls the store back, clearing chunk metadata above the rollback point;
  3. the batch resumes and appends its size anyway.

The chunker is now permanently one entry ahead of the store. Its counter is independent of the store''s version, so the offset never self-corrects: every subsequent entry is recorded against the wrong version, get_unchunked_size() sums the wrong range, and chunk boundaries drift from what the ledger actually contains. Snapshotter::rollback() had the same shape, able to interleave with a commit recording a committable index.

What changes

Both are updated under version_lock, the same lock the rollback takes, and the append is skipped when the batch''s rollback epoch or view no longer holds.

The two conditions are complementary rather than redundant, which is worth stating because it is not obvious:

  • a rollback can only discard a batch''s writes by truncating, which requires its target below version and therefore increments rollback_count;
  • a rollback that does not truncate leaves the writes intact, but may still move the view on, which consensus will reject.

This invariant is recorded as a comment at the check, so a future change to rollback semantics cannot silently invalidate it.

Why this is minimal

Only the ordering changes. No new locks are introduced and no state is added: the epoch and view compared were already captured by Store::commit() for its existing last_replicated guard.

Moving the chunker and snapshotter calls inside the existing version_lock section is what makes them atomic with respect to rollback_count, which is the property the check depends on. Leaving them outside and adding a separate lock would reintroduce the same window.

Performance

This is the one change in the stack with a cost on the commit path: one version_lock acquire/release per entry in a batch. The lock is short and usually uncontended, but it scales with batch size. If it shows up in benchmarks the check can be hoisted out of the loop, since the epoch cannot change mid-loop without the whole batch failing anyway.

Lock ordering is unchanged in shape: commit_lock -> version_lock -> chunker_lock. commit_lock is only ever taken at the top of Store::commit(), before version_lock, and LedgerChunker is a leaf that holds no store reference, so no cycle is introduced.

Testing

kv_test gains "Chunk metadata is not restored by a batch a rollback discarded". It drives the race deterministically and without threads, using a PendingTx that performs the rollback inline - Store::commit() invokes it in exactly the window concerned.

Verified that the test fails without the fix (the chunker ends one entry ahead of the store) and passes with it.

Labelled run-long-test.


Review stack

These five PRs come from one investigation and are stacked; review and merge in order.

PR Change
#8242 Reject stale-view writes before local commit
#8243 Order chunk metadata and snapshot scheduling with rollback
#8244 Stop a rollback moving chunk metadata forward
#8245 Guard rollback-sensitive transaction flags
#8246 Guard reserved signature side effects

All were found by an interleaving-exploration harness built over the real KV, consensus and history stack (draft #8238). The harness itself is deliberately not included here; these PRs carry only the fixes and the single-threaded regression tests that pin them.

Ledger chunk sizes were appended, and snapshot scheduling rolled back, outside the lock guarding the rollback epoch. A transaction could therefore restore chunk metadata for an entry a concurrent view change had already discarded, leaving the chunker permanently ahead of the store and skewing every later chunk boundary.

Take the version lock for both the rollback and the append, and skip the append when the batch's rollback epoch or view no longer holds. A rollback can only discard a batch's writes by truncating, which moves the epoch on; a rollback that does not truncate may still move the view, which consensus rejects - so both are checked.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 75d99c5d-6efa-4048-8032-8c78b97208d9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-long-test Run Long Test job

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant