Skip to content

fix: Virtual child contexts exit replay early and omit resumed operation notifications - #909

Open
nandanadileep wants to merge 2 commits into
aws:mainfrom
nandanadileep:oss-harness/issue-889
Open

nandanadileep wants to merge 2 commits into
aws:mainfrom
nandanadileep:oss-harness/issue-889

Conversation

@nandanadileep

Copy link
Copy Markdown
Contributor

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.py

  • Patched code: 135 tests, zero failures/errors.
  • Original code with the same regression tests: 1 failures, zero collection/setup errors.
  • Base commit: 7bed3879b6c1eebf0012984cf757825d02ea1f90.

AI-assisted contribution: OpenCode using Autoloops wrote this patch. The automated harness independently ran the tests listed above. No human review is claimed.

@nandanadileep
nandanadileep deployed to ai-pr-review-runtime November 5, 2026 03:05 — with GitHub Actions Active
Comment on lines +617 to +619
if was_replaying and (
(not next_exists and not is_virtual) or (next_is_step and not next_terminal)
):

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.

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:

  1. With DurableFunctionTestRunner and a plugin, I ran a handler that calls a step and then a virtual child that waits. On the resumed invocation it behaves as on main: is_replaying() returns False inside the child, and the plugin gets no on_operation_end for 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 makes is_replaying() return True and delivers the on_operation_end.
  2. 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, checkAndUpdateReplayMode also 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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():

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.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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():

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.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed test_replay_aware_brand_new_non_virtual_op_flips_to_new. test_replay_aware_flips_before_brand_new_operation already covers that path.

@yaythomas yaythomas added changes-requested Reviewer requested changes; waiting on the author and removed needs-review labels Nov 6, 2026
_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>
@nandanadileep

Copy link
Copy Markdown
Contributor Author

Addressed the CHANGES_REQUESTED review in 84b7b61:

  • _next_operation_exists() now treats a virtual child's first inner checkpoint as existence (JS SDK feat(ci): enable AI review commands and activity events #697).
  • _replay_aware docstring updated for the virtual case. Kept the first-op is_virtual guard; look-ahead only runs after a previous operation.
  • Tests cover a replayed step before a virtual child and two virtual children in a row, asserting plugin on_operation_end rather than patching emit_operation_update_hook.
  • Removed duplicate test_replay_aware_brand_new_non_virtual_op_flips_to_new.

pytest on context_test.py: 137 passed.

This branch was successfully deployed

1 active (outdated) deployment
ai-pr-review-runtime — 7947da91 Deployed Nov 5, 2026 by nandanadileep via notify / Generate notification summary #381
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes-requested Reviewer requested changes; waiting on the author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Virtual child contexts exit replay early and omit resumed operation notifications

3 participants