elsa-workflows / elsa-workflows/elsa-core
An exception thrown from a signal handler or an incident strategy faults the workflow and misattributes the incident
- Dominant language
- C#
- Stars
- 7.9k
- Forks
- 1.5k
- Avg merge
- 15h 22m
- Merged PRs (30d)
- 114
Description
Flagged as an explicit non-goal while implementing #7911 (PR #7913). It is not a regression from that change — the same shape exists today — but `FaultSignal` makes the path reachable from ordinary container code for the first time, so it is worth its own issue.
## What happens
`ExceptionHandlingMiddleware` (the activity-level one) does its work inside a `catch` block:
```csharp
catch (Exception e)
{
logger.LogWarning(e, "An exception was caught from a downstream middleware component");
context.Fault(e);
if (await context.TrySendSignalAsync(new FaultSignal(e, context)))
{
context.RecoverFromFault();
return;
}
await HandleIncidentAsync(context);
}
```
Nothing in that block is itself guarded. If `TrySendSignalAsync` throws — because a `FaultSignal` handler threw — or if `HandleIncidentAsync` throws, the second exception escapes the activity pipeline entirely and is caught by the workflow-level `ExceptionHandlingMiddleware`, which faults the whole workflow.
## Measured
Both cases, run through `IWorkflowRunner` with a container whose child faults:
| | Workflow | Incidents | Second incident attributed to |
| --- | --- | --- | --- |
| `FaultSignal` handler throws, `FaultStrategy` | `Finished/Faulted` | 2 | `Elsa.Workflow` |
| `FaultSignal` handler throws, `ContinueWithIncidentsStrategy` | `Finished/Faulted` | 2 | `Elsa.Workflow` |
| `IIncidentStrategy.HandleIncident` throws (no `FaultSignal` involved) | `Finished/Faulted` | 2 | `Elsa.Workflow` |
```
INCIDENTS: 2
- Elsa.Fault :: Elsa.Workflows.Exceptions.FaultException :: Exception of type 'FaultException' was thrown.
- Elsa.Workflow :: System.ApplicationException :: handler blew up
ctx Elsa.Workflow status=Running faultCount=1
ctx FaultHandlingContainer status=Running faultCount=1
ctx Elsa.Fault status=Faulted faultCount=1
```
The last row of the table is the important one: this is pre-existing behavior, not something #7911 introduced.
## Why it is worth fixing
Four things go wrong, in rough order of how hard they are to diagnose:
1. **The incident is misattributed.** The second incident is recorded against `Elsa.Workflow`, not against the container whose handler actually threw. An operator looking at the incident list cannot tell which handler is at fault, and in a workflow with several fault-handling containers there is nothing to narrow it down.
2. **The incident strategy is bypassed.** The workflow ends `Faulted` under `ContinueWithIncidentsStrategy` too. A host that deliberately configured "continue with incidents" silently does not get it, and the reason is invisible.
3. **The fault bookkeeping is left mid-flight.** `RecoverFromFault()` never runs, so `AggregateFaultCount` stays incremented on the faulting context and every ancestor, and the child stays `Faulted`. Those counts are persisted and surface through `ActivityExecutionRecord.AggregateFaultCount` and `ActivityExecutionStats`.
4. **The rest of the activity pipeline is skipped**, since the exception unwinds past `UseExecutionLogging` and `UseNotifications`.
Note the failure is silent in the sense that matters: nothing rethrows to the caller (`RunAsync` returns normally), and the workflow reports a plausible-looking faulted state. The only signal that a handler misbehaved is a second incident pinned to the wrong activity.
## Sketch of a fix
Not settled, and deliberately so — this issue is a report, not a spec. The obvious shape is to contain a secondary failure at the point it occurs and attribute it to the activity that actually caused it, rather than letting it unwind to the workflow root:
- Wrap the signal send and the incident-strategy call so a throw is recorded against the receiver that threw, then continue with the fallback the middleware would otherwise have taken. For a throwing `FaultSignal` handler that means treating the fault as unhandled and falling through to `HandleIncidentAsync`, which is the safe default: the handler did not demonstrably take responsibility, so it should not be credited with having done so.
- Decide what "propagation stopped, then threw" means. Stopping propagation is an assertion of responsibility; throwing immediately after arguably retracts it. Falling through to the incident strategy is the conservative reading.
- Whatever is chosen, `RecoverFromFault()` must still run exactly once or not at all — never a partial application of the fault bookkeeping, given the asymmetry documented on `FaultSignal`.
Worth also deciding whether a throwing incident strategy and a throwing signal handler should behave the same way. They are the same code path today by accident rather than by design.
## Scope
In: containment and attribution for exceptions thrown out of `TrySendSignalAsync` and `HandleIncidentAsync` in the activity-level `ExceptionHandlingMiddleware`, plus tests.
Out: anything about which faults a container should claim, which is #7911's business. Out: the workflow-level `ExceptionHandlingMiddleware`, unless the fix needs to touch it.
## Reproduction
The probe was a throwaway `FaultHandlingContainer` (from `test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/`) constructed with a handler that throws, and a two-line `IIncidentStrategy` that throws from `HandleIncident`. Both were run against `main` with PR #7913 applied and, for the incident-strategy case, without it. Not committed.
Contributor guide
Research direction
Start with the activity-level ExceptionHandlingMiddleware and the existing scenarios under test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/. Reproduce both throwing FaultSignal handler and IIncidentStrategy cases through IWorkflowRunner, then define and test containment, correct incident attribution, and fault bookkeeping while preserving the configured incident strategy behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100