feat(plugin): operation maps on invocation hooks - #629
Conversation
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
This comment has been minimized.
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
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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
| if not self._plugins or operations_provider is None: | ||
| return {} | ||
| try: | ||
| return _to_operation_info_map(operations_provider()) |
There was a problem hiding this comment.
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.
Codex AI reviewOne medium-priority performance regression remains. Tests cover map semantics but not operation-heavy executions where plugins ignore these fields. Reviewed commit |
Claude AI reviewNo actionable findings. The change adds
Residual test risk: (1) the PR description and the Reviewed commit |
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 toOperationInfo, on invocation-start and invocation-end. Mirrors JS, whereit sits on
InvocationBaseInfo.InvocationStartInfo.updated_operations— the subset named by the durableinvocation input's
UpdatedOperationIds, i.e. operations completed externallywhile 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 areunaffected.
Notable decisions
start-of-invocation snapshot. This matches JS, which overrides
operations: toOperationInfoMap(...)at everyonInvocationEndsite, and itis strictly more informative — the suspending invocation reports 1 operation
at start and 2 at end.
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
OperationInfoconversion is deferred and cached. A unittest pins this.
and idempotent, so a race can only duplicate work — and implements
__deepcopy__to materialize a plaindict. Without that, the lock madedataclasses.asdict(info)andcopy.deepcopy(info)raiseTypeError, andbecause 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.)
fields in the generated
__hash__would make a previously hashableInvocationStartInforaiseTypeError, and their entries carry operationresults and errors that instrumentation logs wholesale through
repr. fix(plugin): make payload fields additive on hook infos #627'stest caught this the moment these commits were rebased onto
main.is_replayedis leftFalseon map entries, matching JS'stoOperationInfoMap(isReplay: false) — these describe stored state, notreplay events.
Testing
hatch fmt --checkclean.applied: 23/23, exit 0 — 10-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; animmediate 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-processrepeats of the 10-22 handler satisfy its primary matcher. JS's handler for that
requirement is structurally identical, so it is likely cross-SDK.