From 13eb48e002367be73db61d5d0f4ac182b8f4c34a Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Tue, 11 Aug 2026 01:11:25 +0200 Subject: [PATCH 1/3] feat(core): let a container handle a child's fault via FaultSignal A container activity had no way to learn that one of its children faulted. ExceptionHandlingMiddleware caught the exception, called context.Fault(e) and handed off to the workflow-global IIncidentStrategy; the container's completion callback never fired, because the child never completed. Add a seam on the ancestor-bubbling signal channel that already exists: - FaultSignal(Exception, ActivityExecutionContext), beside CancelSignal. Its XML doc carries the contract, including why a handler must not call RecoverFromFault and why the CompleteActivityAsync sweep is a backstop rather than the mechanism. - An internal bool-returning TrySendSignalAsync, since SignalContext .StopPropagationRequested is internal and SendSignalAsync reported nothing. SendSignalAsync keeps its public signature and delegates to it. - ExceptionHandlingMiddleware sends the signal after faulting and, when an ancestor stops propagation, calls RecoverFromFault once and returns instead of raising an incident. RecoverFromFault now transitions to Running only when the activity is still Faulted. It is called after the handler runs, so the unconditional transition would otherwise undo a handler that cancelled or completed the faulted child. The counts are still reset unconditionally, and the one pre-existing caller is unaffected. Behavior is unchanged when nobody handles the signal: verified by running the new unhandled-fault theory against the pre-change middleware, and by IncidentStrategyTests and Primitives/FaultTests passing unmodified. Refs #7911 Co-Authored-By: Claude Opus 5 --- .../ActivityExecutionContextExtensions.cs | 25 +- .../Activities/ExceptionHandlingMiddleware.cs | 13 +- .../Signals/FaultSignal.cs | 78 +++++++ .../Activities/FaultHandlingContainer.cs | 36 +++ .../FaultSignals/FaultSignalTests.cs | 217 ++++++++++++++++++ .../FaultHandlingTests.cs | 70 ++++++ .../TestHelpers.cs | 24 ++ 7 files changed, 460 insertions(+), 3 deletions(-) create mode 100644 src/modules/Elsa.Workflows.Core/Signals/FaultSignal.cs create mode 100644 test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/Activities/FaultHandlingContainer.cs create mode 100644 test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/FaultSignalTests.cs diff --git a/src/modules/Elsa.Workflows.Core/Extensions/ActivityExecutionContextExtensions.cs b/src/modules/Elsa.Workflows.Core/Extensions/ActivityExecutionContextExtensions.cs index 43c92034e..b39f8aa6c 100644 --- a/src/modules/Elsa.Workflows.Core/Extensions/ActivityExecutionContextExtensions.cs +++ b/src/modules/Elsa.Workflows.Core/Extensions/ActivityExecutionContextExtensions.cs @@ -221,6 +221,18 @@ public static partial class ActivityExecutionContextExtensions /// Send a signal up the current hierarchy of ancestors. /// public async ValueTask SendSignalAsync(object signal) + { + await activityExecutionContext.TrySendSignalAsync(signal); + } + + /// + /// Send a signal up the current hierarchy of ancestors and report whether a receiver claimed it. + /// + /// The signal to send. + /// + /// true if a receiver called ; otherwise, false. + /// + internal async ValueTask TrySendSignalAsync(object signal) { var receivingContexts = new[] { @@ -244,9 +256,11 @@ public static partial class ActivityExecutionContextExtensions if (signalContext.StopPropagationRequested) { logger.LogDebug("Propagation of signal {SignalType} on activity {ActivityId} of type {ActivityType} was stopped", signalTypeName, ancestorContext.Activity.Id, ancestorContext.Activity.Type); - return; + return true; } } + + return false; } /// @@ -335,6 +349,12 @@ public static partial class ActivityExecutionContextExtensions /// /// Recovers the current activity from a faulted state by resetting fault counts and transitioning the activity to the running status. /// + /// + /// This method is asymmetric with Fault: it sets the current context's fault count to zero, which is idempotent, but + /// decrements the count on every ancestor, which is not. Calling it more than once per fault drives ancestor counts negative. + /// The status transition only applies to an activity that is still , so that a + /// handler that already terminalized the activity does not see its decision undone. + /// public void RecoverFromFault() { activityExecutionContext.AggregateFaultCount = 0; @@ -344,7 +364,8 @@ public static partial class ActivityExecutionContextExtensions foreach (var ancestor in ancestors) ancestor.AggregateFaultCount--; - activityExecutionContext.TransitionTo(ActivityStatus.Running); + if (activityExecutionContext.Status == ActivityStatus.Faulted) + activityExecutionContext.TransitionTo(ActivityStatus.Running); } /// diff --git a/src/modules/Elsa.Workflows.Core/Middleware/Activities/ExceptionHandlingMiddleware.cs b/src/modules/Elsa.Workflows.Core/Middleware/Activities/ExceptionHandlingMiddleware.cs index e2237498c..49f240a55 100644 --- a/src/modules/Elsa.Workflows.Core/Middleware/Activities/ExceptionHandlingMiddleware.cs +++ b/src/modules/Elsa.Workflows.Core/Middleware/Activities/ExceptionHandlingMiddleware.cs @@ -1,5 +1,6 @@ using Elsa.Extensions; using Elsa.Workflows.Pipelines.ActivityExecution; +using Elsa.Workflows.Signals; using Microsoft.Extensions.Logging; namespace Elsa.Workflows.Middleware.Activities; @@ -16,7 +17,8 @@ public static class ExceptionHandlingMiddlewareExtensions } /// -/// Catches any exceptions thrown by downstream components and transitions the workflow into the faulted state. +/// Catches any exceptions thrown by downstream components and transitions the workflow into the faulted state, +/// unless an enclosing container claims the fault by handling the . /// public class ExceptionHandlingMiddleware(ActivityMiddlewareDelegate next, IIncidentStrategyResolver incidentStrategyResolver, ILogger logger) : IActivityExecutionMiddleware @@ -32,6 +34,15 @@ public class ExceptionHandlingMiddleware(ActivityMiddlewareDelegate next, IIncid { logger.LogWarning(e, "An exception was caught from a downstream middleware component"); context.Fault(e); + + // Give the ancestor chain a chance to handle the fault. See FaultSignal for the contract: the handler owns + // terminalizing the faulted activity, and recovering the fault bookkeeping is ours alone to do, exactly once. + if (await context.TrySendSignalAsync(new FaultSignal(e, context))) + { + context.RecoverFromFault(); + return; + } + await HandleIncidentAsync(context); } } diff --git a/src/modules/Elsa.Workflows.Core/Signals/FaultSignal.cs b/src/modules/Elsa.Workflows.Core/Signals/FaultSignal.cs new file mode 100644 index 000000000..f29072c0b --- /dev/null +++ b/src/modules/Elsa.Workflows.Core/Signals/FaultSignal.cs @@ -0,0 +1,78 @@ +namespace Elsa.Workflows.Signals; + +/// +/// Sent up the ancestor chain when an activity faulted, giving an enclosing container the chance to handle the failure +/// itself instead of letting it fall through to the workflow-global . +/// +/// +/// +/// A container opts in exactly the way it opts into cancellation, by registering a handler for this signal: +/// +/// +/// public MyContainer() +/// { +/// OnSignalReceived<FaultSignal>(OnChildFaultedAsync); +/// } +/// +/// private async ValueTask OnChildFaultedAsync(FaultSignal signal, SignalContext context) +/// { +/// if (!IsMine(signal.FaultedContext)) +/// return; // Not my child: let it keep bubbling. +/// +/// context.StopPropagation(); // I am handling this. +/// await signal.FaultedContext.CancelActivityAsync(); +/// await ScheduleFallbackAsync(context.ReceiverActivityExecutionContext); +/// } +/// +/// +/// Bubbling is the default. A handler that does not recognize simply returns without +/// stopping propagation, and the signal continues to the next ancestor, so nested containers compose without any of +/// them knowing about the others. If no ancestor stops propagation, the incident strategy runs exactly as it does +/// when this signal does not exist at all. +/// +/// +/// The contract. Responsibilities are split, and the split is deliberate: +/// +/// +/// +/// +/// The middleware calls Fault(), sends this signal, and — only when propagation was stopped — calls +/// RecoverFromFault() exactly once. Nothing else. +/// +/// +/// +/// +/// The handler decides the fault is its own, calls , and then +/// terminalizes the faulted activity: cancel it, complete it, or reschedule it. There is no single right answer +/// for the middleware to pick here — an interrupting error boundary wants the activity cancelled, a retry handler +/// wants it rescheduled, a fallback handler may want it completed with a substitute result — so a handler that +/// claims the failure also owns deciding what becomes of the failed work. +/// +/// +/// +/// +/// The handler must not call RecoverFromFault(). +/// +/// +/// +/// +/// That last rule is not stylistic. RecoverFromFault() is asymmetric: it sets the faulting context's +/// to zero, which is idempotent, but decrements the +/// count on every ancestor, which is not. A second call is therefore harmless for the faulting context and harmful for +/// every ancestor, driving their counts to -1. Those counts are persisted and surface through +/// ActivityExecutionRecord.AggregateFaultCount and ActivityExecutionStats, so the failure mode is +/// silently wrong fault numbers in the UI rather than an exception anyone would notice. +/// +/// +/// On the faulted activity's status. RecoverFromFault() transitions the activity out of +/// and back to , so a handler that stops +/// propagation and terminalizes nothing leaves the activity having already thrown. +/// That is the handler's bug. A completing container happens to sweep such an activity via +/// , which cancels its non-completed children, so the +/// mistake degrades rather than hangs — but that is a backstop, not the mechanism, and a container that suspends +/// instead of completing will persist the activity as . +/// +/// +/// The exception that caused the fault. +/// The of the activity that faulted. +public record FaultSignal(Exception Exception, ActivityExecutionContext FaultedContext); diff --git a/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/Activities/FaultHandlingContainer.cs b/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/Activities/FaultHandlingContainer.cs new file mode 100644 index 000000000..f05e6e34d --- /dev/null +++ b/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/Activities/FaultHandlingContainer.cs @@ -0,0 +1,36 @@ +using Elsa.Testing.Shared.Activities; +using Elsa.Workflows.Signals; + +namespace Elsa.Workflows.IntegrationTests.Scenarios.FaultSignals.Activities; + +/// +/// A sequential container that delegates handling to a caller-supplied strategy, so that a +/// single activity type covers every case under test: declining a fault, claiming it, and claiming it incorrectly. +/// +public class FaultHandlingContainer : TestContainer +{ + private readonly Func? _onChildFaulted; + + /// + /// Invoked for every fault bubbling through this container. Passing null declines every fault, which lets it + /// keep bubbling to the next ancestor. + /// + public FaultHandlingContainer(Func? onChildFaulted = null) + { + _onChildFaulted = onChildFaulted; + OnSignalReceived(OnChildFaultedAsync); + } + + /// + /// The number of faults this container was notified of. + /// + public int FaultsSeen { get; private set; } + + private async ValueTask OnChildFaultedAsync(FaultSignal signal, SignalContext context) + { + FaultsSeen++; + + if (_onChildFaulted != null) + await _onChildFaulted(signal, context); + } +} diff --git a/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/FaultSignalTests.cs b/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/FaultSignalTests.cs new file mode 100644 index 000000000..560cb9e8e --- /dev/null +++ b/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/FaultSignalTests.cs @@ -0,0 +1,217 @@ +using Elsa.Extensions; +using Elsa.Testing.Shared; +using Elsa.Testing.Shared.Activities; +using Elsa.Workflows.Activities; +using Elsa.Workflows.IncidentStrategies; +using Elsa.Workflows.IntegrationTests.Scenarios.FaultSignals.Activities; +using Elsa.Workflows.Models; +using Elsa.Workflows.Signals; +using Xunit.Abstractions; + +namespace Elsa.Workflows.IntegrationTests.Scenarios.FaultSignals; + +/// +/// Integration tests for : a container's opportunity to claim a child's fault before it +/// becomes a workflow-global incident. +/// +public class FaultSignalTests(ITestOutputHelper testOutputHelper) +{ + private readonly WorkflowTestFixture _fixture = new(testOutputHelper); + private readonly Fault _faultingActivity = Fault.Create("Whoops!", "Test", "Test"); + + [Fact(DisplayName = "A container that handles the signal suppresses the incident strategy")] + public async Task HandledFault_DoesNotFaultTheWorkflow() + { + // Arrange + var container = ContainerAround(_faultingActivity, ClaimAndCancelChildAsync); + + // Act + var result = await RunAsync(container); + + // Assert + Assert.Equal(1, container.FaultsSeen); + Assert.Equal(WorkflowStatus.Finished, result.WorkflowState.Status); + Assert.Equal(WorkflowSubStatus.Finished, result.WorkflowState.SubStatus); + + // The incident stays on record even though the fault was handled: caught failures remain visible to operators. + Assert.Single(result.WorkflowState.Incidents); + } + + [Theory(DisplayName = "A fault nobody handles is left to the incident strategy, exactly as before")] + [InlineData(typeof(FaultStrategy), WorkflowSubStatus.Faulted)] + [InlineData(typeof(ContinueWithIncidentsStrategy), WorkflowSubStatus.Suspended)] + public async Task UnhandledFault_LeavesIncidentStrategyInCharge(Type incidentStrategyType, WorkflowSubStatus expectedSubStatus) + { + // Arrange: a plain container, with no FaultSignal handler anywhere in the chain. + var container = new TestContainer + { + Activities = + { + _faultingActivity + } + }; + + // Act + var result = await RunAsync(container, incidentStrategyType); + + // Assert + Assert.Equal(expectedSubStatus, result.WorkflowState.SubStatus); + Assert.Single(result.WorkflowState.Incidents); + Assert.Equal(ActivityStatus.Faulted, result.GetActivityStatus(_faultingActivity)); + AssertFaultCounts(result, expected: 1); + } + + [Fact(DisplayName = "A fault the inner container declines keeps bubbling to the outer one")] + public async Task DeclinedFault_ReachesTheNextAncestor() + { + // Arrange + var inner = ContainerAround(_faultingActivity); + var outer = ContainerAround(inner, ClaimAndCancelChildAsync); + + // Act + var result = await RunAsync(outer); + + // Assert + Assert.Equal(1, inner.FaultsSeen); + Assert.Equal(1, outer.FaultsSeen); + Assert.Equal(WorkflowSubStatus.Finished, result.WorkflowState.SubStatus); + } + + [Fact(DisplayName = "A handled fault restores the fault count on the faulting context and every ancestor")] + public async Task HandledFault_RestoresFaultCounts() + { + // Arrange + var container = ContainerAround(_faultingActivity, ClaimAndCancelChildAsync); + + // Act + var result = await RunAsync(container); + + // Assert + AssertFaultCounts(result, expected: 0); + } + + [Fact(DisplayName = "A handler that also recovers from the fault drives ancestor fault counts negative")] + public async Task HandlerThatAlsoRecoversFromFault_CorruptsAncestorFaultCounts() + { + // Asserting the documented consequence of violating the contract rather than leaving it accidental: recovery + // *sets* the faulting context's count to zero, which is idempotent, but *decrements* every ancestor, which is + // not. The middleware recovers too, so the ancestors end up one below where they started. + + // Arrange + var container = ContainerAround(_faultingActivity, async (signal, context) => + { + context.StopPropagation(); + signal.FaultedContext.RecoverFromFault(); + await context.ReceiverActivityExecutionContext.CompleteActivityAsync(); + }); + + // Act + var result = await RunAsync(container); + + // Assert + var faultedContext = result.GetActivityContext(_faultingActivity); + Assert.NotNull(faultedContext); + + var ancestors = faultedContext.GetAncestors().ToList(); + Assert.NotEmpty(ancestors); + Assert.Equal(0, faultedContext.AggregateFaultCount); + Assert.All(ancestors, x => Assert.Equal(-1, x.AggregateFaultCount)); + } + + [Fact(DisplayName = "A handler that cancels the faulted child leaves it Canceled, not Running")] + public async Task HandlerThatCancelsChild_LeavesItCanceled() + { + // Arrange + var container = ContainerAround(_faultingActivity, ClaimAndCancelChildAsync); + + // Act + var result = await RunAsync(container); + + // Assert + Assert.Equal(ActivityStatus.Canceled, result.GetActivityStatus(_faultingActivity)); + } + + [Fact(DisplayName = "A handler that terminalizes nothing leaves the child Running and the workflow suspended")] + public async Task HandlerThatTerminalizesNothing_DegradesRatherThanHangs() + { + // The handler's bug, documented: recovery transitions the faulted child back to Running, and a handler that + // schedules no follow-up work moves it no further. The run returns rather than hanging, but the workflow + // suspends with nothing left to resume it, which is why terminalizing is the handler's responsibility. + + // Arrange + var container = ContainerAround(_faultingActivity, (_, context) => + { + context.StopPropagation(); + return default; + }); + + // Act + var result = await RunAsync(container); + + // Assert + Assert.Equal(ActivityStatus.Running, result.GetActivityStatus(_faultingActivity)); + Assert.Equal(WorkflowStatus.Running, result.WorkflowState.Status); + Assert.Equal(WorkflowSubStatus.Suspended, result.WorkflowState.SubStatus); + } + + [Fact(DisplayName = "A completing container sweeps a child the handler failed to terminalize")] + public async Task HandlerThatTerminalizesNothing_IsBackstoppedByTheCompletingContainer() + { + // The backstop, not the mechanism: CompleteActivityAsync cancels non-completed children on the way out. + + // Arrange + var container = ContainerAround(_faultingActivity, async (_, context) => + { + context.StopPropagation(); + await context.ReceiverActivityExecutionContext.CompleteActivityAsync(); + }); + + // Act + var result = await RunAsync(container); + + // Assert + Assert.Equal(ActivityStatus.Completed, result.GetActivityStatus(container)); + Assert.Equal(ActivityStatus.Canceled, result.GetActivityStatus(_faultingActivity)); + } + + /// + /// The canonical handler: claim the fault, terminalize the faulted child, and wind the container up. + /// + private static async ValueTask ClaimAndCancelChildAsync(FaultSignal signal, SignalContext context) + { + context.StopPropagation(); + await signal.FaultedContext.CancelActivityAsync(); + await context.ReceiverActivityExecutionContext.CompleteActivityAsync(); + } + + private static FaultHandlingContainer ContainerAround(IActivity child, Func? onChildFaulted = null) + { + return new(onChildFaulted) + { + Activities = + { + child + } + }; + } + + private Task RunAsync(IActivity root, Type? incidentStrategyType = null) + { + return _fixture.RunWorkflowAsync(new TestWorkflow(builder => + { + builder.WorkflowOptions.IncidentStrategyType = incidentStrategyType; + builder.Root = root; + })); + } + + private void AssertFaultCounts(RunWorkflowResult result, int expected) + { + var faultedContext = result.GetActivityContext(_faultingActivity); + Assert.NotNull(faultedContext); + + var ancestors = faultedContext.GetAncestors().ToList(); + Assert.NotEmpty(ancestors); + Assert.Equal(expected, faultedContext.AggregateFaultCount); + Assert.All(ancestors, x => Assert.Equal(expected, x.AggregateFaultCount)); + } +} diff --git a/test/unit/Elsa.Workflows.Core.UnitTests/Extensions/ActivityExecutionContextExtensions/FaultHandlingTests.cs b/test/unit/Elsa.Workflows.Core.UnitTests/Extensions/ActivityExecutionContextExtensions/FaultHandlingTests.cs index 8db759f1e..d8e15fe5f 100644 --- a/test/unit/Elsa.Workflows.Core.UnitTests/Extensions/ActivityExecutionContextExtensions/FaultHandlingTests.cs +++ b/test/unit/Elsa.Workflows.Core.UnitTests/Extensions/ActivityExecutionContextExtensions/FaultHandlingTests.cs @@ -36,4 +36,74 @@ public class FaultHandlingTests Assert.Equal(0, context.AggregateFaultCount); Assert.Equal(ActivityStatus.Running, context.Status); } + + [Fact] + public async Task Fault_IncrementsFaultCountOnEveryAncestor() + { + // Arrange + var chain = await CreateContextChainAsync(); + + // Act + chain[^1].Fault(new InvalidOperationException("Test error")); + + // Assert + Assert.Equal(new[] { 1, 1, 1 }, chain.Select(x => x.AggregateFaultCount)); + } + + [Fact] + public async Task RecoverFromFault_ResetsFaultCountOnEveryAncestor() + { + // Arrange + var chain = await CreateContextChainAsync(); + var faultedContext = chain[^1]; + faultedContext.Fault(new InvalidOperationException("Test error")); + + // Act + faultedContext.RecoverFromFault(); + + // Assert + Assert.Equal(new[] { 0, 0, 0 }, chain.Select(x => x.AggregateFaultCount)); + Assert.Equal(ActivityStatus.Running, faultedContext.Status); + } + + [Fact] + public async Task RecoverFromFault_CalledTwice_DrivesAncestorFaultCountsNegative() + { + // This is why a FaultSignal handler must not call RecoverFromFault: recovery *sets* the faulting context's + // count to zero, which is idempotent, but *decrements* every ancestor, which is not. The failure mode is + // silently wrong fault numbers rather than an exception, so it is asserted here rather than left accidental. + + // Arrange + var chain = await CreateContextChainAsync(); + var faultedContext = chain[^1]; + faultedContext.Fault(new InvalidOperationException("Test error")); + + // Act + faultedContext.RecoverFromFault(); + faultedContext.RecoverFromFault(); + + // Assert + Assert.Equal(0, faultedContext.AggregateFaultCount); + Assert.Equal(new[] { -1, -1 }, chain.Take(chain.Count - 1).Select(x => x.AggregateFaultCount)); + } + + [Fact] + public async Task RecoverFromFault_LeavesAlreadyTerminalizedStatusAlone() + { + // A FaultSignal handler terminalizes the faulted activity, and the middleware recovers the fault bookkeeping + // afterwards. Recovery must restore the counts without resurrecting the status the handler chose. + + // Arrange + var chain = await CreateContextChainAsync(); + var faultedContext = chain[^1]; + faultedContext.Fault(new InvalidOperationException("Test error")); + faultedContext.TransitionTo(ActivityStatus.Canceled); + + // Act + faultedContext.RecoverFromFault(); + + // Assert + Assert.Equal(ActivityStatus.Canceled, faultedContext.Status); + Assert.Equal(new[] { 0, 0, 0 }, chain.Select(x => x.AggregateFaultCount)); + } } diff --git a/test/unit/Elsa.Workflows.Core.UnitTests/Extensions/ActivityExecutionContextExtensions/TestHelpers.cs b/test/unit/Elsa.Workflows.Core.UnitTests/Extensions/ActivityExecutionContextExtensions/TestHelpers.cs index 81d04a34e..51be2fab9 100644 --- a/test/unit/Elsa.Workflows.Core.UnitTests/Extensions/ActivityExecutionContextExtensions/TestHelpers.cs +++ b/test/unit/Elsa.Workflows.Core.UnitTests/Extensions/ActivityExecutionContextExtensions/TestHelpers.cs @@ -19,4 +19,28 @@ public static class TestHelpers var fixture = new ActivityTestFixture(activity); return fixture.BuildAsync(); } + + /// + /// Creates a chain of nested activity execution contexts for testing ancestor-sensitive behavior. + /// + /// The number of contexts to create. Must be at least two. + /// The contexts, ordered from the outermost ancestor to the innermost descendant. + public static async Task> CreateContextChainAsync(int depth = 3) + { + // Every level is a Sequence so that a single registered activity type covers the whole chain. + var activities = Enumerable.Range(0, depth).Select(_ => new Sequence()).ToList(); + + for (var i = 0; i < depth - 1; i++) + activities[i].Activities.Add(activities[i + 1]); + + var contexts = new List + { + await CreateContextAsync(activities[0]) + }; + + for (var i = 1; i < depth; i++) + contexts.Add(await contexts[0].WorkflowExecutionContext.CreateActivityExecutionContextAsync(activities[i], new() { Owner = contexts[i - 1] })); + + return contexts; + } } From d085b536f0c9b0d1ae2c4f077ddd4465e38d86be Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Tue, 11 Aug 2026 03:04:49 +0200 Subject: [PATCH 2/3] docs(core): document FaultSignal self-receipt and pin it with a test Review raised that TrySendSignalAsync delivers to the faulting activity before walking ancestors, so an activity that throws and also handles FaultSignal can claim its own fault and suppress the incident strategy. That is real, but it is the channel's existing dispatch, which #7911 chose deliberately over a variant of it, and SignalContext.IsSelf exists so handlers can discriminate. It also grants no capability: an activity that catches its own exception never faults at all, ending Finished/Finished with zero incidents, which is a cleaner suppression than self-handling (incident still recorded, activity left Running, workflow suspended). So dispatch is unchanged. What was missing is that none of this was written down: the contract describes the handler as an enclosing container and never mentioned self-receipt. Document it on FaultSignal, including how a handler that wants ancestors-only semantics opts out, and add a test so the behavior is pinned rather than incidental. Refs #7911 Co-Authored-By: Claude Opus 5 --- .../Signals/FaultSignal.cs | 9 +++++++ .../SelfHandlingFaultingActivity.cs | 17 ++++++++++++ .../FaultSignals/FaultSignalTests.cs | 27 +++++++++++++++++++ 3 files changed, 53 insertions(+) create mode 100644 test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/Activities/SelfHandlingFaultingActivity.cs diff --git a/src/modules/Elsa.Workflows.Core/Signals/FaultSignal.cs b/src/modules/Elsa.Workflows.Core/Signals/FaultSignal.cs index f29072c0b..ff6305340 100644 --- a/src/modules/Elsa.Workflows.Core/Signals/FaultSignal.cs +++ b/src/modules/Elsa.Workflows.Core/Signals/FaultSignal.cs @@ -31,6 +31,15 @@ namespace Elsa.Workflows.Signals; /// when this signal does not exist at all. /// /// +/// The faulting activity is itself a receiver. SendSignalAsync delivers to the sender before walking its +/// ancestors, and this signal reuses that dispatch rather than introducing a variant of it. An activity that throws and +/// also handles therefore sees its own fault first, and may claim it, which is what a +/// self-retrying or self-compensating activity wants. This grants no ability to hide a failure that an activity did not +/// already have, since one that simply catches its own exception never faults at all. A handler that wants +/// ancestors-only semantics should check , or compare +/// against its own receiver context, the same way it already checks that the faulting context is one of its children. +/// +/// /// The contract. Responsibilities are split, and the split is deliberate: /// /// diff --git a/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/Activities/SelfHandlingFaultingActivity.cs b/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/Activities/SelfHandlingFaultingActivity.cs new file mode 100644 index 000000000..0ae5cd096 --- /dev/null +++ b/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/Activities/SelfHandlingFaultingActivity.cs @@ -0,0 +1,17 @@ +using Elsa.Workflows.Signals; + +namespace Elsa.Workflows.IntegrationTests.Scenarios.FaultSignals.Activities; + +/// +/// An activity that throws and also claims its own , used to pin the channel's self-receipt +/// behavior: a signal is delivered to its sender before the walk up the ancestor chain begins. +/// +public class SelfHandlingFaultingActivity : CodeActivity +{ + public SelfHandlingFaultingActivity() + { + OnSignalReceived((_, context) => context.StopPropagation()); + } + + protected override void Execute(ActivityExecutionContext context) => throw new InvalidOperationException("Whoops!"); +} diff --git a/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/FaultSignalTests.cs b/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/FaultSignalTests.cs index 560cb9e8e..6552a7dff 100644 --- a/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/FaultSignalTests.cs +++ b/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/FaultSignalTests.cs @@ -77,6 +77,33 @@ public class FaultSignalTests(ITestOutputHelper testOutputHelper) Assert.Equal(WorkflowSubStatus.Finished, result.WorkflowState.SubStatus); } + [Fact(DisplayName = "The faulting activity receives its own fault before any ancestor does")] + public async Task FaultingActivity_ReceivesItsOwnSignalFirst() + { + // Pinning the channel's self-plus-ancestors dispatch, which this signal reuses rather than varying. It lets a + // self-retrying or self-compensating activity claim its own failure, and grants no ability to hide a failure + // that an activity did not already have: one that simply catches its own exception never faults at all. + + // Arrange + var faultingActivity = new SelfHandlingFaultingActivity(); + var container = ContainerAround(faultingActivity); + + // Act + var result = await RunAsync(container, typeof(FaultStrategy)); + + // Assert: the activity claimed its own fault, so the walk never reached the container. + Assert.Equal(0, container.FaultsSeen); + Assert.NotEqual(WorkflowSubStatus.Faulted, result.WorkflowState.SubStatus); + + // The incident is still on record, and the fault bookkeeping was still recovered exactly once. + Assert.Single(result.WorkflowState.Incidents); + + var faultedContext = result.GetActivityContext(faultingActivity); + Assert.NotNull(faultedContext); + Assert.Equal(0, faultedContext.AggregateFaultCount); + Assert.All(faultedContext.GetAncestors(), x => Assert.Equal(0, x.AggregateFaultCount)); + } + [Fact(DisplayName = "A handled fault restores the fault count on the faulting context and every ancestor")] public async Task HandledFault_RestoresFaultCounts() { From a61833792378cd139eeec0bc04c236e6dbe63087 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Tue, 11 Aug 2026 22:08:44 +0200 Subject: [PATCH 3/3] docs(core): give FaultSignal handlers a working completion path Review caught that the contract advertised something that cannot work. It offered handlers three ways to terminalize the faulted activity - cancel, complete, or reschedule - but CompleteActivityAsync returns immediately unless the activity is Running, and throughout the handler it is still Faulted, since recovery runs only after the handler returns. Completing inline did nothing at all, silently, leaving the child Running. Measured, same container, handler completing the faulted child: inline complete -> child Running, no output, Running/Suspended TransitionTo(Running), complete -> child Completed, "after", Finished/Finished So a supported path exists; it just needed writing down. Document it on FaultSignal, note that it is not licence to call RecoverFromFault (which also rewrites the fault counts), and note that cancelling and rescheduling need no equivalent step. Cover it with an integration test asserting that completing the child with a substitute result fires the container's completion callback and resumes its sequencing. Refs #7911 Co-Authored-By: Claude Opus 5 --- .../Signals/FaultSignal.cs | 18 +++++++++++ .../FaultSignals/FaultSignalTests.cs | 32 +++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/src/modules/Elsa.Workflows.Core/Signals/FaultSignal.cs b/src/modules/Elsa.Workflows.Core/Signals/FaultSignal.cs index ff6305340..50c839117 100644 --- a/src/modules/Elsa.Workflows.Core/Signals/FaultSignal.cs +++ b/src/modules/Elsa.Workflows.Core/Signals/FaultSignal.cs @@ -65,6 +65,24 @@ namespace Elsa.Workflows.Signals; /// /// /// +/// Completing the faulted activity takes one extra step. CompleteActivityAsync returns immediately unless +/// the activity is , and throughout the handler it is still +/// , because recovery runs only once the handler has returned. Completing it inline +/// therefore does nothing at all, silently. A handler that wants to complete the activity — substituting a result for +/// the work that failed, say — has to move it out of the faulted state first: +/// +/// +/// context.StopPropagation(); +/// signal.FaultedContext.TransitionTo(ActivityStatus.Running); +/// await signal.FaultedContext.CompleteActivityAsync(substituteResult); +/// +/// +/// This is not licence to call RecoverFromFault(), which also rewrites the fault counts and remains the +/// middleware's job alone. Completing this way fires the enclosing container's completion callback, so the container's +/// normal sequencing resumes. Cancelling needs no equivalent step, because CancelActivityAsync already accepts a +/// faulted activity, and rescheduling needs none either. +/// +/// /// That last rule is not stylistic. RecoverFromFault() is asymmetric: it sets the faulting context's /// to zero, which is idempotent, but decrements the /// count on every ancestor, which is not. A second call is therefore harmless for the faulting context and harmful for diff --git a/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/FaultSignalTests.cs b/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/FaultSignalTests.cs index 6552a7dff..962e72a62 100644 --- a/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/FaultSignalTests.cs +++ b/test/integration/Elsa.Workflows.IntegrationTests/Scenarios/FaultSignals/FaultSignalTests.cs @@ -145,6 +145,38 @@ public class FaultSignalTests(ITestOutputHelper testOutputHelper) Assert.All(ancestors, x => Assert.Equal(-1, x.AggregateFaultCount)); } + [Fact(DisplayName = "A handler can complete the faulted child with a substitute result")] + public async Task HandlerThatCompletesChildWithSubstituteResult_ResumesTheContainer() + { + // Arrange + var container = new FaultHandlingContainer(async (signal, context) => + { + context.StopPropagation(); + + // CompleteActivityAsync no-ops on a non-Running activity, and the middleware recovers only once this + // handler has returned, so the faulted child has to be moved out of Faulted first. This is not the same as + // RecoverFromFault, which would also rewrite the fault counts. + signal.FaultedContext.TransitionTo(ActivityStatus.Running); + await signal.FaultedContext.CompleteActivityAsync("substitute"); + }) + { + Activities = + { + _faultingActivity, + new WriteLine("after") + } + }; + + // Act + var result = await RunAsync(container); + + // Assert: completing the child fires the container's completion callback, so sequencing resumes. + Assert.Equal(ActivityStatus.Completed, result.GetActivityStatus(_faultingActivity)); + Assert.Equal(new[] { "after" }, _fixture.CapturingTextWriter.Lines); + Assert.Equal(WorkflowSubStatus.Finished, result.WorkflowState.SubStatus); + AssertFaultCounts(result, expected: 0); + } + [Fact(DisplayName = "A handler that cancels the faulted child leaves it Canceled, not Running")] public async Task HandlerThatCancelsChild_LeavesItCanceled() {