Repository navigation
Conversation
When a checkpoint response omits its token, the current invocation must stop checkpointing. CheckpointManager previously kept the spent token, so a later call failed and the invocation reported FAILED. It now ends PENDING. - Latch the missing token and prevent further checkpoint and paginated state calls. - Exempt a batch carrying the execution's terminal update because that execution is already complete. - Complete the accepted batch's waiters with SuspendExecutionException so user code cannot continue with results that cannot be recorded. - Consult the latch before every terminal DurableExecutor exit and when handling oversized results. - Describe only the SDK-observed condition and resulting behavior in comments, logs, and exception messages. - Cover completion versus withdrawal, no further API calls, plugins, oversized results, and step, wait, callback, map, and parallel paths. Ports aws/aws-durable-execution-sdk-js#931. Fixes #706
A checkpoint response without a checkpoint token means the invocation may checkpoint no further, so it reports PENDING. Tests had no way to produce that response against the in-memory checkpoint client. - LocalMemoryExecutionClient.pause() answers every later checkpoint without a token until resume(). Those checkpoints are still recorded, so the next invocation replays the work the SDK had not yet sent. - LocalDurableTestRunner exposes pauseExecution() and resumeExecution(), both marked @experimental. - runUntilComplete stops once a paused invocation reports PENDING. Without that check it advances operations and re-invokes, running handler work that no checkpoint could record. - CloudDurableTestRunner throws from both methods, because withholding a token is a behaviour of the in-memory client. Ports the testing half of aws/aws-durable-execution-sdk-js#931.
Drain already-submitted async operation and checkpoint work before returning terminal execution results. If that drain observes that the checkpoint token was revoked, report the invocation as PENDING instead of SUCCEEDED/FAILED/RETRYING. Add regression coverage for a handler returning while an unawaited checkpoint is still queued, and clarify paused local runner behavior.
35e7a0d to
e31d0cf
Compare
…n-a-checkpoint-response-has-no-checkpointtoken
Paused checkpoints can record both START and SUCCEED in one batch, so resume may reuse the step result. Remove the fixed execution count and keep the pause/resume status and result checks.
![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.
|
Thanks for simplifying this, Harry. The implementation is now very close to the alternative I drafted, especially the invocation-thread draining, reuse of shutdown, and single invocation-end notification. The lifecycle is much easier to follow now. I verified the two remaining issues raised in the inline comments:
The second issue is also present in my draft. I covered replayed values and errors but missed the plugin lifecycle notifications after resume. Thanks again for consolidating the implementation and expanding the regression coverage. |
Track tokenless checkpoint update IDs so resumed plugins receive operation outcomes and lifecycle callbacks, with result/error coverage. Codex-Automation: issue-implementation Codex-Pull-Request: #778
This comment has been minimized.
This comment has been minimized.
Add three regressions for root-handler draining, late steps after checkpoint token withdrawal, and invocation-end notification when exception serialization fails. The tests fail on a7562ea and pass on the main baseline 0c8618e. This commit contains tests only and leaves the implementation fixes to the PR author.
|
@hln33 I added 2c313b2 with three regression tests in
All three fail on PR head To reproduce: mvn -pl sdk test -Dtest=DurableExecutorLifecycleRegressionTestThis commit changes only tests and intentionally leaves them failing on this PR. Please fix the underlying behavior so all three pass before merging. |
| }) | ||
| .handle(HandlerResult::new) | ||
| .join(); | ||
| executionManager.drainOperations(); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_dkyum4fs4fue2gzpxtziesneje
[P1] Wait for the root handler before returning PENDING. When revocation completes executionExceptionFuture, handlerResult is available even while handlerFuture is still running. drainOperations() waits only operation workers, so the invocation can return and fire its final plugin event while root user code continues executing. This also makes revocationMustWaitForRootHandlerBeforeReturning fail. After safely rejecting late operations, await the root handler’s termination before choosing the invocation outcome.
| if (executionManager.isCheckpointTokenRevoked()) { | ||
| throw new SuspendExecutionException(); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_a3djkbqpkb4ludbasbcmgnonmi
[P1] Complete operations created after suspension. If the root handler starts an operation after signalSuspension() has already called stopAllOperations, this branch throws on the worker but the wrapper swallows SuspendExecutionException, leaving the newly registered operation’s completion future unresolved. A synchronous step()/child-context call then blocks indefinitely, as exercised by laterStepMustUnwindAfterTokenWithdrawal. Reject registration using the stored suspension cause or complete this operation exceptionally before throwing.
| var response = config.getDurableExecutionClient().checkpoint(durableExecutionArn, checkpointToken, request); | ||
| logger.debug("Durable checkpoint API called (latency={}ns): {}.", System.nanoTime() - startTime, response); | ||
|
|
||
| checkpointToken = response.checkpointToken(); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_amn6ijnyynsws36ou6adwlnm6p
[P2] Publish revocation before exposing the missing token. checkpointToken becomes null before handleMissingToken() sets checkpointTokenRevoked. A queued worker can start in this window, observe false, execute user side effects, and then fail to checkpoint, causing duplicate execution on replay. Read the response token into a local and publish nonterminal revocation before assigning null or otherwise releasing workers; add a latch-based test for this window.
| if (cause != null) { | ||
| logger.debug("Execution failed: {}", cause.getMessage()); | ||
| return new InvocationOutcome( | ||
| DurableExecutionOutput.failure(buildErrorObject(cause, config.getSerDes())), |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_zhc2qnxgh55wuhku6zqjhaxsy7
[P2] Preserve the invocation-end hook when failure serialization throws. finishInvocation() now calls buildErrorObject() before fireOnInvocationEnd(). If a custom SerDes rejects the handler exception, execution exits without the matching lifecycle hook; the added failureSerializationStillFiresInvocationEnd test consequently fails. Fire the FAILED hook with the original handler error before building the output, or guarantee it through a finally path.
Codex AI reviewFound four blocking lifecycle issues around checkpoint-token revocation and plugin completion. Static review only; tests were not executed per constraints. Reviewed commit |
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 #706.
Description
A checkpoint response can omit its checkpoint token. The SDK previously retained the old token, so a later checkpoint could fail and cause the invocation to report FAILED.
This change stops checkpointing and returns PENDING when a nonterminal checkpoint response omits the token. Updates accepted by the service remain available for replay. A response without a token still counts as completion when the request contains the execution’s terminal update.
The SDK also drains submitted operation and checkpoint work before choosing the invocation outcome. This catches token revocation discovered after the handler returns and ensures lifecycle plugins receive the final PENDING status without a result or error.
Experimental
pauseExecution()andresumeExecution()methods let local tests simulate token revocation and resume execution. The cloud test runner rejects these methods because simulation requires the in-memory client.Demo/Screenshots
Not applicable. This change affects execution behavior and testing utilities.
Checklist
Testing
Unit Tests
Added coverage for:
Validation reported 1,983 passing tests across the SDK, testing utilities, integration, and OpenTelemetry modules. Insight tests encountered sandbox socket restrictions. The installed-artifact compatibility matrix was not run.
Integration Tests
Added local integration coverage for accepted step results and errors surviving suspension. After resume, the stored outcome replays without repeating the step’s user code.
Examples
No new example. The change fixes execution lifecycle behavior and adds test controls.