Repository navigation
Conversation
get() on a parallel branch recorded as SKIPPED now throws ParallelBranchSkippedException instead of suspending with no pending operations. This covers branches that never started and in-flight branches skipped at completion, on the first invocation and on replay.
Move the existing waitForOperationCompletion javadoc back onto that method and document waitForCompletionFuture separately.
zhongkechen
left a comment
zhongkechen
left a comment
There was a problem hiding this comment.
Found three issues in this revision. Validation: 97 existing focused tests passed. Targeted integration repros confirmed all three findings; the anyOf replay and changing-result repros also passed against the parent commit.
| return enqueueItem(name, func, resultType, serDes, OperationSubType.PARALLEL_BRANCH, skip); | ||
| var branch = enqueueItem(name, func, resultType, serDes, OperationSubType.PARALLEL_BRANCH, skip); | ||
| if (skip) { | ||
| branch.completeAsSkipped(); |
There was a problem hiding this comment.
[P1] Preserve anyOf() results across replay
Immediately completing skipped futures changes which branch wins DurableFuture.anyOf(). With firstSuccessful() and maxConcurrency(1), register the winning branch first, then call anyOf(skipped, winner) while it is running. This returns the winner initially, but after a later suspension it throws ParallelBranchSkippedException because replay immediately settles the skipped future. Previously successful handler code now fails on replay.
A deterministic integration repro observes [winner, winner] on the parent commit and [winner, SKIPPED:skipped] on this PR. Preserve the observable winner when skipped futures participate in anyOf().
| statuses); | ||
| var serializedResult = serializeAndDeserializeResult(cachedResult); | ||
| cachedResult = serializedResult.deserialized(); | ||
| completeSkippedBranches(items, statuses); |
There was a problem hiding this comment.
[P2] Make the completion snapshot and skip transition atomic
A branch can finish after the status snapshot records it as SKIPPED but before completeSkippedBranches() runs, particularly during result serialization. Its get() can then return a successful value before completeAsSkipped() changes that same future to throw.
A controlled integration repro gates ParallelResult serialization while branch B finishes: the first b.get() returns "B", but after parallel.get(), the second throws ParallelBranchSkippedException. The parent commit returns "B" both times. Coordinate the snapshot and skip transition with branch completion so an already-observed outcome cannot change.
| childContextOperation.get(); | ||
| return ParallelResult.Status.SUCCEEDED; | ||
| } catch (ParallelBranchSkippedException e) { | ||
| return ParallelResult.Status.SKIPPED; |
There was a problem hiding this comment.
[P2] Keep propagated nested exceptions classified as failures
An outer branch can fail by calling get() on a skipped inner branch. Its checkpoint correctly records FAILED, but this catch classifies the outer branch as SKIPPED, reports failed() == 0, and replaces the inner exception with one naming the outer branch.
A nested-parallel integration repro confirms that the outer checkpoint is FAILED while the returned statuses are [SKIPPED] and outerBranch.get() names outer-branch instead of inner-skipped. Use the branch's explicit isSkipped() state to identify skips; an exception propagated from its body remains a failure.
This branch was successfully deployed
2 participants
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Issue Link, if available
Fixes #752
Description
get()on a parallel branch recorded asSKIPPEDno longer waits until the SDK suspends with no pending operations. It throwsParallelBranchSkippedException, which names the branch, on the first invocation and on replay.This covers both cases from the issue:
firstSuccessful()(or another early-completion config) finished the parallel first.SKIPPED.get()now throws on that first invocation as well, so the first run and replay agree.docs/core/parallel.mddocumentsskipped(),statuses(), and the new exception.ExecutionManageris unchanged. The skip settlement lives inParallelOperationplus a smallChildContextOperation/BaseDurableOperationwait path soget()can complete without a checkpointedOperation.DurableContextis unchanged.Demo/Screenshots
N/A
Checklist
Testing
Unit Tests
Have unit tests been written for these changes?
Yes.
ParallelBranchSkippedExceptionTestcovers the named-branch message.ParallelOperationTest.minSuccessful_notExecuteSkippedBranchWhenReplaynow asserts thatget()on the skipped branch throwsParallelBranchSkippedException.Integration Tests
Have integration tests been written for these changes?
Yes. Local runner tests in
ParallelIntegrationTestusefirstSuccessful()with two branches and callget()on the skipped one, with a bounded timeout:maxConcurrency(1)), first run and replaytestInvalidSuspensionWhenWaitingOnSkippedBranchnow expects the named skipped exception instead of invalid suspension.Examples
Has a new example been added for the change? (if applicable)
No. Existing parallel examples still pass; the behavior is documented in
docs/core/parallel.md.This change was prepared with AI assistance and reviewed by the author.