Expose DispatchWorkflow child instance ID#7418
Expose DispatchWorkflow child instance ID#7418sfmskywalker with Copilot wants to merge 2 commits into
Conversation
Agent-Logs-Url: https://github.com/elsa-workflows/elsa-core/sessions/0bbd5643-3bbf-473d-b29e-c538763b6183 Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
Greptile SummaryThis PR exposes the generated child workflow instance ID as the activity Confidence Score: 4/5Safe to merge; the change is minimal and well-tested for the targeted path. Only P2 findings: the dual-semantics of the Result output (string ID vs child workflow object) could surprise callers, and the WaitForCompletion=true path silently omits the instance ID. No logic errors, security issues, or breaking changes detected. No files require special attention beyond the P2 documentation notes on DispatchWorkflow.cs.
|
| Filename | Overview |
|---|---|
| src/modules/Elsa.Workflows.Runtime/Activities/DispatchWorkflow.cs | Adds context.SetResult(instanceId) before CompleteActivityAsync() in the fire-and-forget branch, exposing the child workflow instance ID; the WaitForCompletion = true path is unchanged and does not expose the ID. |
| test/unit/Elsa.Activities.UnitTests/Composition/DispatchWorkflowTests.cs | New test file with two focused tests validating that the dispatched instance ID is both set as the activity result and forwarded to the dispatcher; only covers WaitForCompletion = false path. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ExecuteAsync] --> B{WaitForCompletion?}
B -- true --> C[DispatchChildWorkflowAsync\nreturns instanceId]
C --> D[CreateBookmark with\nDispatchWorkflowStimulus]
D --> E[OnChildWorkflowCompletedAsync\nResult = child workflow output]
B -- false --> F[DispatchChildWorkflowAsync\nreturns instanceId]
F --> G["context.SetResult(instanceId) NEW"]
G --> H[CompleteActivityAsync\nResult = instanceId string]
Comments Outside Diff (1)
-
src/modules/Elsa.Workflows.Runtime/Activities/DispatchWorkflow.cs, line 84-93 (link)Instance ID not exposed in
WaitForCompletion = truepathWhen
WaitForCompletion = true, the generatedinstanceIdis only stored in the bookmark stimulus and is never surfaced as an output. A caller who setsWaitForCompletion = trueand still wants to correlate the dispatch (e.g., for logging or timeout detection) cannot retrieve it. This is a documentation gap at minimum; if the intent is to limit this feature to fire-and-forget only, a comment to that effect would prevent future confusion.Prompt To Fix With AI
This is a comment left during a code review. Path: src/modules/Elsa.Workflows.Runtime/Activities/DispatchWorkflow.cs Line: 84-93 Comment: **Instance ID not exposed in `WaitForCompletion = true` path** When `WaitForCompletion = true`, the generated `instanceId` is only stored in the bookmark stimulus and is never surfaced as an output. A caller who sets `WaitForCompletion = true` and still wants to correlate the dispatch (e.g., for logging or timeout detection) cannot retrieve it. This is a documentation gap at minimum; if the intent is to limit this feature to fire-and-forget only, a comment to that effect would prevent future confusion. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/modules/Elsa.Workflows.Runtime/Activities/DispatchWorkflow.cs
Line: 97-98
Comment:
**Result type differs between modes**
When `WaitForCompletion = true`, `Result` is set to the child workflow's output (`context.WorkflowInput` — an arbitrary `object`) in `OnChildWorkflowCompletedAsync`. When `WaitForCompletion = false`, `Result` is now a `string` (the instance ID). Because the property is typed as `object`, this compiles fine, but callers must know which mode they're in to safely cast the result. Consider adding XML-doc to the inherited `Result` output (or a code comment here) clarifying the two distinct return semantics.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/modules/Elsa.Workflows.Runtime/Activities/DispatchWorkflow.cs
Line: 84-93
Comment:
**Instance ID not exposed in `WaitForCompletion = true` path**
When `WaitForCompletion = true`, the generated `instanceId` is only stored in the bookmark stimulus and is never surfaced as an output. A caller who sets `WaitForCompletion = true` and still wants to correlate the dispatch (e.g., for logging or timeout detection) cannot retrieve it. This is a documentation gap at minimum; if the intent is to limit this feature to fire-and-forget only, a comment to that effect would prevent future confusion.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Expose dispatch workflow instance id res..." | Re-trigger Greptile
| context.SetResult(instanceId); | ||
| await context.CompleteActivityAsync(); |
There was a problem hiding this comment.
Result type differs between modes
When WaitForCompletion = true, Result is set to the child workflow's output (context.WorkflowInput — an arbitrary object) in OnChildWorkflowCompletedAsync. When WaitForCompletion = false, Result is now a string (the instance ID). Because the property is typed as object, this compiles fine, but callers must know which mode they're in to safely cast the result. Consider adding XML-doc to the inherited Result output (or a code comment here) clarifying the two distinct return semantics.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/modules/Elsa.Workflows.Runtime/Activities/DispatchWorkflow.cs
Line: 97-98
Comment:
**Result type differs between modes**
When `WaitForCompletion = true`, `Result` is set to the child workflow's output (`context.WorkflowInput` — an arbitrary `object`) in `OnChildWorkflowCompletedAsync`. When `WaitForCompletion = false`, `Result` is now a `string` (the instance ID). Because the property is typed as `object`, this compiles fine, but callers must know which mode they're in to safely cast the result. Consider adding XML-doc to the inherited `Result` output (or a code comment here) clarifying the two distinct return semantics.
How can I resolve this? If you propose a fix, please make it concise.
Purpose
Expose the generated child workflow instance ID from fire-and-forget
DispatchWorkflowexecutions so parent workflows can correlate each dispatch with the created instance.Scope
Select one primary concern:
Description
Problem
DispatchWorkflowgenerated a child workflow instance ID internally but did not expose it whenWaitForCompletion = false, making repeated child dispatches hard to map back to parent loop iterations.Solution
Example usage:
Verification
Reviewers can verify this change by checking the new focused unit coverage.
Steps:
dotnet test test/unit/Elsa.Activities.UnitTests/Elsa.Activities.UnitTests.csproj --filter FullyQualifiedName~Elsa.Activities.UnitTests.Composition.DispatchWorkflowTests /p:RestoreIgnoreFailedSources=trueDispatchWorkflowreturns the generated child instance ID as its result.Expected outcome:
The parent workflow can read the dispatched child workflow instance ID from the
DispatchWorkflowactivity result.Screenshots / Recordings (if applicable)
Not applicable.
Commit Convention
We recommend using conventional commit prefixes:
fix:– Bug fixes (behavior change)feat:– New featuresrefactor:– Code changes without behavior changedocs:– Documentation updateschore:– Maintenance, tooling, or dependency updatestest:– Test additions or modificationsClear commit messages make reviews easier and history more meaningful.
Checklist