Skip to content

fix(testing): derive updated operation IDs - #898

Merged
zhongkechen merged 1 commit into
mainfrom
fix/testing-updated-operation-ids
Nov 2, 2026
Merged

zhongkechen merged 1 commit into
mainfrom
fix/testing-updated-operation-ids

Conversation

@yaythomas

Copy link
Copy Markdown
Contributor

Issue #, if available

Fixes #860. Fixes #861.

Description of changes

The local runner kept UpdatedOperationIds in a stored list. The wait, step retry and chained invoke completions appended to it, and building the invocation input cleared it. That gave four differences from the service:

  1. Callback completions never appended. So a callback that completed while the execution was suspended looked like a replay ([Testing]: Record callback terminal completions as updated operations #860).
  2. A failed invocation lost the list. The service lists every change since the last successful invocation again, as the Lambda measurements in [Bug]: OTel re-exports already completed STEP spans after invocation retry #892 show ([Testing]: Acknowledge invocation input without losing pending updates #861).
  3. The first invocation sent an empty list. The service lists every operation, starting with the EXECUTION operation.
  4. Building the input did not record that the handler had seen the state. So the first checkpoint response of an invocation sent the input's operations again.

This change derives the list from the touch sequences the runner already keeps for checkpoint responses:

  • Building the input sets handler_seen_seq to seq_counter.
  • A valid response (PENDING, SUCCEEDED or FAILED) saves handler_seen_seq as last_successful_invocation_seq. A failed invocation leaves it.
  • Execution.updated_operation_ids is now a read-only property: every operation until an invocation succeeds, then every operation touched after last_successful_invocation_seq.

Removed: the stored list and its JSON key, _record_updated_operation and its three callers, has_changes_after, and the invocation_seq value that worked around difference 4. The core SDK changes only docstrings: they described the list as the operations changed while suspended, and one said it is empty on the first invocation.

Design

  • Design note (rule 3): not needed. No public API changes; the approach is above.
  • Simpler options considered (rule 2): this change removes logic. fix: record callback terminal operation updates #868 adds the three callback completions to the stored list. fix: acknowledge delivered invocation input #867 prunes an entry once a checkpoint response or a valid invocation delivers it, and its pruning test, operation_last_touched_seq > seq, is the rule this change applies to every operation. With both, the list still differs from the service after a failed invocation. It holds only completions from outside the handler, so the retry cannot list the failed invocation's own steps. And a checkpoint response prunes entries before the invocation succeeds, so a failed invocation loses them. A list that recorded every change and pruned only at a successful invocation would be a second copy of the touch sequences, which this change reads directly.
  • Root cause (rule 5): the runner kept a second record of "what changed since the handler last saw the state" beside the sequences it already keeps, and updated that record at the wrong points.
  • Net change: runner source +71/−53 lines, mostly docstrings, in execution.py and executor.py. Core: docstrings only. No public API members added or removed.

Assumptions

  • After a failed invocation, the retry lists every change since the last successful invocation, including the failed invocation's own steps: measured on Lambda in [Bug]: OTel re-exports already completed STEP spans after invocation retry #892.
  • After a successful invocation, the next input lists only the operations that changed after it: measured on Lambda in [Bug]: OTel re-exports already completed STEP spans after invocation retry #892 (wait resume).
  • The first invocation lists the EXECUTION operation: checked against the service's implementation; not measured on Lambda in this PR.
  • The first checkpoint response of an invocation does not return the operations its input carried: checked against the service's implementation; not measured on Lambda in this PR.
  • A callback that completed while suspended is listed on resume: checked against the service's implementation; not measured on Lambda in this PR.

Decisions not covered by the issue or the design note

  • updated_operation_ids is a derived property, so the invokers are unchanged and no completion path can be missed.
  • A stored execution written by an older runner has no saved watermark, so its next input lists every operation once.
  • PENDING with no pending operation stays valid when an operation changed after the handler last saw the state, as before. The check now reads handler_seen_seq instead of invocation_seq.
  • A deferred re-invoke no longer fires when the only unseen changes were already in the input.
  • complete_callback_success decodes the result before it touches the operation, so an undecodable result leaves no change behind.
  • Plugins now see the EXECUTION operation in the first invocation's updated_operations, and see a failed invocation's changes again on the retry. So the OTel plugin now reproduces the duplicate STEP span from [Bug]: OTel re-exports already completed STEP spans after invocation retry #892 locally, as on Lambda. [Bug]: OTel re-exports already completed STEP spans after invocation retry #892 stays open for the SDK fix.
  • The e2e test from fix: record callback terminal operation updates #868 is kept, with enum identity checks instead of .value strings, and @zhongkechen is credited as co-author. Its unit coverage moved into tests/execution_test.py.

Checklist

  • Every new branch has a test that fails without it (rule 4)
  • For a bug fix: the new tests fail on the current code (rule 5). The branch is one commit, so the evidence is below.
  • Every edited test expectation is listed below, with the reason (rule 5)

Before and after. With main's runner source, all 16 new tests fail: tests/e2e/updated_operation_ids_test.py (9), tests/e2e/callback_updated_operations_test.py (6) and tests/executor_checkpoint_test.py::test_first_checkpoint_response_omits_operations_the_input_carried. With this change they pass.

Edited tests, each because it used the stored list, invocation_seq or has_changes_after:

  • tests/execution_test.py: test_complete_wait_records_updated_operation_id and test_record_invocation_completion_keeps_updated_operation_ids asserted the stored list. Tests of the derived rule replace them.
  • tests/executor_test.py:
    • test_a_completion_during_the_invocation_reaches_the_next_input asserted the stored list. It now asserts the derived list after a successful invocation.
    • test_begin_invocation_baseline_predates_a_completion_queued_behind_it asserted the returned invocation_seq. Renamed to test_begin_invocation_watermark_predates_a_completion_queued_behind_it, it asserts handler_seen_seq.
    • test_pending_response_is_valid_when_an_operation_completed_after_the_handler_saw_it asserted a has_changes_after call. It now mocks has_unseen_changes and asserts that the valid response calls record_successful_invocation.
    • test_pending_response_with_only_input_time_state_is_still_an_error and test_pending_response_is_an_error_when_the_handler_saw_the_completion passed invocation_seq. They now set the watermark with mark_state_delivered().
    • test_should_retry_when_pending_response_has_no_operations mocked has_changes_after. It now mocks has_unseen_changes.
  • tests/executor_pause_resume_test.py: test_paused_pending_is_accepted_only_when_a_token_was_withheld passed invocation_seq. It drops the argument.
  • tests/invoker_test.py: test_in_process_invoker_create_invocation_input set the stored list. It now expects the first input to list the EXECUTION operation.

Validation, on Python 3.14 with Hatch 1.17.1: hatch fmt --check in every package, hatch run types:check, hatch run test:cov (6600 passed, 6 skipped), hatch build and both OTel wheel tests. JS SDK examples (hatch run dev-testing:js-examples, JS SDK main, Node 22): 114 test files, 111 passed, 0 failed, 3 have no cloud tests, the same counts as the workflow's latest run.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

The local runner kept UpdatedOperationIds in a list that every
completion path had to append to, and cleared it when it built the
invocation input. Callback completions never appended, so a callback
that completed while suspended looked like a replay. A failed
invocation lost the list, while the service lists every change since
the last successful invocation again. The runner also did not record
that the input delivered the state, so the first checkpoint response
of an invocation sent those operations again.

Derive the list from the touch sequences the runner already keeps.
Building the input sets handler_seen_seq. A valid response saves it as
last_successful_invocation_seq. The next input lists every operation
touched after that point, or every operation before the first success.
The stored list, its producer calls and the invocation_seq baseline
are removed.

The retry behavior matches the Lambda measurements in #892.

Fixes #860
Fixes #861

Co-authored-by: Zhongke Chen <65260095+zhongkechen@users.noreply.github.com>
carries the state as of ``seq_counter``. So the next checkpoint
response returns only the operations touched after it.
"""
self.handler_seen_seq = self.seq_counter

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex AI review · Finding arf_v1_6huvwhctwuu2th554g3cfkucyi

[P2] Establish evidence for the service-parity assumptions

Rule 6, “Mark unverified assumptions,” requires cited evidence or an explicit unverified label. The description says input-state omission from the first checkpoint response, first-invocation EXECUTION inclusion, and callback inclusion were checked against the service implementation, but supplies no identifiable source reference or validation artifact for those checks. The new local tests encode these same assumptions, so they cannot establish service parity. Per Rule 9, return to the design phase (Rule 8, step 6): cite reviewable evidence for these behaviors, or mark them unverified and obtain a maintainer decision on the necessary validation.

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.

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Codex AI review

One Rule 6 approach finding at packages/aws-durable-execution-sdk-python-testing/src/aws_durable_execution_sdk_python_testing/execution.py:494. No additional correctness defects confirmed. Tests were inspected but not executed.

Reviewed commit 9d53081e9b30f0ba4fbdc85b4c39217be1451ad7. Workflow run

@zhongkechen zhongkechen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zhongkechen
zhongkechen merged commit 1d352b9 into main Nov 2, 2026
48 checks passed
@zhongkechen
zhongkechen deleted the fix/testing-updated-operation-ids branch November 2, 2026 18:13
@SilanHe SilanHe mentioned this pull request Nov 9, 2026
3 tasks done

This branch was successfully deployed

1 active deployment
ai-pr-review-runtime — 9d53081e Deployed Nov 2, 2026 by yaythomas via ai-pr-review / Codex review / Generate Codex review #2708
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Testing]: Acknowledge invocation input without losing pending updates [Testing]: Record callback terminal completions as updated operations

2 participants