feat(workflow): align history propagation API with go-sdk#1825
feat(workflow): align history propagation API with go-sdk#1825nelson-parente wants to merge 7 commits into
Conversation
Cassie (durabletask-go author) flagged the .NET surface for cross-SDK divergence post-merge of dotnet-sdk#1802 / dapr#1818. This rewrites the public history-propagation API to match the go-sdk shape — same one the python-sdk just adopted (python-sdk#1047). Issue dotnet-sdk#1801 was closed before her review; this PR delivers what the issue originally described. Three concrete gaps closed: 1. Activity-level opt-in (was missing entirely) - PropagationScope moved from ChildWorkflowTaskOptions to base WorkflowTaskOptions; ChildWorkflowTaskOptions inherits it. - WithHistoryPropagation() extension method added on the base record. - scheduleTaskAction.HistoryPropagationScope is now wired in WorkflowOrchestrationContext.CallActivityInternalAsync so activities can opt into propagation, matching CallChildWorkflowInternalAsync. - Without this, the Go SDK's reference example (SettlePayment activity using PropagateOwnHistory) literally cannot be ported to .NET. 2. Read API rewritten as high-level resolvers (was lossy FilterBy* + a PropagatedHistoryEvent record that dropped input/output/failure payloads) - PropagatedHistory.FilterByAppId/InstanceId/WorkflowName removed. - PropagatedHistory now exposes GetWorkflows(), GetWorkflowsByName(), GetLastWorkflowByName(), GetAppIds(), GetWorkflowsByAppId(), GetWorkflowsByInstanceId(). - New WorkflowResult class with InstanceId/AppId/Name plus GetActivitiesByName(), GetLastActivityByName(), GetChildWorkflowsByName(), GetLastChildWorkflowByName() — mirrors durabletask-go's GetLastWorkflowByName / GetLastActivityByName / GetLastChildWorkflowByName renames from durabletask-go#105. - New ActivityResult record carries Name, Started, Completed, Failed, Input, Output, FailureDetails — matching the Go/Python equivalents so chain-of-custody patterns line up. - New ChildWorkflowResult record with the equivalent shape. 3. Event payload preserved internally (was discarded by ConvertChunk) - ConvertChunk in WorkflowOrchestrationContext now parses raw events, walks them to resolve TaskScheduled <-> TaskCompleted/Failed and ChildWorkflowInstanceCreated <-> ChildWorkflowInstanceCompleted/ Failed by scheduleId, and produces fully-populated ActivityResult / ChildWorkflowResult instances. SDK retries reuse TaskExecutionId so matching is on scheduleId (matching Go/Python semantics). - Public API does not leak the proto HistoryEvent type — resolution happens at construction time inside Dapr.Workflow. Additional surface additions: - PropagationNotFoundException for missing-name lookups (mirrors Python's PropagationNotFoundError / Go's error returns). - Static WorkflowHistory.PropagateLineage() / PropagateOwnHistory() factory helpers for go-sdk call-site parity. Removed (clean break — 1.18 unreleased): PropagatedHistoryEntry, PropagatedHistoryEvent, HistoryEventKind, FilterByAppId, FilterByInstanceId, FilterByWorkflowName. Tests: - WorkflowHistoryPropagationTests.cs rewritten end-to-end to cover the new resolvers, query helpers, factory helpers, activity-level scope wiring, and child-workflow-level scope wiring. - HistoryPropagationWorkflowTests.cs (integration) updated to use GetWorkflows().Count in place of Entries.Count. Refs: dapr#1801, dapr/durabletask-go#105, dapr/go-sdk#823, dapr/python-sdk#1047 Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
…ignment - Document the `new`-hiding contract on ChildWorkflowTaskOptions .WithHistoryPropagation and add a regression test that asserts the returned type is ChildWorkflowTaskOptions (not the base record), so InstanceId survives the with-expression. - Add the standard `()`, `(string)`, and `(string, Exception)` constructors on PropagationNotFoundException so callers can wrap inner exceptions. - Alias StringValue alongside the existing Timestamp alias in WorkflowOrchestrationContext so the propagation helper signature stays consistent with the rest of the file. Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
Renames the test fixtures in GetPropagatedHistory_PreservesChunkOrder so the variable order matches the documented oldest-first chunk ordering (index 0 is the oldest ancestor, the last chunk is the immediate parent). No behavior change. Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
protoc unwraps google.protobuf.StringValue to a plain string in the generated C# (only the wire codec uses the wrapper). The StringValueOrNull(StringValue?) helper added in this branch expected the wrapper type, breaking the build with CS1503 at the three call sites in ResolveActivity / ResolveChildWorkflow. Drop the helper and pass the generated string fields straight through — they are already nullable at runtime and ActivityResult/ChildWorkflowResult accept string? for Input/Output. Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
Same StringValue mismatch as the production fix — protoc-generated
properties for google.protobuf.StringValue fields are plain string,
not the wrapper. Drop the new StringValue { Value = ... } wrappers
in the test helpers.
Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
|
@nelson-parente I'm not inclined to accept the type name changes. For types that are exclusively used for workflow history propagation, it makes sense that they'd reflect that in the name. For example, there's no such thing as a That said, completely agree about the activity propagation oversight. I'll review this later this afternoon for specific notes, but I wanted to call this out for posterity. |
WhitWaldo
left a comment
There was a problem hiding this comment.
Thank you for putting this PR together.
As I said in the earlier comment and repeat throughout, there's a distinction to me in terminology we use internally within the implementation of the project (in the gRPC protos, in the runtime) and the terminology we use in public-facing documentation. The protos refer to chunks and I've got no problem extending that to private methods, but most of this PR exposes public types with public-facing XML documentation - the names should be clear as to purpose and not introduce anything not present in publicly available Dapr documentation (like "chains" or "chunks"), especially when far more specific and particular names are already exposed in the implementation.
All of this work is scoped to history propagation - the names of these types should reflect this accordingly. It'd be one thing if you put them in a namespace like Dapr.Workflow.Abstractions.ProgagatedHistory and named them as such, but this isn't the long-standing Dapr .NET convention - most types are just left in the project root. So in this case, vague types like ActivityResult or ChildWorkflowResult just aren't clear. A developer shouldn't have to look at the XML documentation to understand that this isn't actually describing the returned result from a workflow or a child workflow - the name should be self-documenting, so I've proposed better alternatives like PropagatedHistoryActivityResult and PropagatedHistoryChildWorkflowResult.
The kind of event from the history was dropped in this implementation. Yes, it's not mentioned in #1801 but doesn't it provide some value to the user regarding what the event represents (e.g. activity started or completed)?
I certainly do appreciate you identifying the gap between supporting the activity propagation in addition to the existing workflow implementation.
Please do let me know if you have any follow-up questions.
There was a problem hiding this comment.
As I said in the comment, ActivityResult isn't helpful, I don't think, because users provide their own types for activity results. Rather, this would be more aptly named as a PropagatedHistoryActivityResult.
There was a problem hiding this comment.
Done — renamed to PropagatedHistoryActivityResult in b604220.
| /// <remarks> | ||
| /// Mirrors the <c>ChildWorkflowResult</c> type in the Go and Python SDKs. | ||
| /// </remarks> | ||
| public sealed record ChildWorkflowResult( |
There was a problem hiding this comment.
Again, workflows (child or not) return user-defined types, so this isn't helpfully named. As the original implementation didn't have activity support, I think there's a great opportunity here to opt for a rename from the original and instead call this PropgatedHistoryChildWorkflowResult.
I'm not terribly enthused by including "Child" in the type name as a workflow is a workflow whether it has a parent or not - that's just a provenance question. In other words, if a child workflow doesn't do something distinctly different from any other workflow, I don't entirely follow why we would call it something else (and I extend this argument to the "Detached Workflows" - they're just workflows that don't have a parent).
But I digress - in the interest of uniformity to the gRPC prototype descriptions, I'm fine calling this PropgatedHistoryChildWorkflowResult.
There was a problem hiding this comment.
Done — renamed to PropagatedHistoryChildWorkflowResult in b604220.
| /// <param name="name">The name of the ancestor workflow.</param> | ||
| /// <param name="activities">Activities resolved from this chunk, in execution order.</param> | ||
| /// <param name="childWorkflows">Child workflows resolved from this chunk, in execution order.</param> | ||
| public WorkflowResult( |
There was a problem hiding this comment.
I'd rather see this implemented using a primary constructor for conciseness and SDK uniformity as nothing is happening in the constructor outside of validation.
There was a problem hiding this comment.
Done — PropagatedHistoryEntry now uses a primary constructor.
There was a problem hiding this comment.
As in other classes, let's not introduce vague new terminology like "chunks" or "chains" in public-facing XML documentation when we already have terms that accurately and more obviously describe what they actually refer to. Such descriptions might be perfectly fine in internal implementations as they reflect usage from the gRPC protos themselves, but this is a public type and there should be nothing in here that doesn't have an analog directly to the publicly accessible Dapr runtime documentation.
There was a problem hiding this comment.
Done — public XML scrubbed of "chunks" and "chains" across the abstractions.
There was a problem hiding this comment.
The protos describe the type of history event returned. Is this not something meaningful to provide to the user?
There was a problem hiding this comment.
Lifecycle stage is captured by Started / Completed / Failed booleans on each result record — covers pending (Started && !Completed && !Failed) and matches go-sdk / python-sdk. HistoryEventKind removed.
| bool Failed, | ||
| string? Input, | ||
| string? Output, | ||
| WorkflowTaskFailureDetails? FailureDetails); |
There was a problem hiding this comment.
No interest in reflecting what type of event was recorded here? It's in the protos.
There was a problem hiding this comment.
Captured via Started / Completed / Failed booleans rather than a separate event-kind enum, matching go-sdk ActivityResult and python-sdk.
| bool Completed, | ||
| bool Failed, | ||
| string? Output, | ||
| WorkflowTaskFailureDetails? FailureDetails); |
There was a problem hiding this comment.
No interest in reflecting what type of event was recorded here? It's in the protos.
There was a problem hiding this comment.
Same as PropagatedHistoryActivityResult — Started / Completed / Failed cover the lifecycle without a separate enum, matching go-sdk / python-sdk.
| catch (InvalidProtocolBufferException) | ||
| { | ||
| continue; | ||
| // Skip malformed events; a single bad event cannot poison the chunk. |
There was a problem hiding this comment.
This seems like something the SDK should absolutely throw on if the runtime is throwing bad events to the SDK, right? Not just swallow the exception and pretend nothing is wrong here.
There was a problem hiding this comment.
Done — the try / catch around HistoryEvent.Parser.ParseFrom is gone; malformed event bytes now surface as exceptions. See b604220.
Addresses Whit's review on dapr#1825: - Rename ActivityResult -> PropagatedHistoryActivityResult - Rename ChildWorkflowResult -> PropagatedHistoryChildWorkflowResult - Rename WorkflowResult -> PropagatedHistoryEntry (primary constructor) - Drop WorkflowHistory static class; callers pass HistoryPropagationScope directly - Switch GetLast*ByName to TryGet*ByName + drop PropagationNotFoundException - Drop chunk/chain terminology from public XML docs - Surface malformed propagated event bytes via InvalidProtocolBufferException instead of silently skipping - Update unit tests to new names and TryGet asserts Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
Test names previously embedded the old type names (ActivityResult, ChildWorkflowResult); rename to the more general Activity_/ChildWorkflow_ prefix to avoid confusion with the renamed public types. Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
There was a problem hiding this comment.
Pull request overview
Aligns the .NET workflow history propagation API with the Go/Python SDK shape by (1) enabling propagation scope on activity scheduling and (2) upgrading the read-side propagated history model from “minimal events” to typed activity/child-workflow results reconstructed from raw history events.
Changes:
- Added
PropagationScopetoWorkflowTaskOptionsand wired it into the activity scheduling (ScheduleTaskAction.HistoryPropagationScope) path. - Reworked propagated history domain types to expose workflow entries with resolved
ActivitiesandChildWorkflows(including input/output/failure details), removing the older “event-kind” model. - Updated unit/integration tests to validate the new surface and behavior (including malformed raw-event handling).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Dapr.Workflow.Test/Worker/Internal/WorkflowHistoryPropagationTests.cs | Rewritten unit tests for new propagated history shape, activity/child resolution, and outbound propagation scope wiring. |
| test/Dapr.IntegrationTest.Workflow/HistoryPropagationWorkflowTests.cs | Updated integration test assertion to use the new GetWorkflows() API. |
| src/Dapr.Workflow/Worker/Internal/WorkflowOrchestrationContext.cs | Wires activity propagation scope into ScheduleTaskAction; reconstructs propagated history entries from raw events. |
| src/Dapr.Workflow.Abstractions/WorkflowTaskOptions.cs | Adds PropagationScope to base options + fluent WithHistoryPropagation; updates child options accordingly. |
| src/Dapr.Workflow.Abstractions/WorkflowContext.cs | Updates GetPropagatedHistory() docs to point to the new lookup helpers. |
| src/Dapr.Workflow.Abstractions/PropagatedHistory.cs | Replaces filter-based API with Get*/TryGet* navigation helpers and app/workflow/instance querying. |
| src/Dapr.Workflow.Abstractions/PropagatedHistoryEntry.cs | Changes from a record of minimal events into a class representing a workflow entry with typed activity/child results + lookup helpers. |
| src/Dapr.Workflow.Abstractions/PropagatedHistoryActivityResult.cs | Introduces typed activity result surfaced through propagated history. |
| src/Dapr.Workflow.Abstractions/PropagatedHistoryChildWorkflowResult.cs | Introduces typed child-workflow result surfaced through propagated history. |
| src/Dapr.Workflow.Abstractions/PropagatedHistoryEvent.cs | Removes the old minimal propagated event model. |
| src/Dapr.Workflow.Abstractions/HistoryEventKind.cs | Removes the old public event-kind enum. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses Whit's review on #1825: - Rename ActivityResult -> PropagatedHistoryActivityResult - Rename ChildWorkflowResult -> PropagatedHistoryChildWorkflowResult - Rename WorkflowResult -> PropagatedHistoryEntry (primary constructor) - Drop WorkflowHistory static class; callers pass HistoryPropagationScope directly - Switch GetLast*ByName to TryGet*ByName + drop PropagationNotFoundException - Drop chunk/chain terminology from public XML docs - Surface malformed propagated event bytes via InvalidProtocolBufferException instead of silently skipping - Update unit tests to new names and TryGet asserts Signed-off-by: Nelson Parente <nelson_parente@live.com.pt> Co-authored-by: nelson-parente <20144601+nelson-parente@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/b8e98abe-cc48-40ba-ac78-f1cd82018a30 Co-authored-by: nelson-parente <20144601+nelson-parente@users.noreply.github.com>
Summary
Aligns the workflow history propagation surface added in #1802 / #1818 with the canonical go-sdk shape — the same one python-sdk just adopted in python-sdk#1047. @cicoyle (durabletask-go author) flagged the divergence post-merge while building 1.18 quickstarts; this PR delivers what issue #1801 originally described.
This brings .NET in line for cross-SDK parity before 1.18 ships.
Three concrete gaps closed
1. Activity-level opt-in (was missing entirely)
Before this PR,
PropagationScopeonly lived onChildWorkflowTaskOptions— there was no way to propagate history when scheduling an activity. The go-sdk reference example uses exactly this pattern (SettlePaymentactivity withPropagateOwnHistory), so the cross-SDK quickstart literally couldn't be ported.PropagationScopemoved to baseWorkflowTaskOptions.WithHistoryPropagation()extension method available on bothWorkflowTaskOptionsandChildWorkflowTaskOptions.scheduleTaskAction.HistoryPropagationScopewired inWorkflowOrchestrationContext.CallActivityInternalAsync, matching the existing child-workflow path.2. Read API replaced (was lossy
FilterBy*+ minimal events)The old
PropagatedHistoryEventonly kept(EventId, Kind, Timestamp)— input/output/failure payloads were thrown away byConvertChunk, making the high-level audit/fraud-detection patterns from the Go/Python quickstarts impossible.PropagatedHistory.FilterByAppId/InstanceId/WorkflowName, the old minimalPropagatedHistoryEvent,HistoryEventKind(clean break — 1.18 unreleased).PropagatedHistory:GetWorkflows(),GetWorkflowsByName(),TryGetLastWorkflowByName(),GetAppIds(),GetWorkflowsByAppId(),GetWorkflowsByInstanceId().PropagatedHistoryEntryclass (one per ancestor workflow) withTryGetLastActivityByName(),GetActivitiesByName(),TryGetLastChildWorkflowByName(),GetChildWorkflowsByName()— mirrors theGetLast*ByNamerename merged in durabletask-go#105, adapted to the idiomatic .NETTryGet*pattern with[NotNullWhen(true)] outparameters.PropagatedHistoryActivityResult(Name, Started, Completed, Failed, Input, Output, FailureDetails)record — matches the Go SDK'sActivityResultstruct and Python'sActivityResultdataclass.PropagatedHistoryChildWorkflowResultwith the equivalent shape.WorkflowsFactoryregisters workflow/activity names and how the SDK matches app IDs elsewhere.3. Event payload preserved internally
ConvertChunknow walks the rawHistoryEvents, pre-indexesTaskCompleted/TaskFailed(and the child-workflow counterparts) byTaskScheduledIdin a single pass — so each scheduled activity or child resolves in O(1) — and produces fully-populatedPropagatedHistoryActivityResult/PropagatedHistoryChildWorkflowResultrecords. SDK retries reuseTaskExecutionId, so we match on the scheduling event ID (matching Go/Python semantics). The protoHistoryEventtype is never exposed publicly — resolution happens at construction time insideDapr.Workflow.Usage example (matches the go-sdk reference)
```csharp
// Parent workflow propagates lineage to a child workflow and own-history to an activity
var fraudResult = await context.CallChildWorkflowAsync(
nameof(FraudDetection),
input: req,
options: new ChildWorkflowTaskOptions()
.WithHistoryPropagation(HistoryPropagationScope.Lineage));
await context.CallActivityAsync(
nameof(SettlePayment),
input: req,
options: new WorkflowTaskOptions()
.WithHistoryPropagation(HistoryPropagationScope.OwnHistory));
// Receive side — query specific events by name with TryGet*
var history = context.GetPropagatedHistory();
if (history is not null &&
history.TryGetLastWorkflowByName("MerchantCheckout", out var merchant) &&
merchant.TryGetLastActivityByName("ValidateMerchant", out var validate))
{
if (validate.Completed) { /* inspect validate.Output */ }
}
```
Non-goals
ScheduleTaskAction.historyPropagationScopefield 6,CreateChildWorkflowAction.historyPropagationScopefield 6); we just had to wire the activity path.Breaking changes (1.18 unreleased)
The propagation API shipped in #1802/#1818 has not been released. This PR replaces it cleanly rather than carrying
[Obsolete]shims for an API that has no users yet. Removed types: the old minimalPropagatedHistoryEvent,HistoryEventKind,PropagatedHistory.FilterByAppId,FilterByInstanceId,FilterByWorkflowName. (PropagatedHistoryEntryis reused as the new per-ancestor entry type with a richer shape.)Test plan
WorkflowHistoryPropagationTests.csrewritten end-to-end: activity resolution (completed/failed/pending/retried), child-workflow resolution, history helpers (GetAppIds, TryGetLastWorkflowByName, GetWorkflowsByName, case-insensitive name/AppId lookups), scheduling option records (both base + derived), activity-level + child-workflow outbound action scope wiring.HistoryPropagationWorkflowTests.cs(integration) updated toGetWorkflows().Count.dotnet build+dotnet testto be verified by CI — couldn't build locally (no .NET 10 SDK on dev machine). Sweep confirmed no stale references to removed types insrc/ortest/.References
GetLast*ByNamerename: dapr/durabletask-go#105cc @cicoyle @WhitWaldo