elsa-workflows / elsa-workflows/elsa-core

An exception thrown from a signal handler or an incident strategy faults the workflow and misattributes the incident

Open
#7,914 1 comment 0 reactions 0 assignees View on GitHub
bug core elsa 3 triaged
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.