Skip to content

Unwrap PortableValue before invoking edge Condition and fan-out Assigner - #708

Open
PratikDhanave wants to merge 2 commits into
microsoft:mainfrom
PratikDhanaveFork:unwrap-portablevalue-edge-condition-assigner
Open

Unwrap PortableValue before invoking edge Condition and fan-out Assigner#708
PratikDhanave wants to merge 2 commits into
microsoft:mainfrom
PratikDhanaveFork:unwrap-portablevalue-edge-condition-assigner

Conversation

@PratikDhanave

Copy link
Copy Markdown
Contributor

What

EdgeRunner.PrepareDeliveryForEdge passed the raw envelope.Message to the caller-supplied edge Condition (func(any) bool) and fan-out Assigner (func(int, any)). When the in-flight message is a workflow.PortableValue (e.g. a message delayed-deserialized after a checkpoint restore), a user delegate that type-asserts the concrete type sees the wrapper instead:

  • a predicate like m.(Order).Total > 100 fails its assertion, so the edge silently drops or misroutes the message;
  • an Assigner that type-asserts panics the superstep, since PrepareDeliveryForEdge has no recover().

This resolves the PortableValue to its declared runtime type once (via the existing messageRuntimeType) and threads the unwrapped value into both the Condition call and selectedTargetIDs for the Assigner. It falls back to the raw message whenever the runtime type cannot be resolved.

Why

This mirrors messageRouter.routeMessage, which already unwraps pvalue.As(info.runtimeType) before invoking a handler, and matches the .NET runtime, which unwraps maybeObj is PortableValue before invoking the edge delegate. Cross-SDK parity: the concrete message the user's edge delegates observe should not depend on whether the message happens to be carried as a PortableValue.

Tests

Added two black-box tests in the canonical edgerunner_test.go that build a MessageEnvelope whose Message is a PortableValue with a resolvable runtime type:

  • TestPrepareDeliveryForEdge_UnwrapsPortableValueForCondition — a direct edge whose Condition type-asserts the concrete type; asserts the delegate sees the concrete type and delivery happens.
  • TestPrepareDeliveryForEdge_UnwrapsPortableValueForAssigner — a fan-out edge whose Assigner type-asserts the concrete type; asserts routing to the selected sink.

Both fail before the change (Condition/Assigner see a PortableValue) and pass after. go build ./..., go vet, and go test ./workflow/... are green.

Copilot AI review requested due to automatic review settings July 24, 2026 02:39
@PratikDhanave
PratikDhanave requested a review from a team as a code owner July 24, 2026 02:39

Copilot AI left a comment

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.

Pull request overview

This PR fixes edge routing behavior in EdgeRunner.PrepareDeliveryForEdge so caller-supplied edge delegates (Condition and fan-out Assigner) observe the concrete message value rather than a workflow.PortableValue wrapper when the message’s runtime type can be resolved. This aligns edge-delegate behavior with the existing router unwrapping logic and improves cross-SDK parity.

Changes:

  • Add EdgeRunner.unwrapMessage to resolve workflow.PortableValue to its declared runtime type (when possible).
  • Invoke edge.Condition and edge.Assigner with the unwrapped message value (and thread it through target selection).
  • Add black-box tests verifying unwrapping for both Condition and Assigner.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
workflow/internal/execution/edgerunner.go Unwraps PortableValue before invoking edge Condition/Assigner, and updates target selection to use the unwrapped message.
workflow/internal/execution/edgerunner_test.go Adds tests asserting Condition/Assigner receive the concrete message when PortableValue runtime type is resolvable.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread workflow/internal/execution/edgerunner.go
@github-actions github-actions Bot added the parity-approved Go API consistency review found no parity issues label Jul 24, 2026
@github-actions

This comment has been minimized.

@PratikDhanave
PratikDhanave force-pushed the unwrap-portablevalue-edge-condition-assigner branch from 806a12f to 2294e30 Compare July 24, 2026 03:01
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

PrepareDeliveryForEdge passed the raw envelope.Message to the caller-supplied
Condition (func(any) bool) and fan-out Assigner (func(int, any)). When the
message is a workflow.PortableValue, a user delegate that type-asserts the
concrete type (e.g. m.(Order).Total > 100) sees the wrapper and fails: the
Condition drops or misroutes the message, and an Assigner assertion can panic
the superstep since PrepareDeliveryForEdge has no recover().

Resolve the PortableValue to its declared runtime type once (via
messageRuntimeType) and use the unwrapped value for both delegates, falling
back to the raw message when the type cannot be resolved. This mirrors
messageRouter.routeMessage and the .NET runtime, which unwrap PortableValue
before invoking the delegate.
@PratikDhanave
PratikDhanave force-pushed the unwrap-portablevalue-edge-condition-assigner branch from dab3656 to a54d3c0 Compare July 24, 2026 09:33
@github-actions

Copy link
Copy Markdown
Contributor

Parity Review: No Issues

This PR fixes a bug in workflow/internal/execution/edgerunner.go, an internal (unexported) package. No exported Go APIs or public contracts are changed.

Cross-repo parity: The change explicitly aligns Go behavior with the .NET runtime, which already unwraps PortableValue before invoking edge delegates. The fix also mirrors the existing messageRouter.routeMessage path in this same Go package. Semantics are now consistent across Go and .NET: user-supplied Condition and Assigner delegates always receive the concrete message type, regardless of whether the message was carried as a PortableValue after checkpoint restore.

Label assessment: No public-api-change label is needed (no exported API surface changed). The existing parity-approved label is correct and can remain.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

Generated by Go API Consistency Review Agent · 22.7 AIC · ⌖ 3.94 AIC · ⊞ 5.9K ·

@qmuntal

qmuntal commented Jul 26, 2026

Copy link
Copy Markdown
Member

Requesting changes for .NET semantic equivalence

The current implementation fixes the reported routing issue, but it does not reproduce .NET semantics.

.NET determines whether to unwrap from the delegate's declared generic type:

  • For concrete T, CreateConditionFunc<T>() and CreateTargetAssignerFunc<T>() convert PortableValue to T.
  • If conversion fails, the delegate receives default(T).
  • For T == object, conversion is intentionally skipped and the delegate receives the original PortableValue.

This PR instead unwraps inside EdgeRunner using the message's runtime type. Since Go's stored callbacks are func(any), this means an untyped callback receives the concrete value—the opposite of the equivalent .NET object behavior. The current tests therefore validate Go-specific semantics rather than .NET parity.

Exact parity cannot be implemented solely in EdgeRunner, because the declared callback type has already been erased. Please move conversion into typed callback adapters, for example package-level generic helpers such as:

  • AddDirectEdgeFor[T](...)
  • WithEdgeAssignerFor[T](...)

The adapters should:

  1. Preserve the original value when T is any.
  2. For concrete T, convert PortableValue using reflect.TypeFor[T]().
  3. Pass the converted value when successful.
  4. Pass zero(T) when conversion or type matching fails.
  5. Store the resulting adapter in the existing func(any) callback fields.

Raw AddDirectEdge(..., func(any) bool) and WithEdgeAssigner(func(int, any) ...) callbacks should continue receiving the original wrapper, matching .NET's T == object path.

Please add parity tests covering:

  • typed condition receives the concrete value;
  • typed assigner receives the concrete value;
  • any condition receives the original PortableValue;
  • any assigner receives the original PortableValue;
  • failed typed conversion supplies zero(T);
  • no delegate causes no conversion.

Without retaining the delegate's declared type through a typed adapter or equivalent metadata, this PR cannot provide .NET semantic equivalence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parity-approved Go API consistency review found no parity issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants