Repository navigation
fix: Virtual child contexts exit replay early and omit resumed operation notifications - #909
nandanadileep wants to merge 2 commits into
Conversation
| if was_replaying and ( | ||
| (not next_exists and not is_virtual) or (next_is_step and not next_terminal) | ||
| ): |
There was a problem hiding this comment.
This fixes a virtual child that is the first operation in its context, but not one that comes after a replayed operation. After each operation, context.py:626, the look-ahead switches the context to NEW when context.py:522-524, _next_operation_exists() finds no checkpoint at the next operation's id. A virtual child never has a checkpoint at its own id. So when a replayed operation comes before it, the context is already NEW by the time run_in_child_context reaches this condition, and the child runs as NEW. Can _next_operation_exists() also check the first operation inside the next one, as the JS SDK does?
Two things I checked support this:
- With
DurableFunctionTestRunnerand a plugin, I ran a handler that calls a step and then a virtual child that waits. On the resumed invocation it behaves as onmain:is_replaying()returnsFalseinside the child, and the plugin gets noon_operation_endfor the wait. The second of two virtual children in a row fails in the same two ways. When the virtual child is the first operation, this branch makesis_replaying()returnTrueand delivers theon_operation_end. - The JS SDK fixed the same problem in aws-durable-execution-sdk-js #697: keep replay mode active after a virtual context. Before a context leaves replay mode, durable-context.ts:316-333,
checkAndUpdateReplayModealso checks the first child id,${pendingStepId}-1.
This is the change I tested on top of this branch:
def _next_operation_exists(self) -> bool:
"""True if the next operation or its first inner operation has a checkpoint."""
if self._peek_next_checkpoint().is_existent():
return True
# A virtual child never checkpoints itself, so its history starts at its
# first inner operation.
first_child_id: str = OperationIdNamespace(
self._peek_next_operation_id()
).create_id_for_step(1)
return self.state.get_checkpoint_result(
first_child_id, claim_end_hook=False
).is_existent()With it, the core unit and e2e tests pass, and both handlers above get the right is_replaying() and on_operation_end. The second read is an in-memory lookup. It runs only while replaying, when the next operation has no checkpoint at its own id. One case is still not covered: after a replayed operation, the context still switches to NEW before a virtual child that has no checkpoint at its first inner id, such as an empty one. JS has the same limit, so I think that can stay out of this PR.
Can the context.py:543-552, _replay_aware docstring also describe the virtual case? It still says that a next operation without a checkpoint switches the context to NEW, both before and after the operation.
There was a problem hiding this comment.
Addressed in 84b7b61. _next_operation_exists() now also checks the first inner id via OperationIdNamespace(...).create_id_for_step(1), matching the JS SDK. The _replay_aware docstring now describes that virtual case.
Kept the is_virtual before-op guard: it is still needed when the virtual child is the first operation in its context. _next_operation_exists() only runs after a previous operation, so it cannot cover that first-op path.
| return updates | ||
|
|
||
|
|
||
| def test_virtual_child_preserves_replay_and_emits_update_for_inner_wait(): |
There was a problem hiding this comment.
Every new test that runs a virtual child runs it as the first operation of its context. That is the one position this branch fixes, so the tests pass even though a virtual child after a replayed operation still runs as NEW (see the comment on context.py). Can the tests also cover a replayed step before the virtual child, and two virtual children in a row?
The tests that check the notification replace state.emit_operation_update_hook with a recorder. They show that the state method is called, but not what a plugin receives. Can they use a real plugin instead, like the capturing plugin in context_test.py:3482, test_replay_aware_emits_update_hook_for_operation_updated_since_last_invocation, and assert the on_operation_end that the plugin gets for the inner wait?
A test with DurableFunctionTestRunner, like plugin_operation_type_int_test.py:145-205, a virtual child resumed by the runner, would also show the fix across two real invocations. That test is optional, since unit tests with a real plugin cover this fix.
There was a problem hiding this comment.
Addressed in 84b7b61. Added coverage for a replayed step before the virtual child, and for two virtual children in a row. The virtual-child notification tests now assert a capturing plugin's on_operation_end instead of patching state.emit_operation_update_hook. Skipped the optional DurableFunctionTestRunner e2e; unit tests with a real plugin cover the fix.
| assert result == "ok" | ||
|
|
||
|
|
||
| def test_replay_aware_brand_new_non_virtual_op_flips_to_new(): |
There was a problem hiding this comment.
This test runs the same input as context_test.py:2857, test_replay_aware_flips_before_brand_new_operation, because is_virtual=False is the default, and it checks part of what that test checks. Can it be removed? This one is optional.
There was a problem hiding this comment.
Removed test_replay_aware_brand_new_non_virtual_op_flips_to_new. test_replay_aware_flips_before_brand_new_operation already covers that path.
_next_operation_exists now treats a virtual child's first inner checkpoint as existence, matching the JS SDK. Tests cover a replayed step before a virtual child and two virtual children in a row, asserting plugin on_operation_end instead of patching emit_operation_update_hook. Co-authored-by: Nandana Dileep <nandanadileep@users.noreply.github.com>
|
Addressed the CHANGES_REQUESTED review in 84b7b61:
|
Fixes #889.
Focused Python fix for: [Bug]: Virtual child contexts exit replay early and omit resumed operation notifications
Changed implementation: packages/aws-durable-execution-sdk-python/src/aws_durable_execution_sdk_python/context.py.
Regression tests: packages/aws-durable-execution-sdk-python/tests/context_test.py.
Verification (working directory
packages/aws-durable-execution-sdk-python):.venv/bin/python -m pytest -q -x -p no:cacheprovider -- /workspace/packages/aws-durable-execution-sdk-python/tests/context_test.py7bed3879b6c1eebf0012984cf757825d02ea1f90.AI-assisted contribution: OpenCode using Autoloops wrote this patch. The automated harness independently ran the tests listed above. No human review is claimed.