fix: Refactored the default activity invoker to handle exceptions#7426
fix: Refactored the default activity invoker to handle exceptions#7426ByronMayne wants to merge 2 commits into
Conversation
Before there was no exception handling so the behaviour of the activity execution middleware would change depending on if a fault was created by throwing or by using the Fault method.
Greptile SummaryThis PR refactors Confidence Score: 5/5Safe to merge — the core fault-handling change is well-scoped and backed by new unit tests; only minor P2 test quality issues remain. All findings are P2 (style/completeness): an unused import and a missing AggregateFaultCount assertion in one test method. No logic bugs or regressions were identified in the production code path. ActivityInvokerMiddlewareTestsBase.cs — unused import and incomplete assertion on the CanExecuteThrows fault path.
|
| Filename | Overview |
|---|---|
| src/modules/Elsa.Workflows.Core/Middleware/Activities/DefaultActivityInvokerMiddleware.cs | Adds try/catch around both CanExecuteAsync and ExecuteActivityAsync; exceptions now fault the activity at the call site consistently with direct context.Fault() usage. |
| src/modules/Elsa.Workflows.Core/Services/ActivityDescriber.cs | Guards GetFriendlyActivityName against IndexOf returning -1 for edge-case generic types; defensive fix, no behavioural regressions. |
| test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/ActivityExecutionMiddlewareTestsBase.cs | New base test fixture with TestActivity supporting throw/fault/cancel scenarios; good overall, some test property name typos flagged in prior review threads. |
| test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/ActivityInvokerMiddlewareTestsBase.cs | New abstract test base with coverage for exception-throw and direct-fault paths; unused System.ComponentModel import and missing AggregateFaultCount assertion on CanExecute path. |
| test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/DefaultActivityInvokerMiddlewareTests.cs | Minimal concrete test class wiring DefaultActivityInvokerMiddleware into the base test suite; looks correct. |
| test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/NotificationPublishingMiddlewareTests.cs | Tests that ActivityExecuting/ActivityExecuted notifications are raised; pipeline ordering (NotificationPublishingMiddleware → DefaultActivityInvokerMiddleware) is correct. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[InvokeAsync] --> B[EvaluateInputProperties]
B --> C{Cancellation Requested?}
C -- Yes --> D[TransitionTo Canceled / return]
C -- No --> E[CanExecuteAsync]
E -- throws OperationCanceled --> F[CancelActivityAsync / return]
E -- throws Exception --> G[Fault + HandleIncident / return]
E -- returns false --> H[TransitionTo Pending / return]
E -- returns true --> I[EnterExecution / TransitionTo Running]
I --> J[ExecuteActivityAsync]
J -- throws OperationCanceled --> K[CancelActivityAsync / return]
J -- throws Exception --> L[Fault + HandleIncident / return]
J -- returns normally --> M[Reset ExecuteDelegate / Auto-burn bookmark / IncrementExecutionCount]
M --> N[next context]
N --> O{activityDidComplete?}
O -- Yes --> P[Send ActivityCompleted]
O -- No --> Q[ShouldCommit check / done]
P --> Q
Prompt To Fix All With AI
This is a comment left during a code review.
Path: test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/ActivityInvokerMiddlewareTestsBase.cs
Line: 1-4
Comment:
**Unused import after removing `[Description]`**
`System.ComponentModel` is imported but nothing in this file uses it. If the `[Description]` attribute that prompted this import was intentionally removed, the `using` directive should be cleaned up too.
```suggestion
using Elsa.Mediator.Contracts;
using NSubstitute;
using Xunit.Abstractions;
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/ActivityInvokerMiddlewareTestsBase.cs
Line: 83-84
Comment:
**Missing `AggregateFaultCount` assertion in `CanExecute` fault test**
The parallel `ActivityExceuteThrows_ActivityStatus_IsFaulted` test asserts both `Status == Faulted` and `AggregateFaultCount == 1`, but this test only checks `Status`. Given the PR's goal is consistent fault handling across all execution paths, the `CanExecuteAsync` exception path should be verified to produce the same fault count.
```suggestion
Assert.Equal(ActivityStatus.Faulted, ExecutionContext.Status);
Assert.Equal(1, ExecutionContext.AggregateFaultCount);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "Did some changes based off the code revi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public Exception? ExectueThrows { get; set; } | ||
| public Exception? ExectueFaults { get; set; } | ||
| public Exception? CanExecuteThrows { get; set; } | ||
| public bool AutoComplete { get; set; } = false; |
There was a problem hiding this comment.
ExectueThrows, ExectueFaults, and ExectueThrows are misspellings of ExecuteThrows and ExecuteFaults. These names appear in test assertion messages and test setup code, making typo-induced bugs harder to spot.
| public Exception? ExectueThrows { get; set; } | |
| public Exception? ExectueFaults { get; set; } | |
| public Exception? CanExecuteThrows { get; set; } | |
| public bool AutoComplete { get; set; } = false; | |
| public Exception? ExecuteThrows { get; set; } | |
| public Exception? ExecuteFaults { get; set; } | |
| public Exception? CanExecuteThrows { get; set; } | |
| public bool AutoComplete { get; set; } = false; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/ActivityExecutionMiddlewareTestsBase.cs
Line: 26-29
Comment:
**Typos in test property names**
`ExectueThrows`, `ExectueFaults`, and `ExectueThrows` are misspellings of `ExecuteThrows` and `ExecuteFaults`. These names appear in test assertion messages and test setup code, making typo-induced bugs harder to spot.
```suggestion
public Exception? ExecuteThrows { get; set; }
public Exception? ExecuteFaults { get; set; }
public Exception? CanExecuteThrows { get; set; }
public bool AutoComplete { get; set; } = false;
```
How can I resolve this? If you propose a fix, please make it concise.| [Fact] | ||
| [Description("There was a bug where DefaultActivityInvoker was raising the ")] | ||
| public async Task ActivityExectue_RaiseNoEvents() | ||
| { | ||
| // Setup, force auto complete since that triggers the event | ||
| _activity.AutoComplete = true; | ||
|
|
||
| // Act | ||
| await Pipeline.ExecuteAsync(ExecutionContext); | ||
|
|
||
| // Assert | ||
| await _notificationSender | ||
| .DidNotReceive() | ||
| .SendAsync(Arg.Any<INotification>()); | ||
| } |
There was a problem hiding this comment.
[Description] and overly-broad assertion
The [Description] text is cut off mid-sentence. More importantly, the assertion DidNotReceive().SendAsync(Arg.Any<INotification>()) covers all notifications. DefaultActivityInvokerMiddleware sends an ActivityCompleted notification whenever activityDidComplete is true, which is the case here with AutoComplete = true. If the mock matches that call (same CancellationToken path), this test will fail. If it doesn't match (e.g. due to a CancellationToken value mismatch), the test passes vacuously without verifying the intended constraint. The description should be completed and the assertion scoped to the specific event types that shouldn't be raised by this middleware (e.g. ActivityExecuting / ActivityExecuted).
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/ActivityInvokerMiddlewareTestsBase.cs
Line: 33-47
Comment:
**Incomplete `[Description]` and overly-broad assertion**
The `[Description]` text is cut off mid-sentence. More importantly, the assertion `DidNotReceive().SendAsync(Arg.Any<INotification>())` covers *all* notifications. `DefaultActivityInvokerMiddleware` sends an `ActivityCompleted` notification whenever `activityDidComplete` is `true`, which is the case here with `AutoComplete = true`. If the mock matches that call (same `CancellationToken` path), this test will fail. If it doesn't match (e.g. due to a `CancellationToken` value mismatch), the test passes vacuously without verifying the intended constraint. The description should be completed and the assertion scoped to the specific event types that shouldn't be raised by this middleware (e.g. `ActivityExecuting` / `ActivityExecuted`).
How can I resolve this? If you propose a fix, please make it concise.| [Fact] | ||
| public async Task ActivityExceuteThrows_IncidentsCount_IsOne() | ||
| { | ||
| // Setup | ||
| _activity.ExectueThrows = new Exception("EXCEPTION!"); | ||
|
|
||
| // Act | ||
| await Pipeline.ExecuteAsync(ExecutionContext); | ||
|
|
||
| // Assert | ||
| Assert.Single(ExecutionContext.WorkflowExecutionContext.Incidents); | ||
| } | ||
|
|
||
| [Fact] | ||
| [Description("There was a bug where DefaultActivityInvoker was raising the ")] | ||
| public async Task ActivityExectue_RaiseNoEvents() | ||
| { | ||
| // Setup, force auto complete since that triggers the event | ||
| _activity.AutoComplete = true; | ||
|
|
||
| // Act | ||
| await Pipeline.ExecuteAsync(ExecutionContext); | ||
|
|
||
| // Assert | ||
| await _notificationSender | ||
| .DidNotReceive() | ||
| .SendAsync(Arg.Any<INotification>()); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task ActivityExceuteThrows_ActivityStatus_IsFaulted() | ||
| { | ||
| // Setup | ||
| _activity.ExectueThrows = new Exception("EXCEPTION!"); | ||
|
|
||
| // Act | ||
| await Pipeline.ExecuteAsync(ExecutionContext); | ||
|
|
||
| // Assert | ||
| Assert.Equal(ActivityStatus.Faulted, ExecutionContext.Status); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task ActivityCanExceuteThrows_IncidentsCount_IsOne() | ||
| { | ||
| // Setup | ||
| _activity.CanExecuteThrows = new Exception("EXCEPTION!"); | ||
|
|
||
| // Act | ||
| await Pipeline.ExecuteAsync(ExecutionContext); | ||
|
|
||
| // Assert | ||
| Assert.Single(ExecutionContext.WorkflowExecutionContext.Incidents); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task ActivityCanExceuteThrows_ActivityStatus_IsFaulted() | ||
| { | ||
| // Setup | ||
| _activity.CanExecuteThrows = new Exception("EXCEPTION!"); | ||
|
|
||
| // Act | ||
| await Pipeline.ExecuteAsync(ExecutionContext); | ||
|
|
||
| // Assert | ||
| Assert.Equal(ActivityStatus.Faulted, ExecutionContext.Status); | ||
| } | ||
|
|
There was a problem hiding this comment.
ExecuteFaults (direct-fault) path
TestActivity exposes an ExectueFaults property (which calls context.Fault() directly without throwing), but ActivityInvokerMiddlewareTestsBase only tests the exception-throwing paths. The PR's stated goal is to make the exception-thrown and direct-fault paths consistent, yet there is no test that verifies the incident count and status are correct when ExectueFaults is set. Without this, a regression on the fault path would go undetected.
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/ActivityInvokerMiddlewareTestsBase.cs
Line: 20-87
Comment:
**No test coverage for `ExecuteFaults` (direct-fault) path**
`TestActivity` exposes an `ExectueFaults` property (which calls `context.Fault()` directly without throwing), but `ActivityInvokerMiddlewareTestsBase` only tests the exception-throwing paths. The PR's stated goal is to make the exception-thrown and direct-fault paths *consistent*, yet there is no test that verifies the incident count and status are correct when `ExectueFaults` is set. Without this, a regression on the fault path would go undetected.
How can I resolve this? If you propose a fix, please make it concise.| { | ||
| // Act | ||
| await Pipeline.ExecuteAsync(ExecutionContext); | ||
|
|
||
| // Assert | ||
| await _notificationSender.Received().SendAsync(Arg.Any<ActivityExecuting>()); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task InvokeAsync_Rasies_ActivityExecutedEvent() | ||
| { | ||
| // Act | ||
| await Pipeline.ExecuteAsync(ExecutionContext); | ||
|
|
||
| // Assert | ||
| await _notificationSender.Received().SendAsync(Arg.Any<ActivityExecuted>()); |
There was a problem hiding this comment.
InvokeAsync_Rasies_ActivityExecutingEvent and InvokeAsync_Rasies_ActivityExecutedEvent contain the misspelling "Rasies" (should be "Raises"). While this doesn't affect test execution, it makes the test suite harder to read.
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/NotificationPublishingMiddlewareTests.cs
Line: 18-33
Comment:
**Typos in test method names**
`InvokeAsync_Rasies_ActivityExecutingEvent` and `InvokeAsync_Rasies_ActivityExecutedEvent` contain the misspelling "Rasies" (should be "Raises"). While this doesn't affect test execution, it makes the test suite harder to read.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Pull request overview
This PR addresses inconsistent activity lifecycle notifications when faults are produced via throw vs context.Fault(...) by handling activity exceptions inside DefaultActivityInvokerMiddleware (so upstream middleware like NotificationPublishingMiddleware can still complete its post-invocation behavior).
Changes:
- Catch and convert exceptions from
CanExecuteAsyncand activity execution into faults/incidents insideDefaultActivityInvokerMiddleware. - Add unit test infrastructure and initial tests for activity execution middleware behavior and notification publishing.
- Harden
ActivityDescribergeneric type friendly-name formatting to avoid substring issues.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/modules/Elsa.Workflows.Core/Middleware/Activities/DefaultActivityInvokerMiddleware.cs |
Handles exceptions at the invoker call site and routes them through incident handling instead of letting them bubble past notification middleware. |
src/modules/Elsa.Workflows.Core/Services/ActivityDescriber.cs |
Makes generic type name formatting safer by guarding the backtick index lookup. |
test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/ActivityExecutionMiddlewareTestsBase.cs |
Introduces a reusable pipeline-based test fixture for activity execution middleware. |
test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/ActivityInvokerMiddlewareTestsBase.cs |
Adds a reusable test suite for activity-invoker middleware behavior (faulting and incidents). |
test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/DefaultActivityInvokerMiddlewareTests.cs |
Hooks the standard invoker tests up to DefaultActivityInvokerMiddleware. |
test/unit/Elsa.Workflows.Core.UnitTests/Middleware/Activities/NotificationPublishingMiddlewareTests.cs |
Adds basic tests asserting ActivityExecuting/ActivityExecuted notifications are sent. |
| public Exception? ExectueThrows { get; set; } | ||
| public Exception? ExectueFaults { get; set; } | ||
| public Exception? CanExecuteThrows { get; set; } | ||
| public bool AutoComplete { get; set; } = false; |
There was a problem hiding this comment.
Several newly introduced identifiers appear to be misspelled (e.g., ExectueThrows / ExectueFaults). Since these are part of the shared test fixture API, consider renaming them to ExecuteThrows / ExecuteFaults to improve readability and avoid propagating typos into new tests.
Fixes: #7372
Refactored the DefaultActivityInvokerMiddleware to handle exceptions at the call site to make sure that the behaviour of the middleware remains consisten .
Purpose
Exceptions were only handled at the middleware level and the behaviour would change when faults happened depending on if the fault was created by throwing an exception or using the
Faultmethod. This change makes the middleware handle exceptions and fault in a consistent manorScope
Select one primary concern:
Description
Problem
The behaviour of Activity execution event would change depending on how faults are created
Solution
Exceptions are handled at the invocation level instead of depending on the middleware.
Verification
Run the unit tests which create an real pipeline and validate the results. They would fail before the fix was created.
Screenshots / Recordings (if applicable)
Checklist