test: add structural guard for SavePendingOrchestration ordering#520
Merged
test: add structural guard for SavePendingOrchestration ordering#520
Conversation
Adds a structural test that verifies SavePendingOrchestration() appears before ExecuteWorkerAsync/Task.WhenAll in both SendViaOrchestratorAsync and SendViaOrchestratorReflectAsync. The code already has the correct ordering — this test locks it in to prevent regressions. Closes #517 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
🔍 Multi-Model Code Review — PR #520CI StatusAll Findings Across 4 Review Rounds
All 5 findings resolved across 4 commits. Tests pass (2/2 structural tests green). Round 5 — Final Review (commit 07c66ba, 3/3 reviewers)✅ No new issues found. All 3 reviewers independently verified:
Final Recommendation✅ Approve — clean to merge. |
- Change IndexOf anchor from 'SendViaOrchestratorReflectAsync' (matches call site) to 'private async Task SendViaOrchestratorReflectAsync' (matches method definition) so the test actually guards the reflect path. - Move Assert.True(reflectSave >= 0) before using reflectSave as startIndex to avoid ArgumentOutOfRangeException on failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… assertion Compute reflectWhenAll from reflectMethod (same independent anchor as reflectSave) instead of from reflectSave, so the < comparison is load-bearing. Previously it was vacuously true since IndexOf with startIndex always returns >= startIndex. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nc check to reflect path - Non-reflect path: substring from Phase 3 marker to ExecuteWorkerAsync method definition, preventing fall-through to later occurrences. Matches the bounding pattern used in PendingOrchestration_ShouldClearInFinallyBlock. - Reflect path: also assert SavePendingOrchestration before ExecuteWorkerAsync (not just Task.WhenAll) to guard against future eager materialization refactors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
1 participant
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.
What
Adds a structural test that verifies
SavePendingOrchestration()is called beforeExecuteWorkerAsync/Task.WhenAllin bothSendViaOrchestratorAsyncandSendViaOrchestratorReflectAsync.Why
Issue #517 describes a crash window where orchestration state could be lost if
SavePendingOrchestrationran after worker dispatch. Upon investigation, the code already has the correct ordering — both methods save before dispatching workers. The issue's "Current Flow" diagram was inaccurate.This PR adds a structural test to lock in the ordering and prevent future regressions, following the same pattern as the existing
PendingOrchestration_ShouldClearInFinallyBlocktest.Testing
SavePendingOrchestration_MustAppearBeforeWorkerDispatch— verifies source ordering in both pathsPendingOrchestration_ShouldClearInFinallyBlockcontinues to passCloses #517