Skip to content

feat(plugin): operation maps on invocation hooks - #629

Open
wangyb-A wants to merge 5 commits into
mainfrom
feat/plugin-invocation-operation-maps
Open

feat(plugin): operation maps on invocation hooks#629
wangyb-A wants to merge 5 commits into
mainfrom
feat/plugin-invocation-operation-maps

Conversation

@wangyb-A

@wangyb-A wangyb-A commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Closes #617.

What

The invocation-level plugin info objects carried no view of the execution's
operation state, so Python plugins could not see what the JS SDK exposes on
every invocation hook.

  • InvocationInfo.operations — the checkpointed operation map converted to
    OperationInfo, on invocation-start and invocation-end. Mirrors JS, where
    it sits on InvocationBaseInfo.
  • InvocationStartInfo.updated_operations — the subset named by the durable
    invocation input's UpdatedOperationIds, i.e. operations completed externally
    while the execution was suspended. Start hook only, matching JS's
    InvocationInfo.updatedOperations.

Both are kw-only with empty-map defaults, and repr=False, compare=False, hash=False, so existing plugins, hook constructors and positional callers are
unaffected.

Notable decisions

  • The end hook re-reads the operation map rather than reusing the
    start-of-invocation snapshot. This matches JS, which overrides
    operations: toOperationInfoMap(...) at every onInvocationEnd site, and it
    is strictly more informative — the suspending invocation reports 1 operation
    at start and 2 at end.
  • Snapshot eagerly, convert lazily. Plugin invocation hook infos missing operations and updated-operations maps #617 suggested lazy construction for
    operation-heavy executions. Fully lazy was wrong: a plugin that stashes the
    info and reads it later would observe a later state than its own hook. So
    the raw map is snapshotted when the hook fires (a cheap dict copy) and only
    the per-operation OperationInfo conversion is deferred and cached. A unit
    test pins this.
  • The lazy view is copy-safe. It holds no lock — building the map is pure
    and idempotent, so a race can only duplicate work — and implements
    __deepcopy__ to materialize a plain dict. Without that, the lock made
    dataclasses.asdict(info) and copy.deepcopy(info) raise TypeError, and
    because plugin exceptions are swallowed a plugin that serialized an info would
    have its hook silently dropped. (This was the Codex finding on feat(plugin): operation maps on invocation hooks #623.)
  • The maps are additive, like the payload fields in fix(plugin): make payload fields additive on hook infos #627: mapping-valued
    fields in the generated __hash__ would make a previously hashable
    InvocationStartInfo raise TypeError, and their entries carry operation
    results and errors that instrumentation logs wholesale through repr. fix(plugin): make payload fields additive on hook infos #627's
    test caught this the moment these commits were rebased onto main.
  • is_replayed is left False on map entries, matching JS's
    toOperationInfoMap (isReplay: false) — these describe stored state, not
    replay events.

Testing

  • 1538 unit tests passing; hatch fmt --check clean.
  • Live plugin conformance suite (us-west-2), with the conformance handlers
    applied: 23/23, exit 010-19 flips PASSED with no change to the
    requirement
    , and 10-1..10-18 / 10-20..10-23 are unaffected. That was
    measured before this rebase; the handlers themselves are in the separate
    conformance PR (Add hook-info field-shape conformance handlers #615, also closed by accident) and are still needed to
    re-verify end to end.

Known pre-existing flake (not from this change)

One earlier suite run showed 10-22 (PluginOperationChangeShape) red; an
immediate re-run on byte-identical code was 23/23 clean. The only edit on that
path here is a pure refactor (an inline dict comprehension replaced by
_to_operation_info_map, the same conversion), and 60/60 local in-process
repeats of the 10-22 handler satisfy its primary matcher. JS's handler for that
requirement is structurally identical, so it is likely cross-SDK.

Alex Wang added 3 commits August 11, 2026 20:55
Invocation-level plugin infos carried no view of the execution's
operation state, so Python plugins could not see what the JS SDK
exposes on every invocation hook.

Add to the invocation infos:
- InvocationInfo.operations, the checkpointed operation map converted
  to OperationInfo, on both invocation-start and invocation-end
- InvocationStartInfo.updated_operations, the subset named by the
  invocation input's UpdatedOperationIds, i.e. operations completed
  externally while the execution was suspended

Both are kw-only with empty-map defaults, so existing constructor
calls and plugins are unaffected. The end hook re-reads the map so it
reports end-of-invocation state rather than the start snapshot.

The map is snapshotted when the hook fires but converted to
OperationInfo only on first access, so an operation-heavy execution
does not pay per invocation for a view no plugin reads, while a
plugin that stashes the info still sees the state as of its hook.

Refs #617
Addresses the Codex review comment on #623.

The thread lock in the lazy operation map made the enclosing frozen
info uncopyable: dataclasses.asdict() deep-copies any field that is not
a dict, list, tuple or dataclass, and a lock cannot be copied. Both
asdict() and deepcopy() raised TypeError, and since plugin exceptions
are swallowed a plugin that serialized an info would have its hook
silently dropped.

Drop the lock: building the map is pure and idempotent, so a race can
only duplicate work, never corrupt the result, and the assignment is
atomic. Add __deepcopy__ so the view materializes into an ordinary dict
when copied, which also keeps the provider closure out of the copy.

Adds regression tests using asdict() and deepcopy() over both hook
infos and both maps, including the not-yet-read case.

Refs #617
The operation maps had the same defect #627 fixed for the payload
fields, and #627's test caught it as soon as these commits were rebased
onto main: a mapping-valued field in the generated __hash__ makes a
previously hashable InvocationStartInfo raise TypeError, and the map
entries carry operation results and errors that instrumentation logs
wholesale via repr.

Set repr=False, compare=False, hash=False on operations and
updated_operations, and extend the field-declaration tests to cover
them alongside hashability, equality and repr assertions.

Refs #617
@wangyb-A
wangyb-A temporarily deployed to ai-pr-review-runtime August 11, 2026 20:58 — with GitHub Actions Inactive
@wangyb-A
wangyb-A had a problem deploying to ai-pr-review-runtime August 11, 2026 20:58 — with GitHub Actions Failure
Comment thread packages/aws-durable-execution-sdk-python/tests/plugin_test.py
@github-actions

This comment has been minimized.

Addresses the review comments on #629.

Error isolation: OperationInfo.from_operation reused the checkpoint's
ErrorObject, whose stack_trace is a mutable list handed to user code on
replay. A plugin reading info.operations could append to or clear that
list and change the error the execution later raises. Reproduced, then
fixed by cloning the error and its stack_trace at the conversion, so
every plugin-facing OperationInfo is isolated, not just the ones in the
new maps. Only the list needs cloning; the other fields are strings.

Provider gating: durable_execution() passes an operations provider
unconditionally, so a plugin-free execution invoked it at both hooks.
Snapshotting and provider retention are now gated on self._plugins.

The reviewer also asked to drop the snapshot copy since
ExecutionState.operations already returns one. I kept it: the copy is
what pins the point in time, and an existing test proved that removing
it lets a provider returning a live mapping leak later mutations into
an already-reported hook. It is a shallow pointer copy and now only
runs when plugins are registered.

Marks operations and updated_operations experimental, matching the
payload fields.

Adds unit tests for error isolation, the no-plugin gate and snapshot
pinning, plus e2e coverage under tests/e2e/ for the start and end maps
and UpdatedOperationIds across a real suspend/replay pair.

Refs #617
@wangyb-A
wangyb-A had a problem deploying to ai-pr-review-runtime August 11, 2026 22:29 — with GitHub Actions Error
@wangyb-A
wangyb-A temporarily deployed to ai-pr-review-runtime August 11, 2026 22:29 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@wangyb-A
wangyb-A had a problem deploying to ai-pr-review-runtime August 11, 2026 23:31 — with GitHub Actions Failure
@wangyb-A
wangyb-A temporarily deployed to ai-pr-review-runtime August 11, 2026 23:31 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

Addresses the remaining review comments on #629, and reverses my own
earlier design decision.

The lazy Mapping view produced three separate findings in a row -- an
embedded lock made the info undeepcopyable, a captured closure made it
unpicklable, and asdict() does not recurse through arbitrary Mappings so
values stayed OperationInfo instances. Each fix added machinery to make
a custom Mapping behave like a dict in a public dataclass field.

Replace it with a plain dict built eagerly, matching OperationChangeInfo
in the same module, which never had any of these problems. asdict() now
recurses to nested dicts, pickle round-trips, deepcopy works, and the
point-in-time guarantee is exact rather than argued.

The trade-off: a plugin-registered execution now converts the map at
both hooks even if nothing reads it. Executions with no plugins still
pay nothing thanks to the existing gate, and serializability is the
whole point of these fields, so paying the conversion is the right side
of the trade.

Also corrects the operations docstring: the initial execution state
already carries the EXECUTION operation, so the map is not empty on a
first invocation -- callers should use is_first_invocation.

Tests: asdict yields nested dicts, pickle round-trips for populated and
empty maps, and conversion happens at hook time.

Refs #617
@wangyb-A
wangyb-A temporarily deployed to ai-pr-review-runtime August 12, 2026 21:36 — with GitHub Actions Inactive
@wangyb-A
wangyb-A temporarily deployed to ai-pr-review-runtime August 12, 2026 21:36 — with GitHub Actions Inactive
if not self._plugins or operations_provider is None:
return {}
try:
return _to_operation_info_map(operations_provider())

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

[P2] Defer conversion until the map is read. This executes at both invocation hooks whenever any plugin is registered, so existing plugins that never access the new fields still allocate every OperationInfo and copied error twice per invocation. Large histories can consume substantial Lambda time and memory. Snapshot the raw operation mapping at hook time, then expose a lazy, cached, copy/pickle-safe view that converts entries only on access.

@github-actions

Copy link
Copy Markdown
Contributor

Codex AI review

One medium-priority performance regression remains. Tests cover map semantics but not operation-heavy executions where plugins ignore these fields.

Reviewed commit 6d51dc7af491b9d03bc599068a282c61aba44c21. Workflow run

@github-actions

Copy link
Copy Markdown
Contributor

Claude AI review

No actionable findings. The change adds InvocationInfo.operations and InvocationStartInfo.updated_operations to the plugin invocation hooks, and it holds up under review:

  • Correctness/determinism: operations_provider=lambda: execution_state.operations reads under lock and returns a fresh copy; ExecutionState.close() (which runs before on_invocation_end) stops/joins but never clears the operation map, so the end-hook re-read correctly reports end-of-invocation state and the start snapshot correctly pins start state. These are observability-only signals, so re-reading at end is safe with respect to replay semantics.
  • Isolation: _copy_error clones only the mutable stack_trace list (other ErrorObject fields are immutable), preventing a plugin from altering the error later raised to user code. OperationInfo is frozen, so map entries are otherwise safe to share.
  • API compatibility: the new fields are kw-only with empty-dict defaults and repr=False, compare=False, hash=False, so equality/hash/repr of existing infos are unchanged and no operation payloads leak into instrumentation that logs infos wholesale (verified against the OTel invocation plugin and the example plugin). New hook parameters are keyword-defaulted and the sole core caller was updated.
  • Gating/error handling: _snapshot_operation_infos returns {} when there are no plugins or no provider and swallows provider exceptions with a logged empty-map fallback, so the plugin-facing view can never break an execution.
  • Tests: unit + e2e coverage is thorough (subset semantics, missing-id skip, plugin-free path, provider release, snapshot pinning, dict/pickle/asdict/deepcopy serializability, error-mutation isolation, suspend/replay via UpdatedOperationIds).

Residual test risk: (1) the PR description and the TestInvocationInfoCopySafety docstrings describe a "lazy view / __deepcopy__" design, but the shipped code builds the map eagerly via _to_operation_info_map and stores a plain dict; code and tests are self-consistent (eager), so this is only stale prose, not a defect — worth cleaning up to avoid confusing future maintainers. (2) The author notes an acknowledged pre-existing conformance flake (10-22, PluginOperationChangeShape) and that end-to-end conformance re-verification depends on handlers living in a separate PR, so the live suite has not been re-run against this rebased branch.

Reviewed commit 6d51dc7af491b9d03bc599068a282c61aba44c21. Workflow run

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.

Plugin invocation hook infos missing operations and updated-operations maps

2 participants