[r3.5] execution/stagedsync: prune overlay to committed boundary on no-op execution unwind - #21849
Merged
Merged
Conversation
…o-op execution unwind Cherry-pick of #21847 (release/3.4) / #21848 (main) to release/3.5. The no-op-disk-unwind branch in UnwindExecutionStage pruned the in-RAM overlay to Min(u.UnwindPoint+1), but that branch does not call u.Done, so re-execution resumes from the committed progress (SeekCommitment == s.BlockNumber+1), not u.UnwindPoint+1. When committed progress sits below u.UnwindPoint (common with BatchCommitments), the overlay writes for (s.BlockNumber, u.UnwindPoint] are kept but re-executed, re-reading their own stale overlay — same bug class, deferred. Prune to the committed boundary (Min(s.BlockNumber+1)) instead, and restructure into an explicit disk-unwind / overlay-only if-else with a shared tail. This also hoists doms.ResetPendingUpdates() ahead of the branch so it runs on both paths (the pre-refactor early return skipped it). See #21847 for the full rationale. Relates to #21681 Co-authored-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts UnwindExecutionStage to correctly prune the in-RAM SharedDomains/TemporalMemBatch overlay to the committed execution boundary on the “no-op disk unwind” path (when nothing above the unwind point was committed), aligning the overlay state with the actual re-execution resume point.
Changes:
- Restructures
UnwindExecutionStageinto explicitdisk-unwindvsoverlay-onlybranches and ensuresdoms.ResetPendingUpdates()runs on both paths. - Fixes the overlay prune boundary in the overlay-only path to use
Min(s.BlockNumber+1)(committed boundary) instead ofMin(u.UnwindPoint+1). - Updates/extends the regression test to assert pruning of writes in
(committedBlock, unwindPoint]as well as the failed-block write, while preserving committed writes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
execution/stagedsync/stage_execute.go |
Corrects overlay prune boundary on no-op disk unwind; restructures unwind logic and shared tail handling. |
execution/stagedsync/stage_execute_unwind_test.go |
Strengthens regression coverage to ensure all uncommitted overlay writes above committed progress are pruned. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| _, _, _ = doms.SeekCommitment(ctx, rwTx) // ensure internal state of `doms` is set | ||
| _, _, _ = doms.SeekCommitment(ctx, rwTx) // re-establish doms' position at the post-unwind committed state |
- Trim the disk-branch and overlay-only comments per CLAUDE.md (incident forensics and PR refs live in the commit message / PR body, not source). - Fix wording: TemporalMemBatch.Unwind prunes the in-RAM overlay (not "on-disk"); SeekCommitment returns the committed block s.BlockNumber (re-execution resumes at s.BlockNumber+1) — the two were conflated, here and in the test. - Propagate the doms.SeekCommitment error instead of discarding it (it now runs on the no-op path and sets the txNum the removed SetTxNum used to). Addresses @yperbasis review on #21847. Co-authored-by: Claude <noreply@anthropic.com>
yperbasis
approved these changes
Jun 17, 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.
Cherry-pick of #21847 (release/3.4) / #21848 (main) to
release/3.5(which carries #21826).Same change: in
UnwindExecutionStage, prune the in-RAM overlay to thecommitted boundary (
Min(s.BlockNumber+1)) on the no-op-disk-unwind pathinstead of
Min(u.UnwindPoint+1), and restructure into an explicitdisk-unwind/overlay-onlyif/elsewith a shared tail. Full rationale in#21847 (yperbasis #1 boundary, AskAlexSharov structure, yperbasis #2 state-cache
note).
Clean cherry-pick of the
maincommit —release/3.5matchesmainhere(it has
doms.ResetPendingUpdates(), which the restructure hoists ahead of thebranch so it runs on both paths; the test uses
[:]slicing ascommon.Hash/common.Addressare raw arrays).Verification on release/3.5 + this patch
Min(u.UnwindPoint+1)boundary the test fails (the(committed, unwindPoint]write0x1122survives); with this PR it passes.go build ./execution/stagedsync/...— cleango test ./execution/stagedsync/ -run TestUnwindExecutionStage_PrunesUncommittedOverlayWrite— passgofmt/golangci-lint run ./execution/stagedsync/— 0 issuesRelates to #21681.
Co-authored-by: Claude noreply@anthropic.com