Stop a rollback moving chunk metadata forward - #8244
Open
Amaury Chamayou (achamayou) wants to merge 1 commit into
Open
Stop a rollback moving chunk metadata forward#8244Amaury Chamayou (achamayou) wants to merge 1 commit into
Amaury Chamayou (achamayou) wants to merge 1 commit into
Conversation
Amaury Chamayou (achamayou)
force-pushed
the
achamayou-clamp-chunker-rollback
branch
from
August 31, 2026 07:50
e32fbd1 to
3d925bc
Compare
This was referenced Aug 31, 2026
A rollback whose target is at or beyond the store's own version discards nothing, but still reset the chunker to that target. The chunker can legitimately lag the store, because an entry is allocated a version well before its size is recorded, so this moved the chunker forward past the store and left a permanent offset that skewed every later chunk boundary. Clamp the target to the store's version, so a rollback can only ever move chunk metadata back. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 75d99c5d-6efa-4048-8032-8c78b97208d9
Amaury Chamayou (achamayou)
force-pushed
the
achamayou-clamp-chunker-rollback
branch
from
August 31, 2026 12:29
3d925bc to
4562e3c
Compare
cjen1-msft
approved these changes
Sep 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this is necessary
Store::rollback()has two paths. If the target is below the store''s version it truncates; if the target is at or beyond it, nothing local is discarded and it returns early. Both paths reset the chunker to the rollback target.For the truncating path that is right. For the early-return path it is not, because the chunker is allowed to lag the store. An entry is assigned a version by
next_version()well beforeStore::commit()records its size, so at any moment the chunker can legitimately be behind. Resetting it to a target at or beyond the store''s version therefore moves it forward, past entries whose sizes were never recorded.The chunker''s counter is independent of the store''s version and only ever advances, so the offset is permanent. Every subsequent entry is then recorded against the wrong version,
transaction_sizesacquires a hole, andget_unchunked_size()silently under-counts - chunk boundaries drift from what the ledger contains, andcompacted_to()can prune entries that were never accounted for.A rollback to exactly the current version is routine: it is how a view change is communicated when there is nothing to truncate.
What changes
Clamp the rollback target to the store''s own version, so a rollback can only ever move chunk metadata back:
chunker->rolled_back_to(std::min<Version>(tx_id.seqno, version));Why this is minimal
One
std::minon the non-truncating path. The truncating path is untouched - there the target is belowversionby construction, so the clamp would be a no-op.The alternative, keeping the chunker eagerly in step with allocation rather than with commit, would mean recording sizes for entries that may never be replicated, which is the resurrection problem the previous PR in this stack fixes.
Performance
None. One comparison on a path taken only during a view change.
Testing
kv_testgains "A rollback never moves chunk metadata past the store''s version", covering both a rollback to exactly the current version and one beyond it, then asserting later entries are still recorded against their own version. Single-threaded and deterministic.Verified that the test fails without the clamp - the chunker ends ahead of the store, and stays ahead - 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.
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.