fix(plugin): surface missing state fields on hook info records - #618
fix(plugin): surface missing state fields on hook info records#618wangyb-A wants to merge 4 commits into
Conversation
3907209 to
4f5639c
Compare
This comment has been minimized.
This comment has been minimized.
Claude AI reviewNo blocking findings. The diff is a mechanically consistent, compile-safe expansion of the preview ( Verified:
Residual test risk (this PR only): The behavioral additions in this PR are effectively unverified here. The only test changes are mechanical constructor-arity updates to existing OTel/unit tests plus one Note: the PR description references Reviewed commit |
4f5639c to
b42d0db
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Plugin hook infos dropped state that the Python and JS SDKs carry, so a Java plugin could not see per-operation replay status at the attempt and change hooks, nor operation state at the invocation hooks. Attempt and change-item fields: - UserFunctionStartInfo / UserFunctionEndInfo gain `isReplay`, the operation-level indicator for whether THIS operation was observed via checkpointed state. This is distinct from the existing `isReplayingChildren`, which describes the child operations of a context body and does not substitute for it. - OperationChangeItemInfo was a reduced record; it now carries the full operation surface (`status`, `attempt`, `isReplay`, `error`), ordered to match OperationEndInfo, so an operation seen through a change delta exposes the same fields as through the per-operation hooks. Invocation-info enrichment: - InvocationInfo gains `operations` and `updatedOperations`. - InvocationEndInfo gains `operations` and `executionStartTime`; the latter was present on the start info but dropped from the end record, forcing plugins to correlate back to the start hook. - `updatedOperations` derives from the input's UpdatedOperationIds intersected with the tracked operations, so it is empty on the first invocation and names the externally-completed operations on a replay. - The end-info snapshot is taken at end time, so unlike the start info it also includes operations created during the invocation. ExecutionManager now snapshots the operation ids delivered in the initial state and exposes non-mutating accessors for them. The attempt hook needs the replay indicator at its firing site, but getOperation() delegates to getOperationAndUpdateReplayState, which flips REPLAY to EXECUTION mode as a side effect; reading it from a plugin-hook site would mutate execution state. wasObservedAtInvocationStart is a pure containment check instead, and gives one consistent definition of `isReplay` across the attempt, change and invocation hooks. Both invocation maps are keyed by operation id and valued with OperationChangeItemInfo, now the richest operation snapshot record the SDK has, so a single conversion path feeds the change hook and both invocation hooks. The record name is a wart in this role; renaming it is left as a follow-up. These are positional records, so the added components surface every constructor call site. No defaulting overloads were introduced: a convenience constructor is exactly how a future internal call site would silently ship empty maps to plugins, which is the failure mode this change fixes. Call sites in the OpenTelemetry plugin tests were updated mechanically. Payload surfaces stay out of scope: no `result` and no execution input/result on any info. Verified with the full module build, unit tests and spotless:check. The conformance handlers that assert these field shapes, and the live suite results, are in the stacked follow-up PR. Refs: #604
30b89d3 to
ecc5856
Compare
This comment has been minimized.
This comment has been minimized.
| * @param identifier the operation identifier containing id, name, type, and subType | ||
| * @param parentId the parent operation ID (may be null) | ||
| * @param isReplay true if the user function is called during replay (context operations) | ||
| * @param isReplay true if this operation was already present in the checkpointed state when it started |
There was a problem hiding this comment.
Confusing. Can we find a better name and explain the behavior better?
There was a problem hiding this comment.
only keep isReplay to match js
There was a problem hiding this comment.
The interface hasn't released for js yet. We can rename for both Java and JS if we have a better name
There was a problem hiding this comment.
I think we can keep this name isReplay, since this apply to all operations, not limited to child context operation, do you have any suggession on this?
| * @param parentId the parent operation ID (may be null) | ||
| * @param isReplay true if the user function is called during replay (context operations) | ||
| * @param isReplay true if this operation was already present in the checkpointed state when it started | ||
| * @param isReplayingChildren true if the child operations of this context body are replaying from checkpoints |
There was a problem hiding this comment.
Confusing. What's the difference from above? Can we find a better name and explain the behavior better?
There was a problem hiding this comment.
only keep isReplay to match js
There was a problem hiding this comment.
Same as the above. isReplayingChildren could be renamed.
Codex AI reviewThree public plugin API regressions need correction. Residual risk: runtime tests were not executed under the read-only review constraint. Reviewed commit |
Summary
Plugin hook info records dropped state that the Python and JS SDKs carry, so a Java plugin could not see per-operation replay status at the attempt and change hooks, nor operation state at the invocation hooks.
Bottom of a 2-PR stack. The conformance handlers that assert these field shapes — and the live suite results — are in the stacked follow-up. This PR is the SDK change only and is reviewable on its own.
What changed
Execution payload surface (JS parity)
InvocationInfogainsexecutionInput: the deserialized input the execution was started with — the same value the durable handler receives.InvocationEndInfogainsexecutionInput(carried through from the start hook) andexecutionResult, the value the handler returned.executionResultis populated only when the invocation completed the execution successfully; null for PENDING/FAILED/RETRYING.Object, matching JS'sunknown. Note Python instead exposesexecution_resultas the serialized string from the invocation output; Java follows the JS shape and reports the returned object.Ordering change — please review. The execution input is now deserialized before
onInvocationStartfires, because the hook must carry it. Deserialization can fail, so the failure path explicitly fires the start hook with a nullexecutionInputbefore propagating: plugins must observe an invocation-start for every invocation, including one with a malformed input payload. A regression test covers this using aSerDesthat rejects every deserialize.Operation result (JS parity)
resultadded to the three operation-snapshot records:OperationInfo,OperationEndInfoandOperationChangeItemInfo.Stringto match JS'sresult?: string. The serialized form is deliberate — the plugin boundary has no access to the caller's target type, so it cannot deserialize. Null when the operation produced none (still running, failed, or a type carrying none such asWAIT).*Detailsmember per operation type, soBaseDurableOperation.getResultPayloaddispatches on operation type exactly as the existinggetErrorObjectdoes (STEP, CHAINED_INVOKE, CALLBACK, CONTEXT).Attempt and change-item fields
UserFunctionStartInfo/UserFunctionEndInfogainisReplay, the operation-level indicator for whether THIS operation was observed via checkpointed state. Distinct from the existingisReplayingChildren, which describes the child operations of a context body and does not substitute for it.OperationChangeItemInfowas a reduced record. It now carries the full operation surface (status,attempt,isReplay,error), ordered to matchOperationEndInfo, so an operation observed through a change delta exposes the same fields as through the per-operation hooks.Invocation-info enrichment
InvocationInfogainsoperationsandupdatedOperations.InvocationEndInfogainsoperationsandexecutionStartTime. The latter was present on the start info but dropped from the end record, forcing plugins to correlate back to the start hook.updatedOperationsderives from the invocation input'sUpdatedOperationIdsintersected with the tracked operations, so it is empty on the first invocation and names the externally-completed operations on a replay.Design notes for reviewers
Where the replay indicator comes from. The attempt hook needs the operation-level replay flag at its firing site in
BaseDurableOperation.runUserFunction, butgetOperation()delegates togetOperationAndUpdateReplayState, which flipsREPLAYtoEXECUTIONmode as a side effect. Reading it from a plugin-hook site would mutate execution state.ExecutionManagerinstead snapshots the operation ids delivered in the initial state once, andwasObservedAtInvocationStartis a pure containment check. That yields one consistent definition ofisReplayacross the attempt, change and invocation hooks: the operation predates this invocation.Map value type. Both invocation maps are keyed by operation id and valued with
OperationChangeItemInfo, now the richest operation snapshot record the SDK has, so a single conversion path (PluginInfoConverter.toOperationItemMap) feeds the change hook and both invocation hooks. The record's name is a wart in this role; renaming it to something likeOperationItemInfois a reasonable follow-up, kept out of this change to limit blast radius.No defaulting constructors. These are positional records, so the added components surface every constructor call site at compile time. I deliberately did not add convenience overloads: a defaulting constructor is exactly how a future internal call site would silently ship empty maps to plugins, which is the failure mode this fixes. Call sites in the OpenTelemetry plugin tests were updated mechanically. All touched APIs are
@Deprecatedpreview, so the record-shape changes are sanctioned.Out of scope
onOperationStartwithisReplay=truefor a pending WAIT) is a separate behavioral fix with OpenTelemetry span-model coupling. Not bundled here.Testing
Full module build, unit tests and
spotless:check: BUILD SUCCESS. SDK 1129 tests, integration 400, OpenTelemetry 165, examples 120 — all green. The first commit was additionally verified applied directly tomainwith no handler code present.Adds six
PluginIntegrationTestcases: execution input and result on success, no execution result when suspended, none when the execution fails, the start-hook-fires-on-malformed-input invariant, and operation-end reporting the serialized result on success versus no result plus an error on failure.Known flaky test, not caused by this PR:
InvocationOtelPluginTest.invocationEnd_closesNestedSpansChildFirst(a test added onmain) failed 3 times across ~12 full-reactor runs on this branch, and 0 times across 8 runs on pristinemain; it passes 6/6 in isolation on both. This PR changes no OpenTelemetry production code (git diff main HEAD -- otel-plugin/src/mainis empty) and the span close order inendOpenSpansChildFirstis deterministic, so the correlation looks like timing sensitivity in the test rather than a behaviour change. Flagging rather than hiding it.Conformance evidence lives in the stacked follow-up, where the handlers exist to assert these shapes: 21/21 covered plugin requirements pass, with 10-19, 10-21 and 10-22 flipping to green.
Refs: #604