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.
GitHub Actions
Inactive
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.
anyOf ignores a branch that settled only because it was skipped, so replay returns the same winner. The parallel snapshot marks an incomplete branch skipped under the same lock that publishes its result, and a nested skip exception stays a failure of the branch that propagated it.
|
The Oct 21 merge only brings in main, so the three comments were still open. They're fixed in aed55d3. anyOf no longer lets a skipped branch win while another future can still settle, so replay returns the same winner. The parallel snapshot and the skip happen under the same lock as branch completion, so get() can't return a value and then throw. A nested skip exception stays a failure of the outer branch; only an explicit skip is SKIPPED, and get() still names the inner branch. |
firstSuccessful() can checkpoint a parallel operation before a later branch is registered. get() on that branch then suspends with no pending operation. Decide the skip and publish the completion snapshot under one lock, and settle any branch still missing from that result.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
Defer publishing parallel branch skips until the parent parallel checkpoint succeeds, return exhausted anyOf candidates when only skipped futures remain, and fire onOperationEnd for virtual branches that finish after a skip reservation.
|
/ai review |
| if (skip) { | ||
| branch.completeAsSkipped(); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_kc5klhh4uqegosyk6mmj4px7do
[P1] Delay late-registered skips until the parent checkpoint succeeds
shouldSkipBranch() now reads cachedResult, which is populated before serialization and submission of the parent's SUCCEED checkpoint. A branch registered in that window completes immediately here. Its caller can catch ParallelBranchSkippedException and checkpoint subsequent work before the skip decision is durable. An interruption then allows replay to execute that branch, changing control flow against those subsequent checkpoints. Reserve late-registered skips without exposing them, and publish them only after acknowledged parent completion; immediate publication is safe when replaying an already-persisted parent.
To suppress repeats, reply /ai verdict deferred, /ai verdict false-positive, or /ai verdict out-of-scope on the first nonblank line. Add rationale on later lines. A new /ai verdict accepted allows future repeats again.
| super.onCheckpointComplete(operation); | ||
| if (operation.status() == OperationStatus.SUCCEEDED) { | ||
| publishReservedBranchSkips(); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_o2vvyetv3entpiyvzoe7nm5rrp
[P1] Remove the completion/settlement lock inversion
ExecutionManager.processCheckpointUpdate() invokes this callback while holding the completion monitor shared by the parent and its branches. This call then acquires each branch's settlementLock. Concurrent late registration takes those locks in reverse order: completeAsSkipped() holds settlementLock and calls completeOperation(), which needs the shared completion monitor. If registration appends the branch before this iteration begins, the handler and checkpoint worker can deadlock. Use a consistent lock order or publish skips outside the manager's completion monitor, with a deterministic concurrency regression test.
To suppress repeats, reply /ai verdict deferred, /ai verdict false-positive, or /ai verdict out-of-scope on the first nonblank line. Add rationale on later lines. A new /ai verdict accepted allows future repeats again.
| for (var branch : getBranches()) { | ||
| branch.publishReservedSkip(); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_r4xyuv5a6hwgmerfx6itegdhys
[P1] Snapshot branch membership before publishing skips
getBranches() returns a Collections.synchronizedList, whose iterator is not protected by its synchronized mutators. Early completion permits the context thread to keep registering branches while this checkpoint callback runs. An append between iterator creation and a subsequent next() can throw ConcurrentModificationException, interrupting checkpoint response processing and leaving skip publication incomplete. Iterate a stable snapshot and coordinate concurrent registrations through the same membership/publication protocol.
To suppress repeats, reply /ai verdict deferred, /ai verdict false-positive, or /ai verdict out-of-scope on the first nonblank line. Add rationale on later lines. A new /ai verdict accepted allows future repeats again.
| parallel.branch("branch-a", String.class, ctx -> "A"); | ||
| var skipped = parallel.branch("branch-b", String.class, ctx -> "B"); | ||
| var result = parallel.get(); | ||
| context.step("record", String.class, stepCtx -> observe(skipped) + "|" + result.statuses()); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_ju3x4rei2iqoutsmdl2i2q7egz
[P2] Make the skipped-branch fixture deterministic
This parallel uses unlimited concurrency, so branch-b can succeed or win instead of being skipped. In that valid scheduling outcome, observe(skipped) calls a normal branch's get() from a STEP thread, and validateCurrentThreadCanWaitForDurableOperation() throws IllegalStateException. The step retries/fails and the test intermittently rejects correct execution. Establish a deterministic skipped branch and observe/assert its outcome on the context thread, outside the checkpointed step body, so replay also evaluates the assertion.
To suppress repeats, reply /ai verdict deferred, /ai verdict false-positive, or /ai verdict out-of-scope on the first nonblank line. Add rationale on later lines. A new /ai verdict accepted allows future repeats again.
| * every branch has started, or while a branch is still running. The SDK does not start or resume that branch later, so | ||
| * {@code get()} throws this exception on the first invocation and on every replay. | ||
| */ | ||
| public class ParallelBranchSkippedException extends DurableExecutionException { |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_llegoqsd74ysgxzfnasy4wqgms
[P2] Return to design with the required API and regression evidence
Rules 2, “Simplify before you add,” and 3, “Write a design note before the code,” require a design note and simpler alternatives for this new public exception and the changed anyOf contract. The description supplies neither. It also lists post-fix tests without the pre-fix failure evidence required by rule 5, “A bug fix starts with a failing test”; no test-first commit sequence is supplied. Return to the design phase under rule 8, step 6, and provide the problem, cause, alternatives, chosen rationale, removals, and regression evidence. Record the anyOf selection policy explicitly in that design.
To suppress repeats, reply /ai verdict deferred, /ai verdict false-positive, or /ai verdict out-of-scope on the first nonblank line. Add rationale on later lines. A new /ai verdict accepted allows future repeats again.
| if (isVirtual) { | ||
| fireOnOperationEnd(null, exception, false); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_x4qkcg5xveftbgdpxl75zckk4f
[P2] Return to design for skipped-branch lifecycle coverage
Rule 4, “Every branch has a test that fails without it,” is not satisfied for these skipped-branch plugin paths. The new virtual-child unit test completes successfully without a plugin, and the new integration cases use default NESTED branches. Removing this terminal-hook call or its success counterpart changes no assertion, leaving the lifecycle regression unprotected. Return to the design phase under rule 8, step 6, and establish regression coverage that requires the terminal hook for both successful and failing skipped FLAT branches.
To suppress repeats, reply /ai verdict deferred, /ai verdict false-positive, or /ai verdict out-of-scope on the first nonblank line. Add rationale on later lines. A new /ai verdict accepted allows future repeats again.
Codex AI reviewChanges requested: three checkpoint/concurrency defects, one flaky test, and two approach violations.
All 14 changed files were inspected. Tests were not executed under the review constraints. Reviewed commit |
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.