Repository navigation
fix(testing): derive updated operation IDs - #898
Conversation
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 |
There was a problem hiding this comment.
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.
Codex AI reviewOne Rule 6 approach finding at Reviewed commit |
Issue #, if available
Fixes #860. Fixes #861.
Description of changes
The local runner kept
UpdatedOperationIdsin 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:This change derives the list from the touch sequences the runner already keeps for checkpoint responses:
handler_seen_seqtoseq_counter.PENDING,SUCCEEDEDorFAILED) saveshandler_seen_seqaslast_successful_invocation_seq. A failed invocation leaves it.Execution.updated_operation_idsis now a read-only property: every operation until an invocation succeeds, then every operation touched afterlast_successful_invocation_seq.Removed: the stored list and its JSON key,
_record_updated_operationand its three callers,has_changes_after, and theinvocation_seqvalue 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
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.execution.pyandexecutor.py. Core: docstrings only. No public API members added or removed.Assumptions
Decisions not covered by the issue or the design note
updated_operation_idsis a derived property, so the invokers are unchanged and no completion path can be missed.PENDINGwith no pending operation stays valid when an operation changed after the handler last saw the state, as before. The check now readshandler_seen_seqinstead ofinvocation_seq.complete_callback_successdecodes the result before it touches the operation, so an undecodable result leaves no change behind.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..valuestrings, and @zhongkechen is credited as co-author. Its unit coverage moved intotests/execution_test.py.Checklist
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) andtests/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_seqorhas_changes_after:tests/execution_test.py:test_complete_wait_records_updated_operation_idandtest_record_invocation_completion_keeps_updated_operation_idsasserted the stored list. Tests of the derived rule replace them.tests/executor_test.py:test_a_completion_during_the_invocation_reaches_the_next_inputasserted the stored list. It now asserts the derived list after a successful invocation.test_begin_invocation_baseline_predates_a_completion_queued_behind_itasserted the returnedinvocation_seq. Renamed totest_begin_invocation_watermark_predates_a_completion_queued_behind_it, it assertshandler_seen_seq.test_pending_response_is_valid_when_an_operation_completed_after_the_handler_saw_itasserted ahas_changes_aftercall. It now mockshas_unseen_changesand asserts that the valid response callsrecord_successful_invocation.test_pending_response_with_only_input_time_state_is_still_an_errorandtest_pending_response_is_an_error_when_the_handler_saw_the_completionpassedinvocation_seq. They now set the watermark withmark_state_delivered().test_should_retry_when_pending_response_has_no_operationsmockedhas_changes_after. It now mockshas_unseen_changes.tests/executor_pause_resume_test.py:test_paused_pending_is_accepted_only_when_a_token_was_withheldpassedinvocation_seq. It drops the argument.tests/invoker_test.py:test_in_process_invoker_create_invocation_inputset 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 --checkin every package,hatch run types:check,hatch run test:cov(6600 passed, 6 skipped),hatch buildand both OTel wheel tests. JS SDK examples (hatch run dev-testing:js-examples, JS SDKmain, 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.