execution/stagedsync: extract helpers so robustness tests drive production#21046
Merged
mh0lt merged 1 commit intoMay 7, 2026
Conversation
…ction Addresses Copilot's review feedback on #21039: the original tests modeled the exec-loop and apply-loop decision trees in local closures, which would let the model drift from production silently. This follow-up extracts three pure helpers from the production code and rewires the tests to drive them directly. Helpers extracted: * `closeApplyChannels` (pe method) — pins commitResults-then-applyResults close ordering. Returns the close sequence as a string slice so tests can verify the order deterministically without racing on observer- goroutine wakeups (the previous test had a real flake here). * `execLoopShouldExit` (pure func) — encodes the per-blockResult exit precedence: sizeLimit > maxBlockNum > Exhausted > StopAfterBlock. Production exec-loop now switches on the returned decision; tests pin each precedence rule with table-driven cases. * `shouldMarkExhaustedAtBlock` (pure func) — encodes executeBlocks's per-cycle block-limit gate (initial-cycle protections + blockLimit span check + maxBlockNum guard). Replaces the old "dispatch convention" model test that didn't drive any production code. Tests updated: * `TestApplyLoopChannelCloseOrder` — calls `pe.closeApplyChannels()` on a real `parallelExecutor` with mock channels and asserts on the returned close-order slice. Also covers double-close recovery and the post-close nil-out. * `TestExecLoopShouldExitPriority` — table-driven coverage of the exit-priority decisions including the cross-condition cases (e.g. maxBlockNum AND Exhausted both set on the final block — maxBlockNum must win). * `TestShouldMarkExhaustedAtBlock` — table-driven coverage of the initial-cycle gates, blockLimit==0 disabled case, span < limit case, and the maxBlockNum exact-hit no-mark case. No production behaviour change — pure refactor + test rewire.
5 tasks
52dffb3
into
fix/parallel-apply-loop-partial-batch-no-spurious-flag
1 check passed
Member
|
@mh0lt this PR didn't target |
5 tasks
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
2 participants
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.
Follow-up to #21039 addressing Copilot's review feedback that the new robustness tests were modelling the exec-loop and apply-loop decision trees in local closures — which would let the model drift from production silently.
This PR extracts three pure helpers from the production code and rewires the tests to drive them directly. No production behaviour change — the helpers wrap the exact same logic that was inline before.
Helpers extracted
closeApplyChannels(pe method)execLoopdeferred closeexecLoopShouldExit(pure)execLoopper-blockResult branchshouldMarkExhaustedAtBlock(pure)executeBlocksper-cycle limit gatecloseApplyChannelsreturns the actual close-order as a string slice — the previous test was racy because observer goroutines could wake up out of order even when the helper closed the channels in the right order; the slice removes that race entirely.Tests rewired
TestApplyLoopChannelCloseOrder— now callspe.closeApplyChannels()on a realparallelExecutorwith mock channels and asserts on the returned close-order slice. Also covers double-close recovery and the post-close nil-out.TestCloseApplyChannelsDoubleCloseRecovers— verifies the recover-on-double-close path doesn't count pre-closed channels as freshly closed.TestExecLoopShouldExitPriority— table-driven coverage of each branch, including cross-condition cases (e.g. maxBlockNum AND Exhausted both set on the final block — maxBlockNum must win for the clean reachedMaxBlock path).TestShouldMarkExhaustedAtBlock— table-driven coverage of the initial-cycle gates (`lastExecutedStep`/`lastFrozenStep`/`DiscardCommitment`), the `blockLimit==0` disabled case, the span < limit case, and the `maxBlockNum` exact-hit no-mark case.The previous `TestExecLoopHonorsBlockResultExhausted` and `TestExecLoopExhaustedOnlySetOnFinalBlock` tests are removed — both are subsumed by the new `TestExecLoopShouldExitPriority` and `TestShouldMarkExhaustedAtBlock` cases that drive production code.
Test plan
Stack
Stacked on #21039 (the apply-loop fix). Both should land together — this PR alone doesn't make sense without #21039's helper-using tests as context.