Repository navigation
Conversation
A checkpoint response without a token means the current invocation must stop checkpointing. The background thread previously treated every such response as execution completion, which could report SUCCEEDED or FAILED while operation work was still in flight. It now reports PENDING. - Add ExecutionSuspendedByService and a token-revoked latch distinct from execution completion. - Exempt a batch carrying the execution's terminal update because that execution is already complete. - Reject newly queued operations and refreshes after token withdrawal, and wake accepted and queued waiters through the suspension path. - Return a bare PENDING response from every otherwise-terminal exit, including oversized-result paths and the CheckpointError exit, which now consults the latch for symmetry with the other exits. - Treat a refresh-only batch exactly like every other non-terminal batch when its response omits the token. - Document ExecutionSuspendedByService in the Raises section of create_checkpoint and schedule_refresh. - Drop ExecutionSuspendedByService's redundant constructor override, identical to SuspendExecution's. - Describe only the SDK-observed condition and resulting behavior in comments and docstrings. - Cover queue guards, deferred refreshes, completion versus withdrawal, oversized results, the CheckpointError exit, and every PENDING exit. Fixes #722
- Executor: skip PENDING-validator rejection when execution.paused - Add 9 new tests (6 unit + 3 e2e) covering pause/resume semantics - Fix 61 pre-existing executor_test.py mocks missing .paused=False (unconfigured Mock attrs are truthy, broke new paused-aware paths) - Update 3 mock-sequencing tests for new _invoke_execution store.load call
Use the existing SuspendExecution control flow for revoked checkpoint tokens and simplify the surrounding comments and tests. Clean up the local test runner pause and resume path, including deferred invocation naming and focused pause/resume coverage.
![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.
This comment has been minimized.
This comment has been minimized.
8859615 to
aa2d825
Compare
…n-a-checkpoint-response-has-no-checkpointtoken
yaythomas
left a comment
yaythomas
left a comment
There was a problem hiding this comment.
Thank you @hln33, this is a careful port of the JS fix, and the state_test.py coverage of the real checkpoint loop is excellent!
Thanks for the follow-up commit for the Codex findings. _invoke_execution no longer reads and saves outside the worker lane, a retried checkpoint no longer receives the withheld token, and the mid-step pause test now fails without the fix (I checked against main's core SDK).
Please could you rename the PR to the conventional commit format, e.g. fix(sdk): answer PENDING on revoked token (your first commit's subject)? The PR title becomes the squash commit message: https://github.com/aws/aws-durable-execution-sdk-python/blob/main/CONTRIBUTING.md#pull-request-title-and-commit-message-format
Maybe this behaviour also warrants a conformance requirement? If so, please could you note it in the description or open an issue in aws-durable-execution-conformance-tests?
This comment has been minimized.
This comment has been minimized.
|
@yaythomas Thanks for identifying these follow-ups. I opened separate issues for:
The conformance issue calls out the need to agree on a deterministic test mechanism before adding requirements or SDK handlers. |
hln33
changed the title
…ken-revoked # Conflicts: # packages/aws-durable-execution-sdk-python/src/aws_durable_execution_sdk_python/execution.py
This comment has been minimized.
This comment has been minimized.
…n-a-checkpoint-response-has-no-checkpointtoken
Codex AI reviewFour confirmed findings:
Static review completed; tests were not executed under the read-only constraints. Reviewed commit |
…n-a-checkpoint-response-has-no-checkpointtoken
yaythomas
left a comment
yaythomas
left a comment
There was a problem hiding this comment.
Thank you @hln33, this is excellent! Every earlier comment is addressed, and thank you for opening the follow-up issues.
What I checked on this revision:
- The new
test_checkpoint_without_token_answers_pendingfails against current main, where the invocation answers SUCCEEDED with"x". It passes here. - The merge with main: the check that turns SUCCEEDED or FAILED into PENDING after a revocation now runs in
invoke_worker. It runs after theExitStackhas closedexecution_stateand the checkpoint thread has finished, and beforeon_invocation_end. So plugins also see the final PENDING. Nice. - The coverage gates pass:
hatch run dev-core:covreports 98.68% (gate 98%), andhatch run dev-testing:covreports 96.34% (gate 95%).
TimeoutError for the pause wait makes sense to me, since wait_for_result and wait_for_callback raise it too.
For future work, in separate PRs: add pause and resume to the web server (WebRunner and its routes) and to the dex-local-runner CLI. SAM CLI runs this emulator as a container and calls its routes. So once the routes exist, SAM CLI could add pause and resume commands too, and SAM users could pause and resume executions from SAM's tooling.
no-action notes:
lint-commitsfails on five branch commit subjects over 50 characters. It isn't a required check, and the squash commit takes the PR title, which passes.- Codex's latest findings repeat ones you already answered.
Good suggestion. Created a new issue to track this here - #787 |
2 participants
Issue #, if available:
Fixes #722
Description of changes:
This PR updates the SDK to return
PENDINGwhen a checkpoint response omits the next checkpoint token for non-terminal work, instead of treating that response as execution completion.Changes include:
SuspendExecutionpath.PENDINGwhen the token has been revoked.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.