From b7076fd0cfff053ffa59dc918f58833050403a56 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Thu, 21 May 2026 02:13:19 +0200 Subject: [PATCH] Address health check review feedback --- doc/wiki/health-checks.md | 2 + .../Extensions/HealthCheckExtensions.cs | 4 + .../ElsaDistributedLockHealthCheck.cs | 25 ++-- .../ElsaWorkflowPersistenceHealthCheck.cs | 123 +++++++++++------- .../ElsaReadinessHealthCheckOptions.cs | 5 + ...ElsaWorkflowPersistenceHealthCheckTests.cs | 39 +++++- .../HealthCheckExtensionsTests.cs | 19 +++ 7 files changed, 145 insertions(+), 72 deletions(-) diff --git a/doc/wiki/health-checks.md b/doc/wiki/health-checks.md index c2582c728..3fbb0b916 100644 --- a/doc/wiki/health-checks.md +++ b/doc/wiki/health-checks.md @@ -27,6 +27,8 @@ services .AddElsaReadinessChecks(includePersistence: false); ``` +Persistence probing stops after the first failed store by default to keep readiness responses bounded during dependency outages. Set `ContinuePersistenceProbesAfterFailure = true` when richer per-store diagnostics are more important than the fastest readiness response. + Map separate endpoints for liveness and readiness: ```csharp diff --git a/src/modules/Elsa.Workflows.Runtime/Extensions/HealthCheckExtensions.cs b/src/modules/Elsa.Workflows.Runtime/Extensions/HealthCheckExtensions.cs index a440a2cda..ea81ad17c 100644 --- a/src/modules/Elsa.Workflows.Runtime/Extensions/HealthCheckExtensions.cs +++ b/src/modules/Elsa.Workflows.Runtime/Extensions/HealthCheckExtensions.cs @@ -40,6 +40,10 @@ public static class HealthCheckExtensions if (configureOptions != null) optionsBuilder.Configure(configureOptions); + optionsBuilder + .Validate(x => x.DistributedLockAcquisitionTimeout > TimeSpan.Zero, "Distributed lock acquisition timeout must be greater than zero.") + .ValidateOnStart(); + builder.AddCheck("elsa-runtime", tags: ReadinessTags); if (includePersistence) diff --git a/src/modules/Elsa.Workflows.Runtime/HealthChecks/ElsaDistributedLockHealthCheck.cs b/src/modules/Elsa.Workflows.Runtime/HealthChecks/ElsaDistributedLockHealthCheck.cs index 5d5b32655..a532120b3 100644 --- a/src/modules/Elsa.Workflows.Runtime/HealthChecks/ElsaDistributedLockHealthCheck.cs +++ b/src/modules/Elsa.Workflows.Runtime/HealthChecks/ElsaDistributedLockHealthCheck.cs @@ -24,26 +24,17 @@ public class ElsaDistributedLockHealthCheck( var distributedLockProvider = serviceProvider.GetService(); if (distributedLockProvider == null) { - return HealthCheckResult.Degraded("Elsa distributed lock provider is not registered.", data: new Dictionary - { - ["category"] = "distributed-locks" - }); + return HealthCheckResult.Degraded("Elsa distributed lock provider is not registered.", data: CreateData()); } var lockName = $"elsa-health-check-{Guid.NewGuid():N}"; await using var handle = await distributedLockProvider.TryAcquireLockAsync(lockName, options.Value.DistributedLockAcquisitionTimeout, cancellationToken); if (handle == null) { - return HealthCheckResult.Degraded("Elsa distributed lock provider was reachable, but the probe lock was not acquired.", data: new Dictionary - { - ["category"] = "distributed-locks" - }); + return HealthCheckResult.Degraded("Elsa distributed lock provider was reachable, but the probe lock was not acquired.", data: CreateData()); } - return HealthCheckResult.Healthy("Elsa distributed lock provider is reachable.", new Dictionary - { - ["category"] = "distributed-locks" - }); + return HealthCheckResult.Healthy("Elsa distributed lock provider is reachable.", CreateData()); } catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { @@ -52,10 +43,12 @@ public class ElsaDistributedLockHealthCheck( catch (Exception e) when (!e.IsFatal()) { logger.LogWarning(e, "Elsa distributed lock provider is not reachable."); - return HealthCheckResult.Unhealthy("Elsa distributed lock provider is not reachable.", data: new Dictionary - { - ["category"] = "distributed-locks" - }); + return HealthCheckResult.Unhealthy("Elsa distributed lock provider is not reachable.", data: CreateData()); } + + static Dictionary CreateData() => new() + { + ["category"] = "distributed-locks" + }; } } diff --git a/src/modules/Elsa.Workflows.Runtime/HealthChecks/ElsaWorkflowPersistenceHealthCheck.cs b/src/modules/Elsa.Workflows.Runtime/HealthChecks/ElsaWorkflowPersistenceHealthCheck.cs index 41bd1046c..461e91e9f 100644 --- a/src/modules/Elsa.Workflows.Runtime/HealthChecks/ElsaWorkflowPersistenceHealthCheck.cs +++ b/src/modules/Elsa.Workflows.Runtime/HealthChecks/ElsaWorkflowPersistenceHealthCheck.cs @@ -2,16 +2,21 @@ using Elsa.Common; using Elsa.Workflows.Management; using Elsa.Workflows.Management.Filters; using Elsa.Workflows.Runtime.Filters; +using Elsa.Workflows.Runtime.Options; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Diagnostics.HealthChecks; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; namespace Elsa.Workflows.Runtime.HealthChecks; /// /// Performs small read-only probes against the workflow management and runtime stores. /// -public class ElsaWorkflowPersistenceHealthCheck(IServiceProvider serviceProvider, ILogger logger) : IHealthCheck +public class ElsaWorkflowPersistenceHealthCheck( + IServiceProvider serviceProvider, + IOptions options, + ILogger logger) : IHealthCheck { private const string ProbeId = "00000000-0000-0000-0000-000000000000"; @@ -19,45 +24,85 @@ public class ElsaWorkflowPersistenceHealthCheck(IServiceProvider serviceProvider public async Task CheckHealthAsync(HealthCheckContext context, CancellationToken cancellationToken = default) { // These probes verify store reachability only; returned entities and counts are intentionally ignored. - var probeResults = new List - { - await ProbeAsync("workflow-definitions", serviceProvider.GetService(), async (store, ct) => + var probeResults = new List(); + + if (!await AddProbeAsync("workflow-definitions", serviceProvider.GetService(), async (store, ct) => { await store.FindAsync(new WorkflowDefinitionFilter { Id = ProbeId }, ct); - }), - await ProbeAsync("workflow-instances", serviceProvider.GetService(), async (store, ct) => + })) + return CreateResult(); + + if (!await AddProbeAsync("workflow-instances", serviceProvider.GetService(), async (store, ct) => { await store.CountAsync(new WorkflowInstanceFilter { Id = ProbeId }, ct); - }), - await ProbeAsync("triggers", serviceProvider.GetService(), async (store, ct) => + })) + return CreateResult(); + + if (!await AddProbeAsync("triggers", serviceProvider.GetService(), async (store, ct) => { await store.FindAsync(new TriggerFilter { Id = ProbeId }, ct); - }), - await ProbeAsync("bookmark-queue", serviceProvider.GetService(), async (store, ct) => + })) + return CreateResult(); + + if (!await AddProbeAsync("bookmark-queue", serviceProvider.GetService(), async (store, ct) => { await store.FindAsync(new BookmarkQueueFilter { Id = ProbeId }, ct); - }) - }; + })) + return CreateResult(); - var attemptedProbes = probeResults.Where(x => !x.Skipped).Select(x => x.StoreName).ToList(); - var successfulProbes = probeResults.Where(x => !x.Skipped && x.Exception == null).Select(x => x.StoreName).ToList(); - var skippedProbes = probeResults.Where(x => x.Skipped).Select(x => x.StoreName).ToList(); - var failedProbes = probeResults.Where(x => x.Exception != null).Select(x => x.StoreName).ToList(); - var failedProbe = probeResults.FirstOrDefault(x => x.Exception != null); - if (failedProbe != null) + return CreateResult(); + + async Task AddProbeAsync(string storeName, TStore? store, Func probe) where TStore : class { - var data = CreateData(); - data["failedStore"] = failedProbe.StoreName; - data["failedProbe"] = failedProbe.StoreName; - - logger.LogWarning(failedProbe.Exception, "Elsa workflow store {StoreName} is not reachable.", failedProbe.StoreName); - return HealthCheckResult.Unhealthy($"Elsa workflow store '{failedProbe.StoreName}' is not reachable.", data: data); + var result = await ProbeAsync(storeName, store, probe); + probeResults.Add(result); + return result.Exception == null || options.Value.ContinuePersistenceProbesAfterFailure; } - var healthyData = CreateData(); - return attemptedProbes.Count == 0 - ? HealthCheckResult.Degraded("No Elsa workflow persistence stores are registered.", data: healthyData) - : HealthCheckResult.Healthy("Elsa workflow stores are reachable.", healthyData); + HealthCheckResult CreateResult() + { + var attemptedProbes = probeResults.Where(x => !x.Skipped).Select(x => x.StoreName).ToList(); + var successfulProbes = probeResults.Where(x => !x.Skipped && x.Exception == null).Select(x => x.StoreName).ToList(); + var skippedProbes = probeResults.Where(x => x.Skipped).Select(x => x.StoreName).ToList(); + var failedProbes = probeResults.Where(x => x.Exception != null).Select(x => x.StoreName).ToList(); + var failedProbe = probeResults.FirstOrDefault(x => x.Exception != null); + if (failedProbe != null) + { + var data = CreateData(); + data["failedStore"] = failedProbe.StoreName; + data["failedProbe"] = failedProbe.StoreName; + + logger.LogWarning(failedProbe.Exception, "Elsa workflow store {StoreName} is not reachable.", failedProbe.StoreName); + return HealthCheckResult.Unhealthy($"Elsa workflow store '{failedProbe.StoreName}' is not reachable.", data: data); + } + + var healthyData = CreateData(); + return attemptedProbes.Count == 0 + ? HealthCheckResult.Degraded("No Elsa workflow persistence stores are registered.", data: healthyData) + : HealthCheckResult.Healthy("Elsa workflow stores are reachable.", healthyData); + + Dictionary CreateData() + { + var data = new Dictionary + { + ["category"] = "persistence" + }; + + if (successfulProbes.Count > 0) + data["successfulProbes"] = string.Join(",", successfulProbes); + + if (attemptedProbes.Count > 0) + data["attemptedProbes"] = string.Join(",", attemptedProbes); + + if (failedProbes.Count > 0) + data["failedProbes"] = string.Join(",", failedProbes); + + if (skippedProbes.Count > 0) + data["skippedProbes"] = string.Join(",", skippedProbes); + + return data; + } + } async Task ProbeAsync(string storeName, TStore? store, Func probe) where TStore : class { @@ -80,28 +125,6 @@ public class ElsaWorkflowPersistenceHealthCheck(IServiceProvider serviceProvider return new ProbeResult(storeName, false, e); } } - - Dictionary CreateData() - { - var data = new Dictionary - { - ["category"] = "persistence" - }; - - if (successfulProbes.Count > 0) - data["probes"] = string.Join(",", successfulProbes); - - if (attemptedProbes.Count > 0) - data["attemptedProbes"] = string.Join(",", attemptedProbes); - - if (failedProbes.Count > 0) - data["failedProbes"] = string.Join(",", failedProbes); - - if (skippedProbes.Count > 0) - data["skippedProbes"] = string.Join(",", skippedProbes); - - return data; - } } private sealed record ProbeResult(string StoreName, bool Skipped, Exception? Exception); diff --git a/src/modules/Elsa.Workflows.Runtime/Options/ElsaReadinessHealthCheckOptions.cs b/src/modules/Elsa.Workflows.Runtime/Options/ElsaReadinessHealthCheckOptions.cs index 351531bbe..3f082fe8a 100644 --- a/src/modules/Elsa.Workflows.Runtime/Options/ElsaReadinessHealthCheckOptions.cs +++ b/src/modules/Elsa.Workflows.Runtime/Options/ElsaReadinessHealthCheckOptions.cs @@ -9,4 +9,9 @@ public class ElsaReadinessHealthCheckOptions /// The maximum time the distributed-lock readiness probe waits to acquire its probe lock. /// public TimeSpan DistributedLockAcquisitionTimeout { get; set; } = TimeSpan.FromSeconds(1); + + /// + /// Whether persistence readiness probing should continue after the first failed store probe. + /// + public bool ContinuePersistenceProbesAfterFailure { get; set; } } diff --git a/test/unit/Elsa.Workflows.Runtime.UnitTests/HealthChecks/ElsaWorkflowPersistenceHealthCheckTests.cs b/test/unit/Elsa.Workflows.Runtime.UnitTests/HealthChecks/ElsaWorkflowPersistenceHealthCheckTests.cs index 26c37cc95..a602d9a64 100644 --- a/test/unit/Elsa.Workflows.Runtime.UnitTests/HealthChecks/ElsaWorkflowPersistenceHealthCheckTests.cs +++ b/test/unit/Elsa.Workflows.Runtime.UnitTests/HealthChecks/ElsaWorkflowPersistenceHealthCheckTests.cs @@ -2,6 +2,7 @@ using Elsa.Workflows.Management; using Elsa.Workflows.Management.Filters; using Elsa.Workflows.Runtime.Filters; using Elsa.Workflows.Runtime.HealthChecks; +using Elsa.Workflows.Runtime.Options; using Microsoft.Extensions.Diagnostics.HealthChecks; using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; @@ -27,7 +28,7 @@ public class ElsaWorkflowPersistenceHealthCheckTests _serviceProvider.GetService(typeof(IWorkflowInstanceStore)).Returns(_workflowInstanceStore); _serviceProvider.GetService(typeof(ITriggerStore)).Returns(_triggerStore); _serviceProvider.GetService(typeof(IBookmarkQueueStore)).Returns(_bookmarkQueueStore); - _sut = new ElsaWorkflowPersistenceHealthCheck(_serviceProvider, NullLogger.Instance); + _sut = CreateSut(); } [Fact] @@ -37,7 +38,7 @@ public class ElsaWorkflowPersistenceHealthCheckTests Assert.Equal(HealthStatus.Healthy, result.Status); Assert.Equal("persistence", result.Data["category"]); - Assert.Equal("workflow-definitions,workflow-instances,triggers,bookmark-queue", result.Data["probes"]); + Assert.Equal("workflow-definitions,workflow-instances,triggers,bookmark-queue", result.Data["successfulProbes"]); Assert.Equal("workflow-definitions,workflow-instances,triggers,bookmark-queue", result.Data["attemptedProbes"]); await _workflowDefinitionStore.Received(1).FindAsync( Arg.Is(x => x.Id == "00000000-0000-0000-0000-000000000000"), @@ -64,7 +65,7 @@ public class ElsaWorkflowPersistenceHealthCheckTests } [Fact] - public async Task ReturnsUnhealthyWithFailedStoreWhenAStoreProbeFails() + public async Task ReturnsUnhealthyWithFailedStoreAndStopsProbingWhenAStoreProbeFails() { _triggerStore.FindAsync(Arg.Any(), Arg.Any()).Returns>(_ => throw new InvalidOperationException("store unavailable")); @@ -75,9 +76,25 @@ public class ElsaWorkflowPersistenceHealthCheckTests Assert.Equal("persistence", result.Data["category"]); Assert.Equal("triggers", result.Data["failedStore"]); Assert.Equal("triggers", result.Data["failedProbe"]); - Assert.Equal("workflow-definitions,workflow-instances,bookmark-queue", result.Data["probes"]); + Assert.Equal("workflow-definitions,workflow-instances", result.Data["successfulProbes"]); + Assert.Equal("workflow-definitions,workflow-instances,triggers", result.Data["attemptedProbes"]); + Assert.Equal("triggers", result.Data["failedProbes"]); + await _bookmarkQueueStore.DidNotReceive().FindAsync(Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task ReturnsUnhealthyWithAllProbeDataWhenContinuationIsEnabled() + { + var sut = CreateSut(continueAfterFailure: true); + _triggerStore.FindAsync(Arg.Any(), Arg.Any()).Returns>(_ => throw new InvalidOperationException("store unavailable")); + + var result = await sut.CheckHealthAsync(new HealthCheckContext()); + + Assert.Equal(HealthStatus.Unhealthy, result.Status); + Assert.Equal("workflow-definitions,workflow-instances,bookmark-queue", result.Data["successfulProbes"]); Assert.Equal("workflow-definitions,workflow-instances,triggers,bookmark-queue", result.Data["attemptedProbes"]); Assert.Equal("triggers", result.Data["failedProbes"]); + await _bookmarkQueueStore.Received(1).FindAsync(Arg.Any(), Arg.Any()); } [Fact] @@ -90,7 +107,7 @@ public class ElsaWorkflowPersistenceHealthCheckTests Assert.Equal(HealthStatus.Healthy, result.Status); Assert.Equal("persistence", result.Data["category"]); - Assert.Equal("triggers,bookmark-queue", result.Data["probes"]); + Assert.Equal("triggers,bookmark-queue", result.Data["successfulProbes"]); Assert.Equal("triggers,bookmark-queue", result.Data["attemptedProbes"]); Assert.Equal("workflow-definitions,workflow-instances", result.Data["skippedProbes"]); } @@ -109,7 +126,17 @@ public class ElsaWorkflowPersistenceHealthCheckTests Assert.Equal("No Elsa workflow persistence stores are registered.", result.Description); Assert.Equal("persistence", result.Data["category"]); Assert.Equal("workflow-definitions,workflow-instances,triggers,bookmark-queue", result.Data["skippedProbes"]); - Assert.False(result.Data.ContainsKey("probes")); + Assert.False(result.Data.ContainsKey("successfulProbes")); + } + + private ElsaWorkflowPersistenceHealthCheck CreateSut(bool continueAfterFailure = false) + { + var options = Microsoft.Extensions.Options.Options.Create(new ElsaReadinessHealthCheckOptions + { + ContinuePersistenceProbesAfterFailure = continueAfterFailure + }); + + return new ElsaWorkflowPersistenceHealthCheck(_serviceProvider, options, NullLogger.Instance); } private static async Task TrackProbeAsync(ProbeConcurrencyTracker tracker, T result) diff --git a/test/unit/Elsa.Workflows.Runtime.UnitTests/HealthChecks/HealthCheckExtensionsTests.cs b/test/unit/Elsa.Workflows.Runtime.UnitTests/HealthChecks/HealthCheckExtensionsTests.cs index 9033fb664..c2ba45795 100644 --- a/test/unit/Elsa.Workflows.Runtime.UnitTests/HealthChecks/HealthCheckExtensionsTests.cs +++ b/test/unit/Elsa.Workflows.Runtime.UnitTests/HealthChecks/HealthCheckExtensionsTests.cs @@ -22,6 +22,25 @@ public class HealthCheckExtensionsTests Assert.NotNull(serviceProvider.GetRequiredService>()); } + [Theory] + [InlineData(0)] + [InlineData(-1)] + public void AddElsaReadinessChecksRejectsNonPositiveDistributedLockTimeout(int timeoutMilliseconds) + { + var services = new ServiceCollection(); + + services + .AddHealthChecks() + .AddElsaReadinessChecks( + includePersistence: false, + includeDistributedLocks: true, + configureOptions: options => options.DistributedLockAcquisitionTimeout = TimeSpan.FromMilliseconds(timeoutMilliseconds)); + + using var serviceProvider = services.BuildServiceProvider(); + + Assert.Throws(() => serviceProvider.GetRequiredService>().Value); + } + [Fact] public void AddElsaReadinessChecksUsesElsaSpecificReadinessTag() {